Repository navigation
fix(push): stop offering a teammate's update back as a local edit (#823) - #827
Merged
Merged
Conversation
|
No findings. The PR description documents sufficient testing for this runtime change, including representative real-CLI verification. Per instruction, I reviewed only the specified diff and did not run or build the PR code. |
…ncent#823) Three more ways push could list a copy the member never edited as modified, ready to send a teammate's change back as the old version. Single-repo mode (item 2). Push runs against a knowledge worktree whose team root is <wt>/.teamai, a subdirectory of the git repo. The pre-push sync read each base version with `git show <rev>:rules/x.md`, which git resolves from the repo root, so it never found one, and every rule or skill a teammate updated read as a local edit. The three reads now pass `./<path>`, which git resolves from the working directory, as getFileContentWhenAdded and the agent guard already did. Placed agents (item 3). An agent placed with --role/--project is held when it changed on the team since this machine's copy was current, and "current" meant the version at the shared lastPullRev, which a pull in another checkout moves past a copy a stale worktree still holds (the Tencent#812 revert, for agents). The guard now reads this checkout's bases through checkoutBaseRevs, and falls back to the shared lastPullRev for a checkout with no entry, as the pre-push sync does. Push bases record where the sync moved rules and skills, not agents, so the copy stays at the revision pull delivered: the guard holds an agent that differs from its version at any base. Push records the team HEAD as a base before the scan, and the file there is always the current one, so the version the agent was added with is compared too whenever a base predates it; otherwise a placement that landed after the last pull would go back over a teammate's later edit. The hold message now says "this checkout". Skill copy (item 5). The sync overwrote a local skill in place, so a copy that failed partway left files from two revisions, matching no base, and the next push listed the skill as modified. The update is now built in a hidden sibling (the local copy, then the team version over it, so files only the member has survive as before) and renamed into place; a failure leaves the previous version whole. The stage carries the local modes, so cleanup makes a read-only stage writable before removing it, and warns with the path if a leftover cannot be removed; if the previous version cannot be renamed back, the error names where it is. Item 4 (user-scope push base) follows once Tencent#814 is merged.
SaulMoro
force-pushed
the
fix-823-push-sync-followups
branch
from
September 25, 2026 10:12
0302ab4 to
51c4b75
Compare
|
No findings. The PR description documents sufficient testing for this runtime change, including representative real-CLI verification. I reviewed only the specified diff and did not run, build, or install the PR code. |
jeff-r2026
self-requested a review
September 25, 2026 12:15
jeff-r2026
approved these changes
Sep 25, 2026
This was referenced Sep 25, 2026
Merged
This was referenced Sep 27, 2026
jeff-r2026
pushed a commit
that referenced
this pull request
Sep 27, 2026
) (#855) * fix(persistence): write state.json and the search index atomically (#854) saveState, saveStateForScope and buildIndex wrote in place: open + truncate + write. A crash, kill, ENOSPC or power loss mid-write leaves truncated JSON, and the readers answer null for what they cannot parse: - state.json: loadStateForScope falls back to StateSchema.parse({}) — lastPullRev and every per-checkout pushBaseRevs entry are gone without a word, so the next push compares against a stale base. That is the stale-base overwrite class the worktree fixes (#827) closed. - search-index.json: loadIndex returns null, so recall silently loses the whole corpus until the next rebuild, and the shrink guard loses its baseline. The repo already writes config.yaml (#831) and the votes file atomically for exactly this reason, and writeJsonAtomic exists: temp file + rename, preserving the target's permission bits. Move the three writers to it. No reader changes; a successful write behaves as before. * test: inject index-write failures at the write call, not the file mode CI failed recall-rebuild-roots: the 'cannot be written' test chmod'd the index file 0o444, which fails the in-place writer but not the atomic one — writeJsonAtomic stages a temp sibling (the directory is writable) and renames over the target, and rename needs only the directory's permission. Same for the EISDIR test: renaming a file over a directory is EISDIR on POSIX but EPERM on Windows. Fail the fse.writeFile calls at the index path itself — the staged temp file included — so the injection works for both writers on every platform. The root-skip guard goes with the chmod it existed for. * test: force the #812 e2e state-write failure at the write call, not the file mode CI failed the 'cannot be recorded' case: it chmod'd state.json 0o444, which stops the in-place writer but not the atomic one — writeJsonAtomic stages a temp sibling and renames, and rename needs only the directory's permission, so the push completed normally and exited 0. (The e2e config retries once; the retry then failed teammatePublishes' git commit with 'nothing to commit', the run's second error.) The CLI runs as a subprocess, so the write call cannot be spied on the way state-atomic-save does it. Inject the failure from outside instead: a preload hook (NODE_OPTIONS --require) fails every fs.writeFile targeting the staged temp of this state file inside the CLI process — the same seam as the unit tests, on every platform. --------- Co-authored-by: ydflow <ydflow@users.noreply.github.com> Co-authored-by: ydflow <314143294+ydflow@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Push could still list a copy the member never edited as modified, ready to send a teammate's update back as the old version. This fixes three of the paths #823 lists.
Before / After (real CLI,
teamai --dry-run push):origin/mainf558b94).teamai/rules/team-rule.md, noteamai pull[rules] team-rule (modified)[agents] vr (modified)Skipped vr: ... changed on the team since this checkout last synced itType of Change
Causes:
repo.localPath = <knowledge-wt>/.teamai, andgit showresolves paths from the repo root, so no base was ever found.lastPullRevis moved by a pull in another checkout (the [bug] push from a stale worktree diffs against another checkout's revision and can revert a teammate's update #812 revert, for agents). Push bases mark where rules and skills were synced, not agents, so any differing base counts. Push records the team HEAD before the scan, so the added-with version is also compared when a base predates the placement.Test Plan
npx tsc --noEmitpassesnpx vitest runpasses: 316 files, 4964 passed, 1 skippedNew tests, all red on
origin/main(f558b94) or under ablation:Ablations, repeated after the rebase onto 21cb76a: each change was reverted alone and its test failed at the expected assertion. That covers the
./prefix, agents.ts, the added-with fallback, the staged copy and the cleanup chmod. Three assertions inpre-push-sync.test.tspinned the buggy argumentrules/..., and now say./rules/....Build/Result:
npm run buildok. E2E run (--retry 0): the new file, pluspush-stale-worktree-812,push-namespace-e2e,role-scoped-agents,namespaced-entries,project-agent-cold-start,e2e,multi-projectandcopilot-agents: 9 files, 84 passed, 22 skipped (the skipped ones need a remote). The full e2e suite was not run.Real CLI, before (origin/main f558b94) and after (this branch)
Provider
git(local bare remote), agent Claude. Other providers and agents are left to CI.Related Issues
Part of #823 (items 2, 3 and 5; item 4 follows once #814 is merged)
Notes for Reviewers
Reproduced with the real CLI: items 2 and 3 (above).
From reading the code, covered by unit tests only: item 5. The partial-copy failure is forced by a mocked
copyDir, plus a real read-only skill.git merge-treeof this branch with fix(data-layout): let every self-mode worktree publish learnings and keep its queue (#808) #814's head (628f797) reports only the conflicts fix(data-layout): let every self-mode worktree publish learnings and keep its queue (#808) #814 already has withmain(CHANGELOG.md,src/contribute.ts,src/recall.ts), none from this branch.candidates === recorded); the guard now compares them with this checkout's bases like the rest..<skill>.<pid>.<hex>.teamai-prev; a failed restore names that path. A read-only subfolder can leave a leftover, and the warning names it..teamai/rulesis itself a scan source. A branch that has not merged a teammate's update lists its older copy as modified. That is why the item 2 test fast-forwards the branch. It is tracked in [bug] Follow-ups to the worktree fixes: import --from-mr never shares its learning, and push can still revert a teammate's update #823.push.tsand the agent guard (it belongs inpull.ts, which fix(data-layout): let every self-mode worktree publish learnings and keep its queue (#808) #814 changes).