Conversation
📝 WalkthroughWalkthroughThis PR introduces a comprehensive git worktree and shadow-repository isolation infrastructure for agent workspaces during multi-agent coordination. It adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Orch as Orchestrator
participant ICM as IsolationContextManager
participant WM as WorktreeManager
participant FM as FilesystemManager
participant Modal as ReviewModal
participant CA as ChangeApplier
participant Orig as Original Repo
Orch->>ICM: initialize_context(path)
ICM->>WM: create_worktree(target, branch)
WM-->>ICM: worktree path
ICM-->>FM: isolated path & context info
FM-->>Orch: ready for execution
Orch->>Orch: execute agent in isolated context
Orch->>ICM: get_changes(context_path)
ICM-->>Orch: list of changes
alt TUI Mode
Orch->>Modal: show_change_review_modal(changes)
Modal->>Modal: parse diffs & render
Modal-->>Orch: ReviewResult (approved files)
else Non-TUI
Orch->>Orch: auto-approve all changes
end
Orch->>CA: apply_changes(isolated, original, approved_files)
CA->>Orig: copy/delete approved files
CA-->>Orch: applied files list
Orch->>ICM: move_scratch_to_workspace()
Orch->>ICM: cleanup_round()
ICM->>WM: remove_worktree()
WM-->>ICM: worktree removed
ICM-->>Orch: cleanup complete
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes The changes are substantial and intricate, spanning foundational infrastructure (git utilities, isolation management, change application), mid-layer plumbing (configuration, backend wiring, filesystem integration), and extensive UI/frontend reimplementation. The diff introduces multiple new classes and modules (~2700+ lines of substantive new code), complex orchestration logic with per-round isolation workflows, interactive modal UI with approval flows, and extensive theming/styling updates. While many changes follow consistent patterns (e.g., write_mode propagation, YAML config updates), the heterogeneity across layers—from git operations to TUI modals to system prompts—and the density of new logic (isolation manager with worktree/shadow/legacy modes, change diffing and application, modal interactions) demand careful, multi-faceted review. Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (7 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: 6
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
massgen/frontend/displays/textual_widgets/setup_wizard.py (1)
1055-1079:⚠️ Potential issue | 🟠 MajorRedundant synchronous
docker pullblocks the event loop.
on_wizard_completeperforms a synchronoussubprocess.run("docker", "pull", ...)(line 1067) for images remaining inimages_to_pull. This blocks the Textual event loop and freezes the TUI. Moreover, with the new in-step pull button inDockerSetupStep, successfully-pulled images are already removed from the list—but if the user skips the pull button, this code path runs synchronously.Consider either removing this redundant pull (letting the in-step button be the sole mechanism), or switching to
asyncio.create_subprocess_execto match the in-step approach.massgen/filesystem_manager/_docker_manager.py (1)
490-526:⚠️ Potential issue | 🟡 MinorDocument
extra_mount_pathsin the Google‑style docstring.
create_containergained a new parameter but the Args section wasn’t updated, so the docstring is now incomplete.As per coding guidelines
**/*.py: “For new or changed functions, include Google-style docstrings”.massgen/backend/base.py (1)
97-104:⚠️ Potential issue | 🟡 MinorAdd a Google‑style docstring to
LLMBackend.__init__.This function changed (new write_mode wiring) but still lacks a docstring. Please add a Google‑style docstring describing key args/behavior.
As per coding guidelines
**/*.py: “For new or changed functions, include Google-style docstrings”.massgen/frontend/displays/tui_event_pipeline.py (1)
96-100:⚠️ Potential issue | 🟡 MinorAdd a Google‑style docstring to
_apply_output.The function was modified but still lacks a docstring. Please document the method and its Args.
As per coding guidelines
**/*.py: “For new or changed functions, include Google-style docstrings”.✍️ Example docstring
def _apply_output(self, output: ContentOutput) -> None: + """Apply a parsed ContentOutput to the timeline. + + Args: + output: Parsed output payload to render. + """ timeline = self._get_timeline()massgen/orchestrator.py (1)
7562-7614:⚠️ Potential issue | 🟠 MajorRestart handling is now gated by isolation review.
The restart check is currently nested under the isolation review block, so when write_mode is off (no isolation manager), restarts will be skipped. That changes existing behavior and can silently disable restarts. Please dedent the restart block to run regardless of isolation.
🐛 Suggested fix
if hasattr(self, "_isolation_manager") and self._isolation_manager: selected_agent = self.agents.get(self._selected_agent) if selected_agent: # Re-add removed context paths so ChangeApplier can write back ppm = selected_agent.backend.filesystem_manager.path_permission_manager for orig_path, removed_mp in getattr(self, "_isolation_removed_paths", {}).items(): ppm.re_add_context_path(removed_mp) async for chunk in self._review_isolated_changes( agent=selected_agent, isolation_manager=self._isolation_manager, selected_agent_id=self._selected_agent, ): yield chunk # Clear the references after review self._isolation_manager = None self._isolation_worktree_paths = {} self._isolation_removed_paths = {} - # Check if restart was requested - if self.restart_pending and self.current_attempt < (self.max_attempts - 1): - ... - return + # Check if restart was requested + if self.restart_pending and self.current_attempt < (self.max_attempts - 1): + ... + returnmassgen/frontend/displays/textual/widgets/modals/content_modals.py (1)
108-117:⚠️ Potential issue | 🟡 Minor
subprocess.runcalls lackcheckandtimeoutparameters.These calls could silently fail (non-zero exit) or block the event loop indefinitely if the file manager doesn't detach. Since this is a Textual app (single-threaded event loop), a blocking subprocess would freeze the TUI.
Proposed fix: use `Popen` for fire-and-forget, or at least add a timeout
try: system = platform.system() if system == "Darwin": # macOS - subprocess.run(["open", str(workspace_path)]) + subprocess.Popen(["open", str(workspace_path)]) elif system == "Windows": - subprocess.run(["explorer", str(workspace_path)]) + subprocess.Popen(["explorer", str(workspace_path)]) else: # Linux - subprocess.run(["xdg-open", str(workspace_path)]) + subprocess.Popen(["xdg-open", str(workspace_path)]) except Exception as e:
🤖 Fix all issues with AI agents
In `@massgen/filesystem_manager/_change_applier.py`:
- Around line 98-109: The change detection currently only checks unstaged
working-tree vs index (repo.index.diff(None)) and untracked files, missing
staged-but-uncommitted changes; update _apply_git_changes (and similarly
get_changes_summary) to also include index-vs-HEAD diffs via
repo.index.diff("HEAD") and merge those results with the existing
repo.index.diff(None) and repo.untracked_files output so staged changes are
captured; for each diff from repo.index.diff("HEAD") use diff.a_path or
diff.b_path and diff.change_type[0].upper(), and ensure when merging you
deduplicate paths preferring the staged (HEAD vs index) change_type when present
so staged changes are not overwritten by working-tree diffs.
In `@massgen/filesystem_manager/_docker_manager.py`:
- Around line 657-663: The extra mount handling currently resolves container
paths on the host and doesn't validate host paths; update the loop that
processes extra_mount_paths (the block building volumes and mount_info) to (1)
stop calling Path(container_path).resolve() — treat container_path as a
container-side string (e.g., ensure it remains unmodified and normalized only as
a string) and (2) validate host_path before adding to volumes by resolving
host_path to an absolute path with Path(host_path).resolve() and checking
existence/type (raise or skip with a clear error if missing or not allowed) so
Docker doesn't auto-create unintended directories; update mount_info
construction to use the validated host_path_resolved and the original
container_path string.
In `@massgen/filesystem_manager/_path_permission_manager.py`:
- Around line 217-241: The update_context_path_permission method currently sets
mp.permission to Permission.WRITE without checking the coordination-phase flag;
enforce the context_write_access_enabled guard so WRITE is not applied
immediately when self.context_write_access_enabled is False (preserve
will_be_writable but keep mp.permission read-only), i.e. in
update_context_path_permission validate Permission.WRITE against
self.context_write_access_enabled and only set mp.permission to WRITE when the
flag is True (otherwise leave mp.permission unchanged and only set
mp.will_be_writable), mirroring the coordination behavior used in
add_context_paths and ensuring managed_paths, Permission, mp.permission and
mp.will_be_writable semantics are preserved.
In `@massgen/frontend/displays/textual_terminal_display.py`:
- Around line 2572-2577: The combined CSS is built from user_settings.theme
which can diverge from the resolved display.theme passed by callers; update
TextualApp.__init__ to determine the effective theme by using display.theme if
set (fallback to get_user_settings().theme), validate against self.PALETTE_MAP,
and then call self._get_combined_css_path(effective_theme) instead of using
user_settings.theme so the CSS palette matches the runtime theme applied in
on_mount; adjust references to theme variable accordingly (look for
TextualApp.__init__, get_user_settings, self.PALETTE_MAP,
_get_combined_css_path, and on_mount).
In `@massgen/infrastructure/worktree_manager.py`:
- Around line 43-52: The exception handlers in create_worktree and
remove_worktree currently catch GitCommandError and re-raise a RuntimeError
without preserving the original traceback; update both exception raises to chain
the original exception using "raise RuntimeError(f'Failed to create/remove
worktree: {e.stderr}') from e" (keep the existing logger.error calls that
reference e.stderr) so the original GitCommandError is preserved for debugging
while still normalizing the error type.
🟡 Minor comments (27)
massgen/frontend/displays/textual_widgets/setup_wizard.py-469-494 (1)
469-494:⚠️ Potential issue | 🟡 MinorPotential pipe deadlock: read stderr only after stdout is fully consumed.
Both
stdoutandstderrusePIPE. The code reads stdout line-by-line to completion before callingproc.stderr.read(). Ifdocker pullwrites enough to stderr to fill the OS pipe buffer (~64 KB) before closing stdout, the subprocess will block on the stderr write and never close stdout, creating a deadlock.For
docker pullthis is unlikely in practice (stderr is small), but consider usingproc.communicate()or reading both streams concurrently for robustness.Suggested safer approach
- proc = await asyncio.create_subprocess_exec( - "docker", - "pull", - image, - stdout=asyncio.subprocess.PIPE, - stderr=asyncio.subprocess.PIPE, - ) - # Stream stdout line-by-line to show layer progress - if proc.stdout: - async for raw_line in proc.stdout: - line = raw_line.decode(errors="replace").strip() - if line: - self._spinner_message = line - stderr = await proc.stderr.read() if proc.stderr else b"" - await proc.wait() + proc = await asyncio.create_subprocess_exec( + "docker", + "pull", + image, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + ) + + stderr_chunks: list[bytes] = [] + + async def _drain_stderr(): + if proc.stderr: + async for chunk in proc.stderr: + stderr_chunks.append(chunk) + + stderr_task = asyncio.create_task(_drain_stderr()) + + if proc.stdout: + async for raw_line in proc.stdout: + line = raw_line.decode(errors="replace").strip() + if line: + self._spinner_message = line + + await stderr_task + stderr = b"".join(stderr_chunks) + await proc.wait()massgen/frontend/displays/textual_widgets/file_explorer_panel.py-44-48 (1)
44-48:⚠️ Potential issue | 🟡 MinorPlaceholder entries are unintentionally clickable — clicking shows "file not found".
absolute_path=""is falsy, so_add_path(line 57:absolute_path or display_path) stores the display text"... (N more files)"as the tree-node data. That string is truthy, soon_tree_node_selectedcalls_show_preview, which fails to find the "file" and shows a confusing "file not found" message. The same issue applies to line 141.Fix
_add_pathto distinguish an explicit empty string fromNone:Proposed fix
def _add_path(self, display_path: str, status: str, absolute_path: Optional[str] = None) -> None: """Track a path for display and lookup.""" self._all_paths[display_path] = status - self._path_lookup[display_path] = absolute_path or display_path + self._path_lookup[display_path] = absolute_path if absolute_path is not None else display_pathmassgen/configs/debug/test_write_mode_isolation.yaml-1-38 (1)
1-38:⚠️ Potential issue | 🟡 MinorConfig placement doesn’t follow required configs/ subdirectory structure.
Please move this file under one of the approved subdirectories (
basic/,tools/,providers/,teams/) to keep the configs tree consistent.Based on learnings “Configuration YAML files in
massgen/configs/should be organized into subdirectories:basic/for simple single/multi-agent configs,tools/for MCP/filesystem/code execution configs,providers/for provider-specific examples, andteams/for pre-configured specialized teams”.massgen/infrastructure/shadow_repo.py-69-72 (1)
69-72:⚠️ Potential issue | 🟡 MinorChain the original exception for debuggability.
raise RuntimeError(...)discards the originalGitCommandErrortraceback. Usefrom eto preserve the exception chain. Also,logging.exceptionwould auto-include the traceback.Proposed fix
except GitCommandError as e: - logger.error(f"Failed to initialize shadow repo: {e.stderr}") + logger.exception(f"Failed to initialize shadow repo: {e.stderr}") self.cleanup() - raise RuntimeError(f"Failed to initialize shadow repo: {e.stderr}") + raise RuntimeError(f"Failed to initialize shadow repo: {e.stderr}") from emassgen/infrastructure/shadow_repo.py-127-139 (1)
127-139:⚠️ Potential issue | 🟡 MinorDocstring claims
.gitignoreis respected, but only.gitdirs are skipped.The docstring says "respecting .gitignore if present" but the implementation only filters out
.gitdirectories viashutil.ignore_patterns(".git"). Files that would be git-ignored (e.g.,__pycache__,node_modules) are still copied. Either update the docstring to reflect actual behavior, or add.gitignoreparsing.Proposed docstring fix
def _copy_source_files(self) -> None: - """Copy files from source to temp_dir, respecting .gitignore if present.""" + """Copy files from source to temp_dir, excluding .git directories."""massgen/utils/git_utils.py-109-134 (1)
109-134:⚠️ Potential issue | 🟡 Minor
get_changesdocstring claims staged changes are included, but they aren't.The docstring says "Get list of all changes (staged, unstaged, untracked)" but the implementation only collects unstaged changes (
repo.index.diff(None)) and untracked files. Staged changes (repo.index.diff("HEAD")) are not captured. Either update the docstring to say "unstaged and untracked" or add the staged diff.Option A: Fix docstring to match implementation
def get_changes(repo: Repo) -> List[Dict[str, str]]: """ - Get list of all changes (staged, unstaged, untracked) in a repo. + Get list of unstaged and untracked changes in a repo.Option B: Add staged changes to match docstring
changes = [] + # Staged changes (index vs HEAD) + try: + for diff in repo.index.diff("HEAD"): + changes.append( + { + "status": diff.change_type[0].upper(), + "path": diff.a_path or diff.b_path, + }, + ) + except Exception: + pass # Empty repo with no HEAD commit + # Unstaged changes (working tree vs index) for diff in repo.index.diff(None):massgen/cli.py-1471-1474 (1)
1471-1474:⚠️ Potential issue | 🟡 MinorPreserve explicit falsy
write_modevalues.
if write_mode_setting:skips explicit falsy values (e.g.,falsein YAML). IfFalseis a valid override, it won’t propagate to the backend config.🔧 Proposed fix
- write_mode_setting = coordination_settings_for_injection.get("write_mode") - if write_mode_setting: + write_mode_setting = coordination_settings_for_injection.get("write_mode") + if write_mode_setting is not None: backend_config["write_mode"] = write_mode_settingmassgen/tests/test_isolation_context.py-725-731 (1)
725-731:⚠️ Potential issue | 🟡 MinorRemove unused
tmp_pathfixture to satisfy lint.Proposed fix
- def test_remove_nonexistent_path_returns_none(self, tmp_path): + def test_remove_nonexistent_path_returns_none(self):massgen/tests/test_isolation_context.py-17-30 (1)
17-30:⚠️ Potential issue | 🟡 MinorAdd Google-style Args/Returns to
init_test_repodocstring.As per coding guidelines: `**/*.py`: For new or changed functions, include Google-style docstrings.Proposed fix
def init_test_repo(path: Path, with_commit: bool = True) -> Repo: - """Helper to initialize a test git repo with GitPython.""" + """Helper to initialize a test git repo with GitPython. + + Args: + path: Directory to initialize as a git repository. + with_commit: Whether to create an initial commit. + + Returns: + Initialized GitPython Repo instance. + """massgen/frontend/displays/textual/widgets/modals/review_modal.py-35-67 (1)
35-67:⚠️ Potential issue | 🟡 MinorAdd Google-style docstrings to
_ApprovalToggle/_FileEntrymethods.The new
__init__andon_clickhandlers are missing Google-style docstrings.As per coding guidelines:
**/*.py: For new or changed functions, include Google-style docstrings.scripts/test_review_modal.py-183-223 (1)
183-223:⚠️ Potential issue | 🟡 MinorAdd Google-style Args/Returns to
_build_combined_cssdocstring.As per coding guidelines: `**/*.py`: For new or changed functions, include Google-style docstrings.Proposed fix
def _build_combined_css(theme: str = "dark") -> Path: - """Build a combined CSS file from palette + modal_base + base, matching the real app. - - Always writes to the same fixed path so the stylesheet can be re-read on theme toggle. - """ + """Build a combined CSS file from palette + modal_base + base, matching the real app. + + Always writes to the same fixed path so the stylesheet can be re-read on theme toggle. + + Args: + theme: Theme name ("dark" or "light"). + + Returns: + Path to the combined CSS file on disk. + """scripts/test_review_modal.py-25-31 (1)
25-31:⚠️ Potential issue | 🟡 MinorRemove unused
noqa: E402directives to avoid Ruff warnings.Proposed fix
-from textual.app import App, ComposeResult # noqa: E402 -from textual.containers import Center, Vertical # noqa: E402 -from textual.widgets import Button, Footer, Header, Label, Static # noqa: E402 +from textual.app import App, ComposeResult +from textual.containers import Center, Vertical +from textual.widgets import Button, Footer, Header, Label, Static -from massgen.filesystem_manager import ReviewResult # noqa: E402 +from massgen.filesystem_manager import ReviewResult from massgen.frontend.displays.textual.widgets.modals.review_modal import ( # noqa: E402 GitDiffReviewModal, )scripts/test_review_modal.py-231-333 (1)
231-333:⚠️ Potential issue | 🟡 MinorAdd Google-style docstrings to
ReviewModalTestApppublic methods.
compose,on_button_pressed,action_*,_open_modal, and_handle_resultare new and should include Google-style docstrings (summary + Args/Returns where relevant).As per coding guidelines:
**/*.py: For new or changed functions, include Google-style docstrings.massgen/filesystem_manager/_change_applier.py-144-191 (1)
144-191:⚠️ Potential issue | 🟡 Minor
_apply_file_changesdoes not handle file deletions.The fallback method walks the source to find new/modified files, but never detects files that exist in the target but were deleted in the source. This means deletions would silently be ignored when the git-based path fails. Consider walking the target as well to detect removed files.
massgen/infrastructure/worktree_manager.py-25-29 (1)
25-29:⚠️ Potential issue | 🟡 Minor
repo.working_tree_dircan beNonefor bare repositories.If a bare repo path is passed,
Repo()succeeds butworking_tree_dirreturnsNone, leavingself.repo_path = Nonesilently. Subsequent operations would fail with confusing errors.Proposed fix
try: self.repo = Repo(repo_path, search_parent_directories=True) self.repo_path = self.repo.working_tree_dir + if self.repo_path is None: + raise ValueError(f"Path {repo_path} is in a bare git repository (no working tree)") except InvalidGitRepositoryError: - raise ValueError(f"Path {repo_path} is not in a git repository") + raise ValueError(f"Path {repo_path} is not in a git repository") from Nonemassgen/filesystem_manager/_filesystem_manager.py-411-429 (1)
411-429:⚠️ Potential issue | 🟡 MinorNon-git context paths are silently dropped when
write_modeis active.When
write_modeis set, Line 426 clearscontext_paths = []unconditionally. However,extra_mount_pathsonly includes entries for context paths that have a.gitdirectory (Line 420). If a user configures a context path that is not a git repository, it will neither be mounted as a regular context path nor as an extra.gitmount — effectively making it invisible to the agent with no warning.Consider logging a warning (or raising a validation error upstream) for context paths that lack a
.gitdirectory whenwrite_moderequires worktree-based isolation.Proposed fix
if ctx_path and os.path.isdir(git_dir): extra_mount_paths.append((git_dir, git_dir, "rw")) logger.info( f"[FilesystemManager] write_mode: mounting .git/ dir for worktree refs: {git_dir}", ) + elif ctx_path: + logger.warning( + f"[FilesystemManager] write_mode: context path {ctx_path} has no .git directory — " + "it will not be mounted. Worktree isolation requires a git repository.", + )massgen/frontend/displays/textual_themes/base.tcss-4966-4976 (1)
4966-4976:⚠️ Potential issue | 🟡 Minor
SubagentScreen AgentTabBarneeds.theme-light/.theme-darkoverrides for consistency.
AgentStatusRibbonand#input_headerboth pair$separator-borderwith explicit.theme-light/.theme-darkhex overrides (lines ~396–403, ~1093–1100).SubagentScreen AgentTabBar(line 4969) lacks equivalent overrides. WhileAgentStatusRibboninherits theme rules via global descendant selectors,AgentTabBarhas no corresponding theme-specific rules anywhere, risking incorrect border visibility in light mode.massgen/frontend/displays/textual_terminal_display.py-2907-2977 (1)
2907-2977:⚠️ Potential issue | 🟡 MinorAvoid writing debug JSON to a fixed
/tmppath.Even behind a debug flag, fixed
/tmp/textual_debug.jsoncan be clobbered or redirected (symlink attacks). Use a secure temp file or the session log directory to avoid predictable paths.🛠️ Suggested fix
- with open("/tmp/textual_debug.json", "w") as f: - json.dump(debug_info, f, indent=2, default=str) - self.log("DEBUG: Widget info written to /tmp/textual_debug.json") + import tempfile + with tempfile.NamedTemporaryFile("w", suffix=".json", delete=False) as f: + json.dump(debug_info, f, indent=2, default=str) + debug_path = f.name + self.log(f"DEBUG: Widget info written to {debug_path}")massgen/frontend/displays/textual_terminal_display.py-1687-1721 (1)
1687-1721:⚠️ Potential issue | 🟡 MinorTimeout handler can pop the wrong screen.
On timeout,
pop_screen()is invoked without checking that the review modal is still the active screen. If another modal was opened (or the review modal already closed), this can dismiss the wrong UI. Track the modal instance and only pop when it’s still on top (similar to the broadcast modal flow).🛠️ Suggested fix
- loop = asyncio.get_running_loop() - result_future: asyncio.Future[ReviewResult] = loop.create_future() + loop = asyncio.get_running_loop() + result_future: asyncio.Future[ReviewResult] = loop.create_future() + modal_ref = {"modal": None} @@ - modal = GitDiffReviewModal(changes=changes) + modal = GitDiffReviewModal(changes=changes) + modal_ref["modal"] = modal @@ - # Try to dismiss the modal on timeout - try: - self._app.call_from_thread(self._app.pop_screen) - except Exception: - pass + # Try to dismiss the modal on timeout (only if it’s still on top) + def safe_pop(): + if self._app and modal_ref["modal"] and self._app.screen_stack: + if self._app.screen_stack[-1] is modal_ref["modal"]: + self._app.pop_screen() + try: + self._app.call_from_thread(safe_pop) + except Exception: + passmassgen/frontend/displays/textual_terminal_display.py-603-607 (1)
603-607:⚠️ Potential issue | 🟡 MinorFinal-presentation gating still allows default “thinking” content to leak into the timeline.
The comment says only tools should pass, but the allowlist includes
"thinking". Sincecontent_typedefaults to"thinking", final-presentation chunks can still append to the timeline and reintroduce duplicates. If the goal is to suppress all non-tool content in this phase, drop"thinking"from the allowlist or ensure final presentation usescontent_type="presentation".🛠️ Possible adjustment (if tools-only is intended)
- if hasattr(self, "_in_final_presentation") and self._in_final_presentation: - if content_type not in ("tool", "thinking"): - return + if hasattr(self, "_in_final_presentation") and self._in_final_presentation: + if content_type != "tool": + returnmassgen/tests/test_write_mode_scratch.py-142-142 (1)
142-142:⚠️ Potential issue | 🟡 MinorTypo in class names: "Scatch" → "Scratch".
TestMoveScatchToWorkspace(line 142) andTestShadowModeCreatesScatch(line 405) both misspell "Scratch."Fix
-class TestMoveScatchToWorkspace: +class TestMoveScratchToWorkspace:-class TestShadowModeCreatesScatch: +class TestShadowModeCreatesScratch:Also applies to: 405-405
massgen/system_prompt_sections.py-949-971 (1)
949-971:⚠️ Potential issue | 🟡 MinorBranch info and scratch-archive note are duplicated once per worktree path.
The
for wt_path in self.worktree_paths:loop (line 951) contains the "### Code Branches" header,branch_name,other_branches, and the.scratch_archive/note. When multiple context paths produce multiple worktree entries, all of this gets repeated for every iteration. Only the "### Project Checkout" block is per-worktree; the branch/coordination info should be rendered once, outside the loop.Proposed fix: move branch info outside the loop
# Worktree-based workspace (new unified model) takes precedence if self.worktree_paths: for wt_path in self.worktree_paths: content_parts.append("### Project Checkout\n") content_parts.append(f"The project is checked out at `{wt_path}`. Full read/write access.\n") content_parts.append("**Scratch Space**: `.massgen_scratch/` inside the checkout") content_parts.append("- For experiments, eval scripts, notes") content_parts.append("- Git-excluded, invisible to reviewers") content_parts.append("- Can import from project naturally (e.g., `from src.foo import bar`)\n") - content_parts.append("### Code Branches\n") - if self.branch_name: - content_parts.append(f"Your work is on branch `{self.branch_name}`. All changes are auto-committed when your turn ends.\n") - else: - content_parts.append("All changes are auto-committed when your turn ends.\n") - - if self.other_branches: - content_parts.append("**Other agents' branches:**") - for label, branch in self.other_branches.items(): - content_parts.append(f"- {label}: `{branch}`") - content_parts.append("\nUse `git diff <branch>` to compare, `git merge <branch>` to incorporate.\n") - - content_parts.append("**Previous scratch files**: Check `.scratch_archive/` in your workspace for experiments from prior rounds.\n") + content_parts.append("### Code Branches\n") + if self.branch_name: + content_parts.append(f"Your work is on branch `{self.branch_name}`. All changes are auto-committed when your turn ends.\n") + else: + content_parts.append("All changes are auto-committed when your turn ends.\n") + + if self.other_branches: + content_parts.append("**Other agents' branches:**") + for label, branch in self.other_branches.items(): + content_parts.append(f"- {label}: `{branch}`") + content_parts.append("\nUse `git diff <branch>` to compare, `git merge <branch>` to incorporate.\n") + + content_parts.append("**Previous scratch files**: Check `.scratch_archive/` in your workspace for experiments from prior rounds.\n")massgen/orchestrator.py-8659-8675 (1)
8659-8675:⚠️ Potential issue | 🟡 MinorUnused parameter in
_review_isolated_changes.
agentisn’t used; either remove it or prefix with_to satisfy lint and clarify intent.🔧 Suggested fix
- async def _review_isolated_changes( - self, - agent: "ChatAgent", + async def _review_isolated_changes( + self, + _agent: "ChatAgent", isolation_manager: "IsolationContextManager", selected_agent_id: str, ) -> AsyncGenerator[StreamChunk, None]:massgen/orchestrator.py-7893-7905 (1)
7893-7905:⚠️ Potential issue | 🟡 MinorAvoid silent isolation cleanup failures.
The
try/except: passmasks cleanup issues and will make isolation failures hard to debug. Logging at debug is safer and satisfies lint.🧹 Suggested fix
- if self._isolation_manager: - try: - self._isolation_manager.cleanup_all() - except Exception: - pass + if self._isolation_manager: + try: + self._isolation_manager.cleanup_all() + except Exception as cleanup_err: + logger.debug( + f"[Orchestrator] Isolation cleanup failed during fallback: {cleanup_err}", + )massgen/orchestrator.py-5828-5883 (1)
5828-5883:⚠️ Potential issue | 🟡 MinorClean up partial worktrees on setup failure.
If setup fails mid‑way, lingering worktrees/branches can leak and affect later rounds. Consider cleaning up the partially created isolation session in the exception path.
🔧 Suggested fix
- if write_mode and write_mode != "legacy" and agent.backend.filesystem_manager: - try: + if write_mode and write_mode != "legacy" and agent.backend.filesystem_manager: + round_isolation_mgr = None + try: from .filesystem_manager import IsolationContextManager @@ - round_isolation_mgr = IsolationContextManager( + round_isolation_mgr = IsolationContextManager( session_id=f"{self.session_id}-{round_suffix}", write_mode=write_mode, workspace_path=workspace_path, previous_branch=previous_branch, ) @@ - except Exception as e: - logger.warning(f"[Orchestrator] Failed to create per-round worktree for {agent_id}: {e}") - round_worktree_paths = None + except Exception as e: + if round_isolation_mgr: + try: + round_isolation_mgr.cleanup_session() + except Exception as cleanup_err: + logger.debug( + f"[Orchestrator] Failed to cleanup partial worktree for {agent_id}: {cleanup_err}", + ) + logger.warning(f"[Orchestrator] Failed to create per-round worktree for {agent_id}: {e}") + round_worktree_paths = Nonemassgen/filesystem_manager/_isolation_context_manager.py-506-510 (1)
506-510:⚠️ Potential issue | 🟡 Minor
shutil.movebehaves differently when destination already exists.If
move_scratch_to_workspaceis called twice with the samearchive_label, the second call will nest the scratch directory inside the existing archive directory (e.g.,.scratch_archive/agent1/.massgen_scratch/) instead of replacing it. This could surprise consumers of the archive.Proposed fix: remove existing archive dir before moving
archive_dir = os.path.join(workspace, ".scratch_archive", archive_name) os.makedirs(os.path.dirname(archive_dir), exist_ok=True) try: + if os.path.exists(archive_dir): + shutil.rmtree(archive_dir) shutil.move(scratch_path, archive_dir)massgen/filesystem_manager/_isolation_context_manager.py-214-220 (1)
214-220:⚠️ Potential issue | 🟡 MinorBranch-name collision is possible when
branch_labelis set and multiple context paths exist in the same repository, though this is an edge case.When
initialize_contextis called multiple times with differentcontext_pathvalues that point to the same git repository (e.g.,repo/srcandrepo/testsboth withinrepo), andbranch_labelis provided (e.g.,"presenter"), the first call succeeds but the second will fail. Theinitialize_contextcache is keyed by individualcontext_path, not byrepo_root, so it cannot prevent the collision. Whengit worktree add -b "presenter"is called the second time, it will raiseGitCommandErrorbecause the branch already exists, which propagates as aRuntimeError.While multiple writable context paths in the same repository during final presentation is not a typical usage pattern and lacks test coverage, it is technically possible through the orchestrator's managed paths system. Consider either: (1) detecting this scenario and reusing the existing branch/worktree for same-repo paths, (2) appending a suffix to
branch_labelper context path, or (3) documenting thatbranch_labelassumes a single context path per repository during final presentation.
🧹 Nitpick comments (36)
massgen/frontend/displays/textual_widgets/multi_line_input.py (1)
601-608: Pre-existing: redundant identical branches in_delete_to_position.Both branches of the ternary on Line 603 evaluate to the same expression (
target_col + 1), and similarly Line 607 evaluates totarget_colin both branches. The logic happens to be correct because_find_char_positionalready adjuststarget_colfort/Tvsf/F, but the dead conditionals are confusing and suggest the author may have intended different behavior per motion type.Not introduced by this PR, so just flagging for awareness.
♻️ Suggested simplification
if motion in ("f", "t"): # Delete forward (include target for 'f', exclude for 't' already handled) - end = target_col + 1 if motion == "f" else target_col + 1 + end = target_col + 1 lines[row] = line[:col] + line[end:] else: # Delete backward - start = target_col if motion == "F" else target_col + start = target_col lines[row] = line[:start] + line[col:] col = start # Move cursor to deletion pointmassgen/frontend/displays/textual_widgets/setup_wizard.py (3)
519-521: Successfully-pulled image checkboxes are re-enabled and remain interactive.After a successful pull,
_selected_imagesis trimmed (line 503) but the corresponding checkboxes are re-enabled on line 520–521. A user could re-check a successfully-pulled image, causing it to be re-added to_selected_imagesviaon_checkbox_changedand re-pulled on retry.Consider disabling or removing checkboxes for successfully-pulled images.
Suggested fix
# Re-enable remaining checkboxes - for cb in self._checkboxes.values(): - cb.disabled = False + for img_name, cb in self._checkboxes.items(): + if img_name in pulled: + cb.value = False + cb.disabled = True + else: + cb.disabled = False
304-310: Instance attribute_SPINNER_FRAMESusesUPPER_CASEnaming.PEP 8 reserves
UPPER_CASEfor class-level constants. Since this is an instance attribute, either promote it to a class constant (likeAVAILABLE_IMAGES) or rename it to_spinner_frames.Suggested fix
Move to class level alongside
AVAILABLE_IMAGES:class DockerSetupStep(StepComponent): ... AVAILABLE_IMAGES = [...] + _SPINNER_FRAMES = ("⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏") def __init__(self, ...): ... - self._SPINNER_FRAMES = ("⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏")This aligns with the pattern in
textual_terminal_display.pywhereSPINNER_FRAMESis a class-level attribute (see relevant code snippet at line 2339).
1016-1025: Fragile .env parsing doesn't handle quoted values or inline comments.The manual
.envparser (lines 1020–1025) splits on=and strips whitespace, but doesn't handle quoted values (KEY="value") or inline comments (KEY=value # comment). If any existing.enventry uses these patterns, the re-written file will corrupt them.Consider using
dotenv's owndotenv_values()to read the existing file, sincepython-dotenvis already a dependency.Suggested fix
+ from dotenv import dotenv_values + existing_content = {} if env_path.exists(): - try: - with open(env_path, "r") as f: - for line in f: - line = line.strip() - if line and not line.startswith("#") and "=" in line: - key, value = line.split("=", 1) - existing_content[key.strip()] = value.strip() - except Exception as e: - _setup_log(f"SetupWizard: Could not read existing .env: {e}") + existing_content = dotenv_values(env_path) or {}massgen/frontend/displays/textual_widgets/content_sections.py (3)
36-38:_env_flagduplicatestui_debug_enabledlogic — consider reusing.This helper replicates the same truthy-env-var check that
tui_debug_enabled()intui_debug.pyuses. If more env flags are needed, this generic helper is fine. But if it's only used forMASSGEN_TUI_SCROLL_DEBUG(Line 445), you could document that relationship or import a shared helper to keep the "truthy env var" definition in one place.Not blocking — just a minor DRY note.
813-843: Consider logging timer stop failures instead of silencing them.The
enter_final_lockmethod thoroughly resets all scroll state and cancels pending timers — good. However, thetry/except passblocks at Lines 824-828 and 829-834 silently swallow exceptions, which was flagged by Ruff (S110). Elsewhere in this file (Line 752-754), timer stop failures are logged viatui_log. For consistency:♻️ Suggested fix
if self._scroll_timer is not None: try: self._scroll_timer.stop() - except Exception: - pass + except Exception as e: + tui_log(f"[ContentSections] enter_final_lock scroll_timer stop: {e}") self._scroll_timer = None if self._debounce_timer is not None: try: self._debounce_timer.stop() - except Exception: - pass + except Exception as e: + tui_log(f"[ContentSections] enter_final_lock debounce_timer stop: {e}") self._debounce_timer = None
2591-2591: Back-compat alias_vote_resultsshould stay in sync withvote_results.
self._vote_results = self.vote_resultsis set once in__init__. Ifvote_resultsis later mutated (e.g., dict updated in place), both references point to the same object, so they stay in sync. However, ifself.vote_resultsis ever reassigned to a new dict,_vote_resultswill go stale. If external code only reads_vote_results, this is fine for the current usage, but a@propertywould be safer long-term.massgen/frontend/displays/textual_widgets/file_explorer_panel.py (1)
142-143: Silentexcept Exception: passhides scanning failures.If
os.walkor the loop body raises an unexpected error, the panel silently shows nothing with no way to diagnose why. Consider logging atdebuglevel so issues are traceable without disrupting the TUI.Proposed fix
+import logging + +logger = logging.getLogger(__name__)- except Exception: - pass + except Exception: + logger.debug("Error scanning workspace %s", self.workspace_path, exc_info=True)pyproject.toml (1)
72-72: Consider makinggitpythonan optional dependency.If the worktree/shadow-repo isolation features aren't used by all users, moving
gitpythoninto an optional dependency group (e.g.,[project.optional-dependencies] isolation = ["gitpython>=3.1.46"]) would keep the default install lighter. This is similar to howdockeris already handled as an optional extra on line 110-112.massgen/configs/debug/test_write_mode_isolation.yaml (1)
16-19: Prefer a cost‑effective model for example/debug configs.Unless this test specifically needs Claude, consider switching to one of the preferred budget models (e.g., gpt‑5‑mini or gemini‑2.5‑flash).
As per coding guidelines
massgen/configs/**/*.yaml: “Prefer cost-effective models (gpt-5-nano, gpt-5-mini, gemini-2.5-flash)”.massgen/agent_config.py (1)
118-163: Add write_mode validation for programmatic configs.
CoordinationConfigcan be instantiated directly (bypassing YAML validation). Consider validatingwrite_modein__post_init__to fail fast on invalid values.♻️ Suggested patch
@@ def __post_init__(self): """Validate configuration after initialization.""" self._validate_broadcast_config() self._validate_timeout_config() + self._validate_write_mode() + + def _validate_write_mode(self) -> None: + """Validate write_mode configuration.""" + if self.write_mode is None: + return + valid = {"auto", "worktree", "isolated", "legacy"} + if self.write_mode not in valid: + raise ValueError( + f"Invalid write_mode: {self.write_mode}. Must be one of {sorted(valid)} or None." + )massgen/user_settings.py (2)
19-22: AnnotateDEFAULT_SETTINGSwithClassVar.Mutable class attributes should be annotated with
typing.ClassVarto signal they aren't instance attributes. Flagged by Ruff (RUF012).Proposed fix
+from typing import Any, ClassVar, Dict, Optional -from typing import Any, Dict, Optional ... - DEFAULT_SETTINGS = { + DEFAULT_SETTINGS: ClassVar[Dict[str, Any]] = { "theme": "dark", "vim_mode": False, }
24-28: Missing docstring on__init__.As per coding guidelines, new or changed functions should include Google-style docstrings.
__init__should document its behavior (loading settings from disk).Proposed fix
def __init__(self): + """Initialize UserSettings, loading from ~/.config/massgen/settings.json.""" self._settings: Dict[str, Any] = {}massgen/message_templates.py (1)
510-522: Docstring forbranch_infounderstates the accepted type ofother_branches.Line 521 documents
other_branchesaslist[str], but the implementation (Lines 556–560) also handlesdict(label→branch mapping). The docstring should reflect both accepted formats to avoid confusion for callers.Proposed docstring fix
- branch_info: Optional dict with 'own_branch' (str) and 'other_branches' (list[str]) - for communicating branch names from the previous attempt + branch_info: Optional dict with 'own_branch' (str) and 'other_branches' + (dict[str, str] mapping label to branch, or list[str] for legacy format) + for communicating branch names from the previous attemptmassgen/infrastructure/shadow_repo.py (1)
74-99:get_changes()duplicatesgit_utils.get_changes().The logic here is identical to
massgen/utils/git_utils.py:get_changes(repo). Consider delegating to avoid maintaining the same diff-enumeration code in two places.Proposed fix
+from massgen.utils.git_utils import get_changes as git_get_changes + def get_changes(self) -> List[Dict[str, str]]: - """ - Get list of changes in the shadow repo since initial commit. - - Returns: - List of dicts with 'status' and 'path' keys - """ + """Get list of changes in the shadow repo since initial commit.""" if not self.is_initialized or not self.repo: return [] - - changes = [] - - # Unstaged changes (working tree vs index) - for diff in self.repo.index.diff(None): - changes.append( - { - "status": diff.change_type[0].upper(), # M, A, D, R - "path": diff.a_path or diff.b_path, - }, - ) - - # Untracked files - for path in self.repo.untracked_files: - changes.append({"status": "?", "path": path}) - - return changes + return git_get_changes(self.repo)massgen/frontend/coordination_ui.py (1)
824-829: Duplicatedensure_workspace_symlinksblock acrosscoordinate()andcoordinate_with_context().Both methods contain an identical 6-line block for symlink creation. This is consistent with the existing parallelism between these two methods, but worth noting as a candidate for extraction into a shared helper if these methods are ever refactored.
The broad
Exceptioncatch (flagged by Ruff BLE001) is acceptable here since symlink creation is non-critical and the orchestrator method already has its own internal exception handling (seemassgen/orchestrator.py, Lines 528-548).Also applies to: 1458-1463
docs/source/user_guide/agent_workspaces.rst (1)
82-83: Minor: RST underline length mismatch.The title is 37 characters but the underline is 39. RST requires the underline to be at least as long as the title (so this works), but convention is to match exactly. Very minor nit.
docs/modules/worktrees.md (1)
9-23: Add language identifiers to fenced code blocks.Three code blocks lack language specifiers (flagged by markdownlint MD040). Since these are ASCII diagrams and text blocks,
textis appropriate:Suggested fix
Line 9:
-``` +```textLine 41:
-``` +```textLine 59:
-``` +```textAlso applies to: 41-45, 59-65
massgen/infrastructure/__init__.py (1)
14-21:__all__is not sorted (Ruff RUF022).The
filesystem_manager/__init__.pyin this same PR uses alphabetical ordering. Align for consistency:Suggested fix
__all__ = [ + "ShadowRepo", "WorktreeManager", - "ShadowRepo", - "get_git_root", "get_git_branch", + "get_git_root", "get_git_status", "is_git_repo", ]massgen/filesystem_manager/_change_applier.py (2)
93-96: Chain the original exception for debuggability.Ruff B904 is valid here — re-raising without
fromloses the original traceback context.Proposed fix
try: repo = Repo(str(source)) except InvalidGitRepositoryError: - raise ValueError(f"Source is not a git repository: {source}") + raise ValueError(f"Source is not a git repository: {source}") from None
139-140: Uselog.exception()instead oflog.error()in exception handlers.Within
exceptblocks,log.exception()automatically includes the traceback, which is valuable for debugging file-apply failures. This applies to Lines 140, 189, and 240.Proposed fix (example for line 140)
except Exception as e: - log.error(f"Failed to apply change for {rel_path}: {e}") + log.exception(f"Failed to apply change for {rel_path}: {e}")Also applies to: 188-189, 239-240
massgen/infrastructure/worktree_manager.py (2)
127-135: Silent error swallowing in_delete_branchloses diagnostic information.If branch deletion fails (e.g., branch is checked out elsewhere), the bare
passmakes it invisible. A debug-level log would aid troubleshooting without being noisy.Proposed fix
except GitCommandError: - pass + logger.debug(f"Could not delete branch {branch_name} (may already be deleted or in use)")
15-16: Missing Google-style class docstring.The class has a one-liner docstring but no
Attributes:section documentingrepoandrepo_path. As per coding guidelines, new classes inmassgen/**/*.pyshould include Google-style docstrings.massgen/frontend/displays/textual_themes/base.tcss (2)
4819-4857: Toast theming approach is sound; hardcoded hex duplicates the pattern from AgentStatusRibbon.The override strategy (base → theme-class → severity) is correct and source-order ensures severity
border-leftwins over the theme default. Minor observation: the hardcoded hex values (#ffffff,#d0d7de,#0969da, etc.) for light/dark toasts duplicate the same GitHub-style palette values used in the AgentStatusRibbon and#input_headeroverrides. If these are already defined as variables in your palette files, referencing the variables would reduce the duplication and make future palette changes easier.
392-403: Hardcoded separator hex values duplicated across selectors.The same hex pairs (
#eaeef2for light,#30363dfor dark) appear in bothAgentStatusRibbon(lines 398, 402) and#input_header(lines 1095, 1099). If$separator-borderis already theme-aware in the palette, the.theme-light/.theme-darkoverrides may be redundant. If not, consider extracting these into dedicated palette variables to avoid the duplication and make future color changes a single-point edit.Also applies to: 1088-1100
massgen/frontend/displays/textual_terminal_display.py (1)
3861-3866: Don’t silently swallow preference persistence failures.Failures to persist vim mode/theme are currently swallowed, which makes preference bugs very hard to diagnose. Consider logging at least a warning so users/devs can see why preferences didn’t save.
🛠️ Suggested fix (apply similarly to theme persistence)
try: user_settings = get_user_settings() user_settings.vim_mode = self.question_input.vim_mode - except Exception: - pass + except Exception as e: + tui_log(f"[TextualDisplay] Failed to persist vim_mode: {e}")Also applies to: 6261-6265
massgen/system_prompt_sections.py (1)
918-937: Update class docstring with the new constructor parameters.The class-level docstring (lines 905–916) still lists only
workspace_path,context_paths, anduse_two_tier_workspace. The three new parameters—worktree_paths,branch_name, andother_branches—should be documented there as well to keep the Args block accurate.As per coding guidelines,
**/*.py: "For new or changed functions, include Google-style docstrings."massgen/tests/test_write_mode_scratch.py (1)
381-402: Testing a private method (_apply_file_changes) directly.This is acceptable for unit tests of internal logic, but note that if the method signature changes, this test will break without a public API change. Consider whether a thin public wrapper or integration test would be more maintainable long-term.
massgen/filesystem_manager/_isolation_context_manager.py (5)
265-267: Chain exceptions withraise ... from eto preserve traceback context.Both
_create_worktree_contextand_create_shadow_contextre-raise asRuntimeErrorbut discard the original exception chain. Also,log.errorwon't include the traceback; preferlog.exception.Proposed fix (showing worktree variant; apply same pattern to shadow)
except Exception as e: - log.error(f"Failed to create worktree: {e}") - raise RuntimeError(f"Failed to create worktree context: {e}") + log.exception(f"Failed to create worktree: {e}") + raise RuntimeError(f"Failed to create worktree context: {e}") from eexcept Exception as e: - log.error(f"Failed to create shadow repo: {e}") - raise RuntimeError(f"Failed to create shadow context: {e}") + log.exception(f"Failed to create shadow repo: {e}") + raise RuntimeError(f"Failed to create shadow context: {e}") from eAlso applies to: 288-290
645-660: Silenttry/except/passblocks swallow errors during session cleanup.These blocks (lines 648–652 and 657–660) silently discard exceptions, making it difficult to diagnose cleanup failures. Even at session end, a
log.debugwould aid troubleshooting stale branches or worktree metadata.Proposed fix
for branch_name in self._session_branches: for wm in self._worktree_managers.values(): try: wm._delete_branch(branch_name, force=True) log.info(f"Cleaned up session branch: {branch_name}") - except Exception: - pass + except Exception as e: + log.debug(f"Could not delete branch {branch_name}: {e}") self._session_branches.clear() # Prune any stale worktree metadata for wm in self._worktree_managers.values(): try: wm.prune() - except Exception: - pass + except Exception as e: + log.debug(f"Failed to prune worktree metadata: {e}")
744-761:get_difffor worktree mode stages and resets — non-atomic and leaks staged state on failure.The
add -A→diff --staged→reset HEADsequence at lines 756–758 is a known trick, but ifdifforresetthrows (e.g., corrupted index), the worktree is left in a fully-staged state. Since this is an isolated worktree and the window is small, it's low risk, but worth noting.Consider wrapping the reset in a
finallyblock:Proposed fix
# Stage everything so untracked (new) files appear in the diff, # then unstage so ChangeApplier can still detect changes via # repo.index.diff(None) and repo.untracked_files. repo.git.add("-A") - diff_output = repo.git.diff("--staged") - repo.git.reset("HEAD") - return diff_output + try: + diff_output = repo.git.diff("--staged") + finally: + repo.git.reset("HEAD") + return diff_output
811-816: Anothertry/except/passincleanup_all— same silent swallowing concern ascleanup_session.Same pattern as flagged in
cleanup_session. Addlog.debugfor consistency.Proposed fix
for wm in self._worktree_managers.values(): try: wm.prune() - except Exception: - pass + except Exception as e: + log.debug(f"Failed to prune worktree metadata: {e}")
292-293: Method name_setup_scratch_in_worktreeis misleading — it is also called for shadow repos.Line 284 calls this from
_create_shadow_context. Consider renaming to_setup_scratch_diror_setup_scratch_in_isolationto reflect actual usage.massgen/frontend/displays/textual/widgets/modals/content_modals.py (3)
390-403: Index-based button dispatch has a TOCTOU window if the path list changes between render and click.
on_button_pressedparses an integer index from the button ID and uses it to index into a fresh call to_get_current_paths()(inside_toggle_permission/_remove_path). If another agent or process modifies the paths between the render and the click, the index could refer to a different path—or be out of bounds (though the bounds check at lines 408/422 prevents a crash).The risk is low in practice since changes require user action in the same modal, but worth noting.
289-301:_get_current_pathsreads only from the first agent's PPM.If agents' path lists ever diverge (e.g., a propagation partially fails), the modal will only reflect the first agent's state. Consider adding a defensive log if agents have inconsistent path sets, so divergence is surfaced early.
500-525:_update_agent_config_context_pathssilently drops through when no keyword flags match.If none of
remove,add, ornew_permissionis truthy, the method exits without doing anything. This is technically correct since callers always set exactly one, but adding anelselog or assertion would catch misuse.
| # Get all changed files | ||
| changed_files: Dict[str, str] = {} # path -> change_type (M, A, D, ?) | ||
|
|
||
| # Unstaged changes (working tree vs index) | ||
| for diff in repo.index.diff(None): | ||
| rel_path = diff.a_path or diff.b_path | ||
| if rel_path: | ||
| changed_files[rel_path] = diff.change_type[0].upper() | ||
|
|
||
| # Untracked files (new files) | ||
| for rel_path in repo.untracked_files: | ||
| changed_files[rel_path] = "A" # Treat untracked as added |
There was a problem hiding this comment.
_apply_git_changes only detects unstaged changes — staged changes are missed.
repo.index.diff(None) (Line 102) detects working-tree-vs-index changes, and repo.untracked_files (Line 108) catches new files. However, if the isolation context stages changes via git add (without committing), those would only appear in repo.index.diff("HEAD"), not in repo.index.diff(None). The same gap exists in get_changes_summary (Line 222).
Proposed fix
# Unstaged changes (working tree vs index)
for diff in repo.index.diff(None):
rel_path = diff.a_path or diff.b_path
if rel_path:
changed_files[rel_path] = diff.change_type[0].upper()
+ # Staged changes (index vs HEAD)
+ for diff in repo.index.diff("HEAD"):
+ rel_path = diff.a_path or diff.b_path
+ if rel_path and rel_path not in changed_files:
+ changed_files[rel_path] = diff.change_type[0].upper()
+
# Untracked files (new files)
for rel_path in repo.untracked_files:
changed_files[rel_path] = "A" # Treat untracked as added🤖 Prompt for AI Agents
In `@massgen/filesystem_manager/_change_applier.py` around lines 98 - 109, The
change detection currently only checks unstaged working-tree vs index
(repo.index.diff(None)) and untracked files, missing staged-but-uncommitted
changes; update _apply_git_changes (and similarly get_changes_summary) to also
include index-vs-HEAD diffs via repo.index.diff("HEAD") and merge those results
with the existing repo.index.diff(None) and repo.untracked_files output so
staged changes are captured; for each diff from repo.index.diff("HEAD") use
diff.a_path or diff.b_path and diff.change_type[0].upper(), and ensure when
merging you deduplicate paths preferring the staged (HEAD vs index) change_type
when present so staged changes are not overwritten by working-tree diffs.
| # Mount extra paths (e.g., worktree isolation paths) | ||
| if extra_mount_paths: | ||
| for host_path, container_path, mode in extra_mount_paths: | ||
| host_path_resolved = str(Path(host_path).resolve()) | ||
| container_path_str = str(Path(container_path).resolve()) | ||
| volumes[host_path_resolved] = {"bind": container_path_str, "mode": mode} | ||
| mount_info.append(f" {host_path_resolved} ← {container_path_str} ({mode}, extra)") |
There was a problem hiding this comment.
Avoid resolving container paths on the host and validate extra mounts.
Path(container_path).resolve() converts relative container paths into host paths, which can mount to an unintended location inside the container. Also, missing host-path checks can cause Docker to create directories unexpectedly. Consider treating container_path as a container-side string and validating host_path existence before mounting.
🛠️ Suggested fix
- if extra_mount_paths:
- for host_path, container_path, mode in extra_mount_paths:
- host_path_resolved = str(Path(host_path).resolve())
- container_path_str = str(Path(container_path).resolve())
- volumes[host_path_resolved] = {"bind": container_path_str, "mode": mode}
- mount_info.append(f" {host_path_resolved} ← {container_path_str} ({mode}, extra)")
+ if extra_mount_paths:
+ for host_path, container_path, mode in extra_mount_paths:
+ host_path_resolved = Path(host_path).expanduser().resolve()
+ if not host_path_resolved.exists():
+ logger.warning(f"⚠️ [Docker] Extra mount path not found: {host_path}")
+ continue
+ # Keep container_path as a container-side path (don’t resolve on host).
+ container_path_str = str(container_path)
+ volumes[str(host_path_resolved)] = {"bind": container_path_str, "mode": mode}
+ mount_info.append(f" {host_path_resolved} ← {container_path_str} ({mode}, extra)")🤖 Prompt for AI Agents
In `@massgen/filesystem_manager/_docker_manager.py` around lines 657 - 663, The
extra mount handling currently resolves container paths on the host and doesn't
validate host paths; update the loop that processes extra_mount_paths (the block
building volumes and mount_info) to (1) stop calling
Path(container_path).resolve() — treat container_path as a container-side string
(e.g., ensure it remains unmodified and normalized only as a string) and (2)
validate host_path before adding to volumes by resolving host_path to an
absolute path with Path(host_path).resolve() and checking existence/type (raise
or skip with a clear error if missing or not allowed) so Docker doesn't
auto-create unintended directories; update mount_info construction to use the
validated host_path_resolved and the original container_path string.
| def _cleanup_single_context(self, context_path: str) -> None: | ||
| """Cleanup a single isolated context.""" | ||
| if context_path not in self._contexts: | ||
| return | ||
|
|
||
| ctx = self._contexts[context_path] | ||
| mode = ctx.get("mode") | ||
|
|
||
| if mode == "shadow": | ||
| manager = ctx.get("manager") | ||
| if isinstance(manager, ShadowRepo): | ||
| manager.cleanup() | ||
|
|
||
| elif mode == "worktree": | ||
| manager = ctx.get("manager") | ||
| isolated_path = ctx.get("isolated_path") | ||
| if isinstance(manager, WorktreeManager) and isolated_path: | ||
| try: | ||
| manager.remove_worktree(isolated_path, force=True, delete_branch=True) | ||
| except Exception as e: | ||
| log.warning(f"Failed to cleanup worktree {isolated_path}: {e}") | ||
|
|
||
| del self._contexts[context_path] |
There was a problem hiding this comment.
_cleanup_single_context does not handle "workspace" mode — branches may leak.
cleanup / cleanup_all delegates to _cleanup_single_context, which only handles "shadow" and "worktree" modes. For "workspace" contexts (created by setup_workspace_scratch), the branch is not switched back to the default, nor is it deleted. Compare with cleanup_round (line 576–596) and cleanup_session (line 625–637), which both handle workspace mode explicitly.
Since __exit__ calls cleanup_all, using the context manager will leak workspace branches.
Proposed fix: add workspace handling to `_cleanup_single_context`
def _cleanup_single_context(self, context_path: str) -> None:
"""Cleanup a single isolated context."""
if context_path not in self._contexts:
return
ctx = self._contexts[context_path]
mode = ctx.get("mode")
if mode == "shadow":
manager = ctx.get("manager")
if isinstance(manager, ShadowRepo):
manager.cleanup()
elif mode == "worktree":
manager = ctx.get("manager")
isolated_path = ctx.get("isolated_path")
if isinstance(manager, WorktreeManager) and isolated_path:
try:
manager.remove_worktree(isolated_path, force=True, delete_branch=True)
except Exception as e:
log.warning(f"Failed to cleanup worktree {isolated_path}: {e}")
+ elif mode == "workspace":
+ branch_name = ctx.get("branch_name")
+ try:
+ from git import Repo
+
+ repo = Repo(context_path)
+ for name in ("main", "master"):
+ if name in [b.name for b in repo.branches]:
+ repo.git.checkout(name)
+ break
+ if branch_name:
+ repo.git.branch("-D", branch_name)
+ except Exception as e:
+ log.warning(f"Failed to cleanup workspace {context_path}: {e}")
+
del self._contexts[context_path]
log.info(f"Cleaned up isolated context: {context_path}")🧰 Tools
🪛 Ruff (0.14.14)
[warning] 798-798: Do not catch blind exception: Exception
(BLE001)
| def update_context_path_permission(self, path_str: str, new_permission: str) -> bool: | ||
| """Update the permission of a context path. | ||
|
|
||
| Args: | ||
| path_str: The context path to update | ||
| new_permission: New permission value ("read" or "write") | ||
|
|
||
| Returns: | ||
| True if the path was found and updated, False otherwise | ||
| """ | ||
| resolved = Path(path_str).resolve() | ||
| try: | ||
| perm = Permission(new_permission.lower()) | ||
| except ValueError: | ||
| logger.warning(f"[PathPermissionManager] Invalid permission '{new_permission}', ignoring update") | ||
| return False | ||
|
|
||
| for mp in self.managed_paths: | ||
| if mp.path.resolve() == resolved and mp.path_type == "context": | ||
| mp.permission = perm | ||
| mp.will_be_writable = perm == Permission.WRITE | ||
| self._permission_cache.clear() | ||
| logger.info(f"[PathPermissionManager] Updated context path permission: {path_str} -> {new_permission}") | ||
| return True | ||
| return False |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "update_context_path_permission" -SRepository: massgen/MassGen
Length of output: 355
🏁 Script executed:
# First, let's examine the file structure and find the class definition
head -250 massgen/filesystem_manager/_path_permission_manager.py | tail -100Repository: massgen/MassGen
Length of output: 4462
🏁 Script executed:
# Search for context_write_access_enabled usage
rg -n "context_write_access_enabled" -A 2 -B 2Repository: massgen/MassGen
Length of output: 24215
🏁 Script executed:
# Look for the full PathPermissionManager class to understand context_write_access_enabled
fd "_path_permission_manager.py" -x wc -l {}Repository: massgen/MassGen
Length of output: 173
🏁 Script executed:
# Look at the call site in content_modals.py
sed -n '450,490p' massgen/frontend/displays/textual/widgets/modals/content_modals.pyRepository: massgen/MassGen
Length of output: 2134
🏁 Script executed:
# Also check the get_permission method to see if it enforces context_write_access_enabled
grep -n "def get_permission" massgen/filesystem_manager/_path_permission_manager.pyRepository: massgen/MassGen
Length of output: 178
🏁 Script executed:
# Read the get_permission implementation
sed -n '330,420p' massgen/filesystem_manager/_path_permission_manager.pyRepository: massgen/MassGen
Length of output: 4123
🏁 Script executed:
# Get the full get_permission method implementation
sed -n '541,600p' massgen/filesystem_manager/_path_permission_manager.pyRepository: massgen/MassGen
Length of output: 3377
🏁 Script executed:
# Let's see more of the get_permission method
sed -n '541,650p' massgen/filesystem_manager/_path_permission_manager.pyRepository: massgen/MassGen
Length of output: 5802
🏁 Script executed:
# Look for _validate_write_tool to see if it has a secondary guard
grep -n "_validate_write_tool" massgen/filesystem_manager/_path_permission_manager.pyRepository: massgen/MassGen
Length of output: 325
🏁 Script executed:
# Check the actual implementation of _validate_write_tool
sed -n '1140,1210p' massgen/filesystem_manager/_path_permission_manager.pyRepository: massgen/MassGen
Length of output: 4145
Enforce context_write_access_enabled guard in update_context_path_permission.
This method can grant WRITE access even when context_write_access_enabled is False, bypassing the coordination-phase isolation enforced by add_context_paths(). The design intent is that during coordination (when the flag is False), all context paths remain read-only. While will_be_writable is correctly preserved, the actual mp.permission is set without checking the flag, allowing tool validation to grant immediate write access.
Recommended: Prevent setting Permission.WRITE when context_write_access_enabled is False, or verify that the UI caller enforces write-access ordering before invoking this method.
Suggested approach
- mp.permission = perm
- mp.will_be_writable = perm == Permission.WRITE
+ effective_perm = perm
+ if perm == Permission.WRITE and not self.context_write_access_enabled:
+ effective_perm = Permission.READ
+ mp.permission = effective_perm
+ mp.will_be_writable = perm == Permission.WRITE📝 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.
| def update_context_path_permission(self, path_str: str, new_permission: str) -> bool: | |
| """Update the permission of a context path. | |
| Args: | |
| path_str: The context path to update | |
| new_permission: New permission value ("read" or "write") | |
| Returns: | |
| True if the path was found and updated, False otherwise | |
| """ | |
| resolved = Path(path_str).resolve() | |
| try: | |
| perm = Permission(new_permission.lower()) | |
| except ValueError: | |
| logger.warning(f"[PathPermissionManager] Invalid permission '{new_permission}', ignoring update") | |
| return False | |
| for mp in self.managed_paths: | |
| if mp.path.resolve() == resolved and mp.path_type == "context": | |
| mp.permission = perm | |
| mp.will_be_writable = perm == Permission.WRITE | |
| self._permission_cache.clear() | |
| logger.info(f"[PathPermissionManager] Updated context path permission: {path_str} -> {new_permission}") | |
| return True | |
| return False | |
| def update_context_path_permission(self, path_str: str, new_permission: str) -> bool: | |
| """Update the permission of a context path. | |
| Args: | |
| path_str: The context path to update | |
| new_permission: New permission value ("read" or "write") | |
| Returns: | |
| True if the path was found and updated, False otherwise | |
| """ | |
| resolved = Path(path_str).resolve() | |
| try: | |
| perm = Permission(new_permission.lower()) | |
| except ValueError: | |
| logger.warning(f"[PathPermissionManager] Invalid permission '{new_permission}', ignoring update") | |
| return False | |
| for mp in self.managed_paths: | |
| if mp.path.resolve() == resolved and mp.path_type == "context": | |
| effective_perm = perm | |
| if perm == Permission.WRITE and not self.context_write_access_enabled: | |
| effective_perm = Permission.READ | |
| mp.permission = effective_perm | |
| mp.will_be_writable = perm == Permission.WRITE | |
| self._permission_cache.clear() | |
| logger.info(f"[PathPermissionManager] Updated context path permission: {path_str} -> {new_permission}") | |
| return True | |
| return False |
🤖 Prompt for AI Agents
In `@massgen/filesystem_manager/_path_permission_manager.py` around lines 217 -
241, The update_context_path_permission method currently sets mp.permission to
Permission.WRITE without checking the coordination-phase flag; enforce the
context_write_access_enabled guard so WRITE is not applied immediately when
self.context_write_access_enabled is False (preserve will_be_writable but keep
mp.permission read-only), i.e. in update_context_path_permission validate
Permission.WRITE against self.context_write_access_enabled and only set
mp.permission to WRITE when the flag is True (otherwise leave mp.permission
unchanged and only set mp.will_be_writable), mirroring the coordination behavior
used in add_context_paths and ensuring managed_paths, Permission, mp.permission
and mp.will_be_writable semantics are preserved.
| # Load user settings for theme preference | ||
| user_settings = get_user_settings() | ||
| theme = user_settings.theme | ||
| if theme not in self.PALETTE_MAP: | ||
| theme = "dark" | ||
| combined_css_path = self._get_combined_css_path(theme) |
There was a problem hiding this comment.
Align combined CSS with the effective theme (explicit overrides are ignored today).
TextualApp.__init__ builds the combined CSS using user_settings.theme, which can diverge from display.theme when a caller explicitly passes a theme. That creates a mismatch between the palette in CSS and the theme/classes later applied in on_mount, producing inconsistent colors. Use the already-resolved display.theme (fallback to user settings if needed) so CSS and runtime theme stay aligned.
🛠️ Suggested fix
- # Load user settings for theme preference
- user_settings = get_user_settings()
- theme = user_settings.theme
+ # Use the effective theme (explicit override wins)
+ theme = display.theme or get_user_settings().theme
if theme not in self.PALETTE_MAP:
theme = "dark"🤖 Prompt for AI Agents
In `@massgen/frontend/displays/textual_terminal_display.py` around lines 2572 -
2577, The combined CSS is built from user_settings.theme which can diverge from
the resolved display.theme passed by callers; update TextualApp.__init__ to
determine the effective theme by using display.theme if set (fallback to
get_user_settings().theme), validate against self.PALETTE_MAP, and then call
self._get_combined_css_path(effective_theme) instead of using
user_settings.theme so the CSS palette matches the runtime theme applied in
on_mount; adjust references to theme variable accordingly (look for
TextualApp.__init__, get_user_settings, self.PALETTE_MAP,
_get_combined_css_path, and on_mount).
| target_path = os.path.abspath(target_path) | ||
|
|
||
| try: | ||
| # git worktree add -b <branch> <path> <base_commit> | ||
| self.repo.git.worktree("add", "-b", branch_name, target_path, base_commit) | ||
| logger.info(f"Created worktree at {target_path} on branch {branch_name}") | ||
| return target_path | ||
| except GitCommandError as e: | ||
| logger.error(f"Failed to create worktree: {e.stderr}") | ||
| raise RuntimeError(f"Failed to create worktree: {e.stderr}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Chain the original exception for debuggability.
Both create_worktree and remove_worktree catch GitCommandError and re-raise as RuntimeError, but without from e the original traceback is lost. This makes debugging harder when worktree operations fail.
Proposed fix
except GitCommandError as e:
logger.error(f"Failed to create worktree: {e.stderr}")
- raise RuntimeError(f"Failed to create worktree: {e.stderr}")
+ raise RuntimeError(f"Failed to create worktree: {e.stderr}") from e except GitCommandError as e:
logger.error(f"Failed to remove worktree: {e.stderr}")
- raise RuntimeError(f"Failed to remove worktree: {e.stderr}")
+ raise RuntimeError(f"Failed to remove worktree: {e.stderr}") from eAlso applies to: 66-78
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 49-49: Consider moving this statement to an else block
(TRY300)
[warning] 51-51: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 52-52: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 52-52: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@massgen/infrastructure/worktree_manager.py` around lines 43 - 52, The
exception handlers in create_worktree and remove_worktree currently catch
GitCommandError and re-raise a RuntimeError without preserving the original
traceback; update both exception raises to chain the original exception using
"raise RuntimeError(f'Failed to create/remove worktree: {e.stderr}') from e"
(keep the existing logger.error calls that reference e.stderr) so the original
GitCommandError is preserved for debugging while still normalizing the error
type.
2 participants
PR Draft: v0.1.47 Release Fixes
Summary
This PR addresses multiple issues scheduled for the v0.1.47 release.
Closes MAS-264
MAS-272: Unify write_mode and two-tier workspace with in-worktree scratch
Closes MAS-272
Summary: Unified
write_mode(worktree isolation) anduse_two_tier_workspace(scratch/deliverable dirs) into a single system where every agent works inside a per-round git worktree with.massgen_scratch/for experiments.Key Changes
IsolationContextManager (
massgen/filesystem_manager/_isolation_context_manager.py):.massgen_scratch/creation inside worktrees (git-excluded viainfo/exclude)move_scratch_to_workspace()to archive scratch to.scratch_archive/before teardowncleanup_round()(remove worktree, keep branch) vscleanup_session()(remove everything)previous_branchsupport for one-branch-per-agent invariantsetup_workspace_scratch()for no-context-paths case: git-inits workspace, creates branch + scratch--git-common-dirrelative path resolution for non-worktree reposChangeApplier (
massgen/filesystem_manager/_change_applier.py):.massgen_scratchfiles in fallback_apply_file_changes()Orchestrator (
massgen/orchestrator.py):_stream_agent_execution()with_agent_current_branchestrackingcleanup_session()at endSystem Prompts (
massgen/system_prompt_sections.py,massgen/system_message_builder.py):WorkspaceStructureSectionacceptsworktree_pathsparamDeprecation (
massgen/config_validator.py,massgen/agent_config.py,massgen/config_builder.py, 5 YAML configs):use_two_tier_workspacedeprecated with config validator warningswrite_mode: autoCLAUDE.md: Documented agent statelessness and anonymity principle
Test Plan
uv run pytest massgen/tests/test_write_mode_scratch.py -v(20 tests)uv run pytest massgen/tests/test_isolation_context.py -v(39 tests)uv run pytest massgen/tests/test_two_tier_workspace.py -v(15 tests)uv run python scripts/validate_all_configs.pyuv run massgen --config @basic/multi/three_agents_default "Create hello.py with tests"Documentation
docs/source/user_guide/agent_workspaces.rstCLAUDE.md(agent statelessness principle)MAS-269: Fix subagent timeout
Issue: Subagent spawning tools were timing out with the MCP client timeout (400s), even though subagents have their own internal timeout handling and can legitimately run longer.
Error observed:
Fix: Added timeout exemption for subagent-related tools in the MCP client.
Changes
massgen/mcp_tools/client.py:TIMEOUT_EXEMPT_TOOLSfrozenset containing tools that manage their own timeouts:spawn_subagents- Subagent spawning has its own timeout configurationget_subagent_status- May block waiting for subagent completioncancel_subagents- May need to wait for graceful shutdowncall_tool()method to skipasyncio.wait_for()wrapper for exempt toolsTest Plan
Closes MAS-269
Closes #852
Light Theme Visibility Fixes
Issue: Multiple UI elements were invisible or had poor contrast in light theme mode:
/themecommand didn't update all styled elementsRoot Cause: The CSS used Textual's built-in
$surface-lighten-1and$surface-lighten-2variables which compute to near-white colors in light mode, providing no contrast against the white background.Changes
Palette files (
_light.tcss,_dark.tcss):$mode-toggle-border- underline color for inactive toggles$mode-toggle-active-color- color for active toggle states$mode-toggle-inactive-color- color for inactive toggle states$separator-border- for horizontal divider lines$tool-card-border- for tool card left borders$toast-bg,$toast-border- theme-appropriate toast stylingBase CSS (
base.tcss):ModeTogglestyles to use semantic variables instead of$surface-lighten-2#input_headerandAgentStatusRibbonto use$separator-borderToolCallCard.terminal-toolto use$tool-card-border.theme-lightand.theme-darkCSS class rules for dynamic theme switchingborder-left: tallinstead of Textual's defaultouterborderPython (
textual_terminal_display.py):.theme-light/.theme-dark) on mount and theme switch/themeTest Plan
/themecommand, verify all elements update correctlyMAS-267: Quickstart Wizard Docker Setup
Issue: The TUI quickstart wizard lets users select "Docker Mode" but doesn't check if Docker is available or offer to pull the required images. Users end up with a config that references Docker but fails at runtime.
Fix: Integrated the existing
DockerSetupStepfrom the setup wizard into the quickstart wizard flow, shown conditionally when Docker mode is selected. Added an in-step "Pull Selected Images" button with animated progress so users can pull images directly within the wizard.Changes
massgen/frontend/displays/textual_widgets/quickstart_wizard.py:docker_setupwizard step afterexecution_mode, using existingDockerSetupStepskip_conditiononexecution_modestate)massgen/frontend/displays/textual_widgets/setup_wizard.py(DockerSetupStep):⠋⠙⠹⠸⠼⠴⠦⠧⠇⠏) on status label during pulldocker pullstdout line-by-line so users see layer download progress in real-timeasyncio.create_subprocess_execto avoid blocking the TUI event loopvalidate()that blocks advancing to next step while pull is in progressTest Plan
uv run massgen(launches TUI with quickstart wizard)Closes MAS-267
MAS-272: Worktree Isolation for File Writes
Issue: During final presentation, agents write directly to context paths, making changes immediately and irreversibly. Users have no opportunity to review or approve file changes before they're applied.
Fix: Added a full worktree isolation pipeline: agents write to a git worktree inside their workspace, a review modal shows the diff, and only approved changes are copied to the original context path.
Architecture
Changes
New files:
massgen/infrastructure/worktree_manager.py- Git worktree create/remove/list/prune via GitPythonmassgen/infrastructure/shadow_repo.py- Shadow repo for non-git directoriesmassgen/infrastructure/__init__.py- Exports WorktreeManager, ShadowRepo, is_git_repomassgen/filesystem_manager/_isolation_context_manager.py- Manages isolated write contexts (worktree or shadow mode), handles diff/change detectionmassgen/filesystem_manager/_change_applier.py- Applies approved changes from worktree back to original paths, with ReviewResult dataclassmassgen/frontend/displays/textual/widgets/modals/review_modal.py- Two-panel TUI modal: file list with click-to-toggle approval (✓/○) + syntax-highlighted diff viewer with green/red background tintingmassgen/utils/git_utils.py- Shared git utilities (get_git_root, get_changes, is_git_repo)massgen/tests/test_isolation_context.py- 39 unit tests covering worktree, shadow, change detection, PPM integrationmassgen/configs/debug/test_write_mode_isolation.yaml- Debug config for testing the isolation flowscripts/test_review_modal.py- Standalone test harness for iterating on modal design with theme toggleModified files:
massgen/agent_config.py- Addedwrite_modefield toCoordinationConfigmassgen/config_validator.py- AddedVALID_WRITE_MODESvalidationmassgen/api_params_handler/_api_params_handler_base.py- Excludedwrite_modefrom API passthroughmassgen/backend/base.py- Excludedwrite_modefrom API passthroughmassgen/cli.py- Parsewrite_modefrom coordination config in all code pathsmassgen/orchestrator.py:get_final_presentation(): creates IsolationContextManager, removes original paths from PPM, adds worktree info to presentation prompt_present_final_answer(): re-adds paths, calls_review_isolated_changes()_review_isolated_changes(): collects changes/diff, shows TUI modal, applies via ChangeApplierrepo_rootfor correct path resolution when context is a git subdirectorymassgen/filesystem_manager/__init__.py- Exports IsolationContextManager, ChangeApplier, ReviewResultmassgen/filesystem_manager/_filesystem_manager.py- Addedextra_mount_pathsto Docker container recreationmassgen/filesystem_manager/_docker_manager.py- Mount extra paths in containermassgen/filesystem_manager/_path_permission_manager.py- Addedremove_context_path()andre_add_context_path()for isolationmassgen/frontend/displays/textual_terminal_display.py:show_change_review_modal()with thread-safe Future-based cross-thread communicationMODAL_BASE_CSS(was missing, causing no borders in real app)massgen/frontend/displays/textual_themes/base.tcss- Full review modal CSS section with toggle indicators, diff panel, file list, instructionsmassgen/frontend/displays/textual_themes/palettes/_dark.tcss- Added$diff-add-bg,$diff-remove-bg,$modal-*variablesmassgen/frontend/displays/textual_themes/palettes/_light.tcss- Added matching light theme variablespyproject.toml- AddedgitpythondependencyYAML Config
Test Plan
uv run pytest massgen/tests/test_isolation_context.py -v(39 tests)uv run python scripts/test_review_modal.py- standalone modal test with theme toggleuv run massgen --config @debug/test_write_mode_isolation "Create hello.py"- full end-to-endCloses MAS-272
MAS-268: Disable post-evaluation restarts in quickstart defaults
Issue: TUI crashes on orchestration restart (shows attempt 2 banner then crashes).
Fix: Set
max_orchestration_restarts: 0in both Docker and local quickstart config templates inconfig_builder.py. Users who manually setmax_orchestration_restartsin their YAML are unaffected.Changes
massgen/config_builder.py- Changedmax_orchestration_restartsfrom2to0in both Docker and local quickstart templates, with comment referencing MAS-268Test Plan
max_orchestration_restarts: 0max_orchestration_restarts: 2still workRelates to MAS-268
Summary by CodeRabbit
Release Notes
New Features
Configuration Changes
Documentation