From d95f94754b7876e90ce062a22517551265719348 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Fri, 16 Oct 2026 09:15:50 +1100 Subject: [PATCH 1/7] fix(ci): measure a PR's change impact from the commit its merge was built on On pull_request events the checkout is refs/pull/N/merge, built on main's current tip, but the workflow pinned the target ref to pull_request.base.sha, which lags main whenever main moved on after the PR last synced. Every commit main gained in between was scored as part of the PR: on #1193 that turned an OK (3.8M, 87.9th percentile) into a WARN (10.5M, 91.8th). A new `pin` driver command takes the merge commit's first parent on PRs, and refuses a checkout that is not the merge of the PR head. Push events still pin their own commit. The baseline cache key follows the pinned commit. Closes #1218 --- .github/workflows/impact.yml | 15 ++++-- scripts/__tests__/impact-workflow.test.mjs | 29 ++++++++++-- scripts/impact-gate.py | 38 ++++++++++++++-- scripts/tests/test_impact_gate.py | 53 ++++++++++++++++++++++ 4 files changed, 122 insertions(+), 13 deletions(-) diff --git a/.github/workflows/impact.yml b/.github/workflows/impact.yml index faa48832c..97278edaa 100644 --- a/.github/workflows/impact.yml +++ b/.github/workflows/impact.yml @@ -45,11 +45,16 @@ jobs: id: target env: TARGET_BRANCH: ${{ github.base_ref || github.ref_name }} - BASE_SHA: ${{ github.event.pull_request.base.sha || github.sha }} + EVENT_SHA: ${{ github.sha }} + PR_HEAD: ${{ github.event.pull_request.head.sha }} run: | # Keep one logical target ref across push/PR events, pinned to the - # event's commit even if the remote branch advances while queued. - git update-ref "$BASE_REF" "$BASE_SHA" + # commit this run measures from even if the remote branch advances + # while queued. A PR pins its merge commit's first parent, not + # pull_request.base.sha, which lags main (see `pin` in the driver). + args=(--event-sha "$EVENT_SHA") + if [[ -n "$PR_HEAD" ]]; then args+=(--pr-head "$PR_HEAD"); fi + python scripts/impact-gate.py pin --base "$BASE_REF" "${args[@]}" echo "BASELINE_DIR=$RUNNER_TEMP/impact-baseline" >> "$GITHUB_ENV" python - <<'PY' import hashlib @@ -70,9 +75,9 @@ jobs: uses: actions/cache/restore@0057852bfaa89a56745cba8c7296529d2fc39830 # v4 with: path: ${{ env.BASELINE_DIR }} - key: impact-${{ hashFiles('.github/impact-gate/**', 'scripts/impact-gate.py', '.github/workflows/impact.yml') }}-${{ steps.target.outputs.key }}-${{ github.event.pull_request.base.sha || github.sha }} + key: impact-${{ hashFiles('.github/impact-gate/**', 'scripts/impact-gate.py', '.github/workflows/impact.yml') }}-${{ steps.target.outputs.key }}-${{ steps.target.outputs.sha }} restore-keys: | - impact-${{ hashFiles('.github/impact-gate/**', 'scripts/impact-gate.py', '.github/workflows/impact.yml') }}-${{ steps.target.outputs.key }}-${{ github.event.pull_request.base.sha || github.sha }}- + impact-${{ hashFiles('.github/impact-gate/**', 'scripts/impact-gate.py', '.github/workflows/impact.yml') }}-${{ steps.target.outputs.key }}-${{ steps.target.outputs.sha }}- impact-${{ hashFiles('.github/impact-gate/**', 'scripts/impact-gate.py', '.github/workflows/impact.yml') }}-${{ steps.target.outputs.key }}- - name: Validate or build baselines diff --git a/scripts/__tests__/impact-workflow.test.mjs b/scripts/__tests__/impact-workflow.test.mjs index 93e623bad..b0ef7ec0d 100644 --- a/scripts/__tests__/impact-workflow.test.mjs +++ b/scripts/__tests__/impact-workflow.test.mjs @@ -90,16 +90,16 @@ describe('advisory change impact', () => { expect(restore.with.key).toContain("'scripts/impact-gate.py'") expect(restore.with.key).toContain("'.github/workflows/impact.yml'") expect(restore.with.key).toContain('steps.target.outputs.key') - expect(restore.with.key).toContain( - 'github.event.pull_request.base.sha || github.sha', - ) + // The commit `pin` measured from, never pull_request.base.sha: that lags + // the target whenever it moved on after the PR last synced. + expect(restore.with.key).toContain('steps.target.outputs.sha') // This commit's run-suffixed saves first, then the target branch's // ancestors — never a broader prefix. expect(restore.with['restore-keys'].trim().split('\n')).toEqual([ `${restore.with.key}-`, restore.with.key.replace( // biome-ignore lint/suspicious/noTemplateCurlyInString: literal Actions expression - '-${{ github.event.pull_request.base.sha || github.sha }}', + '-${{ steps.target.outputs.sha }}', '-', ), ]) @@ -125,6 +125,25 @@ describe('advisory change impact', () => { expect(save.with.path).toBe(restore.with.path) }) + it('measures a pull request from the commit its merge was built on', () => { + // pull_request.base.sha lags main whenever main moved on after the PR last + // synced, while the checkout is refs/pull/N/merge on main's current tip; + // scoring from the stale base counted main's newer commits as the PR's. + // `pin` takes the merge commit's first parent instead (test_impact_gate.py). + const job = readWorkflow(workflowPath).jobs.impact + const target = job.steps.find((step) => step.id === 'target') + expect(target.run).toContain( + 'scripts/impact-gate.py pin --base "$BASE_REF"', + ) + // biome-ignore lint/suspicious/noTemplateCurlyInString: literal Actions expression + expect(target.env.EVENT_SHA).toBe('${{ github.sha }}') + // biome-ignore lint/suspicious/noTemplateCurlyInString: literal Actions expression + expect(target.env.PR_HEAD).toBe('${{ github.event.pull_request.head.sha }}') + expect(readFileSync(join(REPO_ROOT, workflowPath), 'utf8')).not.toContain( + 'pull_request.base.sha }}', + ) + }) + it('replaces a corrupt same-commit cache instead of colliding with its key', () => { // A cache key is immutable. When the entry saved for a commit fails // validation, prepare rebuilds; saving under that same key is refused @@ -142,7 +161,7 @@ describe('advisory change impact', () => { const run = (event, runId, attempt, restored) => { const context = { github: { ...event.github, run_id: runId, run_attempt: attempt }, - steps: { target: { outputs: { key: 'main' } } }, + steps: { target: { outputs: { key: 'main', sha: event.github.sha } } }, } const primary = interpolate(restore.with.key, context) context.steps.restore = { diff --git a/scripts/impact-gate.py b/scripts/impact-gate.py index c834b59c8..4898e930f 100644 --- a/scripts/impact-gate.py +++ b/scripts/impact-gate.py @@ -151,15 +151,47 @@ def report(args: argparse.Namespace, policy: str) -> None: publish(result) +def pin(args: argparse.Namespace) -> None: + """Point the target ref at the commit this run's change is measured from. + + A push measures from its own commit. A pull request checks out + refs/pull/N/merge, GitHub's merge of the PR into the target branch's tip at + run time. The event's pull_request.base.sha is older whenever the target + moved on after the PR last synced, and scoring from it counts the target's + newer commits as the PR's. The merge's first parent is the tip it was built + on, so HEAD^1..HEAD is exactly what the PR adds. + """ + base = args.event_sha + if args.pr_head: + parents = run('git', 'rev-list', '--parents', '-n', '1', 'HEAD').split()[1:] + if len(parents) != 2 or parents[1] != args.pr_head: + raise ValueError(f'HEAD is not the merge of {args.pr_head} into the target; ' + 'check out refs/pull/N/merge.') + base = parents[0] + run('git', 'update-ref', args.base, base) + output('sha', base) + + def main() -> int: parser = argparse.ArgumentParser(description=__doc__) - parser.add_argument('command', choices=('prepare', 'report')) + parser.add_argument('command', choices=('pin', 'prepare', 'report')) parser.add_argument('--base', required=True) - parser.add_argument('--cache-dir', type=Path, required=True) + parser.add_argument('--cache-dir', type=Path) parser.add_argument('--refresh', action='store_true') + parser.add_argument('--event-sha', help='pin: the commit the event was raised for') + parser.add_argument('--pr-head', help='pin: the pull request head, on pull_request events') args = parser.parse_args() - args.cache_dir = args.cache_dir.resolve() + if args.command == 'pin': + if not args.event_sha: + parser.error('pin requires --event-sha') + elif args.cache_dir is None: + parser.error(f'{args.command} requires --cache-dir') + else: + args.cache_dir = args.cache_dir.resolve() try: + if args.command == 'pin': + pin(args) + return 0 head = run('git', 'rev-parse', '--verify', args.base + '^{commit}') policy = identity() if args.command == 'prepare': diff --git a/scripts/tests/test_impact_gate.py b/scripts/tests/test_impact_gate.py index 0bbeecca2..b1fec8856 100644 --- a/scripts/tests/test_impact_gate.py +++ b/scripts/tests/test_impact_gate.py @@ -57,6 +57,59 @@ def run_driver(self, command, *args, expect=0, driver=DRIVER): self.assertEqual(result.returncode, expect, result.stdout + result.stderr) return result + def pin(self, *args, expect=0): + result = subprocess.run([sys.executable, str(DRIVER), 'pin', + '--base', 'refs/remotes/origin/main', *args], + cwd=self.repo, text=True, capture_output=True, + env={**os.environ, 'GITHUB_OUTPUT': str(self.output), + 'GITHUB_STEP_SUMMARY': str(self.summary)}) + self.assertEqual(result.returncode, expect, result.stdout + result.stderr) + return result + + def pull_request_merge(self): + """Build GitHub's refs/pull/N/merge after main moved past the PR's base.""" + stale_base = self.git('rev-parse', 'main') + self.git('checkout', '-qb', 'feature') + pr_head = self.commit('src/feature.ts', 'export function feature() { return 1; }\n') + self.git('checkout', '-q', 'main') + landed = self.commit('src/landed.ts', 'export function landed() { return 2; }\n') + self.git('checkout', '-q', '--detach', 'main') + self.git('merge', '-q', '--no-ff', '-m', 'merge', 'feature') + return stale_base, pr_head, landed + + def test_pull_request_scores_only_its_own_changes_when_main_moved_on(self): + stale_base, pr_head, landed = self.pull_request_merge() + # The event's recorded base, which the workflow used to pin: main's + # newer commit then lands inside merge-base..HEAD as if the PR made it. + self.git('update-ref', 'refs/remotes/origin/main', stale_base) + self.run_driver('prepare', '--base', 'refs/remotes/origin/main') + self.run_driver('report', '--base', 'refs/remotes/origin/main') + self.assertIn('src/landed.ts', self.summary.read_text()) + + self.output.write_text('') + self.summary.write_text('') + self.pin('--event-sha', stale_base, '--pr-head', pr_head) + self.assertEqual(self.git('rev-parse', 'refs/remotes/origin/main'), landed) + self.assertIn(f'sha={landed}\n', self.output.read_text()) + self.run_driver('prepare', '--base', 'refs/remotes/origin/main') + self.run_driver('report', '--base', 'refs/remotes/origin/main') + summary = self.summary.read_text() + self.assertIn('src/feature.ts', summary) + self.assertNotIn('src/landed.ts', summary) + + def test_push_pins_the_event_commit(self): + head = self.git('rev-parse', 'HEAD') + self.pin('--event-sha', head) + self.assertEqual(self.git('rev-parse', 'refs/remotes/origin/main'), head) + self.assertIn(f'sha={head}\n', self.output.read_text()) + + def test_pull_request_pin_refuses_a_checkout_that_is_not_the_pr_merge(self): + _, pr_head, _ = self.pull_request_merge() + self.git('checkout', '-q', 'feature') + result = self.pin('--event-sha', pr_head, '--pr-head', pr_head, expect=1) + self.assertIn('not the merge of', self.summary.read_text()) + self.assertNotIn('sha=', result.stdout) + def test_cold_baseline_then_reuse_and_refresh(self): self.run_driver('prepare') self.assertIn('rebuilt=true', self.output.read_text()) From 27ecbaffef54adda5315cb7ccdd4240f7849ae5f Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Fri, 16 Oct 2026 09:24:32 +1100 Subject: [PATCH 2/7] fix(ci): mark change-impact rows whose function span lizard misread lizard's TypeScript reader sometimes misses a function's closing brace and runs its span into the next top-level declaration, so edits to that neighbour are charged to a function the PR never touched. On #1193 the two top cost drivers, formatProgress and pickEncryptedColumn, were such rows. The report now marks a function row when lizard's span, at the merge-base or at HEAD, contains a column-0 declaration strictly inside it, and explains the mark under the table. Scores, grades and the job outcome are unchanged. Closes #1219 --- scripts/impact-gate.py | 57 ++++++++++++++++++++++++++++++- scripts/tests/test_impact_gate.py | 35 +++++++++++++++++++ 2 files changed, 91 insertions(+), 1 deletion(-) diff --git a/scripts/impact-gate.py b/scripts/impact-gate.py index 4898e930f..b2b9f5654 100644 --- a/scripts/impact-gate.py +++ b/scripts/impact-gate.py @@ -10,12 +10,14 @@ import json import os from pathlib import Path +import re import subprocess import sys import tempfile from impact_gate.baseline import build_baseline, save_baseline from impact_gate.core.config import MeasureConfig +import lizard ROOT = Path(__file__).resolve().parents[1] POLICY = ROOT / '.github' / 'impact-gate' @@ -148,7 +150,60 @@ def report(args: argparse.Namespace, policy: str) -> None: '--enforcement', 'warn', '--format', 'markdown', '--warn-percentile', '90', '--block-percentile', '98', '--curve-prior-weight', '200', '--baseline-file', str(path), '--measure-config', str(POLICY / f'{scope}.yml')) - publish(result) + publish(mark_misread_spans(result, args.base)) + + +# lizard's TypeScript reader sometimes misses a function's closing brace (an +# arrow callback followed by a ternary returning an object literal is one +# trigger) and runs the span on into the next top-level declaration. Edits to +# that neighbour are then charged to a function the PR never touched. In +# formatted code nothing inside a function body starts at column 0, so a +# top-level declaration strictly inside a span is the tell. +SPAN_SUFFIXES = ('.ts', '.tsx', '.js', '.jsx') +DECLARATION = re.compile( + r'(export\s+)?(default\s+)?(async\s+)?(function|const|let|var|class|interface|type|enum)\b') +LOCATION = re.compile(r'^(\| [^|]* \| )`([^`]+):([^`:]+)`( \|)') +MISREAD = '⚠️ span may be misattributed' +MISREAD_NOTE = (f'{MISREAD}: lizard read its line span to include a later top-level ' + 'declaration, so edits to that code are charged to this function. Check ' + 'the diff before refactoring it. Scores and grades are unchanged.') + + +def misread_functions(revisions: list[str], path: str) -> set[str]: + names = set() + for revision in revisions: + shown = subprocess.run(['git', 'show', f'{revision}:{path}'], + capture_output=True, text=True) + if shown.returncode != 0: # Added or deleted on one side. + continue + lines = shown.stdout.split('\n') + for function in lizard.analyze_file.analyze_source_code(path, shown.stdout).function_list: + # lizard often ends a span on the next declaration's first line; + # that boundary is harmless, so only strictly inner lines count. + if any(DECLARATION.match(lines[n - 1]) + for n in range(function.start_line + 1, function.end_line)): + names.add(function.name.split('::')[-1]) + return names + + +def mark_misread_spans(markdown: str, base: str) -> str: + revisions = [run('git', 'merge-base', base, 'HEAD'), 'HEAD'] + cache: dict[str, set[str]] = {} + marked = False + lines = markdown.split('\n') + for index, line in enumerate(lines): + match = LOCATION.match(line) + if not match or not match.group(2).endswith(SPAN_SUFFIXES): + continue + path, name = match.group(2), match.group(3) + if path not in cache: + cache[path] = misread_functions(revisions, path) + if name in cache[path]: + lines[index] = (match.group(1) + f'`{path}:{name}` {MISREAD}' + + match.group(4) + line[match.end():]) + marked = True + text = '\n'.join(lines) + return text + f'\n\n{MISREAD_NOTE}' if marked else text def pin(args: argparse.Namespace) -> None: diff --git a/scripts/tests/test_impact_gate.py b/scripts/tests/test_impact_gate.py index b1fec8856..247458c2f 100644 --- a/scripts/tests/test_impact_gate.py +++ b/scripts/tests/test_impact_gate.py @@ -110,6 +110,41 @@ def test_pull_request_pin_refuses_a_checkout_that_is_not_the_pr_merge(self): self.assertIn('not the merge of', self.summary.read_text()) self.assertNotIn('sha=', result.stdout) + def test_marks_cost_drivers_whose_function_span_lizard_misread(self): + # lizard ends `pick` at the end of `after`: a ternary returning an + # object literal after an arrow callback loses the closing brace. Edits + # to `after` are then charged to `pick`, which the PR never touched. + def branches(name, arg, result): + return (f'export function {name}({arg}: number): number {{\n' + + ''.join(f' if ({arg} === {i}) return {i};\n' for i in range(200)) + + f' return {result}\n}}\n') + pick = ('export function pick(xs: readonly string[], hint?: string) {\n' + ' const hinted = xs.find((c) => c === hint)\n' + " return hinted ? { hinted, via: 'hint' } : null\n" + '}\n') + self.commit('src/pick.ts', 'export function third(y: number): number {\n return y\n}\n\n' + + pick + '\nexport function after(x: number): number {\n return x + 1\n}\n') + self.run_driver('prepare') + self.git('checkout', '-qb', 'feature') + self.commit('src/pick.ts', branches('third', 'y', 'y') + '\n' + pick + '\n' + + branches('after', 'x', 'x + 1')) + # The grade multiplies cost by files changed; the drivers table is only + # printed above the warn percentile. + for index in range(6): + self.commit(f'src/extra{index}.ts', f'export function extra{index}() {{ return 1; }}\n') + self.run_driver('report') + rows = [line for line in self.summary.read_text().splitlines() + if line.startswith('|') and 'src/pick.ts:' in line] + pick_rows = [row for row in rows if 'src/pick.ts:pick`' in row] + third_rows = [row for row in rows if 'src/pick.ts:third`' in row] + self.assertTrue(pick_rows, self.summary.read_text()) + self.assertTrue(third_rows, self.summary.read_text()) + for row in pick_rows: + self.assertIn('span may be misattributed', row) + for row in third_rows: + self.assertNotIn('span may be misattributed', row) + self.assertIn('lizard read its line span', self.summary.read_text()) + def test_cold_baseline_then_reuse_and_refresh(self): self.run_driver('prepare') self.assertIn('rebuilt=true', self.output.read_text()) From 71f23609281adb1dc0241f6925751af82442a2e1 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Fri, 16 Oct 2026 09:32:41 +1100 Subject: [PATCH 3/7] fix(ci): harden the misread-span label against false marks and failures Review follow-ups for the span label: - Pygments decides what is code, so a column-0 `const` or `import` inside a source-generating template literal no longer reads as a declaration. - Rows match on class and name, and (anonymous) is never marked, so one misread function does not label every same-named row in the file. - import, declare, abstract and decorators count as declarations. - A failure while checking spans appends a note and keeps the report, rather than failing the job and losing the second scope. - Both scopes share one merge-base and one parse per file. Refs #1219 --- scripts/impact-gate.py | 93 +++++++++++++++++++------------ scripts/tests/test_impact_gate.py | 28 ++++++++++ 2 files changed, 86 insertions(+), 35 deletions(-) diff --git a/scripts/impact-gate.py b/scripts/impact-gate.py index b2b9f5654..4833cb05d 100644 --- a/scripts/impact-gate.py +++ b/scripts/impact-gate.py @@ -18,6 +18,8 @@ from impact_gate.baseline import build_baseline, save_baseline from impact_gate.core.config import MeasureConfig import lizard +from pygments.lexers import JavascriptLexer, TypeScriptLexer +from pygments.token import Keyword, Name ROOT = Path(__file__).resolve().parents[1] POLICY = ROOT / '.github' / 'impact-gate' @@ -140,6 +142,7 @@ def report(args: argparse.Namespace, policy: str) -> None: f'Baseline target: `{args.base}`; commit: `{meta["head"]}`. ' f'History: latest {HISTORY_COMMITS} first-parent commits; recursively expanded ' 'merge observations may exceed this limit. p90/p98 are advisory; prior weight: 200.') + spans: dict = {} # Both scopes list the same functions. for scope, label in SCOPES.items(): path = args.cache_dir / f'{scope}.json' n = read_baseline(path, meta['head']) @@ -150,7 +153,7 @@ def report(args: argparse.Namespace, policy: str) -> None: '--enforcement', 'warn', '--format', 'markdown', '--warn-percentile', '90', '--block-percentile', '98', '--curve-prior-weight', '200', '--baseline-file', str(path), '--measure-config', str(POLICY / f'{scope}.yml')) - publish(mark_misread_spans(result, args.base)) + publish(mark_misread_spans(result, args.base, spans)) # lizard's TypeScript reader sometimes misses a function's closing brace (an @@ -158,52 +161,72 @@ def report(args: argparse.Namespace, policy: str) -> None: # trigger) and runs the span on into the next top-level declaration. Edits to # that neighbour are then charged to a function the PR never touched. In # formatted code nothing inside a function body starts at column 0, so a -# top-level declaration strictly inside a span is the tell. +# top-level declaration strictly inside a span is the tell. Pygments decides +# what is code: a column-0 `const` inside a template literal is string text. SPAN_SUFFIXES = ('.ts', '.tsx', '.js', '.jsx') -DECLARATION = re.compile( - r'(export\s+)?(default\s+)?(async\s+)?(function|const|let|var|class|interface|type|enum)\b') -LOCATION = re.compile(r'^(\| [^|]* \| )`([^`]+):([^`:]+)`( \|)') +DECLARATION = re.compile(r'@|(export|import|declare|abstract|async|function|const|let|var' + r'|class|interface|type|enum)\b') +LOCATION = re.compile(r'^\| [^|]* \| `(?P[^`:]+):(?P[^`]+)` \| ' + r'(?:`(?P[^`]+)`|_file scope_) \|') MISREAD = '⚠️ span may be misattributed' MISREAD_NOTE = (f'{MISREAD}: lizard read its line span to include a later top-level ' 'declaration, so edits to that code are charged to this function. Check ' 'the diff before refactoring it. Scores and grades are unchanged.') -def misread_functions(revisions: list[str], path: str) -> set[str]: - names = set() +def declaration_lines(path: str, source: str) -> set[int]: + lexer = TypeScriptLexer() if path.endswith(('.ts', '.tsx')) else JavascriptLexer() + line_at, offset = {}, 0 + for number, line in enumerate(source.split('\n'), 1): + line_at[offset] = number + offset += len(line) + 1 + return {line_at[offset] for offset, token, _ in lexer.get_tokens_unprocessed(source) + if offset in line_at and (token in Keyword or token in Name.Decorator) + and DECLARATION.match(source, offset)} + + +def misread_functions(revisions: list[str], path: str) -> set[tuple[str, str]]: + """(owner, name) of each function whose span, in any revision, swallows a declaration.""" + found = set() for revision in revisions: - shown = subprocess.run(['git', 'show', f'{revision}:{path}'], - capture_output=True, text=True) + shown = subprocess.run(['git', 'show', f'{revision}:{path}'], capture_output=True) if shown.returncode != 0: # Added or deleted on one side. continue - lines = shown.stdout.split('\n') - for function in lizard.analyze_file.analyze_source_code(path, shown.stdout).function_list: + source = shown.stdout.decode('utf-8', errors='replace') + declarations = declaration_lines(path, source) + for function in lizard.analyze_file.analyze_source_code(path, source).function_list: + owner, _, name = function.name.rpartition('::') # lizard often ends a span on the next declaration's first line; # that boundary is harmless, so only strictly inner lines count. - if any(DECLARATION.match(lines[n - 1]) - for n in range(function.start_line + 1, function.end_line)): - names.add(function.name.split('::')[-1]) - return names - - -def mark_misread_spans(markdown: str, base: str) -> str: - revisions = [run('git', 'merge-base', base, 'HEAD'), 'HEAD'] - cache: dict[str, set[str]] = {} - marked = False - lines = markdown.split('\n') - for index, line in enumerate(lines): - match = LOCATION.match(line) - if not match or not match.group(2).endswith(SPAN_SUFFIXES): - continue - path, name = match.group(2), match.group(3) - if path not in cache: - cache[path] = misread_functions(revisions, path) - if name in cache[path]: - lines[index] = (match.group(1) + f'`{path}:{name}` {MISREAD}' - + match.group(4) + line[match.end():]) - marked = True - text = '\n'.join(lines) - return text + f'\n\n{MISREAD_NOTE}' if marked else text + if name != '(anonymous)' and any( + function.start_line < n < function.end_line for n in declarations): + found.add((owner, name)) + return found + + +def mark_misread_spans(markdown: str, base: str, cache: dict | None = None) -> str: + """Label function rows whose span lizard misread. Never fails the report.""" + cache = {} if cache is None else cache + try: + if 'revisions' not in cache: + cache['revisions'] = [run('git', 'merge-base', base, 'HEAD'), 'HEAD'] + marked = False + lines = markdown.split('\n') + for index, line in enumerate(lines): + match = LOCATION.match(line) + if not match or not match['path'].endswith(SPAN_SUFFIXES): + continue + path = match['path'] + if path not in cache: + cache[path] = misread_functions(cache['revisions'], path) + if (match['owner'] or '', match['name']) in cache[path]: + cell = f'`{path}:{match["name"]}`' + lines[index] = line.replace(cell, f'{cell} {MISREAD}', 1) + marked = True + text = '\n'.join(lines) + return text + f'\n\n{MISREAD_NOTE}' if marked else text + except Exception as error: # A cosmetic label must not cost the report. + return markdown + f'\n\n_Note: could not check function spans ({describe(error)})._' def pin(args: argparse.Namespace) -> None: diff --git a/scripts/tests/test_impact_gate.py b/scripts/tests/test_impact_gate.py index 247458c2f..2dc7a0db4 100644 --- a/scripts/tests/test_impact_gate.py +++ b/scripts/tests/test_impact_gate.py @@ -1,4 +1,5 @@ """Exercise the workflow driver against the pinned real CLI and Git histories.""" +import importlib.util import json import os from pathlib import Path @@ -145,6 +146,33 @@ def branches(name, arg, result): self.assertNotIn('span may be misattributed', row) self.assertIn('lizard read its line span', self.summary.read_text()) + def load_driver(self): + spec = importlib.util.spec_from_file_location('impact_gate_driver', DRIVER) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + def test_code_generating_template_literals_are_not_read_as_declarations(self): + # A template that emits source puts `const`/`import` at column 0 inside + # the string; that is not a declaration lizard ran into. + self.commit('src/gen.ts', 'export function generate(name: string): string {\n' + ' return `\nimport { x } from "y"\nconst ${name} = x\n' + 'export const client = ${name}\n`\n}\n') + head = self.git('rev-parse', 'HEAD') + cwd = os.getcwd() + os.chdir(self.repo) + self.addCleanup(os.chdir, cwd) + self.assertEqual(self.load_driver().misread_functions([head], 'src/gen.ts'), set()) + + def test_marking_failure_keeps_the_unmarked_report(self): + cwd = os.getcwd() + os.chdir(self.repo) + self.addCleanup(os.chdir, cwd) + report = '| 1 | `src/main.ts:count` | _file scope_ | 1 | 0 | mutation |' + marked = self.load_driver().mark_misread_spans(report, 'missing-target') + self.assertTrue(marked.startswith(report)) + self.assertIn('could not check function spans', marked) + def test_cold_baseline_then_reuse_and_refresh(self): self.run_driver('prepare') self.assertIn('rebuilt=true', self.output.read_text()) From aeaf4490f656f74bc8463a2dca7afbbb95fbf906 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 23:22:07 +0000 Subject: [PATCH 4/7] fix(ci): count braces before calling a declaration top level, and cover both misread shapes Address review feedback on #1223: - declaration_lines tracks brace depth, so an unformatted function body with column-0 lines is no longer marked as misread. - Fixtures for a function above a removed declaration and above appended code, each with a really-edited function left unmarked. - Document that a misread which drops functions cannot be marked. - pin docstring: the merge is built when the event fires, not at run time. - requirements.txt header names pygments as a direct dependency. - Rollout plan notes the label and the boundary case it skips. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01XsxT13tj7h1VEF8nNgHA1m --- .github/impact-gate/requirements.txt | 9 ++-- docs/plans/2026-10-08-impact-gate-rollout.md | 2 +- scripts/impact-gate.py | 28 ++++++---- scripts/tests/test_impact_gate.py | 55 +++++++++++++++----- 4 files changed, 68 insertions(+), 26 deletions(-) diff --git a/.github/impact-gate/requirements.txt b/.github/impact-gate/requirements.txt index ccf657c97..5f5e7b084 100644 --- a/.github/impact-gate/requirements.txt +++ b/.github/impact-gate/requirements.txt @@ -2,11 +2,12 @@ # CI installs it with `pip install --require-hashes --no-deps`, so nothing here # resolves at install time and a transitive dependency cannot float. # -# The top-level pins are impact-gate==0.4.1 and lizard==1.23.0 (Lizard 1.24.0 is -# excluded by ImpactGate's own metadata). To regenerate after changing them, from -# the repository root: +# The top-level pins are impact-gate==0.4.1, lizard==1.23.0 (Lizard 1.24.0 is +# excluded by ImpactGate's own metadata) and pygments==2.21.0, which +# scripts/impact-gate.py imports directly. uv still annotates pygments "via +# lizard". To regenerate after changing them, from the repository root: # -# printf 'impact-gate==0.4.1\nlizard==1.23.0\n' \ +# printf 'impact-gate==0.4.1\nlizard==1.23.0\npygments==2.21.0\n' \ # | uv pip compile - --generate-hashes --universal --python-version 3.12 \ # --no-header -o .github/impact-gate/requirements.txt # diff --git a/docs/plans/2026-10-08-impact-gate-rollout.md b/docs/plans/2026-10-08-impact-gate-rollout.md index 10d592cfb..67b568e23 100644 --- a/docs/plans/2026-10-08-impact-gate-rollout.md +++ b/docs/plans/2026-10-08-impact-gate-rollout.md @@ -26,7 +26,7 @@ Manual comparison of representative Stack source at the base SHA also found a re The inspected Rust `is_fresh_at` in `packages/stack-encrypt/src/keyset.rs` is correctly reported at 103–105 (CC 1). Go `StatusError` and `PackedResult` in `languages/golang/internal/guest/status.go` are correctly reported at 45–106 (CC 29) and 111–116 (CC 2). These are sample checks, not proof that either parser is universally correct. -Keep the report advisory. Do not modify production source to satisfy this parser. These silent span defects are different from process failures: successful CLI exit cannot establish accurate function boundaries. +Keep the report advisory. Do not modify production source to satisfy this parser. These silent span defects are different from process failures: successful CLI exit cannot establish accurate function boundaries. The report now labels a function row `⚠️ span may be misattributed` when lizard's span contains a later top-level declaration (#1219). It does not label a span that ends on the next declaration's first line, so `parseSelectorSegments` above stays unlabelled; that boundary is harmless because the neighbour's body is still outside the span. ## Coverage and interpretation diff --git a/scripts/impact-gate.py b/scripts/impact-gate.py index 4833cb05d..da70ed244 100644 --- a/scripts/impact-gate.py +++ b/scripts/impact-gate.py @@ -19,7 +19,7 @@ from impact_gate.core.config import MeasureConfig import lizard from pygments.lexers import JavascriptLexer, TypeScriptLexer -from pygments.token import Keyword, Name +from pygments.token import Keyword, Name, Punctuation ROOT = Path(__file__).resolve().parents[1] POLICY = ROOT / '.github' / 'impact-gate' @@ -163,6 +163,9 @@ def report(args: argparse.Namespace, policy: str) -> None: # formatted code nothing inside a function body starts at column 0, so a # top-level declaration strictly inside a span is the tell. Pygments decides # what is code: a column-0 `const` inside a template literal is string text. +# Limit: when the misread makes lizard drop functions instead (the same +# trigger inside a class method reports only an `(anonymous)` function), the +# edits land on a row that has no span to check, and nothing is marked. SPAN_SUFFIXES = ('.ts', '.tsx', '.js', '.jsx') DECLARATION = re.compile(r'@|(export|import|declare|abstract|async|function|const|let|var' r'|class|interface|type|enum)\b') @@ -180,9 +183,15 @@ def declaration_lines(path: str, source: str) -> set[int]: for number, line in enumerate(source.split('\n'), 1): line_at[offset] = number offset += len(line) + 1 - return {line_at[offset] for offset, token, _ in lexer.get_tokens_unprocessed(source) - if offset in line_at and (token in Keyword or token in Name.Decorator) - and DECLARATION.match(source, offset)} + # Column 0 is not top level in unformatted code, so count braces too. + declarations, depth = set(), 0 + for offset, token, value in lexer.get_tokens_unprocessed(source): + if (depth == 0 and offset in line_at and (token in Keyword or token in Name.Decorator) + and DECLARATION.match(source, offset)): + declarations.add(line_at[offset]) + elif token in Punctuation: + depth = max(0, depth + value.count('{') - value.count('}')) + return declarations def misread_functions(revisions: list[str], path: str) -> set[tuple[str, str]]: @@ -233,11 +242,12 @@ def pin(args: argparse.Namespace) -> None: """Point the target ref at the commit this run's change is measured from. A push measures from its own commit. A pull request checks out - refs/pull/N/merge, GitHub's merge of the PR into the target branch's tip at - run time. The event's pull_request.base.sha is older whenever the target - moved on after the PR last synced, and scoring from it counts the target's - newer commits as the PR's. The merge's first parent is the tip it was built - on, so HEAD^1..HEAD is exactly what the PR adds. + refs/pull/N/merge, GitHub's merge of the PR into the target branch's tip + when the event fired (a re-run reuses it). The event's pull_request.base.sha + is older whenever the target moved on after the PR last synced, and scoring + from it counts the target's newer commits as the PR's. The merge's first + parent is the tip it was built on, so HEAD^1..HEAD is exactly what the PR + adds. """ base = args.event_sha if args.pr_head: diff --git a/scripts/tests/test_impact_gate.py b/scripts/tests/test_impact_gate.py index 2dc7a0db4..39803e1ac 100644 --- a/scripts/tests/test_impact_gate.py +++ b/scripts/tests/test_impact_gate.py @@ -10,6 +10,12 @@ import unittest DRIVER = Path(__file__).resolve().parents[1] / 'impact-gate.py' +# lizard misses this function's closing brace (a ternary returning an object +# literal after an arrow callback) and runs its span on into whatever follows. +PICK = ('export function pick(xs: readonly string[], hint?: string) {\n' + ' const hinted = xs.find((c) => c === hint)\n' + " return hinted ? { hinted, via: 'hint' } : null\n" + '}\n') class ImpactWorkflowTest(unittest.TestCase): @@ -119,15 +125,11 @@ def branches(name, arg, result): return (f'export function {name}({arg}: number): number {{\n' + ''.join(f' if ({arg} === {i}) return {i};\n' for i in range(200)) + f' return {result}\n}}\n') - pick = ('export function pick(xs: readonly string[], hint?: string) {\n' - ' const hinted = xs.find((c) => c === hint)\n' - " return hinted ? { hinted, via: 'hint' } : null\n" - '}\n') self.commit('src/pick.ts', 'export function third(y: number): number {\n return y\n}\n\n' - + pick + '\nexport function after(x: number): number {\n return x + 1\n}\n') + + PICK + '\nexport function after(x: number): number {\n return x + 1\n}\n') self.run_driver('prepare') self.git('checkout', '-qb', 'feature') - self.commit('src/pick.ts', branches('third', 'y', 'y') + '\n' + pick + '\n' + self.commit('src/pick.ts', branches('third', 'y', 'y') + '\n' + PICK + '\n' + branches('after', 'x', 'x + 1')) # The grade multiplies cost by files changed; the drivers table is only # printed above the warn percentile. @@ -147,6 +149,10 @@ def branches(name, arg, result): self.assertIn('lizard read its line span', self.summary.read_text()) def load_driver(self): + """Import the driver with the fixture repository as the working directory.""" + cwd = os.getcwd() + os.chdir(self.repo) + self.addCleanup(os.chdir, cwd) spec = importlib.util.spec_from_file_location('impact_gate_driver', DRIVER) module = importlib.util.module_from_spec(spec) spec.loader.exec_module(module) @@ -159,15 +165,40 @@ def test_code_generating_template_literals_are_not_read_as_declarations(self): ' return `\nimport { x } from "y"\nconst ${name} = x\n' 'export const client = ${name}\n`\n}\n') head = self.git('rev-parse', 'HEAD') - cwd = os.getcwd() - os.chdir(self.repo) - self.addCleanup(os.chdir, cwd) self.assertEqual(self.load_driver().misread_functions([head], 'src/gen.ts'), set()) + def test_unindented_declarations_inside_a_function_body_are_not_top_level(self): + # Column 0 alone is not top level: an unformatted body whose span + # lizard read correctly must not be marked. + self.commit('src/flat.ts', 'export function outer(): number {\nconst inner = 2\n' + 'function nested() { return inner }\nreturn nested()\n}\n\n' + 'export const sibling = 3\n') + head = self.git('rev-parse', 'HEAD') + self.assertEqual(self.load_driver().misread_functions([head], 'src/flat.ts'), set()) + + def test_marks_a_function_above_a_removed_declaration(self): + # At the base, `pick`'s span swallows `gone`; the PR deletes `gone`. + self.commit('src/pick.ts', PICK + '\nexport function gone(x: number): number {\n' + ' return x\n}\n\nexport function kept(y: number): number {\n return y\n}\n') + base = self.git('rev-parse', 'HEAD') + self.commit('src/pick.ts', PICK + '\nexport function kept(y: number): number {\n' + ' return y + 1\n}\n') + found = self.load_driver().misread_functions([base, 'HEAD'], 'src/pick.ts') + self.assertIn(('', 'pick'), found) + self.assertNotIn(('', 'kept'), found) + + def test_marks_a_function_above_appended_code(self): + # At the base `pick` ends the file; the PR appends after it. + self.commit('src/pick.ts', 'export function edited(y: number): number {\n return y\n}\n\n' + + PICK) + base = self.git('rev-parse', 'HEAD') + self.commit('src/pick.ts', 'export function edited(y: number): number {\n return y + 1\n}\n\n' + + PICK + '\nexport function appended(x: number): number {\n return x\n}\n') + found = self.load_driver().misread_functions([base, 'HEAD'], 'src/pick.ts') + self.assertIn(('', 'pick'), found) + self.assertNotIn(('', 'edited'), found) + def test_marking_failure_keeps_the_unmarked_report(self): - cwd = os.getcwd() - os.chdir(self.repo) - self.addCleanup(os.chdir, cwd) report = '| 1 | `src/main.ts:count` | _file scope_ | 1 | 0 | mutation |' marked = self.load_driver().mark_misread_spans(report, 'missing-target') self.assertTrue(marked.startswith(report)) From 5a43b2abe84fda01b3c9d4b8f8f78750f773a5bd Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 23:44:45 +0000 Subject: [PATCH 5/7] fix(ci): mark a span that ends on a whole one-line declaration lizard often ends a function's span on the next declaration's first line. That was exempt as a harmless boundary, but when the next declaration is a one-liner (`export const LIMIT = 10`, a one-line arrow function) the whole of it is inside the span, and edits to it are charged to the function above. declaration_lines now reports whether each declaration closes on its own line, and misread_functions marks a span ending on one that does. A span ending on the first line of a multi-line declaration stays exempt. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01XsxT13tj7h1VEF8nNgHA1m --- docs/plans/2026-10-08-impact-gate-rollout.md | 2 +- scripts/impact-gate.py | 32 ++++++++++++-------- scripts/tests/test_impact_gate.py | 16 +++++++++- 3 files changed, 36 insertions(+), 14 deletions(-) diff --git a/docs/plans/2026-10-08-impact-gate-rollout.md b/docs/plans/2026-10-08-impact-gate-rollout.md index 67b568e23..ec7fd6040 100644 --- a/docs/plans/2026-10-08-impact-gate-rollout.md +++ b/docs/plans/2026-10-08-impact-gate-rollout.md @@ -26,7 +26,7 @@ Manual comparison of representative Stack source at the base SHA also found a re The inspected Rust `is_fresh_at` in `packages/stack-encrypt/src/keyset.rs` is correctly reported at 103–105 (CC 1). Go `StatusError` and `PackedResult` in `languages/golang/internal/guest/status.go` are correctly reported at 45–106 (CC 29) and 111–116 (CC 2). These are sample checks, not proof that either parser is universally correct. -Keep the report advisory. Do not modify production source to satisfy this parser. These silent span defects are different from process failures: successful CLI exit cannot establish accurate function boundaries. The report now labels a function row `⚠️ span may be misattributed` when lizard's span contains a later top-level declaration (#1219). It does not label a span that ends on the next declaration's first line, so `parseSelectorSegments` above stays unlabelled; that boundary is harmless because the neighbour's body is still outside the span. +Keep the report advisory. Do not modify production source to satisfy this parser. These silent span defects are different from process failures: successful CLI exit cannot establish accurate function boundaries. The report now labels a function row `⚠️ span may be misattributed` when lizard's span contains a later top-level declaration (#1219). A span that ends on the first line of a multi-line declaration is not labelled, so `parseSelectorSegments` above stays unlabelled: the neighbour's body is still outside the span. A span that ends on a one-line declaration, such as `export const LIMIT = 10`, is labelled, because that whole declaration is inside it. ## Coverage and interpretation diff --git a/scripts/impact-gate.py b/scripts/impact-gate.py index da70ed244..5f96fbaab 100644 --- a/scripts/impact-gate.py +++ b/scripts/impact-gate.py @@ -5,6 +5,7 @@ so tests can exercise the same workflow against isolated Git histories. """ import argparse +import bisect import hashlib import importlib.metadata import json @@ -177,21 +178,26 @@ def report(args: argparse.Namespace, policy: str) -> None: 'the diff before refactoring it. Scores and grades are unchanged.') -def declaration_lines(path: str, source: str) -> set[int]: +def declaration_lines(path: str, source: str) -> dict[int, bool]: + """Line of each top-level declaration, mapped to whether it ends on that line.""" lexer = TypeScriptLexer() if path.endswith(('.ts', '.tsx')) else JavascriptLexer() - line_at, offset = {}, 0 - for number, line in enumerate(source.split('\n'), 1): - line_at[offset] = number - offset += len(line) + 1 + starts = [0] + for line in source.split('\n')[:-1]: + starts.append(starts[-1] + len(line) + 1) # Column 0 is not top level in unformatted code, so count braces too. - declarations, depth = set(), 0 + # Brackets of every kind decide whether a declaration closes on its line. + declarations, depth, nesting, open_at_end = set(), 0, 0, {} for offset, token, value in lexer.get_tokens_unprocessed(source): - if (depth == 0 and offset in line_at and (token in Keyword or token in Name.Decorator) + line = bisect.bisect_right(starts, offset) + if (depth == 0 and starts[line - 1] == offset + and (token in Keyword or token in Name.Decorator) and DECLARATION.match(source, offset)): - declarations.add(line_at[offset]) + declarations.add(line) elif token in Punctuation: depth = max(0, depth + value.count('{') - value.count('}')) - return declarations + nesting = max(0, nesting + sum(map(value.count, '({[')) - sum(map(value.count, ')}]'))) + open_at_end[line] = nesting > 0 + return {line: not open_at_end.get(line, False) for line in declarations} def misread_functions(revisions: list[str], path: str) -> set[tuple[str, str]]: @@ -205,10 +211,12 @@ def misread_functions(revisions: list[str], path: str) -> set[tuple[str, str]]: declarations = declaration_lines(path, source) for function in lizard.analyze_file.analyze_source_code(path, source).function_list: owner, _, name = function.name.rpartition('::') - # lizard often ends a span on the next declaration's first line; - # that boundary is harmless, so only strictly inner lines count. + # lizard often ends a span on the next declaration's first line. + # That boundary is harmless unless the whole declaration is there. if name != '(anonymous)' and any( - function.start_line < n < function.end_line for n in declarations): + function.start_line < n < function.end_line + or (n == function.end_line and whole) + for n, whole in declarations.items()): found.add((owner, name)) return found diff --git a/scripts/tests/test_impact_gate.py b/scripts/tests/test_impact_gate.py index 39803e1ac..ad5bd6845 100644 --- a/scripts/tests/test_impact_gate.py +++ b/scripts/tests/test_impact_gate.py @@ -172,10 +172,24 @@ def test_unindented_declarations_inside_a_function_body_are_not_top_level(self): # lizard read correctly must not be marked. self.commit('src/flat.ts', 'export function outer(): number {\nconst inner = 2\n' 'function nested() { return inner }\nreturn nested()\n}\n\n' - 'export const sibling = 3\n') + 'export function sibling(): number {\n return 3\n}\n') head = self.git('rev-parse', 'HEAD') self.assertEqual(self.load_driver().misread_functions([head], 'src/flat.ts'), set()) + def test_a_span_ending_on_a_one_line_declaration_swallows_it(self): + # lizard ends `path` on the next declaration's first line. That is + # harmless when only a signature is there, but a one-line declaration + # is there whole, so edits to it are charged to `path`. + path = ('export function path(segments: string[]): string {\n' + ' return `$.${segments.join(".")}`\n}\n\n') + self.commit('src/path.ts', path + 'export const LIMIT = 10\n') + self.commit('src/boundary.ts', path + 'export function next(x: number): number {\n' + ' return x\n}\n') + head = self.git('rev-parse', 'HEAD') + driver = self.load_driver() + self.assertEqual(driver.misread_functions([head], 'src/path.ts'), {('', 'path')}) + self.assertEqual(driver.misread_functions([head], 'src/boundary.ts'), set()) + def test_marks_a_function_above_a_removed_declaration(self): # At the base, `pick`'s span swallows `gone`; the PR deletes `gone`. self.commit('src/pick.ts', PICK + '\nexport function gone(x: number): number {\n' From 8914409aa6553ab10746f2df8f7b8682ed009b05 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 16 Oct 2026 01:05:10 +0000 Subject: [PATCH 6/7] fix(ci): a declaration that continues on the next line is not whole declaration_lines called a declaration whole whenever its first line left no bracket open. `export const LIMIT =` / ` 10` and `export type T =` / ` | 'a'` balance on line 1 and still continue, so a span ending on that line was marked although the declaration's body lies outside it. A first line now continues when it ends on an operator (other than a closing generic `>`, a non-null `!` or `++`/`--`), `=>`, `,` or `.`, or when the next code line leads with a token that can only continue an expression (`|`, `&`, `.`, `?.`, `?`, `:`, `=>`, `&&`, `||`, `??`, `=`, `,`). Across the repo's TypeScript sources this drops 12 false marks, such as `isCiEnvBroad` before `export type CallerKind =`, and adds none. Reported by freshtonic in the #1223 review. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01XsxT13tj7h1VEF8nNgHA1m --- scripts/impact-gate.py | 23 +++++++++++++++++++++-- scripts/tests/test_impact_gate.py | 19 +++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/scripts/impact-gate.py b/scripts/impact-gate.py index 5f96fbaab..e0736e013 100644 --- a/scripts/impact-gate.py +++ b/scripts/impact-gate.py @@ -20,7 +20,7 @@ from impact_gate.core.config import MeasureConfig import lizard from pygments.lexers import JavascriptLexer, TypeScriptLexer -from pygments.token import Keyword, Name, Punctuation +from pygments.token import Comment, Keyword, Name, Operator, Punctuation ROOT = Path(__file__).resolve().parents[1] POLICY = ROOT / '.github' / 'impact-gate' @@ -170,6 +170,9 @@ def report(args: argparse.Namespace, policy: str) -> None: SPAN_SUFFIXES = ('.ts', '.tsx', '.js', '.jsx') DECLARATION = re.compile(r'@|(export|import|declare|abstract|async|function|const|let|var' r'|class|interface|type|enum)\b') +# Tokens that can only continue the previous line's expression. Unary +# operators (`-`, `!`) are left out: a line may begin a new statement with one. +LEADING = frozenset(('|', '&', '.', '?.', '?', ':', '=>', '&&', '||', '??', '=', ',')) LOCATION = re.compile(r'^\| [^|]* \| `(?P[^`:]+):(?P[^`]+)` \| ' r'(?:`(?P[^`]+)`|_file scope_) \|') MISREAD = '⚠️ span may be misattributed' @@ -187,8 +190,12 @@ def declaration_lines(path: str, source: str) -> dict[int, bool]: # Column 0 is not top level in unformatted code, so count braces too. # Brackets of every kind decide whether a declaration closes on its line. declarations, depth, nesting, open_at_end = set(), 0, 0, {} + first, last = {}, {} # Each line's first and last code token. for offset, token, value in lexer.get_tokens_unprocessed(source): line = bisect.bisect_right(starts, offset) + if value.strip() and token not in Comment: + first.setdefault(line, (token, value)) + last[line] = (token, value) if (depth == 0 and starts[line - 1] == offset and (token in Keyword or token in Name.Decorator) and DECLARATION.match(source, offset)): @@ -197,7 +204,19 @@ def declaration_lines(path: str, source: str) -> dict[int, bool]: depth = max(0, depth + value.count('{') - value.count('}')) nesting = max(0, nesting + sum(map(value.count, '({[')) - sum(map(value.count, ')}]'))) open_at_end[line] = nesting > 0 - return {line: not open_at_end.get(line, False) for line in declarations} + # A balanced first line still continues when it ends on an operator + # (`type T =`, `=>`) or the next code line leads with one (`| 'b'`, `.next()`). + code_lines = sorted(first) + + def continues(line: int) -> bool: + following = code_lines[bisect.bisect_right(code_lines, line):] + token, value = last.get(line, (None, '')) + trails = (token in Operator and value not in ('>', '!', '++', '--') + or value in ('=>', ',', '.')) + return trails or bool(following) and first[following[0]][1] in LEADING + + return {line: not open_at_end.get(line, False) and not continues(line) + for line in declarations} def misread_functions(revisions: list[str], path: str) -> set[tuple[str, str]]: diff --git a/scripts/tests/test_impact_gate.py b/scripts/tests/test_impact_gate.py index ad5bd6845..ba5ef3594 100644 --- a/scripts/tests/test_impact_gate.py +++ b/scripts/tests/test_impact_gate.py @@ -190,6 +190,25 @@ def test_a_span_ending_on_a_one_line_declaration_swallows_it(self): self.assertEqual(driver.misread_functions([head], 'src/path.ts'), {('', 'path')}) self.assertEqual(driver.misread_functions([head], 'src/boundary.ts'), set()) + def test_a_declaration_continued_on_the_next_line_is_not_whole(self): + # A first line with no open bracket still continues after `=`, `=>` + # or before a leading `|`, `&` or `.`; only a finished one is whole. + cases = { + "export type Foo =\n | 'a'\n | 'b'\n": False, + 'export const LIMIT =\n 10\n': False, + 'export const twice = (x: number) =>\n x * 2\n': False, + 'export const chain = start\n .next()\n': False, + 'export const LIMIT = 10\n': True, + 'export const LIMIT = 10 // a comment\n\nexport const MORE = 11\n': True, + 'export const x = foo(\n 1,\n)\n': False, + 'export type Row = Record\nexport const y = 1\n': True, + 'export const value = input!\n\nexport const y = 1\n': True, + } + driver = self.load_driver() + for source, whole in cases.items(): + with self.subTest(source=source): + self.assertEqual(driver.declaration_lines('a.ts', source)[1], whole) + def test_marks_a_function_above_a_removed_declaration(self): # At the base, `pick`'s span swallows `gone`; the PR deletes `gone`. self.commit('src/pick.ts', PICK + '\nexport function gone(x: number): number {\n' From f740a0845c5527e199e7577847f6c9de5a8b3d6a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 16 Oct 2026 01:10:06 +0000 Subject: [PATCH 7/7] fix(ci): tell a closing `>` or `!` from one that continues the line 8914409a treated every trailing `>` and `!` as closing, so `export const large = value >` / ` 10` and `export const off = !` / ` enabled` read as whole. It also missed `>>`, which Pygments emits as one operator, so `Map>` read as continuing. A trailing run of `>` now closes only when the line has opened at least as many `<`, and `!`, `++` and `--` close only when written straight after their operand. Marks across the repo's TypeScript sources are unchanged. Reported by CodeRabbit on #1223. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01XsxT13tj7h1VEF8nNgHA1m --- scripts/impact-gate.py | 16 ++++++++++++---- scripts/tests/test_impact_gate.py | 4 ++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/scripts/impact-gate.py b/scripts/impact-gate.py index e0736e013..c25688bc1 100644 --- a/scripts/impact-gate.py +++ b/scripts/impact-gate.py @@ -195,7 +195,7 @@ def declaration_lines(path: str, source: str) -> dict[int, bool]: line = bisect.bisect_right(starts, offset) if value.strip() and token not in Comment: first.setdefault(line, (token, value)) - last[line] = (token, value) + last[line] = (token, value, offset) if (depth == 0 and starts[line - 1] == offset and (token in Keyword or token in Name.Decorator) and DECLARATION.match(source, offset)): @@ -208,12 +208,20 @@ def declaration_lines(path: str, source: str) -> dict[int, bool]: # (`type T =`, `=>`) or the next code line leads with one (`| 'b'`, `.next()`). code_lines = sorted(first) + def closes(line: int, value: str, offset: int) -> bool: + # An operator that ends its operand: the `>` of a generic the line + # opened, or a postfix `!`/`++`/`--` written against that operand. + text = source[starts[line - 1]:offset + len(value)] + if value.strip('>') == '': + return text.count('<') >= text.count('>') + return value in ('!', '++', '--') and re.match(r'[\w)\]]', source[offset - 1]) is not None + def continues(line: int) -> bool: following = code_lines[bisect.bisect_right(code_lines, line):] - token, value = last.get(line, (None, '')) - trails = (token in Operator and value not in ('>', '!', '++', '--') + token, value, offset = last.get(line, (None, '', 0)) + trails = ((token in Operator and not closes(line, value, offset)) or value in ('=>', ',', '.')) - return trails or bool(following) and first[following[0]][1] in LEADING + return trails or (bool(following) and first[following[0]][1] in LEADING) return {line: not open_at_end.get(line, False) and not continues(line) for line in declarations} diff --git a/scripts/tests/test_impact_gate.py b/scripts/tests/test_impact_gate.py index ba5ef3594..15c501051 100644 --- a/scripts/tests/test_impact_gate.py +++ b/scripts/tests/test_impact_gate.py @@ -203,6 +203,10 @@ def test_a_declaration_continued_on_the_next_line_is_not_whole(self): 'export const x = foo(\n 1,\n)\n': False, 'export type Row = Record\nexport const y = 1\n': True, 'export const value = input!\n\nexport const y = 1\n': True, + 'export const nested = new Map>()\nexport const y = 1\n': True, + 'export type Nested = Map>\nexport const y = 1\n': True, + 'export const large = value >\n 10\n': False, + 'export const off = !\n enabled\n': False, } driver = self.load_driver() for source, whole in cases.items():