Skip to content

chore: add agentic coding quality gates#90

Open
deferredreward wants to merge 2 commits intounfoldingWord:mainfrom
deferredreward:chore/agentic-coding-quality-gates
Open

chore: add agentic coding quality gates#90
deferredreward wants to merge 2 commits intounfoldingWord:mainfrom
deferredreward:chore/agentic-coding-quality-gates

Conversation

@deferredreward
Copy link
Copy Markdown
Contributor

Summary

We researched current best practices for agentic coding (March 2026) and found gaps in our quality infrastructure. These changes ensure AI-generated code goes through proper security scanning and that Claude Code operates with least-privilege permissions instead of unrestricted access.

  • .claude/settings.json — Replaced bypassPermissions with an explicit tool allowlist so Claude Code runs under least-privilege instead of unrestricted access. Removed the per-repo PreToolUse hook (block-no-verify), which is now handled globally in ~/.claude/.
  • .husky/pre-commit — Added gitleaks secret scanning to the pre-commit hook so leaked secrets are caught locally before they reach the remote.
  • .github/workflows/ci.yml — Added a secret-scan job using gitleaks/gitleaks-action@v2 for defense-in-depth in CI.

Test plan

  • Verify the pre-commit hook runs gitleaks on staged files (or prints a warning if gitleaks is not installed)
  • Verify the CI secret-scan job passes on a clean branch
  • Verify Claude Code respects the new allowlist and prompts for confirmation on unlisted tools

🤖 Generated with Claude Code

- Replace bypassPermissions with explicit tool allowlists in
  .claude/settings.json so Claude Code operates with least-privilege
  permissions instead of unrestricted access
- Add gitleaks secret scanning to the pre-commit hook so leaked
  secrets are caught before they reach the remote
- Add a gitleaks secret-scan job to the CI workflow for defense-in-depth
- Remove the per-repo PreToolUse hook (block-no-verify); this is now
  handled globally in ~/.claude/
Copy link
Copy Markdown
Collaborator

@IanLindsley IanLindsley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

This PR adds defense-in-depth secret scanning (pre-commit + CI) and replaces bypassPermissions with an explicit allowlist -- both good security hygiene moves. However, the --no-verify protection has a coverage gap, the pre-commit gitleaks step silently degrades, and the CI secret-scan job does not gate the rest of the pipeline.

Issues: CRITICAL: 0 | HIGH: 1 | MEDIUM: 2 | LOW: 1

Blocking Issues

  • [HIGH] --no-verify protection removed from repo-level config without equivalent repo-level replacement (details in inline comment on .claude/settings.json)

Other Issues

  • [MEDIUM] Pre-commit gitleaks silently skips when not installed -- defeats the purpose of a quality gate (inline on .husky/pre-commit)
  • [MEDIUM] CI secret-scan job is not wired as a dependency of downstream jobs -- secrets can leak while the rest of the pipeline passes (inline on .github/workflows/ci.yml)
  • [LOW] Orphaned .claude/hooks/block-no-verify.sh file left in the repo after its only reference is removed (inline on .claude/settings.json)

Positive Highlights

  • Moving from bypassPermissions to an explicit allowlist is a meaningful security improvement -- least-privilege is the right default.
  • Adding gitleaks in both pre-commit and CI provides proper defense-in-depth layering.
  • The fetch-depth: 0 on the CI secret-scan checkout is correct for full-history scanning.

Recommendation: REQUEST CHANGES

"Bash(mv *)",
"Bash(chmod *)",
"Read",
"Write",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] --no-verify protection gap

The old config had a PreToolUse hook that explicitly blocked git commit --no-verify. That hook reference is removed here, and Bash(git *) in the allowlist permits all git commands including git commit --no-verify.

The PR body says this is "now handled globally in ~/.claude/", but:

  1. User-level config is not tracked in the repo -- other contributors and CI do not get this protection.
  2. There is no way for a reviewer to verify the global hook exists or is correct.

Recommendation: Keep the PreToolUse hook in the repo-level settings alongside the new allowlist. Both can coexist:

{
  "permissions": {
    "allow": [ ... ]
  },
  "hooks": {
    "PreToolUse": [
      {
        "matcher": "Bash",
        "hooks": [
          {
            "type": "command",
            "command": ".claude/hooks/block-no-verify.sh"
          }
        ]
      }
    ]
  }
}

This also resolves the LOW issue of the orphaned block-no-verify.sh file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed .claude/ from version control entirely and added to .gitignore — per your suggestion in the general comment. Each dev manages their own Claude Code settings locally. --no-verify protection is covered by CLAUDE.md instructions and each dev's own ~/.claude/ config. This also resolves the orphaned block-no-verify.sh file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — removed .claude/ from version control and added to .gitignore. CLAUDE.md already covers the critical guardrails (no --no-verify, no direct deploys, etc.), and each developer can manage their own Claude Code settings locally via ~/.claude/.

elif [ -x "$HOME/.local/bin/gitleaks" ]; then
"$HOME/.local/bin/gitleaks" git --staged --no-banner --exit-code 1
else
echo "WARNING: gitleaks not found — skipping secret scan"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] Silent skip undermines the quality gate

When gitleaks is not installed, the hook prints a warning but exits 0 (success). This means the "quality gate" is silently bypassed for any developer who hasn't installed gitleaks. Since this PR's purpose is to add secret scanning as a gate, a silent skip works against that goal.

Recommendation: Consider failing the pre-commit hook when gitleaks is not found, with a message telling the developer how to install it:

else
  echo "ERROR: gitleaks not found. Install it: https://github.com/gitleaks/gitleaks#installing"
  echo "Or use: brew install gitleaks / go install github.com/gitleaks/gitleaks/v8@latest"
  exit 1
fi

If the team decides that a hard fail is too aggressive (e.g., for contributors without Go), document that decision in a comment and rely on the CI job as the mandatory gate -- but then the CI job must actually gate the pipeline (see the CI comment).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added install instructions to the warning message. Since gitleaks-action@v2 requires a paid GITLEAKS_LICENSE for org repos, I replaced it with a direct binary install in CI — that becomes the mandatory gate. Local pre-commit stays as a soft warning so contributors without gitleaks installed can still commit, with clear instructions on how to install it.

with:
fetch-depth: 0
- uses: gitleaks/gitleaks-action@v2
env:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] secret-scan does not gate downstream jobs

The secret-scan job runs in parallel with all other jobs and no job has needs: [secret-scan]. If gitleaks finds a leaked secret, the rest of the pipeline (lint, test, build) still passes and the PR shows mostly green checks. A developer could easily miss the one failing check.

Recommendation: Add secret-scan to the needs array of security-audit (which already gates everything else):

security-audit:
  name: Security Audit
  needs: [secret-scan]
  runs-on: ubuntu-latest

This way, if secrets are found, the entire pipeline halts immediately.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wired secret-scan into security-audit via needs: [secret-scan] so the entire pipeline halts if secrets are found. Also replaced gitleaks-action@v2 with direct binary installation — the action requires a paid GITLEAKS_LICENSE for org repos, but the gitleaks binary itself is MIT-licensed and works without any license key.

Copy link
Copy Markdown
Collaborator

@IanLindsley IanLindsley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] Consider removing .claude/ from the repository entirely.

Some developers run Claude Code in sandboxed environments and prefer using bypassPermissions — having .claude/settings.json checked into the repo can conflict with their workflow. The project instructions in CLAUDE.md (which lives at the repo root, not inside .claude/) already cover the critical guardrails.

Recommendation: Remove .claude/ from version control and add it to .gitignore, letting each developer manage their own local Claude Code settings.

- Remove .claude/ from version control and add to .gitignore so each
  developer manages their own Claude Code settings locally
- Improve gitleaks pre-commit warning with install instructions and
  note that CI serves as the mandatory gate
- Replace gitleaks-action@v2 with direct binary install in CI to avoid
  the paid GITLEAKS_LICENSE requirement for org repos
- Wire secret-scan as a dependency of security-audit so the entire
  pipeline halts if secrets are detected
Copy link
Copy Markdown
Collaborator

@IanLindsley IanLindsley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up Review

Previous Issues

Resolved

  • [MEDIUM] CI secret-scan not gating downstream jobs -- Fixed. security-audit now has needs: [secret-scan], which correctly gates the entire pipeline since all other jobs depend on security-audit.
  • [LOW] Orphaned block-no-verify.sh file -- Fixed. The entire .claude/ directory is deleted.
  • [MEDIUM] Remove .claude/ from the repository -- Fixed. All .claude/ files are deleted and .claude/ is added to .gitignore.

Resolved (with caveats)

  • [HIGH] --no-verify protection gap -- Resolved by design choice. The author removed .claude/ entirely rather than keeping the repo-level PreToolUse hook. The --no-verify protection now relies on CLAUDE.md instructions ("NEVER use --no-verify") and each developer's ~/.claude/ config. This is acceptable: CLAUDE.md is checked into the repo and Claude Code loads it automatically, providing the same guardrail via a different mechanism. The tradeoff is that CLAUDE.md instructions are "advisory" (the model follows them but there is no hard block), whereas the PreToolUse hook was a hard block. For this project's risk profile, the advisory approach is reasonable.
  • [MEDIUM] Pre-commit gitleaks silent skip -- Acknowledged by design. The author chose to keep the soft warning (exit 0) when gitleaks is not installed, relying on CI as the mandatory gate. The warning message was improved with install instructions. Since secret-scan now properly gates CI, this is an acceptable defense-in-depth strategy: local pre-commit is best-effort, CI is mandatory.

New Issues

  • [HIGH] Deleting .claude/agents/ci-watcher.md breaks CLAUDE.md workflow -- see inline comment.

Status: 4/4 previous issues resolved. 1 new issue found.

Recommendation: REQUEST CHANGES -- the ci-watcher agent deletion needs to be addressed before merge.

@@ -1,74 +0,0 @@
---
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] Deleting ci-watcher.md breaks the CI monitoring workflow

CLAUDE.md (lines 135-137) instructs Claude Code to invoke the ci-watcher subagent after every git push:

After every git push, you MUST invoke the ci-watcher subagent to verify CI passes:

  1. Invoke the ci-watcher agent using the Task tool with subagent_type: "ci-watcher"

This PR deletes .claude/agents/ci-watcher.md as part of removing the .claude/ directory, but CLAUDE.md still references it. After this PR merges, every git push will trigger a failed subagent invocation.

Recommendation: Either:

  1. Move ci-watcher.md outside .claude/ (e.g., docs/agents/ci-watcher.md or a top-level .claude-agents/ directory not covered by the .gitignore entry), OR
  2. Update CLAUDE.md to remove the ci-watcher subagent reference and inline the CI monitoring instructions, OR
  3. Exclude .claude/agents/ from the .gitignore entry (use a more specific pattern like .claude/settings.json and .claude/hooks/).

Option 3 is the most surgical -- change the .gitignore entry from .claude/ to specific paths:

.claude/settings.json
.claude/hooks/

This keeps ci-watcher.md tracked while still letting developers manage their own settings locally.

Copy link
Copy Markdown
Collaborator

@IanLindsley IanLindsley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] Consider removing .claude/ from the repository entirely. Some developers run Claude Code in sandboxed environments and prefer using bypassPermissions — having .claude/settings.json checked into the repo can conflict with their workflow. The project instructions in CLAUDE.md (which lives at the repo root, not inside .claude/) already cover the critical guardrails. Recommendation: Remove .claude/ from version control and add it to .gitignore, letting each developer manage their own local Claude Code settings.

Note: This was flagged in the initial review and has been addressed in this revision -- .claude/ is now deleted and .gitignored. Posting again for traceability. However, the .gitignore entry should be refined to allow .claude/agents/ to remain tracked (see the HIGH inline comment about ci-watcher.md).

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.

3 participants