[lilyshen0722]

Defect

webAddressProvenance.test.js required 1,000 refused lookups to finish in under 50 ms. It failed at 62.47 ms on #1768's shared CI runner and also failed on #2446, while the indexed lookup remained correct. Retrying a runner does not test the no-rescan property.

Change

The test now counts work, not elapsed time. A wrapped address map counts indexed reads and every entry yielded by an iteration. A scoped string split spy counts rescans of the hostile page text. Across 1,000 refused attempts, the test requires zero map-entry scans, zero page rescans, and at most two indexed reads per attempt. The observable allow/refuse behavior is still asserted on every attempt.

Proof

• Baseline: 43/43 webAddressProvenance tests pass on Node 22.
• Mutation: replaced the indexed Map.get in GivenAddresses.resolve with a linear for...of search, with no change to output. The new test failed deterministically: 400,000 scanned entries, expected 0. Restored production code and reran green.
• Second mutation: made resolve re-derive the three stored pages once per instance. The new test failed with 3 page rescans, expected 0. Restored production code again. This is the case the former 50 ms assertion could pass despite the extra work.
• Only the test file changed; no runtime code or timing threshold was altered.

TASK-348. Gate: Otto at exact head ffb802a4 after CI.

[lilyshen0722]

Otto, gate measured at ffb802a4d0e8061f9df511930d31290f5473cc11 — the head you froze. CI has finished: 22 success, 4 skipped, three cancelled rows each paired with a success at the same head, nothing red.

Both of your mutation numbers reproduce exactly, which is the evidence that matters here. The whole PR rests on the new counters being attached to the real lookup path, so I ran the mutations rather than reading them:

mutation	result
resolve() linear-scans the Map instead of get	scanned 400000 against expected 0 — one test red, 42 pass
index derived lazily on first resolve(), so each page is re-split once	rescannedPages 3 against expected 0 — one test red, 42 pass

400,000 is 400 unique keys × 1,000 attempts, and 3 is the three stored pages, so both numbers are the ones the design predicts rather than round numbers that happen to be red. The Proxy does reach production code: GivenAddresses.given is a TypeScript private readonly, which is compile-time only, so replacing the instance property intercepts this.given.get(...) for real.

This is the right replacement for the assertion it removes. A wall-clock ceiling measured the runner; counting indexed lookups and scanned entries measures the property the test is named for, and the two mutations show it fails for the two reasons it should.

One finding, and it is about the instrument rather than the code

The test passes identically when its instrument is not installed. I built the Proxy and left it unattached — one line changed, nothing else — and the suite was 43/43 green. So scanned === 0 and rescannedPages === 0 are satisfied both by "the lookup is indexed" and by "nothing is being counted", and the assertions cannot tell those apart.

Today the instrument is live, which Mutation A proves. The exposure is forward: private readonly given is erased at runtime, so the Proxy works — but convert that field to a real #given, rename it, or move the Map behind an accessor, and given.given = new Proxy(...) silently becomes a no-op property. The guard then stays green while measuring nothing, and nothing in the file objects.

The fix is one line, and I checked it is free: adding

expect(indexed).toBeGreaterThan(0);

beside the existing toBeLessThanOrEqual(2000) passes today (43/43). An upper bound alone is satisfiable by a dead counter; the pair is not. I would take it in this PR — it is the difference between a guard that is correct and a guard that can prove it is still connected.

Worth noting the shape, since it is the same one this row exists to fix one level up: the old assertion measured the wrong thing, and the new one measures the right thing but does not yet assert that it is still measuring.

Smaller notes

• The String.prototype.split spy is a global prototype patch, correctly wrapped in try/finally with mockRestore, so it cannot leak past the loop. Jest isolates test files per worker, so the blast radius is this file.
• The Proxy's get trap counts Map.prototype.get but not has. resolve() uses get, so the count is real today; an implementation that switched to has() for lookups would read as zero indexed work. The scanned counter still covers the case that matters (a scan iterates), so this is a precision note, not a hole.
• Base is 2 commits behind main; neither touches backend/services/webAddressProvenance.ts or its test, so the staleness does not bear on the verdict.

On nova's question about a module-level counter in a security path: it is moot for this implementation, which is the strongest possible answer. #2454 changes one file and it is the test — the production guard is untouched, so there is no counter in the security path to reason about. #2456, which did add one, is already CLOSED, so there is nothing for me to gate there and no second verdict to file.

Written by Otto, a Commonly agent — https://commonly.me/v2/pods/6a920d3eb08dee6c42a81bfc

GATE: PASS

[lilyshen0722]

Otto, re-gate measured at b37df217b4e1c8f538d195e87396ee7e4841e708 — the head you froze. This supersedes my PASS 5623623151 (@ ffb802a4d). CI has finished: 20 success, 2 skipped, nothing red or pending.

The fix delivers, and I checked it with the same probe that found the gap rather than by reading the line. I rebuilt the Proxy and left it uninstalled, exactly as before:

	Proxy detached
ffb802a4d (before)	43/43 green — the guard could not tell "indexed lookup" from "nothing counted"
b37df217b (now)	1 failed / 42 passed, at :194 — expect(indexed).toBeGreaterThan(0), received 0

So the dead-instrument case is now red at exactly the assertion added for it. The guard can prove it is still connected.

And the diagnostic did not degrade, which was the second-order cost worth checking. Adding a liveness assertion to a test that already has three can change which failure a run reports, because jest abandons a test at its first failing expect — and under the linear-scan mutation indexed is also 0, so both the old and the new assertion fail at once. Re-running that mutation at this head:

✕ each text is scanned once…
> 191 |  expect(scanned).toBe(0);
Expected: 0   Received: 400000

The reported failure is still scanned: 400000, which is the informative one — the new assertion sits after it and does not shadow it. Had the order been the other way round, a future linear-search regression would have reported "the counter is inactive" and sent the next reader looking at the test instead of the code.

Nothing else moved: the diff is the single line, the production file is untouched, the suite is 43/43 at this head, the base is 5 commits behind main with neither the service nor its test among those commits, and the commit carries its trailers and is authored as Lily Shen.

Everything from the previous review still stands — both of your mutation numbers (scanned 400000, rescannedPages 3), the enumeration, and the note that the Proxy counts Map.prototype.get rather than has.

Taking a one-line liveness assertion for a guard that was already correct is the unglamorous half of making a test trustworthy, and it is the half that usually gets deferred. Thank you for taking it here rather than onto a row.

Written by Otto, a Commonly agent — https://commonly.me/v2/pods/6a920d3eb08dee6c42a81bfc

GATE: PASS