Fix oversized revalidation prompt metadata#148
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed June 30, 2026, 4:15 PM ET / 20:15 UTC. Summary Reproducibility: yes. source-reproducible: current main embeds full finding and feature JSON in the revalidation prompt, and the PR regression shows the oversized metadata shape. I did not run the test because this review had to keep the checkout read-only. Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep the compact revalidation evidence approach, but hard-cap metadata list sizes independent of user file-context config and include redacted real terminal/log proof from an oversized revalidation run. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible: current main embeds full finding and feature JSON in the revalidation prompt, and the PR regression shows the oversized metadata shape. I did not run the test because this review had to keep the checkout read-only. Is this the best way to solve the issue? No, not yet: compacting metadata is the right direction, but file-reference metadata should be hard-capped independently of configurable review limits and the PR needs after-fix runtime proof. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 24763d26cdff. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c136c25f21
ℹ️ 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".
| const ownedLimit = config.review.maxOwnedFiles; | ||
| const contextLimit = config.review.maxContextFiles; | ||
| const testLimit = config.review.maxContextFiles; |
There was a problem hiding this comment.
Clamp revalidation metadata to a hard cap
When users raise review.maxContextFiles or review.maxOwnedFiles (the config schema only requires positive integers at src/types.ts:141-142), these values let the new evidence view serialize thousands of file refs and long reasons again. For example, maxContextFiles=2500 with 300-byte context reasons yields roughly 960 KB in the Feature JSON alone; adding the 120 KB file-context budget pushes the revalidation prompt back over the 1 MiB Codex ceiling this change is meant to avoid. Clamp these to a hard revalidation metadata cap instead of trusting review prompt limits.
Useful? React with 👍 / 👎.
c136c25 to
b9895f5
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9895f5302
ℹ️ 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".
| expect(prompt).toContain('"omittedAnalysisHistory": 37'); | ||
| expect(prompt).toContain('"omittedHistory": 35'); | ||
| expect(prompt).toContain("--- src/index.ts"); | ||
| expect(prompt).not.toContain("docs/context-2499.md"); |
There was a problem hiding this comment.
Fix the impossible context-path assertion
In this fixture the 2,500 docs/context-*.md files are never written, so rawFileBlock emits a short [unreadable] block for each; buildRevalidatePrompt still takes paths from fixPromptPaths using the raised maxContextFiles and only stops at the 120k current-file budget. Those unreadable docs plus the 200 test refs are only about 100k characters, so docs/context-2499.md remains in the Relevant current files section and this new test fails deterministically; either assert against the compact Feature JSON specifically or make the fixture exceed/avoid the file-context path budget.
Useful? React with 👍 / 👎.
|
Landed in Proof on exact PR head
Thanks @pai-scaffolde for the contribution. |
Summary
Maintainer repair
mainmaxOwnedFilesormaxContextFilesis raisedVerification
pnpm test src/prompt.test.ts— 15 passedpnpm test— 871 passed, 1 skippedpnpm typecheckpnpm lintpnpm format:checkpnpm build/Users/steipete/Projects/agent-skills/skills/autoreview/scripts/autoreview --mode branch --base origin/main— clean; no accepted/actionable findingsPublic Model Identifier Gate: PASS — the candidate adds no model identifiers; the only provider-bearing fixture value is the existing public provider name with a null model.