Restore frontmatter schema consistency coverage - #52577
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR TriageCategory: chore · Risk: low · Priority: low · Score: 33/100
Recommended action: Restores schema-consistency coverage by extracting fields directly from
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (only 1 addition detected).
|
|
✅ PR Code Quality Reviewer completed the code quality review. Completed PR review for #52577; no GitHub writes emitted because no actionable review findings were identified.
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
I didn't find any blocking issues in the changed lines.
Review notes
I checked the schema-doc generator change for the new patternProperties path, the schema-consistency workflow updates to the active frontmatter type surface, and the regression tests added for the generated reference output. The implementation and tests line up with the schema changes, and I did not find a correctness or maintainability problem in the diff that warrants a review comment.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 14 AIC · ⌖ 5.17 AIC · ⊞ 6.5K
Comment /review to run again
There was a problem hiding this comment.
Pull request overview
Restores schema-consistency checks against active frontmatter types and expands generated reference coverage.
Changes:
- Targets active JSON/YAML frontmatter fields.
- Supports
patternPropertiesexamples. - Adds regression assertions and regenerated documentation.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/schema-consistency-checker.md |
Updates schema comparison inputs and reporting. |
.github/workflows/schema-consistency-checker.lock.yml |
Regenerates the compiled workflow. |
scripts/generate-schema-docs.js |
Expands dynamic-key schemas. |
scripts/generate-schema-docs.test.js |
Adds regression checks. |
pkg/parser/schemas/main_workflow_schema.json |
Annotates the runtime example key. |
docs/src/content/docs/reference/frontmatter-full.md |
Adds missing searchable fields. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
| FRONTMATTER_FIELDS=$(grep -Eho '(json|yaml):"[^"]*"' pkg/workflow/frontmatter_types.go 2>/dev/null \ | ||
| | sed -E 's/^(json|yaml):"//;s/"$//' \ | ||
| | sed 's/,omitempty//' \ | ||
| | sed 's/,.*$//' \ | ||
| | grep -v '^-$' \ |
There was a problem hiding this comment.
The changes are correct and well-scoped.
- Extracting frontmatter fields from
pkg/workflow/frontmatter_types.goinstead ofpkg/parser/*.gois the right fix — the parser package only has a logger declaration for frontmatter. - The new
getDynamicPropertyhelper correctly handles bothadditionalPropertiesandpatternPropertiesschemas, fixing the silent skip for theruntimesfield (which usespatternProperties). - Test 8 correction (removing
#/properties/defaults) is valid —defaultsis not a root schema property. - All tests pass (
node scripts/generate-schema-docs.test.js→ ✅ All tests passed).
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 40.4 AIC · ⌖ 7.04 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on two correctness issues.
📋 Key Themes & Highlights
Key Themes
- Test signal loss: Two test assertions were weakened (
"claude"→"example-value",#/properties/defaultsremoved) without replacement coverage, reducing confidence that the doc generator correctly resolves real values. - Null-safety gap:
getDynamicPropertycan returnnullfor multi-pattern schemas; both call sites use the result without a null guard, risking a runtimeTypeError.
Positive Highlights
- ✅ Correctly redirecting the schema-consistency checker to
pkg/workflow/frontmatter_types.go— this was the real source file all along. - ✅ The
getDynamicPropertyrefactor is a good abstraction that removes duplication betweengenerateVariantsandgenerateProperty. - ✅ New regression tests for
run-install-scriptsandreport-failed-jobsin Test 11 are a solid addition. - ✅
x-example-keyadded to theruntimesschema entry cleanly solves the{}placeholder problem.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 41.9 AIC · ⌖ 8.58 AIC · ⊞ 7K
Comment /matt to run again
Comments that could not be inline-anchored
scripts/generate-schema-docs.test.js:66
[/tdd] The assertion was updated from 'engine: "claude"' to 'engine: "example-value"' — this no longer verifies meaningful output. Any string fallback would pass; if getExampleValue ever silently regresses (e.g. returns null or empty), the test stays green.
<details>
<summary>💡 Suggested fix</summary>
The engine_config $ref is a oneOf with no enum on the string branch, so the generator falls back to "example-value". To preserve signal:
- Add a
default(e.g. `"copilot"…
scripts/generate-schema-docs.test.js:108
[/tdd] Removing "#/properties/defaults" from allRefs silently skips ref-resolution coverage for defaults. The comment says "Defaults is nested within safe-output configuration" — but that may change and removing the assertion offers no protection.
<details>
<summary>💡 Suggested fix</summary>
If defaults is now a sub-property rather than a root schema property, add an assertion targeting its actual location:
assertContains(output, 'defaults:', 'defaults should be present in…
</details>
<details><summary>scripts/generate-schema-docs.js:96</summary>
**[/codebase-design]** `getDynamicProperty` silently returns `null` when `patternProperties` has more than one entry. Nothing in the calling code guards against `null`, so a schema with two patterns would produce a runtime `TypeError` (`Cannot read properties of null`).
<details>
<summary>💡 Suggested fix</summary>
```js
// In getDynamicProperty, consider returning the first entry even with multiple patterns:
if (patternProperties.length >= 1 && typeof patternProperties[0] === 'object') {
r…
</details>
🧪 Test Quality Sentinel Report
📊 Analysis Summary
Changes Breakdown:
|
|
@copilot Please resolve the blocking review feedback on this PR, refresh the branch if needed, and run the Outstanding review items (newest first):
Branch update was requested automatically for this run.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Resolved in caa3fa4. Field extraction is now limited to the top-level |
Schema-consistency reporting inspected stale parser files and missed the active frontmatter type surface. The generated full reference also omitted searchable entries for two supported fields.
Schema consistency workflow
pkg/workflow/frontmatter_types.go.jsonandyamltags.Generated frontmatter reference
patternProperties.node, exposing nested runtime fields including:Regression coverage
Run: https://github.com/github/gh-aw/actions/runs/31767348894> Generated by 👨🍳 PR Sous Chef · gpt54 · 23.3 AIC · ⌖ 5.78 AIC · ⊞ 8.5K · ◷