Repository navigation
milestone 12: add deterministic report cli slice - #12
Conversation
WalkthroughAdds a deterministic report engine and CLI that generate checklist-style reports from analysis output. Introduces a domain report builder (buildReport), new report-related types (ReportInput/ReportOutput/etc.), a report CLI (src/cli/report.ts), package.json scripts, test helpers for invoking the CLI, integration tests, e2e snapshot tests, snapshot fixtures, and a snapshot-generation script. The CLI supports JSON and human-readable output and validates input flags. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7023ac0006
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1 @@ | |||
| {"analyze":{"input":{"aspect_ratio":"0.7500","audio_bitrate_kbps":null,"audio_channels":null,"audio_codec":null,"audio_sample_rate_hz":null,"bitrate_kbps":null,"codec":null,"colorspace":"unknown","duration_seconds":null,"fps":0,"has_audio":false,"height":40,"orientation":"portrait","path":"/Users/jonas/repos/passepartout/tests/fixtures/images/portrait_sample_30x40.png","width":30},"selection":{"mode":"reliable","profile":"reliable_feed_portrait_safe","surface":"feed","target_resolution":"1080x1350","workflow":"unknown"},"tier":{"name":"tier_aspect_correction","reason":"Aspect ratio 0.7500 is outside supported feed bounds.","risk_level":"medium"},"white_canvas":{"contain_only":false,"enabled":false,"margins":null,"no_crop":false,"profile":null}},"checks":[{"id":"input_width_min","label":"Input width baseline","message":"Input width 30px is below baseline threshold (320px).","status":"warn"},{"id":"aspect_fit","label":"Aspect fit","message":"Input aspect is outside supported bounds for selected surface.","status":"warn"},{"id":"audio_present","label":"Audio track","message":"Still image input: audio track is not applicable.","status":"pass"},{"id":"codec_preference","label":"Codec preference","message":"Still image input: codec preference is not applicable.","status":"pass"}],"next_actions":["Use export-image for deterministic still export.","Review warning checks before upload."]} | |||
There was a problem hiding this comment.
This snapshot hard-codes analyze.input.path with a machine-specific root (/Users/jonas/...), but tests/e2e/report.snapshots.e2e.test.ts asserts exact string equality for the full JSON payload, so the new report e2e tests fail whenever the repo is checked out under a different path (for example /workspace/passepartout). Please scrub or canonicalize the path field before writing/validating snapshots so the tests are portable across environments.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/cli/report.ts`:
- Around line 13-79: In parseArgs, guard every flag that expects a value
(--mode, --surface, --workflow, --canvas-profile) by checking that the next
token exists and does not startWith("--") before assigning and advancing i; if
the check fails throw a clear error like "Missing value for --mode" (use
similarly named messages for each flag). Update the switch cases for "--mode",
"--surface", "--workflow", and "--canvas-profile" in the parseArgs function to
perform this validation prior to casting/assignment and incrementing i, so a
flag-by-itself produces a helpful error instead of consuming the next flag or
producing confusing behavior.
In `@tests/fixtures/e2e/generate-report-snapshots.ts`:
- Around line 28-39: The code only checks payload startsWith("{") before writing
snapshots, which can allow malformed JSON; modify the logic around payload (the
variable derived from proc.stdout and used in writeFileSync) to JSON.parse the
payload to validate it (catching and rethrowing a clear error that includes
testCase.id), and only call writeFileSync(join(snapshotDir,
`${testCase.id}.json`), `${payload}\n`, "utf8") after successful parsing; ensure
any parse errors include the payload and testCase.id for debugging rather than
silently writing invalid JSON.
In
`@tests/fixtures/e2e/snapshots/report/report-reliable-feed-landscape-white.json`:
- Line 1: The snapshot contains an absolute filesystem path in the JSON at the
analyze.input.path field which makes tests non-deterministic; update the report
generation or the snapshot test harness to normalize that value (e.g., replace
absolute paths with repo-relative paths or a fixed placeholder like "<repo>")
before writing or comparing snapshots, ensuring the normalization happens where
reports are serialized so tests using report-reliable-feed-landscape-white.json
no longer embed machine-specific paths.
In `@tests/fixtures/e2e/snapshots/report/report-reliable-feed-portrait-png.json`:
- Line 1: The snapshots include absolute paths produced by resolve() in
export_image.ts and export_video.ts which cause cross-machine test failures;
update the code that serializes the analyze/export output (where resolve() is
used to populate the "path" field) to convert absolute file paths to
repository-root-relative paths (or a stable normalized form) before JSON
serialization or before writing snapshots. Locate the use of resolve() in
export_image.ts and export_video.ts (and the same serialization helper used by
analyze/report generation), replace the direct absolute path assignment with a
normalization step that strips the CWD/repo root prefix (or calls a utility like
toRepoRelativePath(filePath)) so the snapshots store stable relative paths.
Ensure tests still receive valid relative paths and update any helper used
across report/export/export_video/analyze to avoid duplicating logic.
In `@tests/integration/report.integration.test.ts`:
- Around line 28-47: Add a new integration test mirroring the existing "audio
check passes for video with audio" test but using a silent/no-audio fixture file
to exercise the warning path; call runReportCli with that fixture (same flags:
"--mode", "reliable", "--surface", "reel", "--workflow", "unknown", "--json"),
parse the result with parseJsonStdout, locate the check by id "audio_present"
from the checks array, assert the check is defined and that audioCheck.status
equals "warn". Use the same test structure and helpers (runReportCli,
parseJsonStdout, fixtures) and name the test clearly (e.g., "audio check warns
for video without audio").
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
docs/phase1_knowledge.mdpackage.jsonsrc/cli/report.tssrc/domain/report.tssrc/types/contracts.tstests/e2e/report.snapshots.e2e.test.tstests/fixtures/e2e/generate-report-snapshots.tstests/fixtures/e2e/report_cases.jsontests/fixtures/e2e/snapshots/report/report-reliable-feed-landscape-white.jsontests/fixtures/e2e/snapshots/report/report-reliable-feed-portrait-png.jsontests/fixtures/e2e/snapshots/report/report-reliable-reel-portrait-mp4-audio.jsontests/helpers/cli.tstests/integration/report.integration.test.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/domain/report.ts (2)
src/types/contracts.ts (2)
ReportInput(166-166)ReportOutput(177-181)src/domain/analyze.ts (1)
analyze(6-38)
src/cli/report.ts (3)
src/types/contracts.ts (5)
ReportInput(166-166)Mode(1-1)Surface(2-2)Workflow(4-4)CanvasProfile(5-5)src/domain/report.ts (1)
buildReport(4-91)src/domain/recommend.ts (1)
stableStringify(104-106)
tests/e2e/report.snapshots.e2e.test.ts (1)
tests/helpers/cli.ts (1)
runReportCli(47-49)
tests/integration/report.integration.test.ts (1)
tests/helpers/cli.ts (2)
runReportCli(47-49)parseJsonStdout(51-63)
🪛 LanguageTool
docs/phase1_knowledge.md
[style] ~11-~11: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...input and tier classification output. - Deterministic report engine (report) layered on ana...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~49-~49: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...input videos in baseline export mode. - Added report integration/e2e snapshot tests a...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (15)
src/types/contracts.ts (1)
165-181: LGTM!The new type definitions are well-structured and follow consistent patterns with existing types in the file. Using
ReportInputas an alias forAnalyzeInputprovides flexibility for future divergence, and theReportCheckStatusunion type withReportCheckandReportOutputinterfaces properly define the report contract.src/domain/report.ts (1)
4-91: LGTM!The
buildReportfunction is well-structured with clear, deterministic logic:
- Check conditions are correctly evaluated (width baseline, aspect tier, audio presence, codec preference).
- The
as constassertions ensure proper type narrowing for status fields.- Next actions are conditionally built based on media type, warning presence, and white-canvas state.
- Return structure matches the
ReportOutputcontract.docs/phase1_knowledge.md (1)
11-11: LGTM!Documentation accurately reflects the new report engine functionality. The repetitive sentence beginnings flagged by static analysis are consistent with the existing document style (bullet lists with parallel structure), so no change needed.
Also applies to: 25-25, 49-49
package.json (1)
9-9: LGTM!The new script entries follow the established naming conventions and patterns in the project.
Also applies to: 24-24
tests/fixtures/e2e/report_cases.json (1)
1-14: LGTM!Test cases provide good coverage across different input types (PNG, MP4, JPG), surfaces (feed, reel), and features (white-canvas). The case IDs are descriptive and align with the snapshot filename convention.
tests/integration/report.integration.test.ts (1)
1-48: LGTM!The integration tests effectively validate the report CLI contract and audio check behavior. The test structure is clean and uses the helper functions appropriately.
tests/fixtures/e2e/generate-report-snapshots.ts (3)
1-15: Clean fixture setup and case loading.
The directory setup and case loading are straightforward and deterministic.
16-26: Process execution and failure handling look solid.
The sync spawn and non-zero exit handling are clear and actionable for fixture generation.
42-42: Nice completion log.
Useful for tracking how many fixtures were generated.tests/e2e/report.snapshots.e2e.test.ts (2)
1-14: Fixture wiring is clear and consistent.
The fixture/snapshot setup matches the generator layout cleanly.
15-30: E2E comparison logic is straightforward.
The per-scenario tests and last-line comparison are consistent with the snapshot generator.tests/helpers/cli.ts (2)
11-14: Command union extension is correct.
Adding "report" keeps the helper type-safe and consistent.
47-49: Wrapper is consistent with other CLI helpers.
Matches existing patterns and keeps call sites clean.src/cli/report.ts (2)
81-94: Human-readable output is clear and actionable.
The summary, checks, and next-actions formatting is easy to scan.
96-108: Main flow is concise and predictable.
JSON output vs. human output switch is straightforward.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
c0e0f82 to
7805f57
Compare
7023ac0 to
7082d52
Compare
Addressed/accepted for stacked merge.
1 participant
Summary
reportCLI slice built onanalyzeanalyzepayloadpass|warn)next_actionsVerification
Greptile Summary
Added deterministic
reportCLI command that layers checklist validation and next-action recommendations on top of the existinganalyzeengine. The implementation includes comprehensive integration and E2E snapshot tests with fixture tooling for deterministic verification.Key Changes
reportcommand with CLI argument parsing matchinganalyzeinput contractConfidence Score: 5/5
analyzefoundation, includes comprehensive test coverage (integration + E2E snapshots), uses proper TypeScript types, and maintains deterministic output contractImportant Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[CLI: report.ts] -->|parseArgs| B[Parse file + flags] B -->|ReportInput| C[buildReport] C -->|calls| D[analyze] D -->|inspectMedia| E[Media Inspector] D -->|recommend| F[Recommendation Engine] D -->|classifyTier| G[Tier Classifier] E -->|MediaInspection| H[AnalyzeOutput] F -->|target resolution| H G -->|TierOutput| H H -->|analyzed| C C -->|build checks| I{Check input_width} C -->|build checks| J{Check aspect_fit} C -->|build checks| K{Check audio_present} C -->|build checks| L{Check codec_preference} I -->|≥320px| M[pass] I -->|<320px| N[warn] J -->|tier != aspect_correction| M J -->|tier == aspect_correction| N K -->|has_audio or still| M K -->|no audio in video| N L -->|null or h264| M L -->|other codec| N M -->|status: pass| O[checks array] N -->|status: warn| O O -->|build actions| P{Determine next_actions} P -->|codec null| Q[Use export-image] P -->|codec exists| R[Use export-video] P -->|has warnings| S[Review warnings] P -->|white_canvas enabled| T[Confirm margins] Q --> U[next_actions array] R --> U S --> U T --> U O --> V[ReportOutput] U --> V H --> V V -->|json flag| W[JSON output] V -->|no json flag| X[Human output]Last reviewed commit: 7082d52