fix(session-flow): move git out of skill pre-compute so worktree-isolated agents can invoke these skills - #1676
Conversation
…ts can invoke these skills The harness composes an entire `## Pre-computed context` block into ONE shell invocation. The worktree-isolation Bash guard refuses a git-bearing compound command it cannot statically verify, so all seven git-bearing skills in this plugin failed at load from a worktree-isolated agent with "this command is too complex to verify that it stays inside the worktree". Removes the git lines from each pre-compute block and re-acquires those values in the skill body as individual Bash calls, one command per call. Non-git pre-compute lines are left untouched; they were never the trigger. `shell: bash` is left in place on every skill, including `workflow`, which now has no `!` lines at all — the key is inert without pre-compute lines, and removing it would be a frontmatter-contract change with no behavioral benefit. Refs #1619 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 56s —— View job PR Review
SummaryThe fix is mechanically correct and well-reasoned. The root cause diagnosis (composition, not per-command complexity) is convincingly demonstrated in the PR body, the change is tightly scoped to the seven affected skills, and the CHANGELOG is thorough. One stale internal cross-reference and a loss of graceful degradation are worth noting before merge. Findings1. Stale cross-reference in
|
|
Claude finished @kyle-sexton's task in 1m 27s —— View job Security Review — PR #1676
No security issues found. The PR makes one structural change across 7 skill files: git commands ( What was examined:
The diff is a mechanical structural refactor of read-only shell commands. No new data can flow into these commands from user input, environment, or the skill body's own argument handling. No new exfiltration paths are opened. The security-sensitive controls in |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0aaf89064
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ndling, prose weight Three findings from the PR review on #1676: 1. `orient` step 3 still pointed at "the pre-computed git block above", which no longer exists. It now names the gathered git context, matching the renamed section. This was the one finding that could mislead the model at runtime. 2. The pre-compute lines carried `2>/dev/null || echo "unknown"` fallbacks that the plain-Bash replacements dropped, so a non-repo invocation would surface a raw error instead of degrading. Each gather block now states explicitly that a failure means "unknown, carry on" — recovery-oriented skills like `orient` and `find-handoff` are exactly where ambiguous repo state is expected. 3. The rationale paragraph was three lines of contributor background consumed as prompt on every invocation. Compressed to two, and it now ends with the operational instruction that matters ("do not fold them back") rather than trailing explanation. This shape is what #1661 will replicate 31 more times, so the per-copy weight is worth getting right here. `find-handoff`'s remaining "pre-computed above" reference is correct and left alone — it points at the session-id `echo` line, which is still pre-computed. Refs #1619 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 4s —— View job PR Review — Follow-up (commit
|
|
Claude finished @kyle-sexton's task in 1m 55s —— View job Security Review — PR #1676 (HEAD: a83f203)
No security issues found. This PR makes two commits across 7 skill files: the first moves git commands out of harness-executed What was examined
The diff is a mechanical structural refactor of read-only shell commands. No new data can flow into these commands from user input, environment, or argument handling. The launch command in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a83f203bb3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…se verification claim Two findings from the Codex review on #1676, both valid. 1. The pre-computed lines capped working-tree output with `| head -20`; the plain-Bash replacements dropped the cap, so a repository with many changed paths would push its entire `git status --porcelain` into model context. The bound is restored as a reading rule rather than a pipe: a piped git command is compound, which is exactly the shape this PR exists to remove, so re-adding `| head -20` would reintroduce the defect in the body. 2. The CHANGELOG claimed the edited skills were verified by invoking them from a worktree-isolated agent. They were not, and the PR body says so explicitly. What was actually observed is now stated precisely: the *unfixed* skills were seen to be refused, plain git commands were seen to succeed as individual Bash calls, and a multi-line non-git pre-compute block was seen to load. The edited skills cannot be invoked until they ship, because skills load from the version-keyed plugin cache and 0.17.15 does not exist there yet. The second finding is the more serious one — a release note asserting a verification that did not happen is worse than no note at all. Refs #1619 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 32s —— View job PR Review — Follow-up (commit
|
| Finding | File | Status |
|---|---|---|
| Stale "pre-computed git block" cross-reference | orient/SKILL.md L61 |
✅ "the git context gathered above" |
| Loss of graceful degradation | All 7 skills | ✅ explicit "treat failure as unknown, carry on" |
| Background prose weight | All 7 skills | ✅ trimmed to 2–3 sentences |
One informational note (not blocking)
retro's git diff --name-only HEAD call carries no 20-entry bound instruction. This is not a regression — the original pre-compute block also had no cap on that line (| head -20 was on status only). git diff --name-only HEAD produces one filename per changed file, which is considerably less verbose than porcelain status output in practice. If the mass migration of the remaining 31 skills (#1661) includes any skill that had an explicit diff cap, that cap should be carried forward.
Summary
No blocking issues. Every finding across all three review rounds (initial, a83f203, and this 988e991 follow-up) has been resolved. The fix is mechanically correct, well-reasoned, and the CHANGELOG now accurately represents what was verified and what still needs post-ship confirmation. Ready to merge.
|
Claude finished @kyle-sexton's task in 2m 3s —— View job Security Review — PR #1676 (HEAD: 988e991)
No security issues found. This PR makes one structural change across 7 skill files: git commands ( What was examined
The diff is a mechanical structural refactor of read-only shell commands. No new data can flow into these commands from user input, environment, or argument handling. The launch command in |
…verification precisely Same two classes the Codex review caught on the sibling PR #1676, applied here before they were reported. The pre-computed lines carried output caps that plain Bash calls do not reproduce: `git status --short | head -20`, `git diff --cached --stat | tail -1`, `git worktree list | head -30`, and `git status | head -4`. Without them a large or deeply-worktreed repository would push its full output into model context. Each cap is restored as a reading rule rather than a pipe — a piped git command is compound, which is exactly the shape this PR removes, so re-adding the pipes would reintroduce the defect in the body. The CHANGELOG now states what was actually observed rather than implying the edited skills were exercised under isolation. They cannot be: skills load from the version-keyed plugin cache, and 0.33.2 does not exist there until this ships. Refs #1619 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…olated agents can invoke these skills (#1679) Moves git out of the `## Pre-computed context` block in all five git-bearing `source-control` skills, so they can be invoked from a worktree-isolated agent. Sibling of #1676 (`session-flow`). No linked issue — this is a partial remediation of #1619 (12 of 43 affected skills, together with #1676), so it must not auto-close it. #1661 tracks the remaining 31 skills across 16 plugins. ## The defect The harness composes an entire pre-compute block into **one** shell invocation, and the worktree-isolation Bash guard refuses that composed command when it contains git and is too complex to statically verify: > This agent is isolated in the worktree `…`, but this command is too complex to verify that it stays > inside the worktree; break it into plain, separate commands. So `commit`, `pull-request`, `worktree`, `resolve-conflicts`, and `babysit-prs` all failed **at load** from an isolated agent. `worktree` is the sharpest case — the skill for managing worktrees could not be invoked from inside one. Mechanism confirmed by direct probe (full write-up on #1619): `code-tidying:batch-simplify` (one git pre-compute line, compound) **loads**; `knowledge:course-digest` (four complex non-git lines) **loads**; `session-flow:find-handoff` (four lines, one pipe-free git line) **is refused**. The trigger is git inside a composed block, not per-line complexity. ## The change Delete the git lines from each pre-compute block; re-acquire those values in the skill body as **individual** Bash calls, one command per call. Non-git pre-compute lines are untouched — `commit` keeps its exec-bit and user-global config probes, `babysit-prs` keeps both `gh` lines. Three things worth review attention: 1. **`commit`'s config-layer probes were themselves compound one-liners** that re-derived the repository root inline (`R="$(git rev-parse --show-toplevel)" && git -C "$R" ls-files …`). Moving them across intact would have reproduced the defect as a body call, so they are **decomposed**: resolve the root once with `git rev-parse --show-toplevel`, then substitute the literal path into the tracked-layer `ls-files` check and the personal-overlay `test -f`. The root-anchoring requirement and the `present but UNTRACKED` rule are unchanged. 2. **Failure handling.** The old lines carried `2>/dev/null || echo "unknown"`; plain Bash calls do not reproduce that, so each gather block now says explicitly that a failed command means "unknown, carry on" rather than surfacing a raw error. 3. **Two reference spokes went stale** and are corrected. `commit/reference/exec-bit.md` no longer calls the config-layer probes pre-computed. `pull-request/reference/create.md`'s `--pushed` section justified ignoring the session context partly on the grounds that a `!`-substituted line *cannot* be `git -C`-redirected — no longer true once these are ordinary Bash calls. The section keeps its instruction (re-resolve explicitly from the target worktree) on the reason that still holds (session cwd is the wrong branch for an out-of-tree orchestrator). `shell: bash` stays on every skill, including the three that now have no `!` lines at all — inert without pre-compute lines, and removing it would be a frontmatter-contract change with no behavioral benefit. `babysit-prs` is held at **exactly 499 lines** (net-zero), so this does not consume the single line it has left under the 500-line hard cap (#1626). ## Known pre-existing CI failure — not introduced here `babysit-prs/scripts/engine.test.sh` fails on **clean `main`** at `9f73fc2e`, before any change in this PR: ``` FAIL: test_every_refusal_row (test_guards.RefusalsFireOnArgumentShape.test_every_refusal_row) (row='merge.wrapper-reaches-failclosed-cli') AssertionError: 1 != 3 (row='resolve.wrapper-reaches-failclosed-cli') AssertionError: 1 != 3 (row='resolve.wrapper-filters-nothing') AssertionError: 1 != 2 ``` `main` stays green because `check-changed-skills` only runs a skill's script tests when that skill changes, and `babysit-prs` had not changed. This PR touches it, so the gate runs and surfaces the latent failure. Verified pre-existing by running the test on an unmodified `main` checkout — this PR changes only SKILL.md prose and touches no engine code. Filed as **#1678**; **not** fixed here, since folding an unrelated guard-test repair into a documentation-shaped migration would obscure both. ## Verification - **Proven by probe:** four plain git commands (`rev-parse --show-toplevel`, `branch --show-current`, `status --porcelain`, `log --oneline -5`) each succeed as ordinary Bash calls from inside a worktree-isolated agent — that is what the replacement bodies rely on. - **Proven by probe:** a multi-line pre-compute block containing no git loads fine under isolation. Every skill here is now exactly that shape. - **Not directly observed, and deliberately not faked:** invoking the *edited* skills from an isolated agent. Skills load from the version-keyed plugin cache (`~/.claude/plugins/cache/<marketplace>/<plugin>/<version>/`), so `0.33.2` does not exist there until this merges and plugins are updated. Post-merge confirmation: update plugins, then invoke `/source-control:worktree` from an isolated agent, with `knowledge:course-digest` as the positive control. - CI **cannot** prove this fix — it never invokes a skill from an isolated agent. Local gates against `origin/main`: `check-changelog-parity --check-bump` pass, `check-skill-portability` pass, `markdownlint-cli2` 0 issues, `check-changed-skills` 4 of 5 pass with the fifth failing only on the pre-existing `engine.test.sh` above. ## Fresh-docs `https://code.claude.com/docs/en/skills` fetched this session (2026-07-26): the page documents *"Each `` !`<command>` `` executes immediately (before Claude sees anything)"* and says nothing about composing the block into a single invocation — the observed behavior contradicts it. It also confirms `shell:` accepts `bash` (default) or `powershell`, which is why leaving `shell: bash` is inert. ## Related - #1676 — the `session-flow` half of this remediation. - #1619 — the defect, the confirmed composition mechanism, and the full 49-skill sweep. - #1661 — the remaining 31 skills across 16 plugins. - #1662 — no gate catches a second pre-compute line silently re-breaking a git-bearing skill. - #1678 — the pre-existing `engine.test.sh` failure this PR surfaces but does not cause or fix. - #1626 — the `babysit-prs` line-cap headroom this PR deliberately does not consume. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ents can load these skills (#1688) Closes #1687 `0.17.15` (#1676) removed git from seven `session-flow` skills' pre-compute blocks on the theory that the harness composes a block into one shell invocation and the worktree-isolation Bash guard refuses a git-bearing compound command. **That diagnosis is wrong and that release fixed nothing.** It kept `` !`echo "${CLAUDE_CODE_SESSION_ID:-unknown}" || echo "unknown"` `` in six of the seven skills, so `handoff`, `continue-in-background`, `orient`, `retro`, `running-retro`, and `find-handoff` were still refused at load for the whole of `0.17.15`. Only `workflow` was fixed, and only incidentally — its entire pre-compute block had been deleted. ## Step 1 — probe results, verbatim Every command below was run as its **own** standalone Bash call from this worktree-isolated agent — no `&&`, no `;`, no loops. REFUSED means the guard's `too complex to verify that it stays inside the worktree` error; PASS means the command ran and returned output. | command | result | | --- | --- | | `echo $HOME` | **PASS** — printed the home path | | `echo "$HOME"` | **PASS** — printed the home path | | `echo ${HOME}` | **REFUSED** | | `echo "${HOME}"` | **REFUSED** | | `echo $CLAUDE_CODE_SESSION_ID` | **REFUSED** | | `ls -1dt "$HOME/.claude/projects/"*/` | **PASS** — listed 11 project dirs | | `ls -1dt "$HOME/.claude/projects/"*/ 2>/dev/null \| head -8 \|\| echo "none"` | **PASS** — `find-handoff`'s line with only the braces dropped | | `printenv CLAUDE_CODE_SESSION_ID` | **PASS** — printed the session UUID | | `printenv CLAUDE_CODE_SESSION_ID \|\| echo unknown` | **PASS** | | `echo "${CLAUDE_CODE_SESSION_ID:-unknown}" \|\| echo "unknown"` | **REFUSED** — the current pre-compute line, verbatim | | `ls -1dt ~/.claude/projects/*/ 2>/dev/null \| head -3 \|\| echo "none"` | **PASS** — tilde form, `$`-free | Two things follow. **#1687's rule holds, unmodified.** A command is refused iff it contains a `$`-expansion in any form other than bare `$HOME` or `"$HOME"`. Pipes, `2>/dev/null`, `||`, and quoting are all irrelevant. **One thing #1687 flagged as inferred is now observed.** #1687 wrote: *"**Inferred, not observed:** that the guard uses a narrow name-and-form allowlist rather than a resolver."* `echo $CLAUDE_CODE_SESSION_ID` — bare, no braces, same form as the passing `echo $HOME` — is **REFUSED**. So the allowlist is **name-specific**, not merely form-specific, and `HOME` is the only member found across two independent probe sessions. That is uncharacterized upstream behavior; a guard tightening would regress `find-handoff`'s one remaining pre-compute line and nothing else in this plugin. Nothing observed this session contradicts #1687 as stated. ## The change | skill | pre-compute before | pre-compute after | | --- | --- | --- | | `handoff` | 1 line (session id) | **block removed** | | `continue-in-background` | 1 line (session id) | **block removed** | | `orient` | 1 line (session id) | **block removed** | | `retro` | 1 line (session id) | **block removed** | | `running-retro` | 1 line (session id) | **block removed** | | `find-handoff` | 3 lines (session id, handoffs glob, `${HOME}` transcript glob) | 2 lines — session id gone, `${HOME}` → bare `$HOME` | - The session id is re-acquired in each body with `printenv CLAUDE_CODE_SESSION_ID` — no `$`, observed to pass. Failure is treated as "unknown, carry on", same as the old `:-unknown` fallback. - Deleting that line emptied five blocks entirely, so the bare `## Pre-computed context` heading is removed rather than left dangling — the shape #1676 gave `workflow`. - `find-handoff`'s glob keeps its pre-compute line with the braces dropped; the full line was run verbatim under isolation and passes. Moving it to the body would have been the alternative, but the minimal change is sufficient and keeps the value pre-rendered. - **Downstream reference fixed:** `find-handoff`'s body said "*(`$CLAUDE_CODE_SESSION_ID`, pre-computed above)*" for the transcript-exclusion step. Removing the line without repointing that reference would have left the skill loading but misbehaving. It now reads "the session id gathered above". ### One scope addition, deliberate `0.17.15` wrote the falsified mechanism into the **body prose** of all seven skills, and derived an instruction from it: *"do not restore it as a `| head -20` pipe. A piped git command is compound, which is the shape #1619 is about."* The probe above shows that exact form passes. Shipping a CHANGELOG that declares the diagnosis wrong while leaving it in the prompt text of every invocation would be self-contradictory, so the rationale is corrected and compressed to one sentence in all seven skills — **including `workflow`**, whose pre-compute is otherwise untouched. The 20-entry read bound is kept as a plain instruction; only its false justification is dropped. This also answers the review note on #1676 that this prose is consumed as prompt on every invocation: it drops from two paragraphs to one sentence per skill. ## Verification — what was and was not established **Verified.** Every command form in the Step 1 table, run standalone from a worktree-isolated agent, including the replacement `printenv` call and `find-handoff`'s rewritten glob line. **NOT verified, and deliberately not claimed.** That the *edited* skills load from an isolated agent. They cannot be tested before merge: skills load from the version-keyed plugin cache (`<cache_root>/<marketplace>/<plugin>/<version>/`), so `0.17.16` does not exist there until this ships and plugins are updated. **No isolated-agent invocation of the edited skills was performed** — #1676 claimed one it had not done, and this PR does not repeat that. The post-merge confirmation is: update plugins, then from an `Agent` with `isolation: "worktree"` invoke `/session-flow:handoff`, with `knowledge:course-digest` as the positive control **and a still-`$`-bearing skill (for example `claude-memory:audit`) as the required negative control** — a green result without the control is exactly what produced the wrong diagnosis twice. CI cannot prove this fix. It never invokes a skill from an isolated agent. **Known remaining limitation, in scope for a later issue, not fixed here.** Several bodies and `reference/` snippets still run `$`-bearing shell — `retro`'s transcript-parser invocation, for instance. The six skills now **load** under isolation; those specific body commands remain subject to the same guard. ## Local gates Run from the worktree root against `origin/main`: - `npx markdownlint-cli2 "plugins/session-flow/**/*.md"` — 0 issues in 35 files - `bash scripts/check-skill-portability.sh origin/main` — pass, 7 skill files - `bash scripts/check-changelog-parity.sh --check-bump origin/main` — pass - `bash scripts/check-changed-skills.sh origin/main` — 7 checked, 0 failed ## Fresh-docs `https://code.claude.com/docs/en/skills` fetched this session (2026-07-26), via `docs/OFFICIAL-DOCS.md`. Confirmed from that page: - The injection contract: *"The `` !`<command>` `` syntax runs shell commands before the skill content is sent to Claude"*, and *"Each `` !`<command>` `` executes immediately (before Claude sees anything)"*. The page documents nothing about env-var expansion inside those commands and nothing about a worktree guard — the refusal is Bash-tool behavior, not documented skill behavior. - `shell` frontmatter: *"Shell to use for `` !`command` `` and ` ```! ` blocks in this skill. Accepts `bash` (default) or `powershell`."* It is optional and scoped to `!` commands, so it is inert on a skill with no `!` lines. `shell: bash` is therefore left in place on the five skills whose pre-compute block is now gone — removing it would be a frontmatter-contract change with no behavioral benefit, matching #1676's precedent for `workflow`. ## Related - #1619 — the original defect; its correction comment carries the `$`-expansion rule and the controlled pair this PR re-derived. - #1676 — merged as `0.17.15`; fixed the wrong thing and left the `$` lines in place. Its body-instruction half is kept: individual git calls demonstrably pass under isolation. - #1661 / #1662 — the remainder list and the CI gate that would catch this class, both re-scoped by #1687 to `$`-expansion. - #1687 tracks the other 15 affected skills across 10 plugins; this PR covers `session-flow` only. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Moves git out of the
## Pre-computed contextblock in all seven git-bearingsession-flowskills,so they can be invoked from a worktree-isolated agent.
No linked issue — this is a partial remediation of #1619 (12 of 43 affected skills), so it must not
auto-close it. #1661 tracks the remaining 31 skills across 16 plugins.
The defect
The harness composes an entire pre-compute block into one shell invocation. The
worktree-isolation Bash guard then refuses that composed command when it contains git and is too
complex to statically verify:
So
handoff,continue-in-background,workflow,running-retro,orient,retro, andfind-handoffall failed at load from an isolated agent — the sessions that most need asave-point, an orientation pass, or handoff recovery were exactly the ones that could not get one.
Evidence the mechanism is composition, not per-line complexity
Probed directly from an
Agentwithisolation: "worktree"(full write-up on #1619):code-tidying:batch-simplify||fallback)knowledge:course-digest||)session-flow:find-handoffsession-flow:handoffbatch-simplifyvsfind-handoffis the discriminator: the same git line passes alone and failsinside a multi-line block.
find-handoffin particular had a single, pipe-free git line — aboutas simple as a git pre-compute line gets — and was still refused. Per-line simplification would not
have fixed anything.
The change
For each skill: delete the git lines from the pre-compute block, and re-acquire those values in the
skill body as individual Bash calls, one command per call. Non-git pre-compute lines are left
exactly as they were —
course-digestproves they are not the trigger.shell: bashstays on every skill, includingworkflow, which now has no!lines at all. The keyis inert without pre-compute lines, and removing it would be a frontmatter-contract change with no
behavioral benefit.
Verification
rev-parse --show-toplevel,branch --show-current,status --porcelain,log --oneline -5) each succeed as ordinary Bash calls from inside aworktree-isolated agent. That is what the replacement body relies on.
(
course-digest). Every skill in this PR is now exactly that shape.isolated agent. Skills load from the version-keyed plugin cache
(
~/.claude/plugins/cache/<marketplace>/<plugin>/<version>/), so0.17.15does not exist thereuntil this merges and plugins are updated. The post-merge confirmation is: update plugins, then
invoke
/session-flow:handofffrom an isolated agent, withknowledge:course-digestas thepositive control.
Local gates run against
origin/main:check-changelog-parity --check-bumppass,check-skill-portabilitypass (7 files),check-changed-skills7 checked / 0 failed,markdownlint-cli20 issues.Fresh-docs
https://code.claude.com/docs/en/skillsfetched this session (2026-07-26). It documents theinjection syntax as "Each
!`<command>`executes immediately (before Claude sees anything)" andsays nothing about composing the block into a single invocation — the observed behavior contradicts
the page. That discrepancy is captured in the upstream draft, not fixed here. The page also confirms
shell:acceptsbash(default) orpowershell, which is why leavingshell: bashin place isinert.
Related
49-skill sweep.