From 4f535497eba3d3c328408e13d9c2ed612ae31f16 Mon Sep 17 00:00:00 2001 From: NagyVikt Date: Thu, 14 May 2026 23:44:30 +0200 Subject: [PATCH] feat(mcp-server): explain task_claim_file rejections with a reason-specific message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `task_claim_file` and the `TaskThread.claimFile` / `normalizeOptionalClaimPath` paths used to throw `INVALID_CLAIM_PATH: claim path is not claimable` with no hint at the reason. Telemetry showed agents bouncing off the same surface for the same input — e.g. `colony/packages/core/test` (a directory) — because the message gave them nothing to act on. The rejection branch now classifies the failure and renders a specific message per reason: directory ("claim individual files inside it instead"), pseudo (/dev/null etc), outside_repo, empty, plus the legacy generic fallback. Existing `INVALID_CLAIM_PATH` error code is preserved. New `@colony/storage` exports: * `classifyClaimPathRejection(context)` — pure classifier paralleling `normalizeRepoFilePath`. Returns the reason or `null`. * `claimPathRejectionMessage(reason, file_path)` — single source of truth for the user-facing message so the MCP `task_claim_file` handler and `TaskThread.claimFile` stay in sync. * New storage method `classifyTaskFilePathRejection(task_id, file_path, cwd?)` plumbs the task → repo_root lookup that `normalizeTaskFilePath` already does, so callers only pay for the classifier on the error branch. Tests: * `packages/storage/test/claim-path.test.ts` covers all five reasons (existing directory, trailing-slash directory, pseudo, empty, outside_repo) plus message rendering for each. * `apps/mcp-server/test/task-threads.test.ts` updates the existing pseudo- path test to assert the new message and adds a directory case asserting the recovery hint. Storage: 149/149 PASS. mcp-server: 273/273 PASS. Lint clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../explain-task-claim-file-rejection.md | 39 ++++++++ apps/mcp-server/src/tools/task.ts | 4 +- apps/mcp-server/test/task-threads.test.ts | 24 ++++- .../.openspec.yaml | 2 + .../notes.md | 27 ++++++ packages/core/src/task-thread.ts | 30 ++++-- packages/storage/src/claim-path.ts | 73 +++++++++++++++ packages/storage/src/index.ts | 12 ++- packages/storage/src/storage.ts | 25 ++++- packages/storage/test/claim-path.test.ts | 92 ++++++++++++++++++- 10 files changed, 312 insertions(+), 16 deletions(-) create mode 100644 .changeset/explain-task-claim-file-rejection.md create mode 100644 openspec/changes/agent-claude-explain-task-claim-file-rejection-direct-2026-05-14-23-35/.openspec.yaml create mode 100644 openspec/changes/agent-claude-explain-task-claim-file-rejection-direct-2026-05-14-23-35/notes.md diff --git a/.changeset/explain-task-claim-file-rejection.md b/.changeset/explain-task-claim-file-rejection.md new file mode 100644 index 0000000..5540cac --- /dev/null +++ b/.changeset/explain-task-claim-file-rejection.md @@ -0,0 +1,39 @@ +--- +'@colony/storage': minor +'@colony/core': patch +'@colony/mcp-server': patch +--- + +Explain `task_claim_file` rejections instead of returning a generic "not claimable" + +`task_claim_file` (and the `TaskThread.claimFile` / +`normalizeOptionalClaimPath` paths inside `@colony/core`) used to throw +`INVALID_CLAIM_PATH: claim path is not claimable` with no hint at the +reason. Telemetry showed agents bouncing off the same surface for the same +input — e.g. `colony/packages/core/test` (a directory) — because the +message gave them nothing to act on. + +The rejection branch now classifies the failure and renders a specific +message per reason: + +- `directory` — *"claim path "X" is a directory; claim individual files inside it instead."* +- `pseudo` — *"claim path "X" is a pseudo path (e.g. /dev/null) and cannot be claimed."* +- `outside_repo` — *"claim path "X" resolves outside this task's repo_root and cannot be claimed."* +- `empty` — *"claim path is empty."* +- fallback — the legacy generic message, still keyed on the input path. + +New exports from `@colony/storage`: + +- `classifyClaimPathRejection(context)` — pure classifier paralleling + `normalizeRepoFilePath`. Returns the reason or `null`. +- `claimPathRejectionMessage(reason, file_path)` — single source of + truth for the user-facing message so the MCP `task_claim_file` + handler and `TaskThread.claimFile` stay in sync. +- New storage method `classifyTaskFilePathRejection(task_id, file_path, + cwd?)` plumbs the task → repo_root lookup that the existing + `normalizeTaskFilePath` already does, so callers only pay for the + classifier on the error branch. + +No behavior change: the same inputs that used to be rejected are still +rejected; only the error message and code surface improve. Existing +INVALID_CLAIM_PATH error code is preserved. diff --git a/apps/mcp-server/src/tools/task.ts b/apps/mcp-server/src/tools/task.ts index 4e07082..3e7ede9 100644 --- a/apps/mcp-server/src/tools/task.ts +++ b/apps/mcp-server/src/tools/task.ts @@ -7,6 +7,7 @@ import { listPlans, liveFileContentionsForClaim, } from '@colony/core'; +import { claimPathRejectionMessage } from '@colony/storage'; import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; import { z } from 'zod'; import { type ToolContext, defaultWrapHandler } from './context.js'; @@ -364,7 +365,8 @@ export function register(server: McpServer, ctx: ToolContext): void { wrapHandler('task_claim_file', async ({ task_id, session_id, file_path, note }) => { const normalizedFilePath = store.storage.normalizeTaskFilePath(task_id, file_path); if (normalizedFilePath === null) { - return mcpErrorResponse('INVALID_CLAIM_PATH', `file path is not claimable: ${file_path}`); + const reason = store.storage.classifyTaskFilePathRejection(task_id, file_path); + return mcpErrorResponse('INVALID_CLAIM_PATH', claimPathRejectionMessage(reason, file_path)); } const previous = store.storage.getClaim(task_id, normalizedFilePath); const liveContentions = liveFileContentionsForClaim(store, { diff --git a/apps/mcp-server/test/task-threads.test.ts b/apps/mcp-server/test/task-threads.test.ts index 3f3f32c..e16e61f 100644 --- a/apps/mcp-server/test/task-threads.test.ts +++ b/apps/mcp-server/test/task-threads.test.ts @@ -97,7 +97,7 @@ describe('task threads — file claims', () => { expect(claim?.metadata).toContain('"file_path":"src/viewer.tsx"'); }); - it('rejects pseudo task_claim_file paths', async () => { + it('rejects pseudo task_claim_file paths with a pseudo-specific message', async () => { const { task_id, sessionA } = seedTwoSessionTask(); const result = await callError('task_claim_file', { @@ -107,6 +107,28 @@ describe('task threads — file claims', () => { }); expect(result.code).toBe('INVALID_CLAIM_PATH'); + expect(result.error).toBe( + 'claim path "/dev/null" is a pseudo path (e.g. /dev/null) and cannot be claimed.', + ); + expect(store.storage.listClaims(task_id)).toEqual([]); + }); + + it('rejects directory task_claim_file paths with a directory-specific recovery hint', async () => { + const { task_id, sessionA } = seedTwoSessionTask(); + + // Trailing-slash form: classified as a directory without needing the + // path to exist on disk, so the assertion stays portable across CI + // working directories. + const result = await callError('task_claim_file', { + task_id, + session_id: sessionA, + file_path: 'packages/core/test/', + }); + + expect(result.code).toBe('INVALID_CLAIM_PATH'); + expect(result.error).toBe( + 'claim path "packages/core/test/" is a directory; claim individual files inside it instead.', + ); expect(store.storage.listClaims(task_id)).toEqual([]); }); diff --git a/openspec/changes/agent-claude-explain-task-claim-file-rejection-direct-2026-05-14-23-35/.openspec.yaml b/openspec/changes/agent-claude-explain-task-claim-file-rejection-direct-2026-05-14-23-35/.openspec.yaml new file mode 100644 index 0000000..66dd08a --- /dev/null +++ b/openspec/changes/agent-claude-explain-task-claim-file-rejection-direct-2026-05-14-23-35/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-05-14 diff --git a/openspec/changes/agent-claude-explain-task-claim-file-rejection-direct-2026-05-14-23-35/notes.md b/openspec/changes/agent-claude-explain-task-claim-file-rejection-direct-2026-05-14-23-35/notes.md new file mode 100644 index 0000000..8245951 --- /dev/null +++ b/openspec/changes/agent-claude-explain-task-claim-file-rejection-direct-2026-05-14-23-35/notes.md @@ -0,0 +1,27 @@ +# agent-claude-explain-task-claim-file-rejection-direct-2026-05-14-23-35 (minimal / T1) + +Branch: `agent/claude/explain-task-claim-file-rejection-direct-2026-05-14-23-35` + +`task_claim_file` (MCP) and `TaskThread.claimFile` / +`normalizeOptionalClaimPath` (core) now classify the rejection branch of +`normalizeTaskFilePath` and render a specific user-facing message per +reason: directory, pseudo, outside_repo, empty, or unknown (legacy +fallback). The 5 `INVALID_CLAIM_PATH: file path is not claimable: +colony/packages/core/test` errors visible in `colony gain` are exactly the +"directory" branch — the message now tells the agent to claim individual +files instead of bouncing off the same input. + +Shared primitives live in `@colony/storage` so the MCP tool and TaskThread +both consume the same `claimPathRejectionMessage()` and reason enum. +Behavior unchanged; only the message text + new optional classifier export +are introduced. + +## Handoff + +- Handoff: change=`agent-claude-explain-task-claim-file-rejection-direct-2026-05-14-23-35`; branch=`agent/claude/explain-task-claim-file-rejection-direct-2026-05-14-23-35`; scope=`packages/storage + packages/core + apps/mcp-server`; action=`finish via PR after user sign-off`. + +## Cleanup + +- [ ] Run: `gx branch finish --branch agent/claude/explain-task-claim-file-rejection-direct-2026-05-14-23-35 --base main --via-pr --wait-for-merge --cleanup` +- [ ] Record PR URL + `MERGED` state in the completion handoff. +- [ ] Confirm sandbox worktree is gone (`git worktree list`, `git branch -a`). diff --git a/packages/core/src/task-thread.ts b/packages/core/src/task-thread.ts index 8d73790..e6d2978 100644 --- a/packages/core/src/task-thread.ts +++ b/packages/core/src/task-thread.ts @@ -1,10 +1,11 @@ -import type { - LinkedTask, - ObservationRow, - TaskClaimRow, - TaskLinkRow, - TaskParticipantRow, - TaskRow, +import { + type LinkedTask, + type ObservationRow, + type TaskClaimRow, + type TaskLinkRow, + type TaskParticipantRow, + type TaskRow, + claimPathRejectionMessage, } from '@colony/storage'; import { classifyClaimAge, isStrongClaimAge } from './claim-age.js'; import type { MemoryStore } from './memory-store.js'; @@ -617,8 +618,13 @@ export class TaskThread { metadata?: Record; }): number { const filePath = this.store.storage.normalizeTaskFilePath(this.task_id, p.file_path); - if (filePath === null) - throw taskError(TASK_THREAD_ERROR_CODES.INVALID_CLAIM_PATH, 'claim path is not claimable'); + if (filePath === null) { + const reason = this.store.storage.classifyTaskFilePathRejection(this.task_id, p.file_path); + throw taskError( + TASK_THREAD_ERROR_CODES.INVALID_CLAIM_PATH, + claimPathRejectionMessage(reason, p.file_path), + ); + } return this.store.storage.transaction(() => { this.store.storage.claimFile({ task_id: this.task_id, @@ -1870,7 +1876,11 @@ export class TaskThread { if (file_path === undefined) return null; const normalized = this.store.storage.normalizeTaskFilePath(this.task_id, file_path); if (normalized === null) { - throw taskError(TASK_THREAD_ERROR_CODES.INVALID_CLAIM_PATH, 'claim path is not claimable'); + const reason = this.store.storage.classifyTaskFilePathRejection(this.task_id, file_path); + throw taskError( + TASK_THREAD_ERROR_CODES.INVALID_CLAIM_PATH, + claimPathRejectionMessage(reason, file_path), + ); } return normalized; } diff --git a/packages/storage/src/claim-path.ts b/packages/storage/src/claim-path.ts index 820ee8f..01eff10 100644 --- a/packages/storage/src/claim-path.ts +++ b/packages/storage/src/claim-path.ts @@ -76,6 +76,79 @@ export function normalizeClaimPath( return normalizeRepoFilePath(context); } +/** + * Discriminated reason for a claim-path rejection, used by callers that want + * to surface a specific error to the agent instead of the generic "claim path + * is not claimable". Kept parallel to `normalizeRepoFilePath` so any future + * rejection branch added there should also be classified here. + */ +export type ClaimPathRejectionReason = + | 'empty' + | 'pseudo' + | 'directory' + | 'outside_repo' + | 'unknown'; + +/** + * Renders a specific user-facing message for a claim-path rejection so agents + * see why their input was rejected (directory vs pseudo vs outside-repo vs + * empty) instead of the generic "not claimable". Lives next to the classifier + * so any reason added to the enum is forced to grow a message branch too. + */ +export function claimPathRejectionMessage( + reason: ClaimPathRejectionReason | null, + file_path: string, +): string { + switch (reason) { + case 'directory': + return `claim path "${file_path}" is a directory; claim individual files inside it instead.`; + case 'pseudo': + return `claim path "${file_path}" is a pseudo path (e.g. /dev/null) and cannot be claimed.`; + case 'outside_repo': + return `claim path "${file_path}" resolves outside this task's repo_root and cannot be claimed.`; + case 'empty': + return 'claim path is empty.'; + default: + return `claim path is not claimable: ${file_path}`; + } +} + +export function classifyClaimPathRejection( + context: ClaimPathContext, +): ClaimPathRejectionReason | null { + const rawPath = context.file_path.trim(); + if (!rawPath) return 'empty'; + if (isPseudoClaimPath(rawPath)) return 'pseudo'; + if (looksLikeDirectoryPath(rawPath)) return 'directory'; + + const repoRoot = context.repo_root + ? realpathWithMissingTail(context.repo_root) + : context.cwd + ? realpathWithMissingTail(context.cwd) + : undefined; + const cwdRoot = context.cwd ? realpathWithMissingTail(context.cwd) : repoRoot; + const absolutePath = path.isAbsolute(rawPath) + ? realpathWithMissingTail(rawPath) + : cwdRoot + ? realpathWithMissingTail(path.resolve(relativePathBase(repoRoot, cwdRoot, rawPath), rawPath)) + : undefined; + if (absolutePath && isExistingDirectoryPath(absolutePath)) return 'directory'; + + // The non-null happy path of normalizeRepoFilePath ends at this comment. + // If we reach here without one of the explicit short-circuits, it means + // normalize would have either returned a value (so the caller wouldn't be + // here classifying a rejection) or fallen through. The remaining + // null-return is "absolutePath was inside repoRoot's filesystem chain but + // not strictly inside repoRoot itself", which only happens when the path + // resolves to a sibling worktree/repo — i.e. outside this task's repo_root. + if (absolutePath && repoRoot) { + const relativePath = repoRelativePath({ absolutePath, repoRoot }); + if (relativePath === null) return 'outside_repo'; + } + + return 'unknown'; +} + function relativePathBase(repoRoot: string | undefined, cwdRoot: string, rawPath: string): string { if (!repoRoot) return cwdRoot; const cwdRelative = normalizeRelativePath(path.relative(repoRoot, cwdRoot)); diff --git a/packages/storage/src/index.ts b/packages/storage/src/index.ts index 03a9b61..9759626 100644 --- a/packages/storage/src/index.ts +++ b/packages/storage/src/index.ts @@ -1,7 +1,16 @@ export { Storage, PROTECTED_BRANCH_NAMES, isProtectedBranch } from './storage.js'; export { withBusyRetry } from './busy-retry.js'; export type { BusyRetryOptions } from './busy-retry.js'; -export { isPseudoClaimPath, normalizeClaimPath, normalizeRepoFilePath } from './claim-path.js'; +export { + claimPathRejectionMessage, + classifyClaimPathRejection, + isPseudoClaimPath, + normalizeClaimPath, + normalizeRepoFilePath, + type ClaimPathContext, + type ClaimPathRejectionReason, + type RepoFilePathContext, +} from './claim-path.js'; export { RunAttemptError, createRunAttempt, @@ -15,7 +24,6 @@ export { RUN_ATTEMPT_ACTIVE_STATUSES, RUN_ATTEMPT_TERMINAL_STATUSES, } from './types.js'; -export type { ClaimPathContext, RepoFilePathContext } from './claim-path.js'; export { COORDINATION_COMMIT_TOOLS, COORDINATION_READ_TOOLS, diff --git a/packages/storage/src/storage.ts b/packages/storage/src/storage.ts index 659ca31..46ef5a3 100644 --- a/packages/storage/src/storage.ts +++ b/packages/storage/src/storage.ts @@ -3,7 +3,11 @@ import { mkdirSync } from 'node:fs'; import { dirname, isAbsolute, normalize, relative, resolve } from 'node:path'; import Database from 'better-sqlite3'; -import { normalizeClaimPath } from './claim-path.js'; +import { + type ClaimPathRejectionReason, + classifyClaimPathRejection, + normalizeClaimPath, +} from './claim-path.js'; import { COLUMN_MIGRATIONS, POST_MIGRATION_SQL, SCHEMA_SQL } from './schema.js'; import { COORDINATION_COMMIT_TOOLS, @@ -1983,6 +1987,25 @@ export class Storage { }); } + /** + * Companion to `normalizeTaskFilePath` for the error path: when normalize + * returned null, this tells the caller *why* (directory, pseudo path, + * outside repo, empty, or unknown) using the same task→repo_root lookup. + * Cheap to call only on the rejection branch. + */ + classifyTaskFilePathRejection( + task_id: number, + file_path: string, + cwd?: string, + ): ClaimPathRejectionReason | null { + const task = this.getTask(task_id); + return classifyClaimPathRejection({ + repo_root: task?.repo_root, + cwd, + file_path, + }); + } + private matchingClaimFilePaths(task_id: number, file_path: string): string[] { const normalized = this.normalizeTaskFilePath(task_id, file_path); if (normalized === null) return []; diff --git a/packages/storage/test/claim-path.test.ts b/packages/storage/test/claim-path.test.ts index 0477450..be21c78 100644 --- a/packages/storage/test/claim-path.test.ts +++ b/packages/storage/test/claim-path.test.ts @@ -2,7 +2,12 @@ import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it } from 'vitest'; -import { normalizeClaimPath, normalizeRepoFilePath } from '../src/index.js'; +import { + claimPathRejectionMessage, + classifyClaimPathRejection, + normalizeClaimPath, + normalizeRepoFilePath, +} from '../src/index.js'; let dir: string; @@ -149,3 +154,88 @@ describe('normalizeRepoFilePath', () => { expect(normalizeClaimPath(repoRoot, repoRoot, './src/x.ts')).toBe('src/x.ts'); }); }); + +describe('classifyClaimPathRejection', () => { + it('returns "directory" for an existing directory the repo path resolves to', () => { + const { repoRoot } = repoFixture(); + // `src` exists as a directory inside the fixture repo. + expect( + classifyClaimPathRejection({ + repo_root: repoRoot, + cwd: repoRoot, + file_path: 'src', + }), + ).toBe('directory'); + }); + + it('returns "directory" for a trailing-slash path even if it does not exist', () => { + expect( + classifyClaimPathRejection({ + repo_root: dir, + cwd: dir, + file_path: 'never-created/', + }), + ).toBe('directory'); + }); + + it('returns "pseudo" for /dev/null and friends', () => { + expect(classifyClaimPathRejection({ repo_root: dir, cwd: dir, file_path: '/dev/null' })).toBe( + 'pseudo', + ); + }); + + it('returns "empty" for blank input', () => { + expect(classifyClaimPathRejection({ repo_root: dir, cwd: dir, file_path: ' ' })).toBe( + 'empty', + ); + }); + + it('returns "outside_repo" for an absolute path outside repo_root and no shared git common dir', () => { + const { repoRoot } = repoFixture(); + const outsideRoot = join(dir, 'outside'); + mkdirSync(outsideRoot, { recursive: true }); + const outsideFile = join(outsideRoot, 'lib.ts'); + writeFileSync(outsideFile, 'export const x = 1;\n'); + + expect( + classifyClaimPathRejection({ + repo_root: repoRoot, + cwd: repoRoot, + file_path: outsideFile, + }), + ).toBe('outside_repo'); + }); +}); + +describe('claimPathRejectionMessage', () => { + it('renders a directory-specific recovery hint', () => { + expect(claimPathRejectionMessage('directory', 'packages/core/test')).toBe( + 'claim path "packages/core/test" is a directory; claim individual files inside it instead.', + ); + }); + + it('renders a pseudo-path-specific message', () => { + expect(claimPathRejectionMessage('pseudo', '/dev/null')).toBe( + 'claim path "/dev/null" is a pseudo path (e.g. /dev/null) and cannot be claimed.', + ); + }); + + it('renders an outside-repo message', () => { + expect(claimPathRejectionMessage('outside_repo', '/tmp/foreign.ts')).toBe( + 'claim path "/tmp/foreign.ts" resolves outside this task\'s repo_root and cannot be claimed.', + ); + }); + + it('renders an empty-input message', () => { + expect(claimPathRejectionMessage('empty', '')).toBe('claim path is empty.'); + }); + + it('falls back to the legacy generic message when the reason is unknown / null', () => { + expect(claimPathRejectionMessage(null, 'weird/thing.ts')).toBe( + 'claim path is not claimable: weird/thing.ts', + ); + expect(claimPathRejectionMessage('unknown', 'weird/thing.ts')).toBe( + 'claim path is not claimable: weird/thing.ts', + ); + }); +});