Add frontmatter source/hash/body-modified metadata to OTEL spans#32196
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot remove gh-aw.frontmatter.source |
![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 /zoom-out — this is a new-feature PR adding OTEL span attributes, so TDD coverage gaps and architectural consistency are the most important lenses.
Key Themes
-
Code duplication (
send_otlp_span.cjs): The 6-line extraction + 3-lineattributes.pushblock for frontmatter fields is copy-pasted identically in bothsendJobSetupSpanandsendJobConclusionSpan. AbuildFrontmatterAttributes(awInfo)helper would eliminate the duplication and give the logic a testable, named home. -
Missing edge-case tests (
generate_aw_info.test.cjs): The new test only exercisesbody_modified: true. The"false"case and the absence cases (empty env var → field not written) are untested. These are exactly the cases that catch regressions. -
Coupling asymmetry in compiler (
compiler_yaml.go):FRONTMATTER_SOURCEandBODY_MODIFIEDshare one guard, butFRONTMATTER_HASHhas its own — implying they can appear independently. This may be intentional, but it's surprising and worth a comment or collapsing into a single guard. -
Unimplemented override path (
compiler_yaml.go):BODY_MODIFIEDis always"false"at compile time; the comment says an update flow will override it, but that flow isn't visible here. Until it exists, every span emitsbody_modified: falseregardless of actual drift — silently misleading telemetry.
Positive Highlights
- ✅ Both
sendJobSetupSpanandsendJobConclusionSpanare consistently updated — no asymmetry at the span-emission level. - ✅ Tests are present for the happy path in all three layers (generate, send setup, send conclusion), which is the right instinct.
- ✅ Defensive typing (
typeof awInfo.body_modified === "boolean") for theundefinedcase onbody_modifiedis handled carefully. - ✅ The
aw_info_tmp_test.gointegration test validates the compiler-to-env-var pipeline end-to-end.
Verdict
No blocking issues — the feature works for the expected case. The inline comments flag maintainability and test-coverage improvements worth addressing before this area grows further.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 6.8M
| const itemNumber = typeof awInfo.context?.item_number === "string" ? awInfo.context.item_number : ""; | ||
| const triggerLabel = typeof awInfo.context?.trigger_label === "string" ? awInfo.context.trigger_label : ""; | ||
| const commentId = typeof awInfo.context?.comment_id === "string" ? awInfo.context.comment_id : ""; | ||
| const frontmatterSource = typeof awInfo.frontmatter_source === "string" ? awInfo.frontmatter_source : ""; |
There was a problem hiding this comment.
[/tdd] The 6 lines extracting frontmatterSource, frontmatterHash, bodyModified from awInfo (lines 927–929 and 1335–1337) and the 3 attributes.push(...) calls (lines 997–999 and 1430–1432) are duplicated identically between sendJobSetupSpan and sendJobConclusionSpan.
Consider extracting a helper:
function buildFrontmatterAttributes(awInfo) {
const attrs = [];
const source = typeof awInfo.frontmatter_source === "string" ? awInfo.frontmatter_source : "";
const hash = typeof awInfo.frontmatter_hash === "string" ? awInfo.frontmatter_hash : "";
const modified = typeof awInfo.body_modified === "boolean" ? awInfo.body_modified : undefined;
if (source) attrs.push(buildAttr("gh-aw.frontmatter.source", source));
if (hash) attrs.push(buildAttr("gh-aw.frontmatter.hash", hash));
if (typeof modified === "boolean") attrs.push(buildAttr("gh-aw.frontmatter.body_modified", modified));
return attrs;
}This is the classic deep-module opportunity from /tdd: a small interface (awInfo → attrs[]) with all the defensive-typing complexity hidden inside.
| expect(awInfo.created_at).toBeTruthy(); | ||
| }); | ||
|
|
||
| it("should include frontmatter source/hash and body_modified when configured", async () => { |
There was a problem hiding this comment.
[/tdd] The new test only verifies body_modified: true. The generate_aw_info.cjs code has a distinct code path for "false" and for invalid/absent values:
if (bodyModified === "true" || bodyModified === "false") {
awInfo.body_modified = bodyModified === "true";
}Consider adding tests for:
GH_AW_INFO_BODY_MODIFIED = "false"→awInfo.body_modified === falseGH_AW_INFO_BODY_MODIFIED = ""(empty) →awInfo.body_modifiedis absent from the JSONGH_AW_INFO_FRONTMATTER_SOURCE = ""→awInfo.frontmatter_sourceis absent
The negative/absence cases are what catch regressions when someone later changes the conditional.
| fmt.Fprintf(yaml, " GH_AW_INFO_AWMG_VERSION: \"%s\"\n", mcpGatewayVersion) | ||
| fmt.Fprintf(yaml, " GH_AW_INFO_FIREWALL_TYPE: \"%s\"\n", firewallType) | ||
| if data.Source != "" { | ||
| fmt.Fprintf(yaml, " GH_AW_INFO_FRONTMATTER_SOURCE: \"%s\"\n", data.Source) |
There was a problem hiding this comment.
[/zoom-out] There's an asymmetry here: GH_AW_INFO_FRONTMATTER_SOURCE and GH_AW_INFO_BODY_MODIFIED are guarded by data.Source != "", but GH_AW_INFO_FRONTMATTER_HASH has its own independent guard (data.FrontmatterHash != ""). This means a workflow with a hash but no source would emit hash without source or body_modified.
Is that intentional? If source and hash always come from the same frontmatter provenance, it may be clearer and less surprising to emit them together (same guard), or at least document the independent-emission intent with a comment.
The test in aw_info_tmp_test.go checks GH_AW_INFO_FRONTMATTER_SOURCE and GH_AW_INFO_FRONTMATTER_HASH but doesn't test the hash-without-source scenario.
| fmt.Fprintf(yaml, " GH_AW_INFO_FRONTMATTER_SOURCE: \"%s\"\n", data.Source) | ||
| // Body-modified defaults to false at compile time; update flows may override this | ||
| // signal when source/body drift is detected before execution. | ||
| yaml.WriteString(" GH_AW_INFO_BODY_MODIFIED: \"false\"\n") |
There was a problem hiding this comment.
[/tdd] GH_AW_INFO_BODY_MODIFIED is hardcoded to "false" at compile time with a comment that "update flows may override this signal". But there's no test — and no code visible in this PR — that exercises the override path.
If the override mechanism exists elsewhere, a cross-reference comment or link to that code would help future readers understand when the value actually becomes "true" at runtime. If the override hasn't been implemented yet, this is a [tracer-bullet]((mattpocock.com/redacted) placeholder that could silently produce misleading telemetry: body_modified: false on every span, even when the body was actually modified.
There was a problem hiding this comment.
Pull request overview
Adds frontmatter provenance/state metadata to agentic workflow OTEL telemetry, but the implementation is incomplete for setup spans and committed lockfiles.
Changes:
- Adds
frontmatter_source,frontmatter_hash, andbody_modifiedtoaw_info.json. - Emits the new fields as OTEL span attributes in setup/conclusion span code paths.
- Updates tests and regenerates many workflow lockfiles, primarily adding wazero MCP cache cleanup steps.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_yaml.go |
Adds frontmatter metadata env vars for generate_aw_info. |
pkg/workflow/aw_info_tmp_test.go |
Extends aw_info generation test expectations. |
actions/setup/js/generate_aw_info.cjs |
Persists frontmatter metadata into aw_info.json. |
actions/setup/js/generate_aw_info.test.cjs |
Tests persisted frontmatter metadata. |
actions/setup/js/send_otlp_span.cjs |
Emits frontmatter metadata as OTEL attributes. |
actions/setup/js/send_otlp_span.test.cjs |
Tests setup/conclusion span attributes. |
.github/workflows/*.lock.yml |
Regenerated workflow lockfiles, mostly adding wazero cache cleanup. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 232/232 changed files
- Comments generated: 8
| if data.Source != "" { | ||
| fmt.Fprintf(yaml, " GH_AW_INFO_FRONTMATTER_SOURCE: \"%s\"\n", data.Source) | ||
| // Body-modified defaults to false at compile time; update flows may override this | ||
| // signal when source/body drift is detected before execution. | ||
| yaml.WriteString(" GH_AW_INFO_BODY_MODIFIED: \"false\"\n") |
| fmt.Fprintf(yaml, " GH_AW_INFO_AWMG_VERSION: \"%s\"\n", mcpGatewayVersion) | ||
| fmt.Fprintf(yaml, " GH_AW_INFO_FIREWALL_TYPE: \"%s\"\n", firewallType) | ||
| if data.Source != "" { | ||
| fmt.Fprintf(yaml, " GH_AW_INFO_FRONTMATTER_SOURCE: \"%s\"\n", data.Source) |
| GATEWAY_PID: ${{ steps.start-mcp-gateway.outputs.gateway-pid }} | ||
| run: | | ||
| bash "${RUNNER_TEMP}/gh-aw/actions/stop_mcp_gateway.sh" "$GATEWAY_PID" | ||
| - name: Remove wazero MCP cache |
| GATEWAY_PID: ${{ steps.start-mcp-gateway.outputs.gateway-pid }} | ||
| run: | | ||
| bash "${RUNNER_TEMP}/gh-aw/actions/stop_mcp_gateway.sh" "$GATEWAY_PID" | ||
| - name: Remove wazero MCP cache |
| GATEWAY_PID: ${{ steps.start-mcp-gateway.outputs.gateway-pid }} | ||
| run: | | ||
| bash "${RUNNER_TEMP}/gh-aw/actions/stop_mcp_gateway.sh" "$GATEWAY_PID" | ||
| - name: Remove wazero MCP cache |
| GATEWAY_PID: ${{ steps.start-mcp-gateway.outputs.gateway-pid }} | ||
| run: | | ||
| bash "${RUNNER_TEMP}/gh-aw/actions/stop_mcp_gateway.sh" "$GATEWAY_PID" | ||
| - name: Remove wazero MCP cache |
| GATEWAY_PID: ${{ steps.start-mcp-gateway.outputs.gateway-pid }} | ||
| run: | | ||
| bash "${RUNNER_TEMP}/gh-aw/actions/stop_mcp_gateway.sh" "$GATEWAY_PID" | ||
| - name: Remove wazero MCP cache |
| GATEWAY_PID: ${{ steps.start-mcp-gateway.outputs.gateway-pid }} | ||
| run: | | ||
| bash "${RUNNER_TEMP}/gh-aw/actions/stop_mcp_gateway.sh" "$GATEWAY_PID" | ||
| - name: Remove wazero MCP cache |
🧪 Test Quality Sentinel ReportTest Quality Score: 85/100✅ Excellent test quality
Test Classification DetailsView All Test Classifications (4 tests)
Flagged Tests — Minor SuggestionsNo tests fail the quality bar. The following are low-priority suggestions:
|
|
|
|
@copilot mrerge main recompile review all comments abd reviews |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in |
|
pr-sous-chef: please confirm the refreshed branch is green and summarize any remaining blockers.
|
|
…ata-fields # Conflicts: # .github/workflows/daily-cache-strategy-analyzer.lock.yml # .github/workflows/daily-fact.lock.yml # .github/workflows/daily-observability-report.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 in two commits:
|
✨ Enhancement
OTEL payloads did not include workflow frontmatter provenance/state needed for update analysis. This change adds
frontmatter_source(when present),frontmatter_hash, andbody_modifiedso traces can distinguish source drift from frontmatter drift.What does this improve?
gh-aw.frontmatter.sourcegh-aw.frontmatter.hashgh-aw.frontmatter.body_modifiedWhy is this valuable?
Implementation approach:
generate_aw_infoenv:GH_AW_INFO_FRONTMATTER_SOURCE(conditional)GH_AW_INFO_FRONTMATTER_HASH(conditional)GH_AW_INFO_BODY_MODIFIED(defaultfalsefor source-backed workflows)/tmp/gh-aw/aw_info.jsonas:frontmatter_source,frontmatter_hash,body_modified{ "frontmatter_source": "owner/repo/.github/workflows/example.md@main", "frontmatter_hash": "abc123def456...", "body_modified": false }