examples(pr-reviewer): keep the shell gate on the review file, and document why - #459
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reachedNext included review available in 51 seconds. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 282f9cb. Configure here.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 3 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| }) | ||
| // TODO: `.gate({ type: "artifact_exists", path: REVIEW_FILE })` once flows#434's follow-up lands. | ||
| .gate({ type: "subprocess_gate", command: `test -s ${REVIEW_FILE}` }); | ||
| .gate({ type: "artifact_exists", path: REVIEW_FILE }); |
There was a problem hiding this comment.
🔴 Hidden review artifact always fails
Every review gates on .workforce/review.md, but the artifact scanner skips .workforce. The gate always fails, so tests, comments, and fixes never run.
Learn more
The agent worker snapshots its working directory before and after execution. The scanner ignores every entry whose name starts with a dot, including the entire .workforce directory. Consequently, the journaled artifacts array can never contain .workforce/review.md, even when the agent created it successfully. The named gate reads only that array and exits unsuccessfully when the path is absent.
Example: The wrapper creates .workforce/review.md and exits zero. The post-run scan skips .workforce, journals no matching artifact, and review.gate fails before verification.
Recommended fix: Store the review in a path that snapshotWorkspaceFiles records, or extend artifact collection to support explicitly gated dot-directory paths without collecting runtime metadata. Keep scratch files excluded from staging and commits.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Agreed — this and the sibling threads show artifact_exists is the wrong gate for a dot-directory file with non-empty and rerun requirements. Reworked in 0ae7c8d: the swap is dropped, subprocess_gate (test -s) stays, and the TODO is replaced with the rationale in the flow and README.
| }) | ||
| // TODO: `.gate({ type: "artifact_exists", path: REVIEW_FILE })` once flows#434's follow-up lands. | ||
| .gate({ type: "subprocess_gate", command: `test -s ${REVIEW_FILE}` }); | ||
| .gate({ type: "artifact_exists", path: REVIEW_FILE }); |
There was a problem hiding this comment.
🟡 Empty review files pass validation
When review creates an empty file, artifact_exists passes because it checks only the journaled path. Tests can then authorize edits that have no review text.
Learn more
The replaced shell gate used test -s, which required the review file to exist and contain at least one byte. artifact_exists checks only whether the path appears in the worker's changed-file list. A newly created empty file appears in that list, so the new gate weakens the output contract before the flow reads, publishes, or acts on the review.
Example: A reviewer creates an empty review.md, modifies src/a.ts, and exits successfully. The gate passes, green tests permit the edit to be pushed, and the resulting review body contains no findings.
Recommended fix: Preserve both requirements: first verify the journaled artifact attribution, then deterministically validate that the review content is non-empty before tests or pushes. If the surface permits only one gate, add a following deterministic validation step or retain the non-empty subprocess gate until both checks can be expressed.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Agreed — this and the sibling threads show artifact_exists is the wrong gate for a dot-directory file with non-empty and rerun requirements. Reworked in 0ae7c8d: the swap is dropped, subprocess_gate (test -s) stays, and the TODO is replaced with the rationale in the flow and README.
| }) | ||
| // TODO: `.gate({ type: "artifact_exists", path: REVIEW_FILE })` once flows#434's follow-up lands. | ||
| .gate({ type: "subprocess_gate", command: `test -s ${REVIEW_FILE}` }); | ||
| .gate({ type: "artifact_exists", path: REVIEW_FILE }); |
There was a problem hiding this comment.
🟡 Repeated review falsely fails gate
When review rewrites identical text, artifact_exists fails because artifact detection records only content changes. The retained review file makes repeat runs stop despite valid output.
Learn more
The worker snapshots file content before the agent starts and journals only paths whose final content differs. This flow creates .workforce but never removes an existing review file before the agent step. Rewriting the same review therefore proves a write at the filesystem level but produces no journaled artifact, unlike the previous disk-existence gate.
Example: A first run leaves Looks fine.\nREADY\n in the review file. A synchronize run produces the same review; the final hash equals the initial hash, so the artifact list omits the path and the gate fails.
Recommended fix: Remove the prior review file in a deterministic step immediately before invoking review, or define a gate contract that journals write activity rather than final content changes. Ensure retries and resumes do not erase a completed step's output.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Agreed — this and the sibling threads show artifact_exists is the wrong gate for a dot-directory file with non-empty and rerun requirements. Reworked in 0ae7c8d: the swap is dropped, subprocess_gate (test -s) stays, and the TODO is replaced with the rationale in the flow and README.
Review swarm: maintainabilityNo fresh transcript was produced for run |
Review swarm: historyNo fresh transcript was produced for run |
Review swarm: structureNo fresh transcript was produced for run |
Review swarm: FAILED
Cloud run: |
…y why Review on #459 showed artifact_exists cannot gate .workforce/review.md: the worker's artifact scanner skips dot-directories, it records path presence on content change only (an empty file would pass, an identical rewrite would fail), while test -s demands a non-empty file and is idempotent. The TODO promising a swap is replaced by that rationale. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
282f9cb to
0ae7c8d
Compare
khaliqgant
changed the title
1 participant
Review on the first version of this PR (a swap to
artifact_exists) showed it cannot work here and is the wrong gate anyway: the worker's artifact scanner skips dot-directories so.workforce/review.mdis never journaled;artifact_existsrecords path presence on content change only, so an empty review would pass and an identical rewrite on a rerun would fail.test -sdemands a non-empty file and is idempotent. This PR replaces the TODO that promised the swap with that rationale, in the flow and the README. No behaviour change.🤖 Generated with Claude Code
Note
Low Risk
Comment and README edits only; no flow logic or gate configuration changes.
Overview
Documentation-only: the review step still uses
subprocess_gatewithtest -son.workforce/review.md; runtime behavior is unchanged.This PR drops the follow-up TODO that planned swapping to
artifact_existsonce flows#434 landed, and documents why that gate is wrong here: the worker artifact scanner skips dot-directories (so.workforce/is never journaled),artifact_existsonly tracks paths on content change (empty file would pass; identical rewrite on rerun would fail), whiletest -srequires a non-empty file and is idempotent. The rationale appears in the flow file header and in the README Not yet wired section.Reviewed by Cursor Bugbot for commit 0ae7c8d. Bugbot is set up for automated code reviews on this repo. Configure here.