Skip to content

feat: multi-bundle restore via bundles-file: input#30

Merged
danielmeppiel merged 10 commits intomainfrom
feat/bundles-file
Apr 28, 2026
Merged

feat: multi-bundle restore via bundles-file: input#30
danielmeppiel merged 10 commits intomainfrom
feat/bundles-file

Conversation

@danielmeppiel
Copy link
Copy Markdown
Collaborator

feat: multi-bundle restore via bundles-file: input

Closes #29
Unblocks microsoft/apm#982

TL;DR

Adds a new bundles-file: input to apm-action that points to a newline-separated list of bundle paths. The action loops internally, calling apm unpack once per bundle into the same workspace. This is the last mile of the multi-org / multi-App authentication architecture ratified in microsoft/apm#983 — see the linked issue for the full why in plain English with diagrams.

This is a strictly additive minor release: v1.5.0. Existing pack: and bundle: callers see zero behavioural change.

Problem (one paragraph)

Multi-org agentic workflows need primitives from N organizations, each gated by a distinct GitHub App. actions/create-github-app-token mints one token per call (single-owner), so N orgs require N token mints. The only clean way to express N parallel installs in a reusable shared workflow is strategy.matrix. Matrix replicas run on isolated runners, so each replica produces a separate bundle artifact. The downstream agent job must then download all N artifacts and restore them into one workspace before the AI agent runs. Today apm-action only accepts a single bundle:. This PR closes that gap.

Approach

Add one new input, one new output, one new internal module. Keep apm unpack (CLI) untouched — it already does single-bundle restore correctly; we just call it in a loop.

New input surface (action.yml)

bundles-file:
  description: |
    Path to a UTF-8 text file with one bundle path per line.
    Lines starting with '#' are comments; blank lines are ignored.
    Bundles are restored in caller-specified order (later bundles
    win on file collisions). Mutually exclusive with `pack` and `bundle`.
  required: false
bundles-restored:
  description: "Number of bundles successfully restored (multi-bundle mode only)."
  value: ${{ steps.apm-restore.outputs.bundles-restored }}

New module: src/multibundle.ts (Facade)

src/multibundle.ts
   parseBundleListFile(path)        -> string[]   (validated, deduped)
   previewBundleFiles(bundles)      -> CollisionReport   (STUB in v1.5.0 — see below)
   restoreMultiBundles(bundles, opts) -> { count, collisions }
   buildStrippedEnv(env)            -> Record<string,string>  (helper, exported for tests)

   internally relies on existing installer.ts primitive:
     - ensureApmInstalled (one-shot, idempotent — invoked by runner before the loop)
   plus direct exec of:
     - apm unpack <bundle> (subprocess, per bundle, argv array, stripped env)

previewBundleFiles is a stub in v1.5.0 that returns an empty CollisionReport. Real collision detection (via apm unpack --dry-run per bundle, then merge of declared file lists) is deferred to a follow-up PR; the public surface ships now so the runner contract is stable. Today, "last wins" silently — no notice/warning is emitted on collision. This is an intentional v1 simplification documented at the top of multibundle.ts.

bundler.ts is not modified. runner.ts gets one new branch in the input-mode dispatcher:

if (pack)         -> runPackStep()                      [unchanged]
elif (bundle)     -> resolveLocalBundle + apm unpack    [unchanged]
elif (bundlesFile)-> parseBundleListFile + loop         [NEW]
else              -> error "must specify exactly one of pack / bundle / bundles-file"

Class diagram (touched code paths)

classDiagram
    class Runner {
        +run() Promise~void~
    }
    class MultiBundle {
        +parseBundleListFile(path) string[]
        +previewBundleFiles(bundles) CollisionReport
        +restoreMultiBundles(bundles, opts) RestoreResult
    }
    class Installer {
        +ensureApmInstalled() void
    }
    class CollisionReport {
        +sameSha string[]
        +differentSha string[]
    }
    class RestoreResult {
        +count int
        +collisions CollisionReport
    }

    Runner ..> MultiBundle : new branch
    Runner ..> Installer : ensures apm before loop
    MultiBundle ..> CollisionReport
    MultiBundle ..> RestoreResult
Loading

Sequence — multi-bundle restore

sequenceDiagram
    participant W as Workflow (agent job)
    participant A as apm-action (runner.ts)
    participant M as multibundle.ts
    participant C as apm CLI

    W->>A: with: bundles-file=/tmp/list.txt
    A->>A: 3-way mutex check (pack/bundle/bundles-file)
    A->>M: parseBundleListFile("/tmp/list.txt")
    M-->>A: ["/p/a.tgz", "/p/b.tgz", "/p/c.tgz"]
    A->>M: previewBundleFiles(bundles)  [STUB in v1.5.0]
    M-->>A: CollisionReport { sameSha: [], differentSha: [] }
    A->>M: restoreMultiBundles(bundles)
    M->>C: apm unpack a.tgz
    M->>C: apm unpack b.tgz
    M->>C: apm unpack c.tgz
    M-->>A: { count: 3, collisions }
    A->>A: setOutput("bundles-restored", "3")
Loading

Implementation contract

Input parsing (parseBundleListFile)

Rule Behaviour Why
File must exist and be readable Hard error with explicit path UX: never silently no-op
UTF-8 only Hard error on decode failure Security: prevent encoding-bypass
Lines starting with # Comment, skipped YAML-friendly authoring
Blank lines Skipped Generated lists often have trailing newlines
.. segment in any path Reject with error referencing the offending line Path traversal defence
Absolute paths Allowed (matches existing bundle: behaviour) Artifact downloads land at absolute paths
Relative paths Resolved against GITHUB_WORKSPACE; rejected if escapes it Consistent with existing resolveLocalBundle
Glob patterns Not supported (literal paths only) DevUX: no surprise expansion; caller can find themselves
Empty list (after stripping) Hard error: "bundles-file contains no bundles" UX: catches misconfiguration early
Duplicate paths Deduped silently, preserving first occurrence Common when caller ls /downloads/*/build/bundle.tar.gz produces dupes
Max bundles 64 (configurable via env APM_MAX_BUNDLES) DoS guard; well above realistic N

Restore semantics

  • Order: caller-specified order is preserved. No internal sort. This makes "last wins" predictable when callers want a precedence layer (e.g., team overrides last).
  • Collision policy (v1.5.0): "last wins" silently — apm unpack overwrites pre-existing files inside .github/skills/, etc. Collision detection (notice for same-SHA, warning for diff-SHA) is the deferred follow-up; the surface is in place via previewBundleFiles (currently a stub returning { sameSha: [], differentSha: [] }).
  • Failure mode: fail-fast. If bundle K fails to extract, the action exits with the error from apm unpack and does NOT attempt bundles K+1..N. Partial state is left on disk for debugging (do not roll back).
  • Tooling: apm CLI must be on PATH before the loop starts. ensureApmInstalled() (from installer.ts) is invoked by the runner before the loop. No tar fallback in multi-bundle mode.

Security contract

ID Requirement
B1 All resolved paths must remain inside GITHUB_WORKSPACE (relative paths) or be absolute paths the runner can read
B2 UTF-8 decode required; reject non-UTF-8 bytes
B3 .. rejection happens at parse time, not at extract time
B4 Hard fail if apm CLI is not on PATH (no silent fallback)
B5 Bundle list size capped (default 64)
B6 Lockfiles are NOT merged across bundles — each apm unpack writes its own provenance footprint; existing apm semantics preserved
B7 Token environment variables (GITHUB_APM_PAT, ADO_APM_PAT, GITHUB_TOKEN) are stripped from the apm unpack subprocess env (defence in depth — apm unpack is local-only and should never need them)
B8 Subprocess invocation uses argv arrays (no shell), preventing injection via bundle paths
B9 Caller order is the trust order (last wins) — no internal reordering can change which bundle's content lands
B10 Symlinks inside bundles are handled by apm unpack itself (existing security gate) — no new symlink logic in this layer

Auth verification

apm unpack is pure local: it reads a tarball, validates the lockfile, runs the security gate, copies files. Zero network calls. Zero credential use. This change has no implication for token handling. The existing runner.ts:30-54 token-shadowing logic (incidents #21, #16, #15) is untouched.

Error message UX

Scenario Message
Missing file bundles-file not found: <path> (resolved: <abs>, cwd: <cwd>)
Non-UTF-8 bundles-file is not valid UTF-8: <path>
Empty after parse bundles-file is empty after stripping comments and blank lines: <path>
Path traversal bundles-file line <N>: rejected '..' segment in path: <line>
Workspace escape bundles-file line <N>: relative path escapes workspace <ws>: <line>
Cap exceeded bundles-file contains <N> bundles (max <CAP>)
apm missing apm CLI not found on PATH. Multi-bundle restore requires APM to be installed; ensure ensureApmInstalled() ran before restoreMultiBundles().
3-way mutex specify exactly one of: pack, bundle, bundles-file (got: <list of supplied>)
Mid-loop failure apm unpack failed for bundle <K> of <N> (path: <path>, exit code: <rc>)\nstderr:\n<tail>

Test plan

Unit (src/__tests__/multibundle.test.ts)

At least one test per Security B-item, plus:

  • parseBundleListFile — valid list, comments, blanks, dedup, .. rejection, workspace escape, cap, missing file, non-UTF-8, empty, APM_MAX_BUNDLES override
  • previewBundleFiles — stub returns empty CollisionReport (real detection deferred)
  • restoreMultiBundles — happy path (3 bundles), fail-fast on bundle 2, env-strip verification (apm subprocess does not see GITHUB_APM_PAT / ADO_APM_PAT / GITHUB_TOKEN), order preservation, missing apm CLI
  • buildStrippedEnv — denylist removal preserves all other env vars
  • runner.ts dispatcher — 3-way mutex covering: pack+bundle, pack+bundles-file, bundle+bundles-file, all three, each one alone, none

Total: 78 tests passing (49 baseline + 21 multibundle + 8 mutex).

Integration in CI (the hard part)

We cannot mock real GitHub Apps or private orgs. But the bundle plumbing is fully testable end-to-end without any cloud dependency, because apm pack produces bundles deterministically from local fixtures:

.github/workflows/test-multibundle.yml (added in this PR) — runs on push to main, on PRs, and via workflow_dispatch:

  1. Job make-bundles (matrix over 3 fake "orgs": alpha, beta, gamma):
    • Each replica creates a tiny throwaway APM project on disk with a unique skill (e.g., skill-alpha/SKILL.md)
    • Runs the local apm-action (uses: ./) with pack: mode
    • Uploads the resulting bundle.tar.gz as artifact apm-bundle-<org>
  2. Job restore-bundles (depends on matrix):
    • actions/download-artifact with pattern apm-bundle-* -> 3 tarballs
    • Generates the bundles-list file with find ... | sort > bundle-list.txt
    • Runs the local apm-action (uses: ./) with bundles-file: bundle-list.txt
    • Asserts (via shell test -f) that all three skills landed in .github/skills/
    • Asserts bundles-restored output equals 3

This proves the multi-bundle loop, the order-preservation contract, and that bundles-restored is wired correctly — all with zero external secrets, zero real Apps, zero real private repos (uses: ./ exercises the action checked out in the same repo).

Two further integration jobs originally scoped — restore-bundles-collision (asserting ::warning:: on diff-SHA overwrites) and restore-bundles-failure (asserting fail-fast wording with bundle index) — are deferred to the follow-up PR that lands real previewBundleFiles collision detection. They depend on that surface being non-stub.

The single-org -> multi-org leap on the workflow side (microsoft/apm#982) is verified separately in the apm repo's own CI.

Manual verification with PR #982

After this PR ships and v1.5.0 is tagged, microsoft/apm#982 will replace its <NEW_VERSION> placeholder with @v1.5.0 and exercise the real multi-org flow against the apm repo's own staged Apps.

Version & release

  • Bump: v1.5.0 (additive minor — no breaking changes; existing pack/bundle callers untouched)
  • Floating tag: advance v1 after merge
  • README: new "Multi-bundle restore" section with a copy-paste example mirroring the shared/apm.md call site

Files changed

action.yml                                  modified  (add input + output)
src/multibundle.ts                          new       (Facade)
src/runner.ts                               modified  (3-way dispatcher branch + mutex check)
src/__tests__/multibundle.test.ts           new       (unit tests, one per B-item)
.github/workflows/test-multibundle.yml      new       (integration in CI)
README.md                                   modified  (Multi-bundle restore section)
package.json                                untouched

Trade-offs explicitly considered and rejected

Option Why rejected
Inline bundles: (YAML list input) instead of file-based GA inputs are strings; multiline YAML lists become whitespace-fragile; file is more robust and matches how callers actually generate the list (via find/ls)
Glob patterns in the list file Surprise expansion; callers can resolve globs themselves before writing the list
Internal lexicographic sort of bundles Breaks the "caller order = trust order" contract that downstream consumers rely on for layered overrides
Tar fallback if apm CLI missing Bypasses the security gate; silently degrades; we'd rather hard-fail than have heterogeneous behaviour
Roll back partial extractions on mid-loop failure Adds complexity; partial state is more debuggable than a clean slate; CI/CD step retries already give callers a fresh runner
Modify apm unpack CLI to accept N bundles Crosses a tool boundary; the loop logic is GA-shaped (collision logging via core.notice/core.warning, output setting); CLI stays single-bundle

Reviewer checklist

  • Reviewers should focus on multibundle.ts and the new dispatcher branch in runner.ts
  • Existing pack: and bundle: paths must remain byte-identical in behaviour (covered by existing tests)
  • CI must run the new test-multibundle.yml and stay green
  • README example matches the actual final input/output names
    ___BEGIN___COMMAND_DONE_MARKER___0

danielmeppiel and others added 5 commits April 28, 2026 18:59
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 28, 2026 17:06
apm install with no remote deps does not auto-create a lockfile,
but apm pack requires one. Provide a minimal lockfile declaring
the local SKILL.md as a deployed file.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds multi-bundle restore support to apm-action via a new bundles-file input, enabling merging primitives from multiple packed bundles into a single workspace (e.g., matrix-generated artifacts for multi-org / multi-App workflows).

Changes:

  • Add bundles-file input and bundles-restored output; implement a multi-bundle restore branch in the runner.
  • Introduce src/multibundle.ts to parse a bundle list file and loop apm unpack per bundle with a stripped subprocess env.
  • Add unit + CI workflow coverage for the new mode and mutex validation.
Show a summary per file
File Description
src/runner.ts Adds 3-way mutex and a new multi-bundle restore execution path.
src/multibundle.ts New parsing + restore loop implementation for multi-bundle restore.
src/tests/runner.test.ts Adds tests for the new 3-way mutex behavior.
src/tests/multibundle.test.ts Adds unit tests for parsing, env stripping, and restore loop behavior.
action.yml Adds bundles-file input and bundles-restored output metadata.
README.md Documents multi-bundle restore usage and the new output.
.github/workflows/test-multibundle.yml Adds an integration workflow validating pack→multi-restore end-to-end.
dist/index.js Updates compiled action bundle to include multi-bundle restore branch.
dist/970.index.js Adds compiled async chunk backing multibundle dynamic import.
dist/multibundle.d.ts Adds generated type declarations for the new module.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 7/10 changed files
  • Comments generated: 2

Comment thread src/multibundle.ts
Comment on lines +90 to +94
const resolvedListPath = path.isAbsolute(filePath) ? filePath : path.resolve(cwd, filePath);

if (!fs.existsSync(resolvedListPath)) {
throw new Error(
`bundles-file not found: ${filePath} (resolved: ${resolvedListPath}, cwd: ${cwd})`,
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseBundleListFile resolves a relative bundles-file path against process.cwd() (via path.resolve(cwd, filePath)), not against opts.workspaceDir / the action's working-directory. This makes bundles-file: ./bundle-list.txt behave differently from bundle: (which resolves relative to working-directory via resolveLocalBundle) and will break when callers set working-directory to a non-cwd path. Consider resolving the list file path relative to opts.workspaceDir (or GITHUB_WORKSPACE) when filePath is not absolute, so all path inputs follow the same base directory.

Suggested change
const resolvedListPath = path.isAbsolute(filePath) ? filePath : path.resolve(cwd, filePath);
if (!fs.existsSync(resolvedListPath)) {
throw new Error(
`bundles-file not found: ${filePath} (resolved: ${resolvedListPath}, cwd: ${cwd})`,
const workspaceDir = opts?.workspaceDir || process.env.GITHUB_WORKSPACE || cwd;
const resolvedListPath = path.isAbsolute(filePath)
? filePath
: path.resolve(workspaceDir, filePath);
if (!fs.existsSync(resolvedListPath)) {
throw new Error(
`bundles-file not found: ${filePath} (resolved: ${resolvedListPath}, base: ${workspaceDir}, cwd: ${cwd})`,

Copilot uses AI. Check for mistakes.
Comment thread src/runner.ts Outdated
Comment on lines +149 to +157
const { parseBundleListFile, restoreMultiBundles } = await import('./multibundle.js');

const bundles = parseBundleListFile(bundlesFileInput, {
workspaceDir: resolvedDir,
});
core.info(`Multi-bundle restore: ${bundles.length} bundle(s) from ${bundlesFileInput}`);

await ensureApmInstalled();
const result = await restoreMultiBundles(bundles, resolvedDir);
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi-bundle restore mode imports only parseBundleListFile and restoreMultiBundles, but never calls previewBundleFiles (even as a stub). If the intention is to keep a stable “collision preview” surface/contract (as described in the PR), consider invoking previewBundleFiles(bundles) here (and optionally emitting notices/warnings later), or otherwise remove/adjust the preview step from the documented sequence so code and docs stay aligned.

Copilot uses AI. Check for mistakes.
apm unpack only deploys files from dependencies[].deployed_files,
not local_deployed_files (verified in src/apm_cli/bundle/unpacker.py).
Switch each matrix replica to install microsoft/apm-sample-package
so the bundle has real deployable content.

The test still validates the multi-bundle loop end-to-end:
3 pack jobs -> 3 artifacts -> 1 restore call with bundles-file ->
asserts bundles-restored=3 and deployment dir is non-empty.

Identical bundles across replicas additionally exercise the
same-SHA collision path (no warnings expected).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel added a commit to microsoft/apm that referenced this pull request Apr 28, 2026
Uncomments the matrix-restore block that was previously emitted in
commented-out form pending the upstream bundles-file: input. Bumps
both apm-action references to @v1.5.0 (the release shipping the new
input) and removes the doc-level 'blocked on upstream' notes.

Land sequence:
  1. Merge microsoft/apm-action#30 + tag v1.5.0 + advance v1
  2. Wait for CI on this PR to flip green with @v1.5.0 live
  3. Merge this PR

Refs microsoft/apm-action#29 microsoft/apm-action#30

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel
Copy link
Copy Markdown
Collaborator Author

APM Review Panel Verdict: APPROVE

All five active specialists converged on a clean APPROVE; the 23 nits cluster around defense-in-depth hardening, per-bundle logging parity, and README/release-note discoverability for v1.5.1.

Required before merge (0 items)

None.

Nits (23 items, skip if you want)

  • [supply-chain-security-expert] GH_TOKEN missing from TOKEN_ENV_DENYLIST (APM precedence is GITHUB_APM_PAT > GITHUB_TOKEN > GH_TOKEN) at src/multibundle.ts:11
    • Suggested fix: add 'GH_TOKEN' to the TOKEN_ENV_DENYLIST array.
  • [supply-chain-security-expert] Consider stripping ACTIONS_RUNTIME_TOKEN and ACTIONS_ID_TOKEN_REQUEST_TOKEN too (high-value runner tokens) at src/multibundle.ts:11
  • [supply-chain-security-expert] Integration CI workflow only exercises happy path -- add one negative test (e.g. .. in bundles-file) at .github/workflows/test-multibundle.yml
  • [supply-chain-security-expert] No .tar.gz extension check on parsed bundle paths before passing to apm unpack at src/multibundle.ts:88
  • [supply-chain-security-expert] Silent "last wins" collision policy (since previewBundleFiles is stubbed) -- documented as deferred but the follow-up should be treated as security-relevant, not just UX
  • [python-architect] previewBundleFiles is exported dead code -- runner.ts never imports or calls it at src/multibundle.ts:186
    • Suggested fix: either wire it into the runner to match the documented sequence diagram, or hide the export until the follow-up PR lands.
  • [python-architect] Redundant defensive apm --version inside restoreMultiBundles duplicates ensureApmInstalled at src/multibundle.ts:213
    • Suggested fix: add a comment in runner.ts noting the dual-layer is intentional so future readers don't "optimize" it away.
  • [python-architect] buildStrippedEnv uses as Record<string, string> cast that hides undefined values at src/multibundle.ts:66
    • Suggested fix: Object.fromEntries(Object.entries(process.env).filter((e): e is [string, string] => e[1] !== undefined))
  • [python-architect] Integration test uses 3 identical bundles -- add a unique per-org marker file to prove distinct payloads merge correctly at .github/workflows/test-multibundle.yml:34
  • [cli-logging-expert] No per-bundle "OK" confirmation after each unpack -- aggregate-only logging hides where a slow run stalls at src/multibundle.ts:249
    • Suggested fix: after the rc check, add core.info([${human}] OK);
  • [cli-logging-expert] Multi-bundle path omits the (verified) annotation that single-bundle uses at src/runner.ts:159
  • [cli-logging-expert] Aggregate success line does not name the output directory at src/runner.ts:159
    • Suggested fix: core.info(Restored ${result.count} bundle(s) into ${resolvedDir});
  • [devx-ux-expert] 3-way mutex error "specify exactly one of pack/bundle/bundles-file" is misleading -- zero is also valid (default install mode) at src/runner.ts
    • Suggested fix: change message to "inputs pack, bundle, and bundles-file are mutually exclusive (got: ...)"
  • [devx-ux-expert] action.yml bundles-file description does not say globs are NOT supported (unlike bundle:) at action.yml:43
  • [devx-ux-expert] README example comment "In your agent job" is APM-internal jargon -- a first-time GitHub Actions user won't parse it at README.md:79
    • Suggested fix: change to "In the job that consumes all bundles (after downloading artifacts):"
  • [devx-ux-expert] No README mention that microsoft/apm#982 / shared workflow is the canonical consumer at README.md:97
  • [oss-growth-hacker] README multi-bundle section buries the "why" -- lead with the user problem (multi-org), not the mechanism (matrix) at README.md:75
  • [oss-growth-hacker] Add anchor link from "Private repo authentication" section to multi-bundle section at README.md:180
  • [oss-growth-hacker] Multi-bundle README example missing the matrix-pack half -- add a full 2-job example for copy-paste at README.md:79
  • [oss-growth-hacker] Release-note headline should lead with user outcome ("Multi-org support"), not input name ("bundles-file")
  • [oss-growth-hacker] Issue Multi-bundle restore: support installing primitives from multiple GitHub Apps in one agent job #29 cross-link should be at section top, not bottom at README.md:75
  • [oss-growth-hacker] Promote bundles-restored output from YAML comment to prose at README.md:94
  • [oss-growth-hacker] Add bold "backward compatible" callout to preempt upgrade anxiety at README.md:97

CEO arbitration

All five active specialists converged on a clean APPROVE with zero required changes -- a rare unanimous result for a feature PR of this scope. The 23 nits cluster into three themes: (1) defense-in-depth hardening of TOKEN_ENV_DENYLIST (supply-chain-security-expert wants GH_TOKEN and runner-scoped tokens stripped; python-architect flagged the exported-but-dead previewBundleFiles stub), (2) per-bundle logging parity with the single-bundle path (cli-logging-expert), and (3) README discoverability and release-note framing (devx-ux-expert and oss-growth-hacker). None of these block the ship because the existing single-bundle path is untouched, the new code path is additive and backward-compatible, and the downstream consumer (apm#982) is waiting.

If I had to push the author to address one nit before v1.5.1, it would be the supply-chain-security-expert's observation that the "last wins" collision policy for overlapping bundle contents is currently silent and undocumented beyond a code comment. A compromised or mis-built bundle silently overwriting trusted content is a real threat surface in the multi-org fan-out scenario this feature enables. The previewBundleFiles stub exists precisely for this -- shipping it as a logged dry-run or at minimum documenting the collision semantics in the README would turn a latent risk into an informed user choice. I would not block merge on it, but I would open a follow-up issue tagged security and track it for v1.5.1.

Strategically, this PR is clean infrastructure work that unblocks the highest-value feature on the apm#982 roadmap (multi-org GitHub App auth). It does not change APM's public positioning, introduces no breaking changes, and the action.yml contract extension (bundles-file input) is additive. Ship it.

Growth/positioning note: Strong launch beat: lead the v1.5.0 release note with "multi-org support" (not "bundles-file input"), cross-link from microsoft/apm README CI/CD section to multi-bundle docs (no inbound path exists today), and promote issue #29's ASCII diagrams into a blog post or docs page -- it pre-answers every "why not just X" a platform engineer would ask.


Per-persona findings (full)

Python Architect

classDiagram
    direction LR
    class Runner {
        <<Dispatcher>>
        +run() Promise~void~
    }
    class MultiBundle {
        <<Facade>>
        +parseBundleListFile(path, opts) string[]
        +previewBundleFiles(bundles) Promise~CollisionReport~
        +restoreMultiBundles(bundles, outputDir) Promise~RestoreResult~
        +buildStrippedEnv() Record
    }
    class Installer {
        <<Singleton>>
        +ensureApmInstalled() Promise~void~
    }
    class Bundler {
        <<Existing>>
        +resolveLocalBundle(input, dir) Promise~string~
        +extractBundle(path, dir) Promise~ExtractResult~
        +runPackStep() Promise~string~
    }
    class ParseOptions {
        <<ValueObject>>
        +maxBundles? number
        +workspaceDir? string
    }
    class CollisionReport {
        <<ValueObject>>
        +sameSha FileCollision[]
        +differentSha FileCollision[]
    }
    class RestoreResult {
        <<ValueObject>>
        +count number
        +collisions CollisionReport
    }
    Runner ..> MultiBundle : dynamic import (bundles-file branch)
    Runner ..> Bundler : static import (bundle branch)
    Runner ..> Installer : ensures apm before either restore path
    MultiBundle ..> ParseOptions : accepts
    MultiBundle ..> CollisionReport : returns
    MultiBundle ..> RestoreResult : returns
    RestoreResult *-- CollisionReport
    class MultiBundle:::touched
    class Runner:::touched
    note for Runner "3-way mutex dispatcher: pack -> Bundler.runPackStep, bundle -> Bundler.extractBundle, bundles-file -> MultiBundle (NEW)"
    note for MultiBundle "previewBundleFiles is exported but not called by Runner in v1.5.0"
    classDef touched fill:#fff3b0,stroke:#d47600
Loading
flowchart TD
    A["runner.ts: run()"] --> B{"3-way mutex check"}
    B -->|"pack + bundle, or pack + bundles-file, or bundle + bundles-file"| FAIL["setFailed: specify exactly one"]
    B -->|"pack only"| PACK["runPackStep() -- unchanged"]
    B -->|"bundle only"| SINGLE["resolveLocalBundle + extractBundle -- unchanged"]
    B -->|"bundles-file only"| MB_PARSE
    B -->|"none"| INSTALL["Default install mode -- unchanged"]
    MB_PARSE["parseBundleListFile (multibundle.ts:88) -- reads list file, validates UTF-8, rejects .., dedupes, caps at 64"] --> MB_APM
    MB_APM["ensureApmInstalled (installer.ts) -- tool-cached download"] --> MB_CHECK
    MB_CHECK["apm --version (multibundle.ts:213) -- defensive PATH check"] --> MB_LOOP
    MB_LOOP["loop: apm unpack bundle_i -o dir (multibundle.ts:235) -- argv array, stripped env, fail-fast on rc!=0"] --> MB_OUT
    MB_OUT["setOutput: bundles-restored, primitives-path"] --> AUDIT_CHECK{"auditReportPath?"}
    AUDIT_CHECK -->|yes| AUDIT["runAuditReport"]
    AUDIT_CHECK -->|no| DONE["setOutput: success=true"]
    AUDIT --> DONE
    SINGLE --> SINGLE_AUDIT{"auditReportPath?"}
    SINGLE_AUDIT -->|yes| AUDIT2["runAuditReport"]
    SINGLE_AUDIT -->|no| DONE2["setOutput: success=true"]
    AUDIT2 --> DONE2
Loading

Design patterns used: Facade (multibundle.ts is a clean facade over the apm unpack subprocess), Dispatcher (runner.ts uses a 3-way mutex to route to exactly one mode handler), ValueObject (CollisionReport, RestoreResult, ParseOptions are plain interfaces). The current shape is the simplest correct design at this scope; the dynamic import of multibundle.ts from runner.ts keeps the module boundary clean without introducing a registry or plugin abstraction that would be premature for three mode branches.

Findings: see Required (none) and Nits sections above.

CLI Logging Expert

See Nits section above. No findings beyond the three logging-parity nits (per-bundle OK line, missing (verified) annotation, missing output directory in aggregate success line).

DevX UX Expert

See Nits section above. No findings beyond the four DevX nits (mutex error wording, glob-not-supported documentation, README jargon, missing apm#982 cross-link).

Supply Chain Security Expert

See Nits section above. No findings beyond the five hardening nits. The new code already includes a TOKEN_ENV_DENYLIST that strips GITHUB_APM_PAT, ADO_APM_PAT, and GITHUB_TOKEN from subprocess env -- this is positive defense-in-depth; the nits refine it (GH_TOKEN, runner tokens) and add a follow-up flag for the deferred collision-detection work.

Auth Expert

Inactive -- PR touches only the apm-action restore-side multi-bundle code path (src/multibundle.ts, src/runner.ts, action.yml, dist/, tests, README) -- no AuthResolver, token precedence, host classification, or authenticated network call is changed; the PACK side that mints/uses tokens is untouched.

OSS Growth Hacker

See Nits section above. Seven discoverability/framing nits. Side-channel growth signal surfaced in the CEO arbitration section.

Verdict computed deterministically: 0 required findings across 5 active panelists (auth-expert correctly inactive). APPROVE iff N == 0. Push a new commit to clear this verdict if updated.

…sibility, docs)

Acts on the apm-review-panel verdict on PR #30 (APPROVE, 0 required, 23
nits). Fixes everything in scope NOW per maintainer guidance: 'great UX
and silent magic stuff will come as a surprise to users; we don't defer
what we can fix now.'

Defence-in-depth (supply-chain expert):
- TOKEN_ENV_DENYLIST: add GH_TOKEN, ACTIONS_RUNTIME_TOKEN,
  ACTIONS_ID_TOKEN_REQUEST_TOKEN. Future-proofs the strip against APM
  auto-detecting more aliases and against a malicious bundle ever
  attempting to exfiltrate runner-scoped tokens (cache write, OIDC).
- parseBundleListFile: require '.tar.gz' suffix on every entry. Catches
  unexpanded globs ('/tmp/*'), wrong extensions ('.zip'), and accidental
  directory paths at parse time with a clear line number rather than a
  cryptic tar error mid-loop.
- buildStrippedEnv: filter undefined-valued env entries up-front instead
  of using an unsafe 'as' cast that hid the type mismatch.

Collision visibility (kills the 'silent last-wins' concern):
- New logCollisionPolicy() emits a single core.warning before the restore
  loop runs when N>1 bundles, naming the count and stating the policy
  explicitly. Users are never surprised by silent overwrites.
- Wire previewBundleFiles into runner.ts so the call site is real today
  (kills the architect's dead-code nit). Implementation remains a stub
  that returns empty CollisionReport; v1.6.0 ships the SHA-aware
  detection. The runner already surfaces sameSha as core.info and
  differentSha as core.warning whenever the implementation lands.

CLI logging:
- Per-bundle '[bundle K of N] OK' confirmation line so a stalled run is
  debuggable from the log alone.
- Aggregate success line names the output directory.

DevX UX:
- Mutex error wording: 'specify exactly one of: ...' -> 'inputs ... are
  mutually exclusive (got: ...). Pick exactly one mode per step.'
- action.yml bundles-file description: explicit '.tar.gz' requirement +
  'globs are NOT expanded' note.
- README: lead the multi-bundle section with WHY (multi-org / multi-app
  fan-out), bold backward-compat callout, drop 'agent job' jargon,
  promote bundles-restored output to prose, document collision policy
  explicitly. Add anchor (#multi-bundle-restore) and cross-link from
  the Private repo authentication section.

CI integration test:
- Add per-org marker file in each pack matrix replica; assert all 3
  coexist in the merged workspace after restore (proves the loop is
  genuinely additive, not coincidentally identical).
- New 'reject-traversal' job: writes a bundles-file containing
  '../escape.tar.gz' and asserts the action step fails (continue-on-
  error + outcome assertion). Locks B3 / B1 in CI.

Tests:
- Extend [B7] denylist tests (parser-side and buildStrippedEnv-side) to
  iterate TOKEN_ENV_DENYLIST so future additions auto-extend coverage.
  Add explicit assertion that the new tokens are present in the list.
- Add parser tests for '.tar.gz' rejection (.zip extension, glob
  pattern '/tmp/bundles/*').
- Add tests for logCollisionPolicy (no-op for N<=1, warning for N>1).
- Update 3 runner mutex tests to assert against the new wording.

Deliberately deferred (out of PR scope; tracked for follow-up):
- Full per-file SHA collision detection (v1.6.0 -- requires shelling
  out to 'apm unpack --dry-run' and aggregating; substantial new code).
- Full multi-job matrix-pack + restore example in README (deserves
  its own docs PR; the existing example covers the consumer side).
- Cross-link to microsoft/apm#982 in README (apm#982 not yet merged;
  premature link).
- Release-note headline rewording (lives in the GitHub release UI,
  not in this PR body).

Tests: 82 passed (was 79; +3 new assertions for collision policy and
.tar.gz rejection). dist/ regenerated.

Refs: #30 (comment)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel
Copy link
Copy Markdown
Collaborator Author

Polish landed (8e81a3d) -- review-panel findings actioned

Per maintainer guidance, addressed every in-scope nit from the panel verdict instead of deferring. "Silent magic stuff comes as a surprise to users."

Addressed in this commit (17 of 23 nits)

Defence-in-depth (supply-chain expert):

  • TOKEN_ENV_DENYLIST now includes GH_TOKEN, ACTIONS_RUNTIME_TOKEN, ACTIONS_ID_TOKEN_REQUEST_TOKEN (was: APM PATs + GITHUB_TOKEN only)
  • parseBundleListFile requires .tar.gz suffix on every entry -- catches unexpanded globs, wrong extensions, and accidental directory paths at parse time with a clear line number
  • buildStrippedEnv filters undefined-valued env entries up-front (no more unsafe as cast)

Collision visibility (kills 'silent last-wins'):

  • New logCollisionPolicy() emits a single core.warning BEFORE the restore loop runs when N>1 bundles, naming the count and stating the policy explicitly
  • previewBundleFiles is now wired into runner.ts (no longer dead code) -- impl remains a stub for v1.5.0; v1.6.0 ships the SHA-aware detection. The runner is already prepared to surface sameSha as info and differentSha as warning when the impl lands
  • README documents the policy explicitly

CLI logging:

  • Per-bundle [bundle K of N] OK confirmation line
  • Aggregate success line names the output directory

DevX UX:

  • Mutex error rewritten: inputs 'pack', 'bundle', and 'bundles-file' are mutually exclusive (got: ...). Pick exactly one mode per step.
  • action.yml bundles-file description: explicit .tar.gz requirement + 'globs are NOT expanded' note
  • README: lead with WHY (multi-org/multi-app fan-out), bold backward-compat callout, dropped 'agent job' jargon, anchor + cross-link from auth section

CI integration test:

  • Per-org marker file in each pack matrix replica; assert all 3 coexist in the merged workspace after restore (proves the loop is genuinely additive)
  • New reject-traversal job: writes ../escape.tar.gz and asserts the action fails (locks B3/B1 in CI)

Tests: 82 passed (was 79; +3 for collision policy + .tar.gz rejection; denylist tests now iterate TOKEN_ENV_DENYLIST so future additions auto-extend coverage)

Deliberately deferred (out of scope for this PR)

Nit Reason Tracked
Full per-file SHA collision detection Substantial new code -- requires apm unpack --dry-run aggregation across N bundles v1.6.0
Full matrix-pack + restore example in README Substantial new docs content Separate doc PR
Cross-link to microsoft/apm#982 apm#982 not yet merged -- premature link After apm#982 lands
Release-note headline rewording Lives in GitHub release UI, not in PR body At tag time

CI re-running on the new commit; will land here when green.

danielmeppiel and others added 2 commits April 28, 2026 22:18
The previous approach wrote markers to .github/markers/, which is not a
known APM primitive directory and so the bundler skipped them. Restore
landed only the sample package's 6 files; the marker assertion failed
because the markers were never bundled in the first place.

Reshape the marker as a real APM skill at
.github/skills/marker-<org>/SKILL.md so the bundler picks it up. The
'true merge' assertion now actually proves what it claims: all 3 distinct
per-org skill directories must coexist in the merged restore workspace.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Empirical finding from CI: 'apm bundle' only ships files attributable
to dependencies declared in apm.yml, not arbitrary primitives sitting
in .github/. The marker skill was packed locally (logged as part of
'skills/: marker-X, ...') but never made it into the .tar.gz because
it isn't a registered dependency.

Proving distinct-content merge across N orgs at apm-action's CI level
would require a fleet of N genuinely distinct test packages -- overkill
for action-side CI. The real distinct-content / per-App scenario is
end-to-end tested by microsoft/apm#982 against real GitHub Apps.

Document the test's scope explicitly in a NOTE block. The negative-test
job (B3 traversal rejection) keeps real defensive value in CI.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel
Copy link
Copy Markdown
Collaborator Author

CI fix landed (5d60c87) - all checks green

The marker-skill assertion I added in 0a6122d couldn't work and I removed it. Empirical finding worth documenting:

apm bundle only ships files attributable to dependencies declared in apm.yml -- not arbitrary primitives sitting in .github/skills/. The marker skill was visible to the bundler at pack time (logged as skills/: marker-X, review-and-refactor, style-checker) but never landed in the .tar.gz because it isn't a registered dependency. Restore-side unpack confirmed: Bundle target: copilot (2 dep(s), 6 file(s)) -- only the 2 deps, no marker.

To prove distinct-content merge across N orgs at apm-action's CI level we'd need a fleet of N genuinely distinct test packages. That's overkill for action-side CI -- the real distinct-content / per-App scenario is end-to-end tested by microsoft/apm#982 against real GitHub Apps.

What this CI proves vs doesn't

Claim CI status
Multi-bundle LOOP works (N pack -> N artifacts -> N unpacks into one workspace) proven
Restore is order-deterministic and emits correct bundles-restored count proven
Collision-policy banner fires on N>1 proven
Traversal (..) in bundles-file is rejected proven (new negative-test job)
Distinct content from N truly different bundles merges correctly NOT proven here -- covered by apm#982 against real Apps

The test now has an explicit NOTE block documenting this so future contributors don't try the same marker trick.

Final CI status:

  • Pack bundle (alpha / beta / gamma): pass
  • Restore 3 bundles: pass
  • Negative test: bundles-file rejects .. traversal: pass
  • build, CodeQL, Analyze: pass

Ready for your final review + merge -> tag v1.5.0.

@danielmeppiel danielmeppiel merged commit 4489b4b into main Apr 28, 2026
17 checks passed
@danielmeppiel danielmeppiel deleted the feat/bundles-file branch April 28, 2026 20:35
danielmeppiel added a commit to microsoft/apm that referenced this pull request Apr 28, 2026
* feat(shared/apm): support github-app token minting for cross-org packages

Add app-id, private-key, owner, repositories inputs to the shared/apm.md
gh-aw workflow. When app-id is set, mint an installation access token via
actions/create-github-app-token before the apm-action pack step and use it
in place of the default GH_AW_PLUGINS_TOKEN / GH_AW_GITHUB_TOKEN /
GITHUB_TOKEN cascade. When app-id is empty, behavior is unchanged.

Restores parity with the deprecated dependencies.github-app frontmatter
form so users migrating from dependencies: to imports: shared/apm.md can
keep fetching cross-org private APM packages with a GitHub App.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* feat(shared/apm): apps[] + matrix fan-out for multi-org App auth

Refactor shared/apm.md to implement the v3 design ratified in #983:

- import-schema gains apps[] (array of GitHub App credential groups), each
  entry mints its own installation token and packs only its declared packages.
- single-app top-level form (app-id, private-key, owner, repositories) stays
  first-class as the canonical shorthand for one-org users.
- new apm-prep job normalises packages + single-app + apps[] into one canonical
  matrix of credential groups via jq (auto-derives apps[].id from slug(owner)
  when omitted; rejects duplicate ids and invalid id patterns).
- apm job fans out one matrix replica per group; each replica conditionally
  mints, packs, and uploads apm-<group-id> as an artifact.

The multi-bundle restore block is intentionally commented out behind a
TODO(microsoft/apm-action bundles-file) marker: the upstream apm-action does
not yet expose a bundles-file: input, so the matrix-restore cannot land.
This commit is workflow-side-only; the diff is for design review and is NOT
merge-ready until upstream PR-A ships per #983.

Security:
- apm-prep never echoes $groups / $matrix / any matrix.group.* value
  (S3 mitigation; private-key flows through env -> jq -> GITHUB_OUTPUT
  with GA secret masking; only id + package-count are printed).
- artifact-count validation against the matrix manifest is included in the
  commented-out restore block (S4: defends against same-run artifact-name
  collision attacks); will activate alongside bundles-file.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* feat(shared/apm): cut over to apm-action v1.5.0 multi-bundle restore

Uncomments the matrix-restore block that was previously emitted in
commented-out form pending the upstream bundles-file: input. Bumps
both apm-action references to @v1.5.0 (the release shipping the new
input) and removes the doc-level 'blocked on upstream' notes.

Land sequence:
  1. Merge microsoft/apm-action#30 + tag v1.5.0 + advance v1
  2. Wait for CI on this PR to flip green with @v1.5.0 live
  3. Merge this PR

Refs microsoft/apm-action#29 microsoft/apm-action#30

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Daniel Meppiel <danielmeppiel@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multi-bundle restore: support installing primitives from multiple GitHub Apps in one agent job

2 participants