fix(ship): resume human-gated findings from human answers - #2167
Conversation
There was a problem hiding this comment.
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.
dded69c to
70a79e7
Compare
There was a problem hiding this comment.
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.
70a79e7 to
19ceecd
Compare
There was a problem hiding this comment.
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.
19ceecd to
6ef734c
Compare
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
6ef734c to
f9316bf
Compare
There was a problem hiding this comment.
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.
4959d0b to
10fb06b
Compare
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
10fb06b to
9cb7a91
Compare
There was a problem hiding this comment.
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.
9cb7a91 to
fc8cd70
Compare
There was a problem hiding this comment.
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.
fc8cd70 to
299def7
Compare
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
299def7 to
ea24162
Compare
There was a problem hiding this comment.
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>
ea24162 to
648088f
Compare
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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
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)
/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.Validation (11 criteria)
/repositories/{id}/…page-2 and page-3 URLs.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 withnpx 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