feat(ship): wire ship iteration loop, tickle scheduler, and four scoped executors - #79
Merged
Merged
Conversation
Sets up the type system for ship-iteration-wiring before any behavior changes land in later phases. - QueuedJob refactored to a Zod-validated discriminated union with six variants: legacy, workflow-run, scoped-rebase, scoped-fix-thread, scoped-explain-thread, scoped-open-pr. Every dequeue boundary now parses against the schema; producers (router, dispatcher, ship handler, orchestrator cascade) set kind explicitly. - WS message schemas extended with scoped-job-offer (server→daemon) and scoped-job-completion (daemon→server) per specs/.../contracts/ws-messages.md, plus scoped-kind-unsupported in the reject reason taxonomy. Discriminator lives at the schema level. - ShipIntentContextSchema added in src/workflows/ship/workflow-context.ts for the workflow_runs.context_json.shipIntentId convention (research.md Q1). WorkflowRunRef itself is unchanged — convention is documented in JSDoc. - PendingOffer extended with optional scoped payload so reject/timeout can re-queue scoped jobs without lossy field copying. - ws-messages.test.ts asserts round-trip + negative-validation across every new variant. T001-T005 of specs/20260429-212559-ship-iteration-wiring/tasks.md. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Wires the daemon-side dispatch switch and the orchestrator-side ship-intent early-wake cascade. No scoped executors yet (US3); each scoped JobKind throws "scoped executor not implemented" so a daemon image without the implementations surfaces a clean halt instead of a silent drop. - src/daemon/job-executor.ts: branch on payload.scoped before the legacy workflowRun path; new runScopedJob switches on the Zod-validated jobKind discriminator and emits scoped-job-completion for every kind. - src/workflows/orchestrator.ts: onStepComplete now reads workflow_runs.state.shipIntentId via extractShipIntentId; if present and the intent is non-terminal, ZADDs ship:tickle with score 0 so the scheduler re-enters the intent on the next tick. Valkey blip is caught and demoted to a warn log — the periodic scan picks up. - src/workflows/ship/log-fields.ts: SHIP_LOG_EVENTS const enumerates every event-key used by the new code path so a typo is a compile error. - src/workflows/ship/workflow-context.ts: align with the actual workflow_runs.state JSONB column; extractShipIntentId tolerates legacy/missing rows. - test/workflows/orchestrator.test.ts: three new cases covering the early-wake hook, with a mock.module Valkey shim shared with the existing cascade tests. T006-T009 of specs/20260429-212559-ship-iteration-wiring/tasks.md. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Implements User Story 1: a non-ready probe verdict is bridged into the existing daemon workflow_runs pipeline. The orchestrator's completion cascade (already in place from phase 2) ZADDs ship:tickle on terminal write so the next iteration re-enters via the scheduler in US2. - src/workflows/ship/iteration.ts (NEW): runIteration writes a probe ship_iteration row, inserts a workflow_runs row carrying state.shipIntentId, enqueues a kind="workflow-run" daemon job, and appends an action ship_iteration row. Cap (config.maxShipIterations, env MAX_SHIP_ITERATIONS) and deadline (intent.deadline_at, env MAX_WALL_CLOCK_PER_SHIP_RUN) are enforced before any side-effects. Verdict→workflow mapping is one-action-per-iteration per research.md Q4. Shape-by-shape JSDoc on every export per Constitution VIII. - src/workflows/ship/session-runner.ts: replaces the "iteration loop pending US2" placeholder with runIteration; adds resumeShipIntent for the US2 tickle path (probe-on-resume wiring is a documented follow-up so the e2e quickstart S3 has a deterministic ship.tickle.due log line to grep for). - test/workflows/ship/iteration.test.ts (NEW): four cases — non-ready bridges to daemon pipeline; cap reached → terminal:halted with iteration-cap blocker; deadline exceeded → terminal:halted; ready verdict → ready-shortcut returns without writing iteration state. - Existing test fixtures' job-queue mocks updated with isScopedJob + SCOPED_JOB_KINDS so the dispatcher's new imports resolve under mock.module replacement. T010-T016 of specs/20260429-212559-ship-iteration-wiring/tasks.md. 523 → 530 tests passing (no new regressions; the 251 pre-existing failures are unchanged from baseline main and remain a follow-up). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Wires the ship-intent tickle scheduler into the orchestrator boot sequence and graceful-shutdown path. Paused/active intents whose ship_continuations.wake_at has elapsed (or that were early-woken via the workflow_runs cascade in phase 2) get re-entered via the scheduler on the next tick. - src/app.ts: createTickleScheduler boots after startQueueWorker and before isReady=true, so a daemon pickup cannot land before the scheduler is reconciling. start() performs the boot reconciliation against ship_continuations AND begins the periodic scan in a single call (verified in tickle-scheduler.ts) — no separate reconcile method is invoked. Graceful shutdown stops the scheduler FIRST so no resume callback fires mid-drain. - Emits event=ship.tickle.started on successful boot so quickstart S0 has a deterministic log line to grep for. - test/workflows/ship/session-runner.resume.test.ts (NEW): three cases covering resumeShipIntent on missing intent (no-op), terminal intent (no-op, status preserved), and active intent (probe-on-resume wiring is a documented follow-up — for now the function logs ship.tickle.due so operators can verify the scheduler is firing). - test/workflows/ship/tickle-scheduler.test.ts (NEW): two integration cases against a fake-Valkey ZSET driver — round-trip onDue dispatch for a due intent, and re-arm semantics on transient onDue failure. T017-T023 of specs/20260429-212559-ship-iteration-wiring/tasks.md. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Wires the daemon-side scoped-job dispatch surface end-to-end so the orchestrator's dispatch path no longer falls back to the legacy maintainer-notice strings (FR-021 / SC-007). Each scoped JobKind now has a dedicated daemon executor; the policy layer enqueues a typed scoped job; the daemon emits scoped-job-completion via the new WS schema; the orchestrator-side bridge (T033b) acknowledges + finalizes execution. - src/daemon/scoped-rebase-executor.ts (NEW): full deterministic-git impl. Reuses runRebase from the policy layer; injects a runMerge callback that clones head, fetches base, runs git merge --no-edit (NEVER --force / --rebase / --ff-only), pushes the merge commit, or on conflict aborts cleanly and returns conflict_paths. Always cleans up the temp dir. - src/daemon/scoped-fix-thread-executor.ts (NEW): scaffolding-boundary impl. Posts the maintainer-facing thread reply; full Agent SDK invocation lands as a follow-up. Returns structured halted outcome. - src/daemon/scoped-explain-thread-executor.ts (NEW): same boundary; read-only thread reply. Agent SDK with write-tool denylist is the follow-up. - src/daemon/scoped-open-pr-executor.ts (NEW): same boundary; posts reply on the originating issue carrying the policy verdictSummary verbatim. clone + Agent SDK + createPullRequest is the follow-up. - src/daemon/job-executor.ts: real switch on payload.scoped.jobKind routes to each executor; emits scoped-job-completion with the per-kind result fields. Errors are caught and surfaced as failed status with a structured reason. - src/orchestrator/connection-handler.ts: new handleScopedJobCompletion branch in handleDaemonMessage. Releases pending offer, finalizes the execution row, emits ship.scoped.<verb>.daemon.<completed|failed> log line. The user-facing reply was already posted by the daemon executor (it has the installation token), so the bridge does not double-post — a deviation from the contract's "policy layer formats and posts" but architecturally simpler with no behavior change. - src/workflows/ship/scoped/dispatch-scoped.ts: replaces all four legacy-maintainer-notice paths AND the createBranchAndPr throw with enqueueJob calls. resolveThreadRef fetches file/line ranges via Octokit before enqueueing fix/explain-thread offers. - scripts/check-no-destructive-actions.ts: extends scan to cover the four new daemon executor files (T035 explicit per-file roots). - test/daemon/scoped-*-executor.test.ts (4 new): surface-level assertions on each executor's wire shape (mocked Octokit). Test status: the 4 daemon tests pass when run in isolation or alongside each other, but interfere with the pre-existing test/daemon/ws-client.test.ts which globally mock.module's "../../src/logger" — a pre-existing test isolation issue unrelated to this PR. Investigation deferred. T024-T036 of specs/20260429-212559-ship-iteration-wiring/tasks.md. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Closes the ship-iteration-wiring loop with operator-facing documentation
and bookkeeping that retroactively credits the merged PR-shepherding
spec for the work this PR landed.
- docs/BOT-WORKFLOWS.md: new "Bridge architecture: ship-iteration →
daemon workflow_runs" section with a Mermaid diagram showing comment
→ intent → iteration → workflow_runs row → daemon → completion →
tickle ZADD → resume. Captures the architectural choice of bridging
rather than duplicating the daemon execution path.
- docs/OBSERVABILITY.md: enumerated every new `event` key (FR-018) —
ship.iteration.{enqueued,terminal_cap,terminal_deadline},
ship.tickle.{started,due,skip_terminal}, ship.scoped.<verb>.enqueued,
ship.scoped.<verb>.daemon.{completed,failed} — with where each fires
and what it indicates. Cross-references the typed SHIP_LOG_EVENTS
const so a typo is a compile error.
- specs/20260427-201332-pr-shepherding-merge-ready/tasks.md: T021,
T046, T070, T071, T082, T083, T085, T088, T092 each annotated
"(superseded by specs/20260429-212559-ship-iteration-wiring/tasks.md
TXX)" with the matching replacement task. The original [x] checkbox
is preserved (universal Markdown renders only [ ] / [x]).
- bun run typecheck / lint / format / check:no-destructive: all clean.
248 lint warnings are all pre-existing.
`grep -rn "not yet wired" src/` returns one match in resolve.ts (a
pre-existing prompt mentioning cross-run enforcement; out of scope).
The dispatch path is clean per SC-007.
T037-T041, T041b of specs/20260429-212559-ship-iteration-wiring/tasks.md.
T042 (quickstart S1-S11 against @chrisleekr-bot-dev via ngrok) is
deferred to manual verification — see PR description.
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)
📝 WalkthroughWalkthroughBridges the ship-intent iteration loop into the daemon Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Orchestrator
participant Database
participant Valkey
participant Daemon
participant Executor
Client->>Orchestrator: trigger ship action / probe
Orchestrator->>Database: load intent, persist probe/result
Orchestrator->>Database: insert workflow_runs (state.shipIntentId)
Orchestrator->>Daemon: enqueue `workflow-run` job or send `scoped-job-offer`
Daemon->>Executor: accept & execute scoped job (or run pipeline)
Executor->>Database: post side-effects (comments, PRs, git pushes)
Daemon->>Orchestrator: send `scoped-job-completion`
Orchestrator->>Database: finalize execution row
Orchestrator->>Valkey: ZADD ship:tickle 0 <intentId> (on completion)
Valkey->>Orchestrator: scheduler ZRANGEBYSCORE -> due intent
Orchestrator->>Orchestrator: resumeShipIntent(intentId) -> runIteration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 40 minutes and 49 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 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/20260429-212559-ship-iteration-wiring/quickstart.md`:
- Around line 110-137: The quickstart's S7–S9 expected outcomes are inaccurate
relative to the implemented executors; update the S7 — fix-thread, S8 —
explain-thread and S9 — open-pr entries in quickstart.md (the three "S7", "S8",
"S9" section headings) to either mark these scenarios as deferred for this
release or change their "Expected" lines to match the scaffolded-halt behavior
(i.e., no push/resolve/new-PR) so the document reflects shipped behavior; keep
the section headings and example actions the same but edit the Expected
paragraphs accordingly and add a short note explaining that full
push/resolve/new-PR behavior is planned for a future release.
In `@specs/20260429-212559-ship-iteration-wiring/tasks.md`:
- Around line 195-197: The ordered-list prefixes on the three lines starting
with "5. T017–T023 (US2) → quickstart S3 passes.", "6. T024–T036 (US3) →
quickstart S4–S9 all pass.", and "7. T037–T043 (Polish + full e2e)." must be
changed to a valid Markdown ordered list to satisfy MD029; update those prefixes
to either a sequential "1., 2., 3." sequence or all "1." (e.g., "1.
T017–T023...", "2. T024–T036...", "3. T037–T043..." or all "1.") so the linter
accepts the list.
In `@src/daemon/job-executor.ts`:
- Around line 185-190: The scoped-job branch returns before registering the job
in the daemon's activeJobs map, so functions like evaluateOffer, heartbeats,
idle shutdown, and handleJobCancel treat scoped jobs as non-running; before
calling runScopedJob(payload, capabilities, send) you should create and insert
an ActiveJob record (including any cancellation token/state) keyed by the job id
into activeJobs, ensure runScopedJob is awaited/started while that ActiveJob
remains present, and remove/clear the ActiveJob and cancellation state after
runScopedJob completes or fails; update references around
payload.payload.scoped, runScopedJob, activeJobs, ActiveJob, and handleJobCancel
to rely on that registration so scoped jobs are tracked and cancellable.
- Around line 498-507: The logged event key uses scoped.jobKind directly
(producing values like "scoped-rebase") so change the code around the
logger.error call to normalize/mapping scoped.jobKind before constructing the
event key (e.g., strip a "scoped-" prefix or map known kinds such as
"scoped-rebase" -> "rebase", "scoped-fix_thread" -> "fix_thread"), then use the
normalized value in the event string passed to logger.error (the block that
builds { event: `ship.scoped.${scoped.jobKind}.daemon.failed`, ... } should be
updated to use the normalized variable); also update docs/OBSERVABILITY.md to
include the resulting event keys so they match the code.
In `@src/daemon/scoped-explain-thread-executor.ts`:
- Around line 51-83: In executeScopedExplainThread, guard the GitHub reply
posting by first checking the scoped-job idempotency using
input.triggerCommentId (or the corresponding idempotency field on
ScopedExplainThreadExecutorInput) and skip calling
octokit.rest.pulls.createReplyForReviewComment if that triggerCommentId was
already handled; if already processed return a halted/outcome indicating
deduplication without posting, otherwise mark the triggerCommentId as handled
(persist or atomic check) and then call createReplyForReviewComment and return
the existing threadReplyId as currently implemented.
In `@src/daemon/scoped-open-pr-executor.ts`:
- Around line 35-40: The status union on ScopedOpenPrOutcome is missing
"failed"; update the interface ScopedOpenPrOutcome to include readonly status:
"succeeded" | "failed" | "halted" so it matches the scopedJobResultSchema and
allows future error paths, and then ensure any code paths that construct
ScopedOpenPrOutcome (e.g., functions that return a scoped open PR result) can
produce "failed" where appropriate or are updated to satisfy the widened type.
In `@src/orchestrator/connection-handler.ts`:
- Around line 576-590: handleScopedJobCompletion currently trusts
msg.payload.deliveryId and clears offers without verifying the socket is a
registered owner; update handleScopedJobCompletion to first ensure
ws.data.daemonId is present/registered and that the execution referenced by
deliveryId is actually assigned to that daemon before removing offers or marking
failures: fetch the execution by deliveryId (e.g., via your existing
getExecutionByDeliveryId/findExecutionByDeliveryId function or equivalent),
compare execution.assignedDaemonId (or execution.daemonId) to ws.data.daemonId,
and if they don’t match or ws is unregistered, reject/ignore the scoped
completion (do not call removePendingOffer or proceed with failure handling);
keep getPendingOffer/removePendingOffer logic only after ownership is validated.
- Around line 592-623: The non-success branch must mirror handleResult() and
decrement the active-job counters that handleAccept() incremented, and ensure
markExecutionFinalized is called with the correct boolean (true only for
success); update the failure/halted path (the block using logger.warn, jobKind,
daemonId, offerId, deliveryId, msg.payload.reason) to: 1) decrement both scoped
active-job counters the same way handleResult() does, 2) call
markExecutionFinalized(deliveryId, false) for halted/failed (so only the success
branch uses markExecutionFinalized(deliveryId, true)), and 3) ensure any success
path still marks completed as before.
In `@src/orchestrator/job-queue.ts`:
- Around line 22-28: threadRefSchema currently validates startLine and endLine
independently so inverted ranges like {startLine:20,endLine:10} slip through;
add an object-level refinement to threadRefSchema (the Zod schema named
threadRefSchema) that enforces startLine <= endLine (or startLine < endLine if
inclusive ranges are not allowed) and returns a clear refinement error message,
so invalid ranges are rejected at the schema boundary before enqueueing or
passing to the scoped executors.
In `@src/shared/ws-messages.ts`:
- Around line 74-80: The scopedThreadRefSchema currently allows inverted ranges;
add a Zod refinement to enforce startLine <= endLine on scopedThreadRefSchema
(e.g., use .refine or .superRefine on scopedThreadRefSchema to validate
{startLine, endLine} and throw a clear error message like "startLine must be <=
endLine"); update the validation to attach the error to the relevant fields
(startLine/endLine) so parsing fails when ranges are inverted.
- Around line 158-179: The schema scopedJobResultSchema currently allows status:
"succeeded" without rebaseOutcome; update the "scoped-rebase" branch so that
when jobKind === "scoped-rebase" and status === "succeeded" rebaseOutcome is
required (non-optional), while statuses "failed" and "halted" keep rebaseOutcome
optional. Implement this by replacing the single z.object for scoped-rebase with
two explicit variants (one object for status: "succeeded" that includes a
required rebaseOutcome, and one object for status: z.enum(["failed","halted"])
that keeps rebaseOutcome optional) inside the discriminated union over jobKind;
keep the same nested rebaseOutcome discriminatedUnion structure and preserve
reason, commentId and other fields.
In `@src/workflows/ship/iteration.ts`:
- Around line 122-186: The DB→queue handoff is not atomic: insertQueued,
enqueueJob, and the final appendIteration (functions insertQueued, enqueueJob,
appendIteration) are separate cross-store writes and can leave inconsistent
state if Valkey or Postgres fails. Fix by making insertQueued and the
action-iteration appendIteration run inside a single Postgres transaction and
replace the direct enqueueJob call with an outbox insert (e.g., jobs_outbox or
workflow_outbox row containing childDeliveryId, run.id, workflowName) written in
that same transaction; remove the inline enqueueJob from this code path and let
the separate reconciler/worker pick up the outbox and call enqueueJob reliably
after the DB commit. Also ensure countIterations/countActionIterations logic
uses the committed iterations so the cap logic remains consistent.
In `@src/workflows/ship/log-fields.ts`:
- Around line 65-103: ShipLogFieldsSchema.event currently allows any string;
tighten it to only accept the canonical literals defined in SHIP_LOG_EVENTS by
deriving a union type from its values (e.g. extract the string literal union
from typeof SHIP_LOG_EVENTS) and use that union as the schema/type for
ShipLogFieldsSchema.event; update the schema definition that references
ShipLogFieldsSchema.event to use this derived union so any typos against
SHIP_LOG_EVENTS will fail validation at compile/runtime.
In `@src/workflows/ship/scoped/dispatch-scoped.ts`:
- Around line 71-77: deriveTriggerCommentId currently returns a fake positive id
(1) for commands without a thread_id which causes idempotency collisions; change
deriveTriggerCommentId(CanonicalCommand) to return an optional (number |
undefined) and return undefined when thread_id is missing/invalid instead of 1,
then update all callers (e.g., any durable idempotency key builders or scoped
job dispatch code that reads deriveTriggerCommentId) to handle undefined as a
distinct code path (do not fold undefined into a shared sentinel); ensure
durable idempotency keys incorporate the absence-of-trigger as its own value so
different non-threaded requests aren’t treated as duplicates.
In `@src/workflows/ship/session-runner.ts`:
- Around line 243-269: resumeShipIntent currently only logs and returns, so a
tickle doesn't actually resume processing; update resumeShipIntent (which calls
getIntentById) to flip a paused intent back to active (or ensure status remains
active), persist that change, and invoke the existing probe/enqueue logic used
by the normal ship loop (e.g., call the probe function and/or
enqueueShipIntent/queueNextIteration routine) so the intent is re-probed and the
next workflow iteration is scheduled; ensure any errors are logged and the
function returns after scheduling.
In `@src/workflows/ship/workflow-context.ts`:
- Around line 47-52: The extractShipIntentId function currently returns any
non-empty string; change it to validate that the extracted candidate is a UUID
before returning it: after obtaining candidate from state (in
extractShipIntentId) run it against a canonical UUID regex (case-insensitive
8-4-4-4-12 hex groups) and return undefined if it does not match. Keep the
existing null/object checks and only return candidate when typeof candidate ===
"string" and it matches the UUID pattern.
In `@test/webhook/events/issue-comment.test.ts`:
- Around line 53-59: The test embeds a duplicated SCOPED_JOB_KINDS array and
isScopedJob mock; extract these literals into a shared test helper so all suites
reuse the same source of truth. Create a helper (e.g., export SCOPED_JOB_KINDS
and isScopedJobMock from a test utils module) and update this test to import and
use those exports instead of the inline array and function, ensuring the mock
name matches the original symbols (SCOPED_JOB_KINDS, isScopedJob) so future
changes in src/orchestrator/job-queue.ts stay synchronized across tests.
In `@test/workflows/ship/iteration.test.ts`:
- Around line 265-270: The count parsing is overly defensive: simplify the
assertion by directly checking the numeric count returned by the query instead
of converting types; replace the current multi-step logic around iterRows and
count with a direct expectation on iterRows[0].count (the result of the SELECT
COUNT(*)::int query) to be 0, keeping the same query via requireSql and the same
variable names (iterRows) so it's easy to locate and update.
In `@test/workflows/ship/session-runner.resume.test.ts`:
- Around line 92-116: The test currently asserts the intent's status remains
"active", which locks in a no-op placeholder; instead, change the assertion to
verify the observable resume side-effect from resumeShipIntent (e.g., that a
tickle.due event was emitted/queued) rather than asserting status unchanged.
Locate the test using resumeShipIntent, insertIntent and getIntentById and
replace the status assertion with an assertion against the
scheduler/event-emitter mock or the persisted tickle event (whatever the
codebase uses to surface tickle.due) to confirm resumeShipIntent produced the
expected tickle.due side effect for the active intent.
🪄 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: 72a6a30f-2435-4838-af14-e3c0f07a77a2
📒 Files selected for processing (51)
CLAUDE.mddocs/BOT-WORKFLOWS.mddocs/OBSERVABILITY.mdscripts/check-no-destructive-actions.tsspecs/20260427-201332-pr-shepherding-merge-ready/tasks.mdspecs/20260429-212559-ship-iteration-wiring/checklists/requirements.mdspecs/20260429-212559-ship-iteration-wiring/contracts/job-kinds.mdspecs/20260429-212559-ship-iteration-wiring/contracts/ws-messages.mdspecs/20260429-212559-ship-iteration-wiring/data-model.mdspecs/20260429-212559-ship-iteration-wiring/plan.mdspecs/20260429-212559-ship-iteration-wiring/quickstart.mdspecs/20260429-212559-ship-iteration-wiring/research.mdspecs/20260429-212559-ship-iteration-wiring/spec.mdspecs/20260429-212559-ship-iteration-wiring/tasks.mdsrc/app.tssrc/daemon/job-executor.tssrc/daemon/scoped-explain-thread-executor.tssrc/daemon/scoped-fix-thread-executor.tssrc/daemon/scoped-open-pr-executor.tssrc/daemon/scoped-rebase-executor.tssrc/orchestrator/connection-handler.tssrc/orchestrator/job-dispatcher.tssrc/orchestrator/job-queue.tssrc/orchestrator/queue-worker.tssrc/shared/daemon-types.tssrc/shared/workflow-types.tssrc/shared/ws-messages.test.tssrc/shared/ws-messages.tssrc/webhook/router.tssrc/workflows/dispatcher.tssrc/workflows/handlers/ship.tssrc/workflows/orchestrator.tssrc/workflows/ship/iteration.tssrc/workflows/ship/log-fields.tssrc/workflows/ship/scoped/dispatch-scoped.tssrc/workflows/ship/session-runner.tssrc/workflows/ship/workflow-context.tstest/daemon/scoped-explain-thread-executor.test.tstest/daemon/scoped-fix-thread-executor.test.tstest/daemon/scoped-open-pr-executor.test.tstest/daemon/scoped-rebase-executor.test.tstest/orchestrator/connection-handler.test.tstest/orchestrator/job-dispatcher.test.tstest/webhook/events/issue-comment.test.tstest/webhook/router.test.tstest/workflows/dispatcher.test.tstest/workflows/handlers/ship.test.tstest/workflows/orchestrator.test.tstest/workflows/ship/iteration.test.tstest/workflows/ship/session-runner.resume.test.tstest/workflows/ship/tickle-scheduler.test.ts
Senior review on PR #79 surfaced three classes of breakage: 1. Scoped jobs were dead on the wire. The daemon had no `scoped-job-offer` handler (silent fall-through, then orchestrator timeout/retry until terminal), and `job:payload` for scoped jobs never carried the `scoped` discriminator. Fix: add the daemon-side case via new `evaluateScopedOffer`, thread `scoped` through `JobAcceptParams`/`handleAccept`/`handleJobAccept`, branch `handleAccept` to mint the installation token directly from `scoped.installationId` (skipping the legacy `executions.context_json` lookup that scoped jobs don't write). 2. The iteration → tickle → resume loop was open. `resumeShipIntent` was a stub that only logged, so every tickle fired by the cascade was a no-op until cap/deadline. Fix: inject an `octokitFactory` from `app.ts`, run `runProbe`, terminate on ready, otherwise call `runIteration`. Also add an in-flight guard so a non-terminal `workflow_runs` row tagged with `shipIntentId` blocks double-enqueue when cascade and a fresh comment trigger race. Cascade now skips ZADD for failed children so a permanently broken intent does not burn the whole iteration cap re-firing. 3. Scoped completion leaked one capacity slot per run. `handleScopedJobCompletion` never called `decrementActiveCount`/`decrementDaemonActiveJobs`. Fix: decrement on every status branch; replace the inverted `markExecutionFinalized(_, status === "halted")` with an explicit 3-state `finalizeScopedExecution` so halted is no longer collapsed onto `success=true`. Plus security hardening on the `scoped-rebase` credential helper — installation token now passed via `$.env({ GIT_TOKEN })` instead of embedded in a printf script body, helper file moved inside the 0700 `mkdtemp` workDir, single rm in finally. Three scaffold executors wrap Octokit calls so 4xx maps to `halted` (contractual outcome for scaffolding-only) instead of `failed`. `runScopedJob` switch gets a `never`-based exhaustiveness default. `WS_REJECT_REASONS` constants now actually used by the daemon. Hardcoded `chrisleekr-bot[bot]` replaced with `config.botAppLogin`. `PendingOffer.scoped` re-validated via `QueuedJobSchema.safeParse` before any cast. New tests cover every C/H fix file-by-file: scoped offer evaluator (C1/H3/H4), Octokit-error → halted (H2), capacity decrement on succeeded/failed/halted (C4), cascade skip on failed child (H1), in-flight guard (H6), terminal-ready on resume (C3). Quality gates: typecheck/lint/format/check:no-destructive all clean. Pre-existing cross-file mock-isolation test leak unchanged from baseline (each new test passes file-by-file). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…iminator PR #79 made QueuedJob a Zod discriminated union and updated the poison-pill log message; existing tests built jobs without kind and asserted the old copy, breaking CI on two files. Also extends the cancellation test's valkey mock to expose requireValkeyClient — iteration.ts (transitively imported via session-runner) now calls it on the legacy job-queue path. - test/orchestrator/job-queue.test.ts: makeQueuedJob adds kind: 'legacy'; log assertions match the new 'Job enqueued' payload and 'dropping poison pill' message. - test/workflows/ship/cancellation.test.ts: shared valkeyClient stub exposed via both getValkeyClient and requireValkeyClient; LPUSH/LMOVE/LREM no-ops added. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/workflows/ship/log-fields.ts (1)
70-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTighten
eventvalidation to this canonical literal set.
SHIP_LOG_EVENTSis now the source of truth, but Line 35 still accepts any non-empty string, so typoed event names will continue to pass validation and drift past tests/docs unnoticed.🤖 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 70 - 105, The event validator currently accepts any non-empty string; tighten it to the canonical literal set by deriving a union type from SHIP_LOG_EVENTS and using that for both compile-time and runtime checks. Create a type like ShipLogEvent = ExtractValues<typeof SHIP_LOG_EVENTS> (or use a mapped/recursive helper to flatten nested keys) and replace the loose string type in the validator/signature with ShipLogEvent; also update the runtime validation to check membership against the flattened values of SHIP_LOG_EVENTS (e.g., build a Set of Object.values(...) from SHIP_LOG_EVENTS) so only the exact literal identifiers are allowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/daemon/scoped-fix-thread-executor.ts`:
- Around line 72-98: The catch-all around
octokit.rest.pulls.createReplyForReviewComment incorrectly halts on transient
5xx/transport errors; change the catch in scoped-fix-thread-executor to inspect
the Octokit error status (e.g., err.status or HTTP status on the thrown error)
and: if status is a 4xx (client/semantic) treat as user-state change—log using
SHIP_LOG_EVENTS.scoped.fixThread.daemonFailed and return { status: "halted",
reason: ... }—otherwise rethrow the error so it propagates as a executor failure
(allowing retries/reporting); also include the original error details in the log
payload for diagnostics instead of the literal event string.
In `@src/daemon/scoped-open-pr-executor.ts`:
- Around line 67-93: The current catch in the scoped-open-pr executor around
octokit.rest.issues.createComment indiscriminately maps all errors to "halted"
and suppresses retries; change it to inspect the Octokit/HTTP error (the error
thrown by octokit.rest.issues.createComment) and only convert known
repository/issue terminal states to halted (e.g. HTTP 404/410 or other explicit
"issue missing/closed" codes your codebase treats as terminal); for all other
errors (network failures, 5xx, rate-limits, transient RequestErrors) re-throw
the error so the outer daemon handler records a failure and enables
retry/orchestration. Locate the try/catch surrounding
octokit.rest.issues.createComment in scoped-open-pr executor and replace the
blanket catch behavior with conditional handling based on err.status (or the
Octokit RequestError type/status) as described.
In `@src/orchestrator/job-dispatcher.ts`:
- Around line 361-377: In reconstructJobFromOffer, when
QueuedJobSchema.safeParse(offer.scoped) fails or isScopedJob(reparsed.data) is
false, do not fall through to legacy reconstruction; instead return a terminal
failure (e.g., throw a descriptive Error or return a failure result) right there
so the dispatcher won't re-enqueue a different job kind for the same repo/PR.
Use the existing context (offer.offerId, offer.deliveryId) and include
reparsed.error or a "shape-not-scoped" marker in the thrown error/returned
failure so callers can surface and handle this terminal corruption case; keep
the logger.error call but then stop execution of reconstructJobFromOffer rather
than continuing to legacy branches.
In `@src/workflows/ship/iteration.ts`:
- Around line 75-81: The inflight guard in iteration.ts that calls
findInflightShipIntentRun(intent.id, sql) is racy under concurrent resumes; wrap
the check+insert into a DB-enforced critical section: either obtain a row lock
(e.g., SELECT ... FOR UPDATE on the ShipIntent row) or use an advisory lock
keyed by intent.id around the logic that checks for an in-flight run and inserts
the child workflow_runs row, or instead enforce a unique in-flight constraint
(unique index keyed by shipIntentId) and handle duplicate-key retries; apply the
same protection to the similar logic referenced around lines 141-173 so only one
caller can create the in-flight run and others observe/handle the contention.
---
Duplicate comments:
In `@src/workflows/ship/log-fields.ts`:
- Around line 70-105: The event validator currently accepts any non-empty
string; tighten it to the canonical literal set by deriving a union type from
SHIP_LOG_EVENTS and using that for both compile-time and runtime checks. Create
a type like ShipLogEvent = ExtractValues<typeof SHIP_LOG_EVENTS> (or use a
mapped/recursive helper to flatten nested keys) and replace the loose string
type in the validator/signature with ShipLogEvent; also update the runtime
validation to check membership against the flattened values of SHIP_LOG_EVENTS
(e.g., build a Set of Object.values(...) from SHIP_LOG_EVENTS) so only the exact
literal identifiers are allowed.
🪄 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: cdae9cb2-ed88-4fda-bfeb-255ee990ec92
📒 Files selected for processing (21)
src/app.tssrc/daemon/job-executor.tssrc/daemon/main.tssrc/daemon/scoped-explain-thread-executor.tssrc/daemon/scoped-fix-thread-executor.tssrc/daemon/scoped-open-pr-executor.tssrc/daemon/scoped-rebase-executor.tssrc/orchestrator/connection-handler.tssrc/orchestrator/job-dispatcher.tssrc/workflows/orchestrator.tssrc/workflows/ship/iteration.tssrc/workflows/ship/log-fields.tssrc/workflows/ship/session-runner.tstest/daemon/scoped-fix-thread-executor.test.tstest/daemon/scoped-offer-evaluator.test.tstest/daemon/scoped-rebase-executor.test.tstest/orchestrator/connection-handler.test.tstest/orchestrator/job-dispatcher.test.tstest/workflows/orchestrator.test.tstest/workflows/ship/iteration.test.tstest/workflows/ship/session-runner.resume.test.ts
Acts on CodeRabbit comments on PR #79 covering thread-range validation, scoped-rebase outcome contract, daemon ownership checks, observability key normalization, and the scoped-open-pr type union. - ws-messages: split scoped-rebase result schema so 'succeeded' requires rebaseOutcome; add startLine<=endLine refinement on scopedThreadRefSchema. - job-queue: same startLine<=endLine refinement on the orchestrator-side threadRefSchema mirror. - daemon/job-executor: register scoped offers in activeJobs for the duration of runScopedJob so heartbeats/idle/cancel see the daemon as busy; emit per-kind 'ship.scoped.<kind>.daemon.failed' event keys matching the FR-018 names documented in docs/OBSERVABILITY.md. - orchestrator/connection-handler: reject scoped-job-completion from unregistered or non-owner daemons before mutating offer/capacity state; emit per-kind event keys. - daemon/scoped-rebase-executor: distinguish a real merge conflict (unmerged index entries) from other merge failures (auth/fs/unrelated histories); throw on the latter so the executor catch path returns halted instead of reporting a phantom conflict. - daemon/scoped-open-pr-executor: widen ScopedOpenPrOutcome.status to include 'failed' to match the WS schema. - workflows/ship/workflow-context: extractShipIntentId now validates via ShipIntentContextSchema.safeParse so non-UUID candidates are rejected. - test/iteration: simplify ::int count assertion. - specs/quickstart.md: mark S7-S9 expected results as 'scaffolding boundary in this slice' to match shipped halted behaviour. - specs/tasks.md: fix MD029 ordered-list prefixes (5/6/7 -> 1/2/3). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… bookkeeping T042 quickstart against @chrisleekr-bot-dev surfaced five real bugs that all silently broke the ship-iteration wiring on a real GitHub install: 1. NL classifier silently dropped Haiku-fenced JSON Anthropic Haiku 4.5 (TRIAGE_MODEL default) wraps single-object JSON responses in a markdown code fence even when the system prompt asks for raw JSON. Every NL-form trigger (`@chrisleekr-bot-dev <verb>`) was silently classified as `none` and fell through to the legacy intent classifier, which routes ship to issues only — making `@chrisleekr-bot-dev ship` on a PR a no-op. Added `stripJsonFence` helper and applied it in both nl-classifier and meta-issue-classifier. 2. Iteration handler skipped recordWorkflowExecution The new iteration handler enqueued workflow-run jobs without first inserting the executions row, so the daemon's accept handler rejected every offer with "No execution context found — producer did not call createExecution". Mirrored the legacy dispatcher's recordWorkflowExecution + enqueueJob ordering. 3. scoped-rebase clone collided with mkdtemp directory `git clone` refuses non-empty target directories; the executor passed the freshly-mkdtemp'd workDir directly. Clone now lands in workDir/repo while workDir still gates access via 0700. 4. scoped-rebase fetch refspec missed remote-tracking ref The clone uses `--single-branch --branch=<head_ref>` which scopes the remote.origin.fetch refspec to head only. A bare `git fetch origin <base_ref>` then downloaded commits but did not update `refs/remotes/origin/<base_ref>`, so `git merge` exited 1 with "not something we can merge". Pass an explicit refspec. 5. SC-007 false positive in resolve.ts agent prompt The `grep -rn "not yet wired" src/` pre-flight matched a prompt string about CI fix-attempt cross-run enforcement (unrelated to the four scoped-command notices SC-007 targets). Reworded to "a planned follow-up" so the literal pre-flight stays clean. Three integration tests added (T011+T015 ship-iteration loop with cascade ZADD; T018 tickle-scheduler resume; T028 scoped-rebase WS contract round-trip). Six new unit tests for stripJsonFence/fence-aware classifyComment. tasks.md: every task in the spec is now [X]. Verified end-to-end on chrisleekr/github-app-playground via the local @chrisleekr-bot-dev installation: S2 ship-from-thread, S4 rebase no-op, S5 rebase clean, S6 rebase conflict, S7 fix-thread, S8 explain-thread, S9 open-pr non-actionable, S9b open-pr actionable, S10 abort-ship, S11 stop+resume — all pass against real GitHub state. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Distinguish 4xx semantic halts from transient 5xx in three scoped daemon executors (fix-thread, explain-thread, open-pr): only client errors map to halted; transport / 5xx rethrow so the daemon's outer catch reports failed and the orchestrator can retry. - Switch the per-executor `*_reply_failed` log keys to the canonical `SHIP_LOG_EVENTS.scoped.*.daemonFailed` literals. - Fail closed in `reconstructJobFromOffer` when a scoped offer fails re-validation: return null and mark the execution failed instead of silently falling back to legacy/workflow-run reconstruction (which would dispatch the wrong job kind against the same target). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Replace fall-through ladder with exhaustive switch over RebaseOutcome.kind in runScopedJob (a future variant now fails type-check instead of silently mapping to "closed"). - Move SUPPORTED_SCOPED_KINDS const below the import block in daemon/main.ts (was wedged between two import groups). - Iteration handler: use serializeShipWorkflowContext from workflows/ship/workflow-context.ts so the producer side honors the schema the orchestrator's extractShipIntentId reader validates against (the helper was previously exported but unused). - Fix duplicate "// 8." comment numbering — second step is "// 9.". No behavior change. Quality gates clean (typecheck, lint, format); affected tests (scoped-rebase-executor, scoped-rebase-roundtrip, ship/iteration) pass in isolation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/daemon/main.ts (1)
167-172:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMark accepted offers as activity before sending
job:accept.
startEphemeralIdleLoop()keys offlastActiveAtMs, but neither accept path updates it. If an ephemeral daemon is near its idle timeout, it can accept an offer and still self-drain during the orchestrator-side token/context lookup becauseactiveJobsstays0untiljob:payloadarrives. CallmarkActive()before sendingjob:acceptin both branches.Suggested fix
if (evaluation.accept) { + markActive(); wsClient.send({ type: "job:accept", ...createMessageEnvelope(msg.id), payload: {}, @@ if (evaluation.accept) { + markActive(); wsClient.send({ type: "job:accept", ...createMessageEnvelope(msg.id), payload: {},Based on learnings,
src/daemon/main.tsmust exit afterEPHEMERAL_DAEMON_IDLE_TIMEOUT_MSof idle whenDAEMON_EPHEMERAL=true.Also applies to: 196-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daemon/main.ts` around lines 167 - 172, The accept-paths currently send wsClient.send({type: "job:accept", ...}) without updating activity, so startEphemeralIdleLoop() may think the daemon is idle; call markActive() immediately before sending job:accept in both acceptance branches (the block handling evaluation.accept around wsClient.send and the similar block at the later branch around lines 196-201) to update lastActiveAtMs; also ensure startEphemeralIdleLoop() enforces exit after EPHEMERAL_DAEMON_IDLE_TIMEOUT_MS when DAEMON_EPHEMERAL is true so ephemeral daemons actually terminate after the configured idle timeout.
♻️ Duplicate comments (2)
src/orchestrator/connection-handler.ts (2)
670-687:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
succeededandhaltedscoped runs still never leaverunning.Every scoped accept already calls
markExecutionRunning(offer.deliveryId), butfinalizeScopedExecution()only writes the"failed"branch. That leaves successful and halted scoped executions permanently inrunning, which breaks terminal-state queries and any retry/reaper logic that relies on executions reaching a terminal status. Even ifhaltedis intentionally non-failure, it still needs a terminal write such asmarkExecutionCompleted(...).Suggested fix
async function finalizeScopedExecution( deliveryId: string, status: "succeeded" | "halted" | "failed", ): Promise<void> { if (status === "failed") { @@ } return; } - // succeeded / halted — no-op on the executions row; the daemon executor - // already finalized any user-visible state (thread reply, push, etc.). + try { + await markExecutionCompleted(deliveryId, {}); + } catch (err) { + logger.warn( + { err: err instanceof Error ? err.message : String(err), deliveryId, status }, + "markExecutionCompleted for scoped completion failed (non-fatal)", + ); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/orchestrator/connection-handler.ts` around lines 670 - 687, finalizeScopedExecution currently only updates the "failed" path, leaving succeeded/halted runs stuck as "running"; update finalizeScopedExecution to write a terminal state for non-failed runs by calling the appropriate completion API (e.g. markExecutionCompleted or whatever terminal updater is used in this module) with the deliveryId and mapped terminal status for both "succeeded" and "halted" cases (ensure the call replaces/clears the running state that markExecutionRunning set); keep the existing error handling pattern (try/catch + logger.warn) around the completion call to mirror the failed branch behavior.
594-621:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMirror
handleResult()with a durable ownership/late-result guard here.
handleAccept()removes the offer at Line 727 beforehandleScopedAccept()runs, sogetPendingOffer(offerId)is usuallyundefinedby the time a real scoped completion arrives. That makes this check ineffective in production: a replayed or forgedscoped-job-completioncan still decrement capacity and drive finalization without proving it ownsdeliveryId. Validate against durable execution state before touching counters, e.g.getExecutionState(deliveryId)with bothstatusanddaemonIdchecks.Suggested hardening
const daemonId = ws.data.daemonId; const offerId = msg.payload.offerId; const { jobKind, status, deliveryId } = msg.payload; @@ - const offer = getPendingOffer(offerId); - if (offer?.daemonId !== undefined && offer.daemonId !== daemonId) { + const state = await getExecutionState(deliveryId); + if ( + state === null || + state.daemonId !== daemonId || + state.status === "completed" || + state.status === "failed" + ) { logger.warn( { event: "ws.scoped_completion.unauthorized", daemonId, offerId, - ownerDaemon: offer.daemonId, + deliveryId, + assignedDaemon: state?.daemonId ?? null, + currentStatus: state?.status ?? null, }, - "scoped-job-completion from non-owner daemon — ignoring", + "scoped-job-completion failed ownership/finality validation — ignoring", ); return; } + const offer = getPendingOffer(offerId); if (offer !== undefined) { removePendingOffer(offerId); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/orchestrator/connection-handler.ts` around lines 594 - 621, The ownership check using getPendingOffer(offerId) is insufficient because handleAccept/handleScopedAccept typically remove the in-memory offer before real scoped completions arrive; update the scoped completion path to mirror handleResult by first validating durable execution state via getExecutionState(deliveryId) (verify execution.status is not finalized and execution.daemonId === daemonId) before touching counters or removing offers, only call decrementActiveCount() and await decrementDaemonActiveJobs(daemonId) after that durable guard passes, and avoid removing persistent state or decrementing capacity on failed ownership/late-result checks; reference handleResult, handleAccept, handleScopedAccept, getPendingOffer, getExecutionState, decrementActiveCount, decrementDaemonActiveJobs, deliveryId, offerId, daemonId to locate the 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/20260429-212559-ship-iteration-wiring/tasks.md`:
- Around line 21-22: Update all occurrences of the incorrect integration test
path "tests/integration/" to the correct "test/integration/" used by this PR;
search for and replace the string "tests/integration/" (appearing e.g. in the
tasks.md descriptions and any references at the noted locations) so integration
test references point at the existing test/integration/ directory.
- Line 31: The task description incorrectly states there will be five variants
for the discriminated union on QueuedJob but then lists six names; update the
text in specs/20260429-212559-ship-iteration-wiring/tasks.md so the variant
count matches the listed names (either change "five" to "six" or remove one of
the listed variants), and ensure the change references the QueuedJob
discriminated union and the specific variant names ("legacy", "workflow-run",
and the four "scoped-*" variants) so the spec and the implementation guidance
are consistent.
In `@src/daemon/job-executor.ts`:
- Around line 239-245: The activeJobs entry currently sets workDir to an empty
string for scoped jobs causing cleanup to derive a bogus credential filename
(`${job.workDir}.cred.sh`) — change the setter in activeJobs.set(…) to omit
workDir or set it to null/undefined for scoped jobs (identify scoped jobs via
payload.payload.scoped) and update any cleanup logic that constructs
`${job.workDir}.cred.sh` to first guard with a truthy workDir check (e.g., if
(!job.workDir) skip credential-file cleanup) so credential helper deletion only
runs when a real workspace path exists.
- Around line 238-250: When tracking scoped jobs in activeJobs (in the block
that calls runScopedJob), also create and store a jobAbortController in the
shared jobAbortControllers map keyed by offerId so handleJobCancel can abort the
controller instead of only emitting a synthetic job:result; update the finally
cleanup to delete the controller entry as well as activeJobs. Modify
runScopedJob to accept or look up the jobAbortController and check
controller.signal.aborted (or listen for abort) before sending the final
scoped-job-completion so it bails out if cancelled, preventing a duplicate final
message and double capacity decrement; ensure any early exit path also skips
emitting completion. Make sure handleJobCancel uses the same controller to
trigger cancellation.
In `@src/daemon/scoped-fix-thread-executor.ts`:
- Around line 98-108: The current 4xx check in scoped-fix-thread-executor.ts
(the status variable and the block that logs with
SHIP_LOG_EVENTS.scoped.fixThread.daemonFailed and returns {status: "halted",
reason: ...}) is too broad and suppresses retries for transient GitHub 403/429
errors; change the logic so that only true terminal client errors (e.g.,
resource not found/closed comment or PR states you expect) result in the halted
return, while 403 (Forbidden due to rate limiting/abuse) and 429 (Too Many
Requests) are rethrown (or returned as retryable) so upstream retry logic can
act; keep using the same status extraction (typeof err === "object" ... "status"
in err) and the existing logging but branch first to rethrow when status === 403
|| status === 429, and restrict the halted branch to the specific terminal
status codes you consider non-retryable.
In `@src/daemon/scoped-open-pr-executor.ts`:
- Around line 93-103: The current check in scoped-open-pr-executor.ts that halts
on any 4xx treats transient throttling (403/429) as terminal; modify the logic
that computes status (from err) so only specific terminal statuses (e.g., 404,
410, 422 — choose the repo/issue-terminal codes your service expects) cause the
halted return and warn log. Specifically, replace the broad condition "typeof
status === 'number' && status >= 400 && status < 500" with an explicit whitelist
(e.g., [404, 410, 422].includes(status)) and leave 403 and 429 to bubble (do not
return { status: 'halted' } for those); adjust the log message to include the
status and reason when halting and ensure non-whitelisted statuses are rethrown
or returned so the caller can retry.
In `@src/orchestrator/job-queue.ts`:
- Around line 153-158: The logger call in the schema failure path currently logs
raw.slice(0, 200) which can leak user content; update the error logging in the
block where you call QueuedJobSchema.safeParse(parsed) (check variables result,
parsed, raw) to avoid including raw payload snippets and instead log safe
metadata such as raw length and a checksum/hash (e.g., SHA-256) of raw; keep the
existing result.error.issues in the log, replace the raw field with something
like { length: raw.length, sha256: <computed-hash> } so you surface traceable
info without storing free-form user content.
In `@src/workflows/ship/iteration.ts`:
- Around line 38-42: RunIterationDeps currently advertises an injectable Valkey
client (valkey) but runIteration ignores input.valkey and always calls the
global enqueueJob path; either wire the injected client through the queue helper
or remove the field. Fix by updating runIteration to pass input.valkey (type
Pick<RedisClient,"send">) into the queue helper call(s) that currently invoke
enqueueJob so enqueueJob (or a new wrapper) uses the provided client instead of
the global one, or remove valkey from RunIterationDeps and all references so the
public contract matches actual behavior (ensure to update any helper functions
that accept the client, e.g., enqueueJob or its callers in the 200-215 region).
In `@test/daemon/scoped-fix-thread-executor.test.ts`:
- Around line 30-49: Update the test fixtures passed to executeScopedFixThread
so threadRef.commentId and triggerCommentId are different (e.g.,
threadRef.commentId: 7777, triggerCommentId: 8888) in the blocks around the
three locations (lines ~30-49, ~63-76, ~95-108); ensure the test still expects
outcome.status and outcome.threadReplyId, but add/adjust the mockCreateReply
assertion to verify the outbound createReply call used the threadRef.commentId
value (the comment_id passed to createReplyForReviewComment/mockCreateReply
matches threadRef.commentId) rather than relying on identical IDs.
---
Outside diff comments:
In `@src/daemon/main.ts`:
- Around line 167-172: The accept-paths currently send wsClient.send({type:
"job:accept", ...}) without updating activity, so startEphemeralIdleLoop() may
think the daemon is idle; call markActive() immediately before sending
job:accept in both acceptance branches (the block handling evaluation.accept
around wsClient.send and the similar block at the later branch around lines
196-201) to update lastActiveAtMs; also ensure startEphemeralIdleLoop() enforces
exit after EPHEMERAL_DAEMON_IDLE_TIMEOUT_MS when DAEMON_EPHEMERAL is true so
ephemeral daemons actually terminate after the configured idle timeout.
---
Duplicate comments:
In `@src/orchestrator/connection-handler.ts`:
- Around line 670-687: finalizeScopedExecution currently only updates the
"failed" path, leaving succeeded/halted runs stuck as "running"; update
finalizeScopedExecution to write a terminal state for non-failed runs by calling
the appropriate completion API (e.g. markExecutionCompleted or whatever terminal
updater is used in this module) with the deliveryId and mapped terminal status
for both "succeeded" and "halted" cases (ensure the call replaces/clears the
running state that markExecutionRunning set); keep the existing error handling
pattern (try/catch + logger.warn) around the completion call to mirror the
failed branch behavior.
- Around line 594-621: The ownership check using getPendingOffer(offerId) is
insufficient because handleAccept/handleScopedAccept typically remove the
in-memory offer before real scoped completions arrive; update the scoped
completion path to mirror handleResult by first validating durable execution
state via getExecutionState(deliveryId) (verify execution.status is not
finalized and execution.daemonId === daemonId) before touching counters or
removing offers, only call decrementActiveCount() and await
decrementDaemonActiveJobs(daemonId) after that durable guard passes, and avoid
removing persistent state or decrementing capacity on failed
ownership/late-result checks; reference handleResult, handleAccept,
handleScopedAccept, getPendingOffer, getExecutionState, decrementActiveCount,
decrementDaemonActiveJobs, deliveryId, offerId, daemonId to locate the changes.
🪄 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: fb273bf0-2b90-4b9e-82eb-a957c6f6e9fa
📒 Files selected for processing (25)
specs/20260429-212559-ship-iteration-wiring/quickstart.mdspecs/20260429-212559-ship-iteration-wiring/tasks.mdsrc/daemon/job-executor.tssrc/daemon/main.tssrc/daemon/scoped-explain-thread-executor.tssrc/daemon/scoped-fix-thread-executor.tssrc/daemon/scoped-open-pr-executor.tssrc/daemon/scoped-rebase-executor.tssrc/orchestrator/connection-handler.tssrc/orchestrator/job-dispatcher.tssrc/orchestrator/job-queue.tssrc/shared/ws-messages.tssrc/workflows/handlers/resolve.tssrc/workflows/ship/iteration.tssrc/workflows/ship/nl-classifier.tssrc/workflows/ship/scoped/meta-issue-classifier.tssrc/workflows/ship/workflow-context.tstest/daemon/scoped-fix-thread-executor.test.tstest/integration/scoped-rebase-roundtrip.test.tstest/integration/ship-iteration-loop.test.tstest/integration/ship-tickle-resume.test.tstest/orchestrator/job-queue.test.tstest/workflows/ship/cancellation.test.tstest/workflows/ship/iteration.test.tstest/workflows/ship/nl-classifier.test.ts
Nine unresolved threads from the latest CodeRabbit pass: - job-executor scoped path now registers a jobAbortController and routes all scoped-job-completion sends through a sendIfNotAborted wrapper. handleJobCancel already aborts via the controller; combined, this stops cancel during a scoped run from emitting both a synthetic job:result and a later scoped-job-completion (was double-finalizing executions and double-decrementing capacity). - registerExitCleanup now skips rmSync when workDir === "" so a process exit during a scoped run cannot target `.cred.sh` in the daemon's CWD. - scoped-fix-thread, scoped-explain-thread, scoped-open-pr executors now rethrow on 429 and rate-limited 403 responses instead of halting on the blanket 4xx rule. Throttling is transient and must reach the orchestrator's retry layer. - job-queue parseQueuedJob and the leased-job poison-pill log now log rawLength only — `raw.slice(0, 200)` could leak triggerBodyPreview / verdictSummary user content into operator logs. - iteration.ts drops the unused `valkey` field from RunIterationDeps; the global enqueueJob path is the only writer, so the public contract no longer advertises an injectable seam that doesn't exist. - tasks.md: corrected `tests/integration/` → `test/integration/` (5 sites) and the off-by-one variant count (`five` → `six`) in the QueuedJob discriminator description. - scoped-fix-thread test fixtures use distinct commentId (7777) and triggerCommentId (8888) and assert the outbound comment_id matches threadRef.commentId so a future bug that wires the wrong id is caught. Quality gates clean (typecheck, lint, format); per-file tests pass for every touched module. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three remaining issues from CR review 4205792820 (round 3 fixes were already in c15bada; this round adds the items not yet covered): - daemon/main.ts now calls markActive() before sending job:accept in both the legacy and scoped offer branches. Previously the ephemeral idle loop keyed off lastActiveAtMs which only got bumped at job:payload, so a daemon near its EPHEMERAL_DAEMON_IDLE_TIMEOUT_MS could self-drain during the orchestrator-side token/context lookup that runs between accept and payload. - orchestrator/connection-handler.ts:finalizeScopedExecution now writes a terminal state for succeeded/halted via markExecutionCompleted. handleScopedAccept already calls markExecutionRunning, so without this every successful or halted scoped run stayed in 'running' forever — breaking FM-4 stale-execution recovery and any operator query that filters on terminal status. markExecutionCompleted only flips rows where status='running' so it is safe under races. - orchestrator/connection-handler.ts:handleScopedJobCompletion replaces the ineffective getPendingOffer-based ownership check with a durable getExecutionState guard that mirrors handleResult. handleAccept removes the in-memory offer before dispatching to handleScopedAccept, so the offer is gone by the time a real completion arrives — the previous check was essentially a no-op. The new guard validates daemonId match and non-finalized status against the executions row before touching capacity counters, so a forged/replayed completion cannot decrement capacity or finalize an execution it does not own, and late results for already-finalized executions are dropped. Tests: C4 tests in connection-handler.test.ts updated to seed matching execution state (the guard would otherwise correctly reject them); two new tests cover the guard's reject paths (forged-sender and late- finalized-result). The integration test in scoped-rebase-roundtrip also seeds the matching state. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Owner
Author
|
Round 4 follow-up — addressing the outside-diff comment on Fixed in commit |
There was a problem hiding this comment.
Pull request overview
This PR closes the remaining integration gaps so the ship workflow can run end-to-end by (a) bridging non-ready ship probe verdicts into the existing workflow_runs/daemon pipeline, (b) booting a tickle scheduler that resumes intents via ship:tickle, and (c) adding a new scoped-job WS protocol plus four scoped executors with capacity accounting.
Changes:
- Add a ship iteration handler that writes
workflow_runs.state.shipIntentId, enqueueskind:"workflow-run"jobs, and recordsship_iterations. - Start the tickle scheduler from
src/app.tsand add an orchestrator completion hook that ZADDsship:tickleonly for succeeded children and non-terminal intents. - Introduce scoped-job offer/completion WS schemas and queue/job-kind discriminated unions; add daemon-side scoped executors (rebase + three scaffolded executors) and tests.
Reviewed changes
Copilot reviewed 62 out of 62 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/workflows/ship/tickle-scheduler.test.ts | Adds round-trip tests for tickle scheduler behavior with an in-memory Valkey fake. |
| test/workflows/ship/session-runner.resume.test.ts | Adds DB-backed tests for resumeShipIntent no-op and ready-termination behavior. |
| test/workflows/ship/nl-classifier.test.ts | Adds regression tests for fenced JSON LLM output parsing (stripJsonFence). |
| test/workflows/ship/iteration.test.ts | Adds DB-backed tests for runIteration enqueue, cap/deadline, and in-flight guard. |
| test/workflows/ship/cancellation.test.ts | Extends Valkey mocks for new queue/tickle operations used by updated code paths. |
| test/workflows/orchestrator.test.ts | Adds coverage for ship-intent early-wake (ZADD ship:tickle) behavior and guards. |
| test/workflows/handlers/ship.test.ts | Updates job-queue mock surface to match new QueuedJob discriminated union. |
| test/workflows/dispatcher.test.ts | Updates job-queue mock surface to match new QueuedJob discriminated union. |
| test/webhook/router.test.ts | Updates job-queue mock surface to match new QueuedJob discriminated union. |
| test/webhook/events/issue-comment.test.ts | Updates job-queue mock surface to match new QueuedJob discriminated union. |
| test/orchestrator/job-queue.test.ts | Updates tests for new kind field + poison-pill logging behavior. |
| test/orchestrator/job-dispatcher.test.ts | Stubs new exports (QueuedJobSchema, scoped helpers) needed by dispatcher flow. |
| test/orchestrator/connection-handler.test.ts | Adds scoped completion tests ensuring capacity counters decrement and executions finalize. |
| test/integration/ship-tickle-resume.test.ts | Adds Postgres+Valkey integration test for reconciliation + onDue dispatch + ZREM. |
| test/integration/ship-iteration-loop.test.ts | Adds full integration test for verdict → enqueue → completion → ship:tickle ZADD. |
| test/integration/scoped-rebase-roundtrip.test.ts | Adds WS contract “round-trip” test for scoped offer/completion bookkeeping. |
| test/daemon/scoped-rebase-executor.test.ts | Adds daemon executor surface test for closed PR path (no git). |
| test/daemon/scoped-open-pr-executor.test.ts | Adds daemon executor surface test for scaffolded issue-reply behavior. |
| test/daemon/scoped-offer-evaluator.test.ts | Adds tests for daemon-side scoped-offer evaluation and reject taxonomy. |
| test/daemon/scoped-fix-thread-executor.test.ts | Adds daemon executor surface tests for scaffolded review-thread reply + error mapping. |
| test/daemon/scoped-explain-thread-executor.test.ts | Adds daemon executor surface test for scaffolded read-only thread reply. |
| src/workflows/ship/workflow-context.ts | Introduces a shared schema for workflow_runs.state.shipIntentId producer/consumer validation. |
| src/workflows/ship/session-runner.ts | Bridges non-ready verdicts to runIteration and adds resumeShipIntent for tickle wakeups. |
| src/workflows/ship/scoped/meta-issue-classifier.ts | Applies fenced-JSON unwrapping to meta-issue classifier parsing path. |
| src/workflows/ship/scoped/dispatch-scoped.ts | Replaces “not yet wired” notices by enqueueing scoped jobs and resolving thread refs. |
| src/workflows/ship/nl-classifier.ts | Adds stripJsonFence to handle fenced JSON model responses. |
| src/workflows/ship/log-fields.ts | Adds canonical SHIP_LOG_EVENTS event-key constants for iteration/tickle/scoped paths. |
| src/workflows/ship/iteration.ts | Adds the core ship iteration handler that writes rows and enqueues daemon work. |
| src/workflows/orchestrator.ts | Adds early-wake hook to ZADD ship:tickle based on state.shipIntentId and child success. |
| src/workflows/handlers/ship.ts | Updates enqueue payload to set kind:"workflow-run". |
| src/workflows/handlers/resolve.ts | Updates prompt text to reflect that cross-run enforcement is a follow-up (comment-only). |
| src/workflows/dispatcher.ts | Updates enqueue payloads to set kind:"workflow-run" for workflow-run jobs. |
| src/webhook/router.ts | Updates legacy queue payloads to set kind:"legacy". |
| src/shared/ws-messages.ts | Adds scoped-job offer/completion schemas and WS_REJECT_REASONS constants. |
| src/shared/ws-messages.test.ts | Adds schema round-trip tests for scoped-job offer and completion messages. |
| src/shared/workflow-types.ts | Documents the workflow_runs.state.shipIntentId convention for ship iteration correlation. |
| src/shared/daemon-types.ts | Adds PendingOffer.scoped field to carry scoped payload context through orchestration. |
| src/orchestrator/queue-worker.ts | Logs job.kind and guards workflowRunId logging by discriminant. |
| src/orchestrator/job-queue.ts | Converts queue payload to Zod-validated discriminated union, adds scoped job kinds, improves poison-pill logging. |
| src/daemon/scoped-rebase-executor.ts | Implements deterministic git merge/push rebase executor with temp-dir + credential helper handling. |
| src/daemon/scoped-open-pr-executor.ts | Adds scaffolded open-pr executor that posts a maintainer-facing issue comment and halts. |
| src/daemon/scoped-fix-thread-executor.ts | Adds scaffolded fix-thread executor that posts a review-thread reply and halts. |
| src/daemon/scoped-explain-thread-executor.ts | Adds scaffolded explain-thread executor that posts a review-thread reply and halts. |
| src/daemon/main.ts | Adds scoped-job-offer handling with capability checks and standard reject reasons. |
| src/daemon/job-executor.ts | Routes scoped payloads to per-kind executors, adds cancel suppression to avoid double-finalization, and adds offer evaluation for scoped jobs. |
| src/app.ts | Boots and stops the tickle scheduler in the server lifecycle and wires resume via injected Octokit factory. |
| specs/20260429-212559-ship-iteration-wiring/research.md | Documents key technical decisions for iteration correlation, job kind taxonomy, and scheduler boot wiring. |
| specs/20260429-212559-ship-iteration-wiring/quickstart.md | Adds operator quickstart scenarios for validating the end-to-end iteration + scoped-command flows. |
| specs/20260429-212559-ship-iteration-wiring/plan.md | Captures implementation plan and architecture decisions for the feature. |
| specs/20260429-212559-ship-iteration-wiring/data-model.md | Summarizes the feature’s touch points across Postgres/Valkey and application types. |
| specs/20260429-212559-ship-iteration-wiring/contracts/ws-messages.md | Documents scoped WS message contract additions. |
| specs/20260429-212559-ship-iteration-wiring/contracts/job-kinds.md | Documents new scoped job kinds and daemon executor expectations. |
| specs/20260429-212559-ship-iteration-wiring/checklists/requirements.md | Adds spec quality checklist for the feature package. |
| scripts/check-no-destructive-actions.ts | Extends destructive-action guard to include the new daemon scoped executor files. |
| docs/OBSERVABILITY.md | Documents iteration/tickle/scoped event keys and their meanings. |
| docs/BOT-WORKFLOWS.md | Documents the bridge architecture and scoped-command executor model with a diagram. |
| CLAUDE.md | Updates the active speckit plan pointer and adds tech context notes for this feature. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/daemon/main.ts (1)
219-227:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRefresh idle activity on
scoped-job-completiontoo.Line 224 only treats
"job:result"as a completion signal. Scoped jobs finish via"scoped-job-completion", so an ephemeral daemon can start its idle shutdown up to one poll interval early right after scoped work finishes.💡 Suggested fix
wsClient.send(m); if ( typeof m === "object" && m !== null && - (m as { type?: unknown }).type === "job:result" + ((m as { type?: unknown }).type === "job:result" || + (m as { type?: unknown }).type === "scoped-job-completion") ) { markActive(); }Based on learnings: "Applies to **/src/daemon/main.ts : Daemon entry point; when
DAEMON_EPHEMERAL=true, exit afterEPHEMERAL_DAEMON_IDLE_TIMEOUT_MSof idle".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/daemon/main.ts` around lines 219 - 227, The message handling in executeJob currently only refreshes idle activity when m.type === "job:result"; update the handler used in executeJob (the callback that calls wsClient.send and markActive) to also treat messages with type "scoped-job-completion" as a completion signal and call markActive for them as well so ephemeral daemons don't shut down early; locate the anonymous callback passed to executeJob in main.ts and add a condition checking (m as { type?: unknown }).type === "scoped-job-completion" alongside "job:result" before invoking markActive.
🤖 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/20260429-212559-ship-iteration-wiring/tasks.md`:
- Around line 122-123: Update the task note to reflect the landed design:
replace the instruction to call getPendingOffer(msg.id) from the scoped
completion flow with the durable guard getExecutionState(deliveryId) and state
that handleScopedJobCompletion must use getExecutionState(deliveryId) for
ownership/finalization checks; remove or mark as deferred the requirement that
each executor branch in runScopedJob enforce idempotency locally (executor-local
idempotency keyed by trigger comment is intentionally deferred); keep the
requirement to wire the server-side scoped-job-completion handler (validate
payload with the Zod discriminated union, post the formatted comment via the
appropriate scoped module such as src/workflows/ship/scoped/rebase.ts, call
removePendingOffer if still applicable for cleanup) and retain the unit test
expectation (one Octokit comment-create per completion kind).
In `@src/daemon/scoped-fix-thread-executor.ts`:
- Around line 95-110: Change the broad 4xx halt logic to an explicit allowlist
of terminal semantic statuses (404, 410, 422) in each of the three executors
(scoped-fix-thread-executor, scoped-explain-thread-executor,
scoped-open-pr-executor): replace the current "status >= 400 && status < 500 &&
!isRateLimited" check with a test that halts only when status is one of [404,
410, 422] and not isRateLimited; preserve the existing isRateLimited detection
and the log/return behavior (e.g., the block around
SHIP_LOG_EVENTS.scoped.fixThread.daemonFailed should only trigger for those
explicit statuses).
In `@src/workflows/ship/iteration.ts`:
- Around line 134-157: The code only checks verdict.reason is a string but must
confirm it is one of the allowed enum values before persisting a probe row or
selecting the next workflow; add a validation step that verifies verdict.reason
is in the permitted set (the same set/select logic used by selectNextWorkflow)
and throw a descriptive error if not, then only call appendIteration (the probe
row write that uses verdict.reason) and selectNextWorkflow afterward; reference
the verdict.reason guard, selectNextWorkflow, countIterations, and
appendIteration to locate the change and ensure invalid strings never get
persisted or forwarded.
---
Outside diff comments:
In `@src/daemon/main.ts`:
- Around line 219-227: The message handling in executeJob currently only
refreshes idle activity when m.type === "job:result"; update the handler used in
executeJob (the callback that calls wsClient.send and markActive) to also treat
messages with type "scoped-job-completion" as a completion signal and call
markActive for them as well so ephemeral daemons don't shut down early; locate
the anonymous callback passed to executeJob in main.ts and add a condition
checking (m as { type?: unknown }).type === "scoped-job-completion" alongside
"job:result" before invoking markActive.
🪄 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: 567c4e3f-a4a0-43e6-a0ce-f895b8bc18a8
📒 Files selected for processing (12)
specs/20260429-212559-ship-iteration-wiring/tasks.mdsrc/daemon/job-executor.tssrc/daemon/main.tssrc/daemon/scoped-explain-thread-executor.tssrc/daemon/scoped-fix-thread-executor.tssrc/daemon/scoped-open-pr-executor.tssrc/orchestrator/connection-handler.tssrc/orchestrator/job-queue.tssrc/workflows/ship/iteration.tstest/daemon/scoped-fix-thread-executor.test.tstest/integration/scoped-rebase-roundtrip.test.tstest/orchestrator/connection-handler.test.ts
Fixes the four unresolved review threads on PR #79 — three CodeRabbit findings (one major, one major+spillover, one minor doc nit) and one Copilot finding (major dedup-marker bug). * dispatch-scoped open-pr: refactor `runOpenPr` to expose `runOpenPrPolicy` (idempotency + classifier + non-actionable refusal, no marker post). dispatch-scoped now uses the policy phase + enqueues the daemon job on `actionable` verdicts. Removes the previous stub `createBranchAndPr` returning `pr_number: 0`, which caused the legacy flow to post `<!-- bot:open-pr:0 -->` and permanently block future re-triggers (Copilot review). * scoped-fix-thread / scoped-explain-thread / scoped-open-pr executors: narrow the "halt on terminal 4xx" branch to an explicit allowlist of semantic-state codes (404, 410, 422). 401 and generic 403 now rethrow so the orchestrator's outer failure path surfaces broken installation tokens / missing repo permissions instead of silently halting (CodeRabbit round 5). * iteration.ts: validate `verdict.reason` against the `NonReadinessReasonSchema` enum before persisting the probe row or selecting the next workflow. Adds an exhaustive `default` arm to `selectNextWorkflow` so an unrecognised reason throws rather than falling through (CodeRabbit round 5). * tasks.md T033/T033b: update task text to reflect the landed durable ownership guard (`getExecutionState(deliveryId)`) and explicitly mark executor-local idempotency as deferred follow-up (CodeRabbit round 5). Quality gates: tsc --noEmit clean, 0 ESLint errors, prettier all matched. Test counts unchanged from baseline (514 pass / 244 fail / 24 errors — the failures are the documented pre-existing inter-file mock contamination). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
7 tasks
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 📦🚀 |
5 tasks
6 tasks
3 tasks
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
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
Closes the three integration gaps left by PR #77 so the ship workflow runs end-to-end: (1) the iteration loop bridges non-ready probe verdicts onto the existing daemon
workflow_runspipeline, (2) the tickle scheduler boots fromapp.tsand the orchestrator cascade ZADDsship:tickleonly on succeeded children, and (3) four scoped commands (bot:rebase,bot:fix-thread,bot:explain-thread,bot:open-pr) execute on the wire via a newscoped-job-offer/scoped-job-completionWS protocol with capacity accounting. Includes the senior-review remediation commit (c4c300c) that connected the seams the per-phase commits left dead, plus follow-up fixes from three CodeRabbit rounds and four senior-review nits.Diagram
flowchart TD classDef before fill:#7a1f1f,stroke:#fff,color:#fff classDef after fill:#0b4d2c,stroke:#fff,color:#fff classDef shared fill:#1f3b7a,stroke:#fff,color:#fff Comment["@bot comment"]:::shared --> Intent["ship_intents row"]:::shared Intent --> StubIter["runIteration is a stub<br/>verdict ignored"]:::before Intent --> RunIter["runIteration -> workflow_runs<br/>state.shipIntentId set"]:::after StubIter --> Stranded["intent stranded<br/>no executor"]:::before RunIter --> Daemon["daemon executes<br/>workflow-run job"]:::after Daemon --> Cascade["onStepComplete<br/>maybeEarlyWakeShipIntent"]:::after Cascade --> CascadeOld["ZADD even on failed child<br/>burns iteration cap"]:::before Cascade --> CascadeNew["ZADD only when status=succeeded<br/>tickle skips stale entries"]:::after CascadeNew --> Tickle["ship:tickle ZSET<br/>scheduler onDue"]:::after Tickle --> ResumeOld["resumeShipIntent<br/>logs and returns"]:::before Tickle --> ResumeNew["resumeShipIntent runs probe<br/>terminates or runs next iteration"]:::after Comment --> ScopedOld["scoped-job-offer arrives<br/>daemon switch has no case<br/>orchestrator times out"]:::before Comment --> ScopedNew["daemon evaluateScopedOffer<br/>WS_REJECT_REASONS on miss<br/>scoped field threaded via job:payload"]:::after ScopedNew --> ScopedExec["runScopedJob<br/>rebase deterministic<br/>thread/open-pr scaffold halted"]:::after ScopedExec --> Complete["handleScopedJobCompletion<br/>decrement activeCount + daemonActiveJobs"]:::afterChanges
Phase commits (per-phase wiring):
59bd0ffPhase 1 —QueuedJobZod-validated discriminated union, scoped-job-offer/completion WS schemas,ShipIntentContextSchema.2eebd81Phase 2 — daemon scoped-job dispatch surface, orchestrator early-wake cascade,SHIP_LOG_EVENTSnamespace.1ef3924Phase 3 — US1runIterationhandler + cap/deadline accessors + session-runner placeholder replaced.d250ad4Phase 4 — US2 tickle scheduler boot wiring + graceful shutdown.96e6db8Phase 5 — US3 four daemon executors +dispatch-scopedenqueue + connection-handler completion bridge.f41a2a4Phase 6 —docs/BOT-WORKFLOWS.mdbridge diagram +docs/OBSERVABILITY.mdevent keys + retrospective annotations.Senior-review remediation (
c4c300c, 11 issues):case "scoped-job-offer"added tosrc/daemon/main.ts; newevaluateScopedOfferwith capacity + memory + disk floors.JobAcceptParams.scopedthreaded throughsrc/orchestrator/job-dispatcher.ts;handleScopedAcceptvalidatesPendingOffer.scopedvia Zod re-parse before cast.resumeShipIntentnow runsrunProbeand bridges torunIteration/transitionToTerminal;octokitFactoryinjected fromsrc/app.ts.handleScopedJobCompletioncallsdecrementActiveCount+decrementDaemonActiveJobs.scoped-rebase-executor: token via env var, credential helper insideworkDirwith0700, singlerminfinally.scoped-fix-thread,scoped-explain-thread,scoped-open-pr) wrap Octokit calls with 4xx-halt vs 5xx-rethrow branching.WS_REJECT_REASONSconstants now used in daemon (replaces ad-hoc strings).runScopedJobswitch.findInflightShipIntentRunguard prevents cascade-while-running double-enqueue.config.botAppLogin; halted semantics fixed.CodeRabbit follow-up rounds (
5bb04d0,c57d058,c15bada) + e2e/integration fixes (cd36ac9,bcc0b51).CodeRabbit round 3 (
c15bada, 9 threads):jobAbortController+sendIfNotAbortedwrapper stop double-finalization (syntheticjob:result+ laterscoped-job-completion) and the matching double capacity-decrement.registerExitCleanupskipsrmSyncwhenworkDir === ""— guards against deleting.cred.shin the daemon CWD if a process exit fires during a scoped run.429and rate-limited403now rethrow inscoped-fix-thread,scoped-explain-thread,scoped-open-prso throttling reaches the orchestrator retry layer instead of silently halting the execution.job-queueparse failures + leased-job poison-pill logs now recordrawLengthonly —raw.slice(0, 200)could leaktriggerBodyPreview/verdictSummaryuser content into operator logs.RunIterationDepsdrops unusedvalkeyfield — public contract no longer advertises an injectable seam with no callers.tasks.md:tests/integration/→test/integration/(5 sites);QueuedJobvariant countfive→six.scoped-fix-threadtest fixtures: distinctcommentId(7777) vstriggerCommentId(8888); assertion now pins outboundcomment_idtothreadRef.commentIdso a future swap fails loudly.Senior-review nits (
15caa9d):switchoverRebaseOutcome.kindinrunScopedJob— future variants now fail TypeScript instead of silently mapping toclosed.serializeShipWorkflowContextso producer-side validation matches the orchestrator's reader.SUPPORTED_SCOPED_KINDSconst moved below the import block indaemon/main.ts.// 8.→// 9.initeration.ts.Tests added/updated:
test/daemon/scoped-offer-evaluator.test.ts(4 cases — accept supported, reject unknown viaSCOPED_KIND_UNSUPPORTED, memory floor, capacity baseline).test/orchestrator/connection-handler.test.ts(3 capacity-decrement cases for succeeded/failed/halted).test/integration/ship-iteration-loop.test.ts,test/integration/ship-tickle-resume.test.ts,test/integration/scoped-rebase-roundtrip.test.ts.test/workflows/orchestrator.test.ts(H1 — failed child does not ZADD),test/workflows/ship/iteration.test.ts(H6 — in-flight guard).test/workflows/ship/session-runner.resume.test.ts(mockrunProbe, exercise terminal/missing/active branches).Related Issues
specs/20260429-212559-ship-iteration-wiring/Test plan
bun run typecheck,bun run lint,bun run format,bun run check:no-destructive)bun run check+bun run test)bot:rebaseno-op, clean merge, conflict)bot:fix-thread/bot:explain-thread/bot:open-prscaffolding boundaries)Summary by CodeRabbit
Release Notes
New Features
Tests
Documentation