Skip to content

feat(harness): coordinate project bootstrap [Agent Map 06/15] - #825

Merged
ynadge merged 1 commit into
mainfrom
review/agent-map-06-bootstrap-coordinator
Sep 6, 2026
Merged

feat(harness): coordinate project bootstrap [Agent Map 06/15]#825
ynadge merged 1 commit into
mainfrom
review/agent-map-06-bootstrap-coordinator

Conversation

@ynadge

@ynadge ynadge commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Primary change type

  • Feature

Problem and motivation

Automatic map creation must yield to user work and recover consistently across readiness changes, retries, process replacement, and shutdown.

Summary and scope

Add the complete bootstrap and user-input coordinator over the durable store: evidence-first turn generation, FIFO input, readiness and completion correlation, preemption, and recovery. Server activation follows in the next stack increment.

Emit recovery metadata only after its state commits, avoiding contradictory duplicate lifecycle events. Injection failure retains the correct retryable recovery state.

How this increment fits

The complete bootstrap state machine is tested as one unit over part 5 storage and part 4 input delivery. Server activation and old-path removal stay together in part 7.

Stack and review boundary

  • Part 06 of 15 in the Agent Map review stack; review this increment against its predecessor.
  • Base: review/agent-map-05-bootstrap-state.
  • Current head: d10f6055f0d6c7336a46593c304cecfbddf0ef9c; 7,556 changed lines across 3 files, counting additions and deletions including tests.
  • Repackages the corresponding final behavior from #804. Original code and review history remain preserved.
  • Complete coworker testing branch: fix/studio-onboarding-followups.
  • The stack remains unmerged. Dependent PRs target their predecessor, so their diffs do not repeat earlier increments.

Related work

Agent Map checkpoint SAP-3147; relevant work SAP-3148. This packaging follows the maintainer-approved 15-PR split.

Validation

Root checks ran against c3023e9e643e21ccc503969243666faa081e3462. The final head changes only README terminology or commit ancestry; a complete tracked-file comparison confirms identical executable source and build inputs. The terminology gate was rerun on d10f6055f0d6c7336a46593c304cecfbddf0ef9c.

pnpm build — passed (exit 0)
pnpm typecheck — passed (exit 0)
pnpm lint — passed (exit 0)
pnpm test — passed (exit 0)

Tests and documentation

Regression coverage: FIFO user input, initial-input precedence, readiness and completion correlation, durable recovery, preemption, failed injection, and shutdown fencing.

See part 15 for integrated browser, native CLI, and Mac journey validation. The checks above were run independently on this PR’s own commit.

Linux tests run with ordinary user filesystem permissions; the sandbox's extra ambient capabilities are dropped. Hosted CI and automated review are separate from these recorded local results.

Compatibility and release impact

  • Compatibility: Internal coordinator groundwork. Existing server startup remains active until the next increment.
  • Changeset: Included: .changeset/bootstrap-coordinator.md

Security

  • No secrets, credentials, private user data, or unsanitized logs are included.
  • This PR does not publicly disclose a suspected vulnerability.

AI assistance

  • Codex assembled the implementation, addressed reproduced defects, supplied tests and documentation, inspected the diff, and ran the checks above. Reviews are handled by hosted PR automation.

Checklist

  • Read CONTRIBUTING.md; implementation follows the requested 15-PR split.
  • Description reflects this PR's actual predecessor-relative diff.
  • Relevant tests accompany the changed behavior.
  • Root build, typecheck, lint, and test evidence matches the final implementation; any documentation-only update is identified above.
  • Release/documentation treatment is explained above.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Review — PR #825 (Agent Map 06/15, bootstrap coordinator)

Scope reviewed: .changeset/bootstrap-coordinator.md, packages/harness/src/core/project-bootstrap.ts,
packages/harness/src/core/project-bootstrap.test.ts. No frontend files in the diff, so the component-hygiene
section does not apply. No confidentiality leaks found: no third-party company names, business arrangements,
private hostnames, or internal links in any prose surface; SAP-#### in the legacyStateRoot JSDoc matches
existing repo convention (packages/tools/**, packages/tools/CHANGELOG.md). Test fixtures are excluded from
the tarball by tsconfig.build.json.

Findings

1. Boot recovery emits an error code that contradicts the state it just persisted — project-bootstrap.ts:1524

In register()'s boot-recovery branch, the persisted classification is chosen at :1493:

: definitelyUnsubmitted && state.retryCount < MAX_RETRIES
  ? { status: "failed", retryable: true,  errorCode: "injection_failed" }
  : { status: "failed", retryable: false, errorCode: "delivery_timeout" };

but the lifecycle event at :1524 is hardcoded to errorCode: "delivery_timeout", retryable: false, and it is
gated on !shouldRetry — which additionally requires session.ready && session.status === "running".

Failure scenario: process is killed mid-attempt with the attempt at phase claimed/not-submitted
(bootstrap.status === "generating" on disk), retryCount < MAX_RETRIES, no queued inputs. On restart the
session is restored but not yet ready (readiness arrives later via onSessionStatus), so shouldRetry is
false. The durable state and session.projectBootstrap say injection_failed / retryable: true, while the
emitted project_bootstrap.failed says delivery_timeout / retryable: false. Any onEvent consumer —
analytics, and whatever surfaces the retry affordance — records a non-retryable delivery timeout for a state
that is retryable and is in fact retried moments later when readiness lands. Not covered by the suite: no test
asserts the lifecycle event for this branch (the injection_failed tests at :1411/:1570 all go through
mode: "created" / explicit retry()).

Fix: derive the emitted errorCode/retryable from state.metadata.bootstrap rather than restating them.

2. Same block emits terminal lifecycle events before persist()project-bootstrap.ts:1514-1533

The skipped and failed emits at :1514 and :1524 run before await this.persist(session.id, state) at
:1549. Everywhere else this file is strict about the opposite order and says so in comments
("Release only after the terminal/non-replayable state is durable", :1055; commitTerminalPreemption's
copy-on-write note at :2571).

Failure scenario: the queue-file write fails during boot registration. Consumers have already been told the
bootstrap was skipped/failed for a transition that never committed, and persist()'s fallback path then
emits a second, different project_bootstrap.failed with persistence_failed for the same attempt. On the
next boot the state is re-read as generating and the whole branch — including the emits — runs again.

Fix: move both emits below the persist() call, as commitBootstrapFailureTransition and
commitPendingBootstrapFailureTransition already do.

3. Changeset advertises a feature no consumer can reach — .changeset/bootstrap-coordinator.md

Stage the complete bootstrap and user-input coordinator with recovery, FIFO delivery, and shutdown handling.
Automatic bootstrap remains inactive until server integration.

This compiles into packages/harness/CHANGELOG.md and ships in the npm tarball permanently. Nothing imports
project-bootstrap.ts (grep: only its own test), it is not re-exported from src/index.ts, and the PR states
activation lands in slot 7 — so an npm consumer reading this line will look for "recovery, FIFO delivery, and
shutdown handling" and find no API, no flag, and no behavior change. "until server integration" is internal
stack sequencing that means nothing outside this repo.

patch is the right level (no reachable surface changes). Rewrite the body for the consumer, e.g. "Internal
groundwork for automatic Agent Map bootstrap. No user-facing change in this release." — and let the release
that activates it carry the feature description.

Nits

  • project-bootstrap.ts:101legacyStateRoot is introduced already @deprecated. If it is only ever set by
    internal callers, drop it from the public options type rather than shipping a deprecated option at birth.

Verdict

Request changes: fix the contradictory lifecycle event (1) and the emit-before-persist ordering (2) — both are
in the same ~40-line boot-recovery block — and rewrite the changeset body (3) before this merges, since that
text cannot be retracted after publish.

@ynadge
ynadge force-pushed the review/agent-map-05-bootstrap-state branch from 37ad9f0 to cc19a8a Compare September 5, 2026 11:57
@ynadge
ynadge force-pushed the review/agent-map-06-bootstrap-coordinator branch from 983b3da to c3023e9 Compare September 5, 2026 11:57
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Follow-up review — PR #825 (delta since 983b3da)

All three findings from the previous round are fixed, with tests. No new findings.

  • (1) contradictory lifecycle event — fixed. The recovery emit now derives errorCode/retryable/reason from the committed state.metadata.bootstrap (project-bootstrap.ts:1530-1551); covered by the new claimed/not-submitted cases asserting injection_failed / retryable: true.
  • (2) emit-before-persist — fixed. Both emits moved below await this.persist(...) (:1529); persist() throws on a failed commit, so nothing is published for an uncommitted transition. The new storage-failure test asserts exactly one persistence_failed event and eventsBeforeCommit is empty in the success test.
  • (3) changeset — fixed. .changeset/bootstrap-coordinator.md now reads as internal groundwork with no user-facing change; patch still correct. No confidentiality issues in the new text.

Not fixed (carried nit, non-blocking)

  • project-bootstrap.ts:102legacyStateRoot still ships @deprecated at birth. The class is not exported from src/index.ts, so this is internal-only; leaving it is fine.

New nits (one line each, non-blocking)

  • project-bootstrap.ts:1528 — the comment "A failed commit publishes its own persistence failure instead" holds for the failed classification but not the skipped one: isTerminal() is true for skipped, so persist()'s catch emits nothing and just throws. Silence is the right behavior (disk still says generating, so the next boot re-runs recovery) — the comment just overstates.
  • The persistence_failed event raised from that path carries no attemptId (project-bootstrap-store.ts:1202, only set when the previous bootstrap was generating), so a consumer correlating terminal events per attempt gets an uncorrelated failure for boot recovery.

Verdict

Approve — the push resolved everything raised in round 1; remaining items are non-blocking nits.

@ynadge
ynadge force-pushed the review/agent-map-05-bootstrap-state branch from cc19a8a to 2b5ee34 Compare September 5, 2026 12:17
@ynadge
ynadge force-pushed the review/agent-map-06-bootstrap-coordinator branch from c3023e9 to d10f605 Compare September 5, 2026 12:17
Base automatically changed from review/agent-map-05-bootstrap-state to main September 6, 2026 22:21
@ynadge
ynadge merged commit d047c3a into main Sep 6, 2026
1 check passed
@ynadge
ynadge deleted the review/agent-map-06-bootstrap-coordinator branch September 6, 2026 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant