still getting stuck in infinite loops sometimes#231
Conversation
derekmisler
changed the title
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🔴 CRITICAL
The new "Post /review deprecation notice" step has two bugs that prevent it from ever posting successfully. Both reference step IDs that do not exist in this workflow job.
b59e389 to
28ec0d1
Compare
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The changes are well-structured and address the infinite loop problem effectively. Adding max_iterations caps (drafter:40, verifier:20) and increasing timeouts (agent 1800→2700s, job 35→50 min) are straightforward and correct. The deprecation of /review in favour of sidebar review-request is a clean UX improvement. Two minor messaging issues were noted as inline comments.
Signed-off-by: Derek Misler <[email protected]>
28ec0d1 to
4137f2b
Compare
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
This PR correctly addresses infinite loops with max_iterations limits, increases timeouts, and deprecates the /review command. One finding warrants verification before merge.
Summary of changes reviewed:
- ✅ Timeout increase (1800s → 2700s) and workflow limit (35 → 50 min) — straightforward and well-commented
- ✅ Deprecation notice step — correctly fires on
issue_commentwithis_review == 'true'; bot-loop guards prevent the posted notice from re-triggering the workflow - ✅
$TIMEOUT_BODYshell quoting — double-quoting correctly prevents word splitting; backticks in hardcoded strings are safe ⚠️ max_iterationsschema validity — see inline comment
|
|
||
| drafter: | ||
| model: sonnet | ||
| max_iterations: 40 |
There was a problem hiding this comment.
[MEDIUM] max_iterations field validity in cagent YAML schema v6 is unconfirmed — if unrecognized, infinite loop prevention is silently ineffective
The file history for review-pr/agents/pr-review.yaml shows a suspicious pattern:
- Commit a588b69 added
max_iterationsto the drafter - Commit 724b3f7 ("fix(review-pr): schema mismatch, chunk size, and timeout diagnostics") followed shortly after
- This PR re-adds
max_iterationsto both drafter and verifier
If commit 724b3f7 removed max_iterations specifically because it caused a schema validation error in cagent v6, then re-adding it here would either silently fail (field ignored → infinite loop prevention is a no-op) or cause agents to fail to start. Since the entire purpose of this PR is to fix infinite loops via max_iterations, silently ignored limits would leave the root problem unresolved.
Suggested verification: Check whether max_iterations appears in the cagent v6 agent schema documentation, or review the diff of commit 724b3f7 to confirm what the schema mismatch actually was (output schema vs. agent config schema). If max_iterations is a valid v6 field, this is fine.
There was a problem hiding this comment.
Thanks for the question! After investigating, I believe there's a conflation between two different things here:
- Hook-based
max_iterations— this was removed in PR fix(review-pr): remove unsupported before_llm_call hooks from drafter and verifier agents #200 (commit d634a35), wheremax_iterationswas set via ahooks.before_llm_callmechanism. - Top-level
max_iterationsconfig field — this is a separate, natively supported field in the agent config schema and was not affected by that removal.
The commit you cited (724b3f7) only renamed output schema field names and did not touch max_iterations.
I verified this in the docker-agent source: max_iterations exists in pkg/config/ types from v6 through the latest (v10). All versions use strict YAML parsing (DisallowUnknownField()), which means any unrecognized field would cause the agent to crash rather than be silently ignored — so this field is definitely valid and actively supported.
Happy to share the specific source references if that would help!
There was a problem hiding this comment.
Thank you for the thorough investigation and clarification! You're absolutely right — I conflated the hook-based iteration control (removed in PR #200) with the native max_iterations config field, which has always been supported in the schema.
Your point about DisallowUnknownField() is particularly reassuring: if max_iterations were invalid, the agents would fail to start rather than silently ignoring the limit. That eliminates the "silent failure" risk I was concerned about. Given that verification, this change should work as intended.
Appreciate you taking the time to trace through the commit history and source code!
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The changes look correct. All three hypothesized bugs were investigated and dismissed after careful analysis:
Investigated: backtick escaping in gh api -f body="..." calls — The \`` sequences in the new deprecation notice and timeout body strings are valid bash. In YAML run: |literal blocks,`passes verbatim to bash as backslash+backtick. Within bash double-quoted strings,`escapes the backtick (per the bash manual: *"The backslash retains its special meaning only when followed by:$, `` ``, ", ``, or newline"*), producing a literal backtick character — no command substitution occurs. The posted comments will render correctly with backtick-wrapped names.
CHUNK_COUNT branching logic — The three-way conditional (-z, = "1", else) correctly handles all cases: missing step output, single-chunk diff, and multi-chunk diff. The env var propagation from steps.chunk-diff.outputs.chunk_count is correct.
max_iterations in agent YAML — Confirmed valid per project history: this is a natively supported top-level field in the agent config schema (strict DisallowUnknownField() parsing means invalid fields crash at startup, so its acceptance proves validity).
Other changes — Timeout increase (1800→2700 s), workflow timeout (35→50 min), and the deprecation notice step ordering (notice posts before review proceeds, || echo ensures continue-on-error behavior) are all correct.
3 participants
Summary
Deprecates the
/reviewcomment command in favor of re-requesting review fromdocker-agentin the PR sidebar. Increases timeout limits to accommodate complex reviews, adds iteration limits to prevent agent loops, and improves timeout error messaging with context about diff size.Changes
/reviewcommand: Posts a deprecation notice when used; users should re-request review fromdocker-agentin the PR sidebar insteadmax_iterations: 40for drafter andmax_iterations: 20for verifier to prevent infinite loops/reviewas deprecated, and explain re-trigger workflowTest plan
/reviewcomment posts deprecation notice and review still completes