Repository navigation
Add experimental sessions download with legacy run reconstruction - #65263
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (>100 new lines) but does not have a linked Architecture Decision Record (ADR). 📄 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. 🔎 Evidence used
❓ 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
|
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
This adds useful session export plumbing, but two correctness regressions make it unsafe to merge as-is: audit no longer honors behavior-defined engine log parsers, and the legacy metadata matcher can misclassify *-aw-info as *-info.
Blocking themes
- Behavior-defined engines now fall back to the generic custom parser during audit/session parsing, which can drop structured findings from existing workflows.
- The artifact-name matcher is too broad for
info, so mixed legacy/current metadata layouts can resolve ambiguously or fail even though this command is meant to support them.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 81.5 AIC · ⌖ 7.35 AIC · ⊞ 20.2K
Comment /review to run again
| cmd := exec.Command("node", "parser.js") | ||
| cmd.Dir = tempDir | ||
| output, err := cmd.CombinedOutput() | ||
| output, err := runSessionParser(context.Background(), "agent-markdown", agentLogPath, engine.GetID()) |
There was a problem hiding this comment.
This refactor drops behavior-defined engines’ custom log parsers, so gh aw audit can silently lose structured findings for any workflow that relies on a generated parser.
💡 Why this blocks merge
Before this change, audit keyed off engine.GetLogParserScriptId(), which is how behavior-defined engines surface their generated <engine>_log_parser.cjs parser. The new path passes only engine.GetID() into runSessionParser(), and session_cli.cjs only knows the built-in engine IDs; anything else falls back to parse_custom_log.cjs.
That means a custom engine with a real parser now degrades from its purpose-built parser to the generic heuristic parser, which can drop log entries, MCP failures, and max-turn detection from audit output even though the run itself produced the richer parser.
Please preserve the parser-script path for behavior-defined engines here — for example by materializing the engine-specific parser alongside the bundled runtime sources, or by teaching the session CLI to dispatch on GetLogParserScriptId() instead of only GetID().
There was a problem hiding this comment.
Fixed in acf1182: audit now materializes and invokes a behavior-defined engine’s generated parser source, preserving its structured entries and surfacing MCP failures and max-turn detection. Added regression coverage.
| } | ||
| var matches []string | ||
| for _, name := range names { | ||
| if strings.HasSuffix(name, "-"+base) || (legacy != "" && strings.HasSuffix(name, "-"+legacy)) { |
There was a problem hiding this comment.
The -<base> suffix match is too broad for info: it treats *-aw-info as an info artifact, which breaks the legacy/current metadata fallback this command is trying to support.
💡 Why this blocks merge
sessionEngine() looks for info before aw-info, but sessionArtifactName() matches any artifact whose name ends with -info. A workflow-call artifact like caller-aw-info therefore matches the generic info branch first, and a run that contains both caller-info and caller-aw-info will now surface as "multiple info artifacts" even though those are two different compatibility buckets.
That is exactly the mixed historical layout this feature claims to support, so the current matching can reject valid legacy runs or pick the wrong metadata source. The fix should make the workflow-call prefix match exact for the whole artifact name, or special-case info so aw-info and aw_info are resolved only by the legacy branch.
There was a problem hiding this comment.
Fixed in acf1182: workflow-call *-aw-info and *-aw_info names are excluded from generic info matching and handled by the legacy metadata bucket. Added a mixed-layout regression test.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
File output currently gives sensitive full-session JSONL world-readable permissions.
Review effort: Balanced
Findings: 1
What changed in this PR
Adds experimental session export with legacy run reconstruction and shared audit/session parsers.
Changes:
- Adds JSONL/Markdown session downloads with artifact fallback and validation.
- Embeds shared Node.js parsers and reuses them for audit rendering.
- Adds comprehensive tests, documentation, CLI wiring, and a changeset.
| File | Description |
|---|---|
.changeset/minor-sessions-download.md |
Records the minor feature addition. |
actions/setup/js/session_cli.cjs |
Implements reconstruction and rendering adapter. |
actions/setup/js/session_cli.test.cjs |
Tests adapter behavior and validation. |
actions/setup/js/unified_session.cjs |
Corrects the collector return type. |
actions/setup/session_parsers.go |
Embeds runtime parser sources. |
cmd/gh-aw/main.go |
Registers the sessions command. |
docs/src/content/docs/setup/cli.md |
Documents session downloads. |
pkg/cli/logs_parsing_javascript.go |
Reuses shared session parsing for audits. |
pkg/cli/session_parser.go |
Executes embedded parsers through Node.js. |
pkg/cli/session_parser_test.go |
Covers engines and audit integration. |
pkg/cli/sessions_command.go |
Defines command flags and output routing. |
pkg/cli/sessions_download.go |
Downloads, reconstructs, and validates sessions. |
pkg/cli/sessions_download_test.go |
Covers artifacts, formats, legacy layouts, and errors. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| } | ||
| output, _ := cmd.Flags().GetString("output") | ||
| if output != "" { | ||
| if err := writeFileAtomically(output, content); err != nil { |
There was a problem hiding this comment.
Fixed in acf1182: session output now uses a dedicated atomic writer with owner-only permissions. It applies 0600 for new/public destinations and intersects existing permissions with 0600 so restrictive modes are not widened; permission tests cover these cases.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design (via pr-triage's recommendation) to the new gh aw sessions download command and its reconstruction pipeline.
📋 Key Themes & Highlights
Key Themes
- Untested fallback path:
sessionEnginecan return an empty engine ID when no metadata is found anywhere, relying entirely on the unified collector's source detection. No test exercises this branch directly. - Duplicated repo-flag parsing:
sessions_command.gore-implements owner/repo splitting with a different helper (repoutil) thanaudit_command.go'sapplyAuditRepoFlag, with no comment explaining the divergence.
Positive Highlights
- ✅ Excellent test coverage overall: 367 lines covering precedence (usage → agent reconstruction), six engine parsers, legacy artifact layouts, output routing, and error paths, all verified passing locally.
- ✅ Clean extraction of
session_cli.cjs/session_parsers.goto share runtime parsers betweenauditand the new command, removing a large chunk of fragile inline Node.js bootstrap code fromlogs_parsing_javascript.go. - ✅
validateSessionJSONLis a solid, well-tested safety net against malformed or unsupported session files before they reach the renderer. - ✅ Verified locally:
go build ./...succeeds and the focused test suite (TestSessionsDownload*,TestSessionArtifactName,TestValidateSessionJSONL,TestSessionParser*,TestParseAgentLog*) passes.
No blocking issues found; the two inline notes are suggestions for follow-up test coverage and code-path clarity, not correctness bugs.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 102.5 AIC · ⌖ 13.5 AIC · ⊞ 10K
Comment /matt to run again
| } | ||
| // The collector can read canonical events without engine metadata and can | ||
| // recognize Pi's dedicated stream. Other logs use its custom-engine parser. | ||
| sessionsDownloadLog.Print("No engine metadata available; using the unified collector's source detection") |
There was a problem hiding this comment.
[/tdd] The silent fallback path (no engine metadata found, returns "") isn't exercised by any test in sessions_download_test.go — every test case supplies an aw_info.json with an engine_id. This is the exact path a truly legacy/metadata-less run would hit.
💡 Suggested test
Add a case to TestSessionsDownloadReconstructs (or a new focused test) that downloads only an agent artifact with no info/aw-info/activation artifact and no aw_info.json, then asserts the reconstruction still succeeds via the collector's source detection (e.g. for a Pi-stream or custom-engine log).
@copilot please address this.
There was a problem hiding this comment.
Fixed in acf1182: added an agent-artifact-only Pi-stream reconstruction test with no aw_info.json or metadata artifact, verifying collector source detection succeeds.
| } | ||
| repoFlag, _ := cmd.Flags().GetString("repo") | ||
| if repoFlag != "" && components.Owner == "" { | ||
| ownerRepo, host := repoutil.NormalizeRepoForAPI(repoFlag) |
There was a problem hiding this comment.
[/codebase-design] This re-implements audit_command.go's applyAuditRepoFlag repo-slug parsing inline instead of reusing it, and the behavior differs subtly: applyAuditRepoFlag validates owner/repo with strings.SplitN and rejects malformed input with a clear error, while this path uses repoutil.NormalizeRepoForAPI + SplitRepoSlug, a different parser with different error semantics.
💡 Suggestion
Either extract a single shared helper used by both audit and sessions, or add a short comment explaining why sessions intentionally uses the repoutil parser (e.g. because it must also accept a host prefix). As written, a reader has no way to tell if the divergence is intentional, which hurts navigability per /codebase-design.
@copilot please address this.
There was a problem hiding this comment.
Clarified in acf1182: sessions intentionally uses NormalizeRepoForAPI because its --repo flag supports [HOST/]owner/repo for GHES/Proxima; the audit helper only parses plain owner/repo.
There was a problem hiding this comment.
Impeccable review (mode: critique + audit, new CLI command/feature)
Reviewed gh aw sessions download (new experimental command), its shared session_parser.go adapter, the embedded JS runtime bundle in actions/setup/session_parsers.go, and the pkg/cli/logs_parsing_javascript.go refactor that now reuses the same parser.
Findings: none blocking.
- Command/flag design is consistent with existing
audit/logsconventions (--repo,--output/-o,--formatwith completion), andsessions/downloadare both clearly marked "Experimental" in help text (verified viaTestSessionsDownloadExperimentalHelp). - Artifact precedence (
usage/aw_session.jsonl→ agent artifact → legacy layouts) mirrors the legacy-layout handling already used infirewall_policy.go/logs_download_flatten.go(tmp/gh-aw/prefix stripping), so it's not introducing a new ad hoc pattern. validateSessionJSONLcorrectly enforces the leadingsession.formatheader, rejects duplicate headers, and validates namespacedtype/objectdata, with solid table-driven tests (TestValidateSessionJSONL) covering malformed/legacy/array/null cases.sessionArtifactNameambiguity handling (erroring on multiple suffix matches instead of silently combining different called workflows' sessions) is a good safety choice, and is tested (TestSessionArtifactName).runSessionParserpropagates stderr diagnostics and wraps Node.js-not-found errors clearly;writeFileAtomically/temp-dir cleanup patterns reused correctly.go build,gofmt, and the full targeted Go test suite (TestSessionsDownload*,TestSessionArtifactName,TestValidateSessionJSONL,TestSessionParser*) pass locally with Node.js available.- JS-side
session_cli.test.cjscouldn't be executed in this sandbox (vitest install blocked by a network/TLS restriction unrelated to the PR), but the test file itself is well-structured (round-trips reconstruction and Markdown rendering, verifies privacy redaction of prompts/tool args, and checks invalid-input error messages) and the PR description states 56 JS tests already passed in CI.
No actionable inline comments — the change is incremental, reuses established helpers (buildRepoFlag, flattenArtifactTree, parseAwInfo, resolveAuditHostname), and ships solid test coverage for both the happy paths and error paths.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 127.8 AIC · ⌖ 13.2 AIC · ⊞ 8.1K
|
@copilo run pr-finisher skill |
|
@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>
Fixed in |
|
🎉 This pull request is included in a new release. Release: |
Workflow sessions should be easy to export without generating a full audit report, including runs produced before gh-aw published unified session files. This adds experimental
gh aw sessions download <run-id-or-url>with the same engine parsers used by audit and workflow summaries.Approach
usage/aw_session.jsonland return published JSONL byte-for-byte. If it is missing, reconstruct the current unified format from native agent events or raw engine logs, retaining available MCP, firewall, usage, and grader evidence.agent-artifacts, standalone agent logs, legacy metadata, workflow-call prefixes, andtmp/gh-awnesting. Neitheraw_session.jsonlnoragent-session.jsonlis required for historical runs.--format markdown,--output/-o, and--repo/-r. Markdown uses the shared privacy-preserving, bounded renderer. Node.js is required for reconstruction and Markdown, but not for published JSONL downloads.Reconstruction uses only downloaded evidence and reports missing components rather than inventing them. It does not download all downstream artifacts.
Validation
Passed:
make build,make fmt,make lint-cjs, and standard Go lint for all changed packages.Historical samples below had no unified session file and successfully exported both JSONL and Markdown:
The impacted test groups in
make agent-report-progresspassed. The combined repository gate remains blocked by seven pre-existing custom slice-index lint findings in unchanged code withincmd/gh-aw/main.go; these were left outside this feature's scope.