refactor: reduce coordinator context bloat by removing redundant verification layers#97
Merged
refactor: reduce coordinator context bloat by removing redundant verification layers#97
Conversation
…heck) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ion) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…odic rules Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…model Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…dic artifact review Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cution Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…model Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…re-reads Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…r model Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fixed stale "5 verification layers" reference in failure-recovery.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
All Phase 3 verification checks passed (3.1-3.9). Local quality check 4.1 passed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR bumps ralph-specum to version 4.1.2, refactors the coordinator verification model from five layers to three across docs and scripts, inlines a TASK_BLOCK/current-task context into the stop-hook, and adds planning/progress docs for a context‑bloat refactor. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(66,133,244,0.5)
participant Coordinator
end
rect rgba(219,68,55,0.5)
participant SpecExecutor
end
rect rgba(15,157,88,0.5)
participant StopWatcher
end
rect rgba(128,0,128,0.5)
participant SpecReviewer/Git
end
Coordinator->>SpecExecutor: Delegate task (include TASK_START_SHA) & request self-verification (contradiction detection)
SpecExecutor-->>Coordinator: Return TASK_COMPLETE signal (or ALL_TASKS_COMPLETE)
Coordinator->>StopWatcher: Ask to continue / surface Current Task block and verify TASK_COMPLETE signal
StopWatcher->>SpecReviewer/Git: Run periodic artifact review (on trigger: phase boundary / every 5th / final)
SpecReviewer/Git-->>Coordinator: Review result (pass / request fixes)
Coordinator->>Coordinator: Advance taskIndex if 3 verification layers pass
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/ralph-specum/hooks/scripts/stop-watcher.sh (1)
233-254:⚠️ Potential issue | 🟡 MinorHeredoc delimiter
EOFcould be truncated by task content.If any line in
tasks.mdis exactlyEOF, bash will close the heredoc early, silently truncatingREASON. The probability is very low for typical task content, but using a unique sentinel removes the risk entirely.🛡️ Suggested fix — use a longer unique delimiter
- REASON=$(cat <<EOF + REASON=$(cat <<'STOP_WATCHER_REASON_EOF' Continue spec: $SPEC_NAME (Task $((TASK_INDEX + 1))/$TOTAL_TASKS, Iter $GLOBAL_ITERATION) ... -EOF +STOP_WATCHER_REASON_EOF )Note: switching to
<<'STOP_WATCHER_REASON_EOF'(quoted) would suppress variable expansion. To keep expansion, use<<STOP_WATCHER_REASON_EOF(unquoted) with only the delimiter changed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ralph-specum/hooks/scripts/stop-watcher.sh` around lines 233 - 254, The heredoc for REASON uses the plain EOF delimiter which can be accidentally closed if any task line equals EOF; update the heredoc to use a unique sentinel (e.g. STOP_WATCHER_REASON_EOF) while preserving variable expansion (use an unquoted delimiter) so REASON, SPEC_NAME, TASK_BLOCK, TASK_INDEX, TOTAL_TASKS, GLOBAL_ITERATION, etc. expand correctly; locate the REASON assignment and replace <<EOF / EOF with <<STOP_WATCHER_REASON_EOF / STOP_WATCHER_REASON_EOF (or another long unique token) to eliminate accidental truncation.plugins/ralph-specum/references/verification-layers.md (1)
49-79:⚠️ Potential issue | 🟡 MinorFenced code block at line 49 is missing a language specifier.
Add a language tag to resolve the MD040 markdownlint warning.
📝 Proposed fix
-``` +```text Set reviewIteration = 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ralph-specum/references/verification-layers.md` around lines 49 - 79, The fenced code block in plugins/ralph-specum/references/verification-layers.md that begins with "Set reviewIteration = 1" is missing a language tag; add a language specifier (e.g., ```text) to the opening fence so the block becomes ```text ... ``` to satisfy MD040 and avoid markdownlint warnings.
🧹 Nitpick comments (3)
plugins/ralph-specum/references/coordinator-pattern.md (1)
269-272:taskIndex % 5 == 0triggers artifact review on the very first task (index 0).This runs Layer 3 on task 1 (1-based) before meaningful artifacts exist, then again at tasks 6, 11, 16… If the intent is "every 5 completed tasks" (tasks 5, 10, 15… in 1-based), the condition should be
(taskIndex + 1) % 5 == 0. The current behavior is documented in the design notes as intentional, but consider adding a comment here to make the "starts-at-0" semantics explicit and prevent future confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ralph-specum/references/coordinator-pattern.md` around lines 269 - 272, The condition `taskIndex % 5 == 0` triggers reviews at index 0 (first task) which may be surprising; update the doc around the condition (the line describing "Every 5th task" and the `taskIndex % 5 == 0` expression) to either change the condition to `(taskIndex + 1) % 5 == 0` if you want reviews after every five completed tasks (1-based 5,10,15...) or, if the current starts-at-0 behavior is intended, add a short clarifying comment stating that `taskIndex` is zero-based and that Layer 3 intentionally runs on task index 0, 5, 10..., to prevent future confusion.plugins/ralph-specum/references/verification-layers.md (2)
169-170:ALL_TASKS_COMPLETEhandling is not addressed in the verification layers.The retrieved learning notes that spec-executor outputs
ALL_TASKS_COMPLETE(distinct fromTASK_COMPLETE) when the final task finishes. This document's Layer 2 only validatesTASK_COMPLETEpresence; it is silent on whetherALL_TASKS_COMPLETEsatisfies Layer 2 or bypasses these layers entirely. The final-task Layer 3 trigger (taskIndex == totalTasks - 1) assumesTASK_COMPLETEwas received for that task, but if the executor emitsALL_TASKS_COMPLETEinstead, Layer 2 would reject it.Confirm whether
ALL_TASKS_COMPLETEis accepted as equivalent toTASK_COMPLETEfor Layer 2 purposes, and document the behavior explicitly here.Based on learnings: "Task executor must output TASK_COMPLETE for coordinator to advance; output ALL_TASKS_COMPLETE when all tasks are done."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ralph-specum/references/verification-layers.md` around lines 169 - 170, The document omits how ALL_TASKS_COMPLETE should be treated by Layer 2 and Layer 3; update the verification-layers.md to explicitly state that spec-executor's ALL_TASKS_COMPLETE is accepted as equivalent to TASK_COMPLETE for Layer 2 checks (i.e., Layer 2 should treat ALL_TASKS_COMPLETE as satisfying the TASK_COMPLETE presence requirement), and clarify that Layer 3's final-task trigger (taskIndex == totalTasks - 1) must accept either TASK_COMPLETE or ALL_TASKS_COMPLETE from spec-executor when deciding to run final-task verification; reference the symbols ALL_TASKS_COMPLETE, TASK_COMPLETE, Layer 2, Layer 3, spec-executor and the taskIndex == totalTasks - 1 condition in the text so the behavior is unambiguous.
44-45: The formula for computing "next task N" in the skip message is undefined.The skip annotation
"Skipping artifact review (next at task N)"requires the coordinator to computeN, but the document provides no formula. Agents implementing this will deriveNinconsistently. Define it explicitly, e.g.:📝 Proposed clarification
-When skipped, coordinator appends to .progress.md: -"Skipping artifact review (next at task N)" where N is the next task index that would trigger review. +When skipped, coordinator appends to .progress.md: +"Skipping artifact review (next at task N)" where N is the smallest index > taskIndex satisfying +any trigger condition: next phase-boundary task, or (taskIndex + (5 - taskIndex % 5)) for the periodic rule.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ralph-specum/references/verification-layers.md` around lines 44 - 45, Update the document to explicitly define how the coordinator computes N for the skip message "Skipping artifact review (next at task N)"; state that N is the next task index that is a review boundary and give the formula using the configured review interval (e.g., if the config key is review_interval or review_every_tasks then N = current_task_index + (review_interval - (current_task_index % review_interval)) with a note that if current_task_index % review_interval == 0 then N = current_task_index + review_interval), and add a short example showing the computation for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/ralph-specum/hooks/scripts/stop-watcher.sh`:
- Line 250: The inline comment "Verify all 3 layers before advancing (see
implement.md Section 7)" is pointing to a non-existent section; update that
comment in stop-watcher.sh (the string "Verify all 3 layers before advancing
(see implement.md Section 7)") to reference the correct documentation, e.g.,
"Verify all 3 layers before advancing (see coordinator-pattern.md §
'Verification Layers')" or "… (see verification-layers.md)", ensuring the doc
filename and section title are accurate.
In `@plugins/ralph-specum/references/verification-layers.md`:
- Around line 39-42: The "Every 5th task" condition currently uses taskIndex % 5
== 0 which triggers on taskIndex 0 and conflicts with the recommended 2–3 task
checkpoint cadence; update the Layer 3 condition to avoid firing on the first
task (e.g., require taskIndex > 0 && taskIndex % 5 == 0 or use (taskIndex + 1) %
5 == 0) and—preferably—adjust the cadence to meet the guidance by replacing the
5-task interval with a 2–3 task interval (e.g., (taskIndex + 1) % 3 == 0 &&
taskIndex > 0) while keeping the Phase boundary and Final task conditions intact
(references: "Layer 3", "taskIndex", "totalTasks").
- Line 53: Replace the brittle usage of the literal git command "git diff
--name-only HEAD~1" with a range diff using a recorded start SHA (e.g., "git
diff --name-only $TASK_START_SHA HEAD"); capture and store the task start commit
SHA in the task initialization logic (set TASK_START_SHA before any task
commits) and use that variable wherever "git diff --name-only HEAD~1" appears
(the two occurrences noted in the diff), while keeping the existing fallback to
the task's Files list; ensure the variable is persisted into the execution
context so the diff range reliably reflects all commits made during the task.
In `@specs/fix-impl-context-bloat/.progress.md`:
- Line 150: The file contains a duplicate section header "## Learnings" which
triggers markdownlint MD024; locate the second occurrence (the one around the
later content, shown in the diff as "## Learnings") and rename it to a distinct
heading such as "## Post-Completion Learnings" (or another descriptive variant)
so headings are unique and tooling won't warn.
In `@specs/fix-impl-context-bloat/tasks.md`:
- Around line 467-469: The fenced code block shown (containing "Phase 1 (POC:
all 6 file edits) → Phase 2 (Refactor: orphan cleanup) → Phase 3 (Testing:
content verification) → Phase 4 (Quality Gates + PR)") needs an explicit
language specifier to satisfy markdownlint MD040; update the opening fence from
``` to ```text (and keep the closing fence as ```), so the block becomes a
```text fenced code block containing the same line.
---
Outside diff comments:
In `@plugins/ralph-specum/hooks/scripts/stop-watcher.sh`:
- Around line 233-254: The heredoc for REASON uses the plain EOF delimiter which
can be accidentally closed if any task line equals EOF; update the heredoc to
use a unique sentinel (e.g. STOP_WATCHER_REASON_EOF) while preserving variable
expansion (use an unquoted delimiter) so REASON, SPEC_NAME, TASK_BLOCK,
TASK_INDEX, TOTAL_TASKS, GLOBAL_ITERATION, etc. expand correctly; locate the
REASON assignment and replace <<EOF / EOF with <<STOP_WATCHER_REASON_EOF /
STOP_WATCHER_REASON_EOF (or another long unique token) to eliminate accidental
truncation.
In `@plugins/ralph-specum/references/verification-layers.md`:
- Around line 49-79: The fenced code block in
plugins/ralph-specum/references/verification-layers.md that begins with "Set
reviewIteration = 1" is missing a language tag; add a language specifier (e.g.,
```text) to the opening fence so the block becomes ```text ... ``` to satisfy
MD040 and avoid markdownlint warnings.
---
Nitpick comments:
In `@plugins/ralph-specum/references/coordinator-pattern.md`:
- Around line 269-272: The condition `taskIndex % 5 == 0` triggers reviews at
index 0 (first task) which may be surprising; update the doc around the
condition (the line describing "Every 5th task" and the `taskIndex % 5 == 0`
expression) to either change the condition to `(taskIndex + 1) % 5 == 0` if you
want reviews after every five completed tasks (1-based 5,10,15...) or, if the
current starts-at-0 behavior is intended, add a short clarifying comment stating
that `taskIndex` is zero-based and that Layer 3 intentionally runs on task index
0, 5, 10..., to prevent future confusion.
In `@plugins/ralph-specum/references/verification-layers.md`:
- Around line 169-170: The document omits how ALL_TASKS_COMPLETE should be
treated by Layer 2 and Layer 3; update the verification-layers.md to explicitly
state that spec-executor's ALL_TASKS_COMPLETE is accepted as equivalent to
TASK_COMPLETE for Layer 2 checks (i.e., Layer 2 should treat ALL_TASKS_COMPLETE
as satisfying the TASK_COMPLETE presence requirement), and clarify that Layer
3's final-task trigger (taskIndex == totalTasks - 1) must accept either
TASK_COMPLETE or ALL_TASKS_COMPLETE from spec-executor when deciding to run
final-task verification; reference the symbols ALL_TASKS_COMPLETE,
TASK_COMPLETE, Layer 2, Layer 3, spec-executor and the taskIndex == totalTasks -
1 condition in the text so the behavior is unambiguous.
- Around line 44-45: Update the document to explicitly define how the
coordinator computes N for the skip message "Skipping artifact review (next at
task N)"; state that N is the next task index that is a review boundary and give
the formula using the configured review interval (e.g., if the config key is
review_interval or review_every_tasks then N = current_task_index +
(review_interval - (current_task_index % review_interval)) with a note that if
current_task_index % review_interval == 0 then N = current_task_index +
review_interval), and add a short example showing the computation for clarity.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@plugins/ralph-specum/hooks/scripts/stop-watcher.sh`:
- Line 251: Update the stale cross-reference in the comment inside
stop-watcher.sh that currently reads "Verify all 3 layers before advancing (see
implement.md Section 7)"—either remove the parenthetical reference or replace it
with the correct document/section name/number that matches the updated
implement.md, ensuring the comment now reads something like "Verify all 3 layers
before advancing" or "Verify all 3 layers before advancing (see implement.md:
<correct section/title>)" so the reference is accurate.
- Fix stale cross-references in stop-watcher.sh (implement.md Section 7 → verification-layers.md) - Use unique heredoc sentinel (STOP_WATCHER_REASON_EOF) to prevent accidental truncation - Fix taskIndex % 5 == 0 firing on index 0 (now requires taskIndex > 0) - Replace brittle git diff HEAD~1 with range diff using $TASK_START_SHA - Add ALL_TASKS_COMPLETE handling for Layer 2 and Layer 3 - Define skip message N computation formula - Add language specifiers to fenced code blocks (MD040) - Rename duplicate "## Learnings" heading in .progress.md (MD024) - Bump version to 4.1.2 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/ralph-specum/references/coordinator-pattern.md (1)
228-233:⚠️ Potential issue | 🟡 Minor
ALL_TASKS_COMPLETEnot handled in the "After Delegation" trigger.This PR updated
verification-layers.md(Layer 2, line 26) to acceptALL_TASKS_COMPLETEas equivalent toTASK_COMPLETE, but line 230 here was not updated. If spec-executor emitsALL_TASKS_COMPLETEon the final task, the coordinator's "After Delegation" logic does not match it, potentially falling into the "no completion signal" path (line 234) and triggering a spurious retry.✏️ Proposed fix
-If spec-executor outputs TASK_COMPLETE (or qa-engineer outputs VERIFICATION_PASS): +If spec-executor outputs TASK_COMPLETE or ALL_TASKS_COMPLETE (or qa-engineer outputs VERIFICATION_PASS):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ralph-specum/references/coordinator-pattern.md` around lines 228 - 233, Update the "After Delegation" trigger so it treats ALL_TASKS_COMPLETE the same as TASK_COMPLETE: change the condition text that currently reads "If spec-executor outputs TASK_COMPLETE (or qa-engineer outputs VERIFICATION_PASS)" to accept "ALL_TASKS_COMPLETE" as an equivalent completion signal (e.g., "TASK_COMPLETE or ALL_TASKS_COMPLETE (or qa-engineer outputs VERIFICATION_PASS)"). Ensure references to the trigger name "After Delegation" and the tokens ALL_TASKS_COMPLETE and TASK_COMPLETE are updated so the coordinator will match either completion token before running verification layers and state update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/ralph-specum/hooks/scripts/stop-watcher.sh`:
- Around line 214-223: The awk script that builds TASK_BLOCK mis-handles task
headers because once the target header is found the `next` prevents `count++` so
subsequent headers still match; update the awk logic used with
TASK_INDEX/TASKS_FILE so header lines first check `if (found) { exit }` before
checking `if (count == idx) { found=1; print; next }` and keep the `count++` for
non-target headers—i.e., in the /^- \[[ x]\]/ action block, move the `if (found)
{ exit }` to be the first check so extraction stops after the matched task.
In `@plugins/ralph-specum/references/verification-layers.md`:
- Around line 177-181: The compact summary for the coordinator's verification
layers omits ALL_TASKS_COMPLETE; update the summary text for "Signal
verification" (Layer 2) to reflect that it accepts both TASK_COMPLETE and
ALL_TASKS_COMPLETE by changing the line to something like "2. Signal
verification - requires TASK_COMPLETE (or ALL_TASKS_COMPLETE)" so it matches the
explicit acceptance in the Layer 2 body and the mentions elsewhere.
In `@specs/.index/index-state.json`:
- Around line 33-56: The index contains two inconsistent entries: the spec named
"return-ralph-wrigum" uses "phase": "complete" but the canonical status is
"completed" — change its "phase" value to "completed" to match tooling
expectations; the spec "fix-impl-context-bloat" is still marked "phase":
"research" and lacks taskIndex/totalTasks despite all tasks being done — update
its "phase" to "completed" and set "taskIndex": 41 and "totalTasks": 41 (or
whatever the repo’s ALL_TASKS_COMPLETE convention requires). After editing the
JSON keys for the entries "return-ralph-wrigum" and "fix-impl-context-bloat",
mirror those same changes in specs/.index/index.md so the rendered status column
and progress reflect the fixes.
---
Outside diff comments:
In `@plugins/ralph-specum/references/coordinator-pattern.md`:
- Around line 228-233: Update the "After Delegation" trigger so it treats
ALL_TASKS_COMPLETE the same as TASK_COMPLETE: change the condition text that
currently reads "If spec-executor outputs TASK_COMPLETE (or qa-engineer outputs
VERIFICATION_PASS)" to accept "ALL_TASKS_COMPLETE" as an equivalent completion
signal (e.g., "TASK_COMPLETE or ALL_TASKS_COMPLETE (or qa-engineer outputs
VERIFICATION_PASS)"). Ensure references to the trigger name "After Delegation"
and the tokens ALL_TASKS_COMPLETE and TASK_COMPLETE are updated so the
coordinator will match either completion token before running verification
layers and state update.
---
Duplicate comments:
In `@plugins/ralph-specum/hooks/scripts/stop-watcher.sh`:
- Around line 240-247: The prompt emits "Delegate the task above" even when
TASK_BLOCK is empty; update the logic that builds the "## Current Task" block
(where TASK_BLOCK is set/used along with TASK_INDEX and SPEC_PATH) to detect an
empty TASK_BLOCK (or missing tasks.md / out-of-range TASK_INDEX) and in that
case replace step 2 with a directive to read tasks.md directly and delegate the
appropriate task from tasks.md (or include a fallback sentence like "If no
current task found, read $SPEC_PATH/tasks.md and delegate the corresponding
task"). Ensure the change is applied to the prompt construction near the
variables TASK_BLOCK, TASK_INDEX and the "## Current Task" / step 2 text so the
coordinator is instructed to fall back to tasks.md when TASK_BLOCK is empty.
In `@specs/.index/index.md`:
- Around line 21-24: Fix the canonical phase values in index-state.json so the
downstream renderer recognizes them: open index-state.json and locate the
objects with id "fix-impl-context-bloat" and "return-ralph-wrigum" and normalize
their "phase" fields to the expected canonical strings (e.g., ensure
"fix-impl-context-bloat" uses the exact canonical "research" value and change
"return-ralph-wrigum" from "complete" to "completed" if that is the canonical
form), save and re-run the renderer to confirm the status column in the table
now renders correctly.
Comment on lines
+214
to
+223
| TASK_BLOCK=$(awk -v idx="$TASK_INDEX" ' | ||
| /^- \[[ x]\]/ { | ||
| if (count == idx) { found=1; print; next } | ||
| if (found) { exit } | ||
| count++ | ||
| } | ||
| found && /^ / { print; next } | ||
| found && /^$/ { print; next } | ||
| found && !/^ / && !/^$/ { exit } | ||
| ' "$TASKS_FILE" | sed -e :a -e '/^[[:space:]]*$/{' -e '$d' -e N -e ba -e '}') |
There was a problem hiding this comment.
TASK_BLOCK captures all remaining tasks, not just the one at TASK_INDEX — logic-ordering bug in awk.
Once the target task is found, count is never incremented (the next statement in the first if branch skips count++). Every subsequent task-header line therefore still satisfies count == idx, so if (found) { exit } — which would stop extraction — is never reached. The result is that TASK_BLOCK contains every task from TASK_INDEX to EOF, producing the same (or worse) context bloat this PR is trying to eliminate.
Concrete trace for idx=1 with a spec having 41 tasks:
- Header for task 0:
count(0) ≠ idx(1)→count++→count=1 - Header for task 1:
count(1) == idx(1)→found=1, print,next—countstays at 1 - Header for task 2:
count(1) == idx(1)→ TRUE again, printed → BUG - … repeats through task 40
The three content-only verification commands in tasks 3.5 and the Phase 4 bash -n check do not catch this because they only test syntax and pattern presence, not awk runtime behavior.
Fix: promote if (found) { exit } to the first check inside the /^- \[[ x]\]/ action block:
🐛 Proposed fix
TASK_BLOCK=$(awk -v idx="$TASK_INDEX" '
/^- \[[ x]\]/ {
+ if (found) { exit }
if (count == idx) { found=1; print; next }
- if (found) { exit }
count++
}
found && /^ / { print; next }
found && /^$/ { print; next }
found && !/^ / && !/^$/ { exit }
' "$TASKS_FILE" | sed -e :a -e '/^[[:space:]]*$/{' -e '$d' -e N -e ba -e '}')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/ralph-specum/hooks/scripts/stop-watcher.sh` around lines 214 - 223,
The awk script that builds TASK_BLOCK mis-handles task headers because once the
target header is found the `next` prevents `count++` so subsequent headers still
match; update the awk logic used with TASK_INDEX/TASKS_FILE so header lines
first check `if (found) { exit }` before checking `if (count == idx) { found=1;
print; next }` and keep the `count++` for non-target headers—i.e., in the /^-
\[[ x]\]/ action block, move the `if (found) { exit }` to be the first check so
extraction stops after the matched task.
Comment on lines
+177
to
181
| The coordinator enforces 3 verification layers: | ||
| 1. Contradiction detection - rejects "requires manual... TASK_COMPLETE" | ||
| 2. Uncommitted files check - rejects if spec files not committed | ||
| 3. Checkmark verification - validates task is marked [x] | ||
| 4. Signal verification - requires TASK_COMPLETE | ||
| 2. Signal verification - requires TASK_COMPLETE | ||
| 3. Periodic artifact review - validates implementation against spec | ||
|
|
There was a problem hiding this comment.
Summary line 179 omits ALL_TASKS_COMPLETE — inconsistent with Layer 2 body.
Line 26 and line 175 both explicitly accept ALL_TASKS_COMPLETE as equivalent to TASK_COMPLETE, but the compact summary at line 179 says only "requires TASK_COMPLETE". A reader relying on the summary alone would not know ALL_TASKS_COMPLETE is also valid.
✏️ Proposed fix
-2. Explicit TASK_COMPLETE signal present
+2. Explicit TASK_COMPLETE (or ALL_TASKS_COMPLETE) signal present🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/ralph-specum/references/verification-layers.md` around lines 177 -
181, The compact summary for the coordinator's verification layers omits
ALL_TASKS_COMPLETE; update the summary text for "Signal verification" (Layer 2)
to reflect that it accepts both TASK_COMPLETE and ALL_TASKS_COMPLETE by changing
the line to something like "2. Signal verification - requires TASK_COMPLETE (or
ALL_TASKS_COMPLETE)" so it matches the explicit acceptance in the Layer 2 body
and the mentions elsewhere.
Comment on lines
+33
to
+56
| { | ||
| "name": "fix-impl-context-bloat", | ||
| "path": "./specs/fix-impl-context-bloat", | ||
| "phase": "research" | ||
| }, | ||
| { | ||
| "name": "plan-source-feature", | ||
| "path": "./specs/plan-source-feature", | ||
| "phase": "completed", | ||
| "taskIndex": 12, | ||
| "totalTasks": 12 | ||
| }, | ||
| { | ||
| "name": "return-ralph-wrigum", | ||
| "path": "./specs/return-ralph-wrigum", | ||
| "phase": "complete", | ||
| "taskIndex": 19, | ||
| "totalTasks": 19 | ||
| }, | ||
| { | ||
| "name": "refactor-plugins", | ||
| "path": "./specs/refactor-plugins", | ||
| "phase": "new" | ||
| }, |
There was a problem hiding this comment.
Two phase-value inconsistencies in the new entries.
-
return-ralph-wrigumuses"complete"(line 48) while every other finished spec uses"completed". Any tooling or rendering logic that matches the exact string"completed"(e.g., the status column inindex.md) will skip this entry — it already shows blank status inindex.mdline 23 as a result. -
fix-impl-context-bloatis still listed as"phase": "research"with notaskIndex/totalTasks, even though all 41 tasks are checked off intasks.mdand.progress.mdshows task 4.2 ("Create PR and verify CI") complete. The coordinator should have invokedupdate-spec-index.shonALL_TASKS_COMPLETE; if that was skipped, the index needs a manual correction.
✏️ Proposed fixes
- "phase": "complete",
+ "phase": "completed",
"taskIndex": 19,
"totalTasks": 19 {
"name": "fix-impl-context-bloat",
"path": "./specs/fix-impl-context-bloat",
- "phase": "research"
+ "phase": "completed",
+ "taskIndex": 41,
+ "totalTasks": 41
},These corrections must also be mirrored in specs/.index/index.md.
📝 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.
Suggested change
| { | |
| "name": "fix-impl-context-bloat", | |
| "path": "./specs/fix-impl-context-bloat", | |
| "phase": "research" | |
| }, | |
| { | |
| "name": "plan-source-feature", | |
| "path": "./specs/plan-source-feature", | |
| "phase": "completed", | |
| "taskIndex": 12, | |
| "totalTasks": 12 | |
| }, | |
| { | |
| "name": "return-ralph-wrigum", | |
| "path": "./specs/return-ralph-wrigum", | |
| "phase": "complete", | |
| "taskIndex": 19, | |
| "totalTasks": 19 | |
| }, | |
| { | |
| "name": "refactor-plugins", | |
| "path": "./specs/refactor-plugins", | |
| "phase": "new" | |
| }, | |
| { | |
| "name": "fix-impl-context-bloat", | |
| "path": "./specs/fix-impl-context-bloat", | |
| "phase": "completed", | |
| "taskIndex": 41, | |
| "totalTasks": 41 | |
| }, | |
| { | |
| "name": "plan-source-feature", | |
| "path": "./specs/plan-source-feature", | |
| "phase": "completed", | |
| "taskIndex": 12, | |
| "totalTasks": 12 | |
| }, | |
| { | |
| "name": "return-ralph-wrigum", | |
| "path": "./specs/return-ralph-wrigum", | |
| "phase": "completed", | |
| "taskIndex": 19, | |
| "totalTasks": 19 | |
| }, | |
| { | |
| "name": "refactor-plugins", | |
| "path": "./specs/refactor-plugins", | |
| "phase": "new" | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/.index/index-state.json` around lines 33 - 56, The index contains two
inconsistent entries: the spec named "return-ralph-wrigum" uses "phase":
"complete" but the canonical status is "completed" — change its "phase" value to
"completed" to match tooling expectations; the spec "fix-impl-context-bloat" is
still marked "phase": "research" and lacks taskIndex/totalTasks despite all
tasks being done — update its "phase" to "completed" and set "taskIndex": 41 and
"totalTasks": 41 (or whatever the repo’s ALL_TASKS_COMPLETE convention
requires). After editing the JSON keys for the entries "return-ralph-wrigum" and
"fix-impl-context-bloat", mirror those same changes in specs/.index/index.md so
the rendered status column and progress reflect the fixes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test Plan
bash -n stop-watcher.sh)Summary by CodeRabbit
Version Updates
New Features
Documentation