feat: add Linear guardrails to CLAUDE.md (SEC-20)#3
Merged
Conversation
…rails - Install secret scanning pre-commit hook via git core.hooksPath in entrypoint.sh - Hook blocks commits matching sk-ant-, ghp_, ghs_, lin_api_, PEM blocks, base64 JWT patterns - Add Security Boundaries section to groups/CLAUDE.md with email policy (krewtrack.com free, external requires approval) and data protection rules
- Add DEFAULT_MAX_BUDGET_USD=5 to config.ts (overridable via env var) - Wire maxBudgetUsd: DEFAULT_MAX_BUDGET_USD into Slack-triggered runs in index.ts - Use DEFAULT_MAX_BUDGET_USD as fallback in task-scheduler.ts for scheduled tasks - Add --cpus=2 to docker run args in container-runner.ts
Destructive Linear actions (delete tickets/projects, archive, remove members) require human approval. Bulk modifications capped at 5 tickets without asking. Read + status updates are free. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
bryantb2
added a commit
that referenced
this pull request
Apr 7, 2026
- Remove direct dev-team→QA IPC path; let dispatch handle all QA routing via build loop to maintain hub-and-spoke model (issue #1) - Add explicit CI re-poll instruction after pushing fixes (issue #2) - Remove --depth=1 from repo clones to avoid branch op failures (issue #3) - Parameterize repo name in QA screenshot protocol (issue #4) - Use subshell for cd in setup script to avoid dir state issues (#7) - Add default branch note to session startup (#5) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
bryantb2
added a commit
that referenced
this pull request
Apr 7, 2026
…n, QA handoff, persistent repos (#15) * fix: orchestration loop failures — completion records, CI verification, QA handoff Root cause: dispatch→dev-team→qa-sentinel pipeline broken because (1) completion record rules excluded dispatch-routed tasks, (2) dev-team had no post-PR protocol for CI/QA/records, (3) QA posted file paths instead of embedded screenshots. Changes: - Global CLAUDE.md: broaden completion record scope to include dispatch-routed tasks (identified by [DISPATCH-ROUTED] tag), add mandatory Engineer delegation prompt injections for commit discipline, CI verification, PR targeting, and completion records - Dev-team CLAUDE.md: add Session Startup for persistent repos, Post-PR Protocol (CI polling, Linear comment, completion record, QA handoff via IPC), PR targeting rule - QA-sentinel CLAUDE.md: add GitHub PR Screenshot Protocol (upload as release assets, embed URLs — never file paths), session startup for persistent repos - Dispatch CLAUDE.md: add [DISPATCH-ROUTED] tag requirement for all IPC messages, scheduled task manifest requirement, update all IPC message templates with tag and explicit post-PR instructions - Mount allowlist example: add ~/repos and ~/nanoclaw/data roots - New script: setup-persistent-repos.sh for per-group repo clones Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address code review feedback on orchestration PR - Remove direct dev-team→QA IPC path; let dispatch handle all QA routing via build loop to maintain hub-and-spoke model (issue #1) - Add explicit CI re-poll instruction after pushing fixes (issue #2) - Remove --depth=1 from repo clones to avoid branch op failures (issue #3) - Parameterize repo name in QA screenshot protocol (issue #4) - Use subshell for cd in setup script to avoid dir state issues (#7) - Add default branch note to session startup (#5) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
bryantb2
added a commit
that referenced
this pull request
Apr 12, 2026
Code review of PR #40 (superpowers:code-reviewer subagent) flagged 4 Important issues. All addressed in this commit. ## Issue #1 — `rmSync` lacks `force: true` (TOCTOU vulnerability) Previous helper used `if (existsSync(dest)) rmSync(dest)` — vulnerable to a TOCTOU race if an operator/backup script deleted the dir between the existsSync check and the rmSync call. The new implementation uses `force: true` on every rmSync (both scratch cleanup and final swap), making the helper resilient to concurrent external deletion. ## Issue #2 — Atomic rename pattern (highest-value hardening) Previous helper did `rmSync(dest); cpSync(src, dest)` — if cpSync failed mid-copy (disk full, EIO, permission revocation), the cache was left partial and the next container's `tsc` step would crash on the broken source tree. The reviewer correctly called this out as the single highest-value hardening: low probability but high blast radius. New implementation uses canonical atomic-rename: 1. `rmSync(scratchDir, { force: true })` — clean any leftover from prior failed run (no-op if absent thanks to force flag) 2. `cpSync(src, scratchDir)` — copy to scratch first; if this throws, the existing cache at dest is untouched 3. `rmSync(dest, { force: true })` — drop the old cache 4. `renameSync(scratchDir, dest)` — swap into place If step 2 fails: existing cache intact, helper throws, next spawn retries cleanly (and removes the partial scratch first). If step 3 or 4 fails (extremely unlikely): wrapped in try/catch with best-effort scratch cleanup so retries don't accumulate scratch dirs. The window between step 3 and step 4 is sub-millisecond and protected by GroupQueue.active serialization (only one spawn per group at a time, verified by walking the call chain enqueueMessageCheck → runForGroup → runContainerAgent → buildVolumeMounts). ## Issue #3 — Production logging on the refresh Previous helper was silent in production. The reviewer correctly noted: "the original bug went undetected for 2 days partly because there was zero observability into the cache lifecycle." Added a single `logger.debug` line on success and a `logger.error` line in the catch block. Cheap, valuable forever. ## Issue #4 — Integration test pinning the wiring The 11 unit tests in refresh-agent-runner-src-cache.test.ts cover the helper's real wipe-and-recopy semantics, but none of them verifies that buildVolumeMounts ACTUALLY CALLS the helper. A future refactor that drops the helper invocation would not break a single existing test and the populate-once regression would silently return. Added a new "container-runner agent-runner cache refresh wiring" describe block in container-runner.test.ts with 2 integration tests that exercise the full runContainerAgent → buildVolumeMounts → refreshAgentRunnerSrcCache code path and assert: 1. cpSync was called with the scratch dir target (verifies the atomic-rename pattern is wired through, not just direct cpSync into dest — if anyone reverts to in-place copy, this fails) 2. renameSync was called with `<dest>.new → dest` pair (verifies the swap step is wired) Required adding `cpSync`, `rmSync`, `renameSync` to the global fs mock in container-runner.test.ts (they were not previously used). ## Issue #6 (minor) — Stale "agents can customize it" comment The buildVolumeMounts call site comment said "Copy agent-runner source into a per-group writable location so agents can customize it." That framing was already misleading before this PR (no group actually customizes) and is now actively wrong (refresh wipes customizations on every spawn). Updated the comment to point at the helper's docstring and to explicitly warn against writing runtime state into the cache dir. ## Tests added (6 net new) In refresh-agent-runner-src-cache.test.ts (4 new): - cleans up a leftover .new scratch dir from a prior failed refresh - preserves the existing cache when cpSync fails mid-copy (disk full simulation via vi.spyOn, the scenario from Issue #2) - cleans up scratch dir on failure so retries do not accumulate state - does not throw when the dest directory does not exist (force flag, the scenario from Issue #1) In container-runner.test.ts (2 new): - invokes cpSync on the agent-runner src path during runContainerAgent - invokes renameSync to swap the scratch dir into place ## Verification - 587 / 587 tests passing locally (was 581, +6 new) - npx tsc --noEmit: clean - All 4 Important issues from code review addressed - Issue #5 (filename naming nit) skipped — minor consistency note, not worth churn - Issue #7 (symlink test) skipped — no symlinks in source today, acceptable to defer until needed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
bryantb2
added a commit
that referenced
this pull request
Apr 12, 2026
…ery spawn (#40) * fix(container-runner): refresh per-group agent-runner src cache on every spawn The per-group `data/sessions/<group>/agent-runner-src/` cache was implemented in commit 5fb1064 (security fix qwibitai#392) as part of mounting the project root read-only to prevent sandbox escape. Each group needs its own writable copy of the agent-runner source so agents can customize tools/behavior without affecting other groups. But the cache was created with `if (!fs.existsSync(...)) cpSync(...)` — populate-once-and-never-refresh. Once a group spawned its first container, the cache was frozen forever. Every subsequent agent-runner change was silently swallowed for that group, even after `restart-fleet.sh` and submodule bumps. ## How the bug went undetected NanoClaw's deploy flow gives a false sense of correctness: 1. `git pull` brings in new container/agent-runner/src/ — this works 2. `npm run build` produces new container/agent-runner/dist/ — this works 3. Container image build copies and recompiles — this works (image has the new code in /app/dist/index.js) 4. Container spawn bind-mounts /app/src ← per-group cache — STALE 5. Container's entrypoint.sh recompiles /app/src to /tmp/dist and runs from /tmp/dist — runs the STALE code The image build at step 3 did the right thing, but its result is shadowed by the stale cache mount at step 4. Investigators see fresh code on the host, fresh code in /app/dist inside the container, and no obvious failure — but the entrypoint compiles from /app/src (stale) not /app/dist (fresh). ## Real-world impact 3 PRs merged into main between Apr 9 and Apr 11 never reached the 4 cached groups (slack_dispatch, slack_dev-team, slack_qa-sentinel, slack_fleet-ops): - aec983e (PR #29) feat(cost): accurate cost tracking from token counts Apr 9 22:26 MDT — host-side schema migration ran, but the agent-runner side never deployed. cost_log shows `cost_source='sdk'` with `input_tokens=0` for every dispatch row since Apr 9 because the cached version still uses the SDK-fallback path. We spent today investigating this as an "SDK quirk" — it was actually this bug. - 6cfff71 (PR #38) chore: post-deploy cleanup nits Diagnostic logger for the (apparent) SDK token gap — never deployed. - be22ff2 (PR #39) fix(agent-runner): write DIAG zero-token dump File-based diagnostic capture — never deployed. The 2 groups WITHOUT a cache (slack_dev-ops, slack_product-brain) will get fresh code on first spawn and don't share the bug. Once they spawn their first container, they will inherit the same staleness unless this fix lands. ## Fix Replace the populate-once guard with unconditional refresh: wipe the cache directory if it exists, then re-copy from the upstream source. Per-group isolation is preserved because each group still gets its own independent writable copy at a distinct path. Extracted into `refreshAgentRunnerSrcCache(src, dest)` for testability, called from `buildVolumeMounts` on every container spawn. ~10-50ms cp overhead per spawn (recursive copy of ~33KB of TypeScript), well below container spawn baseline. ## Tests added (7) New file `src/refresh-agent-runner-src-cache.test.ts` (separate from container-runner.test.ts which mocks `fs` globally): 1. creates the cache directory when it does not exist 2. REFRESHES a stale cache (the regression fix from PR #29 cache bug) — pre-seeds an Apr-9-style stale cache + a file that no longer exists in upstream, asserts both are replaced and cleaned 3. preserves per-group isolation (security fix qwibitai#392) — two groups get independent cache dirs; mutating one does not affect the other 4. refresh wipes per-group customizations (acceptable trade-off) — pins the design choice so a future feature relying on cross-spawn customization persistence will fail this test and force a redesign 5. is a no-op when the upstream source dir does not exist 6. idempotent: calling twice with no source changes produces identical result 7. handles nested subdirectories in the upstream source 581 / 581 tests passing locally (was 574, +7 new). ## Deploy impact warning When this PR ships and `restart-fleet.sh` runs, the next container spawn for ANY group will retroactively activate every agent-runner change since Apr 9 for the 4 cached groups. That includes: - PR #29's per-token cost computation → cost_log will start showing non-zero token counts for dispatch / dev-team / qa-sentinel / fleet-ops on the next spawn - PR #38 + PR #39's diagnostic logging — both become live but neither is needed anymore since #29 is now actually doing what it should Most behavior changes from #29 are pure observability improvements (token counts in cost_log) so the deploy is low-risk. But Blake should be aware that cost reporting will look DIFFERENT after deploy: `cost_source` will switch from `'sdk'` to `'computed'` for cached groups, and `input_tokens`/`output_tokens` will populate. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * style(test): prettier --write on refresh-agent-runner-src-cache.test.ts (CI format:check) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fixup(audit): atomic rename + force flag + logging + integration test Code review of PR #40 (superpowers:code-reviewer subagent) flagged 4 Important issues. All addressed in this commit. ## Issue #1 — `rmSync` lacks `force: true` (TOCTOU vulnerability) Previous helper used `if (existsSync(dest)) rmSync(dest)` — vulnerable to a TOCTOU race if an operator/backup script deleted the dir between the existsSync check and the rmSync call. The new implementation uses `force: true` on every rmSync (both scratch cleanup and final swap), making the helper resilient to concurrent external deletion. ## Issue #2 — Atomic rename pattern (highest-value hardening) Previous helper did `rmSync(dest); cpSync(src, dest)` — if cpSync failed mid-copy (disk full, EIO, permission revocation), the cache was left partial and the next container's `tsc` step would crash on the broken source tree. The reviewer correctly called this out as the single highest-value hardening: low probability but high blast radius. New implementation uses canonical atomic-rename: 1. `rmSync(scratchDir, { force: true })` — clean any leftover from prior failed run (no-op if absent thanks to force flag) 2. `cpSync(src, scratchDir)` — copy to scratch first; if this throws, the existing cache at dest is untouched 3. `rmSync(dest, { force: true })` — drop the old cache 4. `renameSync(scratchDir, dest)` — swap into place If step 2 fails: existing cache intact, helper throws, next spawn retries cleanly (and removes the partial scratch first). If step 3 or 4 fails (extremely unlikely): wrapped in try/catch with best-effort scratch cleanup so retries don't accumulate scratch dirs. The window between step 3 and step 4 is sub-millisecond and protected by GroupQueue.active serialization (only one spawn per group at a time, verified by walking the call chain enqueueMessageCheck → runForGroup → runContainerAgent → buildVolumeMounts). ## Issue #3 — Production logging on the refresh Previous helper was silent in production. The reviewer correctly noted: "the original bug went undetected for 2 days partly because there was zero observability into the cache lifecycle." Added a single `logger.debug` line on success and a `logger.error` line in the catch block. Cheap, valuable forever. ## Issue #4 — Integration test pinning the wiring The 11 unit tests in refresh-agent-runner-src-cache.test.ts cover the helper's real wipe-and-recopy semantics, but none of them verifies that buildVolumeMounts ACTUALLY CALLS the helper. A future refactor that drops the helper invocation would not break a single existing test and the populate-once regression would silently return. Added a new "container-runner agent-runner cache refresh wiring" describe block in container-runner.test.ts with 2 integration tests that exercise the full runContainerAgent → buildVolumeMounts → refreshAgentRunnerSrcCache code path and assert: 1. cpSync was called with the scratch dir target (verifies the atomic-rename pattern is wired through, not just direct cpSync into dest — if anyone reverts to in-place copy, this fails) 2. renameSync was called with `<dest>.new → dest` pair (verifies the swap step is wired) Required adding `cpSync`, `rmSync`, `renameSync` to the global fs mock in container-runner.test.ts (they were not previously used). ## Issue #6 (minor) — Stale "agents can customize it" comment The buildVolumeMounts call site comment said "Copy agent-runner source into a per-group writable location so agents can customize it." That framing was already misleading before this PR (no group actually customizes) and is now actively wrong (refresh wipes customizations on every spawn). Updated the comment to point at the helper's docstring and to explicitly warn against writing runtime state into the cache dir. ## Tests added (6 net new) In refresh-agent-runner-src-cache.test.ts (4 new): - cleans up a leftover .new scratch dir from a prior failed refresh - preserves the existing cache when cpSync fails mid-copy (disk full simulation via vi.spyOn, the scenario from Issue #2) - cleans up scratch dir on failure so retries do not accumulate state - does not throw when the dest directory does not exist (force flag, the scenario from Issue #1) In container-runner.test.ts (2 new): - invokes cpSync on the agent-runner src path during runContainerAgent - invokes renameSync to swap the scratch dir into place ## Verification - 587 / 587 tests passing locally (was 581, +6 new) - npx tsc --noEmit: clean - All 4 Important issues from code review addressed - Issue #5 (filename naming nit) skipped — minor consistency note, not worth churn - Issue #7 (symlink test) skipped — no symlinks in source today, acceptable to defer until needed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * style: prettier --write on container-runner.ts + cache helper test (CI format:check) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes SEC-20 from security audit.