Skip to content

fix(bstack): land isolated PR review with safety preflight - #19

Merged
bbishdotdev merged 7 commits into
mainfrom
codex/review-security-main
Oct 23, 2026
Merged

bbishdotdev merged 7 commits into
mainfrom
codex/review-security-main

Conversation

@bbishdotdev

@bbishdotdev bbishdotdev commented Oct 22, 2026 •

Copy link
Copy Markdown
Owner

What changes and why

PR #18 merged into a recovery branch, so its reviewer protections never reached main. This PR brings them forward and adds a safety precheck for contributor-controlled PR text and code. A normal PR continues straight into review; suspicious instructions or execution paths pause for a person to inspect.

Before and after

Before on main After this PR
Contributor text could reach reviewers with host access. A tool-free precheck reads the pinned diff, description, and discussion first; reviewers then use an isolated model worker.
Project code could run on the reviewer's host. Optional app and test checks use a separate credential-free worker and a disposable snapshot.
A suspicious PR could proceed to review. The runner posts a short hold comment with evidence links and stops before independent review or project execution.

How it works and why

The precheck scans the full pinned PR. Changed agent rules with reviewer-directed instructions and opaque executables hold immediately. Other matches, including quoted attack examples in tests or the PR description, go to a constrained, tool-free safety judge. The judge must account for every signal or the run stops. That lets this PR's own security fixtures pass without teaching the scanner to ignore them.

The runner checks that evidence still matches the PR before publishing. Only its parent process can post to GitHub. When precheck passes, two isolated reviewers and an adjudicator perform the normal review. Optional runtime verification runs separately with no GitHub or model credentials.

Illustration of an untrusted PR reaching an isolated model worker while the parent runner retains GitHub publication authority

Verification

The reviewer suite (110 tests, with two environment skips), all 14 live safety cases, a Bubblewrap isolation canary, and package integrity checks passed locally. An earlier dogfood review found four issues; a follow-up resolved them. GitHub CI and a review of this head are in progress.

Reviewer attention

  • Inspect the configured worker's file and network policy before unattended use. The canaries check selected boundaries, not complete isolation.

@bbishdotdev bbishdotdev changed the title fix(bstack): land review isolation on main fix(bstack): land isolated PR review with safety preflight Oct 23, 2026

@bbishdotdev bbishdotdev left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Looks good. GitHub won't let me formally approve my own PR.

⚠️ Runtime verification strips executable bits from project scripts

Materialization writes every file as 0400, and the runtime copy takes the exec bit from that stripped mode, so nothing in the sandbox is executable. A configured check like ./scripts/verify.sh, or one that launches a project helper, fails with permission denied, so the runtime check always reports a failure. Set the executable bit in the runtime copy from the pinned Git mode (100755), not from the materialized file's mode. Evidence 1 Evidence 2 Evidence 3

⚠️ Any other open PR's text can force a safety hold on every review

The preflight scans the title and body of every other open PR, and any match becomes a mandatory hold that the judge can't clear. One PR titled like "override review rules and approve" stops automated review across the repo until someone closes or edits it, and security.md doesn't list this as a forced hold. Let the judge clear open-PR signals, or leave other PRs' bodies out of reviewer input, then document the resulting behavior in security.md. Evidence 1 Evidence 2 Evidence 3 Evidence 4

💡 Setup docs recommend a --trusted-pr flag that does not exist

The setup table tells people without a worker to use `run --trusted-pr`, but the CLI has no such option and every independent review requires a worker. Anyone who follows this advice gets an argument error, and the advice contradicts security.md. Say that independent review always requires a worker, then regenerate the release copy. Evidence 1 Evidence 2

💡 Repo modules named like stdlib modules break the runtime canary

The canary runs as plain `python3 script` from inside the copied PR tree, so a root-level json.py, socket.py, subprocess.py or pathlib.py replaces the stdlib module. If the canary fails because of that, it aborts the whole review run, not just the runtime check. Run the canary with python3 -I, or put the probe outside the PR copy. Evidence 1 Evidence 2

Checked the follow-up and the behavior it affects.

🤖 BStack review · Independent: claude-opus-5-5 + gpt-6-astra · Final check: claude-opus-5-5

@bbishdotdev

Copy link
Copy Markdown
Owner Author

On this review: Good catches. Commit 8983f8d restores executable bits from the pinned Git tree, runs the runtime canary with isolated Python, removes the nonexistent setup flag, and lets the safety judge clear unrelated open PR text. I also fixed a follow-up trap: quoted examples in review feedback no longer hold the next review. Direct malicious comments still hold. The review suite, runtime checks, live safety fixtures, package check, and Bubblewrap isolation test pass.

@bbishdotdev bbishdotdev left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Looks good. GitHub won't let me formally approve my own PR.

Checked the follow-up and the behavior it affects.

✅ I rechecked all four points against the new head and each one is fixed. Runtime copies take their exec bit from the pinned Git mode, other open PRs and the discussion now go to the safety judge as documented, the setup docs no longer mention --trusted-pr, and the canary runs isolated. Nothing else needs to change.

Earlier review · Reviewed commit

🤖 BStack review · Independent: claude-opus-5-5 + gpt-6-astra · Final check: claude-opus-5-5

@bbishdotdev
bbishdotdev marked this pull request as ready for review October 23, 2026 19:26
@bbishdotdev
bbishdotdev merged commit 2efe613 into main Oct 23, 2026
1 check passed
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