Skip to content

Add top-level max-turn-cache-misses support with env-managed default#40388

Merged
pelikhan merged 11 commits into
mainfrom
copilot/add-max-cache-misses-frontmatter
Jun 20, 2026
Merged

Add top-level max-turn-cache-misses support with env-managed default#40388
pelikhan merged 11 commits into
mainfrom
copilot/add-max-cache-misses-frontmatter

Conversation

Copilot AI commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

This adds first-class workflow frontmatter support for the AWF cache-miss guardrail and aligns it with existing compiler-managed defaults. max-turn-cache-misses now resolves with the expected precedence: frontmatter value, env-managed default override, then built-in default 5.

  • Frontmatter and schema

    • Accept top-level max-turn-cache-misses
    • Document it as the source for AWF apiProxy.maxCacheMisses
    • Validate it as a positive integer; schema description reflects the full precedence chain
  • Compiler and AWF config mapping

    • Thread max-turn-cache-misses through EngineConfig (MaxTurnCacheMisses)
    • Emit apiProxy.maxCacheMisses in generated AWF config
    • Apply imported workflow values with the existing first-wins top-level guardrail merge behavior
  • Default resolution

    • Add built-in default 5 (DefaultMaxTurnCacheMisses)
    • Add GH_AW_DEFAULT_MAX_TURN_CACHE_MISSES as the compiler-managed override
    • Apply precedence as:
      1. workflow frontmatter
      2. GH_AW_DEFAULT_MAX_TURN_CACHE_MISSES
      3. built-in default 5
  • Environment manager and docs

    • Extend gh aw env defaults support with default_max_turn_cache_misses
    • Update enterprise/defaults docs to include the new variable and precedence chain
  • Tests and golden output

    • Add focused coverage for schema validation, import merging, compiler default resolution, and AWF config emission
    • Refresh wasm goldens to reflect the new default AWF config field

Example:

---
on: push
engine: copilot
max-turn-cache-misses: 8
---

Compiles to AWF config including:

{
  "apiProxy": {
    "maxCacheMisses": 8
  }
}


✨ PR Review Safe Output Test - Run 27855089943

Warning

Firewall blocked 6 domains

The following domains were blocked by the firewall during workflow execution:

  • accounts.google.com
  • android.clients.google.com
  • clients2.google.com
  • contentautofill.googleapis.com
  • safebrowsingohttpgateway.googleapis.com
  • www.google.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "accounts.google.com"
    - "android.clients.google.com"
    - "clients2.google.com"
    - "contentautofill.googleapis.com"
    - "safebrowsingohttpgateway.googleapis.com"
    - "www.google.com"

See Network Configuration for more information.

💥 [THE END] — Illustrated by Smoke Claude · 83.6 AIC · ⌖ 28.1 AIC · ⊞ 8.5K ·

@github-actions

Copy link
Copy Markdown
Contributor

Hey @Copilot 👋 — thanks for picking up the max-cache-misses frontmatter feature! The issue #40383 lays out a clear spec and this PR is well-scoped. A few things to address once the implementation lands:

  • No code changes yet — the PR currently has 0 lines changed (only the "Initial plan" commit). The implementation, schema updates, and compiler mapping still need to land.
  • Add tests — per the acceptance criteria in the issue, cover: (1) the frontmatter field is accepted by the schema, (2) the compiler emits apiProxy.maxCacheMisses from it, (3) the built-in default of 5 applies when unset, and (4) the env-var override works correctly.
  • Update the PR description — once implementation is underway, replace the placeholder body with a summary of what changed and why, so reviewers have context at a glance.

If you'd like a hand getting started, here's a ready-to-use prompt:

Implement the `max-cache-misses` top-level frontmatter field as described in issue #40383.

Steps:
1. Add `max-cache-misses` to the workflow frontmatter schema (follow the pattern used by existing top-level guardrails like `max-ai-credits` or `max-turns`).
2. In the compiler, map the frontmatter field to `apiProxy.maxCacheMisses` in the generated AWF config.
3. Apply a built-in default of `5` when the field is not set.
4. Add an environment-manager-controlled variable that can override that default (follow the existing env-manager pattern for similar guardrails).
5. Surface the new env variable through environment manager docs.
6. Add or update tests covering: schema acceptance, compiler emission, default fallback, and env-var override.
7. Update the PR description to reflect what was changed.

Generated by ✅ Contribution Check · 133.8 AIC · ⌖ 7.3 AIC · ⊞ 5.9K ·

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Add top-level max-cache-misses frontmatter support with env-managed default Add top-level max-cache-misses support with env-managed default Jun 19, 2026
Copilot AI requested a review from pelikhan June 19, 2026 21:26
@pelikhan pelikhan marked this pull request as ready for review June 19, 2026 21:29
Copilot AI review requested due to automatic review settings June 19, 2026 21:29

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 adds a new top-level workflow frontmatter guardrail, max-cache-misses, and threads it through compilation so generated AWF config includes apiProxy.maxCacheMisses, with defaults resolved using the documented precedence (frontmatter → GH_AW_DEFAULT_MAX_CACHE_MISSES → built-in 5).

Changes:

  • Adds schema + parsing + import-merge support for top-level max-cache-misses and carries it through EngineConfig.
  • Emits apiProxy.maxCacheMisses in AWF config JSON with an env-managed default override (GH_AW_DEFAULT_MAX_CACHE_MISSES) and built-in default (5).
  • Extends gh aw env defaults YAML support/docs/tests and refreshes wasm golden outputs accordingly.
Show a summary per file
File Description
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/with-imports.golden Updates golden output to include apiProxy.maxCacheMisses in emitted AWF config JSON.
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden Updates golden output to include apiProxy.maxCacheMisses in emitted AWF config JSON.
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden Updates golden output to include apiProxy.maxCacheMisses in emitted AWF config JSON.
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/basic-copilot.golden Updates golden output to include apiProxy.maxCacheMisses in emitted AWF config JSON.
pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden Updates golden output to include apiProxy.maxCacheMisses in emitted AWF config JSON.
pkg/workflow/testdata/TestWasmGolden_AllEngines/gemini.golden Updates golden output to include apiProxy.maxCacheMisses in emitted AWF config JSON.
pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden Updates golden output to include apiProxy.maxCacheMisses in emitted AWF config JSON.
pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden Updates golden output to include apiProxy.maxCacheMisses in emitted AWF config JSON.
pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden Updates golden output to include apiProxy.maxCacheMisses in emitted AWF config JSON.
pkg/workflow/frontmatter_types.go Adds typed frontmatter field for max-cache-misses.
pkg/workflow/engine.go Threads max-cache-misses into EngineConfig extraction and adds default-resolving accessor.
pkg/workflow/engine_config_test.go Adds coverage for top-level max-cache-misses extraction without an explicit engine.
pkg/workflow/engine_config_parser.go Adds parser helper for max-cache-misses.
pkg/workflow/compilerenv/manager.go Adds env var name + resolution helper for GH_AW_DEFAULT_MAX_CACHE_MISSES.
pkg/workflow/compilerenv/manager_test.go Adds tests for env-based default resolution of max cache misses.
pkg/workflow/compiler_orchestrator_engine.go Preserves/import-merges max-cache-misses using existing first-wins behavior.
pkg/workflow/compiler_orchestrator_engine_test.go Adds test ensuring imported top-level max-cache-misses is applied.
pkg/workflow/awf_config.go Emits apiProxy.maxCacheMisses with precedence-aware defaulting.
pkg/workflow/awf_config_test.go Adds assertions and focused precedence tests for maxCacheMisses emission and schema validation.
pkg/parser/schemas/main_workflow_schema.json Adds schema support for top-level max-cache-misses (integer, min 1, default 5).
pkg/parser/schema_test.go Adds schema validation tests for accepted/rejected max-cache-misses values.
pkg/parser/import_processor.go Extends ImportsResult to carry first-wins merged max-cache-misses.
pkg/parser/import_field_extractor.go Extracts first-wins max-cache-misses across imports.
pkg/parser/import_field_extractor_test.go Adds test coverage for first-wins merge behavior on max-cache-misses.
pkg/constants/constants.go Introduces built-in default constant DefaultMaxCacheMisses = 5.
pkg/cli/env_command.go Adds default_max_cache_misses to defaults YAML bindings + validation.
pkg/cli/env_command_test.go Adds coverage for YAML keys, validation, and update changes list including default_max_cache_misses.
docs/src/content/docs/reference/environment-variables.md Documents the new default_max_cache_misses defaults key alongside other gh aw env defaults.
docs/src/content/docs/reference/compiler-enterprise-environment-controls.md Documents GH_AW_DEFAULT_MAX_CACHE_MISSES and the precedence chain for max cache misses.
.github/aw/cli-commands.md Updates CLI docs to include default_max_cache_misses in the recognized defaults list and example.

Copilot's findings

Tip

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

  • Files reviewed: 30/30 changed files
  • Comments generated: 1

"type": "integer",
"minimum": 1,
"default": 5,
"description": "Maximum number of consecutive AWF cache misses allowed before the API proxy blocks further requests. Maps to `apiProxy.maxCacheMisses` and defaults to 5 when omitted."
@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Test Quality Sentinel completed test quality analysis.

@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

PR Code Quality Reviewer completed the code quality review.

@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Design Decision Gate 🏗️ completed the design decision gate check.

@github-actions

Copy link
Copy Markdown
Contributor

🏗️ Design Decision Gate — ADR Required

This PR makes significant changes to core business logic (262 new lines in pkg/) but does not have a linked Architecture Decision Record (ADR).

📄 Draft ADR committed: docs/adr/40388-max-cache-misses-frontmatter-default-precedence.md — review and complete it before merging.

🔒 This PR cannot merge until an ADR is linked in the PR body.

📋 What to do next
  1. Review the draft ADR committed to your branch — it was generated from the PR diff and description (the three-tier max-cache-misses resolution precedence).
  2. Complete the missing sections — add context the AI couldn't infer, refine the decision rationale, and confirm the alternatives reflect what you actually weighed.
  3. Commit the finalized ADR to docs/adr/ on your branch.
  4. Reference the ADR in this PR body by adding a line such as:

    ADR: ADR-40388: Resolve max-cache-misses via Frontmatter, Env Default, Then Built-In Constant

Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision.

❓ Why ADRs Matter

"AI made me procrastinate on key design decisions. Because refactoring was cheap, I could always say 'I'll deal with this later.' Deferring decisions corroded my ability to think clearly."

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 Reference

An ADR must contain these four sections to be considered complete:

  • Context — What is the problem? What forces are at play?
  • Decision — What did you decide? Why?
  • Alternatives Considered — What else could have been done?
  • Consequences — What are the trade-offs (positive and negative)?

All ADRs are stored in docs/adr/ as Markdown files numbered by PR number (e.g., 0042-use-postgresql.md for PR #42).

🔒 This gate is blocking: link the ADR in the PR body to unblock merge.

🏗️ ADR gate enforced by Design Decision Gate 🏗️ · 86.2 AIC · ⌖ 10.5 AIC · ⊞ 13.6K ·

@github-actions

Copy link
Copy Markdown
Contributor

🧪 Test Quality Sentinel Report

⚠️ Test Quality Score: 73/100 — Acceptable

Analyzed 16 test cases across 7 modified Go test files: 16 design (behavioral), 0 implementation, 0 guideline violations — with test inflation in 2 files and missing assertion messages in 2 new functions.

📊 Metrics & Test Classification (16 tests analyzed)
Metric Value
New/modified tests analyzed 16
✅ Design tests (behavioral contracts) 16 (100%)
⚠️ Implementation tests (low value) 0 (0%)
Tests with error/edge cases 7 (44%)
Duplicate test clusters 0
Test inflation detected Yes (awf_config_test.go 8.7:1, schema_test.go 4.7:1)
🚨 Coding-guideline violations 0 (Go mock libraries / missing build tags / no assertion messages)
Test File Classification Issues Detected
TestBuildAWFConfigJSON (new assertion) pkg/workflow/awf_config_test.go ✅ Design
TestBuildAWFConfigJSON/default max-cache-misses uses built-in default pkg/workflow/awf_config_test.go ✅ Design
TestBuildAWFConfigJSON/enterprise default max-cache-misses env var overrides pkg/workflow/awf_config_test.go ✅ Design + edge case
TestBuildAWFConfigJSON/frontmatter max-cache-misses takes precedence over env default pkg/workflow/awf_config_test.go ✅ Design + edge case
TestSetupEngineAndImports_ImportedTopLevelMaxCacheMisses pkg/workflow/compiler_orchestrator_engine_test.go:243 ✅ Design Missing assertion messages
TestResolveDefaultMaxCacheMisses/unset uses fallback pkg/workflow/compilerenv/manager_test.go:87 ✅ Design + edge case Missing assertion messages
TestResolveDefaultMaxCacheMisses/invalid uses fallback pkg/workflow/compilerenv/manager_test.go:92 ✅ Design + edge case Missing assertion messages
TestResolveDefaultMaxCacheMisses/zero uses fallback pkg/workflow/compilerenv/manager_test.go:97 ✅ Design + edge case Missing assertion messages
TestResolveDefaultMaxCacheMisses/valid value overrides fallback pkg/workflow/compilerenv/manager_test.go:102 ✅ Design Missing assertion messages
TestExtractEngineConfig/top-level max-cache-misses without engine (table row) pkg/workflow/engine_config_test.go:80 ✅ Design
TestDefaultsFileYAMLKeys (new assertions) pkg/cli/env_command_test.go:87 ✅ Design
TestDefaultsValidateFile/accepts valid values (new data) pkg/cli/env_command_test.go:148 ✅ Design
TestDefaultsValidateFile/rejects invalid (new assertions) pkg/cli/env_command_test.go:172 ✅ Design + error case
TestDefaultsBuildUpdateChanges (new field) pkg/cli/env_command_test.go:217 ✅ Design
TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_MaxCacheMissesPositiveAccepted pkg/parser/schema_test.go:638 ✅ Design
TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_MaxCacheMissesZeroRejected pkg/parser/schema_test.go:652 ✅ Design + error case

Go: 16 (*_test.go); JavaScript: 0. Other languages: none detected.

⚠️ Non-Blocking Observations (3)

TestSetupEngineAndImports_ImportedTopLevelMaxCacheMisses (pkg/workflow/compiler_orchestrator_engine_test.go:~277) — ⚠️ Missing assertion messages: assert.Equal(t, "copilot", result.engineSetting) and assert.Equal(t, 7, result.engineConfig.MaxCacheMisses) lack descriptive context strings. Suggested fix: assert.Equal(t, "copilot", result.engineSetting, "imported workflow should resolve copilot engine").

TestResolveDefaultMaxCacheMisses (pkg/workflow/compilerenv/manager_test.go:~87) — ⚠️ All 4 subtests use bare assert.Equal(t, N, ResolveDefaultMaxCacheMisses(M)) with no message argument. Suggested fix: add a descriptive label per subtest, e.g. assert.Equal(t, 5, ResolveDefaultMaxCacheMisses(5), "empty env should fall back to built-in default").

pkg/workflow/compilerenv/manager_test.go missing //go:build !integration — Pre-existing issue (file was MODIFIED, not newly added by this PR). Still worth a follow-up to align with the project convention.

Test inflation noteawf_config_test.go adds 61 lines against 7 production lines (8.7:1). This is contextually reasonable: the tests cover three distinct priority-layer scenarios (built-in default → env override → frontmatter override) that each require a full AWFCommandConfig struct to be assembled. Not a quality concern — only an inflation metric that triggers the scoring deduction.

Verdict

Check passed. 0% implementation tests (threshold: 30%). All 16 new test cases verify observable behavioral contracts — JSON output content, resolution priority rules, fallback semantics, schema acceptance/rejection, and import propagation. Missing assertion messages in 2 new functions are a minor style concern and do not trigger a failure.

Scoring breakdown: behavioral (40/40) + edge cases (13/30) + no duplication (20/20) + inflation deduction (0/10) = 73/100

🧪 Test quality analysis by Test Quality Sentinel · 61.6 AIC · ⌖ 10.1 AIC · ⊞ 8.3K ·

@github-actions github-actions Bot 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.

✅ Test Quality Sentinel: 73/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). All 16 new test cases verify observable behavioral contracts covering default resolution, env-var override, frontmatter precedence, import propagation, schema validation, and CLI key/validation coverage for the new max-cache-misses field.

@github-actions github-actions Bot 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.

Skills-Based Review 🧠

Applied /zoom-out, /improve-codebase-architecture, /grill-with-docs, and /tdd — no blocking issues, but several maintainability and test-coverage improvements suggested.

📋 Key Themes & Highlights

Key Themes

  • Maintainability: extractEngineBudgetLimits now returns a 4-tuple of positional ints that will grow with each new guardrail — a small struct would future-proof this seam.
  • Redundant resolution: BuildAWFConfigJSON pre-resolves the env default then immediately overwrites it via GetMaxCacheMisses(); the nil-safe accessor already handles the fallback.
  • Intentional asymmetry undocumented: FrontmatterConfig.MaxCacheMisses uses *int32 (not *TemplatableInt32) and GetMaxCacheMisses() embeds env resolution (unlike GetMaxRuns()) — both correct but uncommented.
  • Test coverage gaps: negative schema boundary (-1) and the WorkflowData == nil path in BuildAWFConfigJSON are not exercised.

Positive Highlights

  • ✅ Clean full vertical slice: schema → parser → import accumulator → engine config → AWF JSON, all consistent
  • ✅ Three-tier precedence (frontmatter > env var > built-in default) is clearly documented and all three tiers have explicit tests
  • ✅ 9 wasm golden files updated correctly — confirms the default is unconditionally emitted
  • ✅ Consistent use of first-wins import merge pattern, matching all other guardrail fields
  • compilerenv.ResolveDefaultMaxCacheMisses follows the established ResolveDefaultTimeoutMinutes pattern exactly

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 123 AIC · ⌖ 7.78 AIC · ⊞ 6.9K

}

func extractEngineBudgetLimits(engineConfig *EngineConfig) (string, int64, int) {
func extractEngineBudgetLimits(engineConfig *EngineConfig) (string, int64, int, int) {

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.

[/zoom-out] extractEngineBudgetLimits now returns a 4-tuple (string, int64, int, int) — the last two ints are indistinguishable at the call site. If another guardrail is added, this signature grows again.

💡 Suggested refactor

A small struct makes the return self-documenting and call sites future-proof:

type preservedBudgetLimits struct {
    MaxTurns       string
    MaxAICredits   int64
    MaxRuns        int
    MaxCacheMisses int
}

func extractEngineBudgetLimits(engineConfig *EngineConfig) preservedBudgetLimits {
    if engineConfig == nil {
        return preservedBudgetLimits{}
    }
    return preservedBudgetLimits{
        MaxTurns:       engineConfig.MaxTurns,
        MaxAICredits:   engineConfig.MaxAICredits,
        MaxRuns:        engineConfig.MaxRuns,
        MaxCacheMisses: engineConfig.MaxCacheMisses,
    }
}

This avoids the positional ambiguity between the two trailing ints and makes adding future fields a one-liner.

Comment thread pkg/workflow/awf_config.go Outdated
// BuildAWFCommand (see injectMaxAICreditsExpression in awf_helpers.go).
maxAICredits := int64(0)
maxRuns := constants.DefaultMaxRuns
maxCacheMisses := compilerenv.ResolveDefaultMaxCacheMisses(constants.DefaultMaxCacheMisses)

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.

[/improve-codebase-architecture] ResolveDefaultMaxCacheMisses is called unconditionally here, but its result is immediately overwritten by GetMaxCacheMisses() on line 367 whenever EngineConfig != nil. Since GetMaxCacheMisses() already handles a nil receiver, the pre-call is redundant.

💡 Simplification

Replace the pre-call + conditional block with a single nil-safe call:

var ec *EngineConfig
if config.WorkflowData != nil {
    ec = config.WorkflowData.EngineConfig
}
maxCacheMisses := ec.GetMaxCacheMisses()  // handles nil, env override, and built-in default

This matches the single-source-of-truth intent of GetMaxCacheMisses() and removes the double ResolveDefaultMaxCacheMisses call that happens today when EngineConfig != nil && MaxCacheMisses == 0.

Comment thread pkg/workflow/engine.go Outdated

// GetMaxCacheMisses returns the configured AWF max-cache-misses value, falling back
// to the enterprise override or built-in default.
func (e *EngineConfig) GetMaxCacheMisses() int {

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.

[/improve-codebase-architecture] GetMaxCacheMisses() embeds env-var resolution (ResolveDefaultMaxCacheMisses), but the parallel GetMaxRuns() falls back only to a constant. This asymmetry is intentional but means env-override logic is split: partly in GetMaxCacheMisses(), partly in BuildAWFConfigJSON. A brief comment would make the design choice explicit for future maintainers.

💡 Suggestion

Add a comment explaining why the fallback strategy differs from GetMaxRuns():

// GetMaxCacheMisses returns the configured AWF max-cache-misses value, falling back
// to the enterprise override or built-in default.
// Unlike GetMaxRuns, which falls back to a compile-time constant, this method
// consults GH_AW_DEFAULT_MAX_CACHE_MISSES at call time so enterprise admins can
// override the default without recompiling.
func (e *EngineConfig) GetMaxCacheMisses() int {

Also consider adding an explicit test for the nil-receiver path (e.g., (*EngineConfig)(nil).GetMaxCacheMisses() returns the env/default value), since that branch is currently only exercised implicitly via golden tests.

Comment thread pkg/workflow/frontmatter_types.go Outdated
Version string `json:"version,omitempty"`
TimeoutMinutes *TemplatableInt32 `json:"timeout-minutes,omitempty"`
MaxAICredits *TemplatableInt32 `json:"max-ai-credits,omitempty"`
MaxCacheMisses *int32 `json:"max-cache-misses,omitempty"`

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.

[/grill-with-docs] MaxCacheMisses uses *int32 while every neighbouring field (MaxAICredits, TimeoutMinutes, MaxDailyAICredits, MaxToolDenials) uses *TemplatableInt32. The departure is intentional — max-cache-misses is compile-time only and deliberately doesn't support GitHub Actions expressions — but the asymmetry will confuse the next maintainer who adds a field here.

💡 Suggestion

Add a brief comment to make the intent obvious:

// MaxCacheMisses uses plain *int32 (not *TemplatableInt32) because the
// field is resolved at compile time and does not support GitHub Actions expressions.
MaxCacheMisses *int32 `json:"max-cache-misses,omitempty"`

Comment thread pkg/parser/schema_test.go Outdated
}
}

func TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_MaxCacheMissesZeroRejected(t *testing.T) {

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.

[/tdd] Tests cover max-cache-misses: 0 (rejected) and max-cache-misses: 5 (accepted), but not negative values. The schema defines minimum: 1, which should also reject -1, and that boundary isn't currently verified.

💡 Suggested test
func TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_MaxCacheMissesNegativeRejected(t *testing.T) {
    t.Parallel()
    err := ValidateMainWorkflowFrontmatterWithSchemaAndLocation(map[string]any{
        "on":               "push",
        "max-cache-misses": -1,
    }, "/tmp/gh-aw/max-cache-misses-negative-test.md")
    if err == nil {
        t.Fatal("expected max-cache-misses=-1 to fail schema validation")
    }
}

Completing the three-point boundary coverage (negative / zero / positive) is a spec-readable guarantee that minimum: 1 is wired correctly.

Comment thread pkg/workflow/awf_config_test.go Outdated
assert.Contains(t, jsonStr, `"maxCacheMisses":5`, "apiProxy should emit built-in maxCacheMisses default when unset")
})

t.Run("enterprise default max-cache-misses env var overrides built-in default", func(t *testing.T) {

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.

[/tdd] All three new max-cache-misses tests in BuildAWFConfigJSON use WorkflowData with a non-nil EngineConfig. The initial compilerenv.ResolveDefaultMaxCacheMisses call in BuildAWFConfigJSON is only the effective value when WorkflowData == nil or EngineConfig == nil — those paths aren't exercised for cache misses.

💡 Suggested test
t.Run("env default max-cache-misses applied when WorkflowData is nil", func(t *testing.T) {
    t.Setenv(compilerenv.DefaultMaxCacheMisses, "11")
    config := AWFCommandConfig{
        EngineName:     "copilot",
        AllowedDomains: "github.com",
        WorkflowData:   nil,
    }
    jsonStr, err := BuildAWFConfigJSON(config)
    require.NoError(t, err)
    assert.Contains(t, jsonStr, `"maxCacheMisses":11`, "should fall back to env default when WorkflowData is nil")
})

This pins the WorkflowData == nil branch and verifies the pre-call at line 361 actually does useful work.

@github-actions github-actions Bot 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.

REQUEST_CHANGES — 3 issues need fixing before merge

The implementation logic is correct and the precedence chain (frontmatter → env var → built-in default) is properly wired. Three issues require attention before this is safe to merge.

### Blocking issues

1. Environment-sensitive tests (awf_config_test.go lines 52 and 165) — HIGH
Two subtests assert "maxCacheMisses":5 (the built-in default) without first calling t.Setenv(compilerenv.DefaultMaxCacheMisses, "") to clear any ambient env override. If GH_AW_DEFAULT_MAX_CACHE_MISSES is set in the CI runner, these tests emit a wrong value and fail with a confusing message. The sibling "env var overrides" test correctly sets the env var; the "built-in default" test must symmetrically clear it.

2. Redundant os.Getenv call in BuildAWFConfigJSON (awf_config.go line 361) — MEDIUM
maxCacheMisses is pre-initialized via ResolveDefaultMaxCacheMisses (an os.Getenv call), then immediately overwritten by GetMaxCacheMisses() when EngineConfig != nil — which itself calls ResolveDefaultMaxCacheMisses again when the field is unset. The pre-init value is dead in every non-nil EngineConfig branch. maxRuns uses constants.DefaultMaxRuns (no syscall) for its pre-init and defers env-lookup to GetMaxRuns(); maxCacheMisses should follow the same pattern.

3. Missing test: frontmatter beats import (compiler_orchestrator_engine_test.go line 243) — MEDIUM
The only end-to-end test for max-cache-misses import merging covers the "import provides value, main has none" path. The inverse — main declares max-cache-misses: 3, import declares max-cache-misses: 7, result must be 3 — is untested through applyEngineImportDefaults. A future refactor of preservedMaxCacheMisses could silently break the documented "frontmatter wins" behavior.

🔎 Code quality review by PR Code Quality Reviewer · 158.8 AIC · ⌖ 7.47 AIC · ⊞ 5.1K

Comment thread pkg/workflow/awf_config_test.go Outdated
assert.Contains(t, jsonStr, `"maxAiCredits":333`, "apiProxy should bake in frontmatter maxAiCredits (skipping runtime expression)")
})

t.Run("default max-cache-misses uses built-in default when unset", func(t *testing.T) {

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.

Environment-sensitive test will produce a false failure when GH_AW_DEFAULT_MAX_CACHE_MISSES is set in the runner: this subtest is named "default max-cache-misses uses built-in default when unset", but it never calls t.Setenv(compilerenv.DefaultMaxCacheMisses, "") to actually enforce the "unset" condition. If GH_AW_DEFAULT_MAX_CACHE_MISSES is present in the CI environment, ResolveDefaultMaxCacheMisses returns that value, BuildAWFConfigJSON emits it, and the assertion at line 181 ("maxCacheMisses":5) fails with a misleading error. The base fixture assertion at line 52 (fmt.Sprintf("maxCacheMisses:%d", constants.DefaultMaxCacheMisses)) has the same gap.

💡 Suggested fix

Add at the top of this subtest (and at the top of the base fixture subtest):

t.Setenv(compilerenv.DefaultMaxCacheMisses, "")  // guarantee the built-in default path

The sibling test "enterprise default max-cache-misses env var overrides built-in default" already calls t.Setenv(compilerenv.DefaultMaxCacheMisses, "9") correctly. The "unset" case needs the symmetric clearing call to actually test what its name claims.

Comment thread pkg/workflow/awf_config.go Outdated
// BuildAWFCommand (see injectMaxAICreditsExpression in awf_helpers.go).
maxAICredits := int64(0)
maxRuns := constants.DefaultMaxRuns
maxCacheMisses := compilerenv.ResolveDefaultMaxCacheMisses(constants.DefaultMaxCacheMisses)

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.

Redundant os.Getenv call: the pre-init at this line is overwritten immediately when EngineConfig != nil.

maxCacheMisses is assigned via ResolveDefaultMaxCacheMisses (which calls os.Getenv), then unconditionally overwritten by GetMaxCacheMisses() at line 367 — which also calls ResolveDefaultMaxCacheMisses internally when MaxCacheMisses <= 0. In the common non-nil EngineConfig / unset-field path, the env-var is read twice and the first result is thrown away.

Compare maxRuns: its pre-init is constants.DefaultMaxRuns (no syscall), deferring env-lookup entirely to GetMaxRuns(). maxCacheMisses should follow the same pattern.

💡 Suggested fix
// Before:
maxRuns := constants.DefaultMaxRuns
maxCacheMisses := compilerenv.ResolveDefaultMaxCacheMisses(constants.DefaultMaxCacheMisses)
if config.WorkflowData != nil && config.WorkflowData.EngineConfig != nil {
    maxRuns = config.WorkflowData.EngineConfig.GetMaxRuns()
    maxCacheMisses = config.WorkflowData.EngineConfig.GetMaxCacheMisses()
}

// After — GetMaxCacheMisses already handles the nil-receiver and fallback:
maxRuns := constants.DefaultMaxRuns
maxCacheMisses := 0
if config.WorkflowData != nil && config.WorkflowData.EngineConfig != nil {
    maxRuns = config.WorkflowData.EngineConfig.GetMaxRuns()
    maxCacheMisses = config.WorkflowData.EngineConfig.GetMaxCacheMisses()
} else {
    maxCacheMisses = compilerenv.ResolveDefaultMaxCacheMisses(constants.DefaultMaxCacheMisses)
}

Alternatively, since GetMaxCacheMisses() already handles e == nil, you can drop the guard entirely:

maxCacheMisses := (*EngineConfig)(nil).GetMaxCacheMisses()  // resolves env/built-in default via nil-safe method
if config.WorkflowData != nil && config.WorkflowData.EngineConfig != nil {
    maxCacheMisses = config.WorkflowData.EngineConfig.GetMaxCacheMisses()
}

assert.Equal(t, "9", result.engineConfig.MaxToolDenials)
}

func TestSetupEngineAndImports_ImportedTopLevelMaxCacheMisses(t *testing.T) {

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.

Missing test: main-workflow max-cache-misses taking precedence over the same field in an imported workflow.

TestSetupEngineAndImports_ImportedTopLevelMaxCacheMisses only exercises the "import provides the value, main workflow has none" path. The inverse — main workflow declares max-cache-misses: 3, import declares max-cache-misses: 7, result must be 3 — is untested through the full applyEngineImportDefaults flow. Without this, a future refactor of preservedMaxCacheMisses handling could silently break the documented "frontmatter wins" precedence and no test would catch it.

💡 Suggested fix — add a second table entry
func TestSetupEngineAndImports_MainMaxCacheMissesTakesPrecedenceOverImport(t *testing.T) {
    tmpDir := testutil.TempDir(t, "engine-main-cache-misses-wins")

    sharedContent := `---
engine: copilot
max-cache-misses: 7
---
# Shared`
    // ... write shared file ...

    testContent := `---
on: push
max-cache-misses: 3
imports:
  - shared/common.md
---
# Test`
    // ... compile and assert result.engineConfig.MaxCacheMisses == 3 ...
}

@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot Rename “Max Cash Misses” to “Max Turn Cash Misses.”

@github-actions

Copy link
Copy Markdown
Contributor

@copilot review all comments and address unresolved review feedback.

Generated by 👨‍🍳 PR Sous Chef · 41.5 AIC · ⌖ 1.12 AIC · ⊞ 17.2K ·

@github-actions

Copy link
Copy Markdown
Contributor

Please refresh the branch and rerun checks after resolving the review threads.

Generated by 👨‍🍳 PR Sous Chef · 41.5 AIC · ⌖ 1.12 AIC · ⊞ 17.2K ·

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
@github-actions

github-actions Bot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟

@github-actions

github-actions Bot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

🚀 Smoke Pi MISSION COMPLETE! Pi delivered. 🥧

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

The threat detection results could not be parsed.

Review the workflow run logs for details.

@github-actions

github-actions Bot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

🎬 THE ENDSmoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨

@github-actions

github-actions Bot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

🚀 Smoke Antigravity MISSION COMPLETE! Antigravity has spoken. ✨

@github-actions

github-actions Bot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

✅ All tools validated successfully! Agent Container Smoke Test confirms agent container is ready.

@github-actions

Copy link
Copy Markdown
Contributor

📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing...

@github-actions

Copy link
Copy Markdown
Contributor

📰 BREAKING: Smoke Copilot - AOAI (apikey) is now investigating this pull request. Sources say the story is developing...

@github-actions

Copy link
Copy Markdown
Contributor

📰 BREAKING: Smoke Copilot - AOAI (Entra) is now investigating this pull request. Sources say the story is developing...

@github-actions

github-actions Bot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

🚀 Smoke Gemini MISSION COMPLETE! Gemini has spoken. ✨

@github-actions

Copy link
Copy Markdown
Contributor

Agent Container Tool Check

Tool Status Version
bash 5.2.21
sh available
git 2.54.0
jq 1.7
yq 4.53.3
curl 8.5.0
gh 2.94.0
node 22.22.3
python3 3.11.15 (PyPy)
go 1.24.13
java 21.0.11
dotnet 10.0.301

Result: 12/12 tools available ✅ — PASS

🔧 Tool validation by Agent Container Smoke Test · 17.3 AIC · ⌖ 6.61 AIC · ⊞ 5.4K ·

@github-actions

Copy link
Copy Markdown
Contributor

Commit pushed: d515fec

Generated by Changeset Generator · 5.31 AIC · ⊞ 15.4K

@github-actions

Copy link
Copy Markdown
Contributor

Smoke Test Results

  • GitHub MCP Testing: ✅
  • Web Fetch Testing: ✅
  • File Writing Testing: ✅
  • Bash Tool Testing: ✅
  • Build gh-aw: ❌

Overall status: FAIL

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • localhost

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "localhost"

See Network Configuration for more information.

Smoke Gemini — Powered by Gemini ·

@github-actions

Copy link
Copy Markdown
Contributor

Smoke Test: Codex - 27855089944
#40402 [docs] Self-healing documentation fixes from issue analysis
#40394 feat(codemods): rename allow-team-membersallowed-collaborators in safe-outputs.mentions
#1 GitHub reads | ✅ #2 Serena | ✅ #3 Playwright
#4 Web fetch | ✅ #5 File write | ✅ #6 Bash verify
#7 Build | ✅ #8 Comment memory | ✅ #9 Cache memory | ❌ #10 Set issue field
Overall: FAIL

Warning

Firewall blocked 6 domains

The following domains were blocked by the firewall during workflow execution:

  • accounts.google.com
  • android.clients.google.com
  • clients2.google.com
  • contentautofill.googleapis.com
  • safebrowsingohttpgateway.googleapis.com
  • www.google.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "accounts.google.com"
    - "android.clients.google.com"
    - "clients2.google.com"
    - "contentautofill.googleapis.com"
    - "safebrowsingohttpgateway.googleapis.com"
    - "www.google.com"

See Network Configuration for more information.

🔮 The oracle has spoken through Smoke Codex · 7.21 AIC · ⊞ 13.6K ·

@github-actions

Copy link
Copy Markdown
Contributor

Comment Memory

Console dawns quietly
Temp caches bloom, then settle still
Git hooks hum at dusk

Note

This comment is managed by comment memory.

It stores persistent context for this thread in the code block at the top of this comment.
Edit only the text inside the backtick fences; workflow metadata and the footer are regenerated automatically.

Learn more about comment memory

Warning

Firewall blocked 6 domains

The following domains were blocked by the firewall during workflow execution:

  • accounts.google.com
  • android.clients.google.com
  • clients2.google.com
  • contentautofill.googleapis.com
  • safebrowsingohttpgateway.googleapis.com
  • www.google.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "accounts.google.com"
    - "android.clients.google.com"
    - "clients2.google.com"
    - "contentautofill.googleapis.com"
    - "safebrowsingohttpgateway.googleapis.com"
    - "www.google.com"

See Network Configuration for more information.

🔮 The oracle has spoken through Smoke Codex · 7.21 AIC · ⊞ 13.6K ·

@github-actions

Copy link
Copy Markdown
Contributor

💥 Smoke Test: Claude — Run 27855089943

Core #1-12: ✅ all passed (MCP, GH CLI, build, Playwright, Tavily, file/bash, discussion #335, AW status, Slack, code-scan alert, check run)
PR review #13-19: ✅ update, ✅ review comments, ✅ submit review, ⚠️ resolve thread (none unresolved), ✅ add reviewer, ✅ push branch, ⚠️ close PR (no safe PR)

Overall: PASS

Warning

Firewall blocked 6 domains

The following domains were blocked by the firewall during workflow execution:

  • accounts.google.com
  • android.clients.google.com
  • clients2.google.com
  • contentautofill.googleapis.com
  • safebrowsingohttpgateway.googleapis.com
  • www.google.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "accounts.google.com"
    - "android.clients.google.com"
    - "clients2.google.com"
    - "contentautofill.googleapis.com"
    - "safebrowsingohttpgateway.googleapis.com"
    - "www.google.com"

See Network Configuration for more information.

💥 [THE END] — Illustrated by Smoke Claude · 83.6 AIC · ⌖ 28.1 AIC · ⊞ 8.5K ·

@github-actions github-actions Bot 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.

💥 Automated smoke test review - all systems nominal!

Warning

Firewall blocked 6 domains

The following domains were blocked by the firewall during workflow execution:

  • accounts.google.com
  • android.clients.google.com
  • clients2.google.com
  • contentautofill.googleapis.com
  • safebrowsingohttpgateway.googleapis.com
  • www.google.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "accounts.google.com"
    - "android.clients.google.com"
    - "clients2.google.com"
    - "contentautofill.googleapis.com"
    - "safebrowsingohttpgateway.googleapis.com"
    - "www.google.com"

See Network Configuration for more information.

💥 [THE END] — Illustrated by Smoke Claude · 83.6 AIC · ⌖ 28.1 AIC · ⊞ 8.5K


```yaml
default_max_ai_credits: "1000"
default_max_turn_cache_misses: "5"

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.

Nice — documenting the new default_max_turn_cache_misses env default here keeps the recognized-keys list discoverable. 👍

```
Recognized keys include `default_max_ai_credits`, `default_detection_max_ai_credits`, `default_max_daily_ai_credits`, `default_timeout_minutes`, `default_max_turns`, `default_detection_model`, `default_utc`, `default_model_copilot`, `default_model_claude`, `default_model_codex`. The compiler resolves model selection as `GH_AW_MODEL_*` → `GH_AW_DEFAULT_MODEL_*` → built-in engine fallback.
Recognized keys include `default_max_ai_credits`, `default_max_turn_cache_misses`, `default_detection_max_ai_credits`, `default_max_daily_ai_credits`, `default_timeout_minutes`, `default_max_turns`, `default_detection_model`, `default_utc`, `default_model_copilot`, `default_model_claude`, `default_model_codex`. The compiler resolves model selection as `GH_AW_MODEL_*` → `GH_AW_DEFAULT_MODEL_*` → built-in engine fallback.

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.

Consider keeping the recognized-keys ordering consistent with the YAML example above for easier scanning.

@pelikhan pelikhan merged commit a896c99 into main Jun 20, 2026
@pelikhan pelikhan deleted the copilot/add-max-cache-misses-frontmatter branch June 20, 2026 00:58
@github-actions

Copy link
Copy Markdown
Contributor

Smoke Test PASS
• MCP PR listing: ✅
• mcpscripts PR query: ✅
• Serena CLI lookup: ✅
• Playwright nav: ✅
• HTTP fetch: ✅
• File create: ✅
• Discussion query: ✅
• Build: ✅
@app/copilot-swe-agent @pelikhan @Copilot

Warning

Firewall blocked 6 domains

The following domains were blocked by the firewall during workflow execution:

  • accounts.google.com
  • android.clients.google.com
  • clients2.google.com
  • contentautofill.googleapis.com
  • safebrowsingohttpgateway.googleapis.com
  • www.google.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "accounts.google.com"
    - "android.clients.google.com"
    - "clients2.google.com"
    - "contentautofill.googleapis.com"
    - "safebrowsingohttpgateway.googleapis.com"
    - "www.google.com"

See Network Configuration for more information.

📰 BREAKING: Report filed by Smoke Copilot - AOAI (Entra) · 124.7 AIC · ⌖ 8.4 AIC · ⊞ 18.2K ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants