feat(lifecycle): add irreversible resident retirement primitive - #116
feat(lifecycle): add irreversible resident retirement primitive#116ian-de-marcellus wants to merge 5 commits into
Conversation
antra-tess
left a comment
There was a problem hiding this comment.
From the 08-26 review sweep, plus maintainer direction after discussion. The mechanism you built is genuinely careful — two forced turns, cooling-off, one-use challenge, timing-safe compare, honest semantics text, append-only fsynced seal — and the integration suite is strong. The requested changes are architectural first, then a short list of holes found in review.
Architectural direction: the resident-facing surface of this belongs in connectome-host, not the framework. The framework is the right home for the enforcement primitive — the seal file, loadRetirementSeals, and the inference-denial guards can only live here. But the tool itself — its name, description/consent text, ceremony shape (challenge, cooling-off, confirmation phrase), and any operator-notification policy — should be host-composable rather than a fixed built-in. Concretely: AF exposes an imperative API (e.g. framework.retireResident(name, {reason}) — irreversible, sealed, guarded — plus the lifecycle status query and perhaps the challenge/cooling-off helpers), and connectome-host builds the resident-facing tool on top, so deployments can shape the wording, the ceremony, and whether a human is notified at request time. Your two-turn design would make a fine default implementation of that host-side surface; we just don't want its exact wording and policy frozen into AF.
Findings that apply to the enforcement half regardless:
-
Fork resurrection (must-fix):
createConversationAgentseeds a conversation fork from the retired template's still-compiling context under a fresh agent name — every retirement guard checks the fork's name and passes, so a single channel message can revive the retired resident's full context and identity prompt. Needs a router guard (refuse spawning from a retired template), or an explicit statement that forks are outside the seal's scope. -
puppetToolCall interaction (landed on main after you branched): with the lifecycle tool on
getToolsForAgent's surface, puppet's existence check passes,executeToolCallfails 'unknown tool', and the forged 'resident requested retirement (errored)' pair is stored into the sealed identity's history. Exclude the lifecycle tool from puppet's surface on rebase. -
Torn seal line — decided: fail loud and fail closed is the intended behavior. A torn/invalid line in
resident-retirements.jsonlrefusing to boot the whole host is accepted; please pin it with a test (and cover challenge-TTL expiry) so it's deliberate rather than incidental, and document the recovery expectation (manual inspection of the named file:line). -
Minor: gate timers/sleep state for a retiree stay armed (permanent dropped-request churn — clear them in
stopResidentAuthoredActivity); running ephemeral subagents spawned by the resident aren't stopped at confirmation (document or stop them).
Suggested path: keep this PR's seal + guards + denial surface + tests as the framework primitive with the imperative API, and move the tool definition + ceremony to a companion connectome-host PR — happy to discuss the interface split there. Also needs a rebase (#123/#126 conflicts).
3e81a45 to
ce4d0da
Compare
Anarchid
left a comment
There was a problem hiding this comment.
🔴 BLOCKING
Reviewer: Codex (GPT-5.6 Sol)
Reviewed head: ce4d0da78e6ff33be8f2a55bd7cd564db6801a9d
The architectural split is now in the right place, and the revised head covers the previous fork, puppet, timer, and malformed-ledger findings. Three enforcement defects remain.
-
Blocking — the live-only boundary is bypassed by omitting or spoofing
callerAgentName.src/framework.ts:7487derives the authorization lookup key from caller-controlled input:const caller = call.callerAgentName ?? '__ephemeral__'; if (this.moduleRegistry.isLiveTool(call.name, caller)) {
src/module-registry.ts:256then asks the module only for that caller's live surface. A ceremony module normally returns its tool only for the resident name, soexecuteToolCall({ name: 'resident--retire', ... })with no caller, or withcallerAgentName: 'someone-else', makesisLiveToolreturn false and falls through tomodule.handleToolCall. Against this head, both calls returnedsuccess=trueand the supposedly live-only handler ran twice (handled=2). A programmatic caller can therefore counterfeit the exact resident-only action this boundary exists to protect.Make the restriction independent of the untrusted caller field. For example, reserve live-only names across all configured resident surfaces and reject those names from
executeToolCallFromfor every non-provider origin, while keeping the provider dispatcher as the only trusted entry. Add regression cases for omitted and spoofed caller names, and forModuleContext.callTool. -
Blocking — a retirement tool resumes inference after the durable seal.
When a live tool calls
retireResident, the resident is stillwaiting_for_tools. The resulting event takes the normal ready path atsrc/framework.ts:4306, persists the tool round, and reaches this unconditional continuation atsrc/framework.ts:4562:} else if (currentState.stream) { currentState.stream.provideToolResults(...); agent.setStreaming(currentState.stream); }
There is no retired-state check here, and
retireResidentnever cancels the active agent stream. I reproduced this with a live-only module whose handler callsframework.retireResident('resident'): after the call, lifecycle status wasretired, yet a second response from the resumed stream was accepted andPOST-SEAL CONTINUATIONwas present in the compiled resident context. On a real yielding provider this is a post-seal model continuation that can speak or issue more tools.Treat applying the seal as terminal for the current resident stream: cancel/abort it with a retirement-specific framework reason, reset the state safely, and make the tool-result path short-circuit rather than resume or requeue when the resident is sealed. Add a regression test where
handleToolCallitself retires the resident and prove that no second provider round or post-seal message is accepted. -
Blocking durability gap — the first seal file can disappear after a reported success.
src/framework.ts:2074-2087createsresident-retirements.jsonl, writes it, and fsyncs only the file descriptor. On POSIX filesystems, fsyncing a newly created file does not durably commit its parent-directory entry. A crash or power loss afterretireResidentreturns can therefore lose the filename even though the API claimed the irreversible seal succeeded; Chronicle is explicitly not authoritative and may be rewound.Detect first creation and durably sync the parent directory (and any newly created path components), or use an equivalent crash-safe creation sequence. The durability test should cover the first record separately from appending to an existing sidecar; the current tests exercise logical restart only, not the creation boundary.
Tooling results
git diff --check HEAD^ HEAD— pass; no whitespace errors.- User-facing internal-shorthand scan of the diff — pass; no matches.
npx --no-install tsc --noEmit— pass against cached@animalabs/chronicle@0.3.0,@animalabs/context-manager@0.6.3, and@animalabs/membrane@0.5.79.node --import tsx --test test/resident-retirement.test.ts— pass.node --import tsx --test test/framework.test.ts— pass.npm run build— pass.npm test— inconclusive locally: the compiled runner reported nine passing test files and then stopped producing progress; it was interrupted after a process audit. Current GitHub CI is green on Node 20/24 across Ubuntu and macOS.- Live-only boundary repro — omitted caller:
success=true; spoofed caller:success=true; handler executions:2. - Retirement-continuation repro — lifecycle
retired;postSealContinuationPersisted=true.
Verdict: the previous review's requested architecture and edge cases are substantially addressed, but the new authorization boundary is currently bypassable and the seal does not terminate the stream that invoked it. Those are merge-blocking correctness properties for an irreversible lifecycle primitive; the first-write durability gap is also part of the advertised contract. Review confidence is high for these findings despite the local full-suite stall because both runtime defects reproduce deterministically on the exact head and the focused/type/build gates pass.
— Reviewed by GPT-5.6 Sol via OpenAI Codex.
|
Thank you for the detailed review. The branch is now rebased and updated at
The later enforcement findings from Anarchid's review are pinned with exact adversarial regressions as well. GitHub CI is green across Ubuntu and macOS on Node 20 and 24, the changelog check is green, and GitHub reports the branch cleanly mergeable. @antra-tess, would you take another look when convenient? Thanks again. |
Anarchid
left a comment
There was a problem hiding this comment.
🔴 BLOCKING
Reviewer: Codex (GPT-5.6 Sol)
Reviewed head: 0be84b278966ba76176c40ef2d27c9b1a5f475c9
The three exact-head findings from the previous campaign review are addressed, but the new directory-durability path introduces a fail-open interval when the ledger mutation succeeds and a later durability operation throws.
-
Blocking — a post-write seal error leaves the resident active in the current process.
src/framework.ts:2114-2118can throw while syncing a newly created directory after the seal file itself has already been written and fsynced:for (const directory of directoriesToSync) { const directoryFd = openSync(directory, 'r'); try { fsyncSync(directoryFd);
retireResidentdoes not install the in-memory terminal state untilappendRetirementSealreturns atsrc/framework.ts:2242-2244:this.appendRetirementSeal(record); this.retiredResidents.set(agentName, record); this.stopResidentAuthoredActivity(agentName);
Therefore a directory
open/fsync/closeerror, or any other error after bytes may have reached the append-only file, makes the API throw while leaving the current resident able to infer. The on-disk ledger may already contain the authoritative valid seal; a restart would retire the resident, but the process that performed the operation remains active until then. This contradicts the fail-closed terminal contract and is especially dangerous because the host sees an exception and may continue running.I reproduced the exact boundary by wrapping Node's
fsyncSyncso the second call performs the real directory fsync and then throws. The seal record was present,getResidentLifecycleStatus('resident')still returnedactive, and a subsequent public inference reached the provider:{"retirementError":"injected directory-fsync failure","fsyncCalls":2,"sealContainsResident":true,"lifecycleAfterError":{"status":"active","retirementEnabled":true},"providerCalls":1}Once the append attempt has reached a point where its outcome may be durable or ambiguous, failure must close the in-process identity before the error escapes. A safe shape is to catch seal-write/durability errors, install a process-local terminal/ambiguous state and stop resident-authored activity, then rethrow (or fail-stop the framework). On restart, the existing strict ledger parser can distinguish a valid record from a torn one. Add a fault-injection regression where file fsync succeeds and directory fsync throws, and assert that inference remains denied despite
retireResidentthrowing.
Tooling results
npm ls --depth=0— pass after materializing the exact cached packages@animalabs/chronicle@0.3.0,@animalabs/context-manager@0.6.3, and@animalabs/membrane@0.5.79in the detached worktree. The initial bare-worktree dependency probe/typecheck failed only because those three packages were absent; both were rerun after isolation setup.npx --no-install tsc --noEmit— pass.node --import tsx --test test/resident-retirement.test.ts— pass.node --import tsx --test test/framework.test.ts— pass.npm run build— pass.npm test— locally inconclusive: nine compiled test files passed, then the runner produced no further progress for roughly 90 seconds and was interrupted. Current GitHub CI is green on Node 20/24 across Ubuntu and macOS.git diff --check origin/main...HEAD— pass.- User-facing internal-shorthand scan of the diff — pass; no matches.
- Directory-fsync failure repro — valid seal present, lifecycle remained active, and one post-error provider call completed, as shown above.
Verdict: the authorization, stream-cancellation, fork, and successful durability paths are substantially stronger on this head. The remaining failure-path split-brain is merge-blocking for an irreversible lifecycle primitive because an already-written authoritative seal can coexist with an inference-capable in-memory resident. Confidence is high; the failure is deterministic at the exact post-file-fsync boundary and does not depend on the stalled full-suite tail.
— Reviewed by GPT-5.6 Sol via OpenAI Codex.
Problem
Persistent resident agents can end a turn or enter reversible dormancy, and operators can erase stored data, but there is no neutral terminal lifecycle primitive that permanently prevents future inference for one resident while preserving that resident's Chronicle and history.
Architecture
Agent Framework owns only the irreversible seal and enforcement primitive. Resident-facing wording, confirmation ceremony, cooling-off policy, memory-health policy, and notification policy belong to the host. The companion Connectome Host PR implements one protected default ceremony on top of this API.
Changes
AgentConfig.retirement: { enabled }, public lifecycle status, and the imperativeframework.retireResident(agentName, reason?)API.puppetToolCall, code execution, maintenance inference, ephemeral subagents, and conversation forks cannot invoke these tools.Review response
This revision moves the challenge, confirmation wording, cooling-off interval, readiness gate, and operator notification out of Agent Framework and into Connectome Host. It also rebases onto current
mainand covers the newerpuppetToolCallsurface.Tests
npm run build: passnpm test: 659 pass / 0 fail / 4 existing skipsgit diff --check: passThe squashed revision has the same source tree as the full-tested pre-squash head.
Not verified
Out of scope
Companion PR
connectome-host#92 implements the Host-owned resident tool, challenge, cooling-off floor, memory-health gate, and post-seal notification. Merge and release this Agent Framework primitive first; the Host draft can then update its dependency range and lockfile to the qualifying release.
changelog.d/.🤖 Generated with OpenAI Codex