feat: add Zero provider runtime resolver#12
Conversation
|
@Vasanthdev2004 @anandh8x please review this PR when you get a chance. This is the next Gnanam M1 slice after the merged model registry: Zero provider runtime resolver only, with headless wiring and tests. |
anandh8x
left a comment
There was a problem hiding this comment.
Code Review: PR #12 — feat: add Zero provider runtime resolver
Author: @gnanam1990 → feat/zero-provider-runtime → main
Scope: 11 files, +384/-15. Adds src/zero-provider-runtime/ (types + resolver + barrel), defensive parseProviderProfileKind parsing, provider metadata on saved profiles, and rewires headless execution through the resolver while preserving the current OpenAI streaming path.
Verdict: Clean, well-scoped slice that does exactly what the PR description says — and what the previous Gnanam-owned slice (#9) set up. Two real correctness bugs and a UX regression in the env-var fallback path are blockers; the rest are important follow-ups. The architectural concern about three divergent ZeroConfig types is the one I'd push hardest on before this lands.
Blocking
1. Official OpenAI baseURL is not normalized to /v1 — runtime 404
src/zero-provider-runtime/resolver.ts:22-25 declares OFFICIAL_OPENAI_BASE_URLS to include both https://api.openai.com/v1 and https://api.openai.com. src/zero-provider-runtime/resolver.ts:57 and :72 return the user's baseURL verbatim:
baseURL: baseURL ?? DEFAULT_BASE_URL[provider],A user who configures baseURL: 'https://api.openai.com' (no /v1 suffix — a common shorthand) will match the "official" set, the resolver picks provider: 'openai', the runtime returns baseURL: 'https://api.openai.com', and OpenAIProvider (src/providers/openai.ts:15-18) constructs a client that points at the wrong path. Every request 404s.
Ask: When OFFICIAL_OPENAI_BASE_URLS matches, return DEFAULT_BASE_URL[provider] (which has the /v1 suffix) instead of the user-provided URL. Or, more surgically, append /v1 only when the input matched https://api.openai.com without a path. Add a regression test that uses baseURL: 'https://api.openai.com' (no suffix) and asserts the resolved runtime's baseURL ends with /v1.
2. OPENAI_API_KEY alone no longer triggers the env-var fallback (silent model downgrade)
src/config/manager.ts:117:
if (process.env.OPENAI_BASE_URL || process.env.OPENAI_MODEL) {This regression is a UX blocker. The most common local dev setup is only OPENAI_API_KEY set. With this condition, that user gets null from getEffectiveProviderConfig, falls through to src/config/provider.ts:40-57, and ends up silently using the hardcoded default model 'gpt-4o' from src/config/manager.ts:122 — they didn't ask for that model.
If the user has no key at all, the previous code path produced a clear "No LLM provider configured" error. Now that path is only reached when no key is set, so most users won't see it.
Ask: Trigger the env fallback on OPENAI_API_KEY too: if (process.env.OPENAI_API_KEY || process.env.OPENAI_BASE_URL || process.env.OPENAI_MODEL). And while there, drop the gpt-4o hardcode — look up the default model for the resolved provider from the registry (ZERO_DEFAULT_MODEL_ID is gpt-4.1, per PR #9).
3. src/config/types.ts and src/config/loader.ts define two different ZeroConfig types
The ZeroConfig in src/config/types.ts:32 (used by manager.ts and provider.ts) is the old M0 shape:
export interface ZeroConfig { activeProvider?: string; providers: ProviderProfile[]; }The ZeroConfig in src/config/loader.ts:38 (used by loadConfig and the new tests) is the new Zod-inferred shape that adds maxTurns, planMode, debug. These are two different types with different fields.
This means:
ConfigManagerandloadProviderConfigoperate on a config that has nomaxTurns/planMode/debugfields.loadConfig(and any TUI code that wants runtime config) operates on a config that does.- A user who sets
maxTurns: 5in their config file and expects the agent loop to honor it gets a silent miss, becauserunAgentreads itsmaxTurnsdefault fromAgentOptions(PR #11), not from the config.
Ask: Pick one home for ZeroConfig. Options:
- (a) Delete
ZeroConfigfromsrc/config/types.tsand havemanager.tsimport fromloader.ts(with the Zod-derived type).PROVIDER_PROFILE_KINDSandparseProviderProfileKindstay intypes.tssince they're not Zod-derived. - (b) Move the Zod schema into
types.ts, deriveZeroConfigfrom it there, and haveloader.tsre-export the type.
Either way, the two types should converge before this slice lands, because every downstream slice (/config UI, runAgent options) is going to have to pick one.
Important
4. Bun.$\\\\${providerCommand}\`.quiet()` has no timeout and no error handling
src/config/provider.ts:22-31:
const { stdout } = await Bun.$`${providerCommand}`.quiet();
const parsed = JSON.parse(stdout.toString());
return { provider: ..., apiKey: parsed.api_key, baseURL: parsed.base_url || ..., model: parsed.model };- No timeout. A misbehaving provider command hangs the CLI indefinitely.
JSON.parsecan throw — no try/catch, so the user gets"Unexpected token ..."instead of"provider command returned invalid JSON".parsed.modelis treated as required, but if the command returnsparsed.model_idor no model key at all, this silently returnsmodel: undefined, which then explodes insideresolveZeroProviderRuntimeas"Missing Zero provider model".
Ask: Wrap in try/catch with clear messages; validate the output shape (must have model); add a 5s timeout. Also, the parsed.provider || parsed.provider_kind and parsed.api_key / parsed.base_url snake_case mapping is undocumented — a JSDoc on loadProviderConfig listing the expected command output shape would help external command authors.
5. cli/index.ts re-implements source detection that loadProviderConfig should own
src/cli/index.ts:9-13:
const source = activeProfile
? 'profile'
: process.env.ZERO_PROVIDER_COMMAND
? 'provider-command'
: 'environment';This duplicates the priority logic that's already in loadProviderConfig (src/config/provider.ts:19-57). If loadProviderConfig's priority order changes (a future slice adds cliOverrides as priority 0, say), this detection becomes out of sync — and the runtime's source field would lie to the user/logs.
Ask: Have loadProviderConfig return the resolved source as part of its ProviderConfig result, and have cli/index.ts read it from there. The runtime should never have to re-derive the source.
6. runHeadless doesn't catch the "adapter not implemented yet" error
src/cli/index.ts:22 calls createZeroProvider(runtime) without a try/catch. For an Anthropic or Gemini profile (which the resolver accepts but createZeroProvider rejects at src/zero-provider-runtime/resolver.ts:93-96), the user sees a stack trace with the banner and runtime info already printed — and then a crash. Should be a friendly CLI error.
Ask: Wrap in try/catch. On the not-implemented error, print the runtime info (already logged) plus a one-line hint like "Anthropic adapter is not yet implemented. Set provider: 'openai-compatible' or use an OpenAI model.", then process.exit(1) cleanly.
7. providerInstance? field is declared but never populated
src/zero-provider-runtime/types.ts:36 declares providerInstance?: Provider on ZeroResolvedProviderRuntime. src/zero-provider-runtime/resolver.ts never sets it. createZeroProviderFromInput constructs the provider at the call site and returns it alongside the runtime.
The field is dead. Either remove it from the type, or have resolveZeroProviderRuntime also construct the provider (and have createZeroProvider become redundant, which collapses the resolver+factory split — probably not the intent).
Ask: Remove the field. If a future slice needs the provider cached on the runtime, add it then.
8. loadConfig Zod validation is decoupled from the actual config path
src/config/loader.ts:24:
baseURL: z.string().url(),The new schema rejects invalid URLs at parse time. But the actual config loading path used by runHeadless is loadProviderConfig → configManager.getEffectiveProviderConfig → provider.ts, which doesn't go through loader.ts at all. So a malformed baseURL like 'not a url' in the user's ~/.config/zero/config.json is happily accepted by the runtime path, then the resolver's OFFICIAL_OPENAI_BASE_URLS.has(baseURL) returns false (because the URL is malformed), and the runtime falls into the unknown-model branch. Silent misroute.
Same issue with maxTurns: z.number().int().min(1).max(100) (line 33) — validated by loadConfig, ignored by the manager path.
Ask: Either (a) route configManager.getEffectiveProviderConfig through the Zod schema for validation, or (b) document that the loader.ts path is the "validated" path and the manager path is "trusted local config." Option (a) is the safer default.
9. readConfigFile swallows all errors silently
src/config/loader.ts:71-80:
function readConfigFile(path: string): ZeroConfig {
if (!existsSync(path)) return {};
try {
const text = readFileSync(path, 'utf-8');
const parsed = JSON.parse(text);
return ZeroConfigSchema.partial().parse(parsed);
} catch {
return {};
}
}If the user's config.json is malformed JSON, the file is silently treated as empty. Same for a Zod validation failure. The user has no idea their config is being ignored — they just see defaults. configManager.readConfig (src/config/manager.ts:33-35) has the same problem.
Ask: Distinguish "file doesn't exist" (silent, since that's the normal first-run case) from "file is invalid" (warn to stderr with the path and a one-line hint). Either use console.warn or pass a logger in via LoadConfigOptions.
10. configManager singleton performs I/O at import time
src/config/manager.ts:131:
export const configManager = new ConfigManager();The constructor calls readConfig() which reads ~/.config/zero/config.json. So import { configManager } from './config/manager' does filesystem I/O. The CLI binary is fine with this, but tests that import the CLI module (src/cli/index.ts) will read the user's real config — including any saved active provider.
Ask: Make ConfigManager instantiable and have cli/index.ts call new ConfigManager() lazily, or expose a factory function.
11. cli/index.ts is not tested
The new wiring of loadProviderConfig + resolveZeroProviderRuntime + createZeroProvider + runAgent in src/cli/index.ts:6-52 is the central surface for headless execution. It has no test coverage. At minimum, a test that:
- Stubs
loadProviderConfigto return a profile, - Stubs
runAgentto return a known string, - Asserts the banner +
[zero] Provider/Model/Base URLlog lines are emitted,
would lock the wiring. Without it, any refactor that breaks the headless flow lands silently.
12. parseProviderProfileKind rejects valid case variants
src/config/types.ts:10-21:
if (typeof value === 'string' && (PROVIDER_PROFILE_KINDS as readonly string[]).includes(value)) {
return value as ProviderProfileKind;
}'OPENAI', 'Openai', 'Google' are all rejected. A user with ZERO_PROVIDER=OpenAI in their shell gets the "Unknown Zero provider kind" error. Should lowercase the input before the check, like the registry's normalizeModelKey does.
Suggestions
13. ZeroProviderRuntimeKind and ProviderProfileKind are duplicate enums
src/zero-provider-runtime/types.ts:8: export type ZeroProviderRuntimeKind = ZeroModelProvider | 'openai-compatible';
src/config/types.ts:1-6: PROVIDER_PROFILE_KINDS is the same enum.
Two definitions of the same thing. parseProviderProfileKind returns the same union. Pick one and have the other derive from it.
14. model: 'gpt-4o' hardcoded as the env default
src/config/manager.ts:122 defaults to 'gpt-4o' when no OPENAI_MODEL is set. This is reasonable for OpenAI, but the env-fallback branch is supposed to be provider-agnostic — gpt-4o is meaningless if ZERO_PROVIDER=anthropic. Either default to a provider-aware model (look up the provider's default model) or document that the env-fallback is OpenAI-only by default. Tied to #2 above.
15. mergeLayers partial override behavior is untested
src/config/loader.ts:99-118 does field-level merge of providers with the same name ({...existing, ...profile}). The test at tests/config-loader.test.ts:22-28 only verifies the "empty later layer preserves earlier" case. Add a test that has user config { name: 'p', baseURL: 'A' } and project config { name: 'p', model: 'm' } and asserts the merged result has both baseURL: 'A' and model: 'm'.
16. removeProvider silently switches to another provider
src/config/manager.ts:88-90:
if (this.config.activeProvider === name) {
this.config.activeProvider = this.config.providers[0]?.name;
}If a user removes their active provider and others remain, this auto-switches to providers[0]. The user might not expect that. Either set to undefined (force explicit /provider switch) or document the auto-switch.
17. Hardcoded capabilities for unknown models
For the OpenAI-compatible case, the capabilities at src/zero-provider-runtime/resolver.ts:74 are hardcoded. The cli/index.ts:35 runtime-info log uses runtime.modelId ?? runtime.requestedModel for the displayed model, which handles the undefined case. A JSDoc on the runtime field clarifying that unknown-model capabilities are assumed would be useful.
18. writeFileSync is not atomic
src/config/manager.ts:38-41:
writeFileSync(CONFIG_PATH, JSON.stringify(config, null, 2), 'utf-8');A crash mid-write corrupts the config. Standard fix: write to CONFIG_PATH + '.tmp', then fs.renameSync. Low risk for a personal config file, but cheap to do.
19. envLayer silently drops malformed ZERO_MAX_TURNS
src/config/loader.ts:85-87:
const n = parseInt(env.ZERO_MAX_TURNS, 10);
if (!Number.isNaN(n)) layer.maxTurns = n;ZERO_MAX_TURNS=abc is silently ignored. Should warn.
20. ZeroProviderRuntimeInput.requestedModel semantics
requestedModel is the user-supplied string (case-preserved), modelId is the registry-canonical id (case-normalized). Worth a JSDoc on the input type clarifying this so downstream consumers don't compare requestedModel === 'GPT-4.1' and fail.
21. Cross-PR conflict: package.json typecheck script
PR #10 (feat/m0-ci-scripts) defines "typecheck": "tsc --noEmit" and PR #11 (codex/p0-foundation-tools) defines "typecheck": "bunx tsc --noEmit". This PR doesn't touch package.json (no typecheck line in the diff), so when #10 and #11 merge, the conflict surfaces. Worth coordinating merge order or aligning scripts.
Questions for the author
- The
zero-provider-runtimenaming — was "provider-factory" rejected because of a name conflict with the closed PR #8, or is there a more substantive reason? A one-line note in the PR description would help reviewers outside the team understand the design choice. - When Anthropic and Gemini streaming adapters land (per the PRD: "next Gnanam PRD slices"), is there a plan to remove the "adapter not implemented yet" throw in
createZeroProviderand replace it with a registry lookup? Or will the throw stay forever as a guard? - The "API key required for
openaibut notopenai-compatible" asymmetry (resolver.ts:82-90) — is the assumption that OpenAI-compatible gateways have their own auth model (or no auth), while the official OpenAI requires a real key? Worth a JSDoc oncreateZeroProviderclarifying. - The hardcoded
model: 'gpt-4o'default inmanager.ts:122— does this become an explicit dependency on the model registry in a follow-up? Right nowmanager.tsknows the string'gpt-4o', but the resolver would be the right place to look up the default model by provider. - The "PR #11 is under requested changes" note in the PR body — does this slice have any dependency on PR #11's tool-call delta fix, or is the OpenAI provider usage here independent of that path?
Validation
- The PR body claims
73 pass, 0 fail. I didn't run the tests (no repo access), but the new test file attests/zero-provider-runtime.test.ts:96-104covers the official-OpenAI-needs-key case. The OpenAI-compatible-no-key case is not covered (resolver.ts:87accepts thezero-openai-compatiblesentinel without testing it). - I hand-traced the resolver for
model: 'gpt-4.1', no explicit provider,baseURL: 'https://api.openai.com':registryModel = gpt-4.1resolveKnownModelProvider('openai', undefined, 'https://api.openai.com')→'openai'(matchesOFFICIAL_OPENAI_BASE_URLS)- Runtime returns
baseURL: 'https://api.openai.com' OpenAIProvideris constructed withbaseURL: 'https://api.openai.com'- First API call → 404.
- This is the bug in #1.
- I hand-traced
OPENAI_API_KEY=sk-..., no other env:manager.ts:117returnsnull(noOPENAI_BASE_URLand noOPENAI_MODEL)provider.ts:34-37fromProfileisnullprovider.ts:40-57builds env config:envApiKey='sk-...', envBaseURL='https://api.openai.com/v1', envModel='gpt-4o'- The if-check passes (apiKey is set)
- Returns
{ provider: undefined, apiKey: 'sk-...', baseURL: 'https://api.openai.com/v1', model: 'gpt-4o' } - The user gets
gpt-4owhether they wanted it or not. Silent model downgrade. - This is the bug in #2.
Recommendation
Request changes. The two real correctness bugs (#1 baseURL normalization, #2 OPENAI_API_KEY handling) need to land before this slice. The two-ZeroConfig-types architectural concern (#3) is the one I'd push hardest on — it sets a pattern that compounds with every downstream slice. The cli/index.ts rewiring deserves a test (#11). Once those are addressed, the rest are nits and follow-ups that can land in a quick follow-up PR.
| 'openai-compatible': 'https://api.openai.com/v1', | ||
| } as const satisfies Record<ZeroProviderRuntimeKind, string>; | ||
|
|
||
| const OFFICIAL_OPENAI_BASE_URLS = new Set([ |
There was a problem hiding this comment.
Blocking — official OpenAI baseURL not normalized to /v1.
OFFICIAL_OPENAI_BASE_URLS includes both https://api.openai.com/v1 and https://api.openai.com, but lines 57 and 72 return the user's baseURL verbatim:
baseURL: baseURL ?? DEFAULT_BASE_URL[provider],A user who configures baseURL: 'https://api.openai.com' (no /v1 — a common shorthand) will match this set, the resolver picks provider: 'openai', the runtime returns baseURL: 'https://api.openai.com', and OpenAIProvider (src/providers/openai.ts:15-18) constructs a client pointing at the wrong path. Every request 404s.
Ask: When OFFICIAL_OPENAI_BASE_URLS matches, return DEFAULT_BASE_URL[provider] (which has the /v1) instead of the user-provided URL. Or, more surgically, append /v1 only when the input matched https://api.openai.com without a path. Add a regression test that uses baseURL: 'https://api.openai.com' (no suffix) and asserts the resolved runtime's baseURL ends with /v1.
| @@ -110,6 +116,7 @@ export class ConfigManager { | |||
| // Fallback to env vars | |||
| if (process.env.OPENAI_BASE_URL || process.env.OPENAI_MODEL) { | |||
There was a problem hiding this comment.
Blocking — OPENAI_API_KEY alone no longer triggers the env-var fallback.
if (process.env.OPENAI_BASE_URL || process.env.OPENAI_MODEL) {The most common local dev setup is only OPENAI_API_KEY set. With this condition, that user gets null from getEffectiveProviderConfig, falls through provider.ts:34-37, and ends up at the "No LLM provider configured" error at provider.ts:46-50 even though they have a valid key.
Even when OPENAI_API_KEY is set and OPENAI_BASE_URL is the default fallback (https://api.openai.com/v1), the user is silently given the hardcoded default model gpt-4o from line 122 — they didn't ask for that model.
Ask: Trigger the env fallback on OPENAI_API_KEY too: if (process.env.OPENAI_API_KEY || process.env.OPENAI_BASE_URL || process.env.OPENAI_MODEL). And while there, drop the gpt-4o hardcode — look up the default model for the resolved provider from the registry (ZERO_DEFAULT_MODEL_ID is gpt-4.1, per PR #9).
| @@ -1,5 +1,28 @@ | |||
| export const PROVIDER_PROFILE_KINDS = [ | |||
There was a problem hiding this comment.
Blocking — two different ZeroConfig types in the codebase.
src/config/types.ts:32 (used by manager.ts and provider.ts):
export interface ZeroConfig { activeProvider?: string; providers: ProviderProfile[]; }src/config/loader.ts:38 (used by loadConfig and the new tests):
export type ZeroConfig = z.infer<typeof ZeroConfigSchema>;…which adds maxTurns, planMode, debug.
The two types have different fields. A user who sets maxTurns: 5 in their config file and expects the agent loop to honor it gets a silent miss, because runAgent reads its maxTurns default from AgentOptions (PR #11), not from the config.
Ask: Pick one home. Either (a) delete ZeroConfig from types.ts and have manager.ts import from loader.ts (with the Zod-derived type) — keeping PROVIDER_PROFILE_KINDS and parseProviderProfileKind in types.ts since they're not Zod-derived — or (b) move the Zod schema into types.ts and derive ZeroConfig from it there. Either way, the two types should converge before this slice lands.
| @@ -21,6 +23,7 @@ export async function loadProviderConfig(): Promise<ProviderConfig> { | |||
| const parsed = JSON.parse(stdout.toString()); | |||
There was a problem hiding this comment.
Important — Bun.$\\\${providerCommand}\.quiet() has no timeout and no error handling.
const { stdout } = await Bun.$`${providerCommand}`.quiet();
const parsed = JSON.parse(stdout.toString());
return { provider: ..., apiKey: parsed.api_key, baseURL: parsed.base_url || ..., model: parsed.model };- No timeout. A misbehaving provider command hangs the CLI indefinitely.
JSON.parsecan throw — no try/catch, so the user gets"Unexpected token ..."instead of"provider command returned invalid JSON".parsed.modelis treated as required, but if the command returnsparsed.model_idor nomodelkey, this silently returnsmodel: undefined, which then explodes insideresolveZeroProviderRuntimeas"Missing Zero provider model".
Ask: Wrap in try/catch with clear messages; validate the output shape (must have model); add a 5s timeout. Also, the parsed.provider || parsed.provider_kind and parsed.api_key / parsed.base_url snake_case mapping is undocumented — a JSDoc on loadProviderConfig listing the expected command output shape would help external command authors.
| : process.env.ZERO_PROVIDER_COMMAND | ||
| ? 'provider-command' | ||
| : 'environment'; | ||
| const runtime = resolveZeroProviderRuntime({ |
There was a problem hiding this comment.
Important — source detection is re-implemented here.
const source = activeProfile
? 'profile'
: process.env.ZERO_PROVIDER_COMMAND
? 'provider-command'
: 'environment';This duplicates the priority logic that's already in loadProviderConfig (src/config/provider.ts:19-57). If loadProviderConfig's priority order changes (a future slice adds cliOverrides as priority 0, say), this detection becomes out of sync — and the runtime's source field would lie to the user/logs.
Ask: Have loadProviderConfig return the resolved source as part of its ProviderConfig return type, and have cli/index.ts read it from there. The runtime should never have to re-derive the source.
| const baseURL = normalizeOptionalUrl(input.baseURL); | ||
| const registryModel = getZeroModel(requestedModel); | ||
|
|
||
| if (registryModel) { |
There was a problem hiding this comment.
Important — providerInstance? field is declared but never populated.
src/zero-provider-runtime/types.ts:36 declares providerInstance?: Provider on ZeroResolvedProviderRuntime. resolveZeroProviderRuntime never sets it. createZeroProviderFromInput constructs the provider at the call site and returns it alongside the runtime.
The field is dead. Either remove it from the type, or have resolveZeroProviderRuntime also construct the provider (and have createZeroProvider become a thin wrapper). The current contract lies about what's available on the runtime object.
Ask: Remove the field. If a future slice needs the provider cached on the runtime, add it then.
| apiModel: requestedModel, | ||
| baseURL: baseURL ?? DEFAULT_BASE_URL[provider], | ||
| apiKey: input.apiKey, | ||
| capabilities: ['chat', 'streaming', 'tool-calling', 'system-prompt'], |
There was a problem hiding this comment.
Important — hardcoded capabilities for unknown models are wrong for some gateways.
capabilities: ['chat', 'streaming', 'tool-calling', 'system-prompt'],For an Anthropic-compatible or Gemini-compatible gateway running in OpenAI passthrough mode (LiteLLM, etc.), the gateway may not support tool calling. The agent loop using these capabilities to decide which tools to send would be wrong. There's no way to know what the gateway supports from the runtime alone.
Ask: Either gate capability-driven behavior on runtime.model !== undefined (only trust capabilities for registry-known models), or add a JSDoc on the field that says "For unknown models, capabilities are assumed minimal and may not reflect gateway support. Downstream code should not rely on this list as ground truth."
| if (explicitProvider) return explicitProvider; | ||
| if (baseURL && !OFFICIAL_OPENAI_BASE_URLS.has(baseURL)) return 'openai-compatible'; | ||
|
|
||
| throw new Error( |
There was a problem hiding this comment.
Suggestion — unknown model error message could list valid providers.
throw new Error(
`Unknown Zero model for ${source} provider runtime. ` +
`Use a model from the Zero model registry or set provider: "openai-compatible" for custom OpenAI-compatible gateways.`
);Helpful, but doesn't list the valid provider values. A user who got here is probably typing into a config file and would benefit from a one-line "Valid providers: openai, anthropic, google, openai-compatible." appended to the message.
| export const ProviderProfileSchema = z.object({ | ||
| name: z.string().min(1), | ||
| provider: z.enum(PROVIDER_PROFILE_KINDS).optional(), | ||
| baseURL: z.string().url(), |
There was a problem hiding this comment.
Important — Zod baseURL: z.string().url() validation is decoupled from the actual config path.
runHeadless goes through loadProviderConfig → configManager.getEffectiveProviderConfig → provider.ts, which never goes through loader.ts. So a malformed baseURL like 'not a url' in ~/.config/zero/config.json is happily accepted by the runtime path, then the resolver's OFFICIAL_OPENAI_BASE_URLS.has(baseURL) returns false, and the runtime falls into the unknown-model branch. Silent misroute.
Same with maxTurns: z.number().int().min(1).max(100) (line 33) — validated by loadConfig, ignored by the manager path.
Ask: Route configManager.getEffectiveProviderConfig through the Zod schema for validation, or document that loader.ts is the "validated" path and the manager path is "trusted local config." Option (a) is the safer default — and the new ProviderProfileSchema in this file is the right schema to apply.
| ); | ||
| }); | ||
|
|
||
| it('requires an API key for the official OpenAI runtime', () => { |
There was a problem hiding this comment.
Important — openai-compatible-without-apiKey path is not tested.
The test at lines 96-104 covers the official-OpenAI case (runtime.provider === 'openai' with no key → throws). The mirror case — runtime.provider === 'openai-compatible' with no key, which falls through to the runtime.apiKey || 'zero-openai-compatible' sentinel at src/zero-provider-runtime/resolver.ts:87 — is not tested. Add a case that asserts the sentinel is used (or whatever the contract ends up being) and that createZeroProvider doesn't throw.
Blockers
Non-Blocking
Looks Good
Verdict: Changes Requested — the resolver currently has unsafe fallback behavior for OpenAI-compatible configs and accepts unknown official-provider models, plus the PR branch does not pass the claimed build/typecheck script validation. |
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Changes requested — resolver has unsafe OpenAI-compatible fallback behavior, accepts unknown official-provider models, and the branch does not pass the claimed build/typecheck script validation.
Resolve provider runtime inputs through the Zero model registry, enforce dispatch-safe OpenAI-compatible gateway URLs, normalize official OpenAI base URLs, and keep provider source metadata in the config boundary. Unify config typing on the loader schema, make provider-command parsing bounded and validated, improve headless adapter errors, and add regression coverage for requested-change cases.
86c77b7 to
ff64254
Compare
BlockersNone from my side now. The previous dispatch-safety blockers are fixed:
Non-Blocking
Looks Good
Verdict: Approved — my requested-change blockers are resolved. |
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approved — my requested-change blockers are resolved. Dispatch safety and branch validation look good now.
anandh8x
left a comment
There was a problem hiding this comment.
Re-review: APPROVE.
All three blockers and all eight important items from the prior review are resolved in ff642544. Test coverage grew from 6 to 25 cases across tests/zero-provider-runtime.test.ts and tests/config-loader.test.ts, and the regression cases I called out (openai shorthand → /v1 normalization, OPENAI_API_KEY-only env fallback, openai-compatible without an API key, explicit-provider mismatch, unknown-model rejection for official providers) all have explicit assertions.
Highlights of what changed:
resolveRuntimeBaseURLnow returnsDEFAULT_BASE_URL.openaiwhen the input is unset or matches an official URL, fixing the 404 path.manager.ts:getEffectiveProviderConfigtriggers onOPENAI_API_KEY || OPENAI_BASE_URL || OPENAI_MODELand usesZERO_DEFAULT_MODEL_ID(not the hardcodedgpt-4o).config/types.tsre-exportsZeroConfigandProviderProfilefromloader.ts— one type, no more split definitions.execawith 5s timeout + Zod-validatedProviderCommandOutputSchemareplaces the unsafeBun.$shell call.runHeadlesscatches adapter-not-implemented errors and prints a targeted hint for anthropic/google.ZeroResolvedProviderRuntime.providerInstance?removed.
Remaining nits (non-blocking, posted inline):
loader.ts:readConfigFilestill silently returns{}on error — fine for introspection but inconsistent withmanager.ts:readConfig's newconsole.warn.cli/index.tsASCII banner is a plainconsole.log; minor style.cli/index.tsre-checksruntime.provider === 'anthropic' || 'google'to print the adapter hint; a single string-match on the resolver error would be more resilient.
CI pending; will merge once green.
| provider = createZeroProvider(runtime); | ||
| } catch (err: any) { | ||
| console.error(`[zero] ${err?.message ?? String(err)}`); | ||
| if (runtime?.provider === 'anthropic' || runtime?.provider === 'google') { |
There was a problem hiding this comment.
Nit — provider-kind check duplicates the resolver's adapter-not-implemented signal.
if (runtime?.provider === 'anthropic' || runtime?.provider === 'google') {
console.error(`[zero] ${runtime.provider} adapter is not yet implemented. ...`);
}If a future slice adds e.g. cohere and the resolver's not implemented yet throw lands, this hint will be silently dropped for that provider. A single check on the error string is more resilient:
const msg = err?.message ?? String(err);
console.error(`[zero] ${msg}`);
if (msg.includes('not implemented yet') && runtime) {
console.error(`[zero] ${runtime.provider} adapter is not yet implemented. ...`);
}Approving either way.
| : 'environment'; | ||
| if (!runtime || !provider) return; | ||
|
|
||
| console.log(` |
There was a problem hiding this comment.
Nit — ASCII banner is plain console.log.
The other runtime lines use [zero] prefix and a key/value shape ([zero] Provider:, [zero] Runtime:, etc.). The raw banner is fine for a CLI, but for parity you could prefix it with a dim color and let it land before the structured log lines. Purely cosmetic; approving.
Summary
Adds the next Gnanam-owned M1 runtime slice after the merged model registry: a Zero provider runtime resolver.
This PR gives Zero a backend contract that resolves
model + profile + provider + baseURLinto a normalized provider runtime plan. It wires headless execution through that resolver while preserving the currently implemented OpenAI-compatible streaming path.The slice intentionally does not implement Anthropic or Gemini streaming adapters yet. Those are the next Gnanam PRD slices. This resolver identifies those providers and returns clear adapter-not-implemented errors until their concrete provider modules land.
What Changed
src/zero-provider-runtime/.resolveZeroProviderRuntimeto:zero-model-registry/v1openaifrom dispatch-safeopenai-compatiblegatewayscreateZeroProvider/createZeroProviderFromInput.providermetadata to saved provider profiles:openaianthropicgoogleopenai-compatibleZeroConfigtyping on the loader schema while keeping provider-kind parsing inconfig/types.ts.PRD Alignment
This implements the second Gnanam M1 dependency from
Zero-WorkSplit-PRD-v2-balanced:Provider factory that resolves model/profile/providerIt depends on the merged Zero model registry (#9) and prepares the contract for later M1 slices:
Validation
Local checks run successfully after rebasing on latest
origin/main(1d1eac2, PR #10 merged):npx --yes bun run typechecknpx --yes bun test ./tests --timeout 1500083 pass0 failnpx --yes bun run buildnpx --yes bun run smoke:build./zero --help./zero providers currentgit diff --check origin/main..HEADReview Notes
main; this branch is rebased on that baseline and uses its build/typecheck/smoke scripts..hermes/monitor files were left untouched and are not included.factory; the Zero-aligned module name iszero-provider-runtime.