fix/audit-findings #5

Merged
trevon merged 14 commits from fix/audit-findings into main 2026-09-25 02:39:30 +00:00
Owner
No description provided.
The Rust impl index keyed types by leaf name, so `a::Config` and
`b::Config` shared one `(Config, load)` entry and the inherent method
won: `use crate:🅱️:Config; c.load()` produced a `declared-receiver-type`
edge to `a::Config::load`.

The index now records which files declare each type name and the file
of every impl. When several files declare a name, a call resolves only
among impls of the type the calling file means (its own declaration,
then a `use` binding, then the only declaration); with no such binding
the call is left external as `ambiguous` instead of guessed. Unique
names behave as before.

Test: rust_methods::same_named_types_in_different_modules_resolve_through_use_bindings
fails before this change (edge to src/a.rs).
With `using namespace a; using namespace b;` and both `a::run` and
`b::run` declared, a call to `run` resolved to whichever directive came
first in file order and was labelled `using-directive`. In C++ that
call is ambiguous (or an overload choice Brick does not model).

Directives are now grouped: each `using N::x;` and `using namespace N;`
nominates at most one declaration (its innermost resolving scope), and
the call resolves only when exactly one distinct declaration is
nominated; otherwise it stays external as `ambiguous`.

Test: cpp_xtu::two_using_directives_naming_the_same_function_are_ambiguous
fails before this change; its single-directive case still resolves.
Every preprocessor branch was indexed as live. In yams,
InternalEventBus.h guards spdlog with
`#if __has_include(<spdlog/spdlog.h>) ... #else namespace spdlog { stubs }`,
so 1,243 spdlog::warn/info/error call sites resolved to the fallback
stubs, which became the 4th and 5th highest fan-in nodes.

Brick does not evaluate conditions, so only the first branch is treated
as live: declarations and prototypes inside `#elif`/`#else` stay in the
graph (marked `source.preprocessor-branch: alternative`) but are not
definitions, declarations or classes for call resolution. Branches are
tracked textually from the directive lines, because conditions like
`__has_include(<x>)` leave tree-sitter-cpp with an error-recovered tree
that has no `preproc_else` node to find. Include-guard (`#ifndef`) code
is a first branch and still resolves.

Test: cpp_xtu::preprocessor_alternative_branches_are_not_call_targets
fails before this change (spdlog::info resolves into the #elif stub).
The base walker descended into type-argument lists, so
`class MyBloc extends Bloc<Ev, St>` emitted inherits edges to Bloc, Ev
and St; on pipe, 39 of 145 Dart inherits edges (27%) pointed at type
arguments (e.g. `Command<int>` -> `int`). C++ `: public Base<Arg>`
shared the walker through `template_argument_list`.

The walker stops at `type_arguments` / `template_argument_list` /
`type_parameters`, and handles tree-sitter-dart's shape, where the
arguments are sibling `type` fields of `superclass` (only the first
names the base) and nested `type` children of a `type`.

Test: languages::generic_type_arguments_are_not_base_classes fails
before this change (["Bloc", "Ev", "St"]).
`brick check` read `metrics` from the model JSON and gated on them, so
setting `cycle_count` to 0 and `score` to 100 in the file turned a
failing `--max-cycles 0` gate into "passed" without changing the graph.

check now recomputes the metrics from the loaded nodes and edges and
gates on those; if the stored copy differs, that is itself a failure
("stored metrics do not match the graph"). Honest models recompute to
identical values, so existing gates are unchanged.

Test: diff_cli::check_recomputes_metrics_instead_of_trusting_the_file
fails before this change (the tampered model passes).
A directory with no supported source (Go/Java only) built
"1 nodes / 0 edges; score 100.0" with no diagnostic, and
`check --min-score 90 --max-cycles 0` passed it. The one-line summary
also hid diagnostics entirely: the yams run's 7,428 warnings (505 files
with grammar errors) never appeared.

- scan records a warning diagnostic when no source file was scanned;
- `build` prints the diagnostic and warning counts in its summary and
  warns on stderr when the model is empty;
- `check` fails a model that contains no scanned source files.

Test: diff_cli::a_scan_with_no_supported_source_is_reported_and_fails_check
fails before this change (no diagnostic; check passes).
Three lexical-preview bugs, reproduced on one probe:

- A blank or comment-only line in a Python function fell through to the
  brace-depth scope rule, which popped every open scope, so every call
  after the first blank line in a body was attributed to the file.
  Blank/comment lines now leave Python scopes unchanged.
- `class Child(Base):` matched the call pattern and produced a call edge
  from the file to Child. The declared class name is no longer a call
  on its own header line.
- Calls resolved to the only function with that name anywhere in the
  repository, imported or not (a NameError at runtime became an edge).
  Resolution now prefers the caller's file; Python then considers only
  files the caller imports (resolved import edges, cached per file);
  otherwise the call stays external. Swift keeps the module-wide unique
  name after the same-file check, because files of one Swift module see
  each other without imports. Imported bases resolve the same way, so
  `from pkg.b import Base` disambiguates `class Child(Base)`.

Test: languages::python_calls_keep_their_function_and_resolve_only_what_is_visible
fails before this change (go() had no call edges at all).
A Dart import exposes a library's public top-level names without a
per-name binding, so the binding-based lookup never matched and no
cross-file Dart call resolved, even when the import itself did (pipe's
VerifyCommand::run lost loadProject, analyzeProject, verifyAnalysis and
decodeBaselineEntries).

After the lexical-owner walk and binding lookup fail, a Dart call now
resolves to the one public (not `_`-prefixed) top-level declaration of
that name in the libraries the file imports; several such declarations
make it `ambiguous`, none leaves it external. `package:<name>/x.dart`
imports resolve to `lib/x.dart` when `<name>` is the root pubspec's
package; other packages stay external. `show`/`hide` combinators are not
modelled (noted as a follow-up).

Test: languages::dart_calls_resolve_through_imported_libraries fails
before this change (helper stays not-visible); an unimported file and a
library-private name stay unresolved.
The Fold report importer was schema-less and trusted self-description:
- `{"hello":1}` imported 0 findings with no diagnostic, like a clean run;
- findings without a file location were dropped silently;
- any JSON declaring backend "source" and confidence "proved"/"latent"
  got the Symbolic lane, whoever produced it;
- SARIF `file://` URIs (and %-escapes) never matched a scanned file.

Now a report must be a SARIF log (`runs`) or a Fold findings report
(`findings`), otherwise the import fails with "not a Fold report". The
Symbolic lane requires the producer to be Fold (`tool.name`, or every
SARIF run's `tool.driver.name`, equal to "fold", which is what Fold
writes); other producers' findings are imported in the Imported lane
with a diagnostic. An unsupported `output_version` and findings dropped
for lack of a location are diagnosed. `file://[localhost]` URIs lose
their scheme and are percent-decoded before path matching. Existing
tests that model Fold reports now carry Fold's real envelope.

Tests (fold::tests) failing before this change:
rejects_a_file_that_is_not_a_fold_report,
symbolic_lane_requires_a_fold_producer_not_a_self_declared_label,
sarif_file_uris_match_and_unlocated_findings_are_diagnosed.
Fold report paths were normalized lexically, so `linkout/../src/main.rs`
with `linkout` pointing outside the root attached to the repository's
`src/main.rs` although the OS path names a different file.

A relative path containing `..` that exists in this checkout is now
canonicalized (and must stay under the canonical root); paths absent
from the checkout, such as reports from another machine, keep the
documented lexical matching.

Test: fold::tests::parent_steps_after_a_symlinked_directory_follow_the_filesystem
fails before this change (the finding attaches to src/main.rs).
A single invalid byte, such as a Latin-1 `é` in a comment, made
`read_to_string` fail and the whole file and its symbols were skipped.

Files are now read as bytes: a NUL byte still marks a binary file,
which is skipped with a diagnostic; otherwise invalid UTF-8 is decoded
lossily (U+FFFD) and a warning diagnostic records it, so the file keeps
its symbols and calls.

Test: languages::a_non_utf8_byte_does_not_drop_the_whole_file fails
before this change (latin1.rs has no symbols).
`.c` files were parsed with tree-sitter-cpp, so valid C that uses C++
keywords as identifiers (`int delete = 0; int new = 1;`, a parameter
named `class`) produced grammar-error diagnostics and lost symbols.

`.c` files now parse with tree-sitter-c (pinned =0.24.2, backend
`c:0.24.2`) while staying on the shared C/C++ extraction path, so C
sources and the headers they include still share one include/declaration
index. Headers (`.h`) keep the C++ grammar, which parses the C subset.

Test: cpp_xtu::c_sources_parse_with_the_c_grammar fails before this
change (grammar errors in util.c).
tree-sitter's `Node::parent` re-descends from the root, so each call is
O(depth), and `enclosing_function`, `lexical_scope` and
`parameter_shadows` walked ancestors through it for every call site and
declaration: extraction time grew quadratically with nesting depth
(sample profile: >90% of time in ts_node_parent under those three).

The main traversal now records each node's parent, and the three
ancestor walks look it up (falling back to `Node::parent` for nodes the
traversal skips, so results are unchanged).

Measured on one C++ file with a call nested N blocks deep (release,
macOS arm64): N=2.5k/5k/10k/20k took 0.26/1.05/4.24/16.8 s before;
after, 20k takes 0.02 s with the same nodes and call edge. The full
suite (111 tests) passes unchanged.
docs: state resolution rules and limits as the code now implements them
Some checks failed
build / build-linux (pull_request) Successful in 1m3s
build / build-macos (pull_request) Has been cancelled
build / release (pull_request) Has been cancelled
46698cd87d
The audit found README claims the code did not back: resolution was said
to respect lexical namespace everywhere (Python/Swift matched a
repo-wide unique name, Rust types their leaf name), "ambiguous targets
stay external" (not for rival using-directives or #else stubs), Dart
imports were said to resolve (no Dart symbol ever did across files), and
"C/C++" hid that C went through the C++ grammar.

The README now describes the fixed behaviour and its remaining limits:
per-language visibility rules (Python imports, Swift module-wide names,
Dart imported libraries without show/hide), Rust type identity by
declaring file, preprocessor alternatives excluded from resolution,
using-directive ambiguity, the C grammar for .c, Fold report
validation and producer-gated symbolic lane, lossy UTF-8 decoding,
diagnostic counts and empty-scan reporting, check recomputing metrics,
low call-site coverage on large C++ code, no virtual-override fan-out,
and unrecognized Catch2 TEST_CASE bodies. The research note lists the
pinned C grammar.
trevon merged commit 2a9716dac0 into main 2026-09-25 02:39:30 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
1 participant
Notifications
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!5
No description provided.