Skip to content

Remote query attach via kernel-intrinsic MCP (LLP 0033/0034) - #135

Merged
philcunliffe merged 1 commit into
masterfrom
worktree-remote-query-plugin
Jun 23, 2026
Merged

Remote query attach via kernel-intrinsic MCP (LLP 0033/0034)#135
philcunliffe merged 1 commit into
masterfrom
worktree-remote-query-plugin

Conversation

@philcunliffe

Copy link
Copy Markdown
Contributor

Implements the client/kernel half of remote attach (LLP 0033/0034): a verb surface that projects one typed declaration into both a CLI command and an MCP tool, a stdio MCP host, and the --remote MCP client + credential store. The producer-side design folds the old @hypaware/remote REST plugin into core.

What's in here

Verb surface (new kernel contribution surface)

  • ctx.verbs.register({ name, tool, inputSchema, operation, render, exposure?, authClass? })src/core/registry/verbs.js, wired into KernelRuntime, the activation context, and the command run context.
  • argv↔schema codec (src/core/cli/verb_codec.js) — the single place CLI parsing for the query family lives; also emits the clean JSON Schema the MCP tool advertises, so the flag set and the tool schema can't drift.
  • verb→command projection (verb_command.js) — one declaration → CLI command + MCP tool.

MCP host — hyp mcp over stdio (src/core/mcp/{jsonrpc,server,stdio}.js)

  • Hand-rolled JSON-RPC 2.0 (no SDK dep — resolved per the LLP 0034 dependency note). initialize/tools/list/tools/call/resources/*.
  • Tool surface is emergent from active plugins' verbs; per-verb exposure + read/operator auth-class gating. Dataset schemas surface as MCP resources. stdout = protocol only.

Migrated to verbs:query_sql (core, intrinsic, read-class) and graph_neighbors (@hypaware/context-graph, read-class), reusing their existing pure cores + render machinery.

Consumer side (--remote)mcp/{client,remote_verb,proxy}.js

  • --remote routes a verb to the remote MCP tool with the same render; query.remotes/default_remote config (validated, local-only per LLP 0031); 0600 credential store with env→file resolution; hyp remote add/login/list/remove; hyp mcp --remote stdio proxy fallback.
  • --remote + --refresh is a hard error; two-line truncation surfacing (server cap + client display budget).

Tests

6 new test/core/* suites (codec, verb registry, MCP server, credentials, config validation, the --remote path with a stubbed server) + the mcp_stdio_tools hermetic smoke (boots hyp mcp against a seeded cache, asserts protocol-clean stdout + the mcp.serve_start lifecycle log). Full suite 1331 pass / 0 fail, typecheck clean, lint clean. @ref LLP 0033/0034 annotations attached throughout.

Scope notes

  • The remote path is E2E-blocked on the server (scoped credential + MCP route, out of tree). The client is written against the wire contract and unit-tested via an injectable fetch; the local stdio path is fully working now.
  • Deferred follow-ups (noted in LLP 0034): graph_project/github_backfill as operator-class verbs (machinery + gating in place, not yet migrated); hyp mcp attach AI-client sugar; local HTTP MCP (hyp mcp --http rejected today); OS keychain for the cred store.
  • The context_graph_projects_rows smoke fails on this branch and on the clean baseline — pre-existing, unrelated to this change.

🤖 Generated with Claude Code

Implements the client/kernel half of remote attach: a verb surface that
projects one typed declaration into both a CLI command and an MCP tool, a
stdio MCP host, and the --remote MCP client + credential store.
- Verb surface: ctx.verbs registry, argv<->schema codec, verb->command/tool
projection. query_sql (intrinsic) and graph_neighbors migrate to verbs.
- hyp mcp: hand-rolled JSON-RPC stdio host (no SDK dep); tools from verbs,
dataset schemas as resources; per-verb exposure + read/operator auth class.
- Consumer side: --remote routes a verb to the remote MCP tool with the
same render; query.remotes targets; 0600 credential store with env->file
resolution; hyp remote add/login/list/remove; hyp mcp --remote proxy.
--remote+--refresh is a hard error; two-line truncation surfacing.
Remote path is E2E-blocked on the server's MCP route + scoped credential
(out of tree); the client is unit-tested via an injectable fetch. Adds
traditional tests + an mcp_stdio_tools hermetic smoke; full suite green,
typecheck clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
ContributorAuthor

Dual-agent review — request_changes

  • Verdict:request_changes
  • Risk class:high
  • Auto-merge advisory: 👎 thumbs down — verdict is request_changes; needs human-gated follow-up

Advisory only: no merge was attempted. Reviewers: Codex errored (auth/model — gpt-5.5 unreachable via the local proxy; produced no output), so the verdict rests on the Claude review (5 parallel subagents).

Risk capstone

Cross-reference: reviewer findings vs high-risk surfaces

SourceFinding (severity, evidence)Intersects
Clauderemote_commands.js untested (major, remote_commands.js)Risk #3 (untested command surface) + Targets
Claudedefault_remote dead config (minor, verb_codec.js:114 / schema.js:541)Risk #5 + Config field chain
Claudeinline import() type (minor, verb_command.js:90; verbs.js:62,71)Targets (style, no risk surface)
Claude--port parsed but ignored (nit, core_commands.js)Targets (hyp mcp arg parsing)
Claude-o=<path> not recognized (nit, verb_codec.js:62)Targets (codec)

Recommended scope

  • Minimal: add test/core/remote-commands.test.js (resolves the major); decide default_remote (wire it or drop it + the type-doc claim); fix the two inline-import() style violations.

  • Expanded (optional): a mcp-client.test.js covering the SSE/array/401 branches and a runMcpProxy forwarding test with a fake fetchImpl; a serveStdio ordering test (interleaved slow/fast tool calls).

  • Out of scope: the server-side MCP route + scoped credential (separate repo, explicitly E2E-blocking the remote path); graph_project operator-verb migration and local HTTP MCP (declared follow-ups).

  • Codex review: (errored — no output)

Claude review

Claude review

Codex produced no output (auth/model error — gpt-5.5 unreachable via the local proxy); verdict rests on this Claude review. Five parallel review subagents ran: guidance-compliance, shallow-bug-scan, historical-context, contract-&-callers, comments-&-tests. Historical-context and contract-&-callers found nothing (the migration is behavior-preserving and every verbs touchpoint is wired). Findings below survived a ≥80 confidence filter.

remote_commands.js (add/login/list/remove) has no automated test

  • Severity: major
  • Confidence: 85
  • Evidence: src/core/cli/remote_commands.js:1 (whole module, ~300 lines, not imported by any test or smoke)
  • Why it matters: CLAUDE.md says to add traditional tests for deterministic logic incl. "config parsing and validation" and "similar local contracts". The create-or-augment local-config writer (mutateLocalConfig), the login TTY-without-token guard (the "never a hang" promise), token-from-stdin vs --token-file, and remote remove dropping default_remote when it names the removed target are all deterministic and untested; remote-credentials.test.js covers the store but not these command wrappers.
  • Suggested fix: add test/core/remote-commands.test.js driving the four runRemote* functions with a fake ctx/stdin over a tmp HYP_HOME (add→login-via-stdin→list→remove, the TTY guard, and the default_remote cleanup).

default_remote is dead config — bare --remote is unimplemented

  • Severity: minor
  • Confidence: 90
  • Evidence: src/core/cli/verb_codec.js:114 (--remote hard-errors when its value is missing); src/core/config/schema.js:541 (validates + stores default_remote); collectivus-plugin-kernel-types.d.ts (default_remote doc: "Default target used by --remote with no argument")
  • Why it matters: the config field is validated, stored, and cleaned up on remote remove, and the type doc + LLP 0033 promise that bare --remote resolves to default_remote, but no code path ever reads it — so default_remote has zero runtime effect and a bare --remote errors instead of using the default.
  • Suggested fix: either resolve default_remote when --remote is given no target, or (given the deliberate argv-ambiguity reason for deferring it) remove default_remote validation/storage and the type-doc claim so it isn't misleading dead config.

Inline import('...') type in verb_command.js instead of a bare @import name

  • Severity: minor
  • Confidence: 90
  • Evidence: src/core/cli/verb_command.js:90 (/** @type {import('../../../collectivus-plugin-kernel-types.d.ts').VerbRenderResult} */)
  • Why it matters: CLAUDE.md Code Style: "Never use inline import('...') types. Declare type imports at the top of the file with @import JSDoc comments, then reference the bare names." The file already has an @import block, so this is internally inconsistent.
  • Suggested fix: add VerbRenderResult to the top-of-file @import and use /** @type {VerbRenderResult} */.

Inline import('...') types in registry/verbs.js@returns

  • Severity: minor
  • Confidence: 90
  • Evidence: src/core/registry/verbs.js:62 and src/core/registry/verbs.js:71 (@returns {import('...').VerbExposure} / VerbAuthClass)
  • Why it matters: same CLAUDE.md rule; the file already imports other kernel types via @import at the top.
  • Suggested fix: add VerbExposure, VerbAuthClass to the @import and use bare names in both @returns.

hyp mcp --port is parsed but silently ignored

  • Severity: nit
  • Confidence: 80
  • Evidence: src/core/cli/core_commands.js (parseMcpArgv returns port; runMcp only branches on parsed.http and never reads parsed.port)
  • Why it matters:hyp mcp --port 8080 (without --http) is accepted and boots the stdio host as if no flag were passed — a silent no-op; the --http follow-up error only fires when --http is present.
  • Suggested fix: reject a stray --port (or roll it into the --http follow-up error) so it isn't silently dropped.

-o=<path> inline short form is not recognized as --output

  • Severity: nit
  • Confidence: 85
  • Evidence: src/core/cli/verb_codec.js:62 (isFlag = token.startsWith('--') || token === '-o'-o=file matches neither, so it falls through to the verb tail)
  • Why it matters:--output=file and -o file both work, but -o=file is silently treated as a positional — inconsistent and surprising.
  • Suggested fix: treat a -o= prefix as the output flag (e.g. token.startsWith('-o')).

Reports: /Users/phil/workspace/hypaware/.git/worktrees/remote-query-plugin/dual-review/pr-135

@philcunliffe

Copy link
Copy Markdown
ContributorAuthor

Codex review now available — verdict reaffirmed request_changes

Codex errored on the first pass (backend proxy down); it has now run cleanly. The verdict is unchanged — request_changes — and is now backed by both reviewers raising a major.

Updated tallies

  • Codex: 3 findings — 1 major, 2 minor (cats 1, 3, 4, 6–11 clean; suite not run by Codex)
  • Claude: 6 findings — 1 major, 3 minor, 2 nits
  • Risk class:high

Cross-reviewer agreement (the priority fix)

default_remote is dead config / bare --remote is unimplemented was flagged by both reviewers — Claude (minor) and Codex (major, Contract & Interface Fidelity, high confidence). The config field is validated, stored, and cleaned up on remote remove, and the type doc promises bare---remote resolves to it, but no code reads it. This is the highest-confidence item: either wire it or drop it + the doc claim.

New from Codex (not in the Claude pass)

  • Boolean tool-args silently coerce invalid values to false (minor) — validateToolArguments maps any non-boolean/non-"true" value to false instead of returning INVALID_PARAMS, so the MCP tool schema is laxer than it advertises (verb_codec.js). Worth a boolean-schema test.
  • --output file write sits outside the render try/catch (minor, medium-confidence) — a bad output path escapes runVerbCommand. In practice dispatch backstops it with an equivalent hyp <verb>: <message> exit-1, so the UX is preserved, but folding the write into the command-scoped handler is tidier.
Full Codex review

Fix Validations

No bug-fix validation claimed in the PR context.

Findings

2) Contract & Interface Fidelity

  • Severity: major
  • Confidence: high
  • Evidence: src/core/config/schema.js:542, src/core/cli/verb_codec.js:114, src/core/mcp/remote_verb.js:25
  • Why it matters:query.default_remote is accepted specifically so bare --remote resolves to a configured target, but the CLI parser rejects --remote without a value and the remote runner only accepts an explicit target.
  • Suggested fix: Let parseControlFlags represent bare --remote, resolve it to ctx.config.query.default_remote before runRemoteVerb, and add a regression test for --remote with only default_remote configured.

2) Contract & Interface Fidelity

  • Severity: minor
  • Confidence: high
  • Evidence: src/core/cli/verb_codec.js:199, src/core/cli/verb_codec.js:227, src/core/mcp/server.js:128
  • Why it matters: The MCP validator advertises the same schema contract as CLI parsing, but boolean arguments silently coerce any non-boolean/non-"true" value to false.
  • Suggested fix: For boolean tool args, accept only actual booleans or string true|false; otherwise return INVALID_PARAMS, and add a boolean-schema test.

5) Error Handling & Resilience

  • Severity: minor
  • Confidence: medium
  • Evidence: src/core/cli/verb_command.js:92, src/core/cli/verb_command.js:98
  • Why it matters: Render failures are caught, but the --output file write is outside that error boundary, so a bad output path can reject out of the verb command instead of returning a command-scoped error and exit code.
  • Suggested fix: Wrap the file write with the same command-level error handling and return exit 1 with hyp <verb>: <message>.

No Finding

  1. Behavioral Correctness
  2. Change Impact / Blast Radius
  3. Concurrency, Ordering & State Safety
  4. Security Surface
  5. Resource Lifecycle & Cleanup
  6. Release Safety
  7. Test Evidence Quality
  8. Architectural Consistency
  9. Debuggability & Operability

Evidence Bundle

  • Changed hot paths: verb CLI parser/projection, MCP tools/call argument validation, remote MCP verb execution, query remote config validation.
  • Impacted callers: src/core/cli/verb_command.js:44, src/core/cli/verb_command.js:71, src/core/mcp/remote_verb.js:25, src/core/mcp/server.js:128
  • Impacted tests: test/core/verb-codec.test.js:58, test/core/verb-codec.test.js:126
  • Unresolved uncertainty: I did not run the suite; review was constrained to the diff plus targeted line checks.

Verdict stands: request_changes. The shortest path to approve: add test/core/remote-commands.test.js (Claude major), resolve default_remote (both reviewers), and the smaller fixes (boolean validation, the two @import nits, --port/-o= parsing).

@philcunliffe
philcunliffe merged commit 5c47f81 into masterJun 23, 2026
6 checks passed
@philcunliffe
philcunliffe deleted the worktree-remote-query-plugin branch June 23, 2026 22:38
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@philcunliffe