Quality

Quality Skill

The loop's quality gate: find the bugs, clean up the code. Run after the work is implemented (/context → work → /quality), before /test. It opens the PR as its first step so CI and the review bots run while it reviews, and it acts on their results before it finishes. This skill is correctness + maintainability only — docs, CLI, TUI, and mobile parity are owned by /test's parity passes, so it does not touch them.

Print a one-line phase status as you go (no banner). Update it after each phase:

quality · Phase 1 Thermo Dual-Review → Synthesize + Fix · ACTIVE

Execution Model (parallel by default, teams on Claude)

Both tracks run in parallel. Reviewers return findings (severity + file:line + evidence + proposed fix); they do not apply fixes — the synthesis step owns all edits so dedupe and severity-gating happen in one place.

• Subagent limits. Follow the Subagents rules in AGENTS.md. A track agent is a leaf: it reviews its track itself and does not start parallel reviewers or other subagents. Put that sentence in each track's brief. When the user has asked to limit agents, run both tracks yourself, one after the other, and the delta re-reviews in step 7 are always yours.
• Any runtime — spawn one agent per track; the lead runs synthesis.
• Claude Code with agent teams (CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS=1, already set in .claude/settings.json) — realize the tracks as a team, one teammate per track, lead runs synthesis. Per the global git-worktrees policy, do not pass worktree isolation. Never require a team to run this skill.

Each reviewer receives the same scoped context: git diff "$QUALITY_REVIEW_BASE" plus the full contents of the changed files — including new untracked files, which the tracked diff omits — so it evaluates without guessing.

Setup

Invocation: /quality [feature] [--base <ref>]. --base is the explicit direct-parent binding for a stacked layer. Resolve the base in this order:

1. a validated --base <ref> argument;
2. an existing PR's baseRefName;
3. the current entry's parent from non-interactive gh stack view --json;
4. ADE_REVIEW_BASE_REF from a trusted ship state file;
5. main for the unchanged ordinary workflow.

Do not stop discovery after reading an existing PR. Still inspect gh stack view --json: a current branch present in that stack makes its PR base exact, including a bottom layer based on main. A PR with a non-default base is also an exact direct-parent binding. Only an ordinary unstacked PR targeting the repository default branch keeps QUALITY_EXACT_BASE=false and the historical merge-base behavior. When both PR and stack metadata exist, their parent names and SHAs must agree.

Normalize refs/heads/<name>, refs/remotes/origin/<name>, origin/<name>, and plain <name> to one plain branch name. Reject another remote, symbolic refs, revision syntax (.., ~, ^, :), an empty value, or a name that fails git check-ref-format --branch; never concatenate an unvalidated ref into a command. Fetch the normalized name into its exact remote-tracking ref:

# QUALITY_BASE_REF is the validated, normalized plain branch name selected
# above. QUALITY_EXACT_BASE is true for --base, stack metadata, or trusted
# stack ship state, a non-default PR base, or a PR confirmed in gh-stack;
# ordinary unstacked /quality against the default branch keeps it false.
git check-ref-format --branch "$QUALITY_BASE_REF"
git fetch origin "refs/heads/$QUALITY_BASE_REF:refs/remotes/origin/$QUALITY_BASE_REF"
QUALITY_BASE_SHA=$(git rev-parse "origin/$QUALITY_BASE_REF")
if [ "$QUALITY_EXACT_BASE" = true ]; then
  git merge-base --is-ancestor "$QUALITY_BASE_SHA" HEAD || {
    echo "stack-coordinator-sync-required: direct parent is not an ancestor of HEAD"
    exit 1
  }
  QUALITY_REVIEW_BASE="$QUALITY_BASE_SHA"
else
  QUALITY_REVIEW_BASE=$(git merge-base HEAD "$QUALITY_BASE_SHA")
fi
git diff "$QUALITY_REVIEW_BASE" --name-only
git status --short                 # NEW (untracked) files — git diff omits these
git diff "$QUALITY_REVIEW_BASE" --stat | tail -20
git log "$QUALITY_REVIEW_BASE"..HEAD --oneline

For stack metadata, also require its reported parent SHA to equal QUALITY_BASE_SHA; a name match alone is insufficient. The base must be the direct parent of the current stack entry, not main and not the root of the stack. Record the normalized parent branch, fetched parent SHA, merge-base, reviewed head SHA, and content-tree SHA. If the parent cannot be fetched or sources disagree, stop; silently widening or narrowing a stacked review is not valid evidence. A parent-head or branch change invalidates this result and every result above it in the stack.

Run quality once per layer against its direct parent. For the fifth/top layer, also run both review tracks cumulatively against origin/main; the layer passes only when both the incremental and cumulative gates are empty. Record both bindings. A lower-parent change cascades invalidation through all higher-layer bindings, so the coordinator must sync/rebase the stack and rerun them in order.

A new service or module added but not yet committed will not appear in the tracked diff. Fold the untracked files from git status into the review set and read their full contents — an unreviewed new file is the easiest place for a Blocker to hide.

Windows parity rules

Windows parity is a default requirement, not a conditional check. Every change reviewed here must work on the Windows build of ADE. Windows is part of "done"; it is never a follow-up. Review every changed file for Windows behavior even when the diff looks unrelated to paths or processes — the regressions that ship are the ones nobody thought to look for. references/windows-quirks.md holds the concrete failure classes and the named helper that resolves each one; read it before raising or dismissing a Windows finding.

When parity is not achievable, stop and gate it (Synthesis step 8, reason three). Do not ship a half-working surface and do not quietly gate the whole product. The decision procedure:

1. Name the capability, not the feature. "Native window screenshot capture", not "Computer Use".

2. State the OS-level reason it cannot work on Windows — the missing API, the absent primitive, the security model. "Not implemented yet" is not a parity blocker; that is work you owe.

3. Say what macOS and Linux keep. If they keep it, this is a per-platform divergence the human must approve, not an implementation detail.

4. Present three options with a recommendation:

   • Hidden — the surface does not exist on Windows. Nothing to discover, no explanation given. Right when the capability is not something a user would look for.
   • Disabled with a reason shown — the control is visible, inert, and says why. Right when a user would otherwise hunt for a missing feature, or when macOS docs/screenshots reference it.
   • Removed — deleted from the Windows build entirely, including its code path, settings, and IPC surface. Right when the half-feature carries real cost to keep.

   Hidden and disabled are different user experiences, so the human picks per item — never apply one answer across a batch.

5. State the blast radius: which settings, IPC routes, docs, and tests change under each option.

When the scoped diff touches filesystem paths, process launch, executable resolution, IPC, SQLite/native modules, startup services, or Computer Use, these additional checks apply:

• Treat Windows as a first-class runtime. Verify drive letters, native and mixed separators, UNC paths, quoting, PATHEXT and executable discovery. Audit PowerShell, cmd.exe, and Git Bash invocation separately for argument loss, shell injection, and environment drift. Require process-tree termination, per-user/per-channel named-pipe ACL isolation, Stable/Beta identity isolation, semantic runtime readiness (not merely a live supervisor PID), stale-PID cleanup, bounded supervisor restart/backoff, and packaged native dependencies.
• Trace installer, updater, signing, Windows Firewall, Relay, and capability-gate effects. Verify IPC/preload/shared contracts, CLI/RPC, SQLite/CRR, mobile, hosted web, and release-manifest compatibility rather than treating a native host fix as isolated.
• Require platform gates to state the capability, not infer the whole product is unsupported. Native screenshot/video/OS GUI automation may be blocked on Windows while App Control and proof-file ingestion remain available.
• Trace the same change through macOS and Linux owners and tests. A Windows fix that regresses launchd, Unix sockets, POSIX executable lookup, or graceful Linux capability degradation is a correctness finding.
• Separate code-backed evidence from external proof. Native Windows tests and CI can prove contracts; installed Stable/Beta isolation, second-account pipe denial, clean-host restart, and GUI evidence remain explicit blockers until captured on the corresponding hosts.

Step 0: Open the PR early

Before the review starts, follow Early PR and harvests → Open in docs/playbooks/ship-lane.md: checkpoint commit, push, open the PR ready for review (not a draft), and write the ship state with status: "prepping". Then start Phase 1 immediately — do not wait for CI or the bots.

Skip this step only when the branch is main, the tree holds changes that do not belong to this lane, or the user asked for no PR. Say which one applied. When a PR already exists, push the checkpoint only if HEAD is ahead of the remote and the push rule allows it. When a ship state file already exists (a re-run on a lane in prepping or running), keep its status, iteration, and handled comment ids; the playbook's Open step 5 says which fields change.

Do not push again while the review runs. Each push restarts Greptile's 15–25-minute review.

Phase 1: Thermo Dual-Review → Synthesize + Fix

Track A — Correctness & Security (always runs)

Apply all three reference files:

1. references/correctness-security-review.md — diff-scoped audit for bugs, changes that break existing features (trace cross-app/IPC side effects), devex breakage, and the ADE security surface (computer-use policy & artifact ownership, plaintext secrets, runtime action allowlists, sync/CRR data integrity). Calibrate severity honestly; never present a finding with unfinished research.
2. references/ade-review-rules.md — ADE-specific correctness: runtime-backed null services on bypassed IPC routes, daemon action-domain wiring, cr-sqlite CRR constraints, mobile-host compatibility, IPC/preload/shared/renderer contract drift, fast-tier loading, Node/test-env gotchas, worktree path discipline, and the surface coverage sweep (entry points, clients, providers, reverse states, connection modes — rule 11).
3. references/windows-quirks.md — the Windows failure classes ADE has actually hit and the named helper that resolves each one. Windows parity is a default requirement (see Windows parity rules above), so this file applies to every diff, not only obviously path-or-process work.

Return prioritized findings. Mark each fix unambiguous + behavior-preserving (synthesis may auto-apply) or needs human judgment (synthesis surfaces it).

Track B — Maintainability (always runs)

Apply references/thermo-nuclear-review.md — the 7 structural standards: structural simplification, file-size threshold (1k-line rule), spaghetti prevention, design over acceptance, direct code, type/boundary clarity, canonical layer logic.

For each finding: cite file:line, name the standard, describe the judo move (the smallest change that resolves it structurally), and mark whether it is behavior-preserving. This track is the simplification arm — its applied moves are handled by the synthesis step below, not a separate phase.

UI primitives (whenever the diff touches apps/desktop/src/renderer/**). Run npm run lint:ci in apps/desktop. Every ade-ui/* warning in a file this branch touched is a finding to fix, not just the ones the ratchet fails on. Fix it by moving to the primitive the message names, then run npm run lint:baseline so the lower count is locked in. Never raise the baseline to make a violation pass. Then read the touched UI by eye for hand-rolled notice or dialog styling the lint cannot see: a card that copies the banner look (tone border plus icon tile) instead of <Banner>, a corner card instead of showToast, a custom scrim and panel instead of <Dialog>, a class constant carrying fixed or z-[N], a toast durationMs or banner tone that contradicts docs/design/notices.md. Cite the doc section in the finding.