fix: enforce worker TODO.md restriction with multi-layer guards (t173)#649
fix: enforce worker TODO.md restriction with multi-layer guards (t173)#649marcusquinn merged 1 commit intomainfrom
Conversation
Workers (headless dispatch runners) must never edit TODO.md — the supervisor owns all TODO.md updates. Without enforcement, multiple workers + supervisor pushing to TODO.md on main simultaneously causes merge conflicts. Three layers of protection: 1. Prompt-level: build_dispatch_cmd() injects explicit 'DO NOT EDIT TODO.md' restriction into every worker prompt (supervisor-helper.sh) 2. Context-level: loop-common.sh skips TODO.md reads in headless mode so workers never see TODO.md tasks in their re-anchor context, and adds MANDATORY Worker Restrictions block to the re-anchor prompt 3. Git-level: full-loop-helper.sh installs a pre-commit hook in the worker worktree that rejects any commit containing TODO.md or todo/ changes when FULL_LOOP_HEADLESS=true. Cleaned up on loop cancel/complete.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @marcusquinn, 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 significantly enhances the robustness of the system by implementing a comprehensive strategy to manage Highlights
Changelog
Activity
Using Gemini Code AssistThe 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
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 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
|
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Sun Feb 8 18:55:13 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
![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 a multi-layered defense to prevent workers from modifying TODO.md, which is a solid approach to fixing the race condition. The changes include prompt injection, context isolation, and a git pre-commit hook. The implementation is mostly good, but I've found a critical issue in the git hook removal logic that could corrupt the pre-commit file, and a medium-severity issue regarding duplicated code that affects maintainability. My review includes suggestions to address these points.
| install_headless_todo_guard() { | ||
| local git_dir | ||
| git_dir=$(git rev-parse --git-dir 2>/dev/null || echo "") | ||
| if [[ -z "$git_dir" ]]; then | ||
| return 0 | ||
| fi | ||
|
|
||
| local hooks_dir="$git_dir/hooks" | ||
| local hook_file="$hooks_dir/pre-commit" | ||
| local guard_marker="# t173-headless-todo-guard" | ||
|
|
||
| mkdir -p "$hooks_dir" | ||
|
|
||
| # If a pre-commit hook already exists, append our guard (if not already present) | ||
| if [[ -f "$hook_file" ]]; then | ||
| if grep -q "$guard_marker" "$hook_file" 2>/dev/null; then | ||
| return 0 # Already installed | ||
| fi | ||
| # Append to existing hook | ||
| cat >> "$hook_file" << 'GUARD' | ||
|
|
||
| # t173-headless-todo-guard | ||
| # Block TODO.md and planning file commits in headless worker mode | ||
| if [[ "${FULL_LOOP_HEADLESS:-false}" == "true" ]]; then | ||
| if git diff --cached --name-only | grep -qE '^(TODO\.md|todo/)'; then | ||
| echo "[t173 GUARD] BLOCKED: Headless workers must not commit TODO.md or todo/ files." | ||
| echo "[t173 GUARD] The supervisor owns all TODO.md updates. Put notes in commit messages or PR body." | ||
| exit 1 | ||
| fi | ||
| fi | ||
| GUARD | ||
| else | ||
| # Create new hook | ||
| cat > "$hook_file" << 'GUARD' | ||
| #!/usr/bin/env bash | ||
| # t173-headless-todo-guard | ||
| # Block TODO.md and planning file commits in headless worker mode | ||
| if [[ "${FULL_LOOP_HEADLESS:-false}" == "true" ]]; then | ||
| if git diff --cached --name-only | grep -qE '^(TODO\.md|todo/)'; then | ||
| echo "[t173 GUARD] BLOCKED: Headless workers must not commit TODO.md or todo/ files." | ||
| echo "[t173 GUARD] The supervisor owns all TODO.md updates. Put notes in commit messages or PR body." | ||
| exit 1 | ||
| fi | ||
| fi | ||
| GUARD | ||
| chmod +x "$hook_file" | ||
| fi | ||
|
|
||
| return 0 | ||
| } |
There was a problem hiding this comment.
To make the removal of this git hook guard more robust, I recommend adding unique start and end markers to the heredoc. This will allow for a more reliable removal process that isn't prone to errors from nested if/fi blocks. I'll suggest a corresponding change in remove_headless_todo_guard.
# Install a git pre-commit hook that blocks TODO.md changes in headless mode (t173)
# This is a hard guard — even if the AI agent tries to commit TODO.md, git rejects it.
install_headless_todo_guard() {
local git_dir
git_dir=$(git rev-parse --git-dir 2>/dev/null || echo "")
if [[ -z "$git_dir" ]]; then
return 0
fi
local hooks_dir="$git_dir/hooks"
local hook_file="$hooks_dir/pre-commit"
local guard_marker="# t173-headless-todo-guard-start"
mkdir -p "$hooks_dir"
# If a pre-commit hook already exists, append our guard (if not already present)
if [[ -f "$hook_file" ]]; then
if grep -q "$guard_marker" "$hook_file" 2>/dev/null; then
return 0 # Already installed
fi
# Append to existing hook
cat >> "$hook_file" << 'GUARD'
# t173-headless-todo-guard-start
# Block TODO.md and planning file commits in headless worker mode
if [[ "${FULL_LOOP_HEADLESS:-false}" == "true" ]]; then
if git diff --cached --name-only | grep -qE '^(TODO\.md|todo/)'; then
echo "[t173 GUARD] BLOCKED: Headless workers must not commit TODO.md or todo/ files."
echo "[t173 GUARD] The supervisor owns all TODO.md updates. Put notes in commit messages or PR body."
exit 1
fi
fi
# t173-headless-todo-guard-end
GUARD
else
# Create new hook
cat > "$hook_file" << 'GUARD'
#!/usr/bin/env bash
# t173-headless-todo-guard-start
# Block TODO.md and planning file commits in headless worker mode
if [[ "${FULL_LOOP_HEADLESS:-false}" == "true" ]]; then
if git diff --cached --name-only | grep -qE '^(TODO\.md|todo/)'; then
echo "[t173 GUARD] BLOCKED: Headless workers must not commit TODO.md or todo/ files."
echo "[t173 GUARD] The supervisor owns all TODO.md updates. Put notes in commit messages or PR body."
exit 1
fi
fi
# t173-headless-todo-guard-end
GUARD
chmod +x "$hook_file"
fi
return 0
}| remove_headless_todo_guard() { | ||
| local git_dir | ||
| git_dir=$(git rev-parse --git-dir 2>/dev/null || echo "") | ||
| if [[ -z "$git_dir" ]]; then | ||
| return 0 | ||
| fi | ||
|
|
||
| local hook_file="$git_dir/hooks/pre-commit" | ||
| if [[ ! -f "$hook_file" ]]; then | ||
| return 0 | ||
| fi | ||
|
|
||
| # Remove the guard block (from marker to end of guard) | ||
| if grep -q "t173-headless-todo-guard" "$hook_file" 2>/dev/null; then | ||
| # Use sed to remove the guard block | ||
| local tmp_file | ||
| tmp_file=$(mktemp) | ||
| awk '/# t173-headless-todo-guard/{skip=1} /^fi$/ && skip{skip=0; next} !skip' "$hook_file" > "$tmp_file" | ||
| mv "$tmp_file" "$hook_file" | ||
| chmod +x "$hook_file" | ||
| fi | ||
|
|
||
| return 0 | ||
| } |
There was a problem hiding this comment.
The awk command used here is not robust. It stops skipping lines at the first fi it encounters. Since the guard block contains a nested if, this will corrupt the pre-commit hook by only partially removing the block.
To fix this, I've suggested adding unique start/end markers in install_headless_todo_guard. With those markers, sed can be used here for a reliable removal.
# Remove the t173 headless guard from pre-commit hook (cleanup)
remove_headless_todo_guard() {
local git_dir
git_dir=$(git rev-parse --git-dir 2>/dev/null || echo "")
if [[ -z "$git_dir" ]]; then
return 0
fi
local hook_file="$git_dir/hooks/pre-commit"
if [[ ! -f "$hook_file" ]]; then
return 0
fi
# Remove the guard block (from marker to end of guard)
if grep -q "# t173-headless-todo-guard-start" "$hook_file" 2>/dev/null; then
# Use sed to remove the guard block
local tmp_file
tmp_file=$(mktemp)
sed '/^# t173-headless-todo-guard-start$/,/^# t173-headless-todo-guard-end$/d' "$hook_file" > "$tmp_file"
mv "$tmp_file" "$hook_file"
chmod +x "$hook_file"
fi
return 0
}| # Build headless worker restriction block (t173) | ||
| local headless_restriction="" | ||
| if [[ "$is_headless" == "true" ]]; then | ||
| headless_restriction=" | ||
| ## MANDATORY Worker Restrictions (t173 - Headless Mode) | ||
|
|
||
| - **Do NOT edit, commit, or push TODO.md** — the supervisor owns all TODO.md updates. | ||
| - **Do NOT edit todo/PLANS.md or todo/tasks/*** — these are supervisor-managed. | ||
| - Report status via exit code, log output, and PR creation only. | ||
| - Put task notes in commit messages or PR body, never in TODO.md. | ||
| - Work ONLY on the assigned task described above. Do not pick tasks from TODO.md. | ||
| " | ||
| fi |
There was a problem hiding this comment.
This block of text defining worker restrictions is very similar to another block added in .agents/scripts/supervisor-helper.sh. Duplicating this text can lead to maintainability issues, as changes would need to be made in multiple places, and they have already diverged slightly.
Since both scripts source shared-constants.sh, consider defining this restriction text as a readonly variable there and reusing it in both places. This would ensure consistency and simplify future updates.
| # t173: Explicit worker restriction — prevents TODO.md race condition | ||
| prompt="$prompt | ||
|
|
||
| ## MANDATORY Worker Restrictions (t173) | ||
| - Do NOT edit, commit, or push TODO.md — the supervisor owns all TODO.md updates. | ||
| - Do NOT edit todo/PLANS.md or todo/tasks/* — these are supervisor-managed. | ||
| - Report status via exit code, log output, and PR creation only. | ||
| - Put task notes in commit messages or PR body, never in TODO.md." | ||
|
|
There was a problem hiding this comment.
This block of text defining worker restrictions is very similar to another block added in .agents/scripts/loop-common.sh. Duplicating this text can lead to maintainability issues, as changes would need to be made in multiple places, and they have already diverged slightly.
Since both scripts source shared-constants.sh, consider defining this restriction text as a readonly variable there and reusing it in both places. This would ensure consistency and simplify future updates.
1 participant
Summary
Changes
supervisor-helper.sh— Prompt-level guardbuild_dispatch_cmd()now injects explicit "DO NOT EDIT TODO.md" restriction into every worker promptloop-common.sh— Context-level guardFULL_LOOP_HEADLESS=truefull-loop-helper.sh— Git-level guardinstall_headless_todo_guard()installs a pre-commit hook in the worker worktree that rejects any commit containingTODO.mdortodo/changes whenFULL_LOOP_HEADLESS=trueremove_headless_todo_guard()cleans up the hook on loop cancel/completecmd_start()when--headlessis activeTesting
bash -nsyntax check: all 3 files passshellcheck -S error: zero violations on all 3 filestest-smoke-help.sh: 284 passed, 0 failedtest-batch-quality-hardening.sh: 55/56 passed (1 expected diff: deployed vs repo version)