engineering-conventions

name	engineering-conventions
audience	swarm-plugin
description	Guidelines and non-negotiable engineering invariants for modifying opencode-swarm. Load before architecture, plugin initialization, subprocess, tool registration, plan durability, .swarm storage, runtime portability, session/global state, guardrails/retry, chat/system message hooks, or release/cache changes. Authoritative source: AGENTS.md at the repo root and docs/engineering-invariants.md.
effort	medium

Engineering Conventions for opencode-swarm (Claude Code)

Authoritative source: AGENTS.md at the repo root and docs/engineering-invariants.md. This skill is a pointer + summary so Claude Code loads the right invariants before touching dangerous areas. Read AGENTS.md first. When this skill conflicts with AGENTS.md, AGENTS.md wins.

When to load this skill

Before changing shared/exported or runtime-contract code, use repo_map impact_cone to identify likely consumers, then verify them in direct source. Graph evidence is advisory only. If freshness is stale or inconclusive, confidence is low, source is missing, the language is unsupported/dynamic, the graph is absent, or the action fails, rely on direct source, searches, and executable evidence.

Load this skill before beginning implementation work that touches any of:

• src/index.ts (plugin entry / initializeOpenCodeSwarm)
• src/hooks/* (any hook that may run during init or QA review)
• src/tools/* (tool registration, working-directory anchoring, test_runner)
• src/utils/bun-compat.ts (subprocess shim — every spawn in the repo eventually flows through here)
• src/utils/timeout.ts (the withTimeout primitive used by every bounded init step)
• src/utils/gitignore-warning.ts (Git hygiene; runs on plugin init path)
• package.json, build configuration, dist/, plugin export shape
• Plan ledger / projection / checkpoint code (src/plan/*, .swarm/plan-*)
• Session / guardrails / runtime state (src/state.ts, src/hooks/guardrails.ts)
• Tests involving subprocesses, plugin startup, mock.module, or temp directories

If you are not sure whether you are touching one of these, you are touching one of these.

Highest-risk invariants (the ones that have already shipped regressions)

The full list of 12 invariants is in AGENTS.md. The four that have caused the most recent production regressions:

1. Plugin initialization is bounded and fail-open. Every awaited operation on the plugin-init path must be wrapped in withTimeout(...) and degrade non-fatally on timeout. Issue #704 (v7.0.3), the v7.3.3 git-hygiene regression, and PR #1920's merge-queue smoke failure all came from violating this. The OpenCode plugin host silently drops a plugin whose entry never resolves; users see "no agents in TUI / GUI" with no error. Bounded ≠ free: withTimeout only stops an unbounded hang — awaited work's real latency still counts toward the ~400 ms repro-704 init deadline. If init work does non-trivial I/O and nothing downstream needs it before server() resolves, register it in the wrapper-owned post-resolution task queue (the repoGraphHook precedent; PR #1356's bundled-skill sync is the exemplar), don't await it or use queueMicrotask inside the initializer; await only fast (<~50 ms) work a later init step depends on. Linux/macOS repro-704 green does not prove Windows — the smoke matrix enforces the 400 ms T1 deadline on the Windows runner, where cold-FS latency is several× higher.
2. Subprocesses are bounded, non-interactive, and killable. Every bunSpawn(['<bin>', ...]) call must pass cwd, stdin: 'ignore' (unless intentionally interactive), timeout: <ms>, bounded stdio, and call proc.kill() in a finally. An outer withTimeout is not enough — it lets the awaiter proceed but does not abort the child.
3. Runtime portability — Node-ESM-loadable + v1 plugin shape. No top-level bun: imports in dist/index.js. Default export is { id, server }. All Bun.* calls go through src/utils/bun-compat.ts. v6.86.8 / v6.86.9 are the cautionary tales.
4. Test mock isolation. mock.module(...) leaks across files in Bun's shared test-runner process. Use a file-scoped _internals dependency-injection seam (see src/utils/gitignore-warning.ts:_internals and src/hooks/diff-scope.ts:_internals) instead. Restore in afterEach. The writing-tests skill covers this in detail; load it before modifying tests.

Cross-link: writing tests

For test changes, also load .claude/skills/writing-tests/SKILL.md. It covers bun:test API, mock isolation rules, CI per-file isolation, and cross-platform anti-patterns.

Hard warning: do NOT use broad test_runner for repo validation

The OpenCode test_runner tool is for targeted agent validation with explicit files: [...] or small targeted scopes. It is not the way to validate the full repo from inside a Claude Code session that orchestrates OpenCode. In this repo:

• MAX_SAFE_TEST_FILES = 50 (src/tools/test-runner.ts). Resolutions exceeding this return outcome: 'scope_exceeded' with a SKIP. Do not lean on this — broad scopes can stall or kill OpenCode before that guard fires.
• For repo validation, run the shell commands in contributing.md / TESTING.md directly (per-file isolation loops + tier orchestration).
• scope: 'all' is gated behind the SWARM_ALLOW_FULL_SUITE=1 env var (intended for opt-in CI mirrors only); there is no allow_full_suite arg. Default to files: [...] instead.

Agent prompt strings — escaping pitfalls

Agent prompts in src/agents/*.ts are large TypeScript template literals. They frequently contain characters that have special meaning inside template literals and cause silent parse errors if unescaped:

Character	Inside template literal	Correct escape
Backtick `	Terminates the literal	\` (single backslash — renders as ` in output)
${	Starts an interpolation	\${ (single backslash)
Literal backslash \	Consumed by escape processing	\\ (double backslash renders as \ in output)

The most common failure pattern: A coder adds an inline code example containing backticks to an agent prompt string. The unescaped backtick silently terminates the template literal, producing a SyntaxError: Unexpected identifier or Unexpected token at the character after the backtick — which appears unrelated to the actual cause.

// WRONG — unescaped backtick terminates the template literal
const PROMPT = `
Use `bun:test` for all tests.   // ← bare backtick before "bun" closes the literal
`;

// CORRECT — single backslash before each backtick; renders as Use `bun:test` in output
const PROMPT = `
Use \`bun:test\` for all tests.
`;

// OVER-ESCAPED (also wrong) — triple backslash produces literal \` in the rendered prompt
const PROMPT = `
Use \\\`bun:test\\\` for all tests.  // renders as: Use \`bun:test\` (backslashes visible)
`;

Detection: If bun run build or bun --smol test reports a parse error at a line number that seems far from any recent change, search the surrounding lines for an unescaped backtick inside a template literal.

Prevention: After adding any inline code example to an agent prompt, run bun run build immediately — the TypeScript compiler catches unescaped backticks as a syntax error before any tests run.

The invariant-audit gate (PR-time)

Every PR that touches a relevant area must include an ## Invariant audit section in its description. The format is in AGENTS.md ("Invariant audit required in PRs"). The commit-pr skill enforces this gate before push/PR — load it before committing.

If you cannot prove a touched invariant from source and test output, do not push.

Evidence file flow (.swarm/evidence/{taskId}.json)

Agents NEVER write these files directly. The delegation-gate hook writes them automatically after each reviewer/test_engineer Task delegation returns. The schema is defined in src/gate-evidence.ts:

export interface GateEvidence {
  sessionId: string;  // actual session ID from the Task delegation
  timestamp: string;  // ISO 8601
  agent: string;     // 'reviewer' | 'test_engineer' | 'sme' | etc.
}

export interface TaskEvidence {
  taskId: string;
  required_gates: string[];
  gates: Record<string, GateEvidence>;
  turbo?: boolean;
}

How to verify the flow is working:

1. After dispatching a reviewer/test_engineer Task, the delegation-gate toolAfter hook should automatically write/update .swarm/evidence/{taskId}.json.
2. When you call update_task_status(completed), the tool reads the evidence file and verifies the required_gates are all present.
3. If update_task_status fails with "required QA gates not yet satisfied" or "Evidence file is corrupt or unreadable," inspect the evidence file with cat .swarm/evidence/{taskId}.json to diagnose.

Do NOT manually write or fabricate evidence files. This bypasses the gate enforcement and can cause downstream tool failures when the real session IDs are looked up.

When to suspect the flow is broken:

• The evidence file doesn't exist after a reviewer/test_engineer Task delegation returns
• The evidence file exists but has wrong agent or sessionId values
• The plan has newly-added task IDs that the hook may not recognize

Workaround for broken flow: If the hook consistently fails to write the evidence file, escalate to the user — do NOT silently fabricate evidence with placeholder session IDs. The gate check exists to enforce that a real review/test run happened.

See .claude/skills/writing-tests/SKILL.md § Cross-Platform Requirements → "macOS rename-visibility race" for the ENONENT retry pattern that this gate flow triggers on macOS CI.

Init-path-safe imports (invariant 1 deep-dive)

The most expensive invariant-1 violations come from transitive import chains that silently load heavy modules (WASM, tree-sitter) at plugin init time. A single import { X } from '../../lang' in a tool-time module can transitively load runtime.ts → web-tree-sitter (heavy WASM), spiking init latency well past the repro-704 T1 deadline (observed during issue #1471 development).

The lang barrel trap

src/lang/index.ts re-exports from ./runtime, which statically imports web-tree-sitter. Importing anything from the barrel (from '../../lang') transitively loads WASM at module-eval time.

Wrong: import { LANGUAGE_REGISTRY } from '../../lang' — loads runtime → web-tree-sitter. Right: import { LANGUAGE_REGISTRY } from '../../lang/profiles' — loads only profiles (string data, no WASM).

Type-only vs value imports

• import type { Query } from 'web-tree-sitter' — safe (erased at compile time, no module load).
• import { Query } from 'web-tree-sitter' — unsafe on the init path (loads the WASM module).
• For value dependencies on heavy modules in init-reachable code, use dynamic import() inside an async function (deferred to first call, not module load).

The --external build flag

Dynamic import('web-tree-sitter') only defers loading at runtime if --external web-tree-sitter is set in the bun build config. Without it, bun bundles web-tree-sitter inline and the dynamic import resolves from the bundle (no deferral). Check package.json build scripts for the flag.

Verification checklist

For any import-chain change touching src/lang/, runtime, or web-tree-sitter:

1. Trace the transitive chain from src/index.ts to verify no heavy module loads at init.
2. Rebuild dist: bun run build (stale dist gives false regressions).
3. Run node scripts/repro-704.mjs — T1 must be under 400ms.
4. Run bun --smol test tests/unit/lang/symbol-graph-init-purity.test.ts — init-path purity tests must pass.

Sandbox env overrides (subprocess-safety deep-dive)

When a sandbox executor (src/sandbox/{linux,macos,win32}/*.ts) interpolates environment variables into a sandbox profile, a bwrap rule, or a PowerShell -EnvironmentVariables block, the following rules apply. They exist because a future shell-injection regression in any new sandbox path is a security vulnerability, not just a bug:

• Keys must match POSIX env-var name syntax. Every env key must be validated against /^[A-Za-z_][A-Za-z0-9_]*$/ (a leading letter or underscore, then letters/digits/underscores) before being interpolated. Define or reuse a single isValidEnvKey(key: string): boolean helper colocated with the SandboxExecutor interface in src/sandbox/executor.ts (around line 24+); do not duplicate the regex inline at every call site. Keys that fail validation must be silently dropped (not raised) so that one bad caller cannot wedge the sandbox path — but the drop must be observable in pendingAdvisoryMessages or a structured log, never silent.
• Values must be shell-quoted or treated as opaque single tokens. On POSIX, prepend a leading single quote, escape embedded ' by replacing with '\'', then append a trailing single quote. On Windows PowerShell, prefer single-quoted literal contexts (e.g. '$env:NAME') and run values through a psStringEscape-style helper that escapes backtick, $, ", and ` (the special characters in double-quoted PowerShell strings). Single-quoted PowerShell strings are literal — only ' needs escaping, doubling it to ''. If a context requires double-quoted PS values, escape embedded " as `, backtick as , and `$` as `` (backtick is the PS escape character in double-quoted strings;$must be escaped to prevent variable expansion). On bwrap, always pass values as separate argv tokens after the--setenv flag (--setenv KEY VALUE, two tokens), never as a single concatenated KEY=VALUE` token that an intermediate shell would interpret.
• Use array-form argv for every sandbox subprocess. Never shell:-interpolate. The same invariant-3 rules (array-form spawn, stdin: 'ignore', cwd, timeout, proc.kill() in finally) apply to sandbox spawns as to any other subprocess — see subprocess-safety cross-link.

Sandbox fallback parity (Windows and Linux)

sandbox/{linux,macos,win32}/*.ts has primary executors plus legacy fallbacks: Windows NativeWindowsSandboxExecutor with RestrictedEnvironmentExecutor / PowerShell wrapper, Linux BubblewrapSandboxExecutor with no-sandbox fallback. When you modify any of the following on the primary executor, update the fallback path in the same change to keep behavior parity and add a parity test:

• getEnvOverrides signature or merge semantics.
• wrapCommand scoping rules (allowed roots, read-only mounts, temp-dir allocation).
• isAvailable() / capability probe logic.
• Failure-mode handling (does a missing sandbox envelope hard-fail or soft-fail to env-only isolation?).
• Scope-materialization for lane-scoped resources.

A divergence between primary and fallback that is not exercised by a parity test is a regression. The existing per-OS test files tests/unit/sandbox/{linux,macos,win32}.test.ts must continue to cover both the primary and fallback paths after every env-affecting change — extend these tests rather than relying on dedicated sandbox-envoverride test files that may or may not exist in your branch.

SAST baseline capturing (differential scanning)

The sast_scan tool supports capture_baseline: true with a phase parameter to snapshot pre-existing findings. Subsequent scans with the same phase value perform differential checking — they only fail on new findings, not pre-existing ones.

When to capture a baseline

• Before Phase 1 code changes. The baseline must reflect the state of the codebase before any new work is done. This ensures the differential scan catches findings introduced by the current session's changes.

Critical safety guard

NEVER capture a baseline after code changes have been made in a phase. A baseline captured post-edit silently encodes the very bugs the scan is meant to catch as "pre-existing," suppressing them indefinitely. This turns the SAST gate into theater.

Baseline capture also requires at least one supported, existing file to be successfully scanned. Omitted, empty, or entirely unscannable changed_files returns capture_baseline requires changed_files to produce a non-empty baseline instead of reporting a successful no-op capture.

Moved findings and audited absorption (#2302)

Every baseline fingerprint carries a position-independent reflow identity (file + rule + flagged-line content). A pre-existing finding whose neighbor lines were edited, or that moved to a new line, is reported as a moved_findings entry on the next diff scan — it never gates. Only findings matching neither the baseline fingerprints nor its reflow identities are NEW.

Re-capturing into an existing baseline with findings that were not in it is BLOCKED unless the capture passes baseline_refresh_rationale — for already-indexed AND first-time-indexed files alike, because the tool cannot distinguish a pre-delegation capture from a failure-response recapture; every absorbed finding then records a who/when/rationale entry in the baseline triage log. This is the audited path for genuinely pre-existing findings (routine per-task captures of first-time files, or findings discovered late after a file rename) — it is NOT a way to make a failed gate go green: recapturing with a rationale after a gate failure means accepting findings that may be coder-introduced. First writes (baseline creation) are snapshots, not absorptions, and stay free.

How to use it

1. Identify the files to scan. In a phase, use the union of declared task-scope files plus files the coder is expected to touch. Derive the list from declare_scope outputs, git diff --name-only, or the phase's task specs.
2. Before any coder delegation in Phase 1, capture the baseline:

   sast_scan(directory, changed_files=[...], capture_baseline=true, phase=1)
   

3. After coder work, scan the same file set:

   sast_scan(directory, changed_files=[...], phase=1)
   

   This returns only NEW findings (absent from the baseline).
4. If a pre-existing finding is legitimately fixed, the baseline can be re-captured at the start of the next phase with the updated file list.

Why this matters

During PR #1704 review, SAST flagged RegExp.prototype.exec() as "command injection via child_process.exec()" — a false positive that blocked the gate. With a baseline captured before the phase, this pre-existing false positive would have been suppressed, and only genuinely new findings would surface.