Virt-Manager review support

resources/false-positive-digest-virt-manager.md

@@ -0,0 +1,87 @@
+<!-- SPDX-License-Identifier: Apache-2.0 -->
+
+# What NOT to flag — false-positive digest (virt-manager specialist stages)
+
+This is a tight subset of `false-positive-guide.md` distilled for the specialist
+stages. Apply these rules BEFORE you emit a concern, not after. If a concern
+fails any applicable rule, drop it. The full guide still runs at consolidation;
+your job here is to stop weak concerns at the source.
+
+## Core principle
+
+If you cannot point to **specific code in the diff** that proves the issue, do
+not emit it. "Could happen," "might race," "should validate" → drop.
+
+## Concrete rules
+
+### 1. Defensive programming requests
+Do NOT request `None`/type/bounds checks or input validation unless you can show
+an actual code path in the diff that reaches the use without an intervening
+check, AND the value can realistically take the bad form there. Generic "add a
+None check for safety" with no reaching path: drop. Python raising a clean
+`ValueError`/`libvirt.libvirtError` that a caller already handles is not a bug.
+
+### 2. API misuse assumptions
+Do NOT report "caller might pass None" or "this could be the wrong type" without
+showing the actual calling path. Many virtinst methods document/assume a built
+object or an open connection; check the callers before flagging.
+
+### 3. Unverifiable claims from the commit message
+The commit message is not evidence on its own. Verify the author's "this is safe
+because Y" from the code. Do not flag only on a missing justification.
+
+### 4. GTK thread-safety — trace the call context
+Before flagging a "widget touched off the main thread" concern, confirm the code
+actually runs on a worker thread. Code inside a normal signal handler, a
+`GLib.idle_add` callback, or the main event loop IS on the main thread and may
+touch widgets freely. Only flag when a `vmmAsyncJob`/thread callback reaches
+widget/GObject state without marshaling back via `idle_add`.
+
+### 5. Exceptions vs. crashes
+A raised, caught exception is control flow, not a crash. Only flag an unhandled
+exception when a realistic input reaches a raise that no caller in the diff (or
+its documented callers) handles, producing a traceback or a broken UI state.
+
+### 6. "Leaks" in a garbage-collected language
+Python frees unreferenced objects, and its cyclic GC reclaims pure-Python
+reference cycles. Do NOT flag a local that goes out of scope, or a plain
+reference cycle, as a leak. Real leaks here are: a libvirt connection/stream or
+file handle never closed, a GObject signal handler connected but never
+disconnected so it fires on a stale object, or a reference cycle that pins a
+GObject/widget alive. Show the retained reference or the missing
+`disconnect`/`close`.
+
+### 7. Races — show two concurrent paths
+A race concern must name both paths and the contested state. "X could race with
+Y" with no specific call sites is not a finding.
+
+### 8. None / attribute access
+Before flagging an `AttributeError`/`None` deref, check whether an earlier line
+(or the calling convention) guarantees the value is set. A property that returns
+a default, a value already checked with `if x is None: return`, or an object the
+method requires to be built does not need another guard downstream.
+
+### 9. Style and naming
+Do not emit style complaints (naming, function size, comment style, f-string vs
+%) as substantive findings. They belong in commit-message concerns (`msg:style`)
+at most, and usually not at all.
+
+### 10. Patch-series context
+If the diff is patch N of a series and a concern is fixed in a later patch of the
+same series, treat it as not-a-finding (or note the later-fix patch).
+
+### 11. Fixed old-code bugs
+Do NOT emit a concern merely because the removed/old code had a real bug. If the
+reviewed diff fixes that behavior, the old bug is evidence the patch is a fix,
+not a finding. Only report it if the new code still has the bug, the fix is
+incomplete, or the patch introduces a different bug.
+
+## How to write a concern that survives consolidation
+
+- Name the function, file, and line/region from the diff.
+- Quote or paraphrase the specific code, not the general pattern.
+- State the trigger condition that actually fires (not "if input is malformed").
+- For thread-safety/leak/race, name the thread context or the retained reference.
+
+If you cannot do these for a concern, **do not emit it**. The consolidator will
+not invent the evidence; it can only drop what you wrote.

resources/one-shot-review-virt-manager.md

@@ -0,0 +1,58 @@
+<!-- SPDX-License-Identifier: Apache-2.0 -->
+
+# Single-pass review (boro, virt-manager)
+
+You are performing **one consolidated pass** over a virt-manager patch that must
+cover the same dimensions as the multi-stage protocol, without running separate
+model calls per stage. The codebase is Python: the `virtinst` library (domain
+XML building, device models, install/guest logic, the `virt-install`/`virt-xml`
+CLI) and the `virtManager` GTK GUI, both driving libvirt through `libvirt-python`.
+
+1. **Intent / architecture** — CLI/API/GUI design, `XMLBuilder` model changes,
+   maintainability, conceptual flaws.
+2. **Commit message** — English spelling, grammar, syntax, and clarity (subject
+   and body); misleading or incomplete changelog vs the diff; missing updates,
+   semantic correctness.
+3. **Execution flow** — branches, exception handling, early returns, off-by-one,
+   and especially `None` handling (an attribute/lookup that can be `None` used
+   without a guard raises `AttributeError`/`TypeError`). Track the identity of
+   every validated value through its final use; a check on one object does not
+   transfer to a different one.
+4. **Resources** — Python is garbage-collected, so focus on: leaked or
+   never-closed libvirt connections/streams and file handles, GObject/GTK signal
+   handlers connected but never disconnected (callbacks firing on dead objects),
+   reference cycles that pin a GObject/widget alive (CPython's GC already
+   reclaims pure-Python cycles), and background jobs not cleaned up.
+5. **Concurrency / GTK thread-safety** — GTK is **not** thread-safe: touching
+   widgets or GObject state off the main thread is a bug. Background work runs
+   via `vmmAsyncJob`/worker threads and must marshal UI updates back with
+   `GLib.idle_add` (or equivalent). Flag widget/model access from a worker thread.
+6. **Security / correctness of generated artifacts** — validate user/config
+   input; generate valid, safe domain XML (proper escaping via the builder, no
+   hand-concatenated XML) and safe commands (`virtinst` builds argv, never a
+   `shell=True` string from untrusted data). No `eval`/`exec` on external input.
+7. **Portability** — Python version compatibility, optional dependencies guarded
+   with `try/except ImportError`, and correct `gi.require_version(...)` before
+   `from gi.repository import ...`. This is not a compiled-language build audit.
+8. **XML / CLI surface** — `XMLBuilder` parse↔format round-trips (a new property
+   must both parse and re-serialize), CLI option wiring in `cli.py`, and
+   `libvirt-python` error handling (`libvirt.libvirtError` caught where the API
+   can raise).
+
+Every finding must carry concrete proof appropriate to its issue type: the
+relevant code facts, a reachable trigger or witness when applicable, the
+violated invariant or direct contradiction, and the concrete failure or
+user-visible defect (a traceback, a malformed XML, a hung/blocked UI, a wrong
+command). Do not use "may", "might", "could", or "not guaranteed" as a
+substitute for missing evidence.
+
+Be skeptical of the commit message. Prefer reporting a suspected issue with
+clear reasoning over silence.
+
+Do not report the bug that the patch is fixing. A defect visible only in
+removed/old code is not a finding when the new/right-side diff fixes it. Report
+only if the new code still has the defect, the fix is incomplete, or the patch
+introduces a different bug.
+
+When the diff is documentation-only or trivial comment fixes, return an empty
+`findings` array.

resources/prompts/virt-manager/callstack.md

@@ -0,0 +1,49 @@
+# Execution-Flow and Call-Stack Verification (Virt-manager)
+
+When you suspect a bug, **prove the reachable path** before reporting it. A
+finding without a concrete call chain is usually a false positive.
+
+## Build the chain explicitly
+
+State the path from an entry point (a GUI signal, a CLI option, a libvirt event)
+to the defect, naming each function:
+
+```
+vmmConnection._tick()            <- background polling thread
+  -> vmmDomain.tick()            <- refreshes cached domain state (worker thread)
+    -> self.emit("state-changed")  <- emit off the main thread; a handler that
+                                      touches widgets must go via idle_emit (bug)
+```
+
+## Virt-manager entry points worth tracing back to
+
+- **GUI signals**: Glade/`.ui` `connect`ed handlers (`on_*_clicked`,
+  `_signal_*`) — these run on the main thread.
+- **Background threads**: `vmmConnection` polling/`_tick`, `vmmAsyncJob` worker
+  callbacks, and libvirt event callbacks — these are **not** the main thread.
+- **CLI**: `virtinst/cli.py` `Parser*` classes → `virtinst` object setters →
+  `XMLBuilder` → `Installer.start_install(guest)` / domain define.
+- **libvirt events**: lifecycle/agent callbacks registered on the connection.
+
+## Context questions to answer
+
+- **Which thread am I on?** If the code path originates in polling / an async job
+  / a libvirt event callback, any GTK widget access must go through
+  `idle_add`/`idle_emit`. If it originates in a GUI signal handler, it's already
+  on the main thread.
+- **Can this value be None?** Trace whether a libvirt lookup, an optional XML
+  node, or a dict `.get()` upstream can yield `None` before this use.
+- **Does an exception escape?** If a `libvirtError`/`OSError` can be raised here,
+  is it caught at a level that shows the user an error rather than crashing the
+  app or aborting a multi-step operation half-done?
+- **Is the target correct?** For a destructive action (delete/overwrite), is the
+  object it operates on the one the user selected?
+
+## Confirm, don't assume
+
+- If the None/error check is in the caller or a base method, the "missing check"
+  is not a bug — show it.
+- If the code is reached only from a GUI signal handler, it's on the main thread
+  — a "needs idle_add" claim is wrong; read the callers.
+- Read to the end of the method (and any `finally`) before claiming a leaked
+  reference or unhandled exception.

resources/prompts/virt-manager/coding-style.md

@@ -0,0 +1,49 @@
+# Virt-manager Coding Style
+
+Virt-manager is Python. Style is enapsulated by `pycodestyle` (PEP 8) and
+`pylint`, run via the test suite (`pytest`, the `test_dist`/lint targets). Flag
+deviations as **Low** severity unless a style issue also causes a real bug (then
+use the bug's severity). Be specific and quote the offending line; do not
+bikeshed.
+
+## Layout
+
+- **4-space indentation, never tabs.** No trailing whitespace; files end with a
+  newline.
+- Follow PEP 8 line length as enforced by the repo's `pycodestyle` config; wrap
+  long lines rather than disabling the check.
+- Two blank lines between top-level defs/classes, one between methods.
+
+## Naming
+
+- `lower_snake_case` for functions, methods, variables; `UpperCamelCase` for
+  classes (virt-manager GUI classes are prefixed `vmm`, e.g. `vmmDomain`,
+  `vmmConnection`); `UPPER_SNAKE_CASE` for constants.
+- "Private" attributes/methods use a leading underscore; don't reach into
+  another object's `_private` members.
+
+## Idioms
+
+- Prefer explicit `is None` / `is not None` over truthiness when `0`/`""`/empty
+  are valid values.
+- Use context managers (`with open(...) as f:`) for files; don't leave file
+  handles dangling.
+- Use f-strings or `.format()` for interpolation; user-facing strings in the GUI
+  are wrapped for translation with `_()` (gettext).
+- `except Exception` (not bare `except:`); log via the module `log`
+  (`from virtinst import log`) rather than `print`.
+
+## Imports
+
+- Standard library, then third-party (`gi`/GObject, `libvirt`), then local
+  (`virtinst`, `virtManager`) groups. No unused imports (pylint flags them).
+- GTK is imported through `gi` with explicit versions
+  (`gi.require_version("Gtk", "3.0")`).
+
+## What to flag (and how)
+
+- Quote the line and name the rule: "tab indentation; project uses 4 spaces",
+  "bare `except:`; use `except Exception`", "mutable default argument",
+  "unused import", "missing `_()` on a user-facing GUI string".
+- Keep style points Low and brief. Lead with substantive correctness findings;
+  don't pad the report with nits.

resources/prompts/virt-manager/false-positive-guide.md

@@ -0,0 +1,52 @@
+# Avoiding False Positives (Virt-manager)
+
+Most rejected review comments are false positives. Before keeping a finding,
+clear it against this guide. When in doubt and you cannot prove the path,
+**drop it** — a wrong comment costs maintainer trust.
+
+## Read enough context first
+
+- Use `read_files` / `git_show` / `git_blame` to read the **whole** function/
+  method and its callers, not just the diff hunk. Many "missing None checks" or
+  "missing idle_add" are handled by a base class, a decorator, or one frame up.
+- The diff shows a delta. The bug must be real in the *resulting* code.
+
+## Virt-manager-specific non-bugs (do NOT report these)
+
+- **`idle_add` already in the call chain**: virt-manager has helpers
+  (`vmmGObject.idle_add`, `vmmGObject.idle_emit`) and patterns where the caller
+  is already on the main thread. Only flag a thread-safety issue if you can show
+  the code runs on a worker thread *and* touches widgets without marshalling.
+- **GObject source cleanup**: `vmmGObject` tracks signal connections made
+  through its connection helpers and timers made through `timeout_add()`.
+  `idle_add()` is not tracked for cleanup, but its callback normally removes
+  itself after returning `False`/`None`. Do not report a leak merely because a
+  raw or wrapped GLib source is used. Trace whether it is self-removing,
+  explicitly removed, or can remain active after its owning object is cleaned
+  up. Keep a finding only when the surviving source has a concrete stale-object
+  or retention consequence.
+- **`XMLProperty` round-trips**: a new attribute wired via `XMLProperty` parses
+  and formats through the builder; you don't need a manual getter/setter.
+- **`except libvirt.libvirtError` then continue**: catching a libvirt error to
+  show a dialog / fall back is the normal pattern, not swallowed.
+- **CLI option not "validated"**: `virtinst/cli.py` defers most validation to
+  libvirt when the domain is defined; a value passed through to XML that libvirt
+  will reject is not necessarily a virt-install bug. Flag only when virt-manager
+  itself would crash or silently mis-set it.
+- **Test-only code**: changes under `tests/` follow different conventions; don't
+  apply production-path expectations to fixtures and mocks.
+
+## Reportable vs not
+
+- Reportable: a concrete, reachable path where input or a libvirt return causes
+  a traceback, a worker-thread UI access, a destructive action on the wrong
+  target, a shell-injection, or invalid/dangerous generated XML — with the chain
+  shown.
+- Not reportable: "could be cleaner", "might want a check" with no demonstrated
+  trigger, or speculation about callers you didn't read.
+
+## Calibrate to the changelog
+
+If the commit message explains a trade-off or says a follow-up handles X, factor
+that in. Don't report something the author documented as intentional — unless
+the code contradicts the message (that mismatch *is* reportable).

resources/prompts/virt-manager/inline-template.md

@@ -0,0 +1,65 @@
+Produce a report of regressions found based on this template.
+
+- The report must be in plain text only. No markdown, no special characters,
+  absolutely and completely plain text fit for a review comment on a
+  virt-manager / virtinst GitHub pull request.
+
+- Any long lines present in the unified diff should be preserved, but any
+  summary, comments, or questions you add should be wrapped at 76 characters.
+
+- Never include bugs filtered out as false positives in the report.
+
+- Always end the report with a blank line.
+
+- The report must be conversational with undramatic wording, fit for posting as
+  a review on the pull request.
+  - The report must be **factual** — just technical observations.
+  - Frame issues as **questions**, not accusations.
+  - Call issues "regressions" or describe the concrete effect; never use the
+    word "critical" and never use ALL CAPS.
+
+- Explain the regressions as questions about the code, but do not address the
+  author personally.
+  - Don't say: "Did you forget idle_add here?"
+  - Instead say: "Is this called from the polling thread? If so, does the
+    set_text() need to be marshalled with idle_add?"
+
+- Vary your phrasing. Don't start every point with "Does this code ...".
+
+- Ask your question specifically about the thing you are referencing:
+  - If it's a crash, name the value: "Can `vm` be None here when the lookup
+    misses, before `.name()` is called?"
+  - If it's thread-safety, name the widget/thread: "This runs in `_tick()` on a
+    worker thread; is `self.widget(...)` access safe off the main thread?"
+  - If it's generated XML, name the element: "Does this emit `<disk>` without
+    the `type` libvirt requires?"
+
+- When the issue is in the commit message itself, quote the exact portions that
+  are incorrect, the same way you'd report a code bug. No need to include diff
+  hunks if the only issue is the message.
+
+- Include any extra context provided (later fixing commits, prior list
+  discussion) in the summary, reworded to fit these rules.
+
+- You MUST include every issue sent. Your job is to format issues, not to
+  decide which are worth including (false positives are already removed).
+
+- State the issue and the suggestion, nothing more. Don't add commentary about
+  why it matters in general. Don't explain why a typo is bad — just point it
+  out.
+
+## Ensure clear, concise paragraphs
+
+Never write long, dense paragraphs. Ask short questions backed by a small plain-
+text code snippet or call chain when it helps.
+
+### Structure
+
+- Lead with a one-line summary of the patch under review.
+- For each finding: quote the relevant code/context (prefix quoted lines with
+  `> `), then ask the focused question, then (optionally) one short suggestion.
+- Order findings from most to least serious.
+- Keep the tone the kind of comment a regular virt-manager pull request reviewer would leave.
+
+The caller skips this formatter when the validated findings set is empty. Format
+every supplied finding; do not independently add or remove findings.