From 1c0a2584b75ceb9f00e8adb6066b858ffd5690f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Guimar=C3=A3es?= Date: Fri, 10 Apr 2026 17:56:18 +0100 Subject: [PATCH 1/2] feat: propagate HCL inline comments on attributes to YAML output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduces an `annotated` wrapper type that threads trailing `# comments` from HCL attributes through the `map[string]any` pipeline. `toYAMLNode` unwraps it and attaches the comment as a `LineComment` on the YAML node, so any attribute-level comment in HCL appears inline in the generated YAML. Validators that type-assert map values use `unwrapAnnotatedMap` to strip the wrapper before checking. The permissions-specific `parsePermissionsBlock` and `permissionsMapNode` helpers are removed — the general mechanism covers that case automatically. --- provider/github/github_test.go | 15 +++++++++++ provider/github/parse_workflow.go | 39 +++++++++++++++++++++++++++- provider/github/validate.go | 8 ++++++ provider/github/workflow_yaml.go | 43 +++++++++++++++++++++++++++++-- 4 files changed, 102 insertions(+), 3 deletions(-) diff --git a/provider/github/github_test.go b/provider/github/github_test.go index eeb1e61..8b8cd25 100644 --- a/provider/github/github_test.go +++ b/provider/github/github_test.go @@ -1244,4 +1244,19 @@ workflow "ci" { t.Errorf("expected contents: read in output\ngot:\n%s", yaml) } }) + + t.Run("inline comments on permission scopes appear in YAML output", func(t *testing.T) { + yaml := parseAndReadYAML(t, minimalWorkflow(`permissions { + contents = "read" # only needed for private repos + actions = "read" # required by the action + }`)) + + if !strings.Contains(yaml, "contents: read # only needed for private repos") { + t.Errorf("expected inline comment on contents\ngot:\n%s", yaml) + } + + if !strings.Contains(yaml, "actions: read # required by the action") { + t.Errorf("expected inline comment on actions\ngot:\n%s", yaml) + } + }) } diff --git a/provider/github/parse_workflow.go b/provider/github/parse_workflow.go index 6923f97..33fa8c8 100644 --- a/provider/github/parse_workflow.go +++ b/provider/github/parse_workflow.go @@ -4,8 +4,11 @@ package github import ( + "bytes" "errors" "fmt" + "os" + "strings" "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/gohcl" @@ -399,6 +402,34 @@ func parseWorkflowConfig(cfg hclWorkflowBlock, hv *hclparser.HCLVars) (map[strin return out, nil } +// extractInlineComment reads the source file and returns the trailing # comment +// on the same line as the attribute, or empty string if none is present. +func extractInlineComment(attr *hclsyntax.Attribute) string { + if attr.SrcRange.Filename == "" { + return "" + } + + src, err := os.ReadFile(attr.SrcRange.Filename) + if err != nil || int(attr.SrcRange.End.Byte) >= len(src) { + return "" + } + + rest := src[attr.SrcRange.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 +} + func setOptionalYAMLAttr(out map[string]any, yamlKey string, expr hcl.Expression, hv *hclparser.HCLVars) error { val, err := parseAttr(expr, hv) if err != nil { @@ -488,7 +519,13 @@ func parseBodyMap(body hcl.Body, hv *hclparser.HCLVars, scope string) (map[strin return nil, err } - out[naming.ToYAMLKey(name)] = val + yamlKey := naming.ToYAMLKey(name) + + if c := extractInlineComment(attr); c != "" { + out[yamlKey] = annotated{value: val, comment: c} + } else { + out[yamlKey] = val + } } } diff --git a/provider/github/validate.go b/provider/github/validate.go index 0d62eb8..fbca7bb 100644 --- a/provider/github/validate.go +++ b/provider/github/validate.go @@ -142,6 +142,10 @@ func validateParsedWorkflow(workflow ghworkflow.Parsed) error { // Validate workflow-level permissions. if perms, ok := workflow.Body["permissions"]; ok { + if permsMap, ok := perms.(map[string]any); ok { + perms = unwrapAnnotatedMap(permsMap) + } + if err := ghworkflow.ValidatePermissions(perms); err != nil { return withPath("workflow."+workflow.ID+".permissions", err) } @@ -182,6 +186,10 @@ func validateParsedJobs(jobs map[string]ghjob.Parsed) error { // Validate job-level permissions. if perms, ok := job.Body["permissions"]; ok { + if permsMap, ok := perms.(map[string]any); ok { + perms = unwrapAnnotatedMap(permsMap) + } + if err := ghworkflow.ValidatePermissions(perms); err != nil { return withPath("job."+id+".permissions", err) } diff --git a/provider/github/workflow_yaml.go b/provider/github/workflow_yaml.go index 9f32eb8..24f871e 100644 --- a/provider/github/workflow_yaml.go +++ b/provider/github/workflow_yaml.go @@ -88,8 +88,7 @@ func workflowMapNode(workflow map[string]any) (*yamlv3.Node, error) { seen := map[string]struct{}{} - // "jobsOrder" is a private sentinel set by the parser to preserve the - // HCL-defined job sequence; it must never appear in the YAML output. + // "jobsOrder" is a private sentinel that must never appear in the YAML output. jobOrder, _ := workflow["jobsOrder"].([]string) seen["jobsOrder"] = struct{}{} @@ -152,8 +151,48 @@ func appendMappingPair(node *yamlv3.Node, key string, value any) error { return nil } +// annotated wraps a value with an optional inline YAML comment. +// It is used to thread HCL trailing # comments through the map[string]any +// pipeline so that toYAMLNode can attach them as LineComment on the output node. +type annotated struct { + value any + comment string +} + +// unwrapAnnotated returns the inner value if v is annotated, otherwise v itself. +// Use this before type-asserting values that may have been produced by parseBodyMap. +func unwrapAnnotated(v any) any { + if a, ok := v.(annotated); ok { + return a.value + } + + return v +} + +// unwrapAnnotatedMap returns a copy of m with all annotated values replaced by +// their inner values. Use before passing maps to validators that do not know +// about the annotated wrapper. +func unwrapAnnotatedMap(m map[string]any) map[string]any { + out := make(map[string]any, len(m)) + + for k, v := range m { + out[k] = unwrapAnnotated(v) + } + + return out +} + func toYAMLNode(value any) (*yamlv3.Node, error) { switch v := value.(type) { + case annotated: + node, err := toYAMLNode(v.value) + if err != nil { + return nil, err + } + + node.LineComment = v.comment + + return node, nil case nil: return &yamlv3.Node{Kind: yamlv3.ScalarNode, Tag: "!!null", Value: "null"}, nil case string: From 85725eaf98075c487b6d879396c2223231b2da3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Guimar=C3=A3es?= Date: Fri, 10 Apr 2026 18:00:58 +0100 Subject: [PATCH 2/2] docs: document HCL inline comment propagation to YAML output --- .../hcl-inline-comment-propagation-to-yaml.md | 196 ++++++++++++++++++ 1 file changed, 196 insertions(+) create mode 100644 docs/solutions/logic-errors/hcl-inline-comment-propagation-to-yaml.md diff --git a/docs/solutions/logic-errors/hcl-inline-comment-propagation-to-yaml.md b/docs/solutions/logic-errors/hcl-inline-comment-propagation-to-yaml.md new file mode 100644 index 0000000..8e00c32 --- /dev/null +++ b/docs/solutions/logic-errors/hcl-inline-comment-propagation-to-yaml.md @@ -0,0 +1,196 @@ +--- +title: "HCL inline comments not propagated to YAML output" +problem_type: feature_implementation +component: provider/github +symptoms: + - HCL inline comments on attributes silently dropped in generated YAML + - Feature initially scoped only to permissions blocks, leaving other attributes without comment propagation + - Type assertion error after wrapping values: `permissions scope "actions" must have a string value` +tags: + - hcl + - yaml + - comments + - permissions + - type-assertion + - annotated + - parse + - workflow +date: 2026-04-10 +status: solved +--- + +## Problem + +HCL attributes with trailing inline comments: + +```hcl +permissions { + contents = "read" # only needed for private repos + actions = "read" # required by the action +} +``` + +produced YAML with the comments silently dropped: + +```yaml +permissions: + contents: read + actions: read +``` + +The initial fix introduced a permissions-specific mechanism (`parsePermissionsBlock` + a `permissionsComments` sentinel key + `permissionsMapNode`), but this was bespoke, duplicated logic, and didn't cover any other HCL attribute. + +--- + +## Root Cause + +The parse pipeline converts HCL to a `map[string]any` that flows through validation and ultimately into the `yaml.v3` node serializer. The intermediate map representation has no slot for comment metadata — values are plain Go types (`string`, `bool`, `map[string]any`, etc.) — so any comment extracted from the HCL source was immediately discarded. + +--- + +## Solution + +### 1. Introduce an `annotated` wrapper type (`provider/github/workflow_yaml.go`) + +A lightweight struct that carries a comment alongside any value through the `map[string]any` pipeline: + +```go +type annotated struct { + value any + comment string +} +``` + +Two helpers strip the wrapper at validation boundaries: + +```go +func unwrapAnnotated(v any) any { + if a, ok := v.(annotated); ok { + return a.value + } + return v +} + +func unwrapAnnotatedMap(m map[string]any) map[string]any { + out := make(map[string]any, len(m)) + for k, v := range m { + out[k] = unwrapAnnotated(v) + } + return out +} +``` + +### 2. Handle `annotated` in `toYAMLNode` (same file) + +The terminal serializer unwraps the value and attaches the comment as a `LineComment` on the `yaml.v3` node — which renders as a trailing `# comment` on the same output line: + +```go +case annotated: + node, err := toYAMLNode(v.value) + if err != nil { + return nil, err + } + node.LineComment = v.comment + return node, nil +``` + +### 3. Wrap at parse time in `parseBodyMap` (`provider/github/parse_workflow.go`) + +In the `default` attribute case, check for a trailing comment and wrap if present: + +```go +default: + val, err := parseAttr(attr.Expr, hv) + if err != nil { + return nil, err + } + yamlKey := naming.ToYAMLKey(name) + if c := extractInlineComment(attr); c != "" { + out[yamlKey] = annotated{value: val, comment: c} + } else { + out[yamlKey] = val + } +``` + +`extractInlineComment` reads the source file using `attr.SrcRange.End.Byte` to locate the byte immediately after the attribute value, then scans the rest of the line for a `#` prefix. + +### 4. Simplify permissions parsing + +With `parseBodyMap` now handling comments, the permissions-specific code is redundant. Replace `parsePermissionsBlock` with a plain `parseBodyMap` call: + +```go +for _, block := range cfg.PermBlocks { + child, err := parseBodyMap(block.Body, hv, "permissions") + if err != nil { + return nil, err + } + out["permissions"] = child +} +``` + +This removes `parsePermissionsBlock`, `permissionsMapNode`, the `permissionsComments` sentinel key, and the special-case branch in `workflowMapNode`. + +### 5. Fix validators (`provider/github/validate.go`) + +`ValidatePermissions` does `levelRaw.(string)` on map values. After wrapping, this assertion fails. Unwrap before validating at both workflow and job level: + +```go +if perms, ok := workflow.Body["permissions"]; ok { + if permsMap, ok := perms.(map[string]any); ok { + perms = unwrapAnnotatedMap(permsMap) + } + if err := ghworkflow.ValidatePermissions(perms); err != nil { + return withPath("workflow."+workflow.ID+".permissions", err) + } +} +``` + +--- + +## Why the General Approach Over the Permissions-Specific One + +| Permissions-specific | General `annotated` | +|---|---| +| Magic `permissionsComments` sentinel key pollutes map | No sentinel; comment travels with the value | +| Requires `permissionsMapNode` rendering path | `toYAMLNode` handles it in one new case | +| Only permissions attributes carry comments | Any HCL attribute with `# comment` propagates | +| More code, less reuse | Less code, works for free on future attributes | + +--- + +## Error Encountered During Implementation + +``` +error in workflow 'ci': workflow.ci.permissions: permissions scope "actions" must have a string value +``` + +`ValidatePermissions` iterates `map[string]any` and asserts each value to `string`. After the `annotated` wrapper was introduced, the assertion received an `annotated` struct instead of a plain string. Fixed by calling `unwrapAnnotatedMap` before validation. + +--- + +## Prevention + +### Pitfalls + +- **`map[string]any` type assertions are silent**: Any code doing `v.(string)` on pipeline values breaks when a new wrapper type is introduced. The compiler gives no warning; it only surfaces at runtime with specific inputs. +- **Blast radius is invisible**: You cannot grep to find all consumers of pipeline map values statically — every function that touches the map may need updating. +- **Validator/serializer ordering**: Unwrapping must happen before validation. This ordering is load-bearing but usually implicit. + +### Testing Strategy + +- For every function that type-asserts pipeline map values, add a parallel test that passes `annotated`-wrapped values alongside plain values. +- Include a roundtrip test that injects commented HCL attributes and asserts they appear as inline comments in the generated YAML. + +### Design Guidance + +- **Unwrap at a defined boundary**: establish one normalisation step (e.g. `unwrapAnnotatedMap`) that all validators call, rather than unwrapping ad hoc. +- **Use `annotated` when**: metadata must travel with the value through the pipeline and the consumer count is small and controlled. +- **Use a separate metadata map when**: consumers are numerous or externally defined — validators never need to see the metadata at all. + +--- + +## Related + +- [`github-pin-comment-placement-and-empty-permissions-default.md`](./github-pin-comment-placement-and-empty-permissions-default.md) — documents the sentinel-swap pattern for `permissions: {}` and inline `# vtag` comments on version lines +- [`yaml-map-key-order-lost-use-node-api.md`](./yaml-map-key-order-lost-use-node-api.md) — documents `yaml.v3` Node API usage for order-preserving serialization +- [`preserve-hcl-job-order-in-yaml-output.md`](./preserve-hcl-job-order-in-yaml-output.md) — documents the `jobsOrder` sentinel key pattern, a predecessor to the `annotated` approach