You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
chore(norms): forbid weakening checks to reach green - #76
A green gate is not proof of correctness: the cheapest path to green for an agent is to change the test, not the behaviour. Issue #75 found that skills/verify/SKILL.md only forbade "fixing" an unrelated failure by skipping, and skills/review/SKILL.md asked whether new tests fail without the change but set no test-before-code order and no red flag for a diff that rewrites many tests. This PR adds that rule to the two norms files: a shared test-integrity gate in skills/review/SKILL.md and the matching maker-side prohibitions in skills/verify/SKILL.md.
What changed
skills/review/SKILL.md — new ## Test-integrity gate (both lenses, before line-level review):
Reviewer A reads the test changes before the code changes; both lenses apply the same rule.
A diff that rewrites, deletes or skips many tests is a red flag: read each one, compare the new assertion with the old, require the stated reason.
Never accept as the way to green: a deleted or skipped test, a relaxed lint/format/type/coverage or tsconfig setting, or an assertion edited to match new behaviour without a stated reason in the PR body. Signer-oracle expectations are pinned and never editable.
Coverage is not this check; mutation testing is the check coverage cannot give.
Such a changeset is request-changes (L1), citing the section and the exact rule broken, before approving the behaviour it hides.
Reviewer A's Tests bullet and the Done when list now reference the gate.
skills/verify/SKILL.md — new ## Never make a gate pass by weakening it: no deleting/skipping/renaming-away tests, no relaxing .oxlintrc.json, .oxfmtrc.json, tsconfig*.json, the vitest.config.ts coverage block or a package.json script, no editing an assertion/snapshot/fixture to match new behaviour without the reason in the PR body, and signer oracles never editable. If the maker believes a test itself is wrong, it says so and leaves the test alone — the reviewer decides. Steps 1-2 now say "never suppressed or configured away" and add "deleting or editing the test".
Prose only: 2 files changed, +50/−4, no source, signer, test, config or .github/workflows/** file touched, so this PR cannot itself be an instance of the behaviour it forbids and the host can push it. scripts/loop/shared/agent.mjs was deliberately left alone: the host-built reviewer prompt already applies each lens "as defined in skills/review/SKILL.md", so the skill prose is what both reviewer processes read.
Proof it works
All five gates run in this checkout, with the actual output:
$ pnpm lint
Found 0 warnings and 0 errors. (90 files, 89 rules)
$ pnpm fmt:check
All matched files use the correct format.
$ pnpm check:types
EXIT=0 (all seven test-type entrypoints; needs dist/ from a prior build)
$ pnpm test # with pnpm serve on :8080
Test Files 24 passed (24)
Tests 255 passed | 26 skipped (281)
$ pnpm build
EXIT=0 (all six d.ts bundles + protocol)
Two environment notes, neither caused by this change: check:types resolves ../dist/tiny-oss.es and so needs a prior pnpm build on a fresh runner; pnpm test needs pnpm serve plus the Playwright chromium headless shell. The 26 skips are the integration specs with no .env credentials, not tests skipped by this change. No test was deleted, skipped or weakened.
Risk tier + AI role
AI role: the agent produced the entire diff — 50 lines of norms prose across two markdown files. No human-written code is included and no host logic changed.
Risk tier: low. Documentation/instructions only; there is no runtime, build or test behaviour to regress. The gates above are green precisely because nothing executable changed.
If it is wrong: the rule is mis-phrased and either over-blocks legitimate assertion updates (behaviour intentionally changed, with the reason in the PR body) or is too vague to cite. The failure mode is reviewer friction, not incorrect software. The signer oracles and the two-lens merge are unchanged.
Review focus
Does the wording keep a legitimate assertion update allowed when the behaviour change is intended and the reason is stated in the PR body, while still making a bare request-changes the outcome for assertion-rewriting? The acceptance criterion for verify/review do not defend against an agent weakening CI or rewriting assertions to pass #75 depends on that distinction reading cleanly.
Is the "many tests" red flag actionable enough to be applied consistently by Reviewer A without becoming noise on legitimate refactors? If it needs a sharper bound, say so before merge.
This is 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.
PR-contract gate (step 0): pass — all four author sections and both identity markers present. Test-integrity gate: pass — the diff is exactly skills/review/SKILL.md (+26/−2) and skills/verify/SKILL.md (+24/−2); no test, snapshot, fixture, vitest.config.ts, .oxlintrc.json, .oxfmtrc.json, tsconfig*.json, package.json or workflow file is touched, so no check was weakened and there is no test diff to read first.
Every claim in the body checks out: file list and +50/−4 match gh pr diff, git diff --check is clean, the named config files exist (vitest.config.ts:13-16 is a real coverage block with no thresholds), and scripts/loop/shared/agent.mjs:227 does render the lens "as defined in skills/review/SKILL.md", so leaving the host prompt alone is defensible. I reproduced pnpm lint (0 warnings / 0 errors, 90 files) and pnpm fmt:check (all matched files correct); CI Lint and Test pass. Issue #75's acceptance criterion — assertion-rewriting to reach green is request-changes with the rule cited — is met, and the existing two-lens structure and five-gate list are intact.
Problems (all L3, none blocking):
[L3] skills/review/SKILL.md:50-52 — the closing paragraph drops the "stated reason in the PR body" qualifier from the bullet above it, so read alone it makes any assertion edit that reaches green an L1 — repeat the qualifier. The bullet (:43-46) keeps a documented, intended behaviour change allowed; the paragraph is the sentence a reviewer will quote in the verdict. Suggested: "...by weakening the check (a deleted or skipped test, a relaxed gate config, or an undocumented assertion change) instead of fixing the behaviour...". This is precisely the distinction your Review focus #1 raises, so it is a wording call for the owner, not a defect.
[L3] skills/review/SKILL.md:41 — "many tests" is unquantified, so Reviewer A will apply the red flag inconsistently — add a bound ("any deleted/skipped test, or more than one rewritten assertion"). Faithful to issue #75's wording, but Review focus #2 is right that it risks noise on a legitimate refactor.
[L3] skills/review/SKILL.md:43-44 / skills/verify/SKILL.md:39-41 — no cited gate reads the changed files.lint = oxlint src test, fmt:check = oxfmt --check src test, check:types = test/*.ts, test = test/**, build = src; all 90 files checked are under src/test. The proof therefore shows "nothing executable broke", not that the prose is well-formed. The body already says exactly that, so this is transparency only; adding skills to the fmt scope could be a follow-up.
[L3] skills/review/SKILL.md:45-46 — the (AGENTS.md "Hard constraints") citation covers a rule AGENTS.md does not word. AGENTS.md pins the signers ("must stay byte-identical to their official SDK") and requires the oracles to pass; it does not itself say the oracle files may never be edited. The rule is wanted (issue #75 lists it) and has the right escape hatch ("state that in the PR body and leave the test alone"), so just phrase it as new norms text citing AGENTS.md as the existing pin. Same paragraph at skills/verify/SKILL.md:42-44; todo at :37 also lacks its receiver forms (it.todo/describe.todo) — cosmetic.
Checked and not findings: no src/signer/dependency/script/workflow change (tree-shaking and signer oracles untouched, chore(norms) keeps CHANGELOG.md clean); no surviving norms text permits the now-forbidden actions; the new gate is reachable by both reviewer processes (skill referenced by the host prompt and read in the opening ritual); all body claims accurate.
Verdict: approve — L3 items are optional polish for the owner's approval pass.
Reviewer B — Standards, safety & maintainability: approve.
No L1 or L2 problems.
PR-contract gate: all four author sections present and non-empty (What/why, Proof it works, Risk tier + AI role, Review focus).
Test-integrity gate: the diff is skills/review/SKILL.md + skills/verify/SKILL.md only (+50/−4); no src/**, test/**, signer, config or .github/workflows/** file is touched, so there is no test diff to read and no gate was weakened.
Standards: no source/entry touched, so AGENTS.md architecture and hard constraints are untouched — and the new prose reaffirms signer-oracle pinning in both files. No secrets, no dangerous patterns, no silent behaviour change; docs are inapplicable (no runtime/options change).
Accuracy verified, not assumed: every filename the verify prohibition names exists and is the real gate config — .oxlintrc.json, .oxfmtrc.json, tsconfig.json (matches tsconfig*.json), vitest.config.ts (has a coverage: block), and the package.json scripts; it.skip/describe.skip/todo matches Vitest's API. The two skills carry the same prohibitions and the same stated-reason carve-out (assertion edits are permitted when the reason is recorded in the PR body), so a legitimate intended-behaviour assertion update is not over-blocked.
Baseline check:pnpm lint (0/0) and pnpm fmt:check green; these gates cover only src test, so a markdown-only diff cannot regress check:types/test/build.
Non-blocking notes (L3, no action required): (1) the "many tests" red flag has no numeric bound — acceptable, since the crisp prohibitions (deleted/skipped test, relaxed config, assertion-without-reason) do not depend on count; (2) "Both lenses apply the same rule" sits on the Reviewer A bullet, but the section heading already scopes the gate to both lenses. Pre-existing and out of scope: AGENTS.md "Quick notes" still omits .oxfmtrc.json from the formatting/lint list.
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
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.
Closes #75
loop-task: #75
What/why
A green gate is not proof of correctness: the cheapest path to green for an agent is to change the test, not the behaviour. Issue #75 found that
skills/verify/SKILL.mdonly forbade "fixing" an unrelated failure by skipping, andskills/review/SKILL.mdasked whether new tests fail without the change but set no test-before-code order and no red flag for a diff that rewrites many tests. This PR adds that rule to the two norms files: a shared test-integrity gate inskills/review/SKILL.mdand the matching maker-side prohibitions inskills/verify/SKILL.md.What changed
skills/review/SKILL.md— new## Test-integrity gate (both lenses, before line-level review):tsconfigsetting, or an assertion edited to match new behaviour without a stated reason in the PR body. Signer-oracle expectations are pinned and never editable.request-changes(L1), citing the section and the exact rule broken, before approving the behaviour it hides.Reviewer A's Tests bullet and the
Done whenlist now reference the gate.skills/verify/SKILL.md— new## Never make a gate pass by weakening it: no deleting/skipping/renaming-away tests, no relaxing.oxlintrc.json,.oxfmtrc.json,tsconfig*.json, thevitest.config.tscoverage block or apackage.jsonscript, no editing an assertion/snapshot/fixture to match new behaviour without the reason in the PR body, and signer oracles never editable. If the maker believes a test itself is wrong, it says so and leaves the test alone — the reviewer decides. Steps 1-2 now say "never suppressed or configured away" and add "deleting or editing the test".Prose only: 2 files changed, +50/−4, no source, signer, test, config or
.github/workflows/**file touched, so this PR cannot itself be an instance of the behaviour it forbids and the host can push it.scripts/loop/shared/agent.mjswas deliberately left alone: the host-built reviewer prompt already applies each lens "as defined inskills/review/SKILL.md", so the skill prose is what both reviewer processes read.Proof it works
All five gates run in this checkout, with the actual output:
Two environment notes, neither caused by this change:
check:typesresolves../dist/tiny-oss.esand so needs a priorpnpm buildon a fresh runner;pnpm testneedspnpm serveplus the Playwright chromium headless shell. The 26 skips are the integration specs with no.envcredentials, not tests skipped by this change. No test was deleted, skipped or weakened.Risk tier + AI role
Review focus
request-changesthe outcome for assertion-rewriting? The acceptance criterion for verify/review do not defend against an agent weakening CI or rewriting assertions to pass #75 depends on that distinction reading cleanly.This is 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.