feat(cli): add interactive TUI config editor#1529
Conversation
|
All contributors have signed the CLA. Thank you! ✅ |
There was a problem hiding this comment.
7 issues found across 7 files
Confidence score: 3/5
- There is some user-impacting risk in
src/cli/config/index.ts: canceling the discard prompt reloads from disk and drops unsaved in-memory edits, which could surprise users and lose changes. createBackupinsrc/cli/config/index.tsmoves the original config beforewriteConfigAtomically; if the write fails, the original path is left missing, which is a concrete configuration-loss scenario.- Several medium-severity config editor/validation inconsistencies (
src/cli/config/agents.ts,src/cli/config/types.ts,src/cli/config/validation.ts) can silently drop or fail to warn about fallback settings, adding behavioral uncertainty. - Pay close attention to
src/cli/config/index.ts,src/cli/config/agents.ts,src/cli/config/types.ts,src/cli/config/validation.ts- config editor data loss and warning/serialization mismatches.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/cli/config/agents.ts">
<violation number="1" location="src/cli/config/agents.ts:251">
P2: Switching from simple to detailed bash permissions loses the existing simple value; a string permission is treated like a record so every command defaults to "ask".</violation>
</file>
<file name="src/cli/config/types.ts">
<violation number="1" location="src/cli/config/types.ts:20">
P2: The TUI editor adds a `fallback_model` field to agent overrides, but the official config schema (`AgentOverrideConfigSchema`) does not define this property. As a result, any `fallback_model` value saved by the editor will be stripped/ignored during schema validation and won’t be honored at runtime.</violation>
</file>
<file name="src/cli/config/validation.ts">
<violation number="1" location="src/cli/config/validation.ts:40">
P2: Fallback warning checks the primary model field instead of the configured fallback_model, so missing-fallback warnings are incorrect.</violation>
<violation number="2" location="src/cli/config/validation.ts:53">
P2: displayValidationWarnings ignores checkFallbackWarnings results, so "missing-fallback" warnings are never shown despite display logic for them.</violation>
</file>
<file name="src/cli/config/index.ts">
<violation number="1" location="src/cli/config/index.ts:40">
P2: createBackup uses renameSync (move) before writeConfigAtomically; if the write fails, the original config is no longer at configPath and is not restored, risking configuration loss.</violation>
<violation number="2" location="src/cli/config/index.ts:50">
P2: TUI save path re-serializes JSONC with JSON.stringify, which strips all comments/trailing commas from oh-my-opencode.jsonc files.</violation>
<violation number="3" location="src/cli/config/index.ts:169">
P2: Canceling the discard prompt restarts the editor and reloads config from disk, which drops the user's unsaved in-memory changes instead of returning to the current menu state.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| function writeConfigAtomically(configPath: string, config: OhMyOpenCodeConfig): boolean { | ||
| try { | ||
| const tempPath = `${configPath}.tmp` | ||
| writeFileSync(tempPath, JSON.stringify(config, null, 2) + "\n") |
There was a problem hiding this comment.
P2: TUI save path re-serializes JSONC with JSON.stringify, which strips all comments/trailing commas from oh-my-opencode.jsonc files.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/config/index.ts, line 50:
<comment>TUI save path re-serializes JSONC with JSON.stringify, which strips all comments/trailing commas from oh-my-opencode.jsonc files.</comment>
<file context>
@@ -0,0 +1,199 @@
+function writeConfigAtomically(configPath: string, config: OhMyOpenCodeConfig): boolean {
+ try {
+ const tempPath = `${configPath}.tmp`
+ writeFileSync(tempPath, JSON.stringify(config, null, 2) + "\n")
+ renameSync(tempPath, configPath)
+ return true
</file context>
|
🙏 thank you so much I really need this feature |
- Fix createBackup to use copyFileSync instead of renameSync Prevents config loss if write fails after moving original - Fix cancel/discard flow to preserve in-memory unsaved changes Return to menu loop instead of restarting editor - Fix bash permission switching from simple to detailed mode Preserve existing simple value as default for all commands - Fix checkFallbackWarnings to check fallback_model field Was incorrectly checking model field instead - Fix displayValidationWarnings to include fallback warnings Now combines both model and fallback warnings Addresses issues raised by @cubic-dev-ai in PR code-yeongyu#1529
|
I have read the CLA Document and I hereby sign the CLA |
|
Thanks @cubic-dev-ai for the thorough review! I've addressed all the issues in commit ca09979: Fixed Issues✅ P2: createBackup uses move instead of copy - Now uses ✅ P2: Cancel discard restarts editor, loses unsaved changes - Refactored to use ✅ P2: Bash permission switching loses existing simple value - Now detects if existing value is a string and uses it as default for all commands ✅ P2: Fallback warning checks wrong field - Fixed to check ✅ P2: displayValidationWarnings ignores checkFallbackWarnings - Now combines both Deferred⏸️ P2: JSON.stringify strips JSONC comments - This matches the existing project pattern in Regarding
|
|
Thanks @acamq! 🙏 I built this because I needed it too - manually editing JSON configs was painful. Let me know if you find any issues when testing! |
**Source**: code-yeongyu#1529 **Author**: WietRob (contributor) **Branch**: WietRob/feature/tui-config-editor → dev ## Feature Overview Adds a new `oh-my-opencode config` command providing an interactive TUI for managing configuration. - **Interactive Editing**: Configure agents, categories, root defaults, and bulk operations via `@clack/prompts`. - **Validation**: checks for missing models, undefined categories, and invalid permissions. - **Safety**: Atomic writes and automatic backups (`config.json.backup.{timestamp}`) before saving. ## Integration Details - **Modular CLI Structure**: Integrated `config` command into `src/cli/cli-program.ts` (the new CLI entry point in dev branch), resolving conflict with legacy `src/cli/index.ts`. - **New Files**: Added `src/cli/config/` directory with agent, category, and bulk editors. ## Patch Hierarchy This feature adds a new CLI command and does not modify core agent logic or existing hooks. It depends on `src/cli/cli-program.ts` structure introduced in recent dev refactors. ## Verification ```bash bun run typecheck # ✅ Passed (0 errors) bun run build # ✅ Passed ```
|
recheck |
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/cli/config/models.ts">
<violation number="1" location="src/cli/config/models.ts:9">
P2: Cache directory logic is duplicated and ignores XDG_CACHE_HOME; CLI may read the wrong cache when the env var is set. Use the shared getOpenCodeCacheDir() helper to stay consistent.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/cli/config/models.ts">
<violation number="1" location="src/cli/config/models.ts:13">
P2: The new model loader only reads models.json and ignores provider-models.json, even though the app still writes provider-models.json as the preferred cache. This can break the config model picker when only provider-models.json is present.</violation>
</file>
<file name="src/cli/config/bulk.ts">
<violation number="1" location="src/cli/config/bulk.ts:30">
P2: Bulk fallback model operation ignores the "Clear selection" choice and returns early, so users cannot bulk-unset fallback_model even though the UI offers that option.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
recheck |
|
Hi @code-yeongyu, it seems the CLA workflow is consistently failing/crashing (after ~5s) despite my signature comment. I suspect an issue with the workflow configuration or permissions in the repo. Could you please check? |
|
Hi @WietRob, I found the root cause — all commits in this PR were made via the GitHub Web UI, so Git records the author/committer as I've added |
|
recheck |
- Fix createBackup to use copyFileSync instead of renameSync Prevents config loss if write fails after moving original - Fix cancel/discard flow to preserve in-memory unsaved changes Return to menu loop instead of restarting editor - Fix bash permission switching from simple to detailed mode Preserve existing simple value as default for all commands - Fix checkFallbackWarnings to check fallback_model field Was incorrectly checking model field instead - Fix displayValidationWarnings to include fallback warnings Now combines both model and fallback warnings Addresses issues raised by @cubic-dev-ai in PR code-yeongyu#1529
0e28964 to
72157c4
Compare
|
recheck |
There was a problem hiding this comment.
3 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/shared/preflight/live-probe.ts">
<violation number="1" location="src/shared/preflight/live-probe.ts:15">
P1: Custom agent: **Opencode Compatibility**
The Opencode SDK does not expose a `model.list()` method. To retrieve available models, use `client.provider.list()` instead.</violation>
<violation number="2" location="src/shared/preflight/live-probe.ts:23">
P1: Redundant model validation that always returns success even when model is not found</violation>
<violation number="3" location="src/shared/preflight/live-probe.ts:29">
P2: Error message checks are case-sensitive and will miss standard capitalized API error messages. Convert `errMsg` to lowercase before checking to ensure detection of errors like "Rate Limit Exceeded" or "Unauthorized".</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/shared/preflight/cache.ts">
<violation number="1" location="src/shared/preflight/cache.ts:21">
P0: Custom agent: **Opencode Compatibility**
Removing the `models.json` fallback breaks compatibility with OpenCode's native model caching. Preflight will incorrectly flag valid OpenCode models as unavailable (because it now only checks Oh-My-Opencode's internal cache) and aggressively auto-switch users away from their configured models.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".opencode/oh-my-opencode.jsonc">
<violation number="1" location=".opencode/oh-my-opencode.jsonc:7">
P1: Schema mismatch: Configuration uses `fallback_models` (array) but the TypeScript interface `AgentConfigExtended` expects `fallback_model` (string)</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| "agents": { | ||
| "oracle": { | ||
| "model": "zai/glm5", | ||
| "fallback_models": ["kimi/k2p5", "openai/gpt4o"] |
There was a problem hiding this comment.
P1: Schema mismatch: Configuration uses fallback_models (array) but the TypeScript interface AgentConfigExtended expects fallback_model (string)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .opencode/oh-my-opencode.jsonc, line 7:
<comment>Schema mismatch: Configuration uses `fallback_models` (array) but the TypeScript interface `AgentConfigExtended` expects `fallback_model` (string)</comment>
<file context>
@@ -0,0 +1,48 @@
+ "agents": {
+ "oracle": {
+ "model": "zai/glm5",
+ "fallback_models": ["kimi/k2p5", "openai/gpt4o"]
+ },
+ "sisyphus": {
</file context>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/shared/preflight/live-probe.ts">
<violation number="1" location="src/shared/preflight/live-probe.ts:14">
P1: Model ID parsing truncates model names containing multiple slashes. The destructuring `const [providerId, modelId] = model.split("/")` only captures the first two segments, losing any additional path segments in model identifiers like `openrouter/anthropic/claude-3.5-sonnet`. This causes validation to fail for valid multi-slash models.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/shared/preflight/index.ts">
<violation number="1" location="src/shared/preflight/index.ts:163">
P2: Custom agent: **Opencode Compatibility**
The `opencode` CLI does not have a `doctor` command. This should instruct the user to run the plugin's own diagnostics command.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/cli/run/runner.ts">
<violation number="1" location="src/cli/run/runner.ts:63">
P1: Resource leak: If `preflightWithProbe` throws, execution jumps to outer catch which has no access to `serverCleanup`. `preflightWithProbe` should be moved inside the inner try block where `cleanup()` is available.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Your configuration at .opencode/oh-my-opencode.jsonc was commited and is included in this PR |
|
The configuration doesn't handle an empty string when I select "Root Defaults" and type nothing and press enter. ┌ oh-my-opencode config
│
● Config file: /[redacted]/oh-my-opencode.json
│
◇ What would you like to configure?
│ [3] Root Defaults
│
◇ Enter default agent for 'oh-my-opencode run' command:
│ e.g., sisyphus
62 | placeholder: "e.g., sisyphus",
63 | })
64 |
65 | if (p.isCancel(model)) return
66 |
67 | const finalModel = model.trim() || undefined
^
TypeError: undefined is not an object (evaluating 'model.trim')
at editRootDefaults (/[redacted]/src/cli/config/index.ts:67:22)
at async showMainMenu (/[redacted]/src/cli/config/index.ts:102:13)
at async runMenuLoop (/[redacted]/src/cli/config/index.ts:121:26)
at async runConfigEditor (/[redacted]/src/cli/config/index.ts:201:9)
at async <anonymous> (/[redacted]/src/cli/cli-program.ts:189:28)
Bun v1.3.9 (Linux x64) |
|
I cloned and tested this PR. It's great! I have a few suggestions for future enhancements that could make this TUI config editor even more powerful: 1. Schema-Driven Configuration UICurrently, the TUI manually exposes only a subset of options (agents model/category/fallback/permissions, categories, default_run_agent). However, the Zod schema in
Suggestion: Consider parsing the Zod schema introspectively (e.g., using
2. Search/Filter for Models and ProvidersThe model picker currently requires browsing by provider, which works well for small lists. However, with 50+ models across providers, a fuzzy search/filter would significantly improve UX:
This would be especially valuable when users know what they're looking for but don't remember which provider it belongs to. |
81947f8 to
0c0a08f
Compare
There was a problem hiding this comment.
4 issues found across 9 files
Confidence score: 2/5
- Two high-severity, high-confidence issues in
src/cli/config/bulk.tsandsrc/cli/config/models.tsindicate likely regressions in core config behavior (fallback promotion/cleanup andprovider-models.jsonparsing), which makes this risky to merge as-is. - The
src/cli/config/bulk.tsbehavior can drop secondary fallbacks and mishandle duplicate promotion when clearing primary fallback models, creating concrete user-facing config corruption risk. - There is additional security/behavior risk in
src/cli/config/index.ts: atomic writes may reset restrictive file permissions on sensitive config files, though confidence is low;src/cli/config/agents.tsalso allows untrimmed empty values that can trigger unintended fallback deletion paths. - Pay close attention to
src/cli/config/bulk.ts,src/cli/config/models.ts,src/cli/config/index.ts, andsrc/cli/config/agents.ts- fallback integrity, model parsing correctness, permission preservation, and input validation need verification.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/cli/config/bulk.ts">
<violation number="1" location="src/cli/config/bulk.ts:126">
P1: Custom agent: **Opencode Compatibility**
The bulk operation for setting fallback models fails to preserve secondary fallbacks when clearing the primary, and does not filter duplicates when promoting a secondary fallback.
(Based on your team's feedback about preserving secondary fallbacks and filtering duplicates.) [FEEDBACK_USED]</violation>
</file>
<file name="src/cli/config/models.ts">
<violation number="1" location="src/cli/config/models.ts:19">
P1: Custom agent: **Opencode Compatibility**
Fix `provider-models.json` parsing to iterate over `data.models` instead of the root object, and handle metadata objects.</violation>
</file>
<file name="src/cli/config/index.ts">
<violation number="1" location="src/cli/config/index.ts:62">
P2: Atomic write replaces the config file with a temp file created using default permissions, so any restrictive permissions on the original config (e.g., 0600) are lost and may expose sensitive config data.</violation>
</file>
<file name="src/cli/config/agents.ts">
<violation number="1" location="src/cli/config/agents.ts:102">
P2: Custom model/category/fallback inputs are saved without trimming/validation, allowing empty/whitespace values to be written to config and potentially triggering unintended fallback deletion logic.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (!agentsMutable[agentName]) agentsMutable[agentName] = {} | ||
| const existing = agentsMutable[agentName].fallback_models | ||
| const existingArray = Array.isArray(existing) ? existing : existing ? [String(existing)] : [] | ||
| if (finalModel) { |
There was a problem hiding this comment.
P1: Custom agent: Opencode Compatibility
The bulk operation for setting fallback models fails to preserve secondary fallbacks when clearing the primary, and does not filter duplicates when promoting a secondary fallback.
(Based on your team's feedback about preserving secondary fallbacks and filtering duplicates.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/config/bulk.ts, line 126:
<comment>The bulk operation for setting fallback models fails to preserve secondary fallbacks when clearing the primary, and does not filter duplicates when promoting a secondary fallback.
(Based on your team's feedback about preserving secondary fallbacks and filtering duplicates.) </comment>
<file context>
@@ -0,0 +1,201 @@
+ if (!agentsMutable[agentName]) agentsMutable[agentName] = {}
+ const existing = agentsMutable[agentName].fallback_models
+ const existingArray = Array.isArray(existing) ? existing : existing ? [String(existing)] : []
+ if (finalModel) {
+ agentsMutable[agentName].fallback_models = [finalModel, ...existingArray.slice(1)]
+ } else {
</file context>
| try { | ||
| const content = readFileSync(providerModelsJsonPath, "utf-8") | ||
| const data = JSON.parse(content) | ||
| if (typeof data === "object" && data !== null && !Array.isArray(data)) { |
There was a problem hiding this comment.
P1: Custom agent: Opencode Compatibility
Fix provider-models.json parsing to iterate over data.models instead of the root object, and handle metadata objects.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/config/models.ts, line 19:
<comment>Fix `provider-models.json` parsing to iterate over `data.models` instead of the root object, and handle metadata objects.</comment>
<file context>
@@ -0,0 +1,93 @@
+ try {
+ const content = readFileSync(providerModelsJsonPath, "utf-8")
+ const data = JSON.parse(content)
+ if (typeof data === "object" && data !== null && !Array.isArray(data)) {
+ for (const [provider, models] of Object.entries(data)) {
+ // Skip special keys and prototype pollution vectors
</file context>
|
PR is mergeable! Can someone with merge permissions please merge? Thanks! |
|
@cubic-dev-ai Already fixed in latest commit:
All 4 issues addressed. Please re-review. |
@WietRob I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
7 issues found across 9 files
Confidence score: 2/5
- There is a high-confidence regression risk in
src/cli/config/index.ts:writeConfigAtomicallynot preserving original permissions can weaken intended config-file restrictions after save, which is a significant user-impacting behavior change. src/cli/config/bulk.tsandsrc/cli/config/validation.tsintroduce concrete config-integrity risks (fallback loss/duplication and accepting unknown categories), so users may end up with misleadingly “valid” but broken settings.src/cli/config/agents.tsandsrc/cli/config/categories.tscontain interactive-flow issues (missing current-value preselection, untrimmed custom inputs, sentinel collisions) that can silently overwrite or misroute user choices.- Pay close attention to
src/cli/config/index.ts,src/cli/config/bulk.ts,src/cli/config/agents.ts,src/cli/config/validation.ts,src/cli/config/categories.ts- permission preservation and config editing flows can produce unintended or invalid persisted state.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/cli/config/index.ts">
<violation number="1" location="src/cli/config/index.ts:62">
P1: Custom agent: **Opencode Compatibility**
The `writeConfigAtomically` function does not preserve original file permissions. When writing to the new `.tmp` file, it uses default permissions which overwrite any restrictive permissions on the target configuration file during `renameSync`. Please update the implementation to read the original file's mode (e.g., via `statSync`) and apply it when writing the temporary file.
(Based on your team's feedback about preserving original file permissions after atomic write.) [FEEDBACK_USED]</violation>
<violation number="2" location="src/cli/config/index.ts:83">
P2: Dirty-state is set unconditionally in root defaults editor, causing false save prompts even when `default_run_agent` is unchanged.</violation>
</file>
<file name="src/cli/config/bulk.ts">
<violation number="1" location="src/cli/config/bulk.ts:125">
P1: Custom agent: **Opencode Compatibility**
The bulk operations logic fails to preserve secondary fallbacks when clearing the primary model, and it doesn't filter out duplicates when promoting a new fallback model.
(Based on your team's feedback about bulk.ts: Preserved secondary fallbacks when clearing primary + filter duplicates when promoting) [FEEDBACK_USED]</violation>
</file>
<file name="src/cli/config/validation.ts">
<violation number="1" location="src/cli/config/validation.ts:15">
P2: Validation accepts any non-empty agent category string without checking whether that category exists, which can hide configuration problems and produce false "no warnings" results.</violation>
</file>
<file name="src/cli/config/categories.ts">
<violation number="1" location="src/cli/config/categories.ts:122">
P2: Category option values collide with reserved control sentinels (`__new__`, `__back__`), causing wrong TUI action for valid config keys.</violation>
</file>
<file name="src/cli/config/agents.ts">
<violation number="1" location="src/cli/config/agents.ts:102">
P2: Custom agent text inputs are saved without trim/validation, allowing blank values and causing unintended fallback clearing behavior.</violation>
<violation number="2" location="src/cli/config/agents.ts:242">
P2: Permission selection prompts omit current-value preselection, so Enter can silently overwrite existing settings with default `ask`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (model === "__clear__") { | ||
| finalModel = undefined | ||
| } else if (model === "__custom__") { | ||
| const custom = await p.text({ |
There was a problem hiding this comment.
P2: Custom agent text inputs are saved without trim/validation, allowing blank values and causing unintended fallback clearing behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/config/agents.ts, line 102:
<comment>Custom agent text inputs are saved without trim/validation, allowing blank values and causing unintended fallback clearing behavior.</comment>
<file context>
@@ -0,0 +1,344 @@
+ if (model === "__clear__") {
+ finalModel = undefined
+ } else if (model === "__custom__") {
+ const custom = await p.text({
+ message: "Enter custom model name:",
+ initialValue: agent.model ?? "",
</file context>
|
@cubic-dev-ai All 7 P2 issues fixed:
Please re-review. |
@WietRob I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
8 issues found across 9 files
Confidence score: 2/5
- There are multiple high-confidence, user-facing behaviors in config flows (sev 6–7) that raise regression risk, especially around fallback management and save semantics.
- Most severe impact is in
src/cli/config/bulk.ts: clearing/promoting fallbacks can drop secondary fallbacks and create duplicates, which can leave runtime fallback state inconsistent. src/cli/config/index.tscan overwrite config even if backup creation fails and also rewrites JSONC viaJSON.stringify, removing comments/formatting; together this weakens recovery and alters user-maintained files unexpectedly.- Pay close attention to
src/cli/config/bulk.ts,src/cli/config/index.ts,src/cli/config/categories.ts, andsrc/cli/config/models.ts- fallback integrity, backup/write safety, cancel-side effects, and null-safe legacy parsing need validation.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/cli/config/bulk.ts">
<violation number="1" location="src/cli/config/bulk.ts:126">
P1: Bulk fallback update logic drops secondary fallbacks on clear and can create duplicate entries when promoting an existing fallback.</violation>
</file>
<file name="src/cli/config/index.ts">
<violation number="1" location="src/cli/config/index.ts:63">
P2: Saving config with `JSON.stringify` after JSONC parsing silently strips user comments and formatting from the config file.</violation>
<violation number="2" location="src/cli/config/index.ts:182">
P2: Config save proceeds even when backup creation fails, causing silent loss of backup protection before overwrite.</violation>
</file>
<file name="src/cli/config/categories.ts">
<violation number="1" location="src/cli/config/categories.ts:156">
P2: New category creation mutates config before edit confirmation, so canceling edit leaves undeletable empty categories and marks config modified.</violation>
</file>
<file name="src/cli/config/agents.ts">
<violation number="1" location="src/cli/config/agents.ts:8">
P1: The `ExtendedAgentConfig` type is identically defined in both agents.ts and bulk.ts. This duplication should be consolidated into a shared type in types.ts.</violation>
<violation number="2" location="src/cli/config/agents.ts:77">
P2: Edit/webfetch permissions can be set but not cleared in the TUI, so once an override is applied there is no way to remove it and return to inherited defaults.</violation>
</file>
<file name="src/cli/config/models.ts">
<violation number="1" location="src/cli/config/models.ts:44">
P2: Accessing `item.provider` in the legacy array parsing assumes every element is an object; a `null` entry will throw and abort parsing the entire file. Add a null/object guard before checking properties.</violation>
<violation number="2" location="src/cli/config/models.ts:44">
P3: Legacy array fallback does not filter special provider keys ("__proto__", "constructor", "prototype"), unlike other formats, allowing these values to be registered as providers.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (!agentsMutable[agentName]) agentsMutable[agentName] = {} | ||
| const existing = agentsMutable[agentName].fallback_models | ||
| const existingArray = Array.isArray(existing) ? existing : existing ? [String(existing)] : [] | ||
| if (finalModel) { |
There was a problem hiding this comment.
P1: Bulk fallback update logic drops secondary fallbacks on clear and can create duplicate entries when promoting an existing fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/config/bulk.ts, line 126:
<comment>Bulk fallback update logic drops secondary fallbacks on clear and can create duplicate entries when promoting an existing fallback.</comment>
<file context>
@@ -0,0 +1,201 @@
+ if (!agentsMutable[agentName]) agentsMutable[agentName] = {}
+ const existing = agentsMutable[agentName].fallback_models
+ const existingArray = Array.isArray(existing) ? existing : existing ? [String(existing)] : []
+ if (finalModel) {
+ agentsMutable[agentName].fallback_models = [finalModel, ...existingArray.slice(1)]
+ } else {
</file context>
| s.start("Saving configuration...") | ||
|
|
||
| const backupPath = createBackupWithCopy(state.configPath) | ||
| const success = writeConfigAtomically(state.configPath, state.config) |
There was a problem hiding this comment.
P2: Config save proceeds even when backup creation fails, causing silent loss of backup protection before overwrite.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/config/index.ts, line 182:
<comment>Config save proceeds even when backup creation fails, causing silent loss of backup protection before overwrite.</comment>
<file context>
@@ -0,0 +1,226 @@
+ s.start("Saving configuration...")
+
+ const backupPath = createBackupWithCopy(state.configPath)
+ const success = writeConfigAtomically(state.configPath, state.config)
+
+ if (!success) {
</file context>
|
|
||
| const tempPath = `${targetPath}.tmp` | ||
| const existingPerms = existsSync(targetPath) ? statSync(targetPath).mode : undefined | ||
| writeFileSync(tempPath, JSON.stringify(config, null, 2) + "\n") |
There was a problem hiding this comment.
P2: Saving config with JSON.stringify after JSONC parsing silently strips user comments and formatting from the config file.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/config/index.ts, line 63:
<comment>Saving config with `JSON.stringify` after JSONC parsing silently strips user comments and formatting from the config file.</comment>
<file context>
@@ -0,0 +1,226 @@
+
+ const tempPath = `${targetPath}.tmp`
+ const existingPerms = existsSync(targetPath) ? statSync(targetPath).mode : undefined
+ writeFileSync(tempPath, JSON.stringify(config, null, 2) + "\n")
+ if (existingPerms) {
+ chmodSync(tempPath, existingPerms)
</file context>
|
@cubic-dev-ai Final round of fixes:
All 8 issues addressed. Please re-review. |
@WietRob I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
8 issues found across 9 files
Confidence score: 3/5
- There are several concrete, user-facing behavior risks in the CLI config flows, so this sits at moderate merge risk rather than a safe-to-merge 4/5.
- The highest-impact issue is in
src/cli/config/bulk.ts: bulk clearing the fallback model can wipe the entirefallback_modelsarray, which can remove secondary fallbacks unexpectedly; the same file can also introduce duplicate fallback entries during promotion. - Control-flow and validation regressions in
src/cli/config/index.ts,src/cli/config/agents.ts, andsrc/cli/config/validation.tscan mis-handle Ctrl+C exits, accept empty fallback input, hide save error details, and emit false warnings for valid configs. - Pay close attention to
src/cli/config/bulk.ts,src/cli/config/index.ts,src/cli/config/agents.ts,src/cli/config/validation.ts,src/cli/config/ui-utils.ts- fallback mutation, exit/save handling, and navigation/validation behavior can lead to confusing or incorrect outcomes.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/cli/config/models.ts">
<violation number="1" location="src/cli/config/models.ts:19">
P1: Custom agent: **Opencode Compatibility**
Fix `provider-models.json` parsing to iterate over `data.models` instead of the root object, and handle metadata objects.</violation>
<violation number="2" location="src/cli/config/models.ts:22">
P1: Custom agent: **Opencode Compatibility**
The parsing of `provider-models.json` fails to extract models because it iterates over the root object instead of `data.models`, and it incorrectly discards object-based models instead of extracting their `id`.</violation>
</file>
<file name="src/cli/config/ui-utils.ts">
<violation number="1" location="src/cli/config/ui-utils.ts:1">
P1: Custom agent: **Opencode Compatibility**
The new `config` command needs to be registered in `src/cli/cli-program.ts` so that it can be executed by the CLI.
(Based on your team's feedback about the config command needing to be registered in cli-program.ts.) [FEEDBACK_USED]</violation>
<violation number="2" location="src/cli/config/ui-utils.ts:54">
P2: Back from provider model list incorrectly restarts the root selector instead of returning to provider selection.</violation>
</file>
<file name="src/cli/config/index.ts">
<violation number="1" location="src/cli/config/index.ts:28">
P2: Filesystem write errors are swallowed by a bare catch, hiding critical error context needed to debug save failures.</violation>
<violation number="2" location="src/cli/config/index.ts:61">
P2: Static temporary file path creates race conditions for concurrent saves. Use unique temp filename pattern like `${targetPath}.tmp.${Date.now()}` to match existing codebase pattern.</violation>
<violation number="3" location="src/cli/config/index.ts:62">
P1: Custom agent: **Opencode Compatibility**
The `writeConfigAtomically` function does not preserve original file permissions. When writing to the new `.tmp` file, it uses default permissions which overwrite any restrictive permissions on the target configuration file during `renameSync`. Please update the implementation to read the original file's mode (e.g., via `statSync`) and apply it when writing the temporary file.
(Based on your team's feedback about preserving original file permissions after atomic write.) [FEEDBACK_USED]</violation>
<violation number="4" location="src/cli/config/index.ts:63">
P2: Saving config with `JSON.stringify` after JSONC parsing silently strips user comments and formatting from the config file.</violation>
<violation number="5" location="src/cli/config/index.ts:167">
P2: Ctrl+C cancel is handled as a negative response in save/discard prompts, causing users with unsaved changes to be routed back into menu + recursive save prompts instead of exiting cleanly.</violation>
<violation number="6" location="src/cli/config/index.ts:182">
P2: Config save proceeds even when backup creation fails, causing silent loss of backup protection before overwrite.</violation>
</file>
<file name="src/cli/config/bulk.ts">
<violation number="1" location="src/cli/config/bulk.ts:124">
P2: Bulk fallback promotion can create duplicate fallback models because the new primary model is prepended without filtering it out of existing secondary entries.</violation>
<violation number="2" location="src/cli/config/bulk.ts:125">
P1: Custom agent: **Opencode Compatibility**
The bulk operations logic fails to preserve secondary fallbacks when clearing the primary model, and it doesn't filter out duplicates when promoting a new fallback model.
(Based on your team's feedback about bulk.ts: Preserved secondary fallbacks when clearing primary + filter duplicates when promoting) [FEEDBACK_USED]</violation>
<violation number="3" location="src/cli/config/bulk.ts:126">
P1: Custom agent: **Opencode Compatibility**
The bulk operation for setting fallback models fails to preserve secondary fallbacks when clearing the primary, and does not filter duplicates when promoting a secondary fallback.
(Based on your team's feedback about preserving secondary fallbacks and filtering duplicates.) [FEEDBACK_USED]</violation>
<violation number="4" location="src/cli/config/bulk.ts:126">
P1: Bulk fallback update logic drops secondary fallbacks on clear and can create duplicate entries when promoting an existing fallback.</violation>
<violation number="5" location="src/cli/config/bulk.ts:126">
P2: Bulk clear of fallback model deletes the entire `fallback_models` array, causing loss of secondary fallbacks instead of only removing the primary fallback.</violation>
</file>
<file name="src/cli/config/categories.ts">
<violation number="1" location="src/cli/config/categories.ts:122">
P2: Category option values collide with reserved control sentinels (`__new__`, `__back__`), causing wrong TUI action for valid config keys.</violation>
</file>
<file name="src/cli/config/agents.ts">
<violation number="1" location="src/cli/config/agents.ts:102">
P2: Custom agent text inputs are saved without trim/validation, allowing blank values and causing unintended fallback clearing behavior.</violation>
<violation number="2" location="src/cli/config/agents.ts:183">
P2: Custom fallback model input is not validated/trimmed, so empty input can unintentionally clear or reorder fallback_models due to downstream truthiness logic.</violation>
</file>
<file name="src/cli/config/validation.ts">
<violation number="1" location="src/cli/config/validation.ts:10">
P2: Category validation only checks user-defined categories, so valid built-in categories can be incorrectly flagged as undefined.</violation>
<violation number="2" location="src/cli/config/validation.ts:28">
P2: `validateConfig` emits `missing-model` for sparse agent overrides that intentionally only set non-model fields, causing false-positive warnings.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const existing = agentsMutable[agentName].fallback_models | ||
| const existingArray = Array.isArray(existing) ? existing : existing ? [String(existing)] : [] | ||
| if (finalModel) { | ||
| agentsMutable[agentName].fallback_models = [finalModel, ...existingArray.slice(1)] |
There was a problem hiding this comment.
P2: Bulk fallback promotion can create duplicate fallback models because the new primary model is prepended without filtering it out of existing secondary entries.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/config/bulk.ts, line 124:
<comment>Bulk fallback promotion can create duplicate fallback models because the new primary model is prepended without filtering it out of existing secondary entries.</comment>
<file context>
@@ -0,0 +1,198 @@
+ const existing = agentsMutable[agentName].fallback_models
+ const existingArray = Array.isArray(existing) ? existing : existing ? [String(existing)] : []
+ if (finalModel) {
+ agentsMutable[agentName].fallback_models = [finalModel, ...existingArray.slice(1)]
+ } else {
+ delete agentsMutable[agentName].fallback_models
</file context>
| if (finalModel) { | ||
| agentsMutable[agentName].fallback_models = [finalModel, ...existingArray.slice(1)] | ||
| } else { | ||
| delete agentsMutable[agentName].fallback_models |
There was a problem hiding this comment.
P2: Bulk clear of fallback model deletes the entire fallback_models array, causing loss of secondary fallbacks instead of only removing the primary fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/config/bulk.ts, line 126:
<comment>Bulk clear of fallback model deletes the entire `fallback_models` array, causing loss of secondary fallbacks instead of only removing the primary fallback.</comment>
<file context>
@@ -0,0 +1,198 @@
+ if (finalModel) {
+ agentsMutable[agentName].fallback_models = [finalModel, ...existingArray.slice(1)]
+ } else {
+ delete agentsMutable[agentName].fallback_models
+ }
+ }
</file context>
| const content = readFileSync(path, "utf-8") | ||
| const config = parseJsonc<OhMyOpenCodeConfig>(content) | ||
| return config ?? {} | ||
| } catch { |
There was a problem hiding this comment.
P2: Filesystem write errors are swallowed by a bare catch, hiding critical error context needed to debug save failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/config/index.ts, line 28:
<comment>Filesystem write errors are swallowed by a bare catch, hiding critical error context needed to debug save failures.</comment>
<file context>
@@ -0,0 +1,230 @@
+ const content = readFileSync(path, "utf-8")
+ const config = parseJsonc<OhMyOpenCodeConfig>(content)
+ return config ?? {}
+ } catch {
+ return null
+ }
</file context>
| initialValue: false, | ||
| }) | ||
|
|
||
| if (p.isCancel(discard) || !discard) { |
There was a problem hiding this comment.
P2: Ctrl+C cancel is handled as a negative response in save/discard prompts, causing users with unsaved changes to be routed back into menu + recursive save prompts instead of exiting cleanly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/config/index.ts, line 167:
<comment>Ctrl+C cancel is handled as a negative response in save/discard prompts, causing users with unsaved changes to be routed back into menu + recursive save prompts instead of exiting cleanly.</comment>
<file context>
@@ -0,0 +1,230 @@
+ initialValue: false,
+ })
+
+ if (p.isCancel(discard) || !discard) {
+ p.log.info("Returning to menu...")
+ await runMenuLoop(state)
</file context>
| const warnings: ValidationWarning[] = [] | ||
| const agents = state.config.agents ?? {} | ||
| const categories = state.config.categories ?? {} | ||
| const validCategories = Object.keys(categories) |
There was a problem hiding this comment.
P2: Category validation only checks user-defined categories, so valid built-in categories can be incorrectly flagged as undefined.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/config/validation.ts, line 10:
<comment>Category validation only checks user-defined categories, so valid built-in categories can be incorrectly flagged as undefined.</comment>
<file context>
@@ -0,0 +1,90 @@
+ const warnings: ValidationWarning[] = []
+ const agents = state.config.agents ?? {}
+ const categories = state.config.categories ?? {}
+ const validCategories = Object.keys(categories)
+
+ for (const agentName of AGENT_NAMES) {
</file context>
| }) | ||
| } | ||
|
|
||
| if (!hasModel && !hasCategory) { |
There was a problem hiding this comment.
P2: validateConfig emits missing-model for sparse agent overrides that intentionally only set non-model fields, causing false-positive warnings.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/config/validation.ts, line 28:
<comment>`validateConfig` emits `missing-model` for sparse agent overrides that intentionally only set non-model fields, causing false-positive warnings.</comment>
<file context>
@@ -0,0 +1,90 @@
+ })
+ }
+
+ if (!hasModel && !hasCategory) {
+ warnings.push({
+ type: "missing-model",
</file context>
| }) | ||
|
|
||
| if (p.isCancel(model)) return model | ||
| if (model === "__back__") return selectModelWithCacheLoader(message, initialValue) |
There was a problem hiding this comment.
P2: Back from provider model list incorrectly restarts the root selector instead of returning to provider selection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/config/ui-utils.ts, line 54:
<comment>Back from provider model list incorrectly restarts the root selector instead of returning to provider selection.</comment>
<file context>
@@ -0,0 +1,82 @@
+ })
+
+ if (p.isCancel(model)) return model
+ if (model === "__back__") return selectModelWithCacheLoader(message, initialValue)
+
+ return model
</file context>
| initialValue: currentFallback ?? "", | ||
| }) | ||
| if (p.isCancel(custom)) return false | ||
| finalFallback = custom |
There was a problem hiding this comment.
P2: Custom fallback model input is not validated/trimmed, so empty input can unintentionally clear or reorder fallback_models due to downstream truthiness logic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/config/agents.ts, line 183:
<comment>Custom fallback model input is not validated/trimmed, so empty input can unintentionally clear or reorder fallback_models due to downstream truthiness logic.</comment>
<file context>
@@ -0,0 +1,367 @@
+ initialValue: currentFallback ?? "",
+ })
+ if (p.isCancel(custom)) return false
+ finalFallback = custom
+ } else {
+ finalFallback = model as string
</file context>
|
[sisyphus-bot] PR sweep first-pass triage on
Needs rebase + review. Please rebase onto current Assigning |
Policy-based model routing for oh-my-openagent with dual-mode design: - Policy mode: rankings + provider state -> auto-resolve model + fallback - Manual mode: schema-driven TUI for direct per-field editing - Cross-provider fallback enforcement (primary != fallback provider) - Provider enable/disable with usage counts - Multi-target model assignment - 10-rule validation system - Idempotent build with backup - CLI subcommands: omoa status/build/provider 20 unit tests (resolver + validator), 0 tsc errors.
P1: Silent fallback guard - readOmoaRankings/readOmoaState now log on parse failure; writeOmoaRankings refuses to overwrite existing file with empty defaults. P1: Schema editor persistence - root-editor, agent-editor, and category-editor now save to disk after every field mutation using shared saveRuntimeConfig() helper. P2: Edit ranking no-op - ranking-screen 'edit' action now offers remove/add flows instead of silently returning. P2: Category fallback counting - countProviderUsage now counts fallback models in categories, not just agents. P2: free_only diagnostic - resolver now reports specific reason per model (banned/deprecated/free-only/provider-disabled) instead of generic 'banned/deprecated'. P2: Dry-run validation - includes agents that would be newly created from rankings, not just existing agents. P2: Sentinel collision - replaced '__clear__' with '__sentinel_clear__' to avoid collision with legitimate user values. P2: Hard-coded field list - acknowledged as Phase 2 work (deep Zod introspection). fallback_models intentionally excluded since OMOA manages it.
The 'newly created from rankings' loops iterated all changes but wrote into agent/category maps without checking targetKind. When both agent and category changes coexist, a category change would write into validationState (agent map) and vice versa, causing incorrect data or TypeError. Added early continue for mismatched targetKind.
|
[sisyphus-bot] Hi WietRob. 🙏 Thanks for the interactive TUI config editor work. Picking this back up from the 5/16 triage. The PR is large (1308/0 across 9 files) and shows CONFLICTING against current Adding a new TUI surface is a substantive product decision. Worth noting: your other open PR #4158 (OMOA model management system) also adds a CLI editor surface. The two CLI editors should probably share a unified entry point + framework rather than diverging. Could you rebase against current |
eb25d29 to
2bfad49
Compare
Add type definitions, validation logic, and unit tests for the interactive TUI config editor. Types define ConfigEditorState and agent/category config structures. Validation checks for missing models and fallbacks. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Implement model cache parsing from provider-models.json and models.json, a model selection UI with provider-browse and flat-list views, and the main config editor entry point with backup, atomic write, and load/save cycle. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Implement interactive editors for agent model assignments, category model assignments, and bulk operations (set model for all agents, clear models). Uses @clack/prompts for TUI and supports cache-backed model browsing. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Add omoaConfig export to omoa/index.ts that delegates to the interactive TUI config editor. Register 'omoa config' subcommand in cli-program.ts with help text. Both OMOA-managed (rankings) and manual (config editor) model management now accessible under the unified omoa CLI. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
6ad01fc to
740c542
Compare
|
Rebased against current Changes made:
Relationship to PR #4158 (OMOA):
The PR description has been updated with the full composition framework. Ready for review. |
Summary
Rebases the interactive TUI config editor against current
origin/devand integrates it as anomoa configsubcommand under the unified OMOA CLI framework.Key change: The config editor is now accessible via
omoa configinstead of a standalone command, sharing the same entry point as the OMOA model management system (PR #4158).Relationship to PR #4158 (OMOA Model Management)
These two PRs are complementary, not competing:
omoa buildreads rankings + provider state → writes optimal configomoa configlets you manually browse models, edit assignments, bulk-setomoa-rankings.json+omoa-state.json→ resolved intooh-my-openagent.jsonoh-my-openagent.jsonwith validation and backupHow they compose:
omoa buildkeeps them optimal)omoa configfor agents where you need manual control (overrides rankings for specific agents)omoa build|omoa config|omoa statusomoa interactive→ "Edit Config"What Changed in This Rebase
New Files (8)
src/cli/config/index.tssrc/cli/config/agents.tssrc/cli/config/categories.tssrc/cli/config/models.tssrc/cli/config/ui-utils.tssrc/cli/config/validation.tssrc/cli/config/bulk.tssrc/cli/config/types.tsModified Files (2)
src/cli/omoa/index.tsomoaConfig()exportsrc/cli/cli-program.tsomoa configsubcommandTest File (1)
src/cli/config/__tests__/config-editor.test.tsWhat Was Removed
oh-my-opencode configcommand — integrated asomoa configschema-editor/is untouched)Usage
Testing
Type of Change
Checklist