Skip to content

Require inferred-vs-verified disclosure in reviews; deny turbo in the review shell - #37

Merged
jboyson-twenty merged 1 commit into
mainfrom
review-inference-disclosure-and-turbo-deny
Aug 21, 2026
Merged

Require inferred-vs-verified disclosure in reviews; deny turbo in the review shell#37
jboyson-twenty merged 1 commit into
mainfrom
review-inference-disclosure-and-turbo-deny

Conversation

@jboyson-twenty

@jboyson-twenty jboyson-twenty commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Description

Two additions to .github/workflows/claude-review.yml, the reusable workflow every repo's review job consumes. Both were raised by reviewers on downstream PRs; both are generic, so they belong upstream here where all repos get them, rather than in one repo's extra_prompt (@gobetti's structural point on isapp/ht-platform#3602 — that PR was closed as redundant because its 22 lines duplicated this base prompt).

Neither addition touches AIDLC's canonical review body: per isapp/AIDLC#214 a parity test asserts that surface stays byte-identical to ht-platform's extra_prompt, and an environment-constraints rule and a --disallowedTools entry live in neither.

1. Inferred-vs-verified disclosure rule (## Environment constraints)

Because nothing here can be executed, most of what you report is
inferred from reading rather than confirmed by running it. Disclose
which is which. When a finding depends on something you could not
confirm by reading — a runtime value, whether a test actually fails
without the change, how a dependency behaves internally, whether a
migration succeeds against real data — say that part is inferred and
name what would confirm it. Do not present an inference as verified.

The reviewer runs in a sandbox that cannot execute anything, so nearly everything it reports is inferred. It matters more here than for a local review, where a human can just run the thing. @jdrorrer's framing: this is the right shape of rule for a layer that has none, and it needs no technical enforcement to be worth having.

Verified absent before this change: grep -ciE 'inferred|verified|distinguish' over the whole file returns 0. The nearest existing text — "If something genuinely requires a blocked tool, note it once in the final comment instead of retrying" — is about blocked tools, not about marking inference.

Scoped deliberately to findings the reviewer is already writing up. A draft also said "'I could not verify X' is a legitimate thing to report", covering the case where it clears code it could not check. I dropped that: it cuts against the existing Behavior rule "if you initially find an issue but then determine no fix is needed, do not comment on it", and would push comment volume up. Easy to add back if reviewers want the clearing direction covered too.

2. Bash(turbo:*) in --disallowedTools

Verified gap: Bash(npm:*), Bash(npx:*), Bash(yarn:*) and Bash(pnpm:*) were all denied; turbo was not. Placed next to Bash(pnpm:*) — it's the JS-monorepo runner that pairs with them — and it is a Bash(...) pattern, never an MCP tool name, per the constraint in the comment above the list.

Is turbo even reachable from this job? Not today, as far as I can verify — so this entry is defense-in-depth, not a closed hole:

  • Verified: the review job runs exactly three steps (harden-runner, actions/checkout, claude-code-action) on ubuntu-latest. No setup-node, no dependency install, and installs are themselves denied.
  • Verified locally: in ht-platform, turbo is a devDependency (package.json, "turbo": "^2.9.17") resolved from node_modules/.bin/turbo, with a root turbo.json and eleven package.json scripts fronted by turbo run (build, test, lint, typecheck, codegen, the db:* family).
  • Inferred, not verified: that turbo is therefore not on PATH in the job. I did not inspect the GitHub-hosted runner image; I'm reasoning from "no install step ran and the binary lives in node_modules". Confirming it would take a run that echoes command -v turbo.

So the value is (a) drift protection — the day a caller adds a setup/install step, or a repo vendors or globally installs turbo, the deny is already there; and (b) it removes the invitation: a repo whose turbo.json and scripts advertise turbo run lint is inviting the reviewer to spend a turn trying it. That's also why it belongs in the deny list rather than in prose, which @jdrorrer noted "only works if the agent chooses to follow it."

Same best-effort caveat the whole list already carries: a prefix pattern on turbo does not fence a path-qualified node_modules/.bin/turbo, exactly as Bash(pnpm:*) does not. The header already frames this list as scope guard plus defense-in-depth, not a boundary.

Ticket

None — follow-ups from review comments on isapp/ht-platform#3602 and isapp/AIDLC#214.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactor (code change that neither fixes a bug nor adds a feature)
  • Documentation update
  • Chore (dependency updates, config changes, etc.)

Testing Done

There is no test harness in this repo, so verification was static:

  • YAML parses; the prompt block scalar keeps uniform 12-space indentation and the ${{ inputs.extra_prompt }} placeholder is intact.
  • claude_args is still a single folded line; the deny list parses to 43 comma-separated entries, all of the form Bash(...), no duplicates, with Bash(turbo:*) present.
  • Grepped the repo for prose that describes the deny list. The two comments that do (file header, and the block above claude_args) describe it categorically — "package managers, build/test runners, and network tools" — and enumerate nothing, so neither goes stale. README.md and CLAUDE.md do not mention it.

Not exercised by this PR: claude-review.yml is workflow_call-only and this repo has no caller for it, so the changed prompt and deny list do not run against this PR. Consumers reference it at @main (e.g. ht-platform's uses: isapp/.github/.github/workflows/claude-review.yml@main), so merging takes effect on every consumer's next review with no staging step. First real signal is the next downstream PR review.

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my code
  • I have added tests that prove my fix/feature works
  • New and existing tests pass locally
  • I have updated documentation as needed

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Two additions to the reusable review workflow, both surfaced by reviewers
on downstream PRs (isapp/ht-platform#3602, isapp/AIDLC#214). Both are
generic — they apply to every repo that consumes this workflow — so they
belong here rather than in any single repo's extra_prompt.

1. Inferred-vs-verified disclosure rule in the base prompt. The reviewer
   runs in a sandbox with no execution, so nearly everything it reports is
   inferred from reading. The rule requires it to say which part of a
   finding it could not confirm by reading, instead of presenting the
   inference as verified. Prose-only by nature; there is nothing to
   enforce mechanically.

2. Bash(turbo:*) in --disallowedTools. pnpm/npm/npx/yarn were already
   denied; turbo — the task runner those repos front build, test, lint and
   migrations with — was not. Placed next to Bash(pnpm:*) and matching the
   list's existing Bash(...)-patterns-only constraint.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jboyson-twenty
jboyson-twenty requested a review from gobetti August 21, 2026 14:39
@jboyson-twenty
jboyson-twenty merged commit e6c039d into main Aug 21, 2026
2 checks passed
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.

2 participants