Repository navigation
fix(reports): retry pushes with existing report content - #1291
Conversation
|
Closing this duplicate in favor of #1280. #1280 already includes the #1277 report-retry fix in its branch: it changes the shared reports writer so an empty Thank you for the issue and implementation context. |
|
Correction to my close note: #1280 only partially overlaps #1277, so this is not a duplicate. #1280's own description marks #1277 as related but outside its scope. Its diff changes the shared push mechanics and Reopening #1291 to keep the full #1277 fix available. Sorry for the incorrect duplicate assessment. |
|
No findings in the specified PR diff. The description documents sufficient testing, including a build followed by a representative real-CLI E2E test for retrying a rejected session-summary push. I did not run, build, or install anything from the PR. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@huong8373tt-beep Thanks, this does what #1277 asks. We built b87d8a9e and origin/main (0d57ac46) and checked it against the issue.
| #1277 | Result |
|---|---|
| A retry whose content is already committed pushes the earlier commit | Reproduced with session save --push against a local origin that rejects the first push. On main the retry prints Session already present in the team log — nothing to push. and origin never gets the commit. On this PR it prints Pushed a previously unpushed session summary. and origin has sessions/alice/2026-10.md. |
bootstrap.ts, both init.ts sites, both votes sites in hook-handlers.ts, and the team-push.ts auto-report return { files: [] } |
Checked by reading the code. A local fixture can't re-run init or bootstrap into the retry. |
Read-only callbacks keep null and never push |
Every updateReports caller checked; reports-branch-readonly.test.ts passes |
We also checked that nothing local is dropped when the retry fails again. Usage events, votes deltas and the member record are cleared only after origin confirms the push. A pull whose reports push was rejected kept its 3 usage events and cleared them only after the next run delivered.
tsc, lint and git diff --check pass, the related unit tests pass, and your e2e passes on this head.
Non-blocking
[P2 non-blocking] src/save-session.ts:163-164: when the retry is rejected again, session save --push still prints Session already present in the team log — nothing to push. and exits 0. isPublished is false for failed and busy as well as for already-present. Main prints the same line, but there it was true, because no push was attempted. A later teamai pull does deliver the commit. A small fix would be:
const result = await withTimeout(updateReports(/* unchanged */), 10_000, 'Push timeout (10s)');
const pushed = isPublished(result);
// in the `ran && !rel` branch:
if (pushed) spin.succeed('Pushed a previously unpushed session summary.');
else if (result.status === 'failed') spin.warn('Push failed. The summary is saved locally; retry with: teamai session save --push');
else spin.info('Session already present in the team log — nothing to push.');[P2 non-blocking] No unit test asserts the new { files: [] } returns in bootstrap, init, votes sync and judge, or the auto-report. Only the shared path and session save are covered. A test for one of them, ideally the votes sync, would keep these from going back to null.
[P3 nit] The body lists the test commands but not what passed. Adding the summary lines would make it a record.
Note: #1280 changes the same updateImpl branch in src/utils/branch-worktree.ts, where the two versions behave the same, and the same saveSession branch, where it uses different messages. Whichever lands second has to reconcile save-session.ts and the assertion at src/__tests__/e2e/reports-writer-sync-561.test.ts:237.
|
No findings in the specified PR diff. The PR description documents sufficient testing: a build followed by a representative real-CLI E2E test covering a session-summary retry after a rejected push. No additional provider × agent coverage is required. The earlier review reported no findings, so there are none to mark resolved. I did not run, build, or install anything from the PR. |
|
Thanks for the approval and the specific follow-up. I addressed the first non-blocking item in
Validated on this head: I am leaving the suggested per-writer fixtures as follow-up rather than add speculative fixtures in this focused patch; the shared writer behavior and the reviewed call-site conversions remain covered by the existing regression. |
Keep this branch's reports publish contract over Tencent#1291's: a write that returns null pushes nothing, an empty list retries the unpushed commit, and a refused session push fails the command. Tencent#1291's session retry test now expects that contract (exit 1 while origin refuses, then Pushed: <file>).
Summary
fileslist when a retry finds content already committed locallyalready-presentsession save --pushretry after a rejected pushFixes #1277
Validation
npx vitest run src/__tests__/git-kind-reports.test.ts -t "pushes an earlier unpushed report commit when the retry has no new files"npm run build; npx vitest run --config vitest.e2e.config.ts src/__tests__/e2e/reports-writer-sync-561.test.ts -t "pushes an earlier rejected session summary when retrying an existing session"npm run typechecknpm run lintgit diff --checkSlai Agent Runtime assisted with investigation, implementation, and testing. The account owner reviewed and authorized this contribution.