fix(run): guard home dir in _detect_project_root_from_seed_path#1313
Conversation
…1312) Seeds stored in the global ~/.ouroboros/seeds/ store structurally match the central-seed pattern (.ouroboros/seeds ancestry), causing _detect_project_root_from_seed_path to return ~ as the candidate project root. _resolve_cli_project_dir then passed ~ to OpenCodeRuntime as cwd, so opencode always started with cwd=$HOME instead of the actual project. Add a guard that returns None when the detected candidate equals Path.home(), treating globally-stored seeds as non-central seeds and falling back to the existing resolution chain in the caller. Fixes Q00#1312 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…letion On Windows, opencode (Node.js) spawns child processes such as a session server that outlive the parent subprocess. When execute_task completes normally, process_finished=True causes the finally block to skip _terminate_process, leaving the children as zombies. With N ACs, N orphaned opencode processes accumulate. Windows preserves the original PPID on orphaned processes (unlike Linux which re-parents to init), so we can find and terminate them by PPID even after the parent has already exited. Add a Windows-only wmic call in the finally block that runs on both success and error paths. Fixes Q00#1314 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Maintainer review: the What looks correct:
Changes requested before merge:
Disposition:
|
![ouroboros-agent[bot]](https://avatars.githubusercontent.com/in/3091302?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|---|
| PR | #1313 |
| HEAD checked | 03b1f600ba504da7a0314b95e65e51cdf56c671d |
| Request ID | manual-open-pr-review-Q00_ouroboros-1313-03b1f60 |
| Review record | 6ecc9a89-d0c3-4913-95cb-3dca8599eadd |
What Improved
_detect_project_root_from_seed_path()now avoids treating~/.ouroboros/seeds/...as a project-local central seed root, preventingPath.home()from being returned directly for global seed storage.
Issue Requirements
| Requirement | Status |
|---|---|
Prevent global ~/.ouroboros/seeds/... from being treated as a project-local central seed root |
Partially met |
Preserve resolver precedence for explicit --project-dir, raw metadata project_dir / working_directory, and brownfield_context.target_dir |
Partially met |
| Keep this PR scoped to the #1312 cwd-resolution fix | Not met |
Prior Findings Status
Human maintainer concerns are still present in current HEAD: the unrelated opencode_runtime.py change remains, and the #1312 coverage is still helper-level rather than resolver-level. No prior bot findings were copied.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/orchestrator/opencode_runtime.py:1296 | BLOCKING | This PR still changes the OpenCode Windows process cleanup path even though the scoped #1312 fix is the CLI seed cwd resolver. The new os.name == "nt" branch shells out to wmic ... delete from the async runtime finally block and has no targeted test coverage in tests/unit/orchestrator/test_opencode_runtime.py for normal completion, cancellation, missing wmic, or child-process cleanup behavior. This is a separate runtime boundary with non-obvious failure modes and should be removed from this PR or covered in its own #1314-focused change. |
| 2 | tests/unit/cli/test_run_qa.py:697 | BLOCKING | The new regression test only covers _detect_project_root_from_seed_path() returning None, but the production caller is _resolve_cli_project_dir() at src/ouroboros/cli/commands/run.py:576. Because _resolve_cli_project_dir() applies metadata, brownfield target, context-reference, and fallback logic after helper detection, the PR does not prove the actual run cwd boundary for a seed under ~/.ouroboros/seeds/... no longer resolves to Path.home() while preserving explicit --project-dir, raw metadata, and brownfield precedence. Add resolver-level regression coverage for the #1312 path. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| 1 | /tmp/pr_body_Q00_ouroboros_1313.md:1 | Medium | High | The PR body is empty. Add a narrow description that states this fixes the global seed-store HOME misclassification, not DB-registered brownfield default cwd behavior. |
Non-blocking Suggestions
| 1 | src/ouroboros/cli/commands/run.py:193 | Documentation | The inline comment is useful, but the function docstring still only describes central project seeds and does not explicitly mention the global seed-store exclusion. |
Test Coverage Notes
- Reviewed changed tests in
tests/unit/cli/test_run_qa.pyand existing OpenCode runtime tests. - Could not execute tests:
/root/.local/bin/uvis absent, nouvis on PATH, and systempython3 -m pytestreportsNo module named pytest.
Design Notes
The run.py helper change is directionally sound, but the PR mixes a CLI resolver fix with an unrelated OpenCode runtime cleanup. The resolver boundary needs caller-level evidence before merge.
Design / Roadmap Gate
Affected boundaries are CLI cwd resolution for global versus project-local seed stores, and OpenCode subprocess lifecycle cleanup on Windows. The former needs resolver-level replay coverage because helper output is only one input to runtime cwd selection. The latter is a separate async runtime/process boundary and does not belong in a narrow cwd-resolution PR without its own tests.
Directional Notes
Maintainer memory emphasized conservative runtime and persistence boundaries. I treated the OpenCode subprocess cleanup as a critical runtime boundary and required current-source evidence rather than accepting it as incidental.
Test Coverage
- Reviewed changed tests in
tests/unit/cli/test_run_qa.pyand existing OpenCode runtime tests. - Could not execute tests:
/root/.local/bin/uvis absent, nouvis on PATH, and systempython3 -m pytestreportsNo module named pytest.
Merge Recommendation
Do not merge yet. Remove the unrelated OpenCode runtime change or move it to its own tested PR, and add _resolve_cli_project_dir() regression coverage for global ~/.ouroboros/seeds seeds and the documented precedence cases.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 03b1f60
request_id: manual-open-pr-review-Q00_ouroboros-1313-03b1f60
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Remove the unrelated OpenCode Windows process cleanup from the Q00#1312 branch and add caller-level resolver coverage so the runtime cwd boundary is proven directly, not only through the helper. Constraint: Q00#1312 is scoped to global ~/.ouroboros/seeds HOME misclassification; Q00#1314 owns OpenCode orphan-process cleanup. Rejected: keeping the Windows OpenCode cleanup in this PR | it crosses into a separate async runtime/process boundary without targeted coverage. Confidence: high Scope-risk: narrow Directive: Do not expand this PR into DB brownfield-default cwd semantics; preserve --project-dir, raw metadata, and brownfield_context.target_dir precedence. Tested: uv run pytest tests/unit/cli/test_run_qa.py -q; uv run ruff check src/ouroboros/cli/commands/run.py tests/unit/cli/test_run_qa.py src/ouroboros/orchestrator/opencode_runtime.py; uv run ruff format --check src/ouroboros/cli/commands/run.py tests/unit/cli/test_run_qa.py src/ouroboros/orchestrator/opencode_runtime.py Not-tested: live Windows opencode runtime; DB-registered brownfield default cwd semantics intentionally out of scope
|
@ouroboros-agent please re-review PR #1313 at current HEAD The current head addresses the prior blockers:
Local verification:
Please provide an updated formal verdict including #961 SSOT / run-path direction alignment and explicit APPROVE or REQUEST_CHANGES. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Metadata
| Field | Value |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|---|
| PR | #1313 |
| HEAD checked | 29e50f9abba62f6b2e210d603ed4e618af525589 |
| Request ID | req_1780363382_9 |
| Review record | 8e200a4e-70d4-4c1e-ac2d-facfaf025a6a |
What Improved
- Prevents globally stored seeds under
~/.ouroboros/seeds/...from being misclassified as project-local central seeds withPath.home()as the runtime cwd. - Adds resolver-level regression coverage for the global seed-store case and precedence preservation.
Issue Requirements
| Requirement | Status |
|---|---|
Prevent global ~/.ouroboros/seeds/... from resolving runtime cwd to Path.home() |
Met |
Preserve explicit --project-dir precedence |
Met |
Preserve raw metadata project_dir / working_directory precedence |
Met |
Preserve brownfield_context.target_dir precedence |
Met |
| Remove unrelated Windows orphan-process cleanup from this PR | Met |
| PR body material claims | N/A |
Prior Findings Status
Prior concerns are resolved or withdrawn against current HEAD evidence. The unrelated opencode_runtime.py change is no longer in the authoritative changed-files list. Resolver-level coverage was added. The current PR body artifact contains no broad DB brownfield-default cwd claim to re-raise.
Blockers
No in-scope blocking findings remained after policy filtering.
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
| None. | | | |
Test Coverage Notes
- Reviewed resolver tests in
tests/unit/cli/test_run_qa.py, including global seed-store fallback, explicit--project-dir, raw metadataproject_dir, brownfieldtarget_dir, central seed root detection, and non-central seed behavior. - Could not execute tests in this environment:
/root/.local/bin/uvis missing,pythonis missing, andpython3 -m pytestreportsNo module named pytest.
Design Notes
The fix is narrow and aligned with the run-path SSOT direction: _resolve_cli_project_dir() remains the single CLI cwd boundary, while _detect_project_root_from_seed_path() only excludes the global home seed store from central-seed root detection.
Design / Roadmap Gate
The changed boundary feeds runtime cwd, task workspace creation, agent runtime cwd, and QA artifact cwd. Current source keeps explicit project-dir first, raw metadata next, then brownfield target dir / seed-derived resolution. Returning None for ~/.ouroboros/seeds causes global seeds without hints to fall back to the seed directory, not $HOME, while project-local <root>/.ouroboros/seeds still resolves to <root>.
Directional Notes
Review focused on cwd resolution, seed path trust boundaries, central vs global seed-store distinction, and preserving existing #1160/#1161 precedence. Maintainer memory was advisory only; no blocker was raised without current source evidence.
Test Coverage
- Reviewed resolver tests in
tests/unit/cli/test_run_qa.py, including global seed-store fallback, explicit--project-dir, raw metadataproject_dir, brownfieldtarget_dir, central seed root detection, and non-central seed behavior. - Could not execute tests in this environment:
/root/.local/bin/uvis missing,pythonis missing, andpython3 -m pytestreportsNo module named pytest.
Merge Recommendation
Approve. I found no blocking runtime, persistence, replay, or contract regression in the changed resolver boundary. Test execution was unavailable locally, but the added tests cover the critical cases for this narrow fix.
Review-Metadata:
verdict: APPROVE
head_sha: 29e50f9
request_id: req_1780363382_9
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Maintainer merge-readiness note: I rechecked this PR against #961 and the accepted What changed after review:
Why this is aligned with #961 / run-path direction:
Why I consider it mergeable:
Verdict: the PR has been improved to a mergeable narrow cwd-resolution fix and is OK to merge. |
PR Review SummaryVerdictApprove Scope Reviewed
Blocking IssuesNone. WarningsNone. Mutation-Test Thinking
Complexity / CRAP-style Risk
Test Quality Assessment (6/7)
Security / Operational RiskNone. The change reduces path-boundary risk by refusing to treat the user's home directory as an inferred project root for global seed storage. It does not introduce shelling out, persistence migration, or runtime lifecycle changes. Looks Good
Final RecommendationApprove. The current head is narrow, tested at the correct resolver boundary, aligned with #961/run-path direction, and merge-ready. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Metadata
| Field | Value |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|---|
| PR | #1313 |
| HEAD checked | 29e50f9abba62f6b2e210d603ed4e618af525589 |
| Request ID | req_1780363883_16 |
| Review record | 80279d83-3783-44aa-aa7b-41198abf6295 |
What Improved
- Prevents global
~/.ouroboros/seeds/...paths from being mistaken for project-local central seeds with$HOMEas the runtime cwd. - Adds resolver-level regression coverage for global seed-store fallback and precedence of explicit project dir, raw metadata project dir, and brownfield target dir.
Issue Requirements
| Requirement | Status |
|---|---|
Global ~/.ouroboros/seeds/... must not resolve runtime project cwd to Path.home() by matching the project-local .ouroboros/seeds ancestry pattern |
Met |
Preserve existing precedence for explicit --project-dir, raw metadata project_dir / working_directory, and brownfield_context.target_dir |
Met |
| Do not add DB-registered brownfield-default cwd semantics in this narrow fix | Met |
Prior Findings Status
Prior scope blockers are withdrawn for current HEAD: the authoritative changed-files list contains only src/ouroboros/cli/commands/run.py and tests/unit/cli/test_run_qa.py, and the earlier unrelated runtime file is no longer in scope. Prior approval metadata was noted but not copied; this review rechecked the current diff and source.
Blockers
No in-scope blocking findings remained after policy filtering.
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
- Reviewed
tests/unit/cli/test_run_qa.pycoverage for_resolve_cli_project_dir()and_detect_project_root_from_seed_path(). - Attempted
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest tests/unit/cli/test_run_qa.py -q, but/root/.local/bin/uvis unavailable. - Attempted
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 python3 -m pytest tests/unit/cli/test_run_qa.py -q, butpytestis not installed. - Human comment reports local
uv run pytest tests/unit/cli/test_run_qa.py -qpassed with39 passed; I did not rely on that as direct execution evidence.
Design Notes
The change is narrow and fits the existing resolver design: central-seed detection still identifies project-local .ouroboros/seeds, while the global seed store is excluded from project-root inference.
Design / Roadmap Gate
Affected boundary is _resolve_cli_project_dir() through _detect_project_root_from_seed_path(). The new guard prevents Path.home() from crossing from global storage into runtime project cwd, while explicit operator override and seed-declared project hints retain precedence. No persistence, replay, lifecycle, or plugin contract changes are introduced.
Directional Notes
Review focus followed the run-path SSOT concern: runtime cwd selection feeds task workspace creation, agent backend cwd, and verification artifact location. Maintainer memory was treated as advisory only; no blocker was raised without current source evidence.
Test Coverage
- Reviewed
tests/unit/cli/test_run_qa.pycoverage for_resolve_cli_project_dir()and_detect_project_root_from_seed_path(). - Attempted
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest tests/unit/cli/test_run_qa.py -q, but/root/.local/bin/uvis unavailable. - Attempted
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 python3 -m pytest tests/unit/cli/test_run_qa.py -q, butpytestis not installed. - Human comment reports local
uv run pytest tests/unit/cli/test_run_qa.py -qpassed with39 passed; I did not rely on that as direct execution evidence.
Merge Recommendation
Approve. I found no current-HEAD blocking issues in the changed boundary. Test execution was limited by missing local tooling, but the added tests cover the critical resolver regression and precedence paths.
Review-Metadata:
verdict: APPROVE
head_sha: 29e50f9
request_id: req_1780363883_16
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
2 participants
Fixes #1312.
This PR narrows the #1312 fix to the confirmed cwd-resolution bug:
~/.ouroboros/seeds/...structurally matches the project-local<project>/.ouroboros/seeds/...pattern._detect_project_root_from_seed_path()could therefore returnPath.home()as a false project root._resolve_cli_project_dir()could then pass$HOMEas the runtime cwd for globally stored seeds.Fix
Treat the global seed store as not being a project-local central seed store:
The caller then uses the existing resolution chain instead of accepting
$HOMEas the project root.Scope boundaries
This PR intentionally does not implement DB-registered brownfield default repo cwd semantics. The accepted run-path precedence remains:
--project-dir;metadata.project_dir/metadata.working_directory;brownfield_context.target_dir;The unrelated OpenCode Windows child-process cleanup was removed from this PR; that belongs to #1314 or another dedicated runtime-process PR.
Tests