Rejudge hardening: UTF-8-safe hooks, PowerShell coverage, guard/mem/workflow fixes, live demo - #8
Conversation
…orkflow fixes, live demo
Adversarial re-audit of the whole kit (multi-agent finder/verifier fleet + a
review of the resulting diff) surfaced real defects the earlier self-review
passes missed. Fixes for the verified findings:
Hooks
- Every hook now forces UTF-8 stdio and opens transcripts/state with an explicit
encoding. On Windows Python <=3.14 (cp1252 default) a non-ASCII payload used to
crash the read and fail the hook OPEN — the claim-audit gate and compaction
recovery were data-dependently inert on native Windows.
- Destructive guard: scans EVERY rm argument (`rm -rf build/ /` stray-space typo
is caught), handles long-form GNU flags and the PowerShell deletion spellings
(Remove-Item/ri/del, -Recurse + abbreviations) without misreading -Force,
covers Windows catastrophic targets and the `<target>/*` glob form, treats
quoted-delimiter heredoc bodies as literal data, and guards the PowerShell tool.
- Claim-audit gate: suite-claim negations ("not all tests pass yet") no longer
false-block; counts PowerShell file-writes.
- Loop alarm: tracks the PowerShell tool; nudge wording is threshold-agnostic.
fable-mem
- Atomic index upsert (concurrent session-ends can't corrupt the index).
- Recall relevance gate is token-boundary, not substring; the keyword-count knob
is split to FABLE_MEM_MIN_OVERLAP, decoupled from mem search's bm25 threshold.
Workflows
- Three-way honesty: a dead finder is surfaced in the returned result, never a
falsely-clean report. big-task independently verifies each commit landed before
a step counts green. memory-gc's judge fan-out is capped + budget-guarded.
check-workflows strips strings/comments before the Date.now/Math.random scan.
Installers / doctors
- install.ps1 / doctor.ps1 ship a UTF-8 BOM: without it Windows PowerShell 5.1
(the only PowerShell on stock Windows) ParserError'd on the documented
`powershell -File` install. Both doctors gained event-level wiring checks (a
partial merge dropping one block of a multi-event hook is now caught), a
staleness check, and a wrong-interpreter check; CI adds a PS 5.1 step and the
test suite falls back to powershell.exe when pwsh is absent.
Bench
- Windows-hermetic scoring (inherits os.environ, plugin autoload disabled);
run.sh detects the venv layout; an honest 2026-07-16 A/B re-run is recorded in
RESULTS.md (current stock Opus 4.8 now aces the task — no measurable delta,
underpowered n; the deterministic floor still fires).
New
- tools/demo.py runs the real shipped hooks against five failure modes in a
throwaway sandbox and asserts each fires; wired into CI via tests/test_demo.py.
Verifier agent now derives the changed surface itself via git diff instead of
trusting the caller's file list. Docs (README, CHANGELOG v2.1, SUCCESSION,
skills) updated to match. Suite: 226 passing, 23 skipped (the Windows baseline
was failing 6); ~34 new regression tests.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR hardens Windows PowerShell support, UTF-8 handling, destructive-command guards, memory indexing and recall, workflow verification, benchmark portability, doctor checks, and documentation. It adds a cross-platform demo and broad regression coverage for these behaviors. ChangesWindows installation and enforcement
Memory consistency and tooling
Documentation and guidance
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant install.ps1
participant settings.json
participant doctor.ps1
CI->>install.ps1: Execute with Windows PowerShell 5.1
CI->>settings.json: Install Windows hook settings
CI->>doctor.ps1: Validate files and event-level wiring
doctor.ps1-->>CI: Return validation status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.3)claude/workflows/big-task.jsFile contains syntax errors that prevent linting: Line 14: Illegal return statement outside of a function; Line 28: Illegal return statement outside of a function; Line 29: Illegal return statement outside of a function; Line 112: Illegal return statement outside of a function; Line 239: Illegal return statement outside of a function claude/workflows/memory-gc.jsFile contains syntax errors that prevent linting: Line 15: Illegal return statement outside of a function; Line 54: Illegal return statement outside of a function; Line 178: Illegal return statement outside of a function claude/workflows/bug-hunt.jsFile contains syntax errors that prevent linting: Line 129: Illegal return statement outside of a function
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
tests/test_loop_alarm.py (1)
214-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the post-reset behavior assertions.
Assert that the successful write returns zero, then verify the third subsequent failure alarms. Otherwise, an erroneous alarm or permanently disabled counter can pass this test.
Proposed test extension
- run_hook(tmp_path, "PowerShell", {"command": "Set-Content -Path x.py -Value 'fix'"}) + reset = run_hook(tmp_path, "PowerShell", + {"command": "Set-Content -Path x.py -Value 'fix'"}) + assert reset.returncode == 0 for _ in range(2): assert run_hook(tmp_path, "PowerShell", {"command": "python -m pytest -q"}, {}, event="PostToolUseFailure").returncode == 0 + alarm = run_hook(tmp_path, "PowerShell", {"command": "python -m pytest -q"}, + {}, event="PostToolUseFailure") + assert alarm.returncode == 2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_loop_alarm.py` around lines 214 - 222, Extend test_powershell_write_success_resets_counts to assert the successful Set-Content run returns zero, then add a third subsequent PostToolUseFailure invocation and assert it alarms with the expected nonzero return code, covering both reset and resumed counting behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bench/run.sh`:
- Around line 21-24: Update bench/run.sh to select the Windows
virtual-environment executable as Scripts/python.exe while preserving the POSIX
bin/python path. In tests/test_bench.py, update the fixture around the benchmark
setup to create Scripts/python.exe and assert that exact path is used.
In `@claude/agents/verifier.md`:
- Line 13: Update the repository discovery instructions in verifier.md to
include staged, unstaged, and untracked content: use git diff HEAD for tracked
changes, and explicitly inspect paths marked untracked by git status --short.
Ensure the verifier derives the complete changed surface from actual command
output rather than relying on the caller’s file list or git diff statistics.
In `@claude/hooks/posttool-loop-alarm.py`:
- Around line 62-68: Update the $null sink exclusion in the BASH_WRITE regex to
match PowerShell’s case-insensitive behavior, so redirects such as > $NULL are
not classified as writes. Apply the same case-insensitive change to the mirrored
claim-audit regex to keep both patterns synchronized.
In `@claude/hooks/pretool-destructive-guard.py`:
- Around line 92-111: Update _blank_quoted_heredocs to recognize heredoc
operators only when they are active shell syntax, excluding occurrences inside
ordinary single- or double-quoted strings while preserving valid heredoc
handling. Add a regression case covering an echoed quoted heredoc opener
followed by a destructive command, ensuring the command is not blanked and the
guard rejects it.
- Line 150: Update the _RM_INVOCATION regex to recognize the PowerShell rd and
rmdir aliases alongside the existing destructive commands, preserving
case-insensitive matching and argument capture. Add regression cases covering
both aliases, including the recursive rmdir form shown in the comment, and
verify they trigger the destructive-operation guard.
In `@claude/hooks/stop-claim-audit.py`:
- Line 47: Update the stop-claim audit regex and its synchronized loop-alarm
regex to match the PowerShell variable `$null` case-insensitively, while
preserving the existing redirect and device-path exclusions. Ensure mixed-case
forms such as `$NULL` and `$Null` are treated like `$null` and do not trigger a
modification.
In `@tests/test_demo.py`:
- Around line 34-52: Update test_demo_writes_no_state_to_the_users_real_dir to
isolate the fallback location by setting HOME and USERPROFILE to tmp_path in the
demo environment. Snapshot the synthetic ~/.claude/tmp/fable-protocol directory
recursively, including file contents and nested entries, before and after
run_demo, and assert the snapshots are identical.
In `@tests/test_destructive_guard.py`:
- Around line 333-337: Update the PowerShell destructive-command matcher
exercised by test_powershell_recurse_switch_syntax_blocks to parse the value of
the -Recurse switch, treating -Recurse:$false as non-recursive while continuing
to block enabled or abbreviated recursive forms. Add a regression assertion in
test_powershell_recurse_switch_syntax_blocks confirming the disabled form is
allowed.
In `@tests/test_install_doctor.py`:
- Around line 108-117: Update test_doctor_warns_on_windows_python_settings to
assert r.returncode == 0 after running DOCTOR, while retaining the existing bare
'python' diagnostic assertion.
In `@tests/test_small_tier.py`:
- Around line 89-92: Update the SKILL.md read in the test around the skill
assignment to pass encoding="utf-8" explicitly, matching the existing README
read and removing host-locale dependence.
In `@tools/check-workflows.mjs`:
- Around line 56-58: The stripStringsAndComments() scanner must recognize
JavaScript regex literals before interpreting // or /* as comments, so patterns
such as /[//]/ do not truncate the source before forbidden calls. Add
regex-literal handling with correct escaping and character-class support, or
replace the scanner with a real JavaScript lexer/parser, while preserving
existing string and comment stripping behavior.
In `@tools/doctor.ps1`:
- Around line 43-48: Update Test-AgentSame so it conditionally ignores only an
installer-injected frontmatter model pin in the installed copy: detect whether
the source content contains a model pin, and remove the installed copy’s
corresponding frontmatter pin only when the source has none. Preserve all source
pins and any model: text outside frontmatter, then compare the normalized
contents as before.
---
Nitpick comments:
In `@tests/test_loop_alarm.py`:
- Around line 214-222: Extend test_powershell_write_success_resets_counts to
assert the successful Set-Content run returns zero, then add a third subsequent
PostToolUseFailure invocation and assert it alarms with the expected nonzero
return code, covering both reset and resumed counting behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6faf3de1-d484-4adc-922a-3ecc7c5ddb42
📒 Files selected for processing (47)
.github/workflows/ci.ymlCHANGELOG.mdREADME.mdbench/RESULTS.mdbench/run.shbench/score.pyclaude/CLAUDE.mdclaude/agents/verifier.mdclaude/cli/mem.pyclaude/hooks/posttool-loop-alarm.pyclaude/hooks/posttool-test-weakening-alarm.pyclaude/hooks/precompact-save-task.pyclaude/hooks/pretool-destructive-guard.pyclaude/hooks/pretool-mem-privacy-guard.pyclaude/hooks/sessionend-mem-journal.pyclaude/hooks/sessionstart-compact-recovery.pyclaude/hooks/stop-claim-audit.pyclaude/hooks/userpromptsubmit-mem-recall.pyclaude/settings/settings-snippet-small.jsonclaude/settings/settings-snippet-windows-small.jsonclaude/settings/settings-snippet-windows.jsonclaude/skills/fable/SKILL.mdclaude/skills/webdesign/SKILL.mdclaude/workflows/big-task.jsclaude/workflows/bug-hunt.jsclaude/workflows/memory-gc.jsclaude/workflows/paranoid-review.jsdocs/SUCCESSION.mdinstall.ps1tests/test_bench.pytests/test_compaction_hooks.pytests/test_demo.pytests/test_destructive_guard.pytests/test_docs.pytests/test_install_doctor.pytests/test_loop_alarm.pytests/test_mem_cli.pytests/test_mem_journal_hook.pytests/test_mem_privacy_guard.pytests/test_mem_recall_hook.pytests/test_small_tier.pytests/test_stop_claim_audit.pytests/test_windows_port.pytools/check-workflows.mjstools/demo.pytools/doctor.ps1tools/doctor.sh
| VBIN=bin | ||
| [ -x "$INST/.venv/bin/python" ] || VBIN=Scripts | ||
| VENV_PY="$INST/.venv/$VBIN/python" | ||
| "$VENV_PY" -m pip -q install pytest |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
# Run in native Windows Git Bash to confirm the actual venv executable layout.
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
python -m venv "$tmp/venv"
find "$tmp/venv" -maxdepth 2 -type f -iname 'python*' -print
test -x "$tmp/venv/Scripts/python.exe"Repository: blyatiful1/fable-protocol
Length of output: 565
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '\n== bench/run.sh ==\n'
git ls-files bench/run.sh tests/test_bench.py
wc -l bench/run.sh tests/test_bench.py
printf '\n-- bench/run.sh (lines 1-120) --\n'
cat -n bench/run.sh | sed -n '1,120p'
printf '\n-- tests/test_bench.py (lines 120-190) --\n'
cat -n tests/test_bench.py | sed -n '120,190p'
printf '\n== Search for venv path handling ==\n'
rg -n 'Scripts/python|bin/python|python.exe|venv' bench tests -SRepository: blyatiful1/fable-protocol
Length of output: 5312
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '1,120p' bench/run.sh | cat -n
printf '\n----\n'
sed -n '130,180p' tests/test_bench.py | cat -nRepository: blyatiful1/fable-protocol
Length of output: 3522
🌐 Web query:
MSYS Bash execute path without .exe suffix python.exe windows bash CreateProcess PATHEXT
💡 Result:
MSYS2 Bash does not automatically use the Windows PATHEXT environment variable to resolve executables without their file extensions [1][2]. While Windows native APIs, specifically CreateProcess, utilize PATHEXT to resolve commands when the executable extension is omitted, Bash operates as a POSIX-compliant shell that searches for files based on execution permissions within the Linux-style environment [2][3][4]. Key technical points regarding this behavior: 1. Windows API Behavior: The Windows CreateProcess function automatically appends.exe if no extension is provided and the file is not found, or iterates through PATHEXT if the executable is not in the current directory [3][4][5]. However, this logic is specific to Windows shells like cmd.exe and PowerShell, not the Bash shell itself [1][2]. 2. MSYS2/Bash Limitations: Bash requires an explicit path or filename (including the extension, such as.exe) to execute a file, unless a specific mechanism is in place to bridge this gap [6][7]. Some common Windows binaries may appear to run without extensions in MSYS2 only because they are provided as symbolic links or wrapper scripts within the MSYS2 /usr/bin directory, not because the shell is natively resolving them via PATHEXT [6]. 3. Workarounds: Users typically handle this by: - Creating aliases in.bashrc (e.g., alias python=python.exe) [6]. - Defining a command_not_found_handle function in their.bashrc that intercepts command-not-found errors, iterates through PATHEXT, and attempts to execute the command if a matching file is found [6][7]. - Creating shell scripts or wrappers in a directory already in the PATH that call the intended Windows executable [8][9]. In summary, because Bash is an independent environment, it does not share the same command resolution logic as Windows cmd.exe, and it remains the user's responsibility to handle extension-less execution through shell-level configuration [2][7].
Citations:
- 1: https://sourceware.org/pipermail/cygwin/2005-April/131877.html
- 2: https://sourceware.org/pipermail/cygwin/2016-August/229004.html
- 3: https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa
- 4: https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw
- 5: Fix windows create process retry/path search ziglang/zig#2705
- 6: https://superuser.com/questions/1546664/git-bash-in-windows-requires-exe-extension-for-some-but-not-all-programs
- 7: obviate typing the filename extension for Windows executables microsoft/WSL#2003
- 8: https://superuser.com/questions/1731090/run-bash-scripts-without-typing-the-file-extension-the-right-way
- 9: https://unix.stackexchange.com/questions/612352/how-to-run-windows-executables-from-terminal-without-the-explicitly-specifying-t
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n '\bScripts/python(\.exe)?\b|python\.exe\b|PATHEXT|msys|cygwin' -S .Repository: blyatiful1/fable-protocol
Length of output: 377
🌐 Web query:
Cygwin Bash execute file without .exe suffix path /usr/bin/python.exe direct path no extension
💡 Result:
In the Cygwin environment, you typically do not need to include the.exe suffix to execute a binary, as Cygwin handles extensionless executable files automatically [1][2][3]. However, the ability to invoke these files depends on how the command is called and the environment from which it is executed [4][5][6]. Understanding Cygwin Execution Behavior: 1. Cygwin Shells (Bash, etc.): Within a Cygwin terminal, Bash is "Cygwin-aware." It recognizes executable files based on their POSIX permissions (chmod +x) or specific "magic" heuristics (such as the presence of #!, MZ, or: headers), regardless of whether they have a.exe extension [7][2][3]. If /usr/bin/python is in your PATH, running 'python' in a Cygwin shell should work without the.exe suffix [8][6]. 2. Windows CMD/PowerShell: Windows applications, including cmd.exe, do not share this Cygwin-specific awareness [4][5]. They generally require the literal.exe extension to identify and execute a binary correctly [4][9]. If you attempt to run /usr/bin/python (a symlink) from Windows CMD, it will likely fail because CMD does not understand the Cygwin symlink format or the expectation of implicit extension handling [4][9]. Troubleshooting and Workarounds: * Direct Path Execution: If you need to execute a file by its direct path from a Windows-native context (or a program that behaves like one), you must provide the full path including the.exe extension (e.g., C:\cygwin64\bin\python3.9.exe) [4][5]. * Creating Explicit Copies: If you require a specific command name to work across both environments (Cygwin and Windows CMD), a common workaround is to create a physical copy of the executable (or a hard link, if supported by the filesystem) with the required extension [1][4][9]. Avoid symlinks if you need to call the file from non-Cygwin programs, as these programs often fail to resolve them [4][6]. * PATH Configuration: Ensure that the directory containing the desired.exe is included in the Windows system PATH if you need to call it by name from outside the Cygwin environment [8][5][6]. For managing different versions of binaries like Python, it is recommended to use the 'alternatives' command within Cygwin to point 'python' to the desired version, which maintains the appropriate symlink structure within the Cygwin environment [8][4].
Citations:
- 1: https://superuser.com/questions/1374741/running-an-exe-file-in-cygwin
- 2: https://sourceware.org/legacy-ml/cygwin/2009-05/msg00615.html
- 3: https://stackoverflow.com/questions/787522/why-is-it-that-cygwin-can-run-bat-scripts
- 4: https://sourceware.org/pipermail/cygwin/2025-October/258868.html
- 5: https://cygwin.com/pipermail/cygwin/2021-February/247709.html
- 6: https://stackoverflow.com/questions/13070289/calling-python-from-maven-in-a-cygwin-shell
- 7: https://superuser.com/questions/329336/how-do-i-mark-batch-files-as-executables-in-bash-cygwin
- 8: https://stackoverflow.com/questions/67711956/how-to-force-cygwin-to-run-specific-application-when-multiple-with-same-name-exi
- 9: https://sourceware.org/legacy-ml/cygwin/2004-09/msg00979.html
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n 'windows-latest|Git Bash|MSYS|cygwin|Windows' .github bench tests README* -SRepository: blyatiful1/fable-protocol
Length of output: 10985
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '\n-- bench/README.md --\n'
wc -l bench/README.md
cat -n bench/README.md | sed -n '1,140p'
printf '\n-- CI / benchmark mentions --\n'
rg -n 'bench/run.sh|benchmark arm|run one benchmark|bench/' README.md .github tests bench -SRepository: blyatiful1/fable-protocol
Length of output: 7866
Handle the Windows venv executable name. bench/run.sh#L21-L24 still falls back to Scripts/python, but native Windows venvs expose Scripts/python.exe, so the benchmark stops before pip install pytest. tests/test_bench.py#L140-L160 only covers the POSIX bin/ case; make it use a Scripts/python.exe fixture and assert that exact path.
📍 Affects 2 files
bench/run.sh#L21-L24(this comment)tests/test_bench.py#L140-L160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bench/run.sh` around lines 21 - 24, Update bench/run.sh to select the Windows
virtual-environment executable as Scripts/python.exe while preserving the POSIX
bin/python path. In tests/test_bench.py, update the fixture around the benchmark
setup to create Scripts/python.exe and assert that exact path is used.
|
|
||
| Try to REFUTE the claim: | ||
| 1. Read the actual changed code — not the caller's description of it. | ||
| 1. Derive the changed surface yourself — `git status` + `git diff` (and `git diff --stat HEAD`), not the caller's file list. The caller's list is part of the claim, not ground truth: anything it omits is exactly where a false green hides. Then read the actual changed code, not the caller's description of it. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include staged and untracked content when deriving the changed surface.
git diff omits staged changes, while git diff --stat HEAD provides only statistics; neither command reads untracked files. The verifier can therefore skip real changed code and return a false green. Use git diff HEAD for tracked staged/unstaged content and explicitly inspect paths reported as untracked by git status --short.
Suggested wording
-1. Derive the changed surface yourself — `git status` + `git diff` (and `git diff --stat HEAD`), not the caller's file list.
+1. Derive the changed surface yourself — inspect `git status --short`, read tracked changes with `git diff HEAD`, and inspect every untracked path reported by status; do not rely on the caller's file list.Based on learnings, incomplete repository discovery is a false-green risk and must be verified from actual command output.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 1. Derive the changed surface yourself — `git status` + `git diff` (and `git diff --stat HEAD`), not the caller's file list. The caller's list is part of the claim, not ground truth: anything it omits is exactly where a false green hides. Then read the actual changed code, not the caller's description of it. | |
| 1. Derive the changed surface yourself — inspect `git status --short`, read tracked changes with `git diff HEAD`, and inspect every untracked path reported by status; do not rely on the caller's file list. The caller's list is part of the claim, not ground truth: anything it omits is where a false green hides. Then read the actual changed code, not the caller's description of it. |
🧰 Tools
🪛 LanguageTool
[style] ~13-~13: Consider an alternative for the overused word “exactly”.
Context: ... not ground truth: anything it omits is exactly where a false green hides. Then read th...
(EXACTLY_PRECISELY)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@claude/agents/verifier.md` at line 13, Update the repository discovery
instructions in verifier.md to include staged, unstaged, and untracked content:
use git diff HEAD for tracked changes, and explicitly inspect paths marked
untracked by git status --short. Ensure the verifier derives the complete
changed surface from actual command output rather than relying on the caller’s
file list or git diff statistics.
Source: Learnings
| BASH_WRITE = re.compile( | ||
| r"(?<![0-9&])>>?\s*(?!&|/dev/(?:null|stdout|stderr)\b)\S" | ||
| r"(?<![0-9&])>>?\s*(?!&|\$null(?=[\s;|&]|$)|/dev/(?:null|stdout|stderr)\b)\S" | ||
| r"|(?:^|[|&;]\s*)(?:sed\s+(?:-\S+\s+)*-i|tee\s|patch\s|truncate\s" | ||
| r"|(?:git\s+(?:apply|mv|rm|checkout|restore|stash))|mv\s|cp\s|rm\s)" | ||
| r"|(?:git\s+(?:apply|mv|rm|checkout|restore|stash))|mv\s|cp\s|rm\s" | ||
| r"|(?i:set-content|add-content|out-file|new-item|move-item|copy-item" | ||
| r"|remove-item|rename-item)\b)" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Match PowerShell’s case-insensitive $null sink.
A successful command > $NULL is currently classified as a file modification, clearing all loop counts despite making no change. Make this exemption case-insensitive and keep the mirrored claim-audit regex synchronized.
Proposed fix
- r"(?<![0-9&])>>?\s*(?!&|\$null(?=[\s;|&]|$)|/dev/(?:null|stdout|stderr)\b)\S"
+ r"(?<![0-9&])>>?\s*(?!&|(?i:\$null)(?=[\s;|&]|$)|/dev/(?:null|stdout|stderr)\b)\S"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| BASH_WRITE = re.compile( | |
| r"(?<![0-9&])>>?\s*(?!&|/dev/(?:null|stdout|stderr)\b)\S" | |
| r"(?<![0-9&])>>?\s*(?!&|\$null(?=[\s;|&]|$)|/dev/(?:null|stdout|stderr)\b)\S" | |
| r"|(?:^|[|&;]\s*)(?:sed\s+(?:-\S+\s+)*-i|tee\s|patch\s|truncate\s" | |
| r"|(?:git\s+(?:apply|mv|rm|checkout|restore|stash))|mv\s|cp\s|rm\s)" | |
| r"|(?:git\s+(?:apply|mv|rm|checkout|restore|stash))|mv\s|cp\s|rm\s" | |
| r"|(?i:set-content|add-content|out-file|new-item|move-item|copy-item" | |
| r"|remove-item|rename-item)\b)" | |
| ) | |
| BASH_WRITE = re.compile( | |
| r"(?<![0-9&])>>?\s*(?!&|(?i:\$null)(?=[\s;|&]|$)|/dev/(?:null|stdout|stderr)\b)\S" | |
| r"|(?:^|[|&;]\s*)(?:sed\s+(?:-\S+\s+)*-i|tee\s|patch\s|truncate\s" | |
| r"|(?:git\s+(?:apply|mv|rm|checkout|restore|stash))|mv\s|cp\s|rm\s" | |
| r"|(?i:set-content|add-content|out-file|new-item|move-item|copy-item" | |
| r"|remove-item|rename-item)\b)" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@claude/hooks/posttool-loop-alarm.py` around lines 62 - 68, Update the $null
sink exclusion in the BASH_WRITE regex to match PowerShell’s case-insensitive
behavior, so redirects such as > $NULL are not classified as writes. Apply the
same case-insensitive change to the mirrored claim-audit regex to keep both
patterns synchronized.
| _HEREDOC_START = re.compile(r"<<-?\s*(['\"])([A-Za-z_]\w*)\1") | ||
|
|
||
|
|
||
| def _blank_quoted_heredocs(s): | ||
| """Length- and newline-preserving blanking of QUOTED-delimiter heredoc bodies | ||
| (<<'EOF' ... EOF): those are pure literal data — no expansions execute inside — | ||
| so a test file or doc written through one must not trip the guard on strings it | ||
| merely CONTAINS (the kit's own test suite writes `rm -rf /` fixtures this way). | ||
| Unquoted-delimiter heredocs are left visible: `$(...)` executes inside them. | ||
| A missing terminator blanks to the end — which is also what the shell does.""" | ||
| out = s | ||
| for m in list(_HEREDOC_START.finditer(s)): | ||
| delim = m.group(2) | ||
| line_end = out.find("\n", m.end()) | ||
| if line_end == -1: | ||
| break | ||
| t = re.compile(r"\n[ \t]*" + re.escape(delim) + r"[ \t]*(?=\n|$)").search(out, line_end) | ||
| end = t.start() if t else len(out) | ||
| body = out[line_end + 1:end] | ||
| out = out[:line_end + 1] + re.sub(r"[^\n]", " ", body) + out[end:] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Only blank active heredocs.
The matcher also accepts heredoc syntax inside ordinary quotes. For example, this command blanks the destructive second line even though Bash executes it:
echo "<<'EOF'"
rm -rf /
EOFTrack shell quoting when locating heredoc operators, and add this as a regression case. The current behavior fails the guard open.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 107-107: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.compile(r"\n[ \t]" + re.escape(delim) + r"[ \t](?=\n|$)")
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@claude/hooks/pretool-destructive-guard.py` around lines 92 - 111, Update
_blank_quoted_heredocs to recognize heredoc operators only when they are active
shell syntax, excluding occurrences inside ordinary single- or double-quoted
strings while preserving valid heredoc handling. Add a regression case covering
an echoed quoted heredoc opener followed by a destructive command, ensuring the
command is not blanked and the guard rejects it.
| # token, so EVERY argument is a candidate target: `rm -rf build/ /` (the classic | ||
| # stray-space typo) is exactly as blocked as `rm -rf /`. -f is irrelevant: rm -r | ||
| # deletes without prompting in non-interactive shells. | ||
| _RM_INVOCATION = re.compile(r"\b(?:rm|ri|del|erase|remove-item)\s+(.*)", re.IGNORECASE) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '120,220p' claude/hooks/pretool-destructive-guard.py
printf '\n--- tests/search ---\n'
rg -n "rd|rmdir|remove-item|pretool-destructive-guard|RM_INVOCATION|destructive-guard" claude tests .Repository: blyatiful1/fable-protocol
Length of output: 50382
🏁 Script executed:
sed -n '240,290p' tests/test_destructive_guard.py
printf '\n--- powershell alias search ---\n'
rg -n "rmdir|rd|remove-item|Remove-Item|PowerShell" tests/test_destructive_guard.py claude/hooks/pretool-destructive-guard.pyRepository: blyatiful1/fable-protocol
Length of output: 13129
🏁 Script executed:
sed -n '1,120p' claude/hooks/pretool-destructive-guard.py
printf '\n--- around rm parser ---\n'
sed -n '150,210p' claude/hooks/pretool-destructive-guard.pyRepository: blyatiful1/fable-protocol
Length of output: 8957
Cover the rd and rmdir PowerShell aliases. The PowerShell matcher already handles Remove-Item and ri, but not rd/rmdir, so rmdir -Recurse C:\ still bypasses the guard. Add regression cases for both aliases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@claude/hooks/pretool-destructive-guard.py` at line 150, Update the
_RM_INVOCATION regex to recognize the PowerShell rd and rmdir aliases alongside
the existing destructive commands, preserving case-insensitive matching and
argument capture. Add regression cases covering both aliases, including the
recursive rmdir form shown in the comment, and verify they trigger the
destructive-operation guard.
| def test_powershell_recurse_switch_syntax_blocks(tmp_path): | ||
| repo = make_repo(tmp_path, dirty=False) | ||
| assert run_hook("Remove-Item -Recurse:$true C:\\", cwd=repo, | ||
| tool_name="PowerShell").returncode == 2 | ||
| assert run_hook("Remove-Item -rec C:\\", cwd=repo, tool_name="PowerShell").returncode == 2 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover the disabled -Recurse:$false form.
The supplied matcher treats -Recurse:$false as recursive, so it will incorrectly block a non-recursive command. Parse the switch value and add an allowed regression case.
Suggested regression case
def test_powershell_recurse_switch_syntax_blocks(tmp_path):
repo = make_repo(tmp_path, dirty=False)
assert run_hook("Remove-Item -Recurse:$true C:\\", cwd=repo,
tool_name="PowerShell").returncode == 2
assert run_hook("Remove-Item -rec C:\\", cwd=repo, tool_name="PowerShell").returncode == 2
+ assert run_hook("Remove-Item -Recurse:$false C:\\", cwd=repo,
+ tool_name="PowerShell").returncode == 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_powershell_recurse_switch_syntax_blocks(tmp_path): | |
| repo = make_repo(tmp_path, dirty=False) | |
| assert run_hook("Remove-Item -Recurse:$true C:\\", cwd=repo, | |
| tool_name="PowerShell").returncode == 2 | |
| assert run_hook("Remove-Item -rec C:\\", cwd=repo, tool_name="PowerShell").returncode == 2 | |
| def test_powershell_recurse_switch_syntax_blocks(tmp_path): | |
| repo = make_repo(tmp_path, dirty=False) | |
| assert run_hook("Remove-Item -Recurse:$true C:\\", cwd=repo, | |
| tool_name="PowerShell").returncode == 2 | |
| assert run_hook("Remove-Item -rec C:\\", cwd=repo, tool_name="PowerShell").returncode == 2 | |
| assert run_hook("Remove-Item -Recurse:$false C:\\", cwd=repo, | |
| tool_name="PowerShell").returncode == 0 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_destructive_guard.py` around lines 333 - 337, Update the
PowerShell destructive-command matcher exercised by
test_powershell_recurse_switch_syntax_blocks to parse the value of the -Recurse
switch, treating -Recurse:$false as non-recursive while continuing to block
enabled or abbreviated recursive forms. Add a regression assertion in
test_powershell_recurse_switch_syntax_blocks confirming the disabled form is
allowed.
| def test_doctor_warns_on_windows_python_settings(tmp_path): | ||
| # Finding 5: a Windows snippet (bare `python`) merged on a POSIX box has hooks | ||
| # that never fire; doctor.sh must warn, mirroring doctor.ps1's python3 guard. | ||
| claude = tmp_path / "claude" | ||
| r = run(INSTALL, claude) | ||
| assert r.returncode == 0 | ||
| win = json.loads((ROOT / "claude" / "settings" / "settings-snippet-windows.json").read_text()) | ||
| (claude / "settings.json").write_text(json.dumps(win)) | ||
| r = run(DOCTOR, claude, state_dir=tmp_path / "state") | ||
| assert "bare 'python'" in r.stdout, r.stdout |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that this diagnostic remains non-fatal.
The test currently passes even if doctor.sh prints the warning and exits with failure.
Proposed test fix
r = run(DOCTOR, claude, state_dir=tmp_path / "state")
+ assert r.returncode == 0, r.stdout + r.stderr
assert "bare 'python'" in r.stdout, r.stdout📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_doctor_warns_on_windows_python_settings(tmp_path): | |
| # Finding 5: a Windows snippet (bare `python`) merged on a POSIX box has hooks | |
| # that never fire; doctor.sh must warn, mirroring doctor.ps1's python3 guard. | |
| claude = tmp_path / "claude" | |
| r = run(INSTALL, claude) | |
| assert r.returncode == 0 | |
| win = json.loads((ROOT / "claude" / "settings" / "settings-snippet-windows.json").read_text()) | |
| (claude / "settings.json").write_text(json.dumps(win)) | |
| r = run(DOCTOR, claude, state_dir=tmp_path / "state") | |
| assert "bare 'python'" in r.stdout, r.stdout | |
| def test_doctor_warns_on_windows_python_settings(tmp_path): | |
| # Finding 5: a Windows snippet (bare `python`) merged on a POSIX box has hooks | |
| # that never fire; doctor.sh must warn, mirroring doctor.ps1's python3 guard. | |
| claude = tmp_path / "claude" | |
| r = run(INSTALL, claude) | |
| assert r.returncode == 0 | |
| win = json.loads((ROOT / "claude" / "settings" / "settings-snippet-windows.json").read_text()) | |
| (claude / "settings.json").write_text(json.dumps(win)) | |
| r = run(DOCTOR, claude, state_dir=tmp_path / "state") | |
| assert r.returncode == 0, r.stdout + r.stderr | |
| assert "bare 'python'" in r.stdout, r.stdout |
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 114-114: use jsonify instead of json.dumps for JSON output
Context: json.dumps(win)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_install_doctor.py` around lines 108 - 117, Update
test_doctor_warns_on_windows_python_settings to assert r.returncode == 0 after
running DOCTOR, while retaining the existing bare 'python' diagnostic assertion.
| readme = (REPO / "README.md").read_text(encoding="utf-8") | ||
| assert "/big-task" in readme | ||
| assert "ultracode" in readme, "README must document the ultracode composition story" | ||
| skill = (REPO / "claude/skills/orchestrate/SKILL.md").read_text() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the remaining documentation read UTF-8 explicit.
Line 89 is fixed, but Line 92 still calls read_text() without an encoding for claude/skills/orchestrate/SKILL.md. This keeps the test dependent on the host locale on Windows and can misdecode or fail on non-ASCII content.
Proposed fix
- skill = (REPO / "claude/skills/orchestrate/SKILL.md").read_text()
+ skill = (REPO / "claude/skills/orchestrate/SKILL.md").read_text(encoding="utf-8")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| readme = (REPO / "README.md").read_text(encoding="utf-8") | |
| assert "/big-task" in readme | |
| assert "ultracode" in readme, "README must document the ultracode composition story" | |
| skill = (REPO / "claude/skills/orchestrate/SKILL.md").read_text() | |
| readme = (REPO / "README.md").read_text(encoding="utf-8") | |
| assert "/big-task" in readme | |
| assert "ultracode" in readme, "README must document the ultracode composition story" | |
| skill = (REPO / "claude/skills/orchestrate/SKILL.md").read_text(encoding="utf-8") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_small_tier.py` around lines 89 - 92, Update the SKILL.md read in
the test around the skill assignment to pass encoding="utf-8" explicitly,
matching the existing README read and removing host-locale dependence.
| if (c === '/' && c2 === '/') { while (i < src.length && src[i] !== '\n') i++; continue } | ||
| if (c === '/' && c2 === '*') { i += 2; while (i < src.length && !(src[i] === '*' && src[i + 1] === '/')) i++; i++; continue } | ||
| if (c === "'" || c === '"') { i++; while (i < src.length && src[i] !== c) { if (src[i] === '\\') i++; i++ } out += ' '; continue } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file first
ast-grep outline tools/check-workflows.mjs --view expanded || true
# Show the relevant section with line numbers
sed -n '1,180p' tools/check-workflows.mjs | cat -n
# Run a read-only probe that mimics the string/comment stripping logic
python3 - <<'PY'
src = "const slash = /[//]/; Date.now()"
out = ""
i = 0
while i < len(src):
c = src[i]
c2 = src[i+1] if i + 1 < len(src) else ""
if c == '/' and c2 == '/':
while i < len(src) and src[i] != '\n':
i += 1
continue
if c == '/' and c2 == '*':
i += 2
while i < len(src) and not (src[i] == '*' and i + 1 < len(src) and src[i+1] == '/'):
i += 1
i += 1
continue
if c == "'" or c == '"':
i += 1
while i < len(src) and src[i] != c:
if src[i] == '\\':
i += 1
i += 1
out += ' '
continue
out += c
i += 1
print("INPUT:", src)
print("OUTPUT:", out)
print("contains Date.now:", "Date.now" in out)
PYRepository: blyatiful1/fable-protocol
Length of output: 5819
Handle regex literals before comment stripping.
stripStringsAndComments() treats // inside a regex literal as a line comment, so a line like const slash = /[//]/; Date.now() gets truncated before the forbidden call and bypasses the scan. Use a real JS lexer/parser, or add regex-literal handling before dropping comments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/check-workflows.mjs` around lines 56 - 58, The
stripStringsAndComments() scanner must recognize JavaScript regex literals
before interpreting // or /* as comments, so patterns such as /[//]/ do not
truncate the source before forbidden calls. Add regex-literal handling with
correct escaping and character-class support, or replace the scanner with a real
JavaScript lexer/parser, while preserving existing string and comment stripping
behavior.
| function Test-AgentSame([string]$A, [string]$B) { | ||
| if (-not (Test-Path $B -PathType Leaf)) { return $false } | ||
| $ca = (Get-NormContent $A) -split "`n" | Where-Object { $_ -notmatch '^model: ' } | ||
| $cb = (Get-NormContent $B) -split "`n" | Where-Object { $_ -notmatch '^model: ' } | ||
| return (($ca -join "`n") -eq ($cb -join "`n")) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Only ignore installer-injected frontmatter pins.
Removing every model: line from both files hides legitimate repository pin changes—and matching body text—from drift detection. Remove a frontmatter pin from the installed copy only when the source has no pin.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/doctor.ps1` around lines 43 - 48, Update Test-AgentSame so it
conditionally ignores only an installer-injected frontmatter model pin in the
installed copy: detect whether the source content contains a model pin, and
remove the installed copy’s corresponding frontmatter pin only when the source
has none. Preserve all source pins and any model: text outside frontmatter, then
compare the normalized contents as before.
1 participant
An adversarial re-audit of the whole kit (multi-agent finder/verifier fleet, then a review of the resulting diff) surfaced defects the earlier self-review passes missed. This PR fixes the verified findings and adds a runnable hook demo.
Highlights
rmargument (rm -rf build/ /is caught), long-form + PowerShell spellings (Remove-Item -Recurse, without misreading-Force), Windows targets and<target>/*globs, quoted-heredoc bodies as literal data, and guards the PowerShell tool.install.ps1/doctor.ps1now ship a UTF-8 BOM — Windows PowerShell 5.1 (the only PowerShell there) ParserError'd on the documentedpowershell -Fileinstall. Doctors gained event-level wiring, staleness, and wrong-interpreter checks; CI adds a PS 5.1 step.tools/demo.pyruns the real hooks against 5 failure modes in a sandbox (in CI viatests/test_demo.py).Benchmark honesty
bench/RESULTS.mdrecords a fresh 2026-07-16 A/B re-run: current stock Opus 4.8 now aces the task, so there is no measurable score delta (and n=3 is underpowered). The deterministic floor still fires —demo.pyproves it — which is exactly the succession-doctrine outcome (keep the cheap floor, retire ceremony the model outgrew).Verification
Suite: 226 passing, 23 skipped (the Windows baseline was failing 6); ~34 new regression tests;
check-workflows.mjsgreen;demo.py5/5.🤖 Generated with Claude Code
Summary by CodeRabbit