trial: render logical repo in github-context prompt#50640
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (91 additions detected). |
There was a problem hiding this comment.
🟡 Changes recommended
The directive still permits omitted repository arguments, causing MCP calls to retain the host-repository default.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Updates trial-mode prompts to identify the logical target repository.
Changes:
- Rewrites GitHub context for logical-repo trials.
- Adds trial and non-trial unit coverage.
File summaries
| File | Description |
|---|---|
pkg/workflow/unified_prompt_step.go |
Rewrites repository context and adds trial guidance. |
pkg/workflow/unified_prompt_step_test.go |
Tests trial and non-trial prompt behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| promptText = strings.Replace(promptText, repoLine, replacement, 1) | ||
|
|
||
| directive := fmt.Sprintf( | ||
| "\nThis workflow is running in trial mode. The repository above (%s) is the logical target repository. When calling GitHub tools without an explicit owner/repo, use this repository.\n", |
| var githubContext string | ||
| for _, section := range sections { | ||
| if !section.IsFile && strings.Contains(section.Content, "github-context") { | ||
| githubContext = section.Content | ||
| break | ||
| } | ||
| } | ||
| require.NotEmpty(t, githubContext, "Should have a github-context section") | ||
|
|
||
| assert.Contains(t, githubContext, "**repository**:", | ||
| "github-context should report the repository via the github.repository expression") |
| assert.Contains(t, githubContext, "trial mode", | ||
| "github-context should include a trial mode directive") |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Test Quality Sentinel Report 🧪
PR: #50640 — "trial: render logical repo in github-context prompt"
Test Coverage: 2 new behavioral tests added
Test Inflation Ratio: 60 test lines / 31 prod lines = 1.94:1 ✅ (under 2:1 threshold)
Summary
Two well-structured behavioral tests were added to verify trial-mode repository context rendering. Both tests:
- Cover distinct behavioral contracts (trial mode ON vs. OFF)
- Include descriptive failure messages
- Test both positive and negative assertions
- Avoid mocking, instead testing actual integration behavior
- Are designed as complementary control tests
Quality Analysis
Score: 100/100 ✅ Excellent
Breakdown:
- Design tests: 2/2 (100%) → 40 points
- Edge-case coverage: 2/2 (100%) → 30 points
- No duplicates → 20 points
- Test inflation (1.94 < 2:1) → 10 points
Implementation tests: 0% (threshold: ≤30%)
Quality Signals ✅
- No mock libraries — tests integration behavior, not implementation details
- Complementary coverage — two tests form control pair (ON/OFF scenarios)
- Assertion quality — all assertions include descriptive failure messages
- Edge cases — both positive and negative assertions
- Build tags — file has required
//go:build !integrationtag - Test inflation — 1.94:1 ratio is acceptable (< 2:1 threshold)
Test Details
TestCollectPromptSections_TrialLogicalRepoGitHubContext
- Verifies trial mode with logical target routes agent prompts to correct repository
- 4 assertions with descriptive messages
- Tests design invariant: "In trial mode, agent's notion of repository = logical target, not host"
- Covers edge case via negative assertions (what should NOT appear in output)
TestCollectPromptSections_NoTrialLogicalRepoKeepsHostRepo
- Verifies default behavior without trial mode preserves standard github.repository reference
- 3 assertions with descriptive messages
- Control test ensuring normal workflow unaffected by trial mode feature
- Negative assertion prevents regression in non-trial mode workflows
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving with two minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Silent no-op risk:
strings.Replacewon't error if the template line drifts; a log guard would surface future regressions early. - Assertion tightness: the directive assertion checks for
"trial mode"but not the actual logical repo slug — a minimal addition would close the gap.
Positive Highlights
- ✅ Clean, scoped helper with a well-chosen single-replacement call
- ✅ Defensive guard (
TrialMode && TrialLogicalRepo != "") prevents any impact on non-trial workflows - ✅ Both paths (trial and non-trial) are covered by tests
- ✅ Fallback for missing close tag prevents data loss
| func applyTrialLogicalRepoToGitHubContext(promptText, logicalRepo string) string { | ||
| const repoLine = "- **repository**: ${{ github.repository }}" | ||
| replacement := "- **repository**: " + logicalRepo | ||
| promptText = strings.Replace(promptText, repoLine, replacement, 1) |
There was a problem hiding this comment.
[/diagnosing-bugs] Silent no-op if the repository line format ever drifts — strings.Replace returns the original string unchanged with no log or error.
💡 Suggestion
If the template changes (extra space, different casing, etc.) the replacement silently finds nothing, so the agent still sees the host repo with no indication anything failed.
Consider adding a guard:
replaced := strings.Replace(promptText, repoLine, replacement, 1)
if replaced == promptText {
unifiedPromptLog.Printf("warning: repository line not found in github-context prompt; logical repo not applied")
}
promptText = replaced@copilot please address this.
|
|
||
| sections := compiler.collectPromptSections(data) | ||
|
|
||
| var githubContext string |
There was a problem hiding this comment.
[/tdd] The trial-mode test asserts the directive contains the string "trial mode" but not the logical repo slug inside the directive text itself — a future rephrasing could drop the slug from the directive while still passing this test.
💡 Suggestion
Add a tighter assertion on the directive content:
assert.Contains(t, githubContext, "owner/target",
"trial mode directive should reference the logical repo slug")This ensures both the repository line and the directive consistently reference the logical repo.
@copilot please address this.
There was a problem hiding this comment.
Verdict: Request changes — the fix has the same silent-failure shape as the bug it's fixing
💡 Themes
The intent is right (rewrite the reported repository in trial mode), but applyTrialLogicalRepoToGitHubContext rewrites the repository line via an exact-literal strings.Replace with no signal or fallback if the match fails. If the underlying template (github_context_prompt.md) ever drifts even slightly, this silently reverts to reporting the host repo — the exact "undetectable in a green run" failure mode described in the PR body — while still appending a directive that falsely claims a specific repo was substituted. There's also no test coverage for that drift scenario, nor for the interaction between the trial-mode injection and the checkout-list injection that share the same </github-context> insertion point. See inline comments for concrete fixes.
| func applyTrialLogicalRepoToGitHubContext(promptText, logicalRepo string) string { | ||
| const repoLine = "- **repository**: ${{ github.repository }}" | ||
| replacement := "- **repository**: " + logicalRepo | ||
| promptText = strings.Replace(promptText, repoLine, replacement, 1) |
There was a problem hiding this comment.
The strings.Replace on repoLine silently no-ops if the literal template string doesn't match, which would silently reintroduce the exact host-repo leak this PR exists to fix.
💡 Fix: detect and surface a failed replacement
strings.Replace(promptText, repoLine, replacement, 1) gives no signal whether a match occurred. If pkg/workflow/prompts/github_context_prompt.md is later edited (reformatted, reindented, or the ${{ github.repository }} expression is changed), this function will silently return the unmodified text — still containing ${{ github.repository }} — while still appending the directive claiming "The repository above (%s) is the logical target repository." That's an internally inconsistent, wrong prompt with no compile-time or runtime signal, i.e. exactly the silent host/logical-repo mismatch class of bug this PR was written to eliminate.
Suggested change:
newText := strings.Replace(promptText, repoLine, replacement, 1)
if newText == promptText {
unifiedPromptLog.Printf("WARNING: could not rewrite repository line for trial logical repo %q; github-context template may have changed", logicalRepo)
return promptText // don't append a directive that references a substitution that never happened
}
promptText = newText| // No run of four or more newlines (i.e., 3+ consecutive blank lines) anywhere. | ||
| assert.NotContains(t, output, "\n\n\n\n", "blank run should be capped throughout the output") | ||
| } | ||
|
|
There was a problem hiding this comment.
No test exercises the failure mode where repoLine doesn't match promptText, so a template-drift regression that silently reintroduces the host-repo leak would pass CI undetected.
💡 Add a regression test for template drift
Both new tests only check the "happy path" where githubContextPromptText contains the exact repoLine constant. Add a test that calls applyTrialLogicalRepoToGitHubContext directly with a prompt text that does not contain the literal - **repository**: ${{ github.repository }} line (e.g. simulating template drift), and assert on the desired behavior (e.g. no directive appended, or a specific fallback), once the fallback logic from the sibling comment is implemented. Without this, a future edit to github_context_prompt.md can silently break trial-mode's repo redirection with zero test signal — the exact "undetectable in a green run" failure class described in this PR's own description.
| // logical target. Rewrite the reported repository so the agent's notion of "the | ||
| // repository" matches the logical target, and add a directive so GitHub MCP calls | ||
| // that omit owner/repo don't silently operate against the host repo. | ||
| if data.TrialMode && data.TrialLogicalRepo != "" { |
There was a problem hiding this comment.
There's no test covering the case where trial-mode logical-repo injection and buildCheckoutsPromptContent both fire in the same run, even though both insert content at the same </github-context> anchor sequentially in this function.
💡 Add an interaction test
applyTrialLogicalRepoToGitHubContext inserts its directive before </github-context> first, then (a few lines later) the checkout-list injection does strings.LastIndex(combinedPromptText, closeTag) again and inserts its own content before the same tag. This ordering happens to work today (checkouts land after the trial directive, both still land before the closing tag), but there's no test asserting the combined output is well-formed (e.g. checkout content isn't inserted between the repository line and the trial directive, tags aren't duplicated, and the trial directive doesn't get needlessly re-scanned/altered by expression extraction due to ${{ ... }}-looking content it doesn't produce). Add a test with TrialMode: true, TrialLogicalRepo set, and non-empty CheckoutConfigs to lock in the expected ordering.
There was a problem hiding this comment.
Review: trial logical repo in github-context prompt
The fix is well-scoped and correct: it patches only the trial-mode code path, leaves non-trial workflows untouched, and the tests cover both branches.
One non-blocking suggestion (inline): the strings.Replace call at line 800 silently no-ops if the expected repo line is absent from the prompt text. The directive is still appended, creating a misleading prompt that contradicts itself. Adding a log warning when the replacement doesn't occur would make template drift detectable at run time.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 22.3 AIC · ⌖ 12.1 AIC · ⊞ 5.4K
| func applyTrialLogicalRepoToGitHubContext(promptText, logicalRepo string) string { | ||
| const repoLine = "- **repository**: ${{ github.repository }}" | ||
| replacement := "- **repository**: " + logicalRepo | ||
| promptText = strings.Replace(promptText, repoLine, replacement, 1) |
There was a problem hiding this comment.
If repoLine is not found in promptText (e.g., the template in github_context_prompt.md is later updated), strings.Replace silently no-ops and returns the original text unchanged. The host repo remains visible in the prompt, but the directive still says "The repository above (owner/target) is the logical target" — a direct contradiction that would mislead the agent with no observable failure.
Consider logging a warning when the replacement doesn't occur:
if !strings.Contains(promptText, repoLine) {
unifiedPromptLog.Printf("WARNING: trial logical repo substitution skipped: repo line not found in github-context prompt")
}
promptText = strings.Replace(promptText, repoLine, replacement, 1)@copilot please address this.
|
@copilot please resolve the remaining review feedback on this PR:
Run: https://github.com/github/gh-aw/actions/runs/31034308996
|
PR Triage: #50640Category: feature | Risk: medium | Priority Score: 55/100 (impact 25, urgency 15, quality 15)
|
In trial mode,
--logical-repo owner/targetredirects checkout and safe-output targeting to the logical repo, but the agent prompt's<github-context>.repositoryblock still reported the host repo. GitHub MCP calls that omit explicitowner/repotherefore silently operated against the host repo — undetectable from a green run.The github-context prompt renders
- **repository**: ${{ github.repository }}unconditionally, which resolves to the host repo.TrialLogicalRepowas only consumed by safe-outputs env and checkout generation, never reaching prompt rendering.Changes
unified_prompt_step.go: whenTrialMode && TrialLogicalRepo != "", post-process the github-context section via a newapplyTrialLogicalRepoToGitHubContexthelper that:- **repository**:line to the literal logical slug instead of${{ github.repository }}<github-context>steering GitHub tool calls that omitowner/repoto the logical repo${{ github.repository }}expression is preserved and still flows through the expression extractor.Rendered output in trial mode:
Notes
The issue also suggested optionally overriding the GitHub MCP server's default owner/repo. This PR takes the prompt-level fix as the minimal change that eliminates the silent mismatch; the added directive covers the MCP concern without a broader config change. If reviewers prefer enforcing the default at the MCP server level as well, that can be layered on top.
--logical-repoleaves<github-context>.repositorypointing at the host repo #50580