Repository navigation
feat: add BC native MCP passthrough - #11
Conversation
SShadowS
left a comment
There was a problem hiding this comment.
Verified locally on pr-11: bun run typecheck clean, bun test 619 pass / 0 fail across 43 files. Runtime deps unchanged (signalr, mcp sdk, zod), so the three dep rule holds.
Not verified: the SaaS Sandbox run. No SaaS credentials on this machine, so scripts/e2e-native-mcp-passthrough-2026-07-27.md is the only support for the new wire facts (Dev: ALRuntime, the mcp-troubleshooting-options shape, versionNumber: "1.0"). Those need a second pair of eyes from whoever has Sandbox access.
Strengths
The executionRevision work is the best part of this PR. Break arriving before sessionBound was a real ordering bug caught live, and rollbackResume correctly declines to restore state that a newer callback already owns. Both orderings have deterministic tests.
Other things done right:
nonblankHeaderrejects CRLF on every header value at the core boundary, with zod regex on top at the tool boundary. Header injection closed twice.requestHeadersre-checksdebugIdentityeven thoughtargetFromalready guarantees it. Core does not trust its caller.- Loose zod schemas with the comment explaining that the SDK convenience helpers normalize unknown fields away. Correct call for a passthrough, and the reasoning is written down rather than assumed.
- Endpoint, authorization, and routing headers are not caller controlled.
- Conventions followed throughout: claim then
finallylock matchingtest-tools.ts:111,// WIRE:provenance on every assumption, e2e checklist plus dated evidence, embedded skill with drift test, README table and count and roadmap all updated.
1. Runtime timeout releases the test-run lock without cancelling the upstream run
bcdev_native_call with context: "runtime" claims the singleton test-run lock, then releases it unconditionally in finally. On the whole-operation timeout (default 180s, hard max 300s) the local lock drops while Business Central's run_tests is still executing server side. A following bcdev_test_run is then free to start against a live run, which is the exact contention the lock exists to prevent. bcdev_test_run has no timeout of its own, it awaits hub completion, so this hole is new.
It gets worse from the error surface: TIMEOUT carries retryable: true and recoverySteps says "Retry the operation". An agent following that guidance starts a second test run on top of the first.
Suggested fix, keeping the release in finally and making the consequence explicit in the payload:
handler: async (params) => {
const context = params["context"] as NativeMcpContext;
const runLocked = context === "runtime";
if (runLocked) claimTestRun(state);
try {
const target = targetFrom(state, deps, params);
const toolName = params["toolName"] as string;
const response = await deps.nativeMcpGateway.callTool(
target,
toolName,
(params["arguments"] as Record<string, unknown> | undefined) ?? {},
);
return { context: target.context, toolName, ...response };
} catch (error) {
// A timeout stops the bridge waiting; it does not ask Business Central to stop
// the native run. The test-run slot is released either way, so the caller has to
// be told the server may still be executing before it retries or starts a run.
if (runLocked && error instanceof BcDevError && error.code === "TIMEOUT") {
throw new BcDevError(
error.code,
error.message,
error.category,
error.retryable,
{
...error.details,
upstreamRunCancelled: false,
warning:
"The native run was not cancelled and may still be executing on the server. "
+ "Confirm it finished before retrying or starting another test run.",
},
{ cause: error },
);
}
throw error;
} finally {
if (runLocked) state.testRunActive = false;
}
},details is typed Record<string, string | number | boolean | null>, so both fields fit without a type change. Worth a test asserting upstreamRunCancelled: false reaches the agent body, alongside the existing "releases the native runtime lock when setup or invocation fails" case.
Separately, consider whether 300s is the right ceiling for runtime at all. A real suite can exceed five minutes, so the cap turns normal runs into this failure mode rather than an edge case.
2. Bare JWT is outside the redactor (hardening, no demonstrated leak)
redactAuthorization matches URL userinfo, ?Authentication=, and a literal Authorization prefix. It does not match a bare token:
in: Bearer eyJ0eXAi....SIGNATUREPART
out: Bearer eyJ0eXAi....SIGNATUREPART (unchanged)
That matters here because this is the first path that sends Authorization: Bearer <Entra JWT> to an external gateway and echoes that gateway's body back: SDK streamableHttp.js:364 builds Error POSTing to endpoint: ${text} from the raw response body, which reaches the agent via nativeErrorDetail into details.upstreamMessage.
To be clear about scope, I did not show that Business Central or the Entra front end ever echoes a token in a response body, and normally it will not. Our own outgoing headers are not in the message. So this is defense in depth on an unproven path, not a known exposure. One line in redactAuthorization closes it:
.replace(/\beyJ[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]*/g, "[REDACTED_JWT]")3. serverIdentity() runs before the operation (low)
return await this.use(target, async (connection, options) => ({
server: serverIdentity(connection),
catalog: await connection.listTools(cursor, options),
}));Property evaluation order means a server that omits serverInfo.name or version fails every list and call with PROTOCOL_ERROR, even though the request itself would have succeeded. serverInfo is spec required so this is unlikely, but the strictness buys nothing since identity is decoration. Degrading to null is strictly better than throwing.
4. Session per operation is undocumented (low)
Every bcdev_native_list and bcdev_native_call builds a fresh Client and transport: initialize, request, DELETE, three to four round trips, no reuse. Fine and deliberately stateless, but for the debugging context that is three-plus round trips per inspection against one hub invocation for bcdev_debug_variables. Worth a line in skills/bc-native-mcp/SKILL.md so agents do not reach for native calls where the first class tools are cheaper.
5. Debug identity is point in time (low)
nativeDebugIdentity is read once in targetFrom, then the call can run for minutes. A concurrent bcdev_debug_continue resumes the session mid flight and the native troubleshooting call proceeds against a resumed session. Needs a client that issues concurrent tool calls, which most do not do within a turn, so this is a documentation note rather than a code change.
6. Loose error message matching (cosmetic)
lower.includes("debug session") && lower.includes("identity") in fromKnownMessage catches any stray Error containing both words. Only reachable for non BcDevError throws and it matches existing house style. Leave it.
Verdict
Approve. Item 1 is the only one I would want resolved or consciously accepted before merge, since it converts a correctness guarantee into a silent one under a condition that real test suites will hit.
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
|
Addressed all six review notes in
I also reran the real handlers against the SaaS Sandbox after these corrections. Business search, native runtime test execution, and paused-debug Final local gates: 622 tests passed, typecheck clean, build clean, embedded-skill generation idempotent, and diff check clean. The synthetic JWT redaction vector is assembled at runtime to avoid a scanner false positive; both the repository verification job and GitGuardian now pass. |
411d28d to
89c4c20
Compare
Summary
This PR ships roadmap item 8, BC native MCP passthrough, for the verified Business Central 28 cloud surface.
bcdev_native_listto discover the dynamic native catalog for business actions, AL runtime, or an active paused debugger.bcdev_native_callto invoke an exact discovered tool while preserving the complete upstreamCallToolResult, including content blocks, structured content, metadata, unknown fields, andisError.Compatibility and safety
bcdev_native_callis conservatively marked destructive and non-idempotent because safety depends on the selected upstream tool.business,runtime, anddebugging. BC29/native profiling features are not exposed.server: nullwithout discarding a successful list or call result.upstreamRunCancelled: false, and warns that the server-side run may still be executing.Validation
bun test: 622 passed, 0 failedbun run typecheck: passedbun run build: passedDeterministic coverage includes the installed Streamable HTTP SDK flow for both
tools/listandtools/call, pagination, unknown-field preservation, upstreamisError, phase-aware timeouts, runtime timeout recovery guidance, bare-JWT redaction, nullable server identity, DELETE cleanup, debugger callback ordering, paused-session identity gates, and native/direct/debug test-run contention.SaaS Sandbox validation
Validated against a Business Central 28 SaaS Sandbox:
bc_actions_searchcallrun_testsagainst a disposable published fixtureget_stack_frames,get_variables,get_source_code, andadd_breakpointNo Production, on-premises, or native profiling call was made. Redacted evidence is recorded in:
scripts/e2e-native-mcp-passthrough-2026-07-27.mdscripts/e2e-native-mcp-passthrough-2026-07-28.md