Conversation
Wire the existing Issue #28/#33 task-lifecycle domain layer (src/workflow/domain/task.ts, policy, sqlite store) to MCP as mottainai_task_start and mottainai_task_status, gated behind a new opt-in gateway.workflowTasks config flag (default false, mirroring mottainai_worktree_new's gating). - mottainai_task_start never passes skipWorktree, so it always reserves a dedicated worktree/branch off the current branch regardless of policy — main/default branch can never become the work branch itself. - mottainai_task_status resolves the active task (if any) for the calling worktree without needing a task id, with no side effects. - Added startTask's missing invariant: reject starting a task from a worktree that already hosts another active task (active-task-in-workspace), needed once callers can start tasks from inside worktrees they didn't create themselves. - Corrupted/invalid .mottainai/workflow.json fails closed at the tool boundary instead of silently falling back to a preset; a missing file still falls back to the standard preset per existing documented behavior. - WorkflowSqliteStateStore is loaded via dynamic import only when the tools actually run, so workspaces that don't opt in never pay the node:sqlite experimental-warning cost on every CLI invocation.
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds the ChangesWorkflow task lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant LocalTools
participant WorkflowDomain
participant WorkflowSqliteStateStore
Client->>LocalTools: mottainai_task_start
LocalTools->>WorkflowDomain: validate and start task
WorkflowDomain->>WorkflowSqliteStateStore: read active tasks and persist task
WorkflowDomain-->>LocalTools: task and worktree data
LocalTools-->>Client: structured task response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/workflow/domain/task.test.ts (1)
263-263: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a substring assertion instead of a regex built from a variable.
outer.task.taskIdis a store-generated identifier, so the ReDoS warning from static analysis is a false positive. A plain substring check is still clearer and removes the warning.♻️ Proposed change
- assert.match(inner.detail, new RegExp(outer.task.taskId)); + assert.ok(inner.detail.includes(outer.task.taskId));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workflow/domain/task.test.ts` at line 263, Replace the regex-based assertion involving the store-generated outer.task.taskId with a plain substring assertion, while preserving the expected "active-task-in-workspace" reason validation in the surrounding test.Source: Linters/SAST tools
src/local-tools.ts (1)
589-589: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
ISSUE_REF_PATTERNallows.., which git rejects later in the flow.The pattern permits values such as
7..9.issueRefflows into the branch name throughbuildWorktreeNaming, and git refuses a ref that contains... The failure surfaces asgit-worktree-add-failedafter the task and worktree rows are reserved and rolled back, instead of as a direct validation error.Rejecting
..at the boundary gives a clearer message and avoids the reserve-then-roll-back cycle.♻️ Proposed change
-const ISSUE_REF_PATTERN = /^[A-Za-z0-9][A-Za-z0-9._-]*$/; +// `..` を除外する — issueRef は branch 名に入るため、git が ref として拒否する値を +// 予約前に弾く。 +const ISSUE_REF_PATTERN = /^[A-Za-z0-9](?!.*\.\.)[A-Za-z0-9._-]*$/;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/local-tools.ts` at line 589, Update ISSUE_REF_PATTERN validation to reject any issueRef containing consecutive dots, while preserving the existing allowed-character rules. Ensure this boundary validation runs before buildWorktreeNaming and worktree/task reservation so values such as 7..9 produce the direct validation error.src/workflow/domain/task.ts (2)
150-153: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe
task === undefinedfallback makes the fail-closed branch ingetTaskStatusForWorkspaceunreachable.
findActiveTaskAtWorktreePathreturnsundefinedwhen an active worktree row references a task thatgetTaskcannot resolve. Both call sites then treat that as "no active task here":
startTask(Line 176) lets a new task start on a physical worktree that still has an active worktree row.getTaskStatusForWorkspace(Line 412) returnsactive: false, so the explicit error at Line 418 (references task ... which is missing from the store) can never run.Consider returning the dangling worktree so callers can decide. The store enforces a FK from worktrees to tasks, so this state should not occur; making it visible instead of silent matches the fail-closed intent stated in the function comment.
♻️ Proposed change
-): { task: TaskRecord; worktree: WorktreeRecord } | undefined { +): { task: TaskRecord | undefined; worktree: WorktreeRecord } | undefined { const worktree = store .listWorktreesForInstance(instanceId) .find((candidate) => candidate.status === "active" && candidate.canonicalPath === canonicalWorktreePath); if (worktree === undefined) return undefined; - const task = store.getTask(worktree.taskId); - if (task === undefined) return undefined; - return { task, worktree }; + return { task: store.getTask(worktree.taskId), worktree }; }Callers then handle
task === undefinedexplicitly:startTaskrejects withactive-task-in-workspace, andgetTaskStatusForWorkspacereturns its existingok: falsereason.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workflow/domain/task.ts` around lines 150 - 153, Update findActiveTaskAtWorktreePath to return the active worktree alongside an undefined task instead of returning undefined when store.getTask cannot resolve the task. Then handle task === undefined explicitly in startTask by rejecting with active-task-in-workspace, while preserving getTaskStatusForWorkspace’s existing missing-task error response and fail-closed behavior.
175-182: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚖️ Poor tradeoffThe guard reads the store outside the reservation transaction.
findActiveTaskAtWorktreePathruns here, andreserveTask/reserveWorktreerun later in separate transactions. Two concurrentstartTaskcalls with the sameworkspaceRootand differenttaskSlugvalues can both pass this guard. TheircandidateCanonicalPathvalues differ, so theUNIQUEpartial index oncanonical_pathdoes not reject the second one. Both tasks then become active for the same physical worktree, which is the exact state this guard prevents.The window is small and the tools are opt-in, so this may be acceptable for now. If you want the guarantee to hold under concurrency, the check must run inside the same
BEGIN IMMEDIATEtransaction asreserveTask, or the schema needs a uniqueness constraint on the active task per repository instance path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workflow/domain/task.ts` around lines 175 - 182, Move the active-worktree conflict check from the pre-reservation flow into the same BEGIN IMMEDIATE transaction used by reserveTask and reserveWorktree, ensuring the check and reservations are atomic. Update startTask and the reservation transaction path around findActiveTaskAtWorktreePath so concurrent calls for the same physical worktree cannot both proceed; preserve the existing conflict response.src/config.ts (1)
306-308: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider an
optionalBooleanhelper to match the neighboring validation style.Every other field in
normalizeGatewayuses a small validator helper (optionalString,positiveIntegerConfig,stringArrayRecord). TheworkflowTasksfield uses an inline guard plus anas boolean | undefinedcast. A helper removes the cast and keeps the function uniform.♻️ Proposed refactor
- if (value.workflowTasks !== undefined && typeof value.workflowTasks !== "boolean") { - throw new Error("invalid gateway workflowTasks"); - } return { workspaceRoot, @@ - workflowTasks: value.workflowTasks as boolean | undefined, + workflowTasks: optionalBoolean(value.workflowTasks, "invalid gateway workflowTasks"), };Add the helper next to
optionalString:function optionalBoolean(value: unknown, message: string): boolean | undefined { if (value === undefined) return undefined; if (typeof value !== "boolean") throw new Error(message); return value; }Also applies to: 323-323
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config.ts` around lines 306 - 308, Refactor normalizeGateway to add an optionalBoolean helper alongside optionalString, validating undefined or boolean values and throwing the supplied message otherwise. Replace the inline workflowTasks type guard and boolean cast with this helper, preserving the existing error message and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/local-tools.test.ts`:
- Around line 526-529: Assert that the initial `mottainai_task_start` result in
the test succeeds before accessing `started.worktree`; validate its success
status and fail immediately with the returned failure details when it is not
successful. Only then cast or read the worktree and pass its canonical path to
the second call.
In `@src/local-tools.ts`:
- Around line 145-146: In src/local-tools.ts lines 145-146, update the
mottainai_task_start and mottainai_task_status arms in callLocalTool to check
config.workflowTasks and throw before resolving workflowStore or calling
defaultWorkflowStore, while retaining the existing guards in taskStartToolImpl
and taskStatusToolImpl. In src/local-tools.test.ts lines 437-455, pass an
explicit openWorkflowStore() instance to both disabled-feature tests so they
never open the default state database.
- Around line 163-172: Replace the resolved-store cache used by
defaultWorkflowStore with a cached initialization promise so concurrent callers
share one import, WorkflowSqliteStateStore construction, and init sequence.
Return the promise result while preserving the existing store behavior, and
reset the cached promise to undefined when initialization rejects so later calls
can retry.
- Line 103: Update the tool annotation for mottainai_task_start by setting
openWorldHint to true, while preserving the existing values for readOnlyHint,
destructiveHint, and idempotentHint.
In `@src/workflow/domain/task.test.ts`:
- Around line 357-362: Register store1 cleanup with t.after immediately after
store1.init(), matching the existing store2 pattern, so it runs even when
startTask returns early; then remove the later direct store1.close() or avoid
duplicate cleanup. Confirm WorkflowSqliteStateStore.close() is idempotent before
relying on the callback.
---
Nitpick comments:
In `@src/config.ts`:
- Around line 306-308: Refactor normalizeGateway to add an optionalBoolean
helper alongside optionalString, validating undefined or boolean values and
throwing the supplied message otherwise. Replace the inline workflowTasks type
guard and boolean cast with this helper, preserving the existing error message
and behavior.
In `@src/local-tools.ts`:
- Line 589: Update ISSUE_REF_PATTERN validation to reject any issueRef
containing consecutive dots, while preserving the existing allowed-character
rules. Ensure this boundary validation runs before buildWorktreeNaming and
worktree/task reservation so values such as 7..9 produce the direct validation
error.
In `@src/workflow/domain/task.test.ts`:
- Line 263: Replace the regex-based assertion involving the store-generated
outer.task.taskId with a plain substring assertion, while preserving the
expected "active-task-in-workspace" reason validation in the surrounding test.
In `@src/workflow/domain/task.ts`:
- Around line 150-153: Update findActiveTaskAtWorktreePath to return the active
worktree alongside an undefined task instead of returning undefined when
store.getTask cannot resolve the task. Then handle task === undefined explicitly
in startTask by rejecting with active-task-in-workspace, while preserving
getTaskStatusForWorkspace’s existing missing-task error response and fail-closed
behavior.
- Around line 175-182: Move the active-worktree conflict check from the
pre-reservation flow into the same BEGIN IMMEDIATE transaction used by
reserveTask and reserveWorktree, ensuring the check and reservations are atomic.
Update startTask and the reservation transaction path around
findActiveTaskAtWorktreePath so concurrent calls for the same physical worktree
cannot both proceed; preserve the existing conflict response.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 19c4543a-a905-45d8-bb6d-07a98de84072
📒 Files selected for processing (7)
src/code-search.test.tssrc/config.test.tssrc/config.tssrc/local-tools.test.tssrc/local-tools.tssrc/workflow/domain/task.test.tssrc/workflow/domain/task.ts
| const started = structured(await callLocalTool( | ||
| "mottainai_task_start", { taskSlug: "outer" }, workflowConfig(config), store, undefined, undefined, wfStore, | ||
| )); | ||
| const worktree = started.worktree as { canonicalPath: string }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert that the first task_start succeeded before reading its worktree.
The cast on Line 529 hides a failure. If task_start returns status: "failed", started.worktree is undefined, and Line 532 passes workspaceRoot: undefined into the second call. The test then fails somewhere downstream with an unrelated message instead of at the real cause.
💚 Proposed fix
const started = structured(await callLocalTool(
"mottainai_task_start", { taskSlug: "outer" }, workflowConfig(config), store, undefined, undefined, wfStore,
));
+ assert.equal(started.status, "success");
const worktree = started.worktree as { canonicalPath: string };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const started = structured(await callLocalTool( | |
| "mottainai_task_start", { taskSlug: "outer" }, workflowConfig(config), store, undefined, undefined, wfStore, | |
| )); | |
| const worktree = started.worktree as { canonicalPath: string }; | |
| const started = structured(await callLocalTool( | |
| "mottainai_task_start", { taskSlug: "outer" }, workflowConfig(config), store, undefined, undefined, wfStore, | |
| )); | |
| assert.equal(started.status, "success"); | |
| const worktree = started.worktree as { canonicalPath: string }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/local-tools.test.ts` around lines 526 - 529, Assert that the initial
`mottainai_task_start` result in the test succeeds before accessing
`started.worktree`; validate its success status and fail immediately with the
returned failure details when it is not successful. Only then cast or read the
worktree and pass its canonical path to the second call.
| inputSchema: { type: "object", properties: { | ||
| taskSlug: { type: "string" }, issueRef: { type: "string" }, | ||
| }, required: ["taskSlug"] }, outputSchema: OUTPUT_SCHEMA, | ||
| annotations: { readOnlyHint: false, destructiveHint: false, idempotentHint: false, openWorldHint: false }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare annotations across worktree-creating local tools.
rg -n -B4 -A2 'annotations:' src/local-tools.ts
# Inspect the remote-facing paths reached by startTask.
ast-grep run --pattern 'async function checkStaleBaseBranch($$$) { $$$ }' --lang typescript src/workflow/domain/task.ts
ast-grep run --pattern 'async function runBootstrap($$$) { $$$ }' --lang typescript
rg -n 'ls-remote|fetch|origin/' src/workflow/domain/task.tsRepository: yohn-jp/mottainai
Length of output: 5576
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- task.ts outline ---'
ast-grep outline src/workflow/domain/task.ts
printf '%s\n' '--- startTask and bootstrap call sites ---'
rg -n -B8 -A20 'startTask|runBootstrap|checkStaleBaseBranch|bootstrapMode' src/workflow/domain/task.ts src
printf '%s\n' '--- local-tools task registration and handler ---'
cat -n src/local-tools.ts | sed -n '75,115p'
rg -n -B10 -A35 'mottainai_task_start|taskStartTool|startTask\(' srcRepository: yohn-jp/mottainai
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- relevant policy defaults and tool implementation ---'
rg -n -B8 -A18 'bootstrapMode|standard|taskStartTool|mottainai_task_start|worktreeNewTool' src/workflow/policy src/local-tools.ts
printf '%s\n' '--- exact task-start annotation and handler ---'
sed -n '95,107p' src/local-tools.ts
sed -n '585,635p' src/local-tools.ts
printf '%s\n' '--- exact bootstrap implementation ---'
sed -n '74,153p' src/workflow/git/worktree.ts
printf '%s\n' '--- deterministic bootstrap reachability check ---'
python3 - <<'PY'
from pathlib import Path
text = Path("src/workflow/git/worktree.ts").read_text()
assert 'const BOOTSTRAP_COMMAND = "pnpm install --frozen-lockfile";' in text
assert 'if (mode === "automatic")' in text
assert 'return { mode, command: BOOTSTRAP_COMMAND, shouldExecute: true' in text
assert 'runProgram(program, args, worktreePath' in text
print("automatic bootstrap executes:", "pnpm install --frozen-lockfile")
print("execution environment:", "PATH, HOME, LANG, TMPDIR, NODE_ENV allowlist")
PYRepository: yohn-jp/mottainai
Length of output: 31874
Set openWorldHint to true for mottainai_task_start.
checkStaleBaseBranch reads only local tracking refs. However, bootstrapMode: "automatic" runs pnpm install --frozen-lockfile, which can contact package registries.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/local-tools.ts` at line 103, Update the tool annotation for
mottainai_task_start by setting openWorldHint to true, while preserving the
existing values for readOnlyHint, destructiveHint, and idempotentHint.
Source: Coding guidelines
| let defaultWorkflowStoreInstance: WorkflowStateStore | undefined; | ||
| async function defaultWorkflowStore(): Promise<WorkflowStateStore> { | ||
| if (defaultWorkflowStoreInstance === undefined) { | ||
| const { WorkflowSqliteStateStore } = await import("./workflow/state/sqlite-store.js"); | ||
| const created = new WorkflowSqliteStateStore(); | ||
| created.init(); | ||
| defaultWorkflowStoreInstance = created; | ||
| } | ||
| return defaultWorkflowStoreInstance; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cache the promise, not the resolved store, to avoid a double-initialization race.
defaultWorkflowStore is async, and it assigns defaultWorkflowStoreInstance only after two await points. Two concurrent mottainai_task_start / mottainai_task_status calls both observe undefined, both run await import(...), and both call new WorkflowSqliteStateStore() and created.init(). The second assignment overwrites the first. The first store keeps an open SQLite handle plus its WAL and SHM files, and nothing ever closes it.
The MCP server can dispatch tool calls concurrently, so this path is reachable. Cache the in-flight promise so initialization happens exactly once.
🔒️ Proposed fix
-let defaultWorkflowStoreInstance: WorkflowStateStore | undefined;
-async function defaultWorkflowStore(): Promise<WorkflowStateStore> {
- if (defaultWorkflowStoreInstance === undefined) {
- const { WorkflowSqliteStateStore } = await import("./workflow/state/sqlite-store.js");
- const created = new WorkflowSqliteStateStore();
- created.init();
- defaultWorkflowStoreInstance = created;
- }
- return defaultWorkflowStoreInstance;
-}
+let defaultWorkflowStorePromise: Promise<WorkflowStateStore> | undefined;
+function defaultWorkflowStore(): Promise<WorkflowStateStore> {
+ // 生成済みインスタンスではなく生成中の promise を保持する — await の間に
+ // 同時呼び出しが入ると store が二重に init() され、先に作った SQLite handle が
+ // close されないまま漏れるため。
+ defaultWorkflowStorePromise ??= (async () => {
+ const { WorkflowSqliteStateStore } = await import("./workflow/state/sqlite-store.js");
+ const created = new WorkflowSqliteStateStore();
+ created.init();
+ return created;
+ })();
+ return defaultWorkflowStorePromise;
+}If init() throws, reset defaultWorkflowStorePromise to undefined in a .catch so a later call can retry.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let defaultWorkflowStoreInstance: WorkflowStateStore | undefined; | |
| async function defaultWorkflowStore(): Promise<WorkflowStateStore> { | |
| if (defaultWorkflowStoreInstance === undefined) { | |
| const { WorkflowSqliteStateStore } = await import("./workflow/state/sqlite-store.js"); | |
| const created = new WorkflowSqliteStateStore(); | |
| created.init(); | |
| defaultWorkflowStoreInstance = created; | |
| } | |
| return defaultWorkflowStoreInstance; | |
| } | |
| let defaultWorkflowStorePromise: Promise<WorkflowStateStore> | undefined; | |
| function defaultWorkflowStore(): Promise<WorkflowStateStore> { | |
| // 生成済みインスタンスではなく生成中の promise を保持する — await の間に | |
| // 同時呼び出しが入ると store が二重に init() され、先に作った SQLite handle が | |
| // close されないまま漏れるため。 | |
| defaultWorkflowStorePromise ??= (async () => { | |
| const { WorkflowSqliteStateStore } = await import("./workflow/state/sqlite-store.js"); | |
| const created = new WorkflowSqliteStateStore(); | |
| created.init(); | |
| return created; | |
| })(); | |
| return defaultWorkflowStorePromise; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/local-tools.ts` around lines 163 - 172, Replace the resolved-store cache
used by defaultWorkflowStore with a cached initialization promise so concurrent
callers share one import, WorkflowSqliteStateStore construction, and init
sequence. Return the promise result while preserving the existing store
behavior, and reset the cached promise to undefined when initialization rejects
so later calls can retry.
| const store1 = new WorkflowSqliteStateStore({ dbPath }); | ||
| store1.init(); | ||
| const started = await startTask({ workspaceRoot: root, store: store1, policy: standardPolicy(), taskSlug: "restart-check" }); | ||
| assert.equal(started.ok, true); | ||
| if (!started.ok) return; | ||
| store1.close(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Register the store1 cleanup before the early return.
If startTask fails, Line 361 returns before store1.close() on Line 362. The SQLite handle and its WAL files then stay open for the rest of the test process. Register the close with t.after right after init(), the same way the test already does for store2.
🛡️ Proposed fix
const store1 = new WorkflowSqliteStateStore({ dbPath });
store1.init();
+ t.after(() => store1.close());
const started = await startTask({ workspaceRoot: root, store: store1, policy: standardPolicy(), taskSlug: "restart-check" });
assert.equal(started.ok, true);
if (!started.ok) return;
store1.close();WorkflowSqliteStateStore.close() must tolerate a second call for this to be safe. Confirm that before applying.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/workflow/domain/task.test.ts` around lines 357 - 362, Register store1
cleanup with t.after immediately after store1.init(), matching the existing
store2 pattern, so it runs even when startTask returns early; then remove the
later direct store1.close() or avoid duplicate cleanup. Confirm
WorkflowSqliteStateStore.close() is idempotent before relying on the callback.
…, new module, deprecation, CLI) Fills in the gaps flagged on PR #64 against Issue #34's actual acceptance criteria: the previous mottainai_task_start/task_status lived directly in src/local-tools.ts and had no policy-explain counterpart or CLI parity. - New src/workflow/commands/mcp-tools.ts exposes mottainai_workflow_policy_explain, mottainai_workflow_task_start, and mottainai_workflow_task_status as their own Tool[] + dispatch family (mirroring src/local-tools.ts's pattern), replacing the earlier mottainai_task_start/task_status pair removed from local-tools.ts. Wired into src/proxy.ts's tool listing/dispatch/risk-lookup alongside the existing adaptive/broker/codeSearch tool families, gated by gateway.workflowTasks. - mottainai_workflow_policy_explain resolves the genuine RuleMode fields (protectedBranchRule.*, worktree.{required,issueRequired, multipleActiveTasksPerIssue,multipleWorktreesPerTask,staleBaseBranch}, cleanup.*) through resolve.ts's resolveRule(), showing value/mode/authority/ weakening per rule (src/workflow/policy/explain.ts). protectedBranches, controlPlaneRole, stagingMode, and worktree.bootstrapMode are returned as plain descriptive values since the schema has no associated mode for them. - src/cli.ts gains `policy explain`, `task start`, and `task status` commands, sharing the same default state DB as the MCP tools so CLI- and MCP-started tasks are visible to each other. - mottainai_worktree_new is annotated as deprecated in favor of mottainai_workflow_task_start, unchanged in behavior. - resolveEffectiveWorkflowPolicy() (src/workflow/policy/load.ts) centralizes the "load .mottainai/workflow.json, fall back to the standard preset, fail closed on corruption" logic shared by task start/status and the CLI.
|
Closing — this PR's branch ( Replaced by #69, rebuilt from the same diff on a correctly named branch ( Generated by Claude Code |
2 participants
Summary
Exposes
policy explain,task start, andtask statusas MCP tools and CLI commands, wiring the existing task-lifecycle domain layer (src/workflow/domain/task.ts, policy resolution,WorkflowSqliteStateStore) to a newsrc/workflow/commands/mcp-tools.tsmodule and tosrc/cli.ts, gated behind opt-ingateway.workflowTasks.Linked issue
Closes #34.
An earlier revision of this PR shipped
mottainai_task_start/mottainai_task_statusdirectly insrc/local-tools.tswithoutpolicy explain, without CLI commands, and without deprecatingmottainai_worktree_new— flagged in review as not fully matching #34's acceptance criteria. This revision fills those gaps; see Scope and Implementation below for the one deliberate, documented scope decision that remains (howpolicy explainhandles non-RuleModefields).Scope
Included
src/workflow/commands/mcp-tools.ts:mottainai_workflow_policy_explain,mottainai_workflow_task_start,mottainai_workflow_task_status, following theTool[]+ dispatch-function pattern fromsrc/local-tools.ts. Wired intosrc/proxy.ts's tool listing, dispatch, and risk lookup alongside the existing adaptive/broker/codeSearch tool families — its own family, since it has its own gating (config.workflowTasks) and its own state dependency (WorkflowStateStore).mottainai_workflow_policy_explain: resolves the effective policy for the workspace and, for every genuineRuleModefield (protectedBranchRule.*;worktree.required/issueRequired/multipleActiveTasksPerIssue/multipleWorktreesPerTask/staleBaseBranch;cleanup.*), returns the fullresolve.tsResolvedRule— value, mode, authority (presetvs.repository), and weakening permission. When.mottainai/workflow.jsondeclares apreset, that preset's values become the"preset"authority and the file's own values become"repository", so a declared preset'senforcerule can't be silently weakened by editing the file (nohumanApprovalchannel exists in the file schema yet, so any such weakening attempt is rejected and the preset's stronger mode wins — this isresolveRule()'s existing, tested behavior, now actually exercised).mottainai_workflow_task_start/_status: same behavior as before —task_startnever passesskipWorktree, so it always creates a dedicated worktree/branch off the current branch;task_statusresolves the active task for the calling worktree without a task id, with no side effects. (Renamed from the earliermottainai_task_start/mottainai_task_status, which were removed fromsrc/local-tools.ts.)src/cli.ts:mottainai policy explain [--workspace path],mottainai task start <slug> [--issue ref] [--workspace path],mottainai task status [--workspace path]. These share the same default state DB as the MCP tools (sameresolveStateDbPath()), so a task started via CLI is visible via MCPtask_statusand vice versa.mottainai_worktree_new(src/local-tools.ts) is annotated as deprecated in its description and with a doc comment, pointing atmottainai_workflow_task_start. Not removed, not behaviorally changed — same input schema, same annotations, same implementation.resolveEffectiveWorkflowPolicy()(src/workflow/policy/load.ts) centralizes "load.mottainai/workflow.json, fall back to the built-instandardpreset if missing, fail closed on corruption" fortask_start/task_status/CLI.startTaskrejects starting a second task from inside a worktree that already hosts an active task (active-task-in-workspace), which is also this PR's policy-driven rejection test path.Excluded / scope decisions
policy explain'sResolvedRuleoutput covers only fields that are actuallyRuleMode-shaped insrc/workflow/policy/schema.ts.protectedBranches(string[]),controlPlaneRole,stagingMode, andworktree.bootstrapModehave no associated mode field in the schema at all (confirmed by readingresolve.test.tsandschema.tstogether — theResolvedPolicymapped type inresolve.tsis typed generically enough to wrap these too, but there is no existing builder or schema support for what "mode" would even mean for e.g.stagingMode). Rather than fabricate a mode for these, they're returned as plaindescriptivevalues, documented insrc/workflow/policy/explain.ts's module comment. Extending the schema itself to carry authority/weakening for these fields is left to Child Issue 9a-1, as the original Issue anticipated ("Child Issue 9a-1 later extends these same three tools to their final full spec").task_start/task_statusstill consult the plain effective policy document (resolveEffectiveWorkflowPolicy), not the authority-resolved viewpolicy explainshows — task-start enforcement does not yet route throughresolveRule(). This isn't a regression introduced here (startTasknever consultedresolve.ts, going back to Issue feat: add Issue-bound task and worktree lifecycle #33);policy explainis simply the first real caller of that resolution engine. Noted indocs/workflow-policy.md.Implementation
src/workflow/commands/mcp-tools.ts: the three tools, a lazily dynamic-imported defaultWorkflowStateStoresingleton (tests inject their own),TASK_SLUG_PATTERN/ISSUE_REF_PATTERNboundary validation.src/workflow/policy/explain.ts:explainWorkflowPolicy(), buildsPolicySource<RuleMode>[]per rule field from the preset/repository documents and callsresolveRule().src/workflow/policy/load.ts:resolveEffectiveWorkflowPolicy().src/proxy.ts:isWorkflowCommandflag threaded throughauthorize/dispatch/withRequestId/gatewayToolRisk, parallel to the existingisLocal/isAdaptiveflags;workflowCommandToolsFor(gatewayConfig)added to theListToolsresponse.src/cli.ts:resolveWorkflowWorkspace()(defaults to--workspace, then the current Git repo's top level, then cwd) andopenWorkflowStateStore()(dynamic import, same reasoning as the MCP side).src/local-tools.ts: reverted themottainai_task_start/mottainai_task_statusaddition (moved to the new module);mottainai_worktree_new's deprecation notice is the only remaining change there.Behavioral changes
gateway.workflowTasks(defaultfalse; unchanged for existing configs). When enabled: three new MCP tools, three new CLI commands, none of which change any existing tool/command's behavior.mottainai_worktree_new's tool description now starts with "Deprecated: superseded by mottainai_workflow_task_start." — same schema, same behavior, purely descriptive.Validation
pnpm run typecheckpnpm test(705 pass): unit coverage forexplainWorkflowPolicy(preset-only, repository-only, preset-declared strengthening/weakening, corrupted file),resolveEffectiveWorkflowPolicy, the newmcp-tools.tsdispatch (gating, each tool's happy path, theactive-task-in-workspacepolicy-driven rejection, boundary validation), aproxy.tsend-to-end test (listTools/callToolthrough the real MCPClient/Server, gated listing), and CLI subprocess tests (policy explain,task start/statusround-trip across two invocations, the same-worktree rejection, corrupted-policy fail-closed).pnpm run buildsrc/cli.tsis apackageCheckPathsmatch; deferring to CI's Package check job (not independently reproducible locally beyond typecheck/test/build, which all pass).pnpm run lint,pnpm run architecture:check(73 production files), andpnpm run format:checkalso pass.Risks
mottainai_worktree_new's deprecation is description-only; nothing currently enforces migration off it, and it is not scheduled for removal by this PR.skipWorktree) tasks remain outsideactive-task-in-workspace's detection, sinceTaskRecorddoesn't carry a physical location for that mode. Not reachable via either the MCP tools or the CLI commands added here (both always create a dedicated worktree), but noted for future work if that mode is ever exposed.policy explain's authority resolution is display-only for now (see Excluded); a user readingpolicy explainmight reasonably assume it reflects whattask_startwill enforce, which isn't yet true. Documented indocs/workflow-policy.mdto avoid that misreading.Breaking changes
No.
Migration / compatibility
None required —
workflowTasksdefaults tofalse.Security impact
mottainai_workflow_task_startand the CLI'stask startmutate git state (worktree creation) behind opt-in config, same posture asmottainai_worktree_new.mottainai_workflow_policy_explainandtask statusare read-only. Input (taskSlug,issueRef) is validated against fixed patterns before being interpolated into branch/path names; all git invocations use argv-array subprocess helpers (no shell interpolation).Review focus
policy explain'sResolvedRuleoutput to genuineRuleModefields (rather than fabricating modes forprotectedBranches/controlPlaneRole/stagingMode/bootstrapMode) is an acceptable reading of the acceptance criterion, or whether that should block this PR pending a schema extension.mottainai_workflow_policy_explain/_task_start/_task_statusandpolicy explain/task start/task status.