Skip to content

docs(protocol): correct the provenance honesty contract to the shipped behaviour - #68

Merged
ualtinok merged 1 commit into
cortexkit:masterfrom
iceteaSA:docs/provenance-contract-accuracy
Aug 26, 2026
Merged

ualtinok merged 1 commit into
cortexkit:masterfrom
iceteaSA:docs/provenance-contract-accuracy

Conversation

@iceteaSA

@iceteaSA iceteaSA commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Closes #67. Documentation only — one doc comment on ManifestProvenance, no code, no wire, no behaviour.

The defect

The honesty contract told adopters:

the daemon renders absence as declared_absent rather than inventing a value.

declared_absent does not exist. It appeared exactly once in the repository — in that sentence. What actually happens:

  • ModuleDeclaredProvenance (subc-control/src/lib.rs:413-419) has two variants, Reported { build } and Unverifiable, discriminated by whether the HELLO manifest carried a provenance block at all.
  • An omitted field is dropped from the wire by skip_serializing_if = "Option::is_none" (manifest.rs:125-131).
  • ck provenance renders it through provenance_value's fallback arm as the literal unavailable (ck.rs:1393-1405).

This is mine, shipped in #59 — and it is the same announced-name-vs-shipped-name class that got CK_BUILD_REV reverted on #58, sitting in the one file whose purpose is telling adopters what is honest.

Three edits

  1. declared_absent → the real behaviour. An omitted field renders as unavailable, and the module still reports as Reported, since declaration status is decided at the block level.

  2. "No field is required" stated explicitly. "Every field is a VERIFIED-AT-BUILD claim" read as though a full set was expected, so an adopter with a legitimately inapplicable field had to go read the wire type to discover otherwise.

  3. None for build_git_sha no longer implies "needs a release pipeline." The CK_BUILD_REV-via-packaging-path example read as the only sanctioned source. The actual bar is knowing whether the tree was clean — a .git read, not infrastructure — and the existing -dirty and omit-when-dirty clauses already sanction both upgrade paths.

Everything else is preserved: the option_env! guidance, -dirty semantics, cerebellum's stricter-is-better note, the two CEREB silent-when-wrong checks, and the closing fill-fields-FROM-THE-BUILD clause.

How it surfaced, since the provenance is the interesting part

The first module seat to adopt the declaration read the contract literally and asked whether None in store_schema_version would score as a gap or as honest absence — their module is cache-only and has no store schema, so declaring a version would have meant minting a schema identity to satisfy a field. Answering required reading the type, which is where the missing state showed up.

Both gaps this PR fills are things that adopter hit in the first hour of using the contract: one concluded they needed a packaging pipeline to declare commit identity (they do not), and one could not tell whether omitting an inapplicable field would score against them (it does not).

Verification

cargo test --workspace --locked      47 suites · 760 passed · 0 failed
cargo clippy --all-targets           clean with -D warnings
cargo fmt --all --check              clean
aft_inspect                          0 errors · 0 warnings
grep declared_absent                 0 occurrences

Gates are not ceremony for a doc-only change here — rustdoc link syntax and doctests both build.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith with what you need. Autofix is disabled.

…d behaviour

The contract told adopters the daemon renders absence as \declared_absent\.\nThat state does not exist: ModuleDeclaredProvenance has only Reported and\nUnverifiable, an omitted field is dropped by skip_serializing_if, and ck\nrenders it as \unavailable\. The prose also read as though a full field set\nwas expected and as though commit identity required a release pipeline, both\nof which sent a real adopter toward inventing values it could not honestly\ndeclare.\n\nCONSUMER-IMPACT: none — documentation only.
@iceteaSA
iceteaSA force-pushed the docs/provenance-contract-accuracy branch from 87570ac to 2472ede Compare August 26, 2026 16:47
@iceteaSA

Copy link
Copy Markdown
Collaborator Author

Amended to 2472ede — the contract now names both absence words instead of one.

A fleet seat pointed out that the render already distinguishes the two levels, and I confirmed it against two live modules that happen to be in opposite states right now:

MODULE: aft                      MODULE: claustrum
MODULE-DECLARED                  MODULE-DECLARED
  unverifiable                     COMMIT: unavailable
                                   LOCK DIGEST: unavailable
                                   WIRE CRATE VERSION: 0.13.0
                                   STORE SCHEMA VERSION: 1

unverifiable = no provenance block declared at all. unavailable = block declared, this field omitted. Two words, two levels, and a reader can tell which state they are in without knowing the schema.

My first version only named unavailable, which fixed the wrong word but still left an adopter to infer the block-level case. The amended clause names both:

Absence is reported at two levels with two distinct words: a module that declared no provenance block at all reads unverifiable, while an omitted field inside a declared block is dropped from the wire and reads unavailable. So omitting a field never costs a module its Reported status — declaration is decided by whether the manifest carried a block, not by which fields it filled.

Still doc-only, no new state introduced, and the capability was already shipped — this is the contract catching up to the render rather than asking anything of it.

Worth recording that I had this backwards a moment ago: I told that seat the two-level distinction would be a "deeper fix" for later. It was already there. The check that settled it was having one module in each state simultaneously and running both renders side by side — a comparison that is only available while the fleet is mid-adoption, and will not be once every module declares.

cargo test -p subc-protocol 38 passed, fmt and clippy clean on the amendment.

@ualtinok
ualtinok merged commit fdc69a5 into cortexkit:master Aug 26, 2026
4 checks passed
@iceteaSA

Copy link
Copy Markdown
Collaborator Author

Amended again to b5892ee — added the first per-field docs on ManifestProvenance, after a fleet adopter hit a real collision on wire_crate_version.

What happened

Two modules declared the same field with values four minor versions apart, in different numbering spaces:

claustrum   wire_crate_version = 0.13.0     (subc-protocol)
insula      wire_crate_version = 0.7.0      (cortexkit-provider-usage — its own envelope crate)

Both readings are natural from the name alone. The adopter's build.rs parsed Cargo.lock for its own wire crate; mine assumed the subc protocol crate. Same field name, opposite referents, and a version comparison across them returns a confident wrong answer rather than an error — the module in question links subc-protocol 0.13.0 and reports a number that says otherwise.

The field was already defined — in code, not in prose

subc-client-rs/src/lib.rs:54:

pub fn build_provenance(
    build_git_sha: Option<&str>,
    build_lock_digest: Option<&str>,
    store_schema_version: Option<&str>,       // three params
) -> ManifestProvenance {
    ManifestProvenance {
        ...
        wire_crate_version: Some(SUBC_PROTOCOL_CRATE_VERSION.to_string()),   // not a param
        ...
    }
}

wire_crate_version is not a caller parameter. The sanctioned constructor fills it from SUBC_PROTOCOL_CRATE_VERSION (subc-protocol/src/lib.rs:125, env!("CARGO_PKG_VERSION")), which is the only source that can attest what the binary actually links.

So the referent was pinned in code and silent in the doc, and a module that hand-rolls the field instead of calling build_provenance can mean anything by it. The struct-level contract covers honesty thoroughly and never says which crate — and there were zero per-field docs.

Added

On wire_crate_version: it is the subc-protocol crate version this binary links, explicitly not the module's own envelope crate; do not populate by hand; build_provenance fills it; a hand-stamped value answers a different question with the same field name.

On store_schema_version: a module with no persistent store declares None rather than minting a schema identity for something that has none — the case the same adopter raised and that the honesty contract implies but never stated.

Note on the name

If this field had been subc_protocol_crate_version the collision could not have happened. I am not proposing a rename of a shipped wire field, but the doc now carries what the name should have said. Flagging it in case you would rather rename before more modules adopt — there are two declaring modules today, so the window for that is open and closing.

Still doc-only. cargo test -p subc-protocol -p subc-client-rs 0 failed suites, fmt and clippy clean.

@iceteaSA

Copy link
Copy Markdown
Collaborator Author

Amended to 3822c07. My previous comment's framing was wrong in a way that matters for the fix, and the adopter caught it.

I said the diverging module "bypassed the constructor." It cannot reach the constructor. Verified against its manifest:

crates/quota-module/Cargo.toml
  subc-client-rs   0 hits
  subc-protocol    line 22
  subc-transport   line 23

It is a transport-direct consumer — subc-protocol + subc-transport, no SDK. So build_provenance was never in its dependency graph, and hand-rolling the field was the only option available rather than a shortcut. A doc that says "call build_provenance" documents a remedy that population cannot apply, and would make a correct bug report easy to dismiss with "you already have this function."

The fix that does reach them is the constant, not the constructor:

// subc-protocol/src/lib.rs:125
pub const SUBC_PROTOCOL_CRATE_VERSION: &str = env!("CARGO_PKG_VERSION");

build_provenance is a convenience that reads it. Every module linking subc-protocol — which is all five transport-direct consumers — can read it directly with no new dependency edge. The per-field doc now prescribes that, and mentions the SDK only as the free path for modules that already link it.

Why this strengthens the rename question

The per-field doc works here only because the referent happens to live in the crate every consumer shares. Had it been pinned solely in subc-client-rs, documenting it in subc-protocol would have stated a fact half the readers could not act on — the doc would have been correct and unusable at the same time.

A field named subc_protocol_crate_version needs no doc, no constant lookup, and no constructor: the name carries the referent, and a transport-direct module writing its own struct literal cannot get it wrong. That is worth weighing against the cost of renaming a shipped wire field while exactly two modules declare it.

I am not proposing the rename in this PR — it is doc-only and should stay that way. Flagging that the window is open now and closes as adoption spreads.

Verification

cargo test -p subc-protocol --locked    0 failed suites
cargo doc -p subc-protocol --no-deps    intra-doc link resolves, no new warnings
cargo fmt --all --check                 clean

One pre-existing rustdoc warning survives on frame.rs:3 (redundant explicit link target). Present on clean master, untouched by this branch — checked specifically because this PR changes docs and I wanted to be certain I had not introduced it. Happy to fold the one-character fix in if you would rather it not sit there, but it is out of scope as filed.

@iceteaSA

Copy link
Copy Markdown
Collaborator Author

Thanks for f8434a1 — and that lint was mine to have caught, not CI's. Worth recording why it escaped, because the reason is not the one I would have assumed.

My first instinct was toolchain skew: CI runs a newer clippy than my distro cargo 1.98.0. That is wrong. Checked it rather than assuming — a scratch worktree at fdc69a5 (your fix's parent) with my local toolchain:

$ cargo clippy -p subc-protocol --all-targets --locked -- -D warnings
error: doc list item without indentation
   = help: ... rust-1.98.0/index.html#doc_lazy_continuation
error: doc list item without indentation

Fires loudly, locally, on the exact merged commit. The lint was fully catchable here.

What actually happened: I amended the commit twice and re-ran only a subset the second time. Amendment 1 (per-field docs) got test + clippy + fmt, all clean. Amendment 2 — the one that rewrote the wire_crate_version doc into multiple paragraphs, i.e. the change that introduced the lazy continuation — got test + cargo doc + fmt, and no clippy. I had a full green from the previous version of the same commit and let it stand in for the new one.

Two things I am taking from it:

  1. An amendment invalidates every prior gate result for that commit. A pass on the previous version proves nothing about the current one, and --amend makes that easy to miss because the commit "already passed."

  2. "Doc-only" is not a gate-reducing category. I ran cargo doc instead of clippy precisely because the change was prose — but doc comments are compiled artifacts, and rustdoc lints, doctests, and intra-doc links all live in them. Reaching for the narrower tool was the specific mistake.

Also flagging the near-miss on the diagnosis itself: had I accepted "CI has a newer clippy," I would have filed a toolchain story over a process defect and kept the hole. Same class as everything else in this PR's history — a plausible mechanism that is not this mechanism.

Separately, from the run that carried these merges: check-wire-crate-versions.sh printed 6 cross-repo crates examined … none changed without a version bump immediately after six fatal: … no merge base errors. Filed as #72 — the base-ref guard checks that the ref resolves rather than that it is comparable, || true swallows the diff failure, and examined counts attempts rather than successes, so a broken checkout reads as a clean fleet. I did not attribute the job failure itself to it; the fail-open shape is what reading the script proves.

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.

ManifestProvenance honesty contract promises a declared_absent render state that does not exist

2 participants