← back to Homesonspec

Apple review poller: stop disarming itself while awaiting Apple

043c09b71e5d3fb42897e5304ee7e7e37eb88fab · 2026-09-13 16:12:44 -0700 · Steve

The poller treated REJECTED as terminal and called launchctl remove on
every run, i.e. it tried to unschedule itself 303 times since 09-08. It
survived only because JOB_LABEL said 'com.steve.homesonspec-apple-review'
while the label actually loaded is '...-apple-review-v2', so every removal
returned status 3 (no such label). Correcting that constant alone would
have armed the self-destruct.

- shouldDisarm(): only an 'approved' outcome may stop the monitor. REJECTED
  is the state we are waiting to LEAVE while a Resolution Center reply is
  outstanding; stopping there blinds us at the moment it matters.
- JOB_LABEL corrected to the label that is actually loaded, with a comment
  that it is only safe alongside the guard above.
- Alert on state CHANGE only (was: a desktop banner every 15 minutes) and
  record alert_delivered via the shared cncp_post helper, which asserts the
  HTTP status instead of swallowing it.
- Negative tests: an attention outcome must NOT disarm, with a positive
  control so the guard cannot pass as dead code, plus a JOB_LABEL/plist
  assertion. Verified red-then-green by re-injecting the original logic;
  all 5 pre-existing tests stayed green through the injected fault, which
  is why this shipped untested.

TK-11155

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BPS3gnB4H5MQKmWcHKvuf6

Files touched

• M ops/apple-review-poller.mjs
• M ops/apple-review-poller.test.mjs

Diff

commit 043c09b71e5d3fb42897e5304ee7e7e37eb88fab
Author: Steve <steve@designerwallcoverings.com>
Date:   Sun Sep 13 16:12:44 2026 -0700

    Apple review poller: stop disarming itself while awaiting Apple
    
    The poller treated REJECTED as terminal and called launchctl remove on
    every run, i.e. it tried to unschedule itself 303 times since 09-08. It
    survived only because JOB_LABEL said 'com.steve.homesonspec-apple-review'
    while the label actually loaded is '...-apple-review-v2', so every removal
    returned status 3 (no such label). Correcting that constant alone would
    have armed the self-destruct.
    
    - shouldDisarm(): only an 'approved' outcome may stop the monitor. REJECTED
      is the state we are waiting to LEAVE while a Resolution Center reply is
      outstanding; stopping there blinds us at the moment it matters.
    - JOB_LABEL corrected to the label that is actually loaded, with a comment
      that it is only safe alongside the guard above.
    - Alert on state CHANGE only (was: a desktop banner every 15 minutes) and
      record alert_delivered via the shared cncp_post helper, which asserts the
      HTTP status instead of swallowing it.
    - Negative tests: an attention outcome must NOT disarm, with a positive
      control so the guard cannot pass as dead code, plus a JOB_LABEL/plist
      assertion. Verified red-then-green by re-injecting the original logic;
      all 5 pre-existing tests stayed green through the injected fault, which
      is why this shipped untested.
    
    TK-11155
    
    Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
    Claude-Session: https://claude.ai/code/session_01BPS3gnB4H5MQKmWcHKvuf6
---
 ops/apple-review-poller.mjs      | 61 +++++++++++++++++++++++++++++--
 ops/apple-review-poller.test.mjs | 77 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 134 insertions(+), 4 deletions(-)

diff --git a/ops/apple-review-poller.mjs b/ops/apple-review-poller.mjs
index f9080605..7d1f0bda 100644
--- a/ops/apple-review-poller.mjs
+++ b/ops/apple-review-poller.mjs
@@ -6,7 +6,13 @@ import { spawnSync } from 'node:child_process';
 import { fileURLToPath } from 'node:url';
 
 export const APP_NAME = 'Homes on Spec';
-export const JOB_LABEL = 'com.steve.homesonspec-apple-review';
+// NOTE (TK-11155, 2026-09-13): this constant said 'com.steve.homesonspec-apple-review'
+// while the label ACTUALLY loaded in gui/501 is '...-apple-review-v2'. Because of that
+// mismatch every self-disarm call below silently failed (launchctl remove -> status 3,
+// "no such label") and this monitor survived BY ACCIDENT for five days. Correcting the
+// label ALONE would have armed the self-destruct — it is only safe alongside the
+// DISARM_OUTCOMES guard below. Never "fix" one without the other.
+export const JOB_LABEL = 'com.steve.homesonspec-apple-review-v2';
 
 const TERMINAL_STATES = new Set([
   'READY_FOR_SALE',
@@ -40,13 +46,49 @@ function record(path, event) {
   appendFileSync(path, `${JSON.stringify(event)}\n`);
 }
 
+function readPreviousState(statePath) {
+  try {
+    return JSON.parse(readFileSync(statePath, 'utf8')).state ?? null;
+  } catch {
+    return null; // no prior run, or unreadable -> treat as a change so we never stay silent
+  }
+}
+
+// Returns whether the alert was actually DELIVERED, not merely attempted.
+// A desktop banner alone leaves no receipt and shows nothing on a locked/headless Mac,
+// so an attention alert also posts a CNCP parking-lot card through the shared helper,
+// which asserts the HTTP status and writes a delivery receipt (CLAUDE.md TK-11431 #2).
 function notify(state, outcome) {
   const message = outcome === 'approved'
     ? `Apple completed review: ${state}`
     : `Apple review needs attention: ${state}`;
+
   spawnSync('/usr/bin/osascript', [
     '-e', 'display notification ' + JSON.stringify(message) + ' with title "Homes on Spec"',
   ]);
+
+  const helper = `${process.env.HOME}/.claude/skills/_shared/cncp_post.sh`;
+  const posted = spawnSync('/bin/bash', [
+    '-c',
+    `. ${JSON.stringify(helper)} && cncp_post "ios://homesonspec/apple-review" ` +
+      JSON.stringify(`Homes on Spec — Apple review state changed to ${state} (${outcome}). TK-11155.`),
+  ], { encoding: 'utf8', timeout: 30_000 });
+
+  return posted.status === 0;
+}
+
+// Only an APPROVED outcome may disarm this monitor.
+//
+// REJECTED / METADATA_REJECTED / DEVELOPER_REJECTED are "terminal" in Apple's sense but
+// they are precisely the state we are WAITING TO LEAVE while a Resolution Center reply is
+// outstanding. Disarming there is self-blinding at the exact moment monitoring matters —
+// it is the same class of failure that let the 2026-09-08 rejection sit unnoticed.
+// 'terminal' answers "did Apple stop working on it"; DISARM answers "may we stop looking".
+// They are not the same question and must not share one flag.
+export const DISARM_OUTCOMES = new Set(['approved']);
+
+export function shouldDisarm(classification) {
+  return Boolean(classification?.terminal) && DISARM_OUTCOMES.has(classification.outcome);
 }
 
 export function removeScheduler(label = JOB_LABEL) {
@@ -83,13 +125,26 @@ export function run({ fixturePath = null, runtimeDir = resolve('ops/runtime') }
     error: commandError || (state ? null : 'App state missing from ipa-status output'),
   };
 
+  // Read the prior state BEFORE overwriting it — notify only on a CHANGE.
+  // Firing every 15 minutes for an unchanged REJECTED trains the eye to ignore the
+  // one banner that will matter, and buries the transition in its own noise.
+  const previousState = readPreviousState(statePath);
+  const stateChanged = previousState !== state;
+  event.previousState = previousState;
+  event.stateChanged = stateChanged;
+
+  if (classification.terminal && stateChanged && !fixturePath) {
+    event.alert_delivered = notify(state, classification.outcome);
+  }
+
   record(logPath, event);
   mkdirSync(dirname(statePath), { recursive: true });
   writeFileSync(statePath, `${JSON.stringify(event, null, 2)}\n`);
   process.stdout.write(`${JSON.stringify(event)}\n`);
 
-  if (classification.terminal && !fixturePath) {
-    notify(state, classification.outcome);
+  // Disarm ONLY on an approved outcome. See DISARM_OUTCOMES above: an 'attention'
+  // outcome is the state we are waiting to leave, so stopping there blinds us.
+  if (shouldDisarm(classification) && !fixturePath) {
     const removal = removeScheduler();
     record(logPath, {
       checkedAt: new Date().toISOString(),
diff --git a/ops/apple-review-poller.test.mjs b/ops/apple-review-poller.test.mjs
index 87de4974..92e10e5c 100644
--- a/ops/apple-review-poller.test.mjs
+++ b/ops/apple-review-poller.test.mjs
@@ -4,7 +4,12 @@ import { tmpdir } from 'node:os';
 import { join } from 'node:path';
 import test from 'node:test';
 
-import { classifyState, parseAppState, removeScheduler, run } from './apple-review-poller.mjs';
+import { existsSync } from 'node:fs';
+import { spawnSync } from 'node:child_process';
+
+import {
+  DISARM_OUTCOMES, JOB_LABEL, classifyState, parseAppState, removeScheduler, run, shouldDisarm,
+} from './apple-review-poller.mjs';
 
 const row = (state) => `🟡 Homes on Spec          ${state}         1.0\n`;
 
@@ -42,3 +47,73 @@ test('scheduler removal returns a real launchctl result for an absent isolated l
   const result = removeScheduler('com.steve.homesonspec-apple-review-test-absent');
   assert.equal(typeof result.status, 'number');
 });
+
+
+// ─────────────────────────────────────────────────────────────────────────────
+// TK-11155 (2026-09-13) — NEGATIVE TESTS for the self-disarm path.
+//
+// The suite above never exercised the real disarm decision: its only disarm test
+// runs in FIXTURE mode, where removal is skipped by the `!fixturePath` guard no
+// matter what the logic says. So the branch that can permanently blind this
+// monitor was shipped untested. These tests inject the fault (Apple rejects us)
+// and prove the monitor SURVIVES, plus a positive control proving the disarm
+// path is not simply dead.
+// ─────────────────────────────────────────────────────────────────────────────
+
+test('NEGATIVE: an attention outcome must NOT disarm the monitor', () => {
+  for (const state of ['REJECTED', 'METADATA_REJECTED', 'DEVELOPER_REJECTED', 'INVALID_BINARY']) {
+    const classification = classifyState(state);
+    // Apple has stopped working on it...
+    assert.equal(classification.terminal, true, `${state} should still be terminal`);
+    // ...but that is NOT permission for us to stop looking. This is the whole bug.
+    assert.equal(shouldDisarm(classification), false, `${state} must NOT disarm the monitor`);
+  }
+});
+
+test('POSITIVE CONTROL: an approved outcome DOES disarm (guard is not dead code)', () => {
+  for (const state of ['READY_FOR_SALE', 'READY_FOR_DISTRIBUTION', 'PENDING_DEVELOPER_RELEASE']) {
+    assert.equal(shouldDisarm(classifyState(state)), true, `${state} should disarm`);
+  }
+  assert.deepEqual([...DISARM_OUTCOMES], ['approved']);
+});
+
+test('NEGATIVE: an unknown/unparsed state must never disarm', () => {
+  assert.equal(shouldDisarm(classifyState(null)), false);
+  assert.equal(shouldDisarm(undefined), false);
+});
+
+test('JOB_LABEL names the label actually loaded in launchd', () => {
+  const plist = `${process.env.HOME}/Library/LaunchAgents/${JOB_LABEL}.plist`;
+  if (!existsSync(plist)) {
+    assert.fail(
+      `NOT MEASURED -> FAIL: no plist at ${plist}. JOB_LABEL '${JOB_LABEL}' names a job that is ` +
+      'not installed here. A label mismatch is exactly what made every self-disarm call silently ' +
+      'fail for five days, so this is asserted, never skipped.');
+  }
+  const label = spawnSync('/usr/libexec/PlistBuddy', ['-c', 'Print :Label', plist], { encoding: 'utf8' });
+  assert.equal((label.stdout ?? '').trim(), JOB_LABEL);
+});
+
+test('repeated identical state does not re-alert (no 15-minute banner spam)', () => {
+  const dir = mkdtempSync(join(tmpdir(), 'homesonspec-review-poller-nospam-'));
+  const fixture = join(dir, 'status.txt');
+  writeFileSync(fixture, row('REJECTED'));
+  const first = run({ fixturePath: fixture, runtimeDir: dir });
+  assert.equal(first.stateChanged, true, 'first sighting is a change');
+  const second = run({ fixturePath: fixture, runtimeDir: dir });
+  assert.equal(second.stateChanged, false, 'unchanged REJECTED must not re-alert');
+  assert.equal(second.previousState, 'REJECTED');
+});
+
+test('a state TRANSITION out of REJECTED is detected (this is what we are waiting for)', () => {
+  const dir = mkdtempSync(join(tmpdir(), 'homesonspec-review-poller-transition-'));
+  const fixture = join(dir, 'status.txt');
+  writeFileSync(fixture, row('REJECTED'));
+  run({ fixturePath: fixture, runtimeDir: dir });
+  writeFileSync(fixture, row('IN_REVIEW'));
+  const moved = run({ fixturePath: fixture, runtimeDir: dir });
+  assert.equal(moved.stateChanged, true);
+  assert.equal(moved.previousState, 'REJECTED');
+  assert.equal(moved.state, 'IN_REVIEW');
+  assert.equal(moved.terminal, false, 'IN_REVIEW is Apple working again, not terminal');
+});

← fb984ed6 eas: pin production ios.image=latest (ITMS-90725 guard)  ·  back to Homesonspec  ·  Poller: notify() must never be able to fail its caller 471d1568 →