[ianrayianray]

Summary

This PR completes roadmap item 9: On-demand source & symbols.

• bcdev_source gets the deployed AL source for one object and keeps the existing REST/debugger-hub behavior.
• The new bcdev_package_download gets one installed dependency package and writes it to the current AL project's .alpackages directory.
• The package selector uses publisher, app name, a four-part minimum version, and an optional app ID.
• Each successful result reports the requested and resolved versions, validated package identity, file path, byte count, SHA-256 value, and install status.
• A new embedded skill explains when an agent must get source and when it must get symbols.

Package behavior

• The tool sends one GET dev/packages request. It does not list applications, resolve transitive dependencies, or synchronize all symbols.
• It requires an existing AL project and app.json before authorization or the request.
• Requests default to 120 seconds and 256 MiB. Callers can raise those limits to hard caps of 300 seconds and 512 MiB.
• SymbolReference.json inflation has an independent 512 MiB allocation cap.
• It validates the NAVX/ZIP archive and the identity in SymbolReference.json.
• It rejects a different publisher, name, requested app ID, or a version below the requested minimum.
• It derives a Windows-safe filename from validated identity fields instead of trusting a response filename.
• It writes a temporary file and moves it to the final path only after validation.
• It returns unchanged for the same bytes and replaced for different validated bytes.
• It serializes simultaneous writes to the same package path.
• It omits appId only for the live-validated Microsoft Application concept; third-party apps with that name keep ordinary app-ID selection.

Compatibility and error handling

• The bcdev_source input and result fields stay unchanged.
• An empty source result explains that the application can be compiled-only or can disable source exposure.
• Authorization failures keep their typed error instead of becoming endpoint failures.
• Source connection errors do not include the authenticated request URL.
• Package HTTP failures use stable authentication, not-found, server, timeout, and endpoint error codes.
• A 404 states that no package matched or the server does not expose dev/packages; details retain a supplied app ID and report whether it was sent.
• The tool cancels response bodies that it does not use.
• .alpackages must be a direct directory, not a symlink or Windows junction.
• Windows backup-swap success and restoration paths are covered through injected filesystem faults.
• The tool returns package metadata only. It does not return the package body through MCP.

Validation

• bun test: 668 passed, 0 failed
• bun run typecheck: passed
• bun run build: passed
• Embedded-skill drift check: passed

The built MCP server was also tested against a Business Central SaaS Sandbox. The test covered:

• Deployed source and an empty no-source result
• Exact package selection and selection without app ID
• Minimum-version resolution
• Repeat download and corrupt-cache replacement
• Two simultaneous downloads to the same destination
• A large Microsoft Base Application package
• The special Microsoft Application concept
• Too-new, wrong-ID, unknown-name, and invalid-project errors
• Cleanup of temporary and backup files

All Business Central calls in the live test used GET. The test wrote packages only to a temporary local AL project and removed that project afterward. No Production calls were made.

Post-review hardening was deterministic and made no additional Sandbox call: bounded DEFLATE output, backup-swap success/restoration, Windows junction coverage, adjustable bounded limits, selector length validation, and corrected error details/provenance.

Redacted evidence is in scripts/e2e-on-demand-source-symbols-2026-07-30.md.

Integration

This branch is rebased onto roadmap item 6 after #12 merged. The combined tree exposes 24 tools, serves four embedded skills, reports the measured 668-test total, and reuses the shared AL identifier normalization utility.

[SShadowS]

Review

Reviewed on a clean worktree of 1cfc265, plus a second independent pass from another model with live measurements. Verified locally: bun install --frozen-lockfile clean, bun run typecheck clean, bun run build clean with zero embed-skills drift, git status clean after build.

bun test reports 644 pass / 0 fail across 44 files on Windows, not the 645 in the PR description and README badge. Not a miscount: tests/core/package-download.test.ts:403 gates the symlink test behind process.platform !== "win32", so the dated evidence file's 645 was a non-Windows run. That matters for a reason beyond the badge, see finding 3.

This is careful work on a genuinely dangerous surface. Most of what looks scary here is actually handled, so the explicit clean verdicts are listed too, to save the next reviewer from re-deriving them.

---

1. inflateRawSync has no output cap on a newly untrusted path

src/core/snapshot/zip.ts:69, reached from packageIdentity at src/core/package-download.ts:166.

The download itself is bounded to 256 MiB (:242-275), and the identity is validated before anything is written. But the inflation of SymbolReference.json is unbounded: Node's default maxOutputLength is the 4 GB Buffer limit.

Concrete scenario: on-prem dev endpoints are plain HTTP (the project's own live box is http://Cronus28:7049). A MITM or a compromised server returns a crafted NAVX whose DEFLATE entry inflates toward 4 GB. zlib allocates multi-GB RSS before ERR_BUFFER_TOO_LARGE finally lands as a typed PROTOCOL_ERROR. On a constrained box (CI, container) the MCP server process is OOM-killed first, taking any live debug session or run lock with it.

zip.ts is pre-existing and unchanged by this PR, but this PR is what starts feeding it a 256 MiB network archive. The PR already treats the server as untrusted enough to bound the archive and cross-check the identity, so leaving the inflation unbounded is internally inconsistent. Fix is one line: thread a maxOutputLength through extractEntry into inflateRawSync (512 MiB, or 8x maxBytes), plus one test.

2. WIRE provenance cites a reference directory that does not exist

package-download.ts:100-102 and :232-235 cite al-codeanalysis-decomp (SymbolReferenceSpecification.IsApplicationConceptReference, NavAppManifest.cs, FileNameUtilities.cs). reference/al18-decompiled/ contains only almcp-decomp, dep-decomp, esp-decomp, lmt-decomp, tw-decomp. These are the only two citations in the whole of src/ pointing at a nonexistent directory, so the project's provenance rule is not satisfied for either.

The practical consequence is a behaviour nobody can now check: whether IsApplicationConceptReference is name-only case-insensitive, as :89 and :103 implement, or also requires publisher Microsoft. If the latter, a third-party app literally named "Application" silently loses appId enforcement. Impact is negligible because the publisher/name/version cross-check still holds, but a deviation from the official client should be knowable rather than inferred.

Separately, :109 claims "Validated live on BC28 on-prem 2026-07-04" while the dated evidence file covers SaaS 2026-07-30 only. The on-prem half of that claim has no listed evidence.

3. The backup-swap branch has zero automated coverage on any platform

replaceValidatedFile:343-351.

POSIX rename replaces atomically, and Windows fs.rename uses MoveFileEx with REPLACE_EXISTING, so the replaced tests never enter the EEXIST/EPERM path on either platform. It only fires on a locked or read-only destination: alc.exe holding the .app open, or antivirus. That is precisely the situation where the restore-on-failure path and the orphaned .backup cleanup matter, and neither is pinned. rename is imported directly, so exercising it needs either a small injection seam or mock.module.

Related, and the more useful half: the symlink test at :403-414 is skipped on win32, which is this project's stated primary dev platform. The guard itself does work there. A junction is creatable with zero privileges (New-Item -ItemType Junction) and Node's lstat().isSymbolicLink() returns true for one, so :378-380 rejects it. Replacing the platform skip with a junction-based test would pin the guard where it is most likely to be needed, and would also reconcile the 644/645 count.

4. Nits

• No length cap on publisher/appName, so an absurdly long name fails late as a CONFIGURATION_ERROR on MAX_PATH rather than as validation.
• :463: the NOT_FOUND details report appId: null for the Application concept even when the caller supplied one. Accurate about the request, potentially confusing about the input.
• :452-466: a pre-2.0 server missing the dev/packages route 404s into "No installed Business Central package matched ...", conflating route-absent with package-absent. bcdev_source documents exactly this conflation at source-content.ts:56-58; this path does not.
• installPackage:376-384 wraps the mkdir, the lstat, and the symlink rejection in one try, so a rejected symlinked .alpackages surfaces as "Unable to create the AL package directory", which is misleading.
• A crash in the window between the two renames in replaceValidatedFile leaves the package parked at .backup and absent from .alpackages until re-download. Self-healing, worth a comment.
• maxBytes and timeoutMs are PackageDownloadOptions but are not exposed on the tool schema, so an agent hitting either bound has no recourse.

---

Checked and found safe

Listing these explicitly so they do not get re-raised.

• Filename derivation is safe, and stronger than the official client's. packageFilename:237 strips exactly .NET's Windows Path.GetInvalidFileNameChars set (controls plus <>:"/\|?*), so no separators and no ADS colon survive. The server can only vary the case of publisher and appName, because sameText:200 cross-checks both against the caller's selector before the filename is built, and the version is forced through parseVersion:196 to \d+\.\d+\.\d+\.\d+. The name therefore always ends _d.d.d.d.app, so it can never be .., never a bare Windows device name (underscores are always present), and never ends in a dot or space. Traversal is impossible. For contrast, dep-decomp PackagesApiClient.DownloadPackage does Path.Combine(directory, ContentDisposition.FileName) with the server's unsanitized filename; the identity-derived naming here is a deliberate improvement and is worth keeping.
• Memory is fine for legitimate packages, measured rather than estimated. Against the real Microsoft_Base Application_28.0.46665.47126.app (43.7 MB): SymbolReference.json inflates to 55.6 MB, JSON.parse takes 239 ms, peak RSS 425 MB, peak heapUsed about 120 MB, since the object graph is dropped immediately after the four scalars are read. Worst legitimate transient at the 256 MiB cap is roughly 770 MB of off-heap buffers. The only unbounded axis is finding 1. (There is a redundant full-buffer copy at zip.ts:56, Buffer.from(zip) on an already-Buffer, but at these sizes it is not worth a change on its own.)
• withInstallLock:354-369 cannot deadlock or poison the chain. release() runs in finally, so every link resolves regardless of whether the operation rejects, and by induction await previous never throws. Map entries are deleted when the finishing tail is still current, so growth is bounded by in-flight distinct paths. Cross-process is unguarded but outcome-safe: temps are wx plus UUID, renames are atomic, and both writers install only fully validated bytes, so the worst case is an orphaned .tmp/.backup that alc ignores.
• No credential or URL leak. agentErrorBody (src/mcp/agent-errors.ts:103-116) serializes only code, category, message, retryable, tool, and details, so the { cause } attached at package-download.ts:487,496 never reaches the MCP body. The message passes redactAuthorization, details are key- and value-redacted, and the ENDPOINT_UNREACHABLE/TIMEOUT messages contain no URL by construction. The source-content.ts change correctly hoists getAuthorizationHeader() out of the try so typed auth failures are no longer swallowed into endpoint errors, and drops ${url}/${err} from the unreachable message. Both are pinned by tests.
• Hostile zip input other than finding 1 is handled. Negative archiveBase, oversized centralSize, zip64 sentinels, and truncated entries all end in a Buffer RangeError or an explicit throw, wrapped as PROTOCOL_ERROR at :168 with no partial install, and the corrupt-archive tests assert no temp or package residue. findEocd's backward scan over a 256 MiB non-zip 200 response measures 520 ms, which is acceptable. No silent-misparse path found.
• Tool wiring is correct. The output schema matches PackageDownloadResult field for field and outputSchema.parse(result) is pinned in tools.test; project resolves through the same resolve() as every other tool.
• Tests are meaningful, not happy-path. The size cap is covered on both the content-length and the streamed path, and corrupt archive, missing symbols, bad JSON, identity and version mismatch, authorization ordering, abort signal, and no-request-on-bad-project are all pinned with real assertions. The gaps are exactly finding 3, plus no bomb case (which finding 1's fix should bring with it) and no DEFLATE-method fixture, since tests/fixtures/app-package.ts builds STORE-only archives and the real .app DEFLATE path is therefore only exercised in live E2E.

---

Merge gate

• Borderline block: finding 1. The failure mode is process OOM rather than compromise, so it is not a hard block, but it is a one-line hardening on the new untrusted-input path this PR introduces, and the PR's own threat model already implies it.
• Should fix: finding 2 (re-cite or add the directory to the documented layout, and reconcile the on-prem evidence claim at :109), finding 3 (junction-based test on win32, and an injection seam or mock for the backup-swap branch).
• Nits: everything in section 4.

---

One cross-PR note

#12 and this PR both take the tool count from 22 to 23, and both rewrite the test badge. Whichever merges second will be wrong on both (the count should end at 24, and the badge at the combined total). Worth resolving deliberately rather than in a conflict resolution.

[SShadowS]

Heads-up: #12 merged as 5d58cb7, so this branch now conflicts (mergeable_state: dirty) and needs a rebase onto main.

Beyond the mechanical conflict resolution, four things to get right:

1. Tool count is now 24, not 23. Both PRs independently wrote 22 to 23. After this one lands the real count is 24. Three places:

• README.md, architecture diagram: tools/ (23 bcdev_* tools)
• README.md, file table: The 23 bcdev_* tool definitions
• scripts/e2e.md: tools/list shows title/annotations/outputSchema for all 23 tools

The e2e.md line also says "the three skill:// resources" on main and "four" on this branch. Four is correct once bc-al-source-symbols lands, since #12 added no skill.

2. Test badge. #12 landed at 641. This branch's 645 was measured against the old base, so the post-rebase number will be different again. Worth re-running rather than arithmetic, and note the count is platform-dependent (see below).

3. upperInvariantUtf16 now has a shared home. #12's review-fix commit extracted it to src/core/al-identifiers.ts and pointed both test-orchestration.ts and tools/shared.ts at it. package-download.ts:117-125 currently carries its own private copy of the same function, so please import the shared one on rebase rather than leaving a third copy in the tree.

4. Overlapping-file conflicts to expect: src/mcp/tools/agent-response.ts and src/mcp/skills.generated.ts were touched by both PRs. Regenerate the latter with bun run embed-skills rather than hand-merging it.

None of this changes the review verdict already posted. The open items from that review are still finding 1 (inflateRawSync output cap), finding 2 (the al-codeanalysis-decomp citations and the on-prem evidence claim at package-download.ts:109), and finding 3 (junction-based symlink test on win32 plus coverage for the backup-swap branch).

On finding 3 and the badge: the 645 was a non-Windows run. tests/core/package-download.test.ts:403 skips the symlink test on win32, so the suite is 644 there. A junction-based test would both pin the guard on the primary dev platform and remove the platform-dependent count.

[ianrayianray]

Resolved the review in 906eecf.

• ZIP extraction now accepts an output ceiling, checks the declared uncompressed size, and passes maxOutputLength to DEFLATE so a lying archive remains bounded. Package symbols use an independent 512 MiB ceiling, with both understated-DEFLATE and package-level regression tests.
• Package wire comments now cite the tracked 2026-07-30 SaaS evidence. The nonexistent decompilation references and unsupported on-prem package claim are gone. The special omission is limited to the live-validated Microsoft/Application identity.
• The Windows backup swap has an injected filesystem seam and deterministic success/cleanup plus failure/restore tests.
• The linked-directory test now runs on every platform and creates a junction on Windows; the error identifies a non-direct directory.
• Publisher/name lengths are bounded, 404 details retain the supplied app ID and state whether it was sent, route-absent ambiguity is documented, and crash-between-renames behavior is recorded.
• timeoutMs and maxBytes are exposed with bounded defaults and hard caps.

Verification on the pushed head: 649 tests passed, typecheck passed, build passed, and embedded-skill drift passed. The review corrections were deterministic and made no additional Sandbox call; the existing redacted SaaS evidence remains the source for the wire behavior.

[ianrayianray]

Rebased the corrected branch onto merged #12 and force-pushed c053e9f.

• The PR is now based on upstream 5d58cb7; it no longer has the reported dirty merge state.
• Combined tool assertions and docs now use 24 tools and four skills.
• package-download.ts imports the shared upperInvariantUtf16 utility introduced by feat: add test orchestration #12; the private duplicate is gone.
• skills.generated.ts was regenerated from all four skill sources.
• The combined suite was measured rather than inferred: 668 passed, 0 failed across 46 files. Typecheck and build also pass.

The PR description and test badge now reflect the rebased tree. The source/symbol review corrections remain unchanged and covered after the rebase.

[SShadowS]

Re-review at c053e9f

Second independent pass over the fix delta, plus my own verification. The branch moved twice during review, so this supersedes everything said about 906eecf: head is now c053e9f, rebased onto merged main (5d58cb7), mergeable_state: clean.

Verified on that head in a clean worktree: typecheck clean, build clean with zero embed-skills drift, bun test 668 pass / 0 fail across 46 files on Windows, matching the badge and the evidence file exactly. All four items from the rebase note are done: tool count reads 24 in the architecture diagram, the file table, and e2e.md; four skill:// resources; and upperInvariantUtf16 now exists once, in src/core/al-identifiers.ts, with no private copy left in package-download.ts.

Finding 1, inflate bound: genuinely closed

zip.ts:57-66 validates the cap and fast-fails on the declared central-directory size, zip.ts:77-80 bounds the STORE branch on actual data length, and zip.ts:83-87 passes maxOutputLength into inflateRawSync, which is the authoritative allocation bound when the archive lies about its size.

The test that matters is tests/core/snapshot/zip.test.ts:137: it forges uncompressedSize=1 against a real 4096-byte entry with a cap of 128, so the throw can only originate from maxOutputLength. That proves the authoritative path rather than just the declared-size shortcut. Package level is pinned at tests/core/package-download.test.ts:316 with no residue. maxSymbolBytes stays core-internal and is validated to 1..512 MiB at package-download.ts:487.

Two extractEntry callers remain uncapped, and both are defensible. al-procedures.ts:1227 is transitively bounded against a hostile server, because any .app this tool installs already had that same first-match SymbolReference.json entry inflated under the 512 MiB cap during packageIdentity, so a bomb cannot survive install and detonate later; anything else in .alpackages is a local user file. profile-tools.ts:184 is a pre-existing snapshot path with an unchanged trust model. Capping the latter is a reasonable separate follow-up, not work for this PR.

Finding 2, provenance: fixed

The al-codeanalysis-decomp citations and the unsupported on-prem 2026-07-04 claim are gone, and the WIRE comments now cite the dated evidence file. The e2e.md concept line records the narrowing with its deterministic-test provenance.

One consequence worth stating plainly: dev/packages now has no live on-prem evidence at all. It is SaaS-only, and the on-prem URL shape is pinned by unit test alone. A single Cronus28 run would close that, and is the other optional follow-up.

On the isApplicationConcept narrowing, no legitimate regression. The aggregate Application package's returned Publisher is "Microsoft", and the identity cross-check has always required the caller's publisher to match the returned one, so a non-Microsoft publisher plus the name "Application" could never have completed a concept download under the old code either; it failed at identity instead. The only real behaviour change is that a third-party app literally named "Application" now sends and enforces its appId (package-download.ts:95-115, URL pinned at tests/core/package-download.test.ts:94-99), where previously a wrong appId was silently ignored. That turns a silent mismatch into an error, which is the right direction.

Finding 3, backup-swap and junction tests: real, not seam theater

The injected seam substitutes only the rename and rm primitives, so the assertions still land on the real filesystem. The success test (tests/core/package-download.test.ts:170) injects one EPERM on the tmp-to-destination rename and asserts two attempts, destination bytes equal to the new package, and zero .backup files remaining. The restore test (:200) makes the destination rename fail persistently (EPERM then EACCES) and asserts the original bytes are back in place with no .backup or .tmp orphans. Both genuinely exercise replaceValidatedFile's restore branch at package-download.ts:377-386.

The junction test was mutation-verified rather than taken on trust: replacing the guard condition at package-download.ts:428 with if (false) in a scratch worktree makes the test fail, because the download then succeeds through the junction into the outside directory. symlinkSync(..., "junction") needs no privileges, so it runs on stock Windows dev boxes and CI.

New-breakage sweep on the restructure

Nothing introduced.

• The installPackage lstat move separates both failure paths (mkdir at :420, lstat at :424), and the guard throws a clean typed CONFIGURATION_ERROR at :428-435 instead of being double-wrapped by the mkdir message. A directory removed between the two calls lands in "Unable to inspect". No path is swallowed.
• appId versus requestedAppId: enforcement still keys on selector.appId, the value actually sent, while the NOT_FOUND details carry the supplied appId plus appIdSent. Pinned including the concept case where appIdSent is false (tests/core/package-download.test.ts:349-363).
• The new bounds cannot drift: zod (source-tools.ts:89-94) imports its maxima from the same core constants it validates against (package-download.ts:479-488), so zod-valid implies core-valid, and direct library callers still hit the core checks. The hard caps 300000 and 536870912 are pinned in tools.test. No gap in either direction.
• The integration commit's dedupe of upperInvariantUtf16 into al-identifiers.ts:4 is character-identical to the removed local copy, so sameText semantics are unchanged, including the Strasse case.

Nits, neither blocking

• If fileOps.remove(backup) fails after a successful swap (package-download.ts:386), the tool reports the whole install as failed even though the new package is correctly in place. Rare, since we just renamed that file ourselves, but a catch-with-warning would be friendlier.
• Academic: the 512 MiB symbol cap sits about 24 bytes above V8's max string length, so a SymbolReference.json landing in that sliver fails as a typed PROTOCOL_ERROR at toString rather than at the cap. Typed either way, nothing to do.

Verdict

Approve. All findings closed with substance, the closure I most doubted (the junction guard) is mutation-verified on Windows, the restructure introduced nothing new, and the rebase is already done with every gate green on the actual head.

Outstanding items are both optional follow-ups rather than blockers: cap profile-tools.ts:184 the same way, and record one on-prem dev/packages run against Cronus28 so the wire evidence is not SaaS-only.