refactor(opencode): deduplicate shared code, add Zod schemas, fix recursive dialog, add logging#166
refactor(opencode): deduplicate shared code, add Zod schemas, fix recursive dialog, add logging#166aaronjo-Telnyx wants to merge 11 commits into
Conversation
…ursive dialog, add logging
Comparison with opencode-tree plugin revealed several quality issues:
1. Deduplicate server/TUI shared code (HIGHEST)
- Extract isObject(), isTelnyxHostedModel(), storedApiKey(),
apiKey(), normalizeModel(), fetchModels(), fetchAllHostedModelIDs(),
fetchHostedModels(), providerModels() to src/shared.ts
- Both index.ts and tui.tsx now import from shared module
- Zero duplication between the two entry points
2. Add Zod schemas for config validation
- ModelsConfigFileSchema validates telnyx-models.json format
- TelnyxCredentialSchema validates auth.json credential format
- safeParse used in loadEnabledModels() and storedApiKey()
- Parse errors logged before falling back to defaults
- Added zod to dependencies
3. Add logging in all catch blocks
- Replaced empty catch blocks with console.error('[telnyx]', ...)
- Includes operation context for debugging
- Applied across shared.ts and models-config.ts
4. Fix recursive openManager dialog pattern
- Replaced recursive await openManager(api) with local
renderDialog() function that re-renders in-place
- Each model toggle now calls renderDialog() instead of
re-entering openManager(), avoiding stack growth
5. Inject IO dependencies for testability
- Dependencies interface with readFile and fetchModels
- Default implementations use real fs/fetch
- replaceDependencies() for test injection
- Shared fetch logic consolidated into deps.fetchModels
…er, unit tests
1. Switch models-config.ts to async file I/O (fs/promises)
- loadEnabledModels, persistEnabledModels, persistDefaultModelsConfigIfMissing
are now async and use readFile/writeFile/mkdir/rename from node:fs/promises
- All callers in index.ts, tui.tsx, shared.ts updated to await
- shared.ts Dependencies.readFile now returns Promise<string>
- storedApiKey() and apiKey() are now async
2. Validate-on-write in persistEnabledModels
- Runs ModelsConfigFileSchema.parse(payload) before writing to disk
- Guarantees we can never write something we can't read back
3. Extract buildProviderConfig() helper to shared.ts
- Eliminates duplicated provider config construction in index.ts and tui.tsx
- Handles optional apiKey (omitted when key is undefined)
4. Add unit tests for shared.ts and models-config.ts
- test/shared.test.ts: isObject, isTelnyxHostedModel, normalizeModel,
modelConfigEntry, buildProviderConfig, storedApiKey (via DI)
- test/models-config.test.ts: defaultModelsConfig, loadEnabledModels
(valid, missing, invalid JSON, wrong schema, wrong version, empty strings),
persistEnabledModels (write, dedup, empty, atomic, schema validation)
- All 39 tests pass
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9bbc81f06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!parsed.success) { | ||
| console.error("[telnyx] invalid models config file, falling back to defaults:", parsed.error) | ||
| return [...DEFAULT_ENABLED_MODELS] |
There was a problem hiding this comment.
Falling back to DEFAULT_ENABLED_MODELS for any schema mismatch means a single malformed entry in enabledModels (for example from manual edits or a stale file) discards all otherwise valid saved selections. Before this refactor, invalid entries were filtered out while valid strings were retained; now users can unexpectedly lose their curated allowlist and silently switch back to defaults. Consider salvaging valid IDs from enabledModels instead of resetting the entire config when only part of the payload is invalid.
Useful? React with 👍 / 👎.
| } catch (error) { | ||
| console.error("[telnyx] failed to read auth file:", error) | ||
| return undefined |
There was a problem hiding this comment.
When no Telnyx credential has been saved yet, reading auth.json commonly fails with ENOENT, but this path now logs an error every time storedApiKey() runs. That makes normal first-run behavior look like a failure and can flood logs during startup or /telnyx usage. Treat missing-file errors as expected (return undefined quietly) and reserve console.error for unexpected read/parse failures.
Useful? React with 👍 / 👎.
|
Both issues flagged by @codex have been addressed in the current branch:
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
… silence ENOENT on first run - models-config.ts: when Zod validation fails, attempt to salvage valid string entries from enabledModels rather than falling back to defaults entirely. Only fall back to DEFAULT_ENABLED_MODELS if zero valid entries can be salvaged. - shared.ts: storedApiKey() now returns undefined silently for ENOENT (auth.json missing = normal first-run state). Only console.error on unexpected failures. Addresses P2 and P3 from Codex review.
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
… silence ENOENT in storedApiKey loadEnabledModels now validates each model ID individually when the overall schema fails, keeping valid entries rather than replacing the entire config with defaults. storedApiKey silently returns undefined on ENOENT (normal first-run) and only logs unexpected errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
aaronjo-Telnyx
requested review from
Oliver-Zimmerman,
aisling404 and
gbattistel
as code owners
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
3 participants
Summary
Refactor of the
@telnyx/opencodeplugin for code quality improvements identified from comparison with the opencode-tree plugin.Changes
1. Deduplicate Server/TUI Shared Code (HIGHEST PRIORITY)
src/shared.ts:isObject(),isTelnyxHostedModel()type helpersstoredApiKey(),apiKey()auth readingnormalizeModel(),modelConfigEntry()model normalizationfetchModels(),fetchAllHostedModelIDs(),fetchHostedModels()API callsproviderModels()for building the provider config mapindex.tsandtui.tsxnow import from the shared module2. Add Zod Schemas for Config
ModelsConfigFileSchemavalidatestelnyx-models.jsonformatTelnyxCredentialSchemavalidatesauth.jsoncredential formatsafeParseused inloadEnabledModels()andstoredApiKey()zodtodependencies3. Add Logging in Catch Blocks
catch {}blocks withconsole.error("[telnyx]", ...)loggingshared.tsandmodels-config.ts4. Fix Recursive openManager Dialog Pattern
await openManager(api)with a localrenderDialog()function that re-renders in-placerenderDialog()instead of re-enteringopenManager()5. Inject IO Dependencies for Testability
Dependenciesinterface withreadFileandfetchModelsinjectable functionsfs/fetchreplaceDependencies()for test injectionVerification
npm run typecheck— passesnpm run build— passes,dist/output correctnode --test test/models-config.test.ts— 3/3 tests passWhat NOT Changed
test/run.ts) untouchedchat.paramshook preserved