Skip to content

Address sink-visibility runtime check review feedback#8907

Merged
lpcox merged 2 commits into
mainfrom
sink-visibility-review-fixes
Jul 8, 2026
Merged

Address sink-visibility runtime check review feedback#8907
lpcox merged 2 commits into
mainfrom
sink-visibility-review-fixes

Conversation

@lpcox

@lpcox lpcox commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

Addresses the three review feedback items from #8903 for the sink-visibility runtime verification.

Changes

1. Skip runtime check when sink-visibility is omitted

When sink-visibility is not explicitly configured (empty string), the runtime API check is now skipped entirely. This preserves the documented backward-compatible behavior where omitted sink-visibility uses accept patterns without any surprise upgrade to "public".

Before: Omitted config + public repo → runtime check overrides to "public" (breaking backward compat)
After: Omitted config → runtime check skipped, accept patterns used as-is

2. Remove duplicate logging from VerifySinkVisibility

The helper function no longer logs override warnings via logger.LogWarn. All logging is now handled by the caller (verifySinkVisibilityAtRuntime) which has the serverID context for proper correlation.

3. Return configured value when no override needed

VerifySinkVisibility now returns the configured value unchanged in all non-override cases, instead of returning the actual API visibility. This prevents policy drift where configured="private" but actual="internal" would have returned "internal".

Before: configured="private", actual="internal" → returns "internal"
After: configured="private", actual="internal" → returns "private" (unchanged)

The only case that changes the return value is the security-critical one: repo is actually public but config doesn't say "public".

Testing

All integration tests and unit tests pass, including the existing sink-visibility test suite.

Related

- Skip runtime check when sink-visibility is omitted (empty string),
  preserving backward-compatible behavior for legacy configs
- Remove duplicate logging from VerifySinkVisibility helper; let the
  caller (verifySinkVisibilityAtRuntime) handle all logging with
  serverID context
- Return configured value when no override is needed, instead of
  returning the actual API value which could drift from compiler config
  (e.g. configured='private' but actual='internal' no longer returns
  'internal')

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 8, 2026 02:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refines the sink-visibility runtime verification behavior to align with review feedback from #8903 and preserve backward compatibility when sink-visibility is omitted.

Changes:

  • Skip the runtime GitHub API visibility check entirely when sink-visibility is omitted (empty string).
  • Centralize override/pass logging in verifySinkVisibilityAtRuntime (removing duplicate logging from the helper).
  • Adjust VerifySinkVisibility to only override in the security-critical case (actual repo is public but config isn’t “public”), and otherwise keep the configured value.
Show a summary per file
File Description
internal/server/guard_init.go Skips runtime sink-visibility verification when sink-visibility is omitted; retains existing skip behavior for missing repo/token.
internal/githubhttp/visibility.go Narrows override behavior to the public-repo mismatch case and removes helper-side warning logging.
internal/githubhttp/visibility_test.go Updates unit expectation so configured “private” remains “private” when actual is “internal”.

Review details

Tip

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

  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Low

Comment thread internal/githubhttp/visibility.go Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants