[purelock] Lock down simplifyDataSchemaNode with a pure-function test suite - #51167
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
|
|
|
|
|
|
❌ 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. |
There was a problem hiding this comment.
Pull request overview
Adds comprehensive table-driven tests for simplifyDataSchemaNode, covering normalization, validation, recursion, and error paths.
Changes:
- Exercises schema shorthand and type inference.
- Covers object, array, scalar, enum, and validation behavior.
- Runs cases concurrently.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_outputs_data_schema_test.go |
Adds comprehensive unit coverage for schema simplification. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
| }, | ||
| }, | ||
| { | ||
| name: "unicode description and property names pass through", |
There was a problem hiding this comment.
The test suite is thorough and well-structured. All cases use t.Parallel() correctly, error messages are checked precisely with assert.Contains, and the table covers the key invariants: type inference, enum validation, recursive normalization, additionalProperties defaulting, and shorthand expansion. No blocking issues.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 16.4 AIC · ⊞ 5.5K
There was a problem hiding this comment.
Verdict: No blocking issues
Test-only PR adding TestSimplifyDataSchemaNode (34 subtests) for the pure simplifyDataSchemaNode function. Ran the suite locally — all subtests pass, and test expectations match the actual implementation branches (shorthand handling, object/array/enum/required/additionalProperties validation, error messages).
Review notes
- No production code changed; this is purely additive test coverage.
- Table-driven structure with proper
t.Parallel()usage on both parent and subtests. - Error message assertions use
Contains, which is appropriately tolerant of message formatting while still verifying key content. - Coverage of edge cases (unicode property names, enum scalar validation, nested property/array error propagation, forced-required+sorted output) is thorough.
No correctness, performance, or maintainability concerns found in the diff.
🔎 Code quality review by PR Code Quality Reviewer · auto · 16.3 AIC · ⊞ 7.8K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — approving with two minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Shorthand expansion check (line 196): the test verifies count and top-level fields but not individual expanded property schemas — a malformed child would silently pass.
int64enum value (line 306):int64(2)is accepted by the implementation type switch, but JSON/YAML unmarshaling never producesint64. This could be testing a realistic but unreachable code path without explanation.
Positive Highlights
- ✅ 34 parallel subtests — excellent isolation and fast feedback
- ✅ Exhaustive error-path coverage: every
wantErrcase maps to a distinct error message - ✅ Unicode test is a nice touch for a parser that touches user-facing paths
- ✅
assert.Nil(t, result)on error path is precise — catches the function contract - ✅ Table-driven structure is clean and easy to extend
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 34.6 AIC · ⊞ 7.1K
Comment /matt to run again
| assert.Equal(t, []string{"score", "verdict"}, result["required"]) | ||
| properties, ok := result["properties"].(map[string]any) | ||
| require.True(t, ok) | ||
| assert.Len(t, properties, 2) |
There was a problem hiding this comment.
[/tdd] The shorthand-expansion check verifies the count and top-level fields but never inspects individual expanded property schemas — a malformed child would still pass.
💡 Suggested addition
scoreSchema, ok := properties["score"].(map[string]any)
require.True(t, ok)
assert.Equal(t, "number", scoreSchema["type"])@copilot please address this.
| path: "root", | ||
| allowShorthand: false, | ||
| check: func(t *testing.T, result map[string]any) { | ||
| assert.Equal(t, []any{"a", 1, 1.5, true, int64(2)}, result["enum"]) |
There was a problem hiding this comment.
[/tdd] The enum test uses int64(2) as an enum value, but the implementation accepts int64 alongside int and float64. In practice, YAML/JSON unmarshalling never produces int64 — it produces float64 or int. If the production code path doesn't preserve int64, this assertion would silently be testing a dead path.
Consider replacing int64(2) with float64(2) (matching what JSON unmarshal produces) or adding a comment explaining why int64 is a legitimate value here.
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics
✅ Test Classification
Verdict
|
|
🎉 This pull request is included in a new release. Release: |
Overview
This PR is a test-only change that adds a comprehensive, pure-function test suite for
simplifyDataSchemaNodeinpkg/workflow/safe_outputs_data_schema_test.go. No production code is modified.Key facts
pkg/workflow/safe_outputs_data_schema_test.go)TestSimplifyDataSchemaNode— table-driven,t.Parallel()at top level and per subtestWhat is tested
The suite locks down the behavior of
simplifyDataSchemaNode(raw any, path string, allowShorthand bool) (map[string]any, error):"string"→{"type": "string"}) whenallowShorthandis true"string shorthand is not allowed here""unsupported type ...""expected an object schema"minimum/maximumpassthrough)日本語, path根), verifyingrequiredfield extraction and pass-through behaviorFile-by-file detail
pkg/workflow/safe_outputs_data_schema_test.go(modified): AddedTestSimplifyDataSchemaNode, a table-driven test covering string shorthand handling, object/array schema validation, numeric constraint passthrough, boolean types, unicode property names, and error paths forsimplifyDataSchemaNode.Purpose
Consistent with the PR title ("[purelock]"), this change locks down the existing pure-function behavior of
simplifyDataSchemaNodewith regression-guarding test coverage, without altering runtime logic.> Generated by PR Description Updater for #51167 · auto · 42.3 AIC · ⊞ 6.9K · ◷