Repository navigation
feat(credentials): S1 voice-source truth table — voicePreference + logout quarantine across every consumer (voice plan WS2 Steps 3–6) - #2691
Conversation
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Reviewed fresh (came out of draft this pass, no prior reviews). Ran everything myself rather than trusting the PR description:
tests/voice-preference-consumers.test.sh: 20 rows x 4 consumers (TS resolver, Python resolver, health-check.py, startup-runtime.sh shell gate), all agree - PASS.tests/credential-resolver.test.pyandtests/credential-resolver.test.ts: PASS (33/33 combined).- Verified the harness isn't vacuous - it imports/sources the REAL production files directly, not reimplementations: dynamic import of the real TS resolver,
from credential_resolver import ...on the real module,spec_from_file_locationloading the realsrc/health-check.py, andsource .../src/startup-runtime.shcalling the real_managed_voice_credential_present/configure_startup_runtimeshell functions. - Spot-checked the security-critical property directly against the matrix output:
entries.prefManaged.quarantined.envKey -> noneon BOTH the TS and Python resolvers - a quarantined managed entry undermanagedpreference does NOT fall back to an env key, matching the stated "env keys never silently substitute" guarantee.entries.prefUnset.quarantined.envKey -> byokcorrectly preserves legacy behavior when preference is unset, as claimed.
Didn't read every line of the 33-file diff, but the load-bearing claims (quarantine can't be bypassed, all 4 consumers agree, unset preserves legacy behavior) are directly verified, not just described. Docs commit (72084dda) not deep-read.
|
Merge position: Stage 3 — after #2689 (merge commit, not squash). Sibling of #2690 (either order between the two). The desktop control-plane PR (ag2-space/ag2space-cinny-desktop#287) is the writer of the policy fields this PR reads; both sides fail safe in either merge order, and the packaged activation happens via the desktop pin-bump PR after both trains land. 🤖 Generated with Claude Code |
|
Review note (exact head |
john-the-dev
left a comment
There was a problem hiding this comment.
Approved current head 72084dd after exact-head validation: credential-contract-golden 32/32 PASS; credential-proxy-refresh 21/21 PASS; voice-connect-resolver 8/8 PASS; voice-mode-resolver 7/7 PASS; health-check-voice-config 25/25 PASS. GitHub currently reports MERGEABLE but BEHIND; required CI is not present on this head, so do not merge until branch is updated and checks pass.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 77%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
…y engine consumer
Design 2b (impl plan WS2 Steps 3-4 + the engine half of Step 6; amendments
S1, R15 read side, S3, Y4 read side): managed-credentials.json's top-level
voicePreference / quarantined policy fields are now honored by every
engine-side credential consumer, implementing the SHARED S1 truth table:
- unset => legacy managed(voice->text)->env resolution (byte-identical
regression-pinned behavior for every pre-preference install);
- managed => ONLY a non-quarantined managed entry satisfies the voice
capability — a present env key must NOT silently satisfy a
managed preference (the logout-quarantine bypass);
- byok => the managed tier is skipped entirely; only env keys satisfy;
- quarantined entries are treated as ABSENT in every mode (and for the
text capability too — quarantine is revocation, not source choice).
Consumers changed in lockstep:
- src/credential-resolver.ts: readManaged returns caps+policy; the S1
gates above; exported credentialSourceLabel() maps the internal
'managed'|'env'|'none' onto the design's 'managed'|'byok'|'none'
vocabulary (voice-agent-state's frame mapper now delegates to it, so
the env->byok mapping has one definition). R15: top-level
preferenceRevision/sessionRevision tolerated + ignored on read.
- src/credential_resolver.py: full twin parity, including the S3/Y4
read-side generation reporting the TS resolver already had (managed
entry `generation` field; SUTANDO_VOICE_CREDENTIAL_GENERATION for
materialized env keys; manual/legacy keys stay generationless).
- src/startup-runtime.sh: _managed_voice_credential_present grows the
quarantine/byok guards; new _voice_credential_preference helper;
configure_startup_runtime decides managed-preference installs by the
managed gate ALONE (env keys never enable them) and names the reason
in every disabled branch. Interpreter probing factored into
_voice_gate_python (same canonical order, loud UNKNOWN warning kept
at the gate call site).
- src/health-check.py: managed_voice_credential_present gets the same
two guards; new managed_voice_preference(); resolve_voice_health_config
implements the S1 order with effective-source details ("BYOK
preference set (managed entries ignored)" / voicePreference=managed
reasons) so disabled states read as disabled-with-a-reason.
Cross-consumer guardrail (Step 6, engine half): a 14-row fixture matrix
(tests/fixtures/voice-preference-matrix.json — {entries x unset/managed/
byok x quarantined x env key}) driven over the TS resolver, the python
twin, the shell gate + launcher decision, and health-check by
tests/voice-preference-consumers.test.sh; every row asserts one agreed
effective source. The desktop supervisor half (spawn-env injection +
requires gate) is covered desktop-side by
engine/backend-supervisor.voice-preference.test.mjs consuming the same
fixture verbatim — the fixture documents its
supervisorInjectedManagedKey/voiceEnabled columns for that twin.
Also: tests/startup-voice-managed-gate.test.sh's bundled-runtime CONTROL
still asserted the pre-T1 "silently fall through" python-bin contract;
under T1's fail-closed smoke test (already on this branch) the assignment
aborted the suite under set -e before the final assertions ran. The
control now tolerates the loud non-zero exit and keeps asserting the
removed bundled path is never claimed.
Suites: tests/credential-resolver.test.ts (25) + .py twin,
tests/startup-voice-managed-gate.test.sh, tests/voice-preference-consumers.test.sh,
tests/health-check-voice-config.test.py (25), full TS suite 787/787,
health-check-*.test.py family, tsc --noEmit, eslint, ruff, shellcheck -S error.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… noEntries x prefUnset/prefByok
The Step 6 cross-consumer harness has always had a managedFile:null branch
(materialize NO managed-credentials.json) but no row ever exercised it, and
noEntries only covered prefManaged. Add the six missing rows:
- noFile.envKey / noFile.noEnvKey — file absent (every pre-managed
install): resolution must be byte-for-byte the legacy env chain.
- noEntries.prefUnset.{envKey,noEnvKey} — empty managed tier under the
legacy walk falls through to env.
- noEntries.prefByok.{envKey,noEnvKey} — byok with an empty managed tier
is env-only, none without a key.
All four engine consumers (TS resolver, py twin, shell gate + launcher,
health-check) agree on all 20 rows; desktop columns
(supervisorInjectedManagedKey / voiceEnabled) are pinned for the twin.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n the resolver spec CONTRIBUTING's feature/documentation-consistency rule: this branch changes the resolution contract (voicePreference / quarantined / generation), and docs/design-credential-capability-resolver.md is canonical for it. Adds the S1 truth-table subsection, extends the schema example with the policy fields, updates last_verified. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
02c283e to
fc15f90
Compare
|
@cla-assistant check |
|
Stack rebuild complete at |
Part of the voice reliability & onboarding plan (desktop repo:
docs/design-voice-reliability-and-onboarding.md§2b,docs/impl-plan-voice-reliability-and-onboarding.mdWS2 Steps 3–6 + amendments S1/S3/Y4-read-side). #2689 and #2690 are merged; this branch is rebuilt on currentmainand pairs with the desktop control-plane PR (the writer of the fields this PR reads).What changed, and why?
The S1 credential-source truth table, honored by every engine consumer.
managed-credentials.jsongains two policy readers:voicePreference(managed | byok, unset ⇒ legacy): under managed, only a non-quarantined managed entry satisfiesgemini-voice— env keys never silently substitute (resolvingnoneis a typed absence the desktop enable-voice flow routes on, instead of a wrong-source key that "works" until it doesn't). Under byok, managed slots are skipped entirely. Unset keeps today's chain byte-for-byte.quarantined(stamped by the desktop host at logout): every managed entry treated as absent, in every mode — a preserved managed key must never satisfy a resolution after logout.Consumers updated to agree exactly:
src/credential-resolver.ts+ its Python twinsrc/credential_resolver.py(which also gains the S3credentialGenerationread side — opaquecg1-<UUID>from the entry'sgenerationfield, orSUTANDO_VOICE_CREDENTIAL_GENERATIONfor env keys),startup-runtime.sh's managed-voice gate, andhealth-check.py's voice-config resolution. A canonicalcredentialSourceLabelmapper lands in the resolver per Step 3; the phase-1voice-agent-statemapper now delegates to it (zero wire change).Rebuilt commits:
56a77843(truth table + consumers + matrix),e6edb5cb(review follow-up: fixture rows for the no-managed-file and no-entries states),fc15f908(canonical doc update).What files / sections should reviewers look at first?
src/credential-resolver.ts/src/credential_resolver.py— the S1 gate (module docstring holds the truth table); the two are one-for-one twins.tests/voice-preference-consumers.test.sh+tests/fixtures/voice-preference-matrix.json— 20 rows × 4 consumers, all asserted to agree; this is the PR's spine.startup-runtime.shconfigure_startup_runtime(managed-preference branch bypasses the env-key check) andhealth-check.pyresolve_voice_health_config(same preference-first ordering).What user behavior or bug does this prove?
Two incident-class behaviors, shown in the evidence below: (1) after logout, the preserved managed token no longer resolves (the engine-side half of the desktop plan's revocation guarantee — the desktop supervisor's Z2 proof rules assume exactly this); (2) an explicit "use managed key" preference is never silently satisfied by a leftover env key.
Feature/documentation consistency
docs/design-credential-capability-resolver.mdis canonical for the resolution contract and is updated in this PR (fc15f908): new "Voice-source policy gates (S1)" subsection, schema example extended with the policy fields + per-entrygeneration,catalog.jsonlast_verifiedbumped.docs/README.mdnavigation unchanged (same doc set).Before/after evidence
Python-twin resolver, identical script (fixture file + env key), run at the branch's base (
ab6d75ee, worktree) and at HEAD:Base loads the preserved managed token after logout and silently substitutes an env key under a managed preference; HEAD does neither.
What tests did you run?
bash tests/voice-preference-consumers.test.sh— PASS: 20-row matrix × 4 consumers (TS resolver, Python resolver, shell gate/launcher, health check), all agreeing.npx --no-install tsx --test tests/credential-resolver.test.ts— 25/25;python3 tests/credential-resolver.test.py— PASS.python3 tests/health-check-voice-config.test.py— 25/25;bash tests/startup-voice-managed-gate.test.sh— both T1 and S1 summaries PASS.npx --no-install tsc --noEmit— clean;python3 tests/release-docs-audit.test.py— 5/5;git diff origin/main...HEAD | bash scripts/review-checks.sh— PASS.Stack-rebuild note: #2684's T1 fail-closed startup-test correction is already on
main. The sole cherry-pick conflict was resolved by retaining that merged two-line contract and adding #2691's S1 cases below it; no duplicate T1 change remains in this PR.Touching a live path (startup)?
The startup gate decides whether the voice agent launches — covered by the gate suite + matrix rows for the launcher decision. The packaged DMG evidence below exercised the same three child patches before the history-only rebuild;
range-diffconfirms the fixture/docs patches are identical and the implementation differs only by dropping its duplicate T1 test correction.Stack status
#2689 merged as
ffc0e0afand #2690 merged as4c79ccdf. This branch is rebuilt directly on that currentmain; its diff is now the three-commit S1 credential-policy layer only. The desktop control-plane PR remains the paired writer side for preference/quarantine/generation fields and revocation proof.Hardcoded-path check
git diff origin/main...HEAD | bash scripts/review-checks.sh→ PASS on rebuilt headfc15f908.Manual verification — packaged DMG pass (2026-08-06)
Local DMG with the content-equivalent pre-rebuild commits in the engine (integration merge: phase-2 transport
4bb2d9a9+ S1 branch72084dd; rebuilt equivalents are4c79ccdf+fc15f908); desktopvoice/phase3-control-plane(the writer of the fields this branch reads); cinnyvoice/phase3-enable-flow.Keyless truth-table row, engine-side, live (
sidecar boot summary):Clean config-gate, zero restarts, no crash loop — and the UI side surfaced the same truth as "Effective source: none" in the enable-voice flow's credential stage (the resolver's source readout reaching the user).
Key present (after the BYOK commit materialized):
voice-agent … state:"ready" … errors:[]— same boot path, opposite row.Quarantine honored with BYOK preference (S1 rows
byok × quarantined): after sign-out stampedquarantined: true(+sessionRevisionbump) withvoicePreference: "byok", voice kept working on the user's own env key — quarantine hides managed entries only, exactly the truth-table's byok column. The managed-preference logout variant (quarantined managed entry must config-gate voice) remains to be exercised once a managed key is minted; the row is pinned by the cross-consumer matrix tests meanwhile.Transcript evidence (CONTRIBUTING) — the resolved key drove a real session (
conversation.sqlite):🤖 Generated with Claude Code