[purelock] Lock down extractExperimentVariantStubs, isValidToolName, validateObjectInput with pure-function test suites - #52048
Conversation
…, isValidToolName, validateObjectInput Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
✅ 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.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully!
|
There was a problem hiding this comment.
Pull request overview
Adds focused regression coverage for three pure helper functions across CLI metadata, tool-name validation, and parser schema validation.
Changes:
- Tests deterministic experiment-variant extraction and sorting.
- Adds table-driven, stop-word, and fuzz coverage for tool names.
- Covers object-schema validation success and error paths.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/forecast_metadata_extract_experiment_variant_stubs_test.go |
Tests variant extraction, ordering, nil entries, and defaults. |
pkg/cli/logs_report_tools_isvalidtoolname_test.go |
Tests tool-name rules and fuzz invariants. |
pkg/parser/import_schema_validation_object_input_test.go |
Tests object property and nested type validation. |
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: 0
- Review effort level: Balanced
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The three new test files add solid pure-function coverage for extractExperimentVariantStubs, isValidToolName, and validateObjectInput. Tests are correct, edge cases are well-chosen, and the fuzz test for isValidToolName is a nice addition. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18.9 AIC · ⌖ 6.21 AIC · ⊞ 5.4K
|
🧪 Test Quality Sentinel Report❌ Test Quality Score: 95/100 — Excellent
📊 Metrics (22 tests)
Per-Test Classification
|
There was a problem hiding this comment.
❌ Test Quality Sentinel: 95/100. Hard violation detected: missing (go/redacted):build !integration build tags on all new test files. This is a mandatory coding guideline that must be fixed before merging. Test design quality is excellent (100% design tests, 82% edge coverage), but coding compliance is required.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — commenting with actionable suggestions; no blocking correctness issues.
📋 Key Themes & Highlights
Key Themes
- Assertion consistency: mixing
assert.Nilandassert.Emptyfor the same emptiness concept creates fragile expectations. - Redundant test: the idempotency test duplicates what the fuzz corpus already proves more thoroughly.
- Opaque fallback contract: the "permissive on malformed schema" behaviour needs a comment or a better name to survive future refactors.
- Monolithic multi-type assertion: packing three type checks into one test reduces failure diagnosability.
Positive Highlights
- ✅ Excellent fuzz test for
isValidToolNamewith meaningful seeds and a determinism invariant - ✅
TestIsValidToolName_AllStopWordsiterates the live map — zero drift risk - ✅
validateObjectInputsuite reaches 100% and covers all documented edge cases - ✅ Good use of
assert.Containson error messages to pin the contract without over-specifying the full string
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 32.7 AIC · ⌖ 6.88 AIC · ⊞ 7.1K
Comment /matt to run again
| func TestExtractExperimentVariantStubs_NoExperiments(t *testing.T) { | ||
| cfg := &workflow.FrontmatterConfig{} | ||
| stubs := extractExperimentVariantStubs(cfg) | ||
| assert.Nil(t, stubs) |
There was a problem hiding this comment.
[/tdd] assert.Nil vs assert.Empty — assert.Nil(t, stubs) enforces a nil-slice contract, but the companion test at line 23 uses assert.Empty. If the implementation ever returns a non-nil empty slice instead of nil, the first test fails while the second passes silently. Either make both assertions consistent (assert.Empty for emptiness-only) or add a comment documenting that the nil return is a deliberate contract worth enforcing.
@copilot please address this.
There was a problem hiding this comment.
Updated in 472e5e3: the no-experiments case now uses assert.Empty, consistent with the companion empty-result case.
| assert.Equal(t, "y", stubs[1].Variant) | ||
| assert.Equal(t, "zeta", stubs[2].ExperimentName) | ||
| assert.Equal(t, "a", stubs[2].Variant) | ||
| assert.Equal(t, "zeta", stubs[3].ExperimentName) |
There was a problem hiding this comment.
[/tdd] No test for an experiment with an empty Variants slice — TestExtractExperimentVariantStubs_MultipleExperimentsMixedNil covers a nil config but exp3 uses {Variants: []string{}}. The test asserts Len(t, stubs, 1) which implicitly validates the empty-variants case, but there is no standalone test that names this contract. A dedicated _EmptyVariants case makes the specification explicit and easier to maintain.
@copilot please address this.
There was a problem hiding this comment.
Added a dedicated TestExtractExperimentVariantStubs_EmptyVariants case in 472e5e3.
| {"single character", "a", false}, | ||
| {"single character uppercase", "X", false}, | ||
| {"stop word calls", "calls", false}, | ||
| {"stop word to", "to", false}, |
There was a problem hiding this comment.
[/tdd] The comment // len 9 < 10 is correct, but the case name "short lowercase single word exactly under length limit" could be sharpened — "abcdefghi" is 9 chars, and the rule is len < 10, so "exactly under" is accurate. However, there is no boundary test for len == 10 with mixed-case ("abcdefghiJ") to confirm that a capital letter alone makes a 10-char string pass. The existing cases test length and capitalisation independently but not their interaction at the boundary.
@copilot please address this.
There was a problem hiding this comment.
Added the mixed-case len == 10 boundary case (abcdefghiJ) in 472e5e3.
| } | ||
| } | ||
|
|
||
| func TestIsValidToolName_Idempotent(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] TestIsValidToolName_Idempotent checks purity (same in → same out), which the fuzz test already covers more thoroughly. These are the same property. Consider replacing this test with one that adds real coverage value — e.g., asserting that stop-word lookup is case-sensitive ("Calls" vs "calls") or that Unicode multi-byte input does not panic.
@copilot please address this.
There was a problem hiding this comment.
Removed the redundant standalone idempotency test in 472e5e3; determinism remains covered by the fuzz test.
| paramDef := map[string]any{"properties": "not-a-map"} | ||
| value := map[string]any{"key": "value"} | ||
| err := validateObjectInput("config", value, paramDef, "import") | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
[/tdd] TestValidateObjectInput_PropertiesNotAMap_AcceptsAnyObject confirms that a non-map properties value causes the function to silently accept anything. This is important behaviour, but the test name does not hint that this is a lenient fallback — a future reader might change the implementation to return an error and not realise this test encodes a deliberate "permissive" contract. A one-line comment in the test body (or a name like _MalformedPropertiesField_FallsBackToPermissive) would make the intent explicit.
@copilot please address this.
There was a problem hiding this comment.
Renamed the case to TestValidateObjectInput_MalformedPropertiesField_FallsBackToPermissive in 472e5e3 to make the fallback contract explicit.
| "count": map[string]any{"type": "number"}, | ||
| }, | ||
| } | ||
| value := map[string]any{"count": "not-a-number"} |
There was a problem hiding this comment.
[/tdd] TestValidateObjectInput_TypeMatch packs three distinct type checks (number, string, boolean) into one assertion. When it fails, Go will report only which field was wrong, but the test reads as one monolithic pass/fail. Per /tdd, each case should be independently identifiable — either split into three top-level tests or use t.Run sub-tests so failures are individually named and CI output is immediately actionable.
@copilot please address this.
There was a problem hiding this comment.
Converted the type-match checks to independently named table-driven subtests in 472e5e3.
There was a problem hiding this comment.
Ponytail review (over-engineering only).
net: -7 lines possible.
Generated by ✂️ Ponytail Reviewer for #52048 · auto · 19.6 AIC · ⌖ 6.52 AIC · ⊞ 6.8K
Comment /ponytail to run again
| func TestIsValidToolName_Idempotent(t *testing.T) { | ||
| // Pure function property: calling twice with the same input yields the same result. | ||
| inputs := []string{"", "-", "a", "run_tests", "calls", " spaced ", "camelCase"} | ||
| for _, in := range inputs { | ||
| assert.Equal(t, isValidToolName(in), isValidToolName(in)) | ||
| } | ||
| } |
There was a problem hiding this comment.
L55-61: delete: TestIsValidToolName_Idempotent duplicates the determinism check already inlined in FuzzIsValidToolName (got1 != got2). Nothing replaces it.
There was a problem hiding this comment.
Removed TestIsValidToolName_Idempotent in 472e5e3; the fuzz test remains the single determinism check.
|
🎯 Excellent work on this test coverage PR! The PureLock workflow has generated three well-scoped, high-quality test suites that lock down pure functions with comprehensive coverage. What's great here:
Ready for review — this PR demonstrates strong test engineering practices and is well-aligned with the project's testing standards. The focused scope and comprehensive edge-case coverage make this a solid addition to the codebase.
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…object-ac8ca124f4b490a7
|
@copilot Please address the open review feedback, refresh the branch if needed, and use the pr-finisher skill before handing back. Open review threads (newest reviewer feedback first):
No failed checks were reported in the compact candidate set for this run.
|
Addressed in 472e5e3 and replied to both linked threads. The branch already includes current |
Summary
Locks down 3 pure Go functions with maximum-coverage testify test suites, selected from the precomputed candidate ranking (
purelock/candidates.json).extractExperimentVariantStubs—pkg/cli/forecast_metadata.go:158func extractExperimentVariantStubs(cfg *workflow.FrontmatterConfig) []ForecastVariantResultPurity: no observable side effects (per precomputed
purity_notes); confirmed manually — reads only from the inputcfg.ExperimentConfigsmap, builds a new slice, and sorts it deterministically withslices.SortFunc. No I/O, no globals, no mutation of inputs.pkg/cli): 61.8% → 61.8% (function is small relative to a large package; net package coverage unaffected within reported precision, added tests cover previously untested logic)fuzz_friendly)slices.SortFunc's comparator (equal variant strings,default: return 0) is exercised only implicitly by Go's sort algorithm and not directly hit by the added cases; low risk since logic is trivial.isValidToolName—pkg/cli/logs_report_tools.go:34func isValidToolName(toolName string) boolPurity: no observable side effects; confirmed manually — pure string predicate over
toolNameStopWords(package-level constant map, read-only) and the input string. No mutation, no I/O.pkg/cli): 61.8% → 61.8%FuzzIsValidToolName)fuzz_friendlyvalidateObjectInput—pkg/parser/import_schema_validation.go:75func validateObjectInput(name string, value any, paramDef map[string]any, importPath string) errorPurity: no observable side effects; confirmed manually — validates an object value against a schema definition, returning an
errorornil. No mutation ofvalue/paramDef, no I/O.pkg/parser): 70.9% → 71.3%fuzz_friendly)Validation
gofmt -l— clean on all 3 new test filesgo vet ./pkg/cli/ ./pkg/parser/— cleango test ./pkg/cli/ -race -count=1andgo test ./pkg/parser/ -race -count=1— all new tests pass (2 pre-existing, unrelated failures inpkg/cli—TestRenderScheduleCalendarCell_UsesANSIInColorTerminalandTestConfirmRunAddedWorkflow_ContextCancelled— were already failing before this change and are untouched by it)Draft PR — please review test cases for edge-case completeness before merging.
Run: https://github.com/github/gh-aw/actions/runs/31511130973> Generated by 👨🍳 PR Sous Chef · gpt54 · 8.27 AIC · ⌖ 5.37 AIC · ⊞ 8.5K · ◷