Skip to content

fix(conformance): distinguish unsupported custody from an unavailable store - #307

Closed
BunsDev wants to merge 2 commits into
mainfrom
fix/219-installation-custody-diagnostic
Closed

BunsDev wants to merge 2 commits into
mainfrom
fix/219-installation-custody-diagnostic

Conversation

@BunsDev

@BunsDev BunsDev commented Sep 16, 2026

Copy link
Copy Markdown
Member

Integrated follow-up Chat #313 is published at signed head 157aa554bf908a2b1c396692b490f6cfa4d4a5ab, binding signed source aa4d33004c59202cae70e3fc5a56a70cdf451dac. It carries #307 custody classification on top of #311, adds the missing framed RPC test, and regenerates metadata and source anchors. Local normal/heavy suites passed 2,164 tests (98 skipped); targeted Rust custody, Clippy, formatting, typecheck, and lint passed. Three PowerShell checks timed out in the parallel normal run but passed serially with unchanged limits; full serial normal suite passed. Independent source and binding reviews found no blockers. Hosted checks/review are running. Original #307 branch/worktree remain preserved and its threads are not resolved; replacement landing remains required. Protected run 35146928092 still tests Chat #311 / SDK #293, not #313. No aggregate acceptance or Windows repair is claimed.

Isolated integration verification on Chat #311 main 157fb3206b9b90f24049aa2043bae534d2b9a709, with #307 executable changes and the missing RPC coverage row applied: 48 TypeScript tests passed (native preflight diagnostics plus Cave build HOME isolation), Rust formatting passed, and the conformance-feature Rust test unsupported_custody_installation_is_distinct_from_an_unavailable_store passed. The active PR branch was not modified. This is runtime compatibility evidence only: four binding-file integration conflicts, source-identity regeneration, broader verification, and review remain before landing. It does not establish protected Windows success.

Current landing audit against Chat #311 main 157fb3206b9b90f24049aa2043bae534d2b9a709: reviewed head 579fc4abdb0ace683a394fb81fd496dc3277e099 has four integration conflicts: workflow, conformance documentation, conformance lock, and lock tests. This supersedes the earlier clean-merge observation against the pre-#311 base. The missing installation_custody_unsupported RPC coverage row and stale overview source reference remain unaddressed. Required next steps: integrate the Cave build HOME repair, add the missing RPC case, regenerate source/binding identities from the combined executable tree, verify, and review before landing. Active worktree preserved; no threads resolved.

Current-head landing audit — 2026-09-16

Current candidate 579fc4abdb0ace683a394fb81fd496dc3277e099 incorporates #309 and pins executable source fe58cb1d4353b44fc61884ad76802ce924571526. Earlier identities and conflict reports are historical.

Required before landing:

  • Add the missing RPC matrix row ['response', 'installation_custody_unsupported', 'custody-unsupported'] for review comments 4028435239 and 4028435299. The public allowlist test alone does not test RPC response decoding. The prepared patch passed 60 combined native-preflight/cleanup tests in an isolated integration snapshot. Both the producer and test file are byte-identical at this current head; the earlier negative control failed precisely when the production mapping was removed. The row remains absent from this PR.
  • Correct the binding overview: docs/phase1-conformance.md still names source 7dda439daa59ee350ff12cd195ff169b52e1085c instead of this PR's pinned source.
  • Reconcile with current main after Say what is true when no familiar is selected #310 (c38aad66e78e03706c2202a1d974c5452b624ad1) and sequence with fix(conformance): isolate Cave build state from the Windows profile #311. Git merges this head cleanly with Say what is true when no familiar is selected #310, but the resulting complete tree differs from the reviewed head, so it is not eligible for the protected producer binding as-is. Regenerate bindings after integration; do not equate conflict-free merging with bindability.
  • Complete exact-head verification and review resolution. Active custody worktree remains preserved; no acceptance is claimed.

⚠️ Merge with a real merge commit, and rebase immediately before merging

harnessAuthority.revision is pinned to fe58cb1d, a commit inside this branch. Squashing orphans it and the authority checkout breaks. And because the SDK contract requires the producer merge's tree to equal its reviewed second parent's tree (scripts/conformance-contract.mjs:1090, :1571-1578), the merge is only bindable if main has not moved since this branch was last rebased — #299 became permanently unbindable by missing that window by about 25 minutes.

What this separates

secure_store_unavailable covered two unrelated causes on the installation preflight:

  1. a secure store that is genuinely unavailable, and
  2. CredentialCustody::installation_id's default trait body (src-tauri/src/keyring.rs:289-292), reached by any custody implementation that never overrides it.

InstallationStage::classify (keyring.rs:140-146) already rewrites Unavailable into installation_{lock,entry,read,write,persistence}_unavailable, so those five stages were distinguishable. The default trait body was the last unclassified path on that operation, and protected run 35100084575 hit one of the two on Windows without the log being able to say which.

It now returns installation_custody_unsupported, published as phase1.native-scenarios.native-preflight-installation-custody-unsupported. Only the fixed identifier is published — no message, stack, path, credential or subprocess output.

Note that this also rules out the most attractive theory by elimination. ensure_store_initialized() returns a bare Unavailable when windows_native_keyring_store::Store::new() fails (keyring.rs:2107-2113), which is plausible on a CI runner — but it is reached through entry_for(), which installation_id() wraps in Entry.classify. A Credential Manager that failed to initialize would already have reported installation_entry_unavailable. It did not.

Scope

Diagnostics only. No change to request counts, timeouts, resource limits, native-provider behaviour, or assertion semantics. This does not repair the Windows failure and does not establish its cause — it makes the next protected run attribute it instead of reporting an ambiguous code.

The outer launcher derives its native-stage allowlist from the producer registry, so the new identifier is allowlisted without a second edit.

Historical author verification (before the current repin)

Suite Result
cargo test --features phase1-conformance --lib keyring 38 passed
vitest run src/native-preflight-diagnostics.test.ts 41 passed (includes the new category)
vitest run --config vitest.heavy.config.ts src/phase1-conformance-lock.test.ts 95 passed, 1 skipped
vitest run (full normal suite) 1173 passed, 63 skipped

Four files (windows-supervisor-source, windows-shared-quota, windows-process-termination-race, chat-side-selection) fail intermittently under full-suite parallel load on this machine and pass in isolation; they are untouched by this change.

Historical original repin

Two commits, because the lock cannot pin the commit it lives in — writing the pin changes that commit's tree and invalidates the pin it just wrote.

commit role
A ddc4eb29 sources; the pinned revision, tree 18fae508
B 8460866 the lock pinning A, plus golden expectations and docs

Three governed objects changed: the schema-v2 producer, the conformance workflow (its frozen module table), and the keyring.rs production delta. src/phase1-conformance-lock.test.ts's golden values were updated in the same change — on #299 that step was missed and cost a CI cycle.

A follow-up SDK rebinding will be required before a protected run can use this, since the evidence producer binding names an exact Chat merge.

Refs #219. Context in OpenCoven/sdk#38.

🤖 Generated with Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Two moderate coverage findings and one documentation nit remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This pull request distinguishes unsupported credential custody from unavailable secure stores in conformance diagnostics.

Changes:

  • Adds Rust classification and test coverage.
  • Propagates the diagnostic through schema-v2 reporting.
  • Updates authority pins, integrity metadata, and documentation.
File summaries
File Reviewed change Findings
src/phase1-conformance-lock.test.ts Updates authority expectations. None
src/native-preflight-diagnostics.test.ts Adds public-boundary coverage. Moderate (2 votes): add the missing response-matrix mapping.
src-tauri/src/keyring.rs Adds custody-unsupported classification and tests. None
scripts/phase1-schema-v2-producer.mjs Maps and publishes the new diagnostic. Moderate (2 votes): cover the response-code mapping in the RPC classification table.
phase1-conformance.lock.json Repins authority and governed-file hashes. None
docs/phase1-conformance.md Documents the diagnostic and refreshed metadata. Nit (1 vote): update the stale authority pins in the overview.
.github/workflows/client-v1-conformance.yml Refreshes frozen integrity metadata. None
Review details

Suppressed comments (1)

docs/phase1-conformance.md:1985

  • This repin changes harnessAuthority to ddc4eb29... / 18fae508..., but the current overview at docs/phase1-conformance.md:166-170 still says the lock selects 6e74fb60... / a1002364.... After this change the guide contradicts the lock and points readers to the old governed source; update that overview as part of the repin.
| `scripts/phase1-schema-v2-producer.mjs` | 231,998 | `7dbac79f3d8b6e956905f0d38a58afbb0087e628358c24a25acf3909aea1c744` |
  • Files reviewed: 7/7 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

const nativeRpcFailureCategories = new WeakMap();
const nativeInstallationResponseCategories = new Map([
['secure_store_unavailable', 'secure-store-unavailable'],
['installation_custody_unsupported', 'custody-unsupported'],
'installation-rpc',
'installation-id',
'installation-secure-store-unavailable',
'installation-custody-unsupported',
@BunsDev
BunsDev force-pushed the fix/219-installation-custody-diagnostic branch from 8460866 to 60e9dec Compare September 16, 2026 17:34
@BunsDev BunsDev added the ci:full Run the macOS and Windows CI jobs on this pull request label Sep 16, 2026
@BunsDev
BunsDev force-pushed the fix/219-installation-custody-diagnostic branch from 60e9dec to b9b9823 Compare September 16, 2026 18:30
@BunsDev

BunsDev commented Sep 16, 2026

Copy link
Copy Markdown
Member Author

Handoff — this PR is ready but I am stopping here

I have stopped the automation that was waiting to merge this. Nothing further will be pushed to the branch from my side. Everything below is current as of b9b9823, rebased onto 4355c19 (#308).

State

commit role
A a202a7c sources; the pinned revision, tree d013b4d7
B b9b9823 the lock pinning A, golden expectations, docs

Both signed. Base is 4355c19, equal to main at the time of writing, so a merge right now is tree-equivalent and bindable.

Verified locally

Suite Result
cargo test --features phase1-conformance --lib keyring 38 passed
vitest run src/native-preflight-diagnostics.test.ts 41 passed
vitest run --config vitest.heavy.config.ts src/phase1-conformance-lock.test.ts 95 passed, 1 skipped
vitest run (full normal suite) 1173 passed, 63 skipped

Three constraints, in the order they bite

  1. ci:full is required on this branch. It touches src-tauri/, so without the label Rust and Windows supervisor behavior skip, and because both are required contexts the PR sits BLOCKED with every visible check green. The label is applied now.
  2. Merge with a real merge commit, never squash, and rebase immediately before merging. harnessAuthority.revision is pinned to a202a7c, a commit inside this branch. Squashing orphans it. And the SDK contract requires the producer merge's tree to equal its reviewed second parent's tree (scripts/conformance-contract.mjs:1090, :1571-1578), so the merge is only bindable if main has not moved since the last rebase.
  3. Rebasing invalidates the pin. The lock names the sources commit by SHA, so every rebase needs the lock and the golden expectations in src/phase1-conformance-lock.test.ts regenerated against the new SHA. I have done that three times on this branch; the procedure is: repin harness.revision, harnessAuthority.revision/.tree, the changed files entries and the keyring.rs production delta, then mirror those values into the test.

One unresolved CI issue

Earlier heads accumulated two workflow runs — a pre-label one where Rust and Windows supervisor behavior recorded skipped, and a post-label one where they recorded success. Both conclusions stayed on the SHA and the merge remained BLOCKED despite the latest check-run per name being success. Re-running the stale run did not clear it; it created additional check-runs. A fresh head with the label already applied avoids the situation, which is what b9b9823 is. #308 looks like it may address this class of problem directly.

What this change does

Separates two causes that both reported secure_store_unavailable on the installation preflight: a genuinely unavailable secure store, and CredentialCustody::installation_id's default trait body. The default body now returns installation_custody_unsupported, published as phase1.native-scenarios.native-preflight-installation-custody-unsupported.

Protected run 35100084575 reported the ambiguous code on Windows while Linux and macOS passed. This does not repair that failure and does not establish its cause — it makes the next protected run attribute it. Note also that store-initialization failure is already excluded: ensure_store_initialized()'s bare Unavailable is reached through entry_for(), which installation_id() wraps in Entry.classify, so it would have reported installation_entry_unavailable.

After merge

An SDK rebinding is required before a protected run can use this, since evidenceProducer.commit names an exact Chat merge. SDK 404c2bf89 currently binds 1f69306 and both validator scopes point at it; merging this will make that binding stale.

Full analysis of the binding race is on OpenCoven/sdk#38 and #219.

@BunsDev

BunsDev commented Sep 16, 2026

Copy link
Copy Markdown
Member Author

Correction to the handoff above: main has since moved to 2b4dd40, so the branch's base (4355c19) is no longer current. All 11 required checks are green on b9b9823, but merging as-is would produce a merge whose tree differs from its reviewed second parent, which the SDK contract rejects — the defect that made #299 unbindable.

It needs one more rebase onto current main, with the lock pin and the golden expectations in src/phase1-conformance-lock.test.ts regenerated against the new sources SHA, before it is bindable. Everything else in the handoff stands.

BunsDev and others added 2 commits September 16, 2026 14:33
… store

The installation preflight reported `secure_store_unavailable` for two
unrelated causes: a secure store that is genuinely unavailable, and a custody
implementation that never overrode `installation_id` and fell through to the
default trait body. Protected run 35100084575 hit one of them on Windows and
the log cannot say which.

Give the default body its own fixed code, `installation_custody_unsupported`,
mapped to the bounded stage `native-preflight-installation-custody-unsupported`.
Every classified installation stage already rewrites `Unavailable` into its own
identifier, so this closes the last unclassified path on that operation. No
message, path or private value is published.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ostic

Points harness.revision and harnessAuthority.revision/tree at the sources
commit on this branch and refreshes the governed objects whose bytes changed.
The golden expectations in phase1-conformance-lock.test.ts follow.

Merge with a real merge commit, never squash; the pinned revision must stay an
ancestor of main.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@BunsDev
BunsDev force-pushed the fix/219-installation-custody-diagnostic branch from b9b9823 to 579fc4a Compare September 16, 2026 19:36
@BunsDev

BunsDev commented Sep 16, 2026

Copy link
Copy Markdown
Member Author

Status update — branch is green, base is stale

Current as of this comment:

value
branch head 579fc4a (lock repin)
sources commit / pinned revision fe58cb1, tree 7dc95a78
branch base 2b4dd40 (#309)
main 157fb329 commits ahead of the base
checks 11/11 pass, no skipped required contexts
state BLOCKED (base out of date for a bindable merge)

All 11 required contexts pass on this head, including Rust and Windows supervisor behavior — the ci:full label is applied, and pushing a fresh head removed the duplicate skipped/success check-run problem described earlier.

What it needs

A rebase onto current main, then the pin regenerated against the new sources SHA:

  1. phase1-conformance.lock.jsonharness.revision, harnessAuthority.revision/.tree, the changed files entries (schema-v2 producer, conformance workflow) and the src-tauri/src/keyring.rs production delta.
  2. src/phase1-conformance-lock.test.ts — the same values in expectedBehaviorAuthority.
  3. The frozen module table in .github/workflows/client-v1-conformance.yml and the two rows in the docs/phase1-conformance.md pin table, since the producer's bytes shift on each rebase.

Then merge with a real merge commit while main is unchanged.

Why it is still open

Four attempts, and main advanced during every one. A cycle is roughly 25 minutes, dominated by the macOS Rust cross-build that ci:full requires because this branch touches src-tauri/.

Worth noting for anyone sizing this: the last drift came from #310, a familiar-identity UI change with no relationship to conformance. Because the SDK contract requires the producer merge's tree to equal its reviewed second parent's tree, any commit to main closes the window, not just conformance work. Landing this from outside the lane means winning a 25-minute race against the whole repository; landing it from inside, sequenced against your own merges, is one pass.

I am not pushing to this branch again. It is at 579fc4a and ready for whoever takes it.

@BunsDev

BunsDev commented Sep 18, 2026

Copy link
Copy Markdown
Member Author

Closing in favour of #313, which is this same fix rebased after #311.

The custody change is byte-identical in both branches:

+  ['installation_custody_unsupported', 'custody-unsupported'],
+  'native-preflight-installation-custody-unsupported',

The difference is what else each branch carries. This one was cut before #311 landed, so its diff against current main also reverts #311's Cave build isolation — it drops the COVEN_HOME / COVEN_CAVE_HOME resolution from schemaV2CaveBuildEnvironment and narrows the signature back to one argument. Merging it would undo that work, which is why the branch now shows as conflicting.

#313 also already carries the test coverage the review asked for here: ['response', 'installation_custody_unsupported', 'custody-unsupported'] is in its src/native-preflight-diagnostics.test.ts change, so the NativeRpcClient.ok response mapping is exercised rather than only the derived allowlist.

Nothing from this branch is lost. Reopen if #313 turns out not to cover something.

@BunsDev BunsDev closed this Sep 18, 2026
@BunsDev
BunsDev deleted the fix/219-installation-custody-diagnostic branch September 21, 2026 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:full Run the macOS and Windows CI jobs on this pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants