fix: address bot-review findings on the generator-profile PRs - #180
Conversation
…agent resolve on extraction, narrow POST except + tests
…rface, not silent-empty)
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
✨ 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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| else: | ||
| _cfg.set_generator_profile(pid, data_dir=data_dir) | ||
| except Exception as exc: # e.g. AgentNotFoundError | ||
| except (_agents.AgentNotFoundError, ValueError) as exc: |
There was a problem hiding this comment.
SUGGESTION: ValueError is a very broad catch — this can swallow programmer errors (e.g., a KeyError re-raised as ValueError, bad config types). The PR description says this was narrowed from a bare except Exception, which is good, but consider catching the specific validation error the underlying call raises rather than all of ValueError. If set_agent_generator_profile is the only relevant caller and it only raises AgentNotFoundError, the ValueError arm may be dead code.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| agents.set_agent_generator_profile(agent, profile_id, data_dir=data_dir) | ||
| try: | ||
| agents.set_agent_generator_profile(agent, profile_id, data_dir=data_dir) | ||
| except agents.AgentNotFoundError: |
There was a problem hiding this comment.
WARNING: Inconsistent with http_server.py:1048 which catches both AgentNotFoundError and ValueError. Today set_agent_generator_profile only raises AgentNotFoundError, but if a future validation (e.g., profile_id name rules) raises ValueError, the CLI will surface an uncaught traceback — which is exactly what the PR set out to fix for the unknown-agent case. Consider catching (agents.AgentNotFoundError, ValueError) here too for symmetry.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| .catch((err: unknown) => { | ||
| // Surface via the existing ErrorBanner so the user sees a signal rather | ||
| // than silently falling back to the global "all" scope only. | ||
| setError(err instanceof Error ? err.message : String(err)); |
There was a problem hiding this comment.
WARNING: The ErrorBanner rendered at line 88 wires onRetry={() => void loadProfile(scope)}, but the error you just set comes from getStats() in this useEffect, not from loadProfile. Clicking Retry will (a) re-fetch the profile (which may not be the failing call) and (b) immediately clear the error via setError(null) inside loadProfile, but agentNames will remain empty and getStats() will not be retried. The user sees a misleading "retry" affordance that doesn't address the underlying failure.
Suggested fix: track which fetch errored and route retry accordingly (e.g., retryFn local state, or split the error banner so getStats failures have a separate retry handler that re-runs the useEffect).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| resolved_model = extraction_model | ||
| if not resolved_model or resolved_model == "default": | ||
| resolved_model = resolve_memory_model() or "default" | ||
| resolved_model = resolve_memory_model(agent=agent_name or "default") or "default" |
There was a problem hiding this comment.
SUGGESTION: agent_name or "default" will almost never match a registered agent, so resolve_generator silently falls back to the global profile anyway. The agent= kwarg only adds value when agent_name is truthy. Consider resolve_memory_model(agent=agent_name) if agent_name else resolve_memory_model() so the per-agent lookup only runs when the caller actually has a registered agent, and document the fallback semantics.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (13 files)
Fix these issues in Kilo Cloud Reviewed by minimax-m3 · Input: 42.3K · Output: 5.7K · Cached: 494.7K |
1 participant
Sweeps the real CodeRabbit + kilo-code-bot findings on #177/#178 (auto-merged before the bots posted) into one pass. Whole-branch reviewed READY TO MERGE.
--data-dirnow threaded to the generator-profile subcommands (was writing the default store).--agentreturns a clean error + nonzero instead of a traceback.resolve_memory_modelgains anagentparam (fallback stays first positional; callers converted to keyword), forwarded so per-agent profiles are honored on the extraction path;resolve_generatorguards the lookup so an unregistered/"default" agent falls back to global (no crash).exceptnarrowed to (AgentNotFoundError, ValueError); other errors propagate to 500.getStats()failure via ErrorBanner instead of silent-empty.