fix: AlgoPhase fails closed on ambiguous session resolution#1468
Closed
jbmml wants to merge 1 commit into
Closed
Conversation
resolveSlug()'s no-flag fallback picked the most-recent live algorithm-mode row in work.json, printed a WARN, and wrote the phase to it anyway. With two Algorithm sessions alive, a bare AlgoPhase <phase> from one session could flip the OTHER session's phase — cross-session registry corruption that propagates to the Pulse dashboard. Resolution is now deterministic and fail-closed: 1. --slug: trusted verbatim, including phase:complete rows (resume-after-complete is a sanctioned flow). Object.hasOwn instead of the in operator so prototype keys like "constructor" can't false-match; __pulse_strip is unaddressable from every path. When --uuid is also given, the pair is cross-checked. 2. --uuid: explicit flags outrank ambient env; no live match is an error, never a fall-through. 3. CLAUDE_SESSION_ID env: set-but-unmatched is a hard error. Claude Code does not export this variable to tool subprocesses today; the path is forward-looking defense-in-depth, local-trust only. 4. Exactly ONE live algorithm-mode row updated within the last 60 minutes: convenience path preserved (stderr NOTE). Older non-complete rows are crash leftovers (the registry stale sweep only prunes after ~7 days) and are never guessable. Zero or multiple candidates: ERR listing them, exit 1, no write. Also updates the ALGORITHM v6.24.0 phase-tracking doc to pass --slug at every transition, since bare calls now fail closed under concurrency. Adds a self-contained bun test suite (subprocess-driven against a temp LIFEOS_DIR registry) covering all resolution branches.
Owner
|
Thank you @jbmml — a legit fail-open bug. Heads up though: as of v8.2.0, AlgoPhase is no longer a required station (the per-transition phase ceremony was removed; phase tracking is now ISA frontmatter + ISASync, and nothing invokes AlgoPhase.ts in the loop). Since the tool it hardens is being sunset, we're not pulling this into v7.0.0. Genuinely appreciate the fix and the test. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 2 of 2 for #1466 (builds on the same analysis as #1467, which fixes the sibling bug in
Inference.ts --auto-state). This half is a deliberate policy change — warn-and-guess becomes refuse-and-explain — so the behavior deltas are listed explicitly below.What
resolveSlug()'s no-flag fallback picked the most-recent live algorithm-mode row inwork.json, printed a WARN, and wrote the phase to it anyway. With two Algorithm sessions alive, a bareAlgoPhase <phase>from one session could flip the OTHER session's phase — cross-session registry corruption that propagates to the Pulse dashboard and every other work.json reader. The hazard was already flagged in the code comment at the fallback site; this PR makes the code act on it.How
Deterministic priority chain, fail closed on ambiguity:
--slug— trusted verbatim, includingphase: completerows (resume-after-complete is a sanctioned flow). Lookup usesObject.hasOwninstead of theinoperator so prototype keys like--slug constructorcan't false-match and trigger a registry write; the__pulse_stripmetadata row is unaddressable from every path. When--uuidis also given, the pair is cross-checked and a mismatch errors.--uuid— explicit flags outrank ambient env (an explicit flag must never be silently overridden by environment state); no live match is an error, never a fall-through.CLAUDE_SESSION_IDenv — set-but-unmatched is a hard error rather than a silent fall-through to a guess.ERR:listing them, exit 1, no write.The 60-minute freshness horizon exists because "not complete" is a weak liveness signal: crashed sessions leave non-complete rows behind and the registry stale sweep only prunes them after ~7 days — without a horizon, one week-old ghost row would hard-fail every bare call. A session actively running the Algorithm touches its row on every phase emit and tool-activity hook, so an hour of silence means the row isn't driving anything. Stale rows remain addressable explicitly via
--slug/--uuid.Output contract otherwise unchanged:
OK: <slug> <prev>→<phase>on stdout,ERR: <reason>on stderr, exit 0/1.Behavior changes (please review deliberately)
AlgoPhase <phase>with multiple live sessions gets an error listing the candidates instead of a write to the most-recent row. The ALGORITHM v6.24.0 phase-tracking doc is updated in this PR to instructAlgoPhase.ts <phase> --slug <slug>at every transition, which sidesteps the ambiguity entirely.--uuidwith no live match now exits 1 (previously fell through to the guess path).--slugnow exits 1 — complete rows are only addressable explicitly (--slug), matching the resume-after-complete flow.CLAUDE_SESSION_IDhonesty: Claude Code does not export this variable to tool subprocesses today, so the env path is forward-looking defense-in-depth (and local-trust only — not an auth boundary). The operative contract today is: explicit--slug/--uuid, or exactly one fresh live row.Inference.ts --auto-stateso an undeterminable caller yields an explicit no-state marker (with a 5-minute completion window preserving the advisor-after-complete doctrine) instead of a possibly-wrong ISA — see fix: deterministic session resolution for Inference --auto-state #1467 for that disclosure.Verification
Self-contained bun suite added alongside the tool (
AlgoPhase.resolution.test.ts, subprocess-driven against a tempLIFEOS_DIRregistry —hooks/lib/paths.tshonors the override, the real registry is never touched): ambiguous bare call fails closed with candidates listed and a byte-identical registry; single fresh live row still works bare; stale rows excluded from guessing but stale-only registries error; env match/env-unmatched;--uuidprecedence over env and no-match error;--slugon complete rows;--slug/--uuidmismatch;constructorprototype guard;__pulse_stripunaddressable. 12/12 pass;tsc --noEmit --strictclean.