Skip to content

Fix what the independent review of the two-node lab found - #11

Merged
hotpixelgroup merged 4 commits into
masterfrom
review-fixes
Oct 12, 2026
Merged

hotpixelgroup merged 4 commits into
masterfrom
review-fixes

Conversation

@hotpixelgroup

@hotpixelgroup hotpixelgroup commented Oct 12, 2026 •

Copy link
Copy Markdown
Owner

Why this follows so closely

The previous pull request (#10) was set to merge when its checks passed, and it did, before an independent review of it had come back. That review found four serious problems and eight lesser ones. This pull request fixes them. Nothing from the earlier change has been released or published.

The fixes were then reviewed a second time before merging, and the second round a third time. Each found gaps; the third and fourth commits respond. All three reports are posted below in full, as advice from a reviewer and not as instructions.

What the first review found and what changes

Found Change How it was checked
Linux sandbox left /run and /tmp visible (session bus, Docker socket, key agents, X server) and shared the operator's terminal session Those places, /var and the other shared folders are replaced by empty ones; every command gets its own session CI now fails unless bubblewrap starts, compiles the node inside it, and runs the real-node tests and sandbox probes inside it
macOS sandbox let confined code ask other programs to act for it and signal any process Launch services, launchd, Apple events and outside signals are denied; shared temporary folders and other disks are hidden; a node on a real network cannot reach local sockets Probed by hand, and by tests that try writes, reads and the network from inside the sandbox
Docker compiled a proposal before review, with the network and the container's AI sign-in in reach A container no longer counts as confinement. Docker is review-first Unit test
"Same as the unmodified node" was decided from src/ alone; build tooling could be edited; a proposal's build could leave things behind in the shared build folder Identity covers every file that can feed the build. Top-level build tooling is refused. A proposal is compiled in a copy of the build folder that is thrown away. The reference node never comes from a proposal's build, or from a build folder that a build of changed source could write to Unit tests, and a real run through the console on macOS
Unusual file names slipped past the protected, consensus and test path checks, because git quotes them Changed paths are read unquoted and NUL-separated, compared without regard to case, and refused unless plain Unit tests
A .gitattributes in a subfolder could hide a change from the reviewer Changes are always shown as text; .gitattributes, .gitignore and .gitmodules are refused at any depth Unit tests
A build that could not produce a block was reported as "No difference observed" New outcome, "The comparison could not be completed". It blocks automatic applying and starting on a shared network Unit tests
A branch that could not be put back after a build was hidden; SIGTERM and SIGHUP were not handled The console reports it and says what to run; both signals behave like Ctrl-C, and tidying up cannot be interrupted Unit test; real run
The agent could read most of the home folder Its read rules cover everything under the home folder except the path to its own workspace. A Claude Code too old to honour the confinement flags is not used Unit test of the rules. Not tried with a live agent
Invisible characters other than bidirectional ones passed the shared-request filter Zero-width, tag, private-use and variation characters are refused, in the page and on the server Unit and browser tests
Stopping a lab run left its two nodes running on macOS Nodes stay in the lab's process group, and the lab stops them Real-node test, confirmed to fail against the old code
A crafted sharing token could break the page until its storage was cleared Token fields are validated before use Browser test
Smaller items The release is a draft until every file is attached. No partial upgrade on Arch. The access key file is private from creation. The installer runs nothing if its download is cut short. The operator's git hooks are not run on AI-edited source Unit tests where practical. The release change runs for the first time when a release is made

What the second review found and what changes

Found Change
A build file under doc/ was outside the "same node" check although the build reads it; a proposal's own build could still become the reference A build file counts wherever it lives; a proposal's build never becomes the reference
On macOS a program started during a build can outlive it and keep permission to write to the build folder The reference is never taken from a build folder that a build of changed source could write to; after the first proposal it is compiled in a folder of its own
A second console started during a build moved the build folder and the branch from under it One console process per lab folder, enforced with a lock taken before any tidying up
The refused-paths check was case-sensitive; a Mac's usual filesystem is not Compared without regard to case; names that are not plain are refused
Showing a binary file's change as text could stop the job Shown with replacement marks
Exit tidying could be cut short; a linked or replaced build folder was mishandled Signals ignored and terminal errors tolerated while tidying up; a linked build folder is refused; whatever sits in the build folder's place is cleared before it is put back
Two nodes that refused nothing and still parted were "no difference" "Could not be completed"
The manual and QA record claimed more than the code did Reworded

What a third, narrower check found and what changes

Found Change
Confined code may write to its build folder, so it could put a link to the home folder in its place; the next build would have been given permission to write there A folder a command may write to is never followed through a link: the sandbox refuses, nothing is compiled in a linked build folder, and a linked scratch folder is replaced
A documentation-only proposal, judged identical, could leave a program from a shared build folder as the node to run when no reference existed Unmodified source from a shared folder is always the reference, compiled apart if need be, for proposals too
The macOS rule against local sockets also blocked name lookups for a node on a real network The lookup service is allowed again, with a test
Smaller items The lab folder's lock is taken before anything is written; a lab folder from an earlier version is treated as shared; two nodes that never connected are "could not be completed"; stopping a job ends its whole process group; two sandbox tests assert what must hold on both systems

Three rounds of review each found something. Review stops here for this pull request; more may remain, and a specialist's attempt to break out of the sandboxes has not been made.

What this does not settle

Stated in the manual and the security policy, so that nobody reads more into the lab than it offers:

  • Both sandboxes start from "everything readable" and hide chosen places. System folders stay readable. On macOS, confined code can read other programs' command lines and environment variables, though it has no network to send them anywhere, and a program that outlives a build could alter a later build of changed source.
  • Compiling runs build scripts and tools from the source being compiled. During a build it is the sandbox, not the review, that protects the machine.
  • Both lab nodes and the script that drives them share one sandbox. A node program written to deceive, as opposed to one that merely differs, could forge a comparison.
  • The agent's limits are enforced by Claude Code, not by the operating system, and it runs with the operator's environment.
  • A shared request that needs invisible joiners (some scripts, some emoji) is refused and has to be retyped.
  • A live AI session has still not been run. The release workflow, the Mac app and notarisation are untested until a release is made.

Cost and effort

Cost: TBD (no rate card applies; the work is already done). Effort: L, on a scale where sizes are relative to each other and not to a calendar.

🤖 Generated with Claude Code

hotpixelgroup and others added 2 commits October 12, 2026 12:05
An independent review of the previous change found ways the lab's checks
could be bypassed or its claims overstated. This fixes them.

Confinement
- Linux: hide /tmp, /run and the other places that hold local service
  sockets, not only the home folder; start every command in its own
  session, so it cannot type into the operator's terminal.
- macOS: deny asking other programs to act (launchd, LaunchServices,
  Apple events) and signalling anything outside the sandbox.
- A container is no longer counted as confinement. Inside Docker,
  AI-written build scripts would have the network and the container's AI
  login, so nothing is compiled there until it has been reviewed.

What "unmodified" means
- Two revisions are the same node only if everything that feeds the
  build matches, not just src/.
- A proposal may not edit top-level build tooling, anything under
  contrib/, or any .gitattributes, .gitignore or .gitmodules.
- A proposal is compiled in a copy of the build folder that is thrown
  away afterwards, so nothing it leaves behind reaches a later build.
- The reference node is never taken from a build folder that has
  compiled changed source.

Checks that could be sidestepped
- Changed paths are read NUL-separated and unquoted, so an unusual file
  name cannot slip past the protected, consensus or test path checks.
- Changes are always shown as text, whatever a .gitattributes says.
- A comparison in which a check could not run is "could not be
  completed", never "no difference", and blocks automatic applying and
  starting on a shared network.
- The operator's git hooks are not run on AI-edited trees.

Robustness
- If the operator's branch cannot be put back after a build, the console
  says so and shows the real state instead of hiding it; SIGTERM and
  SIGHUP are handled like Ctrl-C.
- Stopping a lab run stops its nodes.
- Shared requests containing zero-width, tag, private-use or variation
  characters are refused, on the server as well as in the page; a
  malformed sharing token can no longer break the page.
- The agent's read rules cover everything under the home folder except
  the path to its own workspace; a Claude Code too old to honour the
  confinement flags is not used.
- The access key file is created private; the release is published only
  once every file is attached; the installer runs nothing if its
  download is cut short.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…over

Bring the manual, the security policy, the README, the landing page and
the Docker comments in line with the review fixes:

- Docker is review-first: a container is not a sandbox around each build.
- What a proposal may not change, and why build files always wait.
- "Unmodified" means everything that feeds the build; a proposal is
  compiled in a throwaway copy of the build folder.
- The new outcome for a comparison that could not be completed.
- Limits stated plainly: compiling runs code from the source, both lab
  nodes share one sandbox so a program built to deceive could forge a
  result, and the agent's read limits are Claude Code's own.

Record the independent review in QA.md, with how each fix was checked
and what is still unchecked.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@hotpixelgroup

Copy link
Copy Markdown
Owner Author

Independent review of the previous pull request (#10), posted for the record.

This is the report of a second reviewer (an AI reviewer that had not written the code) asked to read the change adversarially. It is advice, quoted as received, not instructions to anyone. Line numbers refer to the code as merged in that pull request. Every item below is addressed by this pull request; the table in the description says how.


Independent review of PR #10 (lab-hardening)

I read every file you listed in full. I read app.js and import.js for rendering and key handling, and the tests README and doc/vibes.md only to compare against claims. I ran nothing. Where an item rests on OS behaviour I did not execute, I say so and name the check that would settle it.

Paths below are relative to the repository root. BV = contrib/vibes/bitcoin-vibes, SB = contrib/vibes/vibeslib/sandbox.py, LR = contrib/vibes/lab_runner.py.


HIGH

H1. The Linux sandbox shows the whole host to confined code, including the sockets that allow escape (SB:211-232).

  • Defect: bwrap ... --ro-bind / / makes everything outside $HOME visible. --unshare-net removes IP networking but not filesystem AF_UNIX sockets. There is no --tmpfs /run, --tmpfs /tmp or --new-session.
  • Scenario: a proposal's build script or test binary connects to one of these:
    • /run/user/$UID/bus: the D-Bus session bus. systemd-run --user or StartTransientUnit then runs a command outside the sandbox.
    • /var/run/docker.sock: root for anyone in the docker group.
    • /run/user/$UID/gnupg/S.gpg-agent or the keyring ssh-agent socket.
    • /tmp/.X11-unix/X0: keystroke injection.
  • Related, medium: run() (BV:384-394) has no start_new_session and does not close stdin. It is used for ask_node_program (BV:1581, runs the AI-built bitcoind -help and -version) and test_bitcoin --list_content (BV:2221). With no --new-session, a program started from a console running in a terminal can use TIOCSTI to queue commands into the operator's shell. The _stream and run_lab paths do call setsid, so they are not affected.
  • To confirm: check whether /run/user/UID/bus exists in a desktop session, and whether the kernel has dev.tty.legacy_tiocsti=1. The generated bwrap argv shows the exposure directly.
  • Fix: bind only the system directories a build needs. Add --tmpfs /run, --tmpfs /tmp, --new-session, and pass stdin=DEVNULL plus start_new_session=True in run().

H2. The macOS profile is a deny-list on (allow default) (SB:182).

  • Defect: only file-read under $HOME, file-write and network are denied. Process exec, Mach lookups, signals, pasteboard and sysctl stay open.
  • Scenario: confined code runs /usr/bin/open -a Terminal <build>/x.command or launchctl submit. LaunchServices and launchd are Mach services, which the profile does not restrict, so the command runs unconfined.
  • Certain by construction: confined code can signal any process of the same user, including the console and the operator's other programs.
  • Uncertain: whether the open route runs end to end on current macOS. Run the generated profile around open to find out.
  • Fix: base the profile on (deny default) with explicit allowances, or at least add (deny mach-lookup) with a short allow-list and (deny signal (target others)).

H3. Docker mode compiles hostile code before review, with network access and the Claude login readable (Dockerfile:31-47, docker-compose.yml:22, SB:78-79 and 248, BV:1794).

  • Defect: with VIBES_SANDBOX=container, wrap() returns the bare argv and contained is true. A proposal is therefore built before any review, as the same uid that owns /home/vibes/.claude, with the network open.
  • Scenario: an imported request leads the agent to add execute_process(curl ... -d @$ENV{HOME}/.claude/.credentials.json) to a CMake file. The pre-review build runs it. The review screen appears only after the credentials are gone.
  • The compose comment says the agent never sees the host's ~/.claude. That is true, but the build code sees the container's login.
  • Fix: run build, test and node as a separate unprivileged user that cannot read .claude or .vibes/key. Alternatively, in container mode, do not build imported proposals before review. State the credential exposure in doc/vibes.md.

H4. "Same rules as the unmodified node" is decided from src/ alone, and the build directory is shared and persistent.

  • Evidence:
    • identical = src_tree(sha) == src_tree(stock_rev()) at BV:2147. When true, the comparison is skipped (BV:2162-2164) and the build is stored as the reference node by _capture_reference (BV:1492-1510).
    • PROTECTED_PATHS (BV:109-110) does not include CMakeLists.txt, cmake/, depends/, share/, .gitignore or .dockerignore.
    • The agent prompt (BV:65-67) tells the agent "the console refuses any proposal that does" edit build tooling. The console does not.
    • Every proposal builds in the same repo/build (BV:2119, 2128-2134), which confined build scripts can write.
  • Scenario 1: a typed request pasted from a hostile source makes the agent add add_compile_options(-include ${CMAKE_SOURCE_DIR}/share/x.h) to the root CMakeLists.txt. x.h redefines a consensus constant. src/ is byte-identical to stock, so the verdict is "identical" and no lab run happens.
    • _review_reason (BV:1810-1830) returns "" for "identical", so auto mode applies it.
    • If the reference is missing or stale, the modified binary becomes the reference node for every later comparison. This happens even when the proposal is later discarded, because _build_proposal calls _build, which captures the reference.
    • On mainnet, activation_block (BV:2299-2318) allows "identical".
  • Scenario 2: a discarded proposal's build leaves a poisoned CMakeCache.txt, compiler setting or .ninja_log in build/. The next build of unmodified source reuses it. That build is labelled stock, captured as the reference and promoted.
  • Fix: protect build tooling at any depth. Define identity over the whole tree except an allow-list of doc paths. Build each proposal in a fresh directory such as .vibes/builds/<job>, never in repo/build. Build the reference only from a clean checkout, as reference_runner already does.

MEDIUM

M1. The protected-path, consensus and test gates are bypassed by git's path quoting (BV:1751-1757, 1764, 1770-1772, 1832-1835).

  • Defect: git diff --cached --numstat without -z quotes paths containing non-ASCII bytes, ", \ or control characters. A path comes out as "contrib/vibes/\303\251.x", which does not start with contrib/vibes/.
  • Scenario: the agent writes .github/workflows/é.yml, .claude/commands/é.md or .claude/agents/é.md.
    • _is_protected returns false, and sensitive and tests_touched are also missed.
    • In auto mode with an identical verdict, the file is committed unreviewed. A workflow runs with the repo's secrets when the branch is pushed. A .claude/ command or agent definition loads the next time the operator runs Claude Code in the repo.
    • doc/vibes.md says such proposals are refused.
  • Fix: use git diff --cached --numstat -z --no-renames (and -c core.quotePath=false) and parse NUL-separated fields.

M2. A subdirectory .gitattributes with -diff hides the change from the reviewer (BV:1745, 1299, 3115-3124, PROTECTED at 110).

  • Defect: only the root .gitattributes is protected, and the diffs are generated without --text.
  • Scenario: the agent adds src/.gitattributes containing * -diff. The stored patch, the review screen and /api/diff show "Binary files differ" for the real source edits. _judge_consensus sees no text. The path-based sensitive flag still fires for files under src/consensus/ and similar, but not for other paths such as src/net_processing.cpp or non-src/ tooling.
  • Fix: pass --text --no-ext-diff --no-textconv to every diff and show, and refuse .gitattributes, .gitignore and .gitmodules at any depth.

M3. The lab can say "No difference observed" about a node that cannot mine (LR:410-416, 473-488; BV:1827-1829).

  • Defect: if the candidate's generatetoaddress returns an RpcError, the code adds a row with status "error" and sets hashes = []. finish() ignores row status, so the verdict is "none" or "policy" and the headline is "No difference observed".
  • Scenario: a rule change that the candidate's own block assembler violates, so TestBlockValidity fails. _review_reason lets "none" through to auto-apply, and activation_block lets it start on mainnet.
  • Fix: treat any "error" row as a distinct verdict such as "inconclusive", and have _review_reason and activation_block fail closed on it.
  • Design limit worth stating in the docs: the harness and the candidate run in one sandbox, as one uid, in one PID namespace on Linux. A candidate can forge RPC answers and can reach /proc/<harness pid>/fd/1 to inject a final {"t":"result"} line. The console keeps the last one. The comparison detects accidental rule changes, not a deliberately deceptive node.

M4. The operator's repo can be left on the AI commit with a UI that says it is clean (BV:1887-1898, 3497-3502, 683-688).

  • Defect:
    • _restore_checkout runs a plain git checkout <branch>. On failure it prints to stderr and leaves the marker.
    • status() then reports the marker's branch with dirty = "", hiding the detached HEAD.
    • Ctrl-C calls cancel_job() and stop_node() and exits. The job thread is a daemon, so its restore may never run.
    • SIGTERM and SIGHUP are not handled.
  • Scenarios:
    • The operator saves a file while a 30-minute build is running (the build checks the proposal out in their working tree). The restore fails and the operator keeps working on a detached HEAD at a "vibe:" commit.
    • The operator presses Ctrl-C mid-build. The tree stays detached until the next launch.
    • A proposal that edits .gitignore and tracks a file under build/ that the build then modifies also makes the restore fail.
  • Fix: make the restore join the job thread on exit, handle SIGTERM and SIGHUP, surface a failed restore as a job error and a UI banner, and base the status on real git output.

M5. The agent runs unsandboxed, with the full environment and a deny-list for reads (BV:96-104, 1943-1952, 1981).

  • Defect: Read, Glob and Grep are limited only by a list of about 25 credential paths. ~/.zshrc, ~/.zprofile, ~/.env, other repos' .env files, ~/.config/* except gh and gcloud, and browser profiles other than Chrome and Firefox are not on it.
  • Scenario: a hostile request tells the agent to paste the exports from ~/.zshrc into a comment. In auto mode with an identical verdict that is committed. In review mode the operator must notice it in the diff.
  • To confirm: ask the real agent to read ~/.zshrc with this exact command line. The tests only use a stand-in claude.
  • The --safe-mode flag needs Claude Code 2.1.169 or later and is not version-checked. I could not confirm whether it leaves the --settings deny rules in force, so test that too.
  • Fix: run the agent under the same seatbelt or bwrap wrapper, with the network allowed and reads limited to the workspace.

M6. The "hidden characters" filter misses the usual smuggling characters (app.js:1002).

  • Defect: it blocks C0 controls and bidi overrides only. Unicode tag characters (U+E0000-E007F, surrogate pairs in this non-u regex), zero-width characters (U+200B-200F, U+2060-2064, U+FEFF), U+00AD and variation selectors pass.
  • Scenario: a shared request shows the operator a harmless sentence. The agent receives invisible extra instructions.
  • Fix: use the u flag and reject \p{Cf}, \p{Co} and the tag and variation-selector ranges. Do the same check server-side in /api/vibe.

M7. Cancelling or timing out a lab run orphans the nodes on macOS (LR:128-134, BV:1401-1409, 1669-1670).

  • Defect: LabNode.start uses start_new_session=True. _kill signals only the lab_runner's process group, and lab_runner has no SIGTERM handler, so its finally does not run.
  • Scenario: the operator presses Cancel, or the 600 s timer fires. Two bitcoind processes, one of them AI-written, keep running, holding ports and writing into a deleted work folder.
  • Linux is unaffected because of the PID namespace.
  • Fix: handle SIGTERM in lab_runner and stop the nodes, or kill by sandbox-wide process group and parent chain.

M8. A crafted shared token can break the console page persistently (app.js:359, 1020-1022, 1251-1252).

  • Defect: value.f.slice(0, 12) keeps unvalidated elements, and renderImportedNote runs sender.f.map(file => file[0]).
  • Scenario: a token whose payload has "f":[null] throws a TypeError. Once the draft is saved, the same exception is thrown at top level on every page load, before refresh() and the polling timer start. The page stays dead until localStorage is cleared.
  • Fix: validate the shape of f and c, and wrap the render in a try/catch.

LOW

  • release.yml:353-359 vs the claim at lines 12-14, "either complete or absent": action-gh-release creates a non-draft release and then uploads assets. A failed upload leaves a published, partial "latest". Use draft: true and publish after all assets are verified.
  • install.sh:122: pacman -Sy --needed --noconfirm is a partial-upgrade on Arch. Use -S after a documented full upgrade, or tell the user.
  • BV:476-478: the key file is written with the default umask and then chmod'ed, and .vibes is created 0755. This is a short window of world-readable key on a multi-user machine. Use os.open(..., 0o600) and a 0700 directory.
  • BV:3008: hmac.compare_digest raises TypeError on a non-ASCII ?key= or header. The connection is dropped with a traceback. It does not bypass auth. Compare bytes.
  • SB:114-118: the bwrap text says "a private network", but network="local" for the operator's regtest node leaves the network open (SB:214-219). doc/vibes.md discloses this, but the in-app text does not.
  • BV:1785: the proposal commit and git add -A in the workspace run the operator's own git hooks on AI-edited trees. This only matters if the operator has repo-relative hooks installed.
  • install.sh:498-559: there is no main() wrapper, so a truncated curl | sh runs a prefix of the install steps.

Checked and found sound

  • Every dynamic string is rendered with textContent; there is no innerHTML anywhere in web/.
  • The CSP and nosniff headers are set on all _send responses.
  • The key never appears in app.js, and the cookie is HttpOnly and SameSite=Strict.
  • POST routes check both Origin and Host against loopback.
  • GET routes need the key, so DNS rebinding reads only public pages (/api/ping, /import, /web/import.js).
  • sha and job IDs are regex-validated before reaching git.
  • Archive extraction in the console rejects links, devices, absolute paths and ...
  • The imported-request review is enforced on the server whenever the page says "imported". The page sets that flag consistently, including on Refine and Repair.
  • The .git pointer tamper check and the PROTECTED check for ASCII names work.
  • The pinned action SHAs resolve.

The three things to fix before public release

  1. Close the confinement gaps. The Linux root bind with /run and /tmp sockets and no --new-session, the macOS (allow default) profile, and the Docker build that runs before review as the user who owns the Claude login (H1-H3). Until then, soften "no network, no view of your home folder" in the README.
  2. Make the review and identity gates real. Define "identical" by the whole tree, use a fresh build directory per proposal, protect build tooling, and parse changed paths with -z and --no-renames. Pass --text to all diffs (H4, M1, M2).
  3. Fail closed. An inconclusive comparison must never read as "No difference observed" or permit auto-apply (M3). A failed or skipped checkout restore must be visible and recoverable rather than hidden by the status marker (M4).

A second independent review, of the fixes themselves, found that several
did not fully close what they set out to close.

- A build file under doc/ was outside the "same node" check although the
  build reads it. A build file now counts wherever it lives.
- A proposal's own build could still become the reference node, and on
  macOS a program started during a build can outlive it and keep its
  permission to write to the build folder. The reference is now taken
  from the operator's build folder only while no build of changed source
  has ever been able to write there; after that it is compiled in a
  folder of its own. A proposal's build never becomes the reference.
  Whatever a confined command leaves in its process group is ended when
  the command ends.
- A second console started during a build moved the build folder and
  the branch from under it. One console process per lab folder, with a
  lock taken before any tidying up.
- The refused-paths check was case-sensitive; a Mac's usual filesystem
  is not. File names that are not plain are refused.
- A --text diff of a binary file raised a decoding error.
- Tidying up on exit could be cut short by a second signal or a closed
  terminal; a linked or replaced build folder was mishandled.
- Two nodes that refused nothing and still parted were "no difference".
- macOS: shared temporary folders and other disks are hidden; a node on
  a real network cannot reach local sockets such as a key agent's.
- Linux: /var is hidden too; tools installed in any hidden place are
  shown again. CI now fails unless bubblewrap starts, compiles the node
  inside it, and runs probes from inside the sandbox.

The manual, the security policy and the QA record are corrected where
they claimed more than the code does, and now state what the sandboxes
do not hide.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@hotpixelgroup

Copy link
Copy Markdown
Owner Author

Second independent review, of the fixes in this pull request (first two commits), posted for the record.

A second AI reviewer, which had not written the code, was asked to check whether each earlier finding was really closed and to look for new defects. It read the code and ran nothing. This is its report as received: advice, not instructions. The third commit on this branch responds to it; the description says how.


REVIEW OF review-fixes (5b168ab..187de65). Nothing was run; everything below is from reading.

EARLIER FINDINGS

  • H1 partly. bwrap_args hides /run, /tmp, /home and adds --new-session, but still starts from --ro-bind / /. Path sockets elsewhere (snap LXD's /var/snap/lxd/common/lxd/unix.socket) stay reachable, and the operator's node (no --unshare-net) keeps abstract sockets such as X11.
  • H2 partly. MAC_LOCKDOWN closes LaunchServices, launchd, Apple events and signals. The base is still (allow default): sysctl/process-info (other processes' environments) and reads of /private/tmp, /var/folders, /Volumes remain.
  • H3 closed. Sandbox.contained is false for a container; vibe_runner skips _build_proposal and _review_reason forces review.
  • H4 partly. See 1, 2, 3, 7.
  • M1 partly. _parse_numstat with -z closes the quoting hole. _is_protected is case-sensitive (see 4). _judge_consensus still parses the diff --git header, which git quotes for names with quotes or tabs.
  • M2 closed. DIFF_AS_TEXT is on all five diff/show calls; the three names are refused at any depth.
  • M3 closed. finish(), _review_reason, activation_block and app.js fail closed. Residual: 9.
  • M4 partly. The banner and retry in status() work; shutdown has holes (2, 6).
  • M5 partly. Home is covered. _run_agent still passes the full environment, and outside home the agent reads anything (Linux: /proc/self/environ). Untested with a live agent, as QA.md says.
  • M6 closed. Side effect: ZWJ/ZWNJ are refused, so emoji sequences and Persian or Indic text are rejected.
  • M7 closed. Nodes share lab_runner's process group and its handler kills them.
  • M8 closed. cleanSender runs on import, load and render.
  • Lows: all seven closed (the release step by reading only). install.sh's top level now holds only assignments and definitions. GIT_SAFE is on every console git call.

NEW OR REMAINING, RANKED

  1. HIGH. _affects_node / NOT_THE_NODE / _build.

    • Scenario: doc/ is outside the identity, yet CMakeLists.txt:645 runs add_subdirectory(doc) before src, and doc/CMakeLists.txt is not protected. A proposal editing it (for example setting CMAKE_CXX_FLAGS in PARENT_SCOPE to force-include a header under doc/) is judged "identical". No lab run happens, unit tests are treated as stock, and _build calls _capture_reference in the pre-review build. That binary becomes the reference when none is current, even if the proposal is discarded. It is flagged as a build file, so it is not auto-applied.
    • Fix: make _affects_node true for every _is_build_file path, and never capture the reference when promote=False.
  2. MEDIUM. Console.__init__ / main().

    • Scenario: recovery (_restore_build_folder, _restore_checkout, deleting build-discard-*) runs in the constructor, before main() discovers a console is already running for this repo. A second ./vibes during a proposal build puts the trusted folder back at build/ under the live confined build. That build keeps writing there with no marker (in_copy was read earlier), and the branch is switched.
    • Fix: take an exclusive lock on .vibes before any recovery.
  3. MEDIUM, macOS. _restore_build_folder.

    • Scenario: the seatbelt write rule is the path repo/build, and nothing ends confined processes before the trusted folder returns to it. A process that detached during the build can then write into it. That could come from an edited src CMakeLists.txt, or from mpgen, which is compiled from src/ipc/libmultiprocess and run by the build (ENABLE_IPC defaults ON).
    • Also: the same rule lets it replace build/ with a dangling symlink. kept.rename(build) then raises in every job and in __init__, so the console cannot start.
    • Fix: build the reference only in a folder no proposal was granted, as reference_runner does, and handle a non-directory build/.
  4. MEDIUM, macOS. _is_protected.

    • Scenario: it compares case-sensitively on a case-insensitive filesystem. .Claude/settings.json, Claude.md, .MCP.json and src/.GitAttributes pass (none exists in the tree). In auto mode with verdict "none" they are applied unreviewed.
    • Fix: casefold first.
  5. MEDIUM. run() with DIFF_AS_TEXT.

    • Scenario: run() decodes strictly and --text now emits raw bytes. A proposal that overwrites any tracked binary (94 under src, doc, test) raises UnicodeDecodeError in wg("diff"), again inside _run_job's finally (skipping cleanup and _save_job), and in /api/diff.
    • Fix: errors="replace".
  6. MEDIUM/LOW. main() shutdown.

    • Scenario: print to a closed terminal raises OSError, so SIGHUP cleanup is skipped; a second signal aborts it. After join(30), the main thread's restore can overlap _set_build_folder_aside still in cp, whose failure branch shutil.rmtree(build) then deletes the restored folder.
    • Fix: one lock around both functions; ignore signals and OSError during cleanup.
  7. LOW. Marker logic in _build_proposal / _build.

    • "Compile current source first" uses _compile and never writes build-compiled-a-change when HEAD has applied changes.
    • _build clears the marker while in the copy.
    • A symlinked build/ is copied as a link, so the proposal compiles in the real folder.
    • No test touches these branches.
  8. LOW, Linux. _readable.

    • Scenario: it re-exposes toolchains only under self.home, but all of /home is now hidden, so Homebrew at /home/linuxbrew disappears.
    • Fix: compare against _hidden_roots().
  9. LOW. finish(). Nodes that never connect or do not converge, without a shuttle rejection, still yield "none".

DOCS

  • doc/vibes.md: "nothing … can reach a later one", "never taken from a build folder that has compiled changed source" and "everything that feeds the build" exceed the code (1, 2, 3, 7).
  • QA.md: "All were fixed" is not accurate. It cites "unit tests" for the reference rule, and none exist. It says Linux "runs in CI", but vibes.yml passes when bubblewrap is unavailable and no test compiles under it.

VERIFIED VS INFERRED

  • Read in code: 1, 2, 5, 7, 8, 9 and the checks behind 4.
  • Inferred from operating-system behaviour, not run: 3 (the subpath rule covers the folder itself; detached processes keep the profile), 4 (APFS opens case variants), 6 (EIO on print), the LXD socket path, and sysctl under H2.

A third, narrower check of the previous commit found two more problems
of substance.

- Confined code may write to its build folder, so it could put a link
  to the home folder in its place, and the next build would have been
  given permission to write wherever the link pointed. A folder a
  command may write to is now never followed through a link: the sandbox
  refuses, nothing is compiled in a linked build folder, and a linked
  scratch folder is replaced by a real one.
- A documentation-only proposal, judged identical, could leave a program
  from a shared build folder as the node to run when no reference
  existed. Unmodified source from a shared folder is now always the
  reference, compiled apart if need be, for proposals too.

Also: with the network open, the macOS rule against local sockets
blocked the system's name lookups, so a node could not find its first
peers by name; the lookup service is allowed again. The lab folder's
lock is taken before anything is written to it. A lab folder left by an
earlier version is treated as shared. Two nodes that never connected
are "could not be completed". Stopping a job ends its whole process
group. Two sandbox tests now assert what must hold on both systems.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@hotpixelgroup

Copy link
Copy Markdown
Owner Author

Third check, of the second-round commit only (1068443), posted for the record.

A narrower check by an AI reviewer that had not written the code. It read the commit and ran nothing. This is its report as received: advice, not instructions. The fourth commit on this branch responds to it. This was the last round of review before merging; three rounds each found something, so more may remain, and the description says what is known to be open.


High/medium remain: yes, two medium (N1 is high on macOS with automatic applying). Nothing was run; all from reading commit 1068443.

FINDINGS 1-9

  1. Closed. _affects_node returns True for any _is_build_file; the unmodified doc/CMakeLists.txt only feeds Doxyfile.in to the docs target; _build captures only when promote and not shared. Residual: N2.
  2. Partly. _claim_lab (flock, taken before recovery) gates tidying and main() exits. Two owners only where flock is unsupported or the lock file is deleted. But __init__ still rewrites runtime/ before claiming (N5).
  3. Partly. The reference never comes from build/ once _note_build_folder_shared has run (written before every compile of changed source, never cleared). _restore_build_folder handles links and files and catches OSError. A swapped-in link is still followed by later builds (N1).
  4. Closed. casefold in _is_protected and _is_build_file; _is_plain_name (no tracked file violates it).
  5. Closed. run() uses errors="replace".
  6. Closed. build_folder_lock, SIG_IGN, tell(). Low residual: shutdown waits uninterruptibly on a cp holding the lock.
  7. Partly. Marker order fixed, tests added. The link check runs once in _build_proposal; _set_build_folder_aside returns silently on a link; _compile never checks.
  8. Closed as specified; probably ineffective for Linuxbrew (N7).
  9. Partly. Non-convergence is now inconclusive; never-connected nodes with equal tips (from shuttled blocks) are still "none".

_reap: no realistic stray hit. The group id stays reserved while any member lives; otherwise it needs a full PID wrap between wait and killpg.
CI steps: should work. Workspace is re-bound read-only, build/ bound writable, ccache falls onto an empty cache in the tmpfs over /home. They prove a one-file rebuild and relink with default configure arguments, not the console's command line.

NEW OR REMAINING
N1. Medium (high on macOS, automatic mode). sandbox.py seatbelt_profile, _compile. Inferred (seatbelt subpath covers the folder itself); missing check read in code. Write paths go through _real(). Confined code, or a survivor, replaces repo/build with a link to the home folder. The next _build(promote=True) (Build, update, revert) grants read and write there, and _secrets stops hiding .vibes/key because it sits inside a write grant. Code of an applied change (mpgen) then runs with the home folder. Fix: before every confined command granted build/, refuse unless it is a real directory at repo/build.
N2. Medium. _build, _apply_proposal. Read in code. Proposal with identical node_key (doc-only), folder shared, reference not ready (baseline moved, not rebuilt): promote=False skips _build_reference_apart, the release keeps the copy-built bitcoind, verdict "identical", _review_reason returns "", _promote(candidate), and activation_block allows shared networks. Fix: build the reference apart regardless of promote, or record "not compared" and do not promote.
N3. Low-medium, macOS. seatbelt_profile network="full". Inferred. Denying all unix-socket connects also blocks /var/run/mDNSResponder, which name lookups use: DNS seeds and -addnode by name fail. Fix: allow that path after the deny; add a lookup probe to RealSandboxTests.
N4. Low-medium. RealSandboxTests.test_writes_stay_inside_the_folder_it_was_given. Inferred. On Linux hidden roots are writable tmpfs; where ~/.gitconfig is re-bound, the home write succeeds inside and assertNotEqual(returncode, 0) fails. CI passes only if /home/runner has none. Fix: assert only that the file is absent outside.
N5. Low. Console.__init__. Read. A non-owner overwrites runtime/lab_runner.py and deletes and recopies runtime/experiments. Fix: claim first.
N6. Low. Read. Labs made on master compiled proposals in build/ with no marker; _adopt_installed_reference and wants_reference trust that folder. Fix: write the marker when any job record exists.
N7. Low. _readable. Inferred. realpath lands in Cellar/tool/version; brew's ld.so and libraries under .linuxbrew/lib stay hidden.
N8. Low, macOS. _kill, _reap. Read. _kill returns once the leader has exited and _reap runs only after stdout closes, so a child holding stdout stalls the job uncancellably.

DOCS

  • SECURITY.md, doc/vibes.md: the reference claims hold. They omit N1, and that a survivor can alter later builds of applied changes.
  • QA.md: "compiles the node inside it" is one file; Linuxbrew and a real-network node on macOS are unverified. The code comment "never taken from such a folder" exceeds the code (N2).

What the fourth commit does about it: N1, N2, N3, N4, N5, N6, N8 and the rest of 9 are fixed, with tests. N7 (Homebrew on Linux) is stated as a limit, with VIBES_SANDBOX_READ as the way round it. The manual and security policy now say that a program outliving a build could alter a later build of changed source.

@hotpixelgroup
hotpixelgroup merged commit f653412 into master Oct 12, 2026
3 checks passed
@hotpixelgroup
hotpixelgroup deleted the review-fixes branch October 12, 2026 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant