Beginner UX pass: godoc clarity across the lockfile package - #5
Merged
Conversation
There was a problem hiding this comment.
⚠️ Not ready to approve
There are a couple of concrete doc/API contract mismatches (duplicate SemVer doc block, and IsLocalReusableWorkflow being overly permissive vs its documented contract) that should be corrected before approval.
Pull request overview
This PR is a documentation-focused UX pass over the go/pkg/lockfile public API, aiming to make the package easier to understand for new consumers (clear entry points, clearer field/method contracts, and more actionable godoc examples), plus adding a missing exported-function test.
Changes:
- Reworked/expanded godoc for key exported types and functions (
Parse,File,ParseError,SemVer,ParsePin,ParseActionMeta, package docs). - Clarified the “local reusable workflow vs local composite action” distinction and added a test for
IsLocalReusableWorkflow. - Improved caller guidance via examples and more explicit contract descriptions (e.g., canonical pin key usage and dependency lookup).
File summaries
| File | Description |
|---|---|
| go/pkg/lockfile/version.go | Expanded SemVer documentation and field comments. |
| go/pkg/lockfile/uses.go | Clarified IsLocalReusableWorkflow contract and renamed parameter for readability. |
| go/pkg/lockfile/uses_test.go | Added test coverage for IsLocalReusableWorkflow. |
| go/pkg/lockfile/pin.go | Expanded ParsePin documentation to enumerate failure modes and normalization. |
| go/pkg/lockfile/lockfile.go | Improved docs for ParseError, File fields, LookupWorkflow, Action, and Parse behavior. |
| go/pkg/lockfile/doc.go | Rewrote package docs to lead with Parse → File → LookupWorkflow and clarify parsing helpers. |
| go/pkg/lockfile/action_meta.go | Clarified ParseActionMeta godoc and generalized error message. |
Copilot's findings
- Files reviewed: 7/7 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.
Comment on lines
23
to
28
| // versions ("v4", "v4.2"), and arbitrary suffixes all appear in the | ||
| // wild. x/mod/semver rejects bare and partial tags, and doesn't expose | ||
| // individual components — we need Major/Minor/Patch to compute | ||
| // MajorTag, MinorTag, and IsFull for the tag recommendation engine. | ||
| // SemVer holds parsed semantic version components. | ||
| // |
Comment on lines
+340
to
342
| func IsLocalReusableWorkflow(localUses string) bool { | ||
| return isYAMLFile(localUses) | ||
| } |
Comment on lines
+146
to
+153
| {name: "local reusable yml", input: "./.github/workflows/ci.yml", want: true}, | ||
| {name: "local reusable yaml", input: "./.github/workflows/ci.yaml", want: true}, | ||
| {name: "local composite action directory", input: "./my-action", want: false}, | ||
| {name: "local composite no extension", input: "./my-action/", want: false}, | ||
| {name: "remote action is not local", input: "actions/checkout@v4", want: false}, | ||
| {name: "remote reusable workflow is not local", input: "octo/repo/.github/workflows/ci.yml@v1", want: false}, | ||
| {name: "empty string", input: "", want: false}, | ||
| } |
Comment on lines
+227
to
+231
| // pins, ok := f.LookupWorkflow(".github/workflows/deploy.yml") | ||
| // for _, key := range pins { | ||
| // action := f.Dependencies[key] | ||
| // fmt.Println(action.Branch, action.Commit) | ||
| // } |
A new user's first question is 'how do I read a lockfile,' not 'what is the security boundary.' The old package comment opened with ParseActionRef as 'the choke point' and never mentioned Parse() or File at all — burying the actual entry point below a security treatise. New doc: lead with Parse/File/LookupWorkflow, move ParseActionRef and ParseReusableWorkflowRef to a clear secondary section, and demote the security internals note to a clearly labeled contributor section.
Both fields had zero documentation at the struct level. A reader looking at the struct definition had no idea what the map keys or values were — the context lived only in the type-level comment block's YAML example. Add field-level doc comments that name the key/value formats and point readers toward LookupWorkflow and Dependencies for the intended lookup pattern.
…etails The old ParseError comment told callers the internals (yaml.v3 package prefix, node tree walking) rather than the contract (structured line/col for diagnostics). A caller doesn't care that we use yaml.v3 or how we extract the position — they care what fields they get, when Line/Column are zero, and how to use the type. Rewrite the comment to explain the public contract and add a usage nudge pointing to errors.As.
'Fork-network signal' is GitHub-internal terminology that means nothing to an external consumer. Replace with a concrete explanation: a commit reachable from no branch in the source repo could belong to a fork, which SHA-only pinning cannot detect. Also clarify Tag (optional, why), Commit (same digest as pin key), OwnerID/RepoID (what they're for), and Uses (which action types populate it).
The old doc said 'returns the dependency closure' with no hint about what to do with the strings. A beginner gets a []string and has no idea they should index into File.Dependencies. Add a concrete usage example showing the key→Dependencies lookup, explain the pin key format, and clarify the ok=false vs empty-slice distinction.
The parameter was named 'localPath' suggesting callers should pass a resolved file path, but the function expects the raw uses: string (with the './' prefix intact). Rename the parameter to 'localUses' and rewrite the doc with a concrete two-example comparison showing what returns true vs false, plus a warning to call this only after the './' prefix check. Add TestIsLocalReusableWorkflow — this exported function had zero test coverage despite being part of the public API.
The old doc said 'returns ok=false if the string doesn't match' and called out only sub-action paths as an example. A caller trying to understand why their string failed had nowhere to look. List all actual rejection conditions: missing separators, sub-action paths, unknown algorithm, wrong digest length, invalid hex. Also note that Ref casing is preserved while all other components are lowercased.
The error wrapped 'parsing action.yml' but the function takes a content string — it has no idea if the caller read from action.yml or action.yaml. Change to the format-agnostic 'parsing action metadata'. Also update the godoc to explicitly mention both file name variants and clarify that the file name is not needed (just pass the content).
SemVer.Rest said 'anything after patch' which is not useful — callers who check it need to know what a non-empty Rest implies. Add that Rest != "" means pre-release (see IsStable). Likewise IsStable's comment said 'no trailing junk' which is vague; make it concrete. IsMutable said 'partial tag that should be narrowed' without explaining WHY a partial tag is dangerous — point callers to the actual security concern (authors can silently move major/minor tags). Cross-link IsMutable↔IsFull since they are exact inverses.
The old doc used expert shorthand ('scopes per-dependency validation',
'fail-open by design') that means nothing to someone new to the library.
Specific improvements:
- Lead with what to pass: the lockfile bytes, with a pointer to Path.
- Separate the optional paths parameter into its own section with two
clearly labeled cases (omit = validate all; pass paths = limit scope).
- Explain WHY the paths parameter exists — so a corrupt unrelated entry
doesn't block the workflows you care about.
- Make the 'path not in lockfile' behavior explicit and explain it's
intentional (not-yet-onboarded workflow should not fail Parse).
- Rename 'Document-level invariants' to the plain 'Canonicalization'
section, clarify it's about case normalization, and cross-link
ParsePin and Pin.String.
pkgsite (and godoc) only hyperlink doc references to methods and package-level symbols — not struct fields. [File.Workflows] and [File.Dependencies] rendered as literal bracket-wrapped text instead of links. Replace all field references with plain field names; keep [File.LookupWorkflow] since it's a method and does render correctly.
The function only checked for a .yml/.yaml suffix, making it return true for non-local paths like '.github/workflows/ci.yml' (missing ./ prefix). This contradicts the documented contract which says the value must start with './'. Add the HasPrefix check. Also add test cases for the missing-prefix failure mode and other edge cases.
nodeselector
force-pushed
the
nodeselector/lockfile-eval-beginner-ux
branch
from
June 22, 2026 22:11
7b006f7 to
6511578
Compare
nodeselector
added a commit
that referenced
this pull request
Jul 8, 2026
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.
The package godoc was written from the perspective of someone who already knows the codebase. A new user trying to parse a lockfile had to hunt for the entry point, decode jargon ("fork-network signal"), and guess what to do with the values they got back. This is a systematic pass to fix that, touching every exported symbol with a UX problem.
What changed
Package entry point (doc.go) -- The package comment opened with
ParseActionRefas "the choke point" and never mentionedParse()orFile. Rewrote to lead with the actual entry point (Parse -> File -> LookupWorkflow), with security notes moved to a labeled contributor section.File struct fields --
WorkflowsandDependencieshad zero field-level doc. Added comments naming the key/value formats and pointing to LookupWorkflow.ParseError -- Doc mentioned yaml.v3 internals ("package prefix", "node tree walking") that mean nothing to a caller. Rewrote around the public contract: structured line/col for diagnostics, when Column is zero, how to use errors.As.
Action.Branch -- "fork-network signal" is opaque GitHub-internal jargon. Replaced with a plain explanation of why a branch is required (commit unreachable from any branch could belong to a fork, which SHA-only pinning can't detect).
LookupWorkflow -- Returned
[]stringwith no hint about the next step. Added a code example showing the -> Dependencies map lookup and clarified the ok=false vs empty-slice distinction.IsLocalReusableWorkflow -- Parameter named
localPathbut the function expects the rawuses:string with./intact. Renamed tolocalUses, rewrote doc with concrete true/false examples. AddedTestIsLocalReusableWorkflow-- this exported function had no test coverage.ParsePin -- Only mentioned sub-action paths as a rejection reason. Enumerated all five failure conditions (missing @, missing :, sub-action paths, unknown algo, wrong hex).
ParseActionMeta -- Error wrapped "parsing action.yml" but the function accepts both action.yml and action.yaml content (it never sees the filename). Changed to "parsing action metadata".
SemVer --
Restsaid "anything after patch" without saying what non-empty Rest implies.IsMutablesaid "should be narrowed" without explaining why partial tags are dangerous. Fixed: Rest != "" means pre-release; IsMutable explains that authors can silently move major/minor tags.Parse paths parameter -- Expert shorthand ("scopes per-dependency validation", "fail-open by design") replaced with a dedicated section covering both cases (omit = validate all; pass paths = limit scope).
Doc link fix -- Struct field doc links (
[File.Workflows]) don't render as hyperlinks in pkgsite/godoc -- only methods do. Replaced with plain text.SemVer dedup -- Removed an accidentally duplicated type doc comment block.
Notes
go test -race ./...green throughoutDependency.OwnerReporeference in nwo.go is pre-existing on main, not introduced here