tests(#476): add cli/env.js with 41 unit tests for env-var resolution, override precedence, fallback guards#328
Open
javimosch wants to merge 3 commits into
Open
tests(#476): add cli/env.js with 41 unit tests for env-var resolution, override precedence, fallback guards#328javimosch wants to merge 3 commits into
javimosch wants to merge 3 commits into
Conversation
Export diffCatalogs, readLocalCatalog, writeLocalCatalog, downloadTarball, and extractPluginsFromTarball for direct testing. Test coverage: - diffCatalogs (7): null/empty local → all added, identical checksums → unchanged, stale checksum → changed, mixed add+change+unchanged - readLocalCatalog (6): missing file, valid JSON, invalid JSON, null/non-object parsed value, fs.readFileSync throws - writeLocalCatalog (4): creates dir when missing, skips mkdirSync when dir exists, writes to correct path, trailing newline - downloadTarball (5): spawnSync error, non-zero exit, missing archive, zero-byte archive, success path - extractPluginsFromTarball (7): creates destDir, correct tar args, tolerates exit 2, throws on exit 1, throws on spawnSync error, batches >200 into 2 calls, 200 uses 1 call - updatePlugins (8): check=true skips download, up-to-date skips download, force=true ignores local catalog, stale catalog triggers full update+write, tmpDir cleaned via finally even on error, remote_count in result, fetch failure throws, invalid catalog format throws Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Covers all four exported functions: - handleCommandsQuery: guard clauses (null config, invalid limit/offset), namespace/resource/action/query filters, pagination (limit, offset, auto-limit), _warning for truncated auto-limit, human mode (outputHumanTable), args * formatting, and exception catch → outputError code 110 - handleInspect: positional length guard, unknown command (code 92), full spec structure (side_effects, risk_level, input_schema required array, defaults), human mode console.log path - handleSchema: input_schema property/required mapping, type defaulting, output_schema default and override, empty-args edge case - handleNamespaceBrowse: namespace browse (1 positional), resource browse (2 positionals), unknown namespace/resource errors (code 92), human mode All 50 tests pass in 0.265s. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Create cli/env.js to centralise all SUPERCLI_* and OPENAI_* env-var reads
with explicit fallback defaults and a clear override-precedence contract.
Getters:
- getSupercliHome() → SUPERCLI_HOME || ~/.supercli
- getServer() → SUPERCLI_SERVER || null
- getApiKey() → SUPERCLI_API_KEY || null
- getClientId() → SUPERCLI_CLIENT_ID trimmed || null (whitespace-only → null)
- getOpenAiBaseUrl() → OPENAI_BASE_URL || null (doubles as hasLocalLLM gate)
- getOpenAiModel() → OPENAI_MODEL || "gpt-3.5-turbo"
- getOpenAiApiKey() → OPENAI_API_KEY || "dummy"
- interpolateEnvPlaceholders(text) → expands ${VARNAME} in strings
__tests__/env.test.js: 41 tests covering env-set, env-unset, empty-string
fallback, whitespace-trim, truthy gating, non-string pass-through, adjacent
and repeated placeholders, dollar-sign-no-braces not expanded, and unknown
variable → empty string.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds Jest coverage for CLI command querying, inspection, schema, and namespace browsing; introduces ChangesCommands Handler Coverage
Environment Helpers
Plugin Update Helpers and Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Owner
Author
|
Review — Head of Org Engineering: The PR adds a new test file (approved — review-only mode; merge when ready.) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
__tests__/commands-handler.test.js (2)
131-199: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd one trim-coverage case for filters.
handleCommandsQuerytrimsnamespace,resource,action, andquerybefore matching, but this suite only locks in case-insensitivity. A single" git "/" push "style assertion would pin that contract and catch regressions incli/commands-handler.js:3-12.🤖 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 `@__tests__/commands-handler.test.js` around lines 131 - 199, Add a trim-coverage test in the filtering suite for handleCommandsQuery to verify that leading/trailing spaces are ignored for filter inputs. Use the existing makeCmd, makeConfig, and handleCommandsQuery helpers, and assert that values like " git " and " push " still match the expected commands, so the trim contract for namespace/resource/action/query stays pinned even if case-insensitivity already passes.
257-275: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake console mocking failure-safe.
These tests restore
console.logmanually, so an assertion failure beforemockRestore()can leak the spy into later cases. A sharedafterEach(() => jest.restoreAllMocks())is safer and lets you drop the per-test cleanup.Also applies to: 381-388, 492-499, 520-527
🤖 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 `@__tests__/commands-handler.test.js` around lines 257 - 275, The console.log spy cleanup in the human mode tests is fragile because a failed assertion can skip mockRestore and leak the spy into later cases. Update the commands handler test suite to use a shared afterEach with jest.restoreAllMocks(), and remove the per-test manual restore from the human mode block and any similar console spy tests so cleanup is guaranteed even on failures.cli/env.js (1)
48-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate placeholder interpolation with
cli/mcp-discovery.js.This implementation matches
cli/mcp-discovery.js:9-12, so the new “centralized” helper can still drift from the existing runtime path. Reusing one implementation would keep the regex and missing-var behavior aligned.🤖 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 `@cli/env.js` around lines 48 - 53, The placeholder interpolation logic in interpolateEnvPlaceholders is duplicated with the runtime path in mcp-discovery, so keep both behaviors aligned by reusing a single shared helper instead of maintaining two separate implementations. Move the ${VARNAME} replacement logic into one common utility and have both env.js and the mcp-discovery code call that shared function so the regex handling and unknown-variable behavior stay consistent.__tests__/plugins-update.test.js (1)
201-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCapture each failure once instead of re-running the helper.
These cases invoke the same failing path twice just to assert the message and metadata. That duplicates
spawnSync/fetchside effects and makes the tests fragile if these code paths start usingmockReturnValueOnce, retries, or extra cleanup. Capture the thrown error from a single call and assert everything on that object.♻️ Suggested pattern
- expect(() => downloadTarball(tmpDir)).toThrow("curl exited with status 7") - const err = (() => { try { downloadTarball(tmpDir) } catch (e) { return e } })() + let err + try { + downloadTarball(tmpDir) + } catch (e) { + err = e + } + expect(err.message).toContain("curl exited with status 7") expect(err.code).toBe(105) - await expect(updatePlugins()).rejects.toThrow("Failed to fetch plugin catalog") - const err = await updatePlugins().catch(e => e) + const err = await updatePlugins().catch(e => e) + expect(err.message).toContain("Failed to fetch plugin catalog") expect(err.code).toBe(105)Also applies to: 273-280, 427-437
🤖 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 `@__tests__/plugins-update.test.js` around lines 201 - 215, The plugin update tests are calling the same failing helper twice to check message and metadata, which can duplicate side effects and make the mocks brittle. In the affected tests around downloadTarball, capture the thrown error from a single invocation in a local variable and assert both the error message and its properties on that object instead of re-running the helper. Apply the same pattern to the other duplicated failure assertions in plugins-update.test.js so each error path is exercised only once.
🤖 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 `@__tests__/commands-handler.test.js`:
- Around line 131-199: Add a trim-coverage test in the filtering suite for
handleCommandsQuery to verify that leading/trailing spaces are ignored for
filter inputs. Use the existing makeCmd, makeConfig, and handleCommandsQuery
helpers, and assert that values like " git " and " push " still match the
expected commands, so the trim contract for namespace/resource/action/query
stays pinned even if case-insensitivity already passes.
- Around line 257-275: The console.log spy cleanup in the human mode tests is
fragile because a failed assertion can skip mockRestore and leak the spy into
later cases. Update the commands handler test suite to use a shared afterEach
with jest.restoreAllMocks(), and remove the per-test manual restore from the
human mode block and any similar console spy tests so cleanup is guaranteed even
on failures.
In `@__tests__/plugins-update.test.js`:
- Around line 201-215: The plugin update tests are calling the same failing
helper twice to check message and metadata, which can duplicate side effects and
make the mocks brittle. In the affected tests around downloadTarball, capture
the thrown error from a single invocation in a local variable and assert both
the error message and its properties on that object instead of re-running the
helper. Apply the same pattern to the other duplicated failure assertions in
plugins-update.test.js so each error path is exercised only once.
In `@cli/env.js`:
- Around line 48-53: The placeholder interpolation logic in
interpolateEnvPlaceholders is duplicated with the runtime path in mcp-discovery,
so keep both behaviors aligned by reusing a single shared helper instead of
maintaining two separate implementations. Move the ${VARNAME} replacement logic
into one common utility and have both env.js and the mcp-discovery code call
that shared function so the regex handling and unknown-variable behavior stay
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: acdbcd7b-5461-44c8-b1e3-6499233a84aa
📒 Files selected for processing (5)
__tests__/commands-handler.test.js__tests__/env.test.js__tests__/plugins-update.test.jscli/env.jscli/plugins-update.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Create
cli/env.js— a thin, testable module that centralises all SUPERCLI_* and OPENAI_* environment-variable reads, and add 41 unit tests for it.Why
Before this PR every file read
process.env.Xinline, with no shared contract for which vars have defaults and which return null when missing. Two separate functions inskills-catalog.jsboth readSUPERCLI_HOMEbut returned different hard-coded fallback paths — a latent inconsistency. Centralising incli/env.jsgives a single place to audit override precedence, default values, and whitespace handling.Getters added (
cli/env.js)getSupercliHome()SUPERCLI_HOME~/.supercligetServer()SUPERCLI_SERVERnullgetApiKey()SUPERCLI_API_KEYnullgetClientId()SUPERCLI_CLIENT_IDnull(trims whitespace; whitespace-only → null)getOpenAiBaseUrl()OPENAI_BASE_URLnull(truthy = hasLocalLLM)getOpenAiModel()OPENAI_MODEL"gpt-3.5-turbo"getOpenAiApiKey()OPENAI_API_KEY"dummy"interpolateEnvPlaceholders(text)${VARNAME}""if unsetTests (
__tests__/env.test.js) — 41 tests, 8 suites$VAR(no braces) left unchanged, non-string passthrough (number, null, undefined, array), lowercase var names not expandedVerification
Closes #476 (mago task #476)
Summary by CodeRabbit
New Features
Bug Fixes
Tests