From f3db05e5fc46a3237844b976856501a303c792ce Mon Sep 17 00:00:00 2001 From: ejclaw Date: Wed, 22 Apr 2026 20:48:33 +0900 Subject: [PATCH 001/285] review: harden readonly git checks and lint verification --- prompts/arbiter-paired-room.md | 2 +- prompts/claude-paired-room.md | 4 +- runners/agent-runner/src/ipc-mcp-stdio.ts | 4 +- runners/agent-runner/src/verification.ts | 7 +- .../test/reviewer-runtime.test.ts | 27 ++++++ .../test/reviewer-runtime.test.ts | 27 ++++++ runners/shared/src/reviewer-git-guard.ts | 15 +++ src/verification.test.ts | 92 ++++++++++++++++++- src/verification.ts | 16 +++- 9 files changed, 185 insertions(+), 9 deletions(-) diff --git a/prompts/arbiter-paired-room.md b/prompts/arbiter-paired-room.md index f620a39..9811ddc 100644 --- a/prompts/arbiter-paired-room.md +++ b/prompts/arbiter-paired-room.md @@ -34,7 +34,7 @@ You may receive reference opinions from external models appended to your prompt. ## Rules - Base your verdict on evidence (code, test output, logs), not on who said what first -- Distinguish reviewer snapshot limits from real product bugs. Reviewer workspaces may intentionally omit heavy artifacts like `node_modules`, `dist`, and `build`; inability to run direct local test/typecheck/build there is not, by itself, a blocker if dedicated verification evidence exists +- Distinguish reviewer snapshot limits from real product bugs. Reviewer workspaces may intentionally omit heavy artifacts like `node_modules`, `dist`, and `build`; inability to run direct local test/typecheck/build/lint there is not, by itself, a blocker if dedicated verification evidence exists - When verification evidence exists from the dedicated verification path, judge that evidence on its merits instead of requiring the reviewer to reproduce the same result from the lightweight reviewer snapshot - Your verdict is final for this deadlock cycle — after it, work resumes normally - You do NOT implement or review code — you only judge the disagreement diff --git a/prompts/claude-paired-room.md b/prompts/claude-paired-room.md index 87d9803..1d422fd 100644 --- a/prompts/claude-paired-room.md +++ b/prompts/claude-paired-room.md @@ -32,10 +32,10 @@ If you see a materially better design, debugging path, or scoping choice, propos ## Rules - Judge completion only by verification output. "It should work now" means run it. "I'm confident" means nothing — confidence is not evidence. "I tested earlier" means test again if code changed since. "It's a trivial change" means verify anyway -- Reviewer runs against the owner's current workspace in read-only mode. Do not treat the inability to run direct local test/typecheck/build from the reviewer workspace as a product bug by itself +- Reviewer runs against the owner's current workspace in read-only mode. Do not treat the inability to run direct local test/typecheck/build/lint from the reviewer workspace as a product bug by itself - Treat `EJCLAW_WORK_DIR` as the canonical verification root for this turn. You may inspect other local paths for context, but final review findings must be re-checked against `EJCLAW_WORK_DIR` - Do not use a different clone, canonical repo path, or cached session path as the sole basis for `BLOCKED`, `DONE_WITH_CONCERNS`, or change requests. If another path disagrees with `EJCLAW_WORK_DIR`, prefer `EJCLAW_WORK_DIR` and explicitly call out the mismatch -- When test/typecheck/build evidence is needed, prefer the dedicated verification path (`run_verification`) over assuming the reviewer workspace should execute the full project locally +- When test/typecheck/build/lint evidence is needed, prefer the dedicated verification path (`run_verification`) over assuming the reviewer workspace should execute the full project locally - Separate correctness issues from improvement ideas. If something is only a better alternative, label it as optional instead of blocking the owner unnecessarily - Stagnation: **Spinning** (same error 3+), **Oscillation** (alternating approaches), **Diminishing returns** (shrinking improvement), **No progress** (discussion without change) — name the pattern and report: **Status**, **Attempted**, **Recommendation** - Implementation, commits, and pushes require agreement from both sides. Either can veto diff --git a/runners/agent-runner/src/ipc-mcp-stdio.ts b/runners/agent-runner/src/ipc-mcp-stdio.ts index b47c67f..0fe7ad3 100644 --- a/runners/agent-runner/src/ipc-mcp-stdio.ts +++ b/runners/agent-runner/src/ipc-mcp-stdio.ts @@ -479,12 +479,12 @@ server.tool( server.tool( 'run_verification', - 'Run a fixed verification profile directly on the host against the current repo snapshot using a restricted environment and snapshot checks. Use this instead of broad shell write access for test/typecheck/build verification.', + 'Run a fixed verification profile directly on the host against the current repo snapshot using a restricted environment and snapshot checks. Use this instead of broad shell write access for test/typecheck/build/lint verification.', { profile: z .enum(VERIFICATION_PROFILES) .describe( - 'Fixed verification profile. Runs the workspace test/typecheck/build scripts with the repo-configured package manager.', + 'Fixed verification profile. Runs the workspace test/typecheck/build/lint scripts with the repo-configured package manager.', ), }, async (args) => { diff --git a/runners/agent-runner/src/verification.ts b/runners/agent-runner/src/verification.ts index 01cdc62..4d7c662 100644 --- a/runners/agent-runner/src/verification.ts +++ b/runners/agent-runner/src/verification.ts @@ -3,7 +3,12 @@ import path from 'path'; import { pathToFileURL } from 'url'; export { computeVerificationSnapshotId } from '../../../shared/verification-snapshot.js'; -export const VERIFICATION_PROFILES = ['test', 'typecheck', 'build'] as const; +export const VERIFICATION_PROFILES = [ + 'test', + 'typecheck', + 'build', + 'lint', +] as const; export type VerificationProfile = (typeof VERIFICATION_PROFILES)[number]; diff --git a/runners/agent-runner/test/reviewer-runtime.test.ts b/runners/agent-runner/test/reviewer-runtime.test.ts index a8f06b0..0052108 100644 --- a/runners/agent-runner/test/reviewer-runtime.test.ts +++ b/runners/agent-runner/test/reviewer-runtime.test.ts @@ -169,6 +169,9 @@ describe('claude reviewer runtime guard', () => { const env = buildReviewerGitGuardEnv({ PATH: process.env.PATH }, true); expect(env.PATH).toContain('ejclaw-reviewer-git-'); expect(env.EJCLAW_REAL_GIT).toBeTruthy(); + expect(env.GIT_CONFIG_GLOBAL).toContain('global.gitconfig'); + expect(env.GIT_CONFIG_NOSYSTEM).toBe('1'); + expect(fs.existsSync(env.GIT_CONFIG_GLOBAL!)).toBe(true); }); it('prefers an executable HOME-scoped wrapper dir before tmp', () => { @@ -245,6 +248,30 @@ describe('claude reviewer runtime guard', () => { } }); + it('overrides problematic git config paths so read-only git queries still work', () => { + const cwd = createTempRepo('ejclaw-reviewer-readonly-git-'); + const env = buildReviewerGitGuardEnv( + { + PATH: process.env.PATH, + HOME: cwd, + EJCLAW_WORK_DIR: cwd, + GIT_CONFIG_GLOBAL: path.join(cwd, '.gitconfig'), + }, + true, + ); + + expect(env.GIT_CONFIG_GLOBAL).not.toBe(path.join(cwd, '.gitconfig')); + expect(() => + execFileSync('git', ['log', '--oneline', '-1'], { + cwd, + env, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'pipe'], + }), + ).not.toThrow(); + expect(fs.existsSync(path.join(cwd, '.gitconfig'))).toBe(false); + }); + it('accepts a mounted local origin path that resolves as a git repo', () => { const originDir = createTempRepo('ejclaw-reviewer-origin-'); const cwd = createTempRepo('ejclaw-reviewer-workspace-'); diff --git a/runners/codex-runner/test/reviewer-runtime.test.ts b/runners/codex-runner/test/reviewer-runtime.test.ts index d4493e3..04df0a0 100644 --- a/runners/codex-runner/test/reviewer-runtime.test.ts +++ b/runners/codex-runner/test/reviewer-runtime.test.ts @@ -75,6 +75,9 @@ describe('codex reviewer runtime guard', () => { const env = buildReviewerGitGuardEnv({ PATH: process.env.PATH }, true); expect(env.PATH).toContain('ejclaw-reviewer-git-'); expect(env.EJCLAW_REAL_GIT).toBeTruthy(); + expect(env.GIT_CONFIG_GLOBAL).toContain('global.gitconfig'); + expect(env.GIT_CONFIG_NOSYSTEM).toBe('1'); + expect(fs.existsSync(env.GIT_CONFIG_GLOBAL!)).toBe(true); }); it('prefers an executable HOME-scoped wrapper dir before tmp', () => { @@ -151,6 +154,30 @@ describe('codex reviewer runtime guard', () => { } }); + it('overrides problematic git config paths so read-only git queries still work', () => { + const cwd = createTempRepo('ejclaw-reviewer-readonly-git-'); + const env = buildReviewerGitGuardEnv( + { + PATH: process.env.PATH, + HOME: cwd, + EJCLAW_WORK_DIR: cwd, + GIT_CONFIG_GLOBAL: path.join(cwd, '.gitconfig'), + }, + true, + ); + + expect(env.GIT_CONFIG_GLOBAL).not.toBe(path.join(cwd, '.gitconfig')); + expect(() => + execFileSync('git', ['log', '--oneline', '-1'], { + cwd, + env, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'pipe'], + }), + ).not.toThrow(); + expect(fs.existsSync(path.join(cwd, '.gitconfig'))).toBe(false); + }); + it('accepts a mounted local origin path that resolves as a git repo', () => { const originDir = createTempRepo('ejclaw-reviewer-origin-'); const cwd = createTempRepo('ejclaw-reviewer-workspace-'); diff --git a/runners/shared/src/reviewer-git-guard.ts b/runners/shared/src/reviewer-git-guard.ts index 3fedb8d..3d62cf5 100644 --- a/runners/shared/src/reviewer-git-guard.ts +++ b/runners/shared/src/reviewer-git-guard.ts @@ -78,6 +78,18 @@ function createExecutableWrapperDir(baseEnv: NodeJS.ProcessEnv): string { ); } +function prepareGitConfigSandbox(wrapperDir: string): { + gitConfigGlobalPath: string; +} { + const configDir = path.join(wrapperDir, 'git-config'); + fs.mkdirSync(configDir, { recursive: true }); + const gitConfigGlobalPath = path.join(configDir, 'global.gitconfig'); + if (!fs.existsSync(gitConfigGlobalPath)) { + fs.writeFileSync(gitConfigGlobalPath, ''); + } + return { gitConfigGlobalPath }; +} + export function buildReviewerGitGuardEnv( baseEnv: NodeJS.ProcessEnv, reviewerRuntime: boolean, @@ -90,6 +102,7 @@ export function buildReviewerGitGuardEnv( const protectedWorkDir = baseEnv.EJCLAW_WORK_DIR || ''; const wrapperDir = createExecutableWrapperDir(baseEnv); const wrapperPath = path.join(wrapperDir, 'git'); + const { gitConfigGlobalPath } = prepareGitConfigSandbox(wrapperDir); const blocked = [...BLOCKED_GIT_SUBCOMMANDS] .map((value) => `'${value}'`) .join(' '); @@ -179,6 +192,8 @@ exec "$real_git" "$@" ...baseEnv, EJCLAW_REAL_GIT: realGitPath, EJCLAW_PROTECTED_WORK_DIR: protectedWorkDir, + GIT_CONFIG_GLOBAL: gitConfigGlobalPath, + GIT_CONFIG_NOSYSTEM: '1', PATH: `${wrapperDir}:${baseEnv.PATH || ''}`, }; } diff --git a/src/verification.test.ts b/src/verification.test.ts index 4555424..de8fb61 100644 --- a/src/verification.test.ts +++ b/src/verification.test.ts @@ -20,7 +20,7 @@ describe('verification helpers', () => { expect(isVerificationProfile('test')).toBe(true); expect(isVerificationProfile('typecheck')).toBe(true); expect(isVerificationProfile('build')).toBe(true); - expect(isVerificationProfile('lint')).toBe(false); + expect(isVerificationProfile('lint')).toBe(true); }); it('builds deterministic commands for each profile', () => { @@ -44,6 +44,49 @@ describe('verification helpers', () => { args: ['run', 'build'], requiredScript: 'build', }); + expect(buildVerificationCommand('lint', repoDir)).toMatchObject({ + file: 'npm', + args: ['run', 'lint'], + requiredScript: 'lint', + }); + }); + + it('prefers lint:ci and lint:check before plain lint', () => { + const repoDir = fs.mkdtempSync( + path.join(os.tmpdir(), 'ejclaw-verification-lint-'), + ); + fs.writeFileSync( + path.join(repoDir, 'package.json'), + JSON.stringify({ + scripts: { + lint: 'eslint .', + 'lint:check': 'eslint --max-warnings 0 .', + 'lint:ci': 'eslint --format compact .', + }, + }), + ); + + expect(buildVerificationCommand('lint', repoDir)).toMatchObject({ + file: 'npm', + args: ['run', 'lint:ci'], + requiredScript: 'lint:ci', + }); + + fs.writeFileSync( + path.join(repoDir, 'package.json'), + JSON.stringify({ + scripts: { + lint: 'eslint .', + 'lint:check': 'eslint --max-warnings 0 .', + }, + }), + ); + + expect(buildVerificationCommand('lint', repoDir)).toMatchObject({ + file: 'npm', + args: ['run', 'lint:check'], + requiredScript: 'lint:check', + }); }); it('selects the workspace package manager for verification commands', () => { @@ -250,6 +293,53 @@ describe('verification helpers', () => { expect(result.snapshotId).toBe(expectedSnapshotId); }); + it('runs lint verification using the preferred lint script', async () => { + const repoDir = fs.mkdtempSync( + path.join(os.tmpdir(), 'ejclaw-verification-direct-lint-'), + ); + fs.mkdirSync(path.join(repoDir, 'node_modules', '.bin'), { + recursive: true, + }); + fs.mkdirSync(path.join(repoDir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(repoDir, 'package.json'), + JSON.stringify({ + name: 'verification-direct-lint', + packageManager: 'bun@1.3.11', + scripts: { + 'lint:check': + "node -e \"process.stdout.write('lint-check:' + (process.env.CI || 'missing'))\"", + }, + }), + ); + fs.writeFileSync(path.join(repoDir, 'bun.lock'), ''); + fs.writeFileSync( + path.join(repoDir, 'node_modules', '.bin', 'placeholder'), + '', + ); + fs.writeFileSync( + path.join(repoDir, 'src', 'index.ts'), + 'export const value = 1;\n', + ); + ensureWorkspaceDependenciesInstalled(repoDir); + + const expectedSnapshotId = computeVerificationSnapshot(repoDir).snapshotId; + const result = await runVerificationRequest( + { + requestId: 'req-direct-lint', + profile: 'lint', + expectedSnapshotId, + }, + { repoDir }, + ); + + expect(result.ok).toBe(true); + expect(result.exitCode).toBe(0); + expect(result.command).toContain('lint:check'); + expect(result.stdout).toContain('lint-check:1'); + expect(result.snapshotId).toBe(expectedSnapshotId); + }); + it('does not leak host secrets into direct verification commands', async () => { const repoDir = fs.mkdtempSync( path.join(os.tmpdir(), 'ejclaw-verification-direct-secret-'), diff --git a/src/verification.ts b/src/verification.ts index 787502d..4aacc2a 100644 --- a/src/verification.ts +++ b/src/verification.ts @@ -11,7 +11,12 @@ import { } from './workspace-package-manager.js'; import { computeVerificationSnapshotId } from '../shared/verification-snapshot.js'; -export const VERIFICATION_PROFILES = ['test', 'typecheck', 'build'] as const; +export const VERIFICATION_PROFILES = [ + 'test', + 'typecheck', + 'build', + 'lint', +] as const; export type VerificationProfile = (typeof VERIFICATION_PROFILES)[number]; @@ -91,12 +96,19 @@ export function buildVerificationCommand( profile: VerificationProfile, repoDir: string = process.cwd(), ): VerificationCommandSpec { + const scripts = readPackageScripts(repoDir); const scriptName = profile === 'test' ? 'test' : profile === 'typecheck' ? 'typecheck' - : 'build'; + : profile === 'build' + ? 'build' + : scripts['lint:ci'] + ? 'lint:ci' + : scripts['lint:check'] + ? 'lint:check' + : 'lint'; const command = buildWorkspaceScriptCommand(repoDir, scriptName); return { file: command.file, From f0d96b94f6272742b4e78a2e264b146b9a3a87ec Mon Sep 17 00:00:00 2001 From: ejclaw Date: Wed, 22 Apr 2026 20:50:39 +0900 Subject: [PATCH 002/285] test: satisfy readonly reviewer sandbox typing --- runners/agent-runner/test/reviewer-runtime.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/runners/agent-runner/test/reviewer-runtime.test.ts b/runners/agent-runner/test/reviewer-runtime.test.ts index 0052108..6f968af 100644 --- a/runners/agent-runner/test/reviewer-runtime.test.ts +++ b/runners/agent-runner/test/reviewer-runtime.test.ts @@ -138,7 +138,8 @@ describe('claude reviewer runtime guard', () => { 'best-effort', ); - expect(sandbox.failIfUnavailable).toBe(false); + expect(sandbox).toBeDefined(); + expect(sandbox?.failIfUnavailable).toBe(false); }); it('keeps non-linux reviewers in best-effort sandbox mode', () => { @@ -152,7 +153,8 @@ describe('claude reviewer runtime guard', () => { 'best-effort', ); - expect(sandbox.failIfUnavailable).toBe(false); + expect(sandbox).toBeDefined(); + expect(sandbox?.failIfUnavailable).toBe(false); }); it('flags mutating shell commands', () => { From ea6d77b5ef9a1a69722c899bf5ae0b845bc9d8cb Mon Sep 17 00:00:00 2001 From: ejclaw Date: Wed, 22 Apr 2026 20:52:13 +0900 Subject: [PATCH 003/285] test: fix readonly reviewer sandbox assertions --- runners/agent-runner/test/reviewer-runtime.test.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/runners/agent-runner/test/reviewer-runtime.test.ts b/runners/agent-runner/test/reviewer-runtime.test.ts index 6f968af..3bfbde7 100644 --- a/runners/agent-runner/test/reviewer-runtime.test.ts +++ b/runners/agent-runner/test/reviewer-runtime.test.ts @@ -138,8 +138,10 @@ describe('claude reviewer runtime guard', () => { 'best-effort', ); - expect(sandbox).toBeDefined(); - expect(sandbox?.failIfUnavailable).toBe(false); + if (!sandbox) { + throw new Error('expected sandbox settings'); + } + expect(sandbox.failIfUnavailable).toBe(false); }); it('keeps non-linux reviewers in best-effort sandbox mode', () => { @@ -153,8 +155,10 @@ describe('claude reviewer runtime guard', () => { 'best-effort', ); - expect(sandbox).toBeDefined(); - expect(sandbox?.failIfUnavailable).toBe(false); + if (!sandbox) { + throw new Error('expected sandbox settings'); + } + expect(sandbox.failIfUnavailable).toBe(false); }); it('flags mutating shell commands', () => { From 94e8f5b2d58dda2b004803e196134b13a4c4f23b Mon Sep 17 00:00:00 2001 From: ejclaw Date: Wed, 22 Apr 2026 20:55:12 +0900 Subject: [PATCH 004/285] test: remove impossible readonly sandbox guards --- runners/agent-runner/test/reviewer-runtime.test.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/runners/agent-runner/test/reviewer-runtime.test.ts b/runners/agent-runner/test/reviewer-runtime.test.ts index 3bfbde7..0052108 100644 --- a/runners/agent-runner/test/reviewer-runtime.test.ts +++ b/runners/agent-runner/test/reviewer-runtime.test.ts @@ -138,9 +138,6 @@ describe('claude reviewer runtime guard', () => { 'best-effort', ); - if (!sandbox) { - throw new Error('expected sandbox settings'); - } expect(sandbox.failIfUnavailable).toBe(false); }); @@ -155,9 +152,6 @@ describe('claude reviewer runtime guard', () => { 'best-effort', ); - if (!sandbox) { - throw new Error('expected sandbox settings'); - } expect(sandbox.failIfUnavailable).toBe(false); }); From a9ca482b142ab5459fb1b93b44c7c1cf60184eb0 Mon Sep 17 00:00:00 2001 From: ejclaw Date: Wed, 22 Apr 2026 20:56:27 +0900 Subject: [PATCH 005/285] test: relax readonly sandbox assertions --- runners/agent-runner/test/reviewer-runtime.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/runners/agent-runner/test/reviewer-runtime.test.ts b/runners/agent-runner/test/reviewer-runtime.test.ts index 0052108..4d002df 100644 --- a/runners/agent-runner/test/reviewer-runtime.test.ts +++ b/runners/agent-runner/test/reviewer-runtime.test.ts @@ -138,7 +138,7 @@ describe('claude reviewer runtime guard', () => { 'best-effort', ); - expect(sandbox.failIfUnavailable).toBe(false); + expect(sandbox?.failIfUnavailable).toBe(false); }); it('keeps non-linux reviewers in best-effort sandbox mode', () => { @@ -152,7 +152,7 @@ describe('claude reviewer runtime guard', () => { 'best-effort', ); - expect(sandbox.failIfUnavailable).toBe(false); + expect(sandbox?.failIfUnavailable).toBe(false); }); it('flags mutating shell commands', () => { From 28b0f38d99027b8f14e47b4d64ad4689f0cee09f Mon Sep 17 00:00:00 2001 From: ejclaw Date: Wed, 22 Apr 2026 20:57:59 +0900 Subject: [PATCH 006/285] test: cast readonly sandbox expectation shape --- runners/agent-runner/test/reviewer-runtime.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/runners/agent-runner/test/reviewer-runtime.test.ts b/runners/agent-runner/test/reviewer-runtime.test.ts index 4d002df..edb67ce 100644 --- a/runners/agent-runner/test/reviewer-runtime.test.ts +++ b/runners/agent-runner/test/reviewer-runtime.test.ts @@ -136,9 +136,9 @@ describe('claude reviewer runtime guard', () => { ['/repo/work'], 'linux', 'best-effort', - ); + ) as { failIfUnavailable?: boolean }; - expect(sandbox?.failIfUnavailable).toBe(false); + expect(sandbox.failIfUnavailable).toBe(false); }); it('keeps non-linux reviewers in best-effort sandbox mode', () => { @@ -150,9 +150,9 @@ describe('claude reviewer runtime guard', () => { ['/repo/work'], 'darwin', 'best-effort', - ); + ) as { failIfUnavailable?: boolean }; - expect(sandbox?.failIfUnavailable).toBe(false); + expect(sandbox.failIfUnavailable).toBe(false); }); it('flags mutating shell commands', () => { From e99145c86e88fffb0c4159219a9acd37e0e3d077 Mon Sep 17 00:00:00 2001 From: ejclaw Date: Wed, 22 Apr 2026 20:59:10 +0900 Subject: [PATCH 007/285] test: cast readonly sandbox expectation through unknown --- runners/agent-runner/test/reviewer-runtime.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/runners/agent-runner/test/reviewer-runtime.test.ts b/runners/agent-runner/test/reviewer-runtime.test.ts index edb67ce..4ff8a25 100644 --- a/runners/agent-runner/test/reviewer-runtime.test.ts +++ b/runners/agent-runner/test/reviewer-runtime.test.ts @@ -136,7 +136,7 @@ describe('claude reviewer runtime guard', () => { ['/repo/work'], 'linux', 'best-effort', - ) as { failIfUnavailable?: boolean }; + ) as unknown as { failIfUnavailable?: boolean }; expect(sandbox.failIfUnavailable).toBe(false); }); @@ -150,7 +150,7 @@ describe('claude reviewer runtime guard', () => { ['/repo/work'], 'darwin', 'best-effort', - ) as { failIfUnavailable?: boolean }; + ) as unknown as { failIfUnavailable?: boolean }; expect(sandbox.failIfUnavailable).toBe(false); }); From 87b12043239257cdc773b91bb449f970bc1b107e Mon Sep 17 00:00:00 2001 From: ejclaw Date: Thu, 23 Apr 2026 13:06:26 +0900 Subject: [PATCH 008/285] =?UTF-8?q?Phase=200=20=E2=80=94=20STEP=5FDONE/TAS?= =?UTF-8?q?K=5FDONE=20split=20+=20owner-follow-up=20integration=20smoke?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- prompts/arbiter-paired-room.md | 1 + prompts/claude-paired-room.md | 6 +- prompts/owner-common-paired-room.md | 9 +- src/message-agent-executor-paired.ts | 6 + src/message-agent-executor-rules.test.ts | 12 + src/message-agent-executor-rules.ts | 6 + src/message-runtime-flow.ts | 4 + src/message-runtime-follow-up.test.ts | 29 +++ src/message-runtime-follow-up.ts | 71 ++++-- src/message-runtime-prompts.ts | 5 +- src/message-runtime-queue.ts | 7 + src/message-runtime-rules.test.ts | 35 +++ src/message-runtime-rules.ts | 13 +- src/message-runtime.test.ts | 238 +++++++++++++++++++- src/paired-execution-context-owner.ts | 40 +++- src/paired-execution-context-shared.test.ts | 42 ++++ src/paired-execution-context-shared.ts | 22 ++ src/paired-execution-context.test.ts | 59 +++++ 18 files changed, 574 insertions(+), 31 deletions(-) diff --git a/prompts/arbiter-paired-room.md b/prompts/arbiter-paired-room.md index 9811ddc..43a2f71 100644 --- a/prompts/arbiter-paired-room.md +++ b/prompts/arbiter-paired-room.md @@ -34,6 +34,7 @@ You may receive reference opinions from external models appended to your prompt. ## Rules - Base your verdict on evidence (code, test output, logs), not on who said what first +- When reading owner/reviewer summaries, treat **TASK_DONE** as full task completion, **STEP_DONE** as intermediate progress that should keep the owner flow alive, and **DONE** as a legacy alias for **TASK_DONE** - Distinguish reviewer snapshot limits from real product bugs. Reviewer workspaces may intentionally omit heavy artifacts like `node_modules`, `dist`, and `build`; inability to run direct local test/typecheck/build/lint there is not, by itself, a blocker if dedicated verification evidence exists - When verification evidence exists from the dedicated verification path, judge that evidence on its merits instead of requiring the reviewer to reproduce the same result from the lightweight reviewer snapshot - Your verdict is final for this deadlock cycle — after it, work resumes normally diff --git a/prompts/claude-paired-room.md b/prompts/claude-paired-room.md index 1d422fd..96d2cae 100644 --- a/prompts/claude-paired-room.md +++ b/prompts/claude-paired-room.md @@ -22,9 +22,11 @@ If you see a materially better design, debugging path, or scoping choice, propos ## Completion status -**Start your first line** with one of these four statuses. This is required. +**Start your first line** with one of these six statuses. This is required. -- **DONE** — Approved. The owner's response is correct and complete. Include the evidence +- **STEP_DONE** — The current step is acceptable, but the original requested task still has remaining work. Send the task back to the owner without escalating to the arbiter +- **TASK_DONE** — Approved. The owner's work satisfies the full requested task. Include the evidence +- **DONE** — Legacy alias for **TASK_DONE**. Prefer **TASK_DONE** for new turns - **DONE_WITH_CONCERNS** — Approved with concerns. List specific actions the owner must take. If the same concerns repeat for 2+ turns, escalate to BLOCKED - **BLOCKED** — Cannot proceed without user decision - **NEEDS_CONTEXT** — Missing information from user diff --git a/prompts/owner-common-paired-room.md b/prompts/owner-common-paired-room.md index 0368cb3..50a9365 100644 --- a/prompts/owner-common-paired-room.md +++ b/prompts/owner-common-paired-room.md @@ -18,16 +18,19 @@ Challenge the reviewer's reasoning. Point out logical gaps, over-engineering, sc ## Completion status -**Start your first line** with one of these four statuses. This is required. +**Start your first line** with one of these six statuses. This is required. -- **DONE** — All steps completed. Include the evidence (test output, build log, diff) +- **STEP_DONE** — A meaningful intermediate step is complete, but the original task still has remaining work. This keeps the task active and continues the owner flow without reviewer or arbiter intervention +- **TASK_DONE** — The original requested task is complete. Include the evidence (test output, build log, diff) +- **DONE** — Legacy alias for **TASK_DONE**. Prefer **TASK_DONE** for new turns - **DONE_WITH_CONCERNS** — Completed, but there are issues worth flagging. If the reviewer raises the same concerns again, fix them or escalate to BLOCKED - **BLOCKED** — Cannot proceed. State what is stopping you - **NEEDS_CONTEXT** — Missing information needed to continue ### Finalize semantics -- When the reviewer already approved and you are finalizing, **DONE** closes the paired turn +- When the reviewer already approved and you are finalizing, **TASK_DONE** closes the paired turn +- In that same finalize step, **STEP_DONE** keeps the task active and resumes the owner flow because the original request still has remaining work - In that same finalize step, **DONE_WITH_CONCERNS** does not close the turn — it intentionally reopens review - Use **DONE_WITH_CONCERNS** on finalize only when you are explicitly asking the reviewer loop to resume diff --git a/src/message-agent-executor-paired.ts b/src/message-agent-executor-paired.ts index 349bea2..7cbf0ea 100644 --- a/src/message-agent-executor-paired.ts +++ b/src/message-agent-executor-paired.ts @@ -14,6 +14,7 @@ import { completePairedExecutionContext, type PreparedPairedExecutionContext, } from './paired-execution-context.js'; +import { parseVisibleVerdict } from './paired-execution-context-shared.js'; import { resolvePairedFollowUpQueueAction } from './message-agent-executor-rules.js'; import { enqueuePairedFollowUpAfterEvent } from './message-runtime-follow-up.js'; import type { PairedTurnIdentity } from './paired-turn-identity.js'; @@ -438,6 +439,7 @@ export function createPairedExecutionLifecycle(args: { executionStatus: effectiveStatus, sawOutput: sawOutputForFollowUp, taskStatus: finishedTask?.status ?? null, + outputSummary: pairedExecutionSummary, }); if (queueAction !== 'pending' || !finishedTask) { return; @@ -452,6 +454,10 @@ export function createPairedExecutionLifecycle(args: { executionStatus: effectiveStatus, sawOutput: sawOutputForFollowUp, fallbackLastTurnOutputRole: sawOutputForFollowUp ? completedRole : null, + fallbackLastTurnOutputVerdict: + sawOutputForFollowUp && pairedExecutionSummary + ? parseVisibleVerdict(pairedExecutionSummary) + : null, enqueueMessageCheck, }); if (followUpResult.kind !== 'paired-follow-up') { diff --git a/src/message-agent-executor-rules.test.ts b/src/message-agent-executor-rules.test.ts index 74583ad..6fe983f 100644 --- a/src/message-agent-executor-rules.test.ts +++ b/src/message-agent-executor-rules.test.ts @@ -216,6 +216,18 @@ describe('message-agent-executor-rules', () => { ).toBe('none'); }); + it('does not request a duplicate executor-side follow-up after successful owner STEP_DONE output', () => { + expect( + resolvePairedFollowUpQueueAction({ + completedRole: 'owner', + executionStatus: 'succeeded', + sawOutput: true, + taskStatus: 'active', + outputSummary: 'STEP_DONE\nkeep going', + }), + ).toBe('none'); + }); + it('returns none after successful output when no next-turn action is needed', () => { expect( resolvePairedFollowUpQueueAction({ diff --git a/src/message-agent-executor-rules.ts b/src/message-agent-executor-rules.ts index 4fab503..134d6f5 100644 --- a/src/message-agent-executor-rules.ts +++ b/src/message-agent-executor-rules.ts @@ -2,6 +2,7 @@ import { resolveFollowUpDispatch, resolveNextTurnAction, } from './message-runtime-rules.js'; +import { parseVisibleVerdict } from './paired-execution-context-shared.js'; import type { PairedRoomRole, PairedTaskStatus } from './types.js'; export { isRetryableClaudeSessionFailureAttempt, @@ -20,10 +21,15 @@ export function resolvePairedFollowUpQueueAction(args: { executionStatus: 'succeeded' | 'failed'; sawOutput: boolean; taskStatus: PairedTaskStatus | null; + outputSummary?: string | null; }): PairedFollowUpQueueAction { const nextTurnAction = resolveNextTurnAction({ taskStatus: args.taskStatus, lastTurnOutputRole: args.sawOutput ? args.completedRole : null, + lastTurnOutputVerdict: + args.sawOutput && args.outputSummary + ? parseVisibleVerdict(args.outputSummary) + : null, }); const dispatch = resolveFollowUpDispatch({ source: 'executor-recovery', diff --git a/src/message-runtime-flow.ts b/src/message-runtime-flow.ts index 8058d63..5d4f46d 100644 --- a/src/message-runtime-flow.ts +++ b/src/message-runtime-flow.ts @@ -29,6 +29,7 @@ import { type ScheduledPairedFollowUpIntentKind, } from './paired-follow-up-scheduler.js'; import { hasReviewerLease } from './service-routing.js'; +import { parseVisibleVerdict } from './paired-execution-context-shared.js'; import type { Channel, NewMessage, @@ -127,6 +128,9 @@ export function buildPendingPairedTurn(args: { const nextTurnAction = resolveNextTurnAction({ taskStatus, lastTurnOutputRole: lastTurnOutput?.role ?? null, + lastTurnOutputVerdict: lastTurnOutput?.output_text + ? parseVisibleVerdict(lastTurnOutput.output_text) + : null, }); const recentMessages = getRecentChatMessages(chatJid, 20); const lastHumanMessage = getLastHumanMessageContent(chatJid); diff --git a/src/message-runtime-follow-up.test.ts b/src/message-runtime-follow-up.test.ts index 8eae6f7..4bf4dfc 100644 --- a/src/message-runtime-follow-up.test.ts +++ b/src/message-runtime-follow-up.test.ts @@ -83,6 +83,35 @@ describe('message-runtime-follow-up', () => { expect(enqueue).toHaveBeenCalledTimes(1); }); + it('uses the fallback STEP_DONE verdict to schedule owner follow-ups when the owner keeps the task active', () => { + const enqueue = vi.fn(); + const result = dispatchPairedFollowUpForEvent({ + chatJid: 'group@test', + runId: 'run-owner-step-done', + task: { + id: 'task-owner-step-done', + status: 'active', + round_trip_count: 1, + updated_at: '2026-03-30T00:00:00.000Z', + }, + source: 'owner-delivery-success', + completedRole: 'owner', + fallbackLastTurnOutputRole: 'owner', + fallbackLastTurnOutputVerdict: 'step_done', + enqueue, + }); + + expect(result).toMatchObject({ + kind: 'paired-follow-up', + intentKind: 'owner-follow-up', + scheduled: true, + taskStatus: 'active', + lastTurnOutputRole: 'owner', + lastTurnOutputVerdict: 'step_done', + }); + expect(enqueue).toHaveBeenCalledTimes(1); + }); + it('prefers the latest persisted turn output over the fallback delivery role when both are present', () => { const enqueue = vi.fn(); vi.mocked(getPairedTurnOutputs).mockReturnValue([ diff --git a/src/message-runtime-follow-up.ts b/src/message-runtime-follow-up.ts index e774021..97725eb 100644 --- a/src/message-runtime-follow-up.ts +++ b/src/message-runtime-follow-up.ts @@ -1,4 +1,8 @@ import { getPairedTurnOutputs } from './db.js'; +import { + parseVisibleVerdict, + type VisibleVerdict, +} from './paired-execution-context-shared.js'; import { matchesExpectedPairedFollowUpIntent, resolveFollowUpDispatch, @@ -20,6 +24,7 @@ export interface PairedFollowUpDecision { taskId: string | null; taskStatus: PairedTaskStatus | null; lastTurnOutputRole: PairedRoomRole | null; + lastTurnOutputVerdict: VisibleVerdict | null; nextTurnAction: NextTurnAction; dispatch: FollowUpDispatch; } @@ -37,15 +42,23 @@ export type PairedFollowUpDispatchResult = scheduled: boolean; }); -export function resolveLatestPairedTurnOutputRole(args: { +export function resolveLatestPairedTurnOutputContext(args: { task: Pick | null | undefined; fallbackLastTurnOutputRole?: PairedRoomRole | null; -}): PairedRoomRole | null { - return ( - (args.task ? getPairedTurnOutputs(args.task.id).at(-1)?.role : null) ?? - args.fallbackLastTurnOutputRole ?? - null - ); + fallbackLastTurnOutputVerdict?: VisibleVerdict | null; +}): { + role: PairedRoomRole | null; + verdict: VisibleVerdict | null; +} { + const latestOutput = args.task + ? getPairedTurnOutputs(args.task.id).at(-1) + : null; + return { + role: latestOutput?.role ?? args.fallbackLastTurnOutputRole ?? null, + verdict: latestOutput?.output_text + ? parseVisibleVerdict(latestOutput.output_text) + : (args.fallbackLastTurnOutputVerdict ?? null), + }; } export function resolvePairedFollowUpDecision(args: { @@ -55,14 +68,18 @@ export function resolvePairedFollowUpDecision(args: { executionStatus?: 'succeeded' | 'failed'; sawOutput?: boolean; fallbackLastTurnOutputRole?: PairedRoomRole | null; + fallbackLastTurnOutputVerdict?: VisibleVerdict | null; }): PairedFollowUpDecision { - const lastTurnOutputRole = resolveLatestPairedTurnOutputRole({ - task: args.task, - fallbackLastTurnOutputRole: args.fallbackLastTurnOutputRole, - }); + const { role: lastTurnOutputRole, verdict: lastTurnOutputVerdict } = + resolveLatestPairedTurnOutputContext({ + task: args.task, + fallbackLastTurnOutputRole: args.fallbackLastTurnOutputRole, + fallbackLastTurnOutputVerdict: args.fallbackLastTurnOutputVerdict, + }); const nextTurnAction = resolveNextTurnAction({ taskStatus: args.task?.status ?? null, lastTurnOutputRole, + lastTurnOutputVerdict, }); const dispatch = resolveFollowUpDispatch({ source: args.source, @@ -76,6 +93,7 @@ export function resolvePairedFollowUpDecision(args: { taskId: args.task?.id ?? null, taskStatus: args.task?.status ?? null, lastTurnOutputRole, + lastTurnOutputVerdict, nextTurnAction, dispatch, }; @@ -91,23 +109,31 @@ export function schedulePairedFollowUpIntent(args: { intentKind: ScheduledPairedFollowUpIntentKind; enqueue: () => void; fallbackLastTurnOutputRole?: PairedRoomRole | null; + fallbackLastTurnOutputVerdict?: VisibleVerdict | null; lastTurnOutputRole?: PairedRoomRole | null; + lastTurnOutputVerdict?: VisibleVerdict | null; }): boolean { if (!args.task) { return false; } - const lastTurnOutputRole = - args.lastTurnOutputRole ?? - resolveLatestPairedTurnOutputRole({ - task: args.task, - fallbackLastTurnOutputRole: args.fallbackLastTurnOutputRole, - }); + const latestOutputContext = + args.lastTurnOutputRole != null || args.lastTurnOutputVerdict != null + ? { + role: args.lastTurnOutputRole ?? null, + verdict: args.lastTurnOutputVerdict ?? null, + } + : resolveLatestPairedTurnOutputContext({ + task: args.task, + fallbackLastTurnOutputRole: args.fallbackLastTurnOutputRole, + fallbackLastTurnOutputVerdict: args.fallbackLastTurnOutputVerdict, + }); if ( !matchesExpectedPairedFollowUpIntent({ taskStatus: args.task.status, - lastTurnOutputRole, + lastTurnOutputRole: latestOutputContext.role, + lastTurnOutputVerdict: latestOutputContext.verdict, intentKind: args.intentKind, }) ) { @@ -133,7 +159,9 @@ export function schedulePairedFollowUpWithMessageCheck(args: { intentKind: ScheduledPairedFollowUpIntentKind; enqueueMessageCheck: () => void; fallbackLastTurnOutputRole?: PairedRoomRole | null; + fallbackLastTurnOutputVerdict?: VisibleVerdict | null; lastTurnOutputRole?: PairedRoomRole | null; + lastTurnOutputVerdict?: VisibleVerdict | null; }): boolean { return schedulePairedFollowUpIntent({ chatJid: args.chatJid, @@ -142,7 +170,9 @@ export function schedulePairedFollowUpWithMessageCheck(args: { intentKind: args.intentKind, enqueue: args.enqueueMessageCheck, fallbackLastTurnOutputRole: args.fallbackLastTurnOutputRole, + fallbackLastTurnOutputVerdict: args.fallbackLastTurnOutputVerdict, lastTurnOutputRole: args.lastTurnOutputRole, + lastTurnOutputVerdict: args.lastTurnOutputVerdict, }); } @@ -158,6 +188,7 @@ export function dispatchPairedFollowUpForEvent(args: { executionStatus?: 'succeeded' | 'failed'; sawOutput?: boolean; fallbackLastTurnOutputRole?: PairedRoomRole | null; + fallbackLastTurnOutputVerdict?: VisibleVerdict | null; enqueue: () => void; enqueueMessageCheck?: () => void; }): PairedFollowUpDispatchResult { @@ -168,6 +199,7 @@ export function dispatchPairedFollowUpForEvent(args: { executionStatus: args.executionStatus, sawOutput: args.sawOutput, fallbackLastTurnOutputRole: args.fallbackLastTurnOutputRole, + fallbackLastTurnOutputVerdict: args.fallbackLastTurnOutputVerdict, }); if ( @@ -193,6 +225,7 @@ export function dispatchPairedFollowUpForEvent(args: { intentKind: decision.nextTurnAction.kind, enqueue: args.enqueue, lastTurnOutputRole: decision.lastTurnOutputRole, + lastTurnOutputVerdict: decision.lastTurnOutputVerdict, }); return { @@ -221,6 +254,7 @@ export function enqueuePairedFollowUpAfterEvent(args: { executionStatus?: 'succeeded' | 'failed'; sawOutput?: boolean; fallbackLastTurnOutputRole?: PairedRoomRole | null; + fallbackLastTurnOutputVerdict?: VisibleVerdict | null; enqueueMessageCheck: () => void; }): PairedFollowUpDispatchResult { return dispatchPairedFollowUpForEvent({ @@ -232,6 +266,7 @@ export function enqueuePairedFollowUpAfterEvent(args: { executionStatus: args.executionStatus, sawOutput: args.sawOutput, fallbackLastTurnOutputRole: args.fallbackLastTurnOutputRole, + fallbackLastTurnOutputVerdict: args.fallbackLastTurnOutputVerdict, enqueue: args.enqueueMessageCheck, enqueueMessageCheck: args.enqueueMessageCheck, }); diff --git a/src/message-runtime-prompts.ts b/src/message-runtime-prompts.ts index 99a6380..263e3c0 100644 --- a/src/message-runtime-prompts.ts +++ b/src/message-runtime-prompts.ts @@ -212,7 +212,8 @@ export function buildFinalizePendingPrompt(args: { ? `\n\nReviewer's final assessment:\n${lastReviewerOutput.output_text.slice(0, 2000)}` : ''; - return `The reviewer approved your work (DONE). Finalize and report the result. -If you intend to close this paired turn now, your first line must be DONE. + return `The reviewer approved the current task scope (TASK_DONE / legacy DONE). Finalize and report the result. +If you intend to close this paired turn now, your first line must be TASK_DONE. +If the original request still has remaining work and the owner flow should continue, your first line may be STEP_DONE. If your first line is DONE_WITH_CONCERNS, the system will reopen review instead of finishing.${reviewerSummary}`; } diff --git a/src/message-runtime-queue.ts b/src/message-runtime-queue.ts index 698b444..1ceaee8 100644 --- a/src/message-runtime-queue.ts +++ b/src/message-runtime-queue.ts @@ -11,6 +11,7 @@ import { isBotOnlyPairedRoomTurn, } from './message-runtime-flow.js'; import { buildPairedTurnIdentity } from './paired-turn-identity.js'; +import { parseVisibleVerdict } from './paired-execution-context-shared.js'; import { advanceLastAgentCursor, resolveActiveRole, @@ -183,6 +184,11 @@ export async function runQueuedGroupTurn(args: { const lastTurnOutputRole = currentTask ? (turnOutputs.at(-1)?.role ?? null) : null; + const lastTurnOutputVerdict = currentTask + ? turnOutputs.at(-1)?.output_text + ? parseVisibleVerdict(turnOutputs.at(-1)?.output_text) + : null + : null; const turnRole = currentTask ? hasHumanMsg ? resolveQueuedTurnRole({ @@ -193,6 +199,7 @@ export async function runQueuedGroupTurn(args: { taskStatus, hasHumanMessage: false, lastTurnOutputRole, + lastTurnOutputVerdict, }) : 'owner'; if (!turnRole) { diff --git a/src/message-runtime-rules.test.ts b/src/message-runtime-rules.test.ts index 7c78c6b..db9b4bd 100644 --- a/src/message-runtime-rules.test.ts +++ b/src/message-runtime-rules.test.ts @@ -113,6 +113,14 @@ describe('message-runtime-rules', () => { intentKind: 'owner-follow-up', }), ).toBe(true); + expect( + matchesExpectedPairedFollowUpIntent({ + taskStatus: 'active', + lastTurnOutputRole: 'owner', + lastTurnOutputVerdict: 'step_done', + intentKind: 'owner-follow-up', + }), + ).toBe(true); }); it('maps active tasks with reviewer output to an owner follow-up', () => { @@ -124,6 +132,16 @@ describe('message-runtime-rules', () => { ).toEqual({ kind: 'owner-follow-up' }); }); + it('maps active tasks with owner STEP_DONE output to an owner follow-up', () => { + expect( + resolveNextTurnAction({ + taskStatus: 'active', + lastTurnOutputRole: 'owner', + lastTurnOutputVerdict: 'step_done', + }), + ).toEqual({ kind: 'owner-follow-up' }); + }); + it('returns none when an active task has no reviewer or arbiter handoff output', () => { expect( resolveNextTurnAction({ @@ -152,6 +170,15 @@ describe('message-runtime-rules', () => { kind: 'enqueue', queueKind: 'paired-follow-up', }); + expect( + resolveFollowUpDispatch({ + source: 'owner-delivery-success', + nextTurnAction: { kind: 'owner-follow-up' }, + }), + ).toEqual({ + kind: 'enqueue', + queueKind: 'paired-follow-up', + }); expect( resolveFollowUpDispatch({ source: 'owner-delivery-success', @@ -295,6 +322,14 @@ describe('message-runtime-rules', () => { lastTurnOutputRole: 'owner', }), ).toBe('reviewer'); + expect( + resolveQueuedPairedTurnRole({ + taskStatus: 'active', + hasHumanMessage: false, + lastTurnOutputRole: 'owner', + lastTurnOutputVerdict: 'step_done', + }), + ).toBe('owner'); }); it('resolves reviewer execution target from review_ready task status', () => { diff --git a/src/message-runtime-rules.ts b/src/message-runtime-rules.ts index 7d24a07..033abb1 100644 --- a/src/message-runtime-rules.ts +++ b/src/message-runtime-rules.ts @@ -4,6 +4,7 @@ import { normalizeStoredSeqCursor } from './message-cursor.js'; import { isTriggerAllowed, loadSenderAllowlist } from './sender-allowlist.js'; import { isTaskStatusControlMessage } from './task-watch-status.js'; import { ARBITER_AGENT_TYPE, REVIEWER_AGENT_TYPE } from './config.js'; +import type { VisibleVerdict } from './paired-execution-context-shared.js'; import { hasReviewerLease, resolveLeaseServiceId, @@ -79,6 +80,7 @@ export function resolveQueuedPairedTurnRole(args: { taskStatus?: PairedTaskStatus | null; hasHumanMessage: boolean; lastTurnOutputRole?: PairedRoomRole | null; + lastTurnOutputVerdict?: VisibleVerdict | null; }): 'owner' | 'reviewer' | 'arbiter' | null { if (args.hasHumanMessage) { return resolveQueuedTurnRole({ @@ -90,6 +92,7 @@ export function resolveQueuedPairedTurnRole(args: { const nextTurnAction = resolveNextTurnAction({ taskStatus: args.taskStatus, lastTurnOutputRole: args.lastTurnOutputRole, + lastTurnOutputVerdict: args.lastTurnOutputVerdict, }); switch (nextTurnAction.kind) { @@ -125,6 +128,7 @@ export type FollowUpDispatch = export function resolveNextTurnAction(args: { taskStatus?: PairedTaskStatus | null; lastTurnOutputRole?: PairedRoomRole | null; + lastTurnOutputVerdict?: VisibleVerdict | null; }): NextTurnAction { switch (args.taskStatus) { case 'review_ready': @@ -142,7 +146,8 @@ export function resolveNextTurnAction(args: { ? { kind: 'none' } : { kind: 'finalize-owner-turn' }; case 'active': - return args.lastTurnOutputRole === 'reviewer' || + return args.lastTurnOutputVerdict === 'step_done' || + args.lastTurnOutputRole === 'reviewer' || args.lastTurnOutputRole === 'arbiter' ? { kind: 'owner-follow-up' } : { kind: 'none' }; @@ -154,12 +159,14 @@ export function resolveNextTurnAction(args: { export function matchesExpectedPairedFollowUpIntent(args: { taskStatus?: PairedTaskStatus | null; lastTurnOutputRole?: PairedRoomRole | null; + lastTurnOutputVerdict?: VisibleVerdict | null; intentKind: ScheduledNextTurnActionKind; }): boolean { return ( resolveNextTurnAction({ taskStatus: args.taskStatus, lastTurnOutputRole: args.lastTurnOutputRole, + lastTurnOutputVerdict: args.lastTurnOutputVerdict, }).kind === args.intentKind ); } @@ -184,7 +191,8 @@ export function resolveFollowUpDispatch(args: { args.completedRole === 'owner' ) { return args.nextTurnAction.kind === 'reviewer-turn' || - args.nextTurnAction.kind === 'arbiter-turn' + args.nextTurnAction.kind === 'arbiter-turn' || + args.nextTurnAction.kind === 'owner-follow-up' ? { kind: 'enqueue', queueKind: 'paired-follow-up' } : { kind: 'none' }; } @@ -219,6 +227,7 @@ export function resolveFollowUpDispatch(args: { return { kind: 'none' }; } return args.nextTurnAction.kind === 'reviewer-turn' || + args.nextTurnAction.kind === 'owner-follow-up' || args.nextTurnAction.kind === 'arbiter-turn' || args.nextTurnAction.kind === 'finalize-owner-turn' ? { kind: 'enqueue', queueKind: 'paired-follow-up' } diff --git a/src/message-runtime.test.ts b/src/message-runtime.test.ts index e9c6d68..b5d82df 100644 --- a/src/message-runtime.test.ts +++ b/src/message-runtime.test.ts @@ -1632,8 +1632,9 @@ describe('createMessageRuntime', () => { vi.mocked(agentRunner.runAgentProcess).mockImplementation( async (_group, input, _onProcess, onOutput) => { expect(input.prompt).toBe( - `The reviewer approved your work (DONE). Finalize and report the result. -If you intend to close this paired turn now, your first line must be DONE. + `The reviewer approved the current task scope (TASK_DONE / legacy DONE). Finalize and report the result. +If you intend to close this paired turn now, your first line must be TASK_DONE. +If the original request still has remaining work and the owner flow should continue, your first line may be STEP_DONE. If your first line is DONE_WITH_CONCERNS, the system will reopen review instead of finishing.\n\nReviewer's final assessment:\n${truncatedReviewerOutput}`, ); expect(input.prompt).not.toContain(longReviewerOutput); @@ -1741,7 +1742,7 @@ If your first line is DONE_WITH_CONCERNS, the system will reopen review instead vi.mocked(agentRunner.runAgentProcess).mockImplementation( async (_group, input, _onProcess, onOutput) => { expect(input.prompt).toBe( - "The reviewer approved your work (DONE). Finalize and report the result.\nIf you intend to close this paired turn now, your first line must be DONE.\nIf your first line is DONE_WITH_CONCERNS, the system will reopen review instead of finishing.\n\nReviewer's final assessment:\n리뷰 승인 요약", + "The reviewer approved the current task scope (TASK_DONE / legacy DONE). Finalize and report the result.\nIf you intend to close this paired turn now, your first line must be TASK_DONE.\nIf the original request still has remaining work and the owner flow should continue, your first line may be STEP_DONE.\nIf your first line is DONE_WITH_CONCERNS, the system will reopen review instead of finishing.\n\nReviewer's final assessment:\n리뷰 승인 요약", ); expect(input.prompt).not.toContain('DONE\n승인합니다.'); await onOutput?.({ @@ -2004,6 +2005,110 @@ If your first line is DONE_WITH_CONCERNS, the system will reopen review instead expect(pending?.prompt).toContain('이전 reviewer 피드백'); }); + it('builds an owner follow-up pending turn when the latest owner output is STEP_DONE on an active task', () => { + const chatJid = 'group@test'; + const group = makeGroup('claude-code'); + const task = { + id: 'task-owner-step-done-pending', + chat_jid: chatJid, + group_folder: group.folder, + owner_service_id: 'claude', + reviewer_service_id: 'codex-main', + title: null, + source_ref: 'HEAD', + plan_notes: null, + review_requested_at: null, + round_trip_count: 1, + status: 'active', + arbiter_verdict: null, + arbiter_requested_at: null, + completion_reason: null, + created_at: '2026-03-30T00:00:10.000Z', + updated_at: '2026-03-30T00:00:10.000Z', + } as any; + + vi.mocked(db.getLatestPreviousPairedTaskForChat).mockReturnValue(undefined); + vi.mocked(db.getPairedTurnOutputs).mockReturnValue([ + { + id: 1, + task_id: task.id, + turn_number: 1, + role: 'owner', + output_text: 'STEP_DONE\n1단계는 끝났고 2단계로 이어가야 함', + created_at: '2026-03-30T00:00:11.000Z', + }, + ] as any); + + const pending = buildPendingPairedTurn({ + chatJid, + timezone: 'UTC', + task, + rawMissedMessages: [{ seq: 42, timestamp: '2026-03-30T00:00:12.000Z' }], + recentHumanMessages: [], + labeledRecentMessages: [], + resolveChannel: () => makeChannel(chatJid), + }); + + expect(pending).not.toBeNull(); + expect(pending).toMatchObject({ + role: 'owner', + intentKind: 'owner-follow-up', + taskId: task.id, + }); + }); + + it('builds a reviewer pending turn when a review_ready task follows owner TASK_DONE output', () => { + const chatJid = 'group@test'; + const group = makeGroup('claude-code'); + const task = { + id: 'task-owner-task-done-pending', + chat_jid: chatJid, + group_folder: group.folder, + owner_service_id: 'claude', + reviewer_service_id: 'codex-main', + title: null, + source_ref: 'HEAD', + plan_notes: null, + review_requested_at: '2026-03-30T00:00:10.000Z', + round_trip_count: 1, + status: 'review_ready', + arbiter_verdict: null, + arbiter_requested_at: null, + completion_reason: null, + created_at: '2026-03-30T00:00:10.000Z', + updated_at: '2026-03-30T00:00:10.000Z', + } as any; + + vi.mocked(db.getLatestPreviousPairedTaskForChat).mockReturnValue(undefined); + vi.mocked(db.getPairedTurnOutputs).mockReturnValue([ + { + id: 1, + task_id: task.id, + turn_number: 1, + role: 'owner', + output_text: 'TASK_DONE\n요청 범위 전체 완료', + created_at: '2026-03-30T00:00:11.000Z', + }, + ] as any); + + const pending = buildPendingPairedTurn({ + chatJid, + timezone: 'UTC', + task, + rawMissedMessages: [{ seq: 42, timestamp: '2026-03-30T00:00:12.000Z' }], + recentHumanMessages: [], + labeledRecentMessages: [], + resolveChannel: () => makeChannel(chatJid, 'discord-review', false), + }); + + expect(pending).not.toBeNull(); + expect(pending).toMatchObject({ + role: 'reviewer', + intentKind: 'reviewer-turn', + taskId: task.id, + }); + }); + it('re-enqueues reviewer after a successful owner delivery moves the task to review_ready', async () => { const chatJid = 'group@test'; const group = makeGroup('codex'); @@ -2109,6 +2214,133 @@ If your first line is DONE_WITH_CONCERNS, the system will reopen review instead ); }); + it('re-enqueues owner follow-up after a successful owner STEP_DONE delivery keeps the task active', async () => { + const chatJid = 'group@test'; + const group = makeGroup('codex'); + const ownerChannel: Channel = { + ...makeChannel(chatJid), + isOwnMessage: vi.fn((msg) => msg.sender === 'owner-bot@test'), + }; + const reviewerChannel = makeChannel(chatJid, 'discord-review', false); + const enqueueMessageCheck = vi.fn(); + const pairedTask = { + id: 'task-owner-step-done-follow-up', + chat_jid: chatJid, + group_folder: group.folder, + owner_service_id: 'claude', + reviewer_service_id: 'codex-main', + title: null, + source_ref: 'HEAD', + plan_notes: null, + review_requested_at: null, + round_trip_count: 1, + owner_failure_count: 2, + status: 'active', + arbiter_verdict: null, + arbiter_requested_at: null, + completion_reason: null, + created_at: '2026-03-30T00:00:00.000Z', + updated_at: '2026-03-30T00:00:00.000Z', + } as any; + let turnOutputs: any[] = []; + + vi.mocked(serviceRouting.hasReviewerLease).mockReturnValue(true); + vi.mocked(db.getLatestOpenPairedTaskForChat).mockImplementation( + () => pairedTask, + ); + vi.mocked(db.getPairedTurnOutputs).mockImplementation((taskId: string) => + taskId === pairedTask.id ? turnOutputs : [], + ); + vi.mocked(db.getMessagesSince).mockReturnValue([ + { + id: 'human-step-done-1', + chat_jid: chatJid, + sender: 'user@test', + sender_name: 'User', + content: '이 리팩토링 이어서 해줘', + timestamp: '2026-03-30T00:00:00.000Z', + seq: 1, + is_bot_message: false, + } as any, + ]); + vi.mocked(agentRunner.runAgentProcess).mockImplementation( + async (_group, _input, _onProcess, onOutput) => { + pairedTask.status = 'active'; + pairedTask.owner_failure_count = 0; + turnOutputs = [ + { + id: 1, + task_id: pairedTask.id, + turn_number: 1, + role: 'owner', + output_text: 'STEP_DONE\n리팩토링 1단계 완료, 다음 단계 진행', + created_at: '2026-03-30T00:00:01.000Z', + }, + ]; + await onOutput?.({ + status: 'success', + phase: 'final', + result: 'STEP_DONE\n리팩토링 1단계 완료, 다음 단계 진행', + newSessionId: 'session-owner-step-done-follow-up', + }); + return { + status: 'success', + result: 'STEP_DONE\n리팩토링 1단계 완료, 다음 단계 진행', + newSessionId: 'session-owner-step-done-follow-up', + }; + }, + ); + + const runtime = createMessageRuntime({ + assistantName: 'Andy', + idleTimeout: 1_000, + pollInterval: 1_000, + timezone: 'UTC', + triggerPattern: /^@Andy\b/i, + channels: [ownerChannel, reviewerChannel], + queue: { + registerProcess: vi.fn(), + closeStdin: vi.fn(), + notifyIdle: vi.fn(), + enqueueMessageCheck, + } as any, + getRoomBindings: () => ({ [chatJid]: group }), + getSessions: () => ({}), + getLastTimestamp: () => '', + setLastTimestamp: vi.fn(), + getLastAgentTimestamps: () => ({}), + saveState: vi.fn(), + persistSession: vi.fn(), + clearSession: vi.fn(), + }); + + const result = await runtime.processGroupMessages(chatJid, { + runId: 'run-owner-step-done-follow-up', + reason: 'messages', + }); + + expect(result).toBe(true); + expect(ownerChannel.sendMessage).toHaveBeenCalledWith( + chatJid, + 'STEP_DONE\n리팩토링 1단계 완료, 다음 단계 진행', + ); + expect(enqueueMessageCheck).toHaveBeenCalledWith(chatJid); + expect(pairedTask.status).toBe('active'); + expect(pairedTask.round_trip_count).toBe(1); + expect(pairedTask.owner_failure_count).toBe(0); + expect(logger.info).toHaveBeenCalledWith( + expect.objectContaining({ + chatJid, + runId: 'run-owner-step-done-follow-up', + completedRole: 'owner', + taskId: 'task-owner-step-done-follow-up', + taskStatus: 'active', + intentKind: 'owner-follow-up', + }), + 'Queued paired follow-up after successful owner delivery', + ); + }); + it('defers reviewer enqueue after owner delivery when a CI watcher is still active', async () => { const chatJid = 'group@test'; const group = makeGroup('codex'); diff --git a/src/paired-execution-context-owner.ts b/src/paired-execution-context-owner.ts index 9d2b7c4..c59ae8b 100644 --- a/src/paired-execution-context-owner.ts +++ b/src/paired-execution-context-owner.ts @@ -20,7 +20,7 @@ import { } from './paired-execution-context-shared.js'; import type { PairedTask } from './types.js'; -type OwnerFinalizeOutcome = 'stop' | 're_review'; +type OwnerFinalizeOutcome = 'stop' | 're_review' | 'continue_owner'; const OWNER_FAILURE_ESCALATION_THRESHOLD = 2; export function handleFailedOwnerExecution(args: { @@ -170,6 +170,28 @@ function handleOwnerFinalizeCompletion(args: { return 're_review'; } + if (signal.kind === 'request_owner_continue') { + transitionPairedTaskStatus({ + taskId, + currentStatus: task.status, + nextStatus: 'active', + expectedUpdatedAt: task.updated_at, + updatedAt: now, + patch: { + owner_failure_count: 0, + }, + }); + logger.info( + { + taskId, + ownerVerdict, + summary: summary?.slice(0, 100), + }, + 'Owner marked finalize output as an intermediate step — task returned to active without re-review', + ); + return 'continue_owner'; + } + transitionPairedTaskStatus({ taskId, currentStatus: task.status, @@ -290,6 +312,22 @@ export function handleOwnerCompletion(args: { return; } + if (signal.kind === 'request_owner_continue') { + applyPairedTaskPatch({ + taskId, + expectedUpdatedAt: task.updated_at, + updatedAt: now, + patch: { + owner_failure_count: 0, + }, + }); + logger.info( + { taskId, ownerVerdict, summary: summary?.slice(0, 100) }, + 'Owner marked the current output as an intermediate completed step — keeping task active for follow-up', + ); + return; + } + maybeAutoTriggerReviewerAfterOwnerCompletion({ task, taskId, diff --git a/src/paired-execution-context-shared.test.ts b/src/paired-execution-context-shared.test.ts index b19568b..3cb197a 100644 --- a/src/paired-execution-context-shared.test.ts +++ b/src/paired-execution-context-shared.test.ts @@ -9,6 +9,8 @@ import { describe('paired execution context shared verdict helpers', () => { it('parses visible verdicts from the first summary line only', () => { + expect(parseVisibleVerdict('STEP_DONE\nmore to do')).toBe('step_done'); + expect(parseVisibleVerdict('TASK_DONE\nall done')).toBe('task_done'); expect( parseVisibleVerdict( 'DONE_WITH_CONCERNS\n\nfollow-up detail that should not affect parsing', @@ -19,6 +21,15 @@ describe('paired execution context shared verdict helpers', () => { }); it('maps normal owner completion verdicts to reviewer or arbiter signals', () => { + expect( + resolveOwnerCompletionSignal({ + phase: 'normal', + visibleVerdict: 'step_done', + }), + ).toEqual({ + kind: 'request_owner_continue', + resetStatusToActive: false, + }); expect( resolveOwnerCompletionSignal({ phase: 'normal', @@ -37,6 +48,18 @@ describe('paired execution context shared verdict helpers', () => { }); it('maps finalize owner outcomes to complete, re-review, or arbiter', () => { + expect( + resolveOwnerCompletionSignal({ + phase: 'finalize', + visibleVerdict: 'step_done', + hasChangesSinceApproval: false, + roundTripCount: 0, + deadlockThreshold: 2, + }), + ).toEqual({ + kind: 'request_owner_continue', + resetStatusToActive: true, + }); expect( resolveOwnerCompletionSignal({ phase: 'finalize', @@ -70,6 +93,20 @@ describe('paired execution context shared verdict helpers', () => { }); it('maps reviewer completion verdicts to finalize, owner changes, or arbiter', () => { + expect( + resolveReviewerCompletionSignal({ + visibleVerdict: 'task_done', + roundTripCount: 0, + deadlockThreshold: 3, + }), + ).toEqual({ kind: 'request_owner_finalize' }); + expect( + resolveReviewerCompletionSignal({ + visibleVerdict: 'step_done', + roundTripCount: 0, + deadlockThreshold: 3, + }), + ).toEqual({ kind: 'request_owner_changes' }); expect( resolveReviewerCompletionSignal({ visibleVerdict: 'done', @@ -94,6 +131,11 @@ describe('paired execution context shared verdict helpers', () => { }); it('maps reviewer failure verdicts to explicit failure signals', () => { + expect( + resolveReviewerFailureSignal({ + visibleVerdict: 'task_done', + }), + ).toEqual({ kind: 'request_owner_finalize' }); expect( resolveReviewerFailureSignal({ visibleVerdict: 'done', diff --git a/src/paired-execution-context-shared.ts b/src/paired-execution-context-shared.ts index 18d3faa..8bf11fc 100644 --- a/src/paired-execution-context-shared.ts +++ b/src/paired-execution-context-shared.ts @@ -6,6 +6,8 @@ import { logger } from './logger.js'; import type { PairedTaskStatus } from './types.js'; export type VisibleVerdict = + | 'step_done' + | 'task_done' | 'done' | 'done_with_concerns' | 'blocked' @@ -15,6 +17,7 @@ export type VisibleVerdict = export type CompletionSignal = | { kind: 'request_reviewer'; resetStatusToActive: boolean } | { kind: 'request_owner_finalize' } + | { kind: 'request_owner_continue'; resetStatusToActive: boolean } | { kind: 'request_owner_changes' } | { kind: 'request_arbiter' } | { kind: 'complete'; completionReason: 'done' | 'escalated' } @@ -29,6 +32,8 @@ export function parseVisibleVerdict( const firstLine = cleaned.split('\n')[0].trim(); if (/^\*{0,2}BLOCKED\*{0,2}\b/i.test(firstLine)) return 'blocked'; if (/^\*{0,2}NEEDS_CONTEXT\*{0,2}\b/i.test(firstLine)) return 'needs_context'; + if (/^\*{0,2}STEP_DONE\*{0,2}\b/i.test(firstLine)) return 'step_done'; + if (/^\*{0,2}TASK_DONE\*{0,2}\b/i.test(firstLine)) return 'task_done'; if (/^\*{0,2}DONE_WITH_CONCERNS\*{0,2}\b/i.test(firstLine)) return 'done_with_concerns'; if (/^\*{0,2}DONE\*{0,2}\b/i.test(firstLine)) return 'done'; @@ -57,12 +62,25 @@ export function resolveOwnerCompletionSignal(args: { } if (phase === 'normal') { + if (visibleVerdict === 'step_done') { + return { + kind: 'request_owner_continue', + resetStatusToActive: false, + }; + } return { kind: 'request_reviewer', resetStatusToActive: false, }; } + if (visibleVerdict === 'step_done') { + return { + kind: 'request_owner_continue', + resetStatusToActive: true, + }; + } + const needsReReview = visibleVerdict === 'done_with_concerns' || hasChangesSinceApproval === true; @@ -90,8 +108,11 @@ export function resolveReviewerCompletionSignal(args: { const { visibleVerdict, roundTripCount, deadlockThreshold } = args; switch (visibleVerdict) { + case 'task_done': case 'done': return { kind: 'request_owner_finalize' }; + case 'step_done': + return { kind: 'request_owner_changes' }; case 'blocked': case 'needs_context': return { kind: 'request_arbiter' }; @@ -111,6 +132,7 @@ export function resolveReviewerFailureSignal(args: { const { visibleVerdict } = args; switch (visibleVerdict) { + case 'task_done': case 'done': return { kind: 'request_owner_finalize' }; case 'blocked': diff --git a/src/paired-execution-context.test.ts b/src/paired-execution-context.test.ts index bfee801..e3f29bc 100644 --- a/src/paired-execution-context.test.ts +++ b/src/paired-execution-context.test.ts @@ -742,6 +742,65 @@ describe('paired execution context', () => { ).not.toHaveBeenCalled(); }); + it('keeps an active task in owner follow-up mode when the owner reports STEP_DONE', () => { + vi.mocked(db.getPairedTaskById).mockReturnValue( + buildPairedTask({ + status: 'active', + round_trip_count: 1, + }), + ); + + completePairedExecutionContext({ + taskId: 'task-1', + role: 'owner', + status: 'succeeded', + summary: 'STEP_DONE\n1단계 완료, 후속 작업 계속', + }); + + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + owner_failure_count: 0, + }), + ); + expect( + pairedWorkspaceManager.markPairedTaskReviewReady, + ).not.toHaveBeenCalled(); + expect(db.updatePairedTask).not.toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'active', + }), + ); + }); + + it('returns merge_ready owner finalize output to active when the owner reports STEP_DONE', () => { + vi.mocked(db.getPairedTaskById).mockReturnValue( + buildPairedTask({ + status: 'merge_ready', + round_trip_count: 1, + }), + ); + + completePairedExecutionContext({ + taskId: 'task-1', + role: 'owner', + status: 'succeeded', + summary: 'STEP_DONE\n남은 범위가 있어서 계속 진행', + }); + + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'active', + owner_failure_count: 0, + }), + ); + expect( + pairedWorkspaceManager.markPairedTaskReviewReady, + ).not.toHaveBeenCalled(); + }); + it('re-triggers review once when owner reports DONE_WITH_CONCERNS during finalize', () => { vi.mocked(db.getPairedTaskById).mockReturnValue( buildPairedTask({ From ae0e1999b47082eee46b77ac92a5df568b25753c Mon Sep 17 00:00:00 2001 From: ejclaw Date: Thu, 23 Apr 2026 15:27:49 +0900 Subject: [PATCH 009/285] Add STEP_DONE guards, verdict storage, and stale delivery suppression --- src/db.test.ts | 43 ++++++ src/db/base-schema.ts | 5 + src/db/bootstrap.test.ts | 1 + .../012_paired-verdict-and-step-telemetry.ts | 75 ++++++++++ src/db/migrations/index.ts | 2 + src/db/paired-state.ts | 46 +++++- src/db/paired-turn-outputs.ts | 6 +- src/message-runtime-flow.ts | 9 +- src/message-runtime-follow-up.ts | 10 +- src/message-runtime-queue.ts | 9 +- src/message-runtime.test.ts | 121 ++++++++++++++++ src/message-runtime.ts | 91 +++++++++--- src/paired-execution-context-owner.ts | 129 ++++++++++++++++- src/paired-execution-context-shared.ts | 45 +++--- src/paired-execution-context.test.ts | 136 ++++++++++++++++++ src/paired-execution-context.ts | 55 ++++++- src/paired-verdict.ts | 41 ++++++ src/types.ts | 7 + 18 files changed, 767 insertions(+), 64 deletions(-) create mode 100644 src/db/migrations/012_paired-verdict-and-step-telemetry.ts create mode 100644 src/paired-verdict.ts diff --git a/src/db.test.ts b/src/db.test.ts index 0d687f5..0a15fc6 100644 --- a/src/db.test.ts +++ b/src/db.test.ts @@ -1242,10 +1242,53 @@ describe('paired task state', () => { expect(outputs.map((output) => output.turn_number)).toEqual([1, 2]); expect(outputs[0].role).toBe('owner'); expect(outputs[0].output_text).toHaveLength(50_000); + expect(outputs[0].verdict).toBe('continue'); expect(outputs[1].output_text).toBe('review turn'); + expect(outputs[1].verdict).toBe('continue'); expect(getLatestTurnNumber('paired-task-turn-output')).toBe(2); }); + it('stores the parsed visible verdict with paired turn outputs', () => { + createPairedTask({ + id: 'paired-task-turn-output-verdict', + chat_jid: 'dc:paired', + group_folder: 'paired-room', + owner_service_id: 'codex-main', + reviewer_service_id: 'codex-review', + title: null, + source_ref: null, + plan_notes: null, + round_trip_count: 0, + review_requested_at: null, + status: 'active', + arbiter_verdict: null, + arbiter_requested_at: null, + completion_reason: null, + created_at: '2026-03-28T00:00:00.000Z', + updated_at: '2026-03-28T00:00:00.000Z', + }); + + insertPairedTurnOutput( + 'paired-task-turn-output-verdict', + 1, + 'owner', + 'STEP_DONE\n1단계 완료', + ); + insertPairedTurnOutput( + 'paired-task-turn-output-verdict', + 2, + 'owner', + 'TASK_DONE\n요청 범위 전체 완료', + ); + + const outputs = getPairedTurnOutputs('paired-task-turn-output-verdict'); + + expect(outputs.map((output) => output.verdict)).toEqual([ + 'step_done', + 'task_done', + ]); + }); + it('preserves explicit created_at when inserting a paired turn output', () => { createPairedTask({ id: 'paired-task-turn-output-created-at', diff --git a/src/db/base-schema.ts b/src/db/base-schema.ts index 1fd66ba..d44e693 100644 --- a/src/db/base-schema.ts +++ b/src/db/base-schema.ts @@ -119,6 +119,10 @@ export function applyBaseSchema(database: Database): void { review_requested_at TEXT, round_trip_count INTEGER NOT NULL DEFAULT 0, owner_failure_count INTEGER NOT NULL DEFAULT 0, + owner_step_done_streak INTEGER NOT NULL DEFAULT 0, + finalize_step_done_count INTEGER NOT NULL DEFAULT 0, + task_done_then_user_reopen_count INTEGER NOT NULL DEFAULT 0, + empty_step_done_streak INTEGER NOT NULL DEFAULT 0, status TEXT NOT NULL DEFAULT 'active', arbiter_verdict TEXT, arbiter_requested_at TEXT, @@ -154,6 +158,7 @@ export function applyBaseSchema(database: Database): void { turn_number INTEGER NOT NULL, role TEXT NOT NULL, output_text TEXT NOT NULL, + verdict TEXT, created_at TEXT NOT NULL, UNIQUE(task_id, turn_number, role) ); diff --git a/src/db/bootstrap.test.ts b/src/db/bootstrap.test.ts index c375201..49dc122 100644 --- a/src/db/bootstrap.test.ts +++ b/src/db/bootstrap.test.ts @@ -36,6 +36,7 @@ function getExpectedSchemaMigrations(): Array<{ { version: 9, name: 'paired_workspace_project_schema_cleanup' }, { version: 10, name: 'paired_turn_provenance_upgrade' }, { version: 11, name: 'owner_failure_count' }, + { version: 12, name: 'paired_verdict_and_step_telemetry' }, ]; } diff --git a/src/db/migrations/012_paired-verdict-and-step-telemetry.ts b/src/db/migrations/012_paired-verdict-and-step-telemetry.ts new file mode 100644 index 0000000..d4f9e6e --- /dev/null +++ b/src/db/migrations/012_paired-verdict-and-step-telemetry.ts @@ -0,0 +1,75 @@ +import type { Database } from 'bun:sqlite'; + +import { tableHasColumn } from './helpers.js'; +import type { SchemaMigrationDefinition } from './types.js'; + +export const PAIRED_VERDICT_AND_STEP_TELEMETRY_MIGRATION: SchemaMigrationDefinition = + { + version: 12, + name: 'paired_verdict_and_step_telemetry', + apply(database: Database) { + if (!tableHasColumn(database, 'paired_turn_outputs', 'verdict')) { + database.exec(` + ALTER TABLE paired_turn_outputs + ADD COLUMN verdict TEXT + `); + } + + if (!tableHasColumn(database, 'paired_tasks', 'owner_step_done_streak')) { + database.exec(` + ALTER TABLE paired_tasks + ADD COLUMN owner_step_done_streak INTEGER NOT NULL DEFAULT 0 + `); + } + + if ( + !tableHasColumn(database, 'paired_tasks', 'finalize_step_done_count') + ) { + database.exec(` + ALTER TABLE paired_tasks + ADD COLUMN finalize_step_done_count INTEGER NOT NULL DEFAULT 0 + `); + } + + if ( + !tableHasColumn( + database, + 'paired_tasks', + 'task_done_then_user_reopen_count', + ) + ) { + database.exec(` + ALTER TABLE paired_tasks + ADD COLUMN task_done_then_user_reopen_count INTEGER NOT NULL DEFAULT 0 + `); + } + + if (!tableHasColumn(database, 'paired_tasks', 'empty_step_done_streak')) { + database.exec(` + ALTER TABLE paired_tasks + ADD COLUMN empty_step_done_streak INTEGER NOT NULL DEFAULT 0 + `); + } + + database.exec(` + UPDATE paired_tasks + SET owner_step_done_streak = 0 + WHERE owner_step_done_streak IS NULL + `); + database.exec(` + UPDATE paired_tasks + SET finalize_step_done_count = 0 + WHERE finalize_step_done_count IS NULL + `); + database.exec(` + UPDATE paired_tasks + SET task_done_then_user_reopen_count = 0 + WHERE task_done_then_user_reopen_count IS NULL + `); + database.exec(` + UPDATE paired_tasks + SET empty_step_done_streak = 0 + WHERE empty_step_done_streak IS NULL + `); + }, + }; diff --git a/src/db/migrations/index.ts b/src/db/migrations/index.ts index 85eaf42..d6b5e83 100644 --- a/src/db/migrations/index.ts +++ b/src/db/migrations/index.ts @@ -11,6 +11,7 @@ import { PAIRED_TASK_SCHEMA_CLEANUP_MIGRATION } from './008_paired-task-schema-c import { PAIRED_WORKSPACE_PROJECT_SCHEMA_CLEANUP_MIGRATION } from './009_paired-workspace-project-schema-cleanup.js'; import { PAIRED_TURN_PROVENANCE_UPGRADE_MIGRATION } from './010_paired-turn-provenance-upgrade.js'; import { OWNER_FAILURE_COUNT_MIGRATION } from './011_owner-failure-count.js'; +import { PAIRED_VERDICT_AND_STEP_TELEMETRY_MIGRATION } from './012_paired-verdict-and-step-telemetry.js'; import type { SchemaMigrationArgs, SchemaMigrationDefinition, @@ -30,6 +31,7 @@ const ORDERED_SCHEMA_MIGRATIONS: readonly SchemaMigrationDefinition[] = [ PAIRED_WORKSPACE_PROJECT_SCHEMA_CLEANUP_MIGRATION, PAIRED_TURN_PROVENANCE_UPGRADE_MIGRATION, OWNER_FAILURE_COUNT_MIGRATION, + PAIRED_VERDICT_AND_STEP_TELEMETRY_MIGRATION, ]; function ensureSchemaMigrationsTable(database: Database): void { diff --git a/src/db/paired-state.ts b/src/db/paired-state.ts index cb589bd..0ebbb20 100644 --- a/src/db/paired-state.ts +++ b/src/db/paired-state.ts @@ -51,6 +51,10 @@ export type PairedTaskUpdates = Partial< | 'review_requested_at' | 'round_trip_count' | 'owner_failure_count' + | 'owner_step_done_streak' + | 'finalize_step_done_count' + | 'task_done_then_user_reopen_count' + | 'empty_step_done_streak' | 'status' | 'arbiter_verdict' | 'arbiter_requested_at' @@ -173,6 +177,10 @@ export function createPairedTaskInDatabase( review_requested_at, round_trip_count, owner_failure_count, + owner_step_done_streak, + finalize_step_done_count, + task_done_then_user_reopen_count, + empty_step_done_streak, status, arbiter_verdict, arbiter_requested_at, @@ -180,7 +188,7 @@ export function createPairedTaskInDatabase( created_at, updated_at ) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) `, ) .run( @@ -198,6 +206,10 @@ export function createPairedTaskInDatabase( task.review_requested_at, task.round_trip_count, task.owner_failure_count ?? 0, + task.owner_step_done_streak ?? 0, + task.finalize_step_done_count ?? 0, + task.task_done_then_user_reopen_count ?? 0, + task.empty_step_done_streak ?? 0, task.status, task.arbiter_verdict, task.arbiter_requested_at, @@ -306,6 +318,22 @@ export function updatePairedTaskInDatabase( fields.push('owner_failure_count = ?'); values.push(updates.owner_failure_count); } + if (updates.owner_step_done_streak !== undefined) { + fields.push('owner_step_done_streak = ?'); + values.push(updates.owner_step_done_streak); + } + if (updates.finalize_step_done_count !== undefined) { + fields.push('finalize_step_done_count = ?'); + values.push(updates.finalize_step_done_count); + } + if (updates.task_done_then_user_reopen_count !== undefined) { + fields.push('task_done_then_user_reopen_count = ?'); + values.push(updates.task_done_then_user_reopen_count); + } + if (updates.empty_step_done_streak !== undefined) { + fields.push('empty_step_done_streak = ?'); + values.push(updates.empty_step_done_streak); + } if (updates.status !== undefined) { fields.push('status = ?'); values.push(updates.status); @@ -369,6 +397,22 @@ export function updatePairedTaskIfUnchangedInDatabase( fields.push('owner_failure_count = ?'); values.push(updates.owner_failure_count); } + if (updates.owner_step_done_streak !== undefined) { + fields.push('owner_step_done_streak = ?'); + values.push(updates.owner_step_done_streak); + } + if (updates.finalize_step_done_count !== undefined) { + fields.push('finalize_step_done_count = ?'); + values.push(updates.finalize_step_done_count); + } + if (updates.task_done_then_user_reopen_count !== undefined) { + fields.push('task_done_then_user_reopen_count = ?'); + values.push(updates.task_done_then_user_reopen_count); + } + if (updates.empty_step_done_streak !== undefined) { + fields.push('empty_step_done_streak = ?'); + values.push(updates.empty_step_done_streak); + } if (updates.status !== undefined) { fields.push('status = ?'); values.push(updates.status); diff --git a/src/db/paired-turn-outputs.ts b/src/db/paired-turn-outputs.ts index 9c89bb3..01eceda 100644 --- a/src/db/paired-turn-outputs.ts +++ b/src/db/paired-turn-outputs.ts @@ -1,6 +1,7 @@ import { Database } from 'bun:sqlite'; import { logger } from '../logger.js'; +import { parseVisibleVerdict } from '../paired-verdict.js'; import { PairedRoomRole, PairedTurnOutput } from '../types.js'; const MAX_TURN_OUTPUT_CHARS = 50_000; @@ -29,14 +30,15 @@ export function insertPairedTurnOutputInDatabase( database .prepare( `INSERT OR REPLACE INTO paired_turn_outputs - (task_id, turn_number, role, output_text, created_at) - VALUES (?, ?, ?, ?, ?)`, + (task_id, turn_number, role, output_text, verdict, created_at) + VALUES (?, ?, ?, ?, ?, ?)`, ) .run( taskId, turnNumber, role, outputText.slice(0, MAX_TURN_OUTPUT_CHARS), + parseVisibleVerdict(outputText), createdAt ?? new Date().toISOString(), ); } diff --git a/src/message-runtime-flow.ts b/src/message-runtime-flow.ts index 5d4f46d..9fa5aeb 100644 --- a/src/message-runtime-flow.ts +++ b/src/message-runtime-flow.ts @@ -24,12 +24,12 @@ import { } from './message-runtime-rules.js'; import type { ExecuteTurnFn } from './message-runtime-types.js'; import { buildPairedTurnIdentity } from './paired-turn-identity.js'; +import { resolveStoredVisibleVerdict } from './paired-verdict.js'; import { claimPairedTurnExecution, type ScheduledPairedFollowUpIntentKind, } from './paired-follow-up-scheduler.js'; import { hasReviewerLease } from './service-routing.js'; -import { parseVisibleVerdict } from './paired-execution-context-shared.js'; import type { Channel, NewMessage, @@ -128,9 +128,10 @@ export function buildPendingPairedTurn(args: { const nextTurnAction = resolveNextTurnAction({ taskStatus, lastTurnOutputRole: lastTurnOutput?.role ?? null, - lastTurnOutputVerdict: lastTurnOutput?.output_text - ? parseVisibleVerdict(lastTurnOutput.output_text) - : null, + lastTurnOutputVerdict: resolveStoredVisibleVerdict({ + verdict: lastTurnOutput?.verdict ?? null, + outputText: lastTurnOutput?.output_text ?? null, + }), }); const recentMessages = getRecentChatMessages(chatJid, 20); const lastHumanMessage = getLastHumanMessageContent(chatJid); diff --git a/src/message-runtime-follow-up.ts b/src/message-runtime-follow-up.ts index 97725eb..43fbf0d 100644 --- a/src/message-runtime-follow-up.ts +++ b/src/message-runtime-follow-up.ts @@ -1,8 +1,8 @@ import { getPairedTurnOutputs } from './db.js'; import { - parseVisibleVerdict, type VisibleVerdict, } from './paired-execution-context-shared.js'; +import { resolveStoredVisibleVerdict } from './paired-verdict.js'; import { matchesExpectedPairedFollowUpIntent, resolveFollowUpDispatch, @@ -55,9 +55,11 @@ export function resolveLatestPairedTurnOutputContext(args: { : null; return { role: latestOutput?.role ?? args.fallbackLastTurnOutputRole ?? null, - verdict: latestOutput?.output_text - ? parseVisibleVerdict(latestOutput.output_text) - : (args.fallbackLastTurnOutputVerdict ?? null), + verdict: + resolveStoredVisibleVerdict({ + verdict: latestOutput?.verdict ?? null, + outputText: latestOutput?.output_text ?? null, + }) ?? args.fallbackLastTurnOutputVerdict ?? null, }; } diff --git a/src/message-runtime-queue.ts b/src/message-runtime-queue.ts index 1ceaee8..4cacf38 100644 --- a/src/message-runtime-queue.ts +++ b/src/message-runtime-queue.ts @@ -11,7 +11,7 @@ import { isBotOnlyPairedRoomTurn, } from './message-runtime-flow.js'; import { buildPairedTurnIdentity } from './paired-turn-identity.js'; -import { parseVisibleVerdict } from './paired-execution-context-shared.js'; +import { resolveStoredVisibleVerdict } from './paired-verdict.js'; import { advanceLastAgentCursor, resolveActiveRole, @@ -185,9 +185,10 @@ export async function runQueuedGroupTurn(args: { ? (turnOutputs.at(-1)?.role ?? null) : null; const lastTurnOutputVerdict = currentTask - ? turnOutputs.at(-1)?.output_text - ? parseVisibleVerdict(turnOutputs.at(-1)?.output_text) - : null + ? resolveStoredVisibleVerdict({ + verdict: turnOutputs.at(-1)?.verdict ?? null, + outputText: turnOutputs.at(-1)?.output_text ?? null, + }) : null; const turnRole = currentTask ? hasHumanMsg diff --git a/src/message-runtime.test.ts b/src/message-runtime.test.ts index b5d82df..1af391c 100644 --- a/src/message-runtime.test.ts +++ b/src/message-runtime.test.ts @@ -842,6 +842,127 @@ describe('createMessageRuntime', () => { ); }); + it('suppresses a stale owner work item when a new human message arrives while the paired task is still active', async () => { + const chatJid = 'group@test'; + const group = makeGroup('codex'); + const channel = makeChannel(chatJid); + const enqueueMessageCheck = vi.fn(); + const activeTask = { + id: 'task-active-owner-follow-up', + chat_jid: chatJid, + group_folder: group.folder, + owner_service_id: 'claude', + reviewer_service_id: 'codex-main', + title: null, + source_ref: 'HEAD', + plan_notes: null, + review_requested_at: null, + round_trip_count: 0, + status: 'active', + arbiter_verdict: null, + arbiter_requested_at: null, + completion_reason: null, + created_at: '2026-03-30T00:00:00.000Z', + updated_at: '2026-03-30T00:00:05.000Z', + } as any; + + vi.mocked(serviceRouting.hasReviewerLease).mockReturnValue(true); + vi.mocked(db.getOpenWorkItem).mockReturnValue({ + id: 109, + group_folder: group.folder, + chat_jid: chatJid, + agent_type: 'codex', + service_id: 'claude', + delivery_role: 'owner', + status: 'delivery_retry', + start_seq: 1, + end_seq: 1, + result_payload: '이전 step 결과입니다.', + delivery_attempts: 1, + created_at: '2026-03-30T00:00:05.000Z', + updated_at: '2026-03-30T00:00:05.000Z', + delivered_at: null, + delivery_message_id: null, + last_error: 'discord send failed', + }); + vi.mocked(db.getMessagesSince).mockReturnValue([ + { + id: 'msg-2', + chat_jid: chatJid, + sender: 'user@test', + sender_name: 'User', + content: '이전 답 말고 이 방향으로 진행해줘', + timestamp: '2026-03-30T00:00:10.000Z', + seq: 2, + }, + ]); + vi.mocked(db.getLatestOpenPairedTaskForChat).mockReturnValue(activeTask); + vi.mocked(agentRunner.runAgentProcess).mockImplementation( + async (_group, input, _onProcess, onOutput) => { + expect(input.prompt).toContain('이 방향으로 진행해줘'); + expect(input.prompt).not.toContain('이전 step 결과입니다.'); + await onOutput?.({ + status: 'success', + phase: 'final', + result: 'STEP_DONE\n새 입력 기준으로 계속 진행', + output: { + visibility: 'public', + text: 'STEP_DONE\n새 입력 기준으로 계속 진행', + }, + } as any); + return { + status: 'success', + result: 'STEP_DONE\n새 입력 기준으로 계속 진행', + }; + }, + ); + + const runtime = createMessageRuntime({ + assistantName: 'Andy', + idleTimeout: 1_000, + pollInterval: 1_000, + timezone: 'UTC', + triggerPattern: /^@Andy\b/i, + channels: [channel], + queue: { + registerProcess: vi.fn(), + closeStdin: vi.fn(), + notifyIdle: vi.fn(), + enqueueMessageCheck, + } as any, + getRoomBindings: () => ({ [chatJid]: group }), + getSessions: () => ({}), + getLastTimestamp: () => '', + setLastTimestamp: vi.fn(), + getLastAgentTimestamps: () => ({}), + saveState: vi.fn(), + persistSession: vi.fn(), + clearSession: vi.fn(), + }); + + const result = await runtime.processGroupMessages(chatJid, { + runId: 'run-suppress-stale-active-owner-work-item', + reason: 'messages', + }); + + expect(result).toBe(true); + expect(db.markWorkItemDelivered).toHaveBeenCalledWith(109); + expect(channel.sendMessage).not.toHaveBeenCalledWith( + chatJid, + '이전 step 결과입니다.', + ); + expect(agentRunner.runAgentProcess).toHaveBeenCalledTimes(1); + expect(logger.info).toHaveBeenCalledWith( + expect.objectContaining({ + chatJid, + workItemId: 109, + taskId: 'task-active-owner-follow-up', + taskStatus: 'active', + }), + 'Suppressed stale owner delivery retry because a new human message arrived while the paired task was still active', + ); + }); + it('suppresses duplicate stale work item delivery and logs the suppression reason', async () => { const chatJid = 'group@test'; const group = makeGroup('codex'); diff --git a/src/message-runtime.ts b/src/message-runtime.ts index de06cbe..39dbf06 100644 --- a/src/message-runtime.ts +++ b/src/message-runtime.ts @@ -56,6 +56,7 @@ import { } from './types.js'; import { createScopedLogger, logger } from './logger.js'; import { hasReviewerLease } from './service-routing.js'; +import type { WorkItem } from './db/work-items.js'; export { resolveHandoffCursorKey, resolveHandoffRoleOverride, @@ -113,6 +114,54 @@ export function createMessageRuntime(deps: MessageRuntimeDeps): { deps.queue, ); + const getFreshHumanPreflightMessages = ( + chatJid: string, + channel: Channel, + ): NewMessage[] => { + const sinceSeqCursor = deps.getLastAgentTimestamps()[chatJid] || '0'; + const preflightRawMessages = getMessagesSinceSeq( + chatJid, + sinceSeqCursor, + deps.assistantName, + ); + const preflightMessages = filterLoopingPairedBotMessages( + chatJid, + getProcessableMessages(chatJid, preflightRawMessages, channel), + FAILURE_FINAL_TEXT, + ); + return preflightMessages.filter( + (message) => message.is_from_me !== true && !message.is_bot_message, + ); + }; + + const hasHumanMessageAfterWorkItem = ( + openWorkItem: WorkItem, + freshHumanMessages: NewMessage[], + ): boolean => { + if (freshHumanMessages.length === 0) { + return false; + } + + const workItemSeq = openWorkItem.end_seq ?? openWorkItem.start_seq ?? null; + const workItemUpdatedAt = Date.parse(openWorkItem.updated_at); + + return freshHumanMessages.some((message) => { + if (message.seq != null && workItemSeq != null) { + return message.seq > workItemSeq; + } + + const messageTimestamp = Date.parse(message.timestamp); + if ( + Number.isFinite(messageTimestamp) && + Number.isFinite(workItemUpdatedAt) + ) { + return messageTimestamp > workItemUpdatedAt; + } + + return true; + }); + }; + const scheduleQueuedPairedFollowUp = (args: { chatJid: string; runId: string; @@ -351,25 +400,12 @@ export function createMessageRuntime(deps: MessageRuntimeDeps): { ? getLatestOpenPairedTaskForChat(chatJid) : null; let openWorkItem = getOpenWorkItemForChat(chatJid, SERVICE_SESSION_SCOPE); - if ( - pendingTask?.status === 'merge_ready' && - openWorkItem?.delivery_role === 'owner' - ) { - const sinceSeqCursor = deps.getLastAgentTimestamps()[chatJid] || '0'; - const preflightRawMessages = getMessagesSinceSeq( - chatJid, - sinceSeqCursor, - deps.assistantName, - ); - const preflightMessages = filterLoopingPairedBotMessages( - chatJid, - getProcessableMessages(chatJid, preflightRawMessages, channel), - FAILURE_FINAL_TEXT, - ); - const hasFreshHumanMessage = preflightMessages.some( - (message) => message.is_from_me !== true && !message.is_bot_message, - ); - if (hasFreshHumanMessage) { + if (openWorkItem?.delivery_role === 'owner' && pendingTask) { + const freshHumanMessages = getFreshHumanPreflightMessages(chatJid, channel); + if ( + pendingTask.status === 'merge_ready' && + freshHumanMessages.length > 0 + ) { const resolvedTask = resolveOwnerTaskForHumanMessage({ group, chatJid, @@ -389,6 +425,23 @@ export function createMessageRuntime(deps: MessageRuntimeDeps): { ); openWorkItem = undefined; } + } else if ( + pendingTask.status === 'active' && + hasHumanMessageAfterWorkItem(openWorkItem, freshHumanMessages) + ) { + markWorkItemDelivered(openWorkItem.id); + log.info( + { + chatJid, + workItemId: openWorkItem.id, + taskId: pendingTask.id, + taskStatus: pendingTask.status, + workItemEndSeq: openWorkItem.end_seq ?? null, + freshHumanMessageCount: freshHumanMessages.length, + }, + 'Suppressed stale owner delivery retry because a new human message arrived while the paired task was still active', + ); + openWorkItem = undefined; } } const openWorkItemOutcome = await processOpenWorkItemDelivery({ diff --git a/src/paired-execution-context-owner.ts b/src/paired-execution-context-owner.ts index c59ae8b..547b4b9 100644 --- a/src/paired-execution-context-owner.ts +++ b/src/paired-execution-context-owner.ts @@ -22,6 +22,8 @@ import type { PairedTask } from './types.js'; type OwnerFinalizeOutcome = 'stop' | 're_review' | 'continue_owner'; const OWNER_FAILURE_ESCALATION_THRESHOLD = 2; +const EMPTY_STEP_DONE_THRESHOLD = 2; +const OWNER_STEP_DONE_LOOP_THRESHOLD = 3; export function handleFailedOwnerExecution(args: { task: PairedTask; @@ -101,6 +103,14 @@ function handleOwnerFinalizeCompletion(args: { const hasNewChanges = workspace?.workspace_dir ? hasCodeChangesSinceRef(workspace.workspace_dir, task.source_ref) : null; + const nextFinalizeStepDoneCount = + ownerVerdict === 'step_done' + ? (task.finalize_step_done_count ?? 0) + 1 + : task.finalize_step_done_count ?? 0; + const nextEmptyStepDoneStreak = + ownerVerdict === 'step_done' && hasNewChanges === false + ? (task.empty_step_done_streak ?? 0) + 1 + : 0; const signal = resolveOwnerCompletionSignal({ phase: 'finalize', visibleVerdict: ownerVerdict, @@ -138,6 +148,41 @@ function handleOwnerFinalizeCompletion(args: { }, patch: { owner_failure_count: 0, + owner_step_done_streak: 0, + finalize_step_done_count: nextFinalizeStepDoneCount, + empty_step_done_streak: nextEmptyStepDoneStreak, + }, + }); + return 'stop'; + } + + if ( + signal.kind === 'request_owner_continue' && + hasNewChanges === false && + nextEmptyStepDoneStreak >= EMPTY_STEP_DONE_THRESHOLD + ) { + requestArbiterOrEscalate({ + taskId, + currentStatus: task.status, + expectedUpdatedAt: task.updated_at, + now, + arbiterLogMessage: + 'Owner repeated STEP_DONE during finalize without code changes — requesting arbiter', + escalateLogMessage: + 'Owner repeated STEP_DONE during finalize without code changes — escalating to user', + logContext: { + taskId, + ownerVerdict, + hasNewChanges, + emptyStepDoneStreak: nextEmptyStepDoneStreak, + finalizeStepDoneCount: nextFinalizeStepDoneCount, + summary: summary?.slice(0, 100), + }, + patch: { + owner_failure_count: 0, + owner_step_done_streak: 0, + finalize_step_done_count: nextFinalizeStepDoneCount, + empty_step_done_streak: nextEmptyStepDoneStreak, }, }); return 'stop'; @@ -153,6 +198,9 @@ function handleOwnerFinalizeCompletion(args: { updatedAt: now, patch: { owner_failure_count: 0, + owner_step_done_streak: 0, + finalize_step_done_count: nextFinalizeStepDoneCount, + empty_step_done_streak: 0, }, }); } @@ -179,12 +227,17 @@ function handleOwnerFinalizeCompletion(args: { updatedAt: now, patch: { owner_failure_count: 0, + owner_step_done_streak: 0, + finalize_step_done_count: nextFinalizeStepDoneCount, + empty_step_done_streak: nextEmptyStepDoneStreak, }, }); logger.info( { taskId, ownerVerdict, + emptyStepDoneStreak: nextEmptyStepDoneStreak, + finalizeStepDoneCount: nextFinalizeStepDoneCount, summary: summary?.slice(0, 100), }, 'Owner marked finalize output as an intermediate step — task returned to active without re-review', @@ -201,6 +254,9 @@ function handleOwnerFinalizeCompletion(args: { patch: { completion_reason: 'done', owner_failure_count: 0, + owner_step_done_streak: 0, + finalize_step_done_count: nextFinalizeStepDoneCount, + empty_step_done_streak: 0, }, }); logger.info( @@ -242,6 +298,8 @@ function maybeAutoTriggerReviewerAfterOwnerCompletion(args: { patch: { round_trip_count: task.round_trip_count + 1, owner_failure_count: 0, + owner_step_done_streak: 0, + empty_step_done_streak: 0, }, }); if (hasActiveCiWatcherForChat(task.chat_jid)) { @@ -287,6 +345,16 @@ export function handleOwnerCompletion(args: { } const ownerVerdict = parseVisibleVerdict(summary); + const workspace = getPairedWorkspace(task.id, 'owner'); + const hasNewChanges = workspace?.workspace_dir + ? hasCodeChangesSinceRef(workspace.workspace_dir, task.source_ref) + : null; + const nextOwnerStepDoneStreak = + ownerVerdict === 'step_done' ? (task.owner_step_done_streak ?? 0) + 1 : 0; + const nextEmptyStepDoneStreak = + ownerVerdict === 'step_done' && hasNewChanges === false + ? (task.empty_step_done_streak ?? 0) + 1 + : 0; const signal = resolveOwnerCompletionSignal({ phase: 'normal', visibleVerdict: ownerVerdict, @@ -303,10 +371,46 @@ export function handleOwnerCompletion(args: { logContext: { taskId, ownerVerdict, + ownerStepDoneStreak: nextOwnerStepDoneStreak, + emptyStepDoneStreak: nextEmptyStepDoneStreak, summary: summary?.slice(0, 100), }, patch: { owner_failure_count: 0, + owner_step_done_streak: nextOwnerStepDoneStreak, + empty_step_done_streak: nextEmptyStepDoneStreak, + }, + }); + return; + } + + if ( + signal.kind === 'request_owner_continue' && + hasNewChanges === false && + nextOwnerStepDoneStreak >= OWNER_STEP_DONE_LOOP_THRESHOLD && + nextEmptyStepDoneStreak >= OWNER_STEP_DONE_LOOP_THRESHOLD + ) { + requestArbiterOrEscalate({ + taskId, + currentStatus: task.status, + expectedUpdatedAt: task.updated_at, + now, + arbiterLogMessage: + 'Owner repeated STEP_DONE in active mode without code changes — requesting arbiter', + escalateLogMessage: + 'Owner repeated STEP_DONE in active mode without code changes — escalating to user', + logContext: { + taskId, + ownerVerdict, + hasNewChanges, + ownerStepDoneStreak: nextOwnerStepDoneStreak, + emptyStepDoneStreak: nextEmptyStepDoneStreak, + summary: summary?.slice(0, 100), + }, + patch: { + owner_failure_count: 0, + owner_step_done_streak: nextOwnerStepDoneStreak, + empty_step_done_streak: nextEmptyStepDoneStreak, }, }); return; @@ -319,15 +423,38 @@ export function handleOwnerCompletion(args: { updatedAt: now, patch: { owner_failure_count: 0, + owner_step_done_streak: nextOwnerStepDoneStreak, + empty_step_done_streak: nextEmptyStepDoneStreak, }, }); logger.info( - { taskId, ownerVerdict, summary: summary?.slice(0, 100) }, + { + taskId, + ownerVerdict, + hasNewChanges, + ownerStepDoneStreak: nextOwnerStepDoneStreak, + emptyStepDoneStreak: nextEmptyStepDoneStreak, + summary: summary?.slice(0, 100), + }, 'Owner marked the current output as an intermediate completed step — keeping task active for follow-up', ); return; } + if ( + nextOwnerStepDoneStreak !== (task.owner_step_done_streak ?? 0) || + nextEmptyStepDoneStreak !== (task.empty_step_done_streak ?? 0) + ) { + applyPairedTaskPatch({ + taskId, + expectedUpdatedAt: task.updated_at, + updatedAt: now, + patch: { + owner_step_done_streak: nextOwnerStepDoneStreak, + empty_step_done_streak: nextEmptyStepDoneStreak, + }, + }); + } maybeAutoTriggerReviewerAfterOwnerCompletion({ task, taskId, diff --git a/src/paired-execution-context-shared.ts b/src/paired-execution-context-shared.ts index 8bf11fc..6e59957 100644 --- a/src/paired-execution-context-shared.ts +++ b/src/paired-execution-context-shared.ts @@ -3,17 +3,12 @@ import { execFileSync } from 'child_process'; import { isArbiterEnabled } from './config.js'; import { updatePairedTaskIfUnchanged } from './db.js'; import { logger } from './logger.js'; +import { + parseVisibleVerdict, + type VisibleVerdict, +} from './paired-verdict.js'; import type { PairedTaskStatus } from './types.js'; -export type VisibleVerdict = - | 'step_done' - | 'task_done' - | 'done' - | 'done_with_concerns' - | 'blocked' - | 'needs_context' - | 'continue'; - export type CompletionSignal = | { kind: 'request_reviewer'; resetStatusToActive: boolean } | { kind: 'request_owner_finalize' } @@ -23,24 +18,8 @@ export type CompletionSignal = | { kind: 'complete'; completionReason: 'done' | 'escalated' } | { kind: 'preserve_review_ready' }; -export function parseVisibleVerdict( - summary: string | null | undefined, -): VisibleVerdict { - if (!summary) return 'continue'; - const cleaned = summary.replace(/[\s\S]*?<\/internal>/g, '').trim(); - if (!cleaned) return 'continue'; - const firstLine = cleaned.split('\n')[0].trim(); - if (/^\*{0,2}BLOCKED\*{0,2}\b/i.test(firstLine)) return 'blocked'; - if (/^\*{0,2}NEEDS_CONTEXT\*{0,2}\b/i.test(firstLine)) return 'needs_context'; - if (/^\*{0,2}STEP_DONE\*{0,2}\b/i.test(firstLine)) return 'step_done'; - if (/^\*{0,2}TASK_DONE\*{0,2}\b/i.test(firstLine)) return 'task_done'; - if (/^\*{0,2}DONE_WITH_CONCERNS\*{0,2}\b/i.test(firstLine)) - return 'done_with_concerns'; - if (/^\*{0,2}DONE\*{0,2}\b/i.test(firstLine)) return 'done'; - if (/^\*{0,2}Approved\.?\*{0,2}/i.test(firstLine)) return 'done'; - if (/^\*{0,2}LGTM\*{0,2}/i.test(firstLine)) return 'done'; - return 'continue'; -} +export { parseVisibleVerdict }; +export type { VisibleVerdict }; export function resolveOwnerCompletionSignal(args: { phase: 'normal' | 'finalize'; @@ -270,6 +249,10 @@ export function transitionPairedTaskStatus(args: { review_requested_at?: string | null; round_trip_count?: number; owner_failure_count?: number; + owner_step_done_streak?: number; + finalize_step_done_count?: number; + task_done_then_user_reopen_count?: number; + empty_step_done_streak?: number; arbiter_verdict?: string | null; arbiter_requested_at?: string | null; completion_reason?: string | null; @@ -314,6 +297,10 @@ export function applyPairedTaskPatch(args: { review_requested_at?: string | null; round_trip_count?: number; owner_failure_count?: number; + owner_step_done_streak?: number; + finalize_step_done_count?: number; + task_done_then_user_reopen_count?: number; + empty_step_done_streak?: number; status?: PairedTaskStatus; arbiter_verdict?: string | null; arbiter_requested_at?: string | null; @@ -355,6 +342,10 @@ export function requestArbiterOrEscalate(args: { review_requested_at?: string | null; round_trip_count?: number; owner_failure_count?: number; + owner_step_done_streak?: number; + finalize_step_done_count?: number; + task_done_then_user_reopen_count?: number; + empty_step_done_streak?: number; arbiter_verdict?: string | null; arbiter_requested_at?: string | null; completion_reason?: string | null; diff --git a/src/paired-execution-context.test.ts b/src/paired-execution-context.test.ts index e3f29bc..c7a6df2 100644 --- a/src/paired-execution-context.test.ts +++ b/src/paired-execution-context.test.ts @@ -143,6 +143,10 @@ function buildPairedTask(overrides: Partial = {}): PairedTask { review_requested_at: null, round_trip_count: 0, owner_failure_count: 0, + owner_step_done_streak: 0, + finalize_step_done_count: 0, + task_done_then_user_reopen_count: 0, + empty_step_done_streak: 0, status: 'active', arbiter_verdict: null, arbiter_requested_at: null, @@ -293,6 +297,62 @@ describe('paired execution context', () => { expect(db.insertPairedTurnOutput).not.toHaveBeenCalled(); }); + it('records a quick reopen when a new owner task starts shortly after TASK_DONE completion', () => { + const previousTask = buildPairedTask({ + id: 'task-completed', + status: 'completed', + completion_reason: 'done', + updated_at: new Date(Date.now() - 60_000).toISOString(), + task_done_then_user_reopen_count: 0, + }); + vi.mocked(db.getLatestOpenPairedTaskForChat).mockReturnValue(undefined); + vi.mocked(db.getLatestPairedTaskForChat).mockReturnValue(previousTask); + + const result = resolveOwnerTaskForHumanMessage({ + group, + chatJid: 'dc:test', + roomRoleContext: ownerContext, + existingTask: null, + }); + + expect(result.task).not.toBeNull(); + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-completed', + expect.objectContaining({ + task_done_then_user_reopen_count: 1, + }), + ); + }); + + it('resets active STEP_DONE loop counters when a new human message continues an active task', () => { + vi.mocked(db.getLatestOpenPairedTaskForChat).mockReturnValue( + buildPairedTask({ + status: 'active', + owner_failure_count: 1, + owner_step_done_streak: 2, + empty_step_done_streak: 2, + }), + ); + + preparePairedExecutionContext({ + group, + chatJid: 'dc:test', + runId: 'run-human-reset-step-done-loop', + roomRoleContext: ownerContext, + hasHumanMessage: true, + }); + + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + round_trip_count: 0, + owner_failure_count: 0, + owner_step_done_streak: 0, + empty_step_done_streak: 0, + }), + ); + }); + it('uses room role context agent overrides when creating a paired task', () => { preparePairedExecutionContext({ group, @@ -761,6 +821,7 @@ describe('paired execution context', () => { 'task-1', expect.objectContaining({ owner_failure_count: 0, + owner_step_done_streak: 1, }), ); expect( @@ -774,6 +835,43 @@ describe('paired execution context', () => { ); }); + it('requests arbiter when active STEP_DONE repeats without code changes', () => { + vi.spyOn(config, 'isArbiterEnabled').mockReturnValue(true); + + const repoDir = createCanonicalRepoWithCommit('active step done loop'); + const sourceRef = resolveTreeRef(repoDir); + vi.mocked(db.getPairedTaskById).mockReturnValue( + buildPairedTask({ + status: 'active', + source_ref: sourceRef, + owner_step_done_streak: 2, + empty_step_done_streak: 2, + }), + ); + vi.mocked(db.getPairedWorkspace).mockImplementation((_taskId, role) => + role === 'owner' ? buildWorkspace('owner', repoDir) : undefined, + ); + + completePairedExecutionContext({ + taskId: 'task-1', + role: 'owner', + status: 'succeeded', + summary: 'STEP_DONE\n요약만 반복되고 코드 변경은 없음', + }); + + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'arbiter_requested', + owner_step_done_streak: 3, + empty_step_done_streak: 3, + }), + ); + expect( + pairedWorkspaceManager.markPairedTaskReviewReady, + ).not.toHaveBeenCalled(); + }); + it('returns merge_ready owner finalize output to active when the owner reports STEP_DONE', () => { vi.mocked(db.getPairedTaskById).mockReturnValue( buildPairedTask({ @@ -794,6 +892,44 @@ describe('paired execution context', () => { expect.objectContaining({ status: 'active', owner_failure_count: 0, + finalize_step_done_count: 1, + }), + ); + expect( + pairedWorkspaceManager.markPairedTaskReviewReady, + ).not.toHaveBeenCalled(); + }); + + it('requests arbiter when finalize STEP_DONE repeats without code changes', () => { + vi.spyOn(config, 'isArbiterEnabled').mockReturnValue(true); + + const repoDir = createCanonicalRepoWithCommit('reviewed'); + const approvedSourceRef = resolveTreeRef(repoDir); + vi.mocked(db.getPairedTaskById).mockReturnValue( + buildPairedTask({ + status: 'merge_ready', + source_ref: approvedSourceRef, + empty_step_done_streak: 1, + finalize_step_done_count: 1, + }), + ); + vi.mocked(db.getPairedWorkspace).mockImplementation((_taskId, role) => + role === 'owner' ? buildWorkspace('owner', repoDir) : undefined, + ); + + completePairedExecutionContext({ + taskId: 'task-1', + role: 'owner', + status: 'succeeded', + summary: 'STEP_DONE\n아직 남았지만 코드 변경은 없음', + }); + + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'arbiter_requested', + empty_step_done_streak: 2, + finalize_step_done_count: 2, }), ); expect( diff --git a/src/paired-execution-context.ts b/src/paired-execution-context.ts index 45038e3..3732eee 100644 --- a/src/paired-execution-context.ts +++ b/src/paired-execution-context.ts @@ -30,6 +30,7 @@ import { hasActiveCiWatcherForChat, insertPairedTurnOutput, releasePairedTaskExecutionLease, + updatePairedTask, upsertPairedProject, } from './db.js'; import { logger } from './logger.js'; @@ -72,6 +73,8 @@ import type { } from './types.js'; import { resolveRoleAgentPlan } from './role-agent-plan.js'; +const TASK_DONE_REOPEN_WINDOW_MS = 10 * 60_000; + function ensurePairedProject( group: RegisteredGroup, chatJid: string, @@ -144,6 +147,10 @@ function createActiveTaskForRoom(args: { review_requested_at: null, round_trip_count: 0, owner_failure_count: 0, + owner_step_done_streak: 0, + finalize_step_done_count: 0, + task_done_then_user_reopen_count: 0, + empty_step_done_streak: 0, status: 'active', arbiter_verdict: null, arbiter_requested_at: null, @@ -164,6 +171,39 @@ function createActiveTaskForRoom(args: { return task; } +function maybeRecordTaskDoneReopen(previousTask: PairedTask | null): void { + if ( + !previousTask || + previousTask.status !== 'completed' || + previousTask.completion_reason !== 'done' + ) { + return; + } + + const completedAt = Date.parse(previousTask.updated_at); + if (!Number.isFinite(completedAt)) { + return; + } + if (Date.now() - completedAt > TASK_DONE_REOPEN_WINDOW_MS) { + return; + } + + updatePairedTask(previousTask.id, { + task_done_then_user_reopen_count: + (previousTask.task_done_then_user_reopen_count ?? 0) + 1, + updated_at: new Date().toISOString(), + }); + logger.info( + { + taskId: previousTask.id, + chatJid: previousTask.chat_jid, + reopenCount: (previousTask.task_done_then_user_reopen_count ?? 0) + 1, + completionReason: previousTask.completion_reason, + }, + 'Recorded paired task reopen shortly after TASK_DONE completion', + ); +} + function cancelOutstandingFinalizeOwnerTurn(task: PairedTask): void { const turnIdentity = buildPairedTurnIdentity({ taskId: task.id, @@ -253,6 +293,7 @@ export function resolveOwnerTaskForHumanMessage(args: { args.existingTask ?? getLatestOpenPairedTaskForChat(args.chatJid) ?? null; if (!existing) { + maybeRecordTaskDoneReopen(getLatestPairedTaskForChat(args.chatJid) ?? null); return { task: canonicalWorkDir ? createActiveTaskForRoom({ @@ -432,7 +473,12 @@ export function preparePairedExecutionContext(args: { updatedAt: now, patch: { ...(hasHuman - ? { round_trip_count: 0, owner_failure_count: 0 } + ? { + round_trip_count: 0, + owner_failure_count: 0, + owner_step_done_streak: 0, + empty_step_done_streak: 0, + } : {}), }, }); @@ -443,7 +489,12 @@ export function preparePairedExecutionContext(args: { updatedAt: now, patch: { ...(hasHuman - ? { round_trip_count: 0, owner_failure_count: 0 } + ? { + round_trip_count: 0, + owner_failure_count: 0, + owner_step_done_streak: 0, + empty_step_done_streak: 0, + } : {}), }, }); diff --git a/src/paired-verdict.ts b/src/paired-verdict.ts new file mode 100644 index 0000000..57e375d --- /dev/null +++ b/src/paired-verdict.ts @@ -0,0 +1,41 @@ +export type VisibleVerdict = + | 'step_done' + | 'task_done' + | 'done' + | 'done_with_concerns' + | 'blocked' + | 'needs_context' + | 'continue'; + +export function parseVisibleVerdict( + summary: string | null | undefined, +): VisibleVerdict { + if (!summary) return 'continue'; + const cleaned = summary.replace(/[\s\S]*?<\/internal>/g, '').trim(); + if (!cleaned) return 'continue'; + const firstLine = cleaned.split('\n')[0].trim(); + if (/^\*{0,2}BLOCKED\*{0,2}\b/i.test(firstLine)) return 'blocked'; + if (/^\*{0,2}NEEDS_CONTEXT\*{0,2}\b/i.test(firstLine)) + return 'needs_context'; + if (/^\*{0,2}STEP_DONE\*{0,2}\b/i.test(firstLine)) return 'step_done'; + if (/^\*{0,2}TASK_DONE\*{0,2}\b/i.test(firstLine)) return 'task_done'; + if (/^\*{0,2}DONE_WITH_CONCERNS\*{0,2}\b/i.test(firstLine)) + return 'done_with_concerns'; + if (/^\*{0,2}DONE\*{0,2}\b/i.test(firstLine)) return 'done'; + if (/^\*{0,2}Approved\.?\*{0,2}/i.test(firstLine)) return 'done'; + if (/^\*{0,2}LGTM\*{0,2}/i.test(firstLine)) return 'done'; + return 'continue'; +} + +export function resolveStoredVisibleVerdict(args: { + verdict?: VisibleVerdict | null; + outputText?: string | null; +}): VisibleVerdict | null { + if (args.verdict) { + return args.verdict; + } + if (!args.outputText) { + return null; + } + return parseVisibleVerdict(args.outputText); +} diff --git a/src/types.ts b/src/types.ts index 49842fa..5c00ed3 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1,3 +1,5 @@ +import type { VisibleVerdict } from './paired-verdict.js'; + export interface AgentConfig { timeout?: number; // Default: 300000 (5 minutes) // Per-group model/effort overrides (take precedence over global env vars) @@ -83,6 +85,10 @@ export interface PairedTask { review_requested_at: string | null; round_trip_count: number; owner_failure_count?: number | null; + owner_step_done_streak?: number | null; + finalize_step_done_count?: number | null; + task_done_then_user_reopen_count?: number | null; + empty_step_done_streak?: number | null; status: PairedTaskStatus; arbiter_verdict: string | null; arbiter_requested_at: string | null; @@ -97,6 +103,7 @@ export interface PairedTurnOutput { turn_number: number; role: PairedRoomRole; output_text: string; + verdict?: VisibleVerdict | null; created_at: string; } From 7d098cbc00235d45a9054847334430ef309847f6 Mon Sep 17 00:00:00 2001 From: ejclaw Date: Thu, 23 Apr 2026 18:51:56 +0900 Subject: [PATCH 010/285] style: format paired stepdone telemetry files --- src/message-runtime-follow-up.ts | 8 ++++---- src/message-runtime.ts | 5 ++++- src/paired-execution-context-owner.ts | 2 +- src/paired-execution-context-shared.ts | 5 +---- src/paired-verdict.ts | 3 +-- 5 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/message-runtime-follow-up.ts b/src/message-runtime-follow-up.ts index 43fbf0d..0bed522 100644 --- a/src/message-runtime-follow-up.ts +++ b/src/message-runtime-follow-up.ts @@ -1,7 +1,5 @@ import { getPairedTurnOutputs } from './db.js'; -import { - type VisibleVerdict, -} from './paired-execution-context-shared.js'; +import { type VisibleVerdict } from './paired-execution-context-shared.js'; import { resolveStoredVisibleVerdict } from './paired-verdict.js'; import { matchesExpectedPairedFollowUpIntent, @@ -59,7 +57,9 @@ export function resolveLatestPairedTurnOutputContext(args: { resolveStoredVisibleVerdict({ verdict: latestOutput?.verdict ?? null, outputText: latestOutput?.output_text ?? null, - }) ?? args.fallbackLastTurnOutputVerdict ?? null, + }) ?? + args.fallbackLastTurnOutputVerdict ?? + null, }; } diff --git a/src/message-runtime.ts b/src/message-runtime.ts index 39dbf06..bea78ab 100644 --- a/src/message-runtime.ts +++ b/src/message-runtime.ts @@ -401,7 +401,10 @@ export function createMessageRuntime(deps: MessageRuntimeDeps): { : null; let openWorkItem = getOpenWorkItemForChat(chatJid, SERVICE_SESSION_SCOPE); if (openWorkItem?.delivery_role === 'owner' && pendingTask) { - const freshHumanMessages = getFreshHumanPreflightMessages(chatJid, channel); + const freshHumanMessages = getFreshHumanPreflightMessages( + chatJid, + channel, + ); if ( pendingTask.status === 'merge_ready' && freshHumanMessages.length > 0 diff --git a/src/paired-execution-context-owner.ts b/src/paired-execution-context-owner.ts index 547b4b9..f820007 100644 --- a/src/paired-execution-context-owner.ts +++ b/src/paired-execution-context-owner.ts @@ -106,7 +106,7 @@ function handleOwnerFinalizeCompletion(args: { const nextFinalizeStepDoneCount = ownerVerdict === 'step_done' ? (task.finalize_step_done_count ?? 0) + 1 - : task.finalize_step_done_count ?? 0; + : (task.finalize_step_done_count ?? 0); const nextEmptyStepDoneStreak = ownerVerdict === 'step_done' && hasNewChanges === false ? (task.empty_step_done_streak ?? 0) + 1 diff --git a/src/paired-execution-context-shared.ts b/src/paired-execution-context-shared.ts index 6e59957..998e374 100644 --- a/src/paired-execution-context-shared.ts +++ b/src/paired-execution-context-shared.ts @@ -3,10 +3,7 @@ import { execFileSync } from 'child_process'; import { isArbiterEnabled } from './config.js'; import { updatePairedTaskIfUnchanged } from './db.js'; import { logger } from './logger.js'; -import { - parseVisibleVerdict, - type VisibleVerdict, -} from './paired-verdict.js'; +import { parseVisibleVerdict, type VisibleVerdict } from './paired-verdict.js'; import type { PairedTaskStatus } from './types.js'; export type CompletionSignal = diff --git a/src/paired-verdict.ts b/src/paired-verdict.ts index 57e375d..a2eef37 100644 --- a/src/paired-verdict.ts +++ b/src/paired-verdict.ts @@ -15,8 +15,7 @@ export function parseVisibleVerdict( if (!cleaned) return 'continue'; const firstLine = cleaned.split('\n')[0].trim(); if (/^\*{0,2}BLOCKED\*{0,2}\b/i.test(firstLine)) return 'blocked'; - if (/^\*{0,2}NEEDS_CONTEXT\*{0,2}\b/i.test(firstLine)) - return 'needs_context'; + if (/^\*{0,2}NEEDS_CONTEXT\*{0,2}\b/i.test(firstLine)) return 'needs_context'; if (/^\*{0,2}STEP_DONE\*{0,2}\b/i.test(firstLine)) return 'step_done'; if (/^\*{0,2}TASK_DONE\*{0,2}\b/i.test(firstLine)) return 'task_done'; if (/^\*{0,2}DONE_WITH_CONCERNS\*{0,2}\b/i.test(firstLine)) From 65dd82ba6b581a40eeb515423f70578cebac521c Mon Sep 17 00:00:00 2001 From: ejclaw Date: Fri, 24 Apr 2026 12:03:12 +0900 Subject: [PATCH 011/285] Route STEP_DONE through reviewer --- src/message-runtime-follow-up.test.ts | 8 +- src/message-runtime-rules.test.ts | 8 +- src/message-runtime-rules.ts | 3 +- src/message-runtime.test.ts | 20 +-- src/paired-execution-context-owner.ts | 132 +++++--------------- src/paired-execution-context-shared.test.ts | 4 +- src/paired-execution-context-shared.ts | 9 +- src/paired-execution-context.test.ts | 77 +++++++++--- 8 files changed, 105 insertions(+), 156 deletions(-) diff --git a/src/message-runtime-follow-up.test.ts b/src/message-runtime-follow-up.test.ts index 4bf4dfc..f8c3c76 100644 --- a/src/message-runtime-follow-up.test.ts +++ b/src/message-runtime-follow-up.test.ts @@ -83,7 +83,7 @@ describe('message-runtime-follow-up', () => { expect(enqueue).toHaveBeenCalledTimes(1); }); - it('uses the fallback STEP_DONE verdict to schedule owner follow-ups when the owner keeps the task active', () => { + it('does not use fallback owner STEP_DONE verdict to schedule owner follow-ups while active', () => { const enqueue = vi.fn(); const result = dispatchPairedFollowUpForEvent({ chatJid: 'group@test', @@ -102,14 +102,12 @@ describe('message-runtime-follow-up', () => { }); expect(result).toMatchObject({ - kind: 'paired-follow-up', - intentKind: 'owner-follow-up', - scheduled: true, + kind: 'none', taskStatus: 'active', lastTurnOutputRole: 'owner', lastTurnOutputVerdict: 'step_done', }); - expect(enqueue).toHaveBeenCalledTimes(1); + expect(enqueue).not.toHaveBeenCalled(); }); it('prefers the latest persisted turn output over the fallback delivery role when both are present', () => { diff --git a/src/message-runtime-rules.test.ts b/src/message-runtime-rules.test.ts index db9b4bd..d8eba00 100644 --- a/src/message-runtime-rules.test.ts +++ b/src/message-runtime-rules.test.ts @@ -120,7 +120,7 @@ describe('message-runtime-rules', () => { lastTurnOutputVerdict: 'step_done', intentKind: 'owner-follow-up', }), - ).toBe(true); + ).toBe(false); }); it('maps active tasks with reviewer output to an owner follow-up', () => { @@ -132,14 +132,14 @@ describe('message-runtime-rules', () => { ).toEqual({ kind: 'owner-follow-up' }); }); - it('maps active tasks with owner STEP_DONE output to an owner follow-up', () => { + it('does not map active owner STEP_DONE output to an owner follow-up', () => { expect( resolveNextTurnAction({ taskStatus: 'active', lastTurnOutputRole: 'owner', lastTurnOutputVerdict: 'step_done', }), - ).toEqual({ kind: 'owner-follow-up' }); + ).toEqual({ kind: 'none' }); }); it('returns none when an active task has no reviewer or arbiter handoff output', () => { @@ -329,7 +329,7 @@ describe('message-runtime-rules', () => { lastTurnOutputRole: 'owner', lastTurnOutputVerdict: 'step_done', }), - ).toBe('owner'); + ).toBeNull(); }); it('resolves reviewer execution target from review_ready task status', () => { diff --git a/src/message-runtime-rules.ts b/src/message-runtime-rules.ts index 033abb1..c7582e2 100644 --- a/src/message-runtime-rules.ts +++ b/src/message-runtime-rules.ts @@ -146,8 +146,7 @@ export function resolveNextTurnAction(args: { ? { kind: 'none' } : { kind: 'finalize-owner-turn' }; case 'active': - return args.lastTurnOutputVerdict === 'step_done' || - args.lastTurnOutputRole === 'reviewer' || + return args.lastTurnOutputRole === 'reviewer' || args.lastTurnOutputRole === 'arbiter' ? { kind: 'owner-follow-up' } : { kind: 'none' }; diff --git a/src/message-runtime.test.ts b/src/message-runtime.test.ts index 1af391c..6bf1b59 100644 --- a/src/message-runtime.test.ts +++ b/src/message-runtime.test.ts @@ -2126,7 +2126,7 @@ If your first line is DONE_WITH_CONCERNS, the system will reopen review instead expect(pending?.prompt).toContain('이전 reviewer 피드백'); }); - it('builds an owner follow-up pending turn when the latest owner output is STEP_DONE on an active task', () => { + it('does not build an owner follow-up pending turn from owner STEP_DONE on an active task', () => { const chatJid = 'group@test'; const group = makeGroup('claude-code'); const task = { @@ -2170,12 +2170,7 @@ If your first line is DONE_WITH_CONCERNS, the system will reopen review instead resolveChannel: () => makeChannel(chatJid), }); - expect(pending).not.toBeNull(); - expect(pending).toMatchObject({ - role: 'owner', - intentKind: 'owner-follow-up', - taskId: task.id, - }); + expect(pending).toBeNull(); }); it('builds a reviewer pending turn when a review_ready task follows owner TASK_DONE output', () => { @@ -2335,7 +2330,7 @@ If your first line is DONE_WITH_CONCERNS, the system will reopen review instead ); }); - it('re-enqueues owner follow-up after a successful owner STEP_DONE delivery keeps the task active', async () => { + it('does not re-enqueue owner follow-up after a successful owner STEP_DONE delivery keeps the task active', async () => { const chatJid = 'group@test'; const group = makeGroup('codex'); const ownerChannel: Channel = { @@ -2445,17 +2440,12 @@ If your first line is DONE_WITH_CONCERNS, the system will reopen review instead chatJid, 'STEP_DONE\n리팩토링 1단계 완료, 다음 단계 진행', ); - expect(enqueueMessageCheck).toHaveBeenCalledWith(chatJid); + expect(enqueueMessageCheck).not.toHaveBeenCalled(); expect(pairedTask.status).toBe('active'); expect(pairedTask.round_trip_count).toBe(1); expect(pairedTask.owner_failure_count).toBe(0); - expect(logger.info).toHaveBeenCalledWith( + expect(logger.info).not.toHaveBeenCalledWith( expect.objectContaining({ - chatJid, - runId: 'run-owner-step-done-follow-up', - completedRole: 'owner', - taskId: 'task-owner-step-done-follow-up', - taskStatus: 'active', intentKind: 'owner-follow-up', }), 'Queued paired follow-up after successful owner delivery', diff --git a/src/paired-execution-context-owner.ts b/src/paired-execution-context-owner.ts index f820007..beb958a 100644 --- a/src/paired-execution-context-owner.ts +++ b/src/paired-execution-context-owner.ts @@ -20,10 +20,9 @@ import { } from './paired-execution-context-shared.js'; import type { PairedTask } from './types.js'; -type OwnerFinalizeOutcome = 'stop' | 're_review' | 'continue_owner'; +type OwnerFinalizeOutcome = 'stop' | 're_review'; const OWNER_FAILURE_ESCALATION_THRESHOLD = 2; const EMPTY_STEP_DONE_THRESHOLD = 2; -const OWNER_STEP_DONE_LOOP_THRESHOLD = 3; export function handleFailedOwnerExecution(args: { task: PairedTask; @@ -157,7 +156,7 @@ function handleOwnerFinalizeCompletion(args: { } if ( - signal.kind === 'request_owner_continue' && + ownerVerdict === 'step_done' && hasNewChanges === false && nextEmptyStepDoneStreak >= EMPTY_STEP_DONE_THRESHOLD ) { @@ -200,7 +199,8 @@ function handleOwnerFinalizeCompletion(args: { owner_failure_count: 0, owner_step_done_streak: 0, finalize_step_done_count: nextFinalizeStepDoneCount, - empty_step_done_streak: 0, + empty_step_done_streak: + ownerVerdict === 'step_done' ? nextEmptyStepDoneStreak : 0, }, }); } @@ -213,36 +213,27 @@ function handleOwnerFinalizeCompletion(args: { }, ownerVerdict === 'done_with_concerns' ? 'Owner raised concerns during finalize — task set back to active' - : 'Owner made changes after reviewer approval — task set back to active before re-review', + : ownerVerdict === 'step_done' + ? 'Owner reported STEP_DONE during finalize — task set back to active before review' + : 'Owner made changes after reviewer approval — task set back to active before re-review', ); - return 're_review'; - } - - if (signal.kind === 'request_owner_continue') { - transitionPairedTaskStatus({ + maybeAutoTriggerReviewerAfterOwnerCompletion({ + task, taskId, - currentStatus: task.status, - nextStatus: 'active', - expectedUpdatedAt: task.updated_at, - updatedAt: now, - patch: { - owner_failure_count: 0, - owner_step_done_streak: 0, - finalize_step_done_count: nextFinalizeStepDoneCount, - empty_step_done_streak: nextEmptyStepDoneStreak, - }, + now, + logMessage: + ownerVerdict === 'step_done' + ? 'Auto-triggered reviewer after owner finalize STEP_DONE' + : 'Auto-triggered reviewer after owner finalize required re-review', + patch: + ownerVerdict === 'step_done' + ? { + finalize_step_done_count: nextFinalizeStepDoneCount, + empty_step_done_streak: nextEmptyStepDoneStreak, + } + : undefined, }); - logger.info( - { - taskId, - ownerVerdict, - emptyStepDoneStreak: nextEmptyStepDoneStreak, - finalizeStepDoneCount: nextFinalizeStepDoneCount, - summary: summary?.slice(0, 100), - }, - 'Owner marked finalize output as an intermediate step — task returned to active without re-review', - ); - return 'continue_owner'; + return 'stop'; } transitionPairedTaskStatus({ @@ -271,6 +262,11 @@ function maybeAutoTriggerReviewerAfterOwnerCompletion(args: { taskId: string; now: string; logMessage: string; + patch?: { + owner_step_done_streak?: number; + finalize_step_done_count?: number; + empty_step_done_streak?: number; + }; }): void { const { task, taskId, now, logMessage } = args; if (task.round_trip_count >= PAIRED_MAX_ROUND_TRIPS) { @@ -300,6 +296,7 @@ function maybeAutoTriggerReviewerAfterOwnerCompletion(args: { owner_failure_count: 0, owner_step_done_streak: 0, empty_step_done_streak: 0, + ...args.patch, }, }); if (hasActiveCiWatcherForChat(task.chat_jid)) { @@ -345,16 +342,8 @@ export function handleOwnerCompletion(args: { } const ownerVerdict = parseVisibleVerdict(summary); - const workspace = getPairedWorkspace(task.id, 'owner'); - const hasNewChanges = workspace?.workspace_dir - ? hasCodeChangesSinceRef(workspace.workspace_dir, task.source_ref) - : null; const nextOwnerStepDoneStreak = ownerVerdict === 'step_done' ? (task.owner_step_done_streak ?? 0) + 1 : 0; - const nextEmptyStepDoneStreak = - ownerVerdict === 'step_done' && hasNewChanges === false - ? (task.empty_step_done_streak ?? 0) + 1 - : 0; const signal = resolveOwnerCompletionSignal({ phase: 'normal', visibleVerdict: ownerVerdict, @@ -372,86 +361,23 @@ export function handleOwnerCompletion(args: { taskId, ownerVerdict, ownerStepDoneStreak: nextOwnerStepDoneStreak, - emptyStepDoneStreak: nextEmptyStepDoneStreak, summary: summary?.slice(0, 100), }, patch: { owner_failure_count: 0, owner_step_done_streak: nextOwnerStepDoneStreak, - empty_step_done_streak: nextEmptyStepDoneStreak, }, }); return; } - if ( - signal.kind === 'request_owner_continue' && - hasNewChanges === false && - nextOwnerStepDoneStreak >= OWNER_STEP_DONE_LOOP_THRESHOLD && - nextEmptyStepDoneStreak >= OWNER_STEP_DONE_LOOP_THRESHOLD - ) { - requestArbiterOrEscalate({ - taskId, - currentStatus: task.status, - expectedUpdatedAt: task.updated_at, - now, - arbiterLogMessage: - 'Owner repeated STEP_DONE in active mode without code changes — requesting arbiter', - escalateLogMessage: - 'Owner repeated STEP_DONE in active mode without code changes — escalating to user', - logContext: { - taskId, - ownerVerdict, - hasNewChanges, - ownerStepDoneStreak: nextOwnerStepDoneStreak, - emptyStepDoneStreak: nextEmptyStepDoneStreak, - summary: summary?.slice(0, 100), - }, - patch: { - owner_failure_count: 0, - owner_step_done_streak: nextOwnerStepDoneStreak, - empty_step_done_streak: nextEmptyStepDoneStreak, - }, - }); - return; - } - - if (signal.kind === 'request_owner_continue') { - applyPairedTaskPatch({ - taskId, - expectedUpdatedAt: task.updated_at, - updatedAt: now, - patch: { - owner_failure_count: 0, - owner_step_done_streak: nextOwnerStepDoneStreak, - empty_step_done_streak: nextEmptyStepDoneStreak, - }, - }); - logger.info( - { - taskId, - ownerVerdict, - hasNewChanges, - ownerStepDoneStreak: nextOwnerStepDoneStreak, - emptyStepDoneStreak: nextEmptyStepDoneStreak, - summary: summary?.slice(0, 100), - }, - 'Owner marked the current output as an intermediate completed step — keeping task active for follow-up', - ); - return; - } - - if ( - nextOwnerStepDoneStreak !== (task.owner_step_done_streak ?? 0) || - nextEmptyStepDoneStreak !== (task.empty_step_done_streak ?? 0) - ) { + if (nextOwnerStepDoneStreak !== (task.owner_step_done_streak ?? 0)) { applyPairedTaskPatch({ taskId, expectedUpdatedAt: task.updated_at, updatedAt: now, patch: { owner_step_done_streak: nextOwnerStepDoneStreak, - empty_step_done_streak: nextEmptyStepDoneStreak, }, }); } diff --git a/src/paired-execution-context-shared.test.ts b/src/paired-execution-context-shared.test.ts index 3cb197a..c6ceddf 100644 --- a/src/paired-execution-context-shared.test.ts +++ b/src/paired-execution-context-shared.test.ts @@ -27,7 +27,7 @@ describe('paired execution context shared verdict helpers', () => { visibleVerdict: 'step_done', }), ).toEqual({ - kind: 'request_owner_continue', + kind: 'request_reviewer', resetStatusToActive: false, }); expect( @@ -57,7 +57,7 @@ describe('paired execution context shared verdict helpers', () => { deadlockThreshold: 2, }), ).toEqual({ - kind: 'request_owner_continue', + kind: 'request_reviewer', resetStatusToActive: true, }); expect( diff --git a/src/paired-execution-context-shared.ts b/src/paired-execution-context-shared.ts index 998e374..1ede0b1 100644 --- a/src/paired-execution-context-shared.ts +++ b/src/paired-execution-context-shared.ts @@ -9,7 +9,6 @@ import type { PairedTaskStatus } from './types.js'; export type CompletionSignal = | { kind: 'request_reviewer'; resetStatusToActive: boolean } | { kind: 'request_owner_finalize' } - | { kind: 'request_owner_continue'; resetStatusToActive: boolean } | { kind: 'request_owner_changes' } | { kind: 'request_arbiter' } | { kind: 'complete'; completionReason: 'done' | 'escalated' } @@ -38,12 +37,6 @@ export function resolveOwnerCompletionSignal(args: { } if (phase === 'normal') { - if (visibleVerdict === 'step_done') { - return { - kind: 'request_owner_continue', - resetStatusToActive: false, - }; - } return { kind: 'request_reviewer', resetStatusToActive: false, @@ -52,7 +45,7 @@ export function resolveOwnerCompletionSignal(args: { if (visibleVerdict === 'step_done') { return { - kind: 'request_owner_continue', + kind: 'request_reviewer', resetStatusToActive: true, }; } diff --git a/src/paired-execution-context.test.ts b/src/paired-execution-context.test.ts index c7a6df2..1c4ab42 100644 --- a/src/paired-execution-context.test.ts +++ b/src/paired-execution-context.test.ts @@ -802,13 +802,22 @@ describe('paired execution context', () => { ).not.toHaveBeenCalled(); }); - it('keeps an active task in owner follow-up mode when the owner reports STEP_DONE', () => { + it('requests review when the owner reports STEP_DONE in active mode', () => { vi.mocked(db.getPairedTaskById).mockReturnValue( buildPairedTask({ status: 'active', round_trip_count: 1, }), ); + vi.mocked(pairedWorkspaceManager.markPairedTaskReviewReady).mockReturnValue( + { + ownerWorkspace: buildWorkspace('owner', '/tmp/paired/task-1/owner'), + reviewerWorkspace: buildWorkspace( + 'reviewer', + '/tmp/paired/task-1/reviewer', + ), + }, + ); completePairedExecutionContext({ taskId: 'task-1', @@ -820,22 +829,17 @@ describe('paired execution context', () => { expect(db.updatePairedTask).toHaveBeenCalledWith( 'task-1', expect.objectContaining({ - owner_failure_count: 0, - owner_step_done_streak: 1, + round_trip_count: 2, + owner_step_done_streak: 0, + empty_step_done_streak: 0, }), ); expect( pairedWorkspaceManager.markPairedTaskReviewReady, - ).not.toHaveBeenCalled(); - expect(db.updatePairedTask).not.toHaveBeenCalledWith( - 'task-1', - expect.objectContaining({ - status: 'active', - }), - ); + ).toHaveBeenCalledWith('task-1'); }); - it('requests arbiter when active STEP_DONE repeats without code changes', () => { + it('requests review instead of arbiter when active STEP_DONE repeats without code changes', () => { vi.spyOn(config, 'isArbiterEnabled').mockReturnValue(true); const repoDir = createCanonicalRepoWithCommit('active step done loop'); @@ -851,6 +855,15 @@ describe('paired execution context', () => { vi.mocked(db.getPairedWorkspace).mockImplementation((_taskId, role) => role === 'owner' ? buildWorkspace('owner', repoDir) : undefined, ); + vi.mocked(pairedWorkspaceManager.markPairedTaskReviewReady).mockReturnValue( + { + ownerWorkspace: buildWorkspace('owner', repoDir), + reviewerWorkspace: buildWorkspace( + 'reviewer', + '/tmp/paired/task-1/reviewer', + ), + }, + ); completePairedExecutionContext({ taskId: 'task-1', @@ -862,23 +875,44 @@ describe('paired execution context', () => { expect(db.updatePairedTask).toHaveBeenCalledWith( 'task-1', expect.objectContaining({ - status: 'arbiter_requested', - owner_step_done_streak: 3, - empty_step_done_streak: 3, + round_trip_count: 1, + owner_step_done_streak: 0, + empty_step_done_streak: 0, }), ); expect( pairedWorkspaceManager.markPairedTaskReviewReady, - ).not.toHaveBeenCalled(); + ).toHaveBeenCalledWith('task-1'); + expect(db.updatePairedTask).not.toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'arbiter_requested', + }), + ); }); - it('returns merge_ready owner finalize output to active when the owner reports STEP_DONE', () => { + it('re-triggers review when the owner reports STEP_DONE during finalize', () => { + const repoDir = createCanonicalRepoWithCommit('reviewed'); + const approvedSourceRef = resolveTreeRef(repoDir); vi.mocked(db.getPairedTaskById).mockReturnValue( buildPairedTask({ status: 'merge_ready', + source_ref: approvedSourceRef, round_trip_count: 1, }), ); + vi.mocked(db.getPairedWorkspace).mockImplementation((_taskId, role) => + role === 'owner' ? buildWorkspace('owner', repoDir) : undefined, + ); + vi.mocked(pairedWorkspaceManager.markPairedTaskReviewReady).mockReturnValue( + { + ownerWorkspace: buildWorkspace('owner', repoDir), + reviewerWorkspace: buildWorkspace( + 'reviewer', + '/tmp/paired/task-1/reviewer', + ), + }, + ); completePairedExecutionContext({ taskId: 'task-1', @@ -893,11 +927,20 @@ describe('paired execution context', () => { status: 'active', owner_failure_count: 0, finalize_step_done_count: 1, + empty_step_done_streak: 1, + }), + ); + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + round_trip_count: 2, + finalize_step_done_count: 1, + empty_step_done_streak: 1, }), ); expect( pairedWorkspaceManager.markPairedTaskReviewReady, - ).not.toHaveBeenCalled(); + ).toHaveBeenCalledWith('task-1'); }); it('requests arbiter when finalize STEP_DONE repeats without code changes', () => { From 4e329a7717103d2bb44220851041ed6455905186 Mon Sep 17 00:00:00 2001 From: ejclaw Date: Sat, 25 Apr 2026 09:20:28 +0900 Subject: [PATCH 012/285] Add structured Discord attachments --- prompts/claude-platform.md | 22 +++ prompts/codex-platform.md | 27 +++ prompts/owner-common-platform.md | 27 +++ runners/agent-runner/src/output-protocol.ts | 4 +- runners/shared/src/agent-protocol.ts | 46 ++++- runners/shared/src/index.ts | 1 + runners/shared/test/agent-protocol.test.ts | 41 +++++ src/agent-output.ts | 10 + src/agent-protocol.ts | 1 + src/channels/discord.test.ts | 69 +++++++ src/channels/discord.ts | 117 ++++++++---- src/db.test.ts | 32 ++++ src/db/base-schema.ts | 1 + .../migrations/013_work-item-attachments.ts | 17 ++ src/db/migrations/index.ts | 2 + src/db/work-items.ts | 50 ++++- src/message-runtime-delivery.ts | 29 ++- src/message-runtime-turns.ts | 7 +- src/message-runtime.ts | 4 + src/message-turn-controller.test.ts | 56 ++++++ src/message-turn-controller.ts | 35 +++- src/outbound-attachments.test.ts | 149 +++++++++++++++ src/outbound-attachments.ts | 174 ++++++++++++++++++ src/types.ts | 23 ++- 24 files changed, 891 insertions(+), 53 deletions(-) create mode 100644 src/db/migrations/013_work-item-attachments.ts create mode 100644 src/outbound-attachments.test.ts create mode 100644 src/outbound-attachments.ts diff --git a/prompts/claude-platform.md b/prompts/claude-platform.md index 4373ff9..42a7abf 100644 --- a/prompts/claude-platform.md +++ b/prompts/claude-platform.md @@ -4,3 +4,25 @@ You have a `send_message` tool that sends a message immediately while you are st Use it to acknowledge a request before starting longer work. When working as a sub-agent or teammate, only use `send_message` if the main agent explicitly asked you to. + +## Image attachments + +When a locally generated image or screenshot should appear in Discord, return an EJClaw structured output with `attachments` instead of only writing the file path in prose: + +```json +{ + "ejclaw": { + "visibility": "public", + "text": "스크린샷을 첨부했습니다.", + "attachments": [ + { + "path": "/absolute/path/screenshot.png", + "name": "screenshot.png", + "mime": "image/png" + } + ] + } +} +``` + +Use absolute local paths only, and do not repeat the same path in the visible text. Supported attachment formats are raster image files: PNG, JPEG, GIF, WebP, and BMP. SVG is not accepted. diff --git a/prompts/codex-platform.md b/prompts/codex-platform.md index 612cef2..d2e07ca 100644 --- a/prompts/codex-platform.md +++ b/prompts/codex-platform.md @@ -28,9 +28,36 @@ Your output is sent directly to the Discord group. - Do not use generic recurring task registration from Codex - If the user wants a reminder or other non-CI recurring task, tell them to ask Claude/클코 to schedule it +## Image attachments + +When you need to show a locally generated image, screenshot, or other raster output in Discord, do not rely on a raw file path in prose. Emit an EJClaw structured output with `attachments`. + +```json +{ + "ejclaw": { + "visibility": "public", + "text": "이미지를 생성했습니다.", + "verdict": "done", + "attachments": [ + { + "path": "/absolute/path/image.png", + "name": "image.png", + "mime": "image/png" + } + ] + } +} +``` + +- `attachments.path` must be an absolute local path; URLs and relative paths are ignored +- Do not repeat the same file path in the visible text +- Supported attachment formats are raster image files: PNG, JPEG, GIF, WebP, and BMP. SVG is not accepted. +- Use this for generated images and e2e screenshots; the Discord channel validates and uploads the file + ## CI 감시 (watch_ci) GitHub Actions run 감시는 structured 필드를 우선 사용: + - ci_provider: "github", ci_repo: "owner/repo", ci_run_id: run ID - 이 조합 → host-driven fast path (LLM 토큰 소모 없음, 15초 polling) - structured 필드 없이 generic 등록 시 매 tick LLM 실행됨 diff --git a/prompts/owner-common-platform.md b/prompts/owner-common-platform.md index b217fc8..67204b8 100644 --- a/prompts/owner-common-platform.md +++ b/prompts/owner-common-platform.md @@ -20,9 +20,36 @@ Do not use markdown headings in chat replies. Keep messages clean and readable f The group folder may contain a `conversations/` directory with searchable history from earlier sessions. Use it when you need prior context. +## Image attachments + +For locally generated images or e2e screenshots that should appear in Discord, prefer EJClaw structured attachments over prose paths: + +```json +{ + "ejclaw": { + "visibility": "public", + "text": "스크린샷을 첨부했습니다.", + "verdict": "done", + "attachments": [ + { + "path": "/absolute/path/screenshot.png", + "name": "screenshot.png", + "mime": "image/png" + } + ] + } +} +``` + +- Use absolute local paths only +- Do not duplicate the same path in the visible text +- Supported attachment formats are raster image files: PNG, JPEG, GIF, WebP, and BMP. SVG is not accepted. +- The channel harness validates and uploads attachments; plain prose paths are not reliable + ## CI monitoring (watch_ci) GitHub Actions run monitoring uses structured fields first: + - ci_provider: "github", ci_repo: "owner/repo", ci_run_id: run ID - This combination → host-driven fast path (no LLM token cost, 15s polling) - Without structured fields → generic path, each tick runs LLM diff --git a/runners/agent-runner/src/output-protocol.ts b/runners/agent-runner/src/output-protocol.ts index 8d38f0d..f5ded31 100644 --- a/runners/agent-runner/src/output-protocol.ts +++ b/runners/agent-runner/src/output-protocol.ts @@ -3,7 +3,7 @@ import path from 'path'; import { extractImageTagPaths, - normalizePublicTextOutput, + normalizeEjclawStructuredOutput, type RunnerStructuredOutput, writeProtocolOutput, } from 'ejclaw-runners-shared'; @@ -151,7 +151,7 @@ export function normalizeStructuredOutput(result: string | null): { result: string | null; output?: RunnerOutput['output']; } { - return normalizePublicTextOutput(result); + return normalizeEjclawStructuredOutput(result); } export function extractAssistantText(message: unknown): string | null { diff --git a/runners/shared/src/agent-protocol.ts b/runners/shared/src/agent-protocol.ts index 89aa8f0..20a1494 100644 --- a/runners/shared/src/agent-protocol.ts +++ b/runners/shared/src/agent-protocol.ts @@ -1,7 +1,8 @@ export const OUTPUT_START_MARKER = '---EJCLAW_OUTPUT_START---'; export const OUTPUT_END_MARKER = '---EJCLAW_OUTPUT_END---'; -export const IMAGE_TAG_RE = /\[Image:\s*(\/[^\]]+)\]/g; +export const IMAGE_TAG_RE = + /\[Image:\s*(?:(?:[^\]\n]*?)\s*→\s*)?(\/[^\]\n]+)\]/g; export const IPC_POLL_MS = 500; export const IPC_INPUT_SUBDIR = 'input'; @@ -21,11 +22,18 @@ export type RunnerOutputVerdict = export type RunnerOutputVisibility = 'public' | 'silent'; +export interface RunnerOutputAttachment { + path: string; + name?: string; + mime?: string; +} + export type RunnerStructuredOutput = | { visibility: 'public'; text: string; verdict?: Exclude; + attachments?: RunnerOutputAttachment[]; } | { visibility: 'silent'; @@ -89,6 +97,33 @@ function isVisibleVerdict( ); } +function normalizeAttachments(value: unknown): RunnerOutputAttachment[] { + if (!Array.isArray(value)) return []; + + const attachments: RunnerOutputAttachment[] = []; + for (const item of value) { + if (!item || typeof item !== 'object' || Array.isArray(item)) continue; + const candidate = item as { + path?: unknown; + name?: unknown; + mime?: unknown; + }; + if (typeof candidate.path !== 'string' || candidate.path.length === 0) { + continue; + } + attachments.push({ + path: candidate.path, + ...(typeof candidate.name === 'string' && candidate.name.length > 0 + ? { name: candidate.name } + : {}), + ...(typeof candidate.mime === 'string' && candidate.mime.length > 0 + ? { mime: candidate.mime } + : {}), + }); + } + return attachments; +} + export function normalizeEjclawStructuredOutput( result: string | null, ): NormalizedRunnerOutput { @@ -99,7 +134,12 @@ export function normalizeEjclawStructuredOutput( const trimmed = result.trim(); try { const parsed = JSON.parse(trimmed) as { - ejclaw?: { visibility?: unknown; text?: unknown; verdict?: unknown }; + ejclaw?: { + visibility?: unknown; + text?: unknown; + verdict?: unknown; + attachments?: unknown; + }; }; const envelope = parsed?.ejclaw; if (envelope && typeof envelope === 'object' && !Array.isArray(envelope)) { @@ -128,6 +168,7 @@ export function normalizeEjclawStructuredOutput( ) { return normalizePublicTextOutput(result); } + const attachments = normalizeAttachments(envelope.attachments); return { result: envelope.text, output: { @@ -136,6 +177,7 @@ export function normalizeEjclawStructuredOutput( verdict: isVisibleVerdict(envelope.verdict) ? envelope.verdict : undefined, + ...(attachments.length > 0 ? { attachments } : {}), }, }; } diff --git a/runners/shared/src/index.ts b/runners/shared/src/index.ts index eb7cd92..8e432d1 100644 --- a/runners/shared/src/index.ts +++ b/runners/shared/src/index.ts @@ -15,6 +15,7 @@ export { writeProtocolOutput, type NormalizedRunnerOutput, type RunnerOutputPhase, + type RunnerOutputAttachment, type RunnerOutputVerdict, type RunnerOutputVisibility, type RunnerStructuredOutput, diff --git a/runners/shared/test/agent-protocol.test.ts b/runners/shared/test/agent-protocol.test.ts index 37adb62..74e631f 100644 --- a/runners/shared/test/agent-protocol.test.ts +++ b/runners/shared/test/agent-protocol.test.ts @@ -13,6 +13,12 @@ describe('shared agent protocol helpers', () => { cleanText: 'hello', imagePaths: ['/tmp/a.png'], }); + expect( + extractImageTagPaths('hello [Image: screenshot.png → /tmp/a.png]'), + ).toEqual({ + cleanText: 'hello', + imagePaths: ['/tmp/a.png'], + }); expect(extractImageTagPaths('[Image: /tmp/b.png] second')).toEqual({ cleanText: 'second', imagePaths: ['/tmp/b.png'], @@ -45,6 +51,41 @@ describe('shared agent protocol helpers', () => { }); }); + it('parses public ejclaw attachments', () => { + expect( + normalizeEjclawStructuredOutput( + JSON.stringify({ + ejclaw: { + visibility: 'public', + text: '이미지를 생성했습니다.', + verdict: 'done', + attachments: [ + { + path: '/tmp/image.png', + name: 'image.png', + mime: 'image/png', + }, + ], + }, + }), + ), + ).toEqual({ + result: '이미지를 생성했습니다.', + output: { + visibility: 'public', + text: '이미지를 생성했습니다.', + verdict: 'done', + attachments: [ + { + path: '/tmp/image.png', + name: 'image.png', + mime: 'image/png', + }, + ], + }, + }); + }); + it('falls back to visible raw text on invalid public verdicts', () => { const raw = JSON.stringify({ ejclaw: { diff --git a/src/agent-output.ts b/src/agent-output.ts index 1d7e8d9..d8fb478 100644 --- a/src/agent-output.ts +++ b/src/agent-output.ts @@ -34,6 +34,16 @@ export function getAgentOutputText(output: { return stringifyLegacyAgentResult(output.result); } +export function getAgentOutputAttachments(output: { + output?: StructuredAgentOutput; +}): NonNullable< + Extract['attachments'] +> { + const structured = getStructuredAgentOutput(output); + if (structured?.visibility !== 'public') return []; + return structured.attachments ?? []; +} + export function hasAgentOutputPayload(output: { output?: StructuredAgentOutput; result?: string | object | null; diff --git a/src/agent-protocol.ts b/src/agent-protocol.ts index 53db438..17daa32 100644 --- a/src/agent-protocol.ts +++ b/src/agent-protocol.ts @@ -10,6 +10,7 @@ export { OUTPUT_START_MARKER, writeProtocolOutput, type NormalizedRunnerOutput, + type RunnerOutputAttachment, type RunnerOutputPhase, type RunnerOutputVerdict, type RunnerOutputVisibility, diff --git a/src/channels/discord.test.ts b/src/channels/discord.test.ts index 1171424..277098f 100644 --- a/src/channels/discord.test.ts +++ b/src/channels/discord.test.ts @@ -1,3 +1,7 @@ +import fs from 'fs'; +import os from 'os'; +import path from 'path'; + import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest'; // --- Mocks --- @@ -127,6 +131,18 @@ import { logger } from '../logger.js'; // --- Test helpers --- +const ONE_PIXEL_PNG = Buffer.from( + 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAwMCAO+/p9sAAAAASUVORK5CYII=', + 'base64', +); + +function createTempPng(name = 'image.png'): { dir: string; filePath: string } { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'ejclaw-discord-image-')); + const filePath = path.join(dir, name); + fs.writeFileSync(filePath, ONE_PIXEL_PNG); + return { dir, filePath }; +} + function createTestOpts( overrides?: Partial, ): DiscordChannelOpts { @@ -839,6 +855,59 @@ describe('DiscordChannel', () => { }); }); + it('sends structured attachments as Discord files', async () => { + const opts = createTestOpts(); + const channel = new DiscordChannel('test-token', opts); + await channel.connect(); + const { dir, filePath } = createTempPng('structured.png'); + const mockChannel = { + send: vi.fn().mockResolvedValue({ id: 'discord-message-1' }), + sendTyping: vi.fn(), + }; + currentClient().channels.fetch.mockResolvedValue(mockChannel); + + await channel.sendMessage( + 'dc:1234567890123456', + '이미지를 생성했습니다.', + { + attachments: [ + { path: filePath, name: 'result.png', mime: 'image/png' }, + ], + }, + ); + + expect(mockChannel.send).toHaveBeenCalledWith({ + content: '이미지를 생성했습니다.', + files: [{ attachment: filePath, name: 'result.png' }], + flags: 1 << 2, + }); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('uses legacy image tags as Discord attachment fallback', async () => { + const opts = createTestOpts(); + const channel = new DiscordChannel('test-token', opts); + await channel.connect(); + const { dir, filePath } = createTempPng('screenshot.png'); + const mockChannel = { + send: vi.fn().mockResolvedValue({ id: 'discord-message-1' }), + sendTyping: vi.fn(), + }; + currentClient().channels.fetch.mockResolvedValue(mockChannel); + + await channel.sendMessage( + 'dc:1234567890123456', + `스크린샷입니다.\n[Image: screenshot.png → ${filePath}]`, + ); + + expect(mockChannel.send).toHaveBeenCalledWith({ + content: '스크린샷입니다.', + files: [{ attachment: filePath, name: 'screenshot.png' }], + flags: 1 << 2, + }); + fs.rmSync(dir, { recursive: true, force: true }); + }); + it('logs channel name and Discord message ids after sending', async () => { const opts = createTestOpts(); const channel = new DiscordChannel('test-token', opts); diff --git a/src/channels/discord.ts b/src/channels/discord.ts index 75bcaad..84a64f9 100644 --- a/src/channels/discord.ts +++ b/src/channels/discord.ts @@ -19,8 +19,11 @@ import { } from '../config.js'; import { getEnv } from '../env.js'; import { logger } from '../logger.js'; +import { extractImageTagPaths } from '../agent-protocol.js'; +import { validateOutboundAttachments } from '../outbound-attachments.js'; import { formatOutbound } from '../router.js'; import { hasReviewerLease } from '../service-routing.js'; +import type { OutboundAttachment, SendMessageOptions } from '../types.js'; const ATTACHMENTS_DIR = path.join(DATA_DIR, 'attachments'); const TRANSCRIPTION_CACHE_DIR = path.join(CACHE_DIR, 'transcriptions'); @@ -30,6 +33,45 @@ const DISCORD_ARBITER_CHANNEL = 'discord-arbiter'; const DISCORD_OWNER_TOKEN_KEY = 'DISCORD_OWNER_BOT_TOKEN'; const DISCORD_REVIEWER_TOKEN_KEY = 'DISCORD_REVIEWER_BOT_TOKEN'; const DISCORD_ARBITER_TOKEN_KEY = 'DISCORD_ARBITER_BOT_TOKEN'; +const IMAGE_EXTS = /\.(png|jpe?g|gif|webp|bmp)$/i; +const MD_LINK_RE = /\[[^\]]*\]\((\/[^)]+)\)/g; + +function extractMarkdownImageAttachments(text: string): { + cleanText: string; + attachments: OutboundAttachment[]; +} { + const attachments: OutboundAttachment[] = []; + const seen = new Set(); + + const cleanText = text.replace(MD_LINK_RE, (_full, rawPath: string) => { + const trimmed = rawPath.trim(); + if (IMAGE_EXTS.test(trimmed)) { + if (!seen.has(trimmed)) { + attachments.push({ + path: trimmed, + name: path.basename(trimmed), + }); + seen.add(trimmed); + } + return ''; + } + + const basename = path.basename(trimmed.replace(/#.*$/, '')); + const lineMatch = trimmed.match(/#L(\d+)/); + return lineMatch ? `\`${basename}:${lineMatch[1]}\`` : `\`${basename}\``; + }); + + return { cleanText, attachments }; +} + +function imageTagPathsToAttachments(paths: string[]): OutboundAttachment[] { + return paths + .filter((filePath) => IMAGE_EXTS.test(filePath)) + .map((filePath) => ({ + path: filePath, + name: path.basename(filePath), + })); +} /** * Download a Discord attachment to local disk. @@ -403,7 +445,11 @@ export class DiscordChannel implements Channel { }); } - async sendMessage(jid: string, text: string): Promise { + async sendMessage( + jid: string, + text: string, + options: SendMessageOptions = {}, + ): Promise { if (!this.client) { logger.warn('Discord client not initialized'); return; @@ -420,37 +466,43 @@ export class DiscordChannel implements Channel { const textChannel = channel as TextChannel; - // Extract image attachments from markdown links with image extensions - // e.g. [name.png](/absolute/path/name.png) - const IMAGE_EXTS = /\.(png|jpe?g|gif|webp|svg|bmp)$/i; - const MD_LINK_RE = /\[[^\]]*\]\((\/[^)]+)\)/g; - const imageFiles: string[] = []; - const seen = new Set(); - let match; + const structuredAttachments = options.attachments ?? []; + const hasStructuredAttachments = structuredAttachments.length > 0; + const markdownExtracted = extractMarkdownImageAttachments(text); + const imageTagExtracted = extractImageTagPaths( + markdownExtracted.cleanText, + ); + const legacyImageTagAttachments = imageTagPathsToAttachments( + imageTagExtracted.imagePaths, + ); + const outboundAttachments = hasStructuredAttachments + ? structuredAttachments + : [...markdownExtracted.attachments, ...legacyImageTagAttachments]; + const attachmentSource = hasStructuredAttachments + ? 'structured' + : markdownExtracted.attachments.length > 0 + ? 'md-link' + : legacyImageTagAttachments.length > 0 + ? 'image-tag' + : 'none'; + const validation = validateOutboundAttachments(outboundAttachments, { + baseDirs: options.attachmentBaseDirs, + }); + const files = validation.files; - while ((match = MD_LINK_RE.exec(text)) !== null) { - const imgPath = match[1].trim(); - if ( - !seen.has(imgPath) && - IMAGE_EXTS.test(imgPath) && - fs.existsSync(imgPath) - ) { - imageFiles.push(imgPath); - seen.add(imgPath); - } + if (validation.rejected.length > 0) { + logger.warn( + { + jid, + channelName: this.name, + attachmentSource, + rejected: validation.rejected, + }, + 'Rejected outbound Discord attachments', + ); } - let cleaned = text - .replace(MD_LINK_RE, (full, p, _offset, _str, groups) => { - const trimmed = p.trim(); - // Image links: remove entirely (attached as files) - if (IMAGE_EXTS.test(trimmed) && seen.has(trimmed)) return ''; - // Non-image local path links: convert to readable filename - const basename = path.basename(trimmed.replace(/#.*$/, '')); - const lineMatch = trimmed.match(/#L(\d+)/); - return lineMatch - ? `\`${basename}:${lineMatch[1]}\`` - : `\`${basename}\``; - }) + + let cleaned = imageTagExtracted.cleanText .replace(/^[ \t]*[•\-\*][ \t]*$/gm, '') // remove empty bullet lines .replace(/\n{3,}/g, '\n\n') // collapse excessive blank lines .trim(); @@ -467,10 +519,6 @@ export class DiscordChannel implements Channel { // Discord has a 2000 character limit per message and 10 attachments per message const MAX_LENGTH = 2000; const MAX_ATTACHMENTS = 10; - const files = imageFiles.map((f) => ({ - attachment: f, - name: path.basename(f), - })); const sentMessageIds: string[] = []; let chunkCount = 0; @@ -544,6 +592,7 @@ export class DiscordChannel implements Channel { deliveryMode: 'send', chunkCount, attachmentCount: files.length, + attachmentSource, messageId: sentMessageIds[0] ?? null, messageIds: sentMessageIds, botUserId: this.client.user?.id ?? null, diff --git a/src/db.test.ts b/src/db.test.ts index 0a15fc6..8a5a3f0 100644 --- a/src/db.test.ts +++ b/src/db.test.ts @@ -9106,6 +9106,38 @@ describe('work items', () => { ).toBeUndefined(); }); + it('stores produced work item attachments for delivery retries', () => { + const item = createProducedWorkItem({ + group_folder: 'discord_test', + chat_jid: 'dc:attachments', + agent_type: 'claude-code', + delivery_role: 'owner', + start_seq: 1, + end_seq: 2, + result_payload: 'image ready', + attachments: [ + { + path: '/tmp/image.png', + name: 'image.png', + mime: 'image/png', + }, + ], + }); + + const stored = getOpenWorkItem( + 'dc:attachments', + 'claude-code', + item.service_id, + ); + expect(stored?.attachments).toEqual([ + { + path: '/tmp/image.png', + name: 'image.png', + mime: 'image/png', + }, + ]); + }); + it('finds pending delivery retries even when they were created by a fallback agent type', () => { const fallbackItem = createProducedWorkItem({ group_folder: 'discord_test', diff --git a/src/db/base-schema.ts b/src/db/base-schema.ts index d44e693..e4fd949 100644 --- a/src/db/base-schema.ts +++ b/src/db/base-schema.ts @@ -38,6 +38,7 @@ export function applyBaseSchema(database: Database): void { start_seq INTEGER, end_seq INTEGER, result_payload TEXT NOT NULL, + attachment_payload TEXT, delivery_attempts INTEGER NOT NULL DEFAULT 0, delivery_message_id TEXT, last_error TEXT, diff --git a/src/db/migrations/013_work-item-attachments.ts b/src/db/migrations/013_work-item-attachments.ts new file mode 100644 index 0000000..5bc7b26 --- /dev/null +++ b/src/db/migrations/013_work-item-attachments.ts @@ -0,0 +1,17 @@ +import type { Database } from 'bun:sqlite'; + +import { tableHasColumn } from './helpers.js'; +import type { SchemaMigrationDefinition } from './types.js'; + +export const WORK_ITEM_ATTACHMENTS_MIGRATION: SchemaMigrationDefinition = { + version: 13, + name: 'work_item_attachments', + apply(database: Database) { + if (!tableHasColumn(database, 'work_items', 'attachment_payload')) { + database.exec(` + ALTER TABLE work_items + ADD COLUMN attachment_payload TEXT + `); + } + }, +}; diff --git a/src/db/migrations/index.ts b/src/db/migrations/index.ts index d6b5e83..2c8567f 100644 --- a/src/db/migrations/index.ts +++ b/src/db/migrations/index.ts @@ -12,6 +12,7 @@ import { PAIRED_WORKSPACE_PROJECT_SCHEMA_CLEANUP_MIGRATION } from './009_paired- import { PAIRED_TURN_PROVENANCE_UPGRADE_MIGRATION } from './010_paired-turn-provenance-upgrade.js'; import { OWNER_FAILURE_COUNT_MIGRATION } from './011_owner-failure-count.js'; import { PAIRED_VERDICT_AND_STEP_TELEMETRY_MIGRATION } from './012_paired-verdict-and-step-telemetry.js'; +import { WORK_ITEM_ATTACHMENTS_MIGRATION } from './013_work-item-attachments.js'; import type { SchemaMigrationArgs, SchemaMigrationDefinition, @@ -32,6 +33,7 @@ const ORDERED_SCHEMA_MIGRATIONS: readonly SchemaMigrationDefinition[] = [ PAIRED_TURN_PROVENANCE_UPGRADE_MIGRATION, OWNER_FAILURE_COUNT_MIGRATION, PAIRED_VERDICT_AND_STEP_TELEMETRY_MIGRATION, + WORK_ITEM_ATTACHMENTS_MIGRATION, ]; function ensureSchemaMigrationsTable(database: Database): void { diff --git a/src/db/work-items.ts b/src/db/work-items.ts index f5af722..8a9e51c 100644 --- a/src/db/work-items.ts +++ b/src/db/work-items.ts @@ -6,7 +6,7 @@ import { inferRoleFromServiceShadow, resolveRoleServiceShadow, } from '../role-service-shadow.js'; -import { AgentType, PairedRoomRole } from '../types.js'; +import { AgentType, OutboundAttachment, PairedRoomRole } from '../types.js'; export interface WorkItem { id: number; @@ -19,6 +19,7 @@ export interface WorkItem { start_seq: number | null; end_seq: number | null; result_payload: string; + attachments?: OutboundAttachment[]; delivery_attempts: number; delivery_message_id: string | null; last_error: string | null; @@ -29,10 +30,11 @@ export interface WorkItem { interface StoredWorkItemRow extends Omit< WorkItem, - 'agent_type' | 'service_id' + 'agent_type' | 'service_id' | 'attachments' > { agent_type: string; service_id?: string | null; + attachment_payload?: string | null; } export interface CreateProducedWorkItemInput { @@ -44,6 +46,7 @@ export interface CreateProducedWorkItemInput { start_seq: number | null; end_seq: number | null; result_payload: string; + attachments?: OutboundAttachment[]; } function normalizeStoredAgentType( @@ -96,9 +99,48 @@ function hydrateWorkItemRow(row: StoredWorkItemRow): WorkItem { ...row, agent_type: agentType, service_id: readStoredWorkItemServiceId(row), + attachments: parseAttachmentPayload(row.attachment_payload), }; } +function parseAttachmentPayload( + payload: string | null | undefined, +): OutboundAttachment[] { + if (!payload) return []; + try { + const parsed = JSON.parse(payload) as unknown; + if (!Array.isArray(parsed)) return []; + return parsed + .filter( + (item): item is OutboundAttachment => + item !== null && + typeof item === 'object' && + !Array.isArray(item) && + typeof (item as { path?: unknown }).path === 'string', + ) + .map((item) => ({ + path: item.path, + ...(typeof item.name === 'string' ? { name: item.name } : {}), + ...(typeof item.mime === 'string' ? { mime: item.mime } : {}), + })); + } catch { + return []; + } +} + +function serializeAttachmentPayload( + attachments: OutboundAttachment[] | undefined, +): string | null { + if (!attachments?.length) return null; + return JSON.stringify( + attachments.map((attachment) => ({ + path: attachment.path, + ...(attachment.name ? { name: attachment.name } : {}), + ...(attachment.mime ? { mime: attachment.mime } : {}), + })), + ); +} + function resolvePreferredWorkItemRole( serviceId: string | null | undefined, ): PairedRoomRole | null { @@ -220,10 +262,11 @@ export function createProducedWorkItemInDatabase( start_seq, end_seq, result_payload, + attachment_payload, delivery_attempts, created_at, updated_at - ) VALUES (?, ?, ?, ?, ?, 'produced', ?, ?, ?, 0, ?, ?)`, + ) VALUES (?, ?, ?, ?, ?, 'produced', ?, ?, ?, ?, 0, ?, ?)`, ) .run( input.group_folder, @@ -234,6 +277,7 @@ export function createProducedWorkItemInDatabase( input.start_seq, input.end_seq, input.result_payload, + serializeAttachmentPayload(input.attachments), now, now, ); diff --git a/src/message-runtime-delivery.ts b/src/message-runtime-delivery.ts index d64ac34..4d01962 100644 --- a/src/message-runtime-delivery.ts +++ b/src/message-runtime-delivery.ts @@ -28,11 +28,14 @@ export async function deliverOpenWorkItem(args: { channel: Channel; item: WorkItem; log: RuntimeDeliveryLog; + attachmentBaseDirs?: string[]; replaceMessageId?: string | null; isDuplicateOfLastBotFinal: (chatJid: string, text: string) => boolean; openContinuation: (chatJid: string) => void; }): Promise { const replaceMessageId = args.replaceMessageId ?? null; + const attachments = args.item.attachments ?? []; + const hasAttachments = attachments.length > 0; const isDuplicate = args.isDuplicateOfLastBotFinal( args.item.chat_jid, @@ -52,7 +55,7 @@ export async function deliverOpenWorkItem(args: { } try { - if (replaceMessageId && args.channel.editMessage) { + if (replaceMessageId && args.channel.editMessage && !hasAttachments) { args.log.info( buildDeliveryLogContext(args.channel, args.item, { deliveryAttempts: args.item.delivery_attempts + 1, @@ -93,19 +96,32 @@ export async function deliverOpenWorkItem(args: { try { args.log.info( buildDeliveryLogContext(args.channel, args.item, { + attachmentCount: attachments.length, deliveryAttempts: args.item.delivery_attempts + 1, deliveryMode: 'send', }), 'Attempting to deliver produced work item as a new message', ); - await args.channel.sendMessage( - args.item.chat_jid, - args.item.result_payload, - ); + if (hasAttachments) { + await args.channel.sendMessage( + args.item.chat_jid, + args.item.result_payload, + { + attachmentBaseDirs: args.attachmentBaseDirs, + attachments, + }, + ); + } else { + await args.channel.sendMessage( + args.item.chat_jid, + args.item.result_payload, + ); + } markWorkItemDelivered(args.item.id); args.openContinuation(args.item.chat_jid); args.log.info( buildDeliveryLogContext(args.channel, args.item, { + attachmentCount: attachments.length, deliveryAttempts: args.item.delivery_attempts + 1, deliveryMode: 'send', }), @@ -117,6 +133,7 @@ export async function deliverOpenWorkItem(args: { markWorkItemDeliveryRetry(args.item.id, errorMessage); args.log.warn( buildDeliveryLogContext(args.channel, args.item, { + attachmentCount: attachments.length, deliveryAttempts: args.item.delivery_attempts + 1, deliveryMode: 'send', err, @@ -135,6 +152,7 @@ export async function processOpenWorkItemDelivery(args: { channel: Channel; roleToChannel: Record<'owner' | 'reviewer' | 'arbiter', Channel | null>; log: RuntimeDeliveryLog; + attachmentBaseDirs?: string[]; isPairedRoom: boolean; getMissingRoleChannelMessage: (role: 'reviewer' | 'arbiter') => string; isDuplicateOfLastBotFinal: (chatJid: string, text: string) => boolean; @@ -186,6 +204,7 @@ export async function processOpenWorkItemDelivery(args: { channel: deliveryChannel, item: openWorkItem, log: args.log, + attachmentBaseDirs: args.attachmentBaseDirs, isDuplicateOfLastBotFinal: args.isDuplicateOfLastBotFinal, openContinuation: args.openContinuation, }); diff --git a/src/message-runtime-turns.ts b/src/message-runtime-turns.ts index 5e2bce3..237de10 100644 --- a/src/message-runtime-turns.ts +++ b/src/message-runtime-turns.ts @@ -11,9 +11,10 @@ import { resolvePairedTurnRunOwnership } from './paired-turn-run-ownership.js'; import { normalizeMessageForDedupe } from './router.js'; import type { ExecuteTurnFn } from './message-runtime-types.js'; import type { + AgentType, Channel, NewMessage, - AgentType, + OutboundAttachment, PairedRoomRole, RegisteredGroup, } from './types.js'; @@ -100,6 +101,7 @@ interface CreateExecuteTurnDeps { clearSession: (groupFolder: string, opts?: { allRoles?: boolean }) => void; deliverFinalText: (args: { text: string; + attachments?: OutboundAttachment[]; chatJid: string; runId: string; channel: Channel; @@ -202,6 +204,9 @@ export function createExecuteTurn(deps: CreateExecuteTurnDeps): ExecuteTurnFn { try { return await deps.deliverFinalText({ text, + ...(options?.attachments?.length + ? { attachments: options.attachments } + : {}), chatJid, runId, channel, diff --git a/src/message-runtime.ts b/src/message-runtime.ts index bea78ab..b2c197a 100644 --- a/src/message-runtime.ts +++ b/src/message-runtime.ts @@ -210,6 +210,7 @@ export function createMessageRuntime(deps: MessageRuntimeDeps): { clearSession: deps.clearSession, deliverFinalText: async ({ text, + attachments, chatJid, runId, channel, @@ -248,11 +249,13 @@ export function createMessageRuntime(deps: MessageRuntimeDeps): { start_seq: startSeq, end_seq: endSeq, result_payload: text, + attachments, }); return deliverOpenWorkItem({ channel, item: workItem, log: logger, + attachmentBaseDirs: group.workDir ? [group.workDir] : undefined, replaceMessageId, isDuplicateOfLastBotFinal: checkDuplicateOfLastBotFinal, openContinuation: (targetChatJid) => @@ -455,6 +458,7 @@ export function createMessageRuntime(deps: MessageRuntimeDeps): { channel, roleToChannel, log, + attachmentBaseDirs: group.workDir ? [group.workDir] : undefined, isPairedRoom: hasReviewerLease(chatJid), getMissingRoleChannelMessage, isDuplicateOfLastBotFinal: checkDuplicateOfLastBotFinal, diff --git a/src/message-turn-controller.test.ts b/src/message-turn-controller.test.ts index 7137190..ecc686a 100644 --- a/src/message-turn-controller.test.ts +++ b/src/message-turn-controller.test.ts @@ -317,6 +317,62 @@ describe('MessageTurnController outbound audit logging', () => { ); }); + it('passes structured final attachments to final delivery', async () => { + const channel = makeChannel(); + const deliverFinalText = vi.fn().mockResolvedValue(true); + const attachments = [ + { + path: '/tmp/e2e-screenshot.png', + name: 'e2e-screenshot.png', + mime: 'image/png', + }, + ]; + const controller = new MessageTurnController({ + chatJid: 'dc:test-room', + group: makeGroup(), + runId: 'run-review-attachments', + channel, + idleTimeout: 1_000, + failureFinalText: '실패', + isClaudeCodeAgent: true, + clearSession: vi.fn(), + requestClose: vi.fn(), + deliverFinalText, + deliveryRole: 'reviewer', + deliveryServiceId: 'codex-review', + pairedTurnIdentity: makeTurnIdentity(), + }); + + await controller.start(); + await controller.handleOutput({ + status: 'success', + phase: 'final', + result: '스크린샷을 첨부했습니다.', + output: { + visibility: 'public', + text: '스크린샷을 첨부했습니다.', + attachments, + }, + } as any); + await controller.finish('success'); + + expect(deliverFinalText).toHaveBeenCalledWith( + '스크린샷을 첨부했습니다.', + { + replaceMessageId: null, + attachments, + }, + ); + expect(getAuditEntries()).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + auditEvent: 'final-delivery-attempt', + attachmentCount: 1, + }), + ]), + ); + }); + it('replaces the tracked progress message when an owner final arrives', async () => { const channel = { ...makeChannel(), diff --git a/src/message-turn-controller.ts b/src/message-turn-controller.ts index 31424a4..ba0b6fd 100644 --- a/src/message-turn-controller.ts +++ b/src/message-turn-controller.ts @@ -1,5 +1,8 @@ import { type AgentOutput } from './agent-runner.js'; -import { getAgentOutputText } from './agent-output.js'; +import { + getAgentOutputAttachments, + getAgentOutputText, +} from './agent-output.js'; import { createScopedLogger, logger } from './logger.js'; import { formatOutbound } from './router.js'; import { shouldResetSessionOnAgentFailure } from './session-recovery.js'; @@ -11,6 +14,7 @@ import { toVisiblePhase, type AgentOutputPhase, type Channel, + type OutboundAttachment, type PairedRoomRole, type RegisteredGroup, type VisiblePhase, @@ -35,7 +39,10 @@ interface MessageTurnControllerOptions { requestClose: (reason: string) => void; deliverFinalText: ( text: string, - options?: { replaceMessageId?: string | null }, + options?: { + attachments?: OutboundAttachment[]; + replaceMessageId?: string | null; + }, ) => Promise; canDeliverFinalText?: () => boolean; allowProgressReplayWithoutFinal?: boolean; @@ -160,6 +167,7 @@ export class MessageTurnController { const raw = getAgentOutputText(result); const text = raw ? formatOutbound(raw) : null; + const attachments = getAgentOutputAttachments(result); if (raw) { this.log.info( @@ -297,6 +305,7 @@ export class MessageTurnController { // then discard the pending buffer so it never shows up. if (text) { await this.publishTerminalText(text, { + attachments, flushPendingText: text, }); } else if (raw) { @@ -648,14 +657,22 @@ export class MessageTurnController { private async publishTerminalText( text: string, - options?: { flushPendingText?: string | null }, + options?: { + attachments?: OutboundAttachment[]; + flushPendingText?: string | null; + }, ): Promise { if (options?.flushPendingText) { await this.flushPendingProgress(options.flushPendingText); } const replaceMessageId = this.consumeProgressForFinalDelivery(); - await this.deliverFinalText(text, { replaceMessageId }); + await this.deliverFinalText(text, { + ...(options?.attachments?.length + ? { attachments: options.attachments } + : {}), + replaceMessageId, + }); } private consumeProgressForFinalDelivery(): string | null { @@ -675,7 +692,10 @@ export class MessageTurnController { private async deliverFinalText( text: string, - options?: { replaceMessageId?: string | null }, + options?: { + attachments?: OutboundAttachment[]; + replaceMessageId?: string | null; + }, ): Promise { await this.activateTyping('turn:deliver-final'); this.visiblePhase = toVisiblePhase('final'); @@ -697,14 +717,19 @@ export class MessageTurnController { return; } this.logOutboundAudit('final-delivery-attempt', { + attachmentCount: options?.attachments?.length ?? 0, messageId: replaceMessageId, textLength: text.length, deliveryMode: replaceMessageId ? 'edit' : 'send', }); const delivered = await this.options.deliverFinalText(text, { + ...(options?.attachments?.length + ? { attachments: options.attachments } + : {}), replaceMessageId, }); this.logOutboundAudit('final-delivery-result', { + attachmentCount: options?.attachments?.length ?? 0, messageId: replaceMessageId, textLength: text.length, deliveryMode: replaceMessageId ? 'edit' : 'send', diff --git a/src/outbound-attachments.test.ts b/src/outbound-attachments.test.ts new file mode 100644 index 0000000..85c0fa3 --- /dev/null +++ b/src/outbound-attachments.test.ts @@ -0,0 +1,149 @@ +import fs from 'fs'; +import os from 'os'; +import path from 'path'; + +import { afterEach, describe, expect, it } from 'vitest'; + +import { validateOutboundAttachments } from './outbound-attachments.js'; + +const ONE_PIXEL_PNG = Buffer.from( + 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAwMCAO+/p9sAAAAASUVORK5CYII=', + 'base64', +); + +const cleanupDirs: string[] = []; + +function makeTempDir(baseDir: string, prefix: string): string { + const dir = fs.mkdtempSync(path.join(baseDir, prefix)); + cleanupDirs.push(dir); + return dir; +} + +function writeFile( + dir: string, + name: string, + content: Buffer | string, +): string { + const filePath = path.join(dir, name); + fs.writeFileSync(filePath, content); + return filePath; +} + +afterEach(() => { + for (const dir of cleanupDirs.splice(0)) { + fs.rmSync(dir, { force: true, recursive: true }); + } +}); + +describe('validateOutboundAttachments', () => { + it('accepts real image files under default attachment directories', () => { + const dir = makeTempDir(os.tmpdir(), 'ejclaw-attachment-'); + const imagePath = writeFile(dir, 'screenshot.png', ONE_PIXEL_PNG); + + const result = validateOutboundAttachments([ + { + path: imagePath, + name: '../unsafe-name.png', + mime: 'image/png', + }, + ]); + + expect(result.rejected).toEqual([]); + expect(result.files).toEqual([ + { + attachment: fs.realpathSync(imagePath), + name: 'unsafe-name.png', + }, + ]); + }); + + it('requires workspace paths to be explicitly allowlisted', () => { + const dir = makeTempDir(process.cwd(), '.ejclaw-attachment-'); + const imagePath = writeFile(dir, 'workspace-shot.png', ONE_PIXEL_PNG); + + expect(validateOutboundAttachments([{ path: imagePath }])).toEqual({ + files: [], + rejected: [{ path: imagePath, reason: 'outside-allowed-dirs' }], + }); + + const allowed = validateOutboundAttachments([{ path: imagePath }], { + baseDirs: [dir], + }); + + expect(allowed.rejected).toEqual([]); + expect(allowed.files).toEqual([ + { + attachment: fs.realpathSync(imagePath), + name: 'workspace-shot.png', + }, + ]); + }); + + it('rejects symlink attempts that escape the allowed directory', () => { + const workspaceDir = makeTempDir(process.cwd(), '.ejclaw-attachment-'); + const targetPath = writeFile( + workspaceDir, + 'secret-shot.png', + ONE_PIXEL_PNG, + ); + const tmpDir = makeTempDir(os.tmpdir(), 'ejclaw-attachment-'); + const linkPath = path.join(tmpDir, 'linked-shot.png'); + fs.symlinkSync(targetPath, linkPath); + + const result = validateOutboundAttachments([{ path: linkPath }]); + + expect(result.files).toEqual([]); + expect(result.rejected).toEqual([ + { path: linkPath, reason: 'outside-allowed-dirs' }, + ]); + }); + + it('rejects SVG attachments before inspecting file content', () => { + const dir = makeTempDir(os.tmpdir(), 'ejclaw-attachment-'); + const svgPath = writeFile(dir, 'vector.svg', ''); + + const result = validateOutboundAttachments([{ path: svgPath }]); + + expect(result.files).toEqual([]); + expect(result.rejected).toEqual([ + { path: svgPath, reason: 'unsupported-extension' }, + ]); + }); + + it('rejects files whose extension and image signature do not match policy', () => { + const dir = makeTempDir(os.tmpdir(), 'ejclaw-attachment-'); + const fakePng = writeFile(dir, 'fake.png', 'not an image'); + const realPng = writeFile(dir, 'real.png', ONE_PIXEL_PNG); + + expect(validateOutboundAttachments([{ path: fakePng }])).toEqual({ + files: [], + rejected: [{ path: fakePng, reason: 'invalid-image-signature' }], + }); + expect( + validateOutboundAttachments([{ path: realPng, mime: 'image/jpeg' }]), + ).toEqual({ + files: [], + rejected: [{ path: realPng, reason: 'mime-mismatch' }], + }); + }); + + it('rejects non-files and files over the size cap', () => { + const dir = makeTempDir(os.tmpdir(), 'ejclaw-attachment-'); + const nestedDir = path.join(dir, 'nested.png'); + fs.mkdirSync(nestedDir); + const largePng = writeFile( + dir, + 'large.png', + Buffer.concat([ONE_PIXEL_PNG, Buffer.alloc(8 * 1024 * 1024)]), + ); + + expect(validateOutboundAttachments([{ path: nestedDir }])).toEqual({ + files: [], + rejected: [{ path: nestedDir, reason: 'not-file' }], + }); + expect(validateOutboundAttachments([{ path: largePng }])).toEqual({ + files: [], + rejected: [{ path: largePng, reason: 'too-large' }], + }); + }); +}); diff --git a/src/outbound-attachments.ts b/src/outbound-attachments.ts new file mode 100644 index 0000000..fc7feb7 --- /dev/null +++ b/src/outbound-attachments.ts @@ -0,0 +1,174 @@ +import fs from 'fs'; +import path from 'path'; + +import { DATA_DIR } from './config.js'; +import type { OutboundAttachment } from './types.js'; + +export interface ValidatedOutboundAttachment { + attachment: string; + name: string; +} + +export interface ValidateOutboundAttachmentsResult { + files: ValidatedOutboundAttachment[]; + rejected: Array<{ path: string; reason: string }>; +} + +const MAX_ATTACHMENT_BYTES = 8 * 1024 * 1024; +const IMAGE_EXTS = /\.(png|jpe?g|gif|webp|bmp)$/i; + +function unique(values: Array): string[] { + return [ + ...new Set(values.filter((value): value is string => Boolean(value))), + ]; +} + +function resolveExistingDir(dir: string): string | null { + try { + if (!fs.existsSync(dir)) return null; + return fs.realpathSync(dir); + } catch { + return null; + } +} + +export function getDefaultAttachmentBaseDirs(): string[] { + const home = process.env.HOME; + const codexHome = + process.env.CODEX_HOME || (home ? path.join(home, '.codex') : null); + // Keep defaults narrow. Runtime-specific workspaces must be passed via + // attachmentBaseDirs so one room cannot attach another room's files by path. + return unique([ + '/tmp', + path.join(DATA_DIR, 'attachments'), + codexHome ? path.join(codexHome, 'generated_images') : null, + ]) + .map(resolveExistingDir) + .filter((dir): dir is string => Boolean(dir)); +} + +function isWithinBaseDir(realPath: string, baseDir: string): boolean { + const relative = path.relative(baseDir, realPath); + return ( + relative === '' || + (!!relative && !relative.startsWith('..') && !path.isAbsolute(relative)) + ); +} + +function matchesAllowedBaseDir(realPath: string, baseDirs: string[]): boolean { + return baseDirs.some((baseDir) => isWithinBaseDir(realPath, baseDir)); +} + +function detectImageMime(filePath: string): string | null { + const handle = fs.openSync(filePath, 'r'); + try { + const buffer = Buffer.alloc(512); + const bytesRead = fs.readSync(handle, buffer, 0, buffer.length, 0); + const header = buffer.subarray(0, bytesRead); + if ( + header + .subarray(0, 8) + .equals(Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a])) + ) { + return 'image/png'; + } + if (header[0] === 0xff && header[1] === 0xd8 && header[2] === 0xff) { + return 'image/jpeg'; + } + if ( + header.subarray(0, 6).toString('ascii') === 'GIF87a' || + header.subarray(0, 6).toString('ascii') === 'GIF89a' + ) { + return 'image/gif'; + } + if ( + header.subarray(0, 4).toString('ascii') === 'RIFF' && + header.subarray(8, 12).toString('ascii') === 'WEBP' + ) { + return 'image/webp'; + } + if (header[0] === 0x42 && header[1] === 0x4d) { + return 'image/bmp'; + } + return null; + } finally { + fs.closeSync(handle); + } +} + +function normalizeAttachmentName( + attachment: OutboundAttachment, + realPath: string, +): string { + const candidate = attachment.name + ? path.basename(attachment.name) + : path.basename(realPath); + return candidate || 'attachment'; +} + +export function validateOutboundAttachments( + attachments: OutboundAttachment[] | undefined, + options: { baseDirs?: string[] } = {}, +): ValidateOutboundAttachmentsResult { + const baseDirs = unique([ + ...getDefaultAttachmentBaseDirs(), + ...(options.baseDirs ?? []).map(resolveExistingDir), + ]).filter((dir): dir is string => Boolean(dir)); + const files: ValidatedOutboundAttachment[] = []; + const rejected: Array<{ path: string; reason: string }> = []; + const seen = new Set(); + + for (const attachment of attachments ?? []) { + const requestedPath = attachment.path; + try { + if (!path.isAbsolute(requestedPath)) { + rejected.push({ path: requestedPath, reason: 'not-absolute' }); + continue; + } + if (!IMAGE_EXTS.test(requestedPath)) { + rejected.push({ path: requestedPath, reason: 'unsupported-extension' }); + continue; + } + if (!fs.existsSync(requestedPath)) { + rejected.push({ path: requestedPath, reason: 'not-found' }); + continue; + } + const realPath = fs.realpathSync(requestedPath); + if (seen.has(realPath)) continue; + const stat = fs.statSync(realPath); + if (!stat.isFile()) { + rejected.push({ path: requestedPath, reason: 'not-file' }); + continue; + } + if (stat.size > MAX_ATTACHMENT_BYTES) { + rejected.push({ path: requestedPath, reason: 'too-large' }); + continue; + } + if (!matchesAllowedBaseDir(realPath, baseDirs)) { + rejected.push({ path: requestedPath, reason: 'outside-allowed-dirs' }); + continue; + } + const detectedMime = detectImageMime(realPath); + if (!detectedMime) { + rejected.push({ + path: requestedPath, + reason: 'invalid-image-signature', + }); + continue; + } + if (attachment.mime && attachment.mime !== detectedMime) { + rejected.push({ path: requestedPath, reason: 'mime-mismatch' }); + continue; + } + files.push({ + attachment: realPath, + name: normalizeAttachmentName(attachment, realPath), + }); + seen.add(realPath); + } catch { + rejected.push({ path: requestedPath, reason: 'validation-error' }); + } + } + + return { files, rejected }; +} diff --git a/src/types.ts b/src/types.ts index 5c00ed3..07f72f6 100644 --- a/src/types.ts +++ b/src/types.ts @@ -26,6 +26,22 @@ export type VisiblePhase = 'silent' | 'progress' | 'final'; export type AgentVisibility = 'public' | 'silent'; +export interface OutboundAttachment { + path: string; + name?: string; + mime?: string; +} + +export interface SendMessageOptions { + attachments?: OutboundAttachment[]; + /** + * Extra realpath roots that are valid for this delivery attempt. Runtime + * callers can pass the active project/workspace directory without widening + * the global Discord attachment allowlist. + */ + attachmentBaseDirs?: string[]; +} + export type PairedRoomRole = 'owner' | 'reviewer' | 'arbiter'; export type PairedTaskStatus = @@ -126,6 +142,7 @@ export type StructuredAgentOutput = | { visibility: 'public'; text: string; + attachments?: OutboundAttachment[]; } | { visibility: 'silent'; @@ -234,7 +251,11 @@ export interface ChannelOutboundAuditMeta { export interface Channel { name: string; connect(): Promise; - sendMessage(jid: string, text: string): Promise; + sendMessage( + jid: string, + text: string, + options?: SendMessageOptions, + ): Promise; isConnected(): boolean; ownsJid(jid: string): boolean; // Optional: whether a stored inbound message was authored by this channel's own bot/user. From ed686fc24d16c880741051942bca2b3dffd42e66 Mon Sep 17 00:00:00 2001 From: ejclaw Date: Sat, 25 Apr 2026 09:20:49 +0900 Subject: [PATCH 013/285] style: format structured attachment test --- src/message-turn-controller.test.ts | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/message-turn-controller.test.ts b/src/message-turn-controller.test.ts index ecc686a..acc4912 100644 --- a/src/message-turn-controller.test.ts +++ b/src/message-turn-controller.test.ts @@ -356,13 +356,10 @@ describe('MessageTurnController outbound audit logging', () => { } as any); await controller.finish('success'); - expect(deliverFinalText).toHaveBeenCalledWith( - '스크린샷을 첨부했습니다.', - { - replaceMessageId: null, - attachments, - }, - ); + expect(deliverFinalText).toHaveBeenCalledWith('스크린샷을 첨부했습니다.', { + replaceMessageId: null, + attachments, + }); expect(getAuditEntries()).toEqual( expect.arrayContaining([ expect.objectContaining({ From 11ab18280cc1dabc5d09a8f21e38fa0bcd16c0c0 Mon Sep 17 00:00:00 2001 From: Eyejoker Date: Sun, 26 Apr 2026 22:03:52 +0900 Subject: [PATCH 014/285] [codex] Switch dashboard sections to view routing Switch dashboard navigation from long section scrolling to hash-backed views, preserving Usage as the default operational view and keeping mobile drawer navigation visible. --- apps/dashboard/src/App.tsx | 154 +++++++++++++++++++++++++--------- apps/dashboard/src/styles.css | 25 ++++++ 2 files changed, 140 insertions(+), 39 deletions(-) diff --git a/apps/dashboard/src/App.tsx b/apps/dashboard/src/App.tsx index 7fd51b0..6d96882 100644 --- a/apps/dashboard/src/App.tsx +++ b/apps/dashboard/src/App.tsx @@ -27,9 +27,28 @@ interface DashboardState { type UsageRow = DashboardOverview['usage']['rows'][number]; type RiskLevel = 'ok' | 'warn' | 'critical'; type UsageGroup = 'primary' | 'codex'; +type DashboardView = 'usage' | 'health' | 'rooms' | 'scheduled'; const REFRESH_INTERVAL_MS = 15_000; const LOCALE_STORAGE_KEY = 'ejclaw.dashboard.locale'; +const DEFAULT_VIEW: DashboardView = 'usage'; + +function isDashboardView( + value: string | null | undefined, +): value is DashboardView { + return ( + value === 'usage' || + value === 'health' || + value === 'rooms' || + value === 'scheduled' + ); +} + +function readViewFromHash(): DashboardView { + if (typeof window === 'undefined') return DEFAULT_VIEW; + const raw = window.location.hash.replace(/^#\/?/, ''); + return isDashboardView(raw) ? raw : DEFAULT_VIEW; +} function readInitialLocale(): Locale { const stored = @@ -110,10 +129,10 @@ function queueLabel( function navItems(t: Messages) { return [ - { href: '#overview', label: t.nav.health }, - { href: '#usage', label: t.nav.usage }, - { href: '#rooms', label: t.nav.rooms }, - { href: '#work', label: t.nav.scheduled }, + { href: '#/usage', label: t.nav.usage, view: 'usage' as const }, + { href: '#/health', label: t.nav.health, view: 'health' as const }, + { href: '#/rooms', label: t.nav.rooms, view: 'rooms' as const }, + { href: '#/scheduled', label: t.nav.scheduled, view: 'scheduled' as const }, ]; } @@ -170,15 +189,19 @@ function LoadingSkeleton({ t }: { t: Messages }) { } function SideRail({ + activeView, lastRefreshed, locale, + onNavigate, onLocaleChange, onRefresh, refreshing, t, }: { + activeView: DashboardView; lastRefreshed: string | null; locale: Locale; + onNavigate: (view: DashboardView) => void; onLocaleChange: (locale: Locale) => void; onRefresh: () => void; refreshing: boolean; @@ -192,7 +215,13 @@ function SideRail({