Refactor import-input substitution to shared resolver/serializer utility#37455
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR consolidates duplicated “import-input” path resolution and value formatting logic previously split between pkg/parser and pkg/workflow into a shared utility package, then updates both call sites to use the shared implementation.
Changes:
- Added
pkg/importinpututilwith sharedResolvePathValueandFormatResolvedValuehelpers plus unit tests. - Updated workflow import-input substitution to delegate formatting/path resolution to the shared utility.
- Updated parser import-input substitution to delegate formatting/path resolution to the shared utility and removed duplicated helpers.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/expression_extraction.go | Switches import-input lookup/formatting to importinpututil during expression substitution. |
| pkg/parser/import_field_extractor.go | Switches import-input lookup/formatting to importinpututil and removes duplicated local helpers. |
| pkg/importinpututil/import_input.go | Introduces shared resolver/formatter implementation for import-input values. |
| pkg/importinpututil/import_input_test.go | Adds tests for shared resolver/formatter behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 3
| 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() | ||
| } | ||
| return normalized | ||
| } |
| func TestFormatResolvedValue(t *testing.T) { | ||
| tests := []struct { |
| {name: "typed map", value: map[string]string{"k": "v"}, want: `{"k":"v"}`, ok: true}, | ||
| {name: "nil value", value: nil, want: "", ok: false}, |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (163 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Even for a refactor, capturing why a new dedicated package was chosen over the alternatives (leaving the duplication, or hosting the helpers in an existing package) helps future contributors understand the dependency boundaries. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out, /improve-codebase-architecture, and /tdd — no blocking issues, commenting with improvement suggestions.
📋 Key Themes & Highlights
Key Themes
- Implicit contract coupling (
/zoom-out):marshalImportInputValueinexpression_extraction.gorelies onFormatResolvedValuereturningfalsefor nil, but the caller nil-checksvalueafter the call — making the behaviour depend on an undocumented internal detail of the utility. - Undocumented
falsesemantics (/improve-codebase-architecture):FormatResolvedValuereturns(string, false)for both nil input and json.Marshal failure; the two reasons are indistinguishable and undocumented. - Single-use wrapper (
/improve-codebase-architecture):resolveImportInputValuein the parser is now a one-line passthrough with a single call site. - Test coverage gaps (
/tdd): Multi-level dotted paths (e.g."a.b.c") and themarshalValuefailure branch are not tested.
Positive Highlights
- ✅ Clean deduplication — ~127 lines of identical logic removed across two packages with no behavioral regressions found.
- ✅
normalizeMappreserves key-ordering sort from both originals — behavioral parity maintained. - ✅ Tests cover the main happy paths and most failure modes for the new utility.
- ✅
FormatResolvedValue/ResolvePathValueare well-named and match existing domain vocabulary.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 734.5K · 220.3 AIC · ⌖ 13.3 AIC
| } | ||
| case nil: | ||
| if formatted, ok := importinpututil.FormatResolvedValue(value); ok { | ||
| return formatted |
There was a problem hiding this comment.
[/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.
| {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}, |
There was a problem hiding this comment.
[/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.
| value, ok := obj[sub] | ||
| return value, ok | ||
| } | ||
|
|
There was a problem hiding this comment.
[/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.
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
[/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.
| } | ||
| return normalized | ||
| return importinpututil.ResolvePathValue(inputs, inputPath) | ||
| } |
There was a problem hiding this comment.
[/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.
🧪 Test Quality Sentinel Report❌ Test Quality Score: 100/100 — Excellent (but check fails — see violation below)
📊 Metrics & Test Classification (2 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
There was a problem hiding this comment.
❌ Test Quality Sentinel: Coding-guideline violation detected. pkg/importinpututil/import_input_test.go is missing the required //go:build !integration build tag on line 1. All other *_test.go files in this repo (e.g. pkg/workflow/expression_extraction_test.go, pkg/stringutil/stringutil_test.go) begin with //go:build !integration. The tests themselves are excellent — table-driven, behavioral contracts, good edge-case coverage. Please add //go:build !integration as the first line of the file to unblock this PR.
There was a problem hiding this comment.
Refactor correctly deduplicates behaviour across both call sites with no regressions. Three non-blocking concerns worth addressing before this shared utility grows more callers.
### Themes
API design — FormatResolvedValue nil contract (medium)
The public function returns ("", false) for both nil input and json.Marshal failure, conflating two distinct outcomes under a single ok=false signal. Every caller must add a post-call nil check to distinguish them. See inline comment on line 41.
Latent panic in normalizeMap (medium)
rv.MapIndex(reflect.ValueOf(k)).Interface() will panic on a zero reflect.Value if the map has non-string keys. The bug pre-existed in both removed inline copies, but it now lives in a shared public function with no documented precondition. See inline comment on line 83.
Fragile nil guard in marshalImportInputValue (low)
The value == nil check on line 508 of expression_extraction.go is correct but implicitly depends on FormatResolvedValue continuing to return ("", false) — not ("", true) — for nil. Any future change to the nil handling in the shared util would silently break this without a compile error. See inline comment.
Test gaps in new package (low)
No tests for typed-nil asymmetry, multi-level dot paths, or the marshal-failure fallback path. See inline comment on the test file.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 422.4K · 8.77 AIC · ⌖ 13.4 AIC
| sort.Strings(keys) | ||
| normalized := make(map[string]any, rv.Len()) | ||
| for _, k := range keys { | ||
| normalized[k] = rv.MapIndex(reflect.ValueOf(k)).Interface() |
There was a problem hiding this comment.
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).
| case map[string]any: | ||
| return marshalValue(v) | ||
| case nil: | ||
| return "", false |
There was a problem hiding this comment.
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.
| if formatted, ok := importinpututil.FormatResolvedValue(value); ok { | ||
| return formatted | ||
| } | ||
| if value == nil { |
There was a problem hiding this comment.
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:
- Adding a comment here:
// value == nil is reachable because FormatResolvedValue returns (""‚ false) for nil rather than (""‚ true) - Or fixing the root cause: make
FormatResolvedValuereturn("", true)for nil so callers don't need a separate guard.
| t.Fatalf("FormatResolvedValue(%#v) = %q, want %q", tt.value, got, tt.want) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
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:
-
Multi-level dot path (
"a.b.c") — silently resolves asinputs["a"]["b.c"](two-level, not three), returning(nil, false). This should be documented and tested, since a future caller might expect recursive traversal. -
Marshal failure fallback — when
json.Marshalfails (e.g., achan intvalue),FormatResolvedValuereturns("", false). Callers rely on this to fall back tofmt.Sprintf. The expected fallback behavior should be tested. -
Typed nil —
(*int)(nil)passed asanydoes NOT match thecase nilbranch; it reachesformatReflectiveValuewhosedefaultpath returnsfmt.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},
The issue identified duplicated import-input path resolution and value serialization logic in
pkg/parserandpkg/workflow, creating a maintenance divergence risk. This PR consolidates that behavior into one shared implementation and updates both call sites to use it.Shared import-input utility
pkg/importinpututil/import_input.gowith:ResolvePathValue(inputs, path)for one-level dotted lookup (key/key.subkey)FormatResolvedValue(value)for normalization + JSON formatting of slices/maps (including typed variants), scalar formatting, and nil handling contract.Parser consolidation
pkg/parser/import_field_extractor.gonow delegates:importinpututil.ResolvePathValueimportinpututil.FormatResolvedValueWorkflow consolidation
pkg/workflow/expression_extraction.gonow delegates:importinpututil.ResolvePathValueimportinpututil.FormatResolvedValueCoverage for shared behavior
pkg/importinpututil/import_input_test.gofor: