From 0ffa5e64892a4c4c38a977887d600dac26b8ed23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Almeida?= <57680069+zeval@users.noreply.github.com> Date: Sun, 13 Sep 2026 06:24:45 +0200 Subject: [PATCH] fix(ci): reuse documentation requirement lookups (#3640) --- .github/scripts/pr-docs.cjs | 27 +- .github/scripts/pr-docs.test.cjs | 290 ++++++++++++++++++ docs/plans/pr-documentation-coverage/plan.md | 9 + .../task-01-coverage-validator.md | 50 +++ 4 files changed, 369 insertions(+), 7 deletions(-) diff --git a/.github/scripts/pr-docs.cjs b/.github/scripts/pr-docs.cjs index b8386d9620..3600addab3 100644 --- a/.github/scripts/pr-docs.cjs +++ b/.github/scripts/pr-docs.cjs @@ -1111,6 +1111,8 @@ async function loadCoverageContents({ client, changedFiles, headSha }) { } const contents = {}; const loaded = new Set(); + const requirementSearches = new Map(); + const requirementDirectories = new Map(); let documentCount = 0; let totalBytes = 0; async function load(pathname) { @@ -1213,7 +1215,14 @@ async function loadCoverageContents({ client, changedFiles, headSha }) { const unresolvedRequirementIds = []; if (typeof client.searchCode === 'function') { for (const requirementId of workOrder.requirements ?? []) { - const matches = await client.searchCode(requirementId, requirementDirectory); + const searchKey = JSON.stringify([requirementDirectory, requirementId]); + if (!requirementSearches.has(searchKey)) { + requirementSearches.set( + searchKey, + await client.searchCode(requirementId, requirementDirectory), + ); + } + const matches = requirementSearches.get(searchKey); if (matches.length === 0) { unresolvedRequirementIds.push(requirementId); } @@ -1225,14 +1234,18 @@ async function loadCoverageContents({ client, changedFiles, headSha }) { unresolvedRequirementIds.push(...(workOrder.requirements ?? [])); } if (unresolvedRequirementIds.length > 0 && typeof client.listDirectory === 'function') { - let entries = []; - try { - entries = await client.listDirectory(requirementDirectory, headSha); - } catch (error) { - if (!isMissingResourceError(error)) { - throw error; + if (!requirementDirectories.has(requirementDirectory)) { + let entries = []; + try { + entries = await client.listDirectory(requirementDirectory, headSha); + } catch (error) { + if (!isMissingResourceError(error)) { + throw error; + } } + requirementDirectories.set(requirementDirectory, entries); } + const entries = requirementDirectories.get(requirementDirectory); const candidateNames = new Set(unresolvedRequirementIds.map(requirementId => { const parts = requirementId.split('-'); return `${parts.slice(2).join('-').replace(/-\d+$/, '').toLowerCase()}.md`; diff --git a/.github/scripts/pr-docs.test.cjs b/.github/scripts/pr-docs.test.cjs index 603eb6c968..2cad3c1df1 100644 --- a/.github/scripts/pr-docs.test.cjs +++ b/.github/scripts/pr-docs.test.cjs @@ -798,6 +798,296 @@ test('coverage loading searches only the referenced requirement files', async () ]); }); +function repeatedCoverageFixture() { + const contents = uiFixtureContents(); + const planPath = 'docs/plans/recovery/plan.md'; + const workOrderPath = 'docs/plans/recovery/task-01-recovery.md'; + const designPath = 'docs/specs/ui/system-design/ui-coverage.md'; + const requirementPath = 'docs/specs/ui/requirements/ui-coverage.md'; + const requirementIds = [1, 2, 3].map(index => `REQ-UI-COVERAGE-00${index}`); + for (const pathname of [planPath, workOrderPath, designPath]) { + contents[pathname] = contents[pathname].replace( + ' - REQ-UI-COVERAGE-001', + requirementIds.map(id => ` - ${id}`).join('\n'), + ); + } + contents[workOrderPath] = contents[workOrderPath].replace( + ' - AC-UI-COVERAGE-001.3', + requirementIds.map(id => ` - ${id.replace('REQ-', 'AC-')}.3`).join('\n'), + ); + contents[requirementPath] = requirementIds.map(id => + `### ${id}\n\n- **${id.replace('REQ-', 'AC-')}.3:** Runtime changes have a work order.\n` + ).join('\n'); + const changed = runtimeAndWorkOrderDiff(); + for (let index = 2; index <= 6; index += 1) { + const basename = `task-0${index}-recovery.md`; + const pathname = `docs/plans/recovery/${basename}`; + contents[pathname] = contents[workOrderPath].replace('01-recovery', `0${index}-recovery`); + contents[planPath] += `\n- [Task ${index}](${basename})\n`; + changed.push({ filename: pathname, status: 'modified', changes: 1 }); + } + return { contents, changed, requirementPath, requirementIds }; +} + +function coverageClient(contents, changed, overrides = {}) { + return { + async getPullRequest() { + return pullRequest(42, SHA_B, [], changed.length); + }, + async listFiles() { + return changed; + }, + async getFile(pathname, ref) { + assert.equal(ref, SHA_B); + if (!Object.hasOwn(contents, pathname)) { + throw new Error('GitHub API request failed with HTTP 404: Not Found'); + } + return contents[pathname]; + }, + ...overrides, + }; +} + +// @covers AC-CI-PR-DOCS-001.4, AC-CI-PR-DOCS-003.2 +test('six work orders sharing three requirements stay within the search quota', async () => { + const { contents, changed, requirementPath, requirementIds } = repeatedCoverageFixture(); + const searches = []; + const client = coverageClient(contents, changed, { + async searchCode(requirementId, directory) { + searches.push({ requirementId, directory }); + if (searches.length > 10) { + throw new Error('GitHub API request failed with HTTP 403: API rate limit exceeded'); + } + return [requirementPath]; + }, + }); + + const result = await validator.evaluatePullRequest({ client, pullNumber: 42 }); + + assert.equal(result.status, 'covered', result.errors.join('; ')); + assert.equal(result.workOrders.length, 6); + assert.deepEqual(searches, requirementIds.map(requirementId => ({ + requirementId, + directory: 'docs/specs/ui/requirements', + }))); +}); + +// @covers AC-CI-PR-DOCS-001.4, AC-CI-PR-DOCS-003.2 +test('PR-only requirements and empty searches reuse a bounded directory lookup', async () => { + const { contents, changed, requirementPath, requirementIds } = repeatedCoverageFixture(); + changed.push({ filename: requirementPath, status: 'added' }); + let searches = 0; + let listings = 0; + const client = coverageClient(contents, changed, { + async searchCode() { + searches += 1; + return []; + }, + async listDirectory(directory, ref) { + assert.equal(directory, 'docs/specs/ui/requirements'); + assert.equal(ref, SHA_B); + listings += 1; + if (listings > 1) { + throw new Error('GitHub API request failed with HTTP 429: Too Many Requests'); + } + return [{ path: requirementPath, type: 'file' }]; + }, + }); + + const result = await validator.evaluatePullRequest({ client, pullNumber: 42 }); + + assert.equal(result.status, 'covered', result.errors.join('; ')); + assert.equal(searches, requirementIds.length); + assert.equal(listings, 1); +}); + +// @covers AC-CI-PR-DOCS-001.4, AC-CI-PR-DOCS-001.5 +test('directory fallback reuses exact-head entries without accepting missing requirements', async t => { + for (const outcome of ['file', 'empty', 'missing']) { + await t.test(outcome, async () => { + const { contents, changed, requirementPath } = repeatedCoverageFixture(); + const fallbackPath = 'docs/specs/ui/requirements/coverage.md'; + contents[fallbackPath] = contents[requirementPath]; + delete contents[requirementPath]; + let listings = 0; + const client = coverageClient(contents, changed, { + async searchCode() { + return []; + }, + async listDirectory(directory, ref) { + assert.equal(directory, 'docs/specs/ui/requirements'); + assert.equal(ref, SHA_B); + listings += 1; + if (outcome === 'missing') { + throw new Error('GitHub API request failed with HTTP 404: Not Found'); + } + return outcome === 'file' ? [{ path: fallbackPath, type: 'file' }] : []; + }, + }); + + const result = await validator.evaluatePullRequest({ client, pullNumber: 42 }); + + assert.equal(result.status, outcome === 'file' ? 'covered' : 'invalid', result.errors.join('; ')); + assert.equal(listings, 1); + }); + } +}); + +// @covers AC-CI-PR-DOCS-001.4, AC-CI-PR-DOCS-001.5 +test('designs sharing an ID reuse lookups only within their requirement directory', async () => { + const contents = uiFixtureContents(); + const reference = ' - ../../specs/ui/system-design/ui-coverage.md'; + for (const pathname of ['docs/plans/recovery/plan.md', 'docs/plans/recovery/task-01-recovery.md']) { + contents[pathname] = contents[pathname].replace(reference, `${reference} + - ../../specs/ui/system-design/shared.md + - ../../specs/ci/system-design/shared.md`); + } + contents['docs/specs/ui/system-design/shared.md'] = contents['docs/specs/ui/system-design/ui-coverage.md']; + contents['docs/specs/ci/system-design/shared.md'] = contents['docs/specs/ui/system-design/ui-coverage.md'] + .replace('system: ui', 'system: ci'); + contents['docs/specs/ci/requirements/shared.md'] = contents['docs/specs/ui/requirements/ui-coverage.md']; + const searches = []; + const client = coverageClient(contents, runtimeAndWorkOrderDiff(), { + async searchCode(requirementId, directory) { + searches.push({ requirementId, directory }); + return [directory === 'docs/specs/ui/requirements' + ? 'docs/specs/ui/requirements/ui-coverage.md' + : 'docs/specs/ci/requirements/shared.md']; + }, + }); + + const result = await validator.evaluatePullRequest({ client, pullNumber: 42 }); + + assert.equal(result.status, 'covered', result.errors.join('; ')); + assert.deepEqual(searches, ['ui', 'ci'].map(system => ({ + requirementId: 'REQ-UI-COVERAGE-001', + directory: `docs/specs/${system}/requirements`, + }))); +}); + +// @covers AC-CI-PR-DOCS-003.2 +test('lookup errors fail closed without poisoning later evaluations', async t => { + for (const boundary of ['searchCode', 'listDirectory']) { + await t.test(boundary, async () => { + const { contents, changed, requirementPath } = repeatedCoverageFixture(); + changed.push({ filename: requirementPath, status: 'added' }); + let fail = true; + const client = coverageClient(contents, changed, { + async searchCode() { + if (fail && boundary === 'searchCode') { + throw new Error('GitHub API request failed with HTTP 403: API rate limit exceeded'); + } + return []; + }, + async listDirectory() { + if (fail && boundary === 'listDirectory') { + throw new Error('GitHub API request failed with HTTP 503: Service Unavailable'); + } + return []; + }, + }); + + const failed = await validator.evaluatePullRequest({ client, pullNumber: 42 }); + assert.equal(failed.ok, false); + assert.equal(failed.status, 'error'); + assert.match(failed.errors.join('; '), /GitHub API request failed/); + fail = false; + const recovered = await validator.evaluatePullRequest({ client, pullNumber: 42 }); + assert.equal(recovered.status, 'covered', recovered.errors.join('; ')); + }); + } +}); + +// @covers AC-CI-PR-DOCS-001.5 +test('lookup reuse preserves ambiguous requirement detection', async t => { + for (const source of ['search', 'PR-only file']) { + await t.test(source, async () => { + const { contents, changed, requirementPath } = repeatedCoverageFixture(); + const duplicatePath = 'docs/specs/ui/requirements/duplicate.md'; + contents[duplicatePath] = contents[requirementPath]; + if (source === 'PR-only file') { + changed.push({ filename: duplicatePath, status: 'added' }); + } + const client = coverageClient(contents, changed, { + async searchCode() { + return source === 'search' ? [requirementPath, duplicatePath] : [requirementPath]; + }, + }); + + const result = await validator.evaluatePullRequest({ client, pullNumber: 42 }); + + assert.equal(result.status, 'invalid'); + assert.match(result.errors.join('; '), /ambiguous definitions/); + }); + } +}); + +// @covers AC-CI-PR-DOCS-003.1 +test('revision retries reload requirement lookups and files at the new exact head', async () => { + const { contents, changed, requirementPath, requirementIds } = repeatedCoverageFixture(); + const renamedPath = 'docs/specs/ui/requirements/renamed.md'; + let metadataReads = 0; + let searches = 0; + const client = coverageClient(contents, changed, { + async getPullRequest() { + return pullRequest(42, metadataReads++ === 0 ? SHA_B : SHA_C, [], changed.length); + }, + async searchCode() { + searches += 1; + return [metadataReads === 1 ? requirementPath : renamedPath]; + }, + async getFile(pathname, ref) { + assert.equal(ref, metadataReads === 1 ? SHA_B : SHA_C); + if (ref === SHA_C && pathname === requirementPath) { + throw new Error('GitHub API request failed with HTTP 404: Not Found'); + } + return contents[pathname === renamedPath ? requirementPath : pathname]; + }, + }); + + const result = await validator.evaluatePullRequest({ client, pullNumber: 42 }); + + assert.equal(result.status, 'covered', result.errors.join('; ')); + assert.equal(result.headSha, SHA_C); + assert.equal(searches, requirementIds.length * 2); +}); + +// @covers AC-CI-PR-DOCS-003.4 +test('merge-group members do not share cached requirement lookups or contents', async () => { + const { contents, changed, requirementPath } = repeatedCoverageFixture(); + let currentMember; + const client = coverageClient(contents, changed, { + async getPullRequest(number) { + currentMember = number; + return pullRequest(number, number === 1 ? SHA_D : SHA_E, [], changed.length); + }, + async searchCode() { + return currentMember === 1 ? [requirementPath] : []; + }, + async getFile(pathname, ref) { + assert.equal(ref, currentMember === 1 ? SHA_D : SHA_E); + if (currentMember === 2 && pathname === requirementPath) { + throw new Error('GitHub API request failed with HTTP 404: Not Found'); + } + return contents[pathname]; + }, + }); + + const result = await validator.evaluateMergeGroup({ + client, + baseSha: SHA_A, + headSha: SHA_C, + entries: [ + { baseCommit: { oid: SHA_A }, headCommit: { oid: SHA_B }, pullRequest: { number: 1, headRefOid: SHA_D } }, + { baseCommit: { oid: SHA_B }, headCommit: { oid: SHA_C }, pullRequest: { number: 2, headRefOid: SHA_E } }, + ], + }); + + assert.equal(result.ok, false); + assert.deepEqual(result.memberResults.map(member => member.status), ['covered', 'invalid']); + assert.match(result.memberResults[1].errors.join('; '), /not defined in ui requirements/); +}); + test('missing referenced artifacts become invalid coverage through the API adapter', async () => { const contents = fixtureContents(); const workOrderPath = 'docs/plans/recovery/task-01-recovery.md'; diff --git a/docs/plans/pr-documentation-coverage/plan.md b/docs/plans/pr-documentation-coverage/plan.md index 9e4be6e387..b58135e430 100644 --- a/docs/plans/pr-documentation-coverage/plan.md +++ b/docs/plans/pr-documentation-coverage/plan.md @@ -66,6 +66,15 @@ Execute sequentially. No subagents are authorized. ## Verification results +Task 01's 2026-09-13 regression fix reuses requirement searches and directory +listings within each exact-head snapshot. Its deterministic quota regression +now uses three searches for six work orders. All 50 Node tests, five workflow +contract tests, documentation catalog/specification checks, and whitespace +checks pass. A GET-only evaluation of exact PR #3626 head `93abff34efe8c9ab2bec68acb8e6aba1eb2a23d4` +returned covered with three searches and no failed requests. See Task 01 for +red/green evidence and limits. Consumption by trusted `main` and the new PR's +CI/review gates remain delivery work; no status override or merge is claimed. + Design validation on 2026-09-10: - `python3 scripts/lint-spec-files.test.py`: 36 tests passed. diff --git a/docs/plans/pr-documentation-coverage/task-01-coverage-validator.md b/docs/plans/pr-documentation-coverage/task-01-coverage-validator.md index ca066b4960..c7c1cec0ae 100644 --- a/docs/plans/pr-documentation-coverage/task-01-coverage-validator.md +++ b/docs/plans/pr-documentation-coverage/task-01-coverage-validator.md @@ -7,12 +7,17 @@ depends_on: [] plan: "plan.md" requirements: - REQ-CI-PR-DOCS-001 + - REQ-CI-PR-DOCS-003 acceptance_criteria: - AC-CI-PR-DOCS-001.2 - AC-CI-PR-DOCS-001.3 - AC-CI-PR-DOCS-001.4 - AC-CI-PR-DOCS-001.5 - AC-CI-PR-DOCS-001.6 + - AC-CI-PR-DOCS-003.1 + - AC-CI-PR-DOCS-003.2 + - AC-CI-PR-DOCS-003.3 + - AC-CI-PR-DOCS-003.4 system_design: - ../../specs/ci/system-design/pull-request-documentation-coverage.md --- @@ -37,6 +42,9 @@ Application code, unrelated workflow cleanup, live label changes, and live rules - Only explicitly exempt changes pass without a work order; the #3137 runtime-change fixture fails. - A changed work order and valid referenced package pass without cosmetic edits to existing specifications. - Missing, empty, deleted, escaping, or mismatched references produce precise failures. +- Repeated requirement lookups reuse successful results only within one PR + snapshot and requirement directory. Empty search results still load changed + requirements at the exact head; failed lookups remain evaluation errors. ## Verification @@ -44,6 +52,7 @@ Run from the repository root: ```bash node --test .github/scripts/pr-docs.test.cjs +python3 .github/scripts/pr-docs-workflow-contract_test.py git diff --check ``` @@ -85,3 +94,44 @@ Verification: - `node --test .github/scripts/pr-docs.test.cjs`: 35 tests passed. - `git diff --check`: passed. + +### Requirement lookup regression, 2026-09-13 + +Successful code-search results (including empty results) now reuse a cache +keyed by requirement directory and ID within one exact-head snapshot. +Directory fallback reuses the complete listing for that snapshot. Existing +bounded file reads still load PR-only requirements and search candidates at +the exact head. Each distinct ID is searched even when a previously loaded +document defines it, so other candidate definitions remain visible to the +ambiguity check. Lookup failures remain errors; retries and merge-group +members get fresh caches. + +TDD evidence: + +- Red: six work orders sharing three IDs exhausted a deterministic ten-search + budget on request 11 and returned `error` instead of `covered`. +- Green: the same fixture returns `covered` with three searches. A second red + regression exceeded a one-listing budget for PR-only requirements; it now + passes with one listing and three empty search results. +- `node --test .github/scripts/pr-docs.test.cjs`: passed. +- `node --test-isolation=none --test --test-reporter=dot .github/scripts/pr-docs.test.cjs`: + 50 tests passed, including system/design boundaries, PR-only and missing + requirements, ambiguous definitions, failure recovery, head retries, and + independent merge-group members. Disabling process isolation exposes the + individual tests in this executor, whose default runner reports only the file. +- `python3 .github/scripts/pr-docs-workflow-contract_test.py`: 5 tests passed. +- `python3 scripts/list-docs.py validate`: 265 decisions and 824 specifications validated. +- `python3 scripts/lint-spec-files.py --all`: passed. +- `git diff --check`: passed. + +A GET-only diagnostic using this workspace's fixed evaluator on exact PR +#3626 head `93abff34efe8c9ab2bec68acb8e6aba1eb2a23d4` returned `covered`: six +work orders, nine accepted references, three searches, 18 total requests, and +no failed requests. It published no status. The earlier CI run's caught +exception was not observed, and distinct IDs or a depleted shared quota can +still fail closed. The fix must reach trusted `main` before the privileged +workflow consumes it; rerunning an older run does not substitute this branch's +evaluator. Workflow permissions, bounds, policy, and status rules are unchanged. + +Internal docs updated only. This CI implementation change has no public product +documentation or screenshot impact.