-
-
Notifications
You must be signed in to change notification settings - Fork 3
fix: address bot-review findings on the generator-profile PRs #180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -173,7 +173,11 @@ def _generator_profile_set(profile_id: str, agent=None, data_dir=None) -> int: | |
| print(f"error: unknown profile {profile_id!r}", file=sys.stderr) | ||
| return 1 | ||
| if agent: | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: Inconsistent with Reply with |
||
| print(f"error: agent {agent!r} is not registered", file=sys.stderr) | ||
| return 1 | ||
| print(f"agent {agent}: generator profile = {profile_id}") | ||
| else: | ||
| config.set_generator_profile(profile_id, data_dir=data_dir) | ||
|
|
@@ -1776,11 +1780,11 @@ def main(argv: list[str] | None = None) -> int: | |
|
|
||
| if args.cmd == "generator-profile": | ||
| if args.generator_profile_cmd == "list": | ||
| return _generator_profile_list() | ||
| return _generator_profile_list(data_dir=args.data_dir) | ||
| if args.generator_profile_cmd == "show": | ||
| return _generator_profile_show(args.profile_id) | ||
| return _generator_profile_show(args.profile_id, data_dir=args.data_dir) | ||
| if args.generator_profile_cmd == "set": | ||
| return _generator_profile_set(args.profile_id, agent=args.agent) | ||
| return _generator_profile_set(args.profile_id, agent=args.agent, data_dir=args.data_dir) | ||
|
|
||
| parser.print_help() | ||
| return 1 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1045,7 +1045,7 @@ def _handle_generator_profile_post(self) -> None: | |
| _agents.set_agent_generator_profile(agent, pid, data_dir=data_dir) | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: Reply with |
||
| self._send_json(400, {"error": str(exc)}) | ||
| return | ||
| profiles = [ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -260,7 +260,7 @@ async def process_conversation_turn( | |
|
|
||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: Reply with |
||
| facts = await extract_facts_with_llm( | ||
| text, llm_url, http_client, | ||
| agent_name=agent_name or "default", | ||
|
|
||
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WARNING: The
ErrorBannerrendered at line 88 wiresonRetry={() => void loadProfile(scope)}, but the error you just set comes fromgetStats()in thisuseEffect, not fromloadProfile. Clicking Retry will (a) re-fetch the profile (which may not be the failing call) and (b) immediately clear the error viasetError(null)insideloadProfile, butagentNameswill remain empty andgetStats()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.,
retryFnlocal state, or split the error banner sogetStatsfailures have a separate retry handler that re-runs theuseEffect).Reply with
@kilocode-bot fix itto have Kilo Code address this issue.