chore(publish): stage filtered skills at publish time

[lavaman131]

Summary

Replace the committed .agents/skills directory (which carries development-only skills useful in-repo) with the filtered, end-user skill set produced by bunx skills add during the publish job. This keeps dev-only skills out of the published npm package without having to scrub them from the working tree.

Changes

• .github/workflows/publish.yml: New "Stage filtered skills for npm package" step runs before npm publish — clears .agents/skills, runs bunx skills add "$SKILLS_REPO" --skill "*" -a opencode -a github-copilot -g -y, copies the filtered skills into .agents/skills/, then patches package.json in-place via jq to whitelist the directory for npm pack.
• package.json: Removes the static .agents/skills entry from the files array — it's now added ad-hoc at publish time so the committed config doesn't pull in dev skills locally.

Breaking Changes

None — CI-side packaging fix.

[claude]

Code Review

Thanks for the packaging fix — the high-level approach (strip dev-only skills from the published tarball, whitelist .agents/skills just-in-time) is sound. A few things worth addressing before merge.

Blocking

1. PR description is massively out of sync with the diff. The body advertises a new _context/ module (budget.ts, masking.ts, scratchpad.ts, a 220-line test file), D5 prose-guard wiring across three SDK variants, CAVEMAN_PREAMBLE injections, renderHugeFiles, etc. — none of which appears in this PR. The actual diff is 2 files / +25 / -1 and is entirely the CI skill-staging change. Please either split/rebase so the body matches, or rewrite the description — otherwise anyone reading the changelog later will be confused and reviewers can't tell if something got lost in a force-push.

2. Missing claude-code agent filter (.github/workflows/publish.yml:89). The existing precedent in src/scripts/bundle-configs.ts:29 uses all three agents:
```ts
const AGENT_FLAGS = ["claude-code", "opencode", "github-copilot"];
```
but the new step ships only `-a opencode -a github-copilot`. Per `CLAUDE.md`, `.claude/skills` is a symlink to `.agents/skills`, and the release zip (produced by the same `bundle-configs.ts`) includes claude-code. So npm consumers will get a different — smaller — skill set than the release-zip consumers. If this is intentional, worth a comment; if not, add `-a claude-code`.

Should fix

3. Drift between what was tested and what gets published. The step does `bunx skills add "$SKILLS_REPO"` which fetches from the remote git URL's HEAD, not the current checkout. Tests on line 58 ran against the checked-out skills; publish ships whatever the remote default branch had at that moment. During concurrent merges this is a real race. Since the skills already live at `.agents/skills` in the checkout, consider pointing `bunx skills add` at a local path (`.` or `file://`) or filtering the local tree directly rather than re-fetching over the network.

4. No verification that `cp -R` actually copied anything. If `bunx skills add` silently no-ops (e.g. network hiccup, upstream change in flag semantics), `$HOME/.agents/skills/.` could be empty, `cp -R` succeeds with zero files, and an empty skills dir gets published without any signal. Add a post-copy assertion, e.g.:
```bash
if [ -z "$(ls -A .agents/skills 2>/dev/null)" ]; then
echo "::error::.agents/skills is empty after staging" && exit 1
fi
```

Nice to have

5. Hardcoded fork URL. `SKILLS_REPO: https://github.com/flora131/atomic.git\` matches `package.json`'s `repository.url` so it's canonically correct, but `${{ github.server_url }}/${{ github.repository }}.git` would make the workflow portable across forks without edits. (Same nit applies to the existing constant in `bundle-configs.ts:26`.)

6. No version pin for `bunx skills`. `bunx` fetches latest, so reproducibility of past releases depends on whatever `skills` version was current at publish time. Consider pinning (`bunx skills@x.y.z ...`).

7. In-place `package.json` mutation. The `jq ... > tmp && mv tmp package.json` is fine, but it tightly couples "stage skills" and "allow them through the files array." If a future step is inserted between stage and publish, it's easy to miss. A tiny comment in the file pointing from the files array (currently line 42-53 of `package.json`) to the staging step would save the next maintainer a minute.

Positive

• `set -euo pipefail` is the right choice here.
• Explicit `rm -rf .agents/skills` before re-staging avoids the merge-with-stale-contents footgun.
• Removing `.agents/skills` from the committed `files` array (rather than the opposite — committing the filtered set) keeps the dev checkout honest.

Test coverage

No tests for this CI change, which is reasonable given its nature, but it does mean the first real validation will be the next prerelease publish. Consider a dry-run via `npm pack` in CI (even on PR) that diffs the tarball's skill set against expectations — would catch regressions like issue #2 above before they hit npm.

[claude]

PR Review — Context-engineering refinements + staged-skills publish

Thanks for the thorough write-up. The shared _context/ module is a nice consolidation and the tests on masking.ts cover the interesting edge cases well (the untracked-file safety test in particular is the right instinct). Below are the issues I'd want addressed before merge, roughly ordered by severity.

Blockers / likely bugs

1. PR description overstates scope — Ralph is not actually wired up.
The summary says Ralph gets scratchpad persistence, priorRfc/sessionIntent injection, cached infra-discovery, and the CAVEMAN_PREAMBLE. But nothing under src/sdk/workflows/builtin/ralph/ is modified in this diff — no imports from _context/, no calls to initScratchpad / recordPlannerOutput / recordDebuggerReport. This PR ships only the shared utilities + deep-research wiring. Either (a) split the description so reviewers know the Ralph integration is follow-up work, or (b) include the Ralph changes you described. Shipping the scratchpad helpers with zero callers risks them drifting / rotting before the follow-up lands.

2. extractSection / replaceSection break on embedded H2 inside fenced blocks (scratchpad.ts:816, scratchpad.ts:824).
The terminator regex is (?=\n##\s|$). Prior RFCs explicitly stores planner output inside ```markdown fences — and RFCs routinely contain ## Background, ## Implementation, etc. The regex doesn't know about fences, so extractSection(content, \"Prior RFCs\") will terminate at the first ## … line inside any stored RFC, and replaceSection will silently truncate the scratchpad, stranding the tail below as orphan content. latestPriorRFC suffers similarly: /```markdown\\s*\\n([\\s\\S]*?)\\n```/g with the ? lazy quantifier fires at the first \\n\`` it sees, which can be inside a nested fence. Please either escape H2s on write (##→# #` or indent) or use a line-scan parser that tracks fence state.

3. truncateMarkdownReport produces negative tailLen for small caps (masking.ts:553).

const headLen = Math.floor(maxChars * 0.6);
const tailLen = maxChars - headLen - 128;

For maxChars < 320 the tailLen goes negative and content.slice(-tailLen) silently reads from content.slice(headLen) instead of from the tail — returning a middle slice, not the intended head+tail. The default is 8 000 so this doesn't bite today, but the function is exported and parameterised. Add const tailLen = Math.max(0, maxChars - headLen - 128); (and short-circuit if tailLen === 0).

4. detectSpecPath has false positives (scratchpad.ts:711).
/\\.(md|txt|rst|adoc|org)$/i tests the trailing extension of the whole trimmed string, not that the whole string is a single token. A terse one-liner like \"see docs/spec.md\" or \"update README.md\" from the planner will be treated as a spec-path short-circuit and stored as Prior Spec Path, silently replacing a real prior path. Require the whole line be path-shaped (add \\S+$, reject anything with whitespace), or keep a stricter whitelist (^[\\w./~-]+\\.(md|txt|rst|adoc|org)$).

5. SCRATCHPAD 'single-writer discipline' is only a comment. scratchpad.ts:645 promises no concurrent writes, but every record* function does read → modify → write with no lock. If a future caller forgets and does await Promise.all([recordPlannerOutput(...), recordFilesModified(...)]), the later write wins and the earlier record is lost — silently. Either serialise internally (module-level p = p.then(...) queue keyed by filePath), or throw if overlapping writes are detected. A doc-only contract around a race-prone primitive is not safe for a workflow that will grow more writers.

Medium

6. Copilot deep-research calls getMessages() twice per stage (copilot/index.ts, all 6 fan-out stages). Inside the query closure you already fetch messages; then after queryWithProseGuard you call s.save(await s.session.getMessages()) again. That's a redundant round-trip on the Copilot SDK (three calls on the retry path). Capture the result of the query closure and reuse it for s.save, matching the pattern used in the OpenCode variant (which already buffers lastResult).

7. queryWithProseGuard's attempts field is dead. The return type is { text: string; attempts: 1 | 2 }, but every caller destructures only { text }. If retry is being hit in production you'll have no visibility. Either log it from inside the helper (console.warn(\"prose-guard retry triggered\")) or expose it to the session via s.save. Right now a silent regression in specialist prose quality is invisible.

8. CAVEMAN_PREAMBLE is prepended to every deep-research stage with no evidence and no kill switch. 106 lines of prose-style instructions on every locator/analyzer/scout/aggregator call — including history-analyzer whose output you then try to parse for ### Synthesis in deriveHistoryBrief. The preamble's "fragments OK, drop articles" guidance can conflict with downstream structured-section parsing. At minimum: (a) cite an A/B run showing token savings exceed any measured drop in truncateMarkdownReport-capped output quality, (b) gate it behind an input flag so it can be disabled without a code change, (c) exempt the aggregator/history stages whose outputs are consumed by regex extractors.

9. bunx skills add filter excludes Claude Code skills. publish.yml:86 passes -a opencode -a github-copilot but not -a claude-code. Per CLAUDE.md, .claude/skills is a symlink to .agents/skills, so Claude Code consumers will receive whatever ends up in .agents/skills. Is the intent to publish only OpenCode/Copilot skills and let Claude Code users miss out, or is -a claude-code being omitted by accident? If intentional, a one-line comment in the workflow would save future readers a trip through the skills add source.

10. compactDiffStat silently drops binary files. The regex /\\|\\s*(\\d+)/ doesn't match file.bin | Bin 0 -> 1234 bytes, so binary files never make it into fileLines and are omitted from the compacted output (and from the summary count discrepancy). Not hair-on-fire but worth a line-count marker or a separate preserved section.

Minor / nitpicks

• CHARS_PER_TOKEN = 4 (budget.ts:11) is defined and approxTokens is exported, but nothing in the diff consumes it. Dead on arrival or follow-up dependency? If the latter, a // used by src/.../ralph.ts comment prevents it from looking orphaned.
• deriveHistoryBrief word-cap math (masking.ts:574) splits on /\\s+/ which treats code blocks, list markers, and inline backticks as word-breaks. For technical prose that's fine, but the 150-word cap can land mid-code-fence and produce unclosed ``` in the rendered hint. Either balance fences post-trim or do a simple .indexOf(\"```\") → nearest-pair check.
• maskChangeset tests only exercise the under-threshold short-circuit. The 'over-threshold + top-N kept' path isn't covered via maskChangeset itself (only via the compactDiffStat unit). Add a single assertion that maskChangeset produces a smaller payload on a big changeset so the orchestration stays covered.
• queryWithProseGuard has no test file — it's a narrow function (5 lines) but it's the only retry path protecting eight prompt sites. Worth a 10-line bun test with stubbed query/retry to lock in the "only retry when empty" semantics.
• Inline scratchpad headings in initScratchpad (scratchpad.ts:669) use human-friendly names like "Prior RFCs" — if anyone ever renames one, the regex-based extractSection calls go silently empty (all lookups miss; append creates a second section below, etc.). Worth a const of section names shared between the seed template and the accessors.
• .agents/skills is being cp -R'd into the publish working tree but not re-verified before npm publish. A test -d .agents/skills && find .agents/skills -name '*.md' | head -1 sanity-check step would save a broken release if bunx skills add ever returns success with no output.

Security

Nothing sensitive. SKILLS_REPO is pinned to the repo's own upstream (flora131/atomic, which matches package.json#repository.url). No user input reaches the shell in the new CI step. The publish flow remains token-less (provenance only), consistent with the note in CLAUDE.md.

Test coverage summary

Scratchpad I/O functions (initScratchpad, readScratchpad, recordPlannerOutput, recordDebuggerReport, recordFilesModified, latestPriorRFC, priorSpecPath) and queryWithProseGuard have no tests. Given issues #2 and #5 above, the scratchpad happy-path + concurrent-write case deserves coverage before the Ralph wiring lands.

---

Happy to iterate on any of the above. The overall direction (shared context utilities, masking invariants, published-package hygiene) is solid — most of the above are the last 10%.

[claude]

Code Review

Solid refactor — pulling masking + scratchpad utilities into a shared _context/ module is a clean improvement, and the _context.test.ts coverage of the pure helpers is good. Most of the issues below are localized; one in publish.yml and one in the scratchpad section parser look like real correctness risks.

Critical

1. publish.yml SKILLS_REPO points at a personal fork. SKILLS_REPO: https://github.com/flora131/atomic.git (.github/workflows/publish.yml:78) hardcodes a fork URL into the npm publish workflow. When this runs against the canonical repo, it will install skills from flora131/atomic rather than the published source. Use ${{ github.server_url }}/${{ github.repository }}.git, or hardcode the canonical org/repo — but not a personal fork.

Significant

2. Scratchpad section parser doesn't track fenced code blocks. extractSection / replaceSection (_context/scratchpad.ts:183-203) and compactScratchFile (_context/masking.ts:301-348) treat any line matching /^##\s/ or /^###\s/ as a section heading. But recordPlannerOutput wraps planner RFCs inside a ```markdown fence (scratchpad.ts:106-111), and RFCs almost always contain ## section headings. With a real RFC, extractSection("Prior RFCs") will short-circuit at the first inner ## inside the fence, returning truncated content. Worse, replaceSection uses the same regex to splice, so it will mangle the file on the second iteration. Track ``` fence parity when scanning lines (or use a real markdown parser). A regression test using a planner output that contains ## Goals / ## Approach inside a fenced block would have caught this.

3. Broken indentation in deep-research-codebase/claude/index.ts (lines 195-244). The async (s) => { callback body is at 10-space indent while the closing }, jumps back to 8 spaces, and the inner stage is itself at 8 vs the surrounding 10. Compare to copilot/index.ts (clean) for the intended shape. bun lint will likely flag this; even if it parses, it's hard to read.

4. recordFilesModified discards per-iteration attribution. scratchpad.ts:152-155 rebuilds the body from the cumulative set plus a single trailing _(iteration N added M)_ tag. After iteration 2, the iteration-1 tag is gone — the scratchpad only records that file X was touched, not when. If "iteration N added M" is intended as cumulative provenance (the doc string suggests it is), append the tag instead of overwriting.

Minor

5. Duplicated helpers across SDK adapters. deriveSessionIntent and parseFilesFromNameStatus are identical copies in ralph/{claude,copilot,opencode}/index.ts. CAVEMAN_PREAMBLE is defined once in ralph/helpers/prompts.ts and again (slightly different) in deep-research-codebase/helpers/prompts.ts. CLAUDE.md says "Modularize code and avoid re-inventing the wheel" — these belong in _context/.

6. Wasted getMessages() round-trip in copilot deep-research. After queryWithProseGuard returns, code does s.save(await s.session.getMessages()) (e.g. deep-research-codebase/copilot/index.ts:178) — a second fetch over messages already produced inside the guard. Have queryWithProseGuard (or its getText callback) hand back the raw messages so the save uses the same array.

7. lastResult! non-null pattern in opencode. Works because query() is always invoked first, but the closure-side-effect plus non-null assertion pattern is ugly and easy to break. Consider widening queryWithProseGuard to return { text, raw } so call sites get the raw response without the side channel.

8. compactDiffStat re-orders by churn. masking.ts:96-105 sorts kept lines by churn descending. Original git diff --stat is path-ordered. Functional, but downstream consumers (and humans reading the masked output) may expect original ordering. Consider sorting back to original order after picking the top-N.

9. compactScratchFile heading-cost edge case. When headingCost > maxChars (lots of ### subheadings + tight cap), budgetForBodies clamps to 0 and every body becomes [… N chars elided …]. Worth either failing closed (return original / log warning) or reserving a minimum body budget per section.

10. Dead exports. approxTokens, CHARS_PER_TOKEN, COMPACT_TRIGGER_FRACTION (budget.ts), and readScratchpad, priorSpecPath (scratchpad.ts) are exported but unused anywhere. CLAUDE.md: "Don't add features … beyond what the task requires." Either wire them in or drop them.

11. runId = crypto.randomUUID() per .run(). A new UUID per workflow start means scratchpads can never resume across runs — the doc string in scratchpad.ts:5-8 advertises iteration-spanning persistence, which is true within a run but lost between them. If cross-run resumption is desired, derive id from hash(prompt + cwd). If strictly per-run, document so future readers don't assume otherwise.

12. process.cwd() in initScratchpad. Hardcodes the project root from the shell's cwd. If the workflow context exposes a project root, prefer that — it's resilient to invocations from subdirectories.

13. No cleanup of .atomic/ralph/<sessionId>/. Each run creates a new dir; nothing prunes old ones. Add to .gitignore (verify it's there) and consider a --keep-last N policy.

Test coverage

The pure helpers are well-covered. Notably missing:

• queryWithProseGuard (no direct tests — currently only validated by integration).
• All filesystem-touching scratchpad APIs (initScratchpad, recordPlannerOutput, recordDebuggerReport, recordFilesModified, latestPriorRFC) — easy to test with Bun.tempDir / os.tmpdir().
• compactScratchFilesForAggregator.
• A regression test for updates to readme and instructions #2 (heading-detection inside fenced blocks).

Style / spec compliance

14. CAVEMAN_PREAMBLE is risky and large. Telling models to "respond terse like smart caveman" is creative but can backfire on nuanced RFC-quality output. It's also ~1KB injected per stage — fan-out workflows pay it 10-20x. Worth measuring net token effect (saved output minus added preamble) and quality regression on a real spec before shipping. The "Boundaries / structured outputs" carve-out helps but relies on the model honoring it.

15. package.json .files mutation at publish time. Functionally fine but worth a comment in CHANGELOG/PR — the published package.json will diverge from the committed one.