Add AI-assisted triage#3
Conversation
Also make the agent perma prompt part of the package
- make it part of the markdown when that is used - save to separate file if not used - print to stdout for ai-triage command
grayson-wolf
left a comment
grayson-wolf
left a comment
There was a problem hiding this comment.
looks good, to me, just a few suggestions / nit-questions
TheJJ
left a comment
TheJJ
left a comment
There was a problem hiding this comment.
coming together pretty nicely. the overall architecture is good, i'd say.
one thing that worries me is the implicit assumption this is run in a secure container now:
how about a text-only analysis without funny containers as the first agent, and when it decides we wanna run shell stuff, it spins up a subagent in an lxd container (using the same mechanism we use for git ubuntu deltarebase resolving - we can hopefully create a shared ai lxd containering library so we don't have to duplicate/reinvent stuff)
otherwise many nits and naming issues, you'll see. great work, overall :)
| logger.debug("Starting Copilot session (model=%s)", self.model) | ||
| async with CopilotClient(**build_client_kwargs(self._ai_config)) as client: | ||
| async with await client.create_session( | ||
| on_permission_request=PermissionHandler.approve_all, |
There was a problem hiding this comment.
that should only be done in the container. people should assume running startriage on their hosts doesn't extract precious cat pictures.
Accept only a bare/#-prefixed integer or a genuine Launchpad bug URL (.../+bug/<n> or /bugs/<n>) and raise ValueError otherwise.
Plain 'analyze <bug>' now prints the bug metadata (status, tags, affected targets, description, comments) with no provider or credentials required; 'analyze --ai <bug>' runs the agent as the old 'ai-triage' command did.
triage --ai no longer writes a separate autotriage-YYYY-MM-DD.md file. Without --markdown the AI section is printed to stdout after the normal triage output; with --markdown it is folded into that same file behind the review notice, giving one cohesive report. Drops the now-unused report_filename/write_report/resolve_report_dir helpers and their tests, and updates the prompt and README accordingly.
|
@TheJJ I addressed most of your review points, responded to a couple inline. I see your point with the two-step agent run, but I would ask you if we could not do this in a future iteration. You mentioned there may be some overlap in the "Run this container and do stuff" tasks - let's go for the shared LXD library then. But the size and scope of this PR make me thing later is better. For now we can tell users that this is potentially unsafe, and ask them to please containerize (or isolate the environment right) before running AI commands. WDYT? |
TheJJ
left a comment
TheJJ
left a comment
There was a problem hiding this comment.
just minor things left, i think for a first version we are nearly ready.
this runs the ai analysis within the snap confinement, but as future work we should add properly containerized ai execution - maybe open an issue for that then :)
| def _build_ai_provider(config: StarTriageConfig) -> Provider | None: | ||
| """Build the AI provider, printing a friendly hint and returning None on misconfig.""" | ||
| from .ai import build_provider | ||
|
|
||
| try: | ||
| return build_provider(config.ai) | ||
| except AIConfigError as exc: | ||
| print(f"error: {exc}", file=sys.stderr) | ||
| return None |
There was a problem hiding this comment.
shouldn't be part of cli.py, rather ai/provider.py. return value should always be Provider, and if that fails, let the AIConfiError be raised to top for crash.
| def _emit_ai_report(report: str, markdown_path: Path | None) -> None: | ||
| """Emit an AI ``report`` for a ``triage --ai`` run. | ||
|
|
||
| With ``--markdown`` the AI section is appended (behind a review notice) to | ||
| that same file, so the human triage report and the AI aid live in one | ||
| cohesive document. Without ``--markdown`` the report is printed to stdout, | ||
| together with the normal triage output that already went there. | ||
| """ | ||
| from .ai import append_report | ||
| from .ai.render import AI_APPEND_NOTICE | ||
|
|
||
| if markdown_path is not None: | ||
| append_report(markdown_path, report) | ||
| print(f"AI triage appended to {markdown_path}") | ||
| else: | ||
| print("\n" + AI_APPEND_NOTICE + report) |
| return self.github_token or _first_env(COPILOT_TOKEN_ENV_VARS) | ||
| return self.openrouter_api_key or _first_env(OPENROUTER_KEY_ENV_VARS) | ||
|
|
||
| def require_configured(self) -> None: |
There was a problem hiding this comment.
@model_validator(mode='after')
def check_token(self) -> Self:
if self.resolve_token() is not None:
return self
match self.provider:
case AIProvider.copilot:
...
raise ValueError('bla')
case _:
raise RuntimeError("unhandled provider")
Move the missing-credential check from AIConfig.require_configured() onto a model validator that only enforces when validated with a require_ai context. This keeps the [ai] section optional for non-AI commands while build_provider re-validates with that context so a misconfigured provider fails before a session starts. AIConfigError is not a ValueError, so pydantic propagates it unwrapped to the top.
|
@TheJJ issue open Claude is worried that the model validator will break users who have no [ai] configured when trying to run non-ai commands, so it proposed something in-the-middle. |
3 participants
This adds the whole AI-assisted functionality to startriage. It's full of abstractions and details but it works well.
--aifor usual triage second opinionai-triagefor single bugs