PRs: apply the quality findings, and make /quality require it

.agents/skills/quality/SKILL.md

@@ -4,8 +4,10 @@ description: >-
   Make the code correct, clean, and current. A thermo dual-review: a
   correctness/security track and a maintainability/code-judo track run in
   parallel, then a synthesis step dedupes, severity-ranks (Blocker/High/Medium/
-  Low), verifies each finding against the real code, auto-applies the safe
-  behavior-preserving fixes, re-reviews, and gates the rest. Grounded in ADE's
+  Low), verifies each finding against the real code, and FIXES EVERY VERIFIED
+  FINDING at any severity — re-reviewing until clean. Only findings needing a
+  product decision, or a behavior change this branch was not asked to make,
+  reach the merge-blocking gate. Grounded in ADE's
   own bug classes (runtime-backed null services, daemon action-domain wiring,
   cr-sqlite CRR, IPC contract drift, fast-tier loading).
 ---
@@ -107,15 +109,35 @@ are handled by the synthesis step below, not a separate phase.
 5. **Sweep the bug class.** When an accepted finding is a repeated pattern, scan
    the diff scope for sibling instances and fix them together — stop at touched
    surfaces and owner boundaries; no refactor beyond the class.
-6. **Apply** the fixes that are **unambiguous and behavior-preserving** — safe
-   correctness fixes and Track B judo moves. Every applied change must be
-   verifiable by reading the diff; do not change behavior.
+6. **Apply every finding you accepted in step 4 — all of them, whatever the
+   severity.** Verified means valid; valid means fix it. Medium and Low are not a
+   backlog, and "behavior-preserving" describes *how* you apply a fix, not which
+   findings earn one. This is the entire point of the skill: a run that surfaces
+   real problems and leaves them in the code has cost the user tokens and
+   returned nothing.
+
+   Fix correctness findings and Track B judo moves alike. If a fix is genuinely
+   large (a multi-file extraction, a schema migration), it is still yours to do —
+   do it here, in this run, not "as a follow-up".
 7. **Re-review until clean.** If step 6 changed code, re-run Track A on the *new*
    diff. New accepted findings → verify (4), apply (6), re-check. Stop when a
-   pass yields no new accepted findings (cap 2 extra passes; anything still open
-   goes to the gate). Catches fix-induced regressions before `/test` or `/ship`.
-8. **Gate** — do NOT auto-apply Blockers or judgment-call findings. Surface them
-   in the report for the author and for `/ship` to gate the merge on.
+   pass yields no new accepted findings. A re-review count is never a reason to
+   defer a verified finding or move it to the gate. This catches fix-induced
+   regressions before `/test` or `/ship`.
+8. **Gate — the narrow exception, not the escape hatch.** Only two kinds of
+   accepted finding may go to the gate unfixed:
+   - it needs a **product decision you cannot make** (which of two valid
+     behaviors the user wants), or
+   - the fix is **not behavior-preserving** and changing behavior is not what
+     this branch was asked to do.
+
+   "Structural", "large", "risky", "pre-existing", "out of scope for this PR",
+   and "worth doing deliberately" are **not** gate reasons — those are fixes you
+   owe. If you gate a finding, the report must say which of the two reasons
+   applies and what decision you need. Anything in the Gate table blocks the
+   merge until the author resolves it; `/ship` treats a non-empty gate as a stop.
+
+   A finding you neither fixed nor gated is a bug in your run.
 9. **Reconcile (optional)** — if a PR exists, *after* the independent audit, read
    the PR discussion and review-bot comments (`gh pr view --comments`, or the
    `ade-pr-workflows` skill). ADE's review bots are `@copilot` (first push) and
@@ -126,8 +148,9 @@ are handled by the synthesis step below, not a separate phase.
 
 ## Completion
 
-Output a summary. The **Gate** section is what `/test` and `/ship` consume — list
-every Blocker and High finding that was NOT auto-fixed.
+Output a summary. The **Gate** section is what `/test` and `/ship` consume, and a
+non-empty gate blocks the merge. List only findings you could not fix for one of
+the two permitted reasons — not findings you chose to defer.
 
 ```markdown
 ## Quality Summary
@@ -137,11 +160,21 @@ every Blocker and High finding that was NOT auto-fixed.
 - Auto-applied: [count] (safe correctness fixes + structural judo moves)
 - Re-review passes: [n]
 
-### Gate (not auto-fixed — for /test regression targets and /ship merge gate)
-| Severity | file:line | Finding | Why not auto-fixed |
-|---|---|---|---|
-| Blocker | ... | ... | needs human judgment / product intent |
-| High | ... | ... | ... |
+### Gate (MERGE-BLOCKING — every row needs an author decision)
+Only two reasons belong here: a product decision you cannot make, or a fix that
+is not behavior-preserving on a branch that was not asked to change behavior.
+Empty is the expected outcome. "Structural / large / out of scope" is not a
+gate reason — those get fixed above.
 
-Next: /test (turn each Blocker/High above into a named regression test).
+When empty, print `- Empty.` and omit the table. Never leave an example or
+placeholder row that another skill could mistake for a live gate.
+
+| Severity | file:line | Finding | Which gate reason | Decision needed |
+|---|---|---|---|---|
+| Blocker | ... | ... | product decision | which behavior do you want? |
+
+Next: /test (turn each Blocker/High into a named regression test).
+
+**Before you print this:** every accepted finding is either in "Auto-applied" or
+in this table. If one is in neither, go back to step 6 and fix it.
 ```

.agents/skills/ship/SKILL.md

@@ -38,6 +38,23 @@ The playbook's Phase 0 is **commit → push → open PR** only. Test generation
 the local-CI gate are NOT part of ship — that's `/test` (and optionally
 `/finalize`) before you reach this skill.
 
+## Precondition: the `/quality` gate must be empty
+
+Before Phase 0, and again before the Phase 3c merge, check for an open
+`/quality` gate. A non-empty gate **blocks the merge** — every row in it is a
+finding that was verified as real and left unfixed, and by `/quality`'s contract
+the only two things that may be there are a product decision the author owes, or
+a behavior change this branch was not asked to make. Both need the author.
+
+- Gate rows exist → do not merge. Surface them, state the decision needed, and
+  stop with `blocked`. Do not merge and mention them afterwards.
+- If `/quality` was never run on this lane, or its final gate result is not
+  available in the lane handoff, stop with `blocked`; unknown is not empty.
+
+Severity is irrelevant here: a Medium in the gate blocks exactly as hard as a
+Blocker, because presence in the gate means it needed a human, not that it was
+minor.
+
 ---
 
 ## Execution Mode: Autonomous
@@ -184,7 +201,7 @@ self-resume signal. Either:
 |--------|---------|
 | `done-clean` | PR merged on main |
 | `done-max` | 5 normal + 1 force-finalize exhausted, merge genuinely blocked |
-| `blocked` | Unrecoverable conflict, gate failure, API error, or force-finalize CI failed |
+| `blocked` | Unrecoverable conflict, gate failure, API error, force-finalize CI failed, or a non-empty `/quality` gate awaiting an author decision |
 
 Always print the final summary (PR, branch, iterations, status, reason,
 per-iteration log, unaddressed items) on exit. Do NOT schedule a wake when

.agents/skills/test/SKILL.md

@@ -1,6 +1,6 @@
 ---
 name: test
-description: 'Prove the new code works: enforce the logging/PostHog ground truth, prune dead tests, consolidate fragments, add only tests that prove new contracts, turn each /quality Blocker/High finding into a named regression test, then run CI-mirrored shards. Also keeps docs/mobile/CLI/TUI parity in lockstep.'
+description: 'Prove the new code works: enforce the logging/PostHog ground truth, prune dead tests, consolidate fragments, add only tests that prove new contracts, turn accepted /quality correctness findings into named regression tests, then run CI-mirrored shards. Also keeps docs/mobile/CLI/TUI parity in lockstep.'
 ---
 
 # /test — Test Suite Steward
@@ -17,7 +17,15 @@ The suite has bloated for three reasons. You exist to fight all three:
 
 Every run does three passes in this order: **PRUNE → CONSOLIDATE → ADD**. You may finish at any pass — adding is optional.
 
-**Consume the `/quality` gate.** If `/quality` ran on this lane, take its Summary's **Gate** section — every Blocker/High it surfaced but did not auto-fix. Each is a named regression-test target for the ADD pass: a test that fails on the bug and passes once it's fixed. A finding isn't "handled" until a test pins it. No gate available → derive the same targets from the diff.
+**Consume the `/quality` result.** A non-empty `/quality` gate blocks this skill:
+the branch still contains a verified finding awaiting an author decision, and
+committing a knowingly failing test is not a substitute for fixing it. Resume
+after the decision and the corresponding `/quality` fix. From the completed
+quality summary, turn accepted correctness findings into named regression-test
+targets when a test can pin the public contract. Structural maintainability
+findings do not require artificial tests when existing coverage already proves
+the behavior-preserving move. No quality result available → derive the same
+targets from the diff and state that quality evidence was unavailable.
 
 **Run the way CI would.** After the suite work, run only the affected shards, never the full suite (that's `/finalize`'s local gate and `/ship`'s remote CI). Verify every new/edited test file matches a vitest workspace glob so CI actually picks it up.
 

apps/desktop/src/main/services/prs/prService.ts

@@ -1078,6 +1078,46 @@ function normalizeMergeMethod(value: unknown): MergeMethod | null {
   return value === "squash" || value === "merge" || value === "rebase" ? value : null;
 }
 
+/**
+ * Resolve the lane columns of a list row.
+ *
+ * A detached row reports NO live lane (`linkedLaneId`/`linkedLaneName` stay null) and
+ * carries `detached` instead, so the renderer shows it as history rather than as a
+ * mapping it could act on. A missing lane row yields a null name, never the raw lane
+ * UUID — a bare id in the list is worse than no chip at all.
+ */
+function deriveGithubSnapshotLaneLink(
+  linked: PullRequestRow | null,
+  laneById: Map<string, LaneSummary> | undefined,
+): Pick<GitHubPrListItem, "linkedLaneId" | "linkedLaneName" | "detached"> {
+  if (!linked) return { linkedLaneId: null, linkedLaneName: null, detached: null };
+  const detached = rowDetachedLane(linked);
+  if (detached) return { linkedLaneId: null, linkedLaneName: null, detached };
+  return {
+    linkedLaneId: linked.lane_id,
+    linkedLaneName: laneById?.get(linked.lane_id)?.name ?? null,
+    detached: null,
+  };
+}
+
+/** Merge outcome + size, so a merged row can describe itself without a GitHub call. */
+function deriveGithubSnapshotMergeFacts(
+  linked: PullRequestRow | null,
+): Pick<
+  GitHubPrListItem,
+  "mergedAt" | "mergedBy" | "mergeMethod" | "additions" | "deletions" | "commitCount" | "changedFiles"
+> {
+  return {
+    mergedAt: linked?.merged_at ?? null,
+    mergedBy: linked ? rowMergedBy(linked) : null,
+    mergeMethod: normalizeMergeMethod(linked?.merge_method),
+    additions: normalizeCount(linked?.additions),
+    deletions: normalizeCount(linked?.deletions),
+    commitCount: normalizeCount(linked?.commit_count),
+    changedFiles: normalizeCount(linked?.changed_files),
+  };
+}
+
 function rowMergedBy(row: PullRequestRow): PrMergedBy | null {
   const login = String(row.merged_by_login ?? "").trim();
   if (!login) return null;
@@ -8625,54 +8665,6 @@ export function createPrService({
     };
   };
 
-  /**
-   * Resolve the lane columns of a list row.
-   *
-   * A detached row reports NO live lane (`linkedLaneId`/`linkedLaneName` stay null) and
-   * carries `detached` instead, so the renderer shows it as history rather than as a
-   * mapping it could act on. Previously the lane name fell back to the raw lane UUID
-   * when the lane row was missing, which surfaced a bare id in the list.
-   */
-  const deriveGithubSnapshotLaneLink = (
-    linked: PullRequestRow | null,
-    laneById: Map<string, LaneSummary> | undefined,
-  ): Pick<GitHubPrListItem, "linkedLaneId" | "linkedLaneName" | "detached"> => {
-    if (!linked) return { linkedLaneId: null, linkedLaneName: null, detached: null };
-    const detached = rowDetachedLane(linked);
-    if (detached) return { linkedLaneId: null, linkedLaneName: null, detached };
-    const laneName = laneById?.get(linked.lane_id)?.name ?? null;
-    return { linkedLaneId: linked.lane_id, linkedLaneName: laneName, detached: null };
-  };
-
-  /** Merge outcome + size, so a merged row can describe itself without a GitHub call. */
-  const deriveGithubSnapshotMergeFacts = (
-    linked: PullRequestRow | null,
-  ): Pick<
-    GitHubPrListItem,
-    "mergedAt" | "mergedBy" | "mergeMethod" | "additions" | "deletions" | "commitCount" | "changedFiles"
-  > => {
-    if (!linked) {
-      return {
-        mergedAt: null,
-        mergedBy: null,
-        mergeMethod: null,
-        additions: null,
-        deletions: null,
-        commitCount: null,
-        changedFiles: null,
-      };
-    }
-    return {
-      mergedAt: linked.merged_at ?? null,
-      mergedBy: rowMergedBy(linked),
-      mergeMethod: normalizeMergeMethod(linked.merge_method),
-      additions: normalizeCount(linked.additions),
-      deletions: normalizeCount(linked.deletions),
-      commitCount: normalizeCount(linked.commit_count),
-      changedFiles: normalizeCount(linked.changed_files),
-    };
-  };
-
   /**
    * Persist how a PR shipped. Written once at merge time and again by the poller when
    * it observes the merge; `coalesce` keeps whichever arrived first rather than letting

apps/desktop/src/main/services/prs/pullRequestRowCleanup.ts

@@ -4,13 +4,17 @@ type DbLike = {
 
 /**
  * Reads are only needed by the detach path, which must count before rows vanish.
- * Signature mirrors `AdeDb.get` so the concrete db satisfies it structurally.
+ * Signature mirrors `AdeDb.get`/`AdeDb.all` so the concrete db satisfies it structurally.
  */
 type ReadableDbLike = DbLike & {
   get<T extends Record<string, unknown> = Record<string, unknown>>(
     sql: string,
-    params?: any[],
+    params?: unknown[],
   ): T | null;
+  all<T extends Record<string, unknown> = Record<string, unknown>>(
+    sql: string,
+    params?: unknown[],
+  ): T[];
 };
 
 /**
@@ -113,22 +117,25 @@ export function deletePullRequestRowsByIds(db: DbLike, projectId: string, prIds:
  * Storage does not grow: the heavy snapshot columns are nulled here, which frees more
  * than the retained row costs. `commit_count` / `changed_files` are lifted onto the row
  * first so the merged view survives the purge.
+ *
+ * The mutation takes explicit PR ids rather than a SQL predicate, keeping one parameter
+ * list for all four statements below instead of making them agree positionally.
  */
 function detachRows(
   db: ReadableDbLike,
   args: {
     projectId: string;
-    /** SQL predicate over `pull_requests`, e.g. `lane_id = ? and project_id = ?`. */
-    predicate: string;
-    predicateParams: unknown[];
+    prIds: string[];
     laneName: string | null;
     laneColor: string | null;
     detachedAt: string;
     provenance: DetachedLaneProvenance;
   },
 ): void {
-  const { projectId, predicate, predicateParams, laneName, laneColor, detachedAt, provenance } = args;
-  const prSelect = `select id from pull_requests where ${predicate}`;
+  const { projectId, prIds, laneName, laneColor, detachedAt, provenance } = args;
+  if (prIds.length === 0) return;
+  const placeholders = prIds.map(() => "?").join(", ");
+  const scope = [projectId, ...prIds];
 
   // Lift counts off the snapshot before nulling it, so the merged row can still say
   // "12 commits · 9 files" once the JSON is gone.
@@ -147,9 +154,9 @@ function detachRows(
              from pull_request_snapshots s
              where s.pr_id = pull_requests.id and json_valid(s.files_json))
           )
-      where ${predicate}
+      where project_id = ? and id in (${placeholders})
     `,
-    predicateParams,
+    scope,
   );
 
   // `detached_at is null` keeps the first detach authoritative: re-detaching an already
@@ -161,9 +168,9 @@ function detachRows(
           detached_lane_name = ?,
           detached_lane_color = ?,
           detached_provenance = ?
-      where ${predicate} and detached_at is null
+      where project_id = ? and id in (${placeholders}) and detached_at is null
     `,
-    [detachedAt, laneName, laneColor, JSON.stringify(provenance), ...predicateParams],
+    [detachedAt, laneName, laneColor, JSON.stringify(provenance), ...scope],
   );
 
   // Keep detail/status/commits (small, and what the merged view reads); drop the bulky
@@ -175,13 +182,13 @@ function detachRows(
           checks_json = null,
           comments_json = null,
           reviews_json = null
-      where pr_id in (${prSelect})
+      where pr_id in (${placeholders})
     `,
-    predicateParams,
+    prIds,
   );
 
   // Group membership is lane-scoped work-in-progress, not history — it goes.
-  db.run(`delete from pr_group_members where pr_id in (${prSelect})`, predicateParams);
+  db.run(`delete from pr_group_members where pr_id in (${placeholders})`, prIds);
   pruneEmptyPrGroups(db, projectId);
 }
 
@@ -190,15 +197,19 @@ export function detachPullRequestRowsForLane(
   args: DetachPullRequestRowsArgs,
 ): void {
   const { projectId, laneId, laneName, laneColor, detachedAt } = args;
+  const prIds = db
+    .all<{ id: string }>("select id from pull_requests where lane_id = ? and project_id = ?", [laneId, projectId])
+    .map((row) => String(row.id));
   detachRows(db, {
     projectId,
-    predicate: "lane_id = ? and project_id = ?",
-    predicateParams: [laneId, projectId],
+    prIds,
     laneName,
     laneColor,
     detachedAt,
     provenance: countLaneProvenance(db, projectId, laneId),
   });
+  // Lane-scoped membership survives the id lookup above (a group member can outlive its
+  // PR row), so clear it explicitly and prune once.
   db.run("delete from pr_group_members where lane_id = ?", [laneId]);
   pruneEmptyPrGroups(db, projectId);
 }
@@ -214,17 +225,12 @@ export function detachPullRequestRowsByIds(
   args: DetachPullRequestRowsArgs & { prIds: string[] },
 ): void {
   const { projectId, laneId, laneName, laneColor, detachedAt } = args;
-  const ids = uniqueIds(args.prIds);
-  if (ids.length === 0) return;
-  const placeholders = ids.map(() => "?").join(", ");
   detachRows(db, {
     projectId,
-    predicate: `project_id = ? and id in (${placeholders})`,
-    predicateParams: [projectId, ...ids],
+    prIds: uniqueIds(args.prIds),
     laneName,
     laneColor,
     detachedAt,
     provenance: countLaneProvenance(db, projectId, laneId),
   });
 }
-