perf(validation): remove YAML parse hot path in permission scope validation#34797
perf(validation): remove YAML parse hot path in permission scope validation#34797Copilot wants to merge 4 commits into
Conversation
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Optimizes ValidatePermissionScopeNames by avoiding full YAML unmarshalling on the hot path, while preserving strict behavior via a fallback parse for non-standard YAML forms. This targets a measured validation regression by reducing repeated allocation and YAML decode overhead.
Changes:
- Added a fast-path extractor to scan block-style YAML and validate only top-level permission scope keys without
yaml.Unmarshal. - Hoisted reusable scope validation data (scope list, meta-key set, error-preview string) into package-level cached state.
- Added tests covering quoted permission keys and flow-mapping permission syntax (fallback path).
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/permissions_validation.go | Adds cached validation state, a fast-path key extractor, and a helper to validate a single scope key; retains YAML unmarshal fallback for non-standard formats. |
| pkg/workflow/permissions_scope_validation_test.go | Adds regression tests for quoted keys and flow mapping shorthand to ensure compatibility across fast/fallback paths. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
|
✅ 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. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 70/100 — Acceptable
📊 Metrics & Test Classification (2 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (138 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
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 ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs in this repo are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose, /tdd, and /zoom-out. The performance motivation is clear and the refactor is well-structured — commenting with observations rather than requesting changes.
📋 Key Themes & Highlights
Key Themes
- Missing benchmark (
/diagnose): The perf regression was measured, but noBenchmarkValidatePermissionScopeNameswas added to prevent it from silently regressing again. - Thin fast-path test coverage (
/tdd):extractTopLevelPermissionScopeKeysis only exercised indirectly; tab indentation, multi-line scalars (|/>), and mixed-indent inputs are not covered. - Silent skip on unexpected indentation (
/zoom-out): Theindent != baseIndent → continuepath correctly handles value continuations, but also silently drops any scope key at an unexpected indent level, suppressing typo detection for that key.
Positive Highlights
- ✅ Excellent factoring:
validateSinglePermissionScopeKeyis a clean, reusable helper with clear error semantics - ✅ Hoisting
allScopes,validMeta, and the preview string to package-level vars is a textbook hot-path fix - ✅ The bracket-detection bail-out (
{,},[,]) is a pragmatic and safe guard for the YAML fallback - ✅ The two new test cases target the exact failure modes the fast path could introduce
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.9M
Comments that could not be inline-anchored
pkg/workflow/permissions_scope_validation_test.go:104
[/diagnose] No benchmark added — the PR motivates the change with a specific regression measurement (+17.9%), but there is no BenchmarkValidatePermissionScopeNames to lock in the gain and detect future regressions.
<details>
<summary>💡 Suggested benchmark skeleton</summary>
func BenchmarkValidatePermissionScopeNames(b *testing.B) {
yaml := `contents: read
issues: write
pull-requests: read
actions: write`
b.ResetTimer()
for i := 0; i < b.N; i++ {
_ = ValidatePe…
</details>
<details><summary>pkg/workflow/permissions_scope_validation_test.go:52</summary>
**[/tdd]** The new tests exercise `ValidatePermissionScopeNames` end-to-end but leave the fast-path extractor (`extractTopLevelPermissionScopeKeys`) untested for several structural YAML patterns it must handle or correctly reject.
<details>
<summary>💡 Suggested additional cases</summary>
Missing test scenarios:
- **Tab-indented keys**: YAML allows tabs in some contexts — the fast path counts tabs as whitespace. Does `\t\tcontents: read` produce a correct `baseIndent`?
- **Multi-line scalar v…
</details>
<details><summary>pkg/workflow/permissions_validation.go:523</summary>
**[/zoom-out]** When `indent != baseIndent`, the fast path silently skips the line rather than bailing out to the YAML fallback. This is the right behaviour for multi-line scalar continuations, but it also silently drops any scope key whose indentation differs from the first key — meaning a typo in such a key would never surface a "Did you mean" error.
<details>
<summary>💡 Why this matters and a safer alternative</summary>
Consider:
```yaml
contents: read
pull-rquests: write # indent=2, …
</details>
There was a problem hiding this comment.
REQUEST_CHANGES — The fast-path optimisation has two correctness bugs and one misleading error-message defect that need to be fixed before merge.
### Blocking issues
-
Fast path silently drops nested-indented key-value lines (line 525) — when
indent != baseIndentand the line still has a colon, the fast path skips it and returnsparsedFast=true. The YAML fallback that would have caught the anomaly never runs, so malformed YAML or genuinely nested map content is silently accepted instead of validated or rejected. -
strings.Trimstrips unbalanced quotes (line 532) — a key like"write-all(missing closing quote) has its leading"stripped and validates as the valid meta-keywrite-all, masking the authoring error. Only a single balanced pair should be removed. -
"..."always appended to scopes preview (line 27) — minor but produces misleading error messages when the scope list has ≤ 10 entries.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.9M
| } | ||
| if indent != baseIndent { | ||
| // Nested block content (value continuation), not a top-level scope key. | ||
| continue |
There was a problem hiding this comment.
Fast path silently skips deeper-indented key-value lines instead of falling back, diverging from YAML semantics.
💡 Details and fix
When indent != baseIndent, the code does continue. This is fine for plain scalar continuations, but if a deeper-indented line is itself a key: value pair (e.g. from an accidentally-indented permission or a nested map), the fast path silently drops it and returns parsedFast=true. The YAML fallback — which would surface the nested key or return a parse error — never runs.
Example: contents: read\n badscope: write is invalid YAML (the YAML parser would error → ValidatePermissionScopeNames returns nil). The fast path returns ["contents"] and parsedFast=true, silently accepting the document.
Fix: when a deeper-indented line contains a colon, bail out to the YAML parser rather than skipping:
if indent != baseIndent {
if colonIdx > 0 {
// Looks like a nested map key — hand off to YAML parser.
return nil, false
}
continue // plain scalar value continuation, safe to skip
}| if key == "" || strings.HasPrefix(key, "-") { | ||
| return nil, false | ||
| } | ||
| key = strings.Trim(key, `"'`) |
There was a problem hiding this comment.
strings.Trim strips all leading/trailing quote characters rather than one balanced pair, silently normalising malformed keys.
💡 Details and fix
strings.Trim(key, "\"'") removes every leading and trailing " or ' character. A malformed key like ""contents"" becomes contents and validates successfully. A key like "write-all (unbalanced quote, meaning a workflow authoring error) loses its leading quote and is validated as write-all — a valid meta key — masking the typo.
The intent is to unquote YAML keys that were written with explicit quotes, but only a single balanced pair should be removed:
if len(key) >= 2 {
q := key[0]
if (q == '"' || q == '\'' ) && key[len(key)-1] == q {
key = key[1 : len(key)-1]
}
}If the quotes are unbalanced, leaving the key unchanged (or returning nil, false) lets the YAML fallback handle it correctly.
| } | ||
| permissionScopeValidationValidScopesPreview = strings.Join( | ||
| permissionScopeValidationAllScopes[:min(10, len(permissionScopeValidationAllScopes))], | ||
| ", ", |
There was a problem hiding this comment.
"..." is unconditionally appended to the scopes preview even when all scopes fit within the 10-item cap.
💡 Details and fix
If buildPermissionScopeValidationAllScopes() ever returns ≤ 10 entries — possible in tests or if the scope list is trimmed — the error message claims there are more valid scopes than shown, which is misleading.
// Fix:
if len(permissionScopeValidationAllScopes) > 10 {
permissionScopeValidationValidScopesPreview = strings.Join(
permissionScopeValidationAllScopes[:10], ", ",
) + "..."
} else {
permissionScopeValidationValidScopesPreview = strings.Join(
permissionScopeValidationAllScopes, ", ",
)
}
Validation benchmark regressed to ~29.7µs/op (+17.9% vs historical). Profiling showed
ValidatePermissionScopeNameson the hot path, dominated by repeated scope-list construction and YAML unmarshal work.Hot-path optimization: permission scope-name validation
yaml.Unmarshal.all,read-all,write-all,none)Behavior-preserving coverage