Repository navigation
chore(agents): add test-audit skill for contributor test-quality gating - #3697
politerealism wants to merge 3 commits into
Conversation
|
Different layers and classes of tests would allow agents to use this and give humans a reference. |
| 2. What credible regression makes it fail? | ||
| 3. Why does existing coverage not already catch that failure? Each contract | ||
| has one primary test owner at the strongest boundary — usually the | ||
| `crates/openshell-conformance` scenario if the contract applies to more |
There was a problem hiding this comment.
PR #3945 makes tests/suites/conformance/README.md the canonical source for test placement. This rule is broader than that guide: applying to more than one driver is not sufficient for conformance; the contract must also be portable, publicly observable through the CLI/gateway, and runnable as an installed-product black-box test. Please replace this local ownership rule with a concise route to the canonical README so internal ComputeDriver contracts are not moved into conformance merely because multiple drivers share them.
| Kubernetes, VM), feature-gated per driver (`e2e-docker`, `e2e-podman`, | ||
| `e2e-kubernetes*`), driven by shell scripts (`e2e-*.sh`) that stand up the | ||
| actual stack. | ||
| - **`crates/openshell-conformance/src/scenarios/`** — driver-agnostic |
There was a problem hiding this comment.
PR #3945 distinguishes the reusable scenario library from the separately compiled installed-artifact workspace. This inventory omits tests/suites/conformance/, so an audit following it can miss the actual archive tests, and it describes execution through the mise run e2e:* path that is being removed separately. Please list both conformance components and defer their placement and direct Cargo verification commands to tests/suites/conformance/README.md.
| @@ -0,0 +1,365 @@ | |||
| --- | |||
| name: test-audit | |||
| description: Invoke whenever writing, changing, reviewing, or sweeping tests anywhere in the OpenShell workspace (Rust crates, e2e/rust, the conformance suite, tests/ansible tmachine suites, Python, TypeScript SDK, Go SDK). Authoring gate for new tests plus an audit workflow for low-value, implementation-coupled, mock-only, or duplicative tests and the test-only production seams they demand. Trigger keywords - test audit, audit tests, junk tests, low-value test, prune tests, test quality, test coverage gap, mock-only coverage. | |||
There was a problem hiding this comment.
This promises a gate whenever tests are written, changed, or reviewed, but the PR only adds the skill to the human inventory in CONTRIBUTING.md. Please wire test-audit into the sync-agent-infra maintenance map and add concise routing from the applicable build/review workflows, following the discoverability pattern in PR #3945; otherwise repository-aware agents may not discover the gate during normal implementation and review.
|
|
||
| ## Landing and continuation | ||
|
|
||
| Commit, push, open a PR, or land only when authorized — this repo requires |
There was a problem hiding this comment.
This introduces an authorization rule that is not present in AGENTS.md; CLAUDE.md only redirects to that file. Because this skill claims to trigger for every test change, the rule can incorrectly stop an already-authorized build-from-issue workflow before its normal commit/push/PR steps. Please remove this policy and defer commit and push authority to the invoking workflow and repository instructions.
OpenShell had no internal skill for auditing test quality across its five distinct test surfaces (Rust unit tests, e2e/rust, the conformance suite, the tmachine/Ansible harness, and the Python/TypeScript/Go SDKs). Modeled on openclaw's test-audit skill, adapted to this repo's actual tooling (mise tasks, cargo nextest, uv, driver feature gates) instead of Vitest, with two OpenShell-specific additions grounded in lessons from NVIDIA#3663/NVIDIA#3690: a section on deriving expected values from an independent source rather than tracing the implementation itself, and a section on testing the handoff between components (process, privilege, and declaration-to-enforcement boundaries) rather than only what's inside each one. Wired into CONTRIBUTING.md's contributor skills table per sync-agent-infra's maintenance map; no other cross-references needed updating. Signed-off-by: politerealism <burdcat17@gmail.com>
Wire test-audit into build-from-issue and review-github-pr so repo-aware agents discover the authoring gate and junk-pattern checks, and into sync-agent-infra's maintenance map and consistency checklist. Defer conformance test placement and verification commands to tests/suites/conformance/README.md instead of a local ownership rule, and list both conformance components in the surface inventory. Remove the skill's hardcoded git-push authorization rule, deferring commit/push authority to the invoking workflow and AGENTS.md. Signed-off-by: politerealism <burdcat17@gmail.com>
Use the RFC's test families and e2e types as vocabulary, stop telling agents to consolidate internal ComputeDriver contracts into conformance (only portable, publicly observable contracts qualify under tests/suites/conformance/README.md), and add junk patterns for silent skips, conformance tests that change deployment configuration or reach public services, text matching where structured output exists, and retry loops that hide races. Audits now also check the tmachine exclusion list and wrapper test lists, record a rationale and tracking issue for excluded tests, and coordinate with queued migrations. Signed-off-by: politerealism <burdcat17@gmail.com>
d8ed3cf to
f4a811f
Compare
|
@elezar, thanks again for the review. I think the four points were already addressed in the follow-up commit I pushed on October 3 (after your September 30 review), and I've now rebased the branch onto current
I also read the skill against RFC 0016 and your conformance README (#3945) and tightened it in a new commit: it uses the RFC's test families and e2e types, only routes portable, publicly observable contracts to conformance (an internal One thing I'd like your view on: the skill points at |
This branch has not been deployed
2 participants
Summary
Adds
.agents/skills/test-audit, an internal contributor skill for gating new tests and auditing existing ones for junk patterns, mock-only coverage of real dependencies, and low-value duplication across OpenShell's test surfaces (Rust unit tests,e2e/rust, the conformance suite, the tmachine/Ansible harness, and the Python/TypeScript/Go SDKs). Modeled on openclaw's test-audit skill, adapted to this repo's tooling.Related Issue
N/A: internal contributor tooling, no product behavior change. (Precedent: PR #293, which introduced
.agents/skills/andsync-agent-infraitself, used the same "N/A, internal initiative" framing.)Changes
.agents/skills/test-audit/SKILL.md(new): three modes (authoring gate, audit, campaign) built around one value bar: does a test protect an independent contract, not just mirror the implementation. It includes sections on verifying contracts independently of the implementation, on testing what crosses real boundaries (process, privilege or identity, declaration to enforcement, our code to an external tool), and on checking for overlapping issues and PRs before filing.CONTRIBUTING.md: adds the skill to the "Agent Skills for Contributors" table.build-from-issueapplies the authoring gate when adding or changing tests,review-github-prapplies the junk-pattern checks to test diffs, andsync-agent-infralists the skill in its maintenance map and consistency checks.Review feedback addressed (the branch is also rebased onto current
main):tests/suites/conformance/README.mdand notes that sharing a contract across drivers is not enough on its own.crates/openshell-conformance/andtests/suites/conformance/) and defers placement and focused verification to that README. It no longer refers tomise run e2e:*.sync-agent-infraand routed frombuild-from-issueandreview-github-pr.AGENTS.md.RFC 0016 alignment (latest commit): the skill uses the RFC's test families and e2e types as vocabulary and no longer tells agents to consolidate internal
ComputeDrivercontracts into conformance (only portable, publicly observable contracts qualify under the conformance README). It adds junk patterns for tests that skip and still pass, conformance tests that change deployment configuration or reach public services, text matching where structured output exists, and retry loops that hide races. Audits also check the tmachine exclusion list and wrapper test lists, record a rationale and tracking issue for excluded tests, and coordinate with queued migrations.Merge order: the skill refers to
tests/suites/conformance/README.md, which is added by #3945. This PR should land after #3945, or the reference will not resolve onmainuntil it does.Testing
sync-agent-infraconsistency items for a new internal skill are satisfied (name matches directory,metadata.internal: true, listed inCONTRIBUTING.mdand the maintenance map).mise run pre-commitpasses after the rebase onto currentmain(format, all linters, Markdown lint).Checklist