Skip to content

fix(ship): resume human-gated findings from human answers - #2167

Merged
justinhelmer merged 1 commit into
mainfrom
plan/fix-issue-2154-a-human-g-7bdce4/u1
Sep 22, 2026
Merged

justinhelmer merged 1 commit into
mainfrom
plan/fix-issue-2154-a-human-g-7bdce4/u1

Conversation

@coreplane-switchboard

@coreplane-switchboard coreplane-switchboard Bot commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Human-gated review findings now park until the requester answers in the unit thread or from their bound GitHub account. The answer resumes the fix and mandatory re-review flow instead of ending or repeating the gate.

Why: Issue #2154 exposed a maintainer receipt being ignored, violating record 0054. Built on #2156, this uses record 0051's durable idle owner.

Where to look

  1. Park the human-gated question Persists the typed finding, verdict boundary, pull request and prior review while keeping the unit live. ⚠ A wrong ending can terminate or double-run the unit.
  2. Require post-answer re-review Runs the mandatory review beyond the ordinary fix-round cap when a person's answer enables the findings round. ⚠ A cap check here can strand accepted evidence without review.
  3. Admit live pull-request answers Accepts only comments authorized for the unfinished runner-owned unit and appends them to its durable event stream. ⚠ Bad ownership or identity routing can wake the wrong unit.
  4. Filter adopted-attempt answers Consumes only a newer answer from the requester's trusted GitHub account before review. ⚠ Review-first adoption can strand evidence; open admission enables injection.
  5. Accept canonical GitHub page links Allows repo-name or canonical repository-ID paths only when origin and pull-request resource remain unchanged. ⚠ Over-rejection misses answers; over-acceptance can follow an unrelated resource.
  6. Prove canonical three-page recovery Finds the page-2 gate and page-3 answer through GitHub's canonical review and comment pagination URLs.
  7. Quiet human-gate contract Names the parked question as Waiting for you: while retaining its executable proof binding.

Feedback wanted: Confirm the canonical pagination guard accepts GitHub's real Link targets without widening origin or resource scope, and inspect the answer-to-re-review transition.

Risk: This is 1,841 changed lines across 23 files; intake, paging, persisted state and round transitions must agree or answers can strand. Rollback is one commit; splitting intake from parking was rejected because either half is incomplete.

Verified: 56 pagination tests, TypeScript, changed-file formatting, hygiene, specs, docs and title gates pass; CI and re-review remain pending.

Decisions (8)
  • Trust only the requester's bound GitHub account. Rejected accepting every human GitHub account or trusting prose. A PR comment can brief a write-capable child running as the requester, so immutable GitHub id and current login must match the stored binding; unbound and mismatched accounts fail closed.
  • Accept GitHub's canonical pagination route. Rejected exact repo-name prefixes because GitHub emits /repositories/{id}/… Link targets. The shared pager accepts only the same GitHub origin and list resource through either route, while retaining malformed-link and cycle refusal.
  • Park independently of ship.idleDays. Rejected preserving the terminal hold because it leaves no owner to receive the answer. Human-gated findings always use the indexed idle state, bounded by its platform ceiling.
  • Resume without spending a renewal. Rejected charging a renewal for waiting on evidence because the coding lease did not expire mid-work. The answer reopens the same segment under one bounded lease for fix and re-review.
  • Limit the unchanged-head exception to successful answers. Rejected applying the ordinary fix-round cap or inaction rejection after a successful answer because that suppresses its promised review. Failed and interrupted findings children retain their own endings.
  • Recover only the legacy title omission. Current markers persist the complete typed finding. Earlier markers omitted only the title, which remains in the same immutable typed table; recovery accepts exactly one matching row and otherwise fails closed.
  • Normalize only GitHub's timestamp boundary. Rejected strict millisecond comparison because GitHub drops sub-second order, and rejected weakening precise thread ordering. GitHub comments include the verdict's reported second; other events remain strictly later.
  • Share complete Markdown cell escaping. Rejected another local pipe-only escape after CodeQL showed a backslash could consume it. The renderer and legacy parser share ordered escaping and compatible decoding.
Validation (11 criteria)
Criterion Proof
Canonical three-page review and comment recovery npx vitest run src/execution/githubPulls.test.ts passed at 648088f: 1 file, 56 tests; the fixture asserts canonical /repositories/{id}/… page-2 and page-3 URLs.
The regression failed before the implementation The same focused Vitest command produced 1 failure because canonical page 2 was rejected and the human-gated review was absent.
Changed root TypeScript remains type-safe NODE_OPTIONS=--max-old-space-size=6144 npx tsc --noEmit -p tsconfig.json passed at 648088f.
Every changed file remains formatted npx prettier --check on all 23 paths in origin/main...HEAD passed at 648088f.
Every changed spec proof resolves npm run specs:check passed: 51 specs and 5,668 proof references checked.
Generated documentation remains current npm run docs:check passed: 12 files match the registry, records, diagrams and vocabulary rows.
The change adds no public-tree imprint npm run hygiene:check passed: 9 files, 37 existing hits and 139 allowed lines.
The PR title satisfies changelog grammar npm run check:pr-title -- "fix(ship): resume human-gated findings from human answers" passed.
All previously changed behavior remained green before this focused amendment npx vitest run on all 8 changed test files passed at 299def7: 8 files, 880 tests.
Answered gates retain dead-child endings The coordinator regression passed with interrupted and failed findings-child endings preserved at the reviewed head.
Quiet human-gate wording matches implementation agent-ship.md item 12a says Waiting for you: and retains its prior executable proof binding.
For agents

Rebased onto origin/main at 737ec9b immediately before the force-push and squashed to one commit at 648088f. The latest review fix changes only src/execution/githubPulls.ts and its test: GitHub canonicalizes Link targets from /repos/{owner}/{repo}/… to /repositories/{id}/…, so the pager now validates the GitHub origin and exact list-resource suffix rather than an exact base prefix. Reproduce with npx vitest run src/execution/githubPulls.test.ts -t "follows every review and comment page". No generated files changed.
Requested by @justinhelmer in slack:C0BRRHKFLCB

🤖 Generated with Claude Code

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested: A valid same-second human answer can be consumed as stale and strand the parked unit.

Warning

Changes requested · head dded69c · 1 finding: 1 minor

Severity Finding Where
minor F1 Spec contradiction — docs/reference/specs/agent-ship.md item 9: same-second human answers are discarded src/channels/adminCoordinator.ts:2146
Full review

F1 (minor, high confidence): GitHub’s created_at timestamps have one-second precision, while reviewAskedAt can include milliseconds. A valid comment posted later in the same second therefore fails the strict event.at > askedAt check at src/channels/adminCoordinator.ts:2146; the wake then consumes that event without using it, leaving the unit parked. The adopted-attempt comparison at line 1120 has the same problem. Normalize the verdict boundary to GitHub’s timestamp precision or otherwise preserve ordering without a strict millisecond comparison.

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested: Existing human-gated verdict markers cannot be adopted, so the reported recovery path remains review-first for already-open PRs.

Warning

Changes requested · head 70a79e7 · 1 finding: 1 minor

Severity Finding Where
minor F1 Legacy human-gated verdict markers are rejected src/channels/adminCoordinator.ts:1115
Full review

F1 — Minor, high confidence. Before this PR, verdict markers omitted title, but postedHumanGateAnswer now requires every recovered finding to satisfy isFindingShape, which requires a title. Therefore an existing PR with a human-gated verdict and a newer human comment—such as the motivating scenario—returns no humanGate and proceeds review-first again. Accept the prior marker shape through a backward-compatible parser or otherwise recover its typed finding before adoption.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2154-a-human-g-7bdce4/u1 branch from 70a79e7 to 19ceecd Compare September 21, 2026 22:08
Comment thread src/channels/adminCoordinator.ts Fixed

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM: The legacy-marker recovery resolves F1 without weakening the typed human-gate checks.

Note

Approved · head 19ceecd · no findings

Full review

F1 is resolved: legacy verdict markers recover the omitted title from the typed findings table and remain fail-closed for malformed or ambiguous rows.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approved: coreplane-switchboard[bot] reviewed this PR and posted an LGTM verdict (see its review). This repository opted in through its REVIEW_BOT_LOGIN and REVIEW_BOT_ID variables.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2154-a-human-g-7bdce4/u1 branch from 19ceecd to 6ef734c Compare September 21, 2026 22:18

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM: The prior CodeQL failure is resolved at the current head, and the re-review delta is correct.

Note

Approved · head 6ef734c · no findings

Full review

The prior CodeQL failure is resolved at the current head, and the re-review delta introduces no new issues.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approved: coreplane-switchboard[bot] reviewed this PR and posted an LGTM verdict (see its review). This repository opted in through its REVIEW_BOT_LOGIN and REVIEW_BOT_ID variables.

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested: A human-gated answer at the last allowed review round is fixed but never re-reviewed.

Warning

Changes requested · head 6ef734c · 1 finding: 1 major

Severity Finding Where
major F1 Spec contradiction — docs/reference/specs/agent-ship.md item 9: a last-round human answer reaches the cap before re-review src/core/ship/coordinator.ts:1274
Full review

F1: With maxRounds=1, a human-gated finding from review round 1 parks correctly. After the person answers, the restored state still has reviewRounds=1; the findings child runs, but nextReview() immediately emits round_cap instead of spawning re-review. This contradicts the spec’s promised answer → fix → re-review flow. Allow the mandatory post-answer re-review beyond the ordinary fix-round cap, as already done for post-rebase review, and add a regression test for answering a gate from the last allowed round.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2154-a-human-g-7bdce4/u1 branch from 6ef734c to f9316bf Compare September 21, 2026 22:37

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested: PR-comment answers bypass actor authorization and can start a write-capable fix round from an untrusted commenter.

Warning

Changes requested · head f9316bf · 1 finding: 1 major

Severity Finding Where
major F1 PR comments resume the requester’s coding pipeline without authorizing the commenter src/core/coordinator/checksIntake.ts:187
Full review

F1: The webhook accepts any GitHub account with type === "User" and feeds its comment into a write-capable coding child running as the original requester. On a public PR, an unrelated commenter can therefore wake a parked unit and inject its text into the fix brief. The adopted-attempt path repeats this at src/channels/adminCoordinator.ts:1151. Resolve and authorize the commenter—or enforce an equivalent trusted-author policy—in both paths, with regression coverage proving an untrusted comment cannot resume the pipeline.

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM: The prior authorization finding is resolved in both live and adopted comment paths, with the full change consistent with its contract and specs.

Note

Approved · head 10fb06b · no findings

Full review

F1 is resolved in both live and adopted comment paths. The reviewVerdict.test.ts title change preserves verification while adding backslash-escaping coverage.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approved: coreplane-switchboard[bot] reviewed this PR and posted an LGTM verdict (see its review). This repository opted in through its REVIEW_BOT_LOGIN and REVIEW_BOT_ID variables.

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested: The adopted human-gate recovery can miss relevant reviews and answers on heavily paginated pull requests.

Warning

Changes requested · head 10fb06b · 1 finding: 1 minor

Severity Finding Where
minor F1 Middle GitHub pages are skipped during gate recovery src/execution/githubPulls.ts:1180
Full review

F1 — Both new pagination paths fetch only page 1 and the final page, skipping every middle page. On a PR with over 200 reviews or comments, the latest bot-authored human gate or the requester’s valid answer can be omitted, leaving the pipeline parked or resuming from a stale gate. Follow every next link, or page backward until the relevant review/comment is found.

The reviewVerdict test title change is a refactor with verification intact; it expands the same assertion to cover backslashes and pipes.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2154-a-human-g-7bdce4/u1 branch from 10fb06b to 9cb7a91 Compare September 21, 2026 23:17

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested: An evidence-only human-gate answer can still abort before its required re-review.

Warning

Changes requested · head 9cb7a91 · 1 finding: 1 major

Severity Finding Where
major F1 Spec contradiction — docs/reference/specs/agent-ship.md item 9: answered evidence-only findings abort before re-review src/core/ship/coordinator.ts:2465
Full review

F1: When the human answer is evidence-only, the findings child may correctly record the finding as fixed without changing the branch head. The same-head guard then aborts because only declined dispositions permit re-review, before the human-answer exemption at line 2480 is reached. This breaks the promised answer → fix → re-review flow in the motivating receipt case. Bypass the same-head rejection whenever the round carries a human answer, and add a maxRounds=1 regression that remains on HEAD_A through the findings step and still reaches merge-ready.

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested: The prior finding is fixed, but the same-head exception now skips failed/interrupted findings-child handling when a human answer is present.

Warning

Changes requested · head fc8cd70 · 1 finding: 1 minor

Severity Finding Where
minor F2 Answered gates bypass dead-child handling at an unchanged head src/core/ship/coordinator.ts:2465
Full review

F2 (minor, high confidence): !hasHumanAnswer wraps the dead-child checks as well as the ordinary same-head rejection. If the answered gate’s findings child fails or is interrupted without pushing, the unchanged PR is sent directly to mandatory re-review instead of ending with the child’s failure. Keep failed/interrupted handling unconditional at an unchanged head, and apply the human-answer exception only to the no-new-head/disposition rejection.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2154-a-human-g-7bdce4/u1 branch from fc8cd70 to 299def7 Compare September 21, 2026 23:36

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM: F2 is resolved: dead findings children retain their own endings before any answered-gate same-head bypass.

Note

Approved · head 299def7 · no findings

Full review

F2 is resolved: dead findings children now retain interrupted or failed endings before the answered-gate same-head bypass. The test-title rename strengthens the existing escaping case, so verification remains intact.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approved: coreplane-switchboard[bot] reviewed this PR and posted an LGTM verdict (see its review). This repository opted in through its REVIEW_BOT_LOGIN and REVIEW_BOT_ID variables.

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested: One agent-ship validation row still contradicts the new quiet human-gate wording.

Warning

Changes requested · head 299def7 · 1 finding: 1 minor

Severity Finding Where
minor F1 Spec contradiction — agent-ship.md item 12a: quiet human gates now say “Waiting for you,” not “Held” docs/reference/specs/agent-ship.md:124
Full review

F1: Item 12a still promises the quiet human-gated report starts with Held:, but the implementation and updated test now emit Waiting for you:. Update the validation row to match the new wording. The reviewVerdict.test.ts title change is a refactor with verification intact because it extends the same assertion to cover backslashes before pipes.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2154-a-human-g-7bdce4/u1 branch from 299def7 to ea24162 Compare September 21, 2026 23:52

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested: The multi-page recovery still fails against GitHub's real canonical pagination links.

Warning

Changes requested · head ea24162 · 1 finding: 1 minor

Severity Finding Where
minor F1 GitHub's canonical pagination URLs are rejected src/execution/githubPulls.ts:1160
Full review

F1: GitHub canonicalizes pagination links from /repos/{owner}/{repo}/… to /repositories/{id}/…. The startsWith(base) guard therefore rejects the first real next URL and returns undefined, so multi-page reviews or comments still leave adopted recovery unable to find the gate or answer. Validate the GitHub origin and expected canonical resource path instead, and make the regression fixture use GitHub’s canonical Link form.

Co-Authored-By: coreplane-switchboard[bot] <318072483+coreplane-switchboard[bot]@users.noreply.github.com>
@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2154-a-human-g-7bdce4/u1 branch from ea24162 to 648088f Compare September 22, 2026 00:03

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM: The canonical GitHub pagination fix resolves F1 without weakening resource or origin validation.

Note

Approved · head 648088f · no findings

Full review

No findings. F1 is fixed; the test-guard title rename expands backslash coverage with verification intact.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approved: coreplane-switchboard[bot] reviewed this PR and posted an LGTM verdict (see its review). This repository opted in through its REVIEW_BOT_LOGIN and REVIEW_BOT_ID variables.

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM: The human-gate recovery flow is coherent, authorized, restart-safe, and covered by the updated contracts and regressions.

Note

Approved · head 648088f · no findings

Full review

No findings. The reviewVerdict test title change is a refactor with verification intact, expanding the same table-cell assertion to cover backslashes as well as pipes.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approved: coreplane-switchboard[bot] reviewed this PR and posted an LGTM verdict (see its review). This repository opted in through its REVIEW_BOT_LOGIN and REVIEW_BOT_ID variables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants