Skip to content

fix(cdp): handle optional DOMSnapshot string sentinels - #191

Merged
aram-devdocs merged 2 commits into
mainfrom
issue-190-cdp-optional-string-sentinel
May 1, 2026
Merged

fix(cdp): handle optional DOMSnapshot string sentinels#191
aram-devdocs merged 2 commits into
mainfrom
issue-190-cdp-optional-string-sentinel

Conversation

@aram-devdocs

Copy link
Copy Markdown
Owner

$Closes #190.\n\n## Summary\n- add optional DOMSnapshot string lookup for -1 sentinels\n- keep required string fields strict\n- preserve missing attribute values as empty strings and skip missing computed style values\n- add regression coverage for optional and required string lookup behavior\n\n## Validation\n- cargo test -p plumb-cdp\n- git diff --check\n\n## Live dogfood note\nLocal environment has no Chromium binary, so live URL lint cannot be proven locally. Needs Dogfood/manual workflow in Chromium-capable CI after this PR is available.

aram-devdocs and others added 2 commits May 1, 2026 23:03
Chrome returns `-1` as a string-table index for optional DOMSnapshot
fields (attribute values, computed-style values) to signal "no value."
The existing `lookup_string` treated all negative indices as malformed,
causing `plumb lint` against live sites to fail with:

    DOMSnapshot response was malformed: negative string index -1

Add `lookup_optional_string` that maps negative indices to `None`.
Use it for attribute values (preserving the attribute name with an
empty value) and computed-style values (replacing the inline guard).
Required fields (node names, attribute names) keep strict validation.

Closes #190

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor

I have all the context I need. Let me deliver the review.


PR #191cdp: handle optional DOMSnapshot string sentinels

Scope: crates/plumb-cdp/src/lib.rs only. 79 additions, 6 deletions.


1. Determinism

Clean. No SystemTime::now / Instant::now, no HashMap / HashSet, no ordering change in observable output. IndexMap is used throughout.

2. Workspace layering

Change is confined to plumb-cdp. No new internal dependencies. No new unsafe blocks. The crate-level #![deny(unsafe_op_in_unsafe_fn)] is untouched.

3. Error handling

lookup_optional_string returns Result<Option<&str>, CdpError> — correct shape. Library code contains no .unwrap() / .expect(). Test .unwrap() calls are covered by clippy.toml allow-unwrap-in-tests = true.

4. Test coverage

Six unit tests added covering all arms of the new helper:

Test Arm covered
lookup_string_rejects_negative_index lookup_string negative
lookup_string_rejects_out_of_range lookup_string OOB
lookup_string_resolves_valid_index lookup_string happy
lookup_optional_string_returns_none_for_sentinel < 0 branch
lookup_optional_string_resolves_valid_index happy path
lookup_optional_string_rejects_out_of_range OOB delegated to lookup_string

Warning (crates/plumb-cdp/src/lib.rs:1162–1169): no integration-level fixture exercises attributes_for_node with a -1 value sentinel. FakeDriver bypasses the CDP parsing path entirely (it calls PlumbSnapshot::canned()), so the sentinel path is unit-tested only. This is acceptable given the constraints but worth a follow-up fixture when a mock CDP response layer is available.

5. Documentation

lookup_optional_string has a clear doc comment explaining the Chrome sentinel contract and the hard boundary between required and optional string slots. Both functions are private so missing_docs enforcement does not apply. No # Errors section is needed for private functions.


Behavioral note (not a blocker)

There is a deliberate asymmetry between the two call sites:

  • Attributes (lib.rs:1164–1166): sentinel -1 → attribute inserted with value "". The PR description explicitly states "preserve missing attribute values as empty strings." This is correct — an HTML attribute can be present with an empty value, and callers downstream treat absence of the key differently from presence with "".
  • Computed styles (lib.rs:1257–1260): sentinel -1 → entry skipped. Correct — a computed style property has no "present but unknown" state.

The asymmetry is intentional and matches the semantics of each slot type.


Punch list

Severity Location Finding
Warning lib.rs:1162 No integration fixture for attributes_for_node with a -1 value sentinel; unit tests cover the helper, but the call-site wiring is untested end-to-end. Track as follow-up.

No blockers found.

Verdict: APPROVE

@aram-devdocs

Copy link
Copy Markdown
Owner Author

$Validation update after rebasing on #188/#189:\n\n- cargo test -p plumb-cdp — passed (19 tests + doctests)\n- git diff --check — clean\n- Dogfood workflow dispatched on branch issue-190-cdp-optional-string-sentinel @ 393eb42: https://github.com/aram-devdocs/plumb/actions/runs/25236983201 — passed. This proves the live lint path no longer fails with DOMSnapshot response was malformed: negative string index -1.\n

@aram-devdocs aram-devdocs changed the title cdp: handle optional DOMSnapshot string sentinels fix(cdp): handle optional DOMSnapshot string sentinels May 1, 2026
@aram-devdocs
aram-devdocs merged commit b1d0639 into main May 1, 2026
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cdp: handle DOMSnapshot negative string index from live dogfood

1 participant