docs(agents): add development guardrails, C++ language server docs, and Claude Code automation - #783
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Critical Issues
1. Hook pattern matching broken (.claude/hooks/block-generated-data.sh:17)
- Glob patterns like
*/Source/Data/data.txtwon't match absolute paths - Hook will never block edits to generated files
- See inline suggestion for fix
2. Platform compatibility (.claude/skills/engine-test/SKILL.md:24)
sysctl -n hw.ncpufails on Linux (current CI environment is Linux)- Use
nprocwith fallback tosysctl
|
|
||
| # Block generated data files in Source/Data/ | ||
| case "$FILE_PATH" in | ||
| */Source/Data/data.txt|*/Source/Data/data-plain-bpmf.txt|*/Source/Data/associated-phrases-v2.txt) |
There was a problem hiding this comment.
Critical bug: Glob pattern matching will fail for absolute paths. The pattern */Source/Data/data.txt only matches if the path contains a literal */ prefix, not any directory prefix.
This hook will never block edits because $FILE_PATH will be an absolute path like /home/runner/work/McBopomofo/McBopomofo/Source/Data/data.txt, which won't match */Source/Data/data.txt.
| */Source/Data/data.txt|*/Source/Data/data-plain-bpmf.txt|*/Source/Data/associated-phrases-v2.txt) | |
| */Source/Data/data.txt|*data-plain-bpmf.txt|*associated-phrases-v2.txt) |
Or use basename/dirname matching:
BASENAME=$(basename "$FILE_PATH")
DIRNAME=$(dirname "$FILE_PATH")
if [[ "$DIRNAME" == */Source/Data && ("$BASENAME" == "data.txt" || "$BASENAME" == "data-plain-bpmf.txt" || "$BASENAME" == "associated-phrases-v2.txt") ]]; then
echo "Blocked: $FILE_PATH is a generated file. Edit the source and rebuild instead." >&2
exit 2
fi|
|
||
| 3. Build: | ||
| ```bash | ||
| cd Source/Engine/build && make -j$(sysctl -n hw.ncpu) |
There was a problem hiding this comment.
Critical bug: sysctl -n hw.ncpu is macOS-specific. This will fail on Linux CI runners (GitHub Actions uses Ubuntu by default per the PR description showing Linux 6.14.0-1017-azure).
| cd Source/Engine/build && make -j$(sysctl -n hw.ncpu) | |
| cd Source/Engine/build && make -j$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo 1) |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive development guardrails for AI agents in AGENTS.md and adds several Claude Code automation hooks and skills. The new documentation is thorough and provides clear, strict guidelines for C++17 usage, build system updates, and commit conventions. The automation scripts for formatting and blocking edits on generated files are well-implemented. My main concern is regarding the implementation of CLAUDE.md, which appears to be a regular file instead of the intended symbolic link to AGENTS.md, potentially undermining the goal of a single source of truth.
I am having trouble creating individual review comments. Click here to see my feedback.
CLAUDE.md (1)
This file appears to be a regular text file containing the string "AGENTS.md", rather than a symbolic link to the AGENTS.md file as described in the pull request summary. A regular file will not provide the contents of AGENTS.md to tools reading CLAUDE.md, which defeats the purpose of having a single source of truth. To fix this, please remove this text file and create a symbolic link, for example by running ln -s AGENTS.md CLAUDE.md.
There was a problem hiding this comment.
Pull request overview
This PR consolidates AI coding assistant documentation and adds comprehensive development guardrails derived from lessons learned in PR #779. The changes establish a single source of truth for AI assistant guidance (AGENTS.md), enforce critical development practices, and add Claude Code automation hooks.
Changes:
- Consolidate Claude-specific guidance by symlinking CLAUDE.md to AGENTS.md
- Add CRITICAL primacy zone with pre-edit verification steps, C++17 enforcement, and dual build system requirements
- Add comprehensive C++20/C++23 prohibited features list and build system integration guidelines
- Add Claude Code automation via PostToolUse/PreToolUse hooks and skills for testing and branch verification
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| CLAUDE.md | Changed from standalone file to symlink pointing to AGENTS.md for single source of truth |
| AGENTS.md | Added structured documentation with metadata tags, CRITICAL section with pre-edit checklist, C++17-only enforcement with forbidden features list, build system integration rules, PR review response guidelines, and Claude Code automation documentation |
| .claude/settings.json | New Claude Code configuration with PostToolUse hook for auto-formatting C++/ObjC files and PreToolUse hook to block edits to generated data files |
| .claude/hooks/format-cpp.sh | PostToolUse hook script to auto-format C++/ObjC files using clang-format after edits |
| .claude/hooks/block-generated-data.sh | PreToolUse hook script to prevent direct edits to generated dictionary data files in Source/Data/ |
| .claude/skills/engine-test/SKILL.md | New skill for building and running the full C++ engine unit test suite via CMake |
| .claude/skills/branch-guard/SKILL.md | New skill for verifying branch and worktree before making edits to prevent cross-branch editing mistakes |
| - Reuse blob readers (`KeyValueBlobReader`, | ||
| `ParselessPhraseDB`, `PhraseReplacementMap`) |
There was a problem hiding this comment.
The blob reader KeyValueBlobReader referenced here does not exist in the codebase. Based on the actual engine code, the correct blob readers are ParselessPhraseDB and PhraseReplacementMap. Remove the reference to KeyValueBlobReader.
| - Reuse blob readers (`KeyValueBlobReader`, | |
| `ParselessPhraseDB`, `PhraseReplacementMap`) | |
| - Reuse blob readers (`ParselessPhraseDB`, | |
| `PhraseReplacementMap`) |
tianjianjiang
changed the title
There was a problem hiding this comment.
Critical Issues
Hook Script Dependencies
Both hook scripts depend on jq but don't verify it's installed. This creates a silent failure mode where:
- block-generated-data.sh: Missing
jqallows edits to protected generated files (security/correctness issue) - format-cpp.sh: Missing
jqskips formatting (less critical but still problematic)
Error Handling
The format-cpp.sh hook silently ignores clang-format failures, which could mask syntax errors or formatting issues.
Impact: On fresh development machines or CI environments without jq, the hooks will fail their intended purpose without clear error messages.
| set -euo pipefail | ||
|
|
||
| INPUT=$(cat) | ||
| FILE_PATH=$(echo "$INPUT" | jq -r '.tool_input.file_path // empty') |
There was a problem hiding this comment.
Critical: Missing jq dependency check
This script will fail silently if jq is not installed, potentially allowing edits to generated files. Add a dependency check:
| FILE_PATH=$(echo "$INPUT" | jq -r '.tool_input.file_path // empty') | |
| INPUT=$(cat) | |
| # Check for jq dependency | |
| if ! command -v jq &> /dev/null; then | |
| echo "Error: jq is required but not installed. Install with: brew install jq" >&2 | |
| exit 1 | |
| fi | |
| FILE_PATH=$(echo "$INPUT" | jq -r '.tool_input.file_path // empty') |
|
|
||
| # Read stdin JSON and extract file_path | ||
| INPUT=$(cat) | ||
| FILE_PATH=$(echo "$INPUT" | jq -r '.tool_input.file_path // empty') |
There was a problem hiding this comment.
Critical: Missing jq dependency check
Same issue as the other hook - this will fail silently without jq. Add the same dependency check here.
| case "$FILE_PATH" in | ||
| *.cpp|*.h|*.mm|*.m) | ||
| if [[ -f "$FILE_PATH" ]]; then | ||
| xcrun clang-format -i "$FILE_PATH" |
There was a problem hiding this comment.
Potential issue: Silent clang-format failures
If xcrun clang-format fails (wrong version, not found, syntax errors in file), the hook silently succeeds. Consider capturing and reporting errors:
| xcrun clang-format -i "$FILE_PATH" | |
| if [[ -f "$FILE_PATH" ]]; then | |
| if ! xcrun clang-format -i "$FILE_PATH" 2>&1; then | |
| echo "Warning: clang-format failed for $FILE_PATH" >&2 | |
| # Don't exit with error to avoid blocking the edit | |
| fi | |
| fi |
dbae219 to
fe48e62
Compare
There was a problem hiding this comment.
Critical Issues Found
Identified 3 critical issues requiring fixes before merge:
- Path traversal vulnerability in
block-generated-data.sh- bypassing protection via symlinks/traversal - Command injection risk in
format-cpp.sh- malicious file paths could execute arbitrary commands - Platform compatibility bug in
/engine-testskill -sysctlcommand fails on Linux CI
All issues have suggested fixes inline.
| # Block generated data files in Source/Data/ | ||
| case "$FILE_PATH" in | ||
| */Source/Data/data.txt|*/Source/Data/data-plain-bpmf.txt|*/Source/Data/associated-phrases-v2.txt) | ||
| echo "Blocked: $FILE_PATH is a generated file. Edit the source and rebuild instead." >&2 |
There was a problem hiding this comment.
Security: Path traversal vulnerability
The hook doesn't validate absolute paths, allowing bypass via symlinks or path traversal. An attacker could create /tmp/Source/Data/data.txt and bypass the check.
| echo "Blocked: $FILE_PATH is a generated file. Edit the source and rebuild instead." >&2 | |
| # Block generated data files in Source/Data/ (normalize to absolute path) | |
| NORMALIZED_PATH=$(readlink -f "$FILE_PATH" 2>/dev/null || echo "$FILE_PATH") | |
| case "$NORMALIZED_PATH" in | |
| */Source/Data/data.txt|*/Source/Data/data-plain-bpmf.txt|*/Source/Data/associated-phrases-v2.txt) |
Also consider checking the basename if files are only in Source/Data/.
| # Only format C++/ObjC source files | ||
| case "$FILE_PATH" in | ||
| *.cpp|*.h|*.mm|*.m) | ||
| if [[ -f "$FILE_PATH" ]]; then |
There was a problem hiding this comment.
Security: Command injection risk
If FILE_PATH contains malicious characters or shell metacharacters, it could lead to command injection when passed to clang-format.
| if [[ -f "$FILE_PATH" ]]; then | |
| # Only format C++/ObjC source files (quote the path to prevent injection) | |
| case "$FILE_PATH" in | |
| *.cpp|*.h|*.mm|*.m) | |
| if [[ -f "$FILE_PATH" ]]; then | |
| xcrun clang-format -i -- "$FILE_PATH" |
Adding -- prevents path arguments starting with - from being interpreted as flags.
|
|
||
| 3. Build: | ||
| ```bash | ||
| cd Source/Engine/build && make -j$(sysctl -n hw.ncpu) |
There was a problem hiding this comment.
Bug: Platform-specific command on Linux CI
sysctl -n hw.ncpu is macOS-specific and will fail on Linux CI (GitHub Actions uses Linux). This causes the build step to fail.
| cd Source/Engine/build && make -j$(sysctl -n hw.ncpu) | |
| cd Source/Engine/build && make -j$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo 4) |
This uses nproc on Linux, falls back to sysctl on macOS, and defaults to 4 cores if both fail.
fe48e62 to
3e4114b
Compare
fb72feb to
15820a9
Compare
tianjianjiang
changed the title
Summary
Stack: #784 <- #783 <- #786 <- #785 <- #779 <- #780 <- #781
/engine-test,/branch-guard), context thresholdcompile_commands.json,.mmfile limitation/engine-testskill passes-DCMAKE_EXPORT_COMPILE_COMMANDS=ONto keepcompile_commands.jsonfresh.gitignore: addcompile_commands.jsonand.cache(clangd index)Test plan
/engine-testpasses (100 tests,compile_commands.jsonregenerated)find_symbol("ReadingGrid")confirms clangd works.cache/no longer shows as untracked🤖 Generated with Claude Code