From aad13197abdf9710ac7e86f033a36db2907c0199 Mon Sep 17 00:00:00 2001 From: Multi-Repo Pushback Bot Date: Fri, 24 Apr 2026 08:24:24 +0200 Subject: [PATCH 1/3] feat(sdk): add applySiblingLinks for workflow dep linking Adds a generic, language-agnostic helper that workflow authors can chain into their setup to link sibling-repo packages into the workflow's working directory. Solves the "agents fabricate interfaces when the installed package is stale" class of bugs by making the real, head-of-main interface visible during the workflow run. Usage: import { workflow, applySiblingLinks } from '@agent-relay/sdk/workflows'; const wf = applySiblingLinks(workflow('my-feature').pattern('dag'), { dependsOn: ['install-deps'], links: [ { name: '@agent-assistant/proactive', path: '../agent-assistant/packages/proactive', expect: ['recordSignal', 'drainSignals'], }, ], }); Detection auto-dispatches per manifest: - package.json -> 'npm link' (symlinks, no committed manifest change) - pyproject.toml / setup.py / setup.cfg -> 'pip install -e' or 'uv pip install -e' Post-link, when \`expect\` is provided, smoke-tests the linked package via \`node --input-type=module -e\` (npm) or \`python3 -c\` (python). Any missing named export fails the step. MVP scope: - npm + python (pip / uv). Auto-detect. - Fail-fast on missing sibling path, unknown manifest, or missing expected export. - Other languages (Go, Cargo, Gradle, Bundler, .NET) land as follow-up dispatch branches behind the same public API. 12 tests covering script generation, manifest branches, quote escaping, expect-list JSON round-trip, and the builder integration. --- .../workflows/__tests__/sibling-links.test.ts | 130 +++++++++ packages/sdk/src/workflows/index.ts | 2 + packages/sdk/src/workflows/sibling-links.ts | 246 ++++++++++++++++++ 3 files changed, 378 insertions(+) create mode 100644 packages/sdk/src/workflows/__tests__/sibling-links.test.ts create mode 100644 packages/sdk/src/workflows/sibling-links.ts diff --git a/packages/sdk/src/workflows/__tests__/sibling-links.test.ts b/packages/sdk/src/workflows/__tests__/sibling-links.test.ts new file mode 100644 index 000000000..8d8702e6c --- /dev/null +++ b/packages/sdk/src/workflows/__tests__/sibling-links.test.ts @@ -0,0 +1,130 @@ +import { describe, expect, it, vi } from 'vitest'; + +import { applySiblingLinks, buildSiblingLinkScript } from '../sibling-links.js'; + +describe('buildSiblingLinkScript', () => { + it('detects npm manifest and emits an npm link block', () => { + const script = buildSiblingLinkScript([{ name: '@scope/pkg', path: '../sibling/packages/pkg' }]); + expect(script).toContain('-f "$SIBLING_PATH/package.json"'); + expect(script).toContain('npm link --silent'); + expect(script).toContain('@scope/pkg'); + expect(script).toContain('../sibling/packages/pkg'); + }); + + it('detects python manifest and emits a pip install -e block', () => { + const script = buildSiblingLinkScript([{ name: 'my_pkg', path: '../py/pkg' }]); + expect(script).toContain('-f "$SIBLING_PATH/pyproject.toml"'); + expect(script).toContain('pip install -e'); + expect(script).toContain('uv pip install -e'); + }); + + it('fails-fast shell: script uses set -euo pipefail', () => { + const script = buildSiblingLinkScript([{ name: 'x', path: './x' }]); + expect(script.startsWith('set -euo pipefail')).toBe(true); + }); + + it('guards missing sibling path with explicit error', () => { + const script = buildSiblingLinkScript([{ name: 'x', path: '../missing' }]); + expect(script).toContain('SIBLING_PATH_MISSING'); + expect(script).toContain('exit 1'); + }); + + it('guards unknown manifest with explicit error', () => { + const script = buildSiblingLinkScript([{ name: 'x', path: './x' }]); + expect(script).toContain('UNKNOWN_MANIFEST'); + }); + + it('emits one verify block per link with expected exports', () => { + const script = buildSiblingLinkScript([ + { name: 'pkg-a', path: '../a', expect: ['foo', 'bar'] }, + { name: 'pkg-b', path: '../b' }, + { name: 'pkg-c', path: '../c', expect: ['baz'] }, + ]); + const verifyCount = (script.match(/APPLY_SIBLING_LINKS_EXPECT/g) ?? []).length; + // Two verify blocks (for pkg-a + pkg-c), each referenced at least twice + // (env var declaration + two command variants for node/python fallback). + expect(verifyCount).toBeGreaterThanOrEqual(4); + expect(script).toContain('APPLY_SIBLING_LINKS_OK'); + }); + + it('JSON-encodes expected exports safely for shell and downstream JSON.parse', () => { + const script = buildSiblingLinkScript([{ name: 'p', path: './p', expect: ["it's-ok", 'with"quote'] }]); + // Expect list is embedded as: EXPECT= + // so the inner JSON survives round-trip through bash env var into + // Node.JSON.parse / Python json.loads. + const expectedInner = JSON.stringify(["it's-ok", 'with"quote']); + const expectedShellArg = JSON.stringify(expectedInner); + expect(script).toContain(`EXPECT=${expectedShellArg}`); + }); + + it('emits both node and python verifiers wrapped in manifest-conditional', () => { + const script = buildSiblingLinkScript([{ name: 'p', path: './p', expect: ['x'] }]); + expect(script).toContain('node --input-type=module'); + expect(script).toContain('python3 -c'); + // The wrapping if/elif/else pattern keeps python as a fallback inside + // the non-package.json branch. + expect(script).toMatch(/if \[ -f "\$SIBLING_PATH\/package\.json" \]; then[\s\S]+?else[\s\S]+?python/); + }); +}); + +describe('applySiblingLinks', () => { + it('is a no-op when links is empty', () => { + const builder = { step: vi.fn() }; + const result = applySiblingLinks(builder, { links: [] }); + expect(builder.step).not.toHaveBeenCalled(); + expect(result).toBe(builder); + }); + + it('adds a single deterministic step named setup-sibling-links by default', () => { + const builder = { step: vi.fn(() => builder) }; + applySiblingLinks(builder, { + links: [{ name: 'pkg', path: '../pkg' }], + }); + expect(builder.step).toHaveBeenCalledTimes(1); + const call = builder.step.mock.calls[0] as unknown as + | [string, { command: string; [k: string]: unknown }] + | undefined; + if (!call) throw new Error('expected step call'); + const [stepName, cfg] = call; + expect(stepName).toBe('setup-sibling-links'); + expect(cfg).toMatchObject({ + type: 'deterministic', + dependsOn: ['install-deps'], + captureOutput: true, + failOnError: true, + }); + expect(cfg.command).toContain("bash -c '"); + }); + + it('honors custom stepName and dependsOn', () => { + const builder = { step: vi.fn(() => builder) }; + applySiblingLinks(builder, { + links: [{ name: 'pkg', path: '../pkg' }], + stepName: 'custom-name', + dependsOn: ['setup-branch'], + }); + const call = builder.step.mock.calls[0] as unknown as + | [string, { command: string; [k: string]: unknown }] + | undefined; + if (!call) throw new Error('expected step call'); + const [stepName, cfg] = call; + expect(stepName).toBe('custom-name'); + expect(cfg).toMatchObject({ dependsOn: ['setup-branch'] }); + }); + + it('escapes single quotes in the embedded script safely for bash -c', () => { + const builder = { step: vi.fn(() => builder) }; + applySiblingLinks(builder, { + links: [{ name: "has'quote", path: "./path'with-quote" }], + }); + const call = builder.step.mock.calls[0] as unknown as [string, { command: string }] | undefined; + if (!call) throw new Error('expected step call'); + const command = call[1].command; + // Verify the bash -c wrapper is well-formed: starts with bash -c ' and + // ends with matching close quote. The POSIX escape pattern is '\'' + // (close-quote, escaped-quote, re-open-quote) — the end result should + // not have an odd number of unescaped single quotes. + expect(command.startsWith(`bash -c '`)).toBe(true); + expect(command.endsWith(`'`)).toBe(true); + }); +}); diff --git a/packages/sdk/src/workflows/index.ts b/packages/sdk/src/workflows/index.ts index 2d04a933d..edfa8be5b 100644 --- a/packages/sdk/src/workflows/index.ts +++ b/packages/sdk/src/workflows/index.ts @@ -46,3 +46,5 @@ export { executeApiStep, type ApiExecutorOptions } from './api-executor.js'; export type { CloudRunOptions } from './cloud-runner.js'; export * from './proxy-env.js'; export * from './budget-tracker.js'; +export { applySiblingLinks, buildSiblingLinkScript } from './sibling-links.js'; +export type { SiblingLink, SiblingLinkOptions } from './sibling-links.js'; diff --git a/packages/sdk/src/workflows/sibling-links.ts b/packages/sdk/src/workflows/sibling-links.ts new file mode 100644 index 000000000..9c9808e72 --- /dev/null +++ b/packages/sdk/src/workflows/sibling-links.ts @@ -0,0 +1,246 @@ +/** + * Sibling-package link setup for workflows that consume a package living in + * a sibling repo / worktree on disk. + * + * Problem it solves: agents running inside a workflow sometimes find that + * `npm install` (or `pip install`) resolved an older version of a package + * than the one the workflow actually needs — for example when the consumer + * workflow runs before the producer has published a new release. Rather + * than letting agents see a stale interface (and react by augmenting the + * module or writing fallback implementations), linking redirects the + * package resolution at dev-time to the sibling's on-disk build output. + * + * Usage (ESM): + * + * import { workflow, applySiblingLinks } from '@agent-relay/sdk/workflows'; + * + * const base = workflow('my-feature').pattern('dag').agent('impl', ...); + * const wf = applySiblingLinks(base, { + * dependsOn: ['install-deps'], + * links: [ + * { + * name: '@agent-assistant/proactive', + * path: '../agent-assistant/packages/proactive', + * expect: ['recordSignal', 'drainSignals'], + * }, + * { + * name: '@agent-assistant/surfaces', + * path: '../agent-assistant/packages/surfaces', + * expect: ['classifySlackPresenceSignal'], + * }, + * ], + * }); + * + * await wf.step('plan', { agent: 'impl', dependsOn: ['setup-sibling-links'], task: ... }) + * .run({ cwd: process.cwd() }); + * + * MVP language support: npm (package.json), Python (pyproject.toml / + * setup.py / setup.cfg). Auto-detects from the sibling's manifest. Fails + * fast on missing path, unknown manifest, or missing expected exports. + */ + +/** A single sibling package to link into the workflow's working directory. */ +export interface SiblingLink { + /** + * Package name as it appears in imports (e.g. "@agent-assistant/proactive", + * "my_python_pkg"). For Python, use the import name (underscored), not the + * distribution name. + */ + name: string; + + /** + * Path to the sibling package root, relative to the workflow's cwd. + * For npm, this is the directory containing package.json. + * For Python, the directory containing pyproject.toml / setup.py. + */ + path: string; + + /** + * Optional list of top-level named exports / attributes the workflow + * expects to find on the linked package post-setup. When provided, a + * language-appropriate import smoke test runs and fails the step if any + * are missing. + */ + expect?: string[]; +} + +export interface SiblingLinkOptions { + /** Link declarations. All must succeed (fail-fast on any error). */ + links: SiblingLink[]; + + /** + * Step name for the setup step emitted by this helper. + * Defaults to `"setup-sibling-links"`. + */ + stepName?: string; + + /** + * dependsOn for the setup step. Typically `['install-deps']` so that + * `npm install` / `pip install` has run first. + * Defaults to `['install-deps']`. + */ + dependsOn?: string[]; +} + +/** Minimal builder shape — accepts anything with a chainable `.step()` method. */ +interface StepChain { + step: (name: string, cfg: unknown) => StepChain; +} + +/** + * Adds a single deterministic step to the workflow that links each sibling + * package into the workflow's working directory using the appropriate + * language-specific mechanism, then smoke-tests each linked package for + * expected exports. + * + * The step fails fast on: + * - Sibling path missing + * - Unknown manifest (no package.json / pyproject.toml / setup.py) + * - Link command failure + * - Missing expected export + */ +export function applySiblingLinks(wf: T, opts: SiblingLinkOptions): T { + if (opts.links.length === 0) { + return wf; + } + + const stepName = opts.stepName ?? 'setup-sibling-links'; + const dependsOn = opts.dependsOn ?? ['install-deps']; + + const script = buildSiblingLinkScript(opts.links); + const chain = wf as unknown as StepChain; + chain.step(stepName, { + type: 'deterministic', + dependsOn, + command: `bash -c ${shSingleQuote(script)}`, + captureOutput: true, + failOnError: true, + }); + return wf; +} + +// ─── Internal: shell-script generation ───────────────────────────────────── + +/** Shell-quote a string for safe single-quoted inclusion in a bash command. */ +function shSingleQuote(value: string): string { + return `'${value.replace(/'/g, `'\\''`)}'`; +} + +/** JSON-encode a string for safe inclusion inside a shell double-quoted string. */ +function shJsonString(value: string): string { + return JSON.stringify(value); +} + +/** + * Builds a bash script that: + * 1. For each link, detects its manifest and applies the right link command. + * 2. After all links succeed, runs one import smoke test per link that + * declared expected exports. + * + * Exported for test visibility; not part of the public API. + */ +export function buildSiblingLinkScript(links: SiblingLink[]): string { + const lines: string[] = ['set -euo pipefail', 'echo "=== applySiblingLinks: setting up ==="']; + + for (const link of links) { + const escapedName = shJsonString(link.name); + const escapedPath = shJsonString(link.path); + lines.push( + `echo "--- link: ${link.name} <- ${link.path} ---"`, + linkOneBlock(link, escapedName, escapedPath) + ); + } + + lines.push('echo "=== applySiblingLinks: verifying exports ==="'); + for (const link of links) { + if (!link.expect || link.expect.length === 0) { + continue; + } + lines.push(verifyExportsBlock(link)); + } + + lines.push('echo "APPLY_SIBLING_LINKS_OK"'); + return lines.join('\n'); +} + +function linkOneBlock(link: SiblingLink, jsonName: string, jsonPath: string): string { + return [ + `SIBLING_PATH=${jsonPath}`, + `SIBLING_NAME=${jsonName}`, + 'if [ ! -d "$SIBLING_PATH" ]; then', + ' echo "SIBLING_PATH_MISSING: $SIBLING_PATH" >&2', + ' exit 1', + 'fi', + 'if [ -f "$SIBLING_PATH/package.json" ]; then', + ' echo "detected: npm"', + ' ( cd "$SIBLING_PATH" && npm link --silent )', + ' npm link --silent "$SIBLING_NAME"', + 'elif [ -f "$SIBLING_PATH/pyproject.toml" ] || [ -f "$SIBLING_PATH/setup.py" ] || [ -f "$SIBLING_PATH/setup.cfg" ]; then', + ' echo "detected: python"', + ' if command -v uv >/dev/null 2>&1; then', + ' uv pip install -e "$SIBLING_PATH" --quiet', + ' elif command -v pip >/dev/null 2>&1; then', + ' pip install -e "$SIBLING_PATH" --quiet', + ' elif command -v pip3 >/dev/null 2>&1; then', + ' pip3 install -e "$SIBLING_PATH" --quiet', + ' else', + ' echo "NO_PYTHON_INSTALLER: uv / pip / pip3 not found" >&2', + ' exit 1', + ' fi', + 'else', + ' echo "UNKNOWN_MANIFEST: expected package.json / pyproject.toml / setup.py / setup.cfg at $SIBLING_PATH" >&2', + ' exit 1', + 'fi', + ].join('\n'); +} + +function verifyExportsBlock(link: SiblingLink): string { + const jsonName = shJsonString(link.name); + const jsonPath = shJsonString(link.path); + const expectList = JSON.stringify(link.expect ?? []); + // Pick the smoke-test runtime based on what manifest type the sibling had. + return [ + `SIBLING_PATH=${jsonPath}`, + `SIBLING_NAME=${jsonName}`, + `EXPECT=${shJsonString(expectList)}`, + 'if [ -f "$SIBLING_PATH/package.json" ]; then', + nodeVerifyCommand(), + 'else', + pythonVerifyCommand(), + 'fi', + ].join('\n'); +} + +function nodeVerifyCommand(): string { + const script = [ + 'const want = JSON.parse(process.env.APPLY_SIBLING_LINKS_EXPECT);', + 'const name = process.env.APPLY_SIBLING_LINKS_NAME;', + 'const mod = await import(name);', + 'const missing = want.filter((k) => !(k in mod));', + 'if (missing.length) {', + ' console.error(`MISSING_EXPORTS in ${name}: ${missing.join(",")}`);', + ' process.exit(1);', + '}', + 'console.log(`${name} OK: ${want.join(",")}`);', + ].join(' '); + return ` APPLY_SIBLING_LINKS_NAME="$SIBLING_NAME" APPLY_SIBLING_LINKS_EXPECT="$EXPECT" node --input-type=module -e ${shSingleQuote(script)}`; +} + +function pythonVerifyCommand(): string { + const script = [ + 'import json, os, importlib', + 'name = os.environ["APPLY_SIBLING_LINKS_NAME"]', + 'want = json.loads(os.environ["APPLY_SIBLING_LINKS_EXPECT"])', + 'mod = importlib.import_module(name)', + 'missing = [k for k in want if not hasattr(mod, k)]', + 'if missing:', + ' print(f"MISSING_EXPORTS in {name}: {\\",\\".join(missing)}", flush=True)', + ' raise SystemExit(1)', + 'print(f"{name} OK: {\\",\\".join(want)}", flush=True)', + ].join('\n'); + return [ + ' APPLY_SIBLING_LINKS_NAME="$SIBLING_NAME" APPLY_SIBLING_LINKS_EXPECT="$EXPECT" \\', + ` python3 -c ${shSingleQuote(script)} 2>/dev/null || \\`, + ` APPLY_SIBLING_LINKS_NAME="$SIBLING_NAME" APPLY_SIBLING_LINKS_EXPECT="$EXPECT" python -c ${shSingleQuote(script)}`, + ].join('\n'); +} From 8f177506da8172ddd70abc7f001578b54002e85a Mon Sep 17 00:00:00 2001 From: Multi-Repo Pushback Bot Date: Fri, 24 Apr 2026 08:38:31 +0200 Subject: [PATCH 2/3] fix(sdk): address review feedback on PR #776 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Python verify: backslashes inside f-string expressions are a SyntaxError on Python < 3.12. Bind `sep = ","` outside the f-string and reference it from inside (Devin, 🔴 critical — the verifier failed every run previously). - uv dispatch: uv refuses to install outside a venv without --system. Pass --system explicitly AND wrap the uv attempt in an `if ...; then :` so failure falls through to pip/pip3 via the elif chain rather than exiting under `set -e` (Codex, P1 — broken on hosts with uv installed globally but no active venv). - Echo interpolation: the initial script echoed `link.name` / `link.path` into a double-quoted shell string via raw template interpolation, so a name containing `\"`, `$`, or backticks would break quoting or trigger command substitution. Move the echo after the SIBLING_NAME / SIBLING_PATH assignments (which use JSON-encoded safe literals) and reference the shell vars (Devin, 🟡). 3 new tests covering each fix; existing test updated for the --system flag. 15/15 green. --- .../workflows/__tests__/sibling-links.test.ts | 42 ++++++++++++++++++- packages/sdk/src/workflows/sibling-links.ts | 32 ++++++++++---- 2 files changed, 64 insertions(+), 10 deletions(-) diff --git a/packages/sdk/src/workflows/__tests__/sibling-links.test.ts b/packages/sdk/src/workflows/__tests__/sibling-links.test.ts index 8d8702e6c..4b8450ded 100644 --- a/packages/sdk/src/workflows/__tests__/sibling-links.test.ts +++ b/packages/sdk/src/workflows/__tests__/sibling-links.test.ts @@ -15,7 +15,7 @@ describe('buildSiblingLinkScript', () => { const script = buildSiblingLinkScript([{ name: 'my_pkg', path: '../py/pkg' }]); expect(script).toContain('-f "$SIBLING_PATH/pyproject.toml"'); expect(script).toContain('pip install -e'); - expect(script).toContain('uv pip install -e'); + expect(script).toContain('uv pip install --system -e'); }); it('fails-fast shell: script uses set -euo pipefail', () => { @@ -65,6 +65,46 @@ describe('buildSiblingLinkScript', () => { // the non-package.json branch. expect(script).toMatch(/if \[ -f "\$SIBLING_PATH\/package\.json" \]; then[\s\S]+?else[\s\S]+?python/); }); + + it('echoes link name/path via shell vars, not raw interpolation (review: shell injection)', () => { + // Fix for review: a name containing `"$(cmd)` previously broke out of the + // echo's double-quoting and triggered command substitution. With the fix, + // the echo happens AFTER the SIBLING_NAME / SIBLING_PATH assignments + // (which use JSON-encoded safe literals) and references them via + // $-expansion. + const script = buildSiblingLinkScript([{ name: 'pkg"$(evil)', path: '../path"$(also-evil)' }]); + const echoLines = script.split('\n').filter((l) => l.startsWith('echo "--- link:')); + expect(echoLines).toHaveLength(1); + expect(echoLines[0]).toBe('echo "--- link: $SIBLING_NAME <- $SIBLING_PATH ---"'); + const assignmentLines = script + .split('\n') + .filter((l) => l.startsWith('SIBLING_NAME=') || l.startsWith('SIBLING_PATH=')); + expect(assignmentLines.some((l) => l.includes(JSON.stringify('pkg"$(evil)')))).toBe(true); + expect(assignmentLines.some((l) => l.includes(JSON.stringify('../path"$(also-evil)')))).toBe(true); + }); + + it('uv is invoked with --system and falls through to pip on failure (review: non-venv)', () => { + // Fix for review: uv refuses to install outside a venv without --system. + // The dispatch now uses --system AND wraps the uv attempt in an `if` so + // failure falls through to pip/pip3 instead of exiting under `set -e`. + const script = buildSiblingLinkScript([{ name: 'p', path: '../p' }]); + expect(script).toContain('uv pip install --system -e'); + expect(script).toMatch( + /if command -v uv[^\n]+uv pip install --system[^\n]+; then\s*\n\s*:\s*\n\s*elif command -v pip/ + ); + }); + + it('python verifier avoids backslashes inside f-string expressions (review: Python < 3.12 SyntaxError)', () => { + // Fix for review: backslashes (e.g. `\",\"`) inside f-string expression + // braces are a SyntaxError on Python < 3.12. We bind `sep = ","` outside + // the f-string and reference it from inside. The old escaped form must + // not appear anywhere in the emitted script. + const script = buildSiblingLinkScript([{ name: 'p', path: './p', expect: ['foo'] }]); + expect(script).toContain('sep = ","'); + expect(script).toContain('sep.join(missing)'); + expect(script).toContain('sep.join(want)'); + expect(script).not.toContain('\\",\\".join('); + }); }); describe('applySiblingLinks', () => { diff --git a/packages/sdk/src/workflows/sibling-links.ts b/packages/sdk/src/workflows/sibling-links.ts index 9c9808e72..7497167cf 100644 --- a/packages/sdk/src/workflows/sibling-links.ts +++ b/packages/sdk/src/workflows/sibling-links.ts @@ -145,10 +145,11 @@ export function buildSiblingLinkScript(links: SiblingLink[]): string { for (const link of links) { const escapedName = shJsonString(link.name); const escapedPath = shJsonString(link.path); - lines.push( - `echo "--- link: ${link.name} <- ${link.path} ---"`, - linkOneBlock(link, escapedName, escapedPath) - ); + // Bind the shell vars BEFORE echoing so we don't interpolate unescaped + // link.name / link.path (which may contain `"`, `$`, backticks) into a + // double-quoted echo. Use them via $SIBLING_NAME / $SIBLING_PATH, which + // are already quoted-safe because shJsonString produced them. + lines.push(linkOneBlock(link, escapedName, escapedPath)); } lines.push('echo "=== applySiblingLinks: verifying exports ==="'); @@ -164,9 +165,11 @@ export function buildSiblingLinkScript(links: SiblingLink[]): string { } function linkOneBlock(link: SiblingLink, jsonName: string, jsonPath: string): string { + void link; return [ `SIBLING_PATH=${jsonPath}`, `SIBLING_NAME=${jsonName}`, + 'echo "--- link: $SIBLING_NAME <- $SIBLING_PATH ---"', 'if [ ! -d "$SIBLING_PATH" ]; then', ' echo "SIBLING_PATH_MISSING: $SIBLING_PATH" >&2', ' exit 1', @@ -177,14 +180,20 @@ function linkOneBlock(link: SiblingLink, jsonName: string, jsonPath: string): st ' npm link --silent "$SIBLING_NAME"', 'elif [ -f "$SIBLING_PATH/pyproject.toml" ] || [ -f "$SIBLING_PATH/setup.py" ] || [ -f "$SIBLING_PATH/setup.cfg" ]; then', ' echo "detected: python"', - ' if command -v uv >/dev/null 2>&1; then', - ' uv pip install -e "$SIBLING_PATH" --quiet', + // Try uv first (fastest when available), but uv refuses to install + // outside a venv without --system. Pass --system explicitly so uv + // works in non-venv sandboxes (common CI/agent runner shape). + // If uv still fails (e.g. broken install), fall through to pip/pip3 + // via the explicit OR chain rather than relying on `set -e` to + // short-circuit between elif branches. + ' if command -v uv >/dev/null 2>&1 && uv pip install --system -e "$SIBLING_PATH" --quiet 2>/dev/null; then', + ' :', ' elif command -v pip >/dev/null 2>&1; then', ' pip install -e "$SIBLING_PATH" --quiet', ' elif command -v pip3 >/dev/null 2>&1; then', ' pip3 install -e "$SIBLING_PATH" --quiet', ' else', - ' echo "NO_PYTHON_INSTALLER: uv / pip / pip3 not found" >&2', + ' echo "NO_PYTHON_INSTALLER: uv / pip / pip3 not found or all failed" >&2', ' exit 1', ' fi', 'else', @@ -227,16 +236,21 @@ function nodeVerifyCommand(): string { } function pythonVerifyCommand(): string { + // Python < 3.12 forbids backslashes inside f-string expressions, so we + // can't inline `{",".join(missing)}` (which needs `\",\".` when written + // as a JS string literal). Bind the separator to a name outside the + // f-string first. const script = [ 'import json, os, importlib', 'name = os.environ["APPLY_SIBLING_LINKS_NAME"]', 'want = json.loads(os.environ["APPLY_SIBLING_LINKS_EXPECT"])', 'mod = importlib.import_module(name)', 'missing = [k for k in want if not hasattr(mod, k)]', + 'sep = ","', 'if missing:', - ' print(f"MISSING_EXPORTS in {name}: {\\",\\".join(missing)}", flush=True)', + ' print(f"MISSING_EXPORTS in {name}: {sep.join(missing)}", flush=True)', ' raise SystemExit(1)', - 'print(f"{name} OK: {\\",\\".join(want)}", flush=True)', + 'print(f"{name} OK: {sep.join(want)}", flush=True)', ].join('\n'); return [ ' APPLY_SIBLING_LINKS_NAME="$SIBLING_NAME" APPLY_SIBLING_LINKS_EXPECT="$EXPECT" \\', From 16959fe2401f993f0932756206737cb6731139de Mon Sep 17 00:00:00 2001 From: Multi-Repo Pushback Bot Date: Fri, 24 Apr 2026 09:11:10 +0200 Subject: [PATCH 3/3] fix(sdk): single-quote shell assignments to block \$() / backtick substitution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the earlier shell-injection fix on #776. Moving the echo after the variable assignments was necessary but not sufficient — the assignments themselves used JSON.stringify (shJsonString) which produces DOUBLE-quoted bash literals. Double-quoted strings in bash still expand \$VAR, \$(cmd), and \`cmd\`, so a value like \`../path\$(evil)\` still triggered command substitution on assignment. Switch all three assignment sites (SIBLING_PATH, SIBLING_NAME, EXPECT) to single-quoted bash literals via shSingleQuote, which are literal end-to- end. Embedded single quotes are handled by the existing '\\'' POSIX idiom. The EXPECT JSON payload passes through bash untouched and downstream Node/Python JSON.parse it back. Removes the now-unused shJsonString helper. Updates two tests to assert the single-quoted form; adds one more test covering the embedded-single- quote escape path. 16/16 tests green. --- .../workflows/__tests__/sibling-links.test.ts | 47 ++++++++++++------- packages/sdk/src/workflows/sibling-links.ts | 47 ++++++++++--------- 2 files changed, 57 insertions(+), 37 deletions(-) diff --git a/packages/sdk/src/workflows/__tests__/sibling-links.test.ts b/packages/sdk/src/workflows/__tests__/sibling-links.test.ts index 4b8450ded..2ec049be5 100644 --- a/packages/sdk/src/workflows/__tests__/sibling-links.test.ts +++ b/packages/sdk/src/workflows/__tests__/sibling-links.test.ts @@ -47,14 +47,15 @@ describe('buildSiblingLinkScript', () => { expect(script).toContain('APPLY_SIBLING_LINKS_OK'); }); - it('JSON-encodes expected exports safely for shell and downstream JSON.parse', () => { + it('expects-list survives bash env var round-trip via single-quoted JSON payload', () => { const script = buildSiblingLinkScript([{ name: 'p', path: './p', expect: ["it's-ok", 'with"quote'] }]); - // Expect list is embedded as: EXPECT= - // so the inner JSON survives round-trip through bash env var into - // Node.JSON.parse / Python json.loads. - const expectedInner = JSON.stringify(["it's-ok", 'with"quote']); - const expectedShellArg = JSON.stringify(expectedInner); - expect(script).toContain(`EXPECT=${expectedShellArg}`); + // Assignment is `EXPECT=''` where the JSON is single-quoted so + // bash leaves it literal (no `$` / backtick substitution), then + // Node/Python JSON.parse it back to the original array. Embedded `'` + // gets the '\'' POSIX-escape treatment. + const jsonPayload = JSON.stringify(["it's-ok", 'with"quote']); + const shellArg = `'${jsonPayload.replace(/'/g, `'\\''`)}'`; + expect(script).toContain(`EXPECT=${shellArg}`); }); it('emits both node and python verifiers wrapped in manifest-conditional', () => { @@ -66,21 +67,35 @@ describe('buildSiblingLinkScript', () => { expect(script).toMatch(/if \[ -f "\$SIBLING_PATH\/package\.json" \]; then[\s\S]+?else[\s\S]+?python/); }); - it('echoes link name/path via shell vars, not raw interpolation (review: shell injection)', () => { - // Fix for review: a name containing `"$(cmd)` previously broke out of the - // echo's double-quoting and triggered command substitution. With the fix, - // the echo happens AFTER the SIBLING_NAME / SIBLING_PATH assignments - // (which use JSON-encoded safe literals) and references them via - // $-expansion. - const script = buildSiblingLinkScript([{ name: 'pkg"$(evil)', path: '../path"$(also-evil)' }]); + it('assignments use single-quoted literals so $() / backticks do not substitute (review: shell injection)', () => { + // Two-stage review fix: + // (1) echo happens AFTER assignments and references the shell vars, + // not raw link.name / link.path template interpolation. + // (2) assignments themselves use SINGLE-quoted bash literals so that + // `$(cmd)` and backticks inside the value are NOT interpreted as + // command substitution (which JSON.stringify / double-quoted form + // did NOT protect against). + const script = buildSiblingLinkScript([{ name: 'pkg$(evil)', path: '../path`also-evil`' }]); const echoLines = script.split('\n').filter((l) => l.startsWith('echo "--- link:')); expect(echoLines).toHaveLength(1); expect(echoLines[0]).toBe('echo "--- link: $SIBLING_NAME <- $SIBLING_PATH ---"'); const assignmentLines = script .split('\n') .filter((l) => l.startsWith('SIBLING_NAME=') || l.startsWith('SIBLING_PATH=')); - expect(assignmentLines.some((l) => l.includes(JSON.stringify('pkg"$(evil)')))).toBe(true); - expect(assignmentLines.some((l) => l.includes(JSON.stringify('../path"$(also-evil)')))).toBe(true); + // Assignments should wrap the value in single quotes — the exact literal + // passes through bash. `$(evil)` sits inside single quotes → no + // substitution; same for backticks. + expect(assignmentLines.some((l) => l === "SIBLING_NAME='pkg$(evil)'")).toBe(true); + expect(assignmentLines.some((l) => l === "SIBLING_PATH='../path`also-evil`'")).toBe(true); + // Sanity: no double-quoted assignment form present for these lines. + expect(assignmentLines.some((l) => l.startsWith('SIBLING_NAME="'))).toBe(false); + expect(assignmentLines.some((l) => l.startsWith('SIBLING_PATH="'))).toBe(false); + }); + + it("escapes embedded single quotes in link values via POSIX '\\'' idiom", () => { + const script = buildSiblingLinkScript([{ name: "pkg'q", path: "../p'q" }]); + expect(script).toContain("SIBLING_NAME='pkg'\\''q'"); + expect(script).toContain("SIBLING_PATH='../p'\\''q'"); }); it('uv is invoked with --system and falls through to pip on failure (review: non-venv)', () => { diff --git a/packages/sdk/src/workflows/sibling-links.ts b/packages/sdk/src/workflows/sibling-links.ts index 7497167cf..b8ba7a887 100644 --- a/packages/sdk/src/workflows/sibling-links.ts +++ b/packages/sdk/src/workflows/sibling-links.ts @@ -121,16 +121,20 @@ export function applySiblingLinks(wf: T, opts: SiblingLinkOptions): T { // ─── Internal: shell-script generation ───────────────────────────────────── -/** Shell-quote a string for safe single-quoted inclusion in a bash command. */ +/** + * Shell-quote a string for safe single-quoted inclusion in a bash command. + * Single-quoted strings in bash are literal for every character except the + * single quote itself, so `$` and backticks are NOT interpreted — which is + * exactly what we want for link.name / link.path / JSON payloads that must + * pass through bash unchanged. + * + * Embedded single quotes are escaped via the standard `'\''` POSIX idiom + * (close, escape, reopen). + */ function shSingleQuote(value: string): string { return `'${value.replace(/'/g, `'\\''`)}'`; } -/** JSON-encode a string for safe inclusion inside a shell double-quoted string. */ -function shJsonString(value: string): string { - return JSON.stringify(value); -} - /** * Builds a bash script that: * 1. For each link, detects its manifest and applies the right link command. @@ -143,12 +147,11 @@ export function buildSiblingLinkScript(links: SiblingLink[]): string { const lines: string[] = ['set -euo pipefail', 'echo "=== applySiblingLinks: setting up ==="']; for (const link of links) { - const escapedName = shJsonString(link.name); - const escapedPath = shJsonString(link.path); - // Bind the shell vars BEFORE echoing so we don't interpolate unescaped - // link.name / link.path (which may contain `"`, `$`, backticks) into a - // double-quoted echo. Use them via $SIBLING_NAME / $SIBLING_PATH, which - // are already quoted-safe because shJsonString produced them. + // Use SINGLE-quoted shell literals for the assignments. Double-quoted + // literals (via JSON.stringify) would let `$`, backticks, and `\` still + // trigger substitution or escaping — single-quoted is literal end-to-end. + const escapedName = shSingleQuote(link.name); + const escapedPath = shSingleQuote(link.path); lines.push(linkOneBlock(link, escapedName, escapedPath)); } @@ -164,11 +167,11 @@ export function buildSiblingLinkScript(links: SiblingLink[]): string { return lines.join('\n'); } -function linkOneBlock(link: SiblingLink, jsonName: string, jsonPath: string): string { +function linkOneBlock(link: SiblingLink, escapedName: string, escapedPath: string): string { void link; return [ - `SIBLING_PATH=${jsonPath}`, - `SIBLING_NAME=${jsonName}`, + `SIBLING_PATH=${escapedPath}`, + `SIBLING_NAME=${escapedName}`, 'echo "--- link: $SIBLING_NAME <- $SIBLING_PATH ---"', 'if [ ! -d "$SIBLING_PATH" ]; then', ' echo "SIBLING_PATH_MISSING: $SIBLING_PATH" >&2', @@ -204,14 +207,16 @@ function linkOneBlock(link: SiblingLink, jsonName: string, jsonPath: string): st } function verifyExportsBlock(link: SiblingLink): string { - const jsonName = shJsonString(link.name); - const jsonPath = shJsonString(link.path); - const expectList = JSON.stringify(link.expect ?? []); + const escapedName = shSingleQuote(link.name); + const escapedPath = shSingleQuote(link.path); + const expectJson = JSON.stringify(link.expect ?? []); // Pick the smoke-test runtime based on what manifest type the sibling had. + // Single-quoted assignments are literal — the JSON payload inside EXPECT + // survives bash untouched and downstream Node/Python JSON.parse it back. return [ - `SIBLING_PATH=${jsonPath}`, - `SIBLING_NAME=${jsonName}`, - `EXPECT=${shJsonString(expectList)}`, + `SIBLING_PATH=${escapedPath}`, + `SIBLING_NAME=${escapedName}`, + `EXPECT=${shSingleQuote(expectJson)}`, 'if [ -f "$SIBLING_PATH/package.json" ]; then', nodeVerifyCommand(), 'else',