Skip to content

Refactor import-input substitution to shared resolver/serializer utility #37455

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 3 commits into from
Jun 7, 2026
+163 −127
Merged

Refactor import-input substitution to shared resolver/serializer utility #37455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c7effbf
Initial plan
Copilot Jun 7, 2026
d1eac4e
refactor: share import-input resolve and formatting logic
Copilot Jun 7, 2026
5a73b1b
Merge branch 'main' into copilot/refactor-duplicate-serialization-logic
pelikhan Jun 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions pkg/importinpututil/import_input.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
package importinpututil

import (
"encoding/json"
"fmt"
"reflect"
"sort"
"strings"
)

// ResolvePathValue resolves either a top-level input key ("count") or a one-level
// dotted object sub-key ("config.apiKey") from import inputs.
func ResolvePathValue(inputs map[string]any, inputPath string) (any, bool) {
top, sub, hasDot := strings.Cut(inputPath, ".")
if !hasDot {
value, ok := inputs[top]
return value, ok
}
topVal, topOK := inputs[top]
if !topOK {
return nil, false
}
obj, isMap := topVal.(map[string]any)
if !isMap {
return nil, false
}
value, ok := obj[sub]
return value, ok
}

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Jun 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/improve-codebase-architecture] The (string, bool) return conflates two distinct failure modes: nil input and json.Marshal failure. Callers cannot distinguish them, which matters if they want to log errors or handle each case differently.

For now both call sites are fine (they treat both as "use fallback"), but the contract is undocumented.

💡 Suggestion

Amend the doc comment to make the false-case contract explicit:

// FormatResolvedValue formats a resolved import input value for textual
// substitution. Returns (formatted, true) on success. Returns ("", false) when
// value is nil or when JSON marshaling fails — callers should treat false as
// "no formatted output available" and apply their own fallback.
func FormatResolvedValue(value any) (string, bool) {

This avoids future callers misreading false as a hard error.

// FormatResolvedValue formats a resolved import input value for textual
// substitution. []any/map[string]any and typed slices/maps are normalized and
// JSON-marshaled, nil returns ("", false), and scalars use fmt.Sprintf("%v", v).
func FormatResolvedValue(value any) (string, bool) {
switch v := value.(type) {
case []any:
return marshalValue(v)
case map[string]any:
return marshalValue(v)
case nil:
return "", false
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Jun 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FormatResolvedValue conflates nil with marshal failure via ok=false: The bool return means two very different things.

💡 Details

There are three possible outcomes when calling FormatResolvedValue:

Scenario Return
Scalar / slice / map formatted successfully ("...", true)
nil input ("", false)
json.Marshal failure ("", false)

Case 2 and 3 are indistinguishable. Callers must add an extra value == nil guard after the call to preserve correct semantics — as marshalImportInputValue does. A naive caller who writes:

formatted, ok := FormatResolvedValue(v)
if !ok {
    // handle error case
}

...will silently swallow explicit null import inputs, treating them as errors.

A less footgun-prone design: return ("", true) for nil (nil → empty string is a deterministic, documented choice), and reserve false exclusively for actual errors (marshal failures). This removes the need for every caller to re-check nil.

If nil-as-absent is intentional semantics (matching current call-site behavior), the doc comment should say so explicitly, and the function should be named accordingly (e.g., FormatResolvedValueOrEmpty) to signal the two-failure-mode contract.

default:
return formatReflectiveValue(v)
}
}

func formatReflectiveValue(value any) (string, bool) {
rv := reflect.ValueOf(value)
switch rv.Kind() {
case reflect.Slice:
return marshalValue(normalizeSlice(rv))
case reflect.Map:
return marshalValue(normalizeMap(rv))
default:
return fmt.Sprintf("%v", value), true
}
}

func marshalValue(value any) (string, bool) {
b, err := json.Marshal(value)
if err != nil {
return "", false
}
return string(b), true
}

func normalizeSlice(rv reflect.Value) []any {
normalized := make([]any, rv.Len())
for i := range rv.Len() {
normalized[i] = rv.Index(i).Interface()
}
return normalized
}

func normalizeMap(rv reflect.Value) map[string]any {
keys := make([]string, 0, rv.Len())
for _, key := range rv.MapKeys() {
keys = append(keys, key.String())
}
sort.Strings(keys)
normalized := make(map[string]any, rv.Len())
for _, k := range keys {
normalized[k] = rv.MapIndex(reflect.ValueOf(k)).Interface()
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Jun 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latent panic in normalizeMap with non-string-keyed maps: rv.MapIndex(reflect.ValueOf(k)).Interface() will panic on zero-Value when the map has non-string keys.

💡 Details

When rv has a non-string key type (e.g., map[int]string), key.String() does not return the integer as a decimal string — for non-string reflect.Kinds it returns "<int Value>" or similar. The subsequent rv.MapIndex(reflect.ValueOf(k)) then looks up a string key in an int-keyed map and returns a zero reflect.Value. Calling .Interface() on a zero reflect.Value panics:

panic: reflect: call of reflect.Value.Interface on zero Value

This was a pre-existing bug in both removed inline copies, but those were private package functions. Now it lives in a shared public utility with no documented key-type precondition, making it easy to trigger from new callers.

Suggested guard:

func normalizeMap(rv reflect.Value) map[string]any {
    if rv.Type().Key().Kind() != reflect.String {
        // Non-string keys cannot be round-tripped through string lookup;
        // fall back to JSON default marshaling.
        return nil  // or handle differently
    }
    ...
}

Alternatively, add a doc comment to FormatResolvedValue stating it only handles map[string]any-like maps (matching YAML/JSON decode output).

}
return normalized
}
Comment on lines +75 to +86
67 changes: 67 additions & 0 deletions pkg/importinpututil/import_input_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package importinpututil

import "testing"

func TestResolvePathValue(t *testing.T) {
inputs := map[string]any{
"name": "alice",
"config": map[string]any{
"token": "abc",
},
"bad": "not-map",
}

tests := []struct {
name string
path string
want any
found bool
}{
{name: "top level", path: "name", want: "alice", found: true},
{name: "dotted path", path: "config.token", want: "abc", found: true},
{name: "missing top level", path: "missing", found: false},
{name: "missing dotted key", path: "config.missing", found: false},
{name: "dotted non map", path: "bad.token", found: false},
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Jun 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Multi-level path (e.g. "config.api.key") is not tested — the current behavior silently fails because strings.Cut produces "api.key" as the sub-key, which misses in the map.

This is documented as one-level-only in the doc comment, but a test makes the behavior explicit for future readers and guards against accidental relaxation.

💡 Suggested test case 
{name: "two-level path not supported", path: "config.token.nested", found: false},

Add this to the test table to nail down the contract.

}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, ok := ResolvePathValue(inputs, tt.path)
if ok != tt.found {
t.Fatalf("ResolvePathValue(%q) found = %v, want %v", tt.path, ok, tt.found)
}
if ok && got != tt.want {
t.Fatalf("ResolvePathValue(%q) = %#v, want %#v", tt.path, got, tt.want)
}
})
}
}

func TestFormatResolvedValue(t *testing.T) {
tests := []struct {
Comment on lines +40 to +41
name string
value any
want string
ok bool
}{
{name: "scalar string", value: "hello", want: "hello", ok: true},
{name: "scalar int", value: 42, want: "42", ok: true},
{name: "slice any", value: []any{"a", 1}, want: `["a",1]`, ok: true},
{name: "typed slice", value: []string{"x", "y"}, want: `["x","y"]`, ok: true},
{name: "map any", value: map[string]any{"k": "v"}, want: `{"k":"v"}`, ok: true},
{name: "typed map", value: map[string]string{"k": "v"}, want: `{"k":"v"}`, ok: true},
{name: "nil value", value: nil, want: "", ok: false},
Comment on lines +52 to +53
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, ok := FormatResolvedValue(tt.value)
if ok != tt.ok {
t.Fatalf("FormatResolvedValue(%#v) ok = %v, want %v", tt.value, ok, tt.ok)
}
if got != tt.want {
t.Fatalf("FormatResolvedValue(%#v) = %q, want %q", tt.value, got, tt.want)
}
})
}
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Jun 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test coverage for the failure path and edge cases: The test suite covers only the happy path — no test for marshal failure, multi-level dot paths, or typed nils.

💡 Suggested additions

Three important behaviors are unspecified and untested:

  1. Multi-level dot path ("a.b.c") — silently resolves as inputs["a"]["b.c"] (two-level, not three), returning (nil, false). This should be documented and tested, since a future caller might expect recursive traversal.

  2. Marshal failure fallback — when json.Marshal fails (e.g., a chan int value), FormatResolvedValue returns ("", false). Callers rely on this to fall back to fmt.Sprintf. The expected fallback behavior should be tested.

  3. Typed nil — (*int)(nil) passed as any does NOT match the case nil branch; it reaches formatReflectiveValue whose default path returns fmt.Sprintf("%v", v) = "<nil>". This asymmetry from untyped nil (→ "") is surprising and should be documented/tested.

// Suggested additions to TestFormatResolvedValue:
{name: "typed nil", value: (*int)(nil), want: "<nil>", ok: true},

// Suggested additions to TestResolvePathValue:
{name: "multi-level dot path", path: "config.token.sub", found: false},

}
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Jun 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] marshalValue returns ("", false) on json.Marshal error, but there is no test exercising that branch — it is dead from a coverage perspective.

In practice, standard Go types rarely fail marshaling, but channels and function values do. A test guarding this path makes the contract explicit.

💡 Suggested test case 
// Add to TestFormatResolvedValue:
{name: "unmarshalable channel", value: make(chan int), want: "", ok: false},

This explicitly documents and covers the marshal-failure path in marshalValue.

76 changes: 4 additions & 72 deletions pkg/parser/import_field_extractor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ import (
"fmt"
"maps"
"path/filepath"
"reflect"
"regexp"
"sort"
"strings"

"github.com/github/gh-aw/pkg/importinpututil"
)

// importAccumulator centralizes the builder/slice/set variables used during
Expand Down Expand Up @@ -1238,77 +1238,9 @@ func resolveImportInputPath(inputs map[string]any, inputPath string) (string, bo
if !ok {
return "", false
}
return formatResolvedImportInputValue(value)
return importinpututil.FormatResolvedValue(value)
}

func resolveImportInputValue(inputs map[string]any, inputPath string) (any, bool) {
top, sub, hasDot := strings.Cut(inputPath, ".")
if !hasDot {
value, ok := inputs[top]
return value, ok
}
topVal, topOK := inputs[top]
if !topOK {
return nil, false
}
obj, isMap := topVal.(map[string]any)
if !isMap {
return nil, false
}
value, ok := obj[sub]
return value, ok
}

func formatResolvedImportInputValue(value any) (string, bool) {
switch v := value.(type) {
case []any:
return marshalImportInputValue(v)
case map[string]any:
return marshalImportInputValue(v)
case nil:
return "", false
default:
return formatReflectiveImportInputValue(v)
}
}

func formatReflectiveImportInputValue(value any) (string, bool) {
rv := reflect.ValueOf(value)
switch rv.Kind() {
case reflect.Slice:
return marshalImportInputValue(normalizeSliceForImportInput(rv))
case reflect.Map:
return marshalImportInputValue(normalizeMapForImportInput(rv))
default:
return fmt.Sprintf("%v", value), true
}
}

func marshalImportInputValue(value any) (string, bool) {
b, err := json.Marshal(value)
if err != nil {
return "", false
}
return string(b), true
}

func normalizeSliceForImportInput(rv reflect.Value) []any {
normalized := make([]any, rv.Len())
for i := range rv.Len() {
normalized[i] = rv.Index(i).Interface()
}
return normalized
}

func normalizeMapForImportInput(rv reflect.Value) map[string]any {
keys := make([]string, 0, rv.Len())
for _, key := range rv.MapKeys() {
keys = append(keys, key.String())
}
sort.Strings(keys)
normalized := make(map[string]any, rv.Len())
for _, k := range keys {
normalized[k] = rv.MapIndex(reflect.ValueOf(k)).Interface()
}
return normalized
return importinpututil.ResolvePathValue(inputs, inputPath)
}
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Jun 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/improve-codebase-architecture] resolveImportInputValue is now a single-call passthrough with one call site — the indirection adds a hop without adding semantics.

Since this wrapper is only called once (line 1239), you can inline it into resolveImportInputPath to remove the layer:

💡 Inlined version 
func resolveImportInputPath(inputs map[string]any, inputPath string) (string, bool) {
	value, ok := importinpututil.ResolvePathValue(inputs, inputPath)
	if !ok {
		return "", false
	}
	return importinpututil.FormatResolvedValue(value)
}

If the wrapper is kept intentionally for future callers, a brief comment stating that intent would help.

61 changes: 6 additions & 55 deletions pkg/workflow/expression_extraction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,14 @@ package workflow
import (
"crypto/sha256"
"encoding/hex"
"encoding/json"
"fmt"
"os"
"reflect"
"regexp"
"sort"
"strings"

"github.com/github/gh-aw/pkg/console"
"github.com/github/gh-aw/pkg/importinpututil"
"github.com/github/gh-aw/pkg/logger"
)

Expand Down Expand Up @@ -503,45 +502,12 @@ func SubstituteImportInputs(content string, importInputs map[string]any) string
// goccy/go-yaml may produce typed slices (e.g. []string) instead of []any, so
// a reflection fallback converts any slice kind to []any before JSON marshaling.
func marshalImportInputValue(value any) string {
switch v := value.(type) {
case []any:
if b, err := json.Marshal(v); err == nil {
return string(b)
}
case map[string]any:
if b, err := json.Marshal(v); err == nil {
return string(b)
}
case nil:
if formatted, ok := importinpututil.FormatResolvedValue(value); ok {
return formatted
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Jun 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/zoom-out] This nil guard is now implicit dead code unless you know FormatResolvedValue returns false for nil — the coupling is invisible.

Because FormatResolvedValue(nil) already returns ("", false), the if value == nil branch is reached only by that one case. If the utility ever changes its nil behavior (e.g., returning ("null", true) for JSON semantics), this guard silently becomes unreachable and the caller behavior diverges.

💡 Suggested refactor

Document the dependency explicitly, or drive the nil case entirely from the utility:

func marshalImportInputValue(value any) string {
	if value == nil {
		return ""
	}
	if formatted, ok := importinpututil.FormatResolvedValue(value); ok {
		return formatted
	}
	return fmt.Sprintf("%v", value)
}

This makes the nil contract owned by the caller rather than delegated implicitly.

}
if value == nil {
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Jun 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fragile nil guard: implicit coupling to FormatResolvedValue's nil contract: This value == nil branch is only reachable because FormatResolvedValue returns ("", false) for nil, not ("", true).

💡 Details

If FormatResolvedValue ever changes nil handling (e.g., to return ("", true) for nil), this check becomes silently unreachable dead code — and the behavior change is invisible at this call site. Conversely, if nil handling is removed from FormatResolvedValue entirely, the fmt.Sprintf("%v", nil) fallback would silently produce "<nil>" in substituted templates instead of "".

The two functions are now tightly coupled through an undocumented nil-as-false contract. Consider either:

  1. Adding a comment here: // value == nil is reachable because FormatResolvedValue returns (""‚ false) for nil rather than (""‚ true)
  2. Or fixing the root cause: make FormatResolvedValue return ("", true) for nil so callers don't need a separate guard.

// Null import input — return empty string rather than panicking.
return ""
default:
// Handle typed slices (e.g. []string) that goccy/go-yaml may produce
// instead of []any, and typed maps.
rv := reflect.ValueOf(v)
switch rv.Kind() {
case reflect.Slice:
normalized := make([]any, rv.Len())
for i := range rv.Len() {
normalized[i] = rv.Index(i).Interface()
}
if b, err := json.Marshal(normalized); err == nil {
return string(b)
}
case reflect.Map:
keys := make([]string, 0, rv.Len())
for _, key := range rv.MapKeys() {
keys = append(keys, key.String())
}
sort.Strings(keys)
normalized := make(map[string]any, rv.Len())
for _, k := range keys {
normalized[k] = rv.MapIndex(reflect.ValueOf(k)).Interface()
}
if b, err := json.Marshal(normalized); err == nil {
return string(b)
}
}
}
return fmt.Sprintf("%v", value)
}
Expand All @@ -552,20 +518,5 @@ func marshalImportInputValue(value any) string {
// supporting one level of nesting as defined by import-schema object types.
// Returns the resolved value and true on success, or nil and false when the path is not found.
func resolveImportInputPath(importInputs map[string]any, path string) (any, bool) {
topKey, subKey, hasDot := strings.Cut(path, ".")
if !hasDot {
// Scalar: direct lookup
value, ok := importInputs[topKey]
return value, ok
}
// Object sub-key: one-level deep lookup
topValue, ok := importInputs[topKey]
if !ok {
return nil, false
}
if obj, ok := topValue.(map[string]any); ok {
value, ok := obj[subKey]
return value, ok
}
return nil, false
return importinpututil.ResolvePathValue(importInputs, path)
}
Loading