diff --git a/.agents/skills/build-from-issue/SKILL.md b/.agents/skills/build-from-issue/SKILL.md index 16ef5bbd14..aa6acd43d0 100644 --- a/.agents/skills/build-from-issue/SKILL.md +++ b/.agents/skills/build-from-issue/SKILL.md @@ -26,7 +26,7 @@ Use a single issue comment beginning with `> **🏗️ build-plan**` when a plan 1. Check the current branch and working tree. Preserve unrelated work. Create an issue-specific branch or worktree as needed, following Branch Names in `CONTRIBUTING.md`. 2. Implement the smallest coherent change that fulfills the acceptance criteria. Update relevant skills when behavior or commands change. Keep published documentation minimal: explain exactly what users need, avoid duplication across pages, and omit internal details with no user impact. -3. Add meaningful tests for changed behavior and follow the verification guidance in `CONTRIBUTING.md`. Select format, lint, compile or type checks, and tests for affected components and their dependencies. Run the relevant E2E lane for infrastructure, sandbox, or policy changes. Guidance and template edits need applicable Markdown, YAML, link, and consistency checks. Do not require full Rust, SDK, or repository CI solely because a commit or PR is being created; broaden checks only for a concrete remaining risk or failed check. +3. Add meaningful tests for changed behavior and follow the verification guidance in `CONTRIBUTING.md`. Before adding or changing a test, apply the `test-audit` skill's authoring gate so new coverage protects an independent contract instead of mirroring the implementation. Select format, lint, compile or type checks, and tests for affected components and their dependencies. Run the relevant E2E lane for infrastructure, sandbox, or policy changes. Guidance and template edits need applicable Markdown, YAML, link, and consistency checks. Do not require full Rust, SDK, or repository CI solely because a commit or PR is being created; broaden checks only for a concrete remaining risk or failed check. 4. Before pushing, apply the contributor access and issue acceptance check in `create-github-pr`. If blocked, preserve local work and report the reason. Review the diff, use a signed-off Conventional Commit, and prepare the PR. Every PR from this issue-backed workflow must have its own existing issue and use `Closes #` in its Related Issue section. For work needing multiple PRs, split the scope into a closable issue per PR. A high-level issue may track those issues but should not be closed by an incomplete PR. Report the implementation, verification, and any remaining limitation in the PR description, rather than copying earlier issue diagnostics. diff --git a/.agents/skills/review-github-pr/SKILL.md b/.agents/skills/review-github-pr/SKILL.md index d69c891281..f14c1a9fd0 100644 --- a/.agents/skills/review-github-pr/SKILL.md +++ b/.agents/skills/review-github-pr/SKILL.md @@ -154,6 +154,12 @@ Read through the full diff (and the PR description if available). Produce a summ - Do not fabricate concerns or claim a behavioral regression without evidence for both the prior and proposed behavior. - **Agent infrastructure**: When the PR changes behavior, commands, or development workflows, use the `sync-agent-infra` maintenance map to check that related skills were updated. When it adds, removes, or renames skills or crates; changes workflow relationships or skill coverage; modifies issue or PR templates; or changes agent cross-references, apply the full consistency checklist. Report missing companion updates or drift under **Potential Concerns**. +- **Test quality**: When the PR adds or changes tests, apply the `test-audit` + skill's junk-pattern checks to the diff. Report an assertion-free probe, + implementation-coupled test, mock-only coverage of a dependency a real + integration test could exercise cheaply, or near-duplicate coverage under + **Potential Concerns** when it creates a concrete risk, rather than + duplicating `test-audit`'s detailed criteria here. ## Step 5: Output diff --git a/.agents/skills/sync-agent-infra/SKILL.md b/.agents/skills/sync-agent-infra/SKILL.md index 8a07921e62..e17a47cc9f 100644 --- a/.agents/skills/sync-agent-infra/SKILL.md +++ b/.agents/skills/sync-agent-infra/SKILL.md @@ -31,6 +31,7 @@ Run after adding, removing, or renaming a skill or crate; changing issue or PR c | Gator supervision | `launch-openshell-gator` | | SBOM workflow | `sbom` | | RFC workflow | `create-rfc` | +| Test authoring, test-quality gating, or junk-test patterns | `build-from-issue`, `review-github-pr` | Search both `skills/` and `.agents/skills/` for affected commands, fields, and components; the map is a starting point. @@ -43,6 +44,7 @@ Search both `skills/` and `.agents/skills/` for affected commands, fields, and c 5. Check the PR template, `create-github-pr`, `build-from-issue`, `fix-security-issue`, and `CONTRIBUTING.md`: every PR must close an existing issue covering its scope, except automated dependency updates as described in `CONTRIBUTING.md`. Multi-PR work needs an issue per PR; a separate issue may track the overall effort. 6. Check contributor branch instructions and examples against Branch Names in `CONTRIBUTING.md`; AGENTS.md and skills should refer to that convention. Check `README.md`, `.github/ISSUE_TEMPLATE/`, `.github/workflows/`, `.agents/agents/`, and related skill cross references for stale workflow statements. Keep user-facing documentation changes minimal and avoid duplicated internal explanations. 7. Check that PR, build, TUI, and docs contribution guidance follow the scoped verification guidance in `CONTRIBUTING.md`; they must not require full Rust, SDK, or repository CI for changes that cannot affect those areas. -8. Use `npx -y skills add . --list` from a disposable clean copy when skill discovery changes. It should expose public skills only; clean generated files afterward. +8. **Test-audit routing** — `build-from-issue` and `review-github-pr` must route test authoring and review to the `test-audit` skill's authoring gate and junk-pattern checks without copying its detailed criteria. +9. Use `npx -y skills add . --list` from a disposable clean copy when skill discovery changes. It should expose public skills only; clean generated files afterward. Fix contradictions, then repeat the affected checks. Report files changed and any remaining drift. Do not treat an old document's label list as a source of truth over current GitHub metadata. diff --git a/.agents/skills/test-audit/SKILL.md b/.agents/skills/test-audit/SKILL.md new file mode 100644 index 0000000000..bf1637bd82 --- /dev/null +++ b/.agents/skills/test-audit/SKILL.md @@ -0,0 +1,217 @@ +--- +name: test-audit +description: Invoke whenever writing, changing, reviewing, or sweeping tests anywhere in the OpenShell workspace (Rust crates, the e2e and conformance suites, the tmachine harness, and the SDKs). 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. +metadata: + internal: true +--- + +# Test Audit + +Three modes, one value bar. Authoring mode gates every new or changed test at +write time. Audit mode runs focused sweeps for tests that re-assert source, +duplicate stronger proof, couple behavior to implementation, use mocks where a +real dependency is cheap, or keep test-only production seams alive. Campaign +mode prunes one subsystem's whole test surface; treat it as several coherent +audit passes, not one giant edit. + +This skill covers test quality, not coverage gaps. If an audit shows that a +mock-only test stands in for coverage that should exist against a real +dependency, record that as a coverage gap in its own issue (see Landing). Do +not delete the weak test and leave the gap unrecorded. + +## Finding and verifying tests + +Test surfaces differ in harness and failure mode: Rust unit tests with fakes, +e2e tests against a real gateway and driver, the portable conformance suite, the +tmachine VM harness, and each SDK's own runner. Do not rely on a remembered +layout. + +- Discover where a test lives now. List `tests/`, `e2e/`, and each crate's + `tests/`, and read the suite READMEs. RFC 0016 sorts e2e tests into + conformance, feature, driver, and disruption families; use that vocabulary. +- For conformance placement and focused verification commands, use + `tests/suites/conformance/README.md` as the source of truth. Sharing a + contract across drivers is not enough on its own to move it there. +- For test and lint commands, follow the verification guidance in + `CONTRIBUTING.md` and the task definitions under `tasks/` (`mise tasks ls`). +- A test can be well-formed and never run. Check which workflow and task + executes it, its `required-features`, and any environment defaults. Check + exclusion lists and wrapper allowlists too: a test can run in one lane and be + excluded from another, and a nextest archive can only include or exclude whole + test binaries. +- Before calling a fixture, script, or variable unused, read the harness + configuration and playbooks that consume it. A grep over Rust sources misses + consumers elsewhere. + +## Authoring gate + +Before adding any test, answer these. A missing answer means do not add it yet: + +1. What observable behavior, invariant, or independent contract does it + protect? If the feature crosses a real boundary, name the boundary (see + Boundaries). +2. What credible regression makes it fail? +3. Why does existing coverage not already catch that failure? Each contract has + one primary owner at the strongest boundary. Use the conformance README to + decide whether a portable contract belongs in the conformance suite or is + genuinely driver-specific. Another layer needs its own distinct risk, such as + a real daemon that a mock cannot fail the way the real one does. Extend a + table-driven case or shared fixture before writing a near-duplicate. +4. Does it need a production seam (export, flag, wrapper, injection hook) that + no production caller needs? If so, move the test to the real boundary. + +Then check the test against the junk patterns. A match fails the gate unless the +retention bar names the contract it independently guards. A test that breaks +under behavior-preserving refactoring asserts implementation; rewrite it at the +owning boundary first. + +A bug regression test must fail on the pre-fix code for the intended reason and +pass after the owner-boundary repair. Cover the bug once at the owner boundary; +do not replay it at every layer it crosses. + +## Verify the contract, not the implementation + +Ask of every assertion: where did the expected value come from? + +- Traced or run from the implementation itself is circular. It proves the code + matches itself. Rederive it from an independent source or reject the test. +- Derived from a spec, protocol, real dependency, or independently computed + value is valid, because it can fail when the code is wrong. + +For example, a test of a unit-conversion helper should not hardcode what the +helper returns. It should check what a real consumer observes, such as the +resource limit a real container reports. When no ground truth exists, compute +the expectation by a different path than the implementation uses. The same rule +applies to e2e and conformance assertions computed by calling the code under +test a second time. + +## Boundaries + +A bug can live in the handoff between components, not inside either one. +Mocking every collaborator proves each side is correct given its assumptions +about the other, not that the assumptions hold. Name the boundaries a feature +crosses and make sure at least one test crosses each for real: + +- **Process:** does an error surface as the right exit code and message through + the real entry point? +- **Privilege or identity:** does data written by one identity stay readable by + the identity that reads it later? A fixture captured as root but read back by + an unprivileged runner passes every step in isolation and fails the handoff. +- **Declaration to enforcement:** does a value the API stores get enforced + underneath? Proving storage says nothing about enforcement. +- **Our code to an external tool:** does the tool behave as our code assumes? + Check against the tool or its documentation, not our own comments. + +Ask: if every collaborator were mocked, which handoffs would never be +exercised? Those deserve a real dependency. + +## Junk patterns + +The authoring gate rejects a new test that matches one; audits hunt for existing +tests that do. + +| Pattern | Notes | +| --- | --- | +| Assertion-free coverage probes | Runs code, asserts nothing | +| Self-comparisons, identity copiers | Compares a value with itself or a copy | +| Copied fixtures, inventories, manifests, export lists | Duplicates the source it checks | +| Exact source, import, or string greps | Breaks on rename, not on behavior | +| Private predicate or call-shape tests | Duplicated at real boundaries | +| Same contract replayed across unit, e2e, and conformance layers | No distinct failure mode per layer | +| Per-driver replays of a portable contract the conformance suite owns | | +| Tests that only preserve test-only exports, globals, or wrappers | | +| Dead production code whose only callers are tests | | +| Expected values taken from the implementation under test | See "Verify the contract" | +| Mocks that implement the asserted behavior, or one mock standing in for different APIs | | +| A mock standing in for a real dependency that is available and cheap in CI | Flag even if well-formed; the gap is coverage-shaped | +| Fixtures that supply the ordering or receipt the owner should produce, or persistence asserted against a store the path never writes | | +| Capability tests that restate declared flags | Should exercise what the flag promises | +| Negative controls that pass for an unrelated reason | A different guard denied it, the path was never reached, or the environment was never reached | +| Names or fixtures that promise more than the input exercises | A "rootful parity" test that only runs rootless | +| A test that skips or returns early and still passes | Fail with a clear message, or exclude or waive it with a rationale and tracking issue; a green result must never hide a check that did not run | +| A conformance test that changes deployment configuration, depends on launcher-set variables or one provisioning tool, or reaches a public internet service | Belongs in a driver, feature, or disruption suite, or its dependencies come from target preparation (RFC 0016) | +| Assertions on human-formatted CLI output where structured output exists | Use `--output json` or equivalent | +| Retry loops that hide a product race | Waiting for an expected eventual state is fine; retrying a failed assertion until it passes is not | + +## Retention bar + +A test justifies its maintenance cost by protecting behavior, a credible +regression, or an independently meaningful contract. An existing test that must +change for a behavior-preserving reorganization is suspect, not automatically +deletable. The authoring gate still rejects new ones. + +Keep a test that independently enforces a public API or proto contract, a +`ComputeDriver` guarantee, the gateway config schema, a policy contract, a +security boundary, a sandbox lifecycle invariant, an SDK contract, or a CI or +release artifact contract. Also keep: + +- call ordering when order is observable behavior; +- regressions with a credible failure mode; +- source inspection when it is the cheapest independent guard: it fails when the + contract changes (a config key, a wire byte, a CLI flag) and survives an + identifier-only refactor; +- a retained test that fails on the baseline. Treat it as a possible product + bug, reproduce it, and repair the owner rather than deleting it. + +Static or slow is not a deletion reason. A VM-backed suite may be the only real +daemon proof that exists. Prove a test is redundant before removing it. + +## Auditing + +Keep discovery read-only and report evidence before editing. For broad scope, +run parallel discovery lanes when available, one per surface. Outside campaign +mode, prefer a few high-confidence candidates over a large speculative +inventory. + +Before judging a candidate, read the complete test and its production owner, +callers, sibling implementations, overlapping tests at other layers, CI routing, +and history. Read root and scoped `AGENTS.md` files first. When a test claims +dependency-backed behavior, verify it against the real dependency or its +documentation, not the mock. + +Record each of these before editing. A missing field means the candidate is not +ready for deletion: + +- test name and location, and the failure it can actually detect; +- non-test callers of the covered seam; +- the stronger remaining owner-boundary proof, or why none is needed; +- whether it runs in CI at all, and the cost of deleting it versus fixing its + wiring; +- for an excluded, skipped, or ignored test, the rationale and tracking issue + (none is itself a finding); +- why the test or seam exists, and what its removal unlocks; +- risk and the focused validation command. + +Edit one coherent owner-boundary batch at a time: one crate, one SDK, or one +driver's tests. Delete obsolete test-only exports, wrappers, and dead production +paths instead of keeping aliases. Move retained regressions to their canonical +owner. Consolidate per-driver assertions of the same contract into one +conformance scenario only when it meets the conformance README's criteria; an +internal `ComputeDriver` contract stays with the driver. Coordinate with open +migration issues so an audit does not delete or rewrite a test already queued +to move. Prefer net-negative production LOC, and do not add replacement tests +that restate the implementation. + +Validate with the smallest owner and sibling tests first. For e2e tests that +need a real driver and can run locally, build and run them instead of trusting a +compile check, because wiring bugs hide there. Run the language's own format, +lint and pre-commit gates (see `AGENTS.md`) once per coherent batch, not after +every edit, and inspect `git diff --numstat`. + +## Landing + +Commit, push, and PR authority belongs to the invoking workflow and `AGENTS.md`, +not this skill. Use `create-github-pr` for PR mechanics once authorized, with +one coherent PR per audited surface. + +Before filing an issue or opening a PR, search issues with `--state all` and +more than one phrasing, and check open PRs for overlapping files or behavior, +not just titles. Cross-reference adjacent work and narrow scope instead of +duplicating it. A real product gap that an audit surfaces (missing coverage, not +a junk test) belongs in its own issue via `create-github-issue` or +`create-spike`, not in the audit PR. + +Hand off by reporting: the low-value categories removed, by surface; production +simplifications; retained false positives and why they stay; mock-only coverage +findings not fixed here; the proof actually run and what could not be run +locally; production versus test LOC; PR and merge state; and named follow-ups. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2c1e215b4b..ca43352056 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -102,6 +102,7 @@ Contributor and maintainer skills live in `.agents/skills/`. They are marked int | Contributing | `build-from-issue` | Plan and implement work from a GitHub issue (maintainer workflow) | | Contributing | `create-github-issue` | Create well-structured GitHub issues | | Contributing | `create-github-pr` | Create pull requests with proper conventions | +| Contributing | `test-audit` | Gate new tests and audit existing ones for junk patterns and mock-only coverage of real dependencies | | Reviewing | `review-github-pr` | Summarize PR diffs and key design decisions | | Reviewing | `review-security-issue` | Assess security issues for severity and remediation | | Reviewing | `fix-security-issue` | Implement an approved security remediation plan |