Skip to content

refactor(rust): dispose the whole-tree Rust audit corpus - #608

Merged
vicondoa merged 936 commits into
v3from
refactor-rust-skills-remediation
Sep 26, 2026
Merged

vicondoa merged 936 commits into
v3from
refactor-rust-skills-remediation

Conversation

@vicondoa

@vicondoa vicondoa commented Sep 26, 2026 •

Copy link
Copy Markdown
Owner

What this is

A repository-wide Rust audit of the whole tree, and the remediation of every
finding it produced. The work is a static-analysis sweep of the Rust workspace
across ten lenses (idiom, type, api, err, docs, perf, async, unsafe, test,
supply), applied as a series of small, separately-reasoned changes rather than
one sweeping rewrite, and recorded row by row in
docs/audits/2026-09-24-rust-skills-audit/.

Head: e1c7e85d576301e4e84cdcaf784555fd9b0ca043. Observed base: v3 at
133b78fc49c6ca101aaa573c87a5884adad74cb7.

Provider publisher key rotation

The head commit rotates the cloud hypervisor Provider publisher keypair. The
committed signature verified at 88f13fc1e, d3e58b328 and 0d2a5bc17 and
broke at 2ca9a22dd, which moved root-config.schema.json and updated the
manifest configDigest without re-signing, leaving the compiler refusing the
artifact. The previous private key is not recoverable, so the keypair is
rotated rather than re-derived.

  • packages/d2b-provider-guest-cloud-hypervisor/provider-manifest.json is
    byte-for-byte unchanged (blob OID identical on both sides of the commit).
  • publisher-public-key.pem is modified, not added: it carries the new SPKI
    Ed25519 public key.
  • provider-manifest.json.sig is modified, not added: a fresh raw 64-byte
    Ed25519 signature over the current manifest bytes.
  • No private-key path and no key material were ever committed. There is no
    private key in the tree at this head, none in any commit in this pull
    request's range, and no private-key location recorded anywhere in it. The
    operator holds the private half outside the repository, and the manifest,
    the sidecar and the public key are the only key-related bytes in the change.
  • No consumer in the tree pins the old publisher key; the host-integration
    suites mint their own ephemeral keypairs at test time.

Verified independently of the repository's own code with OpenSSL against the
committed bytes: openssl pkeyutl -verify -pubin -rawin over
provider-manifest.json with provider-manifest.json.sig and
publisher-public-key.pem returns Signature Verified Successfully; a
one-byte change to the manifest and the old public key against the new
signature both return Signature Verification Failure.

A hermetic test, committed_artifact_signature_verifies_over_the_committed_manifest
in packages/d2b-resource-compiler/src/lib.rs, now pins the pair in the
Layer-1 make check aggregate: it loads the trio from the Bazel runfiles tree,
asserts the committed signature verifies over the committed manifest through
the production verify_ed25519 path, and probes that a manifest or a signature
moving alone does not verify. The trio is declared as test data on both
unit-test targets in packages/d2b-resource-compiler/BUILD.bazel, so Bazel
re-runs the test when any of the three files changes. It ran and passed in
this head's check lane: test tests::committed_artifact_signature_verifies_over_the_committed_manifest ... ok
in the //packages/d2b-resource-compiler:d2b_resource_compiler_test target
log, 9 passed, 0 failed. The Nix build only asserted the sidecar was 64 bytes
long (nix/provider-artifact.nix:135), which is why the staleness shipped.

Scope of the change

1092 files changed, +40842 / -22925: 718 Rust sources across the workspace,
the Cargo.toml and BUILD.bazel for each crate, regenerated supply-chain
policy inputs, the deny.toml / clippy.toml / flake.nix policy surface,
166 new changelog.d/ fragments, and the regenerated reference schemas and
docs. Work landed in seven waves (U1-U3, W3-W7), each dispatched as parallel
slices, merged, and gated.

Relationship to the base

This branch merges onto a v3 that has just landed a separate unit-test audit
removing 197 redundant unit tests (base commit 133b78fc4, pull request #607).
That is why test counts read differently in different places across this
history, and it is expected rather than a regression: the two audits removed
different things from the same crates.

Validation

Four-lane gate, green on this head

Run on head e1c7e85d5 in the gates worktree at the pull request's standard
scan base 542920db8. All four lanes exited zero. Each lane's raw summary,
from its own log:

lane command rc raw summary
security-scan tests/tools/security-scan.sh 0 security-scan: clean (changed in-tree Rust lines since the merge base 542920db8eb94293734f0954c3ec01f863fc3d28)
census make check-census 0 crates censused: 94, blocking-census check: PASS (no crate above its committed baseline), 65 entry classes at or below baseline per crate
check make check 0 Executed 6 out of 1023 tests: 1023 tests pass
host-integration make test-host-integration 0 11 of 11 vmChecks PASS (bridge-isolation, daemon-smoke, device-worker-launch, guest-agent-cap-confinement, guest-shell-service, privilege-oracle, resource-operator-activation, runtime-cloud-hypervisor-guest-preflight, state-posture-contract, virtiofsd-volume-runtime, wayland-proxy)

The scan base is 542920db8 rather than the fixer's 5001d7b26: the
identifier-in-log redaction rule scans the added lines of the diff from the
merge base, so the wider base is the superset and the one this pull request
has used throughout.

Required checks

The required contexts on v3 are eval, check and security-scan. All
three pass on this head, read from the check runs recorded on
e1c7e85d576301e4e84cdcaf784555fd9b0ca043 itself:

context conclusion job
eval success 36247557598/108419457816
check success 36247557668/108422373294
security-scan success 36247557668/108419458217

check is the aggregate job in pr-l1-static-fast.yml and reported only once
its eleven upstream suites finished; it turned green at 14:28:44Z on this head.
Of the eighteen contexts on this head, seventeen pass. The one exception is
not a required context and is recorded below.

Known residuals

These are known and deliberately left. Each is stated with what was actually
measured.

  1. A non-required CodeQL check is red. CodeQL reports one critical
    alert, packages/d2b-core-controller/src/authority_persistence.rs:375,
    "This hard-coded value is used as a nonce." It is a false positive,
    verified at this head: the #[cfg(test)] mod tests block begins at line
    337, and the literal 7 at line 375 is the nonce argument of
    PreparedAuthorityOperation::new used to build deterministic test data. The
    production path mints its own non-zero value. CodeQL is not a required
    context, so it does not block the merge; it is recorded here rather than
    suppressed.
  2. changelog-fold is a merge-time step, not a pull-request gate. This
    branch does not modify CHANGELOG.md, so the pre-existing fold-time
    defects in the changelog history recorded in earlier rounds are untouched
    by it. The one fragment this branch adds is structurally validated by
    //packages/xtask:policy_changelog_gate, which passes in this head's
    check lane.
  3. Two lock-worker refusals drop the originating error's detail.
    packages/d2b-broker/src/ops/host_generation_handoff.rs:265 and :268 map
    the try_send and oneshot-receive failures onto generic io::Error
    strings ("handoff lock worker busy" / "handoff lock worker unavailable").
    The refusal is forwarded, but the originating variant - a full queue
    versus a closed one, or a worker that dropped the reply - is not carried
    with it, so the two failure modes are indistinguishable to a caller. This
    is a diagnosability gap, not a correctness defect, and it is unchanged at
    this head.
  4. Wave-close evidence is uneven between waves. Wave 3 has a green
    four-lane gate of its own, at d68d0dbc (an ancestor of this head); an
    earlier wave-3 gate run at d322db6c8 was red and was superseded by it.
    Wave 7's close record carries a five-command preflight rather than a
    four-lane gate, and records its wave-gate cells as not run, the four-lane
    gate being run centrally. Both wave close records live in gitignored
    scratch directories and are therefore not part of the committed tree; the
    evidence for the current head is the gate run in the Validation section
    above.

Review

The change is reviewed in two slices, each by a separate review in its own
clean context, bound to head e1c7e85d5 against base 133b78fc4:

  • the docs, changelog and plan slice (195 files: 29 under docs/, 166 under
    changelog.d/)
  • the packages, policy and build-tooling slice (897 files)

Each review's binding table, findings and verdict are recorded as a comment
on this pull request, together with a reconciliation of every candidate
against the committed head. Neither record is reusable after the head moves.

The packages, policy and build-tooling slice returned approve with no
finding. The docs, changelog and plan slice returned request-changes with
four findings, all documentation-only and all verified against this head:

  1. docs/specs/providers/ADR-046-provider-transport-azure-relay.md:1369-1371
    (and :1464, :1465) still name the xtask gen-provider-transport-schemas
    subcommand and the Rust type AzureRelayTransportSettings. Neither exists;
    the same file already says at :198-203 that the schema is hand-authored,
    so the file contradicts itself.
  2. docs/plans/2026-09-24-002-refactor-rust-skills-remediation-plan.md:151
    names three source paths that do not resolve; the committed tree carries
    them one directory up, and under wayland_proxy/.
  3. changelog.d/fix-provider-artifact-signature.md:13-15 states that the new
    key's location and public fingerprint are recorded in the fragment, and
    records neither. The fragment exposes no key material either way; the
    defect is that the clause describes a record that is not there and the
    fold copies it verbatim into the released changelog.
  4. docs/plans/2026-09-24-002-refactor-rust-skills-remediation-plan.md:71
    defers "the retired labs/ tree"; the head carries 171 tracked files
    there and the repository treats it as live.

None is acted on at this head: a fix moves the head and would invalidate both
review records, so the pull request is not merge-ready until these are
fixed and the head is re-gated and re-reviewed.

owner_key_for listed the entire Zone row set and linear-searched for the
owner uid on every Delete and owner-less update. The manager already
maintains a uid-to-key index (by_uid); expose it as a KeyForUid RPC on
ResourceManagerClient and resolve owners through it instead.
…macro

- Add From<&EndpointPolicy> for HandshakeOffer and use it at all seven
  HandshakeOffer::from(policy.clone()) sites (component_session, session
  admission/engine/handshake); the by-value impl stays for owned policies.
- Add From<&ResourceEnvelope> for ResourceRef in d2b-contracts-resource and
  use it in ResourceExportSpec::validate_target instead of rebuilding the
  ref from cloned parts.
- Consolidate 28 hand-written Wire-struct Deserialize impls behind a shared
  wire_deserialize! macro in d2b-contracts-zone-session; wire shapes,
  defaults, and constructor validation gates are unchanged.
validate_unique_ids and validate_lock_order now return StorageValidationError and SyncValidationError carrying the wire reason and offending id, replacing the String messages that storage_lifecycle re-derived by string-prefix matching. The classifier functions are deleted; the provider-volume-local re-export and its classifier test are dropped.
Mutating-verb response projection now parses the wire outcome into the
typed MutatingVerbOutcome vocabulary (new response_outcome_typed accessor)
and retarget_mutating_response matches enum variants instead of raw
strings, so unknown outcomes fail closed.

EstablishedShell hides its Arc<dyn ShellBackend> behind delegating
handle_op/close_attachment/cancel_attachment methods; the best_effort_*
audit helpers take the shell instead of the trait object.

The public read model returns Arc<CachedPublicFrame> from load_list and
load_status instead of cloning the whole frame per poll; the wire response
clones only at the response boundary.

The console drainer runtime static is gone: drainer tasks spawn on the
daemon-owned tokio runtime handle passed into the console session
creation functions.
The catalog view emitted the committed authz facets as string literals
into BrokerAuthzFacets while the sibling authz view emitted the same
declared data as typed enums. Emit SecretAccess/BrokerRequirement/AuditMode
from the catalog view too, type the BrokerAuthzFacets fields to those
enums (re-exported from d2b-core privileges), and align the hand-written
test row that drifted case (audit_mode "yes" vs "Yes"). The d2b-core
privilege enums gain Copy so the Copy-bearing row structs keep their
derives.
…rt doubles

The Guest, User, and VolumeBinding test-support doubles each hand-rolled
the same recorder shape (a Mutex-guarded Vec<String> call log plus a
snapshot accessor) instead of reusing the toolkit's shipped testing
module. Ship the shape once as testing::SharedLog and delegate the
family recorders onto it; the doubles' public APIs are unchanged.
- Import ZoneId in the security-key, usbip, and network-local driver test
  modules where the driver-args constructors now parse zones.
- d2bd: replace the ? on Result key_ref inside map_err closures with the
  already-computed device reference (reconcile_tpm) or a hoisted canonical
  string (finalize_gpu), since those closures do not return Result.
CredentialRuntime::dependency_facts now returns
Result<Option<CredentialDependencyFacts>, CredentialResourceRuntimeError>
instead of Option: a manager RPC failure reaches the driver as Err
(DependencyFacts) rather than being logged and answered as absence at the
d2bd trait boundary. Ok(None) stays the honest not-committed answer, and
the driver fails closed on both. The d2bd log-and-absent workaround is
removed; the daemon error detail stays at debug level.
…tion

# Conflicts:
#	docs/audits/2026-09-24-rust-skills-audit/ledger.md
…tion arm

The leading comment on the custom Deserialize for AzureVmRecoveryState
claimed the legacy fold is total because the write side always sets or
clears both values together. Seventy lines below, in the same match, the
branch-introduced comment and the Err arm it describes refuse a half-Some
pair with a typed serde error. Two comments in one function stated
opposite contracts for the same arm, both added by this branch.

The claim is also false as written: the total-fold sentence reads as a
property of the decode, but the write side is only one of the two ways a
record reaches it. The other is a record read back off disk, which the
fold is now explicitly told can be malformed. A reader reasoning from
the header alone concludes the Err arm is unreachable, treats a decode
failure there as an impossible state, and widens the arm back to the
total fold this branch removed a panic to break.

The sentence is rescoped to the write side, which does hold, and the
refusal is stated where the reader will meet it. Only comment text
changes; no code, no schema, no generated artifact, no test.
`modern_run` resolves the Zone before it dispatches a command, and
`ZoneContext::discover` refuses a command whose public socket does not
answer. `host validate`, `host reconcile` and `host doctor` are built with
a local-only context and skip that probe; `host prepare` and `host destroy`
do not, so their missing-mode refusal - a property of the invocation - was
reported as `zone-unavailable` at exit 1 instead of the documented
`--apply-or-dry-run-required` envelope at exit 78 whenever d2bd was not
listening. The verbs that carry a mode keep their transport refusal, and
an explicit `--zone` keeps its routing.

The rule now lives in one place, `host::missing_mutation_mode`, which
`modern_run` consults before it builds a context; the per-verb copies in
`mutation`, `reconcile` and `validate` are gone.
The legacy decode arm refuses a record whose operation and start stamp
are only half present, and nothing exercised it. The paired and
unpaired records of the same shape decode, so the two controls tie the
refusal to the half pair rather than to the record. The enclosing
`Repr` is untagged, so the refusal is asserted on the error itself.
`admit_authority` refuses with `StartupRehydrationRequired` until the
startup barrier completes, and no test reached that guard. Its only
former coverage went with the external-NIC authority surface, while the
guard itself stayed on the generic admission path the controller shares
the index into, so a caller outside the startup path reached it
untested. Distinct from the process-level stage barrier in `main`.
The escape scan ends at the closing brace of the declaring
`SeedProvider` block instead of running to the end of the file, and
nothing distinguished the two windows. A row written after the block
belongs to no provider of its own and is no longer attributed to the
preceding one, while a later block is still scanned in its own right.
@vicondoa

Copy link
Copy Markdown
Owner Author

Review - slice 1 of 2: docs, changelog and plan

Bound to head 7b611cbc6 against base 133b78fc4. 195 files under docs/
and changelog.d/. Reviewed in a clean context against the committed head.

Verdict: request-changes. Two contract-surface defects in regenerated
reference material. Both are verified below against the head checkout.

Finding 1 (blocking) - the v2 storage-lifecycle schema contradicts the bytes the daemon writes

docs/reference/schemas/v2/storage-lifecycle-report.json:88

This branch replaced the per-field #[serde(rename = ...)] attributes on
StorageLifecycleIssue with rename_all_fields = "camelCase"
(packages/d2b-core/src/storage_lifecycle.rs:46). Serde honours that
attribute, so the daemon still writes bundleVersion, contractId,
offendingId and roleId on the wire - the in-source test
issue_variant_fields_serialize_with_schema_casing
(packages/d2b-core/src/storage_lifecycle.rs:236) pins exactly those keys.

The schema is produced by schemars::schema_for!(StorageLifecycleReport)
(packages/xtask/src/main.rs:717). schemars 0.8.22 does not read
rename_all_fields, so the regenerated file now requires bundle_version
(line 89) and declares snake_case properties.

Consequence: a consumer validating a real storage-lifecycle-report.json
against the published v2 schema fails on every degraded issue that carries
extra fields. Five of the seven issue variants are affected
(legacy-bundle-contracts-unavailable, storage-contract-invalid,
sync-contract-invalid, missing-restart-policy,
adoptable-missing-cgroup-leaf).

The drift gate does not catch this, because the committed file does equal fresh
generator output. The artifact whose job is to keep schema and type in sync is
the artifact that drifted. The fix is either to keep explicit per-field
rename attributes that schemars understands, or to move the field casing out
of the enum so the generated schema and the wire agree.

Finding 2 (major) - the daemon API table documents an in-memory field that never reaches the wire

docs/reference/daemon-api.md:386

The regenerated table now lists AuditResponse as
struct { entries; page_end: AuditPageEnd }. That is the in-memory shape. At
head, AuditResponse carries a hand-written Serialize
(packages/d2b-contracts-control/src/public_wire.rs:2389) that writes only
entries, nextCursor and complete. Its JsonSchema impl
(packages/d2b-contracts-control/src/public_wire.rs:2411) deliberately
delegates to AuditResponseWire, so the published schema correctly keeps the
flat wire keys - only this table row is wrong.

The next row, ExportBrokerAuditResponse, still shows next_cursor and
complete, and that type really does serialize flat, so two responses with
identical wire JSON are documented two different ways in one table.

Consequence: a reader of the reference writes a client that looks for pageEnd
and never finds it, while nextCursor and complete appear nowhere in that
row, even though protocol v5 has always carried them. Documentation-only fix.

What this slice checked and cleared

  • All 165 changelog.d/ fragments this branch adds parse cleanly under the
    repository's own fragment rules, and the added set is disjoint from the
    malformed fragments already in the tree. No changelog defect in the tree is
    attributable to this branch.
  • The "Audio mutation refusals" section of docs/reference/error-codes.md
    matches the error kinds, exit codes and operation names at head.
  • Every other regenerated schema under docs/reference/ was checked field by
    field against the Rust types at head, including the closed enums, the
    azure-relay identifier patterns, and the audit export entry rule. Those
    agree.
  • The 7 ADR-046 spec files: every added or changed claim checks out against
    head.
  • All relative markdown links in the changed files resolve; no file added by
    this branch references a path removed at head.

Coverage limits

No generator, gate or test suite was executed, so this review does not state
whether the drift gate currently passes. The mechanical-retyping commits in
this branch were not read line by line.

@vicondoa

Copy link
Copy Markdown
Owner Author

Review - slice 2 of 2: packages, policy and build tooling

Bound to head 7b611cbc6 against base 133b78fc4. 892 files, +37406 /
-22572. Reviewed in a clean context against the committed head.

Verdict: approve-with-comments. No functional defect, no gate turned green
by loosening a lint, and no tautological test. One stale baseline row is
recorded as a minor finding.

Finding (minor) - a census baseline row is one unit looser than the tree justifies

packages/xtask/data/blocking-census-baseline.json:2143

This branch raises tokio::runtime::Runtime::block_on from 0 to 1 for the
packages/d2b-provider-device block. That crate has no such call site: a
recursive search of every file under packages/d2b-provider-device returns
zero block_on occurrences at head, at the base, and at 8c3c48c0c, the
branch commit that recorded the row. That commit's message attributes the row
to tests/device_family.rs:179; the file has no block_on at any of those
revisions and line 179 sits inside a different test.

check_against_baseline (packages/xtask/src/blocking_census.rs:1129) raises
a violation only when the observed count exceeds the committed count, so a
stale-high row can never fail the lane.

Consequence: the cap for that crate silently allows one
Runtime::block_on that nothing in the tree justifies, and no record explains
why the headroom was granted. Fix: set the row back to 0, or record the site
the cap is paying for.

What this slice checked and cleared

  • No gate was bought by loosening one. deny.toml changed only the
    confidence threshold (0.8 to 0.9, a tightening); the licence allow list and
    the ban and source sections are unchanged. clippy.toml renamed three method
    paths, with the entry count identical before and after, and the stated
    rationale matches the real crate: the renamed parking_lot mutex and rwlock
    are aliases over lock_api, while Condvar is a real type, which is why it
    is carved out explicitly. That is a tightening that repairs a dead ban.
  • The census baseline is otherwise honest. Across all 94 crates the only
    net value changes are this finding and one packages/xtask
    std::fs::read_to_string row raised 11 to 13, which matches the exact
    occurrence count in that crate at head.
  • The async-gate inventory shrank from 251 to 195 sites, consistent with
    the branch converting blocking locks. The one in-scope allow marker not
    recorded in the inventory exists identically at the base, so it is a
    pre-existing scanner characteristic, not drift introduced here.
  • Golden files match real output. Re-deriving the output from the command
    set at head yields exactly the 32 names in both
    tests/golden/cli-output/auth-status-human.golden and
    auth-status-json.golden, in the expected order, with the role, effective
    uid, socket rows and the single denied row all matching.
  • Supply-chain inputs lose nothing. Comparing the package name sets of all
    18 changed closure files between base and head, zero entries are dropped. The
    one addition is mirrored consistently across flake.nix and the guest
    workspace fixture.
  • The cloud-hypervisor manifest digest is fresh, recomputed locally at head
    and equal to sha256(root-config.schema.json). nixos-modules/privileges-json.nix
    now uses the correct wire form for the destructive field.
  • The three commits on top of the branch are not tautological. Each drives
    real production APIs and asserts a specific refusal: the startup barrier test
    asserts the rehydration-required refusal together with an effects counter of
    zero, and the legacy recovery decode test carries two positive controls (the
    paired and the unpaired record both decode) that tie the refusal to the
    half-pair specifically rather than to the record shape. Every API those tests
    call was confirmed to exist at head.
  • Targeted sweeps over the full diff found no added unsafe, and 955
    assertions added against 268 removed, the removals concentrated in deleted
    dead code. The highest-risk behaviour commits were read in full: the host
    mutation-mode refusal ordering preserves its exit code and envelope and is
    exhaustive over the host command set, the per-zone lock scoping releases its
    guards before the returned handle is used, and the broker leg budget uses a
    const fn duration constructor.
  • Shell completions list exactly the head value-enum variants.

On the one red check

The CodeQL check on this head reports a critical alert at
packages/d2b-core-controller/src/authority_persistence.rs:375. This review
independently confirms it is a false positive: #[cfg(test)] mod tests begins
at line 337, so line 375 is inside it, and the literal 7 is passed as the
nonce argument of PreparedAuthorityOperation::new to build a test fixture.

One point of correction against a claim in the pull request description: the
manifest itself carries no signature field, but the signature is a detached
sidecar that is shipped and verified. packages/d2b-resource-compiler reads
share/d2b/provider/provider-manifest.json.sig and verifies it as an Ed25519
signature over the manifest bytes
(packages/d2b-resource-compiler/src/lib.rs:1418). This branch changes
provider-manifest.json and does not change the sidecar, so the detached
signature over the manifest bytes is stale. The configDigest inside the
manifest is fresh, as this review confirms; it is the signature over the
enclosing file that no longer matches.

Coverage limits

No project-wide suite was run, so this review does not confirm that the
census, async-gate, clippy, cargo-deny and Bazel lanes are green at this head -
that comes from the gate run recorded in the pull request description. The
census tool's observed counts for the other 92 crates were not recomputed, only
the diff. The new readlink -f call in the Bazel generated script was not
verified to resolve inside the test sandbox. The roughly forty commits that
mechanically retyped enums and structs were not read line by line.

`StorageLifecycleIssue` named its variant fields with serde's container-level
`rename_all_fields = "camelCase"`. schemars 0.8.22 reads `rename_all` on a
variant, where it renames that variant's fields, but not `rename_all_fields`,
so the generated schema required snake_case keys: `bundle_version`,
`contract_id`, `offending_id`, and `role_id`. A consumer validating a real
report against the committed schema rejected five of the eight issue variants,
and the drift gate could not see it because the committed file equalled fresh
generator output.

Every struct variant now repeats the field casing as a `schemars` attribute,
leaving serde's wire bytes untouched, and the schema is regenerated through
`make generate`.

A new contract test serializes one report carrying every issue variant and
validates the bytes against the committed schema instead of against a fresh
generator run, so the artifact and the bytes are compared to each other. It
refuses a keyword it does not interpret, so a schema that grows one fails
rather than passing unvalidated. Against the pre-fix schema the test fails on
the first variant that carries a renamed field.
`AuditResponse` holds `entries` and a `page_end`, and its hand-written
`Serialize` projects that into the flat `entries`, `nextCursor`, and `complete`
keys the protocol has always carried. The response tables read the struct's
in-memory fields, so the published row documented a `page_end: AuditPageEnd`
member no consumer ever receives.

The row now follows the type's hand-written `JsonSchema`, which delegates to
the wire struct that publishes the same three keys. A type whose schema
delegates is documented from the shape it publishes; every other row still
renders its own fields, and the regenerated table differs on that row alone.
…on row

The baseline raises `tokio::runtime::Runtime::block_on` from 0 to 1 for
`packages/d2b-provider-device`, and a text search of that crate finds no
`block_on` at all: a source grep cannot see a `#[tokio::test]` expansion.

The census decides the question. Run in its write mode on a clean tree it still
reports 1 for that crate, and the crate holds exactly one `#[tokio::test]`, at
`tests/device_family.rs:157`. The row is therefore a real counted site, not
stale headroom, and it stays. Only the recorded site was wrong: it named
`tests/device_family.rs:179`, the closing line of an assertion inside the test
and not the attribute the expansion comes from.
@vicondoa

Copy link
Copy Markdown
Owner Author

Review - slice 1 of 2: docs, changelog and plan

Independent ce-code-review mode:agent pass over this PR's own diff, in a
clean context, bound to the head below. Slice A is the 194 files under docs/
and changelog.d/.

Bindings

field value
repository vicondoa/d2b
PR 608
observed base ref v3
observed base OID 133b78fc49c6ca101aaa573c87a5884adad74cb7
reviewed head OID 2317e1980d0c35d6469f544e30599ab10440dea2
verdict request-changes

This evidence binds the head OID above and is not reusable after the head
changes. It supersedes the slice 1 record bound to 7b611cbc6.

Verdict: request-changes. Three of the four reported findings were
re-verified against the committed head and stand. One is refuted below and
needs no fix. The three head-moving commits are otherwise correct in this
slice: the regenerated v2 storage-lifecycle schema matches the casing the
daemon writes, the regenerated audit-response row matches the delegating wire
type, and every one of the 165 changelog fragments parses under the repository's
own fragment grammar.

Finding 1 (blocking) - a release-bound fragment ends with an internal plan marker

changelog.d/w3-38-d2bd-conc-async.md:3

The entry's final parenthetical names an internal planning unit.
docs/contributing/changelog-and-commits.md:44 requires that internal planning
identifiers not appear in these fragments. The repository's plan states the same
rule as a close-out condition at
docs/plans/2026-09-24-002-refactor-rust-skills-remediation-plan.md:323: no
finding identifier may reach source, doc comments, commits, changelog fragments,
or the pull request body.

Refinement to the reviewer's supporting detail: the fragment's unit label is not
unique to this file. Three other shipped plans also define a unit with the same
label, so the parenthetical is ambiguous rather than dangling. The defect is the
marker itself, not an unresolvable reference. Fix: delete the parenthetical.

Finding 2 (blocking) - the plan still points at a removed corpus

docs/plans/2026-09-24-002-refactor-rust-skills-remediation-plan.md:331

The wave-to-cluster appendix derives its counts from a README.md section 6 in
a corpus directory that is not in the tree at this head. Commit 564bd6166
removed that directory and states in its own message that it also "updates the
plan that pointed at them, so the committed tree carries no reference to a path
it no longer has". This line survived that cleanup, so the appendix counts
cannot be checked from the repository at all. Fix: restate the counts as the
plan's own record, or drop the pointer.

Finding 3 (minor) - a Fixed entry documents a reconciliation, not a change

changelog.d/refactor-rust-skills-remediation.md:8

The line added by 2317e1980 states that the row's recorded call site named a
line inside the test. The census baseline stores per-crate counts only; a search
of every committed non-documentation file for a device_family.rs:<line> record
returns nothing. The only occurrences of that record anywhere on this checkout
are in gitignored build output under ./target/debug/.fingerprint/ and in
gitignored scratch. There was therefore no committed record to correct, and a
reader auditing the row cannot reproduce the claim. The entry also does not
mention the substantive change in that row, which moved from 0 to 1 in this PR.

Fix: state that the count is confirmed by the crate's single #[tokio::test]
at packages/d2b-provider-device/tests/device_family.rs:157, and that the base
carried an allow on that same function which this branch removed. The sibling
slice records the same defect against the same line.

Refuted - the appendix arithmetic reconciles exactly

docs/plans/2026-09-24-002-refactor-rust-skills-remediation-plan.md:344

The review reported that the wave rows sum to 978, or 961 excluding the
recorded no-op row, against a stated total of 965. This does not hold. The
rows sum to 965 exactly:

  • U2 through U6 total 923 (368 + 298 + 167 + 70 + 20), matching the plan's own
    summary at line 27: "923 actionable findings".
  • U7 is 25, matching line 27's "25 needs-contract findings".
  • recorded no-op is 17, matching line 27's "17 policy-confirmed findings".
  • 923 + 25 + 17 = 965, which is the stated total on line 344 and the goal
    capsule's figure on line 14.

The 978 figure comes from counting the U1 row's "13, a subset" as 13 additional
findings. That cell is annotated as a subset precisely because those 13
high-severity rows are already inside the 923 owned by U2 through U6; they are
executed first as their own wave, not added to the total. U8 is the close-out
wave and correctly contributes 0. No fix is required, and the appendix is
internally consistent in two independent ways.

Coverage

Examined all 29 docs/ and 165 changelog.d/ files at this head. Re-parsed
every fragment against the repository's own fragment parser; scanned for
prohibited dash code points, non-ASCII introductions, broken relative links
(345, all resolve), and marker leakage. Reimplemented the daemon-API generator's
row selection over the head sources and compared all eight sections, verifying
all 273 line anchors and all 126 struct shape columns. Validated all 16 changed
JSON fixtures for parse and $ref integrity and traced every changed enum,
$ref, and pattern back to its Rust type or serde attribute. Read the plan in
full and checked its arithmetic and every path it cites. The reviewer also
recorded five candidate defects it disproved against the head, including a
missing error row that the generator's own predicate explains and a dangling
ADR reference that resolves.

Not examined: the accuracy of every factual claim inside the 165 fragment bodies
(grammar and content rules were checked exhaustively; factual claims were
spot-verified on roughly a dozen high-risk entries); packages/, bazel/,
tests/, completions/, and all code, which the sibling slice owns; and
whether the generators themselves emit byte-identical output. No formatter,
linter, build, or test suite was run.

@vicondoa

Copy link
Copy Markdown
Owner Author

Review - slice 2 of 2: packages, policy and build tooling

Independent ce-code-review mode:agent pass over this PR's own diff, in a
clean context, bound to the head below. Slice B is the 893 remaining files:
872 under packages/, plus bazel/checks/nix/defs.bzl, Cargo.lock,
clippy.toml, deny.toml, flake.nix, completions/, labs/,
nixos-modules/, and tests/.

Bindings

field value
repository vicondoa/d2b
PR 608
observed base ref v3
observed base OID 133b78fc49c6ca101aaa573c87a5884adad74cb7
reviewed head OID 2317e1980d0c35d6469f544e30599ab10440dea2
verdict request-changes

This evidence binds the head OID above and is not reusable after the head
changes. It supersedes the slice 2 record bound to 7b611cbc6.

Verdict: request-changes. Four reported findings were re-verified against
the committed head and stand; a fifth instance of the same defect class as
finding 2 is added here because neither pass reported it. None of the three
head-moving commits is refuted on its code claims: the storage-lifecycle casing
and its schema test were independently reproduced, including a mutation proof,
and the daemon-API generator's delegating-type row was traced to its source.

Finding 1 (blocking) - auth status reports retired commands for the none role

packages/d2b/src/dispatch.rs:705-712

The AuthRoleV2::None branch of allowed_subcommands hardcodes
realm list and realm inspect, while packages/d2b/src/dispatch.rs:1167
asserts the parser rejects ["d2b", "realm", "list"]. The Admin and Launcher
branches are derived from all_known_subcommands(), so they can never contain a
retired verb. The result is that one d2b auth status response contradicts
itself: role none is told realm list and realm inspect are allowed, while
role launcher is not told that.

This branch is the one the branch's own cleanup missed. The diff removes
realm list, realm inspect, realm enter, and realm run from the
neighbouring allowed-subcommand surface and adds a projection filter in their
place, and adds the parser assertion above, but leaves this literal list intact.
packages/d2b/tests/auth_status_contract.rs:142-153 pins the stale entries, so
the drift cannot be caught by the suite.

The daemon half is unreconciled with the CLI half of the same response:
packages/d2bd/src/composition.rs:23274-23293 still reports realm list,
realm inspect, realm enter, and realm run for both roles, and this branch
does not touch it.

Fix: derive the None list from the same parser-derived source as the sibling
branches, keeping list, status, auth status, and op inspect; update the
expected vector in the contract test; and reconcile the daemon half in the same
change so both halves of the response agree.

Finding 2 (blocking) - an audit corpus name and review date sit in shipped config

deny.toml:4

The first comment line above the cluster inventory names an audit corpus and
carries a dated review marker. AGENTS.md:290-291 forbids leaking revision,
follow-up, or finding markers into shipped source. The named corpus is also not
in the tree at this head, so the comment points at a path a reader cannot
resolve. This is the only committed non-documentation file that names a corpus;
it was found by a repository-wide search excluding docs/ and third_party/.

Fix: keep the cluster inventory, which is accurate and useful, and drop the
corpus name and the review date from that comment line.

Finding 3 (minor) - a changelog entry documents a correction to a record that does not exist

changelog.d/refactor-rust-skills-remediation.md:8

The census baseline is built from (crate_dir, counts) in
packages/xtask/src/blocking_census.rs:1184-1330; no artifact under
packages/xtask/data/ carries a .rs:line key, and a search of every committed
non-documentation file for such a record returns nothing. The only occurrences on
this checkout are in gitignored build output and gitignored scratch. The line
therefore claims a fix to an artifact that was never committed, and it does not
mention the substantive change in that row, which moved from 0 to 1 in this PR.

The census half of 2317e1980 is correct and was verified independently: the
baseline row is 1, the crate holds exactly one #[tokio::test], at
packages/d2b-provider-device/tests/device_family.rs:157, and at the base that
same function carried an allow on the disallowed method which this branch
removed. That removal is why the count rose.

Fix: state that the row's count is confirmed by that attribute rather than by a
corrected record. The sibling slice records the same defect against the same
line; one fix closes both.

Finding 4 (minor) - a doc comment names deny-list entries this branch replaced

packages/xtask/src/async_gate.rs:296-301

The comment on lock_method_names still points at the std::sync and
parking_lot Mutex::lock, RwLock::read, RwLock::write entries, which this
branch replaced with the resolved lock_api paths at clippy.toml:89-91. A
maintainer following the comment looks for entries the deny list no longer
contains. Behaviour is unaffected: the derived method names are still lock,
read, and write. Fix: name the lock_api entries.

Finding 5 (minor) - five more plan markers introduced into shipped files

Added by this pass; neither slice reported these.

clippy.toml:89, clippy.toml:90, clippy.toml:91,
packages/d2b-broker/src/ops/host_generation_handoff.rs:279,
packages/d2b-broker/src/sys.rs:1968

This branch adds five parenthetical internal plan markers to shipped source and
config, in the same class as finding 2. AGENTS.md:290-291 applies, as does the
plan's own close-out condition at
docs/plans/2026-09-24-002-refactor-rust-skills-remediation-plan.md:323. The
total marker count is unchanged by this branch, which removes six elsewhere, so
this is not a regression in aggregate; these five are new instances of a rule the
branch is meant to leave clean, and three of them sit in the very file finding 4
cites as stale. Fix: drop the parentheticals, keeping each reason's technical
content.

Refuted during this pass

Recorded because each looked like a defect from a text search and each was
disproved against the committed head, the failure mode this slice is most
exposed to.

  • An async-gate-allow marker on a .lock() that the committed inventory does
    not record, which would fail the gate. Refuted. The scanner classifies the
    enclosing body as sync because a semicolon inside the parameter list clears
    its segment state before the async fn pair is seen, so the call is neither a
    violation nor an inventory obligation. A port of the gate's own recogniser over
    its 908-file default scan set found every honored site recorded, and the nine
    recorded-but-not-honored entries are all pre-existing.
  • Two new shared-family-knowledge exemption rows that look dead, because the
    module contains neither token. Refuted. The module does produce both
    signals on a format string where the token is glued by hyphens, and a dead row
    would fail the staleness check.
  • The deny-list rename from parking_lot to lock_api could have weakened the
    lock ban. Not a weakening. Diffing both revisions per crate per entry, the
    only substantive count changes in the whole PR are the device crate's async
    entry 0 to 1 and xtask's file-read entry 11 to 13. Every one of the 94 crates
    is 0 for both the old and the new lock key, so no live parking-lot lock use
    exists at this head either way.
  • The supply-chain config's confidence threshold change could have loosened
    license acceptance. Refuted. It tightens it; the version and registry
    policies are untouched, and the added cluster inventory is prose.
  • A feature split on the credential backend's test constructors could have broken
    a cross-crate consumer. Refuted. The only such caller is in dev-dependencies
    that enable the feature, the in-crate callers sit inside a test module, and the
    build side keeps the split.

Coverage

Read in full or cross-checked mechanically: the three head-moving commits plus
the two commits that changed the deny list; packages/xtask/data/*.json in full,
with both baseline revisions parsed and differenced per crate per entry, all 195
recorded async-gate sites resolved to their source lines, and a port of the
gate's own recogniser run over its default scan set; clippy.toml, deny.toml,
bazel/checks/nix/defs.bzl, the three completions, nixos-modules/, and
flake.nix; xtask's census, async-gate, provider-crate-policy, main and delivery
modules; the storage-lifecycle module with its new schema test and a
reimplemented-validator mutation proof; the provider-toolkit feature split; the
auth-status surface with its two goldens and its contract test; and the policy
input locks, where every host-side lock is an exact subset of the root lock while
the four guest-static locks are not, as a separate lock root explains.

Not examined: roughly 830 of the packages/ files, read as diff hunks and
spot-checked by change class rather than line by line, since the change is
almost entirely doc comments, iterator and From simplifications, dead-code
removal, and deny_unknown_fields additions; twelve xtask generator modules
beyond those cited; labs/window-chrome/proxy/; the broker beyond its async-gate
sites; and the Nix closure outputs, which cannot be validated without running
the closure. Clippy was not executed, so the claim that lock_api::Mutex::lock
is the form clippy resolves a parking_lot::Mutex::lock call to rests on the
commit message; as noted, the rename is effect-neutral at this head. No
formatter, linter, build, or test suite was run.

Internal planning identifiers had leaked into shipped source, config and
changelog fragments, and one plan appendix still pointed at a corpus
directory that commit 564bd61 removed, so its counts could not be
checked from the tree at all.

The markers carry no information a reader needs: each reason's technical
content stays and only the unit label goes. The plan's appendix now
records its own counts instead of citing a path the tree no longer has,
and the census fragment states why the device Provider's `block_on` cap
is 1 - the single async test expands to that call and the base's
suppressive allow is gone - rather than claiming to correct a call-site
record the baseline never held.

The async-gate doc comment now names the `lock_api` deny-list entries
this branch put in place of the `parking_lot` aliases, and deny.toml
keeps its duplicate-cluster inventory without naming the audit corpus or
carrying a review date.
The `none` role's allowed list was a hand-typed literal naming `realm
list` and `realm inspect`, which this branch's parser no longer accepts
- the same response told a caller with no role that two retired verbs
were allowed while telling a launcher they were not. The daemon half
reported those verbs plus `realm enter`, `realm run` and `host check`,
none of which the parser declares either, and nothing covered either
list, so the drift could not be caught.

Both halves now read one shared read-only list of command paths, and a
test walks the parser's own command tree to prove every name each half
reports resolves to a declared subcommand. A retired verb reintroduced
on either side fails that test.

The daemon API reference and the async-gate inventory are regenerated
through their owning tools, so their line anchors follow the moved
types and marker sites.
@vicondoa

Copy link
Copy Markdown
Owner Author

Review - slice 1 of 2: docs, changelog and plan

Bindings

field value
repository vicondoa/d2b
PR 608
observed base ref v3
observed base OID 133b78fc49c6ca101aaa573c87a5884adad74cb7
reviewed head OID 5001d7b269eade9fc6e29a271f5ba2200b40f93d
verdict approve-with-comments

This evidence binds the head OID above and is not reusable after the head changes. It supersedes the slice 1 record bound to head 2317e1980 and, further back, the slice 1 record bound to head 7b611cbc6.

Verdict: approve-with-comments.

Scope is exactly git diff --name-only 133b78fc4 5001d7b26 -- docs changelog.d: 194 files, 29 under docs/ and 165 under changelog.d/. At head 7b611cbc6 the same command returned 195; the one that left the diff is docs/reference/schemas/v2/storage-lifecycle-report.json, which 6cbcf185b restored to the base's exact blob, so it is no longer a changed file. The pull request body's "195 files" is therefore correct for the head that body names and one high for this one. That body's Review section also still reads "Three findings are open against this head ... none acted on" against 7b611cbc6; all three are fixed on this head (the storage-lifecycle schema, the audit-response row, and the census row's justification), and the two superseding lines above replace that section's record.

Prior items re-derived at this head

The three findings reported at head 2317e1980, and the one item that round refuted, were re-derived from scratch against this head. No conclusion from that round was carried forward.

  1. The Fixed entry that ended with an internal plan marker is gone. Confirmed fixed. 30033948e deleted the parenthetical. changelog.d/w3-38-d2bd-conc-async.md:3 now ends at "...needing acquisition/release." and the technical content is intact.

  2. The wave-to-cluster appendix no longer cites the removed corpus directory. Confirmed fixed. docs/plans/2026-09-24-002-refactor-rust-skills-remediation-plan.md:331 now reads "Counts are the per-lens cluster membership this plan applied, recorded here because the audit record itself is not part of the shipped tree; each wave executes the rows behind them." No corpus path is cited anywhere in the appendix. One residual of the same class survives elsewhere in the plan and is reported as finding 2 below.

  3. The Fixed entry that documented a correction to a record which was never committed now states the substantive change. Confirmed fixed, and both of its new sub-claims were verified independently against this head:

    • packages/xtask/data/blocking-census-baseline.json rises from 0 to 1 for packages/d2b-provider-device on the tokio::runtime::Runtime::block_on key inside this pull request's own diff, and that is the only Runtime::block_on change in the file. The block it sits in is packages/d2b-provider-device.
    • The crate holds exactly one #[tokio::test], at packages/d2b-provider-device/tests/device_family.rs:157, and the observed base carried #[allow(clippy::disallowed_methods, reason = "cfg(test) helper")] on the line immediately above it, which this branch removed. The cap of 1 is real and the census must not be corrected back to 0.
  4. The refutation that the wave rows reconcile to 965 exactly is correct, and still is. Confirmed. 368 + 298 + 167 = 833; 833 + 70 + 20 = 923, matching the summary's "923 actionable findings" at line 27; + 25 matches "25 needs-contract findings"; + 17 matches "17 policy-confirmed findings"; 923 + 25 + 17 = 965, matching the total row at line 345 and the goal capsule at line 14. U1's "13, a subset" is annotated as a subset precisely because those rows sit inside the 923, and U8 correctly contributes 0. The appendix reconciles in two independent ways.

Findings

1. (minor) One of three statements about the relay settings schema was corrected; two were left, and now contradict it

docs/specs/providers/ADR-046-provider-transport-azure-relay.md:1369

The patch rewrote the spec.transportSettings schema section at lines 198-203 to state that the schema "is a hand-authored committed artifact: the crate embeds it verbatim (RelayTransportSettings::schema_json) and applies the same admission rules to every settings object it deserializes", deleting the previous claim that the file is "kept in sync by make test-drift (via xtask gen-provider-transport-schemas && git diff --exit-code)".

Two other statements in the same file were left:

  • docs/specs/providers/ADR-046-provider-transport-azure-relay.md:1369-1371 still says the file "is committed, version-controlled, and kept in sync with the Rust AzureRelayTransportSettings type by make test-drift (via xtask gen-provider-transport-schemas && git diff --exit-code)."
  • docs/specs/providers/ADR-046-provider-transport-azure-relay.md:1464-1465 still lists "xtask gen-provider-transport-schemas integration" in the work item's Detailed design row, and "make test-drift gate: xtask gen-provider-transport-schemas && git diff --exit-code" as its Integration row.

Evidence that the surviving claim is false against the committed head: git grep gen-provider-transport-schemas 5001d7b26 returns three hits, all in documentation - the two above, plus specs/001-adr046-d2b3-completion/amendment-provider-derivation-layout.md:974, which is outside this pull request's diff and outside this slice. packages/xtask/src/main.rs dispatches no such subcommand and its usage string does not list one. The same sentence also names AzureRelayTransportSettings, a type that appears nowhere under packages/ at this head; the type is RelayTransportSettings at packages/d2b-provider-transport-azure-relay/src/transport_settings.rs:14.

Before the patch all three locations made the same (false) claim, so the file was at least internally consistent. Correcting one and leaving two is what makes the contradiction new, and it is the same stale-pointer class the patch set out to sweep.

The corrected text at lines 198-203 is itself accurate and I verified it end to end: RelayTransportSettings::schema_json is an include_str! of the committed JSON, validate enforces the same bounds the JSON declares (valid_namespace 3..=50 against ^[a-zA-Z0-9][a-zA-Z0-9-]{1,48}[a-zA-Z0-9]$, valid_entity 2..=50 against ^[a-z][a-z0-9-]{1,49}$, plus the SharedAccessSignature substring refusal the JSON expresses as not), and nixos-modules/provider-runtime-contracts.nix:563 and :568 carry the same two patterns.

Impact: the work item tells a maintainer that a drift gate protects an artifact the same file now says is hand-authored and embedded, and a maintainer who runs the named command gets an unknown-subcommand error. Fix: drop the drift-gate sentence at 1369-1371, keeping the "committed, version-controlled" wording and the derivation-copy paragraph below it, and remove the generator clause from the two work-item rows at 1464-1465. If the generation is genuinely planned rather than done, mark it as planned instead of stating it as an integration that exists.

2. (minor) The plan still tells a reader the audit corpus directory is untracked; the tree has no such directory

docs/plans/2026-09-24-002-refactor-rust-skills-remediation-plan.md:138

The ### Assumptions bullet reads: "The audit corpus is committed with wave 0 so worktrees and reviewers can read the lanes (its directory is currently untracked)."

Evidence against the committed head: git ls-tree -r --name-only 5001d7b26 -- docs/audits lists only the 2026-09-23 ponytail audit. The 2026-09-24 rust-skills corpus is not in the tree in any form; 564bd6166 removed it. The plan's own Definition of Done at line 321 already states the opposite, "The record itself is not part of the shipped tree". So the assumption is wrong twice over: the directory is neither untracked nor committed, it is absent. U1 reinforces it at lines 146, 153 and 167, which instruct a reader to commit the corpus and to verify "the audit corpus is tracked".

Impact: a reader picking the plan up for the follow-up work its Scope Boundaries defers to will look for a directory the repository does not contain, and the U1 verification step can never pass. This is the identical failure the appendix was corrected for in the same commit, and that sweep stopped one section short. Fix: rewrite the assumption to say the corpus is not part of the shipped tree, and mark the U1 corpus-commit step as already reverted by 564bd6166. The appendix wording at line 331 is the right model to copy.

3. (nit) The rename-map table header still schedules the V3 column at v1.1.1

docs/how-to/migrate-d2b-v1-0-to-v1-1.md:212

The patch rewrote the status note at lines 212-215 so that it says the V3 wire schema "ships with the emit-side flip, scheduled for v1.1.2". Fifteen lines below, the rename-map table header at line 231 still labels its second column "V3 field (wire-side, v1.1.1+)", and the section heading at line 222 already reads "v1.1.1 SHIPPED -> CLI-emit at v1.1.2 rename map". The block the patch rewrote is now internally inconsistent about when V3 exists.

This inconsistency predates the patch in form, but the patch restated the schedule directly above the table and left the header alone, so the block it just corrected still carries a schedule a reader would act on. Fix: change the column header to "V3 field (wire-side, v1.1.2+)".

What this slice checked and cleared

  • Fragment grammar, exhaustively. All 165 fragments were parsed against the repository's own rules in packages/xtask/src/changelog.rs: only the six Keep a Changelog ### headings at that level, no content before the first heading, no repeated heading within a fragment, no empty section, and every section's first non-blank line a - bullet. Zero violations. The checker was proven to fire on a duplicate heading, an empty section, a non-bullet first entry, a #### sub-heading and content before the first heading before it was run over the slice.
  • Prohibited dash code points. Every file in the slice at this head was scanned for U+2010, U+2011, U+2012, U+2013, U+2014, U+2015, U+2212, U+FE58 and U+FF0D. Zero occurrences. The only non-ASCII bytes anywhere in the slice are box-drawing characters, arrows, §, comparison operators, an ellipsis and a box tree, none of which is a dash code point.
  • Marker leakage. A sweep of all 194 files for finding, revision, round, supersession and follow-up markers found none in shipped prose. The plan carries its own R/KTD/U requirement and decision identifiers, which are the plan's vocabulary rather than audit finding identifiers, and no audit finding identifier appears anywhere in the slice.
  • Relative links and anchors. Every markdown link in every changed markdown file was resolved against the committed head after normalising the path. Zero broken. The cross-file anchors the slice cites were checked by hand: #set-booted-race-free-serialization resolves to the heading at docs/adr/0018-microvm-nix-removal.md:661, and #broker-caller-role-audit-labels to docs/reference/naming-conventions.md:95.
  • The docs/reference/error-codes.md additions. The new "Audio mutation refusals" section sits outside the generated block (which spans lines 38-78) and matches the code: audio-vm-not-found exits 2 and audio-not-enabled exits 70 at packages/d2bd-runtime/src/typed_error.rs:922 and :926, the wire kinds are the strings at :857-858, and the mutation op names setVolume and mute are the camelCase serde names of AudioOp::SetVolume and AudioOp::Mute at packages/d2b-contracts-control/src/public_wire.rs:2062-2066. The two d2b_core::error to d2b_contracts::error corrections are right: packages/d2b-core/src/error.rs does not exist at this head.
  • The manifest-bundle.md addition. The typed manifest-parse-error kind exists at packages/d2b-contracts/src/error.rs:38 and the resolver's Network-intent refusal is pinned to it at packages/d2b-core/src/bundle_resolver.rs:8170.
  • The ADR-046 spec edits. credential-already-running matches the wire kind emitted at packages/d2b-contracts-provider/src/v3/credential_controller.rs:95; AudioLastSetApplied::NotApplied is the variant at packages/d2b-provider-audio-pipewire/src/controller.rs:143; admission.rs exists under the notification-desktop Provider's src/; and the Azure VM recovery paragraph matches the grouped in_flight_operation field and the legacy operation plus operation_started_at_unix_ms pair at packages/d2b-provider-guest-azure-virtual-machine/src/controller/mod.rs:94, :122 and :201.
  • The migrate-d2b-v1-0-to-v1-1.md rewrite is a real correction, not a hedge. The base told a reader that v1.1.1 "ships the StatusOutputV3 wire schema (packages/d2b/src/lib.rs StatusServicesOutputV3 + from_v2 migration shim)". At this head StatusServicesOutputV3 and from_v2 exist nowhere, and the emitted type is StatusServicesOutputV2 at packages/d2b-contracts-control/src/cli_output.rs:242 and :295. The base named a path the type no longer lives at.
  • Generated reference material. All 17 changed JSON artifacts parse, and every changed enum, $ref and pattern was traced to its Rust type or serde attribute: AuthRoleV2, QemuMediaRegistryState, QemuMediaRunnerState, VmAutostartMode, RealmMode, RealmGatewayState, PublicReadModelKind, NmReloadBehavior, Destructive and ConfigurationGeneration all match their sources, and the Destructive boolean-to-no/yes-string change in privileges.json matches the kebab-case enum at packages/d2b-core/src/privileges.rs:48. Definition names that cross two artifacts (PublicReadModelKind, QemuMediaRegistryState) appear in exactly the files whose types reference them.
  • The daemon API reference. The diff is line-anchor movement. Ten anchors across the four regenerated sections were resolved against the head sources and every one lands on the declaration it names.
  • The two newest commits. 30033948e keeps each reason's technical content and removes only the unit label, in clippy.toml, deny.toml, the six source comments and the two fragments; its deny.toml and clippy.toml edits match its message. 5001d7b26 is a packages change and was not reviewed here beyond the check it asks for: the only file the docs cleanup and the cli commit both touch is changelog.d/refactor-rust-skills-remediation.md, and at head its seven Fixed bullets and three Changed bullets are well formed with the cli entry landing last. The cli commit's regenerated anchors in docs/reference/daemon-api.md resolve. docs/reference/cli-output/auth-status.schema.json needs no regeneration for that commit because allowedSubcommands is typed as an array of strings at line 14, and it does not. Every source edit the docs cleanup made is line-count neutral, so it cannot have shifted a file-and-line key in the async-gate inventory the cli commit also regenerated.

Refuted

Recorded because each looked like a defect from a text search and each was disproved against the committed head, which is the failure mode this slice is most exposed to.

  • "The v2 storage-lifecycle schema is not in this pull request's diff, so the changelog entry describing the fix is unsupported." Refuted. The artifact blob is indeed identical at base and head, but the fix that produces it is net-present: packages/d2b-core/src/storage_lifecycle.rs gains five schemars(rename_all attributes against zero at base, packages/d2b-core/tests/storage_lifecycle_schema.rs is new at 194 lines, and packages/d2b-core/BUILD.bazel gains 17 lines. The fragment line at changelog.d/refactor-rust-skills-remediation.md:6 is supported.
  • "share/d2b/provider/config-schema.json is a dead path named at docs/specs/providers/ADR-046-provider-transport-azure-relay.md:1373." Refuted. It is a Nix derivation output path, not a repository path: packages/d2b-resource-compiler/src/lib.rs:56 and packages/xtask/src/provider_packaging.rs:144 both treat it as one. The sentence is correct and was left alone.
  • "docs/reference/schemas/v2/wire-protocol.json still leaks U10, KTD6 and KTD10 planning markers into shipped prose." The content is real, at lines 1836, 2925 and 4549, but every one of those lines is byte-identical at the observed base and at this head, and no +/- line in this pull request's diff for that file touches them. Pre-existing, not introduced here, and therefore not this pull request's to fix under the reviewed-head contract.
  • "Some fragment in the slice carries an internal planning identifier." Refuted. The seven fragments previously reported for this class were fixed before this head, and a fresh sweep of all 165 fragments at this head returns nothing.

Coverage

Examined: all 29 docs/ files and all 165 changelog.d/ fragments at this head, read from the committed object rather than the working tree. The working tree was confirmed clean for the slice first; the only untracked path in the checkout is a plan file outside the diff. Re-parsed every fragment against the repository's own parser semantics; scanned for prohibited dash code points, for all non-ASCII, and for marker leakage; resolved every relative markdown link and the cross-file anchors the slice cites. Read the plan in full and checked its arithmetic twice over and every path it cites resolves at this head. Verified each docs/reference/daemon-api.md anchor sampled, each changed enum, $ref and pattern in all 17 changed JSON artifacts, and every factual claim the seven changed markdown files make against the Rust, Nix and xtask sources at this head. Re-derived the four prior items from scratch.

Not examined: the factual accuracy of the prose inside all 165 fragment bodies (grammar was checked exhaustively; content claims were spot-verified on the entries tied to the head-moving commits, the branch fragment, and the ADR-046 and relay spec edits); whether the generators emit byte-identical output, which needs the drift gate; the four lanes of the gate and any other build, test or lint; packages/, bazel/, tests/, completions/, nixos-modules/, flake.nix, clippy.toml and deny.toml as code, which the sibling slice owns; and the specs/001-adr046-d2b3-completion/amendment-provider-derivation-layout.md copy of the same stale generator name, which is outside this pull request's diff and outside this slice. No Bazel invocation, formatter, linter, build or test suite was run, and no file in the repository was modified.

@vicondoa

Copy link
Copy Markdown
Owner Author

Review - slice 2 of 2: packages, policy and build tooling

Supersedes the prior slice-2 record bound to head 2317e1980, and further back the record bound to head 7b611cbc6.

Binding Value
Repository vicondoa/d2b
Pull request 608
Observed base ref v3
Observed base OID 133b78fc49c6ca101aaa573c87a5884adad74cb7
Reviewed head OID 5001d7b269eade9fc6e29a271f5ba2200b40f93d
Verdict approve-with-comments

Verdict: approve-with-comments.

Grounding held: git rev-parse HEAD is 5001d7b269eade9fc6e29a271f5ba2200b40f93d, git rev-parse origin/v3 is 133b78fc49c6ca101aaa573c87a5884adad74cb7, and git merge-base origin/v3 5001d7b26 returns that same OID. The slice enumerates to 894 files exactly as scoped (-- . ':(exclude)docs' ':(exclude)changelog.d'): 873 under packages/, plus tests/ 4, completions/ 3, labs/ 8, nixos-modules/ 1, flake.nix, deny.toml, clippy.toml, bazel/, Cargo.lock. Everything below was read from the committed head, not the working tree.

The head commit 5001d7b26 is correct. The new shared READ_ONLY_CLI_COMMANDS list resolves in the parser: list and status are top-level ModernCommand variants, auth carries GenericAuthCommand::Status, and op carries GenericOpCommand::Inspect. The new test every_reported_allowed_subcommand_is_a_command_the_parser_declares at packages/d2b/src/dispatch.rs:1183 walks ModernCli::command() and covers all three roles plus the shared constant, so the none role, the launcher/admin sets derived from all_known_subcommands(), and the daemon's report are all covered by one walk. Nothing downstream still hard-codes a CLI command list the parser now owns: allowed_subcommands and denied_reason are consumed only inside auth_status itself, the report is informational and gates nothing, and projection collision plus shell completion already read the parser through is_builtin_command / builtin_commands. The five names the commit removed are genuinely gone from the parser (HostCommand is get/list/status/prepare/destroy/doctor/reconcile/validate, so host check never existed, and no top-level realm exists), and both auth-status goldens agree with the parser's top-level set minus the three projection carriers minus audit.

Findings

1. Blocking-API baseline caps record the test-inclusive count, so this patch raises a production crate's Runtime::block_on cap from 0 to 1 on a test-only site

  • Severity: minor
  • Path: packages/xtask/data/blocking-census-baseline.json:2143
  • Defect: the packages/d2b-provider-device row for tokio::runtime::Runtime::block_on moves from 0 to 1 in this diff. The single counted call site is a #[tokio::test] in a tests/ directory, and the census tool's own production/test classification already identifies it as a test hit. The baseline writer nevertheless records the test-inclusive clippy count as the cap, so a production crate that had zero permitted blocking Runtime::block_on calls now has one.
  • Evidence: the only block_on in the crate is the macro expansion of #[tokio::test] at packages/d2b-provider-device/tests/device_family.rs:157; the census runs cargo clippy --all-targets (packages/xtask/src/blocking_census.rs:779); the tool splits each file into production and test line sets and has a dedicated hit_is_test predicate at packages/xtask/src/blocking_census.rs:1116 that classifies any line in a tests/ directory as test context; but the value written into the baseline is let authoritative = clippy_count; at packages/xtask/src/blocking_census.rs:1271, which is the total including test hits. A crate-by-crate, entry-by-entry numeric comparison of the base and head baselines shows exactly two raised caps: this one, and packages/xtask std::fs::read_to_string 11 to 13 (a build tool, not an async crate).
  • Consequence: because the cap is a ceiling, adding a production Runtime::block_on to this crate while the test is renamed or removed nets the count back to 1 and the gate passes. That is a narrower hole than it looks, and the mechanism is standing rather than new: packages/d2b-provider-user already carried a cap of 1 at the base on the same #[tokio::test] pattern. What this patch adds is one more production crate whose blocking-call headroom is now spent.
  • Fix: record the production count as the cap rather than the total. run already computes it for every entry class as production; changing counts.insert(entry.path.clone(), authoritative) at packages/xtask/src/blocking_census.rs:1274 to insert production keeps the printed report unchanged, keeps the authoritative total for the summary line, and restores a 0 cap for this crate. If the test-inclusive total is wanted as well, it belongs in a second, separately reported column rather than in the value the gate compares against.

2. Two indentation artifacts introduced by the same wave

  • Severity: nit
  • Paths: packages/d2b-session/src/engine.rs:227 and packages/d2bd/src/audio_dispatch.rs:372
  • Defect: pub async fn establish_initiator sits at column 0 inside its impl block, with its parameter list still at eight spaces. The base had the signature indented to four. Separately, the new doc comment on dispatch_audio starts at column 0 and continues at four.
  • Evidence: base packages/d2b-session/src/engine.rs:245 is pub async fn establish_initiator(; head line 227 is pub async fn establish_initiator(. A count of column-0 pub fn in that file goes from 0 at the base to 1 at the head. The audio_dispatch.rs diff is the mirror image: it re-indents the signature to four spaces and adds the doc comment with only its first line at column 0.
  • Consequence: none at runtime. Neither the Makefile nor .github/workflows/pr-l1-static-fast.yml runs cargo fmt or rustfmt, so make check will not correct or report these, and they will persist into the merged tree.
  • Fix: indent packages/d2b-session/src/engine.rs:227 to four spaces to match the rest of the block, and align the doc comment at packages/d2bd/src/audio_dispatch.rs:372 with the signature it documents.

Refuted

Each item below was re-derived from this head's own sources, not carried over from the prior round.

  1. Published v2 storage lifecycle schema casing. Refuted as still broken; the finding is fixed and the fix is correct. The committed docs/reference/schemas/v2/storage-lifecycle-report.json now requires and declares lockCount, pathCount, restartPolicyCount, schemaVersion, storageContractPresent, syncContractPresent, bundleVersion, contractId, offendingId, and roleId. That matches the daemon's bytes: StorageLifecycleReport carries #[serde(rename_all = "camelCase")] at packages/d2b-core/src/storage_lifecycle.rs:11, and every struct variant of StorageLifecycleIssue that renames a field also repeats the casing as a #[schemars(rename_all = "camelCase")] attribute at packages/d2b-core/src/storage_lifecycle.rs:52,58,70,80,85, which is what schemars 0.8 needs. offendingId is correctly optional and typed ["string","null"], matching its skip_serializing_if. The new contract test at packages/d2b-core/tests/storage_lifecycle_schema.rs validates real serialized bytes against the committed file rather than against fresh generator output, exercises all eight issue variants including the offending_id: None case, and fails on any schema keyword outside its interpreted set. It is wired as a real test with the schema as compile_data at packages/d2b-core/BUILD.bazel:279.

  2. AuditResponse row in the daemon API reference. Refuted as a defect; the row is correct at this head. docs/reference/daemon-api.md documents the shape as entries / next_cursor / complete at the anchor public_wire.rs#L2359, and line 2359 is pub struct AuditResponse. The documented shape is the serialized shape, not the in-memory shape: impl Serialize for AuditResponse at packages/d2b-contracts-control/src/public_wire.rs:2403 writes exactly those three camelCase keys through AuditResponseOut, and impl JsonSchema for AuditResponse at packages/d2b-contracts-control/src/public_wire.rs:2423 delegates to AuditResponseWire, which holds those same three fields at packages/d2b-contracts-control/src/public_wire.rs:2376. The AuditPageEnd enum is an in-memory convenience the wire does not carry, so documenting the wire is right.

  3. Runtime::block_on cap for packages/d2b-provider-device. The count itself is real, and the changelog's account of it is accurate: the diff does move that row from 0 to 1, and the only counted site is the #[tokio::test] expansion at packages/d2b-provider-device/tests/device_family.rs:157, which cargo clippy --all-targets counts because --all-targets includes test targets. What is not accurate as a resolution is the implication that a real count is a real cap; that narrower point is finding 1 above.

  4. host check and the four realm verbs removed from auth status. Refuted as an over-report. HostCommand at packages/d2b/src/host.rs:21 declares only get, list, status, prepare, destroy, doctor, reconcile, validate, and ModernCommand at packages/d2b/src/dispatch.rs:104 declares no realm variant, so all five names the commit removed are unparseable. auth status and op inspect both resolve.

  5. parking_lot to lock_api retarget of the deny list in clippy.toml. Refuted; the resolved-path claim in the committed comment is exact. parking_lot 0.12.5 declares pub type Mutex<T> = lock_api::Mutex<RawMutex, T>; at src/mutex.rs:86, so a parking_lot::Mutex::lock call resolves to lock_api::mutex::Mutex::lock at lock_api 0.4.14 src/mutex.rs:212, and the same holds for RwLock::read / write. parking_lot::Condvar is a real type at parking_lot src/condvar.rs:90, so its own entry correctly keeps the parking_lot name. The three entries in packages/xtask/data/blocking-census-baseline.json were renamed to match, in lockstep, with no cap raised.

  6. The two deleted reload-behavior refusal tests. Refuted as removed coverage. validate_nm_reload_behavior and LiveHandlerError::NmReloadBehaviorRefused are gone, replaced by the closed NmReloadBehavior enum at packages/d2b-core/src/host.rs:470, which makes an undeclared value fail at artifact resolution instead of at the handler. The refusal moved to a stronger layer and is covered there: host_json_refuses_unknown_nm_reload_behavior at packages/d2b-core/src/host.rs:667 asserts a typo fails the HostJson parse, and nm_reload_behavior_round_trips_its_wire_values at packages/d2b-core/src/host.rs:641 pins the closed vocabulary including the empty sentinel. The stale committed fixture that proved the old gap, tests/fixtures/deny-unknown/host-valid.json, declared "nmcli general reload", which the old string field accepted and the old validator would have refused at apply time; it now declares atomic-reload, which is the correct fix for a fixture named deny-unknown that must stay valid.

  7. live_usbip_bind dropping daemon_uid. Refuted as a behavior change. The parameter was already dead: the base signature read _daemon_uid: u32, and the lock path already fchowned to nix::unistd::Uid::current() with the declared daemon_gid. The new UsbipLockError::Io carrying a real std::io::Error plus a source() impl only adds the failure chain; the PathSafetyViolation variant splits out a case that previously rode in a rendered detail string.

  8. The new audio-vm-not-found and audio-not-enabled kinds missing from the published error-code table. Refuted as introduced by this patch. The published table is generated from d2b_contracts::Error::all_kinds() at packages/d2b-contracts/src/error.rs:641, which is a different taxonomy from the daemon's TypedError, and existing TypedError kinds are already absent from it: internal-config-invalid has no registry row, and d2b-contracts::AudioError carries only ProviderMisconfigured. The gap is structural and pre-dates this change; the new kinds join the same set. cli_failure_from_daemon_error at packages/d2b/src/context.rs:337 passes exit_code through generically, so the new exit codes 2 and 70 reach the CLI unchanged and no CLI table needed updating.

  9. packages/d2b-session/src/engine.rs indexing bytes[0] on wire input. Refuted. decode_stream_control at packages/d2b-session/src/engine.rs:1717 returns Err(UnknownControl) unless bytes.len() == 7 before the index, and from_tag returns Option.

  10. Removal of the establish_*_with_metrics constructors. Refuted as a lost capability. establish_initiator_with_metrics at packages/d2b-session/src/engine.rs:243 and establish_responder_with_generation_floor_and_metrics at packages/d2b-session/src/engine.rs:392 remain, along with with_metrics at packages/d2b-session/src/engine.rs:572 and set_metrics at packages/d2b-session/src/engine.rs:577; only the redundant third wrapper that already passed NoopMetrics was folded in. The base's only external caller of a metrics constructor was a test.

  11. daemon_version.rs reporting UpToDate when the on-disk path cannot be read. Refuted as introduced. The base had _ => return DaemonRestartStatus::UpToDate at the same site; the new Err(_) => arm covers exactly the same outcomes, now with the error classified by the new VersionFileReadError enum.

  12. deny.toml duplicate-cluster comment naming crates that do not exist. Refuted. d2b-provider-host, d2b-provider-audio-pipewire, d2b-provider-observability-otel, and d2b-sk-frontend all exist under packages/; which is a crates.io crate, not a workspace member, and the comment lists it among transitive pullers, which is correct. The two named head clusters list 17 and 23 pullers respectively, matching their stated counts.

  13. nixos-modules/privileges-json.nix changing destructive from a boolean to "no". Refuted as a broken pairing. OperationAuthz.destructive is now the typed Destructive enum at packages/d2b-core/src/privileges.rs:34, serialized kebab-case from No / Yes at packages/d2b-core/src/privileges.rs:46, so the artifact's "no" and "yes" decode. The boolean still present in packages/*/operations.json is the generator's input form, consumed as destructive: bool at packages/xtask/src/gen_broker_operations.rs:125 and emitted as the enum at packages/xtask/src/gen_broker_operations.rs:982; packages/d2b-broker-composition carries its own BrokerAuthzFacets with a bool field, which is a separate type. A sweep for residual boolean destructive consumers of the privileges artifact outside operations.json and the generator found none.

  14. The Nix privileges matrix and the generated Rust authz matrix disagreeing (63 rows against 98). Refuted; they are different surfaces. The Nix module lists public CLI operation names such as auth status, audio, and boot, while packages/d2b-core/src/generated/broker_operation_authz.rs lists private broker operations such as ApplyNftables and CgroupKill. Name-set drift is already covered by tests/unit/nix/surfaces/privileges-json-drift.nix, and the two generated files carry a byte-for-byte drift gate.

  15. exec kill --signal and endpoint --endpoint-class narrowing to closed vocabularies. Refuted as a regression. The base validators accepted exactly term | kill | int | hup at packages/d2b/src/exec.rs and exactly service | device | transport | control | data at packages/d2b/src/endpoint.rs; the new ValueEnum types at packages/d2b/src/exec.rs:88 and packages/d2b/src/endpoint.rs:33 carry the same sets and the same exit code 2. The regenerated completions/ value hints match.

  16. The clipboard display-route digest for the daemon-committed path not binding the Guest subject reference. Refuted as introduced. from_committed_display_route at packages/d2b-provider-clipboard-wayland/src/controller/mod.rs:223 mixes user_ref.to_canonical_string() into the digest and never mixes route.subject_ref(), so two Guest subjects under the same committed User would hash alike for the same zone, host execution ref, and generations. The base already had user_ref.to_canonical_string() at the same place, so this is not this patch's change. Separately, the DisplayRouteShape extraction is correct: user_ref_type is derived from the real inputs at both call sites rather than hardcoded, so the new is_none() / Some("User") discrimination is not vacuous.

  17. The 195 async-gate inventory line anchors drifting. Refuted. Every anchor in packages/xtask/data/async-gate-inventory.json resolves to a line consistent with its stated reason: entries marked tokio OpenOptions builder flag land on .write(true) lines, entries marked as lock acquisitions land on .lock() / .read() / .write(), and the two file-I/O entries land on file.read / file.write. No (file, line) pair is duplicated. The head commit's anchor shifts in that file, including the test_support.rs entry moving from 50 to 54, are correct.

Coverage

Examined, from the committed head:

  • The full 5001d7b26 diff read line by line across packages/d2b-contracts-control/src/public_wire.rs, packages/d2b/src/dispatch.rs, packages/d2b/tests/auth_status_contract.rs, packages/d2bd/src/composition.rs, and packages/xtask/data/async-gate-inventory.json; then the parser it depends on (ModernCli, ModernCommand, HostCommand, GenericOpCommand, GenericAuthCommand), every consumer of allowed_subcommands and denied_reason, is_builtin_command / builtin_commands, and both auth-status goldens.
  • The three re-derivations named for this round: the storage lifecycle schema against StorageLifecycleReport / StorageLifecycleIssue and its new contract test; the AuditResponse reference row against the domain struct, its Serialize impl, its JsonSchema impl, and AuditResponseWire; and the packages/d2b-provider-device Runtime::block_on cap against the crate's real call sites and the census tool's counting and prod/test logic.
  • packages/d2bd-runtime/src/typed_error.rs in full for the new ErrorSource field, the two new audio variants, the hand-written Display and Error impls, and the SourceChain depth guard; then the call sites in packages/d2bd/src/audio_dispatch.rs, the exit-code precedents cited in the new comments, and the registry question behind the published error table.
  • packages/d2b-broker/src/protocol.rs for the MSG_PEEK length-prefix optimization, including decode_frame, the fixed-ceiling fallback, the fd-passing variant, the zero-length-packet case, and confirmation that no SockType::STREAM socket exists anywhere in the tree.
  • Concurrency surfaces: DispatchPool and its nested pool in packages/d2b-broker/src/runtime.rs, and the new dedicated seats in packages/d2b-broker/src/ops/host_generation_handoff.rs and packages/d2b-broker/src/sys.rs, checking each for nested block_on starvation and for the bounded-queue refusal semantics.
  • Policy and supply-chain inputs: the full clippy.toml and deny.toml diffs, a crate-by-crate and entry-by-entry numeric comparison of the blocking census baseline at base and head, the resolved-path claim behind the lock_api retarget, and the 72 packages/policy-inputs files including the newly mirrored d2b-resource-client entry against packages/Cargo.guest.lock, tests/fixtures/guest-rust-workspace/Cargo.toml, and the flake.nix copy line.
  • Build tooling and generated artifacts: bazel/checks/nix/defs.bzl, the new storage_lifecycle_schema Bazel target and its compile_data, the gen_broker_operations generator against both generated files it owns, the gen_error_codes and gen_daemon_api generator paths, and the three completions/ files against the parser that generates them.
  • Mechanical sweeps across the whole slice: every added unwrap / expect / panic! / todo! filtered to non-test lines (157 candidates, each read at its site; all are either infallible operations, just-inserted invariants, or test-support modules), and every added Err(_) => / .ok() / let _ = ....await / unwrap_or_default in production code (183 candidates; the ones with real failure semantics were read individually, including live_handlers.rs:1791, d2b-session/src/engine.rs:1723, d2bd-runtime/src/daemon_version.rs:157, and d2b-provider-clipboard-wayland/src/controller/mod.rs).
  • nixos-modules/privileges-json.nix against packages/d2b-core/src/privileges.rs, and the clipboard provider's display-route validation and digest at packages/d2b-provider-clipboard-wayland/src/controller/mod.rs.

Not examined, and why:

  • docs/ and changelog.d/, which are the sibling slice's surface. Where a generated doc was the object of a finding, I checked it against its Rust source and its generator rather than its prose.
  • packages/d2b-provider-clipboard-wayland/src/bin/d2b-clipd.rs (337 lines changed) and the rest of the clipboard binary, packages/d2b-provider-toolkit/src/testing/fakes.rs, and the remaining labs/, completions/, and nixos-modules/generated/ content beyond the files named above. These are the largest remaining churn in the slice and are the most likely place a further defect is hiding.
  • Full-file reads of packages/d2b-broker/src/runtime.rs (20860 lines) and packages/d2bd/src/composition.rs; I read the diff and the regions the findings and refutations turned on rather than the whole files.
  • The container, VM, and live-host layers, and any runtime behavior. No build, test, formatter, or linter was run for this review; the four-lane gate result at this head was taken as given.
  • packages/Cargo.lock beyond the aggregate line count, and the per-crate Cargo.lock refreshes, beyond confirming that d2b-resource-client is the one added mirrored crate across the guest lock, the fixture workspace, and the flake copy list.

The committed Provider signature verified at 88f13fc, d3e58b3, and
0d2a5bc and broke at 2ca9a22, which moved root-config.schema.json
and updated the manifest configDigest without re-signing. The old
private key is not recoverable, so rotate the publisher keypair rather
than re-derive the old one: publisher-public-key.pem carries the new
SPKI public key and provider-manifest.json.sig is a fresh raw 64-byte
Ed25519 signature over the current manifest bytes. The manifest is
byte-for-byte unchanged; only the signature and the public key move.
The operator holds the private half outside the repository.

Nothing caught the staleness because the Nix build only asserts that
the sidecar is 64 bytes long. Add a hermetic test in the resource
compiler that loads the committed manifest, sidecar, and public key and
asserts the signature verifies over the manifest, with a probe for each
half moving alone, so a later manifest or schema change that forgets to
re-sign fails the check lane instead of shipping. The test reads the
trio from the runfiles tree, and the trio is declared as test data so
Bazel re-runs it when any of the three files changes.
@vicondoa

Copy link
Copy Markdown
Owner Author

Review - slice 1 of 2: docs, changelog and plan

Scope: exactly the 195 files under docs/ and changelog.d/ between the observed base and the reviewed head. Packages, policy and build-tooling surface are out of scope and are covered by the other slice.

Bindings

binding value
repository vicondoa/d2b
pull request 608
observed base ref v3
observed base OID 133b78fc49c6ca101aaa573c87a5884adad74cb7
reviewed head OID e1c7e85d576301e4e84cdcaf784555fd9b0ca043
verdict request-changes

Verdict: request-changes.

Required question 1 - the new signature changelog fragment

changelog.d/fix-provider-artifact-signature.md records the rotation and the reason. It states that the signature was computed over an earlier manifest, that the manifest that moved root-config.schema.json and updated configDigest was committed without re-signing, that the compiler refused the artifact with provider-signature-verification-failed, that the old private key is not recoverable so the publisher key changes rather than being re-derived, and that the manifest is byte-for-byte unchanged with only publisher-public-key.pem and provider-manifest.json.sig moving. All of that matches the head commit's own description and the committed artifact.

No key material is exposed. The fragment names only the two public artifacts (publisher-public-key.pem, provider-manifest.json.sig) and the public path of the manifest. It contains no private key byte, no private key path, no secret, no token, and no credential. It states that the operator holds the private half outside the repository, which is the permitted form. The only defect in the file is finding 3 below, and it is an accuracy problem, not a disclosure problem.

Fragment format is compliant with docs/contributing/changelog-and-commits.md and changelog.d/README.md: exactly two ### headings from the permitted set (Fixed, then Added), no content before the first heading (the leading blank line is accepted - the parser rejects only non-blank content before the first heading, at packages/xtask/src/changelog.rs:186-193), no repeated heading, and each section carries at least one entry beginning with - . Entry lines wrap and indent as the README's verbatim-copy rule expects.

Required question 2 - documentation drift under docs/

No file under docs/ asserts that the cloud hypervisor provider manifest signature is stale, unsigned, or unverified, and no file documents the publisher key in a way the committed artifact contradicts. A sweep of docs/ and the provider tree for stale/broken/unsigned/unverified/re-sign claims over the manifest signature returns only generic, correct material: the fail-closed contract in docs/specs/providers/ADR-046-provider-runtime-cloud-hypervisor.md:116 and docs/specs/ADR-046-provider-model-and-packaging.md:145, the layout and verification rules in docs/adr/0050-provider-derivation-artifact-layout.md:265-268, 635-638, 968-972, and the error-codes row for provider-signature-verification-failed at docs/reference/error-codes.md:70. The committed publisher-public-key.pem at this head is a bare SPKI Ed25519 PUBLIC KEY PEM with no comment block and no fingerprint annotation, so no doc's description of it can conflict.

This branch also removes real drift rather than adding it, and each removal was checked against committed code:

  • docs/how-to/migrate-d2b-v1-0-to-v1-1.md:212-216 drops the claim that StatusServicesOutputV3 and a from_v2 migration shim live in the public surface. Neither identifier exists anywhere in packages/ at this head; only StatusServicesOutputV2 does (packages/d2b-contracts-control/src/cli_output.rs:295). The replacement wording, that the V3 shape is the rename map applied to V2 and is not a type you can name today, is accurate.
  • docs/reference/error-envelope-guidance.md:30 and docs/reference/error-codes.md:14,34 move the registry from d2b_core::error::Error to d2b_contracts::error::Error. packages/d2b-core/src/error.rs no longer exists; the enum is at packages/d2b-contracts/src/error.rs:537.
  • The three credential specs and ADR-046-resources-credential.md add credential-already-running, which is a real code at packages/d2b-contracts-provider/src/v3/credential_controller.rs:95.
  • ADR-046-provider-audio-pipewire.md:748 changes OfflineOnly to NotApplied; NotApplied is the real variant at packages/d2b-provider-audio-pipewire/src/controller.rs:143 and is the value asserted in packages/d2b-provider-wayland-policy/src/audio_registry/audio_registry_tests.rs:138.
  • ADR-046-provider-notification-desktop.md:76,671,673,1353,1390 renames stream_admission.rs to admission.rs; packages/d2b-provider-notification-desktop/src/admission.rs exists and stream_admission.rs does not.
  • ADR-046-provider-runtime-azure-virtual-machine.md:468-474 describes the grouped inFlightOperation plus the legacy operation + operationStartedAtUnixMs pair. The grouped field, the legacy pair, and the fold that accepts both are all at packages/d2b-provider-guest-azure-virtual-machine/src/controller/mod.rs:94,122,201-216.
  • docs/reference/error-codes.md:121-131 adds the audio mutation refusals. audio-vm-not-found maps to exit 2 and audio-not-enabled to exit 70 at packages/d2bd-runtime/src/typed_error.rs:922,926, exactly as documented.
  • docs/reference/manifest-bundle.md:48-51 claims the resolver surfaces a Network spec parse failure as manifest-parse-error rather than intent-not-found. That is the real behavior and it is asserted at packages/d2b-core/src/bundle_resolver.rs:8148-8176.

Planning, revision, follow-up and finding markers. The branch adds no marker to any shipped doc. A scan of all 166 new changelog fragments' contents for audit identifiers (U<number>, #C<number>, #S<number>, ADR046-, wave tags, corpus paths) returns nothing. No Unicode dash appears in any changed file. The new plan's KTD/R/U identifiers and its Deferred to Follow-Up Work heading are plan-internal labels, which is the established convention for docs/plans/ and is not one of the surfaces AGENTS.md enumerates (source, docs, CLI text, CI names, changelogs, commit messages, PR bodies) in a way the plan violates; see the refuted list for the two marker candidates I disproved.

Required question 3 - regenerated reference material against committed code

The v2 storage-lifecycle casing defect is genuinely fixed. StorageLifecycleIssue in packages/d2b-core/src/storage_lifecycle.rs:53 still carries serde's container-level rename_all_fields = "camelCase", which schemars 0.8 does not read. All five struct variants now repeat the casing as a #[schemars(rename_all = "camelCase")] attribute (storage_lifecycle.rs:57,62,70,78,83), covering every variant that carries a renamed field, and the committed docs/reference/schemas/v2/storage-lifecycle-report.json now requires bundleVersion, contractId, offendingId and roleId. The three unit variants need no attribute. The regression test at packages/d2b-core/tests/storage_lifecycle_schema.rs validates serialized bytes against the committed file rather than against fresh generator output, and the committed file's required sets omit offendingId, matching #[serde(skip_serializing_if = "Option::is_none")]. There is no residual casing disagreement.

The same defect class is not present in the sibling v2/v3 schema pairs. rename_all_fields has exactly three occurrences in the workspace. One is the fixed type above. The other two are ZoneBootstrapReply (packages/d2b-contracts-zone-session/src/v3/zone_session.rs:686) and ZoneEnrollReply (:718). Neither is published: no committed schema under docs/reference/schemas/ derives from either type, and their peers decode with serde rather than against a schema. ZoneEnrollReply's fields (zone, generation) are single words, where camelCase and snake_case are identical anyway. I also swept every changed schema for the other direction of the same drift by listing all property names and comparing them with the committed serde attributes:

  • v2/host.json reloadBehavior and v2/privileges.json destructive now $ref the real NmReloadBehavior and Destructive enums; the property names are unchanged and the previously unconstrained "type": "string" / "type": "boolean" placeholders are now closed value sets. Strictly tighter, no key renamed.
  • v3/core.d2bus.org_Command.schema.json CommandExec and CommandArgvSlot match EXEC_PATTERN and ARGV_SLOT_PATTERN in packages/d2b-provider-command/src/command.rs:35-43, and the \u0000-\u001f\u007f-\u009f ranges are exactly what char::is_control rejects in CommandExec::parse (:57) and CommandArgvSlot::parse (:117).
  • v3/core.d2bus.org_EphemeralProcess.schema.json targetGeneration now $refs ConfigurationGeneration rendered by the hand-written JsonSchema at packages/d2b-contracts-resource/src/v3/identity.rs:476-484 as {"type":"integer","minimum":1.0}, which is what the committed definition says.
  • Both v3/activation-nixos.d2bus.org_NixosGeneration*.schema.json observedGeneration now carry format: uint64, minimum: 0, which is the derived schema of the #[serde(transparent)] pub struct ObservedGeneration(u64) at packages/d2b-contracts-resource/src/v3/identity.rs:508 that the branch put on the field.
  • The oneOf enum splits in v2/unsafe-local-helper-wire.json, v2/wire-protocol.json and the cli-output schemas preserve their accepted value sets exactly; I checked every oneOf in the 16 changed schema files for branches whose enums overlap, which would make oneOf unsatisfiable, and found none.
  • The snake_case property names in cli-output/audit.schema.json are correct, not drift: AuditOutputV2 carries only #[serde(deny_unknown_fields)] and no rename_all (packages/d2b-contracts-control/src/cli_output.rs:326-339), so serde really does write auto_upgrade_commits_lock, ch_version and the rest.

v2/manifest_v04.json and the rest of the changed schemas are description-only enrichments.

Required question 4 - paths, targets, counts and commands

Covered by finding 2 (three nonexistent source paths) and finding 1 (a nonexistent command named three times). All other named targets in the slice resolve at this head: the 63 path-like tokens in the new plan were each checked against the committed tree, as were every relative link in the 13 changed markdown files, and the Makefile targets the new docs name (check, test-integration, test-host-integration, check-census, test-drift, test-changelog, changelog-fold) all exist. The plan's finding-count arithmetic is internally consistent (the naive row sum of 978 minus the 13 high rows the table marks "a subset" gives the stated 965, and 923 + 25 + 17 = 965).

Findings

1. The same file still names a command that does not resolve, in three places the branch left behind

docs/specs/providers/ADR-046-provider-transport-azure-relay.md:1369-1371 (and the same claim again at :1464 and :1465).

Defect: line 1369-1371 still says the committed settings schema "is committed, version-controlled, and kept in sync with the Rust AzureRelayTransportSettings type by make test-drift (via xtask gen-provider-transport-schemas && git diff --exit-code)". Line 1464 names "xtask gen-provider-transport-schemas integration" in the detailed design, and line 1465 gives the integration as "make test-drift gate: xtask gen-provider-transport-schemas && git diff --exit-code". gen-provider-transport-schemas is not a subcommand this repository's build driver implements.

Evidence: at the reviewed head the subcommand dispatch in packages/xtask/src/main.rs:122-217 handles sixteen gen-* names and the usage string it prints at main.rs:225-230 does not list gen-provider-transport-schemas; any unrecognized command falls through to the usage printer and returns ExitCode::FAILURE, so the command cannot succeed. The drift gate is bazel/checks/BUILD.bazel:193 -> //bazel/checks/policy:drift -> //packages/xtask:generated_artifacts (bazel/checks/policy/BUILD.bazel:78-83), which can only regenerate artifacts an implemented subcommand owns, so the committed file is not drift-gated. This branch already established that the correct description is "hand-authored": the same file's settings-schema section at lines 198-203 was rewritten in this diff to say exactly that, and the surrounding sentence at :1366-1368 plus the artifact tree comment at :1355-1360 still name the removed mechanism. Impact: a maintainer is told a safety net exists for the Provider transport settings schema when none does, and a later edit to that schema will not fail any gate.

Fix: delete the make test-drift sync claim at lines 1369-1371 and the xtask gen-provider-transport-schemas clauses at lines 1464 and 1465, keeping the hand-authored wording the branch already adopted at lines 198-203. Line 1370 also names the Rust type AzureRelayTransportSettings; the actual type is RelayTransportSettings (packages/d2b-provider-transport-azure-relay/src/lib.rs:38), and AzureRelayTransportSettings is only the schema's title.

2. The new plan names three source files that do not exist at the reviewed head

docs/plans/2026-09-24-002-refactor-rust-skills-remediation-plan.md:151.

Defect: the unit U1 "Files" list names packages/d2b-broker/src/ops/kernel_ops.rs, packages/d2b-broker/src/ops/sys.rs and packages/d2b-provider-display-wayland/src/filter.rs. None of the three resolves.

Evidence: the committed tree at the reviewed head places the first at packages/d2b-broker/src/kernel_ops.rs and the second at packages/d2b-broker/src/sys.rs (both are siblings of src/ops/, not members of it, and packages/d2b-broker/src/ops/ contains no kernel_ops.rs or sys.rs); the third is at packages/d2b-provider-display-wayland/src/wayland_proxy/filter.rs. The same list names packages/d2b-broker/src/runtime.rs, packages/d2bd-runtime/src/runtime_process.rs and packages/d2b-resource-runtime/src/revision.rs, which all resolve, so the defect is confined to those three entries rather than the whole list. The plan is the unit-of-record document a wave executor works from, and the corresponding test scenario in the same unit names the display-wayland registry-handler test, so a reader following the list cannot locate two of the three sites.

Fix: correct the three entries to packages/d2b-broker/src/kernel_ops.rs, packages/d2b-broker/src/sys.rs and packages/d2b-provider-display-wayland/src/wayland_proxy/filter.rs.

3. The new changelog fragment asserts a record that is not present

changelog.d/fix-provider-artifact-signature.md:13-15.

Defect: the fragment says "the operator holds the new private half outside the repository, and only its location and public fingerprint are recorded here", and the next sentence instructs consumers that they "must trust the new public key before building a Zone that installs this artifact". The fragment records neither a location nor a fingerprint, so the sentence describes a record that does not exist and the pinning instruction gives the reader nothing to pin.

Evidence: the fragment is 26 lines and contains no fingerprint string and no private-key location; the only paths it names are the two public artifacts. The committed key at the reviewed head, packages/d2b-provider-guest-cloud-hypervisor/publisher-public-key.pem, is a bare SPKI Ed25519 PUBLIC KEY PEM whose sha256 over the DER SubjectPublicKeyInfo is d798fa9f94f64015d5711c0484b5b30cef1b6c9bb918cb46bf43338e0d817b57, and that value appears nowhere in docs/ or changelog.d/. Because the fold copies entry lines verbatim into the released CHANGELOG.md section, the false clause becomes permanent release prose. The fix must stay on the public side: a public fingerprint and the committed public-key path are safe, and nothing that locates or reconstructs the private half may be added.

Fix: either add the public-key fingerprint and the committed public-key path, or delete the "and only its location and public fingerprint are recorded here" clause and point readers at packages/d2b-provider-guest-cloud-hypervisor/publisher-public-key.pem for the key they must trust.

4. The new plan describes labs/ as retired when the tree is present and live

docs/plans/2026-09-24-002-refactor-rust-skills-remediation-plan.md:71.

Defect: the Scope Boundaries section defers "The retired labs/ tree and any pre-existing failure the baseline attributes to work outside the audit's crates". labs/ is not retired at this head.

Evidence: the committed tree at the reviewed head carries 171 tracked files under labs/, and the repository treats that tree as live rather than retired: docs/contributing/gates-and-lints.md:21 names labs/venus-vulkan-video/AGENTS.md among the governing documents contributors must read; docs/adr/0047-window-identity-chrome.md:11 lists labs/window-chrome/ as the prototype for that ADR and :294 records obligations against labs/window-chrome/SPOOF-FINDINGS.md as satisfied; and docs/explanation/over-engineering-audit-record.md:342 records a labs/window-chrome/proxy copy as "refused - not deleted: it is a standalone workspace whose disposition ADR 0047 owns". A reader who takes the plan at its word would look for a removed tree and would not look for lab-scoped failures, which is the only thing the deferral is about. The harm is modest, which is why this is ranked lowest.

Fix: replace "The retired labs/ tree" with wording that says the labs/ tree is out of scope for this plan and whose disposition belongs to ADR 0047, matching the existing record.

Refuted

Candidates I considered and disproved, each against the committed head:

  1. changelog.d/fix-provider-artifact-signature.md leaks key material. Refuted: the fragment names only the public manifest, the public publisher-public-key.pem, and the public provider-manifest.json.sig; there is no private key byte, no private-key path, no secret, and no credential anywhere in it, and the fragment's only key-location statement is that the private half is held outside the repository.
  2. The same fragment violates the repository's fragment format. Refuted: two permitted ### headings, no non-blank content before the first heading, no repeated heading, and each section carries at least one - bullet, all of which the parser at packages/xtask/src/changelog.rs:154-250 accepts; the leading blank line is explicitly tolerated.
  3. The v2 storage-lifecycle casing defect is still present after 6cbcf185b. Refuted: all five struct variants of StorageLifecycleIssue carry #[schemars(rename_all = "camelCase")] at packages/d2b-core/src/storage_lifecycle.rs:57,62,70,78,83 and the committed schema requires the camelCase keys.
  4. ZoneBootstrapReply and ZoneEnrollReply carry the identical rename_all_fields schemars gap and reach a published artifact. Refuted: they are the only other rename_all_fields sites in the workspace, no committed schema under docs/reference/schemas/ derives from either, and their consumers decode with serde; ZoneEnrollReply's fields are single words where the two casings coincide.
  5. The not keyword added to docs/reference/schemas/v3/providers/transport-azure-relay.transport-settings.json is rejected by the resource compiler's schema integrity allowlist, which omits not. Refuted: the allowlist at packages/d2b-resource-compiler/src/main.rs:1232-1258 does omit not, but the only sweep that applies it, every_committed_v3_schema_passes_integrity_validation at main.rs:2202-2221, uses a non-recursive read_dir over docs/reference/schemas/v3 and skips the providers subdirectory, and SchemaCache::schema at main.rs:1642-1660 only ever resolves core.d2bus.org_<Type>.schema.json or a qualified <namespace>_<Type>.schema.json, never a providers/ file. The keyword is intentional and pinned by packages/d2b-provider-transport-azure-relay/tests/transport_settings_schema.rs.
  6. The branch's 166 new changelog fragments break the one-fragment-per-branch rule. Refuted: changelog.d/README.md explicitly anticipates wave-tagged fragment filenames ("no branch-named, wave-tagged fragment filename leaks into a release artifact") and the release gate deletes fragments at fold time, so the names never reach a release; the fragment contents carry no planning or finding identifier, and the fold accepts them.
  7. 104 of the 166 new fragments lack a trailing newline, which corrupts the fold. Refuted: parse_fragment iterates text.lines() at packages/xtask/src/changelog.rs:161, which is newline-agnostic at end of input, and trim_blank_edges at :253-259 normalizes both edges, so a missing final newline is indistinguishable from a present one and the fold loses nothing.
  8. The plan's finding-count arithmetic does not sum to its own stated total. Refuted: the naive row sum is 978, and the U1 row is explicitly annotated "13, a subset" of the leaf, family and wide pools it is drawn from, so 978 minus those 13 double-counted rows is the stated 965, which also equals 923 actionable plus 25 needs-contract plus 17 policy-confirmed.
  9. The new plan leaks finding identifiers into the shipped tree. Refuted: it carries aggregate cluster counts and the wave-to-cluster map, not per-finding identifiers, and the plan's own R9 and Definition of Done enumerate source, doc comments, commits, changelog fragments and pull-request bodies; docs/plans/ is the designated home for KTD/R/U identifiers, which every existing plan in that directory already uses.
  10. The committed audit corpora under docs/audits/ and docs/residual-review-findings/ are a finding-marker leak introduced here. Refuted: both trees are byte-unchanged by this branch - docs/audits/ holds 179 files at the base and 179 at the head - so no marker was added by this diff.
  11. The regenerated docs/reference/daemon-api.md left stale source line anchors. Refuted: the anchors moved correctly, for example FeatureFlag from #L70 to #L101 and Hello from #L151 to #L182, and pub struct FeatureFlag(String) and pub struct Hello are at lines 101 and 182 of packages/d2b-contracts/src/lib.rs at the reviewed head; every relative ../../packages/... link target in the file resolves.

@vicondoa

Copy link
Copy Markdown
Owner Author

Reconciliation of the two slice reviews bound to e1c7e85d5

Both records above are bound to head e1c7e85d576301e4e84cdcaf784555fd9b0ca043
against observed base v3 at 133b78fc49c6ca101aaa573c87a5884adad74cb7. They
supersede the earlier records bound to 7b611cbc6, 2317e1980 and
5001d7b26.

Every candidate raised by either review was re-verified against the committed
head before being accepted. Nothing was accepted on the reviewer's word.

Slice 2: verdict approve, no findings - confirmed

All 20 refuted candidates stand. One of them carries a misreading that does
not change the outcome: the refuted item about the d2b-provider-device
census row reads commit 2317e1980 as "citing line 179, which is off". That
commit's message does the opposite: it names tests/device_family.rs:179 as
the site the row previously recorded and wrong, and tests/device_family.rs:157
as the correct one. #[tokio::test] is at line 157 of that file, so the
recorded baseline of 1 has the real cause the commit describes.

The slice 2 evidence that matters most was independently reproduced outside
the repository's own code: openssl pkeyutl -verify -pubin -rawin over the
committed provider-manifest.json with the committed
provider-manifest.json.sig and publisher-public-key.pem reports
Signature Verified Successfully; a one-byte change to the manifest and the
previous public key against the new signature both report
Signature Verification Failure. The manifest blob OID is
378c1546f496da29f9c5204d8887071d0ecb27f0 on both sides of the head commit, so
the rotation is signature-and-key only.

The new test was also confirmed to execute rather than merely compile: the
//packages/d2b-resource-compiler:d2b_resource_compiler_test test log
consumed by this head's check lane records
test tests::committed_artifact_signature_verifies_over_the_committed_manifest ... ok
with test result: ok. 9 passed; 0 failed.

Slice 1: verdict request-changes, four findings - all four hold

Each was reproduced with read-only inspection of the committed head.

  1. docs/specs/providers/ADR-046-provider-transport-azure-relay.md:1369-1371,
    and the same claim at :1464 and :1465, name
    xtask gen-provider-transport-schemas. No such subcommand is implemented
    in packages/xtask/src/; the same file already states at :198-203 that
    the schema is hand-authored, so the file contradicts itself at this head.
    One location in the review is off: the Rust type is
    RelayTransportSettings at
    packages/d2b-provider-transport-azure-relay/src/transport_settings.rs:14,
    not src/lib.rs:38. The substance of the finding is unaffected.
  2. docs/plans/2026-09-24-002-refactor-rust-skills-remediation-plan.md:151
    names packages/d2b-broker/src/ops/kernel_ops.rs,
    packages/d2b-broker/src/ops/sys.rs and
    packages/d2b-provider-display-wayland/src/filter.rs. None resolves; the
    committed tree carries them at packages/d2b-broker/src/kernel_ops.rs,
    packages/d2b-broker/src/sys.rs and
    packages/d2b-provider-display-wayland/src/wayland_proxy/filter.rs.
  3. changelog.d/fix-provider-artifact-signature.md:13-15 says "only its
    location and public fingerprint are recorded here" and then records
    neither. No public-key fingerprint appears anywhere under docs/ or
    changelog.d/. Because the fold copies entry lines verbatim into the
    released changelog, the clause becomes permanent release prose. The fix
    must stay on the public side.
  4. docs/plans/2026-09-24-002-refactor-rust-skills-remediation-plan.md:71
    defers "the retired labs/ tree". The head carries 171 tracked files under
    labs/, docs/contributing/gates-and-lints.md:21 names
    labs/venus-vulkan-video/AGENTS.md as governing, and
    docs/adr/0047-window-identity-chrome.md:11 and :294 treat
    labs/window-chrome/ as the live prototype that owns its disposition.

Disposition

All four are documentation-only: three are in docs/, and the fourth is a
changelog fragment whose defect is accuracy rather than disclosure - it
exposes no key material, and the fragment's only key-location statement is
that the private half is held outside the repository. None is a correctness
defect in the product, and none turns a gate green.

None is acted on in this round. A fix moves the head, which would invalidate
both review records above and require a fresh four-lane gate and fresh reviews
before the pull request is mergeable again.

@vicondoa

Copy link
Copy Markdown
Owner Author

Review - slice 2 of 2: packages, policy and build tooling

Bindings

Field Value
repository vicondoa/d2b
PR 608
observed base ref v3
observed base OID 133b78fc49c6ca101aaa573c87a5884adad74cb7
reviewed head OID e1c7e85d576301e4e84cdcaf784555fd9b0ca043
verdict approve

Verdict: approve. No finding in this slice met the bar of provable impact plus a discrete fix. Slice 2 covers every non-docs/, non-changelog.d/ path in the pull request: the packages/ tree, Cargo.lock and the manifest files, BUILD.bazel files, bazel/, clippy.toml, deny.toml, flake.nix, nixos-modules/, completions/, labs/, and tests/. All work below was done by read-only git inspection, file reading, and text search at the reviewed head. No build, test, lint, format, or Bazel command was run, and the working tree was not modified.

1. Manifest untouched by the rotation

Confirmed, and it is the load-bearing check for this commit.

  • git rev-parse e1c7e85d5:packages/d2b-provider-guest-cloud-hypervisor/provider-manifest.json and the same rev-parse against e1c7e85d5^ both return blob 378c1546f496da29f9c5204d8887071d0ecb27f0.
  • git show --name-status e1c7e85d5 lists five paths and provider-manifest.json is not among them.
  • The content hash matches the value stated for this head: git show e1c7e85d5:...provider-manifest.json | sha256sum is b8e838b66540777edfbeed5fe3cb92025a71160ecc3c1ada8a3a75bf0d5b39d3.

The manifest is a different blob at the pull request base (cc2b6fffe45f683fa2f7ee9ec2edda252272dcf1), which is expected: commit 2ca9a22dd in this pull request updated it, and this commit re-signs the current bytes rather than reverting them. So the rotation is genuinely signature-and-key only.

Related consistency check I also ran, because the commit message names the schema as part of the failure: the manifest's digests.config and its single components[0].configDigest are both sha256:1962ab36cc527cf99a924936b57e4daf77a5416805c245c99a2c64e121c2bf9c, and git show e1c7e85d5:...root-config.schema.json | sha256sum is exactly that value. The signed manifest and the committed schema agree at this head.

2. Public key replaced, not added; sidecar is 64 raw bytes

  • git show --name-status --format= e1c7e85d5 reports M for packages/d2b-provider-guest-cloud-hypervisor/publisher-public-key.pem and M for packages/d2b-provider-guest-cloud-hypervisor/provider-manifest.json.sig. Both are modifications, not additions. The only A in the commit is the changelog fragment.
  • The new PEM body is MCowBQYDK2VwAyEA8t2y3JtHJ7SVcYOmgtd/YPoB3OGERUZfwewLGLchzcA=, and base64 -d of that body yields exactly 30 2a 30 05 06 03 2b 65 70 03 21 00 followed by 32 key bytes. That is a well-formed SubjectPublicKeyInfo: a SEQUENCE, an AlgorithmIdentifier whose OID is 06 03 2b 65 70 (1.3.101.112, Ed25519) with no trailing parameters, and a BIT STRING with a zero-length prefix and a 32-octet key. It also satisfies the repository's own stricter parser: decode_spki_der at packages/d2b-resource-compiler/src/lib.rs:2329 requires exactly this shape, the cursor == der.len() check included.
  • The key is genuinely new. The parent body was j1WEGxEqg+v6kj43ai6GChZRU8spFROiOaH/4UHoCNs=, raw 8f 55 84 1b 11 2a 83 eb ...; the head body decodes to f2 dd b2 dc 9b 47 27 b4 95 .... Different 32-byte values.
  • git cat-file -s returns 64 for the sidecar at e1c7e85d5 and 64 at e1c7e85d5^. An octal dump of the tail shows the final octet is 0x5f, not 0x0a, so there is no trailing newline; the 64-octet length also rules out base64, which would be 88 characters. This matches the layout contract that nix/provider-artifact.nix:135 asserts and the test's own length assertion at packages/d2b-resource-compiler/src/lib.rs:2611.
  • Independent confirmation that the pair is real, not just well shaped: openssl pkeyutl -verify -pubin -inkey <committed pem> -rawin -in <committed manifest> -sigfile <committed sig> reports Signature Verified Successfully, and the same check against the manifest with one byte appended reports Signature Verification Failure.

3. No private key material, in the tree or in the range

  • Tracked-file search for BEGIN PRIVATE KEY, BEGIN RSA PRIVATE KEY, BEGIN EC PRIVATE KEY, BEGIN OPENSSH PRIVATE KEY, and BEGIN DSA PRIVATE KEY at this head returns exactly one file, tests/unit/nix/cases/provider-catalog.nix:385, and that is a Nix unit-test fixture string used to assert that a value never contains a private key marker (the same file asserts the negative at line 1266). It is not key material and it is not in this pull request's diff.
  • Tracked-name search for private, *.key, id_rsa, id_ed25519, secret returns only credential-provider crate paths, docs, and changelog fragments.
  • Every path added anywhere in the range 133b78fc4 (exclusive) to e1c7e85d5 (inclusive) was listed; the only key-adjacent additions are credential-provider changelog and audit documents. No key file was added.
  • The consuming code cannot read a private key. compile_inner at packages/d2b-resource-compiler/src/lib.rs:1415 calls keys.resolve_key(entry.publisher(), entry.signature_id()) to obtain the public key, then hands it to verify_ed25519 at line 1467, which passes it to decode_ed25519_spki and then to UnparsedPublicKey::new(&ED25519, ...). There is no branch anywhere in that path that reads, or can read, a private key.
  • The repository's only private-key generation is in tests/host-integration/lib.nix:141 and tests/host-integration/device-worker-launch.nix, which derive a throwaway Ed25519 key from a digest of the fixture binary at build time. That file is not in this pull request's diff and the key never exists in the repository.

4. The new test, read in full

committed_artifact_signature_verifies_over_the_committed_manifest lives at packages/d2b-resource-compiler/src/lib.rs:2600, with its constants at 2551 and 2556 and its root resolver at 2562.

  • Same production code path, not a reimplementation. The test calls verify_ed25519 (lib.rs:2302), which is the identical function the production compiler calls at lib.rs:1467 inside compile_inner. There is no second Ed25519 verifier, no test-only key decoder, and no duplicated ASN.1 walk. The test therefore fails exactly when the shipped compiler would fail.
  • Hermetic. No network. No host absolute path. No working-directory dependence. The only inputs are RUNFILES_DIR and TEST_WORKSPACE (both set by the test runner), D2B_REPO_ROOT (set by the repository's own gate, in Makefile:119 and tests/tools/bazel-check:390), and CARGO_MANIFEST_DIR (set by Cargo for cargo test). The path is read at runtime with std::env::var_os, not baked in with env! or include_str!, so a sandboxed or relocated test binary is unaffected.
  • Reads runfiles, not a source-relative path. repository_root puts RUNFILES_DIR joined with TEST_WORKSPACE first in the candidate list (lib.rs:2565), and the workspace module name is d2b (MODULE.bazel:2). The staged trio then sits at <runfiles>/d2b/packages/d2b-provider-guest-cloud-hypervisor/, which is the path the test joins. This is the correct construction; a concat!(env!("CARGO_MANIFEST_DIR"), ...) style path would have broken under a sandbox, and this does not use one.
  • Each half really moves. If the manifest alone changes, assertion at line 2617 fails, because Ed25519 verification over a different message fails. If the sidecar alone changes, the same assertion fails. If the public key alone changes, the same assertion fails, because a signature made by the old key does not verify under a new key. There is no separate negative probe for the key, but none is needed: the key-moving-alone case is caught by the main positive assertion turning negative, not by a probe.
  • Not vacuous. Two probes guard the assertion. A verifier hardwired to return true would fail the probe at line 2628 (manifest moved, same signature) and the probe at line 2634 (signature moved, same manifest). I confirmed externally that a one-byte manifest change really does break verification, so the probes assert a true property of the primitive, not an accident of the committed bytes.
  • Also asserted: the raw 64-octet sidecar length at line 2611, which is the check the Nix build already made and which caught nothing when the signature went stale.
  • One small residual gap, stated as a coverage limit rather than a defect: the schema file is not read by this test, so a schema-only move does not fail here. It is caught elsewhere, by the manifest's digests.config compared against the installed schema and against the catalog inside compile_inner. And I verified above that the two agree at this head.

5. Bazel wiring, and consumers of the rotated key

  • Both unit-test targets carry the trio. d2b_rust_test(name = "d2b_resource_compiler_test", crate = ":d2b_resource_compiler") at packages/d2b-resource-compiler/BUILD.bazel:86 lists the three labels in data at lines 94, 95, 96. d2b_rust_test(name = "d2b-resource-compiler_test", crate = ":d2b_resource_compiler") at line 104 lists them at lines 112, 113, 114. Both compile the same crate in test mode, which is why the new test needs the data in both, and it has it in both.
  • The labels exist and are exported. exports_files at packages/d2b-provider-guest-cloud-hypervisor/BUILD.bazel:14 lists provider-manifest.json (20), provider-manifest.json.sig (21), and publisher-public-key.pem (22) with visibility = ["//visibility:public"]. The same three files are also in that package's cargo_workspace_sources filegroup, so the labels resolve from either direction.
  • The data declaration really gates the test. In Bazel a test action's inputs include every runfile, and a file named in data is a runfile; changing any of the three changes the action key and forces a re-run. The mechanism is the same one the two targets already rely on for the compiler binary and the schema files. Per the constraints I did not execute Bazel, so this is stated from the action-input semantics of the committed BUILD file rather than from an observed run. I did confirm the target is reachable from the gate: bazel/checks/BUILD.bazel:62 includes //packages/d2b-resource-compiler:all-tests in the aggregate Rust suite, and the package's test_suite(name = "all-tests") picks up both targets because neither sets tests =.
  • No consumer still pins the old key. A whole-tree search at this head for publisher-public-key, provider-manifest.json.sig, the old PEM body, the old body's raw bytes in hex, and the old key file's SHA-256 turns up no stale pin. Specifically: the only production consumer is flake.nix:476, which passes the PEM into providerArtifact, and nix/provider-artifact.nix:223 turns it into signingKey for the catalog. nixos-modules/bundle-zones.nix:308 reads catalog.signingKey or a zone override, so the rotated key flows through with no hardcoded value anywhere. The tests/host-integration/ hits are fixture-local keys, not the committed one. tests/fixtures/provider-volume-acceptance/publisher-public-key.pem is an unrelated acceptance fixture and is not in this diff. The signature sidecar's consumers (nix/provider-artifact.nix:130, packages/xtask/src/provider_packaging.rs:143, nixos-modules/generated/provider-catalog-shape.nix:169, tests/unit/nix/cases/provider-catalog.nix:760) all reference the layout-relative path and the 64-octet length, none of which changed.

6. Regressions across the slice

Nothing provable. What I checked and cleared:

  • No lint was loosened. clippy.toml changes only in the direction of making the parking_lot ban actually fire: the three entries that named an alias are replaced by the resolved lock_api paths they describe, and parking_lot::Condvar::wait correctly stays. deny.toml raises [licenses] confidence-threshold from 0.8 to 0.9, a tightening, and leaves multiple-versions = "warn" and unknown-registry = "deny" untouched; the large added comment block is documentation of known duplicate clusters, not a suppression list.
  • No test was hardened into a tautology. The only new test is the one in question 4, and it carries a positive assertion plus two negative probes.
  • No committed artifact and its gate are out of sync. I recomputed the blocking-census baseline deltas across the whole range: only two rows move, packages/xtask std::fs::read_to_string 11 to 13 and packages/d2b-provider-device tokio::runtime::Runtime::block_on 0 to 1, and both are explained by real code in the range (the census scanner rewrite, and the crate's single #[tokio::test] at packages/d2b-provider-device/tests/device_family.rs:157, whose expansion is the Runtime::block_on the clippy-derived count sees). packages/async-gate-inventory.json shrinks from 251 recorded sites to 195, which is the expected direction for a change that removes blocking work. completions/ moved from free-form file completion to concrete value lists for --signal and --endpoint-class, and those lists match the CLI exactly: ExecKillSignal is a ValueEnum of Term, Kill, Int, Hup (packages/d2b/src/exec.rs:88) and the CLI EndpointClass is a ValueEnum of Service, Device, Transport, Control, Data (packages/d2b/src/endpoint.rs:33).
  • The Nix privilege projection is in step with the wire change it follows. nixos-modules/privileges-json.nix moves destructive from false/true to "no"/"yes" because this pull request changed the field from bool to the Destructive enum, which is #[serde(rename_all = "kebab-case")] with No and Yes (packages/d2b-core/src/privileges.rs:46). The projection now matches the wire form rather than drifting from it.
  • The new code does not trip the repository's own policy checks. The one new #[allow(clippy::disallowed_methods, reason = "cfg(test) helper")] at lib.rs:2599 carries a reason that is on the sanctioned list in packages/xtask/src/provider_crate_policy.rs:456, and the only std::fs::read site is inside that annotated function, which matters because the crate and the workspace both set the lint to deny. The new string literal naming the cloud-hypervisor package does not trip the shared-crate family-knowledge gate, because module_family_signals skips #[cfg(test)] mod blocks and the entire addition lives inside one. The new cross-package data edge does not trip the Bazel visibility gate, which by its own documented scope covers only deps and proc_macro_deps, and the referenced file labels are public in any case.

Findings

None. No defect in this slice met the bar of provable impact, a discrete fix, and introduction by this pull request.

Refuted

Candidates I raised and disproved against the committed head, one line each:

  1. "The rotation silently changed the manifest" - refuted: identical blob OID 378c1546f496da29f9c5204d8887071d0ecb27f0 on both sides of the commit, and the file is absent from git show --name-status.
  2. "The sidecar is base64 or carries a trailing newline" - refuted: git cat-file -s is 64 on both sides and the last octet is 0x5f, not 0x0a.
  3. "The public key was added rather than replaced" - refuted: git show --name-status reports M for the PEM and M for the sidecar; the only addition is the changelog fragment.
  4. "The new PEM is not a well-formed SPKI Ed25519 key" - refuted: the base64 body decodes to exactly the 44-octet SPKI the repository's own decode_spki_der requires, OID 1.3.101.112, 32-octet key.
  5. "The new signature does not verify, so the rotation is cosmetic" - refuted: openssl pkeyutl -verify on the committed triple reports success, and reports failure after a one-byte manifest change.
  6. "The test reimplements verification instead of using production code" - refuted: it calls verify_ed25519 at lib.rs:2302, the same function compile_inner calls at lib.rs:1467.
  7. "The test passes vacuously" - refuted: probes at lib.rs:2628 and lib.rs:2634 assert non-verification and would fail a verifier that accepts everything.
  8. "The test would not fail if only the public key moved" - refuted: a key moved alone makes the positive assertion at lib.rs:2617 fail, since a signature from the old key does not verify under a new one.
  9. "The test is not hermetic or breaks under a sandbox" - refuted: the path is derived at runtime from RUNFILES_DIR plus TEST_WORKSPACE first, with no env! path embedding, no host absolute path, and no working-directory dependence.
  10. "The trio is declared on only one of the two unit-test targets" - refuted: packages/d2b-resource-compiler/BUILD.bazel:94-96 and 112-114.
  11. "The three labels do not exist or are not visible" - refuted: exports_files at packages/d2b-provider-guest-cloud-hypervisor/BUILD.bazel:14 lists all three with public visibility.
  12. "A private key or private-key path is in the tree or in this range" - refuted: the only PEM-private marker in the tree is a Nix test fixture string, no key file exists, no key path was added, and compile_inner only ever resolves a public key.
  13. "A consumer still pins the old publisher key" - refuted: no occurrence anywhere at this head of the old PEM body, its raw bytes, or the old key file's SHA-256; flake.nix:476 and nix/provider-artifact.nix:223 are the only production consumers and both read the committed file.
  14. "The new clippy allow is an unsanctioned suppression that fails the crate policy check" - refuted: cfg(test) helper is on SANCTIONED_ALLOW_REASONS at provider_crate_policy.rs:456, and the only denied-method site is inside the annotated function.
  15. "The new literal naming the cloud-hypervisor package trips the shared-crate family-knowledge gate" - refuted: module_family_signals skips #[cfg(test)] mod blocks and the addition is entirely inside one.
  16. "The new cross-package data edge violates the Bazel visibility policy" - refuted: check_bazel_dependency_visibility is documented to cover deps and proc_macro_deps only, and the referenced file labels are public.
  17. "d2b_resource_compiler_test lost a runtime environment variable the test needs" - refuted: the only reader of CARGO_BIN_EXE_d2b-resource-compiler is tests/cli.rs, which uses env! at compile time and belongs to the cli target, which keeps both env and rustc_env at lines 54 and 58. The change to rustc_env alone at line 98 has no consumer and no effect.
  18. "deny.toml was loosened to hide duplicate-dependency clusters" - refuted: [licenses] confidence-threshold rose from 0.8 to 0.9, and multiple-versions stayed at warn.
  19. "nixos-modules/privileges-json.nix regressed the destructive wire form" - refuted: Destructive is a kebab-case serde enum of No and Yes at packages/d2b-core/src/privileges.rs:46, and the projection now matches it.
  20. "The census baseline was loosened with a fabricated Runtime::block_on site" - refuted: packages/d2b-provider-device holds exactly one #[tokio::test], at tests/device_family.rs:157, and that expansion is the Runtime::block_on the clippy-derived count observes; the introducing commit cites line 179, which is off, but the recorded value of 1 has a real cause. The other moved row, packages/xtask std::fs::read_to_string 11 to 13, matches the scanner rewrite in the range. Separately, the clippy.toml header sentence claiming the lint level currently stands at allow is stale, but that sentence is byte-identical at the base commit, so this pull request does not introduce it.

The Azure Relay ADR named an `xtask gen-provider-transport-schemas`
subcommand in three places and a `make test-drift` gate built on it. No
such subcommand exists and the suite that target resolves to does not
cover this file; the same ADR already states the schema is hand-authored
and embedded verbatim, so the file contradicted itself. Cite the embed
and the crate test that pins the schema instead, and use the real type
name `RelayTransportSettings`, which the same ADR already uses elsewhere.

The remediation plan listed three finding sites under paths that do not
resolve, and described the `labs/` tree as retired while the head carries
171 tracked files under it, its own governing `AGENTS.md`, and a live
prototype. Correct the paths and describe the tree as live.

The signature fragment promised a location and a public fingerprint and
recorded neither, and the fold copies the line into released prose. Drop
the location claim and record the public fingerprint with the convention
that produces it, so a reader can recompute it and pin the key.
@vicondoa
vicondoa force-pushed the refactor-rust-skills-remediation branch from 87b82cf to 05092a6 Compare September 26, 2026 14:50
The persistence fixtures passed a literal 7 as the prepared nonce, which the
code-scanning rule reads as a hard-coded cryptographic value. Both fixtures
now derive the nonce through the crate's existing test_nonce_for_operation
helper, widened to the crate for reuse from the persistence tests. The
rejection test keeps its literal inputs: zero and a valid control are the
values under test there.
The Guest workspace mirror did not declare the `tracing` dependency the
bundle resolver uses, so the static Guest build and the container lane failed
compiling `d2b-core`; the fixture override and the Guest lock now carry it,
and `tests/tools/guest-workspace-drift.py` is clean again.

Applies the fresh review's findings as well: the seccomp-profile re-export
list regains the four public items it dropped (the error type its public
methods return and the three enforcement bounds), the eight documents that
still cite the deleted `d2b-core` compat shim modules are re-pointed at
`d2b-contracts`, the plan accounts for all thirteen `high` rows in its wave-0
disposition, and the broker transport's audit-join refusal describes itself
accurately with its tautological test and now-unused helper removed.
@vicondoa

Copy link
Copy Markdown
Owner Author

Bound to head bf6ee0ec5. The head has since moved to abfe912ec, which applies this record's findings; a fresh round is bound to that head.

Review - slice 1 of 3: docs/ and changelog.d/

binding value
repository vicondoa/d2b (/home/paydro/projects/d2b-v3)
pull request 608
observed base ref origin/v3
observed base OID 133b78fc49c6ca101aaa573c87a5884adad74cb7
reviewed head OID bf6ee0ec57a90a2de1e185f8b05af9f8a8238376
scope git diff --name-only 133b78fc4..HEAD -- docs/ changelog.d/ (166 changelog.d/ fragments, 1 new plan, 27 modified docs)
file count 195

Method: read-only. git rev-parse HEAD matched the expected OID; git fetch origin v3 then git merge-base HEAD origin/v3 re-verified the base. Every claim in the slice was checked against the committed tree with a command, not against other prose: 273 path#Lnnn anchors in daemon-api.md were each resolved to a file and a line; all 166 fragments were validated against changelog.d/README.md's six fail-closed rules; all 139 PascalCase identifiers, 7 constants and 70 Type::member references named in fragments were swept against packages/; every non-description delta in all 16 changed JSON schemas was traced to its Rust definition; and the slice was scanned for the repository's hard rules on shipped text. .scratch/ and the PR's existing review comments were not read.

Findings

1. Live docs still cite the six d2b-core compat shim modules this patch deletes

Location. docs/reference/tracing-contract.md:151, plus 23 further citations across seven docs/specs/ files: ADR-046-cli-and-operations.md:68; ADR-046-current-code-migration-map.md:779; ADR-046-nix-configuration.md:859,2494,2793; ADR-046-resources-device.md:1046,2015,2085,2133; ADR-046-resources-host-guest-process-user.md:2319,2323,2325,2362,2499; ADR-046-resources-zone-control.md:4404,4509,4696,4714,4932; ADR-046-telemetry-audit-and-support.md:40,49,1952,2207.

Defect. The patch deletes all six one-line compat shim modules in d2b-core, but 8 live documentation files still name four of those paths as the current package for live symbols - in tables whose own column headers are present-tense (Current baseline symbol, Current source, Live in resolver) and whose rows are marked implemented-and-reachable. All 24 cited paths no longer exist at this head. The symbols themselves survived; they moved to d2b-contracts, exactly as the patch's own changelog.d/w5-03-core-shim-deletion.md records - so every one of the 24 is a dead path pointing at a live type. The sharpest instance is docs/reference/tracing-contract.md:151, an actionable instruction in a document whose header reads Status: codified in the daemon and broker owner-local tests and generated artifact checks. AGENTS.md makes this a defect: "Existing code is canon. When a plan, specification, README, or reference document disagrees with committed, passing code, keep the code and document the drift." The patch already re-pointed code and one generated doc for the same move, so only the documentation half of the sweep is incomplete.

Evidence.

  • git diff --name-status 133b78fc4..HEAD -- packages/d2b-core/ shows D for error.rs, contract_id.rs, configured_argv.rs, privileges_w3.rs, workload_identity.rs, unsafe_local_workloads.rs - six deletions, matching the fragment's "six one-line compat shim modules". Base contents were one-line re-exports; git show 133b78fc4:packages/d2b-core/src/workload_identity.rs is exactly pub use d2b_contracts::workload_identity::*;.
  • Citation census: grep -rcE 'd2b-core/src/(error|privileges_w3|workload_identity|unsafe_local_workloads)\.rs' docs/reference/*.md docs/specs/*.md docs/specs/providers/*.md sums to 24 across 8 files.
  • docs/reference/tracing-contract.md:151 - (packages/d2b-core/src/error.rs) and the broker audit log,
  • docs/specs/ADR-046-telemetry-audit-and-support.md:40 - |`WorkloadIdentity` / `WorkloadTarget` / `RealmTarget` (`d2b-core/src/workload_identity.rs`)|Zone self-resource reference `Zone/<zone_name>`|implemented-and-reachable|
  • docs/specs/ADR-046-cli-and-operations.md:68 - |`WorkloadId`, `WorkloadTarget` (`= RealmTarget`)|`d2b-realm-core/src/ids.rs`, `d2b-core/src/workload_identity.rs`|...|, under the table header at :60 |Current baseline symbol|Package|Target name|.
  • Current homes confirmed live: WorkloadTarget at packages/d2b-contracts/src/workload_identity.rs:55, WorkloadIdentity at :71, UnsafeLocalLauncherItem at packages/d2b-contracts/src/unsafe_local_workloads.rs:248, Error::all_kinds at packages/d2b-contracts/src/error.rs:641, W3BrokerOperation at packages/d2b-contracts/src/privileges_w3.rs:28.
  • None of the 8 files is in this patch (git diff --name-only 133b78fc4..HEAD -- <each> empty for all 8).

Fix. Re-point all 24 citations to the canonical homes the patch's own fragment already names: d2b-core/src/error.rs -> d2b-contracts/src/error.rs, and likewise for privileges_w3.rs, workload_identity.rs, unsafe_local_workloads.rs. Where a cited symbol was the shim-local UnsafeLocalWorkloadIdentity alias, point at d2b_contracts_resource::v3::ZoneResourceIdentity per changelog.d/w5-03-core-shim-deletion.md. One sweep over the 8 files suffices.

2. The new plan's wave-0 disposition enumerates 12 of the 13 high rows it says it disposes of

Location. docs/plans/2026-09-24-002-refactor-rust-skills-remediation-plan.md:155, against :38 (R2) and the Appendix U1 row.

Defect. U1 step 3 opens "Dispose of the 13 high rows" and then enumerates 4 test rows + 2 wire-digest panic rows + 1 provider-wayland-policy caller-input panic + 4 executor-blocking rows = 11 applied, plus 1 recorded as a policy-confirmed no-op = 12. R2 and the Appendix both put the high set at 12 actionable + 1 policy-confirmed = 13, and the Appendix's own breakdown lists 3 wide high rows that step 3 never mentions. A reader following the disposition step cannot tell which high row it handles, and the wave-0 close-out cannot be reconciled against it. Both offending statements are in the same new file, so the correction is local.

Evidence.

  • :155 - "3. Dispose of the 13 high rows, re-verified per R3: apply the four test rows, the two wire-digest panic rows, the provider-wayland-policy caller-input panic at its driver-args boundary, and the four executor-blocking rows ...; record the provider-user blocking-NSS row as a policy-confirmed no-op citing its policy (R14, KTD8); escalate the remaining member sites of the shared driver-args class to U5, with the escalation recorded." (4+2+1+4 = 11 applied, +1 no-op = 12, against the stated 13.)
  • :38 (R2) - "the 12 actionable high rows land before their lenses' bulk work, and the thirteenth (policy-confirmed) is recorded as a no-op in the same wave."
  • Appendix U1 row - "| U1 | cross-cutting: the 13 high rows - 8 leaf (7 actionable plus the policy-confirmed row recorded as a no-op), 2 family (the driver-args class, applied at its panicking member and escalated for the remaining sites), 3 wide | 13, a subset |". 8+2+3 = 13 and 7+2+3 = 12 actionable are internally consistent; only step 3's enumeration is short, and the 3 wide it omits appear nowhere else in the approach.

Fix. Amend U1 approach step 3 so its enumeration covers all 13 rows - naming the 3 wide high rows explicitly alongside the 11 already listed - or correct "the 13 high rows" to the count actually enumerated and cross-reference the Appendix row for the remainder.

Verdict

request-changes

Finding 1 is a documentation defect against committed code introduced by this same patch, in the repository's own authority documents, and AGENTS.md requires the drift be closed. Finding 2 is a low-severity internal contradiction in a newly added plan. Nothing in the slice contradicted the committed tree in the other direction, and the majority of the slice verified clean: all 273 line anchors in daemon-api.md resolve to a line containing the named symbol; all 166 fragments satisfy every fail-closed rule in changelog.d/README.md; all 16 changed JSON schemas match their Rust definitions; the plan's paths, make targets, crate count and finding arithmetic all resolve; and the slice is free of non-ASCII dashes, agent/tool/model attribution in release prose, and secrets, hostnames, user identifiers or network ranges.

Reconciliation

Candidates examined and dismissed, with the evidence that dismissed them.

  1. W6UsbipOperation and "ledger" in fragments read as leaked wave/planning markers. Both false: "The ACA Guest completed-operation ledger evicts its oldest entry by record sequence" is a product noun, and packages/d2b-broker/src/ops/usbip_firewall.rs:60 declares pub enum W6UsbipOperation, so the fragment names a type. A sweep for wave/lane/plan/ledger/finding vocabulary over all 166 fragments returned only these two lines.
  2. The new plan ships ce-plan-bootstrap / ce-work / ce-code-review mode:agent. artifact_contract: ce-unified-plan/v1 is byte-identical to the precedent plan's frontmatter (docs/plans/2026-09-24-001-...:2), product_contract_source is a field of that same contract, and ce-code-review mode:agent is quoted from AGENTS.md's own mandated profile. AGENTS.md forbids model/tool/agent attribution; these name the repository's required workflow, and no changelog fragment carries any such reference.
  3. Six PascalCase identifiers absent from packages/. All six (ActivationRunnerInputError, AzureVmStatus, AzureVmUpdate, Reconfiguring, StatusServicesOutputV3, UnsafeLocalWorkloadIdentity) appear in fragments as removals, and the corresponding deletions are real in the diff.
  4. docs/explanation/over-engineering-audit-record.md:457 and the ponytail lane files say the six shims "still exist at HEAD". The record states it was verified against 515cbf610 (:18-21), so "HEAD" means that revision; the lane files are the unit of record for that completed pass. Point-in-time records, and none is in this diff.
  5. Missed stream_admission.rs -> admission.rs rename sweep in ADR-046-provider-notification-desktop.md:84,1394,1517. The patch renames only the src module; no tests/stream_admission.rs exists at base or head, and the same listing at :83 also says stream_redaction.rs where the real file is redaction.rs, under a row marked Implementation state | Planned. The four citations the patch fixed (:76, :671, :673, :1390) are exactly the src-module and Destination-cell references. Pre-existing drift, not introduced.
  6. Plan counts, gates and paths. All resolve: every referenced path exists; check/test-integration/generate/test-host-integration/check-census/changelog-fold are all in the Makefile; disallowed_methods = "deny" (Cargo.toml:167) against clippy.toml's stale "it stands at allow" is exactly as the Sources section says; 923+25+17 = 965; the per-wave lens sums check out; and "the 94-crate workspace" matches the members count of 94.
  7. "Seven changelog fragments no longer open with a filename title line" in audit-review-remediation.md - not verifiable: all 166 fragments are added whole, so no pre-edit state exists to count. The end state is verifiably correct (every fragment's line 1 is an allowed ### heading). Recorded as not verifiable; no defect to act on.
  8. Claimed fixes and digests. audio-vm-not-found=2 and audio-not-enabled=70 match packages/d2bd-runtime/src/typed_error.rs:857,858,922,926; the manifest-parse-error claim matches the test at packages/d2b-core/src/bundle_resolver.rs:8147-8177; the azure-relay "no generator owns the file" text is accurate (gen-provider-transport-schemas exists nowhere outside docs/, and RelayTransportSettings::schema_json is at src/transport_settings.rs:73); the auth-status shared-list claim matches packages/d2bd/src/composition.rs:23265; the blocking-census 0->1 claim matches packages/xtask/data/blocking-census-baseline.json; and the published SPKI digest matches byte-for-byte (openssl pkey -pubin -outform DER | sha256sum = d798fa9f...17b57, sidecar 64 bytes).
  9. Shipped-text hard rules. No U+2010/2011/2012/2013/2014/2015/2212/FE58/FF0D anywhere in the slice; the plan is pure ASCII; no non-ASCII on any added line of the 27 modified docs; the only non-ASCII in changelog.d/ is in the 589-* fragments, which are not in this diff. No IPv4 literal, /home/<user> path, or hostname-shaped token in any new file or added line; no secret material (the only regex hits are schema test names such as secret_key_field_rejected).
  10. Fragment grammar. All six fail-closed rules in changelog.d/README.md hold for all 166 fragments: only the six allowed ### headings, first line always a heading, no repeated heading, no empty fragment, every section non-empty and bullet-led. The apparent "first body line is not a bullet" hit is the blank line the README's own example format uses.
  11. Generated artifacts. All 273 path#Lnnn anchors in daemon-api.md resolve to a file plus a line containing the named symbol (zero MISSINGFILE, zero MISALIGNED). Every non-description schema delta traces to its Rust: CommandArgvSlot/CommandExec, NmReloadBehavior, Destructive "no"/"yes", ConfigurationGeneration, the HelperFailureCode oneOf split, the SharedAccessSignature not-pattern, PublicReadModelKind, QemuMediaRegistryState, QemuMediaRunnerState, VmAutostartMode, RealmMode, RealmGatewayState. The mangled FdKind description in wire-protocol.json is repaired by this patch, not introduced.
  12. Cross-references. Every relative markdown link in all 28 changed .md files resolves; the two error-codes.md links are pre-existing; the 0018 anchor matches the heading at docs/adr/0018-microvm-nix-removal.md:661. The plan's StatusServicesOutputV3 removal is correct - the type is absent from packages/, so the migration guide's rewrite is accurate.
  13. Fragment-named members and paths. All 7 constants, and every sampled Type::member and file reference, resolve: ResolvedTarget::matches_assignment (d2b-resource-client/src/target.rs:412), ConsoleSessionTable::ring_notify/read_output, CredentialRuntime::dependency_facts, GuestLocalError::EndpointMismatch, HelperSnapshot::validate/HelperLaunchRequest::validate_bounds (both pub(crate), matching "now crate-internal"), LaunchedObserver::launched, StartupPlan.steps, clipd_host::audit::AuditEvent, BusId::new/as_str, AudioLastSetApplied::NotApplied, and credential-already-running. Bare filenames needing context all resolve to real files or emitted documents.

@vicondoa

Copy link
Copy Markdown
Owner Author

Bound to head bf6ee0ec5. The head has since moved to abfe912ec, which applies this record's findings; a fresh round is bound to that head.

Review - slice 2 of 3: daemon, control-plane, contracts, runtime and build-tooling

Bindings

Field Value
repository /home/paydro/projects/d2b-v3
PR 608
observed base ref v3 (origin/v3 re-fetched)
observed base OID 133b78fc49c6ca101aaa573c87a5884adad74cb7 (re-verified as git merge-base HEAD origin/v3)
reviewed head OID bf6ee0ec57a90a2de1e185f8b05af9f8a8238376
scope git diff --name-only 133b78fc4..HEAD -- . ':(exclude)docs/' ':(exclude)changelog.d/' ':(exclude)packages/d2b-provider*' ':(exclude)packages/d2b-sk-frontend' ':(exclude)labs/'
file count 462 files, +26404 / -16618

Method: read the scoped diff, then read the tree at both revisions for every candidate - the base side via git show 133b78fc4:<path> - and reported only defects substantiated by a quoted source line or a command's output. No build, test, formatter or linter was run; this pass is source-level only. .scratch/ and the PR's existing review comments were not read.

Findings

1. malformed_audit_join_material_does_not_panic cannot fail, and the refusal arms it covers are unreachable

Location. packages/d2bd-runtime/src/broker_transport.rs:360-367 (the test, added by this diff) and :73-77 (the two map_err arms it purports to exercise).

Defect. default_audit_join_context gained a typed-refusal arm for a malformed audit join, and the new test asserts the daemon survives one. The premise is false: authoritative_audit_join hashes whatever the request carries, so the client-supplied opaque id is never the value that reaches CanonicalAuditDigest::parse. The assertion is true by construction, the two map_err arms cannot be taken, and the test's own comment asserts a fact about the wire that the code disproves.

Evidence.

packages/d2bd-runtime/src/broker_transport.rs:360-367
    #[test]
    fn malformed_audit_join_material_does_not_panic() {
        // The audit-join material is wire-supplied (a client-controlled
        // opaque id), so a non-canonical value must never panic the daemon.
        let request = request_with_opaque_id("not-a-canonical-digest");
        let context = default_audit_join_context(&request)
            .expect("malformed audit-join material must not panic")
            .expect("audit join is present for secret requests");
        assert!(context.zone_id.as_str().starts_with("sha256:"));
    }

Introduced by this diff, not pre-existing:

$ git show 133b78fc4:packages/d2bd-runtime/src/broker_transport.rs | grep -n "malformed_audit_join"
(no match)

The hashed-output proof, packages/d2b-contracts-broker/src/broker_wire.rs:825-832:

        Some((
            d2b_contracts_resource::v3::canonical_digest("d2b:broker-zone:v2", scope.as_bytes()),
            d2b_contracts_resource::v3::canonical_digest(
                "d2b:broker-operation:v2",
                operation.as_bytes(),
            ),
        ))

canonical_digest (packages/d2b-contracts-resource/src/v3/resource_schema.rs:548) ends in digest_bytes (:604), which pushes the literal "sha256:" at :609 and then 64 lowercase hex nibbles; is_canonical_digest (:558) is its exact inverse, and CanonicalAuditDigest::parse (broker_wire.rs:2876-2883) returns Ok precisely when is_canonical_digest holds. Hence starts_with("sha256:") cannot be false, and the arms at broker_transport.rs:73-77 are dead.

Worth stating plainly so the finding is not read as broader than it is: the production half of this change is a strict improvement. The same diff replaced two .expect("canonical broker zone digest") panics on the daemon dispatch path with these typed refusals. The defect is the dead test and the dead arms shipped as if they were coverage.

Fix. Either drop the unreachable surface - remove both map_err arms and the test, since the sibling request_without_audit_join_yields_none already covers the one reachable branch (Ok(None)) - or keep the arms as defence-in-depth and retarget the test at the boundary that is client-reachable: DispatchAuditContext::from_request_with_join in packages/d2b-broker/src/runtime.rs, which refuses a supplied-versus-recomputed join mismatch and a missing join for a request that requires one. If the arms stay, the doc comment on default_audit_join_context must stop describing a malformed client-supplied join as a reachable input.

Verdict

request-changes

One substantiated finding, and it is P3: a test that cannot fail plus two unreachable refusal arms. It is a real, actionable, introduced defect in the categories this slice is chartered to catch, so it is not an approve. It is not a correctness bug, a contract or wire break, or a weakened gate - the gate and ratchet surfaces in this slice held under every check I ran, and the wire and privilege-plane changes are symmetric on both ends. The author may reasonably weigh it as a follow-up rather than a merge blocker; I flag it because the repository's own rule (AGENTS.md: "Do not hide a failing assertion by deleting it; fix its predicate or message") makes a test whose predicate is a tautology a defect, not a style note.

Reconciliation

Candidates I examined and dismissed, with the evidence that dismissed each.

Build and policy surface

  1. packages/policy-inputs/** closure drift (72 files). Dismissed. The generated lockSha256 values match the files the generator actually hashes: sha256sum Cargo.lock = c81d3e88... matches the main-product metadata, and sha256sum packages/Cargo.guest.lock = a67f038b... matches guest-static, consistent with production_closure.rs:24 (PRODUCT_LOCK = "Cargo.lock") and :305 (lock_path: "packages/Cargo.guest.lock"). The new d2b-resource-client@0.0.0-bootstrap#path row and the d2b-provider-guest-cloud-hypervisor -> d2b-resource-client edge are present in packages/Cargo.guest.lock at :1774 and :1269. Both targets (x86_64-unknown-linux-musl, aarch64-unknown-linux-musl) received the identical six-file change.
  2. packages/xtask/data/blocking-census-baseline.json baseline widened (284 apparent raises). Dismissed. After normalizing the parking_lot:: -> lock_api:: key rename that clippy.toml forces (282 removed keys, 282 added, all at 0), exactly two real deltas remain: packages/d2b-provider-device / tokio::runtime::Runtime::block_on 0 -> 1, which is the provider slice, not mine; and packages/xtask / std::fs::read_to_string 11 -> 13, which is in the CLI-only xtask crate and matches the two new fs::read_to_string sites this diff adds (blocking_census.rs check_against_baseline, gen_broker_operations.rs). No lock entry and no spawn_blocking entry was raised anywhere.
  3. packages/xtask/data/async-gate-inventory.json shrank 251 -> 195 sites, i.e. dropped coverage. Dismissed, and the ratchet tightened. 56 rows removed, 0 added, and the gate validates in both directions (async_gate.rs: "a recorded site whose file no longer carries the marker fails too"), so a stale removal turns the gate red rather than green - it is fail-closed. The one removed row in my slice was verified against the source: packages/d2bd/src/resource_runtime.rs:2509 at base carried *sink.lock() = Some(desired_status); // async-gate-allow: ... and at HEAD only the two markers at :7500 and :8651 remain, so the marker itself is gone.
  4. production_closure.rs production_kinds() weakened by the EdgeKind enum. Dismissed - it is the opposite. It still returns [EdgeKind::Normal, EdgeKind::Build, EdgeKind::ProcMacro] (:75-79), excluding Dev/Test/Example/Bench; the test was retyped to the new enum (:1818-1821) rather than relaxed, and EdgeKind::parse rejects an out-of-vocabulary spelling rather than dropping the edge. I also confirmed the committed closure.json edge order is unchanged base-to-head: sorted by (from, to, kind) in string order, matching at both revisions (enum-order match: False at both).
  5. blocking_census.rs baseline check weakened by the --json-out refactor. Dismissed after reading both arms. At base the baseline check ran unconditionally after the write; at HEAD it runs inside the json_out branch using baseline_record.crates (the same data mem::take moved out of crates), or in the else if branch against the untouched crates. All four flag combinations behave identically, and the comparison predicate *count > committed_count is byte-identical.
  6. collect_self_binding_scope lost a pending-state reset in provider_crate_policy.rs. Dismissed. The first arm (inner.contains("}") && inner.contains("SeedSelfBinding")) is still present at HEAD :6670. The change is that the closing-brace arm now breaks instead of clearing and continuing (:672-675). That is a tightening: continuing past a } line could attribute a later, unrelated subject_ref:/role_ref: pair to the closed SeedProvider, producing a false self-binding-subject-escape. The two }-on-its-own-line shapes reachable in valid Rust (a vec![SeedSelfBinding { ... }] row closing as }], and a bare trailing }) are both still scanned through to the SeedProvider close, and the three fixtures at :10786, :10815, :10828 still cover the three layouts.
  7. New gate added, violating "Do not add a linter, formatter, pre-commit hook, new overlay". Dismissed. git diff --name-status --diff-filter=A over my scope returns exactly one added file, packages/d2b-core/tests/storage_lifecycle_schema.rs, a test inside the existing suite. tests/unit/meta/rust-main-packages-suite-guard.sh is pre-existing and not in the diff.
  8. deny.toml confidence-threshold 0.8 -> 0.9 loosens licence detection. Dismissed. Higher is stricter. multiple-versions stays "warn", so the added accepted-clusters comment block documents known clusters without changing enforcement.
  9. privileges-json.nix destructive: false -> "no" breaks the boolean-typed consumer. Dismissed. The Nix side and the Rust side moved together, and I verified the whole facet set: extracting (operation, destructive) from the Nix brokerOperations JSON block (:812-1774) and from docs/reference/policy/broker-operations.json, no operation name appears on both sides with differing values - the Nix-only names are exactly the 12 hostActivationVerbs in the closed allowlist of tests/unit/nix/surfaces/privileges-json-drift.nix, and the catalog-only names are EnvelopeInvoke/PublishTrustedContext plus the family-declared kebab-case rows.
  10. clippy.toml parking_lot::* -> lock_api::* weakens the lock deny list. Dismissed on the stated alias reason plus the effect: the deny list is unchanged in coverage, and the generated broker_operation_authz.rs / broker_operation_catalog.rs remain at 0 for every lock_api::* entry.
  11. completions/* hand-edited rather than regenerated. Dismissed. _default -> (service device transport control data) is clap_complete's ValueEnum rendering, and the five values match EndpointClass at packages/d2b/src/endpoint.rs:33-39 exactly. The --signal values match the KillSignal variant set.

Contracts and wire shape

  1. OperationAuthz.destructive: bool -> Destructive enum breaks the privileges.json wire. Dismissed after tracing every producer and consumer. Nix emits "no"/"yes" (privileges-json.nix, 143 lines rewritten); the Rust type is the enum (packages/d2b-core/src/privileges.rs:34-48, #[serde(rename_all = "kebab-case")]); docs/reference/schemas/v2/privileges.json moved in lockstep ("type": "boolean" -> $ref: #/definitions/Destructive); and the fuzz corpus moved in lockstep (corpus/privileges/03-enum-mismatch.json, false -> "no"). The boolean literals in packages/d2b-provider-*/operations.json feed a different type - packages/d2b-broker/src/catalog.rs:96 pub destructive: bool - and I verified the generated authz table against that catalog across all 98 rows on all four facets: gen 98 cat 98 facet problems 0.
  2. docs/reference/schemas/v1/privileges.json still declares destructive as boolean while the Rust type is now an enum. Dismissed. No gate references schemas/v1 (git grep -rn "schemas/v1" -- packages/xtask/src tests/ bazel/ returns nothing), and the artifact the daemon reads is the v2 one nixos-modules/privileges-json.nix emits (schemaVersion = "v2"). v1 is a frozen historical snapshot, not a live contract.
  3. NmReloadBehavior closed enum rejects a valid host artifact. Dismissed. nixos-modules/host-json.nix:22 emits nmReloadBehavior = "atomic-reload", inside the closed set; flake.nix:883 emits "none", also inside it; and docs/reference/schemas/v2/host.json gained the matching NmReloadBehavior definition. The one fixture change (tests/fixtures/deny-unknown/host-valid.json, "nmcli general reload" -> "atomic-reload") was required by the new enum, not a re-baseline to dodge it.
  4. ManifestV04 broken without a manifestVersion bump (the AGENTS.md manifest rule). Dismissed. git diff 133b78fc4..HEAD over my scope returns no added or removed manifestVersion line; the value stays 6 everywhere.
  5. MutationFlags restructure lets a request with neither dryRun nor apply execute. Dismissed after mapping every call site. MutationMode::from_flags(false, false) is Err and TryFrom maps it to mode: None (public_wire.rs:402); mutating_verb_preflight refuses None with the structured MutatingVerbOutcome::InvalidRequest envelope (composition.rs:10259-10282); the raw-frame path refuses separately via mutation_mode_from_request (:10232-10249). I enumerated the nine typed preflight sites and their verbs - usb attach/detach, vm start/stop/restart, host prepare/destroy/reconcile, and dispatch_broker_activation (:20698) - covering all six request types that flatten MutationFlags. The one .unwrap_or((false, false)) I found (next_provider_lifecycle_operation_id, :19131) feeds a dedup fingerprint, not admission, and is unreachable for a refused request.
  6. AuditPageEnd variant silently dropped by a consumer. Dismissed. Both ends are explicit: public_wire.rs:2320 defines it, packages/d2b/src/dispatch.rs:569-570 matches Complete and More(next_cursor), and packages/d2bd-runtime/src/wire.rs:475 constructs it.
  7. New TypedError audio variants unhandled in the wire mapping. Dismissed. AudioVmNotFound and AudioNotEnabled are branched in kind() (:857-858), exit_code() (:922-926), message() (:1068-1072), remediation() (:1211-1217) and ErrorEnvelope (:1794-1842). Their exit codes (2 and 70) reuse the existing "target not found" and "capability refusal" conventions with the rationale in comments.

Daemon and control plane

  1. ResourceQuery.assignment/scope merged into one tuple drops the both-or-neither validation. Dismissed - it is the opposite. The base's two independent Options plus a validate_scoped that rejected the mixed state are replaced by a single Option<(AssignmentIdentity, ScopedResourceScope)> (router.rs:220), which makes the invalid state unrepresentable; validate_scoped is now Ok(()) for None because None is the unscoped state.
  2. state_cells.rs key-layout change loses the cross-principal record. Dismissed after reading consume_locked and complete_locked (:1068-1148). Moving principal from the map key into the record preserves the property: a record for the same (cell, invocation_id) under a different principal returns ConsumeDecision::ForeignPrincipal (:1104) and is never overwritten or freshly granted, and complete_locked returns ForeignPrincipal likewise. The new layout additionally cannot hold two principals' records for one invocation id, so it is strictly safer. DurableRecord.outcome moving from String to CellOutcome keeps the same wire spellings via #[serde(rename_all = "lowercase")] -> unknown/completed.
  3. authority.rs removed the external-NIC authority surface and left callers behind. Dismissed. EXTERNAL_PHYSICAL_NIC_IDENTITY_DOMAIN, EXTERNAL_PHYSICAL_NIC_AUTHORITY_CLASS, ResolvedExternalNicIdentity, TrustedExternalNicInventory and ExternalNicOwnerProof return zero remaining references repo-wide; admit_external_nic_claims, ExternalNicAdmissionError, ExternalNicAuthorityStatus and ExternalNicClaim all moved to packages/d2b-contracts-resource/src/v3/network.rs and every call site was migrated.
  4. validate_usb_busid removed from the QemuMedia attach/detach daemon path. Dismissed on where the validation went, not on whether it left. The two removed blocks (composition.rs, dispatch_broker_qemu_media_attach / ..._detach) were defence in depth; the authoritative check remains at the broker, packages/d2b-broker/src/ops/media.rs:259, :457 and :506, each d2b_host::media::BusId::try_from(req.bus_id.as_str()) returning MediaOpError::InvalidBusId, and BusId::try_from is the caller of validate_usb_busid (packages/d2b-host/src/media.rs:133). An invalid bus id is still refused, by the privileged owner rather than the daemon. I also confirmed no test covered the removed daemon-side check at base (git show 133b78fc4:...composition.rs | grep "invalid USB busid selector" matched only the two production lines), so no coverage was deleted. The user-visible envelope class does change from invalid-request to a broker error; I judged that a deliberate consequence of this diff's consolidation onto typed broker errors rather than an unintentional regression.
  5. bundle_resolver field-to-accessor change alters resolution. Dismissed. bundle(), host(), manifest() and storage() (bundle_resolver.rs:1688-1721) are plain borrows of the same private fields; the change is encapsulation, not behavior.
  6. redacted_debug! conversion leaks a payload. Dismissed after reading the macro (packages/d2b-contracts-resource/src/v3/execution_policy.rs:23-86). Each field is rendered by an explicit redacted_debug_field_ref (borrowed field) or redacted_debug_field_value (closure result); every identifier field in the eight converted types is |_| "<redacted>" or a count, the borrowed fields are the non-sensitive verb, provenance and bootstrap_phase, and the variants: arm renders payload-bearing variants as Type::Variant(<redacted>). The rendered output matches the hand-written impls it replaced.
  7. owner_connection_test_hook leaks a wire field into production. Dismissed. The module is #[cfg(test)] (composition.rs:491), so the ownerConnectionHookClaim field only ever reaches a test frame.
  8. #[cfg(test)] helper visibility widened (test_nonce_for_operation, authority.rs:40, now pub(crate) for authority_persistence.rs:341). Dismissed as not binding. The repository's rule (docs/contributing/gates-and-lints.md:283-288) scopes the overbroad-public-API check to cargo hawk over production targets; a #[cfg(test)] item is not a production target, so no repository gate governs it. The call is a legitimate cross-module test fixture, and the value is now derived from the operation id rather than the hard-coded 7 it replaced.

Repository rules, mechanical sweeps

  1. Non-ASCII dashes in shipped artifacts. Dismissed. git diff 133b78fc4..HEAD over my scope piped through grep -P '^\+.*[\x{2010}-\x{2015}\x{2212}]' returns no match. AGENTS.md:292-294 names U+2010-2015, U+2212, U+FE58 and U+FF0D, all inside the searched range.
  2. Agent/tool/model attribution or finding markers leaked into shipped source. Dismissed. The same diff piped through a case-insensitive pattern for claude|chatgpt|gpt-4|gpt-5|codex|copilot|gemini|as an ai|language model|\bLLM\b|reviewer round|round \d returns no match.
  3. Real network ranges committed. Dismissed. The only added literals are 10.20.0.0/24 and 10.0.0.1, both RFC1918, which AGENTS.md:285-286 names as the sanctioned form ("Use generic names and RFC1918 or RFC5737 examples").
  4. A retired knob was made functional again. Dismissed. D2B_LEGACY_BASH_OPT_IN, D2B_LEGACY_CLI and D2B_NATIVE_ONLY appear in the diff only in a doc comment and in tests that set them to prove they are inert.
  5. New unsafe block under unsafe_code = "forbid". Dismissed. The only unsafe token on an added line in my scope is the word inside a doc comment ("unresolved or unsafe image sources"). packages/d2b-unsafe-local-helper changed four files and introduced no unsafe block.
  6. bundle_resolver_tamper.rs newly #[ignore]d a test - deleted coverage. Dismissed; it is the reverse. The base silently returned early when getuid() != 0; the diff replaces that with #[ignore = "root-only: requires CAP_CHOWN to chown the bundle file"] and documents running it with --ignored. The skip becomes visible in test output instead of passing vacuously.
  7. changelog.rs crash-recovery hook weakened by the macro hoist. Dismissed. crash_if_hooked! moves from two function-local definitions to one module-scope macro taking the hook as a parameter, and every call site passes the same RecoverStage and the same message string as before. The rest of the changelog.rs and delivery/* diffs are # Errors doc comments and per-field doc comments satisfying missing_docs.

Not verifiable in this pass, stated plainly. I did not run any build, test, linter or formatter, so I make no claim about whether the tree compiles or the suite passes. Two wire_deserialize! families (contracts-provider credential/provider, and roughly ninety sites across contracts-resource and contracts-zone-session) were sampled for casing agreement rather than compared field-for-field, and the 36 members of V3_CONVERTED_RESOURCE_TYPES were not diffed against every packages/d2b-provider-*/resource-types.json declaration. Those gaps are contract-surface risk, not a claim that a defect exists.

@vicondoa

Copy link
Copy Markdown
Owner Author

Bound to head bf6ee0ec5. The head has since moved to abfe912ec, which applies this record's findings; a fresh round is bound to that head.

Review - slice 3 of 3: Provider and prototype side

Bindings

Field Value
Repository /home/paydro/projects/d2b-v3
PR 608
Observed base ref v3
Observed base OID 133b78f
Reviewed head OID bf6ee0e
Scope git diff --name-only 133b78fc4..HEAD -- packages/d2b-provider* packages/d2b-sk-frontend labs/
File count 435

Method

Read AGENTS.md, docs/contributing/*.md, tests/AGENTS.md as binding sources and judged every candidate against the repository's own rules; git rev-parse HEAD returned the reviewed head and git fetch origin v3 && git merge-base HEAD origin/v3 re-derived the base OID independently. All evidence below is from git diff/git show/git grep/sha256sum/openssl/direct file reads; no file in the repository was created, edited, moved or deleted, and no linter, build or test suite was run. .scratch/ and the PR's existing GitHub review comments were deliberately not read so that every finding is independently derived. Three read-only sub-investigations were run in parallel; where a sub-investigation had no shell access I re-ran its scope myself rather than accepting an unverified lead.

Findings

1. Provider manifest trust and compatibility admission lost its only production caller

Location. packages/d2b-provider-provider/src/providers.rs - plan_external deleted; the surviving replacement plan_observed is at lines 118-133. The trust gate it used to call is ProviderManifest::admit at packages/d2b-contracts-provider/src/v3/provider.rs:2663-2672.

Defect. admit() is the one production entry point that evaluates TrustEvidence and the compatibility range. Deleting plan_external - which contained manifest.admit(required_api_major, required_api_minor, required_descriptor_fingerprint).map_err(|_| ProviderError::TrustOrCompatibilityDenied)? - leaves admit() with no production caller. At HEAD every call site is inside #[cfg(test)] code. The four production gates that still receive a ProviderManifest all call validate_installation_contract() only, which is the graph/well-formedness check, not trust and not compatibility:

  • packages/d2bd/src/provider_registry.rs:338-341 deploy_target_local_controllers, documented as taking "one signed Provider manifest"
  • packages/d2b-core-controller/src/controller_assignment.rs:967-976 from_signed_manifest, documented as "Derive one role contract from a trusted signed Provider manifest"
  • packages/d2b-provider-toolkit/src/declaration/manifest.rs:80-82 validate_for_installation
  • packages/d2b-resource-compiler/src/lib.rs:1480 the compiler

plan_observed no longer takes a manifest at all, so it structurally cannot check trust. The new doc comment asserts that "Manifest admission remains the authority ... after those facts have been established by the runtime", but no runtime component establishes them - both surviving from_manifest paths derive directly from the manifest while checking only the installation contract. That comment documents an invariant the tree does not implement.

TrustEvidence::admit (provider.rs:491-500) is a fail-closed conjunction over publisher_trusted && signature == Valid && revocation == Clear && !emergency_deny && provenance.admits() && sbom.admits() && license.admits() && vulnerability.admits() && conformance.admits(). git grep 'SignatureState::Valid|publisher_trusted|emergency_deny' outside the contracts crate returns only fixture constructors (including a hardcoded true/Valid/false at packages/d2b-bus/src/authorization.rs:813) and test files; nothing evaluates the fields.

Evidence.

  • git diff 133b78fc4..HEAD -- packages/d2b-provider-provider/src/providers.rs removes plan_external, the ProviderError::TrustOrCompatibilityDenied variant and its "provider-admission-denied" code.
  • git grep -n '\.admit(' HEAD -> admit() is called at provider.rs:2669 (its own body) and only in tests: provider.rs:3659, 3675, 4197, 4199, 4219, d2b-provider-toolkit/tests/fake_provider.rs:251, d2b-provider-toolkit/tests/malicious_provider.rs:305, 318, 322, 326.
  • git grep -n 'trust\.admit()' HEAD -> only provider.rs:2669 and provider.rs:3675 (a test assertion).
  • git grep -n 'compatibility\.admits' HEAD -> only provider.rs:2612, 2671, 2708, i.e. only inside admit().
  • git grep -n 'untrusted_provider' 133b78fc4 -> packages/d2b-provider-provider/src/providers.rs:725; git grep -n 'untrusted_provider' HEAD -> only docs/audits/2026-09-24-unit-test-audit/lane/d2b-provider-provider.md:49, a static doc. The deleted test body asserted plan_external(...).unwrap_err() == ProviderError::TrustOrCompatibilityDenied for an untrusted manifest.
  • The same hunk also deletes ready_external_provider_publishes_only_after_children_are_ready and missing_dependency_keeps_exports_withdrawn. The five surviving tests in the file cover only the system-core, minijail and observation paths - none covers trust.
  • d2b-provider-toolkit/tests/malicious_provider.rs still exercises manifest.admit(...) directly, which is precisely why the suite stays green while production no longer calls it.

Impact. A cryptographically authentic signed Provider manifest whose TrustEvidence records publisher_trusted: false, signature != Valid, revocation != Clear, emergency_deny: true, or a non-admitting provenance/SBOM/license/vulnerability/conformance is no longer refused on the production deployment path. To be precise about what is and is not broken: the resource compiler does verify the Ed25519 signature over the raw manifest bytes, so this is not a signature-forgery hole - but authenticity is exactly what TrustEvidence::admit() exists to refuse, and that policy conjunction now has no production caller. Separately, the API-major/minor compatibility check and the required descriptor SchemaFingerprint pinning are unenforced in production, so a Provider built against an incompatible API or a different descriptor fingerprint is admitted.

Fix. Restore an admission call on the production path: in deploy_target_local_controllers and ControllerRoleContract::from_signed_manifest, call manifest.admit(required_api_major, required_api_minor, &required_descriptor_fingerprint) (or at minimum manifest.trust().admit()) before deriving, and map failure to the existing structured refusal. Restore untrusted_provider_is_rejected_before_child_planning against whichever entry point survives, asserting the refusal for each TrustEvidence field the matrix mutates. If the runtime is genuinely meant to own admission, that owner must call admit() and the doc comment must name it.

2. seccomp-profile dropped four public items, including the error type its public methods return

Location. packages/d2b-provider-seccomp-profile/src/lib.rs:19-26; affected items at packages/d2b-provider-seccomp-profile/src/seccomp_profile.rs:18, 20, 22, 37, 192, 232.

Defect. The wildcard pub use seccomp_profile::* became an explicit 8-item list that omits four items still pub in the file, so they are unreachable outside the crate. The consequential omission is SeccompProfileContractError: DeviceNodePath::parse (seccomp_profile.rs:37) and SeccompProfileSpec::new (seccomp_profile.rs:192) are pub methods on re-exported types and both return Result<_, SeccompProfileContractError>, but downstream code can no longer name that type and therefore cannot match on TooManySyscalls, TooManyDeviceBinds or InvalidDevicePath - the three confinement-bound refusals this crate exists to raise. The three enforcement bounds MAX_SECCOMP_SYSCALLS, MAX_SECCOMP_DEVICE_BINDS and MAX_DEVICE_NODE_PATH_BYTES are likewise unnameable, so a consumer cannot pre-validate against the same limits the crate enforces. Rust's private_interfaces lint does not fire because the items are pub inside a private module, so this ships silently.

Evidence.

  • git show 133b78fc4:./packages/d2b-provider-seccomp-profile/src/lib.rs -> pub mod seccomp_profile; and pub use seccomp_profile::*;
  • lib.rs at HEAD:19-26 -> mod seccomp_profile; and pub use seccomp_profile::{ DeviceBind, DeviceNodeKind, DeviceNodePath, SECCOMP_PROFILE_RESOURCE_TYPE, SeccompCgroups, SeccompDeviceAccess, SeccompNamespaces, SeccompProfileSpec, };
  • The pub item set in seccomp_profile.rs is identical at base and HEAD: SECCOMP_PROFILE_RESOURCE_TYPE:16, MAX_SECCOMP_SYSCALLS:18, MAX_SECCOMP_DEVICE_BINDS:20, MAX_DEVICE_NODE_PATH_BYTES:22, DeviceNodePath:27, SeccompNamespaces:80, SeccompCgroups:102, DeviceNodeKind:117, SeccompDeviceAccess:127, DeviceBind:141, SeccompProfileSpec:171, SeccompProfileContractError:232. The re-export list omits the four named above.
  • git grep for each of the four shows all referents are inside the same file (enforcement at :193, :196, :40, :72 and the crate's own tests at :299, :324, :336-345) plus docs/audits/ prose, so nothing fails to compile - this is removed public surface with no migrating caller, not a build break.

Fix. Add the four omitted names to the pub use list, restoring parity with the previous wildcard: SeccompProfileContractError, MAX_SECCOMP_SYSCALLS, MAX_SECCOMP_DEVICE_BINDS, MAX_DEVICE_NODE_PATH_BYTES.

Verdict

request-changes

Finding 1 removes a live refusal path together with the test that pinned it, and leaves a doc comment asserting an enforcement that no component performs. Finding 2 is an unnameable error type in a confinement contract crate. Both are small, discrete fixes.

Reconciliation

Candidates examined and dismissed, with the evidence that dismissed each. Several are the areas this slice was most asked to look hardest at, and several are genuinely clean.

  • Committed private key or key-derivation material (would be critical). Clean. Only two tracked .pem files exist at HEAD, both named publisher-public-key.pem. openssl pkey -pubin -text -noout reports ED25519 Public-Key (64-byte SPKI). Scanning every added line in the slice for -----BEGIN returns 0 matches (the sole earlier hit was the binary provider-manifest.json.sig). A per-file loop over all 435 changed files found no added PEM header, and files added by the diff that are not source/manifest files: none. The four privateKeyPath matches in added lines are config-path literals in packages/d2b-core/src/static_invariants.rs, outside this slice, containing no key bytes.
  • Stale publisher signature or untruthful manifest digest. Clean, and independently confirmed. openssl pkeyutl -verify -pubin -rawin prints Signature Verified Successfully (exit 0) over the committed manifest with the committed key. A one-byte manifest change fails, a one-byte public-key change fails, and the base key against the new manifest and signature fails - so the rotation is real, not a no-op. The manifest's only semantic delta across the PR is digests.config / components[0].configDigest (e4a434b8... -> 1962ab36...), and sha256sum root-config.schema.json yields exactly those two values at base and HEAD respectively, so the digest is truthful at both revisions. changelog.d/fix-provider-artifact-signature.md describes the same rotation and states the private half is held outside the repository.
  • GuestSetupDescriptorVerifier::verify is a string-equality stub (key_fingerprint.as_str() == self.expected_key && signature == "catalog-signature"), with a self-referential key fingerprint, so nothing cryptographically verifies the Guest setup descriptor. Not reportable here: pre-existing. git diff 133b78fc4..HEAD -- packages/d2bd/src/resource_runtime.rs matches only line 419 (GuestSetupDescriptor::from_canonical_bytes), never the verify body, and packages/d2b-provider-guest-cloud-hypervisor/src/descriptor.rs is touched only by added # Errors doc sections at existing hunks. It fails the "introduced in the patch" bar, so it is recorded rather than filed. It is serious enough to deserve its own ticket: composition.rs does propagate the failure to guest-session:descriptor-untrusted rather than swallowing it, so the weakness is the verifier, not a dropped error.
  • SystemdProviderError deleted wholesale from process-systemd/src/error.rs with its module unexported. Clean cutover - git grep -n 'SystemdProviderError' HEAD returns 0 hits.
  • pub mod lifecycle narrowed to mod lifecycle in process-systemd. Clean - git grep -n 'd2b_provider_process_systemd::lifecycle' HEAD returns 0 files, and the crate root still re-exports the items. External consumers (device-security-key, d2b-provider-process, five d2bd files) all use the root path.
  • take-controller-bootstrap payload rewritten from hand-built json! to a typed struct, with skip_serializing_if turning present-null keys into omitted keys. Wire-compatible. The builder at broker.rs:1715-1726 maps the same six fields; ResourceRef serializes as the canonical string (packages/d2b-contracts/src/identity.rs:831-838), so Some(ResourceRef) equals the old to_canonical_string(); runtime_scope is Option<[u8; 32]> in both forms and serializes to the same JSON array; and every skipped field is #[serde(default)] Option on the consuming struct (broker_wire.rs:1715-1724), so omitted and null decode identically. The response side is strictly enforced - take_controller_bootstrap_verdict (broker.rs:1739-1756) treats a missing or non-typed result as LaunchFailed rather than reading it as "not taken".
  • New #[cfg(any(test, feature = "test-support"))] gate on six process-systemd modules could silently skip integration tests. Consistent. The only referents of the gated module paths are tests/controller.rs and tests/boundaries.rs, and Cargo.toml at HEAD declares [features] test-support = [] with required-features = ["test-support"] on exactly those two [[test]] targets. The lib.rs doc comment's claim is accurate.
  • Supervisor observation ledger lost its bound when extracted to crate::observations. Preserved verbatim - MAX_PENDING_OBSERVATIONS: usize = 1024 with oldest-entry eviction at bound, and the same error mapping at both call sites. The #[allow(clippy::disallowed_methods, reason = "...")] annotations were carried across with reasons, matching the repository's tracked per-site allow policy.
  • labs/window-chrome/proxy moved its WorkloadTarget import to d2b_contracts. Required by this same PR, not drift: the PR deletes packages/d2b-core/src/workload_identity.rs, whose entire base content was pub use d2b_contracts::workload_identity::*; (confirmed by changelog.d/w5-03-core-shim-deletion.md). The target resolves (packages/d2b-contracts/src/lib.rs:65, workload_identity.rs:55). On the "live but out of the audit's scope" question: the lab declares its own [workspace], the root Cargo.toml members list contains no labs/ entry, and no labs/ reference exists in the root Cargo.toml/BUILD.bazel/WORKSPACE/MODULE.bazel - so it does not leak into the aggregate build graph. The lab's own header states "Standalone by design ... Nothing here reaches production without an ADR", and docs/contributing/gates-and-lints.md:21 lists labs/venus-vulkan-video/AGENTS.md as a live instruction file. Live, dash-gated, out of the production audit, and consistent with the change.
  • Shipped-artifact rule breaches. Zero. A non-ASCII scan over added text lines in the slice returns 0 matches, and a marker/attribution scan for TODO, FIXME, XXX, HACK, CLAUDE, Co-Authored-By, "Generated with", "as an AI", "per review", "follow-up:", "revision N" and "note to reviewer" returns no matches.
  • Seven ADRs still cite the deleted packages/d2b-core/src/workload_identity.rs. Real but out of scope and untouched: docs/plans/2026-08-19-002-..., ADR-046-cli-and-operations.md, ADR-046-current-code-migration-map.md, ADR-046-nix-configuration.md, ADR-046-resources-host-guest-process-user.md, ADR-046-resources-zone-control.md, ADR-046-telemetry-audit-and-support.md - a per-file diff check shows all 7 are unmodified, and the deleted file is in packages/d2b-core, outside this slice. The list was forwarded to the docs reviewer as actionable for their slice.
  • ProviderIntent::Disable/Delete and ProviderPhase::Draining removed without a migrating caller. Clean cutover - git grep for all three returns no hits at HEAD; the only surviving mentions are static docs and CHANGELOG.md:1267.

Limits of this review

I verified the code and the committed artifacts directly. I did not run the build, linters or test suites, per the review constraints, so I make no claim about whether the workspace currently compiles; a consequence is that findings 1 and 2 are argued from the export surface, call graph and enum/visibility structure rather than from a compiler diagnostic. A sub-investigation into privilege, namespace and device-mediation boundaries reported that the per-provider crates in this slice are declarative - they name a seccomp policy reference, a namespace set and a device-bind list rather than installing confinement, which lives in d2b-broker and d2b-core/src/bundle_resolver.rs, outside this slice; I re-ran the diff-scoped portion of that check myself and found no removed or weakened confinement argument in the slice.

Adding the mirror's `tracing` dependency moved `packages/Cargo.guest.lock`, so
the committed guest-static policy inputs recorded a stale `lockSha256` and the
drift gate failed; regenerated through `make generate`, which also emits the
new `d2b-core -> tracing` closure edge.

The storage-lifecycle schema test now compares the issue kinds its fixtures
exercise against the kinds the published schema declares, instead of asserting
the length of the literal list it just built, and the auth-status contract's
module doc no longer says the launcher gains `up`.
The plan still made committing the audit corpus a wave-0 requirement in its
wave map, its assumptions, and U1's heading, approach step and verification,
while the same document's R8 and Definition of Done state that the record does
not ship and the corpus was deliberately dropped from the tree. Those steps now
read as the ledger and baseline work they are.

Alongside it: the USBIP lock-taxonomy fragment quotes the three real
`device-worker-*` refusal codes instead of truncated tails, the parking-lot
fragment names the bounded-worker allow reason instead of a plan identifier, and
the v1.0-to-v1.1 migration guide's table header matches the v1.1.2 arrival its
own note records.
@vicondoa
vicondoa merged commit 0c85afa into v3 Sep 26, 2026
18 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.

2 participants