Skip to content

ci: use canonical dogfood URL - #194

Merged
aram-devdocs merged 1 commit into
mainfrom
issue-193-canonical-dogfood-url
May 2, 2026
Merged

ci: use canonical dogfood URL#194
aram-devdocs merged 1 commit into
mainfrom
issue-193-canonical-dogfood-url

Conversation

@aram-devdocs

Copy link
Copy Markdown
Owner

Spec

Closes #193.

Summary

Test plan

  • just check passes (fmt + clippy, no warnings)
  • just test passes on my machine
  • just determinism-check passes
  • cargo deny check passes
  • New tests added (or reason-not given)
  • Docs updated (docs/src/**, rustdoc, CHANGELOG if user-visible)
  • Humanizer skill run on any docs/src/** prose

Validation run in this environment:

  • bash tests/ci-canonical-docs-url-validate.sh
  • git diff --check
  • just validate was unavailable because just is not installed in this environment.

Files changed:

  • .github/workflows/dogfood.yml
  • .github/workflows/ci.yml
  • justfile
  • tests/ci-canonical-docs-url-validate.sh

Screenshots / terminal output

Not applicable.

Breaking change?

  • No
  • Yes — describe the migration path

Anything reviewers should double-check

  • The new validator intentionally scans active CI/release acceptance paths only, so historical non-canonical prose outside those paths can remain untouched.
  • just validate could not be run locally here because just is not installed; CI will exercise the wired path.

@github-actions

github-actions Bot commented May 2, 2026

Copy link
Copy Markdown
Contributor

I have everything I need. Here is the full review.


Review: PR #194 — ci: use canonical dogfood URL

Files touched: .github/workflows/ci.yml, .github/workflows/dogfood.yml, justfile, tests/ci-canonical-docs-url-validate.sh
Crate buckets: ci, justfile, shell tests (no Rust)

Also reviewed: merged fix(cdp) commit b1d0639 (fix(cdp): handle optional DOMSnapshot string sentinels #191), which is in HEAD and is the most substantive code change touching a library crate.


1. Determinism (PR #194)

No Rust code changes. Not applicable.

2. Determinism (fix(cdp) b1d0639)

crates/plumb-cdp/src/lib.rs:1035–1039lookup_optional_string is a pure function of (strings, idx). No wall-clock, no env, no HashMap in observable output (styles and attributes both use IndexMap). Clear.

3. Workspace layering

plumb-cdp imports plumb_core only. #![deny(unsafe_op_in_unsafe_fn)] is present at line 47; no new unsafe blocks introduced in either change. Layering intact.

4. Error handling

crates/plumb-cdp/src/lib.rs:1164–1165:

let value = lookup_optional_string(strings, *chunk[1].inner())?
    .unwrap_or("")

The ? propagates the CdpError; .unwrap_or("") operates on the resulting Option<&str>. This is Option::unwrap_or, not Result::unwrap; clippy::unwrap_used does not flag it. Semantically correct — an absent attribute value is represented as an empty string, matching Chrome's own behavior for present-but-empty attributes.

#![deny(clippy::unwrap_used, clippy::expect_used)] at line 47 is in force. All non-test unwrap calls in the file are unwrap_or/unwrap_or_else. The unwrap() calls at lines 1427–1478 are inside #[cfg(test)]. Compliant.

5. Test coverage (fix(cdp))

Three unit tests at lines 1452–1478 cover:

  • Sentinel (-1) and arbitrary negative → None
  • Valid in-range index → Some("…")
  • Out-of-range → Err(MalformedSnapshot)

That is the full decision surface of lookup_optional_string. Coverage is adequate.

6. Shell script issues (PR #194)

W1 — tests/ci-canonical-docs-url-validate.sh:74–76: dead self-exclusion filter

if [[ "$line" == *"tests/ci-canonical-docs-url-validate.sh:"* ]]; then
    continue
fi

The file on disk contains 'plumb\.dev' (backslash-dot, as a regex literal). rg searches for the pattern plumb\.dev, which matches the literal string plumb.dev (dot only). A backslash-dot in the file content is not plumb.dev, so rg will never produce a match line pointing at this script. The filter is dead code. Harmless, but it implies a self-reference risk that doesn't exist and will confuse future editors.

W2 — tests/ci-canonical-docs-url-validate.sh:69–81: silent rg fallback

The scan for remaining plumb.dev references does rg … || true. On ubuntu-latest, rg is pre-installed, so CI is unaffected. But just check is also a local command; a developer without ripgrep silently skips step 3 and gets a false green. A guard like command -v rg >/dev/null || { echo "ripgrep required (cargo install ripgrep)"; exit 1; } at the top would make the dependency explicit. Neither the other validator script (ci-chrome-sandbox-validate.sh) nor the justfile lists rg as a prerequisite.

These are warnings, not blockers.

7. Documentation

justfile:65 — new recipe has a one-line comment describing its purpose. Consistent with surrounding recipes. docs/src/ci/ was not changed, which is appropriate since this is internal CI wiring, not a user-visible feature.


Punch list

# File:Line Class Finding
W1 tests/ci-canonical-docs-url-validate.sh:74–76 Warning Dead self-exclusion filter — the file contains plumb\.dev (with backslash), not plumb.dev, so rg never matches the script itself; the filter is misleading dead code
W2 tests/ci-canonical-docs-url-validate.sh:69 Warning rg dependency is silently optional; local just check passes step 3 vacuously if rg is absent — add a command -v rg guard

No blockers found in either the PR #194 CI changes or the merged fix(cdp) lookup_optional_string implementation.


Verdict: APPROVE

@aram-devdocs
aram-devdocs merged commit 04f6047 into main May 2, 2026
14 of 15 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.

ci: use plumb.aramhammoudeh.com as canonical dogfood URL

1 participant