Skip to content

Make test evidence answerable against the CI definition - #148

Merged
tzhouam merged 1 commit into
mainfrom
agent/review-rule-test-evidence
Sep 11, 2026
Merged

tzhouam merged 1 commit into
mainfrom
agent/review-rule-test-evidence

Conversation

@tzhouam

@tzhouam tzhouam commented Sep 10, 2026 •

Copy link
Copy Markdown
Collaborator

Purpose

Owner rule request: when reviewing a PR, check the description's Test Plan / Test Result for (1) whether the CI guard cases covering the changed features/models were actually run, and (2) whether the commands match what CI really executes.

REV-3a in knowledge/general/review/rules.md states the gate. The comparison is answerable rather than a matter of taste, because CI declares both halves at the frozen head:

  • a pipeline step's commands are the commands CI runs;
  • its source_file_dependencies declare which changed paths that step guards;
  • pyproject.toml owns the marker semantics those commands select on.

Equivalence is by selector -- test targets, -m expression, run-level -- never by literal text, so timeout, --cov* and container prefixes are not mismatches. Running a test file directly is explicitly not evidence that the lane collects it: the marker deselects it there.

Coverage: Direct consumers (including /imreview) get it via DIRECT_REVIEW_CHECKLIST; the Strict reviewer gets the repo-shaped form via the injected strict-review-checklist.md. The enforced version for the bot's Direct lane is omni-reviewbot#56.

The injection budget had no room

strict-review-checklist.md was 6,991 characters against a 7,000-character cap (engine/steps/review/steps.py). Anything appended would have been silently cut mid-sentence -- the gate would have looked present in the page while never reaching the model. The cap moves to 8,000 and the page now sits at 7,484.

Three places documented the cap as 4k or 7k while the code said 7,000; they now all say 8k (doc/knowledge/writing.md, doc/features/review-recall.md, the check_wiki_lint.py comment, the test docstring).

SPEC freshness

Touching direct_routing.py and engine/steps/review/ made their SPEC pages stale and failed the --strict gate on the first push. Both are re-verified against the change, and engine/steps/review.md now states the injection cap as a contract, since that is the thing this PR moves.

Test Plan

PYTHONPATH=$PWD/src python -m pytest -q
python knowledge/tools/check_knowledge_tree.py
python knowledge/tools/check_wiki_lint.py

test_review_checklist_resolves_from_adapter_knowledge now pins the budget from both sides: a marker past the old 7k cap must be injected, and a tail past the new 8k cap must not be.

Note for anyone running the suite here: the shared venv has infermatrix_copilot installed editable against another checkout, and this repo's pytest config does not set pythonpath. Without PYTHONPATH=$PWD/src the tests import the other tree and silently pass against unmodified code -- that is how the first run of the budget test looked green-adjacent while the injection was still 7k.

Test Result

Full suite on this branch: 1 failure, test_phase1_cluster.py::test_drift_guard_unimportable_checked_class_fails_guard, identical to origin/main at e40258d3b (verified by running the full suite on a pristine baseline worktree). A test_testing_substrate.py process-reaping test failed once under full-suite load and passes in isolation on both trees; it did not reproduce on rerun.

Knowledge validators: check_knowledge_tree.py 0 errors / 22 pre-existing size reminders; check_wiki_lint.py 0 errors, 0 warnings across 387 pages.

Two ratchets caught a first draft that named the target repo in a core comment (test_repo_neutral_core, test_parity_vocabulary_is_contained); the comment was neutralized rather than the ceilings bumped.

Not covered: no live review was run, so the rule's finding quality on real PRs is unmeasured.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JCk5xJGRyydouK7tuhs8kk

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

A PR body that says tests pass is not evidence that the guard cases CI
runs for the changed feature were executed. The lane selects by marker,
so running a test file directly can prove nothing about what the lane
collects. REV-3a makes that comparison a gate: find the guard steps from
the pipeline definition at the frozen head, compare their commands with
what the PR reports running by selector rather than by literal text, and
treat a missing, unrun or mismatched case as a blocking finding.

The rule reaches every review channel it applies to. Direct consumers get
it through the first-review checklist; the Strict reviewer gets the
repo-shaped form through the injected checklist page.

That injection had no room left. The page was 6,991 characters against a
7,000-character cap, so anything appended would have been cut off
mid-sentence and the gate would have looked present while never reaching
the model. The cap moves to 8,000, and the three places that documented
it as 4k or 7k now agree with the code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JCk5xJGRyydouK7tuhs8kk
@tzhouam
tzhouam force-pushed the agent/review-rule-test-evidence branch from fb5a9c4 to dc0edd5 Compare September 10, 2026 09:31
@tzhouam
tzhouam merged commit 4f55842 into main Sep 11, 2026
3 checks passed
@tzhouam
tzhouam deleted the agent/review-rule-test-evidence branch September 11, 2026 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant