diff --git a/.changelog/NEXT.md b/.changelog/NEXT.md index 18c866569..a51d17a40 100644 --- a/.changelog/NEXT.md +++ b/.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. diff --git a/server/services/taskSchedule.js b/server/services/taskSchedule.js index 46d7786a5..65b246b1e 100644 --- a/server/services/taskSchedule.js +++ b/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- worktree (same // rationale as plan-task), so CoS must not pre-create one or open the PR. 'claim-issue': ['useWorktree', 'openPR'], diff --git a/server/services/taskSchedule.test.js b/server/services/taskSchedule.test.js index 9b791ed13..dea738210 100644 --- a/server/services/taskSchedule.test.js +++ b/server/services/taskSchedule.test.js @@ -134,6 +134,7 @@ import { PROMPT_VERSIONS, DEFAULT_TASK_INTERVALS, MANAGED_AGENT_OPTIONS, + stripManagedAgentOptionsFromOverride, TASK_TYPE_DESCRIPTIONS, REFERENCE_WATCH_AUDITED_VERSION } from './taskSchedule.js' @@ -147,6 +148,10 @@ import { import { DEFAULT_TASK_PROMPTS, PREVIOUS_DEFAULT_PROMPTS } from './taskPromptDefaults.js' +// The source of truth for "this type's deliverable is a side effect, not a commit" +// — the posture guard below iterates it so the two can't drift apart. +import { NON_COMMITTING_COORDINATOR_TASK_TYPES } from './taskTypeHooks.js' + import { loadState } from './cosState.js' import { readJSONFile } from '../lib/fileUtils.js' @@ -1092,6 +1097,83 @@ describe('taskSchedule', () => { }) }) + // The gh/git coordinator types deliver their work as a SIDE EFFECT (a deleted + // branch, a merged PR, a relabeled issue, a posted report) in the app's live + // checkout — never as a commit in a CoS-managed worktree. Two code-shipping + // criteria have to be switched off for them or every successful run is recorded + // as a failure: `openPR` (→ `pr-missing` at finalization, since there is no code + // to open a PR for) and `worktreeChangesExpected` (→ `idle-no-changes` at the TUI + // idle-complete gate, since a clean tree IS the success shape). + describe('non-committing coordinator posture', () => { + // Driven off NON_COMMITTING_COORDINATOR_TASK_TYPES — the same set that grants + // these types their commit-criterion exemption — rather than a hand-typed list, + // so a FUTURE coordinator type added there can't silently ship without the + // posture. That drift is exactly how `branch-cleanup` shipped with no + // taskMetadata at all, letting an app-level `defaultOpenPR: true` attach a + // worktree + PR expectation to a task that only runs `git branch -d`. + it.each([...NON_COMMITTING_COORDINATOR_TASK_TYPES])( + '%s declares no worktree/PR, expects a clean tree, and locks both flags', + (taskType) => { + const meta = DEFAULT_TASK_INTERVALS[taskType].taskMetadata + // Explicitly false, NOT merely absent — applyAppWorktreeDefault fills these + // from the app's defaults on an `=== undefined` check, so an absent key is + // the bug, not a pass. + expect(meta.openPR).toBe(false) + expect(meta.useWorktree).toBe(false) + expect(meta.worktreeChangesExpected).toBe(false) + // Locked, so a per-app override can't re-attach what the defaults cleared. + // worktreeChangesExpected is managed too — see MANAGED_AGENT_OPTIONS. + expect(MANAGED_AGENT_OPTIONS[taskType]).toEqual(['useWorktree', 'openPR', 'worktreeChangesExpected']) + } + ) + + // An explicit `taskMetadata: null` (accepted by PUT /api/cos/schedule as "clear + // it") makes loadSchedule SKIP the defaults deep-merge, so enforceManagedAgentOptions + // is the only thing that rebuilds the bag — and it rebuilds MANAGED fields only. + // Before worktreeChangesExpected was managed, it went absent here and a successful + // clean-tree run was scored `idle-no-changes` all over again. + it.each([...NON_COMMITTING_COORDINATOR_TASK_TYPES])( + '%s keeps the full posture when stored taskMetadata was cleared to null', + async (taskType) => { + mockSchedule({ + tasks: { + [taskType]: { type: 'cron', enabled: true, providerId: null, model: null, prompt: null, taskMetadata: null } + } + }) + + const meta = (await loadSchedule()).tasks[taskType].taskMetadata + expect(meta.useWorktree).toBe(false) + expect(meta.openPR).toBe(false) + expect(meta.worktreeChangesExpected).toBe(false) + } + ) + + it('forces branch-cleanup useWorktree/openPR back off when stored true (loadSchedule)', async () => { + mockSchedule({ + tasks: { + 'branch-cleanup': { + type: 'cron', + enabled: true, + providerId: null, + model: null, + prompt: null, + taskMetadata: { useWorktree: true, openPR: true } + } + } + }) + + const schedule = await loadSchedule() + expect(schedule.tasks['branch-cleanup'].taskMetadata.useWorktree).toBe(false) + expect(schedule.tasks['branch-cleanup'].taskMetadata.openPR).toBe(false) + // Backfilled from the defaults for installs whose stored config predates it. + expect(schedule.tasks['branch-cleanup'].taskMetadata.worktreeChangesExpected).toBe(false) + }) + + it('strips a per-app branch-cleanup worktree/PR override', () => { + expect(stripManagedAgentOptionsFromOverride('branch-cleanup', { useWorktree: true, openPR: true })).toBeNull() + }) + }) + describe('claim-issue defaults', () => { it('is registered as a self-improvement task type', () => { expect(SELF_IMPROVEMENT_TASK_TYPES).toContain('claim-issue')