Skip to content

feat(autonomy): measure data flow and the workspace mount in the isolation probe - #2150

Merged
kyle-sexton merged 11 commits into
mainfrom
feat/autonomy-harden-isolation-probe
Aug 11, 2026
Merged

feat(autonomy): measure data flow and the workspace mount in the isolation probe#2150
kyle-sexton merged 11 commits into
mainfrom
feat/autonomy-harden-isolation-probe

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Summary

The isolation probe's two assertions left the entire host-execution attack class unmeasured, and its
exit-code egress test graded a kernel-separated boundary wrongly. This adds a third assertion covering
the workspace mount and reproves egress denial by peer identity rather than by a failed connection.

This is a breaking change for anyone with an existing L2/L3 binding. See the migration note
below.

Why the old egress test was wrong

Two behaviors defeat it, both observed live rather than theorized:

  • A raw TCP connect() succeeds where an interception layer accepts the SYN and then drops the
    session. "Connection refused" was the wrong thing to require.
  • A policy block page is a valid HTTP response, so a fetch client exits 0 against a fully sealed
    boundary.

Certificate verification does not settle it either, and this PR has the measurement to prove it. On
the boundary probed for this change, the interception layer presented a certificate carrying the
correct hostname, signed by a CA the boundary trusted:

ssl_verify_result=0
peer  subject: O=Docker Sandboxes; CN=example.com
peer  issuer:  O=Docker Sandboxes; CN=Docker Sandboxes Proxy CA

Hostname matching passed and verification returned success — a certificate-verification test would
have graded a fully sealed boundary as reached egress.
Only peer IDENTITY distinguished the two:

Target Outer fingerprint Inner fingerprint Verdict
example.com 61:53:A9:6F:D1:A6:AB:7F… FE:9E:55:15:61:71:1A:C5… differs → peer-substituted
iana.org 14:6E:78:EF:81:43:4A:17… EA:1C:71:B5:DF:3A:3A:37… differs → peer-substituted

Three legs close the remaining holes: the probe client must be shown to RUN inside the boundary (an
absent client would satisfy every egress assertion trivially), at least two targets under different
operators must be denied (one denial is consistent with a policy that allows others), and the
exercised address families are recorded rather than inferred.

The third assertion

workspace_host_write_contained proves that no write performed inside the boundary becomes visible at
the host workspace path. It is proven from the outer side, which is the only framing that covers
both substrate shapes at once: a read-only mount rejects the inner write, a copy-on-read mount accepts
and discards it, and both are contained. The inner exit code is recorded but never asserted on —
constraining it would grade copy-on-read substrates wrongly.

Canaries span an ordinary file, a dotfile, and a version-control path, and are randomized per run so a
substrate cannot special-case a known probe path. The host re-check runs after teardown, because a
caching or asynchronously-flushed mount can propagate a write behind an in-flight check.

.git/config is named explicitly: it is a command key ring, and core.fsmonitor executes host code on
a read-only-looking git status.

Scope is stated rather than implied. This proves host-WRITE containment. It does not measure READ
exposure, and a copy-on-read workspace leaves reads fully open — which is why the assertion carries the
longer name.

MIGRATION — action required for existing bindings

Every level bound under the two-assertion recipe must be re-probed. Those transcripts carry no
workspace assertion, so the check leaves the level UNPROVEN, and the ladder's fail-closed rule then
blocks autonomous dispatch on that surface until a fresh probe lands.

Nothing degrades silently and no binding becomes invalid — the affected levels simply stop counting
toward isolation eligibility, and the failure names the missing assertion so the remedy reads off it.
This is a deliberate bar raise: the old recipe certified boundaries it had never measured.

To restore dispatch: re-run the probe under the updated recipe and re-record probe_evidence.

Verification

  • 446/446 fixture checks pass (125 fixtures graded, 0 quarantined); 12 cases added.
  • 0 pre-existing pinned findings_substrings changed, 0 removed. Both new check groups run LAST in
    verifyProbeTranscript — the function returns the first problem it finds, and all 58 pre-existing
    probe-evidence-* cases pin their own rejection reason, so an earlier placement would have silently
    answered for every one of them.
  • Test-first: each negative fixture was written and confirmed red before its check was implemented.
  • The recipe was run against a live boundary, because fixtures test the CHECKER and never the RECIPE.

What the live run found that review had not

  • Both defects above in the recipe as first written: a direct-TLS fingerprint tool cannot traverse an
    HTTP CONNECT proxy (it reports no peer at all, identically for a sealed and an open boundary), and
    the probe shape had left fail-on-HTTP-error to the operator. The first attempt at the live run
    reproduced the original false negative exactly.
  • A scope change stated rather than absorbed: the intended "randomized target no allowlist anticipated"
    leg cannot exist. Unguessability and outer-reachability are mutually exclusive over DNS, and
    outer-reachability is the older load-bearing rule (a target that fails everywhere "fails" inside too
    and proves nothing). Substituted: two distinct targets under different operators.

Also in this change

Every hardcoded assertion COUNT moved with the assertion set — the probe template, the security-binding
schema's probe_evidence description, the guided-setup narration, and the probe-before-bind eval all
said "both", which would have left the contract self-contradicting. The eval was found by the phase's
pre-flight consumer check, not by the plan.

L2 in the isolation ladder now names contained workspace host-writes alongside default-deny egress and
credential protection.

No product or vendor name enters the plugin's contract surface; the vendor-specific detail above is
capture evidence in this description only.

No linked issue

Related

  • melodic-software/claude-code-plugins#2110 — the durable record for this work. This PR discharges its
    items 1 and 2 (harden the egress assertion; add the workspace assertion). Items 3 (whether the ladder
    models kit-supplied allow rules) and 4 (re-verification cadence for version-bound evidence) are
    deliberately untouched and remain open.
  • Branch docs/docker-sandbox-substrate-plan carries the full PLAN artifact, including the remaining
    phases 2–5 (verification topology). The contract slice is pruned from this PR per
    docs/conventions/topic-docs/README.md.

@cursor

cursor Bot commented Aug 10, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e3faaa7b8c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/autonomy/skills/setup/templates/isolation-probe.md Outdated
Comment thread plugins/autonomy/skills/setup/scripts/check-security-binding.mjs
Comment thread plugins/autonomy/skills/setup/scripts/check-security-binding.mjs
Comment thread plugins/autonomy/skills/setup/scripts/check-security-binding.mjs
Comment thread plugins/autonomy/skills/setup/scripts/check-security-binding.mjs
@kyle-sexton
kyle-sexton force-pushed the feat/autonomy-harden-isolation-probe branch 2 times, most recently from b327d52 to d49de23 Compare August 10, 2026 13:54
@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 5m 24s —— View job


Code review complete

  • Gather context (diff, CLAUDE.md/REVIEW.md, changed files)
  • Determine review scope split (security lane vs this lane)
  • Review checker script logic (check-security-binding.mjs)
  • Review probe template (isolation-probe.md)
  • Review schema/docs/eval changes
  • Review test fixture coverage
  • Post findings

Scope note: this repo has .github/workflows/claude-security-review.yml, so per REVIEW.md this lane owns correctness/design/tests/docs and leaves vulnerability-class findings to the security lane. All five findings below are validation-logic correctness gaps in the new assertion code, verified by hand-tracing check-security-binding.mjs (I was not able to execute the fixture test suite or a crafted-transcript check in this sandbox — Bash/Write calls needed to spawn node or write scratch files required interactive approval that wasn't available; findings below are from static reading of the diff, not from running the checker).

Posted 5 inline findings, most-significant first:

  1. isolation-probe.md:76-80 — The example recipe's --fail-on-http-error abort, combined with the checker's unconditional non-zero exit_code requirement (check-security-binding.mjs:1250-1253), means a boundary that seals via an HTTP-200 block page — the exact scenario this PR's peer-fingerprint mechanism was built to prove — can never produce a passing transcript through the shipped recipe. Fails closed (no false eligibility), but the "peer identity, not exit code" fix the PR describes isn't reachable for the case it targets.
  2. check-security-binding.mjs:1403-1406 — The "≥2 distinct targets" floor checks egressHosts.length without deduping; "example.com,example.com" passes as two targets.
  3. check-security-binding.mjs:1478-1482 — Canary count/non-emptiness is checked, but not the three required shapes (ordinary/dotfile/.git/-relative) or their distinctness.
  4. check-security-binding.mjs:1431 — Peer-fingerprint consistency is only checked for outcome === "peer-substituted"; other outcomes can carry non-none, even matching, fingerprints unchecked, contradicting the documented "literal none elsewhere" invariant.
  5. check-security-binding.mjs:1499-1505 (minor) — inner_exit_code only checks split length, so ",," passes; doesn't gate eligibility (field is never asserted on) but leaves the reviewer-facing evidence record silently emptyable.

Everything else checked out well: version bump, CHANGELOG, schema description, isolation-ladder.md, SKILL.md, and evals.json are all updated consistently with the new three-assertion contract (no stale "both" language found outside what the PR already fixed), and the new negative fixtures + manifest entries are well-formed and correctly exercise the checks that are implemented — none of them happen to cover the five gaps above, which is consistent with those being real gaps rather than something the existing 446-case suite would have already caught.

Note: I did not independently run the fixture suite or CI in this session (see scope note above) — the PR's own "446/446" claim is author-reported and unverified by me.

@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 3m 12s —— View job


Security review

  • Gather context (PR diff, custom instructions)
  • Read changed files (isolation-probe.md, check-security-binding.mjs, schema, ladder doc, fixtures)
  • Hunt for logic flaws, auth gaps, injection, secret handling issues
  • Cross-check zizmor-covered concerns are excluded (no workflow files changed in this PR)
  • Post findings as PR review

This PR changes no .github/workflows/* files, so the Actions-specific attack surface (permission widening, pull_request_target, github-context script injection, unpinned actions) doesn't apply — zizmor's lane is a no-op here. The relevant trust boundary is check-security-binding.mjs: it's a security gate — its logic errors are "fail open" bugs that let an insecure/unmeasured boundary get certified for autonomous dispatch. I read every line the diff added (the new tail of verifyProbeTranscript plus the new verifyWorkspaceContainment) and independently verified the following against fixtures.


IMPORTANT — Duplicate egress targets aren't rejected, defeating the "two distinct targets" requirement.
Confidence: High

check-security-binding.mjs#L1405

egressHosts.length < 2 only counts comma-separated entries; nothing deduplicates them. A transcript with "host": "example.com,example.com" has length 2 and sails through the rest of the function, which then only re-validates each entry independently (no cross-entry uniqueness check anywhere in verifyProbeTranscript). The PR's own stated rule — "at least two targets under different operators must be denied (one denial is consistent with a policy that allows others)" — is not actually enforced; a policy that allows every host except one could still probe that one host twice and pass. I confirmed no fixture exercises this: probe-evidence-egress-single-target.json/ci-pool-a-l2-egress-single-target.json only test the true single-host case ("host": "example.com"), not a duplicated pair.


IMPORTANT — Non-peer-substituted egress outcomes never validate their fingerprints, so a mislabeled "denial" with real matching peer identity passes.
Confidence: High

check-security-binding.mjs#L1423-L1439

The fingerprint-consistency check (outer_peer_fingerprint/inner_peer_fingerprint must be "none" unless the outcome is peer-substituted, per the template's own documented contract at isolation-probe.md#L101) only runs inside if (outcome === "peer-substituted"). For dns-unresolved, connect-failed, or tls-failed, the loop does nothing with the fingerprint fields at all — a transcript can claim transport_outcome: "connect-failed" while outer_peer_fingerprint/inner_peer_fingerprint both carry the same real, non-"none" value (i.e., internally self-contradictory evidence that a handshake with the origin's own identity actually completed) and the checker never flags the inconsistency. Given this whole PR's thesis is "peer identity, not exit code, is the only thing that actually proves denial," an unenforced internal-consistency rule on the identity fields for 3 of the 4 outcome tokens is a real gap in the gate the PR is trying to harden.


SUGGESTION — Workspace canary shapes aren't validated, and inner-write exit codes accept empty entries.
Confidence: Medium

check-security-binding.mjs#L1481 and #L1504

canaries only checks count (>= 3) and non-emptiness, never that the set actually contains one ordinary file, one dotfile, and one .git/-relative path as the design requires (.git/config is called out specifically as "a command key ring"). same,same,same or three ordinary-file paths pass identically to a correctly-shaped set. Separately, inner_exit_code (L1499-1504) is length-checked but not content-checked, so ",," (three empty strings) satisfies the positional-pairing requirement. This field is explicitly never asserted on by design (a clone-mode mount legitimately accepts the write), so the containment verdict itself (host_pre_absent/host_post_absent, both strictly checked === "0") isn't weakened by either gap — but it does mean the gate can't distinguish a genuinely-attempted write from a skipped one, and the canary-shape gap means the control-plane (.git/config) coverage this assertion exists to add isn't actually structurally required. Lower severity than the two above because the load-bearing pass/fail fields (host_pre_absent, host_post_absent, digest equality) are still strictly enforced.


No other injection, auth, or secret-handling issues found in this diff. The credential-root/deny-by-default logic, path-traversal/symlink handling in verifyProbeTranscript's evidence-root resolution, and the IPv4/IPv6 non-external-address classification (all pre-existing, unmodified by this PR) look sound on inspection.

Comment thread plugins/autonomy/skills/setup/templates/isolation-probe.md Outdated
Comment thread plugins/autonomy/skills/setup/scripts/check-security-binding.mjs
Comment thread plugins/autonomy/skills/setup/scripts/check-security-binding.mjs
Comment thread plugins/autonomy/skills/setup/scripts/check-security-binding.mjs
Comment thread plugins/autonomy/skills/setup/scripts/check-security-binding.mjs
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude has reviewed this PR 4 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

@kyle-sexton
kyle-sexton force-pushed the feat/autonomy-harden-isolation-probe branch from d49de23 to 4dc8747 Compare August 10, 2026 14:26
@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 5m 26s —— View job


Security review

  • Gather context (git diff origin/main...HEAD, prior review comments, custom instructions)
  • Identify the delta since the last security review: commit 4dc8747 ("close five gaps both PR reviewers found independently")
  • Re-verify each of the 5 previously reported findings against the fix
  • Hunt for new logic flaws in the fix itself
  • Post findings

Scope note: no .github/workflows/* files changed in this PR, so zizmor's lane (unpinned actions, permission widening, dangerous triggers) is a no-op here; this review focuses on check-security-binding.mjs, the security gate that certifies a boundary for autonomous dispatch — its logic errors are fail-open bugs.

I was not able to execute the fixture suite (node check-security-binding.fixtures.test.mjs) in this sandbox — node invocations required interactive approval that wasn't available — so the below is from hand-tracing the diff and cross-checking it against the new fixtures/manifest pins, not a live run.


The five previously-reported findings are all correctly closed by 4dc8747:

  1. Duplicate egress targets — now rejected via a lowercased, trailing-dot-normalized Set size check (check-security-binding.mjs#L1422-L1424).
  2. Fingerprint on non-peer-substituted outcomes — now rejected unless the literal "none" (#L1456-L1461). Only inner_peer_fingerprint is checked, not outer_peer_fingerprint — I traced this deliberately: the recipe (isolation-probe.md#L72-L75) captures the outer fingerprint unconditionally for every target (reachability is required for all outcomes), so it's never legitimately "none" — requiring it to be "none" elsewhere would be self-contradictory. Correct as implemented.
  3. Canary count → shape coverage — distinctness, traversal, and ordinary/dotfile/.git/-relative shape coverage are all now enforced (#L1507-L1528) — see new finding below, though.
  4. Empty inner_exit_code entries — now rejected by an integer-shape regex per entry (#L1555-L1558).
  5. The unreachable zero-exit case (Codex P1 on the recipe) — the recipe no longer aborts on a zero exit from the inner fetch (isolation-probe.md#L76-L88), and the checker now accepts a zero exit only where transport_outcome === "peer-substituted" and the two fingerprints differ (check-security-binding.mjs#L1250-L1264) — every other target still requires a non-zero exit, and that same fingerprint pair is independently re-validated later in the function (#L1449-L1455), so the exception can't be claimed without also surviving the identity check.

IMPORTANT — The new canary "workspace-relative, no traversal" check only recognizes POSIX-style absolute/traversal forms; a Windows drive-letter or UNC path slips through as a valid "ordinary" canary.
Confidence: High

check-security-binding.mjs#L1513:

if (canaries.some((entry) => entry.startsWith("/") || entry.split(/[\\/]/).includes(".."))) {

This is meant to guarantee every canary names a location inside the workspace under test — the comment right above it says "an absolute or escaping path proves containment somewhere other than the workspace mount under test." But it only catches a POSIX-rooted /... absolute path or a literal .. traversal segment. Two forms it misses, both traced by hand against the exact regex used:

  • Drive-letter absolute: "C:\\escape.txt" — does not start with /, and entry.split(/[\\/]/) yields ["C:", "escape.txt"] (no ..). It passes both guards, and since its basename doesn't start with . and its first segment isn't .git, it's classified as a valid "ordinary" canary shape.
  • UNC: "\\\\evilserver\\share\\escape.txt" — doesn't start with / (starts with \), and splitting on [\\/] yields ["", "", "evilserver", "share", "escape.txt"] — no .., so it also passes.

Notably, this exact class of path (drive-root, UNC) is already handled correctly elsewhere in this same file for credential-path validation — normalizeHostPath (#L209-L236) explicitly folds backslashes to slashes first and rejects both //-UNC and refuses to treat a bare drive root as containable. The new canary check doesn't reuse that logic and re-implements a narrower, POSIX-only version.

Impact: a transcript can name one of its three required canary shapes (the "ordinary file" one) as an absolute path elsewhere on the host filesystem rather than inside the workspace mount, and the checker's host_pre_absent/host_post_absent/inner_exit_code fields for that entry are then taken purely on the transcript's say-so (the checker never independently re-executes anything) — so this specific canary tests nothing about the workspace mount while still counting toward the "3 required shapes" coverage the assertion exists to enforce.

Fix: normalize separators before the check (entry.replaceAll("\\", "/")) and additionally reject a drive-letter-rooted form (e.g. /^[a-zA-Z]:\//) and a //-prefixed UNC form after normalization — mirroring normalizeHostPath's existing handling a few hundred lines up in the same file.


No other injection, authorization, or secret-handling issues found in this delta. The rest of the fix commit (duplicate-host dedup, fingerprint-consistency, inner-exit-code shape) is sound, fails closed, and each behavior is now pinned by a fixture in check-security-binding.fixtures.test.manifest.json.

@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 3m 1s —— View job


Code review — re-review after fix commit 4dc8747

  • Gather context (diff, CLAUDE.md, prior review comments)
  • Verify each of the 5 previously-reported gaps against the fix commit
  • Review the fix commit for new correctness issues
  • Post findings

Scope note (unchanged): this repo has .github/workflows/claude-security-review.yml, so per REVIEW.md this lane owns correctness/design/tests/docs and leaves vulnerability-class findings to the security lane.

I traced commit 4dc8747 ("fix(autonomy): close five gaps both PR reviewers found independently") against the five findings both reviewers converged on in the earlier pass. All five are correctly closed:

  1. Zero-exit-on-block-page unreachable via the shipped recipe — fixed at isolation-probe.md#L76-L92: the recipe no longer aborts on a zero exit, and always runs the fingerprint verdict. The checker at check-security-binding.mjs#L1247-L1256 now accepts a "0" exit only when that entry's transport_outcome === "peer-substituted" — and that entry still has to survive the downstream fingerprint-difference check (L1453-L1456), so the exception can't be claimed by a target that simply succeeded. Confirmed the new positive fixture (probe-evidence-egress-zero-exit-peer-substituted.json / its transcript) exercises exactly this path, and every other outcome token still requires a non-zero exit.
  2. Duplicate egress host defeating the "≥2 targets" floor — fixed at check-security-binding.mjs#L1419-L1423: hosts are lowercased and trailing-dot-stripped into a Set before the count check. probe-evidence-egress-duplicate-host.json (host "example.com,example.com") now fails with a "distinct target" reason.
  3. Fingerprint consistency unchecked outside peer-substituted — fixed at check-security-binding.mjs#L1456-L1461: every other outcome now requires inner_peer_fingerprint === "none", closing the internally-contradictory-evidence gap.
  4. Canary shape/distinctness unchecked — fixed at check-security-binding.mjs#L1507-L1527: case-insensitive distinctness, workspace-relative confinement (rejects absolute/..), and a same-index shape classifier requiring one ordinary, one dotfile, and one .git/-relative entry, each mutually exclusive by construction (control-plane checked first, then dotfile excludes it, then ordinary excludes both). same,same,same and three ordinary files are now rejected by dedicated fixtures.
  5. Empty inner_exit_code entries accepted — fixed at check-security-binding.mjs#L1552-L1557: each entry must now match /^(0|[1-9][0-9]*)$/, so ",," no longer passes, while the value itself is still correctly never asserted on.

All five fixes ship with a matching manifest entry and transcript fixture pinning the new rejection substring (or, for #1, a positive fixture pinning acceptance), consistent with this PR's stated test-first discipline. I didn't find any new correctness issue introduced by the fix commit itself — the shape-classification helpers are mutually exclusive, the new checks are positioned consistently with the "first problem found" contract the rest of the function already follows, and none of the 58 pre-existing probe-evidence-* pinned reasons are touched by this commit's diff.

One very minor, non-blocking observation: the new duplicate-canary check normalizes case (toLowerCase()) but the shape classifier's .git match is case-sensitive, so a canary like .GIT/config would be silently bucketed as "ordinary" rather than recognized as (or rejected as a mis-cased attempt at) the control-plane shape. This still fails closed — the transcript would then be rejected for "missing a VCS-control-plane path" rather than mis-accepted — so I'm not filing it as a finding, just noting it in case it's worth a follow-up for clean rejection messaging.

What I could not verify: as in the prior pass, I was unable to execute the fixture suite (check-security-binding.fixtures.test.mjs) in this sandbox — node invocation required interactive approval that wasn't available here. The "474 checks pass" figure in the fix commit's message is author-claimed and unverified by me; my review above is a hand-trace of the checker logic and fixture content, not a test run.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Isolation probe evidence — hardened three-assertion recipe

Run 2026-08-10 against the recipe as revised in Phase 1, on Windows 11 Pro. This is the Phase 1
merge gate: fixtures test the CHECKER, never the RECIPE, so the recipe only becomes evidence when it
is run against a real boundary.

This is capture evidence, not a level binding. No product instance is named in the plugin's
contract surface; the substrate id belongs in a consumer's security binding.

Result — all three assertions fail inside the boundary, which is the passing condition

Assertion Outer context Inside boundary Verdict
Denied egress both targets reachable, exit 0, distinct peer fingerprints HTTP 403 policy block page; peer certificate issued by an in-boundary CA, fingerprints differ from outer denied (peer-substituted)
Absent host credentials not re-run this pass carried from the prior run under the same posture; the hardened changes did not touch this assertion
Contained workspace host-writes canaries absent, .git/config digest recorded writes rejected on the read-only mount; clone-side writes accepted contained — every canary still absent after teardown, digest unchanged, no fsmonitor key on the host

The finding that vindicates the peer-identity design

Certificate verification passes cleanly inside this boundary:

ssl_verify_result=0
peer  subject: O=Docker Sandboxes; CN=example.com
peer  issuer:  O=Docker Sandboxes; CN=Docker Sandboxes Proxy CA

The interception layer presents a certificate carrying the correct hostname, signed by a CA the
boundary trusts. Hostname matching passes and verification returns success. A
certificate-verification test would therefore have graded this sealed boundary as reached egress

the exact failure the cross-vendor review predicted and the reason the recipe compares peer IDENTITY
instead. The comparison decides it correctly:

Target Outer fingerprint (SHA-256, truncated) Inner fingerprint Verdict
example.com 61:53:A9:6F:D1:A6:AB:7F… FE:9E:55:15:61:71:1A:C5… differs → peer-substituted
iana.org 14:6E:78:EF:81:43:4A:17… EA:1C:71:B5:DF:3A:3A:37… differs → peer-substituted

This substrate is a live instance of the TLS-inspection case the design was hardened against, not a
hypothetical one.

Recipe defects this run exposed, both now fixed

  1. openssl s_client cannot capture the peer through an HTTP CONNECT proxy. It speaks TLS
    directly, so against a proxied boundary it reports no peer at all and the fingerprint leg returns
    none — identically for a sealed boundary and a wide-open one. The capture must be proxy-aware;
    the recipe now requires it and names why.
  2. A bare fetch without fail-on-HTTP-error exits 0 against a block page. The first attempt at this
    run reproduced precisely the false negative the recipe exists to prevent. The recipe now shows the
    fail-mode flag in its probe shape rather than leaving it to the operator.

Machine-state correction to the prior handoff

The handoff recorded the global network policy as deny-all and cited sbx policy ls as
confirmation. policy ls does not display the global policy at all — it lists only local and
kit-scoped policies, so the listing reads as though no deny-all exists. policy init reports it is
already initialized, and policy check network <host> reports Denied … no matching allow rule (default deny). Use policy check, never policy ls, to confirm the posture.

The kit-widening caveat still reproduces: a network: 1 allow policy for openrouter.ai, source
kit, was attached to the newly created sandbox on top of the global default deny. That remains the
open USER-RESERVED question, untouched here.

Transcript record

{
  "schema_version": "1",
  "surface": "<execution-surface-id — resolved at binding time, outside this repo>",
  "level": "L3",
  "substrate": "<instance id — recorded in the consumer's security binding, never here>",
  "substrate_class": "vm-microvm",
  "probed_at": "2026-08-10T05:52:00Z",
  "assertions": {
    "egress_denied": {
      "host": "example.com,iana.org",
      "exit_code": "22,22",
      "outer_exit_code": "0,0",
      "transport_outcome": "peer-substituted,peer-substituted",
      "outer_peer_fingerprint": "61:53:A9:6F:D1:A6:AB:7F:4D:43:8F:C3:49:32:48:42:99:D0:72:9D:91:40:B3:A1:26:BB:2F:9C:07:B0:22:00,14:6E:78:EF:81:43:4A:17:A4:A7:87:FA:41:A2:E5:41:0E:6B:67:7B:A0:30:76:1E:AE:F1:F3:00:8F:5F:AE:01",
      "inner_peer_fingerprint": "FE:9E:55:15:61:71:1A:C5:9E:44:41:C1:DE:00:42:61:1E:89:73:9A:FC:E2:19:92:61:65:98:08:04:70:7B:7C,EA:1C:71:B5:DF:3A:3A:37:6B:A7:D0:1B:15:21:37:B8:92:C3:63:74:16:94:3A:95:19:21:08:C0:6C:0A:92:03",
      "client_ready": "0",
      "address_families": "ipv4",
      "outcome": "denied"
    },
    "workspace_host_write_contained": {
      "workspace_host_path": "<throwaway workspace root — never the real repository>",
      "canaries": "cnry-a1.txt,.cnry-a1,.git/cnry-a1",
      "inner_exit_code": "2,2,2",
      "host_pre_absent": "0,0,0",
      "host_post_absent": "0,0,0",
      "git_config_digest_pre": "ee675b70e7cb54baed7df267094697b5f555a1d6b6bfd676a8b36a296ed3c774",
      "git_config_digest_post": "ee675b70e7cb54baed7df267094697b5f555a1d6b6bfd676a8b36a296ed3c774",
      "checked_after_teardown": true,
      "outcome": "contained"
    }
  },
  "outer_context_networked": true
}

credentials_absent is omitted above rather than fabricated: it was not re-run this pass. A binding
transcript requires all three, so this capture is recipe evidence, not a bindable transcript.

Scope

One kit, one workspace, one host, one version. address_families records ipv4 only — the IPv6 leg
was not exercised, and per the recipe an unexercised family is never counted as denied.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Correction to the probe-evidence comment above.

That comment states sbx policy ls does not display the global network policy at all. That conclusion was wrong, and cleanup falsified it: once both probe sandboxes were removed, the listing showed default-deny-all plainly.

What was actually observed: the global policy was absent from the listing while two kit-scoped policies were present, and policy init reported it already initialized. The mechanism behind that omission is unexplained, and I am not claiming one.

The durable lesson is narrower than I first wrote it — confirm the posture with policy check network <host>, which reports the effective decision, rather than inferring it from a listing whose completeness is not guaranteed. Nothing in the PR's code or contract changes depends on this; it affects only the environment note.

kyle-sexton and others added 10 commits August 11, 2026 17:56
Records the decisions from an interview covering two questions that turned out
to be one: whether to adopt a kernel-separated local isolation substrate, and
what shape an autonomous multi-run delivery pipeline should take.

The substrate half resolved to a non-change here. Docker Sandboxes is an
instance for the sandbox-provider seam the autonomy pack already specifies, the
guided-setup path already detects capability-shaped rather than instance-shaped,
and the ladder forbids naming a product instance in this repository at all — so
integration costs zero files. It was probed live under a hardened invocation and
both ladder assertions failed inside the boundary, which is the passing
condition.

The pipeline half is governed by one finding: verifier independence pays at the
model level rather than the context level, and judge diversity outranks judge
size. Verification topology therefore becomes a configurable column on the
existing guardrail matrix, keyed on roles and relational constraints rather than
capability labels, which do not survive a model release.

Four questions are deferred with arbiters; the sharpest is whether the ladder
should model kit-supplied allow rules, since a kit widened egress on top of a
global deny-all during the probe.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0133WoxqBf7kLDviEfrcYL5Z
Resolves Q20 (arbiter /planning:plan): the egress assertion becomes a
certificate-verified TLS fetch, which converts the two observed false
negatives into true negatives while keeping the non-zero-exit invariant;
workspace containment becomes a third assertion proven from the outer
side, so one rule covers both read-only and clone-mode mounts.

Records the Tier A design gate as resolved by the interview register
rather than re-running /planning:design, and corrects the Brief TLDR's
assertion polarity to match the contract (both assertions must FAIL
inside the boundary).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0133WoxqBf7kLDviEfrcYL5Z
The same-vendor sub-agent review channel failed three times, so the
independent pass ran cross-vendor instead — the route the skill names as
preferred over the sub-agent fallback. Its findings were verified against
the files before being applied.

Material changes: the egress discriminator becomes a peer-fingerprint
comparison against the outer context rather than certificate verification,
which a TLS-inspection CA trusted inside the boundary would defeat; the
workspace assertion is renamed workspace_host_write_contained and its
post-check moves after boundary teardown; raising the L2 bar is recorded
as a breaking migration that blocks dispatch for existing adopters until
they re-probe; the normative leaf carries no capability label at all; the
machine-checkable predicates reach the schema rather than staying prose;
and criteria 6 and 7 state plainly that their runtime halves cannot be
demonstrated without a runner the Brief keeps trigger-gated.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0133WoxqBf7kLDviEfrcYL5Z
…shes

The cross-vendor pass was an independent reading, not an execution: no
fixture harness run, no substrate re-probed. It inherits the same limit
the Test Strategy already states, which is why the live re-probe stays a
merge gate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0133WoxqBf7kLDviEfrcYL5Z
…ation probe

The probe's two assertions left the whole host-execution attack class
unmeasured and graded a microVM-class boundary wrongly. A raw connect()
succeeds where an interception layer accepts the SYN and drops the
session, and a policy block page is still an HTTP response, so an
exit-code test could read either as reached egress.

Egress denial is now proven by peer IDENTITY rather than reachability or
certificate validity: an interceptor cannot present the origin's own key,
so an in-boundary fingerprint matching the outer context's means the
origin answered. Certificate verification alone would not do it, because
an inspection CA trusted inside the boundary verifies cleanly. Three legs
close the remaining holes: the client must be shown to run (an absent
fetch tool would otherwise satisfy every assertion trivially), at least
two targets under different operators must be denied (one denial is
consistent with a policy that allows others), and the exercised address
families are recorded rather than inferred.

The third assertion proves host-write containment from the OUTER side,
which is the only framing that covers both substrate shapes at once: a
read-only mount rejects the inner write, a clone-mode mount accepts and
discards it, and both are contained. The inner exit code is recorded but
never asserted on. Canaries span an ordinary file, a dotfile, and a
.git/ path, and the re-check runs after teardown so a caching mount
cannot propagate the write behind the probe's back.

Both new check groups run LAST in verifyProbeTranscript. The function
returns the first problem it finds and all 58 pre-existing
probe-evidence fixtures pin their own rejection reason, so an earlier
placement would silently answer for every one of them. Verified: 0
pre-existing pinned reasons changed, 12 cases added.

Scope stated rather than implied: this proves host-WRITE containment.
Read exposure is untouched, and a clone-mode workspace leaves reads
fully open.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0133WoxqBf7kLDviEfrcYL5Z
The checker was ahead of the documents. Every hardcoded assertion COUNT
moved with the assertion set — the probe template, the security-binding
schema's probe_evidence description, the guided-setup narration, and the
probe-before-bind eval all asserted "both", which would have left the
contract self-contradicting.

The isolation ladder's L2 now names contained workspace host-writes
alongside default-deny egress and credential protection, with the scope
stated rather than implied: this is WRITE containment, read exposure is
not covered, and a copy-on-read workspace leaves reads fully open.

The CHANGELOG leads with the migration, because this release blocks
autonomous dispatch on every surface bound under the old recipe until it
is re-probed. Nothing degrades silently and no binding becomes invalid;
the affected levels stop counting toward eligibility and the check names
the missing assertion so the remedy reads off the failure. MINOR bump.

Two plan sanity checks were corrected rather than satisfied: "must fail
to CONNECT" belongs to the credential assertion's metadata clause where
it is still right, and a literal-token count of three was arbitrary
against a template that documents the assertion correctly in prose.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0133WoxqBf7kLDviEfrcYL5Z
…icit

The live re-probe that gates this phase exposed two defects in the recipe
as written, both of which would have made a sealed boundary look open or
an open one look sealed.

A direct-TLS tool cannot traverse an HTTP CONNECT proxy. Against a
proxied boundary it reports no peer at all — identically whether egress
is denied or wide open — so an unaware fingerprint capture silently
records "no peer" and proves nothing. The capture must run over the same
egress path the boundary routes traffic through.

And the probe shape now shows fail-on-HTTP-error rather than leaving it
to the operator: without it a policy block page is a successful transfer
that exits 0, which is the original false negative this rework exists to
eliminate. The first attempt at the live run reproduced it exactly.

The run also produced the empirical case for comparing peer identity
rather than verifying certificates: the measured interception layer
presented a certificate with the CORRECT hostname signed by a CA the
boundary trusted, so verification returned success on a fully sealed
boundary. Only the differing fingerprint told the truth.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0133WoxqBf7kLDviEfrcYL5Z
The merge gate was not a formality. The live run found two recipe
defects and one stale environment claim that review had not caught, and
it produced the empirical case for comparing peer identity rather than
verifying certificates.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0133WoxqBf7kLDviEfrcYL5Z
docs/topics/<slug>/ is Contract tier per docs/conventions/topic-docs/README.md
— committed on a task branch only, pruned before merge. The plan's durable
homes are this PR body and branch docs/docker-sandbox-substrate-plan, which
carries the full artifact for the remaining phases.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0133WoxqBf7kLDviEfrcYL5Z
Two reviewers converged on the same five findings, which is the signal
that they are real rather than stylistic. All five let a transcript
satisfy the letter of an assertion while measuring nothing.

The serious one is mine by construction. The plan recorded that the
non-zero-exit invariant "stands unchanged — no relaxation", justified by
the claim that certificate verification achieved the same discrimination
while keeping the exit requirement. That justification was then
invalidated when certificate verification was replaced by peer-identity
comparison, and the exit requirement outlived its own reasoning: an
interception layer whose block page carries a SUCCESSFUL HTTP status
exits 0 on a fully sealed boundary, and both the recipe and the checker
would refuse to certify it. A zero exit is now accepted where, and only
where, that target's transport_outcome is peer-substituted and its two
fingerprints differ. A positive fixture proves the relaxation admits the
boundary it exists for; every other target still requires a non-zero
exit, so the exception cannot excuse one that simply succeeded.

The other four all counted entries where they meant to measure coverage.
Two egress targets are now DISTINCT targets, so "example.com,example.com"
no longer presents one policy decision as two. A peer fingerprint on any
outcome other than peer-substituted is rejected rather than ignored,
because a recorded peer contradicts an outcome asserting that none
completed. Canaries must be distinct, workspace-relative, and cover all
three shapes, so "same,same,same" and three ordinary files no longer pass
while leaving the control plane — the whole reason the assertion exists —
unprobed. And every inner exit code must be a recorded integer: host
absence evidences containment only where a write was actually attempted,
so an empty entry described a probe that never ran.

474 checks pass; 0 pre-existing pinned reasons changed, 19 cases added.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0133WoxqBf7kLDviEfrcYL5Z
@kyle-sexton
kyle-sexton force-pushed the feat/autonomy-harden-isolation-probe branch from 4dc8747 to ad9f226 Compare August 11, 2026 21:58
@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 3m 48s —— View job


Security review

  • Gather context (git diff origin/main...HEAD, prior review history, custom instructions)
  • Identify what's new/unresolved since the last security review (commit 4dc8747, now ad9f226 after rebase)
  • Re-verify the five previously-reported gaps are still closed
  • Review check-security-binding.mjs and isolation-probe.md for new logic flaws
  • Review remaining changed files (schema, ladder doc, SKILL.md, evals, CHANGELOG) for security relevance
  • Post findings

Scope note: no .github/workflows/* files changed in this PR, so zizmor's lane (unpinned actions, permission widening, dangerous triggers, template injection) is a no-op here. This review focuses on check-security-binding.mjs, the security gate that certifies a boundary for autonomous dispatch — its logic errors are fail-open bugs.

I re-verified the five gaps both prior reviewers converged on (zero-exit-on-block-page, duplicate egress host, fingerprint-on-non-substituted-outcome, canary shape/distinctness, empty inner_exit_code) — all five are correctly and durably closed in the current HEAD; I found no regression in any of them.


IMPORTANT — The canary workspace-confinement check accepts Windows drive-letter and UNC paths, letting one required canary "shape" test a location outside the workspace mount.
Confidence: High

check-security-binding.mjs#L1513:

if (canaries.some((entry) => entry.startsWith("/") || entry.split(/[\\/]/).includes(".."))) {

This is the confinement guard the comment above it exists for: "an absolute or escaping path proves containment somewhere other than the workspace mount under test." It only catches a POSIX-rooted /... absolute path or a literal .. traversal segment. Two forms traced by hand against this exact regex slip through:

  • Drive-letter absolute"C:\\escape.txt" does not start with /, and entry.split(/[\\/]/) yields ["C:", "escape.txt"] (no ..). It passes, and since its basename doesn't start with . and its first segment isn't .git, the shape classifier a few lines below buckets it as the required "ordinary file" canary.
  • UNC"\\\\evilserver\\share\\escape.txt" doesn't start with / (starts with \), and splitting on [\\/] yields ["", "", "evilserver", "share", "escape.txt"] — no .., so it also passes.

host_pre_absent/host_post_absent/inner_exit_code for that entry are then taken purely on the transcript's say-so — the checker never independently re-executes anything (true of the whole assertion by design). So a transcript can satisfy the "ordinary file" canary requirement with a path that never touches the workspace mount at all, and the containment proof for that shape verifies nothing. This is exactly the class of gap this PR's own canary-shape hardening was written to close ("counting paths is not covering shapes") — just in a form the added check didn't anticipate.

This exact class of path (drive-root, UNC) is already handled correctly a few hundred lines up in the same file: normalizeHostPath (#L209-L236) explicitly folds backslashes to slashes first, refuses a bare drive root, and refuses a //-UNC form. The new canary confinement check doesn't reuse that logic and re-implements a narrower, POSIX-only version.

Suggested fix: normalize separators before the check (entry.replaceAll("\\", "/")) and additionally reject a drive-letter-rooted form (/^[a-zA-Z]:\//) and a //-prefixed UNC form after normalization, mirroring normalizeHostPath.

Fix this →


No other injection, authorization, or secret-handling issues found in this diff. The rest of the hardened assertion set (peer-fingerprint comparison, duplicate-host dedup, address-family recording, .git/config digest pairing, credential-root containment) is sound and fails closed. All evidentiary fields in this checker are self-reported by the probe operator rather than independently re-executed — a pre-existing, stated design limitation, not a new gap introduced by this PR.

@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 4m 8s —— View job


Code review complete

  • Gather context (gh pr diff 2150, REVIEW.md, CLAUDE.md, prior review threads)
  • Determine what's new since the last code-review pass (commit 4dc8747/current HEAD ad9f226)
  • Check whether the still-open Windows drive-letter/UNC canary-path finding from the security lane was addressed
  • Review remaining changed files for correctness/design/tests/docs
  • Post findings

Scope note: this repo has .github/workflows/claude-security-review.yml, so per REVIEW.md this lane owns correctness/design/conventions/error-handling/tests/docs and leaves vulnerability-class findings to the security lane.

Context: origin/main..HEAD for this PR is unchanged from what the prior two rounds of review (both lanes) already covered — I confirmed no new commits landed since the fix commit both lanes reviewed (4dc8747, same content as current HEAD ad9f226, just a different SHA post-rebase). Rather than re-paraphrase those rounds, I independently re-verified the load-bearing claims by hand-tracing the checker against its fixtures (I could not execute node in this sandbox — same limitation both lanes hit previously — so this is static tracing, not a live run):

  • All 5 previously-reported gaps are correctly and completely fixed. I traced each fixture (probe-evidence-egress-duplicate-host, -fingerprint-on-nonpeer, -zero-exit-peer-substituted, -workspace-canary-*, -workspace-inner-code-empty) against its paired transcript and the checker code path by hand — each one produces exactly the rejection (or acceptance) reason its manifest entry pins. Sampled in full: probe-evidence-workspace-canary-traversal.json (canaries: "../escape.txt,...") correctly trips the entry.split(/[\\/]/).includes("..") guard at check-security-binding.mjs#L1513.
  • The migration/UNPROVEN mechanism is real, not just claimed. Traced the actual path: a transcript missing workspace_host_write_contained fails workspace?.outcome !== "contained" at check-security-binding.mjs#L1490, which verifyProbeTranscript surfaces up through the existing UNPROVEN/fail-closed path — confirmed a fixture (probe-evidence-workspace-missing.json) exercises exactly this.
  • findings_substrings claim verified mechanically: git diff on check-security-binding.fixtures.test.manifest.json shows zero removed/modified lines — the manifest diff is purely additive, so "0 pre-existing findings_substrings changed" is confirmed by the diff itself, not just author say-so.
  • Docs/schema/evals/CHANGELOG consistency confirmed by reading each changed file directly: isolation-ladder.md, SKILL.md, evals.json, the schema's probe_evidence description, and the new CHANGELOG.md entry all consistently say "three"/"ALL THREE" — no stale "both" language remains outside what the PR already replaced. Version bump (0.14.40.15.0) matches the CHANGELOG's [0.15.0] heading.

New (minor) finding — inline comment posted:
check-security-binding.mjs:1522 — the VCS-control-plane entry in missingShapes does isVcsControlPlane(canaries.find(isVcsControlPlane) ?? "") instead of the plain canaries.some(isVcsControlPlane) its two neighboring array entries use. Not a bug (I traced both branches — it's correct), just an inconsistent, more roundabout idiom sitting next to the simpler pattern used one and two lines below it.

Not re-reported (already owned by the security lane): the Windows drive-letter (C:\...) / UNC (\\server\share\...) gap in the canary-path containment check at check-security-binding.mjs:1513 that the security lane flagged in its prior round is still present in this diff — unchanged since that review. Per REVIEW.md's scope split this is a security-gate-bypass finding, so it stays with that lane rather than being duplicated here; flagging its continued presence for visibility only.

No other correctness, design, test-coverage, or documentation issues found. This diff is exceptionally well-instrumented for its own migration story: the checker fails closed for pre-existing transcripts, that behavior is proven by a fixture rather than asserted, and every doc/schema/eval surface that named the old assertion count was updated in lockstep.

Comment thread plugins/autonomy/skills/setup/scripts/check-security-binding.mjs Outdated
The VCS-control-plane check applied find() and then re-applied the same
predicate to the result, which is functionally identical to some() but
sits next to two lines already using the direct form. Same behavior, one
idiom.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0133WoxqBf7kLDviEfrcYL5Z
@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 21s —— View job


Security review

  • Gather context (git diff origin/main...HEAD, prior review history, custom instructions)
  • Identify what's new since the last security review (commit ad9f226 → current HEAD 084285c)
  • Re-verify previously reported findings are still closed / still open
  • Review the delta commit for new logic flaws
  • Post findings

Scope note: no .github/workflows/* files changed in this PR, so zizmor's lane (unpinned actions, permission widening, dangerous triggers, template injection) is a no-op here. This review focuses on check-security-binding.mjs, the security gate that certifies a boundary for autonomous dispatch.

What's new since the last security review pass (ad9f226): exactly one commit, 084285c, and it touches exactly one line:

- isVcsControlPlane(canaries.find(isVcsControlPlane) ?? "") ? null : "a VCS-control-plane path (a `.git/`-relative entry)",
+ canaries.some(isVcsControlPlane) ? null : "a VCS-control-plane path (a `.git/`-relative entry)",

This is a pure idiom swap (find + re-apply-predicate → some), functionally identical for every input — I traced both branches by hand: canaries.find(isVcsControlPlane) returns either a matching element (predicate re-applied is trivially true) or undefined (?? "" then fails the predicate, false), which is exactly canaries.some(isVcsControlPlane)'s truth table. No behavior change, no new security surface. (This was the code-review lane's nit from the prior round — confirmed the fix matches what was suggested.)

Previously reported finding — still open, unchanged by this commit:

The Windows drive-letter / UNC canary-confinement gap reported in the prior review round is untouched by 084285c (it lives at the canaries.some((entry) => entry.startsWith("/") || ...) check, several lines away from the line this commit touched):

check-security-binding.mjs#L1513:

if (canaries.some((entry) => entry.startsWith("/") || entry.split(/[\\/]/).includes(".."))) {

IMPORTANT — Confidence: High. The workspace-confinement guard ("an absolute or escaping path proves containment somewhere other than the workspace mount under test") only catches a POSIX-rooted /... absolute path or a literal .. traversal segment. A Windows drive-letter absolute path ("C:\\escape.txt") or a UNC path ("\\\\evilserver\\share\\escape.txt") does not start with / and contains no .. segment after splitting on [\\/], so both pass through and the shape classifier a few lines below buckets the drive-letter form as the required "ordinary file" canary. Since every field for that canary (host_pre_absent/host_post_absent/inner_exit_code) is taken purely on the transcript's say-so, a transcript can satisfy the "ordinary file" shape requirement with a path that never touches the workspace mount at all — exactly the class of gap this PR's own canary-shape hardening exists to close, just in a form the added check doesn't yet cover. normalizeHostPath a few hundred lines up in the same file (#L209-L236) already handles this exact path class correctly and could be reused/mirrored here.

Fix this →

No other injection, authorization, or secret-handling issues found. I attempted to run the fixture suite directly (node check-security-binding.fixtures.test.mjs) to move past static tracing this round, but node execution required interactive approval that wasn't available in this sandbox — same limitation prior review rounds hit — so the above remains hand-traced against the code and fixtures, not a live run.

@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 41s —— View job


Code review complete

  • Gather context (git diff origin/main...HEAD, prior review threads, CLAUDE.md/REVIEW.md)
  • Identify what changed since the last code-review pass (commit ad9f226)
  • Verify the style fix (084285c) that addressed the outstanding nit
  • Re-check for any new correctness/design/test/doc issues
  • Post findings

Scope note: this repo has .github/workflows/claude-security-review.yml, so per REVIEW.md this lane owns correctness/design/tests/docs and leaves vulnerability-class findings to the security lane.

Delta since the last review round: git diff ad9f226..084285c touches exactly one file, one line:

check-security-binding.mjs:1522

-    isVcsControlPlane(canaries.find(isVcsControlPlane) ?? "") ? null : "a VCS-control-plane path (a `.git/`-relative entry)",
+    canaries.some(isVcsControlPlane) ? null : "a VCS-control-plane path (a `.git/`-relative entry)",

This is exactly the nit I flagged in the prior review round (the find() + re-apply-predicate construction was functionally identical to some() but inconsistent with the two neighboring lines). I re-read the surrounding block (check-security-binding.mjs:1501-1528) to confirm: missingShapes is still built the same way, the three shape predicates (isVcsControlPlane, isDotfile, "ordinary" = neither) are still mutually exclusive by construction, and this line now uses the same canaries.some(predicate) ? null : "..." idiom as the two lines directly below it. No behavior change — purely a maintainability cleanup, as the commit message states.

No other files changed in this commit, and no other commits landed since the last review pass. I have no new findings to report.

@kyle-sexton
kyle-sexton merged commit c1b4c62 into main Aug 11, 2026
34 checks passed
@kyle-sexton
kyle-sexton deleted the feat/autonomy-harden-isolation-probe branch August 11, 2026 23:10
kyle-sexton added a commit that referenced this pull request Aug 11, 2026
… last (#2171)

No linked issue

## Summary

`babysit_merge.branch_rules` reads the right endpoint —
`repos/{repo}/rules/branches/{branch}` — but folds it as if each rule
type appeared at most once. That endpoint returns one rule of a given
type **per ruleset** governing the branch, and the fold is a plain
assignment inside the loop, so each ruleset overwrote the previous one
and only the last survived.

Measured live on this repository. `main` is governed by two rulesets
carrying required contexts, both org-sourced:

| ruleset id | contexts |
| --- | --- |
| 17989001 | `pr-title / pr-title`, `do-not-merge / do-not-merge`,
`ci-status` |
| 19388547 | `security-review / security-review` |

19388547 is returned last, so the helper reported
`effectiveRules.requiredContexts` as **only** `["security-review /
security-review"]` — three of four required contexts silently dropped.
The single-rule assumption held under classic branch protection, which
has exactly one such rule. It does not hold under rulesets.

**Impact: a reporting and defence-in-depth defect, not a merge-safety
hole.** The gate refuses independently on `mergeStateStatus not in
READY_MERGE_STATES` (`{CLEAN, HAS_HOOKS}`), and GitHub integrates
required checks into that field — live `MergeStateStatus` introspection
gives `CLEAN: "Mergeable and passing commit status"`, `UNSTABLE:
"Mergeable with non-passing commit status"`, `BLOCKED: "The merge is
blocked"` — so a **failing** required context cannot present as
`CLEAN`/`HAS_HOOKS`. The **absent**-context case is derived from
required-status-check semantics, not observed: every required context
runs on every PR here, so there was no live PR to reproduce it against.
Unconditional `if failing:` / `if pending:` blockers built from the
whole rollup cover the rest. What the bug cost is the **explanation**:
`effectiveRules` and the `required checks not satisfied` blocker both
under-reported, so an operator could not see which contexts actually
govern.

One safety-adjacent consequence, in the **over-holding** direction.
`base_is_unprotected = not required_reviews and not
required_context_list`, and this repo's `pull_request` rule sets
`required_approving_review_count: 0`, so the flag hangs entirely on
`requiredContexts` being empty. Under the bug that meant "the **last**
status-checks rule is empty"; fixed, it means "**all** of them are".
"All empty" is a subset of "last empty", and both consumers of the flag
only ever *add* blockers — so the bug produced a **false hold** on a
superset of cases and never retired one. Latent here, since neither
ruleset carries an empty context list. It is not a fail-open.

## Fix

**Commit 1 — `required_status_checks`.**

- Accumulate contexts into a set across **all** rules, reported
`sorted()`. Deduped because two rulesets may legitimately require the
same context; sorted so the reported set is stable regardless of the
order the API returns rulesets in.
- Entries carrying no `context` are dropped rather than carried.
Previously a missing key produced a `None` that reached the
reconciliation loop and surfaced as a literal `"None"` required context;
it would also crash the new sort. This is a visible change in the
helper's output.
- `base_is_unprotected` needs **no code change** and is confirm-safe
once the union is correct: the union is empty only when no ruleset
requires anything, which is exactly what the flag means.

**Commit 2 — `pull_request`.** The same assign-in-loop shape sat three
lines below, in the same function. Not observed misreporting — exactly
one `pull_request` rule (ruleset 17988999) governs the branch today —
but nothing prevents a second, and a ruleset requiring 2 approvals
returned before one requiring 0 would have reported 0.
`requiredApprovingReviews` now takes the `max`,
`requireThreadResolution` the `OR`.

That fold direction is deliberately argued from safety, not from
GitHub's internal composition rule, which this change does not claim to
know: **max/OR can only ever over-report**, which holds a PR for a
human, where last-wins can under-report and release one.

This one could lose a blocker outright, not merely under-report: a
trailing `pull_request` rule with `required_approving_review_count: 0`
erased an earlier ruleset's requirement and **dropped the `needs N
approving review(s)` blocker**. Keep that distinct from the
`base_is_unprotected` consequence above, which runs the other way
(over-hold).

A malformed-but-present count reads as **one** review, never zero —
reading it as zero would be the single fail-open step in a fold whose
whole argument is that it can only over-report.

Severity split, kept separate on purpose:

- `requiredApprovingReviews` — a **fail-closed behaviour change**, not
currently firing. It feeds both `base_is_unprotected` and the `needs N
approving review(s)` blocker.
- `requireThreadResolution`, `requireSignatures`, `requireLinearHistory`
— **report-only**. Set into the summary, never consumed as a blocker;
the gate holds on unresolved threads unconditionally via `if threads:`.
They do not borrow the first item's severity.

Version bumped `0.51.5` → `0.51.6` with a matching CHANGELOG entry,
following the plugin's convention — every comparable
`fix(source-control)` commit in recent history (`cf743d61`, `ac27ea5a`,
`30be2a0b`, `e6ee72ef`) bumped the manifest version.

## Verification

New module `tests/test_babysit_merge_branch_rules.py` (7 tests), each
run against the fixed code and against the unfixed file:

| test | fixed | unfixed |
| --- | --- | --- |
| `test_contexts_from_every_ruleset_survive` | ok | **FAIL** —
`['security-review / security-review'] != ['ci-status', 'do-not-merge /
do-not-merge', 'pr-title / pr-title', 'security-review /
security-review']` |
| `test_a_context_required_by_two_rulesets_is_reported_once` | ok |
**FAIL** — `['ci-status'] != ['ci-status', 'pr-title / pr-title']` |
| `test_a_context_less_entry_is_dropped` | ok | **FAIL** — `[None] !=
[]` |
| `test_empty_trailing_rule_leaves_the_base_protected` | ok | **FAIL** —
`True is not false` (`baseUnprotected` flipped) |
| `test_the_strictest_approval_count_wins` | ok | **FAIL** — `0 != 2` |
| `test_thread_resolution_required_by_any_ruleset_survives` | ok |
**FAIL** — `False is not true` |
| `test_no_context_anywhere_still_reports_an_unprotected_base` | ok | ok
|

Six regress. The seventh passes both ways **by design** — it is the
over-correction guard, pinning that a genuinely context-less base still
reports unprotected. It is labelled as such in its class docstring so
nobody counts it among the regression tests.

`test_empty_trailing_rule_leaves_the_base_protected` asserts on
`evaluate()`'s `baseUnprotected` and blocker list, not on `branch_rules`
alone, and its fixture sets `required_approving_review_count: 0` — with
a non-zero count the flag would be `False` against the unfixed code too
and the test would prove nothing.

**End-to-end against a live CLEAN PR.** The fix feeds four contexts into
the reconciliation matcher where one went before, so a context that
failed to match its rollup entry would convert a silent under-report
into a spurious blocker. Ran `evaluate()` against #2150 (CLEAN, all four
contexts green):

```
requiredContexts: ["ci-status", "do-not-merge / do-not-merge",
                   "pr-title / pr-title", "security-review / security-review"]
requiredChecks:   all four found: true, satisfied: true, category: "success"
baseUnprotected: false   blockers: []   ready: true   mergeStateStatus: CLEAN
```

**Suite.** `bash
plugins/source-control/skills/babysit-prs/scripts/engine.test.sh` exits
0 — 612 tests OK, `ruff` (CI pin) clean, guarded-wrapper behaviour all
PASS. No shell files changed, so no shellcheck surface.

**Changelog parity.** `--check` and `--check-order` pass. `--check-bump
origin/main` passed 8/8 consecutive local runs on GNU Awk 5.4.0.
Recording that as an observation, not a health claim: the gate is
reported to have a SIGPIPE race after #2154, and local green does not
establish CI green.

**Not verified — recorded, not claimed.** A ruleset carrying **bypass
actors** is the one shape where GitHub could plausibly report `CLEAN` to
a bypassing identity while a required context is unmet; there the
unmet-required blocker would be the only defence, which raises the
severity of the under-report. Untestable here — every ruleset carries
`bypass_actors: []`. Likewise the absent-required-context case above.
Neither refutes the characterisation; both are open.

**Two things worth knowing about the union.** Adding `security-review /
security-review` does not mint a false blocker when that check skips:
`babysit_checks.py` treats `NEUTRAL`/`SKIPPED`/`SUCCESS` as success
states, so a name-stable skipped check still satisfies. And the
deliberately loose context matcher now processes four contexts where it
processed one — this **amplifies** pre-existing false-match exposure
rather than introducing it, which is exactly what the live `evaluate()`
check above is there to catch.

**Sibling scripts.** `babysit_resolve_thread.py` reads no branch rules,
and a repo-wide search for `rules/branches` / `required_status_checks` /
`effectiveRules` finds no other fold and no other consumer —
`babysit_merge.py` is the only one.
`plugins/source-control/skills/setup/SKILL.md:121` documents the same
endpoint to operators but instructs them to read the whole payload and
flag zero-reviews-and-zero-contexts repos, so it carries no
one-rule-wins assumption and needs no change.

## Related

Refs #2130, #2135 — reported as observed there.

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Aug 12, 2026
…e obligation (#2326)

## Summary

Makes an autonomous multi-run delivery pipeline expressible as policy:
verifier count, model
routing, and lens diversity bound per work class, with no vendor or
capability label anywhere on the
contract surface.

Five phases ship together. Phase 1 (the isolation probe hardening)
merged separately as #2150.

- **Verification-topology contract leaf** + a sixth guardrail-matrix
column. Verification policy was
expressible only as prose; it is now roles, relational constraints, and
predicates a binding can
actually evaluate. Floors ship as `min_checkers` and
`min_model_checkers`, tighten-only, on the
  agent-unwritable security binding.
- **`verification_topology`** as an optional top-level binding key
modeling all three axes. Absent
is not a hole — the shipped floors apply, exactly as
`escalation_severity` already does — so
`schema_version` stays `"1.0"` and every adopting org's binding keeps
validating.
- **The Q21 probe obligation**: an `L2`+ surface's policy layer must let
installed components narrow
reachability and never widen it, and the probe must cover every ratified
component-reachable
  destination in full.
- **Merge gating**: a class bound `auto` with a verification layer below
`blocking` is now an
invalid binding, and demotion cascades from the `C3` review cell to `C3`
auto-merge.
- **Two `userConfig` options** for lens selection and an advisory
narration lane, on the operator
  surface because neither can weaken a floor.

## The defect this work kept finding

Three times, in three different artifacts, the same shape: **a count
that does not guarantee the
coverage it exists for.** Two independent audit rounds on the contract
leaf each found one, and the
second was inside the repair for the first.

- `min_checkers` counted role TYPES, so a class declaring `[A, A, B]`
satisfied a floor of 3.
- After that repair: relations and predicates bind only
model-adjudicated slots, so three
DETERMINISTIC slots met `C4`'s floor while `cross_vendor_required` bound
an empty set of model
judges and was vacuously satisfied — a binding valid with **no model
judge at all**, against a
  matrix cell that mandates AI review. Closed by `min_model_checkers`.
- Phase 1's review round had already found four findings of this shape
(`example.com,example.com`
  passing as two targets; `same,same,same` as three shapes).

The generalization, now recorded in the contract: **whenever a rule
counts things, ask what it would
accept if every counted thing were identical.** Every count-shaped check
here asserts distinctness
and coverage rather than length, and each has a fixture that would pass
a length-only
implementation.

## Two things only running the code revealed

- **A rule pair, each individually sound, was jointly unsatisfiable.**
The pairwise-distinctness
check demanded a relational constraint between every checker pair, while
the deterministic/model
split rejects those same constraints on deterministic slots — so a
conforming binding could not be
written. Scoped to model slots, since a cross-kind pair is distinct by
construction.
- **Phase 5's check as briefed could not be built.** The topology floors
are tighten-only and
floor-bounded, so no value a binding can set describes a topology
incapable of unanimity: the
check would never fire and its fixture pair could never be constructed.
The real failure is a
JOIN — the floors supply the checker population, the security-review
knob supplies its force, and
only force is configurable into absence. A verifier then reached the
forbidden state *dynamically*,
through the exact resolution the checker tells consumers to perform,
which is why
  `PROMOTION_DEPENDENCIES` gained the review cell.

## Why the Q21 host set rides the binding, not the transcript

A first design put it behind a CLI seam. Running it showed the no-hosts
sentinel binding `L2` with
zero evidence, and the seam put the fact the check turns on where no
reviewer of either the binding
or the capture could see it. `substrate_class` already set the precedent
in the same function, for
the same reason: a transcript value is capture evidence the executing
agent could doctor.

## What ships unverified, stated rather than implied

- **Per-run aggregation is not asserted.** Unanimous pass, single
dissent, checker timeout, and
duplicate checker identity at run time are runner-seam obligations; no
runner exists to exercise
them. What ships is the contract obligation plus a binding-validity
rule.
- **Force is checked; RESOLVED distinctness is not.** Two slots held
distinct by declared
  constraints can still resolve to one instance at run time.
- **No capture can prove the ratified host set is COMPLETE.** A
component requesting an unratified
destination is invisible to every transcript — that is where the human
ratifying the binding takes
responsibility. Nothing at all proves the probe ran with those
components installed.
- **The narration lane is inert as shipped:** its precondition is an
upstream deterministic
  comparator this repository does not ship.

## Breaking changes

Both are deliberate bar raises with readable remedies in the check
output, documented in the
CHANGELOG:

1. Every `L2`/`L3` level binding needs `component_reachable_hosts`; a
level without it is UNPROVEN.
The empty list is valid and meaningful — it claims the surface installs
nothing carrying policy
   rules of its own.
2. A class bound `auto` merge with a layer below `blocking` is now
invalid.

## Test plan

- `node
plugins/autonomy/skills/setup/scripts/check-security-binding.fixtures.test.mjs`
— **540
checks, 154 fixtures, 0 quarantined.** Manifest diff verified
**semantically** purely additive at
every step: 22 fixtures added, 0 removed, **0 pre-existing
`findings_substrings` modified.**
- Fixture pairs prove each rule fires rather than merely existing: an
all-deterministic `C4` binding
and a `cross_vendor_required` class with one model slot both exit 1; the
auto-merge pair is one
  token apart and exits 1 / 0.
- `node scripts/validate-plugin-contracts.mjs` — 45 setup skills, 2377
files. This gate caught a
  product path in draft contract prose.
- `bash scripts/check-contract-slice-prune.sh --check-diff origin/main`
— passes.
- `python scripts/sync-plugin-options-docs.py --check` and `bash
scripts/check-changelog-parity.sh --check`
  — both pass; neither was named in the phase briefs.
- `grep -rniE "frontier|flagship|daily driver"
plugins/autonomy/reference/` — empty.
- `grep -c blocking` over the narration lane's section — 0. `grep -c
visual` over the
  security-binding schema — 0.
- Plugin manifest change gated on a live fetch of
<https://code.claude.com/docs/en/plugins-reference.md>
  (2026-08-11).

## Related

Closes #2110.

Phase 1 shipped separately as #2150. The per-label rationale for
rejecting capability labels, and
the measurement behind the independent-aggregation invariant with its
confidence grade and the
corpus disagreement, are recorded as a comment on #2110 — deliberately
outside the contract surface,
because a single-study figure parked in a normative artifact strands the
contract when a contrary
result lands.

The approved implementation plan — the pruned contract slice — is posted
as the first comment below.

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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