Skip to content

fix(connector-mcp): an include a credentialed MCP server does not list degrades the instance as a credential refusal, not the boot - #28357

Merged
objectstack-fleet[bot] merged 9 commits into
mainfrom
claude/issue-28311-mcp-scope-hidden-tools
Nov 1, 2026
Merged

objectstack-fleet[bot] merged 9 commits into
mainfrom
claude/issue-28311-mcp-scope-hidden-tools

Conversation

@objectstack-fleet

Copy link
Copy Markdown
Contributor

Fixes #28311
Clause-②: no
Metadata-protocol: none

维护者速读

  • 改了什么:MCP 连接器实例发送了凭据、而服务器的 tools/list 没有列出 include 点名的工具时,不再按“声明错误”让整个服务启动失败,而是按“凭据问题”把该实例降级,存储的凭据标为 rejected;没有凭据的实例行为完全不变。
  • 为什么改:GitHub 这类服务器按令牌权限过滤工具列表,权限不够的令牌放在环境变量里会让整个运行时下次重启起不来,放在凭据存储里又提示管理员“去修声明”;能修的其实是凭据,而列表本身分不清是“被隐藏”还是“根本没有”,所以提示同时写明两种原因。
  • 风险与回滚:只改变“实例发送了 auth 凭据且工具缺失”这一种情况的归类,复用已有的 ConnectorCredentialRejectedError 与 rejected 状态,没有新类型、新状态。回滚即还原 mcp-provider.ts 里的 if (credentialSent) 分支。
  • 先看:packages/connectors/connector-mcp/src/mcp-provider.ts(if (credentialSent) 分支与 403 的理由)、packages/connectors/connector-mcp/src/mcp-include-credential-scope.test.ts(环境变量路径与存储路径两条启动测试)、content/docs/automation/connectors.mdx。
  • 两分钟验证:依赖闭包构建后运行 pnpm --filter @objectstack/connector-mcp exec vitest run src/mcp-include-credential-scope.test.ts,预期 Tests 9 passed (9)。
  • 需要你做的:无,席位落地。

What changes

  • @objectstack/connector-mcp (mcp-provider.ts). When providerConfig.include names a tool the server's tools/list does not carry, the refusal now depends on whether the handshake carried the instance's credential (an http transport with the instance auth sent as a header — the same predicate credentialVerified already used):
    • carried: ConnectorCredentialRejectedError, status 403. The message names the missing tools, the tools the server lists for that credential, and both causes with the fix for each (the credential lacks the scopes those tools need, or the declaration names a tool the server does not have). The materializer's existing credential path then applies: degraded, boot not failed; a stored credential marked rejected and held; an environment or vault credential retried on the backoff.
    • not carried (no auth, an api-key sent as a query parameter, a stdio transport, or a credential written into providerConfig.transport.headers, which is part of the declaration): the plain configuration fault, byte-identical message, fatal at boot as before.
  • Docs. content/docs/automation/connectors.mdx: the include paragraph, the "credential the upstream refuses" paragraph and two rows of the failure-mode table. packages/apps/connector-github/README.md: the Credentials paragraph no longer says an environment-variable token stops the server from starting.
  • Contract text the change would otherwise make false. ConnectorCredentialRejectedError / CONNECTOR_CREDENTIAL_REJECTED TSDoc in packages/spec (comments only, no type change) and the sys_connector_credential.status field description now name the second way the marker is raised.
  • Tests. src/mcp-include-credential-scope.test.ts (new): the three probes the card was filed from, made permanent on the real provider, the real materializer and the real credential store (ObjectQL over SQLite), plus controls. For the store, connector-mcp gains three devDependencies (@objectstack/objectql, @objectstack/driver-sql, @objectstack/platform-objects, all already in its build closure) and a vitest.config.ts that reads them from source.
  • Changeset. @objectstack/connector-mcp patch, @objectstack/service-automation patch (the field description).

Tests

At head b6312fe6:

  • pnpm --filter @objectstack/connector-mcp test: Test Files 6 passed (6), Tests 65 passed (65); typecheck exit 0, test-typecheck ledger unchanged (1 file(s) / 5 error(s), the new file adds none).
  • pnpm --filter @objectstack/connector-app-github test (validate + vitest): Validation passed, Test Files 2 passed (2), Tests 10 passed (10); typecheck exit 0.
  • @objectstack/service-automation (the description edit), at c7628191, no later commit touches it: Test Files 237 passed (237), Tests 2985 passed (2985); typecheck exit 0.
  • The new file, 9 cases: credentialed refusal (403, both causes, missing and listed tools, client closed, no token in the message); controls — no auth, api-key as a query parameter, a credential in transport.headers, stdio with auth, and a credential whose listing carries every entry (materializes, credentialVerified); through the materializer — environment credential (boot not failed, degraded, listed rejected / env, not held, recovers on a reconcile once the same token's scopes cover the tools), stored credential (degraded with the credential reason, row rejected with that reason, held as credential, a reconcile dials nothing, a better-scoped value recovers it with lastVerifiedAt set; neither token in any log line; no "Fix its declaration" line), and no credential (boot still fails with the declaration fault).
  • Ablation, once, at a51f1291: node scripts/ablation-replace.mjs replaced if (credentialSent) { with if (false as boolean) { (anchor 1 to 0, blob 05b010b670de to 2dd4cd2773e3): Tests 3 failed | 6 passed (9), the three credential-path cases red, every control green; restored, blob equals HEAD, git diff HEAD empty. The test imports the provider by relative path from src, so no build sits between the mutation and the run.

Acceptance notes

  • Can the server tell the two causes apart? Measured: no. github/github-mcp-server at f2a07cd1: the scope filter is applied to the inventory before tools/list (pkg/http/handler.go PATScopeFilter, pkg/github/scope_filter.go), the result carries no field about hidden tools, and the HTTP layer sets no scope header on that answer. The only scope signal is WWW-Authenticate: Bearer error="insufficient_scope" with 403 on a tools/call (pkg/http/middleware/scope_challenge.go). So the message names both causes, and status 403 follows that same convention (RFC 6750 section 3.1); the message itself quotes no HTTP status.
  • The GitHub app's own two tools are never hidden by GitHub's server. At f2a07cd1, search_issues and issue_read are declared with scopes.PublicRead(scopes.Repo), whose visibility check returns true for any scopes (pkg/scopes/scopes.go, PublicRead), matching GitHub's docs/scope-filtering.md "Public Repository Access". The defect is reachable with GitHub through other tools (write tools, notifications, project and org tools) and with any server that filters by credential. The README paragraph now says so.
  • Row state. The existing rejected state is reused; no new state. It clears when the value is set again or a handshake lists every entry (markVerified). A credentialed instance whose include has a real typo now marks its stored credential rejected too; the message names that cause, and editing the declaration re-attempts it and clears the mark on success.
  • Where the probes live. In connector-mcp's own tests, which run without the app; packages/apps/connector-github/test/boot.test.ts is unchanged.
  • packages/services/service-automation/src/plugin.ts is not touched. Its hold log line ("its upstream refused its credential") reads acceptably for this case and is left alone.
  • origin/main was merged at efdddc90, after feat(apps): connector-github, GitHub's remote MCP server as an open-edition connector app (E4 batch 1, GitHub half) #28306 landed. Main has moved since; GitHub's merge of the newer commits is clean (git merge-tree exit 0), and none of them touches these packages.

Verification

Gate families derived for this diff: 114 run, 0 NOT MEASURED

node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack at b6312fe6: 114 commands for 10 paths, 569 changed lines (+533 / -36) (under the 3000 threshold). Every command was run with its exit code captured before any pipe. 112 exited 0 on the first pass. pnpm --filter @objectstack/spec run check:skill-examples and pnpm check:dual-build-cjs-loads exited 3 (PREREQUISITE NOT MET: unbuilt client-react and siblings). Re-run once the dists existed, both exited 0 (277 prose examples type-check across 3 surface(s); 113 published require entry point(s) across 70 package(s) load). --ran over the final record: 114 derived famil(ies) accounted for — 114 run, 0 NOT-MEASURED.

Not run locally, by rule

test:repo, pnpm lint and the other repo-wide scans belong to CI. Dogfood does not apply: no ! in the title and no (narrowing) arm.


Generated by Claude Code

…kes the credential path

When providerConfig.include names a tool the MCP server's tools/list does
not carry and the handshake carried the instance's credential, the refusal
is raised as ConnectorCredentialRejectedError (status 403), naming both
causes the answer cannot tell apart: the credential's scopes hide the tool,
or the declaration names a tool the server does not have. An instance whose
handshake carried no credential keeps the plain configuration fault.

Claude-Session: https://claude.ai/code/session_017xRBcVBKAFk1ntXSjvPt7U
Co-authored-by: Claude <noreply@anthropic.com>
…ialed server hides, with docs and changeset

The three probes as permanent tests, on the real provider and materializer
and the real credential store (ObjectQL over SQLite): an environment
credential degrades the instance instead of failing the boot and recovers
once the scopes cover the tools; a stored credential is marked rejected and
held, and a better-scoped value recovers it without a restart; with no
credential the boot still fails as a declaration fault.

Claude-Session: https://claude.ai/code/session_017xRBcVBKAFk1ntXSjvPt7U
Co-authored-by: Claude <noreply@anthropic.com>
…e rejected row also cover an mcp listing that withholds an included tool

ConnectorCredentialRejectedError's TSDoc and its status member, and the
sys_connector_credential status description, named only a 401 / 403. The
mcp provider now also raises the marker (status 403) for a server that
accepted the credential and did not list a tool the instance's include
names, so each text names that case too. Comments only in spec.

Claude-Session: https://claude.ai/code/session_017xRBcVBKAFk1ntXSjvPt7U
Co-authored-by: Claude <noreply@anthropic.com>
…nnected and the server starting

The Credentials paragraph said a classic token whose scopes hide an
included tool stops the server from starting when set as the environment
variable. The mcp provider now degrades the instance on either path. It
also notes that GitHub's server keeps read tools that work on public
repositories, these two among them, listed whatever the scopes.

Claude-Session: https://claude.ai/code/session_017xRBcVBKAFk1ntXSjvPt7U
Co-authored-by: Claude <noreply@anthropic.com>
…s the declaration fault

Pins the boundary the plain branch's comment now states: only the instance's
auth takes the credential path; a header credential is part of the
declaration and is fixed by editing it.

Claude-Session: https://claude.ai/code/session_017xRBcVBKAFk1ntXSjvPt7U
Co-authored-by: Claude <noreply@anthropic.com>
…rom source in tests

The store-backed test imports the three; check:test-source-alias requires
an anchored source alias for each rather than a wider unaliased ledger. The
registry log is quieted through OS_REGISTRY_LOG.

Claude-Session: https://claude.ai/code/session_017xRBcVBKAFk1ntXSjvPt7U
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/l dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation tests tooling labels Oct 31, 2026
@github-actions

github-actions Bot commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 4 package(s): @objectstack/connector-app-github, @objectstack/connector-mcp, @objectstack/service-automation, @objectstack/spec, touching 4 documentable anchor(s). ⚠️ 1 changed file(s) yielded no anchor (packages/apps/connector-github/README.md), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/permissions/tenant-audit-census.mdx (via HERE (symbol, a top-level const object))
What this run could not see
  • 1 changed file(s) yielded no anchor (packages/apps/connector-github/README.md) — pages documenting those are invisible to this run
  • the SDK route bridge reached 60 of 212 client-bound route-ledger rows — the other 152 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 152: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 97 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.
  • a key NAME is not a key, so the hand re-read the line above prescribes can land on the wrong schema. The same spelling is authorable on one governed type and a [REMOVED] tombstone on another for each of active, aria, joins, objects, template, tools and version (censused on [finding] tools is a key on BOTH AgentSchema (tombstoned, dead) and SkillSchema (live, cloud-attested), so a name-based search attributes skill examples to the agent key — it produced a false stop-the-line alarm on PR #19059 #19093 over the liveness ledger's governed types, top-level keys); nothing in a search result distinguishes the two, so a grep hit on a LIVE example reads as evidence about the DEAD key. Measured on fix(spec): the agent.tools liveness row says dead — it claimed live on a key the schema tombstoned #19059: content/docs/ai/agents.mdx was reported as contradicting the agent.tools tombstone over its tools: example at :161, which is inside the defineSkill({ block opened at :155 — the page was already correct. Settle ownership by PARSING the value against both schemas, never by the name: that literal PASSES SkillSchema, and as an AgentSchema it FAILS at tools with the tombstone prescription. ⛔ These names are not the whole class — a key retired through a .strict() guidance map leaves no tombstone in the walked shape and none of them here (tool.category, live as AIToolDefinition.category).

Coarse fallback — 168 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json ea74ebf93f8b1b955b2b77327c3a34c677d4d751 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 887472c6ce223462181ccc8b9ece3c04830b5ac3 — the merge of head a91c84adc9210ce1edd29d87b42af40c96dbef70 into base ea74ebf93f8b1b955b2b77327c3a34c677d4d751, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 887472c6ce223462181ccc8b9ece3c04830b5ac3 && git checkout 887472c6ce223462181ccc8b9ece3c04830b5ac3
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin ea74ebf93f8b1b955b2b77327c3a34c677d4d751 a91c84adc9210ce1edd29d87b42af40c96dbef70 && git checkout -B drift-repro ea74ebf93f8b1b955b2b77327c3a34c677d4d751 && git merge --no-ff a91c84adc9210ce1edd29d87b42af40c96dbef70

node scripts/docs-audit/affected-docs.mjs --json ea74ebf93f8b1b955b2b77327c3a34c677d4d751

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs ea74ebf93f8b1b955b2b77327c3a34c677d4d751 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

epic:#27293#1 · sunhaolin · session_017xRBcVBKAFk1ntXSjvPt7U

Contract-review-handoff: triage — CONTRACT_REVIEW_TIER unavailable (HTTP 429 rate_limit, the tier's usage limit reached, 2026-10-31T06:14Z) · head b6312fe

Why a review at that tier is owed: the diff edits packages/spec/src/integration/connector-provider-errors.ts (published spec source; TSDoc of ConnectorCredentialRejectedError). The question the review must answer, from this seat's reading: the PR throws that published marker (status 403) for an upstream that accepted the credential and listed fewer tools, and widens its TSDoc to say so. Whether that is a truthful, closed widening or a marker used outside its declared meaning is the same class of question #27401 sent back to the maintainer. ⛔ Not landed until a same-shape record at CONTRACT_REVIEW_TIER is on the current head; needs:contract-review added in this act.

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

domain:server#1 · os-tesla · session_01L9qibw58ehQ2gRHP2h2Puz

Contract-review-claim: session_01L9qibw58ehQ2gRHP2h2Puz · head b6312fe6f3108fe588197fd5256ba041077789cf · 2026-10-31T06:44Z

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

epic:#27293#1 · os-tesla · session_01L9qibw58ehQ2gRHP2h2Puz

Contract review

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: b6312fe6f3108fe588197fd5256ba041077789cf
Local-runs: none
Scope: packages/connectors/connector-mcp/src/mcp-provider.ts (credentialSent, the include refusal's two branches, credentialVerified), packages/spec/src/integration/connector-provider-errors.ts (the TSDoc of CONNECTOR_CREDENTIAL_REJECTED, ConnectorCredentialRejectedError and its status; no type or value changes), packages/services/service-automation/src/sys-connector-credential.object.ts (the status description), content/docs/automation/connectors.mdx (the include paragraph, the refused-credential paragraph, two failure-table rows), packages/apps/connector-github/README.md (the Credentials paragraph), packages/connectors/connector-mcp/src/mcp-include-credential-scope.test.ts, packages/connectors/connector-mcp/vitest.config.ts, packages/connectors/connector-mcp/package.json (three devDependencies) + pnpm-lock.yaml, .changeset/28311-mcp-include-scope-hidden-tools.md; read on main for the marker's meaning and not reviewed: packages/services/service-automation/src/plugin.ts (isConnectorCredentialRejected at :2450, holdConnectorForRejectedCredential at :3166 to :3250)

Inputs: card #28311 (body, the dev note 6538998021, triage 6539260047 which routes the credentialed case to the credential path, the claim), PR #28357 (body, the docs-drift comment, the seat's hand-off 6539986174 naming the question this record must answer, the file list of 10 files, the diff of 569 lines against main read whole, the seven commits from the merge-base), #27401 (the precedent the hand-off cites: a marker borrowed for a case whose retry and log semantics it did not describe, sent back to the maintainer), the 71 check-runs on this head (read 2026-10-31T06:47Z: all 71 runs completed, none failed, none cancelled, none in progress; success: Build & E2E UI, Build Core, Build Docs, Build UI Deps, Check Changeset, Check Documentation Links, Dogfood Regression Gate, Flag docs affected by code changes, Governed Surface Queue Guard, Lint & Repo Gates, Lint UI, Lint · CI, release & engine gates, Lint · ESLint, Lint · PM dispatch-gates, Lint · docs & code guards, Lint · repo & tooling gates, Lint · scoped · query-options erasure, Lint · scoped · slot lookup & corpus, Lint · scoped · tooling self-tests, Live E2E UI, PR-body guards (claim · duplicate · Part-of · single-writer path), Plan Test Core shards, Spec property liveness, Test Core, Test Core (all 8), Test UI, Test UI (all 16), Type Check UI, Type Check · consumer gates, Type Check · debt ledger, Type Check · source gates, Type Check · workspace, TypeScript Type Check, Validate Package Dependencies, filter; skipped: 18.x upgrade harness (opt-in), Auto Label, Check PR Size, Dogfood Regression Gate (matrix shard/3), Dogfood Verify CLI, Packed-tarball smoke (opt-in), Schedule gate · skip when a green push run covers the commit, Temporal Conformance (live PG + MySQL), UI Published Artifacts), and the tree on main for the marker's declared contract and the materializer's handling of it. The head is mergeable and clean against today's main (merge-tree clean); main moved connectors.mdx and pnpm-lock.yaml since the merge-base and the merge is clean.

① Derived judgments

  • The hand-off's question: a truthful, closed widening of the marker, not the [Decision] connector-mcp tool-snapshot pin (#26709 item 3(c)): one aggregate toolsSnapshotSha256, or a per-tool pin record that can name the tool a server rewrote #27401 class - yes. The marker's declared meaning on main is the outcome, not the status: "neither an authoring fault nor an operational one; the upstream answered, and retrying the same value cannot change its answer", with the materializer degrading, holding a stored value as rejected until it is set again, the declaration changes or the process restarts, and retrying an environment or vault value on the backoff. A server that was sent the credential and lists fewer tools than include names is exactly that outcome: the answer was given to this credential, the same value gets the same answer, and the administrator who supplies the value is the one who can change it. The materializer's handling (holdConnectorForRejectedCredential, read on main) is the behaviour the card and triage 6539260047 prescribe for this case, so the marker is not borrowed for semantics it does not carry; in [Decision] connector-mcp tool-snapshot pin (#26709 item 3(c)): one aggregate toolsSnapshotSha256, or a per-tool pin record that can name the tool a server rewrote #27401 the borrowed code's backoff and "upstream unavailable" log described the case falsely, which is why it went back. The TSDoc is widened in words that name the one new raiser (an MCP server that lists only the tools a credential may use), so the contract reads true after the change; no type, value or code moves.
  • The predicate is the one the provider already used - right. credentialSent is the former credentialVerified expression (http transport and sendsInstanceAuth(ctx.auth)), so the credentialed branch fires only when the instance's auth reached the server as a header; an api-key as a query parameter, a stdio transport, no auth, or a credential written into transport.headers (part of the declaration) keep the plain fault with a byte-identical message. The five controls pin each.
  • Status 403, and the message - right. The server answered 2xx and refused nothing in words, and 403 is what a credential short of the scopes a resource needs is answered with (RFC 6750 section 3.1, insufficient_scope; GitHub's own server answers an out-of-scope tools/call that way). The message names the missing tools, the tools listed for this credential, and both causes with their fixes; it quotes no HTTP status and no credential (withoutCredentialThrown scrubs the thrown value in the materializer in any case). The dev measured that GitHub's tools/list answer carries no field or header about hidden tools, so "both causes" is the truthful message.
  • Downstream texts follow - right. The sys_connector_credential.status description names the second way a row becomes rejected; the connectors page's include paragraph, refused-credential paragraph and two failure-table rows state the split; the GitHub README no longer says an environment-variable token stops the server, and corrects itself on which of the app's own tools GitHub hides (none: search_issues and issue_read are public-read, which the dev read at f2a07cd1).
  • Tests and their closure - right. The new file runs the real provider, materializer and credential store (ObjectQL over SQLite), which is why connector-mcp gains three workspace devDependencies and a vitest.config.ts that aliases them to source (check:test-source-alias), disarms the console intercept and quiets the registry log through OS_REGISTRY_LOG (both repo gates). Nine cases: the credentialed refusal, five controls, the environment path through the materializer (degraded, listed rejected / env, recovers on a reconcile), the stored path (row rejected, held as credential, a better value recovers it), and the ablation of if (credentialSent) reds the three credential cases alone. The lock file carries the three links only.
  • Accept-set: nothing an author writes moves - right. Clause-②: no and Metadata-protocol: none are right.
  • Check-runs as gate verdicts - read as above; every run on this head completed and none is red, cancelled or in progress at the read.

② Semver level

③ Boundary flags

④ Design judgment

  • Nothing blocking, no should-fix on the diff. Tried: a server that lists every tool for an under-scoped token and refuses at tools/call (unchanged: the call-time 403 already takes the credential path through credentialRefusals); an include of zero missing tools (the branch is not reached); a server that hides tools from an api-key sent as a query parameter (not credentialSent, plain fault, the administrator's fix is the declaration or moving the key to a header, which the docs' split implies). One predicate reused, one existing marker with its existing handling, the plain fault kept for every case the credential did not reach: the smallest truthful shape.

Verdict reasoning. The marker's contract on main is defined by its outcome and handling, both of which this case satisfies, and the TSDoc now names the second raiser, so the widening is truthful and closed rather than a borrowed code; the split is keyed on the predicate the provider already trusted, the message carries both causes because the server cannot tell them apart, every path is pinned on the real stack with controls and an ablation, and every check is green or skipped on a head clean against main.

Implemented-by: claude/issue-28311-mcp-scope-hidden-tools
Reviewed-by: session_01L9qibw58ehQ2gRHP2h2Puz

VERDICT: PASS

…p-scope-hidden-tools

Two textual conflicts resolved. content/docs/automation/connectors.mdx:
main's failure-table rows kept (the vendorPublicKeyRef, signed-key and
alipay-v3 rows), with this branch's wording on the refused-credential row.
packages/connectors/connector-mcp/vitest.config.ts: both alias sets kept,
main's @objectstack/types beside this branch's driver-sql, objectql and
platform-objects, with the OS_REGISTRY_LOG declaration.

Claude-Session: https://claude.ai/code/session_017xRBcVBKAFk1ntXSjvPt7U
Co-authored-by: Claude <noreply@anthropic.com>
…he plain include fault, pinned

main's MCP authorization flow hands an oauth2 / app-credential instance a
credential handle, and its discovery listing is answered for an
organization's borrowed token. That token does reach the server, but the
credential path a refusal takes holds and marks the instance's
credentialRef row, which for such an instance is its client secret, and a
reconnect of the same organization does not re-attempt the held instance.
So credentialSent stays the static-auth predicate, the comment says why,
one test pins it, and the docs and changeset name the grant-bearing kinds
among the instances that keep the plain fault.

Claude-Session: https://claude.ai/code/session_017xRBcVBKAFk1ntXSjvPt7U
Co-authored-by: Claude <noreply@anthropic.com>
@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

epic:#27293#1 · sunhaolin · session_017xRBcVBKAFk1ntXSjvPt7U

Contract review

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: a91c84adc9210ce1edd29d87b42af40c96dbef70
Local-runs: none
Scope: packages/spec/src/integration/connector-provider-errors.ts (unchanged by the delta; re-read on the new head as the contract the decision is judged against), packages/connectors/connector-mcp/src/mcp-provider.ts (the merged credentialSent / credentialVerified next to main's ctx.credentialHandle, borrowingFetch, discoveryTransport and clientFor; the new comment), packages/connectors/connector-mcp/src/mcp-include-credential-scope.test.ts (the new oauth2-handle case), content/docs/automation/connectors.mdx (the conflict resolution and the static credential wording), packages/connectors/connector-mcp/vitest.config.ts (the conflict resolution), .changeset/28311-mcp-include-scope-hidden-tools.md (the Unchanged bullet) + packages/services/service-automation/src/plugin.ts on the new head, read and not reviewed (credentialRefOf :435, the connection signature :2476 to :2484, the held skip :2534, the credential and handle resolution :2592 to :2720, isConnectorCredentialRejected :2799, holdConnectorForRejectedCredential :3991 to :4068, scheduleDeclarativeRetry :4243), packages/types/src/connector-credential-source.ts (callWithBorrowedCredential, the 401 refresh-once rule)

Delta record. The previous record on this PR (comment 6540253498, PASS on b6312fe6) stands for the branch's change; this record judges only what moved the head since: merge commit 4b4a4ca8 (parents b6312fe6 and main at ea74ebf9; two textual conflicts, connectors.mdx and vitest.config.ts; mcp-provider.ts auto-merged against main's #28437 at 70646157) and commit a91c84ad (a comment on credentialSent, one test, the docs and changeset naming the grant-bearing kinds among the instances that keep the plain fault). Inputs: git diff b6312fe6 a91c84ad for the ten files of the PR, git diff ea74ebf9 a91c84ad (the branch's net change on top of the main it merged: 10 files, 568 insertions, 51 deletions, the same file list the PR carries), the combined diff of the merge (git show --cc 4b4a4ca8: hunks only in the two conflict files, none in mcp-provider.ts), the three versions of each conflict file, the main-side diff efdddc90..ea74ebf9 for mcp-provider.ts, the tree at a91c84ad for the cited code, and the 66 check-runs on this head (read 2026-11-01T03:54Z: all 66 completed, none failed, none cancelled, none in progress; success: Auto Label, Build & E2E UI, Build Core, Build Docs, Build UI Deps, Check Changeset, Check Documentation Links, Check PR Size, Dogfood Regression Gate, Flag docs affected by code changes, Governed Surface Queue Guard, Lint & Repo Gates, Lint UI, Lint · CI, release & engine gates, Lint · ESLint, Lint · PM dispatch-gates, Lint · docs & code guards, Lint · repo & tooling gates, Lint · scoped · query-options erasure, Lint · scoped · slot lookup & corpus, Lint · scoped · tooling self-tests, Live E2E UI, PR-body guards (claim · duplicate · Part-of · single-writer path), Plan Test Core shards, Spec property liveness, Test Core, Test Core (all 8 shards), Test UI, Test UI (all 16 shards), Type Check UI, Type Check · consumer gates, Type Check · debt ledger, Type Check · source gates, Type Check · workspace, TypeScript Type Check, Validate Package Dependencies, filter; skipped: 18.x upgrade harness (opt-in), Dogfood Regression Gate (matrix shard/3), Dogfood Verify CLI, Packed-tarball smoke (opt-in), Schedule gate · skip when a green push run covers the commit, Temporal Conformance (live PG + MySQL), UI Published Artifacts). The PR reads mergeable: true, mergeable_state: clean, head repo = base repo, 10 changed files, draft. No PR comment follows 6540253498.

① Derived judgments

  • (1) credentialSent next to main's credential-handle path - holds. On the new head the materializer hands a grant-bearing instance credentialHandle and leaves auth undefined (plugin.ts :2700 to :2720, :2755: auth is set only in the final else), and the provider itself refuses a handle beside a static credential or a signer as a plain configuration fault. So for such an instance sendsInstanceAuth(ctx.auth) is false, credentialSent is false, the include miss stays the plain fault, and credentialVerified = credentialSent stays false, which matches the materializer's own gate on recordCredentialProven (credentialHandle === undefined). The borrowed token travels through discoveryTransport.fetch (borrowingFetch), never through transport.headers, so the predicate cannot be tripped by it. The new test pins exactly this: ctx({ credentialHandle: handle }), transport carrying a fetch and no Authorization header, the fake server answering its anonymous list, the plain message, the client closed. Order of evaluation after the merge is as the branch intended: handle checks, discovery, the catch (401 / 403 by SDK status, else upstream-unavailable), then credentialSent, the include check, credentialVerified, and main's close wrapper (borrowed clients first) is kept.
  • The comment's four factual claims, read in plugin.ts - true. (a) The path a ConnectorCredentialRejectedError takes at materialization is holdConnectorForRejectedCredential with ref and credential resolved from credentialRefOf(entry.auth) (:2592 to :2612), which for oauth2 / app-credential is the client or app secret (createGrantHandle(..., credential?.value), appSecret: credential?.value). (b) A stored one is marked rejected (markRejected(stored, reason), :4049) and the instance held. (c) A held instance is skipped while its signature is unchanged (:2534); the signature of an mcp instance with connections carries only the discovery organization id (|connections:connected:discovery:${state.discovery}, :2483), so a reconnect of the same organization, a wider token included, leaves it held: "neither marks nor re-attempts on a reconnect" is right. (d) The token the server was sent is the connection's, which the hold path never touches. Routing the handle case to the marker would therefore have produced a false rejected on the client secret and a hold no reconnect lifts; keeping the plain fault is the truthful choice until the materializer has a connection-keyed route.
  • (2) Consistency with the published TSDoc on the new head - consistent by the contract's own terms. CONNECTOR_CREDENTIAL_REJECTED defines the marker by its outcome: "the instance is well-formed and its credentialRef resolved", the materializer degrades, "a stored value's row is marked rejected", retried on the backoff only when the value is an environment or vault one. For a handle instance the credentialRef value is not what the listing depended on, so that outcome would be false for it; excluding it keeps every sentence of the paragraph true for every raiser that remains. ConnectorCredentialRejectedError's own TSDoc ("refused the credential it sent, or accepted it and withheld what the instance declares") and the status doc (403 for a 2xx that withheld) remain true of what is raised. The one unqualified clause is the paragraph's last sentence, "an MCP server that lists only the tools a credential may use, and so leaves out a tool the instance's include names, is answered with this marker by the mcp provider": read alone it covers a borrowed token too. The factory TSDoc in mcp-provider.ts ("when the handshake carried no instance credential ... and as ConnectorCredentialRejectedError when it did"), the new comment, the connectors page ("sends a static auth credential (bearer, basic, or an api-key header)") and the changeset all carry the qualification; the spec sentence does not. A should-fix on wording, below, not a contract break: no type, value or code moved in the delta, and the marker's defined outcome is the reason for the exclusion.
  • (3) The conflict resolutions - nothing dropped, nothing misstated. connectors.mdx: the net change on top of main (ea74ebf9..4b4a4ca8) is the branch's three hunks byte-for-byte (the include paragraph, the refused-credential paragraph, the two failure-table rows) placed among main's new rows (vendorPublicKeyRef, the signed key, the two alipay-v3 rows, the grant-bearing row reworded by main to the credentialHandle: true registration, the authorization_code and token-endpoint rows); main's wording of every row it changed is kept, the branch's wording of the refused-credential row is kept, the merge message says exactly this. vitest.config.ts: the merged file keeps main's @objectstack/types alias (re-anchored on the same HERE the branch's three use; same target), the branch's driver-sql, objectql and platform-objects aliases, disableConsoleIntercept: true and env: { OS_REGISTRY_LOG: 'warn' }; the two header comments are folded into one that names all four entries and both reasons; main's "and nothing else" claim is rightly gone. mcp-provider.ts: the combined diff has no hunk, i.e. every merged hunk is one parent's; the net diff against ea74ebf9 is the branch's change and only that (the factory TSDoc paragraph, the credentialSent block, the two-branch refusal, credentialVerified = credentialSent), on top of main's imports, borrowingFetch, handle checks, discoveryTransport, clientFor and close.
  • The delta's prose - right. The changeset's Unchanged bullet and the connectors page's include paragraph now list the grant-bearing kinds among the plain-fault cases and name the static kinds that take the credential path, which is what sendsInstanceAuth says; the two table rows say "a static credential". "Fails the boot" for the plain fault is the fatal-mode fail (:2390), as before for the other plain cases; on a reload or retry it is a skipped entry or failDegradedConnectorInstance, as the retry row already states.
  • Accept-set, semver - unchanged: Clause-②: no, Metadata-protocol: none, two patches; the delta adds no entry and the lock file is untouched by it.
  • Check-runs as gate verdicts - read as above; every run on this head completed and none is red, cancelled or in progress at the read.

② Semver level

  • @objectstack/connector-mcp patch, @objectstack/service-automation patch - accepted, as on b6312fe6; the delta changes a comment, a test and prose.

③ Boundary flags

④ Design judgment

  • The decision holds against the code it cites. The alternative, counting the handle as "credential sent", was tried on the live materializer: it would raise the marker for a token the hold path cannot mark or re-attempt, so the degraded reason would prescribe "set the credential again" for a client secret that was never the cause, and the only exit would be an edit of the declaration or a restart. Keeping the plain fault costs the grant-bearing instance a boot failure (fatal mode) where a static-credential instance degrades; the message still names the missing tools and the listed ones, and the docs now say which kinds take which path. One predicate, unchanged in meaning by the merge, a comment that states the reason in terms a later materializer change can retire, one test that pins the predicate on the shape main actually hands the provider: the smallest truthful delta. The two conflict resolutions are faithful to both sides.

Verdict reasoning. The merge changed nothing the previous record judged: the predicate the provider keys on is the one it already used, and main's handle path cannot reach it; the decision to keep a grant-bearing instance on the plain fault is the one the marker's own TSDoc outcome demands, pinned by a test on the real shape; both conflict resolutions are both sides whole; every check on the new head is green or skipped and the head is clean against main. One wording should-fix in the spec TSDoc and one pre-existing gap on main to file are named above; neither is a defect this delta introduces.

Implemented-by: claude/issue-28311-mcp-scope-hidden-tools
Reviewed-by: session_017xRBcVBKAFk1ntXSjvPt7U

VERDICT: PASS

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

epic:#27293#1 · sunhaolin · session_017xRBcVBKAFk1ntXSjvPt7U

席位核对

ACCEPT at head a91c84ad · Implemented-by: claude/issue-28311-mcp-scope-hidden-tools · Reviewed-by: this seat (lane ACCEPT at TIER_DEFAULT) + CONTRACT_REVIEW_TIER records: full PASS 6540253498 on b6312fe6 (another seat, after the hand-off 6539986174) and delta PASS 6553099315 on this head. needs:contract-review is off.

Check Result
Shape draft, base main, first line Fixes #28311; the only closing keyword
Scope 10 files, +568/−51 against the merge base; not governed; changesets for @objectstack/connector-mcp and @objectstack/service-automation (patch); surface addendum: packages/spec/src/integration/connector-provider-errors.ts (TSDoc only), service-automation/src/sys-connector-credential.object.ts (one description), connector-mcp/package.json + vitest.config.ts
Gates 66 check-runs on the head: 59 success, 7 skipped, none red; mergeable clean
Evidence 10 provider cases including env and store paths through the real materializer and credential store; ablation of the credential branch turns exactly the 3 static-credential cases red
Accepted deviation a grant-bearing instance (oauth2, app-credential) keeps the plain include fault — main's behaviour since #28437, unchanged here. Taking it onto the credential path would mark the client-secret row rejected or hold the instance behind a remedy a reconnect does not re-attempt; no server is known to filter tools/list by an OAuth token's scopes (GitHub's lists every tool to OAuth tokens, measured at f2a07cd1). The seat's call: keep it until a producer is named
Stale PR-body figures (the dev writes the body once) read as: head a91c84ad, 619 changed lines, connector-mcp 7 files / 74 tests, service-automation 247 files / 3,152 tests, the 2-minute check Tests 10 passed (10)

Contract-review should-fix (wording only, non-blocking per 6553099315): the CONNECTOR_CREDENTIAL_REJECTED TSDoc's last sentence gains the qualifier "the tools the instance's auth credential may use" — carried by #27401, which edits the same file next (rider sent to its dev).

Landing: not governed: the seat lands it now (pr_ready + automerge_enable).

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

Labels

dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

1 participant