lock worktree/PR off for the non-committing coordinator task types

.changelog/NEXT.md

@@ -8,6 +8,7 @@
 
 ## Fixed
 
+- **A successful `branch-cleanup` run is no longer recorded as a failure.** The task deletes merged branches in the app's live checkout — its prompt `cd`s to the repo path and runs `git branch -d` / `git push --delete` — but it was the only git/gh coordinator type shipping with no `taskMetadata` at all, so `applyAppWorktreeDefault` filled `useWorktree`/`openPR` from the app's `defaultOpenPR: true`. Every run got a CoS-managed worktree it never touched (one that holds the very refs it was trying to delete) and a PR expectation it could never satisfy, and finalization scored the completed run `pr-missing`. The four non-committing coordinator types — `branch-cleanup`, `branch-reconcile`, `issue-reconcile`, `jira-status-report` — now share one documented `taskMetadata` posture: `useWorktree`/`openPR` explicitly `false` (not merely absent, which is what let the app defaults in) *and* locked via `MANAGED_AGENT_OPTIONS` so a per-app override can't re-attach them, plus `worktreeChangesExpected: false`, because a clean tree is the success shape for a branch deletion or a posted report and the TUI idle-complete gate must not read it as "the model produced no work". `jira-status-report` was hit by the same bug for a subtler reason: its `readOnly: true` skips worktree *creation* but the finalize-time PR-claim check reads `metadata.openPR` directly, so a posted report still scored `pr-missing`. A guard test now iterates `NON_COMMITTING_COORDINATOR_TASK_TYPES` — the set that already grants these types their commit-criterion exemption — so a future coordinator type can't be added to one list and not the other. Existing installs pick all of this up on the next schedule load: the taskMetadata deep-merge backfills the new keys and `enforceManagedAgentOptions` rewrites any stored `true`.
 - **[issue-3396] CyberCity's ambient music no longer drifts under main-thread load.** The chord changes (every 2400ms) and the 16th-note arp plucks (every 150ms) rode two independent `setInterval`s that each read `ctx.currentTime` at fire time — so under load both drifted against the audio clock *and* against each other, the failure mode the sibling synth players adopted the shared lookahead scheduler to avoid. Both now come off one 16th-note step counter (16 steps per chord, so the arp is phase-locked to the chord changes by construction) and every event is written to the audio clock at an absolute grid time, `gridOrigin + step × 0.15s`, which cannot accumulate drift. The timing constants come from `SYNTH_TIMING` in `lib/lookaheadTransport.js` so the feel stays shared; the transport body itself is deliberately not reused, because it reads its clock from the shared `lib/audioContext.js` singleton while `cityAudioEngine` is a documented holdout that owns (and closes) its own `AudioContext` — scheduling this graph against a clock it doesn't run on would be worse than the drift. A backgrounded tab now re-anchors to the grid on return instead of dumping every missed step into one burst. Generative behavior (chord walk, mood sets, energy-driven arp gain) and the stop-ramp contract are unchanged.
 - **[issue-3400] Rendering two storyboard scenes at once no longer loses one of the jobs.** `enqueueStoryboardSceneVideo`, `enqueueStoryboardShotStartFrame`, and `refineStoryboardScenePrompt` each read `stages.storyboards.scenes` up front, then wrote the whole pre-read array back through `updateStage`. That write *is* serialized, but the array it shallow-merges was built outside the lock — so two enqueues for different scenes that both read before either wrote would revert each other's freshly-stamped `sceneVideoJobId` / `startFrameJobId`, and the losing render would run to completion with nothing left pointing at it. All three now patch a single scene through `updateStageWithLatest`, so the read-modify-write of `scenes` happens inside the write region and only the targeted index is replaced. A scene that disappeared between the caller's read and the write now 404s instead of resurrecting itself.
 - **[issue-3407] Stacker News sync works again.** Stacker News renamed its territory listing route from `/recent` to `/new`, so every browser-transport sync was refused with "landed on a different page than the one requested" — the redirect guard correctly caught the rename. Syncs now request the new route directly; the guard stays strict.

server/services/taskSchedule.js

@@ -230,6 +230,29 @@ export const SELF_IMPROVEMENT_TASK_TYPES = [
   // overrides across; do not re-add it as a scheduled type.
 ];
 
+// Shared taskMetadata posture for the NON-COMMITTING COORDINATOR types
+// (NON_COMMITTING_COORDINATOR_TASK_TYPES in taskTypeHooks.js — branch-cleanup /
+// branch-reconcile / issue-reconcile / jira-status-report). Each delivers its work
+// as a SIDE EFFECT in the app's live checkout — a deleted branch, a merged PR, a
+// relabeled issue, a posted report — and by design produces no commit of its own.
+// Two code-shipping criteria therefore have to be switched off or every SUCCESSFUL
+// run is recorded as a failure:
+//   * `useWorktree`/`openPR` — explicitly false (not merely absent) so
+//     applyAppWorktreeDefault's `=== undefined` checks can't fill them from the
+//     app's `defaultOpenPR`/`defaultUseWorktree`. A worktree these tasks never cd
+//     into is at best unused and at worst harmful (it holds refs branch-cleanup is
+//     trying to delete), and the implied PR expectation fails the run at finalize
+//     with `pr-missing` — there is no code to open a PR for. Locked in
+//     MANAGED_AGENT_OPTIONS below so a per-app override can't re-attach them.
+//     `readOnly: true` is NOT a substitute: it gates worktree CREATION
+//     (agentWorkspacePrep.js) but the PR-claim check reads `metadata.openPR`
+//     directly (agentTuiSpawning.js).
+//   * `worktreeChangesExpected` — a clean tree IS the success shape here, so the
+//     TUI idle-complete changes gate must not read it as "produced no work".
+// A guard test in taskSchedule.test.js asserts every member of that set carries
+// this posture, so a new coordinator type can't be added to one list only.
+const NON_COMMITTING_COORDINATOR_METADATA = { useWorktree: false, openPR: false, worktreeChangesExpected: false };
+
 // Shared config for code-reviewer-a and code-reviewer-b (two instances for independent provider/model configuration)
 const CODE_REVIEWER_INTERVAL = { type: INTERVAL_TYPES.WEEKLY, enabled: false, weekdaysOnly: true, providerId: null, model: null, prompt: null, taskMetadata: { useWorktree: true, openPR: true, simplify: true, pipeline: { stages: [{ name: 'Codebase Review', promptKey: 'code-reviewer-review', readOnly: true, providerId: null, model: null, precondition: { fileNotExists: 'REVIEW.md' } }, { name: 'Triage & Implement', promptKey: 'code-reviewer-implement', readOnly: false, providerId: null, model: null, precondition: { fileExists: 'REVIEW.md' } }] } } };
 
@@ -239,7 +262,10 @@ export const DEFAULT_TASK_INTERVALS = {
   'test-coverage':       { type: INTERVAL_TYPES.WEEKLY, enabled: false, providerId: null, model: null, prompt: null },
   'performance':         { type: INTERVAL_TYPES.WEEKLY, enabled: false, providerId: null, model: null, prompt: null },
   'accessibility':       { type: INTERVAL_TYPES.ONCE, enabled: false, providerId: null, model: null, prompt: null },
-  'branch-cleanup':      { type: INTERVAL_TYPES.WEEKLY, enabled: false, providerId: null, model: null, prompt: null },
+  // branch-cleanup deletes merged local/remote branches in the app's LIVE checkout
+  // (its prompt `cd`s to {repoPath} and runs `git branch -d` / `git push --delete`),
+  // hence the shared non-committing-coordinator posture above.
+  'branch-cleanup':      { type: INTERVAL_TYPES.WEEKLY, enabled: false, providerId: null, model: null, prompt: null, taskMetadata: { ...NON_COMMITTING_COORDINATOR_METADATA } },
   // branch-reconcile finishes THIS machine's in-flight LOCAL branches per app
   // (open a PR for pushed-but-unopened work, resolve merge conflicts, drive the
   // review loop, auto-merge when green) AFTER a deterministic pass that removes
@@ -253,9 +279,11 @@ export const DEFAULT_TASK_INTERVALS = {
   // as unfinished. useWorktree/openPR are LOCKED off (MANAGED_AGENT_OPTIONS):
   // the coordinator runs in the app's live checkout so it can see + operate on the
   // sibling worktrees; a CoS-managed worktree would hide the branches and could
-  // trigger cleanupAgentWorktree's auto-merge. Off by default — enabling it is the
-  // user's explicit consent to let it drive PRs on a schedule.
-  'branch-reconcile':    { type: INTERVAL_TYPES.PERPETUAL, enabled: false, providerId: null, model: null, prompt: null, recheckCron: '0 3 * * *', taskMetadata: { useWorktree: false, openPR: false, cleanupMerged: true, openPr: true, resolveConflicts: true, autoMerge: true, finishAbandoned: true } },
+  // trigger cleanupAgentWorktree's auto-merge. Its edits likewise land in those
+  // SIBLING worktrees, never in its own cwd — hence the shared non-committing
+  // -coordinator posture above. Off by default — enabling it is the user's
+  // explicit consent to let it drive PRs on a schedule.
+  'branch-reconcile':    { type: INTERVAL_TYPES.PERPETUAL, enabled: false, providerId: null, model: null, prompt: null, recheckCron: '0 3 * * *', taskMetadata: { ...NON_COMMITTING_COORDINATOR_METADATA, cleanupMerged: true, openPr: true, resolveConflicts: true, autoMerge: true, finishAbandoned: true } },
   // issue-reconcile heals ZOMBIE issues: open + `in-progress` (claimed) yet with
   // their PR already MERGED and no live claim anywhere — a partial ship left the
   // claim marker on, so the queue (which skips `in-progress`) never re-picks the
@@ -267,10 +295,11 @@ export const DEFAULT_TASK_INTERVALS = {
   // follow-up when the remainder is separable, else comment "done/remaining" +
   // release the claim). `autoClose` (ON unless explicitly false) is the only
   // per-app toggle: OFF forbids closing/filing — comment + unlabel only.
-  // useWorktree/openPR are LOCKED off (MANAGED_AGENT_OPTIONS): the coordinator
-  // works over `gh` (no code changes, no worktree). Off by default — enabling it
-  // is the user's explicit consent to let it mutate issue state on a schedule.
-  'issue-reconcile':     { type: INTERVAL_TYPES.PERPETUAL, enabled: false, providerId: null, model: null, prompt: null, recheckCron: '0 4 * * *', taskMetadata: { useWorktree: false, openPR: false, autoClose: true } },
+  // The coordinator works purely over `gh` — no code changes, no worktree, and
+  // issue-state mutation is its whole deliverable — hence the shared
+  // non-committing-coordinator posture above. Off by default — enabling it is the
+  // user's explicit consent to let it mutate issue state on a schedule.
+  'issue-reconcile':     { type: INTERVAL_TYPES.PERPETUAL, enabled: false, providerId: null, model: null, prompt: null, recheckCron: '0 4 * * *', taskMetadata: { ...NON_COMMITTING_COORDINATOR_METADATA, autoClose: true } },
   'console-errors':      { type: INTERVAL_TYPES.ROTATION, enabled: false, providerId: null, model: null, prompt: null },
   'dependency-updates':  { type: INTERVAL_TYPES.WEEKLY, enabled: false, providerId: null, model: null, prompt: null },
   'documentation':       { type: INTERVAL_TYPES.ONCE, enabled: false, providerId: null, model: null, prompt: null },
@@ -322,7 +351,12 @@ export const DEFAULT_TASK_INTERVALS = {
   'code-reviewer-a':     { ...CODE_REVIEWER_INTERVAL },
   'code-reviewer-b':     { ...CODE_REVIEWER_INTERVAL },
   'jira-sprint-manager': { type: INTERVAL_TYPES.DAILY, enabled: false, weekdaysOnly: true, providerId: null, model: null, prompt: null, taskMetadata: { useWorktree: true, openPR: true, simplify: true } },
-  'jira-status-report':  { type: INTERVAL_TYPES.WEEKLY, enabled: false, weekdaysOnly: true, providerId: null, model: null, prompt: null, taskMetadata: { readOnly: true } },
+  // jira-status-report posts its report to JIRA and edits nothing in the repo, so it
+  // takes the shared non-committing-coordinator posture above. `readOnly: true` alone
+  // was NOT enough: it skips worktree creation but leaves `openPR` free to be filled
+  // from the app's `defaultOpenPR`, and the finalize-time PR-claim check reads
+  // `metadata.openPR` directly — scoring a posted report as `pr-missing`.
+  'jira-status-report':  { type: INTERVAL_TYPES.WEEKLY, enabled: false, weekdaysOnly: true, providerId: null, model: null, prompt: null, taskMetadata: { ...NON_COMMITTING_COORDINATOR_METADATA, readOnly: true } },
   // do-replan audits PLAN.md after open PRs and stale branches have been cleaned up,
   // so the plan reflects what actually merged.
   'do-replan':           { type: INTERVAL_TYPES.WEEKLY, enabled: false, providerId: null, model: null, prompt: null, runAfter: ['pr-reviewer', 'branch-cleanup'], taskMetadata: { useWorktree: true, openPR: true } },
@@ -381,15 +415,27 @@ export const DEFAULT_TASK_INTERVALS = {
 // CoS-managed worktree would clobber it).
 export const MANAGED_AGENT_OPTIONS = {
   'plan-task': ['useWorktree', 'openPR'],
-  // branch-reconcile's coordinator MUST run in the app's live checkout (never a
-  // CoS-managed worktree) so it can enumerate + operate on the sibling worktrees
-  // of the in-flight branches; a managed worktree would hide those branches from
-  // the scan AND could trip cleanupAgentWorktree's auto-merge. Lock both off.
-  'branch-reconcile': ['useWorktree', 'openPR'],
-  // issue-reconcile's coordinator works purely over `gh` (issue label/state +
-  // follow-up filing) — it makes no code changes, so it needs neither a worktree
-  // nor a PR. Lock both off so a hand-edited config can't attach one.
-  'issue-reconcile': ['useWorktree', 'openPR'],
+  // The non-committing coordinators (NON_COMMITTING_COORDINATOR_METADATA above) all
+  // run in the app's LIVE checkout and ship no code, so a CoS-managed worktree is at
+  // best unused and at worst harmful — branch-reconcile needs to see the sibling
+  // worktrees of the in-flight branches (a managed worktree hides them from the scan
+  // AND could trip cleanupAgentWorktree's auto-merge), and branch-cleanup's worktree
+  // would hold refs it is trying to delete. Locking both off also keeps the
+  // finalize-time PR-claim check from scoring a completed run `pr-missing`.
+  //
+  // `worktreeChangesExpected` is managed for these four as well — the one type-keyed
+  // exception to it being a free per-app override. A clean tree is definitionally the
+  // success shape for a branch deletion or a posted report, so setting it back to
+  // `true` can only make successful runs fail; it is exactly as non-negotiable here as
+  // the other two. Managing it is also what carries it through an explicit
+  // `taskMetadata: null` clear — loadSchedule preserves that null (skipping the
+  // defaults deep-merge), and enforceManagedAgentOptions rebuilds only the MANAGED
+  // fields, so an unmanaged `worktreeChangesExpected` would silently go absent and the
+  // TUI idle-complete gate would fail the run as `idle-no-changes`.
+  ...Object.fromEntries(
+    ['branch-reconcile', 'branch-cleanup', 'issue-reconcile', 'jira-status-report']
+      .map((t) => [t, ['useWorktree', 'openPR', 'worktreeChangesExpected']])
+  ),
   // claim-issue's prompt creates its own claim/issue-<num> worktree (same
   // rationale as plan-task), so CoS must not pre-create one or open the PR.
   'claim-issue': ['useWorktree', 'openPR'],