Repository navigation
fix(recall): preserve confirmed votes in narrow clones - #1282
Conversation
|
No confident findings in the specified PR diff. The PR description satisfies the testing requirement: it records final-head build, type-check, lint, and 38 real-CLI tests, including narrow-clone regressions. Deferring other provider/agent environments to CI is acceptable. Review was read-only; 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.
@MertensMing Approving with notes. We built this head and main, gave feedback, delivered it with pull, then rebuilt the index without a fetch, and checked the recall verdict.
| Clone | main |
this head |
|---|---|---|
| full | RELEVANT score=8.6 |
RELEVANT score=8.6 |
--single-branch |
NOT_RELEVANT score=2.5 |
RELEVANT score=8.6 |
--depth |
NOT_RELEVANT score=2.5 |
RELEVANT score=8.6 |
A push the remote rejects does not move the tracking ref. The new narrow-clone e2e cases fail on main and pass here. Reading the ref and the local deltas in one lock hold, then reading blobs from that pinned commit, neither double-counts nor misses a delta.
[P2 non-blocking] src/utils/branch-worktree.ts:654-655 swallows every update-ref error. With a leftover refs/remotes/origin/teamai-reports.lock, the push succeeds, update-ref exits 128 with cannot lock ref, nothing is printed, and a rebuild without fetch drops the vote again: 1.3 before, 0.3 after. main loses it too, so this is not a regression. Also, whenever the clone's fetch refspec covers the reports branch (full clones, and a single-branch clone of that branch), git push already moves the ref, so the compare-and-swap always fails there and the comment's "another updater" reason is not what happened. Re-reading the ref after the push, calling update-ref only when it differs from sent, and logging a warning on failure would cover both.
[P3 nit] indexableVotesDir now runs inside aggregateVotes, which buildIndex wraps. An unexpected checkOwner failure therefore no longer aborts the rebuild: it continues without votes and logs a warning. That fits how vote reads already degrade, and it's worth one line in the description.
[P3 nit] src/viz.ts:344 is a no-op change, because aggregateVotes ignores votesDir when voteScope is set. src/votes.ts:584 re-parses content that was already validated and casts it with as UserVotes.
#1280 also edits src/utils/branch-worktree.ts in adjacent hunks. A trial merge of the two heads is clean and typechecks. This PR owns the tracking-ref write in the push loop, readIndexVoteFiles and the votesDir removals.
018ac61 to
67fef9d
Compare
|
No confident findings in the specified PR diff. The earlier review raised no findings, so there are none to mark resolved. The PR description satisfies the testing requirement with 38 reported real-CLI tests, including narrow-clone regressions. The record names Review was read-only and limited to the specified diff. 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.
@MertensMing CI is red on 67fef9dd. Every Lint & Test job and the Windows job fail on windows-test-list.test.ts and codex-team-hooks.test.ts. Those failures came from main, not from this PR, and #1287 fixed them in c15fc0db. The Node 24 macOS job also failed a timing check in env-loader.test.ts (expected 104.33 to be less than 100), which this PR does not touch.
Please rebase or merge current main so CI runs again. The code review from the earlier approval still holds, and we'll approve again once CI is green. Note that #1285 adds a test at the end of the same describe block in search-index-local-votes.test.ts. Whichever PR lands second keeps both tests.
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@MertensMing Approving again so this can land. The red CI on 67fef9dd came from main's Windows test list, which #1287 fixed. We merged this branch onto current main (c15fc0db) locally: tsc and lint pass, and so do the vote, confidence and Windows-list tests. No rebase is needed on your side.
Summary
Follow-up to the review of #1276 (already merged), for #1233.
A successful reports push in a single-branch or shallow clone can leave origin/teamai-reports stale. After local deltas are cleared, contribute and recall index rebuilds then miss the delivered votes. Record the accepted branch commit in the tracking ref after a successful push, using compare-and-swap to preserve concurrent updates. The index still reads a confirmed commit, never an unpushed worktree HEAD.
Also address the remaining review notes:
Type of Change
Test Plan
Final head 018ac61 on main e45baeb, Node 20.19.5:
npx vitest run --config vitest.e2e.config.ts src/__tests__/e2e/index-local-votes-1233.test.ts src/__tests__/e2e/votes-sync-retry-1027.test.ts src/__tests__/e2e/self-mode-worktrees-808.test.ts src/__tests__/e2e/reports-writer-sync-561.test.ts --retry=0The real-CLI regression uploads feedback to an isolated local Git origin, then rebuilds through contribute and through recall with a missing index. Full, single-branch and depth-1 clones now all retain 1.3 votes; before this follow-up the latter two fell to 0.3 until pull fetched again. Original rejected-push and pushed-but-unconfirmed cases still pass. The unconfirmed case now runs its hook beneath a type:module package. The hook was also checked independently in both module modes.
Unit regressions cover unchanged legacy scores, corrupt filenames with and without local deltas, and a writer adding feedback during blob reads: it takes the released lock successfully, the current index retains its coherent snapshot, and the next rebuild includes the new feedback.
Before rebasing this follow-up onto e45baeb, the full unit suite completed with 11,490 passed, 25 skipped, and one failure: project-hook-rules.test.ts, ZCode registered-entry check. That failure was independently reproduced from an unmodified archive of upstream e45baeb on this machine. The full suite was not repeated after the rebase; the final-head checks above were. Other provider/agent environments remain for CI.
Related Issues
Follow-up to #1233 and the #1276 review.
Notes for Reviewers
Tracking-ref maintenance is in the shared side-branch push helper so it records a confirmed delivery before the existing onDelivered callbacks clear vote deltas. Failed pushes do not update the ref. Retry and delivery-ID accounting remain unchanged. No added network fetch or changes to the upload protocol.