Skip to content

Commit 396404a

Browse files
randleeclaude
andcommitted
fix(sc-gh-stack): resolve verification-pass findings from Opus review
- major: dry-run preview now uses the SAME freshness classifier as execution (_freshness_verdict shared by chain and plan_chain), so a re-run preview reports skip, never a false refuse_diverged that could lure a caller into rebasing an already-converted layer onto stale remote state; the plan stops at the first refusal instead of printing speculative entries; regression test pins preview==execution agreement on re-runs - minor: agent dry_run now skips worktree creation entirely and runs the script against repo_root — a preview creates nothing - minor: preflight distinguishes PREFLIGHT.GIT_MISSING (rc 127) from PREFLIGHT.NOT_A_REPO and adds a git_cli check symmetric with gh_cli - minor: CHANGELOG/README caught up (--dry-run, Standard envelope, *_RISKY codes, rc-127 envelope guarantee, 60 tests) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 85ea754 commit 396404a

7 files changed

Lines changed: 78 additions & 20 deletions

File tree

packages/sc-gh-stack/CHANGELOG.md

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,18 @@ Orchestration release: background agents over the deterministic scripts.
2828
and `playbook-landing.md` were folded into existing routes instead of shipping
2929
separately (new stack = `playbook-graph-to-stacks.md` step 3; daily loop =
3030
`playbook-sync.md`; landing = `gh stack merge --yes` via SKILL.md).
31-
- 55 pytest cases.
31+
- `gh_stack_convert.py --dry-run` (alias `--validate`): previews the per-layer
32+
plan (skip / rebase / fast-forward / refuse) using the same freshness
33+
classifier the executor uses, mutating nothing; with preflight this is the
34+
package's `--validate` surface (`--auto-fix` is deliberately replaced by the
35+
trivial-conflict rubric).
36+
- Convert and sync agents use the Standard envelope (`canceled`, `aborted_by`,
37+
`metadata.*`); a risky-conflict stop is `canceled: true, aborted_by: "policy"`
38+
with `CONVERT.CONFLICT_RISKY` / `SYNC.CONFLICT_RISKY`, `recoverable: false`.
39+
- Scripts survive missing binaries (synthetic rc-127 results — always a fenced
40+
envelope, never a traceback; `PREFLIGHT.GIT_MISSING` distinct from
41+
`PREFLIGHT.NOT_A_REPO`).
42+
- 60 pytest cases.
3243

3344
## 0.1.0 — 2026-08-28
3445

packages/sc-gh-stack/README.md

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,10 @@ Invoked by the skill via the Task tool (`run_in_background: true`), one per stac
3636
(all-or-nothing: conflicts restore every branch), same conflict rubric,
3737
verifies layers above a fix contain it.
3838

39-
Every report carries per-branch `before`/`after` SHAs and a `pushed` flag.
39+
Convert and sync report with the Standard envelope (`canceled`/`aborted_by`/
40+
`metadata`): every report carries per-branch `before`/`after` SHAs and a
41+
`pushed` flag, and a risky-conflict stop comes back as `canceled: true,
42+
aborted_by: "policy"` (`*_RISKY` error codes) — a hold for review, not a failure.
4043

4144
## Scripts
4245

@@ -49,6 +52,7 @@ Stdlib-only Python 3; every run emits one fenced JSON envelope (`success`/`data`
4952
idempotent on re-run; refuses dirty trees, in-progress rebases, and diverged
5053
branches; fast-forwards branches merely behind; linearises trunk-merge
5154
layers; then `gh stack init`. Exit 5 on bad input or refused state.
55+
`--dry-run` (alias `--validate`) previews the per-layer plan, mutating nothing.
5256
- `scripts/gh_stack_sync.py` — wraps `gh stack sync` with the same guards and
5357
envelope; exit 3 on conflict (all branches restored).
5458
- `scripts/gh_stack_shared.py` — git/gh wrappers and the envelope.

packages/sc-gh-stack/agents/sc-stack-convert.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,10 @@ Complete the task quickly; report only decisions and discrepancies.
3131
- **worktree** (optional): worktree path; default `<repo_root>-worktrees/stack/<bottom-slug>`
3232
where `<bottom-slug>` is the resolved bottom branch name with `/` replaced by `-`
3333
- **push** (optional, default `true`): run `gh stack submit --auto` when fully clean
34-
- **dry_run** (optional, default `false`): pass `--dry-run` to the convert script and return
35-
its per-layer plan without executing anything (skip steps 6–7)
34+
- **dry_run** (optional, default `false`): preview only — skip the worktree steps (2–3) and
35+
the execution steps (6–7), and run the convert script with `--dry-run` directly against
36+
`--cwd <repo_root>`; nothing is created or mutated (the fetch updates remote-tracking refs
37+
only). Return the script's per-layer plan.
3638

3739
## Execution
3840

packages/sc-gh-stack/scripts/gh_stack_convert.py

Lines changed: 29 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -144,18 +144,20 @@ def begin_conversion(trunk: str, layers: List[str], cwd: Optional[Path] = None)
144144
gs.git(["config", "sc-gh-stack.conversion", conv_id], cwd=cwd)
145145

146146

147-
def _check_remote_freshness(layer: str, remote: str, cwd: Optional[Path]) -> Optional[Dict[str, Any]]:
148-
"""Refuse to chain a local branch missing remote commits; ff if merely behind.
147+
def _freshness_verdict(layer: str, remote: str, cwd: Optional[Path]) -> str:
148+
"""Pure classifier shared by execution (chain) and preview (plan_chain):
149+
"ok" | "fast_forward" | "diverged".
149150
150151
A layer already adopted by this conversion (orig ref recorded and the branch
151152
tip has moved off it) is judged against its recorded pre-rebase tip instead:
152153
the rebase rewrote local history, so plain ancestry against the remote no
153154
longer means anything — but any commit the remote gained AFTER adoption
154155
shows up as the remote tip no longer being an ancestor of the recorded tip.
155-
Returns a failure dict, or None when the layer is safe to chain.
156156
"""
157+
if not gs.local_branch_exists(layer, cwd=cwd):
158+
return "ok" # will be created tracking the remote tip
157159
if not gs.remote_branch_exists(remote, layer, cwd=cwd):
158-
return None
160+
return "ok"
159161
remote_ref = f"{remote}/{layer}"
160162
orig = _orig_tip(layer, cwd=cwd)
161163
adopted = orig is not None and orig != _rev(_head(layer), cwd=cwd)
@@ -166,10 +168,23 @@ def _check_remote_freshness(layer: str, remote: str, cwd: Optional[Path]) -> Opt
166168
# (`git rebase <remote>/<layer>`), both of which must pass.
167169
if gs.is_ancestor(remote_ref, orig, cwd=cwd) \
168170
or gs.is_ancestor(remote_ref, _head(layer), cwd=cwd):
169-
return None
170-
elif gs.is_ancestor(remote_ref, _head(layer), cwd=cwd):
171+
return "ok"
172+
return "diverged"
173+
if gs.is_ancestor(remote_ref, _head(layer), cwd=cwd):
174+
return "ok"
175+
if gs.is_ancestor(_head(layer), remote_ref, cwd=cwd):
176+
return "fast_forward"
177+
return "diverged"
178+
179+
180+
def _check_remote_freshness(layer: str, remote: str, cwd: Optional[Path]) -> Optional[Dict[str, Any]]:
181+
"""Refuse to chain a local branch missing remote commits; ff if merely behind.
182+
Returns a failure dict, or None when the layer is safe to chain."""
183+
verdict = _freshness_verdict(layer, remote, cwd)
184+
if verdict == "ok":
171185
return None
172-
elif gs.is_ancestor(_head(layer), remote_ref, cwd=cwd):
186+
remote_ref = f"{remote}/{layer}"
187+
if verdict == "fast_forward":
173188
ff = _fast_forward(layer, remote_ref, cwd=cwd)
174189
if ff.returncode == 0:
175190
return None
@@ -285,13 +300,13 @@ def plan_chain(layers: List[str], trunk_ref: str, remote: str,
285300
else f"refs/remotes/{remote}/{layer}"
286301
entry: Dict[str, Any] = {"branch": layer,
287302
"onto": below if below_name is None else below_name}
288-
remote_ref = f"{remote}/{layer}"
289-
if gs.remote_branch_exists(remote, layer, cwd=cwd) \
290-
and not gs.is_ancestor(remote_ref, layer_ref, cwd=cwd):
291-
if gs.is_ancestor(layer_ref, remote_ref, cwd=cwd):
292-
entry["action"] = "fast_forward_then_rebase"
293-
else:
294-
entry["action"] = "refuse_diverged"
303+
verdict = _freshness_verdict(layer, remote, cwd) # same classifier chain() uses
304+
if verdict == "diverged":
305+
entry["action"] = "refuse_diverged"
306+
planned.append(entry)
307+
break # chain() stops here too; later entries would be speculative
308+
if verdict == "fast_forward":
309+
entry["action"] = "fast_forward_then_rebase"
295310
planned.append(entry)
296311
below, below_name = layer_ref, layer
297312
continue

packages/sc-gh-stack/scripts/gh_stack_preflight.py

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ def run_checks(cwd: Optional[Path] = None) -> List[Dict[str, Any]]:
3232
"""All checks, in order. Pure function of the environment; no side effects."""
3333
checks: List[Dict[str, Any]] = []
3434

35+
checks.append(_check("git_cli", gs.git(["--version"], cwd=cwd).returncode == 0, INSTALL_DOC))
3536
checks.append(_check("gh_cli", gs.gh(["--version"], cwd=cwd).returncode == 0, INSTALL_DOC))
3637
ext = gs.gh(["extension", "list"], cwd=cwd)
3738
checks.append(_check("gh_stack_extension",
@@ -74,7 +75,13 @@ def main(argv: Optional[List[str]] = None) -> int:
7475
parser.add_argument("--cwd", type=Path, default=None, help="repository path (default: current dir)")
7576
args = parser.parse_args(argv)
7677

77-
if not gs.in_git_repo(cwd=args.cwd):
78+
probe = gs.git(["rev-parse", "--is-inside-work-tree"], cwd=args.cwd)
79+
if probe.returncode == 127:
80+
gs.emit(gs.envelope(False, None, gs.error_obj(
81+
"PREFLIGHT.GIT_MISSING", "git is not on PATH", False,
82+
f"install git or fix PATH (probe loop in SKILL.md Step 1); {INSTALL_DOC}")))
83+
return 1
84+
if probe.returncode != 0:
7885
gs.emit(gs.envelope(False, None, gs.error_obj(
7986
"PREFLIGHT.NOT_A_REPO", "not inside a git repository", False, "cd into the repository")))
8087
return 1

packages/sc-gh-stack/tests/test_convert.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -498,6 +498,16 @@ def test_dry_run_previews_without_mutating(self, repo):
498498
assert gs.config_get("rerere.enabled", cwd=repo) == ""
499499
assert _orig_refs(repo) == ""
500500

501+
def test_dry_run_agrees_with_execution_on_rerun(self, repo):
502+
"""The preview must make the SAME decision the executor makes: after a
503+
successful conversion, --dry-run reports skip (never refuse_diverged)
504+
for every already-chained layer."""
505+
code, env = cv.convert("main", ["pr1", "pr3"], cwd=repo)
506+
assert code == cv.EXIT_OK, env
507+
code, env = cv.convert("main", ["pr1", "pr3"], cwd=repo, dry_run=True)
508+
assert code == cv.EXIT_OK, env
509+
assert [p["action"] for p in env["data"]["planned"]] == ["skip", "skip"]
510+
501511
def test_pr_number_resolution_uses_gh_stub(self, repo):
502512
# No patching: the PATH gh stub answers `gh pr view <n> --json headRefName -q .headRefName`.
503513
code, env = cv.convert("main", ["1", "3"], cwd=repo)

packages/sc-gh-stack/tests/test_preflight.py

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ def test_preflight_emits_envelope_when_no_binaries_on_path(self, monkeypatch, ca
6363
out = capsys.readouterr().out
6464
payload = json.loads(out.split("```json")[1].split("```")[0])
6565
assert payload["success"] is False
66+
assert payload["error"]["code"] == "PREFLIGHT.GIT_MISSING"
6667

6768
def test_envelope_error_shape(self):
6869
env = gs.envelope(False, None, gs.error_obj("X.Y", "m", True, "do it"))
@@ -78,6 +79,7 @@ def _healthy_env(monkeypatch, **overrides):
7879
}
7980
gh_map.update(overrides.pop("gh", {}))
8081
monkeypatch.setattr(gs, "gh", lambda args, cwd=None: gh_map[tuple(args)])
82+
monkeypatch.setattr(gs, "git", lambda args, cwd=None: cp(0, "git version 2.44.0"))
8183
cfg = {"rerere.enabled": "true", "remote.pushDefault": ""}
8284
cfg.update(overrides.pop("config", {}))
8385
monkeypatch.setattr(gs, "config_get", lambda key, cwd=None: cfg.get(key, ""))
@@ -146,6 +148,13 @@ def test_main_reports_failed_names_and_exit_1(self, monkeypatch, capsys):
146148
assert payload["data"]["failed"] == ["working_tree_clean"]
147149

148150
def test_main_outside_repo(self, monkeypatch, capsys):
149-
monkeypatch.setattr(gs, "in_git_repo", lambda cwd=None: False)
151+
monkeypatch.setattr(gs, "git", lambda args, cwd=None: cp(128, "", "not a git repository"))
150152
assert pf.main([]) == 1
151153
assert "PREFLIGHT.NOT_A_REPO" in capsys.readouterr().out
154+
155+
def test_main_git_missing_is_distinct_from_not_a_repo(self, monkeypatch, capsys):
156+
monkeypatch.setattr(gs, "git", lambda args, cwd=None: cp(127, "", "git: cannot execute"))
157+
assert pf.main([]) == 1
158+
out = capsys.readouterr().out
159+
assert "PREFLIGHT.GIT_MISSING" in out
160+
assert "PREFLIGHT.NOT_A_REPO" not in out

0 commit comments

Comments
 (0)