Fix close_pull_request target repo resolution at safe-output ingestion and alias handling#37073
Fix close_pull_request target repo resolution at safe-output ingestion and alias handling#37073Copilot wants to merge 9 commits into
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens default target repository resolution for safe-output handlers by teaching repo_helpers.getDefaultTargetRepo to recognize additional dynamic-config key variants (target_repo, targetRepo) in addition to target-repo, aligning close_pull_request behavior with other handlers that already resolve these shapes correctly.
Changes:
- Extend
getDefaultTargetRepo(config)to accepttarget_repo(snake_case) andtargetRepo(camelCase) aliases. - Expand
repo_helpersconfig JSDoc to document the supported alias keys. - Add regression tests covering both new alias variants.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/repo_helpers.cjs | Accepts target_repo and targetRepo config keys when resolving the default target repo. |
| actions/setup/js/repo_helpers.test.cjs | Adds focused tests ensuring alias keys resolve to the expected default target repo. |
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: 0
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #37073 does not have the 'implementation' label (has_implementation_label=false) and has 0 new lines of code in business logic directories (default_business_additions=0, threshold=100). Neither Condition A nor Condition B is met. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
pelikhan
left a comment
There was a problem hiding this comment.
@copilot the fox looks incorrect read the issue:
When safe outputs attempts to close the requested PR, it attempts to close it from the working repo where the agentic workflow resides, instead of the target repo. This results in a failure.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — approving with minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Precedence not tested: the
target-repo>target_repo>targetRepopriority order is not exercised by any test case, so it only lives in the implementation. - Falsy-value fallthrough: truthiness guards silently skip empty strings/null — worth documenting the intent (or switching to
!== undefined). - Upstream normalization opportunity: multi-alias checks in helpers compound over time; a single
normalizeConfig()at ingestion would prevent future drift.
Positive Highlights
- ✅ Root cause correctly identified and fixed in the one place that was inconsistent.
- ✅ JSDoc typedef updated alongside the implementation — good hygiene.
- ✅ Regression tests added for both new alias forms before the PR landed.
- ✅ Fix is non-breaking: existing callers using
target-repoare unaffected.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.5M · 186.8 AIC
| const config = { targetRepo: "camel-org/camel-repo" }; | ||
| const result = getDefaultTargetRepo(config); | ||
| expect(result).toBe("camel-org/camel-repo"); | ||
| }); |
There was a problem hiding this comment.
[/tdd] No test covers the precedence rule when multiple aliases coexist — e.g. { "target-repo": "a/b", targetRepo: "c/d" }. Without it, the priority order (target-repo > target_repo > targetRepo) lives only in implementation, not in the spec.
💡 Suggested test
it("should prefer target-repo over targetRepo when both are set", async () => {
const { getDefaultTargetRepo } = await import("./repo_helpers.cjs");
const config = { "target-repo": "kebab-org/repo", targetRepo: "camel-org/repo" };
const result = getDefaultTargetRepo(config);
expect(result).toBe("kebab-org/repo");
});This makes the tie-breaking rule explicit and guards against accidental reordering.
| if (config["target-repo"]) { | ||
| return config["target-repo"]; | ||
| } | ||
| if (config.target_repo) { |
There was a problem hiding this comment.
[/tdd] if (config.target_repo) silently skips falsy values — an empty string "" or null would fall through to the next alias or env var without any signal. The original target-repo path had the same gap, but extending the logic is a good moment to clarify the intended behavior with a test.
💡 Suggested test (or inline comment)
// Option A: document that empty string intentionally falls through
it("should fall through to env var when target_repo is empty string", async () => {
process.env.GH_AW_TARGET_REPO_SLUG = "env-org/env-repo";
const { getDefaultTargetRepo } = await import("./repo_helpers.cjs");
const result = getDefaultTargetRepo({ target_repo: "" });
expect(result).toBe("env-org/env-repo");
});Alternatively, switch to !== undefined checks if an explicit empty override should be respected. Either approach is fine — the key is making it explicit.
| if (config && config["target-repo"]) { | ||
| return config["target-repo"]; | ||
| // First check if there's an explicit target repo in config | ||
| if (config) { |
There was a problem hiding this comment.
[/diagnose] The root issue is that config keys are not normalized at ingestion — callers can produce target-repo, target_repo, or targetRepo interchangeably. The fix is correct here, but if allowed_repos or future keys develop the same variant shapes, each helper will need the same multi-alias treatment independently.
💡 Follow-up to consider
A single upstream normalizeConfig(config) step — called before any handler receives the config — would canonicalize all keys to one form (e.g. kebab-case) and prevent the pattern from spreading. This is not blocking, but worth tracking as a follow-up.
function normalizeConfig(config) {
if (!config) return config;
return {
...config,
"target-repo": config["target-repo"] ?? config.target_repo ?? config.targetRepo,
// normalize other keys here
};
}
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (2 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 70/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). Both new tests verify the observable return value of getDefaultTargetRepo for the two newly supported config key aliases (target_repo, targetRepo), directly covering the behavioral contract this PR introduces.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed. Addressed in commits: |
|
@copilot correct issue at ingestion in safe outputs MCP server |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed at safe-outputs MCP ingestion.
Covered by new regression tests in Fix commits: |
close_pull_requestwas not consistently resolving the intended target repository in dynamic safe-output paths, causing operations to fall back to the workflow host repo instead of the requested target repo.Target repo resolution hardening
repo_helpers.cjsto support all accepted config key variants for default target repo:target-repotarget_repotargetReporepoas canonical):repotarget-repotarget_repotargetRepoSafe outputs MCP ingestion fix
safe_outputs_tools_loader.cjsso repo alias inputs are normalized at ingestion time:target-repotarget_repotargetReporeporepotoclose_pull_requesttool schema insafe_outputs_tools.jsonso canonical repo survives strict schema filtering.Regression coverage
repo_helpers.test.cjsto assert alias handling for:target_repo,targetRepotarget_repo,targetRepoclose_pull_request.test.cjscoverage to verify PR close operations route to the aliased message target repo (target_repo) rather than the workflow host repository.safe_outputs_tools_loader.test.cjscoverage for strict-schema ingestion behavior:target_repo→repo)repoprecedence over alias fields.