Retain firewall and MCP observability artifacts#51153
Conversation
Triage ResultCategory: bug · Risk: low · Priority: low (score 20/100 — impact 8, urgency 5, quality 7) Notes: Early-stage WIP (0 diff yet), draft, agent still forming a plan for observability artifact retention fix. CI: copilot check in progress. Revisit once implementation lands.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds dedicated MCP and firewall observability artifacts so post-run telemetry remains independently downloadable.
Changes:
- Generates
mcp-logsand conditionalfirewall-audit-logsuploads. - Adds constants, compiler tests, and golden coverage.
- Recompiles generated workflow lock files.
Show a summary per file
| File | Description |
|---|---|
pkg/constants/constants.go |
Defines observability artifact names. |
pkg/workflow/compiler_yaml_artifacts.go |
Generates dedicated artifact uploads. |
pkg/workflow/compiler_yaml_post_agent.go |
Adds uploads to post-agent processing. |
pkg/workflow/compiler_artifacts_test.go |
Tests firewall artifact generation. |
pkg/workflow/mcp_logs_upload_test.go |
Tests MCP artifact generation. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/{claude,codex,copilot,gemini,pi}.golden |
Updates engine golden outputs. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/{basic-copilot,playwright-cli-mode,smoke-copilot,with-imports}.golden |
Updates fixture golden outputs. |
.github/workflows/*.lock.yml |
Recompiles generated workflows with dedicated observability uploads. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 290/290 changed files
- Comments generated: 1
- Review effort level: Balanced
| // Generate dedicated firewall/MCP observability artifact uploads so security | ||
| // reviews and debugging tools can fetch these logs directly even when they do | ||
| // not download the full agent artifact. | ||
| c.generateObservabilityArtifactUploads(yaml, data, agentArtifactPrefix) |
|
No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
|
|
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.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 59.4 AIC · ⊞ 5.5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on two issues.
📋 Key Themes & Highlights
Issues
- CLI artifact-set mismatch:
ArtifactSetMCPandArtifactSetFirewallinlogs_artifact_set.gostill map to theagentartifact. The new dedicatedmcp-logsandfirewall-audit-logsartifacts won't be fetched bygh aw logs --artifacts mcp/firewall. - Missing negative test: No test verifies that
Upload firewall observability logsis absent when the firewall is disabled (nonetwork:block). This is a correctness guard that should accompany the feature.
Positive Highlights
- ✅ Clean extraction of
generateObservabilityArtifactUploadsandgenerateDedicatedArtifactUpload— well-factored helpers. - ✅
continue-on-error: true+if-no-files-found: ignoreis the right defensive posture for optional observability logs. - ✅ ARC/DinD topology awareness for path resolution is consistently handled.
- ✅ Good regression test coverage for the happy path in both
compiler_artifacts_test.goandmcp_logs_upload_test.go. - ✅ Constants
MCPLogsArtifactNameandFirewallAuditArtifactNamecentralised inconstants.go.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 48.5 AIC · ⊞ 7.1K
Comment /matt to run again
| @@ -317,6 +317,63 @@ This workflow should generate a unified artifact upload step that includes the p | |||
| t.Log("Unified artifact upload step verified successfully (includes prompt)") | |||
There was a problem hiding this comment.
[/tdd] Missing test for the firewall-disabled case: when no network: block is present, Upload firewall observability logs should NOT appear in the compiled YAML.
Without this, a regression that unconditionally emits the firewall step would go undetected.
💡 Suggested test outline
func TestNoFirewallObservabilityUploadWhenFirewallDisabled(t *testing.T) {
// compile a workflow with no 'network:' block
// assert lockYAML does NOT contain 'Upload firewall observability logs'
// assert lockYAML DOES contain 'Upload MCP observability logs'
}@copilot please address this.
…ifact retention Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate — ADR RequiredThis PR makes significant changes to CI/CD observability infrastructure (>100 new lines in tracked directories) but does not have a linked Architecture Decision Record (ADR). Draft ADR committed: This PR cannot merge until an ADR is linked in the PR body. 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. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
Recent workflow runs had firewall and MCP runtime topology active, but most did not retain the key post-run evidence: firewall
access.logand MCP JSONL telemetry. That made egress auditing and tool-call reconstruction unreliable after the run completed.Artifact retention
mcp-logsartifact uploads for MCP gateway telemetry.firewall-audit-logsartifact uploads for firewall proxy logs, audit logs, AWF reflect data, and AWF config.agentartifact unchanged.Generated workflow output