Repository navigation
docs(credential): note the G8 resolver precedence at the three gemini-voice call sites - #2131
qingyun-wu wants to merge 3 commits into
Conversation
Coverage Gate✅ No Python changes — nothing to gate (bar: 95% on changed lines). |
|
Blocking for the managed-token pilot: Please make the startup preflight validate the resolved voice credential instead of the raw fallback env key, and add a regression test for "managed gemini-voice token present, no GEMINI_API_KEY/GEMINI_VOICE_API_KEY env, voice startup/key preflight succeeds". Local checks:
Reviewed by Qingyun's Codex. |
|
Coverage note (spec corrected, sha 859e779): re-audit shows |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Justification: [change-requested: bandaid_generalize, missing_before_after]
Daily triage sweep (gate 2) — conditions fired on this PR:
- bandaid_generalize (single-site patch where the class needs a root fix): Route sibling voice credential readers through the resolver too.
- missing_before_after (behavior claim needs a significant BEFORE/AFTER, not just unit tests): Add before/after output proving voice credential resolution behavior.
Addressing these (push fixes or reply with evidence) resets the clock; the re-review loop re-judges automatically after new commits.
|
Approval signal: I do not see blocking issues on this head. The resolver keeps the no-managed-source path equivalent to the existing Focused checks run:
Reviewed by Qingyun's Codex. |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Auto re-review (author pushed after the change-request):
- ask 1: ADDRESSED — Current diff adds
resolveCredentialand routessrc/voice-key.tsthrough `resolveCredential('gemi - ask 2: REMAINS — Current diff shows code changes only; no before/after output or artifact proving voice credential re
Remaining asks above — the change-request stays until they land.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Auto re-review (author pushed after the change-request):
- ask 1: REMAINS — Current diff adds
resolveCredentialand routesvoice-key.ts, but shows no sibling voice credenti - ask 2: REMAINS — Current diff includes code changes only; no significant BEFORE/AFTER output proving voice credential
Remaining asks above — the change-request stays until they land.
|
Daily reminder — commits landed after the 2026-07-16 change-request, but the auto re-review found asks still open (see the re-review comment above for exactly what remains). Pushing those closes this out; ~2 weeks without them routes to final-notice per the triage policy.
|
|
Before/after artifact (addressing the triage review's Reading: the BYO path is byte-identical across all four env states (no behavior change for existing installs — the seam is invisible until used), and the new G8 capability works: a registered managed source takes precedence and clearing it restores the BYO chain, per the managed-first precedence note in the module doc. On Harness source (reproducible)// BEFORE/AFTER artifact for sonichi#2131 (susan ask-2).
// BEFORE = the pre-PR chain (main's voice-key.ts): GEMINI_VOICE_API_KEY → GEMINI_API_KEY → ''
// AFTER = the PR's path: resolveCredential('gemini-voice', [VOICE, MAIN]) — must be byte-identical
// with no managed source, and managed-first once a source registers (the G8 drop-in).
import { resolveCredential, setManagedCredentialSource } from '../src/credential-resolver.js';
import { resolveVoiceGeminiKey } from '../src/voice-key.js';
const beforeChain = () => process.env.GEMINI_VOICE_API_KEY || process.env.GEMINI_API_KEY || '';
const cases: Array<[string, string|undefined, string|undefined]> = [
['both set (voice wins)', 'sk-voice-AAA', 'sk-main-BBB'],
['voice only', 'sk-voice-AAA', undefined ],
['main only', undefined, 'sk-main-BBB'],
['neither (empty)', undefined, undefined ],
];
let allMatch = true;
console.log('== BYO path: BEFORE (main env-chain) vs AFTER (resolver via voice-key.ts) ==');
for (const [name, v, m] of cases) {
if (v === undefined) delete process.env.GEMINI_VOICE_API_KEY; else process.env.GEMINI_VOICE_API_KEY = v;
if (m === undefined) delete process.env.GEMINI_API_KEY; else process.env.GEMINI_API_KEY = m;
const before = beforeChain();
const after = resolveVoiceGeminiKey();
const ok = before === after;
allMatch = allMatch && ok;
console.log(` ${ok ? 'IDENTICAL' : 'MISMATCH '} [${name}] before=${JSON.stringify(before)} after=${JSON.stringify(after)}`);
}
console.log(`BYO regression: ${allMatch ? 'PASS — byte-identical across all 4 env states' : 'FAIL'}`);
console.log('\n== NEW capability: managed source drop-in (G8) ==');
process.env.GEMINI_VOICE_API_KEY = 'sk-voice-AAA';
setManagedCredentialSource({ get: (cap) => cap === 'gemini-voice' ? 'managed-token-ZZZ' : null });
console.log(` managed registered → resolveVoiceGeminiKey() = ${JSON.stringify(resolveVoiceGeminiKey())} (managed wins)`);
setManagedCredentialSource(null);
console.log(` managed cleared → resolveVoiceGeminiKey() = ${JSON.stringify(resolveVoiceGeminiKey())} (BYO chain restored)`); |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Cold review (bot) — early feedback, not gating since it's a draft awaiting your "go pilot" + @rick's W4.
The seam design is sound and the one edge that usually bites this pattern is handled correctly:
- Empty/undefined managed → fall through, no blanking.
resolveCredentialusesif (managed) return managed(truthy), so a managed source that returns''orundefineddoesn't shadow a working BYO env key — it falls straight to the env chain. This is the classic bug in a managed-drop-in seam and you've covered it (and tested it: "empty/undefined managed falls through — no accidental blanking"). ✓ - Absent-by-default = byte-identical.
_managednull +_managed?.get()optional-chaining means every current consumer keeps exact behavior until a source is registered. The pilot (voiceApiKey()→resolveCredential('gemini-voice', [GEMINI_VOICE_API_KEY, GEMINI_API_KEY])) reproduces the old chain 1:1 when unmanaged. ✓ - Precedence isolated to one block — managed-first vs BYO-first really is a two-line swap, matching owner call #3. Good that the decision touches nothing else.
One thing to confirm when you extend to anthropic/cartesia/twilio: those consumers must route through resolveCredential with their env chain in the same order they read today, else the drop-in silently reorders precedence. The gemini-voice pilot gets this right; just a note for the fan-out PR.
No blockers from me. Reads ready to un-draft on your go.
|
Approval signal on current head Scope note: the existing external change-request about routing sibling voice credential readers through the resolver still exists until that reviewer accepts the pilot boundary or the fan-out lands. I am not treating that as a new correctness blocker for this scoped pilot head. Focused checks run:
Reviewed by Qingyun's Codex. |
4eb1124 to
0bb8102
Compare
|
Owner go received (2026-07-20) — implementation landed at 0bb8102, rebased on current main. What changed vs the draft shell: the pluggable in-process Verification: Follow-up (separate PR): migrate the remaining direct |
qingyun-wu
changed the title
|
Approval signal for head I do not see a remaining code-level blocker for this scoped G8 voice credential pilot. The new file-backed managed credential contract is capability-scoped, absent-by-default, and fail-open: unreadable, malformed, wrong-shape, or empty managed credentials fall through to the existing BYO env chain instead of throwing or blanking a working key. With no managed file, Scope/security notes:
Focused checks in the isolated
Remote CI: tsc/tests, diff coverage, smoke, CLA, and lint guards are green. The remaining red Reviewed by Qingyun's Codex. |
|
Addressing both review findings with evidence (air core, at integration tip f221bd4 which contains this PR + its stack): 1. bandaid_generalize — resolved by the stack. The sibling voice credential readers are routed through the resolver in #2197 (stacked directly on this branch): 2. missing_before_after — output below, produced by running the resolver at Reading: scenario 1 proves BYO parity (no behavior change for every existing install); 2–3 prove the managed-tier drop-in that motivates the PR, including the fresh-provisioning case the legacy chain simply fails; 4 shows failure is now observable ( |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Auto re-review (author pushed after the change-request):
- ask 1: REMAINS — Current diff adds
credential-resolver.ts, but does not show sibling voice credential readers being - ask 2: REMAINS — Current diff contains no BEFORE/AFTER output demonstrating voice credential resolution behavior.
Remaining asks above — the change-request stays until they land.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Auto re-review (author pushed after the change-request):
- ask 1: REMAINS — Current diff shows a new resolver, but the provided diff only evidences
src/voice-key.tsbeing tou - ask 2: REMAINS — Current diff includes code changes only; no before/after output or captured behavior evidence provin
Remaining asks above — the change-request stays until they land.
|
Daily reminder — commits landed after the 2026-07-16 change-request, but the auto re-review found asks still open (see the re-review comment above for exactly what remains). Pushing those closes this out; ~2 weeks without them routes to final-notice per the triage policy.
|
|
On the TypeScript, could route through the new resolver:
( A scoping constraint worth deciding explicitly rather than discovering later: four consumers are Python and cannot use a TypeScript resolver at all —
So "route every sibling through the resolver" can't be satisfied as stated. Either the capability contract needs a Python counterpart reading the same I'd argue for stating the boundary in this PR rather than expanding it: the managed-tier file is the actual contract, and a Python reader of the same file is a separate change with its own tests. But it should be a written decision, since right now the resolver silently covers some consumers and not others. Happy to produce the before/after for the resolution behavior too — this host has the managed tier populated, so the tier-walk is exercisable. Say the word if that's still wanted after the scope question settles. |
Test suite executed — 9/9 pass; and one docs fixRan the suite (bundled node v22.17.0,
|
Counter-evidence on
|
|
Conflicts resolved (merge of |
|
Blocking finding on head 14f4b77:
Validation notes:
Reviewed by Qingyun's Personal Codex. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested on current head 14f4b7764fb710c0d8a767e844fb640c795b794b (formal self-request-changes is unavailable).
- [P1] The merge resolution duplicated the
resolveCredentialimport in bothsrc/browser-tools.ts(lines 7 and 14) andsrc/recording-tools.ts(lines 7 and 14). The focused resolver suite still passes 9/9, butnpx tsc --noEmitnow fails with fourTS2300: Duplicate identifier 'resolveCredential'errors, so this head cannot pass the required typecheck. Remove the second import in each file and rerun the typecheck.
Review P1: git's auto-merge unioned the import both branches had added at different line positions, leaving a duplicate identifier in browser-tools.ts and recording-tools.ts (TS2300 x4). One import per file now.
|
Duplicate imports removed in the new head — git's auto-merge had unioned the |
|
@cla-assistant check |
|
Reviewed head No blocking findings. The duplicate Focused checks in the isolated
Approval signal from me. Reviewed by Qingyun's Personal Codex. |
|
The
The bad-merge duplicate is gone, so the four — qingyun-001 |
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Re-verified the delta since my last approval (blob-diff: only browser-tools.ts/recording-tools.ts touched) — it's lint-only: import reordering + dropping an unused remaining var and a now-unneeded let→const. No behavior change. Still good.
|
Addressing both gate-2 asks on the current head ( ask 1 — bandaid_generalize (sibling voice credential readers through the resolver). The current head routes both remaining voice-side sibling readers — ask 2 — missing_before_after (behavior proof, real output). Methodology: BEFORE is the verbatim pre-resolver line from S1 is the new managed-tier capability; S2–S4 show resolution is byte-for-byte identical to the legacy env chain whenever no managed file exists (including the empty failure mode), with |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Evidence-only re-review on current head 17bff23e40b67de8c40d2ad40eab184c0d8f416c.
The new S1–S4 before/after transcript matches the implementation and committed resolver tests: managed credentials win when present, GEMINI_VOICE_API_KEY remains the first BYO choice, GEMINI_API_KEY remains the fallback, and the empty case remains empty. The current effective diff against qingyun-dev is comment-only at the already-routed browser/recording call sites; the exact-head typecheck and 12 focused resolver/voice-key tests from the prior pass remain applicable, and current checks are green.
No new blockers; the existing approval signal remains current.
Reviewed by Qingyun's Personal Codex.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Blocking on stack metadata and merge order at current head 17bff23e.
This PR is now marked ready, but its body still begins DRAFT, targets qingyun-dev rather than main, and does not identify the parent PR or intended merge order. The effective child-only delta is tiny, while the cumulative stack is much larger; merging it safely requires an explicit parent and the post-parent rebase/check plan required by CONTRIBUTING.md.
Please update the body with:
- the parent PR/branch and intended merge order;
- whether this PR is meant to merge into
qingyun-devor ultimately retargetmain; - the checks to rerun after the parent lands/rebase completes;
- removal of the stale
DRAFTlabel in the body if it is genuinely ready.
The current child-only code delta has no new blocking code finding; this is a merge-readiness/documentation gate.
Reviewed by John’s Codex.
|
@john-the-dev metadata addressed at head |
|
@john-the-dev — addressed the merge-readiness/documentation gate (no code change; the child-only delta is unchanged at
CI is green and merge state is CLEAN. Ready for re-review. |
Addressed: the body now names #2197 as parent, gives the merge order, retarget/rebase step, and rerun plan, and no longer presents the PR as draft.
|
Rechecked current head 17bff23 after the body update. My stack-metadata blocker is resolved and I dismissed that stale review. This remains intentionally blocked on #2197 landing first, followed by retarget/rebase to main and a fresh full check run; if the retarget leaves no meaningful child-only diff, close this as superseded instead of merging an empty layer. |
|
Corrected the stack metadata to match the file reality: the resolver + tests migrated into #2197 during the merge-resolution churn, so #2197 is the standalone main-lander and this PR is a 3-line |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Reviewed at head 17bff23e. The effective diff is now 3 comment lines into qingyun-dev (base is 47 ahead / 6 behind main): the resolver this PR's title advertises already lives on the base branch, and #2197 (base main) ships the identical content to main — this PR delivers no code anywhere.
That matches the corrected PR body and @john-the-dev's dismissal condition verbatim ("if the retarget leaves no meaningful child-only diff, close this as superseded instead of merging an empty layer"). Verdict: do not merge — close as superseded once #2197 lands on main.
On the owner's active CHANGES_REQUESTED: both asks appear substantively addressed — sibling readers routed via commit 708a6e79 (browser-tools + recording-tools through resolveCredential('gemini-voice')), and the 7/25 S1–S4 before/after transcript covers the behavior evidence (though it lives in comments, not the body). That state remains hers to clear, as is the close decision.
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Comment rather than a second CHANGES_REQUESTED, since the PR is already blocked and stacking a duplicate verdict adds noise rather than information.
The three added comment lines are accurate — resolveCredential('gemini-voice') is already the call at all three sites on the base, so documenting the managed → GEMINI_VOICE_API_KEY → GEMINI_API_KEY precedence describes real behavior rather than aspirational behavior. No objection to the content.
The problem is scope versus title. This is titled feat(credential): capability resolver — managed-tier file contract + voice pilot (G8) and the entire diff is three comment lines in two files. There is no resolver, no file contract, and no pilot in it. Two concrete costs:
- A reviewer opens it expecting the G8 resolver and has to reconstruct that the implementation landed elsewhere — I did exactly that before checking the diff stat.
feat(...)in the subject means this shows up in changelog/release tooling as a feature that ships nothing.
Either retitle to docs(credential): note the G8 resolver precedence at the three gemini-voice call sites, or fold these lines into whichever PR introduces the resolver so the comments land with the code they describe. The latter is better — a comment explaining a precedence chain is much easier to review next to the chain.
Per CONTRIBUTING's single-concern rule this is already a clean single concern; it's only the label that's wrong.
|
|
Author position, so this stops looking like a PR waiting on work: @liususan091219 is right, and I verified it rather than taking it. Close as superseded once #2197 lands. I checked the "delivers no code" claim myself against this PR's own base: All three additions are comment lines ( What I am not claiming: that the work is gone. The resolver content ships to Two things I am deliberately leaving alone:
Happy to close it the moment #2197 is on |
|
Local verification — CI on this base cannot typecheck the change. Base is The risk that carries is a contract break invisible to every green tick here, so I verified the contract statically on head So the module exists on this branch, the imported symbol is exported, Stating the limit rather than overclaiming: this is a static contract check, not a full typecheck. A real |
qingyun-wu
changed the title
|
Closing as superseded by #2197, per the record: the resolver + tests migrated into #2197 during the July merge-resolution churn (see the 07-26 stack-metadata correction), the reviewer's 07-26 'do not merge' stands, the 07-30 shared-login comment already recommended close-as-superseded, and this PR's base ( |
Stack + merge order (canonical — corrected 2026-07-26, resolves John's gate)
mainand lands first. During the merge-resolution churn the resolver + its tests migrated INTO refactor(credentials): route remaining TS GEMINI_* readers through the G8 resolver #2197 (credential-resolver.ts+83,tests/credential-resolver.test.ts+105), so refactor(credentials): route remaining TS GEMINI_* readers through the G8 resolver #2197 is now self-contained — it is not stacked on this PR, and this PR is not its parent. (Earlier metadata on both PRs named the other as parent; that's the contradiction being fixed.)qingyun-devintegration branch (browser-tools.ts+2,recording-tools.ts+1) that refactor(credentials): route remaining TS GEMINI_* readers through the G8 resolver #2197 fully subsumes. It no longer carries the resolver shell (that's in refactor(credentials): route remaining TS GEMINI_* readers through the G8 resolver #2197).main— owner's call, since theqingyun-devbase + review history are deliberate. If instead qingyun-dev should carry these lines independently, it picks them up automatically when it next integratesmainpost-refactor(credentials): route remaining TS GEMINI_* readers through the G8 resolver #2197.the G8 credential-resolver shell, pilot = Gemini voice. Non-breaking; ready to un-draft on the owner's "go pilot." Spec:
roadmap/spec-g8-credential-resolver.md.What
resolveCredential(capability, envChain)— a service resolves the capability it needs, not a hardcoded env key, so a BYO key and an AgentUniverse-managed token are interchangeable sources behind one call (the "managed drop-in").The managed source is absent by default (
getManagedCredentialSource() → null), so every consumer keeps byte-identical behavior — the resolver falls straight through to the BYO env chain. A composition root registers a real source only once a managed session is active (lands with the AgentUniverse account, W4 — blocked on @rick, same as W1 prod cutover).Pilot (proves drop-in, zero consumer change)
voice-key.tsvoiceApiKey()now resolves'gemini-voice'via[GEMINI_VOICE_API_KEY, GEMINI_API_KEY]. With no managed source → identical to the old chain (verified: returns the VOICE key). With a managed token present → the same call returns it. No voice-agent code changes between the two.Owner decisions this encodes (adjustable)
voice-key.tschain is the natural seam.resolveCredential()so BYO-first is a one-line flip.ManagedCredentialSourceimpl, not this shell.Tests
tests/credential-resolver.test.ts— no-managed→env chain; managed wins; empty/undefined managed falls through (no accidental blanking); register/clear. All green.Un-draft + extend to anthropic/cartesia/twilio on your go.