Repository navigation
fix(browser): mark an undecodable worker inventory unavailable - #3513
NIU-123370 wants to merge 2 commits into
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…meration A malformed entry followed by a valid worker is the only shape that tells a per-entry exclusion apart from abandoning the rest of the inventory. Refs morluto#3378.
baron
left a comment
baron
left a comment
There was a problem hiding this comment.
Reviewed head 9126757. The worker capture producer now distinguishes absent/non-array inventory from valid empty inventory and records non-object omissions before filtering, retaining later valid workers. The completeness owner already makes invalid_protocol_value unavailable; tests assert both survivor and omission and preserve the valid-empty control. No blocker found within this focused page-inventory fix. Isolated matching-lock snapshot with pinned Node 24.18.0: cdpWorkerAttachmentProtocol.test.ts passed 6/6. This is controlled CDP evidence, not a new real-browser verification claim. Current unchanged head is open, mergeable and all reported checks green (conditional skips only), including CI required.
baron
left a comment
baron
left a comment
There was a problem hiding this comment.
Supersedes my earlier approval: a follow-up malformed-value probe confirms directly related producer-boundary cleanup is incomplete. The current six tests pass, but all four added probes for an absent/null/numeric/empty TargetInfo.type fail because worker completeness stays falsely complete. Holding this head until the remaining unknown-to-empty collapse is addressed. Reproduced in an isolated snapshot using the existing inspectWithWorkerInventory helper, controlled loopback CDP, matching locked dependencies and pinned Node 24.18.0 (6 existing passed; 4 probes failed); no live-browser claim.
| completeness.exclude("workers", "invalid_protocol_value"); | ||
| continue; | ||
| } | ||
| const type = cdpStringValue(target.type) ?? ""; |
There was a problem hiding this comment.
[P2] Preserve uncertainty when a record has an undecodable target type. The new loop rejects non-object entries, but cdpStringValue(target.type) ?? "" still turns a record-shaped malformed entry into a definite non-worker at if (!type.includes("worker")) continue. For example, return targetInfos: [{ targetId: "worker-1", url: origin + "/worker.js", openerId: "allowed-page", attached: true }] through the existing helper: the inspection returns workers: [] and no worker unavailable/exclusion marker, just like the false-complete inventory this PR fixes. Null, numeric and empty type values behave identically. A missing/undecodable required discriminator cannot establish that an entry is not a worker. Validate that discriminator at this owning boundary before the non-worker filter; record invalid_protocol_value for undecodable entries, retain later valid entries, and cover malformed-record plus valid-non-worker/empty controls. This is the same directly related unknown-to-empty representation issue, not a request to broaden validation to unrelated TargetInfo fields.
|
Compared this PR with main This uncertainty fix is still absent on main. The current worker inventory path converts missing/non-list targetInfos and filters malformed records in a way that can make an undecodable inventory look empty; command rejection is handled separately. Please keep missing, malformed, explicitly empty and valid inventories distinct at the producing CDP parser, with partial facts and coverage passed to consumers. Coordinate with #3850/#3901 so worker/resource inventory admission has one owner rather than parallel repairs. #3378 remains actionable. PR head reviewed: |
|
TL;DR: The PR's fix works for the cases it names. On current main, 3 of its 4 new tests fail, and with the PR all 6 tests in the file pass. Baron's four extra probes still fail on this head, because a bad worker type leaves the list marked complete. Thanks, NIU-123370, for the fix, and thanks, baron, for the probes. I tested head 9126757 on top of main 62bf098. The PR's changes were applied as a patch. Results
Baron's probes
How the probes workBaron reported four probes where The cause is in My probe file is in the draft folder as Checks and scope
Not run
Commandsgit -C ~/code/rea-mirror/rea.git fetch origin
git -C ~/code/rea-mirror/rea.git archive 62bf09809ef7bb224028916424f8f07652a20f4f | tar -x -C /tmp/t17-1015/main
# npm ci, Node 24.18.0, exit 0
npm run test:focused -- tests/boundary/browser/cdpWorkerAttachmentProtocol.test.ts # main alone: 2 passed
# Before: apply only the PR's test diff onto main
git apply --include='tests/*' 3513.patch
npm run test:focused -- tests/boundary/browser/cdpWorkerAttachmentProtocol.test.ts # 3 failed, 3 passed
# Probes on main, then After: apply the code change
npm run test:focused -- tests/boundary/browser/baronProbes.test.ts # 4 failed
git apply --exclude='tests/*' 3513.patch
npm run test:focused -- tests/boundary/browser/cdpWorkerAttachmentProtocol.test.ts # 6 passed
npm run test:focused -- tests/boundary/browser/baronProbes.test.ts # 4 failedI grepped the PR's test file and the fake CDP browser fixture for doctor first. There were no hits. The patch is the diff from the merge base to the head. Test setup🤖 Generated with Claude Code (Sonnet 5.5, verified by Opus 5.5) 🦊 |
This branch has not been deployed
4 participants
Summary
Fixes #3378.
inspect-web-pagereportedworkers: []with no omission marker whenever thebrowser's
Target.getTargetsreply could not be read as a worker inventory. Only arejected command was treated as unobservable; a reply that omitted
targetInfos, delivered it as a scalar, or delivered a non-object entry wassilently turned into "this page has no workers", so a failed enumeration was
indistinguishable from a valid empty inventory.
Problem and expected behavior
Target.getTargetsreply is{}, or{ targetInfos: "worker-one" }, or{ targetInfos: [null] }.unavailable/uncertain instead of being reported as an empty result.
targetInfosmarks theworkerssectionunavailable; a malformed entry is recorded as an item exclusion; a delivered
empty list remains a valid empty inventory and gains no marker.
This follows the pattern the repository already uses for the same protocol shape
in storage capture (
src/browser/CdpCaptureStorage.ts:145treatsraw === undefined || !Array.isArray(entries)as "cannot observe", distinguishingprovider_unavailablefrominvalid_protocol_value) rather than inventing a newvocabulary.
Change and scope
src/browser/CdpPageCaptureWorkers.ts— the inventory is read once, and:completeness.unavailable("workers")and theenumeration stops;
completeness.exclude("workers", "invalid_protocol_value"),The now-unused
recordsValueimport was dropped; the pre-filtering helper thatcaused the silent drop is no longer used on this path.
tests/boundary/browser/cdpWorkerAttachmentProtocol.test.ts— four new cases(absent key, non-list, malformed entry followed by a valid worker, valid empty
control). The malformed-entry case and the existing
it.each(["omitted","null"])case now share one local
workerTarget(origin, attachment)builder, so the filekeeps a single capture preamble and a single
TargetInfoliteral.Intentionally not included:
(
src/browser/CdpElectronWorkers.ts:42pre-filtersrecordValue(result)?.targetInfoswithrecordsValue). I left it untouchedbecause its boundary lane needs a real
.appfixture that I did not execute inthis session, and I would rather ship a proven page-side fix than an
unproven second copy. The same three cases apply there and can follow this PR.
WebPageInspection["workers"]items, the completeness schema, orthe comparison rules: the fix only stops an undecodable enumeration from being
reported as an empty observation.
Contract and boundary impact
CdpCaptureCompletenessremains theonly owner of completeness state; the earliest changed stage is worker inventory
decoding inside page capture.
existing boundary — not applicable.
malformed reply now yields
unavailable_sections: ["workers"](plus thecorresponding
provider_unavailableexclusion) instead of a clean-looking emptylist.
inspection only; no provider or platform gate touched.
claims disappear for this section; comparisons that previously treated two such
captures as equal on
workersnow see the uncertainty. No retained field isrenamed or removed.
new CDP command is sent, and the same single
Target.getTargetscall is used.docs/public/product-catalog.json), package, or installationimpact: none.
Expected effects:
Evidence and regression coverage
Tests added or updated: four cases in
tests/boundary/browser/cdpWorkerAttachmentProtocol.test.ts, including thenegative control that a delivered empty inventory stays unmarked, and the case
that a malformed entry is excluded without ending the enumeration (arm M7).
Review revision
9126757b, taken after the Greptile [P2] on headf2cf93be:the malformed-entry case used to feed
[null]alone, so it also passed when theper-entry
continuebecamereturn items, i.e. when the rest of the inventorywas abandoned. Measured, not inferred — that mutation gives
6 passed (6)against the previous file and
1 failed | 5 passed (6)against this one. Everyarm below was re-run against the revised file.
Base reproduction: with the final test file and only the source change reverted,
the same file reports
3 failed | 3 passed (6)(arm M1 below is exactly thatbase revision), i.e. the reported behavior was "no marker at all". Executed.
Why the non-list case asserts the exclusion entry rather than only
unavailable_sections: while measuring this fix I found that a weaker guard(
inventory === undefined) still leavesworkersinunavailable_sectionsforthat reply, so the section marker alone does not pin the distinction. The
observable that does is which exclusion the section carries:
{ reason: "provider_unavailable", count: null }(section cannot be observed)versus
{ reason: "invalid_protocol_value", count: 10 }(one per character ofthe scalar, i.e. the reply decoded item-by-item). The assertion records the
former. The same mechanism is why arm M3 is now killed by that one case alone:
CdpCaptureCompleteness.exclude(src/browser/CdpCaptureCompleteness.ts:63-67)also puts a section into
unavailable_sectionsforinvalid_protocol_value, soan item-exclusion-only implementation still satisfies a bare
unavailable_sectionsassertion and only the recordedreason/counttellsthe two apart.
Mutation arms, each actually run against
tests/boundary/browser/cdpWorkerAttachmentProtocol.test.ts, tree restored andre-verified between arms (final test file re-run afterwards, same result):
6 passed (6)3 failed | 3 passed (6)inventory === undefinedinstead of!Array.isArray)1 failed | 5 passed (6)1 failed | 5 passed (6)1 failed | 5 passed (6)1 failed | 5 passed (6)1 failed | 5 passed (6)continue→return items)1 failed | 5 passed (6)M4 is killed only by the empty-list control, which is why that case is in the
file. M5, M6 and M7 are killed only by the malformed-entry case, and M7 is the
arm that separates "excluded this entry" from "stopped enumerating": it can only
fail if a valid entry follows the malformed one.
Remaining proof gaps: the malformed replies are delivered by the repository's
fake CDP peer (
commandResultknob), matching the reporter's ownproducer-simulation method; no live Chrome was made to send a malformed
Target.getTargets. The Electron sibling path is uncovered here, stated above.Validation performed
npx vitest run tests/boundary/browser/cdpWorkerAttachmentProtocol.test.tswith only the source change reverted —
3 failed | 3 passed (6); with the fix —Test Files 1 passed (1),Tests 6 passed (6).npx vitest run tests/boundary/browser/ tests/boundary/mcp/browserMcp.test.ts—
Test Files 50 passed | 1 skipped (51),Tests 554 passed | 6 skipped (560);re-run on the final base
eab789aed, same counts.npm run typecheck— exit 0 (also re-run oneab789aed).npm run lint— exit 0 (Checked resolved module boundaries in 1885 source files.).npm run verify:test-discovery— exit 0 (Discovered all 1279 test files exactly once.).npm run verify:architecture-guards— exit 0(
Architecture guards passed across 1398 source files.)../node_modules/.bin/oxfmt --checkon the two changed files — exit 0,All matched files use the correct format.(prettier was not run).npm run jscpdon this head —Duplication: 2724 files, 2008 clones, 3.22%.Measured three times in one tree swapping only the two PR files: trunk
eab789aed= 2008 clones / 21105 duplicated lines / 3.215794%, previous headf2cf93be= 2008 / 21105 / 3.215520%, this head = 2008 / 21105 / 3.215441%.The clone count and duplicated-line count are unchanged, so neither the patch
nor the review revision adds a clone — the percentage moves only because the
total line count does. The review revision replaces a second inline
TargetInfoliteral with the sharedworkerTargetbuilder instead of addingone.
npm run build/real-browser runtime for this path (the cases areprotocol-level),
check:ci, the Electron boundary lane, other platforms.Compatibility, safety, and release
silent empty
workersarray for a malformed reply now see the uncertainty,which is the reported defect.
fix; no live browser produced a malformed inventory in this session.
origin scope, no new retained content; malformed protocol data is disclosed
rather than interpreted.
Author readiness
Human owner: @NIU-123370
Before merge
type(scope): outcome.