Add a pytest suite for scripts/ - #106
Open
LibertasSpZ wants to merge 6 commits into
Open
Conversation
anthropics#41 patched two mktemp -t templates in deploy-managed-agent.sh, but test-cookbooks.sh runs that script only under --dry-run, and the second template sits inside upload_skill() past the DRY_RUN early return — reachable only on a live deploy with ANTHROPIC_API_KEY set. Reverting it would leave all 5 cookbooks passing dry-run and fail only in production on Linux. Reading both templates as text covers them equally, with no network, credentials, or particular coreutils. Scope is deliberately static: no extracted mktemp is executed, since that tests coreutils rather than this repo and duplicates test-cookbooks.sh for the one template a dry-run does reach. A canary test fails if fewer than two -t sites parse out of the script, so a restructured script or a broken regex surfaces as a failure instead of an assertion that silently checks nothing. Also folds in the tests/README.md suite proposal from the scaffold commit, and ignores .pytest_cache/. Co-Authored-By: Claude Fable 5 and Claude Opus 5 (1M context) <noreply@anthropic.com>
The scripts resist ordinary importing. scripts/ is not a package and lint-tool-scope.py is not a valid identifier, so modules load by path via importlib. orchestrate.py also imports anthropic at module scope, used only for the client inside run(); conftest satisfies that import with a stand-in that raises on client construction rather than taking the SDK on as a test dependency. Both scripts resolve paths at import time. orchestrate.AUDIT_PATH is relative, so an unredirected rejection test writes out/handoff-audit.jsonl into whatever directory pytest ran from. The orchestrate fixture is the only way to reach the module and always redirects it under tmp_path, reading the records back rather than suppressing them — the raw_len derivation from anthropics#56 is observable nowhere else. Cookbook and handoff fixtures are factories defaulting to the passing artifact, so a test introduces exactly one violation. test_conftest.py covers the fixtures themselves, since the four modules they exist for land over the following weeks and nothing would otherwise execute them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
extract_handoff is where untrusted document text meets the target allowlist and the closed intent schema, so the tests are grouped by the gate they reach, in order: locating the blob, the allowlist, the payload schema, the per-intent parameter schemas, then rendering and logging. The anthropics#56 regression is pinned on raw_len rather than on parse success. A non-greedy regex truncates every real payload and fails loudly — 29 of these tests — but a greedy one still parses most blobs and leaves 59 of 62 green; only the assertion that raw_len equals the decoded object's own length catches it, and that value appears nowhere but the audit record. The pattern rule above HANDOFF_INTENTS — interpolated parameters stay slug-shaped, so a field that looks like an ID cannot smuggle a sentence into the steering prompt — is checked against every field parsed out of HANDOFF_TEMPLATES rather than a hand-listed few, with a canary against the derivation silently checking nothing. One xfail(strict): a JSON array or object in target_agent raises TypeError from the allowlist rather than being rejected, and run() has no handler. Not fixed here — a tests change should not touch the script. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lint-tool-scope.py is the only executable check on the rule that MCP clients, Write and Slack belong to the subagent leaves rather than the orchestrator; the rest of that boundary is prose in READMEs and agent.yaml comments. All four violation classes are covered — the three its docstring names, plus default_config.enabled, which grants the whole toolset implicitly and is enforced but undocumented. The first section runs the linter over the repo's own five cookbooks rather than over fixtures. That is the assertion CLAUDE.md's pre-PR checklist depends on and synthetic data cannot make, and a thin subprocess test covers the documented CLI invocation and its exit code. Three behaviours are pinned as documented rather than endorsed: Slack matching is case-sensitive, toolset types other than agent_toolset* are skipped whole, and a malformed entry inside configs is ignored while a malformed entry in tools is reported. One xfail(strict), four cases: an empty or comment-only agent.yaml makes yaml.safe_load return None and _lint_one raise AttributeError, as do a non-string type and a non-mapping default_config. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_strip_controls and sanitize_event are the two DEFENCE-IN-DEPTH controls, and orchestrate.py is explicit that the denylist is "trivially bypassed" and exists to keep casual noise out of audit logs rather than to stop an attacker. The module is written to that spec, in three sections: what the functions do reliably, why their order matters, and where they give up. Control stripping is tested as the category rule it is — every Cc and Cf codepoint in the BMP, not a list of famous offenders, since such a list would pass equally against an implementation hard-coding the same list. The order test is the one with teeth. Stripping runs before matching, so a zero-width space wedged into IGNORE PREVIOUS cannot split the token out of the pattern's reach. Moving the strip after the line filter looks like a harmless refactor and fails seven cases here. Documented evasions — homoglyphs, digit substitution, rephrasing — are asserted to pass through, so the limit is visible in test names rather than implied by absence. Every invisible character is written as an escape; a fixture that cannot be seen in a diff cannot be reviewed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
validate.py sits between a reader subagent and the orchestrator in the deploy harness, which branches on its exit code, so the contract under test is three integers: 0 valid, 1 invalid, 2 usage. The schemas it validates against are not synthetic. Eleven ship in this repo under output_schema: in the subagent manifests, and the deploy script extracts them. Running the real entry point over the real blocks is what the module is for, and it found one: diligence-grid's extractor declares type: [string, number, null], where YAML's bare null is a null value and JSON Schema wants the string "null". The block fails check_schema, so validate.py dies with an uncaught SchemaError on any extractor output. Marked xfail(strict). Two behaviours are pinned as documented rather than endorsed: _load picks its parser by exact suffix, so .YAML is read as JSON, and a missing file, an unparseable document and an invalid schema all exit 1, the same code an honestly-invalid document produces. Those cases assert only that the run fails and never prints OK, so they keep holding if the handling improves. This completes the six modules the README proposed. Its Running section now covers PEP 668, and its opening claim is corrected: all three pull requests merged from a fork landed in scripts/ (anthropics#41, anthropics#54, anthropics#56), not two of three. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All contributors have signed the CLA ✍️ ✅ |
Author
|
I have read the CLA Document and I hereby sign the CLA |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
scripts/carries the repo's only executable logic — schema validation, cookbook tool-scope linting, and the handoff machinery inorchestrate.py— and currently has no tests. Every pull request merged from a fork so far has landed inscripts/(#41, #54, #56), two of them bug fixes; this suite pins those two down as regression tests and builds coverage around them.Nothing outside
tests/changes, apart from one.gitignoreline for.pytest_cache/.What it covers
test_orchestrate_handoff.pyextract_handoff,_validate_paramstest_orchestrate_sanitize.py_strip_controls,sanitize_eventtest_validate.py_load,main, the shippedoutput_schemablockstest_lint_tool_scope.py_lint_one,maintest_conftest.pytest_shell_static.pybash -n, and everymktemp -ttemplate (#41)213 cases: 206 passing and 7 expected failures, covered below. Branch coverage is 98% for
lint-tool-scope.py, 94% forvalidate.pyand 83% fororchestrate.py; what remains isrun(), which needs a live session, and the__main__blocks, which the subprocess tests exercise out of process.Three things worth flagging
The #56 regression is pinned on
raw_len, not on parse success. A greedy regex still parses most payloads correctly — reverting to one leaves 59 of 62 cases green, and only the assertion thatraw_lenequals the decoded object's own length catches it. That value is observable nowhere but the audit record, which is why the records are read back rather than discarded.Some assertions run against this repo's content rather than fixtures. The five orchestrator
agent.yamlfiles are linted, the eleven subagentoutput_schema:blocks are checked as JSON Schema, and every valid one is driven throughvalidate.py's real entry point. Synthetic data proves a script detects what it looks for; only the real tree proves there is nothing to detect. Two of the three defects below surfaced that way.Where a control is documented as weak, the tests say so out loud.
orchestrate.pycalls its denylist "trivially bypassed... not to stop a motivated attacker," sotest_orchestrate_sanitize.pycarries a section asserting that homoglyphs, digit substitution and rephrasing pass straight through. Absent tests read as an absent weakness. If the denylist is ever hardened, those cases fail and the docstring's claims want revisiting in the same change.Known defects, marked
xfail(strict=True)Writing the suite turned up three. Each is marked with the behaviour it should have, so the suite is green today and goes red the moment the defect is fixed and the marker outlives it. No script or cookbook is edited in this PR — a tests change should not edit what it tests.
extract_handoffraisesTypeErrorrather than rejecting whentarget_agentis a JSON array or object, because the allowlist check hashes the valuediligence-grid/subagents/extractor.yamldeclarestype: [string, number, null], where YAML's barenullis a null value and JSON Schema wants the string"null", so the block failscheck_schema_lint_oneraisesAttributeErroron an empty or comment-onlyagent.yaml, becauseyaml.safe_loadreturnsNonefor oneMerge order matters. Each fix must drop its marker in the same merge, or
XPASS(strict)turns this suite red. If you would rather take the fixes first, say so and I will fold the marker removals into them.Where this fits
CLAUDE.mdkeeps these checks local rather than in CI, so the obvious home forpython3 -m pytest tests/is that validation block andCONTRIBUTING.md's "Run the validators" bullet. Left both files out to keep this PR insidetests/— happy to add the two lines, or a workflow instead, as needed.Running
The only new dependency is
pytest; the scripts already importjsonschemaandpyyaml.orchestrate.pyalso importsanthropicat module scope, used only for the client insiderun()—conftest.pysatisfies that with a stand-in that raises if anything tries to construct a client, rather than taking the SDK on as a test dependency. The venv is because a barepip installis refused under PEP 668 on Debian/Ubuntu/Fedora, and a venv does not inherit distro-packaged modules.Happy to close this if you would rather not carry a test suite, or to cut the scope down if it is more than you want.