Consolidate near-duplicate WorkflowListItem ⊂ WorkflowStatus structs in pkg/cli#39637
Conversation
…latten in console rendering Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🏗️ Design Decision Gate — ADR RequiredThis PR adds 222 new lines to core business logic (
📄 Draft ADR-39637 (copy into
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out, /improve-codebase-architecture, /tdd, and /grill-with-docs.
The DRY-up goal is well-executed — embedding eliminates the 5-field duplication cleanly and the JSON wire format is preserved. The main themes are architectural friction introduced by the renderer extension (three independent recursive walks, one duplicated function body) and test coverage gaps on the new recursive paths.
No blocking correctness bugs in changed code. All issues are maintainability/coverage concerns.
📋 Key Themes & Highlights
Key Themes
-
Renderer duplication —
renderInlineEmbeddedFieldsbody is nearly identical to the inner loop ofrenderStruct. MakingrenderStructdelegate to it would give a single source of truth (/improve-codebase-architecture, render.go:109). -
Three independent recursive walks —
computeMaxFieldLen,renderInlineEmbeddedFields, andcollectTableFieldsall traverse the same anonymous-embed tree with slightly different logic. A sharedwalkFieldsvisitor would collapse this and make future changes (e.g. pointer-embed support) a one-liner (/improve-codebase-architecture, render.go:140). -
Pointer-embedded struct not handled —
field.Kind() == reflect.Structsilently skips*EmbeddedTypeanonymous embeds in all three traversal sites. Fix or document the value-only restriction (/zoom-out, render.go:79, 116, 305). -
Test coverage gaps — New flattening paths miss:
omitemptyon embedded fields, 3-level nested embeds, and column ordering in slice output (/tdd, render_test.go).
Positive Highlights
- ✅ Clean separation between struct-rendering and table-rendering paths — both updated symmetrically
- ✅
FieldByIndexcorrectly used for multi-level path traversal inbuildTableRows - ✅ New renderer tests use isolated local types (no cli coupling) — clean unit test design
- ✅ All composite literals mechanically updated; no missed call sites
- ✅ PR description accurately documents the JSON promotion behaviour
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer
| // computeMaxFieldLen computes the longest visible field name for alignment. | ||
| // renderInlineEmbeddedFields renders the fields of an anonymous embedded struct | ||
| // directly into the parent struct output, flattening the hierarchy. | ||
| func renderInlineEmbeddedFields(val reflect.Value, maxFieldLen int, output *strings.Builder, depth int) { |
There was a problem hiding this comment.
[/improve-codebase-architecture] renderInlineEmbeddedFields body is nearly identical to the inner loop of renderStruct — the module is shallow and the duplication will drift.
💡 Suggestion: make `renderStruct` delegate its field loop to `renderInlineEmbeddedFields`
The only things renderStruct adds around the loop are: (a) the title block and (b) the trailing output.WriteString("\n"). If renderInlineEmbeddedFields accepted the top-level reflect.Value, it could replace the loop entirely:
func renderStruct(val reflect.Value, title string, output *strings.Builder, depth int) {
// ... title block ...
maxFieldLen := computeMaxFieldLen(val, val.Type())
renderInlineEmbeddedFields(val, maxFieldLen, output, depth) // replaces the loop
output.WriteString("\n")
}This makes renderInlineEmbeddedFields the single source of truth for field iteration and eliminates the copy-paste.
| fieldType := typ.Field(i) | ||
|
|
||
| // Flatten anonymous embedded structs by inlining their fields. | ||
| if fieldType.Anonymous && field.Kind() == reflect.Struct { |
There was a problem hiding this comment.
[/zoom-out] field.Kind() == reflect.Struct silently misses anonymous pointer embeds (type Foo struct { *Bar }), which Go allows. Those fields would fall through to normal rendering (producing a Bar: {...} row) rather than being flattened.
💡 Suggestion: handle the pointer case, or document the value-only assumption
Either dereference the pointer and recurse:
if fieldType.Anonymous {
f := field
if f.Kind() == reflect.Ptr {
if f.IsNil() { continue }
f = f.Elem()
}
if f.Kind() == reflect.Struct {
renderInlineEmbeddedFields(f, maxFieldLen, output, depth)
continue
}
}Or add a short comment: // Only value-type anonymous embeds are flattened; pointer embeds are treated as normal fields.
The same gap exists at line 116 (the recursive call in renderInlineEmbeddedFields) and line 305 in collectTableFields.
|
|
||
| // computeMaxFieldLen computes the longest visible field name for alignment, | ||
| // recursing into anonymous embedded structs to include their fields. | ||
| func computeMaxFieldLen(val reflect.Value, typ reflect.Type) int { |
There was a problem hiding this comment.
[/improve-codebase-architecture] Three separate recursive traversals of the same anonymous-embed field tree now exist: computeMaxFieldLen, renderStruct→renderInlineEmbeddedFields, and collectTableFields. Each is slightly different, so future changes (e.g. adding pointer-embed support) need to be made in three places.
💡 Suggestion: a shared field visitor
A single walkFields(v reflect.Value, fn func(val reflect.Value, sf reflect.StructField)) helper that recurses into anonymous value-type embeds would reduce this to one traversal. All three current functions could be expressed as walkFields callbacks. This would also make the pointer-embed fix a one-liner.
This is the deepening opportunity from /improve-codebase-architecture: replace three shallow helpers with one deep one.
|
|
||
| // collectTableFields recursively walks a struct type, inlining the fields of any | ||
| // anonymous embedded structs so they appear as top-level table columns. | ||
| func collectTableFields(t reflect.Type, prefix []int, headers *[]string, fieldPaths *[][]int, fieldTags *[]consoleTag) { |
There was a problem hiding this comment.
[/improve-codebase-architecture] Triple pointer-to-slice parameters (headers *[]string, fieldPaths *[][]int, fieldTags *[]consoleTag) make the signature harder to read than necessary, given the only call site is buildTableHeaders.
💡 Suggestion: a collector struct
type tableFieldCollector struct {
headers []string
fieldPaths [][]int
fieldTags []consoleTag
}
func (c *tableFieldCollector) collect(t reflect.Type, prefix []int) { ... }Or simply return a []tableFieldEntry slice from collectTableFields and have buildTableHeaders unzip it. Either approach reduces pointer indirection and makes recursive calls self-documenting.
|
|
||
| // TestRenderStruct_EmbeddedStruct tests that anonymous embedded struct fields are | ||
| // inlined into the parent struct output rather than rendered as a nested section. | ||
| func TestRenderStruct_EmbeddedStruct(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] The test uses plain fields with no omitempty tag, leaving the omitempty branch of renderInlineEmbeddedFields (line 125–127 in render.go) untested. A zero-valued embedded field with omitempty would silently fall through to output — there is no regression guard.
💡 Suggested addition to this test
type BaseWithOmit struct {
Name string `console:"header:Name"`
Engine string `console:"header:Engine,omitempty"`
}
type ExtendedWithOmit struct {
BaseWithOmit
Status string `console:"header:Status"`
}
data := ExtendedWithOmit{BaseWithOmit: BaseWithOmit{Name: "wf"}, Status: "active"}
out := RenderStruct(data)
assert.NotContains(t, out, "Engine", "zero omitempty field in embed should be suppressed")|
|
||
| // TestRenderSlice_EmbeddedStruct tests that a slice of structs with anonymous embedded | ||
| // fields is rendered as a flat table with all promoted fields as columns. | ||
| func TestRenderSlice_EmbeddedStruct(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] renderInlineEmbeddedFields recurses for nested anonymous embedded structs (render.go line 116–118), but no test exercises that path. A 3-level embed (type C struct { Base }; type D struct { C }) would hit the recursive case.
💡 Suggested test skeleton
func TestRenderStruct_NestedEmbeddedStruct(t *testing.T) {
type Inner struct {
Name string `console:"header:Name"`
}
type Middle struct {
Inner
Engine string `console:"header:Engine"`
}
type Outer struct {
Middle
Status string `console:"header:Status"`
}
out := RenderStruct(Outer{Middle: Middle{Inner: Inner{Name: "wf"}, Engine: "copilot"}, Status: "ok"})
assert.Contains(t, out, "wf")
assert.Contains(t, out, "copilot")
assert.Contains(t, out, "ok")
assert.NotContains(t, out, "Middle")
assert.NotContains(t, out, "Inner")
}| } | ||
|
|
||
| output := RenderStruct(items) | ||
|
|
There was a problem hiding this comment.
[/tdd] assert.Contains checks that column values exist anywhere in the output, but not their order. Since embedded fields are promoted to the left of outer fields, a column reorder would pass this test silently. Pinning the expected header line would make this test a true specification.
💡 Suggested assertion
// Column order: embedded fields first, then outer fields.
assert.Contains(t, output, "Name"+" "+"Engine")
// or use a regex / index comparison:
nameIdx := strings.Index(output, "Name")
statusIdx := strings.Index(output, "Status")
assert.Less(t, nameIdx, statusIdx, "embedded Name column must appear before outer Status column")|
|
||
| statuses = append(statuses, WorkflowStatus{ | ||
| Workflow: name, | ||
| WorkflowListItem: WorkflowListItem{ |
There was a problem hiding this comment.
[/zoom-out] The WorkflowListItem inside buildRemoteWorkflowStatuses is partially initialised — only Workflow is set; EngineID, Compiled, Labels, and On stay at zero values. This was the same behaviour before the refactor, but the embedding makes the partial init more visible now.
💡 Suggestion: add a comment clarifying intent
A short comment prevents a future contributor from treating the missing fields as an oversight:
statuses = append(statuses, WorkflowStatus{
// Remote-status entries only carry the workflow name; engine/compiled
// metadata is not available from the GitHub Actions API at this point.
WorkflowListItem: WorkflowListItem{Workflow: name},
...
})| | `WorkflowFileStatus` | struct | Status of a workflow file (exists, outdated, etc.) | | ||
| | `WorkflowJob` | struct | A GitHub Actions job within a workflow run | | ||
| | `WorkflowListItem` | struct | A single item in the `gh aw list` output | | ||
| | `WorkflowListItem` | struct | Shared workflow metadata (name, engine, compiled status, labels, triggers); embedded in `WorkflowStatus` | |
There was a problem hiding this comment.
[/grill-with-docs] The new description focuses on the embedding relationship and de-emphasises that WorkflowListItem is the primary output type for gh aw list. A reader scanning the table might assume it exists only for WorkflowStatus.
💡 Suggested wording
| `WorkflowListItem` | struct | A single item in the `gh aw list` output; shared workflow metadata fields (name, engine, compiled status, labels, triggers) also embedded in `WorkflowStatus` |This preserves the original intent while documenting the new relationship.
| field := t.Field(i) | ||
|
|
||
| // Flatten anonymous embedded structs. | ||
| if field.Anonymous && field.Type.Kind() == reflect.Struct { |
There was a problem hiding this comment.
[/zoom-out] Same anonymous pointer-embed gap as in renderStruct/renderInlineEmbeddedFields: field.Type.Kind() == reflect.Struct will not match *EmbeddedType, so pointer-embedded struct fields would not be promoted to table columns. Consistent with the other two call sites but worth fixing or documenting in one place (ideally a shared walkFields helper) so future readers know this is a deliberate scope restriction.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics & Test Classification (12 tests analyzed)
Go: 12 ( Verdict
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 80/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). All 12 tests classified as design/behavioral contracts, with 2 new tests directly verifying the embedded-struct flattening contract introduced by this PR.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed in 8b8fb77. Local CI on the latest agent-pushed HEAD is still unverified until a maintainer re-triggers it. |
WorkflowListItemandWorkflowStatusinpkg/cliduplicated 5 fields with identical tags (Workflow,EngineID,Compiled,Labels,On).WorkflowStatusnow embedsWorkflowListItemas the single source of truth for shared workflow metadata.Struct change
JSON serialisation is unchanged — Go promotes embedded fields in
encoding/jsonby default.Console rendering (
pkg/console/render.go)The existing table renderer used flat
[]intfield indices and had no concept of embedded structs, so a naïve embed would have produced a broken table with a rawWorkflowListItemcolumn. Two rendering paths updated:buildTableHeaders/buildTableRows): switched from[]intto[][]intfield index paths;collectTableFieldsnow flattens anonymous embedded structs, including pointer embeds, so their fields surface as top-level columns.renderStruct/computeMaxFieldLen): shared inline field traversal now flattens anonymous embedded structs at the parent level instead of opening a nested section, including nested and pointer embeds.Tests
pkg/console/render_test.go: added coverage for embedded-struct flattening in struct and slice rendering, includingomitempty, nested embeds, pointer embeds, header ordering, and nil embedded-pointer rows.WorkflowStatus{...}composite literals in production code and tests updated to the embedded-struct initialisation form (WorkflowStatus{WorkflowListItem: WorkflowListItem{...}, ...}).Documentation
pkg/cli/README.md: clarified thatWorkflowListItemis thegh aw listitem type and that its shared workflow metadata is also embedded inWorkflowStatus.pkg/cli/status_command.go: added a clarifying comment that remote workflow status entries only populate the workflow name because the GitHub Actions API does not provide the list metadata fields at that point.