Skip to content

chore: add reviewer-feedback guardrails (CLAUDE.md, pre-commit hook, skill) - #665

Merged
ilblackdragon merged 2 commits into
mainfrom
chore/reviewer-feedback-guardrails
Mar 7, 2026
Merged

ilblackdragon merged 2 commits into
mainfrom
chore/reviewer-feedback-guardrails

Conversation

@ilblackdragon

Copy link
Copy Markdown
Member

Summary

Analysis of ~50 PRs from the past week identified 10 recurring themes in Copilot and Gemini code review comments — issues that should have been caught at development time. This PR adds three layers of guardrails:

1. CLAUDE.md — 7 new development rules

Rule Theme it addresses Frequency
Transaction safety Multi-step DB ops without transactions 10+ comments
UTF-8 string safety Byte-index slicing panics on multi-byte chars 4+ comments
Case-insensitive comparisons .ends_with(".png") fails for .PNG 4+ comments
Decorator/wrapper delegation New trait methods not forwarded through provider chain 4+ comments
Sensitive data in logs/SSE Raw tool params broadcast without redaction 8+ comments
Test temporary files Hardcoded /tmp paths collide in parallel runs 6+ comments
Trust boundaries Worker container data treated as trusted by orchestrator 3+ comments

2. Pre-commit hook — scripts/pre-commit-safety.sh

Mechanical grep-based checks on staged .rs files for:

  • Unsafe UTF-8 byte slicing (&s[..n] without is_char_boundary)
  • Case-sensitive file extension/media type comparisons
  • Hardcoded /tmp/ paths in tests
  • Tool parameters logged without redact_params()
  • Multiple DB operations in same hunk without transaction

Installed automatically via dev-setup.sh. Suppressible with // safety: <reason> inline comments.

3. Review checklist skill — skills/review-checklist/SKILL.md

Activates on "review"/"merge" keywords. Covers judgment-based items that can't be linted:

  • Transaction atomicity, dual-backend consistency
  • SSRF validation, approval checks, trust boundaries
  • Decorator chain delegation, test quality, doc accuracy

Test plan

  • pre-commit-safety.sh runs clean on current codebase (exit 0)
  • Script is executable and installed by dev-setup.sh
  • SKILL.md has valid frontmatter (name, version, activation patterns)
  • Manual: verify skill activates on "review checklist" message
  • Manual: verify pre-commit hook catches a staged &s[..100] in a .rs file

Generated with Claude Code

…skill)

Analysis of ~50 PRs from the past week identified 10 recurring themes
in Copilot and Gemini code review comments. This change addresses them
at development time through three layers:

1. CLAUDE.md additions (7 new rules):
   - Transaction safety for multi-step DB operations
   - UTF-8 string safety (no byte-index slicing)
   - Case-insensitive comparisons for paths/media types
   - Decorator/wrapper trait method delegation
   - Sensitive data redaction in logs/SSE
   - tempfile crate for test temporary files
   - Trust boundaries for worker container data

2. Pre-commit hook (scripts/pre-commit-safety.sh):
   Mechanical checks for unsafe byte slicing, case-sensitive
   extension comparisons, hardcoded /tmp paths, unredacted
   tool parameter logging, and non-transactional DB operations.
   Installed via dev-setup.sh alongside existing commit-msg hook.

3. Review checklist skill (skills/review-checklist/SKILL.md):
   Activates on "review"/"merge" keywords. Covers the judgment-based
   items that can't be linted: transaction safety, SSRF validation,
   approval checks, decorator delegation, test quality, and doc accuracy.

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 7, 2026 09:18
@github-actions github-actions Bot added scope: docs Documentation size: M 50-199 changed lines risk: low Changes to docs, tests, or low-risk modules contributor: core 20+ merged PRs labels Mar 7, 2026
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a set of guardrails designed to proactively address common code quality issues frequently identified by AI code reviewers. It aims to improve code robustness and reduce review cycles by catching potential problems earlier in the development process by adding new development rules, an automated pre-commit hook, and a review checklist skill.

Highlights

  • CLAUDE.md Updated with New Development Rules: Seven new guidelines were added covering critical areas like transaction safety, UTF-8 string handling, case-insensitive comparisons, decorator delegation, sensitive data logging, temporary file usage in tests, and trust boundaries.
  • New Pre-commit Hook (scripts/pre-commit-safety.sh): A new script was introduced and integrated into dev-setup.sh to perform automated, grep-based checks on staged Rust files for unsafe UTF-8 slicing, case-sensitive comparisons, hardcoded /tmp paths, unredacted tool parameters, and multi-step DB operations without transactions.
  • Review Checklist Skill (skills/review-checklist/SKILL.md): A new skill definition was added to provide a pre-merge checklist for judgment-based items that cannot be easily linted, covering database operations, security, string safety, trait wrappers, tests, and documentation.
Changelog
  • CLAUDE.md
    • Added seven new development rules covering transaction safety, UTF-8 string safety, case-insensitive comparisons, decorator/wrapper trait delegation, sensitive data in logs, test temporary files, and trust boundaries.
    • Updated the "Mechanical verification before committing" section to include running scripts/pre-commit-safety.sh.
  • scripts/dev-setup.sh
    • Modified the script to install the new pre-commit-safety.sh as a git pre-commit hook.
  • scripts/pre-commit-safety.sh
    • Added a new executable shell script that performs automated safety checks on staged Rust files.
  • skills/review-checklist/SKILL.md
    • Added a new skill definition with a pre-merge review checklist.
Activity
  • No human activity has occurred on this pull request yet.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several guardrails to improve code quality, including new development rules in CLAUDE.md, a pre-commit hook to catch common issues, and a review checklist skill. The changes are well-structured and address recurring problems. I've suggested a performance improvement for the new pre-commit script to make it faster for developers.

Note: Security Review is unavailable for this PR.

Comment thread scripts/pre-commit-safety.sh Outdated
Comment on lines +17 to +84
# Support both pre-commit hook (staged files) and standalone (all changed vs main)
if git diff --cached --quiet 2>/dev/null; then
# No staged changes -- compare working tree against main
DIFF_CMD="git diff origin/main -- "
else
DIFF_CMD="git diff --cached -U0 -- "
fi

WARNINGS=0

warn() {
if [ "$WARNINGS" -eq 0 ]; then
echo ""
echo "=== Pre-commit Safety Checks ==="
echo ""
fi
WARNINGS=$((WARNINGS + 1))
echo " [$1] $2"
}

# 1. Unsafe UTF-8 byte slicing: &s[..N] or &s[..some_var] on strings
# Safe patterns: is_char_boundary, char_indices, // safety:
if $DIFF_CMD '*.rs' 2>/dev/null | grep -nE '^\+' | grep -E '\[\.\..*\]' | grep -vE 'is_char_boundary|char_indices|// safety:|as_bytes|Vec<|&\[u8\]|\[u8\]|bytes\(\)|&bytes' | head -3 | grep -q .; then
warn "UTF8" "Possible unsafe byte-index string slicing. Use is_char_boundary() or char_indices()."
$DIFF_CMD '*.rs' 2>/dev/null | grep -nE '^\+' | grep -E '\[\.\..*\]' | grep -vE 'is_char_boundary|char_indices|// safety:|as_bytes|Vec<|&\[u8\]|\[u8\]|bytes\(\)|&bytes' | head -3 | sed 's/^/ /'
fi

# 2. Case-sensitive file extension or media type checks
# Match: .ends_with(".png") or == "image/jpeg" without prior to_lowercase
if $DIFF_CMD '*.rs' 2>/dev/null | grep -nE '^\+.*ends_with\("\.([pP][nN][gG]|[jJ][pP][eE]?[gG]|[gG][iI][fF]|[wW][eE][bB][pP]|[mM][dD])"\)' | grep -vE 'to_lowercase|to_ascii_lowercase|// safety:' | head -3 | grep -q .; then
warn "CASE" "Case-sensitive file extension comparison. Normalize to lowercase first."
$DIFF_CMD '*.rs' 2>/dev/null | grep -nE '^\+.*ends_with\("\.([pP][nN][gG]|[jJ][pP][eE]?[gG]|[gG][iI][fF]|[wW][eE][bB][pP]|[mM][dD])"\)' | grep -vE 'to_lowercase|to_ascii_lowercase|// safety:' | head -3 | sed 's/^/ /'
fi

# 3. Hardcoded /tmp paths in test files
if $DIFF_CMD '*.rs' 2>/dev/null | grep -nE '^\+.*"/tmp/' | grep -vE 'tempfile|tempdir|// safety:' | head -3 | grep -q .; then
warn "TMPDIR" "Hardcoded /tmp path. Use tempfile::tempdir() for parallel-safe tests."
$DIFF_CMD '*.rs' 2>/dev/null | grep -nE '^\+.*"/tmp/' | grep -vE 'tempfile|tempdir|// safety:' | head -3 | sed 's/^/ /'
fi

# 4. Logging tool parameters without redaction
if $DIFF_CMD '*.rs' 2>/dev/null | grep -nE '^\+.*tracing::(info|debug|warn|error).*param' | grep -vE 'redact|// safety:' | head -3 | grep -q .; then
warn "REDACT" "Logging tool parameters without redaction. Use redact_params() first."
$DIFF_CMD '*.rs' 2>/dev/null | grep -nE '^\+.*tracing::(info|debug|warn|error).*param' | grep -vE 'redact|// safety:' | head -3 | sed 's/^/ /'
fi

# 5. Multi-step DB operations without transaction
if $DIFF_CMD '*.rs' 2>/dev/null | grep -nE '^\+.*(\.execute\(|\.query\()' | head -1 | grep -q .; then
# Check if there are multiple execute/query calls in the same hunk without transaction/tx
HUNK_COUNT=$($DIFF_CMD '*.rs' 2>/dev/null | awk '
/^@@/ { count=0; has_tx=0 }
/^\+.*\.(execute|query)\(/ { count++ }
/^\+.*(transaction|\.tx\.|begin)/ { has_tx=1 }
/^@@/ { if (prev_count >= 2 && !prev_tx) found++ }
{ prev_count=count; prev_tx=has_tx }
END { if (count >= 2 && !has_tx) found++; print found+0 }
')
if [ "$HUNK_COUNT" -gt 0 ]; then
warn "TX" "Multiple DB operations in same hunk without transaction. Wrap in a transaction for atomicity."
fi
fi

if [ "$WARNINGS" -gt 0 ]; then
echo ""
echo "Found $WARNINGS potential issue(s). Fix them or add '// safety: <reason>' to suppress."
echo ""
exit 1
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

This script is a great addition for catching common issues early. However, it currently calls git diff up to 10 times, which can be slow, especially on large changesets. You can significantly improve performance by running git diff just once at the beginning and caching its output in a variable. This variable can then be piped to all subsequent grep and awk commands.

I've also added an early exit if there are no relevant changes to check, which further improves performance for commits that don't touch .rs files.

# Support both pre-commit hook (staged files) and standalone (all changed vs main)
if git diff --cached --quiet 2>/dev/null; then
    # No staged changes -- compare working tree against main
    DIFF_CMD="git diff origin/main -- "
else
    DIFF_CMD="git diff --cached -U0 -- "
fi

# Cache the diff output to avoid multiple expensive git calls
DIFF_OUTPUT=$($DIFF_CMD '*.rs' 2>/dev/null)

# If there are no relevant changes, exit early.
if [ -z "$DIFF_OUTPUT" ]; then
    exit 0
fi

WARNINGS=0

warn() {
    if [ "$WARNINGS" -eq 0 ]; then
        echo ""
        echo "=== Pre-commit Safety Checks ==="
        echo ""
    fi
    WARNINGS=$((WARNINGS + 1))
    echo "  [$1] $2"
}

# 1. Unsafe UTF-8 byte slicing: &s[..N] or &s[..some_var] on strings
#    Safe patterns: is_char_boundary, char_indices, // safety:
if echo "$DIFF_OUTPUT" | grep -nE '^\+' | grep -E '\[\.\..*\]' | grep -vE 'is_char_boundary|char_indices|// safety:|as_bytes|Vec<|&\[u8\]|\[u8\]|bytes\(\)|&bytes' | head -3 | grep -q .; then
    warn "UTF8" "Possible unsafe byte-index string slicing. Use is_char_boundary() or char_indices()."
    echo "$DIFF_OUTPUT" | grep -nE '^\+' | grep -E '\[\.\..*\]' | grep -vE 'is_char_boundary|char_indices|// safety:|as_bytes|Vec<|&\[u8\]|\[u8\]|bytes\(\)|&bytes' | head -3 | sed 's/^/    /'
fi

# 2. Case-sensitive file extension or media type checks
#    Match: .ends_with(".png") or == "image/jpeg" without prior to_lowercase
if echo "$DIFF_OUTPUT" | grep -nE '^\+.*ends_with\("\.([pP][nN][gG]|[jJ][pP][eE]?[gG]|[gG][iI][fF]|[wW][eE][bB][pP]|[mM][dD])"\)' | grep -vE 'to_lowercase|to_ascii_lowercase|// safety:' | head -3 | grep -q .; then
    warn "CASE" "Case-sensitive file extension comparison. Normalize to lowercase first."
    echo "$DIFF_OUTPUT" | grep -nE '^\+.*ends_with\("\.([pP][nN][gG]|[jJ][pP][eE]?[gG]|[gG][iI][fF]|[wW][eE][bB][pP]|[mM][dD])"\)' | grep -vE 'to_lowercase|to_ascii_lowercase|// safety:' | head -3 | sed 's/^/    /'
fi

# 3. Hardcoded /tmp paths in test files
if echo "$DIFF_OUTPUT" | grep -nE '^\+.*"/tmp/' | grep -vE 'tempfile|tempdir|// safety:' | head -3 | grep -q .; then
    warn "TMPDIR" "Hardcoded /tmp path. Use tempfile::tempdir() for parallel-safe tests."
    echo "$DIFF_OUTPUT" | grep -nE '^\+.*"/tmp/' | grep -vE 'tempfile|tempdir|// safety:' | head -3 | sed 's/^/    /'
fi

# 4. Logging tool parameters without redaction
if echo "$DIFF_OUTPUT" | grep -nE '^\+.*tracing::(info|debug|warn|error).*param' | grep -vE 'redact|// safety:' | head -3 | grep -q .; then
    warn "REDACT" "Logging tool parameters without redaction. Use redact_params() first."
    echo "$DIFF_OUTPUT" | grep -nE '^\+.*tracing::(info|debug|warn|error).*param' | grep -vE 'redact|// safety:' | head -3 | sed 's/^/    /'
fi

# 5. Multi-step DB operations without transaction
if echo "$DIFF_OUTPUT" | grep -qE '^\+.*(\.execute\(|\.query\()'; then
    # Check if there are multiple execute/query calls in the same hunk without transaction/tx
    HUNK_COUNT=$(echo "$DIFF_OUTPUT" | awk '
        /^@@/ { count=0; has_tx=0 }
        /^\+.*\.(execute|query)\(/ { count++ }
        /^\+.*(transaction|\.tx\.|begin)/ { has_tx=1 }
        /^@@/ { if (prev_count >= 2 && !prev_tx) found++ }
        { prev_count=count; prev_tx=has_tx }
        END { if (count >= 2 && !has_tx) found++; print found+0 }
    ')
    if [ "$HUNK_COUNT" -gt 0 ]; then
        warn "TX" "Multiple DB operations in same hunk without transaction. Wrap in a transaction for atomicity."
    fi
fi

if [ "$WARNINGS" -gt 0 ]; then
    echo ""
    echo "Found $WARNINGS potential issue(s). Fix them or add '// safety: <reason>' to suppress."
    echo ""
    exit 1
fi

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in f74ced1 — cached diff output in $DIFF_OUTPUT variable (single git diff call), added early exit when no .rs changes are present.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds development-time guardrails to reduce recurring reviewer-caught issues by documenting rules, enforcing mechanical checks in a pre-commit hook, and providing a “pre-merge checklist” skill for judgment-based review items.

Changes:

  • Add a new skill (review-checklist) that triggers on review/merge keywords and provides a pre-merge checklist.
  • Add scripts/pre-commit-safety.sh to run grep-based safety checks on Rust diffs (UTF-8 slicing, case-sensitivity, /tmp, redaction, transactions).
  • Update scripts/dev-setup.sh to install the new pre-commit hook and extend CLAUDE.md with new development rules + a reminder to run the safety script.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
skills/review-checklist/SKILL.md Adds an activatable pre-merge checklist skill covering common review pitfalls.
scripts/pre-commit-safety.sh Introduces a pre-commit safety checker for common Rust footguns and review patterns.
scripts/dev-setup.sh Installs the new pre-commit hook alongside the existing commit-msg hook.
CLAUDE.md Documents new guardrail rules and adds the safety script to the pre-commit verification list.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

# 1. Unsafe UTF-8 byte slicing (panics on multi-byte chars)
# 2. Case-sensitive file extension / media type comparisons
# 3. Hardcoded /tmp paths in tests (flaky in parallel runs)
# 4. Tool parameters logged without redaction (secret leaks)

Copilot AI Mar 7, 2026

Copy link

Choose a reason for hiding this comment

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

The header comment says this hook checks 4 items, but the script also includes a 5th check for multi-step DB operations/transactions. Please update the header comment (or the implemented checks list) so it accurately reflects what the hook enforces; otherwise it’s easy for developers to miss why commits are blocked.

Suggested change
# 4. Tool parameters logged without redaction (secret leaks)
# 4. Tool parameters logged without redaction (secret leaks)
# 5. Multi-step DB operations / transactions safety issues

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in f74ced1 — header now lists all 5 checks.

Comment thread scripts/pre-commit-safety.sh Outdated
Comment on lines +44 to +45
# 2. Case-sensitive file extension or media type checks
# Match: .ends_with(".png") or == "image/jpeg" without prior to_lowercase

Copilot AI Mar 7, 2026

Copy link

Choose a reason for hiding this comment

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

The “Case-sensitive file extension or media type checks” comment mentions matching media type equality (e.g. == "image/jpeg"), but the actual grep only targets .ends_with(".<ext>"). Either implement the media type comparison check or adjust the comment to avoid misleading behavior expectations.

Suggested change
# 2. Case-sensitive file extension or media type checks
# Match: .ends_with(".png") or == "image/jpeg" without prior to_lowercase
# 2. Case-sensitive file extension checks
# Match: .ends_with(".png") without prior to_lowercase

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in f74ced1 — comment now says "Case-sensitive file extension checks" only, matching the actual grep behavior.

Comment thread scripts/pre-commit-safety.sh Outdated
Comment on lines +17 to +20
# Support both pre-commit hook (staged files) and standalone (all changed vs main)
if git diff --cached --quiet 2>/dev/null; then
# No staged changes -- compare working tree against main
DIFF_CMD="git diff origin/main -- "

Copilot AI Mar 7, 2026

Copy link

Choose a reason for hiding this comment

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

Standalone mode diffs against origin/main. In clones without that remote/ref (forks, offline, shallow clones), the diff command can fail and the checks may end up being skipped silently. Consider resolving the default branch more robustly (e.g., @{upstream}, local main, or git merge-base HEAD <ref> fallback) and emitting a clear error if no baseline ref is available.

Suggested change
# Support both pre-commit hook (staged files) and standalone (all changed vs main)
if git diff --cached --quiet 2>/dev/null; then
# No staged changes -- compare working tree against main
DIFF_CMD="git diff origin/main -- "
# Determine a suitable base ref for standalone diffs.
resolve_base_ref() {
# Prefer the current branch's upstream, then common default branches.
local candidates=(
"@{upstream}"
"origin/HEAD"
"origin/main"
"origin/master"
"main"
"master"
)
for ref in "${candidates[@]}"; do
if git rev-parse --verify --quiet "$ref" >/dev/null; then
echo "$ref"
return 0
fi
done
echo "pre-commit-safety: could not determine a base Git ref for diff (tried: ${candidates[*]})." >&2
echo "pre-commit-safety: ensure your repository has an upstream or a local main/master branch." >&2
exit 1
}
# Support both pre-commit hook (staged files) and standalone (all changed vs main)
if git diff --cached --quiet 2>/dev/null; then
# No staged changes -- compare working tree against a resolved base ref
BASE_REF="$(resolve_base_ref)"
DIFF_CMD="git diff ${BASE_REF} -- "

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in f74ced1 — added resolve_base_ref() that tries @{upstream}, origin/HEAD, origin/main, origin/master, main, master in order, with a clear error if none resolve.

Comment on lines +63 to +76
# 5. Multi-step DB operations without transaction
if $DIFF_CMD '*.rs' 2>/dev/null | grep -nE '^\+.*(\.execute\(|\.query\()' | head -1 | grep -q .; then
# Check if there are multiple execute/query calls in the same hunk without transaction/tx
HUNK_COUNT=$($DIFF_CMD '*.rs' 2>/dev/null | awk '
/^@@/ { count=0; has_tx=0 }
/^\+.*\.(execute|query)\(/ { count++ }
/^\+.*(transaction|\.tx\.|begin)/ { has_tx=1 }
/^@@/ { if (prev_count >= 2 && !prev_tx) found++ }
{ prev_count=count; prev_tx=has_tx }
END { if (count >= 2 && !has_tx) found++; print found+0 }
')
if [ "$HUNK_COUNT" -gt 0 ]; then
warn "TX" "Multiple DB operations in same hunk without transaction. Wrap in a transaction for atomicity."
fi

Copilot AI Mar 7, 2026

Copy link

Choose a reason for hiding this comment

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

The transaction-safety check operates on git diff ... -U0 added lines only, so it will warn when you add a second .execute()/.query() inside an existing transaction that wasn’t modified in the same hunk. It’s also not suppressible via the advertised // safety: mechanism and doesn’t print the offending hunk(s), which makes it hard to act on. Consider running this check with some context (or -W like commit-msg-regression.sh), honoring // safety: markers in the awk logic, and printing a small snippet of the triggering hunks to guide fixes.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in f74ced1 — TX check now uses -W (function context) to see surrounding transaction/begin calls, honors // safety: suppression in awk, and prints the triggering .execute()/.query() lines.

- Cache diff output in variable to avoid ~10 redundant git diff calls (Gemini)
- Add early exit when no .rs files are changed (Gemini)
- Fix header comment: list all 5 checks, not just 4 (Copilot)
- Fix check 2 comment: only mentions file extensions, not media types (Copilot)
- Add resolve_base_ref() with fallback candidates instead of hardcoded
  origin/main for standalone mode (Copilot)
- TX check: use -W (function context) to reduce false positives, honor
  // safety: suppression, print triggering lines (Copilot)

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ilblackdragon
ilblackdragon merged commit 3b57d5b into main Mar 7, 2026
22 checks passed
@ilblackdragon
ilblackdragon deleted the chore/reviewer-feedback-guardrails branch March 7, 2026 21:20
This was referenced Mar 7, 2026
bkutasi pushed a commit to bkutasi/ironclaw that referenced this pull request Mar 28, 2026
…skill) (nearai#665)

* chore: add reviewer-feedback guardrails (CLAUDE.md, pre-commit hook, skill)

Analysis of ~50 PRs from the past week identified 10 recurring themes
in Copilot and Gemini code review comments. This change addresses them
at development time through three layers:

1. CLAUDE.md additions (7 new rules):
   - Transaction safety for multi-step DB operations
   - UTF-8 string safety (no byte-index slicing)
   - Case-insensitive comparisons for paths/media types
   - Decorator/wrapper trait method delegation
   - Sensitive data redaction in logs/SSE
   - tempfile crate for test temporary files
   - Trust boundaries for worker container data

2. Pre-commit hook (scripts/pre-commit-safety.sh):
   Mechanical checks for unsafe byte slicing, case-sensitive
   extension comparisons, hardcoded /tmp paths, unredacted
   tool parameter logging, and non-transactional DB operations.
   Installed via dev-setup.sh alongside existing commit-msg hook.

3. Review checklist skill (skills/review-checklist/SKILL.md):
   Activates on "review"/"merge" keywords. Covers the judgment-based
   items that can't be linted: transaction safety, SSRF validation,
   approval checks, decorator delegation, test quality, and doc accuracy.

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: address PR review feedback on pre-commit-safety.sh

- Cache diff output in variable to avoid ~10 redundant git diff calls (Gemini)
- Add early exit when no .rs files are changed (Gemini)
- Fix header comment: list all 5 checks, not just 4 (Copilot)
- Fix check 2 comment: only mentions file extensions, not media types (Copilot)
- Add resolve_base_ref() with fallback candidates instead of hardcoded
  origin/main for standalone mode (Copilot)
- TX check: use -W (function context) to reduce false positives, honor
  // safety: suppression, print triggering lines (Copilot)

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
drchirag1991 pushed a commit to drchirag1991/ironclaw that referenced this pull request Apr 8, 2026
…skill) (nearai#665)

* chore: add reviewer-feedback guardrails (CLAUDE.md, pre-commit hook, skill)

Analysis of ~50 PRs from the past week identified 10 recurring themes
in Copilot and Gemini code review comments. This change addresses them
at development time through three layers:

1. CLAUDE.md additions (7 new rules):
   - Transaction safety for multi-step DB operations
   - UTF-8 string safety (no byte-index slicing)
   - Case-insensitive comparisons for paths/media types
   - Decorator/wrapper trait method delegation
   - Sensitive data redaction in logs/SSE
   - tempfile crate for test temporary files
   - Trust boundaries for worker container data

2. Pre-commit hook (scripts/pre-commit-safety.sh):
   Mechanical checks for unsafe byte slicing, case-sensitive
   extension comparisons, hardcoded /tmp paths, unredacted
   tool parameter logging, and non-transactional DB operations.
   Installed via dev-setup.sh alongside existing commit-msg hook.

3. Review checklist skill (skills/review-checklist/SKILL.md):
   Activates on "review"/"merge" keywords. Covers the judgment-based
   items that can't be linted: transaction safety, SSRF validation,
   approval checks, decorator delegation, test quality, and doc accuracy.

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: address PR review feedback on pre-commit-safety.sh

- Cache diff output in variable to avoid ~10 redundant git diff calls (Gemini)
- Add early exit when no .rs files are changed (Gemini)
- Fix header comment: list all 5 checks, not just 4 (Copilot)
- Fix check 2 comment: only mentions file extensions, not media types (Copilot)
- Add resolve_base_ref() with fallback candidates instead of hardcoded
  origin/main for standalone mode (Copilot)
- TX check: use -W (function context) to reduce false positives, honor
  // safety: suppression, print triggering lines (Copilot)

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: core 20+ merged PRs risk: low Changes to docs, tests, or low-risk modules scope: docs Documentation size: M 50-199 changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants