--- phase: 25.3-project-context-envelope-persistence-encoding-defects reviewed: 2026-05-22T00:00:00Z depth: standard files_reviewed: 10 files_reviewed_list: - plugin/spt/skills/commune/commune.md - plugin/spt/skills/signoff/SKILL.md - src/common/owlery.rs - src/live/context.rs - src/live/signoff.rs - src/live/wrapper/claude.rs - src/live/wrapper/mod.rs - src/owl/echo_commune.rs - src/owl/poll.rs - tests/native_owl.rs findings: critical: 3 warning: 8 info: 5 total: 16 status: issues_found --- # Phase 25.3: Code Review Report **Reviewed:** 2026-05-22T00:00:00Z **Depth:** standard **Files Reviewed:** 10 **Status:** issues_found ## Summary Phase 25.3 wires a new file_drop control envelope, a precedence-aware two-slice router, and a shared `info.json.cwd`-based project resolver. The implementation is dense and well-tested at the unit level, but the dispatch-arm and inbound- routing paths introduce several real-world hazards. Three findings are classification-Critical: (1) the inbound `file_drop` path attribute is not bound to the receiving wrapper's identity, so any sender that can deliver to the psyche perch can drive the wrapper into reading and feeding arbitrary `.../.claude/*-{commune,signoff}.md` files into a `claude -p --resume` subprocess; (2) `process_file_drop`'s signoff branch ignores the file-read result and proceeds to run `final_session` regardless of whether `route_inbound_commune_body` actually persisted the inbound slices, breaking the documented contract that the inbound payload reaches disk synchronously BEFORE the LLM handoff; (3) the `path.contains("/.claude/")` validator is satisfied by any path component (`/tmp/x/.claude/foo-commune.md`, `/etc/.claude/foo-signoff.md`) — there is no anchor or ownership check. Three commune-bodies-from-Self bugs are Warning-class: (a) the precedence suppression window suppresses LLM writes inside the window, but the route_two_ slice path under `--source=llm` writes through `WriteOutcome::Suppressed` and the caller maps that to `SliceWriteState::SkippedNoSlice` which loses the distinction between "no slice in payload" and "slice was suppressed by precedence" — observability damage. (b) `read_route_guard_window_secs` reads config from a path relative to process cwd, and the code already acknowledges deployed wrappers always fall back to default 60 because cwd is `psyche_dir` — the configured value is therefore effectively a dead surface in production. (c) `init_session` removes `last_commune_epoch_path` BEFORE the new session UUID is captured, but if `init_session` fails (the function exits 1) the cursor is gone with no recovery. Skill docs are accurate but the project-resolution wording in commune.md slightly misrepresents the resolver chain: SPT actually consults info.json.cwd first, not Self's perch info.json on every call — minor doc drift only. ## Critical Issues ### CR-01: file_drop EVENT path is not bound to receiving wrapper identity (path-injection in wrapper LLM stdin) **File:** `src/live/wrapper/mod.rs:295-328` (parse_file_drop_event) + `src/live/wrapper/mod.rs:1430-1584` (process_file_drop) **Issue:** `parse_file_drop_event` validates only: 1. `kind ∈ {commune, signoff}` 2. `path.ends_with("-{kind}.md")` 3. `path.contains("/.claude/")` There is NO check that the `path` attribute names a file owned by the receiving wrapper's `self_id`. The wrapper for `{X}-psyche` will happily read `/anywhere/.claude/{anybody}-commune.md` and feed the file body — wrapped in a real `` envelope — to its own `claude -p --resume` subprocess. The `from=` attribute is parsed by `extract_attr_value` but never consulted, and there is no comparison against `self.self_id`. Concrete impact: - Any actor able to deliver a message to a psyche perch (the spool fabric is not authenticated) can construct `` and the receiving wrapper will read that file and inject its body into its Psyche session. - A stale signoff envelope sitting in spool from another agent's namespace similarly lands inside a different wrapper's `final_session` and tears it down with an unrelated body. - `path.contains("/.claude/")` matches `/etc/some/.claude/...`, `~/projects/foo/.claude/bar-commune.md`, any path with a `.claude/` segment anywhere — including paths the operator did not anticipate. The wrapper IS supposed to receive paths from its own Self listener only (siblings on the same machine), but the wire envelope makes no statement to that effect and the parser does not enforce it. **Fix:** ```rust // In parse_file_drop_event: also extract from= and return it. let from = extract_attr_value(header, "from")?; // ...add to return tuple. // In handle_file_drop_arm / process_file_drop: reject when from != self_id // AND when the path basename does not start with `{self_id}-`. if from != self_id_of_this_wrapper { state.log_line(&format!( "[FILE-DROP] rejecting cross-agent file_drop: from={} expected={}", from, self_id_of_this_wrapper )); return FileDropOutcome::Miss; } // Stronger: confirm path basename equals `{self_id}-{kind}.md` exactly. let expected_basename = format!("{}-{}.md", self_id_of_this_wrapper, kind); let basename = std::path::Path::new(&path).file_name() .and_then(|s| s.to_str()) .unwrap_or(""); if basename != expected_basename { state.log_line(&format!( "[FILE-DROP] rejecting path with wrong basename: got={} expected={}", basename, expected_basename )); return FileDropOutcome::Miss; } ``` --- ### CR-02: process_file_drop's signoff branch fires final_session even when inbound route write failed; commune branch ignores RouteOutcome IoError before LLM handoff **File:** `src/live/wrapper/mod.rs:1473-1546` **Issue:** The documentation block (lines 1473-1479) and the broader 25.3-04 locked decision state that `route_inbound_commune_body` synchronously persists the inbound slices to disk BEFORE the LLM handoff so that the persisted state is the guarantee, regardless of what the LLM does with the envelope. The code calls `route_inbound_commune_body` only for `kind == "commune"` (line 1480-1498). For `kind == "signoff"` there is NO synchronous slice route before `final_session` runs — the signoff inbound path's ``/`` body slices are NOT persisted to `agents/{self_id}/live_context.md` / `projects//{self_id}.md` synchronously. The signoff body is handed to `compose_init_signoff_payload` which inlines it into the EVENT body for the haiku LLM, but the route-to-disk pre-write is skipped. This contradicts the contract that `route_two_slice_signoff` in `src/live/signoff.rs:76-108` enforces — that path runs in `/spt:signoff`'s online code path, but the **offline path** (file-drop signoff arriving at a wrapper that's still alive) bypasses `route_two_slice_signoff` entirely. The signoff body's two-slice envelope contract is therefore lost: only what the haiku LLM emits during `final_session` reaches disk, and any LLM disagreement (drift, refusal, abbreviated emit) silently loses the original Self-authored body. Additionally: the commune branch checks `RouteOutcome` for `IoError` only to log (line 1486-1497). It does NOT abort or retain. The file gets deleted on LLM exit 0 (line 1549) even if the inbound disk route failed — the file_drop contract that "the file's content lives on disk after consume" is broken on the IoError path because both the file is gone AND the slices are not on disk. **Fix:** ```rust // Signoff branch must also route to disk synchronously before final_session: if kind == "signoff" { // Use the existing signoff router for parity with the online path. let outcome = crate::live::signoff::route_two_slice_signoff(&self.self_id, &body); // ...log + check for IoError as the commune branch does. } // Both branches: do not delete the file on exit 0 unless the inbound route's // live + project outcomes are Written|SkippedNoSlice|SkippedNoProjectName. // IoError on either slice should force retention (D8 atomicity ALSO covers // the persist-to-disk guarantee, not just the LLM ack): if exit_code == 0 && !route_failed { fs::remove_file(path); } else { self.log(&format!("[FILE-DROP] retained kind={} path={} exit={} route_failed={}", kind, path, exit_code, route_failed)); } ``` --- ### CR-03: `init_session` clears the .last-commune-epoch cursor BEFORE confirming the new session boots; on init failure the cursor is permanently lost **File:** `src/live/wrapper/claude.rs:104` (the `let _ = std::fs::remove_file(...)`) **Issue:** The cursor clear is intentional per the Phase 18.8 Pitfall 5 comment to avoid stale-cursor filtering on fresh sessions. But the unconditional remove runs at line 104, BEFORE `init_cmd.spawn()` at line 107 and BEFORE the session_uuid extraction at line 142. If init fails (lines 145-148 or 189-192 both call `std::process::exit(1)`), the wrapper terminates and the cursor is gone with no successor session to use it. The next wrapper boot would also start with a cleared cursor (which is the safe degradation per the comment), BUT any external observer (echo_commune fired by a sibling, manual cursor- peek tool) sees a transient zero cursor for the dead-wrapper window. More concretely, the `let _ =` discards the I/O error — a permission-denied or in-flight rename collision is silently swallowed, masking real bugs. Combined with the WR-08 acknowledged race in the comment (where a previous session's echo_commune can RE-WRITE the cursor after the remove), the cursor state is highly nondeterministic during wrapper init. The recovery path "natural fallback on missing/stale cursor is full 64KB-tail" is correct, but the unconditional clear should at least be gated on successful spawn. **Fix:** Move the remove to AFTER `extract_session_uuid(&stdout)` returns `Some(uuid)` so the cursor is only cleared when we know the new session has started: ```rust // In init_session, after the `match session_uuid { Some(uuid) => { ... } }` // arm has assigned self.session_uuid: match session_uuid { Some(uuid) => { // ... existing code ... // NOW it's safe to clear the cursor — new session is fully up. if let Err(e) = std::fs::remove_file( crate::common::owlery::last_commune_epoch_path(&self.self_id) ) { if e.kind() != std::io::ErrorKind::NotFound { self.log(&format!("init_session cursor clear failed: {}", e)); } } // ... wrapper_state publish ... } None => { /* exit(1) — cursor unchanged */ } } ``` ## Warnings ### WR-01: Suppression-window decision loses observability through SliceWriteState::SkippedNoSlice mapping **File:** `src/owl/echo_commune.rs:571,607` (route_two_slice_with_precedence) **Issue:** When `write_with_precedence` returns `WriteOutcome::Suppressed` (LLM source declined inside the precedence window), the caller maps it to `SliceWriteState::SkippedNoSlice` (lines 571, 607). The downstream logging in `route_inbound_commune_body` and `route_two_slice_signoff` cannot distinguish "the LLM emitted no live slice" from "the LLM emitted a live slice but the precedence guard suppressed it". This collapses two distinct cases that operators will want to grep for separately when diagnosing why a commune body did not land on disk. The stderr `[ROUTE-GUARD] decision=SUPPRESS ...` line printed by `write_with_precedence` is the only signal, and it doesn't propagate to the structured `RouteOutcome` consumed by callers. **Fix:** Add a `Suppressed` variant to `SliceWriteState`: ```rust pub(crate) enum SliceWriteState { Written, SkippedNoSlice, SkippedNoProjectName, Suppressed, // NEW: distinguishes precedence-gate from no-slice-in-payload IoError(String), } ``` Update the format_slice_state helper in `src/live/signoff.rs:110-118` to render the new variant. --- ### WR-02: `read_route_guard_window_secs` reads `.planning/config.json` relative to process cwd; in deployed wrappers cwd is `psyche_dir` so the config value is dead **File:** `src/common/owlery.rs:1580-1594` **Issue:** The function's own doc-comment acknowledges: > "Known follow-up (cycle-4 MEDIUM not closed): config path is relative to > process cwd. Live wrappers run from psyche/runtime cwd, so deployed wrappers > always fall back to default 60." This means the configurable window is non-functional in the deployed path. Tests pass because they `set_current_dir(tmp.path())` before calling. The operator surface is broken-by-design; the default value is the only observable behavior in production. Either move config to `$SPT_HOME/config.json` (canonical owlery resolution, like `spt_home()`) or document the value as test-only. **Fix:** ```rust pub(crate) fn read_route_guard_window_secs() -> i64 { // Use SPT_HOME-relative config so deployed wrappers can read it // regardless of process cwd. let path = spt_home().join("config.json"); let content = match std::fs::read_to_string(&path) { Ok(c) => c, Err(_) => return 60, }; // ... rest unchanged } ``` --- ### WR-03: `route_inbound_commune_body` and `route_two_slice_with_precedence` ignore `tracked::commit_*_payload` failures with `let _ = ...` **File:** `src/owl/echo_commune.rs:478, 513, 564, 600` **Issue:** When the file write succeeds but the git commit fails (network/permissions/ disk full mid-commit), the failure is silently dropped via `let _ = tracked::commit_agent_payload(...)`. The body is on disk but uncommitted; subsequent code paths assume the payload is durable (the `Phase 24 D-02 contract: payload landing on disk is the guarantee; git failures soft-fail` is documented in `src/live/context.rs:31-34` and warned via `warn_tracked_commit_failed`, which is a once-per-process gate). The two callers here do NOT route through `warn_tracked_commit_failed`, so the once-per-process warning logic never fires for commit failures originating in `route_two_slice_*`. **Fix:** Use the existing helper. In `route_two_slice_outcome` / `route_two_slice_with_precedence`: ```rust if let Err(e) = tracked::commit_agent_payload(self_id, &["live_context.md"], &subject) { crate::live::context::warn_tracked_commit_failed(self_id, &e); } ``` This requires promoting `warn_tracked_commit_failed` to `pub(crate)` (currently private to context.rs). --- ### WR-04: `compose_llm_prompt_from_envelope` builds prompt header even on missing attrs, surfacing "(unknown)" / "(no timestamp)" to LLM **File:** `src/live/wrapper/mod.rs:557-625` **Issue:** When an attribute (`machine`, `project`, `branch`, `head_sha`, `timestamp`) is missing from the envelope, the helper substitutes the literal strings `"(unknown)"` / `"(no timestamp)"` (lines 600-619). These literals become user-visible text in the Psyche prompt. The Psyche LLM may interpret "(unknown)" as a meaningful project name or pass it through into its own output. The original substitute "EVENT" for missing type is even more unsafe — the LLM sees an EVENT-shaped natural-language header but no semantic classification. Consider: an inbound commune from outside a tracked project (no `branch` / `head_sha`) renders as `branch: (unknown)`, which the LLM may rewrite as "the inbound branch was unknown" in its own commune output — noise that pollutes downstream context. **Fix:** Omit missing attrs from the header rather than substituting placeholder strings. Build the header conditionally: ```rust let mut lines = vec![format!("Inbound {} envelope at {}:", attrs.get("type").map(String::as_str).unwrap_or("EVENT"), attrs.get("timestamp").map(String::as_str).unwrap_or("(no timestamp)"), )]; for key in &["machine", "project", "branch", "head_sha"] { if let Some(v) = attrs.get(*key) { lines.push(format!(" {}: {}", key, v)); } } format!("{}\n\n{}", lines.join("\n"), body_decoded) ``` --- ### WR-05: `parse_event_attrs` walker bails on `break` for malformed attrs, silently dropping later valid attrs **File:** `src/live/wrapper/mod.rs:634-684` **Issue:** The hand-rolled attribute parser uses `break` on every malformed character class transition (missing `=`, missing opening `"`, missing closing `"`). Once `break` fires, all subsequent attrs in the opening tag are dropped even though they may be valid. A single malformed attr corrupts the prompt attrs map silently — no log line, no telemetry. Example: `body` — the `malformed-attr=` (missing value) triggers `break` at line 663, and `project="foo"` is never recorded. The LLM receives a prompt header missing the project attribute. **Fix:** Skip the malformed attr but continue parsing. Replace the `break`s with `continue`-style logic that scans to the next whitespace or attr boundary: ```rust // On missing '=' or malformed value: advance to next whitespace and continue. if i >= bytes.len() || bytes[i] != b'=' { while i < bytes.len() && !(bytes[i] as char).is_whitespace() { i += 1; } continue; } ``` --- ### WR-06: `dedupe_drops` HashSet of paths grows unbounded across listener iterations when many distinct files appear **File:** `src/owl/poll.rs:332, 1265-1285` **Issue:** `emitted_drops` is allocated once at line 332 and survives the entire listener lifetime. `dedupe_drops` calls `emitted.retain(|p| current_paths.contains(p))` which prunes paths whose files have disappeared — good. But the `current_paths` HashSet is rebuilt every iteration from `drops`, so the working set tracks ONLY currently-existing drop files. Memory is bounded by "number of distinct .claude/{id}-{commune,signoff}.md files ever present simultaneously" — typically 2. The `path` strings are full forward-slash absolute paths. The behavior is correct, but `current_paths` and `emitted_drops` are both allocated on EVERY iteration with HashSet. The retain on the shared set works, but the `current_paths` rebuild is O(n) HashMap+String work per iteration. In a tight idle loop this is allocation churn. Performance is out of v1 scope so this is logged as Warning for code quality only. **Fix:** Use `HashSet<&str>` for `current_paths` (avoiding String clones for the lookup) and reuse a single Vec alloc for emitted_drops keys: ```rust let current_paths: std::collections::HashSet<&str> = drops.iter().map(|(_, p)| p.as_str()).collect(); emitted.retain(|p| current_paths.contains(p.as_str())); ``` --- ### WR-07: `parse_file_drop_event` accepts case-insensitive `EVENT` opening tag but case-sensitive `kind=` / `path=` attribute keys **File:** `src/live/wrapper/mod.rs:295-328, 337-356` **Issue:** Line 302 matches ` Option { let lower = header.to_ascii_lowercase(); let needle = format!("{}=\"", key); let start = lower.find(&needle)?; // Slice ORIGINAL header at lower-found offset so case-sensitive attr VALUES // (e.g. file paths on Unix) are preserved. let value_start = start + needle.len(); let rest = &header[value_start..]; // ... rest unchanged } ``` --- ### WR-08: `is_within_tracked_psyche_dir` falls back to lexical comparison when canonicalize fails, but ONE side may still be canonicalized — inconsistent comparison **File:** `src/common/owlery.rs:869-874` **Issue:** ```rust let path_canon = path.canonicalize().unwrap_or_else(|_| path.to_path_buf()); let psyche_canon = psyche.canonicalize().unwrap_or(psyche); path_canon == psyche_canon || path_canon.starts_with(&psyche_canon) ``` If `path.canonicalize()` succeeds but `psyche.canonicalize()` fails (psyche_dir not yet created? mount unavailable?), `path_canon` is `\\?\C:\...` UNC-prefixed and `psyche_canon` is `C:\Users\foo\spt\psyches\tracked\` plain. The equality and `starts_with` both fail — false negative, the `tracked` filter passes a psyche_dir false positive through. The reverse case (psyche canonicalized but path not) yields the symmetric false negative. **Fix:** Either canonicalize BOTH or NEITHER; do not mix. The defensive documentation hints at a "only consulted when a .git ancestor was found" short-circuit but the helper itself does not enforce that. Make canonicalization all-or-nothing: ```rust fn is_within_tracked_psyche_dir(path: &std::path::Path) -> bool { let psyche = psyche_dir(); match (path.canonicalize(), psyche.canonicalize()) { (Ok(p), Ok(ps)) => p == ps || p.starts_with(&ps), // Either side failed — fall back to lexical on both. _ => path == psyche.as_path() || path.starts_with(&psyche), } } ``` ## Info ### IN-01: commune.md project-resolution wording slightly drifts from implementation **File:** `plugin/spt/skills/commune/commune.md:25` **Issue:** The doc says: > "where `` is resolved BY SPT via Self's perch > `info.json` `cwd` field (25.3-01 Defect B2 fix)" This is accurate for the wrapper-inbound route (`route_inbound_commune_body` + `route_two_slice_with_precedence` → `resolve_self_project_name_via_info_cwd`). But the LEGACY `route_two_slice` function (still called from tests and still the only path for systems running pre-25.3-04 binaries during a rolling deploy) uses `derive_current_repo_names().first()` — process cwd basename walk. The doc should clarify that the resolver chain is `info.json.cwd` first, with no fallback. Currently the wording implies a single canonical source but two coexist. **Fix:** Add a sentence noting the legacy path resolver still exists for back-compat but the wrapper-inbound surface uses info.json.cwd exclusively. --- ### IN-02: `dispatch_commune_markers` static `note` is hardcoded to "Echo commune brief — auto-fired" with no source distinction **File:** `src/owl/echo_commune.rs:236-237` **Issue:** Whether the fire is cadence-triggered (Stop-sentinel) or Phase 29 SessionStart-triggered (`source="clear"` / `"compact"`), the EVENT `note=` attribute is the same literal string. Operators investigating logs cannot tell whether a given echo_commune brief was wrapper-cadence or force-triggered without checking the wrapper's `[FIRE-EC]` log line. The existing `force` and `forward_to_self` parameters are plumbed through but not surfaced in note. **Fix:** Thread `source` through `dispatch_commune_markers` and render `note=` as `"Echo commune brief — auto-fired ({source})"` or similar. --- ### IN-03: `compose_init_signoff_payload` always renders `
` separator, breaking when message contains literal `
` **File:** `src/live/signoff.rs:42-53` **Issue:** When `message.replace('\n', "
")` runs, a user-authored final commune body that contains the literal substring `
` will not round-trip correctly through the EVENT body — the receiver's `event_body_unescape` converts every `
` back to `\n`. This is documented and consistent with the body-escape contract, but the SIGNOFF composer uses raw `.replace('\n', "
")` WITHOUT going through `event_body_escape`. The `current_blocks` body content (read via `read_context_body_stripped` from `live_context.md`) is NOT escaped for `&`, `<`, `>`, `"`. If the prior live_context.md contains an `&` literal or any `<` character, the EVENT envelope is malformed (the inner `<` becomes a structural token). The standard pattern uses `event_body_escape(body).replace('\n', '
')` — but `event_body_escape` already does the newline replacement. Here the composer does the newline replacement WITHOUT the escape pass. **Fix:** Run the inlined `current_blocks` through `event_body_escape` first, not `replace('\n', "
")` alone: ```rust let event_body = match message { Some(msg) => format!( "Signoff initiated by Self

{}

FINAL COMMUNE:
{}", crate::owl::poll::event_body_escape(¤t_blocks), crate::owl::poll::event_body_escape(msg), ), None => format!( "Signoff initiated by Self

{}", crate::owl::poll::event_body_escape(¤t_blocks), ), }; ``` --- ### IN-04: Test `nested_worker_layout` accepts hook no-op stderr signal as success, masking real failures **File:** `tests/native_owl.rs:1034-1041` **Issue:** ```rust let hook_noop = stderr.contains("no_parent") || stderr.contains("could not") || stderr.contains("AUTO-SETUP"); assert!( found_nested_worker || hook_noop, ... ); ``` Any stderr containing "could not" satisfies the assertion. This includes spurious error paths unrelated to the parent-resolution no-op — e.g., a git-not-found failure would print "could not find git" and the test passes without verifying the worker layout. The test should require an EXACT diagnostic substring, not a vague set of substrings. **Fix:** Match a specific debug line emitted by the hook's no-op branch, or remove the no-op acceptance entirely and unconditionally require `found_nested_worker`. --- ### IN-05: `extract_attr_value` unescape order omits `'` decoding (handled in `parse_event_attrs` sibling) **File:** `src/live/wrapper/mod.rs:350-355` **Issue:** The body-decoder `event_body_unescape` (poll.rs:775-781) handles `
`, `<`, `>`, `"`, `&`. The attr decoder `parse_event_attrs` in mod.rs:674-680 additionally handles `'`. The NEW helper `extract_attr_value` (mod.rs:337-356) does NOT decode `'`. If a producer ever emits a single-quote in an attribute value as `'`, the file_drop parse_event consumer sees a literal `'` in the path/kind/from slot. This is unreachable in production because `event_attr_escape` doesn't emit `'` (it doesn't escape `'`), but the inconsistency is real and worth a single-line fix for symmetry. **Fix:** Add the missing replacement: ```rust let unescaped = raw .replace("<", "<") .replace(">", ">") .replace(""", "\"") .replace("'", "'") // <-- add for parity with parse_event_attrs .replace("&", "&"); ``` --- _Reviewed: 2026-05-22T00:00:00Z_ _Reviewer: Claude (gsd-code-reviewer)_ _Depth: standard_