Repository navigation
feat(s16): connect GitHub and reviewed HTTPS MCP, OpenAPI and GraphQL tool sources - #448
Conversation
…evisions POLICY's membership exemption (ADR 0088) now also ignores resources and recipients a work does not name, dropping rules left without any of its resources when all their adapters are resource-bound. The default proposal puts connected tool sources in their own connected_sources rule, so connecting or removing one never changes a rule other work uses.
… sources Adds s16-connections/v1 behind connections: true: the client Connections page for the GitHub app (token connect, reconnect, revoke) and HTTPS tool sources (none, bearer, header or MCP OAuth with registration, PKCE and single-use state). Discovered tools are data until the owner's one-time review; reads run as POLICY reads, changes and approval-gated calls as s16_source actions fenced by the reviewed catalog. MCP sessions are per run, sends are exactly-once with unknown outcomes kept reconcile-only, requests use the public-only checked egress without redirects, and a Bot's propose_connection card opens the protected form. Bot settings can exclude a connection per Bot.
Deterministic importer/network/MCP unit tests, composed scenarios for review, sessions, held calls, drift, unknown outcomes, revocation, OAuth, OpenAPI/GraphQL, GitHub and per-Bot exclusions, and a real-bundle browser test in English and Thai.
[High impact] The PR lets Bots make reviewed calls to owner-connected services under shared policy controls. Fix command recovery and the remaining open-stream timeout before merging. Findings
Fix with agent prompt### Issue 1
src/s16/connections/service.ts:95-98
`#save()` attaches every source change to the awaited command in `#active`, but OAuth callbacks run outside the command queue. If a sign-in is declined while `github.connect` waits on the network, the callback records its own source under the GitHub command's request ID.
If that command fails or the process stops, retrying it enters `#resume()` with the unrelated source and returns a receipt without finishing the GitHub connection. Record pending work explicitly for the command's own changes, rather than from every `#save()`.
### Issue 2
src/s16/connections/mcp.ts:undefined-142
If an MCP server sends the matching SSE response but leaves the HTTP stream open, `bounded()` keeps waiting for the body to end. Discovery times out and calls become `unknown` despite receiving their result. Read SSE events as they arrive and stop once the matching response is complete, while keeping the byte limit.
### Issue 3
src/s16/connections/openapi.ts:75-78
The importer drops parameter `style` and `explode`, while the request builder always repeats query keys and rejects object values. An array with `explode: false` is sent as `id=1&id=2` instead of one comma-separated value. A required object query parameter is accepted for review but can never run. Preserve supported encoding choices and explicitly skip operations whose parameters cannot be encoded.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryAdds client-managed GitHub connections and reviewed HTTPS MCP, OpenAPI and GraphQL tool sources.
|
| if (this.#closed) throw new AppError("source_session_closed", 409); | ||
| const id = this.#id++, body = JSON.stringify({ jsonrpc: "2.0", id, ...message }); | ||
| if (Buffer.byteLength(body) > connectionLimits.argumentBytes + 4_096) throw new AppError("source_arguments_too_large"); | ||
| const response: SourceResponse = await this.network.request(this.endpoint, { method: "POST", headers: await this.#headers(initial), body, ...(signal ? { signal } : {}), timeoutMs, maxBytes: connectionLimits.responseBytes }); |
There was a problem hiding this comment.
Completed MCP replies time out
If an MCP server sends the matching SSE response but leaves the HTTP stream open, bounded() keeps waiting for the body to end. Discovery times out and calls become unknown despite receiving their result. Read SSE events as they arrive and stop once the matching response is complete, while keeping the byte limit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/s16/connections/mcp.ts
Line: 142
Comment:
**Completed MCP replies time out**
If an MCP server sends the matching SSE response but leaves the HTTP stream open, `bounded()` keeps waiting for the body to end. Discovery times out and calls become `unknown` despite receiving their result. Read SSE events as they arrive and stop once the matching response is complete, while keeping the byte limit.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 2841461: event-stream reads stop as soon as the matching JSON-RPC response has arrived (byte limit kept); covered by a test with a stream the server never closes.
Approved calls accept DATA15's added resources; OAuth refreshes are single-flight and never overwrite a newer credential; commands recorded as pending finish after a restart; event-stream replies end at their matching response and failed opens end the assigned session; long tool names stay unique; a run offers at most 64 tools with 16 KB schemas; OpenAPI parameters keep their encoding or the operation is skipped, and repeated path placeholders all resolve; GraphQL variables keep nullability; each connection form keeps its own credentials; the chat return survives an OAuth sign-in; copy avoids markup characters.
| #save(record: SourceRecord): void { | ||
| this.store.db.query("INSERT INTO sources VALUES (?,?) ON CONFLICT(id) DO UPDATE SET record_json=excluded.record_json").run(record.id, canonical(record)); | ||
| // Recorded with the command's first change, so a retry after a stop finishes the command instead of conflicting. | ||
| if (this.#active) this.store.db.query("INSERT OR IGNORE INTO pending VALUES (?,?,?)").run(this.#active.requestId, this.#active.hash, record.id); |
There was a problem hiding this comment.
Retries can skip connection commands
#save() attaches every source change to the awaited command in #active, but OAuth callbacks run outside the command queue. If a sign-in is declined while github.connect waits on the network, the callback records its own source under the GitHub command's request ID.
If that command fails or the process stops, retrying it enters #resume() with the unrelated source and returns a receipt without finishing the GitHub connection. Record pending work explicitly for the command's own changes, rather than from every #save().
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/s16/connections/service.ts
Line: 95-98
Comment:
**Retries can skip connection commands**
`#save()` attaches every source change to the awaited command in `#active`, but OAuth callbacks run outside the command queue. If a sign-in is declined while `github.connect` waits on the network, the callback records its own source under the GitHub command's request ID.
If that command fails or the process stops, retrying it enters `#resume()` with the unrelated source and returns a receipt without finishing the GitHub connection. Record pending work explicitly for the command's own changes, rather than from every `#save()`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 3c65db1: the pending record is now scoped to the command's own source (derived source ID for source.add, the command's sourceId otherwise; GitHub and card commands record none), so an OAuth redirect arriving mid-command never attaches. Regression test drives a declined sign-in while github.connect waits on the network (fails without the fix).
An OAuth redirect runs outside the command queue; its source change no longer attaches to whichever command is awaiting the network.
Closes #417
Closes #418
Closes #419
Refs #392
What this delivers
Behind explicit
connections: true(set by the generated installer; compositionconnections: { vault }, requires DATA15), contracts16-connections/v1, decision record ADR 0089:/ruleswhen nothing else differs. A Bot'spropose_connectiontool shows an inert chat card that opens the protected form prefilled and returns to the conversation. Every connection is a team resource for all Bots;bot.updatecan now carry the Bot's exactexcludedresources (Bot settings).#/refs only, bounded) and GraphQL owner-written named operations (trusted parser, depth/field bounds, no introspection, subscriptions unsupported).source.documentreplaces the document (drift → re-review).Shared contract for all sources:
readOnlyHint, GET, query) are only labelled suggestions; anything not confirmed as a read defaults to needs my approval. A GraphQL mutation can never be reviewed as a read. Any change to names, text, hints, schema or binding changes the catalog hash →needs_review, tools withdrawn, held cards stale (catalog fence).s16_sourceactions withordinary_edit; approval tools carrymajor_changeand wait for the one-press in-chat card. Sources get their ownconnected_sourcesrule. POLICY's membership exemption is generalized so connecting/removing a source does not stop work that does not use it.already_claimed; lost replies areunknownand stay reconcile-only across restarts; OpenAPI/GraphQL writes sendIdempotency-Key: <effect key>.No upstream source or dependency was imported (pinned Rakazo connector stack compared and rejected, see ADR). A small global CSS fix keeps hovered primary buttons readable.
Contract versions
s16-connections/v1(s16-source/v1,s16-source-catalog/v1,s16-connection-command/v1,s16-connection-receipt/v1,s16-client-connections/v1,s16-source-read/v1,s16-source-call/v1,s16-source-result/v1); additive:ClientCapabilities.connections,bot.update.excluded, policy-review row kindconnection, runtime configconnections.Tests (tested head = this PR's head)
bun run typecheck,bun run upstream:check,git diff --check— pass.bun test tests/s16-connections-import.test.ts tests/s16-connections.test.ts— 12 pass (importers, schemas, private-address/metadata/redirect refusals, MCP sessions, review and membership revision, read/change/held calls, hint/schema drift, lost replies, concurrent run sessions, revoke during an awaited call, OAuth wrong/replayed state and single-use code, OpenAPI read + held write + drift + unknown write, GraphQL mutation-as-read refusal, GitHub connect/select/reconnect/revoke, two Bots and per-Bot exclusion, token echo redaction).JIT_S16_BROWSER_TESTS=1 bun test tests/s16-connections.browser.test.ts— 2 pass (English 1440 px, Thai 390 px; chat card → form → review → in rules; no overflow, no external requests). Also re-rans16-bots.browserands16-approval.browser— pass.s15-policy,s15-first-run,s15-composition,s15-web-i18n,s15-web-setup,s15-installer,s15-github-connection,s15-conversation-policy,s16-action-card,s16-autonomy-policy,s16-bots,s16-client,s16-computers,s16-upstream— all pass. Full suite runs in CI.Pending / honest limits
tools/list_changedare unused (refresh is explicit); OAuth servers without dynamic registration, OpenAPI header/cookie parameters and non-JSON bodies are unsupported.review_required→/rules).