fix: 3 bugs in incoherence detector script#21
Open
axatbhardwaj wants to merge 2 commits intosolatis:mainfrom
Open
fix: 3 bugs in incoherence detector script#21axatbhardwaj wants to merge 2 commits intosolatis:mainfrom
axatbhardwaj wants to merge 2 commits intosolatis:mainfrom
Conversation
- Add missing --thoughts CLI arg (documented but not in argparse) - Fix undefined `total` variable, use WORKFLOW.total_steps - Fix false WORKFLOW COMPLETE trigger via exact match
Review Summary by QodoFix three bugs in incoherence detector script
WalkthroughsDescription• Fix undefined total variable by using WORKFLOW.total_steps • Fix false "WORKFLOW COMPLETE" trigger with exact sentinel match • Add missing --thoughts CLI argument for context passing Diagramflowchart LR
A["Incoherence Script Issues"] --> B["Variable Reference Bug"]
A --> C["Workflow Completion Logic Bug"]
A --> D["Missing CLI Argument"]
B --> B1["Use WORKFLOW.total_steps"]
C --> C1["Exact match WORKFLOW COMPLETE."]
D --> D1["Add --thoughts argument"]
File Changes1. skills/scripts/skills/incoherence/incoherence.py
|
Code Review by Qodo
✅ 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
185
to
189
| next_text = guidance.get("next", "") | ||
| if step >= total or "COMPLETE" in next_text.upper(): | ||
| if step >= WORKFLOW.total_steps or next_text.strip().upper() == "WORKFLOW COMPLETE.": | ||
| parts.append("WORKFLOW COMPLETE - Present report to user.") | ||
| else: | ||
| next_cmd = f'python3 -m skills.incoherence.incoherence --step-number {step + 1}' |
There was a problem hiding this comment.
1. Sub-agent steps chain forward 🐞 Bug ✓ Correctness
With the new exact-sentinel completion check, steps whose next says “Sub-agent task complete.” no longer terminate and will emit an <invoke_after> to the next numeric step. Because step 1 mandates invoking <invoke_after>, sub-agent runs will incorrectly continue into parent steps (e.g., 7→8, 11→12, 19→20) with wrong/partial context.
Agent Prompt
### Issue description
Terminal SUB-AGENT steps (7/11/19) currently emit an `<invoke_after>` command due to the new exact-match completion check, causing sub-agents to incorrectly chain into parent steps.
### Issue Context
Several step definitions use `next: "... Sub-agent task complete."` to indicate the sub-agent should stop and return results; step 1 mandates invoking `<invoke_after>` when present.
### Fix Focus Areas
- skills/scripts/skills/incoherence/incoherence.py[162-191]
- skills/scripts/skills/incoherence/incoherence.py[279-292]
- skills/scripts/skills/incoherence/incoherence.py[339-351]
- skills/scripts/skills/incoherence/incoherence.py[478-488]
### Notes
Implement a separate check, e.g.:
- `is_workflow_complete = step >= WORKFLOW.total_steps or next_text.strip().upper() == "WORKFLOW COMPLETE."`
- `is_subagent_complete = "SUB-AGENT TASK COMPLETE" in next_text.upper()` (or a `TERMINAL_SUBAGENT_STEPS={7,11,19}`)
Then:
- if `is_workflow_complete`: print workflow complete message
- elif `is_subagent_complete`: print a sub-agent completion message and DO NOT render `InvokeAfterNode`
- else: render `InvokeAfterNode`
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
648
to
654
| parser = argparse.ArgumentParser(description="Incoherence Detector") | ||
| parser.add_argument("--step-number", type=int, required=True) | ||
| parser.add_argument("--thoughts", type=str, default="", | ||
| help="Accumulated context from previous steps") | ||
| args = parser.parse_args() | ||
|
|
||
| guidance = get_step_guidance(args.step_number, WORKFLOW.total_steps) |
There was a problem hiding this comment.
2. --thoughts not propagated 🐞 Bug ✓ Correctness
The PR adds --thoughts to argparse but the script never uses it and the generated next-step command omits it. Many steps and the skill docs rely on --thoughts to carry state (dimension, findings, resolutions), so following <invoke_after> will drop required context between steps.
Agent Prompt
### Issue description
`--thoughts` is parsed but never used or forwarded in the auto-generated next-step command, so workflow state is lost when following `<invoke_after>`.
### Issue Context
Many step action strings explicitly rely on `--thoughts` (“Your dimension is in --thoughts”, “invoke step N with … in --thoughts”), and SKILL.md documents it as required.
### Fix Focus Areas
- skills/scripts/skills/incoherence/incoherence.py[162-191]
- skills/scripts/skills/incoherence/incoherence.py[641-681]
- skills/incoherence/SKILL.md[13-19]
### Notes
Recommended approach:
1. Update `next_cmd` to include a thoughts placeholder, e.g.
`python3 -m skills.incoherence.incoherence --step-number {step+1} --thoughts "<accumulated>"`
2. Optionally print the received `args.thoughts` (e.g., in a `<context>` block) so the agent can see what state it has.
3. Consider whether `--thoughts` should be `required=True` (or update SKILL.md to reflect optional usage), to avoid silently losing state.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
- Sub-agent terminal steps (7/11/19) no longer emit <invoke_after>, preventing incorrect chaining into parent steps - --thoughts is displayed as <context> and included in next-step command so workflow state is preserved between steps
gcharang
added a commit
to playactai/claude-config
that referenced
this pull request
Apr 16, 2026
PR solatis#21 (incoherence.py): add --thoughts CLI arg with context propagation via <context> block and next-cmd placeholder, replace substring "COMPLETE" check with exact WORKFLOW COMPLETE. sentinel, and add SUB-AGENT TASK COMPLETE branch so terminal sub-agent steps (7/11/19) return to parent instead of chaining forward. PR solatis#18 (arxiv_to_md): replace hardcoded /Users/lmergen path in sub_agent.py with SCRIPTS_DIR computed from __file__, injected via repr() to survive backslashes and embedded quotes (Qodo review). Add package-level CLAUDE.md and README.md documenting the path resolution approach and the f-string vs {result} brace-collision trap. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
--thoughtsCLI arg: The skill docs and step guidance reference--thoughtsfor passing context between steps, but the argparse only defined--step-number. Added it as an optional string argument.totalvariable:format_incoherence_output()referenced baretotalwhich was never defined, causing aNameError. Changed toWORKFLOW.total_steps."COMPLETE" in next_text.upper()matched any step whose "next" text contained the word "complete" (e.g., step 3: "After all agents complete"). Changed to exact sentinel matchnext_text.strip().upper() == "WORKFLOW COMPLETE.".Test plan
<invoke_after>for step 4)--thoughts "test context"is accepted without error