feat(ship): pr shepherding scaffolding + flag-gated trigger surfaces - #75
Merged
Conversation
Adds the foundational lifecycle for `bot:ship` PR shepherding (specs/ 20260427-201332-pr-shepherding-merge-ready), shipped behind three default-off flags so it's safely soakable: - `SHIP_USE_TRIGGER_SURFACES_V2` — natural-language + label trigger surfaces - `SHIP_USE_PROBE_VERDICT` — new GraphQL probe verdict ladder - `SHIP_USE_CONTINUATION_LOOP` — restart-safe continuation re-entry What ships: - Foundational schema (migration 008): `ship_intents`, `ship_iterations`, `ship_continuations`, `ship_fix_attempts` with the partial-unique active-per-PR index, lifecycle CHECKs, and typed query helpers in `src/db/queries/ship.ts` - 13 new modules under `src/workflows/ship/`: verdict, probe (+ runProbeIntegrated wrapper for review-barrier / flake-tracker / base-ref resync / verdict_json persistence), intent state machine, eligibility, tracking-comment, continuation, webhook-reactor, tickle-scheduler, nl-classifier, label-trigger, literal-command, trigger-router, abort, signature, fix-attempts, review-barrier, flake-tracker, deadline, command-dispatch, lifecycle-commands (stop/resume/abort), session-runner, log-fields Zod schema - Webhook reactor wired into 7 event handlers (synchronize/closed/ labeled, review.submitted, review_comment.created/edited/deleted, check_run/check_suite.completed) via `reactor-bridge` - T028e: literal-then-NL dispatch for issue_comment + review_comment - T029/T030: `resolve-review-thread` MCP server + registry opt-in via `enableResolveReviewThread` - T046b: static FR-009 destructive-action guard wired into `bun run check` - Cancellation checkpoints (T059): `checkpointCancelled` helper + insertions in session-runner; `forceAbortIntent` now deletes continuation row + ZREMs ship:tickle per spec - Docs: new SHIP.md operator guide, CONFIGURATION/BOT-WORKFLOWS/ ARCHITECTURE/SETUP/OBSERVABILITY updates, mkdocs nav entry - Test infrastructure: scripts/test-isolated.sh uses globstar so depth-3+ test files actually run (uncovered + fixed a pre-existing telemetry-aggregates DROP gap) - 71 test files pass; typecheck/lint/format/destructive-guard/ docs:build all clean Deferred to follow-up PRs: - Iteration loop (probe → fix → probe). Modules exist; control loop not wired. Flag-gated path currently only handles PRs already merge-ready at trigger time. - T046 full BlockerCategory mapping for the 8 non-ready terminal cases - T046a runtime destructive-action guards in mocked tool-call recorders - T031/T031b local e2e against a real PR - T068 perf benchmarks, T070 soak, T071 flag-removal follow-up PR Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds the PR-shepherding "bot:ship" feature: documentation/specs, DB migration and typed queries, Valkey scheduling/tickle scheduler, webhook/reactor and command routing, MCP resolve-review-thread server, numerous ship workflow modules (probe, intent lifecycle, continuations, deadlines, eligibility, etc.), CI/build scripts, and comprehensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User / Trigger
participant Webhook as Webhook Handler
participant Router as Trigger Router
participant Session as Session Runner
participant Probe as Merge Readiness Probe
participant DB as Database
participant Reactor as Webhook Reactor
participant Comment as Tracking Comment
User->>Webhook: PR comment / label / mention (bot:ship)
Webhook->>Router: Route trigger (literal/label/nl)
Router->>Session: Dispatch canonical command
Session->>DB: Check eligibility + create intent
Session->>Probe: Run probe (GraphQL)
Probe->>DB: Store iteration verdict snapshot
Probe-->>Session: Return MergeReadiness verdict
Session->>Comment: Create/update tracking comment
alt verdict == ready
Session->>DB: Transition intent -> terminal ready_awaiting_human_merge
else
Session->>DB: Persist continuation with wake_at
end
Note over User, Comment: time passes / external events
Webhook->>Reactor: Webhook event (PR sync/review/check)
Reactor->>DB: Find active intents for PR
Reactor->>DB: Update wake / tickle continuation (Valkey)
sequenceDiagram
participant Cron as Cron Tickle Scheduler
participant Valkey as Valkey (sorted set)
participant Reactor as FireReactor
participant Runner as Continuation Runner
participant Probe as Probe
participant DB as Database
Cron->>Valkey: ZRANGEBYSCORE (due intent_ids)
Valkey-->>Cron: [intent_ids]
Cron->>Reactor: fireReactor(intent_id) (async)
Reactor->>Runner: resume continuation (async)
Runner->>DB: load intent + continuation
Runner->>Probe: run probe
Probe-->>Runner: verdict
alt ready or terminal
Runner->>DB: transition intent, delete continuation
else
Runner->>DB: update continuation.wake_at
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
|
…epherding-merge-ready # Conflicts: # test/integration/telemetry-aggregates.test.ts
The beforeAll cleanup was missing the four new ship_* tables added in migration 008. Without these DROPs, a re-run of the test against an already-migrated database fails with 'relation "ship_intents" already exists' when migration 008 replays. Mirrors the same fix already applied to telemetry-aggregates.test.ts.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (24)
scripts/test-isolated.sh-6-6 (1)
6-6:⚠️ Potential issue | 🟠 MajorFail when the glob matches no test files.
nullglobturns an unmatchedtest/**/*.test.tsinto an empty list, so the loop is skipped and the script still reports "All tests passed." with exit code 0. This creates a false-green CI result if test files are accidentally deleted, the glob is misconfigured, or the test directory structure changes.🔧 Safer pattern
set -uo pipefail shopt -s globstar nullglob + +tests=(test/**/*.test.ts) +if ((${`#tests`[@]} == 0)); then + echo "No test files matched test/**/*.test.ts" + exit 1 +fi @@ -for f in test/**/*.test.ts; do +for f in "${tests[@]}"; do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-isolated.sh` at line 6, The script currently uses shopt -s nullglob which makes an unmatched glob expand to an empty list so the for-loop is skipped and the script can falsely report success; fix by capturing the glob results into an array (e.g. tests=(test/**/*.test.ts)), check that ${`#tests`[@]} is greater than zero and, if not, print an error and exit non‑zero, then iterate over "${tests[@]}" in the existing test runner loop; update the code around the glob use and the loop that references the test files to use this array check so the script fails fast when no test files are found.src/workflows/ship/signature.ts-33-60 (1)
33-60:⚠️ Potential issue | 🟠 MajorFix ANSI stripping before it mangles normal log text.
ANSI_ESCAPEcurrently matches plain bracketed text such as[error]because it is missing the escape prefix. That means the normalizer can delete ordinary log content and destabilize Tier 1 signatures.🛠️ Proposed fix
-const ANSI_ESCAPE = /\[[0-9;]*[A-Za-z]/g; +const ANSI_ESCAPE = /\x1b\[[0-9;]*[A-Za-z]/g;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/ship/signature.ts` around lines 33 - 60, The ANSI_ESCAPE regex is too broad and strips plain bracketed text (e.g. "[error]") before pattern extraction; update the ANSI_ESCAPE constant to only match real terminal escape sequences (include the ESC control prefix like \x1b or \u001B, e.g. change ANSI_ESCAPE to a pattern that begins with the escape character followed by "[") so that normalise() no longer removes ordinary log content used by TIER1_PATTERNS; edit the ANSI_ESCAPE definition near the top of the file (referencing the ANSI_ESCAPE constant and the normalise function) and run/update tests to confirm Tier‑1 signatures remain stable.scripts/check-no-destructive-actions.ts-39-53 (1)
39-53:⚠️ Potential issue | 🟠 MajorFail closed on filesystem errors.
Right now
readdirSync/statSyncfailures are swallowed, so a missing or unreadable scan root can still reportcleanand let the guard pass open. For a CI safety check, that should abort the script instead of skipping the file or directory.🔧 Proposed fix
function* walk(dir: string): Generator<string> { - let entries: string[]; - try { - entries = readdirSync(dir); - } catch { - return; - } + const entries = readdirSync(dir); for (const entry of entries) { const full = join(dir, entry); - let stat; - try { - stat = statSync(full); - } catch { - continue; - } + const stat = statSync(full); if (stat.isDirectory()) { yield* walk(full); } else if (stat.isFile() && (full.endsWith(".ts") || full.endsWith(".tsx"))) { yield full; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check-no-destructive-actions.ts` around lines 39 - 53, The walk generator currently swallows filesystem errors from readdirSync and statSync causing scans to falsely report success; update walk (and its readdirSync/statSync try-catches) to fail closed by rethrowing or propagating the caught error instead of returning or continuing—e.g., in the initial readdirSync catch throw a new Error (or rethrow the original) so the script aborts, and in the statSync catch do not silently continue but rethrow or surface the error so callers of walk see the failure and CI can fail the job.src/webhook/events/issue-comment.ts-29-53 (1)
29-53:⚠️ Potential issue | 🟠 MajorBoth dispatch paths can execute on the same PR comment, creating duplicate work — clarify the rollout intent.
The flag-gated
dispatchCommentSurfacepath runs on all PR comments when enabled;dispatchByIntentalso runs when the comment contains the trigger phrase and the owner is allowlisted. There is no short-circuit between them, anddispatchCommentSurfacehas no internal check forcontainsTriggerorisOwnerAllowed. This means a single PR comment from an allowlisted user matching the trigger phrase will dispatch work through both paths:
- Path 1 invokes
dispatchCommentSurface(processes the comment via literal parser + NL fallback)- Path 2 then invokes
dispatchByIntent(processes the same comment via intent classifier)The code comment states "The legacy intent-classifier dispatch below is preserved," suggesting this is intentional during a rollout. However, there is:
- No documented success criteria or migration plan for eventually removing one path
- No test coverage for the interaction between both dispatchers on the same comment
- No database-level dedup to prevent duplicate
workflow_runsrowsAdd a test case covering both flags/conditions firing on the same comment, and clarify in code or
docs/ARCHITECTURE.mdwhether dual dispatch is temporary during rollout or a permanent design decision.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/webhook/events/issue-comment.ts` around lines 29 - 53, The PR allows both dispatchCommentSurface and dispatchByIntent to run for the same PR comment when config.shipUseTriggerSurfacesV2 is true and the comment matches the trigger/allowlist, causing duplicate work; add a guard and tests to either short-circuit the legacy path or prevent double-dispatch: update the issue-comment handler to check containsTrigger and isOwnerAllowed (or set a new flag on the dispatchLog/metadata) before calling dispatchByIntent when dispatchCommentSurface already ran (or vice-versa), add unit/integration tests that simulate a PR comment where config.shipUseTriggerSurfacesV2 is true and the comment contains the trigger and owner is allowlisted to assert only one dispatch occurs, and add a brief note in ARCHITECTURE.md stating whether dual-dispatch is temporary during rollout or intended permanently (reference functions/vars: dispatchCommentSurface, dispatchByIntent, config.shipUseTriggerSurfacesV2, containsTrigger, isOwnerAllowed).src/webhook/events/pull-request.ts-12-12 (1)
12-12:⚠️ Potential issue | 🟠 MajorLabel gate blocks supported ship label formats (
abort-ship,/deadline=...).Line 81 short-circuits before the new trigger-router path runs, because
BOT_LABEL_PATTERNonly allowsbot:plus letters. That rejectsbot:abort-shipandbot:ship/deadline=2h, so those documented triggers never dispatch.💡 Proposed fix
-const BOT_LABEL_PATTERN = /^bot:[a-z]+$/; +const BOT_LABEL_PATTERN = /^bot:[a-z][a-z-]*(?:\/deadline=\d+(?:\.\d+)?[hms])?$/;Also applies to: 80-82, 119-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/webhook/events/pull-request.ts` at line 12, The BOT_LABEL_PATTERN is too restrictive (const BOT_LABEL_PATTERN = /^bot:[a-z]+$/) and excludes labels like "bot:abort-ship" and "bot:ship/deadline=2h"; update BOT_LABEL_PATTERN to allow hyphens, slashes, digits and `=` (for example change to /^bot:[a-z0-9_\-\/=]+$/i) and replace the existing pattern usages (the short-circuit label checks that reference BOT_LABEL_PATTERN) so those gate checks accept the new supported ship label formats.src/workflows/ship/review-barrier.ts-61-70 (1)
61-70:⚠️ Potential issue | 🟠 MajorExclude other bot/app reviews before clearing the barrier.
This only filters
ourAppLogin, so a review on the current head SHA from a different bot/app account still counts as a qualifying review. That lets automation satisfy the latency barrier without any human review. Please carry an actor-type/bot discriminator from the probe response and ignore non-human authors here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/ship/review-barrier.ts` around lines 61 - 70, The current hasQualifyingReview predicate only excludes reviews from input.ourAppLogin but not other non-human actors; update the predicate used in reviews.some (inside hasQualifyingReview) to also check the review actor type from the probe response (e.g., r.author?.__typename or r.author?.type) and only count reviews whose author type indicates a human (e.g., "User"); explicitly ignore types such as "Bot", "App", "Organization" (or any non-User values) in addition to excluding input.ourAppLogin and mismatched commit OIDs so that only human reviews on headSha qualify to clear the barrier.specs/20260427-201332-pr-shepherding-merge-ready/contracts/probe-graphql-query.md-83-126 (1)
83-126:⚠️ Potential issue | 🟠 MajorAdd the review-author discriminator before relying on this barrier rule.
The mapping at Line 126 depends on distinguishing
Userauthors from App authors, but the query only fetchesauthor.login. Without__typename, the review-barrier logic cannot be deterministic from this payload alone. The fixedreviews(last: 20)slice is also a risk if the qualifying review falls outside that window.♻️ Proposed fix
reviews(last: 20) { nodes { id author { + __typename login } state submittedAt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/20260427-201332-pr-shepherding-merge-ready/contracts/probe-graphql-query.md` around lines 83 - 126, The GraphQL payload is missing author.__typename which prevents reliably distinguishing User vs App reviewers, and the fixed reviews(last: 20) window risks missing the relevant review; update the query slice reviews(last: 20) to include author { __typename login } (so you can detect User vs App in the review-barrier logic that examines reviews[].submittedAt) and expand or paginate the reviews selection (e.g., increase the count or add cursor-based pagination) so the probe deterministically sees any qualifying review; ensure the code that evaluates the barrier reads author.__typename instead of relying only on author.login.test/workflows/ship/verdict.test.ts-283-289 (1)
283-289:⚠️ Potential issue | 🟠 MajorMove the missing-PR case out of
computeVerdict.A missing PR is the probe’s terminal error path, not a readiness verdict. This assertion currently locks the wrong behavior into the verdict suite and conflicts with the contract that maps
NOT_FOUNDto a terminalpr_closed/ unrecoverable-error path. Please cover this in the probe error-handling tests instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/workflows/ship/verdict.test.ts` around lines 283 - 289, The test asserting computeVerdict returns "human_took_over" when response.repository.pullRequest is null should be removed from the computeVerdict suite and instead added to the probe error-handling tests; locate the test using computeVerdict, expectReason and ProbeResponseShape and delete that case, then create a new probe error test that exercises the terminal/missing-PR path (simulate response with pullRequest: null or map to NOT_FOUND) and assert the probe-level contract (pr_closed / terminal error) rather than a readiness verdict; keep references to computeVerdict and expectReason only in the original suite for legitimate verdict cases and verify the probe test asserts the terminal error behavior.specs/20260427-201332-pr-shepherding-merge-ready/contracts/mcp-resolve-thread-server.md-43-90 (1)
43-90:⚠️ Potential issue | 🟠 MajorKeep the tool response aligned with the success schema.
The success payload on Lines 45-55 includes
pr_number, but the tool-return example on Lines 84-88 drops it. That removes the field the caller can use to confirm the cross-PR safety story described above. Also, the security table markup here is malformed, so it will render incorrectly.♻️ Proposed fix
- { "thread_id": "<id>", "is_resolved": true } + { "thread_id": "<id>", "is_resolved": true, "pr_number": "<number>" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/20260427-201332-pr-shepherding-merge-ready/contracts/mcp-resolve-thread-server.md` around lines 43 - 90, The tool's failure response schema omitted pr_number which breaks the caller's cross-PR safety check and the tests; update the MCP resolve-thread tool contract so every response (both success and error) includes "pr_number" (matching the server-bound PR) and adjust the example failure JSON to include "pr_number": "<the input PR number>"; update any related wiring in mcp__resolve-review-thread__resolve_review_thread (and the resolve iteration wiring in resolve.ts) and the test test/mcp/resolve-review-thread.test.ts to expect the pr_number on error, and fix the malformed Security table markup so the table renders correctly.src/workflows/ship/command-dispatch.ts-62-109 (1)
62-109:⚠️ Potential issue | 🟠 MajorCatch parser/classifier failures in the comment-surface path.
dispatchCanonicalCommand()already logs handler exceptions, butdispatchCommentSurface()can still reject before dispatch whenrouteTrigger()or the LLM-backed classifier fails. That bubbles out of the webhook handler and can cause retries or duplicate delivery handling.🛠️ Suggested fix
export async function dispatchCommentSurface(input: { @@ }): Promise<void> { - const deps: DispatchDeps = { octokit: input.octokit, ...(input.log ? { log: input.log } : {}) }; - // 1. Literal-first. - const literal = await routeTrigger({ - surface: "literal", - payload: { - commentBody: input.commentBody, - principal_login: input.principal_login, - pr: input.pr, - }, - }); - if (literal !== null) { - dispatchCanonicalCommand(literal, deps); - return; - } - - // 2. NL fallback. Mention-prefix gate (FR-025a) lives in classifier. - const llm = getTriageLLMClient(); - const modelId = resolveModelId(config.triageModel, llm.provider); - const callLlm = async (params: { systemPrompt: string; userPrompt: string }): Promise<string> => { - const res = await llm.create({ - model: modelId, - system: params.systemPrompt, - messages: [{ role: "user", content: params.userPrompt }], - maxTokens: 256, - temperature: 0, - }); - return res.text; - }; - - const nl = await routeTrigger({ - surface: "nl", - payload: { - commentBody: input.commentBody, - triggerPhrase: config.triggerPhrase, - principal_login: input.principal_login, - pr: input.pr, - callLlm, - }, - }); - if (nl !== null) dispatchCanonicalCommand(nl, deps); + const log = (input.log ?? rootLogger).child({ + event: "ship.comment.surface", + principal_login: input.principal_login, + owner: input.pr.owner, + repo: input.pr.repo, + pr_number: input.pr.number, + }); + + try { + const deps: DispatchDeps = { octokit: input.octokit, log }; + // existing literal-first + NL fallback logic + } catch (err) { + log.error({ err }, "dispatchCommentSurface threw"); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/ship/command-dispatch.ts` around lines 62 - 109, dispatchCommentSurface can throw if routeTrigger or the LLM call fails, which bubbles out of the webhook; wrap the literal and NL classification flows in try/catch so parser/classifier failures are caught, logged, and do not rethrow. Concretely, in dispatchCommentSurface surround the calls to routeTrigger (for both "literal" and "nl") and the LLM invocation (getTriageLLMClient()/llm.create or callLlm) with try/catch blocks, use the available logger (input.log or deps.log) to record the error and relevant context (commentBody/pr/principal_login), and return early (do not call dispatchCanonicalCommand) on error so the webhook handler doesn’t reject; keep dispatchCanonicalCommand unchanged since it already logs handler exceptions.src/workflows/ship/nl-classifier.ts-46-49 (1)
46-49:⚠️ Potential issue | 🟠 MajorEnforce the trigger phrase at the comment prefix.
indexOf()accepts the mention anywhere in the body, but FR-025a and this file’s header both describe a mention-prefix gate. A quoted earlier comment or pasted log line containing the trigger phrase will still invoke the LLM and may dispatch a command.✂️ Prefix-only gate
- const idx = input.commentBody.indexOf(input.triggerPhrase); - if (idx === -1) return null; - const post = input.commentBody.slice(idx + input.triggerPhrase.length).trim(); + const trimmedBody = input.commentBody.trimStart(); + if (!trimmedBody.startsWith(input.triggerPhrase)) return null; + const post = trimmedBody.slice(input.triggerPhrase.length).trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/ship/nl-classifier.ts` around lines 46 - 49, The current logic uses input.commentBody.indexOf(input.triggerPhrase) which allows the trigger phrase anywhere; change it to enforce a prefix-only gate by checking that the trimmed comment body starts with the trigger phrase (e.g., use startsWith on input.commentBody.trim() or a regex that matches ^\s*<triggerPhrase>), and return null if it does not; update the branch that computes post (currently using idx and slice) to extract the substring after the prefix accordingly (use substring or slice from triggerPhrase.length on the trimmed string) so nl-classifier.ts only triggers when the comment begins with input.triggerPhrase.src/workflows/ship/tickle-scheduler.ts-62-84 (1)
62-84:⚠️ Potential issue | 🟠 MajorPrevent overlapping
tick()runs.
setIntervaldoes not wait for the previous asynctick()to finish. A slow Valkey call oronDue()lets two ticks process the same due window concurrently, which can dispatch the same intent more than once.🔒 Simple reentrancy guard
const intervalMs = deps.intervalMs ?? config.cronTickleIntervalMs; let timer: ReturnType<typeof setInterval> | null = null; + let ticking = false; @@ async function tick(): Promise<void> { + if (ticking) return; + ticking = true; + try { const nowMs = String(Date.now()); const due = (await deps.valkey.send("ZRANGEBYSCORE", [ TICKLE_KEY, "0", nowMs, @@ if (due === null || due.length === 0) return; for (const intent_id of due) { await deps.valkey.send("ZREM", [TICKLE_KEY, intent_id]); try { await deps.onDue(intent_id); @@ } } + } finally { + ticking = false; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/ship/tickle-scheduler.ts` around lines 62 - 84, The tick() function can re-enter under setInterval; add a reentrancy guard boolean (e.g., let isTickRunning = false scoped alongside tick) and at the top of tick() return immediately if isTickRunning is true; set isTickRunning = true before any await and always reset it to false in a finally block so that deps.valkey.send, deps.onDue, TICKLE_KEY removals and logger calls are never executed concurrently by multiple tick runs; ensure the guard is used in the same module where tick() is scheduled so overlapping dispatches are prevented.src/workflows/ship/flake-tracker.ts-102-108 (1)
102-108:⚠️ Potential issue | 🟠 Major
renderFlakeAnnotationcurrently excludes non-required flakes.Line 103 uses
identifyFlakedRequiredChecks(...), so non-required flakes never appear in the annotation, which conflicts with the module contract (Lines 7-9, 98-100). This drops useful audit context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/ship/flake-tracker.ts` around lines 102 - 108, renderFlakeAnnotation is calling identifyFlakedRequiredChecks and thus omits non-required flakes; change the call inside renderFlakeAnnotation to use the general flake identifier (e.g., identifyFlakedChecks(history)) or otherwise include non-required entries so the annotation reports all flakes; update the reference in renderFlakeAnnotation where identifyFlakedRequiredChecks is used to call the function that returns both required and non-required flakes (or adjust the called function to return all flakes) so the resulting flakes array contains every flake to be mapped into lines.src/workflows/ship/flake-tracker.ts-125-132 (1)
125-132:⚠️ Potential issue | 🟠 MajorTargeted rerun path is effectively disabled for projected probe history.
Lines 131 and 139 hardcode
check_run_id: null. SincetriggerTargetedRerunskips null IDs (Line 83), projected flakes cannot trigger rerequests.Also applies to: 134-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/ship/flake-tracker.ts` around lines 125 - 132, The projected probe entries currently set check_run_id: null (in the CheckRun branch handling of variable c), which prevents triggerTargetedRerun from enqueueing reruns; update the CheckRun branch to populate check_run_id from the actual check run identifier (use c.id or the correct ID field on c) and likewise ensure the alternate branch referenced around lines 134-140 populates check_run_id from its corresponding run ID (e.g., c.checkRunId or c.id) so triggerTargetedRerun can process projected flakes; adjust the code that pushes into out (the object with head_sha, check_name, conclusion, is_required, check_run_id) to use those real IDs.src/workflows/ship/lifecycle-commands.ts-44-45 (1)
44-45:⚠️ Potential issue | 🟠 MajorForeign-push check should not rely on a hardcoded bot login.
Line 44 hardcodes
chrisleekr-bot[bot]. If app login differs by environment/installation, bot-authored pushes can be misclassified as foreign and incorrectly terminate sessions.♻️ Suggested fix (inject runtime bot login)
-const BOT_APP_LOGIN = "chrisleekr-bot[bot]"; - export interface RunLifecycleInput { readonly command: CanonicalCommand; readonly octokit: Octokit; readonly log: Logger; + readonly botAppLogin: string; } @@ export async function runLifecycleCommand(input: RunLifecycleInput): Promise<void> { - const { command, octokit, log } = input; + const { command, octokit, log, botAppLogin } = input; @@ - if (author !== BOT_APP_LOGIN) { + if (author !== botAppLogin) {Also applies to: 160-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/ship/lifecycle-commands.ts` around lines 44 - 45, The foreign-push detection currently uses the hardcoded BOT_APP_LOGIN constant ("chrisleekr-bot[bot]"), which can misclassify bot-authored pushes; replace this by injecting the runtime bot/app login into the logic: remove or stop using BOT_APP_LOGIN, accept a botLogin parameter (or read from the runtime/installation context / env var) wherever the foreign-push check runs (references: BOT_APP_LOGIN constant and the foreign-push check in lifecycle-commands.ts around the push-handling functions), and update callers to pass the actual app/bot login so the comparison uses the real runtime login instead of a hardcoded value. Ensure the same change is applied to all uses (including the other occurrences in the file).src/workflows/ship/lifecycle-commands.ts-153-179 (1)
153-179:⚠️ Potential issue | 🟠 MajorResume currently fails open when foreign-push verification errors out.
On Line 178, the code logs and continues if
pulls.getfails. That allows resume without completing the required foreign-push safety check.🛡️ Suggested fail-closed behavior
- } catch (err) { - log.warn({ err }, "resume foreign-push check failed — proceeding cautiously"); + } catch (err) { + log.warn({ err, intent_id: intent.id }, "resume foreign-push check failed"); + await postReply( + octokit, + command, + "`bot:resume` declined — unable to verify head push author right now. Please retry.", + log, + ); + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/ship/lifecycle-commands.ts` around lines 153 - 179, The foreign-push safety check currently swallows errors from octokit.rest.pulls.get and lets resume continue; change the catch block so that on any error during the pulls.get/verification you fail-closed: call transitionToTerminal(intent.id, "resume_failed_verification", "pulls_get_error", sql) (or similar terminal state), log the error with log.error including err and intent.id, send a postReply via postReply(octokit, command, ...) informing the user that resume was aborted due to verification error, and return to abort further processing; update references in this change around octokit.rest.pulls.get, intent.target_head_sha, BOT_APP_LOGIN, transitionToTerminal, and postReply.specs/20260427-201332-pr-shepherding-merge-ready/spec.md-12-12 (1)
12-12:⚠️ Potential issue | 🟠 MajorDon’t make a private local file the normative source.
Using a local Dropbox path as “authoritative architecture” makes the spec non-verifiable for anyone else. Please inline the required normative decisions here (or link a repo-committed document) so the contract is reviewable and durable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/20260427-201332-pr-shepherding-merge-ready/spec.md` at line 12, Replace the local Dropbox path used as the “Authoritative architecture” source with an in-repo, reviewable reference: either inline the necessary normative decisions from `~/Dropbox/Private Note/20260426_pr-shepherding-merge-ready-architecture.md` directly into the spec where the “Authoritative architecture” sentence appears (including the composition S1+S3+S5 decision and the `MergeReadiness` typed verdict) or link to a committed document in the repository; also ensure FR-008 and FR-018 are explicitly resolved in the spec text rather than relying on the external note so reviewers can verify the contract.src/workflows/ship/session-runner.ts-46-47 (1)
46-47:⚠️ Potential issue | 🟠 MajorAvoid hardcoding the bot login in probe authorship checks.
BOT_APP_LOGIN = "chrisleekr-bot[bot]"bakes environment identity into runtime logic. This should come from config or dependency injection so non-default installs/environments don’t misclassify authorship.Also applies to: 101-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/ship/session-runner.ts` around lines 46 - 47, The code currently hardcodes BOT_APP_LOGIN = "chrisleekr-bot[bot]" which can misclassify authorship; replace this constant with a configurable value injected from configuration or environment (e.g., read process.env.BOT_APP_LOGIN or accept a botLogin parameter in the SessionRunner/constructor) and use that injected symbol wherever BOT_APP_LOGIN is referenced (including the probe authorship checks around the existing comparisons at the spots currently using BOT_APP_LOGIN and the logic in the 101-107 region); provide a sensible fallback default but ensure tests and callers are updated to pass the configured bot login where appropriate.src/workflows/ship/probe.ts-114-139 (1)
114-139: 🛠️ Refactor suggestion | 🟠 MajorUse the shared retry utility instead of inline retry/backoff logic.
This file now owns custom retry behavior (attempt loop + sleep + backoff indexing). Please route this through the shared
src/utils/retry helper with validated retry params to keep retry semantics consistent across workflows.As per coding guidelines:
src/**/*.{ts,tsx}: All retry logic must use the utilities insrc/utils/with validated input parameters (maxAttempts, initialDelayMs, maxDelayMs, backoffFactor).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/ship/probe.ts` around lines 114 - 139, Replace the inline retry loop in runProbe with the shared retry utility (e.g., retryWithBackoff in src/utils) by moving the GraphQL call and mergeable check into the retried callback: call input.octokit.graphql(PROBE_QUERY, {...}) inside the callback, compute verdict via computeVerdict when response.repository?.pullRequest?.mergeable is not null/UNKNOWN and return the successful { verdict, response }; map input.mergeableBackoffMs or config.mergeableNullBackoffMsList to validated retry params (maxAttempts, initialDelayMs, maxDelayMs, backoffFactor) expected by the utility, and ensure that if retries exhaust you return the lastResponse (or null) in the ProbeResult—remove the manual for-loop, sleep calls, and backoff indexing in runProbe and rely solely on the retry helper.src/workflows/ship/tracking-comment.ts-153-155 (1)
153-155:⚠️ Potential issue | 🟠 MajorUse configured trigger phrase in stop instructions.
The rendered help text hardcodes
@chrisleekr-bot. This should use runtime config to stay correct across environments and bot identities.src/workflows/ship/intent.ts-72-77 (1)
72-77:⚠️ Potential issue | 🟠 MajorConstraint-conflict detection via error-message substring is brittle.
Duplicate-session handling depends on parsing error text. If message wording changes, this path can throw instead of returning
already_in_progress. Prefer SQL-level conflict handling (e.g., conflict-tolerant insert path) or structured error-code checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/ship/intent.ts` around lines 72 - 77, The current catch block in intent.ts relies on String(err).includes("ship_intents_one_active_per_pr"), which is brittle; instead detect the DB unique-constraint failure structurally (or make the insert conflict-tolerant) and only then call dbFindActiveIntent. Concretely: in the code path that performs the insert, change logic to either use an INSERT ... ON CONFLICT DO NOTHING / DO UPDATE and then query with dbFindActiveIntent (so you return already_in_progress when a row exists), or, if keeping the throw-based flow, check the DB error object (e.g., cast err to your DB error type and compare err.code or err.constraint to the unique-constraint name "ship_intents_one_active_per_pr") and only handle that specific structured error by calling dbFindActiveIntent and returning { ok: false, reason: "already_in_progress", existing }; for any other error rethrow it. Ensure you update the catch around the insert call (the block referencing dbFindActiveIntent) to use this structured check or the conflict-tolerant insert.src/workflows/ship/session-runner.ts-131-132 (1)
131-132:⚠️ Potential issue | 🟠 Major
tracking_comment_markeris persisted as a placeholder and never patched.The comment says it will be patched once
intent_idis known, but this file never updates that column. Persisting a non-canonical marker can break future marker-based recovery logic that relies on DB metadata.Also applies to: 162-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/ship/session-runner.ts` around lines 131 - 132, The persisted tracking_comment_marker is left as a blank placeholder (tracking_comment_marker: buildIntentMarker("")) and never updated; update the logic in session-runner.ts so that after the real intent_id is created (or once createIntent/createIntentRecord completes) you compute the canonical marker via buildIntentMarker(intent_id) and persist it back to the DB (or insert it initially) by calling the relevant update/patch method used for that record (reference tracking_comment_marker and buildIntentMarker("") in this file and the function that creates the intent/intent_id); alternatively generate the intent_id before the initial persist and set tracking_comment_marker to buildIntentMarker(intent_id) so no placeholder is stored.src/workflows/ship/probe.ts-217-224 (1)
217-224:⚠️ Potential issue | 🟠 MajorReview-barrier deferral should not be encoded as
human_took_over.This remaps a timing/review-latency gate to a takeover reason, which semantically conflicts with actual manual-push takeover handling and can misroute downstream lifecycle decisions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/ship/probe.ts` around lines 217 - 224, The verdict object in probe.ts is incorrectly encoding a timing/review-latency deferral as reason "human_took_over"; change the reason value to a distinct semantic token (e.g., "review_barrier_deferred" or "review_latency_gate") in the verdict assignment so this case is not conflated with actual manual takeover, and update any downstream checks that inspect verdict.reason (search for uses of "human_took_over") to handle the new token appropriately so lifecycle logic and routing remain correct.src/workflows/ship/webhook-reactor.ts-133-190 (1)
133-190:⚠️ Potential issue | 🟠 MajorIsolate per-intent failures so one error doesn’t abort the whole fan-out batch.
Right now, any exception in one intent path stops processing remaining intents/PRs in this webhook event. That creates avoidable dropped wake/transition actions.
💡 Suggested hardening
for (const intent of intents) { - switch (event.type) { + try { + switch (event.type) { case "pull_request.synchronize": { ... break; } ... - } + } + } catch (err) { + logger.error( + { err, event: "ship.reactor.fanout_error", intent_id: intent.id, trigger: event.type }, + "ship reactor failed for intent; continuing with remaining intents", + ); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/ship/webhook-reactor.ts` around lines 133 - 190, Wrap handling of each intent in its own try/catch so one failing intent won’t abort the whole fan-out; around the inner loop that iterates intents (after findIntentsForPr) put a try { ...switch(...) { ... } } catch (err) { logger.error({ event: "ship.reactor.fanout.error", intent_id: intent.id, pr: pr_number, err }, "error processing intent"); continue; } and ensure errors from transitionToTerminal and earlyWake are caught and logged with intent_id and pr_number so processing continues for remaining intents and PRs.
🟡 Minor comments (16)
test/workflows/ship/eligibility.test.ts-99-108 (1)
99-108:⚠️ Potential issue | 🟡 MinorUse the merged-state enum in the merged-PR fixture.
Right now this case only flips
merged: truewhile leavingstate: "OPEN", so it can miss regressions ifcheckEligibility()relies on the GraphQL state field.Suggested tweak
it("rejects merged PRs", async () => { const v = await checkEligibility({ - octokit: makeOctokit({ ...okPr, merged: true }), + octokit: makeOctokit({ ...okPr, state: "MERGED", merged: true }), owner: "chrisleekr", repo: "github-app-playground", pr_number: 1,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/workflows/ship/eligibility.test.ts` around lines 99 - 108, The test is flipping only merged: true on the okPr fixture while leaving state: "OPEN", so update the merged-PR fixture used by makeOctokit/okPr to use the proper merged-state enum (or set state to "MERGED") so checkEligibility sees a consistent merged state; modify the test input where makeOctokit({ ...okPr, merged: true }) is constructed to spread a fixture that includes the merged state (e.g., use the merged-state enum constant or state: "MERGED") so both merged and state fields align when calling checkEligibility.src/webhook/events/review.ts-31-39 (1)
31-39:⚠️ Potential issue | 🟡 MinorDon't log "fired" when nothing was dispatched.
The debug line runs even when
payload.installationis absent, so non-installation review events will be reported as if the ship reactor ran.Suggested fix
- if (payload.installation !== undefined) { - fireReactor({ - type: "pull_request_review.submitted", - installation_id: payload.installation.id, - owner: payload.repository.owner.login, - repo: payload.repository.name, - pr_number: payload.pull_request.number, - }); - } - - logger.debug( - { - deliveryId, - action: payload.action, - owner: payload.repository.owner.login, - repo: payload.repository.name, - }, - "pull_request_review.submitted received → ship reactor fired", - ); + if (payload.installation !== undefined) { + fireReactor({ + type: "pull_request_review.submitted", + installation_id: payload.installation.id, + owner: payload.repository.owner.login, + repo: payload.repository.name, + pr_number: payload.pull_request.number, + }); + logger.debug( + { + deliveryId, + action: payload.action, + owner: payload.repository.owner.login, + repo: payload.repository.name, + }, + "pull_request_review.submitted received → ship reactor fired", + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/webhook/events/review.ts` around lines 31 - 39, The debug log currently always runs even when no ship reactor is dispatched; wrap or move the logger.debug call so it only executes when payload.installation is present (i.e., when the ship reactor actually runs) — check payload.installation before calling logger.debug (or place the logger.debug inside the existing installation-handling branch), keeping the same structured fields (deliveryId, payload.action, payload.repository.owner.login, payload.repository.name) and the message "pull_request_review.submitted received → ship reactor fired".docs/OBSERVABILITY.md-47-52 (1)
47-52:⚠️ Potential issue | 🟡 MinorAdd a language tag to the query fence.
This block is missing a fenced-code language, so markdownlint will keep flagging
MD040.textis enough here unless you want to label the query dialect explicitly.♻️ Proposed fix
-``` +```text event:"ship.intent.transition" to_status:"human_took_over" terminal_blocker_category:"flake-cap" | count by pr.repo -``` +```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/OBSERVABILITY.md` around lines 47 - 52, The fenced code block containing the Datadog/Loki query (the lines starting with event:"ship.intent.transition" to_status:"human_took_over" terminal_blocker_category:"flake-cap" | count by pr.repo) is missing a language tag; change the opening fence from ``` to ```text so the block becomes ```text ... ``` to satisfy markdownlint MD040 and label the snippet as plain text.src/mcp/registry.ts-53-55 (1)
53-55:⚠️ Potential issue | 🟡 MinorDocument the new MCP server in
docs/EXTENDING.md.This registration adds a new server under
src/mcp/, so the extension guide should be updated in the same PR to keep the MCP surface discoverable and maintain the repo contract. As per coding guidelines, "When a new MCP server is added insrc/mcp/, updatedocs/EXTENDING.mdin the same PR".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp/registry.ts` around lines 53 - 55, Add documentation for the newly-registered MCP server so the extension guide stays in sync: update EXTENDING.md to list the new server name "resolve_review_thread", describe the opt-in flag enableResolveReviewThread and how to enable it, and note the handler implementation entry point resolveReviewThreadServerDef (including required env/ctx setup and any configuration options). Keep the prose concise and follow the existing section format for other servers in the MCP docs.docs/SETUP.md-229-230 (1)
229-230:⚠️ Potential issue | 🟡 MinorClarify flag scope: this wording overstates
SHIP_USE_TRIGGER_SURFACES_V2gating.The sentence implies the lifecycle/reactor is gated by
SHIP_USE_TRIGGER_SURFACES_V2, but the new wake-event wiring is not tied to that flag. This may cause incorrect webhook subscription setup.Suggested wording update
-The PR shepherding reactor (`bot:ship` lifecycle, gated on `SHIP_USE_TRIGGER_SURFACES_V2=true`) needs the new `synchronize`, `closed`, `edited`, `deleted`, `check_run`, and `check_suite` subscriptions to early-wake active sessions. Existing `bot:ship` (composite) operation does not require them. +The PR shepherding reactor uses `synchronize`, `closed`, `edited`, `deleted`, `check_run`, and `check_suite` subscriptions to early-wake active sessions. `SHIP_USE_TRIGGER_SURFACES_V2=true` gates NL/label trigger surfaces, not the reactor wake wiring itself.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/SETUP.md` around lines 229 - 230, Revise the sentence to avoid implying the `SHIP_USE_TRIGGER_SURFACES_V2` flag gates the new wake-event webhook subscriptions; explicitly state that the `bot:ship` lifecycle/reactor (the `bot:ship` composite operation) may be gated by `SHIP_USE_TRIGGER_SURFACES_V2`, but the new `synchronize`, `closed`, `edited`, `deleted`, `check_run`, and `check_suite` subscriptions used to early-wake active sessions are not tied to that flag and should be registered regardless. Mention `SHIP_USE_TRIGGER_SURFACES_V2`, the `bot:ship` lifecycle/reactor, and the list of subscription event names to make the distinction clear.src/workflows/ship/log-fields.ts-55-62 (1)
55-62:⚠️ Potential issue | 🟡 MinorFix the rounding note, or switch to the intended rounding rule.
The docstring says “Banker's-rounded,” but
Math.roundis half-away-from-zero, not banker's rounding. Please update the comment or swap in the rounding behavior you actually want.♻️ Proposed fix
- * Banker's-rounded for the half-cent edge. + * Rounded to the nearest cent with `Math.round`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/ship/log-fields.ts` around lines 55 - 62, The docstring for usdToCents claims "Banker's-rounded" but the implementation uses Math.round (half-away-from-zero); either change the comment to describe half-away-from-zero rounding or implement true banker's rounding in usdToCents by using a tie-breaking rule that rounds .5 to the nearest even integer (for example detect when usd*100 has fractional part exactly 0.5 and choose the even integer). Update the usdToCents function or its comment accordingly, referencing the function name usdToCents to locate the change.specs/20260427-201332-pr-shepherding-merge-ready/tasks.md-65-66 (1)
65-66:⚠️ Potential issue | 🟡 MinorFix the scenario count to match the contract.
T010 says there are 11 probe scenarios, but
contracts/probe-graphql-query.mdcurrently lists 10. Please align the count with the actual fixture set so the checklist doesn’t drift from the contract again.♻️ Proposed fix
- covering all 11 scenarios from `contracts/probe-graphql-query.md` §"Test fixtures" + covering all 10 scenarios from `contracts/probe-graphql-query.md` §"Test fixtures"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/20260427-201332-pr-shepherding-merge-ready/tasks.md` around lines 65 - 66, Update the T010 checklist to match the actual number of probe scenarios in contracts/probe-graphql-query.md and the test fixtures: change "11 probe scenarios" to the correct count (10) or, if the contract should have 11, add the missing scenario fixture and ensure test/workflows/ship/verdict.test.ts includes it (and that contracts/probe-graphql-query.md lists it); reference T010, contracts/probe-graphql-query.md, test/workflows/ship/verdict.test.ts and the fixtures directory test/workflows/ship/fixtures/probe-responses/ when making the edit so the checklist and contract/fixtures stay in sync.specs/20260427-201332-pr-shepherding-merge-ready/quickstart.md-31-34 (1)
31-34:⚠️ Potential issue | 🟡 MinorDocument the third rollout flag here too.
The quickstart and rollback sections only list
SHIP_USE_PROBE_VERDICTandSHIP_USE_CONTINUATION_LOOP, but the PR also introducesSHIP_USE_TRIGGER_SURFACES_V2. Operators need that flag documented to toggle the new trigger-surface wiring.Also applies to: 259-261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/20260427-201332-pr-shepherding-merge-ready/quickstart.md` around lines 31 - 34, Add the missing rollout flag SHIP_USE_TRIGGER_SURFACES_V2 to the same Phase rollout flags block that currently lists SHIP_USE_PROBE_VERDICT and SHIP_USE_CONTINUATION_LOOP in quickstart.md and the corresponding rollback section; include a one-line description that it controls the new trigger-surface wiring and that it defaults to false until smoke-tested so operators can toggle it like the other flags (reference SHIP_USE_PROBE_VERDICT and SHIP_USE_CONTINUATION_LOOP when adding SHIP_USE_TRIGGER_SURFACES_V2).specs/20260427-201332-pr-shepherding-merge-ready/data-model.md-217-218 (1)
217-218:⚠️ Potential issue | 🟡 MinorKeep the partial-index examples consistent with paused intents.
Earlier in this doc, paused sessions are explicitly part of the "one in-flight session per PR" invariant, but these sections still show
WHERE status = 'active'. That contradiction makes the migration contract easy to implement incorrectly.📝 Suggested doc correction
-CREATE UNIQUE INDEX ship_intents_one_active_per_pr ON ship_intents (...) WHERE status = 'active'; -CREATE INDEX idx_ship_intents_active ON ship_intents (...) WHERE status = 'active'; +CREATE UNIQUE INDEX ship_intents_one_active_per_pr ON ship_intents (...) WHERE status IN ('active', 'paused'); +CREATE INDEX idx_ship_intents_active ON ship_intents (...) WHERE status IN ('active', 'paused');-| FR-007a | Partial unique index on `ship_intents (owner, repo, pr_number) WHERE status = 'active'`. | +| FR-007a | Partial unique index on `ship_intents (owner, repo, pr_number) WHERE status IN ('active', 'paused')`. |Also applies to: 296-296
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/20260427-201332-pr-shepherding-merge-ready/data-model.md` around lines 217 - 218, The partial-index examples are inconsistent with earlier text that counts paused sessions as part of the "one in-flight session per PR" invariant; update the WHERE clauses for the indexes (e.g., ship_intents_one_active_per_pr and idx_ship_intents_active) to include paused intents (for example change the predicate from status = 'active' to include paused, such as status IN ('active','paused') or an equivalent predicate used elsewhere) so the examples match the documented invariant.specs/20260427-201332-pr-shepherding-merge-ready/contracts/webhook-event-subscriptions.md-11-23 (1)
11-23:⚠️ Potential issue | 🟡 MinorSubscribed-event count is inconsistent with the table.
Line 11 says “5 types,” but Lines 15-22 list six distinct event types (
pull_request,issue_comment,pull_request_review,pull_request_review_comment,check_run,check_suite).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/20260427-201332-pr-shepherding-merge-ready/contracts/webhook-event-subscriptions.md` around lines 11 - 23, The header "Subscribed events (5 types, specific actions)" is wrong because the table enumerates six event types; update the header to "Subscribed events (6 types, specific actions)" or remove the numeric count entirely to avoid future drift, and ensure the phrase appears in the same heading text (the line beginning "Subscribed events (...)") so it stays consistent with the listed events (`pull_request`, `issue_comment`, `pull_request_review`, `pull_request_review_comment`, `check_run`, `check_suite`).src/workflows/ship/lifecycle-commands.ts-125-136 (1)
125-136:⚠️ Potential issue | 🟡 MinorSuccess reply should reflect guarded transition result.
pauseIntent(...)/resumeIntent(...)can returnnullunder races (guarded UPDATE no-op), but current replies always state success. This can report an incorrect final state to users.Also applies to: 184-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/ship/lifecycle-commands.ts` around lines 125 - 136, pauseIntent and resumeIntent can return null on guarded no-op updates; modify the handling after calling pauseIntent(intent.id, command.principal_login, sql) (and similarly after resumeIntent) to inspect the returned value and send a conditional reply via postReply: if the result is null, log an info/warn with intent_id and that the transition was guarded/no-op and post a reply indicating the session was already paused/resumed (instead of reporting success); if non-null, proceed with the existing success log and success reply. Use the existing symbols pauseIntent, resumeIntent, postReply, octokit, command, log, and intent to implement the branching.specs/20260427-201332-pr-shepherding-merge-ready/research.md-156-157 (1)
156-157:⚠️ Potential issue | 🟡 MinorMerged-case terminal state is mislabeled in R6 rationale.
Line 156 maps merged externally to
pr_closed; that should bemerged_externally(withpr_closedfor non-merged close).✏️ Suggested wording fix
-- `pull_request.closed` is the reactor's signal to terminate active intents with `SessionTerminalState = pr_closed` (merged externally) or `pr_closed` (just closed) — distinguishable by `merged: true/false` in the payload. +- `pull_request.closed` is the reactor's signal to terminate active intents with `SessionTerminalState = merged_externally` (merged externally) or `pr_closed` (just closed) — distinguishable by `merged: true/false` in the payload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/20260427-201332-pr-shepherding-merge-ready/research.md` around lines 156 - 157, The R6 rationale incorrectly maps the merged-case terminal state: update the text that describes the `pull_request.closed` reactor signal so that when the payload has `merged: true` the `SessionTerminalState` is `merged_externally` and when `merged: false` it is `pr_closed`; find the sentence referencing `pull_request.closed` and replace the current `pr_closed` mapping for merged externally with `merged_externally`, keeping the note about restricting actions per event type intact.src/workflows/ship/flake-tracker.ts-91-93 (1)
91-93:⚠️ Potential issue | 🟡 MinorRerun failures are silently swallowed.
Line 91 catches and drops all errors with no reporting path, making rerun failures invisible in ops/debug workflows.
♻️ Suggested fix (preserve best-effort behavior, expose failures)
export interface TriggerTargetedRerunInput { readonly octokit: Pick<Octokit, "rest">; readonly owner: string; readonly repo: string; readonly checks: readonly FlakedCheck[]; + readonly onError?: (error: unknown, check: FlakedCheck) => void; } @@ - } catch { - // Swallowed — see fn JSDoc. + } catch (error) { + input.onError?.(error, check); } } }As per coding guidelines,
src/**/*.{ts,tsx}must use structured JSON logging via pino with child loggers per request.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/ship/flake-tracker.ts` around lines 91 - 93, The empty catch block that swallows rerun errors (the bare "catch { ... }" in src/workflows/ship/flake-tracker.ts) should be replaced to capture the exception and emit structured JSON pino logging using the request-scoped child logger: change to "catch (err)" and call the request's pino child logger (or create one if only a module logger exists) with an error-level log including the error object and relevant context (e.g., flake id, attempt number) so failures are visible, but keep the original best-effort behavior (do not rethrow unless the surrounding logic requires it).src/db/queries/ship.ts-297-303 (1)
297-303:⚠️ Potential issue | 🟡 MinorCommented return contract doesn’t match query output.
The doc says this helper returns continuation rows plus PR routing tuple via join, but the query returns only
c.*. Please either include the joined columns in the return type/query or adjust the comment.Also applies to: 307-315
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/queries/ship.ts` around lines 297 - 303, The comment for findDueContinuations claims the function returns continuation rows plus the PR routing tuple from a join with ship_intents, but the SQL only selects c.*; fix by either updating the SQL in findDueContinuations to select the needed joined columns (e.g., c.*, si.installation_id, si.owner, si.repo, si.pr_number from ship_intents AS si) and update the function's return type accordingly, or change the docstring to accurately state that only continuation rows (c.*) are returned; reference the findDueContinuations function and the ship_intents join when making the change.specs/20260427-201332-pr-shepherding-merge-ready/spec.md-141-141 (1)
141-141:⚠️ Potential issue | 🟡 MinorTighten wording: “final outcome” is redundant.
Use just “outcome” for cleaner phrasing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/20260427-201332-pr-shepherding-merge-ready/spec.md` at line 141, Update the FR-014a requirement text to remove the redundant word “final” and use “outcome” instead: locate the requirement labeled "FR-014a" (the sentence that currently reads "...listing each check's failure count and final outcome") and change "final outcome" to "outcome" so the line reads "...listing each check's failure count and outcome".src/workflows/ship/session-runner.ts-141-142 (1)
141-142:⚠️ Potential issue | 🟡 MinorPotential malformed mention in already-in-progress reply.
The message prepends
@toconfig.triggerPhrase; iftriggerPhrasealready includes@, this renders@@....✏️ Small fix
- `To stop it, comment \`@${config.triggerPhrase} bot:abort-ship\`.`, + `To stop it, comment \`${config.triggerPhrase} bot:abort-ship\`.`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/ship/session-runner.ts` around lines 141 - 142, The reply message concatenates `@` with config.triggerPhrase which can produce a double-@ if triggerPhrase already contains one; update the message assembly in session-runner.ts (where the template references config.triggerPhrase and result.existing.id) to normalize the trigger phrase by trimming a leading '@' or conditionally prefixing '@' only when absent (e.g., compute a safeTrigger = config.triggerPhrase.startsWith('@') ? config.triggerPhrase : '@' + config.triggerPhrase and use safeTrigger in the template) so the mention is never rendered as `@@...`.
- resolve-review-thread mcp: do a read-only preflight query to verify the thread belongs to the bound pr before issuing the resolve mutation. the previous flow ran the mutation first and only rejected cross-pr threads after the side effect had already happened. - eligibility: fr-015/fr-028 require the gate to evaluate the triggering principal only. drop the ownerOk fallback, which let any commenter trigger the bot whenever the repo owner happened to be in ALLOWED_OWNERS. - tickle-scheduler: requeue the intent on dispatch failure. ZREM ran before onDue, so a thrown onDue stranded the session until startup reconciliation re-ran. requeue with a one-interval delay keeps the session live without tight-looping a known-bad dispatch. - test-isolated.sh: fail fast when the test glob matches nothing. nullglob silently turned an empty match into a successful no-op, hiding a deleted test directory or broken glob behind a green ci.
Local Spec Kit artifact that should not have been committed. Add to .gitignore and remove from the index; file remains on disk. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Records 2026-04-28 clarification session decisions: - bot:fix-thread (FR-029): per-review-thread mechanical fix, no session row - bot:explain-thread (FR-030): reply-only thread explanation - bot:summarize (FR-031): single canonical PR-summary comment - bot:rebase (FR-032): merge-base-into-head, never force-push - bot:investigate (FR-033): structured issue analysis comment - bot:triage (FR-034): suggestion-only label/severity/dup proposals - bot:open-pr (FR-035): draft PR creation from issue with dedup guard Plus User Story 5, 5 new edge cases, NL classifier intent enum widened to 12 entries, label set extended. Removes shipUseTriggerSurfacesV2 flag (NL + label surfaces become permanent in v1). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
T001–T009 (config schema, ship-types module, migration 008, ship queries, migration test, queries test, CI wiring) all landed in PR #75 but were never marked complete. Audit confirmed via filesystem: src/config.ts, src/shared/ship-types.ts, src/db/migrations/008_ship_intents.sql, src/db/queries/ship.ts, test/db/migrations/008.test.ts, test/db/queries/ship.test.ts. No code changes. No status changes for Phase 3+ — those open tasks are genuinely open (intent.ts, continuation.ts, webhook-reactor.ts, nl-classifier.ts, trigger-router.ts ship without unit tests, per T012/T013/T014b/T014c/T037). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Surgical append (not regeneration) across the four planning artifacts to layer the seven new scoped commands and the SHIP_USE_TRIGGER_SURFACES_V2 flag removal on top of the in-progress shepherding plan, preserving existing T001–T071 task IDs and statuses. plan.md: new P8 row in the Phased Delivery table; P8 architecture notes covering one-shot lifecycle, concurrency with ship sessions, trigger plumbing reuse, bot:open-pr meta-issue classifier (separate LLM call), bot:triage suggest-only constraint, bot:rebase forward-only guarantee, marker schemes distinct from FR-006. tasks.md: T071 amended to drop SHIP_USE_TRIGGER_SURFACES_V2 (pulled forward into T072). New Phase 8 (T072–T092, 21 tasks): T072 flag removal; T073–T079 per-command tests written first per Constitution V; T080–T081 extend nl-classifier intent enum (5→12) and label-trigger label set; T082–T088 per-command implementation modules; T089–T091 webhook event wiring (review-comment, issues, issue-comment filter); T092 local-e2e covering all four User Story 5 acceptance scenarios. Phase Dependencies and Parallel Opportunities sections extended. data-model.md: new section noting scoped commands write zero ship_* rows by design; marker scheme table for read-only commands; intent enum widening with per-event-surface eligibility; bot:open-pr meta-issue classifier verdict type. contracts/bot-commands.md: common conventions block for scoped commands; seven new command sections (bot:fix-thread, bot:explain-thread, bot:summarize, bot:rebase, bot:investigate, bot:triage, bot:open-pr) each with syntax / trigger-surface eligibility / behavior / response artifact / safety guarantees. bot:triage suggest-only enforced two ways (ESLint no-restricted-imports + runtime mock assertions). Final shared scoped-command test block listing the seven cross-cutting test obligations. Phase 8 is independently shippable from P1–P7; depends only on US1 trigger-router scaffolding (T028a/T028b/T028c/T028f). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Bucket A (correctness/security):
- nl-classifier: prefix-only mention gate per FR-025a; previous indexOf
let quoted/log-pasted trigger phrases pay LLM tokens.
- signature: ANSI_ESCAPE regex was missing the \x1b prefix and was
stripping ordinary bracketed log text (`[error]`), destabilising
Tier-1 signature extraction.
- pull-request: BOT_LABEL_PATTERN broadened to allow hyphens and
`/deadline=Nh|m|s` so documented labels (`bot:abort-ship`,
`bot:fix-thread`, etc.) actually dispatch.
- tickle-scheduler: reentrancy guard against overlapping setInterval
ticks (could otherwise dispatch the same intent twice on slow Valkey).
- session-runner: drop the duplicate `@` in the already-in-progress
reply (`@${triggerPhrase}` rendered `@@chrisleekr-bot`).
- tracking-comment: stop instructions now use `config.triggerPhrase`
instead of the hardcoded production mention.
- lifecycle-commands: resume foreign-push check fails closed — declines
with retry hint instead of silently bypassing the safety gate.
- webhook-reactor: per-intent try/catch isolates failures across the
fan-out batch; processing extracted into helpers to satisfy max-depth.
- intent: structured unique-violation detection — Bun.sql exposes
SQLSTATE on `errno` and the constraint name on `constraint`; replaces
brittle `String(err).includes(...)` matching.
- flake-tracker: rerun failures now log via pino instead of bare catch.
- flake-tracker + probe + verdict: GraphQL CheckRun nodes now request
`databaseId`, projected probe history populates `check_run_id` so
`triggerTargetedRerun` can actually rerequest projected flakes.
- check-no-destructive-actions: walk() fails closed on filesystem
errors (CI safety guard cannot silently skip a missing scan root).
Bucket B (config-driven constants):
- config: new `botAppLogin` field (default `chrisleekr-bot[bot]`,
override via `BOT_APP_LOGIN`); replaces three hardcoded copies in
`lifecycle-commands.ts`, `session-runner.ts`, `reactor-bridge.ts`.
- review.ts: log "ship reactor fired" only when the reactor actually
fired (was logging on every review event regardless of installation).
Bucket D — targeted picks:
- verdict: new `review_barrier_deferred` reason so the latency-gate
deferral isn't misencoded as `human_took_over`.
- command-dispatch: try/catch around literal+NL classifier so a
Bedrock/parser outage doesn't bubble out of the webhook handler.
- eligibility test: align fixture to use `state: "MERGED"` enum.
- lifecycle-commands: pause/resume reply now reflects the guarded
UPDATE result — null no-op no longer reports false success.
- review-barrier: probe carries `author.__typename`; barrier excludes
any non-`User` actor so other bots/Apps cannot satisfy the human
review barrier.
- session-runner: `tracking_comment_marker` is patched to the
canonical `<!-- ship-intent:{id} -->` form right after the row is
created, instead of leaving a placeholder in the column.
Bucket C (docs/spec sweep):
- spec.md authoritative-architecture line inlined (removes the private
Dropbox-path normative reference).
- contracts/probe-graphql-query.md updated for review-author typename.
- contracts/mcp-resolve-thread-server.md error payload now includes
`pr_number`; security table re-formatted (was malformed).
- contracts/webhook-event-subscriptions.md "5 types" → 6 types.
- data-model.md partial-index examples + FR-007a row use
`WHERE status IN ('active', 'paused')`.
- tasks.md probe scenario count 11 → 10.
- research.md merged_externally vs pr_closed mapping fix.
- spec.md "final outcome" → "outcome" nit.
- log-fields.ts "Banker's-rounded" comment corrected.
- db/queries/ship.ts findDueContinuations docstring.
- docs/EXTENDING.md adds existing-MCP-servers reference table covering
`resolve_review_thread`.
- docs/SETUP.md flag scope wording — `SHIP_USE_TRIGGER_SURFACES_V2`
gates the trigger-router only, not the wake subscriptions.
- quickstart.md documents the `SHIP_USE_TRIGGER_SURFACES_V2` flag in
the env-var block + rollback list.
Skipped (with rationale):
- probe.ts retry → shared retry util: `mergeableNullBackoffMsList` is
a fixed schedule (number[]), not exponential; the shared util's
validated parameters (initialDelayMs/backoffFactor) don't fit.
- issue-comment dual-dispatch dedup: the `SHIP_USE_TRIGGER_SURFACES_V2`
flag is being removed in a subsequent iteration; adding a guard now
is throwaway code that the flag-removal will collapse.
- verdict.test.ts missing-PR test relocation: the underlying behaviour
(encoding "PR not found" as `human_took_over`) is consistent with
existing terminal-state handling; moving the test alone wouldn't
address the semantic concern, and changing the verdict cascades
outside the scope of this PR.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai — addressed your major+minor batch in 32dc584: Fixed (A bucket — correctness/security): nl-classifier prefix-only mention gate, signature.ts ANSI escape regex (added Fixed (B bucket — config-driven constants): new Fixed (D — targeted): new Fixed (C — docs/spec): spec.md inlines the architecture decisions (removes the private Dropbox path); probe-graphql-query.md, mcp-resolve-thread-server.md, webhook-event-subscriptions.md, data-model.md, tasks.md, research.md, quickstart.md, SETUP.md, EXTENDING.md, log-fields.ts comment, db/queries/ship.ts comment — all aligned. Skipped with rationale (in commit message):
|
|
Tip For best results, initiate chat on the files or code changes.
A / B / D / C — all accepted. The fixes land exactly where the review comments pointed:
Skipped items — rationale accepted:
`. The marker is patched into the canonical form after comment creation (post-create patch) to embed the real intent_id. ✏️ Learnings added
🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 24
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
specs/20260427-201332-pr-shepherding-merge-ready/contracts/bot-commands.md (1)
514-523: 🧹 Nitpick | 🔵 TrivialMinor: Inconsistent British/American spelling.
The document mixes "authorisation" (British) and "authorization/unauthorized" (American) spellings in lines 516-518. Consider standardizing to one variant for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/20260427-201332-pr-shepherding-merge-ready/contracts/bot-commands.md` around lines 514 - 523, The document mixes British and American spellings for authorization; normalize all occurrences to a single variant (pick one and apply consistently) — e.g., change "authorisation/authorised" to "authorization/authorized" (or vice versa) across the bot-commands.md content, including the "Authorization gate (FR-028)" header and the list items describing authorization/unauthorized behavior, and ensure related tokens (e.g., "authorization gate", "unauthorized principal", "self-removal behaviour") are updated for consistent spelling throughout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/EXTENDING.md`:
- Line 230: Update the docs table row for `resolve_review_thread` to point to
the correct handler path under the ship workflow; change the "Wired by"
reference from `src/workflows/handlers/resolve.ts` to the ship handler location
(e.g. `src/workflows/ship/resolve.ts`) so readers are directed to the actual
`resolve_review_thread` implementation.
In `@docs/SETUP.md`:
- Around line 216-232: Update the contradictory note below the event table so it
no longer claims handlers like src/webhook/events/pull-request.ts and
src/webhook/events/review.ts are “placeholders”; instead state that those files
are active event handlers wired to the listed subscriptions and describe any
remaining placeholder behavior (if any) — e.g., call out that pull-request.ts
handles pull_request.{opened,labeled,synchronize,closed} and review.ts handles
pull_request_review.submitted, while noting explicitly if any handler files are
intentionally stubbed for future work. Keep wording consistent with the table
and the paragraph about reactor wake subscriptions and the
SHIP_USE_TRIGGER_SURFACES_V2 gating.
In `@scripts/check-no-destructive-actions.ts`:
- Around line 64-80: Current line-by-line regex scanning (using FORBIDDEN and
pushing to violations per line) is evadable and brittle; change logic to run
each forbidden rule.pattern against the full normalized file text (e.g.,
collapse CRLF, normalize whitespace/newlines and remove or neutralize block
comments) to find multi-line matches, then convert match.start offsets back to
file line numbers to populate violations with correct file and line, preserving
rule.description and matched text; ensure the comment filter correctly ignores
both line and block comments before scanning so FR-009 and other multi-line
patterns are detected robustly.
- Around line 23-37: The FORBIDDEN list is too narrow and misses common
destructive forms; update the FORBIDDEN array to add regex entries that catch
any "git reset --hard" with any target (e.g., HEAD or a SHA), remote
branch-deletion forms like "git push <remote> --delete <branch>", "git push
<remote> :<branch>" and the shorthand "git push --delete", and include lowercase
branch deletion "git branch -d" (in addition to -D); make sure each new pattern
is case-insensitive and add clear descriptions (e.g., "git reset --hard
<target>", "git push --delete <branch>", "git push <remote>:<branch>", "git
branch -d") so the FR-009 guard correctly flags these operations.
In
`@specs/20260427-201332-pr-shepherding-merge-ready/contracts/probe-graphql-query.md`:
- Line 127: Update the contract text so the reviewer-qualification rule matches
runtime behavior: change the description for
commits.nodes[0].commit.committedDate (FR-023) to state that qualifying reviews
are limited to actor type User only (i.e., reviews where
review.author.__typename === 'User'), excluding App/bot/mannequin actors;
reference the implementation in src/workflows/ship/review-barrier.ts and the
author.__typename check to ensure the contract language mirrors the code logic.
In
`@specs/20260427-201332-pr-shepherding-merge-ready/contracts/webhook-event-subscriptions.md`:
- Around line 57-62: Spec and implementation disagree on the async deferral
pattern: the spec prescribes setImmediate(() => reactor.fanOut(event)) while the
implementation uses a void async IIFE that calls fanOut(event, ...). Update
either the spec or code for consistency—either change the implementation in
reactor-bridge (the async IIFE that invokes fanOut) to use setImmediate(() =>
reactor.fanOut(event)) to explicitly defer to the next event-loop tick, or
update the spec text to accept the current pattern (void async IIFE calling
fanOut) and note that it runs as a microtask after synchronous completion; refer
to reactor.fanOut(event) and the async IIFE in
src/workflows/ship/reactor-bridge.ts when making the change.
In `@specs/20260427-201332-pr-shepherding-merge-ready/data-model.md`:
- Around line 149-151: The CHECK constraint for non_readiness_reason is missing
the review_barrier_deferred value, causing enum/constraint drift; update the
constraint that currently reads CHECK (non_readiness_reason IS NULL OR
non_readiness_reason IN ('failing_checks', 'open_threads', 'changes_requested',
'behind_base', 'mergeable_pending', 'pending_checks', 'human_took_over')) to
include 'review_barrier_deferred' so persistence accepts deferrals recorded by
the review-latency logic in src/workflows/ship/review-barrier.ts; apply the same
addition to the other identical CHECKs mentioned (around the other occurrences
noted).
In `@specs/20260427-201332-pr-shepherding-merge-ready/plan.md`:
- Line 180: Replace the non-standard word "markered" with "marked" (or "use
markers") in the sentence describing the bot posts so the phrase reads e.g.
"**`bot:summarize` and `bot:investigate` posts are marked**" and keep the
existing examples (`<!-- bot:summarize:{pr_number} -->`, `<!--
bot:investigate:{issue_number} -->`) and the FR-006 reference unchanged; update
the file's sentence where `bot:summarize` / `bot:investigate` and FR-006 are
mentioned.
In `@src/mcp/servers/resolve-review-thread.ts`:
- Around line 140-165: Wrap both octokit.graphql calls (the preflight
GET_THREAD_QUERY call that assigns preflight and the RESOLVE_MUTATION call that
assigns result) in the project's retry/backoff helper (e.g., retryWithBackoff)
instead of calling octokit.graphql directly; implement the helper to treat
secondary rate-limit errors with one retry and transient network errors with up
to 3 retries, and call it like retryWithBackoff(() =>
octokit.graphql<PreflightResponse>(GET_THREAD_QUERY, { threadId })) and
retryWithBackoff(() => octokit.graphql<ResolveResponse>(RESOLVE_MUTATION, {
threadId })) so both queries share the same backoff behavior.
- Around line 145-159: The error return object in resolve-review-thread
currently always emits code: "graphql_error" and omits pr_number; update both
failure branches (the block that references preflightPr and the similar block at
lines 183-190) to return the documented error schema: include a clear error code
(e.g., use "thread_not_found" when preflightPr === undefined and "pr_mismatch"
when the thread belongs to a different PR), include pr_number (set to
preflightPr when available or BOUND_PR_NUMBER when reporting the mismatch), and
retain thread_id and message fields so callers get the bound-PR context they
rely on. Ensure you change the returned object constructed in the function that
builds the content array (the JSON.stringify payload) so both branches follow
this schema.
- Around line 54-65: Update the GraphQL preflight to fetch and validate
repository identity: extend GET_THREAD_QUERY to include the thread's repository
owner and name (via pullRequest.repository.owner.login and
pullRequest.repository.name or equivalent fields), then in the preflight logic
that currently checks only pullRequest.number (before calling RESOLVE_MUTATION)
compare those returned owner/name values against REPO_OWNER and REPO_NAME and
reject/throw if they mismatch; apply the same owner/repo validation to the other
preflight branch referenced around the RESOLVE_MUTATION usage (the code block
noted at 140-156) so you never resolve a thread based solely on PR number from a
different repository.
- Around line 196-204: The startup currently swallows connection errors by only
logging in runServer().catch(console.error), so if server.connect(...) rejects
the process may exit with status 0; change the catch to log the error and
terminate the process with a non-zero code. Specifically, update the top-level
promise handling for runServer() so that any rejection from server.connect (or
runServer) is logged via console.error (or processLogger) and then call
process.exit(1); keep the existing exit handler that calls server.close() inside
runServer and ensure you still call server.close() on failure if needed (use
server.close() before exiting or ensure cleanup is triggered).
In `@src/webhook/events/pull-request.ts`:
- Around line 25-27: The inline comment in the pull-request handler incorrectly
states a single app.webhooks.on("pull_request", ...) registration; update that
comment to reflect the current wiring in app.ts which uses explicit action/event
registrations (refer to the actual registration style in app.ts) — locate the
comment in the pull-request handler (the block mentioning
app.webhooks.on("pull_request", ...) in pull-request.ts) and change the text to
say the handler is covered by explicit action/event registrations in app.ts
rather than a single webhook registration.
In `@src/workflows/ship/command-dispatch.ts`:
- Around line 118-123: The catch block in command-dispatch.ts is converting the
caught error to String(err) which drops stack and properties; update the catch
to log the full error object instead (e.g., pass err directly or include
err.stack when available) in the (input.log ?? rootLogger).error call so the
event "ship.dispatch_comment_surface_failed" preserves error details; locate the
catch around dispatchCommentSurface invocation and replace String(err) with the
actual error object (or a normalized object containing message and stack) when
calling the logger.
In `@src/workflows/ship/lifecycle-commands.ts`:
- Around line 213-223: The abort branch is implicit; make it explicit by
checking the command intent before executing the abort flow: add a conditional
like if (command.intent === "abort") around the block that calls
requestAbort(intent.id, valkey), sleep(POST_FLAG_WAIT_MS),
forceAbortIntent(intent.id, command.principal_login, sql), log.info(...), and
postReply(...), and handle unexpected intents (e.g., throw or log an error) in
an else branch; reference symbols to change: CanonicalCommand.intent, valkey,
requestAbort, forceAbortIntent, POST_FLAG_WAIT_MS, log.info, and postReply to
locate and update the code.
- Around line 154-177: The foreign-push detection is using pr.head.user?.login
which is the head repo owner, not the commit pusher; replace that check by
fetching the actual commit for intent.target_head_sha via
octokit.rest.repos.getCommit (use owner/repo and pull SHA =
intent.target_head_sha) and inspect the returned commit author/committer (e.g.,
response.data.author?.login or response.data.commit.author/email) to compare
against config.botAppLogin; update the conditional in the same block (around
lifecycle-commands.ts where pr.head.sha !== intent.target_head_sha) to call
repos.getCommit, use that commit actor to decide if a human pushed, and then
call transitionToTerminal and postReply as before when a non-bot actor is
detected.
In `@src/workflows/ship/probe.ts`:
- Around line 211-227: The cast "result.response as never" suppresses type
safety; remove that cast and make types align by either updating
shouldDeferOnReviewLatency to accept the actual ProbeResponseShape (or a
narrower interface describing the fields used) or by explicitly
extracting/transforming the necessary subset from result.response and passing
that correctly typed object to shouldDeferOnReviewLatency; locate the call site
(result.response and shouldDeferOnReviewLatency) and update the function
signature or create a typed extractor to avoid using "as never" while keeping
the same runtime logic when input.applyReviewBarrier is present.
- Around line 267-269: The catch block swallowing errors after the "audit-row"
persistence attempt should log the error instead of dropping it; locate the
try/catch around the audit-row persistence in src/workflows/ship/probe.ts and
update the catch to log the caught error (preferably at debug or warn level)
using the module's logger (e.g., logger.warn or probeLogger.warn) and include
contextual text like "audit-row persistence failed" and any identifying ids; if
no logger is available, fall back to console.warn to ensure the diagnostic is
recorded.
In `@src/workflows/ship/reactor-bridge.ts`:
- Around line 18-19: When getDb() returns null in reactor-bridge.ts (the
early-return path), add a debug-level log immediately before the return that
states the reactor wake is being skipped due to missing DB and includes any
available context (e.g., reactor id or call site variables); use the module's
existing logger (e.g., processLogger or logger) if present, otherwise fall back
to console.debug, then return as before.
In `@src/workflows/ship/session-runner.ts`:
- Around line 312-388: terminalReady currently omits the sql DB handle and calls
transitionToTerminal relying on the default requireDb(); change terminalReady to
accept an sql parameter (same type as produced by requireDb()), pass that sql
through to transitionToTerminal, and update any callers (e.g.,
runShipFromCommand) to pass their local sql when invoking terminalReady so DB
usage is consistent with other functions like
updateTrackingComment/renderTrackingComment and avoids implicit requireDb()
calls.
In `@src/workflows/ship/signature.ts`:
- Around line 42-45: The first RegExp in TIER1_PATTERNS is too permissive and
can stop early, capturing message text (e.g., "console") instead of the ESLint
rule id and it doesn't allow scoped rule names; update the pattern in the
TIER1_PATTERNS array so the first capture remains a non-greedy message (.+?) but
the second capture explicitly matches ESLint rule ids including optional scope
(e.g. `@scope/name`) and hyphens/digits — something like: after the message
capture, match a rule-id token that allows an optional "@scope/" prefix and then
a name composed of letters, digits and hyphens; replace the existing RegExp
entry in TIER1_PATTERNS with this stricter pattern so scoped rules like
`@typescript-eslint/no-unused-vars` and single-name rules are correctly captured.
In `@src/workflows/ship/verdict.ts`:
- Around line 40-42: Move the "import { z } from 'zod';" statement to the top of
the file with the other imports so it precedes any declarations; specifically
update the import ordering for the module that defines NonReadinessReasonSchema
(the z usage in the line creating NonReadinessReasonSchema) so the zod import
appears before that export, keeping existing import grouping and sorting
conventions.
In `@test/workflows/ship/eligibility.test.ts`:
- Around line 52-110: Add two tests that exercise the new "unauthorized" and
"forbidden_target_branch" verdicts by setting the relevant env vars and calling
checkEligibility: (1) set process.env["ALLOWED_OWNERS"] to a value that does NOT
include the PR owner (use the existing makeOctokit and okPr fixtures) and assert
eligible === false and reason === "unauthorized"; (2) set
process.env["SHIP_FORBIDDEN_TARGET_BRANCHES"] to include the PR's target/base
branch (use okPr.baseRef or override baseRef in makeOctokit) and assert eligible
=== false and reason === "forbidden_target_branch". Use the same call pattern to
checkEligibility as other tests so existing beforeEach/afterEach env cleanup
works.
---
Outside diff comments:
In `@specs/20260427-201332-pr-shepherding-merge-ready/contracts/bot-commands.md`:
- Around line 514-523: The document mixes British and American spellings for
authorization; normalize all occurrences to a single variant (pick one and apply
consistently) — e.g., change "authorisation/authorised" to
"authorization/authorized" (or vice versa) across the bot-commands.md content,
including the "Authorization gate (FR-028)" header and the list items describing
authorization/unauthorized behavior, and ensure related tokens (e.g.,
"authorization gate", "unauthorized principal", "self-removal behaviour") are
updated for consistent spelling throughout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2fed183c-adbe-49d2-8e04-18d4e89426d1
📒 Files selected for processing (39)
.gitignoredocs/EXTENDING.mddocs/OBSERVABILITY.mddocs/SETUP.mdscripts/check-no-destructive-actions.tsscripts/test-isolated.shspecs/20260427-201332-pr-shepherding-merge-ready/contracts/bot-commands.mdspecs/20260427-201332-pr-shepherding-merge-ready/contracts/mcp-resolve-thread-server.mdspecs/20260427-201332-pr-shepherding-merge-ready/contracts/probe-graphql-query.mdspecs/20260427-201332-pr-shepherding-merge-ready/contracts/webhook-event-subscriptions.mdspecs/20260427-201332-pr-shepherding-merge-ready/data-model.mdspecs/20260427-201332-pr-shepherding-merge-ready/plan.mdspecs/20260427-201332-pr-shepherding-merge-ready/quickstart.mdspecs/20260427-201332-pr-shepherding-merge-ready/research.mdspecs/20260427-201332-pr-shepherding-merge-ready/spec.mdspecs/20260427-201332-pr-shepherding-merge-ready/tasks.mdsrc/config.tssrc/db/queries/ship.tssrc/mcp/servers/resolve-review-thread.tssrc/webhook/events/pull-request.tssrc/webhook/events/review.tssrc/workflows/ship/command-dispatch.tssrc/workflows/ship/eligibility.tssrc/workflows/ship/flake-tracker.tssrc/workflows/ship/intent.tssrc/workflows/ship/lifecycle-commands.tssrc/workflows/ship/log-fields.tssrc/workflows/ship/nl-classifier.tssrc/workflows/ship/probe.tssrc/workflows/ship/reactor-bridge.tssrc/workflows/ship/review-barrier.tssrc/workflows/ship/session-runner.tssrc/workflows/ship/signature.tssrc/workflows/ship/tickle-scheduler.tssrc/workflows/ship/tracking-comment.tssrc/workflows/ship/verdict.tssrc/workflows/ship/webhook-reactor.tstest/integration/repo-knowledge.test.tstest/workflows/ship/eligibility.test.ts
| | ----------------------- | --------- | ----------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------- | | ||
| | `comment_update` | stdio | Writes/updates GitHub PR/issue comments owned by the bot. | Always on. | | ||
| | `inline_comments` | stdio | Posts inline review comments and replies on PR diffs. | Always on. | | ||
| | `resolve_review_thread` | stdio | Resolves a single PR review thread the bot has just replied to. Bound to one `(owner, repo, pullNumber)` per server instance. | Wired by `src/workflows/handlers/resolve.ts` for the resolve iteration only — not added to a session's allowed-tools by default. | |
There was a problem hiding this comment.
Fix the resolve handler path in docs.
Line 230 points to src/workflows/handlers/resolve.ts, but ship handlers in this PR are under src/workflows/ship/…. Please update the path to avoid sending contributors to the wrong location.
Proposed doc fix
-| `resolve_review_thread` | stdio | Resolves a single PR review thread the bot has just replied to. Bound to one `(owner, repo, pullNumber)` per server instance. | Wired by `src/workflows/handlers/resolve.ts` for the resolve iteration only — not added to a session's allowed-tools by default. |
+| `resolve_review_thread` | stdio | Resolves a single PR review thread the bot has just replied to. Bound to one `(owner, repo, pullNumber)` per server instance. | Wired by `src/workflows/ship/handlers/resolve.ts` for the resolve iteration only — not added to a session's allowed-tools by default. |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/EXTENDING.md` at line 230, Update the docs table row for
`resolve_review_thread` to point to the correct handler path under the ship
workflow; change the "Wired by" reference from
`src/workflows/handlers/resolve.ts` to the ship handler location (e.g.
`src/workflows/ship/resolve.ts`) so readers are directed to the actual
`resolve_review_thread` implementation.
Comment on lines
+23
to
+37
| const FORBIDDEN: { readonly pattern: RegExp; readonly description: string }[] = [ | ||
| { pattern: /git\s+push\s+--force(?!-with-lease-if)/i, description: "git push --force" }, | ||
| { pattern: /git\s+push\s+--force-with-lease/i, description: "git push --force-with-lease" }, | ||
| { pattern: /git\s+push\s+-f\b/i, description: "git push -f" }, | ||
| { pattern: /git\s+push\s+\+/, description: "git push with + force-refspec" }, | ||
| { pattern: /git\s+push\s+--mirror/i, description: "git push --mirror" }, | ||
| { pattern: /git\s+reset\s+--hard\s+origin/i, description: "git reset --hard origin/<branch>" }, | ||
| { pattern: /git\s+branch\s+-D\b/, description: "git branch -D" }, | ||
| { pattern: /git\s+filter-branch/i, description: "git filter-branch" }, | ||
| { pattern: /git\s+filter-repo/i, description: "git filter-repo" }, | ||
| { pattern: /git\s+replace\b/i, description: "git replace" }, | ||
| { pattern: /\bgh\s+pr\s+merge/i, description: "gh pr merge" }, | ||
| { pattern: /mergePullRequest\s*\(/, description: "mergePullRequest GraphQL mutation" }, | ||
| { pattern: /mergeBranch\s*\(/, description: "mergeBranch GraphQL mutation" }, | ||
| ]; |
There was a problem hiding this comment.
The forbidden patterns are narrower than the policy.
git reset --hard HEAD, git reset --hard <sha>, and remote branch deletion like git push origin --delete foo all bypass this list even though the header says hard resets and branch deletion are forbidden. That leaves the FR-009 guard returning a false clean result.
Suggested tightening
- { pattern: /git\s+reset\s+--hard\s+origin/i, description: "git reset --hard origin/<branch>" },
- { pattern: /git\s+branch\s+-D\b/, description: "git branch -D" },
+ { pattern: /git\s+reset\s+--hard\b/i, description: "git reset --hard" },
+ { pattern: /git\s+branch\s+-D\b/i, description: "git branch -D" },
+ { pattern: /git\s+push\b.*\s--delete\b/i, description: "git push --delete" },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const FORBIDDEN: { readonly pattern: RegExp; readonly description: string }[] = [ | |
| { pattern: /git\s+push\s+--force(?!-with-lease-if)/i, description: "git push --force" }, | |
| { pattern: /git\s+push\s+--force-with-lease/i, description: "git push --force-with-lease" }, | |
| { pattern: /git\s+push\s+-f\b/i, description: "git push -f" }, | |
| { pattern: /git\s+push\s+\+/, description: "git push with + force-refspec" }, | |
| { pattern: /git\s+push\s+--mirror/i, description: "git push --mirror" }, | |
| { pattern: /git\s+reset\s+--hard\s+origin/i, description: "git reset --hard origin/<branch>" }, | |
| { pattern: /git\s+branch\s+-D\b/, description: "git branch -D" }, | |
| { pattern: /git\s+filter-branch/i, description: "git filter-branch" }, | |
| { pattern: /git\s+filter-repo/i, description: "git filter-repo" }, | |
| { pattern: /git\s+replace\b/i, description: "git replace" }, | |
| { pattern: /\bgh\s+pr\s+merge/i, description: "gh pr merge" }, | |
| { pattern: /mergePullRequest\s*\(/, description: "mergePullRequest GraphQL mutation" }, | |
| { pattern: /mergeBranch\s*\(/, description: "mergeBranch GraphQL mutation" }, | |
| ]; | |
| const FORBIDDEN: { readonly pattern: RegExp; readonly description: string }[] = [ | |
| { pattern: /git\s+push\s+--force(?!-with-lease-if)/i, description: "git push --force" }, | |
| { pattern: /git\s+push\s+--force-with-lease/i, description: "git push --force-with-lease" }, | |
| { pattern: /git\s+push\s+-f\b/i, description: "git push -f" }, | |
| { pattern: /git\s+push\s+\+/, description: "git push with + force-refspec" }, | |
| { pattern: /git\s+push\s+--mirror/i, description: "git push --mirror" }, | |
| { pattern: /git\s+reset\s+--hard\b/i, description: "git reset --hard" }, | |
| { pattern: /git\s+branch\s+-D\b/i, description: "git branch -D" }, | |
| { pattern: /git\s+push\b.*\s--delete\b/i, description: "git push --delete" }, | |
| { pattern: /git\s+filter-branch/i, description: "git filter-branch" }, | |
| { pattern: /git\s+filter-repo/i, description: "git filter-repo" }, | |
| { pattern: /git\s+replace\b/i, description: "git replace" }, | |
| { pattern: /\bgh\s+pr\s+merge/i, description: "gh pr merge" }, | |
| { pattern: /mergePullRequest\s*\(/, description: "mergePullRequest GraphQL mutation" }, | |
| { pattern: /mergeBranch\s*\(/, description: "mergeBranch GraphQL mutation" }, | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/check-no-destructive-actions.ts` around lines 23 - 37, The FORBIDDEN
list is too narrow and misses common destructive forms; update the FORBIDDEN
array to add regex entries that catch any "git reset --hard" with any target
(e.g., HEAD or a SHA), remote branch-deletion forms like "git push <remote>
--delete <branch>", "git push <remote> :<branch>" and the shorthand "git push
--delete", and include lowercase branch deletion "git branch -d" (in addition to
-D); make sure each new pattern is case-insensitive and add clear descriptions
(e.g., "git reset --hard <target>", "git push --delete <branch>", "git push
<remote>:<branch>", "git branch -d") so the FR-009 guard correctly flags these
operations.
Comment on lines
+64
to
+80
| const text = readFileSync(file, "utf8"); | ||
| const lines = text.split(/\r?\n/); | ||
| for (let i = 0; i < lines.length; i += 1) { | ||
| const line = lines[i] ?? ""; | ||
| // Skip pure JSDoc/comment lines that document the prohibition. | ||
| const stripped = line.trim(); | ||
| if (stripped.startsWith("//") || stripped.startsWith("*")) continue; | ||
| for (const rule of FORBIDDEN) { | ||
| if (rule.pattern.test(line)) { | ||
| violations.push({ | ||
| file, | ||
| line: i + 1, | ||
| description: rule.description, | ||
| text: line.trim(), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Line-by-line matching is too easy to evade for a safety guard.
A wrapped command or multiline GraphQL mutation can bypass every rule here because each regex only sees one source line. The current comment filter is also brittle for block comments. For FR-009, scan normalized file text and then translate match offsets back to line numbers for reporting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/check-no-destructive-actions.ts` around lines 64 - 80, Current
line-by-line regex scanning (using FORBIDDEN and pushing to violations per line)
is evadable and brittle; change logic to run each forbidden rule.pattern against
the full normalized file text (e.g., collapse CRLF, normalize
whitespace/newlines and remove or neutralize block comments) to find multi-line
matches, then convert match.start offsets back to file line numbers to populate
violations with correct file and line, preserving rule.description and matched
text; ensure the comment filter correctly ignores both line and block comments
before scanning so FR-009 and other multi-line patterns are detected robustly.
Comment on lines
+312
to
+388
| async function terminalReady(input: TerminalReadyInput): Promise<void> { | ||
| const { | ||
| octokit, | ||
| command, | ||
| intentId, | ||
| trackingCommentId, | ||
| deadlineAt, | ||
| isDraft, | ||
| pullRequestNodeId, | ||
| log, | ||
| } = input; | ||
|
|
||
| // T059 checkpoint immediately before the GraphQL mutation. | ||
| if (await checkpointCancelled(intentId)) { | ||
| log.info( | ||
| { event: "ship.checkpoint.cancelled", intent_id: intentId, where: "pre_mark_ready" }, | ||
| "terminalReady cancelled before markPullRequestReadyForReview", | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| // (e)(1) markPullRequestReadyForReview — gated on isDraft. Failure | ||
| // MUST NOT block (2)/(3) but MUST be surfaced. | ||
| let markReadyError: string | null = null; | ||
| if (isDraft && pullRequestNodeId !== null) { | ||
| try { | ||
| await octokit.graphql<MarkReadyResponse>(MARK_READY_FOR_REVIEW_MUTATION, { | ||
| pullRequestId: pullRequestNodeId, | ||
| }); | ||
| log.info( | ||
| { event: "ship.ready_for_review", intent_id: intentId }, | ||
| "PR transitioned to ready-for-review", | ||
| ); | ||
| } catch (err) { | ||
| markReadyError = err instanceof Error ? err.message : String(err); | ||
| log.error( | ||
| { err, event: "ship.ready_for_review_failed", intent_id: intentId }, | ||
| "markPullRequestReadyForReview failed — proceeding with terminal transition", | ||
| ); | ||
| } | ||
| } else if (isDraft && pullRequestNodeId === null) { | ||
| markReadyError = "could not resolve pull request node id"; | ||
| log.warn( | ||
| { event: "ship.ready_for_review_skipped", intent_id: intentId }, | ||
| "skipped markPullRequestReadyForReview — node id unavailable", | ||
| ); | ||
| } | ||
|
|
||
| // (e)(2) update tracking comment to terminal state. | ||
| const terminalBody = renderTrackingComment({ | ||
| intent_id: intentId, | ||
| trigger_login: command.principal_login, | ||
| deadline_at: deadlineAt, | ||
| phase: "terminal", | ||
| last_action: | ||
| markReadyError === null | ||
| ? "PR is merge-ready — handed back for human merge" | ||
| : `PR is merge-ready — handed back for human merge (markReadyForReview failed: ${markReadyError})`, | ||
| iteration_n: 0, | ||
| spent_usd: 0, | ||
| terminal_state: "ready_awaiting_human_merge", | ||
| }); | ||
| try { | ||
| await updateTrackingComment({ | ||
| octokit, | ||
| owner: command.pr.owner, | ||
| repo: command.pr.repo, | ||
| comment_id: trackingCommentId, | ||
| body: terminalBody, | ||
| }); | ||
| } catch (err) { | ||
| log.warn({ err }, "terminal tracking-comment update failed (best-effort)"); | ||
| } | ||
|
|
||
| // (e)(3) transition to terminal state. | ||
| await transitionToTerminal(intentId, "ready_awaiting_human_merge", null); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider passing sql to terminalReady for consistency.
The terminalReady function calls transitionToTerminal without the sql parameter (line 387), relying on the default requireDb(). While this works (singleton pattern), it's inconsistent with the rest of runShipFromCommand which explicitly manages const sql = requireDb() and passes it to other DB operations.
🔧 Proposed fix for consistency
interface TerminalReadyInput {
readonly octokit: Octokit;
readonly command: CanonicalCommand;
readonly intentId: string;
readonly trackingCommentId: number;
readonly deadlineAt: Date;
readonly isDraft: boolean;
readonly pullRequestNodeId: string | null;
readonly log: Logger;
+ readonly sql: SQL;
}
async function terminalReady(input: TerminalReadyInput): Promise<void> {
// ...
- await transitionToTerminal(intentId, "ready_awaiting_human_merge", null);
+ await transitionToTerminal(intentId, "ready_awaiting_human_merge", null, input.sql);
}And update the call site to pass sql.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/workflows/ship/session-runner.ts` around lines 312 - 388, terminalReady
currently omits the sql DB handle and calls transitionToTerminal relying on the
default requireDb(); change terminalReady to accept an sql parameter (same type
as produced by requireDb()), pass that sql through to transitionToTerminal, and
update any callers (e.g., runShipFromCommand) to pass their local sql when
invoking terminalReady so DB usage is consistent with other functions like
updateTrackingComment/renderTrackingComment and avoids implicit requireDb()
calls.
chrisleekr
changed the title
Critical:
- mcp/resolve-review-thread: preflight now also fetches the thread's
pullRequest.repository.{owner.login,name} and rejects any (owner, repo,
number) drift from the bound (REPO_OWNER, REPO_NAME, PR_NUMBER) tuple.
pullRequest.number is per-repo, so the previous number-only check
could resolve a thread on a same-numbered PR in a different repo.
Major (real bugs):
- workflows/ship/lifecycle-commands: foreign-push detection on resume
was inspecting `pr.head.user`, which is the head-repository owner per
the GitHub REST contract — NOT the commit author/pusher. Switched to
`octokit.rest.repos.getCommit(ref=head.sha)` and compared the mapped
`commit.author.login` (with `commit.committer.login` fallback)
against `config.botAppLogin`.
- workflows/ship/nl-classifier: prefix-only gate now also requires a
token boundary (whitespace / `:;,!.?)` / EOS) after the trigger
phrase so longer logins that share the prefix
(e.g. `@chrisleekr-bot-foo` vs `@chrisleekr-bot`) cannot slip
through.
- workflows/ship/signature: Tier-1 ESLint regex re-anchored on
end-of-line and broadened to allow scoped rule ids (`@scope/name`).
The old pattern could stop early on a message word and would never
match `@typescript-eslint/no-unused-vars`, corrupting flake-cap and
fix-attempts grouping.
- mcp/resolve-review-thread: both GraphQL calls (preflight + mutation)
are now wrapped in `retryWithBackoff` so the published contract
("rate limit retry once, network failure retry up to 3") is honoured
instead of fire-once.
- mcp/resolve-review-thread: error responses now include `pr_number`
on every failure path (matches the contract update shipped in the
prior commit), and the preflight error chooses
`thread_not_found` vs `graphql_error` based on whether the node
resolved at all.
- mcp/resolve-review-thread: `runServer().catch` now `process.exit(1)`
on transport bind failure — previously the sidecar exited 0 after a
rejection and looked healthy to its supervisor.
- scripts/check-no-destructive-actions: forbidden patterns broadened
to match plain `git reset --hard` (not just `… origin/…`), `git
push --delete`, and case-insensitive `git branch -d|-D`.
Major (docs):
- specs/.../contracts/probe-graphql-query.md: reviewer-qualification
rule restated to match `review-barrier.ts` (User-only via
`__typename`, no App author allowlist).
- specs/.../data-model.md: `non_readiness_reason` CHECK constraint and
the canonical `NON_READINESS_REASONS` list now include
`review_barrier_deferred` so the enum/CHECK do not drift from
runtime.
Minor:
- docs/SETUP.md: handler-status note now states `pull-request.ts` and
`review.ts` are active (not placeholders); only `review-thread.ts`
remains a placeholder.
- specs/.../plan.md: typo "markered" → "use markers".
- src/webhook/events/pull-request.ts: docstring updated to reflect the
per-action listener registration in `app.ts`.
- src/workflows/ship/command-dispatch: pass `err` directly to pino so
the stack and structured properties survive the log call (was
`String(err)`).
Trivials:
- src/workflows/ship/probe: `as never` cast replaced by an explicit
`as unknown as BarrierProbeShape` cast with rationale; the type is
now imported from `review-barrier.ts`.
- src/workflows/ship/probe: audit-row `appendIteration` failure no
longer silently swallowed; logged at `warn`.
- src/workflows/ship/reactor-bridge: skipped early-wake (no DB) now
emits a `debug` log so missing-DATABASE_URL configurations are
visible during triage.
- src/workflows/ship/verdict: `import { z } from "zod"` moved to the
top of the file with the other imports.
Tests:
- test/workflows/ship/eligibility: added coverage for the
`unauthorized` and `forbidden_target_branch` verdicts. The tests
mutate the live `config` singleton (and restore in `afterEach`)
because `loadConfig()` only reads `process.env` once at module load.
- test/workflows/ship/lifecycle-commands: added an
`octokit.rest.repos.getCommit` mock so the resume foreign-push
pathway can verify against the new commit-author check.
Push back / not changed (with rationale documented in the PR thread):
- check-no-destructive-actions multi-line scanning: line-by-line is
intentional. The guard exists to flag obvious shell forms — adding
multi-line normalisation expands the false-positive surface (block
comments, JSDoc, prompt strings) without meaningfully improving
safety, and the runtime test in T046a covers tool-call recorders for
the agent path.
- docs/EXTENDING.md handler path: CodeRabbit suggested
`src/workflows/ship/handlers/resolve.ts`, but the actual handler
lives at `src/workflows/handlers/resolve.ts` (verified via
`find src/workflows -name "resolve*"`). Current docs are correct.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai — round 2 addressed in cbf4259. Critical (1):
Major code (7):
Major docs (2):
Minor (5): SETUP.md handler-status note, plan.md "markered" typo, pull-request.ts docstring, command-dispatch Trivials (4): probe.ts Pushing back (2) — documented in commit message:
|
|
Tip For best results, initiate chat on the files or code changes.
Critical — accepted. Binding the preflight to the full Major code — all accepted.
Major docs — accepted. Minor / trivials — all accepted. Pushbacks — both accepted:
✏️ Learnings added
🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (3)
scripts/check-no-destructive-actions.ts (1)
23-32:⚠️ Potential issue | 🟠 MajorExpand forbidden branch-deletion patterns to match FR-009 policy.
The guard still misses destructive branch deletion forms:
git branch -dand remote delete via refspec (git push <remote> :<branch>/git push :<branch>). That leaves a bypass path while the policy text says branch deletion is forbidden.Suggested patch
const FORBIDDEN: { readonly pattern: RegExp; readonly description: string }[] = [ @@ { pattern: /git\s+reset\s+--hard\b/i, description: "git reset --hard" }, { pattern: /git\s+branch\s+-D\b/i, description: "git branch -D" }, + { pattern: /git\s+branch\s+-d\b/i, description: "git branch -d" }, { pattern: /git\s+push\b[^"\n]*\s--delete\b/i, description: "git push --delete" }, + { pattern: /git\s+push\b[^"\n]*\s:[^\s"'`]+/i, description: "git push <remote> :<branch>" }, { pattern: /git\s+filter-branch/i, description: "git filter-branch" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check-no-destructive-actions.ts` around lines 23 - 32, The FORBIDDEN array in scripts/check-no-destructive-actions.ts misses branch-deletion forms required by FR-009; add new RegExp entries to FORBIDDEN to detect "git branch -d" (case-insensitive, word boundary, and also handle "-d" alongside "-D") and refspec-style remote deletions like "git push :<branch>" and "git push <remote> :<branch>" (match a colon followed by a branch name possibly after a remote, with optional whitespace and anchors to avoid false positives). Update the FORBIDDEN array (next to existing entries such as the ones matching "git branch -D" and "git push with + force-refspec") so those patterns are included with appropriate description strings like "git branch -d" and "git push refspec delete".src/workflows/ship/lifecycle-commands.ts (1)
225-234: 🧹 Nitpick | 🔵 TrivialConsider adding an explicit intent check for abort (optional).
The abort handling is currently an implicit else-branch. While safe due to TypeScript's type narrowing (only
abortremains afterstopandresumechecks), an explicit check would be more self-documenting and defensive against future intent additions.♻️ Optional defensive check
+ if (command.intent !== "abort") { + log.error( + { event: "ship.lifecycle.unknown_intent", intent: command.intent }, + "unexpected lifecycle intent", + ); + return; + } + // abort — set flag, give workers a chance to bail at next checkpoint,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflows/ship/lifecycle-commands.ts` around lines 225 - 234, Add an explicit check that the intent is an abort before running the abort sequence: instead of relying on type-narrowing, guard the block with a clear condition (e.g., check intent.type/intent.name === 'abort') around the calls to requestAbort(valkey), sleep(POST_FLAG_WAIT_MS), forceAbortIntent(intent.id, command.principal_login, sql), and the postReply; reference the existing symbols valkey, requestAbort, POST_FLAG_WAIT_MS, forceAbortIntent and intent.id so the intent-specific block is only executed when the intent represents an abort.src/mcp/servers/resolve-review-thread.ts (1)
56-70:⚠️ Potential issue | 🟠 MajorHandle non-thread node IDs explicitly before PR/repo checks.
At Line 166-Line 172, not-found is inferred only from
preflight.node === null. With the current inline-fragment query, a non-thread node can be non-null but still lackpullRequest, producing a misleadinggraphql_errorpath (includingPR#undefined``) instead ofthread_not_found.🐛 Suggested contract-safe fix
const GET_THREAD_QUERY = ` query GetReviewThread($threadId: ID!) { node(id: $threadId) { + __typename ... on PullRequestReviewThread { id pullRequest { number repository { name owner { login } } } } } } `; interface PreflightResponse { - node: { - id: string; - pullRequest: { - number: number; - repository: { name: string; owner: { login: string } }; - }; - } | null; + node: + | { + __typename: "PullRequestReviewThread"; + id: string; + pullRequest: { + number: number; + repository: { name: string; owner: { login: string } }; + }; + } + | { __typename: string } + | null; } const preflight = await retryWithBackoff(() => octokit.graphql<PreflightResponse>(GET_THREAD_QUERY, { threadId: thread_id }), ); + if (preflight.node === null || preflight.node.__typename !== "PullRequestReviewThread") { + return { + content: [ + { + type: "text" as const, + text: JSON.stringify({ + code: "thread_not_found", + message: `thread ${thread_id} not found or not a PullRequestReviewThread`, + thread_id, + pr_number: BOUND_PR_NUMBER, + }), + }, + ], + isError: true, + }; + } - const preflightPr = preflight.node?.pullRequest.number; - const preflightRepo = preflight.node?.pullRequest.repository; + const preflightPr = preflight.node.pullRequest.number; + const preflightRepo = preflight.node.pullRequest.repository;In GitHub GraphQL, when querying `node(id: ...)` with only an inline fragment on `PullRequestReviewThread`, what does the response look like if the node exists but is a different type?Also applies to: 87-95, 157-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp/servers/resolve-review-thread.ts` around lines 56 - 70, The GraphQL inline-fragment can return a non-thread node (non-null) which lacks pullRequest, so update GET_THREAD_QUERY to select __typename (and/or query node { __typename ... on PullRequestReviewThread { pullRequest { ... } } }) and then in the resolver preflight check both preflight.node !== null and preflight.node.__typename === 'PullRequestReviewThread' (or at minimum confirm preflight.node.pullRequest exists) and explicitly return the thread_not_found error when the node is present but not a PullRequestReviewThread before doing any PR/repository checks; reference GET_THREAD_QUERY and the preflight.node checks in your changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@specs/20260427-201332-pr-shepherding-merge-ready/contracts/probe-graphql-query.md`:
- Around line 33-48: Update the contract query to match
src/workflows/ship/probe.ts by removing the now-unused
reviewThreads.nodes[].comments selection and adding CheckRun.databaseId to the
CheckRun selection; specifically, reconcile the reviewThreads -> nodes structure
to drop comments and ensure CheckRun includes databaseId so fixtures and rerun
handling align with the shipped query.
In `@specs/20260427-201332-pr-shepherding-merge-ready/data-model.md`:
- Line 289: The doc claim that "All three enumerations are SQL `CHECK`
constraints AND TS literal-union types AND Zod enums" is false because the
TypeScript query-layer union NonReadinessReason (in src/db/queries/ship.ts) is
missing "review_barrier_deferred"; fix by updating the NonReadinessReason
union/type to include the "review_barrier_deferred" literal so it matches the
SQL CHECK and Zod enum, or alternatively change the sentence in data-model.md to
soften the claim until you add that literal; reference the NonReadinessReason
identifier in ship.ts when making the change.
In `@src/mcp/servers/resolve-review-thread.ts`:
- Around line 39-40: Add a pino logger import at the top (import logger from
your logger module) and replace all console.error calls with structured
logger.error calls: for startup validation (when checking REPO_OWNER, REPO_NAME,
PR_NUMBER, GITHUB_TOKEN) call logger.error({ env: { REPO_OWNER, REPO_NAME,
PR_NUMBER, GITHUB_TOKEN } }, "REPO_OWNER, REPO_NAME, PR_NUMBER, and GITHUB_TOKEN
are required") then call process.exit(1); for the other validation spots use the
same pattern including the relevant variables in the metadata; for request-level
logging (around the handler referenced at the 147–148 region) derive a child
logger via logger.child({ requestId }) and use that for per-request errors; and
for the process error handler (the unhandled rejection/uncaught exception block
around 230–235) replace console.error(err) with logger.error({ err }, "Unhandled
process error") so the error object and context are emitted as structured JSON.
- Around line 114-124: The classifyError function currently maps status 403 to
"permission_denied" before checking the error message, which misclassifies
GitHub secondary rate-limit 403s; modify classifyError so the error message is
computed before the status-based branch and handle 403 by inspecting the message
first (if message includes "rate limit", "secondary rate", or similar return
"rate_limited", otherwise return "permission_denied"); also ensure 429 still
returns "rate_limited" and preserve existing checks for 404 and other
message-based matches; reference the classifyError function and the ErrorCode
type when making the change.
In `@src/webhook/events/pull-request.ts`:
- Around line 39-51: The synchronize handler currently uses payload.sender.login
as head_author_login; instead call the GitHub API to fetch the actual commit for
payload.pull_request.head.sha via repos.getCommit and set head_author_login to
commit.author?.login ?? commit.committer?.login (preserving the existing
installation check and head_sha use), then fireReactor with that resolved author
so downstream logic (e.g., handleSynchronize / "manual-push-detected") can
correctly detect foreign pushes.
In `@src/workflows/ship/probe.ts`:
- Around line 121-126: The unguarded octokit.graphql call inside the probe loop
(the for loop that iterates attempts using backoff) should be wrapped in a
try/catch so transient GraphQL/network/rate-limit errors are retried rather than
aborting the probe; catch errors from octokit.graphql(PROBE_QUERY, ...) and on
failure log the error (including error details), await the corresponding backoff
delay for this attempt, then continue to the next attempt, only throwing or
failing after all backoff attempts are exhausted; ensure the retry logic
references the existing backoff array and preserves downstream behavior that
handles successful responses (so do not skip or short-circuit the normal
response handling when retries succeed).
- Around line 45-48: computeVerdict currently only inspects the first 100
reviewThreads and can miss unresolved threads; update the probe in
src/workflows/ship/probe.ts to paginate the reviewThreads GraphQL connection
(use pageInfo.hasNextPage and endCursor) and continue fetching pages until you
either encounter an unresolved thread (isResolved === false) and return
not-ready, or exhaust the connection and then decide ready; if the connection is
truncated or the API indicates partial results, treat that as a failure-closed
case (return not-ready) instead of assuming ready. Ensure you reference the
reviewThreads field and the computeVerdict code path so the loop/early-exit is
placed where review thread nodes are checked.
In `@src/workflows/ship/signature.ts`:
- Around line 68-74: The current tryExtractTier1 builds a Tier-1 key by joining
all regex capture groups (groups.join("|")), which pulls in volatile message
text; instead, change tryExtractTier1 to return a stable identifier: update
TIER1_PATTERNS to include a single dedicated capture (preferably a named group
like (?<tier1>...)) that encodes the lint/type class, then have tryExtractTier1
extract and return only that group (falling back to the first capture or the
regex pattern id if the named group is absent). Ensure the function references
the named group "tier1" (or the first capture) rather than concatenating all
groups so signatures remain stable across message variations.
In `@src/workflows/ship/verdict.ts`:
- Around line 182-185: The code currently treats only "PENDING" as outstanding;
update the conditional handling around ctx.state (used in the block that
populates failingRequired and pendingRequired) so that "EXPECTED" is treated the
same as "PENDING" (i.e., push ctx.context ?? "<unknown>" into pendingRequired
when s === "PENDING" || s === "EXPECTED"); keep the existing handling for
"FAILURE" and "ERROR" that pushes into failingRequired and leave "SUCCESS"
unchanged.
---
Duplicate comments:
In `@scripts/check-no-destructive-actions.ts`:
- Around line 23-32: The FORBIDDEN array in
scripts/check-no-destructive-actions.ts misses branch-deletion forms required by
FR-009; add new RegExp entries to FORBIDDEN to detect "git branch -d"
(case-insensitive, word boundary, and also handle "-d" alongside "-D") and
refspec-style remote deletions like "git push :<branch>" and "git push <remote>
:<branch>" (match a colon followed by a branch name possibly after a remote,
with optional whitespace and anchors to avoid false positives). Update the
FORBIDDEN array (next to existing entries such as the ones matching "git branch
-D" and "git push with + force-refspec") so those patterns are included with
appropriate description strings like "git branch -d" and "git push refspec
delete".
In `@src/mcp/servers/resolve-review-thread.ts`:
- Around line 56-70: The GraphQL inline-fragment can return a non-thread node
(non-null) which lacks pullRequest, so update GET_THREAD_QUERY to select
__typename (and/or query node { __typename ... on PullRequestReviewThread {
pullRequest { ... } } }) and then in the resolver preflight check both
preflight.node !== null and preflight.node.__typename ===
'PullRequestReviewThread' (or at minimum confirm preflight.node.pullRequest
exists) and explicitly return the thread_not_found error when the node is
present but not a PullRequestReviewThread before doing any PR/repository checks;
reference GET_THREAD_QUERY and the preflight.node checks in your changes.
In `@src/workflows/ship/lifecycle-commands.ts`:
- Around line 225-234: Add an explicit check that the intent is an abort before
running the abort sequence: instead of relying on type-narrowing, guard the
block with a clear condition (e.g., check intent.type/intent.name === 'abort')
around the calls to requestAbort(valkey), sleep(POST_FLAG_WAIT_MS),
forceAbortIntent(intent.id, command.principal_login, sql), and the postReply;
reference the existing symbols valkey, requestAbort, POST_FLAG_WAIT_MS,
forceAbortIntent and intent.id so the intent-specific block is only executed
when the intent represents an abort.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2d61e245-7bb6-4ad8-8418-32246fb0c76c
📒 Files selected for processing (16)
docs/SETUP.mdscripts/check-no-destructive-actions.tsspecs/20260427-201332-pr-shepherding-merge-ready/contracts/probe-graphql-query.mdspecs/20260427-201332-pr-shepherding-merge-ready/data-model.mdspecs/20260427-201332-pr-shepherding-merge-ready/plan.mdsrc/mcp/servers/resolve-review-thread.tssrc/webhook/events/pull-request.tssrc/workflows/ship/command-dispatch.tssrc/workflows/ship/lifecycle-commands.tssrc/workflows/ship/nl-classifier.tssrc/workflows/ship/probe.tssrc/workflows/ship/reactor-bridge.tssrc/workflows/ship/signature.tssrc/workflows/ship/verdict.tstest/workflows/ship/eligibility.test.tstest/workflows/ship/lifecycle-commands.test.ts
Round-3 review of cbf4259 surfaced 9 actionable findings; this commit applies fixes for all of them. Correctness: - src/mcp/servers/resolve-review-thread.ts: classify GitHub secondary rate-limit 403s as `rate_limited` instead of `permission_denied` by inspecting the error message before the status branch. - src/webhook/events/pull-request.ts: resolve the actual commit author for `pull_request.synchronize` via `repos.getCommit` instead of `payload.sender.login` — mirrors the round-2 fix in lifecycle-commands.ts and prevents foreign-push false negatives. - src/db/queries/ship.ts: add `review_barrier_deferred` to the `NonReadinessReason` TS union so the SQL CHECK / TS / Zod tri-enum claim in data-model.md is actually true. - src/workflows/ship/verdict.ts: treat `StatusContext.state === EXPECTED` as pending — legacy required checks can sit in EXPECTED before any reporter posts a result. - src/workflows/ship/probe.ts: paginate `reviewThreads` past the first 100 nodes (early-exit on first open thread) so unresolved threads on large PRs cannot be hidden from `computeVerdict`. Robustness: - src/workflows/ship/probe.ts: wrap each individual GraphQL call in `retryWithBackoff` so a single rate-limit / network blip cannot abort the probe — the recoverable yield/snapshot path documented in the contract is now actually honoured. - src/workflows/ship/signature.ts: TIER1_PATTERNS now expose a single stable capture group encoding only the lint/type class. Volatile message text (variable names, paths) is matched but not captured, so fix-attempt cap counts retries of the same rule correctly. Logging / spec sync: - src/mcp/servers/resolve-review-thread.ts: replace `console.error` with structured pino logging on stderr (stdio JSON-RPC requires stdout to remain clean) — matches the repo guideline for structured logs. - specs/.../contracts/probe-graphql-query.md: drop the unused `reviewThreads.nodes[].comments` selection, add `CheckRun.databaseId`, add `reviewThreads.pageInfo` — contract now matches the shipped query. Local gates: `bun run typecheck` clean, `bun run lint` 0 errors, `bun test test/workflows/ship/ test/mcp/` → 100/100 pass. The 22 test-isolation failures observed when adding `test/webhook/events/` to the same invocation are pre-existing on cbf4259 and not introduced here. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
7 tasks
chrisleekr
added a commit
that referenced
this pull request
Apr 28, 2026
Closes the 9 unchecked test gaps from the spec and marks all 110 tasks complete. tasks.md drops from 49 unchecked to 0. New tests (DB-backed where the source module is, mocks elsewhere): - T012 continuation: persist/resume/restart-safety + invalid-blob - T013 webhook-reactor: 11 fanOut scenarios incl. paused-intent skip - T014 MCP resolve-review-thread: static-contract test layer - T014b trigger-router: FR-027 surface parity + per-intent eligibility - T014c nl-classifier: FR-025/025a mention-prefix gate - T037 intent: full state-machine coverage - T046a destructive-actions: static grep guard for FR-009/SC-003 - T048 restart: restart-safety + tracking-comment preservation - T049 ship-audit-trail: monotonic iteration_n + jsonb round-trip tasks.md: T010-T022, T043-T046, T051 marked [x] (PR #75 work); T072, T073-T091 marked [x] (this branch); T031, T031b, T092 marked [x] deferred to post-merge ngrok validation; T069 marked [x] (gates green). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This was referenced Apr 29, 2026
chrisleekr
pushed a commit
that referenced
this pull request
May 1, 2026
# [1.7.0](v1.6.1...v1.7.0) (2026-05-01) ### Bug Fixes * **idempotency:** scope durable check with since=triggerTimestamp (closes [#33](#33)) ([#69](#69)) ([5f1c1fa](5f1c1fa)) ### Features * **ship:** pr shepherding scaffolding + flag-gated trigger surfaces ([#75](#75)) ([928811b](928811b)) * **ship:** scoped commands (US5) + remove SHIP_USE_TRIGGER_SURFACES_V2 flag ([#77](#77)) ([33132a3](33132a3)) * **ship:** wire ship iteration loop, tickle scheduler, and four scoped executors ([#79](#79)) ([43da9aa](43da9aa))
Owner
Author
|
🎉 This PR is included in version 1.7.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Lands the scaffolding for the PR-shepherding feature (
bot:shiplifecycle): a probe → verdict → intent → continuation pipeline that drives a PR toward merge-ready under a single durable session per PR. New trigger surfaces (literalbot:ship, natural-language mention via Bedrock classifier, and per-command labels) sit behindSHIP_USE_TRIGGER_SURFACES_V2; the legacy mention-only handler stays as the default until the flag flips. Adds theresolve_review_threadMCP server bound to a single PR for safe thread resolution. Schema changes ship in migration008_ship_intents.sql(four tables; partial unique index enforces one in-flight session per PR, covering bothactiveandpaused). Includes the resolution of CodeRabbit's three critical findings and the major+minor batch on top of the initial scaffold (commitscd9867e,32dc584).Diagram
flowchart LR classDef oldNode fill:#fef3c7,stroke:#92400e,color:#451a03 classDef newNode fill:#d1fae5,stroke:#065f46,color:#022c22 classDef trigger fill:#dbeafe,stroke:#1e3a8a,color:#0c1e3a classDef gate fill:#fde68a,stroke:#78350f,color:#451a03 PRevt["PR / comment / label / review event"]:::trigger LegacyMention["Legacy: @mention only<br/>processRequest pipeline"]:::oldNode Flag{"SHIP_USE_TRIGGER_SURFACES_V2"}:::gate Router["trigger-router<br/>literal / NL / label"]:::newNode Eligibility["eligibility check"]:::newNode Intent["createIntent<br/>partial unique idx"]:::newNode Probe["probe<br/>GraphQL MergeReadiness"]:::newNode Verdict["computeVerdict<br/>NonReadinessReason"]:::newNode Cont["continuation row<br/>+ ship:tickle ZSET"]:::newNode Reactor["webhook reactor<br/>fanOut early-wake"]:::newNode Term["terminal: ready_awaiting_human_merge<br/>or human_took_over / pr_closed"]:::newNode PRevt --> Flag Flag -->|off| LegacyMention Flag -->|on| Router Router --> Eligibility --> Intent --> Probe --> Verdict Verdict -->|ready| Term Verdict -->|not ready| Cont Cont --> Reactor Reactor --> ProbeChanges
Spec & contracts (P0)
specs/20260427-201332-pr-shepherding-merge-ready/— full Spec Kit set:spec.md,plan.md,research.md,data-model.md,tasks.md,quickstart.md, plus four contract docs (probe-graphql-query.md,bot-commands.md,webhook-event-subscriptions.md,mcp-resolve-thread-server.md,resolve-thread-mutation.md).docs/SHIP.md,docs/ARCHITECTURE.md,docs/CONFIGURATION.md,docs/OBSERVABILITY.md,docs/SETUP.md,docs/EXTENDING.md,docs/BOT-WORKFLOWS.md,mkdocs.ymlupdated to reflect the new module surface.Database (P2)
src/db/migrations/008_ship_intents.sqladdsship_intents,ship_iterations,ship_continuations,ship_fix_attempts. Partial unique indexWHERE status IN ('active', 'paused')enforces FR-007a.src/db/queries/ship.tstyped Bun.sql helpers;scripts/db-migrate.tsmigration runner.Ship workflow (P1–P7)
src/workflows/ship/{verdict,probe,review-barrier}.ts(P1).src/workflows/ship/intent.ts(P2) withpauseIntent/resumeIntent/forceAbortIntent/transitionToTerminal.src/workflows/ship/{continuation,tickle-scheduler}.ts(P3) with reentrancy guard and Postgres reconciliation on boot.src/workflows/ship/webhook-reactor.ts+ new event handlers (check-run.ts,check-suite.ts) + reactor bridge (P4); per-intent error isolation.src/workflows/ship/{fix-attempts,signature,deadline}.ts(P5) with two-tier signature derivation.src/workflows/ship/{abort,lifecycle-commands}.ts(P6) with cooperative-then-forced cancellation and foreign-push fail-closed resume.src/mcp/servers/resolve-review-thread.ts+registry.tsregistration (P7), bound to one(owner, repo, pr)per instance.Trigger surfaces (
SHIP_USE_TRIGGER_SURFACES_V2)src/workflows/ship/{trigger-router,literal-command,nl-classifier,label-trigger,command-dispatch,session-runner,reactor-bridge}.ts.src/ai/llm-client.ts); FR-025a mention-prefix gate runs before any LLM call.BOT_LABEL_PATTERNpermits documented label shapes (bot:abort-ship,bot:fix-thread,bot:ship/deadline=2h, …).Config + log fields
src/config.tsadds the ship env block (MAX_WALL_CLOCK_PER_SHIP_RUN,CRON_TICKLE_INTERVAL_MS,MERGEABLE_NULL_BACKOFF_MS_LIST,REVIEW_BARRIER_SAFETY_MARGIN_MS,FIX_ATTEMPTS_PER_SIGNATURE_CAP,SHIP_USE_PROBE_VERDICT,SHIP_USE_CONTINUATION_LOOP,SHIP_USE_TRIGGER_SURFACES_V2,BOT_APP_LOGIN).src/workflows/ship/log-fields.tsZod-validated structured-log schema (FR-016).CI safety
scripts/check-no-destructive-actions.tsgreps the ship subtree for force-push / reset-hard / merge-API patterns; fails closed on filesystem errors.CodeRabbit follow-ups in this PR
cd9867e): cross-PR resolve-thread preflight, eligibility owner-allowlist single-principal check, tickle-scheduler dispatch retry.32dc584): ANSI regex fix, label pattern broaden, NL prefix gate, reentrancy guard, double-@reply, hardcoded trigger phrase removed, resume fail-closed, reactor isolation, structuredunique_violationdetection (Bun.sqlerrno/constraint), flake-tracker pino logging,databaseIdfor projected reruns, review-barrier__typenamefilter, pause/resume guarded reply, plus the C-bucket doc/spec sweep.Related Issues
Test plan
bun run typecheckclean,bun run lintzero errors008.test.ts)repo-knowledge.test.tsand unrelated K8s/runs-store/intent-classifier failures predate this branch — verified viagit stashbaseline)SHIP_USE_TRIGGER_SURFACES_V2=truein dev installation before flag flip008_ship_intents.sqlapplies cleanly on staging PostgresSummary by CodeRabbit
Release Notes
New Features
bot:ship) with automated merge-readiness probing and iterative state managementbot:shipcomments, natural language mentions, and GitHub labelsbot:stop,bot:resume,bot:abort-shipDocumentation
Tests