Public: anyone can read this; the crew can change it.

An agent's pull cannot land without a review by another identity: one table test over every landing door #12427

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

An agent's pull now lands only after someone other than its author approves it, at every landing door. The gap was in the review gate. With **Require review** on, `reviewGate` counted any current approval, including the author's own. `submitReview` already refuses self-approval, but an author's approval saved any other way still let the pull land. Now `reviewGate` (`src/core/pulls.ts`) passes the…

shipped production is serving it deploy b1c6dc4

opened this

An agent's pull now lands only after someone other than its author approves it, at every landing door. The gap was in the review gate. With Require review on, reviewGate counted any current approval, including the author's own. submitReview already refuses self-approval, but an author's approval saved any other way still let the pull land. Now reviewGate (src/core/pulls.ts) passes the pull's author to reviewSummary (src/core/reviews.ts), which ignores that author's own verdicts. Imported reviews from another forge are left alone. Both landing actions read this gate: mergePull (through landPull) and landSlice. So the fix lives in one place, in core rather than in src/actions/pulls.ts as the issue suggested.

The door table is test/landing-doors-review.test.ts.

  • Finding doors: it lists every door: each place in src that calls mergePull, landSlice or landPastGate, each landing route in the contract, and each MCP tool whose code calls one of them. A door missing from the table fails by name, and so does a row whose door no longer exists.
  • Trying doors: with reviews required, each door gets an agent's pull three times. With no review it is refused (review_required). With the agent's own approval it is refused: submitReview says self_review, and an approval row written straight into the table still doesn't count. With a person's approval it lands.
  • Owner's exception: with reviews off and no reviewer seated, an agent's pull lands with no review, as docs/features/reviews-off-lands-on-green.md says.
  • Proof it bites: without the fix, 19 of the 23 tests fail, because every door lands on the agent's own approval.

Things to look at:

  • Only the API (merge, slices) and MCP merge doors are tried over HTTP. The web, studio, terminal, fleet, build, engine, bypass and preview-seed doors call the landing action directly with the options their call site passes. The file scan only checks that each one still calls it.
  • land_past_gate and the web and terminal past-the-gate doors are tried as mergePull with pastGate. That is what landPastGate runs after its own owner and red-gate checks, so those checks are not set up in the test.
  • In the test, the agent's pull is opened by a person and then re-authored to the agent. Crew agents push over git, so commitToBranch refuses them.
  • The test logs a harmless "could not recombine … closed database" warning at teardown, as other landing tests do.
  • An author's "changes requested" on their own pull no longer counts either.

Not done: with Require review off but a reviewer seated, mergePull still lets a person land with no review, though the reviews-off doc says reviews stay on wherever a reviewer is seated. That is outside this issue, and I didn't record a keep_plan or an estimate.

Checks: bun scripts/gate.ts and bun x tsc --noEmit are clean. scripts/fast.ts named 2,345 test files, too many to run here. Instead I ran the new test plus the review, review-gate, slices, MCP merge/review, merge-queue, crew-review, landing and mcp-size suites: 363 tests, all passing.

Try it: bun test test/landing-doors-review.test.ts

🤖 Generated with Claude Code

Closes #12403

commented

An agent's pull now lands only after someone other than its author approves it, at every landing door. The gap was in the review gate. With Require review on, reviewGate counted any current approval, including the author's own. submitReview already refuses self-approval, but an author's approval saved any other way still let the pull land. Now reviewGate (src/core/pulls.ts) passes the pull's author to reviewSummary (src/core/reviews.ts), which ignores that author's own verdicts. Imported reviews from another forge are left alone. Both landing actions read this gate: mergePull (through landPull) and landSlice. So the fix lives in one place, in core rather than in src/actions/pulls.ts as the issue suggested.

The door table is test/landing-doors-review.test.ts.

  • Finding doors: it lists every door: each place in src that calls mergePull, landSlice or landPastGate, each landing route in the contract, and each MCP tool whose code calls one of them. A door missing from the table fails by name, and so does a row whose door no longer exists.
  • Trying doors: with reviews required, each door gets an agent's pull three times. With no review it is refused (review_required). With the agent's own approval it is refused: submitReview says self_review, and an approval row written straight into the table still doesn't count. With a person's approval it lands.
  • Owner's exception: with reviews off and no reviewer seated, an agent's pull lands with no review, as docs/features/reviews-off-lands-on-green.md says.
  • Proof it bites: without the fix, 19 of the 23 tests fail, because every door lands on the agent's own approval.

Things to look at:

  • Only the API (merge, slices) and MCP merge doors are tried over HTTP. The web, studio, terminal, fleet, build, engine, bypass and preview-seed doors call the landing action directly with the options their call site passes. The file scan only checks that each one still calls it.
  • land_past_gate and the web and terminal past-the-gate doors are tried as mergePull with pastGate. That is what landPastGate runs after its own owner and red-gate checks, so those checks are not set up in the test.
  • In the test, the agent's pull is opened by a person and then re-authored to the agent. Crew agents push over git, so commitToBranch refuses them.
  • The test logs a harmless "could not recombine … closed database" warning at teardown, as other landing tests do.
  • An author's "changes requested" on their own pull no longer counts either.

Not done: with Require review off but a reviewer seated, mergePull still lets a person land with no review, though the reviews-off doc says reviews stay on wherever a reviewer is seated. That is outside this issue, and I didn't record a keep_plan or an estimate.

Checks: bun scripts/gate.ts and bun x tsc --noEmit are clean. scripts/fast.ts named 2,345 test files, too many to run here. Instead I ran the new test plus the review, review-gate, slices, MCP merge/review, merge-queue, crew-review, landing and mcp-size suites: 363 tests, all passing.

Try it: bun test test/landing-doors-review.test.ts

🤖 Generated with Claude Code

commented

An agent can no longer land its own pull. Before a pull lands, it needs a review from an identity other than its author, and every way of landing it enforces this (#12403). An approval the author gives on their own pull doesn't count, even if a row recording it got into the database some other way.

Where to look:

  • src/core/reviews.ts, where the author's own verdict is left out when reviews are counted.
  • test/landing-doors-review.test.ts, which seeds an agent's pull the way a git push would write it. It shows that the agent's self-approval is refused and doesn't count, and that an agent seated as reviewer still counts on other people's pulls.

This push only rewraps four comment lines that the gate flagged as longer than 100 columns. The gate, bun x tsc --noEmit and bun test test/landing-doors-review.test.ts (23 pass) are green. The gate only cleared once the rewrap was committed, because it reads committed lines rather than the working tree.

🤖 Generated with Claude Code

Approved

The fix is in the one shared gate: reviewGate now drops the author own native verdicts (imported ones kept), so mergePull and landSlice both enforce it. The door table scans src call sites, contract routes and MCP tools, fails by name on a missing or stale row, and tries each door with no review, a self review and another identity review. Preview boots (200); the change is test and core only, so nothing visible to check. Next, two things: (1) with Require review off but a reviewer seated, mergePull still lets a person land with no review, contradicting the reviews-off doc: open an issue. (2) The web, studio, terminal, fleet, build, engine, bypass and seed doors are only driven through the action, not their own handlers; add an HTTP or handler-level case for the web and terminal doors so a call site that drops the shared action is caught.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs