Skip to content

Reduce BenchmarkValidation overhead by caching safe-output field order #44090

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 4 commits into from
Jul 8, 2026
Merged

Reduce BenchmarkValidation overhead by caching safe-output field order #44090

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f30b546
Initial plan
Copilot Jul 7, 2026
4a6399c
Optimize safe-output sample field iteration in validation path
Copilot Jul 7, 2026
69b6683
Strengthen cached field-name test for safe-output samples
Copilot Jul 7, 2026
81dfd34
Harden cached field-order test for safe-output samples
Copilot Jul 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions pkg/workflow/samples_replay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ import (
"encoding/json"
"fmt"
"strings"

"github.com/github/gh-aw/pkg/sliceutil"
)

// SampleEntry is the per-call payload consumed by apply_samples.cjs.
Expand All @@ -30,7 +28,7 @@ func collectSampleEntries(config *SafeOutputsConfig) []SampleEntry {
return nil
}

fieldNames := sliceutil.SortedKeys(safeOutputFieldMapping)
fieldNames := getSortedSafeOutputFieldNames()

var entries []SampleEntry
for _, fieldName := range fieldNames {
Expand Down
12 changes: 11 additions & 1 deletion pkg/workflow/samples_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ var (
compiledToolSchemasOnce sync.Once
compiledToolSchemas map[string]toolSchemaEntry
compiledToolSchemasErr error

sortedSafeOutputFieldNamesOnce sync.Once
sortedSafeOutputFieldNames []string

@github-actions github-actions Bot Jul 8, 2026

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.

[/codebase-design] The new sync.Once variables are grouped with compiledToolSchemas* in the same var block, but they serve a logically different concern (field-name ordering vs. schema compilation). Consider adding a short comment separating them, following the pattern already used for compiledToolSchemas.

💡 Suggestion 
var (
    compiledToolSchemasOnce sync.Once
    compiledToolSchemas     map[string]toolSchemaEntry
    compiledToolSchemasErr  error
)

// sortedSafeOutputFieldNames caches the deterministic key order of
// safeOutputFieldMapping, computed once and reused across validation passes.
var (
    sortedSafeOutputFieldNamesOnce sync.Once
    sortedSafeOutputFieldNames     []string
)

This mirrors the doc-comment convention already in place for compiledToolSchemas (line 58) and makes each cache group independently navigable.

@copilot please address this.

)

func getCompiledToolSchemas() (map[string]toolSchemaEntry, error) {
Expand Down Expand Up @@ -97,6 +100,13 @@ func getCompiledToolSchemas() (map[string]toolSchemaEntry, error) {
return compiledToolSchemas, compiledToolSchemasErr
}

func getSortedSafeOutputFieldNames() []string {
sortedSafeOutputFieldNamesOnce.Do(func() {
sortedSafeOutputFieldNames = sliceutil.SortedKeys(safeOutputFieldMapping)
})
return sortedSafeOutputFieldNames
}
Comment on lines +104 to +108

@github-actions github-actions Bot Jul 8, 2026

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.

[/codebase-design] getSortedSafeOutputFieldNames returns the mutable cached slice directly. Any caller that appends to or sorts the returned slice will silently corrupt the cache for all future callers.

💡 Suggested fix

Return a copy, or document the contract that callers must not mutate the result:

// getSortedSafeOutputFieldNames returns the sorted keys of safeOutputFieldMapping.
// The returned slice MUST NOT be modified by the caller.
func getSortedSafeOutputFieldNames() []string {
    sortedSafeOutputFieldNamesOnce.Do(func() {
        sortedSafeOutputFieldNames = sliceutil.SortedKeys(safeOutputFieldMapping)
    })
    return sortedSafeOutputFieldNames
}

The existing call sites only range over the slice, so they are safe today — but this is an implicit contract that new callers can easily violate. A doc comment is low-cost insurance.

@copilot please address this.


// validateSafeOutputsSamples validates every `samples` entry on every
// enabled safe-output handler against the corresponding MCP tool's inputSchema.
// Sample sidecar fields (e.g. `patch`) are stripped before validation. Returns
Expand All @@ -107,7 +117,7 @@ func validateSafeOutputsSamples(config *SafeOutputsConfig) error {
return nil
}

fieldNames := sliceutil.SortedKeys(safeOutputFieldMapping)
fieldNames := getSortedSafeOutputFieldNames()
samplesValidationLog.Printf("Validating safe-outputs samples across %d candidate fields", len(fieldNames))

for _, fieldName := range fieldNames {
Expand Down
15 changes: 15 additions & 0 deletions pkg/workflow/samples_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,25 @@ import (
"strings"
"testing"

"github.com/github/gh-aw/pkg/sliceutil"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestGetSortedSafeOutputFieldNames_MatchesSortedKeys(t *testing.T) {
t.Parallel()

@github-actions github-actions Bot Jul 8, 2026

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] The test asserts cache correctness but not the benchmark regression it was designed to fix. There is no regression test or benchmark that would catch a future reintroduction of the per-call sort.

💡 Suggestion

Add a BenchmarkGetSortedSafeOutputFieldNames that calls b.ReportAllocs() alongside the existing BenchmarkValidation in compiler_performance_benchmark_test.go. A zero-alloc assertion is both more meaningful and more durable than the current pointer-equality check:

func BenchmarkGetSortedSafeOutputFieldNames(b *testing.B) {
    b.ReportAllocs()
    for b.Loop() {
        _ = getSortedSafeOutputFieldNames()
    }
    // After warm-up the cache is primed; allocs should be 0
}

This would have caught the original regression directly.

@copilot please address this.


expected := sliceutil.SortedKeys(safeOutputFieldMapping)
first := getSortedSafeOutputFieldNames()
second := getSortedSafeOutputFieldNames()

assert.Equal(t, expected, first)
assert.Equal(t, expected, second)
if assert.NotEmpty(t, first) && assert.NotEmpty(t, second) {
assert.Equal(t, &first[0], &second[0], "expected cached field names to reuse the same backing slice")

@github-actions github-actions Bot Jul 8, 2026

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] The pointer-identity assertion (&first[0] == &second[0]) is an implementation detail of sync.Once, not a public contract of getSortedSafeOutputFieldNames. If the implementation ever switches to returning a defensive copy, this test breaks even though the behaviour is correct.

💡 Suggested alternative

The meaningful invariant is idempotent sorted order, which value-equality already captures:

// first two asserts already cover this
assert.Equal(t, first, second)

If you want to document the zero-alloc property, a BenchmarkGetSortedSafeOutputFieldNames with b.ReportAllocs() is a more meaningful and durable guarantee than a pointer check.

@copilot please address this.

}
Comment on lines +21 to +23
}

// TestValidateSafeOutputsSamples_Valid covers the happy path for the
// strict schema validation of samples entries. We use create_issue (no
// sidecars, just title/body) and create_pull_request (with the `patch` sidecar
Expand Down
Loading