Skip to content

test: discover bundled scenarios against the loader's contract - #170

Open
avp9-nexus wants to merge 1 commit into
OWASP:mainfrom
avp9-nexus:patch-1
Open

avp9-nexus wants to merge 1 commit into
OWASP:mainfrom
avp9-nexus:patch-1

Conversation

@avp9-nexus

Copy link
Copy Markdown

Fixes #169

Describe the changes you have made in this PR -

_bundled_scenario_paths() discovered */*.yaml, one directory level and one extension. The loader accepts .yaml and .yml (src/agent_harness/scenario.py:66), so a scenario the harness would happily run could sit outside the parity test's view.

Two changes:

  1. Discovery now matches the loader's contract: both suffixes, any depth under scenarios/.
  2. Discovery asserts it found something. The list feeds @pytest.mark.parametrize, where an empty list produces zero cases and a passing suite. A future directory move could therefore empty the parity control without any signal.

Equivalent on the current tree. 20 .yaml, 0 .yml, all at depth two. The parity test collects 20 cases before and 20 after, so nothing changes today.

Demonstrated to catch what the old discovery missed. With two temporary scenarios added, one as .yml at the same depth and one as .yaml one level deeper, the old glob found 20 files and the new discovery found 22, and the parity test collected 22 cases instead of 20. Both temporary files were removed afterwards.


Code Understanding and AI Usage

Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code or documentation ?

  • No, I wrote all the code myself
  • Yes, I used AI assistance

If you used AI assistance, briefly describe:

  • Tool(s) used: Claude Code
  • Parts of the contribution that were AI-assisted: locating the mismatch between the glob and the loader's suffix check, and drafting the patch and this description
  • How you reviewed the output: checked the claim against the repository itself rather than the model's summary. Enumerated every file under scenarios/, counted extensions and path depths, and read scenario.py and test_scenario_schema_sync.py directly to confirm both the loader contract and the parametrize consumption
  • Tests or checks I ran: full suite on an unmodified clone first as a baseline, then again after the change; ruff check src tests; mypy; and a deliberate check that the new discovery finds files the old one missed, described above

Checklist before requesting a review

  • I have added proper PR title and linked to the issue
  • I have performed a self-review of my code
  • I can explain the purpose of every documentation, function, class, and logic block I added
  • I understand why my changes work and have tested them thoroughly
  • I have considered potential edge cases and how my code handles them
  • If it is a core feature, I have added thorough tests
  • My code follows the project's style guidelines and conventions
  • I added an entry under [Unreleased] in CHANGELOG.md (or this PR does not need one — pure refactor, internal tests, or CI-only change)

Test results, run locally on Python 3.13.15 in a clean virtual environment:

baseline (unmodified clone)   378 passed, 2 skipped
after this change             378 passed, 2 skipped, coverage 93.33%
ruff check src tests          All checks passed
mypy                          Success: no issues found in 16 source files

One caveat stated rather than glossed: CI pins Python 3.11 and no Windows installer is published for it any more, so the above was run on 3.13.15. The project requires >=3.11, and CI will confirm on 3.11.

The unticked box is the conditional one: this changes an existing test's discovery rather than adding a core feature, so no new tests are introduced.

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

LGTM. Verified against main: the loader accepts .yaml and .yml (src/agent_harness/scenario.py:66) while discovery was glob("*/*.yaml") (one level, one suffix), so a runnable scenario could sit outside the parity test's view. Matching discovery to the loader's contract (both suffixes, any depth) is the correct alignment.

The non-empty assertion is the important half: without it, an empty parametrization collects zero cases and the suite reports green while validating nothing. Good to have that guard in place.

Non-blocking follow-up (not needed to merge): consider adding a fixture that exercises the widened discovery directly, a .yml scenario and/or one nested an extra level, so a future regression in _bundled_scenario_paths() fails loudly rather than silently narrowing again. The assertion already covers the zero-collection case; a fixture would additionally prove the both-suffixes/any-depth behavior stays wired.

@avp9-nexus

Copy link
Copy Markdown
Author

Thanks for the independent verification, and agreed on the follow-up. One note on form: the PR body documents that we demonstrated exactly this during testing, with two temporary probes (a .yml at the same depth, a .yaml one level deeper; discovery went 20→22, then the probes were removed). Making that permanent as a bundled fixture would put test scaffolding into the shipped scenario corpus, which feels wrong for a set that ships to users. The clean version is a unit test over a tmp_path scenarios dir asserting both suffixes and nested depth are discovered, next to the existing sync tests. Happy to add it to this PR if maintainers prefer it in one change, or as a follow-up once this lands.

@ossumpossum

Copy link
Copy Markdown

Good call, and I'll walk back the bundled-fixture suggestion. You're right that a real .yml or nested scenario in the shipped corpus is test scaffolding users shouldn't inherit. The tmp_path unit test asserting both suffixes and nested depth, alongside the existing sync tests, is the clean version and covers the regression I was worried about.

In-PR or as a follow-up both work for me; that's the maintainers' call. Approval stands either way.

@avp9-nexus

Copy link
Copy Markdown
Author

A note on the state of this PR rather than on its content: the workflow run was queued on 13 August, waited for maintainer approval, and expired after thirty days without ever running. None of its three jobs started, so there is no check result on the branch either way.

Unlike a run that leaves a required check pending, this one leaves nothing behind: GitHub still reports the branch as clean and mergeable, so no gate is holding it. It is one file, ten lines added and one removed.

What it changes, briefly. _bundled_scenario_paths() discovered scenarios with glob("*/*.yaml"), which matches one directory level and one extension, while the loader accepts .yaml and .yml at any depth. A scenario the harness would load could therefore sit outside what the parity test ever sees. The patch discovers on the loader's contract instead, and asserts that discovery is non-empty, because parametrizing over an empty list yields zero cases and a green suite that covers nothing.

Whenever a maintainer has a minute, merging or approving a fresh run is enough. If it is easier to trigger one from your side, say so and I will rebase and push.

Avp9.

This branch has not been deployed

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

[Bug] scenario parity test discovers a narrower set of files than the loader accepts

2 participants