fix: Pipeline improvements - reasoning truncation and dead code removal#367
Conversation
- compact-formatter.ts: Add '...' when reasoning is truncated (was cutting mid-sentence) - auto-approve.ts: Remove dead code block (maxLines check unreachable after nonTrivialLines>0) Fixes: #306 Fixes: #307
📝 WalkthroughWalkthroughRemoved unreachable dead code from auto-approve.ts that checked for blank-lines-only diffs with maxLines conditions. Updated compact-formatter.ts to conditionally append ellipsis when reasoning text exceeds 200 characters, fixing silent truncation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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)
packages/core/src/pipeline/auto-approve.ts (1)
97-99:⚠️ Potential issue | 🟠 Major
maxLinesparameter is unused and should be removed from the function signature and configuration schema.The
analyzeTrivialDifffunction acceptsmaxLinesin its config parameter, andAutoApproveConfigSchemaexposes it to users, butmaxLinesis never referenced in the function body. The triviality determination is based solely on file patterns and content types (imports, comments, blanks), not line count limits.This creates a dead configuration option where users can set
maxLinesexpecting it to limit trivial diff detection, but it has no effect on behavior.Remove
maxLinesfrom:
- Function signature at line 99
AutoApproveConfigSchemainpackages/core/src/types/config.ts:219- Template defaults in
packages/core/src/config/templates.ts:104🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/pipeline/auto-approve.ts` around lines 97 - 99, The config option maxLines is unused—remove it from the analyzeTrivialDiff function signature (delete the maxLines property from the config param in analyzeTrivialDiff) and update the public config types and defaults: remove maxLines from AutoApproveConfigSchema in the config types (AutoApproveConfigSchema) and from the template defaults in packages/core/src/config/templates.ts (the auto-approve template block). Also search for any callers or tests referencing maxLines and update them to stop passing or expecting it so types and runtime remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/core/src/pipeline/auto-approve.ts`:
- Around line 97-99: The config option maxLines is unused—remove it from the
analyzeTrivialDiff function signature (delete the maxLines property from the
config param in analyzeTrivialDiff) and update the public config types and
defaults: remove maxLines from AutoApproveConfigSchema in the config types
(AutoApproveConfigSchema) and from the template defaults in
packages/core/src/config/templates.ts (the auto-approve template block). Also
search for any callers or tests referencing maxLines and update them to stop
passing or expecting it so types and runtime remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb4aa756-5c07-47fd-a7c4-c569087fc64f
📒 Files selected for processing (2)
packages/core/src/pipeline/auto-approve.tspackages/core/src/pipeline/compact-formatter.ts
justn-hyeok
left a comment
There was a problem hiding this comment.
LGTM! Reasoning truncation adds '...' within 200-char budget. Dead code removal verified safe.
Fix two pipeline issues:
compact-formatter.ts: Add '...' when reasoning is truncated
easoning.slice(0, 200) cut mid-sentence
easoning.slice(0, 197) + '...' when truncated
auto-approve.ts: Remove dead code block
onTrivialLines > 0
Fixes: #306
Fixes: #307
Summary by CodeRabbit