Repository navigation
refactor(cli): embed DomainBuckets in WorkflowDomainsDetail - #67642
Conversation
Preserve workflow domain JSON keys for populated, empty, and nil lists and cover command output and JSON round trips. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR #67642 lacks the implementation label and has 98 new lines in business logic directories (<=100 threshold, default config).
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
There was a problem hiding this comment.
🟢 Approval recommended
The focused refactor preserves existing behavior and has appropriate regression coverage.
0 open findings
What changed in this PR
Refactors workflow-domain reporting to reuse DomainBuckets while preserving its established flat JSON contract.
Changes:
- Embeds
DomainBucketsinWorkflowDomainsDetail. - Adds detail-specific JSON marshaling for empty and nil lists.
- Expands round-trip and command-output regression tests.
| File | Description |
|---|---|
pkg/cli/domains_command.go |
Embeds shared buckets and preserves JSON output. |
pkg/cli/domains_command_test.go |
Tests command output and JSON edge cases. |
pkg/cli/domain_buckets.go |
Generalizes the embedding documentation. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
There are no blocking issues in the changed lines. The embedding refactor preserves the flat JSON output contract and adds regression coverage for nil, empty, and populated domain lists.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 38.9 AIC · ⌖ 5.41 AIC · ⊞ 21.2K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — approving with one minor maintainability note.
📋 Key Themes & Highlights
Key Themes
- Embedding pattern is consistent:
WorkflowDomainsDetailnow embedsDomainBucketsthe same wayAnalysisBasealready does indomain_buckets.go, reducing duplication without introducing a new abstraction. - Schema preservation via custom
MarshalJSON: a small shadow struct is used to keep the flat, non-omitemptyJSON shape forallowed_domains/blocked_domainswhile the sharedDomainBucketskeepsomitemptyfor other embedders (AnalysisBase). This duplicates field declarations, so a future field added toWorkflowDomainsDetailcould silently be dropped from JSON output — flagged inline with a low-cost suggestion (test or comment guardrail). - Test coverage is thorough: the rewritten
TestWorkflowDomainsDetail_JSONMarshalingnow table-tests populated/empty/nil/partial domain lists plus marshal+unmarshal round-trips, andTestRunWorkflowDomains_JSONOutputasserts actual command output end-to-end via a stdout pipe (consistent with existing patterns elsewhere in the package).
Positive Highlights
- ✅ Comment update in
domain_buckets.gocorrectly generalizes "embedded byDomainAnalysis/FirewallAnalysis" to "CLI reporting types embed it", sinceWorkflowDomainsDetailis now a third embedder. - ✅ No behavioral change to the public JSON schema — verified build and targeted tests pass locally (
TestWorkflowDomainsDetail_JSONMarshaling,TestRunWorkflowDomains_JSONOutput,TestRunWorkflowDomains_TextOutput). - ✅ Good choice to keep
RunWorkflowDomains's JSON-output construction site minimal by using the embedded struct literal.
Verification: ran go build ./pkg/cli/... and go test ./pkg/cli/ -run 'TestWorkflowDomainsDetail_JSONMarshaling|TestRunWorkflowDomains' — all pass.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 44.3 AIC · ⌖ 14.6 AIC · ⊞ 10.3K
Comment /matt to run again
Address review feedback by explicitly noting that the JSON shadow struct and assignments must stay synchronized with WorkflowDomainsDetail. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Summary
DomainBucketsinWorkflowDomainsDetailand update construction sites.nulland empty-array domain values, with a detail-specific marshaler; other reporting types retain their existingomitemptybehavior.Fixes #67201
Validation
make buildpassed; Go formatting completed.golangci-lint run --allow-parallel-runners ./pkg/clipassed with zero issues.make agent-report-progresspassed schema freshness and impacted Go tests, but the overall gate is blocked by two existing custom-linter diagnostics on untoucheddomains_command.gostatements (args[0]and a discarded type-assertionokvalue). The initial regular lint job also collided with sibling sessions; the standalone parallel-safe rerun passed.make fmtcompleted Go formatting but its unrelated JavaScript formatting stage lacked locally installed Prettier. Formatter-only changes outside this issue were reverted.Publication used local Git because this runtime does not expose
report_progress.