Reduce CompileMCPWorkflow overhead by caching runtime requirement detection per compile#43833
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR reduces repeated work during compilation by caching detected runtime requirements on WorkflowData so both YAML generation and runtime package validation can reuse the same detection result within a single compile.
Changes:
- Added
WorkflowDatafields to cache runtime requirements and track whether the cache is populated. - Introduced
detectRuntimeRequirementsCached(...)(with slice cloning) and switched compile-path consumers to use it. - Added a unit test intended to verify copy-on-read behavior for the cached runtime requirements.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/workflow_data.go | Adds per-compilation cache fields for detected runtime requirements. |
| pkg/workflow/runtime_validation.go | Reuses cached runtime requirements during runtime package validation. |
| pkg/workflow/runtime_detection.go | Implements cached runtime requirement detection with cloning to reduce duplicate work. |
| pkg/workflow/runtime_detection_cache_test.go | Adds a test for defensive-copy semantics of cached runtime requirements. |
| pkg/workflow/compiler_yaml_main_job.go | Reuses cached runtime requirements during runtime setup/checkout preparation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Low
| func TestDetectRuntimeRequirementsCached_ReturnsDefensiveCopy(t *testing.T) { | ||
| workflowData := &WorkflowData{ | ||
| CustomSteps: "run: node --version", | ||
| } | ||
|
|
||
| first := detectRuntimeRequirementsCached(workflowData) | ||
| require.NotEmpty(t, first) | ||
| require.True(t, workflowData.CachedRuntimeRequirementsSet) | ||
|
|
||
| first[0].Version = "mutated-version" | ||
|
|
||
| second := detectRuntimeRequirementsCached(workflowData) | ||
| require.NotEmpty(t, second) | ||
| require.NotEqual(t, "mutated-version", second[0].Version) | ||
| } |
There was a problem hiding this comment.
Addressed in cf66986. The cache test now exercises a forced cache-hit path by pre-populating CachedRuntimeRequirements and verifying mutations to the returned slice do not affect the cached value or a subsequent read.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
|
|
✅ 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 (49 additions detected, threshold is 100). |
🧪 Test Quality Sentinel Report❌ Test Quality Score: 35/100 — Poor
📊 Metrics (1 test)
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on two correctness issues in the cloning logic and corresponding test gaps.
📋 Key Themes & Highlights
Key Issues
- Shallow clone bug:
cloneRuntimeRequirementsusescopy(), which is a shallow copy.RuntimeRequirement.ExtraFieldsis amap[string]any— a reference type — so clones share the same underlying map. A caller mutatingExtraFieldswill silently corrupt the cached state, defeating the stated "defensive copy" guarantee. - Asymmetric return on cache-miss: The first call (cache-miss path) returns the raw slice from
DetectRuntimeRequirementsdirectly, while subsequent calls (cache-hit path) return a clone. The defensive copy guarantee currently depends on call order, which is fragile. - Test specification gap: The existing test only mutates
Version(a value type), so it does not catch the shallow-clone bug onExtraFields. Tests fornilinput and zero-requirements input are also absent.
Positive Highlights
- ✅ Clean, minimal scope — only two call sites changed, no unrelated refactoring
- ✅ The
CachedRuntimeRequirementsSetboolean sentinel follows the same established pattern asCachedAllowedDomainsComputed, keepingWorkflowDataconsistent - ✅ Good PR description: the behavioral guardrail section clearly states the defensive-copy intent
- ✅ New test file is well-structured and the test name is descriptive
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 61.6 AIC · ⌖ 7.52 AIC · ⊞ 6.7K
Comment /matt to run again
| requirements := DetectRuntimeRequirements(workflowData) | ||
| workflowData.CachedRuntimeRequirements = cloneRuntimeRequirements(requirements) | ||
| workflowData.CachedRuntimeRequirementsSet = true | ||
| return requirements |
There was a problem hiding this comment.
[/codebase-design] On the cache-miss path, detectRuntimeRequirementsCached returns requirements directly (line 33) — the same slice returned by DetectRuntimeRequirements. The cached copy is protected, but the returned slice on the first call is unprotected: a caller who mutates the first return value is mutating live data (the returned slice is the original, not a clone). Consider returning cloneRuntimeRequirements(requirements) on this path too, for consistency with the cache-hit path.
💡 Suggested fix
requirements := DetectRuntimeRequirements(workflowData)
workflowData.CachedRuntimeRequirements = cloneRuntimeRequirements(requirements)
workflowData.CachedRuntimeRequirementsSet = true
return cloneRuntimeRequirements(requirements) // symmetric with cache-hit pathThis makes every return from detectRuntimeRequirementsCached an independent clone, so the invariant does not depend on call order.
@copilot please address this.
There was a problem hiding this comment.
Addressed in cf66986. The cache-miss path now returns a cloned slice from the cached snapshot, so both miss and hit paths have the same defensive-copy semantics.
There was a problem hiding this comment.
Review: Caching runtime requirement detection
The intent and overall structure are sound — caching DetectRuntimeRequirements per compile to avoid redundant work is a clear win, and the test coverage for the defensive-copy property is a good addition.
Two correctness issues in the clone implementation and one test-fixture concern need addressing before merge.
Blocking
-
Shallow clone misses reference fields (
cloneRuntimeRequirements, line 19 ofruntime_detection.go)
copy()on a[]RuntimeRequirementslice does a bitwise struct copy. TheExtraFields map[string]anyandRuntime *Runtimefields are shared between the cached snapshot and every returned copy. For the current call order this is incidentally safe (ExtraFieldsis nil at detection time), but the declared invariant — defensive copy — is not actually enforced for those fields. The fix is a per-element deep clone ofExtraFields. -
Cache-miss path returns the raw, uncloned slice (
detectRuntimeRequirementsCached, line 33 ofruntime_detection.go)
On the first call the function returnsrequirements(the value fromDetectRuntimeRequirements) directly, while subsequent calls return a clone of the cached copy. The two callers therefore receive objects with different isolation semantics on the first call vs. all later calls. This asymmetry is a latent bug — if the first caller mutates anExtraFieldsmap, the cached snapshot is already protected (cloned before storage), but the inconsistency makes reasoning about correctness harder and will bite ifExtraFieldsis ever non-nil at detection time.
Non-blocking
- Test fixture uses invalid-YAML CustomSteps (
runtime_detection_cache_test.go, line 62)
"run: node --version"is not valid step YAML (missingsteps:wrapper), so it passes through YAML parsing silently and relies on the raw-string command scanner indetectFromCustomSteps. The test works but is fragile. Prefer a well-formed fixture or an in-structParsedToolsinjection.
No issues found in remaining changed files
compiler_yaml_main_job.go: straightforward call-site swap, correct.runtime_validation.go: straightforward call-site swap, correct.workflow_data.go: field additions are well-commented and consistent with the existingCachedAllowedDomains*pattern.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 103.4 AIC · ⌖ 6.25 AIC · ⊞ 4.9K
Comments that could not be inline-anchored
pkg/workflow/runtime_detection.go:19
Shallow clone does not protect ExtraFields or Runtime pointer fields.
copy(cloned, requirements) copies struct values shallowly. RuntimeRequirement has two reference-typed fields:
ExtraFields map[string]any— shared map between the cached copy and any returned copyRuntime *Runtime— shared pointer
For the current call flow this is safe because ExtraFields is always nil at detection time (only populated later inside DeduplicateRuntimeSetupStepsFromCustomSteps). But t…
pkg/workflow/runtime_detection.go:33
Cache-read path returns a copy; cache-write path returns the raw slice.
On the first call (cache miss), line 33 returns the raw requirements slice directly — the same slice that DetectRuntimeRequirements returned, before it was cloned into CachedRuntimeRequirements. Any mutation of that returned slice by the caller does not affect the cache, which is correct, but the asymmetry means the first and subsequent callers get objects with different identity semantics.
More concretely: if t…
pkg/workflow/runtime_detection_cache_test.go:62
Test relies on a coincidental CustomSteps value to trigger node detection.
"run: node --version" causes node to be detected, but this custom-steps string is not valid GitHub Actions YAML (it lacks a steps: wrapper), so it exercises a code path that depends on a silent parse failure in detectFromCustomSteps. A cleaner, more robust fixture would use valid step YAML or go through the WorkflowData.ParsedTools field to inject a known runtime requirement:
workflowData := &Workflow…
</details>
🤖 PR Triage
Summary: Caches runtime-requirement detection per compilation to remove ~10%
|
|
@copilot please run the
|
…egression-fix-compile-mcp-workflow Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the runtime-cache review threads in cf66986 and updated the branch with origin/main in ffdd7b1. I reran local validation ( |
|
@copilot this PR is blocked and needs attention. Please run the
|
Current HEAD already contains the requested review fixes in |
|
@copilot please run the
|
Current HEAD |
|
@copilot please run the Unresolved review feedback:
|
Current HEAD |
|
🎉 This pull request is included in a new release. Release: |
BenchmarkCompileMCPWorkflowregressed by ~10% due to repeated work in the compile path. This change removes duplicate runtime-requirement detection within a single compilation while preserving behavior.What changed
WorkflowDatacache fields for runtime requirements:CachedRuntimeRequirementsCachedRuntimeRequirementsSetdetectRuntimeRequirementsCached(...)inpkg/workflow/runtime_detection.go.validateRuntimePackages(...)(pkg/workflow/runtime_validation.go)prepareRuntimeSetupAndCheckoutInfo(...)(pkg/workflow/compiler_yaml_main_job.go)Behavioral guardrail
Targeted correctness coverage
TestDetectRuntimeRequirementsCached_ReturnsDefensiveCopyto verify copy-on-read semantics.