fix: separate plugin updates from config install - #207
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughThe CLI adds cache-only ChangesInstaller modes and cache management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as oc-codex-multi-auth
participant Cache as OpenCode managed cache
participant Config as OpenCode config files
User->>CLI: Run update
CLI->>Cache: Clear managed package directories
CLI-->>User: Return without reading or writing config
User->>CLI: Run install --plugin-only
CLI->>Config: Read and merge plugin/TUI entries
CLI-->>Config: Write only changed configuration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/install-oc-codex-multi-auth.test.ts (1)
574-592: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test for malformed
tui.jsonin--plugin-onlymode.The core script's plugin-only path now also throws when
tui.jsonfails to parse (mirrors theopencode.jsoncase), but only theopencode.jsonmalformed scenario is covered here. Consider adding a parallel test asserting the TUI-config throw and that the malformedtui.jsoncontent is left untouched.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/install-oc-codex-multi-auth.test.ts` around lines 574 - 592, Add a parallel test for malformed tui.json in the install --plugin-only flow, using runInstaller with a temporary HOME and invalid TUI configuration content. Assert that installation rejects with the TUI-specific refusal error and that tui.json remains unchanged, with no unexpected files added to its directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/install-oc-codex-multi-auth.test.ts`:
- Around line 574-592: Add a parallel test for malformed tui.json in the install
--plugin-only flow, using runInstaller with a temporary HOME and invalid TUI
configuration content. Assert that installation rejects with the TUI-specific
refusal error and that tui.json remains unchanged, with no unexpected files
added to its directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 87935198-3c0a-43f2-95cc-d6abdcb8afa1
📒 Files selected for processing (11)
CHANGELOG.mdREADME.mddocs/development/ARCHITECTURE.mddocs/development/CONFIG_FLOW.mddocs/getting-started.mddocs/tools-and-cli.mdlib/auto-update-checker.tsscripts/install-oc-codex-multi-auth-core.jsskills/oc-codex-setup/SKILL.mdtest/auto-update-checker.test.tstest/install-oc-codex-multi-auth.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/install-oc-codex-multi-auth-core.js (2)
908-923: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject structurally invalid JSON in plugin-only mode.
readJsoncan successfully parse arrays, scalars, ornull, but plugin-only mode then spreads or coerces them into an object and writes a replacement configuration. This can overwrite malformed user configuration instead of refusing the operation. ValidateisPlainObject(existing)before merging both the main and TUI configs.This conflicts with the PR objective that plugin-only mode rejects malformed configuration.
Suggested guard
const existing = await readJson(paths.configPath); +if (pluginOnly && !isPlainObject(existing)) { + throw new Error("Refusing to replace structurally invalid config in --plugin-only mode."); +} existingConfig = existing;Also applies to: 940-950
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install-oc-codex-multi-auth-core.js` around lines 908 - 923, Validate that each parsed configuration is a plain object before merging, including the main config flow around existingConfig/nextConfig and the corresponding TUI config flow. In plugin-only mode, reject arrays, scalars, null, and other structurally invalid values instead of spreading or coercing them into replacement configurations; preserve the existing handling for valid objects.
962-968: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRedact configuration values before logging dry-run diffs.
formatConfigDiffprints the complete existing and proposed JSON. Arbitrary plugin settings may contain account emails, access tokens, refresh tokens, or other secrets, exposing them in normal CLI diagnostics. Redact sensitive fields or limit the diff to managed keys.As per coding guidelines, normal diagnostics must not expose account emails, access tokens, refresh tokens, or raw prompt/response bodies.
Suggested direction
- log(formatConfigDiff(existingConfig, nextConfig)); + log(formatRedactedConfigDiff(existingConfig, nextConfig));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install-oc-codex-multi-auth-core.js` around lines 962 - 968, Update the dryRun logging in the configuration flow to sanitize values before passing existingConfig, nextConfig, existingTuiConfig, or nextTuiConfig to formatConfigDiff. Redact account emails, access tokens, refresh tokens, and raw prompt/response bodies, or restrict the output to managed keys, while preserving the existing diff and change-status messages.Source: Coding guidelines
🧹 Nitpick comments (1)
test/install-oc-codex-multi-auth.test.ts (1)
854-859: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCover both Windows transient lock codes.
The regression test exercises
EPERMonly, while the updater contract includes bothEPERMandEBUSY. Add a second case or parameterize the test so either code cannot regress unnoticed.The PR objective explicitly calls out
EPERM/EBUSYretry handling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/install-oc-codex-multi-auth.test.ts` around lines 854 - 859, Extend the “retries update cache removal after transient Windows lock errors” test to cover both transient error codes, EPERM and EBUSY, preferably by parameterizing the cases. Preserve the existing first-failure retry and successful removal assertions for each code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/install-oc-codex-multi-auth-core.js`:
- Around line 908-923: Validate that each parsed configuration is a plain object
before merging, including the main config flow around existingConfig/nextConfig
and the corresponding TUI config flow. In plugin-only mode, reject arrays,
scalars, null, and other structurally invalid values instead of spreading or
coercing them into replacement configurations; preserve the existing handling
for valid objects.
- Around line 962-968: Update the dryRun logging in the configuration flow to
sanitize values before passing existingConfig, nextConfig, existingTuiConfig, or
nextTuiConfig to formatConfigDiff. Redact account emails, access tokens, refresh
tokens, and raw prompt/response bodies, or restrict the output to managed keys,
while preserving the existing diff and change-status messages.
---
Nitpick comments:
In `@test/install-oc-codex-multi-auth.test.ts`:
- Around line 854-859: Extend the “retries update cache removal after transient
Windows lock errors” test to cover both transient error codes, EPERM and EBUSY,
preferably by parameterizing the cases. Preserve the existing first-failure
retry and successful removal assertions for each code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c05b3bb4-8d06-4d98-95e9-17ffdde34ebc
📒 Files selected for processing (3)
CHANGELOG.mdscripts/install-oc-codex-multi-auth-core.jstest/install-oc-codex-multi-auth.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
|
@coderabbitai Addressed the malformed |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
@coderabbitai Addressed all remaining findings in f4a938d. Default install now preserves |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ ✏️ Learnings added
|
Rationale for changing default install behaviorOpenCode configuration is user-owned and often carefully customized. The previous default installation wrote the complete model catalog into The default installation now makes only the minimum changes required to register the plugin:
Model catalog installation is explicitly opt-in:
The This makes the safe, least-invasive behavior the default while retaining explicit options for users who want the package to manage their OpenAI model catalog. The README, getting-started guide, CLI documentation, configuration guide, FAQ, setup skill, and architecture documentation have been updated accordingly. |
1 participant
Summary
provider.openai--modern,--full, or--legacyfor model catalog installationupdatecommand that never reads or writes OpenCode config@latestcache layoutsEPERMandEBUSYcache locksVerification
npm test(2,836 passed)npm run typechecknpm run lintnote: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr separates cache updates from configuration installation.
updatecommand and redirects update notifications to it.provider.openai.@latestpackage layouts.Confidence Score: 3/5
this pr is not yet safe to merge because a persistent windows filesystem lock can still abort an update after partially clearing the cache.
cache removals run sequentially, exhausted EPERM or EBUSY retries propagate out of the update command, and dependency metadata cleanup is skipped after earlier deletions have already completed; vitest covers only a lock released after one failed attempt.
Files Needing Attention: scripts/install-oc-codex-multi-auth-core.js, test/install-oc-codex-multi-auth.test.ts
Important Files Changed
Reviews (4): Last reviewed commit: "fix: preserve provider config in default..." | Re-trigger Greptile