Repository navigation
Add full MCP diagnostics with API discovery and source links - #35
Conversation
Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
frahlg
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33cc8512f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| name: "api_read", title: "Read full technical diagnostic data", | ||
| description: "Use during technical troubleshooting after api_catalog. Read any listed path, replacing {id} or {name} with the URL-encoded real value returned by the box. Technical names, addresses, exact numbers, source code and provenance stay intact. Use /api/support/report for an overview; /api/energy/history for Wh and provenance; /api/series for original measurements; /api/mpc/diagnose/history then /api/mpc/diagnose/at for incident planning. JSON, CSV, text and binary exports are supported. Large answers have byte pages: pass next_offset as offset and sha256 as if_sha256 to detect changed data. For ZIP or tar.gz exports, use archive_member '.' to list files, then a listed member name to read its contents as text. Binary pages use base64. Time parameters accept Unix milliseconds or RFC 3339 with a timezone. Reads use the existing handlers; controls and settings use their command tools." + dataNote, | ||
| scope: readScope(protocol.MsgAPIRead), readOnly: true, |
There was a problem hiding this comment.
This tool is authorized by the existing diag.run scope, which is granted by default and is presented on the consent page as merely “Run checks that change nothing” (internal/oauth/oauth.go:92-112). Consequently, every previously authorized assistant with the default read scopes gains access after upgrade to the newly exposed full API—including the unmasked backups and persistent credentials described by this change—without the owner ever consenting to that disclosure. Put these exports behind a new explicit scope, or otherwise require renewed consent with an accurate description.
Useful? React with 👍 / 👎.
| if !json.Valid(body) || dec.Decode(&data) != nil || data == nil { | ||
| return nil, userError("The box's diagnostic answer was not a JSON object.") | ||
| } | ||
| data["path"], data["status"] = read.Path, head.Status |
There was a problem hiding this comment.
When a catalogued endpoint returns a top-level path or status field, these assignments silently replace the handler's original diagnostic evidence with the request path and HTTP status. This is especially unsafe now that arbitrary box-discovered and future routes are accepted, because Cloud cannot assume those field names are unused. Return transport metadata alongside a nested body, as the earlier implementation did, rather than mutating the decoded response object.
Useful? React with 👍 / 👎.
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
| if c.queued > 0 { | ||
| if c.queued > 0 || len(c.turn) > 0 { |
There was a problem hiding this comment.
This protects a long-running read from the periodic idle sweep, but evictLocked still chooses solely by used and does not check queued or turn (internal/mcp/agent.go:227-240). Since used is not refreshed again until the call releases its turn, a diagnostic read running for several minutes can become the oldest connection and be closed when creation of another session reaches maxConns, aborting an otherwise healthy export. Apply the same busy-session exclusion to capacity eviction.
Useful? React with 👍 / 👎.
1 participant
MCP's existing summaries do not provide enough evidence for deep fault finding or a bug report.
Add
api_catalogas the entry for technical troubleshooting andapi_readfor the connected box's full read API. The box returns its actual route catalogue, running version, source-code links and a bug-evidence workflow. Transport metadata sits beside the box response and never replaces its fields. Capacity eviction also preserves active, queued and opening sessions; a full busy pool refuses a new call. The consent page names the full diagnostic access and backup credentials. Cloud keeps no endpoint list and adds no per-route permission check: the owner's diagnostic grant covers these reads. Normal status, history and control keep their existing tools.Reads retain technical names, addresses, exact numeric values, source code and provenance. Existing Core handlers keep their own credential masking. Full backups can include unmasked credentials and persistent state; the full diagnostic grant intentionally covers them. Diagnostic calls have a five-minute budget; busy sessions survive idle cleanup and the body-read deadline ends after reading the request. Large responses use byte pages and SHA-256 checks; ZIP/tar.gz members can be listed and read on the box. The MCP tool forwards offset, page size, digest and archive-member arguments. It rejects incomplete or out-of-order wire streams and never presents a broken stream as a successful partial answer. Time queries accept Unix milliseconds or RFC 3339 with a timezone. URL-encoded path parameters keep driver names with spaces, slashes or percent signs intact.
The protocol pin names Core commit
0546e9d76deafrom Core #1726, including the full API from merged Core #1723. That Core capability must ship before users can use it. An older box gets a message naming the needed Core support; the existing typed tools keep working. Contract pairs: webapp #98 and native #91.Validation covers the local OAuth and Noise flow, box-supplied discovery, versioned source links, bug-report guidance, full diagnostic access without a second scope, deliberately narrowed tokens, exact numbers, technical text, paging arguments, invalid paths and arguments, truncation, skipped chunks, timezone conversion and older-box behavior. Core's paired tests cover the real energy store, route coverage, revocation and archive contents. No real assistant-account test has run for this feature.
The MCP service grows by 214 lines of Go excluding tests. Uplink grows by 2 lines. OAuth completes only after the signed roster is committed, so the first MCP call can read the box key. A regression test checks the callback against both committed roster revisions. The consent text was rendered and checked in Chrome with a local test page.
GOWORK=off go vet ./...GOWORK=off go test -race -count=1 ./...GOWORK=off go test -count=1 -tags ftwclouddev ./...GOWORK=off bash scripts/check-imports.shftwclouddandftwkeysd.The full diagnostic scope intentionally applies to existing MCP grants as requested by the product owner. This change does not require a second grant for exports.