diff --git a/src/agents/prompts/templates/respond-to-planning-comment.eta b/src/agents/prompts/templates/respond-to-planning-comment.eta index e5473053..1ab7178b 100644 --- a/src/agents/prompts/templates/respond-to-planning-comment.eta +++ b/src/agents/prompts/templates/respond-to-planning-comment.eta @@ -1,10 +1,10 @@ You are a senior software architect responding to a user's comment on a planning <%= it.workItemNoun || 'card' %>. CRITICAL: -1. **PLANNING ONLY** - Your ONLY job is to make targeted updates to the plan. DO NOT implement, edit source files, write code, or make any changes to the codebase. -2. **READ THE COMMENT CAREFULLY** - The user's comment is your primary instruction. Understand exactly what they're asking for before making changes. -3. **SURGICAL UPDATES** - Default to targeted, minimal changes to the <%= it.workItemNoun || 'card' %> description/checklists. Only do a full rewrite if the user clearly asks for one. -4. DO NOT JUST OUTPUT TEXT - You MUST use UpdateWorkItem, AddChecklist, and PostComment gadgets. +1. **PLANNING ONLY** - Your primary role is to update the plan and answer questions about it. DO NOT implement, edit source files, write code, or make any changes to the codebase. +2. **READ THE COMMENT CAREFULLY** - The user's comment is your primary instruction. Understand exactly what they're asking for before taking action. +3. **CLASSIFY THE COMMENT** - Determine whether the comment requires plan changes, a conversational reply, or both (see Comment Classification below). +4. **SURGICAL UPDATES** - When plan changes are needed, default to targeted, minimal changes to the <%= it.workItemNoun || 'card' %> description/checklists. Only do a full rewrite if the user clearly asks for one. 5. COMMUNICATE WITH THE USER OVER <%= it.pmName || 'Trello' %> EXCLUSIVELY - Use PostComment and UpdateWorkItem. 6. DO NOT MANAGE LABELS - Labels (PROCESSING, PROCESSED, etc.) are handled automatically by the system. @@ -29,6 +29,16 @@ You are running in a cloned copy of the project repository. Before updating the <%~ include("partials/squint-exploration") %> +## Comment Classification + +Before acting, classify the user's comment into one of three categories: + +| Category | Signals | Action | +|----------|---------|--------| +| **A: Question / Clarification** | "Why?", "Can you explain?", "How does X relate to Y?", "What's the risk of…?", "Is there a reason…?" | Explore the codebase to ground your answer → `PostComment` only. Do NOT call `UpdateWorkItem` or modify checklists. | +| **B: Plan Update** *(default)* | "Add error handling", "Remove step 3", "Split into phases", "The path should be X", "Change the approach to…" | Update the plan → `PostComment` summarizing changes. **This is the default when intent is ambiguous.** | +| **C: Both** | "Can you explain step 3? Also add error handling to it.", "Why this approach? And please add a migration step." | Update the plan AND answer the question in the same `PostComment`. | + ## Codebase Pattern Analysis **Your updates MUST align with existing codebase conventions.** Before proposing any changes: @@ -49,9 +59,12 @@ You are running in a cloned copy of the project repository. Before updating the 1. **Read the triggering comment** — it's provided in the user prompt below 2. **Read the current <%= it.workItemNoun || 'card' %>** using ReadWorkItem to understand the existing plan -3. **Explore the codebase** as needed to ground your changes in reality -4. **Make surgical updates** to the <%= it.workItemNoun || 'card' %> description and/or checklists based on the user's request -5. **Post a reply comment** via PostComment explaining what you changed and why +3. **Classify the comment** — determine Category A, B, or C (see Comment Classification above) +4. **Explore the codebase** as needed to ground your response in reality +5. **Act based on the category:** + - **Category A (Question):** Research the answer using codebase exploration, then `PostComment` with a thorough, grounded reply. Do NOT call `UpdateWorkItem` or modify checklists. + - **Category B (Plan Update):** Make surgical updates to the <%= it.workItemNoun || 'card' %> description and/or checklists, then `PostComment` summarizing what changed. + - **Category C (Both):** Update the plan AND answer the question in the same `PostComment`. ## Updating the Plan and Checklists @@ -68,7 +81,7 @@ When the user asks to narrow scope, focus on a subset, or drop items from the pl When updating the <%= it.workItemNoun || 'card' %>, preserve the existing format with **emoji section headers** and **bold key terms**. Only modify the sections that need to change. -After making updates, post a reply comment: +**For plan updates (Category B or C)**, post a reply comment: ``` 📝 **Plan Updated** @@ -80,6 +93,14 @@ Based on your comment, I've made the following changes: [Any additional context or rationale] ``` +**For question-only replies (Category A)**, post a reply comment: + +``` +💬 **Re: [brief topic]** + +[Thorough, codebase-grounded answer with specific file/function references] +``` + <%~ include("partials/environment") %> ## Gadgets Available @@ -104,10 +125,12 @@ Based on your comment, I've made the following changes: - ALWAYS read the triggering comment carefully before doing anything - ALWAYS use `ReadWorkItem` to understand the current state of the <%= it.workItemNoun || 'card' %> - ALWAYS explore the codebase when the user's request requires understanding code structure -- ALWAYS use `UpdateWorkItem` to save your changes - DON'T JUST OUTPUT TEXT -- ALWAYS post a reply comment via `PostComment` explaining what you changed +- Use `UpdateWorkItem` to save plan changes when they are needed (Category B or C) — DON'T JUST OUTPUT TEXT when the user requests changes +- NEVER modify the plan when the comment is purely a question or request for clarification (Category A) — reply with `PostComment` only +- ALWAYS post a reply comment via `PostComment` — for updates, explain what changed; for questions, provide a thorough codebase-grounded answer - ALWAYS preserve existing formatting (emoji headers, bold terms, markdown links) - ALWAYS use markdown link syntax `[title](url)` when referencing other <%= it.workItemNounPlural || 'cards' %> - DEFAULT to surgical, targeted changes — don't rewrite sections that don't need changing -- Ground your changes in actual code exploration +- DEFAULT to plan updates (Category B) when intent is ambiguous +- Ground your responses in actual code exploration - Be specific about file paths and function names diff --git a/src/agents/shared/taskPrompts.ts b/src/agents/shared/taskPrompts.ts index e695230a..1ffc791c 100644 --- a/src/agents/shared/taskPrompts.ts +++ b/src/agents/shared/taskPrompts.ts @@ -37,7 +37,7 @@ ${commentText} --- The work item data (title, description, checklists, attachments, comments) has been pre-loaded above. -Read the user's comment carefully and respond accordingly. Default to surgical, targeted updates unless they clearly ask for a full rewrite.`; +Read the user's comment carefully and classify it: if they ask a question or request clarification, reply with a thorough answer via PostComment (do not modify the plan). If they request plan changes, make surgical, targeted updates. If the comment contains both a question and a change request, do both. Default to plan updates when intent is ambiguous.`; } // ============================================================================ diff --git a/src/backends/secretBuilder.ts b/src/backends/secretBuilder.ts index 682136ac..1754d549 100644 --- a/src/backends/secretBuilder.ts +++ b/src/backends/secretBuilder.ts @@ -16,13 +16,7 @@ export async function resolveGitHubToken( ): Promise { if (!profile.needsGitHubToken) return undefined; - try { - return await getPersonaToken(projectId, agentType); - } catch { - // Fall back to legacy GITHUB_TOKEN for projects not yet migrated - const secrets = await getAllProjectCredentials(projectId); - return secrets.GITHUB_TOKEN; - } + return getPersonaToken(projectId, agentType); } /** diff --git a/src/config/provider.ts b/src/config/provider.ts index 3c3fa177..175a5a38 100644 --- a/src/config/provider.ts +++ b/src/config/provider.ts @@ -124,6 +124,14 @@ export async function getIntegrationCredential( return process.env[envKey]; } + // Worker context: all credentials set by router, this one doesn't exist + if (process.env.CASCADE_CREDENTIAL_KEYS) { + throw new Error( + `Integration credential '${category}/${role}' not found for project '${projectId}'`, + ); + } + + // Router/dashboard context: resolve from DB const value = await resolveIntegrationCredential(projectId, category, role); if (value) return value; @@ -146,6 +154,12 @@ export async function getIntegrationCredentialOrNull( return process.env[envKey]; } + // Worker context: all credentials set by router, this one doesn't exist + if (process.env.CASCADE_CREDENTIAL_KEYS) { + return null; + } + + // Router/dashboard context: resolve from DB return resolveIntegrationCredential(projectId, category, role); } @@ -166,6 +180,12 @@ export async function getOrgCredential( return process.env[envVarKey]; } + // Worker context: all credentials set by router, this one doesn't exist + if (process.env.CASCADE_CREDENTIAL_KEYS) { + return null; + } + + // Router/dashboard context: resolve from DB const orgId = await getOrgIdForProject(projectId); return resolveOrgCredential(orgId, envVarKey); } @@ -181,6 +201,19 @@ export async function getOrgCredential( * 3. Merges integration credentials over org defaults */ export async function getAllProjectCredentials(projectId: string): Promise> { + // Worker context: reconstruct from individual env vars set by the router + const keyList = process.env.CASCADE_CREDENTIAL_KEYS; + if (keyList) { + const result: Record = {}; + for (const key of keyList.split(',')) { + if (key && process.env[key]) { + result[key] = process.env[key]; + } + } + return result; + } + + // Router/dashboard context: resolve from DB (has CREDENTIAL_MASTER_KEY) const orgId = await getOrgIdForProject(projectId); const [integrationCreds, orgCreds] = await Promise.all([ diff --git a/src/router/worker-manager.ts b/src/router/worker-manager.ts index 4a4f0b22..4db95646 100644 --- a/src/router/worker-manager.ts +++ b/src/router/worker-manager.ts @@ -63,15 +63,16 @@ async function buildWorkerEnv(job: Job): Promise { `LOG_LEVEL=${process.env.LOG_LEVEL || 'info'}`, ]; - // Resolve project credentials in the router and pass as JSON. - // Workers cache these on startup, then scrub from env. + // Resolve project credentials in the router and set as individual env vars. // NOTE: CREDENTIAL_MASTER_KEY is intentionally NOT passed to workers. const projectId = await extractProjectIdFromJob(job.data); if (projectId) { try { const secrets = await getAllProjectCredentials(projectId); - env.push(`CASCADE_CREDENTIALS=${JSON.stringify(secrets)}`); - env.push(`CASCADE_CREDENTIALS_PROJECT_ID=${projectId}`); + for (const [key, value] of Object.entries(secrets)) { + env.push(`${key}=${value}`); + } + env.push(`CASCADE_CREDENTIAL_KEYS=${Object.keys(secrets).join(',')}`); } catch (err) { console.warn('[WorkerManager] Failed to resolve credentials for project:', { projectId, @@ -93,7 +94,7 @@ async function spawnWorker(job: Job): Promise { const containerName = `cascade-worker-${jobId}`; const workerEnv = await buildWorkerEnv(job); - const hasCredentials = workerEnv.some((e) => e.startsWith('CASCADE_CREDENTIALS=')); + const hasCredentials = workerEnv.some((e) => e.startsWith('CASCADE_CREDENTIAL_KEYS=')); console.log('[WorkerManager] Spawning worker:', { jobId, diff --git a/src/utils/envScrub.ts b/src/utils/envScrub.ts index db7d795c..17d78155 100644 --- a/src/utils/envScrub.ts +++ b/src/utils/envScrub.ts @@ -15,8 +15,6 @@ const SENSITIVE_ENV_KEYS = [ 'DATABASE_URL', 'DATABASE_SSL', 'REDIS_URL', - 'CASCADE_CREDENTIALS', - 'CASCADE_CREDENTIALS_PROJECT_ID', ] as const; /** @@ -24,7 +22,6 @@ const SENSITIVE_ENV_KEYS = [ * * Call this AFTER: * - Database connection pool is initialized (getDb()) - * - Project credentials are set as individual env vars (loadRouterCredentials()) * * After scrubbing: * - Database pool continues to work (uses cached connection string) diff --git a/src/worker-entry.ts b/src/worker-entry.ts index 9eb4785a..b28efd85 100644 --- a/src/worker-entry.ts +++ b/src/worker-entry.ts @@ -88,20 +88,6 @@ type DashboardJobData = ManualRunJobData | RetryRunJobData | DebugAnalysisJobDat type JobData = TrelloJobData | GitHubJobData | JiraJobData | DashboardJobData; -function loadRouterCredentials(): void { - const credentialsJson = process.env.CASCADE_CREDENTIALS; - if (!credentialsJson) return; - try { - const secrets = JSON.parse(credentialsJson) as Record; - for (const [key, value] of Object.entries(secrets)) { - process.env[key] = value; - } - logger.info('[Worker] Set credentials as env vars from router'); - } catch (err) { - logger.warn('[Worker] Failed to parse CASCADE_CREDENTIALS', { error: String(err) }); - } -} - async function processDashboardJob(jobId: string, jobData: DashboardJobData): Promise { const { loadProjectConfigById } = await import('./config/provider.js'); @@ -177,18 +163,15 @@ async function main(): Promise { const config = await loadConfig(); logger.info('[Worker] Loaded projects config', { projects: config.projects.map((p) => p.id) }); - // Set credentials as individual env vars (passed as JSON in CASCADE_CREDENTIALS). - // Router resolves and decrypts credentials before spawning workers, so workers - // never need the CREDENTIAL_MASTER_KEY. - if (process.env.CASCADE_CREDENTIALS) { - loadRouterCredentials(); - } else { + // Credentials are set as individual env vars by the router (Docker env). + // CASCADE_CREDENTIAL_KEYS lists the key names for reconstruction. + if (!process.env.CASCADE_CREDENTIAL_KEYS) { logger.error('[Worker] No credentials passed from router - job will likely fail', { jobType: jobData.type, }); } - // SECURITY: Scrub sensitive env vars (DATABASE_URL, CASCADE_CREDENTIALS, etc.) + // SECURITY: Scrub sensitive env vars (DATABASE_URL, etc.) // before agent execution. Subprocesses (Tmux, etc.) will not inherit these secrets. scrubSensitiveEnv(); logger.info('[Worker] Scrubbed sensitive env vars'); diff --git a/tests/unit/agents/prompts.test.ts b/tests/unit/agents/prompts.test.ts index b5bb74c8..79662cbd 100644 --- a/tests/unit/agents/prompts.test.ts +++ b/tests/unit/agents/prompts.test.ts @@ -77,6 +77,38 @@ describe('system prompts content', () => { expect(prompt).toContain('Tmux'); expect(prompt).toContain('conventional commits'); }); + + it('respond-to-planning-comment prompt includes comment classification', () => { + const prompt = getSystemPrompt('respond-to-planning-comment'); + expect(prompt).toContain('Comment Classification'); + expect(prompt).toContain('Question / Clarification'); + expect(prompt).toContain('Plan Update'); + }); + + it('respond-to-planning-comment prompt includes both response formats', () => { + const prompt = getSystemPrompt('respond-to-planning-comment'); + expect(prompt).toContain('Plan Updated'); + expect(prompt).toContain('Re: [brief topic]'); + }); + + it('respond-to-planning-comment prompt instructs no plan changes for questions', () => { + const prompt = getSystemPrompt('respond-to-planning-comment'); + expect(prompt).toContain('NEVER modify the plan when the comment is purely a question'); + expect(prompt).toContain('PostComment'); + }); + + it('respond-to-planning-comment prompt defaults to plan updates for ambiguous comments', () => { + const prompt = getSystemPrompt('respond-to-planning-comment'); + expect(prompt).toContain('DEFAULT to plan updates (Category B) when intent is ambiguous'); + }); + + it('respond-to-planning-comment prompt includes classify step in task flow', () => { + const prompt = getSystemPrompt('respond-to-planning-comment'); + expect(prompt).toContain('Classify the comment'); + expect(prompt).toContain('Category A (Question)'); + expect(prompt).toContain('Category B (Plan Update)'); + expect(prompt).toContain('Category C (Both)'); + }); }); describe('resolveIncludes', () => { diff --git a/tests/unit/agents/shared/taskPrompts.test.ts b/tests/unit/agents/shared/taskPrompts.test.ts index 150b3cf4..7a484e18 100644 --- a/tests/unit/agents/shared/taskPrompts.test.ts +++ b/tests/unit/agents/shared/taskPrompts.test.ts @@ -30,7 +30,7 @@ describe('buildCommentResponsePrompt', () => { expect(prompt).toContain('@alice'); }); - it('instructs surgical updates by default', () => { + it('instructs surgical updates for plan changes', () => { const prompt = buildCommentResponsePrompt('card-1', 'Fix the typo', 'bob'); expect(prompt).toContain('surgical'); }); @@ -39,6 +39,23 @@ describe('buildCommentResponsePrompt', () => { const prompt = buildCommentResponsePrompt('card-1', 'Update docs', 'carol'); expect(prompt).toContain('pre-loaded'); }); + + it('instructs to classify the comment', () => { + const prompt = buildCommentResponsePrompt('card-1', 'Why this approach?', 'dave'); + expect(prompt).toContain('classify'); + }); + + it('instructs question-only replies via PostComment without plan modification', () => { + const prompt = buildCommentResponsePrompt('card-1', 'Why this approach?', 'dave'); + expect(prompt).toContain('question'); + expect(prompt).toContain('PostComment'); + expect(prompt).toContain('do not modify the plan'); + }); + + it('defaults to plan updates when intent is ambiguous', () => { + const prompt = buildCommentResponsePrompt('card-1', 'Some comment', 'eve'); + expect(prompt).toContain('Default to plan updates when intent is ambiguous'); + }); }); describe('buildReviewPrompt', () => { diff --git a/tests/unit/backends/secretBuilder.test.ts b/tests/unit/backends/secretBuilder.test.ts index 967e1437..33edb1d6 100644 --- a/tests/unit/backends/secretBuilder.test.ts +++ b/tests/unit/backends/secretBuilder.test.ts @@ -157,19 +157,11 @@ describe('resolveGitHubToken', () => { expect(mockGetPersonaToken).toHaveBeenCalledWith('project-id', 'implementation'); }); - it('falls back to GITHUB_TOKEN from credentials when getPersonaToken throws', async () => { + it('propagates error when getPersonaToken throws', async () => { mockGetPersonaToken.mockRejectedValue(new Error('persona not found')); - mockGetAllProjectCredentials.mockResolvedValue({ GITHUB_TOKEN: 'fallback-token' }); const profile = makeProfile({ needsGitHubToken: true }); - const token = await resolveGitHubToken(profile, 'project-id', 'implementation'); - expect(token).toBe('fallback-token'); - }); - - it('returns undefined from fallback when GITHUB_TOKEN credential is missing', async () => { - mockGetPersonaToken.mockRejectedValue(new Error('persona not found')); - mockGetAllProjectCredentials.mockResolvedValue({}); - const profile = makeProfile({ needsGitHubToken: true }); - const token = await resolveGitHubToken(profile, 'project-id', 'implementation'); - expect(token).toBeUndefined(); + await expect(resolveGitHubToken(profile, 'project-id', 'implementation')).rejects.toThrow( + 'persona not found', + ); }); }); diff --git a/tests/unit/config/provider.test.ts b/tests/unit/config/provider.test.ts index 537af387..efd4986c 100644 --- a/tests/unit/config/provider.test.ts +++ b/tests/unit/config/provider.test.ts @@ -311,6 +311,15 @@ describe('config/provider', () => { "Integration credential 'pm/api_key' not found for project 'proj1'", ); }); + + it('throws without DB fallback when CASCADE_CREDENTIAL_KEYS is set (worker context)', async () => { + setEnvCredential('CASCADE_CREDENTIAL_KEYS', 'OTHER_KEY'); + + await expect(getIntegrationCredential('proj1', 'pm', 'api_key')).rejects.toThrow( + "Integration credential 'pm/api_key' not found for project 'proj1'", + ); + expect(resolveIntegrationCredential).not.toHaveBeenCalled(); + }); }); describe('getIntegrationCredentialOrNull', () => { @@ -337,6 +346,15 @@ describe('config/provider', () => { expect(result).toBe('db-token'); }); + + it('returns null without DB fallback when CASCADE_CREDENTIAL_KEYS is set (worker context)', async () => { + setEnvCredential('CASCADE_CREDENTIAL_KEYS', 'OTHER_KEY'); + + const result = await getIntegrationCredentialOrNull('proj1', 'scm', 'implementer_token'); + + expect(result).toBeNull(); + expect(resolveIntegrationCredential).not.toHaveBeenCalled(); + }); }); describe('getOrgCredential', () => { @@ -376,6 +394,15 @@ describe('config/provider', () => { await expect(getOrgCredential('proj1', 'KEY')).rejects.toThrow('Project not found: proj1'); }); + + it('returns null without DB fallback when CASCADE_CREDENTIAL_KEYS is set (worker context)', async () => { + setEnvCredential('CASCADE_CREDENTIAL_KEYS', 'OTHER_KEY'); + + const result = await getOrgCredential('proj1', 'OPENROUTER_API_KEY'); + + expect(result).toBeNull(); + expect(resolveOrgCredential).not.toHaveBeenCalled(); + }); }); describe('getAllProjectCredentials', () => { @@ -411,6 +438,18 @@ describe('config/provider', () => { const result = await getAllProjectCredentials('proj1'); expect(result).toEqual({}); }); + + it('reconstructs credentials from env vars when CASCADE_CREDENTIAL_KEYS is set (worker context)', async () => { + setEnvCredential('CASCADE_CREDENTIAL_KEYS', 'TRELLO_API_KEY,OPENROUTER_API_KEY'); + setEnvCredential('TRELLO_API_KEY', 'env-key'); + setEnvCredential('OPENROUTER_API_KEY', 'env-or'); + + const result = await getAllProjectCredentials('proj1'); + + expect(result).toEqual({ TRELLO_API_KEY: 'env-key', OPENROUTER_API_KEY: 'env-or' }); + expect(resolveAllIntegrationCredentials).not.toHaveBeenCalled(); + expect(resolveAllOrgCredentials).not.toHaveBeenCalled(); + }); }); describe('invalidateConfigCache', () => { diff --git a/tests/unit/utils/envScrub.test.ts b/tests/unit/utils/envScrub.test.ts index be1c1480..22553102 100644 --- a/tests/unit/utils/envScrub.test.ts +++ b/tests/unit/utils/envScrub.test.ts @@ -12,8 +12,6 @@ describe('scrubSensitiveEnv', () => { DATABASE_URL: process.env.DATABASE_URL, DATABASE_SSL: process.env.DATABASE_SSL, REDIS_URL: process.env.REDIS_URL, - CASCADE_CREDENTIALS: process.env.CASCADE_CREDENTIALS, - CASCADE_CREDENTIALS_PROJECT_ID: process.env.CASCADE_CREDENTIALS_PROJECT_ID, }; }); @@ -52,25 +50,11 @@ describe('scrubSensitiveEnv', () => { expect(process.env.REDIS_URL).toBeUndefined(); }); - it('removes CASCADE_CREDENTIALS from process.env', () => { - process.env.CASCADE_CREDENTIALS = 'eyJzb21lIjoianNvbiJ9'; - scrubSensitiveEnv(); - expect(process.env.CASCADE_CREDENTIALS).toBeUndefined(); - }); - - it('removes CASCADE_CREDENTIALS_PROJECT_ID from process.env', () => { - process.env.CASCADE_CREDENTIALS_PROJECT_ID = 'my-project-id'; - scrubSensitiveEnv(); - expect(process.env.CASCADE_CREDENTIALS_PROJECT_ID).toBeUndefined(); - }); - it('removes all sensitive keys in a single call', () => { process.env.CREDENTIAL_MASTER_KEY = 'key1'; process.env.DATABASE_URL = 'postgres://...'; process.env.DATABASE_SSL = 'true'; process.env.REDIS_URL = 'redis://...'; - process.env.CASCADE_CREDENTIALS = 'creds'; - process.env.CASCADE_CREDENTIALS_PROJECT_ID = 'proj-id'; scrubSensitiveEnv(); @@ -78,8 +62,6 @@ describe('scrubSensitiveEnv', () => { expect(process.env.DATABASE_URL).toBeUndefined(); expect(process.env.DATABASE_SSL).toBeUndefined(); expect(process.env.REDIS_URL).toBeUndefined(); - expect(process.env.CASCADE_CREDENTIALS).toBeUndefined(); - expect(process.env.CASCADE_CREDENTIALS_PROJECT_ID).toBeUndefined(); }); it('does not remove non-sensitive environment variables', () => {