Skip to content

fix(hooks): allow uv add/remove; pin apm install/uninstall as allowed - #1323

Merged
tvna merged 4 commits into
mainfrom
claude/relax-bash-safety-check-euma0j
Aug 25, 2026
Merged

fix(hooks): allow uv add/remove; pin apm install/uninstall as allowed#1323
tvna merged 4 commits into
mainfrom
claude/relax-bash-safety-check-euma0j

Conversation

@tvna

@tvna tvna commented Aug 25, 2026

Copy link
Copy Markdown
Owner

Summary

Relax hooks/check-bash-safety.sh's install-verb deny list to allow uv add/uv remove (declarative, PR-diff-visible dependency changes) while keeping uv pip install/bare uv install denied, and pin the already-allowed apm install/apm uninstall with regression tests.

Facts

  • Operator request (paraphrased from Japanese): "check-bash-safety.sh is too strict and blocks uv package installs; relax it while keeping safety."
  • Paused via the human-harness skill before changing anything, since docs/security-control-inventory.md's LLM05/ASI02 row cites this deny rule as a [deny] supply-chain-risk control -- a blanket removal would be disabling a security control (compliance.md's "Not overridable" class).
  • Asked the operator via AskUserQuestion how far to relax it; they picked the recommended option: allow only uv add/uv remove (mutate pyproject.toml/uv.lock, visible in the PR diff), keep uv pip install/bare uv install denied (install into the venv with no diff trail).
  • Follow-up ask: "also relax apm install/uninstall." Investigation found neither hooks/check-bash-safety.sh nor its sibling skills/executing-a-branch-plan/scripts/check_task_bash_safety.sh has any pattern matching "apm" -- both subcommands are already fully allowed. Live-verified via the apm CLI's own --help output that install/uninstall are real subcommands.
  • Issue: fix(hooks): allow declarative uv add/uv remove in check-bash-safety.sh; pin already-allowed apm install/uninstall #1320 (Acceptance Criteria Map drafted there via drafting-issues).
  • Operator caught a second drift after this PR was already in draft: .gitapex/ssot.json's registered target[0].ref pattern string for this gate still listed uv add among the denied patterns, unsynced with the actual install_re change -- fixed.

Assumptions

None load-bearing beyond what's stated above; the scope of the relaxation (declarative-only) was an explicit operator choice via AskUserQuestion, not an inferred preference.

Skill audit evidence

  • deterministic-gate-quality: RAN -- independent subagent dispatch of evaluating-deterministic-gate-quality against hooks/check-bash-safety.sh, read against skills/evaluating-deterministic-gate-quality/references/dimensions.md, dimension 15 in particular. Verdict: well-formed and well-placed. Dimension 15 live-tested directly against the script on disk (jq missing, truncated/non-object JSON, non-string tool_name, tool_input: false, array-wrapped tool_input.command, missing command, tool_name absent) -- all fail closed exactly as before; this diff only touched the install_re alternation, not any of the malformed-input guards. Pre/post-diff comparison (running identical payloads through origin/main's and this branch's script) confirmed uv add flipped deny->allow, uv remove was allow on both sides (never matched), uv pip install/uv install stayed deny on both. Dimension 9 (known-limitation disclosure) and dimension 12 (duplication/drift) also checked as satisfied for this diff's scope. One caveat the reviewing agent disclosed itself: its own context carried this repo's CLAUDE.md (the skill's dispatch guidance prefers a CLAUDE.md-free context for independence), noted rather than re-run, since the review target was unrelated to CLAUDE.md content.
  • defeat-test-disclosure: RAN -- hooks/test_gitapex_check_bash_safety.py gained DENIED_CHAINED_AFTER_ALLOWED_COMMANDS, three cases chaining a still-denied verb after the newly-allowed uv add/uv remove via &&, ;, and | (e.g. uv add safe && uv pip install malicious), confirming install_re's whole-string substring scan still catches it rather than short-circuiting on the first allowed token. The independent reviewing agent additionally hand-constructed and ran, directly against the script, its own novel attempts not copied from the test file: command-substitution smuggling (uv add "$(echo malicious)" && uv pip install evil), newline-separated chaining, and tab/mixed-case verb obfuscation after uv add -- none defeated the carve-out. One unrelated pre-existing gap surfaced (a fully-obfuscated verb via command substitution, e.g. uv remove x; `echo apt-get` install -y evil) but was confirmed identical on origin/main before this diff -- the already-disclosed "obfuscation that hides the verb itself" class named in the script's own header comment, not introduced or worsened here.

Acceptance Criteria Map

Restated from #1320, row by row:

Criterion Proof method Result
uv package installs are blocked and should be relaxed (declarative commands only) uv run --frozen pytest hooks/test_gitapex_check_bash_safety.py -q --no-cov -- uv add requests/uv remove requests exit 0 (allow), uv pip install requests/uv install requests still exit 2 (deny) Confirmed: 82 passed, including both allow and deny cases
apm install/uninstall should also be relaxed Same pytest run -- apm install foo/apm uninstall foo exit 0 (allow, no warn output) Confirmed: already allowed pre-diff, now pinned by regression test

Risk / blast radius

  • uv add/uv remove can still execute arbitrary code during dependency resolution/build (e.g. an sdist's build backend) -- this relaxation does not eliminate that execution-time risk, only compensates for it via post-hoc PR review of the pyproject.toml/uv.lock diff. Disclosed in the new code comment and in docs/security-control-inventory.md's updated LLM05 row.
  • uv pip install/bare uv install remain denied; apm install/apm uninstall had no behavior change (pinning only).
  • Gate is shared across every Bash tool call in this repository's own agent sessions -- a regression here would either under- or over-block installs repository-wide. Covered by the full existing 82-case test suite plus the 3 new defeat-test cases.
  • ruleset-scan currently fails on this PR (and on every open PR against main): a pre-existing, disclosed gap (issue chore(ci): adopt ruleset-as-code (or classic branch protection) for gitapex's own main branch #439, docs/runbooks/rulesets.md) where PR feat(drafting-a-pr-to-merge): add independent-review-pending status check #1318's new independent-review-pending required check was added to the committed .github/rulesets/main.json but not yet applied to the live GitHub ruleset. Reconciling it requires a maintainer to dispatch Apply rulesets (dry run first) with the ruleset-apply Environment's required-reviewer approval -- outside this PR's scope and outside what this session can or should do unilaterally.

Rollback

Revert the commits on this branch (15d271b, a479c8b, c8e411e, a36381a); install_re and the ssot.json registry entry return to denying uv add, the new allow/defeat-test sections and the security-control-inventory.md line revert with it. No schema/data migration involved.

Verification

  • uv run --frozen pytest hooks/test_gitapex_check_bash_safety.py -q --no-cov -> 82 passed
  • uv run --frozen pytest --no-cov -q (full suite, mirrors CI's test.yml) -> 5626 passed, 1 failed (pre-existing, environment-caused shallow-clone issue in tests/test_gitapex_scan_harden_checkout_pin_drift.py::test_repository_workflows_are_drift_free -- same class already documented in PR fix(hooks): add jq type-confusion matrix test, close two origin-hook gaps #1314's own body; real CI checks out full history via fetch-depth: '0' and does not hit this; unrelated to this diff)
  • uv run --frozen python3 .github/scripts/gitapex_scan_ssot_schema.py -> "No ssot.json drift found." (re-run after the registry fix)
  • python3 .github/scripts/gitapex_gate_owasp_llm_mapping.py -> "OWASP LLM01-10 mapping is complete."
  • bash -n hooks/check-bash-safety.sh -> syntax OK
  • ruff check / ruff format --check / mypy / betterleaks (pre-commit hooks) -> all passed on every commit
  • Codecov (automated PR comment): all modified/coverable lines covered; project coverage 99.52%

Step 8 independent review verdict

  • Verdict: CLEAN
  • Verified commit: a36381a

Outer layer: github:request_copilot_review was called against this PR; no Copilot review appeared afterward (get_reviews returned empty) -- consistent with PR #1318's own recent, explicit finding that neither Anthropic's Claude Code Review GitHub App nor GitHub Copilot's reviewer is actually configured in this repository (Devin Review and CodeRabbit both confirmed skipped via their own check-run status: "trial expired and no credits remaining" / "manual review required for this OSS repository"). Disclosed here rather than silently assumed covered.

Inner layer: an independent, isolated subagent (adversarially framed, did not author this change) reviewed the full diff and file contents (not only the changed hunks) across correctness, regression/blast-radius, reuse/simplification, and convention-adherence, against commit c8e411e. It read the complete current hooks/check-bash-safety.sh and hooks/test_gitapex_check_bash_safety.py, diffed the whole script against origin/main to confirm no unrelated regex changed, ran 30 of its own adversarial commands directly against the live script, confirmed apm matches no pattern in the script at all, confirmed the sibling check_task_bash_safety.sh intentionally still denies uv add (documented pre-existing divergence, out of this PR's scope), and ran the test suite itself (82 passed). One real finding surfaced and was fixed at c8e411e: docs/security-control-inventory.md's LLM05 row implied uv remove also changed status, when it was already outside the deny set -- corrected.

A second, narrower drift was then caught directly by the operator (not by either Step 8 layer) between c8e411e and this verdict's commit a36381a: .gitapex/ssot.json's registered target[0].ref pattern for this same gate still listed uv add as denied. This commit's own diff was independently re-verified directly (git diff c8e411e..a36381a): exactly one line, a registry pattern-string edit removing |uv add to match install_re's actual current content, with no code or test logic touched -- confirmed via git diff, JSON-validity check, and a fresh gitapex_scan_ssot_schema.py run (still "No ssot.json drift found."). Given the narrow, mechanically-verifiable nature of this specific delta, this was independently validated directly rather than re-dispatched to a fresh inner-layer subagent; no new adversarial surface was introduced. No other findings survived verification. This verdict is re-issued against a36381a per this skill's own no-stale-verdict rule.

Checklist

  • Tests pass locally
  • Docs updated if behavior changed -- docs/security-control-inventory.md's LLM05 row and .gitapex/ssot.json's registry entry both updated
  • Issue number cited in every commit
  • Skill audit evidence -- required because this diff changes a registered deterministic gate (hooks/check-bash-safety.sh matches the hooks/check-*.sh naming convention); see the ## Skill audit evidence section above
  • Transfer-check disclosure -- N/A, no evals/*/split.md Kept-edit-log entry added
  • Skill branch/fixture coverage -- N/A, no SKILL.md Stop-boundary bullets or dispatch branches added

Related Issue

Closes #1320

claude added 2 commits August 25, 2026 11:25
…m install/uninstall

check-bash-safety.sh's install_re denied uv's add subcommand alongside
its pip-install-compatible and bare install forms. The add/remove
subcommands mutate pyproject.toml and uv.lock, so the change is
visible and reviewable in the PR diff -- unlike the other two forms,
which install into the venv with no diff trail. Drop the add
subcommand from the deny set (remove was never matched); the other
two forms stay denied.

apm's install/uninstall subcommands were already unmatched by
install_re in both check-bash-safety.sh and its sibling
check_task_bash_safety.sh -- add explicit regression tests pinning
that existing allow so a future widened install_re can't silently
sweep apm back into deny.

Update security-control-inventory.md's LLM05 row to reflect the new
scope of the deny set.

Refs #1320.
…remove

Confirms install_re's substring scan still catches a still-denied
pip-install-compatible/bare-install verb chained after the newly
allowed uv add/remove via &&, ;, or | -- the specific way the new
carve-out could have been exploited had it been implemented as a
first-token check instead of a shared substring pattern.

Refs #1320.
@coderabbitai

coderabbitai Bot commented Aug 25, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: ce10dda8-e4f0-4faa-ac1e-c40fa1dae9c6


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.52%. Comparing base (79476b5) to head (a36381a).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #1323    +/-   ##
========================================
  Coverage   99.51%   99.52%            
========================================
  Files         118      119     +1     
  Lines       20936    21085   +149     
  Branches     2460     2480    +20     
========================================
+ Hits        20835    20984   +149     
  Misses        101      101            

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Step 8 inner-layer review found this row implied uv remove was newly
carved out alongside uv add; uv remove was never in install_re's deny
set and its status did not change in this PR. Reworded to state that
precisely.

Refs #1320.
@tvna
tvna marked this pull request as draft August 25, 2026 11:54
.gitapex/ssot.json's registered target[0].ref for hooks/check-bash-
safety.sh still listed uv add among the denied patterns after this
branch's own earlier commit removed it from install_re. Operator
caught the drift; syncing the registry entry to match actual current
enforcement.

Refs #1320.
@tvna
tvna marked this pull request as ready for review August 25, 2026 14:26
@tvna
tvna deployed to ruleset-verify August 25, 2026 14:29 — with GitHub Actions Active
@tvna
tvna merged commit 50c2a50 into main Aug 25, 2026
27 of 29 checks passed
@tvna
tvna deleted the claude/relax-bash-safety-check-euma0j branch August 25, 2026 14:30
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.

fix(hooks): allow declarative uv add/uv remove in check-bash-safety.sh; pin already-allowed apm install/uninstall

2 participants