refactor(pipeline): consolidate checks, doctor, and workflow parsing - #25
Closed
nodeselector wants to merge 2 commits into
Closed
refactor(pipeline): consolidate checks, doctor, and workflow parsing#25nodeselector wants to merge 2 commits into
nodeselector wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
⚠️ Not ready to approve
Critical workflow correctness issues were found (local composite refs are currently dropped during parsing, and NeedsAttention can incorrectly treat error findings as “no attention needed”).
Pull request overview
This PR refactors the workflow-scanning pipeline by extracting workflow YAML parsing/rewriting into a dedicated workflowfile package, consolidating diagnostics into internal/pipeline/checks, and reorganizing reachability/impostor handling (including recording “observed/live” SHAs for auditability). It also removes the old internal/doctor remediation implementation in favor of the new checks/reporting pipeline and introduces a unified config loader plus optional profiling.
Changes:
- Added
internal/workflowfilefor comment-preserving workflow parsing, action ref extraction, anduses:rewriting (incl. sub-path handling). - Reworked pipeline orchestration to pre-warm resolver/reachability caches, emit new complement findings, and enrich impostor findings with recommended releases.
- Removed the legacy
internal/doctorcode path and added new config/profiling utilities plus additional UI/tests.
File summaries
| File | Description |
|---|---|
| internal/workflowfile/workflowfile.go | New workflow YAML loader/parser, action ref extraction, local composite discovery helpers, and path key normalization. |
| internal/workflowfile/workflowfile_test.go | Unit tests for workflow loading/ref extraction and local composite path traversal refusal. |
| internal/workflowfile/testdata/simple.yml | Test fixture workflow with basic uses: refs. |
| internal/workflowfile/testdata/mixed_refs.yml | Test fixture covering local actions, docker refs, reusable workflows, and expressions. |
| internal/workflowfile/rewrite.go | Comment-preserving uses: rewriting anchored by YAML node (line/column), with sub-path support. |
| internal/workflowfile/rewrite_test.go | Tests for rewrite behavior (comments, anchors/aliases, sub-path actions). |
| internal/ui/ui_test.go | New regression tests for spinner worker buffering, hints, and animator redraw heartbeat. |
| internal/profile/profile.go | New profiling session support (trace/pprof) and aggregated HTTP request logging. |
| internal/pipeline/run.go | New pipeline Run orchestration with parsing, fast-path skip, resolve/reachability prewarm, diagnose, and enrichment. |
| internal/pipeline/resolver_test.go | Tests for prewarmed resolver cache behavior and live-moved dependency synthesis. |
| internal/pipeline/reach_partition.go | Helpers to partition/union deps for reachability pre-warming and live-moved/live-direct sweeps. |
| internal/pipeline/reach_findings.go | Complement findings for reachability gaps and live-direct impostor emission. |
| internal/pipeline/reach_findings_test.go | Tests for complement reachability findings behavior and confidence invariants. |
| internal/pipeline/parse.go | Parsing wrapper(s) for workflows and lockfile deps, plus resolvable union collection. |
| internal/pipeline/impostor_parity_test.go | Parity tests ensuring live-direct impostor detection matches legacy behavior expectations. |
| internal/pipeline/finding_enrich.go | Parent attachment and inventory parent population helpers. |
| internal/pipeline/doc_urls.go | Docs URL mapping for check categories + releases URL helpers. |
| internal/pipeline/diagnose.go | Parallelized per-workflow diagnose path integrating resolver pre-warming and complement sweeps. |
| internal/pipeline/checks/structural.go | Structural checks (not pinned, SHA-as-ref, ref changed, stale). |
| internal/pipeline/checks/run.go | Check runner wiring structural + resolver-bound checks. |
| internal/pipeline/checks/resolver.go | CheckResolver interface and prewarmedResolver adapter with reachability cache. |
| internal/pipeline/checks/parsed.go | New ParsedWorkflow schema for parse/diagnose phases including fast-path flags. |
| internal/pipeline/checks/misleading.go | Resolver-bound checks for misleading SHA, ref moved/forgery, and impostor commit. |
| internal/pipeline/checks/impostor.go | Recommended release discovery + report enrichment for impostor findings. |
| internal/pipeline/checks/impostor_test.go | Tests for recommended release selection/enrichment behavior. |
| internal/pipeline/checks/finding.go | Refactored finding/report schema (confidence axis, observed SHA, recommendations, repo findings). |
| internal/pipeline/checks/category.go | New category/severity/confidence vocab + “inconclusive” classification. |
| internal/pipeline/checks/category_test.go | Frozen-string guards for schema stability and inconclusive partitioning. |
| internal/doctor/version.go | Deleted legacy doctor version helpers. |
| internal/doctor/version_test.go | Deleted legacy doctor tests for mutable version tag logic. |
| internal/doctor/tags.go | Deleted legacy doctor tag listing/cooldown/config logic. |
| internal/doctor/tagging.go | Deleted legacy doctor tag suggestion/picker logic. |
| internal/doctor/session.go | Deleted legacy doctor interactive session state. |
| internal/doctor/remediate.go | Deleted legacy doctor remediation flow. |
| internal/doctor/prompt.go | Deleted legacy doctor prompt abstractions. |
| internal/doctor/picker.go | Deleted legacy doctor picker implementation. |
| internal/doctor/is_upgrade_test.go | Deleted legacy doctor upgrade/narrowing tests. |
| internal/doctor/diagnose.go | Deleted legacy doctor diagnose implementation. |
| internal/doctor/config.go | Deleted legacy doctor config loader. |
| internal/doctor/compare.go | Deleted legacy doctor snapshot comparison logic. |
| internal/doctor/apply.go | Deleted legacy doctor apply/pin implementation. |
| internal/config/config.go | New consolidated config loader (file + env) including cooldown, workers, and stall hint settings. |
Copilot's findings
Comments suppressed due to low confidence (2)
internal/pipeline/checks/finding.go:82
- WorkflowReport.NeedsAttention currently treats MisleadingSHA (severity error) and RefMoved as “no attention needed”. That can cause callers (e.g. pin planning) to incorrectly treat a workflow as fully verified even when it has a security/consistency finding.
internal/pipeline/reach_partition.go:316 - There’s a trailing comment block about reachabilityComplementFindings at the end of this file with no function following it. This looks like a leftover from moving the implementation to reach_findings.go and should be removed to avoid confusing readers.
- Files reviewed: 44/44 changed files
- Comments generated: 14
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.
| out = append(out, pw) | ||
| continue | ||
| } | ||
| pw.Refs, _, pw.ParseWarnings = wf.ExtractActionRefs() |
Comment on lines
+281
to
+291
| endpoint := strings.Join(parts[3:], "/") | ||
| // Collapse SHA-like segments. | ||
| normalized := make([]string, 0, len(parts)-3) | ||
| for _, p := range parts[3:] { | ||
| if len(p) >= 40 && isHex(p) { | ||
| normalized = append(normalized, "{sha}") | ||
| } else { | ||
| normalized = append(normalized, p) | ||
| } | ||
| } | ||
| _ = endpoint |
Comment on lines
+3
to
+9
| import ( | ||
| "github.com/github/gh-actions-pin/internal/dep" | ||
| "github.com/github/gh-actions-pin/internal/ghapi" | ||
| "github.com/github/gh-actions-pin/internal/pipeline/checks" | ||
| "github.com/github/gh-actions-pin/internal/resolve" | ||
| "strings" | ||
| ) |
Comment on lines
+59
to
+74
| // CollectLiveMovedReachDeps returns the deduplicated set of synthetic | ||
| // dependencies (NWO, Ref + LIVE SHA) for which a reachability check | ||
| // should be pre-warmed. Each entry pairs an existing lockfile dep with | ||
| // the LIVE SHA it currently resolves to, when they differ — the input | ||
| // that lets the engine emit checks.ImpostorCommit for the | ||
| // tag-hijacked-to-fork-network shape. Pass live as the result of a | ||
| // single ResolveAllRecursive over the union of refs. | ||
|
|
||
| // CollectLiveMovedReachDeps returns the deduplicated set of synthetic | ||
| // dependencies (NWO, Ref + LIVE SHA) for which a reachability check | ||
| // should be pre-warmed. Each entry pairs an existing lockfile dep with | ||
| // the LIVE SHA it currently resolves to, when they differ — the input | ||
| // that lets the engine emit checks.ImpostorCommit for the | ||
| // tag-hijacked-to-fork-network shape. Pass live as the result of a | ||
| // single ResolveAllRecursive over the union of refs. | ||
| func CollectLiveMovedReachDeps(parsed []checks.ParsedWorkflow, live []dep.Dependency) []dep.Dependency { |
Comment on lines
+128
to
+143
| // liveDirectReachDeps returns live-resolved deps whose (NWO, Ref, SHA) | ||
| // isn't already covered by the locked-SHA sweep (partitionReachByLive) or | ||
| // the tag-moved sweep (liveMovedDeps), so the engine can give them a | ||
| // fresh reachability check before pinning. Covers two pin-time impostor | ||
| // shapes that the existing diagnose paths miss: | ||
| // | ||
| // - NotPinned workflow: no ExistingDep at all, so the locked-SHA sweep | ||
| // never runs. Without this, applyPin's reach loop is the only thing | ||
| // catching these — diagnose now fires the checks.ImpostorCommit | ||
| // finding pre-pin so the auto-fix runs via tryAutoFixImpostors. | ||
| // - Transitive composite dep that ResolveAllRecursive discovered but | ||
| // isn't yet in the lockfile. The locked-SHA sweep can't see it; the | ||
| // live-moved sweep only fires when an ExistingDep exists for the same | ||
| // dep key with a different SHA. | ||
| // | ||
| // Dedup by ghapi.Reach across direct + transitive entries. |
Comment on lines
+3
to
+9
| import ( | ||
| "fmt" | ||
| "github.com/github/gh-actions-pin/internal/dep" | ||
| "github.com/github/gh-actions-pin/internal/ghapi" | ||
| "github.com/github/gh-actions-pin/internal/pipeline/checks" | ||
| "github.com/github/gh-actions-pin/internal/resolve" | ||
| ) |
Comment on lines
+105
to
+114
|
|
||
| // liveReachImpostorFindings emits checks.ImpostorCommit for live-resolved | ||
| // SHAs that come back Unreachable from the live-direct sweep. Operates on | ||
| // synthetic live deps (not pw.ExistingDeps), so it fires for unpinned and | ||
| // transitive-not-in-lockfile cases that reachabilityComplementFindings | ||
| // (keyed on existing deps) can't see. | ||
| // | ||
| // Suppresses duplicates against any prior impostor/forgery finding for the | ||
| // same dep key — the engine's checkImpostorCommit may have already emitted | ||
| // for a direct ref via the live-ref-vs-locked compare in check_misleading. |
Comment on lines
+183
to
+189
|
|
||
| // CollectLiveDirectReachDeps is the cmd-level pre-warm analogue of | ||
| // liveDirectReachDeps. Returns the deduplicated set of synthetic live | ||
| // deps across all parsed workflows that need a fresh reachability check | ||
| // because they're outside both the locked-SHA and live-moved sweeps. On | ||
| // a fully steady-state lockfile this is empty; on a brand-new repo (no | ||
| // lockfile yet) it's the full live set. |
Comment on lines
+228
to
+231
|
|
||
| // liveMovedDeps is the per-workflow analogue of CollectLiveMovedReachDeps. | ||
| // Returns synthetic (NWO, Ref, LIVE SHA) deps for any existing dep whose | ||
| // live resolve differs from the recorded SHA. |
Comment on lines
+273
to
+281
|
|
||
| // partitionReachByLive splits existing deps into the set that needs a fresh | ||
| // reachability network check and the set that can be synthesized as | ||
| // Reachable because the freshly-resolved live deps confirm the recorded | ||
| // (NWO, Ref, SHA) is still what the ref resolves to right now. | ||
| // | ||
| // When skipUnchanged is false, every existing dep goes to toCheck. This | ||
| // is the --rescan path: re-verify every recorded pin against current | ||
| // upstream branches. |
nodeselector
force-pushed
the
ns/release/pipeline
branch
from
June 8, 2026 03:13
032668f to
4a831ff
Compare
nodeselector
force-pushed
the
ns/release/resolve
branch
from
June 8, 2026 03:13
3ccc5f1 to
292128b
Compare
nodeselector
force-pushed
the
ns/release/pipeline
branch
2 times, most recently
from
June 8, 2026 05:39
0d07228 to
91a607c
Compare
nodeselector
force-pushed
the
ns/release/resolve
branch
2 times, most recently
from
June 8, 2026 05:57
022c737 to
0f12384
Compare
nodeselector
force-pushed
the
ns/release/pipeline
branch
2 times, most recently
from
June 8, 2026 13:01
3194ff4 to
43cdacf
Compare
nodeselector
force-pushed
the
ns/release/resolve
branch
2 times, most recently
from
June 8, 2026 13:10
58d9e5c to
378bb07
Compare
nodeselector
force-pushed
the
ns/release/pipeline
branch
from
June 8, 2026 13:10
43cdacf to
1b3b81c
Compare
Fold findings into pipeline/checks, move impostor and misleading-SHA logic into the checks layer, and tidy doctor orchestration. Records the resolver's actual live SHA in provenance so MISLEADING_SHA claims are auditable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address Copilot review on #25: gofmt the checks and pipeline files (parserlock import ordering), remove the accidentally duplicated doc comments across parse.go, reach_partition.go, reach_findings.go, and finding_enrich.go, replace the stray CollectReachDeps doc above populateInventoryParents with an accurate one, and drop the dead endpoint variable in profile.classifyPath. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
nodeselector
force-pushed
the
ns/release/pipeline
branch
from
June 8, 2026 13:46
1b3b81c to
7b5b776
Compare
nodeselector
force-pushed
the
ns/release/resolve
branch
from
June 8, 2026 13:46
378bb07 to
02ac875
Compare
Collaborator
Author
|
Consolidated into #30. Closing this stacked PR. |
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.
Layer 3/7. Base:
ns/release/resolve.Fold findings into
pipeline/checks, move impostor and misleading-SHA logic into the checks layer, and tidy doctor orchestration. Records the resolver`'s actual live SHA in provenance so MISLEADING_SHA claims are auditable.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.