From 12838a8b077c7e41501ee599aeee467e4c6b112f Mon Sep 17 00:00:00 2001 From: ejclaw Date: Wed, 22 Apr 2026 20:48:33 +0900 Subject: [PATCH 01/12] 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 5599791eae907ff4049e10864479340e1d8b66cc Mon Sep 17 00:00:00 2001 From: ejclaw Date: Wed, 22 Apr 2026 20:50:39 +0900 Subject: [PATCH 02/12] 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 921a254120324b9e3adf8cabfda9365b2880ccfa Mon Sep 17 00:00:00 2001 From: ejclaw Date: Wed, 22 Apr 2026 20:52:13 +0900 Subject: [PATCH 03/12] 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 d8a3702b1e1fcd6dd827604fc09c50738d9f8617 Mon Sep 17 00:00:00 2001 From: ejclaw Date: Wed, 22 Apr 2026 20:55:12 +0900 Subject: [PATCH 04/12] 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 13ac26b80e34e71cca918c28472bea6afc6fe5bf Mon Sep 17 00:00:00 2001 From: ejclaw Date: Wed, 22 Apr 2026 20:56:27 +0900 Subject: [PATCH 05/12] 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 bf4cbd03194a030a72281f0474a25b2304ccb703 Mon Sep 17 00:00:00 2001 From: ejclaw Date: Wed, 22 Apr 2026 20:57:59 +0900 Subject: [PATCH 06/12] 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 3b0875e80d26a0c39c8a6438f1951b8ec9d48723 Mon Sep 17 00:00:00 2001 From: ejclaw Date: Wed, 22 Apr 2026 20:59:10 +0900 Subject: [PATCH 07/12] 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 afd9c7a5384fe013edbfa5c19dc870df23933b56 Mon Sep 17 00:00:00 2001 From: ejclaw Date: Thu, 23 Apr 2026 13:06:26 +0900 Subject: [PATCH 08/12] =?UTF-8?q?Phase=200=20=E2=80=94=20STEP=5FDONE/TASK?= =?UTF-8?q?=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 9ef79e382862f5a548fd4116fbb63f6d05cb67ab Mon Sep 17 00:00:00 2001 From: ejclaw Date: Thu, 23 Apr 2026 15:27:49 +0900 Subject: [PATCH 09/12] 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 6d28a4ae192203a05b852b681020d22c7eaac734 Mon Sep 17 00:00:00 2001 From: ejclaw Date: Thu, 23 Apr 2026 18:51:56 +0900 Subject: [PATCH 10/12] 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 f84dccfa10b97dd05b944e2fbef07f489e2d9038 Mon Sep 17 00:00:00 2001 From: ejclaw Date: Fri, 24 Apr 2026 12:03:12 +0900 Subject: [PATCH 11/12] 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 3eee842164e79f9ca2d3ccab591ab1a8e287daa9 Mon Sep 17 00:00:00 2001 From: ejclaw Date: Fri, 24 Apr 2026 13:13:11 +0900 Subject: [PATCH 12/12] fix: restore bundled agent CLI resolution - Upgrade EJClaw bundled Codex CLI to 0.124.0 for gpt-5.5 access - Resolve Claude Agent SDK native binary packages via package.json - Prefer glibc Claude binary before musl on Linux and align platform binary names - Add regression coverage for optional package resolution --- runners/agent-runner/src/bundled-cli-path.ts | 21 +++++-- .../test/bundled-cli-path.test.ts | 55 +++++++++++++++++-- runners/codex-runner/bun.lock | 16 +++--- runners/codex-runner/package.json | 2 +- 4 files changed, 74 insertions(+), 20 deletions(-) diff --git a/runners/agent-runner/src/bundled-cli-path.ts b/runners/agent-runner/src/bundled-cli-path.ts index 281bf70..f48a3f1 100644 --- a/runners/agent-runner/src/bundled-cli-path.ts +++ b/runners/agent-runner/src/bundled-cli-path.ts @@ -45,7 +45,7 @@ function platformCandidates( return [ { pkg: `@anthropic-ai/claude-agent-sdk-darwin-${arch}`, - file: arch === 'arm64' ? 'cli' : 'cli', + file: 'claude', }, ]; } @@ -53,7 +53,7 @@ function platformCandidates( return [ { pkg: `@anthropic-ai/claude-agent-sdk-win32-${arch}`, - file: 'cli.exe', + file: 'claude.exe', }, ]; } @@ -127,12 +127,22 @@ export function resolveBundledClaudeCodeExecutable(options?: { * Default package directory resolver. Uses `require.resolve` against this * module's location so it works regardless of whether agent-runner is invoked * from its own node_modules layout or via a parent workspace. + * + * Important: the SDK's optional native-binary packages may not expose a bare + * package entrypoint, so `require.resolve(pkg)` can fail even when the package + * and binary are installed. Resolve `package.json` first, then fall back to the + * bare package only for package layouts that do expose an entrypoint. */ function defaultResolvePackageDir(pkg: string): string | null { + const req = createRequire(import.meta.url); + try { + const pkgJson = req.resolve(`${pkg}/package.json`); + return path.dirname(pkgJson); + } catch { + // Fall through to legacy/bare-entrypoint resolution below. + } + try { - // Resolve the package entrypoint instead of package.json because the SDK's - // exports map does not expose `./package.json`. - const req = createRequire(import.meta.url); const entrypoint = req.resolve(pkg); return path.dirname(entrypoint); } catch { @@ -142,5 +152,6 @@ function defaultResolvePackageDir(pkg: string): string | null { export const __test__ = { platformCandidates, + defaultResolvePackageDir, ENV_OVERRIDE, }; diff --git a/runners/agent-runner/test/bundled-cli-path.test.ts b/runners/agent-runner/test/bundled-cli-path.test.ts index 608fbf7..a68c8b3 100644 --- a/runners/agent-runner/test/bundled-cli-path.test.ts +++ b/runners/agent-runner/test/bundled-cli-path.test.ts @@ -1,8 +1,12 @@ import fs from 'fs'; +import { createRequire } from 'module'; import path from 'path'; import { afterEach, describe, expect, it } from 'vitest'; -import { resolveBundledClaudeCodeExecutable } from '../src/bundled-cli-path.js'; +import { + __test__, + resolveBundledClaudeCodeExecutable, +} from '../src/bundled-cli-path.js'; describe('resolveBundledClaudeCodeExecutable', () => { const origEnv = process.env.EJCLAW_CLAUDE_CLI_PATH; @@ -83,10 +87,10 @@ describe('resolveBundledClaudeCodeExecutable', () => { expect(result).toBe(path.join(muslDir, 'claude')); }); - it('resolves Darwin arm64 binary under `cli`', () => { + it('resolves Darwin arm64 binary under `claude`', () => { const dir = '/fake/node_modules/@anthropic-ai/claude-agent-sdk-darwin-arm64'; - const existsSync = (p: string): boolean => p === path.join(dir, 'cli'); + const existsSync = (p: string): boolean => p === path.join(dir, 'claude'); const result = resolveBundledClaudeCodeExecutable({ env: {}, existsSync, @@ -95,13 +99,13 @@ describe('resolveBundledClaudeCodeExecutable', () => { resolvePackageDir: (pkg) => pkg === '@anthropic-ai/claude-agent-sdk-darwin-arm64' ? dir : null, }); - expect(result).toBe(path.join(dir, 'cli')); + expect(result).toBe(path.join(dir, 'claude')); }); - it('resolves Windows binary under `cli.exe`', () => { + it('resolves Windows binary under `claude.exe`', () => { const dir = 'C:\\\\fake\\\\node_modules\\\\@anthropic-ai\\\\claude-agent-sdk-win32-x64'; - const binary = path.join(dir, 'cli.exe'); + const binary = path.join(dir, 'claude.exe'); const existsSync = (p: string): boolean => p === binary; const result = resolveBundledClaudeCodeExecutable({ env: {}, @@ -114,6 +118,17 @@ describe('resolveBundledClaudeCodeExecutable', () => { expect(result).toBe(binary); }); + it('default resolver handles SDK optional packages without a bare entrypoint', () => { + if (process.platform !== 'linux' || process.arch !== 'x64') return; + + const dir = __test__.defaultResolvePackageDir( + '@anthropic-ai/claude-agent-sdk-linux-x64', + ); + if (!dir) return; + + expect(fs.existsSync(path.join(dir, 'claude'))).toBe(true); + }); + it('throws a descriptive error when no binary is found', () => { const existsSync = () => false; expect(() => @@ -127,6 +142,34 @@ describe('resolveBundledClaudeCodeExecutable', () => { ).toThrow(/Unable to locate a bundled Claude Code CLI binary/); }); + it('resolves the linux x64 optional package even when it has no JS entrypoint', () => { + if (process.platform !== 'linux' || process.arch !== 'x64') { + return; + } + + const req = createRequire(import.meta.url); + let packageJsonPath: string; + try { + packageJsonPath = req.resolve( + '@anthropic-ai/claude-agent-sdk-linux-x64/package.json', + ); + } catch { + return; + } + + try { + req.resolve('@anthropic-ai/claude-agent-sdk-linux-x64'); + return; + } catch { + // This is the regression case: package.json resolves, bare package does not. + } + + const result = resolveBundledClaudeCodeExecutable({ env: {} }); + + expect(result).toBe(path.join(path.dirname(packageJsonPath), 'claude')); + expect(fs.existsSync(result)).toBe(true); + }); + it('end-to-end: resolves the actual bundled binary on this host when installed', () => { // Real-world smoke test. Some CI/host sandboxes do not install the optional // per-platform SDK package, so treat "package not resolvable" as a host diff --git a/runners/codex-runner/bun.lock b/runners/codex-runner/bun.lock index f4e3765..efcfdf2 100644 --- a/runners/codex-runner/bun.lock +++ b/runners/codex-runner/bun.lock @@ -5,7 +5,7 @@ "": { "name": "ejclaw-codex-runner", "dependencies": { - "@openai/codex": "^0.120.0", + "@openai/codex": "^0.124.0", "ejclaw-runners-shared": "file:../shared", }, "devDependencies": { @@ -15,19 +15,19 @@ }, }, "packages": { - "@openai/codex": ["@openai/codex@0.120.0", "", { "optionalDependencies": { "@openai/codex-darwin-arm64": "npm:@openai/codex@0.120.0-darwin-arm64", "@openai/codex-darwin-x64": "npm:@openai/codex@0.120.0-darwin-x64", "@openai/codex-linux-arm64": "npm:@openai/codex@0.120.0-linux-arm64", "@openai/codex-linux-x64": "npm:@openai/codex@0.120.0-linux-x64", "@openai/codex-win32-arm64": "npm:@openai/codex@0.120.0-win32-arm64", "@openai/codex-win32-x64": "npm:@openai/codex@0.120.0-win32-x64" }, "bin": { "codex": "bin/codex.js" } }, "sha512-e2P1Gya3dwsRe9IPOiswVz5JfR700u+/sWCqDc3jkqv2QViPkNiBmZoGhFnZL5jBpKakSjehC4/Fpspg70nHTw=="], + "@openai/codex": ["@openai/codex@0.124.0", "", { "optionalDependencies": { "@openai/codex-darwin-arm64": "npm:@openai/codex@0.124.0-darwin-arm64", "@openai/codex-darwin-x64": "npm:@openai/codex@0.124.0-darwin-x64", "@openai/codex-linux-arm64": "npm:@openai/codex@0.124.0-linux-arm64", "@openai/codex-linux-x64": "npm:@openai/codex@0.124.0-linux-x64", "@openai/codex-win32-arm64": "npm:@openai/codex@0.124.0-win32-arm64", "@openai/codex-win32-x64": "npm:@openai/codex@0.124.0-win32-x64" }, "bin": { "codex": "bin/codex.js" } }, "sha512-1EVAuPyAQZ8zIVMw3bPJ6a4R8ifLAZ7LGsOyknj5c2he9AFXVRCmWx12WrdZJ25wcBvOEKt1n1Zx+QAj0EVGbQ=="], - "@openai/codex-darwin-arm64": ["@openai/codex@0.120.0-darwin-arm64", "", { "os": "darwin", "cpu": "arm64" }, "sha512-7CU+I5kBaMuoqfG3xisq0mUWzxoEHvfu34cB8a0KpBiIhAgu12fKpmYgZ4/DvRP6Wm9Fu6LJYKVF5apUHFp8nQ=="], + "@openai/codex-darwin-arm64": ["@openai/codex@0.124.0-darwin-arm64", "", { "os": "darwin", "cpu": "arm64" }, "sha512-lnuqeAdl+RjPWsqVZ8rrPskRIOZmzlyr8e5q/wFVEnMPsm9dWgjRW1PKa84UmDSQVOuz9GObF28DEHFyC4A8NA=="], - "@openai/codex-darwin-x64": ["@openai/codex@0.120.0-darwin-x64", "", { "os": "darwin", "cpu": "x64" }, "sha512-d7joNYuwrmd5iIdp/xAE5f8bZT1r82MnmU6Hzgxq3G+xClwEyhxU737ZWnstFSpnZNfxJ5zXCuFUJh4CAkHNtQ=="], + "@openai/codex-darwin-x64": ["@openai/codex@0.124.0-darwin-x64", "", { "os": "darwin", "cpu": "x64" }, "sha512-Br+VlL83IOu96Urw+mkZ1PQXLsQXGAYEH6kXNt4ULuVt7qAdQY2FKYwIgLLVLl0vpMk8qWxdfdVMqhiu2uCHlg=="], - "@openai/codex-linux-arm64": ["@openai/codex@0.120.0-linux-arm64", "", { "os": "linux", "cpu": "arm64" }, "sha512-sVYY25/URlpZPtb0Q0ryLh+lcq9UTEtHAkdZKa0a/R7mAdyPuhpU9V6jWmxwiUh7s53XZOEVFoKmLfH8YIDWCQ=="], + "@openai/codex-linux-arm64": ["@openai/codex@0.124.0-linux-arm64", "", { "os": "linux", "cpu": "arm64" }, "sha512-QXafFVQJxPfU1LSCI5afuHsF5evKAcbQpFuKou0Kl241/HG/zYHdwoWj546zH844gJgegIPAxPf36+RSkUsbbA=="], - "@openai/codex-linux-x64": ["@openai/codex@0.120.0-linux-x64", "", { "os": "linux", "cpu": "x64" }, "sha512-VcP9B/c/O+EFEgqoetCzvHrLfAdo8vrt09Gx1lJ8ikewctqAuJ/ozj/6wuvlz7XaaK64ib5cge01pOAeCyt2Sg=="], + "@openai/codex-linux-x64": ["@openai/codex@0.124.0-linux-x64", "", { "os": "linux", "cpu": "x64" }, "sha512-cJ4QfQIrz2gkXVWephiGo9JDyD3qLKhvkTTLk7gI8rKd7MZpVXn9/1e7pZCQnJVA7Dk6ocA5G+sxnR78SoIejw=="], - "@openai/codex-win32-arm64": ["@openai/codex@0.120.0-win32-arm64", "", { "os": "win32", "cpu": "arm64" }, "sha512-SAaTQU1XHa1qDnmQldmbyROIY5SiaspF+Cw3ziWeeTgyAET3rWusm4ELOElx6QiY1ugYW5ZD+7AFufS2z1xtpQ=="], + "@openai/codex-win32-arm64": ["@openai/codex@0.124.0-win32-arm64", "", { "os": "win32", "cpu": "arm64" }, "sha512-oDSI/CQh0kagBwWVPG9EiUBfHiY27ju3LPrjTVZg4VMpVJVNA31JTK8rHMZ6G/2gfNmOl6DMBA6+0ICxSkkshg=="], - "@openai/codex-win32-x64": ["@openai/codex@0.120.0-win32-x64", "", { "os": "win32", "cpu": "x64" }, "sha512-zja1GNrbHyOUTvOy5FVMa+rAYIs3m+FOS8rAXftxMEhodMmkMw2O8zcvso657SHhZR0hIEiZ6T70lcyH2YX0mQ=="], + "@openai/codex-win32-x64": ["@openai/codex@0.124.0-win32-x64", "", { "os": "win32", "cpu": "x64" }, "sha512-t+lAwmCWwIWQKk6dKaYetRhQsmA+uDmQy1NLka1xAAv3PlNOH8iNz9Lsisr3qYkBR8Tf7tbQlt+pl9tw/A5mfA=="], "@types/node": ["@types/node@22.19.15", "", { "dependencies": { "undici-types": "~6.21.0" } }, "sha512-F0R/h2+dsy5wJAUe3tAU6oqa2qbWY5TpNfL/RGmo1y38hiyO1w3x2jPtt76wmuaJI4DQnOBu21cNXQ2STIUUWg=="], diff --git a/runners/codex-runner/package.json b/runners/codex-runner/package.json index 0dfb377..66788ca 100644 --- a/runners/codex-runner/package.json +++ b/runners/codex-runner/package.json @@ -11,7 +11,7 @@ }, "dependencies": { "ejclaw-runners-shared": "file:../shared", - "@openai/codex": "^0.120.0" + "@openai/codex": "^0.124.0" }, "devDependencies": { "@types/node": "^22.10.7",