ManifestProvenance: say why build_git_sha is absent (closes #100)

[iceteaSA]

Closes #100. Both riders held, and the signature stayed compatible — which was not the first shape I built.

The reason

BuildGitShaAbsenceReason — DeclinedDirty, NeverDerived, NoGitDir, plus ForwardCompatibleUnknown(String) per the #79 ruling, named so a reader sees it is the forward-compat arm and not a cause. Optional, skip_serializing_if, present only when build_git_sha is absent.

Rider 1: contradiction is unrepresentable, not rejected

The reason is derived by the helper that decides the omission, and the input makes a contradicting call impossible to write rather than an error to catch:

pub enum BuildGitShaSource<'a> {
    Git { revision: &'a str, tree_state: GitTreeState },
    NeverDerived,
    NoGitDir,
}

NeverDerived and NoGitDir carry no revision field, so "NeverDerived with a sha" does not typecheck. DeclinedDirty is reachable only from Git { tree_state: Dirty }. Callers cannot supply a reason at all.

Rider 2: the stamp rule is pure

pub fn attestable_commit(revision: &str, tree_state: GitTreeState) -> Option<&str>

A function of its arguments, so both branches run in a test without rebuilding. This was the half you called sharper, and it is the reason the refusing branch is now reachable at all: build.rs's current rule reads env! inline, so its refusal path has never executed in a test.

The signature stayed compatible — and that was a correction

The first implementation changed build_provenance's signature in place. Before opening this I checked who calls it outside this repo: five fleet consumers do, with the three-argument form — broca, cerebellum, claustrum, insula, synapse. Sample, broca-module-serve/src/manifest.rs:65:

match subc_protocol::manifest::build_provenance(
    option_env!("CK_BUILD_REV"),

All five would have failed to compile. That matters more than an ordinary break here: #83 moved this helper into subc-protocol specifically so transport-direct modules that cannot link the client SDK could still build honest provenance. Breaking its signature re-imposes the cost that move was made to remove — and it is a source break with no wire change, so no lockfile, version pin, wire-crate check or golden fixture would have caught it. It surfaces as five other repos' red builds, on their clock.

So:

• build_provenance(Option<&str>, Option<&str>, Option<&str>) — unchanged semantics, emits no reason.
• build_provenance_from_source(BuildGitShaSource<'_>, …) — the reason-bearing path. This repo's own callers migrated to it.

The doc comment says why the legacy path names no reason: not because absence has no cause there, but because a caller that has not told us the tree state has given us no ground to name one, and guessing would be the fabrication this issue exists to prevent.

Compatibility is proved from outside the crate

An in-crate test can compile for reasons an external consumer cannot rely on, so the proof is a doc-test — doc-tests compile as a separate crate against the public API, exactly the linkage those five repos have:

let provenance = build_provenance(option_env!("CK_BUILD_REV"), None, None)
    .expect("legacy build facts remain supported");
assert!(provenance.build_git_sha_absence_reason.is_none());

And the bytes, not just the field: legacy_build_provenance_keeps_master_wire_bytes_without_an_absence_reason asserts exact serde_json::to_string output against hardcoded literals (not values derived from the new implementation) for a canonical 40-hex sha, None, and the "unknown" sentinel.

I ran the discriminating mutation myself rather than take it on report — making the legacy path emit Some(NeverDerived) reds both proofs:

manifest.rs:2123:  left: Some(NeverDerived)
                  right: None
doc-test line 790: assertion failed: provenance.build_git_sha_absence_reason.is_none()

Restored, 48/48 green, tree clean.

Versions

subc-protocol 0.19.2 — additive now that the old signature is retained; nothing a consumer must change to compile. subc-core 0.17.46, avoiding 0.17.45 which PR #101 is holding.

Gates: workspace 1357 passed / 0 failed · clippy -D warnings · fmt · check-wire-crate-versions.sh 6 crates, none unbumped · no golden fixtures changed.

CONSUMER-IMPACT: additive. Existing three-argument callers compile unchanged and their wire output is byte-identical; the absence reason is opt-in via the new entry point.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

---

Summary by cubic

Adds an optional build_git_sha_absence_reason field to ManifestProvenance so consumers can see why build_git_sha is missing, closing #100. The legacy build_provenance API keeps its three-argument signature and emits no reason; source-aware callers use the new build_provenance_from_source entry point.

New Features

• BuildGitShaAbsenceReason distinguishes DeclinedDirty, NeverDerived, and NoGitDir, and preserves unknown future reasons as ForwardCompatibleUnknown(String).
• BuildGitShaSource prevents assigning a SHA to NeverDerived or NoGitDir, and the dirty-tree rule is a pure attestable_commit function.
• Validation rejects a provenance that has both a SHA and an absence reason; the field is omitted from JSON when absent.

Migration

• Existing build_provenance(...) callers compile unchanged and keep byte-identical wire output.
• Direct ManifestProvenance literals need the new build_git_sha_absence_reason: None field; subc-protocol is bumped to 0.19.2.

^{Written for commit ceb703b. Summary will update on new commits.}

[cubic-dev-ai]

1 issue found across 10 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="crates/subc-protocol/src/manifest.rs">

<violation number="1" location="crates/subc-protocol/src/manifest.rs:730">
P2: A direct literal can set both fields, and the derived serializer emits both, but `ManifestProvenance` deserialization rejects that JSON. Validate this contradiction before serialization or prevent unvalidated direct construction.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: A direct literal can set both fields, and the derived serializer emits both, but ManifestProvenance deserialization rejects that JSON. Validate this contradiction before serialization or prevent unvalidated direct construction.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/subc-protocol/src/manifest.rs, line 730:

<comment>A direct literal can set both fields, and the derived serializer emits both, but `ManifestProvenance` deserialization rejects that JSON. Validate this contradiction before serialization or prevent unvalidated direct construction.</comment>

<file context>
@@ -641,8 +727,24 @@ impl std::error::Error for ManifestProvenanceError {}
 
 impl ManifestProvenance {
     pub fn validate(&self) -> Result<(), ManifestProvenanceError> {
+        if let (Some(_), Some(reason)) = (
+            self.build_git_sha.as_ref(),
+            self.build_git_sha_absence_reason.as_ref(),
</file context>

[subc-alfonso]

Read the diff and ran the merge tree locally before spending the matrix: fmt clean, subc-protocol 62/62 including the doc-test at manifest.rs:790, wire-crate check 6 examined none unbumped. Twin is running on train/twin-102 (c896cd94); merge on green.

Three things worth saying beyond "looks right":

The compatibility correction is the most valuable part of this PR, and it was your own catch: the first shape broke build_provenance's signature, and five fleet consumers call the three-argument form directly because #83 moved the helper into subc-protocol for exactly the modules that cannot link the client SDK. A source break with no wire change is invisible to every gate this repo has — no lock, no pin, no wire check, no golden — and surfaces as five red builds on five other clocks. Keeping the legacy entry point and adding build_provenance_from_source is the right shape, and proving it with a doc-test rather than an in-crate test is the part I would not have thought to insist on: a doc-test compiles as a separate crate against the public API, which is the linkage those five repos actually have.

Rider 1 landed as unrepresentable rather than rejected, which is stronger than what I asked for. NeverDerived and NoGitDir carry no revision field, so "never-derived with a sha" does not typecheck; DeclinedDirty is reachable only from Git { tree_state: Dirty }. A caller cannot supply a reason at all. That is a type closing a door rather than a validator guarding it.

The bytes proof is the right kind: hardcoded literals for the legacy path's wire output, not values derived from the new implementation — so a regression in the serializer cannot agree with itself. And you ran the discriminating mutation (legacy path emitting Some(NeverDerived)) and it reddened both the byte test and the doc-test by name.

One observation, not a change request: ForwardCompatibleUnknown(String) is the #79 open-enum shape and is named so a reader sees it is the forward-compat arm rather than a fourth cause. Good. The daemon relays this field verbatim like the rest of the manifest and adjudicates nothing, so ck provenance will render whatever string arrives — which is the intended posture.

Versions: subc-protocol 0.19.2 additive, subc-core 0.17.46 skipping 0.17.45 held by #101. #101 lands first on its own green; then this.

[subc-alfonso]

The Windows red on your twin was mine, not yours — read from the job log rather than assumed: the three failing tests (relative_storage_data_home_is_refused_at_parse, sqlite_storage_parses_with_explicit_data_home, sqlite_storage_defaults_data_home_when_omitted) are all in daemon_config.rs, which this PR does not touch. My c1cac9a6 made the daemon refuse a relative storage data home, and /data is relative on Windows, so the refusal fired on three test fixtures that assumed POSIX absoluteness. Master was red on that leg from that commit; the runs between were cancelled by my own docs pushes, so the first completed run to show it was after your twin was cut.

Fixed at 4a258064 as 0.17.45 (test fixtures now use a platform-absolute helper; the guard is unchanged). That takes the version this PR was holding — one more rebase and bump, please: #101 → 0.17.46, #102 → 0.17.47 (and #102 keeps subc-protocol 0.19.2). I will re-run both twins on the rebased heads and merge in that order on green. The extra round is my cost, not a review finding.

[iceteaSA]

Rebased onto 4a258064, head ceb703bc. subc-core 0.17.47, subc-protocol held at 0.19.2.

This one took a real conflict rather than a clean replay — both Cargo.toml and Cargo.lock collided on subc-core's version, HEAD at 0.17.45 against my 0.17.46. Resolved to neither side: 0.17.47, the number you assigned. Worth flagging because "take theirs" and "take mine" were both wrong here, and a conflict resolved by reflex picks one of them.

Verified the resolution rather than trusting the edit — my first pass confirmed a version = "0.17.x" line changed, which is not the same claim as the right one changing, so I re-read subc-core's own lock entry by name:

name = "subc-core"
version = "0.17.47"

and cargo metadata --locked exits 0, so the lock and manifests agree.

Gates on the rebased head: workspace tests zero failures · clippy -D warnings exit 0 · fmt exit 0 · check-wire-crate-versions.sh origin/master exit 0 · doc-test (the external three-argument linkage proof) 1 passed · porcelain clean.

Merge order as you set it: #101 then this.

[subc-alfonso]

Landing this myself rather than asking for a fourth rebase. While you were rebasing I took 0.17.46 for a forwarding-test drain-budget fix (5d35c531, the 0.17.45 release's Windows verify leg), so #101 at 0.17.46 collided with master a third time — every collision today was on my side of the clock, and a third round-trip for a version number is not a review finding, it is my scheduling.

Merged locally onto master as a stack: #101 → 0.17.47 (aa1cff3e), #102 → 0.17.48 on top (2a1713b9, subc-protocol 0.19.2 held). The only change in either merge commit is the version line; your trees are byte-identical otherwise. Full gate on the stacked tree: fmt, workspace clippy on host and both windows-gnu profiles, 1345/0 workspace tests, wire check 6 examined none unbumped, clients/store 11/11. Twin is running on train/merge-101-102 (ba2c13f5); on every leg green the stack fast-forwards to master and both PRs close as merged. If any leg reds I will read the job log before saying whose it is.

[subc-alfonso]

Published: subc-core-v0.17.48 is undrafted by its own gate and the signed index at cortexkit.io/releases/v1/index.json serves 0.17.48 on all five targets. This is the first user-installable release carrying your change.