Conversation
… for playwright and agent browser
…and claue code skills
📝 WalkthroughWalkthroughThis PR introduces fairness-based pacing controls for multi-agent coordination, expands MCP/skills support across SDK and Codex backends with skill discovery and synchronization, adds an analysis mode and persona-aware coordination to the UI, and extends Docker deployment with overlay images and agent-browser integration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
massgen/frontend/displays/textual_widgets/setup_wizard.py (1)
1088-1133:⚠️ Potential issue | 🟡 MinorAdd Google-style docstring for
on_wizard_complete.This method now produces a structured result payload (skills + Docker artifacts). Please update its docstring to Google style so the API docs stay accurate.
Proposed docstring update
- """Save the API keys to the selected location.""" + """Save API keys and finalize setup. + + Returns: + Dict[str, Any]: Result payload containing setup status and artifacts. + """As per coding guidelines:
**/*.py: For new or changed functions, include Google-style docstrings.massgen/config_builder.py (1)
5072-5161:⚠️ Potential issue | 🟡 MinorSurface new fairness_ parameters in the interactive wizard/validation.*
You now inject
fairness_enabled,fairness_lead_cap_answers, andmax_midstream_injections_per_roundinto quickstart configs, but the full interactive wizard path doesn’t expose or document them. Please add prompts/defaults and validation/help text so non‑quickstart configs remain consistent.Based on learnings: Update
massgen/config_builder.pyto add new YAML parameters to interactive wizard, parameter validation, and help text when new configuration options are added.massgen/frontend/displays/textual_widgets/mode_bar.py (1)
375-478:⚠️ Potential issue | 🟡 MinorAdd Google-style docstrings for new persona helpers.
set_parallel_personas_enabled,_update_coordination_aux_controls, andget_parallel_personas_enabledneed Args/Returns sections.Proposed docstring updates
def set_parallel_personas_enabled(self, enabled: bool) -> None: - """Set the parallel persona toggle state.""" + """Set the parallel persona toggle state. + + Args: + enabled: True to enable personas, False to disable. + """ if self._persona_toggle: self._persona_toggle.set_state("on" if enabled else "off") def _update_coordination_aux_controls(self, mode: str) -> None: - """Update controls that depend on coordination mode.""" + """Update controls that depend on coordination mode. + + Args: + mode: The current coordination mode ("parallel" or "decomposition"). + """ if not self._subtasks_btn: pass elif mode == "decomposition": self._subtasks_btn.remove_class("hidden") else: self._subtasks_btn.add_class("hidden") def get_parallel_personas_enabled(self) -> bool: - """Get current parallel persona toggle state.""" + """Get current parallel persona toggle state. + + Returns: + True if parallel personas are enabled. + """ return self._persona_toggle.get_state() == "on" if self._persona_toggle else FalseAs per coding guidelines:
**/*.py: For new or changed functions, include Google-style docstrings.massgen/agent_config.py (1)
226-240:⚠️ Potential issue | 🟡 MinorDocument
voting_thresholdinAgentConfigargs.
voting_thresholdis now a public field but it isn’t listed in the Args section. Please add it so API docs stay complete.💡 Suggested docstring update
@@ - voting_sensitivity: Controls how critical agents are when voting ("lenient", "balanced", "strict") + voting_sensitivity: Controls how critical agents are when voting ("lenient", "balanced", "strict") + voting_threshold: Optional numeric threshold for ROI-style voting (e.g., 15 = 15% improvement required)As per coding guidelines:
**/*.py: Use Google-style docstrings for all Python modules and functions to enable auto-generation of API documentation.massgen/cli.py (1)
5369-5473:⚠️ Potential issue | 🟡 MinorAdd Google‑style docstrings for the new nested helpers.
_merge_readonly_context_pathand_agents_have_context_path(Lines 5371 and 5407) should include Google‑style Args/Returns sections.Proposed fix
def _merge_readonly_context_path(config_dict: Dict[str, Any], path_str: str, description: str) -> bool: - """Add a read-only orchestrator context path if missing.""" + """Add a read-only orchestrator context path if missing. + + Args: + config_dict: Config dict to update in-place. + path_str: Path to add as a context path. + description: Human-readable description for the path. + + Returns: + True if the path was added; otherwise False. + """ if not path_str: return False @@ def _agents_have_context_path(current_agents: Optional[Dict[str, Any]], path_str: str) -> bool: - """Check whether all active agents already have a specific context path.""" + """Check whether all active agents already have a specific context path. + + Args: + current_agents: Active agents (or None). + path_str: Path to verify. + + Returns: + True if all agents already have the path; otherwise False. + """ if not current_agents: return FalseAs per coding guidelines:
**/*.py: Use Google-style docstrings for all Python modules and functions to enable auto-generation of API documentation
🤖 Fix all issues with AI agents
In `@docs/source/quickstart/installation.rst`:
- Around line 187-192: Add a minimal runnable example and expected output for
the new Skills step in the quickstart note: show the exact command "uv run
massgen --quickstart" (and an example using "--setup-skills" for
retry/pre-install), include a short terminal transcript demonstrating the Skills
step prompts and on-page status updates (e.g., selection prompt, "Installing
Anthropic...", "Installed: Anthropic", success summary), and append a brief
bullet in docs/source/index.rst under "Recent Releases" announcing the
Skills-step feature and linking to the quickstart page; update the note text in
installation.rst (the uv run massgen --quickstart block) to include this
runnable snippet and the expected output example.
In `@massgen/backend/claude_code.py`:
- Around line 710-716: The same-path guard currently swallows OSError and
proceeds to copy, risking self-overwrite; in the try/except around
source.resolve() == dest.resolve() update the except OSError branch to log the
caught exception (including which paths failed) and return early instead of
continuing, so the subsequent copytree/copy2 logic is not executed when
resolve() fails for either source or dest; reference the existing
source.resolve(), dest.resolve(), and the copytree/copy2 call sites when making
this change.
- Around line 564-570: The current loop that calls task.uncancel() over
asyncio.all_tasks() is too broad; instead, capture the specific coordination
task (e.g., the current_task via asyncio.current_task() or the known
coordination Task object used by the coordination loop) before invoking
disconnect(), and after disconnect only check/uncancel that single task (check
its cancelling() > 0 then call uncancel()) rather than iterating all tasks;
apply this same targeted-uncancel pattern in the disconnect() code path
referenced (the block around the disconnect() implementation where all_tasks()
is used).
- Around line 749-751: The claude_code backend now advertises
get_supported_builtin_tools() returning ["Skill"], but the capabilities registry
still lists "Task"; update the claude_code entry in
massgen/backend/capabilities.py to change its builtin_tools list to include
"Skill" (replace "Task" with "Skill" or add "Skill" if preserving others) so the
registry matches the get_supported_builtin_tools implementation and the
backend's supported tools.
In `@massgen/backend/codex.py`:
- Around line 649-659: The cleanup currently unconditionally removes a
workspace-root AGENTS.md (agents_md) which can delete user files; update the
removal logic so you only unlink the workspace-root AGENTS.md if it contains a
MassGen generation marker (or other provenance check) — e.g., open
Path(self.cwd)/"AGENTS.md" and scan for a specific marker string or timestamp
signature you add when generating files; if the marker is present, unlink
agents_md, otherwise leave it and optionally log/emit a warning. Ensure this
check is done where agents_md is defined and keep the existing config_dir
cleanup unchanged.
In `@massgen/cli.py`:
- Around line 115-139: Update the docstring for _quickstart_config_uses_skills
to use Google-style format: include an "Args" section documenting the
config_path: Optional[str] parameter and its meaning, and a "Returns" section
describing the boolean result (True when the config enables coordination skills,
False otherwise). Keep the existing one-line summary, add the Args and Returns
blocks, and ensure typing and brief descriptions match the function behavior.
- Around line 141-159: Update the _ensure_quickstart_skills_ready function
docstring to Google style: add an "Args:" section describing config_path
(Optional[str]) and install_requested (bool, default True) and a "Returns:"
section stating it returns bool indicating whether skill installation succeeded
or was skipped; keep the short summary line ("Install quickstart skill packages
when generated config enables skills.") and ensure types and default behavior
are documented for clarity in tooling and code reviews.
- Around line 5547-5587: The cleanup call for agent backends is not best-effort:
wrap the await ag.backend.__aexit__(None, None, None) in a try/except to catch
any Exception and log it (similar to the existing filesystem_manager.cleanup()
handling) so a failing backend __aexit__ doesn’t abort the whole recreate flow;
keep using logger.warning (include agent id aid and the exception) and continue
the loop so agents are still recreated and context.agents is replaced with the
new_agents.
In `@massgen/docker/build_overlay_from_official.sh`:
- Around line 95-99: The script calls docker build using DOCKERFILE and assumes
the current working directory is the repo root; add a preflight check to verify
the Dockerfile exists (test -f "${DOCKERFILE}") and fail with a clear error if
not found, or resolve DOCKERFILE relative to the script directory (using the
script's dirname) before invoking docker build so that the build context and
Dockerfile path are valid; update the block that uses DOCKERFILE, BASE_IMAGE,
IMAGE_NAME, and TAG to perform this validation and exit early on failure.
- Around line 20-66: The option handlers in the while/case loop (for -t|--tag,
-n|--name, --base-image, --base-image-sudo) currently assign "$2" blindly which
allows missing-argument cases to set empty TAG, IMAGE_NAME, BASE_IMAGE_STANDARD
or BASE_IMAGE_SUDO; update each handler in the while/case block to validate that
a non-empty next argument exists and does not begin with '-' before assigning,
and if the check fails print a clear error (or usage via the existing help text)
and exit nonzero so missing values are caught immediately.
In `@massgen/filesystem_manager/_docker_manager.py`:
- Around line 604-606: The code unconditionally overwrites
PLAYWRIGHT_BROWSERS_PATH in env_vars; change the assignment in
_docker_manager.py so you only set a default when the user hasn't provided one
(e.g., use env_vars.setdefault("PLAYWRIGHT_BROWSERS_PATH",
"/home/massgen/.cache/ms-playwright")) to avoid clobbering values loaded by
_build_environment() including hermetic values like "0".
In `@massgen/filesystem_manager/_filesystem_manager.py`:
- Around line 433-438: The log call in FilesystemManager uses "{}" placeholders
which Python's logging won't interpolate; replace the logger.info invocation so
it formats the message (preferably using an f-string to match the rest of the
file) and include the actual lengths of suppressed_repo_paths, context_paths,
and extra_mount_paths (refer to the logger.info call and the variables
suppressed_repo_paths, context_paths, extra_mount_paths) so the message outputs
the real counts.
- Around line 2056-2060: The try/except around
self.agent_temporary_workspace_parent.mkdir(...) currently swallows errors;
change it to catch Exception as e and log a warning including the exception
details so failures to recreate the directory are visible. Use the class logger
(self.logger.warn/ warning) if available, otherwise get a module logger
(logging.getLogger(__name__)) and log a message like "Failed to recreate
agent_temporary_workspace_parent" with the exception info; keep the existing
behavior of not raising after logging. Ensure you reference
self.agent_temporary_workspace_parent.mkdir and the class logger (self.logger)
in the change.
In `@massgen/frontend/displays/textual_terminal_display.py`:
- Around line 4620-4775: The docstrings for the new persona helper methods
(_get_persona_runtime_subagent, _open_persona_runtime_subagent_screen,
_persona_subagent_events_ready, _auto_open_persona_runtime_subagent_screen) are
missing Google-style "Args:" and "Returns:" sections; update each function's
docstring to include an "Args:" entry describing parameters (e.g., subagent_id:
str, auto_return_on_completion: bool, attempt: int) and a "Returns:" entry
describing the return type/meaning (e.g., Optional[Any], bool, None) following
Google-style formatting so the docstrings comply with the project's Python
docstring guideline.
- Around line 9062-9204: Add Google-style docstrings with Args: and Returns: to
the listed helper methods (_normalize_skill_name_list, _collect_skill_inventory,
_slugify_skill_name, _extract_skill_candidate_from_answer,
_offer_skill_creation_from_analysis). For each function include a one-line
summary, an Args: section documenting parameter names and types (e.g.,
skill_names: List[str], answer_text: str), and a Returns: section describing the
return type and meaning (e.g., List[str], Tuple[List[Dict[str, Any]],
List[Dict[str, Any]]], Optional[Tuple[str,str]], or None). Ensure the docstrings
match existing project style (Google style), are placed immediately under each
def, and keep descriptions concise for the private methods named above.
- Around line 6878-7181: Add Google-style docstrings (including Args: and
Returns: blocks) for each new/changed helper method so they meet the project's
docstring standard; update the docstrings for _get_available_log_sessions,
_get_turn_numbers, _ensure_analysis_defaults, _build_analysis_log_options,
_build_analysis_turn_options, _get_analysis_report_path,
_extract_user_query_fragment, _condense_preview, _read_yaml_file,
_get_latest_attempt_dir, _extract_query_preview_from_attempt,
_extract_winner_from_status, _extract_final_answer_preview_from_attempt, and
_build_analysis_preview_text to describe parameters and return types (and
briefly note side effects where applicable) using the Google-style Args/Returns
format.
- Around line 9195-9232: In _offer_skill_creation_from_analysis: prevent silent
overwrites by checking if target_path.exists() before writing; if SKILL.md
already exists, either (a) change the flow to present a confirmation dialog that
explicitly warns about overwriting (reuse or extend SkillConfirmModal to include
an "overwrite" prompt) or (b) abort and notify the user that a skill with that
slug already exists and suggest a different name; implement this check inside
the _on_confirm handler (or before pushing the modal) and only perform
target_dir.mkdir(...) and target_path.write_text(...) after explicit overwrite
confirmation, ensuring notify(...) is used to report the outcome or
cancellation.
In `@massgen/frontend/displays/textual_widgets/plan_options.py`:
- Around line 84-112: Add Google-style docstrings with an "Args:" section to the
__init__ methods of AnalysisProfileChanged, AnalysisTargetChanged, and
ViewAnalysisRequested so they match the project's docs style (like
PlanSelected/PlanDepthChanged); describe each parameter and its type (e.g.,
profile: str; log_dir: Optional[str]; turn: Optional[int] or int) and keep the
brief one-line summary above the Args section, leaving the existing
super().__init__ calls intact.
In `@massgen/frontend/displays/textual_widgets/quickstart_wizard.py`:
- Around line 990-999: The on_checkbox_changed handler must reset the install
state when selections change: inside QuickstartWizard.on_checkbox_changed (which
iterates self._checkboxes and updates self._selected_packages) set
self._install_attempted = False whenever a package is added or removed so
validate() cannot bypass re-install; ensure the flag is cleared before breaking
out of the loop so any change (event.value true or false) resets the install
attempt state.
- Around line 929-1152: Add Google-style docstrings to all newly added/changed
methods in SkillsInstallStep: compose, on_checkbox_changed,
_run_installer_quiet, on_button_pressed, validate, get_value, and set_value. For
each method include a short summary line, Args with parameter types and
descriptions (e.g., event: Checkbox.Changed for on_checkbox_changed, event:
Button.Pressed for on_button_pressed, installer for _run_installer_quiet),
Returns with the return type and meaning (or None), and Raises if applicable;
keep formatting consistent with other project docstrings and place the docstring
immediately below each def declaration (triple-quoted Google-style).
🧹 Nitpick comments (27)
massgen/frontend/displays/textual/widgets/modals/shortcuts_modal.py (1)
20-20: Consider adding Google-style docstrings tocomposeandon_button_pressed.Per project guidelines, functions should have Google-style docstrings. These methods currently lack them. As per coding guidelines, "Use Google-style docstrings for all Python modules and functions to enable auto-generation of API documentation."
Also applies to: 83-83
massgen/frontend/displays/task_plan_support.py (2)
50-57: Consider narrowing the exception types in the parse fallback chain.Ruff flags the bare
Exceptioncatches (BLE001). Sincejson.loadsraisesjson.JSONDecodeError(aValueErrorsubclass) andast.literal_evalraisesValueErrororSyntaxError, you can tighten these without changing behavior.♻️ Suggested narrower exceptions
try: parsed = json.loads(text) - except Exception: + except (ValueError, TypeError): try: parsed = ast.literal_eval(text) - except Exception: + except (ValueError, SyntaxError): return None
134-141: Redundantisinstancecheck —_extract_structured_resultalready guarantees aDictorNone.
_extract_structured_resultreturnsOptional[Dict[str, Any]]. After theif not result_dataguard at line 135,result_datais always adict, making the check on line 140 dead code.♻️ Remove dead branch
result_data = _extract_structured_result(result) if not result_data: result_len = len(result) if isinstance(result, str) else "n/a" _log(f"_task_plan: parse error (result length={result_len})") return True - if not isinstance(result_data, dict): - return True - tasks: List[Dict[str, Any]] = []massgen/docker/Dockerfile.overlay (1)
8-25: Pin the overlay base image and npm tools to avoid drift.Using
latestplus unpinned npm installs makes the overlay non-reproducible; consider pinning the base image tag/digest and reusing the same tool versions as the runtime Dockerfile.📌 Example pinning pattern
-ARG BASE_IMAGE=ghcr.io/massgen/mcp-runtime:latest +ARG BASE_IMAGE=ghcr.io/massgen/mcp-runtime:<version-or-digest> ... -RUN npm install -g agent-browser && \ - (command -v openskills >/dev/null 2>&1 || npm install -g openskills) +ARG AGENT_BROWSER_VERSION=... +ARG OPENSKILLS_VERSION=... +RUN npm install -g agent-browser@${AGENT_BROWSER_VERSION} && \ + (command -v openskills >/dev/null 2>&1 || npm install -g openskills@${OPENSKILLS_VERSION})What is the latest stable tag (or digest) for the MassGen MCP runtime image, and the current versions for agent-browser and openskills?massgen/docker/Dockerfile (1)
67-67: Pin global npm tool versions for reproducible image builds.Unpinned global installs can change behavior between builds. Pin to specific versions like
[email protected],@openai/[email protected], and[email protected](or use build args for flexibility):📌 Example pinning pattern
-RUN npm install -g openskills `@openai/codex` agent-browser +RUN npm install -g \ + [email protected] \ + `@openai/codex`@0.28.0 \ + [email protected]massgen/frontend/interactive_controller.py (2)
508-514: Missingreturnafter analysis-mode skill creation may cause unintended notifications.The plan-mode block at line 507 returns early to prevent fallthrough into the
was_cancelled/errornotification logic below. This analysis-mode block doesn'treturn, so execution continues to lines 516–519. Currently this is safe because the guard ensureswas_cancelled=Falseanderror=None, but it's inconsistent and fragile if downstream logic changes.Proposed fix
if mode_state.plan_mode == "analysis" and result.answer_text and not result.was_cancelled and not result.error: analysis_profile = getattr(mode_state.analysis_config, "profile", "dev") if analysis_profile == "user": self._display._call_app_method( "_offer_skill_creation_from_analysis", result.answer_text, ) + return
1245-1251: Consider accessing_displayvia the existing adapter pattern rather thangetattr.The
_displayattribute is always set inTextualInteractiveAdapter.__init__, so usinggetattrwith no default is equivalent to direct access. This works, but other handlers (e.g.,show_helpat line 633) call the adapter method directly rather than reaching into the adapter's internals.For consistency with other non-modal UI actions in this method, consider adding a
show_skillsmethod onTextualInteractiveAdapter(likeshow_help,show_vote, etc.) and calling it from here.massgen/backend/codex.py (2)
683-710: Skills source resolution looks correct but consider adding Google-style docstrings.The priority chain (Docker temp → local manager → project
.agent/skills→ home.agent/skills) is sensible. Per coding guidelines, new functions should include Google-style docstrings withArgsandReturnssections.Suggested docstring
def _resolve_codex_skills_source(self) -> Optional[Path]: - """Resolve the best available skills source directory, if any.""" + """Resolve the best available skills source directory, if any. + + Checks multiple locations in priority order: Docker temp skills dirs, + local skills directory from filesystem manager, project-level + ``.agent/skills``, and user-level ``~/.agent/skills``. + + Returns: + Path to the skills source directory, or None if no source found. + """As per coding guidelines,
**/*.py: Use Google-style docstrings for all Python modules and functions.
728-743: A single file-copy failure aborts the entire sync.The
try/except OSErrorwraps the entirefor entry in source.iterdir()loop, so one bad entry (e.g., a broken symlink or permission error on a single file) halts all remaining copies. If best-effort sync is the intent, move the error handling inside the loop.Proposed fix
copied_entries = 0 - try: - for entry in source.iterdir(): + for entry in source.iterdir(): + try: target = dest / entry.name if entry.is_dir(): shutil.copytree(entry, target, dirs_exist_ok=True) copied_entries += 1 elif entry.is_file(): shutil.copy2(entry, target) copied_entries += 1 - except OSError as e: - logger.warning(f"Codex skills sync failed from {source} to {dest}: {e}") - return + except OSError as e: + logger.warning(f"Codex skills sync: failed to copy {entry.name}: {e}") if copied_entries: logger.info(f"Codex skills sync: copied {copied_entries} entries from {source} to {dest}")massgen/backend/claude_code.py (3)
673-699:_resolve_skill_source— docstring is minimal; consider documenting the resolution order.The method silently walks four distinct locations (SDK-dict temp paths → local skills dir → project
.agent/skills→ home.agent/skills). A brief Google-style docstring listing the priority order would help future maintainers. As per coding guidelines, Google-style docstrings are required for new functions in**/*.py.
53-53: Redundantimport shutil— already imported at module top level (line 53).Line 155 does
import shutillocally inside__init__, butshutilis now also imported at line 53. The local import is dead code.Also applies to: 155-155
1894-1918: MCP-style workflow tool capture looks correct; minor: import inside hot loop.The
from ..tool.workflow_toolkits.base import WORKFLOW_TOOL_NAMESimport is executed on everyToolUseBlockiteration. Since Python caches module imports this is functionally fine, but moving it to module level (or at least outside thefor block in message.contentloop) would be cleaner.massgen/docker/Dockerfile.overlay.sudo (2)
13-14: Pin npm package versions for reproducible builds.
agent-browserandopenskillsare installed without version pins, meaning builds on different days may produce different images. Consider pinning to specific versions (e.g.,npm install -g [email protected]).
28-28:tail -f /dev/nullkeeps the container alive as a workspace — consider adding a HEALTHCHECK.Without a HEALTHCHECK, orchestrators (Docker Compose, Kubernetes) cannot distinguish a healthy idle container from a hung one. A simple
HEALTHCHECK CMD truewould suffice if the container is expected to be a long-running workspace.massgen/agent_config.py (1)
249-257: Consider validating the new numeric fairness/voting fields.Adding sanity checks (non-negative/positive) for
voting_threshold,fairness_lead_cap_answers, andmax_midstream_injections_per_roundprevents invalid configs from propagating.♻️ Suggested validation
@@ class AgentConfig: @@ max_midstream_injections_per_round: int = 2 + + def __post_init__(self) -> None: + self._validate_fairness_config() + + def _validate_fairness_config(self) -> None: + if self.voting_threshold is not None and self.voting_threshold <= 0: + raise ValueError("voting_threshold must be positive") + if self.fairness_lead_cap_answers < 0: + raise ValueError("fairness_lead_cap_answers must be >= 0") + if self.max_midstream_injections_per_round < 0: + raise ValueError("max_midstream_injections_per_round must be >= 0")massgen/frontend/web/server.py (2)
900-908: Synchronouscheck_skill_packages_installed()call in async endpoint.
check_skill_packages_installed()performs filesystem I/O (scanning skill directories, reading manifests). Calling it synchronously here blocks the event loop. This is consistent with the existing pattern in this file (e.g.,_scan_workspace_filesin other endpoints), but for a generally heavier operation involving multiple directory scans it could cause noticeable latency.Consider wrapping in
run_in_executor:♻️ Suggested improvement
- from massgen.utils.skills_installer import check_skill_packages_installed + from massgen.utils.skills_installer import check_skill_packages_installed + import asyncio + loop = asyncio.get_running_loop() + packages = await loop.run_in_executor(None, check_skill_packages_installed) return { "skills": skills, "builtin_count": len([s for s in skills if s["location"] == "builtin"]), "user_count": len([s for s in skills if s["location"] == "user"]), "project_count": len([s for s in skills if s["location"] == "project"]), - "packages": check_skill_packages_installed(), + "packages": packages, }
948-1007: Refactored skill installation logic looks good — one edge case to note.The
openskills_installerslookup table and the separatedcrawl4aipath cleanly extend the endpoint. The docstring accurately reflects the expanded accepted inputs.Minor note: if
package_idisNone(i.e., the"package"key is absent from the request), it falls through to line 1004 and returns"Unknown package: None". Consider adding an early validation:🛡️ Optional: validate package_id early
package_id = request_data.get("package") + if not package_id: + return JSONResponse( + {"error": "Missing required field: 'package'"}, + status_code=400, + )Additionally, the installer functions (
install_anthropic_skills,install_openai_skills, etc.) are synchronous and perform subprocess calls (e.g.,npm install -g openskills). Calling them directly from thisasyncendpoint blocks the event loop. Consider wrapping the installation inloop.run_in_executor(None, installer)— the same pattern already used elsewhere in this file forcreate_agents_from_config.massgen/frontend/displays/textual/widgets/modals/skills_modals.py (3)
90-109: BareExceptioncatches silently swallow errors — use the specific Textual exception.Ruff flags
S112andBLE001here.query_oneraisestextual.css.query.NoMatcheswhen the selector doesn't match. Catching that specifically is both safer and self-documenting. At minimum, log unexpected errors to avoid masking real bugs.♻️ Proposed fix
+ from textual.css.query import NoMatches + def _set_all_checkboxes(self, value: bool) -> None: """Set all skill checkboxes to the given value.""" for checkbox_id in self._checkbox_to_name: try: cb = self.query_one(f"#{checkbox_id}", Checkbox) cb.value = value - except Exception: + except NoMatches: continue def _collect_enabled_names(self) -> List[str]: """Collect selected skill names from checkbox state.""" selected: List[str] = [] for checkbox_id, name in self._checkbox_to_name.items(): try: cb = self.query_one(f"#{checkbox_id}", Checkbox) if cb.value: selected.append(name) - except Exception: + except NoMatches: continue return selected
111-125:on_button_pressedoverridesBaseModal.on_button_pressedwithout callingsuper().
BaseModal.on_button_pressedhandles buttons whose IDs start with"close"or equal"cancel_button". SinceSkillsModaldefines its ownon_button_pressed, the parent handler is never reached. This works today because no button IDs here match the parent's patterns, but it's fragile — if a future change adds a close button or renames an ID, the parent logic silently stops working.Consider calling
super()as a fallback:♻️ Suggested improvement
def on_button_pressed(self, event: Button.Pressed) -> None: if event.button.id == "enable_all_skills_btn": self._set_all_checkboxes(True) return if event.button.id == "disable_all_skills_btn": self._set_all_checkboxes(False) return if event.button.id == "apply_skills_btn": self.dismiss(self._collect_enabled_names()) event.stop() return if event.button.id == "skills_cancel_button": self.dismiss(None) event.stop() return + super().on_button_pressed(event)
20-34: Missing Google-style docstrings on__init__methods.Both
SkillsModal.__init__andSkillConfirmModal.__init__lack docstrings describing their parameters. As per coding guidelines, new or changed functions in**/*.pyshould include Google-style docstrings.📝 Example for SkillsModal
def __init__( self, *, default_skills: List[Dict[str, Any]], created_skills: List[Dict[str, Any]], enabled_skill_names: Optional[List[str]], ) -> None: + """Initialize the skills manager modal. + + Args: + default_skills: Built-in skills to display. + created_skills: User-created or project skills to display. + enabled_skill_names: Names of currently enabled skills, + or None to treat all as enabled. + """ super().__init__()As per coding guidelines,
**/*.py: "Use Google-style docstrings for all Python modules and functions to enable auto-generation of API documentation."Also applies to: 128-141
massgen/cli.py (1)
5865-5897: Prefer direct assignment oversetattrforenabled_skill_names.Using
setattrwith a constant attribute is non‑idiomatic here; direct assignment is clearer.Proposed fix
- setattr( - orchestrator_config.coordination_config, - "enabled_skill_names", - enabled_skill_names, - ) + orchestrator_config.coordination_config.enabled_skill_names = enabled_skill_namesmassgen/frontend/displays/textual_widgets/plan_options.py (4)
256-266: Incomplete__init__docstring — newanalysis_*parameters are undocumented.The six new constructor parameters (
analysis_profile,analysis_log_options,analysis_selected_log_dir,analysis_turn_options,analysis_selected_turn,analysis_preview_text) are missing from theArgs:section.Proposed addition
Args: plan_mode: Current mode ("normal", "plan", "execute", or "analysis"). available_plans: List of available plan sessions. current_plan_id: Currently selected plan ID. current_depth: Current plan depth setting. current_broadcast: Current broadcast setting. + analysis_profile: Analysis profile to use ("dev" or "user"). + analysis_log_options: List of (label, value) tuples for log session selection. + analysis_selected_log_dir: Currently selected log directory path. + analysis_turn_options: List of (label, value) tuples for turn selection. + analysis_selected_turn: Currently selected turn number. + analysis_preview_text: Preview text for the selected analysis target. id: Optional DOM ID. classes: Optional CSS classes.As per coding guidelines,
**/*.py: "Use Google-style docstrings for all Python modules and functions to enable auto-generation of API documentation."
370-382: Synchronous filesystem I/O insidecompose()may block the UI thread.Line 377 performs
Path(...).exists()synchronously during widget composition. While fast on local filesystems, this couples I/O to the render path. Consider precomputingreport_existswhen the analysis target changes (e.g., in a handler orshow()) and storing it as instance state, socompose()remains pure.
282-290: AddArgsandReturnssections to_safe_select_valuedocstring.This helper has a one-line summary but no
Args:/Returns:sections, which are needed for Google-style docstrings.Proposed fix
`@staticmethod` def _safe_select_value(options: List[tuple[str, str]], preferred: Optional[str]) -> Optional[str]: - """Return a safe select value from options, preferring the given value.""" + """Return a safe select value from options, preferring the given value. + + Args: + options: List of (label, value) tuples to select from. + preferred: Preferred value to select if available. + + Returns: + The preferred value if present in options, otherwise the first + option's value, or None if options is empty. + """ if not options: return NoneAs per coding guidelines,
**/*.py: "For new or changed functions, include Google-style docstrings."
2-10: Module docstring is outdated — missing analysis mode.The module-level docstring lists only plan/execute mode features. Now that the popover also serves as the analysis mode configuration surface, update the docstring to reflect this.
massgen/frontend/displays/textual_widgets/tab_bar.py (2)
124-132: AddArgssection toupdate_assignmentdocstring.
update_assignmentis a new public method but lacks anArgs:section. For consistency withupdate_infoandupdate_subtask, please add one.Proposed fix
def update_assignment(self, assignment: Optional[str], kind: str = "Subtask") -> None: - """Update the displayed agent assignment label.""" + """Update the displayed agent assignment label. + + Args: + assignment: The assignment text to display, or None to clear. + kind: Label for the assignment type (e.g., "Subtask", "Persona"). + """ self._assignment = assignment self._assignment_kind = kind self.refresh()As per coding guidelines,
**/*.py: "For new or changed functions, include Google-style docstrings."
466-479: AddArgssections toset_agent_personasand_set_agent_assignmentsfor consistency.
set_agent_subtasks(line 458) has a fullArgs:section, but the two new methods lack one. This inconsistency will affect auto-generated API docs.Proposed fix
def set_agent_personas(self, personas: Dict[str, str]) -> None: - """Set per-agent persona assignments for parallel mode.""" + """Set per-agent persona assignments for parallel mode. + + Args: + personas: Mapping of agent_id to persona summary/description. + """ self._set_agent_assignments(personas, kind="Persona") def _set_agent_assignments(self, assignments: Dict[str, str], kind: str) -> None: - """Set and render per-agent assignment labels.""" + """Set and render per-agent assignment labels. + + Args: + assignments: Mapping of agent_id to assignment text. + kind: Label for the assignment type (e.g., "Subtask", "Persona"). + """ self._agent_assignments = assignments self._assignment_kind = kindAs per coding guidelines,
**/*.py: "For new or changed functions, include Google-style docstrings."
| .. note:: | ||
|
|
||
| In ``uv run massgen --quickstart``, when Docker mode is selected the wizard includes | ||
| a Skills step where you can select package(s) and install them immediately with | ||
| on-page status updates (Anthropic/OpenAI/Vercel collections, Agent Browser skill, | ||
| and Crawl4AI). Use ``--setup-skills`` to retry or pre-install manually. |
There was a problem hiding this comment.
Add a runnable example + expected output for the new Skills step and surface it in Recent Releases.
This note introduces new quickstart behavior; please add a minimal command + sample output snippet for the Skills step, and update docs/source/index.rst “Recent Releases” if this is a new feature.
✏️ Suggested RST snippet
.. note::
In ``uv run massgen --quickstart``, when Docker mode is selected the wizard includes
a Skills step where you can select package(s) and install them immediately with
on-page status updates (Anthropic/OpenAI/Vercel collections, Agent Browser skill,
and Crawl4AI). Use ``--setup-skills`` to retry or pre-install manually.
+
+ Example:
+
+ .. code-block:: bash
+
+ uv run massgen --quickstart
+
+ .. code-block:: text
+
+ [Skills] Installing vercel-labs/agent-browser ... doneBased on learnings and coding guidelines: Documentation for new features must include runnable commands with expected output, and docs/source/index.rst "Recent Releases" should be updated.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .. note:: | |
| In ``uv run massgen --quickstart``, when Docker mode is selected the wizard includes | |
| a Skills step where you can select package(s) and install them immediately with | |
| on-page status updates (Anthropic/OpenAI/Vercel collections, Agent Browser skill, | |
| and Crawl4AI). Use ``--setup-skills`` to retry or pre-install manually. | |
| .. note:: | |
| In ``uv run massgen --quickstart``, when Docker mode is selected the wizard includes | |
| a Skills step where you can select package(s) and install them immediately with | |
| on-page status updates (Anthropic/OpenAI/Vercel collections, Agent Browser skill, | |
| and Crawl4AI). Use ``--setup-skills`` to retry or pre-install manually. | |
| Example: | |
| .. code-block:: bash | |
| uv run massgen --quickstart | |
| .. code-block:: text | |
| [Skills] Installing vercel-labs/agent-browser ... done |
🤖 Prompt for AI Agents
In `@docs/source/quickstart/installation.rst` around lines 187 - 192, Add a
minimal runnable example and expected output for the new Skills step in the
quickstart note: show the exact command "uv run massgen --quickstart" (and an
example using "--setup-skills" for retry/pre-install), include a short terminal
transcript demonstrating the Skills step prompts and on-page status updates
(e.g., selection prompt, "Installing Anthropic...", "Installed: Anthropic",
success summary), and append a brief bullet in docs/source/index.rst under
"Recent Releases" announcing the Skills-step feature and linking to the
quickstart page; update the note text in installation.rst (the uv run massgen
--quickstart block) to include this runnable snippet and the expected output
example.
|
|
||
| # anyio's cancel scope propagation may have called task.cancel() on | ||
| # parent tasks during disconnect(). Reverse any pending cancellations | ||
| # to prevent killing the coordination loop. | ||
| for task in asyncio.all_tasks(): | ||
| if not task.done() and task.cancelling() > 0: | ||
| task.uncancel() |
There was a problem hiding this comment.
uncancel() on all event-loop tasks is overly broad and may mask legitimate cancellations.
asyncio.all_tasks() returns every task in the loop — including timeouts, background health checks, and other coordinated cancellations. Blindly uncancelling them all can silently swallow a CancelledError that was raised for a valid reason (e.g., an orchestrator timeout, a user-initiated Ctrl+C propagating through task groups).
A safer approach is to record the current task (or the known coordination task) before calling disconnect() and only uncancel that specific task.
Proposed fix
async def reset_state(self) -> None:
...
if self._client is not None:
+ current = asyncio.current_task()
try:
await self._client.disconnect()
except asyncio.CancelledError:
pass # anyio cancel scope may raise CancelledError
except Exception:
pass # Ignore cleanup errors
- # anyio's cancel scope propagation may have called task.cancel() on
- # parent tasks during disconnect(). Reverse any pending cancellations
- # to prevent killing the coordination loop.
- for task in asyncio.all_tasks():
- if not task.done() and task.cancelling() > 0:
- task.uncancel()
+ # Reverse cancellation only on the task that called disconnect(),
+ # which is the one anyio's cancel scope targeted.
+ if current and not current.done() and current.cancelling() > 0:
+ current.uncancel()Apply the same pattern in disconnect() (lines 2224-2227).
🤖 Prompt for AI Agents
In `@massgen/backend/claude_code.py` around lines 564 - 570, The current loop that
calls task.uncancel() over asyncio.all_tasks() is too broad; instead, capture
the specific coordination task (e.g., the current_task via
asyncio.current_task() or the known coordination Task object used by the
coordination loop) before invoking disconnect(), and after disconnect only
check/uncancel that single task (check its cancelling() > 0 then call
uncancel()) rather than iterating all tasks; apply this same targeted-uncancel
pattern in the disconnect() code path referenced (the block around the
disconnect() implementation where all_tasks() is used).
|
|
||
| try: | ||
| if source.resolve() == dest.resolve(): | ||
| return | ||
| except OSError: | ||
| # Continue best-effort copy if either path cannot be resolved. | ||
| pass |
There was a problem hiding this comment.
Same-path check swallows OSError and continues with the copy — could lead to self-overwrite.
If source.resolve() or dest.resolve() raises OSError, the guard is skipped and the loop proceeds to copy source into dest. When source and dest are actually the same directory on a filesystem where resolve() fails (e.g., broken symlink chain), the copytree / copy2 calls may corrupt the files.
Consider logging the exception and returning early instead of continuing best-effort:
Proposed fix
try:
if source.resolve() == dest.resolve():
return
except OSError:
- # Continue best-effort copy if either path cannot be resolved.
- pass
+ logger.warning(
+ f"Cannot resolve skill paths ({source}, {dest}); skipping sync to avoid self-overwrite"
+ )
+ return🤖 Prompt for AI Agents
In `@massgen/backend/claude_code.py` around lines 710 - 716, The same-path guard
currently swallows OSError and proceeds to copy, risking self-overwrite; in the
try/except around source.resolve() == dest.resolve() update the except OSError
branch to log the caught exception (including which paths failed) and return
early instead of continuing, so the subsequent copytree/copy2 logic is not
executed when resolve() fails for either source or dest; reference the existing
source.resolve(), dest.resolve(), and the copytree/copy2 call sites when making
this change.
| tools = [ | ||
| "Task", # Subagent spawning - unique to Claude Code | ||
| "Skill", | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if capabilities.py reflects the Skill builtin tool for claude_code
rg -n -i 'skill' massgen/backend/capabilities.pyRepository: massgen/MassGen
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check the structure of capabilities.py to see how tools are registered
head -100 massgen/backend/capabilities.pyRepository: massgen/MassGen
Length of output: 3934
🏁 Script executed:
#!/bin/bash
# Look at the claude_code.py change to understand the context
sed -n '740,760p' massgen/backend/claude_code.pyRepository: massgen/MassGen
Length of output: 779
🏁 Script executed:
#!/bin/bash
# Find the claude_code entry in capabilities.py
rg -n 'claude_code' massgen/backend/capabilities.py -A 15Repository: massgen/MassGen
Length of output: 633
🏁 Script executed:
#!/bin/bash
# Get all builtin_tools mentions in capabilities.py to understand the pattern
rg -n 'builtin_tools' massgen/backend/capabilities.py -B 2 -A 2Repository: massgen/MassGen
Length of output: 4911
🏁 Script executed:
#!/bin/bash
# Get the complete builtin_tools list for claude_code in capabilities.py
sed -n '220,235p' massgen/backend/capabilities.pyRepository: massgen/MassGen
Length of output: 407
Add "Skill" to capabilities.py builtin_tools for claude_code backend.
The get_supported_builtin_tools method in claude_code.py now returns ["Skill"], but the capabilities registry still lists "Task" instead. Update the claude_code entry in massgen/backend/capabilities.py (line 220-235) to replace or update the builtin_tools list to reflect this change.
🤖 Prompt for AI Agents
In `@massgen/backend/claude_code.py` around lines 749 - 751, The claude_code
backend now advertises get_supported_builtin_tools() returning ["Skill"], but
the capabilities registry still lists "Task"; update the claude_code entry in
massgen/backend/capabilities.py to change its builtin_tools list to include
"Skill" (replace "Task" with "Skill" or add "Skill" if preserving others) so the
registry matches the get_supported_builtin_tools implementation and the
backend's supported tools.
| for filename in ("config.toml", "custom_tool_specs.json", "workflow_tool_specs.json", "checklist_specs.json", "AGENTS.md"): | ||
| filepath = config_dir / filename | ||
| if filepath.exists(): | ||
| filepath.unlink() | ||
| # Remove dir if empty | ||
| if config_dir.exists() and not any(config_dir.iterdir()): | ||
| config_dir.rmdir() | ||
| # Also remove AGENTS.md we wrote in workspace root | ||
| # Cleanup legacy workspace-root AGENTS.md (older backend behavior). | ||
| agents_md = Path(self.cwd) / "AGENTS.md" | ||
| if agents_md.exists(): | ||
| agents_md.unlink() |
There was a problem hiding this comment.
Cleanup correctly handles both new and legacy file locations.
Adding checklist_specs.json and AGENTS.md to the cleanup list is correct. The legacy AGENTS.md cleanup at the workspace root (lines 656–659) is a nice migration touch, though it will silently delete any user-created AGENTS.md in the workspace root.
Consider checking whether the workspace-root AGENTS.md was created by MassGen (e.g., via a marker comment or timestamp check) before removing it, to avoid deleting user-created files. Alternatively, document this cleanup behavior.
🤖 Prompt for AI Agents
In `@massgen/backend/codex.py` around lines 649 - 659, The cleanup currently
unconditionally removes a workspace-root AGENTS.md (agents_md) which can delete
user files; update the removal logic so you only unlink the workspace-root
AGENTS.md if it contains a MassGen generation marker (or other provenance check)
— e.g., open Path(self.cwd)/"AGENTS.md" and scan for a specific marker string or
timestamp signature you add when generating files; if the marker is present,
unlink agents_md, otherwise leave it and optionally log/emit a warning. Ensure
this check is done where agents_md is defined and keep the existing config_dir
cleanup unchanged.
| @staticmethod | ||
| def _normalize_skill_name_list(skill_names: List[str]) -> List[str]: | ||
| """Normalize and deduplicate skill names while preserving order.""" | ||
| seen: Set[str] = set() | ||
| normalized: List[str] = [] | ||
| for name in skill_names: | ||
| cleaned = (name or "").strip() | ||
| if not cleaned: | ||
| continue | ||
| key = cleaned.lower() | ||
| if key in seen: | ||
| continue | ||
| seen.add(key) | ||
| normalized.append(cleaned) | ||
| return normalized | ||
|
|
||
| def _collect_skill_inventory(self) -> Tuple[List[Dict[str, Any]], List[Dict[str, Any]]]: | ||
| """Collect available skills grouped into default vs created lists.""" | ||
| from massgen.filesystem_manager.skills_manager import scan_skills | ||
| from massgen.logs_analyzer import get_logs_dir | ||
|
|
||
| skills_dir = Path(".agent/skills") | ||
| logs_dir = get_logs_dir() | ||
| all_skills = scan_skills(skills_dir, logs_dir=logs_dir if logs_dir.exists() else None) | ||
|
|
||
| # Keep first instance per name to avoid duplicates across sources. | ||
| deduped: List[Dict[str, Any]] = [] | ||
| seen: Set[str] = set() | ||
| for skill in all_skills: | ||
| name = str(skill.get("name", "")).strip() | ||
| if not name: | ||
| continue | ||
| key = name.lower() | ||
| if key in seen: | ||
| continue | ||
| seen.add(key) | ||
| deduped.append(skill) | ||
|
|
||
| default_skills = sorted( | ||
| [s for s in deduped if s.get("location") == "builtin"], | ||
| key=lambda s: str(s.get("name", "")).lower(), | ||
| ) | ||
| created_skills = sorted( | ||
| [s for s in deduped if s.get("location") != "builtin"], | ||
| key=lambda s: str(s.get("name", "")).lower(), | ||
| ) | ||
| return default_skills, created_skills | ||
|
|
||
| def _show_skills_modal(self) -> None: | ||
| """Show session skill manager modal.""" | ||
| default_skills, created_skills = self._collect_skill_inventory() | ||
| current_enabled = self._mode_state.analysis_config.get_enabled_skill_names() | ||
|
|
||
| modal = SkillsModal( | ||
| default_skills=default_skills, | ||
| created_skills=created_skills, | ||
| enabled_skill_names=current_enabled, | ||
| ) | ||
|
|
||
| def _on_skills_dismiss(result: Optional[List[str]]) -> None: | ||
| if result is None: | ||
| return | ||
|
|
||
| selected = self._normalize_skill_name_list(result) | ||
| discovered = self._normalize_skill_name_list( | ||
| [str(s.get("name", "")) for s in default_skills + created_skills], | ||
| ) | ||
|
|
||
| selected_set = {name.lower() for name in selected} | ||
| discovered_set = {name.lower() for name in discovered} | ||
|
|
||
| if discovered and selected_set == discovered_set: | ||
| # None = unfiltered; automatically includes newly discovered skills. | ||
| self._mode_state.analysis_config.enabled_skill_names = None | ||
| enabled_count = len(discovered) | ||
| self.notify(f"Skills enabled: all {enabled_count} discovered", severity="information", timeout=3) | ||
| else: | ||
| self._mode_state.analysis_config.enabled_skill_names = selected | ||
| self.notify(f"Skills enabled: {len(selected)}", severity="information", timeout=3) | ||
|
|
||
| self.push_screen(modal, _on_skills_dismiss) | ||
|
|
||
| @staticmethod | ||
| def _slugify_skill_name(name: str) -> str: | ||
| """Create a filesystem-safe skill directory name.""" | ||
| slug = re.sub(r"[^a-z0-9]+", "-", (name or "").lower()).strip("-") | ||
| return slug or "skill-from-analysis" | ||
|
|
||
| @staticmethod | ||
| def _extract_skill_candidate_from_answer(answer_text: str) -> Optional[Tuple[str, str]]: | ||
| """Extract a SKILL.md candidate from an answer if present. | ||
|
|
||
| Returns: | ||
| Tuple of (skill_name, skill_markdown) when detected, otherwise None. | ||
| """ | ||
| if not answer_text: | ||
| return None | ||
|
|
||
| def parse_skill_markdown(candidate: str) -> Optional[str]: | ||
| import yaml | ||
|
|
||
| cleaned = candidate.strip() | ||
| if not cleaned.startswith("---"): | ||
| return None | ||
|
|
||
| match = re.match(r"^---\s*\n(.*?)\n---\s*(?:\n|$)", cleaned, re.DOTALL) | ||
| if not match: | ||
| return None | ||
|
|
||
| try: | ||
| metadata = yaml.safe_load(match.group(1)) or {} | ||
| except Exception: | ||
| return None | ||
| if not isinstance(metadata, dict): | ||
| return None | ||
|
|
||
| skill_name = str(metadata.get("name", "")).strip() | ||
| return skill_name or None | ||
|
|
||
| # Prefer fenced blocks so explanatory prose outside the block is ignored. | ||
| for fence in re.finditer(r"```(?:[a-zA-Z0-9_-]+)?\n(.*?)```", answer_text, re.DOTALL): | ||
| block = fence.group(1).strip() | ||
| skill_name = parse_skill_markdown(block) | ||
| if skill_name: | ||
| return skill_name, block + "\n" | ||
|
|
||
| # Fallback: treat the entire answer as a candidate. | ||
| skill_name = parse_skill_markdown(answer_text) | ||
| if skill_name: | ||
| return skill_name, answer_text.strip() + "\n" | ||
|
|
||
| return None | ||
|
|
||
| def _offer_skill_creation_from_analysis(self, answer_text: str) -> None: | ||
| """Show a confirmation modal when a user-mode analysis returns SKILL.md content.""" | ||
| if self._mode_state.plan_mode != "analysis": | ||
| return | ||
| if self._mode_state.analysis_config.profile != "user": | ||
| return | ||
|
|
||
| candidate = self._extract_skill_candidate_from_answer(answer_text) | ||
| if not candidate: | ||
| return |
There was a problem hiding this comment.
Add Google-style docstrings for new skills helpers.
Functions like _normalize_skill_name_list, _collect_skill_inventory, _slugify_skill_name, _extract_skill_candidate_from_answer, and _offer_skill_creation_from_analysis need Args:/Returns: sections.
As per coding guidelines, "**/*.py: For new or changed functions, include Google-style docstrings".
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 9173-9173: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@massgen/frontend/displays/textual_terminal_display.py` around lines 9062 -
9204, Add Google-style docstrings with Args: and Returns: to the listed helper
methods (_normalize_skill_name_list, _collect_skill_inventory,
_slugify_skill_name, _extract_skill_candidate_from_answer,
_offer_skill_creation_from_analysis). For each function include a one-line
summary, an Args: section documenting parameter names and types (e.g.,
skill_names: List[str], answer_text: str), and a Returns: section describing the
return type and meaning (e.g., List[str], Tuple[List[Dict[str, Any]],
List[Dict[str, Any]]], Optional[Tuple[str,str]], or None). Ensure the docstrings
match existing project style (Google style), are placed immediately under each
def, and keep descriptions concise for the private methods named above.
| def _offer_skill_creation_from_analysis(self, answer_text: str) -> None: | ||
| """Show a confirmation modal when a user-mode analysis returns SKILL.md content.""" | ||
| if self._mode_state.plan_mode != "analysis": | ||
| return | ||
| if self._mode_state.analysis_config.profile != "user": | ||
| return | ||
|
|
||
| candidate = self._extract_skill_candidate_from_answer(answer_text) | ||
| if not candidate: | ||
| return | ||
|
|
||
| skill_name, skill_markdown = candidate | ||
| target_dir = Path(".agent") / "skills" / self._slugify_skill_name(skill_name) | ||
| target_path = target_dir / "SKILL.md" | ||
|
|
||
| modal = SkillConfirmModal( | ||
| skill_name=skill_name, | ||
| skill_markdown=skill_markdown, | ||
| target_path=str(target_path), | ||
| ) | ||
|
|
||
| def _on_confirm(result: Optional[bool]) -> None: | ||
| if not result: | ||
| return | ||
| try: | ||
| target_dir.mkdir(parents=True, exist_ok=True) | ||
| target_path.write_text(skill_markdown, encoding="utf-8") | ||
| self.notify(f"Created skill: {target_path}", severity="information", timeout=4) | ||
|
|
||
| # If a filter is active, include the newly created skill automatically. | ||
| enabled = self._mode_state.analysis_config.get_enabled_skill_names() | ||
| if enabled is not None: | ||
| self._mode_state.analysis_config.enabled_skill_names = self._normalize_skill_name_list(enabled + [skill_name]) | ||
| except Exception as e: | ||
| self.notify(f"Failed to create skill: {e}", severity="error", timeout=4) | ||
|
|
||
| self.push_screen(modal, _on_confirm) | ||
|
|
There was a problem hiding this comment.
Prevent accidental overwrite of existing skills.
The current write path always overwrites an existing SKILL.md with the same slug. Consider guarding or prompting before overwrite to avoid losing prior content.
🔧 Suggested guard
def _on_confirm(result: Optional[bool]) -> None:
if not result:
return
try:
+ if target_path.exists():
+ self.notify(f"Skill already exists: {target_path}", severity="warning", timeout=4)
+ return
target_dir.mkdir(parents=True, exist_ok=True)
target_path.write_text(skill_markdown, encoding="utf-8")
self.notify(f"Created skill: {target_path}", severity="information", timeout=4)🧰 Tools
🪛 Ruff (0.14.14)
[warning] 9227-9227: Consider [*enabled, skill_name] instead of concatenation
Replace with [*enabled, skill_name]
(RUF005)
[warning] 9228-9228: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@massgen/frontend/displays/textual_terminal_display.py` around lines 9195 -
9232, In _offer_skill_creation_from_analysis: prevent silent overwrites by
checking if target_path.exists() before writing; if SKILL.md already exists,
either (a) change the flow to present a confirmation dialog that explicitly
warns about overwriting (reuse or extend SkillConfirmModal to include an
"overwrite" prompt) or (b) abort and notify the user that a skill with that slug
already exists and suggest a different name; implement this check inside the
_on_confirm handler (or before pushing the modal) and only perform
target_dir.mkdir(...) and target_path.write_text(...) after explicit overwrite
confirmation, ensuring notify(...) is used to report the outcome or
cancellation.
| class AnalysisProfileChanged(Message): | ||
| """Message emitted when analysis profile is changed.""" | ||
|
|
||
| def __init__(self, profile: str) -> None: | ||
| self.profile = profile | ||
| super().__init__() | ||
|
|
||
|
|
||
| class AnalysisTargetChanged(Message): | ||
| """Message emitted when analysis log/turn target is changed.""" | ||
|
|
||
| def __init__(self, log_dir: Optional[str], turn: Optional[int]) -> None: | ||
| self.log_dir = log_dir | ||
| self.turn = turn | ||
| super().__init__() | ||
|
|
||
|
|
||
| class ViewAnalysisRequested(Message): | ||
| """Message emitted when user wants to view an analysis report.""" | ||
|
|
||
| def __init__(self, log_dir: str, turn: int) -> None: | ||
| self.log_dir = log_dir | ||
| self.turn = turn | ||
| super().__init__() | ||
|
|
||
|
|
||
| class OpenSkillsRequested(Message): | ||
| """Message emitted when user wants to open the skills manager.""" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing Google-style docstrings on new Message __init__ methods.
AnalysisProfileChanged.__init__, AnalysisTargetChanged.__init__, and ViewAnalysisRequested.__init__ lack Google-style docstrings with Args: sections, unlike the existing messages in this file (e.g., PlanSelected, PlanDepthChanged). Please add them for consistency and to satisfy the project's documentation guidelines.
Proposed fix
class AnalysisProfileChanged(Message):
"""Message emitted when analysis profile is changed."""
def __init__(self, profile: str) -> None:
+ """Initialize the message.
+
+ Args:
+ profile: The new analysis profile (e.g., "dev", "user").
+ """
self.profile = profile
super().__init__()
class AnalysisTargetChanged(Message):
"""Message emitted when analysis log/turn target is changed."""
def __init__(self, log_dir: Optional[str], turn: Optional[int]) -> None:
+ """Initialize the message.
+
+ Args:
+ log_dir: Path to the selected log directory, or None.
+ turn: The selected turn number, or None for latest.
+ """
self.log_dir = log_dir
self.turn = turn
super().__init__()
class ViewAnalysisRequested(Message):
"""Message emitted when user wants to view an analysis report."""
def __init__(self, log_dir: str, turn: int) -> None:
+ """Initialize the message.
+
+ Args:
+ log_dir: Path to the log directory to analyze.
+ turn: The turn number to view the report for.
+ """
self.log_dir = log_dir
self.turn = turn
super().__init__()As per coding guidelines, **/*.py: "Use Google-style docstrings for all Python modules and functions to enable auto-generation of API documentation."
🤖 Prompt for AI Agents
In `@massgen/frontend/displays/textual_widgets/plan_options.py` around lines 84 -
112, Add Google-style docstrings with an "Args:" section to the __init__ methods
of AnalysisProfileChanged, AnalysisTargetChanged, and ViewAnalysisRequested so
they match the project's docs style (like PlanSelected/PlanDepthChanged);
describe each parameter and its type (e.g., profile: str; log_dir:
Optional[str]; turn: Optional[int] or int) and keep the brief one-line summary
above the Args section, leaving the existing super().__init__ calls intact.
| def compose(self) -> ComposeResult: | ||
| self._load_packages_status() | ||
| self._checkboxes = {} | ||
| self._available_package_ids = [] | ||
|
|
||
| yield Label("Skill Packages", classes="text-input-label") | ||
| yield Label( | ||
| "Select which quickstart packages to install now, then press Install Selected Packages.", | ||
| classes="password-hint", | ||
| ) | ||
|
|
||
| if self._packages_status is None: | ||
| yield Label("Could not check skill package status.", classes="password-hint") | ||
| return | ||
|
|
||
| missing_packages: List[tuple[str, Dict[str, Any]]] = [] | ||
| for pkg_id, pkg in self._packages_status.items(): | ||
| installed = bool(pkg.get("installed")) | ||
| status_text = "installed" if installed else "not installed" | ||
| yield Label(f"{pkg.get('name', pkg_id)} [{status_text}]", classes="password-hint") | ||
| yield Label(f" {pkg.get('description', '')}", classes="password-hint") | ||
| if not installed: | ||
| missing_packages.append((pkg_id, pkg)) | ||
|
|
||
| if not missing_packages: | ||
| yield Label("All quickstart skill packages are already installed.", classes="password-hint") | ||
| self._install_attempted = True | ||
| return | ||
|
|
||
| self._available_package_ids = [pkg_id for pkg_id, _ in missing_packages] | ||
|
|
||
| if not self._selected_packages: | ||
| self._selected_packages = list(self._available_package_ids) | ||
| else: | ||
| self._selected_packages = [pkg_id for pkg_id in self._selected_packages if pkg_id in self._available_package_ids] | ||
|
|
||
| yield Label("Select packages to install:", classes="text-input-label") | ||
| for pkg_id, pkg in missing_packages: | ||
| cb = Checkbox( | ||
| f"{pkg.get('name', pkg_id)}", | ||
| value=pkg_id in self._selected_packages, | ||
| id=f"skills_pkg_{pkg_id}", | ||
| ) | ||
| cb.disabled = self._installing | ||
| self._checkboxes[pkg_id] = cb | ||
| yield cb | ||
|
|
||
| self._install_button = Button( | ||
| "Install Selected Packages", | ||
| id="install_selected_skill_packages", | ||
| variant="default", | ||
| ) | ||
| if self._installing: | ||
| self._install_button.disabled = True | ||
| self._install_button.label = "Installing..." | ||
| yield self._install_button | ||
|
|
||
| self._status_label = Label(self._status_message, classes="password-hint") | ||
| self._status_label.display = bool(self._status_message) | ||
| yield self._status_label | ||
|
|
||
| def on_checkbox_changed(self, event: Checkbox.Changed) -> None: | ||
| """Handle package selection toggles.""" | ||
| for pkg_id, cb in self._checkboxes.items(): | ||
| if cb.id != event.checkbox.id: | ||
| continue | ||
| if event.value and pkg_id not in self._selected_packages: | ||
| self._selected_packages.append(pkg_id) | ||
| elif not event.value and pkg_id in self._selected_packages: | ||
| self._selected_packages.remove(pkg_id) | ||
| break | ||
|
|
||
| @staticmethod | ||
| def _run_installer_quiet(installer) -> tuple[bool, str]: | ||
| """Run installer while capturing stdout/stderr to avoid TUI corruption.""" | ||
| output_buffer = io.StringIO() | ||
| error_buffer = io.StringIO() | ||
| with contextlib.redirect_stdout(output_buffer), contextlib.redirect_stderr(error_buffer): | ||
| ok = bool(installer()) | ||
| combined = "\n".join([output_buffer.getvalue(), error_buffer.getvalue()]).strip() | ||
| return ok, combined | ||
|
|
||
| async def on_button_pressed(self, event: Button.Pressed) -> None: | ||
| """Install selected skill packages with progress updates.""" | ||
| if event.button.id != "install_selected_skill_packages" or self._installing: | ||
| return | ||
|
|
||
| if not self._selected_packages: | ||
| self._install_attempted = True | ||
| self._status_message = "No packages selected. You can continue." | ||
| if self._status_label: | ||
| self._status_label.update(self._status_message) | ||
| self._status_label.display = True | ||
| return | ||
|
|
||
| self._installing = True | ||
| if self._install_button: | ||
| self._install_button.disabled = True | ||
| self._install_button.label = "Installing..." | ||
| for cb in self._checkboxes.values(): | ||
| cb.disabled = True | ||
|
|
||
| def _status(message: str) -> None: | ||
| self._status_message = message | ||
| if self._status_label: | ||
| self._status_label.update(message) | ||
| self._status_label.display = True | ||
|
|
||
| _status("Preparing skill package installation...") | ||
| self._installed_packages = [] | ||
| self._failed_packages = [] | ||
|
|
||
| try: | ||
| from massgen.utils.skills_installer import ( | ||
| install_agent_browser_skill, | ||
| install_anthropic_skills, | ||
| install_crawl4ai_skill, | ||
| install_openai_skills, | ||
| install_openskills_cli, | ||
| install_vercel_skills, | ||
| ) | ||
|
|
||
| openskills_installers = { | ||
| "anthropic": install_anthropic_skills, | ||
| "openai": install_openai_skills, | ||
| "vercel": install_vercel_skills, | ||
| "agent_browser": install_agent_browser_skill, | ||
| } | ||
|
|
||
| selected = list(self._selected_packages) | ||
| needs_openskills = any(pkg_id in openskills_installers for pkg_id in selected) | ||
| openskills_ready = True | ||
|
|
||
| if needs_openskills: | ||
| _status("Installing openskills CLI...") | ||
| openskills_ready, cli_logs = await asyncio.to_thread( | ||
| self._run_installer_quiet, | ||
| install_openskills_cli, | ||
| ) | ||
| if cli_logs: | ||
| _quickstart_log(f"SkillsInstallStep: openskills logs:\n{cli_logs}") | ||
| if not openskills_ready: | ||
| self._failed_packages.extend([pkg_id for pkg_id in selected if pkg_id in openskills_installers]) | ||
|
|
||
| for pkg_id in selected: | ||
| if pkg_id in openskills_installers: | ||
| if not openskills_ready: | ||
| continue | ||
| _status(f"Installing {pkg_id}...") | ||
| ok, pkg_logs = await asyncio.to_thread( | ||
| self._run_installer_quiet, | ||
| openskills_installers[pkg_id], | ||
| ) | ||
| elif pkg_id == "crawl4ai": | ||
| _status("Installing crawl4ai...") | ||
| ok, pkg_logs = await asyncio.to_thread( | ||
| self._run_installer_quiet, | ||
| install_crawl4ai_skill, | ||
| ) | ||
| else: | ||
| _quickstart_log(f"SkillsInstallStep: Unknown package id '{pkg_id}'") | ||
| pkg_logs = "" | ||
| ok = False | ||
|
|
||
| if pkg_logs: | ||
| _quickstart_log(f"SkillsInstallStep: {pkg_id} logs:\n{pkg_logs}") | ||
|
|
||
| if ok: | ||
| self._installed_packages.append(pkg_id) | ||
| else: | ||
| self._failed_packages.append(pkg_id) | ||
|
|
||
| self._install_attempted = True | ||
| if self._failed_packages: | ||
| _status( | ||
| f"Installed {len(self._installed_packages)} package(s), " f"{len(self._failed_packages)} failed. Retry or adjust selection.", | ||
| ) | ||
| else: | ||
| _status(f"Installed {len(self._installed_packages)} package(s) successfully.") | ||
|
|
||
| except Exception as e: | ||
| self._install_attempted = True | ||
| self._failed_packages = list(self._selected_packages) | ||
| _quickstart_log(f"SkillsInstallStep: Installation error: {e}") | ||
| _status(f"Skills installation failed: {e}") | ||
|
|
||
| self._installing = False | ||
| if self._install_button: | ||
| self._install_button.disabled = False | ||
| self._install_button.label = "Install Selected Packages" | ||
| for cb in self._checkboxes.values(): | ||
| cb.disabled = False | ||
|
|
||
| # Recompose so package status reflects newly installed items. | ||
| self.refresh(recompose=True) | ||
|
|
||
| def validate(self) -> Optional[str]: | ||
| if self._installing: | ||
| return "Please wait for skill installation to finish" | ||
|
|
||
| if not self._available_package_ids: | ||
| return None | ||
|
|
||
| if self._selected_packages and not self._install_attempted: | ||
| return "Click 'Install Selected Packages' before continuing, or deselect all packages" | ||
|
|
||
| return None | ||
|
|
||
| def get_value(self) -> Dict[str, Any]: | ||
| return { | ||
| "packages_to_install": list(self._selected_packages), | ||
| "install_attempted": self._install_attempted, | ||
| "installed_packages": list(self._installed_packages), | ||
| "failed_packages": list(self._failed_packages), | ||
| } | ||
|
|
||
| def set_value(self, value: Any) -> None: | ||
| if not isinstance(value, dict): | ||
| return | ||
| self._selected_packages = list(value.get("packages_to_install", [])) | ||
| self._install_attempted = bool(value.get("install_attempted", False)) | ||
| self._installed_packages = list(value.get("installed_packages", [])) | ||
| self._failed_packages = list(value.get("failed_packages", [])) | ||
|
|
There was a problem hiding this comment.
Add Google-style docstrings for the new SkillsInstallStep methods.
compose, validate, get_value, and set_value (and any other newly added methods) need Google-style docstrings.
💡 Example docstring (apply similarly to other methods)
- def compose(self) -> ComposeResult:
+ def compose(self) -> ComposeResult:
+ """Compose the skills selection UI.
+
+ Returns:
+ ComposeResult: UI nodes for the skills install step.
+ """As per coding guidelines: **/*.py: For new or changed functions, include Google-style docstrings
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 1109-1109: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@massgen/frontend/displays/textual_widgets/quickstart_wizard.py` around lines
929 - 1152, Add Google-style docstrings to all newly added/changed methods in
SkillsInstallStep: compose, on_checkbox_changed, _run_installer_quiet,
on_button_pressed, validate, get_value, and set_value. For each method include a
short summary line, Args with parameter types and descriptions (e.g., event:
Checkbox.Changed for on_checkbox_changed, event: Button.Pressed for
on_button_pressed, installer for _run_installer_quiet), Returns with the return
type and meaning (or None), and Raises if applicable; keep formatting consistent
with other project docstrings and place the docstring immediately below each def
declaration (triple-quoted Google-style).
| def on_checkbox_changed(self, event: Checkbox.Changed) -> None: | ||
| """Handle package selection toggles.""" | ||
| for pkg_id, cb in self._checkboxes.items(): | ||
| if cb.id != event.checkbox.id: | ||
| continue | ||
| if event.value and pkg_id not in self._selected_packages: | ||
| self._selected_packages.append(pkg_id) | ||
| elif not event.value and pkg_id in self._selected_packages: | ||
| self._selected_packages.remove(pkg_id) | ||
| break |
There was a problem hiding this comment.
Reset install state when package selection changes.
If a user changes selection after an install attempt, validate() still allows proceeding without re-installing newly selected packages. Consider resetting _install_attempted on selection changes.
🛠️ Suggested fix
def on_checkbox_changed(self, event: Checkbox.Changed) -> None:
"""Handle package selection toggles."""
for pkg_id, cb in self._checkboxes.items():
if cb.id != event.checkbox.id:
continue
if event.value and pkg_id not in self._selected_packages:
self._selected_packages.append(pkg_id)
elif not event.value and pkg_id in self._selected_packages:
self._selected_packages.remove(pkg_id)
- break
+ if self._install_attempted:
+ self._install_attempted = False
+ self._status_message = ""
+ break🤖 Prompt for AI Agents
In `@massgen/frontend/displays/textual_widgets/quickstart_wizard.py` around lines
990 - 999, The on_checkbox_changed handler must reset the install state when
selections change: inside QuickstartWizard.on_checkbox_changed (which iterates
self._checkboxes and updates self._selected_packages) set
self._install_attempted = False whenever a package is added or removed so
validate() cannot bypass re-install; ensure the flag is cleared before breaking
out of the loop so any change (event.value true or false) resets the install
attempt state.
|
prefer #869 , has these merged |
1 participant
PR Title Format
Your PR title must follow the format:
<type>: <brief description>Valid types:
fix:- Bug fixesfeat:- New featuresbreaking:- Breaking changesdocs:- Documentation updatesrefactor:- Code refactoringtest:- Test additions/modificationschore:- Maintenance tasksperf:- Performance improvementsstyle:- Code style changesci:- CI/CD configuration changesExamples:
fix: resolve memory leak in data processingfeat: add export to CSV functionalitybreaking: change API response formatdocs: update installation guideDescription
Brief description of the changes in this PR
Type of change
fix:) - Non-breaking change which fixes an issuefeat:) - Non-breaking change which adds functionalitybreaking:) - Fix or feature that would cause existing functionality to not work as expecteddocs:) - Documentation updatesrefactor:) - Code changes that neither fix a bug nor add a featuretest:) - Adding missing tests or correcting existing testschore:) - Maintenance tasks, dependency updates, etc.perf:) - Code changes that improve performancestyle:) - Changes that do not affect the meaning of the code (formatting, missing semi-colons, etc.)ci:) - Changes to CI/CD configuration files and scriptsChecklist
Pre-commit status
How to Test
Add test method for this PR.
Test CLI Command
Write down the test bash command. If there is pre-requests, please emphasize.
Expected Results
Description/screenshots of expected results.
Additional context
Add any other context about the PR here.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes