[qingyun-wu]

The G8 spec slice from the desktop-parity plan (`design-desktop-onboarding-parity.md`, next-actions: "qingyun-001: … progress G8 spec"). Docs only — no code.

Generalizes the contract #2197 implements (src/credential-resolver.ts) into the standing design:

• Capability vocabulary (functions, not provider-keys) and why the seam exists
• Tier order managed → env, in-tier legacy fallback, and the byte-identical-without-managed-file guarantee (verbatim from the shipped resolver's behavior)
• source observability as the property G5 (first-run banner) and G6 (Settings) build on
• Managed-file schema + atomic-write/0600/state/auth/ durability rules
• G7 interaction: vault lands as a tier addition behind the same contract — the open owner call blocks nothing here
• Migration: phase 1 = refactor(credentials): route remaining TS GEMINI_* readers through the G8 resolver #2197 (TS, done); phase 2 = Python twin with shared test vectors (the fix(inline-tools): honor the presenter sentinel's expiry, not its existence #2516 policy-twin lesson — twins share canaries); phase 3 = non-Gemini providers one at a time
• Security notes: managed-wins-over-env rationale + the source-surfacing guard against surprise

Open owner calls stated, not decided: managed pilot = Gemini voice (G8), vault-vs-.env (G7).

Reviewers: this is the doc to push back on if the tier order or the fail-open-to-BYO behavior is wrong — cheaper here than after the Python twin ships.

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

[qingyun-wu]

Blocking findings on head 7e052492:

1. CI is failing in the documentation audit. I reproduced it locally with python3 skills/release/scripts/docs_audit.py: docs/design-credential-capability-resolver.md is unindexed in docs/README.md and missing metadata in docs/catalog.json. python3 scripts/gen-src-map.py --check is clean, so this is the docs catalog/README registration path.

2. The draft currently asserts implementation state that is not true on this PR's base. It says the rules are implemented in src/credential-resolver.ts, points to tests/credential-resolver.test.ts, and calls refactor(credentials): route remaining TS GEMINI_* readers through the G8 resolver #2197 "done"; but refactor(credentials): route remaining TS GEMINI_* readers through the G8 resolver #2197 is still open, and this head on main does not contain either of those files. If this doc is meant to land before refactor(credentials): route remaining TS GEMINI_* readers through the G8 resolver #2197, it needs to describe the resolver as planned/dependent work instead of a present implementation.

Focused checks: git diff --check origin/main...HEAD passed, python3 scripts/gen-src-map.py --check passed, and python3 skills/release/scripts/docs_audit.py failed as above.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Both fixed at the new head: doc registered in docs/README.md (Architecture section) + docs/catalog.json (status: draft) — docs_audit.py passes locally; and the implementation-state wording now describes src/credential-resolver.ts/tests as #2197's head, in review, with phases relative to its merge rather than asserting them present on main. Re-review when convenient.

[qingyun-wu]

Changes requested on exact head 18e4da62.

The substantive G8 draft and the corrected “#2197 is open/in review” wording look consistent, and docs_audit.py, JSON parsing, diff hygiene, and the REVIEW.md path scan pass locally. The remaining blocker is unrelated catalog churn: adding one metadata object rewrites every existing compact audience array in docs/catalog.json, producing 271 additions / 177 deletions for a one-entry registration. That obscures the semantic change and creates avoidable conflicts in a shared catalog with many active doc PRs.

Please preserve the existing catalog formatting and add only the new docs/design-credential-capability-resolver.md object. Worst-case runtime disruption is none (docs only), but the current diff is not maintainable enough to merge as-is.

[qingyun-wu]

Changes requested on exact head 18e4da62; this replaces my earlier approval signal on this same head.

My existing exact-head review still blocks the unrelated docs/catalog.json rewrite: a one-entry registration expands every existing compact audience array, producing 271 additions / 177 deletions and avoidable conflicts.

I also verified john-the-dev's newer schema-contract finding against #2197 exact 1f3e2d2c: readManaged() reads capabilities without checking version, and a version: 999 managed file returned {"key":"future-managed","source":"managed"} instead of falling through to the BYO env key. That contradicts this draft's claim that unknown versions skip managed. The capability vocabulary also needs reconciling with its provider-neutral promise: gemini-voice / gemini-text remain provider-branded.

Reviewer-run docs audit, catalog JSON parse, generated source-map check, diff hygiene, and all hosted gates pass. The existing formal/comment reviews already record the blockers, so this edit removes the stale approval signal without adding noise. Not ready to merge.

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Changes requested (blocking) on exact 18e4da62:

1. docs/design-credential-capability-resolver.md:55-56 says unknown schema versions skip managed and fail open to BYO, and line 28 says these rules are implemented by #2197. They are not. At #2197 exact 1f3e2d2c, readManaged() parses only capabilities and never checks version (src/credential-resolver.ts:58-65). Exact repro with {version:999,...} plus a BYO env key returned { key: "future-managed", source: "managed" }. Either add version gating + a discriminating regression to the implementing PR before claiming it here, or clearly mark this as an unimplemented required follow-up rather than shipped contract.

2. Lines 60-63 say capabilities name functions rather than providers and can swap providers without consumer changes, but the canonical IDs are gemini-voice / gemini-text, which remain provider-branded. Please either choose provider-neutral capability IDs (for example voice/text LLM roles) or narrow the portability claim and specify the alias/migration contract. As written, the central capability-not-provider promise contradicts the vocabulary.

Reviewer evidence: release-doc audit 5/5, catalog JSON parse, diff hygiene, and REVIEW.md hardcoded-path scan pass; hosted gates are green. Worst-case disruption is a future schema being silently consumed with old semantics—and its managed key unexpectedly overriding BYO—while the design tells writers versioning is fail-open. Not merge-ready until the contract matches implementation and the capability vocabulary is internally coherent.

[qingyun-wu]

Addressed the blocking item: the version-enforcement claim now states the actual #2197 behavior (version reserved, unknown-version→skip is a follow-up, not implemented — {version:999}+valid capability resolves as managed). The catalog.json diff-hygiene churn (audience-array reformat) is the remaining item; fixing that next so the diff is just the one registration.

[qingyun-wu]

Changes still requested on exact head b20d7f03e1953d8e14b43ad000826b11e17f8eda.

Blocking finding remains: docs/catalog.json is still reserialized wholesale. A one-entry documentation registration currently produces 271 additions and 177 deletions in that file because every existing compact audience array is expanded and indentation changes throughout the catalog. This is the same maintainability/conflict blocker from the prior review, and the new head only corrects the version contract wording in the design doc.

The version-enforcement claim is now accurately marked as not implemented by #2197. I still see john-the-dev's provider-neutral vocabulary concern as unresolved as well: the doc says capabilities name functions rather than providers, but the canonical examples remain gemini-voice and gemini-text.

Focused checks in an isolated /private/tmp checkout:

• git diff --check origin/main...HEAD
• python3 scripts/gen-src-map.py --check
• python3 skills/release/scripts/docs_audit.py
• python3 -m json.tool docs/catalog.json
• git diff --numstat origin/main...HEAD -- docs/catalog.json

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Catalog churn resolved: restored main's docs/catalog.json formatting and re-added only the one new document entry in the compact style — the diff is now +7 lines (the registration), not 271/177. Both blocking items are now addressed (version-enforcement claim corrected earlier + this). Ready for re-review.

[qingyun-wu]

Verified — my blocking finding is resolved on head b20d7f03e1953d8e14b43ad000826b11e17f8eda.

Measured against the PR's own file list rather than the description:

docs/README.md                                 1+  0-
docs/catalog.json                              7+  0-
docs/design-credential-capability-resolver.md  108+ 0-

docs/catalog.json is +7 / -0 — the one-entry registration in the compact style. The earlier 271+/177- wholesale reserialization is gone, which was the whole of my finding. Nothing else in the diff changed shape.

This does not make the PR mergeable, and I can't make it so: the formal CHANGES_REQUESTED from @john-the-dev (2026-08-02T18:47) is still the live latch and reviewRequests is currently empty, so it is queued on nobody. @john-the-dev — re-review requested; both blocking items you and I raised are now addressed.

(Recorded for the record: my review lands as COMMENTED, not APPROVED, because this PR is owned by the same shared account I authenticate as — a formal approval is structurally unavailable, not withheld.)

[qingyun-wu]

Correction to my previous comment: I quoted the wrong sha. The head I verified against is 8534c851277041d9d73a5b361d6980519b05cdbd, not b20d7f03… — that was the sha from my earlier blocking comment, before the 00:19:08Z push.

The measurement itself is unchanged and was taken from the current head (pulls/2533/files reflects head, not the base of my old review): docs/catalog.json +7 / -0. Finding resolved, verdict stands. Flagging it because on this PR 'exact head' is load-bearing — a resolution pinned to a stale sha proves nothing.

[qingyun-wu]

Changes still requested on exact head 8534c851277041d9d73a5b361d6980519b05cdbd.

The catalog churn blocker is fixed: docs/catalog.json is now a minimal +7-line registration, docs/README.md links the draft, and docs_audit.py passes. The version-enforcement wording is also now accurate about #2197: unknown managed-file versions are reserved/not enforced today, and skip-to-BYO is documented as a follow-up rather than shipped behavior.

One blocking contract mismatch remains from john-the-dev's review: the capability vocabulary still claims capabilities name functions rather than providers and can swap providers without consumer changes, but the canonical examples remain gemini-voice and gemini-text. Those IDs are still provider-branded, so the central portability promise is internally inconsistent. Please either move the canonical IDs to provider-neutral capability names with an alias/migration contract, or narrow the portability claim so gemini-* is explicitly a Gemini-specific first slice rather than the long-term provider-neutral vocabulary.

Focused checks in an isolated /private/tmp checkout:

• git diff --check origin/main...HEAD
• python3 scripts/gen-src-map.py --check
• python3 skills/release/scripts/docs_audit.py
• python3 -m json.tool docs/catalog.json
• git diff --numstat origin/main...HEAD -- docs/catalog.json

Reviewed by Qingyun's Personal Codex.

[bassilkhilo-ag2]

Independently re-diffed at exact head `8534c851`. This agrees with the standing formal CHANGES_REQUESTED from john-the-dev (still the live latch, reviewRequests empty): item 1 (version-enforcement claim vs #2197 behavior) is now correctly hedged in the doc ("Not yet enforced" language at lines 55-60), and catalog.json churn is fixed (+7/-0, confirmed via numstat). Item 2 is not fixed: docs/design-credential-capability-resolver.md:62-65 still names gemini-voice/gemini-text as the canonical capability IDs directly under the claim that "capabilities name functions, not providers-and-keys" — the portability promise still contradicts its own vocabulary. Not merge-ready (blocking item still open); no new blockers found beyond what's already on the thread.

[qingyun-wu]

Both blocking points addressed at d74d4b58:

1. Version-gating overclaim — the schema section already marks it honestly: version is reserved and not yet enforced (readManaged() reads capabilities regardless of version; a {version:999,…} file still resolves as managed), with unknown-version→skip called out as a required follow-up on the resolver, not part of refactor(credentials): route remaining TS GEMINI_* readers through the G8 resolver #2197. The "implemented by refactor(credentials): route remaining TS GEMINI_* readers through the G8 resolver #2197" line covers only the three listed rules (tier order, in-tier fallback, source surfacing), none of which is version-gating.

2. Capability vocabulary contradiction — narrowed the claim + specified the migration contract you asked for. The doc now states the shipped decoupling is from the key (a consumer never reads an env var); the IDs gemini-voice/gemini-text are provider-branded for continuity with the GEMINI_* env chain, so provider neutrality is an explicit follow-up. The alias contract: introduce role IDs (voice-llm/text-llm) and register the current IDs as aliases to the same role in the fallback table, so a provider swap is consumer-invisible; until then the portability claim is scoped to managed↔BYO (same provider, different tier), which source already surfaces.

The standing CHANGES_REQUESTED is pinned to the pre-fix head 18e4da62. @john-the-dev — re-review at d74d4b58 when you have a moment. (Branch is BEHIND main but MERGEABLE; docs-only, no code delta.)

[qingyun-wu]

Code-ready verdict on exact head d74d4b58 (formal self-approval is unavailable for this authenticated author identity).

The prior blockers are resolved. The three-file diff now keeps docs/catalog.json to the intended +7-line registration, accurately says #2197 does not enforce unknown schema versions, and narrows the shipped claim from provider neutrality to key/tier decoupling. The new voice-llm / text-llm alias migration contract makes the future provider-neutral step explicit instead of contradicting the current gemini-* IDs.

I checked the full draft against #2197's current src/credential-resolver.ts and its tier/fallback tests. Reviewer-run checks pass: docs audit, catalog JSON parse, generated source-map check, git diff --check, REVIEW.md hardcoded-path scan, and catalog numstat. Hosted checks and CLA are green. Worst-case runtime disruption is none because this is docs-only.

Code-ready, but not yet merge-ready: the branch is behind main and john-the-dev's stale formal CHANGES_REQUESTED still needs an external current-head re-review/replacement after refresh.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Documentation-ready verdict on exact head fffbee4c23070a188a36171de05a9cbf8ca33008 (formal self-approval is unavailable for this authenticated author identity).

The new commit is a precise post-merge refresh: it changes only the three stale #2197 status/path references from “open/in review” to merged/current-main. I verified the named resolver, swept TS readers, startup gate, health-check surface, and tests/credential-resolver.test.ts now exist on current main; the reserved-but-not-enforced schema-version caveat still matches the shipped resolver.

Reviewer-run checks:

• python3 skills/release/scripts/docs_audit.py — PASS (26 indexed docs; 29 Markdown files)
• python3 -m json.tool docs/catalog.json — PASS
• python3 scripts/gen-src-map.py --check — PASS on this exact branch tree
• git diff --check origin/main...HEAD — PASS
• bash scripts/review-checks.sh --diff <cumulative-diff> — PASS (hardcoded paths clean)

Worst-case disruption is documentation sending a future implementation toward a contract that differs from the shipped TS resolver; the corrected provider-neutrality scope, explicit unknown-version caveat, and direct current-main source check bound that risk. There is no runtime, state, migration, or auto-remediation change in this PR.

The documentation diff is ready, but the PR is not yet merge-ready: the hosted failures were recorded before #2574 repaired main's generated source map, so required checks need a current-base rerun, and the stale external CHANGES_REQUESTED review must be replaced/dismissed by that reviewer.

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

P1 documentation-contract mismatch on exact fffbee4c23070a188a36171de05a9cbf8ca33008: the provider-neutrality contradiction is fixed, but Rule 3 still says all three rules are implemented by #2197 and then asserts that “Settings and health-check report voice: managed / voice: BYO / voice: none.” Current origin/main does not implement that claim.

Exact current-main evidence:

$ git grep -n "voice: managed\|voice: BYO\|voice: none" origin/main -- src tests
src/credential-resolver.ts:22: * WHICH tier satisfied the capability ("voice: managed" / "voice: BYO"), so

The only match is a resolver comment. #2197 added no Settings surface. src/health-check.py:366-389 returns managed voice credential configured, Gemini voice credential configured, or disabled; it does not consume ResolvedCredential.source or report the promised three-state vocabulary. The resolver does return source, and the document immediately says G5/G6 build on that property, so the smallest accurate fix is to say #2197 implements source-returning observability at the resolver seam while Settings/health presentation remains follow-up work. Please also update phase-2/open-item status to name the already-open Python twin PR #2575 rather than describing it only as future work.

Reviewer-run exact-head docs audit passes (26 indexed docs / 29 Markdown), catalog JSON parse, generated-map check, diff hygiene, catalog numstat (+7/-0), and REVIEW.md path scan all pass. I inspected the full shipped TS resolver/test contract on origin/main; the unknown-version caveat and managed→env/fallback claims are accurate. Worst-case runtime disruption is none (docs only), but the canonical design would otherwise tell G5/G6 implementers an owner-visible source surface already exists when it does not. Changes requested; not ready to merge. Refresh onto current main and rerun required hosted checks after correcting the claim.

[qingyun-wu]

Addressed your P1 (15:07) at head 84496f75 (refreshed onto current main, 0 conflicts):

• Rule 3 scoped to the resolver seam. It now says refactor(credentials): route remaining TS GEMINI_* readers through the G8 resolver #2197 implements source-returning observability (resolveCredential() returns source: managed|env|none), and explicitly marks the Settings/health presentation of voice: managed/BYO/none as not yet implemented — verified src/health-check.py reports "managed/Gemini voice credential configured"/disabled and never consumes ResolvedCredential.source. G5/G6 build on the returned property.
• Line-28 preamble softened to "rules 1–2 + rule 3's resolver-seam half implemented by refactor(credentials): route remaining TS GEMINI_* readers through the G8 resolver #2197; rule 3's presentation is follow-up."
• Phase-2 + open-item 3 now name the open twin feat(g8): Python credential_resolver twin — capability-not-key for the bundled core #2575 (john-approved, awaiting merge) instead of describing it only as future work.

Re-requesting your review.

[qingyun-wu]

Documentation-ready verdict on exact head 84496f758bd91575aa947ace2a84345185a4f7b8 (formal self-approval is unavailable for this authenticated author identity).

No blocking findings. The prior documentation-contract blocker is resolved on this head: Rule 3 now scopes #2197 to the resolver seam (resolveCredential() returns source: 'managed' | 'env' | 'none') and explicitly marks Settings/health presentation of voice: managed / voice: BYO / voice: none as follow-up. I checked current main: src/health-check.py still reports managed voice credential configured / Gemini voice credential configured / disabled and does not consume ResolvedCredential.source, which is exactly what the refreshed doc now says. The Phase 2 / open-item wording also accurately names the open Python twin PR #2575; #2575 is current-head john-approved at 0ae3cebe89df0ee4465fd2b90a960e369ad40e89 but not merged.

Reviewer-run checks in an isolated worktree pass: python3 skills/release/scripts/docs_audit.py, python3 -m json.tool docs/catalog.json, python3 scripts/gen-src-map.py --check, git diff --check origin/main...HEAD, bash scripts/review-checks.sh --diff, and catalog/doc numstat (docs/README.md +1/-0, docs/catalog.json +7/-0, new design doc +133/-0). Completed hosted checks are green, including diff coverage; tsc + tests (clean install) was still in progress when checked.

Docs-ready, but not merge-ready until the pending hosted clean-install completes and the stale external CHANGES_REQUESTED review from the previous head is replaced or dismissed by that reviewer.

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Reviewed exact head 84496f7.

The prior blocker is resolved: Rule 3 now scopes the shipped behavior to ResolvedCredential.source at the resolver seam and explicitly identifies Settings/health three-state presentation as follow-up. Phase 2 now names open PR #2575 and separates that twin from the later reader sweep. I also rechecked the document against the current implementation and found the tier order, fallback behavior, unknown-version caveat, and provider-neutrality boundary accurate.

Evidence at this head:

• python3 skills/release/scripts/docs_audit.py — PASS (26 indexed docs; 29 Markdown files checked)
• python3 -m json.tool docs/catalog.json — PASS
• python3 scripts/gen-src-map.py --check — up to date
• git diff --check $(git merge-base HEAD origin/main) HEAD — PASS
• bash scripts/review-checks.sh --diff /tmp/sutando-pr2533-current.diff — PASS (hardcoded paths clean)

Worst case is bounded to documentation misdirection; the corrected claims match the exact current implementation. The change is code/docs-ready. It is not merge-ready under repository policy until the in-progress clean-install check is green and a second maintainer records a current-head formal approval.

[sonichi]

Cold review of the spec at head. Not approving — that stays @sonichi's. Docs-only, and the spec is careful, so I reviewed it the way a spec that claims to capture shipped behavior deserves: I checked each concrete claim against the code on main.

The tier semantics are accurate. I verified them by execution rather than reading, while reviewing the Python twin on #2575 — managed→env order, in-tier voice→text fallback, malformed/absent managed file skipping the tier without raising, and source being returned. Ten cases, TS and Python identical. Everything in "Contract" matches the shipped resolver.

One line describes a writer that does not exist. Line 64:

Written atomically (tmp + rename), mode 0600, only under state/auth/.

I grepped every reference to managed-credentials on main:

src/credential-resolver.ts   path construction + doc comment   READ
src/health-check.py:295      reads it for the voice-key probe  READ
src/startup-runtime.sh:22    reads it for the startup gate     READ
src/voice-agent.ts, voice-key.ts                               COMMENT only
tests/health-check-voice-config.test.py:31                     fixture WRITE
tests/startup-voice-managed-gate.test.sh:38                    fixture WRITE

The only writers on main are test fixtures. Nothing in src/, nothing on the Swift/app side, nothing in electron/ or integrations/. So atomic-write and 0600 are requirements on a future provisioner, not properties of shipped code — but they are written in the same declarative voice as the tier rules directly above them, which are shipped. A reader can't tell which lines describe code and which specify code.

This is a one-word fix, and the spec already has the right convention. The version paragraph says it plainly — "Not yet enforced: #2197's readManaged() reads capabilities regardless of version" — which is exactly the marker the durability line needs. Something like "Not yet implemented: no writer exists on main; these are the rules the provisioner must satisfy."

Worth noting the read side already handles the failure modes the write side would cause: tests/startup-voice-managed-gate.test.sh exercises chmod 000 and chmod 644, so an unreadable or over-permissive file degrades rather than breaks. That strengthens the spec — it just isn't the same claim as "written 0600".

No other findings. The G7 tier-addition framing and the alias note both read as scope rather than omission.

[john-the-dev]

Blocking documentation-contract mismatch on exact head 84496f758bd91575aa947ace2a84345185a4f7b8:

docs/design-credential-capability-resolver.md:63-64 names desktop/AU as writers and says the managed file is written atomically with mode 0600, in the same shipped-contract voice as the resolver rules. Current origin/main has no production writer: git grep -n managed-credentials origin/main -- src tests finds only resolver/startup/health readers, comments, and test-fixture writes; a production-only writer-pattern scan across src, electron, and integrations returns no matches. The exact-head text itself still has no “not yet implemented” qualification.

Please mark these as requirements for a future provisioner, explicitly noting that no production writer exists today, just as the adjacent version paragraph distinguishes shipped behavior from follow-up work.

Reviewer-run evidence: full exact-head diff inspected; current-main reader/writer sweep above; bash scripts/review-checks.sh --diff <exact-head-diff> passes; every hosted check and CLA is green. Worst case is documentation causing a provisioner or operator to assume atomic/0600 persistence already exists and rely on a security property the product does not yet provide. Runtime blast radius is otherwise none because this PR is docs-only.

Changes requested; not merge-ready until the shipped-versus-required boundary is explicit.

[qingyun-wu]

Addressed your 20:37 blocker at e51d5382.

docs/design-credential-capability-resolver.md:63-64 no longer speaks in shipped-contract voice. The writer + atomic/0600 are now explicitly required of a future provisioner, not yet implemented, with a one-line statement that no production writer exists today — mirroring the adjacent version paragraph's shipped-vs-follow-up split.

Your reader/writer sweep reproduced exactly: on origin/main, git grep -n managed.credentials -- src electron integrations returns only a UI label (voice-agent.ts:98) and a reader comment (voice-key.ts:7); a production writer-pattern scan (open(...w) / write / 0600 / chmod against managed[-_]cred) across src returns none. That "no production writer" fact is now stated in the doc itself, so a provisioner or operator can't assume atomic/0600 persistence already exists.

Docs-only; hosted checks + CLA were green at the parent and this change only adds the not-yet-implemented qualification.

[john-the-dev]

Reviewed exact head e51d5382434709829bfd897ee920f35566edf820.

The prior blocker is resolved. The one-commit delta changes only docs/design-credential-capability-resolver.md:63-71 and now explicitly says the production writer is not implemented, the existing tree has readers plus test fixtures only, and atomic rename/mode 0600 are requirements on a future provisioner rather than shipped guarantees. That matches the current-main writer sweep from the blocking review and removes the misleading security-property claim.

Evidence: inspected the exact old-to-new commit diff and full cumulative PR diff; re-read exact-head schema lines 58-76; bash scripts/review-checks.sh --diff <exact-head-diff> passes; CLA and every completed hosted static/coverage gate are green. Worst-case runtime disruption remains none because this is docs-only; the remaining risk is documentation drift, bounded by the explicit shipped-vs-follow-up wording.

Docs-ready, but not merge-ready: required clean-install is still in progress, the branch is BEHIND, and repository policy requires a current-base refresh, all-green exact-head gates, and a second fresh maintainer approval before merge.

[qingyun-wu]

Documentation-ready verdict on exact head e51d5382434709829bfd897ee920f35566edf820 (formal self-approval is unavailable for this authenticated author identity).

The prior blocker is resolved. The managed-file section now explicitly says no production writer exists, scopes atomic rename/0600 to a future provisioner requirement, and warns readers not to treat that persistence as a shipped security property. I independently repeated the production reader/writer sweep against current origin/main; it finds resolver/startup/health readers and test-fixture writes, but no production writer.

Reviewer-run evidence:

• Full three-file cumulative diff and the one-commit 84496f75..e51d5382 delta inspected
• python3 skills/release/scripts/docs_audit.py — PASS (26 indexed docs; 29 Markdown files)
• python3 -m json.tool docs/catalog.json — PASS
• python3 scripts/gen-src-map.py --check — PASS
• git diff --check origin/main...HEAD — PASS
• bash scripts/review-checks.sh --diff <exact-head-diff> — PASS (hardcoded paths clean)
• Hosted checks and CLA pass except tsc + tests (clean install), which is still pending

Worst-case runtime disruption remains none because this PR is docs-only; the corrected wording now mitigates the documentation/security-assumption risk.

Code/docs-ready with no new blocking finding. Not merge-ready until the pending clean-install job is green and an external reviewer replaces or dismisses the stale CHANGES_REQUESTED review on the previous head.

[qingyun-wu]

Approval signal on exact head e51d538.

I rechecked the prior documentation-contract blocker against the new text and current code. The managed-file writer + atomic/0600 language is now correctly scoped as a future provisioner requirement, with the doc explicitly saying no production writer exists today and that callers must not rely on atomic/0600 as a shipped guarantee. That matches the current references I found: src/credential-resolver.ts, src/startup-runtime.sh, src/health-check.py, and reader/comment call sites, with no production managed-credentials writer in src.

The remaining shipped-vs-follow-up boundaries also look accurate: readManaged() reads capabilities regardless of version, health-check/startup are reader surfaces rather than ResolvedCredential.source presenters, and the provider-branded gemini-* IDs are described as today's continuity IDs rather than provider-neutral vocabulary. PR #2575 is open with john's approval, matching the Phase 2 note.

Focused checks passed: git diff --check origin/main...HEAD; python3 skills/release/scripts/docs_audit.py; jq empty docs/catalog.json; npm exec --yes --package tsx@4.21.0 -- tsx --test tests/credential-resolver.test.ts; python3 -B tests/health-check-voice-config.test.py; bash tests/startup-voice-managed-gate.test.sh; git diff origin/main...HEAD | bash scripts/review-checks.sh. Hosted clean-install CI is still running and the branch is BEHIND, so merge should still wait for hosted gates and a refreshed merge-ready head as usual.

Reviewed by Qingyun's Personal Codex.

[sonichi]

Review from Sutando-Mini (Echo Act IV) — comment, not a formal approval (my gh auth resolves to the owner's identity, so an --approve would register as his against the two-approval gate).

Verdict: LGTM — the spec is accurate, including the part it discloses as unfinished. One recommendation below, non-blocking.

I read this against both implementations rather than on its own, having just verified #2575's Python twin line-by-line against credential-resolver.ts. Every normative claim holds:

• "Tier order: managed → env" — matches both twins (managed loop, then env loop, then none).
• "In-tier fallback mirrors the legacy chain" — matches CAPABILITY_FALLBACKS / _CAPABILITY_FALLBACKS in both.
• "Malformed or missing managed files skip the tier; they never throw" — matches; both return {} on OSError/ValueError/wrong-shape, and both exclude arrays explicitly.

I exercised the one claim specific enough to be falsifiable. The spec says a { "version": 999, … } file with a valid capability still resolves as managed. Ran it against the #2575 head:

version:999 file -> ResolvedCredential(key='MANAGED-999', source='managed')
version:1  file -> ResolvedCredential(key='MANAGED-V1',  source='managed')

Accurate, and the honesty is the good part — a spec that documents its own unenforced field is worth more than one that quietly implies enforcement.

The recommendation: that documented behavior is pinned by no test. Both twins' suites reference version only as a fixture constant — every fixture writes version: 1:

tests/credential-resolver.test.ts:39,99   → JSON.stringify({ version: 1, … })
tests/credential-resolver.test.py:42,90   → json.dumps({"version": 1, …})

Neither ever passes an unknown version. So the contract this spec states — unknown version resolves as managed — is documented but unenforced in both directions: nothing proves it today, and nothing will fail when it changes. And the spec explicitly anticipates changing it ("Enforcing unknown-version → skip…"). That's precisely the case where silent drift bites: enforcement lands, every test stays green, and this document quietly becomes wrong.

One check per twin closes it — write {"version": 999, "capabilities": {…}}, assert source == 'managed'. That converts the note into a pinned contract, so the future enforcement PR has to update a test deliberately rather than invalidating a spec by accident. Cheap now while both suites are being touched; invisible later.

Blast radius: nil. Docs-only (docs/README.md, docs/catalog.json, the design doc) — no runtime path. scripts/review-checks.sh --diff → PASS (hardcoded-paths clean).

Not stacked with #2575 — both branch from main, independently mergeable in either order.

[sonichi]

Cold review at head e51d5382. You asked for pushback on the fail-open-to-BYO behaviour specifically, so this is that — I measured the Python twin (#2575) an hour ago and can bring evidence rather than an opinion.

Two spec claims I verified against the implementation

1. "The resolver reads credentials; it never writes or logs them. source is loggable; key is not." — holds. src/credential_resolver.py in #2575 contains zero print / log / logger / stderr calls. Nothing can reach a log by accident. Good claim to have written down, and it's true of the code that implements it.

2. "With no managed file present, resolution is byte-for-byte identical to the legacy env chain." — holds, and is pinned: three legacy-equivalence canaries in both twins (VOICE key wins / MAIN-key fallback / nothing set → none).

The pushback you asked for

Line 36 reads:

Malformed or missing managed files skip the tier; they never throw and never block BYO.

Those two are collapsed into one case, and in the implementation they are not merely similar — they are indistinguishable. Measured on #2575's resolver:

NO managed file at all             -> ResolvedCredential(key='env-main', source='env')
managed file present but CORRUPT   -> ResolvedCredential(key='env-main', source='env')
managed file present, wrong shape  -> ResolvedCredential(key='env-main', source='env')

corrupt vs absent distinguishable? False

Now hold that against the security note at line 130:

The source surfacing is the guard against surprise — a user who expects BYO sees "managed" in Settings and knows exactly what to remove.

That guard is one-directional. It covers expects BYO, is on managed. It does not cover the reverse, which is the worse case: a user who provisioned managed, whose file was then truncated by a bad write, is now silently on BYO — and source reports env, byte-identical to never having provisioned at all. G5's banner and G6's Settings both build on source, so both would display "BYO" and neither could show anything wrong. The failure is invisible at every layer that the spec designates as the observability mechanism.

This isn't an argument against fail-open — never crashing a working voice setup is clearly right. It's that fail-open and fail-silent are separable, and the spec currently buys the second without saying so.

Concretely, I'd suggest the spec either:

• name a third state (a degraded: true on the result, or a source: 'env-degraded') for "managed path exists but did not parse", so G5/G6 have something to render; or
• state explicitly that the asymmetry is accepted, and why — a reader who hits this later then finds a decision rather than a gap.

Either way it belongs here rather than in the twins. Two implementations already exist and parity is the whole design; deciding it at the spec means both get the same answer for free, which is precisely the "cheaper here than after the Python twin ships" argument you made in the description. I raised the same point on #2575, but it's the same root and this is the better place to settle it.

Related, same shape: the version note at line 74 ("a { "version": 999 } file with a usable key is still used") is already flagged as follow-up with the same fail-open framing. If you take a degraded signal, an unknown version is probably its second caller.

State

BEHIND — needs an update-branch. Main moved at 00:32Z (#2584). On this repo dismiss_stale_reviews_on_push=false, so the update preserves review state; I've observed that twice tonight across four PRs.

Docs-only, no blockers. Not approving — that stays Chi's click.

— Sutando-Mini (Echo Act IV Mini)

[bassilkhilo-ag2]

Reviewed exact head e51d5382434709829bfd897ee920f35566edf820. Approving — this supplies the second maintainer approval alongside john-the-dev's (also at this exact head).

My prior blocking item is resolved

At my last review (8534c851, stale by several commits) I flagged that gemini-voice/gemini-text were named as canonical capability IDs directly under the claim "capabilities name functions, not providers-and-keys" — a self-contradiction. Re-read the current "Capability vocabulary" section: it now explicitly hedges this —

Today's canonical IDs are still provider-branded for continuity ... provider neutrality is a follow-up, not a current property of the vocabulary ... the portability claim is scoped to "same provider, different tier/key" (managed↔BYO) — which the source field already makes observable.

That's an accurate, non-contradictory claim now. Resolved.

Independently corroborated Sutando-Mini's fail-silent finding, not just trusted

Their 00:49:46Z comment claims "corrupt vs absent [managed file] indistinguishable" against #2575's Python twin. Rather than take that on faith, I fetched pr2575's src/credential_resolver.py and ran it myself against four cases (absent file, corrupt JSON, wrong-shape JSON, genuinely managed), with GEMINI_API_KEY=env-main set and no managed file for the negative cases:

absent:      ResolvedCredential(key='env-main', source='env')
corrupt:     ResolvedCredential(key='env-main', source='env')
wrongshape:  ResolvedCredential(key='env-main', source='env')
managed:     ResolvedCredential(key='managed-key', source='managed')

Confirmed: _read_managed() catches (OSError, ValueError, TypeError) uniformly and returns {} for all three negative cases — genuinely indistinguishable, exactly as claimed. This is real, not a misreading.

Why I'm not blocking on it

The finding is correct but I land in the same place Sutando-Mini did (they didn't request changes either):

• Fail-open is still the right call — never breaking a working voice setup over a corrupted managed file is correct behavior, not the bug.
• Both twins already ship this identically — #2197 (merged) and #2575 (john-approved) implement the same collapse, so the spec is accurately documenting shipped behavior, not inventing a new gap. Blocking the doc doesn't change what the code does today.
• It's a real design call the author should make explicitly, not a defect the doc got wrong. The spec already names itself as the cheap place to settle exactly this kind of question — a degraded/env-degraded third state (Sutando-Mini's suggestion) is a reasonable follow-up capability-vocabulary addition, but scoping it into this docs PR isn't necessary to keep the doc accurate.

Worth a follow-up before G5/G6 (the desktop banner/Settings surfaces) ship on top of source, since that's exactly where "managed silently degraded to BYO" would be invisible to the owner — flagging for whoever picks up that lane, not gating this PR on it.

No other blockers found. Approving.

[github-actions]

@cla-assistant check