diff --git a/apps/worker/src/mcp/roomote-mcp-server/__tests__/slack-reply-satisfaction.test.ts b/apps/worker/src/mcp/roomote-mcp-server/__tests__/slack-reply-satisfaction.test.ts index d7de9c59f..1d3db3af0 100644 --- a/apps/worker/src/mcp/roomote-mcp-server/__tests__/slack-reply-satisfaction.test.ts +++ b/apps/worker/src/mcp/roomote-mcp-server/__tests__/slack-reply-satisfaction.test.ts @@ -373,6 +373,67 @@ describe('Slack reply satisfaction state', () => { }); }); + it('writes terminal state when a current-turn clarification reply is recorded', () => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'roomote-slack-')); + tempDirs.push(tempDir); + const stateFilePath = path.join(tempDir, 'reply-state.json'); + process.env[SLACK_REPLY_SATISFACTION_STATE_FILE_ENV] = stateFilePath; + fs.writeFileSync( + stateFilePath, + JSON.stringify({ + currentTurnMessageTs: '111.222', + currentTurnStartedAtMs: 1000, + }), + 'utf8', + ); + + recordSlackReplySatisfaction({ + messageTs: 'bot-333.444', + tool: 'send_chat_reply', + replyPurpose: 'clarification', + nowMs: 1200, + }); + + expect(JSON.parse(fs.readFileSync(stateFilePath, 'utf8'))).toEqual({ + currentTurnMessageTs: '111.222', + currentTurnStartedAtMs: 1000, + satisfiedTurnMessageTs: '111.222', + terminalSatisfiedTurnMessageTs: '111.222', + terminalSatisfiedAtMs: 1200, + terminalSatisfactionTool: 'send_chat_reply', + messageTs: 'bot-333.444', + tool: 'send_chat_reply', + replyPurpose: 'clarification', + recordedAtMs: 1200, + }); + }); + + it('does not write terminal state for ack or progress replies', () => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'roomote-slack-')); + tempDirs.push(tempDir); + const stateFilePath = path.join(tempDir, 'reply-state.json'); + process.env[SLACK_REPLY_SATISFACTION_STATE_FILE_ENV] = stateFilePath; + fs.writeFileSync( + stateFilePath, + JSON.stringify({ + currentTurnMessageTs: '111.222', + currentTurnStartedAtMs: 1000, + }), + 'utf8', + ); + + recordSlackReplySatisfaction({ + messageTs: 'bot-333.444', + tool: 'send_chat_reply', + replyPurpose: 'progress', + nowMs: 1200, + }); + + const state = JSON.parse(fs.readFileSync(stateFilePath, 'utf8')); + expect(state.terminalSatisfiedTurnMessageTs).toBeUndefined(); + expect(state.terminalSatisfiedAtMs).toBeUndefined(); + }); + it('does not clobber terminal closeout state when a later reaction overwrites the latest tool markers', () => { const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'roomote-slack-')); tempDirs.push(tempDir); diff --git a/apps/worker/src/mcp/roomote-mcp-server/__tests__/tool-descriptions.test.ts b/apps/worker/src/mcp/roomote-mcp-server/__tests__/tool-descriptions.test.ts index 02685ac31..475d98f69 100644 --- a/apps/worker/src/mcp/roomote-mcp-server/__tests__/tool-descriptions.test.ts +++ b/apps/worker/src/mcp/roomote-mcp-server/__tests__/tool-descriptions.test.ts @@ -237,7 +237,7 @@ describe('roomote MCP tool descriptions', () => { 'Choose the current Slack turn purpose before writing: ack, progress, closeout, or clarification.', ); expect(replyTool.config.description).toContain( - 'Use ack for the first visible response when work will continue; use progress only when the message adds new decision-useful state or prevents a 10-minute silence gap; use closeout for the answer, result, blocker, or handoff; use clarification for lightweight non-secret questions. Only closeout is terminal for final task completion; ack, progress, and clarification keep the Slack turn open.', + `Use ack for the first visible response when work will continue; use progress only when the message adds new decision-useful state or prevents a 10-minute silence gap; use closeout for the answer, result, blocker, or handoff; use clarification for lightweight non-secret questions. Use closeout to finish a turn with an outcome; a clarification also ends the turn when the next step depends on the user's answer — do not follow it with a separate "waiting on your answer" message. Ack and progress keep the Slack turn open.`, ); expect(replyTool.config.description).toContain( "For routine successful closeouts, focus on the shipped change and any blocker or delivery outcome that changes the user's next step; do not include exact validation commands, passed-check ledgers, or proof-applicability narration unless the user asked or that detail materially changes what they should do next.", @@ -605,7 +605,7 @@ describe('roomote MCP tool descriptions', () => { const chatReplyTool = getRegisteredTool(registeredTools, 'send_chat_reply'); expect(chatReplyTool.config.description).toBe( - "Slack-visible: posts a lifecycle reply in the originating Slack thread. Choose the current Slack turn purpose before writing: ack, progress, closeout, or clarification. Use ack for the first visible response when work will continue; use progress only when the message adds new decision-useful state or prevents a 10-minute silence gap; use closeout for the answer, result, blocker, or handoff; use clarification for lightweight non-secret questions. Only closeout is terminal for final task completion; ack, progress, and clarification keep the Slack turn open. Use it again on later Slack turns when they need another direct reply; an earlier thread reply does not count as the reply for the current turn. For routine successful closeouts, focus on the shipped change and any blocker or delivery outcome that changes the user's next step; do not include exact validation commands, passed-check ledgers, or proof-applicability narration unless the user asked or that detail materially changes what they should do next. Supports the modern Slack Markdown contract from the Slack instructions. Use rich Markdown when it improves scanability. When the reply mentions actionable code references, follow the Slack prompt source-linking rule. Write the message so its content clearly matches the selected purpose.", + `Slack-visible: posts a lifecycle reply in the originating Slack thread. Choose the current Slack turn purpose before writing: ack, progress, closeout, or clarification. Use ack for the first visible response when work will continue; use progress only when the message adds new decision-useful state or prevents a 10-minute silence gap; use closeout for the answer, result, blocker, or handoff; use clarification for lightweight non-secret questions. Use closeout to finish a turn with an outcome; a clarification also ends the turn when the next step depends on the user's answer — do not follow it with a separate "waiting on your answer" message. Ack and progress keep the Slack turn open. Use it again on later Slack turns when they need another direct reply; an earlier thread reply does not count as the reply for the current turn. For routine successful closeouts, focus on the shipped change and any blocker or delivery outcome that changes the user's next step; do not include exact validation commands, passed-check ledgers, or proof-applicability narration unless the user asked or that detail materially changes what they should do next. Supports the modern Slack Markdown contract from the Slack instructions. Use rich Markdown when it improves scanability. When the reply mentions actionable code references, follow the Slack prompt source-linking rule. Write the message so its content clearly matches the selected purpose.`, ); expect(getInputSchemaField(chatReplyTool, 'message').description).toBe( "Markdown text to post in the Slack thread. Match the selected purpose, lead with the useful takeaway, and keep it conversational like a teammate in a thread. For routine successful closeouts, focus on the shipped change and any blocker or delivery outcome that changes the user's next step instead of listing exact validation commands, passed checks, or proof-applicability notes unless the user asked for them or they materially change what the user should do next. Use the modern Slack Markdown contract from the Slack instructions; tables, headings, blockquotes, and fenced code blocks are allowed when they make the reply clearer.", diff --git a/apps/worker/src/mcp/roomote-mcp-server/chat-reply-satisfaction.ts b/apps/worker/src/mcp/roomote-mcp-server/chat-reply-satisfaction.ts index 35932609d..0a5b62669 100644 --- a/apps/worker/src/mcp/roomote-mcp-server/chat-reply-satisfaction.ts +++ b/apps/worker/src/mcp/roomote-mcp-server/chat-reply-satisfaction.ts @@ -203,8 +203,11 @@ export function recordChatReplySatisfaction(input: { lastNonSlackWorkAfterSatisfactionAtMs: undefined, } : {}), + // A clarification reply is a terminal handoff like a closeout: the turn + // ends waiting on the user's answer, so ending after one is not silence. ...(input.tool === 'send_chat_reply' && - input.replyPurpose === 'closeout' && + (input.replyPurpose === 'closeout' || + input.replyPurpose === 'clarification') && satisfiesCurrentTurn ? { terminalSatisfiedTurnMessageTs: currentTurnMessageTs, diff --git a/apps/worker/src/mcp/roomote-mcp-server/index.ts b/apps/worker/src/mcp/roomote-mcp-server/index.ts index e70ce359f..e72732698 100644 --- a/apps/worker/src/mcp/roomote-mcp-server/index.ts +++ b/apps/worker/src/mcp/roomote-mcp-server/index.ts @@ -1103,7 +1103,7 @@ if (shouldRegisterSlackThreadReplyTool()) { description: `${chatReplySurfaceLabel}-visible: posts a lifecycle reply in the originating ${chatReplySurfaceLabel} thread. ` + `Choose the current ${chatReplySurfaceLabel} turn purpose before writing: ack, progress, closeout, or clarification. ` + - `Use ack for the first visible response when work will continue; use progress only when the message adds new decision-useful state or prevents a 10-minute silence gap; use closeout for the answer, result, blocker, or handoff; use clarification for lightweight non-secret questions. Only closeout is terminal for final task completion; ack, progress, and clarification keep the ${chatReplySurfaceLabel} turn open. ` + + `Use ack for the first visible response when work will continue; use progress only when the message adds new decision-useful state or prevents a 10-minute silence gap; use closeout for the answer, result, blocker, or handoff; use clarification for lightweight non-secret questions. Use closeout to finish a turn with an outcome; a clarification also ends the turn when the next step depends on the user's answer — do not follow it with a separate "waiting on your answer" message. Ack and progress keep the ${chatReplySurfaceLabel} turn open. ` + `Use it again on later ${chatReplySurfaceLabel} turns when they need another direct reply; an earlier thread reply does not count as the reply for the current turn. ` + "For routine successful closeouts, focus on the shipped change and any blocker or delivery outcome that changes the user's next step; do not include exact validation commands, passed-check ledgers, or proof-applicability narration unless the user asked or that detail materially changes what they should do next. " + chatReplyMarkdownGuidance + diff --git a/apps/worker/src/run-task/__tests__/slack-silence-hook-script.test.ts b/apps/worker/src/run-task/__tests__/slack-silence-hook-script.test.ts index d8c95095d..845a970a4 100644 --- a/apps/worker/src/run-task/__tests__/slack-silence-hook-script.test.ts +++ b/apps/worker/src/run-task/__tests__/slack-silence-hook-script.test.ts @@ -731,6 +731,40 @@ describe('SLACK_SILENCE_HOOK_SCRIPT', () => { ).toBeUndefined(); }); + it.each(['todowrite', 'todoread'])( + 'ignores %s todo bookkeeping after a terminal closeout on PostToolUse', + (toolName) => { + const stateFilePath = writeState({ + recordedAtMs: Date.now() - 60_000, + messageTs: 'bot-111.222', + tool: 'send_chat_reply', + replyPurpose: 'closeout', + satisfiedTurnMessageTs: 'web:client-1', + currentTurnMessageTs: 'web:client-1', + terminalSatisfiedTurnMessageTs: 'web:client-1', + terminalSatisfiedAtMs: Date.now() - 60_000, + terminalSatisfactionTool: 'send_chat_reply', + }); + + const result = runHook({ + input: { + hook_event_name: 'PostToolUse', + tool_name: toolName, + }, + env: { + ROOMOTE_SLACK_REPLY_SATISFACTION_STATE_FILE: stateFilePath, + }, + }); + + expect(result.status).toBe(0); + expect(result.stdout).toBe(''); + expect( + JSON.parse(fs.readFileSync(stateFilePath, 'utf8')) + .lastNonSlackWorkAfterTerminalAtMs, + ).toBeUndefined(); + }, + ); + it.each(['request_user_input', 'request_user_input_handoff'])( 'ignores %s bookkeeping after a terminal closeout on PostToolUse', (toolName) => { diff --git a/apps/worker/src/run-task/__tests__/slack-stop-hook-script.test.ts b/apps/worker/src/run-task/__tests__/slack-stop-hook-script.test.ts index 211875f7a..b075ed978 100644 --- a/apps/worker/src/run-task/__tests__/slack-stop-hook-script.test.ts +++ b/apps/worker/src/run-task/__tests__/slack-stop-hook-script.test.ts @@ -310,7 +310,7 @@ describe('SLACK_STOP_HOOK_SCRIPT', () => { expect(result.stderr).toBe(''); }); - it.each(['ack', 'progress', 'clarification'])( + it.each(['ack', 'progress'])( 'blocks Stop when the recent send_chat_reply purpose is %s', (replyPurpose) => { const stateFilePath = path.join( @@ -350,6 +350,79 @@ describe('SLACK_STOP_HOOK_SCRIPT', () => { }, ); + it('allows Stop when the current turn ends on a clarification question', () => { + const stateFilePath = path.join( + fs.mkdtempSync(path.join(os.tmpdir(), 'roomote-stop-state-')), + 'state.json', + ); + tempDirs.push(path.dirname(stateFilePath)); + fs.writeFileSync( + stateFilePath, + JSON.stringify({ + currentTurnMessageTs: 'user-111.222', + satisfiedTurnMessageTs: 'user-111.222', + messageTs: 'bot-333.444', + tool: 'send_chat_reply', + replyPurpose: 'clarification', + recordedAtMs: Date.now(), + }), + 'utf8', + ); + + const result = runHook({ + env: { + ROOMOTE_SLACK_HOOK_DEBUG: 'true', + ROOMOTE_SLACK_REPLY_SATISFACTION_STATE_FILE: stateFilePath, + }, + }); + + expect(result.status).toBe(0); + expect(result.stdout).toBe(''); + expect(result.stderr).toContain('decision="allow"'); + expect(result.stderr).toContain('reason="terminal_reply_satisfied"'); + }); + + it('blocks Stop when non-Slack work happened after a clarification question', () => { + const clarificationAtMs = Date.now() - 60_000; + const stateFilePath = path.join( + fs.mkdtempSync(path.join(os.tmpdir(), 'roomote-stop-state-')), + 'state.json', + ); + tempDirs.push(path.dirname(stateFilePath)); + fs.writeFileSync( + stateFilePath, + JSON.stringify({ + currentTurnMessageTs: 'user-111.222', + satisfiedTurnMessageTs: 'user-111.222', + messageTs: 'bot-333.444', + tool: 'send_chat_reply', + replyPurpose: 'clarification', + recordedAtMs: clarificationAtMs, + terminalSatisfiedTurnMessageTs: 'user-111.222', + terminalSatisfiedAtMs: clarificationAtMs, + terminalSatisfactionTool: 'send_chat_reply', + lastNonSlackWorkAfterTerminalAtMs: clarificationAtMs + 30_000, + }), + 'utf8', + ); + + const result = runHook({ + env: { + ROOMOTE_SLACK_REPLY_SATISFACTION_STATE_FILE: stateFilePath, + }, + }); + + expect(result.status).toBe(0); + expect(JSON.parse(result.stdout)).toEqual({ + decision: 'block', + reason: reminder, + }); + expect(result.stderr).toContain('decision="block"'); + expect(result.stderr).toContain( + 'reason="current_turn_terminal_reply_stale"', + ); + }); + it('blocks Stop when the current Slack turn only has a recent successful reaction', () => { const stateFilePath = path.join( fs.mkdtempSync(path.join(os.tmpdir(), 'roomote-stop-state-')), diff --git a/apps/worker/src/run-task/slack-silence-hook-script.ts b/apps/worker/src/run-task/slack-silence-hook-script.ts index 7ca6b4b06..80a9ae86d 100644 --- a/apps/worker/src/run-task/slack-silence-hook-script.ts +++ b/apps/worker/src/run-task/slack-silence-hook-script.ts @@ -190,10 +190,15 @@ function isCloseoutBookkeepingTool(input) { const toolName = input && typeof input.tool_name === 'string' ? input.tool_name : ''; + // Todo bookkeeping (OpenCode's todowrite/todoread, legacy update_plan) does + // not add user-visible outcome, so it must not stale a terminal reply and + // force a redundant follow-up closeout. return ( toolName.endsWith('update_plan') || toolName.includes('/update_plan') || - toolName.includes('.update_plan') + toolName.includes('.update_plan') || + toolName.endsWith('todowrite') || + toolName.endsWith('todoread') ); } @@ -298,8 +303,14 @@ function hasCurrentTurnTerminalCloseout(state) { return false; } + // Clarification is a terminal handoff like closeout: the turn ends waiting + // on the user's answer (kept in sync with the stop hook's policy). const replyPurpose = trimString(state.replyPurpose); - if (replyPurpose && replyPurpose !== 'closeout') { + if ( + replyPurpose && + replyPurpose !== 'closeout' && + replyPurpose !== 'clarification' + ) { return false; } diff --git a/apps/worker/src/run-task/slack-stop-hook-script.ts b/apps/worker/src/run-task/slack-stop-hook-script.ts index 267cd785c..0830446e6 100644 --- a/apps/worker/src/run-task/slack-stop-hook-script.ts +++ b/apps/worker/src/run-task/slack-stop-hook-script.ts @@ -212,6 +212,16 @@ function isCurrentTurnSatisfied(state) { return trimString(state && state.satisfiedTurnMessageTs) === currentTurnMessageTs; } +// Closeout ends the turn with an outcome; clarification ends it waiting on +// the user's answer. Both are visible handoffs, so neither is "silence" — +// blocking on a fresh clarification only forces a redundant "I'm waiting on +// your answer" echo message. +function isTerminalReplyPurpose(replyPurpose) { + return ( + !replyPurpose || replyPurpose === 'closeout' || replyPurpose === 'clarification' + ); +} + function getLegacyTerminalSatisfaction(state, currentTurnMessageTs) { const tool = trimString(state && state.tool); if (tool !== 'send_chat_reply') { @@ -219,7 +229,7 @@ function getLegacyTerminalSatisfaction(state, currentTurnMessageTs) { } const replyPurpose = trimString(state && state.replyPurpose); - if (replyPurpose && replyPurpose !== 'closeout') { + if (!isTerminalReplyPurpose(replyPurpose)) { return null; } @@ -248,7 +258,7 @@ function getCurrentTurnTerminalSatisfaction(state) { } const replyPurpose = trimString(state && state.replyPurpose); - if (replyPurpose && replyPurpose !== 'closeout') { + if (!isTerminalReplyPurpose(replyPurpose)) { return null; } @@ -337,7 +347,7 @@ function getTerminalCurrentTurnFailureReason(state) { } const replyPurpose = trimString(state && state.replyPurpose); - if (replyPurpose && replyPurpose !== 'closeout') { + if (!isTerminalReplyPurpose(replyPurpose)) { return 'current_turn_nonterminal_reply'; }