Skip to content

Commit 85ea754

Browse files
randleeclaude
andcommitted
fix(sc-gh-stack): resolve Opus guidelines-review findings F1,F3-F12
- F1: gh_stack_shared.run() returns a synthetic rc-127 CompletedProcess when a binary is missing/unexecutable, so every script still emits its fenced JSON envelope instead of a traceback; tests pin run() and an empty-PATH preflight - F3+F6: convert and sync agents promoted to the Standard envelope (canceled, aborted_by, metadata.duration_ms/tool_calls/retry_count); a risky-conflict stop is canceled:true aborted_by:policy with CONVERT/SYNC.CONFLICT_RISKY and recoverable:false; mirrored in both playbooks and SKILL.md report contract - F4: pushed flag sourcing disambiguated — script SHAs are pre-submit; agents re-derive pushed after gh stack submit per step 7 - F5: agent sections renamed to '## Output Format' per the Agent Template - F7: delegation guardrails completed: correlation_id per stack, spec aggregation object ordered by correlation_id, summary.failed surfaced, timeout departure recorded as intentional - F8: install-doc Validation expects both normal warns - F9: worktree slug examples fixed (no case folding); rule restated in sync agent inputs - F10: convert agent error boundary now a catch-all (forward any unlisted script envelope unchanged) with codes as illustrations - F11: Step 1 and install-doc PATH probes iterate gh/git/python3 incl. pyenv shims - F12: gh_stack_convert.py --dry-run/--validate previews the per-layer plan mutating nothing (dry_run agent input added; SKILL.md records preflight + dry-run as the --validate surface and the trivial-conflict rubric as the bounded --auto-fix); GIT.FETCH now recoverable:false per the package rule F2 (registry entries for the agents in repo .claude) is REJECTED per user directive: no installed copies in this repo's .claude — packages/ is the single source of truth. 58 tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 38f80e6 commit 85ea754

11 files changed

Lines changed: 222 additions & 58 deletions

File tree

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

Lines changed: 31 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@ 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)
3436

3537
## Execution
3638

@@ -63,15 +65,18 @@ Complete the task quickly; report only decisions and discrepancies.
6365
8. Never resolve a risky conflict, never `git push` directly, never force-push, never
6466
`git reset --hard`, never run bare interactive `gh stack` commands.
6567

66-
## Output
68+
## Output Format
6769

68-
Return ONE fenced JSON block. Success is a minimal decision log; failure must let the caller
69-
recover without investigating (the scripts' envelopes already carry the failing command,
70-
stderr, and recovery action — forward those fields, do not paraphrase them away).
70+
Return ONE fenced JSON block using the Standard envelope (this agent is multi-step). Success
71+
is a minimal decision log; failure must let the caller recover without investigating (the
72+
scripts' envelopes already carry the failing command, stderr, and recovery action — forward
73+
those fields, do not paraphrase them away).
7174

7275
```json
7376
{
7477
"success": true,
78+
"canceled": false,
79+
"aborted_by": null,
7580
"data": {
7681
"shape": "(main) <- feat/schema <- feat/api",
7782
"worktree": "/path/to/repo-worktrees/stack/feat-schema",
@@ -85,16 +90,26 @@ stderr, and recovery action — forward those fields, do not paraphrase them awa
8590
"surfaced": [],
8691
"next_step": null
8792
},
88-
"error": null
93+
"error": null,
94+
"metadata": { "duration_ms": 48000, "tool_calls": 14, "retry_count": 0 }
8995
}
9096
```
9197

92-
On risky conflicts: `success: false`, `error` = the script's error object, `surfaced` lists
93-
each unresolved conflict as `{ "file", "layer", "worktree", "why_risky", "suggested_resolution" }`,
94-
and `next_step` says exactly where the rebase is paused. Every output produced after the
95-
convert script has run must include `branches` (with `pushed` per branch — the script's
96-
`data.branches` supplies it); outputs that stop earlier (PR resolution, worktree check,
97-
preflight) return that step's envelope alone.
98+
Field sourcing: `branches[].before`/`after` come from the script's `data.branches`;
99+
`pushed` MUST be re-derived after `gh stack submit --auto` per step 7 (the script never
100+
pushes, so its `pushed` values are pre-submit). When submit did not run (`push: false`,
101+
`dry_run: true`, or an early stop), report `pushed: false` for every rebased layer.
102+
103+
Stopping on a risky conflict is a deliberate policy abort, not a failure of the operation:
104+
set `success: false, canceled: true, aborted_by: "policy"`, re-code the script's error as
105+
`CONVERT.CONFLICT_RISKY` with `recoverable: false` (a bare retry must not be attempted —
106+
a human resolves first), keep its forensic fields (`cmd`, files, `next_step`) intact, and
107+
list each conflict in `surfaced` as
108+
`{ "file", "layer", "worktree", "why_risky", "suggested_resolution" }`. Genuine failures
109+
(fetch, init, non-conflict rebase errors) keep `canceled: false` and the script's error
110+
object unchanged. Every output produced after the convert script has run must include
111+
`branches`; outputs that stop earlier (PR resolution, worktree check, preflight) return that
112+
step's envelope alone, wrapped with the `canceled`/`metadata` fields.
98113

99114
## Error Handling
100115

@@ -104,8 +119,11 @@ preflight) return that step's envelope alone.
104119
recipe; earlier pushes stand.
105120

106121
### Propagated to caller (stop and report):
107-
- Preflight failures, `GIT.BRANCH_DIVERGED`, `CONVERT.REBASE_FAILED`, `STACK.INIT_FAILED`,
108-
submit exit 9, and every risky conflict.
122+
- Every non-zero exit from the preflight or convert script not explicitly handled above —
123+
forward the envelope unchanged. Illustrations: preflight failures, `GIT.BRANCH_DIVERGED`,
124+
`GIT.DIRTY_TREE`, `GIT.REBASE_IN_PROGRESS`, `GIT.FETCH`, `CONVERT.REBASE_FAILED`,
125+
`CONVERT.FF_FAILED`, `STACK.INIT_FAILED`, `VALIDATION.INPUT`, submit exit 9, and every
126+
risky conflict.
109127

110128
## Constraints
111129

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,9 +51,9 @@ Apply the mapping rules (a stack is strictly linear; its only external dependenc
5151
Ambiguous dependencies (order both unknown and consequential): do not guess — list them in
5252
`questions` and mark the affected stack `blocked: true`.
5353

54-
## Output
54+
## Output Format
5555

56-
Return ONE fenced JSON block:
56+
Return ONE fenced JSON block (Basic envelope — this agent is single-step and read-only):
5757

5858
```json
5959
{

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

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,9 @@ made and any discrepancy the caller must handle.
2525

2626
## Inputs
2727

28-
- **worktree** (required): path of the stack's worktree (a stack branch must be checked out
29-
there; `gh stack checkout <branch>` if not)
28+
- **worktree** (required): path of the stack's worktree, computed with the SKILL.md slug rule
29+
(`<repo_root>-worktrees/stack/<bottom-slug>`, `/``-`, no case folding); a stack branch
30+
must be checked out there (`gh stack checkout <branch>` if not)
3031
- **fix_branch** (optional): a middle layer that just received commits — after syncing, verify
3132
every layer above it contains its tip
3233

@@ -62,17 +63,20 @@ gh-stack tracking is per-worktree. When the given worktree does not track the st
6263
4. Never `git push` directly, never force-push, never `git reset --hard`, never bare
6364
interactive `gh stack` commands.
6465

65-
## Output
66+
## Output Format
6667

67-
Return ONE fenced JSON block. Success is a minimal decision log; failure must let the caller
68-
recover without investigation (the script's envelope carries the failing command, stderr, and
69-
recovery action — forward those fields, do not paraphrase them away).
68+
Return ONE fenced JSON block using the Standard envelope (this agent is multi-step). Success
69+
is a minimal decision log; failure must let the caller recover without investigation (the
70+
script's envelope carries the failing command, stderr, and recovery action — forward those
71+
fields, do not paraphrase them away).
7072

7173
```json
7274
{
7375
"success": true,
76+
"canceled": false,
77+
"aborted_by": null,
7478
"data": {
75-
"worktree": "/path/to/repo-worktrees/stack/l1",
79+
"worktree": "/path/to/repo-worktrees/stack/L1",
7680
"branches": [
7781
{ "name": "L2", "before": "abc1", "after": "def2", "pushed": true },
7882
{ "name": "L3", "before": "1122", "after": "3344", "pushed": true }
@@ -85,16 +89,21 @@ recovery action — forward those fields, do not paraphrase them away).
8589
"fix_contained_by": ["L3", "L4"],
8690
"next_step": null
8791
},
88-
"error": null
92+
"error": null,
93+
"metadata": { "duration_ms": 32000, "tool_calls": 9, "retry_count": 0 }
8994
}
9095
```
9196

92-
On risky conflicts: `success: false`, `error` = the script's error object, `surfaced` lists
93-
each unresolved conflict as `{ "file", "layer", "worktree", "why_risky", "suggested_resolution" }`,
94-
and `next_step` says exactly where the rebase is paused. Include `branches` (with `pushed`
95-
per branch) in every output produced after `gh_stack_sync.py` has run — its `data.branches`
96-
supplies them; `fix_contained_by` appears only when `fix_branch` was given (layers above it
97-
that contain its tip; report any that do not).
97+
Stopping on a risky conflict is a deliberate policy abort: set
98+
`success: false, canceled: true, aborted_by: "policy"`, re-code the script's error as
99+
`SYNC.CONFLICT_RISKY` with `recoverable: false` (a human resolves before any retry), keep
100+
its forensic fields intact, and list each conflict in `surfaced` as
101+
`{ "file", "layer", "worktree", "why_risky", "suggested_resolution" }` with `next_step`
102+
saying exactly where the rebase is paused. Genuine failures keep `canceled: false` and the
103+
script's error object unchanged. Include `branches` (with `pushed` per branch) in every
104+
output produced after `gh_stack_sync.py` has run — its `data.branches` supplies them;
105+
`fix_contained_by` appears only when `fix_branch` was given (layers above it that contain
106+
its tip; report any that do not).
98107

99108
## Error Handling
100109

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

Lines changed: 77 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
11
#!/usr/bin/env python3
22
"""Convert N existing branches (each based on trunk) into one linear gh stack.
33
4-
Usage: python3 gh_stack_convert.py <trunk> <bottom> ... <top> [--cwd PATH]
4+
Usage: python3 gh_stack_convert.py <trunk> <bottom> ... <top> [--cwd PATH] [--dry-run]
5+
6+
--dry-run (alias --validate) previews the per-layer plan (skip / rebase /
7+
fast_forward_then_rebase / refuse_diverged) after a fetch, mutating nothing.
58
69
Arguments after trunk are branch names or PR numbers (resolved via `gh pr view`),
710
bottom to top. Each layer is rebased `--onto` the layer below, replaying only
@@ -270,8 +273,55 @@ def init_stack(trunk: str, layers: List[str], cwd: Optional[Path] = None) -> Dic
270273
return {"action": "initialised"}
271274

272275

273-
def convert(trunk: str, raw_layers: List[str], cwd: Optional[Path] = None) -> tuple[int, Dict[str, Any]]:
274-
"""Full workflow. Returns (exit_code, envelope). Testable without argparse."""
276+
def plan_chain(layers: List[str], trunk_ref: str, remote: str,
277+
cwd: Optional[Path] = None) -> List[Dict[str, Any]]:
278+
"""Read-only preview of what chain() would do per layer. Mutates nothing:
279+
no fast-forwards, no orig refs, no rebases."""
280+
planned: List[Dict[str, Any]] = []
281+
below = trunk_ref
282+
below_name: Optional[str] = None
283+
for layer in layers:
284+
layer_ref = _head(layer) if gs.local_branch_exists(layer, cwd=cwd) \
285+
else f"refs/remotes/{remote}/{layer}"
286+
entry: Dict[str, Any] = {"branch": layer,
287+
"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"
295+
planned.append(entry)
296+
below, below_name = layer_ref, layer
297+
continue
298+
if gs.is_ancestor(below, layer_ref, cwd=cwd) and not _has_merges(below, layer_ref, cwd=cwd):
299+
entry["action"] = "skip"
300+
else:
301+
upstream = trunk_ref
302+
if below_name is not None:
303+
orig_below = _orig_tip(below_name, cwd=cwd)
304+
if orig_below and gs.is_ancestor(orig_below, layer_ref, cwd=cwd):
305+
upstream = orig_below
306+
elif gs.is_ancestor(below, layer_ref, cwd=cwd):
307+
upstream = below
308+
else:
309+
fork = gs.git_out(["merge-base", orig_below or below, layer_ref], cwd=cwd)
310+
if fork:
311+
upstream = fork
312+
entry["action"] = "rebase"
313+
entry["upstream"] = upstream
314+
planned.append(entry)
315+
below, below_name = layer_ref, layer
316+
return planned
317+
318+
319+
def convert(trunk: str, raw_layers: List[str], cwd: Optional[Path] = None,
320+
dry_run: bool = False) -> tuple[int, Dict[str, Any]]:
321+
"""Full workflow. Returns (exit_code, envelope). Testable without argparse.
322+
323+
dry_run=True previews the per-layer plan (skip/rebase/fast-forward/refuse)
324+
after a fetch, mutating nothing beyond remote-tracking refs."""
275325
def fail(code: int, err_code: str, msg: str, action: str,
276326
data: Optional[Dict[str, Any]] = None, recoverable: bool = True):
277327
# recoverable means: apply suggested_action, re-run, and it succeeds by
@@ -302,14 +352,15 @@ def fail(code: int, err_code: str, msg: str, action: str,
302352
"git config remote.pushDefault origin")
303353
remote = gs.resolve_remote(cwd=cwd)
304354

305-
gs.git(["config", "rerere.enabled", "true"], cwd=cwd)
306-
# autoUpdate stages rerere's replayed resolutions, so a fully-rerere-resolved
307-
# stop reports conflict.files == [] and needs only `git rebase --continue`.
308-
gs.git(["config", "rerere.autoUpdate", "true"], cwd=cwd)
355+
if not dry_run:
356+
gs.git(["config", "rerere.enabled", "true"], cwd=cwd)
357+
# autoUpdate stages rerere's replayed resolutions, so a fully-rerere-resolved
358+
# stop reports conflict.files == [] and needs only `git rebase --continue`.
359+
gs.git(["config", "rerere.autoUpdate", "true"], cwd=cwd)
309360
fetch = gs.git(["fetch", remote, "--prune"], cwd=cwd)
310361
if fetch.returncode != 0:
311362
return fail(EXIT_ERR, "GIT.FETCH", f"git fetch {remote} failed: {fetch.stderr.strip()}",
312-
"check network/auth and re-run")
363+
"check network/auth and re-run", recoverable=False)
313364

314365
trunk_ref = f"{remote}/{trunk}"
315366
if not gs.remote_branch_exists(remote, trunk, cwd=cwd):
@@ -320,11 +371,25 @@ def fail(code: int, err_code: str, msg: str, action: str,
320371
layers = resolve_layers(raw_layers, cwd=cwd)
321372
if trunk in layers:
322373
raise ValueError(f"layer equals trunk: {trunk}")
323-
ensure_local_branches(layers, remote, cwd=cwd)
374+
if dry_run:
375+
for b in layers:
376+
if not gs.local_branch_exists(b, cwd=cwd) \
377+
and not gs.remote_branch_exists(remote, b, cwd=cwd):
378+
raise ValueError(f"branch not found locally or on {remote}: {b}")
379+
else:
380+
ensure_local_branches(layers, remote, cwd=cwd)
324381
except ValueError as exc:
325382
return fail(EXIT_INPUT, "VALIDATION.INPUT", str(exc), "fix the layer list and re-run",
326383
recoverable=False)
327384

385+
if dry_run:
386+
data = {"mode": "dry_run", "trunk": trunk, "remote": remote, "layers": layers,
387+
"shape": stack_shape(trunk, layers),
388+
"planned": plan_chain(layers, trunk_ref, remote, cwd=cwd),
389+
"next_step": "review the plan (any refuse_diverged layer must be reconciled "
390+
"first), then re-run without --dry-run to execute"}
391+
return EXIT_OK, gs.envelope(True, data)
392+
328393
before = {b: _rev(_head(b), cwd=cwd) for b in layers}
329394
base = {"trunk": trunk, "remote": remote, "layers": layers, "shape": stack_shape(trunk, layers)}
330395
begin_conversion(trunk, layers, cwd=cwd)
@@ -360,8 +425,10 @@ def main(argv: Optional[List[str]] = None) -> int:
360425
parser.add_argument("trunk")
361426
parser.add_argument("layers", nargs="+", help="branch names or PR numbers, bottom to top")
362427
parser.add_argument("--cwd", type=Path, default=None)
428+
parser.add_argument("--dry-run", "--validate", action="store_true", dest="dry_run",
429+
help="preview the per-layer plan; mutate nothing")
363430
args = parser.parse_args(argv)
364-
code, payload = convert(args.trunk, args.layers, cwd=args.cwd)
431+
code, payload = convert(args.trunk, args.layers, cwd=args.cwd, dry_run=args.dry_run)
365432
gs.emit(payload)
366433
return code
367434

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,14 @@
1616

1717

1818
def run(cmd: Sequence[str], cwd: Optional[Path] = None) -> subprocess.CompletedProcess:
19-
"""Run a command, capturing output; never raises on non-zero exit."""
20-
return subprocess.run(list(cmd), cwd=cwd, capture_output=True, text=True, check=False)
19+
"""Run a command, capturing output; never raises. A binary missing from
20+
PATH returns a synthetic CompletedProcess (rc 127) so callers always get a
21+
structured result and scripts always emit their fenced JSON envelope."""
22+
try:
23+
return subprocess.run(list(cmd), cwd=cwd, capture_output=True, text=True, check=False)
24+
except (FileNotFoundError, NotADirectoryError, PermissionError) as exc:
25+
return subprocess.CompletedProcess(args=list(cmd), returncode=127, stdout="",
26+
stderr=f"{cmd[0]}: cannot execute ({exc})")
2127

2228

2329
def git(args: Sequence[str], cwd: Optional[Path] = None) -> subprocess.CompletedProcess:

packages/sc-gh-stack/skills/managing-gh-stacks/SKILL.md

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -50,12 +50,16 @@ which git && git --version
5050
which python3 && python3 --version # scripts need >= 3.9
5151
```
5252

53-
If `gh` is not on PATH, probe common install locations before assuming it is absent —
54-
Claude Code's bash may not share PATH with the interactive shell:
53+
If any of them is not on PATH, probe common install locations before assuming it is absent —
54+
Claude Code's bash may not share PATH with the interactive shell (Homebrew dirs and pyenv
55+
shims are the usual omissions):
5556

5657
```bash
57-
for p in "/opt/homebrew/bin/gh" "/usr/local/bin/gh" "$HOME/.local/bin/gh"; do
58-
[ -x "$p" ] && echo "Found at: $p" && break
58+
for cli in gh git python3; do
59+
command -v "$cli" >/dev/null && continue
60+
for d in /opt/homebrew/bin /usr/local/bin "$HOME/.local/bin" "$HOME/.pyenv/shims"; do
61+
[ -x "$d/$cli" ] && echo "$cli found at: $d/$cli" && break
62+
done
5963
done
6064
```
6165

@@ -104,9 +108,15 @@ All stack execution happens in dedicated worktrees, never in the user's checkout
104108

105109
Delegate convert and sync via the Task tool with `run_in_background: true`; the plan agent is
106110
read-only and fast, so it may run in the foreground. Background agents complete
107-
asynchronously — wait for the completion notification rather than assuming a timeout, and
108-
never start a second agent on the same worktree while one is running. Cap 3–4 concurrent;
109-
one agent per stack:
111+
asynchronously — wait for the completion notification rather than assuming a timeout (a
112+
deliberate departure from the spec's per-task timeouts: background Task agents signal
113+
completion themselves). Never start a second agent on the same worktree while one is
114+
running. Cap 3–4 concurrent; one agent per stack, each invocation tagged with a
115+
`correlation_id` (use the stack's bottom-branch slug). When running agents over several
116+
stacks, aggregate per the spec: `{ "parallel": true, "concurrency": N, "results": [...],
117+
"summary": { "all_successful", "failed", "succeeded" } }`, results ordered by
118+
`correlation_id`, and surface `summary.failed` to the user — never silently drop a failed
119+
stack:
110120

111121
| Situation | Agent | Input (as `<input_json>`) | Returns |
112122
|---|---|---|---|
@@ -148,6 +158,15 @@ the expected report. Simple one-command situations (land a green stack:
148158
- **Failure = forensic**: the exact command that failed, its stderr, per-branch state, and
149159
one concrete recovery action — replaying the tool calls must show what happened without
150160
further investigation.
161+
- **Envelope levels**: convert and sync agents use the Standard envelope (`canceled`,
162+
`aborted_by`, `metadata.duration_ms`/`tool_calls`/`retry_count` added); a risky-conflict
163+
stop is `canceled: true, aborted_by: "policy"` with error code `*_RISKY` and
164+
`recoverable: false` — a deliberate hold for human review, not an operation failure. The
165+
plan agent and the scripts use the Basic envelope.
166+
- **Validation surface**: `gh_stack_preflight.py` (environment) plus
167+
`gh_stack_convert.py --dry-run` (per-layer plan preview, nothing mutated) are this
168+
package's `--validate` mode; `--auto-fix` is deliberately absent — its bounded-fix role is
169+
filled by the trivial-conflict rubric above.
151170
- **State guarantees**: scripts never push, never force, never delete refs; conversion stops
152171
at the first conflict with finished layers skipped on re-run; `gh stack sync` restores
153172
every branch on conflict (all-or-nothing); agents push only via `gh stack submit --auto` /

0 commit comments

Comments
 (0)