Consolidate duplicate log parsers and replace local helper reinventions#42868
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors several duplicated helper implementations across the workflow engines, CLI utilities, console formatting, parser suggestions, and intent resolution to reuse shared utilities and consolidate identical logic paths (notably the Gemini/Antigravity stats-JSONL metrics parsing).
Changes:
- Consolidated Gemini/Antigravity stats-JSONL log metric parsing into a shared workflow helper and added direct Gemini coverage.
- Reused existing shared helpers in CLI flows (repo slug parsing, branch operations, sorted map keys) to remove local reinventions.
- Centralized byte-scaling logic and removed pass-through wrappers in parser/intent internals.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/stats_jsonl_logs.go | Adds shared stats-JSONL log parsing helper used by multiple engines. |
| pkg/workflow/gemini_logs.go | Switches Gemini metrics parsing to the shared helper and removes now-redundant overrides. |
| pkg/workflow/gemini_logs_test.go | Adds Gemini log-metrics parsing test coverage aligned with Antigravity. |
| pkg/workflow/antigravity_logs.go | Switches Antigravity metrics parsing to the shared helper and removes now-redundant overrides. |
| pkg/workflow/dispatch_workflow_validation.go | Replaces local slug parsing with repoutil.SplitRepoSlug for consistency. |
| pkg/parser/schema_suggestions.go | Uses stringutil directly for closest-match/distance in internal suggestion paths. |
| pkg/intent/resolver.go | Replaces manual slice cloning with slices.Clone. |
| pkg/console/progress_shared.go | Reuses shared binary byte scaling while preserving existing output formatting. |
| pkg/console/format.go | Introduces a shared scaleBinaryBytes helper used by multiple formatting call sites. |
| pkg/cli/update_version_labels.go | Replaces local repo slug splitting with repoutil.SplitRepoSlug. |
| pkg/cli/pr_command.go | Reuses shared git branch helpers for current-branch lookup and branch creation/switch. |
| pkg/cli/logs_report_firewall.go | Replaces local set-to-sorted-slice logic with sliceutil.SortedKeys. |
| pkg/cli/codemod_dependabot_permissions.go | Removes redundant sorted-keys wrapper in favor of sliceutil.SortedKeys directly. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 13/13 changed files
- Comments generated: 0
- Review effort level: Low
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Great work on this refactor! 🎉 Consolidating the Gemini/Antigravity stats-JSONL log parsing paths and replacing scattered local helpers with shared utilities ( The PR is well-structured — one clear goal, a thorough description with a before/after code example, and new test coverage for Gemini log parsing to complement the existing Antigravity tests. This looks ready for review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (2 tests)
| Test | File | Classification | Issues |
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (167 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 Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot please run the
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Non-blocking observations. The core deduplication — shared parseStatsJSONLMetrics helper, dropping redundant engine overrides, routing to repoutil/sliceutil/stringutil — is mechanically correct and the diff is net-negative at −108 lines.
Three items flagged:
- Dead
verboseparameter (stats_jsonl_logs.go:15): inherited from both original engines but not cleaned up — either use it to gate per-line log output or drop it. - Cross-file test coupling (
gemini_logs_test.go:26):toolCallCountsByNamelives inantigravity_logs_test.go; should move to a neutral shared helper file. - nil→empty-slice behavior change (
intent/resolver.go:175):slices.Clone([]string{})returns non-nil where old code returned nil;omitemptysaves JSON callers but any nil-equality check elsewhere would differ.
All three are non-blocking. No correctness bugs, security concerns, or race conditions found in the changed lines.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 200.6 AIC · ⌖ 9.53 AIC · ⊞ 1.6K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — commenting with improvement suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Test utility duplication:
toolCallCountsByNameis defined twice (inantigravity_logs_test.goand the newgemini_logs_test.go). The PR correctly deduplicates production code but misses this test helper. - Passthrough wrappers left behind:
cloneStringsinpkg/intent/resolver.gois now a single-line passthrough toslices.Clone; the PR's own spirit suggests inlining it. - Implicit sentinel coupling:
scaleBinaryBytesreturns"B"as a sentinel string — both callers must know to check for it to switch to integer formatting. A boolean return would make this contract explicit. verboseparameter is inert: Accepted inparseStatsJSONLMetricsbut only used in a log message prefix, not to gate any output. This is unclear intent.- Turns semantics undocumented:
metrics.Turns = 1is a hard cap (not a counter). No test pins this behaviour, so it could silently regress to a counter.
Positive Highlights
- ✅ Excellent DRY consolidation — 108 net lines removed with identical semantics preserved
- ✅ The delegate pattern for
ParseLogMetricsis clean and extensible to future engines - ✅ Replacing inline shell-out in
pr_command.gowithgetCurrentBranch()/createAndSwitchBranch()is a clear improvement - ✅ New Gemini test file provides direct engine-level coverage alongside Antigravity
- ✅ Consistent use of
repoutil.SplitRepoSlugacross bothupdate_version_labels.goanddispatch_workflow_validation.go
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 120.4 AIC · ⌖ 7.7 AIC · ⊞ 6.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/intent/resolver.go:174
[/codebase-design] cloneStrings is now a single-line passthrough wrapper — the three call sites could be inlined directly with slices.Clone(...) to remove the dead abstraction.
<details>
<summary>💡 Suggested change</summary>
Replace cloneStrings(labels) → slices.Clone(labels) at lines 111, 124, and 137, then delete cloneStrings.
The PR already converts the body to slices.Clone; finishing the job eliminates a trivial indirection that no longer carries meaning.
</details>
@c…
pkg/workflow/gemini_logs_test.go:26
[/tdd] toolCallCountsByName is duplicated verbatim from antigravity_logs_test.go — move it to a shared test helper file (e.g., workflow_test_helpers_test.go) to keep test utilities DRY.
<details>
<summary>💡 Suggested fix</summary>
Create pkg/workflow/helpers_test.go (or similar) containing the single definition of toolCallCountsByName, and remove the duplicate from antigravity_logs_test.go.
This is the same deduplication principle the PR applies to production code, applied t…
pkg/workflow/stats_jsonl_logs.go:15
[/codebase-design] The verbose parameter is accepted but only forwarded to the log message — it doesn't gate any additional output. If verbose behaviour is not yet implemented, consider removing the parameter or adding a _ bool comment so readers know the intent.
<details>
<summary>💡 Options</summary>
- Remove the parameter — both callers can be updated trivially; it can be re-added later when verbose paths exist.
- Document the intent — add a comment like `// verbose reser…
pkg/workflow/stats_jsonl_logs.go:55
[/tdd] The Turns counter is hard-coded to 1 whenever any line has a non-empty response field — a log with multiple response lines would still report 1 turn. A test covering two lines with non-empty responses would pin this behaviour (or surface it as a bug).
<details>
<summary>💡 Suggested test case to add to stats_jsonl_logs_test.go</summary>
t.Run("turns stays 1 with multiple response lines", func(t *testing.T) {
logContent := `{"response":"first"}\n{"response":"second…
</details>
<details><summary>pkg/cli/codemod_dependabot_permissions.go:234</summary>
**[/codebase-design]** `sortedMissingPermissionKeys` is a local wrapper that now only adds type-narrowing on top of `sliceutil.SortedKeys` — consider whether this is worth the indirection or if the three call sites can be converted directly.
<details>
<summary>💡 Context</summary>
The PR removed the analogous `sortedRemainingPermissionKeys` wrapper but left `sortedMissingPermissionKeys`. The remaining wrapper's only value is converting `map[workflow.PermissionScope]workflow.PermissionLevel` (…
</details>
<details><summary>pkg/workflow/gemini_logs_test.go:37</summary>
**[/tdd]** The new Gemini test covers the happy path and empty input, but the shared helper `parseStatsJSONLMetrics` has no direct tests. Adding a `stats_jsonl_logs_test.go` to test edge cases (malformed JSON mid-stream, stats with no models key, whitespace-only lines) would give regressions somewhere to land without depending on a specific engine.
<details>
<summary>💡 Suggested test cases for `stats_jsonl_logs_test.go`</summary>
```go
// malformed line followed by valid line
// stats with n…
</details>
<details><summary>pkg/console/format.go:26</summary>
**[/codebase-design]** `scaleBinaryBytes` returns a `(float64, string)` pair where the string `"B"` is the sentinel for the sub-unit path, then each caller re-checks for `"B"` to produce integer formatting. The sentinel coupling between callee and callers is implicit — a bool return (e.g., `isBytes bool`) or a small struct would make the contract explicit and prevent a future caller from silently missing the integer format.
<details>
<summary>💡 Alternative signature</summary>
```go
// scaleB…
</details>
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 135.5 AIC · ⌖ 6.31 AIC · ⊞ 4.9K
… guard 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>
🤖 PR Triage — Run §28572743533
|
|
@copilot please run the
|
|
🎉 This pull request is included in a new release. Release: |
This refactor removes a small set of confirmed duplicates across workflow, CLI, console, parser, and intent codepaths. The main changes consolidate the Gemini/Antigravity stats-JSONL log parsing path and replace several local helper implementations with existing shared utilities.
Shared workflow log parsing
GetLogFileForParsingandGetDefaultDetectionModelnow that they matchBaseEngine.CLI helper reuse
repoutil.SplitRepoSlug.sliceutil.SortedKeysdirectly where local sorted-key wrappers were redundant.Console formatting cleanup
"1.0 KB"vs"1.0KB") while removing duplicated scaling code.Parser and intent cleanup
stringutildirectly where the local wrappers were only pass-throughs.slices.Clone.Focused coverage
Example of the main consolidation: