Conversation
derekmisler
changed the title
|
/describe |
|
✅ PR description has been generated and updated! |
derekmisler
force-pushed
the
add-fallback-models-retries-and-timeouts
branch
3 times, most recently
from
2d66210 to
bf8b8fc
Compare
![docker-agent[bot]](https://avatars.githubusercontent.com/in/2787757?s=60&v=4){kind=small}
There was a problem hiding this comment.
Critical Issues Found
This PR adds important retry and timeout functionality, but contains critical bugs in exit code handling that will prevent the retry logic from working correctly.
Summary
- 2 HIGH severity issues: Exit codes are captured incorrectly in both timeout and non-timeout cases, causing all failures to be reported as success
- 2 MEDIUM severity issues: Timestamp comparisons could miss reviews posted in the same second
The retry logic implementation is solid, but the exit code capture bugs will completely break its functionality.
derekmisler
force-pushed
the
add-fallback-models-retries-and-timeouts
branch
from
bf8b8fc to
b27edd7
Compare
|
/review |
There was a problem hiding this comment.
Review Summary
This PR successfully adds retry logic with exponential backoff and review verification. However, I've identified 3 medium-severity issues in the error handling and verification logic that should be addressed:
- Clock skew vulnerability in review verification - The timestamp comparison could miss legitimate reviews if clocks are not perfectly synchronized
- API error masking - API failures are silently treated as "no reviews found", making it impossible to distinguish between legitimate zero-review results and API errors
- Silent fallback failures - If the fallback comment posting fails, users receive no notification
These issues don't prevent the core retry functionality from working, but they could cause confusion when verification or fallback mechanisms fail.
Signed-off-by: Derek Misler <derek.misler@docker.com>
derekmisler
force-pushed
the
add-fallback-models-retries-and-timeouts
branch
from
b27edd7 to
3224dd6
Compare
|
/review |
|
/describe |
|
✅ PR description has been generated and updated! |
There was a problem hiding this comment.
Review Summary
I've identified one issue in the newly added review verification logic that should be addressed to improve robustness. The retry logic and timeout handling look good overall.
The main concern is that the timestamp generation lacks validation, which could lead to incorrect verification results if both date command variants fail (though this is extremely unlikely on standard runners).
| fi | ||
|
|
||
| - name: Save reviewer memory | ||
| if: always() |
There was a problem hiding this comment.
is always necessary for some failure cases or and i missing something? feels odd, but then again this is GHA after all so...
There was a problem hiding this comment.
yeah, if the previous step failed, GHA would fail the whole thing and we would not save memory. but even if the run fails, i still want to save whatever memory accumulated during the run before it failed.
| if [ "$REVIEW_VERIFIED" == "false" ]; then | ||
| # Agent succeeded but review wasn't posted (likely token expiry) | ||
| gh api "repos/${{ github.repository }}/issues/comments/${{ steps.resolve-context.outputs.comment-id }}/reactions" \ | ||
| -X POST -f content='confused' || true |
2 participants
Summary
Adds retry logic with exponential backoff and timeout handling to improve reliability of agent execution. Updates cagent version to v1.23.6 across all workflows and documentation. Enhances PR review workflow with verification that reviews are actually posted and fallback error handling for token expiry scenarios.
Changes
max-retriesandretry-delayinputs, improved timeout handling usingPIPESTATUSto distinguish timeout (124) from other failuresgithub.token(6h lifetime) instead of potentially expired App tokenretry-delaydescription to clarify exponential backoff behaviorBreaking Changes
None
How to Test
Closes: https://github.com/docker/gordon/issues/171