Audit and harden Claude Code commands: retire 3 stale, purge dead GitHub MCP for gh CLI, add argv-safe helpers#711
Merged
Merged
Conversation
…gv-safe helpers Ships the injection-safe, tested subset of the command audit. Two helpers route every untrusted value through argv-safe Python; the destructive worktree-rm rewrite was reverted to main after its prose kept reintroducing shell-injection (see "Reverted" below). MCP is dead, not merely disfavored. No GitHub MCP server is configured (only playwright), so every mcp__github__* call hit an empty tool namespace. Replaced with the `gh` CLI in submit-pr and push-pr-update, and the /ai-review comment post (which double-fires the auto reviewer) removed. Shell-injection closed structurally with two tested helpers: - .claude/scripts/pr_prepare.py — resolves/validates the PR title, branch, and base. Untrusted values arrive as files (written by the Write tool, never a shell) and reach git/gh only as quoted, validated variables. Commit messages use `git commit --file`, not a heredoc. - .claude/scripts/premerge_scan.py — discovers changed files via `git … -z` argv arrays, runs the methodology pattern checks (A–D) as pure-Python regex/AST over file *content* (opening a path never executes a filename), resolves test coverage, screens metacharacter paths, fails closed on git errors, and emits NUL-delimited run-lists consumed via `xargs -0 … < list`. Status-aware (added/modified/deleted, untracked). pre-merge-check, submit-pr, and push-pr-update call the helpers instead of greping filenames. Regression tests place `$(touch sentinel)` payloads in titles, commit messages, and staged+untracked filenames and assert nothing executes. Retired three commands: - review-pr: 10 lifetime uses, dead MCP calls, posted publicly with no confirmation, incompatible severity scale. /ai-review-local + CI codex cover the space. - validate-tutorial: invoked `jupyter nbconvert`, never installed. Folded into pre-merge-check as an nbmake check. - dev-checklists: no unique content; pointed at a TODO.md section that no longer exists. The load-bearing rules (np.errstate, fix-all-sites, parameter downstream-propagation) moved to CONTRIBUTING.md. worktree-new: no longer silently branches from current HEAD when it is not the default branch (asks first); resolves the default branch via `git ls-remote --symref` (not `gh repo view <alias>`); and pins VIRTUAL_ENV for `maturin develop` so a rebuild lands in the worktree's own venv. Reverted (regressing prose, kept out of this PR): - worktree-rm.md → main. The destructive-logic rewrite (confirmation gate, detached-HEAD rescue, state persistence) repeatedly reintroduced shell-injection — most recently by sourcing a state file built from a git-derived branch name. Reverted to the known main version; a from-scratch worktree-rm helper is the right way to land those safety improvements. - push-pr-update committed-range scan removed rather than interpolate a raw comparison ref into a shell command; committed-change review is deferred to /pre-merge-check. Commit and PR bodies carry no authorship trailers.
…e branch is already committed and pushed The revised submit-pr keyed its exit on 'is there work to push', but the standard flow commits at /ai-review-local (which requires a commit) and may push during a rebase/prep step — so a committed, already-pushed branch hit 'nothing to submit' and never opened its PR. Reframe the contract to 'ensure this committed branch is pushed and has an open PR', idempotent: push only when there are unpushed commits or no upstream, then open a PR only if 'gh pr view' shows none. Uncommitted changes (which never went through /ai-review-local) warn and offer commit-or-abort rather than being silently absorbed.
Overall AssessmentExecutive Summary
MethodologyNo findings. No registered causal method is affected, so paper-level cross-checking is not applicable. Code Quality1. Clean zero-diff state creates and pushes a useless branch
2. Any open PR satisfies the idempotency check
3. Failed scans can leave stale run lists
PerformanceNo findings. MaintainabilityNo additional findings. Tech DebtTracked limitations
SecurityNo unmitigated findings. File-based ingress, argv-based subprocess calls, quoting, and Documentation/TestsMissing state-machine regression coverage
Path to Approval
|
…ogic) - submit-pr zero-diff guard (P1): the idempotent reframe removed the 'nothing to submit' exit entirely, so a clean branch with zero commits ahead of base would create+push a useless branch and then fail gh pr create. Restore the exit, scoped to 'zero commits ahead of $BASE_REMOTE/$BASE_BRANCH' (not the old 'no unpushed commits', which wrongly caught committed-but-pushed work). - submit-pr PR-match idempotency (P1): the existing-PR check read only url/state, so any open PR counted as success even with a different --base/--draft. Now compares baseRefName/isDraft and reports a mismatch instead of false success. - premerge_scan run-lists (P2): truncate run-tests.z/run-notebooks.z at startup so an early exit (3/4) cannot leave a prior run's stale lists in the reused deterministic scratch dir. Regression test asserts a seeded stale list is cleared on a git-error exit. - Contract tests assert the zero-diff guard and base/draft comparison are present.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment✅ Looks good — both prior P1 findings are resolved; no unmitigated P0/P1 remains. Executive Summary
MethodologyNo findings. The diff does not affect any method registered in Code QualityScan failure is not explicitly terminal
PerformanceNo findings. MaintainabilityControl-group review guidance was lost
Tech DebtTracked limitations
SecurityNo findings. File-based untrusted input, argv-based subprocesses, and Documentation/TestsExisting-PR contract test is too permissive
Validation note: helper syntax and pure-function smoke checks passed. The focused pytest suite could not run because |
- Document premerge_scan exit 4 (git/read failure = incomplete scan) in all three callers (pre-merge-check, submit-pr, push-pr-update) with an explicit 'stop and report, do not continue on an empty scan' instruction. - Restore the control/comparison-group composition check (lost with dev-checklists deletion) to CONTRIBUTING.md Implementation Guidelines: verify the comparison group on new code paths, not just the default. - Tighten the existing-PR contract test to assert baseRefName/isDraft are on the executable 'gh pr view --json' line, not merely somewhere in the prose.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment⛔ Blocker — one unmitigated P0 security vulnerability remains. Executive Summary
MethodologyNo findings. No registered estimator or statistical behavior changed, so paper-level comparison is not applicable. Code QualityNo findings. PerformanceNo findings. MaintainabilityNo findings. The prior control-group guidance finding is resolved in Tech DebtTracked limitations
Security[Newly identified] Repository refs still reach shell source unsafely
Documentation/TestsNo separate findings. The prior existing-PR contract-test weakness is resolved at Validation note: Python syntax and pure-helper smoke checks passed. The focused pytest suite could not run because Path to Approval
|
…ng placeholders (CI P0) Pre-existing (17 sites on main): <comparison-ref>/<default-branch> were pasted unquoted into git fetch/rev-list/diff. git accepts $()/backticks in ref names, so a hostile default-branch/ref could execute. Resolve DEFAULT_BRANCH and COMPARISON_REF into shell variables (deterministic; re-derived per block since variables don't persist across tool calls) and use only quoted "$COMPARISON_REF..HEAD" forms. Contract test asserts no raw <comparison-ref>/<default-branch> in an executable bash line. Verified the resolution block runs clean under zsh.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment✅ Looks good — no unmitigated P0 or P1 findings. Executive Summary
MethodologyNo findings. The PR does not change statistical behavior, so Registry or paper comparison is not applicable. Code QualityNo findings. PerformanceNo findings. MaintainabilityNo findings. Tech DebtTracked limitations
SecurityNo findings. The prior P0 is fixed in Documentation/TestsHostile-ref regression test is only textual
Validation note: both helper scripts parse successfully. The focused pytest, Ruff, and mypy checks could not run because those tools are unavailable in the review environment. |
The prior contract test only rejected the raw <placeholder>; a bare unquoted $COMPARISON_REF..HEAD would still be injectable. Add (1) a quoting assertion — every $COMPARISON_REF/$DEFAULT_BRANCH use in a bash block must be quoted, and (2) a subprocess regression proving a ref value holding $(...) as data is inert when used through the command's quoted "$REF..HEAD" form.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment✅ Looks good — no unmitigated P0 or P1 findings. Executive Summary
MethodologyNo findings. No statistical methods are affected, so paper or Registry comparison is not applicable. Code QualityNo findings. Both new helper scripts parse successfully. PerformanceNo findings. MaintainabilityNo findings. Shared scanning and PR preparation logic is centralized in tested helpers. Tech DebtTracked command limitations
SecurityNo findings. Ref resolution and use remain safe in Documentation/TestsNo findings. The previous P2 is addressed by the quote contract and sentinel subprocess regression in Validation note: pytest, Ruff, and mypy were unavailable; static checks found nine fenced ref-variable uses, all quoted, and zero raw executable ref placeholders. |
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
review-pr— 10 lifetime uses, dead MCP calls, posted publicly with no confirmation, severity scale incompatible with the rest;/ai-review-local(pre-PR) and CI codex (post-PR) cover the space.validate-tutorial— its core commands invokedjupyter nbconvert, which is not (and never was) installed; folded intopre-merge-checkas annbmakecheck.dev-checklists— no unique content and pointed at aTODO.mdsection that no longer exists; the load-bearing rules moved toCONTRIBUTING.md.mcp__github__*call hit an empty tool namespace.submit-pr/push-pr-updatenow use theghCLI, and the redundant/ai-reviewcomment post (which double-fires the auto reviewer) is removed..claude/scripts/pr_prepare.py— PR title/branch/base arrive as files (written with the Write tool, never a shell) and reach git/gh only as quoted, validated variables; commit messages usegit commit --file, not heredocs..claude/scripts/premerge_scan.py— discovers changed files viagit … -zargv arrays, runs the methodology pattern checks (A–D) as pure-Python regex/AST over file content (opening a path never executes a filename), screens metacharacter paths, fails closed on git errors, is status-aware (added/modified/deleted, untracked), and emits NUL-delimited run-lists consumed portably viaxargs -0 … < list.submit-pris now idempotent — its goal is "ensure this committed branch is pushed and has an open PR," so a committed, already-pushed branch opens its PR rather than dead-ending at "nothing to submit" (the standard flow commits at/ai-review-local, so submit-pr always runs on already-committed work).worktree-newno longer silently branches from a non-default HEAD (it asks first), resolves the default branch viagit ls-remote --symref(notgh repo view <alias>), and pinsVIRTUAL_ENVformaturin developso a rebuild lands in the worktree's own venv.tests/test_command_contract.py) enforce these safety properties against the command prose, so a later edit can't silently reintroduce filename/value interpolation or a heredoc commit.Deferred (tracked in
TODO.md):worktree-rm's destructive-logic rewrite (confirmation gate, detached-HEAD rescue) was reverted to itsmainversion because editing that prose repeatedly reintroduced shell-injection; restoring those guards wants a from-scratchworktree_rm.pyhelper. Also tracked: a staged-blob refinement forpremerge_scan.py, and re-adding committed-range scanning topush-pr-updatevia the helper's--rangemode.Methodology references (required if estimator / math changes)
Validation
tests/test_pr_prepare.py,tests/test_premerge_scan.py,tests/test_command_contract.py,tests/test_commit_message_safety.py— including injection regression tests that place$(touch sentinel)payloads in PR titles, commit messages, and staged + untracked filenames and assert nothing executes. All green; ruff / black / mypy clean on the helpers.Security / privacy