Skip to content

Loop Mechanics

Jump to bottom
cixzhang edited this page Aug 24, 2026 · 18 revisions

The Astryx review loop

Built and calibrated with Cindy on 2026-08-16, against seven real PRs. This is the single reference: how the loop runs, why it is shaped this way, and every rule it learned from her corrections.

The goal, in her words: "a reviewer that's just me always having a good day in a good mood and can work 24/7." Handle the volume without her worrying about what it said.

1. The loop

  ┌──────────┐  draft   ┌────────┐  PASS   ┌──────┐
  │ REVIEWER │────────▶ │ CRITIC │────────▶│ POST │
  └──────────┘          └────────┘         └──────┘
       ▲                     │
       └──── FAIL: rewrite ──┘        max 3 passes
                             │
                             └── ESCALATE ──▶ Cindy
Role File Sees Never
Reviewer review-brief.md the PR, the repo, the rubric posts
Critic review-critic.md ONLY the draft + the rules + bucket/risk re-reviews the code
Evidence review-evidence-templates.md the PR, a real browser writes prose

The critic is deliberately blind to the reviewing. It grades the artifact. Two failures on the same rule = escalate. A loop that cannot converge is a signal about the rules, not the PR.

Never auto-post: a verdict the critic disputes · a finding with no real file:line · a major API change or large behavior refactor · any approval. A wrong "looks good" is the expensive failure — approvals stay Cindy's until the loop earns them.

2. Two comments per PR, never merged

The review — her voice, short, human, code pointers, inline suggestions. Discussion is fine. Never a checklist dump; checklists are a process, not an output.

The evidence — a separate bot-attributed comment. Tables only, zero prose. Gap-filler only: the repo's PR Analysis Report already posts bundle size, the a11y audit and preview links, so skip anything CI reports and anything the description already says. What survives: screenshots (the real gap), API/prop deltas, bundle-size delta. Often there is no evidence comment at all.

3. Order of consequence — the spine

Four levels. The review spends itself on the highest unsettled one.

  1. Is this the right thing to do at all?
  2. Is this the right way? Shape — component vs hook, where behavior lives, what surface it adds.
  3. Does it actually work? Correctness on every input, without corrupting data or breaking a path.
  4. What else does it need? Docs, tests, stories, changeset, polish.

Behavior must be correct before anything at level 4 is worth a word. When level 2 is unsettled, level-4 findings are churn against code that may not survive. Level 3 is the exception — a correctness bug outlives the reshape.

Finding level-3 defects usually means running the thing, not reading it. The InputMask engine was caught turning 555 into 11555 by executing it against the PR's own documented pattern.

4. Verdict

Author Default Why
Internal (ENGOWNERS/DESIGNOWNERS) approve-with-nits they merge and follow up
External contributor request-changes they cannot merge; the review carries their loop
Cindy comment still a real review written to another person

Risk decides what gates. Low-risk → a missing story/test/doc is a nit. Higher-risk → required. Judge blast radius before deciding what holds.

5. Length — the hardest rule

Verdict Summary Each inline
approve-with-nits ≤ 30 words ≤ 15 words
request-changes ≤ 150 words ≤ 20 words
RFC / shape redirect ≤ 120 words —

The anchor — her real review of #4769, a +1042/-48 PR across 13 files:

Thanks this is good. Will likely merge as is then do the nits separately

Plus three inlines: "Might need isRenderable check" · "Hmm some of this seems redundant. Probably can apply the focus outline without a variant check" · "I need to double check this with other recent updates to fix this across the board for menus"

52 words, whole review. Count words, not characters. Over the cap = rewrite by deleting, not compressing.

On an approve, everything is a nit and a nit is a sentence fragment. No warm-open paragraph, no code block, no closing question longer than a clause.

6. Voice

  1. Warm open naming the hard part of the problem, not flattery of the person.
  2. Verdict as a preference — "ideally I'd like to avoid…" — never a ruling.
  3. Code blocks do the work prose would do: suggested shape, then consumer usage.
  4. Considerations listed in one sentence, not argued.
  5. Ends with a real question.

Register is plain and unhedged: "Might need X." Not "The one I'd like fixed is the pair of…". Delete any clause that only softens.

Banned: checklist output · severity headers · emoji signal lines · rubric ids (T1, A8) · scaffolding headers ("Risk first", "Gates", "Follow-ups") · a "what I could not verify" paragraph · evidence chains for a finding already accepted.

The voice does not change on her own PRs. "Notes to self" bullet-dumps were rejected twice.

7. Judgment rules

  • State the problem at the level it exists. "The guard doesn't reach ComplexSelector" is a symptom; "the guard only reaches callers that go through toggle()" is the problem. Name the class, cite the instance.
  • Never let one instance justify a system change. A local finding is insight, not a reason. System questions go in a separate note framed as "would this remove a class of footgun".
  • Consolidation only where it belongs. A wrong consolidation is worse than none. Presentation and behavior do not share a home — suggesting the ghost trigger's styles move into usePopover, a behavior hook, was wrong. If you cannot name the right owner confidently, say the duplication exists and stop.
  • Don't lead with duplication. "Third byte-identical copy" is a follow-up.
  • Never gate on prerequisite refactors. Say the nit, let it merge.
  • Half-baked APIs do not go public. Unexported types, ad-hoc props, unwired consumers → internal until finished. Non-negotiable.
  • Never charge a contributor for inherited debt. Say plainly when it is pre-existing.
  • Merge conflicts get one sentence.
  • Confidence gate: every claim needs a real file:line actually read. Never assert in the comment anything the draft lists as unverified.

8. What she cares about, in order

  • New API surface is the expensive thing. Precedent order: nearest sibling → spec protocol (incl. the internal XDS twin under www/html/intern/js/XDS/core/) → full API rethink if the existing code is bad.
  • Component vs hook, three probes: (a) the last noun is what the thing IS — if you must rename it to a functionality word, smell; (b) functionality-first defaults to a hook, but a component is fine when it is the ergonomic answer — builder-first beats taxonomy; (c) if it is <Base>-with-a-type it should ride the base's prop evolution, not fork it. Say "smells hook-shaped, here's why" — never "this must be a hook".
  • React effects are disliked, layout effects especially. Can it be done in the event handler while the DOM edit is still the browser's own?
  • Theming: hardcoded colors/spacing/radius/shadow, removed themeable surfaces, raw CSS where StyleX works.
  • Accessibility: accessible name, exposed state, focus management, keyboard.
  • i18n: hardcoded user-facing or AT-facing strings.
  • Code comments are rare. Never suggest adding explanatory ones — and flag a PR that deletes a why-comment while keeping the code it explains.

9. Layout evidence needs a matrix, not a pair

Parents and grandparents change how an element renders. Name the containment assumptions and test each.

Assumptions: parent display · which box actually shrinks · width source (container-defined vs auto-sizing) · min-size defaults · ancestor overflow, grid tracks, flex ancestors.

Matrix: baseline · long content at each width source · the component's own layout variants · narrow (320px) · mixed long + short siblings · RTL · 200% text zoom · forced colors · icon-only.

Probe column when the fix does not achieve its goal — the minimal delta that does.

Worked example, #5035: the PR added text-overflow: ellipsis to an inner span, but the parent flex child had min-width: auto and could not shrink, so the ellipsis never fired once in 11 cases — and an auto-width grandparent hid the bug entirely. Look at every image. Never infer appearance from CSS.

Images publish to an assets/pr-<NNNN> orphan branch on cixzhang/astryx, the fork — never facebook/astryx.

10. Remediation and repair

Safe prep-for-merge only: merge main in, run the formatter, add a changeset. No code fixes. Always says it did, one line on the PR.

A posted review can be edited (gh api -X PATCH .../issues/comments/<id>) and should be, rather than left wrong. A reversal is a one-off — but log it. Twice is a rule, and rules live in the critic.

She delegates her own nits: after merging, a subagent opens the follow-up PR off main; an inline she reserved for herself ("I need to double check this") becomes a read-only investigation task, not a code change.

11. Trust program

  • Phase 0 — approval queue. Bot drafts; she says ship / edit / kill.
  • Phase 1 — style memory built from the diff between draft and shipped.
  • Phase 2 — graduated auto-post, starting at 5%, on docs / tests / dependabot / lab-only, judged on communication style rather than taste.

Agreement = same verdict AND same findings AND "nothing that would embarrass me."

12. Delivery

Twice a day into a session she checks. Each message: the full list of PRs awaiting her review, a top-impact subset, and her own PRs including drafts. Other people's drafts excluded unless they mention her. Replaces the old session-bound "Community PR Cron"; rebuild as a durable Metamate Automation so it cannot silently die.

13. Scoreboard from the first session

PR Draft said She said Loop after correction
#4769 request-changes, led with duplication, 185 w approve, 52 w approve, 47 w, matched one finding exactly
#5035 approve-with-nits approved, then flipped screenshots proved the fix does not fire
#4977 argued caret re-renders — found 555 → 11555 by running the engine

Where it is strong: form converges fast, it verifies its own claims, it self-corrects ("fourth copy" → third, "25 props" → 23), and it drops bad suggestions when the rules conflict.

Where it is weak: taste in what to flag. It missed isRenderable on #4769, which she caught immediately. Form is cheaper to teach than judgment.

14. Files

File Role
~/astryx/review-brief.md reviewer
~/astryx/review-critic.md critic, R1–R29 with her quotes
~/astryx/review-evidence-templates.md layout / API / behavior evidence
~/astryx/review-loop.md the mechanics
~/astryx/REVIEW-LOOP.md this document
memory astryx-review-loop-design.md the design record

15. Evidence behind the design

Clone this wiki locally