Add body hash to lock metadata; support on.stale-check: full#34941
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…lper Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…xtBased, compareBodyHashes Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…rkflowHash helper, destructured options Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot body_hash should be a single hash string opaque |
…apper) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in the latest commit. |
There was a problem hiding this comment.
Pull request overview
This PR upgrades workflow lock metadata to schema v4 by adding a body_hash (in addition to frontmatter_hash) so prompt-body and imported-file changes are detectable, and introduces on.stale-check: full to enable runtime verification of both hashes via an activation-step environment flag.
Changes:
- Add
body_hashsupport to lock metadata (schema v4) and update lock schema generation inputs to use a struct (LockHashInfo). - Add
on.stale-check: fullsupport in compilation flow and injectGH_AW_STALE_CHECK_FULL=trueinto the activation job when enabled. - Update the main workflow JSON schema and regenerate existing
.lock.ymlfiles to include v4 metadata.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/workflow_builder.go | Parses on.stale-check: full into WorkflowData.StaleCheckFull. |
| pkg/workflow/lock_schema.go | Introduces lock schema v4 and body_hash in LockMetadata; refactors lock metadata generation to accept LockHashInfo. |
| pkg/workflow/compiler_yaml_helpers_test.go | Updates tests for the expanded generateWorkflowHeader signature (now includes bodyHash). |
| pkg/workflow/compiler_types.go | Adds WorkflowData.StaleCheckFull flag used during activation-job generation. |
| pkg/workflow/compiler_activation_job_builder.go | Injects GH_AW_STALE_CHECK_FULL=true when full stale-check mode is enabled. |
| pkg/parser/schemas/main_workflow_schema.json | Expands on.stale-check schema to accept `boolean |
| .github/workflows/*.lock.yml | Regenerates workflow lock files to schema v4 and embeds body_hash in the gh-aw-metadata header. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 251/251 changed files
- Comments generated: 1
| // Populate stale-check flag: disabled when on.stale-check: false is set in frontmatter; | ||
| // full mode when on.stale-check: full is set. | ||
| if onVal, ok := result.Frontmatter["on"]; ok { | ||
| if onMap, ok := onVal.(map[string]any); ok { | ||
| if staleCheck, ok := onMap["stale-check"]; ok { | ||
| if boolVal, ok := staleCheck.(bool); ok && !boolVal { | ||
| workflowData.StaleCheckDisabled = true | ||
| } else if strVal, ok := staleCheck.(string); ok && strVal == "full" { | ||
| workflowData.StaleCheckFull = true |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
|
|
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (398 new lines across 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in References: §26458153802
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 87/100 — Excellent
📊 Metrics & Test Classification (21 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — approving with minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Cross-language hash consistency: Go and JS implementations match algorithm exactly (sorted imports,
\n---\nseparator, SHA-256). No blocking issues found. - Test coverage: Comprehensive — unit tests for hash computation, integration tests for stale-check modes, backward compat tests for pre-v4 lock files.
- Schema evolution: Clean v3→v4 bump with
omitemptyfor backward compat.LockHashInfostruct is a good pattern.
Positive Highlights
- ✅ Graceful degradation: body hash failure is non-fatal at compile time
- ✅ Backward compat: old lock files without
body_hashwork seamlessly - ✅ Skip-write optimization correctly updated to include body hash
- ✅ Tests cover both API and local filesystem paths for runtime verification
Minor Suggestions (Non-blocking)
- Consider adding a golden parity test to verify Go/JS produce identical hashes for a fixed input
- The warning log when body hash fails could be more explicit about continuing without it
- Note: Existing review flagged the PR description showing
stale-check: fullat top-level vs.on.stale-check: fullin implementation
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · opus45 6.3M
| allParts = append(allParts, normalizedBodies...) | ||
| } | ||
|
|
||
| combined := strings.Join(allParts, "\n---\n") |
There was a problem hiding this comment.
[/tdd] Cross-language hash parity: The join separator \n---\n is correct and matches JS, but no explicit test verifies Go/JS parity on identical inputs.
💡 Suggested addition
Consider adding a golden test that asserts Go and JS produce the same hash for a fixed workflow file. A simple approach:
// pkg/workflow/body_hash_parity_test.go
func TestBodyHashParityWithJS(t *testing.T) {
workflowMD := `---\nengine: copilot\n---\n\n# Test Workflow`
// Pre-compute expected hash from JS once and embed it
expectedHash := "<computed from JS>" // run JS once, store here
actualHash, _ := parser.ComputeBodyHashFromParsedContent(...)
assert.Equal(t, expectedHash, actualHash)
}This guards against subtle Unicode normalisation or encoding divergence.
| func GenerateLockMetadata(frontmatterHash string, stopTime string, strict bool, agentInfo AgentMetadataInfo) *LockMetadata { | ||
| lockSchemaLog.Printf("Generating lock metadata: schema=%s, strict=%t, hasStopTime=%t", LockSchemaV3, strict, stopTime != "") | ||
| func GenerateLockMetadata(hashInfo LockHashInfo, stopTime string, strict bool, agentInfo AgentMetadataInfo) *LockMetadata { | ||
| lockSchemaLog.Printf("Generating lock metadata: schema=%s, strict=%t, hasStopTime=%t, hasBodyHash=%t", LockSchemaV4, strict, stopTime != "", hashInfo.BodyHash != "") |
There was a problem hiding this comment.
[/grill-with-docs] Schema versioning and backward compat: Good — the omitempty on BodyHash ensures pre-v4 lock files remain valid.
💡 Consideration for future schema changes
LockHashInfo is a nice pattern for grouping related fields. If future versions add more hash-like fields (e.g., importsHash), this struct scales cleanly.
Consider documenting the "schema version changelog" pattern somewhere (perhaps in lock_schema.go header comments or scratchpad/) so future contributors know when to bump the version vs. add optional fields.
| } else { | ||
| bodyHash = bHash | ||
| compilerYamlLog.Printf("Computed body hash: %s", bodyHash) | ||
| } |
There was a problem hiding this comment.
[/tdd] Good graceful degradation on body hash failure.
💡 Suggestion: Add warning log context
The warning logs could not compute body hash but doesn't show the BodyHash field ends up empty. Consider adding:
if bErr != nil {
compilerYamlLog.Printf("Warning: could not compute body hash for %q: %v (continuing without body hash)", markdownPath, bErr)
} else {
bodyHash = bHash
compilerYamlLog.Printf("Computed body hash: %s", bodyHash)
}This makes it clearer in debug logs that body hash is intentionally omitted vs. missing due to oversight.
| core.info(`No body hash found in lock file; skipping body hash check${suffix} (lock file may predate body hash support)`); | ||
| return true; | ||
| } | ||
| const recomputedBodyHash = await computeBodyHash(mdPath, { fileReader }); |
There was a problem hiding this comment.
[/tdd] Good test coverage for the compareBodyHashes helper — all paths covered.
✅ Test verification
Tests in check_workflow_timestamp_api.test.cjs cover:
- Both hashes match → pass
- Frontmatter matches but body differs → fail
- Old lock file without body hash → graceful skip
- Full mode disabled → body hash ignored
- Local filesystem fallback path
The backward compat case (no body_hash in lock) is correctly handled by returning true early.
|
🚀 Smoke Pi MISSION COMPLETE! Pi delivered. 🥧 |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
Smoke Test: Gemini - 26468168663
Overall Status: FAIL Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
|
Smoke test #26468168621: FAIL
Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
|
@copilot merge main and recompile |
|
Smoke Copilot 26468168604: FAIL Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Smoke review completed. Inline notes are informational only.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · gpt55 8.6M
| - **`stale-check:`** - Control whether the activation job verifies the frontmatter hash matches the compiled workflow (boolean, default: `true`) | ||
| - **`stale-check:`** - Control whether the activation job verifies hashes against the compiled workflow (boolean or `"full"`, default: `true`) | ||
| - When `false`, disables the hash check step; useful when workflow files are managed outside the default repository context (e.g., cross-repo org rulesets) | ||
| - When `"full"`, checks both the frontmatter hash and body hash; use when prompt-body edits should also trigger recompilation detection |
There was a problem hiding this comment.
Smoke note: full mode docs now say body hash too. Me see clear user signal.
| @@ -1,4 +1,4 @@ | |||
| # gh-aw-metadata: {"schema_version":"v3","frontmatter_hash":"36cdbbbe4882806612f8dad2f77146ac361dff5ec231ef9eba66ffb8e0d02ca1","strict":true,"agent_id":"copilot"} | |||
| # gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"36cdbbbe4882806612f8dad2f77146ac361dff5ec231ef9eba66ffb8e0d02ca1","body_hash":"c0c6b51ff748c32142fa383ae68e04752e079d02bf22356046bf39761521b93c","strict":true,"agent_id":"copilot"} | |||
There was a problem hiding this comment.
Smoke note: lock metadata moved to schema v4 and carries body_hash. Generated lock looks aligned.
|
📰 DEVELOPING STORY: Smoke Copilot reports failed to deliver outputs. Our correspondents are investigating the incident... |
🤖 Smoke Test: Claude — Run 26468168627Core (1–13): ✅ all passed Overall: Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
💥 [THE END] — Illustrated by Smoke Claude · opus47 14.3M
| if staleCheck, ok := onMap["stale-check"]; ok { | ||
| if boolVal, ok := staleCheck.(bool); ok && !boolVal { | ||
| workflowData.StaleCheckDisabled = true | ||
| } else if strVal, ok := staleCheck.(string); ok && strVal == "full" { |
There was a problem hiding this comment.
💥 Smoke-test review: comment on the stale-check parsing branch. (Run 26468168627)
| workflowData.StaleCheckFull = true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💥 Smoke-test review #2: consider documenting the StaleCheckFull semantics. (Run 26468168627)
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…-include-body # Conflicts: # .github/workflows/daily-cache-strategy-analyzer.lock.yml # .github/workflows/daily-fact.lock.yml # .github/workflows/daily-observability-report.lock.yml # .github/workflows/dev.lock.yml # .github/workflows/duplicate-code-detector.lock.yml # .github/workflows/grumpy-reviewer.lock.yml # .github/workflows/issue-arborist.lock.yml # .github/workflows/necromancer.lock.yml # .github/workflows/schema-feature-coverage.lock.yml # .github/workflows/smoke-call-workflow.lock.yml # .github/workflows/smoke-codex.lock.yml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Merged |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed — the failure was a Prettier formatting issue in |
Lock files only hashed the workflow frontmatter, so prompt-body edits could silently drift from the compiled lock without any detection signal. This adds a
body_hashfield (covering the markdown body + all imported files) to lock metadata and introduceson.stale-check: fullto enable runtime body hash verification.Lock metadata (schema v4)
Every compiled
.lock.ymlnow includes abody_hashalongside the existingfrontmatter_hash:# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"<hex>","body_hash":"<hex>",...}The body hash is an opaque SHA-256 string computed by concatenating the normalized body text and all transitively imported body texts (sorted, joined with
\n---\n) and hashing the result directly — no JSON wrapper.Old lock files without
body_hashremain valid (field isomitempty).on.stale-check: full
Opt into body hash verification at runtime by setting
stale-check: fullin theon:section of the workflow frontmatter:This injects
GH_AW_STALE_CHECK_FULL=trueinto the "Check workflow lock file" activation step. When set, the runtime JS checker verifies both hashes after the frontmatter check passes — on both the GitHub API path and the local filesystem fallback path.Key changes
Go (compile-time)
pkg/parser/frontmatter_hash.go—collectImportedBodies,ComputeBodyHashFromParsedContent,ComputeBodyHashFromFilepkg/workflow/lock_schema.go—LockSchemaV4,BodyHashfield onLockMetadata,LockHashInfostruct replaces positional hash params inGenerateLockMetadatapkg/workflow/compiler_yaml.go— computes body hash at compile time;computeWorkflowHashclosure deduplicates the parsed-content/file-fallback pattern used by both hash computations; body hash failure is non-fatalpkg/workflow/workflow_builder.go/compiler_activation_job_builder.go/compiler_types.go—on.stale-check: full→StaleCheckFullflag →GH_AW_STALE_CHECK_FULL=trueenv injectionpkg/parser/schemas/main_workflow_schema.json—stale-check(underon) now acceptsboolean | "full"JS (runtime)
frontmatter_hash_pure.cjs—collectImportedBodies,computeBodyHash,extractBodyHashFromLockFilecheck_workflow_timestamp_api.cjs—compareBodyHasheshelper (destructured{ fileReader }, gracefully skips on pre-v4 lock files); called on both API and local filesystem paths when full mode is activeBehaviour change for body edits
The compiler's skip-write optimization (preserve lock file timestamp when content is unchanged) now correctly triggers on body-only edits — a body change updates
body_hashin the metadata, producing different lock file content and causing a rewrite.TestCompilerWritesWhenBodyContentChangedcovers this.