From b018180fecf42cfa8ff2a14a59357f35d801b280 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Fri, 25 Sep 2026 19:14:26 +0200 Subject: [PATCH 01/13] 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/-.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. --- docs/designs/data-directory-layout.md | 33 ++- docs/usage-guide.md | 16 +- docs/usage-guide.zh-CN.md | 11 +- .../e2e/learnings-remnants-823.test.ts | 275 +++++++++++++++++ .../e2e/self-mode-worktrees-808.test.ts | 201 ++++++++++++- .../import-mr-learnings-checkout.test.ts | 26 +- .../learnings-import-remnants.test.ts | 279 ++++++++++++++++++ src/__tests__/maintenance-promote.test.ts | 6 +- src/__tests__/maintenance-two-roots.test.ts | 7 +- src/__tests__/migrate.test.ts | 16 + src/__tests__/reports-branch-readonly.test.ts | 11 +- src/__tests__/reports-reader-lock.test.ts | 76 +++++ src/__tests__/single-repo-mode.test.ts | 27 +- src/__tests__/votes-e2e.test.ts | 4 +- src/contribute.ts | 30 +- src/import.ts | 4 +- src/index.ts | 21 +- src/init.ts | 91 ++++-- src/maintenance/confidence.ts | 14 +- src/maintenance/promote.ts | 12 +- src/maintenance/prune.ts | 15 +- src/migrate.ts | 6 +- src/pull.ts | 8 +- src/utils/branch-worktree.ts | 43 ++- src/utils/learnings-publish.ts | 181 +++++++++++- src/utils/reports-branch.ts | 12 +- 26 files changed, 1265 insertions(+), 160 deletions(-) create mode 100644 src/__tests__/e2e/learnings-remnants-823.test.ts create mode 100644 src/__tests__/learnings-import-remnants.test.ts create mode 100644 src/__tests__/reports-reader-lock.test.ts diff --git a/docs/designs/data-directory-layout.md b/docs/designs/data-directory-layout.md index 87ff235cb..8e934a803 100644 --- a/docs/designs/data-directory-layout.md +++ b/docs/designs/data-directory-layout.md @@ -429,7 +429,7 @@ Upgrading from the per-checkout layout: migrated still says `kind: self`, and its queue would publish to the new repository. It is set aside instead, as a mode switch does (below), to `pending-learnings.self` beside the partition queue, with a warning naming - it; `contribute` and `import --from-mr` take the same step before they queue. + it. A queue with no `config.yaml` beside it is one an older self install left after its config had moved to the partition; once the project serves another install no plan covers it, so every migrating command sets it aside the same @@ -494,7 +494,22 @@ Upgrading from the per-checkout layout: linked worktree works with no pull first. One with uncommitted changes stays: the side-branch step fails with an error naming the path and the next step (commit or move the changes, or delete the path by hand), and the queue keeps - the learnings until then. The error is also printed as a warning, because + the learnings until then. What `import --from-mr` in 0.25.0 to 0.26.0-beta.3 left there is not + such a change (#823 item 7): it wrote `learnings/<YYYY-MM-DD>-<title>.md` with + `source_mr` in the frontmatter and never committed it. `publishQueuedLearnings` + first finds the branch's one registered checkout (`git worktree list` of the + owning repo, so the shared one or the old `.teamai/<dirname>`, never another + repository's), and queues each untracked file of exactly that shape, directly + under `learnings/`, into the active namespace with `contribute`'s name, then + deletes the original. A file the branch (tracked, any namespace) or the queue + already has, by `source_mr` or by content, is deleted without queueing. The + branch is the checkout's tracked files plus what `origin/teamai-learnings` + has that the checkout's commit lacks, fetched first: the old checkout is + never synced again, so it may miss a teammate's later import of the same MR + (#823 item 21). It runs only under the sync lock, before the queue is listed, + and never on a dry run, which publishes nothing from the queue either and + `pull` reports as `Would publish N queued learning(s)` (#823 item 20); any + other file in the checkout is left alone. The error is also printed as a warning, because several callers treat a side-branch failure as non-fatal and log it at debug only; a silent (hook) run prints nothing. `refresh` does not swallow it: `recall maintenance` and `recall promote` stop with exit code 1 and write @@ -511,7 +526,10 @@ Upgrading from the per-checkout layout: reader (`members`, `projects members`, `digest`, `pull`, `stats` and `viz`, through `readableReportsWorktree`) uses the local copy and never ensures it, after the same ownership probe as `indexableVotesDir` (below): another - repository's copy is refused with `ForeignCheckoutError`. The + repository's copy is refused with `ForeignCheckoutError`. A refresh that + failed is tried once more, under the lock, and a second failure throws its + cause: a reader never creates the checkout without the lock, which a writer + that took it meanwhile may be creating (#823 item 15). The checkout there may be another repository's, or not created yet, so `recall maintenance` and `recall promote` stop with exit code 1 and write nothing when either the reports lock (they rank by its votes) or the @@ -571,6 +589,15 @@ exists but cannot be read names no owner: the loaders return null for it as for a fresh install, so `init` checks for the file, and with learnings queued moves them to `pending-learnings.unknown` the same way, the warning naming that config; queued or not, it drops the search indexes, as on an owner change. +When `init` is about to clone another owner's team repository where the old +install's clone was, it settles the old install right then, before the clone, +the same way (queue set aside, indexes dropped), and moves the old +`config.yaml` to the first free `config.yaml.previous[.<n>]` instead of saving +(#823 item 17). The save at the end still compares with the old install +loaded before the clone, finds nothing left to set aside, and writes the new +config. An `init` that stops in between (an unknown `--role`, a busy queue lock, a +prompt left) leaves no config naming the old team beside the new clone, so every +command asks for `teamai init`; nothing is deleted. A self install whose business repository moved to another URL (renamed or transferred) is another owner too: `init` sets its queue aside, and the warning names the directory to diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 94477309d..52f5bd21f 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -407,7 +407,13 @@ a side-branch checkout removes the old one. An old checkout with uncommitted changes is kept, and the command names it: nothing is published to or recalled from that branch until you commit, move or delete those changes, and `recall maintenance` and `recall promote` stop. Queued learnings stay queued -and recallable. Maintenance and promote also stop, naming the cause, when the +and recallable. A learning an older `import --from-mr` (0.25.0 to 0.26.0-beta.3) wrote into a +learnings checkout and never committed does not count: the next `pull` or +`contribute` (or an `import --from-mr` that queues a learning) queues and publishes it (in the project's +namespace, named as `contribute` names it) and says where it was, so the old +checkout can go. If the branch or the queue already has it, by its `source_mr` +or its content, it is deleted instead, and the message names the learning that +has it. Maintenance and promote also stop, naming the cause, when the checkout cannot be created, such as when `teamai-learnings` is checked out somewhere else. A git-mode install of the same project keeps its checkouts at the same paths. @@ -421,7 +427,11 @@ for the old repository (the next `recall` rebuilds them). Re-running `init` against another team repository of the same kind does the same, to `pending-learnings.<kind>-<repo>` (for example `pending-learnings.git-github.com-org-team-a`); the same repository written -another way (with or without `.git`, SSH or HTTPS) keeps the queue. When the old +another way (with or without `.git`, SSH or HTTPS) keeps the queue. Before it +clones the other team repository, `init` moves the old `config.yaml` to +`config.yaml.previous` beside it and says so: if `init` stops before it saves +the new config, every command asks for `teamai init` instead of running the old +team's config against the new clone. Run `init` again. When the old install's `config.yaml` exists but cannot be read, nothing says whose the queue is: `init` moves it to `pending-learnings.unknown`, names that file and deletes the search indexes. A checkout that had not been upgraded yet keeps its old queue the same way: the next command there @@ -1233,6 +1243,8 @@ After `--update-quality`, review the generated `.draft.md` files and rename them While another teamai command holds the learnings or reports checkout's lock, `recall maintenance` and `recall promote` exit 1 without writing anything (`The learnings checkout is locked: …`). Run them again when that command finishes. +Maintenance and promote publish only the learnings they changed. A file in the learnings checkout that nobody committed stays out of their commit. + ### Promoting Learnings When a learning reaches maturity, promote it to formal team knowledge (a skill, rule, or doc). Promotion criteria: confidence ≥ 0.90, ≥ 5 upvotes, ≥ 2 distinct contributors, age ≥ 14 days. diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 846d761a6..b50aad522 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -371,7 +371,10 @@ git 同一分支只能在一个 worktree 中检出，所以仓库的所有检出 的命令会移除旧检出。旧检出中若有未提交的改动则保留，命令会指出其路径：在你提交、 移走或删除这些改动之前，不会向该分支发布内容，也不会从该分支召回内容，`recall maintenance` 与 `recall promote` 会停止。已排队的 learning -仍留在队列中，仍可被召回。检出无法创建时（例如 `teamai-learnings` 已在别处检出），maintenance 与 promote +仍留在队列中，仍可被召回。旧版 `import --from-mr`（0.25.0 至 0.26.0-beta.3）写进 learnings 检出却从未提交的 learning +不算在内：下一次 `pull` 或 `contribute`（或排队了 learning 的 `import --from-mr`）会把它排队并发布（放入项目的命名空间，按 `contribute` +的方式命名），并指出它原来的位置，旧检出因此可以移除。若分支或队列中已有同一条（按 `source_mr` 或内容判断）， +则删除它，提示中会给出已有的那条 learning。检出无法创建时（例如 `teamai-learnings` 已在别处检出），maintenance 与 promote 同样会停止，并说明原因。 同一项目的 git 模式安装把检出放在相同的路径。切换模式后，teamai 会拒绝使用属于另一个 仓库的检出，并打印清除它的 `git worktree remove` 命令：不会向它发布、不会为它建索引（包括其中的投票）、 @@ -379,7 +382,9 @@ git 同一分支只能在一个 worktree 中检出，所以仓库的所有检出 learning 会被移到同一数据目录下的 `pending-learnings.<旧类型>`，新安装不会发布它们；`init` 会说明数量和位置，并删除为旧仓库构建的搜索索引（下一次 `recall` 会重建）。以同一类型对另一个团队仓库重新运行 `init` 也同样处理，队列移到 `pending-learnings.<类型>-<仓库>`（例如 `pending-learnings.git-github.com-org-team-a`）； -同一仓库换一种写法（带或不带 `.git`、SSH 或 HTTPS）不会移动队列。若旧安装的 `config.yaml` 存在但无法读取， +同一仓库换一种写法（带或不带 `.git`、SSH 或 HTTPS）不会移动队列。克隆另一个团队仓库之前， +`init` 会把旧的 `config.yaml` 移到旁边的 `config.yaml.previous` 并给出提示：若 `init` 在保存新配置前停止， +所有命令都会要求先运行 `teamai init`，而不会用旧团队的配置操作新的克隆。重新运行 `init` 即可。若旧安装的 `config.yaml` 存在但无法读取， 就无从得知队列属于谁：`init` 会把它移到 `pending-learnings.unknown`，指出该文件，并删除搜索索引。尚未升级的检出中的旧队列也同样处理： 在该检出运行的下一个命令会把它移到 `pending-learnings.self` 并给出路径。反过来，当某个检出的 `init --self` 把 git 模式项目切换为单仓库模式，而另一个仍保留旧安装的检出从 main 取得了知识时，在那里运行的下一个 @@ -1144,6 +1149,8 @@ teamai recall maintenance --update-quality 另一个 teamai 命令持有 learnings 或 reports checkout 的锁时，`recall maintenance` 与 `recall promote` 会以退出码 1 停止，不写入任何内容（`The learnings checkout is locked: …`）。待该命令结束后再运行。 +maintenance 与 promote 只发布它们改动过的 learning。learnings 检出中无人提交的文件不会进入它们的提交。 + ### 晋升 Learnings 当 learning 达到成熟标准时，可将其晋升为正式团队知识（skill、rule 或 doc）。晋升判据：置信度 ≥ 0.90、≥ 5 次 upvote、≥ 2 个不同贡献者、存在时长 ≥ 14 天。 diff --git a/src/__tests__/e2e/learnings-remnants-823.test.ts b/src/__tests__/e2e/learnings-remnants-823.test.ts new file mode 100644 index 000000000..26a3cb3a0 --- /dev/null +++ b/src/__tests__/e2e/learnings-remnants-823.test.ts @@ -0,0 +1,275 @@ +/** + * E2E (#823 item 7, and the maintenance sweep beside it): git mode, user scope. + * + * `import --from-mr` (0.25.0 to 0.26.0-beta.3) wrote its learning into the learnings checkout and + * never committed it. Nothing published it, and `recall maintenance` staged + * all of `learnings/`, so the first maintenance commit swept it, or any other + * file nobody meant to share, into a commit about something else. + */ +import { afterEach, beforeAll, describe, expect, it } from 'vitest'; +import { execFileSync, spawn } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const ROOT = path.resolve(__dirname, '..', '..', '..'); +const CLI = path.join(ROOT, 'dist', 'index.js'); + +const GIT_ENV = { + GIT_AUTHOR_NAME: 'TeamAI CI', + GIT_AUTHOR_EMAIL: 'ci@teamai.test', + GIT_COMMITTER_NAME: 'TeamAI CI', + GIT_COMMITTER_EMAIL: 'ci@teamai.test', +}; + +interface RunResult { + code: number | null; + output: string; +} + +function git(args: string[], cwd: string): string { + return execFileSync('git', args, { cwd, encoding: 'utf8', env: { ...process.env, ...GIT_ENV } }); +} + +const sandboxes: string[] = []; + +/** A learning file with frontmatter. */ +function learning(title: string, extra: string[] = []): string { + return ['---', `title: "${title}"`, 'date: 2026-09-20', 'confidence: 0.5', ...extra, '---', `# ${title}`, ''].join('\n'); +} + +/** + * A user-scope git install on a local bare team repo, with teamai-learnings + * and teamai-reports already on origin and one `teamai pull` done. With + * `project`, the install has that project active and it owns the learnings + * namespace of the same name. + */ +async function setUp(opts: { + project?: string; + mainLearnings?: Record<string, string>; + branchLearnings?: Record<string, string>; + votes?: string; +} = {}) { + const sandbox = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-issue823-remnants-'))); + sandboxes.push(sandbox); + const home = path.join(sandbox, 'home'); + const remote = path.join(sandbox, 'team.git'); + const clone = path.join(home, '.teamai', 'team-repo'); + const checkout = path.join(home, '.teamai', 'learnings-wt'); + fs.mkdirSync(path.join(home, '.claude'), { recursive: true }); + + const seed = path.join(sandbox, 'seed'); + fs.mkdirSync(seed); + fs.writeFileSync(path.join(seed, 'teamai.yaml'), 'team: issue-823\nrepo: https://github.com/acme/team\nprovider: github\n'); + if (opts.project) { + fs.mkdirSync(path.join(seed, 'manifest')); + fs.writeFileSync(path.join(seed, 'manifest', 'projects.yaml'), [ + 'version: 1', + 'projects:', + ` - id: ${opts.project}`, + ` name: ${opts.project}`, + ' resources:', + ` learnings: [${opts.project}]`, + '', + ].join('\n')); + } + for (const [name, content] of Object.entries(opts.mainLearnings ?? {})) { + fs.mkdirSync(path.join(seed, 'learnings'), { recursive: true }); + fs.writeFileSync(path.join(seed, 'learnings', name), content); + } + git(['init', '-q', '-b', 'main'], seed); + git(['add', '-A'], seed); + git(['commit', '-q', '-m', 'seed'], seed); + + git(['checkout', '-q', '--orphan', 'teamai-learnings'], seed); + git(['rm', '-rfq', '.'], seed); + fs.writeFileSync(path.join(seed, '.gitignore'), 'reports-wt/\nlearnings-wt/\nknowledge-wt/\n'); + fs.mkdirSync(path.join(seed, 'learnings'), { recursive: true }); + fs.writeFileSync(path.join(seed, 'learnings', '.gitkeep'), ''); + for (const [name, content] of Object.entries(opts.branchLearnings ?? {})) { + fs.mkdirSync(path.dirname(path.join(seed, 'learnings', name)), { recursive: true }); + fs.writeFileSync(path.join(seed, 'learnings', name), content); + } + git(['add', '-A'], seed); + git(['commit', '-q', '-m', 'learnings branch'], seed); + + git(['checkout', '-q', '--orphan', 'teamai-reports'], seed); + git(['rm', '-rfq', '.'], seed); + fs.writeFileSync(path.join(seed, '.gitignore'), 'reports-wt/\nlearnings-wt/\nknowledge-wt/\n'); + if (opts.votes) { + fs.mkdirSync(path.join(seed, 'votes')); + fs.writeFileSync(path.join(seed, 'votes', 'teammate.yaml'), opts.votes); + } + git(['add', '-A'], seed); + git(['commit', '-q', '-m', 'reports branch'], seed); + git(['checkout', '-q', 'main'], seed); + + git(['clone', '-q', '--bare', seed, remote], sandbox); + git(['clone', '-q', remote, clone], sandbox); + fs.writeFileSync(path.join(home, '.teamai', 'config.yaml'), [ + 'repo:', + ` localPath: ${clone}`, + ` remote: ${remote}`, + ' kind: git', + 'username: ci-823', + 'updatePolicy: auto', + 'scope: user', + 'enabledAgents: [claude]', + ...(opts.project ? [`projects: [${opts.project}]`] : []), + '', + ].join('\n')); + + const run = (args: string[]): Promise<RunResult> => new Promise((resolve) => { + const { CLAUDE_SESSION_ID: _s, GITHUB_TOKEN: _t, GH_TOKEN: _g, ...env } = process.env; + const child = spawn('node', [CLI, ...args], { + cwd: home, + env: { ...env, ...GIT_ENV, HOME: home, USERPROFILE: home, FORCE_COLOR: '0', NO_COLOR: '1', TEAMAI_CONTRIBUTE_HINT_DISABLED: '1' }, + stdio: ['ignore', 'pipe', 'pipe'], + }); + let output = ''; + child.stdout.on('data', (d: Buffer) => { output += d.toString(); }); + child.stderr.on('data', (d: Buffer) => { output += d.toString(); }); + child.on('close', (code) => resolve({ code, output })); + }); + + const pull = await run(['pull']); + expect(pull.code, pull.output).toBe(0); + expect(fs.existsSync(path.join(checkout, '.git')), pull.output).toBe(true); + + const published = (): string[] => + git(['ls-tree', '-r', '--name-only', 'teamai-learnings'], remote).split('\n').filter((f) => f.endsWith('.md')); + const show = (file: string): string => git(['show', `teamai-learnings:${file}`], remote); + const status = (): string => git(['status', '--porcelain', '--untracked-files=all'], checkout); + return { home, remote, checkout, run, published, show, status }; +} + +/** What an older import --from-mr wrote: `<date>-<title>.md`, with the MR in its frontmatter, never committed. */ +const REMNANT = [ + '---', + 'title: "Quokka cache warmup before deploy"', + 'author: dev', + 'date: 2026-09-20', + 'tags: [cache, deploy]', + 'confidence: 0.85', + 'source_mr: "https://github.com/acme/app/pull/42"', + '---', + '## Background', + 'The quokka cache is cold after each deploy.', + '', +].join('\n'); +const REMNANT_NAME = '2026-09-20-Quokka-cache-warmup-before-deploy.md'; + +describe('learnings the checkout holds but never committed (#823)', () => { + beforeAll(() => { + if (!fs.existsSync(CLI)) throw new Error(`CLI binary not found at ${CLI}. Run "npm run build" first.`); + }); + + afterEach(() => { + for (const sandbox of sandboxes.splice(0)) fs.rmSync(sandbox, { recursive: true, force: true }); + }); + + it("publishes an older import --from-mr learning on the next pull, in the project's namespace, and leaves the checkout clean (item 7)", async () => { + const s = await setUp({ project: 'alpha' }); + const file = path.join(s.checkout, 'learnings', REMNANT_NAME); + fs.writeFileSync(file, REMNANT); + + const pull = await s.run(['pull']); + + expect(pull.code, pull.output).toBe(0); + const queued = s.published().filter((f) => /^learnings\/alpha\/quokka-cache-warmup-before-deploy-\d{4}-\d{2}-\d{2}-[a-z0-9]+\.md$/.test(f)); + expect(queued, `${s.published().join('\n')}\n${pull.output}`).toHaveLength(1); + expect(s.show(queued[0])).toBe(REMNANT); + expect(s.status()).toBe(''); + expect(pull.output).toContain(`Queued 1 learning(s) an older teamai import --from-mr left unpublished: ${file}`); + }); + + it('publishes only what confidence write-back changed, never a file nobody committed (maintenance sweep)', async () => { + const recent = new Date().toISOString(); + const vote = (doc: string) => [ + ` ${doc}:`, + ' recalled_count: 9', + ' upvoted_count: 6', + ` last_recalled_at: ${recent}`, + ` last_upvoted_at: ${recent}`, + ]; + const s = await setUp({ + mainLearnings: { 'inherited-note.md': learning('Inherited note') }, + branchLearnings: { 'tracked-note.md': learning('Tracked note') }, + votes: ['version: 2', 'votes:', ...vote('tracked-note'), ...vote('inherited-note'), ''].join('\n'), + }); + // A draft nobody meant to share. It has no votes, so maintenance never touches it. + const stray = path.join(s.checkout, 'learnings', 'stray-draft.md'); + fs.writeFileSync(stray, learning('Stray draft')); + + const maintenance = await s.run(['recall', 'maintenance', '--confidence-writeback']); + + expect(maintenance.code, maintenance.output).toBe(0); + expect(maintenance.output).toContain('Published maintenance changes to the learnings branch'); + // The tracked learning it rewrote, and the new copy of the inherited one it wrote into the checkout. + expect(s.show('learnings/tracked-note.md')).not.toContain('confidence: 0.5'); + expect(s.show('learnings/inherited-note.md')).not.toContain('confidence: 0.5'); + expect(s.published()).not.toContain('learnings/stray-draft.md'); + expect(fs.readFileSync(stray, 'utf8')).toBe(learning('Stray draft')); + }); + + it('publishes an archived prune, and still not a file nobody committed (maintenance sweep)', async () => { + const s = await setUp({ + branchLearnings: { 'low-note.md': learning('Low note') }, + votes: ['version: 2', 'votes:', ' low-note:', ' recalled_count: 1', ' upvoted_count: 0', + ' last_recalled_at: 2020-01-01T00:00:00.000Z', ''].join('\n'), + }); + const stray = path.join(s.checkout, 'learnings', 'stray-draft.md'); + fs.writeFileSync(stray, learning('Stray draft')); + + const prune = await s.run(['recall', 'maintenance', '--prune', '--archive']); + + expect(prune.code, prune.output).toBe(0); + expect(prune.output).toContain('Published maintenance changes to the learnings branch'); + expect(s.published()).toContain('learnings/_archive/low-note.md'); + expect(s.published()).not.toContain('learnings/low-note.md'); + expect(s.published()).not.toContain('learnings/stray-draft.md'); + expect(fs.existsSync(stray)).toBe(true); + }); +}); + +describe('pull --dry-run and the contribution queue (#823 item 20)', () => { + beforeAll(() => { + if (!fs.existsSync(CLI)) throw new Error(`CLI binary not found at ${CLI}. Run "npm run build" first.`); + }); + + afterEach(() => { + for (const sandbox of sandboxes.splice(0)) fs.rmSync(sandbox, { recursive: true, force: true }); + }); + + it('says what it would publish and publishes nothing, leaving the learning queued for the next pull', async () => { + const s = await setUp(); + const note = path.join(s.home, 'otter.md'); + fs.writeFileSync(note, '# Otter note\nOtters hold hands while they sleep.\n'); + // Origin rejects the push, so the contribution stays queued. + const hook = path.join(s.remote, 'hooks', 'pre-receive'); + fs.writeFileSync(hook, '#!/bin/sh\necho "rejected by the #823 test" >&2\nexit 1\n', { mode: 0o755 }); + const contribute = await s.run(['contribute', '--title', 'otter', '--file', note]); + fs.rmSync(hook); + expect(contribute.output).toContain('Saved locally'); + const queue = path.join(s.home, '.teamai', 'pending-learnings'); + const queued = fs.readdirSync(queue).filter((f) => f.endsWith('.md')); + expect(queued, contribute.output).toHaveLength(1); + const before = s.published(); + + const dryRun = await s.run(['pull', '--dry-run']); + + expect(dryRun.code, dryRun.output).toBe(0); + expect(s.published(), dryRun.output).toEqual(before); + expect(fs.readdirSync(queue).filter((f) => f.endsWith('.md'))).toEqual(queued); + expect(dryRun.output).toContain('[dry-run] Would publish 1 queued learning(s)'); + expect(dryRun.output).not.toContain('Published 1 queued learning(s)'); + expect(dryRun.output).not.toContain('written locally but not published'); + + const pull = await s.run(['pull']); + + expect(pull.output).toContain('Published 1 queued learning(s)'); + expect(s.published()).toContain(`learnings/${queued[0]}`); + }); +}); diff --git a/src/__tests__/e2e/self-mode-worktrees-808.test.ts b/src/__tests__/e2e/self-mode-worktrees-808.test.ts index 8ef3f4fe1..bc656a51c 100644 --- a/src/__tests__/e2e/self-mode-worktrees-808.test.ts +++ b/src/__tests__/e2e/self-mode-worktrees-808.test.ts @@ -35,11 +35,11 @@ interface RunResult { output: string; } -function runCLI(args: string[], cwd: string, home: string): Promise<RunResult> { +function runCLI(args: string[], cwd: string, home: string, env: NodeJS.ProcessEnv = {}): Promise<RunResult> { return new Promise((resolve) => { const child = spawn('node', [CLI, ...args], { cwd, - env: { ...process.env, ...GIT_ENV, HOME: home, FORCE_COLOR: '0' }, + env: { ...process.env, ...GIT_ENV, HOME: home, FORCE_COLOR: '0', ...env }, stdio: ['ignore', 'pipe', 'pipe'], }); let output = ''; @@ -368,6 +368,55 @@ describe('self mode with a linked worktree (#808)', () => { expect(published(project).some((f) => f.includes('unmigrated-note-')), published(project).join('\n')).toBe(true); }); + it('moves the queue an older teamai left in a linked worktree before import --from-mr publishes, so removing the worktree loses nothing', async () => { + const project = setUpProject(); + const { home, projectRoot, worktree } = project; + // The main checkout migrated; the worktree still holds the queue an older + // teamai kept in its own .teamai/, with no config beside it. + expect((await runCLI(['pull'], projectRoot, home)).code).toBe(0); + git(['worktree', 'add', '-q', worktree, '-b', 'wt-808'], projectRoot); + const legacy = 'legacy-wt-2026-01-01-bbbbbb.md'; + fs.mkdirSync(path.join(worktree, '.teamai', 'pending-learnings'), { recursive: true }); + fs.writeFileSync(path.join(worktree, '.teamai', 'pending-learnings', legacy), '# Queued in the worktree by an older teamai\n'); + // `gh` (the MR) and `claude` (the extraction) are stand-ins on PATH. + const bin = path.join(project.sandbox, 'bin'); + fs.mkdirSync(bin); + fs.writeFileSync(path.join(bin, 'gh'), [ + '#!/usr/bin/env bash', + 'case "$1 $2" in', + ` "pr view") echo '{"title":"Retry flaky upload","body":"Retries uploads.","author":{"login":"dev"},"mergedAt":"2026-09-20T10:00:00Z","commits":[]}' ;;`, + " \"pr diff\") printf 'diff --git a/up.ts b/up.ts\\n+retry(3)\\n' ;;", + ' *) exit 1 ;;', + 'esac', + '', + ].join('\n'), { mode: 0o755 }); + fs.writeFileSync(path.join(bin, 'claude'), [ + '#!/usr/bin/env bash', + "cat <<'MD'", + '---', + 'title: Retry flaky uploads', + '---', + '# Retry flaky uploads', + '', + 'Wrap uploads in retry(3).', + 'MD', + '', + ].join('\n'), { mode: 0o755 }); + + const imported = await runCLI( + ['import', '--from-mr', 'https://github.com/acme/app/pull/7', '--all'], + worktree, + home, + { PATH: `${bin}${path.delimiter}${process.env.PATH ?? ''}`, CLAUDE_SESSION_ID: '', GITHUB_TOKEN: '', GH_TOKEN: '' }, + ); + + expect(imported.code, imported.output).toBe(0); + git(['worktree', 'remove', '--force', worktree], projectRoot); + expect(published(project), `${published(project).join('\n')}\n${imported.output}`).toContain(`learnings/${legacy}`); + expect(published(project).some((f) => f.includes('retry-flaky-uploads-')), published(project).join('\n')).toBe(true); + expect(partitionQueue(project)).toEqual([]); + }); + it('stops contribute in an unmigrated linked worktree while another command holds its sync lock, saving nothing there', async () => { const project = setUpProject(); const { home, projectRoot, worktree } = project; @@ -639,6 +688,107 @@ describe('self mode with a linked worktree (#808)', () => { expect(published(project)).toContain('learnings/legacy-wt-2026-01-01-bbbbbb.md'); }); + it("publishes the learning an older import --from-mr left in the old checkout, so git can remove it and contribute publishes both (#823 item 7)", async () => { + const project = setUpProject(); + const { home, remote, projectRoot } = project; + const seed = path.join(project.sandbox, 'seed'); + git(['clone', '-q', remote, seed], project.sandbox); + git(['checkout', '-q', '--orphan', 'teamai-learnings'], seed); + git(['rm', '-rfq', '.'], seed); + fs.mkdirSync(path.join(seed, 'learnings'), { recursive: true }); + fs.writeFileSync(path.join(seed, '.gitignore'), 'reports-wt/\nlearnings-wt/\nknowledge-wt/\n'); + fs.writeFileSync(path.join(seed, 'learnings', '.gitkeep'), ''); + git(['add', '-A'], seed); + git(['commit', '-q', '-m', 'learnings branch'], seed); + git(['push', '-q', 'origin', 'teamai-learnings'], seed); + git(['fetch', '-q', 'origin', 'teamai-learnings:teamai-learnings'], projectRoot); + // The checkout an older teamai kept in .teamai/, holding what an older + // import --from-mr wrote there and never committed. + const oldCheckout = path.join(projectRoot, '.teamai', 'learnings-wt'); + git(['worktree', 'add', '-q', oldCheckout, 'teamai-learnings'], projectRoot); + const remnant = [ + '---', + 'title: "Quokka cache warmup before deploy"', + 'date: 2026-09-20', + 'source_mr: "https://github.com/acme/app/pull/42"', + '---', + 'Warm the quokka cache in the post-deploy hook.', + '', + ].join('\n'); + fs.writeFileSync(path.join(oldCheckout, 'learnings', '2026-09-20-Quokka-cache-warmup-before-deploy.md'), remnant); + + const contribute = await runCLI( + ['contribute', '--title', 'self-note', '--file', note(project, 'self', 'Note contributed beside the old checkout')], + projectRoot, + home, + ); + + expect(contribute.code, contribute.output).toBe(0); + expect(contribute.output).not.toContain('still has teamai-learnings checked out'); + expect(contribute.output).not.toContain('Saved locally'); + expect(fs.existsSync(oldCheckout)).toBe(false); + expect(published(project).some((f) => f.includes('self-note-')), published(project).join('\n')).toBe(true); + const quokka = published(project).filter((f) => /^learnings\/quokka-cache-warmup-before-deploy-[\d-]+-[a-z0-9]+\.md$/.test(f)); + expect(quokka, published(project).join('\n')).toHaveLength(1); + expect(git(['show', `teamai-learnings:${quokka[0]}`], remote)).toBe(remnant); + expect(partitionQueue(project)).toEqual([]); + }); + + it("removes that learning instead when a teammate already published one from the same merge request the old checkout never fetched (#823 item 21)", async () => { + const project = setUpProject(); + const { home, remote, projectRoot } = project; + const seed = path.join(project.sandbox, 'seed'); + git(['clone', '-q', remote, seed], project.sandbox); + git(['checkout', '-q', '--orphan', 'teamai-learnings'], seed); + git(['rm', '-rfq', '.'], seed); + fs.mkdirSync(path.join(seed, 'learnings'), { recursive: true }); + fs.writeFileSync(path.join(seed, '.gitignore'), 'reports-wt/\nlearnings-wt/\nknowledge-wt/\n'); + fs.writeFileSync(path.join(seed, 'learnings', '.gitkeep'), ''); + git(['add', '-A'], seed); + git(['commit', '-q', '-m', 'learnings branch'], seed); + git(['push', '-q', 'origin', 'teamai-learnings'], seed); + git(['fetch', '-q', 'origin', 'teamai-learnings:teamai-learnings'], projectRoot); + const oldCheckout = path.join(projectRoot, '.teamai', 'learnings-wt'); + git(['worktree', 'add', '-q', oldCheckout, 'teamai-learnings'], projectRoot); + const remnantFile = path.join(oldCheckout, 'learnings', '2026-09-20-Quokka-cache-warmup-before-deploy.md'); + fs.writeFileSync(remnantFile, [ + '---', + 'title: "Quokka cache warmup before deploy"', + 'date: 2026-09-20', + 'source_mr: "https://github.com/acme/app/pull/42"', + '---', + 'Warm the quokka cache in the post-deploy hook.', + '', + ].join('\n')); + // A teammate's later import of the same MR, pushed after this repo last fetched. + const teammates = 'learnings/quokka-cache-warmup-2026-09-21-ttt111.md'; + fs.writeFileSync(path.join(seed, teammates), [ + '---', + 'title: "Quokka cache warmup"', + 'date: 2026-09-21', + 'source_mr: "https://github.com/acme/app/pull/42"', + '---', + 'Warm the quokka cache before traffic returns.', + '', + ].join('\n')); + git(['add', '-A'], seed); + git(['commit', '-q', '-m', 'teammate import'], seed); + git(['push', '-q', 'origin', 'teamai-learnings'], seed); + + const contribute = await runCLI( + ['contribute', '--title', 'self-note', '--file', note(project, 'self', 'Note contributed beside the old checkout')], + projectRoot, + home, + ); + + expect(contribute.code, contribute.output).toBe(0); + expect(contribute.output).toContain(`Removed ${remnantFile}, which an older teamai import --from-mr left unpublished: ${teammates} already has it.`); + expect(fs.existsSync(oldCheckout)).toBe(false); + expect(published(project).filter((f) => f.includes('quokka')), published(project).join('\n')).toEqual([teammates]); + expect(published(project).some((f) => f.includes('self-note-')), published(project).join('\n')).toBe(true); + expect(partitionQueue(project)).toEqual([]); + }); + it("refuses a learnings checkout another repository left in the partition, and publishes nothing to it", async () => { const project = setUpProject(); const { sandbox, home, projectRoot } = project; @@ -1372,6 +1522,53 @@ describe('a checkout an older git-mode install left (#808)', () => { expect(git(['log', '--all', '--name-only', '--format='], teamB.bare)).not.toContain('team-a-note-'); }); + it("leaves no config naming team A beside team B's clone when init stops after recloning, and sets team A's queue aside (#823 item 17)", async () => { + const install = setUpGitInstall(); + const { home, projectRoot } = install; + const teamA = serveTeamRepo(install, 'team-a'); + const teamB = serveTeamRepo(install, 'team-b'); + // Team B has roles, so an unknown --role stops init after it has recloned. + const edit = path.join(install.sandbox, 'team-b-edit'); + git(['clone', '-q', teamB.bare, edit], install.sandbox); + fs.mkdirSync(path.join(edit, 'manifest')); + fs.writeFileSync(path.join(edit, 'manifest', 'roles.yaml'), 'version: 1\nroles:\n - id: backend\n resources:\n knowledge: []\n skills: []\n'); + git(['add', '-A'], edit); + git(['commit', '-q', '-m', 'roles'], edit); + git(['push', '-q', 'origin', 'main'], edit); + + const initA = await runCLI(['init', teamA.url, '--scope', 'project', '--force'], projectRoot, home); + expect(initA.code, initA.output).toBe(0); + const queued = await withPushesRejected({ ...install, remote: teamA.bare }, () => runCLI( + ['contribute', '--title', 'team-a-note', '--file', note(install, 'team-a', 'Note for team A')], + projectRoot, + home, + )); + expect(queued.output).toContain('Saved locally'); + const [name] = partitionQueue(install).filter((f) => f.startsWith('team-a-note-')); + expect(name, partitionQueue(install).join('\n')).toBeDefined(); + + const failed = await runCLI(['init', teamB.url, '--scope', 'project', '--force', '--role', 'ghost'], projectRoot, home); + + expect(failed.code, failed.output).toBe(1); + expect(failed.output).toContain('Unknown role "ghost"'); + const partition = partitionOf(install); + expect(git(['remote', 'get-url', 'origin'], path.join(partition, 'team-repo')).trim()).toBe(teamB.url); + const config = path.join(partition, 'config.yaml'); + expect(fs.existsSync(config) ? fs.readFileSync(config, 'utf8') : '', failed.output).not.toContain(teamA.url); + // Moved aside, not deleted, and the member is told where. + expect(fs.readFileSync(`${config}.previous`, 'utf8')).toContain(teamA.url); + expect(failed.output).toContain(`Moved ${config} to ${config}.previous`); + const aside = path.join(partition, 'pending-learnings.git-git.example.com-team-team-a'); + expect(fs.readFileSync(path.join(aside, name), 'utf8')).toContain('Note for team A'); + expect(partitionQueue(install)).not.toContain(name); + + const initB = await runCLI(['init', teamB.url, '--scope', 'project', '--force', '--role', 'backend'], projectRoot, home); + expect(initB.code, initB.output).toBe(0); + const pull = await runCLI(['pull'], projectRoot, home); + expect(pull.code, pull.output).toBe(0); + expect(git(['log', '--all', '--name-only', '--format='], teamB.bare)).not.toContain('team-a-note-'); + }); + it("sets aside the queue under an unknown owner when init replaces a config it cannot read, and the next pull publishes none of it", async () => { const install = setUpGitInstall(); const { home, projectRoot } = install; diff --git a/src/__tests__/import-mr-learnings-checkout.test.ts b/src/__tests__/import-mr-learnings-checkout.test.ts index 9b3b43400..5d2fd4f21 100644 --- a/src/__tests__/import-mr-learnings-checkout.test.ts +++ b/src/__tests__/import-mr-learnings-checkout.test.ts @@ -26,12 +26,9 @@ const localConfig = { dataHome, }; -/** The config teamai detects: the main checkout unless a test runs in a linked worktree. */ -let detected = localConfig; - vi.mock('../config.js', async (importOriginal) => ({ ...(await importOriginal<typeof import('../config.js')>()), - autoDetectInit: vi.fn(async () => ({ localConfig: detected, teamConfig: { team: 't', repo: remote } })), + autoDetectInit: vi.fn(async () => ({ localConfig, teamConfig: { team: 't', repo: remote } })), })); const IMPORTED = 'mr-note-2026-01-01-aaaaaa.md'; @@ -99,7 +96,6 @@ describe('import --from-mr in self mode (#808)', () => { }); afterEach(() => { - detected = localConfig; fs.rmSync(testRoot, { recursive: true, force: true }); }); @@ -114,26 +110,6 @@ describe('import --from-mr in self mode (#808)', () => { expect(fs.existsSync(path.join(pendingLearningsDir(localConfig), IMPORTED))).toBe(false); }); - it('publishes what an older teamai queued in a linked worktree, so removing the worktree loses nothing', async () => { - const worktree = path.join(testRoot, 'business-wt'); - git(['worktree', 'add', '-q', worktree, '-b', 'wt-808'], businessRoot); - const legacy = 'legacy-wt-2026-01-01-bbbbbb.md'; - const legacyQueue = path.join(worktree, '.teamai', 'pending-learnings'); - fs.mkdirSync(legacyQueue, { recursive: true }); - fs.writeFileSync(path.join(legacyQueue, legacy), '# Queued in the worktree by an older teamai\n'); - detected = { - ...localConfig, - repo: { ...localConfig.repo, localPath: path.join(worktree, '.teamai'), businessRepoRoot: worktree }, - projectRoot: worktree, - }; - - await importCmd({ fromMr: 'https://github.com/o/r/pull/1' }); - - git(['worktree', 'remove', '--force', worktree], businessRoot); - expect(publishedLearnings()).toEqual(expect.arrayContaining([`learnings/${legacy}`, `learnings/${IMPORTED}`])); - expect(fs.readdirSync(pendingLearningsDir(localConfig))).toEqual([]); - }); - it('leaves the learnings checkout alone when the learning goes to --output', async () => { await importCmd({ fromMr: 'https://github.com/o/r/pull/1', output: path.join(testRoot, 'drafts') }); diff --git a/src/__tests__/learnings-import-remnants.test.ts b/src/__tests__/learnings-import-remnants.test.ts new file mode 100644 index 000000000..a5185f16b --- /dev/null +++ b/src/__tests__/learnings-import-remnants.test.ts @@ -0,0 +1,279 @@ +/** + * Learnings an older `import --from-mr` (0.25.0 to 0.26.0-beta.3) wrote into the learnings + * checkout and never committed (#823 item 7). They were recallable on this + * machine, never reached the team, and in single-repo mode they keep git from + * removing the old checkout. Publishing now moves them into the queue first. + * + * Real git, no mocks of the units under test, as in git-kind-learnings.test.ts. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { simpleGit } from 'simple-git'; + +import { getDataHome, SYNC_LOCK_FILENAME, type LocalConfig } from '../types.js'; +import { learningsBranch } from '../utils/learnings-branch.js'; +import { listPendingLearnings, savePendingLearning } from '../utils/pending-learnings.js'; +import { publishLearningsMaintenance, publishQueuedLearnings } from '../utils/learnings-publish.js'; +import { log } from '../utils/logger.js'; +import { writeInstallConfig } from './helpers/install-config.js'; + +let tmp: string; +let originalHome: string; + +beforeEach(() => { + // Real path: git lists worktrees by it, and the warnings name those paths. + tmp = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-import-remnants-'))); + originalHome = process.env.HOME ?? ''; + process.env.HOME = path.join(tmp, 'home'); + fs.mkdirSync(process.env.HOME, { recursive: true }); +}); + +afterEach(() => { + vi.restoreAllMocks(); + process.env.HOME = originalHome; + fs.rmSync(tmp, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); +}); + +async function configureGit(dir: string): Promise<void> { + const git = simpleGit(dir); + await git.addConfig('user.email', 't@t.com'); + await git.addConfig('user.name', 't'); +} + +/** A team repo, a member's clone, and the learnings checkout a first publish created. */ +async function setUp(): Promise<{ config: LocalConfig; origin: string; checkout: string }> { + const seed = path.join(tmp, 'seed'); + fs.mkdirSync(seed, { recursive: true }); + const seedGit = simpleGit(seed); + await seedGit.init(['--initial-branch=main']); + await configureGit(seed); + fs.writeFileSync(path.join(seed, 'teamai.yaml'), 'team: acme\n'); + await seedGit.add(['.']); + await seedGit.commit('init'); + const origin = path.join(tmp, 'origin.git'); + await simpleGit().clone(seed, origin, ['--bare']); + const clone = path.join(tmp, 'team-repo'); + await simpleGit().clone(origin, clone); + await configureGit(clone); + + const config: LocalConfig = { + repo: { localPath: clone, remote: origin, kind: 'git' }, + username: 'alice', + scope: 'user', + additionalRoles: [], + }; + writeInstallConfig(config); + const checkout = await learningsBranch.ensure(config); + await configureGit(checkout); + return { config, origin, checkout }; +} + +async function publishedFiles(origin: string): Promise<string[]> { + try { + return (await simpleGit(origin).raw(['ls-tree', '-r', '--name-only', 'teamai-learnings'])).split('\n').filter(Boolean); + } catch { + return []; + } +} + +async function publishedContent(origin: string, file: string): Promise<string> { + return simpleGit(origin).raw(['show', `teamai-learnings:${file}`]); +} + +/** What an older import --from-mr wrote: `<date>-<title>.md` with the MR in its frontmatter. */ +function remnant(mr = 'https://github.com/acme/app/pull/42', title = 'Quokka cache warmup before deploy'): string { + return [ + '---', + `title: "${title}"`, + 'author: dev', + 'date: 2026-09-20', + 'tags: [cache, deploy]', + 'confidence: 0.85', + `source_mr: "${mr}"`, + '---', + '## Background', + 'The quokka cache is cold after each deploy.', + '', + ].join('\n'); +} + +function plant(checkout: string, relPath: string, content: string): string { + const file = path.join(checkout, 'learnings', relPath); + fs.mkdirSync(path.dirname(file), { recursive: true }); + fs.writeFileSync(file, content); + return file; +} + +/** What was warned from now on, one line per call. */ +function watchWarnings(): () => string { + const warn = vi.spyOn(log, 'warn'); + return () => warn.mock.calls.map((c) => String(c[0])).join('\n'); +} + +const QUEUED_NAME = /^learnings\/quokka-cache-warmup-before-deploy-\d{4}-\d{2}-\d{2}-[a-z0-9]+\.md$/; + +describe('a learning an older import --from-mr left untracked in the learnings checkout (#823 item 7)', () => { + it('is queued and published even when nothing else is queued, and leaves the checkout clean', async () => { + const { config, origin, checkout } = await setUp(); + const file = plant(checkout, '2026-09-20-Quokka-cache-warmup-before-deploy.md', remnant()); + const warned = watchWarnings(); + + const report = await publishQueuedLearnings(config, 'alice'); + + const published = (await publishedFiles(origin)).filter((f) => QUEUED_NAME.test(f)); + expect(published, (await publishedFiles(origin)).join('\n')).toHaveLength(1); + expect(await publishedContent(origin, published[0])).toBe(remnant()); + expect(report.published).toHaveLength(1); + expect(fs.existsSync(file)).toBe(false); + expect(await simpleGit(checkout).raw(['status', '--porcelain'])).toBe(''); + expect(await listPendingLearnings(config)).toEqual([]); + expect(warned()).toContain(`Queued 1 learning(s) an older teamai import --from-mr left unpublished: ${file}`); + }); + + it('is queued too when its title kept no character in the file name', async () => { + const { config, origin, checkout } = await setUp(); + // A Cyrillic title: that import replaced every character, leaving `<date>-.md`. + const file = plant(checkout, '2026-09-20-.md', remnant('https://github.com/acme/app/pull/5', 'Прогрев кэша')); + + await publishQueuedLearnings(config, 'alice'); + + expect(fs.existsSync(file)).toBe(false); + const published = (await publishedFiles(origin)).filter((f) => f.endsWith('.md')); + expect(published).toHaveLength(1); + expect(await publishedContent(origin, published[0])).toBe(remnant('https://github.com/acme/app/pull/5', 'Прогрев кэша')); + }); + + it('leaves every other file alone: tracked edits, subdirectories, and files without the date name or source_mr', async () => { + const { config, origin, checkout } = await setUp(); + // A tracked learning the member edited by hand. + await savePendingLearning(config, '2026-09-19-tracked.md', remnant('https://github.com/acme/app/pull/1', 'Tracked')); + await publishQueuedLearnings(config, 'alice'); + const tracked = path.join(checkout, 'learnings', '2026-09-19-tracked.md'); + fs.appendFileSync(tracked, 'An edit nobody committed.\n'); + const kept = [ + plant(checkout, 'alpha/2026-09-20-in-a-namespace.md', remnant('https://github.com/acme/app/pull/2')), + plant(checkout, '2026-09-20-no-merge-request.md', '---\ntitle: No MR\n---\nA draft.\n'), + plant(checkout, 'notes-with-a-merge-request.md', remnant('https://github.com/acme/app/pull/3')), + ]; + const before = await publishedFiles(origin); + + await publishQueuedLearnings(config, 'alice'); + + expect(await publishedFiles(origin)).toEqual(before); + for (const file of kept) expect(fs.existsSync(file), file).toBe(true); + expect(fs.readFileSync(tracked, 'utf8')).toContain('An edit nobody committed.'); + expect(await listPendingLearnings(config)).toEqual([]); + }); + + it('is removed, not queued again, when the branch already has a learning from the same merge request', async () => { + const { config, origin, checkout } = await setUp(); + // A later import of the same MR, published in a project namespace. + await savePendingLearning(config, 'alpha/quokka-2026-09-21-aaa111.md', remnant('https://github.com/acme/app/pull/42', 'Quokka, reimported')); + await publishQueuedLearnings(config, 'alice'); + const file = plant(checkout, '2026-09-20-Quokka-cache-warmup-before-deploy.md', remnant()); + const before = await publishedFiles(origin); + const warned = watchWarnings(); + + await publishQueuedLearnings(config, 'alice'); + + expect(await publishedFiles(origin)).toEqual(before); + expect(fs.existsSync(file)).toBe(false); + expect(await listPendingLearnings(config)).toEqual([]); + expect(warned()).toContain(file); + expect(warned()).toContain('learnings/alpha/quokka-2026-09-21-aaa111.md'); + }); + + it('is removed, not queued again, when a teammate published one from the same merge request that the checkout has not fetched yet (item 21)', async () => { + const { config, origin, checkout } = await setUp(); + await savePendingLearning(config, 'first-2026-09-19-aaa000.md', '---\ntitle: First\n---\nFirst.\n'); + await publishQueuedLearnings(config, 'alice'); + // A teammate imports the same MR from their own clone. Neither this + // checkout nor this clone's origin/teamai-learnings has seen it. + const teammate = path.join(tmp, 'teammate'); + await simpleGit().clone(origin, teammate, ['--branch', 'teamai-learnings']); + await configureGit(teammate); + fs.mkdirSync(path.join(teammate, 'learnings', 'alpha'), { recursive: true }); + fs.writeFileSync(path.join(teammate, 'learnings', 'alpha', 'quokka-2026-09-21-ddd444.md'), remnant('https://github.com/acme/app/pull/42', 'Quokka, by a teammate')); + await simpleGit(teammate).add(['.']); + await simpleGit(teammate).commit('teammate'); + await simpleGit(teammate).push('origin', 'teamai-learnings'); + const file = plant(checkout, '2026-09-20-Quokka-cache-warmup-before-deploy.md', remnant()); + const before = await publishedFiles(origin); + const warned = watchWarnings(); + + await publishQueuedLearnings(config, 'alice'); + + expect(await publishedFiles(origin)).toEqual(before); + expect(fs.existsSync(file)).toBe(false); + expect(await listPendingLearnings(config)).toEqual([]); + expect(warned()).toContain('learnings/alpha/quokka-2026-09-21-ddd444.md'); + }); + + it('is removed, and published once, when the queue already holds the same content', async () => { + const { config, origin, checkout } = await setUp(); + await savePendingLearning(config, 'quokka-copy-2026-09-21-bbb222.md', remnant('https://github.com/acme/app/pull/77')); + const file = plant(checkout, '2026-09-20-Quokka-cache-warmup-before-deploy.md', remnant('https://github.com/acme/app/pull/77')); + + await publishQueuedLearnings(config, 'alice'); + + const published = (await publishedFiles(origin)).filter((f) => f.endsWith('.md')); + expect(published).toEqual(['learnings/quokka-copy-2026-09-21-bbb222.md']); + expect(fs.existsSync(file)).toBe(false); + }); + + it('is left where it is while another command holds the sync lock, which would queue it twice', async () => { + const { config, origin, checkout } = await setUp(); + const file = plant(checkout, '2026-09-20-Quokka-cache-warmup-before-deploy.md', remnant()); + const syncLock = path.join(getDataHome(config), SYNC_LOCK_FILENAME); + fs.writeFileSync(syncLock, JSON.stringify({ pid: process.pid, startedAt: new Date().toISOString(), owner: 'a pull' })); + + await publishQueuedLearnings(config, 'alice'); + + expect(fs.existsSync(file)).toBe(true); + expect(await listPendingLearnings(config)).toEqual([]); + expect((await publishedFiles(origin)).filter((f) => QUEUED_NAME.test(f))).toEqual([]); + }); + + it('is left where it is by a dry run', async () => { + const { config, origin, checkout } = await setUp(); + const file = plant(checkout, '2026-09-20-Quokka-cache-warmup-before-deploy.md', remnant()); + + await publishQueuedLearnings(config, 'alice', { dryRun: true }); + + expect(fs.existsSync(file)).toBe(true); + expect((await publishedFiles(origin)).filter((f) => QUEUED_NAME.test(f))).toEqual([]); + expect(await listPendingLearnings(config)).toEqual([]); + }); +}); + +describe('a dry run (#823 item 20)', () => { + it('publishes nothing from the queue and reports it as still queued', async () => { + const { config, origin } = await setUp(); + await savePendingLearning(config, 'otter-2026-09-25-ccc333.md', '---\ntitle: Otter\n---\nOtters hold hands.\n'); + + const report = await publishQueuedLearnings(config, 'alice', { dryRun: true }); + + expect(report).toEqual({ published: [], remaining: 1 }); + expect((await publishedFiles(origin)).filter((f) => f.endsWith('.md'))).toEqual([]); + expect(await listPendingLearnings(config)).toEqual(['otter-2026-09-25-ccc333.md']); + }); +}); + +describe('publishing what maintenance changed (#823)', () => { + it('publishes the tracked change, and neither fails on nor publishes a removed file git never tracked', async () => { + const { config, origin, checkout } = await setUp(); + await savePendingLearning(config, 'kept-2026-01-01-aaa111.md', '---\ntitle: Kept\nconfidence: 0.5\n---\nKept.\n'); + await publishQueuedLearnings(config, 'alice'); + // Maintenance rewrote a tracked learning and pruned one nobody had committed. + const rewritten = path.join(checkout, 'learnings', 'kept-2026-01-01-aaa111.md'); + fs.writeFileSync(rewritten, '---\ntitle: Kept\nconfidence: 0.9\n---\nKept.\n'); + const prunedUntracked = path.join(checkout, 'learnings', 'draft.md'); + + const result = await publishLearningsMaintenance(config, '[teamai] Maintenance', [rewritten, prunedUntracked]); + + expect(result).toEqual({ status: 'published' }); + expect(await publishedContent(origin, 'learnings/kept-2026-01-01-aaa111.md')).toContain('confidence: 0.9'); + }); +}); diff --git a/src/__tests__/maintenance-promote.test.ts b/src/__tests__/maintenance-promote.test.ts index 04a0ff15b..2328c4daa 100644 --- a/src/__tests__/maintenance-promote.test.ts +++ b/src/__tests__/maintenance-promote.test.ts @@ -133,8 +133,9 @@ describe('executePromotion', () => { suggestedCategory: 'skills' as const, }; - const targetPath = await executePromotion(candidate, tmpDir, { dryRun: true }); + const { targetPath, marked } = await executePromotion(candidate, tmpDir, { dryRun: true }); expect(targetPath).toContain('skills/candidate.md'); + expect(marked).toBeNull(); expect(fs.existsSync(path.join(tmpDir, 'skills', 'candidate.md'))).toBe(false); }); @@ -155,7 +156,8 @@ describe('executePromotion', () => { suggestedCategory: 'skills' as const, }; - await executePromotion(candidate, tmpDir, { category: 'skills' }); + const { marked } = await executePromotion(candidate, tmpDir, { category: 'skills' }); + expect(marked).toBe(path.join(learningsDir, 'ready.md')); expect(fs.existsSync(path.join(skillsDir, 'ready.md'))).toBe(true); diff --git a/src/__tests__/maintenance-two-roots.test.ts b/src/__tests__/maintenance-two-roots.test.ts index 25a8c0dce..3ee8286db 100644 --- a/src/__tests__/maintenance-two-roots.test.ts +++ b/src/__tests__/maintenance-two-roots.test.ts @@ -39,13 +39,13 @@ describe('maintenance across the write root and the inherited root', () => { it('writes an inherited learning back into the write root, leaving the clone untouched', async () => { fs.writeFileSync(path.join(inherited, 'old.md'), '---\ntitle: old\nconfidence: 0.1\n---\nbody'); - const updated = await writeBackConfidence( + const written = await writeBackConfidence( [writeRoot, inherited], new Map([['old', 0.9]]), writeRoot, ); - expect(updated).toBe(1); + expect(written).toEqual([path.join(writeRoot, 'old.md')]); expect(fs.readFileSync(path.join(writeRoot, 'old.md'), 'utf8')).toContain('confidence: 0.9'); expect(fs.readFileSync(path.join(inherited, 'old.md'), 'utf8')).toContain('confidence: 0.1'); }); @@ -69,7 +69,7 @@ describe('maintenance across the write root and the inherited root', () => { reason: 'test', }]); - expect(result).toEqual({ archived: 0, removed: 0 }); + expect(result).toEqual({ archived: 0, removed: 0, changed: [] }); expect(fs.existsSync(path.join(inherited, 'old.md'))).toBe(true); expect(vi.mocked(log.warn).mock.calls.join(' ')).toContain('pull request'); }); @@ -86,6 +86,7 @@ describe('maintenance across the write root and the inherited root', () => { }], { archive: true }); expect(result.archived).toBe(1); + expect(result.changed).toEqual([path.join(writeRoot, 'stale.md'), path.join(writeRoot, '_archive', 'stale.md')]); expect(fs.existsSync(path.join(writeRoot, '_archive', 'stale.md'))).toBe(true); expect(fs.existsSync(path.join(writeRoot, 'stale.md'))).toBe(false); }); diff --git a/src/__tests__/migrate.test.ts b/src/__tests__/migrate.test.ts index 8fb2c0048..307e06e5e 100644 --- a/src/__tests__/migrate.test.ts +++ b/src/__tests__/migrate.test.ts @@ -389,6 +389,22 @@ describe('runMigration', () => { expect(await planMigration(repoRoot)).toBeNull(); }); + it('says where the data already lives when it retires a leftover, without claiming a migration was interrupted (#823)', async () => { + // A linked worktree whose checkout never ran teamai lands here too: another + // checkout built the partition, nothing was interrupted. + await seedLegacyLayout(); + const partition = projectDataHome(repoRoot); + await writePartitionConfig(partition); + vi.mocked(log.success).mockClear(); + + const plan = await planMigration(repoRoot); + if (plan?.mode !== 'retire-only') throw new Error('expected a retire-only plan'); + expect(await runMigration(plan)).toBe('migrated'); + + const said = vi.mocked(log.success).mock.calls.map((c) => String(c[0])); + expect(said).toEqual([`Retired ${legacyDir} to ${legacyDir}.bak: this project's data already lives in ${partition}`]); + }); + it('keeps the legacy dir when an unreadable partition config appears after planning (#797)', async () => { // The re-check under the lock must use the same notion of "built" as the plan. await seedLegacyLayout(); diff --git a/src/__tests__/reports-branch-readonly.test.ts b/src/__tests__/reports-branch-readonly.test.ts index 6ce850180..6627288b5 100644 --- a/src/__tests__/reports-branch-readonly.test.ts +++ b/src/__tests__/reports-branch-readonly.test.ts @@ -153,7 +153,7 @@ describe('commitAndPushReports', () => { // `rev-list --count origin/<branch>..HEAD`: the publish confirms the ref // moved before reporting success. '0' is "nothing left to deliver". mocks.worktreeGit.raw.mockResolvedValue('0'); - mocks.worktreeGit.status.mockResolvedValue({ staged: ['members/alice.yaml'] }); + mocks.worktreeGit.status.mockResolvedValue({ staged: ['members/alice.yaml'], renamed: [] }); vi.mocked(acquireLock).mockResolvedValue(true); vi.mocked(releaseLock).mockResolvedValue(undefined); }); @@ -169,7 +169,7 @@ describe('commitAndPushReports', () => { }); it('pushes an unchanged report retry without making an empty commit', async () => { - mocks.worktreeGit.status.mockResolvedValue({ staged: [] }); + mocks.worktreeGit.status.mockResolvedValue({ staged: [], renamed: [] }); expect(await commitAndPushReports(config, 'retry stats', ['stats/alice.yaml'], { pushIfUnchanged: true })).toBe(true); expect(mocks.worktreeGit.commit).not.toHaveBeenCalled(); expect(mocks.worktreeGit.push).toHaveBeenCalledWith(['origin', 'teamai-reports']); @@ -177,7 +177,7 @@ describe('commitAndPushReports', () => { }); it('preserves the default no-op when no changes are staged', async () => { - mocks.worktreeGit.status.mockResolvedValue({ staged: [] }); + mocks.worktreeGit.status.mockResolvedValue({ staged: [], renamed: [] }); expect(await commitAndPushReports(config, 'unchanged member', ['members/'])).toBe(false); expect(mocks.worktreeGit.push).not.toHaveBeenCalled(); }); @@ -260,9 +260,12 @@ describe('readableReportsWorktree', () => { expect(mocks.worktreeGit.raw).not.toHaveBeenCalled(); }); - it('refreshes and ensures the checkout, publishing nothing, when the lock is free', async () => { + it('refreshes the checkout, publishing nothing, when the lock is free', async () => { vi.mocked(acquireLock).mockResolvedValue(true); mocks.isGitRepo.mockResolvedValue(true); + // A clean checkout, level with origin: the refresh completes. + mocks.worktreeGit.status.mockResolvedValue({ staged: [], renamed: [], conflicted: [], isClean: () => true }); + mocks.worktreeGit.raw.mockResolvedValue('0'); await expect(readableReportsWorktree(config)).resolves.toBe(WT); diff --git a/src/__tests__/reports-reader-lock.test.ts b/src/__tests__/reports-reader-lock.test.ts new file mode 100644 index 000000000..6753fb30c --- /dev/null +++ b/src/__tests__/reports-reader-lock.test.ts @@ -0,0 +1,76 @@ +/** + * A reports reader whose refresh failed must not create the checkout without + * the reports lock (#823 item 15): a writer that took the lock meanwhile may be + * creating it at the same path (#808). + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { simpleGit } from 'simple-git'; + +import { REPORTS_LOCK_FILENAME, REPORTS_WORKTREE_DIRNAME, type LocalConfig } from '../types.js'; + +// The first refresh fails before it creates anything, as a transient failure would. +vi.mock('../utils/git.js', async (importOriginal) => { + const actual = await importOriginal<typeof import('../utils/git.js')>(); + return { ...actual, isDedicatedRepoRoot: vi.fn(actual.isDedicatedRepoRoot).mockResolvedValueOnce(false) }; +}); + +// A writer takes the reports lock the moment the failed refresh lets it go. +const writer = vi.hoisted(() => ({ took: false })); +vi.mock('../update.js', async (importOriginal) => { + const actual = await importOriginal<typeof import('../update.js')>(); + return { + ...actual, + releaseLock: async (lockPath?: string) => { + await actual.releaseLock(lockPath); + if (!writer.took && lockPath && path.basename(lockPath) === REPORTS_LOCK_FILENAME) { + writer.took = true; + fs.writeFileSync(lockPath, JSON.stringify({ pid: process.pid, startedAt: new Date().toISOString(), owner: 'writer' })); + } + }, + }; +}); + +const { readableReportsWorktree } = await import('../utils/reports-branch.js'); + +let tmp: string; + +beforeEach(() => { + tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-reports-reader-lock-')); +}); + +afterEach(() => { + fs.rmSync(tmp, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); +}); + +async function gitInstall(): Promise<LocalConfig> { + const seed = path.join(tmp, 'seed'); + fs.mkdirSync(seed); + const seedGit = simpleGit(seed); + await seedGit.init(['--initial-branch=main']); + await seedGit.addConfig('user.email', 't@t.com'); + await seedGit.addConfig('user.name', 't'); + fs.writeFileSync(path.join(seed, 'teamai.yaml'), 'team: acme\n'); + await seedGit.add(['.']); + await seedGit.commit('init'); + const origin = path.join(tmp, 'origin.git'); + await simpleGit().clone(seed, origin, ['--bare']); + const clone = path.join(tmp, 'team-repo'); + await simpleGit().clone(origin, clone); + return { repo: { localPath: clone, remote: origin, kind: 'git' }, username: 'alice', scope: 'user', additionalRoles: [] }; +} + +describe('readableReportsWorktree after a failed refresh (#823 item 15)', () => { + it('creates no checkout while a writer holds the reports lock', async () => { + const config = await gitInstall(); + const checkout = path.join(tmp, REPORTS_WORKTREE_DIRNAME); + + const dir = await readableReportsWorktree(config); + + expect(writer.took).toBe(true); + expect(dir).toBe(checkout); + expect(fs.existsSync(checkout)).toBe(false); + }); +}); diff --git a/src/__tests__/single-repo-mode.test.ts b/src/__tests__/single-repo-mode.test.ts index 186f9f5b2..071919b80 100644 --- a/src/__tests__/single-repo-mode.test.ts +++ b/src/__tests__/single-repo-mode.test.ts @@ -14,7 +14,7 @@ import { TeamaiConfigSchema, type LocalConfig, } from '../types.js'; -import { buildSelfModeGitignore, migrateSelfModeGitignore, migrateSelfModeGitignoreContent } from '../init.js'; +import { buildProjectScopeGitignore, buildSelfModeGitignore, migrateSelfModeGitignore, migrateSelfModeGitignoreContent } from '../init.js'; /** The names of `names` that `gitignore`, as `.teamai/.gitignore`, makes git ignore. */ function ignoredBy(gitignore: string, names: string[]): string[] { @@ -38,6 +38,8 @@ function ignoredBy(gitignore: string, names: string[]): string[] { /** The files a usage write leaves beside `usage.jsonl` while it runs or when it gives up on the lock (#788). */ const USAGE_SIDE_FILES = ['usage.jsonl.lock', 'usage.jsonl.123.0123456789ab.tmp', 'usage.pending-0f8e2c1a-1b2c-4d5e-8f90-123456789abc.jsonl']; +/** The temp copy an interrupted config save leaves beside config.yaml (writeFileAtomic, #831). */ +const CONFIG_SAVE_TEMP = 'config.yaml.123.0123456789ab.tmp'; function makeConfig(kind: 'git' | 'http' | 'self', localPath = '/repo/.teamai'): LocalConfig { return { @@ -172,6 +174,10 @@ describe('buildSelfModeGitignore', () => { expect(ignoredBy(gi, USAGE_SIDE_FILES)).toEqual(USAGE_SIDE_FILES); }); + it('ignores the temp copy an interrupted config save leaves (#823)', () => { + expect(ignoredBy(gi, [CONFIG_SAVE_TEMP])).toEqual([CONFIG_SAVE_TEMP]); + }); + it('ignores the learnings worktree, its lock and the queue, so contributing leaves git status clean', () => { const lines = gi.split('\n').map((l) => l.trim()); expect(lines).toContain('learnings-wt/'); @@ -180,6 +186,13 @@ describe('buildSelfModeGitignore', () => { }); }); +describe('buildProjectScopeGitignore', () => { + it('ignores the local config and the temp copy an interrupted config save leaves (#823)', () => { + const names = ['config.yaml', CONFIG_SAVE_TEMP, 'state.json', 'token']; + expect(ignoredBy(buildProjectScopeGitignore(), names)).toEqual(names); + }); +}); + describe('migrateSelfModeGitignore', () => { it('leaves the .gitignore whole when the disk fills while it is healed', async () => { const root = fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-self-heal-')); @@ -221,7 +234,7 @@ describe('migrateSelfModeGitignoreContent (self-heal old gitignore)', () => { const old = [ 'env.sh', 'env.local', 'teamai.lock', 'learnings-wt/', '.learnings-lock', 'pending-learnings/', 'env/', - 'usage.jsonl.*', 'usage.pending-*.jsonl', + 'usage.jsonl.*', 'usage.pending-*.jsonl', 'config.yaml.*.tmp', ].join('\n'); const { changed, content } = migrateSelfModeGitignoreContent(old); expect(changed).toBe(false); // nothing to remove, env.local already present @@ -252,7 +265,7 @@ describe('migrateSelfModeGitignoreContent (self-heal old gitignore)', () => { const old = [ '# env is machine-local', 'config.yaml', 'env.local', 'teamai.lock', 'learnings-wt/', '.learnings-lock', 'pending-learnings/', - 'usage.jsonl.*', 'usage.pending-*.jsonl', + 'usage.jsonl.*', 'usage.pending-*.jsonl', 'config.yaml.*.tmp', ].join('\n'); const { changed, content } = migrateSelfModeGitignoreContent(old); // No bare `env` line, env.local already present → unchanged. @@ -275,6 +288,14 @@ describe('migrateSelfModeGitignoreContent (self-heal old gitignore)', () => { expect(content).toContain('usage.jsonl\nusage.jsonl.*\nusage.pending-*.jsonl\nknown-skills.json'); }); + it('adds the temp copy an interrupted config save leaves, after config.yaml (#823)', () => { + const old = ['config.yaml', 'state.json', 'token', 'teamai.lock', 'env.local'].join('\n'); + const { changed, content } = migrateSelfModeGitignoreContent(old); + expect(changed).toBe(true); + expect(content).toContain('config.yaml\nconfig.yaml.*.tmp\nstate.json'); + expect(ignoredBy(content, [CONFIG_SAVE_TEMP])).toEqual([CONFIG_SAVE_TEMP]); + }); + it('adds the learnings worktree, its lock and the queue', () => { const old = ['config.yaml', 'token', 'teamai.lock', 'env.local', 'reports-wt/', 'knowledge-wt/'].join('\n'); const { changed, content } = migrateSelfModeGitignoreContent(old); diff --git a/src/__tests__/votes-e2e.test.ts b/src/__tests__/votes-e2e.test.ts index d3e988071..65c27d3ab 100644 --- a/src/__tests__/votes-e2e.test.ts +++ b/src/__tests__/votes-e2e.test.ts @@ -225,8 +225,8 @@ describe('Phase 3+4 end-to-end data flow', () => { fs.writeFileSync(path.join(votesDir, 'user1.yaml'), YAML.stringify(v2)); const map = await computeAllConfidence(votesDir); - const updated = await writeBackConfidence([learningsDir], map); - expect(updated).toBe(1); + const written = await writeBackConfidence([learningsDir], map); + expect(written).toEqual([path.join(learningsDir, 'test-doc.md')]); // Verify frontmatter has confidence const afterContent = fs.readFileSync(path.join(learningsDir, 'test-doc.md'), 'utf-8'); diff --git a/src/contribute.ts b/src/contribute.ts index 2bde9d058..fd06b1b1a 100644 --- a/src/contribute.ts +++ b/src/contribute.ts @@ -5,13 +5,13 @@ import { assertNotReadOnly } from './read-only.js'; import { pathExists } from './utils/fs.js'; import { log, spinner } from './utils/logger.js'; import { markContributed } from './contribute-check.js'; -import { pendingLearningsDir, queueOwner, queueWriteRefusal, savePendingLearning } from './utils/pending-learnings.js'; +import { pendingLearningsDir, queueWriteRefusal, savePendingLearning } from './utils/pending-learnings.js'; import { publishQueuedLearnings } from './utils/learnings-publish.js'; import { indexableLearningsRoots } from './utils/learnings-roots.js'; import { resolveActiveLearningsNamespaces } from './projects.js'; import { isSafeNamespaceSegment } from './manifest-schema.js'; import type { GlobalOptions, LocalConfig } from './types.js'; -import { getBusinessRoot, getDataHome, getProjectSearchIndexPath, isSelfMode } from './types.js'; +import { getProjectSearchIndexPath, isSelfMode } from './types.js'; /** * Decide which learnings subdirectory a contribution lands in — resolved from @@ -102,31 +102,6 @@ export async function rebuildIndexAfterContribute(localConfig: LocalConfig): Pro // └─ done // -/** - * A queue an older teamai kept in this checkout's `.teamai/` is deleted with a - * linked worktree; move it into the shared queue, so the publish that follows - * sends it too (#808), or aside when the partition now serves another install - * (settleCheckoutQueue). The migration before contribute and import --from-mr - * does it too, and they stop when the queue would stay in the checkout - * (queueKeptInCheckout); this is a second pass. Best effort: the learning - * being queued must still be saved, and the old queue stays where it is. - */ -export async function drainCheckoutQueue(localConfig: LocalConfig): Promise<void> { - if (!isSelfMode(localConfig)) return; - const legacyDir = path.join(getBusinessRoot(localConfig), '.teamai'); - const dataHome = getDataHome(localConfig); - if (path.resolve(legacyDir) === path.resolve(dataHome)) return; - const { settleCheckoutQueue } = await import('./migrate.js'); - try { - await settleCheckoutQueue(legacyDir, dataHome, queueOwner(localConfig)); - } catch (e) { - log.warn( - `Could not move the learnings an older teamai queued in ${legacyDir} ` + - `(${e instanceof Error ? e.message : String(e)}). They stay there; the next teamai pull moves them.`, - ); - } -} - /** * Where a learning saved for an install that changed before it was published * is, for the member: still queued, or set aside with the previous install's @@ -227,7 +202,6 @@ export async function contribute( if (isSelfMode(localConfig)) { const { migrateSelfModeGitignore } = await import('./init.js'); await migrateSelfModeGitignore(localConfig); - await drainCheckoutQueue(localConfig); } try { diff --git a/src/import.ts b/src/import.ts index b84d92252..cf1fabc2d 100644 --- a/src/import.ts +++ b/src/import.ts @@ -245,10 +245,8 @@ export async function importCmd(opts: ImportOptions): Promise<void> { if (!opts.dryRun && !opts.output) assertNotReadOnly(localConfig, 'teamai import --from-mr'); // As contribute: into the active project's learnings namespace when there // is exactly one, else the shared root. - const { drainCheckoutQueue, resolveLearningsSubdir } = await import('./contribute.js'); + const { resolveLearningsSubdir } = await import('./contribute.js'); const learningsSubdir = opts.dryRun || opts.output ? '' : await resolveLearningsSubdir(localConfig); - // As contribute, before the extraction dedupes against the queue it writes into. - if (!opts.dryRun && !opts.output) await drainCheckoutQueue(localConfig); const tasks = new Listr([ { diff --git a/src/index.ts b/src/index.ts index 1d84f6e48..20e0a5641 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1276,9 +1276,9 @@ recallCmd if (cmdOpts.confidenceWriteback) { const { computeAllConfidence, writeBackConfidence } = await import('./maintenance/index.js'); const map = await computeAllConfidence(votesDir); - const updated = await writeBackConfidence(learningsReadDirs, map, learningsWriteDir); - if (updated > 0) { - await publishMaintenance(localConfig, `[teamai] Update confidence for ${updated} learning(s)`); + const written = await writeBackConfidence(learningsReadDirs, map, learningsWriteDir); + if (written.length > 0) { + await publishMaintenance(localConfig, `[teamai] Update confidence for ${written.length} learning(s)`, written); } return; } @@ -1306,6 +1306,7 @@ recallCmd await publishMaintenance( localConfig, `[teamai] Prune ${pruned.archived + pruned.removed} learning(s)`, + pruned.changed, ); } return; @@ -1380,13 +1381,13 @@ recallCmd return; } - await executePromotion(candidate, repoPath, { + const { marked } = await executePromotion(candidate, repoPath, { category: cmdOpts.category as 'skills' | 'rules' | 'docs' | undefined, dryRun: cmdOpts.dryRun, learningsWriteDir, }); - if (!cmdOpts.dryRun) { - await publishMaintenance(localConfig, `[teamai] Mark ${candidate.docId} as promoted`); + if (marked) { + await publishMaintenance(localConfig, `[teamai] Mark ${candidate.docId} as promoted`, [marked]); } }); @@ -1417,14 +1418,14 @@ async function maintenancePathsOrExit(localConfig: LocalConfig): Promise<Mainten } /** - * Publish what a maintenance command just changed in the learnings worktree. - * Best-effort: the change is already on disk, so a failure to publish is worth + * Publish the files a maintenance command just changed in the learnings + * worktree, and nothing else there. Best-effort: the change is already on disk, so a failure to publish is worth * reporting but never worth failing the command over. */ -async function publishMaintenance(localConfig: LocalConfig, message: string): Promise<void> { +async function publishMaintenance(localConfig: LocalConfig, message: string, changed: readonly string[]): Promise<void> { const { publishLearningsMaintenance } = await import('./utils/learnings-publish.js'); const { log } = await import('./utils/logger.js'); - const result = await publishLearningsMaintenance(localConfig, message); + const result = await publishLearningsMaintenance(localConfig, message, changed); if (result.status === 'published') { log.success('Published maintenance changes to the learnings branch'); } else if (result.status === 'failed') { diff --git a/src/init.ts b/src/init.ts index b6c3446f1..a263a6b1b 100644 --- a/src/init.ts +++ b/src/init.ts @@ -35,6 +35,23 @@ async function settleModeSwitch(previous: LocalConfig | null, next: LocalConfig, process.exit(1); } } + +/** + * Move a config that names another install out of the way, to a free + * `config.yaml.previous` name beside it, before init clones the new team repo + * where that install's clone was. If init then stops before it saves the new + * config, no command runs the old config against the new clone (#823 item 17): + * they all ask for `teamai init` instead. Nothing is deleted. + */ +async function moveConfigAside(configPath: string, next: LocalConfig): Promise<void> { + let aside = `${configPath}.previous`; + for (let n = 1; await pathExists(aside); n++) aside = `${configPath}.previous.${n}`; + await fs.promises.rename(configPath, aside); + log.warn( + `Moved ${configPath} to ${aside}: it is another install's config, and init replaces its team repo with ` + + `${redactGitCredentials(next.repo.remote)}. If init stops before it finishes, run it again.`, + ); +} import { log, spinner } from './utils/logger.js'; import { CLAUDE_TOOL_ID, @@ -657,6 +674,8 @@ export function buildSelfModeGitignore(): string { '# As of P2 this data lives in ~/.teamai/projects/<slug>/; these entries guard', '# pre-P2 installs (pre-migration) and any un-relocated path.', 'config.yaml', + // The temp copy an interrupted config save leaves (writeFileAtomic, #831). + 'config.yaml.*.tmp', 'state.json', 'token', 'teamai.lock', @@ -703,6 +722,32 @@ export function buildSelfModeGitignore(): string { ].join('\n'); } +/** The `.gitignore` project-scope init writes beside its local config, once. */ +export function buildProjectScopeGitignore(): string { + return [ + '# teamai local config (do not commit)', + 'config.yaml', + // The temp copy an interrupted config save leaves (writeFileAtomic, #831). + 'config.yaml.*.tmp', + 'state.json', + 'token', + 'teamai.lock', + '.update-lock', + 'env', + 'env.sh', + 'sessions/', + 'dashboard/', + 'usage.jsonl', + 'usage.jsonl.*', + 'usage.pending-*.jsonl', + 'known-skills.json', + 'learnings/', + 'search-index.json', + 'votes/', + '', + ].join('\n'); +} + /** * Migrate an existing single-repo `.teamai/.gitignore` written by an older teamai. * Early versions ignored `env`, which hid `.teamai/env/env.yaml` from push and @@ -748,6 +793,8 @@ export function migrateSelfModeGitignoreContent(content: string): { changed: boo // The usage lock, rewrite temps and pending events arrived with the usage cap (#788). ensure('usage.jsonl.*', 'usage.jsonl'); ensure('usage.pending-*.jsonl', 'usage.jsonl.*'); + // config.yaml has been saved atomically, through a temp copy, since #831. + ensure('config.yaml.*.tmp', 'config.yaml'); return { changed, content: filtered.join('\n') }; } @@ -1438,6 +1485,22 @@ export async function init(options: GlobalOptions & { } if (!await pathExists(localPath)) { + // The clone about to land here is another install's than the config beside + // it: settle that install now, as the config save below would, instead of + // leaving its config to run against this clone should init stop first. + const next: LocalConfig = { + repo: { localPath, remote: repoInfo.httpsUrl }, + username, + scope, + projectRoot, + additionalRoles: [], + ...(scope === 'project' ? { dataHome: teamaiHome } : {}), + }; + const replacesAnother = existingLocalConfig + ? !sameQueueOwner(queueOwner(existingLocalConfig), queueOwner(next)) + : await pathExists(existingConfigPath); + if (replacesAnother) await settleModeSwitch(existingLocalConfig, next, () => moveConfigAside(existingConfigPath, next)); + const cloneSpin = spinner('Cloning team repo...').start(); const cloneTarget = provider.name === 'git' ? repoInfo.httpsUrl @@ -1728,10 +1791,10 @@ export async function init(options: GlobalOptions & { // Persist --agent into enabledAgents (additive across runs) const requestedAgents = normalizeAgentList(options.agent); if (requestedAgents.length > 0) { - const existing = await loadLocalConfigForScope(scope, projectRoot); - const prev = existing?.enabledAgents ?? []; + // As loaded before the clone: that config may have been moved aside since. + const prev = existingLocalConfig?.enabledAgents ?? []; localConfig.enabledAgents = [...new Set([...prev, ...requestedAgents])]; - localConfig.disabledAgents = (existing?.disabledAgents ?? []).filter((t) => !requestedAgents.includes(t)); + localConfig.disabledAgents = (existingLocalConfig?.disabledAgents ?? []).filter((t) => !requestedAgents.includes(t)); } // Carry the member's recorded tool roots across a re-init. `init` is @@ -1758,27 +1821,7 @@ export async function init(options: GlobalOptions & { // Generate .gitignore for project scope to prevent local config from being committed const gitignorePath = path.join(teamaiHome, '.gitignore'); if (!await pathExists(gitignorePath)) { - const gitignoreContent = [ - '# teamai local config (do not commit)', - 'config.yaml', - 'state.json', - 'token', - 'teamai.lock', - '.update-lock', - 'env', - 'env.sh', - 'sessions/', - 'dashboard/', - 'usage.jsonl', - 'usage.jsonl.*', - 'usage.pending-*.jsonl', - 'known-skills.json', - 'learnings/', - 'search-index.json', - 'votes/', - '', - ].join('\n'); - await writeFile(gitignorePath, gitignoreContent); + await writeFile(gitignorePath, buildProjectScopeGitignore()); log.debug('Generated .teamai/.gitignore for project scope'); } } else { diff --git a/src/maintenance/confidence.ts b/src/maintenance/confidence.ts index cd0575885..7358fc43a 100644 --- a/src/maintenance/confidence.ts +++ b/src/maintenance/confidence.ts @@ -88,14 +88,14 @@ export async function computeAllConfidence(votesDir: string): Promise<Map<string /** * Write confidence scores back into learning document frontmatter. * Only updates docs whose confidence changed by > 0.05. - * Returns count of files updated. + * Returns the files it wrote, which are what a publish may stage (#823). */ export async function writeBackConfidence( learningsDirs: readonly string[], confidenceMap: Map<string, number>, writeRoot?: string, -): Promise<number> { - let updated = 0; +): Promise<string[]> { + const written: string[] = []; const files = await listLearningFiles(learningsDirs); for (const { file, absPath, root } of files) { @@ -123,14 +123,14 @@ export async function writeBackConfidence( : absPath; await ensureDir(path.dirname(target)); await writeFile(target, newContent); - updated++; + written.push(target); } catch { log.debug(`confidence: failed to update frontmatter for: ${file}`); } } - if (updated > 0) { - log.info(`Updated confidence scores for ${updated} learning(s)`); + if (written.length > 0) { + log.info(`Updated confidence scores for ${written.length} learning(s)`); } - return updated; + return written; } diff --git a/src/maintenance/promote.ts b/src/maintenance/promote.ts index 5e4ca9307..74ddb4f59 100644 --- a/src/maintenance/promote.ts +++ b/src/maintenance/promote.ts @@ -148,11 +148,15 @@ Output ONLY the transformed markdown content (including YAML frontmatter with ti } } +/** + * The promoted entry's path, and the learning file it marked as promoted (null + * when it marked none), which is what a publish may stage (#823). + */ export async function executePromotion( candidate: PromotionCandidate, repoPath: string, options: PromoteOptions & { learningsWriteDir?: string } = {}, -): Promise<string> { +): Promise<{ targetPath: string; marked: string | null }> { const category = options.category ?? candidate.suggestedCategory; const targetDir = path.join(repoPath, category); await ensureDir(targetDir); @@ -161,13 +165,13 @@ export async function executePromotion( if (options.dryRun) { log.info(`[dry-run] Would promote ${candidate.docId} -> ${category}/${candidate.filename}`); - return targetPath; + return { targetPath, marked: null }; } const originalContent = await readFileSafe(candidate.path); if (!originalContent) { log.error(`Cannot read source file: ${candidate.path}`); - return targetPath; + return { targetPath, marked: null }; } // AI transforms the learning into a generalized format for the target category @@ -189,7 +193,7 @@ export async function executePromotion( await writeFile(markPath, updated); log.success(`Promoted: ${candidate.docId} -> ${category}/${candidate.filename}`); - return targetPath; + return { targetPath, marked: markPath }; } function inferCategoryByKeywords(content: string, title: string): 'skills' | 'rules' | 'docs' { diff --git a/src/maintenance/prune.ts b/src/maintenance/prune.ts index 1ed70076d..971b87eb5 100644 --- a/src/maintenance/prune.ts +++ b/src/maintenance/prune.ts @@ -81,19 +81,21 @@ export async function findPruneCandidates( } /** - * Execute prune: archive or remove low-confidence documents. + * Execute prune: archive or remove low-confidence documents. `changed` is every + * file it removed or wrote, which is what a publish may stage (#823). */ export async function executePrune( learningsWriteDir: string, candidates: PruneCandidate[], options: PruneOptions = {}, -): Promise<{ archived: number; removed: number }> { +): Promise<{ archived: number; removed: number; changed: string[] }> { let archived = 0; let removed = 0; + const changed: string[] = []; if (options.dryRun) { log.info(`[dry-run] Would ${options.archive ? 'archive' : 'remove'} ${candidates.length} file(s)`); - return { archived: 0, removed: 0 }; + return { archived: 0, removed: 0, changed }; } const inherited: string[] = []; @@ -109,11 +111,14 @@ export async function executePrune( if (options.archive) { const archiveDir = path.join(learningsWriteDir, '_archive'); await ensureDir(archiveDir); - await copyFile(candidate.path, path.join(archiveDir, candidate.filename)); + const archivedCopy = path.join(archiveDir, candidate.filename); + await copyFile(candidate.path, archivedCopy); await remove(candidate.path); + changed.push(candidate.path, archivedCopy); archived++; } else { await remove(candidate.path); + changed.push(candidate.path); removed++; } } @@ -126,5 +131,5 @@ export async function executePrune( + `${inherited.join(', ')}. Remove them with a pull request against the team repo.`, ); } - return { archived, removed }; + return { archived, removed, changed }; } diff --git a/src/migrate.ts b/src/migrate.ts index 365a32b5d..c8aa01c43 100644 --- a/src/migrate.ts +++ b/src/migrate.ts @@ -439,7 +439,9 @@ export async function runMigration( await releaseLock(lockPath); lockReleased = true; const backup = await retireLegacy(legacyDir); - log.success(`Finished an interrupted migration: retired ${legacyDir} to ${backup}`); + // Not always an interrupted run: a linked worktree lands here once another + // checkout of the repo built the partition. + log.success(`Retired ${legacyDir} to ${backup}: this project's data already lives in ${partitionDir}`); return 'migrated'; } @@ -881,7 +883,7 @@ async function migrateSelfA1(legacyDir: string, partitionDir: string, legacyOwne * - a config that cannot be read: left in place, since where they would be * published is unknown. */ -export async function settleCheckoutQueue(legacyDir: string, partitionDir: string, owner: QueueOwner): Promise<void> { +async function settleCheckoutQueue(legacyDir: string, partitionDir: string, owner: QueueOwner): Promise<void> { const queue = path.join(legacyDir, SELF_LEGACY_QUEUE); if (!(await pathExists(queue))) return; // The install read below decides where the queue goes; an init switching it diff --git a/src/pull.ts b/src/pull.ts index 33247a0de..d0c70a76e 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -679,11 +679,13 @@ async function pullForScope( // partition sync lock across this scope and the lock is not reentrant, so // publishing must not try to take it again. Never let it block the pull. try { - const queue = await publishQueuedLearnings(localConfig, localConfig.username, { holdsSyncLock: true }); - if (queue.published.length > 0) { + const queue = await publishQueuedLearnings(localConfig, localConfig.username, { holdsSyncLock: true, dryRun: options.dryRun }); + if (options.dryRun) { + if (queue.remaining > 0) log.info(`[${scopeLabel}] [dry-run] Would publish ${queue.remaining} queued learning(s)`); + } else if (queue.published.length > 0) { log.success(`Published ${queue.published.length} queued learning(s)`); } - if (queue.remaining > 0) { + if (queue.remaining > 0 && !options.dryRun) { // Say it out loud. A member whose pushes are rejected would otherwise // queue notes forever and never hear about it. reported.add('pending-learnings'); diff --git a/src/utils/branch-worktree.ts b/src/utils/branch-worktree.ts index 8d9d45ad4..dba28bd40 100644 --- a/src/utils/branch-worktree.ts +++ b/src/utils/branch-worktree.ts @@ -136,7 +136,7 @@ async function remoteBranchExists(spec: BranchWorktreeSpec, repoRoot: string): P * stale. An explicit refspec creates and updates that tracking ref in every * clone (#706). Used by both the reports and learnings branches. */ -async function fetchTrackingRef(git: SimpleGit, branch: string): Promise<void> { +export async function fetchTrackingRef(git: SimpleGit, branch: string): Promise<void> { await git.fetch(['origin', `+refs/heads/${branch}:refs/remotes/origin/${branch}`]); } @@ -274,12 +274,7 @@ async function ensureWorktree( */ async function removeOldCheckoutsInDotTeamai(spec: BranchWorktreeSpec, git: SimpleGit, wt: string): Promise<void> { const oldSuffix = `${path.sep}${path.join('.teamai', spec.worktreeDirname)}`; - const listing = await git.raw(['worktree', 'list', '--porcelain']); - for (const entry of listing.split('\n\n')) { - const lines = entry.split('\n'); - const checkout = lines.find((l) => l.startsWith('worktree '))?.slice('worktree '.length); - const branch = lines.find((l) => l.startsWith('branch '))?.slice('branch '.length); - if (checkout === undefined || branch !== `refs/heads/${spec.branch}`) continue; + for (const checkout of await checkoutsOfBranch(spec, git)) { // resolve: git prints forward slashes on Windows too. if (!path.resolve(checkout).endsWith(oldSuffix) || path.resolve(checkout) === path.resolve(wt)) continue; try { @@ -298,6 +293,31 @@ async function removeOldCheckoutsInDotTeamai(spec: BranchWorktreeSpec, git: Simp } } +/** The checkouts of this branch the repository behind `git` registers (git allows one). */ +async function checkoutsOfBranch(spec: BranchWorktreeSpec, git: SimpleGit): Promise<string[]> { + const listing = await git.raw(['worktree', 'list', '--porcelain']); + const checkouts: string[] = []; + for (const entry of listing.split('\n\n')) { + const lines = entry.split('\n'); + const checkout = lines.find((l) => l.startsWith('worktree '))?.slice('worktree '.length); + const branch = lines.find((l) => l.startsWith('branch '))?.slice('branch '.length); + if (checkout !== undefined && branch === `refs/heads/${spec.branch}`) checkouts.push(checkout); + } + return checkouts; +} + +/** + * Where this repository has the branch checked out, wherever that is: the + * shared checkout, or one an older teamai left in a checkout's `.teamai/`. + * Null when it has none, or no branch worktrees at all. Never another + * repository's checkout, since it is this repository's own registration. + */ +async function registeredCheckoutImpl(spec: BranchWorktreeSpec, localConfig: LocalConfig): Promise<string | null> { + if (!usesBranchWorktree(localConfig)) return null; + const [checkout] = await checkoutsOfBranch(spec, createGit(gitRoot(localConfig))); + return checkout ?? null; +} + /** * Self and git mode keep this checkout at the same path in the partition * (#808), so after a project switches mode, the checkout there may belong to @@ -530,7 +550,9 @@ async function commitAndPushAt( await git.add(files); const status = await git.status(); - if (status.staged.length === 0 && !options.pushIfUnchanged && await nothingLeftToPush(git, spec)) { + // simple-git lists a staged rename (an archived learning) under `renamed`, not `staged`. + const staged = status.staged.length + status.renamed.length; + if (staged === 0 && !options.pushIfUnchanged && await nothingLeftToPush(git, spec)) { // Nothing to commit AND nothing to deliver. Those are two different things: // an earlier attempt may have committed exactly this content and failed to // push it, and a caller that reads "already present" drops the only durable @@ -541,7 +563,7 @@ async function commitAndPushAt( // A retry may reconstruct the same tree as a previously committed // but unconfirmed push. It still needs a push, without an empty commit. - if (status.staged.length > 0) await commitSkippingHooks(git, message); + if (staged > 0) await commitSkippingHooks(git, message); // Push with fetch+rebase retry. Each member only writes <user>.yaml, so // rebase conflicts are effectively impossible; retries handle the pure @@ -890,6 +912,8 @@ export interface BranchWorktree { checkOwner(localConfig: LocalConfig): Promise<void>; /** True when the checkout there is not provably this repository's; reads git's files, runs no git. */ isForeignByFiles(localConfig: LocalConfig): Promise<boolean>; + /** Where this repository has the branch checked out, the old `.teamai/` place included; creates nothing. */ + registeredCheckout(localConfig: LocalConfig): Promise<string | null>; } /** @@ -918,5 +942,6 @@ export function createBranchWorktree(spec: BranchWorktreeSpec): BranchWorktree { refresh: (localConfig, options) => refreshImpl(spec, localConfig, options), checkOwner: (localConfig) => checkOwnerImpl(spec, localConfig), isForeignByFiles: (localConfig) => isForeignByFilesImpl(spec, localConfig), + registeredCheckout: (localConfig) => registeredCheckoutImpl(spec, localConfig), }; } diff --git a/src/utils/learnings-publish.ts b/src/utils/learnings-publish.ts index 25ac5d67d..af49a8122 100644 --- a/src/utils/learnings-publish.ts +++ b/src/utils/learnings-publish.ts @@ -8,16 +8,20 @@ */ import path from 'node:path'; import fs from 'node:fs'; +import type { SimpleGit } from 'simple-git'; import { ensureDir } from './fs.js'; +import { parseFrontmatter } from './frontmatter.js'; +import { createGit } from './git.js'; import { learningsBranch } from './learnings-branch.js'; -import { CheckoutRefusedError, failureReason, type PublishResult } from './branch-worktree.js'; +import { CheckoutRefusedError, failureReason, fetchTrackingRef, type PublishResult } from './branch-worktree.js'; import { log } from './logger.js'; import { dropPendingLearning, listPendingForInstall, listPendingLearnings, readPendingLearning, + savePendingLearning, } from './pending-learnings.js'; import { acquireLock, releaseLock } from '../update.js'; import { getDataHome, SYNC_LOCK_FILENAME, type LocalConfig } from '../types.js'; @@ -57,8 +61,32 @@ function commitMessageFor(username: string): string { export async function publishQueuedLearnings( localConfig: LocalConfig, username: string, - options: { holdsSyncLock?: boolean } = {}, + options: { holdsSyncLock?: boolean; dryRun?: boolean } = {}, ): Promise<PublishQueueReport> { + // Publishing writes to the team clone, which `pull` and `push` guard with the + // partition sync lock. On contention nothing is lost and nothing is forced: + // the learnings stay queued and the run that holds the lock publishes them. + // `pull` already holds the lock when it calls this, and the lock is not + // reentrant, so it says so instead of deadlocking against itself. + const syncLock = options.holdsSyncLock ? null : syncLockPath(localConfig); + const locked = syncLock === null || await acquireLock(syncLock); + try { + return await publishUnderSyncLock(localConfig, username, locked, options.dryRun === true); + } finally { + if (syncLock && locked) await releaseLock(syncLock); + } +} + +async function publishUnderSyncLock( + localConfig: LocalConfig, + username: string, + locked: boolean, + dryRun: boolean, +): Promise<PublishQueueReport> { + // Before the queue is read, which this adds to. Only under the sync lock: + // two commands at once would each queue the same file. + if (locked && !dryRun) await queueImportRemnants(localConfig); + const listing = await listPendingForInstall(localConfig); switch (listing.status) { case 'listed': @@ -84,14 +112,8 @@ export async function publishQueuedLearnings( if (queued.length === 0) { return { published: [], remaining: 0 }; } - - // Publishing writes to the team clone, which `pull` and `push` guard with the - // partition sync lock. On contention nothing is lost and nothing is forced: - // the learnings stay queued and the run that holds the lock publishes them. - // `pull` already holds the lock when it calls this, and the lock is not - // reentrant, so it says so instead of deadlocking against itself. - const syncLock = options.holdsSyncLock ? null : syncLockPath(localConfig); - if (syncLock && !(await acquireLock(syncLock))) { + if (dryRun) return { published: [], remaining: queued.length }; + if (!locked) { log.debug('[learnings] a pull or push is in progress; leaving the queue for it'); return { published: [], @@ -117,8 +139,6 @@ export async function publishQueuedLearnings( lastError: failureReason(e), refused: e instanceof CheckoutRefusedError || undefined, }; - } finally { - if (syncLock) await releaseLock(syncLock); } } @@ -136,6 +156,123 @@ function syncLockPath(localConfig: LocalConfig): string | null { } } +/** + * How `import --from-mr` in 0.25.0 to 0.26.0-beta.3 named the learning it wrote: + * `<YYYY-MM-DD>-<title>.md`, the title part empty when no character of it was kept. + */ +const IMPORT_REMNANT_NAME = /^\d{4}-\d{2}-\d{2}-(.*)\.md$/; + +/** The merge request a learning was extracted from, from its frontmatter. */ +function sourceMr(frontmatter: Record<string, unknown>): string | null { + const mr = frontmatter.source_mr; + return typeof mr === 'string' && mr.trim() !== '' ? mr.trim() : null; +} + +/** + * Queue what `import --from-mr` (0.25.0 to 0.26.0-beta.3) wrote into the learnings checkout and + * never committed (#823 item 7), so the publish that follows sends it. Such a + * file never reached the team and, in single-repo mode, keeps git from removing + * the checkout an older teamai left in `.teamai/`. + * + * Only that exact shape moves: untracked, directly under `learnings/`, named + * `<date>-<title>.md`, with `source_mr` in its frontmatter. Nothing else in the + * checkout is touched; a learning on the branch is tracked, edited or not, so it + * never is. One is removed instead when the branch or the queue already has one + * from the same merge request (a later import of it) or with the same content. The copy is queued before the original goes, so + * a failure leaves it where it was. Never throws. + */ +async function queueImportRemnants(localConfig: LocalConfig): Promise<void> { + try { + const checkout = await learningsBranch.registeredCheckout(localConfig); + if (checkout === null) return; + const git = createGit(checkout); + const lsFiles = async (args: string[]): Promise<string[]> => + (await git.raw(['ls-files', '-z', ...args, '--', 'learnings'])).split('\0').filter(Boolean); + + const remnants: Array<{ file: string; content: string; mr: string; title?: string }> = []; + for (const rel of await lsFiles(['--others', '--exclude-standard'])) { + // Directly under `learnings/`: those versions wrote nothing into a namespace. + if (rel.split('/').length !== 2) continue; + const named = IMPORT_REMNANT_NAME.exec(path.posix.basename(rel)); + if (!named) continue; + const file = path.join(checkout, rel); + const content = await fs.promises.readFile(file, 'utf-8'); + const { data } = parseFrontmatter(content); + const mr = sourceMr(data); + if (mr === null) continue; + const title = typeof data.title === 'string' && data.title.trim() ? data.title : named[1]; + remnants.push({ file, content, mr, title: title || undefined }); + } + if (remnants.length === 0) return; + + // What already covers a remnant: a learning on the branch, in any + // namespace, or one in the queue. + const known: Array<{ label: string; content: string; mr: string | null }> = []; + for (const rel of (await lsFiles([])).filter((f) => f.endsWith('.md'))) { + const content = await fs.promises.readFile(path.join(checkout, rel), 'utf-8').catch(() => null); + if (content !== null) known.push({ label: rel, content, mr: sourceMr(parseFrontmatter(content).data) }); + } + // The checkout may be behind origin: the one an older teamai left in + // `.teamai/` is never synced again (#823 item 21). What a teammate published + // since counts too. + for (const rel of await publishedSinceCheckout(git)) { + const content = await git.show([`origin/${learningsBranch.branch}:${rel}`]).catch(() => null); + if (content !== null) known.push({ label: rel, content, mr: sourceMr(parseFrontmatter(content).data) }); + } + for (const rel of await listPendingLearnings(localConfig)) { + const content = await readPendingLearning(localConfig, rel); + if (content !== null) { + known.push({ label: `the contribution queue (${rel})`, content, mr: sourceMr(parseFrontmatter(content).data) }); + } + } + + const { generateFilename, resolveLearningsSubdir } = await import('../contribute.js'); + const subdir = await resolveLearningsSubdir(localConfig); + const queued: string[] = []; + for (const remnant of remnants) { + const covered = known.find((k) => k.content === remnant.content || k.mr === remnant.mr); + if (covered) { + await fs.promises.rm(remnant.file, { force: true }); + log.warn(`Removed ${remnant.file}, which an older teamai import --from-mr left unpublished: ${covered.label} already has it.`); + continue; + } + const relPath = path.posix.join(subdir, generateFilename(remnant.title)); + const saved = await savePendingLearning(localConfig, relPath, remnant.content); + if (saved.status !== 'saved') { + log.debug(`[learnings] could not queue ${remnant.file}: ${saved.status}`); + break; + } + await fs.promises.rm(remnant.file, { force: true }); + queued.push(remnant.file); + known.push({ label: `the contribution queue (${relPath})`, content: remnant.content, mr: remnant.mr }); + } + if (queued.length > 0) { + log.warn(`Queued ${queued.length} learning(s) an older teamai import --from-mr left unpublished: ${queued.join(', ')}`); + } + } catch (e) { + log.debug(`[learnings] could not queue what an older import --from-mr left: ${failureReason(e)}`); + } +} + +/** + * The learnings origin has and the checkout's commit lacks or holds another + * version of, after a best-effort fetch. None when origin cannot be read. + */ +async function publishedSinceCheckout(git: SimpleGit): Promise<string[]> { + try { + await fetchTrackingRef(git, learningsBranch.branch); + } catch (e) { + log.debug(`[learnings] fetch failed, comparing with the last fetched ${learningsBranch.branch}: ${failureReason(e)}`); + } + try { + const diff = await git.raw(['diff', '-z', '--name-only', '--no-renames', '--diff-filter=AM', 'HEAD', `origin/${learningsBranch.branch}`, '--', 'learnings']); + return diff.split('\0').filter((f) => f.endsWith('.md')); + } catch (e) { + log.debug(`[learnings] cannot compare the checkout with origin/${learningsBranch.branch}: ${failureReason(e)}`); + return []; + } +} + /** * Publish whatever maintenance just changed in the learnings worktree. * @@ -147,11 +284,29 @@ function syncLockPath(localConfig: LocalConfig): string | null { export async function publishLearningsMaintenance( localConfig: LocalConfig, message: string, + changed: readonly string[], ): Promise<PublishResult> { + // Only the files maintenance wrote or removed, never all of `learnings/`: the + // checkout may hold files nobody committed, such as a learning an older + // import --from-mr left there, and they would ride along in this commit (#823). + const checkout = learningsBranch.dir(localConfig); + const inCheckout = changed + .map((file) => path.relative(checkout, file)) + .filter((rel) => rel !== '' && !rel.startsWith('..') && !path.isAbsolute(rel)); + if (inCheckout.length < changed.length) { + log.debug(`[learnings] maintenance changed files outside ${checkout}; not publishing those`); + } + // A removed file git never tracked has nothing to stage, and naming it would + // fail the whole `git add`, publishing nothing of this run. + const removed = inCheckout.filter((rel) => !fs.existsSync(path.join(checkout, rel))); + const tracked = removed.length === 0 ? new Set<string>() + : new Set((await createGit(checkout).raw(['ls-files', '-z', '--', ...removed])).split('\0').filter(Boolean)); + const files = inCheckout.filter((rel) => !removed.includes(rel) || tracked.has(rel.split(path.sep).join('/'))); + if (files.length === 0) return { status: 'already-present' }; // `commitAndPush`, not `update`: maintenance already wrote into the worktree // before this call, and `update` syncs with origin first, which can carry // those uncommitted files into a rebase or leave them behind. - return learningsBranch.commitAndPush(localConfig, message, ['learnings']); + return learningsBranch.commitAndPush(localConfig, message, files); } /** diff --git a/src/utils/reports-branch.ts b/src/utils/reports-branch.ts index 3a66e8fe7..29257ecf8 100644 --- a/src/utils/reports-branch.ts +++ b/src/utils/reports-branch.ts @@ -84,15 +84,19 @@ export function refreshReportsWorktree( * that copy is another repository's. */ export async function readableReportsWorktree(localConfig: LocalConfig): Promise<string> { - const refreshed = await reportsBranch.refresh(localConfig, { pushIfCreated: false }); + let refreshed = await reportsBranch.refresh(localConfig, { pushIfCreated: false }); + // Failed: try once more, under the lock like every creation, and throw the + // cause if it fails again. An `ensure` here would create the checkout without + // the lock, while a writer that took it meanwhile creates it too (#823 item 15). + if (refreshed.status === 'failed') refreshed = await reportsBranch.refresh(localConfig, { pushIfCreated: false }); switch (refreshed.status) { + case 'done': + return reportsBranch.dir(localConfig); case 'busy': await reportsBranch.checkOwner(localConfig); return reportsBranch.dir(localConfig); - // Failed: ensure tries once more, and throws its cause if it fails again. - case 'done': case 'failed': - return reportsBranch.ensure(localConfig, { pushIfCreated: false }); + throw new Error(refreshed.reason); default: { const unhandled: never = refreshed; throw new Error(`Unhandled refresh result: ${JSON.stringify(unhandled)}`); From c23302d85abe122eb546821b70a941aa9af7fa59 Mon Sep 17 00:00:00 2001 From: Saul Moro <saul.moro.gomez@gmail.com> Date: Sat, 26 Sep 2026 09:46:41 +0200 Subject: [PATCH 02/13] =?UTF-8?q?fix(learnings):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20literal=20pathspecs,=20carry=20settings=20after=20a?= =?UTF-8?q?=20failed=20clone?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- docs/designs/data-directory-layout.md | 4 +- docs/usage-guide.md | 3 +- docs/usage-guide.zh-CN.md | 2 +- .../e2e/self-mode-worktrees-808.test.ts | 27 +++++++++++++ .../learnings-import-remnants.test.ts | 19 +++++++++ src/init.ts | 40 ++++++++++++++----- src/utils/branch-worktree.ts | 3 +- src/utils/learnings-publish.ts | 2 +- 8 files changed, 86 insertions(+), 14 deletions(-) diff --git a/docs/designs/data-directory-layout.md b/docs/designs/data-directory-layout.md index 8e934a803..2cf2c5a8e 100644 --- a/docs/designs/data-directory-layout.md +++ b/docs/designs/data-directory-layout.md @@ -597,7 +597,9 @@ the same way (queue set aside, indexes dropped), and moves the old loaded before the clone, finds nothing left to set aside, and writes the new config. An `init` that stops in between (an unknown `--role`, a busy queue lock, a prompt left) leaves no config naming the old team beside the new clone, so every -command asks for `teamai init`; nothing is deleted. +command asks for `teamai init`; nothing is deleted. The rerun finds no +`config.yaml`, so it reads the last `config.yaml.previous[.<n>]` for the +settings a re-init carries forward (`inheritUserScope`, agents, tool roots). A self install whose business repository moved to another URL (renamed or transferred) is another owner too: `init` sets its queue aside, and the warning names the directory to diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 52f5bd21f..9832d5647 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -431,7 +431,8 @@ another way (with or without `.git`, SSH or HTTPS) keeps the queue. Before it clones the other team repository, `init` moves the old `config.yaml` to `config.yaml.previous` beside it and says so: if `init` stops before it saves the new config, every command asks for `teamai init` instead of running the old -team's config against the new clone. Run `init` again. When the old +team's config against the new clone. Run `init` again: it carries that config's +settings (agents, tool roots) from `config.yaml.previous`. When the old install's `config.yaml` exists but cannot be read, nothing says whose the queue is: `init` moves it to `pending-learnings.unknown`, names that file and deletes the search indexes. A checkout that had not been upgraded yet keeps its old queue the same way: the next command there diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index b50aad522..adb18f516 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -384,7 +384,7 @@ learning 会被移到同一数据目录下的 `pending-learnings.<旧类型>`， 也同样处理，队列移到 `pending-learnings.<类型>-<仓库>`（例如 `pending-learnings.git-github.com-org-team-a`）； 同一仓库换一种写法（带或不带 `.git`、SSH 或 HTTPS）不会移动队列。克隆另一个团队仓库之前， `init` 会把旧的 `config.yaml` 移到旁边的 `config.yaml.previous` 并给出提示：若 `init` 在保存新配置前停止， -所有命令都会要求先运行 `teamai init`，而不会用旧团队的配置操作新的克隆。重新运行 `init` 即可。若旧安装的 `config.yaml` 存在但无法读取， +所有命令都会要求先运行 `teamai init`，而不会用旧团队的配置操作新的克隆。重新运行 `init` 即可：它会从 `config.yaml.previous` 沿用该配置的设置（agent、工具根目录）。若旧安装的 `config.yaml` 存在但无法读取， 就无从得知队列属于谁：`init` 会把它移到 `pending-learnings.unknown`，指出该文件，并删除搜索索引。尚未升级的检出中的旧队列也同样处理： 在该检出运行的下一个命令会把它移到 `pending-learnings.self` 并给出路径。反过来，当某个检出的 `init --self` 把 git 模式项目切换为单仓库模式，而另一个仍保留旧安装的检出从 main 取得了知识时，在那里运行的下一个 diff --git a/src/__tests__/e2e/self-mode-worktrees-808.test.ts b/src/__tests__/e2e/self-mode-worktrees-808.test.ts index bc656a51c..db656c33a 100644 --- a/src/__tests__/e2e/self-mode-worktrees-808.test.ts +++ b/src/__tests__/e2e/self-mode-worktrees-808.test.ts @@ -1569,6 +1569,33 @@ describe('a checkout an older git-mode install left (#808)', () => { expect(git(['log', '--all', '--name-only', '--format='], teamB.bare)).not.toContain('team-a-note-'); }); + it("carries the set-aside config's settings into the init that reruns after the replacement clone failed (#823 item 17)", async () => { + const install = setUpGitInstall(); + const { home, projectRoot } = install; + const teamA = serveTeamRepo(install, 'team-a'); + const teamB = serveTeamRepo(install, 'team-b'); + + const initA = await runCLI(['init', teamA.url, '--scope', 'project', '--force', '--agent', 'claude'], projectRoot, home); + expect(initA.code, initA.output).toBe(0); + const config = path.join(partitionOf(install), 'config.yaml'); + expect(fs.readFileSync(config, 'utf8')).toMatch(/enabledAgents:\n\s+- claude\n/); + + // Team B is unreachable, so the clone fails after the config was moved aside. + fs.renameSync(teamB.bare, `${teamB.bare}.offline`); + const failed = await runCLI(['init', teamB.url, '--scope', 'project', '--force'], projectRoot, home); + expect(failed.code, failed.output).toBe(1); + expect(failed.output).toContain('Clone failed'); + expect(fs.existsSync(config)).toBe(false); + fs.renameSync(`${teamB.bare}.offline`, teamB.bare); + + const initB = await runCLI(['init', teamB.url, '--scope', 'project', '--force', '--agent', 'codex'], projectRoot, home); + + expect(initB.code, initB.output).toBe(0); + const saved = fs.readFileSync(config, 'utf8'); + expect(saved).toContain(teamB.url); + expect(saved).toMatch(/enabledAgents:\n\s+- claude\n\s+- codex\n/); + }); + it("sets aside the queue under an unknown owner when init replaces a config it cannot read, and the next pull publishes none of it", async () => { const install = setUpGitInstall(); const { home, projectRoot } = install; diff --git a/src/__tests__/learnings-import-remnants.test.ts b/src/__tests__/learnings-import-remnants.test.ts index a5185f16b..f1dfe8db5 100644 --- a/src/__tests__/learnings-import-remnants.test.ts +++ b/src/__tests__/learnings-import-remnants.test.ts @@ -276,4 +276,23 @@ describe('publishing what maintenance changed (#823)', () => { expect(result).toEqual({ status: 'published' }); expect(await publishedContent(origin, 'learnings/kept-2026-01-01-aaa111.md')).toContain('confidence: 0.9'); }); + + it('stages a filename with [ or * as itself, not as a pattern that sweeps in stray files', async () => { + const { config, origin, checkout } = await setUp(); + await savePendingLearning(config, 'a[1].md', '---\ntitle: A\n---\nA.\n'); + await publishQueuedLearnings(config, 'alice'); + // Maintenance removed a tracked `a[1].md` and wrote a new `b*.md`. The strays + // match those names as glob patterns and nobody committed them. + const removed = path.join(checkout, 'learnings', 'a[1].md'); + fs.rmSync(removed); + const written = path.join(checkout, 'learnings', 'b*.md'); + fs.writeFileSync(written, '---\ntitle: B\n---\nB.\n'); + fs.writeFileSync(path.join(checkout, 'learnings', 'a1.md'), 'stray\n'); + fs.writeFileSync(path.join(checkout, 'learnings', 'bee.md'), 'stray\n'); + + const result = await publishLearningsMaintenance(config, '[teamai] Maintenance', [removed, written]); + + expect(result).toEqual({ status: 'published' }); + expect((await publishedFiles(origin)).filter((f) => f.endsWith('.md')).sort()).toEqual(['learnings/b*.md']); + }); }); diff --git a/src/init.ts b/src/init.ts index a263a6b1b..295eaab04 100644 --- a/src/init.ts +++ b/src/init.ts @@ -52,6 +52,23 @@ async function moveConfigAside(configPath: string, next: LocalConfig): Promise<v `${redactGitCredentials(next.repo.remote)}. If init stops before it finishes, run it again.`, ); } + +/** + * The config {@link moveConfigAside} set aside last. An init that stopped after + * it (a failed clone, an unknown --role) saved no config, so the rerun reads + * this one for the settings a re-init carries forward instead of dropping them. + */ +async function loadConfigSetAside(configPath: string): Promise<LocalConfig | null> { + let latest: string | undefined; + for (let n = 0, aside = `${configPath}.previous`; await pathExists(aside); aside = `${configPath}.previous.${++n}`) latest = aside; + if (!latest) return null; + try { + return LocalConfigSchema.parse(YAML.parse(await readFileSafe(latest) ?? '')); + } catch (e) { + log.debug(`Not carrying settings from ${latest}: ${(e as Error).message}`); + return null; + } +} import { log, spinner } from './utils/logger.js'; import { CLAUDE_TOOL_ID, @@ -64,6 +81,7 @@ import { REPORTS_BRANCH, type GlobalOptions, type LocalConfig, + LocalConfigSchema, type Scope, getTeamaiHome, getConfigPath, @@ -1317,12 +1335,20 @@ export async function init(options: GlobalOptions & { return; } const existingLocalConfig = await loadLocalConfigForScope(scope, projectRoot); + const teamaiHome = scope === 'project' && projectRoot + ? (existingLocalConfig?.dataHome ?? await resolveProjectDataHome(projectRoot)) + : getTeamaiHome(scope, projectRoot); + const existingConfigPath = path.join(teamaiHome, 'config.yaml'); + // The settings this re-init carries forward: the live config's, or, when an + // init stopped after moving it aside, the one it set aside (#823 item 17). + const carriedConfig = existingLocalConfig + ?? (await pathExists(existingConfigPath) ? null : await loadConfigSetAside(existingConfigPath)); let inheritUserScope: boolean | undefined; try { inheritUserScope = resolveInheritUserScope( scope, options.inheritUserScope, - existingLocalConfig?.inheritUserScope, + carriedConfig?.inheritUserScope, ); } catch (e) { log.error((e as Error).message); @@ -1332,9 +1358,6 @@ export async function init(options: GlobalOptions & { if (fallbackReason) { log.warn(fallbackReason); } - const teamaiHome = scope === 'project' && projectRoot - ? (existingLocalConfig?.dataHome ?? await resolveProjectDataHome(projectRoot)) - : getTeamaiHome(scope, projectRoot); printScopeSummary(scope, projectRoot, explicit); if (scope === 'project' && !(await isInsideGitRepo(process.cwd()))) { @@ -1342,7 +1365,6 @@ export async function init(options: GlobalOptions & { } // Step 0.5: Re-init guard — warn if config already exists - const existingConfigPath = path.join(teamaiHome, 'config.yaml'); if (await pathExists(existingConfigPath)) { log.warn(`teamai is already initialized for ${scope} scope at ${existingConfigPath}`); if (options.force) { @@ -1792,9 +1814,9 @@ export async function init(options: GlobalOptions & { const requestedAgents = normalizeAgentList(options.agent); if (requestedAgents.length > 0) { // As loaded before the clone: that config may have been moved aside since. - const prev = existingLocalConfig?.enabledAgents ?? []; + const prev = carriedConfig?.enabledAgents ?? []; localConfig.enabledAgents = [...new Set([...prev, ...requestedAgents])]; - localConfig.disabledAgents = (existingLocalConfig?.disabledAgents ?? []).filter((t) => !requestedAgents.includes(t)); + localConfig.disabledAgents = (carriedConfig?.disabledAgents ?? []).filter((t) => !requestedAgents.includes(t)); } // Carry the member's recorded tool roots across a re-init. `init` is @@ -1804,11 +1826,11 @@ export async function init(options: GlobalOptions & { // entry when the variable IS set. // A project-scope config with no record of its own starts from the user-scope // one: the root is a fact about this machine, and project hooks land in HOME. - const carriedToolRoots = existingLocalConfig?.toolRoots + const carriedToolRoots = carriedConfig?.toolRoots ?? (scope === 'project' ? (await loadLocalConfigForScope('user'))?.toolRoots : undefined); if (carriedToolRoots) localConfig.toolRoots = { ...carriedToolRoots }; recordClaudeConfigRoot(localConfig); - await releasePreviousClaudeRoot(currentConfig, existingLocalConfig, localConfig); + await releasePreviousClaudeRoot(currentConfig, carriedConfig, localConfig); await ensureDir(teamaiHome); if (scope !== 'project') await ensureDir(getTeamaiHomeDir()); diff --git a/src/utils/branch-worktree.ts b/src/utils/branch-worktree.ts index dba28bd40..13f181227 100644 --- a/src/utils/branch-worktree.ts +++ b/src/utils/branch-worktree.ts @@ -548,7 +548,8 @@ async function commitAndPushAt( ): Promise<PublishResult> { const git = createGit(wt); - await git.add(files); + // Literal: a filename with `[` or `*` would otherwise stage whatever it matches as a pattern. + await git.raw(['--literal-pathspecs', 'add', '--', ...files]); const status = await git.status(); // simple-git lists a staged rename (an archived learning) under `renamed`, not `staged`. const staged = status.staged.length + status.renamed.length; diff --git a/src/utils/learnings-publish.ts b/src/utils/learnings-publish.ts index af49a8122..39c8107d5 100644 --- a/src/utils/learnings-publish.ts +++ b/src/utils/learnings-publish.ts @@ -300,7 +300,7 @@ export async function publishLearningsMaintenance( // fail the whole `git add`, publishing nothing of this run. const removed = inCheckout.filter((rel) => !fs.existsSync(path.join(checkout, rel))); const tracked = removed.length === 0 ? new Set<string>() - : new Set((await createGit(checkout).raw(['ls-files', '-z', '--', ...removed])).split('\0').filter(Boolean)); + : new Set((await createGit(checkout).raw(['--literal-pathspecs', 'ls-files', '-z', '--', ...removed])).split('\0').filter(Boolean)); const files = inCheckout.filter((rel) => !removed.includes(rel) || tracked.has(rel.split(path.sep).join('/'))); if (files.length === 0) return { status: 'already-present' }; // `commitAndPush`, not `update`: maintenance already wrote into the worktree From 8523c0d377e7628f39d9626878b6d38cfcf13040 Mon Sep 17 00:00:00 2001 From: Saul Moro <saul.moro.gomez@gmail.com> Date: Sat, 26 Sep 2026 10:15:30 +0200 Subject: [PATCH 03/13] =?UTF-8?q?fix(learnings):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20HTTP=20maintenance,=20agent=20lists=20on=20a=20plai?= =?UTF-8?q?n=20rerun?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../e2e/self-mode-worktrees-808.test.ts | 29 +++++++++++++++++++ .../learnings-import-remnants.test.ts | 19 ++++++++++++ src/init.ts | 4 +++ src/utils/learnings-publish.ts | 12 ++++++-- 4 files changed, 62 insertions(+), 2 deletions(-) diff --git a/src/__tests__/e2e/self-mode-worktrees-808.test.ts b/src/__tests__/e2e/self-mode-worktrees-808.test.ts index db656c33a..e4dc3fc9b 100644 --- a/src/__tests__/e2e/self-mode-worktrees-808.test.ts +++ b/src/__tests__/e2e/self-mode-worktrees-808.test.ts @@ -1596,6 +1596,35 @@ describe('a checkout an older git-mode install left (#808)', () => { expect(saved).toMatch(/enabledAgents:\n\s+- claude\n\s+- codex\n/); }); + it('keeps the agent lists of the set-aside config when the rerun after a failed replacement clone names no --agent (#823 item 17)', async () => { + const install = setUpGitInstall(); + const { home, projectRoot } = install; + const teamA = serveTeamRepo(install, 'team-a'); + const teamB = serveTeamRepo(install, 'team-b'); + + const initA = await runCLI(['init', teamA.url, '--scope', 'project', '--force', '--agent', 'claude'], projectRoot, home); + expect(initA.code, initA.output).toBe(0); + const config = path.join(partitionOf(install), 'config.yaml'); + // What `uninstall --agent codex` records. + const written = fs.readFileSync(config, 'utf8'); + expect(written).toContain('disabledAgents: []\n'); + fs.writeFileSync(config, written.replace('disabledAgents: []\n', 'disabledAgents:\n - codex\n')); + + fs.renameSync(teamB.bare, `${teamB.bare}.offline`); + const failed = await runCLI(['init', teamB.url, '--scope', 'project', '--force'], projectRoot, home); + expect(failed.code, failed.output).toBe(1); + expect(fs.existsSync(config)).toBe(false); + fs.renameSync(`${teamB.bare}.offline`, teamB.bare); + + const initB = await runCLI(['init', teamB.url, '--scope', 'project', '--force'], projectRoot, home); + + expect(initB.code, initB.output).toBe(0); + const saved = fs.readFileSync(config, 'utf8'); + expect(saved).toContain(teamB.url); + expect(saved).toMatch(/enabledAgents:\n\s+- claude\n/); + expect(saved).toMatch(/disabledAgents:\n\s+- codex\n/); + }); + it("sets aside the queue under an unknown owner when init replaces a config it cannot read, and the next pull publishes none of it", async () => { const install = setUpGitInstall(); const { home, projectRoot } = install; diff --git a/src/__tests__/learnings-import-remnants.test.ts b/src/__tests__/learnings-import-remnants.test.ts index f1dfe8db5..f5dccf3e0 100644 --- a/src/__tests__/learnings-import-remnants.test.ts +++ b/src/__tests__/learnings-import-remnants.test.ts @@ -296,3 +296,22 @@ describe('publishing what maintenance changed (#823)', () => { expect((await publishedFiles(origin)).filter((f) => f.endsWith('.md')).sort()).toEqual(['learnings/b*.md']); }); }); + +describe('publishing what maintenance changed for an HTTP install (#823)', () => { + it('reports a failed publish, not a throw, after a prune removed a file from the HTTP cache', async () => { + const cache = path.join(tmp, 'http-cache'); + const removed = path.join(cache, 'learnings', 'stale-2026-01-01-aaa111.md'); + fs.mkdirSync(path.dirname(removed), { recursive: true }); + const config: LocalConfig = { + repo: { localPath: cache, remote: 'https://team.example/api', kind: 'http', url: 'https://team.example/api' }, + username: 'alice', + scope: 'user', + additionalRoles: [], + }; + writeInstallConfig(config); + + const result = await publishLearningsMaintenance(config, '[teamai] Maintenance', [removed]); + + expect(result.status).toBe('failed'); + }); +}); diff --git a/src/init.ts b/src/init.ts index 295eaab04..2e2ed5346 100644 --- a/src/init.ts +++ b/src/init.ts @@ -1817,6 +1817,10 @@ export async function init(options: GlobalOptions & { const prev = carriedConfig?.enabledAgents ?? []; localConfig.enabledAgents = [...new Set([...prev, ...requestedAgents])]; localConfig.disabledAgents = (carriedConfig?.disabledAgents ?? []).filter((t) => !requestedAgents.includes(t)); + } else { + // No --agent: the lists stand as they were, `uninstall --agent`'s exclusion included. + if (carriedConfig?.enabledAgents) localConfig.enabledAgents = [...carriedConfig.enabledAgents]; + if (carriedConfig?.disabledAgents) localConfig.disabledAgents = [...carriedConfig.disabledAgents]; } // Carry the member's recorded tool roots across a re-init. `init` is diff --git a/src/utils/learnings-publish.ts b/src/utils/learnings-publish.ts index 39c8107d5..aa6dee4b6 100644 --- a/src/utils/learnings-publish.ts +++ b/src/utils/learnings-publish.ts @@ -299,8 +299,16 @@ export async function publishLearningsMaintenance( // A removed file git never tracked has nothing to stage, and naming it would // fail the whole `git add`, publishing nothing of this run. const removed = inCheckout.filter((rel) => !fs.existsSync(path.join(checkout, rel))); - const tracked = removed.length === 0 ? new Set<string>() - : new Set((await createGit(checkout).raw(['--literal-pathspecs', 'ls-files', '-z', '--', ...removed])).split('\0').filter(Boolean)); + let tracked = new Set<string>(); + if (removed.length > 0) { + try { + tracked = new Set((await createGit(checkout).raw(['--literal-pathspecs', 'ls-files', '-z', '--', ...removed])).split('\0').filter(Boolean)); + } catch (e) { + // Not a checkout git can read, such as an HTTP install's cache: the same + // non-fatal result commitAndPush gives, never a throw after the removal. + return { status: 'failed', reason: failureReason(e) }; + } + } const files = inCheckout.filter((rel) => !removed.includes(rel) || tracked.has(rel.split(path.sep).join('/'))); if (files.length === 0) return { status: 'already-present' }; // `commitAndPush`, not `update`: maintenance already wrote into the worktree From 331d929629d6483634e6be17537f85277744dc91 Mon Sep 17 00:00:00 2001 From: Saul Moro <saul.moro.gomez@gmail.com> Date: Sat, 26 Sep 2026 10:38:42 +0200 Subject: [PATCH 04/13] =?UTF-8?q?fix(learnings):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20retry=20maintenance=20a=20busy=20lock=20or=20failed?= =?UTF-8?q?=20push=20kept=20local,=20keep=20remnants=20while=20origin=20is?= =?UTF-8?q?=20unreachable?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/designs/data-directory-layout.md | 3 +- docs/usage-guide.md | 2 +- docs/usage-guide.zh-CN.md | 2 +- .../learnings-import-remnants.test.ts | 82 +++++++++++- src/utils/learnings-publish.ts | 117 +++++++++++++++--- 5 files changed, 184 insertions(+), 22 deletions(-) diff --git a/docs/designs/data-directory-layout.md b/docs/designs/data-directory-layout.md index 2cf2c5a8e..591481627 100644 --- a/docs/designs/data-directory-layout.md +++ b/docs/designs/data-directory-layout.md @@ -506,7 +506,8 @@ Upgrading from the per-checkout layout: branch is the checkout's tracked files plus what `origin/teamai-learnings` has that the checkout's commit lacks, fetched first: the old checkout is never synced again, so it may miss a teammate's later import of the same MR - (#823 item 21). It runs only under the sync lock, before the queue is listed, + (#823 item 21). When that fetch fails, every such file stays where it is + until a run can fetch, so a stale ref never queues a duplicate. It runs only under the sync lock, before the queue is listed, and never on a dry run, which publishes nothing from the queue either and `pull` reports as `Would publish N queued learning(s)` (#823 item 20); any other file in the checkout is left alone. The error is also printed as a warning, because diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 9832d5647..bb51d7d0d 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -1244,7 +1244,7 @@ After `--update-quality`, review the generated `.draft.md` files and rename them While another teamai command holds the learnings or reports checkout's lock, `recall maintenance` and `recall promote` exit 1 without writing anything (`The learnings checkout is locked: …`). Run them again when that command finishes. -Maintenance and promote publish only the learnings they changed. A file in the learnings checkout that nobody committed stays out of their commit. +Maintenance and promote publish only the learnings they changed. A file in the learnings checkout that nobody committed stays out of their commit. When the publish cannot run or push (`Maintenance changes stay local for now: …`), the next `teamai pull` or `contribute` publishes the change, even with no learning queued. ### Promoting Learnings diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index adb18f516..db725581e 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -1149,7 +1149,7 @@ teamai recall maintenance --update-quality 另一个 teamai 命令持有 learnings 或 reports checkout 的锁时，`recall maintenance` 与 `recall promote` 会以退出码 1 停止，不写入任何内容（`The learnings checkout is locked: …`）。待该命令结束后再运行。 -maintenance 与 promote 只发布它们改动过的 learning。learnings 检出中无人提交的文件不会进入它们的提交。 +maintenance 与 promote 只发布它们改动过的 learning。learnings 检出中无人提交的文件不会进入它们的提交。发布无法进行或推送失败时（`Maintenance changes stay local for now: …`），下一次 `teamai pull` 或 `contribute` 会发布这些改动，即使队列中没有 learning。 ### 晋升 Learnings diff --git a/src/__tests__/learnings-import-remnants.test.ts b/src/__tests__/learnings-import-remnants.test.ts index f5dccf3e0..5b85ce1b5 100644 --- a/src/__tests__/learnings-import-remnants.test.ts +++ b/src/__tests__/learnings-import-remnants.test.ts @@ -12,7 +12,7 @@ import os from 'node:os'; import path from 'node:path'; import { simpleGit } from 'simple-git'; -import { getDataHome, SYNC_LOCK_FILENAME, type LocalConfig } from '../types.js'; +import { getDataHome, LEARNINGS_LOCK_FILENAME, SYNC_LOCK_FILENAME, type LocalConfig } from '../types.js'; import { learningsBranch } from '../utils/learnings-branch.js'; import { listPendingLearnings, savePendingLearning } from '../utils/pending-learnings.js'; import { publishLearningsMaintenance, publishQueuedLearnings } from '../utils/learnings-publish.js'; @@ -106,6 +106,35 @@ function plant(checkout: string, relPath: string, content: string): string { return file; } +/** Run `fn` with origin unreachable, as when the member is offline. */ +async function offline<T>(origin: string, fn: () => Promise<T>): Promise<T> { + fs.renameSync(origin, `${origin}.offline`); + try { + return await fn(); + } finally { + fs.renameSync(`${origin}.offline`, origin); + } +} + +/** Hold the learnings branch lock, as a publish running in another command does. */ +function holdLearningsLock(config: LocalConfig): () => void { + const lock = path.join(path.dirname(learningsBranch.dir(config)), LEARNINGS_LOCK_FILENAME); + fs.writeFileSync(lock, JSON.stringify({ pid: process.pid, startedAt: new Date().toISOString(), owner: 'a publish' })); + return () => fs.rmSync(lock, { force: true }); +} + +/** A teammate publishes `rel` under `learnings/` from their own clone. */ +async function teammatePublishes(origin: string, rel: string, content: string): Promise<void> { + const teammate = path.join(tmp, 'teammate'); + await simpleGit().clone(origin, teammate, ['--branch', 'teamai-learnings']); + await configureGit(teammate); + fs.mkdirSync(path.dirname(path.join(teammate, 'learnings', rel)), { recursive: true }); + fs.writeFileSync(path.join(teammate, 'learnings', rel), content); + await simpleGit(teammate).add(['.']); + await simpleGit(teammate).commit('teammate'); + await simpleGit(teammate).push('origin', 'teamai-learnings'); +} + /** What was warned from now on, one line per call. */ function watchWarnings(): () => string { const warn = vi.spyOn(log, 'warn'); @@ -211,6 +240,26 @@ describe('a learning an older import --from-mr left untracked in the learnings c expect(warned()).toContain('learnings/alpha/quokka-2026-09-21-ddd444.md'); }); + it('is kept, not queued, while origin cannot be fetched, so a teammate\'s import of the same merge request is still seen', async () => { + const { config, origin, checkout } = await setUp(); + await savePendingLearning(config, 'first-2026-09-19-aaa000.md', '---\ntitle: First\n---\nFirst.\n'); + await publishQueuedLearnings(config, 'alice'); + await teammatePublishes(origin, 'alpha/quokka-2026-09-21-ddd444.md', remnant('https://github.com/acme/app/pull/42', 'Quokka, by a teammate')); + const file = plant(checkout, '2026-09-20-Quokka-cache-warmup-before-deploy.md', remnant()); + + await offline(origin, () => publishQueuedLearnings(config, 'alice')); + + expect(fs.existsSync(file)).toBe(true); + expect(await listPendingLearnings(config)).toEqual([]); + + const before = await publishedFiles(origin); + await publishQueuedLearnings(config, 'alice'); + + expect(await publishedFiles(origin)).toEqual(before); + expect(fs.existsSync(file)).toBe(false); + expect(await listPendingLearnings(config)).toEqual([]); + }); + it('is removed, and published once, when the queue already holds the same content', async () => { const { config, origin, checkout } = await setUp(); await savePendingLearning(config, 'quokka-copy-2026-09-21-bbb222.md', remnant('https://github.com/acme/app/pull/77')); @@ -295,6 +344,37 @@ describe('publishing what maintenance changed (#823)', () => { expect(result).toEqual({ status: 'published' }); expect((await publishedFiles(origin)).filter((f) => f.endsWith('.md')).sort()).toEqual(['learnings/b*.md']); }); + + it('publishes a change another write kept it from publishing on the next publish, with nothing queued', async () => { + const { config, origin, checkout } = await setUp(); + await savePendingLearning(config, 'kept-2026-01-01-aaa111.md', '---\ntitle: Kept\nconfidence: 0.5\n---\nKept.\n'); + await publishQueuedLearnings(config, 'alice'); + const rewritten = path.join(checkout, 'learnings', 'kept-2026-01-01-aaa111.md'); + fs.writeFileSync(rewritten, '---\ntitle: Kept\nconfidence: 0.9\n---\nKept.\n'); + const release = holdLearningsLock(config); + const busy = await publishLearningsMaintenance(config, '[teamai] Maintenance', [rewritten]); + release(); + expect(busy).toEqual({ status: 'busy' }); + + await publishQueuedLearnings(config, 'alice'); + + expect(await publishedContent(origin, 'learnings/kept-2026-01-01-aaa111.md')).toContain('confidence: 0.9'); + expect(await simpleGit(checkout).raw(['status', '--porcelain'])).toBe(''); + }); + + it('pushes a change it committed and could not push on the next publish, with nothing queued', async () => { + const { config, origin, checkout } = await setUp(); + await savePendingLearning(config, 'stale-2026-01-01-aaa111.md', '---\ntitle: Stale\n---\nStale.\n'); + await publishQueuedLearnings(config, 'alice'); + const pruned = path.join(checkout, 'learnings', 'stale-2026-01-01-aaa111.md'); + fs.rmSync(pruned); + const failed = await offline(origin, () => publishLearningsMaintenance(config, '[teamai] Prune 1 learning(s)', [pruned])); + expect(failed.status).toBe('failed'); + + await publishQueuedLearnings(config, 'alice'); + + expect(await publishedFiles(origin)).not.toContain('learnings/stale-2026-01-01-aaa111.md'); + }); }); describe('publishing what maintenance changed for an HTTP install (#823)', () => { diff --git a/src/utils/learnings-publish.ts b/src/utils/learnings-publish.ts index aa6dee4b6..4a81056e2 100644 --- a/src/utils/learnings-publish.ts +++ b/src/utils/learnings-publish.ts @@ -9,6 +9,7 @@ import path from 'node:path'; import fs from 'node:fs'; import type { SimpleGit } from 'simple-git'; +import { z } from 'zod'; import { ensureDir } from './fs.js'; import { parseFrontmatter } from './frontmatter.js'; @@ -108,6 +109,14 @@ async function publishUnderSyncLock( throw new Error(`Unhandled queue listing: ${JSON.stringify(unhandled)}`); } } + // What a maintenance run changed and could not publish goes first, and + // whether or not anything is queued: rerunning maintenance finds nothing to change. + if (locked && !dryRun) { + const maintenance = await publishRecordedMaintenance(localConfig); + if (maintenance.status === 'published') log.success('Published earlier maintenance changes to the learnings branch'); + else if (maintenance.status !== 'already-present') log.debug(`[learnings] recorded maintenance changes stay local: ${maintenance.status}`); + } + const queued = listing.queued; if (queued.length === 0) { return { published: [], remaining: 0 }; @@ -214,8 +223,11 @@ async function queueImportRemnants(localConfig: LocalConfig): Promise<void> { } // The checkout may be behind origin: the one an older teamai left in // `.teamai/` is never synced again (#823 item 21). What a teammate published - // since counts too. - for (const rel of await publishedSinceCheckout(git)) { + // since counts too, so without a current origin every remnant stays for a + // later run: a stale one would queue what a teammate already published. + const sinceCheckout = await publishedSinceCheckout(git); + if (sinceCheckout === null) return; + for (const rel of sinceCheckout) { const content = await git.show([`origin/${learningsBranch.branch}:${rel}`]).catch(() => null); if (content !== null) known.push({ label: rel, content, mr: sourceMr(parseFrontmatter(content).data) }); } @@ -256,39 +268,36 @@ async function queueImportRemnants(localConfig: LocalConfig): Promise<void> { /** * The learnings origin has and the checkout's commit lacks or holds another - * version of, after a best-effort fetch. None when origin cannot be read. + * version of, fetched just now. Null when origin cannot be fetched or compared. */ -async function publishedSinceCheckout(git: SimpleGit): Promise<string[]> { +async function publishedSinceCheckout(git: SimpleGit): Promise<string[] | null> { try { await fetchTrackingRef(git, learningsBranch.branch); - } catch (e) { - log.debug(`[learnings] fetch failed, comparing with the last fetched ${learningsBranch.branch}: ${failureReason(e)}`); - } - try { const diff = await git.raw(['diff', '-z', '--name-only', '--no-renames', '--diff-filter=AM', 'HEAD', `origin/${learningsBranch.branch}`, '--', 'learnings']); return diff.split('\0').filter((f) => f.endsWith('.md')); } catch (e) { - log.debug(`[learnings] cannot compare the checkout with origin/${learningsBranch.branch}: ${failureReason(e)}`); - return []; + log.debug(`[learnings] cannot compare the checkout with origin/${learningsBranch.branch}; leaving what an older import --from-mr left for a later run: ${failureReason(e)}`); + return null; } } /** - * Publish whatever maintenance just changed in the learnings worktree. + * Publish whatever maintenance just changed in the learnings worktree, and + * whatever an earlier run changed and could not publish. * * Pruning, promotion and confidence write-backs used to mutate a checkout * nothing pushes, so their result reached no teammate and the next realign * could undo it. They now write into the worktree, and this is what makes the - * change leave the machine. + * change leave the machine. The files are changed before the branch lock is + * taken, and a rerun finds nothing left to change, so each run records what it + * changed first and the record goes only once origin has it: a busy lock or a + * failed push leaves it for the next publish, contributions and pulls included. */ export async function publishLearningsMaintenance( localConfig: LocalConfig, message: string, changed: readonly string[], ): Promise<PublishResult> { - // Only the files maintenance wrote or removed, never all of `learnings/`: the - // checkout may hold files nobody committed, such as a learning an older - // import --from-mr left there, and they would ride along in this commit (#823). const checkout = learningsBranch.dir(localConfig); const inCheckout = changed .map((file) => path.relative(checkout, file)) @@ -296,8 +305,79 @@ export async function publishLearningsMaintenance( if (inCheckout.length < changed.length) { log.debug(`[learnings] maintenance changed files outside ${checkout}; not publishing those`); } - // A removed file git never tracked has nothing to stage, and naming it would - // fail the whole `git add`, publishing nothing of this run. + if (!learningsBranch.enabled(localConfig)) { + // kind: 'http' has no branch to retry against. + return inCheckout.length === 0 ? { status: 'already-present' } : commitMaintenance(localConfig, message, inCheckout); + } + if (inCheckout.length > 0) { + try { + await recordMaintenance(checkout, { message, files: inCheckout }); + } catch (e) { + log.debug(`[learnings] cannot record maintenance changes for a retry: ${failureReason(e)}`); + return commitMaintenance(localConfig, message, inCheckout); + } + } + return publishRecordedMaintenance(localConfig); +} + +/** What one maintenance run changed, relative to the learnings checkout. */ +const MaintenanceRecord = z.object({ message: z.string(), files: z.array(z.string()) }); +type MaintenanceRecord = z.infer<typeof MaintenanceRecord>; + +/** + * Where maintenance records wait: in the checkout's own git directory, which + * git never shows or commits and which goes with the checkout, and with the + * changes they describe. One file per run, so a publish removes only what it read. + */ +async function maintenanceRecordsDir(checkout: string): Promise<string> { + const gitPath = (await createGit(checkout).raw(['rev-parse', '--git-path', 'teamai-maintenance'])).trim(); + return path.resolve(checkout, gitPath); +} + +async function recordMaintenance(checkout: string, record: MaintenanceRecord): Promise<void> { + const dir = await maintenanceRecordsDir(checkout); + await ensureDir(dir); + const name = `${Date.now()}-${process.pid}-${Math.random().toString(36).slice(2, 10)}.json`; + await fs.promises.writeFile(path.join(dir, name), JSON.stringify(record), 'utf-8'); +} + +/** Publish every recorded maintenance change, dropping the records once origin has them. Never throws. */ +async function publishRecordedMaintenance(localConfig: LocalConfig): Promise<PublishResult> { + const checkout = learningsBranch.dir(localConfig); + try { + if (!fs.existsSync(checkout)) return { status: 'already-present' }; + const dir = await maintenanceRecordsDir(checkout); + const records: Array<MaintenanceRecord & { file: string }> = []; + for (const name of await fs.promises.readdir(dir).catch(() => [])) { + const file = path.join(dir, name); + const parsed = MaintenanceRecord.safeParse(JSON.parse(await fs.promises.readFile(file, 'utf-8'))); + if (parsed.success) records.push({ ...parsed.data, file }); + else log.debug(`[learnings] skipping unreadable maintenance record ${file}`); + } + if (records.length === 0) return { status: 'already-present' }; + const message = records.length === 1 ? records[0].message : `[teamai] Publish ${records.length} learnings maintenance runs`; + const result = await commitMaintenance(localConfig, message, [...new Set(records.flatMap((r) => r.files))]); + if (result.status === 'published' || result.status === 'already-present') { + for (const record of records) await fs.promises.rm(record.file, { force: true }); + } + return result; + } catch (e) { + return { status: 'failed', reason: failureReason(e) }; + } +} + +/** + * Commit and push `files`, relative to the learnings checkout. Also pushes what + * an earlier attempt committed, when none of them is left to stage. + */ +async function commitMaintenance(localConfig: LocalConfig, message: string, inCheckout: string[]): Promise<PublishResult> { + // Only the files maintenance wrote or removed, never all of `learnings/`: the + // checkout may hold files nobody committed, such as a learning an older + // import --from-mr left there, and they would ride along in this commit (#823). + const checkout = learningsBranch.dir(localConfig); + // A removed file git does not track has nothing to stage, and naming it would + // fail the whole `git add`, publishing nothing of this run: one it never + // tracked, or one an earlier attempt already committed the removal of. const removed = inCheckout.filter((rel) => !fs.existsSync(path.join(checkout, rel))); let tracked = new Set<string>(); if (removed.length > 0) { @@ -310,7 +390,8 @@ export async function publishLearningsMaintenance( } } const files = inCheckout.filter((rel) => !removed.includes(rel) || tracked.has(rel.split(path.sep).join('/'))); - if (files.length === 0) return { status: 'already-present' }; + // kind: 'http' has no branch an earlier attempt could have committed to. + if (files.length === 0 && !learningsBranch.enabled(localConfig)) return { status: 'already-present' }; // `commitAndPush`, not `update`: maintenance already wrote into the worktree // before this call, and `update` syncs with origin first, which can carry // those uncommitted files into a rebase or leave them behind. From 4defc3ee87375533173f65429b52005793e2a83c Mon Sep 17 00:00:00 2001 From: Saul Moro <saul.moro.gomez@gmail.com> Date: Sat, 26 Sep 2026 10:58:22 +0200 Subject: [PATCH 05/13] =?UTF-8?q?fix(learnings):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20queue=20remnants=20when=20origin=20has=20no=20learn?= =?UTF-8?q?ings=20branch,=20never=20let=20one=20bad=20maintenance=20record?= =?UTF-8?q?=20or=20remnant=20block=20the=20rest?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/designs/data-directory-layout.md | 5 +- .../learnings-import-remnants.test.ts | 44 +++++++++++++ src/utils/learnings-publish.ts | 64 +++++++++++++++---- 3 files changed, 100 insertions(+), 13 deletions(-) diff --git a/docs/designs/data-directory-layout.md b/docs/designs/data-directory-layout.md index 591481627..d098e51db 100644 --- a/docs/designs/data-directory-layout.md +++ b/docs/designs/data-directory-layout.md @@ -507,7 +507,10 @@ Upgrading from the per-checkout layout: has that the checkout's commit lacks, fetched first: the old checkout is never synced again, so it may miss a teammate's later import of the same MR (#823 item 21). When that fetch fails, every such file stays where it is - until a run can fetch, so a stale ref never queues a duplicate. It runs only under the sync lock, before the queue is listed, + until a run can fetch, so a stale ref never queues a duplicate; when + `git ls-remote` shows origin has no such branch (an offline first publish + never pushed it), origin adds nothing and the files are queued. A file of + that shape that cannot be read is skipped, never holding back the rest. It runs only under the sync lock, before the queue is listed, and never on a dry run, which publishes nothing from the queue either and `pull` reports as `Would publish N queued learning(s)` (#823 item 20); any other file in the checkout is left alone. The error is also printed as a warning, because diff --git a/src/__tests__/learnings-import-remnants.test.ts b/src/__tests__/learnings-import-remnants.test.ts index 5b85ce1b5..bf031a357 100644 --- a/src/__tests__/learnings-import-remnants.test.ts +++ b/src/__tests__/learnings-import-remnants.test.ts @@ -260,6 +260,29 @@ describe('a learning an older import --from-mr left untracked in the learnings c expect(await listPendingLearnings(config)).toEqual([]); }); + it('is queued and published when origin has no learnings branch, as after an offline first publish', async () => { + const { config, origin, checkout } = await setUp(); + await simpleGit(origin).raw(['branch', '-D', 'teamai-learnings']); + const file = plant(checkout, '2026-09-20-Quokka-cache-warmup-before-deploy.md', remnant()); + + await publishQueuedLearnings(config, 'alice'); + + expect(fs.existsSync(file)).toBe(false); + expect((await publishedFiles(origin)).filter((f) => QUEUED_NAME.test(f))).toHaveLength(1); + expect(await listPendingLearnings(config)).toEqual([]); + }); + + it('is queued even when another file of the same shape cannot be read', async () => { + const { config, origin, checkout } = await setUp(); + const file = plant(checkout, '2026-09-20-Quokka-cache-warmup-before-deploy.md', remnant()); + fs.symlinkSync(path.join(tmp, 'nowhere.md'), path.join(checkout, 'learnings', '2026-09-19-Broken.md')); + + await publishQueuedLearnings(config, 'alice'); + + expect(fs.existsSync(file)).toBe(false); + expect((await publishedFiles(origin)).filter((f) => QUEUED_NAME.test(f))).toHaveLength(1); + }); + it('is removed, and published once, when the queue already holds the same content', async () => { const { config, origin, checkout } = await setUp(); await savePendingLearning(config, 'quokka-copy-2026-09-21-bbb222.md', remnant('https://github.com/acme/app/pull/77')); @@ -362,6 +385,27 @@ describe('publishing what maintenance changed (#823)', () => { expect(await simpleGit(checkout).raw(['status', '--porcelain'])).toBe(''); }); + it('publishes the readable records and removes, naming it, one a crash left truncated', async () => { + const { config, origin, checkout } = await setUp(); + await savePendingLearning(config, 'kept-2026-01-01-aaa111.md', '---\ntitle: Kept\nconfidence: 0.5\n---\nKept.\n'); + await publishQueuedLearnings(config, 'alice'); + const rewritten = path.join(checkout, 'learnings', 'kept-2026-01-01-aaa111.md'); + fs.writeFileSync(rewritten, '---\ntitle: Kept\nconfidence: 0.9\n---\nKept.\n'); + const release = holdLearningsLock(config); + expect(await publishLearningsMaintenance(config, '[teamai] Maintenance', [rewritten])).toEqual({ status: 'busy' }); + release(); + const records = path.resolve(checkout, (await simpleGit(checkout).raw(['rev-parse', '--git-path', 'teamai-maintenance'])).trim()); + const truncated = path.join(records, '0-truncated.json'); + fs.writeFileSync(truncated, '{"message":"[teamai] Prune","fi'); + const warned = watchWarnings(); + + await publishQueuedLearnings(config, 'alice'); + + expect(await publishedContent(origin, 'learnings/kept-2026-01-01-aaa111.md')).toContain('confidence: 0.9'); + expect(fs.readdirSync(records)).toEqual([]); + expect(warned()).toContain(truncated); + }); + it('pushes a change it committed and could not push on the next publish, with nothing queued', async () => { const { config, origin, checkout } = await setUp(); await savePendingLearning(config, 'stale-2026-01-01-aaa111.md', '---\ntitle: Stale\n---\nStale.\n'); diff --git a/src/utils/learnings-publish.ts b/src/utils/learnings-publish.ts index 4a81056e2..75eabb711 100644 --- a/src/utils/learnings-publish.ts +++ b/src/utils/learnings-publish.ts @@ -11,7 +11,7 @@ import fs from 'node:fs'; import type { SimpleGit } from 'simple-git'; import { z } from 'zod'; -import { ensureDir } from './fs.js'; +import { ensureDir, writeFileAtomic } from './fs.js'; import { parseFrontmatter } from './frontmatter.js'; import { createGit } from './git.js'; import { learningsBranch } from './learnings-branch.js'; @@ -205,7 +205,12 @@ async function queueImportRemnants(localConfig: LocalConfig): Promise<void> { const named = IMPORT_REMNANT_NAME.exec(path.posix.basename(rel)); if (!named) continue; const file = path.join(checkout, rel); - const content = await fs.promises.readFile(file, 'utf-8'); + // One that cannot be read, such as a dangling link, must not hold back the rest on every run. + const content = await fs.promises.readFile(file, 'utf-8').catch((e: unknown) => { + log.debug(`[learnings] cannot read ${file}: ${failureReason(e)}`); + return null; + }); + if (content === null) continue; const { data } = parseFrontmatter(content); const mr = sourceMr(data); if (mr === null) continue; @@ -244,7 +249,7 @@ async function queueImportRemnants(localConfig: LocalConfig): Promise<void> { for (const remnant of remnants) { const covered = known.find((k) => k.content === remnant.content || k.mr === remnant.mr); if (covered) { - await fs.promises.rm(remnant.file, { force: true }); + if (!await removeRemnant(remnant.file)) continue; log.warn(`Removed ${remnant.file}, which an older teamai import --from-mr left unpublished: ${covered.label} already has it.`); continue; } @@ -254,7 +259,8 @@ async function queueImportRemnants(localConfig: LocalConfig): Promise<void> { log.debug(`[learnings] could not queue ${remnant.file}: ${saved.status}`); break; } - await fs.promises.rm(remnant.file, { force: true }); + // Queued even when it cannot go: the next run finds the queued copy and removes it then. + await removeRemnant(remnant.file); queued.push(remnant.file); known.push({ label: `the contribution queue (${relPath})`, content: remnant.content, mr: remnant.mr }); } @@ -266,13 +272,32 @@ async function queueImportRemnants(localConfig: LocalConfig): Promise<void> { } } +/** Whether the remnant is gone; a failure is logged and left for a later run. */ +async function removeRemnant(file: string): Promise<boolean> { + try { + await fs.promises.rm(file, { force: true }); + return true; + } catch (e) { + log.debug(`[learnings] cannot remove ${file}: ${failureReason(e)}`); + return false; + } +} + /** * The learnings origin has and the checkout's commit lacks or holds another - * version of, fetched just now. Null when origin cannot be fetched or compared. + * version of, fetched just now. None when origin has no learnings branch, as + * after an offline first publish. Null when origin cannot be reached or compared. */ async function publishedSinceCheckout(git: SimpleGit): Promise<string[] | null> { try { - await fetchTrackingRef(git, learningsBranch.branch); + try { + await fetchTrackingRef(git, learningsBranch.branch); + } catch (e) { + // A fetch of a branch origin lacks fails too; ls-remote tells the two + // apart, succeeding with no output only when origin answered without it. + if ((await git.raw(['ls-remote', '--heads', 'origin', `refs/heads/${learningsBranch.branch}`])).trim() !== '') throw e; + return []; + } const diff = await git.raw(['diff', '-z', '--name-only', '--no-renames', '--diff-filter=AM', 'HEAD', `origin/${learningsBranch.branch}`, '--', 'learnings']); return diff.split('\0').filter((f) => f.endsWith('.md')); } catch (e) { @@ -336,9 +361,9 @@ async function maintenanceRecordsDir(checkout: string): Promise<string> { async function recordMaintenance(checkout: string, record: MaintenanceRecord): Promise<void> { const dir = await maintenanceRecordsDir(checkout); - await ensureDir(dir); const name = `${Date.now()}-${process.pid}-${Math.random().toString(36).slice(2, 10)}.json`; - await fs.promises.writeFile(path.join(dir, name), JSON.stringify(record), 'utf-8'); + // Atomic: a record cut short by a crash would otherwise be read on every publish. + await writeFileAtomic(path.join(dir, name), JSON.stringify(record)); } /** Publish every recorded maintenance change, dropping the records once origin has them. Never throws. */ @@ -348,11 +373,26 @@ async function publishRecordedMaintenance(localConfig: LocalConfig): Promise<Pub if (!fs.existsSync(checkout)) return { status: 'already-present' }; const dir = await maintenanceRecordsDir(checkout); const records: Array<MaintenanceRecord & { file: string }> = []; - for (const name of await fs.promises.readdir(dir).catch(() => [])) { + // `.json` only: an interrupted atomic write leaves a `.tmp` copy beside them. + for (const name of (await fs.promises.readdir(dir).catch(() => [])).filter((n) => n.endsWith('.json'))) { const file = path.join(dir, name); - const parsed = MaintenanceRecord.safeParse(JSON.parse(await fs.promises.readFile(file, 'utf-8'))); - if (parsed.success) records.push({ ...parsed.data, file }); - else log.debug(`[learnings] skipping unreadable maintenance record ${file}`); + let problem: string; + try { + const parsed = MaintenanceRecord.safeParse(JSON.parse(await fs.promises.readFile(file, 'utf-8'))); + if (parsed.success) { + records.push({ ...parsed.data, file }); + continue; + } + problem = 'not a maintenance record'; + } catch (e) { + problem = failureReason(e); + } + // One bad record must not hold back the others on every later publish. + await fs.promises.rm(file, { force: true }).catch(() => undefined); + log.warn( + `Removed unreadable maintenance record ${file} (${problem}). A maintenance change it described ` + + `that is not on the learnings branch stays uncommitted in ${checkout}; check with git -C "${checkout}" status.`, + ); } if (records.length === 0) return { status: 'already-present' }; const message = records.length === 1 ? records[0].message : `[teamai] Publish ${records.length} learnings maintenance runs`; From 81af8becb279135f847c92a8d2fe7cdbba8da46b Mon Sep 17 00:00:00 2001 From: Saul Moro <saul.moro.gomez@gmail.com> Date: Sat, 26 Sep 2026 11:16:44 +0200 Subject: [PATCH 06/13] =?UTF-8?q?fix(learnings):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20dedup=20remnants=20against=20origin's=20tree,=20kee?= =?UTF-8?q?p=20a=20maintenance=20record=20a=20read=20failed=20on?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/designs/data-directory-layout.md | 10 +-- .../learnings-import-remnants.test.ts | 50 ++++++++++++++ src/utils/learnings-publish.ts | 67 +++++++++++-------- 3 files changed, 95 insertions(+), 32 deletions(-) diff --git a/docs/designs/data-directory-layout.md b/docs/designs/data-directory-layout.md index d098e51db..1184e65f8 100644 --- a/docs/designs/data-directory-layout.md +++ b/docs/designs/data-directory-layout.md @@ -501,12 +501,12 @@ Upgrading from the per-checkout layout: owning repo, so the shared one or the old `.teamai/<dirname>`, never another repository's), and queues each untracked file of exactly that shape, directly under `learnings/`, into the active namespace with `contribute`'s name, then - deletes the original. A file the branch (tracked, any namespace) or the queue + deletes the original. A file the branch (any namespace) or the queue already has, by `source_mr` or by content, is deleted without queueing. The - branch is the checkout's tracked files plus what `origin/teamai-learnings` - has that the checkout's commit lacks, fetched first: the old checkout is - never synced again, so it may miss a teammate's later import of the same MR - (#823 item 21). When that fetch fails, every such file stays where it is + branch is the tree of `origin/teamai-learnings`, fetched first, never the + checkout's tracked files: the old checkout is never synced again, so it may + miss a teammate's later import of the same MR, or still track one origin has + since deleted (#823 item 21). When that fetch fails, every such file stays where it is until a run can fetch, so a stale ref never queues a duplicate; when `git ls-remote` shows origin has no such branch (an offline first publish never pushed it), origin adds nothing and the files are queued. A file of diff --git a/src/__tests__/learnings-import-remnants.test.ts b/src/__tests__/learnings-import-remnants.test.ts index bf031a357..be0c3d4e3 100644 --- a/src/__tests__/learnings-import-remnants.test.ts +++ b/src/__tests__/learnings-import-remnants.test.ts @@ -240,6 +240,28 @@ describe('a learning an older import --from-mr left untracked in the learnings c expect(warned()).toContain('learnings/alpha/quokka-2026-09-21-ddd444.md'); }); + it('is queued and published, not removed, when origin has since deleted the learning from the same merge request that the checkout still tracks', async () => { + const { config, origin, checkout } = await setUp(); + await savePendingLearning(config, 'alpha/quokka-2026-09-21-aaa111.md', remnant('https://github.com/acme/app/pull/42', 'Quokka, reimported')); + await publishQueuedLearnings(config, 'alice'); + // A teammate deletes it on origin; this checkout still tracks its old copy. + const teammate = path.join(tmp, 'teammate'); + await simpleGit().clone(origin, teammate, ['--branch', 'teamai-learnings']); + await configureGit(teammate); + await simpleGit(teammate).rm(['learnings/alpha/quokka-2026-09-21-aaa111.md']); + await simpleGit(teammate).commit('teammate prunes'); + await simpleGit(teammate).push('origin', 'teamai-learnings'); + const file = plant(checkout, '2026-09-20-Quokka-cache-warmup-before-deploy.md', remnant()); + + await publishQueuedLearnings(config, 'alice'); + + expect(fs.existsSync(file)).toBe(false); + const published = (await publishedFiles(origin)).filter((f) => QUEUED_NAME.test(f)); + expect(published, (await publishedFiles(origin)).join('\n')).toHaveLength(1); + expect(await publishedContent(origin, published[0])).toBe(remnant()); + expect(await listPendingLearnings(config)).toEqual([]); + }); + it('is kept, not queued, while origin cannot be fetched, so a teammate\'s import of the same merge request is still seen', async () => { const { config, origin, checkout } = await setUp(); await savePendingLearning(config, 'first-2026-09-19-aaa000.md', '---\ntitle: First\n---\nFirst.\n'); @@ -406,6 +428,34 @@ describe('publishing what maintenance changed (#823)', () => { expect(warned()).toContain(truncated); }); + it('keeps, and warns about, a record it cannot read, while the readable records publish', async () => { + const { config, origin, checkout } = await setUp(); + await savePendingLearning(config, 'kept-2026-01-01-aaa111.md', '---\ntitle: Kept\nconfidence: 0.5\n---\nKept.\n'); + await publishQueuedLearnings(config, 'alice'); + const rewritten = path.join(checkout, 'learnings', 'kept-2026-01-01-aaa111.md'); + fs.writeFileSync(rewritten, '---\ntitle: Kept\nconfidence: 0.9\n---\nKept.\n'); + const release = holdLearningsLock(config); + expect(await publishLearningsMaintenance(config, '[teamai] Maintenance', [rewritten])).toEqual({ status: 'busy' }); + release(); + const records = path.resolve(checkout, (await simpleGit(checkout).raw(['rev-parse', '--git-path', 'teamai-maintenance'])).trim()); + // A read that fails, as on a permission or I/O error, says nothing about the record itself. + const unreadable = path.join(records, '0-unreadable.json'); + fs.writeFileSync(unreadable, JSON.stringify({ message: '[teamai] Prune', files: [] })); + const readFile = fs.promises.readFile; + vi.spyOn(fs.promises, 'readFile').mockImplementation(async (file, options) => { + if (file === unreadable) throw Object.assign(new Error(`EACCES: permission denied, open '${unreadable}'`), { code: 'EACCES' }); + return readFile(file, options); + }); + const warned = watchWarnings(); + + await publishQueuedLearnings(config, 'alice'); + + expect(await publishedContent(origin, 'learnings/kept-2026-01-01-aaa111.md')).toContain('confidence: 0.9'); + expect(fs.readdirSync(records)).toEqual(['0-unreadable.json']); + expect(warned()).toContain(unreadable); + expect(warned()).not.toContain('Removed'); + }); + it('pushes a change it committed and could not push on the next publish, with nothing queued', async () => { const { config, origin, checkout } = await setUp(); await savePendingLearning(config, 'stale-2026-01-01-aaa111.md', '---\ntitle: Stale\n---\nStale.\n'); diff --git a/src/utils/learnings-publish.ts b/src/utils/learnings-publish.ts index 75eabb711..71a705179 100644 --- a/src/utils/learnings-publish.ts +++ b/src/utils/learnings-publish.ts @@ -186,7 +186,7 @@ function sourceMr(frontmatter: Record<string, unknown>): string | null { * Only that exact shape moves: untracked, directly under `learnings/`, named * `<date>-<title>.md`, with `source_mr` in its frontmatter. Nothing else in the * checkout is touched; a learning on the branch is tracked, edited or not, so it - * never is. One is removed instead when the branch or the queue already has one + * never is. One is removed instead when origin's branch or the queue already has one * from the same merge request (a later import of it) or with the same content. The copy is queued before the original goes, so * a failure leaves it where it was. Never throws. */ @@ -219,23 +219,14 @@ async function queueImportRemnants(localConfig: LocalConfig): Promise<void> { } if (remnants.length === 0) return; - // What already covers a remnant: a learning on the branch, in any - // namespace, or one in the queue. - const known: Array<{ label: string; content: string; mr: string | null }> = []; - for (const rel of (await lsFiles([])).filter((f) => f.endsWith('.md'))) { - const content = await fs.promises.readFile(path.join(checkout, rel), 'utf-8').catch(() => null); - if (content !== null) known.push({ label: rel, content, mr: sourceMr(parseFrontmatter(content).data) }); - } - // The checkout may be behind origin: the one an older teamai left in - // `.teamai/` is never synced again (#823 item 21). What a teammate published - // since counts too, so without a current origin every remnant stays for a - // later run: a stale one would queue what a teammate already published. - const sinceCheckout = await publishedSinceCheckout(git); - if (sinceCheckout === null) return; - for (const rel of sinceCheckout) { - const content = await git.show([`origin/${learningsBranch.branch}:${rel}`]).catch(() => null); - if (content !== null) known.push({ label: rel, content, mr: sourceMr(parseFrontmatter(content).data) }); - } + // What already covers a remnant: a learning on origin, in any namespace, + // or one in the queue. Origin itself, fetched just now, not the checkout: + // the one an older teamai left in `.teamai/` is never synced again (#823 + // item 21), so it may lack what a teammate published since, or still track + // a copy origin has deleted, and a remnant removed against that copy is + // lost. Without a current origin every remnant stays for a later run. + const known = await learningsOnOrigin(git); + if (known === null) return; for (const rel of await listPendingLearnings(localConfig)) { const content = await readPendingLearning(localConfig, rel); if (content !== null) { @@ -283,12 +274,20 @@ async function removeRemnant(file: string): Promise<boolean> { } } +/** A learning that already covers a remnant, and where it is. */ +interface KnownLearning { + label: string; + content: string; + mr: string | null; +} + /** - * The learnings origin has and the checkout's commit lacks or holds another - * version of, fetched just now. None when origin has no learnings branch, as - * after an offline first publish. Null when origin cannot be reached or compared. + * Every learning on origin's learnings branch, fetched just now. None when + * origin has no learnings branch, as after an offline first publish. Null when + * origin cannot be reached or read. */ -async function publishedSinceCheckout(git: SimpleGit): Promise<string[] | null> { +async function learningsOnOrigin(git: SimpleGit): Promise<KnownLearning[] | null> { + const ref = `origin/${learningsBranch.branch}`; try { try { await fetchTrackingRef(git, learningsBranch.branch); @@ -298,10 +297,15 @@ async function publishedSinceCheckout(git: SimpleGit): Promise<string[] | null> if ((await git.raw(['ls-remote', '--heads', 'origin', `refs/heads/${learningsBranch.branch}`])).trim() !== '') throw e; return []; } - const diff = await git.raw(['diff', '-z', '--name-only', '--no-renames', '--diff-filter=AM', 'HEAD', `origin/${learningsBranch.branch}`, '--', 'learnings']); - return diff.split('\0').filter((f) => f.endsWith('.md')); + const files = (await git.raw(['ls-tree', '-r', '-z', '--name-only', ref, '--', 'learnings'])).split('\0').filter((f) => f.endsWith('.md')); + const learnings: KnownLearning[] = []; + for (const rel of files) { + const content = await git.show([`${ref}:${rel}`]); + learnings.push({ label: rel, content, mr: sourceMr(parseFrontmatter(content).data) }); + } + return learnings; } catch (e) { - log.debug(`[learnings] cannot compare the checkout with origin/${learningsBranch.branch}; leaving what an older import --from-mr left for a later run: ${failureReason(e)}`); + log.debug(`[learnings] cannot read ${ref}; leaving what an older import --from-mr left for a later run: ${failureReason(e)}`); return null; } } @@ -376,9 +380,17 @@ async function publishRecordedMaintenance(localConfig: LocalConfig): Promise<Pub // `.json` only: an interrupted atomic write leaves a `.tmp` copy beside them. for (const name of (await fs.promises.readdir(dir).catch(() => [])).filter((n) => n.endsWith('.json'))) { const file = path.join(dir, name); + let text: string; + try { + text = await fs.promises.readFile(file, 'utf-8'); + } catch (e) { + // A failed read says nothing about the record: keep it for a later publish. + log.warn(`Cannot read maintenance record ${file} (${failureReason(e)}); it stays for the next publish.`); + continue; + } let problem: string; try { - const parsed = MaintenanceRecord.safeParse(JSON.parse(await fs.promises.readFile(file, 'utf-8'))); + const parsed = MaintenanceRecord.safeParse(JSON.parse(text)); if (parsed.success) { records.push({ ...parsed.data, file }); continue; @@ -387,7 +399,8 @@ async function publishRecordedMaintenance(localConfig: LocalConfig): Promise<Pub } catch (e) { problem = failureReason(e); } - // One bad record must not hold back the others on every later publish. + // Read and malformed, so no later publish can use it either, and it must + // not hold back the others on every one. await fs.promises.rm(file, { force: true }).catch(() => undefined); log.warn( `Removed unreadable maintenance record ${file} (${problem}). A maintenance change it described ` + From ea4f1f44e3fe8e0c7b23d2b6c4fe9f9517c5dafb Mon Sep 17 00:00:00 2001 From: Saul Moro <saul.moro.gomez@gmail.com> Date: Sat, 26 Sep 2026 13:52:47 +0200 Subject: [PATCH 07/13] =?UTF-8?q?fix(learnings):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20commit=20only=20the=20published=20paths,=20not=20th?= =?UTF-8?q?e=20whole=20index=20(#823)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../learnings-import-remnants.test.ts | 29 +++++++++++++++++++ src/__tests__/reports-branch-readonly.test.ts | 19 ++++++++---- src/utils/branch-worktree.ts | 11 ++++--- src/utils/git.ts | 9 ++++-- 4 files changed, 56 insertions(+), 12 deletions(-) diff --git a/src/__tests__/learnings-import-remnants.test.ts b/src/__tests__/learnings-import-remnants.test.ts index be0c3d4e3..f7361b838 100644 --- a/src/__tests__/learnings-import-remnants.test.ts +++ b/src/__tests__/learnings-import-remnants.test.ts @@ -390,6 +390,35 @@ describe('publishing what maintenance changed (#823)', () => { expect((await publishedFiles(origin)).filter((f) => f.endsWith('.md')).sort()).toEqual(['learnings/b*.md']); }); + it('commits only what maintenance changed, leaving a file someone else staged in the checkout staged', async () => { + const { config, origin, checkout } = await setUp(); + await savePendingLearning(config, 'kept-2026-01-01-aaa111.md', '---\ntitle: Kept\nconfidence: 0.5\n---\nKept.\n'); + await publishQueuedLearnings(config, 'alice'); + plant(checkout, 'staged-by-hand.md', 'not maintenance\n'); + await simpleGit(checkout).add(['learnings/staged-by-hand.md']); + const rewritten = path.join(checkout, 'learnings', 'kept-2026-01-01-aaa111.md'); + fs.writeFileSync(rewritten, '---\ntitle: Kept\nconfidence: 0.9\n---\nKept.\n'); + + const result = await publishLearningsMaintenance(config, '[teamai] Maintenance', [rewritten]); + + expect(result).toEqual({ status: 'published' }); + expect(await publishedFiles(origin)).not.toContain('learnings/staged-by-hand.md'); + expect(await simpleGit(checkout).raw(['diff', '--cached', '--name-only'])).toBe('learnings/staged-by-hand.md\n'); + }); + + it('commits nothing when maintenance has nothing to stage, even with a file someone else staged in the checkout', async () => { + const { config, origin, checkout } = await setUp(); + plant(checkout, 'staged-by-hand.md', 'not maintenance\n'); + await simpleGit(checkout).add(['learnings/staged-by-hand.md']); + const prunedUntracked = path.join(checkout, 'learnings', 'draft.md'); + + const result = await publishLearningsMaintenance(config, '[teamai] Maintenance', [prunedUntracked]); + + expect(result).toEqual({ status: 'already-present' }); + expect(await publishedFiles(origin)).not.toContain('learnings/staged-by-hand.md'); + expect(await simpleGit(checkout).raw(['diff', '--cached', '--name-only'])).toBe('learnings/staged-by-hand.md\n'); + }); + it('publishes a change another write kept it from publishing on the next publish, with nothing queued', async () => { const { config, origin, checkout } = await setUp(); await savePendingLearning(config, 'kept-2026-01-01-aaa111.md', '---\ntitle: Kept\nconfidence: 0.5\n---\nKept.\n'); diff --git a/src/__tests__/reports-branch-readonly.test.ts b/src/__tests__/reports-branch-readonly.test.ts index 6627288b5..37103c533 100644 --- a/src/__tests__/reports-branch-readonly.test.ts +++ b/src/__tests__/reports-branch-readonly.test.ts @@ -142,6 +142,16 @@ describe('ensureReportsWorktree read-only cold start', () => { }); }); +/** + * `diff --cached --name-only -z -- <files>` lists `staged`, what the publish commits. + * Any other git call is `rev-list --count origin/<branch>..HEAD`, which the publish + * checks before reporting success: '0' is "nothing left to deliver". + */ +function stageInWorktree(staged: string[]): void { + mocks.worktreeGit.raw.mockImplementation(async (args: string[]) => + args.includes('diff') ? staged.map((f) => `${f}\0`).join('') : '0'); +} + describe('commitAndPushReports', () => { beforeEach(() => { vi.clearAllMocks(); @@ -150,10 +160,7 @@ describe('commitAndPushReports', () => { mocks.worktreeGit.commit.mockResolvedValue(undefined); mocks.worktreeGit.push.mockResolvedValue(undefined); mockCheckoutOfBusinessRepo(); - // `rev-list --count origin/<branch>..HEAD`: the publish confirms the ref - // moved before reporting success. '0' is "nothing left to deliver". - mocks.worktreeGit.raw.mockResolvedValue('0'); - mocks.worktreeGit.status.mockResolvedValue({ staged: ['members/alice.yaml'], renamed: [] }); + stageInWorktree(['members/alice.yaml']); vi.mocked(acquireLock).mockResolvedValue(true); vi.mocked(releaseLock).mockResolvedValue(undefined); }); @@ -169,7 +176,7 @@ describe('commitAndPushReports', () => { }); it('pushes an unchanged report retry without making an empty commit', async () => { - mocks.worktreeGit.status.mockResolvedValue({ staged: [], renamed: [] }); + stageInWorktree([]); expect(await commitAndPushReports(config, 'retry stats', ['stats/alice.yaml'], { pushIfUnchanged: true })).toBe(true); expect(mocks.worktreeGit.commit).not.toHaveBeenCalled(); expect(mocks.worktreeGit.push).toHaveBeenCalledWith(['origin', 'teamai-reports']); @@ -177,7 +184,7 @@ describe('commitAndPushReports', () => { }); it('preserves the default no-op when no changes are staged', async () => { - mocks.worktreeGit.status.mockResolvedValue({ staged: [], renamed: [] }); + stageInWorktree([]); expect(await commitAndPushReports(config, 'unchanged member', ['members/'])).toBe(false); expect(mocks.worktreeGit.push).not.toHaveBeenCalled(); }); diff --git a/src/utils/branch-worktree.ts b/src/utils/branch-worktree.ts index 13f181227..001c03162 100644 --- a/src/utils/branch-worktree.ts +++ b/src/utils/branch-worktree.ts @@ -550,9 +550,12 @@ async function commitAndPushAt( // Literal: a filename with `[` or `*` would otherwise stage whatever it matches as a pattern. await git.raw(['--literal-pathspecs', 'add', '--', ...files]); - const status = await git.status(); - // simple-git lists a staged rename (an archived learning) under `renamed`, not `staged`. - const staged = status.staged.length + status.renamed.length; + // Only `files`: the checkout may hold a file someone else staged, and it must + // neither count as a change nor ride along in this commit. No paths would be the whole index. + const staged = files.length === 0 + ? 0 + : (await git.raw(['--literal-pathspecs', 'diff', '--cached', '--no-renames', '--name-only', '-z', '--', ...files])) + .split('\0').filter(Boolean).length; if (staged === 0 && !options.pushIfUnchanged && await nothingLeftToPush(git, spec)) { // Nothing to commit AND nothing to deliver. Those are two different things: // an earlier attempt may have committed exactly this content and failed to @@ -564,7 +567,7 @@ async function commitAndPushAt( // A retry may reconstruct the same tree as a previously committed // but unconfirmed push. It still needs a push, without an empty commit. - if (staged > 0) await commitSkippingHooks(git, message); + if (staged > 0) await commitSkippingHooks(git, message, files); // Push with fetch+rebase retry. Each member only writes <user>.yaml, so // rebase conflicts are effectively impossible; retries handle the pure diff --git a/src/utils/git.ts b/src/utils/git.ts index d3e685b66..2453c49a8 100644 --- a/src/utils/git.ts +++ b/src/utils/git.ts @@ -29,9 +29,14 @@ export function createGit(basePath?: string): SimpleGit { * * `--no-verify` is scoped to this git process. It does not write * `core.hooksPath` and does not change the user's ordinary `git commit`. + * + * With `paths`, commits those paths alone, taken literally, and leaves anything + * else staged in the checkout out of the commit and still staged. Without, it + * commits the whole index. */ -export function commitSkippingHooks(git: SimpleGit, message: string) { - return git.commit(message, { '--no-verify': null }); +export function commitSkippingHooks(git: SimpleGit, message: string, paths?: readonly string[]) { + if (paths === undefined) return git.commit(message, { '--no-verify': null }); + return git.raw(['--literal-pathspecs', 'commit', '--no-verify', '-m', message, '--', ...paths]); } /** From 52deae811efc5db00e6a199aaffae83715c989f0 Mon Sep 17 00:00:00 2001 From: Saul Moro <saul.moro.gomez@gmail.com> Date: Sat, 26 Sep 2026 14:06:00 +0200 Subject: [PATCH 08/13] =?UTF-8?q?fix(learnings):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20keep=20a=20staged=20file=20across=20the=20push-retr?= =?UTF-8?q?y=20rebase=20(#823)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../learnings-import-remnants.test.ts | 40 +++++++++++++++++++ src/utils/branch-worktree.ts | 16 +++++++- 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/src/__tests__/learnings-import-remnants.test.ts b/src/__tests__/learnings-import-remnants.test.ts index f7361b838..40b194404 100644 --- a/src/__tests__/learnings-import-remnants.test.ts +++ b/src/__tests__/learnings-import-remnants.test.ts @@ -406,6 +406,46 @@ describe('publishing what maintenance changed (#823)', () => { expect(await simpleGit(checkout).raw(['diff', '--cached', '--name-only'])).toBe('learnings/staged-by-hand.md\n'); }); + it('publishes after a teammate pushed first, leaving a file someone else staged in the checkout staged', async () => { + const { config, origin, checkout } = await setUp(); + await savePendingLearning(config, 'kept-2026-01-01-aaa111.md', '---\ntitle: Kept\nconfidence: 0.5\n---\nKept.\n'); + await publishQueuedLearnings(config, 'alice'); + plant(checkout, 'staged-by-hand.md', 'not maintenance\n'); + await simpleGit(checkout).add(['learnings/staged-by-hand.md']); + const rewritten = path.join(checkout, 'learnings', 'kept-2026-01-01-aaa111.md'); + fs.writeFileSync(rewritten, '---\ntitle: Kept\nconfidence: 0.9\n---\nKept.\n'); + // The push is non-fast-forward, so the publish rebases, which git refuses with anything staged. + await teammatePublishes(origin, 'bob-2026-01-02-bbb222.md', '---\ntitle: Bob\n---\nBob.\n'); + + const result = await publishLearningsMaintenance(config, '[teamai] Maintenance', [rewritten]); + + expect(result).toEqual({ status: 'published' }); + expect(await publishedContent(origin, 'learnings/kept-2026-01-01-aaa111.md')).toContain('confidence: 0.9'); + expect(await publishedFiles(origin)).toContain('learnings/bob-2026-01-02-bbb222.md'); + expect(await publishedFiles(origin)).not.toContain('learnings/staged-by-hand.md'); + expect(await simpleGit(checkout).raw(['diff', '--cached', '--name-only'])).toBe('learnings/staged-by-hand.md\n'); + expect(fs.readFileSync(path.join(checkout, 'learnings', 'staged-by-hand.md'), 'utf8')).toBe('not maintenance\n'); + }); + + it('refreshes an unpushed commit onto a teammate\'s push, leaving a file someone else staged in the checkout staged', async () => { + const { config, origin, checkout } = await setUp(); + await savePendingLearning(config, 'kept-2026-01-01-aaa111.md', '---\ntitle: Kept\nconfidence: 0.5\n---\nKept.\n'); + await publishQueuedLearnings(config, 'alice'); + const rewritten = path.join(checkout, 'learnings', 'kept-2026-01-01-aaa111.md'); + fs.writeFileSync(rewritten, '---\ntitle: Kept\nconfidence: 0.9\n---\nKept.\n'); + expect((await offline(origin, () => publishLearningsMaintenance(config, '[teamai] Maintenance', [rewritten]))).status).toBe('failed'); + // An edit to a tracked file, which a plain `stash apply` restores unstaged (a new file it stages again). + fs.writeFileSync(path.join(checkout, '.gitignore'), 'edited by hand\n'); + await simpleGit(checkout).add(['.gitignore']); + await teammatePublishes(origin, 'bob-2026-01-02-bbb222.md', '---\ntitle: Bob\n---\nBob.\n'); + + await learningsBranch.refresh(config, { pushIfCreated: false }); + + expect(fs.existsSync(path.join(checkout, 'learnings', 'bob-2026-01-02-bbb222.md'))).toBe(true); + expect(await simpleGit(checkout).raw(['rev-list', '--count', 'origin/teamai-learnings..HEAD'])).toBe('1\n'); + expect(await simpleGit(checkout).raw(['diff', '--cached', '--name-only'])).toBe('.gitignore\n'); + }); + it('commits nothing when maintenance has nothing to stage, even with a file someone else staged in the checkout', async () => { const { config, origin, checkout } = await setUp(); plant(checkout, 'staged-by-hand.md', 'not maintenance\n'); diff --git a/src/utils/branch-worktree.ts b/src/utils/branch-worktree.ts index 001c03162..6f0715a70 100644 --- a/src/utils/branch-worktree.ts +++ b/src/utils/branch-worktree.ts @@ -585,8 +585,13 @@ async function commitAndPushAt( log.debug(`[${spec.logTag}] push failed after ${attempt} attempts: ${(pushErr as Error).message}`); return { status: 'failed', reason: (pushErr as Error).message }; } + // The commit took only `files`: anything else staged or modified would + // make git refuse to rebase, on this attempt and every later one. + let carried: string | null = null; try { await fetchTrackingRef(git, spec.branch); + carried = await snapshotDirtyTree(git); + if (carried) await git.raw(['reset', '--hard', 'HEAD']); await git.rebase([`origin/${spec.branch}`]); } catch (rebaseErr) { log.debug(`[${spec.logTag}] rebase failed, retrying: ${(rebaseErr as Error).message}`); @@ -597,6 +602,7 @@ async function commitAndPushAt( // no rebase in progress } } + if (carried) await applyDirtySnapshot(spec, git, carried); } } return { status: 'failed', reason: `push did not land after ${MAX_PUSH_RETRIES} attempts` }; @@ -748,9 +754,15 @@ async function snapshotDirtyTree(git: SimpleGit): Promise<string | null> { async function applyDirtySnapshot(spec: BranchWorktreeSpec, git: SimpleGit, sha: string): Promise<void> { try { - await git.raw(['stash', 'apply', sha]); + // `--index` keeps what was staged staged. It refuses, touching nothing, + // when the staged changes no longer apply; then restore the files alone. + await git.raw(['stash', 'apply', '--index', sha]); } catch { - // apply conflicts leave UU paths; restoreConflictedFiles recovers them + try { + await git.raw(['stash', 'apply', sha]); + } catch { + // apply conflicts leave UU paths; restoreConflictedFiles recovers them + } } await restoreConflictedFiles(spec, git); } From f2f35c643940a733aaaec2ff9456886893ffee7f Mon Sep 17 00:00:00 2001 From: Saul Moro <saul.moro.gomez@gmail.com> Date: Sat, 26 Sep 2026 14:23:19 +0200 Subject: [PATCH 09/13] =?UTF-8?q?fix(learnings):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20read=20the=20queue=20for=20remnant=20dedup=20under?= =?UTF-8?q?=20the=20queue=20lock=20and=20ownership=20check;=20move=20a=20s?= =?UTF-8?q?tale=20config=20aside=20when=20init=20reuses=20a=20clone=20(#82?= =?UTF-8?q?3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/designs/data-directory-layout.md | 10 ++++- docs/usage-guide.md | 2 +- docs/usage-guide.zh-CN.md | 2 +- .../e2e/self-mode-worktrees-808.test.ts | 38 +++++++++++++++++++ .../learnings-import-remnants.test.ts | 16 ++++++++ src/init.ts | 35 +++++++++-------- src/utils/learnings-publish.ts | 16 +++++--- src/utils/pending-learnings.ts | 26 +++++++++++++ 8 files changed, 121 insertions(+), 24 deletions(-) diff --git a/docs/designs/data-directory-layout.md b/docs/designs/data-directory-layout.md index ca94c9227..5462cc05a 100644 --- a/docs/designs/data-directory-layout.md +++ b/docs/designs/data-directory-layout.md @@ -534,7 +534,12 @@ Upgrading from the per-checkout layout: until a run can fetch, so a stale ref never queues a duplicate; when `git ls-remote` shows origin has no such branch (an offline first publish never pushed it), origin adds nothing and the files are queued. A file of - that shape that cannot be read is skipped, never holding back the rest. It runs only under the sync lock, before the queue is listed, + that shape that cannot be read is skipped, never holding back the rest. The + queue is read under the queue lock, and only while the data home's config + still names the install the command loaded (`readPendingForInstall`, the + check `listPendingForInstall` makes): after `init` switched the project to + another team repository the queue holds that install's learnings, so while it + does, or while the lock stays busy, every such file stays where it is. It runs only under the sync lock, before the queue is listed, and never on a dry run, which publishes nothing from the queue either and `pull` reports as `Would publish N queued learning(s)` (#823 item 20); any other file in the checkout is left alone. The error is also printed as a warning, because @@ -618,7 +623,8 @@ a fresh install, so `init` checks for the file, and with learnings queued moves them to `pending-learnings.unknown` the same way, the warning naming that config; queued or not, it drops the search indexes, as on an owner change. When `init` is about to clone another owner's team repository where the old -install's clone was, it settles the old install right then, before the clone, +install's clone was, or reuses a clone of it that an earlier `init` left there, +it settles the old install right then, before the clone or the refresh, the same way (queue set aside, indexes dropped), and moves the old `config.yaml` to the first free `config.yaml.previous[.<n>]` instead of saving (#823 item 17). The save at the end still compares with the old install diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 148c2f6f1..3b362e4ea 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -428,7 +428,7 @@ against another team repository of the same kind does the same, to `pending-learnings.<kind>-<repo>` (for example `pending-learnings.git-github.com-org-team-a`); the same repository written another way (with or without `.git`, SSH or HTTPS) keeps the queue. Before it -clones the other team repository, `init` moves the old `config.yaml` to +clones the other team repository, or reuses a clone of it an earlier `init` left, `init` moves the old `config.yaml` to `config.yaml.previous` beside it and says so: if `init` stops before it saves the new config, every command asks for `teamai init` instead of running the old team's config against the new clone. Run `init` again: it carries that config's diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index a8a162751..c15118384 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -382,7 +382,7 @@ git 同一分支只能在一个 worktree 中检出，所以仓库的所有检出 learning 会被移到同一数据目录下的 `pending-learnings.<旧类型>`，新安装不会发布它们；`init` 会说明数量和位置，并删除为旧仓库构建的搜索索引（下一次 `recall` 会重建）。以同一类型对另一个团队仓库重新运行 `init` 也同样处理，队列移到 `pending-learnings.<类型>-<仓库>`（例如 `pending-learnings.git-github.com-org-team-a`）； -同一仓库换一种写法（带或不带 `.git`、SSH 或 HTTPS）不会移动队列。克隆另一个团队仓库之前， +同一仓库换一种写法（带或不带 `.git`、SSH 或 HTTPS）不会移动队列。克隆另一个团队仓库之前（或复用之前某次 `init` 留下的该仓库克隆之前）， `init` 会把旧的 `config.yaml` 移到旁边的 `config.yaml.previous` 并给出提示：若 `init` 在保存新配置前停止， 所有命令都会要求先运行 `teamai init`，而不会用旧团队的配置操作新的克隆。重新运行 `init` 即可：它会从 `config.yaml.previous` 沿用该配置的设置（agent、工具根目录）。若旧安装的 `config.yaml` 存在但无法读取， 就无从得知队列属于谁：`init` 会把它移到 `pending-learnings.unknown`，指出该文件，并删除搜索索引。尚未升级的检出中的旧队列也同样处理： diff --git a/src/__tests__/e2e/self-mode-worktrees-808.test.ts b/src/__tests__/e2e/self-mode-worktrees-808.test.ts index e4dc3fc9b..a7c3691be 100644 --- a/src/__tests__/e2e/self-mode-worktrees-808.test.ts +++ b/src/__tests__/e2e/self-mode-worktrees-808.test.ts @@ -1569,6 +1569,44 @@ describe('a checkout an older git-mode install left (#808)', () => { expect(git(['log', '--all', '--name-only', '--format='], teamB.bare)).not.toContain('team-a-note-'); }); + it("moves team A's config aside when init reuses a clone of team B an earlier init left beside it and then fails, and carries its settings on the rerun (#823 item 17)", async () => { + const install = setUpGitInstall(); + const { home, projectRoot } = install; + const teamA = serveTeamRepo(install, 'team-a'); + const teamB = serveTeamRepo(install, 'team-b'); + + const initA = await runCLI(['init', teamA.url, '--scope', 'project', '--force', '--agent', 'claude'], projectRoot, home); + expect(initA.code, initA.output).toBe(0); + const partition = partitionOf(install); + const config = path.join(partition, 'config.yaml'); + // What an init that recloned team B and stopped before saving left: team A's config beside team B's clone. + const clone = path.join(partition, 'team-repo'); + fs.rmSync(clone, { recursive: true, force: true }); + git(['clone', '-q', teamB.bare, clone], install.sandbox); + git(['remote', 'set-url', 'origin', teamB.url], clone); + // Without team B's rewrite the clone names team B itself, so init reuses it, + // and with HTTPS refused its refresh fails. + const gitconfig = path.join(home, '.gitconfig'); + const rewrites = fs.readFileSync(gitconfig, 'utf8'); + fs.writeFileSync(gitconfig, rewrites.replace(`[url "${teamB.bare}"]\n\tinsteadOf = ${teamB.url}\n`, '')); + + const failed = await runCLI(['init', teamB.url, '--scope', 'project', '--force'], projectRoot, home, { GIT_ALLOW_PROTOCOL: 'file' }); + + expect(failed.code, failed.output).toBe(1); + expect(failed.output).toContain('using existing clone'); + expect(failed.output).toContain('Failed to refresh existing clone'); + expect(fs.existsSync(config) ? fs.readFileSync(config, 'utf8') : '', failed.output).not.toContain(teamA.url); + expect(fs.readFileSync(`${config}.previous`, 'utf8')).toContain(teamA.url); + fs.writeFileSync(gitconfig, rewrites); + + const initB = await runCLI(['init', teamB.url, '--scope', 'project', '--force'], projectRoot, home); + + expect(initB.code, initB.output).toBe(0); + const saved = fs.readFileSync(config, 'utf8'); + expect(saved).toContain(teamB.url); + expect(saved).toMatch(/enabledAgents:\n\s+- claude\n/); + }); + it("carries the set-aside config's settings into the init that reruns after the replacement clone failed (#823 item 17)", async () => { const install = setUpGitInstall(); const { home, projectRoot } = install; diff --git a/src/__tests__/learnings-import-remnants.test.ts b/src/__tests__/learnings-import-remnants.test.ts index 40b194404..4072308e8 100644 --- a/src/__tests__/learnings-import-remnants.test.ts +++ b/src/__tests__/learnings-import-remnants.test.ts @@ -317,6 +317,22 @@ describe('a learning an older import --from-mr left untracked in the learnings c expect(fs.existsSync(file)).toBe(false); }); + it('is left where it is when init switched the install after this command loaded its config, even though the new queue has the same merge request', async () => { + const { config, origin, checkout } = await setUp(); + const file = plant(checkout, '2026-09-20-Quokka-cache-warmup-before-deploy.md', remnant()); + // What init to another team repo leaves: its config, and a learning its install queued. + const other: LocalConfig = { ...config, repo: { ...config.repo, remote: path.join(tmp, 'other.git') } }; + writeInstallConfig(other); + await savePendingLearning(other, 'quokka-2026-09-21-ccc444.md', remnant('https://github.com/acme/app/pull/42', 'Quokka, the other team')); + + const report = await publishQueuedLearnings(config, 'alice'); + + expect(report.installChanged).toBeDefined(); + expect(fs.readFileSync(file, 'utf8')).toBe(remnant()); + expect(await listPendingLearnings(other)).toEqual(['quokka-2026-09-21-ccc444.md']); + expect((await publishedFiles(origin)).filter((f) => f.endsWith('.md'))).toEqual([]); + }); + it('is left where it is while another command holds the sync lock, which would queue it twice', async () => { const { config, origin, checkout } = await setUp(); const file = plant(checkout, '2026-09-20-Quokka-cache-warmup-before-deploy.md', remnant()); diff --git a/src/init.ts b/src/init.ts index 2e2ed5346..2a4e0e392 100644 --- a/src/init.ts +++ b/src/init.ts @@ -1441,6 +1441,24 @@ export async function init(options: GlobalOptions & { // Step 3: Clone or link repo const defaultLocalPath = path.join(teamaiHome, 'team-repo'); const localPath = expandHome(defaultLocalPath); + // The clone init uses is another install's than the config beside it, whether + // it clones it now or reuses one an earlier init left: settle that install + // now, as the config save below would, instead of leaving its config to run + // against this clone should init stop first. + const settleReplacedInstall = async (): Promise<void> => { + const next: LocalConfig = { + repo: { localPath, remote: repoInfo.httpsUrl }, + username, + scope, + projectRoot, + additionalRoles: [], + ...(scope === 'project' ? { dataHome: teamaiHome } : {}), + }; + const replacesAnother = existingLocalConfig + ? !sameQueueOwner(queueOwner(existingLocalConfig), queueOwner(next)) + : await pathExists(existingConfigPath); + if (replacesAnother) await settleModeSwitch(existingLocalConfig, next, () => moveConfigAside(existingConfigPath, next)); + }; if (await pathExists(localPath)) { if (await isGitRepo(localPath)) { @@ -1474,6 +1492,7 @@ export async function init(options: GlobalOptions & { } } else { log.info(`Repo already exists at ${localPath}, using existing clone`); + await settleReplacedInstall(); // Refresh before resolveActiveProjects so selectors like `--project all` // expand against the current remote manifest, not a stale local snapshot // (re-running init after a new project is added would otherwise keep the @@ -1507,21 +1526,7 @@ export async function init(options: GlobalOptions & { } if (!await pathExists(localPath)) { - // The clone about to land here is another install's than the config beside - // it: settle that install now, as the config save below would, instead of - // leaving its config to run against this clone should init stop first. - const next: LocalConfig = { - repo: { localPath, remote: repoInfo.httpsUrl }, - username, - scope, - projectRoot, - additionalRoles: [], - ...(scope === 'project' ? { dataHome: teamaiHome } : {}), - }; - const replacesAnother = existingLocalConfig - ? !sameQueueOwner(queueOwner(existingLocalConfig), queueOwner(next)) - : await pathExists(existingConfigPath); - if (replacesAnother) await settleModeSwitch(existingLocalConfig, next, () => moveConfigAside(existingConfigPath, next)); + await settleReplacedInstall(); const cloneSpin = spinner('Cloning team repo...').start(); const cloneTarget = provider.name === 'git' diff --git a/src/utils/learnings-publish.ts b/src/utils/learnings-publish.ts index 71a705179..fba48208d 100644 --- a/src/utils/learnings-publish.ts +++ b/src/utils/learnings-publish.ts @@ -21,6 +21,7 @@ import { dropPendingLearning, listPendingForInstall, listPendingLearnings, + readPendingForInstall, readPendingLearning, savePendingLearning, } from './pending-learnings.js'; @@ -227,11 +228,16 @@ async function queueImportRemnants(localConfig: LocalConfig): Promise<void> { // lost. Without a current origin every remnant stays for a later run. const known = await learningsOnOrigin(git); if (known === null) return; - for (const rel of await listPendingLearnings(localConfig)) { - const content = await readPendingLearning(localConfig, rel); - if (content !== null) { - known.push({ label: `the contribution queue (${rel})`, content, mr: sourceMr(parseFrontmatter(content).data) }); - } + // The queue only while it is still this install's: after init switched the + // project to another team repository, it holds that install's learnings, and + // a remnant removed against one of them never reached this one's repository. + const queue = await readPendingForInstall(localConfig); + if (queue.status !== 'read') { + log.debug(`[learnings] leaving what an older import --from-mr left for a later run: the queue is ${queue.status}`); + return; + } + for (const { relPath, content } of queue.queued) { + known.push({ label: `the contribution queue (${relPath})`, content, mr: sourceMr(parseFrontmatter(content).data) }); } const { generateFilename, resolveLearningsSubdir } = await import('../contribute.js'); diff --git a/src/utils/pending-learnings.ts b/src/utils/pending-learnings.ts index 0cdb53862..e87ed457b 100644 --- a/src/utils/pending-learnings.ts +++ b/src/utils/pending-learnings.ts @@ -188,6 +188,32 @@ export async function listPendingForInstall( return locked.status === 'done' ? locked.value : locked; } +/** + * Every queued learning's content, when the queue is still the install's that + * `localConfig` was loaded from, as listPendingForInstall decides it. Read under + * the queue lock, so no learning another install queued after a switch is read + * as this one's. Unreadable entries are left out. + */ +export async function readPendingForInstall( + localConfig: LocalConfig, +): Promise< + | { status: 'read'; queued: Array<{ relPath: string; content: string }> } + | { status: 'busy'; lockPath: string } + | { status: 'changed'; configPath: string; cause: string } +> { + const locked = await withQueueLock(queueHome(localConfig), async () => { + const changed = await installChanged(localConfig); + if (changed) return { status: 'changed' as const, ...changed }; + const queued: Array<{ relPath: string; content: string }> = []; + for (const relPath of await listPendingLearnings(localConfig)) { + const content = await readPendingLearning(localConfig, relPath); + if (content !== null) queued.push({ relPath, content }); + } + return { status: 'read' as const, queued }; + }); + return locked.status === 'done' ? locked.value : locked; +} + /** * Every queued learning, as paths relative to `learnings/`, oldest entries * included. Hidden files and anything that is not Markdown are ignored, so a From dcbe57240bbb3f99fe08224af6bef2362f69d86f Mon Sep 17 00:00:00 2001 From: Saul Moro <saul.moro.gomez@gmail.com> Date: Sat, 26 Sep 2026 14:29:31 +0200 Subject: [PATCH 10/13] ci: re-run checks (flaky dry-run-load-path test, unrelated to this PR) From e29a54e2b57be08dc73e3584f67805b3e8cf2bf8 Mon Sep 17 00:00:00 2001 From: Saul Moro <saul.moro.gomez@gmail.com> Date: Sun, 27 Sep 2026 23:10:08 +0200 Subject: [PATCH 11/13] =?UTF-8?q?fix(learnings):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20keep=20staged=20files=20staged=20when=20the=20snaps?= =?UTF-8?q?hot=20restore=20conflicts;=20never=20publish=20a=20hand=20edit?= =?UTF-8?q?=20as=20a=20recorded=20maintenance=20run=20(#823)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/usage-guide.md | 2 +- docs/usage-guide.zh-CN.md | 2 +- .../learnings-import-remnants.test.ts | 113 ++++++++++++++++++ src/utils/branch-worktree.ts | 70 +++++++++++ src/utils/learnings-publish.ts | 42 ++++++- 5 files changed, 223 insertions(+), 6 deletions(-) diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 796925964..ca3e87d44 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -1274,7 +1274,7 @@ After `--update-quality`, review the generated `.draft.md` files and rename them While another teamai command holds the learnings or reports checkout's lock, `recall maintenance` and `recall promote` exit 1 without writing anything (`The learnings checkout is locked: …`). Run them again when that command finishes. -Maintenance and promote publish only the learnings they changed. A file in the learnings checkout that nobody committed stays out of their commit. When the publish cannot run or push (`Maintenance changes stay local for now: …`), the next `teamai pull` or `contribute` publishes the change, even with no learning queued. +Maintenance and promote publish only the learnings they changed. A file in the learnings checkout that nobody committed stays out of their commit. When the publish cannot run or push (`Maintenance changes stay local for now: …`), the next `teamai pull` or `contribute` publishes the change, even with no learning queued. A learning edited by hand after that run is not published as part of it: the edit stays uncommitted, and a warning names the file. A file someone staged in the checkout stays staged across the publish; when origin changed it too and it can no longer be staged as it was, it keeps its content as an unstaged change, and a warning names it. ### Promoting Learnings diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 26ce38257..5cd5e0418 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -1177,7 +1177,7 @@ teamai recall maintenance --update-quality 另一个 teamai 命令持有 learnings 或 reports checkout 的锁时，`recall maintenance` 与 `recall promote` 会以退出码 1 停止，不写入任何内容（`The learnings checkout is locked: …`）。待该命令结束后再运行。 -maintenance 与 promote 只发布它们改动过的 learning。learnings 检出中无人提交的文件不会进入它们的提交。发布无法进行或推送失败时（`Maintenance changes stay local for now: …`），下一次 `teamai pull` 或 `contribute` 会发布这些改动，即使队列中没有 learning。 +maintenance 与 promote 只发布它们改动过的 learning。learnings 检出中无人提交的文件不会进入它们的提交。发布无法进行或推送失败时（`Maintenance changes stay local for now: …`），下一次 `teamai pull` 或 `contribute` 会发布这些改动，即使队列中没有 learning。在那次运行之后被手动编辑过的 learning 不会作为它的一部分发布：该编辑保持未提交，警告会指出文件名。检出中有人暂存（staged）的文件在发布后仍保持暂存；若 origin 也改动了它、无法按原样重新暂存，它的内容会保留为未暂存的改动，警告会指出该文件。 ### 晋升 Learnings diff --git a/src/__tests__/learnings-import-remnants.test.ts b/src/__tests__/learnings-import-remnants.test.ts index 4072308e8..46bc4adcf 100644 --- a/src/__tests__/learnings-import-remnants.test.ts +++ b/src/__tests__/learnings-import-remnants.test.ts @@ -462,6 +462,60 @@ describe('publishing what maintenance changed (#823)', () => { expect(await simpleGit(checkout).raw(['diff', '--cached', '--name-only'])).toBe('.gitignore\n'); }); + it('keeps a file someone else staged staged when a teammate changed the same lines of it and the publish has to rebase', async () => { + const { config, origin, checkout } = await setUp(); + await savePendingLearning(config, 'kept-2026-01-01-aaa111.md', '---\ntitle: Kept\nconfidence: 0.5\n---\nKept.\n'); + await savePendingLearning(config, 'shared-2026-01-01-ccc333.md', 'one\ntwo\nthree\n'); + await publishQueuedLearnings(config, 'alice'); + const shared = path.join(checkout, 'learnings', 'shared-2026-01-01-ccc333.md'); + fs.writeFileSync(shared, 'one\ntwo by hand\nthree\n'); + await simpleGit(checkout).add(['learnings/shared-2026-01-01-ccc333.md']); + // Staged in part, and a file the teammate did not touch: it keeps exactly that staging. + const gitignore = path.join(checkout, '.gitignore'); + const stagedIgnore = `${fs.readFileSync(gitignore, 'utf8')}staged-by-hand\n`; + fs.writeFileSync(gitignore, stagedIgnore); + await simpleGit(checkout).add(['.gitignore']); + fs.writeFileSync(gitignore, `${stagedIgnore}not staged\n`); + const rewritten = path.join(checkout, 'learnings', 'kept-2026-01-01-aaa111.md'); + fs.writeFileSync(rewritten, '---\ntitle: Kept\nconfidence: 0.9\n---\nKept.\n'); + // The staged edit no longer applies on top of the teammate's, so `stash apply --index` refuses. + await teammatePublishes(origin, 'shared-2026-01-01-ccc333.md', 'one\ntwo by bob\nthree\n'); + const warned = watchWarnings(); + + const result = await publishLearningsMaintenance(config, '[teamai] Maintenance', [rewritten]); + + expect(result).toEqual({ status: 'published' }); + expect(await publishedContent(origin, 'learnings/shared-2026-01-01-ccc333.md')).toBe('one\ntwo by bob\nthree\n'); + expect(fs.readFileSync(shared, 'utf8')).toBe('one\ntwo by hand\nthree\n'); + expect(await simpleGit(checkout).raw(['diff', '--cached', '--name-only'])).toBe('.gitignore\nlearnings/shared-2026-01-01-ccc333.md\n'); + expect(await simpleGit(checkout).raw(['show', ':learnings/shared-2026-01-01-ccc333.md'])).toBe('one\ntwo by hand\nthree\n'); + expect(await simpleGit(checkout).raw(['show', ':.gitignore'])).toBe(stagedIgnore); + expect(fs.readFileSync(gitignore, 'utf8')).toBe(`${stagedIgnore}not staged\n`); + expect(warned()).toBe(''); + }); + + it('warns, naming it, when a file someone else staged merged with a teammate\'s change and can no longer be staged as it was', async () => { + const { config, origin, checkout } = await setUp(); + await savePendingLearning(config, 'kept-2026-01-01-aaa111.md', '---\ntitle: Kept\nconfidence: 0.5\n---\nKept.\n'); + await savePendingLearning(config, 'shared-2026-01-01-ccc333.md', 'one\ntwo\nthree\nfour\nfive\n'); + await publishQueuedLearnings(config, 'alice'); + const shared = path.join(checkout, 'learnings', 'shared-2026-01-01-ccc333.md'); + fs.writeFileSync(shared, 'one\ntwo by hand\nthree\nfour\nfive\n'); + await simpleGit(checkout).add(['learnings/shared-2026-01-01-ccc333.md']); + const rewritten = path.join(checkout, 'learnings', 'kept-2026-01-01-aaa111.md'); + fs.writeFileSync(rewritten, '---\ntitle: Kept\nconfidence: 0.9\n---\nKept.\n'); + // Inside the staged patch's context, so it no longer applies, and one line apart, so the files still merge. + await teammatePublishes(origin, 'shared-2026-01-01-ccc333.md', 'one\ntwo\nthree\nfour by bob\nfive\n'); + const warned = watchWarnings(); + + const result = await publishLearningsMaintenance(config, '[teamai] Maintenance', [rewritten]); + + expect(result).toEqual({ status: 'published' }); + expect(fs.readFileSync(shared, 'utf8')).toBe('one\ntwo by hand\nthree\nfour by bob\nfive\n'); + expect(await simpleGit(checkout).raw(['diff', '--cached', '--name-only'])).toBe(''); + expect(warned()).toContain('learnings/shared-2026-01-01-ccc333.md'); + }); + it('commits nothing when maintenance has nothing to stage, even with a file someone else staged in the checkout', async () => { const { config, origin, checkout } = await setUp(); plant(checkout, 'staged-by-hand.md', 'not maintenance\n'); @@ -492,6 +546,65 @@ describe('publishing what maintenance changed (#823)', () => { expect(await simpleGit(checkout).raw(['status', '--porcelain'])).toBe(''); }); + it('does not publish, as that maintenance run, an edit made by hand after another write kept it from publishing', async () => { + const { config, origin, checkout } = await setUp(); + await savePendingLearning(config, 'kept-2026-01-01-aaa111.md', '---\ntitle: Kept\nconfidence: 0.5\n---\nKept.\n'); + await savePendingLearning(config, 'other-2026-01-01-bbb222.md', '---\ntitle: Other\nconfidence: 0.5\n---\nOther.\n'); + await publishQueuedLearnings(config, 'alice'); + const rewritten = path.join(checkout, 'learnings', 'kept-2026-01-01-aaa111.md'); + const other = path.join(checkout, 'learnings', 'other-2026-01-01-bbb222.md'); + fs.writeFileSync(rewritten, '---\ntitle: Kept\nconfidence: 0.9\n---\nKept.\n'); + fs.writeFileSync(other, '---\ntitle: Other\nconfidence: 0.9\n---\nOther.\n'); + const release = holdLearningsLock(config); + expect(await publishLearningsMaintenance(config, '[teamai] Maintenance', [rewritten, other])).toEqual({ status: 'busy' }); + release(); + fs.writeFileSync(rewritten, '---\ntitle: Kept\nconfidence: 0.9\n---\nEdited by hand.\n'); + const warned = watchWarnings(); + + await publishQueuedLearnings(config, 'alice'); + + expect(await publishedContent(origin, 'learnings/kept-2026-01-01-aaa111.md')).not.toContain('Edited by hand'); + expect(await publishedContent(origin, 'learnings/other-2026-01-01-bbb222.md')).toContain('confidence: 0.9'); + expect(fs.readFileSync(rewritten, 'utf8')).toContain('Edited by hand'); + expect(warned()).toContain(path.join('learnings', 'kept-2026-01-01-aaa111.md')); + const records = path.resolve(checkout, (await simpleGit(checkout).raw(['rev-parse', '--git-path', 'teamai-maintenance'])).trim()); + expect(fs.readdirSync(records)).toEqual([]); + }); + + it('publishes an edit a newer maintenance run recorded over an older record of the same file', async () => { + const { config, origin, checkout } = await setUp(); + await savePendingLearning(config, 'kept-2026-01-01-aaa111.md', '---\ntitle: Kept\nconfidence: 0.5\n---\nKept.\n'); + await publishQueuedLearnings(config, 'alice'); + const rewritten = path.join(checkout, 'learnings', 'kept-2026-01-01-aaa111.md'); + const release = holdLearningsLock(config); + fs.writeFileSync(rewritten, '---\ntitle: Kept\nconfidence: 0.9\n---\nKept.\n'); + expect(await publishLearningsMaintenance(config, '[teamai] Confidence', [rewritten])).toEqual({ status: 'busy' }); + fs.writeFileSync(rewritten, '---\ntitle: Kept\nconfidence: 0.95\n---\nKept.\n'); + expect(await publishLearningsMaintenance(config, '[teamai] Confidence', [rewritten])).toEqual({ status: 'busy' }); + release(); + const warned = watchWarnings(); + + await publishQueuedLearnings(config, 'alice'); + + expect(await publishedContent(origin, 'learnings/kept-2026-01-01-aaa111.md')).toContain('confidence: 0.95'); + expect(warned()).toBe(''); + }); + + it('publishes a record written before records held content hashes, as before', async () => { + const { config, origin, checkout } = await setUp(); + await savePendingLearning(config, 'kept-2026-01-01-aaa111.md', '---\ntitle: Kept\nconfidence: 0.5\n---\nKept.\n'); + await publishQueuedLearnings(config, 'alice'); + fs.writeFileSync(path.join(checkout, 'learnings', 'kept-2026-01-01-aaa111.md'), '---\ntitle: Kept\nconfidence: 0.9\n---\nKept.\n'); + const records = path.resolve(checkout, (await simpleGit(checkout).raw(['rev-parse', '--git-path', 'teamai-maintenance'])).trim()); + fs.mkdirSync(records, { recursive: true }); + fs.writeFileSync(path.join(records, '0-legacy.json'), JSON.stringify({ message: '[teamai] Maintenance', files: [path.join('learnings', 'kept-2026-01-01-aaa111.md')] })); + + await publishQueuedLearnings(config, 'alice'); + + expect(await publishedContent(origin, 'learnings/kept-2026-01-01-aaa111.md')).toContain('confidence: 0.9'); + expect(fs.readdirSync(records)).toEqual([]); + }); + it('publishes the readable records and removes, naming it, one a crash left truncated', async () => { const { config, origin, checkout } = await setUp(); await savePendingLearning(config, 'kept-2026-01-01-aaa111.md', '---\ntitle: Kept\nconfidence: 0.5\n---\nKept.\n'); diff --git a/src/utils/branch-worktree.ts b/src/utils/branch-worktree.ts index 6f0715a70..98ece0ef0 100644 --- a/src/utils/branch-worktree.ts +++ b/src/utils/branch-worktree.ts @@ -746,6 +746,8 @@ async function restoreConflictedFiles(spec: BranchWorktreeSpec, git: SimpleGit): async function snapshotDirtyTree(git: SimpleGit): Promise<string | null> { try { const sha = (await git.raw(['stash', 'create'])).trim(); + // In debug.log, so a run killed before the snapshot is applied again can be recovered by hand. + if (sha.length > 0) log.debug(`[branch-worktree] uncommitted files saved as ${sha}; git stash apply --index ${sha} restores them`); return sha.length > 0 ? sha : null; } catch { return null; @@ -757,6 +759,8 @@ async function applyDirtySnapshot(spec: BranchWorktreeSpec, git: SimpleGit, sha: // `--index` keeps what was staged staged. It refuses, touching nothing, // when the staged changes no longer apply; then restore the files alone. await git.raw(['stash', 'apply', '--index', sha]); + await restoreConflictedFiles(spec, git); + return; } catch { try { await git.raw(['stash', 'apply', sha]); @@ -765,6 +769,72 @@ async function applyDirtySnapshot(spec: BranchWorktreeSpec, git: SimpleGit, sha: } } await restoreConflictedFiles(spec, git); + await restageSnapshotIndex(spec, git, sha); +} + +/** + * After a plain `stash apply`, put back what the snapshot had staged, and only + * where that is provably the same staging: + * - origin did not change the path: its staged entry is restored as it was, + * so a file staged in part stays staged in part; + * - origin changed it and the file holds exactly what was staged: staged again. + * Anything else (merged with origin's change, or edited past what was staged) + * stays unstaged, its content untouched, and is named in a warning. + */ +async function restageSnapshotIndex(spec: BranchWorktreeSpec, git: SimpleGit, sha: string): Promise<void> { + let wt: string; + let paths: string[]; + let base: Map<string, string>; + let staged: Map<string, string>; + let head: Map<string, string>; + try { + wt = (await git.raw(['rev-parse', '--show-toplevel'])).trim(); + // A `stash create` commit: first parent is HEAD at the time, second parent is the index. + paths = (await git.raw(['diff', '--name-only', '--no-renames', '-z', `${sha}^1`, `${sha}^2`])).split('\0').filter(Boolean); + if (paths.length === 0) return; + const entries = async (rev: string): Promise<Map<string, string>> => { + const listing = await git.raw(['--literal-pathspecs', 'ls-tree', '-z', rev, '--', ...paths]); + // `<mode> blob <id>\t<path>` -> path => `<mode>,<id>`, the form --cacheinfo takes. + return new Map(listing.split('\0').filter(Boolean).map((line) => { + const [meta, p] = line.split('\t'); + const [mode, , id] = meta.split(' '); + return [p, `${mode},${id}`]; + })); + }; + base = await entries(`${sha}^1`); + staged = await entries(`${sha}^2`); + head = await entries('HEAD'); + } catch (e) { + log.debug(`[${spec.logTag}] cannot read what the snapshot had staged: ${(e as Error).message}`); + return; + } + + const changed: string[] = []; + for (const p of paths) { + const entry = staged.get(p); + try { + if (head.get(p) === base.get(p)) { + await git.raw(entry === undefined ? ['update-index', '--force-remove', '--', p] : ['update-index', '--add', '--cacheinfo', `${entry},${p}`]); + continue; + } + const abs = path.join(wt, p); + const current = (await pathExists(abs)) ? (await git.raw(['hash-object', '--', abs])).trim() : undefined; + if (current !== entry?.split(',')[1]) { + changed.push(p); + } else { + await git.raw(entry === undefined ? ['update-index', '--force-remove', '--', p] : ['--literal-pathspecs', 'add', '--', p]); + } + } catch (e) { + log.debug(`[${spec.logTag}] cannot stage ${p} again: ${(e as Error).message}`); + changed.push(p); + } + } + if (changed.length > 0) { + log.warn( + `Staged changes in ${wt} could not stay staged while updating from origin: ${changed.join(', ')}. ` + + `The files keep your content as unstaged changes; review them with git -C "${wt}" diff and stage them again.`, + ); + } } /** diff --git a/src/utils/learnings-publish.ts b/src/utils/learnings-publish.ts index fba48208d..969ee1613 100644 --- a/src/utils/learnings-publish.ts +++ b/src/utils/learnings-publish.ts @@ -6,6 +6,7 @@ * the team later instead of being lost. A queue entry is dropped only once its * content is confirmed on origin, so a failure of any kind is always safe. */ +import { createHash } from 'node:crypto'; import path from 'node:path'; import fs from 'node:fs'; import type { SimpleGit } from 'simple-git'; @@ -346,7 +347,9 @@ export async function publishLearningsMaintenance( } if (inCheckout.length > 0) { try { - await recordMaintenance(checkout, { message, files: inCheckout }); + const contents: Record<string, string | null> = {}; + for (const rel of inCheckout) contents[rel] = await contentDigest(path.join(checkout, rel)); + await recordMaintenance(checkout, { message, files: inCheckout, contents }); } catch (e) { log.debug(`[learnings] cannot record maintenance changes for a retry: ${failureReason(e)}`); return commitMaintenance(localConfig, message, inCheckout); @@ -355,10 +358,23 @@ export async function publishLearningsMaintenance( return publishRecordedMaintenance(localConfig); } -/** What one maintenance run changed, relative to the learnings checkout. */ -const MaintenanceRecord = z.object({ message: z.string(), files: z.array(z.string()) }); +/** + * What one maintenance run changed, relative to the learnings checkout, and + * what it left in each file: a sha256, or null for a removal. Records from + * before `contents` existed publish their files as they are. + */ +const MaintenanceRecord = z.object({ + message: z.string(), + files: z.array(z.string()), + contents: z.record(z.string(), z.string().nullable()).optional(), +}); type MaintenanceRecord = z.infer<typeof MaintenanceRecord>; +async function contentDigest(file: string): Promise<string | null> { + if (!fs.existsSync(file)) return null; + return createHash('sha256').update(await fs.promises.readFile(file)).digest('hex'); +} + /** * Where maintenance records wait: in the checkout's own git directory, which * git never shows or commits and which goes with the checkout, and with the @@ -414,8 +430,26 @@ async function publishRecordedMaintenance(localConfig: LocalConfig): Promise<Pub ); } if (records.length === 0) return { status: 'already-present' }; + // A file publishes only as the newest run that changed it left it: an edit + // made since is someone's own, not this maintenance, and stays uncommitted. + // Names start with the time they were written. + records.sort((a, b) => (a.file < b.file ? -1 : a.file > b.file ? 1 : 0)); + const expected = new Map<string, string | null | undefined>(); + for (const record of records) for (const rel of record.files) expected.set(rel, record.contents?.[rel]); + const files: string[] = []; + const editedSince: string[] = []; + for (const [rel, digest] of expected) { + if (digest === undefined || (await contentDigest(path.join(checkout, rel))) === digest) files.push(rel); + else editedSince.push(rel); + } + if (editedSince.length > 0) { + log.warn( + `Not publishing the edit to ${editedSince.join(', ')}: it was made after the maintenance run that changed ` + + `the file, so it is not part of that run. It stays uncommitted in ${checkout}; check with git -C "${checkout}" diff.`, + ); + } const message = records.length === 1 ? records[0].message : `[teamai] Publish ${records.length} learnings maintenance runs`; - const result = await commitMaintenance(localConfig, message, [...new Set(records.flatMap((r) => r.files))]); + const result = await commitMaintenance(localConfig, message, files); if (result.status === 'published' || result.status === 'already-present') { for (const record of records) await fs.promises.rm(record.file, { force: true }); } From fe73cbeb5c47e2e8b033ff37792071d940b6c956 Mon Sep 17 00:00:00 2001 From: Saul Moro <saul.moro.gomez@gmail.com> Date: Sun, 27 Sep 2026 23:23:06 +0200 Subject: [PATCH 12/13] =?UTF-8?q?fix(learnings):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20resolve=20snapshot=20conflicts=20from=20the=20snaps?= =?UTF-8?q?hot=20without=20a=20reset,=20keep=20a=20conflicting=20staged=20?= =?UTF-8?q?file=20unstaged,=20no=20hand-edit=20warning=20for=20a=20merged?= =?UTF-8?q?=20maintenance=20commit=20(#823)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../learnings-import-remnants.test.ts | 50 ++++++++- src/utils/branch-worktree.ts | 106 ++++++++++++------ src/utils/learnings-publish.ts | 10 +- 3 files changed, 129 insertions(+), 37 deletions(-) diff --git a/src/__tests__/learnings-import-remnants.test.ts b/src/__tests__/learnings-import-remnants.test.ts index 46bc4adcf..97124e8e4 100644 --- a/src/__tests__/learnings-import-remnants.test.ts +++ b/src/__tests__/learnings-import-remnants.test.ts @@ -462,7 +462,7 @@ describe('publishing what maintenance changed (#823)', () => { expect(await simpleGit(checkout).raw(['diff', '--cached', '--name-only'])).toBe('.gitignore\n'); }); - it('keeps a file someone else staged staged when a teammate changed the same lines of it and the publish has to rebase', async () => { + it('keeps what someone else staged when a teammate changed the same lines and the publish has to rebase: an untouched file stays staged as it was, the conflicting one keeps its content unstaged and is named', async () => { const { config, origin, checkout } = await setUp(); await savePendingLearning(config, 'kept-2026-01-01-aaa111.md', '---\ntitle: Kept\nconfidence: 0.5\n---\nKept.\n'); await savePendingLearning(config, 'shared-2026-01-01-ccc333.md', 'one\ntwo\nthree\n'); @@ -487,11 +487,36 @@ describe('publishing what maintenance changed (#823)', () => { expect(result).toEqual({ status: 'published' }); expect(await publishedContent(origin, 'learnings/shared-2026-01-01-ccc333.md')).toBe('one\ntwo by bob\nthree\n'); expect(fs.readFileSync(shared, 'utf8')).toBe('one\ntwo by hand\nthree\n'); - expect(await simpleGit(checkout).raw(['diff', '--cached', '--name-only'])).toBe('.gitignore\nlearnings/shared-2026-01-01-ccc333.md\n'); - expect(await simpleGit(checkout).raw(['show', ':learnings/shared-2026-01-01-ccc333.md'])).toBe('one\ntwo by hand\nthree\n'); + // Staging it again would stage a revert of the teammate's line. + expect(await simpleGit(checkout).raw(['diff', '--cached', '--name-only'])).toBe('.gitignore\n'); expect(await simpleGit(checkout).raw(['show', ':.gitignore'])).toBe(stagedIgnore); expect(fs.readFileSync(gitignore, 'utf8')).toBe(`${stagedIgnore}not staged\n`); - expect(warned()).toBe(''); + expect(warned()).toContain('learnings/shared-2026-01-01-ccc333.md'); + expect(warned()).not.toContain('.gitignore'); + }); + + it('keeps every other uncommitted file when a removal someone else staged conflicts with a teammate\'s edit of that file', async () => { + const { config, origin, checkout } = await setUp(); + await savePendingLearning(config, 'kept-2026-01-01-aaa111.md', '---\ntitle: Kept\nconfidence: 0.5\n---\nKept.\n'); + await savePendingLearning(config, 'shared-2026-01-01-ccc333.md', 'one\ntwo\nthree\n'); + await publishQueuedLearnings(config, 'alice'); + await simpleGit(checkout).rm(['learnings/shared-2026-01-01-ccc333.md']); + const gitignore = path.join(checkout, '.gitignore'); + const editedIgnore = `${fs.readFileSync(gitignore, 'utf8')}not staged\n`; + fs.writeFileSync(gitignore, editedIgnore); + const rewritten = path.join(checkout, 'learnings', 'kept-2026-01-01-aaa111.md'); + fs.writeFileSync(rewritten, '---\ntitle: Kept\nconfidence: 0.9\n---\nKept.\n'); + // Removed here, edited there: the plain apply leaves a modify/delete conflict. + await teammatePublishes(origin, 'shared-2026-01-01-ccc333.md', 'one\ntwo by bob\nthree\n'); + const warned = watchWarnings(); + + const result = await publishLearningsMaintenance(config, '[teamai] Maintenance', [rewritten]); + + expect(result).toEqual({ status: 'published' }); + expect(fs.readFileSync(gitignore, 'utf8')).toBe(editedIgnore); + expect(fs.existsSync(path.join(checkout, 'learnings', 'shared-2026-01-01-ccc333.md'))).toBe(false); + expect(await simpleGit(checkout).raw(['show', 'HEAD:learnings/shared-2026-01-01-ccc333.md'])).toBe('one\ntwo by bob\nthree\n'); + expect(warned()).toContain('learnings/shared-2026-01-01-ccc333.md'); }); it('warns, naming it, when a file someone else staged merged with a teammate\'s change and can no longer be staged as it was', async () => { @@ -571,6 +596,23 @@ describe('publishing what maintenance changed (#823)', () => { expect(fs.readdirSync(records)).toEqual([]); }); + it('pushes a committed maintenance change a refresh merged with a teammate\'s edit, without calling that a hand edit', async () => { + const { config, origin, checkout } = await setUp(); + await savePendingLearning(config, 'kept-2026-01-01-aaa111.md', '---\ntitle: Kept\nconfidence: 0.5\n---\nKept.\n'); + await publishQueuedLearnings(config, 'alice'); + const rewritten = path.join(checkout, 'learnings', 'kept-2026-01-01-aaa111.md'); + fs.writeFileSync(rewritten, '---\ntitle: Kept\nconfidence: 0.9\n---\nKept.\n'); + expect((await offline(origin, () => publishLearningsMaintenance(config, '[teamai] Maintenance', [rewritten]))).status).toBe('failed'); + await teammatePublishes(origin, 'kept-2026-01-01-aaa111.md', '---\ntitle: Kept\nconfidence: 0.5\n---\nKept, said bob.\n'); + await learningsBranch.refresh(config, { pushIfCreated: false }); + const warned = watchWarnings(); + + await publishQueuedLearnings(config, 'alice'); + + expect(await publishedContent(origin, 'learnings/kept-2026-01-01-aaa111.md')).toBe('---\ntitle: Kept\nconfidence: 0.9\n---\nKept, said bob.\n'); + expect(warned()).toBe(''); + }); + it('publishes an edit a newer maintenance run recorded over an older record of the same file', async () => { const { config, origin, checkout } = await setUp(); await savePendingLearning(config, 'kept-2026-01-01-aaa111.md', '---\ntitle: Kept\nconfidence: 0.5\n---\nKept.\n'); diff --git a/src/utils/branch-worktree.ts b/src/utils/branch-worktree.ts index 98ece0ef0..163ef63b9 100644 --- a/src/utils/branch-worktree.ts +++ b/src/utils/branch-worktree.ts @@ -590,7 +590,7 @@ async function commitAndPushAt( let carried: string | null = null; try { await fetchTrackingRef(git, spec.branch); - carried = await snapshotDirtyTree(git); + carried = await snapshotDirtyTree(spec, git); if (carried) await git.raw(['reset', '--hard', 'HEAD']); await git.rebase([`origin/${spec.branch}`]); } catch (rebaseErr) { @@ -743,11 +743,11 @@ async function restoreConflictedFiles(spec: BranchWorktreeSpec, git: SimpleGit): * returns a dangling commit). `git rebase --autostash` would push onto the * shared stash list, which other worktrees of this repo also see. */ -async function snapshotDirtyTree(git: SimpleGit): Promise<string | null> { +async function snapshotDirtyTree(spec: BranchWorktreeSpec, git: SimpleGit): Promise<string | null> { try { const sha = (await git.raw(['stash', 'create'])).trim(); // In debug.log, so a run killed before the snapshot is applied again can be recovered by hand. - if (sha.length > 0) log.debug(`[branch-worktree] uncommitted files saved as ${sha}; git stash apply --index ${sha} restores them`); + if (sha.length > 0) log.debug(`[${spec.logTag}] uncommitted files saved as ${sha}; git stash apply --index ${sha} restores them`); return sha.length > 0 ? sha : null; } catch { return null; @@ -759,80 +759,122 @@ async function applyDirtySnapshot(spec: BranchWorktreeSpec, git: SimpleGit, sha: // `--index` keeps what was staged staged. It refuses, touching nothing, // when the staged changes no longer apply; then restore the files alone. await git.raw(['stash', 'apply', '--index', sha]); - await restoreConflictedFiles(spec, git); return; } catch { try { await git.raw(['stash', 'apply', sha]); } catch { - // apply conflicts leave UU paths; restoreConflictedFiles recovers them + // apply conflicts leave unmerged paths; restoreConflictsFromSnapshot resolves them } } - await restoreConflictedFiles(spec, git); - await restageSnapshotIndex(spec, git, sha); + const conflicted = await restoreConflictsFromSnapshot(spec, git, sha); + if (conflicted !== null) await restageSnapshotIndex(spec, git, sha, conflicted); } +/** + * Resolve each stash-apply conflict to the snapshot's own copy of the path, + * left unstaged: its file, or no file where the snapshot had removed it (the + * version that removes is origin's, which HEAD holds). Unlike + * restoreConflictedFiles this needs no `--theirs` side, which a removal lacks, + * and on failure it resets nothing: it names the snapshot to restore from. + * Returns the conflicted paths, or null when they could not be resolved. + */ +async function restoreConflictsFromSnapshot(spec: BranchWorktreeSpec, git: SimpleGit, sha: string): Promise<string[] | null> { + let conflicted: string[] = []; + try { + conflicted = (await git.status()).conflicted ?? []; + if (conflicted.length === 0) return []; + const inSnapshot = new Set((await git.raw(['--literal-pathspecs', 'ls-tree', '-z', '--name-only', sha, '--', ...conflicted])).split('\0').filter(Boolean)); + const kept = conflicted.filter((p) => inSnapshot.has(p)); + const removed = conflicted.filter((p) => !inSnapshot.has(p)); + if (kept.length > 0) await git.raw(['--literal-pathspecs', 'checkout', sha, '--', ...kept]); + if (removed.length > 0) await git.raw(['--literal-pathspecs', 'rm', '-q', '-f', '--', ...removed]); + await git.raw(['--literal-pathspecs', 'reset', '-q', 'HEAD', '--', ...conflicted]); + return conflicted; + } catch (e) { + const wt = await git.raw(['rev-parse', '--show-toplevel']).then((out) => out.trim(), () => spec.worktreeDirname); + log.warn( + `Could not restore uncommitted files in ${wt} after updating from origin (${failureReason(e)})` + + (conflicted.length > 0 ? `; ${conflicted.join(', ')} may hold conflict markers` : '') + + `. Nothing was discarded: commit ${sha} holds them as they were; read one with git -C "${wt}" show ${sha}:<file>, ` + + `or run git -C "${wt}" stash apply --index ${sha} once the conflicts are cleared.`, + ); + return null; + } +} + +/** A tree entry, as `ls-tree` prints it. */ +type TreeEntry = { mode: string; id: string }; + /** * After a plain `stash apply`, put back what the snapshot had staged, and only * where that is provably the same staging: * - origin did not change the path: its staged entry is restored as it was, * so a file staged in part stays staged in part; - * - origin changed it and the file holds exactly what was staged: staged again. - * Anything else (merged with origin's change, or edited past what was staged) - * stays unstaged, its content untouched, and is named in a warning. + * - origin changed it, the apply merged without a conflict, and the file holds + * exactly what was staged: staged again. + * Anything else stays unstaged, its content untouched, and is named in a + * warning: staging a conflicted path again would stage a revert of origin's change. */ -async function restageSnapshotIndex(spec: BranchWorktreeSpec, git: SimpleGit, sha: string): Promise<void> { +async function restageSnapshotIndex(spec: BranchWorktreeSpec, git: SimpleGit, sha: string, conflicted: readonly string[]): Promise<void> { let wt: string; let paths: string[]; - let base: Map<string, string>; - let staged: Map<string, string>; - let head: Map<string, string>; + let base: Map<string, TreeEntry>; + let staged: Map<string, TreeEntry>; + let head: Map<string, TreeEntry>; try { wt = (await git.raw(['rev-parse', '--show-toplevel'])).trim(); // A `stash create` commit: first parent is HEAD at the time, second parent is the index. paths = (await git.raw(['diff', '--name-only', '--no-renames', '-z', `${sha}^1`, `${sha}^2`])).split('\0').filter(Boolean); if (paths.length === 0) return; - const entries = async (rev: string): Promise<Map<string, string>> => { + const entries = async (rev: string): Promise<Map<string, TreeEntry>> => { const listing = await git.raw(['--literal-pathspecs', 'ls-tree', '-z', rev, '--', ...paths]); - // `<mode> blob <id>\t<path>` -> path => `<mode>,<id>`, the form --cacheinfo takes. + // `<mode> <type> <id>\t<path>` return new Map(listing.split('\0').filter(Boolean).map((line) => { const [meta, p] = line.split('\t'); const [mode, , id] = meta.split(' '); - return [p, `${mode},${id}`]; + return [p, { mode, id }]; })); }; base = await entries(`${sha}^1`); staged = await entries(`${sha}^2`); head = await entries('HEAD'); } catch (e) { - log.debug(`[${spec.logTag}] cannot read what the snapshot had staged: ${(e as Error).message}`); + log.warn( + `Could not tell which files were staged before updating from origin (${failureReason(e)}); ` + + `a staged change may now be unstaged, its content kept. git stash apply --index ${sha} in that checkout restores the staging.`, + ); return; } - const changed: string[] = []; + const stage = async (p: string, entry: TreeEntry | undefined, exact: boolean): Promise<void> => { + if (entry === undefined) await git.raw(['update-index', '--force-remove', '--', p]); + else if (exact) await git.raw(['update-index', '--add', '--cacheinfo', `${entry.mode},${entry.id},${p}`]); + else await git.raw(['--literal-pathspecs', 'add', '--', p]); + }; + const sameEntry = (a: TreeEntry | undefined, b: TreeEntry | undefined): boolean => a?.mode === b?.mode && a?.id === b?.id; + + const unstaged: string[] = []; for (const p of paths) { const entry = staged.get(p); try { - if (head.get(p) === base.get(p)) { - await git.raw(entry === undefined ? ['update-index', '--force-remove', '--', p] : ['update-index', '--add', '--cacheinfo', `${entry},${p}`]); + if (sameEntry(head.get(p), base.get(p))) { + await stage(p, entry, true); continue; } const abs = path.join(wt, p); const current = (await pathExists(abs)) ? (await git.raw(['hash-object', '--', abs])).trim() : undefined; - if (current !== entry?.split(',')[1]) { - changed.push(p); - } else { - await git.raw(entry === undefined ? ['update-index', '--force-remove', '--', p] : ['--literal-pathspecs', 'add', '--', p]); - } + if (conflicted.includes(p) || current !== entry?.id) unstaged.push(p); + else await stage(p, entry, false); } catch (e) { - log.debug(`[${spec.logTag}] cannot stage ${p} again: ${(e as Error).message}`); - changed.push(p); + log.debug(`[${spec.logTag}] cannot stage ${p} again: ${failureReason(e)}`); + unstaged.push(p); } } - if (changed.length > 0) { + if (unstaged.length > 0) { log.warn( - `Staged changes in ${wt} could not stay staged while updating from origin: ${changed.join(', ')}. ` + - `The files keep your content as unstaged changes; review them with git -C "${wt}" diff and stage them again.`, + `Origin changed files that were staged in ${wt}: ${unstaged.join(', ')}. They keep your content, unstaged. ` + + `Compare them with origin's version using git -C "${wt}" diff HEAD, then stage what you mean to keep.`, ); } } @@ -874,7 +916,7 @@ async function syncWorktree(spec: BranchWorktreeSpec, wt: string): Promise<void> let carried: string | null = null; if (dirty && ahead > 0) { - carried = await snapshotDirtyTree(git); + carried = await snapshotDirtyTree(spec, git); if (!carried) { log.debug(`[${spec.logTag}] uncommitted files block the refresh; using the local copy`); return; diff --git a/src/utils/learnings-publish.ts b/src/utils/learnings-publish.ts index 969ee1613..ff12306ab 100644 --- a/src/utils/learnings-publish.ts +++ b/src/utils/learnings-publish.ts @@ -370,6 +370,11 @@ const MaintenanceRecord = z.object({ }); type MaintenanceRecord = z.infer<typeof MaintenanceRecord>; +async function hasUncommittedChange(checkout: string, rel: string): Promise<boolean> { + const status = await createGit(checkout).raw(['--literal-pathspecs', 'status', '--porcelain', '-z', '--untracked-files=all', '--', rel]); + return status.length > 0; +} + async function contentDigest(file: string): Promise<string | null> { if (!fs.existsSync(file)) return null; return createHash('sha256').update(await fs.promises.readFile(file)).digest('hex'); @@ -440,12 +445,15 @@ async function publishRecordedMaintenance(localConfig: LocalConfig): Promise<Pub const editedSince: string[] = []; for (const [rel, digest] of expected) { if (digest === undefined || (await contentDigest(path.join(checkout, rel))) === digest) files.push(rel); + // Nothing uncommitted: an earlier attempt committed the run's change, and a refresh merged origin's into it. + else if (!(await hasUncommittedChange(checkout, rel))) files.push(rel); else editedSince.push(rel); } if (editedSince.length > 0) { log.warn( `Not publishing the edit to ${editedSince.join(', ')}: it was made after the maintenance run that changed ` + - `the file, so it is not part of that run. It stays uncommitted in ${checkout}; check with git -C "${checkout}" diff.`, + `the file, so it is not part of that run, and no later teamai run publishes it. It stays uncommitted in ` + + `${checkout}; review it with git -C "${checkout}" diff HEAD, then share it as a learning or discard it.`, ); } const message = records.length === 1 ? records[0].message : `[teamai] Publish ${records.length} learnings maintenance runs`; From d3c108d4caf7f89fe5e38343f855ed6bc6d8a05b Mon Sep 17 00:00:00 2001 From: Saul Moro <saul.moro.gomez@gmail.com> Date: Sun, 27 Sep 2026 23:41:02 +0200 Subject: [PATCH 13/13] =?UTF-8?q?fix(learnings):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20point=20the=20unpublished-edit=20warning=20at=20git?= =?UTF-8?q?=20status=20(#823)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/utils/learnings-publish.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/learnings-publish.ts b/src/utils/learnings-publish.ts index ff12306ab..51d47ffc4 100644 --- a/src/utils/learnings-publish.ts +++ b/src/utils/learnings-publish.ts @@ -453,7 +453,7 @@ async function publishRecordedMaintenance(localConfig: LocalConfig): Promise<Pub log.warn( `Not publishing the edit to ${editedSince.join(', ')}: it was made after the maintenance run that changed ` + `the file, so it is not part of that run, and no later teamai run publishes it. It stays uncommitted in ` + - `${checkout}; review it with git -C "${checkout}" diff HEAD, then share it as a learning or discard it.`, + `${checkout}; find it with git -C "${checkout}" status, then share it as a learning or discard it.`, ); } const message = records.length === 1 ? records[0].message : `[teamai] Publish ${records.length} learnings maintenance runs`;