Skip to content

Add --org to remote SQL, graph, and MCP reads - #1362

Open
philcunliffe wants to merge 5 commits into
masterfrom
codex/operator-cross-org-read
Open

Add --org to remote SQL, graph, and MCP reads#1362
philcunliffe wants to merge 5 commits into
masterfrom
codex/operator-cross-org-read

Conversation

@philcunliffe

@philcunliffe philcunliffe commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Remote SQL, graph neighbors, and the stdio MCP proxy now accept --org <label|*>. For example, hyp query sql --remote hyperparam --org '*' "select org, count(*) from ai_gateway_messages group by org" sends the selector on the derived MCP URL, including the initialize requests. The selector is a transport flag and is rejected without --remote; endpoint derivation preserves path prefixes and other query parameters.

The privacy document states that deployment operators can read forwarded data across orgs and that these reads are audited. The companion server enforces operator standing and implements the per-org union and audit fan-out.

Validation:

  • Latest GitHub CI: all checks passed, including the full test suite and typecheck on Node 22 and Node 24.
  • node --test test/core/remote-mcp-endpoint.test.js: all 9 tests passed, including SQL, graph, the proxy, URL preservation, and local/missing-value rejection.
  • npm run typecheck: passed after installing this branch's declared dependencies in its own worktree.
  • npm test: 6,060 passed, 3 failed, 3 skipped. The sandbox-launch and overlapping-spool-read failures passed on an isolated rerun. The gateway test a body refused with 421 is drained only up to a cap still failed and reproduced identically on a pristine copy of the starting commit, d42c794. No unrelated test or gateway code was changed.
  • Ref-check and git diff --check: passed.

CPU and memory review: one optional string parsed per invocation and one URL search parameter update per remote connection. No new per-row work, retained state, runtime dependencies, or CPU/memory concern.

The managed deployment acceptance gate is documented in the companion server PR; it needs an operator login on that deployment.

Companion server PR: https://github.com/hyparam/hypaware-server/pull/437

@philcunliffe philcunliffe added neutral:adopt Foreign PR adopted into neutral's reconcile scope neutral:adopted Adoption completion record: merged while carrying neutral:adopt (LLP 0031) labels Sep 4, 2026
Review fixes on the operator cross-org read path.

A 403 answering an explicit --org is the server refusing the *selector*, not
the credential, so the shared 401/403 refresh policy was wrong for it: the
attach path forced a token refresh, re-sent the read the server had already
denied (a second entry in that org's audit trail), and then advised re-running
'hyp remote login', which cannot grant operator standing. Both the verb attach
path and the stdio proxy now stop at the first 403 and say what was refused.

Also: adding the selector no longer rewrites the target's own query string
(URL.searchParams.set re-serializes the whole query as form encoding); the
stdio proxy resolves built-in targets the same way the verb path does, so
'hyp mcp serve --remote hyperparam --org *' reaches the shipped server instead
of reporting an unknown target; an empty --org value is named as such rather
than blamed on a missing --remote; and 'hyp query --help' plus CLI_REFERENCE
document the flag the usage strings already advertise.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
Contributor Author

Review: --org on remote SQL, graph, and MCP

Reviewed head 2b75e8f0 in an isolated detached worktree. Verdict: approve with fixes applied. The design is right (a transport flag on the derived MCP URL, rejected without --remote, never a verb parameter), and I cleared the things most likely to be wrong: --org cannot shadow a verb parameter (no registered verb declares one), the --org requires --remote gate runs after the flag loop so order does not matter, bare --remote (the '' sentinel) correctly satisfies it, last-occurrence-wins agrees between the verb codec and parseCommandArgv (unlike the historic report --org a --org b bug at src/core/cli/report_commands.js:459), --org=* and --org '*' both survive unencoded, and the selector does not leak into deriveIdentityBase or the token-refresh URL.

Six findings. Five are fixed and pushed as c85bc649; one is reported only.

1. Medium: a 403 on an --org read is treated as a refreshable credential failure

src/core/mcp/remote_verb.js:78 and src/core/mcp/proxy.js:110. isAuthStatus is 401 or 403 (src/core/mcp/client.js:129). With --org, a 403 is the expected server answer to a caller without operator standing, or one naming an org it may not read - which is this feature's most likely failure. Today that path forces a token refresh, re-sends the read the server already denied (a second entry in that org's audit trail), and then reports sessionExpiredMessage via describeAuthRejection (remote_verb.js:98): "your session has expired, re-run hyp remote login", exit 2. Re-login cannot grant operator standing. The reports plane already draws exactly this distinction deliberately (report_commands.js:511-513: "a 403 is org_mismatch ... which no refresh can fix").

Fixed. With an explicit --org, a 403 is terminal on both paths, and both report it through one new shared message (orgReadRefusedMessage in client.js, beside isAuthStatus) so the two attach paths cannot drift. Without --org, the LLP 0058 D5 401/403 policy is untouched.

2. Low: adding the selector rewrote the target's other query parameters

src/core/remote/credentials.js:117. parsed.searchParams.set('org', org) re-serializes the whole query string as form encoding, but only when --org is passed. Verified before the fix: https://h/base?a=x%20y&b=~z became .../v1/mcp?a=x+y&b=%7Ez&org=*, and a valueless ?flag became flag=. The PR body claims derivation "preserves ... other query parameters"; that held only for the simple existing=1 case the test covered.

Fixed. The raw query string is rewritten instead, so registered parameters reach the server byte-identical. Existing org= replacement semantics are unchanged.

3. Low: hyp mcp serve --remote hyperparam cannot reach the shipped server

src/core/mcp/proxy.js:34 resolved the target from ctx.config?.query?.remotes ?? {} instead of effectiveRemotes(ctx.config) (what the verb path uses), so the built-in hyperparam target was unreachable. Verified before the fix: exit 2, unknown remote target 'hyperparam' - add it with 'hyp remote add hyperparam <url>'. Pre-existing, but it blocks the usage line this PR itself adds on the deployment the PR's privacy note is about.

Fixed (one line, plus a regression test).

4. Low: the empty---org message was unreachable without --remote

src/core/commands/mcp.js:111. The two guards were ordered so hyp mcp serve --org= reported --org requires --remote rather than naming the empty value. Fixed by checking the value first; the !p.remote truthiness guard is kept deliberately so --remote= --org acme still errors instead of silently dropping the selector into the local server.

5. Low: hyp query --help never mentioned the new flag

src/core/cli/core_commands.js:182 enumerates the kernel control flags and was not updated, while usageForVerb and the hyp mcp serve usage line were. docs/CLI_REFERENCE.md (lines 106, 132, 151, 208, 1401) had the same drift. Fixed in both.

6. Low, reported only: agent skill docs

hypaware-core/plugins-workspace/{claude,codex}/skills/hypaware-query/SKILL.md has a "Remote queries" section that documents --remote in detail and does not mention --org. Left alone: those are agent-facing skill docs for an operator-only flag, and updating them is a judgment call about whether agents should be told about cross-org reads at all. Worth a decision, not a silent edit.

Verification

  • npm test: 6071 pass, 0 fail, 1 skipped (on c85bc649).
  • npm run typecheck: clean.
  • Five new tests in test/core/remote-mcp-endpoint.test.js; each was confirmed red against the pre-fix src/ (git stash push -- src/) and green after, so none is vacuous. The 403 tests use a real refreshable oidc session and assert exactly one POST, proving no refresh and no re-send.

philcunliffe and others added 2 commits September 5, 2026 00:49
Round 2 review fixes on the operator cross-org read path.

remote_verb.js reads `.status` off the caught rejection to decide whether an
explicit --org met a 403. It read it unguarded, ahead of isAuthError's own
`!!err && typeof err === 'object'` test, so a rejection that is not an object
(the MCP client awaits fetch directly and rethrows whatever it rejected with)
threw a TypeError out of the catch block. The real failure was then replaced
by `hyp query sql: Cannot read properties of undefined (reading 'status')`.
Optional-chain the read, matching the guard the helper below it already keeps.

Also: the stdio proxy's comment above the failure-detail branches said a
401/403 there is a dead credential, which the org-403 branch added beside it
is not. Say what each branch covers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A 403 under an explicit --org is still terminal (a retry only earns a second
denial in that org's audit trail), but the message asserted a cause the client
cannot see. A deployment that answers a revoked credential with 403 rather than
401, or a proxy in front of it, produces the same status, and the operator was
told "this account may not read that org; re-login cannot grant operator
standing" - advised away from the one move that fixes it.

Name both causes and the remedy for the second, and say "refused" of the
request rather than of a read, since the same 403 can answer the MCP handshake.
Behavior is unchanged: still no refresh and still no re-send.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
Contributor Author

Review round 2: --org on remote SQL, graph, and MCP

Reviewed head c85bc649 (the head round 1 produced) in an isolated detached worktree, plus a second independent pass by a separate reviewer against the same SHA. Verdict: approve with fixes applied. No blocking correctness bug. Two findings fixed and pushed as 5c06f48d and d8c3f9ae; three recorded open for triage, one of them carried forward from round 1.

Round 1's five fixes all hold up. I re-derived each rather than trusting the record: the raw-query rewrite is byte-faithful and injection-safe (probed ?a=x%20y&b=~z, valueless ?flag, ?a=1&&b=2, ?x=1#frag, ?organization=1 correctly not stripped by the org= prefix filter, duplicate org=a&org=b collapsed to one, and org values containing space / & / = correctly percent-encoded); effectiveRemotes in the proxy still fails closed with no credential (hyp mcp serve --remote hyperparam on an empty HYP_HOME exits on no token for 'hyperparam', it does not silently connect); hyp mcp serve --org= names the empty value; and every remote-capable usage line in docs/CLI_REFERENCE.md carries the suffix while hyp report's four correctly do not.

1. Low, fixed: the org-403 check threw on a non-object rejection

src/core/mcp/remote_verb.js:78. Round 1's fix put Number((err).status) at the top of the catch, ahead of isAuthError, which deliberately guards !!err && typeof err === 'object' (remote_verb.js:158). client.js awaits doFetch directly and rethrows whatever it rejected with, so a non-object rejection made the catch block itself throw. Verified against the pre-fix source: the user got

hyp query sql: Cannot read properties of undefined (reading 'status')

instead of the real failure - the TypeError escapes op, escapes attachWithRefresh, and lands in mapRefreshError, so a plain transport error is reported as a token-refresh failure.

Fixed in 5c06f48d: (err)?.status, matching the guard the helper below it already keeps. New regression test confirmed red against the pre-fix src/ (git stash push -- src/) with exactly the message above, green after.

2. Low-medium, fixed: the refusal message asserted a cause the client cannot see

src/core/mcp/client.js:144. The message read this account may not read that org; re-login cannot grant operator standing. That is the likely cause, but the client cannot tell which layer answered 403: a deployment that returns 403 (not 401) for a revoked credential, or a proxy in front of it, produces the identical status. On that path the operator was advised away from the one move that fixes it, and - because round 1 made the 403 terminal - the refresh-and-retry backstop no longer covers them either. The same string also fired on a 403 answering the MCP handshake (rpc('initialize') throws authRejectionError(403) too), calling a handshake refusal a refused "read".

Fixed in d8c3f9ae. Behavior is unchanged - still no refresh, still no re-send, so no second entry in that org's audit trail - but the message now names both causes and the remedy for the second, and says "refused" of the request rather than of a read:

'prod' refused --org 'acme.test' (HTTP 403), not retried - either this account may not read that org (re-login cannot grant operator standing), or its credential was revoked, in which case re-run 'hyp remote login prod'

The two 403 tests were tightened to assert the revocation remedy is present while "your session has expired" is still not the headline; both confirmed red against the pre-fix src/.

On the related question of whether this narrowing makes client.js's @ref LLP 0058#d5 stale: it does not, and I checked rather than assumed. LLP 0058 D5 says "If a live request still returns 401, the attach path refreshes once and retries" - 403 was only ever added by isAuthStatus. The annotation sits on authRejectionError, which still tags 401 and 403 identically; only the consumer declines to spend a refresh. If anything the code now sits closer to what D5 wrote. No LLP edit needed for this.

3. Low, open: --org= means opposite things on two planes

src/core/cli/verb_codec.js:114 and src/core/commands/mcp.js:113 reject an empty --org=, while the pre-existing same-named flag on the reports plane deliberately treats empty as meaningful - applyOrgParam at src/core/cli/report_commands.js:467 carries the comment "--org='' is the admin single-org form, so presence matters, not truthiness" and sends ?org=. So hyp report list --org= works and hyp query sql --remote prod --org= is a usage error, for one flag name against one server. Left alone: which is right is a server-contract question for the companion PR, not something a review rung should guess. Note the metavars already differ deliberately (--org <org> on reports, --org <label|*> here), so this may be intended.

4. Low, open: an adjacent fix landed without a PR-body line

src/core/mcp/proxy.js:34. Swapping ctx.config?.query?.remotes for effectiveRemotes(ctx.config) is correct and is what makes this PR's own hyp mcp serve --remote <target> --org usage line reachable on the shipped deployment, but it is a user-visible change outside the stated scope, and CLAUDE.md asks for adjacent fixes to be noted in the PR description. It is recorded in round 1's review comment (finding 3) but not in the body. I am not permitted to edit the PR body, so flagging it here for triage.

5. Low, open (carried from round 1): agent skill docs

hypaware-core/plugins-workspace/{claude,codex}/skills/hypaware-query/SKILL.md still document --remote in detail with no mention of --org. Non-blocking in my judgment, and still a judgment call rather than a silent doc edit. One extra datum for whoever decides: if --org is added there, the section preamble at claude/.../SKILL.md:61 needs a matching look - it currently tells the agent the remote credential is "query-scoped ... distinct from the server's operator/admin token, which never leaves the server", which is the framing an operator cross-org read sits awkwardly beside.

6. Low, open: no LLP, and a merge-order risk

The second reviewer flagged the absence of an LLP for a change that adds cross-org operator reads and amends docs/PRIVACY.md. I did not create one: numbers must be minted with node scripts/llp-numbers.js next after a git fetch --prune, and whether this warrants a Decision is design authority, not a review rung's call. Recording it for triage.

Related and worth a decision in the same breath: the companion server PR (hyparam/hypaware-server#437) is still open, not merged. If this client lands first, --org acme against a deployment that ignores an unknown query parameter returns the caller's own org's rows with nothing on stderr to say so - a silently wrong answer, which is the failure mode docs/CLI_REFERENCE.md's new paragraph invites operators to trust. Worth gating the merge order or confirming the server rejects an unrecognized org.

Verification

  • npm test at d8c3f9ae: 6072 pass, 0 fail, 1 skipped. npm run typecheck: clean. Also captured green at the reviewed head c85bc649 before any edit (6071 pass, 0 fail) so the delta is attributable.
  • Both fixes positively verified in the pushed tree, not just locally: git diff c85bc649..d8c3f9ae changes src/core/mcp/{client,proxy,remote_verb}.js and the test file, with remote_verb.js:81 now reading (err)?.status and client.js:150 carrying the new message.
  • Every new or changed assertion confirmed red against the pre-fix src/ and green after, so none is vacuous.
  • git diff --check clean; no em dashes, no NUL bytes.

Open findings summary

# Severity Status
1 Low Fixed in 5c06f48d
2 Low-medium Fixed in d8c3f9ae
3 Low Open - server-contract question
4 Low Open - needs a PR-body line I cannot add
5 Low Open - carried from round 1, non-blocking
6 Low Open - LLP judgment + merge-order risk

@philcunliffe philcunliffe added the neutral:stuck neutral attempted this but cannot complete it autonomously — needs a human label Sep 5, 2026
@philcunliffe

Copy link
Copy Markdown
Contributor Author

Triage: stuck on a merge-order blocker

Neutral's triage rung examined PR #1362 at head d8c3f9ae after the review-round cap (2 rounds) was exhausted with four findings still open. Three of the four are non-blocking; one is a true blocker that no client-side edit in this PR currently mitigates, so the PR is marked neutral:stuck rather than triaged for merge.

The blocker: merging before hyparam/hypaware-server#437 produces silently wrong answers

Round 2 flagged this; triage verified it independently on both sides rather than trusting the prose.

What was verified:

  • hyparam/hypaware-server#437 is OPEN, not merged (state: OPEN, mergedAt: null, checked 2026-09-05). That PR is what makes the server honor the selector ("enforces operator standing and implements the per-org union and audit fan-out", per this PR's own body).
  • The shipped server ignores the parameter entirely. On hypaware-server master, src/http/routes-mcp.js contains zero reads of the request URL's query string (no searchParams anywhere in the file). Org scope comes only from the verified credential: the read-limiter key and the mcpForOrg(auth.org, ...) assembly key are commented "auth.org from the verified credential, never anything client-asserted". An unrecognized ?org=acme is dropped without error, and the request succeeds against the caller's own credential scope.
  • The client has no guard. This PR appends org=<value> to the derived MCP URL (src/core/remote/credentials.js:123-125) and uses it verbatim (src/core/mcp/remote_verb.js:53, src/core/mcp/proxy.js:49). Nothing in the client checks that the server acknowledged the selector: no echo of the resolved org is requested or compared, and a 200 carrying the caller's own org's rows is accepted and rendered normally. The only org-aware client behavior is the 403 message (src/core/mcp/client.js:149-150), which never fires on this path because the old server answers 200.

Consequence if this PR merges first: hyp query sql --remote <target> --org acme "..." against any deployment still running today's server returns the caller's own scope's rows, exit 0, nothing on stderr. That is a silently wrong answer on exactly the operator cross-org read path this PR's new docs/CLI_REFERENCE.md:110-112 paragraph invites operators to trust ("the server records each such read in that org's audit trail" - on the old server no such audit entry exists either). Wrong data presented as the requested org's data is a production defect, not a preference, so it blocks.

Non-blocking findings (recorded so the whole PR is visible)

Using round 2's numbering:

  • (3) --org= semantics differ across planes. hyp query ... --org= is a usage error (src/core/cli/verb_codec.js:114, src/core/commands/mcp.js:113) while hyp report list --org= deliberately sends ?org= as the admin single-org form (src/core/cli/report_commands.js:467). It fails loudly, never silently, and the metavars already differ (<org> vs <label|*>), so this is a server-contract question, not a defect. Non-blocking.
  • (4) The adjacent effectiveRemotes fix (src/core/mcp/proxy.js:34) has no PR-body line. The fix itself is correct and user-visible; CLAUDE.md wants adjacent fixes noted in the description. A one-line body addition at merge time closes it. Non-blocking.
  • (5) Agent skill docs (hypaware-core/plugins-workspace/{claude,codex}/skills/hypaware-query/SKILL.md) document --remote without --org. Whether agents should be told about an operator-only cross-org flag at all is a product judgment; if it is added, the claude SKILL.md:61 preamble ("the operator/admin token ... never leaves the server") needs a matching look. Non-blocking docs gap.
  • (6a) No LLP for a change that adds cross-org operator reads and amends docs/PRIVACY.md. CI's LLP-required job passes on this diff, so this is a design-documentation judgment for design authority, not a defect. Non-blocking.

Questions for a human, with options

On the blocker, pick one:

  1. Gate merge order: hold this PR until hyparam/hypaware-server#437 merges and is deployed to the targets that matter (simplest; zero client code change).
  2. Confirm server rejection: establish that the server (as deployed) rejects an unrecognized org query parameter with an error the client surfaces. Today's master demonstrably does not, so this option effectively means Attached Claude sessions report inflated context percent: proxy base URL drops the assumed window from 1M to 200k #437 or a follow-up must make an unknown/unsupported org a hard error, and old deployments remain a risk until upgraded.
  3. Add a client-side guard: have the server echo the resolved org selector (for example in the initialize result or tool result metadata) and make the client refuse, or at minimum warn loudly, when it sent --org and got no confirmation back. Fails safe against every old or third-party deployment, at the cost of a small protocol addition coordinated with Attached Claude sessions report inflated context percent: proxy base URL drops the assumed window from 1M to 200k #437.

Secondary (answer if convenient): for (3), is the empty --org= rejection on the query plane the intended contract, or should it match the reports plane's admin single-org form?

How to unstick

reply with a comment on this PR (or push to the branch); neutral monitors this thread and will re-engage with your guidance on its next tick.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

neutral:adopt Foreign PR adopted into neutral's reconcile scope neutral:adopted Adoption completion record: merged while carrying neutral:adopt (LLP 0031) neutral:stuck neutral attempted this but cannot complete it autonomously — needs a human

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant