diff --git a/.github/workflows/pr-code-quality-reviewer.lock.yml b/.github/workflows/pr-code-quality-reviewer.lock.yml index a4094776a21..91b17b89fa8 100644 --- a/.github/workflows/pr-code-quality-reviewer.lock.yml +++ b/.github/workflows/pr-code-quality-reviewer.lock.yml @@ -1,4 +1,4 @@ -# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"1915423b7590fd2377cb225f67fe7d9de87e69bc461aca7e160c1470fa845af1","body_hash":"d66144ce2443a9def8e7c4286113d8fb5c2fbac95fe62f367ef8cb668b9527c9","strict":true,"agent_id":"copilot","engine_versions":{"copilot":"1.0.77","copilot-sdk":"1.0.8"}} +# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"1915423b7590fd2377cb225f67fe7d9de87e69bc461aca7e160c1470fa845af1","body_hash":"23fad5f911121efe38aabc755ccc0804905411b87387d1565ffd987fb1837fcd","strict":true,"agent_id":"copilot","engine_versions":{"copilot":"1.0.77","copilot-sdk":"1.0.8"}} # gh-aw-manifest: {"version":1,"secrets":["COPILOT_GITHUB_TOKEN","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GH_AW_OTEL_GRAFANA_AUTHORIZATION","GH_AW_OTEL_GRAFANA_ENDPOINT","GH_AW_OTEL_SENTRY_AUTHORIZATION","GH_AW_OTEL_SENTRY_ENDPOINT","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/cache/restore","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/cache/save","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/checkout","sha":"3d3c42e5aac5ba805825da76410c181273ba90b1","version":"v7.0.1"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-node","sha":"820762786026740c76f36085b0efc47a31fe5020","version":"v7.0.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.27.43","digest":"sha256:04e2d1987a565000a8f114b89d806ae7a3864dd4f944be65275b28c93d8690e6","pinned_image":"ghcr.io/github/gh-aw-firewall/agent:0.27.43@sha256:04e2d1987a565000a8f114b89d806ae7a3864dd4f944be65275b28c93d8690e6"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.43","digest":"sha256:d85f57975af5ea23af4996e41ed73fbc8f5b4a47402472bfe82e508f352cb0c1","pinned_image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.43@sha256:d85f57975af5ea23af4996e41ed73fbc8f5b4a47402472bfe82e508f352cb0c1"},{"image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.27.43","digest":"sha256:65c45ea2967984d0024f3df61bc71335658a77ede96c8d9665da7a5f33a795ab","pinned_image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.27.43@sha256:65c45ea2967984d0024f3df61bc71335658a77ede96c8d9665da7a5f33a795ab"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.27.43","digest":"sha256:26be5e0b8c8f4c41c8a59126b29bb5d80b07253597472ded2a16bdd75abcbf9d","pinned_image":"ghcr.io/github/gh-aw-firewall/squid:0.27.43@sha256:26be5e0b8c8f4c41c8a59126b29bb5d80b07253597472ded2a16bdd75abcbf9d"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.4.7","digest":"sha256:7545220a9aca134b71e51193ee0eaf4c50756ebf8fbd25a63ae7556e62815c00","pinned_image":"ghcr.io/github/gh-aw-mcpg:v0.4.7@sha256:7545220a9aca134b71e51193ee0eaf4c50756ebf8fbd25a63ae7556e62815c00"},{"image":"ghcr.io/github/gh-aw-node","digest":"sha256:a8082161d7dceda14b68f32eb39d0eaa96b825d07f5895b096afab9d9e0c7748","pinned_image":"ghcr.io/github/gh-aw-node@sha256:a8082161d7dceda14b68f32eb39d0eaa96b825d07f5895b096afab9d9e0c7748"},{"image":"ghcr.io/github/github-mcp-server:v1.8.0","digest":"sha256:d5a18c04b92714c309eb46a2305087e91a4dbd80420f6e462656699f95093520","pinned_image":"ghcr.io/github/github-mcp-server:v1.8.0@sha256:d5a18c04b92714c309eb46a2305087e91a4dbd80420f6e462656699f95093520"}],"has_pull_request":true} # This file was automatically generated by gh-aw. DO NOT EDIT. To debug this workflow, load the skill at https://github.com/github/gh-aw/blob/main/debug.md # diff --git a/.github/workflows/pr-code-quality-reviewer.md b/.github/workflows/pr-code-quality-reviewer.md index 29349af0913..d3597db76cb 100644 --- a/.github/workflows/pr-code-quality-reviewer.md +++ b/.github/workflows/pr-code-quality-reviewer.md @@ -181,7 +181,7 @@ Use `COMMENT` when all findings are non-blocking. Keep the overall review body c ## agent: `grumpy-coder` --- description: Hyper-critical senior reviewer that aggressively finds merge-blocking issues in changed lines -model: small +model: claude-haiku-4.5 --- You are a grumpy senior engineer doing a hostile first-pass code review. diff --git a/docs/adr/49746-split-map-extraction-helpers-into-lookup-go.md b/docs/adr/49746-split-map-extraction-helpers-into-lookup-go.md new file mode 100644 index 00000000000..c17c3203b20 --- /dev/null +++ b/docs/adr/49746-split-map-extraction-helpers-into-lookup-go.md @@ -0,0 +1,44 @@ +# ADR-49746: Split Map-Extraction Helpers into lookup.go + +**Date**: 2026-08-02 +**Status**: Accepted +**Deciders**: pelikhan, copilot-swe-agent + +--- + +### Context + +`pkg/typeutil/convert.go` grew to contain two unrelated concerns: safe single-value numeric conversion and keyed `map[string]any` extraction. The four map-extraction functions (`ParseBool`, `LookupMap`, `LookupString`, `LookupStringPath`) lived alongside numeric helpers but were not described in the file-level package doc, creating visible documentation drift. Callers of the map-extraction API had no clear signal that these utilities existed or where to look for them. + +### Decision + +We will move the four map-extraction functions (`ParseBool`, `LookupMap`, `LookupString`, `LookupStringPath`) from `convert.go` into a new file `lookup.go` within the same `typeutil` package. The new file carries a focused file-level doc block describing the map-extraction cluster. `convert.go` gains a cross-reference to `lookup.go` and narrows its package doc to numeric conversions only. + +This is a pure reorganization — no signatures or behavior change. All callers and tests are unaffected. + +### Alternatives Considered + +#### Alternative 1: Keep all functions in convert.go (status quo) + +Leave `ParseBool`, `LookupMap`, `LookupString`, and `LookupStringPath` in `convert.go` alongside numeric helpers. Requires no file changes. Rejected because the mixed concerns make the file harder to navigate and the map-extraction group remains undiscoverable via the file-level doc. + +#### Alternative 2: Extract map-extraction helpers into a sub-package (e.g., pkg/typeutil/lookup/) + +Move the four functions into a child package, giving them their own import path. This would enforce the separation at the compiler level. Rejected because it would require import-path changes in all callers, adds package-boundary overhead (exported identifiers already have the `typeutil.` qualifier), and is disproportionate to a four-function cluster that is logically still part of the same utility layer. + +### Consequences + +#### Positive +- Each file in `pkg/typeutil/` has a single focused responsibility, reducing cognitive load when navigating the package. +- Map-extraction functions are discoverable via `lookup.go`'s dedicated file-level doc block. +- The `README.md` gains a dedicated **Map Extraction** section and an updated function-choice table, improving package-level documentation. + +#### Negative +- The `typeutil` package now spans two files; contributors must know that map helpers live in `lookup.go`, not `convert.go`. The cross-reference comment in `convert.go` and the README mitigate but do not eliminate this discovery cost. +- Pure reorganization carries a small risk of merge conflicts if concurrent PRs add functions to `convert.go` in the moved region. + +#### Neutral +- No change to the public API surface, test coverage, or runtime behavior. +- The `docs/adr/` naming convention uses the PR number as the ADR number, consistent with other ADRs in this repository. + +--- diff --git a/pkg/typeutil/README.md b/pkg/typeutil/README.md index 5b6768a059b..47e941a5940 100644 --- a/pkg/typeutil/README.md +++ b/pkg/typeutil/README.md @@ -6,9 +6,11 @@ JSON and YAML parsers produce `any` values whose concrete type varies at runtime (`int`, `float64`, `string`, etc.). This package provides safe, well-documented conversion functions that handle the common cases without requiring callers to write their own type switches. -Functions are grouped into three categories: **strict conversions** (return a `(value, ok)` pair to distinguish zero from missing/invalid), **safe overflow conversions** (clamp to zero on overflow instead of panicking), and **lenient conversions** (also handle string inputs, returning zero on failure). A separate group handles token-limit strings with optional `K`/`M` multiplier suffixes. +Functions are grouped into four categories: **strict conversions** (return a `(value, ok)` pair to distinguish zero from missing/invalid), **safe overflow conversions** (clamp to zero on overflow instead of panicking), **lenient conversions** (also handle string inputs, returning zero on failure), and **map-extraction helpers** (pull typed values from `map[string]any` by key or path). A separate group handles token-limit strings with optional `K`/`M` multiplier suffixes. -Choose the right category based on the source: use strict conversions for YAML config fields where the YAML library has already typed the value; use lenient conversions for heterogeneous sources such as JSON metrics or log-parsed data where a zero default on failure is acceptable. +Choose the right category based on the source: use strict conversions for YAML config fields where the YAML library has already typed the value; use lenient conversions for heterogeneous sources such as JSON metrics or log-parsed data where a zero default on failure is acceptable; use map-extraction helpers when working with JSON/YAML-decoded `map[string]any` structures. + +**File layout**: numeric conversion functions live in `convert.go`; map-extraction helpers (`ParseBool`, `LookupMap`, `LookupString`, `LookupStringPath`) live in `lookup.go`. ## Public API @@ -43,14 +45,6 @@ if !ok { } ``` -#### `ParseBool(m map[string]any, key string) bool` - -Extracts a boolean value from a `map[string]any` by key. Returns `false` if the map is `nil`, the key is absent, or the value is not a `bool`. - -```go -enabled := typeutil.ParseBool(config, "enabled") -``` - ### K/M Suffix Parsing #### `ParseInt64KMSuffix(raw string) (int64, bool)` @@ -112,6 +106,34 @@ Safely converts any value (`float64`, `int`, `int64`, `string`) to `float64`, re ratio := typeutil.ConvertToFloat(jsonData["ratio"]) ``` +### Map Extraction + +> Defined in `lookup.go`. + +#### `ParseBool(m map[string]any, key string) bool` + +Extracts a boolean value from a `map[string]any` by key. Returns `false` if the map is `nil`, the key is absent, or the value is not a `bool`. + +```go +enabled := typeutil.ParseBool(config, "enabled") +``` + +#### `LookupMap(m map[string]any, key string) (map[string]any, bool)` + +Extracts a `map[string]any` value from `m` by key. Returns `(nil, false)` if the map is `nil`, the key is absent, or the value is not a `map[string]any`. + +#### `LookupString(m map[string]any, key string) (string, bool)` + +Extracts a `string` value from `m` by key. Returns `("", false)` if the map is `nil`, the key is absent, or the value is not a `string`. + +#### `LookupStringPath(m map[string]any, path ...string) (string, bool)` + +Extracts a nested string value by following a sequence of keys through `map[string]any` layers. Returns `("", false)` if any step in the path is missing or has an invalid type, or if the path is empty. + +```go +cmd, ok := typeutil.LookupStringPath(event, "input", "command") +``` + ## Choosing the Right Function | Situation | Function to use | @@ -119,6 +141,9 @@ ratio := typeutil.ConvertToFloat(jsonData["ratio"]) | YAML/Go-typed numeric field; must detect missing vs zero | `ParseIntValue` | | JSON / log-parsed metric; zero default on failure is fine | `ConvertToInt` | | Boolean flag in a `map[string]any` | `ParseBool` | +| Nested `map[string]any` value by key | `LookupMap` | +| String value from `map[string]any` by key | `LookupString` | +| String value from nested `map[string]any` by key path | `LookupStringPath` | | Casting `uint64` counter to `int` | `SafeUint64ToInt` | | Numeric value from any source as float | `ConvertToFloat` | | Token/limit string with optional `K`/`M` suffix | `ParseInt64KMSuffix` | @@ -158,9 +183,10 @@ if !ok { ## Design Notes -- All debug output uses `logger.New("typeutil:convert")` and is only emitted when `DEBUG=typeutil:*`. +- Numeric conversion debug output uses `logger.New("typeutil:convert")` and is only emitted when `DEBUG=typeutil:*`. - `float64 → int` truncation is logged at debug level when the fractional part is lost. - `uint64 → int` overflow returns `0` rather than panicking, following the defensive convention used elsewhere in the codebase. +- Map-extraction helpers (`ParseBool`, `LookupMap`, `LookupString`, `LookupStringPath`) are defined in `lookup.go` to keep `convert.go` focused on single-value numeric conversion. --- diff --git a/pkg/typeutil/convert.go b/pkg/typeutil/convert.go index 522c60c525e..b86047e7749 100644 --- a/pkg/typeutil/convert.go +++ b/pkg/typeutil/convert.go @@ -1,8 +1,12 @@ -// Package typeutil provides general-purpose type conversion utilities. +// Package typeutil provides helpers for converting untyped values and extracting +// typed data from map[string]any structures. // -// This package contains safe conversion functions for working with heterogeneous -// any values, particularly those arising from JSON/YAML parsing where types may -// vary at runtime. +// The package includes numeric conversion utilities (in this file) and +// map-extraction helpers (in lookup.go), plus related parsing helpers used by +// the workflow engine. +// +// This file specifically focuses on safe conversion of heterogeneous any values +// that represent a single numeric quantity. // // # Key Functions // @@ -11,9 +15,6 @@ // the caller needs to distinguish "missing/invalid" from a zero value, or when string // inputs are not expected (e.g. YAML config field parsing). // -// Bool Extraction: -// - ParseBool() - Extract a bool from map[string]any by key; returns false on missing, nil map, or non-bool. -// // Safe Conversions (return 0 on overflow or invalid input): // - SafeUint64ToInt() - Convert uint64 to int, returning 0 on overflow // - SafeUintToInt() - Convert uint to int, returning 0 on overflow @@ -113,19 +114,6 @@ func ConvertToInt(val any) int { return 0 } -// ParseBool extracts a boolean value from a map[string]any by key. -// Returns false if the map is nil, the key is absent, or the value is not a bool. -func ParseBool(m map[string]any, key string) bool { - if m == nil { - return false - } - if v, ok := m[key]; ok { - b, _ := v.(bool) - return b - } - return false -} - // ConvertToFloat safely converts any value to float64, returning 0 on failure. // // Supported input types: float64, int, int64, and string (parsed via strconv.ParseFloat). @@ -145,73 +133,3 @@ func ConvertToFloat(val any) float64 { } return 0 } - -// LookupMap extracts a map[string]any value from m by key. -func LookupMap(m map[string]any, key string) (map[string]any, bool) { - if m == nil { - return nil, false - } - - value, ok := m[key] - if !ok { - return nil, false - } - - result, ok := value.(map[string]any) - if !ok { - return nil, false - } - - return result, true -} - -// LookupString extracts a string value from m by key. -func LookupString(m map[string]any, key string) (string, bool) { - if m == nil { - return "", false - } - - value, ok := m[key] - if !ok { - return "", false - } - - result, ok := value.(string) - if !ok { - return "", false - } - - return result, true -} - -// LookupStringPath extracts a nested string value from m by path. -// It returns ("", false) if any step in the path is missing or has an invalid type. -func LookupStringPath(m map[string]any, path ...string) (string, bool) { - if len(path) == 0 { - return "", false - } - - current := m - for i, key := range path { - value, ok := current[key] - if !ok { - return "", false - } - - if i == len(path)-1 { - result, ok := value.(string) - if !ok { - return "", false - } - return result, true - } - - next, ok := value.(map[string]any) - if !ok { - return "", false - } - current = next - } - - return "", false -} diff --git a/pkg/typeutil/convert_test.go b/pkg/typeutil/convert_test.go index 2ca6c67d237..ff8a8ff08fe 100644 --- a/pkg/typeutil/convert_test.go +++ b/pkg/typeutil/convert_test.go @@ -4,7 +4,6 @@ package typeutil import ( "math" - "reflect" "testing" ) @@ -133,89 +132,3 @@ func TestConvertToFloat(t *testing.T) { }) } } - -func TestLookupMap(t *testing.T) { - tests := []struct { - name string - input map[string]any - key string - expected map[string]any - ok bool - }{ - { - name: "existing map value", - input: map[string]any{ - "tool": map[string]any{"name": "Bash"}, - }, - key: "tool", - expected: map[string]any{"name": "Bash"}, - ok: true, - }, - { - name: "missing key", - input: map[string]any{}, - key: "tool", - expected: nil, - ok: false, - }, - { - name: "wrong type", - input: map[string]any{ - "tool": "not-a-map", - }, - key: "tool", - expected: nil, - ok: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result, ok := LookupMap(tt.input, tt.key) - if ok != tt.ok { - t.Errorf("LookupMap(%v, %q) ok = %v, want %v", tt.input, tt.key, ok, tt.ok) - } - if ok && !reflect.DeepEqual(result, tt.expected) { - t.Errorf("LookupMap(%v, %q) = %v, want %v", tt.input, tt.key, result, tt.expected) - } - }) - } -} - -func TestLookupString(t *testing.T) { - nested := map[string]any{ - "type": "tool_use", - "input": map[string]any{ - "command": "echo hello", - }, - } - - if value, ok := LookupString(nested, "type"); !ok || value != "tool_use" { - t.Errorf("LookupString(type) = (%q, %v), want (%q, true)", value, ok, "tool_use") - } - - if _, ok := LookupString(nested, "missing"); ok { - t.Error("LookupString should return ok=false for missing key") - } -} - -func TestLookupStringPath(t *testing.T) { - nested := map[string]any{ - "type": "tool_use", - "input": map[string]any{ - "command": "echo hello", - }, - } - - if value, ok := LookupStringPath(nested, "input", "command"); !ok || value != "echo hello" { - t.Errorf("LookupStringPath(input, command) = (%q, %v), want (%q, true)", value, ok, "echo hello") - } - - if _, ok := LookupStringPath(nested, "input", "missing"); ok { - t.Error("LookupStringPath should return ok=false for missing path segment") - } - - if _, ok := LookupStringPath(nested); ok { - t.Error("LookupStringPath should return ok=false for empty path") - } -} diff --git a/pkg/typeutil/lookup.go b/pkg/typeutil/lookup.go new file mode 100644 index 00000000000..bb32eea2a19 --- /dev/null +++ b/pkg/typeutil/lookup.go @@ -0,0 +1,84 @@ +package typeutil + +// ParseBool extracts a boolean value from a map[string]any by key. +// Returns false if the map is nil, the key is absent, or the value is not a bool. +func ParseBool(m map[string]any, key string) bool { + if m == nil { + return false + } + if v, ok := m[key]; ok { + b, _ := v.(bool) + return b + } + return false +} + +// LookupMap extracts a map[string]any value from m by key. +func LookupMap(m map[string]any, key string) (map[string]any, bool) { + if m == nil { + return nil, false + } + + value, ok := m[key] + if !ok { + return nil, false + } + + result, ok := value.(map[string]any) + if !ok { + return nil, false + } + + return result, true +} + +// LookupString extracts a string value from m by key. +func LookupString(m map[string]any, key string) (string, bool) { + if m == nil { + return "", false + } + + value, ok := m[key] + if !ok { + return "", false + } + + result, ok := value.(string) + if !ok { + return "", false + } + + return result, true +} + +// LookupStringPath extracts a nested string value from m by path. +// It returns ("", false) if any step in the path is missing or has an invalid type. +func LookupStringPath(m map[string]any, path ...string) (string, bool) { + if len(path) == 0 { + return "", false + } + + current := m + for i, key := range path { + value, ok := current[key] + if !ok { + return "", false + } + + if i == len(path)-1 { + result, ok := value.(string) + if !ok { + return "", false + } + return result, true + } + + next, ok := value.(map[string]any) + if !ok { + return "", false + } + current = next + } + + return "", false +} diff --git a/pkg/typeutil/lookup_test.go b/pkg/typeutil/lookup_test.go new file mode 100644 index 00000000000..41cfacf9723 --- /dev/null +++ b/pkg/typeutil/lookup_test.go @@ -0,0 +1,94 @@ +//go:build !integration + +package typeutil + +import ( + "reflect" + "testing" +) + +func TestLookupMap(t *testing.T) { + tests := []struct { + name string + input map[string]any + key string + expected map[string]any + ok bool + }{ + { + name: "existing map value", + input: map[string]any{ + "tool": map[string]any{"name": "Bash"}, + }, + key: "tool", + expected: map[string]any{"name": "Bash"}, + ok: true, + }, + { + name: "missing key", + input: map[string]any{}, + key: "tool", + expected: nil, + ok: false, + }, + { + name: "wrong type", + input: map[string]any{ + "tool": "not-a-map", + }, + key: "tool", + expected: nil, + ok: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, ok := LookupMap(tt.input, tt.key) + if ok != tt.ok { + t.Errorf("LookupMap(%v, %q) ok = %v, want %v", tt.input, tt.key, ok, tt.ok) + } + if ok && !reflect.DeepEqual(result, tt.expected) { + t.Errorf("LookupMap(%v, %q) = %v, want %v", tt.input, tt.key, result, tt.expected) + } + }) + } +} + +func TestLookupString(t *testing.T) { + nested := map[string]any{ + "type": "tool_use", + "input": map[string]any{ + "command": "echo hello", + }, + } + + if value, ok := LookupString(nested, "type"); !ok || value != "tool_use" { + t.Errorf("LookupString(type) = (%q, %v), want (%q, true)", value, ok, "tool_use") + } + + if _, ok := LookupString(nested, "missing"); ok { + t.Error("LookupString should return ok=false for missing key") + } +} + +func TestLookupStringPath(t *testing.T) { + nested := map[string]any{ + "type": "tool_use", + "input": map[string]any{ + "command": "echo hello", + }, + } + + if value, ok := LookupStringPath(nested, "input", "command"); !ok || value != "echo hello" { + t.Errorf("LookupStringPath(input, command) = (%q, %v), want (%q, true)", value, ok, "echo hello") + } + + if _, ok := LookupStringPath(nested, "input", "missing"); ok { + t.Error("LookupStringPath should return ok=false for missing path segment") + } + + if _, ok := LookupStringPath(nested); ok { + t.Error("LookupStringPath should return ok=false for empty path") + } +}