Skip to content

fix(test): guard default artifact run id path#172

Merged
zeshi-du merged 4 commits into
TestSprite:mainfrom
Lexiie:fix/artifact-runid-default-path-windows
Jul 5, 2026
Merged

fix(test): guard default artifact run id path#172
zeshi-du merged 4 commits into
TestSprite:mainfrom
Lexiie:fix/artifact-runid-default-path-windows

Conversation

@Lexiie

@Lexiie Lexiie commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #45.

This reopens the artifact default-path guard from #47 on a clean branch based on the latest main, and includes the Windows normalization coverage that made #71 the preferred consolidation target.

test artifact get <run-id> now treats the positional runId as an opaque identifier when computing the implicit default output directory:

./.testsprite/runs/<run-id>/

Unsafe path-like values fail before auth, network, or filesystem work when --out is omitted. Explicit --out <dir> remains the supported way to choose a custom filesystem destination.

Changes

  • Add resolveDefaultArtifactDir(runId, cwd) for default artifact output paths.
  • Reject unsafe default-path run IDs:
    • . / ..
    • Windows trailing-space dot segments like . / ..
    • Windows trailing-period/dot-suffix segments like ..., . ., and .. .
    • / and \
    • NUL bytes
  • Keep normal run IDs under ./.testsprite/runs/<run-id>/.
  • Validate the implicit path before client/auth/fetch work.
  • Add regression coverage for unsafe segments and the documented default path.

Verification

  • npx vitest run src/commands/test.artifact.spec.ts
  • npm run typecheck

Summary by CodeRabbit

  • New Features

    • test artifact get now automatically saves to the default .testsprite/runs/<run-id>/ directory when --out is not provided.
  • Bug Fixes

    • Improved run-id validation: unsafe values (including ./.., traversal attempts, slashes/backslashes, and null bytes) are rejected consistently with a validation error on run-id and exit code 5, before any fetch work begins.
    • Explicit --out validation continues to occur prior to any fetch.
  • Tests

    • Added unit coverage for default output directory resolution and unsafe run-id rejection.

@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6400463d-870e-41cf-9c47-a08f3796c67d

📥 Commits

Reviewing files that changed from the base of the PR and between be4ecd9 and 7a219bc.

📒 Files selected for processing (2)
  • src/commands/test.artifact.spec.ts
  • src/commands/test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/commands/test.ts
  • src/commands/test.artifact.spec.ts

Walkthrough

Adds resolveDefaultArtifactDir in src/commands/test.ts to validate runId before building the default artifact directory, uses it in runArtifactGet when --out is omitted, and adds regression tests for unsafe and safe run IDs.

Changes

Default artifact directory path validation

Layer / File(s) Summary
resolveDefaultArtifactDir helper and runArtifactGet wiring
src/commands/test.ts
Adds exported resolveDefaultArtifactDir(runId, cwd?) validating runId rejects ./.., /, \, and NUL bytes, throwing VALIDATION_ERROR when unsafe; runArtifactGet now uses it for the default output path when --out is absent, and client creation is moved after out-path validation.
Regression tests for default artifact path handling
src/commands/test.artifact.spec.ts
Adds tests verifying runArtifactGet rejects path-like run IDs with VALIDATION_ERROR/exit code 5 before fetch, validates multiple unsafe segments via it.each, and confirms safe run IDs resolve to the documented default directory.

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

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 is concise and accurately summarizes the main fix to default artifact run-id path handling.
Linked Issues check ✅ Passed The changes match the issue: unsafe path-like run IDs are rejected, normal IDs keep the default .testsprite/runs path, and regression tests were added.
Out of Scope Changes check ✅ Passed The PR stays focused on default artifact path validation and related tests, with no clear unrelated additions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@zeshi-du zeshi-du merged commit d72290d into TestSprite:main Jul 5, 2026
9 checks passed
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.

fix(test): prevent path-like run IDs escaping artifact default output dir

2 participants