fix(data-layout): let every self-mode worktree publish learnings and keep its queue (#808) - #814
Conversation
638c20c to
e7975ec
Compare
|
e7975ec to
e949a62
Compare
|
Thanks, all three were real. Fixed in the same commit, rebased on a725574:
|
The three earlier findings are resolved in the current diff. The PR description includes sufficient real-CLI/e2e testing evidence. |
e949a62 to
81b3015
Compare
|
Reply to #814 (comment) Thanks. Both addressed in the same commit:
|
81b3015 to
0e5605a
Compare
|
Follow-ups: the items that only exist in this branch (queue after a git↔self switch, uninstall and the unpublished queue, the vote-judge hook's read roots, and the old and per-worktree search indexes) are being fixed in this PR, so they are no longer tracked elsewhere. #823 tracks only what already happens on |
0e5605a to
0c21d43
Compare
|
Update in 0c21d43: the recall hook's vote judge now also leaves a refused checkout out. It reads the checkout's |
The earlier findings are resolved, and the PR description contains sufficient real-CLI/e2e testing evidence. |
0c21d43 to
4b51eb0
Compare
|
Both fixed in 4b51eb0 (still one commit). Stale index after a mode switch. When Votes from a refused reports checkout. Each fix was red before, and reverting each part alone (the index drop, the downvote, the contribute and recall builds) fails the e2e. |
4b51eb0 to
03560fe
Compare
The previously reported findings are resolved, and the PR description includes sufficient real-CLI/e2e testing evidence. |
03560fe to
a7e98ef
Compare
|
Fixed in a7e98ef, still one commit, now on f558b94.
|
The previously reported findings are resolved in the current diff. The PR description includes sufficient real-CLI/e2e testing evidence. |
a7e98ef to
31d8498
Compare
|
Fixed in 31d8498. Covered by e2e 5 in |
The PR description includes sufficient real-CLI/e2e testing evidence. The earlier findings are otherwise addressed, but the legacy-config queue case above remains unresolved. |
31d8498 to
a94bd2d
Compare
|
Thanks. All four are fixed in a94bd2d.
Each fix was reverted on its own, and its test fails every time. |
|
a94bd2d to
462b2f3
Compare
The other previously reported cases appear resolved, and the PR description includes sufficient real-CLI/e2e testing evidence. |
7068e5c to
628f797
Compare
|
Round 12 (628f797):
|
The previously reported checkout ownership and reader-path issues appear resolved. The PR description contains sufficient real-CLI/e2e testing evidence. |
…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.
628f797 to
8f3f0be
Compare
|
Rounds 12 and 13 (5830032660, 5830259599), in 8f3f0be:
|
8f3f0be to
03f28d7
Compare
|
Local adversarial review of 8f3f0be. The Codex job hit its 15-minute cap on this diff, so I ran the workflow's prompt (Code Review Rules, the earlier reviews, the PR body, the diff) with three Claude reviewers, one per area, and checked each finding against the code. 12 findings, no P1. Fixed in 03f28d7. Each fix has a test that fails without it:
Not changed:
Checks, |
03f28d7 to
b85644a
Compare
|
This round adds #823 item 13 to the same commit (b85644a). A queue belonged to its install's kind only, so re-running A queue's owner is now the kind plus Tests: 3 unit tests in |
…) (#827) 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 #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 #814 is merged.
The previously reported findings otherwise appear resolved, and the PR description includes sufficient real-CLI/e2e testing evidence. |
b85644a to
76c62a9
Compare
|
Re 5832326638, fixed in d705979c (rebased onto e79db17):
Each fix reverted on its own fails its tests. |
The previously reported findings appear resolved. The PR description includes sufficient real-CLI/e2e testing evidence. |
76c62a9 to
b8c3d7b
Compare
|
Fixed P1 Test: e2e 19 in |
b8c3d7b to
82764f6
Compare
|
Local adversarial review of b8c3d7b. The Codex job was cancelled again at its 15-minute cap on this diff. I ran the workflow's prompt (Code Review Rules, the earlier reviews, the PR body, the diff) with three Claude reviewers on disjoint slices of the diff, and checked each finding against the code. 8 findings, none raised as P1. Fixed in 82764f6, as a P1 after review:
Not changed (listed in Notes):
Checks, |
|
Local adversarial review of 82764f6, scoped to what changed since b8c3d7b. The Codex job was cancelled again at its time cap. The rebase onto 87a606b (#831) brought in only main's
Testing: e2e 21/22 run the built CLI. Removing any one of the four parts makes one of them fail. The earlier checks and the post-rebase run are in the previous comment. |
…keep its queue (Tencent#808) Cause: in self mode, readConfigFrom re-anchors repo.localPath to each checkout's .teamai/, and the teamai-learnings / teamai-reports checkouts (getWorktreeDir) and the queue of unpublished learnings (pendingLearningsDir) were derived from localPath. Git checks a branch out in one worktree only, so the first checkout to create them locked every other checkout out ("'teamai-learnings' is already used by worktree"), and a learning queued in a linked worktree went with it on a plain `git worktree remove`. The search index sat in the shared partition, so recall served whichever checkout rebuilt it last, with paths into it. Mechanism: - getWorktreeDir and pendingLearningsDir return <dataHome>/<dirname> and <dataHome>/pending-learnings in self mode: the partition every checkout shares. The branch locks move with the checkouts. - getProjectSearchIndexPath keys the self-mode index per checkout, workspaces/<managedMcpWorkspaceId(root)>/search-index.json, as managed MCP is. recall, pull, contribute and viz use it. Learnings are shared, so a contribute or pull rebuild deletes the other checkouts' index files in the partition; their next recall rebuilds from their own roots. - contribute and import --from-mr now migrate first, as init/pull/push do (import's other modes do not: --cache-status --json prints only JSON; nor do contribute --scope user and import --from-mr --output, which do not write this project's queue). - The self migration moves a checkout's old queue into the partition file by file, never overwriting: same content is dropped, different content is kept with a warning. - ensureWorktree, before `git worktree add`, removes a registration of the same branch at <checkout>/.teamai/<dirname> without --force. A clean one goes, so a worktree works without a pull first; one with uncommitted changes stays, and the error names it and the next step. The error is also warned, since reports and refresh callers log it at debug only. Deleting it by hand, which the error offers, loses those changes, and it says so. While any checkout refusal blocks the publish, contribute and import --from-mr say the learning stays queued until that checkout is dealt with, not that the next pull publishes it. refresh re-throws it, so recall maintenance/promote stop with exit 1 instead of writing into the missing shared checkout, which the next publish would clear as stale. - Git mode keeps its checkouts at the same partition paths, so ensureWorktree accepts an existing checkout only when its git common dir matches the owning repo's, and otherwise refuses it without removing it (ForeignCheckoutError). A checkout git cannot open is recreated only while this repo still registers it (its gitdir's commondir leads here); one whose registration is gone, even with a clone at the same path (init reclones another team repo there), and any other is refused the same way and never removed. checkOwner and isForeignByFiles fail closed alike. refresh re-throws it too: recall maintenance/promote, members and projects members stop with exit 1. Every index build (pull, contribute, recall's rebuild), digest and viz use indexableLearningsRoots, which leaves out only that checkout's learnings. - contribute and import --from-mr stop with exit 1, saving nothing, when the migration stands down on a busy sync lock: the queue would still be the checkout's. init, pull and push go on as before. - refresh returns busy before ensureWorktree when a write holds the lock: every checkout now shares the path, and the holder may be creating it. recall maintenance/promote stop on either the learnings or the reports lock (CheckoutLockedError); members, projects members, digest, pull, stats and viz read through readableReportsWorktree, which skips ensure when busy and refuses another repository's copy (checkOwner). - A queue's owner is its install's kind and team repository (repo.remote, compared as remotesMatch does, Tencent#823 item 13). init sets a queue aside as pending-learnings.<old kind>, or pending-learnings.<kind>-<repo> for another team repository of the same kind, when it changes the owner, so the new install never publishes the old one's learnings, and deletes every search index in the data home, built from the other repository; a previous config that exists but cannot be read names no owner, and its queue goes to pending-learnings.unknown and the indexes go too; the writer and the publish re-check the owner under the queue lock, and a checkout's old queue of another owner is set aside the same way; uninstall lists every unpublished queue before it asks. - Index builds (pull, contribute, recall's rebuild) and recall feedback --negative take the votes directory from indexableVotesDir, which leaves out another repository's reports checkout, so its team's votes never rank or lower this project's learnings. - The vote-judge hook drops a foreign learnings checkout from its allowed roots, judged from the checkout's .git file with no git process. - The self migration deletes the old shared search-index.json instead of moving it; a full pull removes workspaces/<id>/ of removed worktrees. - import --from-mr, which queues and publishes as contribute does (Tencent#823), leaves another repository's checkout out of its supersede check. - The migration's queue drain drops every checkout's index file, so recall rebuilds with the moved learnings. - A git or http checkout whose partition another checkout already built settles its queue by the partition's config before its .teamai/ is retired to .teamai.bak, which a removed worktree would take with it; a queue that cannot move keeps the directory. The self migration stands down, as the other kinds do, while the partition's config.yaml cannot be read, and relocates nothing, keeping config.yaml, while a learning is left in the checkout's queue once settled (a busy queue lock). A superseded install's config.yaml moves last, after its queue is set aside, and stays while a learning is still queued there, so a failed move or a late learning resumes on the next run. Git and http mode paths are unchanged.
82764f6 to
59a37ae
Compare
…eckout (Tencent#823) Item 7. import --from-mr in 0.25.0 to 0.26.0-beta.3 wrote learnings/<date>-<title>.md, with source_mr in its frontmatter, into the learnings checkout and never committed it. Nothing published it. In single-repo mode it also kept `git worktree remove` from removing the checkout an older teamai left in .teamai/, so every pull and contribute stopped on CheckoutRefusedError. publishQueuedLearnings now takes the sync lock first, and under it, before listing the queue, queues every untracked file of exactly that shape (directly under learnings/, date name, source_mr), in the active namespace and with contribute's name, then deletes the original. It finds the one checkout this repo registers for the branch (git worktree list), so the shared checkout and the old .teamai/learnings-wt are both covered and another repository's never is. A file the branch or the queue already has, by source_mr or by content, is deleted instead, and the warning names what has it. A dry run touches nothing. Item 21. The branch side of that duplicate check was the checkout's own tracked files. In single-repo mode the checkout is often the old .teamai/learnings-wt, which nothing syncs any more, so a teammate's later import of the same MR was missed and the remnant went out as a duplicate. When there are remnants, the check now also fetches origin/teamai-learnings (best effort) and reads what origin has that the checkout's commit lacks. Item 20. pull --dry-run published the queue: publishQueuedLearnings honoured dryRun only for the remnants. It now stops after listing the queue, and pull prints "[dry-run] Would publish N queued learning(s)" instead of publishing or warning. Maintenance sweep. publishLearningsMaintenance staged all of learnings/, so a confidence write-back or a prune swept any uncommitted file into its commit. confidence write-back, prune and promote now return the files they wrote or removed, and only those are staged (a removed file git never tracked is left out, since naming it would fail the add). That exposed a second bug: simple-git lists a staged rename under `renamed`, not `staged`, so a `prune --archive` with nothing else to stage counted as nothing to commit and was never published. commitAndPushAt now counts renames. Tencent#814 follow-ups. drainCheckoutQueue is gone: the preAction migration moves a checkout's queue before contribute and import --from-mr. Retire-only now says "Retired <legacy> to <backup>: this project's data already lives in <partition>"; a linked worktree lands there too, so "Finished an interrupted migration" was wrong for it. config.yaml.*.tmp, the temp an interrupted config save leaves (Tencent#831), is ignored in the single-repo and project-scope .gitignore, and the single-repo self-heal adds it. Item 15. After a failed refresh, readableReportsWorktree called ensure without the reports lock, so it could create the checkout while a writer that had just taken the lock created it too. It now refreshes once more under the lock and throws the cause if that fails as well. Item 17. init replaced the team clone before saving the new config, so an init that stopped in between (an unknown --role, a busy queue lock) left the old team's config.yaml beside the new team's clone. Just before it clones another owner's repo, init now settles the old install as the final save would (queue set aside, indexes dropped) and moves its config.yaml to config.yaml.previous. A failed init then leaves no config, and commands ask for teamai init.
2 participants
Summary
In self mode only a repo's first checkout could publish learnings and reports. In a linked worktree,
contributefailed with'teamai-learnings' is already used by worktreeand saved the learning locally, andgit worktree removethen deleted the queue. Side-branch checkouts and the queue now live in the shared partition and each checkout has its own search index. In every mode, teamai refuses a checkout it cannot prove belongs to this repo: it does not read or delete it. Queue writes, the migration andinitshare one queue lock, and a queue belongs to its install's kind and team repo (#823 items 11, 13).maincontributein linked worktreebalready used by worktree, saved locallygit worktree remove bafter a queuedcontribute(migrated or not)r's nextpullpublishes itrecallinrafterrandbcontributeb's only, path intobimport --from-mrinbb/.teamai/r/.teamai/learnings-wtcontributesays nopullpublishes until it is dealt withb, queue cannot move (sync lock held, partition without a usableconfig.yaml, name clash, queue lock busy)b/.teamai/, lost withbcontribute,import --from-mr,initexit 1, cause and next step named, nothing savedrandb, one queued each;pullinr, thenb; removebb's learning retired intob/.teamai.bakinit --selfinrover a git install;bmergesmainAuto-migration failed: migration refused to rename …on every writepending-learnings.git, old install inb/.teamai.bak, knowledge untouchedconfig.yamldoes not parseconfig.yamldeletedconfig.yamlin the backup; next run publishes the queue as selfconfig.yamlmoves last, kept while a learning is queued ('skipped'); next run resumesr/.teamai/pending-learnings(older self install, project since switched to a team repo)pending-learnings.self, named; beside an unparseableconfig.yaml: exit 1, warning names itcontribute --scope user,import --from-mr --output.teamai/search-index.json; removed worktree'sworkspaces/<id>/pullinitswitches git ↔ self, learnings queuedpending-learnings.<old kind>, reported; indexes droppedinitagainst another team repopending-learnings.<kind>-<repo>; same repo written another way (.git,/, SSH/HTTPS): keptinit, previousconfig.yamldoes not parse, learnings queuedpending-learnings.unknown, config named; every index dropped.git, no git)learnings-wt/.gitleads to a moved, deleted or recloned repo, uncommitted filescontributedeletes it;recallindexes itteamai-learningschecked out outside the partition; maintenance, promotelearnings-wt, later deletedThe learnings checkout is locked: …/The reports checkout is locked: …contributewhile the migration orinitchanges its install.teamai.bak, or published to the new repoThis project's teamai install changed while this command ran (…)Another teamai command is moving this project's queued learnings (…), nothing saveduninstallwith learnings queuedType of Change
Test Plan
npx tsc --noEmitpasses (82764f6 on 87a606b)npx vitest runpasses: 326 files, 5043 passed, 1 skipped (82764f6 on 87a606b)npm run test:e2e -- --retry 0: 57 files passed, 3 skipped; 336 passed, 26 skipped (82764f6 before its rebase onto 87a606b, no conflicts; after it,e2e/self-mode-worktrees-808,e2e/data-layout-migration,e2e/import-mr-publish-823: 35 passed)e2e/self-mode-worktrees-808.test.ts(22, real CLI),e2e/import-mr-publish-823refusal case; unit:import-mr-learnings-checkout,viz-foreign-checkout,reports-votes-owner,reports-readers-busy,stats-foreign-reports(real git),pending-learnings-mode-switch,search-index-drop-all,readableReportsWorktree,migrate,hook-handlers,maintenance-pathse2e 1–22: linked-worktree publish and queue survival (1, 2, 14); dirty, foreign and unproven checkouts (3, 4, 5, 20); locks (6, 15); mode and team switches (7, 9, 12, 17, 18, 19); queue kept in the checkout stops the command (8, 11, 13, 16, 22); checkout outside the partition (10); orphan queue (21).
Real CLI: the issue on origin/main (ec56a67), then this PR (sandbox HOME, bare origin;
$SBsandbox,$PARTpartition)Ablations: origin/main (ec56a67) fails e2e 1 (
not to contain 'already used by worktree'). Each part, reverted alone, fails at least one test. That is 108 parts: 44 in the first build, then 8, 4, 4, 8, 5, 1, 2, 8, 8, 6, 3, 3 and 4 in the rounds from 6 on. The same-form remote test is a guard and passes on both builds, and raw remote equality is caught by a unit test only. The late-learning superseded case is injected inmigrate, since no real-CLI run can time it.Not run:
init <url>against a hosted provider (e2e uses local bare repos behindurl.insteadOf), a real MR fetch, Windows.Related Issues
Closes #808
Part of #823: items 11 (queue lock) and 13 (queue owner).
Notes for Reviewers
Door: two-way, with manual cleanup. From reading
main'sgetWorktreeDirandensureWorktree, not run: after a revert, a self-mode checkout'sworktree addat<checkout>/.teamai/learnings-wtfails on the live<partition>/learnings-wtwith #808's error, so learnings are saved locally untilgit worktree remove <partition>/learnings-wt(andreports-wt).mainalso does not read<partition>/pending-learningsin self mode, so those learnings stay there, unpublished, until someone moves them back by hand. Nothing is deleted except search indexes (rebuilt) and clean old checkouts (the branch is intact). Backups (.teamai.bak) and set-aside queues (pending-learnings.*) stay on disk.Blast radius: wide. Every self-mode install migrates on the first
init,pull,push,contributeorimport --from-mrin each checkout. In every mode, teamai now refuses checkouts it cannot prove are this repo's, including the ones left after a team repo is recloned: the member moves them aside once, as the message says.contribute,import --from-mrandinitexit 1 in the states listed above. The commands affected areinit,pull,push,contribute,import --from-mr,recall maintenance,promote,members,projects members,digest,stats,viz,uninstalland the vote-judge hook.Why these shapes:
'busy'.'skipped'still means the legacy dir is kept on purpose (Migration keeps the legacy .teamai/ while the partition config cannot be read #797).initstops instead of settling, so the migration stays the only thing that moves a checkout's queue..teamai/.Known limits still in force:
contributebeside this version's migration can still leave its learning in.teamai.bak, as onmain.init. This is conservative on purpose: the warning names the directory and says to move the files intopending-learnings/if they belong here.<partition>/search-index.jsonmisses the drained learning until the next rebuild (P2).contributeexits 1 with "see the warning above" and prints no warning:--dry-runwith an orphan queue, an empty legacyconfig.yaml, and an orphan queue beside an unreadable partitionconfig.yaml(P3).settleOrphanQueuetreats a queue with no config beside it as an old self install's. If a git install'srepo.localPathis inside the checkout,pullsets its live queue aside aspending-learnings.self: nothing is lost, andcontributealready refuses that layout (P3).describeConfigErrorexists). With HOME a git repo and an unparseable user config, it says the nextinit,pullorpushmoves the queue, but nothing does (P3).uninstallhint offersteamai pullfor set-aside queues too.digestprints a refused reports checkout twice. The docs miss the CHANGELOG exits, "a plainrecallruns no git" and the reports-lock message (P3).initonly aftercontributehas moved the orphan queue, soinitwith the orphan queue still present is not tested on its own.initstops even with nothing queued when the checkout's data cannot move (chosen in round 13).main, for [bug] Follow-ups to the worktree fixes: import --from-mr never shares its learning, and push can still revert a teammate's update #823: readers callensureunlocked after a failed refresh;vizin user scope reads unfiltered roots;init's busy exit after a team-repo reclone leaves the old config beside the new clone; a recall rebuild with no index during a skills collision; two unmigrated checkouts sweeping each other's old checkout at the same instant.local-agent.test.tsis flaky onmaintoo.Changed expectations:
git-kind-reports' "rebuilds a dangling sibling reports worktree after the clone is removed and re-cloned" (frommain) now expects the refusal, with the uncommitted file kept, then the rebuild once the checkout is moved aside.hook-handlers' "this project, pruned" now expects the vote judge to drop that root.helpers/install-config.ts,writeSelfPartitionConfig), since a write re-reads it. Mocks gainindexableVotesDir,readableReportsWorktree,isSilentand the realremotesMatch. e2e 16's old install records the origin itsinitsees. Other assertions are unchanged.Rebases: #816 (the index builds keep both
deliveredIndexSourcesand the per-checkout index andindexableVotesDir); #825 (import --from-mrqueues and publishes ascontribute, and its supersede check keepsindexableLearningsRoots); the rest, up to 87a606b (#831), without conflicts.Review rounds, one commit throughout (from round 13 on, Codex hit its 15-minute cap, so the workflow prompt ran locally):
import --from-mrcontributedrains its old queue;digest,viz, vote judge skip a refused checkoutimport --from-mrdrains;refreshre-throws every refusalrefreshreturns busy; maintenance and promote exit 1contribute,import --from-mrmigrate first; unproven checkouts refused; reports lock;readableReportsWorktree'busy';stats,vizread throughreadableReportsWorktreerefreshreturnsfailedqueueKeptInCheckout; the superseded planconfig.yamlmoves lastconfig.yamlstatsP2 cannot happen; test onlyinitstops ascontributedoescommondiris no proof; the self migration settles; refusal messagessettleOrphanQueue)