Fix sample replay multi-repo lookup when Configure Git credentials clobbers origin (#37545)#37565
Conversation
…ntials" rewrote the workspace remote (#37545) When a workflow uses `checkout:` to bring a non-workflow repository into `$GITHUB_WORKSPACE`, the compiler-emitted "Configure Git credentials" step runs after the side-repo checkout and unconditionally executes: git remote set-url origin \ "https://x-access-token:${GITHUB_TOKEN}@github.com/${GITHUB_REPOSITORY}.git" This clobbers `remote.origin.url` at the workspace root so it points back at the workflow's own repository, even though the on-disk contents are the side repo. `find_repo_checkout.cjs` then fails because it identifies repos by scanning `git config --get remote.origin.url`, and the URL no longer matches the slug being searched for. In sample-replay mode the safe-output handler reports "Repository '...' not found in workspace" and emits zero outputs. Fix by consulting the existing compile-time checkout manifest first. The "Build checkout manifest for safe-outputs handlers" step writes `$RUNNER_TEMP/gh-aw/checkout-manifest.json` mapping `lowercase(slug) -> { repository, path, default_branch }` before any credential rewriting happens. The manifest is therefore the authoritative source for cross-repo lookup paths and is unaffected by later `git remote set-url` operations. Changes: - `checkout_manifest.cjs`: export `loadAllCheckouts()` returning a Map of every parsed manifest entry. - `find_repo_checkout.cjs`: - `findRepoCheckout()` calls `lookupCheckout(targetSlug)` after the allowed-repos validation block; on hit, returns the resolved path directly and skips the git-remote scan. Falls through to the existing git-dir + workspace-root scan when the manifest has no entry (covers cases where no manifest is written, e.g. unit tests or non-multi-repo workflows). - `buildRepoCheckoutMap()` seeds the map from `loadAllCheckouts()` first, then only adds git-scan results for slugs not already present from the manifest. - New helper `resolveManifestPath()` handles the manifest's `path: ""` convention (= workspace root) and path joining. - `find_repo_checkout.test.cjs`: six new tests under `describe("checkout manifest fallback (issue #37545)")` covering the clobbered-origin regression scenario, subdirectory paths, case-insensitive slug matching, allowedRepos interaction with manifest hits, fallback when the manifest is empty, and `buildRepoCheckoutMap` seeding from manifest entries.
There was a problem hiding this comment.
Pull request overview
This PR fixes a multi-repo sample replay/safe-output regression where the compiler-emitted “Configure Git credentials” step can clobber remote.origin.url at the workspace root, causing runtime git-remote scanning to misidentify the checked-out repository. The fix prioritizes the compiler-written checkout manifest as the source of truth, with git scanning as a fallback.
Changes:
- Added checkout-manifest-first lookup in
findRepoCheckout()and manifest seeding inbuildRepoCheckoutMap(). - Exported
loadAllCheckouts()to enumerate all manifest entries for map seeding. - Added targeted tests covering the clobbered-origin scenario and related manifest behaviors.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/find_repo_checkout.cjs | Consults checkout manifest before git-remote scanning; seeds checkout map from manifest entries. |
| actions/setup/js/checkout_manifest.cjs | Adds loadAllCheckouts() export to provide all manifest entries to callers. |
| actions/setup/js/find_repo_checkout.test.cjs | Adds tests for manifest-first lookup and fallback behavior (issue #37545). |
| .changeset/patch-find-repo-checkout-manifest-first.md | Documents the patch-level change and rationale. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #37565 does not have the implementation label (has_implementation_label=false) and has 0 new lines of code in business logic directories (well below the 100-line threshold). Neither enforcement condition is met. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧪 Test Quality Sentinel completed test quality analysis. test |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — the core fix is correct and well-structured; commenting with a few targeted improvements.
📋 Key Themes & Highlights
Key Themes
- Untested absolute-path branch (
resolveManifestPath): thepath.isAbsoluteescape hatch is unreachable in normal operation and opens a path-traversal vector if the manifest is tampered with. Remove it or add a containment assertion. - Implicit key-casing invariant (
loadAllCheckouts): the function trusts that manifest keys are already lowercase without enforcing it, creating a silent failure mode if the invariant drifts. - Test coverage gaps: the
!map.has(slug)guard — the actual fix forbuildRepoCheckoutMap— has no dedicated test; and the fallback tests only cover an empty manifest, not an absent one.
Positive Highlights
- ✅ Excellent PR description — problem, root cause, chosen approach, and alternatives all documented.
- ✅ Manifest-first with git-scan fallback is the right ordering, and the fallthrough is clean.
- ✅ Defensive type-guards in
loadAllCheckouts()protect against malformed manifest entries. - ✅ 135 lines of new targeted tests covering the regression scenario and five edge cases — great coverage for a bug fix.
- ✅
allowedReposvalidation is correctly preserved even when the manifest matches (confirmed by test). - ✅ Inline comment in
findRepoCheckoutclearly explains why the manifest is consulted beforegit config.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 935.5K · 280.6 AIC · ⌖ 13.8 AIC
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Three issues must be addressed before merge. The core manifest-first lookup design is correct and the regression test coverage is good; the problems are in the new helper functions, not the main fix logic.
### Blocking issues
1. loadAllCheckouts() does not normalize keys — checkout_manifest.cjs:110 sets map.set(key, ...) using the raw JSON object key without .trim().toLowerCase(). All callers use normalized (lowercase) slugs via normalizeRepoSlug, so a manifest with any mixed-case key causes buildRepoCheckoutMap to silently seed the map with an unreachable entry while the git-scan path adds a duplicate normalized entry for the same repo. One-line fix: map.set(key.trim().toLowerCase(), { ... }).
2. resolveManifestPath() does not clamp to workspace root — find_repo_checkout.cjs:138–139 passes absolute relPath values through unchanged and does not detect .. segments in relative paths. A malformed or tampered manifest path field can resolve outside $GITHUB_WORKSPACE. Fix: resolve the joined path and reject/ignore it if path.relative(workspaceRoot, resolved) starts with .., and treat absolute relPath as an error rather than a passthrough.
3. Manifest hit returns success: true without verifying the path exists — see inline comment at line 188. The manifest is written before actions/checkout runs; a failed checkout or stale manifest returns a nonexistent path with a success indicator. The fix is a cheap fs.existsSync guard before the early return, falling back to the git scan on miss.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 391K · 8.26 AIC · ⌖ 13.2 AIC
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics & Test Classification (6 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
📝 Notes — No Violations, Minor Observations�� Test inflation ratio —
|
This comment has been minimized.
This comment has been minimized.
|
@copilot review all comments and address unresolved review feedback, then refresh the branch and summarize any remaining blockers.
|
- resolveManifestPath: simplify redundant empty-string check; clamp result to workspace root, refusing absolute paths and .. traversal so a malformed or tampered manifest cannot redirect lookups outside $GITHUB_WORKSPACE. - findRepoCheckout: verify resolved manifest path exists on disk before returning success; fall back to git scan on stale/missing entries. - buildRepoCheckoutMap: only seed manifest entries whose resolved path is safe and exists, matching the guarantee provided by the git-scan branch. - Tests: cover unset/missing manifest env var, stale path fallback, absolute + .. traversal rejection, manifest-wins-over-git-scan priority, and the new buildRepoCheckoutMap existence guard.
|
✅ smoke-ci: safeoutputs CLI comment + comment-memory run (27094465052)
|
Summary
Fixes multi-repo checkout lookup failing when a "Configure Git credentials" workflow step rewrites
remote.origin.urlat the workspace root (#37545). The previous implementation relied solely ongit configremote-URL scanning; once that URL was overwritten by the credentials step, side-repo checkout paths became unreachable. The fix makesfindRepoCheckoutandbuildRepoCheckoutMapconsult the compiler-written checkout manifest first, falling back to the git-scan only when the manifest cannot resolve the requested repo.Problem
During sample-replay runs involving multiple repositories, the safe-output handler calls
findRepoCheckout/buildRepoCheckoutMapto locate side-repo working directories. When "Configure Git credentials" runs after the checkout steps, it overwritesremote.origin.urlfor the workspace root. Because the old logic walkedgit configto match remote URLs to paths, any repo whose remote URL had been replaced was silently unfindable, causing the entire multi-repo lookup to fail.Changes
actions/setup/js/find_repo_checkout.cjsresolveManifestPathhelper that locates the compiler-written checkout manifest relative to the workspace root, with workspace-escape safety checks to reject absolute paths and..-traversal.findRepoCheckoutandbuildRepoCheckoutMapto callresolveManifestPath(manifest-first) and only fall back to thegit configremote-URL scan when the manifest cannot supply an answer.actions/setup/js/checkout_manifest.cjsloadAllCheckouts()function returning all manifest entries as aMapkeyed by lowercase repo slug, enabling callers to seed bulk lookup structures from the manifest in one call.actions/setup/js/find_repo_checkout.test.cjs"checkout manifest fallback (issue #37545)"describe block covering:allowedReposvalidation..-traversal rejectionbuildRepoCheckoutMap.changeset/patch-find-repo-checkout-manifest-first.mdTest Coverage
All new behaviour is covered by the new
"checkout manifest fallback (issue #37545)"suite. Key edge cases explicitly tested: path-traversal safety, case normalisation,allowedReposgate, priority of manifest over live git-scan, and graceful handling of stale manifest entries pointing to missing paths.Risk
resolveManifestPathexplicitly rejects absolute paths and..segmentsloadAllCheckouts) plus internal logic rewriteRelated