Uh oh!
There was an error while loading. Please reload this page.
Conversation
…ateImportInputType, resolveRuntimeCooldown Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Great work! 🎉 This PR adds comprehensive test coverage for three critical pure functions in
Each test file demonstrates thoughtful test design — from table-driven subtests to explicit error-path validation. The PR body clearly documents why each function qualifies as pure, what the tests cover, and validation performed (gofmt, go vet, go test -race). This looks ready for review and merge. ✅
|
Uh oh!
There was an error while loading. Please reload this page.
✅ Test Quality Sentinel completed test quality analysis.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Pull request overview
Adds exhaustive unit tests for three pure parsing and runtime-resolution helpers.
Changes:
- Covers permission shorthand and scope parsing.
- Tests import input type validation branches.
- Tests typed and legacy runtime cooldown resolution.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/nodejs_cooldown_test.go | Tests cooldown defaults and runtime configurations. |
pkg/workflow/frontmatter_parsing_permissions_test.go | Tests permission parsing paths and scopes. |
pkg/parser/import_schema_validation_input_type_test.go | Tests supported input types and validation errors. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
| wantShort: "read-all", | ||
| }, | ||
| { | ||
| name: "shorthand as key with any value", |
| err = validateImportInputType("level", "low", "choice", map[string]any{}, "owner/repo/import.md") | ||
| assert.NoError(t, err) | ||
| // Options declared but not a []any: falls through the type switch (options ignored) -> no match -> error. |
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 36.1 AIC · ⌖ 10.3 AIC · ⊞ 7.1K
Comment /matt to run again
| // Matching option. | ||
| paramDef := map[string]any{"options": []any{"low", "medium", "high"}} | ||
| err = validateImportInputType("level", "medium", "choice", paramDef, "owner/repo/import.md") |
There was a problem hiding this comment.
[/tdd] Misleading comment: the trailing -> error is wrong — the assertion is assert.NoError. When options is not a []any, the implementation skips the match loop and returns nil. The comment should say -> no error, not -> error.
💡 Suggested fix
// Options declared but not a []any: options ignored (type switch falls through) -> no error.The current wording implies an error is expected, which contradicts the assertion and could mislead future readers.
@copilot please address this.
Test Quality Sentinel 🧪 — Score: ❌ REQUEST_CHANGESOverall Score: Cannot approve — hard violation present. Summary3 new test files, 20 test functions, 118 total assertions, excellent edge-case coverage with comprehensive table-driven parametric testing. Critical Issue: All 3 test files are missing the mandatory Violations & Issues❌ Hard Violation: Missing Build Tags All 3 test files lack the mandatory Required fix: Add Test Quality Assessment3 new test files with 20 test functions and 118 assertions provide excellent behavioral coverage:
Strengths: Action Required: Add build tags to proceed.
|
There was a problem hiding this comment.
❌ Test Quality Sentinel: Hard violation detected.
Missing build tags on all 3 test files (required: (go/redacted):build !integration). Test quality is excellent (118 assertions, 20 tests, comprehensive edge-case coverage), but this is a procedural blocker per gh-aw developer conventions. Add build tags to line 1 of each test file to proceed.
There was a problem hiding this comment.
Review: Pure-function test suites for parsePermissionsConfig, validateImportInputType, resolveRuntimeCooldown
The test coverage is comprehensive and well-structured. One blocking issue found:
parsePermissionsConfig shorthand inconsistency — the new tests lock in "read" and "write" as valid shorthand values, but permissions_parser.go explicitly removed them from the shorthand list because they generate invalid GitHub Actions YAML. The two implementations disagree, and the new tests solidify the inconsistency rather than exposing it as a bug. Please align parsePermissionsConfig's shorthandPerms with the narrower list in permissions_parser.go and update the tests accordingly.
Other observations (non-blocking)
- The
TestValidateImportInputType_Choicetest case foroptions: "not-a-list"comments "no match -> error" but assertsNoError. The comment is misleading; the actual behavior (no error because the type switch onoptssimply doesn't match[]any) is correct but the inline reasoning is confusing. TestResolveRuntimeCooldown_TypedRuntimeConfigis thorough and the table-driven approach is appropriate for covering all runtime IDs.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 50.8 AIC · ⌖ 6.96 AIC · ⊞ 5.5K
| assert.Equal(t, "read", config.OrganizationCustomProperties) | ||
| assert.Equal(t, "write", config.OrganizationCustomRepositoryRoles) | ||
| assert.Equal(t, "read", config.OrganizationAnnouncementBanners) | ||
| assert.Equal(t, "read", config.OrganizationEvents) |
There was a problem hiding this comment.
Inconsistent shorthand definition between parsePermissionsConfig and PermissionsParser.
permissions_parser.go:104-105 explicitly states:
// Note: "read" and "write" are no longer valid shorthands as they create invalid GitHub Actions YAML
shorthandPerms := []string{"read-all", "write-all", "none"}
But parsePermissionsConfig (in frontmatter_parsing.go:251) still includes "read" and "write" in its shorthandPerms. This new test cements that broader behavior.
This is either a latent bug in parsePermissionsConfig (should match permissions_parser.go's narrower list), or the test is locking in incorrect behavior. Please align the two implementations and update the test accordingly.
@copilot please address this.
🎉 This pull request is included in a new release. Release: |
Adds maximum-coverage testify test suites for three pure functions selected from the ranked candidate list.
parsePermissionsConfig(pkg/workflow/frontmatter_parsing.go:244)func parsePermissionsConfig(permissions map[string]any) (*PermissionsConfig, error)Why pure: takes a
map[string]anyand returns a new*PermissionsConfigand error; no I/O, no globals, no mutation of the input map. Precompute notes: "no observable side effects detected". Confirmed by reading the full function body — it only reads from the input map and writes into a freshly allocated struct.Coverage: function 49.1% → 100.0%; package
pkg/workflow~86.7% → 86.8%.Tests: 8 top-level tests, 5 table-driven subtests for shorthand recognition, plus dedicated tests for non-shorthand fallback, non-string shorthand values, all GitHub Actions scopes, all GitHub App scopes, unknown/ignored scopes, empty map, and the "multiple entries never shorthand" rule. ~13 test functions / 5 subtests, 60+ assertions covering every switch case.
Fuzzing: not used — input is a bounded enum-keyed map, table-driven cases give full branch coverage. No residual uncovered lines.
validateImportInputType(pkg/parser/import_schema_validation.go:120)func validateImportInputType(name string, value any, declaredType string, paramDef map[string]any, importPath string) errorWhy pure: returns only an
errorderived from its inputs (recursing into itself andvalidateObjectInput, which is also pure); no side effects. Precompute notes: "no observable side effects detected".Coverage: function 15.6% → 100.0%; package
pkg/parser~70.9% (unchanged at package level rounding, function fully covered).Tests: 7 test functions covering
string,number(all Go numeric types),boolean,choice(including options-not-a-list, non-string options entries, and no-options-declared branches),array(including nested recursive item validation and error propagation with indexed names),objectdelegation, and unknown declared types. ~40 assertions total. Renamed local helper toTestValidateImportInputType_NumberAllTypesto avoid colliding with an existingTestValidateImportInputType_Numberinimport_field_extractor_test.go.Fuzzing: not used — input space is a small fixed set of
declaredTypevalues with well-defined type-switch branches; table/branch coverage is exhaustive. No residual uncovered lines.resolveRuntimeCooldown(pkg/workflow/nodejs.go:351)func resolveRuntimeCooldown(workflowData *WorkflowData, runtimeID string) boolWhy pure: reads only from the passed-in
*WorkflowDataand returns abool; no mutation, no I/O. Precompute notes: "no observable side effects detected".Coverage: function 34.5% → 100.0%; package
pkg/workflow~86.7% → 86.8% (shared with parsePermissionsConfig above).Tests: nil-workflowData default, table-driven typed-runtime-config coverage for all 12 supported runtime IDs (cooldown-false / cooldown-true / cooldown-nil-falls-through = 36 subtests), unknown runtime ID fallthrough, nil ParsedFrontmatter/RuntimesTyped fallback, and 6 legacy
Runtimes map[string]anybranch cases (missing key, wrong type, missing cooldown key, non-bool cooldown, explicit true/false). ~50 assertions total.Fuzzing: not used — bounded runtime-ID enum and boolean/nil combinations are fully enumerated by table-driven subtests. No residual uncovered lines.
Validation performed for all three:
gofmt -lclean,go vet ./pkg/workflow/ ./pkg/parser/clean,go test ./pkg/workflow/ ./pkg/parser/ -race -count=1passing, coverage confirmed viago tool cover -func.