Skip to content

fix(project): validate missing password files#204

Closed
Lexiie wants to merge 1 commit into
TestSprite:mainfrom
Lexiie:fix/project-password-file-validation
Closed

fix(project): validate missing password files#204
Lexiie wants to merge 1 commit into
TestSprite:mainfrom
Lexiie:fix/project-password-file-validation

Conversation

@Lexiie

@Lexiie Lexiie commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Addresses the --password-file part of #79.

testsprite project create/update --password-file <path> previously read the file directly with readFileSync. If the file was missing or unreadable, the raw filesystem error escaped instead of the CLI's typed VALIDATION_ERROR envelope.

This PR keeps dry-run behavior unchanged, but on real create/update paths converts password-file read failures into exit 5 validation errors before any network work.

Changes

  • Add readPasswordFile() for project create/update password-file reads.
  • Map missing files to VALIDATION_ERROR with field: "password-file".
  • Map permission and other read failures to typed validation errors too.
  • Add regression coverage for create and update ensuring missing password files fail before fetch.

Verification

  • npm run format:check
  • npm run typecheck
  • npx vitest run src/commands/project.test.ts
  • npx eslint src/commands/project.ts src/commands/project.test.ts

Summary by CodeRabbit

  • Bug Fixes
    • Improved validation for --password-file in project create/update flows.
    • Missing, unreadable, or inaccessible password files now fail fast with a clear local validation error instead of continuing.
    • Added coverage to ensure invalid password-file paths are rejected before any network request is made.

@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Adds a readPasswordFile helper in src/commands/project.ts that centralizes --password-file reading, mapping ENOENT/EACCES and other read errors to a local VALIDATION_ERROR with structured details. runCreate and runUpdate now use this helper, and tests verify the new error-handling behavior.

Changes

Password-file validation

Layer / File(s) Summary
readPasswordFile helper and integration
src/commands/project.ts
Adds passwordFileValidationError and readPasswordFile helpers converting file read errors into ApiError(VALIDATION_ERROR) with details.field, path, and reason; runCreate and runUpdate now call readPasswordFile instead of inline readFileSync(...).trim().
Validation tests for create/update commands
src/commands/project.test.ts
Adds tests asserting missing --password-file paths return exit code 5, details.field = password-file, and no fetchImpl call for both runCreate and runUpdate.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • TestSprite/testsprite-cli#27: Both PRs update src/commands/project.ts to convert --password-file read failures (e.g., ENOENT) into local VALIDATION_ERROR/typed validation errors, with accompanying tests.
  • TestSprite/testsprite-cli#139: Both PRs add client-side runCreate/runUpdate local VALIDATION_ERROR guards in src/commands/project.ts with matching test assertions.

Suggested reviewers: ruili-testsprite, zeshi-du

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: validating missing password files in project create/update.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/commands/project.ts (1)

632-659: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

nextAction duplicates the raw reason instead of giving actionable guidance.

passwordFileValidationError sets nextAction: reason, but reason is a low-level diagnostic string (e.g. "file does not exist: /path"), already duplicated in details.reason. Elsewhere in this codebase (ApiError.authRequired), nextAction is used for actionable guidance (e.g. "Run testsprite setup..."), while message carries the specific problem description. Here it's inverted: message is a generic 'Invalid request.' and nextAction restates the low-level reason — this weakens the CLI's error clarity for users hitting this path.

As per path instructions, src/** review should focus on "Correctness and clear error handling."

💡 Suggested fix
 function passwordFileValidationError(path: string, reason: string): ApiError {
   return ApiError.fromEnvelope({
     error: {
       code: 'VALIDATION_ERROR',
-      message: 'Invalid request.',
-      nextAction: reason,
+      message: `Invalid --password-file: ${reason}`,
+      nextAction: 'Verify the file path exists and is readable, or use --password instead.',
       requestId: 'local',
       details: { field: 'password-file', path, reason },
     },
   });
 }
🤖 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/project.ts` around lines 632 - 659, The password file error
handling in passwordFileValidationError is using nextAction for the raw
diagnostic text instead of user guidance. Update passwordFileValidationError and
its callers in readPasswordFile so the specific failure text is placed in the
error message/details, while nextAction contains a short actionable instruction
for the user (similar to ApiError.authRequired). Keep the field reference
password-file and preserve the existing ENOENT/EACCES handling, but make the
response clearer and more user-facing.

Source: Path instructions

🤖 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 `@src/commands/project.ts`:
- Around line 632-659: The password file error handling in
passwordFileValidationError is using nextAction for the raw diagnostic text
instead of user guidance. Update passwordFileValidationError and its callers in
readPasswordFile so the specific failure text is placed in the error
message/details, while nextAction contains a short actionable instruction for
the user (similar to ApiError.authRequired). Keep the field reference
password-file and preserve the existing ENOENT/EACCES handling, but make the
response clearer and more user-facing.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 48c05465-e6ea-46de-9323-82a39b487b6d

📥 Commits

Reviewing files that changed from the base of the PR and between 3305dfa and fd939f9.

📒 Files selected for processing (2)
  • src/commands/project.test.ts
  • src/commands/project.ts

@Lexiie

Lexiie commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Closing this to avoid duplicating the existing password-file guard work in #61. I missed that #61 was still the maintainer-preferred path for the remaining password-file half after #27 merged only the --since overflow guard.

@Lexiie Lexiie closed this Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant