Skip to content

Add repository rules review bot - #222

Merged
terasakisatoshi merged 1 commit into
mainfrom
add-repository-rules-review-bot
Aug 4, 2026
Merged

Add repository rules review bot#222
terasakisatoshi merged 1 commit into
mainfrom
add-repository-rules-review-bot

Conversation

@terasakisatoshi

Copy link
Copy Markdown
Member

Ports tenferro-rs's delta-scoped REPOSITORY_RULES.md review bot to strided-rs. Part of the environment work tracked in shinaoka/task-management-terasaki#26; scoped to the #199 retirement.

What lands

File Role
.github/workflows/review_bot.yml Four jobs: LLM review, no-LLM skip, waiver, gate
scripts/repository-rules-review.py Diff chunking, secret gate, section routing, DeepSeek call, diff-anchor filtering
scripts/test-repository-rules-review.py 47 tests, no pytest needed
ai/prompts/repository-rules-review.md System prompt
.github/actions/{post-review-comment,verify-review-label}/ Composite actions
scripts/requirements-dev.txt Local python-dotenv

REPOSITORY_RULES.md gains a ## Retired Crate Freeze section, AGENTS.md documents the bot and local usage, and ci.yml gains a job that runs the script's tests.

Security model

Unchanged from tenferro. pull_request_target checks out the trusted base revision; the PR head is fetched for git diff only and never checked out or executed. External-fork PRs are rejected at the gate. Both label escape hatches require the maintain/admin role and reapplication after the latest push. Secret-shaped text in added lines blocks the upload before anything reaches the API.

Adapted for #199

The deterministic boundary check enforces the Phase 0 freeze instead of tenferro's AD boundary. Added implementation lines in strided-einsum2, strided-opteinsum, mdarray-opteinsum, ndarray-opteinsum, or deprecated/ produce a block. Deprecation notices are exempt — doc comments, #[deprecated] and other attributes, README, Cargo.toml — so Phase 0 can land without a waiver. The regex requires a trailing space, slash, or line end on block-comment continuations so *dst = value; is not mistaken for a comment.

Against 9da9b9f (the einsum2 GEMM fix on main), it behaves as intended:

```
Verdict: fail

A carry-list fix that has to protect the current tenferro pin uses rules-review:waive.

Section routing is rebuilt for the retained crates, with paths matching this repo's root-level crate layout. test_every_rule_section_is_reachable fails if a ## section is neither routed, always-on, nor explicitly human-only — so the sections #216 will add cannot go silently unreviewed.

The prompt carries strided-rs cautions: column-major is the default and is not a bug, unsafe in kernel hot paths is expected, and upstream naming and constants from Strided.jl / HPTT are deliberate.

Benchmark rule clarified

A live run surfaced that the reviewer correctly applied "Keep benchmark programs and published benchmark results in strided-rs-benchmark-suite" to a new harness under strided-kernel/benches/. Read literally the rule contradicts the repo's own layout, and it would have blocked every PR adding a regression bench. The rule now:

  • keeps in-tree benches in <crate>/benches/ and says the rule is about location, not harness choice (a first draft made the reviewer flag a hand-rolled timing loop for not being criterion);
  • sends cross-repository comparisons and published results to the suite repo;
  • scopes the performance-table ban to READMEs and rustdoc, explicitly allowing dated worklogs and design records under docs/ to quote measurements as evidence (a first draft banned those too, and the reviewer flagged an existing worklog).

AGENTS.md matches. This overlaps #214, which can now focus on actually moving strided-kernel/README.md:186.

Verification

  • 47 script tests pass; actionlint clean on both workflows
  • Dry runs: retired-crate commit → fail; retained-crate commit → pass; waived → pass; no-LLM → one warn
  • Live DeepSeek runs: fb6794e 1 chunk / 26s / pass; fe9f2cc (3750 lines) 7 chunks / ~460s

After the rule fix, the fe9f2cc run returns one finding, and it is a fair one against a rule this PR did not touch:

[block] strided-kernel/benches/issue_187_uninit_indexed.rs:94 — benchmark uses a single fixed size per operation, and the worklog cites those numbers as parity evidence.

Notes for review

  • timeout-minutes: 20 covers the largest observed run with margin, but a 3750-line PR costs ~8 minutes of wall clock and seven API calls. Worth watching against the DeepSeek off-peak budget already being managed for the BugHunt bots.
  • Requires the DEEPSEEK_API_KEY secret and, optionally, a DEEPSEEK_MODEL variable.
  • The rules-review:waive and rules-review:no-llm labels need creating before the escape hatches work.

Refs #199

🤖 Generated with Claude Code

Port tenferro-rs's delta-scoped REPOSITORY_RULES review to strided-rs.
The workflow runs from the trusted base revision and treats PR contents as
data: the PR head is fetched for `git diff` only, never checked out or
executed. Findings post as a single updating PR comment; only block-severity
findings fail CI.

Adapted for this workspace and for the #199 retirement:

- The deterministic boundary check enforces the Phase 0 freeze instead of
  tenferro's AD boundary. Added implementation lines in strided-einsum2,
  strided-opteinsum, mdarray-opteinsum, ndarray-opteinsum, or deprecated/
  block; deprecation notices (doc comments, attributes, README, Cargo.toml)
  are exempt so Phase 0 can land. The `rules-review:waive` label is the
  escape hatch for a fix that protects the current tenferro pin.
- Section routing is rebuilt for the retained crates and for crates living at
  the repository root. A coverage test fails if a REPOSITORY_RULES section is
  neither routed, always-on, nor explicitly human-only, so the sections added
  by #216 cannot go silently unreviewed.
- The prompt carries strided-rs cautions: column-major default, expected
  unsafe in kernel hot paths, and upstream naming from Strided.jl / HPTT.

Clarify the benchmark rule while porting it. A live run had the reviewer
correctly apply "keep benchmark programs in the benchmark suite" to a new
harness under strided-kernel/benches/, which would block every in-tree
regression bench. The rule now separates in-tree benches from
cross-repository comparisons and published results, says the location rule
is not a harness mandate, and scopes the performance-table ban to READMEs and
rustdoc so dated worklogs may still quote measurements as evidence.
AGENTS.md matches.

The three duplicated github-script blocks in the tenferro workflow become two
composite actions.

Verified: 47 script tests, actionlint clean, dry-run over the waived, no-LLM,
retired-crate, and retained-crate paths, plus live DeepSeek runs before and
after the rule change.

Refs #199

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@terasakisatoshi

Copy link
Copy Markdown
Member Author

Setup status — the two prerequisites in the PR description are now done:

  • DEEPSEEK_API_KEY secret is set.
  • rules-review:waive and rules-review:no-llm labels created.

DEEPSEEK_MODEL is not set as a repository variable; the workflow falls back to deepseek-v4-pro, which is the intended default. Set the variable only to pin or roll back the model without a code change.

Branch protection is deliberately unchanged for now. main currently requires rustfmt, cargo test (ubuntu-latest), and cargo test (macos-latest). Adding review-bot-gate to that list is deferred until one or two real PRs have run, so the false-positive rate and the wall-clock cost can be judged first. Until then a block finding shows as a failed check and a PR comment, but does not gate merge.

Note that pull_request_target resolves the workflow from the base branch, so this PR does not exercise the bot on itself. The first PR opened after merge is the real test.

@terasakisatoshi
terasakisatoshi merged commit 9c165ed into main Aug 4, 2026
6 checks passed
@terasakisatoshi
terasakisatoshi deleted the add-repository-rules-review-bot branch August 4, 2026 01:22
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