From a9e069ada43edcce4c9537214e19053b622e9959 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Wed, 23 Sep 2026 17:30:09 +0100 Subject: [PATCH] fix: avoid code-search quota for new requirements (#3883) --- .github/scripts/pr-docs.cjs | 96 ++++++++++++++++++++++++-------- .github/scripts/pr-docs.test.cjs | 56 +++++++++++++++---- 2 files changed, 118 insertions(+), 34 deletions(-) diff --git a/.github/scripts/pr-docs.cjs b/.github/scripts/pr-docs.cjs index d919df1c38..a3d00d2ea2 100644 --- a/.github/scripts/pr-docs.cjs +++ b/.github/scripts/pr-docs.cjs @@ -62,7 +62,8 @@ const WORK_ORDER_REQUIRED_FIELDS = [ 'system_design', ]; const MAX_DOCUMENT_BYTES = 256 * 1024; -const MAX_DOCUMENTS = 100; +const MAX_DOCUMENTS = 200; +const MAX_CHANGED_WORK_ORDERS = 100; const MAX_TOTAL_DOCUMENT_BYTES = 4 * 1024 * 1024; const MAX_RESPONSE_BYTES = 8 * 1024 * 1024; const MAX_CHANGED_FILES = 3000; @@ -844,8 +845,8 @@ function validateCoverage({ changedFiles = [], fileContents = {} } = {}) { } result.workOrders = selectChangedWorkOrders(changedFiles); - if (result.workOrders.length > MAX_DOCUMENTS) { - result.errors.push(`More than ${MAX_DOCUMENTS} changed work orders were supplied`); + if (result.workOrders.length > MAX_CHANGED_WORK_ORDERS) { + result.errors.push(`More than ${MAX_CHANGED_WORK_ORDERS} changed work orders were supplied`); } if (result.workOrders.length === 0) { result.status = 'missing'; @@ -1506,7 +1507,8 @@ function changedRequirementSources(changedFiles, requirementDirectory) { } const status = typeof change === 'string' ? undefined : change?.status; const headPath = status === 'removed' ? undefined : normalizedCurrent; - let basePath = normalizedCurrent; + const isAdded = status === 'added'; + let basePath = isAdded ? undefined : normalizedCurrent; if (status === 'renamed' && typeof change.previous_filename === 'string') { try { basePath = normalizeRepoPath(change.previous_filename); @@ -1525,12 +1527,18 @@ function changedRequirementSources(changedFiles, requirementDirectory) { } const key = normalizedHead ?? `base:${normalizedBase}`; if (!sources.has(key)) { - sources.set(key, { basePath: normalizedBase, headPath: normalizedHead }); + sources.set(key, { basePath: normalizedBase, headPath: normalizedHead, isAdded }); } } return [...sources.values()]; } +function candidateRequirementPath(requirementDirectory, requirementId) { + const parts = requirementId.split('-'); + const filename = `${parts.slice(2).join('-').replace(/-\d+$/, '').toLowerCase()}.md`; + return `${requirementDirectory}/${filename}`; +} + async function loadCoverageContents({ client, changedFiles, headSha, baseSha }) { if (!classifyChangedFiles(changedFiles).requiresCoverage) { return {}; @@ -1581,6 +1589,22 @@ async function loadCoverageContents({ client, changedFiles, headSha, baseSha }) return content; } + async function listRequirementDirectory(requirementDirectory, ref) { + const key = JSON.stringify([requirementDirectory, ref]); + if (!requirementDirectories.has(key)) { + let entries = []; + try { + entries = await client.listDirectory(requirementDirectory, ref); + } catch (error) { + if (!isMissingResourceError(error)) { + throw error; + } + } + requirementDirectories.set(key, entries); + } + return requirementDirectories.get(key); + } + for (const workOrderPath of selectChangedWorkOrders(changedFiles)) { const workOrderContent = await load(workOrderPath, headSha, contents); if (workOrderContent === undefined) { @@ -1643,9 +1667,13 @@ async function loadCoverageContents({ client, changedFiles, headSha, baseSha }) const requirementDirectory = `docs/specs/${system}/requirements`; const requirementPaths = new Set(); const baseContentByHeadPath = new Map(); + const addedRequirementPaths = new Set(); for (const source of changedRequirementSources(changedFiles, requirementDirectory)) { if (source.headPath) { requirementPaths.add(source.headPath); + if (source.isAdded) { + addedRequirementPaths.add(source.headPath); + } await load(source.headPath, headSha, contents); } if (source.basePath) { @@ -1655,13 +1683,37 @@ async function loadCoverageContents({ client, changedFiles, headSha, baseSha }) } } } + const checkedAddedRequirementPaths = new Set(); + if (addedRequirementPaths.size > 0 && typeof client.listDirectory === 'function') { + const entries = await listRequirementDirectory(requirementDirectory, headSha); + for (const entry of entries) { + if ( + entry.type !== 'file' + || typeof entry.path !== 'string' + || !entry.path.startsWith(`${requirementDirectory}/`) + || !entry.path.endsWith('.md') + ) { + continue; + } + const pathname = normalizeRepoPath(entry.path); + await load(pathname, headSha, contents); + if (addedRequirementPaths.has(pathname)) { + checkedAddedRequirementPaths.add(pathname); + } + } + } const workOrderRequirements = new Set(workOrder.requirements ?? []); const referencedRequirementIds = designRequirements.filter(requirementId => workOrderRequirements.has(requirementId) ); const verifiedRequirementIds = new Set(); + const ambiguousRequirementIds = new Set(); for (const requirementId of referencedRequirementIds) { const definitions = requirementDefinitions(contents, requirementId, system); + if (definitions.length > 1) { + ambiguousRequirementIds.add(requirementId); + continue; + } if (definitions.length !== 1) { continue; } @@ -1669,7 +1721,13 @@ async function loadCoverageContents({ client, changedFiles, headSha, baseSha }) const baseContent = baseContentByHeadPath.get(definition.pathname); if ( hasRequirementHeading(definition.content, requirementId) - && hasRequirementHeading(baseContent, requirementId) + && ( + hasRequirementHeading(baseContent, requirementId) + || ( + checkedAddedRequirementPaths.has(definition.pathname) + && definition.pathname === candidateRequirementPath(requirementDirectory, requirementId) + ) + ) ) { verifiedRequirementIds.add(requirementId); } @@ -1677,7 +1735,7 @@ async function loadCoverageContents({ client, changedFiles, headSha, baseSha }) const unresolvedRequirementIds = []; if (typeof client.searchCode === 'function') { for (const requirementId of referencedRequirementIds) { - if (verifiedRequirementIds.has(requirementId)) { + if (verifiedRequirementIds.has(requirementId) || ambiguousRequirementIds.has(requirementId)) { continue; } const searchKey = JSON.stringify([requirementDirectory, requirementId]); @@ -1697,26 +1755,16 @@ async function loadCoverageContents({ client, changedFiles, headSha, baseSha }) } } else { unresolvedRequirementIds.push( - ...referencedRequirementIds.filter(requirementId => !verifiedRequirementIds.has(requirementId)), + ...referencedRequirementIds.filter(requirementId => + !verifiedRequirementIds.has(requirementId) && !ambiguousRequirementIds.has(requirementId) + ), ); } if (unresolvedRequirementIds.length > 0 && typeof client.listDirectory === 'function') { - 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`; - })); + const entries = await listRequirementDirectory(requirementDirectory, headSha); + const candidateNames = new Set(unresolvedRequirementIds.map(requirementId => + POSIX_PATH.basename(candidateRequirementPath(requirementDirectory, requirementId)) + )); for (const entry of entries) { if (entry.type === 'file' && candidateNames.has(POSIX_PATH.basename(entry.path))) { requirementPaths.add(entry.path); diff --git a/.github/scripts/pr-docs.test.cjs b/.github/scripts/pr-docs.test.cjs index 14ad613255..1a70ac2790 100644 --- a/.github/scripts/pr-docs.test.cjs +++ b/.github/scripts/pr-docs.test.cjs @@ -1689,23 +1689,23 @@ test('six work orders sharing three requirements stay within the search quota', }); // @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(); +test('newly added requirements resolve from the head diff without code search', async () => { + const { contents, changed, requirementPath: originalPath } = repeatedCoverageFixture(); + const requirementPath = 'docs/specs/ui/requirements/coverage.md'; + contents[requirementPath] = contents[originalPath]; + delete contents[originalPath]; changed.push({ filename: requirementPath, status: 'added' }); let searches = 0; let listings = 0; const client = coverageClient(contents, changed, { async searchCode() { searches += 1; - return []; + throw new Error('code search should not run for a newly added requirement'); }, 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' }]; }, }); @@ -1713,7 +1713,42 @@ test('PR-only requirements and empty searches reuse a bounded directory lookup', const result = await validator.evaluatePullRequest({ client, pullNumber: 42 }); assert.equal(result.status, 'covered', result.errors.join('; ')); - assert.equal(searches, requirementIds.length); + assert.equal(searches, 0); + assert.equal(listings, 1); +}); + +// @covers AC-CI-PR-DOCS-001.5 +test('newly added canonical requirements still reject duplicate IDs', async () => { + const { contents, changed, requirementPath: originalPath } = repeatedCoverageFixture(); + const requirementPath = 'docs/specs/ui/requirements/coverage.md'; + const duplicatePath = 'docs/specs/ui/requirements/duplicate.md'; + contents[requirementPath] = contents[originalPath]; + delete contents[originalPath]; + contents[duplicatePath] = contents[requirementPath]; + changed.push({ filename: requirementPath, status: 'added' }); + let searches = 0; + let listings = 0; + const client = coverageClient(contents, changed, { + async searchCode() { + searches += 1; + return [requirementPath]; + }, + async listDirectory(directory, ref) { + assert.equal(directory, 'docs/specs/ui/requirements'); + assert.equal(ref, SHA_B); + listings += 1; + return [ + { path: requirementPath, type: 'file' }, + { path: duplicatePath, type: 'file' }, + ]; + }, + }); + + const result = await validator.evaluatePullRequest({ client, pullNumber: 42 }); + + assert.equal(result.status, 'invalid', result.errors.join('; ')); + assert.match(result.errors.join('; '), /ambiguous definitions/); + assert.equal(searches, 0); assert.equal(listings, 1); }); @@ -1786,20 +1821,21 @@ 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' }); + const directoryFallbackPath = 'docs/specs/ui/requirements/coverage.md'; + contents[directoryFallbackPath] = contents[requirementPath]; 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 []; + return boundary === 'listDirectory' ? [] : [requirementPath]; }, async listDirectory() { if (fail && boundary === 'listDirectory') { throw new Error('GitHub API request failed with HTTP 503: Service Unavailable'); } - return []; + return [{ path: directoryFallbackPath, type: 'file' }]; }, });