Skip to content

fix(selfhost): remove Claude reviewer tools#5105

Open
JSONbored wants to merge 1 commit into
mainfrom
codex/propose-fix-for-ai-adapter-vulnerability
Open

fix(selfhost): remove Claude reviewer tools#5105
JSONbored wants to merge 1 commit into
mainfrom
codex/propose-fix-for-ai-adapter-vulnerability

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • Close a security regression where the Claude Code review subprocess used --permission-mode bypassPermissions with a brittle mutating-tool denylist, allowing unlisted tools (including MCP tools) to run via prompt injection.
  • Ensure the review subprocess remains headless and JSON-only while preventing any built-in or user/home MCP tools from loading and removing the fragile denylist boundary.

Description

  • Replace the brittle mutating-tool denylist with explicit tool removal and MCP hardening by adding --tools "", --strict-mcp-config, and using --disallowedTools mcp__* when spawning the claude subprocess.
  • Preserve --permission-mode bypassPermissions for headless operation but remove all built-in tools and prevent user/home MCP config from loading as a defense-in-depth boundary.
  • Use the dedicated system-prompt-file path via --append-system-prompt-file (no change in behavior) and keep the one-shot JSON review flow intact.
  • Update the focused unit test to assert the hardened argv shape (no plan, --tools "", --strict-mcp-config, and --disallowedTools mcp__*).

Testing

  • Ran npm run selfhost:env-reference:check and it passed.
  • Ran the focused unit tests with npx vitest run test/unit/selfhost-ai.test.ts -t "Claude Code" and npx vitest run test/unit/selfhost-ai.test.ts, both completed successfully (tests passing for the changed path).
  • Ran npm run typecheck and it passed.
  • Attempted npm run test:coverage (full suite) but the broader suite exceeded the available execution window in this environment and was not completed.
  • Attempted npm audit --audit-level=moderate but the registry audit endpoint returned 403 Forbidden in this execution environment, so the audit did not complete.

Codex Task

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.20%. Comparing base (0800f23) to head (1fe6bab).
⚠️ Report is 8 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5105   +/-   ##
=======================================
  Coverage   94.20%   94.20%           
=======================================
  Files         470      470           
  Lines       39722    39722           
  Branches    14493    14493           
=======================================
  Hits        37421    37421           
  Misses       1645     1645           
  Partials      656      656           
Flag Coverage Δ
shard-1 46.40% <0.00%> (-0.02%) ⬇️
shard-2 33.62% <100.00%> (+0.03%) ⬆️
shard-3 31.36% <0.00%> (+0.01%) ⬆️
shard-4 32.65% <0.00%> (-0.16%) ⬇️
shard-5 33.45% <0.00%> (-0.10%) ⬇️
shard-6 45.10% <0.00%> (+0.19%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/selfhost/ai.ts 98.08% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gittensory-orb gittensory-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 11, 2026
@gittensory-orb

gittensory-orb Bot commented Jul 11, 2026

Copy link
Copy Markdown

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-11 17:27:12 UTC

2 files · 1 AI reviewer · 2 blockers · readiness 93/100 · CI green · unstable

⏸️ Suggested Action - Manual Review

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

Review summary
This PR swaps a mutating-tool denylist (--disallowedTools Bash,Edit,Write,WebFetch,WebSearch) for --tools "" plus --strict-mcp-config plus --disallowedTools mcp__* when spawning the claude subprocess, aiming to close a prompt-injection gap where unlisted/MCP tools could still run. The intent and defense-in-depth layering (remove all tools, block user/home MCP config, explicit MCP deny) is sound in principle, but the entire security value of the change hinges on --tools "" actually being a recognized Claude Code CLI flag that disables all built-in tools — the diff only updates a unit test asserting the argv shape via a stub spawn, not that the real CLI honors this flag. The system-prompt-file and bypassPermissions handling are unchanged from before.

Blockers

  • src/selfhost/ai.ts (~line 905): the new hardening relies entirely on `--tools ""` actually being a supported Claude Code CLI flag that suppresses all built-in tools; the updated test in test/unit/selfhost-ai.test.ts only asserts the argv array contains `--tools`, `""` via a stub spawn — it never exercises the real CLI, so if this flag is unrecognized or ignored by the installed `claude` binary, the previous explicit Bash/Edit/Write/WebFetch/WebSearch denylist is gone and nothing has actually replaced it, silently reopening the exact mutating-tool exposure this PR claims to close.
  • PR description: this does not close or clearly link an eligible open repo issue; add a line such as `Closes feat(orb): add POST /v1/orb/ingest central collector endpoint #1234` or link the maintainer-authorized issue that this security fix is scoped to.
Nits — 5 non-blocking
  • The PR description names `npm audit` and full `test:coverage` as attempted-but-incomplete in this environment; worth re-running before merge given the security-sensitive nature of the change.
  • Consider adding an integration-level (non-stubbed) smoke test that actually spawns the real `claude` CLI with these flags in CI to confirm `--tools ""` and `--strict-mcp-config` are accepted and behave as intended, since the unit test only checks argv shape.
  • src/selfhost/ai.ts: link to or cite Claude Code CLI documentation/changelog confirming `--tools ""` is a stable, supported no-op-safe flag (not an experimental/undocumented one) in the PR description for reviewer confidence.
  • test/unit/selfhost-ai.test.ts: add a case that also asserts the old denylist strings (Bash, Edit, Write, WebFetch, WebSearch) are fully absent from the disallowedTools value, not just that Bash is absent, to lock in the intended removal.
  • src/selfhost/ai.ts:894: the security comment now encodes important Claude CLI semantics, so it should cite the exact Claude Code flag contract or an in-repo helper/test fixture to make future CLI upgrades easier to audit.

Concerns raised — review before merging

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
Signal Result Evidence
Code review ❌ 2 blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 45 registered-repo PR(s), 37 merged, 414 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 45 PR(s), 414 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Improvement ✅ Minor risk: clean · value: minor — Code changes are accompanied by test evidence.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 45 PR(s), 414 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
[BETA] Chat with Gittensory

Ask Gittensory a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @gittensory ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @gittensory chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @gittensory mention with a real question is routed to the closest matching read-only command automatically -- no exact syntax required.

Full command reference: https://gittensory.aethereal.dev/docs/gittensory-commands

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@gittensory-orb gittensory-orb Bot added the manual-review Gittensor contributor context label Jul 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aardvark codex gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant