Repository navigation
fix: render unified session summary at top level - #68820
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall verdict: no blocking issues found.
Reviewed the provided diff and found no actionable blocking issues in the changed lines.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 26.5 AIC · ⌖ 5.42 AIC · ⊞ 21.4K
Comment /review to run again
Remove the Agentic Conversation wrapper for unified traces while preserving the Unified session details section and legacy log rendering. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Skills-Based Review
Applied /diagnosing-bugs (root-cause verification) and /grill-with-docs (spec alignment). No actionable issues found - approving.
Review notes
The fix correctly replaces the <details><summary>Unified session</summary>...</details> wrapper with a top-level ### Unified session heading in unified_session_render.cjs. I traced the byte-budget accounting path (shellBytes computation, the remaining < 512 fallback, and the priority-ordered section trimming) and confirmed the removal of the preamble/tail pair was propagated consistently - no size-accounting regression.
Test coverage is strong: the new disclosure-depth-tracking test in unified_session_render.test.cjs (counting <details>/</details> balance alongside ### Conversation) directly encodes the regression this PR prevents - a nested Conversation disclosure could silently reappear. The heading-regex assertions (/^### Unified session\n\n/, .not.toContain("<summary>Unified session</summary>")) across generate_usage_artifacts.test.cjs, session_cli.test.cjs, and unified_session_render.test.cjs are specific and would not pass on a partial revert.
The spec update in unified-agent-session-specification.md accurately reflects the new behavior (top-level heading, no enclosing disclosure) and keeps the per-source statistics/timeline disclosures collapsed as before.
Note: I could not execute vitest locally (registry blocked in this sandbox with a TLS error), so correctness was verified by manual trace of the budget-accounting logic rather than running the suite; the PR description states 79 tests passed.
Positive highlights
- Minimal, surgical diff (30+/16-) focused exactly on the stated bug
- New test directly encodes the no-nested-disclosure invariant, not just the literal string change
- Documentation and implementation updated together
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 123.8 AIC · ⌖ 15.5 AIC · ⊞ 11.1K
Comment /matt to run again
There was a problem hiding this comment.
🔵 Needs a closer look
Four Go CLI test assertions still expect the removed disclosure markup and fail when Node.js is available.
0 open findings
What changed in this PR
Makes gh-aw’s unified session conversation immediately visible in Actions step summaries.
Changes:
- Replaces the outer disclosure with a heading.
- Keeps overview and timeline collapsible while preserving byte accounting and safe rendering.
- Updates JavaScript assertions and the rendering specification.
| File | Description |
|---|---|
| docs/src/content/docs/specs/unified-agent-session-specification.md | Documents the flat summary layout. |
| actions/setup/js/unified_session_render.test.cjs | Checks headings and disclosure structure. |
| actions/setup/js/unified_session_render.cjs | Removes the wrapper and adjusts byte accounting. |
| actions/setup/js/session_cli.test.cjs | Updates CLI heading expectations. |
| actions/setup/js/generate_usage_artifacts.test.cjs | Checks publication of a single heading. |
🧠 Review effort: Max
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
🎉 This pull request is included in a new release. Release: |
The agent conversation step summary nests Unified session inside an extra Agentic Conversation disclosure, adding an unnecessary layer to inspect the run.
Keep Unified session as a collapsed
<details>section at the document's top level. Skip the extra Agentic Conversation wrapper for unified traces while preserving legacy log rendering, the nested Session overview and Full timeline sections, and separate Safe Outputs previews. Update the rendering specification and add regressions that check top-level placement with and without safe outputs.Validation: 146 focused summary, publication, bootstrap, and session CLI tests passed; JavaScript type checking, formatting/linting, and
make agent-report-progresspassed.