Skip to content

verify/review do not defend against an agent weakening CI or rewriting assertions to pass #75

Description

@Alex1990

What

skills/verify/SKILL.md:10-16 requires the gates to be run (pnpm lint,
pnpm fmt:check, pnpm check:types, pnpm test, pnpm build), and
skills/review/SKILL.md asks Reviewer A whether new tests fail without the change.
But no rule forbids the cheapest path to green: rewriting assertions, deleting or
skipping tests, relaxing lint or coverage configuration, or editing the pinned signer
oracle expectations.

Why it matters

"Agents will also weaken CI to make themselves pass, not maliciously, just gradient
descent finding the cheapest path to green." A green loop PR can therefore mean the
test was edited to match broken behaviour. The signer oracles are protected by a hard
constraint in AGENTS.md, but ordinary tests and the lint/format/type gates are not.

Proposal

  • Reviewer A lens: read the test changes before the code changes; treat a diff that
    rewrites many tests as a red flag.
  • Explicit prohibitions in skills/review/SKILL.md and skills/verify/SKILL.md:
    no deleted or skipped tests, no relaxed lint/coverage config, no assertion edited to
    match new behaviour without a stated reason.
  • Note mutation testing as the check coverage cannot give (coverage proves a line ran;
    mutation testing proves the test would notice if it were wrong).

Norms layer → proposal PR.

Acceptance

  • A PR that changes assertions instead of fixing behaviour is request-changes, with
    the rule cited in the review output.

References

  • Addy Osmani, Agentic Code Review — "Read the test changes more carefully than the code", "Treat CI as the wall that does not move"

Activity

  1. github-actions commented on Sep 22, 2026

    @github-actions
    Contributor

    Loop triage — feature / ready-for-agent (confidence: high)

    Confirmed the gap against the tree, not just the report:

    • skills/verify/SKILL.md:10-16 requires the five gates, and step 2 (:21-23) only says "do not 'fix' by skipping" for an unrelated failure. There is no rule against rewriting an assertion, deleting a test, relaxing a config, or editing an oracle to reach green.
    • skills/review/SKILL.md:34-39 (Reviewer A) asks "do new tests fail without the change?" but sets no test-before-code order and no red flag for a diff that rewrites many tests.
    • The only explicitly pinned checks are the signer oracles (AGENTS.md "Hard constraints"); ordinary specs and the lint/format/type gates carry no such rule. grep -rniE "assertion|\\.skip|coverage|mutation|weaken|delete.*test|relax" skills/ docs/norms/ AGENTS.md scripts/loop/shared/ matches only the branch-ruleset "weaken" wording.
    • The gates are weakenable in-tree: pnpm lint reads .oxlintrc.json, pnpm fmt:check is oxfmt over src test, check:types reads the tsconfig*.json files, and vitest.config.ts:13-16 sets a coverage provider with no thresholds — a green run does not prove the suite would notice a broken behaviour. Mutation testing is not mentioned anywhere.

    Why this is loop work. The fix is a norms-layer proposal: prose in skills/review/SKILL.md (Reviewer A lens + explicit prohibitions) and skills/verify/SKILL.md (the same prohibitions on the maker side), plus a mutation-testing note. AGENTS.md allows norms-layer changes as proposal PRs — the owner's review is the checker and the loop must not merge its own change. No .github/workflows/** edit is needed, so the job token can push. Direct precedent: #69/PR #70, #71/PR #72, #73/PR #74 were all loop-authored norms/host proposal PRs.

    Notes for the implementer.

    1. The host-built reviewer prompt applies the lens "as defined in skills/review/SKILL.md" (scripts/loop/shared/agent.mjs:227), so the change only needs the skill file; mirroring a short version in the reviewer-prompt block (agent.mjs:221-249) is optional emphasis.
    2. The acceptance criterion ("a PR that changes assertions instead of fixing behaviour is request-changes, with the rule cited") is reviewer behaviour, not a host-enforced check. Phrase the prohibitions so violating one is an L1/L2 must-fix finding under Reviewer A, so request-changes follows, and keep the existing two-lens structure and gate list intact.
    3. Type the commit chore(norms) so nothing consumer-visible enters CHANGELOG.md. Suggested branch loop/75-test-integrity.

    Not a duplicate and not needs-info: the issue is owner-authored, fully specified, and no open PR references it. Labelling ready-for-agent.

  2. added
    ready-for-humanRequires a human: implementation, merge of a loop PR, or a release call
    and removed
    ready-for-agentFully specified, an AFK agent may handle it unattended
    on Sep 22, 2026
  3. github-actions commented on Sep 22, 2026

    @github-actions
    Contributor

    Loop feature — PR opened for review ✅

    The gap in #75 was confirmed against the tree during triage: skills/verify/SKILL.md step 2 only forbade "fixing" an unrelated failure by skipping, and skills/review/SKILL.md asked whether tests fail without the change but set no test-before-code order and no red flag for a mass rewrite. The gates are weakenable in-tree (.oxlintrc.json, oxfmt, tsconfig*.json, and vitest.config.ts coverage with no thresholds), and mutation testing was unmentioned.

    This PR adds a shared test-integrity gate to skills/review/SKILL.md — Reviewer A reads the test changes before the code changes, a diff that rewrites/deletes/skips many tests is a red flag, and a changeset that reaches green by deleting/skipping a test, relaxing lint/format/type/coverage/tsconfig config, or editing an assertion to match new behaviour without a stated reason is request-changes (L1) citing the rule, before approving the behaviour it hides. skills/verify/SKILL.md carries the same prohibitions on the maker side plus the coverage-vs-mutation-testing note (coverage proves a line ran, mutation testing proves a test would notice if it were wrong). Signer-oracle expectations stay pinned and never editable.

    Prose only — no source, test, config or workflow file is touched, so the change cannot itself be an instance of the behaviour it forbids. Verification from the runner: pnpm lint 0 warnings/0 errors, pnpm fmt:check clean, pnpm check:types exit 0, pnpm test 24 files / 255 passed / 26 skipped, pnpm build exit 0. Defines a norms-layer change, so it lands as a proposal PR: the owner's approving review and merge are the checker; the loop does not merge its own work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    ready-for-humanRequires a human: implementation, merge of a loop PR, or a release call

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions