Skip to content

fix: validate backtest run_dir CLI input#60

Merged
warren618 merged 2 commits into
HKUDS:mainfrom
BigNounce90:BigNounce90-patch-20260429184210
Apr 29, 2026
Merged

fix: validate backtest run_dir CLI input#60
warren618 merged 2 commits into
HKUDS:mainfrom
BigNounce90:BigNounce90-patch-20260429184210

Conversation

@BigNounce90

Copy link
Copy Markdown
Contributor

Summary

The standalone backtest validation entrypoint accepted sys.argv[1] without checking that it was a usable directory path, which could surface low-level path and file errors to operators. This change adds a small argument parser that rejects empty, malformed, missing, or non-directory run_dir values with clear CLI messages before running validation.

Why it matters

Without this guard, a malformed path such as an embedded-NUL argument or an accidental empty string can produce a raw exception instead of a clear operator-facing error. That makes the failure harder to diagnose and turns a narrow input mistake into an avoidable crash.

Testing

Added focused regression tests for blank, malformed, missing, and valid run_dir arguments. The existing successful path is preserved by asserting that a real directory still parses to the same Path value passed into main before.

@BigNounce90 BigNounce90 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PR #60 direction good. Scope narrow. Failure mode concrete.

Fix before sending:

  • Move new CLI tests under agent/tests/. CI runs pytest from repo root and existing suite lives there.
  • Use from backtest import validation or from backtest.validation import _parse_run_dir. Root-level from agent.backtest import validation breaks repo conventions.
  • Add test for missing argument. Current code changes CLI behavior here but does not lock it down.
  • Add test for non-directory path. PR body claims this case, current tests do not prove it.

Suggested patch file:

  • artifacts/vibe-trading-pr60-improvement.patch

@BigNounce90

Copy link
Copy Markdown
Contributor Author

Updated this PR to address the earlier layout/test concerns:

  • moved the new CLI regression test under agent/tests/
  • aligned imports with the existing test suite (from backtest import validation)
  • added coverage for missing-argument and non-directory path cases
  • hardened embedded-NUL handling so malformed input fails with a clear operator-facing message

Local verification run:

  • python -m py_compile agent\\backtest\\validation.py agent\\tests\\test_validation_cli.py
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 pytest agent\\tests\\test_validation_cli.py -q

@warren618 warren618 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the update 🙂 I re-checked the latest PR head.

The validation CLI now rejects missing, blank, malformed, missing-path, and non-directory run_dir inputs before calling the validation runner. The new regression tests are in the project test tree and match the existing import style.

Verified locally with:

  • pytest agent/tests/test_validation_cli.py -q
  • pytest agent/tests/test_validation.py agent/tests/test_validation_cli.py -q
  • python -m py_compile agent/backtest/validation.py agent/tests/test_validation_cli.py
  • ruff check agent/backtest/validation.py agent/tests/test_validation_cli.py
  • CLI smoke checks for missing args and non-directory paths

No blockers from my side. Approved, thanks for the fix 🙌

@warren618 warren618 merged commit 282c881 into HKUDS:main Apr 29, 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.

2 participants