Skip to content

Commit 2a70a47

Browse files
authored
Merge pull request #1223 from cipherstash/fix/impact-gate-pr-base
fix(ci): score a PR from the commit it merges onto, and label misread function spans
2 parents abf9de5 + f740a08 commit 2a70a47

6 files changed

Lines changed: 383 additions & 19 deletions

File tree

‎.github/impact-gate/requirements.txt‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,12 @@
22
# CI installs it with `pip install --require-hashes --no-deps`, so nothing here
33
# resolves at install time and a transitive dependency cannot float.
44
#
5-
# The top-level pins are impact-gate==0.4.1 and lizard==1.23.0 (Lizard 1.24.0 is
6-
# excluded by ImpactGate's own metadata). To regenerate after changing them, from
7-
# the repository root:
5+
# The top-level pins are impact-gate==0.4.1, lizard==1.23.0 (Lizard 1.24.0 is
6+
# excluded by ImpactGate's own metadata) and pygments==2.21.0, which
7+
# scripts/impact-gate.py imports directly. uv still annotates pygments "via
8+
# lizard". To regenerate after changing them, from the repository root:
89
#
9-
# printf 'impact-gate==0.4.1\nlizard==1.23.0\n' \
10+
# printf 'impact-gate==0.4.1\nlizard==1.23.0\npygments==2.21.0\n' \
1011
# | uv pip compile - --generate-hashes --universal --python-version 3.12 \
1112
# --no-header -o .github/impact-gate/requirements.txt
1213
#

‎.github/workflows/impact.yml‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,11 +45,16 @@ jobs:
4545
id: target
4646
env:
4747
TARGET_BRANCH: ${{ github.base_ref || github.ref_name }}
48-
BASE_SHA: ${{ github.event.pull_request.base.sha || github.sha }}
48+
EVENT_SHA: ${{ github.sha }}
49+
PR_HEAD: ${{ github.event.pull_request.head.sha }}
4950
run: |
5051
# Keep one logical target ref across push/PR events, pinned to the
51-
# event's commit even if the remote branch advances while queued.
52-
git update-ref "$BASE_REF" "$BASE_SHA"
52+
# commit this run measures from even if the remote branch advances
53+
# while queued. A PR pins its merge commit's first parent, not
54+
# pull_request.base.sha, which lags main (see `pin` in the driver).
55+
args=(--event-sha "$EVENT_SHA")
56+
if [[ -n "$PR_HEAD" ]]; then args+=(--pr-head "$PR_HEAD"); fi
57+
python scripts/impact-gate.py pin --base "$BASE_REF" "${args[@]}"
5358
echo "BASELINE_DIR=$RUNNER_TEMP/impact-baseline" >> "$GITHUB_ENV"
5459
python - <<'PY'
5560
import hashlib
@@ -70,9 +75,9 @@ jobs:
7075
uses: actions/cache/restore@0057852bfaa89a56745cba8c7296529d2fc39830 # v4
7176
with:
7277
path: ${{ env.BASELINE_DIR }}
73-
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 }}
78+
key: impact-${{ hashFiles('.github/impact-gate/**', 'scripts/impact-gate.py', '.github/workflows/impact.yml') }}-${{ steps.target.outputs.key }}-${{ steps.target.outputs.sha }}
7479
restore-keys: |
75-
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 }}-
80+
impact-${{ hashFiles('.github/impact-gate/**', 'scripts/impact-gate.py', '.github/workflows/impact.yml') }}-${{ steps.target.outputs.key }}-${{ steps.target.outputs.sha }}-
7681
impact-${{ hashFiles('.github/impact-gate/**', 'scripts/impact-gate.py', '.github/workflows/impact.yml') }}-${{ steps.target.outputs.key }}-
7782
7883
- name: Validate or build baselines

‎docs/plans/2026-10-08-impact-gate-rollout.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ Manual comparison of representative Stack source at the base SHA also found a re
2626

2727
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.
2828

29-
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.
29+
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.
3030

3131
## Coverage and interpretation
3232

‎scripts/__tests__/impact-workflow.test.mjs‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -90,16 +90,16 @@ describe('advisory change impact', () => {
9090
expect(restore.with.key).toContain("'scripts/impact-gate.py'")
9191
expect(restore.with.key).toContain("'.github/workflows/impact.yml'")
9292
expect(restore.with.key).toContain('steps.target.outputs.key')
93-
expect(restore.with.key).toContain(
94-
'github.event.pull_request.base.sha || github.sha',
95-
)
93+
// The commit `pin` measured from, never pull_request.base.sha: that lags
94+
// the target whenever it moved on after the PR last synced.
95+
expect(restore.with.key).toContain('steps.target.outputs.sha')
9696
// This commit's run-suffixed saves first, then the target branch's
9797
// ancestors — never a broader prefix.
9898
expect(restore.with['restore-keys'].trim().split('\n')).toEqual([
9999
`${restore.with.key}-`,
100100
restore.with.key.replace(
101101
// biome-ignore lint/suspicious/noTemplateCurlyInString: literal Actions expression
102-
'-${{ github.event.pull_request.base.sha || github.sha }}',
102+
'-${{ steps.target.outputs.sha }}',
103103
'-',
104104
),
105105
])
@@ -125,6 +125,25 @@ describe('advisory change impact', () => {
125125
expect(save.with.path).toBe(restore.with.path)
126126
})
127127

128+
it('measures a pull request from the commit its merge was built on', () => {
129+
// pull_request.base.sha lags main whenever main moved on after the PR last
130+
// synced, while the checkout is refs/pull/N/merge on main's current tip;
131+
// scoring from the stale base counted main's newer commits as the PR's.
132+
// `pin` takes the merge commit's first parent instead (test_impact_gate.py).
133+
const job = readWorkflow(workflowPath).jobs.impact
134+
const target = job.steps.find((step) => step.id === 'target')
135+
expect(target.run).toContain(
136+
'scripts/impact-gate.py pin --base "$BASE_REF"',
137+
)
138+
// biome-ignore lint/suspicious/noTemplateCurlyInString: literal Actions expression
139+
expect(target.env.EVENT_SHA).toBe('${{ github.sha }}')
140+
// biome-ignore lint/suspicious/noTemplateCurlyInString: literal Actions expression
141+
expect(target.env.PR_HEAD).toBe('${{ github.event.pull_request.head.sha }}')
142+
expect(readFileSync(join(REPO_ROOT, workflowPath), 'utf8')).not.toContain(
143+
'pull_request.base.sha }}',
144+
)
145+
})
146+
128147
it('replaces a corrupt same-commit cache instead of colliding with its key', () => {
129148
// A cache key is immutable. When the entry saved for a commit fails
130149
// validation, prepare rebuilds; saving under that same key is refused
@@ -142,7 +161,7 @@ describe('advisory change impact', () => {
142161
const run = (event, runId, attempt, restored) => {
143162
const context = {
144163
github: { ...event.github, run_id: runId, run_attempt: attempt },
145-
steps: { target: { outputs: { key: 'main' } } },
164+
steps: { target: { outputs: { key: 'main', sha: event.github.sha } } },
146165
}
147166
const primary = interpolate(restore.with.key, context)
148167
context.steps.restore = {

‎scripts/impact-gate.py‎

Lines changed: 159 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,17 +5,22 @@
55
so tests can exercise the same workflow against isolated Git histories.
66
"""
77
import argparse
8+
import bisect
89
import hashlib
910
import importlib.metadata
1011
import json
1112
import os
1213
from pathlib import Path
14+
import re
1315
import subprocess
1416
import sys
1517
import tempfile
1618

1719
from impact_gate.baseline import build_baseline, save_baseline
1820
from impact_gate.core.config import MeasureConfig
21+
import lizard
22+
from pygments.lexers import JavascriptLexer, TypeScriptLexer
23+
from pygments.token import Comment, Keyword, Name, Operator, Punctuation
1924

2025
ROOT = Path(__file__).resolve().parents[1]
2126
POLICY = ROOT / '.github' / 'impact-gate'
@@ -138,6 +143,7 @@ def report(args: argparse.Namespace, policy: str) -> None:
138143
f'Baseline target: `{args.base}`; commit: `{meta["head"]}`. '
139144
f'History: latest {HISTORY_COMMITS} first-parent commits; recursively expanded '
140145
'merge observations may exceed this limit. p90/p98 are advisory; prior weight: 200.')
146+
spans: dict = {} # Both scopes list the same functions.
141147
for scope, label in SCOPES.items():
142148
path = args.cache_dir / f'{scope}.json'
143149
n = read_baseline(path, meta['head'])
@@ -148,18 +154,167 @@ def report(args: argparse.Namespace, policy: str) -> None:
148154
'--enforcement', 'warn', '--format', 'markdown', '--warn-percentile', '90',
149155
'--block-percentile', '98', '--curve-prior-weight', '200',
150156
'--baseline-file', str(path), '--measure-config', str(POLICY / f'{scope}.yml'))
151-
publish(result)
157+
publish(mark_misread_spans(result, args.base, spans))
158+
159+
160+
# lizard's TypeScript reader sometimes misses a function's closing brace (an
161+
# arrow callback followed by a ternary returning an object literal is one
162+
# trigger) and runs the span on into the next top-level declaration. Edits to
163+
# that neighbour are then charged to a function the PR never touched. In
164+
# formatted code nothing inside a function body starts at column 0, so a
165+
# top-level declaration strictly inside a span is the tell. Pygments decides
166+
# what is code: a column-0 `const` inside a template literal is string text.
167+
# Limit: when the misread makes lizard drop functions instead (the same
168+
# trigger inside a class method reports only an `(anonymous)` function), the
169+
# edits land on a row that has no span to check, and nothing is marked.
170+
SPAN_SUFFIXES = ('.ts', '.tsx', '.js', '.jsx')
171+
DECLARATION = re.compile(r'@|(export|import|declare|abstract|async|function|const|let|var'
172+
r'|class|interface|type|enum)\b')
173+
# Tokens that can only continue the previous line's expression. Unary
174+
# operators (`-`, `!`) are left out: a line may begin a new statement with one.
175+
LEADING = frozenset(('|', '&', '.', '?.', '?', ':', '=>', '&&', '||', '??', '=', ','))
176+
LOCATION = re.compile(r'^\| [^|]* \| `(?P<path>[^`:]+):(?P<name>[^`]+)` \| '
177+
r'(?:`(?P<owner>[^`]+)`|_file scope_) \|')
178+
MISREAD = '⚠️ span may be misattributed'
179+
MISREAD_NOTE = (f'{MISREAD}: lizard read its line span to include a later top-level '
180+
'declaration, so edits to that code are charged to this function. Check '
181+
'the diff before refactoring it. Scores and grades are unchanged.')
182+
183+
184+
def declaration_lines(path: str, source: str) -> dict[int, bool]:
185+
"""Line of each top-level declaration, mapped to whether it ends on that line."""
186+
lexer = TypeScriptLexer() if path.endswith(('.ts', '.tsx')) else JavascriptLexer()
187+
starts = [0]
188+
for line in source.split('\n')[:-1]:
189+
starts.append(starts[-1] + len(line) + 1)
190+
# Column 0 is not top level in unformatted code, so count braces too.
191+
# Brackets of every kind decide whether a declaration closes on its line.
192+
declarations, depth, nesting, open_at_end = set(), 0, 0, {}
193+
first, last = {}, {} # Each line's first and last code token.
194+
for offset, token, value in lexer.get_tokens_unprocessed(source):
195+
line = bisect.bisect_right(starts, offset)
196+
if value.strip() and token not in Comment:
197+
first.setdefault(line, (token, value))
198+
last[line] = (token, value, offset)
199+
if (depth == 0 and starts[line - 1] == offset
200+
and (token in Keyword or token in Name.Decorator)
201+
and DECLARATION.match(source, offset)):
202+
declarations.add(line)
203+
elif token in Punctuation:
204+
depth = max(0, depth + value.count('{') - value.count('}'))
205+
nesting = max(0, nesting + sum(map(value.count, '({[')) - sum(map(value.count, ')}]')))
206+
open_at_end[line] = nesting > 0
207+
# A balanced first line still continues when it ends on an operator
208+
# (`type T =`, `=>`) or the next code line leads with one (`| 'b'`, `.next()`).
209+
code_lines = sorted(first)
210+
211+
def closes(line: int, value: str, offset: int) -> bool:
212+
# An operator that ends its operand: the `>` of a generic the line
213+
# opened, or a postfix `!`/`++`/`--` written against that operand.
214+
text = source[starts[line - 1]:offset + len(value)]
215+
if value.strip('>') == '':
216+
return text.count('<') >= text.count('>')
217+
return value in ('!', '++', '--') and re.match(r'[\w)\]]', source[offset - 1]) is not None
218+
219+
def continues(line: int) -> bool:
220+
following = code_lines[bisect.bisect_right(code_lines, line):]
221+
token, value, offset = last.get(line, (None, '', 0))
222+
trails = ((token in Operator and not closes(line, value, offset))
223+
or value in ('=>', ',', '.'))
224+
return trails or (bool(following) and first[following[0]][1] in LEADING)
225+
226+
return {line: not open_at_end.get(line, False) and not continues(line)
227+
for line in declarations}
228+
229+
230+
def misread_functions(revisions: list[str], path: str) -> set[tuple[str, str]]:
231+
"""(owner, name) of each function whose span, in any revision, swallows a declaration."""
232+
found = set()
233+
for revision in revisions:
234+
shown = subprocess.run(['git', 'show', f'{revision}:{path}'], capture_output=True)
235+
if shown.returncode != 0: # Added or deleted on one side.
236+
continue
237+
source = shown.stdout.decode('utf-8', errors='replace')
238+
declarations = declaration_lines(path, source)
239+
for function in lizard.analyze_file.analyze_source_code(path, source).function_list:
240+
owner, _, name = function.name.rpartition('::')
241+
# lizard often ends a span on the next declaration's first line.
242+
# That boundary is harmless unless the whole declaration is there.
243+
if name != '(anonymous)' and any(
244+
function.start_line < n < function.end_line
245+
or (n == function.end_line and whole)
246+
for n, whole in declarations.items()):
247+
found.add((owner, name))
248+
return found
249+
250+
251+
def mark_misread_spans(markdown: str, base: str, cache: dict | None = None) -> str:
252+
"""Label function rows whose span lizard misread. Never fails the report."""
253+
cache = {} if cache is None else cache
254+
try:
255+
if 'revisions' not in cache:
256+
cache['revisions'] = [run('git', 'merge-base', base, 'HEAD'), 'HEAD']
257+
marked = False
258+
lines = markdown.split('\n')
259+
for index, line in enumerate(lines):
260+
match = LOCATION.match(line)
261+
if not match or not match['path'].endswith(SPAN_SUFFIXES):
262+
continue
263+
path = match['path']
264+
if path not in cache:
265+
cache[path] = misread_functions(cache['revisions'], path)
266+
if (match['owner'] or '', match['name']) in cache[path]:
267+
cell = f'`{path}:{match["name"]}`'
268+
lines[index] = line.replace(cell, f'{cell} {MISREAD}', 1)
269+
marked = True
270+
text = '\n'.join(lines)
271+
return text + f'\n\n{MISREAD_NOTE}' if marked else text
272+
except Exception as error: # A cosmetic label must not cost the report.
273+
return markdown + f'\n\n_Note: could not check function spans ({describe(error)})._'
274+
275+
276+
def pin(args: argparse.Namespace) -> None:
277+
"""Point the target ref at the commit this run's change is measured from.
278+
279+
A push measures from its own commit. A pull request checks out
280+
refs/pull/N/merge, GitHub's merge of the PR into the target branch's tip
281+
when the event fired (a re-run reuses it). The event's pull_request.base.sha
282+
is older whenever the target moved on after the PR last synced, and scoring
283+
from it counts the target's newer commits as the PR's. The merge's first
284+
parent is the tip it was built on, so HEAD^1..HEAD is exactly what the PR
285+
adds.
286+
"""
287+
base = args.event_sha
288+
if args.pr_head:
289+
parents = run('git', 'rev-list', '--parents', '-n', '1', 'HEAD').split()[1:]
290+
if len(parents) != 2 or parents[1] != args.pr_head:
291+
raise ValueError(f'HEAD is not the merge of {args.pr_head} into the target; '
292+
'check out refs/pull/N/merge.')
293+
base = parents[0]
294+
run('git', 'update-ref', args.base, base)
295+
output('sha', base)
152296

153297

154298
def main() -> int:
155299
parser = argparse.ArgumentParser(description=__doc__)
156-
parser.add_argument('command', choices=('prepare', 'report'))
300+
parser.add_argument('command', choices=('pin', 'prepare', 'report'))
157301
parser.add_argument('--base', required=True)
158-
parser.add_argument('--cache-dir', type=Path, required=True)
302+
parser.add_argument('--cache-dir', type=Path)
159303
parser.add_argument('--refresh', action='store_true')
304+
parser.add_argument('--event-sha', help='pin: the commit the event was raised for')
305+
parser.add_argument('--pr-head', help='pin: the pull request head, on pull_request events')
160306
args = parser.parse_args()
161-
args.cache_dir = args.cache_dir.resolve()
307+
if args.command == 'pin':
308+
if not args.event_sha:
309+
parser.error('pin requires --event-sha')
310+
elif args.cache_dir is None:
311+
parser.error(f'{args.command} requires --cache-dir')
312+
else:
313+
args.cache_dir = args.cache_dir.resolve()
162314
try:
315+
if args.command == 'pin':
316+
pin(args)
317+
return 0
163318
head = run('git', 'rev-parse', '--verify', args.base + '^{commit}')
164319
policy = identity()
165320
if args.command == 'prepare':

0 commit comments

Comments
 (0)