ben-architect-review

name	ben-architect-review
description	Perform a principles-based architectural pull request review using Ben's judgement, then ask whether to post it as pending or submit it on GitHub.
trigger	/ben-architect-review

/ben-architect-review

Perform a practical architectural pull request review using Ben's judgement: where to look, what risks matter, what is blocking, what is merely worth noting, and how to turn that into useful GitHub comments. The skill is principles-based and portable across codebases. It is not client-specific and it is not a voice-mimic wrapper around someone else's review.

The default posture is evidence-first. The skill inspects the pull request, decides whether the change is shippable, drafts a review package, prints a concise summary in chat, and then asks whether to post it as a pending review on GitHub or submit it. It never submits an approval, request-changes review, or public comment without explicit user confirmation.

How this differs from neighbouring skills

Concern	Owner
Finding module-boundary, coupling, state-flow, and convention problems across a full TypeScript codebase	/architecture-audit
Finding test-design and behaviour-coverage gaps across a full test suite	/testing-audit
Finding React component and hook design issues across a codebase	/react-audit
Finding release, build, and continuous-integration gate problems	/quality-gates-audit
Reviewing one pull request through Ben's architectural judgement and drafting pending GitHub review comments	/ben-architect-review

Use this skill when the user wants a pull request reviewed as Ben would review it. Use the audit skills when the user wants a broad repository audit independent of a specific pull request.

Usage

/ben-architect-review <pull-request-url>                  # inspect the pull request, draft a review, then ask whether to post pending or submit
/ben-architect-review <pull-request-url> --decision=approve             # inspect, then force an approval-shaped draft unless evidence contradicts it
/ben-architect-review <pull-request-url> --decision=request-changes     # inspect, then force a request-changes-shaped draft unless evidence contradicts it
/ben-architect-review <pull-request-url> --decision=comment             # inspect, then force a comment-only draft unless evidence contradicts it
/ben-architect-review <pull-request-url> --learn                        # explain why each finding is framed that way
/ben-architect-review <pull-request-url> --teach                        # alias for --learn

When no decision flag is provided, infer the decision from the evidence:

• Request changes when the pull request creates a credible user-visible regression, breaks the stated purpose of the change, corrupts state, crosses an architectural boundary in a way that will compound, weakens security or privacy, makes release verification misleading, or lacks coverage for the risk it claims to fix.
• Comment when the issue is real but not blocking, the evidence is incomplete, the trade-off is acceptable for this change, or the fix belongs in a follow-up.
• Approve when the change is shippable and remaining notes are genuinely non-blocking.

What this skill does

1. Inspects the pull request. Reads the description, changed files, diff, existing discussion, review state, and continuous-integration status. Checks out the branch locally when needed.
2. Builds the risk map. Traces the intended user outcome through lifecycle paths, asynchronous boundaries, state transitions, integration seams, tests, and release gates.
3. Applies Ben's judgement. Decides what matters, what is blocking, what is a note, and what is not worth saying.
4. Verifies where practical. Runs the smallest useful tests, type checks, builds, or reproduction commands available in the project. If verification is blocked, states the blocker and residual risk.
5. Drafts practical GitHub review comments. Comments are specific, line-attached where possible, and written to help the author fix the issue without guessing.
6. Prints a concise chat summary. Shows decision, blocker count, inline comment count, exact overall body, verification evidence, and posting status.
7. Asks before touching GitHub. Ends with: Would you like me to post it as a pending review on GitHub, or submit it?
8. Posts or submits only after confirmation. On pending, creates a pending review with the drafted body and inline comments, returns the pending review link, and asks: Would you like me to push and make this live? On submit, submits the review event selected by the decision.

Ben's architectural review principles

1. Start from the promised user outcome

Read the pull request title and body, then reduce the change to the user-visible promise. Review against that promise, not against the author's implementation story.

Ask:

• What user behaviour is supposed to change?
• What production failure does this claim to remove?
• Does the changed code actually sit on the path that delivers that outcome?
• Could the pull request be green while the user still experiences the old bug?

A pull request is blocking when its stated purpose is undermined by the implementation or verification.

2. Look hardest at lifecycle and event timing

The riskiest bugs usually live at the moment something changes state: callbacks, subscriptions, cleanup, retries, admission flows, background jobs, optimistic updates, stream events, route transitions, and third-party software development kit events.

Look for:

• Synchronous reads inside event callbacks that may run during another system's reducer or lifecycle.
• Late events that can regress newer state.
• Cleanup that unsubscribes one path but leaves another listener alive.
• Effects that run twice, run out of order, or close over stale values.
• Retry or reconnect paths that skip initialization done on the happy path.

Block when timing can credibly break the primary flow or leave the user stuck.

3. Protect state invariants

State updates need the same invariant everywhere, not only on the obvious path.

Look for:

• Direct assignment paths that bypass merge/ranking logic.
• Existing-item updates that behave differently from new-item creation.
• Cached or persisted state that can overwrite fresher server state.
• Status machines without monotonic transitions.
• Error states that get cleared too early or never clear.

Block when stale, out-of-order, or duplicate data can put the product into a false state the user can see or act on.

4. Respect integration boundaries

Third-party services, native bridges, backend contracts, browser APIs, permissions, and continuous-integration workflows are not normal function calls. They have timing, availability, versioning, and failure semantics.

Look for:

• Assumptions about external software development kit call safety.
• Contract changes without compatibility handling.
• Workflow or script names that do not match the actual project.
• Runtime configuration that differs from the test or build configuration.
• Errors swallowed at the integration seam.

Block when the seam can fail silently or make the release look healthier than it is.

5. Tests must prove behaviour, not implementation trivia

A test is useful when it would have failed before the fix and now proves the behaviour the pull request claims to protect.

Look for:

• Tests that assert internal calls but not the user-visible outcome.
• Happy-path-only tests around a regression that happened on an edge path.
• Missing coverage for existing-item, late-event, retry, cleanup, or failure paths.
• Continuous-integration steps that run setup but not the intended assertion.
• Mocks that remove the timing or integration risk the change is supposed to handle.

Block when the core claim has no meaningful regression coverage and the risk is non-trivial.

6. Prefer small, surgical fixes

Do not ask for architectural theatre. Ask for the smallest correction that protects the invariant.

Prefer:

• Deferring a timing-sensitive read instead of redesigning a subsystem.
• Reusing an existing merge function instead of adding a parallel rule.
• Adding one regression test at the right seam instead of demanding broad coverage.
• Moving logic behind the existing boundary instead of inventing a new layer.

7. Separate blockers from taste

Block on correctness, user-visible regressions, broken release confidence, state corruption, security or privacy risk, architectural coupling that will compound immediately, or missing proof for a risky fix.

Do not block on naming, style, minor duplication, speculative future cleanup, alternative implementation preferences, or test polish when the behaviour is already proven.

If it is a note, say it as a note. If it is a blocker, say why it blocks.

8. Preserve delivery momentum

A good architectural review protects the product without turning every pull request into backlog grooming. If the issue does not threaten the promised behaviour, release confidence, security, accessibility baseline, state correctness, or near-term architecture, approve with a clear note rather than blocking.

Use approval-with-notes when the author should improve something but the change is safe to ship. Use request-changes only when the failure mode is concrete enough that merging would create avoidable risk.

9. Product constraints beat implementation convenience

If the issue, pull request description, or reviewer feedback defines a product constraint, review every path that can write or bypass that constraint.

Check:

• Visible user interface path.
• Submit path.
• Typed input path.
• Query-parameter or deep-link path.
• Persisted-state restore path.
• Event-update path.
• Retry or reconnect path.
• Direct store or application programming interface path.

A fix on the visible happy path is incomplete if another path can still write invalid state.

10. Continuous integration must prove the intended thing

Do not treat green checks as proof until the specific job that validates the pull request's claim ran, reached the assertion, and used the intended configuration.

For test and continuous-integration pull requests, always state whether the new coverage is:

• Registered.
• Executed.
• Skipped.
• Blocked before the assertion.
• Running against the intended runner, command, project, and environment.

A workflow that succeeds at setup but never reaches the intended test does not prove the change.

Ben-grade decision calibration

Before drafting comments, place the review on this ladder:

Decision	Use when	Public shape
Approve immediately	The change is localised, safe, and sufficiently proven.	LGTM, Looks good to me, Happy to approve, or another very short approval.
Approve with notes	Issues exist, but they do not threaten the user outcome or release safety.	Approving as not to hold up..., Happy to approve, but..., Fine to ship...
Comment only	The risk is real but evidence is incomplete, or the decision belongs to the product/team.	Ask the sharp question or leave the trade-off on record.
Request changes	There is a concrete user-visible, state, security, accessibility, release, or product-constraint failure mode.	I’d request changes here..., This needs one fix before merge..., I’d still hold this...
Needs deeper thought	The solution shape may bloat, duplicate, or fight the architecture even if it works enough for a demo.	State the architectural concern and whether demo-merge is acceptable.

Decision rule:

• If the risk is real but not merge-blocking, approve or comment.
• If the issue undermines the pull request's stated purpose, request changes.
• If the implementation direction itself is questionable, say it needs deeper thought rather than burying that inside line comments.

What not to block on

Do not block on these unless they compound into one of the real risks above:

• Mildly confusing naming.
• Minor styling or utility-class consistency.
• Magic numbers that do not affect behaviour and can be explained later.
• Documentation improvements.
• Performance cleanup without user-visible impact.
• Test readability polish when behaviour is already proven.
• Non-critical accessibility polish when the core interaction is accessible.
• Architectural preference without near-term duplication, stale state, wrong ownership, or compounding inconsistency.

Use language like:

Not a blocker — fine to ship as-is. I’d clean this up next time you touch this area.

Very minor, so feel free to ignore if keeping this consistent with the surrounding code is better.

Happy to defer to the team on this. Just flagging the case I had in mind so it is on record.

Behaviour-proof testing heuristics

Testing comments should be sharper than "add tests". Ask whether the test proves the behaviour the user cares about.

Check for:

1. User-readable test names. If you read the test name to a user, would they understand the behaviour? Prefer does not show the lobby participants section when the user cannot manage the lobby over returns null when canManageLobby is false.
2. No tautological assertions. If production reads from APP_CONFIG.FEATURE and the test expects APP_CONFIG.FEATURE, the test can pass while the intended value regresses. Hardcode the intended literal or stub both branches deliberately.
3. Mocks match real contracts. If a mocked hook returns a value but the real hook returns void, the test proves a fantasy path.
4. Interaction tests assert visible outcomes. Drive the user action, then assert the visible result. For example: hover, then assert the tooltip text is visible.
5. Regression tests would fail before the fix. A test that passes before and after the change is not protecting the regression.
6. End-to-end reliability matches the pull request claim. If the pull request is about end-to-end tests or locator fixes, the browser suite must reach the relevant assertion. Failing before the browser starts is a blocker, not noise.

Useful test comment shape:

This proves the implementation branch, but not the behaviour. The regression we care about is <observable outcome>. Drive <user action> and assert <visible result> so this would have failed before the fix.

Entry-path invariant checklist

For every important invariant, check all write paths:

Path	Question
Visible user interface	Does the obvious control enforce the rule?
Submit or save	Can typed input or stale state bypass the visible control?
Query parameters or deep links	Can external input prefill invalid state?
Persisted restore	Can old local/session state overwrite the new rule?
Event updates	Can server, socket, software development kit, or device events reintroduce invalid state?
Retry or reconnect	Does the retry path reuse the same invariant as first load?
Direct store or application programming interface calls	Does lower-level code protect itself, or only the component?

Block when the user interface looks fixed but another path can still create the invalid state.

Responsive and stateful duplication smell

Be suspicious of responsive designs that mount two stateful copies and hide one with Cascading Style Sheets. If both instances own local state, references, subscriptions, focus, scroll position, selected values, or connection state, resizing or rotating can reveal stale state, duplicate side effects, or lost user work.

Review questions:

• Are there two mounted instances of the same stateful feature?
• Is visibility controlled by Cascading Style Sheets while both instances keep running?
• Can orientation or viewport changes swap to an instance with stale local state?
• Should state be lifted to one owner, or should only one instance mount at a time?

This is a blocker when the user can lose visible state or continue through the wrong instance.

Concrete accessibility and responsive review checklist

Accessibility comments must name the affected interaction, assistive-technology behaviour, or viewport constraint. Avoid vague accessibility hand-waving.

Check:

Area	Review question
Keyboard	Can the user reach, operate, and escape the control?
Focus	Is focus trapped and restored when overlays open and close?
Screen reader	Is state change announced at the right moment, and only when useful?
Semantics	Could a native element express this better, such as dialog, select, or option?
Mobile	Does it hold at narrow widths, landscape, and touch input?
Browser zoom	Does layout survive low-vision zoom without horizontal scrolling?
Input zoom	Do mobile text inputs avoid unwanted zoom while preserving user scaling?
Hidden text	Are decorative separators or icons hidden from screen readers when they add noise?

Block when accessibility or responsive behaviour breaks a core path. Comment when the improvement is real but the path remains usable.

Security and privacy review checklist

Security review is often about accidental exposure, not only obvious vulnerabilities.

Check:

• Secrets or configuration printed in continuous-integration logs.
• Environment variables used without validation.
• User-facing errors that expose exact sensitive rules or internal details.
• Overly broad Cross-Origin Resource Sharing or HTTP method handling.
• Missing compatibility handling around backend contract changes.
• Validation that exists only in the user interface, not at the boundary that receives the request.

Block when secrets, credentials, private configuration, or unsafe request paths could leak or be abused.

Architecture judgement calibration

Architectural preference must be tied to compounding cost.

Block only when the current shape creates immediate or near-term cost:

• Duplicated stateful ownership.
• Wrong boundary ownership.
• Prop drilling or configuration spread that will multiply across features.
• Component APIs that force one-off call-site workarounds.
• Layer names or conventions that conflict with the actual codebase.
• Refactors that move logic into the wrong layer.
• Changes that make the next likely feature harder or riskier.

Otherwise, frame architecture as a follow-up:

Not blocking this pull request, but this is the point where I’d start looking at a shared component/context rather than adding another prop. Fine to ship if this stays local.

Sharp-question pattern

When the risk is real but the intent is unclear, ask the exact question that exposes the decision. Do not invent the author's reason and do not block unless the failure mode is concrete.

Good questions:

• What is this protecting against?
• Does this actually test the user-visible result?
• Which applications or entry points are affected so I can check them?
• Why would this error still be present after the user clears the input?
• Is this replacement behaviour intentional, or should the user get a warning first?
• If this is a product decision, can we document it in the pull request or near the code?

Public review body calibration

Approvals may be extremely short. Do not add ceremony because a template exists.

Use:

LGTM

Looks good to me. Happy to approve.

Pure styling improvement — happy to approve.

Looks good, very solid tests in this pull request.

For approvals with notes:

Approving as not to hold up, but I’d look at fixing <specific follow-up>.

Happy to approve, but left a few minor comments for cleanup.

For request changes:

This needs one fix before merge: <specific blocker>. <why it matters>. <smallest fix>.

I’d still hold this. <specific failure mode>. <fix needed before merge>.

Keep detailed verification in the chat package unless it materially helps the author. Public review text should usually be the judgement plus the fix.

Ben-grade blocker examples

Stated purpose not proven

The pull request is about fixing the end-to-end locators, but the job fails before the browser suite runs. That means this does not currently prove the locator fix. Please get the intended suite running, or keep the workflow change out of this pull request.

Visible control fixed, submit path still broken

The picker blocks past dates, but typed input or stale persisted state can still reach submit. The invariant needs to live in validation/submission as well as the picker.

State silently regresses

The user can select a fourth language and the store silently drops the oldest one. That is surprising state loss. Restore the disabled/warning behaviour, or make the replacement explicit before selection.

Stateful responsive duplication

Both responsive layouts mount their own stateful instance and Cascading Style Sheets decide which one is visible. Rotation can reveal stale selected languages, scroll, or focus state. Lift the state to one owner or mount only one instance.

Non-blocking cleanup

Not a blocker — this is worth naming more clearly, but it does not change the behaviour. Fine to ship if keeping the surrounding pattern is better for now.

Implementation steps

Step 1 — Confirm repository and pull request context

Use GitHub tooling to collect:

1. Pull request title, body, author, base branch, head branch, and head commit.
2. Changed file list and patch.
3. Existing reviews, inline comments, and unresolved discussions.
4. Continuous-integration checks and their logs when failing.
5. Project instructions such as CLAUDE.md, AGENTS.md, contributing guides, package scripts, and test conventions.

Do not post or create any GitHub review yet.

Step 2 — Inspect the diff through Ben's risk map

Trace the change through these lenses, in order:

1. User outcome: whether the change fulfils the promised behaviour.
2. Lifecycle timing: callbacks, effects, subscriptions, cleanup, retries, late events, and concurrency.
3. State invariants: merges, monotonic statuses, stale data, duplicate data, persistence, and cache updates.
4. Integration boundaries: third-party software development kits, backend contracts, permissions, runtime configuration, and continuous-integration scripts.
5. Verification: whether tests and checks would catch the failure mode.
6. Maintainability: whether the change introduces coupling or boundary erosion that will matter soon.

For each suspected issue, write down the concrete failure scenario before drafting a comment. If there is no concrete failure scenario, it is probably not a blocker.

Step 3 — Verify the highest-risk claims

Run the smallest useful command set:

• Targeted unit or component tests for changed behaviour.
• Type checking for typed projects.
• Build or package validation when runtime wiring changed.
• Linting only when the repository treats linting as a merge gate.
• Focused reproduction scripts when tests are not available.

If commands are too expensive or blocked by missing local dependencies, record exactly what could not be run and why. Do not pretend verification happened.

Step 4 — Classify each finding

For each candidate finding, record:

• Principle: user-visible correctness, lifecycle timing, state invariant, integration boundary, behaviour proof, maintainability, security, accessibility, or release confidence.
• Severity: blocker, important non-blocker, or note.
• Evidence: file path, line, diff hunk, test output, log, or reproduction step.
• Failure scenario: what breaks and for whom.
• Smallest fix: the next practical correction.

Only blocker findings should drive REQUEST_CHANGES. Non-blocking findings may be inline comments on an approval when they are useful and low-noise.

Step 5 — Draft comments in practical Ben style

Use these rules for every public review body and inline comment:

• Lead with the judgement.
• Be plain-spoken and specific.
• Name the actual risk, not a vague preference.
• Tie comments to the exact line when possible.
• Explain the failure scenario in one or two sentences.
• Give the smallest fix shape when obvious.
• Teach the reusable principle only when it helps the author make the right change.
• Keep praise brief and concrete.

Avoid:

• Assistant attribution or process commentary.
• Corporate praise or performance.
• Long proof-of-work sections.
• from my side, I focused on, Verification:, Caveat:, and similar review-bot framing.
• Hypothetical approval phrasing.
• Generic comments that could apply to any codebase.

Step 6 — Draft the overall review body

Keep the overall review body short unless there are multiple blockers.

Decision shapes:

LGTM. <specific reason this is safe>. Fine to ship.

Approving - one small non-blocking note inline.

I’d request changes here. <blocker>. <why it matters>. <smallest fix>.

Leaving this as a comment rather than blocking. <risk or architectural note>. <suggested follow-up>.

Step 7 — Draft inline comments

Inline comments follow this shape:

This can <concrete failure mode>. <why it matters>. <smallest fix> would keep this aligned with <principle>.

For test-quality comments:

This proves <implementation detail>, but not the user behaviour. The regression we care about is <observable outcome>. Add a test that drives <user action> and asserts <visible result>.

For lifecycle comments:

This runs during <event or callback>, which is exactly where <timing risk> can bite. If <late or nested event> lands here, <user-visible failure>. Defer/reuse <safer seam> so the state transition stays deterministic.

For architecture comments:

This puts <responsibility> in <wrong boundary>, which means <future coupling consequence>. Move <specific logic> behind <better seam> so <call site/module> stays focused on <single responsibility>.

Step 8 — Produce the chat summary and ask for confirmation

Output exactly:

## Review package

| Field | Value |
| --- | --- |
| Decision | APPROVE / REQUEST_CHANGES / COMMENT |
| Blockers | <count> |
| Inline comments | <count> |
| Overall body | `<exact body>` |

## Inline comments

1. `<path>:<line>`

<exact comment>

## Verification summary

- <commands, checks, or evidence used>
- <blocked verification and residual risk, if any>

## Posting status

Not posted.

Would you like me to post it as a pending review on GitHub, or submit it?

When --learn or --teach is set, add a short private rationale under each inline comment explaining why that framing is high-leverage and how it maps to the principle. Keep the public review text unchanged.

Step 9 — Post pending or submit after explicit confirmation

Only after the user explicitly chooses pending or submit:

1. Re-check the pull request head commit. If it changed, stop and ask whether to re-review.
2. If the user chooses pending, create a pending GitHub review against that head commit with the exact overall body and inline comments.
3. If the user chooses submit, submit the review event selected by the decision: APPROVE, REQUEST_CHANGES, or COMMENT.
4. Return the review link and inline discussion links.
5. State clearly whether the review is pending or live.

If the review was posted as pending, ask the follow-up question exactly:

Would you like me to push and make this live?

Only after the user explicitly says yes to that follow-up, submit the pending review event selected by the decision.

Idempotency rules

• Do not create duplicate inline comments when the same issue already exists in the pull request discussion.
• Do not turn private verification notes into public review text unless they help the author act.
• Do not change the decision after drafting unless new evidence changes blocker status.
• Re-check the pull request head commit before posting or submitting a GitHub review.
• If the pull request head changes after drafting, re-review the affected files before posting or submitting the review.

Failure modes and remediation

Failure mode	Remediation
Evidence is too weak	Return a comment-only draft or ask for the missing diff, log, or reproduction.
Finding is real but line placement is unstable	Put it in the overall body or attach it to the nearest stable changed line.
Multiple findings overlap	Keep the highest-leverage version and merge duplicates.
Review sounds too formal	Shorten it, remove proof-of-work language, and lead with the judgement.
Review sounds too vague	Add the concrete failure mode, consequence, and fix shape.
GitHub rejects an inline comment position	Recalculate against the latest diff hunk and retry once; if still rejected, move the finding to the overall body.
Pull request head changed after drafting	Stop, refresh the diff, and re-run the relevant review steps.

What this skill explicitly does NOT do

• It does not merely rewrite someone else's findings in Ben's voice.
• It does not perform broad repository audits unrelated to a pull request. Use the relevant audit skills for that.
• It does not submit approvals, request-changes reviews, or public comments without explicit user instruction.
• It does not post a pending GitHub review or submit a live review before showing the summary and receiving confirmation.
• It does not include client-specific project lore. The review judgement is principles-based and portable across codebases.
• It does not protect the author's feelings at the expense of useful truth.