Refactor duplicated workflow helpers and remove dead audit render stubs#36010
Conversation
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot merge main and recompile |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors duplicated workflow helpers into shared implementations and removes empty audit-report stub files. Adds focused unit tests for the extracted helpers.
Changes:
- Adds shared
workflow.ResolveEngineID,escapeYAMLSingleQuoted, andbuildNormalizedSortedJSONhelpers; renames secret-redaction escaper toescapeSingleQuoteBackslashfor clarity. - Updates
pkg/cli/mcp_inspect.go, OTLP injection, central slash-command generation, and safe-output JSON builders to use the shared helpers. - Removes four empty
pkg/cli/audit_report_render_*.goplaceholder files and adds regression tests.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/engine_helpers.go | Adds exported ResolveEngineID helper. |
| pkg/workflow/engine_helpers_test.go | Adds tests for ResolveEngineID. |
| pkg/workflow/strings.go | Adds shared escapeYAMLSingleQuoted helper. |
| pkg/workflow/strings_test.go | Tests for the new YAML quote escaper. |
| pkg/workflow/observability_otlp.go | Switches to shared helpers; drops local duplicates. |
| pkg/workflow/central_slash_command_workflow.go | Routes single-quote escape through shared helper. |
| pkg/workflow/redact_secrets.go | Renames escaper to escapeSingleQuoteBackslash. |
| pkg/workflow/redact_secrets_test.go | Test for renamed escaper. |
| pkg/workflow/safe_outputs_config_helpers.go | Extracts buildNormalizedSortedJSON; reuses in jobs JSON. |
| pkg/workflow/safe_outputs_config_helpers_test.go | Tests for custom jobs JSON. |
| pkg/workflow/safe_outputs_actions.go | Reuses buildNormalizedSortedJSON for actions JSON. |
| pkg/cli/mcp_inspect.go | Uses workflow.ResolveEngineID with CLI-side "unknown" fallback. |
| pkg/cli/mcp_inspect_tree_test.go | Replaces helper test with rendered-tree fallback test. |
| pkg/cli/audit_report_render_{guard,jobs,overview,tools}.go | Removes empty placeholder files. |
| .github/workflows/daily-news.lock.yml | Regenerated lock file (hash/delimiter churn only). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 18/18 changed files
- Comments generated: 0
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (154 new lines across 📄 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 deduplication refactor, recording which implementation became canonical and why (and which near-identical helper was deliberately kept separate — 📋 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.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.7M
| @@ -498,5 +498,5 @@ func uniqueSorted(values []string) []string { | |||
| } | |||
|
|
|||
| func escapeSingleQuotedYAMLString(input string) string { | |||
There was a problem hiding this comment.
Unnecessary wrapper: escapeSingleQuotedYAMLString is now a trivial one-line delegate to escapeYAMLSingleQuoted and adds indirection without value.
💡 Suggested fix
Since both call sites in this file are in the same package, replace them directly with escapeYAMLSingleQuoted and delete the wrapper:
// Before (two calls in the same file):
escapeSingleQuotedYAMLString(string(slashRoutesJSON))
escapeSingleQuotedYAMLString(string(labelRoutesJSON))
// After:
escapeYAMLSingleQuoted(string(slashRoutesJSON))
escapeYAMLSingleQuoted(string(labelRoutesJSON))The wrapper function serves no purpose now that the canonical name is escapeYAMLSingleQuoted in strings.go.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /zoom-out, and /improve-codebase-architecture — approving with minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap at the nil boundary:
ResolveEngineIDnow contracts to return""for nil (old local helper returned"unknown"), but the new integration test inmcp_inspect_tree_test.goonly covers the empty-struct path, not nil. The call-site fallback is correct, but worth a targeted test. buildNormalizedSortedJSONidentity-valueFn path is untested: Only the""value case is covered; thebuildCustomSafeOutputActionsJSONpath (returns normalised name as value) has no test.escapeSingleQuotedYAMLStringwrapper left over: A one-line forwarding stub incentral_slash_command_workflow.gothat could be inlined.t.Fatalfstyle in one new test:TestEscapeSingleQuoteBackslashdiverges from the testify pattern used everywhere else.
Positive Highlights
- ✅ Excellent consolidation — four distinct deduplication problems all resolved cleanly
- ✅ Regression tests added for every new shared helper
- ✅
escapeSingleQuoteBackslashrename correctly surfaces the semantic distinction between the two escapers - ✅ Dead stub files removed without ceremony
- ✅
ResolveEngineIDdocstring accurately documents the new empty-string contract
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.7M
| @@ -63,42 +63,10 @@ func TestRenderMCPInspectionTree_SortsServersDeterministically(t *testing.T) { | |||
| assert.Less(t, githubStdioIdx, playwrightIdx) | |||
There was a problem hiding this comment.
[/tdd] The replacement test only exercises &workflow.WorkflowData{} (empty struct). The previous table-driven test also covered the nil input — which now follows a different path: ResolveEngineID(nil) returns "", the call-site guard in renderMCPInspectionTree sets it to "unknown", and the tree label becomes "Engine: unknown". That nil → call-site-fallback path is not explicitly exercised here.
💡 Suggested additional case
func TestRenderMCPInspectionTree_NilWorkflowDataFallback(t *testing.T) {
result := renderMCPInspectionTree("/tmp/audit.md", nil, nil)
assert.Contains(t, result, "Engine: unknown")
}Since the nil guard now lives in the caller rather than inside ResolveEngineID, a direct integration test of the nil path through renderMCPInspectionTree gives confidence that the call-site fallback is wired correctly.
| @@ -17,6 +17,31 @@ import ( | |||
|
|
|||
| var safeOutputsConfigGenLog = logger.New("workflow:safe_outputs_config_generation_helpers") | |||
There was a problem hiding this comment.
[/tdd] buildNormalizedSortedJSON is only tested transitively through buildCustomSafeOutputJobsJSON (where valueFn always returns ""). The non-trivial path — valueFn returning the normalised name itself, as used by buildCustomSafeOutputActionsJSON — has no test. A divergent normalization or sort bug there would be invisible.
💡 Suggested direct test
func TestBuildNormalizedSortedJSON(t *testing.T) {
// identity valueFn (mirrors buildCustomSafeOutputActionsJSON usage)
got, err := buildNormalizedSortedJSON(
[]string{"z-action", "a-action"},
func(n string) string { return n },
)
require.NoError(t, err)
assert.JSONEq(t, `{"a_action":"a_action","z_action":"z_action"}`, got)
}Also consider a direct test of buildCustomSafeOutputActionsJSON for parity with the buildCustomSafeOutputJobsJSON tests that were added.
| @@ -83,6 +83,13 @@ func TestCollectSecretReferences(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
[/improve-codebase-architecture] TestEscapeSingleQuoteBackslash uses a bare t.Fatalf style that diverges from the testify pattern used everywhere else in this PR and the wider codebase. Inconsistency here makes the test subtly harder to scan.
💡 Suggested fix
func TestEscapeSingleQuoteBackslash(t *testing.T) {
assert.Equal(t, `owner\'s\\workflow`, escapeSingleQuoteBackslash(`owner's\workflow`),
"should escape single quotes with backslash and double backslashes")
}|
|
||
| func escapeSingleQuotedYAMLString(input string) string { | ||
| return strings.ReplaceAll(input, "'", "''") | ||
| return escapeYAMLSingleQuoted(input) |
There was a problem hiding this comment.
[/improve-codebase-architecture] escapeSingleQuotedYAMLString is now a one-line wrapper that simply delegates to escapeYAMLSingleQuoted — it adds a layer of indirection without adding documentation or behavior. If this name is exported or needed for backwards compat, keep it; otherwise consider inlining the calls to the canonical helper directly and removing this stub.
💡 Context
If there are other callers in the package that use escapeSingleQuotedYAMLString, an alias is fine for staged migration. If this is the only call site after the refactor, the wrapper is orphaned indirection that obscures where the real implementation lives.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 87/100 — Excellent
📊 Metrics & Test Classification (~12 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 87/100 — Excellent. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). All ~12 analyzed tests enforce behavioral contracts with good edge-case coverage. See the detailed comment for two minor advisory items (missing assertion messages, inflation ratio note).
|
@copilot review all comments and reviews from copilot and github-actions. Resolve. |
|
|
1 similar comment
|
|
Already addressed in 34e0bcb — merged
Reviewed all automated feedback:
The smoke-trigger/smoke-water/smoke-multi-caller failures on older commits are pre-existing and unrelated to these changes. The |
pelikhan
deleted the
copilot/refactor-semantic-function-clustering-please-work
branch
The semantic clustering audit found a small set of verified duplicates and dead placeholders in
pkg/workflowandpkg/cli: engine ID resolution was duplicated across packages, two YAML single-quote escapers had identical behavior, safe-output JSON builders repeated the same normalization/sorting logic, and fouraudit_report_render_*files were empty package stubs.Engine ID resolution
workflow.ResolveEngineIDas the canonical helper forEngineConfig.ID -> AIresolution.pkg/cli/mcp_inspect.goto use the shared helper and keep the CLI-specific"unknown"fallback at the call site.YAML single-quote escaping
escapeYAMLSingleQuotedhelper inpkg/workflow/strings.go.escapeSingleQuoteBackslashto make the semantics explicit.Safe-output JSON normalization
buildNormalizedSortedJSON.Dead files
pkg/cli/audit_report_render_{guard,jobs,overview,tools}.goplaceholder files.Targeted regression coverage
Example of the shared engine-ID path after this refactor: