Add source-checkout CLI task adapter - #31
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds a JSON-only source-checkout CLI for ten task/session commands, with strict parsing, stable error envelopes and exit codes, shared DTO mappings, MCP parity, built-process integration, asset validation, and expanded documentation. ChangesSource-checkout CLI adapter
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
krishna916
left a comment
krishna916
left a comment
There was a problem hiding this comment.
Review against #22
Verdict: not ready to merge. This draft establishes a useful skeleton, but it does not yet satisfy the issue acceptance gate.
High — runtime validation starts too early
parse() only validates the generic token shape and --output json. Required command options, allowed options, status/priority values, edit clear/value conflicts, and triage targets are validated later inside execute(), after createRuntime() has already opened the database. Issue #22 requires syntax to be sufficiently validated before runtime creation. For example, task capture --agent codex --session s --output json currently creates a runtime before discovering the missing --title.
Please introduce a command-specific parsed union/schema and complete validation before createRuntime().
High — unknown options are silently accepted
Because options are collected generically and handlers read only the keys they know, commands such as task get id --bogus value --output json succeed. The authoritative spec requires unknown options to be usage errors. This needs strict per-command allowlists/schema validation, with tests for every command.
High — acceptance coverage is still absent
The PR currently has only three unit tests. Issue #22 explicitly requires built-process arbitrary-CWD tests, isolated RELAY_DB_PATH, stable exit-code categories, all ten command families, stdout/stderr separation, runtime close behavior, edit/no-op/clear semantics, and MCP/CLI parity. The PR description acknowledges these gaps; they remain merge blockers rather than optional follow-up work.
Medium — CLI depends on MCP adapter internals
run-cli.ts imports DTO and change-metadata mappers from interfaces/mcp/mapping. The issue asks the CLI and MCP adapters to reuse adapter-neutral contracts. Move these pure serializers/mappers to a shared contracts/mapping location so neither adapter depends on the other adapter’s namespace.
The core direction is sound, but the PR should remain draft until these items and the full pnpm verify gate pass.
| } finally { | ||
| runtime.close(); | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
runtime.close() can throw after a success or failure envelope has already been written. The outer catch then writes another failure envelope, so stdout contains two JSON documents and violates the “exactly one JSON document” contract. Capture execution and close failures before writing output, then emit exactly one final envelope. Add tests for close failure after both successful execution and command failure.
Luna remediation plan for PR #31Use this as the implementation checklist for addressing the review against issue #22. Do not redesign the CLI contract. Preserve the command surface, exit codes, envelopes, application semantics, and scope already defined in issue #22 and its authoritative implementation comment. Working rules
Task 1 — Introduce a fully validated parsed-command modelGoalComplete all syntax and option validation before Required changes
Command-specific option allowlistsImplement explicit allowlists rather than collecting arbitrary options.
Validation rules
TestsAdd parser-focused tests covering every command:
For every parser failure assert:
Acceptance check
Task 2 — Refactor execution into focused command handlersGoalMake command behavior easy to review and prevent parser/application concerns from being mixed in one large function. Required changes
TestsAdd unit tests for every command handler asserting exact application calls and exact returned payload shape. Important cases:
Task 3 — Remove CLI dependency on MCP adapter internalsGoalMCP and CLI must be sibling adapters sharing neutral contracts/mappers. CLI must not import from Required changes
Tests
Acceptance checkA repository search for imports from Task 4 — Guarantee exactly one JSON envelope even when cleanup failsGoalNo execution path may write two JSON documents to stdout. Current defect
Required design
Required testsAdd explicit tests for:
For every case assert:
Also test a writer spy that would expose accidental second writes. Task 5 — Complete stable error and exit-code coverageGoalProve all issue #22 exit categories and output separation rules. Required mapping
Required changes
TestsAdd one explicit unit test for every exit category and representative error subtype:
Assert exact public error code, exit code, stdout shape, stderr behavior, and runtime cleanup. Task 6 — Complete MCP/CLI parity testsGoalEquivalent MCP and CLI operations must produce equivalent task DTOs, warnings, and change metadata. Required changesCreate Use shared fixtures and the same isolated database/runtime where practical. Cover at minimum:
Compare semantic payloads, not transport wrappers that are intentionally different. The task DTO and change metadata must match exactly. Do not implement separate CLI semantics to make the tests pass. Task 7 — Add built-process CLI integration testsGoalProve the actual built CLI works from source checkout, outside the repository CWD, against an isolated database. Required changesCreate
Required scenarios
Acceptance checkTests invoke only the built artifact, not TypeScript source through Task 8 — Complete repository asset/build validationGoalEnsure the new Required changes
TestsUpdate Task 9 — Finish CLI documentationGoalDocument the implemented contract, not planned or unsupported behavior. Required documentationUpdate
Do not document flags or human-readable output that are not implemented. Task 10 — Run the full verification gate and update the PRRequired commandsRun all of the following from a clean working tree: pnpm test -- tests/unit/interfaces/cli
pnpm test -- tests/integration/cli.test.ts
pnpm test -- tests/integration/mcp-cli-parity.test.ts
pnpm format:check
pnpm lint
pnpm typecheck
pnpm test:coverage
pnpm build
pnpm validate:assets
pnpm verifyIf the repository test runner requires slightly different paths, use the equivalent commands but preserve the same coverage. Final Luna self-reviewBefore marking the work complete, verify manually:
PR update required from LunaAfter implementation:
|
krishna916
left a comment
krishna916
left a comment
There was a problem hiding this comment.
Re-review against issue #22
The previously reported blockers are addressed:
- parsing is now command-specific and completes before runtime creation
- unknown and duplicate options are rejected
- edit clear/value conflicts and enum/limit validation are enforced before opening storage
- CLI/MCP mapping logic is adapter-neutral
- runtime execution and cleanup are collected before emitting one final JSON envelope
- built-process arbitrary-CWD, isolated
RELAY_DB_PATH, command-family, exit-code, architecture, and MCP/CLI parity coverage has been added - the latest CI run passes
I did not find a new merge-blocking code issue in this pass.
Administrative cleanup before merge: resolve the now-addressed inline cleanup thread, update the stale PR description/validation section, and mark the PR ready for review. Since this is the author's own PR, GitHub does not permit a formal approval from this account.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
tests/integration/mcp-cli-parity.test.ts (1)
57-95: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the claimed duplicate-warning parity.
Both applications are empty here, so capture cannot produce a duplicate warning;
callClialso discardswarnings. Seed an equivalent candidate in both fixtures and compare the warning envelopes, not justdata.🤖 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 `@tests/integration/mcp-cli-parity.test.ts` around lines 57 - 95, Update the “matches capture payloads and duplicate warnings” test to seed equivalent existing candidate tasks in both application fixtures before capture, ensuring the new capture triggers a duplicate warning. Extend callCli to return the parsed envelope warnings, then compare CLI and MCP warnings alongside data while preserving the existing payload assertions.src/interfaces/cli/parse-cli.ts (1)
283-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify redundant branch in
readId.The outer condition already establishes
value === undefined || value.startsWith('--'); re-testingvalue?.startsWith('--')inside the block is redundant.♻️ Suggested simplification
function readId(value: string | undefined, label: string): string | undefined { - if (value === undefined || value.startsWith('--')) { - if (value?.startsWith('--')) throw new CliUsageError(`A ${label} is required.`); - return undefined; - } - return boundedText(value, label, MAX_ID_LENGTH); + if (value === undefined) return undefined; + if (value.startsWith('--')) throw new CliUsageError(`A ${label} is required.`); + return boundedText(value, label, MAX_ID_LENGTH); }🤖 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/interfaces/cli/parse-cli.ts` around lines 283 - 289, In readId, simplify the outer guard by handling undefined and option-prefixed values without rechecking value?.startsWith('--') inside the block. Preserve returning undefined for missing values and throwing CliUsageError with the existing label-specific message when a value starts with '--'; keep boundedText validation unchanged.src/interfaces/cli/cli-command.ts (1)
47-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit
TaskLifecycleCommandinto a true discriminated union.
kind: 'task.start' | 'task.complete' | 'task.archive'andaction: 'start' | 'complete' | 'archive'are typed independently, so nothing prevents constructing{ kind: 'task.start', action: 'archive' }.parseTaskLifecycleinparse-cli.tsalways keeps them in sync today, but this shape works against PR objective#1's "discriminated, fully validated parsed-command model" and leaves a foot-gun for any downstream code that switches onactioninstead ofkind(or vice versa).♻️ Suggested tightening
-export interface TaskLifecycleCommand { - readonly kind: 'task.start' | 'task.complete' | 'task.archive'; - readonly id: string; - readonly action: 'start' | 'complete' | 'archive'; -} +export interface TaskStartCommand { + readonly kind: 'task.start'; + readonly id: string; +} +export interface TaskCompleteCommand { + readonly kind: 'task.complete'; + readonly id: string; +} +export interface TaskArchiveCommand { + readonly kind: 'task.archive'; + readonly id: string; +} +export type TaskLifecycleCommand = TaskStartCommand | TaskCompleteCommand | TaskArchiveCommand;🤖 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/interfaces/cli/cli-command.ts` around lines 47 - 51, Replace the independently unioned fields in TaskLifecycleCommand with a discriminated union of three object variants, pairing each kind with its corresponding action: task.start/start, task.complete/complete, and task.archive/archive. Preserve the readonly properties and update any dependent types or usage sites as needed so parseTaskLifecycle and downstream command handling retain the validated pairing.
🤖 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 `@docs/superpowers/plans/2026-07-27-issue-22-cli-task-adapter.md`:
- Around line 152-153: Update the Task 3 file list to replace task-start.ts,
task-complete.ts, and task-archive.ts with
src/interfaces/cli/commands/task-lifecycle.ts, while preserving the existing
task-edit.ts and task-triage.ts entries and other listed modifications.
- Around line 61-73: Update the runCli example so it matches the required
phase-separated flow: handle parse failures and runtime-creation failures
through the single JSON error-envelope path, and ensure runtime.close() failures
are also converted into that envelope without replacing an already-emitted
response. Alternatively, clearly mark this snippet as historical pseudocode
rather than presenting it as the implementation.
In `@README.md`:
- Around line 46-53: Clarify the README instructions before the source-checkout
CLI example to state that pnpm build:node must run from the repository checkout
root, or show the equivalent pnpm --dir invocation for an arbitrary working
directory. Keep the existing CLI invocation and behavior description unchanged.
In `@src/interfaces/cli/main.ts`:
- Line 1: Add the Node shebang as the first line of the CLI entrypoint
containing createTaskRuntime, before all imports, so the generated relay binary
is directly executable. Preserve the existing import and runtime behavior.
---
Nitpick comments:
In `@src/interfaces/cli/cli-command.ts`:
- Around line 47-51: Replace the independently unioned fields in
TaskLifecycleCommand with a discriminated union of three object variants,
pairing each kind with its corresponding action: task.start/start,
task.complete/complete, and task.archive/archive. Preserve the readonly
properties and update any dependent types or usage sites as needed so
parseTaskLifecycle and downstream command handling retain the validated pairing.
In `@src/interfaces/cli/parse-cli.ts`:
- Around line 283-289: In readId, simplify the outer guard by handling undefined
and option-prefixed values without rechecking value?.startsWith('--') inside the
block. Preserve returning undefined for missing values and throwing
CliUsageError with the existing label-specific message when a value starts with
'--'; keep boundedText validation unchanged.
In `@tests/integration/mcp-cli-parity.test.ts`:
- Around line 57-95: Update the “matches capture payloads and duplicate
warnings” test to seed equivalent existing candidate tasks in both application
fixtures before capture, ensuring the new capture triggers a duplicate warning.
Extend callCli to return the parsed envelope warnings, then compare CLI and MCP
warnings alongside data while preserving the existing payload assertions.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8143cc2e-a70c-48fd-954b-773877138190
📒 Files selected for processing (42)
README.mddocs/cli-reference.mddocs/superpowers/plans/2026-07-27-issue-22-cli-task-adapter.mddocs/superpowers/tasks/2026-07-27-pr-31-review-tracker.mdpackage.jsonscripts/validate-repository-assets.tssrc/interfaces/cli/cli-command.tssrc/interfaces/cli/commands/command-result.tssrc/interfaces/cli/commands/session-captures.tssrc/interfaces/cli/commands/task-capture.tssrc/interfaces/cli/commands/task-edit.tssrc/interfaces/cli/commands/task-find-similar.tssrc/interfaces/cli/commands/task-get.tssrc/interfaces/cli/commands/task-lifecycle.tssrc/interfaces/cli/commands/task-list.tssrc/interfaces/cli/commands/task-triage.tssrc/interfaces/cli/execute-cli-command.tssrc/interfaces/cli/main.tssrc/interfaces/cli/output/cli-errors.tssrc/interfaces/cli/output/cli-result.tssrc/interfaces/cli/parse-cli.tssrc/interfaces/cli/run-cli.tssrc/interfaces/contracts/change-metadata.tssrc/interfaces/contracts/task-dto.tssrc/interfaces/http/task-dto.tssrc/interfaces/mcp/mapping/task-mcp-dto.tssrc/interfaces/mcp/tools/register-read-tools.tssrc/interfaces/mcp/tools/task-archive.tssrc/interfaces/mcp/tools/task-capture.tssrc/interfaces/mcp/tools/task-complete.tssrc/interfaces/mcp/tools/task-edit.tssrc/interfaces/mcp/tools/task-start.tssrc/interfaces/mcp/tools/task-triage.tstests/integration/cli.test.tstests/integration/mcp-cli-parity.test.tstests/unit/interfaces/cli/architecture.test.tstests/unit/interfaces/cli/cli-errors.test.tstests/unit/interfaces/cli/command-handlers.test.tstests/unit/interfaces/cli/parse-cli.test.tstests/unit/interfaces/cli/run-cli.test.tstests/unit/scripts/validate-repository-assets.test.tstsup.config.ts
💤 Files with no reviewable changes (1)
- src/interfaces/mcp/mapping/task-mcp-dto.ts
1 participant
Summary
relayCLI entry point and JSON envelope runnerValidation
pnpm typecheckpnpm test -- tests/unit/interfaces/cli/run-cli.test.tsFollow-up
This draft intentionally records remaining issue #22 acceptance work identified by review: strict per-command option validation, edit clear/value validation, built-process and MCP/CLI parity integration coverage, and CLI asset validation.
Summary by CodeRabbit
New Features
relaysource-checkout CLI for task and session workflows (capture, list/get, find similar, edit, triage, start/complete/archive, session captures).RELAY_DB_PATHand works from any working directory.Documentation
Tests