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. |
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