Prevent raw observability log loss during artifact extraction - #51907
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review. 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. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #51907 does not have the 'implementation' label and has only 71 new lines of code in business logic directories (threshold: 100).
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
There was a problem hiding this comment.
Pull request overview
Isolates artifact extraction to prevent filename collisions from dropping observability logs.
Changes:
- Extracts each artifact into a dedicated directory.
- Validates artifact names against unsafe paths.
- Adds collision and validation regression coverage.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/logs_download_artifacts.go |
Adds isolated extraction and name validation. |
pkg/cli/logs_artifact_set.go |
Reuses centralized validation for markers. |
pkg/cli/logs_download_test.go |
Tests isolated downloads and updates fixtures. |
pkg/cli/logs_artifact_set_test.go |
Expands unsafe-name tests. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Balanced
| if err := validateArtifactName(name); err != nil { | ||
| return err | ||
| } | ||
| artifactDir := filepath.Join(opts.outputDir, name) |
There was a problem hiding this comment.
Fixed in 2e94fd8: named artifacts now download into temporary sibling staging directories and are promoted to the artifact directory only after gh run download succeeds, so failed extractions cannot satisfy cache checks.
| artifactDir := filepath.Join(opts.outputDir, name) | ||
| args := []string{"run", "download", strconv.FormatInt(opts.runID, 10), "--name", name, "--dir", artifactDir} |
There was a problem hiding this comment.
Fixed in 2e94fd8: usage-only downloads now copy usage/aw_info.json to the run root before attempting activation fallback, preserving existing metadata layout and avoiding unnecessary fallback downloads.
| err := downloadArtifactsByName( | ||
| context.Background(), | ||
| downloadArtifactsOptions{runID: 12345, outputDir: outputDir}, | ||
| []string{"usage", "agent"}, | ||
| ) |
There was a problem hiding this comment.
Fixed in 2e94fd8: the regression now exercises downloadRunArtifacts end-to-end and asserts the final flattened mcp-logs/rpc-messages.jsonl and sandbox/firewall/logs/access.log paths survive overlapping artifacts.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Prevent raw observability log loss during artifact extraction
This PR correctly addresses two issues:
-
Artifact isolation — Each artifact now extracts into
outputDir/<name>/instead of the sharedoutputDir/, preventing cross-artifact file collisions. -
Path traversal hardening —
validateArtifactNameconsolidates and strengthens the existing check by also rejecting.,.., and names containing forward/backslashes, beforefilepath.Basenormalization catches them. -
Test fixture alignment — Existing fake-gh shell scripts updated from
$dir/$name/...to$dir/...to match the new isolation layout.
No blocking issues found. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 24.5 AIC · ⌖ 6.16 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — two low-severity observations; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Isolation fix is correct: downloading each artifact into
opts.outputDir/<name>eliminates the collision root cause cleanly.flattenUnifiedArtifact→findArtifactDirfindsoutputDir/agentas before, so the existing flatten pipeline still works. validateArtifactNameplacement: called indownloadArtifactsByNamebut not inretryCriticalArtifacts(inline comment filed). Low risk since names come from constants, but asymmetric.- Regression test is solid: the fake-
ghscript inTestDownloadArtifactsByName_IsolatesArtifactExtractiondirectly models the collision scenario and asserts nested files survive — this is exactly the right test for the bug. - Existing test updates: the
TestDownloadRunArtifacts_CachedUsageFallbackToActivationandTestDownloadRunArtifactsFallbackWhenListFailsscript adjustments correctly mirror the new--dirsemantics (no subdirectory creation inside the script).
Positive Highlights
- ✅
validateArtifactNameextracted as a shared helper — DRY and testable - ✅ Traversal-safety hardened:
strings.ContainsAny(name, "/\\")+filepath.Basedouble-check - ✅ New test covers nested
mcp-logs/andsandbox/firewall/logs/paths — the exact files that were being lost
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 41.8 AIC · ⌖ 6.67 AIC · ⊞ 7.1K
Comment /matt to run again
| } | ||
|
|
||
| retryArgs := []string{"run", "download", strconv.FormatInt(opts.runID, 10), "--name", name, "--dir", opts.outputDir} | ||
| retryArgs := []string{"run", "download", strconv.FormatInt(opts.runID, 10), "--name", name, "--dir", filepath.Join(opts.outputDir, name)} |
There was a problem hiding this comment.
[/diagnosing-bugs] retryCriticalArtifacts skips validateArtifactName even though it constructs a filepath.Join path from name. Since criticalArtifactNames are compile-time constants this is safe today, but the asymmetry is a maintenance trap.
💡 Suggestion
Add validation at the top of the loop, mirroring downloadArtifactsByName:
if err := validateArtifactName(name); err != nil {
logsDownloadLog.Printf("Skipping invalid critical artifact name: %v", err)
continue
}@copilot please address this.
There was a problem hiding this comment.
Fixed in 2e94fd8: retryCriticalArtifacts now validates critical artifact names before constructing staging paths.
| assert.FileExists(t, filepath.Join(outputDir, "agent", "sandbox", "firewall", "logs", "access.log")) | ||
| } | ||
|
|
||
| func TestDownloadRunArtifacts_CachedUsageFallbackToActivation(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] The fake gh script exits 1 on a collision (if [ -e "$dir/shared.json" ]), but the test never asserts that no error occurred after downloading both usage and agent — it just calls require.NoError. If the isolation fix regresses, the script collision would surface as a test failure but the error message would be opaque (exit status 1 with no artifact context).
💡 Suggestion
Capture and log the fake-script stderr, or add a comment explaining that require.NoError on line 376 IS the regression gate, so future readers understand the test structure at a glance.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 2e94fd8: the test now documents the require.NoError regression gate and covers the production download-and-flatten path.
🧪 Test Quality Sentinel Report
📊 Metrics
📋 Test Classifications🟢 HIGH-VALUE Design Tests (Behavioral Contracts)
🔵 IMPLEMENTATION Tests (Table-Driven, Comprehensive)
⚪ SUPPORT Tests (Infrastructure, Constants)
✅ Strengths
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 52/100 (Acceptable). Design tests: 5% (2 critical). Implementation tests: 75% (threshold: 30%). Edge case coverage: Comprehensive (collisions, traversal, invalid names). Violations: 0. The core behavioral test TestDownloadArtifactsByName_IsolatesArtifactExtraction directly validates the fix for issue #51813 with realistic scenarios. Approved.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot quick triage: this PR looks close, but it still has a recent Matt skills review with two low-severity follow-ups and a fresh maintainer request to run the pr-finisher skill. Please review the latest feedback, decide whether the Branch refresh was requested. Run: https://github.com/github/gh-aw/actions/runs/31456898570
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Firewall and MCP raw logs were present in the unified
agentartifact but disappeared during filtered downloads. Overlapping files fromusage,agent, anddetectionartifacts caused extraction to abort before nestedaccess.logandrpc-messages.jsonlfiles were reached.Isolate extraction
Harden artifact names
Add regression coverage