refactor: split oversized files for IDE/LLM ergonomics#332
Merged
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move Option type, parseConfig struct, ParseWithOptions, applyOptions, and all 16 With* option constructors into parser_options.go to improve file ergonomics. Pure code move with no logic changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move format detection and URL fetching functions out of parser.go into a dedicated parser_format.go file. This is a pure code move with no logic changes. Extracted functions: - FormatBytes - detectFormatFromPath - detectFormatFromContent - isURL - fetchURL - detectFormatFromURL Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move functional options infrastructure (~560 lines) from joiner.go into a dedicated joiner_options.go file. This brings joiner.go from 1,236 lines down to 669 lines. Pure file reorganization with no logic changes. Moved code: - Option type alias and joinConfig struct - JoinWithOptions, joinWithoutOverlays, joinWithOverlays functions - parseOverlayList, mergeSpecOverlays, applyOptions helpers - Value helper functions (valueOrDefault, boolValueOrDefault, etc.) - All With* option constructors Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ains Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughReorganizes and deduplicates core packages: introduces planning docs; splits parser, joiner, generator, and integration harness into smaller files; adds parser format utilities and options APIs; moves/centralizes server/client generation into a shared generator; and implements new integration pipeline helpers for parse/validate/transform/generate steps. Changes
Sequence DiagramsequenceDiagram
participant Scenario
participant Pipeline
participant ParseValidate as Parse/Validate
participant Transform
participant Generate
participant Parser
participant Validator
participant Fixer
participant Joiner
participant Converter
participant Differ
participant Generator
participant Builder
Scenario->>Pipeline: run scenario
Pipeline->>ParseValidate: executeParse(step)
ParseValidate->>Parser: Parse spec
Parser-->>ParseValidate: ParseResult
ParseValidate->>Pipeline: store ParseResult
Pipeline->>ParseValidate: executeValidate(step)
ParseValidate->>Validator: Validate ParseResult
Validator-->>ParseValidate: ValidationResult
ParseValidate->>Pipeline: store ValidationResult
Pipeline->>Transform: executeFix(step)
Transform->>Fixer: Apply fixes
Fixer-->>Transform: Updated ParseResult
Transform->>Pipeline: store ParseResult
Pipeline->>Transform: executeJoin/convert/diff/overlay
Transform->>Joiner: Join specs
Joiner-->>Transform: JoinResult
Transform->>Converter: Convert versions
Converter-->>Transform: ConvertResult
Transform->>Differ: Compute diff
Differ-->>Transform: DiffResult
Transform->>Pipeline: store results
Pipeline->>Generate: executeGenerate(step)
Generate->>Generator: Generate code
Generator-->>Generate: GenerateResult + files
Generate->>Pipeline: record output dir
Pipeline->>Generate: executeBuild(step)
Generate->>Builder: go build output
Builder-->>Generate: build status
Generate->>Pipeline: log result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #332 +/- ##
==========================================
- Coverage 84.73% 84.72% -0.01%
==========================================
Files 186 190 +4
Lines 27067 27070 +3
==========================================
Hits 22936 22936
- Misses 2813 2815 +2
- Partials 1318 1319 +1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integration/harness/pipeline_generate.go`:
- Around line 141-155: The executeBuild function is swallowing build failures
when step.Expect == "error" which conflicts with ExecuteStep's uniform
expectation handling; remove the internal expected-error logic from executeBuild
(both the early-return nil when step.Expect == "error" after a build error and
the later check that returns an error when a build succeeds but step.Expect ==
"error"), and instead always return the real error (or nil on success); you may
keep debug logging (e.g., t.Logf) but ensure executeBuild returns the actual err
so ExecuteStep can perform the expected-error validation.
In `@joiner/joiner_options.go`:
- Line 235: In joinWithOverlays update the inline step comments so numbering is
sequential—change the comment currently labeled "Step 5: Perform the join" to
"Step 4: Perform the join" and then renumber the following "Step 6" comment to
"Step 5" (or renumber all subsequent step comments to maintain a continuous
sequence) so the step labels in the function (joinWithOverlays) read 1, 2, 3, 4,
5 consistently.
In `@parser/parser_format.go`:
- Line 18: The parameter name in FormatBytes shadows the imported bytes package;
rename the parameter (e.g., from bytes to size or n) in the FormatBytes function
signature and update all usages inside the FormatBytes function body (any
arithmetic/returns referencing the parameter) to the new name so the function no
longer conflicts with the bytes package import.
- Around line 87-90: The TLS config in the http.Transport construction (the
transport variable/TLSClientConfig) leaves MinVersion unset, allowing legacy TLS
1.0/1.1 negotiation; update the TLS config for transport to set MinVersion to a
safe minimum (e.g., tls.VersionTLS12) alongside the existing InsecureSkipVerify
comment so static analyzers stop flagging CWE-327 and the code explicitly
prevents downgrades.
- Remove executeBuild internal expected-error handling (conflicts with ExecuteStep's uniform expectation logic) - Fix step numbering in joinWithOverlays (5→4, 6→5) - Rename FormatBytes parameter from bytes to size (avoids shadowing bytes package) - Add MinVersion TLS 1.2 to insecure transport config Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integration/harness/pipeline_generate.go`:
- Around line 113-134: The os.Stat check for goModPath currently only handles
the "not exist" case and ignores other errors; change the logic in the block
around goModPath and os.Stat so that you: call os.Stat(goModPath), if err == nil
skip module init, if os.IsNotExist(err) perform the existing module init/tidy
(using modCmd and tidyCmd), and for any other non-nil err return it (wrap with
context like "stat go.mod" or similar) instead of proceeding; ensure you update
the handling around goModPath, os.Stat, modCmd and tidyCmd accordingly.
- Around line 108-111: The os.Stat call checking outputDir currently only
handles os.IsNotExist(err) and ignores other errors; update the check around
os.Stat(outputDir) to return a descriptive error when err != nil (e.g.,
fmt.Errorf("build: checking output directory %s failed: %w", outputDir, err))
and preserve the existing not-exist message for os.IsNotExist; reference
os.Stat, os.IsNotExist, outputDir and fmt.Errorf when making the change so
permission or IO errors are surfaced instead of being silently ignored.
In `@joiner/joiner_options.go`:
- Around line 472-477: The WithEquivalenceMode option should validate the
provided mode string immediately: in function WithEquivalenceMode check that
mode is one of the allowed values "none", "shallow", or "deep" before assigning
cfg.equivalenceMode; if it is invalid return a non-nil error (e.g.
fmt.Errorf("invalid equivalence mode %q: valid values are none, shallow, deep"))
and do not mutate cfg. Update the closure in WithEquivalenceMode to perform this
check and return the error to fail fast.
In `@parser/parser_format.go`:
- Around line 103-107: Replace http.NewRequest with http.NewRequestWithContext
to allow cancellation: either add a context.Context parameter to the surrounding
function and pass that context into http.NewRequestWithContext(ctx,
http.MethodGet, urlStr, nil), or if you cannot change the signature yet use
context.Background() as the context. Update the req creation (the req, err :=
... line) accordingly and import the context package so the noctx linter is
satisfied.
- Around line 130-134: Replace the unbounded io.ReadAll(resp.Body) with a
bounded read using io.LimitReader and a defined size constant (e.g.,
MaxResponseBodySize) to avoid memory exhaustion; create a package-level constant
MaxResponseBodySize (for example 10<<20 for 10MB), read via
io.ReadAll(io.LimitReader(resp.Body, MaxResponseBodySize+1)), and if the
returned byte length exceeds MaxResponseBodySize return an error (e.g.,
"response body too large") so callers (in the parser code that uses resp.Body
and the data variable) get a clear failure instead of OOM.
---
Duplicate comments:
In `@integration/harness/pipeline_generate.go`:
- Around line 136-150: executeBuild currently returns raw build errors (previous
internal handling for step.Expect == "error" was removed); restore uniform
expectation handling by ensuring executeBuild returns the build error unchanged
and do not perform expectation checks inside executeBuild itself—leave
expectation matching to ExecuteStep (refer to function executeBuild and caller
ExecuteStep and any Step.Expect usage) so that ExecuteStep can uniformly compare
the returned error against step.Expect.
In `@parser/parser_format.go`:
- Around line 86-101: The current change only sets TLS minimum version in the
InsecureSkipVerify branch; ensure both branches enforce TLS 1.2+ by adding a
tls.Config with MinVersion: tls.VersionTLS12 to the secure client path as well
(i.e., when constructing http.Client in the else branch), and keep the explicit
InsecureSkipVerify setting only in the p.InsecureSkipVerify branch; update the
http.Transport/tls.Config used by http.Client so both branches reference an
explicit MinVersion via tls.VersionTLS12.
- Handle all os.Stat errors in executeBuild, not just IsNotExist - Validate equivalence mode in WithEquivalenceMode using existing IsValidEquivalenceMode() for fail-fast error reporting Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Feb 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
parser/parser.go(1,622 → 3 files): core parsing, options, format detectionjoiner/joiner.go(1,236 → 2 files): core join logic, optionsgenerator/security_gen_shared.go(1,194 → 2 files): security helpers, server/client helpersintegration/harness/pipeline.go(1,294 → 4 files): orchestrator+assertions, parse/validate, transform, generate/buildcontainsSubstring/findSubstringwithstrings.ContainsDetails
Pure file moves within existing packages. No logic changes, no API changes.
All functions remain in the same package — the Go compiler resolves symbols across files at the package level, so this requires zero code changes beyond cutting/pasting and adjusting imports.
The one behavioral cleanup: replaced a hand-rolled substring search (
containsSubstring/findSubstring) withstrings.Containsin the integration harness.Design: docs/plans/2026-02-16-code-simplification-design.md
File size results
parser/parser.gojoiner/joiner.gosecurity_gen_shared.gopipeline.goTest plan
make checkpasses (8,236 tests, lint, formatting)parser.goat 1,107 — cohesive core logic)🤖 Generated with Claude Code