feat(cmd): rework CLI surface, formatting, and --no-fix - #27
feat(cmd): rework CLI surface, formatting, and --no-fix#27nodeselector wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
⚠️ Not ready to approve
There are confirmed logic issues in the new summary grouping (workflow lists can be dropped) and --json flag validation is currently a no-op, allowing invalid JSON field selections to fail only after writes may have occurred.
Pull request overview
This PR reshapes the gh actions-pin CLI around a detached per-repo lockfile and makes output format selection (--json) independent from remediation behavior via a new --no-fix read-only mode. It also introduces a dedicated cmd/.../format package (JSON, terminal, URL rendering) and updates/extends the test suite with new golden fixtures to lock down the JSON contract.
Changes:
- Reworked root/check command wiring, exit-code semantics, path discovery/expansion, and the
--no-fixvs--jsonbehavior split. - Added a new
cmd/gh-actions-pin/formatpackage for JSON/terminal/url presentation plus associated tests. - Updated integration tests and added a JSON golden snapshot fixture + workflow/lockfile testdata.
File summaries
| File | Description |
|---|---|
| cmd/gh-actions-pin/upgrade.go | Removed the legacy upgrade command implementation (CLI surface change). |
| cmd/gh-actions-pin/upgrade_test.go | Removed tests tied to the deleted upgrade command implementation. |
| cmd/gh-actions-pin/root.go | Added execute() exit-code mapping, refactored root wiring, added directory arg expansion and shared run setup. |
| cmd/gh-actions-pin/root_test.go | Updated root command construction to use newRootCmd. |
| cmd/gh-actions-pin/main.go | Switched entrypoint to execute(). |
| cmd/gh-actions-pin/check.go | Major refactor: new flag surface (--no-fix, --rescan, --profile), new pipeline wiring, JSON rendering via format.WriteJSON. |
| cmd/gh-actions-pin/errors.go | Added errSilent sentinel in a dedicated file. |
| cmd/gh-actions-pin/pin_summary.go | New terminal summary renderer for post-plan/commit results. |
| cmd/gh-actions-pin/format/json.go | New unified JSON formatter for check --json output. |
| cmd/gh-actions-pin/format/terminal.go | New terminal presenter for check results/warnings/errors. |
| cmd/gh-actions-pin/format/url.go | New helper for deriving GitHub URLs from dep keys (commit/tree/releases). |
| cmd/gh-actions-pin/format/*_test.go | Added unit tests for terminal/json/url formatting behavior. |
| cmd/gh-actions-pin/command_test.go | Updated/expanded integration tests for the new lockfile architecture and --no-fix/--json semantics. |
| cmd/gh-actions-pin/check_test.go | Removed empty placeholder test file. |
| cmd/gh-actions-pin/check_json_golden_test.go | Added golden snapshot test to enforce additive-only JSON contract. |
| cmd/gh-actions-pin/testdata/golden-json/** | Added detached workflow + lockfile fixtures and expected JSON output for golden test. |
Copilot's findings
- Files reviewed: 18/19 changed files
- Comments generated: 4
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| seen := map[string]int{} // NWO@Ref → index | ||
| var grouped []groupedEntry | ||
| directCount := 0 | ||
| workflowSet := map[string]bool{} | ||
| for _, e := range pinned { | ||
| if !e.Direct { | ||
| continue | ||
| } | ||
| key := e.NWO + "@" + e.Ref | ||
| if idx, ok := seen[key]; ok { | ||
| for _, wf := range e.Workflows { | ||
| if !workflowSet[wf] { | ||
| grouped[idx].workflows = append(grouped[idx].workflows, wf) | ||
| } | ||
| } | ||
| } else { | ||
| seen[key] = len(grouped) | ||
| grouped = append(grouped, groupedEntry{Entry: e, workflows: append([]string{}, e.Workflows...)}) | ||
| directCount++ | ||
| } | ||
| for _, wf := range e.Workflows { | ||
| workflowSet[wf] = true | ||
| } | ||
| } |
| // validateOutputFlags rejects incoherent structured-output flag combinations. | ||
| // Wired as PreRunE so the error surfaces at the command layer before any work runs. | ||
| func (opts *checkOptions) validateOutputFlags() error { | ||
| return nil | ||
| } |
| // errSilent is returned by command run functions when blocking findings have | ||
| // already been reported through well-formed output (e.g. JSON on stdout). It | ||
| // maps to exit code 1 in Execute without printing a second error line. | ||
| var errSilent = errors.New("silent error") |
| seenU := map[string]int{} | ||
| var groups []unresolvedGroup | ||
| affectedWFs := map[string]bool{} | ||
| for _, e := range unresolvedEntries { | ||
| key := e.NWO + "@" + e.Ref | ||
| if idx, ok := seenU[key]; ok { | ||
| for _, wf := range e.Workflows { | ||
| if !affectedWFs[wf] { | ||
| groups[idx].wfs = append(groups[idx].wfs, wf) | ||
| } | ||
| } | ||
| } else { | ||
| seenU[key] = len(groups) | ||
| groups = append(groups, unresolvedGroup{nwo: e.NWO, ref: e.Ref, reason: e.Reason, wfs: append([]string{}, e.Workflows...)}) | ||
| } | ||
| for _, wf := range e.Workflows { | ||
| affectedWFs[wf] = true | ||
| } | ||
| } |
2080bb0 to
b200762
Compare
2e51265 to
490e9a1
Compare
b200762 to
d538317
Compare
490e9a1 to
59fbf55
Compare
d538317 to
27c9bf7
Compare
12bb8cd to
dc96b82
Compare
683529f to
19c22cb
Compare
dc96b82 to
1145b9b
Compare
19c22cb to
d3fbe12
Compare
1145b9b to
208df2c
Compare
Decouple --json rendering from autofix via --no-fix, reorganize the format package (json/terminal/url), and update check/upgrade/root command wiring and golden fixtures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
renderPinnedEntries and renderUnresolvedWarnings deduplicated affected workflows with a single global set, so a workflow shared by two actions was listed under only the first. Track workflows per group instead, and keep the global set for the header count only. validateOutputFlags was a no-op stub despite a PreRunE doc promise, so an unknown --json field only failed late in WriteJSON, after check may have mutated workflows. Add format.ValidateJSONFields and call it pre-run. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GetDefaultBranch and RepoIDs now derive from a single repos/{owner}/{repo}
round-trip (ghapi singleflight unification), so the autofix test no longer
needs to register the bare repo endpoint twice.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
With each wave's refs resolved in one batched GraphQL query, setup-go@v6 now resolves and expands during discovery (it no longer starves on a consumed single-shot stub), so its transitive children cache@v4 and helper/only-transitive@v1 are correctly recognized as referenced rather than flagged stale, and the reachability re-resolve is served from cache instead of erroring. Three findings that were artifacts of per-ref stub starvation drop away; the JSON schema is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
208df2c to
85df9bf
Compare
d3fbe12 to
7dcc0a4
Compare
|
Consolidated into #30. Closing this stacked PR. |
Layer 5/7. Base:
ns/release/lockfile.Decouple
--jsonrendering from autofix via--no-fix, reorganize theformatpackage (json/terminal/url), and update check/upgrade/root wiring plus golden fixtures.Part of a stacked series for the pre-release hardening of
gh actions-pin. Review bottom-up; each PR is based on the one below it so the diff shows only that layer.