Repository navigation
fix(config): keep config.yaml readable while it is being saved (#823) - #831
Merged
Merged
Conversation
The PR description includes sufficient unit, E2E, and representative real-CLI testing. |
SaulMoro
force-pushed
the
fix-823-atomic-config
branch
from
September 25, 2026 12:16
5a2b57a to
807e19c
Compare
Collaborator
Author
|
Fixed in the amended commit. |
The earlier finding is resolved for symlinks whose targets exist. The PR description includes sufficient unit, E2E, and representative real-CLI testing. |
…nt#823) config.yaml was rewritten in place, so a command that read it mid-save saw an empty file ("Invalid project config"), and a failed write left it truncated. The three local config writers (saveLocalConfig, saveLocalConfigForScope, the legacy role migration) now go through writeFileAtomic: a sibling temp file renamed over the target, removed on failure. The partition config.yaml already used it. An existing config.yaml keeps its mode; a newly created one is 0600 (was the umask default, usually 0644), as the partition config already is. writeFileAtomic now writes a symlinked target at the end of its link chain (temp file next to that file), so a symlinked config.yaml keeps its link instead of becoming a regular file. A dangling link gets its missing target (and directory) created, as the in-place write did; a link loop is refused with an error and nothing is written.
SaulMoro
force-pushed
the
fix-823-atomic-config
branch
from
September 25, 2026 12:30
807e19c to
952a77c
Compare
Collaborator
Author
|
Fixed in 952a77c (same commit, amended). Test: |
The earlier existing- and dangling-symlink findings are resolved. The PR description includes sufficient unit, E2E, and representative real-CLI testing. |
jeff-r2026
approved these changes
Sep 25, 2026
Merged
5 of 9 tasks
SaulMoro
added a commit
to SaulMoro/teamai-cli
that referenced
this pull request
Sep 25, 2026
…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.
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>
jeff-r2026
pushed a commit
that referenced
this pull request
Sep 28, 2026
…eckout (#823) (#838) * fix(learnings): publish what an older import --from-mr left in the checkout (#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. #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 (#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. * fix(learnings): address review — literal pathspecs, carry settings after 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. * fix(learnings): address review — HTTP maintenance, agent lists on a plain 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. * fix(learnings): address review — retry maintenance a busy lock or failed push kept local, keep remnants while origin is unreachable * fix(learnings): address review — queue remnants when origin has no learnings branch, never let one bad maintenance record or remnant block the rest * fix(learnings): address review — dedup remnants against origin's tree, keep a maintenance record a read failed on * fix(learnings): address review — commit only the published paths, not the whole index (#823) * fix(learnings): address review — keep a staged file across the push-retry rebase (#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. * fix(learnings): address review — read the queue for remnant dedup under the queue lock and ownership check; move a stale config aside when init reuses a clone (#823) * ci: re-run checks (flaky dry-run-load-path test, unrelated to this PR) * fix(learnings): address review — keep staged files staged when the snapshot restore conflicts; never publish a hand edit as a recorded maintenance run (#823) * fix(learnings): address review — resolve snapshot conflicts from the snapshot without a reset, keep a conflicting staged file unstaged, no hand-edit warning for a merged maintenance commit (#823) * fix(learnings): address review — point the unpublished-edit warning at git status (#823)
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
config.yamlwas rewritten in place. Opening it for writing truncates it, so a command reading it mid-save saw an empty file and failed, and a failed write left it truncated. The three writers of the local config now use the existingwriteFileAtomic(same-dir temp file, renamed over the target, temp removed on failure). The partition config already used it.writeFileAtomicnow follows a symlinked target to the end of its link chain first, so a symlinkedconfig.yamlkeeps its link, and a dangling link gets its missing target created, as the in-place write did.Type of Change
Test Plan
npx tsc --noEmitpassesnpx vitest runpasses: 317 files, 4978 passed, 1 skippedsrc/__tests__/config-atomic-save.test.ts(14 tests)--retry 0:init-unattended,init-project-all,data-layout-migration,multi-project,roles-tags-pull(5 files, 10 tests) passteamai init, thenteamai skill exclude addon a plain, a symlinked and a dangling-symlinkedconfig.yaml, andiniton a link loop, in an isolated HOME (below)The new tests make each write into
~/.teamaifirst truncate its target (the stateopen(O_TRUNC)exposes), then run a hook, then finish or throwENOSPC:expected undefined to be 'dev'(x2). A failed write givesexpected '' to be 'repo:...'(x3).expected false to be true(the link became a regular file).realpath, ENOENT ignored): the 2 dangling-link tests fail withexpected false to be true(the link became a regular file), and the 2 loop tests get the rawELOOP: too many symbolic links encounteredinstead of the loop error, and the symlinked-config-dir test fails withENOENT ... dotfiles/shared/config.yaml(5 red).saveLocalConfig2,saveLocalConfigForScope2). Removing the link resolution fromwriteFileAtomicturns exactly the 2 symlink tests red. In the chain walk: resolving each hop against the lexical instead of the real directory of the link turns 1 red (the symlinked config dir); creating the link's directory instead of the target's, or falling back to the link path when the target is missing, turn 3 red (both dangling-link tests and the symlinked config dir); a generic loop message turns the 2 loop tests red.Real CLI (built
dist/index.js, isolated HOME, synthetic HTTPS team URL rewritten to a local bare repo)Claude agent and the
gitprovider only. The write path does not depend on the provider or the agent.Related Issues
Part of #823 (item 14)
Notes for Reviewers
config.yamlis now0600(was0644under umask 022); an existing file keeps its mode. This matches the partition config, which this helper already writes, and the file holds no secrets (the token is~/.teamai/token).src/migrate.ts:428writes the staged copy inside the staging dir, whichverifyStagingchecks before the directory is moved into place, so no reader sees it.src/hermes-config.tswrites Hermes' ownconfig.yaml, not teamai's.config.yamlhas the same content and location.init,skill exclude,projects,roles,uninstall, the legacy role migration), but only how the bytes land.writeFileAtomic's other callers (partitionconfig.yaml, votes, model profiles, generated.gitignores) now also keep a symlinked target instead of replacing the link; none relied on replacing it, andmodels/switch.tsalready resolved links before calling it. A dangling link gets its missing target created, as an in-place write did; a link loop now fails with a named error instead of a rawELOOP.codex-review-on-assign.ymlprompt, plus separate standards and spec passes. No P1/P2. Fixed: added the mode guard test (spec pass: "keep the mode" was untested). Fixed after the Codex reviews: symlinked targets keep their link, and dangling links keep it too. Left as P3: a hard kill between the temp write and the rename leavesconfig.yaml.<pid>.<hex>.tmp, which the generated.gitignoreof a legacy in-repo.teamaidoes not match (config.yamlonly). That is a follow-up that touchesinit.ts, which fix(data-layout): let every self-mode worktree publish learnings and keep its queue (#808) #814 changes.