Repository navigation
Hardening: audit, accuracy fixes, isolation and credential fixes, tests, CI - #1
Merged
Merged
Conversation
- engines node >=24 <25, .nvmrc, packageManager pnpm@12.8.1 - typescript 7, tsx 4.23, commander 15, execa 10, zod 4.6, @types/node 24 - drop the package.json pnpm field (ignored by pnpm 12; pnpm-workspace.yaml allowBuilds already covers esbuild) - tsconfig: noEmit; drop unused jsx/declaration/outDir options - experiment subjects (@playwright/cli, @playwright/mcp, github-mcp-server) stay pinned so new runs remain comparable with committed data
- eslint (typescript-eslint recommended) and prettier with a config that matches the existing style; run artifacts, fixtures and docs excluded - remove unused readJsonIfExists (playwright tier2) and mkSeed (trialState) - drop a no-op `\$` escape in the directed-prompt template; the rendered prompt strings for all 14 tasks were hashed before and after and are byte-identical - hold TypeScript at 6.0.3: typescript-eslint 8.71 refuses TS 7.0
Isolation - pass a positive built-in tool list per arm with `claude --tools`; `--allowed-tools` never restricted anything under bypassPermissions - deny Read/Edit of ~ and the repo via --settings, so file tools can't reach the developer's home directory or other trials' answers - run the GitHub skill arm's Bash in Claude Code's OS sandbox with no unsandboxed fallback (blocks the gh keyring login and $HOME reads) - classifier is now an allow-list derived from the same per-arm list; MCP resource tools count as MCP surface, planning tools as neutral Credentials - execa merged process.env back into the child (extendEnv defaulted to true), so the GH_/GITHUB_ scrub, including 90d0dd7, never removed anything; pass extendEnv: false and scrub by prefix - give each trial an empty GH_CONFIG_DIR Measurement - timed-out trials recorded 0 tokens and 0 ms; sum per-message usage as a flagged lower bound and use the harness-measured duration - report gains a Timeouts column and a note on estimated tokens Robustness and config - task cleanup (sandbox repo deletion) and temp-dir removal now run in finally; setup failures no longer leak repos or dirs - central config.ts (model, timeout, tool lists) and zod-validated GitHub env; .env is loaded by the harness script - CLI validates --trials/--tier/--arm/--run and reports unknown experiments without a stack trace - verify-arms runs each arm in a trial tempdir and prints the tool list from the init event; gains the documented --arm option - redact-artifacts command and redaction of home paths and out-of-trial file-tool results on every transcript the runner writes - small logger with LOG_FORMAT=json and token redaction - shared helper-word pattern; Playwright no longer flags helper names inside URLs/JS, and both classifiers now treat base64 as a helper
- classifiers, shell parsing, metrics (incl. timeout estimation), env scrub and token routing, trial settings, config validation, logger redaction, fixture server (traversal, 413, 405), redaction, seeds - end-to-end runTrial with a fake claude executable over the real tier1_scrape fixture: scoring, redaction, cleanup, timeout path - CLI argument validation and GitHub preflight - pnpm test / pnpm test:coverage
Success-check notes quote output paths under the repo, which live under the home directory. Found in a live smoke run.
Redaction (pnpm harness redact-artifacts, plus two literal edits) - home directory paths -> ~ in all 306 affected artifact files - results of baseline file-tool calls outside the trial directory (~/.gitconfig, ~/.claude.json, shell rc files, gh config, home-dir listings) replaced with a marker; the calls themselves are kept as escape evidence - the author's email quoted in two transcripts' reasoning text replaced with <redacted-email> No credentials were present (gitleaks: 0 findings in tracked files). Recompute (pnpm harness recompute-metrics; github-rw for Tier 2 runs) - 65 timed-out trials now carry lower-bound token estimates and the timeout as wall clock instead of 0 / 0 - allow-list classifier flips 4 trials to invalid surface, all failed trials: 3 GitHub n5 mcp tier1_issue_triage (Grep/Glob) and 1 Playwright n5 baseline tier1_login (TaskOutput) - no pass/fail values changed
- regenerate the report tables in five findings.md files and both smoke report.md files from the recomputed results; narratives kept - GitHub n5: issue_triage was a token-permission failure (403 on every issue call in both arms), and all 5 skill passes used escalated credentials (controller token or the developer's gh login); the env-scrub change did not take effect at the time; commit 64f9436 -> e836d26; MCP turns 58-78, not 75+ - Playwright n5: Tier 2 ratio 2.01x -> 1.86x (1.52x completed trials), tier2_recovery MCP 40% not 70%, cost mechanism and recovery failure description corrected against transcripts - directed-n5: 2.6x -> 2.45x, escape-rate wording, invalid-trial count, compliant-trial walkthrough - tier2-n5, workflow-n5, smoke findings: dependent claims corrected Each corrected narrative carries a dated correction note.
README: summary, why it matters, Mermaid architecture and module table, isolation as implemented, prerequisites and a quickstart verified from a clean clone, configuration and command tables, tasks, corrected N=5 results, status and limitations, development commands, license. Claims removed or corrected: FNV-1a paired seed (it is SHA-256), --allowed-tools as a restriction, verify-arms --arm (now implemented), .env loading (now implemented), numTurns in the schema, redirections invalidating segments, ef3fc97, 2.01x, 75+ turns, 'triples the cost', and the issue_triage fan-out story. CLAUDE.md: describe --tools, deny rules and sandbox; env scrub with extendEnv false; redaction; don't point report --output at findings.md
- remove the v1 pages (cli_mcp_experiments, github_experiment, playwright_experiment) and every link to them; index.html already redirects to the v2 overview - add a "Corrections (October 2026)" note to the overview, linked from the GitHub and Playwright pages - issue_triage: token-permission failure (403s), escalated credentials in all skill passes; MCP turns 58-78; pre-Actions workflow numbers marked as not recorded - Playwright: Tier 2 ratio 1.86x (1.52x completed trials), recovery 40%, cost explained by per-field fill vs batched browser_fill_form, recovery failures described as stalls with no determined cause - directed prompt: pass-and-valid 1/5, about 2.2-2.5x tokens - isolation described as implemented now, noting n5 predates it - remove two images that illustrated disproven claims (playwright-cycle-cost.png: action-then-snapshot loop; surface-escape.png: fan-out into timeouts)
- LICENSE (MIT); NOTICE and third_party/playwright-cli/LICENSE for the Apache-2.0 skill adapted from @playwright/cli, with its changes listed - SECURITY.md (private reporting, supported versions, threat model), CONTRIBUTING.md, CHANGELOG.md for 1.1.0, .editorconfig, .env.example - issue and PR templates - .gitignore covers .env.*, editor files, coverage - fake test tokens labelled; test/ excluded from GitHub secret scanning and gitleaks - version 1.1.0; prettier formatting of .mcp.playwright.json
- CI on push to main and pull requests: frozen install, lint, format check, typecheck, tests with coverage (summary in the job summary), CLI smoke, and a lychee link check of markdown and the docs site - CodeQL for javascript-typescript and actions, weekly schedule - top-level permissions contents: read; security-events: write only on the CodeQL job; actions pinned to commit SHAs; concurrency groups; per-job timeouts; checkout without persisted credentials - Dependabot for npm (grouped dev tooling) and GitHub Actions; the @playwright/cli experiment subject and TS/@types/node majors ignored
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
- resolve request paths with path.resolve and require them to stay under the root (CodeQL js/path-injection); test a sibling-prefix traversal - return generic 413/500 bodies instead of echoing error strings (js/stack-trace-exposure) - exclude committed run artifacts (agent-written scripts) from CodeQL
Under Claude Code's Bash sandbox, Go CLIs like gh fail TLS verification on macOS (x509: OSStatus -26276), so the GitHub skill arm couldn't use gh at all and fell back to curl. Set sandbox.enableWeakerNetworkIsolation, the documented setting for this; re-verified that the keychain and $HOME stay blocked. Adds the live isolation-check run (Tier 1 pr_diff_answer x3 arms, Tier 2 issue_create x2 arms) as evidence, documents the remaining $TMPDIR read reach, and corrects the audit's earlier sandbox claim.
The runner only calls Task.cleanup once setup returns state, so a setup that failed after creating its repo (seen in n5-v2: workflow_status couldn't list Actions runs) leaked the repo, and the next arm's trial for the same seed then failed with 'name already exists'. - provisionRepo seeds inside deleteOnFailure - tier1_pr_diff_answer and tier1_workflow_status run their post-provision steps inside deleteOnFailure - tests with a mocked GitHub API; integration cleanup assertion now checks this trial's own dirs so it is safe alongside live runs Rendered prompts unchanged.
- Claude Code 2.1.288 sometimes records tool input as
{__unparsedToolInput: {raw}}; the parser saw no command and flagged
ordinary gh calls as escapes (2 n5-v2 workflow_status trials). Unwrap
raw (with a fallback for truncated JSON) before classifying.
- tier2_issue_create's success check listed issues 6x500 ms; GitHub's
list lagged a correct create in n5-v2 (false negative). Now 12x1 s.
Rendered prompts unchanged.
…ub-rw n5-v2 stores Tier 1 (read-only) and Tier 2 (read-write) GitHub results in one run; each tier must be re-classified with its own spec.
Claude Code 2.1.288, claude-sonnet-4-6, @playwright/cli 0.1.13, @playwright/mcp 0.0.75, github-mcp-server v1.0.4; harness 29b7e6a (tier1_workflow_status re-run on 8a6cc40 after the controller token gained Actions: read). skill/mcp N=5, baseline N=2; directed variant skill N=5. 161 trials. Metrics recomputed with 40521c2 (unparsed tool input); GitHub Tier 2 classified with github-rw. Lane logs record versions. No tokens, home paths, or leftover sandbox repos.
- README Results: n5-v2 headline (MCP clean on every task; skill/MCP 1.5-2.1x, form 2.67x; base64 escapes reproduced; issue_triage valid at 1.67x; directed 0/5 pass+valid) and a First run (n5) section that explains what changed and why ratios rose - n5-v2 findings with narrative for both experiments - GitHub setup documents read-only and read-write agent tokens - CHANGELOG and AUDIT addendum for the re-run and the fixes it surfaced
- hero metrics, findings, tables and charts from the n5-v2 data: MCP clean on all 13 tasks; skill/MCP 1.5-2.1x (form 2.67x); skill 0/10 valid on repo_inventory and file_patch_pr; issue_triage 1.67x; recovery 5/5; directed 0/5 pass+valid at 2.63x tokens - bar charts recomputed (largest skill bar = 100%: Playwright checkout 266,073; GitHub issue_workflow 100,251) - the October corrections note becomes a 'First Run (May 2026)' section (anchor #first-run, old #corrections anchor kept) with n5 figures and why they differ; per-run setup described
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hardening pass: audit, accuracy fixes, isolation and credential fixes, tests, CI, and repo hygiene. The full audit with evidence is in
AUDIT.md(§2 lists every claim; the addendum covers what came up while implementing).1. Summary of changes
Security and isolation (
d999386,a6ed4ec)claude --toolslist.--allowed-toolsnever restricted anything underbypassPermissions; agents had Read, Glob, Grep, Edit and more in every arm.~and the repo through--settings.allowUnsandboxedCommands: false,denyRead: ["~/"]). This blocks the keychain path to the developer'sghlogin. Verified live:gh auth tokenandsecurity find-generic-passwordare both BLOCKED.extendEnv: falseand removes everyGH_*/GITHUB_*variable.GH_CONFIG_DIRpoints at an empty directory per trial.~, and file-tool results for paths outside the trial directory are replaced with a marker.Measurement correctness
tokensEstimated, and the timeout as wall clock.base64as a helper.9450f9e). 4 verdicts flipped to invalid surface, all on failed trials; no pass/fail value changed.Data hygiene (
9450f9e)~/.gitconfig), fragments of~/.claude.jsonand shell rc files,ghconfig, and home-directory listings naming private projects.Design
config.tsholds the model, timeout and tool lists, plus a zod-validated GitHub config;.envis now actually loaded.finally, so a failed trial can't leak a sandbox repo.--trials abcand--run ../xare rejected); no stack traces for unknown experiments.redact-artifactscommand; a small logger with JSON output and token redaction.mkSeed,hasShellRedirection,LoadedExperiment, unusedagentEnv,readJsonIfExists, and the guessed MCP tool-name lists.verify-armsnow starts each arm exactly like a trial and prints the real tool list from theinitevent. It also gained the documented--armoption.Tests (
dd15e74)runTrialend to end with a fakeclaudeover the realtier1_scrapefixture: scoring, redaction, cleanup, and the timeout path. CLI tests cover argument validation and preflight.Currency (
ffcff61).nvmrc,engines) and pnpm 12.8.1 (packageManager).pnpm auditfinding, a low-severity esbuild advisory), and @types/node 24.@playwright/cli0.1.13 (latest 0.1.22),@playwright/mcp0.0.75 (0.0.83),github-mcp-serverv1.0.4 by digest (v1.13.0), modelclaude-sonnet-4-6. Upgrading them changes what is measured, so they should move with a new run.Docs (
f11b5c4,4916b83,182f56f)index.htmlstill redirects to the v2 overview, so Pages keeps working.playwright-cycle-cost.pngandsurface-escape.png.CI/CD and hygiene (
2066769,e8b9ccd)contents: read, SHA-pinned actions, concurrency groups, per-job timeouts.third_party/for the Apache-2.0 skill adapted from@playwright/cli, SECURITY.md, CONTRIBUTING.md, CHANGELOG.md (1.1.0),.editorconfig, issue/PR templates,.env.example, and a broader.gitignore.test/is excluded from GitHub secret scanning and gitleaks.2. Claims table
Status is the final state after this PR. "Fixed" means the claim was wrong and has been corrected (or the code now makes it true); "Verified" means it was already right. Docs-site claims (182 rows) are in AUDIT.md §2c; the rows below include every README claim plus the docs-site items that changed conclusions.
@playwright/mcp@0.0.75; digest-pinnedgithub-mcp-serverexperiment.ts,.mcp.*.json; digest = v1.0.4 (GHCR manifest match)metrics.ts, testmetrics.test.ts)trialState.test.ts;fixtureServer.tstrialState.ts:mkPairedSeed--allowed-toolslist"; per-arm "Allowed" tables; skill arm without Read/Glob/Grep--tools)initevents listed Read/Glob/Grep/Edit… in all arms;verify-armson 2.1.287 now shows exactly the configured lists--setting-sourceskeeps~/.claudeskills outSkilltoolconfig.ts,--disallowed-toolsclaude-sonnet-4-6; 240 s timeoutconfig.tsverify-armsverifies surfacesinit.tools, fails on extrasverify-armsoutput belowtier1.ts,trialState.test.tsnumTurns;reasonontoolCallsmetrics.tsmetrics.test.tsclassifiers.test.tsbase64shell.ts:SHELL_HELPERS.env)--env-file-if-exists=.envinstall-browserstep added)verify-arms --armcli.tsexperiments/playwright/runs/n5/findings.mdfillvs 8snapshot(skill); MCP 5browser_fill_formissue_triage: MCP fan-out at 75+ turns; skill viagh issue list --labelef3fc97"90d0dd7, and noted it didn't take effect)git logdirected-n5/resultstools)experiment.ts--allowed-toolsrestricts; scrub coversGH_*/GITHUB_*runner.ts,isolation.test.tstier2_recovery70%64f9436"e836d26)git log3. Wording and numbers that changed
The headline results now come from
n5-v2, a full re-run (161 trials) under the enforced isolation on Claude Code 2.1.288. The pinned subjects and model are the same as before. The Mayn5run is kept as a "First run" section.Use these now:
base64 -dand shell variables whereghlacks a primitive. The result reproduced across both runs."Replace the old figures:
n5-v2.ghlogin), because the environment scrub silently didn't apply; the harness now sandboxes the CLI arm and removes all inherited GitHub credentials."Explain the ratio change honestly (optional). Ratios rose from the first run because each turn's fixed context shrank (fewer tool definitions, newer Claude Code), which helps the smaller MCP arm proportionally more. The direction held in both runs; the exact multiple depends on the setup.
4. Recommended repository settings (not applied; run these yourself)
The default branch is already
main; no rename needed.5. Open items not addressed
Your
ghOAuth token was exposed in my local session log. During testing, an unsandboxed probe printed it. It never reached the repo or the PR. Please rotate it (gh auth refresh, or revoke it in Settings → Applications).Git history still contains the personal data that
9450f9eredacted: your email, config-file fragments, and home-directory listings. Your ground rules forbid rewriting history. If you want it gone, you'd have to rewrite history yourself (git filter-repo) and force-push; forks and caches may keep copies. No credentials are in history (gitleaks: 0).One tool-call argument names a private folder (
~/Documents/Projects/mcp-sub-registry, in a Glob path). I kept it as escape evidence; it's a one-line edit to drop it.Live GitHub verification: done (commit
29b7e6a, after the tokens were rotated).ghcouldn't verify TLS inside the macOS sandbox. Fixed withenableWeakerNetworkIsolation; the keychain and$HOMEstay blocked (re-tested).experiments/github/runs/isolation-check: all arms valid; skill and mcp pass Tier 1pr_diff_answerand Tier 2issue_create; baseline fails as expected.Residual: baseline file tools can still read outside
~and the repo (e.g.$TMPDIR). The results are redacted from artifacts. Full confinement needs a container.Re-run done (
n5-v2). 161 trials under the current isolation. It surfaced and fixed 4 harness issues: a setup repo leak (8a6cc40), unparsed tool input (40521c2), a short issue-create checker window (40521c2), and Tier 2 re-classification (8d113c0). Onetier2_issue_createskill trial is a recorded checker false negative.Playwright skill arm is not OS-sandboxed. It needs local browsers and a loopback server, so it relies on the classifier and file-tool deny rules; this is documented in SECURITY.md.
Coverage is 41% of lines overall. Core logic is well covered:
The total is pulled down by three things:
provisioner.ts: they call the live GitHub API, and I didn't add an HTTP mock layer.cli.ts: it is exercised in subprocess tests, which v8 coverage doesn't count.tier1_scrape.There's no build artifact. The harness runs from source via tsx, so CI's "build" step is a CLI smoke test (
pnpm harness --help).Apache-2.0 change notice for the skill is in NOTICE rather than inside SKILL.md, because SKILL.md is the experimental stimulus.
Redesign proposals not done:
.claude/skills/codex-imagegen/was untracked in your working tree before this work. I left it alone and didn't commit it.6. Phase 3 verification (fresh clone of this branch)
Toolchain and checks:
Quickstart, run as written in the README:
Other checks:
CI on this PR (head
896e5c7): all green.The first CI run flagged 4 CodeQL alerts:
js/path-injection, fixture server.js/stack-trace-exposure, fixture server (3 occurrences).Commit
896e5c7fixes them: the path check usespath.resolveplus a containment test, errors return generic 413/500 bodies, and run artifacts are excluded from CodeQL.