Skip to content

feat(harness): activate ordinary bootstrap [Agent Map 07/15] - #826

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

feat(harness): activate ordinary bootstrap [Agent Map 07/15]#826
ynadge merged 1 commit into
mainfrom
review/agent-map-07-bootstrap-activation

Conversation

@ynadge

@ynadge ynadge commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Primary change type

  • Feature

Problem and motivation

Studio must connect the tested bootstrap coordinator to project creation and remove the retired planner-specific lifecycle once its replacement is available.

Summary and scope

Wire first-session creation, outbox recovery, lifecycle callbacks, input routes, event ingestion, and shutdown; retire the obsolete planner greeting/session paths and validate the desktop startup boundary.

Return committed settings while root initialization continues with caught failures. Fence shutdown admission and bound ordered draining to five seconds so an unresolved initialization cannot hold the listener indefinitely.

How this increment fits

Activate the complete replacement and retire the old planner lifecycle in the same increment. The diff includes deletion of the superseded implementation and its tests.

Stack and review boundary

  • Part 07 of 15 in the Agent Map review stack; review this increment against its predecessor.
  • Base: review/agent-map-06-bootstrap-coordinator.
  • Current head: 21684912c20feaf1d86a84e5ab8199dc205f32cc; 10,419 changed lines across 28 files, counting additions and deletions including tests.
  • Repackages the corresponding final behavior from #804, #811. 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-3152. This packaging follows the maintainer-approved 15-PR split.

Validation

Root checks ran against bcb30f8990edeb80cbbba581f2f71ddb58442500. 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 21684912c20feaf1d86a84e5ab8199dc205f32cc.

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: Project creation, root binding, startup recovery, ordinary input routes, settings updates with delayed initialization, lifecycle ingestion, and bounded shutdown.

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: Breaking: planner metadata, planner request/response types, planner-specific HTTP routes, and planner analytics names are retired. Use ordinary session routes, neutral identity, and projectBootstrap lifecycle metadata. Valid persisted startup state migrates.
  • Changeset: Included: .changeset/project-bootstrap-activation.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 #826 feat(harness): activate ordinary bootstrap [Agent Map 07/15]

No confidentiality findings. No frontend component code in the diff.


1. Changeset is patch; this PR removes public API and changes wire contracts

.changeset/project-bootstrap-activation.md declares "@sapiom/harness": patch with no
Breaking section. @sapiom/harness is at 0.14.0 and src/index.ts does
export * from "./shared/types.js", so the following all land in a consumer's .d.ts /
runtime as a patch bump:

  • HarnessSession.planning deleted (shared/types.ts:217) — any consumer branching on it
    stops compiling.
  • AnalyticsEventType loses planner_session.* and planner_greeting.*
    (shared/types.ts:832). A consumer with an exhaustive switch over that union breaks.
  • PlannerSessionMetadata, PlannerQueuedInput, PlannerSessionRequest/Response,
    PlannerMessageRequest, PlannerGreetingState, PlannerLifecycleEvent removed from
    shared/agent-map.ts — reachable today through HarnessSession's import(...) types.
  • SessionManager.setPlanningMetadata() removed; TrustedSession{Create,Resume}Options.planning
    removed. startServer returns a live sessionManager, so this is embedder-visible.
  • HarnessStatePaths.plannerSessionsprojectBootstrap (core/paths.ts:31).
  • REST routes POST /projects/:id/planner-sessions, .../messages, .../greeting/retry
    deleted outright (server/agent-map.ts), with no deprecation window.
  • POST /projects and the root-binding routes now answer 202 instead of 201/200 when
    the lifecycle hook throws (server/agent-map.ts:141,200,251). A client asserting
    status === 201 reads a successful create as a failure.

Fix: bump to minor and write a Breaking block in the changeset body naming each
removed export/route and the ordinary /api/sessions replacement. The
onPlanningUserChangedonProjectUserChanged alias in auth-routes.ts:122 is the right
pattern — nothing else here got it.

2. PATCH /api/settings now blocks on two network round-trips and a PTY spawn

rest.ts:436 awaits options.onRecentDirAdded(root) per newly added root before
res.json(committed). That is initializeOpenedProjectensureProjectFirstSession
sessionManager.create()buildLaunchOpts (server/index.ts:592,600), which awaits
apiKeyProvider.refresh() and loadSystemPrompt() — the latter being the
GET https://api.sapiom.ai/v1/harness/system-prompt fetch.

Failure: a user on a slow or offline network clicks "Open project"; the settings PATCH that
previously returned in milliseconds now hangs for the fetch timeout, multiplied by the
number of new roots in the request, before the UI gets its updated settings back. The
try/catch around the call swallows errors but not latency.

Fix: resolve the response from committed first and run project initialization detached,
or return once the bootstrap intent is durable and let ensureProjectFirstSession finish
out of band — it is already idempotent and re-driven from reconciliation and boot recovery.

3. close() gained four unbounded awaits ahead of the bounded HTTP close

The deleted comment block in server/index.ts documented exactly why every shutdown step
was bounded: an unbounded wait means Electron's before-quit never reaches app.quit(),
the process lingers holding the single-instance lock, and the next launch hangs on
"Starting Sapiom…". The new closeServer reintroduces unbounded waits between the bounded
kill race and the bounded httpServer.close:

  • await bootstrapClosing (index.ts:4249)
  • await settle(() => sessionManager.flush())flush() now spins
    while (this.projectCreateQueues.size > 0) (session-manager.ts:2359)
  • await recordBackfill plus while (pendingRecordArchives.size > 0) (index.ts:4256) —
    a full per-boot index build that was previously fire-and-forget
  • batcher.close() changed from void batcher.close() to await settle(() => batcher.close())
    (index.ts:4262), which is analytics.shutdown() flushing telemetry over the network

Fix: wrap this whole group in the same Promise.race([..., timeout]) pattern already used
for SHUTDOWN_KILL_TIMEOUT_MS, so a stuck collector flush or a never-settling create queue
cannot strand the listener.

4. README drops a published privacy guarantee that the code still honours

The removed README paragraph stated that hook projections reduce session-start source to a
fixed enum, model identity to a presence boolean, and usage to clamped allowlisted
counters. ProjectBootstrapCoordinator.redactForTelemetry still does this
(core/project-bootstrap.ts:2993), but the replacement text only says "Prompts, assistant
text, local paths, and provider error text remain local." Restore the projection sentence —
telemetry commitments are the part of the README users actually rely on.


Verdict: Request changes. The changeset level (1) must be fixed before publish; the
settings-PATCH blocking call (2) and the unbounded shutdown waits (3) are behavioural
regressions in paths this repo has previously had to fix. Test coverage for the new
lifecycle hooks and 202 paths is genuinely good.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Follow-up review — PR #826

Delta reviewed: 8b08219bcb30f8 (16 commits, 26 files).

Earlier findings — all four fixed

  1. Changeset is now minor with a Breaking changes block naming the removed exports, the
    three deleted planner routes, the analytics rename, and the new 202.
  2. rest.ts:437 detaches per-root initialization; PATCH /api/settings returns committed
    without waiting on a fetch or PTY spawn. Concurrent roots are safe —
    ensureProjectFirstSession dedupes by in-flight promise (server/index.ts:3262).
  3. closeServer races the drain group against SHUTDOWN_DRAIN_TIMEOUT_MS (5s, unref'd).
  4. README projection sentence restored (source enum, model presence boolean, clamped counters).

New finding

SESSION_INPUT_ISOLATION_REQUIRED is sold as an embedder contract, but the type isn't exported

.changeset/ordinary-session-input.md tells embedders that SessionManager.write() "can throw
an isolation error with code SESSION_INPUT_ISOLATION_REQUIRED" and that "callers forwarding
terminal bytes should handle this failure". SessionInputIsolationError is declared at
core/session-manager.ts:627 and is not re-exported from src/index.ts, and package.json
exports only exposes "." — so dist/core/session-manager.js is unreachable from a consumer.
An embedder holding the sessionManager returned by startServer can only do an untyped
(error as { code?: string }).code === "SESSION_INPUT_ISOLATION_REQUIRED"; instanceof is
impossible and TypeScript sees nothing. Same for SessionBackgroundInputPreemptedError, which
submitInput() now throws on the new terminal-epoch preemption path
(core/session-manager.ts:1567). Fix: export both classes (or a code constant) from
src/index.ts in this changeset, or drop the "callers should handle this" wording.

Earlier round got one item wrong

HarnessStatePaths.plannerSessionsprojectBootstrap (core/paths.ts:31) was listed as a
break landing in a consumer's .d.ts. HarnessStatePaths is not exported from src/index.ts,
so the rename is internal. The rest of that finding's bullets stand and are now documented.


Verdict: Approve once the error export (or the changeset wording) is settled. Nothing else
in the delta blocks; the storage finally-cleanup, outbox filename filter, post-persist recovery
emission, and workspaceKey dedup all read correctly and carry tests.

@ynadge
ynadge force-pushed the review/agent-map-06-bootstrap-coordinator branch from c3023e9 to d10f605 Compare September 5, 2026 12:17
@ynadge
ynadge force-pushed the review/agent-map-07-bootstrap-activation branch from bcb30f8 to 2168491 Compare September 5, 2026 12:17
Base automatically changed from review/agent-map-06-bootstrap-coordinator to main September 6, 2026 22:21
@ynadge
ynadge merged commit a35426f into main Sep 6, 2026
2 checks passed
@ynadge
ynadge deleted the review/agent-map-07-bootstrap-activation 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