feat: fail-fast guardrail for schema file changes without rebuild

[Copilot]

Schema files under pkg/parser/schemas/ are embedded at compile time via //go:embed; editing them without rebuilding silently leaves the running binary with stale schemas. There was no targeted check to catch this class of drift.

Changes

• pkg/parser/schema_embed_test.go — New TestEmbeddedSchemasAreValid test that compiles all four embedded schemas (main_workflow_schema, mcp_config_schema, repo_config_schema, aw_manifest_schema) as part of make test-unit. Because go test re-embeds files at compile time, running this test is equivalent to make build for schema-embedding purposes — any malformed JSON or invalid schema breaks the test immediately.

• scripts/check-stale-schema-binary.sh — New script (mirrors check-stale-lock-files.sh) that uses git diff to detect modified pkg/parser/schemas/*.json files and compares their mtimes against the binary, failing with a clear make build remediation message when stale.

• Makefile — New check-stale-schema-binary target (supports GITHUB_BASE_REF/CHECK_STALE_SCHEMA_BASE_REF for CI base-ref mode) wired into lint, which is already part of agent-report-progress.

• DEVGUIDE.md — New "Schema changes not taking effect / stale schema errors" entry under Common Error Scenarios documenting the rebuild requirement and pointing to the new guardrails.

[github-actions]

Hey @app/copilot-swe-agent 👋 — excellent work on the fail-fast guardrail for schema changes! The new test, script, Makefile target, and DEVGUIDE entry form a comprehensive solution. This PR looks ready for review! 🎉

Generated by ✅ Contribution Check · 74.6 AIC · ⌖ 16.7 AIC · ⊞ 6.2K · ◷

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #46189 does not have the 'implementation' label and has only 69 new lines of code in business logic directories (threshold: 100).

[Copilot]

Pull request overview

Adds guardrails to detect stale embedded schemas and validate schema integrity.

Changes:

• Adds embedded-schema compilation tests.
• Adds a stale-binary detection script integrated with linting.
• Documents schema rebuild requirements and refreshes generated workflow metadata.

Show a summary per file

File	Description
scripts/check-stale-schema-binary.sh	Detects schema changes newer than the binary.
pkg/parser/schema_embed_test.go	Validates all embedded schemas.
Makefile	Integrates the guard into linting.
DEVGUIDE.md	Documents schema rebuilding and validation.
.github/workflows/skillet.lock.yml	Refreshes generated action annotations.
.github/workflows/release.lock.yml	Refreshes generated action annotations.
.github/workflows/hourly-ci-cleaner.lock.yml	Refreshes generated action annotations.
.github/workflows/avenger.lock.yml	Refreshes generated action annotations.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 8/8 changed files
• Comments generated: 4
• Review effort level: Medium

[github-actions]

Clean, well-reasoned addition. The two-layer approach (unit test + mtime script) is sound. No blocking issues found.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 23.9 AIC · ⌖ 4.24 AIC · ⊞ 5K

[github-actions]

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 100/100 — Excellent

Analyzed 4 test(s) (4 table-driven subtests in 1 function): 4 design, 0 implementation, 0 violation(s).

📊 Metrics (4 subtests across 1 test function)

Metric	Value
Analyzed	4 subtests (Go: 4, JS: 0)
✅ Design	4 (100%)
⚠️ Implementation	0 (0%)
Edge/error coverage	4 (100%)
Duplicate clusters	0
Inflation	No
🚨 Violations	0

Test	File	Classification	Issues
TestEmbeddedSchemasAreValid/main_workflow_schema	pkg/parser/schema_embed_test.go:27	design_test / behavioral_contract	None
TestEmbeddedSchemasAreValid/mcp_config_schema	pkg/parser/schema_embed_test.go:32	design_test / behavioral_contract	None
TestEmbeddedSchemasAreValid/repo_config_schema	pkg/parser/schema_embed_test.go:37	design_test / behavioral_contract	None
TestEmbeddedSchemasAreValid/aw_manifest_schema	pkg/parser/schema_embed_test.go:42	design_test / behavioral_contract	None

Verdict

✅ Passed. 0% implementation tests (threshold: 30%). The test enforces the schema-embedding invariant: every schema embedded via //go:embed must be non-empty, valid JSON, and compilable. Each of the 4 table-driven subtests covers error paths with descriptive t.Fatalf messages. Build tag //go:build !integration is present. No mocks or violations detected.

References: §29632343424

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

🧪 Test quality analysis by Test Quality Sentinel · 28 AIC · ⌖ 7.27 AIC · ⊞ 6.8K · ◷
_{Comment /review to run again}

[github-actions]

✅ Test Quality Sentinel: 100/100. 0% implementation tests (threshold: 30%).

[github-actions]

REQUEST_CHANGES — 3 high/medium correctness issues in the new guardrail script

The goal is sound, but the shell script has two silent-failure modes in CI and one ergonomic regression that collectively undermine the guardrail this PR is meant to provide.

🔴 Blocking issues (3)

1. check-stale-schema-binary.sh line 396 — mtime check is a no-op in CI (HIGH): git checkout resets all timestamps; [ "$f" -nt "$BINARY" ] is always false in CI. The script itself acknowledges this, making the core check a local-only check that provides false confidence in CI pipelines.

2. check-stale-schema-binary.sh line 347 — shallow-clone silent pass (MEDIUM): git diff BASE...HEAD returns empty on shallow clones and the || true swallows the error — the check silently exits 0 even when it can't determine what changed.

3. Makefile line 176 — make lint now fails without binary (MEDIUM): check-stale-schema-binary errors with "binary not found" if make build hasn't run first, turning make lint into a two-step process with no documentation or graceful handling.

🟡 Non-blocking observations (2)

• schema_embed_test.go line 250: intermediate JSON unmarshal into var doc any is dead code — CompileSchema already validates JSON syntax internally.
• check-stale-schema-binary.sh line 354: local fallback misses brand-new untracked schema files; git diff --name-only HEAD only covers tracked changes.

🔎 Code quality review by PR Code Quality Reviewer · 54.4 AIC · ⌖ 4.91 AIC · ⊞ 5.6K
_{Comment /review to run again}

Comments that could not be inline-anchored

scripts/check-stale-schema-binary.sh:396

Stale-binary mtime check is a no-op in CI: after a fresh git checkout, schema files and the binary share the same checkout timestamp, so [ "$f" -nt "$BINARY" ] is always false — the check silently passes even when schemas are stale.

<details>
<summary>💡 Details and suggested fix</summary>

The script acknowledges this limitation on lines 389–391 and defers to CI

scripts/check-stale-schema-binary.sh:347

Shallow-clone silent pass: git diff BASE...HEAD returns empty output (or errors) on shallow clones, and the || true swallows the error — so the check silently reports no modified schemas when it actually can't determine anything.

<details>
<summary>💡 Details</summary>

CI runners commonly use --depth=1 or similar shallow fetches. When the merge base between BASE_REF and HEAD doesn't exist locally, git diff BASE...HEAD either errors or returns empty. Because the result is piped…

scripts/check-stale-schema-binary.sh:354

Local fallback misses new untracked schema files: git diff --name-only HEAD only lists modified tracked files; a brand-new schema file that hasn't been git added yet won't appear, so the check passes even though the binary can't embed a file it doesn't know about.

<details>
<summary>💡 Suggested fix</summary>

Supplement with untracked file detection for the schemas directory:

# In addition to git diff HEAD, also check untracked files in the schemas dir
git ls-files --others -…

</details>

<details><summary>pkg/parser/schema_embed_test.go:250</summary>

**Redundant JSON unmarshal before CompileSchema**: step 2 unmarshals into `var doc any` to validate JSON, but `CompileSchema` already parses the JSON internally — the `doc` variable is never used after assignment (dead code).

&lt;details&gt;
&lt;summary&gt;💡 Suggested fix&lt;/summary&gt;

Remove the intermediate unmarshal and collapse to two steps:

```go
// 1. Non-empty check
if len(tc.schemaVar) == 0 { ... }

// 2. Compile (validates both JSON syntax and schema semantics)
if _, err := CompileSchema(tc.schema…

</details>

<details><summary>Makefile:176</summary>

**`make lint` now requires the binary to exist**: adding `check-stale-schema-binary` to the `lint` target means `make lint` fails with a confusing &quot;binary not found&quot; error if a developer hasn&#39;t run `make build` first — that&#39;s unexpected for a target that should be runnable any time.

&lt;details&gt;
&lt;summary&gt;💡 Suggested fix&lt;/summary&gt;

Either:
1. Make the script exit 0 with a warning (not error) when the binary doesn&#39;t exist and no schema files are modified — the rebuild guard is moot if nothing chan…

</details>

[github-actions]

Skills-Based Review 🧠

Applied /tdd and /diagnosing-bugs — the guardrail is well-structured and fills a real gap. Three issues worth addressing before merge.

📋 Key Themes & Highlights

Issues Found

• Placeholder schema URLs ((contoso.com/redacted) in the test table may cause ID mismatches if schemas declare a real $id` — see inline comment on line 22.
• Silent CI pass-through: the mtime staleness check in base-ref mode can silently pass because git checkout does not preserve file timestamps — see inline comment on line 396 of the script.
• Empty schema not caught: valid JSON {} compiles as a vacuous schema and would pass the current test — see inline comment on line 56.

Positive Highlights

• ✅ Clean parallel structure mirroring check-stale-lock-files.sh — immediately navigable.
• ✅ Wired into make lint (and thus agent-report-progress) — the check runs automatically.
• ✅ Table-driven, parallel test with clear Fatalf messages pointing to make build.
• ✅ Color/NO_COLOR/TERM=dumb handling is correct and CI-friendly.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 49.8 AIC · ⌖ 4.77 AIC · ⊞ 6.7K
_{Comment /matt to run again}

Comments that could not be inline-anchored

pkg/parser/schema_embed_test.go:22

[/tdd] The schemaURL values use the placeholder domain (contoso.com/redacted) — if any of the embedded schemas declare a $idfield pointing to a different URL, the compiler may silently register them under two different IDs, making$ref` resolution behave differently in the test than in production.

<details>
<summary>💡 Suggestion</summary>

Read the $id field from each schema file and use that as the schemaURL, or at minimum assert that the compiled schema is registered under th…

scripts/check-stale-schema-binary.sh:396

[/diagnosing-bugs] The mtime-based staleness check (line 396: if [ "$f" -nt "$BINARY" ]) is documented as unreliable in CI because git checkout does not preserve timestamps. However, the collect_modified_files function in base-ref mode uses git diff --name-only rather than mtime — so in CI the mtime path is never reached when a base-ref is available. If --base-ref is absent in CI, the script falls back to git diff HEAD for file discovery but still uses mtimes to decide sta…

pkg/parser/schema_embed_test.go:56

[/tdd] The test validates that each schema is valid JSON and compiles, but does not assert structural correctness (e.g. required top-level keywords like "$schema", "type", or "properties"). A schema file that is syntactically valid JSON but semantically empty (e.g. {}) would pass this test yet silently disable all validation.

<details>
<summary>💡 Suggestion</summary>

Add a minimal structural assertion for each schema — at least check that the top-level object contains "$schema"…

doned

[pelikhan]

@copilot run pr-finisher skill

[Copilot]

@copilot run pr-finisher skill

Addressed all four review threads in commit fix: address review comments in check-stale-schema-binary.sh and DEVGUIDE.md:

1. || true on three-dot git diff — removed; failures now propagate so CI cannot silently bypass the check on shallow checkouts.
2. Untracked schema files in local mode — git ls-files --others --exclude-standard now appended to the local-path collection so newly added schemas are caught.
3. Deleted schema paths — when a file no longer exists the loop falls back to the schemas directory mtime as a proxy, and error messaging now refers to "schema changes" rather than "files newer than the binary."
4. DEVGUIDE.md go test phrasing — clarified that go test / make test-unit validates schema JSON but does not rebuild ./gh-aw; make build is still required for the running CLI to reflect edits.

Local make fmt, make lint, make test-unit, and make agent-report-progress all pass. CI re-trigger needed before merge.

[gh-aw-bot]

@copilot please run the pr-finisher skill address the unresolved review feedback: copilot-pull-request-reviewer: #46189 (comment); copilot-pull-request-reviewer: #46189 (comment); copilot-pull-request-reviewer: #46189 (comment) and rerun checks once the branch is up to date.

Generated by 👨‍🍳 PR Sous Chef · 11.8 AIC · ⌖ 6.81 AIC · ⊞ 7.5K · ◷
_{Comment /souschef to run again}

[Copilot]

@copilot please run the pr-finisher skill address the unresolved review feedback: copilot-pull-request-reviewer: #46189 (comment); copilot-pull-reque...

All three unresolved threads were addressed in ab82ea4. The branch has now been merged up to origin/main — make fmt, make lint, make test-unit, and make agent-report-progress all pass clean on the updated branch. CI re-trigger needed before merge.