Repository navigation
fix(learnings): publish what an older import --from-mr left in the checkout (#823) - #838
Merged
jeff-r2026 merged 17 commits intoSep 28, 2026
Merged
Conversation
…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.
SaulMoro
force-pushed
the
fix-823-data-layout-followups
branch
from
September 25, 2026 19:28
14bd21b to
b018180
Compare
The PR description includes sufficient unit and representative real-CLI testing evidence. |
…ter a failed clone Maintenance now stages exactly the files it names: commitAndPushAt and the removed-file ls-files lookup pass --literal-pathspecs, so a learning named with [ or * no longer stages the stray files it matches as a pattern. init reads the config it set aside when the rerun finds none, so an init whose replacement clone failed no longer drops enabledAgents, disabledAgents, toolRoots and inheritUserScope on the next run.
The earlier literal-pathspec finding is resolved. The PR description includes sufficient representative real-CLI testing evidence. |
…lain rerun An HTTP install's learnings dir is no git checkout, so the removed-file ls-files lookup threw after a prune had already deleted the file. It now returns the same non-fatal failed publish commitAndPush gives. init without --agent keeps the carried enabledAgents and disabledAgents, so a rerun after a failed replacement clone no longer reactivates tools uninstall --agent excluded.
The previous config-preservation, literal-pathspec, and HTTP-maintenance findings are resolved. The PR description contains sufficient representative real-CLI testing evidence. |
…led push kept local, keep remnants while origin is unreachable
The earlier config-preservation, literal-pathspec, HTTP-maintenance, maintenance-retry, and stale-origin findings are otherwise resolved. The PR description includes sufficient representative real-CLI testing evidence. |
…arnings branch, never let one bad maintenance record or remnant block the rest
|
Findings
The previously reported findings are otherwise resolved. The PR description includes sufficient representative real-CLI testing evidence. |
…, keep a maintenance record a read failed on
|
Findings
All previously reported findings are resolved in the current diff. The PR description includes sufficient representative real-CLI testing evidence. |
jeff-r2026
self-requested a review
September 26, 2026 11:25
jeff-r2026
requested changes
Sep 26, 2026
jeff-r2026
left a comment
jeff-r2026
left a comment
Collaborator
There was a problem hiding this comment.
Please resolve the conflicts.
…ollowups # Conflicts: # src/import.ts
|
Findings
All other previously reported findings are resolved. The PR description includes sufficient representative real-CLI testing evidence. |
… the whole index (Tencent#823)
|
Findings
The prior exact-path commit finding is resolved. The PR description includes sufficient representative real-CLI testing evidence. |
…etry rebase (Tencent#823) The path-limited commit leaves a file someone else staged in the checkout, and git refuses to rebase with anything staged, so a non-fast-forward push failed every retry. Snapshot it with git stash create around the rebase, as syncWorktree does, and re-apply it with --index so it stays staged.
|
Findings
The previous staged-change push-retry finding is resolved. The PR description provides sufficient unit and representative real-CLI testing evidence. |
…er the queue lock and ownership check; move a stale config aside when init reuses a clone (Tencent#823)
|
Findings
The previously reported findings are otherwise resolved. The PR description includes sufficient representative real-CLI testing evidence. |
…ollowups # Conflicts: # src/init.ts
|
Findings
All other previously reported findings are resolved. The PR description includes sufficient representative real-CLI testing evidence. |
…apshot restore conflicts; never publish a hand edit as a recorded maintenance run (Tencent#823)
…snapshot without a reset, keep a conflicting staged file unstaged, no hand-edit warning for a merged maintenance commit (Tencent#823)
|
Findings
The previously reported findings are resolved. The PR description includes sufficient representative real-CLI testing evidence. |
jeff-r2026
approved these changes
Sep 28, 2026
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
The learning an older
import --from-mr(0.25.0 to 0.26.0-beta.3) left uncommitted in the learnings checkout now reaches the team, and it no longer keeps a single-repo project's old.teamai/learnings-wtfrom being removed. Maintenance publishes only what it changed.pull --dry-runno longer publishes the queue, and the leftover's duplicate check sees what a teammate published since the old checkout last synced. This also includes #814's follow-ups and items 15, 17, 20 and 21.Type of Change
Causes:
import --from-mrin 0.25.0 to 0.26.0-beta.3 (feat(learnings): write learnings to the teamai-learnings branch #616 until fix(import): publish the learningimport --from-mrextracts (#823) #825) wrote the learning into the checkout and never committed it. Git will not remove a checkout with untracked files without--force, so in single-repo mode everypullandcontributestopped onCheckoutRefusedError.publishLearningsMaintenancestaged all oflearnings/. Confidence write-back of an inherited learning,prune --archiveand the promote mark create new files, sogit add -ualone was not enough.prune --archive(found here, also broken on main): the archive move stages as a rename, which simple-git does not list instaged. With nothing else staged it counted as "nothing to commit", and the archive never left the machine..teamai/learnings-wt, which nothing syncs any more, so a teammate's later import of the same MR was missed and the leftover went out as a duplicate.publishQueuedLearningshonoureddryRunonly for the leftover adoption, sopull --dry-runpublished the whole queue (confirmed with the real CLI below).ensurewithout the lock, so it could create the checkout while a writer that had just taken the lock created it too.initremoved the old clone and cloned the new team repo long before it saved the new config, so aninitthat stopped in between left team A'sconfig.yamlbeside team B's clone.Test Plan
npx tsc --noEmitpassesnpx vitest run --maxWorkers=4(Node 22), after review 9: 334 files, 5196 passed, 1 skipped, 20 timed out at 15 s. All 20 are ingit-kind-learnings,git-kind-reportsandlearnings-import-remnants(real git, 5 to 7 s a test when the machine is idle), and all three pass on their own: 31 passed, and 32 passed. Review 8: 331 files, 5151 passed, 1 skippednpm run lintexits 0New tests, red on
origin/main(7c834ce) unless marked guard:Ablations: each part was reverted alone and its test failed at the expected assertion. That covers adoption, the sync-lock guard, the dry-run guard, dedup (and source_mr-only), each signature check, the registered-checkout lookup, the namespace, staging the exact list, rename counting, item 15, item 17, Q10, the three Q11 lists, item 20 (the publish gate, the
Would publishline, the warning gate), item 21 (the origin read, and the fetch alone), and for Q9 the preAction migration that now owns the linked-worktree queue.Updated tests:
writeBackConfidencereturns the paths it wrote, andexecutePrune/executePromotionreturn theirs, so their assertions check those paths now. Theimport-mr-learnings-checkoutcase for a linked worktree's old queue calledimportCmddirectly, so it bypassed the preAction migration that owns that job since Q9; it moved to the e2e above. Inreports-branch-readonly, the "lock is free" case passed only because its mocked refresh failed and fell through to the unlockedensure(item 15). Its mock now has the realstatus()shape. Review 6: thecommitAndPushReportscases feed the staged paths through thediff --cachedcall that now counts them, notstatus(); their assertions are unchanged.E2E run (
npm run build,--retry 0): the new files, plusself-mode-worktrees-808,import-mr-publish-823,data-layout-migration,learnings-sync-704-705,init-project-all,init-unattended,reports-writer-sync-561,members-legacy-roster,dashboard-unified,deleted-worktree-scope-810,pull-new-worktree-807,scope-isolation-issue85andmulti-project: 14 files, 80 passed. The other files that runpull --dry-run(e2e,managed-resources-uninstall,push-stale-worktree-812,push-sync-followups-823,role-scoped-agents): 5 files, 34 passed, 22 skipped. The full e2e suite was not run. After the review fixes:self-mode-worktrees-808,learnings-remnants-823,init-project-all,init-unattended,data-layout-migration,multi-project: 6 files, 40 passed. After review 2:self-mode-worktrees-808,learnings-remnants-823: 2 files, 32 passed. After review 3: those two pluslearnings-sync-704-705: 3 files, 33 passed. After review 4: the same 3 files, 33 passed. After review 5: the same 3 files, 33 passed. After review 6:self-mode-worktrees-808,learnings-remnants-823: 2 files, 32 passed. After review 8: those two plusinit-project-all: 3 files, 37 passed. After review 9:learnings-remnants-823,self-mode-worktrees-808: 2 files, 33 passed.Before: single-repo
contributewith the remnant in the old checkout:⚠ ... still has teamai-learnings checked out, and git will not remove it,Saved locally, nothing on origin.After:
⚠ Queued 1 learning(s) an older teamai import --from-mr left unpublished: …,✔ Contributed, the old checkout is gone, and both learnings are onteamai-learnings.Real CLI, #823 item 7 step 5: before (origin/main 7c834ce) and after (this branch)
recall maintenance --prune --archivealone, git mode (before / after): origin keepslearnings/low-note.mdand the checkout keeps a stagedR learnings/low-note.md -> learnings/_archive/low-note.md/✔ Published maintenance changes to the learnings branch, origin haslearnings/_archive/low-note.md.Real CLI, #823 item 20:
pull --dry-runwith one queued learning, before (14bd21b) and afterReal CLI, #823 item 21: single-repo, leftover in the old checkout, a teammate already published its MR; before (14bd21b) and after
Real CLI, review fix for item 17:
initrerun after the replacement clone failed, before (b018180) and afterReal CLI, review 2:
initrerun with no--agentafter the replacement clone failed, before (c23302d) and afterReal CLI, review 3:
recall maintenance --prune --archivewith origin offline, thenpull; before (8523c0d) and afterReal CLI, review 4: origin without
teamai-learnings, a leftover in the checkout, thenpull; before (331d929) and afterReal CLI, review 5: origin deleted the learning the checkout still tracks, a leftover from the same MR, then
pull; before (4defc3e) and afterReal CLI, review 6: a file staged by hand in the learnings checkout, then prune --archive (this branch)
Real CLI, review 7: a file staged by hand, and a teammate's push lands while ours is in flight (before / after)
A one-shot
pre-pushhook in the team clone pushes a teammate'slearnings/bob.mdto origin, so our push is rejected as non-fast-forward and the retry has to rebase. Before is the merge head62b3103e, after is52deae81.Real CLI, review 8: config A beside a clone of B that an earlier
initleft,init Breuses it and its refresh fails (before / after)Sandbox HOME, team A and B as local bare repos behind
insteadOf; B's rewrite is dropped andGIT_ALLOW_PROTOCOL=fileset so the reused clone matches and its refresh fails. Before is52deae81, after is this branch.Real CLI, review 9: a prune whose push failed, the pruned learning restored by hand, then
pull(before / after)Sandbox HOME, the team repo as a local bare repo behind
insteadOf, a teammate vote that putslow-notebelow the prune threshold.team.gitis taken offline for the prune and back online for the pull. Before ise1971a38, after is this branch.Related Issues
Part of #823 (item 7 and #814 follow-ups; also items 15, 17, 20 and 21).
Notes for Reviewers
Door: two-way. Blast radius: learnings publishing and maintenance,
initreplacing a team repo, reports readers.src/import.ts, keeps fix(learnings): find learnings despite a broken manifest and in the dashboard; show the MR import prompt (#823) #834's extraction before the task list and its namespace-aware duplicate check, and drops thedrainCheckoutQueuecall this PR removes.e1971a38): the one conflict, insrc/init.ts's imports, keeps bothLocalConfigSchemaand feat(init): let a member choose the git provider with --provider (#789) #844'sProviderNameSchema/ProviderName; feat(init): let a member choose the git provider with --provider (#789) #844's provider selection and this PR's move-aside and carry-over compose unchanged (real CLI:init B --provider gitover a stale config A, clone-failure rerun and clone-reuse, recordsprovider: git, team B's remote and the carriedenabledAgents). This PR writes no state or index file, so fix(persistence): write state.json and the search index atomically (#854) #855 does not interact, and it adds no loader call fix(dry-run): thread { dryRun } through the loaders contribute, session save and recall use (#850) #853 would need to thread.source_mror content. For a record, it meanscommitAndPushreturnedpublished, oralready-present(nothing staged and nothing pastorigin/teamai-learnings), or a successful read found it malformed. Audit of every delete site in the new code: the covered-remnantrmnow checks origin's tree, not the checkout; the queued-remnantrmfollows asavedqueue write; the malformed-recordrmnow needs a successful read; the published-recordrmis gated on the two statuses above. When origin cannot be fetched or read, or a queue entry or record cannot be read, nothing is deleted. Any other untracked file in the checkout is left alone.publishQueuedLearningsnow takes the sync lock before it lists the queue (sync, then queue lock, the same order as the migration), so two commands cannot adopt one remnant twice.pullstill passesholdsSyncLock.config.yamlreads the lastconfig.yaml.previous[.<n>]for the settings a re-init carries forward (enabledAgents,disabledAgents,toolRoots,inheritUserScope), and only those. With no--agent, both agent lists are kept as they were; before, any re-init without--agentdropped them, which contradicted the documented durableuninstall --agentexclusion. With--agent, behavior is unchanged. Queue ownership and the settle still go by the live config.initHttpnever moves a config aside and is unchanged.origin/teamai-learningsonly when there is a leftover to check, so ordinary runs make no extra network call. When the fetch fails, the leftover stays where it is until a run can fetch (review 3). When origin answersls-remotewithout the branch (an offline first publish never pushed it), origin adds nothing and the leftover is queued (review 4); only a failedls-remotekeeps it. Review 5: the duplicate check reads the fetched tree itself (ls-treeplus onegit showper learning), so a copy the checkout still tracks after origin deleted or replaced it no longer counts. That costs one git call per learning on origin, only in a run that finds a leftover.pull --dry-rundoes not preview leftovers a real run would queue or delete, and it still takes the queue lock briefly to list the queue. Item 16 (viz) is not in this PR: fix(learnings): find learnings despite a broken manifest and in the dashboard; show the MR import prompt (#823) #834 (merged) fixed it.<date>-.mdnames, a removed never-tracked file failing the maintenance add, docs wording. Not changed: item 17 settles the old install before the clone, so a failed clone followed byinit Aagain leaves A's queue inpending-learnings.git-<A>(named in the warning, nothing lost); settling after the clone would bring back item 17's busy-lock case. A remnant with the samesource_mras a published learning is deleted even if its content differs, as decided 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.teamai-maintenance/, one file per run). No gitignore change is needed there, and the record goes with the checkout and its changes. Every laterpublishLearningsMaintenance, and everypublishQueuedLearningsunder the sync lock (pull,contribute,import --from-mr), publishes all the records, including when the queue is empty. A record is dropped only onpublishedoralready-present. A removed file that an earlier attempt already committed is left out of thegit add, so the commit it made is still pushed. Remaining edge: if a refresh rebases an unpushed maintenance commit that conflicts with a teammate's change on origin, the refresh drops that commit, as it always has, so the teammate's version wins, and the record then clears asalready-present. Review 4: records are written throughwriteFileAtomic, and only*.jsonis read, so an interrupted write leaves a.tmpnobody reads; a record that does not parse is removed with⚠ Removed unreadable maintenance record <file> (...), naming the checkout to check withgit status, and the other records still publish.commitSkippingHookstakes an optional path list and then runsgit --literal-pathspecs commit --no-verify -- <paths>, so a file someone else staged in the checkout stays staged and out of the commit. Callers:commitAndPushAt(learnings and reports branches, everycommitAndPush/update) passes itsfilesand counts staged changes only under them, so an empty list or an unrelated staged file commits nothing. Unchanged, still the whole index: the orphan-branch init commit inensureWorktree(it runsgit rm -r --cached .first, so the index holds only.gitignore), andpushRepoBranch(the team-repo clone, not a side-branch checkout; it stages with glob pathspecs and resets the clone when nothing is staged).git rebase --autostashwould fix the refusal, but on an apply conflict it stores the stash inrefs/stash, which every worktree of the repo shares, and its re-apply (stash applywithout--index) leaves a staged edit unstaged. The retry reusessyncWorktree'sstash createsnapshot instead.stash apply --indexfails without touching anything when the staged changes no longer apply, and the code then falls back to the plain apply. The other rebase on this path, insyncWorktree, already snapshotted; it now keeps the index too.merge --ff-onlyruns with staged files as it did before.readPendingForInstall, which lists and reads it under the queue lock after the same ownership checklistPendingForInstallmakes. Whenconfig.yamlnames another install, or the lock stays busy, every remnant stays for a later run. The lock covers only local reads; the origin fetch runs before it. A remnant deleted after the lock is released was covered by this install's own entry, and aninitthat switches afterwards moves that entry aside, not away.initreuses an existing clone of the requested repo, before its refresh, so a config left beside that clone by an earlier failedinitis no longer live if this one fails too.stash createstores the index as the snapshot's second parent, so no extra state is kept. Whenstash apply --indexrefuses, the plain apply runs. Each conflicted path is then set to the snapshot's own copy (checkout <snapshot> -- <path>, orrmwhere the snapshot had removed the file; the version that goes is origin's, which HEAD holds) and left unstaged. On failure it warns with the snapshot id to restore from; it no longer runsreset --hard, which on a modify/delete conflict discarded every other uncommitted file. Staging comes back only with proof: a path origin did not change gets its staged index entry back exactly (update-index --cacheinfo), and a path origin changed is staged again only when it merged cleanly and holds exactly the staged blob. A conflicted path is never staged again, because that would stage a revert of origin's change. Every other path keeps its content, unstaged, and one warning names them withgit diff HEADas the next step. The snapshot id is also written to debug.log, so a run killed between the reset and the re-apply can be recovered.contents, the sha256 of each file as the run left it, or null for a removal. A retry publishes a file when it still matches the newest record that names it, or when it has nothing uncommitted (an earlier attempt committed the change and a refresh merged origin's into it). Otherwise the file is left out and kept, and a warning names it. Records withoutcontentspublish as before. Limits: if a refresh merges origin's change into a maintenance edit that was never committed, the file no longer matches and is treated as a hand edit, so the change stays local with the warning. Telling that apart from a hand edit would need the run's full content, not a hash. The hash is checked before the branch lock is taken, so an edit made in that short window is still published. Forprune --archive, an edited archive copy is left out while the recorded removal goes out, so origin keeps neither copy and the edited one stays local.