diff --git a/src/__tests__/e2e/index-local-votes-1233.test.ts b/src/__tests__/e2e/index-local-votes-1233.test.ts index a661b4edf..d6be04c98 100644 --- a/src/__tests__/e2e/index-local-votes-1233.test.ts +++ b/src/__tests__/e2e/index-local-votes-1233.test.ts @@ -123,9 +123,11 @@ describe('local vote index freshness (#1233)', () => { if (reporting === 'unconfirmed') { // The real push lands, but a live lock prevents local confirmation. // The hook only controls the crash/lock boundary; it never edits votes. + // Extensionless Git hooks also run under an ESM package boundary. + fs.writeFileSync(path.join(origin, 'hooks', 'package.json'), '{"type":"module"}'); const hook = path.join(origin, 'hooks', 'post-receive'); fs.writeFileSync(hook, `#!${process.execPath} -require('node:fs').writeFileSync(${JSON.stringify(votePath + '.lock')}, ${JSON.stringify(JSON.stringify({ pid: process.pid, startedAt: new Date().toISOString(), owner: 'acceptance' }))}); +import('node:fs').then(fs => fs.writeFileSync(${JSON.stringify(votePath + '.lock')}, ${JSON.stringify(JSON.stringify({ pid: process.pid, startedAt: new Date().toISOString(), owner: 'acceptance' }))})); `); fs.chmodSync(hook, 0o755); const delivered = await runCli(home, ['pull'], sandbox); @@ -178,3 +180,71 @@ require('node:fs').writeFileSync(${JSON.stringify(votePath + '.lock')}, ${JSON.s else expect(localVotes(home).deltas).toEqual({ retry: { recalled_delta: 1, upvoted_delta: 1 } }); }, 120_000); }); + +describe('confirmed votes after narrow-clone pushes (#1233)', () => { + it.each(['full', 'single', 'shallow'] as const)('keeps delivered votes through contribute and recall rebuilds: %s', async (mode) => { + sandbox = fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-1233-')); + const origin = path.join(sandbox, 'origin.git'); + const seed = path.join(sandbox, 'seed'); + const home = path.join(sandbox, 'home'); + const env = machineEnv(home); + fs.mkdirSync(seed, { recursive: true }); + fs.writeFileSync(path.join(seed, 'teamai.yaml'), YAML.stringify({ + team: 'acme', repo: origin, provider: 'git', usageReport: true, + sharing: { recall: { enabled: true } }, + })); + const learnings = path.join(seed, 'learnings'); + fs.mkdirSync(learnings, { recursive: true }); + fs.writeFileSync(path.join(learnings, 'retry.md'), '---\ntitle: Retry failed requests\ndate: 2026-10-21\n---\nRetry failed requests with backoff.\n'); + git(['init', '-q', '-b', 'main'], seed, env); + git(['add', '.'], seed, env); git(['commit', '-q', '-m', 'fixture'], seed, env); + git(['clone', '-q', '--bare', seed, origin], sandbox, env); + fs.mkdirSync(path.join(home, '.claude'), { recursive: true }); + fs.writeFileSync(path.join(home, '.gitconfig'), '[user]\nname = alice\nemail = alice@example.invalid\n'); + const clone = path.join(home, '.teamai', 'team-repo'); + git(['clone', '-q', ...(mode === 'single' ? ['--single-branch'] : mode === 'shallow' ? ['--depth=1'] : []), 'file://' + origin, clone], sandbox, env); + fs.writeFileSync(path.join(home, '.teamai', 'config.yaml'), YAML.stringify({ + repo: { localPath: clone, remote: origin, kind: 'git' }, username: 'alice', scope: 'user', + updatePolicy: 'skip', enabledAgents: ['claude'], additionalRoles: [], + })); + // A prior recall credited by Stop; manual feedback is then sent via the real CLI. + const votesDir = path.join(home, '.teamai', 'user-votes'); + fs.mkdirSync(votesDir, { recursive: true }); + fs.writeFileSync(path.join(votesDir, 'alice.yaml'), YAML.stringify({ + version: 2, + votes: { retry: { recalled_count: 1, upvoted_count: 0, last_recalled_at: new Date().toISOString() } }, + deltas: { retry: { recalled_delta: 1, upvoted_delta: 0 } }, + })); + const initial = await runCli(home, ['pull'], sandbox); + expect(initial.code, initial.output).toBe(0); + const initialIndex = JSON.parse(fs.readFileSync(path.join(home, '.teamai', 'search-index.json'), 'utf8')); + expect(initialIndex.entries.find((e: { filename: string }) => e.filename === 'retry.md')).toMatchObject({ votes: 0.3 }); + const feedback = await runCli(home, ['recall', 'feedback', '--positive', 'retry'], sandbox); + expect(feedback.code, feedback.output).toBe(0); + const delivered = await runCli(home, ['pull'], sandbox); + expect(delivered.code, delivered.output).toBe(0); + const note = path.join(sandbox, 'note.md'); + fs.writeFileSync(note, '# Backoff\nRetry with backoff.\n'); + const contributed = await runCli(home, ['contribute', '--file', note, '--title', 'Backoff', '--scope', 'user'], sandbox); + expect(contributed.code, contributed.output).toBe(0); + const indexPath = path.join(home, '.teamai', 'search-index.json'); + const votes = () => JSON.parse(fs.readFileSync(indexPath, 'utf8')).entries.find((e: { filename: string }) => e.filename === 'retry.md').votes; + const afterContribute = votes(); + const remote = YAML.parse(git(['show', 'teamai-reports:votes/alice.yaml'], origin, env)); + const tracking = git(['rev-parse', 'refs/remotes/origin/teamai-reports'], clone, env); + const confirmed = git(['rev-parse', 'teamai-reports'], origin, env); + fs.unlinkSync(indexPath); + const recall = await runCli(home, ['recall', '--check', 'retry failed requests'], sandbox); + expect(recall.code, recall.output).toBe(0); + const afterRecall = votes(); + const deltas = localVotes(home).deltas; + const pull = await runCli(home, ['pull'], sandbox); + expect(pull.code, pull.output).toBe(0); + expect(tracking).toBe(confirmed); + expect(remote.votes.retry.upvoted_count).toBe(1); + expect((deltas as Record).retry).toBeUndefined(); + expect(afterContribute).toBe(1.3); + expect(afterRecall).toBe(1.3); + expect(votes()).toBe(1.3); + }, 120_000); +}); diff --git a/src/__tests__/search-index-local-votes.test.ts b/src/__tests__/search-index-local-votes.test.ts index 1d40e6ed8..b5f0ffe0b 100644 --- a/src/__tests__/search-index-local-votes.test.ts +++ b/src/__tests__/search-index-local-votes.test.ts @@ -11,6 +11,7 @@ import { indexInMemory } from '../utils/search-index.js'; import * as searchIndex from '../utils/search-index.js'; import { buildVizData } from '../viz.js'; import { acquireLock, releaseLock } from '../update.js'; +import * as gitUtils from '../utils/git.js'; import { indexableVotesDir } from '../utils/reports-branch.js'; vi.mock('../utils/reports-branch.js', () => ({ @@ -174,8 +175,64 @@ describe('index local votes (#1233)', () => { it('clamps a negative pending delta at zero and clears its timestamp in memory', async () => { await confirmed(snapshot(0, 1)); await pending(-2); const files = await readIndexVoteFiles(config); - const data = YAML.parse(files[0]); + const data = YAML.parse(files[0].content); expect(data.votes[docId].upvoted_count).toBe(0); expect(data.votes[docId].last_upvoted_at).toBeUndefined(); }); + it('keeps unrelated same-member v1 scores while applying local feedback', async () => { + await confirmed({ votes: { retry: { at: date }, other: { at: date } } }); + const before = await fs.readFile(path.join(config.repo.localPath, 'votes', 'alice.yaml'), 'utf8'); + await fs.mkdir(path.dirname(localPath), { recursive: true }); + await fs.writeFile(localPath, YAML.stringify({ version: 2, votes: { other: entry(0, 1) }, deltas: { other: { recalled_delta: 0, upvoted_delta: 1 } } })); + await fs.writeFile(path.join(dir, 'learnings', 'other.md'), '# Other\nOther knowledge.\n'); + const index = await indexInMemory({ learningsDir: path.join(dir, 'learnings'), voteScope: config }); + expect(index.entries.find(e => e.filename === 'retry.md')?.votes).toBe(1); + expect(index.entries.find(e => e.filename === 'other.md')?.votes).toBe(1.3); + expect(await fs.readFile(path.join(config.repo.localPath, 'votes', 'alice.yaml'), 'utf8')).toBe(before); + }); + it.each([false, true])('names a corrupt votes file, including with local deltas: %s', async (withLocal) => { + const { log } = await import('../utils/logger.js'); + const error = vi.spyOn(log, 'error').mockImplementation(() => {}); + try { + await confirmed(snapshot()); + const file = path.join(config.repo.localPath, 'votes', 'alice.yaml'); + await fs.writeFile(file, 'votes: ['); + git('add', '.'); git('commit', '-qm', 'corrupt'); git('update-ref', 'refs/remotes/origin/teamai-reports', 'HEAD'); + if (withLocal) await pending(); + expect((await indexed()).votes).toBe(withLocal ? 1.3 : 0); + expect(error).toHaveBeenCalledWith(`Failed to parse index votes: ${file}`); + } finally { error.mockRestore(); } + }); + + it('releases the votes lock before reading member blobs and keeps a coherent snapshot', async () => { + await confirmed(snapshot()); await pending(); + const createGit = gitUtils.createGit; + let checked = false; + const spy = vi.spyOn(gitUtils, 'createGit').mockImplementation((...args) => { + const client = createGit(...args); + return new Proxy(client, { + get(target, key, receiver) { + if (key !== 'raw') return Reflect.get(target, key, receiver); + return async (...rawArgs: unknown[]) => { + if (!checked) { + const held = await acquireLock(`${localPath}.lock`); + expect(held).toBe(true); + if (held) await releaseLock(`${localPath}.lock`); + // A writer can proceed during the slow read; this index must still + // use the local snapshot captured with its pinned remote ref. + await incrementUpvoted(localPath, [docId]); + checked = true; + } + return Reflect.apply(target.raw, target, rawArgs); + }; + }, + }); + }); + try { + expect((await indexed()).votes).toBe(2.6); + expect(checked).toBe(true); + } finally { spy.mockRestore(); } + expect((await indexed()).votes).toBe(3.6); + }); + }); diff --git a/src/contribute.ts b/src/contribute.ts index 95ba24ac6..d42650956 100644 --- a/src/contribute.ts +++ b/src/contribute.ts @@ -56,9 +56,6 @@ export async function rebuildIndexAfterContribute(localConfig: LocalConfig): Pro const repoPath = localConfig.repo.localPath; const docsRepoDir = path.join(repoPath, 'docs'); const rulesRepoDir = path.join(repoPath, 'rules'); - // Not another repository's reports checkout (#808). - const { indexableVotesDir } = await import('./utils/reports-branch.js'); - const votesDir = await indexableVotesDir(localConfig); // A workspace's children's too (#913). const activeLearningsNamespaces = await deliveredLearningsNamespaces(localConfig); @@ -81,7 +78,6 @@ export async function rebuildIndexAfterContribute(localConfig: LocalConfig): Pro rulesDir: (await pathExists(rulesRepoDir)) ? rulesRepoDir : undefined, // The docs and skills pull delivers here, not the whole trees (#707). ...await deliveredIndexSources(localConfig), - votesDir: votesDir && (await pathExists(votesDir)) ? votesDir : undefined, voteScope: localConfig, indexPath, children: localConfig.children?.fingerprint, diff --git a/src/pull.ts b/src/pull.ts index 91877f81f..c8620e4c1 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -1510,8 +1510,7 @@ async function pullForScope( const docsRepoDir = path.join(localConfig.repo.localPath, 'docs'); const rulesRepoDir = path.join(localConfig.repo.localPath, 'rules'); const skillsRepoDir = path.join(localConfig.repo.localPath, 'skills'); - const reportsRoot = await reportsRootReady; - const votesDir = reportsRoot ? path.join(reportsRoot, 'votes') : undefined; + await reportsRootReady; // user scope: sync learnings to ~/.teamai/learnings/ (legacy behavior) // project scope: use learnings directly from repo @@ -1590,7 +1589,6 @@ async function pullForScope( // An index already on disk follows the last source away (#1006): left // alone, it would keep serving what the member no longer receives. if (hasAnySource || effectiveCodebaseDir || await pathExists(indexPath)) { - const votesExist = votesDir ? await pathExists(votesDir) : false; const { buildIndex, dropOtherCheckoutIndexes } = await import('./utils/search-index.js'); // Other checkouts' indexes: this checkout's rebuild goes ahead without them. await dropOtherCheckoutIndexes(localConfig) @@ -1630,7 +1628,6 @@ async function pullForScope( unreadable: unresolved, children: localConfig.children?.fingerprint, codebaseDir: undefined, // codebase now served by teamwiki/ graph engine - votesDir: votesExist ? votesDir : undefined, voteScope: localConfig, indexPath, }); diff --git a/src/recall.ts b/src/recall.ts index 39326fc71..d67d51a81 100644 --- a/src/recall.ts +++ b/src/recall.ts @@ -422,12 +422,6 @@ async function loadOrBuildScopeIndex( const needsRebuild = !index || isLegacyIndex(index) || index.children !== localConfig.children?.fingerprint; // A legacy index is rebuilt even when no source can be seen, or it would serve what it holds whole (#1006). if (needsRebuild && (index || effectiveLearningsDir || await pathExists(path.join(localConfig.repo.localPath, 'docs')) || await pathExists(path.join(localConfig.repo.localPath, 'rules')) || await pathExists(path.join(localConfig.repo.localPath, 'skills')))) { - // Legacy votesDir callers use the reports checkout; voteScope also supplies - // local feedback when no reports votes directory exists yet. - // Not another repository's reports checkout (#808). - const { indexableVotesDir } = await import('./utils/reports-branch.js'); - const votesDir = await indexableVotesDir(localConfig); - const votesExist = votesDir !== undefined && await pathExists(votesDir); const docsDir = path.join(localConfig.repo.localPath, 'docs'); const rulesDir = path.join(localConfig.repo.localPath, 'rules'); const repoCodebaseDir = path.join(localConfig.repo.localPath, 'docs', 'team-codebase'); @@ -491,7 +485,6 @@ async function loadOrBuildScopeIndex( // The docs and skills pull delivers here, not the whole trees (#707). ...delivered, codebaseDir: undefined, // codebase now served by teamwiki/ graph engine - votesDir: votesExist ? votesDir : undefined, voteScope: localConfig, indexPath, unreadable, diff --git a/src/utils/branch-worktree.ts b/src/utils/branch-worktree.ts index 6161f8bf5..a8a306de5 100644 --- a/src/utils/branch-worktree.ts +++ b/src/utils/branch-worktree.ts @@ -643,7 +643,16 @@ async function commitAndPushAt( // when it refuses one. Do NOT re-check the remote-tracking ref here — it // is only updated through the remote's fetch refspec, so a `--single-branch` // clone would report failure for every successful side-branch push. + const trackingRef = `refs/remotes/origin/${spec.branch}`; + const before = await git.revparse(['--verify', trackingRef]).then(r => r.trim()).catch(() => ''); + const sent = (await git.revparse([`refs/heads/${spec.branch}`])).trim(); await git.push(['--progress', 'origin', spec.branch]); + // A narrow fetch refspec does not update this ref on push. Record only + // the commit actually accepted by origin, never an unpushed worktree HEAD. + // Compare-and-swap preserves a newer concurrent fetch (and a normal push + // may already have updated the ref). Failure cannot undo delivery. + await git.raw(['update-ref', trackingRef, sent, before || '0'.repeat(sent.length)]) + .catch(() => { /* Another updater owns the newer tracking snapshot. */ }); return { status: 'published' }; } catch (pushErr) { // Killed by the block timeout: origin stopped answering, which no diff --git a/src/utils/search-index.ts b/src/utils/search-index.ts index 3a35ec7a2..9fe97c204 100644 --- a/src/utils/search-index.ts +++ b/src/utils/search-index.ts @@ -355,9 +355,9 @@ async function aggregateVotes(votesDir?: string, voteScope?: LocalConfig): Promi ? await (await import('../votes.js')).readIndexVoteFiles(voteScope) : await Promise.all((votesDir ? await listFiles(votesDir) : []) .filter((file) => /\.ya?ml$/.test(file)) - .map((file) => readFileSafe(path.join(votesDir!, file)))); + .map(async (file) => ({ path: path.join(votesDir!, file), content: await readFileSafe(path.join(votesDir!, file)) }))); - for (const content of contents) { + for (const { path: votePath, content } of contents) { if (!content) continue; try { @@ -392,7 +392,7 @@ async function aggregateVotes(votesDir?: string, voteScope?: LocalConfig): Promi } } } catch { - log.error('Failed to parse index votes'); + log.error(`Failed to parse index votes: ${votePath}`); } } diff --git a/src/viz.ts b/src/viz.ts index 94181e827..8862ad36d 100644 --- a/src/viz.ts +++ b/src/viz.ts @@ -341,7 +341,7 @@ async function loadEntries(paths: VizPaths): Promise { rulesDir: path.join(paths.knowledgeRoot, 'rules'), skillsDir: path.join(paths.knowledgeRoot, 'skills'), ...delivered, - votesDir: paths.votesDir, + votesDir: paths.scope ? undefined : paths.votesDir, voteScope: paths.scope, indexPath: tmpIndexPath, }); diff --git a/src/votes.ts b/src/votes.ts index 371bf6520..ef89ef9d6 100644 --- a/src/votes.ts +++ b/src/votes.ts @@ -539,51 +539,64 @@ function deliveredSyncIdsOf(remote: UserVotesV2): string[] { /** Confirmed team votes plus this scope's undelivered deltas, for index builds. * Never reads staged worktree counts or changes delivery/cleanup state. */ -export async function readIndexVoteFiles(config: LocalConfig): Promise { +export async function readIndexVoteFiles(config: LocalConfig): Promise> { const { getVotesDir, REPORTS_BRANCH } = await import('./types.js'); const localPath = path.join(getVotesDir(config), `${config.username}.yaml`); const { acquireLock, releaseLock } = await import('./update.js'); - // No retries: Stop/sync may hold this lock. A first build without local - // votes must not create a votes directory just to read it. const { pathExists } = await import('./utils/fs.js'); + const { createGit } = await import('./utils/git.js'); + const votesDir = config.repo.kind === 'http' ? undefined + : await (await import('./utils/reports-branch.js')).indexableVotesDir(config); + const git = votesDir && await pathExists(path.dirname(votesDir)) ? createGit(path.dirname(votesDir)) : undefined; + let local: UserVotesV2 | undefined; + let ref: string | undefined; + // Capture local deltas and the confirmed ref together. Blob reads below use + // this immutable commit, without holding the writer's lock for every member. const held = await pathExists(localPath) && await acquireLock(`${localPath}.lock`); try { - const files = new Map(); - if (config.repo.kind !== 'http') { - const { indexableVotesDir } = await import('./utils/reports-branch.js'); - const votesDir = await indexableVotesDir(config); - if (votesDir && await pathExists(path.dirname(votesDir))) { - const { createGit } = await import('./utils/git.js'); - const git = createGit(path.dirname(votesDir)); - // Pin one confirmed snapshot: a push can update the ref while we read. - const ref = await git.revparse(['--verify', `refs/remotes/origin/${REPORTS_BRANCH}^{commit}`]).catch(() => null); - if (ref) { - const names = await git.raw(['ls-tree', '-r', '--name-only', '-z', ref.trim(), '--', 'votes/']); - for (const name of names.split('\0')) { - if (/^votes\/[^/]+\.ya?ml$/.test(name)) { - files.set(path.basename(name), await git.raw(['show', `${ref.trim()}:${name}`])); - } - } - } + ref = await git?.revparse(['--verify', `refs/remotes/origin/${REPORTS_BRANCH}^{commit}`]).then(r => r.trim()).catch(() => undefined); + if (held) local = await readUserVotes(localPath); + } finally { + if (held) await releaseLock(`${localPath}.lock`); + } + const files = new Map(); + if (git && ref && votesDir) { + const names = await git.raw(['ls-tree', '-r', '--name-only', '-z', ref, '--', 'votes/']); + for (const name of names.split('\0')) { + if (/^votes\/[^/]+\.ya?ml$/.test(name)) { + files.set(path.basename(name), { path: path.join(path.dirname(votesDir), name), content: await git.raw(['show', `${ref}:${name}`]) }); } } - if (held) { - const local = await readUserVotes(localPath); - const filename = `${config.username}.yaml`; - const base = readUserVotesFromContent(files.get(filename) ?? ''); - const pending = readPendingSync(local); - // Same delivery accounting as stageVotesMerge, but entirely in memory. - const deltas = pending && deliveredSyncIdsOf(base).includes(pending.syncId) - ? remainingDeltasAfter(local.deltas, pending.deltas) - : local.deltas; - if (Object.keys(deltas).length > 0) { - files.set(filename, YAML.stringify(mergeDeltas({ ...local, deltas }, base))); + } + if (local) { + const filename = `${config.username}.yaml`; + const source = files.get(filename); + const base = readUserVotesFromContent(source?.content ?? ''); + const pending = readPendingSync(local); + const deltas = pending && deliveredSyncIdsOf(base).includes(pending.syncId) + ? remainingDeltasAfter(local.deltas, pending.deltas) : local.deltas; + if (Object.keys(deltas).length > 0) { + const merged = mergeDeltas({ ...local, deltas }, base); + // Indexing must not migrate unrelated v1 entries from their legacy score + // of 1 to the v2 recall weight of 0.3. Delivery migration stays unchanged. + const parsed = parseUserVotesContent(source?.content ?? ''); + if (parsed?.migrated && source) { + const legacy = YAML.parse(source.content) as UserVotes; + for (const id of Object.keys(deltas)) delete legacy.votes[id]; + for (const id of Object.keys(merged.votes)) { + if (!(id in deltas)) delete merged.votes[id]; + } + files.set(filename, { ...source, content: YAML.stringify(legacy) }); + files.set(`${filename}:local`, { path: localPath, content: YAML.stringify(merged) }); + } else if (source && !parsed) { + // Keep the corrupt source so aggregation reports its filename. + files.set(`${filename}:local`, { path: localPath, content: YAML.stringify(merged) }); + } else { + files.set(filename, { path: source?.path ?? localPath, content: YAML.stringify(merged) }); } } - return [...files.values()]; - } finally { - if (held) await releaseLock(`${localPath}.lock`); } + return [...files.values()]; } /**