Conversation
There was a problem hiding this comment.
🟡 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
harnessAuthoritytoddc4eb29.../18fae508..., but the current overview at docs/phase1-conformance.md:166-170 still says the lock selects6e74fb60.../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', |
8460866 to
60e9dec
Compare
60e9dec to
b9b9823
Compare
Handoff — this PR is ready but I am stopping hereI 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 State
Both signed. Base is Verified locally
Three constraints, in the order they bite
One unresolved CI issueEarlier heads accumulated two workflow runs — a pre-label one where What this change doesSeparates two causes that both reported 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: After mergeAn SDK rebinding is required before a protected run can use this, since Full analysis of the binding race is on OpenCoven/sdk#38 and #219. |
|
Correction to the handoff above: It needs one more rebase onto current |
… 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>
b9b9823 to
579fc4a
Compare
Status update — branch is green, base is staleCurrent as of this comment:
All 11 required contexts pass on this head, including What it needsA rebase onto current
Then merge with a real merge commit while Why it is still openFour attempts, and 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 I am not pushing to this branch again. It is at |
|
Closing in favour of #313, which is this same fix rebased after #311. The custody change is byte-identical in both branches: The difference is what else each branch carries. This one was cut before #311 landed, so its diff against current #313 also already carries the test coverage the review asked for here: Nothing from this branch is lost. Reopen if #313 turns out not to cover something. |
Integrated follow-up Chat #313 is published at signed head
157aa554bf908a2b1c396692b490f6cfa4d4a5ab, binding signed sourceaa4d33004c59202cae70e3fc5a56a70cdf451dac. 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 testunsupported_custody_installation_is_distinct_from_an_unavailable_storepassed. 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 head579fc4abdb0ace683a394fb81fd496dc3277e099has four integration conflicts: workflow, conformance documentation, conformance lock, and lock tests. This supersedes the earlier clean-merge observation against the pre-#311 base. The missinginstallation_custody_unsupportedRPC 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
579fc4abdb0ace683a394fb81fd496dc3277e099incorporates #309 and pins executable sourcefe58cb1d4353b44fc61884ad76802ce924571526. Earlier identities and conflict reports are historical.Required before landing:
['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.docs/phase1-conformance.mdstill names source7dda439daa59ee350ff12cd195ff169b52e1085cinstead of this PR's pinned source.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.harnessAuthority.revisionis pinned tofe58cb1d, 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 ifmainhas 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_unavailablecovered two unrelated causes on the installation preflight: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 rewritesUnavailableintoinstallation_{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 asphase1.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 bareUnavailablewhenwindows_native_keyring_store::Store::new()fails (keyring.rs:2107-2113), which is plausible on a CI runner — but it is reached throughentry_for(), whichinstallation_id()wraps inEntry.classify. A Credential Manager that failed to initialize would already have reportedinstallation_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)
cargo test --features phase1-conformance --lib keyringvitest run src/native-preflight-diagnostics.test.tsvitest run --config vitest.heavy.config.ts src/phase1-conformance-lock.test.tsvitest run(full normal suite)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.
ddc4eb2918fae5088460866Three governed objects changed: the schema-v2 producer, the conformance workflow (its frozen module table), and the
keyring.rsproduction 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