From 9a434a951f7abcfd8e269b4c99c2d14035e5e48d Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Sat, 21 Mar 2026 17:37:09 +0000 Subject: [PATCH 01/19] feat(cos): split worktree and PR configs into independent settings Split the combined "Worktree + PR" toggle into two separate configs: - useWorktree: work in an isolated git worktree on a feature branch - openPR: open a pull request to the default branch (implies worktree) Behavior matrix: - Neither: commit directly to default branch - Worktree only: work in worktree, auto-merge on completion - Open PR (implies worktree): work in worktree, push + create PR App-level defaults (defaultUseWorktree, defaultOpenPR) cascade into manual and scheduled task metadata with per-task override support. --- client/src/components/apps/EditAppModal.jsx | 23 ++++++- client/src/components/cos/TaskAddForm.jsx | 30 +++++++-- client/src/components/cos/constants.js | 5 +- server/lib/validation.js | 3 +- server/routes/cos.js | 4 +- server/services/cos.js | 10 ++- server/services/subAgentSpawner.js | 70 ++++++++++++++++++--- server/services/taskSchedule.js | 4 +- 8 files changed, 123 insertions(+), 26 deletions(-) diff --git a/client/src/components/apps/EditAppModal.jsx b/client/src/components/apps/EditAppModal.jsx index 035a9663..c645ea69 100644 --- a/client/src/components/apps/EditAppModal.jsx +++ b/client/src/components/apps/EditAppModal.jsx @@ -1,6 +1,6 @@ import { useState, useEffect } from 'react'; import { Link } from 'react-router-dom'; -import { ChevronDown, ChevronUp, GitBranch } from 'lucide-react'; +import { ChevronDown, ChevronUp, GitBranch, GitPullRequest } from 'lucide-react'; import IconPicker from '../IconPicker'; import * as api from '../../services/api'; @@ -17,6 +17,7 @@ export default function EditAppModal({ app, onClose, onSave }) { pm2ProcessNames: (app.pm2ProcessNames || []).join(', '), editorCommand: app.editorCommand || 'code .', defaultUseWorktree: app.defaultUseWorktree || false, + defaultOpenPR: app.defaultOpenPR || false, jiraEnabled: app.jira?.enabled || false, jiraInstanceId: app.jira?.instanceId || '', jiraProjectKey: app.jira?.projectKey || '', @@ -91,6 +92,7 @@ export default function EditAppModal({ app, onClose, onSave }) { : undefined, editorCommand: formData.editorCommand || undefined, defaultUseWorktree: formData.defaultUseWorktree, + defaultOpenPR: formData.defaultOpenPR, jira: formData.jiraEnabled ? { enabled: true, instanceId: formData.jiraInstanceId || undefined, @@ -242,11 +244,26 @@ export default function EditAppModal({ app, onClose, onSave }) { setFormData({ ...formData, defaultUseWorktree: e.target.checked })} + onChange={e => { + const updates = { defaultUseWorktree: e.target.checked }; + if (!e.target.checked) updates.defaultOpenPR = false; + setFormData(prev => ({ ...prev, ...updates })); + }} className="rounded border-port-border bg-port-bg text-port-accent focus:ring-port-accent" /> - Default to Worktree + PR for new tasks + Default to Worktree for new tasks + + {/* JIRA Integration Section */} diff --git a/client/src/components/cos/TaskAddForm.jsx b/client/src/components/cos/TaskAddForm.jsx index cfdaf9b1..5cda3ba5 100644 --- a/client/src/components/cos/TaskAddForm.jsx +++ b/client/src/components/cos/TaskAddForm.jsx @@ -1,5 +1,5 @@ import { useState, useEffect, useRef, useMemo, useCallback } from 'react'; -import { Plus, Image, X, ChevronDown, ChevronRight, Sparkles, Loader2, Paperclip, FileText, Zap, Bookmark, Ticket, GitBranch, Wand2, RefreshCw } from 'lucide-react'; +import { Plus, Image, X, ChevronDown, ChevronRight, Sparkles, Loader2, Paperclip, FileText, Zap, Bookmark, Ticket, GitBranch, GitPullRequest, Wand2, RefreshCw } from 'lucide-react'; import toast from 'react-hot-toast'; import * as api from '../../services/api'; import { processScreenshotUploads, processAttachmentUploads } from '../../utils/fileUpload'; @@ -11,6 +11,7 @@ export default function TaskAddForm({ providers, apps, onTaskAdded, compact = fa const [enhancePrompt, setEnhancePrompt] = useState(false); const [isEnhancing, setIsEnhancing] = useState(false); const [useWorktree, setUseWorktree] = useState(false); + const [openPR, setOpenPR] = useState(false); const [simplify, setSimplify] = useState(true); const [reviewLoop, setReviewLoop] = useState(false); const [createJiraTicket, setCreateJiraTicket] = useState(false); @@ -45,11 +46,12 @@ export default function TaskAddForm({ providers, apps, onTaskAdded, compact = fa ); const appHasJira = selectedApp?.jira?.enabled; - // Auto-toggle JIRA and worktree checkboxes when app selection changes + // Auto-toggle JIRA, worktree, and PR checkboxes when app selection changes useEffect(() => { const app = apps?.find(a => a.id === newTask.app); setCreateJiraTicket(!!app?.jira?.enabled); setUseWorktree(!!app?.defaultUseWorktree); + setOpenPR(!!app?.defaultOpenPR); }, [newTask.app, apps]); // Get models for selected provider @@ -184,6 +186,7 @@ export default function TaskAddForm({ providers, apps, onTaskAdded, compact = fa app: newTask.app || undefined, createJiraTicket: createJiraTicket || undefined, useWorktree: useWorktree || undefined, + openPR: openPR || undefined, simplify: simplify || undefined, reviewLoop: reviewLoop || undefined, screenshots: screenshots.length > 0 ? screenshots.map(s => s.path) : undefined, @@ -211,6 +214,7 @@ export default function TaskAddForm({ providers, apps, onTaskAdded, compact = fa setEnhancePrompt(false); setCreateJiraTicket(false); setUseWorktree(false); + setOpenPR(false); setSimplify(true); setReviewLoop(false); setExpanded(false); @@ -377,12 +381,28 @@ export default function TaskAddForm({ providers, apps, onTaskAdded, compact = fa setUseWorktree(e.target.checked)} + onChange={(e) => { + setUseWorktree(e.target.checked); + if (!e.target.checked) setOpenPR(false); + }} className="w-4 h-4 rounded border-port-border bg-port-bg text-port-accent focus:ring-port-accent focus:ring-offset-0" /> - + - Worktree + PR + Worktree + + + diff --git a/client/src/components/cos/constants.js b/client/src/components/cos/constants.js index c90be8fb..c170bfd1 100644 --- a/client/src/components/cos/constants.js +++ b/client/src/components/cos/constants.js @@ -54,7 +54,7 @@ export const AGENT_OPTIONS = [ { field: 'useWorktree', label: 'Worktree', shortLabel: 'WT', description: 'Work in an isolated git worktree on a feature branch. If unchecked, commits directly to the default branch.' }, { field: 'openPR', label: 'Open PR', shortLabel: 'PR', description: 'Open a pull request to the default branch (implies worktree). If unchecked with worktree enabled, auto-merges to the default branch on completion.' }, { field: 'simplify', label: 'Run /simplify', shortLabel: '/s', description: 'Review code for reuse and quality before committing' }, - { field: 'reviewLoop', label: 'Review Loop', shortLabel: 'RL', description: 'After opening a PR, run review feedback loop until checks pass and PR is approved' } + { field: 'reviewLoop', label: 'Review Loop', shortLabel: 'RL', description: 'After the agent opens a PR during its run, keep iterating on review feedback until checks pass. Only applies when Open PR is not enabled (manual PR creation by agent).' } ]; // Compute new taskMetadata after toggling a field in a per-app override. From b4e6f7c2d7eb3380b3b8ebce7faea5bd31c969da Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Sat, 21 Mar 2026 18:17:13 +0000 Subject: [PATCH 07/19] fix(cos): robust boolean handling for task metadata round-trip - Only persist true to TASKS.md metadata (not false) to avoid 'false' string being truthy when parsed back - Add isFalsyMeta helper for symmetric string coercion - Apply toBool to createJiraTicket in routes - Send boolean values directly from TaskAddForm (not || undefined) - Fix stale comment about auto-merge fallback --- client/src/components/cos/TaskAddForm.jsx | 10 ++++----- server/routes/cos.js | 2 +- server/services/cos.js | 8 +++----- server/services/subAgentSpawner.js | 7 ++++--- server/services/subAgentSpawner.test.js | 25 ++++++++++++++++++++++- 5 files changed, 37 insertions(+), 15 deletions(-) diff --git a/client/src/components/cos/TaskAddForm.jsx b/client/src/components/cos/TaskAddForm.jsx index 8092296f..e9414fa9 100644 --- a/client/src/components/cos/TaskAddForm.jsx +++ b/client/src/components/cos/TaskAddForm.jsx @@ -186,11 +186,11 @@ export default function TaskAddForm({ providers, apps, onTaskAdded, compact = fa model: newTask.model || undefined, provider: newTask.provider || undefined, app: newTask.app || undefined, - createJiraTicket: createJiraTicket || undefined, - useWorktree: useWorktree || undefined, - openPR: openPR || undefined, - simplify: simplify || undefined, - reviewLoop: reviewLoop || undefined, + createJiraTicket, + useWorktree, + openPR, + simplify, + reviewLoop, screenshots: screenshots.length > 0 ? screenshots.map(s => s.path) : undefined, attachments: attachments.length > 0 ? attachments.map(a => ({ filename: a.filename, diff --git a/server/routes/cos.js b/server/routes/cos.js index d590ac2d..d6658264 100644 --- a/server/routes/cos.js +++ b/server/routes/cos.js @@ -199,7 +199,7 @@ router.post('/tasks', asyncHandler(async (req, res) => { // Coerce boolean flags — values from req.body may arrive as strings like 'false' (truthy in JS) const toBool = (v) => v === true || v === 'true' ? true : v === false || v === 'false' ? false : undefined; - const taskData = { description, priority, context, model, provider, app, approvalRequired, screenshots, attachments, position, createJiraTicket, jiraTicketId, jiraTicketUrl, useWorktree: toBool(useWorktree), openPR: toBool(openPR), simplify: toBool(simplify), reviewLoop: toBool(reviewLoop) }; + const taskData = { description, priority, context, model, provider, app, approvalRequired, screenshots, attachments, position, createJiraTicket: toBool(createJiraTicket), jiraTicketId, jiraTicketUrl, useWorktree: toBool(useWorktree), openPR: toBool(openPR), simplify: toBool(simplify), reviewLoop: toBool(reviewLoop) }; const result = await cos.addTask(taskData, type); if (result?.duplicate) { diff --git a/server/services/cos.js b/server/services/cos.js index f6fce23a..2a81f2cf 100644 --- a/server/services/cos.js +++ b/server/services/cos.js @@ -3204,15 +3204,13 @@ export async function addTask(taskData, taskType = 'user', { raw = false } = {}) if (taskData.provider) metadata.provider = taskData.provider; if (taskData.app) metadata.app = taskData.app; if (taskData.createJiraTicket) metadata.createJiraTicket = true; - // Boolean flags: persist both true and false explicitly so app-level defaults don't override user intent + // Boolean flags: only persist true values. In TASKS.md, false round-trips as the string + // 'false' which is truthy in JS. Omitting (undefined) means "use app defaults", and + // isTruthyMeta handles 'true' strings correctly for the true case. if (taskData.useWorktree === true) metadata.useWorktree = true; - else if (taskData.useWorktree === false) metadata.useWorktree = false; if (taskData.openPR === true) metadata.openPR = true; - else if (taskData.openPR === false) metadata.openPR = false; if (taskData.simplify === true) metadata.simplify = true; - else if (taskData.simplify === false) metadata.simplify = false; if (taskData.reviewLoop === true) metadata.reviewLoop = true; - else if (taskData.reviewLoop === false) metadata.reviewLoop = false; if (taskData.jiraTicketId) metadata.jiraTicketId = taskData.jiraTicketId; if (taskData.jiraTicketUrl) metadata.jiraTicketUrl = taskData.jiraTicketUrl; if (taskData.screenshots?.length > 0) metadata.screenshots = taskData.screenshots; diff --git a/server/services/subAgentSpawner.js b/server/services/subAgentSpawner.js index 565c167c..069a842e 100644 --- a/server/services/subAgentSpawner.js +++ b/server/services/subAgentSpawner.js @@ -39,8 +39,9 @@ const ROOT_DIR = PATHS.root; const AGENTS_DIR = PATHS.cosAgents; const RUNS_DIR = PATHS.runs; -// Metadata booleans may arrive as true or 'true' (from JSON vs form/URL params) +// Metadata booleans may arrive as true/'true' or false/'false' (from JSON vs TASKS.md string round-trip) export const isTruthyMeta = (value) => value === true || value === 'true'; +export const isFalsyMeta = (value) => value === false || value === 'false'; /** * Extract task type key for learning lookup @@ -1529,7 +1530,7 @@ export async function spawnAgentForTask(task) { agentId, worktreePath: worktreeInfo.worktreePath, branchName: worktreeInfo.branchName, baseBranch: worktreeInfo.baseBranch }); } - } else if (!jiraBranchName && task.metadata?.useWorktree !== false) { + } else if (!jiraBranchName && !isFalsyMeta(task.metadata?.useWorktree)) { // No explicit worktree requested and not explicitly disabled: use worktree only when conflict is detected const { getAgents } = await import('./cos.js'); const allAgents = await getAgents(); @@ -1981,7 +1982,7 @@ async function cleanupAgentWorktree(agentId, success, { openPR = false, descript git.getRepoBranches(sourceWorkspace).catch(() => ({ baseBranch: null, devBranch: null })) ]); - // Only create PR if push succeeded; fall back to auto-merge if push fails + // Only create PR if push succeeded; preserve worktree/branch for manual intervention if push fails if (pushResult) { const targetBranch = branchInfo.devBranch || branchInfo.baseBranch || 'main'; const taskDesc = description || 'CoS automated task'; diff --git a/server/services/subAgentSpawner.test.js b/server/services/subAgentSpawner.test.js index 73b1bbe7..cf20239f 100644 --- a/server/services/subAgentSpawner.test.js +++ b/server/services/subAgentSpawner.test.js @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest'; -import { isTruthyMeta } from './subAgentSpawner.js'; +import { isTruthyMeta, isFalsyMeta } from './subAgentSpawner.js'; import { applyAppWorktreeDefault } from './cos.js'; /** @@ -696,6 +696,29 @@ describe('Task Failure Retry Logic', () => { }); }); + // --- isFalsyMeta helper (imported from production) --- + describe('isFalsyMeta', () => { + it('should return true for boolean false', () => { + expect(isFalsyMeta(false)).toBe(true); + }); + + it('should return true for string "false"', () => { + expect(isFalsyMeta('false')).toBe(true); + }); + + it('should return false for true', () => { + expect(isFalsyMeta(true)).toBe(false); + }); + + it('should return false for undefined/null/other values', () => { + expect(isFalsyMeta(undefined)).toBe(false); + expect(isFalsyMeta(null)).toBe(false); + expect(isFalsyMeta('true')).toBe(false); + expect(isFalsyMeta(0)).toBe(false); + expect(isFalsyMeta('')).toBe(false); + }); + }); + // --- openPR worktree decision logic (uses imported isTruthyMeta) --- describe('openPR/useWorktree decision logic', () => { function resolveWorktreeFlags(metadata) { From a3f0f949028cda8fe806e130b439d9279e58f3b3 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Sat, 21 Mar 2026 18:26:41 +0000 Subject: [PATCH 08/19] fix(validation): remove .default(false) from appSchema booleans to prevent partial updates overwriting stored values Zod's .default(false) on archived, defaultUseWorktree, and defaultOpenPR caused appUpdateSchema (appSchema.partial()) to inject false for omitted fields, silently overwriting stored true values during partial updates. --- .changelog/NEXT.md | 1 + server/lib/validation.js | 6 +++--- server/lib/validation.test.js | 9 +++++++++ 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/.changelog/NEXT.md b/.changelog/NEXT.md index 9ccf72f2..2d5ffb73 100644 --- a/.changelog/NEXT.md +++ b/.changelog/NEXT.md @@ -41,6 +41,7 @@ - Chart date range comparison uses local date strings instead of UTC Date objects — fixes today/yesterday missing from 7-day chart - Empty entry objects no longer accumulate in daily-log.json after moving the last item from a date - README screenshot labels corrected to match actual screenshot content (all 6 were mislabeled) +- Zod `.default(false)` on `archived`, `defaultUseWorktree`, and `defaultOpenPR` in `appSchema` caused `appUpdateSchema.partial()` to inject `false` for omitted fields, silently overwriting stored `true` values during partial updates ## Removed - Dead code: `euclideanDistance`, `averageVectors`, `similarityMatrix` from `vectorMath.js` (unused outside tests) diff --git a/server/lib/validation.js b/server/lib/validation.js index 8d7c0db9..c8bb8240 100644 --- a/server/lib/validation.js +++ b/server/lib/validation.js @@ -190,15 +190,15 @@ export const appSchema = z.object({ appIconPath: z.string().nullable().optional(), // Absolute path to detected app icon image editorCommand: z.string().optional(), description: z.string().optional(), - archived: z.boolean().optional().default(false), + archived: z.boolean().optional(), pm2Home: z.string().optional(), // Custom PM2_HOME path for apps that run in their own PM2 instance disabledTaskTypes: z.array(z.string()).optional(), // Legacy: migrated to taskTypeOverrides taskTypeOverrides: z.record(z.object({ enabled: z.boolean().optional(), interval: z.string().nullable().optional() })).optional(), // Per-task overrides: { [taskType]: { enabled, interval } } - defaultUseWorktree: z.boolean().optional().default(false), - defaultOpenPR: z.boolean().optional().default(false), + defaultUseWorktree: z.boolean().optional(), + defaultOpenPR: z.boolean().optional(), jira: jiraConfigSchema.optional().nullable(), datadog: datadogConfigSchema.optional().nullable() }); diff --git a/server/lib/validation.test.js b/server/lib/validation.test.js index 13cb1be3..b402c983 100644 --- a/server/lib/validation.test.js +++ b/server/lib/validation.test.js @@ -191,6 +191,15 @@ describe('validation.js', () => { const result = appUpdateSchema.safeParse(update); expect(result.success).toBe(false); }); + + it('should not inject default values for omitted boolean fields', () => { + const update = { name: 'Updated Name' }; + const result = appUpdateSchema.safeParse(update); + expect(result.success).toBe(true); + expect(result.data).not.toHaveProperty('archived'); + expect(result.data).not.toHaveProperty('defaultUseWorktree'); + expect(result.data).not.toHaveProperty('defaultOpenPR'); + }); }); describe('providerSchema', () => { From bcee355b5fe665ba6748b12b48bf7dbb48b48452 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Sat, 21 Mar 2026 18:29:10 +0000 Subject: [PATCH 09/19] fix(cos): address round 3 Copilot review feedback - Clear reviewLoop state when app with defaultOpenPR is selected - Respect explicit openPR:true in applyAppWorktreeDefault cascade - Handle createPR non-throwing failures in openPR cleanup path --- client/src/components/cos/TaskAddForm.jsx | 1 + server/services/cos.js | 6 +++++- server/services/subAgentSpawner.js | 6 ++++++ server/services/subAgentSpawner.test.js | 15 +++++++++++---- 4 files changed, 23 insertions(+), 5 deletions(-) diff --git a/client/src/components/cos/TaskAddForm.jsx b/client/src/components/cos/TaskAddForm.jsx index e9414fa9..c86f60f5 100644 --- a/client/src/components/cos/TaskAddForm.jsx +++ b/client/src/components/cos/TaskAddForm.jsx @@ -54,6 +54,7 @@ export default function TaskAddForm({ providers, apps, onTaskAdded, compact = fa setCreateJiraTicket(!!app?.jira?.enabled); setUseWorktree(defaultUseWorktree); setOpenPR(defaultOpenPR); + if (defaultOpenPR) setReviewLoop(false); }, [newTask.app, apps]); // Get models for selected provider diff --git a/server/services/cos.js b/server/services/cos.js index 2a81f2cf..d8788967 100644 --- a/server/services/cos.js +++ b/server/services/cos.js @@ -1869,7 +1869,11 @@ Use model: claude-opus-4-5-20251101 for thorough security analysis` // Apply app-level worktree/PR defaults only when not already set by task-type metadata export function applyAppWorktreeDefault(metadata, app) { if (metadata.useWorktree === undefined) { - if (app.defaultUseWorktree === true) { + // openPR implies useWorktree — don't let app default override explicit openPR: true + const explicitOpenPR = metadata.openPR === true || metadata.openPR === 'true'; + if (explicitOpenPR) { + metadata.useWorktree = true; + } else if (app.defaultUseWorktree === true) { metadata.useWorktree = true; } else if (app.defaultUseWorktree === false) { metadata.useWorktree = false; diff --git a/server/services/subAgentSpawner.js b/server/services/subAgentSpawner.js index 069a842e..28e27cef 100644 --- a/server/services/subAgentSpawner.js +++ b/server/services/subAgentSpawner.js @@ -1997,6 +1997,12 @@ async function cleanupAgentWorktree(agentId, success, { openPR = false, descript return null; }); + if (prResult?.success === false) { + emitLog('error', `🌳 PR creation failed for ${worktreeBranch}: ${prResult.error}`, { agentId, branchName: worktreeBranch }); + // Preserve worktree/branch for manual PR creation + return; + } + if (prResult?.success) { emitLog('success', `🌳 Created PR: ${prResult.url}`, { agentId, branchName: worktreeBranch }); } diff --git a/server/services/subAgentSpawner.test.js b/server/services/subAgentSpawner.test.js index cf20239f..5c5d2941 100644 --- a/server/services/subAgentSpawner.test.js +++ b/server/services/subAgentSpawner.test.js @@ -793,12 +793,19 @@ describe('Task Failure Retry Logic', () => { expect(metadata.openPR).toBe(true); }); - it('should apply defaultUseWorktree without affecting already-set openPR', () => { + it('should not let defaultUseWorktree:false override explicit openPR:true', () => { const metadata = { openPR: true }; applyAppWorktreeDefault(metadata, { defaultUseWorktree: false }); - // useWorktree was unset, gets filled with false; invariant enforces openPR=false - expect(metadata.useWorktree).toBe(false); - expect(metadata.openPR).toBe(false); + // openPR implies useWorktree — app default must not override explicit openPR + expect(metadata.useWorktree).toBe(true); + expect(metadata.openPR).toBe(true); + }); + + it('should handle openPR:"true" string the same as boolean true', () => { + const metadata = { openPR: 'true' }; + applyAppWorktreeDefault(metadata, { defaultUseWorktree: false }); + expect(metadata.useWorktree).toBe(true); + expect(metadata.openPR).toBe('true'); }); it('should leave metadata unchanged when app has no defaults', () => { From 80d532746cd888ff2d1ad49da01efb53580b65d4 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Sat, 21 Mar 2026 18:43:31 +0000 Subject: [PATCH 10/19] fix(cos): address round 4 Copilot review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Enforce openPR→useWorktree invariant in toggleAppMetadataOverride and ScheduleTab - Normalize EditAppModal state so defaultOpenPR implies defaultUseWorktree - Add 16 unit tests for cleanupAgentWorktree openPR path - Export cleanupAgentWorktree for testability --- .changelog/NEXT.md | 1 + client/src/components/apps/EditAppModal.jsx | 4 +- client/src/components/cos/constants.js | 20 + .../src/components/cos/tabs/ScheduleTab.jsx | 9 + server/services/cleanupAgentWorktree.test.js | 371 ++++++++++++++++++ server/services/subAgentSpawner.js | 2 +- 6 files changed, 404 insertions(+), 3 deletions(-) create mode 100644 server/services/cleanupAgentWorktree.test.js diff --git a/.changelog/NEXT.md b/.changelog/NEXT.md index 2d5ffb73..8475b2fa 100644 --- a/.changelog/NEXT.md +++ b/.changelog/NEXT.md @@ -17,6 +17,7 @@ - Personal Goals dashboard widget: shows top-level goals with progress bars, category icons, horizon labels, and stall detection (14+ days idle) — replaces broken CoS task-based goal widget that never rendered ## Changed +- Exported `cleanupAgentWorktree` from `subAgentSpawner.js` with dedicated test file covering the openPR completion path (push, PR creation, failure preservation, auto-merge fallback) - Exported `isTruthyMeta` from `subAgentSpawner.js` and `applyAppWorktreeDefault` from `cos.js`; tests now import production helpers instead of duplicating their logic inline - Consolidated 5 duplicate `timeAgo`/`formatTimeAgo` implementations into a single `timeAgo()` in `utils/formatters.js` - Deduplicated `stripCodeFences` from insightsService — now imports from shared `lib/aiProvider.js` diff --git a/client/src/components/apps/EditAppModal.jsx b/client/src/components/apps/EditAppModal.jsx index c645ea69..29d2e46a 100644 --- a/client/src/components/apps/EditAppModal.jsx +++ b/client/src/components/apps/EditAppModal.jsx @@ -16,8 +16,8 @@ export default function EditAppModal({ app, onClose, onSave }) { startCommands: (app.startCommands || []).join('\n'), pm2ProcessNames: (app.pm2ProcessNames || []).join(', '), editorCommand: app.editorCommand || 'code .', - defaultUseWorktree: app.defaultUseWorktree || false, defaultOpenPR: app.defaultOpenPR || false, + defaultUseWorktree: app.defaultUseWorktree || app.defaultOpenPR || false, jiraEnabled: app.jira?.enabled || false, jiraInstanceId: app.jira?.instanceId || '', jiraProjectKey: app.jira?.projectKey || '', @@ -91,7 +91,7 @@ export default function EditAppModal({ app, onClose, onSave }) { ? formData.pm2ProcessNames.split(',').map(s => s.trim()).filter(Boolean) : undefined, editorCommand: formData.editorCommand || undefined, - defaultUseWorktree: formData.defaultUseWorktree, + defaultUseWorktree: formData.defaultUseWorktree || formData.defaultOpenPR, defaultOpenPR: formData.defaultOpenPR, jira: formData.jiraEnabled ? { enabled: true, diff --git a/client/src/components/cos/constants.js b/client/src/components/cos/constants.js index c170bfd1..8927f80c 100644 --- a/client/src/components/cos/constants.js +++ b/client/src/components/cos/constants.js @@ -59,6 +59,8 @@ export const AGENT_OPTIONS = [ // Compute new taskMetadata after toggling a field in a per-app override. // Returns null when all overrides are cleared (inherit everything). +// Enforces invariant: openPR implies useWorktree (turning on openPR forces +// useWorktree on; turning off useWorktree forces openPR off). export function toggleAppMetadataOverride(overrideMetadata, globalMetadata, field) { const current = overrideMetadata || {}; const newMeta = { ...current }; @@ -68,6 +70,24 @@ export function toggleAppMetadataOverride(overrideMetadata, globalMetadata, fiel const effective = overrideMetadata?.[field] ?? globalMetadata?.[field] ?? false; newMeta[field] = !effective; } + + const resolve = (f) => newMeta[f] ?? globalMetadata?.[f] ?? false; + + // openPR requires useWorktree + if (resolve('openPR') && !resolve('useWorktree')) { + newMeta.useWorktree = true; + } + // useWorktree off means openPR must be off + if (!resolve('useWorktree') && resolve('openPR')) { + newMeta.openPR = false; + } + + // Clean entries that match the global value (revert to inherit) + for (const key of Object.keys(newMeta)) { + if (newMeta[key] === (globalMetadata?.[key] ?? false)) { + delete newMeta[key]; + } + } return Object.keys(newMeta).length ? newMeta : null; } diff --git a/client/src/components/cos/tabs/ScheduleTab.jsx b/client/src/components/cos/tabs/ScheduleTab.jsx index 203a3290..9844842e 100644 --- a/client/src/components/cos/tabs/ScheduleTab.jsx +++ b/client/src/components/cos/tabs/ScheduleTab.jsx @@ -23,9 +23,18 @@ const INTERVAL_DESCRIPTIONS = { custom: 'Custom interval' }; +// Toggle a global taskMetadata field, enforcing the openPR→useWorktree invariant. function toggleMetadataField(metadata, field) { const current = metadata || {}; const newMeta = { ...current, [field]: !current[field] }; + // openPR requires useWorktree + if (newMeta.openPR && !newMeta.useWorktree) { + newMeta.useWorktree = true; + } + // useWorktree off means openPR must be off + if (newMeta.useWorktree === false && newMeta.openPR) { + newMeta.openPR = false; + } const active = Object.fromEntries(Object.entries(newMeta).filter(([, v]) => v)); return Object.keys(active).length ? active : null; } diff --git a/server/services/cleanupAgentWorktree.test.js b/server/services/cleanupAgentWorktree.test.js new file mode 100644 index 00000000..7446ae8b --- /dev/null +++ b/server/services/cleanupAgentWorktree.test.js @@ -0,0 +1,371 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +// --- Mock every dependency subAgentSpawner.js imports --- + +vi.mock('child_process', () => ({ + spawn: vi.fn(), + execSync: vi.fn() +})); + +vi.mock('fs/promises', () => ({ + writeFile: vi.fn().mockResolvedValue(undefined), + mkdir: vi.fn().mockResolvedValue(undefined), + readFile: vi.fn().mockResolvedValue(''), + readdir: vi.fn().mockResolvedValue([]), + rm: vi.fn().mockResolvedValue(undefined), + stat: vi.fn().mockResolvedValue({ isDirectory: () => true }) +})); + +vi.mock('fs', () => ({ + existsSync: vi.fn(() => false) +})); + +vi.mock('uuid', () => ({ + v4: vi.fn(() => 'mock-uuid') +})); + +vi.mock('./cos.js', () => ({ + cosEvents: { on: vi.fn(), emit: vi.fn() }, + registerAgent: vi.fn().mockResolvedValue(undefined), + updateAgent: vi.fn().mockResolvedValue(undefined), + completeAgent: vi.fn().mockResolvedValue(undefined), + appendAgentOutput: vi.fn().mockResolvedValue(undefined), + getConfig: vi.fn().mockResolvedValue({}), + updateTask: vi.fn().mockResolvedValue(undefined), + addTask: vi.fn().mockResolvedValue(undefined), + emitLog: vi.fn(), + getTaskById: vi.fn().mockResolvedValue(null), + getAgent: vi.fn().mockResolvedValue(null) +})); + +vi.mock('./appActivity.js', () => ({ + startAppCooldown: vi.fn(), + markAppReviewCompleted: vi.fn() +})); + +vi.mock('./cosRunnerClient.js', () => ({ + isRunnerAvailable: vi.fn(() => false), + spawnAgentViaRunner: vi.fn(), + terminateAgentViaRunner: vi.fn(), + killAgentViaRunner: vi.fn(), + getAgentStatsFromRunner: vi.fn(), + initCosRunnerConnection: vi.fn(), + onCosRunnerEvent: vi.fn(), + getActiveAgentsFromRunner: vi.fn(() => []), + getRunnerHealth: vi.fn() +})); + +vi.mock('./providers.js', () => ({ + getActiveProvider: vi.fn(), + getProviderById: vi.fn(), + getAllProviders: vi.fn(() => []) +})); + +vi.mock('./usage.js', () => ({ + recordSession: vi.fn(), + recordMessages: vi.fn() +})); + +vi.mock('./providerStatus.js', () => ({ + isProviderAvailable: vi.fn(() => true), + markProviderUsageLimit: vi.fn(), + markProviderRateLimited: vi.fn(), + getFallbackProvider: vi.fn(), + getProviderStatus: vi.fn(), + initProviderStatus: vi.fn() +})); + +vi.mock('./promptService.js', () => ({ + buildPrompt: vi.fn() +})); + +vi.mock('./agents.js', () => ({ + registerSpawnedAgent: vi.fn(), + unregisterSpawnedAgent: vi.fn() +})); + +vi.mock('./memoryRetriever.js', () => ({ + getMemorySection: vi.fn() +})); + +vi.mock('./memoryExtractor.js', () => ({ + extractAndStoreMemories: vi.fn() +})); + +vi.mock('./digital-twin.js', () => ({ + getDigitalTwinForPrompt: vi.fn() +})); + +vi.mock('./taskLearning.js', () => ({ + suggestModelTier: vi.fn() +})); + +vi.mock('../lib/fileUtils.js', () => ({ + ensureDir: vi.fn().mockResolvedValue(undefined), + readJSONFile: vi.fn().mockResolvedValue({}), + PATHS: { + root: '/mock/root', + cosAgents: '/mock/root/data/cos/agents', + runs: '/mock/root/data/runs', + worktrees: '/mock/root/data/cos/worktrees', + data: '/mock/root/data', + cos: '/mock/root/data/cos' + } +})); + +vi.mock('./apps.js', () => ({ + getAppById: vi.fn() +})); + +vi.mock('./toolStateMachine.js', () => ({ + createToolExecution: vi.fn(), + startExecution: vi.fn(), + updateExecution: vi.fn(), + completeExecution: vi.fn(), + errorExecution: vi.fn(), + getExecution: vi.fn(), + getStats: vi.fn() +})); + +vi.mock('./thinkingLevels.js', () => ({ + resolveThinkingLevel: vi.fn(), + getModelForLevel: vi.fn(), + isLocalPreferred: vi.fn(() => false) +})); + +vi.mock('./executionLanes.js', () => ({ + determineLane: vi.fn(), + acquire: vi.fn(), + release: vi.fn(), + hasCapacity: vi.fn(() => true), + waitForLane: vi.fn() +})); + +vi.mock('./taskConflict.js', () => ({ + detectConflicts: vi.fn(() => []) +})); + +vi.mock('./worktreeManager.js', () => ({ + createWorktree: vi.fn(), + removeWorktree: vi.fn().mockResolvedValue(undefined), + cleanupOrphanedWorktrees: vi.fn() +})); + +vi.mock('./jira.js', () => ({ + default: {} +})); + +vi.mock('./git.js', () => ({ + push: vi.fn(), + getRepoBranches: vi.fn(), + createPR: vi.fn() +})); + +vi.mock('./runner.js', () => ({ + executeApiRun: vi.fn(), + executeCliRun: vi.fn(), + createRun: vi.fn() +})); + +// --- Import the function under test and the mocked dependencies --- + +import { cleanupAgentWorktree } from './subAgentSpawner.js'; +import { getAgent } from './cos.js'; +import { removeWorktree } from './worktreeManager.js'; +import * as git from './git.js'; + +// Helper: build a mock agent state for worktree agents +function mockWorktreeAgent(overrides = {}) { + return { + metadata: { + isWorktree: true, + isPersistentWorktree: false, + sourceWorkspace: '/mock/workspace', + worktreeBranch: 'cos/task-abc123', + workspacePath: '/mock/root/data/cos/worktrees/agent-1', + ...overrides + } + }; +} + +describe('cleanupAgentWorktree - openPR path', () => { + beforeEach(() => { + vi.clearAllMocks(); + // Default: agent is a worktree agent with valid metadata + getAgent.mockResolvedValue(mockWorktreeAgent()); + git.getRepoBranches.mockResolvedValue({ baseBranch: 'main', devBranch: null }); + }); + + it('should run PR flow when openPR is true and success is true', async () => { + git.push.mockResolvedValue(undefined); + git.createPR.mockResolvedValue({ success: true, url: 'https://github.com/test/repo/pull/1' }); + + await cleanupAgentWorktree('agent-1', true, { openPR: true, description: 'Test task' }); + + expect(git.push).toHaveBeenCalledWith('/mock/root/data/cos/worktrees/agent-1', 'cos/task-abc123'); + expect(git.createPR).toHaveBeenCalledWith('/mock/root/data/cos/worktrees/agent-1', { + title: 'Test task', + body: expect.stringContaining('Test task'), + base: 'main', + head: 'cos/task-abc123' + }); + expect(removeWorktree).toHaveBeenCalledWith('agent-1', '/mock/workspace', 'cos/task-abc123', { merge: false }); + }); + + it('should call removeWorktree with merge: false after successful push and PR', async () => { + git.push.mockResolvedValue(undefined); + git.createPR.mockResolvedValue({ success: true, url: 'https://github.com/test/repo/pull/2' }); + + await cleanupAgentWorktree('agent-1', true, { openPR: true }); + + expect(removeWorktree).toHaveBeenCalledTimes(1); + expect(removeWorktree).toHaveBeenCalledWith( + 'agent-1', + '/mock/workspace', + 'cos/task-abc123', + { merge: false } + ); + }); + + it('should preserve worktree when push fails (no removeWorktree call)', async () => { + git.push.mockRejectedValue(new Error('push rejected')); + + await cleanupAgentWorktree('agent-1', true, { openPR: true, description: 'Test task' }); + + expect(git.push).toHaveBeenCalled(); + expect(git.createPR).not.toHaveBeenCalled(); + expect(removeWorktree).not.toHaveBeenCalled(); + }); + + it('should preserve worktree when createPR returns { success: false }', async () => { + git.push.mockResolvedValue(undefined); + git.createPR.mockResolvedValue({ success: false, error: 'PR already exists' }); + + await cleanupAgentWorktree('agent-1', true, { openPR: true, description: 'Test task' }); + + expect(git.createPR).toHaveBeenCalled(); + expect(removeWorktree).not.toHaveBeenCalled(); + }); + + it('should use auto-merge path when openPR is false (success)', async () => { + await cleanupAgentWorktree('agent-1', true, { openPR: false }); + + expect(git.push).not.toHaveBeenCalled(); + expect(git.createPR).not.toHaveBeenCalled(); + expect(removeWorktree).toHaveBeenCalledWith('agent-1', '/mock/workspace', 'cos/task-abc123', { merge: true }); + }); + + it('should use auto-merge path when openPR is not provided (defaults to false)', async () => { + await cleanupAgentWorktree('agent-1', true); + + expect(git.push).not.toHaveBeenCalled(); + expect(git.createPR).not.toHaveBeenCalled(); + expect(removeWorktree).toHaveBeenCalledWith('agent-1', '/mock/workspace', 'cos/task-abc123', { merge: true }); + }); + + it('should skip PR flow when openPR is true but success is false', async () => { + await cleanupAgentWorktree('agent-1', false, { openPR: true }); + + expect(git.push).not.toHaveBeenCalled(); + expect(git.createPR).not.toHaveBeenCalled(); + // Falls through to auto-merge path with merge: false (failure cleanup) + expect(removeWorktree).toHaveBeenCalledWith('agent-1', '/mock/workspace', 'cos/task-abc123', { merge: false }); + }); + + it('should use devBranch as PR base when available', async () => { + git.push.mockResolvedValue(undefined); + git.createPR.mockResolvedValue({ success: true, url: 'https://github.com/test/repo/pull/3' }); + git.getRepoBranches.mockResolvedValue({ baseBranch: 'main', devBranch: 'develop' }); + + await cleanupAgentWorktree('agent-1', true, { openPR: true, description: 'Test' }); + + expect(git.createPR).toHaveBeenCalledWith(expect.any(String), expect.objectContaining({ + base: 'develop' + })); + }); + + it('should fall back to "main" when getRepoBranches fails', async () => { + git.push.mockResolvedValue(undefined); + git.createPR.mockResolvedValue({ success: true, url: 'https://github.com/test/repo/pull/4' }); + git.getRepoBranches.mockRejectedValue(new Error('not a git repo')); + + await cleanupAgentWorktree('agent-1', true, { openPR: true, description: 'Test' }); + + expect(git.createPR).toHaveBeenCalledWith(expect.any(String), expect.objectContaining({ + base: 'main' + })); + }); + + it('should still remove worktree when createPR throws (null result)', async () => { + git.push.mockResolvedValue(undefined); + git.createPR.mockRejectedValue(new Error('network error')); + + await cleanupAgentWorktree('agent-1', true, { openPR: true, description: 'Test' }); + + // createPR catches the error and returns null; null?.success is undefined (not false), + // so it proceeds to removeWorktree + expect(removeWorktree).toHaveBeenCalledWith('agent-1', '/mock/workspace', 'cos/task-abc123', { merge: false }); + }); + + it('should truncate long descriptions to 100 chars for PR title', async () => { + const longDesc = 'A'.repeat(200); + git.push.mockResolvedValue(undefined); + git.createPR.mockResolvedValue({ success: true, url: 'https://github.com/test/repo/pull/5' }); + + await cleanupAgentWorktree('agent-1', true, { openPR: true, description: longDesc }); + + expect(git.createPR).toHaveBeenCalledWith(expect.any(String), expect.objectContaining({ + title: 'A'.repeat(100) + })); + }); + + it('should use default description when none provided', async () => { + git.push.mockResolvedValue(undefined); + git.createPR.mockResolvedValue({ success: true, url: 'https://github.com/test/repo/pull/6' }); + + await cleanupAgentWorktree('agent-1', true, { openPR: true }); + + expect(git.createPR).toHaveBeenCalledWith(expect.any(String), expect.objectContaining({ + title: 'CoS automated task', + body: expect.stringContaining('CoS automated task') + })); + }); + + // --- Early-exit guard tests --- + + it('should no-op when agent is not a worktree agent', async () => { + getAgent.mockResolvedValue({ metadata: { isWorktree: false } }); + + await cleanupAgentWorktree('agent-1', true, { openPR: true }); + + expect(git.push).not.toHaveBeenCalled(); + expect(removeWorktree).not.toHaveBeenCalled(); + }); + + it('should no-op when agent state is null', async () => { + getAgent.mockResolvedValue(null); + + await cleanupAgentWorktree('agent-1', true, { openPR: true }); + + expect(git.push).not.toHaveBeenCalled(); + expect(removeWorktree).not.toHaveBeenCalled(); + }); + + it('should no-op for persistent worktree agents', async () => { + getAgent.mockResolvedValue(mockWorktreeAgent({ isPersistentWorktree: true })); + + await cleanupAgentWorktree('agent-1', true, { openPR: true }); + + expect(git.push).not.toHaveBeenCalled(); + expect(removeWorktree).not.toHaveBeenCalled(); + }); + + it('should no-op when sourceWorkspace or worktreeBranch is missing', async () => { + getAgent.mockResolvedValue(mockWorktreeAgent({ sourceWorkspace: null })); + + await cleanupAgentWorktree('agent-1', true, { openPR: true }); + + expect(git.push).not.toHaveBeenCalled(); + expect(removeWorktree).not.toHaveBeenCalled(); + }); +}); diff --git a/server/services/subAgentSpawner.js b/server/services/subAgentSpawner.js index 28e27cef..780724b0 100644 --- a/server/services/subAgentSpawner.js +++ b/server/services/subAgentSpawner.js @@ -1957,7 +1957,7 @@ async function handleAgentCompletion(agentId, exitCode, success, duration) { * When openPR is true, pushes the branch and creates a PR instead of auto-merging. * Otherwise, merges the worktree branch back to the source branch on success. */ -async function cleanupAgentWorktree(agentId, success, { openPR = false, description = null } = {}) { +export async function cleanupAgentWorktree(agentId, success, { openPR = false, description = null } = {}) { const { getAgent: getAgentState } = await import('./cos.js'); const agentState = await getAgentState(agentId).catch(() => null); if (!agentState?.metadata?.isWorktree) return; From a89d8a17c30fe41eef402a46d017d21b63a21bab Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Sat, 21 Mar 2026 18:53:25 +0000 Subject: [PATCH 11/19] fix(cos): address round 5 Copilot review feedback - Preserve worktree when createPR throws (was incorrectly falling through) - Keep explicit false overrides in ScheduleTab metadata (filter !== undefined) - Handle string 'false' in applyAppWorktreeDefault invariant checks --- client/src/components/cos/tabs/ScheduleTab.jsx | 2 +- server/services/cleanupAgentWorktree.test.js | 7 +++---- server/services/cos.js | 4 ++-- server/services/subAgentSpawner.js | 9 ++++----- 4 files changed, 10 insertions(+), 12 deletions(-) diff --git a/client/src/components/cos/tabs/ScheduleTab.jsx b/client/src/components/cos/tabs/ScheduleTab.jsx index 9844842e..e64f2b66 100644 --- a/client/src/components/cos/tabs/ScheduleTab.jsx +++ b/client/src/components/cos/tabs/ScheduleTab.jsx @@ -35,7 +35,7 @@ function toggleMetadataField(metadata, field) { if (newMeta.useWorktree === false && newMeta.openPR) { newMeta.openPR = false; } - const active = Object.fromEntries(Object.entries(newMeta).filter(([, v]) => v)); + const active = Object.fromEntries(Object.entries(newMeta).filter(([, v]) => v !== undefined && v !== null)); return Object.keys(active).length ? active : null; } diff --git a/server/services/cleanupAgentWorktree.test.js b/server/services/cleanupAgentWorktree.test.js index 7446ae8b..9e877ec7 100644 --- a/server/services/cleanupAgentWorktree.test.js +++ b/server/services/cleanupAgentWorktree.test.js @@ -296,15 +296,14 @@ describe('cleanupAgentWorktree - openPR path', () => { })); }); - it('should still remove worktree when createPR throws (null result)', async () => { + it('should preserve worktree when createPR throws', async () => { git.push.mockResolvedValue(undefined); git.createPR.mockRejectedValue(new Error('network error')); await cleanupAgentWorktree('agent-1', true, { openPR: true, description: 'Test' }); - // createPR catches the error and returns null; null?.success is undefined (not false), - // so it proceeds to removeWorktree - expect(removeWorktree).toHaveBeenCalledWith('agent-1', '/mock/workspace', 'cos/task-abc123', { merge: false }); + // PR creation failed — worktree preserved for manual intervention + expect(removeWorktree).not.toHaveBeenCalled(); }); it('should truncate long descriptions to 100 chars for PR title', async () => { diff --git a/server/services/cos.js b/server/services/cos.js index d8788967..9730914e 100644 --- a/server/services/cos.js +++ b/server/services/cos.js @@ -1879,7 +1879,7 @@ export function applyAppWorktreeDefault(metadata, app) { metadata.useWorktree = false; } } - if (metadata.openPR === undefined && metadata.useWorktree !== false) { + if (metadata.openPR === undefined && metadata.useWorktree !== false && metadata.useWorktree !== 'false') { if (app.defaultOpenPR === true) { metadata.openPR = true; metadata.useWorktree = true; // openPR implies useWorktree @@ -1888,7 +1888,7 @@ export function applyAppWorktreeDefault(metadata, app) { } } // Ensure openPR=false when useWorktree=false (invariant: openPR implies useWorktree) - if (metadata.useWorktree === false) { + if (metadata.useWorktree === false || metadata.useWorktree === 'false') { metadata.openPR = false; } } diff --git a/server/services/subAgentSpawner.js b/server/services/subAgentSpawner.js index 780724b0..e290c887 100644 --- a/server/services/subAgentSpawner.js +++ b/server/services/subAgentSpawner.js @@ -1997,15 +1997,14 @@ export async function cleanupAgentWorktree(agentId, success, { openPR = false, d return null; }); - if (prResult?.success === false) { - emitLog('error', `🌳 PR creation failed for ${worktreeBranch}: ${prResult.error}`, { agentId, branchName: worktreeBranch }); + if (!prResult?.success) { + const reason = prResult?.error || 'unknown error (createPR returned null or threw)'; + emitLog('error', `🌳 PR creation failed for ${worktreeBranch}: ${reason}`, { agentId, branchName: worktreeBranch }); // Preserve worktree/branch for manual PR creation return; } - if (prResult?.success) { - emitLog('success', `🌳 Created PR: ${prResult.url}`, { agentId, branchName: worktreeBranch }); - } + emitLog('success', `🌳 Created PR: ${prResult.url}`, { agentId, branchName: worktreeBranch }); // Remove worktree without merging (PR handles merge) await removeWorktree(agentId, sourceWorkspace, worktreeBranch, { merge: false }).catch(err => { From 42f36a5f07bfcb5bc3ee0fa815de0a75e170ccbc Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Sat, 21 Mar 2026 19:03:19 +0000 Subject: [PATCH 12/19] fix(cos): address round 6 Copilot review feedback - Restructure applyAppWorktreeDefault to apply defaultOpenPR before defaultUseWorktree so defaultOpenPR:true works when defaultUseWorktree:false - Use isTruthyMeta for simplify and reviewLoop prompt checks to handle string 'false' from TASKS.md metadata parsing --- server/services/cos.js | 29 +++++++++++++++++++---------- server/services/subAgentSpawner.js | 4 ++-- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/server/services/cos.js b/server/services/cos.js index 9730914e..e1b9855e 100644 --- a/server/services/cos.js +++ b/server/services/cos.js @@ -1866,8 +1866,24 @@ Use model: claude-opus-4-5-20251101 for thorough security analysis` * @param {Object} state - Current CoS state * @returns {Object} Generated task */ -// Apply app-level worktree/PR defaults only when not already set by task-type metadata +// Apply app-level worktree/PR defaults only when not already set by task-type metadata. +// openPR is applied first since it implies useWorktree — this prevents defaultUseWorktree: false +// from blocking defaultOpenPR: true when both are app-level defaults. export function applyAppWorktreeDefault(metadata, app) { + const useWorktreeWasSetByTaskType = metadata.useWorktree !== undefined; + const taskTypeDisabledWorktree = metadata.useWorktree === false || metadata.useWorktree === 'false'; + + // Apply defaultOpenPR first (since openPR implies useWorktree) + if (metadata.openPR === undefined) { + if (app.defaultOpenPR === true && !taskTypeDisabledWorktree) { + metadata.openPR = true; + metadata.useWorktree = true; // openPR implies useWorktree + } else if (app.defaultOpenPR === false || taskTypeDisabledWorktree) { + metadata.openPR = false; + } + } + + // Apply defaultUseWorktree (only if not already set by task-type or openPR above) if (metadata.useWorktree === undefined) { // openPR implies useWorktree — don't let app default override explicit openPR: true const explicitOpenPR = metadata.openPR === true || metadata.openPR === 'true'; @@ -1879,15 +1895,8 @@ export function applyAppWorktreeDefault(metadata, app) { metadata.useWorktree = false; } } - if (metadata.openPR === undefined && metadata.useWorktree !== false && metadata.useWorktree !== 'false') { - if (app.defaultOpenPR === true) { - metadata.openPR = true; - metadata.useWorktree = true; // openPR implies useWorktree - } else if (app.defaultOpenPR === false) { - metadata.openPR = false; - } - } - // Ensure openPR=false when useWorktree=false (invariant: openPR implies useWorktree) + + // Final invariant: openPR=false when useWorktree=false if (metadata.useWorktree === false || metadata.useWorktree === 'false') { metadata.openPR = false; } diff --git a/server/services/subAgentSpawner.js b/server/services/subAgentSpawner.js index e290c887..3c1acd5e 100644 --- a/server/services/subAgentSpawner.js +++ b/server/services/subAgentSpawner.js @@ -2691,13 +2691,13 @@ ${worktreeInfo.baseBranch ? `- **Based on**: \`${worktreeInfo.baseBranch}\` (lat ` : ''; // Build simplify section if enabled - const simplifySection = task.metadata?.simplify ? ` + const simplifySection = isTruthyMeta(task.metadata?.simplify) ? ` ## Simplify Step After completing your work and before committing, run \`/simplify\` to review the changed code for reuse, quality, and efficiency. Fix any issues found before committing. ` : ''; // Build review loop section if enabled (only when the agent creates the PR itself, not when openPR auto-creates it post-exit) - const reviewLoopSection = (task.metadata?.reviewLoop && !willOpenPR) ? ` + const reviewLoopSection = (isTruthyMeta(task.metadata?.reviewLoop) && !willOpenPR) ? ` ## Review Loop After opening the PR, run \`/do:rpr\` to resolve PR review feedback and complete the merge validation. Continue running the review loop until all checks pass and the PR is approved. ` : ''; From eeee8963af4b950c97f2851a6ca66a63a1bc4227 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Sat, 21 Mar 2026 19:12:19 +0000 Subject: [PATCH 13/19] fix(cos): normalize PR title and remove dead variable - Strip newlines/whitespace from task description before using as PR title - Remove unused useWorktreeWasSetByTaskType variable --- server/services/cos.js | 1 - server/services/subAgentSpawner.js | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/server/services/cos.js b/server/services/cos.js index e1b9855e..b73e7465 100644 --- a/server/services/cos.js +++ b/server/services/cos.js @@ -1870,7 +1870,6 @@ Use model: claude-opus-4-5-20251101 for thorough security analysis` // openPR is applied first since it implies useWorktree — this prevents defaultUseWorktree: false // from blocking defaultOpenPR: true when both are app-level defaults. export function applyAppWorktreeDefault(metadata, app) { - const useWorktreeWasSetByTaskType = metadata.useWorktree !== undefined; const taskTypeDisabledWorktree = metadata.useWorktree === false || metadata.useWorktree === 'false'; // Apply defaultOpenPR first (since openPR implies useWorktree) diff --git a/server/services/subAgentSpawner.js b/server/services/subAgentSpawner.js index 3c1acd5e..b84338d9 100644 --- a/server/services/subAgentSpawner.js +++ b/server/services/subAgentSpawner.js @@ -1986,9 +1986,10 @@ export async function cleanupAgentWorktree(agentId, success, { openPR = false, d if (pushResult) { const targetBranch = branchInfo.devBranch || branchInfo.baseBranch || 'main'; const taskDesc = description || 'CoS automated task'; + const prTitle = taskDesc.replace(/[\r\n]+/g, ' ').trim().substring(0, 100); const prResult = await git.createPR(worktreePath, { - title: taskDesc.substring(0, 100), + title: prTitle, body: `Automated PR created by PortOS Chief of Staff.\n\n**Task:** ${taskDesc}`, base: targetBranch, head: worktreeBranch From a19ece74feb20060238bfb2f3b8cf9f0000431fb Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Sat, 21 Mar 2026 19:25:14 +0000 Subject: [PATCH 14/19] fix(cos): address round 8 Copilot review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Normalize openPR→useWorktree invariant bidirectionally (openPR:true forces useWorktree:true instead of being silently cleared) - Fix toggleAppMetadataOverride rule ordering to respect user's toggle intent - Propagate openPR flag in direct-spawn completion path - Use isTruthyMeta for simplify/reviewLoop prompt checks --- client/src/components/cos/constants.js | 12 ++++++------ server/services/cos.js | 9 +++++++-- server/services/subAgentSpawner.js | 5 ++++- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/client/src/components/cos/constants.js b/client/src/components/cos/constants.js index 8927f80c..69c1aa40 100644 --- a/client/src/components/cos/constants.js +++ b/client/src/components/cos/constants.js @@ -73,13 +73,13 @@ export function toggleAppMetadataOverride(overrideMetadata, globalMetadata, fiel const resolve = (f) => newMeta[f] ?? globalMetadata?.[f] ?? false; - // openPR requires useWorktree - if (resolve('openPR') && !resolve('useWorktree')) { - newMeta.useWorktree = true; - } - // useWorktree off means openPR must be off - if (!resolve('useWorktree') && resolve('openPR')) { + // Apply useWorktree-off rule first (based on the toggled field's new value) + if (field === 'useWorktree' && !newMeta.useWorktree && newMeta.useWorktree !== undefined) { + // User explicitly toggled worktree off — force openPR off newMeta.openPR = false; + } else if (field === 'openPR' && resolve('openPR') && !resolve('useWorktree')) { + // User toggled openPR on — force useWorktree on + newMeta.useWorktree = true; } // Clean entries that match the global value (revert to inherit) diff --git a/server/services/cos.js b/server/services/cos.js index b73e7465..ea2edded 100644 --- a/server/services/cos.js +++ b/server/services/cos.js @@ -1895,8 +1895,13 @@ export function applyAppWorktreeDefault(metadata, app) { } } - // Final invariant: openPR=false when useWorktree=false - if (metadata.useWorktree === false || metadata.useWorktree === 'false') { + // Final invariant: openPR implies useWorktree (normalize in both directions) + const finalOpenPR = metadata.openPR === true || metadata.openPR === 'true'; + const finalWorktreeOff = metadata.useWorktree === false || metadata.useWorktree === 'false'; + if (finalOpenPR && finalWorktreeOff) { + // openPR wins — force useWorktree on + metadata.useWorktree = true; + } else if (finalWorktreeOff) { metadata.openPR = false; } } diff --git a/server/services/subAgentSpawner.js b/server/services/subAgentSpawner.js index b84338d9..ba4d0829 100644 --- a/server/services/subAgentSpawner.js +++ b/server/services/subAgentSpawner.js @@ -2247,7 +2247,10 @@ async function spawnDirectly(agentId, task, prompt, workspacePath, model, provid await processAgentCompletion(agentId, task, success, outputBuffer); // Clean up worktree if agent was using one - await cleanupAgentWorktree(agentId, success); + await cleanupAgentWorktree(agentId, success, { + openPR: isTruthyMeta(task.metadata?.openPR), + description: task.description + }); unregisterSpawnedAgent(agentData?.pid || claudeProcess.pid); activeAgents.delete(agentId); From d2a987e12663b0f0d9d0ac9c1f021abaf144fdda Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Sat, 21 Mar 2026 19:34:42 +0000 Subject: [PATCH 15/19] fix(cos): persist explicit false for boolean flags and fix toggle semantics - addTask persists both true and false for useWorktree/openPR/simplify/reviewLoop so users can explicitly override app defaults (isTruthyMeta/isFalsyMeta handle string round-trip from TASKS.md) - ScheduleTab toggleMetadataField keeps only true entries (false = inherit default) --- client/src/components/cos/tabs/ScheduleTab.jsx | 4 +++- server/services/cos.js | 10 +++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/client/src/components/cos/tabs/ScheduleTab.jsx b/client/src/components/cos/tabs/ScheduleTab.jsx index e64f2b66..5cee6027 100644 --- a/client/src/components/cos/tabs/ScheduleTab.jsx +++ b/client/src/components/cos/tabs/ScheduleTab.jsx @@ -35,7 +35,9 @@ function toggleMetadataField(metadata, field) { if (newMeta.useWorktree === false && newMeta.openPR) { newMeta.openPR = false; } - const active = Object.fromEntries(Object.entries(newMeta).filter(([, v]) => v !== undefined && v !== null)); + // Keep only entries that differ from the default (false/undefined); remove entries + // that are false (returning to default/inherited state) to keep metadata clean + const active = Object.fromEntries(Object.entries(newMeta).filter(([, v]) => v === true)); return Object.keys(active).length ? active : null; } diff --git a/server/services/cos.js b/server/services/cos.js index ea2edded..e11d8b1b 100644 --- a/server/services/cos.js +++ b/server/services/cos.js @@ -3221,13 +3221,17 @@ export async function addTask(taskData, taskType = 'user', { raw = false } = {}) if (taskData.provider) metadata.provider = taskData.provider; if (taskData.app) metadata.app = taskData.app; if (taskData.createJiraTicket) metadata.createJiraTicket = true; - // Boolean flags: only persist true values. In TASKS.md, false round-trips as the string - // 'false' which is truthy in JS. Omitting (undefined) means "use app defaults", and - // isTruthyMeta handles 'true' strings correctly for the true case. + // Boolean flags: persist both true and false so users can explicitly override defaults. + // The string round-trip ('false' from TASKS.md) is handled by isTruthyMeta/isFalsyMeta. + // undefined means "use app defaults". if (taskData.useWorktree === true) metadata.useWorktree = true; + else if (taskData.useWorktree === false) metadata.useWorktree = false; if (taskData.openPR === true) metadata.openPR = true; + else if (taskData.openPR === false) metadata.openPR = false; if (taskData.simplify === true) metadata.simplify = true; + else if (taskData.simplify === false) metadata.simplify = false; if (taskData.reviewLoop === true) metadata.reviewLoop = true; + else if (taskData.reviewLoop === false) metadata.reviewLoop = false; if (taskData.jiraTicketId) metadata.jiraTicketId = taskData.jiraTicketId; if (taskData.jiraTicketUrl) metadata.jiraTicketUrl = taskData.jiraTicketUrl; if (taskData.screenshots?.length > 0) metadata.screenshots = taskData.screenshots; From 5ebe84b71742a7deac9f8a98fe1bf27f5a11baad Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Sat, 21 Mar 2026 19:43:47 +0000 Subject: [PATCH 16/19] docs: clarify toggleMetadataField is global config, not per-task override --- client/src/components/cos/tabs/ScheduleTab.jsx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/client/src/components/cos/tabs/ScheduleTab.jsx b/client/src/components/cos/tabs/ScheduleTab.jsx index 5cee6027..cc301c76 100644 --- a/client/src/components/cos/tabs/ScheduleTab.jsx +++ b/client/src/components/cos/tabs/ScheduleTab.jsx @@ -35,8 +35,9 @@ function toggleMetadataField(metadata, field) { if (newMeta.useWorktree === false && newMeta.openPR) { newMeta.openPR = false; } - // Keep only entries that differ from the default (false/undefined); remove entries - // that are false (returning to default/inherited state) to keep metadata clean + // This is the GLOBAL taskMetadata config (the app-level default itself), not a per-task-type + // override. Only persist true values; absent keys default to disabled. Per-task-type overrides + // are handled separately by toggleAppMetadataOverride which supports tri-state (inherit/true/false). const active = Object.fromEntries(Object.entries(newMeta).filter(([, v]) => v === true)); return Object.keys(active).length ? active : null; } From fd788904ca0ed7962e95cbdc128dfe04b34f5d33 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Sat, 21 Mar 2026 19:57:30 +0000 Subject: [PATCH 17/19] fix(cos): preserve metadata key casing in TASKS.md round-trip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The task parser lowercased all metadata keys and the serializer capitalized the first character, so openPR→OpenPR→openpr on round-trip. This broke camelCase keys like openPR, useWorktree, reviewLoop after reload. Now both serializer and parser preserve the original key casing. --- server/lib/taskParser.js | 5 ++--- server/lib/taskParser.test.js | 10 +++++----- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/server/lib/taskParser.js b/server/lib/taskParser.js index 8bbd5377..f4cd1aa2 100644 --- a/server/lib/taskParser.js +++ b/server/lib/taskParser.js @@ -144,7 +144,7 @@ function parseMetadataLine(line) { if (!match) return null; return { - key: match[1].toLowerCase(), + key: match[1], value: unescapeNewlines(match[2].trim()) }; } @@ -255,9 +255,8 @@ export function generateTasksMarkdown(tasks, includeApprovalFlags = false) { // Add metadata (escape newlines in values for single-line storage) for (const [key, value] of Object.entries(task.metadata)) { - const capitalizedKey = key.charAt(0).toUpperCase() + key.slice(1); const escapedValue = escapeNewlines(String(value)); - lines.push(` - ${capitalizedKey}: ${escapedValue}`); + lines.push(` - ${key}: ${escapedValue}`); } } diff --git a/server/lib/taskParser.test.js b/server/lib/taskParser.test.js index d1093d37..9a4bc4c7 100644 --- a/server/lib/taskParser.test.js +++ b/server/lib/taskParser.test.js @@ -98,9 +98,9 @@ describe('Task Parser', () => { ## Pending - [ ] #task-001 | HIGH | Fix the bug - - Context: User reported issue - - App: my-app - - Model: claude-sonnet`; + - context: User reported issue + - app: my-app + - model: claude-sonnet`; const tasks = parseTasksMarkdown(markdown); @@ -261,8 +261,8 @@ describe('Task Parser', () => { const markdown = generateTasksMarkdown(tasks); - expect(markdown).toContain('- Context: Some context'); - expect(markdown).toContain('- App: my-app'); + expect(markdown).toContain('- context: Some context'); + expect(markdown).toContain('- app: my-app'); }); it('should escape newlines in metadata values for round-trip preservation', () => { From 004a736795429dac0f0cb69e54a461b2dff55fbe Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Sat, 21 Mar 2026 20:06:43 +0000 Subject: [PATCH 18/19] fix(cos): normalize TASKS.md metadata keys for backward compatibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lowercase first character of parsed keys to handle legacy Title-Case keys (Context→context, App→app) while preserving camelCase keys (openPR, useWorktree). Adds test for camelCase key preservation. --- server/lib/taskParser.js | 6 +++++- server/lib/taskParser.test.js | 24 +++++++++++++++++++++--- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/server/lib/taskParser.js b/server/lib/taskParser.js index f4cd1aa2..069f5f90 100644 --- a/server/lib/taskParser.js +++ b/server/lib/taskParser.js @@ -143,8 +143,12 @@ function parseMetadataLine(line) { const match = line.match(/^\s+-\s*(\w+):\s*(.+)$/); if (!match) return null; + // Normalize key: lowercase first character to handle legacy Title-Case keys (Context→context, + // App→app) while preserving camelCase keys (openPR stays openPR, useWorktree stays useWorktree) + const rawKey = match[1]; + const key = rawKey.charAt(0).toLowerCase() + rawKey.slice(1); return { - key: match[1], + key, value: unescapeNewlines(match[2].trim()) }; } diff --git a/server/lib/taskParser.test.js b/server/lib/taskParser.test.js index 9a4bc4c7..8995e660 100644 --- a/server/lib/taskParser.test.js +++ b/server/lib/taskParser.test.js @@ -98,18 +98,36 @@ describe('Task Parser', () => { ## Pending - [ ] #task-001 | HIGH | Fix the bug - - context: User reported issue - - app: my-app - - model: claude-sonnet`; + - Context: User reported issue + - App: my-app + - Model: claude-sonnet`; const tasks = parseTasksMarkdown(markdown); expect(tasks).toHaveLength(1); + // Legacy Title-Case keys are normalized to camelCase (Context→context) expect(tasks[0].metadata.context).toBe('User reported issue'); expect(tasks[0].metadata.app).toBe('my-app'); expect(tasks[0].metadata.model).toBe('claude-sonnet'); }); + it('should preserve camelCase metadata keys', () => { + const markdown = `# Tasks + +## Pending +- [ ] #task-001 | HIGH | Fix the bug + - openPR: true + - useWorktree: true + - reviewLoop: false`; + + const tasks = parseTasksMarkdown(markdown); + + expect(tasks).toHaveLength(1); + expect(tasks[0].metadata.openPR).toBe('true'); + expect(tasks[0].metadata.useWorktree).toBe('true'); + expect(tasks[0].metadata.reviewLoop).toBe('false'); + }); + it('should handle empty content', () => { const tasks = parseTasksMarkdown(''); expect(tasks).toHaveLength(0); From 3bcb3e443ee97759d1b2ee7da85d39d5576ea22f Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Sat, 21 Mar 2026 20:17:00 +0000 Subject: [PATCH 19/19] fix(cos): use baseBranch for PR target, improve prompt and docs - PR targets baseBranch (not devBranch) since worktrees are created from it - Prompt says "source branch" instead of "default branch" for auto-merge - Updated taskParser docstring for metadata key format --- server/lib/taskParser.js | 5 ++++- server/services/cleanupAgentWorktree.test.js | 4 ++-- server/services/subAgentSpawner.js | 5 +++-- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/server/lib/taskParser.js b/server/lib/taskParser.js index 069f5f90..b786daf0 100644 --- a/server/lib/taskParser.js +++ b/server/lib/taskParser.js @@ -137,7 +137,10 @@ function escapeNewlines(value) { /** * Parse metadata line (indented under task) - * Format: - Key: Value + * Format: - key: Value + * Keys are written in camelCase (e.g., openPR, useWorktree, reviewLoop). + * Legacy Title-Case keys (e.g., Context, App) are accepted and normalized + * to camelCase by lowercasing the first character. */ function parseMetadataLine(line) { const match = line.match(/^\s+-\s*(\w+):\s*(.+)$/); diff --git a/server/services/cleanupAgentWorktree.test.js b/server/services/cleanupAgentWorktree.test.js index 9e877ec7..89a061bc 100644 --- a/server/services/cleanupAgentWorktree.test.js +++ b/server/services/cleanupAgentWorktree.test.js @@ -272,7 +272,7 @@ describe('cleanupAgentWorktree - openPR path', () => { expect(removeWorktree).toHaveBeenCalledWith('agent-1', '/mock/workspace', 'cos/task-abc123', { merge: false }); }); - it('should use devBranch as PR base when available', async () => { + it('should use baseBranch as PR base (not devBranch, since worktrees are created from baseBranch)', async () => { git.push.mockResolvedValue(undefined); git.createPR.mockResolvedValue({ success: true, url: 'https://github.com/test/repo/pull/3' }); git.getRepoBranches.mockResolvedValue({ baseBranch: 'main', devBranch: 'develop' }); @@ -280,7 +280,7 @@ describe('cleanupAgentWorktree - openPR path', () => { await cleanupAgentWorktree('agent-1', true, { openPR: true, description: 'Test' }); expect(git.createPR).toHaveBeenCalledWith(expect.any(String), expect.objectContaining({ - base: 'develop' + base: 'main' })); }); diff --git a/server/services/subAgentSpawner.js b/server/services/subAgentSpawner.js index ba4d0829..f303a99d 100644 --- a/server/services/subAgentSpawner.js +++ b/server/services/subAgentSpawner.js @@ -1984,7 +1984,8 @@ export async function cleanupAgentWorktree(agentId, success, { openPR = false, d // Only create PR if push succeeded; preserve worktree/branch for manual intervention if push fails if (pushResult) { - const targetBranch = branchInfo.devBranch || branchInfo.baseBranch || 'main'; + // Use baseBranch (not devBranch) since worktrees are created from the default branch + const targetBranch = branchInfo.baseBranch || 'main'; const taskDesc = description || 'CoS automated task'; const prTitle = taskDesc.replace(/[\r\n]+/g, ' ').trim().substring(0, 100); @@ -2803,7 +2804,7 @@ ${skillSection ? `## Task-Type Skill Guidelines\n\n${skillSection}\n` : ''}${pla - Do not make unrelated changes - If blocked, explain clearly why - Never update the PortOS changelog (\`.changelog/\`) for work on managed apps — the PortOS changelog tracks PortOS core changes only -${task.metadata?.app && worktreeInfo && willOpenPR ? `- Commit code after each feature or bug fix using the git tools or /cam skill. A pull request will be automatically created when your task completes — do NOT open a PR manually.` : task.metadata?.app && worktreeInfo ? `- Commit code after each feature or bug fix using the git tools or /cam skill. Your worktree branch will be automatically merged back to the default branch when your task completes — do NOT open a PR.` : `- Commit code after each feature or bug fix using the git tools or /cam skill`} +${task.metadata?.app && worktreeInfo && willOpenPR ? `- Commit code after each feature or bug fix using the git tools or /cam skill. A pull request will be automatically created when your task completes — do NOT open a PR manually.` : task.metadata?.app && worktreeInfo ? `- Commit code after each feature or bug fix using the git tools or /cam skill. Your worktree branch will be automatically merged back to the source branch when your task completes — do NOT open a PR.` : `- Commit code after each feature or bug fix using the git tools or /cam skill`} ## Git Hygiene (CRITICAL) - **Before starting work**, run \`git status\` to verify a clean working tree. If there are uncommitted changes from a previous agent or manual work, **stash or discard them** before proceeding — do NOT commit someone else's changes.