refactor: semantic function clustering — dedup, shared helpers, and generics consolidation#38776
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…nlineSections generic, GitHub URL extractor consolidation, perfsprint fix Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors several duplicate / near-duplicate helpers across pkg/ by consolidating shared logic (string truncation, token formatting, inline section extraction, capability validation) and tightening some workflow/runtime configuration paths to be $HOME-relative.
Changes:
- Replaces multiple ad-hoc truncation snippets with
stringutil.Truncate, and centralizes token-count formatting asconsole.FormatTokens. - Extracts shared helpers:
validateCapabilitySupport(...)for engine capability checks and genericextractInlineSections[T]for inline skill/sub-agent extraction loops. - Renames the Copilot events.jsonl metrics parser to
parseEventsJSONLMetricsand updates callers/tests; updates a compiled workflow to use$HOMEinstead of hardcoded/home/runner.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/cache_integrity.go | Switches manual dedup logic to sliceutil.Deduplicate for canonicalization. |
| pkg/workflow/agent_validation.go | Adds validateCapabilitySupport and refactors max-turn/max-continuations validators. |
| pkg/workflow/action_resolver.go | Removes a fmt.Sprintf in favor of string concatenation for the API path. |
| pkg/parser/sub_agent_extractor.go | Uses shared generic extractInlineSections for sub-agent extraction. |
| pkg/parser/inline_skill_extractor.go | Uses shared generic extractInlineSections for skill extraction. |
| pkg/parser/inline_section_helpers.go | Introduces extractInlineSections[T] helper. |
| pkg/console/render.go | Uses stringutil.Truncate for max-len rendering and adds console.FormatTokens. |
| pkg/cli/repo.go | Delegates git-remote slug parsing to parseGitHubRepoSlugFromURL. |
| pkg/cli/logs_metrics.go | Updates to renamed parseEventsJSONLMetrics. |
| pkg/cli/logs_format_compact.go | Uses stringutil.Truncate for compact log rendering. |
| pkg/cli/health_metrics.go | Uses console.FormatTokens instead of local formatting helper. |
| pkg/cli/health_metrics_test.go | Updates tests to call console.FormatTokens. |
| pkg/cli/copilot_events_jsonl.go | Renames metrics parser to parseEventsJSONLMetrics. |
| pkg/cli/copilot_events_jsonl_test.go | Updates tests to call parseEventsJSONLMetrics. |
| pkg/cli/completions.go | Reuses stringutil.Truncate for completion descriptions. |
| pkg/cli/audit_expanded.go | Updates to renamed parseEventsJSONLMetrics. |
| pkg/cli/audit_cross_run_render.go | Switches token formatting to console.FormatTokens. |
| .github/workflows/smoke-copilot-aoai-entra.lock.yml | Uses $HOME for Copilot config paths and adjusts env/export behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/cli/copilot_events_jsonl_test.go:86
- The test name and doc comment still say "TestParseEventsJSONLFile" even though the implementation under test was renamed to parseEventsJSONLMetrics. Renaming the test improves clarity and keeps grep/search results consistent with the new function name.
// TestParseEventsJSONLFile verifies that parseEventsJSONLMetrics correctly extracts
// turns, tool calls, tool sequences, and token counts from events.jsonl using
// the real Copilot CLI format (nested data object, tool.execution_start events).
func TestParseEventsJSONLFile(t *testing.T) {
tests := []struct {
- Files reviewed: 18/18 changed files
- Comments generated: 3
| hasMaxTurns := engineConfig != nil && engineConfig.MaxTurns != "" | ||
|
|
||
| if !hasMaxTurns { | ||
| // No max-turns specified, no validation needed | ||
| return nil | ||
| } | ||
|
|
||
| agentValidationLog.Printf("Validating max-turns support: engine=%s, maxTurns=%s", engine.GetID(), engineConfig.MaxTurns) | ||
|
|
||
| // max-turns is specified, check if the engine supports it | ||
| if !engine.GetCapabilities().MaxTurns { | ||
| agentValidationLog.Printf("Engine %s does not support max-turns feature", engine.GetID()) | ||
| return fmt.Errorf("max-turns not supported: engine '%s' does not support the max-turns feature", engine.GetID()) | ||
| } | ||
|
|
||
| // Engine supports max-turns - additional validation could be added here if needed | ||
| // For now, we rely on JSON schema validation for format checking | ||
|
|
||
| return nil | ||
| agentValidationLog.Printf("Validating max-turns support: engine=%s", engine.GetID()) | ||
| return validateCapabilitySupport("max-turns", hasMaxTurns, engine.GetCapabilities().MaxTurns, engine.GetID()) |
| hasMaxContinuations := engineConfig != nil && engineConfig.MaxContinuations != 0 | ||
|
|
||
| // max-continuations is specified, check if the engine supports it | ||
| if !engine.GetCapabilities().MaxContinuations { | ||
| agentValidationLog.Printf("Engine %s does not support max-continuations feature", engine.GetID()) | ||
| return fmt.Errorf("max-continuations not supported: engine '%s' does not support the max-continuations feature", engine.GetID()) | ||
| } | ||
|
|
||
| return nil | ||
| agentValidationLog.Printf("Validating max-continuations support: engine=%s", engine.GetID()) | ||
| return validateCapabilitySupport("max-continuations", hasMaxContinuations, engine.GetCapabilities().MaxContinuations, engine.GetID()) |
| // Delegate to the shared helper which supports both HTTPS and SSH formats, | ||
| // including GitHub Enterprise hosts configured via getGitHubHost(). | ||
| repoPath := parseGitHubRepoSlugFromURL(remoteURL) | ||
| if repoPath == "" { | ||
| return "", fmt.Errorf("remote URL does not appear to be a GitHub repository: %s", remoteURL) | ||
| } |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
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 (106 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 MatterEven consolidation/refactoring work encodes a decision: which abstraction becomes the single source of truth, and what coupling that introduces. ADRs create a searchable, permanent record of why the codebase looks the way it does, so future contributors understand the reasoning behind the shared helpers and generics introduced here. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot run pr-finisher skill |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 83/100 — Excellent
📊 Metrics & Test Classification (4 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Test Inflation Note
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §27420151282
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 83/100. Test quality is excellent — 0% of modified tests are implementation tests (threshold: 30%). All test changes correctly track the refactored function signatures (parseEventsJSONLFile → parseEventsJSONLMetrics, formatTokens → console.FormatTokens). No coding-guideline violations detected.
… parseGitHubRepoSlugFromURL Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Three review threads addressed in commit
|
Implements all P1 and P2 recommendations from the semantic function clustering analysis (#38748): eliminates duplicate logic, consolidates scattered helpers, and introduces generics where extraction loops are structurally identical.
P1 — inline dedup
stringutil.Truncatereuse (completions.go,logs_format_compact.go,render.go): replaced 4 inline truncation snippets (if len(s) > N { s[:N-3] + "..." }) with the shared utilityparseEventsJSONLFile→parseEventsJSONLMetrics(copilot_events_jsonl.go+ callers): clarifies intent; avoids collision with the existingparseEventsJSONL(returns raw entries, not metrics)FormatTokensinpkg/console(render.go,health_metrics.go,audit_cross_run_render.go): consolidated two near-identicalabbreviateNumberimplementations into one exported helper; removed privateformatTokenssliceutil.Deduplicate(cache_integrity.go): replaced manualseen-map + loop with the shared utilityP2 — shared helpers and generics
validateCapabilitySupport(agent_validation.go): extracted shared helper fromvalidateMaxTurnsSupport/validateMaxContinuationsSupport, which shared identical "is feature set → is capability supported → return error" logic:extractInlineSections[T](inline_section_helpers.go): generic helper eliminates the duplicate collection loop in bothExtractInlineSubAgentsandExtractInlineSkills:repo.go): fallback git-remote parsing now delegates toparseGitHubRepoSlugFromURL(GHE-aware viagetGitHubHost()) instead of duplicating inline SSH/HTTPS parsing with hardcodedgithub.comBonus
perfsprintlint warning inaction_resolver.go(fmt.Sprintf("/repos/github/gh-aw/commits/%s", ref)→ string concatenation)