fix(config): treat empty env vars as unset in loadConfig#8
Conversation
SahilRakhaiya05
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
WalkthroughEmpty or whitespace-only ChangesWhitespace-only env var normalization
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/config.ts (1)
44-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting shared normalization helper.
The same
env.X?.trim() || undefinedpattern is duplicated insrc/commands/auth.ts(Line 194). Extracting a smallnormalizeEnvVar(value)helper into a shared module would prevent future drift between the two call sites.♻️ Example helper
+export function normalizeEnvVar(value: string | undefined): string | undefined { + return value?.trim() || undefined; +}🤖 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 `@src/lib/config.ts` around lines 44 - 45, The env var normalization logic is duplicated between config parsing and auth code, so extract the repeated `env.X?.trim() || undefined` behavior into a shared helper like `normalizeEnvVar(value)` in a common module and reuse it from `config.ts` and `auth.ts`. Update the existing `envApiUrl` and `envApiKey` assignments in `config.ts`, as well as the corresponding call site in `src/commands/auth.ts`, to use the shared helper so both paths stay consistent.
🤖 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.
Inline comments:
In `@src/lib/bundle.ts`:
- Around line 516-534: The post-swap cleanup in the bundle commit flow can
mistakenly roll back a successfully installed bundle if cleanup fails after
rename(stagingDir, dir). Update the commit/rollback logic in bundle.ts so the
cleanup of priorDir and the .partial marker happens only after the new dir is
safely committed and outside the try/catch that performs rollback, or otherwise
ensure cleanup failures do not enter the rollback path. Keep the rollback
behavior in the same commit/restore flow around dirExists, rename, and rm, but
make cleanup best-effort and non-fatal so a committed bundle is never replaced
by stale contents because rm(priorDir, ...) or unlink(join(dir, '.partial'))
throws.
---
Nitpick comments:
In `@src/lib/config.ts`:
- Around line 44-45: The env var normalization logic is duplicated between
config parsing and auth code, so extract the repeated `env.X?.trim() ||
undefined` behavior into a shared helper like `normalizeEnvVar(value)` in a
common module and reuse it from `config.ts` and `auth.ts`. Update the existing
`envApiUrl` and `envApiKey` assignments in `config.ts`, as well as the
corresponding call site in `src/commands/auth.ts`, to use the shared helper so
both paths stay consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 72db9340-77c5-446b-bce7-d8579aa54c3b
📒 Files selected for processing (5)
src/commands/auth.tssrc/lib/bundle.commit.test.tssrc/lib/bundle.tssrc/lib/config.test.tssrc/lib/config.ts
SahilRakhaiya05
force-pushed
the
fix/config-empty-env-vars
branch
from
94cddc3 to
50521fe
Compare
SahilRakhaiya05
requested review from
ruili-testsprite and
zeshi-du
as code owners
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib/bundle.commit.test.ts (2)
21-96: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffNo coverage for rollback-itself-failing.
The upstream
commitBundlecatch block also performs anrm(dir, ...)+rename(priorDir, dir)when bothpriorDiranddirexist post-failure, and an unguardedrename(priorDir, dir)when onlypriorDirexists — if either of these rollback renames itself throws, that error isn't caught, potentially leavingdirabsent. Consider adding a test that forces the rollback rename to fail to confirm behavior (or document it as a known gap) since it's a plausible double-failure scenario for this atomic-swap logic.🤖 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 `@src/lib/bundle.commit.test.ts` around lines 21 - 96, Add coverage for the rollback path in commitBundle by forcing the recovery rename to fail after the staging swap error. Update the commitBundle test helpers in this spec to simulate a failure in the catch-block rollback branch that uses rm and rename(priorDir, dir), and verify the resulting behavior (or explicitly document the known gap) using the existing commitBundle, renameMock, and rmMock setup.
39-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTemp directories are never cleaned up.
mkdtempSynccreates real directories under the OS tmpdir in both tests, but neither has anafterEach/finallyremovingparent. Over repeated CI runs this leaks directories in the temp filesystem.♻️ Suggested cleanup
it('rolls back to the prior complete bundle when the staging swap fails', async () => { const parent = mkdtempSync(join(tmpdir(), 'bundle-commit-parent-')); + try { ... + } finally { + await realRm(parent, { recursive: true, force: true }); + } });Also applies to: 72-73
🤖 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 `@src/lib/bundle.commit.test.ts` around lines 39 - 40, The temp directories created by mkdtempSync in bundle.commit.test.ts are not removed after the tests finish, causing leftover OS tmpdir entries. Add cleanup in the test setup/teardown for the parent directory created in the bundle commit tests, using the existing test helpers around mkdtempSync/join so both cases are deleted after each run. Keep the fix localized to the affected test cases so the temporary parent directory is always removed even if assertions fail.
🤖 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.
Inline comments:
In `@src/commands/auth.ts`:
- Around line 194-195: Add a regression test in auth.test.ts for runWhoami
covering the dry-run fallback chain when TESTSPRITE_API_URL is whitespace-only.
The issue is that normalizeEnvVar in auth.ts may return an empty value for
env.TESTSPRITE_API_URL, so the endpoint should still fall through to the
default. Update the runWhoami test to use TESTSPRITE_API_URL: ' ' and assert
that resolvedEndpoint ends up using the default https://api.testsprite.com
rather than an empty string.
---
Nitpick comments:
In `@src/lib/bundle.commit.test.ts`:
- Around line 21-96: Add coverage for the rollback path in commitBundle by
forcing the recovery rename to fail after the staging swap error. Update the
commitBundle test helpers in this spec to simulate a failure in the catch-block
rollback branch that uses rm and rename(priorDir, dir), and verify the resulting
behavior (or explicitly document the known gap) using the existing commitBundle,
renameMock, and rmMock setup.
- Around line 39-40: The temp directories created by mkdtempSync in
bundle.commit.test.ts are not removed after the tests finish, causing leftover
OS tmpdir entries. Add cleanup in the test setup/teardown for the parent
directory created in the bundle commit tests, using the existing test helpers
around mkdtempSync/join so both cases are deleted after each run. Keep the fix
localized to the affected test cases so the temporary parent directory is always
removed even if assertions fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dc22d066-b0b3-48ce-8653-ef6a4df1a3ae
📒 Files selected for processing (6)
src/commands/auth.tssrc/commands/init.tssrc/lib/bundle.commit.test.tssrc/lib/bundle.tssrc/lib/config.test.tssrc/lib/config.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/config.ts
- src/lib/config.test.ts
- src/lib/bundle.ts
- Add runWhoami dry-run regression for whitespace-only TESTSPRITE_API_URL - Add commitBundle rollback-rename failure coverage (documents known gap) - Clean up bundle.commit temp dirs after each test
|
This PR is solving Issue #97 |
|
Two updates after a deeper review:
The atomicity goal itself is right (today a failed re-commit can delete |
SahilRakhaiya05
force-pushed
the
fix/config-empty-env-vars
branch
from
5af5770 to
0a1ef36
Compare
SahilRakhaiya05
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/commands/auth.test.ts`:
- Around line 853-867: The dry-run whoami test is still injecting a custom fetch
stub, which bypasses the real offline dry-run path. Update the test in
auth.test.ts so the runWhoami call with dryRun: true relies on the canned
client/sample behavior from whoami and src/lib/dry-run/samples.ts instead of
passing fetchImpl: makeFetch(meResponse()), while keeping the TESTSPRITE_API_URL
whitespace assertion intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1929d5a9-7585-47eb-81c0-3e600c677aa3
📒 Files selected for processing (5)
src/commands/auth.test.tssrc/commands/auth.tssrc/commands/init.tssrc/lib/config.test.tssrc/lib/config.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/commands/init.ts
- src/lib/config.test.ts
- src/lib/config.ts
- src/commands/auth.ts
| it('dry-run: whitespace-only TESTSPRITE_API_URL falls through to prod default endpoint', async () => { | ||
| const { capture, deps } = makeCapture(); | ||
| await runWhoami( | ||
| { profile: 'default', output: 'text', debug: false, dryRun: true }, | ||
| { | ||
| ...deps, | ||
| env: { TESTSPRITE_API_URL: ' ' }, | ||
| credentialsPath, | ||
| fetchImpl: makeFetch(meResponse()), | ||
| }, | ||
| ); | ||
| const out = capture.stdout.join('\n'); | ||
| expect(out).toContain('endpoint: https://api.testsprite.com'); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect runWhoami's dry-run branch for an early return before client.get
ast-grep run --pattern 'export async function runWhoami($_, $_) { $$$ }' --lang typescript src/commands/auth.tsRepository: TestSprite/testsprite-cli
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the dry-run branch in src/commands/auth.ts.
ast-grep outline src/commands/auth.ts --view expanded || true
printf '\n--- auth.ts dry-run references ---\n'
rg -n "dryRun|client\.get\('/me'|resolvedEndpoint|runWhoami" src/commands/auth.ts
printf '\n--- auth.ts relevant slice ---\n'
sed -n '180,280p' src/commands/auth.ts
printf '\n--- auth.test.ts relevant slice ---\n'
sed -n '830,890p' src/commands/auth.test.tsRepository: TestSprite/testsprite-cli
Length of output: 8300
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect docs / nearby implementation for dry-run and endpoint resolution behavior.
rg -n "dry-run|whitespace|TESTSPRITE_API_URL|normalize|whoami" DOCUMENTATION.md src/commands src/lib src/config* src -g '!**/dist/**' || true
printf '\n--- possible config normalization helpers ---\n'
rg -n "function normalize|const normalize|normalizeEnv|trim\\(" src -g '*.ts' || trueRepository: TestSprite/testsprite-cli
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- client-factory dry-run slice ---'
sed -n '1,220p' src/lib/client-factory.ts
printf '\n%s\n' '--- client-factory request creation slice ---'
sed -n '220,360p' src/lib/client-factory.ts
printf '\n%s\n' '--- auth.ts surrounding makeHttpClient call ---'
sed -n '201,255p' src/commands/auth.tsRepository: TestSprite/testsprite-cli
Length of output: 14032
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- dry-run sample registry for whoami ---'
sed -n '360,395p' src/lib/dry-run/samples.ts
printf '\n%s\n' '--- config normalization helper ---'
sed -n '1,120p' src/lib/config.ts
printf '\n%s\n' '--- auth test nearby whitespace/env tests ---'
sed -n '700,875p' src/commands/auth.test.tsRepository: TestSprite/testsprite-cli
Length of output: 10713
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- client-factory dry-run behavior ---'
sed -n '1,220p' src/lib/client-factory.ts
printf '\n%s\n' '--- client-factory request path around dry-run handling ---'
sed -n '220,320p' src/lib/client-factory.ts
printf '\n%s\n' '--- whoami dry-run sample registry entry ---'
sed -n '372,388p' src/lib/dry-run/samples.tsRepository: TestSprite/testsprite-cli
Length of output: 12401
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
text = Path('src/commands/auth.ts').read_text()
# Print the runWhoami function body only.
start = text.index('export async function runWhoami')
end = text.index('export async function runLogout')
print(text[start:end])
PYRepository: TestSprite/testsprite-cli
Length of output: 2500
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/lib/client-factory.ts --view expanded || true
printf '\n--- makeHttpClient slice ---\n'
sed -n '120,320p' src/lib/client-factory.tsRepository: TestSprite/testsprite-cli
Length of output: 8408
Drop the custom fetch stub in this dry-run test
runWhoami(..., dryRun: true) already uses the canned offline client, and whoami has a sample in src/lib/dry-run/samples.ts. Removing fetchImpl here keeps the test on the real --dry-run path instead of a local mock.
🤖 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 `@src/commands/auth.test.ts` around lines 853 - 867, The dry-run whoami test is
still injecting a custom fetch stub, which bypasses the real offline dry-run
path. Update the test in auth.test.ts so the runWhoami call with dryRun: true
relies on the canned client/sample behavior from whoami and
src/lib/dry-run/samples.ts instead of passing fetchImpl:
makeFetch(meResponse()), while keeping the TESTSPRITE_API_URL whitespace
assertion intact.
Source: Path instructions
…pace env key expectation
zeshi-du
left a comment
zeshi-du
left a comment
There was a problem hiding this comment.
Reviewed: scoped, tests included, CI green across the matrix.
2 participants
Summary
When
TESTSPRITE_API_URLorTESTSPRITE_API_KEYis set but empty/whitespace (e.g.export TESTSPRITE_API_URL=in a shell profile), runtime commands no longer resolve to a broken empty endpoint or ignore stored credentials. Matches the normalization already used inauth configure/init.Split from #8 per review - bundle re-commit work moved to a separate PR.
Changes
normalizeEnvVarexported fromloadConfigand used forTESTSPRITE_API_URL/TESTSPRITE_API_KEYauth configure/runWhoamidry-run use the shared helperinitendpoint reporting uses the shared helperTesting
npx vitest run src/lib/config.test.ts- 11 passednpx vitest run src/commands/auth.test.ts -t "whitespace|empty / whitespace"- 4 passednpm run typecheckandnpm run lint- passSummary by CodeRabbit
Bug Fixes
New Features
Tests