fix(config): keep config.yaml readable while it is being saved (#823) - #831
Conversation
The PR description includes sufficient unit, E2E, and representative real-CLI testing. |
5a2b57a to
807e19c
Compare
|
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.
807e19c to
952a77c
Compare
|
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. |
…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
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.