E3: orchestrate revision-bound planning suggestions through Store

[mchwang]

Coordinates revision- and snapshot-bound planning suggestions through Store. Captures identity/revision/snapshot before invocation; validates all cards before publication; retains the invocation slot until provider termination after timeout, cancellation, or shutdown. Store remains the only persistence and Apply writer.

The #34 prerequisite landed in #43. E3 now begins requests with the captured snapshot binding and uses pending-only settleSuggestion for automatic failure, timeout, cancellation, and stale cleanup. If another connection completes the attempt first, cleanup loses without changing the ready result. Durable terminal reasons survive restart and oversized provider diagnostics are bounded for Store persistence.

Depends on merged E2 #32; ownership #29. Lifecycle contract: docs/implementation/planning-suggestions.md. D5 supplies real provider isolation and subprocess settlement.

Validation on pushed head 30743b75052249667ccef254af946367d414348f:

• npm test — 368 passed
• npm run typecheck — passed
• npm run test:browser — 43 passed
• Focused coordinator/store suite — 48 passed after the latest review fix
• git diff --check — passed
• GitHub CI — passed

Review readiness:

• Current head: 30743b75052249667ccef254af946367d414348f
• Mergeability: clean and mergeable
• Unresolved review threads: none
• Deferred follow-up: browser/HTTP production wiring remains with F/G; real provider process isolation remains with D5

Review rounds 1-3 surfaced summary concerns about cancellation and stale publication classification. Four controlled regressions cover external cancellation or revision change before publication and before provider rejection; the coordinator now derives cancelled/stale outcomes from durable state while retaining the original diagnostic.

Review round 4 found that unconditional cleanup could cancel a result completed by another Store connection. After #43 added atomic request bindings and pending-only settlement, E3 adopted that contract. A two-connection regression completes the request inside cleanup, proves cleanup loses, retains the exact ready reply, and applies it successfully. The thread is resolved. Self-review additionally found and fixed oversized provider reasons failing Store cleanup; the returned diagnostic remains intact while the durable copy is bounded.

Review round 5 found a settlement-loss classification race. Both stop-time and final cleanup now reread durable state when pending settlement loses, map external cancellation/invalidation to the matching outcome, and retain the original diagnostic. Two controlled-connection regressions cover both terminal races. The finding was fixed and its thread resolved.

Review round 6 found that separate plan/snapshot reads could admit caller data against a newer snapshot. Suggestion input now includes the caller snapshot ID; admission compares it before atomic request allocation. A controlled two-connection test advances history between the reads and proves no request or provider call is admitted. The lifecycle table now distinguishes immediate durable settlement from later provider termination. The finding was fixed and its thread resolved.

Review round 7 returned no new findings and marked the snapshot-admission issue resolved.

Review-lesson audit: every finding is covered by existing AGENTS.md rules under Async jobs and polling, Required race regressions, and Review readiness. No new rule is needed. Nothing was declined.

[Copilot]

Copilot review overview

🔵 Needs a closer look

A moderate race-classification issue can report the wrong terminal state.

Review effort: Lite
Findings: None

What changed in this PR

Adds a single-owner coordinator for revision- and snapshot-bound planning suggestions.

Changes:

• Coordinates validation, cancellation, timeout, shutdown, and stale-result handling.
• Preserves Store as the persistence and Apply authority.
• Adds lifecycle documentation and SQLite-backed race-condition tests.

File	Summary
test/​planning-suggestions.test.ts	Tests coordinator lifecycle, retries, stale results, cancellation, and race conditions.
docs/​implementation/​planning-suggestions.md	Documents lifecycle, ownership, and state-holder contracts.
core/​planning-suggestions.ts	Implements suggestion coordination and guarded publication. Moderate finding: publication races may be classified as provider failures instead of cancellation or staleness.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🔵 Needs a closer look

A moderate issue remains where external cancellation or invalidation may be reported as provider failure.

Review effort: Lite
Findings: None

[mchwang]

Reproduced the summary-only publication race with real SQLite and a controlled mutation at completeSuggestions: cancellation and import invalidation were both classified as failed. The tests assert durable cancelled/invalidated state at the publication boundary before invoking the Store CAS. Both failed before 8a82bf5 and pass after it, with null durable replies and the correct plan revision. The fix rechecks durable state on CAS refusal and returns cancelled/stale while preserving original provider and timeout failures. Current targeted validation: 59 tests and typecheck pass. Production cross-process snapshot atomicity remains separately tracked in #34.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Cancellation-related provider errors need durable-state reclassification.

Review effort: Lite
Findings: None

[mchwang]

Final follow-up at a1a72b2 also reproduces the external-action-then-provider-rejection case in both cancelled and stale variants. The two added tests failed before the fix; all 61 targeted tests and typecheck pass after push. Classification now consults durable state for both rejected provider results and CAS refusals, retaining the original diagnostic. Final full diff self-review found no further issues.

[Copilot]

Copilot review overview

🟡 Changes recommended

A completion race can cancel a valid ready result; cleanup needs an atomic state/ownership check.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Cleanup cancels externally completed suggestions · New

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved settlement races, blocking cleanup behavior, and loss of durable terminal reasons remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Handle lost settlement and reconcile the terminal outcome · New

Resolved since last review (1)

• Cleanup cancels externally completed suggestions

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings affect snapshot consistency and provider invocation safety.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Capture plan and snapshot consistently before admitting request · New

Resolved since last review (1)

• Handle lost settlement and reconcile the terminal outcome

Previously missed (1)

In code that hasn't changed since last review

Distinguish request settlement from active provider invocation

docs/​implementation/​planning-suggestions.md:13

This lifecycle row says terminal cancellation/failure occurs only after provider settlement, but stop() calls settleSuggestion immediately on timeout, user cancellation, and shutdown while the provider is still running (as the regression at test/planning-suggestions.test.ts:74-75 asserts). Distinguish the durable request transition from the still-active invocation here so this contract does not mislead the F/D integration.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Lifecycle and concurrent Store/provider settlement paths warrant final human review.

Review effort: Lite
Findings: None

Resolved since last review (1)

• Capture plan and snapshot consistently before admitting request