Files
pad/internal/mcp/dispatch_http.go
xarmian ee0d945863 fix(cli,mcp): one --field key=value entry means one thing at every door (BUG-2870) (#1283)
* feat(items): one shared parse for a --field key=value entry (BUG-2870)

Six sites parsed that entry independently — item create, list, update, move
and copy in cmd/pad, plus ingestFieldKVP on the remote /mcp door — in four
spellings, and they disagreed about what it meant. The CLI sites used both
halves verbatim, so `--field " effort=l"` stored an undeclared field named
" effort" and left the declared `effort` untouched; the remote door trimmed
both halves and wrote `effort`. Same call, two stored keys, decided by which
transport the caller was on.

This is the helper only; the call sites move over in the commits that follow.

Two rules, deliberately asymmetric, per the day-60 ruling:

- a KEY whose trimmed form differs from what was written is REFUSED at every
  door, rather than silently retargeted to a different field;
- a VALUE is carried VERBATIM at every door, because trimming reinterprets a
  caller's bytes and on a text field the space is content. A padded value
  against a typed field is refused one layer down by validation, naming the
  field — measured, not assumed.

ErrFieldEntryMalformed is returned rather than handled because the six sites
deliberately disagree about a malformed entry (four skip it, copy hard-errors)
and unifying that is a separate decision.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR

* fix(cli,mcp): all six --field parse sites go through the one helper (BUG-2870)

item create, list, update, move and copy in cmd/pad, plus ingestFieldKVP on
the remote /mcp door, now call items.SplitFieldEntry instead of each rolling
its own split. A padded key is refused at every door; a value reaches every
door verbatim.

Two sites keep something specific to them, both documented in place:

- `item list` is a READ filter, and it takes the same key rule deliberately:
  a padded key there filters on a field nobody declared and returns empty,
  which is indistinguishable from "no rows match".
- `item move` gets KEY normalisation only. Its values stay strings because
  the server types a declared field on that path too, so a clean
  `--field n=3` already stores the number 3 — measured before the change.

Each site keeps its historical disposition toward a MALFORMED entry (four
skip silently, copy hard-errors), which is why the helper classifies that
case rather than deciding it.

NOT YET EVIDENCE: ./internal/mcp, ./cmd/pad and ./internal/items all pass,
and that green does not show the divergence closed — the three BUG-2850
pinned tests exercise the catalog conflict pass, which never reaches
ingestFieldKVP. The door-level test and the re-grounding of that pass are
the next commits.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR

* test(cli,mcp): pin the door-parity claim at both doors (BUG-2870)

Nothing in the suite asserted what the remote door STORED for a padded entry
— the three BUG-2850 tests that cite its trimming all exercise the catalog
conflict pass, which never reaches ingestFieldKVP. So the previous commit's
green was not evidence for the thing it changed.

Three files now hold the claim: internal/items pins the rule, internal/mcp
pins the remote door, cmd/pad pins the CLI door, and each cites the other
two. Padded key refused at both; padded value carried verbatim at both; a
refusal aborts the call rather than dropping one entry, and on the CLI it
happens before any request reaches the server.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR

* fix(mcp): re-ground the conflict pass on the new door behaviour (BUG-2870)

The pass's rules were derived from ingestFieldKVP trimming, so changing the
door without changing the layer built on it would have been the same
one-door lapse a level up.

- parseFieldArray splits through items.SplitFieldEntry: a padded key is
  REFUSED before dispatch on both transports, and values are indexed RAW,
  because raw is now what both doors write.
- Both comparison sites compare raw for the same reason. The round-19
  "COMPARED TRIMMED" rule is superseded and its comment says so.
- detectFieldConflicts PROPAGATES the parse refusal instead of returning nil.
  It swallowed it as "the caller owns this error surface", which was true
  when the only possible error was a shape error — reshapeItemFields returns
  early with no `fields` object, so on the no-`fields` path (this bug's path)
  nobody owned it and a padded entry turned back into a success.
- A padded entry is refused in the pass rather than skipped. Skipping dropped
  it from conflict detection entirely, turning four existing refusals into
  successes.

The last two were caught by the BUG-2850 tests, not by reasoning: the first
shape of this commit passed a full package build and turned four guards off.

Seven tests still fail. They assert the OLD door behaviour and are the
specification being changed; each gets read on its own next, and is either
kept because the behaviour survives or replaced by a test stating the new
behaviour that cites the old name.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR

* test(mcp): restate the seven BUG-2850 pins on the new rule (BUG-2870)

Each was read on its own and either kept or replaced; every replacement
names the test it replaces and why the old assertion was right at the time,
so the deletion is traceable rather than a green that appeared.

- padded value is not a conflict → IS a disagreement now that no door trims
  (" done" and "done" are two values), with an equal-values control leg.
- padded entries still caught (hierarchy) → refused EARLIER, by the padded-key
  rule, before the alias pass observes both keys. The alias guard keeps its
  three unpadded cases, which is what stops this being a hole.
- PaddedEqualDuplicateIsCanonicalized → IsRefused, plus a canonical control
  that still emits --field exactly once.
- MixedCanonicalAndPaddedDuplicatesCollapse → Refused. The round-8 finding
  survives: one canonical entry still does not make its padded sibling
  harmless, it is refused rather than swallowed.
- PaddedEntryAloneIsUntouched → IsRefused. That test pinned a DEFERRAL, in
  its own words "BUG-2870's business, not this PR's". This is that business.
- "fields carries the key — canonicalized, so accepted" → still refused,
  since nothing canonicalizes now; the per-key question it defended is still
  tested by the two legs beside it, and a canonical control was added.
- ReEmittedValueKeepsItsWhitespace → the re-emission path is gone, so it
  becomes a refusal test that also asserts the ADVISED form is accepted with
  its value untouched. The property it defended is pinned at both doors.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR

* test(server): pin that a move override is typed server-side (BUG-2870)

The fact the ruling turned on, and the easiest one in this unit to lose: it
is invisible from cmd/pad, where moveCmd plainly sends a string.

- a declared number field given the STRING "3" through field_overrides ends
  up as the NUMBER 3, which is why move needs the shared KEY parse and no
  client-side typing;
- a padded " 3" is REFUSED with a 400 and the item does not move, which is
  the answer the remote door will now give too instead of trimming and
  succeeding.

t.Parallel per CONVE-2086 — both build their own server through testServer,
so each has its own database, limiter and bus.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR

* chore(mcp): bump tool surface to 0.30 and sync the docs the guards enforce (BUG-2870)

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR

* refactor(mcp): remove the canonicalization the door change made unreachable (BUG-2870)

Two mechanisms existed to make a padded entry reach both doors as the same
write: the nonCanonical conflict guard (round 16) and the re-emission path
that rewrote a padded entry to canonical form (rounds 7/8). Both are dead
now — items.SplitFieldEntry refuses a padded key, so every entry that parses
satisfies `entry == key + "=" + value` BY CONSTRUCTION.

Removing each changed no test. That is consistent with "dead" and with
"untested" alike, so the construction argument above is what settles it —
recorded in the comments that replace them, along with what the removed
guard was defending and where that premise is enforced now.

Rewriting a caller's key was also the behaviour this bug is about, applied
by us rather than by a door: canonicalization silently changed the key the
caller wrote. Refusing says so instead.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR

* docs(mcp): put the trimming narrations in the past tense (BUG-2870, CONVE-23)

Six comments described the old door behaviour in the present tense ("HTTP
trims and writes effort"), which reads as a claim about the code as it
stands. The rounds they narrate still explain why the surrounding rules
exist, so they are re-tensed rather than deleted.

Two references were checked and left alone because they are still true:
ingestFieldKVP does still store every field value as a STRING (coerce.go's
BUG-2850 note, and the github_pr hint in dispatch_http.go). This change
stopped it TRIMMING, not stringifying.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR

* docs: sync CLAUDE.md to tool surface 0.30 (BUG-2870)

The drift guards cover instructions.md and README.md but not this file, and
its own 0.27 entry records the consequence: 'This entry was missing from
CLAUDE.md — the 0.27 unit swept instructions.md and README.md and not this
file.' The unit that makes a version line stale is the unit that owes it.

Both markers updated, and the entry states the two behaviour changes in the
terms they were ruled: /mcp refuses what it silently accepted, and the
swallowed parseFieldArray refusal that was landing four refusals as
successes on the no-fields path.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR

* fix(mcp): finish the removal, and correct a claim I made twice (BUG-2870)

Codex round 1: no P1/P2, two nits, both real.

1. The re-emission removal was incomplete. `reEmitFields` and the branch
   that appended its entries survived with nothing populating the map, and
   two comments still described canonical re-emission as something this code
   does. Unreachable, but my own commit message had said the path was
   removed, so the code contradicted the claim. Removed, and the round-16/17
   paragraphs that decided WHEN to canonicalize go with it — they answered a
   question that no longer arises.

2. "The only behaviour change is /mcp refusing what it silently accepted" is
   WRONG, and it was in version.go, README.md and CLAUDE.md. Every door
   refuses a padded key now; they were merely accepting it differently —
   /mcp trimmed it and wrote the declared field, the CLI stored a ghost field
   beside it. What is /mcp-only is the VALUE half. Corrected in all three,
   with the correction itself recorded in the version.go entry so the next
   reader sees the claim was checked rather than a sentence that quietly
   changed shape.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR

* docs(mcp): rename the predicate to the question it asks (BUG-2870)

Codex round 2: no P1/P2, three nits, all naming and prose.

- `canonicalized` is renamed `coveredByFieldsObject`. Nothing canonicalizes
  anything any more, and the only thing that predicate ever asked was
  whether the `fields` object carries THIS key — it kept the old name only
  because the guard it used to feed had been removed a commit earlier.
- parseFieldKVP's doc said invalid entries are skipped silently. True of a
  MALFORMED entry, false of a padded key, which now aborts the call.
- Three test comments still described re-emission as live, and version.go
  described this door's trimming in the present tense.

Nothing in these two rounds was a defect in the change itself; both rounds
found prose describing a version of the code that stopped existing partway
through the unit, which is the failure mode a re-grounding pass invites.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR

* docs(mcp): last of the prose that outlived the code (BUG-2870)

Codex round 3: no P1/P2, prose only.

- the predicate's own comment still asked 'will anything canonicalize THIS
  key'; it asks whether the fields object carries the key, and always did;
- two test comments described re-emission and trimmed comparison as current.
  Both tests are kept — what they pin is narrower now and still worth
  pinning — with the change in what they mean written down.

Deliberately NOT changed: the comments and replacement-test names that cite
the OLD test names. Codex reads them as stale terminology; they are the
traceability the restatement commit was asked for, so a reader can find what
each replacement replaced.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR

* docs(items): the coercion note names what the door does now (BUG-2870)

Codex round 4. The paragraph described ingestFieldKVP as doing
`dst[key] = val` unconditionally. Its CLAIM — every value arrives at the
server as a string — is still true and is the reason this file exists; the
description of the line is not, since that door now parses through
items.SplitFieldEntry. Restated so the still-true part is not carried by a
sentence a reader can falsify.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR
2026-09-07 15:33:05 -04:00

1088 lines
48 KiB
Go

package mcp
import (
"bytes"
"context"
"encoding/json"
"errors"
"fmt"
"io"
"net/http"
"net/http/httptest"
"net/url"
"strings"
"sync"
"github.com/go-chi/chi/v5"
"github.com/mark3labs/mcp-go/mcp"
"github.com/PerpetualSoftware/pad/internal/items"
"github.com/PerpetualSoftware/pad/internal/models"
"github.com/PerpetualSoftware/pad/internal/server"
)
// HTTPHandlerDispatcher executes a pad CLI command by translating it
// into an in-process HTTP request against pad-cloud's existing handler
// chain — no subprocess, no shell-out, no PAT-credential inheritance.
//
// Why not shell out (TASK-965 / PLAN-943 architecture decision):
//
// The subprocess-based ExecDispatcher works for local stdio MCP
// because the user IS the subprocess owner — `pad mcp serve` inherits
// the credentials in `~/.pad/credentials.json`. For remote MCP at
// `mcp.getpad.dev/mcp`, multiple OAuth users share one process; the
// dispatcher can't shell out to `pad item create` because the
// subprocess would have no credentials for the requesting user, and
// minting an ephemeral PAT per call adds DB churn we'd rather avoid.
//
// HTTPHandlerDispatcher instead:
//
// 1. Resolves the requesting user from the MCP request context (via
// UserResolver, supplied by the OAuth-auth middleware that
// handles /mcp).
// 2. Looks up cmdPath in routeTable to find an HTTP method, URL
// template, and JSON body shape.
// 3. Builds an in-process http.Request with the user attached via
// server.WithCurrentUser so the existing handler chain sees it
// the same way it would for a normal Bearer-token request.
// 4. Calls Handler.ServeHTTP with an httptest.ResponseRecorder and
// packages the response as an MCP CallToolResult — JSON bodies
// surface as structured content, matching ExecDispatcher's
// `--format json` parsing behaviour.
//
// Behavioural divergence from ExecDispatcher is zero: the same
// handler chain runs (auth, audit, event-bus, webhooks, FTS index),
// just without forking a subprocess.
//
// Scope:
//
// - TASK-965 shipped the framework + `item create` as the
// proof-of-concept.
// - TASK-966 (this expansion) wires the high-value reads + writes:
// item show / list / delete / move / search / comment / comments,
// project dashboard, collection list, role list. Commands with
// non-trivial shape (item.list's path-varies-on-arg, item.move's
// nested overrides) live as standalone RouteMapper functions in
// dispatch_http_routes.go; the rest use the declarative routeSpec.
//
// Tools the cmdhelp registry advertises but the route table doesn't
// yet wire produce a clear "not yet implemented over HTTP transport"
// error rather than failing silently — see Dispatch below.
type HTTPHandlerDispatcher struct {
// Handler is the pad-cloud API router. *server.Server already
// satisfies http.Handler via its ServeHTTP method.
Handler http.Handler
// UserResolver returns the requesting user from the MCP request
// context. Required. Returning nil → 401-equivalent error to the
// MCP client.
//
// In production the OAuth auth middleware (TASK-953) sets the
// user on context before invoking the dispatcher; UserResolver is
// just `(ctx) → user from ctx`. In tests it's a constant returning
// a pre-built test user.
UserResolver func(ctx context.Context) *models.User
// Apply, if non-nil, is invoked on the synthesized request just
// before ServeHTTP. Useful for tests + future TASK-953 work to
// attach token-scope context (workspace allow-list, capability
// tier) without changing the dispatcher's public surface.
Apply func(r *http.Request) *http.Request
// Routes overrides the built-in routeTable for tests. nil → use
// routeTable. The builtin map is the source of truth for
// production wiring.
Routes map[string]RouteMapper
// OnScopeDenied, when non-nil, fires every time buildAuthedRequest
// rejects a synthesized request because the API token's scope does
// not permit the requested method on the resource (TASK-1119
// follow-up to TASK-961). The callback gets the same (method,
// urlPath) the dispatcher was about to send so the observer can
// add useful labels if needed; today it backs the
// pad_mcp_authz_denials_total{reason="tier_mismatch"} counter.
//
// Why a callback rather than importing internal/metrics directly:
// internal/mcp depends on internal/server, but adding an
// internal/metrics import here would create a circular concern —
// server already owns metrics wiring, and the dispatcher's role is
// "translate tool args → in-process HTTP request." Keeping it
// metrics-naive (matches the OAuth Storage SetRevocationObserver
// pattern from TASK-961) lets cmd/pad attach the observer at
// startup without coupling.
//
// nil → no observer; the existing permission_denied error envelope
// still flows up to the caller unchanged. Tests that don't care
// about metrics can leave this unset.
OnScopeDenied func(method, urlPath string)
// Lister, when non-nil, populates available_workspaces in the
// no_workspace / unknown_workspace error envelopes (TASK-977).
// Production wires an OAuth-aware lister that reads the token's
// workspace allow-list (sub-PR E TokenAllowedWorkspacesFromContext)
// and filters the user's full workspace set so the agent never
// sees workspaces it didn't consent to.
//
// nil → empty available_workspaces (legacy behaviour). Callers
// SHOULD wire one for the remote /mcp transport, where leaking
// non-consented workspace slugs would be a privacy violation.
// The local stdio transport (ExecDispatcher) doesn't go through
// this dispatcher, so its lister wiring is independent.
Lister WorkspaceLister
// RequireVerifiedEmail, when non-nil, gates the remote MCP write
// path for PLAN-1933 DR-4. It's invoked in buildAuthedRequest —
// the single chokepoint every synthesized request passes through,
// same place the token-scope check lives — for MUTATING methods
// only (reads stay open). Returning true BLOCKS the request: the
// dispatcher fails the build with an email_not_verified error
// instead of hitting the handler, so no content mutation reaches
// the store.
//
// The /mcp mount lives outside the /api/v1 middleware stack, so the
// RequireVerifiedEmail HTTP middleware can't cover it directly; this
// hook is the perimeter's own gate. cmd/pad/main.go wires it to
// `srv.IsCloud() && user != nil && !user.IsEmailVerified()`. A
// callback (rather than importing a cloudMode flag) keeps the
// dispatcher decoupled from server config, matching the OnScopeDenied
// pattern above.
//
// nil → no gate (self-host, stdio, and tests that don't exercise the
// verified-email perimeter). Belt-and-suspenders with the /api/v1
// middleware: even without this hook a synthesized write re-enters
// the /api/v1 chain and the middleware blocks it, but wiring the hook
// stops the request before it's ever built.
RequireVerifiedEmail func(user *models.User) bool
// specialOnce / special lazily cache the specialRoutes map so
// Dispatch doesn't rebuild ~20 bound-method entries per call.
// Internal — see specialRoutes().
specialOnce sync.Once
special map[string]specialDispatchFn
}
// RouteMapper translates a tool's JSON input into a concrete HTTP
// method + path + body. nil body is fine (e.g. for GET requests).
type RouteMapper func(input map[string]any) (method, path string, body []byte, err error)
// routeTable wires cmdPaths (joined with " ") to RouteMappers.
//
// Populated by init() in dispatch_http_routes.go — that file owns the
// declarative spec for every wired command. Commands not in the
// table reach Dispatch only when an MCP client invokes them directly
// and produce a clear "not yet implemented over HTTP transport"
// error from Dispatch.
var routeTable map[string]RouteMapper
// commandsAcceptingAssignByName is the allowlist of cmdPaths whose
// `--assign <name|email>` input should be resolved to a user ID via
// the workspace-members endpoint before the mapper sees it. Other
// commands using an `assign` input pass through unchanged so we
// don't accidentally rewrite a non-assignment use of the same key.
var commandsAcceptingAssignByName = map[string]struct{}{
"item create": {},
"item update": {},
"item list": {},
}
// commandsAcceptingRoleBySlug is the allowlist of cmdPaths whose
// `--role <slug>` input should be resolved to an agent_role_id UUID
// via the agent-roles endpoint before the mapper sees it. Symmetric
// to commandsAcceptingAssignByName.
//
// `item list` is intentionally NOT in this set: the LIST handler
// accepts both UUID and slug at the query-param level (the store's
// list path forks on UUID-vs-slug), so resolving slugs there would
// add an unnecessary hop without any behavioural change. Only
// commands whose handlers require the canonical UUID are listed.
var commandsAcceptingRoleBySlug = map[string]struct{}{
"item create": {},
"item update": {},
}
// noRemoteEquivalent enumerates leaf commands that have no useful
// HTTP-transport mapping because they mutate or inspect local state
// only — config files, MCP-client mcp.json entries, the local server
// process, the local git checkout, the local filesystem path
// arguments (attachments). Distinct from "not yet implemented over
// HTTP transport" because those will eventually land; these never
// will.
//
// The map value is a rationale clause appended to the error message,
// giving agents a hint about why the command is local-only AND what
// the alternative path is (when there is one — e.g. attachments
// suggest fetching bytes via the URL directly + using `attachment
// show` for metadata).
//
// Surfacing the distinction lets agents recognize-and-skip rather
// than retrying or escalating. The "no remote equivalent" prefix is
// stable so downstream tooling can match on it if needed.
var noRemoteEquivalent = map[string]string{
"agent status": "operates on local skill-detection state (~/.claude/, etc.), not the workspace",
"mcp status": "inspects local mcp.json across MCP clients, not the workspace",
"mcp uninstall": "mutates local mcp.json, not the workspace",
"server info": "reports local pad-server process state, not workspace data",
"server open": "launches a local browser, not a workspace operation",
"workspace link": "mutates local .pad.toml, not workspace state",
"workspace switch": "mutates local .pad.toml, not workspace state",
"workspace context": "inspects local .pad.toml, not workspace state",
// `github` commands chain `git rev-parse` + `gh` CLI for the
// branch/PR data they write to the linked item — that data
// inherently lives in the agent's local checkout, not on
// pad-cloud.
//
// These hints USED to send agents to `item update --field
// github_pr=...`, and that advice was wrong: ingestFieldKVP stores
// every `field` value as a STRING, so the PR data lands
// double-encoded, no link appears, and the call reports success —
// the worst shape a workaround can have (BUG-2696, found reviewing
// BUG-2627). Until that is fixed there is no working remote path,
// and saying so is better than sending an agent down one that
// silently does nothing.
"github link": "needs the agent's local git branch + `gh` CLI. There is currently NO working remote alternative: `item update --field github_pr=...` stores the value as a string, so no link appears even though the call succeeds (BUG-2696). Hand PR linking to a human rather than retrying.",
"github status": "needs the agent's local git branch + `gh` CLI; query GitHub directly via the agent's tools",
"github unlink": "needs the agent's local git branch + `gh` CLI. Same as `github link`: `item update --field github_pr=null` stores the string \"null\" rather than clearing the key (BUG-2696), so there is no working remote path today.",
// `project reconcile` shells out to `gh` CLI to compare stored
// PR metadata against live GitHub state — same locality argument.
"project reconcile": "shells out to `gh` CLI to compare stored PR metadata against live GitHub state; agents reconcile via their own GitHub tools + `item update`",
// Attachment commands that take a local filesystem `<path>` /
// `<out-path>` argument — agents on a remote MCP server have no
// shared filesystem. Metadata commands (list, show) are wired;
// upload/download/view are not because the path argument is
// fundamentally local.
"attachment upload": "needs a local filesystem `<path>` arg; agents fetch raw bytes via the attachment URL directly (see `attachment show` for metadata)",
"attachment download": "writes to a local filesystem `<out-path>`; agents read raw bytes via the attachment URL directly (see `attachment show` for metadata)",
"attachment view": "writes to a local filesystem path and prints it; agents read raw bytes via the attachment URL directly (see `attachment show` for metadata)",
}
// Dispatch satisfies the Dispatcher interface. cliArgs are accepted
// for interface compatibility but ignored — HTTPHandlerDispatcher
// reads the structured input attached by the registry via
// WithDispatchInput.
//
// Flow:
//
// 1. Validate dispatcher config (Handler + UserResolver wired).
// 2. Resolve the requesting user from the MCP context.
// 3. Pull the structured input from context.
// 4. Preprocess the input — resolve `--assign <name|email>` to
// `assigned_user_id <uuid>` for the commands that accept it
// (TASK-967). Failures here surface as IsError tool results so
// agents see the resolution error message.
// 5. Special-case routes that need read-modify-write semantics
// (item.update merges existing fields with the update payload —
// the handler treats fields as a complete replacement, so the
// dispatcher does the merge).
// 6. Otherwise, look up a RouteMapper in routeTable, build the
// synthesized request, and execute through the handler chain.
func (d *HTTPHandlerDispatcher) Dispatch(ctx context.Context, cmdPath, _ []string) (*mcp.CallToolResult, error) {
if d.Handler == nil {
return NewErrorResult(ErrorPayload{
Code: ErrServerError,
Message: "HTTPHandlerDispatcher: Handler not configured",
Hint: "Wire d.Handler before invoking Dispatch (programmer error).",
}), nil
}
if d.UserResolver == nil {
return NewErrorResult(ErrorPayload{
Code: ErrServerError,
Message: "HTTPHandlerDispatcher: UserResolver not configured",
Hint: "Wire d.UserResolver before invoking Dispatch (programmer error).",
}), nil
}
cmdKey := strings.Join(cmdPath, " ")
// Reject CLI-only commands up front with a stable, agent-recognizable
// message. These never get an HTTP mapping — the action is
// inherently local-state-only — so failing fast saves agents the
// retry-or-escalate cycle they'd run on a "not yet implemented"
// reply. The rationale clause from noRemoteEquivalent gives the
// agent a hint about WHY (and, for the github / attachment
// commands, the alternative path).
if rationale, local := noRemoteEquivalent[cmdKey]; local {
return NewErrorResult(ErrorPayload{
Code: ErrValidationFailed,
Message: fmt.Sprintf("%s: no remote equivalent — CLI-only command", cmdKey),
Hint: rationale,
}), nil
}
user := d.UserResolver(ctx)
if user == nil {
return NewErrorResult(ErrorPayload{
Code: ErrAuthRequired,
Message: fmt.Sprintf("%s: no authenticated user in context", cmdKey),
Hint: "Re-authenticate (Claude Desktop: reconnect connector; CLI: pad auth login).",
}), nil
}
input := DispatchInputFromContext(ctx)
if input == nil {
// Defensive: registry always attaches input. Empty map keeps
// the mapper from panicking on nil.
input = map[string]any{}
}
// Preprocess input: resolve --assign name → assigned_user_id for
// commands that accept the shorthand. Mappers downstream see only
// the resolved UUID; agents that already pass an ID via
// `--field assigned_user_id=<uuid>` are unaffected.
if _, ok := commandsAcceptingAssignByName[cmdKey]; ok {
var err error
input, err = d.resolveAssignName(ctx, user, input)
if err != nil {
return validationFailedResult(cmdKey, "resolve --assign: "+err.Error(),
"Pass `assign=<user-name|email>` matching a workspace member, or `assigned_user_id=<uuid>` directly."), nil
}
}
// Preprocess input: resolve --role slug → agent_role_id for
// commands that accept the shorthand. Symmetric to the --assign
// preprocess above. Failure here surfaces as IsError so agents
// see the resolution error instead of a silently dropped role
// assignment (the regression the older mapper-level rejection was
// guarding against — Codex review #345 round 1).
if _, ok := commandsAcceptingRoleBySlug[cmdKey]; ok {
var err error
input, err = d.resolveRoleSlug(ctx, user, input)
if err != nil {
return validationFailedResult(cmdKey, "resolve --role: "+err.Error(),
"Pass `role=<slug>` matching an existing agent role, or `agent_role_id=<uuid>` directly."), nil
}
}
// Preprocess input: workspace auto-default from OAuth-token
// allow-list (TASK-1076). When a tool call doesn't carry an
// explicit `workspace` param, try to default from the resolved
// workspaces the lister returns. Single resolved workspace →
// inject; otherwise leave alone (the route mapper will surface
// a "missing required input" error if the route needs it,
// which is informative enough for agents to retry with an
// explicit param). Routes that don't need workspace (like
// `pad_workspace list`) just see an extra unused field — harmless.
//
// Caller-passed workspace ALWAYS wins. Tests + non-OAuth paths
// (no Lister wired) skip the injection entirely so behavior
// stays unchanged for them.
input = d.maybeInjectWorkspace(ctx, input)
// Special-case routes that need read-modify-write or other
// in-handler prefetches. These live as methods on the dispatcher
// because they need the Handler reference; the simple route
// table only carries pure mappers.
if fn, ok := d.specialRoutes()[cmdKey]; ok {
return fn(ctx, input, user)
}
// Item link create/delete commands. The asymmetry between which
// arg drives the URL slug and which drives the body's target_id
// (block: source→URL, target→body; blocked-by: blocker→URL,
// source→body) lives in itemLinkSpecs; the dispatcher just looks
// up the spec and forwards.
if spec, ok := itemLinkSpecs[cmdKey]; ok {
switch cmdKey {
case "item unblock", "item unimplements", "item unsupersede", "item unsplit":
return d.dispatchDeleteItemLink(ctx, input, user, spec)
default:
return d.dispatchCreateItemLink(ctx, input, user, spec)
}
}
routes := d.Routes
if routes == nil {
routes = routeTable
}
mapper, ok := routes[cmdKey]
if !ok {
// Tool exists in the cmdhelp-derived registry but isn't yet
// wired into the HTTP route table. The registry intentionally
// advertises every safe leaf command from PLAN-942's tool
// surface; the route table grows incrementally.
return NewErrorResult(ErrorPayload{
Code: ErrServerError,
Message: fmt.Sprintf("%s: not yet implemented over HTTP transport", cmdKey),
Hint: "This command is registered as an MCP tool but no route mapper has been wired in internal/mcp/dispatch_http_routes.go. File a feature request or use the CLI/stdio MCP path.",
}), nil
}
method, urlPath, body, err := mapper(input)
if err != nil {
return validationFailedResult(cmdKey, err.Error(),
"Check the input shape against the tool's schema (most route-mapper errors are missing required path placeholders)."), nil
}
return d.executeRequest(ctx, cmdKey, user, method, urlPath, body)
}
// specialDispatchFn is the signature shared by the dispatcher's
// special-case route methods.
type specialDispatchFn func(context.Context, map[string]any, *models.User) (*mcp.CallToolResult, error)
// specialRoutes maps cmdKeys to the dispatcher methods that handle
// them in place of a routeTable mapper — commands whose HTTP path
// needs read-modify-write, an in-handler prefetch, or a response
// transformation a pure RouteMapper cannot express.
//
// A map rather than a switch is deliberate (BUG-2304): the
// catalog↔route parity test introspects these keys, so a special-case
// route added here automatically counts as "routed", and a catalog
// action missing from BOTH this map and routeTable fails that test
// instead of shipping as "not yet implemented over HTTP transport".
//
// Built once per dispatcher (sync.Once — Dispatch runs concurrently)
// so the hot path doesn't rebuild a ~20-entry map of bound methods on
// every call (codex round 1 P2).
func (d *HTTPHandlerDispatcher) specialRoutes() map[string]specialDispatchFn {
d.specialOnce.Do(func() {
d.special = d.buildSpecialRoutes()
})
return d.special
}
func (d *HTTPHandlerDispatcher) buildSpecialRoutes() map[string]specialDispatchFn {
return map[string]specialDispatchFn{
"item update": d.dispatchItemUpdate,
"item list": d.dispatchItemList,
"item history": d.dispatchItemHistory,
"item deps": d.dispatchItemDeps,
"item related": d.dispatchItemRelated,
"item implemented-by": d.dispatchItemImplementedBy,
"item bulk-update": d.dispatchItemBulkUpdate,
"item note": d.dispatchItemNote,
"item decide": d.dispatchItemDecide,
"item import": d.dispatchItemImport,
"project ready": d.dispatchProjectReady,
"project stale": d.dispatchProjectStale,
"project next": d.dispatchProjectNext,
"project standup": d.dispatchProjectStandup,
"project changelog": d.dispatchProjectChangelog,
"library list": d.dispatchLibraryList,
"library activate": d.dispatchLibraryActivate,
"attachment list": d.dispatchAttachmentList,
"attachment show": d.dispatchAttachmentShow,
}
}
// executeRequest builds + serves + packages a single HTTP request
// against the wrapped handler. Pulled out of Dispatch so the
// special-case methods (dispatchItemUpdate, future RMW commands) can
// reuse the same auth-context + recorder + response-shaping path.
//
// Scope enforcement lives in buildAuthedRequest (called below) so it
// applies uniformly to every synthesized request — including
// bulk-update's per-item PATCH and the RMW prefetches that don't go
// through this method. Codex review #369 round 2 caught the leak that
// motivated centralizing the check.
func (d *HTTPHandlerDispatcher) executeRequest(
ctx context.Context,
cmdKey string,
user *models.User,
method, urlPath string,
body []byte,
) (*mcp.CallToolResult, error) {
req, err := d.buildAuthedRequest(ctx, method, urlPath, body, user)
if err != nil {
return buildRequestErrorResult(cmdKey, err), nil
}
rec := httptest.NewRecorder()
d.Handler.ServeHTTP(rec, req)
// Use req.Context() (NOT the outer ctx) so the workspace lister
// sees everything buildHTTPRequest + d.Apply attached:
// WithCurrentUser, WithAPITokenAuth, and any TokenAllowedWorkspaces
// the dispatcher's Apply hook layered on. Tests that drive
// executeRequest with context.Background() + a UserResolver get
// the user via req.Context(), not via the outer ctx — without
// this fix the lister would silently return empty hints in
// those test paths and (more critically) in any production
// dispatcher that attaches token state via Apply rather than
// inheriting from the inbound request's ctx. Codex review #379
// round 1.
return packageHTTPResponse(req.Context(), cmdKey, rec.Result(), d.Lister)
}
// buildRequestErrorResult maps a buildAuthedRequest failure onto the
// structured error envelope. Extracted from executeRequest (BUG-2305,
// codex round 1) so hand-written dispatchers that call
// buildAuthedRequest directly (dispatchItemList's summary path)
// surface the SAME permission_denied codes as the routeTable path —
// without the extraction, a scope rejection on those paths degraded
// to a generic server_error.
func buildRequestErrorResult(cmdKey string, err error) *mcp.CallToolResult {
// Most build-request failures are scope-rejection from
// buildAuthedRequest's TokenScopeAllows check (PATCH on a
// read-only token, etc.). Surface as permission_denied so
// agents see the same code as a backend 403, not a
// generic server_error.
if strings.HasPrefix(err.Error(), "permission_denied:") {
return NewErrorResult(ErrorPayload{
Code: ErrPermissionDenied,
Message: fmt.Sprintf("%s: %s", cmdKey, err.Error()),
Hint: "Token scope does not permit this operation. Re-issue with the required scopes (read for GET; write for POST/PATCH; admin for workspace settings).",
})
}
// PLAN-1933 DR-4: the RequireVerifiedEmail gate rejected this
// write because the cloud user hasn't verified their email.
// Surface as permission_denied (the closest closed-set code) so
// agents branch consistently with a backend 403.
if strings.HasPrefix(err.Error(), errEmailNotVerifiedPrefix+":") {
return NewErrorResult(ErrorPayload{
Code: ErrPermissionDenied,
Message: fmt.Sprintf("%s: %s", cmdKey, err.Error()),
Hint: "Verify your email address (check your inbox for the verification link) before creating or editing content.",
})
}
return dispatcherErrorResult(cmdKey, "build request", err)
}
// buildAuthedRequest constructs an in-process HTTP request against
// the wrapped handler with the user attached via WithCurrentUser AND
// any caller-supplied decoration (d.Apply) applied. Used by both
// the main dispatch path and the in-handler prefetches
// (dispatchItemUpdate's GET, lookupAssigneeID's members fetch) so
// token-scope context attached via Apply applies uniformly to every
// synthesized request — no scope bypass on the prefetches.
//
// Codex review #345 round 1 caught the inconsistency: if an OAuth
// middleware attached workspace-allow-list context via Apply, the
// prefetches were bypassing that and could read members / items
// outside the allowed set during resolution.
//
// Scope enforcement (Codex review #369 rounds 1+2):
//
// Every synthesized request — main writes, RMW prefetches,
// bulk-update per-item PATCHes, link-create POSTs — passes through
// here, so this is the single place to enforce the API token's
// scope. The MCP middleware (MCPBearerAuth) stashes
// apiToken.Scopes in context via server.WithTokenScopes; we read
// them here and call server.TokenScopeAllows. Reads
// (GET/HEAD/OPTIONS) under a `["read"]` scope still pass; writes
// fail with a "permission_denied" error that flows up through
// each caller's existing build-error handling.
//
// Centralizing here closes the gap Codex round 2 found in
// dispatch_http_project.go's bulk-update path (PATCH issued
// directly via buildAuthedRequest + ServeHTTP, bypassing
// executeRequest's previous scope check).
// errEmailNotVerifiedPrefix tags the build-request error the
// RequireVerifiedEmail gate returns so executeRequest can map it to a
// clean permission_denied envelope (mirrors the "permission_denied:"
// prefix convention used for token-scope denials).
const errEmailNotVerifiedPrefix = "email_not_verified"
// isMutatingMethod reports whether an HTTP method can mutate server
// state. The verified-email gate only fires on these; reads pass
// through so an unverified user can still fetch context over MCP.
func isMutatingMethod(method string) bool {
switch method {
case http.MethodPost, http.MethodPatch, http.MethodPut, http.MethodDelete:
return true
default:
return false
}
}
func (d *HTTPHandlerDispatcher) buildAuthedRequest(
ctx context.Context,
method, urlPath string,
body []byte,
user *models.User,
) (*http.Request, error) {
if scopes := server.TokenScopesFromContext(ctx); !server.TokenScopeAllows(scopes, method, urlPath) {
// TASK-1119: fire the optional observability hook BEFORE
// returning so the caller's error path is unchanged. Best-
// effort — a panicking observer is the observer's bug; we
// don't recover() because that would mask metric-wiring
// breakage in tests where panics are the right signal.
if d.OnScopeDenied != nil {
d.OnScopeDenied(method, urlPath)
}
return nil, fmt.Errorf("permission_denied: token scope does not permit %s on this resource", method)
}
// PLAN-1933 DR-4: block the remote MCP write path for an unverified
// cloud user. Only mutating methods are gated — the read prefetches
// (dispatchItemUpdate's GET, lookupAssigneeID's members fetch) pass
// through so an unverified user can still read. The errEmailNotVerified
// prefix is recognised by executeRequest to surface a clean
// permission_denied envelope.
if isMutatingMethod(method) && d.RequireVerifiedEmail != nil && d.RequireVerifiedEmail(user) {
return nil, fmt.Errorf("%s: verify your email address before mutating content over MCP", errEmailNotVerifiedPrefix)
}
req, err := buildHTTPRequest(ctx, method, urlPath, body, user)
if err != nil {
return nil, err
}
if d.Apply != nil {
req = d.Apply(req)
}
return req, nil
}
// buildHTTPRequest constructs the in-process request, attaching the
// user via the exported server.WithCurrentUser helper so the handler
// chain treats the call as authenticated. Pulled out so tests can
// inspect / decorate it cheaply.
func buildHTTPRequest(ctx context.Context, method, urlPath string, body []byte, user *models.User) (*http.Request, error) {
// Strip any inherited chi.RouteCtxKey from the inbound context
// before synthesizing the new request. Without this, every
// production /mcp tool call 404s on the synthesized /api/v1/...
// request because chi's Mux.ServeHTTP short-circuits when it
// detects an existing RouteCtxKey:
//
// // chi/v5/mux.go:71-75
// rctx, _ := r.Context().Value(RouteCtxKey).(*Context)
// if rctx != nil {
// mx.handler.ServeHTTP(w, r) // bypass fresh routing
// return
// }
//
// chi assumes "if there's already a route context, I'm being
// invoked as a sub-router from a parent — don't reset state."
// That's correct for chi's own Sub() / Mount() patterns, but
// here we're synthesizing a brand-new request that needs to
// route from scratch against the ROOT mux. The stale RouteCtxKey
// from the inbound /mcp request causes chi to skip its
// rctx.Reset() + RoutePath = "/api/v1/..." setup; the route
// table lookup runs against contaminated routing state and
// falls through to chi's default NotFound handler — whose body
// is the literal "404 page not found\n" the production user
// reported on every dispatcher call.
//
// In tests this never fired because Dispatch was always called
// with context.Background() (no RouteCtxKey to inherit). In
// production every call enters via /mcp's chi-routed handler,
// so the contamination is universal.
//
// Setting the value to a typed nil shadows the parent's value:
// chi's `.(*Context)` type assertion on a context.Value of nil
// returns (nil, false), the `rctx != nil` check fails, and
// chi takes the fresh-routing branch as intended.
//
// Critically we DON'T strip pad's own context values
// (WithCurrentUser, WithAPITokenAuth, TokenScopes,
// TokenAllowedWorkspaces) — those are added below / preserved
// from the inbound request and are exactly what the synthesized
// request needs to authenticate as the same user. We only strip
// the chi-specific routing key.
ctx = context.WithValue(ctx, chi.RouteCtxKey, (*chi.Context)(nil))
var bodyReader io.Reader
if len(body) > 0 {
bodyReader = bytes.NewReader(body)
}
req, err := http.NewRequestWithContext(ctx, method, urlPath, bodyReader)
if err != nil {
return nil, err
}
if len(body) > 0 {
req.Header.Set("Content-Type", "application/json")
}
// Loopback-equivalent — handlers gating on RemoteAddr (e.g. the
// localhost-bootstrap path) treat this as in-process. The auth
// chain sees us as already-authenticated via WithCurrentUser, so
// localhost gating doesn't matter for the per-tool calls; this is
// just to keep the request shape sane for any handler that reads
// it.
req.RemoteAddr = "127.0.0.1:0"
authCtx := server.WithCurrentUser(req.Context(), user)
authCtx = server.WithAPITokenAuth(authCtx)
req = req.WithContext(authCtx)
return req, nil
}
// packageHTTPResponse turns the recorded handler response into an MCP
// CallToolResult, mirroring ExecDispatcher's "JSON → structured + text
// fallback" behaviour. 4xx/5xx surface as structured ErrorEnvelopes
// (TASK-973) so MCP clients see the same closed-set error codes
// regardless of transport.
//
// lister is the dispatcher's WorkspaceLister, supplied so
// classifyHTTPStatus can populate available_workspaces in the
// unknown_workspace envelope. TASK-977 (PLAN-943) wires an
// OAuth-aware lister at production-call sites that filters the
// user's workspaces by the token's allow-list — leaking workspace
// slugs the user didn't consent to expose would be a privacy bug.
// nil → empty available_workspaces (legacy behaviour).
func packageHTTPResponse(ctx context.Context, cmdKey string, resp *http.Response, lister WorkspaceLister) (*mcp.CallToolResult, error) {
defer resp.Body.Close()
bodyBytes, err := io.ReadAll(resp.Body)
if err != nil {
return dispatcherErrorResult(cmdKey, "read response", err), nil
}
body := string(bodyBytes)
if resp.StatusCode >= 400 {
return classifyHTTPStatus(ctx, cmdKey, resp.StatusCode, bodyBytes, lister), nil
}
// Use the shared packager so both transports produce the same
// structured-vs-text shape. In particular, top-level arrays get
// wrapped in `{items: [...]}` for MCP host validators (BUG-985).
return packageJSONResult(body), nil
}
// mapItemCreate translates an `item create` MCP call into a POST to
// /api/v1/workspaces/{ws}/collections/{coll}/items.
//
// The MCP tool schema (auto-generated by registry.go from cmdhelp)
// surfaces:
//
// - workspace (string, injected from session)
// - collection (string, positional arg)
// - title (string, positional arg)
// - content (string flag)
// - status / priority / category / parent (string flags — ROLLED
// into the fields JSON object below to mirror the CLI; the
// handler reads them out of fields, not the top level)
// - field (repeatable kvp flag, --field key=value)
//
// The handler at handleCreateItem in internal/server expects a JSON
// body shaped like models.ItemCreate. The CLI's `pad item create`
// path (cmd/pad/main.go ~L2200) builds a `fields` map from
// --status / --priority / --parent / --category and the repeatable
// --field key=value entries, then JSON-encodes the whole thing into
// ItemCreate.Fields. The handler then unmarshals that string and
// extracts parent / status / priority / etc. We mirror the CLI's
// shape exactly so behavior is identical for both transports.
//
// `parent` is left as a free-form ref string — the handler resolves
// non-UUID refs via store.ResolveItem during create (handlers_items.go
// ~L220), so callers can pass "PLAN-3" or a UUID interchangeably.
//
// `assign` and `role` are intentionally NOT wired here for v1: the
// CLI translates user-name/email → user-ID and role-slug → role-ID
// via additional API calls before posting. Replicating that pre-
// resolution belongs in a follow-up that expands the route table for
// production use; flagging unsupported avoids a partial implementation
// that silently drops them.
func mapItemCreate(input map[string]any) (method, path string, body []byte, err error) {
workspace, _ := input["workspace"].(string)
collection, _ := input["collection"].(string)
if workspace == "" {
return "", "", nil, fmt.Errorf("workspace is required — pass workspace=<slug> explicitly")
}
if collection == "" {
return "", "", nil, fmt.Errorf("collection is required")
}
// Build the fields object the CLI would build, then JSON-encode
// it into ItemCreate.Fields. Order doesn't matter — the handler
// re-decodes and validates against the collection schema.
fields := map[string]any{}
for _, key := range []string{"status", "priority", "category", "parent"} {
if v, ok := input[key]; ok && v != nil {
if s, ok := v.(string); ok && s != "" {
fields[key] = s
} else if !isStringType(v) {
// Non-string types (e.g. number from a typed flag) —
// pass through verbatim and let the handler validate.
fields[key] = v
}
}
}
// Repeatable --field key=value flags overlay onto the fields map.
// Ordering matches the CLI: explicit --field entries CAN
// override the named flags above (last-write-wins per key) so an
// agent can set custom fields the schema declares without us
// having to teach the route mapper about every collection.
if rawFields, ok := input["field"]; ok {
extra, err := parseFieldKVP(rawFields)
if err != nil {
return "", "", nil, fmt.Errorf("parse --field: %w", err)
}
for k, v := range extra {
fields[k] = v
}
}
// The `fields` object with its JSON types intact, applied LAST so it wins
// over the stringified copy of itself in `field` (BUG-2850). This is the
// only door where those types survive the trip — parseFieldKVP above turns
// every entry into a string, which is what made a number field unwritable
// and an undeclared number land as "42". The two forms carry the same
// input, so last-write-wins here is a fidelity choice, not a precedence
// one; genuine key conflicts were already refused in the catalog merge.
for k, v := range nativeFields(input) {
fields[k] = v
}
payload := map[string]any{}
for _, key := range []string{"title", "content", "slug"} {
if v, ok := input[key]; ok {
payload[key] = v
}
}
// Lift recognized column keys out of the fields blob into the
// top-level payload. The MCP tool schema (auto-generated from
// cmdhelp) doesn't expose `agent_role_id` or `assigned_user_id`
// as top-level inputs — only `--role` and `--assign` — so
// agents reaching for these column writes via the schema-
// visible escape hatch (`--field agent_role_id=<uuid>`) would
// otherwise have the value sit inert inside the fields JSON
// instead of going to the column the handler writes to.
// (Codex review #345 round 3.)
liftFieldsToColumns(fields, payload)
if len(fields) > 0 {
fb, mErr := json.Marshal(fields)
if mErr != nil {
return "", "", nil, fmt.Errorf("encode fields: %w", mErr)
}
payload["fields"] = string(fb)
}
// Tags are a top-level []string on ItemCreate, separate from
// the fields JSON. Pass through verbatim if provided.
if v, ok := input["tags"]; ok {
payload["tags"] = v
}
// `--assign` resolution lives at the dispatcher level (TASK-967):
// Dispatch's preprocess step rewrites it to `assigned_user_id`
// before the mapper runs, so by the time we get here a name has
// already been resolved to a UUID. If the resolved ID is set,
// pass it through to the handler.
// The empty-string filter STAYS on create, unlike the matching
// branches in mapItemUpdate (TASK-2571). On an update `""` is a
// meaningful instruction — clear the assignment — but a brand-new
// item has no assignment to clear, and CreateItem nulls an empty ID
// anyway (BUG-2566), so forwarding or dropping it produce the same
// row. The filter is kept because it makes the no-op explicit rather
// than incidental; there is no behaviour riding on this line.
if v, ok := input["assigned_user_id"].(string); ok && v != "" {
payload["assigned_user_id"] = v
}
// `agent_role_id` (UUID) passes through directly to the
// ItemCreate column. Agents that know the role's UUID (e.g.
// from a prior `role list` call) can set it without round-
// tripping through `--role` slug resolution.
if v, ok := input["agent_role_id"].(string); ok && v != "" {
payload["agent_role_id"] = v
}
// `--role` is now resolved at the dispatcher level (TASK-968):
// Dispatch's preprocess step rewrites the slug to the canonical
// `agent_role_id` UUID via the agent-roles endpoint before the
// mapper runs. By the time we get here either:
//
// - input had no `role` key — pass-through above already
// handled an explicit `agent_role_id`.
// - input had `role: <slug>` and resolution succeeded — the
// preprocess set agent_role_id and removed `role`, again
// handled by the pass-through above.
// - resolution failed — Dispatch returned IsError before
// calling the mapper, so we don't reach this point.
//
// The mapper just trusts the resolved input. The pass-through
// for explicit `agent_role_id` (above) is the only role-related
// branch that needs to live in the mapper itself.
body, err = json.Marshal(payload)
if err != nil {
return "", "", nil, fmt.Errorf("encode body: %w", err)
}
// Send the collection slug verbatim. This dispatcher runs in-process
// against the same binary, whose create handler resolves shorthand
// server-side with exact-match-first (BUG-2578), so the raw slug keeps
// `item.create(collection: "task", ...)` working AND stops the old
// client-side alias from shadowing a real collection whose slug IS a
// singular like "task" (BUG-2630). The ExecDispatcher path (local stdio)
// gets the same behaviour through the CLI's alias-fallback helper.
urlPath := fmt.Sprintf("/api/v1/workspaces/%s/collections/%s/items",
url.PathEscape(workspace), url.PathEscape(collection))
return http.MethodPost, urlPath, body, nil
}
// isStringType returns true when v is a Go string. Used by the
// fields-builder to distinguish "real value to pass through" from
// "empty/missing".
func isStringType(v any) bool {
_, ok := v.(string)
return ok
}
// columnFieldKeys is the set of fields-blob keys the dispatcher
// recognizes as actually being column writes on the underlying
// item record. When agents pass these via `--field key=value` (the
// only schema-visible path until cmdhelp surfaces them as their own
// flags), the dispatcher lifts them out of the fields JSON into the
// top-level payload so the handler writes the column rather than
// stuffing the value inside the JSON blob.
//
// Adding a key here is a behavioural extension — agents now get
// column writes via --field instead of the inert fields-blob
// no-op. Keep the list short; a real flag in cmdhelp is preferable
// long-term (the eventual TASK-968 follow-up should add named
// inputs for these).
//
// INVARIANT (TASK-2571): every key here must have defined
// clear-to-NULL semantics for the empty string at the store, because
// liftFieldsToColumns forwards `""` verbatim rather than skipping it.
// Both current members do (BUG-2566). A key whose column would instead
// be CORRUPTED by an empty write — `tags` is the canonical example,
// JSONB on Postgres — does not belong in this list at all.
var columnFieldKeys = []string{
"agent_role_id",
"assigned_user_id",
}
// liftFieldsToColumns scans fields for keys that map 1:1 to columns
// on the item record and moves them into payload at the top level.
// Mutates both maps. Caller-supplied top-level values win — so an
// agent that already passed `agent_role_id` directly (e.g. via a
// future named flag) doesn't get clobbered by a stray --field entry.
func liftFieldsToColumns(fields, payload map[string]any) {
for _, key := range columnFieldKeys {
v, ok := fields[key]
if !ok {
continue
}
// Always remove from the fields blob — even if payload
// already has it. The fields blob is the wrong home and
// leaving the duplicate would invite divergence between the
// JSON value and the column.
delete(fields, key)
if _, alreadyTopLevel := payload[key]; alreadyTopLevel {
continue
}
// An empty string used to be dropped here, which made
// `--field assigned_user_id=` a silent no-op — the only
// MCP-reachable way to unassign an item, and it did nothing
// (TASK-2571). It is now forwarded, because for every key in
// columnFieldKeys `""` means clear-to-NULL at the store
// (BUG-2566). See that list's INVARIANT before adding a key.
payload[key] = v
}
}
// parseFieldKVP normalizes the --field flag's various wire shapes
// (single string, []string, []any) into a `key→value` map. A MALFORMED
// entry — empty, or with no usable `key=value` split — is skipped
// silently, matching the CLI's permissive behaviour on that one case. A
// padded KEY is a different thing and is REFUSED, aborting the whole call
// rather than dropping one entry (BUG-2870).
// nativeFields returns the `fields` object the catalog merge preserved with
// its JSON types intact, or nil. See fieldsNativeKey (BUG-2850).
func nativeFields(input map[string]any) map[string]any {
m, _ := input[fieldsNativeKey].(map[string]any)
return m
}
func parseFieldKVP(raw any) (map[string]any, error) {
out := map[string]any{}
switch v := raw.(type) {
case []any:
for _, e := range v {
s, ok := e.(string)
if !ok {
return nil, fmt.Errorf("expected string entries, got %T", e)
}
if err := ingestFieldKVP(s, out); err != nil {
return nil, err
}
}
case []string:
for _, s := range v {
if err := ingestFieldKVP(s, out); err != nil {
return nil, err
}
}
case string:
if err := ingestFieldKVP(v, out); err != nil {
return nil, err
}
default:
return nil, fmt.Errorf("expected array or string, got %T", raw)
}
return out, nil
}
// ingestFieldKVP adds one `key=value` entry to dst, through the SAME parse the
// CLI doors use (BUG-2870). It used to TrimSpace both halves here and nowhere
// else, which is what made one call store two different keys depending on the
// transport: `field:[" effort=l"]` wrote `effort` here and an undeclared
// " effort" at the CLI.
//
// Both halves of that divergence close in the same direction, but not the same
// way: a padded KEY is now REFUSED at both doors rather than trimmed at one,
// and a VALUE is carried VERBATIM at both rather than trimmed at one. The
// second is the behaviour change a caller can notice — a padded value that
// this door silently accepted (and typed) is now passed through and refused
// one layer down by field validation, with a message naming the field, which
// is the same answer the CLI has always given.
func ingestFieldKVP(s string, dst map[string]any) error {
key, val, err := items.SplitFieldEntry(s)
if errors.Is(err, items.ErrFieldEntryMalformed) {
return nil // skipped silently, matching the CLI doors
}
if err != nil {
return err
}
dst[key] = val
return nil
}
// mapPlaybookRun handles `pad_playbook.action=run` for the HTTP MCP
// dispatcher. Accepts both the original MCP shape (args:map +
// raw_args:[]string) and the flattened shape that
// actionPlaybookRun emits when dispatching through ExecDispatcher
// (args:[]string of CLI tokens). When args arrives as a slice it's
// treated as raw_args so the server applies the strict CLI parser.
// When it arrives as a map it's forwarded as a pre-parsed argument
// dictionary.
//
// PLAN-1377 / TASK-1381.
func mapPlaybookRun(input map[string]any) (method, path string, body []byte, err error) {
workspace, _ := input["workspace"].(string)
if workspace == "" {
return "", "", nil, fmt.Errorf("workspace is required — pass workspace=<slug> explicitly")
}
ref, _ := input["ref"].(string)
if ref == "" {
return "", "", nil, fmt.Errorf("ref is required (invocation_slug, item slug, or issue ref)")
}
payload := map[string]any{}
// Coerce raw_args from any of the shapes the catalog / cli /
// HTTPDispatcher paths can supply. The server's parser only cares
// about []string, so anything else gets normalized here.
var rawArgs []string
if rv, ok := input["raw_args"]; ok && rv != nil {
switch v := rv.(type) {
case []any:
for _, t := range v {
if s, ok := t.(string); ok && s != "" {
rawArgs = append(rawArgs, s)
}
}
case []string:
rawArgs = append(rawArgs, v...)
}
}
if argv, ok := input["args"]; ok && argv != nil {
switch v := argv.(type) {
case map[string]any:
payload["args"] = v
case []any:
for _, t := range v {
if s, ok := t.(string); ok && s != "" {
rawArgs = append(rawArgs, s)
}
}
case []string:
rawArgs = append(rawArgs, v...)
}
}
if len(rawArgs) > 0 {
payload["raw_args"] = rawArgs
}
if ad, ok := input["allow_draft"].(bool); ok && ad {
payload["allow_draft"] = true
}
enc, err := json.Marshal(payload)
if err != nil {
return "", "", nil, fmt.Errorf("encode playbook run body: %w", err)
}
return http.MethodPost, "/api/v1/workspaces/" + workspace + "/playbooks/" + ref + "/run", enc, nil
}