fix: review follow-ups for #5 and the Swift lexical preview #8

Merged

trevon merged 9 commits from fix/review-followups into main 2026-09-25 13:33:41 +00:00

Conversation 0 Commits 9 Files changed 11 +801 -55

trevon commented 2026-09-25 03:13:37 +00:00

Owner

Copy link

Summary

This PR fixes the code-review findings on the merged #5 audit fixes, and the Swift lexical-preview bugs. There is one commit per fix, and each commit body gives the problem, the fix and the test. Every new test was run first and failed for the reason its finding describes.

Commits

• fix(fold): decode SARIF escapes safely, keep the lane per run, and follow symlinked absolute paths (findings 1, 6 and 7)
  • Panic. A SARIF URI with a % escape that was not two hex digits (such as file:///src/%aé.c) panicked by slicing through a UTF-8 character. Decoding now works on bytes, and a % escape is decoded only when two hex digits follow it.
  • Lane per run. Whether a finding stays in Fold's Symbolic lane was decided once for the whole report. It is now decided for each SARIF run from that run's tool.driver.name. In a mixed log, only the Fold runs are Symbolic.
  • .. after a symlink. Absolute and file:// paths containing .. were normalized lexically. Now, when such a path exists, it is canonicalized and the canonical root is stripped, the same way repository-relative paths are handled.
  • Tests: three unit tests in src/fold.rs.
• refactor: one definition of "the model scanned a source file" (finding 9)
  • model::has_source_files and Building::has_source_files replace the two copies of this check in scan and check.
• fix(check): warn instead of failing when stored metrics differ from recomputed (finding 4)
  • A model written by an older build has no metrics.resolution block, and check rejected it.
  • Metrics are now compared only on the keys the stored model has. A real mismatch prints a warning, and the gate runs on the recomputed metrics.
  • Test: a_model_from_an_older_build_passes_check_on_recomputed_metrics.
• fix(swift): give the lexical preview Swift's own forms and mask block comments (part B)
  • The Swift preview used generic patterns, with four effects:
    • calls inside /* */ comments became edges;
    • func, class func and class var declarations were counted as calls;
    • calls were attributed to the file instead of the enclosing function;
    • the real call was missed.
  • Swift now has its own class, func and import patterns, and class func and class var are treated as members.
  • Block comments are masked in a way that handles nesting and strings, carrying state across lines. This masking is salvaged from 543bafc and 2c66bec.
  • Test: tests/swift_preview.rs, with a positive fixture and a clean-negative fixture.
• fix(python): resolve relative, package and dotted imports to the right file (finding 2)
  • from .util import x, from .. import, pkg/__init__.py and src/ layouts now resolve by path.
  • The unique-file-stem fallback now applies only to single-component names, so two common.py files no longer shadow each other.
  • Tests: tests/python_imports.rs, 2 tests.
• perf(scan): index import edges once instead of scanning them for every file (finding 8)
  • imported_paths was O(files × edges). It now builds a file → imports index lazily, once.
• fix(rust): keep impls written on a qualified path when leaf names collide (finding 3)
  • For impl crate::a::T { fn run() }, the qualified path was dropped. When another file also had a T, the methods were lost.
  • The impl's full type path is now recorded and resolved through the module path.
  • Test: an_impl_on_a_qualified_path_in_another_file_keeps_its_methods.
• fix(dart): honour import prefixes, show/hide, and one level of export (finding 5)
  • A bare call resolved against every imported library. import 'a.dart' as p; still bound foo(), and hide and show were ignored.
  • Now a bare name binds only through imports with no as prefix whose combinators let the name through, plus those libraries' own exports, one level deep. The README is updated to match.
  • Test: tests/dart_imports.rs, which covers prefixed, hide, show and barrel imports.

Deferred

• Finding 10: C headers are parsed as C++.
  • .h still uses the C++ grammar. Choosing C for a header needs the include graph: the same header can be included from both .c and .cpp translation units.
  • Changing the grammar for a header also changes qualified names and cross-TU prototype pairing, which the C++ corpus depends on. That is not a trivial change.
  • The right fix is to pick the grammar from the languages of the header's includers, or from compile_commands.json. Both are better done as their own PR, with a corpus case.

Tests

• cargo test: 126 passed, 0 failed.
• cargo clippy --all-targets -- -D warnings: clean.
• ./scripts/quality-gate.sh: passed. It covers fmt, clippy, tests with and without the P4 pilot, the Python unit tests, and the benchmark self-tests and runs.

Before / after

I re-ran the review's reproduced cases. "Before" is the #5 merge build; "after" is this branch. The calls listed are resolved call edges.

Case	Before	After
SARIF file:///src/%aé.c	panic at src/fold.rs:298, exit 101	exit 0
Python: from .util import helper, and duplicate common.py	no edges	go -> helper, use -> only_here (the right file)
Rust: impl on a qualified path, leaf name collides	method lost	f -> run
check --min-score 0 on an older model	exit 1: "stored metrics do not match"	exit 0
Swift probe	a.swift -> realTarget @ a.swift:1 (from a comment, attributed to the file)	caller -> realTarget @ a.swift:7
Dart: as p, hide hidden, barrel export	foo, hidden, visible	visible, exported

Language-parity branch

The remaining lexical work on the language-parity branch is superseded. Python and Swift should move to Tree-sitter frontends rather than get more lexical patterns, and the useful pieces of that branch (the Swift comment masking) are salvaged here. That branch can be closed.

AI usage

Disclosure: the code, tests and this description were written by Claude (Claude Code) at the maintainer's direction.

## Summary This PR fixes the code-review findings on the merged #5 audit fixes, and the Swift lexical-preview bugs. There is one commit per fix, and each commit body gives the problem, the fix and the test. Every new test was run first and failed for the reason its finding describes. ## Commits - **`fix(fold)`: decode SARIF escapes safely, keep the lane per run, and follow symlinked absolute paths** (findings 1, 6 and 7) - **Panic.** A SARIF URI with a `%` escape that was not two hex digits (such as `file:///src/%aé.c`) panicked by slicing through a UTF-8 character. Decoding now works on bytes, and a `%` escape is decoded only when two hex digits follow it. - **Lane per run.** Whether a finding stays in Fold's Symbolic lane was decided once for the whole report. It is now decided for each SARIF run from that run's `tool.driver.name`. In a mixed log, only the Fold runs are Symbolic. - **`..` after a symlink.** Absolute and `file://` paths containing `..` were normalized lexically. Now, when such a path exists, it is canonicalized and the canonical root is stripped, the same way repository-relative paths are handled. - Tests: three unit tests in `src/fold.rs`. - **`refactor`: one definition of "the model scanned a source file"** (finding 9) - `model::has_source_files` and `Building::has_source_files` replace the two copies of this check in `scan` and `check`. - **`fix(check)`: warn instead of failing when stored metrics differ from recomputed** (finding 4) - A model written by an older build has no `metrics.resolution` block, and `check` rejected it. - Metrics are now compared only on the keys the stored model has. A real mismatch prints a warning, and the gate runs on the recomputed metrics. - Test: `a_model_from_an_older_build_passes_check_on_recomputed_metrics`. - **`fix(swift)`: give the lexical preview Swift's own forms and mask block comments** (part B) - The Swift preview used generic patterns, with four effects: - calls inside `/* */` comments became edges; - `func`, `class func` and `class var` declarations were counted as calls; - calls were attributed to the file instead of the enclosing function; - the real call was missed. - Swift now has its own class, func and import patterns, and `class func` and `class var` are treated as members. - Block comments are masked in a way that handles nesting and strings, carrying state across lines. This masking is salvaged from 543bafc and 2c66bec. - Test: `tests/swift_preview.rs`, with a positive fixture and a clean-negative fixture. - **`fix(python)`: resolve relative, package and dotted imports to the right file** (finding 2) - `from .util import x`, `from .. import`, `pkg/__init__.py` and `src/` layouts now resolve by path. - The unique-file-stem fallback now applies only to single-component names, so two `common.py` files no longer shadow each other. - Tests: `tests/python_imports.rs`, 2 tests. - **`perf(scan)`: index import edges once instead of scanning them for every file** (finding 8) - `imported_paths` was O(files × edges). It now builds a file → imports index lazily, once. - **`fix(rust)`: keep impls written on a qualified path when leaf names collide** (finding 3) - For `impl crate::a::T { fn run() }`, the qualified path was dropped. When another file also had a `T`, the methods were lost. - The impl's full type path is now recorded and resolved through the module path. - Test: `an_impl_on_a_qualified_path_in_another_file_keeps_its_methods`. - **`fix(dart)`: honour import prefixes, `show`/`hide`, and one level of `export`** (finding 5) - A bare call resolved against every imported library. `import 'a.dart' as p;` still bound `foo()`, and `hide` and `show` were ignored. - Now a bare name binds only through imports with no `as` prefix whose combinators let the name through, plus those libraries' own `export`s, one level deep. The README is updated to match. - Test: `tests/dart_imports.rs`, which covers prefixed, `hide`, `show` and barrel imports. ## Deferred - **Finding 10: C headers are parsed as C++.** - `.h` still uses the C++ grammar. Choosing C for a header needs the include graph: the same header can be included from both `.c` and `.cpp` translation units. - Changing the grammar for a header also changes qualified names and cross-TU prototype pairing, which the C++ corpus depends on. That is not a trivial change. - The right fix is to pick the grammar from the languages of the header's includers, or from `compile_commands.json`. Both are better done as their own PR, with a corpus case. ## Tests - `cargo test`: 126 passed, 0 failed. - `cargo clippy --all-targets -- -D warnings`: clean. - `./scripts/quality-gate.sh`: passed. It covers fmt, clippy, tests with and without the P4 pilot, the Python unit tests, and the benchmark self-tests and runs. ## Before / after I re-ran the review's reproduced cases. "Before" is the #5 merge build; "after" is this branch. The calls listed are resolved call edges. | Case | Before | After | | --- | --- | --- | | SARIF `file:///src/%aé.c` | panic at `src/fold.rs:298`, exit 101 | exit 0 | | Python: `from .util import helper`, and duplicate `common.py` | no edges | `go -> helper`, `use -> only_here` (the right file) | | Rust: impl on a qualified path, leaf name collides | method lost | `f -> run` | | `check --min-score 0` on an older model | exit 1: "stored metrics do not match" | exit 0 | | Swift probe | `a.swift -> realTarget @ a.swift:1` (from a comment, attributed to the file) | `caller -> realTarget @ a.swift:7` | | Dart: `as p`, `hide hidden`, barrel `export` | `foo`, `hidden`, `visible` | `visible`, `exported` | ## Language-parity branch The remaining lexical work on the language-parity branch is superseded. Python and Swift should move to Tree-sitter frontends rather than get more lexical patterns, and the useful pieces of that branch (the Swift comment masking) are salvaged here. That branch can be closed. ## AI usage > Disclosure: the code, tests and this description were written by Claude (Claude Code) at the maintainer's direction.

trevon added 8 commits 2026-09-25 03:13:37 +00:00

fix(fold): decode SARIF escapes safely, lane per run, follow absolute symlinked paths ... 3a0f2adf59

Three import fixes from review of #5:

- sarif_uri_path sliced `&rest[i + 1..i + 3]` by byte index, so a `%`
  followed by a multibyte character (`file:///src/%aé.c`) panicked the
  whole build, and from_str_radix accepted `%+1` as byte 0x01. Escapes
  are now decoded on bytes and only from two hex digits.
- Whether findings keep Fold's Symbolic lane was decided for the whole
  report with all(): a merged SARIF log with one Fold run and one CodeQL
  run demoted every Fold finding to Imported. The lane is now decided
  per SARIF run (per report for native findings), and the diagnostic
  names only the non-Fold producers.
- The rule that `..` after a symlinked directory follows the filesystem
  applied only to relative paths; `/root/linkout/../src/main.rs` and its
  file:// form still normalized lexically onto an in-repository file.
  Absolute paths that exist now resolve through the filesystem too.

Tests (each failed before this change): percent-escape decoding without
panics, per-run lanes in a merged SARIF, and absolute/file:// paths
through a symlinked directory.

refactor: one definition of "the model scanned a source file" ... 5a6f8b143a

The empty-model condition was written three times: a scan.rs
diagnostic, a helper in main.rs for `build`'s warning, and the same
helper for `check`'s failure. They could drift. model.rs now owns
`has_source_files` (and `Building::has_source_files`), used by all
three. No behaviour change; the empty-scan CLI test still passes.

fix(check): warn, don't fail, when stored metrics differ from recomputed ... d0d52edfd1

`check` treated any difference between stored and recomputed metrics as
a gate failure, so a model written by an older brick (without the
`metrics.resolution` field, or with an older formula) always failed,
even with every threshold met. CI checking an archived or cached model
broke on upgrade.

The gate already uses recomputed metrics, so a mismatch is now a
stderr warning. Only fields present in both copies are compared, so a
missing newer field does not count as a mismatch. Tampered metrics
still cannot pass: the existing test's recomputed cycle count fails the
gate, and it still reports "stored metrics".

Test: a freshly built model with `metrics.resolution` removed passes
`check --min-score 0`; it failed before this change.

fix(swift): give the lexical preview Swift's forms and mask block comments ... c3c7c6c3f4

Swift ran through the C-style lexical patterns, which never recognize
`func`. On a small file (`func realTarget()`, `func ghost()`, a class
method `caller` that calls `realTarget()` below a `/* ghost() */`
comment) the preview reported:

  a.swift -> ghost       (a.swift:2 declaration, a.swift:5 in a comment)
  a.swift -> caller      (its declaration line)
  a.swift -> realTarget  (its declaration line)

and missed the real call. Now:

  caller -> realTarget   (a.swift:7)

- Swift gets its own patterns: `func` declarations (with attributes and
  modifiers), class/struct/enum/protocol/actor/extension declarations
  (`class func` / `class var` are members, not classes), and imports.
- Block comments are masked across lines, nested as Swift allows, and a
  `/*` inside a string literal does not open one. The masker is salvaged
  from the unmerged language-parity branch (543bafc).
- Calls are attributed to their enclosing function through the existing
  brace-scoped scopes, now that functions are recognized.

Test: tests/swift_preview.rs (functions and a class recognized, the call
attributed to `caller`, nothing from comments, nothing from the file
node); it failed before this change.

fix(python): resolve relative, package and dotted imports to the right file ... 67334bec59

Since #5 Python calls resolve only through import edges, but the
lexical importer could not produce those edges for the common forms:

- `from .util import helper` never matched (the regex needed a leading
  letter), so the call went unresolved;
- `from pkg import f` looked up the stem "pkg" instead of
  pkg/__init__.py;
- `from x.common import f` failed when y/common.py also existed, and
  `from other.common import f` (a module the repository does not have)
  resolved to y/common.py by basename.

Imports now resolve Python-specifically: leading dots are relative to
the importing package, and a dotted name maps to `a/b.py` or
`a/b/__init__.py` from the repository root or `src/`. Only a
single-component name (`import foo`) may still fall back to a unique
module basename; a dotted name never does. `from . import x` stays
unresolved (the module is in the import list, not the name).

Tests (both failed before): relative, package and duplicate-stem imports
resolve their calls to pkg/util.py, pkg/__init__.py and x/common.py;
an unknown dotted module does not bind to y/common.py.

perf(scan): index import edges once instead of scanning per file ... 3fbb9275a5

imported_paths found the file node with a linear scan of all nodes and
then filtered all edges, for every Python/Dart file it built a set for:
O(files x (nodes + edges)) during resolution, about 10^10 comparisons
for 10k files and a million edges.

Imports edges are now indexed by source file on first use and the file
node is looked up by its stable id, as resolve_import already does. The
index is complete when first built: resolve_relationships resolves
every import before any call. Output is unchanged (all 124 tests pass).

fix(rust): keep impls written on a qualified path when leaf names collide ... 04653788fa

When several files declare the same type leaf name, rust_method keeps
only the impls of the type a call site means, asking each impl's file
which `Foo` it means. An impl in a third file written as
`impl crate:🅰️:Foo` resolved nothing (that file has no `Foo`
declaration or `use` binding), so its methods were dropped and
`x.run()` on an `a::Foo` read "member-not-found". It resolved before #5.

The extractor now records the impl's type path when it is written as
one (`impl_type_path`, generic arguments removed), the method index
keeps it, and the impl filter resolves that path to its declaring file
before falling back to the file's own view of the leaf name. Glob
imports (`use super::*`) are still not expanded; such an impl stays
unresolved rather than guessed.

Test: with `Foo` declared in a.rs and c.rs and `impl crate:🅰️:Foo` in
ext.rs, `x.run()` on `crate:🅰️:Foo` resolves to ext.rs's `run`; it
failed before this change.

fix(dart): honour import prefixes, show/hide and one level of export ... 324900287b

Problem: a bare call in a Dart file resolved against every imported
library. An `import 'a.dart' as p;` still bound `foo()` to a.dart's foo,
`hide` and `show` combinators were ignored, and a name re-exported through
a barrel library (`export 'impl.dart';`) never resolved.

Fix: record each Dart import/export directive with its `as` prefix and
show/hide lists. A bare name resolves only in libraries imported without a
prefix whose combinators let it through, plus that library's own exports
that also let it through (one level).

Test: tests/dart_imports.rs covers a prefixed, a hiding, a showing and a
barrelled import; it failed on the prefixed case before this change.

trevon added 1 commit 2026-09-25 03:14:32 +00:00

Merge main into fix/review-followups: move the Dart import note into docs/resolution.md 67d1980001

trevon merged commit 27d53a1b03 into main 2026-09-25 13:33:41 +00:00

trevon referenced this pull request from a commit 2026-09-25 13:33:42 +00:00

Merge pull request 'fix: review follow-ups for #5 and the Swift lexical preview' (#8) from fix/review-followups into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

trevon/brick!8

No description provided.