Cache checkout repository visibility during compile#44224
Conversation
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>
|
✅ 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 #44224 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (96 additions detected). |
There was a problem hiding this comment.
Pull request overview
This PR improves compile-time performance and stability by caching GitHub repository visibility lookups (keyed by owner/repo) within a single Compiler instance, avoiding repeated gh api /repos/{slug} calls during a compile. As a consequence, regenerated workflow lockfiles now consistently embed sink-visibility in MCP write-sink guard policies when repository visibility is known at compile time.
Changes:
- Add a compiler-scoped
repositoryVisibilitycache and initialize it inNewCompiler. - Reuse cached results in
computeRepositoryVisibility()to avoid repeated GitHub API queries for the same slug. - Regenerate
.lock.ymlworkflows to includesink-visibility: "public"in derived MCP guard policies where applicable.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/push_to_pull_request_branch_validation.go | Adds lazy initialization + reuse of a per-compiler repository visibility cache during validation. |
| pkg/workflow/compiler_types.go | Adds the repositoryVisibility map field and initializes it in NewCompiler. |
| .github/workflows/workflow-normalizer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/workflow-health-manager.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/workflow-generator.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/weekly-safe-outputs-spec-review.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/weekly-issue-summary.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/weekly-blog-post-writer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/unbloat-docs.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/uk-ai-operational-resilience.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/ubuntu-image-analyzer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/tidy.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/terminal-stylist.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/technical-doc-writer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/sub-issue-closer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/step-name-alignment.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/stale-repo-identifier.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/stale-pr-cleanup.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/spec-librarian.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/spec-extractor.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/spec-enforcer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/smoke-update-cross-repo-pr.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/smoke-project.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/smoke-pi.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/smoke-otel-backends.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/smoke-opencode.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/smoke-gemini.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/smoke-crush.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/smoke-create-cross-repo-pr.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/smoke-copilot.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/smoke-copilot-arm.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/smoke-copilot-aoai-entra.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/smoke-copilot-aoai-apikey.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/smoke-codex.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/smoke-claude-on-copilot.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/smoke-ci.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/smoke-antigravity.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/smoke-agent-public-approved.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/smoke-agent-all-none.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/skillet.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/security-review.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/security-compliance.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/schema-feature-coverage.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/schema-consistency-checker.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/repository-quality-improver.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/repo-audit-analyzer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/refiner.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/refactoring-cadence.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/q.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/pr-triage-agent.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/pr-nitpick-reviewer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/pr-description-caveman.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/pr-code-quality-reviewer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/plan.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/outcome-collector.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/org-health-report.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/objective-impact-report.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/notion-issue-summary.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/necromancer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/metrics-collector.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/mergefest.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/mattpocock-skills-reviewer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/linter-miner.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/layout-spec-maintainer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/jsweep.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/issue-triage-agent.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/issue-arborist.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/instructions-janitor.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/impeccable-skills-reviewer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/hourly-ci-cleaner.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/grumpy-reviewer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/gpclean.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/go-logger.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/glossary-maintainer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/github-remote-mcp-auth-test.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/github-mcp-tools-report.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/github-mcp-structural-analysis.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/functional-pragmatist.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/firewall-escape.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/example-permissions-warning.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/eslint-refiner.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/eslint-miner.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/draft-pr-cleanup.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/discussion-task-miner.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/dictation-prompt.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/dev.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/dev-hawk.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/designer-drift-audit.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/design-decision-gate.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/deployment-incident-monitor.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/dependabot-repair.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/dependabot-go-checker.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/dependabot-burner.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/delight.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/dead-code-remover.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/dataflow-pr-discussion-dataset.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-workflow-updater.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-windows-terminal-integration-builder.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-testify-uber-super-expert.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-team-status.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-spdd-spec-planner.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-semgrep-scan.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-security-red-team.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-security-observability.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-secrets-analysis.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-safeoutputs-git-simulator.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-safe-outputs-conformance.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-safe-output-integrator.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-repo-chronicle.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-regulatory.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-performance-summary.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-observability-report.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-model-resolution.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-model-inventory.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-mcp-concurrency-analysis.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-malicious-code-scan.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-issues-report.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-geo-optimizer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-formal-spec-verifier.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-firewall-report.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-file-diet.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-fact.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-experiment-report.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-doc-updater.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-doc-healer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-compiler-threat-spec-optimizer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-compiler-quality.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-community-attribution.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-code-metrics.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-cli-performance.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-choice-test.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-cache-strategy-analyzer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-assign-issue-to-user.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-ambient-context-optimizer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/daily-agent-of-the-day-blog-writer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/craft.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/copilot-session-insights.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/copilot-pr-prompt-analysis.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/copilot-pr-nlp-analysis.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/copilot-opt.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/copilot-cli-deep-research.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/copilot-agent-analysis.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/contribution-check.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/commit-changes-analyzer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/codex-github-remote-mcp-test.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/code-simplifier.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/code-scanning-fixer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/ci-doctor.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/ci-coach.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/breaking-change-checker.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/bot-detection.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/avenger.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/artifacts-summary.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/archie.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/approach-validator.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/ai-moderator.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/agentic-token-optimizer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/agent-performance-analyzer.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
| .github/workflows/ab-testing-advisor.lock.yml | Regenerated lockfile to include sink-visibility in MCP write-sink policy JSON. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 197/197 changed files
- Comments generated: 1
- Review effort level: Low
| pushToPullRequestBranchValidationLog.Printf("Could not determine repository visibility: %v", err) | ||
| return "" | ||
| } | ||
| c.repositoryVisibility[slug] = visibility |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (3 tests)
Score calculation:
Verdict
Test highlights:
References: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
|
@copilot merge main and recompile |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Cache checkout repository visibility during compile
The change is correct and follows the existing ownerTypeCache pattern well. Key aspects reviewed:
Implementation (push_to_pull_request_branch_validation.go)
- Nil guard before map access is correct and consistent with the existing zero-value safety philosophy.
- Only successful lookups are cached; failures re-try on the next call — matching the stated intent and the behaviour described in the PR body.
- Log messages are clear and distinguish cache-hit from fresh-lookup paths.
Types (compiler_types.go)
- Field and
NewCompiler()initialisation mirror theownerTypeCachepattern exactly. One minor nit filed inline (missing goroutine-safety note on the field comment).
Tests (compiler_repository_visibility_cache_test.go)
- Three well-scoped tests cover cache hit (call-count = 1), no-caching on error (call-count = 2), and lazy init for a zero-value
Compiler{}. These directly exercise the invariants stated in the PR description. - The
assertiveTestErrorhelper is defined locally in this new file and does not conflict with any existing type in the package.
Lock files — bulk regeneration of .lock.yml files is expected and correct; skipped in review.
One non-blocking nit inline.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 68 AIC · ⌖ 5.84 AIC · ⊞ 4.8K
| ghesCompatFromCLI bool // If true, GHES compat was requested via --ghes CLI flag (takes precedence over aw.json) | ||
| ghesArtifactCompat bool // If true, GHES compatibility mode is enabled; artifact actions still use latest non-v3 pins | ||
| ownerTypeCache map[string]string // Cached GitHub owner type ("User"/"Organization"/"") keyed by owner login; not goroutine-safe (Compiler is used sequentially) | ||
| repositoryVisibility map[string]string // Cached repository visibility keyed by slug |
There was a problem hiding this comment.
Nit: the neighbouring ownerTypeCache field carries ; not goroutine-safe (Compiler is used sequentially) in its comment. Adding the same note here would keep the two fields consistent and make the invariant explicit for readers who land on this field first.
repositoryVisibility map[string]string // Cached repository visibility keyed by slug; not goroutine-safe (Compiler is used sequentially)@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — commenting with minor improvements on cache semantics documentation and test clarity.
📋 Key Themes & Highlights
Key Themes
- Cache semantics edge case: A successful but unparseable API response stores an empty string, which silently prevents future retries — different from the documented failure-retry behaviour. Worth a clarifying comment.
- Test mock coupling: The failure-test mock uses
calls == 1as a control variable, which couples control-flow to a side-effect counter. A boolean flag is clearer. - Missing edge-case coverage: No test exercises the empty/malformed slug early-return paths.
Positive Highlights
- ✅ Excellent simplification — replacing dual maps (
repositoryVisibility+repositoryVisibilitySet) with a single comma-ok lookup is idiomatic Go and clearly correct - ✅ The behaviour change (no longer caching failures) is well-motivated, well-documented in the PR body, and correctly implemented
- ✅ Test names are descriptive specifications:
CachesSuccessfulLookup,DoesNotCacheFailedLookup,InitializesCacheForZeroValueCompiler - ✅ Lazy cache initialisation guard at line 40–42 is clean and safe
- ✅ Lock-file regeneration diff is mechanical and consistent
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 90.9 AIC · ⌖ 5.5 AIC · ⊞ 6.5K
Comment /matt to run again
| pushToPullRequestBranchValidationLog.Printf("Could not determine repository visibility: %v", err) | ||
| return "" | ||
| } | ||
| c.repositoryVisibility[slug] = visibility |
There was a problem hiding this comment.
[/codebase-design] The cache stores the visibility value for any non-error result, including an empty string when parseRepositoryVisibility cannot parse the API response. This means a successful-but-inconclusive API call permanently silences future retries, contrasting with the new error-retry semantics. A short clarifying comment would make the "success = cache it" invariant explicit.
💡 Suggested comment
// Only successful (non-error) lookups are cached; errors are not stored so
// transient failures retry on the next call. Note: an empty-string result
// from a successful but unparseable response IS cached intentionally.
c.repositoryVisibility[slug] = visibility@copilot please address this.
|
|
||
| func TestComputeRepositoryVisibility_InitializesCacheForZeroValueCompiler(t *testing.T) { | ||
| c := &Compiler{} | ||
| c.SetRepositorySlug("github/gh-aw") |
There was a problem hiding this comment.
[/tdd] The test TestComputeRepositoryVisibility_DoesNotCacheFailedLookup verifies the retry-on-failure path, but the test mock hardcodes calls == 1 as the failure condition. A more resilient approach would make the first-call failure self-documenting and independent of call ordering.
💡 Suggested refactor for clarity
firstCall := true
fetchRepositoryVisibility = func(slug string) (string, error) {
calls++
if firstCall {
firstCall = false
return "", assertiveTestError("boom")
}
return "public", nil
}This makes the intent clear without relying on the mutable calls counter for control flow.
@copilot please address this.
|
|
||
| type assertiveTestError string | ||
|
|
||
| func (e assertiveTestError) Error() string { return string(e) } |
There was a problem hiding this comment.
[/tdd] There is no test for the case where repositorySlug is empty or malformed — the early-exit paths at lines 30–38 of computeRepositoryVisibility. Adding coverage ensures a future refactor of slug validation doesn't accidentally return cached data for an unset slug.
💡 Suggested missing test
func TestComputeRepositoryVisibility_EmptySlugReturnsEmpty(t *testing.T) {
c := NewCompiler()
// do NOT call SetRepositorySlug
got := c.computeRepositoryVisibility()
if got != "" {
t.Fatalf("computeRepositoryVisibility() = %q, want empty string for unset slug", got)
}
}@copilot please address this.
|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
REQUEST_CHANGES — One correctness bug must be fixed before merging; two medium-priority test issues warrant fixing too.
### Findings summary
🔴 Must fix
push_to_pull_request_branch_validation.go:54— Empty-string cached as a successful lookup: WhenparseRepositoryVisibilityreturns("", nil)(valid JSON, unrecognised fields), the cache stores""permanently. The PR's stated intent is "only caching successful lookups"; an empty-string result is not a successful lookup. Fix: only cache values in{"public", "private", "internal"}.
🟡 Should fix
compiler_repository_visibility_cache_test.go:34— Missing test for the("", nil)path: The test suite only coverserror != nil. The uncovered path directly corresponds to the above bug — adding the test first would have caught it.compiler_repository_visibility_cache_test.go:14— GlobalfetchRepositoryVisibilitymutated without race-safe serialisation: Other tests in the package also mutate this global; undergo test -racewith parallel tests this is a data race waiting to happen.
🔵 Non-blocking
compiler_types.go:110— Missing goroutine-safety disclaimer on the new field, inconsistent with theownerTypeCachesibling comment.compiler_repository_visibility_cache_test.go:78— Weak assertion in zero-value compiler test: verifies map non-nil but not that the correct key was stored.
🔎 Code quality review by PR Code Quality Reviewer · 179.9 AIC · ⌖ 7.38 AIC · ⊞ 5.4K
Comment /review to run again
| pushToPullRequestBranchValidationLog.Printf("Could not determine repository visibility: %v", err) | ||
| return "" | ||
| } | ||
| c.repositoryVisibility[slug] = visibility |
There was a problem hiding this comment.
Empty-string visibility is permanently cached when the API response is valid but unrecognised, freezing the compiler into an unknown-visibility state for the remainder of the run.
💡 Details and suggested fix
parseRepositoryVisibility returns "" when JSON unmarshalling succeeds but neither visibility nor private fields are recognizable. fetchRepositoryVisibility then returns ("", nil) — no error — and the current code caches the empty string:
c.repositoryVisibility[slug] = visibility // stores "" permanentlyEvery subsequent computeRepositoryVisibility() call for the same slug hits the cache and returns "" without retrying the API. The PR description says "preserve current failure semantics by only caching successful lookups" — but an empty-string result with err == nil is not a successful lookup.
Fix — only cache recognised values:
switch visibility {
case "public", "private", "internal":
c.repositoryVisibility[slug] = visibility
}This also aligns with the parallel ownerTypeCache pattern, which caches "User" / "Organization" but treats "" as a retriable unknown state.
| } | ||
| } | ||
|
|
||
| func TestComputeRepositoryVisibility_DoesNotCacheFailedLookup(t *testing.T) { |
There was a problem hiding this comment.
The "does not cache failed lookup" tests only cover error != nil; the silent ("", nil) path is untested and actually does get cached.
💡 Details and missing test case
There are two ways computeRepositoryVisibility can produce an empty result:
fetchRepositoryVisibilityreturns(_, err)whereerr != nil— covered by this test.fetchRepositoryVisibilityreturns("", nil)— whenparseRepositoryVisibilityreceives valid JSON but no recognisable visibility field — not covered.
For case 2, the current implementation at line 54 stores "" in the cache because the guard only checks err:
if err != nil {
return ""
}
c.repositoryVisibility[slug] = visibility // ← stores "" when visibility==""Add a test:
func TestComputeRepositoryVisibility_DoesNotCacheEmptyStringResult(t *testing.T) {
c := NewCompiler()
c.SetRepositorySlug("github/gh-aw")
original := fetchRepositoryVisibility
defer func() { fetchRepositoryVisibility = original }()
calls := 0
fetchRepositoryVisibility = func(slug string) (string, error) {
calls++
return "", nil // success but unrecognised visibility
}
if got := c.computeRepositoryVisibility(); got != "" {
t.Fatalf("got %q, want empty", got)
}
if got := c.computeRepositoryVisibility(); got != "" {
t.Fatalf("got %q, want empty", got)
}
if calls != 2 {
t.Fatalf("calls = %d, want 2 (empty string must not be cached)", calls)
}
}This test currently fails against the code as written, confirming the bug in the implementation.
| originalFetchRepositoryVisibility := fetchRepositoryVisibility | ||
| defer func() { fetchRepositoryVisibility = originalFetchRepositoryVisibility }() | ||
|
|
||
| calls := 0 |
There was a problem hiding this comment.
These tests mutate a package-level variable without t.Parallel() protection, creating a data race when other parallel tests in the same package touch fetchRepositoryVisibility.
💡 Details
All three new tests overwrite fetchRepositoryVisibility (a var at package scope) and rely only on defer for cleanup. They do not call t.Parallel(). However, the existing compiler_orchestrator_workflow_test.go:126 also mutates this same global and does run alongside other tests.
Under go test -race -count=1 ./pkg/workflow/..., if the test scheduler runs TestBuildInitialWorkflowData_SetsRepositoryVisibilityForGitHubTool (which also patches fetchRepositoryVisibility) concurrently with any of these three tests, both goroutines write the global var without synchronisation — a race.
The cleanest fix is to eliminate the global and instead make fetchRepositoryVisibility a field on Compiler (same approach as ownerTypeCache which uses a per-instance map). Alternatively, guard all mutations with a sync.Mutex or use t.Setenv-style serialisation. At minimum, add a comment warning about the global mutation pattern so future tests don't accidentally introduce parallel tests in the same package without awareness.
| ghesCompatFromCLI bool // If true, GHES compat was requested via --ghes CLI flag (takes precedence over aw.json) | ||
| ghesArtifactCompat bool // If true, GHES compatibility mode is enabled; artifact actions still use latest non-v3 pins | ||
| ownerTypeCache map[string]string // Cached GitHub owner type ("User"/"Organization"/"") keyed by owner login; not goroutine-safe (Compiler is used sequentially) | ||
| repositoryVisibility map[string]string // Cached repository visibility keyed by slug |
There was a problem hiding this comment.
Missing goroutine-safety disclaimer — inconsistent with the identical sibling field.
💡 Suggested comment
The ownerTypeCache field one line above has an explicit disclaimer:
ownerTypeCache map[string]string // Cached GitHub owner type ("User"/"Organization"/"") keyed by owner login; not goroutine-safe (Compiler is used sequentially)The new repositoryVisibility field omits it. Readers will assume a difference in thread-safety guarantees where there is none. Update to:
repositoryVisibility map[string]string // Cached repository visibility keyed by slug; not goroutine-safe (Compiler is used sequentially)| if got := c.computeRepositoryVisibility(); got != "public" { | ||
| t.Fatalf("computeRepositoryVisibility() = %q, want %q", got, "public") | ||
| } | ||
| if c.repositoryVisibility == nil { |
There was a problem hiding this comment.
Test only verifies the map is non-nil, not that the correct key was stored — a bug that caches under the wrong key would pass undetected.
💡 Suggested assertion
Add after the nil check:
if got := c.repositoryVisibility["github/gh-aw"]; got != "public" {
t.Fatalf("repositoryVisibility[%q] = %q, want %q", "github/gh-aw", got, "public")
}This ensures the cache was keyed on the slug that was actually looked up, not some other key (e.g., "", the repo name alone, etc.).
…t-repository-visibility Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot merge main, recompile |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — main is merged, all 258 workflows recompiled, and I also fixed the empty-visibility caching bug flagged in review: |
|
@copilot revert lock.yml changes |
…t-repository-visibility Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- compiler_types.go: add goroutine-safety note to repositoryVisibility field
- push_to_pull_request_branch_validation.go: use switch to only cache recognized values (public/private/internal) with clarifying comment
- compiler_repository_visibility_cache_test.go:
- refactor DoesNotCacheFailedLookup to use firstCall boolean
- update DoesNotCacheEmptyVisibility to always return ("",nil) per reviewer suggestion
- add key assertion in InitializesCacheForZeroValueCompiler
- add TestComputeRepositoryVisibility_EmptySlugReturnsEmpty
- add package-level comment warning about global var mutation and t.Parallel()
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit
Main merged and all 258 workflows recompiled. CI needs a human re-trigger to confirm.
Main merged and pr-finisher run complete. Local validation (fmt, tests, 258/258 recompile) all green. CI needs a human re-trigger before merge. |
🤖 PR Triage
Rationale: Cache compile-time visibility lookups — targeted perf improvement with tests, approved by CI bots. Small footprint (+784/-924 includes generated lock files). Ready for expedited human review. Run §28924016278
|
Compile-time repository visibility checks were re-querying GitHub for the same checkout target, which adds avoidable overhead across a single compile. This change keeps visibility resolution compiler-scoped and reuses successful results per repository slug.
What changed
owner/repocomputeRepositoryVisibility()instead of callinggh apirepeatedly for the same slugBehavior
Coverage