diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7941b31..76303a2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -250,9 +250,131 @@ jobs: fi echo "✅ Restore mode: primitives restored from bundle" + # Integration: regression test for microsoft/apm-action#26. + # + # Reproduces the gh-aw pull_request_target scenario: when the workflow + # checkout is the base branch and the triggering PR modifies apm.lock.yaml, + # the post-restore `git checkout -- .` must succeed. Before the fix + # it failed with `error: Your local changes to the following files would be + # overwritten by checkout: apm.lock.yaml`, because the action's tar fallback + # had dumped the bundle's apm.lock.yaml on top of the tracked one. + # + # The check has two halves: + # (1) `git status` after restore must show no modifications to apm.lock.yaml + # or apm.yml. apm unpack writes only files listed in the lockfile's + # `deployed_files` -- not the lockfile or manifest themselves. + # (2) A git checkout to a different ref must succeed without conflict. + test-restore-clean-workspace: + needs: build + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@v6 + + - name: Create test apm.yml + commit a baseline lockfile + run: | + # Configure git identity so we can commit in the runner. + git config user.email "ci@apm-action.local" + git config user.name "apm-action ci" + + # Create a throwaway branch so we can manipulate tracked files + # without touching the PR source. + git checkout -b ci/restore-clean-workspace-test + + cat > apm.yml << 'EOF' + name: ci-test-restore-clean + version: 1.0.0 + dependencies: + apm: + - microsoft/apm-sample-package + EOF + + # Commit a placeholder apm.lock.yaml so it is *tracked* by git. + # This is the precondition that makes the bug fire: the file in + # working-directory is tracked, so any extraneous write to it + # surfaces in `git status` and blocks subsequent `git checkout`. + cat > apm.lock.yaml << 'EOF' + # placeholder lockfile - replaced by pack step below + version: 1 + packages: {} + EOF + git add apm.yml apm.lock.yaml + git commit -m "test fixture: tracked apm.yml + apm.lock.yaml" --quiet + + # Capture the SHA of the "tracked baseline" so we can git-checkout + # back to it after restore -- this is the operation that breaks if + # restore dirties the tracked files. + BASELINE_SHA=$(git rev-parse HEAD) + echo "BASELINE_SHA=$BASELINE_SHA" >> "$GITHUB_ENV" + + - name: Pack bundle + id: pack + uses: ./ + with: + pack: 'true' + target: 'vscode' + + - name: Stash bundle and reset workspace to the tracked baseline + run: | + # Move the bundle out of the workspace so subsequent git operations + # don't see it. + mkdir -p /tmp/apm-bundle + cp "${{ steps.pack.outputs.bundle-path }}" /tmp/apm-bundle/ + + # Reset to the tracked baseline so we are testing restore against a + # clean git state with apm.lock.yaml + apm.yml *tracked*. + rm -rf .github/instructions .github/agents .github/skills .github/prompts apm_modules build + git checkout -- . + git clean -fd + test -z "$(git status --porcelain)" || { + echo "::error::pre-restore workspace is dirty: $(git status --porcelain)"; exit 1; } + + - name: Test - restore bundle into the tracked git checkout + uses: ./ + with: + bundle: '/tmp/apm-bundle/*.tar.gz' + + - name: Assert apm.lock.yaml + apm.yml were NOT modified by restore + run: | + # Restore must not touch tracked metadata files. If `git status` shows + # apm.lock.yaml or apm.yml as modified, the action is leaking bundle + # metadata into working-directory -- the exact bug from #26. + DIRTY_META=$(git status --porcelain apm.lock.yaml apm.yml) + if [ -n "$DIRTY_META" ]; then + echo "::error::Restore modified tracked apm metadata files (#26 regression):" + echo "$DIRTY_META" + echo + echo "--- diff ---" + git --no-pager diff apm.lock.yaml apm.yml + exit 1 + fi + echo "✅ apm.lock.yaml and apm.yml are pristine after restore" + + - name: Assert primitives WERE deployed + run: | + if ! find .github -type f \( -name "*.instructions.md" -o -name "*.agent.md" -o -name "*.prompt.md" -o -name "SKILL.md" \) | grep -q .; then + echo "::error::No primitives found after restore" + exit 1 + fi + echo "✅ Primitives deployed under .github/" + + - name: Assert subsequent git checkout succeeds (the actual #26 symptom) + run: | + # This is the operation that originally failed in the gh-aw flow: + # `git checkout -- .` after the action ran. We replay it here + # by checking out the tracked baseline. Before the fix this aborted + # with "Your local changes to the following files would be overwritten + # by checkout: apm.lock.yaml". After the fix it must succeed cleanly. + if ! git checkout "$BASELINE_SHA" -- . 2>checkout.err; then + echo "::error::git checkout failed after restore -- this is the #26 regression" + cat checkout.err + exit 1 + fi + echo "✅ Post-restore git checkout succeeded (no #26 regression)" + # Release: update major version tag (v1 → v1.x.x) release: - needs: [build, test-manifest, test-isolated, test-compile, test-pack, test-restore-artifact] + needs: [build, test-manifest, test-isolated, test-compile, test-pack, test-restore-artifact, test-restore-clean-workspace] if: startsWith(github.ref, 'refs/tags/v') runs-on: ubuntu-latest timeout-minutes: 5 diff --git a/README.md b/README.md index 209e4ac..31d5216 100644 --- a/README.md +++ b/README.md @@ -58,9 +58,9 @@ Install dependencies, scan for hidden Unicode threats, and pack into a self-cont This works with all modes — `isolated`, inline `dependencies`, or from `apm.yml`. -### Restore mode (zero-install) +### Restore mode (verified extraction) -Restore primitives from a bundle — no APM installation, no Python, no network. If APM happens to be on PATH, it uses `apm unpack` for integrity verification; otherwise it falls back to `tar xzf`. +Restore primitives from a bundle. The action installs APM (cached across runs) and uses `apm unpack` for integrity verification — no Python, minimal network. Only files listed in the bundle's lockfile (`deployed_files`) are written to `working-directory`; the lockfile and `apm.yml` themselves are not, so the workspace stays clean for downstream steps such as `git checkout`. ```yaml - uses: actions/download-artifact@v4 @@ -179,7 +179,7 @@ For multi-org or multi-platform scenarios, use the `env:` block for full control | `isolated` | No | `false` | Ignore apm.yml and clear pre-existing primitive dirs — install only inline dependencies | | `compile` | No | `false` | Run `apm compile` after install to generate AGENTS.md | | `pack` | No | `false` | Pack a bundle after install (produces `.tar.gz` by default) | -| `bundle` | No | | Restore from a bundle (local path or glob). Skips APM installation entirely. | +| `bundle` | No | | Restore from a bundle (local path or glob). Installs APM and unpacks via `apm unpack` (verified). | | `target` | No | | Bundle target: `copilot`, `vscode`, `claude`, or `all` (used with `pack: true`) | | `archive` | No | `true` | Produce `.tar.gz` instead of directory (used with `pack: true`) | | `audit-report` | No | | Generate a SARIF audit report (hidden Unicode scanning). `apm install` already blocks critical findings; this adds reporting for Code Scanning and a markdown summary in `$GITHUB_STEP_SUMMARY`. Set to `true` for default path, or provide a custom path. | diff --git a/dist/index.js b/dist/index.js index 2c69d17..1fa33bb 100644 --- a/dist/index.js +++ b/dist/index.js @@ -41139,9 +41139,23 @@ async function extractBundle(bundlePath, outputDir) { const files = countDeployedFiles(resolvedOutput); return { files, verified: true }; } - // Fallback: tar extraction + // Fallback: tar extraction. + // + // Defense-in-depth: even if this path ever runs again (e.g. if a future + // change reintroduces a "skip apm install" mode, or apm install transiently + // fails), exclude the lockfile + manifest. They are bundle metadata, not + // deployable output — the same files that `apm unpack` (the primary path) + // intentionally never copies. Leaking them into a git checkout dirties the + // workspace and breaks downstream `git checkout` steps. See microsoft/apm-action#26. info('APM not available — extracting with tar (no verification)...'); - const rc = await exec_exec('tar', ['xzf', resolvedBundle, '-C', resolvedOutput, '--strip-components=1'], { + const rc = await exec_exec('tar', [ + 'xzf', resolvedBundle, + '-C', resolvedOutput, + '--strip-components=1', + '--exclude=apm.lock.yaml', + '--exclude=apm.lock', + '--exclude=apm.yml', + ], { ignoreReturnCode: true, }); if (rc !== 0) { @@ -41325,13 +41339,39 @@ async function run() { auditReportPath = external_path_.resolve(resolvedDir, auditReportInput); } } - // RESTORE MODE: extract bundle, skip APM installation entirely. + // RESTORE MODE: install APM, then extract via `apm unpack`. // Directory was already created above (actionOwnsDir = true for bundle mode). + // + // Why install APM in restore mode: + // `apm unpack` honors the bundle contract — it copies only files listed in + // the lockfile's `deployed_files` (primitives + apm_modules) and never + // writes `apm.lock.yaml` / `apm.yml` to `working-directory`. The previous + // "skip install" optimization forced extractBundle through its raw + // `tar xzf --strip-components=1` fallback, which dumped the *entire* + // bundle — including lockfile and apm.yml — into working-directory. + // When working-directory was a git checkout (the default + // `${{ github.workspace }}`), those tracked files became dirty and any + // subsequent `git checkout` (e.g. gh-aw's pull_request_target PR-branch + // checkout) aborted with: + // error: Your local changes to the following files would be + // overwritten by checkout: apm.lock.yaml + // See microsoft/apm-action#26. + // + // The install is tool-cached (see installer.ts), so this adds at most a + // single small download per runner — negligible vs. the cost of a typical + // agent job, and we get bundle integrity verification for free. if (bundleInput) { + await ensureApmInstalled(); const bundlePath = await resolveLocalBundle(bundleInput, resolvedDir); info(`Restoring bundle: ${bundlePath}`); const result = await extractBundle(bundlePath, resolvedDir); - const verifiedMsg = result.verified ? ' (verified)' : ' (unverified — install APM for integrity checks)'; + // Restore mode now installs APM up-front, so the verified `apm unpack` + // path is the expected outcome. The unverified branch only runs if APM + // install failed transiently and extractBundle fell through to its tar + // fallback — point operators at the install logs, not at re-installing. + const verifiedMsg = result.verified + ? ' (verified)' + : ' (unverified — APM install did not complete; see earlier install logs)'; info(`Restored ${result.files} file(s)${verifiedMsg}`); const primitivesPath = external_path_.join(resolvedDir, '.github'); setOutput('primitives-path', primitivesPath); diff --git a/src/__tests__/bundler.test.ts b/src/__tests__/bundler.test.ts index d515c9e..1dfddbc 100644 --- a/src/__tests__/bundler.test.ts +++ b/src/__tests__/bundler.test.ts @@ -141,6 +141,15 @@ describe('extractBundle', () => { const tarCall = mockExec.mock.calls.find(c => c[0] === 'tar'); expect(tarCall).toBeTruthy(); expect(tarCall![1]).toContain('--strip-components=1'); + + // Defense-in-depth (microsoft/apm-action#26): even if the tar fallback + // ever runs, it must NOT extract apm.lock.yaml or apm.yml into the output + // dir. Those are bundle metadata, never deployable output, and writing + // them to a git checkout dirties the workspace and breaks downstream + // `git checkout` steps. + expect(tarCall![1]).toContain('--exclude=apm.lock.yaml'); + expect(tarCall![1]).toContain('--exclude=apm.lock'); + expect(tarCall![1]).toContain('--exclude=apm.yml'); }); it('throws when bundle file does not exist', async () => { diff --git a/src/__tests__/runner.test.ts b/src/__tests__/runner.test.ts index 4a3c256..f594cb5 100644 --- a/src/__tests__/runner.test.ts +++ b/src/__tests__/runner.test.ts @@ -38,10 +38,13 @@ jest.unstable_mockModule('../installer.js', () => ({ ensureApmInstalled: mockEnsureApmInstalled, })); +const mockResolveLocalBundle = jest.fn<() => Promise>(); +const mockExtractBundle = jest.fn<() => Promise<{ files: number; verified: boolean }>>(); +const mockRunPackStep = jest.fn<() => Promise>(); jest.unstable_mockModule('../bundler.js', () => ({ - resolveLocalBundle: jest.fn(), - extractBundle: jest.fn(), - runPackStep: jest.fn(), + resolveLocalBundle: mockResolveLocalBundle, + extractBundle: mockExtractBundle, + runPackStep: mockRunPackStep, })); const { clearPrimitives, run } = await import('../runner.js'); @@ -652,3 +655,54 @@ describe('run', () => { } }); }); + +describe('run (restore mode)', () => { + let tmpDir: string; + + beforeEach(() => { + jest.clearAllMocks(); + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'apm-action-restore-')); + mockEnsureApmInstalled.mockResolvedValue(undefined); + mockExec.mockResolvedValue(0); + mockGetExecOutput.mockResolvedValue({ exitCode: 0, stdout: '', stderr: '' }); + mockResolveLocalBundle.mockImplementation(async () => path.join(tmpDir, 'bundle.tar.gz')); + mockExtractBundle.mockResolvedValue({ files: 5, verified: true }); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + // Regression test for microsoft/apm-action#26. + // Before the fix, restore mode deliberately skipped ensureApmInstalled() for + // speed, which forced extractBundle through its raw `tar xzf` fallback and + // dumped the bundle's apm.lock.yaml / apm.yml into working-directory. That + // dirtied any git checkout consumers (e.g. gh-aw pull_request_target flows) + // and broke their subsequent `git checkout` step. Restore mode must always + // install APM so extractBundle takes the verified `apm unpack` path. + it('installs APM before extracting (so apm unpack is used, not the tar fallback)', async () => { + mockGetInput.mockImplementation((name: unknown) => { + switch (name) { + case 'working-directory': return tmpDir; + case 'bundle': return './bundle.tar.gz'; + case 'isolated': return 'false'; + case 'pack': return 'false'; + case 'compile': return 'false'; + case 'script': return ''; + default: return ''; + } + }); + + await run(); + + expect(mockSetFailed).not.toHaveBeenCalled(); + expect(mockEnsureApmInstalled).toHaveBeenCalledTimes(1); + expect(mockExtractBundle).toHaveBeenCalledTimes(1); + + // Order matters: install must complete before extract starts so apm unpack + // is on PATH when extractBundle probes for it. + const installOrder = mockEnsureApmInstalled.mock.invocationCallOrder[0]; + const extractOrder = mockExtractBundle.mock.invocationCallOrder[0]; + expect(installOrder).toBeLessThan(extractOrder); + }); +}); diff --git a/src/bundler.ts b/src/bundler.ts index 8d1bd58..16e7eca 100644 --- a/src/bundler.ts +++ b/src/bundler.ts @@ -77,9 +77,23 @@ export async function extractBundle(bundlePath: string, outputDir: string): Prom return { files, verified: true }; } - // Fallback: tar extraction + // Fallback: tar extraction. + // + // Defense-in-depth: even if this path ever runs again (e.g. if a future + // change reintroduces a "skip apm install" mode, or apm install transiently + // fails), exclude the lockfile + manifest. They are bundle metadata, not + // deployable output — the same files that `apm unpack` (the primary path) + // intentionally never copies. Leaking them into a git checkout dirties the + // workspace and breaks downstream `git checkout` steps. See microsoft/apm-action#26. core.info('APM not available — extracting with tar (no verification)...'); - const rc = await exec.exec('tar', ['xzf', resolvedBundle, '-C', resolvedOutput, '--strip-components=1'], { + const rc = await exec.exec('tar', [ + 'xzf', resolvedBundle, + '-C', resolvedOutput, + '--strip-components=1', + '--exclude=apm.lock.yaml', + '--exclude=apm.lock', + '--exclude=apm.yml', + ], { ignoreReturnCode: true, }); if (rc !== 0) { diff --git a/src/runner.ts b/src/runner.ts index 687d21a..1ca57c7 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -87,13 +87,40 @@ export async function run(): Promise { } } - // RESTORE MODE: extract bundle, skip APM installation entirely. + // RESTORE MODE: install APM, then extract via `apm unpack`. // Directory was already created above (actionOwnsDir = true for bundle mode). + // + // Why install APM in restore mode: + // `apm unpack` honors the bundle contract — it copies only files listed in + // the lockfile's `deployed_files` (primitives + apm_modules) and never + // writes `apm.lock.yaml` / `apm.yml` to `working-directory`. The previous + // "skip install" optimization forced extractBundle through its raw + // `tar xzf --strip-components=1` fallback, which dumped the *entire* + // bundle — including lockfile and apm.yml — into working-directory. + // When working-directory was a git checkout (the default + // `${{ github.workspace }}`), those tracked files became dirty and any + // subsequent `git checkout` (e.g. gh-aw's pull_request_target PR-branch + // checkout) aborted with: + // error: Your local changes to the following files would be + // overwritten by checkout: apm.lock.yaml + // See microsoft/apm-action#26. + // + // The install is tool-cached (see installer.ts), so this adds at most a + // single small download per runner — negligible vs. the cost of a typical + // agent job, and we get bundle integrity verification for free. if (bundleInput) { + await ensureApmInstalled(); + const bundlePath = await resolveLocalBundle(bundleInput, resolvedDir); core.info(`Restoring bundle: ${bundlePath}`); const result = await extractBundle(bundlePath, resolvedDir); - const verifiedMsg = result.verified ? ' (verified)' : ' (unverified — install APM for integrity checks)'; + // Restore mode now installs APM up-front, so the verified `apm unpack` + // path is the expected outcome. The unverified branch only runs if APM + // install failed transiently and extractBundle fell through to its tar + // fallback — point operators at the install logs, not at re-installing. + const verifiedMsg = result.verified + ? ' (verified)' + : ' (unverified — APM install did not complete; see earlier install logs)'; core.info(`Restored ${result.files} file(s)${verifiedMsg}`); const primitivesPath = path.join(resolvedDir, '.github');