feat(config): read ~/.testsprite/config for persistent defaults (endpoint_url, output, project_id) - #225
feat(config): read ~/.testsprite/config for persistent defaults (endpoint_url, output, project_id)#225Andy00L wants to merge 3 commits into
Conversation
|
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 skipped from review as they are similar to previous changes (2)
WalkthroughAdds profile-scoped settings from ChangesConfig-file defaults and hardened parsing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant loadConfig
participant CredentialsFile
participant readConfigFileSettings
CLI->>loadConfig: request configuration for profile
loadConfig->>CredentialsFile: read credentials
loadConfig->>readConfigFileSettings: read profile settings
readConfigFileSettings-->>loadConfig: endpointUrl, output, projectId
loadConfig-->>CLI: resolved apiUrl
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Reviewed: the INI parser hardening is solid (null-prototype accumulator + explicit proto/constructor/prototype skips), and the endpoint_url chain reads correctly as flag > env > credentials-file > config-file > default. Sequencing decision (also posted on #249): #249 lands first, then this PR rebases on top, folding the config-file |
|
@Andy00L #249 just merged, so this is unblocked. Two things for the rebase:
Rebase and it goes back into normal review — the sequencing hold is lifted. |
…oint_url, output, project_id)
a07d97b to
4c96c72
Compare
|
Rebased on main after #249 (and #272). Config-file project_id folded into resolveProjectId as the third fallback (flag > env > config file) with the profile threaded through; requireProjectId keeps main's assert shape with the enriched message; parseIniFile hardening now sits alongside the new profile-write locking in credentials.ts. Full suite green (2022 tests). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
src/commands/test.ts (1)
9179-9183: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep missing-project guidance aligned with config defaults.
The
--allmessage omits the config-file fallback entirely, and the default message omitsTESTSPRITE_CONFIG_FILE. Tell users thatproject_idmay be set in~/.testsprite/configor the override file.Proposed fix
- '--all requires a project id - pass --project <id> or set TESTSPRITE_PROJECT_ID', + '--all requires a project id - pass --project <id>, set TESTSPRITE_PROJECT_ID, or set project_id in ~/.testsprite/config (or TESTSPRITE_CONFIG_FILE)', - message = 'is required; pass --project <id>, set TESTSPRITE_PROJECT_ID, or set project_id in ~/.testsprite/config', + message = 'is required; pass --project <id>, set TESTSPRITE_PROJECT_ID, or set project_id in the config file (~/.testsprite/config or TESTSPRITE_CONFIG_FILE)',As per path instructions, “
TESTSPRITE_PROJECT_IDis still the env fallback, but config-fileproject_idis now the next fallback.”Also applies to: 9827-9832
🤖 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/test.ts` around lines 9179 - 9183, Update the missing-project guidance in requireProjectId call sites, including the --all message near resolveProjectId and the default message around the additionally referenced location, to mention that project_id can be configured in ~/.testsprite/config or the TESTSPRITE_CONFIG_FILE override file. Preserve TESTSPRITE_PROJECT_ID as the environment fallback and keep the existing project and config resolution behavior unchanged.Source: Path instructions
🧹 Nitpick comments (1)
src/lib/credentials.ts (1)
108-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid hardening — null-prototype design correctly neutralizes
__proto__pollution.The
Object.create(null)accumulator plus explicit rejection of__proto__/constructor/prototypesection and key names is well-reasoned defense-in-depth, and the final plain-object copy never re-introduces a dangerous key since it only iterates already-filtered entries.One minor nit: the dangerous-key list (
__proto__,constructor,prototype) is duplicated at Lines 124-126 and Line 148. Extracting a sharedconst DANGEROUS_KEYS = new Set([...])would prevent the two checks from silently diverging if the list ever needs to change.♻️ Suggested consolidation
+const DANGEROUS_INI_KEYS = new Set(['__proto__', 'constructor', 'prototype']); + export function parseIniFile(content: string): Record<string, Record<string, string>> { ... - if ( - sectionName === '__proto__' || - sectionName === 'constructor' || - sectionName === 'prototype' - ) { + if (DANGEROUS_INI_KEYS.has(sectionName)) { currentEntry = null; continue; } ... - if (rawKey === '__proto__' || rawKey === 'constructor' || rawKey === 'prototype') continue; + if (DANGEROUS_INI_KEYS.has(rawKey)) continue;🤖 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/credentials.ts` around lines 108 - 155, Consolidate the duplicated dangerous-key checks in parseIniFile by defining one shared DANGEROUS_KEYS set containing __proto__, constructor, and prototype, then reuse it for both section-name and raw-key validation. Keep the existing skip behavior unchanged and ensure both checks use the same centralized list.
🤖 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 `@src/commands/test.ts`:
- Around line 9179-9183: Update the missing-project guidance in requireProjectId
call sites, including the --all message near resolveProjectId and the default
message around the additionally referenced location, to mention that project_id
can be configured in ~/.testsprite/config or the TESTSPRITE_CONFIG_FILE override
file. Preserve TESTSPRITE_PROJECT_ID as the environment fallback and keep the
existing project and config resolution behavior unchanged.
---
Nitpick comments:
In `@src/lib/credentials.ts`:
- Around line 108-155: Consolidate the duplicated dangerous-key checks in
parseIniFile by defining one shared DANGEROUS_KEYS set containing __proto__,
constructor, and prototype, then reuse it for both section-name and raw-key
validation. Keep the existing skip behavior unchanged and ensure both checks use
the same centralized list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 087a78c0-8ab9-4a71-9598-6f92dd231b46
📒 Files selected for processing (5)
src/commands/test.tssrc/index.tssrc/lib/config.test.tssrc/lib/config.tssrc/lib/credentials.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/config.test.ts
- src/index.ts
- src/lib/config.ts
… dedupe dangerous-key checks, prettier pass
|
Lint red fixed (the rebase resolution had not been through prettier). Also addressed the review notes: both missing-project messages now mention the config-file fallback incl. TESTSPRITE_CONFIG_FILE, and the dangerous-key checks in parseIniFile share one DANGEROUS_INI_KEYS set. |
|
The design here was approved back on #179, before the process accident that closed it. I wrote at the time:
You did both, including re-threading the fold once #144's own recovery (#249) finally merged. I checked the current code: What's left is two small conflicts against
Also still missing, and it's the one thing CodeRabbit's pre-merge check keeps flagging: issue #100 explicitly asked to extend the
One more thing worth saying plainly. This PR's last activity is 2026-07-23, and under this repo's stale-bot settings (45 days idle → marked stale, 21 more days → auto-closed, and it carries none of the exempt labels) it is on track to hit that clock in September. Given that #179 — the PR this one recovers — was already closed once by an unrelated process accident, I'm not letting that happen a second time. I'll keep this from being auto-closed while the conflict resolution above is outstanding. |
zeshi-du
left a comment
zeshi-du
left a comment
There was a problem hiding this comment.
Approving.
Design was already approved. This is #179 under a new number — same commits (head was a07d97b2, now rebased to c333af8a), reopened after our 07-09 release-process incident closed it, through no fault of the branch. I signed off on the shape on 2026-07-05: "the aws-cli credentials-vs-config split, the parseIniFile extraction with the null-prototype/proto-key hardening, and config sitting below the credentials file in the apiUrl cascade are all right." I re-checked the current diff against that, not just the PR description:
- Precedence is flag > env > config file > built-in default everywhere it applies:
loadConfig'sapiUrlcascade (src/lib/config.ts), the global--outputdefault (configFileOutputDefaultinsrc/index.ts), and project-id resolution (resolveProjectIdinsrc/commands/test.ts). - The config-file layer folds into the existing
resolveProjectIdhelper — the fold-in #179 asked for once theTESTSPRITE_PROJECT_IDenv fallback landed — instead of adding a second lookup path. That fallback is#144's feature;#144itself stayed closed by the same incident, but it landed onmainvia#249on 07-23, so this is exactly that ask, done. parseIniFile(src/lib/credentials.ts) is sound: a null-prototype accumulator (Object.create(null)) plus an explicit__proto__/constructor/prototyperejection on both section names and keys.parseCredentialsnow delegates to it with byte-identical behavior, including the CR/LF handling, which is untouched.
The conflict is two adjacent-insertion collisions, not competing logic — I checked the actual merge against current main, not just the "CONFLICTING" label:
src/commands/test.ts—mainpicked up unrelated new imports (emitTargetUrlV3Advisory, thegh-output.jsblock) immediately above theloadConfigimport (currently line 106) since this branch was cut. Your branch changes that same line toimport { loadConfig, readConfigFileSettings } from '../lib/config.js';. Keep both: retainmain's new imports above, and apply your edit to theloadConfigline.src/index.ts—maininsertedimport { TARGETS, type AgentTarget } from './lib/agent-targets.js';(line 18) at the exact point your branch insertsimport { readConfigFileSettings } from './lib/config.js';. Keep both, either order.
Nothing else conflicts — src/lib/config.ts, src/lib/config.test.ts, and src/lib/credentials.ts apply cleanly.
Rebase
git fetch upstream main
git rebase upstream/main
# conflict 1 (src/commands/test.ts): keep main's new imports above the
# loadConfig line, and change that line to import readConfigFileSettings too
# conflict 2 (src/index.ts): keep both import lines — main's agent-targets.js
# import and your readConfigFileSettings import, either order
git add src/commands/test.ts src/index.ts
git rebase --continue
npm run lint && npm run typecheck && npm test
git push --force-with-lease
(swap upstream for whatever remote name points at TestSprite/testsprite-cli in your setup — check with git remote -v)
Still needed before merge — issue #100 explicitly asked for this and it isn't in the diff yet:
- A
CHANGELOG.mdentry under## [Unreleased]for the new config-file precedence. - A new table in
DOCUMENTATION.md's Configuration section (right after "### Profiles & credentials", line 653) documenting the three~/.testsprite/configkeys —endpoint_url,output,project_id— and the flag > env > config-file > default order. While you're there: the "### Environment variables" table (line 671) has no row forTESTSPRITE_CONFIG_FILE, which this PR introduces as the path override — add one.
CI is green, but that run is from 2026-07-23 — before both the v0.5.0 (08-05) and v0.6.0 (08-12) releases, so treat it as informative, not current; the post-rebase run is the real gate.
Rebase, add the two doc pieces, and I'll merge.
2 participants
What does this PR do?
defaultConfigPath()reserved~/.testsprite/configbutloadConfigneverread it, so every non-credential preference had to be repeated per command.
This implements the aws-cli credentials-vs-config split: profile-sectioned INI
settings (
endpoint_url,output,project_id) read via a newreadConfigFileSettings(profile). Precedence is exactly as accepted intriage — flag > env > config file > built-in default:
endpoint_urlslots into theloadConfigcascade just above the built-indefault (below
--endpoint-urlandTESTSPRITE_API_URL);outputbecomes the default of the global--outputflag (flag still wins);project_idbacks--projecton the project-scoped test commands, with thevalidation error now naming both options.
The file is optional by design: missing/unreadable file or section resolves to
{}and falls through.TESTSPRITE_CONFIG_FILEoverrides the path.Related issue
Fixes #100
Type of change
Checklist
mainbranch.npm run lintandnpm run format:checkpass.npm run typecheckpasses.npm testpasses and coverage stays at or above the 80% gate.Notes for reviewers
The INI walk is extracted from
parseCredentialsinto a sharedparseIniFile(null-prototype accumulator +
__proto__/constructor/prototypesection andkey guards);
parseCredentialsdelegates to it, byte-for-byte same behavior —including the recent CR/LF hardening, which is untouched. No secrets ever live
in the config file (the key stays in
credentials). Env-var default for--project(TESTSPRITE_PROJECT_ID) is intentionally NOT included here — thatis issue #76; per its triage, env will slot above the config file.
Carried over from the original review: once #144 (TESTSPRITE_PROJECT_ID
default, also closed by the incident) re-lands, I will fold the config-file
lookup into its
resolveProjectIdhelper as requested by @zeshi-du.Summary by CodeRabbit
New Features
projectandoutputpreferences from the per-profile config file when flags aren’t provided.Bug Fixes
Tests