Skip to content

feat: ux & code refactor - #7

Merged
nodeselector merged 64 commits into
mainfrom
ns/primer-output-formatting
Apr 29, 2026
Merged

feat: ux & code refactor#7
nodeselector merged 64 commits into
mainfrom
ns/primer-output-formatting

Conversation

@nodeselector

@nodeselector nodeselector commented Apr 24, 2026

Copy link
Copy Markdown
Collaborator
  • UX refactor
  • Code arch refactor

nodeselector and others added 2 commits April 23, 2026 19:45
Add internal/ui package that provides structured, PRimer-compliant
output formatting for all CLI messages. Features:

  Skip (-), Info, Header, Hint, Detail
- Style helpers: Bold, Dim, Red, Green, Yellow, Cyan
- Colored diff output (red deletions, green additions, yellow changes)
- TTY detection on stderr (not stdout) so --json piping works correctly
- Respects NO_COLOR and CLICOLOR=0 environment variables
- NewPlain(w) constructor for tests (no ANSI, custom writer)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace all 65 fmt.Fprintf(os.Stderr, ...) calls with structured
output methods from internal/ui. Key changes:

- Bold headers instead of ==> homebrew-style markers
- Dim hints instead of 'hint:' prefix
- Colored diffs: red for removals, green for additions, yellow for changes
- Remove redundant single-file summary (per-file ✓ is sufficient)
- Update main.go top-level error handler to use output.Error()
- Update test helpers to swap package-level output var with NewPlain(w)
- Rename local 'output' vars in pinOneFile/upgradeOneFile to 'written'
  to avoid collision with package-level output *ui.UI

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 24, 2026 00:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a centralized, terminal-aware UI output layer for the gh-actions-pin CLI (aligned with PRimer CLI guidelines) and refactors the main command flows to use structured, icon-prefixed, optionally-colored stderr output instead of ad-hoc fmt.Fprintf(os.Stderr, ...) calls.

Changes:

  • Added internal/ui package to encapsulate TTY-aware styling, icons, and message formatting (including a plain-mode constructor for tests).
  • Refactored CLI output in root.go and main.go to use the new ui.UI methods (headers, status lines, hints/details, and colored diffs).
  • Updated tests to swap the global output instance to ui.NewPlain(...) when capturing stderr.
Show a summary per file
File Description
internal/ui/ui.go New terminal-aware output helper with PRimer-style icons and optional ANSI styling.
root.go Replaces direct stderr formatting with output UI calls; updates diff rendering to use styled symbols.
main.go Uses the shared output.Error(...) for top-level error reporting.
command_test.go Ensures command tests capture deterministic stderr by swapping output to plain UI; updates assertions.
root_test.go Swaps output to plain UI for showDiff stderr-capture tests to avoid ANSI in expectations.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 5/5 changed files
  • Comments generated: 1

Comment thread internal/ui/ui.go
Comment on lines +6 to +13
import (
"fmt"
"io"
"os"

"github.com/muesli/termenv"
"golang.org/x/term"
)

Copilot AI commented Apr 24, 2026

Copy link
Copy Markdown

Warning

This is an internal experiment to assess Copilot's ability to auto-approve PRs. Please 👍 this comment if the assessment below is correct and 👎 if not. Feedback in #f-ccr-auto-approve is appreciated!

Copilot thinks this PR is not ready to approve — see review comments for details.

nodeselector and others added 24 commits April 23, 2026 20:55
Replace technical 'reachability check inconclusive (ref is a raw SHA)'
with actionable guidance: explains that bare SHA pinning weakens
supply-chain security because commit origin cannot be verified, and
directs users to pin to a tag (e.g. v1.2.3) instead.

Also drops the redundant 'reachability check inconclusive' wrapper
from the warning format — the detail message is now self-contained.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Include a direct link to the action's releases page so users can
find the tag they should pin to instead of a bare SHA.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Extract code from tags.go (772→335 lines) and remediate.go (990→400 lines)
into three new files, all staying in package doctor:

- version.go: semver parsing, version comparison (pure computation, zero I/O)
- tagging.go: tag suggestions, picker curation, reorderSuggestions
- apply.go: file mutation (applyPin, applySHAToTag, applyReResolve),
  ref approval tracking, and helper functions

No logic changes — pure structural refactor.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Pure structural refactor — no logic changes. Moves code into three
files by responsibility:

- check.go: validation types, check command, check/validate logic, JSON output
- upgrade.go: upgrade types, upgrade command, upgrade logic, preview/diff helpers
- root.go: wiring (newRootCmd), shared helpers, doctor command

All files remain in package main. Build, vet, and tests pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Augment diagnoseOneWorkflow to reach parity with validateOneFile before
the latter can be deleted:

- Add SHA_MISMATCH detection via lockfile.CheckSHARefMismatches
- Add transitive NWO fuzzy matching (same SHA or IsNarrowedVersion)
- Add ReachabilityUnknown warnings (not just Unreachable)
- Preserve parse warnings from ExtractActionRefs in WorkflowReport
- Fix ReadDependencies error handling (report error, don't fold into not-pinned)
- Add InventoryEntry type and Inventory field to WorkflowReport
- Add CategorySHAMismatch constant and Remediator handler
- Multi-value liveByNWO index for correct transitive matching

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove the parallel validation system that duplicated all of
doctor.Diagnose's work. runCheck now calls doctor.Diagnose once
and maps the Report to the existing JSON contract types.

- Delete validateOneFile (~220 lines)
- Delete errNoDeps/errNoActions sentinel errors
- Add reportToValidationResult: Finding → validationError/Warning mapping
- Extract presentCheckResults for human-readable output
- Fix --json=findings bug: JSON output now runs before any human output
- Eliminate duplicate API calls (was resolving+checking reachability twice)

All existing tests pass with identical JSON output contract.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace 5 mapped types (validationError, validationWarning,
validationResult, jsonDependency, jsonWorkflow) with 3 thin
JSON wrappers (checkFinding, checkDependency, checkWorkflow)
that directly expose doctor.Report data.

- --json fields are now: valid, findings, workflows, dependencies
- findings carry category + severity (not TYPE/errors/warnings split)
- Delete reportToValidationResult, writeValidationJSON, writeDoctorJSON
- Add writeCheckJSON with lazy field builders
- Add reportIsValid/workflowIsValid for CI gate computation
- Extract isValidFinding, isWarningFinding, findingDepKey helpers
- presentCheckResults now consumes doctor.Report directly

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Simplify upgrade command Long description to be concise and
action-oriented. Add NoOptDefVal so --json without an argument
defaults to all fields ("updated").

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace three copies of the oldMap/oldByNWO/handledOld reconciliation
pattern with a single DiffDeps function in the lockfile package.

DiffDeps returns a structured DepDiff with Changed, Rekeyed, Added,
Removed, and Unchanged slices. Fuzzy NWO matching now prefers
same-SHA pairs before falling back to first-unhandled.

PreserveRefs replaces the inline ref-preservation loop, keeping
human-readable tags when transitive deps re-resolve to bare SHAs.

Consumers rewritten:
- upgradeOneFile: uses DiffDeps for change detection
- showDiff: renders DepDiff categories directly
- summarizePreview: counts DepDiff categories by direct/transitive

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ions

Extract the 278-line god function into four cohesive pieces:

- scanWorkflowActions: builds action index from workflow files
- findUpgradeCandidates: resolves latest versions, filters noops
- resolveInternalCandidates: handles same-owner action branching
  (default-branch re-resolve vs version tag upgrade)
- sortedKeys: replaces repeated map-to-sorted-slice pattern

runUpgradeInteractive is now 118 lines of sequential orchestration:
setup → scan → resolve → multiselect → plan → confirm → apply.

UI/progress control stays in the orchestrator; extracted functions
return data and warnings without touching the spinner.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Update shaRefRE in diagnose.go and resolver.go to match both 40-char
  (SHA1) and 64-char (SHA256) commit hashes
- Replace hand-rolled isSHARef in apply.go with lockfile.IsFullSHA
- Add Dependency.HashAlgoOrDetect() and use it in showDiff instead of
  hardcoded "sha1-" prefix
- Replace all fmt.Fprintf/Fprintln(os.Stderr) with UI methods
  (output.Detail, output.Blank) in check.go, upgrade.go, remediate.go
- Add UI.Blank() method for consistent blank line output
- Rename test-token to test-placeholder-token in NewWithTransport

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
cli/cli convention: require.NoError/Nil for preconditions where
subsequent assertions would be meaningless on failure.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace global `output` and `newResolver` with a pinFactory struct that
owns stdio, UI, resolver construction, and TTY detection. This aligns
with cli/cli's cmdutil.Factory pattern and eliminates all global
mutable state from the command layer.

- NewRootCmd(f *pinFactory) takes explicit factory parameter
- runCheck, runUpgrade, runUpgradeInteractive take factory
- writeCheckJSON/writeUpgradeJSON write to injected io.Writer
- presentCheckResults/showDiff take *ui.UI parameter
- Tests create factory with pipes instead of swapping globals
- Remove isTerminal() helper — use f.IsTerminal() instead

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add NewHuhPrompterWithWriter(w, isTerminal) constructor so the prompter
uses the factory's ErrOut and IsTerminal instead of hardcoding os.Stderr.
Wire through check.go and upgrade.go RunE closures.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Migrate isValidFinding, isWarningFinding, findingDepKey, reportIsValid,
and workflowIsValid from check.go free functions to methods on their
respective doctor types: Finding.IsValid(), Finding.IsWarning(),
Finding.DepKey(), Report.IsValid(), WorkflowReport.IsValid().

Removes ~55 lines from check.go. extractRepoNWO stays as a string
utility since it operates on a display key, not a Finding.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Group identical findings by dep key and category so a repo with 55
workflows using actions/checkout@v6 shows one line with a file count
instead of 55 identical NOT_PINNED lines. Same dedup for warnings.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…line

Instead of listing every unpinned workflow individually (72 lines for mcv3-boot),
show one summary: '72 workflows not yet pinned (run `gh actions-pin` to fix)'.
The per-dependency dedup was already working; this fixes the warnings section.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When a repo has many unpinned actions (e.g. 34 unique deps across 72 workflows),
show top 10 with file counts then '... and N more unpinned actions'. Suppress the
repetitive 'used in workflow but not pinned' detail line since the NOT_PINNED label
already conveys the message.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
NOT_PINNED findings now only appear in the summary line (e.g. '197 not_pinned').
Per-dep detail is reserved for actionable categories like TAMPERED, STALE, and
SHA_MISMATCH where knowing which specific dep matters. Dramatically reduces output
for repos with no lockfile (mcv3-boot: ~100 lines → ~15).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
nodeselector and others added 3 commits April 28, 2026 12:43
Non-interactive mode now runs the full SHA→tag resolution logic:
  - same-owner repos: auto-pick matching tag or default branch
  - external repos: auto-pick if exactly one full-semver tag matches
  - only skip when the tag picker would actually need human input
    (multiple candidates, unreleased commit, etc.)

Extracted skipDep() helper to consistently track skipped deps.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…rences

Skipped count was inflated — same dep appearing in 20 workflows
counted as 20 skips. Now uses len(SkippedDeps) which tracks unique
deps. Also changed 'issues' to 'actions' in skip/error messages
since that's what they actually are.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove runUpgradeInteractive and all supporting code: scanWorkflowActions,
findUpgradeCandidates, resolveInternalCandidates, sortedKeys, previewMessage,
summarizePreview, formatChangeSummary, shellQuote, buildCommandHint, and
associated types (upgradeCandidate, actionOccurrence, actionIndex, previewStats).

The upgrade command now always requires --action. Interactive discovery and
tag picker will return as part of the dependabot integration work, extracted
to internal/ as a reusable library.

-630 lines deleted.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@nodeselector nodeselector changed the title feat: PRimer-compliant terminal output formatting feat: ux & code refactor Apr 28, 2026
nodeselector and others added 4 commits April 28, 2026 13:54
When a reachability check finds a transitive dep pinned to a bare SHA,
show the same 'comes from a composite action' hint that we show for
CategorySHAAsRef transitive findings. Previously the raw resolver
message was shown without context about where the dep came from.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ResolveAllRecursive now builds a parentMap (child NWO → parent dep key)
while walking composite action trees. Finding gains a ParentNWO field.

Transitive bare-SHA warnings now show:
  ↳ pulled in by github/go-linter@v1
  ↳ in 1 workflow: .github/workflows/ci.yml
  ↳ ask the maintainer of golangci/golangci-lint-action to onboard

Instead of the generic 'comes from a composite action' message.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Actions don't onboard onto dependency pinning — that message was
misleading. Keep only the provenance lines (parent action + workflows).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Same-owner actions in internal/private repos stay broad (v4) — you
trust your own org's internal actions. Same-owner actions in public
repos narrow to patch (v4 → v4.35.2) — public is the trust boundary.

Previously all same-owner actions skipped narrowing, which meant
github/codeql-action@v4 stayed at mutable v4 when running in any
github/* repo, even though codeql-action is public.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@nodeselector
nodeselector force-pushed the ns/primer-output-formatting branch from be8fe2e to 025302d Compare April 28, 2026 19:23
nodeselector and others added 20 commits April 28, 2026 14:29
Transitive deps pinned to bare SHAs are not actionable by the consumer
and create noise. Swallow both CategorySHAAsRef transitive findings and
transitive reachability-unknown findings silently.

TODO comment explains we need to figure out how to coexist with
composite actions that don't use dependency pinning before resurfacing
these warnings.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Consolidate three SHA detection implementations (doctor.isHexSHA,
  resolver.shaRefRE, diagnose.shaRefRE) into single lockfile.IsFullSHA
- Guard parseSemver/IsMutableVersionTag against hex SHAs that match
  the semver regex (e.g. 1e7e51e... parsed as major=1)
- Remove RewriteActionRefs fallback loop that could match comments
  containing action refs before the actual uses: line; preserve
  trailing content (comments) on rewritten lines
- Fix SHA-as-ref reachability: check against HEAD instead of giving up
- Remove GraphQL double-escaping in buildResolveWithFileQuery
- Warn and exit 1 when upgrade --action matches no refs

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ing and YAML helpers

- Move Semver type and ParseSemver to internal/lockfile/version.go, breaking
  the doctor↔resolver import cycle
- Add Dependency.OwnerRepo() to replace 7 inline strings.SplitN patterns
- Add isYAMLFile() helper, used by isReusableWorkflow and isLocalReusableWorkflow
- Delete duplicate stableTagRE/parseInt/compareVersion from resolver
- Delete duplicate shaRefRE from doctor and resolver (use lockfile.IsFullSHA)
- Remove unused imports (regexp, strconv, strings)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Merge UNREACHABLE findings into TAMPERED dep group by attaching
  Dependency pointer to reachability findings (fixes dep key mismatch)
- Two-tier warning messaging:
  - TAMPERED only: "Validate the change is a legitimate update"
  - TAMPERED + UNREACHABLE: bold warning about no shared history
- Remove broken compareHint from diagnose.go Detail (embedded newlines
  broke out.Detail indentation), render URLs structurally in check.go
- Fix compare URL to use oldSHA...ref instead of trailing ellipsis
- Add demo workflow fixtures for VHS recording and tamper scenarios

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- SHA-as-ref scenario: bare SHA triggers tag picker with ranked suggestions
- Unreleased commit scenario: untagged SHA triggers loud warning + picker
- Tamper warning scenario: updated to show new tiered warning messaging
- VHS tapes for reproducible GIF recording of all three scenarios
- Created semver tags (v1.0.0, v1.0.1, v1.1.0, v2.0.0) on fixtures repo

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add check-autofix.tape and upgrade.tape for core workflow demos
- Add demo/workflows-check/ and demo/workflows-upgrade/ fixture dirs
- Set Theme 'GitHub Dark' on all 5 VHS tapes for visual consistency
- Reset interactive fixtures to clean state

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Use echo for terminal comments (not Type '#...' which shells eat)
- TypingSpeed 50ms for readable typing
- Add reset-fixtures.sh and record-all.sh for repeatable recording
- All 5 tapes: check-autofix, upgrade, tamper-warning, sha-as-ref, unreleased-commit

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Shows --no-interactive behavior when actions need human input:
auto-fixes deterministic issues, skips ambiguous ones, exits 1.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- upgrade-version: --action X --version v5 targeted upgrade
- edit-repin: Dependabot workflow (edit ref, clear lockfile, check)
- record-all.sh updated with new tapes

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Each proposal scenario can be run locally with:
  demo/try-it.sh <scenario>

Resets fixtures, runs the exact commands, shows real output.
Supports: check-autofix, ci-failure, sha-as-ref, unreleased-commit,
upgrade-latest, upgrade-version, edit-repin, tamper-detection, json-output.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rename user-facing finding categories to be more descriptive:
- TAMPERED → REF_MOVED (expected for mutable tags like v4)
- SHA_MISMATCH → MISLEADING_SHA (ref looks like SHA but resolves differently)
- UNREACHABLE → IMPOSTER_COMMIT (locked SHA not in ref's history)

Internal resolver enum (Unreachable) unchanged — it's a reachability
concept, not a user-facing category.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Captures the workflow for preparing RFC-style design proposals
for cli/cli UX review: gist iteration, scenario-driven format
(Input/Output/Expected effects), try-it.sh integration, VHS
recording, and the cli/cli working-with-us process.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Link to try-it.sh and design proposal gist for reviewers.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Code walkthrough with package dependency graph, data flow diagrams,
control flow for check/upgrade/resolve/remediate, and code smells
inventory. Every section links to source.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Extract sessionState type from Remediator into session.go
  (choices, internalRefChoices, approvedRefs maps + their methods)
- Extract compareSnapshots() from diagnoseOneWorkflow into compare.go
  with matchLiveDep() and unmatchedFindings() helpers
- Extract tag picker rendering (tagLabel, defaultBranchOption) and
  selection handling (runPicker) into picker.go
- Both handleSHAWithSuggestions and handleSHATagPicker now use shared
  picker infrastructure while keeping their distinct control flow

Net reduction of ~210 lines from existing files. No behavior change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add fixture 5 (pinned-before-update) for clean REF_MOVED where tag
moved forward in lineage (no imposter warning). Create separate VHS
tapes and try-it.sh scenarios for each case:

- ref-moved: routine upstream release, lineage intact
- imposter-commit: fork injection, no shared ancestry

The old tamper-detection scenario alias still works (runs both).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
'1 issue needs' not '1 issue need', '1 action requires' not
'1 action require'.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@nodeselector
nodeselector merged commit f273d3d into main Apr 29, 2026
5 checks passed
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.

2 participants