feat(claude): run promotion from inside Claude Code as /promote - #217
Conversation
Adds the slash command the promotion spec anticipated ("the Claude Code slash command is a thin
wrapper over it", §4.3), so a workflow is promoted from the session it was authored in rather than
from a second terminal. Promote-only by design: it does not dispatch, so it stays useful with no
cluster reachable and leaves the lockfile as the hand-off.
The command ships at deploy/claude/commands/promote.md and is installed with one `cp` into the
user's real ~/.claude/commands/. That placement is mechanical, not taste: with HOME pointed at the
project, promote reads that project's .claude/commands/ as its prompts directory and bundles every
markdown file there unconditionally -- build.ts has no exclusion hook, and the deny-list only
reaches the skill classifier -- so a /promote living in the sandbox would ship itself into every
bundle as a prompt template.
It encodes three guards, each of which produced a confident wrong result while building the demo:
a missing .git lets the CLAUDE.md walk climb out of the project and sweep ancestor context; an
unset $SH_HARNESS_DIR silently degrades the binary check to inventory_unavailable, because the
inventory resolves relative to the harness module and there is no installed binary; and Redis on
6379 reaches this repo's own test container, which reports a successful upload before the harness
fails with `config bundle not found`. Both halves of the invocation were verified rather than
assumed: without --project the CLI promotes `.../serverless-harness/harness`, and without the HOME
override it resolves 62 skills, travels 56, and cannot find the entry prompt at all.
The command interprets exit codes rather than echoing them -- 2 is preflight, 3 is a structural
credential match that must not be retried -- which is the only reason to do this in Claude instead
of a shell alias.
Two bugs found while wiring it up, both in already-merged work:
- **The demo's per-digest purge was not enough.** Any other cached bundle carrying the same memory
fact answers the bare arm just as well. Concretely: a bundle built from this same fixture before
Prettier normalised one emphasis marker in the memory file has a different content digest, sat
alongside, and made the A/B control fail while the purge reported success. Both script and
walkthrough now empty the whole cache; re-verified with three stale bundles present beforehand.
- **The walkthrough quoted a stale digest.** `46ee1106…` was captured before that same Prettier pass
changed the fixture, so every expected-output block named a bundle the demo can no longer produce.
Now `43b8c4c0…`, which is what a run produces today.
deploy/claude/tests/promote-command.test.sh pins what makes the command correct rather than merely
present -- most usefully that `allowed-tools` covers every command the body invokes, which caught a
missing Bash(pwd) that would have prompted on every run. `make test-deploy` now globs it too.
Verification: `make lint` (9 hooks), `make test-deploy` (121 checks), and the full demo at 14
passed / 0 failed against kind, run under the condition that previously failed. The documented
promote invocation was executed end to end from a scratch sandbox: 1 skill, zero preflight findings,
and `redis-cli EXISTS` returning 1 in-cluster.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
pdettori
left a comment
pdettori
left a comment
There was a problem hiding this comment.
No blocking findings — this would be an APPROVE, but GitHub won't let me approve your own PR, so it goes in as a comment. CI is 12/12 green, the single commit is signed off with a correct Assisted-By trailer, and the work is well evidenced. Three non-blocking findings inline.
Supply-chain note (not a finding). No paths matched .claude//.vscode/, so the usual agent-config gate didn't fire — but deploy/claude/commands/promote.md is agent config carrying a Bash permission grant, and the PR correctly tells you to cp it into your real ~/.claude/commands/. I read it in full on that basis. No hostile content: no obfuscation, no base64, no curl | sh, no unknown hosts; every command targets localhost or your own cluster. Flagging the category so the reasoning is on the record, not because anything is wrong.
I also checked whether the purge rewrite orphaned CACHE_DIR — it doesn't, it's still load-bearing at lines 337/344 for the token-cache assertion. Not a finding.
The two suggestions are both about the new asset rather than the demo fixes: the allowed-tools grant is broader than the body needs, and the test that guards it verifies less than its header comment claims. The second is the more interesting one — the extraction only sees first words, which is the same blind spot that let pwd through.
One nit that has no line to attach to: the PR body footer reads “🤖 Generated with Claude Code”. The hub CLAUDE.md (“PR Description Attribution”) rules that phrasing out in favour of Assisted-By: Claude Code. The commit trailer is already right — it's only the body.
Areas reviewed: Claude Code command asset, Shell (bash test + demo script), Makefile, Docs, PR/commit conventions, security.
Agent/IDE config (.claude/.vscode): none by path; deploy/claude/commands/promote.md reviewed as agent config by intent.
| awk '/^```bash$/{f=1; next} /^```$/{f=0} f' "$CMD" | ||
| } | sed -E 's/^[[:space:]]+//' | | ||
| sed -E 's/^([A-Za-z_][A-Za-z0-9_]*=("[^"]*"|[^[:space:]]*)[[:space:]]+)+//' | | ||
| grep -oE '^[a-z][a-z0-9_-]*' | sort -u | grep -vE '^(echo|then|else|fi|do|done)$')" |
There was a problem hiding this comment.
suggestion — this check verifies less than the header comment claims, and there's a live instance.
The header says allowed-tools "must cover every command the body actually invokes, or the user approves a permission prompt on every run and the 'one command' claim is false." But the extraction keeps only the first word of each probe (grep -oE '^[a-z]...'), so everything after &&/|| is invisible — and echo is then additionally dropped by this skip-list. I ran the pipeline against the real promote.md:
USED = ls lsof pnpm pwd test
PERMITTED = git jq kubectl ls lsof pnpm pwd test
invoked = `&& echo` / `|| echo` in 5 of the 6 context probes,
plus one probe whose first word IS echo
So echo is invoked by nearly every probe, is absent from allowed-tools, and is exempted here. That's the same shape as the pwd gap this test was written to catch — and since you measured that pwd prompted, echo plausibly does too. Either add Bash(echo:*), or keep the exemption and say why in a comment; as written the skip is indistinguishable from an oversight.
Two related observations while I was in here:
kubectldoesn't reachUSEDeither — its invocations (kubectl port-forward,kubectl exec … redis-cli) live in prose backticks rather than in a!probe or a ```bash fence. That's presumably why line 43 needs the hardcodedpermits at least pnpm and kubectlfloor. Worth a comment noting the floor exists to cover the extraction's blind spot, so a future reader doesn't mistake it for redundancy.PERMITTEDcollapsesBash(git rev-parse:*)to baregit, sogrep -cxFwould accept anygitsubcommand against a deliberately narrow grant. Fine today; it quietly weakens the check if the grants are ever tightened.
There was a problem hiding this comment.
Fixed in b550277, and you were right that this was the more interesting one — the extraction now splits on &&/||/;/| before taking words, which surfaces exactly the echo you found. It is granted now rather than exempted.
Also took both related observations:
PERMITTEDkeeps grants WHOLE (Bash(kubectl port-forward:*)→kubectl port-forward) and matches by prefix, so the check strengthens when a grant is tightened instead of accepting any subcommand. Your point about it quietly weakening was going to bite immediately, since the same review asked me to narrowkubectl:*.- The floor now carries a comment saying it exists to cover the extraction blind spot:
kubectl port-forwardand theredis-cli EXISTSverify appear only in prose instructions, so no extraction of probes or fences can see them.
Negative-tested rather than assumed: dropping Bash(echo:*) fails 2 checks, widening back to Bash(kubectl:*) fails 3.
| --- | ||
| description: Promote this project's Claude Code workflow into the serverless harness | ||
| argument-hint: <entry-prompt> [--dry-run] [--deny <skill>] [--sandbox-image <ref>] | ||
| allowed-tools: Bash(pnpm:*), Bash(kubectl:*), Bash(test:*), Bash(ls:*), Bash(jq:*), Bash(lsof:*), Bash(pwd), Bash(git rev-parse:*), Bash(git init:*) |
There was a problem hiding this comment.
suggestion — narrow the grant, since this file is designed to live in real user scope.
Bash(kubectl:*) and Bash(pnpm:*) permit every subcommand — kubectl delete included — and the PR's whole argument (which I agree with) is that this file belongs in ~/.claude/commands/ where Claude Code sees it in every project. The body only ever needs three shapes: kubectl port-forward, kubectl exec, and pnpm --dir … promote.
This file already proves multi-word prefixes work here (Bash(git rev-parse:*)), so it's a one-line tightening:
Bash(kubectl port-forward:*), Bash(kubectl exec:*), Bash(pnpm --dir:*)
Also Bash(jq:*) and Bash(git rev-parse:*) are granted but never invoked by the body. jq is defensible as forward-looking if you expect Claude to read the lockfile when reporting the digest — but then the body should say so, otherwise it reads as leftover. git rev-parse looks genuinely unused: guard 1 uses test -e .git.
Bounded blast radius (grants apply only during /promote, and installing it is deliberate), which is why this is a suggestion rather than a blocker.
There was a problem hiding this comment.
Mostly taken in b550277 — kubectl:* is now kubectl port-forward:* + kubectl exec:*, and jq and git rev-parse are gone (you were right that guard 1 uses test -e .git; git init stays because the guard offers it).
I did not narrow pnpm, and I want to flag that as a reasoned partial rather than an oversight. The documented invocation begins with env assignments — HOME="$PWD" REDIS_URL=... pnpm --dir ... — so a Bash(pnpm --dir:*) prefix would not match the command string as written, and the env has to be inline because exports do not survive between tool calls. Narrowing it looked like it would make the command prompt on every run, which is worse than the broad grant. I could not verify how the matcher treats a leading env assignment, so if you know it strips them I will narrow it. The reasoning is now a comment in the file so a later edit does not just widen it back.
| for p in "${POOL_PODS[@]}"; do | ||
| kubectl exec -n "$NS" "$p" -- test -e "$CACHE_DIR" 2>/dev/null && still=$((still + 1)) | ||
| n=$(kubectl exec -n "$NS" "$p" -- sh -c 'ls -1d /workspace/.sh-config/sha256-* 2>/dev/null | wc -l' 2>/dev/null | tr -d ' \r') | ||
| still=$((still + ${n:-0})) |
There was a problem hiding this comment.
nit — a failed kubectl exec is counted as evidence of an empty cache.
If the exec fails for any reason other than an empty glob — pod not ready, evicted, transient API error — stderr is discarded by 2>/dev/null, n comes back empty, ${n:-0} makes it 0, and the pod contributes nothing to still. The run then prints config cache emptied … so the control is honest having verified nothing on that pod.
The pre-existing test -e version had the same optimistic failure mode, so this isn't a regression. What changed is the strength of the claim: it went from a narrow per-digest statement to "the cache is empty," which is precisely the assertion the A/B control's integrity rests on — and the reason for this rewrite was a control that passed while being wrong. Distinguishing "zero bundles" from "couldn't look" would close the loop:
n=$(kubectl exec -n "$NS" "$p" -- sh -c 'ls -1d /workspace/.sh-config/sha256-* 2>/dev/null | wc -l' 2>/dev/null | tr -d ' \r')
[ -n "$n" ] || ko "could not inspect the cache on $p; the control is unverified"There was a problem hiding this comment.
Fixed in b550277. Unreadable pods are counted separately and reported with ko rather than folded into ${n:-0}. Your framing is the reason I took a nit as substantive: the claim widened from one digest to "the cache is empty", so an uninspectable pod is now load-bearing — and a false green here is the exact shape the purge exists to prevent. Verified against a deliberately nonexistent pod: still=1 unreadable=1, and the guard fires instead of passing.
…y what it claims All three review findings on rossoctl#217. Two were the same defect wearing different clothes: a check that claimed to cover "every command the body invokes" while only ever seeing first words. **The test now sees what the body actually runs.** Splitting on `&&`/`||`/`;`/`|` before extraction reveals `echo` in five of the six Context probes -- invoked everywhere, granted nowhere, and additionally exempted by a skip-list, so the gap was indistinguishable from a decision. `echo` is now granted. Grants are also matched WHOLE rather than collapsed to a first word: collapsing `Bash(kubectl port-forward:*)` to `kubectl` would have accepted any subcommand against a deliberately per-verb grant, i.e. the check would have weakened precisely when the grant was tightened. Both directions are negative-tested: dropping `Bash(echo:*)` fails 2 checks, widening back to `Bash(kubectl:*)` fails 3. **The grant is narrowed where narrowing is sound.** `kubectl:*` becomes `kubectl port-forward:*` and `kubectl exec:*` -- this file installs into real user scope, so `kubectl:*` would carry `kubectl delete` into every project. `jq` and `git rev-parse` were granted and never invoked, and are gone; `git init` stays because guard 1 offers it. `pnpm` deliberately stays broad, which is a partial acceptance rather than an oversight: the documented invocation begins with env assignments (`HOME="$PWD" REDIS_URL=... pnpm --dir ...`), so a `Bash(pnpm --dir:*)` prefix is not reliably matched, and the env must be inline because exports do not survive between tool calls. A comment in the file records that, and the reasoning is on the review thread for correction if the matcher does handle env-prefixed commands. **A failed `kubectl exec` no longer counts as evidence of an empty cache.** `${n:-0}` scored an unreachable pod as clean, so the run could print "config cache emptied ... so the control is honest" having verified nothing there -- the same false-green shape the purge itself exists to prevent, and now load-bearing because the claim widened from one digest to the whole cache. Unreadable pods are counted separately and reported with `ko`. Verified against a deliberately nonexistent pod: the guard fires instead of passing. Also documents the floor that covers the extraction's real blind spot: `kubectl port-forward` and the `redis-cli EXISTS` verify appear only in prose instructions, not in a probe or a fence, so no extraction can see them. Verification: `make lint` (9 hooks), `make test-deploy` (126 checks), and the demo at 14 passed / 0 failed against kind. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
|
All three findings addressed in b550277, with a reply on each thread. The two suggestions turned out
One partial acceptance, flagged rather than buried. I narrowed On the attribution nit — changed in both this PR and #218. One caveat so the record is accurate: Thanks for the supply-chain note. Reading Verification after all three: |
…promote itself Closes the gap left by rossoctl#217. `promote` reads prompts from user scope, so with `HOME` pointed at the project being promoted its `.claude/commands/` IS the prompts directory -- and build.ts adds every markdown file there unconditionally, with no exclusion hook (`userDenyList` reaches only the skill classifier). A slash command that drives promotion therefore could not live in the project it promotes without shipping itself into every bundle as a prompt template. That forced the command in rossoctl#217 into real user scope, which in turn means the authoring session loads the whole of a real ~/.claude while the promoted run gets one skill -- the local/remote parity gap that sandbox-first authoring exists to close. `--exclude-prompt <name>` is repeatable and accounted for in both directions, because the interesting failure is silence: - excluding the **entry** is an error (`entry_excluded`), and it suppresses the `unknown_entry` that would otherwise fire from the same cause -- one report naming what the user typed, not two naming a symptom; - an exclusion matching **nothing** warns (`prompt_exclude_unmatched`), because that is what a typo looks like, and a typo here fails in the worst available way: it ships the prompt you asked to keep out; - an exclusion that **matched** also warns (`prompt_excluded`), so an omission is never silent; - an empty value is rejected at parse time rather than pushed as `''`, which would match no prompt. TDD throughout: five buildBundle behaviours and three CLI ones, each watched failing first. Two things that caught out: - The `requires a value` test initially passed against the unimplemented flag, because `unknown flag: --exclude-prompt` also matches /--exclude-prompt/. Tightened to the specific message so it can only pass for the right reason. - Adding the extra prompts to the shared build fixture broke an unrelated test asserting `promptNames` is `['go']`. The fixture is now local to the exclusion suite rather than the existing expectation loosened -- collateral change is not coverage. Verified at the CLI, not only in unit tests: exclusion warns and omits (exit 0), a typo warns (exit 0), excluding the entry aborts with `entry_excluded` (exit 2), and a missing value fails fast (exit 1). `make typecheck`, `make lint`, `make test-deploy` and `pnpm -r test` all pass -- harness 350, knative-server 209, k8s-sandbox 133, experiments 59, relay 16, work-queue 9. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
…-prompt exists Completes the follow-up rossoctl#218 promised and rossoctl#217 could not take. rossoctl#217 had to install /promote into real user scope, and justified it with a constraint that no longer holds: that a command living in the project it promotes would ship itself into every bundle. --exclude-prompt removes that, so the placement that actually buys local/remote parity becomes available. The runbook now offers both, and says what each costs: - **Option A, real user scope.** Simpler. The authoring session loads your whole ~/.claude, so the local agent has every skill you own while the promoted run gets the sandbox's one -- which makes "it behaved the same locally" weaker evidence than it looks. - **Option B, in the sandbox**, with Claude Code launched as `HOME=$SANDBOX claude`, so the local agent sees exactly what the promoted run will. Costs a re-auth and your own skills for the duration. That is the standard dev/prod-parity trade, and parity is this demo's whole claim. /promote self-excludes rather than making the reader remember: a Context probe reports whether `.claude/commands/promote.md` exists in the project, and the body adds `--exclude-prompt promote` only in that case. The condition matters as much as the flag -- passing it unconditionally would trip the flag's own typo guard (`prompt_exclude_unmatched`), which exists precisely because an unmatched exclusion ships the prompt you meant to omit. Measured, and recorded in the runbook because it is the reassuring part: **both options produce the same bundle.** Option B without the exclusion is 19456 bytes -- the command itself travelling -- and with it 12288 bytes at sha256:43b8c4c0..., byte-identical to Option A. So every digest quoted in the walkthrough holds for either placement. Also reconciles what the rebase exposed: README carried rossoctl#217's claim that a project-local /promote "would ship itself into every bundle" two paragraphs above rossoctl#218 documenting the flag that prevents it, plus a doubled lead-in from the two edits landing separately. The "Notes and limits" fidelity entry is rewritten rather than deleted: Option A's gap is real, it is now a choice rather than a constraint, and a performer should say which option they ran if asked whether local matched remote. Verification: `make lint` (9 hooks), `make test-deploy` (129 checks), the demo at 14 passed / 0 failed against kind, and Option B exercised end to end from a scratch sandbox -- with and without the self-exclusion -- to produce the two byte counts above. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
1 participant
What this does
Adds the slash command the promotion spec anticipated — "the Claude Code slash command is a thin
wrapper over it" (§4.3) — so a workflow is promoted from the session it was authored in rather
than from a second terminal:
Promote-only by design. It does not dispatch, so it stays useful with no cluster reachable and
leaves
.claude/promoted.lock.jsonas the hand-off. Dispatch remains the one-fieldconfigRefon aprompt envelope.
deploy/claude/commands/promote.mdcpinto~/.claude/commands/deploy/claude/tests/promote-command.test.shdocs/demos/promoted-workflow-demo.mdREADME.mdSH_HARNESS_DIR, CLI kept as the alternativeWhy it installs into your real
~/.claude/commands/Mechanical, not taste. With
HOMEpointed at the project,promotereads that project's.claude/commands/as its prompts directory and bundles every markdown file there unconditionally —build.tshas no exclusion hook, anduserDenyListonly reaches the skill classifier. So a/promoteliving in the sandbox would ship itself into every bundle as a prompt template. Inreal user scope, Claude Code sees it in every project and
promotenever does.Three guards, each from a confident wrong result
.git→ theCLAUDE.mdwalk climbs out of the project and sweeps ancestor context,including a personal
~/CLAUDE.md, into a bundle bound for a shared store.$SH_HARNESS_DIRunset → the sandbox inventory resolves relative to the harness module andthere is no installed binary, so running the CLI from the project silently degrades the binary
check to
inventory_unavailable.before the harness fails with
config bundle not foundfor that digest.Both halves of the invocation were verified rather than assumed:
The command also interprets exit codes rather than echoing them — 2 is preflight, 3 is a
structural credential match that must not be retried. That interpretation is the only reason to do
this in Claude rather than a shell alias.
Two bugs found in already-merged work
The demo's per-digest purge was not enough. Any other cached bundle carrying the same memory
fact answers the bare arm just as well. Concretely: a bundle built from this same fixture before
Prettier normalised one emphasis marker in the memory file (
*opened*→_opened_) has a differentcontent digest, sat alongside in
sandbox-0, and made the A/B control fail while the purge reportedsuccess — the same failure class the purge was added to prevent. Script and walkthrough now empty
the whole cache, re-verified with three stale bundles present beforehand.
The walkthrough quoted a stale digest.
46ee1106…was captured before that same Prettier pass, soevery expected-output block named a bundle the demo can no longer produce. Now
43b8c4c0….Verification
make lint(9 hooks) andmake test-deploy(121 checks — the new test is globbed in) pass.preflight findings, and the prescribed
redis-cli EXISTSreturns1in-cluster.failed.
!pwd`` was not inallowed-tools, which wouldhave prompted on every run.
Known limit, unchanged by this PR
Running Claude Code with your real
HOMEso it can see/promotemeans the authoring session loadsyour actual
~/.claude(56 travelling skills) while the promoted run gets the sandbox's one — solocal behaviour is not identical to remote. True parity needs Claude Code itself launched with
HOME=$SANDBOX, which puts/promoteback in the sandbox (travelling) and costs a re-auth. Named inthe demo rather than papered over.
Assisted-By: Claude Code