test: verify MCP and CLI compatibility and integrations - #34
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds isolated MCP/CLI test infrastructure, broad adapter parity and persistence coverage, stricter canonical skill and integration-asset validation, verification records, and build-before-test scripts. ChangesAgent integration verification
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant RelayCLI
participant HTTPAdapter
participant SQLiteDatabase
MCPClient->>SQLiteDatabase: Capture task
RelayCLI->>SQLiteDatabase: Edit and retrieve task
HTTPAdapter->>SQLiteDatabase: Verify shared persisted state
MCPClient->>SQLiteDatabase: Retrieve task after restart
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 verdict: Changes requested
Critical — pnpm verify cannot pass from a clean checkout
The new built-process tests execute dist/mcp/main.js and dist/cli/main.js, but the authoritative verify script still runs pnpm test:coverage before pnpm build. Since dist/ is gitignored, a clean checkout has no built artifacts, so the new tests fail before the build step is reached.
The PR description also says the literal corepack pnpm verify gate was not completed. That means issue #25's explicit acceptance gate—pnpm verify from a clean frozen install—has not been demonstrated.
Please make the test/build ordering self-contained and deterministic. Prefer a focused script such as test:integration:built that builds once before built-artifact tests, then wire it into verify without causing coverage to depend on stale local dist. Add a regression check that removes dist before invoking the authoritative gate.
High — temporary runtime assumes an untracked tmp/ parent already exists
createAgentTestRuntime() calls mkdtemp(join(checkoutPath, 'tmp', 'relay-agent-verification-')) without first creating <checkout>/tmp. mkdtemp requires the parent directory to exist, while tmp/ is neither tracked nor created by setup. This can fail with ENOENT on the clean checkout this issue is specifically supposed to verify.
Create the parent directory first, or use os.tmpdir() and keep the same isolation/cleanup guarantees. Add a test that removes the parent before creating the runtime.
High — storage-error parity is claimed but not verified across adapters
The storage-error test asserts a structured CLI STORAGE_ERROR, but for MCP it only asserts that createMcpTestClient(runtime) rejects. It does not verify the stable MCP error code, external error shape, leakage behavior, or equality with the CLI contract. Despite that, the PR matrix marks storage-error parity as PASS.
Either provide a deterministic MCP operation-level storage failure and compare normalized errors, or explicitly document startup failure as a distinct non-parity scenario and stop claiming storage-error parity. The test and evidence matrix must agree.
Verification note
The PR currently has only a CodeRabbit success status and remains draft. Live Codex and Claude validation is honestly marked unverified, which is appropriate, but the clean authoritative repository gate must pass before this is ready for human client verification or merge.
Luna remediation implementation plan
GoalFix the three merge-blocking review findings:
Files expected to change
Do not change application contracts, MCP tool schemas, CLI schemas, database defaults, coverage thresholds, or the canonical skill/vendor policies. Task 1 — Make built-process tests runnable from a clean checkoutProblemThe new integration tests spawn: But Required implementationModify {
"scripts": {
"test": "pnpm build:node && vitest run",
"test:coverage": "pnpm build:node && vitest run --coverage"
}
}Do not add a Vitest hook that builds production artifacts during test collection. Do not commit Steps
Run from the PR branch: pnpm build:clean
pnpm test -- tests/unit/support/cli-test-process.test.tsExpected before the fix: FAIL because the built CLI entry point does not exist. Record the actual failure text in your working notes; do not put transient machine paths in documentation.
Change only the
Run: pnpm build:clean
pnpm test -- tests/unit/support/cli-test-process.test.tsExpected after the fix:
Run: pnpm build:clean
pnpm test:coverage -- tests/unit/support/mcp-test-client.test.tsExpected: the Node build runs first and the focused coverage test passes.
git add package.json
git commit -m "test: build node artifacts before process tests"Task 2 — Create the temporary parent before calling
|
krishna916
left a comment
krishna916
left a comment
There was a problem hiding this comment.
Follow-up review verdict: One blocking CI failure remains
The three original review findings are substantively addressed:
- built Node artifacts are now created before
testandtest:coverage; <checkout>/tmpis created beforemkdtemp();- storage-error parity now uses an initialized MCP client plus a deterministic
BEGIN IMMEDIATElock and compares structured MCP/CLISTORAGE_ERRORresults.
However, the latest authoritative GitHub Actions run for head 2ffc166 fails, so the PR is not ready yet.
High — runtime isolation test incorrectly assumes the repository is outside the OS home directory
tests/unit/support/agent-test-runtime.test.ts still asserts:
expect(runtime.databasePath).not.toContain(homedir());On the Ubuntu GitHub runner, the repository is checked out under /home/runner/work/..., so the intentionally repository-local disposable path also contains /home/runner. CI therefore fails with 1 failed test and 511 passed.
This assertion does not prove the safety property issue #25 needs. The required property is that the test uses the generated disposable path rather than Relay's platform-default user database—not that the checkout itself lives outside the user's home directory.
Please replace the home-directory assertion with deterministic path-boundary assertions, for example:
const disposableRoot = dirname(dirname(runtime.databasePath));
const repositoryTemporaryRoot = resolve(
dirname(fileURLToPath(import.meta.url)),
'../../..',
'tmp',
);
expect(runtime.databasePath).toBe(join(disposableRoot, 'data', 'relay.db'));
expect(relative(repositoryTemporaryRoot, disposableRoot)).not.toMatch(/^\.\.(?:[\\/]|$)/);
expect(runtime.environment().RELAY_DB_PATH).toBe(runtime.databasePath);Also assert it is not equal to the configured platform-default database path using the existing database-path resolver, if that resolver can be called without touching the filesystem. Do not use substring comparison against homedir().
Then run and record:
corepack pnpm exec vitest run tests/unit/support/agent-test-runtime.test.ts
rm -rf dist coverage tmp
corepack pnpm verifyFinally push the fix and require a green GitHub Actions run. Local Windows success is useful, but issue #25's clean-checkout acceptance gate is not met while the repository's Linux CI is red.
No additional contract or architecture problems were found in the remediation changes reviewed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/support/cli-test-process.ts (1)
42-46: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTimeout doesn't wait for the child to actually exit.
child.kill()sends SIGTERM and the promise rejects immediately; the child process may ignore SIGTERM and keep running (still holding the shared SQLite file) sinceclose's handler is a no-op oncesettledis true. Since a unit test explicitly exercisestimeoutMs: 1(cli-test-process.test.tsLines 37-46), this can leave orphaned processes across CI runs.Suggested hardening: escalate to SIGKILL if the process doesn't exit
const timeout = setTimeout(() => { settled = true; child.kill(); + const forceKill = setTimeout(() => child.kill('SIGKILL'), 2_000); + child.once('exit', () => clearTimeout(forceKill)); reject(new Error(`Relay CLI timed out after ${options.timeoutMs ?? 30_000}ms.`)); }, options.timeoutMs ?? 30_000);🤖 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/support/cli-test-process.ts` around lines 42 - 46, Update the timeout handling in the CLI child-process wrapper so it does not leave a process running after the initial child.kill() and immediate rejection. After requesting termination, wait for the child to exit and escalate to SIGKILL if it remains alive, while preserving the existing timeout rejection and close-event behavior in the surrounding process helper.tests/support/external-contract-normalizers.ts (1)
13-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWhitelisting envelope keys can hide parity divergence.
Both success normalizers keep only
schemaVersion,data, andwarnings, so any extra (or renamed) top-level envelope field on one adapter is silently discarded beforetoEqualcomparisons intests/integration/mcp-cli-parity.test.ts(e.g. Line 340, Line 486). The plan's own checklist requires normalizers that "unwrap transports without hiding parity failures" (docs/superpowers/plans/2026-07-29-issue-25-mcp-cli-compatibility-verification.mdLine 754, Line 778). Consider asserting the envelope key set instead of projecting it.♻️ Sketch: reject unexpected envelope keys
export function normalizeCliSuccess(value: unknown): ExternalOperationResult { const envelope = record(value, 'CLI result'); + assertKeys(envelope, ['schemaVersion', 'data', 'warnings'], 'CLI result'); return { schemaVersion: number(envelope.schemaVersion), data: envelope.data, warnings: array(envelope.warnings), }; }function assertKeys(envelope: Record<string, unknown>, allowed: readonly string[], label: string) { const unexpected = Object.keys(envelope).filter((key) => !allowed.includes(key)); if (unexpected.length > 0) { throw new Error(`${label} has unexpected fields: ${unexpected.join(', ')}`); } }🤖 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/support/external-contract-normalizers.ts` around lines 13 - 30, Update normalizeCliSuccess and normalizeMcpSuccess to validate the unwrapped envelope keys instead of silently projecting only schemaVersion, data, and warnings. Add or reuse an assertKeys-style helper to reject unexpected or renamed fields with a clear error, then preserve the existing normalized result for the allowed keys.tests/integration/mcp-cli-parity.test.ts (1)
259-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRuntime and client lifecycles are not owned by a single cleanup boundary. Both suites create
createAgentTestRuntime()(and then the MCP client) outsidetry, so any startup failure leaks a disposable directory under<checkout>/tmp.
tests/integration/mcp-cli-parity.test.ts#L259-L263: introduce awithAgentRuntime(async (runtime, client) => …)helper that creates and tears down both resources, and use it for all ten tests in this suite.tests/unit/support/mcp-test-client.test.ts#L6-L11: adopt the same helper (or move creation insidetry) for both tests.🤖 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 259 - 263, Ensure runtime and MCP client creation is covered by one cleanup boundary: add or reuse a withAgentRuntime helper that creates both resources, invokes the test callback, and always tears them down. Apply it to all ten tests in tests/integration/mcp-cli-parity.test.ts at the anchor site and to both tests in tests/unit/support/mcp-test-client.test.ts at the sibling site; alternatively, move creation inside each test’s existing try/finally.tests/unit/support/mcp-test-client.test.ts (1)
34-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe sanitization regex is weaker than it looks.
With the
iflag,[A-Z]:\\Users\\also matches lowercase drive letters (fine) butSQL/stackmatch common harmless words, and the assertion is trivially satisfied whilestderr()is empty (see the root cause intests/support/mcp-test-client.ts). Consider asserting on a concrete non-empty stderr snapshot once capture works.🤖 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/unit/support/mcp-test-client.test.ts` around lines 34 - 35, Strengthen the unknown-tool stderr assertion in the test around client.callTool by first ensuring the MCP test client’s stderr capture produces the expected non-empty output, then assert against a concrete sanitized stderr snapshot or specific redaction markers instead of broad SQL/stack/path keywords. Update the related capture logic in MCPTestClient only as needed to make this assertion meaningful.
🤖 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 `@scripts/validate-agent-integration-assets.ts`:
- Around line 120-121: The forbidden-action checks in
scripts/validate-agent-integration-assets.ts must distinguish affirmative
autonomous mutations from explicit prohibitions: update the regex near lines
120-121 and the deletion check near lines 135-140 to ignore negated forms while
still rejecting positive guidance, and add acceptance tests covering “must not
autonomously edit” and “Do not delete the SQLite database.”
In `@tests/integration/agent-workflow-e2e.test.ts`:
- Around line 98-148: Restructure the test body around a single outer
try/finally so every failure after createAgentTestRuntime() still reaches
runtime.close(). Keep the two MCP client phases and their individual client
cleanup, but move final runtime cleanup to the outer finally and guard it with
optional chaining. Update the restart persistence test beginning with
createAgentTestRuntime and retain the existing assertions and phase-specific
behavior.
---
Nitpick comments:
In `@tests/integration/mcp-cli-parity.test.ts`:
- Around line 259-263: Ensure runtime and MCP client creation is covered by one
cleanup boundary: add or reuse a withAgentRuntime helper that creates both
resources, invokes the test callback, and always tears them down. Apply it to
all ten tests in tests/integration/mcp-cli-parity.test.ts at the anchor site and
to both tests in tests/unit/support/mcp-test-client.test.ts at the sibling site;
alternatively, move creation inside each test’s existing try/finally.
In `@tests/support/cli-test-process.ts`:
- Around line 42-46: Update the timeout handling in the CLI child-process
wrapper so it does not leave a process running after the initial child.kill()
and immediate rejection. After requesting termination, wait for the child to
exit and escalate to SIGKILL if it remains alive, while preserving the existing
timeout rejection and close-event behavior in the surrounding process helper.
In `@tests/support/external-contract-normalizers.ts`:
- Around line 13-30: Update normalizeCliSuccess and normalizeMcpSuccess to
validate the unwrapped envelope keys instead of silently projecting only
schemaVersion, data, and warnings. Add or reuse an assertKeys-style helper to
reject unexpected or renamed fields with a clear error, then preserve the
existing normalized result for the allowed keys.
In `@tests/unit/support/mcp-test-client.test.ts`:
- Around line 34-35: Strengthen the unknown-tool stderr assertion in the test
around client.callTool by first ensuring the MCP test client’s stderr capture
produces the expected non-empty output, then assert against a concrete sanitized
stderr snapshot or specific redaction markers instead of broad SQL/stack/path
keywords. Update the related capture logic in MCPTestClient only as needed to
make this assertion meaningful.
🪄 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: 24e0271d-38a6-4972-a191-e9525f240275
📒 Files selected for processing (24)
docs/agent-integration-verification.mddocs/superpowers/plans/2026-07-29-issue-25-mcp-cli-compatibility-verification.mddocs/superpowers/tasks/2026-07-30-pr-34-review-tracker.mdintegrations/claude-code/README.mdintegrations/generic-cli/README.mdpackage.jsonscripts/validate-agent-integration-assets.tstests/fixtures/agent-integrations/valid/integrations/claude-code/README.mdtests/fixtures/agent-integrations/valid/integrations/codex/README.mdtests/fixtures/agent-integrations/valid/integrations/generic-cli/README.mdtests/fixtures/agent-integrations/valid/integrations/generic-mcp/README.mdtests/fixtures/contracts/agent-workflow.tstests/integration/agent-workflow-e2e.test.tstests/integration/database-path-parity.test.tstests/integration/mcp-cli-parity.test.tstests/support/agent-test-runtime.tstests/support/cli-test-process.tstests/support/external-contract-normalizers.tstests/support/mcp-test-client.tstests/unit/scripts/validate-agent-integration-assets.test.tstests/unit/scripts/validate-repository-assets.test.tstests/unit/support/agent-test-runtime.test.tstests/unit/support/cli-test-process.test.tstests/unit/support/mcp-test-client.test.ts
1 participant
Closes #25
Summary
RELAY_DB_PATH.Automated 16-scenario matrix
Validation
corepack pnpm install --frozen-lockfilecompleted with pnpm 10.2.0.corepack pnpm verifypassed from a clean generated-output state beginning withdist/deleted.BEGIN IMMEDIATEon the shared disposable database, ran built CLI/MCP writes concurrently, and verified equal normalizedSTORAGE_ERRORcontracts, CLI exit code 5, and sanitized external output.Client validation status
Access is deniedforcodex --version.claudeandclaude-codewere unavailable.Draft PR only; do not merge.
Summary by CodeRabbit
Documentation
Bug Fixes
Tests