Skip to content

Commit fb4bb87

Browse files
fix(otdf-sdk-mgr): resolve tests repo from CWD in orchestrate run (DSPX-3302)
run_tests_cell previously hardcoded TESTS_REPO = ~/Documents/GitHub/opentdf/tests, which caused [FAIL] tests: tests repo is on branch 'main', expected '<branch>' when orchestrate was invoked from a tests worktree (the canonical checkout typically stays on main while the user works from a worktree on the feature branch). Now both run_tests_cell and the dry-run path resolve the repo via git rev-parse --show-toplevel, so orchestrate works from any tests worktree on the cell branch. Wrong-branch errors are preserved with a clearer message pointing the user to run from a tests worktree on the feature branch. Tests added in test_orchestrate.py mock subprocess to cover both the success path (CWD-resolved repo pushes + opens draft PR) and the wrong-branch error. ruff format also normalized several pre-existing wrap points in cli_orchestrate.py / tests/test_schema_sync.py to fit within line-length=100; included here so the next commit isn't polluted by them. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent c7be3f3 commit fb4bb87

2 files changed

Lines changed: 137 additions & 26 deletions

File tree

otdf-sdk-mgr/src/otdf_sdk_mgr/cli_orchestrate.py

Lines changed: 49 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -89,9 +89,7 @@ def load_spec(path: Path) -> FeatureSpec:
8989
raise ValueError(f"{path}: repos.{key}.depends_on must be a list")
9090
repo_path = entry.get("path")
9191
if key != "tests" and not isinstance(repo_path, str):
92-
raise ValueError(
93-
f"{path}: repos.{key}.path is required for non-tests cells"
94-
)
92+
raise ValueError(f"{path}: repos.{key}.path is required for non-tests cells")
9593
cells[key] = Cell(
9694
key=key,
9795
path=repo_path,
@@ -120,9 +118,7 @@ def load_spec(path: Path) -> FeatureSpec:
120118
# ------------------------------------------------------------------ topo sort
121119

122120

123-
def topological_waves(
124-
cells: dict[str, Cell], *, skip: Iterable[str] = ()
125-
) -> list[list[Cell]]:
121+
def topological_waves(cells: dict[str, Cell], *, skip: Iterable[str] = ()) -> list[list[Cell]]:
126122
"""Group cells into dependency waves; cells within a wave are independent.
127123
128124
Skipped cells are treated as already-done (their dependents see them as
@@ -158,7 +154,6 @@ def topological_waves(
158154

159155
OPENTDF_ROOT = Path.home() / "Documents/GitHub/opentdf"
160156
WORKTREES_ROOT = Path.home() / "Documents/GitHub/worktrees"
161-
TESTS_REPO = OPENTDF_ROOT / "tests"
162157

163158

164159
def worktree_for(spec: FeatureSpec, cell: Cell) -> Path:
@@ -370,10 +365,14 @@ def run_cell(
370365
transcript = transcripts_dir / f"{spec.jira or spec.name}-{cell.key}.jsonl"
371366

372367
cmd = [
373-
"claude", "-p",
374-
"--model", model,
375-
"--permission-mode", "acceptEdits",
376-
"--output-format", "stream-json",
368+
"claude",
369+
"-p",
370+
"--model",
371+
model,
372+
"--permission-mode",
373+
"acceptEdits",
374+
"--output-format",
375+
"stream-json",
377376
"--verbose",
378377
build_prompt(spec, cell),
379378
]
@@ -403,13 +402,27 @@ def run_cell(
403402
def run_tests_cell(spec: FeatureSpec, cell: Cell) -> CellResult:
404403
"""Push the tests branch and open a draft PR without launching a subagent.
405404
405+
The tests repo is resolved from the orchestrator's CWD via
406+
`git rev-parse --show-toplevel` — orchestrate is meant to run from a tests
407+
worktree already on the feature branch, not from a hardcoded checkout.
408+
406409
feature-design already wrote the tests-side artifacts; this step makes them
407410
visible to downstream CI by pushing the branch and opening a PR. Commits
408411
were made by the user (signed normally), so deferred signing doesn't apply.
409412
"""
410-
repo = TESTS_REPO
411-
if not repo.is_dir():
412-
return CellResult(cell, Path(), Path(), False, None, f"tests repo not found: {repo}")
413+
try:
414+
repo = Path(
415+
subprocess.check_output(["git", "rev-parse", "--show-toplevel"], text=True).strip()
416+
)
417+
except subprocess.CalledProcessError as e:
418+
return CellResult(
419+
cell,
420+
Path(),
421+
Path(),
422+
False,
423+
None,
424+
f"could not resolve current git repo (run from a tests worktree): {e}",
425+
)
413426

414427
try:
415428
current = subprocess.check_output(
@@ -420,9 +433,13 @@ def run_tests_cell(spec: FeatureSpec, cell: Cell) -> CellResult:
420433

421434
if current != cell.branch:
422435
return CellResult(
423-
cell, Path(), Path(), False, None,
424-
f"tests repo is on branch '{current}', expected '{cell.branch}' — "
425-
"did feature-design run on this branch?",
436+
cell,
437+
Path(),
438+
Path(),
439+
False,
440+
None,
441+
f"current repo {repo} is on branch '{current}', expected '{cell.branch}' — "
442+
"run orchestrate from a tests worktree on the feature branch.",
426443
)
427444

428445
try:
@@ -478,12 +495,8 @@ def run(
478495
list[str] | None,
479496
typer.Option("--only", help="Only run these cell keys (repeatable)."),
480497
] = None,
481-
timeout_s: Annotated[
482-
int, typer.Option("--timeout", help="Per-cell timeout (seconds).")
483-
] = 1800,
484-
model: Annotated[
485-
str, typer.Option("--model", help="Sub-agent model alias.")
486-
] = "sonnet",
498+
timeout_s: Annotated[int, typer.Option("--timeout", help="Per-cell timeout (seconds).")] = 1800,
499+
model: Annotated[str, typer.Option("--model", help="Sub-agent model alias.")] = "sonnet",
487500
force: Annotated[
488501
bool,
489502
typer.Option("--force", help="Re-run cells even if they already have open PRs."),
@@ -523,11 +536,21 @@ def run(
523536

524537
if dry_run:
525538
typer.echo(f"Feature: {spec.name} ({spec.jira or 'no Jira'}) — {spec.title}")
539+
try:
540+
tests_repo: Path | None = Path(
541+
subprocess.check_output(["git", "rev-parse", "--show-toplevel"], text=True).strip()
542+
)
543+
except subprocess.CalledProcessError:
544+
tests_repo = None
526545
for i, wave in enumerate(waves, 1):
527546
typer.echo(f" Wave {i}:")
528547
for cell in wave:
529548
if cell.key == "tests":
530-
existing = check_existing_pr(TESTS_REPO, cell.branch)
549+
existing = (
550+
check_existing_pr(tests_repo, cell.branch)
551+
if tests_repo is not None
552+
else None
553+
)
531554
pr_note = f" [PR EXISTS: {existing}]" if existing else ""
532555
typer.echo(
533556
f" - {cell.key}: path=(tests repo) branch={cell.branch}"
@@ -560,7 +583,8 @@ def _dispatch(c: Cell) -> CellResult:
560583
if c.key == "tests":
561584
return run_tests_cell(spec, c)
562585
return run_cell(
563-
spec, c,
586+
spec,
587+
c,
564588
transcripts_dir=transcripts_dir,
565589
timeout_s=timeout_s,
566590
model=model,

otdf-sdk-mgr/tests/test_orchestrate.py

Lines changed: 88 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,13 @@
77

88
from __future__ import annotations
99

10+
import subprocess
1011
from pathlib import Path
1112

1213
import pytest
14+
from otdf_sdk_mgr import cli_orchestrate
1315
from otdf_sdk_mgr.cli_orchestrate import (
1416
Cell,
15-
FeatureSpec,
1617
load_spec,
1718
topological_waves,
1819
worktree_for,
@@ -184,3 +185,89 @@ def test_worktree_for_falls_back_to_name_when_no_jira(tmp_path: Path) -> None:
184185
spec = load_spec(_write_spec(tmp_path, body))
185186
wt = worktree_for(spec, spec.cells["platform-proto"])
186187
assert wt.name == "ad_hoc-platform-proto"
188+
189+
190+
# ------------------------------------------------------------ run_tests_cell
191+
#
192+
# These tests mock subprocess to verify that run_tests_cell resolves the tests
193+
# repo from the current working directory (via `git rev-parse --show-toplevel`)
194+
# rather than from a hardcoded path. The orchestrator is invoked from a tests
195+
# worktree on the feature branch, not from the canonical tests checkout.
196+
197+
198+
def test_run_tests_cell_resolves_repo_from_cwd(
199+
monkeypatch: pytest.MonkeyPatch, tmp_path: Path
200+
) -> None:
201+
fake_repo = tmp_path / "worktrees" / "TEST-1-tests" / "tests"
202+
fake_repo.mkdir(parents=True)
203+
204+
check_output_calls: list[list[str]] = []
205+
check_call_calls: list[list[str]] = []
206+
run_calls: list[list[str]] = []
207+
208+
def fake_check_output(args: list[str], text: bool = False, **kwargs: object) -> str:
209+
check_output_calls.append(list(args))
210+
if list(args[:3]) == ["git", "rev-parse", "--show-toplevel"]:
211+
return f"{fake_repo}\n"
212+
if "branch" in args and "--show-current" in args:
213+
return "TEST-1-tdd\n"
214+
if args[0] == "gh" and "pr" in args and "create" in args:
215+
return "Draft PR created: https://github.com/org/repo/pull/42\n"
216+
raise AssertionError(f"unexpected check_output args: {args}")
217+
218+
def fake_check_call(args: list[str], **kwargs: object) -> int:
219+
check_call_calls.append(list(args))
220+
return 0
221+
222+
def fake_run(args: list[str], **kwargs: object) -> subprocess.CompletedProcess[str]:
223+
run_calls.append(list(args))
224+
# gh pr list — return no existing PR.
225+
return subprocess.CompletedProcess(args, 0, stdout="", stderr="")
226+
227+
monkeypatch.setattr(cli_orchestrate.subprocess, "check_output", fake_check_output)
228+
monkeypatch.setattr(cli_orchestrate.subprocess, "check_call", fake_check_call)
229+
monkeypatch.setattr(cli_orchestrate.subprocess, "run", fake_run)
230+
231+
spec = load_spec(_write_spec(tmp_path, _minimal_spec_yaml()))
232+
result = cli_orchestrate.run_tests_cell(spec, spec.cells["tests"])
233+
234+
assert result.success is True, f"expected success, got error={result.error}"
235+
assert result.pr_url == "https://github.com/org/repo/pull/42"
236+
237+
# All git/gh interactions must target the CWD-resolved repo, not TESTS_REPO.
238+
fake_repo_str = str(fake_repo)
239+
branch_check = ["git", "-C", fake_repo_str, "branch", "--show-current"]
240+
push = ["git", "-C", fake_repo_str, "push", "-u", "origin", "TEST-1-tdd"]
241+
assert branch_check in check_output_calls
242+
assert push in check_call_calls
243+
# gh subprocess.run / check_output calls run with cwd=fake_repo (not asserted
244+
# explicitly — fake_run/fake_check_output don't capture cwd — but the
245+
# branch check + push above prove the resolved path flows through).
246+
247+
248+
def test_run_tests_cell_errors_on_wrong_branch(
249+
monkeypatch: pytest.MonkeyPatch, tmp_path: Path
250+
) -> None:
251+
fake_repo = tmp_path / "opentdf" / "tests"
252+
fake_repo.mkdir(parents=True)
253+
254+
def fake_check_output(args: list[str], text: bool = False, **kwargs: object) -> str:
255+
if list(args[:3]) == ["git", "rev-parse", "--show-toplevel"]:
256+
return f"{fake_repo}\n"
257+
if "branch" in args and "--show-current" in args:
258+
return "main\n"
259+
raise AssertionError(f"unexpected check_output args: {args}")
260+
261+
def fail_call(args: list[str], **kwargs: object) -> int:
262+
raise AssertionError(f"push must not happen on wrong branch: {args}")
263+
264+
monkeypatch.setattr(cli_orchestrate.subprocess, "check_output", fake_check_output)
265+
monkeypatch.setattr(cli_orchestrate.subprocess, "check_call", fail_call)
266+
267+
spec = load_spec(_write_spec(tmp_path, _minimal_spec_yaml()))
268+
result = cli_orchestrate.run_tests_cell(spec, spec.cells["tests"])
269+
270+
assert result.success is False
271+
assert result.error is not None
272+
assert "TEST-1-tdd" in result.error
273+
assert "main" in result.error

0 commit comments

Comments
 (0)