fix: run local user commands in a dedicated workspace (closes the 2026-07-27 wipe mechanism) - #239
Merged
Merged
Conversation
Local user commands (run_command, run_script, manage_process) inherited the service's working directory, which is the install root. A bare relative path therefore resolved against the live install: on 2026-07-27, AE2 jar research ran `jar xf <jar> data/ae2/... && cat ... && rm -rf data`, and because the jar's internal layout starts with `data/`, that cleanup deleted /opt/odin/data — memory, learned lessons, skills, sessions, trajectories, the knowledge index, and all three codex_auth files. The intent was legitimate. The relative path was the whole bug. So this closes the mechanism without touching the capability: - tools.local_working_dir (default /var/lib/odin-workspace) is resolved and validated, then passed as the subprocess cwd for local user commands. - Deliberately a SIBLING of /var/lib/odin, since packaged installs use that as the live data directory. Not /tmp (tmpfiles policy ages it out), not $HOME (packaged Odin declares /opt/odin as the service account's home). - STABLE and persistent by design. A fresh directory per command would break two-step workflows that write a relative file in one command and read it in the next — that would cost capability. Restart-required, not hot-reloadable, since swapping mid-run breaks exactly that continuity. - PWD *and* OLDPWD are normalized. cwd= alone leaves an inherited OLDPWD pointing at the install, so a bare `cd -` walks straight back in. - Background starts share the workspace, or manage_process remains an alternate route to the same incident (29 historical background starts had no explicit cd). - Validation fails CLOSED: missing, symlinked, non-private, or inside-the-install workspaces raise rather than silently falling back to the inherited cwd, which would restore the hazard. Resolution is lazy so executor construction never depends on deployment having provisioned it. What is deliberately NOT changed: the Python process cwd and systemd WorkingDirectory stay /opt/odin (the app resolves ./data/... against them — moving those would recreate this incident by another door); explicit `cd`, absolute paths, and `git -C <path>` behave exactly as before, so working inside the install remains possible; remote SSH execution is untouched; and no prompt surface changes — no tool description, schema, or output format. Scope honesty: an external cwd prevents the accidental basename collision. It does not confine the process — `..`, an absolute path, a symlink inside the workspace, or archive path traversal can still reach the install. Preventing those needs filesystem confinement and would cost capability. Tests replay the actual incident: a synthetic jar containing data/ae2/..., extract + read + `rm -rf data` as three separate commands, asserting the workflow SUCCEEDS while a sentinel in the fake install survives. The test process chdirs into the fake install first, so a regression in the cwd plumbing destroys the fixture and fails loudly; every path is under tmp_path and that is asserted before any deletion runs. Also pinned: explicit cd into the install still works, absolute paths unaffected, relative files persist across commands, `cd -` cannot return to the install, background processes use the workspace, remote SSH has no cwd parameter, and the legacy src/odin/tools DAG surfaces are classified so they cannot be wired into the Discord path without reopening this. Gates: ruff clean, mypy clean (231 files), 7520 passed / 4 skipped. Designed with Odin; his review caught the OLDPWD hole, the /var/lib/odin collision, the background-process route, and the sibling-path requirement.
…ecutor-level tests All four blockers were correct. The fourth was the serious one. 1. PROTECTED ROOTS were hardcoded to /opt/odin, so the validator protected nothing under Docker (install root /app) or in a source checkout, and a string-joined `install / "data"` missed that packaged /opt/odin/data is a SYMLINK to /var/lib/odin — a workspace inside the real live-data directory passed. The overlap check was also one-directional. The executor now derives roots from the RUNNING application (package location + the actual configured data paths, canonicalized), and overlap is rejected in BOTH directions. 2. PROVISIONING did not exist on any supported install path, so the fail-closed default would cost local-command capability on first use. Added to Debian postinstall (0700 odin:odin), the Dockerfile, and docker-compose (persisted, so cross-command continuity survives restarts). A test asserts every install path declares it, since the session fixture would otherwise keep masking the gap. 3. OWNERSHIP was never validated and mode 0300 passed — private and writable but not readable, which breaks ordinary use. Now: owner uid must match the execution identity, mode must be exactly 0700, and R_OK|W_OK|X_OK is verified. 4. THE HEADLINE TESTS BYPASSED THE SEAM THAT CAUSED THE INCIDENT. They called run_local_command(cwd=...) directly, proving that function honors a cwd but NOT that ToolExecutor supplies one — deleting the executor's cwd argument would have reopened the wipe with the suite still green. My mutation testing had hit the wrong layer entirely. The replay now runs through ToolExecutor._exec_command, background coverage asserts the registry gets its workspace FROM the executor, and a further pin proves the executor actually passes its protected roots (without it, dropping that argument left every other test green). Mutation-verified at the executor seam: removing the executor's cwd, removing the registry workspace, and removing protected_roots each now fail. Also added, per the review: symlinked live-data alias, non-/opt install root, bidirectional overlap, and wrong-owner cases. Gates: ruff clean, mypy clean (231 files), 7537 passed / 4 skipped.
…cs, executable legacy boundary
All four correct, including the one you flagged as your own round-one miss.
1. HIGH — memory_path was not protected. Data roots came only from
audit_log_path/trajectory_path, so relocating those left a workspace beside
the live memory.json accepted (you reproduced it as UNSAFE_ACCEPTED). The
executor now also protects the canonical parent of the memory_path supplied
by wiring. The Path.suffix heuristic is gone: each path is classified by
DECLARED semantics (audit = file, trajectories = directory, memory = file),
since a suffix guess misreads dotted directories and extensionless files.
Both are pinned, and the memory_path pin is mutation-verified — without it,
removing the protection left every other test green.
2. HIGH — fresh Docker Compose still failed closed. The bind mount masked the
image's 0700 directory, and a fresh host bind is created 755 root:root.
Switched to a NAMED volume, which Docker initializes from the image
directory and therefore inherits the required mode. The from-source path is
documented too: README now shows creating /var/lib/odin-workspace 0700, or
pointing tools.local_working_dir at any private directory outside the
checkout.
3. Accepted requirement delivered — workspace growth metrics. No auto-prune
was always paired with observability: odin_workspace_bytes, _files,
_free_bytes, _free_inodes now export through the existing collector,
registered in __main__ beside the other tool sources. Verified rendering
end to end. Collection never raises, so metrics cannot break a command path.
4. Legacy-surface characterization made executable, in this PR. The old test
grepped two literal import spellings and would have stayed green under
transitive wiring. It now imports the Discord-reachable boundary and proves
the routing table contains no legacy owner, that a fresh import of the
execution path does not pull src.odin.tools.{shell,process} into
sys.modules, and that no executor handler domain is defined in those
modules. The legacy CLI's own behaviour is untouched.
Gates: ruff clean, mypy clean (231 files), 7539 passed / 4 skipped.
…, seamless upgrades
Four review findings plus Aaron's upgrade requirement.
1. HIGH — validation was cached, so fail-closed applied only to the FIRST
command. Replacing the workspace with a symlink into the install afterwards
was accepted, and a post-validation chmod was ignored (both reproduced).
The configured VALUE stays restart-required, but existence, type, ownership
and mode are mutable filesystem state and are now verified immediately
before every foreground spawn. ProcessRegistry takes the RESOLVER rather
than a resolved string, so background spawns revalidate too.
2. HIGH — leaf-symlinked file roots protected the alias directory, not the
target: `.parent` was taken BEFORE canonicalization, so
/aliases/memory.json -> /live-data/memory.json protected /aliases and
accepted /live-data/workspace. The full declared path is resolved first.
3. The legacy-boundary test was still largely vacuous — `__new__` created no
handler owners so the loop inspected nothing, and it reloaded the executor
rather than the Discord path. It now builds a real executor, asserts every
resolved owner's module, and imports src.discord.tool_loop in a CLEAN
interpreter to prove neither legacy module loads.
4. CI was red and I had reported it green: I ran local gates but never the
coverage ratchet. That was a reporting error, not a passing build. The new
metrics/executor lines are now covered (including the defensive branches)
and coverage-no-drop reports findings=0.
UPGRADE SEAMLESSNESS (Aaron): a git-based self-update or an existing install
would have landed on this code with no workspace and failed closed, silently
costing local commands. Every path now provisions itself with no manual step:
- systemd StateDirectory=odin-workspace / StateDirectoryMode=0700 creates it
on EVERY start, which covers fresh installs and any restart after a
self-update;
- the Debian postinstall creates it on install and upgrade;
- the Docker image plus a named volume (not a bind mount, which masked the
image's 0700 directory and arrived 755 root:root);
- the runtime self-provisions when the parent is writable, covering source
checkouts and git self-updates whose unit file was never refreshed.
Creating it is not the dangerous fallback — inheriting the install directory
is — and everything is still validated afterwards. When self-provisioning
cannot work, the error names the exact command instead of degrading silently.
Gates (all four, run as CI runs them): tests 7554 passed / 4 skipped,
lint-no-new 0, types-no-new 0, coverage-no-drop findings=0.
…usal, no mkdir before validation All four correct. Blocker 1 was verified against the live install and would have broken exactly the update requirement Aaron set. 1. BLOCKER — the live git self-update path was not seamless. StateDirectory= only applies when systemd STARTS the service, but the WebUI updater re-execs in place, so systemd never runs and a unit written before this feature never gains it. Verified live: /var/lib/odin-workspace absent, the running unit has no StateDirectory=, odin cannot write /var/lib, and the git update does not install the packaged unit — so updating live Odin would have re-exec'd successfully and then failed closed on the first local command. apply_update now runs a preflight that provisions the workspace BEFORE committing to the update (direct mkdir, then sudo, which packaged installs already configure) and REFUSES with an actionable 409 rather than transitioning to code that cannot work. The preflight reads the one config key directly rather than constructing a full Config: load_config() raises SystemExit when env vars are missing, which must never happen inside a request handler, and its result is cwd-dependent. 2. BLOCKER — the Incus deployment path was missed entirely. Its unprivileged odin user cannot create /var/lib/odin-workspace, and its generated unit had no StateDirectory=. Both added, and the install-path test now covers Incus alongside Debian, Docker, and source. 3. BLOCKER — a background workspace refusal was classified as SUCCESS. The plain "Failed to start process: …" string does not match the error-prefix contract, so the tool loop saw ok=True for a refusal. It now returns a visible Error: prefix, pinned against the real contract. 4. Self-provisioning mutated a protected root before rejecting it: mkdir ran BEFORE the overlap check, so an invalid workspace was created inside the very tree this protects and only then refused. Overlap is now checked first. The previous test here was a tautology that passed either way; it is replaced with one that asserts the directory was never created. Gates, all four as CI runs them: 7564 passed / 4 skipped, lint-no-new 0, types-no-new 0, coverage-no-drop findings=0. Recorded, not fixed: validation and spawn are pathname-separated, so a concurrent same-UID process could swap the directory in the narrow interval before chdir. Odin judged this out of scope under the accepted non-confinement threat model, and closing it would change concurrent replacement semantics.
…ovisioning contract Both blockers were right, and the first is one I had no answer for. 1. BOOTSTRAP PARADOX. A preflight living only in the self-updater cannot bootstrap itself: the update that INTRODUCES the preflight is executed by the previous release's handler, which has none. It merges, re-execs, and the new code finds no workspace. Verified against a live install — that first upgrade would have failed closed on every local command. Fixed where it actually works: a startup migration in __main__, running after the real configuration loads and before the bot (and therefore any command service) is constructed. That runs in the NEW code on the restart following any update, however the update arrived — .deb, git self-update, Incus, Docker or source. Provisioning failure is logged, never fatal: an unusable workspace must not stop Odin from starting and answering on Discord, and local commands still fail closed individually with the same actionable error. Pinned by a test asserting the ordering (config load -> provision -> bot construction), so the migration cannot drift after command service starts. 2. TWO CONTRACTS. The updater helper reimplemented a weaker validation — it reparsed config.yml instead of using live config, ignored an alternate config path, skipped environment substitution, permitted relative paths, followed workspace symlinks, and checked neither protected-root overlap nor ownership, while creating with parents=True. Four concrete mismatches were reproduced where it accepted what the runtime rejects, and it could provision a different directory from the one the restarted executor uses. There is now ONE authoritative routine, workspace.provision_workspace(): everything judgeable from the path alone (absolute, non-symlink, no protected-root overlap) is checked BEFORE creation, creation is direct then sudo, and the full resolve_workspace() contract is applied afterwards. Both the startup migration and the updater call it, and the updater feeds it the LIVE bot configuration so the path validated is the path the restarted process will use. All four mismatch cases are pinned, including that a refused workspace leaves nothing created. Mutation-verified: removing the startup migration, the live-config read, or the pre-creation overlap check each fails its test. Gates, all four as CI runs them: 7567 passed / 4 skipped, lint-no-new 0, types-no-new 0, coverage-no-drop findings=0.
…tected roots Both blockers were real, and the first is embarrassing in a way worth naming: the round-5 fix did not run at all. 1. THE STARTUP MIGRATION NEVER RAN under `python -m src`. The entrypoint guard sat above `_command_protected_roots`, so executing the module as `__main__` called main() before that `def` was reached. The resulting NameError was swallowed by the migration's own deliberately-nonfatal handler: Odin started normally, logged one line, and the workspace was never created — exactly the first-update bootstrap failure round 5 existed to fix, reintroduced by statement order. My round-5 test compared SOURCE positions, which cannot see this; Python's execution order is the thing that matters. The guard is now the last statement in the module with a comment saying why, and the new test runs the real entrypoint in a subprocess — patching OdinBot to record whether the workspace exists at the moment of construction — so the ordering property is asserted by execution, not by reading. 2. THREE DERIVATIONS OF THE PROTECTED ROOTS, and they disagreed. The executor protected the live memory.json supplied by wiring; the startup migration read `config.memory.path`, which does not exist on Config, so it protected nothing; the updater omitted memory entirely. With audit and trajectory paths relocated, the preflight therefore CREATED a workspace inside the live-data directory, reported success, and handed over to an executor that refused every local command — mutating live data and stranding commands in one step. There is now one function, workspace.command_protected_roots(), used by all three, and the live memory path is a shared constant that wiring imports rather than re-spells. The relocated-audit/trajectory scenario is pinned against each caller independently, plus a test asserting all three agree on the live-data root, each asserting nothing was created. Mutation-verified: restoring the guard above the helper, reverting the startup helper to `config.memory.path`, or dropping memory from the updater's roots each fails its test. Gates, all four as CI runs them: 7572 passed / 4 skipped, lint-no-new 0, types-no-new 0, coverage-no-drop findings=0.
…rent things 1. BLANK LIVE VALUE. local_working_dir accepts free strings and can be blanked through PUT /api/config. The preflight treated a present-but-blank value as "nothing configured" and validated the DEFAULT instead, so the update was approved while the restarted process loaded the actual blank value: startup provisioning failed nonfatally and every local command then failed closed. Preflight and runtime disagreeing about the very path being validated — the same class as round 6, one layer up. Closed at both ends. The schema now normalizes blank/whitespace to the default at the boundary, so every consumer — preflight, startup migration, executor — reads one identical value; normalizing rather than rejecting keeps the update seamless and cannot brick startup on a persisted config, which a hard validation error would. And the preflight now validates the live value VERBATIM, never substituting: reaching it with a blank value means the live config is not a validated ToolsConfig, which is precisely when guessing a plausible default is least safe. The schema default is used only when there is no live config at all. Pinned end to end through POST /api/update/apply with a live blank config and nothing stubbed but the exec primitives: 409, actionable message, and the repository never touched. 2. `git diff --check` — trailing blank line at EOF removed. Now clean. Mutation-verified: dropping the schema normalization, or restoring the preflight's substitution, each fails its test (the latter fails both the unit and the endpoint pin). Gates, all four as CI runs them: 7578 passed / 4 skipped, lint-no-new 0, types-no-new 0, coverage-no-drop findings=0. git diff --check clean.
… full config Both blockers were scope violations I had not seen: the change reached further than the mechanism it was written to close. 1. THE WORKSPACE WAS CHANGING UNRELATED TOOLS. _exec_command is shared, and it also backs git_ops, docker, terraform, kubectl, claude_code, PDF host reads and validation probes. Applying the workspace unconditionally there silently repointed every one of them: git_ops documents an omitted `repo` as ".", which has always meant the process cwd — the install repo — so `git_ops status` returned "fatal: not a git repository". Docker build ".", compose's implicit project directory and terraform without working_dir are the same class. The workspace is now opt-in at the call site, passed only by run_command, run_script and run_command_multi — the raw user-command routes the incident came through — plus background starts, which resolve it directly. Every other tool is byte-identical to pre-PR behaviour, including keeping the process cwd and skipping workspace validation entirely. That also makes the incident tests stricter: they now drive the real run_command dispatch rather than the shared primitive, so removing the opt-in from the handler fails them. Two contract tests pin the boundary — git_ops with `repo` omitted keeps process-cwd semantics, and an unusable workspace disables raw user commands ONLY (one bad directory must not cost most of Odin's capability). 2. THE PROTECTED ROOTS WERE INCOMPLETE. They covered install, audit, trajectory and memory — but live state is not confined to the data directory. sessions.persist_directory, context.directory, the search index, permissions, Codex credentials, logs and usage are each independently relocatable, and a valid Config whose sessions directory WAS the workspace was accepted by all three callers. They agreed on an incomplete set, so the "outside the live-data roots" invariant was still false. Roots are now derived from the FULL live configuration through one declared file/directory table, supplied by wiring to the executor and used by the startup migration and the updater. A caller holding only a ToolsConfig gets a strict SUBSET, never a different answer, and a test pins that wiring supplies the full config so production never runs on the reduced form. Every declared path has a relocation case, and the table is asserted exhaustive so adding a path without protecting it fails. Also, per review: PUT /api/config blanking the workspace is now pinned for real — response, runtime config, persisted YAML and fresh reload — and the self-update test renamed to say what it actually drives. Mutation-verified: dropping the run_command opt-in, restoring unconditional application, ignoring the full config in the derivation, or removing wiring's app_config each fails its test. Gates, all four as CI runs them: 7587 passed / 4 skipped, lint-no-new 0, types-no-new 0, coverage-no-drop findings=0. git diff --check clean.
…le, cached metrics 1. SkillContext.run_on_host BYPASSED THE WORKSPACE. Round 8 scoped the workspace to the raw user-command routes, and I judged this one out of scope. That was wrong: it is arbitrary command execution deliberately exposed to user-created skills, so it is exactly a raw command route. Reproduced through the real method against a fake install — the command entered the install and deleted its data/ while the workspace sat untouched. It now opts in, with a localhost incident replay and the same fail-closed pin as the other routes. Remote hosts are unaffected: the workspace applies only after local-address resolution. 2. THE ACTIVE CONFIG FILE WAS UNPROTECTED. Odin accepts `python -m src /arbitrary/path/odin.yml`, and load_config records that canonical path — but it is runtime state rather than a Config field, so the exhaustive-declaration test structurally could not catch it. An alternate config whose parent was also the configured workspace was accepted, leaving a bare relative command able to delete the file needed to restart. Its canonical parent now joins the shared derivation used by startup, executor and updater; the complete file path is resolved before .parent so an aliased config protects the real directory, and both overlap directions are pinned. A process that never loaded a config still protects nothing extra — guessing a path would reject legitimate workspaces. 3. Metrics (his non-blocking note, worth fixing now): the size/count walk ran synchronously on every unauthenticated /metrics scrape, over a directory that deliberately never prunes — an event-loop stall that only gets worse. The walk is now cached for 60s; free space and inodes stay live. Growth is an operator signal measured in hours, so staleness costs nothing. Mutation-verified: dropping the skill opt-in, the active-config root, or the metrics cache each fails its test. Gates, all four as CI runs them: 7594 passed / 4 skipped, lint-no-new 0, types-no-new 0, coverage-no-drop findings=0. git diff --check clean.
Odin's reviews found workspace bypasses one route at a time — round 9's was SkillContext.run_on_host, arbitrary shell handed to user-written skills, still inheriting the process cwd. Finding them individually is not a contract. This enumerates every module in src/ that spawns (or replaces) a local process by AST, and requires each to be classified: routed through the workspace, or justified as not a user-command path. Adding a new spawn site fails the test until someone decides which it is — the default, inheriting Odin's cwd, is the 2026-07-27 mechanism. Twelve sites classified; two use the workspace, eight are argv-form or infrastructure paths, two are the legacy CLI surface already pinned as unreachable from Discord. Mutation-verified with a new unclassified spawn.
…lternate-config persistence Four blockers, and the first pair share a shape my round-9 contract missed: it classified where processes are SPAWNED, not who ASKS for one. Both fixes below are now held by a contract on that second axis. 1. validate_action executed user-supplied command text through the shared helper without the workspace, so a command check could replay the relative-path mechanism into the install. Reproduced through the real dispatch. It opts in now, with an incident replay and a fail-closed pin. 4. write_file's schema documents an absolute path but nothing enforced it, so a relative path silently wrote into the install. Rejected now with an error naming why — better than quietly redirecting into the workspace, since a write landing somewhere the caller did not choose is its own hazard. No documented capability is lost; absolute paths are untouched. Both are now covered by a table of every _exec_command/_run_on_host call site, each declared WORKSPACE or given a reason. A new call site fails until classified, and flipping an existing one's opt-in fails too. 2. The active config's LAUNCH path was unprotected. set_active_config_path canonicalizes, but restart.reexec replays sys.argv — so for an aliased config the next process opens the alias, and deleting it breaks the restart even though the target survives. Both the canonical target's directory and the launch path's own directory are protected now; the launch parent is canonicalized as a DIRECTORY so the symlink is not followed back to the target. 3. PUT /api/config persisted to a cwd-relative config.yml rather than the active config file. On an alternate-config deployment a workspace change lived in bot.config, was validated by the self-update preflight, and then vanished on re-exec — directly contradicting the preflight's contract that it validates what the restarted process will use. It now writes through active_config_path(), which llm_admin already did. Pinned end to end, including that the decoy cwd config.yml stays untouched. Metrics (his correction): the walk no longer runs on the calling thread at all — a stale cache starts a single-flight background refresh and the previous numbers are served meanwhile, with the timestamp taken on COMPLETION so a walk longer than the TTL is not stale the moment it finishes. Free space and inodes stay live on every scrape, now asserted rather than assumed. Mutation-verified: removing the validate_action opt-in, the write_file absolute check, the launch-path root, the background thread, or the active-config persistence each fails its test. Gates, all four as CI runs them: 7607 passed / 4 skipped, lint-no-new 0, types-no-new 0, coverage-no-drop findings=0. git diff --check clean.
CI's coverage ratchet caught one line my local run did not: the background usage walk made coverage TIMING-DEPENDENT. Locally the thread happened to land in a way that exercised the single-flight early return; on CI it did not, so executor.py grew a dark line and the gate failed. A test that passes because a thread won a race is not a test. Every branch of the refresher is now driven deterministically, with no reliance on scheduling: the single-flight guard (a refresh already in flight starts no second thread), the __new__ patch seam with no lock (degrades, never raises), an unreadable workspace (no usage reported AND the in-flight flag cleared, so later scrapes still work), and a file that cannot be stat'd mid-walk (counted, not sized, rather than aborting the scan). Gates: 7611 passed / 4 skipped, lint-no-new 0, types-no-new 0, coverage-no-drop findings=0, git diff --check clean.
Found by sweeping the surfaces this change touches instead of waiting for review to name them one at a time. Both are mine, both were shipped-and-silent. 1. THE KNOB WAS UNDOCUMENTED. tools.local_working_dir appeared in neither the tracked config.yml template nor docs/configuration.md, despite CONTRIBUTING requiring config examples stay aligned with reality. An operator could not discover the one setting that governs where commands run — at precisely the moment they would need it, since an unusable workspace takes local commands away. The template now carries it with the reasoning; a test pins the template against the schema so they cannot drift. The default was also spelled twice (field default and blank-normalizer). It is now DEFAULT_LOCAL_WORKING_DIR, once. 2. THE STARTUP REPORT SAID NOTHING ABOUT IT. Local commands fail closed on a bad workspace by design, but the boot diagnostics — the surface an operator actually reads — had no check for it. The symptom would have been every run_command failing with nothing to explain why. There is now a local_workspace check, registered in _CONFIG_CHECKS like every other one, deriving its roots from the SAME shared function the executor uses; a check applying different rules would be worse than none, since it would pass a workspace the runtime then refuses. Diagnostics go from 7 to 8 config checks (deploy output 8/8 -> 9/9). Registering it changed every test that runs the full report, whose config doubles are MagicMocks — a mock attribute is not a workspace. Those doubles now carry a real 0700 directory. That knock-on is the same "new entry -> all consumers" trace that should have preceded the change. Mutation-verified: unregistering the check, or removing the knob from the template, each fails its test. Gates: 7616 passed / 4 skipped, lint-no-new 0, types-no-new 0, coverage-no-drop findings=0, git diff --check clean.
…nt, per-call contract 1. validate_action's opt-in was a blunt instrument: the shared exec callback runs EVERY check type, so http/port/service/process/log probes — fixed command shapes, not raw user text — all became workspace-dependent, and an unusable workspace stopped them executing. Round 9's narrowing says an invalid workspace disables raw user-command routes ONLY. The decision now travels per check from run_bundle: use_workspace=(check.type == "command"). Pinned both ways — with a broken workspace, process/port probes still produce real verdicts while the command check fails closed visibly; with a valid one, command checks run in the workspace as before. 2. Round 10's launch-path protection handled a symlinked config FILE but not a symlinked DIRECTORY earlier in the path: the derivation canonicalized the launch parent, collapsing ws/cfg -> real onto the target, so a workspace at ws/ was accepted and a relative `rm -rf cfg` would remove the component restart.reexec() replays while the canonical target survived. The launch-side parent is now kept LEXICAL (absolutized, symlinks intact), and overlap compares every root under BOTH spellings — lexical and canonical — so ordinary roots behave identically while the alias component is held. 3. My call-site classification test OR-ed a function's calls together, so an opted-in function could gain a second unopted call and stay green — the claimed "a new call site fails until classified" was not true for additions to existing functions. It now records EVERY call individually with three categories (constant opt-in / forwarded decision / none), and each classified function must be uniform. validation._exec and executor._run_on_host are CONDITIONAL: they forward, they do not decide. Blast radius enumerated before the change this time: run_bundle's exec contract has exactly one production call site and five strict test fakes, all updated together with the docstring. Mutation-verified: unconditional opt-in, canonicalizing the launch parent, canonical-only overlap, and an unopted call smuggled into an opted-in function each fail their test. Gates local: 7619 passed / 4 skipped, lint-no-new 0, types-no-new 0, coverage-no-drop findings=0, git diff --check clean. (CI at the pushed SHA verified before review is requested — round 11 rightly called out a local "coverage 0" claim that CI contradicted at the exact head.)
…sage refresh Odin implemented his own round-12 findings (role-flip). Cross-reviewed his commit to full bar: read the whole diff, independently reproduced the port-probe injection before and after his fix, ran all four gates myself, and mutation-tested his three pins (reverting the port-probe fix, the dual lexical/canonical spellings, or the full-Config diagnostic each fails his tests). His work is correct and the port-probe finding is significant — a target like `$(touch marker):1` escaped the nested quoting and executed arbitrary commands. That vulnerability is PRE-EXISTING on master, not introduced by this PR. One regression found in his metrics fix. Publishing the cache and clearing the single-flight flag as one locked transition is right — clearing first lets a scrape start a duplicate walk before the fresh cache is visible. But it also removed the `finally`, so any exception that is not OSError left the flag set forever and every later scrape declined to refresh: usage metrics frozen permanently with no recovery. Reproduced with a non-OSError from os.walk — flag stuck True, no recovery on subsequent scrapes. Both properties are required and both now hold: the success path still publishes cache and flag atomically under the lock, and a `finally` guarded by a `published` sentinel guarantees the flag never survives the thread on any exception without splitting the atomic publish. Pinned by two tests — recovery after an unexpected exception, and cache-present whenever the flag is clear. Gates run independently, not taken from the report: 7630 passed / 4 skipped, lint-no-new 0, types-no-new 0, coverage-no-drop findings=0, diff --check clean.
Cross-reviewed Odin's commit to full bar: read the whole diff, independently reproduced both bug claims, mutation-tested his pins, and ran all four gates myself. Three of his four changes are correct as written and now pinned: - The post-lock freshness recheck is right; the unlocked fast-path observation is not authoritative once lock acquisition blocks. - Broadening the metrics worker to `except Exception` fixes a real defect in MY ee0d68d: a RuntimeError still escaped the daemon thread and surfaced as a PytestUnhandledThreadExceptionWarning in a run I reported as green. I saw "2 warnings" and did not chase it. Now zero occurrences. - write_file's `$(dirname ...)` was unquoted, so an absolute path containing spaces word-split: reproduced the requested write FAILING while `mkdir` created three stray relative directories in the inherited install cwd. Pre-existing, like the port-probe injection. ONE finding on the legacy-config fallback. The fallback itself is the right call — losing every local command on a first upgrade is worse than a private HOME directory, and explicit operator settings are correctly never substituted (verified independently). But its stated safety property does not hold: the packaged unit sets User= and NO Environment=HOME, so HOME comes from the account record and is typically OUTSIDE the install — /home/odin on this real deployment, not /opt/odin as the comment claimed. A broken PACKAGED default therefore falls back rather than rejecting, and with only an INFO log naming the path it was indistinguishable from normal operation: an operator would never learn their packaging was broken. Keeping the behaviour, removing the silence. The comment now states what is actually true. provision_startup_workspace takes an on_fallback callback and records process state; __main__ logs a WARNING naming the failed default, the reason, and the fix command; and the startup diagnostic — the report an operator actually reads — reports "ready at FALLBACK <path>" with the reason and a recommendation, still passing, because a usable fallback is not a failure. Mutation-verified: silencing the fallback callback/state, dropping the diagnostic's fallback branch, reverting his freshness recheck, or reverting his write_file parent quoting each fails its test. Gates run independently: 7636 passed / 4 skipped, lint-no-new 0, types-no-new 0, coverage-no-drop findings=0, diff --check clean, no unhandled-thread warnings.
CI failed at 11c64ba where my local run was green — the precise gap Odin called out two rounds ago, and this time the cause was my test's premise rather than my code. Both new fallback tests assumed "a path whose parent does not exist cannot be provisioned". provision_workspace falls back to `sudo -n install -d`, which creates parent chains, so on a runner with passwordless sudo the path WAS created: the fallback never engaged and the explicit-config test stopped raising. The premise silently evaporated and the tests asserted nothing. They now use a parent that is a regular FILE, which defeats both mkdir and install -d regardless of identity. Verified by running the suite BOTH as the normal user and as root with passwordless sudo — 132 passed each way. That also confirms the six pre-existing "missing parent" tests are safe: they go through resolve_workspace, which has no sudo path. Gates: full suite green, lint 0, types 0, coverage findings 0, diff --check clean.
Calmingstorm
added a commit
that referenced
this pull request
Jul 28, 2026
analyze_pdf and analyze_image read host files as base64 through _exec_command, whose output is truncated at MAX_OUTPUT_CHARS (16,000). Base64 crosses that at roughly 12,000 source bytes, so any ordinary PDF or image on a managed host arrived truncated and died with "Incorrect padding" — meaning the v3.65.1 soak, which used a tiny fixture, had only ever exercised the small-file branch. Reproduced by Odin with a valid 20,853-byte PDF. New ssh.read_binary_file returns raw BYTES with an explicit size bound and no truncation. Locally it reads the file directly (no subprocess at all); remotely it captures ssh stdout as bytes, with `cat --` so a path beginning with '-' cannot become a flag. Oversize is an error rather than a short read, because a partial binary is indistinguishable from a corrupt one. Both handlers now use it, and the base64 decode step is gone rather than left dead. Verified at Odin's exact reproduction size: a 20,853-byte payload round-trips byte-for-byte, and 256-value binary content survives intact — the old path decoded with errors='replace', which silently corrupts non-UTF-8 bytes. Two things worth noting about the tests: - The existing handler tests were coupled to the BROKEN transport (they faked base64 over exec_ret). Repointed to the binary path, plus new pins for a payload larger than the old transport could carry. Several of them were attempting REAL ssh connections to a fake host once the fake no longer matched — one file took 40s in connection timeouts. Now 0.51s. - My own caller-classification contract from PR #239 caught this change: with analyze_pdf and analyze_image no longer calling the command helpers, their table entries went stale and the test failed until they were removed. The guardrail did exactly what it was built for. The remote branch is covered deterministically with a faked subprocess (bytes returned, oversize, stderr surfaced, timeout, launch failure), and the non-atomic stat-then-read window is pinned so a file that grows in between is rejected rather than returning a partial payload. The timeout test closes the coroutine it abandons — an un-awaited-coroutine RuntimeWarning is exactly the noise I have flagged in others' work. Gates: 7683 passed / 4 skipped, lint-no-new 0, types-no-new 0, coverage-no-drop findings 0, diff --check clean, warnings back to baseline.
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
1 participant
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.
What happened
Local user commands (
run_command,run_script,manage_process) inherited the service's working directory — the install root. A bare relative path therefore resolved against the live install.On 2026-07-27 at 00:36, legitimate AE2 jar research ran:
The jar's internal layout starts with
data/, so the cleanup deleted/opt/odin/data— memory, learned lessons, all 12 skills, sessions, trajectories, the knowledge index, and all threecodex_authfiles. Recovered by merge-restore; 33 minutes lost permanently.The intent was legitimate. The relative path was the whole bug.
The bar
Aaron's requirement was specific: close the mechanism without preventing Odin from doing what he was trying to do, and without preventing him from deliberately working inside his own install for reviews and tests.
So the headline test replays the exact incident sequence and asserts it succeeds — extract, read, clean up — while the install survives.
What changed
tools.local_working_dir(default/var/lib/odin-workspace) is validated and passed as the subprocesscwdfor local user commands./var/lib/odin, not a child — packaged installs use that as the live data dir. Not/tmp(tmpfiles ages it out), not$HOME(packaged Odin declares/opt/odinas the service home).PWDandOLDPWDboth normalized.cwd=alone leaves an inheritedOLDPWDpointing at the install, so a barecd -walks right back in.manage_processstays an alternate route to the same incident (29 historical background starts had no explicitcd).Deliberately NOT changed
WorkingDirectorystay/opt/odin— the app resolves./data/...against them, so moving those recreates this incident by another door.cd, absolute paths,git -C <path>— identical behavior. Working inside the install remains possible (pinned by test).Scope honesty
An external cwd prevents the accidental basename collision. It does not confine the process —
.., an absolute path, a symlink placed in the workspace, or archive path traversal can still reach the install. Preventing those needs filesystem confinement and would cost capability.Tests (22 new)
The incident replay uses a synthetic jar containing
data/ae2/...and runs the three commands separately. Safety is structural: the test processchdirs into the fake install first, so a regression in the cwd plumbing destroys the fixture and fails loudly; every path is undertmp_path, asserted before any deletion runs.Also pinned: explicit
cdinto the install works, absolute paths unaffected, relative files persist across commands,cd -cannot return to the install, background processes use the workspace, remote SSH has nocwdparameter, and the legacysrc/odin/toolsDAG surfaces are classified so they can't be wired into the Discord path without reopening this.Mutation-verified load-bearing — each of these fails its tests when reverted:
cwd=plumbing,OLDPWDnormalization, background workspace, inside-install rejection.Gates
ruff clean · mypy clean (231 files) · 7,520 passed / 4 skipped
Deployment note
/var/lib/odin-workspacemust exist, ownedodin:odin, mode0700. It does not exist yet on the live install — do not deploy this without creating it, or local commands will fail closed (by design).Designed with Odin. His review caught the
OLDPWDhole, the/var/lib/odincollision, the background-process route, and the sibling-path requirement.No merge, no deploy — Aaron decides both.