feat: add -kb to make page type classification opt-in - #2554
feat: add -kb to make page type classification opt-in#2554dogancanbakir wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. WalkthroughThe change adds knowledge-base classification configuration, derives classification from page-type filters, updates output settings, and changes classifier initialization errors to returned wrapped errors. ChangesClassification control
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change makes page-type classification opt-in, reducing unnecessary model downloads while preserving classification when explicitly requested; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant RunnerOptions
participant Runner
participant dit
RunnerOptions->>Runner: Enable classification from flags or filters
Runner->>dit: Create classifier with dit.New()
dit-->>Runner: Return classifier or initialization error
Runner-->>RunnerOptions: Continue or return wrapped error
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@runner/classifier.go`:
- Around line 19-21: Protect the temporary default logger replacement in
Runner.New with a package-level mutex, locking before slog.Default is saved and
slog.SetDefault installs gologgerSlogHandler, and unlocking only after the
deferred restoration completes. Ensure the mutex covers the entire save,
replacement, and restore sequence so overlapping Runner.New calls cannot leave
the temporary handler installed.
In `@runner/runner.go`:
- Around line 434-442: Update the newDitClassifier error handling within the
classificationEnabled path to return the wrapped initialization error
unconditionally, removing the hasPageTypeFilter distinction and non-fatal
logging path. Ensure every enabled-classification failure prevents returning a
Runner with a nil classifier.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3be27126-e8e0-40d8-82b4-83d33926eb23
📒 Files selected for processing (5)
README.mdrunner/classifier.gorunner/options.gorunner/runner.gorunner/runner_test.go
cb58e92 to
9bcc668
Compare
|
@dogancanbakir the failing test was fixed in #2555, can you rebase and merge this change? thanks |
9bcc668 to
1be2364
Compare
Closes #2543
-json/-csvinitialized the dit classifier, downloading a ~92MB model with no way to opt out. Classification is now opt-in via-kb, implied by-fpt/-fep.Supersedes #2544 (thanks @jatinder14).
Summary by CodeRabbit
New Features
-knowledge-baseand-kbcommand-line options.Bug Fixes
Documentation