From 283660246b98f922b9767f85b63d011c766e9d0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Guimar=C3=A3es?= Date: Mon, 13 Apr 2026 16:48:42 +0100 Subject: [PATCH] feat: propagate step uses version inline comment to YAML output --- ...-step-uses-pin-comment-propagation-plan.md | 145 ++++++++++++++++++ provider/github/action/uses.go | 62 ++++++-- provider/github/action/uses_test.go | 10 +- provider/github/github_test.go | 45 ++++++ provider/github/parse_workflow.go | 12 +- provider/github/step/step.go | 1 + provider/github/step/stepparse.go | 4 +- provider/github/step/uses.go | 4 +- 8 files changed, 263 insertions(+), 20 deletions(-) create mode 100644 docs/plans/2026-04-13-feat-step-uses-pin-comment-propagation-plan.md diff --git a/docs/plans/2026-04-13-feat-step-uses-pin-comment-propagation-plan.md b/docs/plans/2026-04-13-feat-step-uses-pin-comment-propagation-plan.md new file mode 100644 index 0000000..3f417c2 --- /dev/null +++ b/docs/plans/2026-04-13-feat-step-uses-pin-comment-propagation-plan.md @@ -0,0 +1,145 @@ +--- +title: "feat: propagate pin inline comment from step uses version to YAML" +type: feat +status: completed +date: 2026-04-13 +--- + +# feat: propagate pin inline comment from step uses version to YAML + +## Problem Statement + +The `pin` command adds an inline `# vtag` comment to the `version` attribute of a `uses {}` block: + +```hcl +uses { + action = "actions/checkout" + version = "abc123sha..." # v4 +} +``` + +This comment is silently dropped during HCL→YAML conversion. The generated YAML: + +```yaml +uses: actions/checkout@abc123sha +``` + +Should instead be: + +```yaml +uses: actions/checkout@abc123sha # v4 +``` + +This mirrors what was done for `permissions` attributes, but the `uses` pipeline is structurally different — it uses `cty.Value` and typed structs, not `map[string]any`. + +## Root Cause + +`UsesListConfig.Parse()` combines `action` and `version` into a single `cty.StringVal("action@version")` string. The `version` attribute's trailing comment is never read — it's discarded before the combined string is formed. There is no slot to carry comment metadata in `cty.Value`. + +## Proposed Solution + +### 1. Extract comment from `version` expression (`provider/github/action/uses.go`) + +`UsesConfig.Version` is an `hcl.Expression`. Unlike `*hclsyntax.Attribute`, it doesn't have a `SrcRange` field, but it does implement `Range() hcl.Range` which provides `End.Byte`. Use this to read the trailing comment from the source file — same technique as `extractInlineComment`, adapted for `hcl.Expression`. + +Add a package-level helper: + +```go +func extractExprComment(expr hcl.Expression) string { + r := expr.Range() + if r.Filename == "" { + return "" + } + src, err := os.ReadFile(r.Filename) + if err != nil || int(r.End.Byte) >= len(src) { + return "" + } + rest := src[r.End.Byte:] + newline := bytes.IndexByte(rest, '\n') + if newline < 0 { + newline = len(rest) + } + tail := strings.TrimSpace(string(rest[:newline])) + if !strings.HasPrefix(tail, "#") { + return "" + } + return tail +} +``` + +### 2. Return comment from `UsesListConfig.Parse()` (same file) + +Change signature: + +```go +func (config *UsesListConfig) Parse(hv *hclparser.HCLVars) (cty.Value, string, error) +``` + +After resolving `version`, call `extractExprComment(c.Version)` and return the comment string alongside the `cty.Value`. + +### 3. Thread comment through step parse (`provider/github/step/`) + +**`step.go`** — add field to `Step`: + +```go +UsesComment string +``` + +**`uses.go`** — update `StepConfig.parseUses()` signature: + +```go +func (config *StepConfig) parseUses(hv *hclparser.HCLVars) (cty.Value, string, error) +``` + +Return the comment from `config.Uses.Parse(hv)`. + +**`stepparse.go`** — capture comment and set on `parsedStep`: + +```go +parsedUses, usesComment, err := config.parseUses(hv) +... +parsedStep.UsesComment = usesComment +``` + +### 4. Attach comment in YAML output + +Find where step maps are built and locate the `uses` key serialization. Attach `UsesComment` as a `LineComment` on the `yaml.v3` node for that key. + +> **Note:** Requires investigating the step→YAML path to confirm whether it goes through `toYAMLNode` (in which case `annotated` wrapping could work) or a separate marshaling path. If the step is serialized as `map[string]any`, wrap the `uses` value with `annotated{value: usesStr, comment: UsesComment}` before the map is passed to the YAML serializer. + +## Acceptance Criteria + +- [x] `version = "sha" # v4` in a `uses {}` block → `uses: action@sha # v4` in YAML +- [x] No comment on `version` → `uses: action@sha` (no trailing comment, no regression) +- [x] `action` without `version` → no comment attempted +- [x] Golden fixtures updated where steps use pinned SHAs with comments +- [x] Roundtrip stable: YAML → HCL → YAML produces same output +- [x] Tests in `provider/github/action/uses_test.go` and `provider/github/step/uses_test.go` cover comment propagation + +## Files + +- `provider/github/action/uses.go` — `extractExprComment`, updated `Parse` signature +- `provider/github/action/uses_test.go` — comment extraction and propagation tests +- `provider/github/step/step.go` — add `UsesComment string` to `Step` +- `provider/github/step/uses.go` — updated `parseUses` signature +- `provider/github/step/stepparse.go` — thread comment into `Step.UsesComment` +- Step YAML serialization path (TBD after investigation) +- Golden fixture YAML files (update where applicable) + +## Out of Scope + +- Comments on `action` attribute (only `version` carries the pin tag) +- Propagating comments from other `uses`-adjacent fields (`with`, `env`) +- Job-level `uses` (reusable workflows) — separate struct, separate path + +## Open Question + +Confirm step→YAML serialization path before implementing step 4. If steps go through `map[string]any` → `toYAMLNode`, the `annotated` wrapper handles it for free. If they are struct-marshaled separately, a custom marshaler or node builder is needed. + +## Sources + +- `provider/github/action/uses.go:73` — `UsesListConfig.Parse()` +- `provider/github/step/step.go:27` — `Step.Uses cty.Value` +- `provider/github/step/stepparse.go:81` — `config.parseUses(hv)` +- Related: `provider/github/parse_workflow.go:437` — `extractInlineComment` for `*hclsyntax.Attribute` +- Related plan: `docs/plans/2026-03-09-feat-cinzelrc-provider-config-precedence-plan.md` diff --git a/provider/github/action/uses.go b/provider/github/action/uses.go index 04ddc6f..1e2010c 100644 --- a/provider/github/action/uses.go +++ b/provider/github/action/uses.go @@ -4,8 +4,11 @@ package action import ( + "bytes" "errors" "fmt" + "os" + "strings" "github.com/hashicorp/hcl/v2" "github.com/yldio/cinzel/internal/hclparser" @@ -69,14 +72,49 @@ func (config *UsesConfig) parseVersion(hv *hclparser.HCLVars) (cty.Value, error) return hp.Result(), nil } -// Parse resolves the uses block into a single "action@version" cty string value. -func (config *UsesListConfig) Parse(hv *hclparser.HCLVars) (cty.Value, error) { +// extractExprComment reads the source file at the position immediately after +// the expression's range end and returns any trailing # comment on the same +// line, or empty string if none is present. +func extractExprComment(expr hcl.Expression) string { + if expr == nil { + return "" + } + + r := expr.Range() + if r.Filename == "" { + return "" + } + + src, err := os.ReadFile(r.Filename) + if err != nil || int(r.End.Byte) >= len(src) { + return "" + } + + rest := src[r.End.Byte:] + newline := bytes.IndexByte(rest, '\n') + + if newline < 0 { + newline = len(rest) + } + + tail := strings.TrimSpace(string(rest[:newline])) + + if !strings.HasPrefix(tail, "#") { + return "" + } + + return tail +} + +// Parse resolves the uses block into a single "action@version" cty string value +// and the inline comment on the version attribute (if any). +func (config *UsesListConfig) Parse(hv *hclparser.HCLVars) (cty.Value, string, error) { if config == nil { - return cty.NilVal, nil + return cty.NilVal, "", nil } if len(*config) != 1 { - return cty.NilVal, errors.New("should only exist one uses") + return cty.NilVal, "", errors.New("should only exist one uses") } c := (*config)[0] @@ -85,26 +123,30 @@ func (config *UsesListConfig) Parse(hv *hclparser.HCLVars) (cty.Value, error) { action, err := c.parseAction(hv) if err != nil { - return cty.NilVal, err + return cty.NilVal, "", err } if action != cty.NilVal { if err := parsedUses.parseAction(action); err != nil { - return cty.NilVal, err + return cty.NilVal, "", err } } else { - return cty.NilVal, errors.New("action must be set") + return cty.NilVal, "", errors.New("action must be set") } version, err := c.parseVersion(hv) if err != nil { - return cty.NilVal, err + return cty.NilVal, "", err } + comment := "" + if version != cty.NilVal { if err := parsedUses.parseVersion(version); err != nil { - return cty.NilVal, err + return cty.NilVal, "", err } + + comment = extractExprComment(c.Version) } var uses string @@ -115,5 +157,5 @@ func (config *UsesListConfig) Parse(hv *hclparser.HCLVars) (cty.Value, error) { uses = parsedUses.Action.AsString() } - return cty.StringVal(uses), nil + return cty.StringVal(uses), comment, nil } diff --git a/provider/github/action/uses_test.go b/provider/github/action/uses_test.go index 2dbc0c1..3501861 100644 --- a/provider/github/action/uses_test.go +++ b/provider/github/action/uses_test.go @@ -39,7 +39,7 @@ func TestUses(t *testing.T) { hv := hclparser.NewHCLVars() - val, err := config.Parse(hv) + val, _, err := config.Parse(hv) if err != nil { t.FailNow() } @@ -66,7 +66,7 @@ func TestUses(t *testing.T) { hv := hclparser.NewHCLVars() - val, err := config.Parse(hv) + val, _, err := config.Parse(hv) if err != nil { t.Fatal(err) } @@ -94,7 +94,7 @@ func TestUses(t *testing.T) { hv := hclparser.NewHCLVars() - val, err := config.Parse(hv) + val, _, err := config.Parse(hv) if err.Error() != expected { t.FailNow() @@ -131,7 +131,7 @@ func TestUses(t *testing.T) { hv := hclparser.NewHCLVars() - val, err := config.Parse(hv) + val, _, err := config.Parse(hv) if err.Error() != expected { t.FailNow() @@ -168,7 +168,7 @@ func TestUses(t *testing.T) { hv := hclparser.NewHCLVars() - val, err := config.Parse(hv) + val, _, err := config.Parse(hv) if err.Error() != expected { t.FailNow() diff --git a/provider/github/github_test.go b/provider/github/github_test.go index 8b8cd25..362b6dd 100644 --- a/provider/github/github_test.go +++ b/provider/github/github_test.go @@ -1260,3 +1260,48 @@ workflow "ci" { } }) } + +func TestParseStepUsesComment(t *testing.T) { + hclInput := ` +step "checkout" { + uses { + action = "actions/checkout" + version = "abc1234abc1234abc1234abc1234abc1234abc123" # v4 + } +} + +job "build" { + runs_on { runners = "ubuntu-latest" } + steps = [step.checkout] +} + +workflow "ci" { + filename = "ci" + on "push" {} + jobs = [job.build] +} +` + + tmpDir := t.TempDir() + inputFile := filepath.Join(tmpDir, "workflow.hcl") + outputDir := filepath.Join(tmpDir, "out") + + if err := os.WriteFile(inputFile, []byte(hclInput), 0o644); err != nil { + t.Fatal(err) + } + + if err := New().Parse(provider.ProviderOps{File: inputFile, OutputDirectory: outputDir}); err != nil { + t.Fatal(err) + } + + out, err := os.ReadFile(filepath.Join(outputDir, "ci.yaml")) + if err != nil { + t.Fatal(err) + } + + yaml := string(out) + + if !strings.Contains(yaml, "uses: actions/checkout@abc1234abc1234abc1234abc1234abc1234abc123 # v4") { + t.Errorf("expected inline comment on uses line\ngot:\n%s", yaml) + } +} diff --git a/provider/github/parse_workflow.go b/provider/github/parse_workflow.go index 33fa8c8..10e6b67 100644 --- a/provider/github/parse_workflow.go +++ b/provider/github/parse_workflow.go @@ -465,7 +465,7 @@ func parseNamedConfig(cfg hclNamedBlock, hv *hclparser.HCLVars) (string, any, er func parseUsesBlockFromConfig(cfg hclUsesBlock, hv *hclparser.HCLVars) (string, error) { list := action.UsesListConfig{{Action: cfg.Action, Version: cfg.Version}} - val, err := list.Parse(hv) + val, _, err := list.Parse(hv) if err != nil { return "", err } @@ -657,7 +657,7 @@ func parseUsesBlock(body hcl.Body, hv *hclparser.HCLVars) (string, error) { } list := action.UsesListConfig{cfg} - val, err := list.Parse(hv) + val, _, err := list.Parse(hv) if err != nil { return "", err } @@ -862,6 +862,14 @@ func stepsToMap(steps step.Steps) (map[string]any, error) { return nil, err } + if parsedStep.UsesComment != "" { + if m, ok := converted.(map[string]any); ok { + if usesVal, exists := m["uses"]; exists { + m["uses"] = annotated{value: usesVal, comment: parsedStep.UsesComment} + } + } + } + out[stepID] = converted } diff --git a/provider/github/step/step.go b/provider/github/step/step.go index b6e4173..bf79a2c 100644 --- a/provider/github/step/step.go +++ b/provider/github/step/step.go @@ -25,6 +25,7 @@ type Step struct { If cty.Value `yaml:"if,omitempty" hcl:"if"` Name cty.Value `yaml:"name,omitempty" hcl:"name"` Uses cty.Value `yaml:"uses,omitempty" hcl:"uses"` + UsesComment string `yaml:"-"` Run cty.Value `yaml:"run,omitempty" hcl:"run"` WorkingDirectory cty.Value `yaml:"working-directory,omitempty" hcl:"working_directory"` Shell cty.Value `yaml:"shell,omitempty" hcl:"shell"` diff --git a/provider/github/step/stepparse.go b/provider/github/step/stepparse.go index 26f78f6..8cc81bd 100644 --- a/provider/github/step/stepparse.go +++ b/provider/github/step/stepparse.go @@ -78,7 +78,7 @@ func (config *StepConfig) Parse(hv *hclparser.HCLVars) (Step, error) { } } - parsedUses, err := config.parseUses(hv) + parsedUses, usesComment, err := config.parseUses(hv) if err != nil { return Step{}, fmt.Errorf("error in step '%s': %w, %w", parsedStep.Identifier, err, cinzelerror.ErrOpenIssue) } @@ -87,6 +87,8 @@ func (config *StepConfig) Parse(hv *hclparser.HCLVars) (Step, error) { if err := parsedStep.parseUses(parsedUses); err != nil { return Step{}, err } + + parsedStep.UsesComment = usesComment } parsedRun, err := config.parseRun(hv) diff --git a/provider/github/step/uses.go b/provider/github/step/uses.go index 6e85c8a..0ee96c2 100644 --- a/provider/github/step/uses.go +++ b/provider/github/step/uses.go @@ -21,9 +21,9 @@ func (s *Step) parseUses(value cty.Value) error { } } -func (config *StepConfig) parseUses(hv *hclparser.HCLVars) (cty.Value, error) { +func (config *StepConfig) parseUses(hv *hclparser.HCLVars) (cty.Value, string, error) { if config.Uses == nil { - return cty.NilVal, nil + return cty.NilVal, "", nil } return config.Uses.Parse(hv)