[REFAC#276] main.go 헬퍼 메소드를 적합한 패키지로 이동 - #277
Conversation
main 의 책임 (DI / lifecycle 관리) 과 무관한 헬퍼 메소드를 각 도메인 패키지로 이동. 시그니처 보존 + 로직 변경 0 — 순수 이동. 이동 매핑: - buildLLMProvider → pkg/llm/wiring.BuildProvider (LLM provider 조립 — llmgen + refiner 공유, neutral 위치) - buildLLMGenerator → llmgen.Build (llmgen.New + Redis locker setup) - buildRefiner → refiner.Build (refiner.New + RefinementConfig 로딩) - verifyParsingRulesSeeded → rule.VerifySeeded (하드코딩 site 목록 + parsing_rules 도메인 검증, seededHostTargets var 분리) main.go 794 lines → 650 lines. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces local builder/helpers in cmd/issuetracker/main.go with public wiring packages and a seed verifier: introduces ChangesInitialization & Wiring Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
There was a problem hiding this comment.
Code Review
This pull request refactors the main.go entry point by moving wiring and initialization logic for LLM providers, generators, and refiners into dedicated packages. This improves modularity by separating domain logic from infrastructure setup. Feedback from the review indicates that the new Build functions in the llmgen and refiner packages should return errors instead of calling log.Fatal to improve testability and reusability. Additionally, there is a recommendation to further decouple the refiner package from the configuration layer by moving its wiring logic to a separate package.
There was a problem hiding this comment.
Pull request overview
이 PR은 cmd/issuetracker/main.go에 있던 main 책임(DI/lifecycle)과 무관한 헬퍼 함수 4개를 각 도메인 패키지로 이동시켜, main.go를 슬림하게 유지하고 wiring 책임을 패키지 경계에 맞게 재배치합니다.
Changes:
- LLM provider 조립 로직을
pkg/llm/wiring.BuildProvider로 이동 - LLM generator/refiner 구성 로직을 각각
llmgen.Build,refiner.Build로 이동 - parsing_rules seed 검증 로직을
rule.VerifySeeded로 이동하고 seed 타겟 목록을 package var로 분리
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cmd/issuetracker/main.go | 기존 헬퍼 호출부를 신규 wiring 함수들로 교체하고 불필요 import 제거 |
| pkg/llm/wiring/wiring.go | 환경변수/config 기반 LLM provider 구성 헬퍼(BuildProvider) 신설 |
| internal/processor/parser/rule/llmgen/wiring.go | LLM generator 구성 헬퍼(Build)를 llmgen 패키지로 이동 |
| internal/processor/parser/rule/refiner/wiring.go | Refiner 구성 헬퍼(Build)를 refiner 패키지로 이동 |
| internal/processor/parser/rule/seeded.go | parsing_rules seed 검증 헬퍼(VerifySeeded) 및 seed 타겟 목록 분리 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/issuetracker/main.go (1)
454-466:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale comments referencing the removed
buildLLMGenerator.Lines 456 and 463 still mention
buildLLMGenerator, which no longer exists in this file (nowllmgen.Build). Worth updating during this move so future readers don’t grep for a dead symbol.Suggested wording
- // 미지정 / gemini (기본) 일 때는 buildLLMGenerator 가 설정한 기본 LLM provider 추출. + // 미지정 / gemini (기본) 일 때는 llmgen.Build 가 설정한 기본 LLM provider 추출. @@ - // 기본 경로 — 분기 미발생 (Gemini 등 buildLLMGenerator 의 provider 사용). + // 기본 경로 — 분기 미발생 (Gemini 등 llmgen.Build 의 provider 사용).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/issuetracker/main.go` around lines 454 - 466, Update the stale comments and log message that reference the removed symbol buildLLMGenerator to the new API llmgen.Build so readers can find the correct symbol; locate the block around the llmExtractor switch that mentions buildLLMGenerator and change those comment text occurrences to reference llmgen.Build, and update any explanatory text in the log.Warn call related to buildLLMGenerator to mention llmgen.Build (symbols to inspect: buildLLMGenerator, llmgen.Build, llmGen, claudegen.ClaudeWorker, and the log.Warn message).
🧹 Nitpick comments (2)
internal/processor/parser/rule/llmgen/wiring.go (1)
18-18: 💤 Low valueWrap the
Buildsignature for line-length and consistency.Line 18 exceeds the 100-character guideline, and the sibling
refiner.Buildalready uses a multi-line signature. Wrapping it here keeps both wiring entry points stylistically aligned.Proposed formatting
-func Build(provider llm.Provider, repo storage.ParsingRuleRepository, resolver *rule.Resolver, redisClient *redis.Client, log *logger.Logger) *Generator { +func Build( + provider llm.Provider, + repo storage.ParsingRuleRepository, + resolver *rule.Resolver, + redisClient *redis.Client, + log *logger.Logger, +) *Generator {As per coding guidelines: "Maximum line length of 100 characters in Go code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/processor/parser/rule/llmgen/wiring.go` at line 18, The Build function signature exceeds the 100-char limit; reformat the declaration for the Generator constructor so parameters are on separate lines like the sibling refiner.Build. Locate func Build(provider llm.Provider, repo storage.ParsingRuleRepository, resolver *rule.Resolver, redisClient *redis.Client, log *logger.Logger) *Generator and wrap the parameter list across multiple lines (one or two params per line) while keeping the same parameter types (llm.Provider, storage.ParsingRuleRepository, *rule.Resolver, *redis.Client, *logger.Logger) and return type *Generator to maintain style and line-length consistency.internal/processor/parser/rule/refiner/wiring.go (1)
20-27: ⚡ Quick winConsider grouping
Buildparameters into aDepsstruct (follow-up).
Buildtakes 6 parameters, which exceeds the 5-parameter guideline. The PR is a pure move so I’m not asking you to change the signature here, but as a follow-up consider aDeps/Optionsstruct (e.g.,Deps{Provider, Rules, Samples, Resolver, MetricsRegistry, Log}) — this also makes future additions (new repos, additional registries) backward-compatible without ABI churn forcmd/*callers.As per coding guidelines: "Functions must have maximum 5 parameters; use structs for multiple parameters in Go".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/processor/parser/rule/refiner/wiring.go` around lines 20 - 27, Build currently accepts six positional parameters (provider, rules, samples, resolver, metricsRegistry, log); refactor by introducing a Deps (or Options) struct containing those fields (Provider llm.Provider, Rules storage.ParsingRuleRepository, Samples storage.SampleURLRepository, Resolver *rule.Resolver, MetricsRegistry *prometheus.Registry, Log *logger.Logger) and change Build to accept a single Deps parameter (e.g., Build(deps Deps) *Refiner); update all internal call sites (including cmd/* callers) to construct and pass the Deps value, and consider adding a small NewDeps/WithDefaults helper if needed to ease caller changes and avoid ABI churn in future.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cmd/issuetracker/main.go`:
- Around line 454-466: Update the stale comments and log message that reference
the removed symbol buildLLMGenerator to the new API llmgen.Build so readers can
find the correct symbol; locate the block around the llmExtractor switch that
mentions buildLLMGenerator and change those comment text occurrences to
reference llmgen.Build, and update any explanatory text in the log.Warn call
related to buildLLMGenerator to mention llmgen.Build (symbols to inspect:
buildLLMGenerator, llmgen.Build, llmGen, claudegen.ClaudeWorker, and the
log.Warn message).
---
Nitpick comments:
In `@internal/processor/parser/rule/llmgen/wiring.go`:
- Line 18: The Build function signature exceeds the 100-char limit; reformat the
declaration for the Generator constructor so parameters are on separate lines
like the sibling refiner.Build. Locate func Build(provider llm.Provider, repo
storage.ParsingRuleRepository, resolver *rule.Resolver, redisClient
*redis.Client, log *logger.Logger) *Generator and wrap the parameter list across
multiple lines (one or two params per line) while keeping the same parameter
types (llm.Provider, storage.ParsingRuleRepository, *rule.Resolver,
*redis.Client, *logger.Logger) and return type *Generator to maintain style and
line-length consistency.
In `@internal/processor/parser/rule/refiner/wiring.go`:
- Around line 20-27: Build currently accepts six positional parameters
(provider, rules, samples, resolver, metricsRegistry, log); refactor by
introducing a Deps (or Options) struct containing those fields (Provider
llm.Provider, Rules storage.ParsingRuleRepository, Samples
storage.SampleURLRepository, Resolver *rule.Resolver, MetricsRegistry
*prometheus.Registry, Log *logger.Logger) and change Build to accept a single
Deps parameter (e.g., Build(deps Deps) *Refiner); update all internal call sites
(including cmd/* callers) to construct and pass the Deps value, and consider
adding a small NewDeps/WithDefaults helper if needed to ease caller changes and
avoid ABI churn in future.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94754961-328b-49ac-aa8f-e0ef56111634
📒 Files selected for processing (5)
cmd/issuetracker/main.gointernal/processor/parser/rule/llmgen/wiring.gointernal/processor/parser/rule/refiner/wiring.gointernal/processor/parser/rule/seeded.gopkg/llm/wiring/wiring.go
gemini-code-assist 2건 + Copilot 1건 리뷰 반영: 1. (gemini, llmgen.Build) 도메인 패키지에서 log.Fatal 호출 지양 — 라이브러리 재사용 / 테스트 어렵게 함. 에러 반환 + main 이 Fatal 결정. 2. (gemini, refiner.Build) log.Fatal + pkg/config 직접 의존 — pkg/llm/wiring 사례처럼 wiring sub-package 로 분리 권장. 3. (Copilot) llmgen.Build GoDoc 이 이전 buildLLMProvider (main 내부 헬퍼) 를 참조 — 현재 구조 (pkg/llm/wiring.BuildProvider) 반영. 변경 사항: - internal/processor/parser/rule/llmgen/wiring.go (same package) → internal/processor/parser/rule/llmgen/wiring/wiring.go (sub-package) - internal/processor/parser/rule/refiner/wiring.go (same package) → internal/processor/parser/rule/refiner/wiring/wiring.go (sub-package) - 두 Build 모두 시그니처 (..., error) 로 변경 — log.Fatal 제거, fmt.Errorf wrap - main.go 호출부에서 에러 처리 (Fatal) - llmgen wiring GoDoc 에 pkg/llm/wiring.BuildProvider 명시 이로써 도메인 패키지 (llmgen / refiner) 는 pkg/config 등 인프라 설정에 직접 의존하지 않으며, wiring 서브패키지가 그 결합을 흡수. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/processor/parser/rule/refiner/wiring/wiring.go`:
- Around line 36-39: The current logic treats failures from
config.LoadRefinement() as non-fatal by logging a warning and returning (nil,
nil); change this so a failed load returns the error instead of silently
disabling refinement: when config.LoadRefinement() returns an error, propagate
that error (return nil, err or a wrapped error) rather than returning (nil,
nil); keep returning (nil, nil) only for the explicit case where cfg.Enabled is
false (cfg.Enabled check), and ensure callers receive an error for malformed
config values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28f8f256-3faa-4c20-8655-5c336c127cbb
📒 Files selected for processing (3)
cmd/issuetracker/main.gointernal/processor/parser/rule/llmgen/wiring/wiring.gointernal/processor/parser/rule/refiner/wiring/wiring.go
CodeRabbit 리뷰 반영: refiner/wiring/wiring.go 의 config.LoadRefinement() 실패가 warn + (nil, nil) 로 silent disable → malformed env 값이 refiner 를 조용히 끄고 main 의 error-return 경로 우회. (nil, nil) 은 explicit !cfg.Enabled 경로에 한정. config load 실패는 fmt.Errorf wrap 하여 명시적으로 에러 반환 — 호출자 (main) 가 Fatal 결정. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/processor/parser/rule/refiner/wiring/wiring.go (1)
28-35: ⚡ Quick win
Buildhas 6 parameters — exceeds the 5-parameter limit; consider an Options struct.As per coding guidelines, Go functions must have a maximum of 5 parameters; when exceeded, parameters should be grouped into an Options/Config struct.
Buildaccepts 6:provider,rules,samples,resolver,metricsRegistry,log.♻️ Proposed refactor
+// Options holds the dependencies required to construct a refiner.Refiner. +type Options struct { + Provider llm.Provider + Rules storage.ParsingRuleRepository + Samples storage.SampleURLRepository + Resolver *rule.Resolver + MetricsRegistry *prometheus.Registry + Log *logger.Logger +} + -func Build( - provider llm.Provider, - rules storage.ParsingRuleRepository, - samples storage.SampleURLRepository, - resolver *rule.Resolver, - metricsRegistry *prometheus.Registry, - log *logger.Logger, -) (*refiner.Refiner, error) { +func Build(opts Options) (*refiner.Refiner, error) {Then update all internal references from
provider→opts.Provider,rules→opts.Rules, etc.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/processor/parser/rule/refiner/wiring/wiring.go` around lines 28 - 35, Build currently takes six parameters (provider, rules, samples, resolver, metricsRegistry, log) which violates the 5-parameter rule; create a new options/config struct (e.g., RefinerOptions) that contains fields Provider llm.Provider, Rules storage.ParsingRuleRepository, Samples storage.SampleURLRepository, Resolver *rule.Resolver, MetricsRegistry *prometheus.Registry, Log *logger.Logger, change the Build signature to Build(opts RefinerOptions) (*refiner.Refiner, error), update the function body to reference opts.Provider, opts.Rules, etc., and then update all call sites that invoke Build(...) to construct and pass a RefinerOptions value instead of positional parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/processor/parser/rule/refiner/wiring/wiring.go`:
- Line 23: Update the stale GoDoc in
internal/processor/parser/rule/refiner/wiring/wiring.go: remove or change the
clause that claims "(nil, nil) 은 ... config load 실패 시" so the comment matches
current behavior (per PR `#277`) — i.e., document that (nil, nil) only indicates
refinement disabled (REINMENT_ENABLED=false) and that config load failures now
return (nil, error); locate the comment containing the literal "(nil, nil) 은 정밀화
비활성..." and adjust it to explicitly state the new return semantics.
---
Nitpick comments:
In `@internal/processor/parser/rule/refiner/wiring/wiring.go`:
- Around line 28-35: Build currently takes six parameters (provider, rules,
samples, resolver, metricsRegistry, log) which violates the 5-parameter rule;
create a new options/config struct (e.g., RefinerOptions) that contains fields
Provider llm.Provider, Rules storage.ParsingRuleRepository, Samples
storage.SampleURLRepository, Resolver *rule.Resolver, MetricsRegistry
*prometheus.Registry, Log *logger.Logger, change the Build signature to
Build(opts RefinerOptions) (*refiner.Refiner, error), update the function body
to reference opts.Provider, opts.Rules, etc., and then update all call sites
that invoke Build(...) to construct and pass a RefinerOptions value instead of
positional parameters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34048b12-c864-490b-accf-91cbe86490c3
📒 Files selected for processing (1)
internal/processor/parser/rule/refiner/wiring/wiring.go
CodeRabbit 리뷰 반영: 직전 commit 0836e96 으로 config load 실패가 (nil, error) 로 변경됐으나 GoDoc 이 여전히 (nil, nil) 로 표기. 현재 시그니처 반영. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
연관 이슈
구현 내용
cmd/issuetracker/main.go(794 lines) 에서 main 책임 (DI / lifecycle 관리) 과 무관한 헬퍼 메소드 4개를 각 도메인 패키지로 이동.시그니처 보존 + 로직 변경 0 — 순수 이동만.
이동 매핑
buildLLMProviderpkg/llm/wiring/wiring.goBuildProviderbuildLLMGeneratorinternal/processor/parser/rule/llmgen/wiring.goBuildbuildRefinerinternal/processor/parser/rule/refiner/wiring.goBuildverifyParsingRulesSeededinternal/processor/parser/rule/seeded.goVerifySeeded부가 개선
verifyParsingRulesSeeded의 하드코딩 site 목록을seededHostTargetspackage var 로 분리 — 추가 검증 / 외부화 작업 시 단일 지점.main.go 영향
fmt,prometheus,storage,llm,chain,policy)pkg/llm/wiringimport 1개 추가 (aliasllmwiring)CI / 머지 게이트 점검
변경 영향 범위
cmd/issuetracker,pkg/llm/wiring(신규),internal/processor/parser/rule,internal/processor/parser/rule/llmgen,internal/processor/parser/rule/refinerLow— 시그니처 / 동작 변경 0, 패키지 이동만Required Status Checks
Commit LintPR Title LintLinked Issue CheckBuildTestLint로컬 검증
make fmt통과make build5개 binary 모두 통과go test -race ./...전체 통과go vet ./...통과롤백 계획
본 PR 은 코드 이동만으로 동작 변경 없음. 단순 revert 만으로 즉시 롤백 가능.
🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Bug Fix