Skip to content

feat: promote a local Claude Code workflow into the serverless harness - #214

Merged
pdettori merged 48 commits into
rossoctl:mainfrom
pdettori:feat/claude-code-workflow-promotion
Sep 3, 2026
Merged

feat: promote a local Claude Code workflow into the serverless harness#214
pdettori merged 48 commits into
rossoctl:mainfrom
pdettori:feat/claude-code-workflow-promotion

Conversation

@pdettori

@pdettori pdettori commented Sep 3, 2026

Copy link
Copy Markdown
Member

What this does

Lets you build an agent workflow locally in Claude Code — skills, the CLAUDE.md chain, a slash
command, project memory — and run it unchanged in the serverless harness.

sh promote canonicalises that configuration into a USTAR tar, content-addresses it by sha256,
stores it in Redis, and writes a committable .claude/promoted.lock.json. A leaf then runs it by
adding one field:

{ "sessionId": "run-1/item-1", "kind": "prompt", "prompt": "", "configRef": "sha256:…" }

Re-promoting unchanged configuration uploads nothing. Design:
docs/specs/2026-09-02-claude-code-workflow-promotion-design.md,
plus ADR-0030 (bundle + transport) and
ADR-0031 (memory travels read-only). Both ADRs are
deliberately left Proposed — accepting them is a maintainer call, and Rule 1 makes them immutable
once accepted.

MCP servers and subagents are explicitly out of scope (spec §2, §9).

Shape

New package @sh/config-bundle holds all pure bundle logic (tar, resolve, classify, secret scan,
lockfile, preflight, build). The harness side adds the CAS store, the resolver that materialises into
the pod's emptyDir, the sandbox overlay, and the promote CLI. Only 7 existing files are modified;
everything else is additive.

The fs-free split (ADR-0020) drives the design: skill prose must reach the harness pod, while
readable content must reach the separate sandbox pod. The overlay pushes a digest-keyed cache under
flock and binds per-leaf paths into it.

Preflight blocks on facts, warns on heuristics

This is the design decision most worth reviewing (spec §6, D10). Three checks were written as
blocking, then demoted after being measured against a real ~/.claude: the secret scan produced 11
false blocks, the binary check 32, the sibling-path check 182. Only unknown_entry blocks, plus a
structural secret match. A preflight that cries wolf gets ignored, and one that lies is worse than
none.

The secret scan is two-tier as a result: five structural rules (AWS/GitHub/OpenAI/Slack tokens,
PEM private-key blocks) refuse the upload and exit 3; two prose heuristics only warn. The README
states this distinction precisely, because promising a guarantee about credentials that the code
does not provide would be the worst error here.

Verification

Against upstream/main @ 6066dc4, with a local Redis:

gate result
make lint pass — all 9 pre-commit hooks
make typecheck pass
make test-deploy pass
pnpm -r test 912 passed / 17 skipped / 0 failed across 9 packages

The full pipeline was also exercised end to end outside the test suite: an 8.60 MiB bundle from a
live ~/.claude (55 skills) through putBundlegetBundleunpackBundle → a real
DefaultResourceLoader, with the harness's own CLAUDE.md confirmed absent from the result. The
overlay scripts were executed against a real container.

Cold-path cost, measured locally (N=10, fresh output dir per run): getBundle 52.6 ms median,
unpackBundle 62.1 ms → 113.7 ms added. Loopback Redis and APFS, so indicative of the added
work only.

Not done — please read before approving

  • The end-to-end in-cluster cold-start comparison is owed, not taken. The spec status says
    Implemented (cold-start measurement owed) rather than a bare "Implemented", and §8 carries the
    reproduction commands. Taking it needs an image build, kind load, and a forced new Revision.
  • The live smoke has never run in a cluster. It is env-gated (SH_PROMOTE_LIVE_SMOKE) like the
    existing live gates. I verified its fixture is well-formed — the skill, the entry prompt, and the
    sibling file it must read all travel, with zero preflight errors — but a gate that has never fired
    is not evidence that the promoted path works in a cluster.
  • Two commit messages are stale relative to later decisions: cc89243 says the secret scan
    "refuses the upload rather than warning" (true only of the structural tier now), and the fix-round
    commit's subject reads as adding the $SH_SKILLS_DIR env vars when it removed them. Both are
    moot under a squash merge; happy to reword if you prefer to preserve history.

Where to look first

  • packages/config-bundle/src/preflight.ts — the blocking-vs-warning boundary (D10).
  • harness/src/config-overlay.ts — generated shell run under flock; the shared cache is made
    read-only with chmod -R a-w so ADR-0031's guarantee is enforced by the filesystem rather than by
    convention.
  • harness/src/promote.tsprojectRoot bounds the context-file walk at a .git entry using
    existsSync deliberately, not a directory test, because in a linked worktree .git is a file.
    Without that bound the walk swept ancestor CLAUDE.md files, including a personal ~/CLAUDE.md,
    into a bundle destined for a shared store.
  • harness/src/run-leaf.ts — the configRef wiring, and why the overlay must not be guarded on
    selected.transport (undefined for pods, which is the default deployment).

🤖 Generated with Claude Code

Add the design for promoting a working local Claude Code workflow (skills,
CLAUDE.md, memory, prompt, slash commands) into the harness as a
content-addressed configuration bundle referenced by a new optional
`configRef` envelope field.

Key findings that shaped it: Pi already implements the Agent Skills standard
and reads CLAUDE.md as a context file, but the harness constructs
DefaultResourceLoader without any of it (run-turn.ts:463); and the fs-free
split means a skill's prose must reach the harness pod while anything it
executes must reach the sandbox, from one digest.

Two ADRs, per the one-decision-per-ADR rule:
- 0030: promote as a content-addressed bundle, pruned by compatibility
  (never by relevance), with a generated lockfile and no prose rewriting.
- 0031: promoted memory travels read-only; discoveries return in the leaf
  result, preserving the leaf idempotency contract (run-leaf.ts:67).

Subagent support and MCP promotion are explicitly deferred.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Determinism is by construction -- sorted entries, zero mtime/uid/gid, fixed
modes -- so the digest is a stable bundle identity. No tar dependency, whose
default flags would put reproducibility outside our control.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
…ace dedupe

Follows Pi's discovery rule (a dir holding SKILL.md is a skill root and is not
recursed into) and collapses the plugins/cache duplicate of plugins/marketplaces,
which otherwise double-counts every plugin skill.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
… frontmatter name

Fix unguarded statSync calls that could crash on dangling symlinks or stack overflow
on symlink cycles. Switch to lstatSync and skip symlinks entirely (don't collect,
don't descend). Also strip a single matched pair of surrounding double or single
quotes from the parsed name field to prevent malformed bundle paths.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Correctly implement symlink handling per upstream Pi: follow symlinks and dedupe
by canonical path rather than refusing symlinks entirely. This fixes the
regression where <userDir>/skills or <pluginDir> entries that are symlinks to
real directories were silently discovered nothing.

Thread a visited set through filesUnder and findSkillDirs to break cycles and
collapse aliases. Wrap all statSync and realpathSync calls in try/catch to
tolerate dangling links and unreadable entries without aborting discovery.

Also dedupe at the canonical-path level in resolveSkills to catch symlinked
skill aliases and prevent duplicate entries.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Curated deny-list plus narrow checks, never heuristic inference: the signal
words appear in unrelated prose, and a wrongly-dropped skill fails remotely.
Interaction-dependent skills warn under --mode unattended rather than drop, so
the classifier stays valid for phase-2 live attach.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Finding 1: Complete SHELL_BUILTINS with full bash builtin and keyword set
(60 entries including pwd, declare, if, while, etc.) to prevent false
"missing binary" reports in detectBinaries output, which feeds a preflight
check that exits non-zero on error.

Finding 2: Make DEFAULT_DENY_LIST, SUBAGENT_DEPENDENT, and
INTERACTION_DEPENDENT immutable at the type level with readonly string[]
to prevent accidental mutation of curated, versioned artifacts.

Finding 3: Skip leading environment variable assignments in detectBinaries
(e.g. FOO=bar command) to detect gh in FOO=bar gh pr list. Add shell-naive
comment noting that the parser does not understand quoting.

Finding 4: Add tests for non-shell fence blocks and fence-language isolation,
verifying that ```ts and bare ``` fences yield no binaries while
adjacent ```bash fences are still detected.

Tests: 40 pass (13 classify + 10 tar + 17 resolve).

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
CRITICAL FIX: All three curated lists were using scope-qualified names
(e.g. superpowers:brainstorming) while ResolvedSkill.name contains the bare
frontmatter name (brainstorming). This caused SUBAGENT_DEPENDENT and
INTERACTION_DEPENDENT to never match, leaving subagent-dependent skills
unblocked on a runtime with no Task tool — exactly the failure this
classifier exists to prevent.

Changes:
- DEFAULT_DENY_LIST: replace document-skills:* with bare docx, pdf, pptx, xlsx
- SUBAGENT_DEPENDENT: bare names dispatching-parallel-agents, subagent-driven-development
- INTERACTION_DEPENDENT: bare names brainstorming, receiving-code-review
- Add defensive comment for Claude Code built-in entries retained for future-proofing
- Update comments to explicitly state bare frontmatter name matching

Test coverage added:
- Assert no ':' in any list entry
- Verify each list entry actually drops/flags correctly by bare name
- Regression guard: prevents this silent failure from reoccurring

Tests: 45 pass (18 classify + 10 tar + 17 resolve).

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Refuses the upload rather than warning: credentials reaching a shared
cluster's store is not recoverable by re-promoting. Pattern-based only --
entropy scoring would false-positive on prose and train people to bypass a
gate that has to stay trusted.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
.gitignore:14's unanchored `secrets.*` silently prevented src/secrets.ts and
its test from being committed -- git add skipped them with no error and
git status did not list them, so index.ts exported a module that would not
exist in a fresh clone.

Renamed to secret-scan.ts rather than forcing past the ignore rule or
narrowing it: the pattern is a deliberate credential guard.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
The lockfile is a package-lock, never a package.json: sorted-key JSON with a
trailing newline so promotions diff legibly in review. Notes carry tool-name
mapping and sandbox path translation, keeping skill prose untouched.

Both notes are multi-line on purpose: pi's resolvePromptInput reads an
appendSystemPrompt string as a file path when it happens to exist.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Replace non-total comparators (a < b ? -1 : 1) with localeCompare to ensure
equal-named entries maintain stability. Add comprehensive sorting tests with
out-of-order, multi-element arrays for all sorted fields: binaries, context,
memory, interactionDependent, dropped, skills.

Add nested key sorting verification: assert that keys within skill records and
dropped records are recursively sorted. Add test for missing skillHashes edge
case defaulting to empty string.

This closes all four reviewer findings on determinism, test coverage, and edge
cases. Test deletion of .sort() calls is verified to fail.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Every report ends by naming what cannot be checked locally. A preflight that
implies completeness is worse than none, because it gets trusted. A missing
inventory warns rather than passing silently: a check that could not run is
not a check that succeeded.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
…y links

Two important fixes to prevent false positives and catch real failures:

1. referencedPaths: Extension must start with a letter (kills 1.2.3, 127.0.0.1,
   node>=18.0) and reject glob/comparison chars (kills *.md, node>=18.0). These
   false positives were blocking valid promotions.

2. checkMemoryLinks: Now matches both markdown [Title](file.md) and [[wikilink]]
   forms, with support for |alias suffix and path prefixes like [[notes/alpha]].
   The real MEMORY.md index had markdown links, not wikilinks, so the check was
   never firing on real data.

Both functions keep their severity and quiet behavior when checks pass.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
checkMemoryLinks now rejects markdown and wikilink targets that are:
- External URLs with a URI scheme (https:, http:, mailto:, etc)
- Protocol-relative URLs (//)
- Fragment or query-only links (#, ?)
- Relative paths outside the memory directory (..)

This prevents false dangling_memory_link warnings on links like:
  [doc](https://example.com/file.md)
  [x](../elsewhere/x.md)

The check still catches legitimate dangling local memory links and warns on
them as intended. Regression guard test ensures local links are still checked.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Five claims in the spec and two decisions in ADR-0030 were corrected by
measuring the design against a real ~/.claude (61 bundled skills, 586 files)
rather than reasoning about it. Each had shipped as fact.

- The secret scan is two-tier, not blocking. A single blocking heuristic
  produced 11 hits on a normal machine, all false positives: 7 documentation
  placeholders and 4 code expressions (`TOKEN = crypto.randomUUID`) that match
  because the value character class accepts dotted identifiers. Two sat inside
  the brainstorming skill. As specified, promotion was impossible on day one.
  Structural credential formats still block; the heuristic warns.
- Skill identity is the bare frontmatter `name`, never `plugin:skill`. All 60
  on-disk skills use bare names, so the namespaced deny-list never matched and
  the subagent-dependency check never fired -- the drop the spec promises in
  section 9 would silently not have happened.
- Memory indexes use `[Title](file.md)` markdown links, not `[[wikilinks]]`.
  The real MEMORY.md holds 9 markdown links and zero wikilinks, so that
  preflight check could never fire.
- The promote sample output was illustrative; it is now measured.
- The scanner lives in secret-scan.ts: .gitignore's unanchored `secrets.*`
  silently untracked a module named secrets.ts.

ADR-0030 is amended in place rather than superseded because it is still
Proposed -- docs/adrs/README.md Rule 1 makes an ADR immutable only once
Accepted -- and carries an Amendments section recording what changed and why.
ADR-0031 (read-only memory) is untouched; no finding bears on it.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Digest is defined over content entries EXCLUDING lockfile.json, since the
lockfile records the digest and would otherwise depend on itself. contentDigest()
is shared with the harness resolver so the two cannot disagree about identity.

The secret scan runs before packing, so a dirty tree yields no bundle at all.

checkBinaries' missing_binary finding is demoted from error to warn: measured
against a real ~/.claude (55 travelling skills), the fenced-block scan produced
44 detected binaries and 32 reported missing, roughly half not commands at all
(angular, django, express, fastapi, vue, prisma, branch, rev-parse, and the
literal placeholder your_command). First-word-of-a-shell-fence detection cannot
distinguish a command from prose, so blocking on it refused nearly every real
promotion for mostly bogus reasons. A genuinely missing tool still fails
remotely with a legible "gh: not found", which is diagnosable and
re-promotable.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
…error

A real ~/.claude build produced 183 blocking preflight errors, almost all
from missing_sibling firing on false positives: bare filenames from prose
that tells the reader to create a file (main.py, requirements.txt,
package.json), code expressions (window.open, sys.path), and cross-skill
or example-project references. main.py and references/guide.md are
structurally indistinguishable, so no regex separates them.

checkSiblingPaths now only flags a reference the skill plausibly owns: it
must contain a slash, and its directory prefix must be a directory the
skill actually ships files in (derived from skill.files). Measured effect:
182 false positives down to 9 across a real 28-skill corpus.

The 9 survivors are still false positives (a skill about writing skills
documenting hypothetical references/*.md), so the finding is demoted from
error to warn. This completes the same pattern applied to possible_secret
and missing_binary: preflight blocks only on facts, warns on heuristics.
The only remaining blocking preflight error is unknown_entry, plus the
secret scanner's structural-format throw.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
checkSiblingPaths' ownedDirs previously registered only each shipped
file's immediate parent directory, so a skill shipping references/deep/x.md
never registered ownership of references/ itself, and a reference's
immediate directory was checked in isolation, so a skill shipping only
references/guide.md could not satisfy a deeper reference to
references/deep/missing.md. Both directions produced false negatives:
a skill demonstrably owning a subtree could escape the check entirely.

Both sides of the comparison are now expanded into their full set of
ancestor directory prefixes (a/b/c.md -> a/, a/b/) and checked for
exact-string intersection. This stays symmetric with the check's other
guarantee: matching is Set.has() on the full prefix string, never
startsWith, so references-old/ still does not satisfy a reference under
references/ -- they are different path segments and never produce the
same string in either ancestor set.

Adds a regression test covering all three shapes: a nested shipped file
satisfying a shallower reference, a shallow shipped file satisfying a
deeper reference, and a same-prefix sibling directory that must NOT
satisfy a reference (the guard against re-loosening into false positives).

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
…ection

Second amendment pass, again driven by measurement during implementation.

Preflight severity (new decision D10). The first real end-to-end bundle build
produced 183 blocking errors, so a real promotion would have failed outright.
The binary check reported 32 missing on a live ~/.claude, roughly half not
binaries at all (angular, django, vue, rev-parse, and `your_command`, a literal
documentation placeholder). The sibling check fired 182 times across 28 skills,
because skill prose names main.py, requirements.txt and package.json -- files
the reader creates -- alongside code expressions like window.open and sys.path.
Neither is fixable by tightening: main.py and references/guide.md are
indistinguishable in shape.

So both are advisory now, and the principle they establish is recorded as D10:
preflight blocks only on facts, warns on heuristics. The sole blocking error is
unknown_entry, alongside the secret scan's structural-format throw. Three
independent measurements forced this one check at a time; a gate that refuses
every legitimate promotion protects nothing.

Section 11 adds the recommended workflow, on the project owner's direction:
author in a minimal local sandbox rather than promoting a whole ~/.claude. The
measurements support it -- 45 of the warnings on a real bundle came from skills
the workflow never uses, and the classifier and deny-list exist only to cope
with an uncurated environment. It also records what sandbox-first would restore
(blocking preflight becomes viable again) and its honest cost (authoring in a
stripped sandbox is less comfortable than a real setup).

Measured bundle size corrected to 8.6 MB across 411 entries.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
An existing key means identical content, so an unchanged re-promotion skips the
write entirely. Fetch verifies by recomputing the content digest and throws on
mismatch rather than degrading -- a silently unconfigured agent produces
plausible-but-wrong work.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
…kip, add comparison test

Three fixes addressing defects in the module's threat model:

1. Validate digest matches tar before any write: prevents key poisoning where a
   mismatched pair blocks the correct value for 30 days. Cost is one untar+hash
   on an operation that already gzips megabytes.

2. Refresh TTL on skip (re-promotion): unchanged content hits the exists branch
   and returns without touching EX. Bundles must not age out while in active use.

3. Add test for digest comparison path: existing tests only hit gunzip/untar
   failures. New test stores valid bundle B under key A and verifies getBundle
   rejects it (exercising the equality check rather than decode failure).

The fakeRedis fixture now tracks expire() calls for assertion.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Carries the harness CLAUDE.md leak regression: pi's ancestor walk for context
files reaches this repo's own CLAUDE.md, which would silently become every
promoted session's instructions. It fails as plausible-but-wrong behavior, never
as an error, so it gets a named test and a comment saying why.

noSkills still honours additionalSkillPaths (resource-loader.ts:405-407), so the
promoted session loads exactly the bundle's skills.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
…e/cleanup coverage

Three review findings on Task 9:

1. The "append-N order" test used append-0/append-1, an already-sorted fixture,
   so it passed whether or not unpackBundle sorted at all. Reversing the source
   array does not fix this: canonicalTar sorts entries byte-wise before writing
   (tar.ts), so untar always hands unpackBundle entries in lexicographic path
   order regardless of source order -- verified by removing the sort and
   observing all 13 tests still pass. The fix instead uses append-2/append-10,
   whose lexicographic order ('10' < '2' byte-wise) diverges from their numeric
   order, so only a genuinely numeric-aware comparator produces the correct
   result. Confirmed by removing the sort (test fails), swapping it for a plain
   lexicographic sort (test still fails), then restoring the real
   implementation (test passes).

2. Added a test for an absolutely-rooted escape path ('/etc/passwd'), which
   resolve() discards the base for -- previously only traversal ('../') was
   tested.

3. Added a test that a rejected unpack leaves no '.tmp-' staging directory
   behind, previously verified only by reading the rmSync in the catch block.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Probes for the digest before pushing bytes, so a 200-leaf fan-out transfers the
bundle once rather than 200 times. Shared cache is populated under converge.ts's
flock and staged-then-renamed; the per-leaf artifact is a link under the leaf
workspace, so cleanupWorkspace stays the only teardown path.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Critical production fixes:
1. Add pipefail to base64|tar pipeline so truncated stdin fails the script
   rather than silently populating an incomplete shared cache
2. Add trap EXIT to clean up staging dirs on extraction failure, preventing
   indefinite accumulation on long-lived pooled sandboxes
3. Document the benign undrained-stdin race in the script so future readers
   do not "fix" it by reintroducing the transfer optimization or re-extracting
   over a populated cache

Three new tests verify: pipefail is present, trap is armed before extraction,
and race reasoning is documented in the script.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
The loader options are assembled by a pure function so the back-compat claim is
assertable rather than asserted: with no promoted bundle the options object has
exactly the four base keys it has today.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Both halves come from one digest and a failure of either fails the leaf: a turn
run with silently-absent configuration produces plausible-but-wrong work.

No knative-server change is needed -- the server passes the envelope through
whole (runLeaf at server.ts:389, enqueue at :368) and isPromptEnvelope is a type
guard, not a field-by-field rebuild.

Also carries over Task 11's key-collision regression guard in
run-turn-promoted.test.ts: resourceLoaderOptionsFor's `{...base, ...promoted}`
merge would let a promoted key silently override a colliding base key with no
error; pins that no such collision exists today.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
getBundleRedis cached the resolved client after the await, a check-then-act
race: two leaves arriving before the first connect() settled would each
create and connect a client, leaking the loser's connection silently. Cache
the in-flight promise instead, mirroring RedisLeaseStore (sandbox-lease.ts),
and clear the cached slot on a rejected connect so a transient Redis outage
doesn't poison every later leaf with a permanently rejected promise. Also
switches to the static `import { createClient } from 'redis'` used by every
neighbouring Redis wrapper in this file's callers.

Move the heartbeat start to immediately after the sandbox lease is obtained,
before resolving/overlaying the promoted config. Resolve+overlay fetches a
multi-MB bundle from Redis and pushes it into the pod over up to three
kubectl execs; with the heartbeat starting only afterward, nothing refreshed
the lease during that window and a slow cluster could reclaim it mid-overlay.

Also asserts, in the overlay-failure test, that the fallback KubectlTransport
built for a transport-less (pod) lease is still closed on that failure path.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Argv parsing and input assembly are a pure module so they are unit-testable;
promote-cli.ts is argv plus I/O only. A secret-scan hit exits 3 with the offending
path and line, and preflight errors exit 2 before anything is uploaded.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
- Finding 1: Lockfile was written before upload confirmed, leaving dangling
  references if Redis connection failed. Now written only after putBundle
  succeeds via a new writeLockfile() helper.

- Finding 2: --dry-run flag still wrote the lockfile unconditionally. Now
  skips the write entirely and shows '(--dry-run: not written)' in output.

- Finding 3: collectContextFiles walked all the way to filesystem root,
  inadvertently including ancestor files like ~/CLAUDE.md in shared bundles.
  Added projectRoot() function that bounds the walk at .git, falling back
  to cwd when no repo is found. Added tests: boundary is respected, and
  files above .git are excluded.

- Finding 4: Added test for projectMemoryDir with hyphens in path segment
  to pin the upstream-matching slug behavior.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
…d lossiness

The Task 13 re-review found the do-not-improve note on `projectMemoryDir` was
never written, leaving the lossy slug looking like a defect to fix. Document why
it is inherited on purpose: the function's job is to FIND the directory Claude
Code already created, so a collision-free scheme would miss it and silently
promote no memory.

`projectRoot`'s docstring also claimed it looks for a `.git` *directory*. In a
linked worktree `.git` is a FILE holding a `gitdir:` pointer, and this repo works
out of worktrees constantly -- so a reader trusting that comment could "fix" the
check into a directory test and reintroduce the ancestor-CLAUDE.md leak in the
checkout where it matters most. Verified against 16 live worktrees.

Add tests for both: the `.git`-as-file boundary, and termination on the
filesystem root plus relative and nonexistent paths (a missing termination check
hangs the CLI rather than failing it). Mutation-checked -- rewriting the boundary
as a directory-only test fails the worktree case and nothing else.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Preflight compares a workflow's detected binaries against these files without
cluster access. Two checks because they catch different failures: the shape test
runs every PR and catches schema mistakes; the drift check needs the image and
catches the file claiming something the image lacks.

An inventory that has drifted makes preflight lie, which is worse than having no
preflight, because people stop checking it.

The inventory is the real 347-binary enumeration of the published image
(digest sha256:0683379d6368ab14c41d9bb46683178946091abba47b1832756d89f39afcdb9f),
not a curated subset -- a curated list invents false "not in inventory" warnings
for binaries the image actually has. The drift check runs against a single
container for the whole declared list rather than one container per binary, since
the CI job would otherwise spend well over a minute on container start-up alone.
The CI step keeps `exit 0` on a failed pull (so a registry outage does not fail
unrelated PRs) but emits a `::warning` annotation so a broken check is visible in
the PR UI instead of silently going dark, and asserts jq is present so an absent
jq degrades to a loud CI failure rather than a silent no-op.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
…arness, not the cwd

Task 14 ships `deploy/knative/sandbox-inventory/<image>.json` so preflight can
verify a workflow's binaries without cluster access. It was inert: `readInventory`
resolved that harness-shipped asset against `process.cwd()`, and `sh promote` is by
design run from the *user's* project -- which will never contain
`deploy/knative/sandbox-inventory/`.

Measured before the fix: run from the repo root, the check produced 29 findings;
run one directory deeper, it produced none and reported `inventory_unavailable`
instead. A check that silently stops checking is the "lying preflight" this
inventory was added to prevent, so this made the whole deliverable a no-op for
every real caller.

Resolve module-relative (walk up from the harness package) with the cwd path kept
as a lower-precedence fallback, so a caller can still override with a local file.
`cwd` remains correct for genuinely user-scoped inputs -- context files, lockfile
output, the memory directory -- and only the shipped inventory changes.

Moved into promote.ts, the tested helpers module, and covered: the filename
derivation both sides agree on, module-relative discovery from an unrelated cwd,
the cwd fallback, module-over-cwd precedence, absent-everywhere, and candidate
termination. Verified end to end from `harness/` and from `/tmp`: both now report
0 `inventory_unavailable` and 29 real findings.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
The smoke's assertion requires the model to resolve a relative path from a
promoted skill's instructions against $SH_SKILLS_DIR in the sandbox, which is
what proves path translation end to end -- otherwise the first proof arrives in
production. Gated behind SH_PROMOTE_LIVE_SMOKE + ANTHROPIC_AUTH_TOKEN +
KAGENTI_SANDBOX_POOL_SELECTOR; confirmed it skips cleanly (2 skipped, 0 failed)
with none of those set.

The end-to-end in-cluster cold-start comparison could not be taken here -- the
deployed image predates this branch, and taking it needs a full image build,
kind load, and a forced new Revision -- so it is not asserted and
deploy/knative/EXPERIMENTS.md is untouched. What was measured locally (added
cold-path cost of the Redis fetch + digest verify + untar, not an end-to-end
cold start) is recorded instead in the spec's Testing and acceptance section,
with caveats and the owed repro commands. The spec Status reflects the gap:
Implemented (cold-start measurement owed).

README's new section states the secret scan's two tiers accurately: a
structural key-shape match refuses the upload, a weaker prose heuristic warns
and lets it proceed -- not a blanket "refuses to upload on a hit".

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
…y it read

Task 14 review, 1 Critical + 1 Important.

Critical: `verify-sandbox-inventory.sh` neutralised the container command substitution
with a blanket `|| true` so a non-empty `missing` would not trip `set -e`. That also
swallowed the runtime's OWN failure, leaving `missing` empty and printing
"PASS: inventory matches". Reproduced: pointed at a nonexistent ref, the script reported
PASS and exited 0 without ever inspecting an image — a drift check that cannot fail,
which is the exact failure mode the inventory exists to prevent.

Now the runtime's exit status is captured explicitly, and the container prints a
`SENTINEL_VERIFIED` line as its last act so a status-0 but truncated run (OOM, SIGKILL)
also cannot read as "nothing missing". The inner loop still always exits 0, so a
non-zero status unambiguously means the runtime or image failed rather than a binary
being absent. Verified all three paths: real image PASS exit 0 (347 verified);
nonexistent ref now exit 1; injected `zzz-not-real` still exit 1 and named.

Important: inventory precedence was silent. Module-relative outranks cwd-local, so a
caller who deliberately dropped an override beside their project was shadowed by the
shipped copy with no way to tell. Added `resolveInventoryPath` and the CLI now prints
the path it read and the binary count, or says plainly that the check will warn rather
than verify.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
sh promote invoked as `cd harness && pnpm promote` silently bundled the
harness checkout's own CLAUDE.md and zero memory files, because pnpm sets
cwd to the package directory rather than the caller's project. Add an
explicit --project flag (resolveProjectDir) so the CLI fails loudly on a
missing directory instead of promoting the wrong one, and prove it
end-to-end: --dry-run from harness/ with --project pointed at the repo
root now reports "context 2 file(s), 11 memory file(s)"; the same command
without --project reports "context 1 file(s), 0 memory file(s)".

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
…r it down

Three fixes land together because they entangle the same files (mainly
run-leaf.ts and config-overlay.test.ts) enough that splitting them by
hunk would risk mis-splitting the assertions:

- notes.ts referenced $SH_SKILLS_DIR / $SH_MEMORY_DIR, but nothing ever
  sets those -- the bundle is built once, before any leaf or sandbox
  exists, and every sandbox tool call is an independent `bash -c` with
  no seam to export into. skillsRootNote() now points at the "Skill
  files:"/"Memory files:" lines run-leaf.ts already appends per leaf,
  and that per-leaf fragment is now multi-line (also sidesteps pi-fork's
  resolvePromptInput treating a single-line string as a file path).

- buildCachePopulateScript now does `chmod -R a-w "$TMP"` after +x and
  before the mv, so the promoted cache is actually read-only on disk per
  ADR-0031, not just read-only by convention. The EXIT trap now restores
  write permission first (`chmod -R u+w "$TMP" || true; rm -rf "$TMP"`),
  since `rm -rf` on a directory needs write permission on that directory
  to unlink its own entries -- proven with a local flock-stripped repro:
  a forced failure after the read-only chmod still fully cleans up.

- buildConfigCleanupScript existed and was unit-tested but was never
  called: runPromptLeaf (the promoted prompt-leaf path) never converges
  a workspace, so cleanupWorkspace is unreachable for it, and the
  per-leaf /workspace/leaves/<sid>/.sh-config link it creates leaked
  forever on a long-lived pooled pod. Added a best-effort teardown call
  in runPromptLeaf's `finally`, guarded on a new `overlayCreated` flag,
  reusing the same transport-fallback pattern used for the overlay call
  itself. Proven revert-sensitive: stashing just the run-leaf.ts change
  drops the new test's assertion from "called 2 times" to "called 1
  times" (kubectlTransportMock), matching exactly the leaked-teardown
  defect it guards against.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Consequences said the deny-list and "the blocking secret scan" were the
only things standing between private context and a shared cluster, and
called the scan "load-bearing, not advisory" as if it were one simple
gate. It's two-tier (packages/config-bundle/src/secret-scan.ts): five
structural rules block promotion via SecretScanError (AWS/GitHub/OpenAI/
Slack token and private-key-block shapes), while two prose heuristics
(bearer tokens, key:value-shaped assignments) only warn, because measured
against a real ~/.claude they were false-positive-dominated. Amended in
place to say only the structural tier is load-bearing, and to note the
overlay's chmod -R a-w pass now separately enforces this ADR's read-only
guarantee at the filesystem level. Status stays Proposed; no new section
added, one clause amended.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
…o sandbox exists

The re-review flagged the last case in the class the env-var removal was closing.
`skillsRootNote()` tells the model to look for the "Skill files:"/"Memory files:" lines
that run-leaf.ts appends — but it appends them only when a sandbox is selected, and the
bundle is built before any leaf exists, so the note cannot be omitted conditionally. With
no sandbox, the note pointed at lines that were never written: an instruction referencing
something absent, which is exactly what the env-var indirection was.

Add a fallback clause telling the model that the promoted files are not reachable and to
say so plainly, rather than guessing a plausible path or reporting a file as missing. It
turns a dangling reference into graceful degradation with no new plumbing.

The note stays multi-line, which pi's resolvePromptInput requires — a single-line prompt
fragment is read as a file path when it happens to exist on disk.

Test is revert-sensitive: deleting the clause fails exactly this test and nothing else.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Comment thread harness/test/promote.test.ts Fixed
Comment thread packages/config-bundle/src/preflight.ts Fixed
Comment thread packages/config-bundle/src/preflight.ts Fixed
Comment thread packages/config-bundle/src/secret-scan.ts Fixed
…ic on bundle content

CodeQL on PR rossoctl#214 raised 4 high-severity alerts. Three were polynomial regexes; two of
those are genuinely reachable and were measured before fixing:

  MEMORY.md md-links  /\[([^\]]+)\]\(([^)]+)\)/   40 KB of '['   2281 ms -> 33 ms
  MEMORY.md wikilinks /\[\[([^\]]+)\]\]/          80 KB of '[['  9186 ms -> 62 ms

Both rescan from every '[', so cost grows quadratically -- a crafted MEMORY.md would hang
`sh promote` for minutes on a 1 MB file. This input is not necessarily the user's own:
promote resolves and scans skills from third-party plugin directories. Bounding the
quantifiers and excluding newlines caps the work and is also more correct, since a markdown
link does not span lines. Real memory links are far inside the bounds.

The third alert (secret-scan.ts's PLACEHOLDER `<[^>]+>`) is bounded too, and the comment
there is explicit that this one is defence in depth rather than a live fix: PLACEHOLDER only
ever runs on a WARNING_RULES match, and both rules' value character classes exclude '<', so
the matched text can never contain one. I had initially written a timing test for it -- the
test passed against the UNBOUNDED regex, proving it exercised nothing, so it is removed
rather than kept as false coverage.

The fourth alert was a predictable temp-directory name in a test; `mkdtempSync` creates it
atomically with 0700 instead.

The surviving ReDoS test is revert-sensitive: restoring either unbounded preflight regex
fails it at ~4.6 s against a 1.5 s budget.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>

@pdettori pdettori left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: approve-equivalent — no must-fix findings. Posted as a comment review because GitHub will not let an author approve their own PR.

Findings are all non-blocking. Nothing breaks existing behavior — an absent configRef leaves loader options byte-identical, and resourceLoaderOptionsFor makes that assertable rather than merely intended.

The one I'd address before this runs against real ~/.claude directories is the first comment below: the unbounded directory-symlink walk reproduces the ancestor-CLAUDE.md leak your .git bound already fixes, through a different vector, with the five structural secret rules as the only backstop.

Two things I checked specifically and found correct: the sandbox receives gzipSync(canonicalTar(promotedConfig.entries)), not the raw stored bytes — so non-regular tar entries (symlink/device types that untar drops but the sandbox's tar -x would honor) cannot survive a Redis round-trip into extraction; that digest-verification-vs-extraction gap is genuinely closed. And there are no new external dependencies — @sh/config-bundle adds only devDeps already pinned at identical versions elsewhere.

Author: pdettori (MEMBER — maintainer)
Areas reviewed: TypeScript (config-bundle, harness), Shell (deploy), CI, Docs, Tests
Agent/IDE config (.claude/.vscode): none
Commits: 41, all signed off (DCO green), all conventional prefixes; 13 subjects exceed 72 chars (moot under squash)
CI status: passing (12/12)

Not covered by this review, per your own disclosure: the in-cluster cold-start comparison and the never-fired SH_PROMOTE_LIVE_SMOKE gate. I read the smoke fixture — its assertion is meaningful, since the secret word is only reachable by resolving a sibling path in the sandbox — but a gate that has never fired isn't evidence.

One item that has no inline anchor: the PR body footer reads "Generated with Claude Code". CLAUDE.md:89 requires Assisted-By for AI attribution.

Assisted-By: Claude Code

} catch {
continue;
}
if (st.isDirectory()) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion — Directory symlinks are followed with no containment check. statSync (not lstatSync) resolves the link, so st.isDirectory() is true for something like refs/linked -> ~/Documents; realpathSync then returns the target, and walk() enumerates that tree at arbitrary depth. build.ts:50 reads each result back through the link, so the whole target tree's content lands in a bundle destined for shared Redis.

This is the same failure class as the ancestor-CLAUDE.md sweep that the .git bound at promote.ts:152 exists to stop — just reached by a different vector. Cycle-breaking via canonical paths prevents a hang, but nothing bounds where the walk goes, and the five structural secret rules are the only backstop (they won't match most private content).

Suggest bounding to the skill directory: after realpathSync(p), require the canonical path to be inside realpathSync(dir) before recursing, and skip with a warn finding otherwise. Aliased skill dirs keep working, but a link out of the tree can no longer sweep. The 8be16bd3afe725 history shows following symlinks was a deliberate reversal — this preserves that while bounding it.

Comment thread harness/src/config-resolver.ts Outdated
baseDir: string = DEFAULT_CONFIG_BASE_DIR,
): PromotedConfig {
const entries = untar(tar);
const root = join(baseDir, digestDirName(digest));

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestiondigest here is env.configRef straight off the leaf envelope, never shape-checked. digestDirName only substitutes characters, so a value like sha256:../../../evil yields root = /evil, and renameSync(staging, root) at line 68 then moves the staged tree there. config-overlay.ts:11 has the mirror problem inside the sandbox, where rm -rf "$TMP" runs on the derived path.

To be precise about reachability: this is not reachable through the shipped write path. getBundle runs first and throws BundleNotFoundError, and putBundle can only ever mint sha256:<64 hex> keys, so an exploit needs out-of-band Redis write access. But putBundle's own comment scopes key poisoning in as a threat, and this read path is exactly where a poisoned key would be spent.

A one-line guard closes it: reject anything not matching /^sha256:[0-9a-f]{64}$/ before the digest becomes a filesystem path — ideally in a shared helper that both this and configCacheDir call, so the two halves can't drift.

Comment thread packages/config-bundle/src/tar.ts Outdated

/** Filesystem-safe form of a digest, for use as a directory name. */
export function digestDirName(digest: string): string {
return digest.replace(':', '-');

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitreplace(':', '-') substitutes only the first occurrence. Harmless for a well-formed sha256:<hex>, but it is what turns an unvalidated configRef (see the config-resolver.ts:46 comment) into a path traversal rather than a mangled-but-contained directory name. replaceAll would make this function safe independent of its callers.


if (prevCanonical) {
// Same canonical path: prefer higher precedence scope
if (SCOPE_RANK[skill.scope] < SCOPE_RANK[prevCanonical.scope]) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion — Dedupe leak when the two maps desync. If the same canonical path arrives twice under different names and the second has higher scope precedence, best.set(skill.name, skill) adds the new key but prevCanonical.name is never removed — so one physical skill is emitted twice and its files travel under two separate skills/<name>/ prefixes.

Reachable when SKILL.md has no name: frontmatter (so load falls back to the link directory's basename at line 117) and the same directory is aliased across two scopes. Narrow, but deduping is precisely what this function exists to do.

best.delete(prevCanonical.name) before the set fixes it.

Comment thread README.md Outdated
`RuntimeDefault` seccomp, no service-account token automount.
- **Built on Pi** — wraps a pinned [`kagenti/pi`](https://github.com/kagenti/pi) coding agent through
an injectable `SessionStorageBackend` seam; the agent itself is unmodified.
- **Promote a local Claude Code workflow** — `sh promote` bundles skills, `CLAUDE.md`, memory, and a

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestionsh promote isn't runnable. There's no sh bin in the repo (harness/src/cli.ts takes a prompt, not subcommands), and the wiring added in this PR is harness/package.json's "promote": "tsx src/promote-cli.ts".

The section at line 186 correctly says cd harness && pnpm promote, so the README names the command two different ways about 80 lines apart — and the copyable one in this feature bullet is the one that fails with command not found.

The same string appears in deploy/knative/sandbox-inventory/README.md:3, the design spec (lines 96, 160, 198, 331), and docs/adrs/0030-claude-code-workflow-promotion.md:38. Either add the sh promote subcommand the spec describes, or make the docs say pnpm promote.

}

const promptNames: string[] = [];
for (const name of markdownFiles(input.promptsDir)) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestionmarkdownFiles reads only .md files directly in the directory, so namespaced commands never travel: ~/.claude/commands/ns/cmd.md, which Claude Code exposes as /ns:cmd, is invisible here.

If the entry is the namespaced one, checkEntry catches it as unknown_entry and promotion fails legibly. Any other namespaced command a promoted workflow invokes silently doesn't make it into the bundle and fails remotely with no local signal — which is the class of remote failure the preflight exists to prevent.

Worth either recursing here, or naming this limit in the spec's out-of-scope list next to MCP servers and subagents.

[ -f "$FILE" ] || { echo "no inventory for $IMAGE (expected $FILE)"; exit 1; }

RUNTIME="${CONTAINER_RUNTIME:-docker}"
mapfile -t declared < <(jq -r '.binaries[]' "$FILE")

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit — A third silent-pass path, alongside the two the header documents. Process substitution failure doesn't trip set -e, so malformed JSON or a missing .binaries key leaves declared empty: the container then checks nothing, still prints the sentinel, missing is empty, and the script reports PASS: inventory matches $IMAGE (0 binaries verified in-image).

The shape test's non-empty-array check covers every committed file, so the realistic CI path is protected — this is about the script being self-contained when run standalone, as its own usage line invites.

[ "${#declared[@]}" -gt 0 ] || { echo "ERROR: no binaries parsed from $FILE"; exit 1; }

filesUnder() followed directory symlinks unconditionally via statSync, so a
skill directory containing a symlink pointing outside itself (e.g. to
~/Documents or another skill's tree) had that unrelated content silently
walked and packed into a bundle destined for shared Redis.

Add isWithin(), a path-segment-aware containment check (relative(), not a
bare startsWith, so a sibling like /a/skillsX is never mistaken for being
inside /a/skills). A directory symlink that resolves outside the skill's own
canonical root is now skipped, and a { severity: 'warn', code:
'skill_symlink_escaped' } finding is recorded on the skill and surfaced
through the existing findings pipeline.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
When two scope roots alias the same physical skill directory via a symlink,
and the target's SKILL.md has no name: frontmatter (so load() falls back to
the link directory's basename), the two aliases can carry different
fallback names. resolveSkills()'s dedupe-by-canonical-path branch was
overwriting the winning name's entry in `best` without removing the
previously-recorded loser's entry under its own (different) name, so the
same physical skill could be emitted twice under two separate skills/<name>/
prefixes.

Add best.delete(prevCanonical.name) before best.set(skill.name, skill) in
that branch.

Note on coverage: resolveSkills() always processes roots in fixed
project -> user -> plugin (precedence-ascending) order, so a later-processed
entry can never have strictly better precedence than an earlier-recorded one
for the same canonical path -- the guarding branch that contains this delete
is consequently unreachable via the public API today. Verified by hand: the
line's removal produces byte-identical output for the added test. Kept as
defense-in-depth per the review comment (a future caller that resolves roots
out of order, or in a different rank arrangement, would hit it), and the
test documents this honestly rather than claiming it as a regression test
for this specific line.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
digest.replace(':', '-') only swaps the first colon and silently leaves the
rest. A digest has exactly one ':' today (sha256:<hex>), so this was latent,
but a future multi-colon shape would collide two distinct digests into the
same directory name instead of erroring -- e.g. 'a:b:c' became 'a-b:c'
instead of 'a-b-c'.

Switch to replaceAll and add a regression test with multiple colons.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
… paths

Neither unpackBundle (harness/src/config-resolver.ts) nor configCacheDir
(harness/src/config-overlay.ts) validated the digest string before turning
it into a filesystem path via digestDirName + join(baseDir, ...). A digest
shaped 'sha256:../../../evil' survives the ':' -> '-' substitution with its
'..' segments intact, and path.join then normalizes them, walking the
result outside baseDir entirely -- confirmed by hand:
join(base, digestDirName('sha256:../../../evil')) resolves to a sibling of
base, not a child of it.

Neither pod currently accepts an attacker-chosen digest over the wire today,
so this is defense-in-depth against a digest that originated somewhere less
trusted than the builder in this package -- not a demonstrated live exploit.

Add one shared assertValidDigest() next to digestDirName in
packages/config-bundle/src/tar.ts, rejecting anything not matching
/^sha256:[0-9a-f]{64}$/ via a new InvalidDigestError. The error names the bad
value's shape (prefix present?, hex length, case) but caps how much of a
long value it echoes, so a crafted or oversized digest can't ride unbounded
into logs. Both config-resolver.ts and config-overlay.ts's configCacheDir
now call it before digestDirName.

Tests cover: a valid digest passing through unchanged; the traversal
payload above; wrong-length hex (63 and 65 chars); uppercase hex; a missing
'sha256:' prefix; the empty string; and that the error message caps a 5000-
char attacker string to under 200 chars. On the harness side, a new
config-resolver test asserts the traversal digest creates nothing at all
under base (readdirSync(base) stays []), and the existing config-overlay
quote-injection test is updated to assert on the now-earlier rejection
instead of the shell-escaping it used to exercise.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
buildBundle only reads .md files directly in promptsDir, so a Claude Code
namespaced command (commands/<ns>/<cmd>.md, exposed as the slash command
/<ns>:<cmd>) was silently dropped with no signal -- a promoted workflow that
invoked one would fail remotely, with nothing pointing back at why.

Do not recurse into namespaced prompt subdirectories (unchanged); instead
add namespacedPromptDirs() in build.ts to detect promptsDir subdirectories
containing .md files, and checkNamespacedPrompts() in preflight.ts to turn
each into a { severity: 'warn', code: 'namespaced_prompt_skipped' } finding
naming the namespace, wired into buildBundle's findings array (and so into
the CLI's printed preflight).

Document the limitation in the spec's out-of-scope list (§2), alongside MCP
servers and subagents, matching the existing style.

Tests: a promptsDir with ns/cmd.md produces the warning and excludes the
file (prompts/go.md from the flat fixture still travels); a flat promptsDir
produces no such finding.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
'sh promote' was never a real command -- there is no sh bin in this repo.
The actual entry point is the pnpm script in harness/package.json. Fix all
occurrences in README.md, deploy/knative/sandbox-inventory/README.md, and
docs/specs/2026-09-02-claude-code-workflow-promotion-design.md to read
'pnpm promote' (or 'cd harness && pnpm promote' where the working directory
isn't otherwise established), including in the ASCII pipeline diagram and
the example CLI output block.

docs/adrs/0030-claude-code-workflow-promotion.md is updated the same way;
its Status line (still Proposed) is untouched.

Verified via `grep -rn "sh promote" --include=*.md .` (excluding
.worktrees/ and .claude/): zero remain.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
mapfile with a process-substitution source does not trip `set -e` on
failure, so malformed JSON or a missing .binaries key left `declared` empty
and the script proceeded to check nothing, still print the sentinel, and
report "PASS ... (0 binaries verified)" -- a drift check that silently
verifies zero binaries is worse than no check at all.

Add a guard immediately after mapfile: exit 1 with "ERROR: no binaries
parsed from $FILE" when declared is empty.

Verified against a scratch inventory (no .binaries key) placed temporarily
in deploy/knative/sandbox-inventory/ and pointed at directly: the script
exits 1 with the expected message. The scratch file was deleted afterward;
sandbox-inventory/ contains only the real inventory and its README.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
@pdettori

pdettori commented Sep 3, 2026

Copy link
Copy Markdown
Member Author

All seven addressed. Commits c46eacc (A), fa0a64e (D), 37728d2 (C), eb3005d (B), 0639223 (F), 97e5c30 (E), b7b9845 (G).

Symlink containment (resolve.ts:44) — you were right, and this one mattered most. Bounded to the skill directory's canonical root with a segment-aware relative() check, skipping escapes with a new skill_symlink_escaped warn finding so files aren't lost silently. Following symlinks stays deliberate; aliased dirs inside the tree still work. Verified live rather than only by unit test: a mine/escape-link -> /tmp/.../secret link is skipped and warns, and with the check stubbed out the same probe emits escape-link/private.md. Your skillsX prefix trap has its own test, and both fail if the containment check is reverted.

Digest validation (config-resolver.ts:46) — reproduced exactly as you described: digestDirName('sha256:../../../evil')'sha256-../../../evil', and join('/tmp/sh-config', …)/tmp/evil. Added one shared validator in tar.ts (/^sha256:[0-9a-f]{64}$/), called by both unpackBundle and configCacheDir so the halves can't drift, per your suggestion. All six malformed shapes (traversal, wrong length, uppercase, no prefix, empty, multi-colon) now throw in both; nothing is created outside the base dir. The comment scopes it as defence in depth against the key poisoning putBundle already names, not a live exploit.

replaceAll — done, kept alongside the validator rather than instead of it. Your 'sha256:..:..:evil' case confirmed the old behaviour retained later colons.

Dedupe (resolve.ts:167) — applied, with a caveat worth recording: the guarding branch is currently unreachable. SCOPE_RANK is project:0, user:1, plugin:2 and found is built in exactly that order, so SCOPE_RANK[skill.scope] < SCOPE_RANK[prevCanonical.scope] can never hold, which also means the double-emit can't occur today. The delete is kept as dormant defence — it activates the moment anyone reorders those pushes or adds a scope — and the new test is explicit that it exercises the dedupe-by-canonical-path guarantee but does not fail if the delete alone is reverted. Flagging that rather than claiming coverage it doesn't have.

sh promote — fixed in the docs, not by adding a bin: README.md (both sites), deploy/knative/sandbox-inventory/README.md, the spec (4 places) and ADR-0030 (Status untouched). grep -rn "sh promote" --include=*.md now returns nothing.

Namespaced commands — took the document-and-warn option rather than recursing. A promoted leaf receives exactly one prompt, so non-entry commands aren't invocable mid-session, and a namespaced entry already fails legibly via unknown_entry; the harm you named is the silence. There's now a namespaced_prompt_skipped warn plus the limit listed in the spec's out-of-scope section next to MCP and subagents. Recursing would change prompt naming to ns:cmd and ripple through checkEntry and the lockfile — happy to do it as a follow-up if multi-prompt bundles are wanted.

declared guard — taken verbatim. Worth noting it's the third silent-pass in that one script, after the || true swallowing the runtime's failure and the missing sentinel. Malformed inventory now exits 1.

Gates: make lint 0, make typecheck 0, make test-deploy 0, pnpm -r test 929 passed / 17 skipped / 0 failed (+17 tests). Promote dry-run unchanged at context 2 file(s), 11 memory file(s), with neither new warning triggered on a real ~/.claude.

Also: thanks for independently confirming the gzipSync(canonicalTar(entries)) path — that the sandbox never sees raw stored bytes is the reason the digest-verify-vs-extract gap is closed, and it's worth having that on the record.

@pdettori
pdettori merged commit f3228fc into rossoctl:main Sep 3, 2026
12 checks passed
@pdettori
pdettori deleted the feat/claude-code-workflow-promotion branch September 3, 2026 15:17
pdettori added a commit to pdettori/serverless-harness that referenced this pull request Sep 3, 2026
Shows PR rossoctl#214 end to end: a Claude Code workflow authored in a minimal local sandbox -- one
skill, a CLAUDE.md, one memory file, one slash command -- running unchanged in the harness.
The payoff is an A/B: the same prompt dispatched twice to the same cluster, differing only by
`configRef`. The bare arm asks what a ship note is; the promoted arm emits the house format
and cites an incident id that exists nowhere but the memory directory.

`HOME=$SANDBOX` is the demo rather than a shortcut. It makes promote read the sandbox as USER
scope, so 1 skill travels in 12 KB with zero preflight findings, against 56 skills and ~8.6 MB
for a real ~/.claude -- the sandbox-first authoring the design recommends (spec §11). It is
also what makes the slash command travel at all, since promote reads prompts from user scope
only.

Three findings from making it actually run, each now defended in the script:

1. The shared config cache leaks across leaves. The overlay materialises the bundle into
   /workspace/.sh-config/<digest>/ in the SHARED pool sandbox, world-readable, holding
   context/agents/0-CLAUDE.md and memory/, and it outlives the leaf. A later bare leaf leasing
   that sandbox can explore the filesystem and answer from another run's promoted workflow --
   measured: the second run's bare arm produced the ticket AND the token, having been told
   neither, so the A/B looked fine while proving nothing. The script purges the digest before
   the control run and asserts the purge; the demo's notes name it as a confidentiality
   question on a multi-tenant pool, since ADR-0031 covers write-protection, not visibility.

2. Probing for an existing listener on 6379 uploads to the wrong Redis. This repo's own test
   container publishes 0.0.0.0:6379, so promote reported `uploaded` and the harness then failed
   with `config bundle not found` for that exact digest. The script binds its own port (16379)
   and reads the key back through the cluster's own client, before the two model calls are paid.

3. A warm cluster silently serves a pre-rossoctl#214 image. `kind load` on a mutable tag rolls no new
   Revision, so the feature appears absent. Claim 0 gates on an unknown digest failing loudly
   and prints the forced-roll commands rather than emitting a green run that proves nothing.

The sandbox half is asserted twice: in the model's words, via a token readable only by a `read`
executed in the sandbox pod against a translated sibling path, and on the filesystem, which
holds even when the model declines to read.

Verification: 14/14 claims pass on three consecutive runs against kind, including two where the
cache was warm from the previous run. Every command in the walkthrough was driven by hand as
written. `make lint` (9 hooks) and `make test-deploy` (94 checks) pass. The new cluster-free test
pins the fixture invariants the A/B depends on -- the ticket must appear only in the memory file
and the token only in references/ -- plus that `--teardown` touches no cluster.

This is also the in-cluster end-to-end exercise rossoctl#214 recorded as owed.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants