diff --git a/scripts/work-items.js b/scripts/work-items.js index 8a130651c..c9161015c 100644 --- a/scripts/work-items.js +++ b/scripts/work-items.js @@ -176,8 +176,36 @@ function slugifyWorkItemSegment(value) { ); } +function normalizeGithubRepo(repo) { + return typeof repo === 'string' ? repo.toLowerCase() : ''; +} + function githubWorkItemId(repo, type, number) { - return `github-${slugifyWorkItemSegment(repo)}-${type}-${number}`; + return `github-${encodeURIComponent(normalizeGithubRepo(repo))}-${type}-${number}`; +} + +function isSameGithubWorkItem(existing, incoming) { + return existing + && existing.source === incoming.source + && existing.sourceId === incoming.sourceId + && existing.metadata?.syncedBy === 'ecc-work-items-sync-github' + && normalizeGithubRepo(existing.metadata.repo) === normalizeGithubRepo(incoming.metadata.repo); +} + +function resolveGithubWorkItemId(store, item) { + const existing = store.getWorkItemById(item.id); + if (existing) { + if (!isSameGithubWorkItem(existing, item)) { + throw new Error(`GitHub work item ID collision: ${item.id} belongs to a different item.`); + } + return item.id; + } + + // Keep references to older imports intact, but never reuse a slug belonging + // to another repository. Prefer an existing canonical ID on every sync. + const type = item.source === 'github-pr' ? 'pr' : 'issue'; + const legacyId = `github-${slugifyWorkItemSegment(item.metadata.repo)}-${type}-${item.sourceId}`; + return isSameGithubWorkItem(store.getWorkItemById(legacyId), item) ? legacyId : item.id; } function githubPrStatus(pr) { @@ -245,7 +273,7 @@ function closeStaleGithubItems(store, repo, activeIds, options = {}) { if (!item.metadata || item.metadata.syncedBy !== 'ecc-work-items-sync-github') { continue; } - if (item.metadata.repo !== repo || activeIds.has(item.id)) { + if (normalizeGithubRepo(item.metadata.repo) !== normalizeGithubRepo(repo) || activeIds.has(item.id)) { continue; } if (item.status === 'closed' || item.status === 'done') { @@ -281,10 +309,12 @@ function syncGithubWorkItems(store, options) { const items = []; for (const pr of prs) { const payload = buildGithubPrWorkItem(repo, pr, options); - activeIds.add(payload.id); + const id = resolveGithubWorkItemId(store, payload); + activeIds.add(id); items.push( store.upsertWorkItem({ ...payload, + id, createdAt: undefined, updatedAt: syncedAt }) @@ -292,10 +322,12 @@ function syncGithubWorkItems(store, options) { } for (const issue of issues) { const payload = buildGithubIssueWorkItem(repo, issue, options); - activeIds.add(payload.id); + const id = resolveGithubWorkItemId(store, payload); + activeIds.add(id); items.push( store.upsertWorkItem({ ...payload, + id, createdAt: undefined, updatedAt: syncedAt }) diff --git a/tests/lib/state-store.test.js b/tests/lib/state-store.test.js index a1e2f045c..800a32e35 100644 --- a/tests/lib/state-store.test.js +++ b/tests/lib/state-store.test.js @@ -980,7 +980,7 @@ async function runTests() { assert.strictEqual(syncPayload.issueCount, 1); assert.strictEqual(syncPayload.closedCount, 0); assert.strictEqual(syncPayload.items.length, 3); - assert.strictEqual(syncPayload.items[0].id, 'github-affaan-m-ecc-pr-3'); + assert.strictEqual(syncPayload.items[0].id, 'github-affaan-m%2Fecc-pr-3'); assert.strictEqual(syncPayload.items[0].status, 'blocked'); assert.strictEqual(syncPayload.items[1].status, 'needs-review'); assert.strictEqual(syncPayload.items[2].metadata.labels[0], 'release'); diff --git a/tests/scripts/work-items-identity.test.js b/tests/scripts/work-items-identity.test.js new file mode 100644 index 000000000..581251103 --- /dev/null +++ b/tests/scripts/work-items-identity.test.js @@ -0,0 +1,265 @@ +'use strict'; + +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); +const { test } = require('node:test'); +const { createStateStore } = require('../../scripts/lib/state-store'); + +const CLI = path.join(__dirname, '..', '..', 'scripts', 'work-items.js'); +const SYNC_MARKER = 'ecc-work-items-sync-github'; + +function fixture(t) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-work-item-identity-')); + const dbPath = path.join(dir, 'state.db'); + const responsePath = path.join(dir, 'response.json'); + const shimPath = path.join(dir, 'gh-shim.cjs'); + fs.writeFileSync(shimPath, [ + "const fs = require('node:fs');", + 'const response = JSON.parse(fs.readFileSync(process.env.ECC_IDENTITY_RESPONSE, "utf8"));', + 'process.stdout.write(JSON.stringify(response[process.argv[2]]));' + ].join('\n')); + t.after(() => fs.rmSync(dir, { recursive: true, force: true })); + + function run(args) { + return spawnSync(process.execPath, [CLI, ...args, '--db', dbPath], { + encoding: 'utf8', + env: { ...process.env, ECC_GH_SHIM: shimPath, ECC_IDENTITY_RESPONSE: responsePath } + }); + } + + function sync(repo, response = { issue: [], pr: [] }, args = []) { + fs.writeFileSync(responsePath, JSON.stringify(response)); + return run(['sync-github', '--repo', repo, '--json', ...args]); + } + + function syncItems(repo, response, args) { + const result = sync(repo, response, args); + assert.equal(result.status, 0, result.stderr); + return JSON.parse(result.stdout); + } + + async function withStore(fn) { + const store = await createStateStore({ dbPath }); + try { + return fn(store); + } finally { + store.close(); + } + } + + return { run, sync, syncItems, withStore }; +} + +function responseFor(repo, types = ['issue', 'pr']) { + const entry = type => ({ + number: 1, + title: `${repo} ${type}`, + url: `https://github.com/${repo}/${type === 'pr' ? 'pull' : 'issues'}/1`, + author: { login: 'author' }, + labels: [], + isDraft: false, + mergeStateStatus: 'CLEAN' + }); + return { + issue: types.includes('issue') ? [entry('issue')] : [], + pr: types.includes('pr') ? [entry('pr')] : [] + }; +} + +function legacyItem(repo, type, overrides = {}) { + return { + id: `github-${repo.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-+|-+$/g, '')}-${type}-1`, + source: `github-${type}`, + sourceId: '1', + title: 'Previously imported item', + status: 'needs-review', + createdAt: '2020-01-02T03:04:05.000Z', + metadata: { repo, type: type === 'pr' ? 'pull_request' : 'issue', syncedBy: SYNC_MARKER }, + ...overrides + }; +} + +test('sync keeps punctuation and owner/repository boundaries distinct for issues and PRs', async t => { + const f = fixture(t); + const repos = ['acme/tool-kit', 'acme/tool_kit', 'acme/tool.kit', 'acme-tool/kit']; + const ids = []; + for (const repo of repos) { + const first = f.syncItems(repo, responseFor(repo)); + const repeated = f.syncItems(repo, responseFor(repo)); + assert.deepEqual(repeated.items.map(item => item.id), first.items.map(item => item.id)); + ids.push(...first.items.map(item => item.id)); + } + assert.equal(new Set(ids).size, repos.length * 2, 'every repository/type identity must have its own ID'); + const rows = await f.withStore(store => store.listWorkItems({ limit: 100 }).items); + assert.equal(rows.length, repos.length * 2); + for (const repo of repos) { + for (const type of ['issue', 'pr']) { + const row = rows.find(item => item.metadata.repo === repo && item.source === `github-${type}`); + assert.ok(row, `${repo} ${type} remains stored`); + assert.equal(row.status, 'needs-review'); + assert.equal(row.url, responseFor(repo)[type][0].url); + } + } +}); + +for (const type of ['issue', 'pr']) { + test(`${type}: reuse a matching legacy ID and preserve creation time and explicit linkage`, async t => { + const f = fixture(t); + const repo = 'Acme/Tool_Kit'; + const legacy = legacyItem(repo, type, { sessionId: 'session-42', repoRoot: '/workspace/tool-kit' }); + await f.withStore(store => { + store.upsertSession({ id: 'session-42', adapterId: 'manual', harness: 'codex', state: 'running' }); + store.upsertWorkItem(legacy); + }); + const args = ['--session', 'session-42', '--repo-root', '/workspace/tool-kit']; + const synced = f.syncItems(repo.toLowerCase(), responseFor(repo, [type]), args); + assert.equal(synced.items[0].id, legacy.id); + assert.equal(synced.items[0].createdAt, legacy.createdAt); + assert.equal(synced.items[0].sessionId, legacy.sessionId); + assert.equal(synced.items[0].repoRoot, legacy.repoRoot); + assert.equal(synced.items[0].title, `${type === 'pr' ? 'PR' : 'Issue'} #1: ${repo} ${type}`); + const repeated = f.syncItems(repo, responseFor(repo, [type]), args); + assert.equal(repeated.items[0].id, legacy.id); + assert.equal(repeated.closedCount, 0); + assert.equal(await f.withStore(store => store.listWorkItems({ limit: 100 }).items.length), 1); + }); + + test(`${type}: a colliding legacy repository is not overwritten`, async t => { + const f = fixture(t); + const original = await f.withStore(store => store.upsertWorkItem(legacyItem('acme/tool-kit', type))); + const incoming = f.syncItems('acme/tool_kit', responseFor('acme/tool_kit', [type])).items[0]; + assert.notEqual(incoming.id, original.id); + assert.deepEqual(await f.withStore(store => store.getWorkItemById(original.id)), original); + assert.equal(await f.withStore(store => store.listWorkItems({ limit: 100 }).items.length), 2); + }); +} + +test('legacy reuse requires the matching source, number, sync marker, and repository', async t => { + const variants = [ + { source: 'manual' }, + { sourceId: '2' }, + { metadata: { repo: 'acme/tool-kit', type: 'issue' } }, + ...[null, 42, { toString: 5 }].map(repo => ({ + metadata: { repo, type: 'issue', syncedBy: SYNC_MARKER } + })) + ]; + for (const overrides of variants) { + await t.test(JSON.stringify(overrides), async subtest => { + const f = fixture(subtest); + const original = await f.withStore(store => store.upsertWorkItem(legacyItem('acme/tool-kit', 'issue', { + ...overrides, + status: 'done' + }))); + const incoming = f.syncItems('acme/tool-kit', responseFor('acme/tool-kit', ['issue'])).items[0]; + assert.notEqual(incoming.id, original.id); + assert.deepEqual(await f.withStore(store => store.getWorkItemById(original.id)), original); + }); + } +}); + +test('canonical IDs stay stable across casing changes and when the legacy ID is vacant', async t => { + const f = fixture(t); + const repo = 'Acme/Tool_Kit'; + const canonical = legacyItem(repo, 'issue', { + id: `github-${encodeURIComponent(repo.toLowerCase())}-issue-1` + }); + await f.withStore(store => store.upsertWorkItem(canonical)); + for (const spelling of [repo.toLowerCase(), repo.toUpperCase()]) { + const result = f.syncItems(spelling, responseFor(spelling, ['issue'])); + assert.equal(result.items[0].id, canonical.id); + assert.equal(result.closedCount, 0); + assert.equal(result.items[0].createdAt, canonical.createdAt); + } + assert.equal(await f.withStore(store => store.listWorkItems({ limit: 100 }).items.length), 1); +}); + +test('canonical IDs take precedence when a matching legacy record also exists', async t => { + const f = fixture(t); + const repo = 'acme/tool-kit'; + const legacy = await f.withStore(store => store.upsertWorkItem(legacyItem(repo, 'issue', { status: 'done' }))); + const canonical = legacyItem(repo, 'issue', { + id: `github-${encodeURIComponent(repo)}-issue-1`, + createdAt: '2021-02-03T04:05:06.000Z' + }); + await f.withStore(store => store.upsertWorkItem(canonical)); + const result = f.syncItems(repo, responseFor(repo, ['issue'])); + assert.equal(result.items[0].id, canonical.id); + assert.equal(result.items[0].createdAt, canonical.createdAt); + assert.equal(result.items[0].status, 'needs-review'); + assert.deepEqual(await f.withStore(store => store.getWorkItemById(legacy.id)), legacy); + assert.equal(await f.withStore(store => store.listWorkItems({ limit: 100 }).items.length), 2); +}); + +test('a canonical ID occupied by a different identity fails without overwriting it', async t => { + const f = fixture(t); + const repo = 'acme/tool-kit'; + const original = await f.withStore(store => store.upsertWorkItem({ + id: `github-${encodeURIComponent(repo)}-issue-1`, + source: 'manual', + title: 'Unrelated local work', + status: 'open' + })); + const result = f.sync(repo, responseFor(repo, ['issue'])); + assert.notEqual(result.status, 0); + assert.match(result.stderr, /collision|already|different|conflict/i); + assert.deepEqual(await f.withStore(store => store.getWorkItemById(original.id)), original); + assert.equal(await f.withStore(store => store.listWorkItems({ limit: 100 }).items.length), 1); +}); + +test('closing stale items isolates punctuation variants and recognizes repository casing', async t => { + const f = fixture(t); + const legacy = legacyItem('Acme/Tool-Kit', 'issue'); + await f.withStore(store => store.upsertWorkItem(legacy)); + const own = f.syncItems('Acme/Tool-Kit', responseFor('Acme/Tool-Kit')).items; + const neighbor = f.syncItems('acme/tool_kit', responseFor('acme/tool_kit')).items; + const closed = f.syncItems('ACME/TOOL-KIT', { issue: [], pr: [] }); + assert.deepEqual(closed.closedItems.map(item => item.id).sort(), own.map(item => item.id).sort()); + assert.equal(closed.closedCount, 2); + for (const item of own) { + assert.equal((await f.withStore(store => store.getWorkItemById(item.id))).status, 'closed'); + } + for (const item of neighbor) { + assert.equal((await f.withStore(store => store.getWorkItemById(item.id))).status, 'needs-review'); + } + assert.equal(f.syncItems('acme/tool-kit', { issue: [], pr: [] }).closedCount, 0); +}); + +test('encoded IDs round-trip through display, claim, and close commands', async t => { + const f = fixture(t); + const repo = 'acme/tool_kit'; + const response = responseFor(repo, ['issue']); + const imported = f.syncItems(repo, { + ...response, + issue: response.issue.map(issue => ({ ...issue, author: null })) + }).items[0]; + assert.match(imported.id, /%2F/); + + for (const args of [ + ['sync-github', '--repo', repo], + ['show', imported.id], + ['list'], + ['claim', imported.id, '--owner', 'maintainer', '--as', 'human'], + ['close', imported.id] + ]) { + const result = f.run(args); + assert.equal(result.status, 0, result.stderr); + assert.ok(result.stdout.includes(imported.id), `${args[0]} displays the exact ID`); + } + + const shown = f.run(['show', imported.id, '--json']); + assert.equal(shown.status, 0, shown.stderr); + const item = JSON.parse(shown.stdout); + assert.equal(item.id, imported.id); + assert.equal(item.status, 'done'); + assert.equal(item.owner, 'maintainer'); + assert.equal(item.metadata.assigneeKind, 'human'); + assert.equal(item.metadata.repo, repo); + assert.equal(item.source, imported.source); + assert.equal(item.sourceId, imported.sourceId); + assert.equal(item.createdAt, imported.createdAt); + assert.equal(item.url, imported.url); + assert.deepEqual(await f.withStore(store => store.listWorkItems({ limit: 100 }).items), [item]); +});