Improve CLI recording rendering - #14507
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Legacy font semantics are documented inaccurately, and new validation and provenance behavior lack ordinary regression coverage.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (4)
What changed in this PR
Moves font selection from immutable capture contracts to rendering and adds a 48px framed safe area.
Changes:
- Adds render-time primary and fallback font overrides with provenance metadata.
- Frames terminal and overview media with consistent margins.
- Updates validation, tests, and documentation for fontless contracts.
| File | Description |
|---|---|
fontutil/font.go |
Records fallback font provenance. |
execution/cli_test.go |
Removes captured font path expectation. |
evidence/session.go |
Propagates session font overrides. |
evidence/render.go |
Encodes framed dimensions and font metadata. |
evidence/raster.go |
Draws framed terminal presentations. |
evidence/presentation_test.go |
Tests framing and session overrides. |
evidence/overview.go |
Frames overview pages. |
evidence/model.go |
Makes contract font fields optional. |
evidence/main.go |
Adds rendering font flags and report fields. |
evidence/fonts_test.go |
Tests font override behavior. |
evidence/evidence_test.go |
Updates raster dimension expectations. |
engine/runtime_test.go |
Removes required contract font fixture. |
engine/contract.go |
Makes font paths legacy-optional. |
references/runtime-interface.md |
Documents rendering fonts and framing. |
references/run-contract.md |
Removes fonts from new execution contracts. |
references/recording-sessions.md |
Documents session-wide font overrides. |
cli-exercise-evidence/SKILL.md |
Updates rendering workflow instructions. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Explicit font enforcement and complete provenance for auto-loaded style fonts remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (3)
williammartin
marked this pull request as draft
September 23, 2026 14:14
williammartin
marked this pull request as ready for review
September 23, 2026 17:58
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
BagToad
force-pushed
the
williammartin-cli-rendering-follow-up
branch
from
September 23, 2026 21:07
e0d2f86 to
4045929
Compare
BagToad
approved these changes
Sep 23, 2026
4 of 9 tasks
Merged
3 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


Follow-up to #14475.
Description
When trying out the skill in #14475, I found that the font chosen before execution could not render the braille loader. Font selection is not used to execute or capture the command, so this PR keeps fonts out of execution contracts and preflight receipts. The evidence skill must select the primary font explicitly with
--fontwhen it renders, and repeated--font-fallbackoptions define the complete ordered fallback chain without modifying or rerunning the capture. Each completed rendering records the exact primary, auto-loaded style companion, and fallback font paths and hashes that could produce its pixels.I also was confused when reviewing the recording because QuickTime's on-hover header was obscuring commands, and I thought the rendering pipeline was broken. This PR introduces a consistent framed 48px safe area.
How did you test this change?
Using the immutable capture from an organization-owned-fork scenario, I rendered a new 1416x1008 session video with Menlo and Apple Braille selected at render time. The video shows the Braille spinner, expected GraphQL failure, validation, cleanup, overview, and chapter boundaries, with terminal and overview content inside a 48px safe area. This demonstrates that missing glyph coverage and presentation styling can be corrected without executing the recorded GitHub operations again.
cli-recording-rendering-evidence.mp4
Key points
--font; repeated--font-fallbackvalues define the complete ordered presentation-only fallback chain.rendering.font, auto-loaded style companions inrendering.fontStyles, and glyph fallbacks inrendering.fontFallbacks, including path, family, and SHA-256.Notes for reviewers
Start with
.github/skills/cli-exercise/tool/internal/engine/contract.gofor the execution-contract boundary andevidence/main.gofor mandatory explicit render-time font selection.fontutil/font.goandevidence/render.gocontain complete primary/style/fallback provenance.raster.goandoverview.gocontain the framed presentation, whilefonts_test.gocovers capture preservation and rendering metadata. This PR is intentionally layered on #14475.Authorship and follow-up
Who wrote this:
Who answers review comments: