fix: batch fix #991 + #966 + #992 + #995 + #1002 + Skill tab UI consistency

package.json

@@ -87,7 +87,8 @@
     "clean": "pnpm -r run clean && rm -rf node_modules",
     "check:start-profile-isolation": "node --test scripts/start-dev-profile-isolation.test.mjs",
     "check:brand-dictionary": "node --test scripts/brand-dictionary-helper.test.mjs",
-    "check:brand-guard": "node --test scripts/intake-from-opensource.test.mjs"
+    "check:brand-guard": "node --test scripts/intake-from-opensource.test.mjs",
+    "check:biome-version": "node -e \"const pkg=require('./package.json');const v=pkg.devDependencies?.['@biomejs/biome']??'?';console.log('biome lockfile version:',v)\""
   },
   "devDependencies": {
     "@biomejs/biome": "^2.4.1",

packages/api/src/config/capabilities/capability-write-guards.ts

@@ -21,13 +21,26 @@ export function resolveCapabilityWriteSessionUserId(request: FastifyRequest): st
   return typeof sessionUserId === 'string' && sessionUserId.trim() ? sessionUserId.trim() : null;
 }
 
+/**
+ * Owner gate for capability/plugin write operations (#794 unified pattern).
+ *
+ * **For write routes** (install, delete, toggle, config save):
+ *   Use `{ allowMissingOwner: true }` — falls through in local single-user mode
+ *   (no DEFAULT_OWNER_USER_ID configured). Security is provided by session auth +
+ *   origin check + loopback guard; the owner gate only adds value when explicitly
+ *   configured for multi-user/shared deployments.
+ *
+ * **For data-visibility filters** (canReadSensitiveMcpConfig):
+ *   Use `{ requireConfiguredOwner: true }` — fail when unconfigured, because
+ *   "not configured" means "hide sensitive data" not "block the request".
+ *
+ * ⚠️ Do NOT use requireConfiguredOwner for write routes — it blocks local single-user
+ *   mode unnecessarily. This mistake has recurred multiple times (#995, #794 history).
+ */
 export function requireCapabilityWriteOwner(
   userId: string,
   options: CapabilityWriteOwnerOptions = {},
 ): CapabilityWriteRouteError | null {
-  // Write callers (MCP install/delete, plugin enable/disable) pass allowMissingOwner: true
-  // → fall through in single-user mode. The data-filter caller (canReadSensitiveMcpConfig)
-  // passes requireConfiguredOwner: true → fail when unconfigured (sensitive data gate).
   const shouldFallThrough = !!options.allowMissingOwner && !options.requireConfiguredOwner;
   return resolveOwnerGate(userId, {
     requireConfiguredOwner: !shouldFallThrough,

packages/api/src/domains/cats/services/agents/providers/acp/AcpProcessPool.ts

@@ -69,6 +69,8 @@ interface AcpPoolVariantConfig {
 interface PoolEntry {
   client: AcpPoolClient;
   leaseCount: number;
+  /** Bumped on stale-lease force-release so old lease closures become no-ops (#992). */
+  leaseGeneration: number;
   lastUsedAt: number;
   state: 'initializing' | 'ready' | 'closing';
   idleTimer: ReturnType<typeof setTimeout> | null;
@@ -144,7 +146,22 @@ export class AcpProcessPool {
         if (this.supportsMultiplexing || owner.leaseCount === 0) {
           return this.leaseReadyEntry(owner, poolKey);
         }
-        throw new Error(`ACP session ${sessionId} is already active on its owning process`);
+        // #992: Stale lease recovery — the previous lease holder is a zombie (e.g. Windows
+        // console disconnect where the async generator finally block never ran). Since the
+        // caller is re-acquiring the SAME sessionId, the previous consumer is necessarily
+        // gone. Force-release the orphaned lease so the process can be reused.
+        log.warn(
+          { poolKey, sessionId, staleLeaseCount: owner.leaseCount },
+          'ACP stale lease detected — force-releasing zombie lease for session re-acquire',
+        );
+        this._metrics.activeLeaseCount -= owner.leaseCount;
+        owner.leaseCount = 0;
+        // Transition to idle so leaseReadyEntry's idleProcessCount-- is balanced.
+        this._metrics.idleProcessCount++;
+        // Bump generation so any late-arriving release() from the old lease becomes a
+        // no-op (the old closure captured the previous generation value).
+        owner.leaseGeneration++;
+        return this.leaseReadyEntry(owner, poolKey);
       }
       if (owner) this.sessionOwners.delete(sessionKey);
     }
@@ -268,12 +285,19 @@ export class AcpProcessPool {
 
   private createLease(entry: PoolEntry, poolKey: PoolKey): AcpLease {
     let released = false;
+    // Capture the generation at lease creation time. If a stale-lease force-release
+    // bumps the generation before this closure runs, the release becomes a no-op —
+    // preventing the late-arriving old finally from corrupting the new lease (#992).
+    const creationGeneration = entry.leaseGeneration;
     return {
       client: entry.client,
       poolKey,
       release: () => {
         if (released) return;
         released = true;
+        // Stale lease guard: generation mismatch means this lease was force-released
+        // and a new lease has been issued on the same entry. The old release is a no-op.
+        if (entry.leaseGeneration !== creationGeneration) return;
         entry.leaseCount--;
         this._metrics.activeLeaseCount--;
         if (entry.leaseCount <= 0) {
@@ -290,6 +314,7 @@ export class AcpProcessPool {
     const entry: PoolEntry = {
       client,
       leaseCount: 0, // caller manages lease count after spawn
+      leaseGeneration: 0,
       lastUsedAt: Date.now(),
       state: 'initializing',
       idleTimer: null,

packages/api/src/infrastructure/email/ReviewFeedbackTaskSpec.ts

@@ -17,7 +17,6 @@ import {
   type Thread,
 } from '../../domains/cats/services/stores/ports/ThreadStore.js';
 import type { ICommunityEventLog } from '../../domains/community/CommunityEventLog.js';
-import { decideDelivery } from '../../domains/community/community-delivery-policy.js';
 import type { DistillationCheckpoint } from '../distillation/DistillationCheckpoint.js';
 import type { ExecuteContext, TaskSpec_P1 } from '../scheduler/types.js';
 import type { ConnectorInvokeTrigger, ConnectorTriggerPolicy } from './ConnectorInvokeTrigger.js';
@@ -505,33 +504,18 @@ export function createReviewFeedbackTaskSpec(opts: ReviewFeedbackTaskSpecOptions
             const reviewFilter = opts.isEchoReview;
             // R5-P2: use safeDeliveryXxx (items up to first failure) so items after a break are
             // not notified this round — they will be retried next poll without double-notification.
+            // #1002: decideDelivery removed — it silenced OWNER/MEMBER reviews,
+            // but PR tracking is opt-in (cat explicitly registered), so ALL reviewer
+            // feedback should be delivered. isEchoComment + isNoiseComment are sufficient.
             const newComments = safeDeliveryComments.filter((c) => {
               if (commentFilter?.(c)) return false;
               if (noiseFilter?.(c)) return false;
-              // F168 Phase B: apply delivery policy — OWNER/MEMBER activity is silent-log
-              const decision = decideDelivery({
-                state: 'in_progress', // stateless function — state field not used
-                eventKind: 'pr.review_submitted',
-                authorAssociation: c.authorAssociation as
-                  | import('@cat-cafe/shared').GitHubAuthorAssociation
-                  | undefined,
-              });
-              if (decision === 'silent-log') return false;
               return true;
             });
-            const newDecisions = (
-              reviewFilter ? safeDeliveryReviews.filter((r) => !reviewFilter(r)) : safeDeliveryReviews
-            ).filter((r) => {
-              // F168 Phase B: apply delivery policy — OWNER/MEMBER review decisions are silent-log
-              const decision = decideDelivery({
-                state: 'in_progress', // stateless function — state field not used
-                eventKind: 'pr.review_submitted',
-                authorAssociation: r.authorAssociation as
-                  | import('@cat-cafe/shared').GitHubAuthorAssociation
-                  | undefined,
-              });
-              return decision !== 'silent-log';
-            });
+            // #1002: same — isEchoReview is the only filter needed for review decisions.
+            const newDecisions = reviewFilter
+              ? safeDeliveryReviews.filter((r) => !reviewFilter(r))
+              : safeDeliveryReviews;
 
             // R4-P1-B: when eventLog is configured, cap cursor advancement at the last
             // successfully projected item (maxSafeXxxCursor). Items beyond a projection

packages/api/src/routes/connector-plugins.ts

@@ -21,6 +21,7 @@ import type { FastifyPluginAsync, FastifyReply, FastifyRequest } from 'fastify';
 import {
   type CapabilityWriteRouteError,
   requireCapabilityWriteOwner,
+  requireLocalCapabilityWriteRequest,
 } from '../config/capabilities/capability-write-guards.js';
 import { configEventBus, createChangeSetId } from '../config/config-event-bus.js';
 import { isOriginAllowed, PRIVATE_NETWORK_ORIGIN, resolveFrontendCorsOrigins } from '../config/frontend-origin.js';
@@ -70,6 +71,11 @@ function resolvePluginIconMime(filePath: string): string | null {
 }
 
 function requirePluginWriteAccess(request: FastifyRequest): CapabilityWriteRouteError | null {
+  // Layer 1: Loopback guard — reject non-localhost and proxy-forwarded requests (#794 pattern).
+  const localError = requireLocalCapabilityWriteRequest(request);
+  if (localError) return localError;
+
+  // Layer 2: Session authentication.
   const userId = resolveSessionUserId(request);
   if (!userId) {
     return { status: 401, error: 'Plugin writes require session authentication' };
@@ -80,13 +86,16 @@ function requirePluginWriteAccess(request: FastifyRequest): CapabilityWriteRoute
     return { status: 403, error: 'Connector plugin writes require same-origin Hub access' };
   }
 
-  return requireCapabilityWriteOwner(userId, {
-    requireConfiguredOwner: true,
-    missingOwnerError: 'Connector plugin writes require DEFAULT_OWNER_USER_ID to be configured',
-  });
+  // Layer 3: Owner gate — fall through in single-user mode (#794 pattern, #995 fix).
+  return requireCapabilityWriteOwner(userId, { allowMissingOwner: true });
 }
 
 function requirePluginListAccess(request: FastifyRequest): CapabilityWriteRouteError | null {
+  // Layer 1: Loopback guard — plugin metadata is reconnaissance-sensitive (#794 pattern).
+  const localError = requireLocalCapabilityWriteRequest(request);
+  if (localError) return localError;
+
+  // Layer 2: Session authentication.
   const userId = resolveSessionUserId(request);
   if (!userId) {
     return { status: 401, error: 'Plugin listing requires session authentication' };
@@ -97,10 +106,8 @@ function requirePluginListAccess(request: FastifyRequest): CapabilityWriteRouteE
     return { status: 403, error: 'Connector plugin listing requires same-origin Hub access' };
   }
 
-  return requireCapabilityWriteOwner(userId, {
-    requireConfiguredOwner: true,
-    missingOwnerError: 'Connector plugin listing requires DEFAULT_OWNER_USER_ID to be configured',
-  });
+  // Layer 3: Owner gate — fall through in single-user mode (#794 pattern, #995 fix).
+  return requireCapabilityWriteOwner(userId, { allowMissingOwner: true });
 }
 
 function toPublicPluginMeta(plugins: ReturnType<typeof listInstalledPlugins>) {

packages/api/src/skills/skill-sync-config.ts

@@ -136,12 +136,20 @@ export interface ConfigSyncCtx {
   pruneMountPaths?: boolean;
   /** When true, inherited-only global mount paths are skipped to preserve cascade. */
   preserveGlobalCascade?: boolean;
+  /** Skill names that already exist in the project config (used with preserveGlobalCascade
+   *  to distinguish existing skills from newly discovered ones — fixes #991). */
+  existingProjectSkills?: ReadonlySet<string>;
   /** Mount point IDs that were just enabled (absent in previous rules, present now).
    *  When set, active skills (mountPaths.length > 0) get these IDs supplemented. */
   newlyEnabledMountPointIds?: string[];
 }
 
 export async function updateConfigAfterSync(projectRoot: string, ctx: ConfigSyncCtx): Promise<void> {
+  // #991: New skills discovered during drift-resolve that inherit global mount paths.
+  // They need a config entry (so drift-check stops reporting configNew) but must NOT
+  // have project-local mountPaths written (that would freeze the global cascade — #962).
+  const cascadeNewSkills: string[] = [];
+
   if (ctx.enabledNames.length > 0) {
     const grouped = new Map<string, { skillNames: string[]; mountPointIds: string[] }>();
     const noPolicySkills: string[] = [];
@@ -157,7 +165,14 @@ export async function updateConfigAfterSync(projectRoot: string, ctx: ConfigSync
         // Only active in drift-resolve context (preserveGlobalCascade=true) where global
         // policy changes should propagate without freezing. Explicit sync operations
         // (sync/sync-skill) write mount paths to establish local baseline.
-        if (ctx.preserveGlobalCascade && !hasLocalPolicy && !ctx.mountPathsBySkill.has(name)) continue;
+        if (ctx.preserveGlobalCascade && !hasLocalPolicy && !ctx.mountPathsBySkill.has(name)) {
+          // #991: New skills still need a config entry so drift-check stops reporting
+          // configNew. Collect them for registration WITHOUT mountPaths below.
+          if (ctx.existingProjectSkills && !ctx.existingProjectSkills.has(name)) {
+            cascadeNewSkills.push(name);
+          }
+          continue;
+        }
         const shouldPrune = ctx.pruneMountPaths || !hasLocalPolicy;
         const mountPointIds = shouldPrune ? declared.filter((id) => activeSet.has(id)) : [...declared];
         // F228: When a mount point is newly enabled, supplement active skills
@@ -181,6 +196,28 @@ export async function updateConfigAfterSync(projectRoot: string, ctx: ConfigSync
       await updateSkillMountPaths(projectRoot, noPolicySkills, ctx.activeTargetIds);
     }
   }
+  // #991: Register new inherited-only skills in config WITHOUT mountPaths.
+  // This creates a managed capability entry (drift-check stops reporting configNew)
+  // while preserving global mount path cascade (no project-local mountPaths written,
+  // so resolveEffectiveSkillMountPaths falls through to global — #962 intent kept).
+  if (cascadeNewSkills.length > 0) {
+    const config = await readCapabilitiesConfig(projectRoot);
+    if (config) {
+      const existingIds = new Set(
+        config.capabilities
+          .filter((c) => c.type === 'skill' && c.source === 'cat-cafe' && !c.pluginId)
+          .map((c) => c.id),
+      );
+      let changed = false;
+      for (const name of cascadeNewSkills) {
+        if (!existingIds.has(name)) {
+          config.capabilities.push({ id: name, type: 'skill', source: 'cat-cafe', enabled: true, globalEnabled: true });
+          changed = true;
+        }
+      }
+      if (changed) await writeCapabilitiesConfig(projectRoot, config);
+    }
+  }
   if (ctx.removedNames.length > 0) {
     const disabledDirs = STANDARD_MOUNT_POINT_IDS.filter((id) => !ctx.mountRules.mountPoints[id].enabled).map((id) =>
       join(projectRoot, ctx.mountRules.mountPoints[id].path),

packages/api/src/skills/skill-sync-engine.ts

@@ -408,6 +408,7 @@ async function syncProjectUnlocked(
       mountRules,
       pruneMountPaths: opts.pruneMountPaths,
       preserveGlobalCascade: opts.preserveGlobalCascade,
+      existingProjectSkills: new Set(previousNames),
       newlyEnabledMountPointIds: opts.pruneMountPaths ? newlyEnabledMountPointIds : undefined,
     });
 

packages/api/test/acp/acp-process-pool.test.js

@@ -216,6 +216,77 @@ describe('AcpProcessPool', () => {
       resumeLease.release();
     });
 
+    test('stale lease on session-owned entry is force-released on re-acquire (#992)', async () => {
+      const { AcpProcessPool } = await import(
+        '../../dist/domains/cats/services/agents/providers/acp/AcpProcessPool.js'
+      );
+      pool = new AcpProcessPool(defaultPoolConfig, nonMultiplexedVariantConfig, createMockClient);
+
+      // Simulate: first acquire + rememberSession, but lease never released (zombie)
+      const lease1 = await pool.acquire(key1);
+      const ownerClient = lease1.client;
+      pool.rememberSession(key1, 'stale-sess', lease1);
+      // Do NOT release lease1 — simulates Windows console disconnect where finally never runs
+
+      assert.strictEqual(pool.getMetrics().activeLeaseCount, 1);
+
+      // Second acquire with same sessionId should recover, not throw
+      const lease2 = await pool.acquire(key1, { sessionId: 'stale-sess' });
+      assert.strictEqual(lease2.client, ownerClient, 'should reuse the same process');
+      assert.ok(lease2.client.isAlive);
+
+      // The stale lease was force-released, and a new lease was granted
+      // activeLeaseCount should be 1 (the new lease), not 2
+      assert.strictEqual(pool.getMetrics().activeLeaseCount, 1);
+
+      lease2.release();
+      assert.strictEqual(pool.getMetrics().activeLeaseCount, 0);
+    });
+
+    test('late release of stale lease does not corrupt new lease (#992 P1)', async () => {
+      const { AcpProcessPool } = await import(
+        '../../dist/domains/cats/services/agents/providers/acp/AcpProcessPool.js'
+      );
+      pool = new AcpProcessPool(
+        { ...defaultPoolConfig, idleTtlMs: 20, healthCheckIntervalMs: 999_999 },
+        nonMultiplexedVariantConfig,
+        createMockClient,
+      );
+
+      // Step 1: acquire lease1, remember session, don't release (zombie)
+      const lease1 = await pool.acquire(key1);
+      const ownerClient = lease1.client;
+      pool.rememberSession(key1, 'late-sess', lease1);
+
+      // Step 2: re-acquire same session → force-release recovery
+      const lease2 = await pool.acquire(key1, { sessionId: 'late-sess' });
+      assert.strictEqual(lease2.client, ownerClient);
+      assert.strictEqual(pool.getMetrics().activeLeaseCount, 1);
+
+      // Step 3: old lease1.release() arrives late (async generator finally fires)
+      lease1.release();
+
+      // Invariants that must hold after late release:
+      // - new lease2 is still active (not corrupted)
+      assert.ok(lease2.client.isAlive, 'new lease client must still be alive');
+      // - activeLeaseCount must not go negative
+      assert.ok(pool.getMetrics().activeLeaseCount >= 0, 'activeLeaseCount must not go negative');
+      // - activeLeaseCount should still be 1 (lease2 is active, lease1's release was stale)
+      assert.strictEqual(pool.getMetrics().activeLeaseCount, 1, 'late stale release must be no-op');
+      // - idleProcessCount must not go negative
+      assert.ok(pool.getMetrics().idleProcessCount >= 0, 'idleProcessCount must not go negative');
+
+      // Step 4: wait past idle TTL — process must NOT be evicted while lease2 is active
+      await new Promise((r) => setTimeout(r, 50));
+      assert.ok(lease2.client.isAlive, 'lease2 client must survive idle TTL');
+      assert.strictEqual(pool.getMetrics().liveProcessCount, 1);
+
+      // Step 5: normal release of lease2 should work correctly
+      lease2.release();
+      assert.strictEqual(pool.getMetrics().activeLeaseCount, 0);
+      assert.strictEqual(pool.getMetrics().idleProcessCount, 1);
+    });
+
     test('double release is safe (no-op)', async () => {
       const { AcpProcessPool } = await import(
         '../../dist/domains/cats/services/agents/providers/acp/AcpProcessPool.js'