Skip to content

Improve CLI recording rendering - #14507

Merged
BagToad merged 6 commits into
trunkfrom
williammartin-cli-rendering-follow-up
Sep 23, 2026
Merged

BagToad merged 6 commits into
trunkfrom
williammartin-cli-rendering-follow-up

Conversation

@williammartin

@williammartin williammartin commented Sep 23, 2026 •

Copy link
Copy Markdown
Member

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 --font when it renders, and repeated --font-fallback options 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

  • Execution contracts and preflight receipts contain no font selection; contract font fields are rejected.
  • Every rendering requires an explicit --font; repeated --font-fallback values define the complete ordered presentation-only fallback chain.
  • Each rendering records the primary font in rendering.font, auto-loaded style companions in rendering.fontStyles, and glyph fallbacks in rendering.fontFallbacks, including path, family, and SHA-256.
  • Unavailable render-time fonts fail rendering without changing the recorded case outcome or captured evidence.
  • The safe-area frame increases final media dimensions by 96 pixels in each axis rather than cropping or scaling terminal content.

Notes for reviewers

Start with .github/skills/cli-exercise/tool/internal/engine/contract.go for the execution-contract boundary and evidence/main.go for mandatory explicit render-time font selection. fontutil/font.go and evidence/render.go contain complete primary/style/fallback provenance. raster.go and overview.go contain the framed presentation, while fonts_test.go covers capture preservation and rendering metadata. This PR is intentionally layered on #14475.

Authorship and follow-up

Who wrote this:

  • A human wrote it.
  • An agent wrote it under close human direction.
  • An agent wrote it independently, and no human has guided the implementation beyond the initial prompt.

Who answers review comments:

  • @williammartin will read and reply directly. Name the account.
  • An agent will draft replies and @username will read them before they are posted.
  • Nobody has explicitly committed to replying.

@williammartin
williammartin added this pull request to stack #14508 September 23, 2026 10:34
Copilot AI balanced review requested due to automatic review settings September 23, 2026 11:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Medium severity · 3 Low severity

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.

Comment thread .github/skills/cli-exercise/tool/internal/engine/contract.go Outdated
Comment thread .github/skills/cli-exercise/internal/cli-exercise-evidence/SKILL.md Outdated
Comment thread .github/skills/cli-exercise/references/runtime-interface.md Outdated
Comment thread .github/skills/cli-exercise/tool/internal/evidence/fonts_test.go Outdated
@williammartin
williammartin requested a balanced review from Copilot September 23, 2026 12:02
@williammartin
williammartin marked this pull request as ready for review September 23, 2026 12:06
@williammartin
williammartin requested a review from a team as a code owner September 23, 2026 12:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Medium severity · 1 Low severity

Open (3)
Resolved since last review (3)

Comment thread .github/skills/cli-exercise/tool/internal/evidence/main.go Outdated
Comment thread .github/skills/cli-exercise/tool/internal/evidence/render.go Outdated
@williammartin
williammartin marked this pull request as draft September 23, 2026 14:14
@williammartin
williammartin marked this pull request as ready for review September 23, 2026 17:58
Base automatically changed from bagtoad/cli-video-comments to trunk September 23, 2026 21:07
williammartin and others added 6 commits September 23, 2026 15:07
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
BagToad force-pushed the williammartin-cli-rendering-follow-up branch from e0d2f86 to 4045929 Compare September 23, 2026 21:07
@BagToad
BagToad merged commit b6770c8 into trunk Sep 23, 2026
11 checks passed
@BagToad
BagToad deleted the williammartin-cli-rendering-follow-up branch September 23, 2026 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants