matrixorigin Review Changes
Review like a maintainer. Report only material issues: correctness, security, data loss, API/contract breakage, concurrency/resource bugs, and meaningful test gaps. Do not comment on style unless it hides a bug.
Hard rule: treat production control flow driven by natural-language text
matching as a correctness issue. Safety, admission, routing, blocking, retry,
recovery, evaluation, and state transitions must use typed fields such as enums,
ErrorKind, result_class, exit_semantics, structured JSON, protocol
parsers, AST/token parsers, or exact machine-owned sentinel fields. Text
matching is acceptable only for UI display/search, rendered-text tests, or
explicitly named legacy fallback parsers that are not the primary decision
path.
Task
$ARGUMENTS
Step 1: Resolve Diff
Start with status and a stat summary:
git status --short
git diff --stat
Target resolution:
| Target | Diff command |
|---|---|
| omitted / unstaged+staged | git diff plus git diff --cached when staged changes exist |
staged |
git diff --cached |
unstaged |
git diff |
branch:<name> |
git diff <name>...HEAD |
commit:<sha> |
git show --stat <sha> then git show <sha> |
commits:<N> |
git log --oneline -N, then git show for every one of the N commits |
pr:<number> / PR URL |
gh pr diff if gh is installed and authenticated; otherwise ask for the diff |
For a commit, take file/add/delete totals from Git's canonical summary:
git show --shortstat --format= <sha>. If a bounded file map needs --numstat,
also pass --format= so commit headers cannot be coerced into numeric totals by
an awk/parser pipeline. Do not replace Git's shortstat with hand-summed raw
git show --numstat output.
If the diff is empty after checking staged and unstaged changes, report "No changes found."
Hard rule for commits:<N>: cover all N commits before presenting a full review,
but inspect them in bounded commit/file sections instead of requesting one
monolithic diff. If only K of N are available, label the review partial before
findings.
If the user requests parallel reviews or multiple reviewers, start one fixed-size
parallel reviewer group immediately after the status/stat/name map. On Astra, use
the skill-exposed agent_fanout directly; do not spend a discovery round searching
for it and do not replace the group with multiple independent agent calls. On
another host, use its canonical parallel delegation capability or disclose that the
capability is unavailable. The parent coordinates coverage and validates results;
it must not perform a duplicate broad review while the reviewers are running.
Read-only reviewers may share immutable source. Use separate worktrees only when a
reviewer may mutate files or reviewers need different refs.
Keep review evidence bounded at the source. Prefer per-file hunks and exact line
ranges over full repository/commit dumps. If a tool returns an artifact reference,
do not pass artifact:// to shell/file tools; rerun the evidence query with a
narrower scope or use the artifact owner supplied by the host.
Step 2: Build Review Map
For broad diffs, group files by owner before reading details:
- Runtime/server:
crates/runtime/ - Services/storage/tasks/journal:
crates/services/ - Turn/tool/prompt core:
crates/astra-turn-*,crates/astra-prompts/,crates/astra-pipeline/ - CLI/TUI:
crates/astra-cli/ - Skills/docs:
.claude/skills/,.agent/skills/, docs - Frontend/SDK:
web/,packages/
Then inspect the highest-risk files first:
- Public API or schema changes.
- Persistence writes, migrations, sync, restore.
- Async/concurrency/cancellation code.
- Auth/permissions/capability/tool visibility.
- Error handling and retry paths.
- Tests that claim coverage for the changed behavior.
For a refactor or subsystem addition, also build an ownership delta:
- Which implementation owned the behavior before and after?
- Did the change add another writer, status vocabulary, registry, observer, parser, table, compatibility path, or retry loop for an existing fact?
- Were replaced implementations and self-only tests deleted?
- Does a real CLI/server/web/edge entrypoint reach the new path?
Treat an unwired implementation, dual writable truth, or a replacement that leaves the old owner active as a correctness finding. Do not recommend another compensating layer until the canonical owner and retirement path are clear.
Step 3: Validate Findings
Before reporting a finding:
- Confirm the changed path is reachable.
- Identify the concrete failure mode and user/system consequence.
- Check whether an existing test would catch it.
- Include file and line, using the new file line when possible.
Reviewer output is candidate evidence, not verified truth. Agreement between reviewers does not raise confidence by itself. The parent must confirm each finding against current source, a reachable failure sequence, and the actual ownership boundary before calling it verified. Stop exploring once every changed ownership boundary has evidence and every reported finding passes this gate.
Do not report:
- speculative risks without a reachable path;
- preferences;
- "consider" suggestions that do not block correctness;
- issues already fixed later in the same diff or commit range.
Output Contract
Self-critique gate: before publishing the final report, re-scan the diff with
git diff --check to catch trailing whitespace and merge-conflict markers that
may have been introduced.
Findings first, ordered by severity:
Critical:
- <file:line> <bug and concrete consequence>
Important:
- <file:line> <bug/test gap and why it matters>
Questions:
- <only blockers to judging correctness>
Summary:
- <one or two sentences on scope>
Tests:
- <tests inspected or missing>
If no material issues are found, say so directly and mention residual risk or unrun tests.