fix(lifecycle): fence stale controller effects - #76
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
src/index.ts currently overwrites the authoritative loop prompt with a potentially stale prompt when emitting loop:fire, which can reintroduce outdated wake instructions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens pi-loop lifecycle correctness by fencing stale workflow notifications, making monitor completion/expiry handling atomic, enforcing task-claim authority during detail edits, tightening workflow definition validation, and switching orchestration completion to be provider-owned (avoiding premature consume and duplicate wakes). It aligns tests and documentation with these lifecycle/ownership contracts and explicitly does not claim to resolve Issue #75.
Changes:
- Fence buffered workflow wakes at delivery time by re-reading
LoopStoreand requiring the queued workflow identity (status/revision/state/seq/execution) to still match. - Settle workflow monitor wait completion vs. controller expiry atomically through
LoopStore, and plumb the settlement result through the runtime. - Enforce live claim bearer + revision checks inside
TaskStore.updateDetails, and switch orchestration terminal settlement toprovider_owned(no automatic consume / no aggregate wake duplication).
File summaries
| File | Description |
|---|---|
| test/workflow-reducer.test.ts | Adds regression tests for rejecting unreachable states in newly created workflow definitions. |
| test/task-store.test.ts | Adds coverage that updateDetails enforces claim bearer + revision inside the lock. |
| test/store.test.ts | Adds tests for atomic monitor expiry vs. wait completion and legacy unreachable history revision loading. |
| test/orchestration-tools.test.ts | Updates expectations for Consume: status to provider_owned. |
| test/orchestration-runtime.test.ts | Updates orchestration runtime behavior expectations to avoid consume and suppress aggregate wake under provider-owned completion. |
| test/orchestration-reducer.test.ts | Updates reducer expectations for consumeStatus: provider_owned on settlement. |
| test/native-task-tools.test.ts | Adds tool-level tests for rejecting claimed-task detail edits without the live bearer (including race/expiry cases). |
| test/native-task-rpc.test.ts | Adds RPC-level test rejecting claimed-task instruction edits without the live bearer. |
| test/monitor-ondone-runtime.test.ts | Updates monitor runtime tests for settlement API and adds expiry-boundary coverage. |
| test/loop-tools.test.ts | Adjusts workflow tool test input (definition shape) to match new reachability policy. |
| test/index.test.ts | Adds an integration test ensuring buffered workflow wakes are dropped after authoritative completion; updates orchestration wiring expectations. |
| test/e2e/orchestration-conformance.mjs | Updates live conformance checks for provider_owned completion ownership and messaging flow. |
| src/workflow-revision.ts | Allows unreachable states for persisted history validation and revision graph validation (while retaining revision-specific reachability/rejoin rules for added states). |
| src/workflow-definition.ts | Implements reachability validation from initialState, with an escape hatch for legacy/persisted cases. |
| src/types.ts | Adds WorkflowMonitorSettlement, extends expiry source, and adds orchestration provider_owned consume status. |
| src/task-store.ts | Extends updateDetails to accept claimId/expectedRevision/now and enforce claim ownership + CAS under lock. |
| src/store.ts | Adds settleWorkflowMonitorWait that atomically settles expiry before clearing a wait; plumbs new settlement type. |
| src/runtime/task-mutations.ts | Enforces claim bearer for detail edits and uses revision-checked updateDetails to close TOCTOU races. |
| src/runtime/subagent-orchestration-runtime.ts | Suppresses duplicate aggregate wakes when provider owns terminal completion; removes automatic consume on normal terminal settlement. |
| src/runtime/notification-runtime.ts | Adds delivery-time workflow wake relevance check using getLoop to drop stale buffered wakes. |
| src/runtime/monitor-ondone-runtime.ts | Switches to settlement API so expiry is settled before clearing/rearming/waking. |
| src/orchestration-reducer.ts | Adds provider_owned to allowed consume statuses and sets it on terminal settlement events. |
| src/notification-reducer.ts | Adds controllerStatus to queued notification payload to support delivery-time fencing. |
| src/index.ts | Wires getLoop into notifications, emits controller status with loop:fire, and routes monitor completion through settlement + expiry emission. |
| docs/USAGE_GUIDE.md | Documents provider-owned orchestration completion, monitor expiry boundary behavior, and claimed-task detail-edit requirements. |
| docs/REFERENCE.md | Updates lifecycle contracts for workflow wake fencing, orchestration completion ownership, workflow reachability rule, and task detail edit authority. |
| docs/architecture/lifecycle-remediation-plan.md | Adds remediation plan and RED/GREEN acceptance criteria and validation command/evidence. |
| docs/architecture/lifecycle-regression-trace.md | Documents regression origins and explicitly separates Issue #75 cause from this PR’s scope. |
| docs/architecture/lifecycle-ownership-audit.md | Adds/updates a detailed lifecycle ownership audit and recommended authority boundaries. |
Review details
- Files reviewed: 29/29 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Summary
Regression trace
The five gaps are traced in
docs/architecture/lifecycle-regression-trace.md.Issue #75 was investigated during this work but has a separate deterministic cause: free-text
/loop <goal>silently usesmaxFires: 20, then deletes the controller before delivering a final wake that still instructsLoopUpdate. This PR does not claim to close #75; a separate focused fix will follow.Validation
npm run lint(four established optional-chain warnings only)npm run typechecknpm test— 857 testsnpm run buildnpm run test:package— 112 filesnpm audit --audit-level=moderategit diff --check@tintinweb/pi-subagentsprotocol-v2 orchestration harness with provider-owned completion