-
Notifications
You must be signed in to change notification settings - Fork 481
trial: render logical repo in github-context prompt #50640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
66fd99d
31ae566
a53b0d6
65361d3
5f5f6e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -146,6 +146,14 @@ func (c *Compiler) collectPromptSections(data *WorkflowData) []PromptSection { | |
| // The checkout list may contain ${{ github.repository }} which must go through | ||
| // the expression extractor so the placeholder substitution step can resolve it. | ||
| combinedPromptText := githubContextPromptText | ||
| // In trial mode with a logical target repository, the workflow runs on the host | ||
| // repo (github.repository) but checkout and safe outputs are redirected to the | ||
| // 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 != "" { | ||
| combinedPromptText = applyTrialLogicalRepoToGitHubContext(combinedPromptText, data.TrialLogicalRepo) | ||
| } | ||
| if checkoutsContent := buildCheckoutsPromptContent(data.CheckoutConfigs); checkoutsContent != "" { | ||
| unifiedPromptLog.Printf("Injecting checkout list into GitHub context (%d checkouts)", len(data.CheckoutConfigs)) | ||
| const closeTag = "</github-context>" | ||
|
|
@@ -780,3 +788,26 @@ func buildSafeOutputsSections(safeOutputs *SafeOutputsConfig) []PromptSection { | |
|
|
||
| return sections | ||
| } | ||
|
|
||
| // applyTrialLogicalRepoToGitHubContext rewrites the github-context prompt so that | ||
| // the reported repository is the trial mode logical target rather than the host | ||
| // repository (github.repository). It replaces the unconditional repository line and | ||
| // appends a directive instructing the agent to target the logical repository for | ||
| // GitHub MCP calls that omit an explicit owner/repo. | ||
| func applyTrialLogicalRepoToGitHubContext(promptText, logicalRepo string) string { | ||
| const repoLine = "- **repository**: ${{ github.repository }}" | ||
| replacement := "- **repository**: " + logicalRepo | ||
| promptText = strings.Replace(promptText, repoLine, replacement, 1) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnosing-bugs] Silent no-op if the repository line format ever drifts — 💡 SuggestionIf 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The 💡 Fix: detect and surface a failed replacement
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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If 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. |
||
|
|
||
| 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", | ||
|
|
||
| logicalRepo, | ||
| ) | ||
| const closeTag = "</github-context>" | ||
| if idx := strings.LastIndex(promptText, closeTag); idx >= 0 { | ||
| promptText = promptText[:idx] + directive + promptText[idx:] | ||
| } else { | ||
| promptText += directive | ||
| } | ||
| return promptText | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -718,3 +718,63 @@ func TestGenerateUnifiedPromptCreationStep_BlankRunCapAdjacentToRuntimeImport(t | |
| // 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") | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No test exercises the failure mode where 💡 Add a regression test for template driftBoth new tests only check the "happy path" where |
||
| func TestCollectPromptSections_TrialLogicalRepoGitHubContext(t *testing.T) { | ||
| compiler := &Compiler{} | ||
|
|
||
| data := &WorkflowData{ | ||
| ParsedTools: NewTools(map[string]any{ | ||
| "github": true, | ||
| }), | ||
| Permissions: "contents: read", | ||
| On: "issue_comment", | ||
| TrialMode: true, | ||
| TrialLogicalRepo: "owner/target", | ||
| } | ||
|
|
||
| sections := compiler.collectPromptSections(data) | ||
|
|
||
| var githubContext string | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The trial-mode test asserts the directive contains the string 💡 SuggestionAdd 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. |
||
| 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**: owner/target", | ||
| "github-context should report the logical target repository") | ||
| assert.NotContains(t, githubContext, "**repository**: ${{ github.repository }}", | ||
| "github-context should not report the host repository in trial mode") | ||
| assert.Contains(t, githubContext, "trial mode", | ||
| "github-context should include a trial mode directive") | ||
|
Comment on lines
+750
to
+751
|
||
| } | ||
|
|
||
| func TestCollectPromptSections_NoTrialLogicalRepoKeepsHostRepo(t *testing.T) { | ||
| compiler := &Compiler{} | ||
|
|
||
| data := &WorkflowData{ | ||
| ParsedTools: NewTools(map[string]any{ | ||
| "github": true, | ||
| }), | ||
| Permissions: "contents: read", | ||
| On: "issue_comment", | ||
| } | ||
|
|
||
| sections := compiler.collectPromptSections(data) | ||
|
|
||
| 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") | ||
|
Comment on lines
+767
to
+777
|
||
| assert.NotContains(t, githubContext, "owner/target", | ||
| "github-context should not include a logical target when trial mode is off") | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no test covering the case where trial-mode logical-repo injection and
buildCheckoutsPromptContentboth fire in the same run, even though both insert content at the same</github-context>anchor sequentially in this function.💡 Add an interaction test
applyTrialLogicalRepoToGitHubContextinserts its directive before</github-context>first, then (a few lines later) the checkout-list injection doesstrings.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 withTrialMode: true,TrialLogicalReposet, and non-emptyCheckoutConfigsto lock in the expected ordering.