MOO-72 Commit 6: harden secure temporary workspaces and artifact lifecycle - #16
Merged
Merged
Conversation
…cycle WorkspaceManager gains a per-process instances/<bootId>/ namespace + PID lock file, an ownership marker (.codeflow-owned-v1), and sweepStaleWorkspaces() -- process-restart cleanup that only removes other instance directories confirmed not-alive (process.kill(pid, 0)), never a live overlapping instance, and refuses to run at all against a root lacking the ownership marker (protects a misconfigured shared WORKSPACE_ROOT from unrelated-file deletion). The workspace object gains writeFile()/copyTree() as the only sanctioned way to put content into a workspace: an ancestor-walking symlink check (not just the immediate parent), an atomic `wx` exclusive-create write (closing the TOCTOU race and the final-target-is-a-symlink case a realpath-only check would miss), and private file/directory permissions. stagePythonFiles (pyan3Adapter.js) and analyze.js's local-tree copy both route through these instead of their own raw mkdir/writeFile/cp calls -- copyTree rejects (aborts) rather than dereferences or skips a symlinked source tree. No diagnostic-artifact retention mechanism is added -- documented as a deliberate, explicit policy (docs/baseline.md): every diagnostic stays in-memory/response-only, consistent with "avoid retaining private source longer than necessary." Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
OwenTanzer
commented
Jul 29, 2026
OwenTanzer
left a comment
Owner
Author
There was a problem hiding this comment.
Not ready to merge yet: two blocking issues.
…I fixture
The ownership marker was written unconditionally by ensureRoot() before
sweepStaleWorkspaces() ever checked for it -- since server/index.js always
calls ensureRoot() first, the marker was always present by sweep time,
so the "refuse to sweep an unmarked root" protection never actually
triggered in the real startup path. Fixed by having ensureRoot() record
whether the marker already existed *before* this call wrote it
(_rootWasPreviouslyOwned), and gating the sweep on that instead -- a
root's first-ever startup now genuinely never sweeps, only a second and
later startup (once a prior process established continuity of ownership)
does. Added the exact regression test the review asked for: the real,
unmodified ensureRoot()->sweepStaleWorkspaces() order against a
previously-unowned root with a stray UUID-shaped directory.
Also fixed a Linux CI failure: one malicious-path fixture
('..\..\windows\system32\config\sam') isn't actually a traversal on
POSIX, where backslash is just an ordinary filename character -- made it
Windows-only in the test.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…never be swept
The previous fix only delayed the destructive behavior by one restart
cycle: a genuinely foreign instances/<uuid>/ directory (no lock file
inside it at all, so nothing proves CodeFlow ever created it) survived
the first startup (root not yet marked owned) but was deleted on the
second, once the first startup's own ensureRoot() call had written the
root-level ownership marker. The root-level marker only proves "CodeFlow
has run against this root before" -- it says nothing about whether any
specific instances/<uuid>/ directory was actually created by CodeFlow.
Fixed at the source: isInstanceAlive's boolean return (missing/malformed
lock file => "not alive" => removed) is replaced with a three-way
instanceLifecycleState ('alive' | 'dead' | 'unknown'). Only 'dead' (a
lock file naming a PID confirmed via process.kill(pid, 0) to not be
running) is ever removed. 'unknown' (no lock file, or one that doesn't
parse as a PID) is now treated the same as 'alive' -- never touched,
regardless of the root-level marker or how many restarts have happened.
Added the exact regression test requested: two full startup cycles
against a root containing a pre-existing, lock-file-less UUID-shaped
directory, confirming it survives both, not just the first.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
OwenTanzer
commented
Jul 29, 2026
OwenTanzer
left a comment
Owner
Author
There was a problem hiding this comment.
Review complete: both blocking fixes are verified, the final thread is resolved, the full local suite and production build pass, and all GitHub Actions checks are green. Ready to merge.
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.
Summary
WorkspaceManagergains a per-processinstances/<bootId>/namespace (a freshrandomUUID()per process) + PID lock file, and an ownership marker (.codeflow-owned-v1) written once byensureRoot().sweepStaleWorkspaces(): process-restart cleanup. Only removes other instance directories confirmed not-alive viaprocess.kill(pid, 0)— a live overlapping instance (e.g. mid rolling-deploy, or any two processes sharing a root) is never touched. Refuses to run at all against a root lacking the ownership marker, and only ever considers UUID-shaped entries underinstances/— aWORKSPACE_ROOTever misconfigured to point at a shared/non-dedicated directory has none of its unrelated contents touched.createRequestWorkspacegainswriteFile()/copyTree()as the only sanctioned way to put content into it:writeFile(): walks every ancestor path component for a symlink (not just the immediate parent), then an atomicwx(O_CREAT|O_EXCL) exclusive-create write — closes both the TOCTOU race between checking and writing, and the case where the final target itself already exists as a symlink (a realpath-only parent check would miss this entirely). Private file/dir permissions (0600/0700).copyTree()(foranalyze.js's local-tree path, a second workspace-materialization path distinct from pyan3's per-file writes): rejects — aborts the whole copy — if the source tree contains any symlink at all, rather than silently dereferencing or skipping. Recursively tightens permissions on everything copied.stagePythonFiles(pyan3Adapter.js) andanalyze.js'scp()call both now route through these instead of their own rawmkdir/writeFile/cp.docs/baseline.mdas a deliberate policy: every diagnostic stays in-memory/response-only, consistent with "avoid retaining private source longer than necessary" right next to it in the same checklist entry.Test plan
npm test— 672/673 passing (1 expected skip: file-symlink creation isn't permitted in this Windows dev environment without Developer Mode — the equivalent junction-based ancestor-symlink test and thecopyTreesymlink-rejection test both pass for real)WorkspaceManagerand hard-exits without running its owncleanup(), then confirms the next process's real startup sweep removes it.runSharedPyan3Analysiswith a realAbortControllerthat fires almost immediately, confirming the actual execFile-level kill path still leaves the shared workspace cleaned up.sweepStaleWorkspaces(); one naming a confirmed-dead PID (a real spawned-then-exited child) is removed.instances/on an owned root are never touched.tests/server-smoke.mjs) against the actual server process — all 22 steps pass, including the updated "every per-request workspace was cleaned up" assertion (now checking this instance's own namespace is empty, since the root legitimately carries the ownership marker + instances/ for the process's lifetime) and confirminganalyze.js's newcopyTree()path still matches the golden-world baseline exactly (files:6 functions:7).🤖 Generated with Claude Code