fix: resolve target repo checkout path in push_to_pull_request_branch handlers

[Copilot]

When a workflow targets a repo checked out in a subdirectory (e.g. caido/proxy-frontend checked out under ./proxy-frontend), both the MCP handler and the conclusion handler were running git operations in GITHUB_WORKSPACE root instead of the subdirectory — making all agent commits invisible and producing "No commits were found to push."

Changes

• safe_outputs_handlers.cjs — Extend the findRepoCheckout guard in pushToPullRequestBranchHandler to also trigger when pushConfig["target-repo"] is set, not only when entry.repo is explicitly passed by the agent:

  // Before: only explicit agent-provided repo triggered checkout lookup
  if (entry.repo && entry.repo.trim()) { ... }
  
  // After: also triggers when workflow config specifies a target repo
  if ((entry.repo && entry.repo.trim()) || pushConfig["target-repo"]) {
    const checkoutResult = findRepoCheckout(itemRepo);
    // repoCwd is then passed to getCurrentBranch() and generateGitPatch()
  }

  Uses pushConfig["target-repo"] (not defaultTargetRepo) to avoid false-triggering on fallback values derived from context.repo / GITHUB_REPOSITORY.

• safe_outputs_handlers.test.cjs — Add coverage for the target-repo config path: error when checkout not found, and successful branch detection + patch generation from the correct subdirectory.

• push_to_pull_request_branch.cjs — Fix the conclusion handler (which applies the patch and pushes to the branch) to also run all git operations in the target repo's checkout directory when itemRepo differs from GITHUB_REPOSITORY:

  • Imports findRepoCheckout and calls it when the target repo differs from the workflow repo
  • Introduces a baseGitOpts = repoCwd ? { cwd: repoCwd } : {} helper spread into every git exec call: ls-remote, fetch, rev-parse --verify, checkout -B, rev-parse HEAD, bundle fetch/merge/update-ref, git am, patch-failure diagnostics, review-branch and fallback-branch checkout/push, pushSignedCommits, rev-list --count, and final rev-parse HEAD
  • Returns a clear error if the target repo checkout is not found in the workspace

• push_to_pull_request_branch.test.cjs — Add two cross-repo tests: error when target repo not found in workspace, and cwd correctly passed to git exec calls when a subdirectory checkout exists.

[pelikhan]

@copilot also fix #28374

[Copilot]

Pull request overview

Fixes push_to_pull_request_branch patch generation when a workflow targets a repo checked out into a subdirectory via push_to_pull_request_branch.target-repo, by resolving and using the correct checkout path.

Changes:

• Extend pushToPullRequestBranchHandler to call findRepoCheckout() when push_to_pull_request_branch.target-repo is configured (not only when entry.repo is provided).
• Update the checkout-not-found error text to match the new behavior.
• Add tests covering the configured target-repo path (error when not found; success when found, including correct repo_cwd and branch detection).

Show a summary per file

File	Description
actions/setup/js/safe_outputs_handlers.cjs	Resolve correct git working directory for patch generation when target-repo is configured.
actions/setup/js/safe_outputs_handlers.test.cjs	Add/adjust tests to validate configured target-repo checkout resolution and resulting behavior.

Copilot's findings

Tip

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

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

[Copilot]

Test name mentions defaultTargetRepo, but the behavior under test is specifically the push_to_pull_request_branch.target-repo configuration path. Renaming the test to reference target-repo (or “configured target repo”) would make the intent clearer and avoid confusion with the fallback default-target-repo logic derived from env/context.

Suggested change

	it("should return error when defaultTargetRepo checkout is not found and entry.repo is not set", async () => {
	it("should return error when configured target-repo checkout is not found and entry.repo is not set", async () => {

[Copilot]

The checkout-not-found error message dropped the explicit actions/checkout mention that was in the previous wording. Consider adding it back (while still referencing the path input) so it’s immediately clear which step/input the workflow author needs to adjust.

Suggested change

	error: `Repository '${itemRepo}' not found in workspace. Check out the target repo with a path in checkout. If checking out multiple repositories, use the 'path' input so the checkout can be located.`,
	error: `Repository '${itemRepo}' not found in workspace. Check out the target repo with actions/checkout and set its 'path' input so the checkout can be located. If checking out multiple repositories, ensure each actions/checkout step uses the appropriate 'path' input.`,

[github-actions]

🧪 Test Quality Sentinel Report

Test Quality Score: 72/100

⚠️ Acceptable, with suggestions

Metric	Value
New/modified tests analyzed	2
✅ Design tests (behavioral contracts)	2 (100%)
⚠️ Implementation tests (low value)	0 (0%)
Tests with error/edge cases	1 (50%)
Duplicate test clusters	0
Test inflation detected	Yes (4.7:1 ratio)
🚨 Coding-guideline violations	0

---

Test Classification Details

Test	File	Classification	Issues Detected
should return error when defaultTargetRepo checkout is not found and entry.repo is not set	safe_outputs_handlers.test.cjs	✅ Design	Error path covered; behavioral contract on error shape
should detect branch from defaultTargetRepo checkout when entry.repo is not provided	safe_outputs_handlers.test.cjs	✅ Design	Verifies behavioral output via mockAppendSafeOutput; some debug-message assertions are implementation details

---

Flagged Tests — Requires Review

⚠️ Test Inflation: safe_outputs_handlers.test.cjs

Issue: 49 lines added to the test file vs. 10 lines added to the production file — a 4.7:1 ratio, exceeding the 2:1 threshold.

This is partly expected here because the PR adds two new test scenarios for a fix that required only a small targeted change in production code. The inflation is not a sign of test padding — the tests cover two legitimate new code paths. However, reviewers should confirm the test volume matches the actual behavioral surface introduced by the fix.

⚠️ Partial implementation-detail assertions in success test

Test: should detect branch from defaultTargetRepo checkout when entry.repo is not provided

Issue: Three of the five expect calls assert on mockServer.debug messages (internal logging), which are implementation details rather than observable behavior. If the debug message wording changes in a refactor, these assertions will break even though the behavior is correct.

The core behavioral assertion (mockAppendSafeOutput called with { type, branch, repo_cwd }) is well-formed and captures the observable contract.

Suggested improvement: Remove or soften the three mockServer.debug assertions, or move them to a separate, clearly-labeled "debug output" test. Keep the mockAppendSafeOutput assertion as the primary behavioral check.

---

Language Support

Tests analyzed:

• 🟨 JavaScript (*.test.cjs): 2 new tests (vitest)

---

Verdict

✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). No coding-guideline violations detected. Minor test inflation and debug-message assertions noted above are improvement opportunities, not blockers.

---

📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

• Assert on observable outputs, return values, or state changes
• Cover error paths and boundary conditions
• Would catch a behavioral regression if deleted
• Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

• Assert on internal function calls (mocking internals)
• Only test the happy path with typical inputs
• Break during legitimate refactoring even when behavior is correct
• Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

References: §24918924358

🧪 Test quality analysis by Test Quality Sentinel · ● 432.8K · ◷

[github-actions]

✅ Test Quality Sentinel: 72/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). Minor test inflation (4.7:1 ratio) and some debug-message assertions noted in the comment, but no blocking issues.

[Copilot]

@copilot also fix #28374

Fixed in commit 82ecb33. push_to_pull_request_branch.cjs (the conclusion handler) now:

1. Imports findRepoCheckout
2. After resolving itemRepo, calls findRepoCheckout when itemRepo differs from GITHUB_REPOSITORY to locate the subdirectory checkout
3. Uses a baseGitOpts = repoCwd ? { cwd: repoCwd } : {} helper spread into every exec.exec / exec.getExecOutput call (fetch, checkout, am, push, ls-remote, rev-parse, rev-list, bundle ops, pushSignedCommits, fallback/review branch ops)
4. Returns a clear error if the target repo checkout isn't found in the workspace

[github-actions]

Hey @Copilot 👋 — great work on tracking down the cross-repo checkout path bug in the push-to-pull-request handlers! The fix is well-scoped, the description clearly explains the before/after behaviour, and the new tests cover both the error and happy-path cases for the target-repo config path. This PR looks ready for maintainer review — nothing blocking here. 🎉

Generated by Contribution Check · ● 731K · ◷

[github-actions]

🩺 Examination complete! CI Failure Doctor has delivered the diagnosis. Prescription issued! 💊

[github-actions]

🩺 CI Doctor Diagnosis

Checked 82ecb333eebf0ed6083896b1d1626e7873c8e4c8

Summary

The js-typecheck check is failing due to two TypeScript type errors introduced in actions/setup/js/push_to_pull_request_branch.cjs by this PR's changes. Both errors relate to the new baseGitOpts helper pattern and the cross-repo allowedRepos forwarding.

Failing Checks

Check	Conclusion	Root Cause
js-typecheck	failure	2 TS type errors in push_to_pull_request_branch.cjs

Detailed Analysis

Error 1 — push_to_pull_request_branch.cjs(365,89): TS2322 Set<string> not assignable to string | string[] | undefined

error TS2322: Type 'Set<string>' is not assignable to type 'string | string[] | undefined'.
  Type 'Set<string>' is missing the following properties from type 'string[]': length, pop, push, concat, and 24 more.

• allowedRepos is Set<string> (from resolveTargetRepoConfig in repo_helpers.cjs, JSDoc: @property {Set<string>} allowedRepos)
• findRepoCheckout's third-argument options type is @param {string[]|string} [options.allowedRepos] — it accepts only string[] | string, not Set<string>
• Line: findRepoCheckout(itemRepo, process.env.GITHUB_WORKSPACE, { allowedRepos }) at line ~365

Fix: Spread the set into an array before passing:

const checkoutResult = findRepoCheckout(itemRepo, process.env.GITHUB_WORKSPACE, { allowedRepos: [...allowedRepos] });

Or change find_repo_checkout.cjs's JSDoc for options.allowedRepos to {Set<string>|string[]|string}.

---

Error 2 — push_to_pull_request_branch.cjs(771,51): TS2345 cwd?: undefined not assignable to cwd: string

error TS2345: Argument of type '... | { gitAuthEnv: any; cwd?: undefined; ... }' is not assignable to parameter of type '{ ...; cwd: string; ... }'.
  Type 'undefined' is not assignable to type 'string'.

• baseGitOpts is typed as { cwd: string } | {} (ternary: repoCwd ? { cwd: repoCwd } : {})
• When repoCwd is undefined (same-repo case), spreading baseGitOpts leaves cwd absent, which TypeScript represents as cwd?: undefined
• pushSignedCommits in push_signed_commits.cjs requires cwd: string (non-optional per JSDoc @param {string} cwd)
• Line: const pushedSha = await pushSignedCommits({ ..., ...baseGitOpts, gitAuthEnv }) at line ~771

Fix option A — always pass an explicit cwd:

const pushedSha = await pushSignedCommits({
  githubClient,
  owner: repoParts.owner,
  repo: repoParts.repo,
  branch: branchName,
  baseRef: remoteHeadBeforePatch || `origin/\$\{branchName}`,
  cwd: repoCwd ?? process.env.GITHUB_WORKSPACE ?? process.cwd(),
  gitAuthEnv,
});

Fix option B — make cwd optional in push_signed_commits.cjs's JSDoc (@param {string} [cwd]), letting the underlying exec calls fall back to the runner's working directory.

Recommended Fixes

• Error 1: In push_to_pull_request_branch.cjs line ~365, change { allowedRepos } to { allowedRepos: [...allowedRepos] } so the Set<string> is converted to string[] as findRepoCheckout expects.
• Error 2: In push_to_pull_request_branch.cjs line ~771, replace the ...baseGitOpts spread with an explicit cwd: repoCwd ?? process.env.GITHUB_WORKSPACE ?? process.cwd() (or make cwd optional in push_signed_commits.cjs's JSDoc).

Prevention Tips

When spreading a conditional object (x ? { prop: x } : {}) into a function call, TypeScript can't guarantee the required property is present — prefer an explicit named argument with a fallback over the spread pattern for required parameters. Similarly, avoid passing a Set where an array type is declared in JSDoc; always convert with [...set] or Array.from(set) at the call site.

Analysis Steps

1. Read /tmp/ci-doctor/pr/summary.txt — identified 1 failing check: js-typecheck (job 72977133465, run 24919086929).
2. Called get_job_logs with job_id=72977133465, return_content=true, tail_lines=150 — retrieved full tsc output.
3. Extracted two ##[error] lines from the log: TS2322 at line 365 col 89, TS2345 at line 771 col 51 in push_to_pull_request_branch.cjs.
4. Read push_to_pull_request_branch.cjs around lines 355–375 and 760–785 to see the call sites.
5. Grepped for findRepoCheckout and pushSignedCommits imports/definitions — found they come from find_repo_checkout.cjs and push_signed_commits.cjs respectively.
6. Read JSDoc type annotations in find_repo_checkout.cjs (options.allowedRepos: string[]|string) and push_signed_commits.cjs (cwd: string, required).
7. Grepped repo_helpers.cjs to confirm allowedRepos is Set<string> from resolveTargetRepoConfig.
8. Identified root cause for each error and drafted targeted fixes.

🩺 Diagnosis provided by CI Failure Doctor · ● 155.5K · ◷

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/ci-doctor.md@ea350161ad5dcc9624cf510f134c6a9e39a6f94d

[pelikhan]

@copilot review all comments

[Copilot]

@copilot review all comments

Done in 47c722f:

• Renamed the test to "should return error when configured target-repo checkout is not found and entry.repo is not set"
• Updated the not-found error message in both handlers to explicitly mention actions/checkout and the path input: "Check out the target repo with actions/checkout and set its 'path' input so the checkout can be located. If checking out multiple repositories, ensure each actions/checkout step uses the appropriate 'path' input."
• Updated test assertions to match