Skip to content

Add tests for the catalog tooling - #183

Open
vietnamesekid wants to merge 1 commit into
xai-org:mainfrom
vietnamesekid:add-catalog-tool-tests
Open

vietnamesekid wants to merge 1 commit into
xai-org:mainfrom
vietnamesekid:add-catalog-tool-tests

Conversation

@vietnamesekid

Copy link
Copy Markdown

What this PR does

Not a plugin submission, so the plugin and ownership checklist in the template does not apply.

This adds the first tests in the repo: scripts/test_catalog_tools.py, plus one CI step that runs them.

#173 offered to add scripts/test_validate_catalog.py if you wanted it ("just say the word"). This is that file. I widened it a bit to also cover the path containment and parsing helpers in plugin_catalog.py, since those have the same problem.

It is independent of #173. No file overlap, merges cleanly in either order, and it deliberately does not assert the source shapes that #173 changes. More on that below.

Why

validate-catalog.py is the supply chain gate. Its own docstring says what is at stake:

Without a pin, the installer would fall back to git clone --branch <ref> (or HEAD), which means a vendor force-push or repo compromise immediately ships to every user who installs or updates that plugin.

plugin_catalog.py is the other half of it. It fetches arbitrary third party repos at index generation time and guards them with resolve_inside / contained plus a few size caps.

Neither has a single test. Right now every rule in that docstring holds only because the code happens to still be correct, and nothing would tell you if that stopped being true.

What it covers

52 cases, stdlib unittest only. No new dependencies, no network, no git subprocesses, about 0.02s.

Group What it catches
TestShaPinning branch, tag, abbreviated, uppercase and non-string pins, regex anchoring, a future re.IGNORECASE or MULTILINE regression
TestSourcePathContainment monorepo path escaping via .., absolute paths, backslash traversal, empty segments
TestLocalSources vendored plugins staying exempt from pinning
TestLiveCatalog the committed catalog passing its own validator
TestPathHelpers resolve_inside / contained / plugin_root_for_fetch, including absolute component replacement and symlink escape
TestClean control chars and over-long strings reaching plugin-index.json
TestFrontmatter the hand-rolled parser: block scalars, quote handling, the ... terminator
TestLoadJsonFile malformed, missing and oversized manifests from untrusted repos
TestManifestVersion isinstance(True, int) turning "version": true into the string "True"
TestReplaceShaInCatalog the bump bot's regex-over-JSON hitting the wrong entry when two plugins share a sha

Each class has a docstring naming the threat it covers, so the file also reads as a description of what the gate is actually for.

One thing I found but did not fix here

SHA_RE accepts a sha with a trailing newline, so "<40 hex>\n" passes. Python's $ also matches right before a trailing newline. A pin like that gets through validation but can never match git rev-parse HEAD at install time, so it fails late instead of failing in CI. Same thing in plugin_catalog.SHA_RE, which is what the daily bump bot uses.

I left it as an @unittest.expectedFailure so the suite stays green and the gap is still written down somewhere. The fix is \Z instead of $, or .fullmatch, in both places. I kept it out of a tests-only PR on purpose. Happy to send it separately, or fold it in here if you would rather have it in one go.

What I deliberately did not assert

Shapes like {"source": "github", "repo": ...}, bare string URLs, null, numbers and arrays currently return no errors, and #173 changes them to be rejected.

If I asserted today's behavior, this suite would become the thing blocking that fix. So those cases are left out, with a comment in the file pointing at #173. Worth adding once it lands.

Verification

Green against unmodified main. This PR does not touch any source file.

$ python3 -m unittest discover -s scripts -p "test_*.py"
..........................................x.........
----------------------------------------------------------------------
Ran 52 tests in 0.016s

OK (expected failures=1)

$ python3 scripts/validate-catalog.py
Catalog OK (.grok-plugin/marketplace.json)

Paths come from Path(__file__).resolve().parent, so the suite does not care about cwd. I checked it from the repo root, from scripts/, and by running the file directly.

I also wanted to be sure the tests actually fail when something breaks, rather than just passing. So I broke the source on purpose and checked:

What I broke Result
SHA_RE gains re.IGNORECASE 1 failure
containment check removed from resolve_inside 4 failures
bool guard removed from manifest_version 1 failure
size cap removed from load_json_file 1 failure

All reverted afterwards, git diff against the scripts is empty.

That is also how I found the trailing newline thing above.

CI

One step appended to the existing workflow:

      - name: Run tests
        run: python3 -m unittest discover -s scripts -p "test_*.py" -v

-s scripts scopes discovery so it can never pick up a test file from a vendored plugin under external_plugins/.

CONTRIBUTING.md and the PR template both promise to list exactly what CI runs, so they each get the same one line.

Notes for reviewers

  • Nothing new to maintain here. Stdlib unittest, zero dependencies, one file, one CI line, under a second. Nothing imports it.
  • validate-catalog.py and bump-plugin-shas.py are hyphenated so they cannot be imported by name. I load them with importlib.util.spec_from_file_location. Both guard their entry point with if __name__ == "__main__", so importing them does not run main().
  • TestReplaceShaInCatalog patches the module level REPO_ROOT because that function has no injection point. I noted it in the docstring.
  • Happy to trim any of these groups if you would rather keep the surface smaller.

@vietnamesekid
vietnamesekid requested a review from a team August 4, 2026 09:32
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