Conversation
2c1bfbf to
f0dd5d4
Compare
b0e1b2c to
4d5c75c
Compare
SummaryReading the diff end-to-end: Code referenceThe shell-free Git invocation is correctly scoped ( result = subprocess.run(
["git", *args],
cwd=str(cwd),
shell=False,
capture_output=True,
text=True,
timeout=timeout,
env=env,
)Path traversal guard inside the temp-index commit flow ( def _repo_rel(ctx: GitContext, workspace_rel: str) -> str:
try:
target = safe_resolve_ws(ctx.workspace, workspace_rel or ".")
except ValueError as exc:
raise GitWorkspaceError(str(exc), "path_outside_workspace") from exc
...The selected-files commit uses a private index ( fd, index_path = tempfile.mkstemp(prefix="hermes-webui-git-index-")
os.close(fd)
Path(index_path).unlink(missing_ok=True)
env = os.environ.copy()
env["GIT_INDEX_FILE"] = index_path
...
_run_git(ctx, ["add", "-A", "--", *specs], check=True, env=env)Diagnosis1. Scope (PR hygiene)
That also lets the maintainer reason about the backend surface separately from the UI choices. 2. CI not run
3. Git hooks execute under the WebUI process
Suggest at minimum: document this in the workspace Git docs, and consider an env scrub before invoking remote/commit ops ( 4. No coordination with active agent runsThe workspace mutation lock at The agent contract is owned by 5. Smaller observations
Test planI did not run the test suite — per cron policy I never execute PR code. But the 30 test cases in
RecommendationIf splitting isn't feasible at this stage, I'd at least ask for: (1) CI green, (2) the env scrub on subprocess Git, (3) an explicit gate against concurrent agent streams for destructive ops. The implementation is solid — this is mostly a scope concern. Nice work on the test coverage and the structured error codes. |
Phase 0 fit assessment — maintainer-review needed, scope split recommendedThanks for the substantial workspace Git surface, @stocky789. The implementation quality is genuinely high — porcelain v2 parsing, shell-free subprocess, structured error codes, path-traversal validation via But at +5390/-80 LOC across 19 files this is going to be very hard to land in its current shape, and I want to flag five concerns before more time gets invested: 1. Scope (Phase 0 Q5 + AGENTS.md)
Strongly suggest splitting:
That separation makes the backend trust-boundary discussion (below) reviewable independent of the UX choices. 2. CI not run
3. Git hooks execute under the WebUI process (trust boundary)
This is correct Git behavior, but for WebUI it's a notable trust-boundary expansion: before this PR, the worst a malicious workspace could do via the WebUI was render its files. After this PR, the workspace can run code on commit/pull via hooks. Suggested minimum mitigations:
4. No coordination with active agent runsThe per-repo mutation lock at The agent contract in 5. Smaller observations
RecommendationApply The implementation work is solid — this is mostly a project-fit and trust-surface question. Happy to coordinate on the slice plan if you'd like to split. |
Appreciate the feedback |
|
Thanks again for the review. I split the original PR into the backend only and made a few tweaks as I did spot a couple bugs. Backend draft PR: That PR contains the workspace Git backend/API work, backend tests, and It also includes the safety changes from your feedback:
The UI slice is separate on |
Closing — superseded by your split #2625 (backend) + the frontend draftThanks for splitting the original 5390-LOC workspace Git PR into focused slices @stocky789. Per your follow-up comment, the backend-only PR #2625 ships:
That's a much better merge target than the combined 20-file PR — backend can land first under feature-flag-off-by-default, frontend comes through the second PR once we've validated the backend in isolation. Closing this combined PR in favor of #2625 (backend) and the upcoming frontend draft. Branch will be deleted via the merge of #2625, no action needed on your end here. |
Thinking Path
What Changed
api/workspace_git.pyfor workspace-scoped Git operations:/api/git-infothrough the same status implementation..gitignorevisibility in the Files tab so ignored files/folders are labelledIgnoredwithout being offered as committable Changes entries.Why It Matters
This makes the workspace editor more complete: users can edit files, inspect Git state, review diffs, select exactly which files to commit, and run common remote actions from Hermes WebUI without switching to a terminal.
The selected-file commit path avoids accidentally committing unrelated staged files by committing through a temporary index, then resetting only the selected paths in the real index after success.
Scope
This PR is a larger cohesive feature PR because the workspace Git flow spans backend Git helpers, route wiring, frontend state, and diff preview UI.
Although the diff touches backend routes, frontend workspace UI, tests, and PR media, those changes support the same workspace source-control workflow. There are no dependency changes, framework changes, build-step changes, product-doc edits, changelog edits, or broad refactors.
Suggested Review Order
api/workspace_git.py— Git operation helpers, workspace/path scoping, temporary-index selected commits, branch/remote operations, and error classification.api/routes.py— HTTP route wiring and request/response handling for Git operations.static/workspace.js— workspace Git state, Changes tab, diff rendering, branch controls, selected-file commit flow, and refresh guards.static/ui.js,static/boot.js,static/index.html,static/style.css,static/i18n.js— panel shell, preview/back affordances, labels, and styling.tests/test_workspace_git.pyandtests/test_workspace_auto_refresh.py— regression coverage for the new behavior.docs/pr-media/workspace-git/— screenshot evidence for the workflow states.UI Media
Workflow overview with staged, unstaged, deleted, added, and untracked files:
Ignored, while the Changes tab still counts only committable changes:Verification
Automated:
git diff --check refs/remotes/upstream/master...HEAD.venv/bin/python -m py_compile api/routes.py api/workspace.py api/workspace_git.pynode --check static/workspace.js && node --check static/ui.js && node --check static/boot.js.venv/bin/python -m pytest tests/test_workspace_panel_session_list.py::TestWorkspacePanelCollapsePriority::test_container_query_hides_git_badge_first -q --timeout=60— 1 passed.venv/bin/python -m pytest tests/test_workspace_git.py tests/test_workspace_auto_refresh.py tests/test_issue2554_workspace_tree_file_indent.py -q --timeout=60— 40 passed.venv/bin/python -m pytest tests/ -q --timeout=60— 5933 passed, 47 skipped, 3 xpassed, 8 subtests passedRisks / Follow-ups
Model Used
AI-assisted.