From 7d20929534b89ee7b5413130345dd33803ba9522 Mon Sep 17 00:00:00 2001 From: ianray <193674599+ianrayianray@users.noreply.github.com> Date: Sun, 26 Jul 2026 08:12:49 -0700 Subject: [PATCH 1/3] feat: add record-write triage --- README.md | 34 +- ...break-on-record-write-triage-2026-07-25.md | 40 ++ scripts/e2e.md | 20 +- skills/bc-al-debugging/SKILL.md | 27 +- src/core/agent-errors.ts | 2 + src/core/hubs/debugger-hub.ts | 47 +- src/core/record-write-triage.ts | 618 ++++++++++++++++++ src/core/types.ts | 4 + src/mcp/agent-errors.ts | 4 + src/mcp/skills.generated.ts | 6 +- src/mcp/state.ts | 38 ++ src/mcp/tools/agent-response.ts | 22 + src/mcp/tools/debug-tools.ts | 50 +- src/mcp/tools/index.ts | 2 + src/mcp/tools/record-write-tools.ts | 264 ++++++++ src/mcp/tools/shared.ts | 43 ++ tests/core/debugger-hub.test.ts | 60 +- tests/core/record-write-triage.test.ts | 485 ++++++++++++++ tests/mcp/agent-response.test.ts | 26 + tests/mcp/record-write-tools.test.ts | 289 ++++++++ tests/mcp/server.test.ts | 8 +- tests/mcp/state.test.ts | 21 +- tests/mcp/tools.test.ts | 5 +- 23 files changed, 2058 insertions(+), 57 deletions(-) create mode 100644 scripts/e2e-break-on-record-write-triage-2026-07-25.md create mode 100644 src/core/record-write-triage.ts create mode 100644 src/mcp/tools/record-write-tools.ts create mode 100644 tests/core/record-write-triage.test.ts create mode 100644 tests/mcp/record-write-tools.test.ts diff --git a/README.md b/README.md index c05951c..0f18595 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ MCP server for Business Central AL development: run tests (with code coverage) a [![Bun](https://img.shields.io/badge/bun-1.x-black)](https://bun.sh) [![TypeScript](https://img.shields.io/badge/typescript-strict-blue)](https://typescriptlang.org) [![BC dev API](https://img.shields.io/badge/BC%20dev%20API-%E2%89%A57.0-purple)]() -[![Tests](https://img.shields.io/badge/tests-534%20passing-green)]() +[![Tests](https://img.shields.io/badge/tests-585%20passing-green)]() ## Overview @@ -28,6 +28,7 @@ MCP server for Business Central AL development: run tests (with code coverage) a | **Coverage gap analysis** | Cross a Git diff with exact procedure coverage to report which changed procedures the selected tests did and did not exercise | | **Multi-codeunit plans** | Sequential codeunit groups over one hub connection | | **Interactive debugging** | Next-session, user-filtered, or exact-session attach; file/line breakpoints, break on all or unhandled errors only, record-write breaks (optionally skipping temp records), stack + variables + watch, stepping, abort/release at a break | +| **Record-write triage** | Arm a bounded capture for one numeric table ID, automatically continue every global record-write stop, and group exact matching writer stacks while failing closed on unresolved evidence | | **Debug-a-test** | Test run bound to the debug session — breakpoints fire during test execution | | **Config auto-discovery** | Server/instance/tenant read from the AL project's `.vscode/launch.json` | | **Preflight diagnostics** | `bcdev_status` distinguishes unreachable / bad credentials / unsupported dev API | @@ -73,6 +74,7 @@ Running from source instead: `git clone` → `bun install && bun run build` → 2. `bcdev_test_discover` — list test codeunits and `[Test]` methods from local `.al` files. 3. `bcdev_test_run { codeunits: [{ id: 50100 }], coverage: "procedure" }` — structured results. Add `coverageAgainst: "origin/main"` for changed-procedure analysis. After publishing the current changed objects to the target, also pass `changesDeployed: true` to permit `covered`/`uncovered` classifications; without that explicit assertion, changed procedures remain `unknown`. 4. `bcdev_debug_attach { breakOnError: true }` → trigger the workload → `bcdev_debug_wait` for `sessionBound` and breaks → inspect with `bcdev_debug_variables` / `bcdev_debug_eval` → `bcdev_debug_continue` → `bcdev_debug_detach`. For a debug-bound test run, trigger with `bcdev_debug_run_tests { codeunits: [{ id: 50100 }] }`. +5. To find writers of one table without stepping through every stop: `bcdev_record_writes_start { tableId: 18 }` → trigger the matching workload → check `bcdev_record_writes_status` → call `bcdev_record_writes_finish` for grouped stacks. Debugger attach returns as soon as Business Central accepts the request; binding is asynchronous. With no selector it binds the next session of the `breakOnNext` client type. Pass `userId` to filter that next session by Business Central user, or pass a known positive `sessionId` to attach to an existing NST session. `sessionId` and `userId` are mutually exclusive; exact `sessionId` targeting takes precedence over `breakOnNext`. `bcdev_debug_wait` reports `{ kind: "sessionBound", sessionId, hostId }` after binding; `hostId` may be null when Business Central omits that optional field. If identity lookup fails, it reports a nonfatal warning-form `sessionBound` event and debugging remains active. If Business Central emits a fatal user-filter rejection before `sessionBound`, the debugger tears down without binding or delivering breaks and reports an actionable `fatal` event. @@ -113,7 +115,7 @@ Azure access tokens are acquired with `az account get-access-token`, cached only MCP client (agent) | v (stdio) -src/mcp/server.ts ── tools/ (17 bcdev_* tools) ── state.ts (session, event queue) +src/mcp/server.ts ── tools/ (20 bcdev_* tools) ── state.ts (debug/test/profile ownership) | v src/core/ (pure library — typed returns, injected deps) @@ -141,12 +143,35 @@ src/core/ (pure library — typed returns, injected deps) | `bcdev_debug_sql` | Live SQL cost at a break: latency, executes, last statements | | `bcdev_debug_breakpoints` | Add/remove breakpoints and report the server's verified or relocated source span | | `bcdev_debug_detach` | End the debug session | +| `bcdev_record_writes_start` | Arm bounded background write triage for one positive numeric table ID | +| `bcdev_record_writes_status` | Read current write-triage lifecycle and classification counts without driving collection | +| `bcdev_record_writes_finish` | Release/stop collection and return grouped exact writer stacks plus unresolved evidence | | `bcdev_source` | Read the server’s deployed AL source for an object not on local disk | | `bcdev_profile_status` | Preflight the snapshot-debugger endpoint; report whether sampling CPU profiling is supported | | `bcdev_profile_start` | Arm a CPU profiler (`kind: "sampling"` or `"instrumentation"`); binds the next matching session (trigger it after) | | `bcdev_profile_poll` | Poll the active capture; `ready` once the session was recorded (Started) | | `bcdev_profile_finish` | Finish, write the `.alcpuprofile`, return a ranked AL self-time hotspot summary | +## Record-write triage + +Business Central's debugger enables record-write breaks globally; it does not accept a table filter. +`bcdev_record_writes_start` therefore arms one debugger session, inspects each paused +`Insert`/`Modify`/`ModifyAll`/`Rename`/`Delete`/`DeleteAll` receiver, compares its runtime numeric +table ID with `tableId`, groups exact matches by operation, receiver, and full stack, and +automatically continues. Unrelated writes are counted but not retained as writer groups. + +Use `sessionId`, `userId`, or `breakOnNext` as with manual debugger attach. Temporary-record writes +are excluded by default; pass `includeTemporary: true` to include them. Deployed source is +authoritative. Local source can establish an exact match only when the caller explicitly asserts +that it is deployed with `changesDeployed: true`; the tool does not verify that assertion. +`maxObservedWrites` bounds all observed breaks and defaults to 500. Reaching it releases the +workload and returns a truncated, incomplete report. + +The final `complete` flag means every record-write break observed in this one attached-session +capture window was classified. It is not proof that no other session or operation writes the +table. Any missing source/span, unsupported receiver, watch failure, unknown runtime type, +unexpected break, lifecycle failure, or truncation keeps the report fail-closed. + ## Profiling `bcdev_profile_*` **captures** a CPU profile; it does not itself drive BC. The division of labour is @@ -260,7 +285,7 @@ seconds, preventing a stale or hung preflight from holding that slot indefinitel |------|---------| | `src/mcp/index.ts` | stdio entry: builds deps, calls buildServer, connects transport | | `src/mcp/server.ts` | buildServer: registerTool/registerResource wiring (testable over InMemoryTransport) | -| `src/mcp/tools/` | The 17 bcdev_* tool definitions (zod schemas + metadata + handlers) | +| `src/mcp/tools/` | The 20 bcdev_* tool definitions (zod schemas + metadata + handlers) | | `src/mcp/state.ts` | Debug session singleton, event queue, run lock | | `src/core/hubs/test-runner-hub.ts` | TestRunnerHub client (Initialize/RunTests, coverage) | | `src/core/hubs/debugger-hub.ts` | DebuggerHub client (attach, breakpoints, stepping, inspection) | @@ -269,6 +294,7 @@ seconds, preventing a stale or hung preflight from holding that slot indefinitel | `src/core/git-changes.ts` | Merge-base-to-working-tree AL change ranges, including untracked files | | `src/core/al-procedures.ts` | Executable procedure/trigger spans and compiler-compatible procedure identities | | `src/core/coverage-gaps.ts` | Exact join between changed procedures and TestRunnerHub coverage evidence | +| `src/core/record-write-triage.ts` | Source-aware runtime table classification and bounded writer-stack collector | | `scripts/e2e.md` | Real-server wire-assumption checklist + known server behaviours | ## Roadmap @@ -281,7 +307,7 @@ Ordered by intent, not commitment: 4. **Entra ID auth** — **shipped.** Azure CLI-backed cloud sandbox/production access alongside explicit on-prem UserPassword. 5. **Coverage gap analysis** — **shipped.** Pass `coverageAgainst` to `bcdev_test_run` to cross the merge-base-to-working-tree Git diff with exact procedure coverage and identify covered, uncovered, or unresolved changed procedures. 6. **Test orchestration** — repeat runs, diff pass/fail sets, flag flaky tests. -7. **Break-on-record-write triage** — arm write breaks on a table and auto-collect the stacks of everything that writes it. +7. **Break-on-record-write triage** — **shipped.** Arm bounded write triage for one numeric table ID and automatically collect grouped exact writer stacks. 8. **BC native MCP passthrough** — a general-purpose front door to Business Central's own MCP endpoint. BC exposes runtime-troubleshooting and server-side profiling tools there that its shipped tooling only partially surfaces (one slice isn't exposed by any Microsoft client at all); a dynamic passthrough would make the full catalog reachable from any agent. 9. **On-demand source & symbols** — fetch a server object's source, or download a single dependency package, without leaving the agent. diff --git a/scripts/e2e-break-on-record-write-triage-2026-07-25.md b/scripts/e2e-break-on-record-write-triage-2026-07-25.md new file mode 100644 index 0000000..911077d --- /dev/null +++ b/scripts/e2e-break-on-record-write-triage-2026-07-25.md @@ -0,0 +1,40 @@ +# Break-on-record-write triage — SaaS evidence + +Validated on 2026-07-26 against a disposable Business Central SaaS Sandbox app. The +fixture was compiled locally and published with `~/bin/bc-al-publish.sh`. The harness +refused any non-Sandbox configuration. No Production or on-premises call was made. + +All tenant, environment, company, user, host, session, connection, token, authorization, +and authenticated-URL values were omitted. No record field value or raw source body was +retained. + +## Capture path + +The triage tool armed a Background-session attachment and returned with phase `arming`. +A disposable API page then started the matching Background workload. Collection bound, +classified each record-write stop, and continued the workload automatically; the +harness made no manual debugger wait or continue call. + +## Results + +- The default capture completed with 13 observed writes: 10 exact target writes in 9 + grouped writers, 3 proven unrelated writes, and 0 unresolved writes. +- The exact groups covered `Insert`, `Modify`, `ModifyAll`, `Rename`, `Delete`, and + `DeleteAll`, including repeated calls from one stack, a second caller stack, and a + runtime `RecordRef` receiver. +- Every exact writer used deployed source and retained a usable full statement span. + The report was complete for its attached-session capture window. +- Repeating the capture with temporary writes enabled added exactly one matched target + write relative to the default non-temporary capture. +- Forced local-source fallback without `changesDeployed` produced 0 exact matches and + incomplete evidence. After the already-published fixture was explicitly asserted + with `changesDeployed: true`, the same narrow workload produced 3 exact + `localAsserted` matches. +- With `maxObservedWrites: 1`, the cap event was classified, the workload was released, + and the report returned `truncated: true`, `complete: false`, with exactly one + observed write. +- Finish cleared the shared debugger owner. An immediate manual attach/detach cycle + succeeded, confirming slot reuse. + +The fixture and harness lived outside the repository and are not part of the feature +diff. diff --git a/scripts/e2e.md b/scripts/e2e.md index ea55521..5c60500 100644 --- a/scripts/e2e.md +++ b/scripts/e2e.md @@ -17,12 +17,14 @@ with its working directory set to the AL project. - [x] `AddBreakpoint({ObjectType,ObjectNumber},{Line,Column},condition)` returns a `BreakpointId`. - [x] `BreakpointDefinition.SourceSpan` uses 0-based debugger coordinates that become 1-based tool coordinates; an all-zero value-type span means unset, not line 1. `RelativeSourceSpan` is editor-relative metadata and is not reported. - [x] Break event fires when a bcdev_debug_run_tests run hits the breakpoint; stack frames carry `StatementSpan.From.Line`. +- [x] Break stack frames carry the complete zero-based `StatementSpan.From/To` line and column positions needed to isolate a multiline write statement; the tool exposes a nondegenerate span as 1-based coordinates and retains the legacy `line`. - [x] `SetBreakpointResponse` 0-3 continue/step correctly. - [x] `GetVariables(0)` returns LocalNode array; note casing. - [ ] `LocalNode.ChangeState` is the integer enum 0/1/2/3 (`unchanged`/`new`/`valueChanged`/`descendantChanged`); observe at least one nonzero value live. - [ ] IsAlive arrives during a long break; session survives (auto-ack works). - [x] bcdev_debug_eval: GetWatchNode(frameId, expression, watchOption 0) returns a LocalNode; note casing. - [x] Observe BreakOnRecordWriteBehaviour semantics for breakOnRecordWrite true/false. +- [x] With record-write breaking active, the paused AL statement is one of `Insert`, `Modify`, `ModifyAll`, `Rename`, `Delete`, or `DeleteAll`, and evaluating its exact receiver with `GetWatchNode(0, receiver, AllowLargeStrings)` returns `TypeName: "Table ()"` for both Record and runtime RecordRef values. ### SaaS / Entra wire assumptions @@ -41,7 +43,23 @@ Never record access tokens, Authorization headers, authenticated URLs, or unreda - [x] bcdev_test_run with two codeunits: results for both, sequential execution. - [x] bcdev_debug_attach + bcdev_debug_run_tests + bcdev_debug_wait → break → bcdev_debug_variables → bcdev_debug_continue → testRunFinished. - [x] bcdev_debug_detach mid-break: BC session released (check server). -- [ ] tools/list shows title/annotations/outputSchema for all 15 tools; resources/list shows the three skill:// resources. +- [ ] tools/list shows title/annotations/outputSchema for all 20 tools; resources/list shows the three skill:// resources. + +## Break-on-record-write triage (SaaS Sandbox) + +Run only against a disposable app and Business Central Sandbox. Never record tenant, environment, +company, user, host, session, connection, token, authorization header, authenticated URL, record +field values, or raw source. Evidence belongs in +`scripts/e2e-break-on-record-write-triage-2026-07-25.md`. + +- [x] `bcdev_record_writes_start` returns while arming, then the matching workload binds and status moves to `collecting` without manual debugger wait/continue calls. +- [x] Default `includeTemporary:false` excludes a temporary target write while retaining real target writes; `includeTemporary:true` observes the temporary write. +- [x] One workload produces repeated exact target writes from one stack, an exact target write from a second stack, and an unrelated-table write; final counts and groups match. +- [x] Every exact group uses deployed source plus a runtime receiver `tableId` equal to the requested numeric ID; unrelated and unresolved writes never enter `writers`. +- [x] The unasserted local-source fallback cannot produce an exact group; `changesDeployed:true` permits it only after the exact fixture has been published. +- [x] `maxObservedWrites` classifies the cap event, sends `release`, and returns `truncated:true`, `complete:false` without leaving the workload paused. +- [x] Finish clears the triage owner and an immediate manual attach or second triage start can claim the debugger slot. +- [x] No Production or on-premises call is made during this feature's acceptance run. ## Targeted debugger attach (Sandbox) diff --git a/skills/bc-al-debugging/SKILL.md b/skills/bc-al-debugging/SKILL.md index cc3c442..ac1a707 100644 --- a/skills/bc-al-debugging/SKILL.md +++ b/skills/bc-al-debugging/SKILL.md @@ -1,6 +1,6 @@ --- name: bc-al-debugging -description: Interactively debug AL code on a Business Central server with the bc-dev-mcp tools — attach, breakpoints, break-on-error, step, inspect variables and watches during a test run. Use when asked to debug AL code, find why an AL test fails, or inspect live state on a BC server. +description: Interactively debug AL code on a Business Central server with the bc-dev-mcp tools — attach, breakpoints, break-on-error, step, inspect variables and watches, or collect the stacks that write a table. Use when asked to debug AL code, find why an AL test fails, identify table writers, or inspect live state on a BC server. --- # Debugging AL on Business Central @@ -27,6 +27,31 @@ description: Interactively debug AL code on a Business Central server with the b then `bcdev_debug_wait` again. 6. `bcdev_debug_detach` when done. +## Find everything that writes one table + +Use the dedicated background workflow instead of manual `bcdev_debug_*` stepping: + +1. `bcdev_record_writes_start { tableId: }`. + Add exactly one of `sessionId` or `userId` when needed, or choose `breakOnNext`. + The tool returns after attach is armed; it does not wait for the session or workload. +2. Trigger the matching session and operation. Business Central breaks on every record write + globally; the collector identifies the runtime receiver, counts unrelated writes, groups exact + target-table stacks, and automatically continues. Do not call `bcdev_debug_wait` or + `bcdev_debug_continue`. +3. `bcdev_record_writes_status` reads progress without driving it. When the target operation has + finished, call `bcdev_record_writes_finish`. +4. Review `writers`, `unresolved`, `warnings`, and `complete`. A complete report covers only the + observed breaks in this one attached-session capture window; it is not a tenant-wide proof. + +Temporary-record writes are excluded by default. Pass `includeTemporary: true` only when they +matter. Deployed source is trusted; local source can prove a receiver only after you have published +that exact source and explicitly pass `changesDeployed: true`. Without that assertion, local-only +mapping remains unresolved. `maxObservedWrites` defaults to 500; reaching it releases the workload +and returns a truncated, incomplete report. Missing source/span, an unsupported receiver, watch +failure, unknown runtime table identity, or debugger failure also keeps the result fail-closed. +Finish always clears the shared debugger slot; manual debugging and record-write triage cannot run +at the same time. + ## Facts that save you time - **Choose one targeting mode.** Omit both selectors for the next client of `breakOnNext` type; diff --git a/src/core/agent-errors.ts b/src/core/agent-errors.ts index 31a4ee1..f82e909 100644 --- a/src/core/agent-errors.ts +++ b/src/core/agent-errors.ts @@ -7,6 +7,8 @@ export type AgentErrorCode = | "NOT_FOUND" | "NO_DEBUG_SESSION" | "DEBUG_SESSION_ACTIVE" + | "RECORD_WRITE_TRIAGE_ACTIVE" + | "RECORD_WRITE_TRIAGE_NOT_ACTIVE" | "TEST_RUN_ACTIVE" | "PROFILE_NOT_ACTIVE" | "PROFILE_ACTIVE" diff --git a/src/core/hubs/debugger-hub.ts b/src/core/hubs/debugger-hub.ts index f2adb98..a0b53f0 100644 --- a/src/core/hubs/debugger-hub.ts +++ b/src/core/hubs/debugger-hub.ts @@ -63,7 +63,10 @@ interface WireStackFrame { objectId: { objectType: number; objectNumber: number }; objectName: string; methodName: string; - statementSpan?: { from?: { line: number; column: number } }; + statementSpan?: { + from?: { line?: unknown; column?: unknown }; + to?: { line?: unknown; column?: unknown }; + }; } interface WireNstSessionInfo { @@ -146,6 +149,28 @@ function errorDetail(error: unknown): string { return redactAuthorization(error instanceof Error ? error.message : String(error)); } +function normalizeStatementSpan(value: WireStackFrame["statementSpan"]): StackFrameInfo["statementSpan"] { + const from = value?.from; + const to = value?.to; + const valid = typeof from?.line === "number" && Number.isInteger(from.line) && from.line >= 0 + && typeof from.column === "number" && Number.isInteger(from.column) && from.column >= 0 + && typeof to?.line === "number" && Number.isInteger(to.line) && to.line >= 0 + && typeof to.column === "number" && Number.isInteger(to.column) && to.column >= 0; + if (!valid) return undefined; + const fromLine = from.line as number; + const fromColumn = from.column as number; + const toLine = to.line as number; + const toColumn = to.column as number; + // WIRE: StackFrame.StatementSpan uses zero-based debugger coordinates (tw-decomp + // StackFrame.cs; live BC28 2026-07-03). Tool coordinates are 1-based. A zero-length + // all-zero value is the value-type default, not a usable statement location. + if (fromLine === 0 && fromColumn === 0 && toLine === 0 && toColumn === 0) return undefined; + return { + from: { line: fromLine + 1, column: fromColumn + 1 }, + to: { line: toLine + 1, column: toColumn + 1 }, + }; +} + function toVariableNode(n: WireLocalNode): VariableNode { // WIRE: LocalNode.ChangeState is the integer LocalNodeChangeState enum 0/1/2/3; the property has // no string-enum converter (tw-decomp LocalNode.cs and LocalNodeChangeState.cs). @@ -219,18 +244,23 @@ export class DebuggerClient { void hub.invoke("AcknowledgeIsAlive").catch(() => {}); }); hub.on("Break", (...args) => { + if (this.hub !== hub) return; const objectId = normalizeKeys<{ objectType: number; objectNumber: number }>(args[0]); if (this.userAttachFailed) return; const frames = normalizeKeys(args[1] ?? []); const errorMessage = (args[2] as string | null) || undefined; // WIRE: server lines are 0-based (live BC28 2026-07-03: wire line 9 = editor line 10); tool surface is 1-based (editor convention), converted here. - const stack: StackFrameInfo[] = frames.map((f) => ({ - objectType: f.objectId.objectType, - objectId: f.objectId.objectNumber, - objectName: f.objectName, - methodName: f.methodName, - line: (f.statementSpan?.from?.line ?? -1) + 1, - })); + const stack: StackFrameInfo[] = frames.map((f) => { + const statementSpan = normalizeStatementSpan(f.statementSpan); + return { + objectType: f.objectId.objectType, + objectId: f.objectId.objectNumber, + objectName: f.objectName, + methodName: f.methodName, + line: statementSpan?.from.line ?? ((typeof f.statementSpan?.from?.line === "number" ? f.statementSpan.from.line : -1) + 1), + ...(statementSpan ? { statementSpan } : {}), + }; + }); this.onEvent?.({ kind: "break", objectType: objectId.objectType, @@ -244,6 +274,7 @@ export class DebuggerClient { if (this.hub === hub) this.onEvent?.({ kind: "detached", terminateSession: Boolean(args[0]) }); }); hub.on("OnFatalDebuggerException", (...args) => { + if (this.hub !== hub) return; const message = String(args[0] ?? ""); // Live Sandbox BC28 can signal an unavailable exact session here immediately before the // Attach invocation resolves. Capture that synchronous attach outcome so the caller's diff --git a/src/core/record-write-triage.ts b/src/core/record-write-triage.ts new file mode 100644 index 0000000..6d1178d --- /dev/null +++ b/src/core/record-write-triage.ts @@ -0,0 +1,618 @@ +import type { DebuggerClient, VariableNode } from "./hubs/debugger-hub"; +import { redactAuthorization } from "./redaction"; +import type { DebuggerEvent, StackFrameInfo } from "./types"; + +export type RecordWriteOperation = "insert" | "modify" | "modifyAll" | "rename" | "delete" | "deleteAll"; +export type RecordWriteSource = "deployed" | "localAsserted"; +export type RecordWriteUnresolvedReason = + | "sourceUnavailable" + | "statementSpanUnavailable" + | "writeStatementUnrecognized" + | "receiverUnsupported" + | "receiverUnavailable" + | "receiverTypeUnresolved" + | "inspectionFailed" + | "unexpectedBreak"; + +export interface RecordWriteTarget { + tableId: number; + tableName: string | null; +} + +export interface RecordWriteSummary { + observedWrites: number; + matchedWrites: number; + uniqueWriters: number; + unrelatedWrites: number; + unresolvedWrites: number; +} + +export interface RecordWriteWriterGroup { + operation: RecordWriteOperation; + receiver: string; + count: number; + firstSequence: number; + lastSequence: number; + source: RecordWriteSource; + stack: StackFrameInfo[]; +} + +export interface RecordWriteUnresolvedGroup { + reason: RecordWriteUnresolvedReason; + operation: RecordWriteOperation | null; + receiver: string | null; + count: number; + firstSequence: number; + lastSequence: number; + stack: StackFrameInfo[]; +} + +export interface RecordWriteStatus { + phase: "arming" | "collecting" | "stopped" | "failed"; + target: RecordWriteTarget; + sessionId: number | null; + summary: RecordWriteSummary; + truncated: boolean; + warning: string | null; +} + +export interface RecordWriteReport { + target: RecordWriteTarget; + outcome: "completed" | "truncated" | "failed"; + stopReason: "finished" | "sessionDetached" | "maxObservedWrites" | "fatal"; + complete: boolean; + truncated: boolean; + summary: RecordWriteSummary; + writers: RecordWriteWriterGroup[]; + unresolved: RecordWriteUnresolvedGroup[]; + warnings: string[]; +} + +export interface RecordWriteStatement { + operation: RecordWriteOperation; + receiver: string; +} + +export type RecordWriteStatementResult = + | { statement: RecordWriteStatement; reason: null } + | { statement: null; reason: "writeStatementUnrecognized" | "receiverUnsupported" }; + +export interface RuntimeTableType { + tableId: number; + tableName: string; +} + +interface Token { + kind: "identifier" | "dot" | "leftParen" | "rightParen" | "leftBracket" | "rightBracket" | "other"; + text: string; +} + +interface SourceRecord { + content: string; + source: "deployed" | "local"; + trusted: boolean; +} + +export interface RecordWriteCollectorOptions { + tableId: number; + includeTemporary: boolean; + changesDeployed: boolean; + maxObservedWrites: number; + client: Pick; + localSource: (objectType: number, objectId: number) => Promise; + localFile: (objectType: number, objectId: number) => string | undefined; +} + +const OPERATION_BY_NAME: Record = { + // WIRE: DebugOptions.BreakOnRecordWrite stops on the AL database write methods + // Insert, Modify, ModifyAll, Rename, Delete, and DeleteAll (Microsoft AL debugger + // documentation; exercised by the live checklist in scripts/e2e.md). + insert: "insert", + modify: "modify", + modifyall: "modifyAll", + rename: "rename", + delete: "delete", + deleteall: "deleteAll", +}; + +function lexStatement(source: string): Token[] { + const tokens: Token[] = []; + for (let i = 0; i < source.length;) { + const ch = source[i]!; + const next = source[i + 1]; + if (/\s/.test(ch)) { + i++; + continue; + } + if (ch === "/" && next === "/") { + i += 2; + while (i < source.length && source[i] !== "\n") i++; + continue; + } + if (ch === "/" && next === "*") { + i += 2; + while (i < source.length && !(source[i] === "*" && source[i + 1] === "/")) i++; + i = Math.min(source.length, i + 2); + continue; + } + if (ch === "'") { + i++; + while (i < source.length) { + if (source[i] === "'" && source[i + 1] === "'") { + i += 2; + } else if (source[i] === "'") { + i++; + break; + } else { + i++; + } + } + continue; + } + if (ch === '"') { + let text = '"'; + i++; + while (i < source.length) { + text += source[i]!; + if (source[i] === '"' && source[i + 1] === '"') { + text += source[i + 1]!; + i += 2; + } else if (source[i] === '"') { + i++; + break; + } else { + i++; + } + } + tokens.push({ kind: "identifier", text }); + continue; + } + if (/[A-Za-z_]/.test(ch)) { + const start = i++; + while (i < source.length && /[A-Za-z0-9_]/.test(source[i]!)) i++; + tokens.push({ kind: "identifier", text: source.slice(start, i) }); + continue; + } + const kind = ch === "." + ? "dot" + : ch === "(" + ? "leftParen" + : ch === ")" + ? "rightParen" + : ch === "[" + ? "leftBracket" + : ch === "]" + ? "rightBracket" + : "other"; + tokens.push({ kind, text: ch }); + i++; + } + return tokens; +} + +function lineOffsets(source: string): number[] { + const offsets = [0]; + for (let i = 0; i < source.length; i++) { + if (source[i] === "\n") offsets.push(i + 1); + } + return offsets; +} + +export function sourceAtStatementSpan(source: string, span: NonNullable): string | null { + const offsets = lineOffsets(source); + const fromLine = span.from.line - 1; + const toLine = span.to.line - 1; + if (fromLine < 0 || toLine < fromLine || fromLine >= offsets.length || toLine >= offsets.length) return null; + const from = offsets[fromLine]! + Math.max(0, span.from.column - 1); + const lineEnd = toLine + 1 < offsets.length ? offsets[toLine + 1]! : source.length; + const to = Math.min(lineEnd, offsets[toLine]! + Math.max(span.to.column, span.from.line === span.to.line ? span.from.column : 1)); + if (from < 0 || from >= source.length || to <= from) return null; + return source.slice(from, to); +} + +export function inspectRecordWriteStatement(source: string): RecordWriteStatementResult { + const tokens = lexStatement(source); + const matches: RecordWriteStatement[] = []; + let unsupportedReceiver = false; + for (let i = 0; i < tokens.length - 1; i++) { + const token = tokens[i]!; + const operation = token.kind === "identifier" ? OPERATION_BY_NAME[token.text.toLowerCase()] : undefined; + if (!operation || tokens[i + 1]?.kind !== "leftParen") continue; + if (tokens[i - 1]?.kind === "dot") { + const receiver = tokens[i - 2]; + if (!receiver || receiver.kind !== "identifier") { + unsupportedReceiver = true; + continue; + } + const beforeReceiver = tokens[i - 3]; + if (beforeReceiver?.kind === "rightParen" || beforeReceiver?.kind === "rightBracket") { + unsupportedReceiver = true; + continue; + } + matches.push({ operation, receiver: receiver.text }); + } else { + matches.push({ operation, receiver: "Rec" }); + } + } + if (matches.length !== 1) { + return { + statement: null, + reason: unsupportedReceiver && matches.length === 0 ? "receiverUnsupported" : "writeStatementUnrecognized", + }; + } + return { statement: matches[0]!, reason: null }; +} + +export function parseRecordWriteStatement(source: string): RecordWriteStatement | null { + return inspectRecordWriteStatement(source).statement; +} + +export function parseRuntimeTableType(typeName: string): RuntimeTableType | null { + // WIRE: Live BC28 debugger record and RecordRef nodes render TypeName as + // "Table ()" (demos/TYPE-ZOO.md, 2026-07-03). + const match = /^\s*Table\s+(.+?)\s+\((\d+)\)\s*$/i.exec(typeName); + if (!match) return null; + const tableId = Number(match[2]); + const tableName = match[1]?.trim() ?? ""; + if (!Number.isSafeInteger(tableId) || tableId <= 0 || tableName === "") return null; + return { tableId, tableName }; +} + +function cloneStack(stack: StackFrameInfo[], localFile: RecordWriteCollectorOptions["localFile"]): StackFrameInfo[] { + return stack.map((frame) => { + const file = frame.file ?? localFile(frame.objectType, frame.objectId); + return { + ...frame, + ...(file ? { file } : {}), + ...(frame.statementSpan + ? { + statementSpan: { + from: { ...frame.statementSpan.from }, + to: { ...frame.statementSpan.to }, + }, + } + : {}), + }; + }); +} + +function stackKey(stack: StackFrameInfo[]): string { + return stack.map((frame) => `${frame.objectType}:${frame.objectId}:${frame.methodName}:${frame.line}`).join(">"); +} + +function receiverKey(receiver: string): string { + // AL identifiers are case-insensitive. Preserve the first source spelling in the + // report, but do not split one writer merely because source casing differs. + return receiver.toLocaleLowerCase("en-US"); +} + +function safeDetail(error: unknown): string { + return redactAuthorization(error instanceof Error ? error.message : String(error)); +} + +export class RecordWriteCollector { + private phaseValue: RecordWriteStatus["phase"] = "arming"; + private sessionObserved = false; + private sessionIdValue: number | null = null; + private tableNameValue: string | null = null; + private observedWrites = 0; + private matchedWrites = 0; + private unrelatedWrites = 0; + private unresolvedWrites = 0; + private truncatedValue = false; + private stopReasonValue: RecordWriteReport["stopReason"] | null = null; + private accepting = true; + private paused = false; + private evidenceLost = false; + private chain: Promise = Promise.resolve(); + private sourceCache = new Map>(); + private writers = new Map(); + private unresolved = new Map(); + private warnings: string[] = []; + + constructor(private readonly options: RecordWriteCollectorOptions) {} + + get client(): RecordWriteCollectorOptions["client"] { + return this.options.client; + } + + onEvent(event: DebuggerEvent): void { + if (!this.accepting) return; + this.chain = this.chain + .then(() => { + if (this.terminal()) return; + return this.handleEvent(event); + }) + .catch((error) => this.fail(`Record-write collector failed: ${safeDetail(error)}`)); + } + + async waitForIdle(): Promise { + await this.chain; + } + + status(): RecordWriteStatus { + return { + phase: this.phaseValue, + target: { tableId: this.options.tableId, tableName: this.tableNameValue }, + sessionId: this.sessionIdValue, + summary: this.summary(), + truncated: this.truncatedValue, + warning: this.warnings.at(-1) ?? null, + }; + } + + async finish(): Promise { + // Close intake synchronously, then drain every event accepted before this call. + // The queued handlers use terminal(), not accepting, so already accepted breaks + // are still classified and resumed. + this.accepting = false; + await this.chain; + if (this.paused) { + try { + await this.options.client.step("release"); + this.paused = false; + } catch (error) { + await this.fail(`Unable to release the paused workload: ${safeDetail(error)}`); + } + } + if (this.stopReasonValue === null) this.stopReasonValue = "finished"; + if (this.phaseValue !== "failed") this.phaseValue = "stopped"; + if (!this.sessionObserved && this.phaseValue !== "failed") { + this.warnings.push("No debugger session bound during the capture window."); + this.evidenceLost = true; + } + await this.options.client.stop().catch((error) => { + this.warnings.push(`Debugger cleanup warning: ${safeDetail(error)}`); + }); + return this.report(); + } + + private terminal(): boolean { + return this.phaseValue === "failed" + || this.stopReasonValue === "sessionDetached" + || this.stopReasonValue === "maxObservedWrites" + || this.stopReasonValue === "fatal"; + } + + private summary(): RecordWriteSummary { + return { + observedWrites: this.observedWrites, + matchedWrites: this.matchedWrites, + uniqueWriters: this.writers.size, + unrelatedWrites: this.unrelatedWrites, + unresolvedWrites: this.unresolvedWrites, + }; + } + + private report(): RecordWriteReport { + const outcome = this.phaseValue === "failed" + ? "failed" + : this.truncatedValue + ? "truncated" + : "completed"; + return { + target: { tableId: this.options.tableId, tableName: this.tableNameValue }, + outcome, + stopReason: this.stopReasonValue ?? "finished", + complete: outcome === "completed" && this.unresolvedWrites === 0 && !this.evidenceLost, + truncated: this.truncatedValue, + summary: this.summary(), + writers: [...this.writers.values()].sort((a, b) => a.firstSequence - b.firstSequence).map((group) => ({ + ...group, + stack: cloneStack(group.stack, this.options.localFile), + })), + unresolved: [...this.unresolved.values()].sort((a, b) => a.firstSequence - b.firstSequence).map((group) => ({ + ...group, + stack: cloneStack(group.stack, this.options.localFile), + })), + warnings: [...this.warnings], + }; + } + + private async handleEvent(event: DebuggerEvent): Promise { + if (event.kind === "sessionBound") { + this.sessionObserved = true; + this.phaseValue = "collecting"; + this.sessionIdValue = event.sessionId; + if ("warning" in event) this.warnings.push(event.warning); + return; + } + if (event.kind === "detached") { + this.sessionObserved = true; + this.paused = false; + this.accepting = false; + this.phaseValue = "stopped"; + this.stopReasonValue ??= "sessionDetached"; + return; + } + if (event.kind === "fatal") { + await this.fail(`Business Central debugger: ${safeDetail(event.message)}`); + return; + } + if (event.kind !== "break") { + return; + } + + this.phaseValue = "collecting"; + this.sessionObserved = true; + this.paused = true; + const sequence = ++this.observedWrites; + const stack = cloneStack(event.stack, this.options.localFile); + if (event.errorMessage) { + this.recordUnresolved(sequence, stack, "unexpectedBreak", null, null); + } else { + await this.classify(sequence, stack); + } + + if (this.observedWrites >= this.options.maxObservedWrites) { + this.truncatedValue = true; + this.evidenceLost = true; + this.stopReasonValue = "maxObservedWrites"; + this.phaseValue = "stopped"; + this.accepting = false; + await this.options.client.step("release"); + this.paused = false; + this.warnings.push( + `Stopped after maxObservedWrites=${this.options.maxObservedWrites}; the capture is truncated.`, + ); + return; + } + + await this.options.client.step("continue"); + this.paused = false; + } + + private async classify(sequence: number, stack: StackFrameInfo[]): Promise { + const frame = stack[0]; + if (!frame) { + this.recordUnresolved(sequence, stack, "statementSpanUnavailable", null, null); + return; + } + if (!frame.statementSpan) { + this.recordUnresolved(sequence, stack, "statementSpanUnavailable", null, null); + return; + } + let source: SourceRecord | null; + try { + source = await this.source(frame.objectType, frame.objectId); + } catch (error) { + this.recordUnresolved(sequence, stack, "inspectionFailed", null, null); + this.warnings.push(`Source inspection failed at write ${sequence}: ${safeDetail(error)}`); + return; + } + if (!source) { + this.recordUnresolved(sequence, stack, "sourceUnavailable", null, null); + return; + } + const statementSource = sourceAtStatementSpan(source.content, frame.statementSpan); + if (statementSource === null) { + this.recordUnresolved(sequence, stack, "statementSpanUnavailable", null, null); + return; + } + const inspected = inspectRecordWriteStatement(statementSource); + if (!inspected.statement) { + this.recordUnresolved(sequence, stack, inspected.reason, null, null); + return; + } + const statement = inspected.statement; + if (!source.trusted) { + this.recordUnresolved(sequence, stack, "sourceUnavailable", statement.operation, statement.receiver); + this.warnings.push( + `Write ${sequence} mapped only to local source; rerun with changesDeployed: true only after publishing that source.`, + ); + return; + } + let node: VariableNode | null; + try { + node = await this.options.client.evalWatch(0, statement.receiver); + } catch (error) { + this.recordUnresolved(sequence, stack, "inspectionFailed", statement.operation, statement.receiver); + this.warnings.push(`Receiver inspection failed at write ${sequence}: ${safeDetail(error)}`); + return; + } + if (!node) { + this.recordUnresolved(sequence, stack, "receiverUnavailable", statement.operation, statement.receiver); + return; + } + const runtime = parseRuntimeTableType(node.typeName); + if (!runtime) { + this.recordUnresolved(sequence, stack, "receiverTypeUnresolved", statement.operation, statement.receiver); + return; + } + if (runtime.tableId !== this.options.tableId) { + this.unrelatedWrites++; + return; + } + if (this.tableNameValue === null) { + this.tableNameValue = runtime.tableName; + } else if (this.tableNameValue !== runtime.tableName) { + this.warnings.push( + `Target table ${this.options.tableId} was reported with both '${this.tableNameValue}' and '${runtime.tableName}'; keeping the first name.`, + ); + } + this.matchedWrites++; + const provenance: RecordWriteSource = source.source === "deployed" ? "deployed" : "localAsserted"; + const key = `${statement.operation}|${receiverKey(statement.receiver)}|${stackKey(stack)}`; + const existing = this.writers.get(key); + if (existing) { + existing.count++; + existing.lastSequence = sequence; + } else { + this.writers.set(key, { + operation: statement.operation, + receiver: statement.receiver, + count: 1, + firstSequence: sequence, + lastSequence: sequence, + source: provenance, + stack, + }); + } + } + + private source(objectType: number, objectId: number): Promise { + const key = `${objectType}:${objectId}`; + const cached = this.sourceCache.get(key); + if (cached) return cached; + const pending = (async () => { + try { + const deployed = await this.options.client.getSourceContent(objectType, objectId); + if (deployed.isAlContent && deployed.content !== "") { + return { content: deployed.content, source: "deployed", trusted: true } satisfies SourceRecord; + } + } catch { + // Local source remains a possible, explicitly asserted fallback. + } + const local = await this.options.localSource(objectType, objectId); + if (local === null) return null; + return { + content: local, + source: "local", + trusted: this.options.changesDeployed, + } satisfies SourceRecord; + })(); + this.sourceCache.set(key, pending); + return pending; + } + + private recordUnresolved( + sequence: number, + stack: StackFrameInfo[], + reason: RecordWriteUnresolvedReason, + operation: RecordWriteOperation | null, + receiver: string | null, + ): void { + this.unresolvedWrites++; + this.evidenceLost = true; + const key = `${reason}|${operation ?? ""}|${receiver ?? ""}|${stackKey(stack)}`; + const existing = this.unresolved.get(key); + if (existing) { + existing.count++; + existing.lastSequence = sequence; + } else { + this.unresolved.set(key, { + reason, + operation, + receiver, + count: 1, + firstSequence: sequence, + lastSequence: sequence, + stack, + }); + } + } + + private async fail(message: string): Promise { + if (this.phaseValue === "failed") return; + this.accepting = false; + this.phaseValue = "failed"; + this.stopReasonValue = "fatal"; + this.evidenceLost = true; + this.warnings.push(message); + if (this.paused) { + await this.options.client.step("release").catch(() => {}); + this.paused = false; + } + await this.options.client.stop().catch(() => {}); + } +} diff --git a/src/core/types.ts b/src/core/types.ts index 7901779..b931c06 100644 --- a/src/core/types.ts +++ b/src/core/types.ts @@ -155,6 +155,10 @@ export interface StackFrameInfo { methodName: string; line: number; file?: string; + statementSpan?: { + from: { line: number; column: number }; + to: { line: number; column: number }; + }; } export type DebuggerEvent = diff --git a/src/mcp/agent-errors.ts b/src/mcp/agent-errors.ts index d633e91..4b510e1 100644 --- a/src/mcp/agent-errors.ts +++ b/src/mcp/agent-errors.ts @@ -18,6 +18,8 @@ function fromKnownMessage(message: string): BcDevError { const lower = message.toLowerCase(); if (lower.includes("no debug session")) return new BcDevError("NO_DEBUG_SESSION", message, "state"); if (lower.includes("debug session already active")) return new BcDevError("DEBUG_SESSION_ACTIVE", message, "state"); + if (lower.includes("record-write triage is active")) return new BcDevError("RECORD_WRITE_TRIAGE_ACTIVE", message, "state"); + if (lower.includes("no active record-write triage")) return new BcDevError("RECORD_WRITE_TRIAGE_NOT_ACTIVE", message, "state"); if (lower.includes("test run is already") || lower.includes("test run is in progress")) return new BcDevError("TEST_RUN_ACTIVE", message, "state"); if (lower.includes("no active profile") || lower.includes("no profile capture")) return new BcDevError("PROFILE_NOT_ACTIVE", message, "state"); if (lower.includes("profile capture already active")) return new BcDevError("PROFILE_ACTIVE", message, "state"); @@ -60,6 +62,8 @@ function recoverySteps(code: AgentErrorCode): string[] { switch (code) { case "NO_DEBUG_SESSION": return ["Call bcdev_debug_attach before using debugger session tools."]; case "DEBUG_SESSION_ACTIVE": return ["Call bcdev_debug_detach before starting another debugger session."]; + case "RECORD_WRITE_TRIAGE_ACTIVE": return ["Call bcdev_record_writes_status or bcdev_record_writes_finish before using manual debugger tools."]; + case "RECORD_WRITE_TRIAGE_NOT_ACTIVE": return ["Call bcdev_record_writes_start before checking or finishing record-write triage."]; case "TEST_RUN_ACTIVE": return ["Wait for the active test run to finish, then retry."]; case "PROFILE_NOT_ACTIVE": return ["Call bcdev_profile_start before polling or finishing a profile."]; case "PROFILE_ACTIVE": return ["Call bcdev_profile_finish before starting another profile capture."]; diff --git a/src/mcp/skills.generated.ts b/src/mcp/skills.generated.ts index c367614..86ce350 100644 --- a/src/mcp/skills.generated.ts +++ b/src/mcp/skills.generated.ts @@ -11,10 +11,10 @@ export interface EmbeddedSkill { export const skills: EmbeddedSkill[] = [ { "name": "bc-al-debugging", - "description": "Interactively debug AL code on a Business Central server with the bc-dev-mcp tools — attach, breakpoints, break-on-error, step, inspect variables and watches during a test run. Use when asked to debug AL code, find why an AL test fails, or inspect live state on a BC server.", + "description": "Interactively debug AL code on a Business Central server with the bc-dev-mcp tools — attach, breakpoints, break-on-error, step, inspect variables and watches, or collect the stacks that write a table. Use when asked to debug AL code, find why an AL test fails, identify table writers, or inspect live state on a BC server.", "uri": "skill://bc-al-debugging/SKILL.md", "mimeType": "text/markdown", - "text": "---\nname: bc-al-debugging\ndescription: Interactively debug AL code on a Business Central server with the bc-dev-mcp tools — attach, breakpoints, break-on-error, step, inspect variables and watches during a test run. Use when asked to debug AL code, find why an AL test fails, or inspect live state on a BC server.\n---\n\n# Debugging AL on Business Central\n\n## The loop\n\n1. `bcdev_debug_attach { breakOnError: true }` — arm the next WebClient session WITHOUT\n file/line breakpoints (see \"session must pause first\" below). To filter that next session,\n add `userId`; to attach to a known active NST session, add its positive `sessionId` instead.\n The selectors are mutually exclusive, and exact `sessionId` targeting takes precedence over\n `breakOnNext`. One debugger at a time; `bcdev_debug_detach` before re-attaching.\n2. `bcdev_debug_run_tests { codeunits: [{ id: }] }` — starts the run bound to\n this debug session and returns immediately.\n3. `bcdev_debug_wait` — long-poll for events. Attach returns before a workload binds; a successful\n bind reports `kind: sessionBound` with `sessionId` and `hostId` (the optional `hostId` may be null). **A timeout is a normal result**\n (`timedOut: true`): just call it again. Other events carry `kind`: `break`,\n `testRunFinished` (with the test results embedded), `detached`, `fatal`.\n If waiting times out before binding, confirm that you actually created or triggered the matching\n WebClient/workload, then wait again; waiting alone cannot create a next-session target.\n4. At a `break`: `bcdev_debug_variables { frameId: 0 }` for locals,\n `bcdev_debug_eval { frameId: 0, expression: \"...\" }` for watches,\n `bcdev_debug_breakpoints { add: [...] }` to place file/line breakpoints.\n5. `bcdev_debug_continue { action: \"continue\" | \"stepOver\" | \"stepInto\" | \"stepOut\" }`,\n then `bcdev_debug_wait` again.\n6. `bcdev_debug_detach` when done.\n\n## Facts that save you time\n\n- **Choose one targeting mode.** Omit both selectors for the next client of `breakOnNext` type;\n use `userId` for the next matching client owned by that user; use `sessionId` only when you\n already know the positive ID of an active NST session. Arm next-session/user-filtered attach\n before triggering the workload. A warning-form `sessionBound` with null identity means only\n correlation metadata failed — debugging remains active.\n- If a user-filtered attach reports `fatal` before `sessionBound`, Business Central rejected the\n user filter. The client tears down that debugger and does not bind or deliver breaks; fix the\n user ID or permissions before retrying.\n- **Session must pause once before breakpoints bind.** `AddBreakpoint` fails server-side\n (\"tenant '' not found\") until the session has paused at least once. That is why you\n attach with `breakOnError: true` and add breakpoints at the first break. Once the\n session is live, breakpoints added for later code in the same run fire normally.\n- **Line numbers are 1-based** everywhere in these tools — use the line you see in your\n editor. (The BC wire is 0-based; conversion is internal.)\n- **Check breakpoint verification.** Each added breakpoint returns `verification.status`.\n `verified` means the server accepted the requested line, `relocated` means it moved to the\n returned 1-based span, and `unverified` means the server returned a usable breakpoint ID without\n a resolved span, including the all-zero value it uses for an unset span. Use a non-null returned\n span rather than assuming the requested line will fire.\n- **Watch expressions are paths, not code.** `bcdev_debug_eval` resolves identifier /\n member paths only (`CustomerName`, `Customer.\"No.\"`). Operators come back\n `` — and leave a synthetic empty-method entry in the test-run summary.\n- **Variable expansion:** nodes with `hasChildren: true` expand via\n `bcdev_debug_variables { frameId, expand: \"\" }`, e.g. `\"Customer\"` or\n `\"Customer.Fields\"`. Globals live under the `` node (or pass\n `globals: true`).\n- **Variable changes come from Business Central.** Each variable/watch node has `changeState`\n (`unchanged`, `new`, `valueChanged`, `descendantChanged`, or `unknown`) and `changed` is true\n for the three changed states. On a first read, or when the server omits the wire flag,\n `unknown` is not proof that a value stayed the same.\n- **Pages double-break:** page triggers fire twice per interaction on BC28 — expect two\n identical breaks, continue through the second.\n- **Test results arrive on the `testRunFinished` event** from `bcdev_debug_wait`, not from\n `bcdev_debug_run_tests` (which only starts the run).\n- `break` events map object IDs back to local `.al` files (`file` on the event and each\n stack frame) when the objects exist in the project.\n- **A break frame with no local `file`** (third-party extension, generated object) is readable\n anyway: `bcdev_source { objectType, objectId }` fetches the server's deployed source. Empty\n content with `isAlContent: false` means the object ships without source (base application).\n- **SQL triage:** attach with `sqlInsight: true` (optionally `longRunningSqlThresholdMs`), then\n at each break `bcdev_debug_sql` returns current latency, execute count, and the last SQL\n statements with duration and approximate rows read. It errors if insight was not enabled at\n attach — the option cannot be turned on mid-session.\n- **abort vs release at a break** (both end debugging with a `detached` event): `abort` kills\n the paused operation — a bound test run records it as failed; `release` lets it finish\n undebugged — a passing test still passes. Neither leaves a reusable session: a debug-bound\n test session ends with its run, so re-attach fresh instead of exact-targeting the old ID.\n- **Precision breaks:** `breakOnError: \"unhandled\"` skips errors a try function catches and\n still breaks on uncaught ones; `breakOnRecordWrite: \"nonTemporary\"` skips temporary-record\n writes and still breaks on real-table writes.\n- **Responses guide the loop.** Every successful tool result includes `nextSteps` (possibly\n empty for terminal results). MCP errors have `isError: true`, no `structuredContent`, and a\n JSON text body with stable `error.code`, `retryable`, redacted detail, and recovery steps.\n" + "text": "---\nname: bc-al-debugging\ndescription: Interactively debug AL code on a Business Central server with the bc-dev-mcp tools — attach, breakpoints, break-on-error, step, inspect variables and watches, or collect the stacks that write a table. Use when asked to debug AL code, find why an AL test fails, identify table writers, or inspect live state on a BC server.\n---\n\n# Debugging AL on Business Central\n\n## The loop\n\n1. `bcdev_debug_attach { breakOnError: true }` — arm the next WebClient session WITHOUT\n file/line breakpoints (see \"session must pause first\" below). To filter that next session,\n add `userId`; to attach to a known active NST session, add its positive `sessionId` instead.\n The selectors are mutually exclusive, and exact `sessionId` targeting takes precedence over\n `breakOnNext`. One debugger at a time; `bcdev_debug_detach` before re-attaching.\n2. `bcdev_debug_run_tests { codeunits: [{ id: }] }` — starts the run bound to\n this debug session and returns immediately.\n3. `bcdev_debug_wait` — long-poll for events. Attach returns before a workload binds; a successful\n bind reports `kind: sessionBound` with `sessionId` and `hostId` (the optional `hostId` may be null). **A timeout is a normal result**\n (`timedOut: true`): just call it again. Other events carry `kind`: `break`,\n `testRunFinished` (with the test results embedded), `detached`, `fatal`.\n If waiting times out before binding, confirm that you actually created or triggered the matching\n WebClient/workload, then wait again; waiting alone cannot create a next-session target.\n4. At a `break`: `bcdev_debug_variables { frameId: 0 }` for locals,\n `bcdev_debug_eval { frameId: 0, expression: \"...\" }` for watches,\n `bcdev_debug_breakpoints { add: [...] }` to place file/line breakpoints.\n5. `bcdev_debug_continue { action: \"continue\" | \"stepOver\" | \"stepInto\" | \"stepOut\" }`,\n then `bcdev_debug_wait` again.\n6. `bcdev_debug_detach` when done.\n\n## Find everything that writes one table\n\nUse the dedicated background workflow instead of manual `bcdev_debug_*` stepping:\n\n1. `bcdev_record_writes_start { tableId: }`.\n Add exactly one of `sessionId` or `userId` when needed, or choose `breakOnNext`.\n The tool returns after attach is armed; it does not wait for the session or workload.\n2. Trigger the matching session and operation. Business Central breaks on every record write\n globally; the collector identifies the runtime receiver, counts unrelated writes, groups exact\n target-table stacks, and automatically continues. Do not call `bcdev_debug_wait` or\n `bcdev_debug_continue`.\n3. `bcdev_record_writes_status` reads progress without driving it. When the target operation has\n finished, call `bcdev_record_writes_finish`.\n4. Review `writers`, `unresolved`, `warnings`, and `complete`. A complete report covers only the\n observed breaks in this one attached-session capture window; it is not a tenant-wide proof.\n\nTemporary-record writes are excluded by default. Pass `includeTemporary: true` only when they\nmatter. Deployed source is trusted; local source can prove a receiver only after you have published\nthat exact source and explicitly pass `changesDeployed: true`. Without that assertion, local-only\nmapping remains unresolved. `maxObservedWrites` defaults to 500; reaching it releases the workload\nand returns a truncated, incomplete report. Missing source/span, an unsupported receiver, watch\nfailure, unknown runtime table identity, or debugger failure also keeps the result fail-closed.\nFinish always clears the shared debugger slot; manual debugging and record-write triage cannot run\nat the same time.\n\n## Facts that save you time\n\n- **Choose one targeting mode.** Omit both selectors for the next client of `breakOnNext` type;\n use `userId` for the next matching client owned by that user; use `sessionId` only when you\n already know the positive ID of an active NST session. Arm next-session/user-filtered attach\n before triggering the workload. A warning-form `sessionBound` with null identity means only\n correlation metadata failed — debugging remains active.\n- If a user-filtered attach reports `fatal` before `sessionBound`, Business Central rejected the\n user filter. The client tears down that debugger and does not bind or deliver breaks; fix the\n user ID or permissions before retrying.\n- **Session must pause once before breakpoints bind.** `AddBreakpoint` fails server-side\n (\"tenant '' not found\") until the session has paused at least once. That is why you\n attach with `breakOnError: true` and add breakpoints at the first break. Once the\n session is live, breakpoints added for later code in the same run fire normally.\n- **Line numbers are 1-based** everywhere in these tools — use the line you see in your\n editor. (The BC wire is 0-based; conversion is internal.)\n- **Check breakpoint verification.** Each added breakpoint returns `verification.status`.\n `verified` means the server accepted the requested line, `relocated` means it moved to the\n returned 1-based span, and `unverified` means the server returned a usable breakpoint ID without\n a resolved span, including the all-zero value it uses for an unset span. Use a non-null returned\n span rather than assuming the requested line will fire.\n- **Watch expressions are paths, not code.** `bcdev_debug_eval` resolves identifier /\n member paths only (`CustomerName`, `Customer.\"No.\"`). Operators come back\n `` — and leave a synthetic empty-method entry in the test-run summary.\n- **Variable expansion:** nodes with `hasChildren: true` expand via\n `bcdev_debug_variables { frameId, expand: \"\" }`, e.g. `\"Customer\"` or\n `\"Customer.Fields\"`. Globals live under the `` node (or pass\n `globals: true`).\n- **Variable changes come from Business Central.** Each variable/watch node has `changeState`\n (`unchanged`, `new`, `valueChanged`, `descendantChanged`, or `unknown`) and `changed` is true\n for the three changed states. On a first read, or when the server omits the wire flag,\n `unknown` is not proof that a value stayed the same.\n- **Pages double-break:** page triggers fire twice per interaction on BC28 — expect two\n identical breaks, continue through the second.\n- **Test results arrive on the `testRunFinished` event** from `bcdev_debug_wait`, not from\n `bcdev_debug_run_tests` (which only starts the run).\n- `break` events map object IDs back to local `.al` files (`file` on the event and each\n stack frame) when the objects exist in the project.\n- **A break frame with no local `file`** (third-party extension, generated object) is readable\n anyway: `bcdev_source { objectType, objectId }` fetches the server's deployed source. Empty\n content with `isAlContent: false` means the object ships without source (base application).\n- **SQL triage:** attach with `sqlInsight: true` (optionally `longRunningSqlThresholdMs`), then\n at each break `bcdev_debug_sql` returns current latency, execute count, and the last SQL\n statements with duration and approximate rows read. It errors if insight was not enabled at\n attach — the option cannot be turned on mid-session.\n- **abort vs release at a break** (both end debugging with a `detached` event): `abort` kills\n the paused operation — a bound test run records it as failed; `release` lets it finish\n undebugged — a passing test still passes. Neither leaves a reusable session: a debug-bound\n test session ends with its run, so re-attach fresh instead of exact-targeting the old ID.\n- **Precision breaks:** `breakOnError: \"unhandled\"` skips errors a try function catches and\n still breaks on uncaught ones; `breakOnRecordWrite: \"nonTemporary\"` skips temporary-record\n writes and still breaks on real-table writes.\n- **Responses guide the loop.** Every successful tool result includes `nextSteps` (possibly\n empty for terminal results). MCP errors have `isError: true`, no `structuredContent`, and a\n JSON text body with stable `error.code`, `retryable`, redacted detail, and recovery steps.\n" }, { "name": "bc-al-testing", @@ -25,4 +25,4 @@ export const skills: EmbeddedSkill[] = [ } ]; -export const skillsIndexJson: string = "{\n \"$schema\": \"https://schemas.agentskills.io/discovery/0.2.0/schema.json\",\n \"skills\": [\n {\n \"name\": \"bc-al-debugging\",\n \"type\": \"skill-md\",\n \"description\": \"Interactively debug AL code on a Business Central server with the bc-dev-mcp tools — attach, breakpoints, break-on-error, step, inspect variables and watches during a test run. Use when asked to debug AL code, find why an AL test fails, or inspect live state on a BC server.\",\n \"url\": \"skill://bc-al-debugging/SKILL.md\"\n },\n {\n \"name\": \"bc-al-testing\",\n \"type\": \"skill-md\",\n \"description\": \"Run AL tests against a Business Central dev endpoint with the bc-dev-mcp tools — discover test codeunits, run them with structured per-method results, optionally collect code coverage. Use when asked to run, re-run, or triage AL tests on a BC server.\",\n \"url\": \"skill://bc-al-testing/SKILL.md\"\n }\n ]\n}"; +export const skillsIndexJson: string = "{\n \"$schema\": \"https://schemas.agentskills.io/discovery/0.2.0/schema.json\",\n \"skills\": [\n {\n \"name\": \"bc-al-debugging\",\n \"type\": \"skill-md\",\n \"description\": \"Interactively debug AL code on a Business Central server with the bc-dev-mcp tools — attach, breakpoints, break-on-error, step, inspect variables and watches, or collect the stacks that write a table. Use when asked to debug AL code, find why an AL test fails, identify table writers, or inspect live state on a BC server.\",\n \"url\": \"skill://bc-al-debugging/SKILL.md\"\n },\n {\n \"name\": \"bc-al-testing\",\n \"type\": \"skill-md\",\n \"description\": \"Run AL tests against a Business Central dev endpoint with the bc-dev-mcp tools — discover test codeunits, run them with structured per-method results, optionally collect code coverage. Use when asked to run, re-run, or triage AL tests on a BC server.\",\n \"url\": \"skill://bc-al-testing/SKILL.md\"\n }\n ]\n}"; diff --git a/src/mcp/state.ts b/src/mcp/state.ts index ea261e8..130e3d2 100644 --- a/src/mcp/state.ts +++ b/src/mcp/state.ts @@ -1,6 +1,8 @@ import type { AlObjectIndex } from "../core/al-objects"; import type { AuthorizationProvider } from "../core/authorization"; +import { BcDevError } from "../core/agent-errors"; import type { DebuggerClient } from "../core/hubs/debugger-hub"; +import type { RecordWriteCollector } from "../core/record-write-triage"; import type { BreakpointSpec, ConnectionConfig, DebuggerEvent, RunTestsResult } from "../core/types"; const QUEUE_CAP = 100; @@ -26,6 +28,7 @@ export class DebugSession { constructor( public readonly client: DebuggerClient, public readonly index: AlObjectIndex, + public readonly debugSlotToken?: symbol, ) {} get queueLength(): number { @@ -65,6 +68,41 @@ export class DebugSession { export class ServerState { debug: DebugSession | null = null; + recordWrites: RecordWriteCollector | null = null; + recordWriteSlotToken: symbol | null = null; testRunActive = false; profile: ProfileHandle | null = null; + private debugSlot: { owner: "manual" | "recordWrites"; token: symbol } | null = null; + + get debugOwner(): "manual" | "recordWrites" | null { + return this.debugSlot?.owner ?? null; + } + + assertDebugSlotAvailable(): void { + if (this.debugSlot?.owner === "recordWrites" || (!this.debugSlot && this.recordWrites)) { + throw new BcDevError( + "RECORD_WRITE_TRIAGE_ACTIVE", + "Record-write triage is active — call bcdev_record_writes_status or bcdev_record_writes_finish first", + "state", + ); + } + if (this.debugSlot || this.debug) { + throw new BcDevError( + "DEBUG_SESSION_ACTIVE", + "Debug session already active — call bcdev_debug_detach first", + "state", + ); + } + } + + claimDebugSlot(owner: "manual" | "recordWrites"): symbol { + this.assertDebugSlotAvailable(); + const token = Symbol(owner); + this.debugSlot = { owner, token }; + return token; + } + + releaseDebugSlot(token: symbol): void { + if (this.debugSlot?.token === token) this.debugSlot = null; + } } diff --git a/src/mcp/tools/agent-response.ts b/src/mcp/tools/agent-response.ts index fd21533..e74f029 100644 --- a/src/mcp/tools/agent-response.ts +++ b/src/mcp/tools/agent-response.ts @@ -65,6 +65,28 @@ function generatedNextSteps(name: string, result: Record, param return []; case "bcdev_debug_detach": return []; + case "bcdev_record_writes_start": + return ["Trigger the matching session and target operation, then call bcdev_record_writes_status."]; + case "bcdev_record_writes_status": + if (result["phase"] === "arming") { + return ["Trigger the matching session and target operation, then call bcdev_record_writes_status again."]; + } + if (result["phase"] === "collecting") { + return ["Let the target operation finish, then call bcdev_record_writes_finish for the grouped writer report."]; + } + return ["Call bcdev_record_writes_finish to retrieve the retained report and clear the debugger slot."]; + case "bcdev_record_writes_finish": { + const summary = result["summary"] as Record | undefined; + if (result["truncated"] === true) { + return ["Narrow the target workload or deliberately raise maxObservedWrites, then rerun the capture."]; + } + if (result["complete"] !== true) { + return ["Review unresolved groups and warnings before treating the capture window as exhaustive, then rerun with trusted deployed source if needed."]; + } + return Number(summary?.["matchedWrites"] ?? 0) > 0 + ? ["Review the grouped writer stacks and occurrence counts."] + : []; + } case "bcdev_profile_status": if (result["reachable"] !== true) { return ["Correct connectivity, authentication, or snapshot-port settings, then call bcdev_profile_status again."]; diff --git a/src/mcp/tools/debug-tools.ts b/src/mcp/tools/debug-tools.ts index ea78a4e..5946b08 100644 --- a/src/mcp/tools/debug-tools.ts +++ b/src/mcp/tools/debug-tools.ts @@ -14,6 +14,7 @@ import { codeunitsShape, connectionShape, mapBreakpoints, + normalizeDebugTarget, requireSession, requireTestRunningSupport, resolve, @@ -31,32 +32,6 @@ const sqlStatementSchema = z.object({ approxRowsRead: z.number().nullable(), }); -interface DebugTarget { - sessionId?: number; - userId?: string; -} - -function normalizeDebugTarget(params: Record): DebugTarget { - const sessionId = params["sessionId"]; - const userId = params["userId"]; - if (sessionId !== undefined && userId !== undefined) { - throw new Error("sessionId and userId are mutually exclusive"); - } - if (sessionId !== undefined) { - if (typeof sessionId !== "number" || !Number.isInteger(sessionId) || sessionId <= 0) { - throw new Error("sessionId must be a positive integer"); - } - return { sessionId }; - } - if (userId !== undefined) { - if (typeof userId !== "string" || userId.trim() === "") { - throw new Error("userId must be a nonblank string"); - } - return { userId: userId.trim() }; - } - return {}; -} - export function createDebugTools(state: ServerState, deps: ToolDeps): ToolDefinition[] { return [ { @@ -112,15 +87,17 @@ export function createDebugTools(state: ServerState, deps: ToolDeps): ToolDefini breakpoints: z.array(addedBreakpointSchema), }), handler: async (params) => { - if (state.debug) throw new BcDevError("DEBUG_SESSION_ACTIVE", "Debug session already active — call bcdev_debug_detach first", "state"); + state.assertDebugSlotAvailable(); const target = normalizeDebugTarget(params); - const { config, authorization, project } = resolve(params, deps); + const slotToken = state.claimDebugSlot("manual"); const client = new DebuggerClient(deps.hubFactory); - const index = await AlObjectIndex.build(project); - const session = new DebugSession(client, index); - state.debug = session; // claim the slot before awaiting — blocks concurrent attach - client.onEvent = (e) => session.push(e); + let session: DebugSession | null = null; try { + const { config, authorization, project } = resolve(params, deps); + const index = await AlObjectIndex.build(project); + session = new DebugSession(client, index, slotToken); + state.debug = session; + client.onEvent = (e) => session?.push(e); await client.connect(config, authorization, { breakOnNext: params["breakOnNext"] as never, ...target, @@ -137,7 +114,8 @@ export function createDebugTools(state: ServerState, deps: ToolDeps): ToolDefini ); return { attached: true, connectionId: client.connectionId, breakpoints }; } catch (err) { - state.debug = null; + if (state.debug === session) state.debug = null; + state.releaseDebugSlot(slotToken); await client.stop().catch(() => {}); throw err; } @@ -344,7 +322,11 @@ export function createDebugTools(state: ServerState, deps: ToolDeps): ToolDefini handler: async () => { const session = requireSession(state); state.debug = null; - await session.client.stop(); + try { + await session.client.stop(); + } finally { + if (session.debugSlotToken) state.releaseDebugSlot(session.debugSlotToken); + } return { detached: true }; }, }, diff --git a/src/mcp/tools/index.ts b/src/mcp/tools/index.ts index 51db8bd..4e09608 100644 --- a/src/mcp/tools/index.ts +++ b/src/mcp/tools/index.ts @@ -6,12 +6,14 @@ import { createDebugTools } from "./debug-tools"; import { createProfileTools } from "./profile-tools"; import { createSourceTools } from "./source-tools"; import { createTestTools } from "./test-tools"; +import { createRecordWriteTools } from "./record-write-tools"; import { withAgentResponses } from "./agent-response"; export function createTools(state: ServerState, deps: ToolDeps): ToolDefinition[] { return [ ...createTestTools(state, deps), ...createDebugTools(state, deps), + ...createRecordWriteTools(state, deps), ...createProfileTools(state, deps), ...createSourceTools(state, deps), ].map(withAgentResponses); diff --git a/src/mcp/tools/record-write-tools.ts b/src/mcp/tools/record-write-tools.ts new file mode 100644 index 0000000..2aa225d --- /dev/null +++ b/src/mcp/tools/record-write-tools.ts @@ -0,0 +1,264 @@ +import { readFile } from "node:fs/promises"; +import { z } from "zod"; +import { AlObjectIndex } from "../../core/al-objects"; +import { BcDevError } from "../../core/agent-errors"; +import { DebuggerClient } from "../../core/hubs/debugger-hub"; +import { + RecordWriteCollector, + type RecordWriteReport, + type RecordWriteStatus, +} from "../../core/record-write-triage"; +import type { ServerState } from "../state"; +import { + connectionShape, + normalizeDebugTarget, + resolve, + stackFrameSchema, + type ToolDefinition, + type ToolDeps, +} from "./shared"; + +const summarySchema = z.object({ + observedWrites: z.number().describe("All record-write breaks observed in this capture window"), + matchedWrites: z.number().describe("Observed writes proven to target tableId"), + uniqueWriters: z.number().describe("Distinct operation/receiver/full-stack groups writing tableId"), + unrelatedWrites: z.number().describe("Observed writes proven to target another table"), + unresolvedWrites: z.number().describe("Observed writes whose runtime table identity could not be proven"), +}).describe("Current record-write classification counts"); + +const targetSchema = z.object({ + tableId: z.number().describe("Requested numeric Business Central table ID"), + tableName: z.string().nullable().describe("Runtime table name once an exact target match establishes it"), +}).describe("Capture target"); + +const writerSchema = z.object({ + operation: z.enum(["insert", "modify", "modifyAll", "rename", "delete", "deleteAll"]) + .describe("Documented AL record-write operation"), + receiver: z.string().describe("AL record or RecordRef receiver evaluated at the break"), + count: z.number().describe("Occurrences sharing this operation, receiver, and complete stack"), + firstSequence: z.number().describe("First observed-write sequence in this group"), + lastSequence: z.number().describe("Last observed-write sequence in this group"), + source: z.enum(["deployed", "localAsserted"]).describe("Source provenance used to identify the receiver"), + stack: z.array(stackFrameSchema).describe("Complete normalized AL call stack for this writer"), +}).describe("One grouped exact writer"); + +const unresolvedSchema = z.object({ + reason: z.enum([ + "sourceUnavailable", + "statementSpanUnavailable", + "writeStatementUnrecognized", + "receiverUnsupported", + "receiverUnavailable", + "receiverTypeUnresolved", + "inspectionFailed", + "unexpectedBreak", + ]).describe("Stable reason the runtime target table could not be proven"), + operation: z.enum(["insert", "modify", "modifyAll", "rename", "delete", "deleteAll"]) + .nullable() + .describe("Parsed write operation, when available"), + receiver: z.string().nullable().describe("Parsed receiver, when available"), + count: z.number().describe("Occurrences sharing this reason and complete stack"), + firstSequence: z.number().describe("First observed-write sequence in this group"), + lastSequence: z.number().describe("Last observed-write sequence in this group"), + stack: z.array(stackFrameSchema).describe("Complete normalized AL call stack for the unresolved write"), +}).describe("One grouped unresolved write"); + +const statusSchema = z.object({ + phase: z.enum(["arming", "collecting", "stopped", "failed"]).describe("Background capture lifecycle phase"), + target: targetSchema, + sessionId: z.number().nullable().describe("Bound NST session ID, or null before binding/when identity lookup failed"), + summary: summarySchema, + truncated: z.boolean().describe("true when maxObservedWrites released the workload before full observation"), + warning: z.string().nullable().describe("Most recent redacted nonfatal or terminal warning"), +}); + +const reportSchema = z.object({ + target: targetSchema, + outcome: z.enum(["completed", "truncated", "failed"]).describe("Final capture outcome"), + stopReason: z.enum(["finished", "sessionDetached", "maxObservedWrites", "fatal"]).describe("Why collection ended"), + complete: z.boolean().describe("true only when every write observed in this capture window was classified without evidence loss"), + truncated: z.boolean().describe("true when the observed-write safety cap ended collection"), + summary: summarySchema, + writers: z.array(writerSchema).describe("Exact target-table writer groups"), + unresolved: z.array(unresolvedSchema).describe("Fail-closed groups that could not be attributed to a runtime table"), + warnings: z.array(z.string()).describe("Redacted lifecycle, source-trust, truncation, or classification warnings"), +}); + +function requireTriage(state: ServerState): RecordWriteCollector { + if (!state.recordWrites) { + throw new BcDevError( + "RECORD_WRITE_TRIAGE_NOT_ACTIVE", + "No active record-write triage — call bcdev_record_writes_start first", + "state", + ); + } + return state.recordWrites; +} + +function normalizeStart(params: Record): { + tableId: number; + includeTemporary: boolean; + changesDeployed: boolean; + maxObservedWrites: number; +} { + const tableId = params["tableId"]; + if (typeof tableId !== "number" || !Number.isSafeInteger(tableId) || tableId <= 0) { + throw new BcDevError("INVALID_ARGUMENT", "tableId must be a positive integer", "validation"); + } + const maxObservedWrites = params["maxObservedWrites"] ?? 500; + if ( + typeof maxObservedWrites !== "number" + || !Number.isInteger(maxObservedWrites) + || maxObservedWrites < 1 + || maxObservedWrites > 10000 + ) { + throw new BcDevError( + "INVALID_ARGUMENT", + "maxObservedWrites must be an integer from 1 through 10000", + "validation", + ); + } + if (params["includeTemporary"] !== undefined && typeof params["includeTemporary"] !== "boolean") { + throw new BcDevError("INVALID_ARGUMENT", "includeTemporary must be a boolean", "validation"); + } + if (params["changesDeployed"] !== undefined && typeof params["changesDeployed"] !== "boolean") { + throw new BcDevError("INVALID_ARGUMENT", "changesDeployed must be a boolean", "validation"); + } + return { + tableId, + includeTemporary: (params["includeTemporary"] as boolean | undefined) ?? false, + changesDeployed: (params["changesDeployed"] as boolean | undefined) ?? false, + maxObservedWrites, + }; +} + +export function createRecordWriteTools(state: ServerState, deps: ToolDeps): ToolDefinition[] { + return [ + { + name: "bcdev_record_writes_start", + title: "Start table write triage", + description: + "Arm background record-write triage for one numeric table ID. Business Central breaks globally; the collector identifies each runtime receiver, groups exact target stacks, and auto-continues. Trigger the matching workload after start, then check status and finish.", + annotations: { readOnlyHint: false, destructiveHint: false, idempotentHint: false, openWorldHint: true }, + schema: { + ...connectionShape, + tableId: z.number().int().positive().describe("Positive Business Central table ID to match exactly at runtime"), + breakOnNext: z + .enum(["WebClient", "WebServiceClient", "Background"]) + .optional() + .describe("Client type for default/user-filtered next-session attach (default WebClient); exact sessionId takes precedence"), + sessionId: z + .number() + .int() + .positive() + .optional() + .describe("Positive existing NST session ID; mutually exclusive with userId"), + userId: z + .string() + .trim() + .min(1) + .optional() + .describe("Business Central user ID whose next matching session should bind; mutually exclusive with sessionId"), + includeTemporary: z + .boolean() + .optional() + .describe("Include temporary-record writes (default false; false uses the proven nonTemporary debugger mode)"), + changesDeployed: z + .boolean() + .optional() + .describe("Caller assertion that local AL source is exactly deployed; required before local-source fallback can prove a match"), + maxObservedWrites: z + .number() + .int() + .min(1) + .max(10000) + .optional() + .describe("Safety cap across all observed writes (default 500); reaching it releases the workload undebugged"), + }, + outputSchema: z.object({ + armed: z.literal(true).describe("Debugger attach succeeded and background collection is armed"), + connectionId: z.string().nullable().describe("Debugger hub connection ID"), + target: targetSchema, + includeTemporary: z.boolean().describe("Whether temporary writes are included"), + changesDeployed: z.boolean().describe("Caller-provided local source deployment assertion"), + maxObservedWrites: z.number().describe("Effective observed-write safety cap"), + }), + handler: async (params) => { + state.assertDebugSlotAvailable(); + const target = normalizeDebugTarget(params); + const options = normalizeStart(params); + const slotToken = state.claimDebugSlot("recordWrites"); + const client = new DebuggerClient(deps.hubFactory); + let collector: RecordWriteCollector | null = null; + try { + const { config, authorization, project } = resolve(params, deps); + const index = await AlObjectIndex.build(project); + collector = new RecordWriteCollector({ + ...options, + client, + localSource: async (objectType, objectId) => { + const file = index.byId(objectType, objectId)?.file; + if (!file) return null; + return await readFile(file, "utf8").catch(() => null); + }, + localFile: (objectType, objectId) => index.byId(objectType, objectId)?.file, + }); + state.recordWrites = collector; + state.recordWriteSlotToken = slotToken; + client.onEvent = (event) => collector?.onEvent(event); + await client.connect(config, authorization, { + breakOnNext: params["breakOnNext"] as "WebClient" | "WebServiceClient" | "Background" | undefined, + ...target, + breakOnError: false, + breakOnRecordWrite: options.includeTemporary ? "all" : "nonTemporary", + skipSystemTriggers: false, + }); + return { + armed: true, + connectionId: client.connectionId, + target: { tableId: options.tableId, tableName: null }, + includeTemporary: options.includeTemporary, + changesDeployed: options.changesDeployed, + maxObservedWrites: options.maxObservedWrites, + }; + } catch (error) { + if (state.recordWrites === collector) state.recordWrites = null; + if (state.recordWriteSlotToken === slotToken) state.recordWriteSlotToken = null; + state.releaseDebugSlot(slotToken); + await client.stop().catch(() => {}); + throw error; + } + }, + }, + { + name: "bcdev_record_writes_status", + title: "Check table write triage", + description: + "Read background table-write triage progress. Collection and automatic continuation do not depend on polling.", + annotations: { readOnlyHint: true, openWorldHint: true }, + schema: {}, + outputSchema: statusSchema, + handler: async (): Promise => requireTriage(state).status(), + }, + { + name: "bcdev_record_writes_finish", + title: "Finish table write triage", + description: + "Finish the capture window, release a paused workload undebugged if necessary, close the debugger, and return grouped exact writers plus fail-closed unresolved evidence.", + annotations: { readOnlyHint: false, destructiveHint: false, idempotentHint: false, openWorldHint: true }, + schema: {}, + outputSchema: reportSchema, + handler: async (): Promise => { + const collector = requireTriage(state); + state.recordWrites = null; + const slotToken = state.recordWriteSlotToken; + state.recordWriteSlotToken = null; + try { + return await collector.finish(); + } finally { + if (slotToken) state.releaseDebugSlot(slotToken); + } + }, + }, + ]; +} diff --git a/src/mcp/tools/shared.ts b/src/mcp/tools/shared.ts index a5bc40d..0a7421b 100644 --- a/src/mcp/tools/shared.ts +++ b/src/mcp/tools/shared.ts @@ -34,6 +34,32 @@ export interface ToolDefinition { handler(params: Record): Promise; } +export interface DebugTarget { + sessionId?: number; + userId?: string; +} + +export function normalizeDebugTarget(params: Record): DebugTarget { + const sessionId = params["sessionId"]; + const userId = params["userId"]; + if (sessionId !== undefined && userId !== undefined) { + throw new BcDevError("INVALID_ARGUMENT", "sessionId and userId are mutually exclusive", "validation"); + } + if (sessionId !== undefined) { + if (typeof sessionId !== "number" || !Number.isInteger(sessionId) || sessionId <= 0) { + throw new BcDevError("INVALID_ARGUMENT", "sessionId must be a positive integer", "validation"); + } + return { sessionId }; + } + if (userId !== undefined) { + if (typeof userId !== "string" || userId.trim() === "") { + throw new BcDevError("INVALID_ARGUMENT", "userId must be a nonblank string", "validation"); + } + return { userId: userId.trim() }; + } + return {}; +} + export function claimTestRun(state: ServerState): void { if (state.testRunActive) { throw new BcDevError("TEST_RUN_ACTIVE", "A test run is already running — wait for it to finish", "state"); @@ -240,6 +266,16 @@ export const stackFrameSchema = z.looseObject({ methodName: z.string(), line: z.number().describe("1-based"), file: z.string().optional().describe("Local source file, when the object id maps to the project"), + statementSpan: z.object({ + from: z.object({ + line: z.number().describe("1-based first statement line"), + column: z.number().describe("1-based first statement column"), + }).describe("Start of the paused AL statement"), + to: z.object({ + line: z.number().describe("1-based last statement line"), + column: z.number().describe("1-based last statement column"), + }).describe("End of the paused AL statement"), + }).optional().describe("Server-reported AL statement span, when usable"), }); export const connectionShape = { @@ -304,6 +340,13 @@ export async function mapBreakpoints( } export function requireSession(state: ServerState): DebugSession { + if (state.debugOwner === "recordWrites") { + throw new BcDevError( + "RECORD_WRITE_TRIAGE_ACTIVE", + "Record-write triage is active — call bcdev_record_writes_status or bcdev_record_writes_finish first", + "state", + ); + } if (!state.debug) throw new BcDevError("NO_DEBUG_SESSION", "No debug session — call bcdev_debug_attach first", "state"); return state.debug; } diff --git a/tests/core/debugger-hub.test.ts b/tests/core/debugger-hub.test.ts index 439fe86..fca4d21 100644 --- a/tests/core/debugger-hub.test.ts +++ b/tests/core/debugger-hub.test.ts @@ -1,7 +1,7 @@ import { describe, expect, test } from "bun:test"; import { DebuggerClient, type DebugAttachOptions } from "../../src/core/hubs/debugger-hub"; import { BasicAuthorizationProvider } from "../../src/core/authorization"; -import type { ConnectionConfig, DebuggerEvent } from "../../src/core/types"; +import type { ConnectionConfig, DebuggerEvent, StackFrameInfo } from "../../src/core/types"; import { FakeHub, fakeHubFactory } from "../fakes/fake-hub"; const config: ConnectionConfig = { @@ -349,11 +349,48 @@ describe("DebuggerClient", () => { objectId: 50100, errorMessage: undefined, line: 13, - stack: [{ objectType: 5, objectId: 50100, objectName: "My Tests", methodName: "PostInvoice", line: 13 }], + stack: [{ + objectType: 5, + objectId: 50100, + objectName: "My Tests", + methodName: "PostInvoice", + line: 13, + statementSpan: { + from: { line: 13, column: 5 }, + to: { line: 13, column: 31 }, + }, + }], }, ]); }); + test("omits missing and all-zero statement spans without changing the legacy line fallback", async () => { + const hub = new FakeHub(); + const { events } = await connected(hub); + hub.emit( + "Break", + { ObjectType: 5, ObjectNumber: 50100 }, + [ + { + ObjectId: { ObjectType: 5, ObjectNumber: 50100 }, + ObjectName: "My Tests", + MethodName: "Missing", + }, + { + ObjectId: { ObjectType: 5, ObjectNumber: 50100 }, + ObjectName: "My Tests", + MethodName: "Unset", + StatementSpan: { From: { Line: 0, Column: 0 }, To: { Line: 0, Column: 0 } }, + }, + ], + "", + ); + expect((events[0] as { stack: StackFrameInfo[] }).stack).toEqual([ + { objectType: 5, objectId: 50100, objectName: "My Tests", methodName: "Missing", line: 0 }, + { objectType: 5, objectId: 50100, objectName: "My Tests", methodName: "Unset", line: 1 }, + ]); + }); + test("queues detach and fatal events", async () => { const hub = new FakeHub(); const { events } = await connected(hub); @@ -365,6 +402,25 @@ describe("DebuggerClient", () => { ]); }); + test("ignores break and fatal callbacks from a stopped hub", async () => { + const hub = new FakeHub(); + const { client, events } = await connected(hub); + await client.stop(); + hub.emit( + "Break", + { ObjectType: 5, ObjectNumber: 50100 }, + [{ + ObjectId: { ObjectType: 5, ObjectNumber: 50100 }, + ObjectName: "Old", + MethodName: "Run", + StatementSpan: { From: { Line: 1, Column: 1 }, To: { Line: 1, Column: 10 } }, + }], + "", + ); + hub.emit("OnFatalDebuggerException", "late fatal"); + expect(events).toEqual([]); + }); + test("addBreakpoint sends wire shape and returns id; step maps enum", async () => { const hub = new FakeHub(); hub.onInvoke = (method) => (method === "AddBreakpoint" ? { BreakpointId: 42 } : undefined); diff --git a/tests/core/record-write-triage.test.ts b/tests/core/record-write-triage.test.ts new file mode 100644 index 0000000..4d237d0 --- /dev/null +++ b/tests/core/record-write-triage.test.ts @@ -0,0 +1,485 @@ +import { describe, expect, test } from "bun:test"; +import { + inspectRecordWriteStatement, + parseRecordWriteStatement, + parseRuntimeTableType, + RecordWriteCollector, + sourceAtStatementSpan, +} from "../../src/core/record-write-triage"; +import type { DebuggerEvent, StackFrameInfo } from "../../src/core/types"; + +const SOURCE = [ + "codeunit 50100 Writer", + "{", + " procedure Run()", + " var", + " Customer: Record Customer;", + " begin", + " Customer.Modify(", + " true);", + " end;", + "}", +].join("\n"); + +const SPAN = { from: { line: 7, column: 9 }, to: { line: 8, column: 19 } }; + +function frame(line = 7): StackFrameInfo { + return { + objectType: 5, + objectId: 50100, + objectName: "Writer", + methodName: "Run", + line, + statementSpan: SPAN, + }; +} + +function brk(stack: StackFrameInfo[] = [frame()]): DebuggerEvent { + return { kind: "break", objectType: 5, objectId: 50100, line: 7, stack }; +} + +function client(typeNames: string[]) { + const steps: string[] = []; + const watchExpressions: string[] = []; + let sourceCalls = 0; + let stops = 0; + let watchIndex = 0; + return { + steps, + watchExpressions, + get sourceCalls() { + return sourceCalls; + }, + get stops() { + return stops; + }, + getSourceContent: async () => { + sourceCalls++; + return { content: SOURCE, isAlContent: true }; + }, + evalWatch: async (_frameId: number, expression: string) => { + watchExpressions.push(expression); + return { + name: expression, + typeName: typeNames[Math.min(watchIndex++, typeNames.length - 1)] ?? "", + summary: "", + hasChildren: true, + changeState: "unknown" as const, + changed: false, + }; + }, + step: async (action: string) => { + steps.push(action); + }, + stop: async () => { + stops++; + }, + }; +} + +describe("record-write statement parsing", () => { + test("extracts a multiline statement span", () => { + expect(sourceAtStatementSpan(SOURCE, SPAN)).toContain("Customer.Modify"); + expect(sourceAtStatementSpan(SOURCE, SPAN)).toContain("true)"); + }); + + test("recognizes every documented operation case-insensitively", () => { + expect(parseRecordWriteStatement("Customer.Insert(true);")).toEqual({ operation: "insert", receiver: "Customer" }); + expect(parseRecordWriteStatement("Customer.MODIFY(false);")).toEqual({ operation: "modify", receiver: "Customer" }); + expect(parseRecordWriteStatement("Customer.ModifyAll(Name, 'x', true);")).toEqual({ operation: "modifyAll", receiver: "Customer" }); + expect(parseRecordWriteStatement("Customer.Rename('10000');")).toEqual({ operation: "rename", receiver: "Customer" }); + expect(parseRecordWriteStatement("Customer.Delete(true);")).toEqual({ operation: "delete", receiver: "Customer" }); + expect(parseRecordWriteStatement("Customer.DeleteAll(false);")).toEqual({ operation: "deleteAll", receiver: "Customer" }); + }); + + test("handles quoted and implicit Rec receivers while ignoring comments and strings", () => { + expect(parseRecordWriteStatement('"Customer Buffer".Insert();')).toEqual({ + operation: "insert", + receiver: '"Customer Buffer"', + }); + expect(parseRecordWriteStatement("Modify(true);")).toEqual({ operation: "modify", receiver: "Rec" }); + expect(parseRecordWriteStatement("// Fake.Insert();\nCustomer.Delete();")).toEqual({ + operation: "delete", + receiver: "Customer", + }); + expect(parseRecordWriteStatement("Message('Fake.Insert()'); Customer.Rename('x');")).toEqual({ + operation: "rename", + receiver: "Customer", + }); + }); + + test("refuses ambiguous, indexed, function-return, and non-write expressions", () => { + expect(parseRecordWriteStatement("A.Insert(); B.Modify();")).toBeNull(); + expect(parseRecordWriteStatement("Records[1].Insert();")).toBeNull(); + expect(parseRecordWriteStatement("GetCustomer().Modify();")).toBeNull(); + expect(parseRecordWriteStatement("Customer.Validate(Name);")).toBeNull(); + expect(inspectRecordWriteStatement("Records[1].Insert();").reason).toBe("receiverUnsupported"); + expect(inspectRecordWriteStatement("GetCustomer().Modify();").reason).toBe("receiverUnsupported"); + expect(inspectRecordWriteStatement("A.Insert(); B.Modify();").reason).toBe("writeStatementUnrecognized"); + }); + + test("parses only positive runtime Table identities", () => { + expect(parseRuntimeTableType("Table Customer (18)")).toEqual({ tableId: 18, tableName: "Customer" }); + expect(parseRuntimeTableType(' Table "My Table" (50100) ')).toEqual({ tableId: 50100, tableName: '"My Table"' }); + expect(parseRuntimeTableType("Record Customer (18)")).toBeNull(); + expect(parseRuntimeTableType("Table Customer (0)")).toBeNull(); + expect(parseRuntimeTableType("Table Customer")).toBeNull(); + }); +}); + +describe("RecordWriteCollector", () => { + test("classifies, groups, and automatically continues target and unrelated writes", async () => { + const fake = client(["Table Customer (18)", "Table Vendor (23)", "Table Customer (18)"]); + const collector = new RecordWriteCollector({ + tableId: 18, + includeTemporary: false, + changesDeployed: false, + maxObservedWrites: 10, + client: fake as never, + localSource: async () => null, + localFile: () => "/project/Writer.Codeunit.al", + }); + collector.onEvent({ kind: "sessionBound", sessionId: 99, hostId: null }); + collector.onEvent(brk()); + collector.onEvent(brk()); + collector.onEvent(brk()); + await collector.waitForIdle(); + + expect(collector.status()).toMatchObject({ + phase: "collecting", + sessionId: 99, + target: { tableId: 18, tableName: "Customer" }, + summary: { + observedWrites: 3, + matchedWrites: 2, + uniqueWriters: 1, + unrelatedWrites: 1, + unresolvedWrites: 0, + }, + }); + expect(fake.steps).toEqual(["continue", "continue", "continue"]); + const report = await collector.finish(); + expect(report.complete).toBe(true); + expect(report.writers).toHaveLength(1); + expect(report.writers[0]).toMatchObject({ + operation: "modify", + receiver: "Customer", + count: 2, + firstSequence: 1, + lastSequence: 3, + source: "deployed", + }); + expect(report.writers[0]?.stack[0]?.file).toBe("/project/Writer.Codeunit.al"); + expect(fake.sourceCalls).toBe(1); + expect(fake.watchExpressions).toEqual(["Customer", "Customer", "Customer"]); + }); + + test("unresolved writes fail closed but still continue", async () => { + const fake = client(["Integer"]); + const collector = new RecordWriteCollector({ + tableId: 18, + includeTemporary: false, + changesDeployed: false, + maxObservedWrites: 10, + client: fake as never, + localSource: async () => null, + localFile: () => undefined, + }); + collector.onEvent(brk()); + await collector.waitForIdle(); + expect(fake.steps).toEqual(["continue"]); + const report = await collector.finish(); + expect(report.complete).toBe(false); + expect(report.summary.unresolvedWrites).toBe(1); + expect(report.unresolved[0]?.reason).toBe("receiverTypeUnresolved"); + }); + + test("finishing before any session binds cannot produce a false complete report", async () => { + const fake = client(["Table Customer (18)"]); + const collector = new RecordWriteCollector({ + tableId: 18, + includeTemporary: false, + changesDeployed: false, + maxObservedWrites: 10, + client: fake as never, + localSource: async () => null, + localFile: () => undefined, + }); + const report = await collector.finish(); + expect(report).toMatchObject({ + outcome: "completed", + stopReason: "finished", + complete: false, + summary: { observedWrites: 0 }, + }); + expect(report.warnings).toContain("No debugger session bound during the capture window."); + }); + + test("local source is exact only with changesDeployed assertion", async () => { + const make = (changesDeployed: boolean) => { + const fake = client(["Table Customer (18)"]); + fake.getSourceContent = async () => ({ content: "", isAlContent: false }); + return { + fake, + collector: new RecordWriteCollector({ + tableId: 18, + includeTemporary: false, + changesDeployed, + maxObservedWrites: 10, + client: fake as never, + localSource: async () => SOURCE, + localFile: () => "/project/Writer.Codeunit.al", + }), + }; + }; + const unasserted = make(false); + unasserted.collector.onEvent(brk()); + await unasserted.collector.waitForIdle(); + expect((await unasserted.collector.finish()).unresolved[0]?.reason).toBe("sourceUnavailable"); + + const asserted = make(true); + asserted.collector.onEvent(brk()); + await asserted.collector.waitForIdle(); + const report = await asserted.collector.finish(); + expect(report.complete).toBe(true); + expect(report.writers[0]?.source).toBe("localAsserted"); + }); + + test("the cap classifies its final event then releases once", async () => { + const fake = client(["Table Customer (18)", "Table Customer (18)"]); + const collector = new RecordWriteCollector({ + tableId: 18, + includeTemporary: false, + changesDeployed: false, + maxObservedWrites: 2, + client: fake as never, + localSource: async () => null, + localFile: () => undefined, + }); + collector.onEvent(brk()); + collector.onEvent(brk()); + collector.onEvent(brk()); + await collector.waitForIdle(); + expect(fake.steps).toEqual(["continue", "release"]); + const report = await collector.finish(); + expect(report).toMatchObject({ + outcome: "truncated", + stopReason: "maxObservedWrites", + complete: false, + truncated: true, + summary: { observedWrites: 2, matchedWrites: 2 }, + }); + }); + + test("different stacks remain separate", async () => { + const fake = client(["Table Customer (18)", "Table Customer (18)"]); + const collector = new RecordWriteCollector({ + tableId: 18, + includeTemporary: false, + changesDeployed: false, + maxObservedWrites: 10, + client: fake as never, + localSource: async () => null, + localFile: () => undefined, + }); + collector.onEvent(brk()); + collector.onEvent(brk([{ ...frame(20), methodName: "Other", statementSpan: SPAN }])); + await collector.waitForIdle(); + expect((await collector.finish()).writers).toHaveLength(2); + }); + + test("groups case-insensitive AL receiver spellings under the first source spelling", async () => { + const caseLine = " if Flag then Customer.Modify(true) else customer.Modify(true);"; + const caseSource = SOURCE.replace(" Customer.Modify(", `${caseLine}\n //`).replace( + " true);", + "", + ); + const firstStart = caseLine.indexOf("Customer") + 1; + const secondStart = caseLine.indexOf("customer") + 1; + const fake = client(["Table Customer (18)", "Table Customer (18)"]); + fake.getSourceContent = async () => ({ content: caseSource, isAlContent: true }); + const collector = new RecordWriteCollector({ + tableId: 18, + includeTemporary: false, + changesDeployed: false, + maxObservedWrites: 10, + client: fake as never, + localSource: async () => null, + localFile: () => undefined, + }); + collector.onEvent(brk([{ + ...frame(), + statementSpan: { + from: { line: 7, column: firstStart }, + to: { line: 7, column: firstStart + "Customer.Modify(true)".length }, + }, + }])); + collector.onEvent(brk([{ + ...frame(), + statementSpan: { + from: { line: 7, column: secondStart }, + to: { line: 7, column: secondStart + "customer.Modify(true)".length }, + }, + }])); + await collector.waitForIdle(); + const report = await collector.finish(); + expect(report.writers).toHaveLength(1); + expect(report.writers[0]).toMatchObject({ receiver: "Customer", count: 2 }); + }); + + test("serializes rapid breaks and finish drains accepted work but rejects late events", async () => { + let resolveWatch!: (value: { + name: string; + typeName: string; + summary: string; + hasChildren: boolean; + changeState: "unknown"; + changed: false; + }) => void; + const watch = new Promise<{ + name: string; + typeName: string; + summary: string; + hasChildren: boolean; + changeState: "unknown"; + changed: false; + }>((resolve) => { + resolveWatch = resolve; + }); + let watchCalls = 0; + const fake = client(["Table Customer (18)"]); + fake.evalWatch = async () => { + watchCalls++; + return await watch; + }; + const collector = new RecordWriteCollector({ + tableId: 18, + includeTemporary: false, + changesDeployed: false, + maxObservedWrites: 10, + client: fake as never, + localSource: async () => null, + localFile: () => undefined, + }); + collector.onEvent(brk()); + await Bun.sleep(0); + expect(watchCalls).toBe(1); + + const finishing = collector.finish(); + collector.onEvent(brk()); + resolveWatch({ + name: "Customer", + typeName: "Table Customer (18)", + summary: "", + hasChildren: true, + changeState: "unknown", + changed: false, + }); + const report = await finishing; + expect(report.summary).toMatchObject({ observedWrites: 1, matchedWrites: 1 }); + expect(fake.steps).toEqual(["continue"]); + }); + + test("inspection failures are redacted, fail closed, and continue automatically", async () => { + const fake = client(["Table Customer (18)"]); + fake.evalWatch = async () => { + throw new Error("Authorization: Bearer super-secret"); + }; + const collector = new RecordWriteCollector({ + tableId: 18, + includeTemporary: false, + changesDeployed: false, + maxObservedWrites: 10, + client: fake as never, + localSource: async () => null, + localFile: () => undefined, + }); + collector.onEvent(brk()); + await collector.waitForIdle(); + const report = await collector.finish(); + expect(fake.steps).toEqual(["continue"]); + expect(report.complete).toBe(false); + expect(report.unresolved[0]?.reason).toBe("inspectionFailed"); + expect(report.warnings.join(" ")).not.toContain("super-secret"); + expect(report.warnings.join(" ")).toContain("[REDACTED]"); + }); + + test("detach and fatal events preserve retrievable partial reports", async () => { + const detachedFake = client(["Table Customer (18)"]); + const detached = new RecordWriteCollector({ + tableId: 18, + includeTemporary: false, + changesDeployed: false, + maxObservedWrites: 10, + client: detachedFake as never, + localSource: async () => null, + localFile: () => undefined, + }); + detached.onEvent(brk()); + detached.onEvent({ kind: "detached", terminateSession: false }); + await detached.waitForIdle(); + expect(await detached.finish()).toMatchObject({ + outcome: "completed", + stopReason: "sessionDetached", + summary: { observedWrites: 1, matchedWrites: 1 }, + }); + + const fatalFake = client(["Table Customer (18)"]); + const fatal = new RecordWriteCollector({ + tableId: 18, + includeTemporary: false, + changesDeployed: false, + maxObservedWrites: 10, + client: fatalFake as never, + localSource: async () => null, + localFile: () => undefined, + }); + fatal.onEvent({ kind: "fatal", message: "Authorization: Bearer fatal-secret" }); + await fatal.waitForIdle(); + const failed = await fatal.finish(); + expect(failed).toMatchObject({ outcome: "failed", stopReason: "fatal", complete: false }); + expect(failed.warnings.join(" ")).not.toContain("fatal-secret"); + }); + + test("a continuation failure becomes fatal and attempts to release the paused workload", async () => { + const fake = client(["Table Customer (18)"]); + fake.step = async (action: string) => { + fake.steps.push(action); + if (action === "continue") throw new Error("resume failed"); + }; + const collector = new RecordWriteCollector({ + tableId: 18, + includeTemporary: false, + changesDeployed: false, + maxObservedWrites: 10, + client: fake as never, + localSource: async () => null, + localFile: () => undefined, + }); + collector.onEvent(brk()); + await collector.waitForIdle(); + const report = await collector.finish(); + expect(fake.steps).toEqual(["continue", "release"]); + expect(fake.stops).toBeGreaterThan(0); + expect(report).toMatchObject({ outcome: "failed", stopReason: "fatal", complete: false }); + }); + + test("warns when one numeric target is reported with conflicting runtime names", async () => { + const fake = client(["Table Customer (18)", 'Table "Customer Alias" (18)']); + const collector = new RecordWriteCollector({ + tableId: 18, + includeTemporary: false, + changesDeployed: false, + maxObservedWrites: 10, + client: fake as never, + localSource: async () => null, + localFile: () => undefined, + }); + collector.onEvent(brk()); + collector.onEvent(brk()); + await collector.waitForIdle(); + const report = await collector.finish(); + expect(report.target.tableName).toBe("Customer"); + expect(report.warnings.join(" ")).toContain("Customer Alias"); + }); +}); diff --git a/tests/mcp/agent-response.test.ts b/tests/mcp/agent-response.test.ts index 99e625e..6dc663f 100644 --- a/tests/mcp/agent-response.test.ts +++ b/tests/mcp/agent-response.test.ts @@ -37,4 +37,30 @@ describe("withAgentResponses", () => { expect(result.nextSteps[0]).toContain("al-perf"); expect(result.nextSteps[1]).toContain("deterministic call-time"); }); + + test("record-write guidance follows arming, collection, truncation, and incomplete states", async () => { + const status = withAgentResponses(tool({ + name: "bcdev_record_writes_status", + handler: async () => ({ phase: "arming" }), + })); + expect((await status.handler({}) as { nextSteps: string[] }).nextSteps.join(" ")).toContain("Trigger"); + + const collecting = withAgentResponses(tool({ + name: "bcdev_record_writes_status", + handler: async () => ({ phase: "collecting" }), + })); + expect((await collecting.handler({}) as { nextSteps: string[] }).nextSteps.join(" ")).toContain("finish"); + + const truncated = withAgentResponses(tool({ + name: "bcdev_record_writes_finish", + handler: async () => ({ truncated: true, complete: false, summary: {} }), + })); + expect((await truncated.handler({}) as { nextSteps: string[] }).nextSteps.join(" ")).toContain("maxObservedWrites"); + + const incomplete = withAgentResponses(tool({ + name: "bcdev_record_writes_finish", + handler: async () => ({ truncated: false, complete: false, summary: {} }), + })); + expect((await incomplete.handler({}) as { nextSteps: string[] }).nextSteps.join(" ")).toContain("unresolved"); + }); }); diff --git a/tests/mcp/record-write-tools.test.ts b/tests/mcp/record-write-tools.test.ts new file mode 100644 index 0000000..8d2a841 --- /dev/null +++ b/tests/mcp/record-write-tools.test.ts @@ -0,0 +1,289 @@ +import { describe, expect, test } from "bun:test"; +import { mkdtempSync, mkdirSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { createAuthorizationProviderFactory } from "../../src/core/authorization"; +import type { GitChangeSet } from "../../src/core/git-changes"; +import { ServerState } from "../../src/mcp/state"; +import { createTools, type ToolDeps } from "../../src/mcp/tools"; +import { FakeHub, fakeHubFactory } from "../fakes/fake-hub"; + +const SOURCE = [ + "codeunit 50100 Writer", + "{", + " procedure Run()", + " var", + " Customer: Record Customer;", + " begin", + " Customer.Modify(true);", + " end;", + "}", +].join("\n"); + +function project(): string { + const dir = mkdtempSync(join(tmpdir(), "bcmcp-record-writes-")); + mkdirSync(join(dir, ".vscode")); + writeFileSync( + join(dir, ".vscode", "launch.json"), + JSON.stringify({ configurations: [{ type: "al", request: "launch", server: "http://localhost", serverInstance: "BC" }] }), + ); + writeFileSync(join(dir, "app.json"), JSON.stringify({ runtime: "16.0" })); + writeFileSync(join(dir, "Writer.Codeunit.al"), SOURCE); + return dir; +} + +function setup(hub: FakeHub) { + const state = new ServerState(); + const deps: ToolDeps = { + hubFactory: fakeHubFactory(hub), + authorizationFactory: createAuthorizationProviderFactory(), + fetchFn: (async () => new Response(JSON.stringify({ WebApiVersion: "7.0" }))) as unknown as typeof fetch, + env: { BC_DEV_USER: "u", BC_DEV_PASSWORD: "p" }, + cwd: project(), + gitChanges: async (_project, baseRef): Promise => ({ + baseRef, + mergeBase: "a".repeat(40), + head: "workingTree", + files: [], + }), + }; + return { + state, + tools: new Map(createTools(state, deps).map((tool) => [tool.name, tool])), + }; +} + +function emitWrite(hub: FakeHub): void { + hub.emit( + "Break", + { ObjectType: 5, ObjectNumber: 50100 }, + [{ + ObjectId: { ObjectType: 5, ObjectNumber: 50100 }, + ObjectName: "Writer", + MethodName: "Run", + StatementSpan: { + From: { Line: 6, Column: 8 }, + To: { Line: 6, Column: 30 }, + }, + }], + "", + ); +} + +describe("record-write MCP tools", () => { + test("start configures fail-closed triage, collects in background, and finish returns a valid grouped report", async () => { + const hub = new FakeHub(); + hub.onInvoke = (method) => { + if (method === "GetNstSessionInfo") return { SessionId: 321, HostId: null }; + if (method === "GetSourceContent") return { Content: SOURCE, IsALContent: true }; + if (method === "GetWatchNode") { + return { + Name: "Customer", + TypeName: "Table Customer (18)", + Summary: "'10000'", + HasChildren: true, + }; + } + return undefined; + }; + const { state, tools } = setup(hub); + const start = await tools.get("bcdev_record_writes_start")!.handler({ + tableId: 18, + sessionId: 321, + }) as { nextSteps: string[] }; + expect(start.nextSteps.join(" ")).toContain("Trigger"); + expect(() => tools.get("bcdev_record_writes_start")!.outputSchema.parse(start)).not.toThrow(); + expect(state.debugOwner).toBe("recordWrites"); + const arming = await tools.get("bcdev_record_writes_status")!.handler({}) as { + phase: string; + nextSteps: string[]; + }; + expect(arming.phase).toBe("arming"); + expect(arming.nextSteps.join(" ")).toContain("Trigger"); + expect(() => tools.get("bcdev_record_writes_status")!.outputSchema.parse(arming)).not.toThrow(); + + hub.emit("HubConnected"); + await Bun.sleep(0); + const config = hub.invoked("DebugAdapterConfigurationDone")[0]?.args[0]; + expect(config).toMatchObject({ + BreakOnError: false, + BreakOnErrorBehaviour: 1, + BreakOnRecordWrite: true, + BreakOnRecordWriteBehaviour: 3, + SkipSystemTriggers: false, + }); + + emitWrite(hub); + await Bun.sleep(0); + await Bun.sleep(0); + expect(hub.invoked("SetBreakpointResponse").map((call) => call.args)).toEqual([[0]]); + + const status = await tools.get("bcdev_record_writes_status")!.handler({}) as { + phase: string; + summary: { matchedWrites: number }; + nextSteps: string[]; + }; + expect(status).toMatchObject({ phase: "collecting", summary: { matchedWrites: 1 } }); + expect(status.nextSteps.join(" ")).toContain("finish"); + + const finishTool = tools.get("bcdev_record_writes_finish")!; + const report = await finishTool.handler({}) as { + complete: boolean; + summary: { matchedWrites: number; uniqueWriters: number }; + writers: Array<{ operation: string; count: number }>; + nextSteps: string[]; + }; + expect(report).toMatchObject({ + complete: true, + summary: { matchedWrites: 1, uniqueWriters: 1 }, + writers: [{ operation: "modify", count: 1 }], + }); + expect(report.nextSteps.join(" ")).toContain("grouped writer stacks"); + expect(() => finishTool.outputSchema.parse(report)).not.toThrow(); + expect(state.debugOwner).toBeNull(); + expect(state.recordWrites).toBeNull(); + }); + + test("default and temporary-inclusive modes send the proven record-write wire enum", async () => { + for (const [includeTemporary, expected] of [[false, 3], [true, 2]] as const) { + const hub = new FakeHub(); + const { tools } = setup(hub); + await tools.get("bcdev_record_writes_start")!.handler({ + tableId: 18, + includeTemporary, + }); + hub.emit("HubConnected"); + await Bun.sleep(0); + expect(hub.invoked("DebugAdapterConfigurationDone")[0]?.args[0]).toMatchObject({ + BreakOnRecordWrite: true, + BreakOnRecordWriteBehaviour: expected, + }); + await tools.get("bcdev_record_writes_finish")!.handler({}); + } + }); + + test("validates target and cap before claiming the slot or opening a hub", async () => { + const hub = new FakeHub(); + const { state, tools } = setup(hub); + for (const params of [ + { tableId: 0 }, + { tableId: 18, maxObservedWrites: 0 }, + { tableId: 18, maxObservedWrites: 10001 }, + { tableId: 18, sessionId: 1, userId: "u" }, + ]) { + await expect(tools.get("bcdev_record_writes_start")!.handler(params)).rejects.toThrow(); + expect(state.debugOwner).toBeNull(); + } + expect(hub.started).toBe(false); + }); + + test("triage and manual debugger entry points share one atomic slot", async () => { + const hub = new FakeHub(); + const { state, tools } = setup(hub); + const first = tools.get("bcdev_record_writes_start")!.handler({ tableId: 18 }); + await expect(tools.get("bcdev_record_writes_start")!.handler({ tableId: 18 })).rejects.toThrow( + /Record-write triage is active/, + ); + await first; + await expect(tools.get("bcdev_debug_attach")!.handler({})).rejects.toThrow(/Record-write triage is active/); + await expect(tools.get("bcdev_debug_wait")!.handler({ timeoutMs: 1 })).rejects.toThrow(/Record-write triage is active/); + await tools.get("bcdev_record_writes_finish")!.handler({}); + expect(state.debugOwner).toBeNull(); + + const firstManual = tools.get("bcdev_debug_attach")!.handler({}); + await expect(tools.get("bcdev_debug_attach")!.handler({})).rejects.toThrow(/Debug session already active/); + await firstManual; + await expect(tools.get("bcdev_record_writes_start")!.handler({ tableId: 18 })).rejects.toThrow( + /Debug session already active/, + ); + await tools.get("bcdev_debug_detach")!.handler({}); + }); + + test("setup failure rolls back the slot and finish/status require an active capture", async () => { + const hub = new FakeHub(); + hub.onInvoke = (method) => { + if (method === "Attach") throw new Error("attach failed"); + return undefined; + }; + const { state, tools } = setup(hub); + await expect(tools.get("bcdev_record_writes_start")!.handler({ tableId: 18 })).rejects.toThrow(/attach failed/); + expect(state.debugOwner).toBeNull(); + expect(state.recordWrites).toBeNull(); + await expect(tools.get("bcdev_record_writes_status")!.handler({})).rejects.toThrow(/No active record-write triage/); + await expect(tools.get("bcdev_record_writes_finish")!.handler({})).rejects.toThrow(/No active record-write triage/); + + hub.onInvoke = undefined; + await tools.get("bcdev_record_writes_start")!.handler({ tableId: 18 }); + expect(state.debugOwner).toBe("recordWrites"); + await tools.get("bcdev_record_writes_finish")!.handler({}); + }); + + test("cap release returns truncated guidance and clears the slot on finish", async () => { + const hub = new FakeHub(); + hub.onInvoke = (method) => { + if (method === "GetSourceContent") return { Content: SOURCE, IsALContent: true }; + if (method === "GetWatchNode") return { Name: "Customer", TypeName: "Table Customer (18)", Summary: "", HasChildren: true }; + return undefined; + }; + const { state, tools } = setup(hub); + await tools.get("bcdev_record_writes_start")!.handler({ tableId: 18, maxObservedWrites: 1 }); + emitWrite(hub); + await Bun.sleep(0); + await Bun.sleep(0); + expect(hub.invoked("SetBreakpointResponse").map((call) => call.args)).toEqual([[4]]); + const report = await tools.get("bcdev_record_writes_finish")!.handler({}) as { + truncated: boolean; + complete: boolean; + nextSteps: string[]; + }; + expect(report).toMatchObject({ truncated: true, complete: false }); + expect(report.nextSteps.join(" ")).toContain("maxObservedWrites"); + expect(state.debugOwner).toBeNull(); + }); + + test("a session identity lookup warning is nonfatal and retained in status", async () => { + const hub = new FakeHub(); + hub.onInvoke = (method) => { + if (method === "GetNstSessionInfo") return {}; + return undefined; + }; + const { tools } = setup(hub); + await tools.get("bcdev_record_writes_start")!.handler({ tableId: 18 }); + hub.emit("HubConnected"); + await Bun.sleep(0); + const status = await tools.get("bcdev_record_writes_status")!.handler({}) as { + phase: string; + sessionId: number | null; + warning: string | null; + }; + expect(status).toMatchObject({ phase: "collecting", sessionId: null }); + expect(status.warning).toContain("identity could not be read"); + await tools.get("bcdev_record_writes_finish")!.handler({}); + }); + + test("an out-of-band user-filter rejection fails closed and preserves a partial report", async () => { + const hub = new FakeHub(); + const { state, tools } = setup(hub); + const start = await tools.get("bcdev_record_writes_start")!.handler({ + tableId: 18, + userId: "missing-user", + }) as { armed: boolean }; + expect(start.armed).toBe(true); + + hub.emit("OnFatalDebuggerException", "The user specified cannot be found on the tenant."); + await Bun.sleep(0); + await Bun.sleep(0); + const status = await tools.get("bcdev_record_writes_status")!.handler({}) as { + phase: string; + warning: string | null; + }; + expect(status.phase).toBe("failed"); + expect(status.warning).toContain("Unable to bind the requested user-filtered session"); + const report = await tools.get("bcdev_record_writes_finish")!.handler({}) as { + outcome: string; + complete: boolean; + }; + expect(report).toMatchObject({ outcome: "failed", complete: false }); + expect(state.debugOwner).toBeNull(); + }); +}); diff --git a/tests/mcp/server.test.ts b/tests/mcp/server.test.ts index d87ce82..4880f01 100644 --- a/tests/mcp/server.test.ts +++ b/tests/mcp/server.test.ts @@ -46,7 +46,7 @@ describe("server wiring", () => { test("tools/list exposes names, titles, annotations, schemas", async () => { const client = await connect(); const { tools } = await client.listTools(); - expect(tools).toHaveLength(17); + expect(tools).toHaveLength(20); const status = tools.find((t) => t.name === "bcdev_status")!; expect(status.title).toBe("BC server status"); expect(status.annotations?.readOnlyHint).toBe(true); @@ -65,6 +65,10 @@ describe("server wiring", () => { expect(JSON.stringify(attachProperties["breakOnRecordWrite"])).toContain("nonTemporary"); const wait = tools.find((t) => t.name === "bcdev_debug_wait")!; expect(JSON.stringify(wait.outputSchema)).toContain("sessionBound"); + const recordWrites = tools.find((t) => t.name === "bcdev_record_writes_start")!; + expect(JSON.stringify(recordWrites.inputSchema)).toContain("maxObservedWrites"); + expect(JSON.stringify(recordWrites.inputSchema)).toContain("changesDeployed"); + expect(JSON.stringify(recordWrites.inputSchema)).toContain("tableId"); for (const t of tools) { expect(t.outputSchema, `${t.name} outputSchema`).toBeDefined(); const properties = (t.outputSchema as { properties?: Record }).properties; @@ -158,7 +162,7 @@ describe("server wiring", () => { test("start exposes the kind:instrumentation option", async () => { const client = await connect(); const { tools } = await client.listTools(); - expect(tools).toHaveLength(17); + expect(tools).toHaveLength(20); const start = tools.find((t) => t.name === "bcdev_profile_start")!; expect(JSON.stringify(start.inputSchema)).toContain("instrumentation"); }); diff --git a/tests/mcp/state.test.ts b/tests/mcp/state.test.ts index d800903..56276f5 100644 --- a/tests/mcp/state.test.ts +++ b/tests/mcp/state.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import { DebugSession } from "../../src/mcp/state"; +import { DebugSession, ServerState } from "../../src/mcp/state"; import type { DebuggerEvent } from "../../src/core/types"; const brk: DebuggerEvent = { kind: "break", objectType: 5, objectId: 1, stack: [] }; @@ -48,3 +48,22 @@ describe("DebugSession queue", () => { expect(s.droppedEvents).toBe(5); }); }); + +describe("ServerState debugger ownership", () => { + test("claims one owner atomically and releases only the matching token", () => { + const state = new ServerState(); + const token = state.claimDebugSlot("manual"); + expect(state.debugOwner).toBe("manual"); + expect(() => state.claimDebugSlot("recordWrites")).toThrow(/Debug session already active/); + state.releaseDebugSlot(Symbol("wrong")); + expect(state.debugOwner).toBe("manual"); + state.releaseDebugSlot(token); + expect(state.debugOwner).toBeNull(); + }); + + test("record-write ownership produces the dedicated state error", () => { + const state = new ServerState(); + state.claimDebugSlot("recordWrites"); + expect(() => state.claimDebugSlot("manual")).toThrow(/Record-write triage is active/); + }); +}); diff --git a/tests/mcp/tools.test.ts b/tests/mcp/tools.test.ts index d8a0112..29b7b20 100644 --- a/tests/mcp/tools.test.ts +++ b/tests/mcp/tools.test.ts @@ -64,7 +64,7 @@ describe("tools", () => { hub = new FakeHub(); }); - test("registers all 17 tools", () => { + test("registers all 20 tools", () => { const { tools } = setup(hub); expect([...tools.keys()].sort()).toEqual([ "bcdev_debug_attach", @@ -80,6 +80,9 @@ describe("tools", () => { "bcdev_profile_poll", "bcdev_profile_start", "bcdev_profile_status", + "bcdev_record_writes_finish", + "bcdev_record_writes_start", + "bcdev_record_writes_status", "bcdev_source", "bcdev_status", "bcdev_test_discover", From bec943ebb864474557e32910ef4298ecc920276e Mon Sep 17 00:00:00 2001 From: ianray <193674599+ianrayianray@users.noreply.github.com> Date: Sun, 26 Jul 2026 10:43:20 -0700 Subject: [PATCH 2/3] fix: close record-write triage edge cases --- README.md | 2 +- ...break-on-record-write-triage-2026-07-25.md | 19 ++ scripts/e2e.md | 3 + src/core/hubs/debugger-hub.ts | 45 ++- src/core/record-write-triage.ts | 276 ++++++++++++++++-- tests/core/debugger-hub.test.ts | 45 ++- tests/core/record-write-triage.test.ts | 155 +++++++++- tests/mcp/record-write-tools.test.ts | 51 ++++ 8 files changed, 559 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index 0f18595..5ce7e46 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ MCP server for Business Central AL development: run tests (with code coverage) a [![Bun](https://img.shields.io/badge/bun-1.x-black)](https://bun.sh) [![TypeScript](https://img.shields.io/badge/typescript-strict-blue)](https://typescriptlang.org) [![BC dev API](https://img.shields.io/badge/BC%20dev%20API-%E2%89%A57.0-purple)]() -[![Tests](https://img.shields.io/badge/tests-585%20passing-green)]() +[![Tests](https://img.shields.io/badge/tests-594%20passing-green)]() ## Overview diff --git a/scripts/e2e-break-on-record-write-triage-2026-07-25.md b/scripts/e2e-break-on-record-write-triage-2026-07-25.md index 911077d..096bfbb 100644 --- a/scripts/e2e-break-on-record-write-triage-2026-07-25.md +++ b/scripts/e2e-break-on-record-write-triage-2026-07-25.md @@ -36,5 +36,24 @@ harness made no manual debugger wait or continue call. - Finish cleared the shared debugger owner. An immediate manual attach/detach cycle succeeded, confirming slot reuse. +## Correction regression + +The disposable fixture was republished as version 1.0.0.5 after the edge-case review. +A table method whose implicit `Rec` was a different table executed an explicit +`with Target` block containing parenthesis-free `Insert`, `Modify`, `Delete`, and +`DeleteAll` calls. + +- The targeted capture completed with every writer receiver reported as `Target`, not + the host table's `Rec`. +- All four parenthesis-free operations were classified from deployed source with zero + unresolved writes. +- The original full acceptance matrix was rerun unchanged and retained its 13 observed, + 10 matched, 9 grouped, 3 unrelated, and 0 unresolved result. +- Configuration rejection, unexpected clean connection closure, and a Break callback + delivered asynchronously during finish are deterministic fake-hub fault injections; + they were not induced against the live Sandbox. The asynchronous finish test verifies + both accepted and rejected release-barrier responses without dropping the Break or + sending a duplicate release. + The fixture and harness lived outside the repository and are not part of the feature diff. diff --git a/scripts/e2e.md b/scripts/e2e.md index 5c60500..d5df411 100644 --- a/scripts/e2e.md +++ b/scripts/e2e.md @@ -59,6 +59,9 @@ field values, or raw source. Evidence belongs in - [x] The unasserted local-source fallback cannot produce an exact group; `changesDeployed:true` permits it only after the exact fixture has been published. - [x] `maxObservedWrites` classifies the cap event, sends `release`, and returns `truncated:true`, `complete:false` without leaving the workload paused. - [x] Finish clears the triage owner and an immediate manual attach or second triage start can claim the debugger slot. +- [x] An explicit `with Target` inside a different table's method resolves unqualified writes to `Target`, and valid parenthesis-free `Insert`, `Modify`, `Delete`, and `DeleteAll` calls classify exactly. +- [x] Finish orders an in-flight asynchronous Break through a best-effort `ReleaseConnection` invocation before closing intake, classifies the callback, and releases exactly once whether the barrier is accepted or rejected. +- [x] Rejected debugger configuration and an unexpected clean hub close fail the retained report closed rather than returning a complete zero-write result. - [x] No Production or on-premises call is made during this feature's acceptance run. ## Targeted debugger attach (Sandbox) diff --git a/src/core/hubs/debugger-hub.ts b/src/core/hubs/debugger-hub.ts index a0b53f0..d4b5337 100644 --- a/src/core/hubs/debugger-hub.ts +++ b/src/core/hubs/debugger-hub.ts @@ -297,7 +297,13 @@ export class DebuggerClient { hub.onclose((err) => { if (this.hub !== hub) return; this.hub = null; - if (err) this.onEvent?.({ kind: "fatal", message: `Hub connection closed: ${String(err)}` }); + this.pendingDebugOptions = null; + this.onEvent?.({ + kind: "fatal", + message: err + ? `Debugger hub connection closed unexpectedly: ${errorDetail(err)}` + : "Debugger hub connection closed unexpectedly", + }); }); // Server-invoked notifications we don't consume — registered to keep the connection log clean (live E2E 2026-07-03). hub.on("LogServerMessage", () => {}); @@ -384,7 +390,14 @@ export class DebuggerClient { } private async finishSessionBinding(hub: HubProxy, options: Record | null): Promise { - if (options) await hub.invoke("DebugAdapterConfigurationDone", options).catch(() => {}); + if (options) { + try { + await hub.invoke("DebugAdapterConfigurationDone", options); + } catch (error) { + await this.failBoundConfiguration(hub, error); + return; + } + } if (this.hub !== hub) return; if (this.userAttachFatal !== null || this.userAttachFailed) { if (this.userAttachFatal !== null) await this.failUserAttach(hub, this.userAttachFatal); @@ -411,6 +424,18 @@ export class DebuggerClient { } } + private async failBoundConfiguration(hub: HubProxy, error: unknown): Promise { + if (this.hub !== hub) return; + this.hub = null; + this.pendingDebugOptions = null; + this.onEvent?.({ + kind: "fatal", + message: `Debugger configuration failed: ${errorDetail(error)}`, + }); + await hub.invoke("StopDebugging").catch(() => {}); + await hub.stop().catch(() => {}); + } + async getSourceContent(objectType: number, objectId: number): Promise<{ content: string; isAlContent: boolean }> { // WIRE: GetSourceContent(ApplicationObjectIdWrapper) -> SourceContent{Content, IsALContent}; // requires DebuggerVersion.Major > 1 (esp-decomp HubBasedDebuggerService.GetSourceAsync). Validated live 2026-07-04. @@ -482,6 +507,22 @@ export class DebuggerClient { await this.requireHub().invoke("SetBreakpointResponse", STEP_WIRE[action]); } + async releaseForShutdown(): Promise { + const hub = this.hub; + if (!hub) return false; + try { + // WIRE: ReleaseConnection=4 lets a paused workload continue undebugged and ends + // its debugger attachment (live BC28 2026-07-16). Awaiting the invocation while + // collector intake remains open also orders any earlier Break callback ahead of + // shutdown. A rejection leaves the release unconfirmed; a delivered Break then + // receives its own release, while an unexpected connection close is fatal. + await hub.invoke("SetBreakpointResponse", STEP_WIRE.release); + return true; + } catch { + return false; + } + } + async getVariables(frameId: number): Promise { const nodes = await this.requireHub().invoke("GetVariables", frameId); return normalizeKeys(nodes ?? []).map(toVariableNode); diff --git a/src/core/record-write-triage.ts b/src/core/record-write-triage.ts index 6d1178d..ca6f9f3 100644 --- a/src/core/record-write-triage.ts +++ b/src/core/record-write-triage.ts @@ -83,8 +83,18 @@ export interface RuntimeTableType { } interface Token { - kind: "identifier" | "dot" | "leftParen" | "rightParen" | "leftBracket" | "rightBracket" | "other"; + kind: + | "identifier" + | "dot" + | "leftParen" + | "rightParen" + | "leftBracket" + | "rightBracket" + | "semicolon" + | "other"; text: string; + start: number; + end: number; } interface SourceRecord { @@ -98,7 +108,7 @@ export interface RecordWriteCollectorOptions { includeTemporary: boolean; changesDeployed: boolean; maxObservedWrites: number; - client: Pick; + client: Pick; localSource: (objectType: number, objectId: number) => Promise; localFile: (objectType: number, objectId: number) => string | undefined; } @@ -118,6 +128,7 @@ const OPERATION_BY_NAME: Record = { function lexStatement(source: string): Token[] { const tokens: Token[] = []; for (let i = 0; i < source.length;) { + const tokenStart = i; const ch = source[i]!; const next = source[i + 1]; if (/\s/.test(ch)) { @@ -164,13 +175,13 @@ function lexStatement(source: string): Token[] { i++; } } - tokens.push({ kind: "identifier", text }); + tokens.push({ kind: "identifier", text, start: tokenStart, end: i }); continue; } if (/[A-Za-z_]/.test(ch)) { const start = i++; while (i < source.length && /[A-Za-z0-9_]/.test(source[i]!)) i++; - tokens.push({ kind: "identifier", text: source.slice(start, i) }); + tokens.push({ kind: "identifier", text: source.slice(start, i), start, end: i }); continue; } const kind = ch === "." @@ -183,8 +194,10 @@ function lexStatement(source: string): Token[] { ? "leftBracket" : ch === "]" ? "rightBracket" - : "other"; - tokens.push({ kind, text: ch }); + : ch === ";" + ? "semicolon" + : "other"; + tokens.push({ kind, text: ch, start: tokenStart, end: tokenStart + 1 }); i++; } return tokens; @@ -198,7 +211,15 @@ function lineOffsets(source: string): number[] { return offsets; } -export function sourceAtStatementSpan(source: string, span: NonNullable): string | null { +interface LocatedStatementSource { + content: string; + start: number; +} + +function locateStatementSource( + source: string, + span: NonNullable, +): LocatedStatementSource | null { const offsets = lineOffsets(source); const fromLine = span.from.line - 1; const toLine = span.to.line - 1; @@ -207,17 +228,42 @@ export function sourceAtStatementSpan(source: string, span: NonNullable= source.length || to <= from) return null; - return source.slice(from, to); + return { content: source.slice(from, to), start: from }; } -export function inspectRecordWriteStatement(source: string): RecordWriteStatementResult { +export function sourceAtStatementSpan(source: string, span: NonNullable): string | null { + return locateStatementSource(source, span)?.content ?? null; +} + +const NO_ARGUMENT_OPERATION = new Set(["insert", "modify", "delete", "deleteAll"]); +const NO_ARGUMENT_FOLLOWER = new Set(["then", "do", "else", "until", "and", "or"]); + +interface InternalRecordWriteStatement { + statement: RecordWriteStatement; + implicitReceiver: boolean; + operationOffset: number; +} + +type InternalStatementResult = + | { match: InternalRecordWriteStatement; reason: null } + | { match: null; reason: "writeStatementUnrecognized" | "receiverUnsupported" }; + +function isOperationInvocation(tokens: Token[], index: number, operation: RecordWriteOperation): boolean { + const next = tokens[index + 1]; + if (next?.kind === "leftParen") return true; + if (!NO_ARGUMENT_OPERATION.has(operation)) return false; + if (!next || next.kind === "semicolon" || next.kind === "rightParen" || next.kind === "rightBracket") return true; + return next.kind === "identifier" && NO_ARGUMENT_FOLLOWER.has(next.text.toLowerCase()); +} + +function inspectStatement(source: string): InternalStatementResult { const tokens = lexStatement(source); - const matches: RecordWriteStatement[] = []; + const matches: InternalRecordWriteStatement[] = []; let unsupportedReceiver = false; - for (let i = 0; i < tokens.length - 1; i++) { + for (let i = 0; i < tokens.length; i++) { const token = tokens[i]!; const operation = token.kind === "identifier" ? OPERATION_BY_NAME[token.text.toLowerCase()] : undefined; - if (!operation || tokens[i + 1]?.kind !== "leftParen") continue; + if (!operation || !isOperationInvocation(tokens, i, operation)) continue; if (tokens[i - 1]?.kind === "dot") { const receiver = tokens[i - 2]; if (!receiver || receiver.kind !== "identifier") { @@ -229,18 +275,171 @@ export function inspectRecordWriteStatement(source: string): RecordWriteStatemen unsupportedReceiver = true; continue; } - matches.push({ operation, receiver: receiver.text }); + matches.push({ + statement: { operation, receiver: receiver.text }, + implicitReceiver: false, + operationOffset: token.start, + }); } else { - matches.push({ operation, receiver: "Rec" }); + matches.push({ + statement: { operation, receiver: "Rec" }, + implicitReceiver: true, + operationOffset: token.start, + }); } } if (matches.length !== 1) { return { - statement: null, + match: null, reason: unsupportedReceiver && matches.length === 0 ? "receiverUnsupported" : "writeStatementUnrecognized", }; } - return { statement: matches[0]!, reason: null }; + return { match: matches[0]!, reason: null }; +} + +function tokenLower(token: Token | undefined): string { + return token?.kind === "identifier" ? token.text.toLowerCase() : ""; +} + +function afterOptionalSemicolon(tokens: Token[], index: number): number { + return tokens[index]?.kind === "semicolon" ? index + 1 : index; +} + +function matchingEnd(tokens: Token[], start: number): number { + let depth = 0; + for (let i = start; i < tokens.length; i++) { + const lower = tokenLower(tokens[i]); + if (lower === "begin" || lower === "case") { + depth++; + } else if (lower === "end") { + depth--; + if (depth === 0) return i + 1; + } + } + return tokens.length; +} + +function matchingUntil(tokens: Token[], start: number): number { + let depth = 0; + for (let i = start; i < tokens.length; i++) { + const lower = tokenLower(tokens[i]); + if (lower === "repeat") { + depth++; + } else if (lower === "until") { + depth--; + if (depth === 0) { + for (let j = i + 1; j < tokens.length; j++) { + if (tokens[j]?.kind === "semicolon") return j + 1; + } + return tokens.length; + } + } + } + return tokens.length; +} + +function statementEnd(tokens: Token[], start: number): number { + const first = tokenLower(tokens[start]); + if (first === "begin" || first === "case") { + const end = matchingEnd(tokens, start); + if (tokenLower(tokens[end]) === "else") return statementEnd(tokens, end + 1); + return afterOptionalSemicolon(tokens, end); + } + if (first === "repeat") return matchingUntil(tokens, start); + + let parentheses = 0; + let brackets = 0; + for (let i = start; i < tokens.length; i++) { + const token = tokens[i]!; + if (token.kind === "leftParen") parentheses++; + if (token.kind === "rightParen") parentheses = Math.max(0, parentheses - 1); + if (token.kind === "leftBracket") brackets++; + if (token.kind === "rightBracket") brackets = Math.max(0, brackets - 1); + if (parentheses !== 0 || brackets !== 0) continue; + + const lower = tokenLower(token); + if (i > start && (lower === "begin" || lower === "case")) { + const end = matchingEnd(tokens, i); + if (tokenLower(tokens[end]) === "else") return statementEnd(tokens, end + 1); + return afterOptionalSemicolon(tokens, end); + } + if (i > start && lower === "repeat") return matchingUntil(tokens, i); + if (token.kind === "semicolon") return i + 1; + } + return tokens.length; +} + +type ExplicitWithContext = + | { kind: "none" } + | { kind: "receiver"; receiver: string } + | { kind: "ambiguous" }; + +function explicitWithContext(source: string, operationOffset: number): ExplicitWithContext { + const tokens = lexStatement(source); + const operationIndex = tokens.findIndex((token) => + token.start <= operationOffset && operationOffset < token.end + ); + if (operationIndex < 0) return { kind: "none" }; + + let context: ExplicitWithContext = { kind: "none" }; + let nearestDo = -1; + for (let i = 0; i < operationIndex; i++) { + if (tokenLower(tokens[i]) !== "with") continue; + let doIndex = -1; + let parentheses = 0; + let brackets = 0; + for (let j = i + 1; j < operationIndex; j++) { + const token = tokens[j]!; + if (token.kind === "leftParen") parentheses++; + if (token.kind === "rightParen") parentheses = Math.max(0, parentheses - 1); + if (token.kind === "leftBracket") brackets++; + if (token.kind === "rightBracket") brackets = Math.max(0, brackets - 1); + if (parentheses === 0 && brackets === 0 && tokenLower(token) === "do") { + doIndex = j; + break; + } + if (parentheses === 0 && brackets === 0 && token.kind === "semicolon") break; + } + if (doIndex < 0 || doIndex + 1 > operationIndex || doIndex <= nearestDo) continue; + const bodyEnd = statementEnd(tokens, doIndex + 1); + if (operationIndex < doIndex + 1 || operationIndex >= bodyEnd) continue; + + const receiverTokens = tokens.slice(i + 1, doIndex); + nearestDo = doIndex; + context = receiverTokens.length === 1 && receiverTokens[0]?.kind === "identifier" + ? { kind: "receiver", receiver: receiverTokens[0].text } + : { kind: "ambiguous" }; + } + return context; +} + +export function inspectRecordWriteStatement(source: string): RecordWriteStatementResult { + const inspected = inspectStatement(source); + return inspected.match + ? { statement: inspected.match.statement, reason: null } + : { statement: null, reason: inspected.reason }; +} + +export function inspectRecordWriteAtSpan( + source: string, + span: NonNullable, +): RecordWriteStatementResult { + const located = locateStatementSource(source, span); + if (!located) return { statement: null, reason: "writeStatementUnrecognized" }; + const inspected = inspectStatement(located.content); + if (!inspected.match) return { statement: null, reason: inspected.reason }; + if (!inspected.match.implicitReceiver) { + return { statement: inspected.match.statement, reason: null }; + } + const withContext = explicitWithContext(source, located.start + inspected.match.operationOffset); + if (withContext.kind === "ambiguous") return { statement: null, reason: "receiverUnsupported" }; + return { + statement: { + ...inspected.match.statement, + receiver: withContext.kind === "receiver" ? withContext.receiver : "Rec", + }, + reason: null, + }; } export function parseRecordWriteStatement(source: string): RecordWriteStatement | null { @@ -302,6 +501,8 @@ export class RecordWriteCollector { private truncatedValue = false; private stopReasonValue: RecordWriteReport["stopReason"] | null = null; private accepting = true; + private finishing = false; + private shutdownRelease: Promise | null = null; private paused = false; private evidenceLost = false; private chain: Promise = Promise.resolve(); @@ -342,14 +543,20 @@ export class RecordWriteCollector { } async finish(): Promise { - // Close intake synchronously, then drain every event accepted before this call. - // The queued handlers use terminal(), not accepting, so already accepted breaks - // are still classified and resumed. + // Keep intake open while a release invocation crosses the SignalR connection. + // Any Break already in flight is enqueued before that invocation settles, after + // which the serialized chain can be drained without mistaking a quiet local queue + // for a quiet transport. + this.finishing = true; + this.shutdownRelease = this.accepting && !this.terminal() + ? this.options.client.releaseForShutdown() + : Promise.resolve(false); + await this.shutdownRelease; + await this.drainEvents(); this.accepting = false; - await this.chain; if (this.paused) { try { - await this.options.client.step("release"); + if (!await this.shutdownRelease) await this.options.client.step("release"); this.paused = false; } catch (error) { await this.fail(`Unable to release the paused workload: ${safeDetail(error)}`); @@ -368,10 +575,15 @@ export class RecordWriteCollector { } private terminal(): boolean { - return this.phaseValue === "failed" - || this.stopReasonValue === "sessionDetached" - || this.stopReasonValue === "maxObservedWrites" - || this.stopReasonValue === "fatal"; + return this.phaseValue === "failed" || this.stopReasonValue !== null; + } + + private async drainEvents(): Promise { + while (true) { + const pending = this.chain; + await pending; + if (pending === this.chain) return; + } } private summary(): RecordWriteSummary { @@ -458,6 +670,15 @@ export class RecordWriteCollector { return; } + if (this.finishing) { + this.stopReasonValue = "finished"; + this.phaseValue = "stopped"; + this.accepting = false; + if (!await this.shutdownRelease) await this.options.client.step("release"); + this.paused = false; + return; + } + await this.options.client.step("continue"); this.paused = false; } @@ -484,12 +705,11 @@ export class RecordWriteCollector { this.recordUnresolved(sequence, stack, "sourceUnavailable", null, null); return; } - const statementSource = sourceAtStatementSpan(source.content, frame.statementSpan); - if (statementSource === null) { + if (sourceAtStatementSpan(source.content, frame.statementSpan) === null) { this.recordUnresolved(sequence, stack, "statementSpanUnavailable", null, null); return; } - const inspected = inspectRecordWriteStatement(statementSource); + const inspected = inspectRecordWriteAtSpan(source.content, frame.statementSpan); if (!inspected.statement) { this.recordUnresolved(sequence, stack, inspected.reason, null, null); return; diff --git a/tests/core/debugger-hub.test.ts b/tests/core/debugger-hub.test.ts index fca4d21..11a08b4 100644 --- a/tests/core/debugger-hub.test.ts +++ b/tests/core/debugger-hub.test.ts @@ -46,6 +46,27 @@ describe("DebuggerClient", () => { expect(hub.invoked("DebugAdapterConfigurationDone")).toHaveLength(1); }); + test("a rejected debugger configuration is fatal and never reports sessionBound", async () => { + const hub = new FakeHub(); + hub.onInvoke = (method) => { + if (method === "DebugAdapterConfigurationDone") throw new Error("configuration rejected"); + if (method === "GetNstSessionInfo") return { SessionId: 10, HostId: null }; + return undefined; + }; + const { client, events } = await connected(hub, { breakOnRecordWrite: "nonTemporary" }); + hub.emit("HubConnected"); + await Bun.sleep(0); + await Bun.sleep(0); + + expect(events).toEqual([ + { kind: "fatal", message: "Debugger configuration failed: configuration rejected" }, + ]); + expect(hub.invoked("GetNstSessionInfo")).toHaveLength(0); + expect(hub.invoked("StopDebugging")).toHaveLength(1); + expect(hub.stopped).toBe(true); + expect(client.connectionId).toBeNull(); + }); + test("break behaviour matrix maps booleans and precision modes to wire enums", async () => { const cases: Array<[DebugAttachOptions, { BreakOnError: boolean; BreakOnErrorBehaviour: number; BreakOnRecordWrite: boolean; BreakOnRecordWriteBehaviour: number }]> = [ // WIRE enums: Unspecified=0, None=1, All=2, ExcludeTry|ExcludeTemporary=3 @@ -302,6 +323,21 @@ describe("DebuggerClient", () => { expect(hub.invoked("SetBreakpointResponse").map((i) => i.args[0])).toEqual([4, 5]); }); + test("shutdown release is a best-effort transport barrier", async () => { + const hub = new FakeHub(); + const { client } = await connected(hub); + expect(await client.releaseForShutdown()).toBe(true); + expect(hub.invoked("SetBreakpointResponse").at(-1)?.args).toEqual([4]); + + hub.onInvoke = (method) => { + if (method === "SetBreakpointResponse") throw new Error("no paused break"); + return undefined; + }; + expect(await client.releaseForShutdown()).toBe(false); + await client.stop(); + expect(await client.releaseForShutdown()).toBe(false); + }); + test("getSourceContent sends the object wrapper and normalizes the response", async () => { const hub = new FakeHub(); hub.onInvoke = (method) => (method === "GetSourceContent" ? { Content: "codeunit 50130 X {}", IsALContent: true } : undefined); @@ -633,10 +669,13 @@ describe("DebuggerClient", () => { expect(client.connectionId).toBeNull(); }); - test("unexpected close clears the hub so calls fail fast", async () => { + test("unexpected close without an Error is fatal and clears the hub", async () => { const hub = new FakeHub(); - const { client } = await connected(hub); - hub.close(new Error("dropped")); + const { client, events } = await connected(hub); + hub.close(); + expect(events).toEqual([ + { kind: "fatal", message: "Debugger hub connection closed unexpectedly" }, + ]); await expect(client.step("continue")).rejects.toThrow(/not connected/); }); }); diff --git a/tests/core/record-write-triage.test.ts b/tests/core/record-write-triage.test.ts index 4d237d0..e05f194 100644 --- a/tests/core/record-write-triage.test.ts +++ b/tests/core/record-write-triage.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from "bun:test"; import { + inspectRecordWriteAtSpan, inspectRecordWriteStatement, parseRecordWriteStatement, parseRuntimeTableType, @@ -38,6 +39,19 @@ function brk(stack: StackFrameInfo[] = [frame()]): DebuggerEvent { return { kind: "break", objectType: 5, objectId: 50100, line: 7, stack }; } +function spanFor(source: string, needle: string): NonNullable { + const index = source.indexOf(needle); + if (index < 0) throw new Error(`Missing test needle: ${needle}`); + const before = source.slice(0, index); + const line = before.split("\n").length; + const lineStart = before.lastIndexOf("\n") + 1; + const column = index - lineStart + 1; + return { + from: { line, column }, + to: { line, column: column + needle.length - 1 }, + }; +} + function client(typeNames: string[]) { const steps: string[] = []; const watchExpressions: string[] = []; @@ -71,6 +85,7 @@ function client(typeNames: string[]) { step: async (action: string) => { steps.push(action); }, + releaseForShutdown: async () => false, stop: async () => { stops++; }, @@ -92,6 +107,76 @@ describe("record-write statement parsing", () => { expect(parseRecordWriteStatement("Customer.DeleteAll(false);")).toEqual({ operation: "deleteAll", receiver: "Customer" }); }); + test("recognizes valid zero-argument write calls without parentheses", () => { + expect(parseRecordWriteStatement("Customer.Insert;")).toEqual({ operation: "insert", receiver: "Customer" }); + expect(parseRecordWriteStatement("Customer.Modify;")).toEqual({ operation: "modify", receiver: "Customer" }); + expect(parseRecordWriteStatement("Customer.Delete;")).toEqual({ operation: "delete", receiver: "Customer" }); + expect(parseRecordWriteStatement("Customer.DeleteAll;")).toEqual({ operation: "deleteAll", receiver: "Customer" }); + expect(parseRecordWriteStatement("Insert;")).toEqual({ operation: "insert", receiver: "Rec" }); + }); + + test("uses an enclosing explicit WITH receiver instead of incorrectly assuming Rec", () => { + const source = [ + "table 50100 Host", + "{", + " procedure WriteTarget()", + " var", + " Target: Record Customer;", + " begin", + " with Target do begin", + " Insert(true);", + " end;", + " Rec.Modify(true);", + " end;", + "}", + ].join("\n"); + expect(inspectRecordWriteAtSpan(source, spanFor(source, "Insert(true);"))).toEqual({ + statement: { operation: "insert", receiver: "Target" }, + reason: null, + }); + expect(inspectRecordWriteAtSpan(source, spanFor(source, "Rec.Modify(true);"))).toEqual({ + statement: { operation: "modify", receiver: "Rec" }, + reason: null, + }); + + const nested = [ + "table 50100 Host", + "{", + " procedure WriteTarget()", + " var", + " Outer: Record Customer;", + " Inner: Record Vendor;", + " begin", + " with Outer do begin", + " with Inner do", + " Insert;", + " end;", + " end;", + "}", + ].join("\n"); + expect(inspectRecordWriteAtSpan(nested, spanFor(nested, "Insert;"))).toEqual({ + statement: { operation: "insert", receiver: "Inner" }, + reason: null, + }); + }); + + test("fails closed when an explicit WITH receiver is not a watch-compatible identifier", () => { + const source = [ + "codeunit 50100 Writer", + "{", + " procedure WriteTarget()", + " begin", + " with GetTarget() do", + " Insert(true);", + " end;", + "}", + ].join("\n"); + expect(inspectRecordWriteAtSpan(source, spanFor(source, "Insert(true);"))).toEqual({ + statement: null, + reason: "receiverUnsupported", + }); + }); + test("handles quoted and implicit Rec receivers while ignoring comments and strings", () => { expect(parseRecordWriteStatement('"Customer Buffer".Insert();')).toEqual({ operation: "insert", @@ -327,7 +412,7 @@ describe("RecordWriteCollector", () => { expect(report.writers[0]).toMatchObject({ receiver: "Customer", count: 2 }); }); - test("serializes rapid breaks and finish drains accepted work but rejects late events", async () => { + test("serializes rapid breaks and finish releases an accepted in-flight break", async () => { let resolveWatch!: (value: { name: string; typeName: string; @@ -366,7 +451,6 @@ describe("RecordWriteCollector", () => { expect(watchCalls).toBe(1); const finishing = collector.finish(); - collector.onEvent(brk()); resolveWatch({ name: "Customer", typeName: "Table Customer (18)", @@ -377,7 +461,72 @@ describe("RecordWriteCollector", () => { }); const report = await finishing; expect(report.summary).toMatchObject({ observedWrites: 1, matchedWrites: 1 }); - expect(fake.steps).toEqual(["continue"]); + expect(fake.steps).toEqual(["release"]); + }); + + test("finish captures and releases a break that arrives as shutdown begins", async () => { + const fake = client(["Table Customer (18)"]); + const collector = new RecordWriteCollector({ + tableId: 18, + includeTemporary: false, + changesDeployed: false, + maxObservedWrites: 10, + client: fake as never, + localSource: async () => null, + localFile: () => undefined, + }); + collector.onEvent({ kind: "sessionBound", sessionId: 99, hostId: null }); + await collector.waitForIdle(); + + const finishing = collector.finish(); + collector.onEvent(brk()); + const report = await finishing; + + expect(report).toMatchObject({ + complete: true, + stopReason: "finished", + summary: { observedWrites: 1, matchedWrites: 1 }, + }); + expect(fake.steps).toEqual(["release"]); + collector.onEvent(brk()); + expect(collector.status().summary.observedWrites).toBe(1); + }); + + test("finish orders an asynchronous Break callback through the shutdown release barrier", async () => { + for (const releaseAccepted of [true, false]) { + const fake = client(["Table Customer (18)"]); + const collector = new RecordWriteCollector({ + tableId: 18, + includeTemporary: false, + changesDeployed: false, + maxObservedWrites: 10, + client: fake as never, + localSource: async () => null, + localFile: () => undefined, + }); + collector.onEvent({ kind: "sessionBound", sessionId: 99, hostId: null }); + await collector.waitForIdle(); + + fake.releaseForShutdown = async () => { + await new Promise((resolve) => { + setTimeout(() => { + collector.onEvent(brk()); + resolve(); + }, 0); + }); + return releaseAccepted; + }; + + const report = await collector.finish(); + expect(report).toMatchObject({ + complete: true, + stopReason: "finished", + summary: { observedWrites: 1, matchedWrites: 1 }, + }); + // An accepted transport-level release already resumed this break. When the + // barrier is rejected, the classified callback sends the one required release. + expect(fake.steps).toEqual(releaseAccepted ? [] : ["release"]); + } }); test("inspection failures are redacted, fail closed, and continue automatically", async () => { diff --git a/tests/mcp/record-write-tools.test.ts b/tests/mcp/record-write-tools.test.ts index 8d2a841..5356baa 100644 --- a/tests/mcp/record-write-tools.test.ts +++ b/tests/mcp/record-write-tools.test.ts @@ -239,6 +239,7 @@ describe("record-write MCP tools", () => { expect(report).toMatchObject({ truncated: true, complete: false }); expect(report.nextSteps.join(" ")).toContain("maxObservedWrites"); expect(state.debugOwner).toBeNull(); + expect(hub.invoked("SetBreakpointResponse").map((call) => call.args)).toEqual([[4]]); }); test("a session identity lookup warning is nonfatal and retained in status", async () => { @@ -261,6 +262,56 @@ describe("record-write MCP tools", () => { await tools.get("bcdev_record_writes_finish")!.handler({}); }); + test("a rejected debugger configuration fails closed instead of returning a complete zero-write report", async () => { + const hub = new FakeHub(); + hub.onInvoke = (method) => { + if (method === "DebugAdapterConfigurationDone") throw new Error("configuration rejected"); + if (method === "GetNstSessionInfo") return { SessionId: 321, HostId: null }; + return undefined; + }; + const { tools } = setup(hub); + await tools.get("bcdev_record_writes_start")!.handler({ tableId: 18 }); + hub.emit("HubConnected"); + await Bun.sleep(0); + await Bun.sleep(0); + + const status = await tools.get("bcdev_record_writes_status")!.handler({}) as { + phase: string; + }; + expect(status.phase).toBe("failed"); + const report = await tools.get("bcdev_record_writes_finish")!.handler({}) as { + outcome: string; + complete: boolean; + warnings: string[]; + }; + expect(report).toMatchObject({ outcome: "failed", complete: false }); + expect(report.warnings.join(" ")).toContain("Debugger configuration failed"); + }); + + test("an unexpected clean hub close fails the retained report closed", async () => { + const hub = new FakeHub(); + hub.onInvoke = (method) => + method === "GetNstSessionInfo" ? { SessionId: 321, HostId: null } : undefined; + const { tools } = setup(hub); + await tools.get("bcdev_record_writes_start")!.handler({ tableId: 18 }); + hub.emit("HubConnected"); + await Bun.sleep(0); + hub.close(); + await Bun.sleep(0); + + const status = await tools.get("bcdev_record_writes_status")!.handler({}) as { + phase: string; + }; + expect(status.phase).toBe("failed"); + const report = await tools.get("bcdev_record_writes_finish")!.handler({}) as { + outcome: string; + complete: boolean; + warnings: string[]; + }; + expect(report).toMatchObject({ outcome: "failed", complete: false }); + expect(report.warnings.join(" ")).toContain("connection closed unexpectedly"); + }); + test("an out-of-band user-filter rejection fails closed and preserves a partial report", async () => { const hub = new FakeHub(); const { state, tools } = setup(hub); From afa6ca50bcf8fa0dcae149da1572fbf7b4b451dc Mon Sep 17 00:00:00 2001 From: ianray <193674599+ianrayianray@users.noreply.github.com> Date: Sun, 26 Jul 2026 12:58:29 -0700 Subject: [PATCH 3/3] fix: fail record-write triage closed on detach --- README.md | 6 +- ...break-on-record-write-triage-2026-07-25.md | 27 +++- scripts/e2e.md | 6 +- skills/bc-al-debugging/SKILL.md | 2 + src/core/record-write-triage.ts | 37 ++++-- src/mcp/skills.generated.ts | 2 +- src/mcp/tools/record-write-tools.ts | 5 +- tests/core/record-write-triage.test.ts | 123 +++++++++++++++--- tests/mcp/record-write-tools.test.ts | 27 ++++ 9 files changed, 195 insertions(+), 40 deletions(-) diff --git a/README.md b/README.md index 5ce7e46..d883ff8 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ MCP server for Business Central AL development: run tests (with code coverage) a [![Bun](https://img.shields.io/badge/bun-1.x-black)](https://bun.sh) [![TypeScript](https://img.shields.io/badge/typescript-strict-blue)](https://typescriptlang.org) [![BC dev API](https://img.shields.io/badge/BC%20dev%20API-%E2%89%A57.0-purple)]() -[![Tests](https://img.shields.io/badge/tests-594%20passing-green)]() +[![Tests](https://img.shields.io/badge/tests-598%20passing-green)]() ## Overview @@ -170,7 +170,9 @@ workload and returns a truncated, incomplete report. The final `complete` flag means every record-write break observed in this one attached-session capture window was classified. It is not proof that no other session or operation writes the table. Any missing source/span, unsupported receiver, watch failure, unknown runtime type, -unexpected break, lifecycle failure, or truncation keeps the report fail-closed. +unexpected break, lifecycle failure, early session detach, or truncation keeps the report +fail-closed. Exact groups collected before a detach remain available, but the tool does not claim +that no later writes were missed. ## Profiling diff --git a/scripts/e2e-break-on-record-write-triage-2026-07-25.md b/scripts/e2e-break-on-record-write-triage-2026-07-25.md index 096bfbb..63cda17 100644 --- a/scripts/e2e-break-on-record-write-triage-2026-07-25.md +++ b/scripts/e2e-break-on-record-write-triage-2026-07-25.md @@ -17,13 +17,13 @@ harness made no manual debugger wait or continue call. ## Results -- The default capture completed with 13 observed writes: 10 exact target writes in 9 +- The default workload produced 13 observed writes: 10 exact target writes in 9 grouped writers, 3 proven unrelated writes, and 0 unresolved writes. - The exact groups covered `Insert`, `Modify`, `ModifyAll`, `Rename`, `Delete`, and `DeleteAll`, including repeated calls from one stack, a second caller stack, and a runtime `RecordRef` receiver. - Every exact writer used deployed source and retained a usable full statement span. - The report was complete for its attached-session capture window. + Classification had zero unresolved writes. - Repeating the capture with temporary writes enabled added exactly one matched target write relative to the default non-temporary capture. - Forced local-source fallback without `changesDeployed` produced 0 exact matches and @@ -38,12 +38,12 @@ harness made no manual debugger wait or continue call. ## Correction regression -The disposable fixture was republished as version 1.0.0.5 after the edge-case review. +The disposable fixture was republished as version 1.0.0.5 after the first edge-case review. A table method whose implicit `Rec` was a different table executed an explicit `with Target` block containing parenthesis-free `Insert`, `Modify`, `Delete`, and `DeleteAll` calls. -- The targeted capture completed with every writer receiver reported as `Target`, not +- The targeted capture classified every writer receiver as `Target`, not the host table's `Rec`. - All four parenthesis-free operations were classified from deployed source with zero unresolved writes. @@ -55,5 +55,24 @@ A table method whose implicit `Rec` was a different table executed an explicit both accepted and rejected release-barrier responses without dropping the Break or sending a duplicate release. +## Detach and conditional-upsert follow-up + +Version 1.0.0.6 added the idiomatic conditional form +`if not Target.Insert(false) then Target.Modify(true)` and reran the full matrix. + +- SaaS reported an individual write-call statement span for each `Insert` and `Modify`, + not one enclosing conditional span. The scenario observed 5 writes, matched all 5, + and produced 0 unresolved writes. +- `multipleWriteCandidates` remains a stable fail-closed reason for a server or source + span that genuinely contains more than one write candidate. +- SaaS Background completion emits `OnDetachedFromConnection` before finish. The final + implementation therefore retains the exact 13/10/9/3/0 evidence but reports + `complete: false`, `stopReason: "sessionDetached"`, and a warning rather than a false + all-clear. A detach caused by finish's own release barrier remains a deliberate + `finished` window. +- The full matrix still passed: temporary delta +1, asserted/unasserted local fallback, + one-write cap release, contextual `with`, parenthesis-free calls, automatic + continuation, and debugger-slot reuse. + The fixture and harness lived outside the repository and are not part of the feature diff. diff --git a/scripts/e2e.md b/scripts/e2e.md index d5df411..480ca2e 100644 --- a/scripts/e2e.md +++ b/scripts/e2e.md @@ -55,11 +55,13 @@ field values, or raw source. Evidence belongs in - [x] `bcdev_record_writes_start` returns while arming, then the matching workload binds and status moves to `collecting` without manual debugger wait/continue calls. - [x] Default `includeTemporary:false` excludes a temporary target write while retaining real target writes; `includeTemporary:true` observes the temporary write. - [x] One workload produces repeated exact target writes from one stack, an exact target write from a second stack, and an unrelated-table write; final counts and groups match. -- [x] Every exact group uses deployed source plus a runtime receiver `tableId` equal to the requested numeric ID; unrelated and unresolved writes never enter `writers`. +- [x] Every exact group uses deployed source plus a runtime receiver `tableId` equal to the requested numeric ID; unrelated and unresolved writes never enter `writers`. - [x] The unasserted local-source fallback cannot produce an exact group; `changesDeployed:true` permits it only after the exact fixture has been published. - [x] `maxObservedWrites` classifies the cap event, sends `release`, and returns `truncated:true`, `complete:false` without leaving the workload paused. - [x] Finish clears the triage owner and an immediate manual attach or second triage start can claim the debugger slot. -- [x] An explicit `with Target` inside a different table's method resolves unqualified writes to `Target`, and valid parenthesis-free `Insert`, `Modify`, `Delete`, and `DeleteAll` calls classify exactly. +- [x] An explicit `with Target` inside a different table's method resolves unqualified writes to `Target`, and valid parenthesis-free `Insert`, `Modify`, `Delete`, and `DeleteAll` calls classify exactly. +- [x] The idiomatic `if not Record.Insert(false) then Record.Modify(true)` reports an individual write-call StatementSpan for each operation rather than one enclosing conditional span. +- [x] A session detach before finish retains partial exact groups but forces `complete:false`, `stopReason:"sessionDetached"`, a warning, and cautious next steps; a detach caused by finish's own release barrier does not. - [x] Finish orders an in-flight asynchronous Break through a best-effort `ReleaseConnection` invocation before closing intake, classifies the callback, and releases exactly once whether the barrier is accepted or rejected. - [x] Rejected debugger configuration and an unexpected clean hub close fail the retained report closed rather than returning a complete zero-write result. - [x] No Production or on-premises call is made during this feature's acceptance run. diff --git a/skills/bc-al-debugging/SKILL.md b/skills/bc-al-debugging/SKILL.md index ac1a707..04af446 100644 --- a/skills/bc-al-debugging/SKILL.md +++ b/skills/bc-al-debugging/SKILL.md @@ -49,6 +49,8 @@ that exact source and explicitly pass `changesDeployed: true`. Without that asse mapping remains unresolved. `maxObservedWrites` defaults to 500; reaching it releases the workload and returns a truncated, incomplete report. Missing source/span, an unsupported receiver, watch failure, unknown runtime table identity, or debugger failure also keeps the result fail-closed. +If the session detaches before `bcdev_record_writes_finish`, retained exact groups remain useful +but `complete` is false because later writes cannot be ruled out. Finish always clears the shared debugger slot; manual debugging and record-write triage cannot run at the same time. diff --git a/src/core/record-write-triage.ts b/src/core/record-write-triage.ts index ca6f9f3..e8309f0 100644 --- a/src/core/record-write-triage.ts +++ b/src/core/record-write-triage.ts @@ -8,6 +8,7 @@ export type RecordWriteUnresolvedReason = | "sourceUnavailable" | "statementSpanUnavailable" | "writeStatementUnrecognized" + | "multipleWriteCandidates" | "receiverUnsupported" | "receiverUnavailable" | "receiverTypeUnresolved" @@ -75,7 +76,10 @@ export interface RecordWriteStatement { export type RecordWriteStatementResult = | { statement: RecordWriteStatement; reason: null } - | { statement: null; reason: "writeStatementUnrecognized" | "receiverUnsupported" }; + | { + statement: null; + reason: "writeStatementUnrecognized" | "multipleWriteCandidates" | "receiverUnsupported"; + }; export interface RuntimeTableType { tableId: number; @@ -105,7 +109,6 @@ interface SourceRecord { export interface RecordWriteCollectorOptions { tableId: number; - includeTemporary: boolean; changesDeployed: boolean; maxObservedWrites: number; client: Pick; @@ -246,7 +249,10 @@ interface InternalRecordWriteStatement { type InternalStatementResult = | { match: InternalRecordWriteStatement; reason: null } - | { match: null; reason: "writeStatementUnrecognized" | "receiverUnsupported" }; + | { + match: null; + reason: "writeStatementUnrecognized" | "multipleWriteCandidates" | "receiverUnsupported"; + }; function isOperationInvocation(tokens: Token[], index: number, operation: RecordWriteOperation): boolean { const next = tokens[index + 1]; @@ -271,7 +277,11 @@ function inspectStatement(source: string): InternalStatementResult { continue; } const beforeReceiver = tokens[i - 3]; - if (beforeReceiver?.kind === "rightParen" || beforeReceiver?.kind === "rightBracket") { + if ( + beforeReceiver?.kind === "dot" + || beforeReceiver?.kind === "rightParen" + || beforeReceiver?.kind === "rightBracket" + ) { unsupportedReceiver = true; continue; } @@ -288,12 +298,9 @@ function inspectStatement(source: string): InternalStatementResult { }); } } - if (matches.length !== 1) { - return { - match: null, - reason: unsupportedReceiver && matches.length === 0 ? "receiverUnsupported" : "writeStatementUnrecognized", - }; - } + if (unsupportedReceiver) return { match: null, reason: "receiverUnsupported" }; + if (matches.length > 1) return { match: null, reason: "multipleWriteCandidates" }; + if (matches.length === 0) return { match: null, reason: "writeStatementUnrecognized" }; return { match: matches[0]!, reason: null }; } @@ -634,7 +641,15 @@ export class RecordWriteCollector { this.paused = false; this.accepting = false; this.phaseValue = "stopped"; - this.stopReasonValue ??= "sessionDetached"; + if (this.finishing) { + this.stopReasonValue ??= "finished"; + } else { + this.stopReasonValue ??= "sessionDetached"; + this.evidenceLost = true; + this.warnings.push( + "Debugger session detached before finish; the capture ended early and may exclude later writes.", + ); + } return; } if (event.kind === "fatal") { diff --git a/src/mcp/skills.generated.ts b/src/mcp/skills.generated.ts index 86ce350..4a87ecd 100644 --- a/src/mcp/skills.generated.ts +++ b/src/mcp/skills.generated.ts @@ -14,7 +14,7 @@ export const skills: EmbeddedSkill[] = [ "description": "Interactively debug AL code on a Business Central server with the bc-dev-mcp tools — attach, breakpoints, break-on-error, step, inspect variables and watches, or collect the stacks that write a table. Use when asked to debug AL code, find why an AL test fails, identify table writers, or inspect live state on a BC server.", "uri": "skill://bc-al-debugging/SKILL.md", "mimeType": "text/markdown", - "text": "---\nname: bc-al-debugging\ndescription: Interactively debug AL code on a Business Central server with the bc-dev-mcp tools — attach, breakpoints, break-on-error, step, inspect variables and watches, or collect the stacks that write a table. Use when asked to debug AL code, find why an AL test fails, identify table writers, or inspect live state on a BC server.\n---\n\n# Debugging AL on Business Central\n\n## The loop\n\n1. `bcdev_debug_attach { breakOnError: true }` — arm the next WebClient session WITHOUT\n file/line breakpoints (see \"session must pause first\" below). To filter that next session,\n add `userId`; to attach to a known active NST session, add its positive `sessionId` instead.\n The selectors are mutually exclusive, and exact `sessionId` targeting takes precedence over\n `breakOnNext`. One debugger at a time; `bcdev_debug_detach` before re-attaching.\n2. `bcdev_debug_run_tests { codeunits: [{ id: }] }` — starts the run bound to\n this debug session and returns immediately.\n3. `bcdev_debug_wait` — long-poll for events. Attach returns before a workload binds; a successful\n bind reports `kind: sessionBound` with `sessionId` and `hostId` (the optional `hostId` may be null). **A timeout is a normal result**\n (`timedOut: true`): just call it again. Other events carry `kind`: `break`,\n `testRunFinished` (with the test results embedded), `detached`, `fatal`.\n If waiting times out before binding, confirm that you actually created or triggered the matching\n WebClient/workload, then wait again; waiting alone cannot create a next-session target.\n4. At a `break`: `bcdev_debug_variables { frameId: 0 }` for locals,\n `bcdev_debug_eval { frameId: 0, expression: \"...\" }` for watches,\n `bcdev_debug_breakpoints { add: [...] }` to place file/line breakpoints.\n5. `bcdev_debug_continue { action: \"continue\" | \"stepOver\" | \"stepInto\" | \"stepOut\" }`,\n then `bcdev_debug_wait` again.\n6. `bcdev_debug_detach` when done.\n\n## Find everything that writes one table\n\nUse the dedicated background workflow instead of manual `bcdev_debug_*` stepping:\n\n1. `bcdev_record_writes_start { tableId: }`.\n Add exactly one of `sessionId` or `userId` when needed, or choose `breakOnNext`.\n The tool returns after attach is armed; it does not wait for the session or workload.\n2. Trigger the matching session and operation. Business Central breaks on every record write\n globally; the collector identifies the runtime receiver, counts unrelated writes, groups exact\n target-table stacks, and automatically continues. Do not call `bcdev_debug_wait` or\n `bcdev_debug_continue`.\n3. `bcdev_record_writes_status` reads progress without driving it. When the target operation has\n finished, call `bcdev_record_writes_finish`.\n4. Review `writers`, `unresolved`, `warnings`, and `complete`. A complete report covers only the\n observed breaks in this one attached-session capture window; it is not a tenant-wide proof.\n\nTemporary-record writes are excluded by default. Pass `includeTemporary: true` only when they\nmatter. Deployed source is trusted; local source can prove a receiver only after you have published\nthat exact source and explicitly pass `changesDeployed: true`. Without that assertion, local-only\nmapping remains unresolved. `maxObservedWrites` defaults to 500; reaching it releases the workload\nand returns a truncated, incomplete report. Missing source/span, an unsupported receiver, watch\nfailure, unknown runtime table identity, or debugger failure also keeps the result fail-closed.\nFinish always clears the shared debugger slot; manual debugging and record-write triage cannot run\nat the same time.\n\n## Facts that save you time\n\n- **Choose one targeting mode.** Omit both selectors for the next client of `breakOnNext` type;\n use `userId` for the next matching client owned by that user; use `sessionId` only when you\n already know the positive ID of an active NST session. Arm next-session/user-filtered attach\n before triggering the workload. A warning-form `sessionBound` with null identity means only\n correlation metadata failed — debugging remains active.\n- If a user-filtered attach reports `fatal` before `sessionBound`, Business Central rejected the\n user filter. The client tears down that debugger and does not bind or deliver breaks; fix the\n user ID or permissions before retrying.\n- **Session must pause once before breakpoints bind.** `AddBreakpoint` fails server-side\n (\"tenant '' not found\") until the session has paused at least once. That is why you\n attach with `breakOnError: true` and add breakpoints at the first break. Once the\n session is live, breakpoints added for later code in the same run fire normally.\n- **Line numbers are 1-based** everywhere in these tools — use the line you see in your\n editor. (The BC wire is 0-based; conversion is internal.)\n- **Check breakpoint verification.** Each added breakpoint returns `verification.status`.\n `verified` means the server accepted the requested line, `relocated` means it moved to the\n returned 1-based span, and `unverified` means the server returned a usable breakpoint ID without\n a resolved span, including the all-zero value it uses for an unset span. Use a non-null returned\n span rather than assuming the requested line will fire.\n- **Watch expressions are paths, not code.** `bcdev_debug_eval` resolves identifier /\n member paths only (`CustomerName`, `Customer.\"No.\"`). Operators come back\n `` — and leave a synthetic empty-method entry in the test-run summary.\n- **Variable expansion:** nodes with `hasChildren: true` expand via\n `bcdev_debug_variables { frameId, expand: \"\" }`, e.g. `\"Customer\"` or\n `\"Customer.Fields\"`. Globals live under the `` node (or pass\n `globals: true`).\n- **Variable changes come from Business Central.** Each variable/watch node has `changeState`\n (`unchanged`, `new`, `valueChanged`, `descendantChanged`, or `unknown`) and `changed` is true\n for the three changed states. On a first read, or when the server omits the wire flag,\n `unknown` is not proof that a value stayed the same.\n- **Pages double-break:** page triggers fire twice per interaction on BC28 — expect two\n identical breaks, continue through the second.\n- **Test results arrive on the `testRunFinished` event** from `bcdev_debug_wait`, not from\n `bcdev_debug_run_tests` (which only starts the run).\n- `break` events map object IDs back to local `.al` files (`file` on the event and each\n stack frame) when the objects exist in the project.\n- **A break frame with no local `file`** (third-party extension, generated object) is readable\n anyway: `bcdev_source { objectType, objectId }` fetches the server's deployed source. Empty\n content with `isAlContent: false` means the object ships without source (base application).\n- **SQL triage:** attach with `sqlInsight: true` (optionally `longRunningSqlThresholdMs`), then\n at each break `bcdev_debug_sql` returns current latency, execute count, and the last SQL\n statements with duration and approximate rows read. It errors if insight was not enabled at\n attach — the option cannot be turned on mid-session.\n- **abort vs release at a break** (both end debugging with a `detached` event): `abort` kills\n the paused operation — a bound test run records it as failed; `release` lets it finish\n undebugged — a passing test still passes. Neither leaves a reusable session: a debug-bound\n test session ends with its run, so re-attach fresh instead of exact-targeting the old ID.\n- **Precision breaks:** `breakOnError: \"unhandled\"` skips errors a try function catches and\n still breaks on uncaught ones; `breakOnRecordWrite: \"nonTemporary\"` skips temporary-record\n writes and still breaks on real-table writes.\n- **Responses guide the loop.** Every successful tool result includes `nextSteps` (possibly\n empty for terminal results). MCP errors have `isError: true`, no `structuredContent`, and a\n JSON text body with stable `error.code`, `retryable`, redacted detail, and recovery steps.\n" + "text": "---\nname: bc-al-debugging\ndescription: Interactively debug AL code on a Business Central server with the bc-dev-mcp tools — attach, breakpoints, break-on-error, step, inspect variables and watches, or collect the stacks that write a table. Use when asked to debug AL code, find why an AL test fails, identify table writers, or inspect live state on a BC server.\n---\n\n# Debugging AL on Business Central\n\n## The loop\n\n1. `bcdev_debug_attach { breakOnError: true }` — arm the next WebClient session WITHOUT\n file/line breakpoints (see \"session must pause first\" below). To filter that next session,\n add `userId`; to attach to a known active NST session, add its positive `sessionId` instead.\n The selectors are mutually exclusive, and exact `sessionId` targeting takes precedence over\n `breakOnNext`. One debugger at a time; `bcdev_debug_detach` before re-attaching.\n2. `bcdev_debug_run_tests { codeunits: [{ id: }] }` — starts the run bound to\n this debug session and returns immediately.\n3. `bcdev_debug_wait` — long-poll for events. Attach returns before a workload binds; a successful\n bind reports `kind: sessionBound` with `sessionId` and `hostId` (the optional `hostId` may be null). **A timeout is a normal result**\n (`timedOut: true`): just call it again. Other events carry `kind`: `break`,\n `testRunFinished` (with the test results embedded), `detached`, `fatal`.\n If waiting times out before binding, confirm that you actually created or triggered the matching\n WebClient/workload, then wait again; waiting alone cannot create a next-session target.\n4. At a `break`: `bcdev_debug_variables { frameId: 0 }` for locals,\n `bcdev_debug_eval { frameId: 0, expression: \"...\" }` for watches,\n `bcdev_debug_breakpoints { add: [...] }` to place file/line breakpoints.\n5. `bcdev_debug_continue { action: \"continue\" | \"stepOver\" | \"stepInto\" | \"stepOut\" }`,\n then `bcdev_debug_wait` again.\n6. `bcdev_debug_detach` when done.\n\n## Find everything that writes one table\n\nUse the dedicated background workflow instead of manual `bcdev_debug_*` stepping:\n\n1. `bcdev_record_writes_start { tableId: }`.\n Add exactly one of `sessionId` or `userId` when needed, or choose `breakOnNext`.\n The tool returns after attach is armed; it does not wait for the session or workload.\n2. Trigger the matching session and operation. Business Central breaks on every record write\n globally; the collector identifies the runtime receiver, counts unrelated writes, groups exact\n target-table stacks, and automatically continues. Do not call `bcdev_debug_wait` or\n `bcdev_debug_continue`.\n3. `bcdev_record_writes_status` reads progress without driving it. When the target operation has\n finished, call `bcdev_record_writes_finish`.\n4. Review `writers`, `unresolved`, `warnings`, and `complete`. A complete report covers only the\n observed breaks in this one attached-session capture window; it is not a tenant-wide proof.\n\nTemporary-record writes are excluded by default. Pass `includeTemporary: true` only when they\nmatter. Deployed source is trusted; local source can prove a receiver only after you have published\nthat exact source and explicitly pass `changesDeployed: true`. Without that assertion, local-only\nmapping remains unresolved. `maxObservedWrites` defaults to 500; reaching it releases the workload\nand returns a truncated, incomplete report. Missing source/span, an unsupported receiver, watch\nfailure, unknown runtime table identity, or debugger failure also keeps the result fail-closed.\nIf the session detaches before `bcdev_record_writes_finish`, retained exact groups remain useful\nbut `complete` is false because later writes cannot be ruled out.\nFinish always clears the shared debugger slot; manual debugging and record-write triage cannot run\nat the same time.\n\n## Facts that save you time\n\n- **Choose one targeting mode.** Omit both selectors for the next client of `breakOnNext` type;\n use `userId` for the next matching client owned by that user; use `sessionId` only when you\n already know the positive ID of an active NST session. Arm next-session/user-filtered attach\n before triggering the workload. A warning-form `sessionBound` with null identity means only\n correlation metadata failed — debugging remains active.\n- If a user-filtered attach reports `fatal` before `sessionBound`, Business Central rejected the\n user filter. The client tears down that debugger and does not bind or deliver breaks; fix the\n user ID or permissions before retrying.\n- **Session must pause once before breakpoints bind.** `AddBreakpoint` fails server-side\n (\"tenant '' not found\") until the session has paused at least once. That is why you\n attach with `breakOnError: true` and add breakpoints at the first break. Once the\n session is live, breakpoints added for later code in the same run fire normally.\n- **Line numbers are 1-based** everywhere in these tools — use the line you see in your\n editor. (The BC wire is 0-based; conversion is internal.)\n- **Check breakpoint verification.** Each added breakpoint returns `verification.status`.\n `verified` means the server accepted the requested line, `relocated` means it moved to the\n returned 1-based span, and `unverified` means the server returned a usable breakpoint ID without\n a resolved span, including the all-zero value it uses for an unset span. Use a non-null returned\n span rather than assuming the requested line will fire.\n- **Watch expressions are paths, not code.** `bcdev_debug_eval` resolves identifier /\n member paths only (`CustomerName`, `Customer.\"No.\"`). Operators come back\n `` — and leave a synthetic empty-method entry in the test-run summary.\n- **Variable expansion:** nodes with `hasChildren: true` expand via\n `bcdev_debug_variables { frameId, expand: \"\" }`, e.g. `\"Customer\"` or\n `\"Customer.Fields\"`. Globals live under the `` node (or pass\n `globals: true`).\n- **Variable changes come from Business Central.** Each variable/watch node has `changeState`\n (`unchanged`, `new`, `valueChanged`, `descendantChanged`, or `unknown`) and `changed` is true\n for the three changed states. On a first read, or when the server omits the wire flag,\n `unknown` is not proof that a value stayed the same.\n- **Pages double-break:** page triggers fire twice per interaction on BC28 — expect two\n identical breaks, continue through the second.\n- **Test results arrive on the `testRunFinished` event** from `bcdev_debug_wait`, not from\n `bcdev_debug_run_tests` (which only starts the run).\n- `break` events map object IDs back to local `.al` files (`file` on the event and each\n stack frame) when the objects exist in the project.\n- **A break frame with no local `file`** (third-party extension, generated object) is readable\n anyway: `bcdev_source { objectType, objectId }` fetches the server's deployed source. Empty\n content with `isAlContent: false` means the object ships without source (base application).\n- **SQL triage:** attach with `sqlInsight: true` (optionally `longRunningSqlThresholdMs`), then\n at each break `bcdev_debug_sql` returns current latency, execute count, and the last SQL\n statements with duration and approximate rows read. It errors if insight was not enabled at\n attach — the option cannot be turned on mid-session.\n- **abort vs release at a break** (both end debugging with a `detached` event): `abort` kills\n the paused operation — a bound test run records it as failed; `release` lets it finish\n undebugged — a passing test still passes. Neither leaves a reusable session: a debug-bound\n test session ends with its run, so re-attach fresh instead of exact-targeting the old ID.\n- **Precision breaks:** `breakOnError: \"unhandled\"` skips errors a try function catches and\n still breaks on uncaught ones; `breakOnRecordWrite: \"nonTemporary\"` skips temporary-record\n writes and still breaks on real-table writes.\n- **Responses guide the loop.** Every successful tool result includes `nextSteps` (possibly\n empty for terminal results). MCP errors have `isError: true`, no `structuredContent`, and a\n JSON text body with stable `error.code`, `retryable`, redacted detail, and recovery steps.\n" }, { "name": "bc-al-testing", diff --git a/src/mcp/tools/record-write-tools.ts b/src/mcp/tools/record-write-tools.ts index 2aa225d..1c7b0a6 100644 --- a/src/mcp/tools/record-write-tools.ts +++ b/src/mcp/tools/record-write-tools.ts @@ -47,6 +47,7 @@ const unresolvedSchema = z.object({ "sourceUnavailable", "statementSpanUnavailable", "writeStatementUnrecognized", + "multipleWriteCandidates", "receiverUnsupported", "receiverUnavailable", "receiverTypeUnresolved", @@ -194,7 +195,9 @@ export function createRecordWriteTools(state: ServerState, deps: ToolDeps): Tool const { config, authorization, project } = resolve(params, deps); const index = await AlObjectIndex.build(project); collector = new RecordWriteCollector({ - ...options, + tableId: options.tableId, + changesDeployed: options.changesDeployed, + maxObservedWrites: options.maxObservedWrites, client, localSource: async (objectType, objectId) => { const file = index.byId(objectType, objectId)?.file; diff --git a/tests/core/record-write-triage.test.ts b/tests/core/record-write-triage.test.ts index e05f194..71f2fe9 100644 --- a/tests/core/record-write-triage.test.ts +++ b/tests/core/record-write-triage.test.ts @@ -46,9 +46,13 @@ function spanFor(source: string, needle: string): NonNullable { expect(parseRecordWriteStatement("A.Insert(); B.Modify();")).toBeNull(); expect(parseRecordWriteStatement("Records[1].Insert();")).toBeNull(); expect(parseRecordWriteStatement("GetCustomer().Modify();")).toBeNull(); + expect(parseRecordWriteStatement("Outer.Inner.Insert();")).toBeNull(); expect(parseRecordWriteStatement("Customer.Validate(Name);")).toBeNull(); expect(inspectRecordWriteStatement("Records[1].Insert();").reason).toBe("receiverUnsupported"); expect(inspectRecordWriteStatement("GetCustomer().Modify();").reason).toBe("receiverUnsupported"); - expect(inspectRecordWriteStatement("A.Insert(); B.Modify();").reason).toBe("writeStatementUnrecognized"); + expect(inspectRecordWriteStatement("Outer.Inner.Insert();").reason).toBe("receiverUnsupported"); + expect(inspectRecordWriteStatement("A.Insert(); B.Modify();").reason).toBe("multipleWriteCandidates"); + expect( + inspectRecordWriteStatement( + "if not Customer.Insert(false) then\n Customer.Modify(true);", + ).reason, + ).toBe("multipleWriteCandidates"); + expect( + inspectRecordWriteStatement("Customer.Insert(); GetCustomer().Modify();").reason, + ).toBe("receiverUnsupported"); }); test("parses only positive runtime Table identities", () => { @@ -217,7 +231,6 @@ describe("RecordWriteCollector", () => { const fake = client(["Table Customer (18)", "Table Vendor (23)", "Table Customer (18)"]); const collector = new RecordWriteCollector({ tableId: 18, - includeTemporary: false, changesDeployed: false, maxObservedWrites: 10, client: fake as never, @@ -263,7 +276,6 @@ describe("RecordWriteCollector", () => { const fake = client(["Integer"]); const collector = new RecordWriteCollector({ tableId: 18, - includeTemporary: false, changesDeployed: false, maxObservedWrites: 10, client: fake as never, @@ -279,11 +291,42 @@ describe("RecordWriteCollector", () => { expect(report.unresolved[0]?.reason).toBe("receiverTypeUnresolved"); }); + test("a conditional statement with two write candidates is retained under its stable reason", async () => { + const statement = "if not Customer.Insert(false) then\n Customer.Modify(true);"; + const source = SOURCE.replace( + "Customer.Modify(\n true);", + statement, + ); + const fake = client(["Table Customer (18)"]); + fake.getSourceContent = async () => ({ content: source, isAlContent: true }); + const collector = new RecordWriteCollector({ + tableId: 18, + changesDeployed: false, + maxObservedWrites: 10, + client: fake as never, + localSource: async () => null, + localFile: () => undefined, + }); + collector.onEvent(brk([{ + ...frame(), + statementSpan: spanFor(source, statement), + }])); + await collector.waitForIdle(); + + const report = await collector.finish(); + expect(report).toMatchObject({ + complete: false, + summary: { observedWrites: 1, unresolvedWrites: 1 }, + unresolved: [{ reason: "multipleWriteCandidates", operation: null, receiver: null }], + }); + expect(fake.watchExpressions).toEqual([]); + expect(fake.steps).toEqual(["continue"]); + }); + test("finishing before any session binds cannot produce a false complete report", async () => { const fake = client(["Table Customer (18)"]); const collector = new RecordWriteCollector({ tableId: 18, - includeTemporary: false, changesDeployed: false, maxObservedWrites: 10, client: fake as never, @@ -308,7 +351,6 @@ describe("RecordWriteCollector", () => { fake, collector: new RecordWriteCollector({ tableId: 18, - includeTemporary: false, changesDeployed, maxObservedWrites: 10, client: fake as never, @@ -334,7 +376,6 @@ describe("RecordWriteCollector", () => { const fake = client(["Table Customer (18)", "Table Customer (18)"]); const collector = new RecordWriteCollector({ tableId: 18, - includeTemporary: false, changesDeployed: false, maxObservedWrites: 2, client: fake as never, @@ -360,7 +401,6 @@ describe("RecordWriteCollector", () => { const fake = client(["Table Customer (18)", "Table Customer (18)"]); const collector = new RecordWriteCollector({ tableId: 18, - includeTemporary: false, changesDeployed: false, maxObservedWrites: 10, client: fake as never, @@ -385,7 +425,6 @@ describe("RecordWriteCollector", () => { fake.getSourceContent = async () => ({ content: caseSource, isAlContent: true }); const collector = new RecordWriteCollector({ tableId: 18, - includeTemporary: false, changesDeployed: false, maxObservedWrites: 10, client: fake as never, @@ -439,7 +478,6 @@ describe("RecordWriteCollector", () => { }; const collector = new RecordWriteCollector({ tableId: 18, - includeTemporary: false, changesDeployed: false, maxObservedWrites: 10, client: fake as never, @@ -468,7 +506,6 @@ describe("RecordWriteCollector", () => { const fake = client(["Table Customer (18)"]); const collector = new RecordWriteCollector({ tableId: 18, - includeTemporary: false, changesDeployed: false, maxObservedWrites: 10, client: fake as never, @@ -497,7 +534,6 @@ describe("RecordWriteCollector", () => { const fake = client(["Table Customer (18)"]); const collector = new RecordWriteCollector({ tableId: 18, - includeTemporary: false, changesDeployed: false, maxObservedWrites: 10, client: fake as never, @@ -536,7 +572,6 @@ describe("RecordWriteCollector", () => { }; const collector = new RecordWriteCollector({ tableId: 18, - includeTemporary: false, changesDeployed: false, maxObservedWrites: 10, client: fake as never, @@ -553,11 +588,10 @@ describe("RecordWriteCollector", () => { expect(report.warnings.join(" ")).toContain("[REDACTED]"); }); - test("detach and fatal events preserve retrievable partial reports", async () => { + test("mid-capture detach and fatal events fail closed while preserving partial reports", async () => { const detachedFake = client(["Table Customer (18)"]); const detached = new RecordWriteCollector({ tableId: 18, - includeTemporary: false, changesDeployed: false, maxObservedWrites: 10, client: detachedFake as never, @@ -567,16 +601,18 @@ describe("RecordWriteCollector", () => { detached.onEvent(brk()); detached.onEvent({ kind: "detached", terminateSession: false }); await detached.waitForIdle(); - expect(await detached.finish()).toMatchObject({ + const detachedReport = await detached.finish(); + expect(detachedReport).toMatchObject({ outcome: "completed", stopReason: "sessionDetached", + complete: false, summary: { observedWrites: 1, matchedWrites: 1 }, }); + expect(detachedReport.warnings.join(" ")).toContain("detached before finish"); const fatalFake = client(["Table Customer (18)"]); const fatal = new RecordWriteCollector({ tableId: 18, - includeTemporary: false, changesDeployed: false, maxObservedWrites: 10, client: fatalFake as never, @@ -590,6 +626,57 @@ describe("RecordWriteCollector", () => { expect(failed.warnings.join(" ")).not.toContain("fatal-secret"); }); + test("a detach caused by finish's release barrier does not mark the deliberate window incomplete", async () => { + const fake = client(["Table Customer (18)"]); + const collector = new RecordWriteCollector({ + tableId: 18, + changesDeployed: false, + maxObservedWrites: 10, + client: fake as never, + localSource: async () => null, + localFile: () => undefined, + }); + collector.onEvent({ kind: "sessionBound", sessionId: 99, hostId: null }); + await collector.waitForIdle(); + fake.releaseForShutdown = async () => { + collector.onEvent({ kind: "detached", terminateSession: false }); + return true; + }; + + const report = await collector.finish(); + expect(report).toMatchObject({ + outcome: "completed", + stopReason: "finished", + complete: true, + summary: { observedWrites: 0 }, + }); + expect(report.warnings).toEqual([]); + }); + + test("a terminated-session detach still fails closed because the detach cause is not provable", async () => { + const fake = client(["Table Customer (18)"]); + const collector = new RecordWriteCollector({ + tableId: 18, + changesDeployed: false, + maxObservedWrites: 10, + client: fake as never, + localSource: async () => null, + localFile: () => undefined, + }); + collector.onEvent(brk()); + collector.onEvent({ kind: "detached", terminateSession: true }); + await collector.waitForIdle(); + + const report = await collector.finish(); + expect(report).toMatchObject({ + outcome: "completed", + stopReason: "sessionDetached", + complete: false, + summary: { observedWrites: 1, matchedWrites: 1 }, + }); + expect(report.warnings.join(" ")).toContain("detached before finish"); + }); + test("a continuation failure becomes fatal and attempts to release the paused workload", async () => { const fake = client(["Table Customer (18)"]); fake.step = async (action: string) => { @@ -598,7 +685,6 @@ describe("RecordWriteCollector", () => { }; const collector = new RecordWriteCollector({ tableId: 18, - includeTemporary: false, changesDeployed: false, maxObservedWrites: 10, client: fake as never, @@ -617,7 +703,6 @@ describe("RecordWriteCollector", () => { const fake = client(["Table Customer (18)", 'Table "Customer Alias" (18)']); const collector = new RecordWriteCollector({ tableId: 18, - includeTemporary: false, changesDeployed: false, maxObservedWrites: 10, client: fake as never, diff --git a/tests/mcp/record-write-tools.test.ts b/tests/mcp/record-write-tools.test.ts index 5356baa..06634f8 100644 --- a/tests/mcp/record-write-tools.test.ts +++ b/tests/mcp/record-write-tools.test.ts @@ -312,6 +312,33 @@ describe("record-write MCP tools", () => { expect(report.warnings.join(" ")).toContain("connection closed unexpectedly"); }); + test("a mid-capture detach returns incomplete evidence and cautionary guidance", async () => { + const hub = new FakeHub(); + hub.onInvoke = (method) => { + if (method === "GetSourceContent") return { Content: SOURCE, IsALContent: true }; + if (method === "GetWatchNode") { + return { Name: "Customer", TypeName: "Table Customer (18)", Summary: "", HasChildren: true }; + } + return undefined; + }; + const { tools } = setup(hub); + await tools.get("bcdev_record_writes_start")!.handler({ tableId: 18 }); + emitWrite(hub); + hub.emit("OnDetachedFromConnection", false); + await Bun.sleep(0); + await Bun.sleep(0); + + const report = await tools.get("bcdev_record_writes_finish")!.handler({}) as { + complete: boolean; + stopReason: string; + warnings: string[]; + nextSteps: string[]; + }; + expect(report).toMatchObject({ complete: false, stopReason: "sessionDetached" }); + expect(report.warnings.join(" ")).toContain("detached before finish"); + expect(report.nextSteps.join(" ")).toContain("before treating"); + }); + test("an out-of-band user-filter rejection fails closed and preserves a partial report", async () => { const hub = new FakeHub(); const { state, tools } = setup(hub);