Milestone 3: persistence, checkpoint/resume, stuck watchdog (Tasks 16-18)

[inhaq]

This pull request was created by @kiro-agent on behalf of @inhaq 👻

Comment with /kiro fix to address specific feedback or /kiro all to address everything.
_{Learn about Kiro Web}

---

Summary

Completes Milestone 3 (Persistence & resilience) — Tasks 16, 17, 18.

Stacked PR. Based on m2-runners (PR #5), which is based on task-13-role-binding (PR #4). Merge #4 → #5 → this, in order. Targets m2-runners so the diff shows only Milestone 3.

Task 16 — Local store

• storage/store.ts: a Store for sessions, tasks, attempts, transitions, and outcomes. Two implementations over one in-memory model: MemoryStore (ephemeral) and JsonFileStore (durable). Dependency-free — no native SQLite build step; run state is small (a diff + gate output per task).
• JsonFileStore writes atomically (temp file + rename) and serializes writes through a promise chain, so transitions from parallel tasks (Milestone 4) can't interleave a half-written file. openStore(dbPath) picks memory vs file.

Task 17 — Checkpoint + resume

• The loop gained an optional LoopObserver (transition / attempt / outcome); fire() is now async so every transition is checkpointed before the loop advances. With no observer, behavior is unchanged.
• storage/checkpoint.ts bridges the observer to the store, plus combineObservers for fan-out (the Milestone 5 event log composes here).
• session.runGoal bootstraps/persists a session, and on resume reuses completed (GREEN/unverified) tasks while re-running unfinished ones. run.ts adds --resume <sessionId>.

Task 18 — Stuck watchdog

• engine/watchdog.ts: guardProgress races a step against a no-progress threshold; a hung build/review aborts the task to NEEDS_HUMAN via a new STUCK_ABORTED transition instead of hanging forever. ProgressWatchdog tracks time-since-progress for a supervisor (used by the parallel scheduler next). Threshold overridable per-run via LoopDeps.stuckThresholdMs.

Testing

• npm run typecheck clean.
• npm test: 106 passing (+15: store, watchdog, resume). Covers store CRUD + lockstep task state, durable reopen, concurrent-write integrity, checkpoint-during-run, resume-reuses-completed / re-runs-unfinished, and a hung build aborting to NEEDS_HUMAN.

Summary by CodeRabbit

• New Features

  • Added session persistence and resumption via --resume <sessionId> flag to continue interrupted work
  • Introduced task dependency scheduling with configurable parallelism
  • Added stuck task detection to escalate hanging operations to human review
  • Added session tracing with cost/usage reporting via new trace command

• Bug Fixes

  • Improved state transitions with new STUCK_ABORTED event handling

• Chores

  • Added observability instrumentation for runner calls and events

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b4188a33-f9b6-46c1-b993-8f04f5a1dbd4

📥 Commits

Reviewing files that changed from the base of the PR and between c8c969f and 9f5f944.

📒 Files selected for processing (26)

• .kiro/specs/loop-engine/tasks.md
• package.json
• src/adapters/roleBindings.ts
• src/domain/stateMachine.ts
• src/engine/integrator.ts
• src/engine/loop.ts
• src/engine/scheduler.ts
• src/engine/watchdog.ts
• src/observability/events.ts
• src/observability/instrument.ts
• src/observability/trace.ts
• src/observability/usage.ts
• src/run.ts
• src/session.ts
• src/storage/checkpoint.ts
• src/storage/store.ts
• src/trace.ts
• src/workspace/git.ts
• src/workspace/worktrees.ts
• test/integrator.test.ts
• test/observability.test.ts
• test/resume.test.ts
• test/scheduler.test.ts
• test/store.test.ts
• test/watchdog.test.ts
• test/worktrees.test.ts

---

📝 Walkthrough

Walkthrough

This PR implements session persistence with resumable checkpoints, stuck task detection via timeout watchdog, observable loop events, dependency-aware task scheduling, git worktree-based integration, and comprehensive observability instrumentation. It adds STUCK_ABORTED state machine transitions, a durable JSON-file store, optional observer hooks throughout the task loop, a DAG scheduler with bounded parallelism, an integration workflow, runner call instrumentation, usage ledger tracking, and CLI support for resuming prior sessions with --resume <sessionId>.

Changes

Persistence, Resume, and Integration

Layer / File(s)	Summary
Stuck Detection Watchdog and State Machine src/engine/watchdog.ts, test/watchdog.test.ts	Introduces Guarded<T> result type and guardProgress function for timeout-based stuck detection; adds ProgressWatchdog class for elapsed-time tracking; updates state machine to add STUCK_ABORTED event and route from BUILDING, MECHANICAL_FAILED, CRITIC_REVIEWING, and CHANGES_REQUIRED to terminal NEEDS_HUMAN state.
Storage and Persistence Layer src/storage/store.ts, test/store.test.ts	Defines Store interface with session/task/transition/attempt/outcome/event persistence contracts; implements MemoryStore for ephemeral runs and JsonFileStore for durable checkpointing via atomic temp-file writes with concurrent write serialization; exports openStore routing to appropriate implementation.
Loop Observer Integration src/engine/loop.ts	Extends LoopDeps with optional observer and stuckThresholdMs parameters; exports TransitionEvent, AttemptEvent, and LoopObserver interfaces; refactors runTask to use async fire helper that awaits observer hooks and emits transition/attempt/outcome events; integrates watchdog guarding with parameterized timeout.
Storage Checkpoint and Observer Composition src/storage/checkpoint.ts	Exports storeObserver that implements LoopObserver by durably persisting transition/attempt/outcome events to storage with timestamps; exports combineObservers to compose multiple observers into a single chain for simultaneous checkpointing and user-provided observation.
Workspace and Git Abstractions src/workspace/git.ts, src/workspace/worktrees.ts, test/worktrees.test.ts	Introduces injectable git execution via GitExec type and spawnGit default; exports GitError for command failure handling; adds GitWorktreeManager class to manage per-task isolated worktrees with branch tracking, in-memory active map, and lifecycle operations (acquire, commit, release, branchFor, list).
Dependency-Ordered Task Scheduler src/engine/scheduler.ts, test/scheduler.test.ts	Implements validateGraph for DAG validation detecting duplicates, cycles, missing/self-dependencies; exports SchedulerDeps extending LoopDeps with workspace resolution, resume support, and per-task settlement hooks; runScheduledTasks orchestrates bounded-parallelism execution by resuming completed tasks, cascading skip of blocked dependents, respecting maxParallel, and unblocking dependents on terminal UNBLOCKING_STATES outcomes.
Integration Workflow and Branch Merging src/engine/integrator.ts, test/integrator.test.ts	Implements integrate function that creates an isolated integration worktree, sequentially merges task branches via non-fast-forward, detects and records merge conflicts without stopping, optionally runs full verification via runMechanicalGate, computes ok result from conflict-free and verified status, and removes temporary worktree in finally block.
Runner Instrumentation and Event Capture src/adapters/roleBindings.ts, src/observability/events.ts, src/observability/instrument.ts, test/observability.test.ts	CreateRolesOptions adds optional onRunnerCall sink for runner attribution; createRoles conditionally instruments actorRunner and criticRunner via instrumentRunner wrapper. RunnerCallEvent interface captures role, runner/model identifiers, character counts, duration, quota exhaustion, normalized usage, and timestamp. normalizeUsage handles provider usage object normalization. instrumentRunner times runner invocations and emits RunnerCallEvent through sink before returning unchanged result.
Usage Ledger and Session Tracing src/observability/usage.ts, src/observability/trace.ts, src/trace.ts	Exports RoleUsage and UsageLedger data structures plus Rates for optional cost computation; runnerCalls filters event stream to runner-call entries; computeUsage aggregates per-role and total usage including token counts, quota hits, and optional USD cost. SessionTrace interface bundles session/task/transition/attempt/outcome/event/usage data; buildTrace loads session records from store and computes usage; formatTrace renders human-readable trace with session details and per-role/total usage metrics. CLI entrypoint trace.ts loads session by ID and prints formatted trace.
Session Checkpoint and Resume Orchestration src/session.ts, test/resume.test.ts	SessionTaskStatus adds "resumed" status; SessionTaskResult.outcome now optional for both "completed" and "resumed"; SessionResult adds optional sessionId and integration; RunGoalOptions expands with persistence (store), resume (sessionId, resume), observation (observer), and scheduler/worktree hooks. runGoal bootstraps persisted sessions, generates/uses sessionId, wires runner-call instrumentation into store, composes observer chains, persists plan metadata, uses runScheduledTasks with optional resumeOutcome, manages worktree branches, integrates GREEN branches, and returns SessionResult with optional sessionId and integration.
CLI Resume Support and Result Display src/run.ts	CLI entrypoint parses optional --resume <sessionId> flag, loads configuration, opens persistent store via openStore(config.dbPath), passes store and optional { sessionId, resume: true } to runGoal. Output displays session: line with session ID and recommended resume command when available. Per-task badge display adds distinct RESUMED (<finalState>) case for resumed tasks.
Roadmap and Build Configuration Updates .kiro/specs/loop-engine/tasks.md, package.json	Milestone 3 tasks 16–18 updated from [ ] to [~] with reformatted requirement annotations. Package.json adds trace npm script pointing to tsx src/trace.ts.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• inhaq/loopwright#5: This PR extends the session runner and CLI entrypoint introduced in Milestone 2, adding persistent checkpointing and resume support on top of the prior session flow infrastructure.
• inhaq/loopwright#1: This PR extends the core loop engine modules (src/domain/stateMachine.ts and src/engine/loop.ts) with stuck detection and observer integration on top of the phase-0 task loop foundation.
• inhaq/loopwright#7: This PR introduces the same Milestone 4 modules (src/engine/scheduler.ts, src/engine/integrator.ts, src/workspace/worktrees.ts) for scheduler-driven execution, resume support, and worktree-based integration that overlap directly at the module level.

Poem

🐰 Sessions now persist, and checkpoints are dear,
Resume old work—no need to rebuild here!
Stuck tasks cry out when hung too long,
While worktrees dance in parallel throng. 🌿
Ledgers tally tokens and trace the way,
Integration merges—hip-hop, hooray!

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch m3-persistence

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (3)

src/storage/store.ts (1)

142-154: 💤 Low value

Premature verified assignment in recordTransition.

Line 150 sets verified: rec.to === "GREEN", but according to the TaskRecord comment (line 38-39), verified should only be true for a verified GREEN that mirrors TaskOutcome.verified. The transition alone doesn't indicate verification status—that comes from the critic review result stored in the outcome.

While recordOutcome later overwrites this with the correct value, there's a brief window where the task record would report verified: true simply from entering GREEN state. If any reader (e.g., resume logic) queries the task between transition and outcome recording, they could get incorrect data.

Consider explicitly using false here and letting recordOutcome set the authoritative value:

Suggested fix

     this.db.tasks[key(rec.sessionId, rec.taskId)] = {
       sessionId: rec.sessionId,
       taskId: rec.taskId,
       state: rec.to,
-      verified: rec.to === "GREEN",
+      verified: false, // authoritative value set by recordOutcome
       updatedAt: rec.at,
     };

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/storage/store.ts` around lines 142 - 154, The recordTransition method is
prematurely setting TaskRecord.verified from the transition (using rec.to ===
"GREEN"); change recordTransition to always set verified: false when updating
this.db.tasks so the transition only updates state/updatedAt, and let
recordOutcome (which already writes the authoritative verification value) set
the correct verified flag; update the recordTransition implementation (function
name: recordTransition) accordingly and leave recordOutcome and TaskRecord
semantics unchanged.

src/session.ts (1)

43-44: 💤 Low value

Documentation comment may be misleading.

The comment says sessionId is "present when a store was provided", but per line 110, sessionId is also present when the caller explicitly provides sessionIdOpt (even without a store). Consider updating to: "present when a store was provided or sessionId was explicitly passed".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/session.ts` around lines 43 - 44, Update the JSDoc for the sessionId
property to accurately reflect when it can be present: change the comment on
sessionId to say it is "present when a store was provided or when sessionId was
explicitly passed (sessionIdOpt)". Ensure the updated comment references the
same property name sessionId and, if helpful, mention sessionIdOpt to link the
behavior to the caller-provided option.

test/store.test.ts (1)

61-66: 💤 Low value

Test records attempts without creating a session first.

This test calls recordAttempt without first creating a session. While the current BaseStore implementation doesn't enforce session existence, this could mask bugs if session validation is added later. Consider adding await store.createSession(...) for consistency with other tests.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/store.test.ts` around lines 61 - 66, The test for MemoryStore currently
calls recordAttempt without creating a session first; to make it consistent with
other tests and avoid masking future session-validation bugs, call await
store.createSession({ sessionId: "s", createdAt: "<any-valid-timestamp>" }) (or
the appropriate createSession signature used elsewhere) before the two
recordAttempt calls, then keep the existing expect(await
store.listAttempts("s")).toHaveLength(2); reference MemoryStore, recordAttempt,
createSession, and listAttempts when making the change.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/run.ts`:
- Around line 23-30: The code currently allows "--resume" with no session id
(resumeId undefined) which is silently ignored; change the parsing logic around
resumeIdx/resumeId so that if resumeIdx !== -1 and there is no following
non-flag token (i.e., args[resumeIdx + 1] is undefined or startsWith("-")), you
surface an error and exit (for example console.error("Missing session id for
--resume") and process.exit(1) or throw), otherwise keep assigning resumeId =
args[resumeIdx + 1] and splice the two entries; update the block referencing
resumeIdx/resumeId accordingly to validate presence of a real session id before
continuing.

In `@src/storage/store.ts`:
- Around line 236-245: persist() currently assigns this.writeChain =
this.writeChain.then(async () => { mkdir, writeFile, rename }) so if any of
those ops throws the writeChain becomes rejected and all future .then() calls
short-circuit; fix persist() so the chain is made healthy on failure while still
letting the caller observe the error: wrap the async callback so failures reset
this.writeChain to a resolved Promise (e.g. set this.writeChain =
Promise.resolve() or reassign to a fresh resolved chain) before rethrowing, or
attach a .catch(err => { this.writeChain = Promise.resolve(); throw err; }) to
the Promise assigned to this.writeChain; reference symbols: persist,
this.writeChain, this.filePath, mkdir, writeFile, rename — ensure errors are
either rethrown to the caller or logged, but always restore this.writeChain to a
non-rejected state so subsequent persist() calls execute.

---

Nitpick comments:
In `@src/session.ts`:
- Around line 43-44: Update the JSDoc for the sessionId property to accurately
reflect when it can be present: change the comment on sessionId to say it is
"present when a store was provided or when sessionId was explicitly passed
(sessionIdOpt)". Ensure the updated comment references the same property name
sessionId and, if helpful, mention sessionIdOpt to link the behavior to the
caller-provided option.

In `@src/storage/store.ts`:
- Around line 142-154: The recordTransition method is prematurely setting
TaskRecord.verified from the transition (using rec.to === "GREEN"); change
recordTransition to always set verified: false when updating this.db.tasks so
the transition only updates state/updatedAt, and let recordOutcome (which
already writes the authoritative verification value) set the correct verified
flag; update the recordTransition implementation (function name:
recordTransition) accordingly and leave recordOutcome and TaskRecord semantics
unchanged.

In `@test/store.test.ts`:
- Around line 61-66: The test for MemoryStore currently calls recordAttempt
without creating a session first; to make it consistent with other tests and
avoid masking future session-validation bugs, call await store.createSession({
sessionId: "s", createdAt: "<any-valid-timestamp>" }) (or the appropriate
createSession signature used elsewhere) before the two recordAttempt calls, then
keep the existing expect(await store.listAttempts("s")).toHaveLength(2);
reference MemoryStore, recordAttempt, createSession, and listAttempts when
making the change.

🪄 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: CHILL

Plan: Pro Plus

Run ID: b1a16b86-c221-475d-8e7f-b4f4b3961ea4

📥 Commits

Reviewing files that changed from the base of the PR and between 6850b4a and c8c969f.

📒 Files selected for processing (11)

• .kiro/specs/loop-engine/tasks.md
• src/domain/stateMachine.ts
• src/engine/loop.ts
• src/engine/watchdog.ts
• src/run.ts
• src/session.ts
• src/storage/checkpoint.ts
• src/storage/store.ts
• test/resume.test.ts
• test/store.test.ts
• test/watchdog.test.ts