Repository navigation
Conversation
`noiseOnly` is a line-level guess and always was. It matches changed lines against regexes — `babelHelpers.extends` and friends — which catches transpiler boilerplate and nothing else. The dominant form of noise in a minified bundle is invisible to it by construction: minifiers name by position, so inserting one binding renames every local after it, and each of those lines looks exactly like real code because it *is* real code that means precisely what it meant before. No line rule can tell that from an edit. Between two WhatsApp revisions this is 3,897 modified modules, and a reviewer has no way in. `--semantic` (`semantic: true` over MCP) compares each changed module with its local names erased. Every identifier binding within the module becomes the order it was first encountered, so a rename is invisible by construction rather than by pattern. Free identifiers, literals and statement order are deliberately kept — each is a real difference a looser comparison would swallow, and literals in particular are the wire values people diff bundles to find. `!0`/`!1`/`void 0` fold, since a minifier changing its mind about an encoding has not changed the program. The result is a `verdict` of `renamedOnly` or `changed`, and where the two disagree the proof overrides the guess: `renamedOnly` sets `noiseOnly`, because it has established what the line rules were estimating. A verdict alone still leaves a reviewer holding a whole module, so a changed one also reports which functions changed, by name. Getting names out of minified code needs the export table: a module declares everything under locals — `function u(t)` — and publishes at the bottom, `l.encodeServerErrorReceipt = u`. The property name is the module's interface and is stable across builds, so locals are resolved through it before being used as labels. `ACSTokenStore` then reads as `changed: getEntries, get` and `added: set, delete, clear` rather than as 63 changed lines. Three details came out of real output rather than out of design. Anonymous ancestors are stripped from labels, because every function in a Metro module is nested inside an unnamed factory and would otherwise hang off an ordinal that shifts whenever an earlier closure is added. Unnamed functions pair on enclosing name and arity, because reporting each as an addition *and* a removal doubled every count — including the factory, so every changed module was double-counted. And only the innermost change is kept, since a shape includes its body, so one edited line marked the closure, the function around it, and the factory around that. Off by default: it parses both sides of every changed module, which the sha256 comparison does not, and costs about a second per thousand modules. `fuzz` proves the classifier on real modules rather than on examples chosen to pass, mutating them two ways where the answer is known by construction and reporting the two error rates separately, since a false alarm and a miss do not cost the same. It returns a `FuzzReport` rather than printing, so a caller decides what to do with it. Measured over 600 WhatsApp modules: 599/600 renames absorbed, 1,467/1,467 edits caught, none missed. One finding worth recording for anyone reaching for this: across two real revisions, *zero* of the 3,897 modified modules were renames alone. A release touches something real in everything it touches, so the module is the wrong unit and the function is the right one — 28,490 of 51,942 functions in those modules were unchanged, which is the 54% of the diff a reviewer never needs to open.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80f960e10d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // the identifier — `VariableDeclarator(h)` — so every parent | ||
| // node leaked the very name the normalization exists to erase, | ||
| // and two renamed-but-identical modules never matched. | ||
| self.push(&format!("{:?}", std::mem::discriminant(&other))); |
There was a problem hiding this comment.
When --semantic compares modules whose real edit is stored in an AST node field rather than a child node—for example var becoming const, a function becoming async, or template-literal text changing—the fallback emits only the node discriminant and discards that field. The revisions can therefore receive the same hash and a renamedOnly verdict, after which the diff suppresses the real change as noise by default; encode all behavior-bearing flags and literal payloads in the canonical stream.
Useful? React with 👍 / 👎.
| if opts.semantic { | ||
| apply_semantic_diff(&mut change, &old_text, &new_text); |
There was a problem hiding this comment.
When either revision exceeds the default 1 MiB maxDiffBytes, the too_big branch skips this call entirely, so --semantic returns neither a verdict nor function results for precisely the large modules where textual diffs are omitted. The option and MCP schema promise classification for each changed module, while hunksOmitted only explains the textual diff; load/classify these modules independently of hunk generation or explicitly report a semantic omission.
Useful? React with 👍 / 👎.
| change.functions_kept = Some(d.kept as u32); | ||
| change.functions_total = Some(d.total as u32); |
There was a problem hiding this comment.
When one nested function changes, its enclosing functions and Metro factory also get new hashes, so they reduce kept before narrow removes those ancestor entries from the reported change lists. Copying the original kept and total here makes the text renderer claim that multiple functions changed even though only one localized function remains; recompute the displayed count after narrowing or expose this as structural overlap rather than a changed-function count.
Useful? React with 👍 / 👎.
`exports` and `functions` were empty for all 187,892 modules of a real bundle,
and had been for as long as the fields existed. Name and dependencies come from
the first two arguments of `__d(...)` and were unaffected, so every module
indexed, looked complete, and quietly claimed to export nothing — which is a
plausible thing for a module to do and therefore never looked like a bug.
Two independent causes, either sufficient alone.
The factory is written parenthesized — `__d("M", [deps], (function(…){…}), 98)`
— and oxc preserves parentheses in the tree, so matching `FunctionExpression`
against the argument saw a `ParenthesizedExpression` and gave up before reading
anything. Hence `functions` being empty too, which is what gave the game away:
that walk does not look at parameters at all, so a parameter-indexing mistake
could not explain it.
And `exports` is not Metro's fifth parameter here. A handful of modules ship
unminified and spell the signature out, which is better evidence than any
documentation; across one revision 135 of them do, in two shapes:
133× (global, require, requireDynamic, requireLazy, module, exports)
2× (global, require, importDefault, importNamespace, requireLazy, module, exports, …)
Anything after `exports` in the long form is a dependency the loader injects
directly — those two receive `invariant` and `fbt` that way — so `exports` is
neither fifth nor last. Both readings were confirmed against 4,000 minified
modules by how each parameter is *used*, resolved through the symbol table:
counting by name gives confident nonsense, because parameters are single letters
and the same letters are reused as locals, so `function u(t)` shadows the
factory's `t` and credits every use of the local to `global`.
Rather than swap one hardcoded index for another, the binding is now found by
what the factory calls it, then by what it does with it — the exports object is
the one with properties hung off it, `module` is the one carrying `.exports` —
and only then by position. Behaviour is a fact about the module in hand; the
position table is a generalisation over a bundler that has already changed once.
The existing fixtures all passed throughout, because all of them were written by
hand to stock Metro's shape: unparenthesized, seven parameters, exports fifth.
They tested what their author expected the bundle to look like. Added: the real
shape, a `.react` module where injected dependencies follow `exports`, and the
six-parameter convention. Also `examples/probe.rs`, which runs the extractor
over actual bundle files, because that is the check that would have caught this.
Measured over 4,000 real modules: exports 0% -> 99%, functions 0% -> 73%.
Finding `module` by `module.exports` and then falling back to arity for
`exports` leaves one shape unread, and arity cannot fix it because arity is
exactly what is ambiguous: the six-parameter convention plus one injected
dependency has seven parameters and is not the seven-parameter convention.
__d("GeminiEmojiCategories", ["fbt"], (function(t, n, r, o, a, i, l) {
a.exports = { … name: l._("") … }
`a` is the module object, `i` is exports, `l` is the injected `fbt`. Read by
arity, exports is looked for on `fbt`, which of course has none.
`exports` immediately follows `module` in both conventions, so the anchor that
was already being found settles the other. Position stays as the last resort,
for factories that say nothing about themselves either way.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f5f755648
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let next = self.seen.len() as u32; | ||
| let n = *self.seen.entry(id).or_insert(next); | ||
| self.push(&format!("${n}")); |
There was a problem hiding this comment.
When a function switches between two module-local bindings, each fresh Canon sees the selected captured symbol for the first time and emits the same ordinal (for example, return a() and return b() both encode that reference as $0). The module verdict is changed, but diff_functions treats the edited function as kept and typically attributes the change to its enclosing Metro factory instead, so functionsChanged fails to name the function containing a common dependency/alias change. Seed function canonicalization with module-level binding identities or otherwise distinguish captures from bindings declared inside the function.
Useful? React with 👍 / 👎.
| if self.depth == 0 | ||
| && let Some(last) = f.params.items.last() | ||
| && let BindingPattern::BindingIdentifier(id) = &last.pattern | ||
| { | ||
| self.obj = id.symbol_id.get(); |
There was a problem hiding this comment.
For factories with injected dependencies after exports, such as the .react shape added to the extractor tests, the last parameter is explicitly not the exports object. Assigning that final parameter here means a declaration such as function u() {}; l.foo = u is never resolved to foo, so function changes are reported under unstable anonymous labels and can be paired incorrectly. Reuse the module/export-position logic rather than assuming the final parameter is exports.
Useful? React with 👍 / 👎.
| // Everything else identifies `exports`. The later slot wins, because | ||
| // `module` precedes `exports` in every convention and a module writing | ||
| // to both is exporting through both. | ||
| let by_exports = assigned.other.iter().filter_map(|n| slot(n)).max(); |
There was a problem hiding this comment.
In a stock seven-parameter factory, a nested closure can shadow a later factory parameter—for example, the real exports parameter e writes e.real, while function f(d) { d.tmp = 1 } shadows the factory's slot-6 d. Assigned walks nested functions and records only identifier text, so taking the maximum matching slot selects factory parameter d as exports, misses real, and records tmp instead. Restrict inference to references resolving to the factory parameters (or at least exclude nested scopes).
Useful? React with 👍 / 👎.
| /// different program", and when it is, "which functions" — the two things a | ||
| /// reviewer actually wants and neither of which a line diff can supply. | ||
| fn apply_semantic_diff(change: &mut ModuleChange, old_text: &str, new_text: &str) { | ||
| let verdict = shape::compare(old_text, new_text); |
There was a problem hiding this comment.
When a module's primary definition is unchanged but a variant is added, removed, or edited, ModuleEntry::fingerprint() correctly marks the module modified, but these texts are loaded only from the primary files. This comparison therefore returns Same and produces no function changes even though a shipped variant's program changed; variants are documented as common and as rollout signals. Semantic classification needs to cover the complete primary-plus-variants definition set used by the fingerprint.
Useful? React with 👍 / 👎.
| // The line counts above measure the text; this measures the program. | ||
| // Printed second so the two read as claim and correction where they | ||
| // disagree, which on a minified bundle is most of the time. | ||
| if let Some(verdict) = c.get("verdict").and_then(Value::as_str) { |
There was a problem hiding this comment.
Semantic fields are rendered only by diff_text; diff_markdown never reads verdict or any functions* field. Consequently, cellar diff --semantic --format md pays for classification but silently emits the same report as a non-semantic run, omitting both the rename verdict and the promised changed-function names. Add equivalent semantic output to the Markdown renderer.
Useful? React with 👍 / 👎.
| d.added | ||
| .retain(|f| !contains_another(f.start, f.end, &inner)); |
There was a problem hiding this comment.
When an added function contains any nested function, both appear in d.added, so this containment filter removes the outer function and reports only its innermost closure. For example, adding exported encode with a callback yields an anonymous encode/#n addition instead of reporting that encode itself was added; the same loss occurs symmetrically for removals. Narrowing ancestor hashes is appropriate for edits propagated from a child, but not for whole-function additions or removals.
Useful? React with 👍 / 👎.
| // `void 0` is `undefined`, for the same reason. | ||
| if expr.operator == UnaryOperator::Void | ||
| && let Expression::NumericLiteral(n) = &expr.argument | ||
| && n.value == 0.0 | ||
| { | ||
| self.push("undefined"); | ||
| self.push(")"); |
There was a problem hiding this comment.
void 0 like a free undefined reference
The special case emits the token undefined, while an actual unbound undefined identifier is emitted by ident as @undefined. Thus var a = void 0 and var a = undefined still receive different shapes and a changed verdict, contrary to the documented folding of this minifier encoding. Emit the same canonical representation used for a free undefined reference while retaining the distinction for a locally shadowed binding.
Useful? React with 👍 / 👎.
| for (start, end, id) in v.hits { | ||
| out.replace_range(start as usize..end as usize, &format!("q{}", id.index())); |
There was a problem hiding this comment.
For shorthand syntax such as var a = 1; use({a}), the identifier span being replaced is also the spelling of the object property key, so this produces {qN} and changes the observable key from a to qN. Destructuring shorthand has the analogous problem. These mutants are not alpha-equivalent, causing valid classifier results to be counted as rename false alarms and undermining the fuzz report's claimed known-by-construction ground truth; shorthand occurrences must be expanded or skipped.
Useful? React with 👍 / 👎.
| for f in &new { | ||
| match pool.get_mut(f.hash.as_str()).and_then(|v| v.pop()) { | ||
| Some(i) => { | ||
| old_used[i] = true; | ||
| d.kept += 1; | ||
| } |
There was a problem hiding this comment.
When two exported functions have identical shapes, this arbitrary pop() can consume the wrong old function. For example, if old A and B are identical and only B changes, unchanged new A pops old B; the remaining changed B then has no old B label to pair with and is reported as added while A is reported removed. Within a hash bucket, prefer the candidate with the same stable label before falling back to an arbitrary shape match.
Useful? React with 👍 / 👎.
| let parsed = Parser::new(&alloc, src, SourceType::cjs()).parse(); | ||
| if parsed.panicked { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
Oxc can return a recovered AST with parse diagnostics while leaving panicked false, so this code hashes a best-effort partial program rather than returning Unknown. If two malformed or unsupported module revisions recover to the same tree, compare can classify them as renamedOnly, after which the diff marks the change as noise and suppresses it. Treat any parser diagnostics as an unparseable input before building semantic symbols or a canonical stream.
Useful? React with 👍 / 👎.
1 participant
noiseOnlyis a line-level guess and always was. It matches changed lines against regexes —babelHelpers.extendsand friends — which catches transpiler boilerplate and nothing else. The dominant form of noise in a minified bundle is invisible to it by construction: minifiers name by position, so inserting one binding renames every local after it, and each of those lines looks exactly like real code because it is real code that means precisely what it meant before. No line rule can tell that from an edit.Between two WhatsApp revisions that is 3,897 modified modules, and a reviewer has no way in.
What this adds
--semanticon the CLI,semantic: trueover MCP. Each changed module is compared with its local names erased: every identifier binding within the module becomes the order it was first encountered, so a rename is invisible by construction rather than by pattern.Three things are deliberately not normalized, because each is a real difference a looser comparison would swallow — free identifiers (
Promiseis not the module's to rename), literals (the wire values people diff bundles to find), and statement order.!0/!1/void 0fold, since a minifier changing its mind about an encoding has not changed the program.Two new outputs per changed module:
verdict—renamedOnlyorchanged. Where this and the line rules disagree, the proof wins:renamedOnlysetsnoiseOnly, because it has established what the line rules were estimating.functionsChanged/functionsAdded/functionsRemoved,functionsKept/functionsTotal— which functions, by name.Names come from the export table, which is the only thing in a minified module that means the same thing next release. A module declares everything under locals —
function u(t)— and publishes at the bottom:l.encodeServerErrorReceipt = u. Locals are resolved through that before being used as labels.```
[modified] ACSTokenStore
16.0% similar, +15 -48 lines
8 of 8 functions changed
changed: getEntries, get, #3
added: set, delete, clear
```
Off by default: it parses both sides of every changed module, which the sha256 comparison does not, and costs about a second per thousand modules.
Correctness
shape::fuzzproves the classifier on real modules rather than on examples chosen to pass. It mutates them two ways where the answer is known by construction — an α-rename through the symbol table, which must come backrenamedOnly, and a literal/operator/argument edit, which must come backchanged— and reports the two error rates separately, since a false alarm and a miss do not cost the same. It returns aFuzzReportrather than printing, so the caller decides what to do with it.Over 600 WhatsApp modules: 599/600 renames absorbed, 1,467/1,467 edits caught, 0 missed.
The harness renames through the symbol table specifically because doing it textually cannot tell
var afromobj.aor from anainside a regex — earlier textual versions produced mutants that really were different programs and then blamed the classifier for noticing.Two engine bugs the tests caught, both the same mistake of normalizing away meaning:
AstKind::debug_name()embeds the identifier, soVariableDeclarator(h)leaked through the parent every name the walk had just erased; and the AST discriminant carries no operator, soa > banda >= bcompared equal.A finding worth recording
Across two real revisions, zero of the 3,897 modified modules were renames alone. A release touches something real in everything it touches, so the module is the wrong unit — but 28,490 of 51,942 functions in those modules were unchanged. That 54% is the part of the diff nobody needs to open, and it is only reachable at function granularity.
Notes for review
cellar-coregains anoxc_semanticdependency (the workspace already pins oxc 0.143; symbol ids are what make a rename invisible, and they only exist once the binder has run).crates/cellar-core/src/shape.rs, 11 unit tests of its own; 2 new tests indiff.rscovering the case the line rules cannot reach and the case where a real edit hides inside a cascade.cargo fmt --checkandcargo clippy --all-targetsclean.semanticis off — every new field isskip_serializing_if.This is extracted from jigger, where it also drives an "observation mode" that joins this diff to the extracted-fact diff, to answer which code change moved which protocol fact.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.