Repository navigation
test(code): exercise the real push-retry instead of mirrored logic - #1834
Conversation
The decide_push_retry helpers in post-code-test and post-fix-test asserted against a reimplemented copy of the §7b / §5 grep. They stayed green if production drifted. Drive the real post-scripts instead, with a git wrapper that feeds each failure wording and lets --force-with-lease run for real. Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
PR Summary by QodoTest real push-retry behavior in post-code and post-fix
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
|
🤖 Finished Review · ✅ Success · Started 8:51 AM UTC · Completed 9:05 AM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $1.82 |
Code Review by Qodo
1. Push tests disable shellcheck checks
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
<!-- fullsend:risk-assessment --> DetailsModerate risk driven by changes to two protected test scripts with elevated Git churn, multi-author contention, and regression history, offset by test-only scope with no dependency or security-sensitive changes. Previous run<!-- fullsend:risk-assessment --> DetailsModerate risk driven by changes to two protected test scripts with elevated Git churn, multi-author contention, and regression history, offset by test-only scope with no dependency or security-sensitive changes. Previous run (2)<!-- fullsend:risk-assessment --> DetailsModerate risk driven by changes to two protected test scripts with elevated Git churn, multi-author contention, and regression history, offset by test-only scope with no dependency or security-sensitive changes. |
ReviewFindingsHigh
Low
Next steps:
Previous run<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJhY3Rpb24iOiJyZXF1ZXN0LWNoYW5nZXMiLCJmaW5kaW5ncyI6W3sic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6Im1pc3NpbmctYXV0aG9yaXphdGlvbiIsImZpbGUiOiJzY3JpcHRzL3Bvc3QtY29kZS10ZXN0LnNoIiwiaWQiOiJmXzVlNjU2MThiMDFlNWRjZTQiLCJhY3Rpb25hYmxlIjpmYWxzZX1dLCJkaXNwb3NpdGlvbnMiOlt7ImlkIjoiZl81ZTY1NjE4YjAxZTVkY2U0Iiwic3RhdHVzIjoib3BlbiJ9XX0= --> ReviewFindingsHigh
Low
Next steps:
Previous run (2)<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJhY3Rpb24iOiJyZXF1ZXN0LWNoYW5nZXMiLCJmaW5kaW5ncyI6W3sic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6Im1pc3NpbmctYXV0aG9yaXphdGlvbiIsImZpbGUiOiJzY3JpcHRzL3Bvc3QtY29kZS10ZXN0LnNoIiwiaWQiOiJmXzVlNjU2MThiMDFlNWRjZTQiLCJhY3Rpb25hYmxlIjpmYWxzZX1dfQ== --> ReviewFindingsHigh
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/fs-fix Address only Qodo finding 2 (“push tests pass without a push”). In both |
|
🤖 Finished Fix · ✅ Success · Started 9:14 AM UTC · Completed 9:19 AM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $0.27 |
In run_code_push_retry_test and run_fix_push_retry_test, fail unless the push log contains a plain git push (no --force-with-lease), and for retry cases require it to precede the --force-with-lease push, so the tests cannot pass without a push. Note: pre-commit could not fetch hook repos (TLS failure in sandbox) and shellcheck is unavailable. Pre-existing unrelated failures in both test scripts (harness signed-commits / conflict-strategy checks, signed-commits-off-plain-push) are unchanged by this commit. Addresses #1834 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
🔧 Fix agent — iteration 1 (human-triggered)Added plain-push presence (and push-before-retry ordering) assertions to both push-retry test helpers, per the human instruction. Other findings were out of scope of the instruction and not addressed. Locally, pre-commit could not fetch hooks (TLS) and shellcheck is unavailable; the test scripts show the same 3 pre-existing, unrelated failures each before and after the change (push-retry cases all pass); CI script-test is green. Fixed (3):
Tests: passed CI inspections (3):
Conflict update: skipped; strategy
|
|
🤖 Finished Review · ✅ Success · Started 9:21 AM UTC · Completed 9:26 AM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $1.35 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Code review by qodo was updated up to the latest commit 3a9abca |
|
🤖 Finished Review · ✅ Success · Started 9:40 AM UTC · Completed 9:47 AM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $1.40 |
fullsend-ai-review
Bot
removed
the
requires-manual-review
|
It makes sense to me, I'm assuming the code is good and all. Agreeing with the intent and direction. |
Problem
scripts/post-code-test.shandscripts/post-fix-test.sheach had adecide_push_retryhelper that reimplemented the production non-fast-forward grep (post-code.src.sh§7b /post-fix.src.sh§5). Those five cases asserted against the copy, not against the post-script. Change the grep in production and the suite stayed green.github-signed-push-test.shcoversforge_push_branch; it does not cover whether the post-script decides to retry with--force-with-lease.Change
Replace both mirrored helpers with integration cases that run the real post-script (
GITHUB_SIGNED_COMMITS=falseso push is plaingit push). A git wrapper feeds each first-push failure wording; the retry path is a realgit push --force-with-lease.Same five case names as before, in both files:
push-successpush-non-fast-forwardpush-rejectedpush-api-not-a-fast-forwardpush-unexpected-errorPre-commit mirroring is left alone:
validate-code-output-test.shalready drivesprecommit_run_gate.Why this shape
No production code changes. The value is that the tests can no longer silently diverge from the §7b / §5 decision.
To show the conversion is not vacuous: temporarily broke the production grep pattern in each
.src.sh, reran the suite, and confirmed the three retry-wording cases went red (exit 1, want 0) whilepush-success/push-unexpected-errorstayed green. Restored before commit. The old helpers would have stayed green through that breakage.Tests
(
make script-buildnot needed — tests only.)