From 106788cdb3e1db9f070d011a404dcba405f99526 Mon Sep 17 00:00:00 2001 From: Miya Date: Tue, 25 Aug 2026 21:37:34 +0200 Subject: [PATCH 1/5] fix: guard deterministic repair scope Session-Id: 01a03a38-b143-7000-84b5-2fc9c9c26f69 --- .../__tests__/builder-deterministic.test.ts | 2 + .../src/__tests__/repair-scope-guard.test.ts | 288 ++++++++++++++++++ .../workflow-reliability-contract.test.ts | 1 + packages/core/src/builder.ts | 7 + packages/core/src/custom-steps.ts | 25 ++ packages/core/src/index.ts | 1 + packages/core/src/repair-protection.ts | 279 +++++++++++++++++ packages/core/src/runner.ts | 285 +++++++++++++++-- packages/core/src/schema.json | 26 ++ packages/core/src/schema.ts | 8 + packages/core/src/types.ts | 2 + 11 files changed, 905 insertions(+), 19 deletions(-) create mode 100644 packages/core/src/__tests__/repair-scope-guard.test.ts create mode 100644 packages/core/src/repair-protection.ts diff --git a/packages/core/src/__tests__/builder-deterministic.test.ts b/packages/core/src/__tests__/builder-deterministic.test.ts index 80caa21..7c8be24 100644 --- a/packages/core/src/__tests__/builder-deterministic.test.ts +++ b/packages/core/src/__tests__/builder-deterministic.test.ts @@ -47,6 +47,7 @@ describe('deterministic/worktree steps in builder', () => { captureOutput: true, failOnError: false, terminalSuccessExitCodes: [78], + repairProtection: { protectedPaths: ['scripts/gate.sh'] }, dependsOn: ['build'], timeoutMs: 30000, }) @@ -57,6 +58,7 @@ describe('deterministic/worktree steps in builder', () => { expect(step.captureOutput).toBe(true); expect(step.failOnError).toBe(false); expect(step.terminalSuccessExitCodes).toEqual([78]); + expect(step.repairProtection).toEqual({ protectedPaths: ['scripts/gate.sh'] }); expect(step.dependsOn).toEqual(['build']); expect(step.timeoutMs).toBe(30000); }); diff --git a/packages/core/src/__tests__/repair-scope-guard.test.ts b/packages/core/src/__tests__/repair-scope-guard.test.ts new file mode 100644 index 0000000..2c64fd4 --- /dev/null +++ b/packages/core/src/__tests__/repair-scope-guard.test.ts @@ -0,0 +1,288 @@ +import { mkdtempSync, readFileSync, rmSync, symlinkSync, unlinkSync, writeFileSync } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const { executeApiStepMock } = vi.hoisted(() => ({ executeApiStepMock: vi.fn() })); +vi.mock('../api-executor.js', () => ({ executeApiStep: executeApiStepMock })); + +import { RepairScopeViolationError } from '../repair-protection.js'; +import { WorkflowRunner } from '../runner.js'; +import type { AgentDefinition, RelayYamlConfig, WorkflowStep } from '../types.js'; + +const fixer = (cli: 'claude' | 'api' = 'claude'): AgentDefinition => ({ + name: 'fixer', + cli, + role: 'implementation engineer', + interactive: false, +}); + +function repairContext( + cwd: string, + options: { + cli?: 'claude' | 'api'; + protectedPaths?: string[]; + command?: string; + verification?: WorkflowStep['verification']; + } = {} +) { + return { + step: { + name: 'gate', + type: 'deterministic', + command: options.command ?? 'node -e "process.exit(1)"', + repairProtection: options.protectedPaths + ? { protectedPaths: options.protectedPaths } + : undefined, + verification: options.verification, + }, + agentDef: fixer(options.cli), + attempt: 1, + maxRetries: 1, + command: options.command ?? 'node -e "process.exit(1)"', + cwd, + error: 'gate failed', + output: 'broken', + }; +} + +describe('deterministic repair scope guard', () => { + let cwd: string; + + beforeEach(() => { + cwd = mkdtempSync(path.join(os.tmpdir(), 'relay-repair-scope-')); + executeApiStepMock.mockReset(); + }); + + afterEach(() => { + rmSync(cwd, { recursive: true, force: true }); + vi.restoreAllMocks(); + }); + + it('allows repair to change mutable state while preserving protected bytes', async () => { + writeFileSync(path.join(cwd, 'gate.js'), 'original gate\n'); + const runner = new WorkflowRunner({ + cwd, + executor: { + executeAgentStep: vi.fn(async () => { + writeFileSync(path.join(cwd, 'state.txt'), 'fixed\n'); + return 'fixed mutable state'; + }), + }, + }); + + await (runner as any).runDeterministicRepairAgent( + repairContext(cwd, { protectedPaths: ['gate.js'] }) + ); + + expect(readFileSync(path.join(cwd, 'gate.js'), 'utf8')).toBe('original gate\n'); + expect(readFileSync(path.join(cwd, 'state.txt'), 'utf8')).toBe('fixed\n'); + }); + + it('detects and restores a protected modification through the injected executor path', async () => { + const protectedPath = path.join(cwd, 'gate.js'); + writeFileSync(protectedPath, 'original\n', { mode: 0o755 }); + const runner = new WorkflowRunner({ + cwd, + executor: { + executeAgentStep: vi.fn(async () => { + writeFileSync(protectedPath, 'tampered\n'); + return 'changed gate'; + }), + }, + }); + + await expect( + (runner as any).runDeterministicRepairAgent( + repairContext(cwd, { protectedPaths: ['gate.js'] }) + ) + ).rejects.toBeInstanceOf(RepairScopeViolationError); + expect(readFileSync(protectedPath, 'utf8')).toBe('original\n'); + }); + + it('detects and restores a protected deletion through the API executor path', async () => { + const protectedPath = path.join(cwd, 'gate.py'); + writeFileSync(protectedPath, 'print("gate")\n'); + executeApiStepMock.mockImplementation(async () => { + unlinkSync(protectedPath); + return 'deleted gate'; + }); + const runner = new WorkflowRunner({ cwd, sandbox: { provider: 'none' } }); + + await expect( + (runner as any).runDeterministicRepairAgent( + repairContext(cwd, { cli: 'api', protectedPaths: ['gate.py'] }) + ) + ).rejects.toBeInstanceOf(RepairScopeViolationError); + expect(readFileSync(protectedPath, 'utf8')).toBe('print("gate")\n'); + }); + + it('detects and removes a protected creation through the CLI executor path', async () => { + const protectedPath = path.join(cwd, 'must-stay-absent.sh'); + const runner = new WorkflowRunner({ cwd, sandbox: { provider: 'none' } }); + vi.spyOn(runner as any, 'execNonInteractive').mockImplementation(async () => { + writeFileSync(protectedPath, '#!/bin/sh\n'); + return { output: 'created file' }; + }); + + await expect( + (runner as any).runDeterministicRepairAgent( + repairContext(cwd, { protectedPaths: ['must-stay-absent.sh'] }) + ) + ).rejects.toBeInstanceOf(RepairScopeViolationError); + expect(() => readFileSync(protectedPath)).toThrow(); + }); + + it('resolves symlinks before hashing and restores a protected symlink swap', async () => { + const protectedPath = path.join(cwd, 'gate.js'); + const aliasTarget = path.join(cwd, 'attacker.js'); + writeFileSync(protectedPath, 'original\n'); + writeFileSync(aliasTarget, 'attacker\n'); + const runner = new WorkflowRunner({ + cwd, + executor: { + executeAgentStep: vi.fn(async () => { + unlinkSync(protectedPath); + symlinkSync(aliasTarget, protectedPath); + return 'swapped gate'; + }), + }, + }); + + await expect( + (runner as any).runDeterministicRepairAgent( + repairContext(cwd, { protectedPaths: ['gate.js'] }) + ) + ).rejects.toBeInstanceOf(RepairScopeViolationError); + expect(readFileSync(protectedPath, 'utf8')).toBe('original\n'); + expect(readFileSync(aliasTarget, 'utf8')).toBe('attacker\n'); + }); + + it('checks and restores protection when the repair executor times out', async () => { + const protectedPath = path.join(cwd, 'gate.js'); + writeFileSync(protectedPath, 'original\n'); + const runner = new WorkflowRunner({ + cwd, + executor: { + executeAgentStep: vi.fn(async () => { + writeFileSync(protectedPath, 'tampered before timeout\n'); + throw new Error('repair timed out'); + }), + }, + }); + + await expect( + (runner as any).runDeterministicRepairAgent( + repairContext(cwd, { protectedPaths: ['gate.js'] }) + ) + ).rejects.toBeInstanceOf(RepairScopeViolationError); + expect(readFileSync(protectedPath, 'utf8')).toBe('original\n'); + }); + + it('auto-protects directly invoked gate and custom verification scripts', async () => { + const gatePath = path.join(cwd, 'gate.js'); + const verificationPath = path.join(cwd, 'verify.sh'); + writeFileSync(gatePath, 'process.exit(1)\n'); + writeFileSync(verificationPath, '#!/bin/sh\nexit 1\n'); + const runner = new WorkflowRunner({ + cwd, + executor: { + executeAgentStep: vi.fn(async () => { + writeFileSync(verificationPath, '#!/bin/sh\nexit 0\n'); + return 'changed verification'; + }), + }, + }); + + await expect( + (runner as any).runDeterministicRepairAgent( + repairContext(cwd, { + command: 'node ./gate.js', + verification: { type: 'custom', value: 'bash ./verify.sh' }, + }) + ) + ).rejects.toBeInstanceOf(RepairScopeViolationError); + expect(readFileSync(verificationPath, 'utf8')).toBe('#!/bin/sh\nexit 1\n'); + }); + + it('skips repair loudly when indirect gate code is unresolvable and no explicit protection exists', async () => { + const executeAgentStep = vi.fn(async () => 'unexpected repair'); + const runner = new WorkflowRunner({ cwd, executor: { executeAgentStep } }); + const log = vi.spyOn(runner as any, 'log'); + + await (runner as any).runDeterministicRepairAgent( + repairContext(cwd, { command: 'npm test' }) + ); + + expect(executeAgentStep).not.toHaveBeenCalled(); + expect(log).toHaveBeenCalledWith(expect.stringContaining('REPAIR PROTECTION WARNING')); + }); + + it('always protects a workflow YAML loaded from disk', async () => { + const workflowPath = path.join(cwd, 'relay.yaml'); + writeFileSync( + workflowPath, + 'version: "1"\nname: guarded\nswarm:\n pattern: pipeline\nagents: []\nworkflows:\n - name: default\n steps:\n - name: gate\n type: deterministic\n command: "false"\n' + ); + const runner = new WorkflowRunner({ + cwd, + executor: { + executeAgentStep: vi.fn(async () => { + writeFileSync(workflowPath, 'tampered: true\n'); + return 'changed workflow'; + }), + }, + }); + await runner.parseYamlFile('relay.yaml'); + + await expect( + (runner as any).runDeterministicRepairAgent(repairContext(cwd)) + ).rejects.toBeInstanceOf(RepairScopeViolationError); + expect(readFileSync(workflowPath, 'utf8')).toContain('name: guarded'); + }); + + it('uses the same guard for an environmental gate failure and never reruns after violation', async () => { + const protectedPath = path.join(cwd, 'gate-state.txt'); + writeFileSync(protectedPath, 'original\n'); + const executeDeterministicStep = vi.fn(async () => { + throw new Error('environmental gate timeout'); + }); + const executeAgentStep = vi.fn(async () => { + writeFileSync(protectedPath, 'tampered\n'); + return 'repair after environmental failure'; + }); + const runner = new WorkflowRunner({ + cwd, + executor: { executeDeterministicStep, executeAgentStep }, + }); + const config: RelayYamlConfig = { + version: '1', + name: 'environmental-guard', + swarm: { pattern: 'pipeline' }, + agents: [fixer()], + errorHandling: { strategy: 'retry', repairRetries: 1, retryDelayMs: 1 }, + workflows: [ + { + name: 'default', + steps: [ + { + name: 'gate', + type: 'deterministic', + command: 'node -e "process.exit(1)"', + repairProtection: { protectedPaths: ['gate-state.txt'] }, + }, + ], + }, + ], + trajectories: false, + }; + + const run = await runner.execute(config, 'default'); + + expect(run.status).toBe('failed'); + expect(run.error).toContain('Repair scope violation'); + expect(executeDeterministicStep).toHaveBeenCalledTimes(1); + expect(executeAgentStep).toHaveBeenCalledTimes(1); + expect(readFileSync(protectedPath, 'utf8')).toBe('original\n'); + }); +}); diff --git a/packages/core/src/__tests__/workflow-reliability-contract.test.ts b/packages/core/src/__tests__/workflow-reliability-contract.test.ts index 56406e5..eed8495 100644 --- a/packages/core/src/__tests__/workflow-reliability-contract.test.ts +++ b/packages/core/src/__tests__/workflow-reliability-contract.test.ts @@ -352,6 +352,7 @@ describe('workflow reliability contract', () => { command: 'npm run typecheck && npm test', captureOutput: true, failOnError: true, + repairProtection: { protectedPaths: ['package.json'] }, }, ], }, diff --git a/packages/core/src/builder.ts b/packages/core/src/builder.ts index 0d70ad8..92a6f07 100644 --- a/packages/core/src/builder.ts +++ b/packages/core/src/builder.ts @@ -123,6 +123,8 @@ export interface DeterministicStepOptions { failOnError?: boolean; /** Exit codes that end the workflow with the distinct completed_early status. */ terminalSuccessExitCodes?: number[]; + /** Files a repair agent must not modify, relative to this step's cwd. */ + repairProtection?: WorkflowStep['repairProtection']; dependsOn?: string[]; verification?: VerificationCheck; timeoutMs?: number; @@ -430,6 +432,11 @@ export class WorkflowBuilder { if (options.terminalSuccessExitCodes !== undefined) { step.terminalSuccessExitCodes = [...options.terminalSuccessExitCodes]; } + if (options.repairProtection !== undefined) { + step.repairProtection = { + protectedPaths: [...options.repairProtection.protectedPaths], + }; + } if (options.dependsOn !== undefined) step.dependsOn = options.dependsOn; if (options.verification !== undefined) step.verification = options.verification; if (options.timeoutMs !== undefined) step.timeoutMs = options.timeoutMs; diff --git a/packages/core/src/custom-steps.ts b/packages/core/src/custom-steps.ts index ba1a2f2..c80f115 100644 --- a/packages/core/src/custom-steps.ts +++ b/packages/core/src/custom-steps.ts @@ -208,6 +208,28 @@ function validateCustomStepDefinition( } } + if (stepDef.repairProtection !== undefined) { + const repairProtection = stepDef.repairProtection; + const protectedPaths = + typeof repairProtection === 'object' && repairProtection !== null + ? (repairProtection as Record).protectedPaths + : undefined; + if ( + stepType !== 'deterministic' || + typeof repairProtection !== 'object' || + repairProtection === null || + !Array.isArray(protectedPaths) || + protectedPaths.length === 0 || + protectedPaths.some((entry) => typeof entry !== 'string' || entry.trim().length === 0) + ) { + throw new CustomStepsParseError( + `Invalid repairProtection for step "${name}"`, + 'repairProtection.protectedPaths must be a non-empty array of non-empty strings on a deterministic step', + filePath + ); + } + } + if (stepType === 'worktree' && !hasBranch) { throw new CustomStepsParseError( `Worktree step "${name}" is missing "branch"`, @@ -442,6 +464,9 @@ export function resolveCustomStep( resolvedStep.terminalSuccessExitCodes = customDef.terminalSuccessExitCodes ? [...customDef.terminalSuccessExitCodes] : undefined; + resolvedStep.repairProtection = customDef.repairProtection + ? { protectedPaths: [...customDef.repairProtection.protectedPaths] } + : undefined; } else if (stepType === 'worktree') { resolvedStep.branch = interpolate(customDef.branch); resolvedStep.baseBranch = interpolate(customDef.baseBranch); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 2269a0b..3a504d5 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -13,6 +13,7 @@ export * from './sandbox-local-runtime.js'; export * from './run-summary-table.js'; export * from './template-resolver.js'; export * from './verification.js'; +export * from './repair-protection.js'; export { StepExecutor, /** @deprecated Use {@link StepExecutor} instead. */ diff --git a/packages/core/src/repair-protection.ts b/packages/core/src/repair-protection.ts new file mode 100644 index 0000000..346c5c9 --- /dev/null +++ b/packages/core/src/repair-protection.ts @@ -0,0 +1,279 @@ +import { createHash } from 'node:crypto'; +import { + chmodSync, + lstatSync, + mkdirSync, + readFileSync, + readlinkSync, + readdirSync, + realpathSync, + rmSync, + symlinkSync, + writeFileSync, +} from 'node:fs'; +import path from 'node:path'; + +export interface RepairScopeViolation { + path: string; + reason: string; + expectedType: ProtectedNodeType; + actualType: ProtectedNodeType; + expectedSha256: string; + actualSha256: string; + expectedCanonicalPath: string; + actualCanonicalPath: string; + restoreError?: string; +} + +export class RepairScopeViolationError extends Error { + constructor(public readonly violations: RepairScopeViolation[]) { + super( + `Repair scope violation: ${violations + .map((violation) => `${violation.path} (${violation.reason})`) + .join(', ')}` + ); + this.name = 'RepairScopeViolationError'; + } +} + +export type ProtectedNodeType = 'absent' | 'file' | 'directory' | 'symlink' | 'other' | 'cycle'; + +interface ProtectedNode { + type: ProtectedNodeType; + mode: number; + bytes?: Buffer; + linkTarget?: string; + canonicalPath?: string; + entries?: Array<{ name: string; node: ProtectedNode }>; + sha256: string; +} + +interface ProtectedPathSnapshot { + requestedPath: string; + canonicalPath: string; + logicalType: ProtectedNodeType; + logicalMode: number; + logicalLinkTarget?: string; + node: ProtectedNode; +} + +const modeBits = (mode: number): number => mode & 0o7777; +const digest = (chunks: Array): string => { + const hash = createHash('sha256'); + for (const chunk of chunks) hash.update(chunk); + return hash.digest('hex'); +}; + +function nodeType(filePath: string): { type: ProtectedNodeType; mode: number; linkTarget?: string } { + try { + const info = lstatSync(filePath); + if (info.isSymbolicLink()) { + return { type: 'symlink', mode: modeBits(info.mode), linkTarget: readlinkSync(filePath) }; + } + if (info.isFile()) return { type: 'file', mode: modeBits(info.mode) }; + if (info.isDirectory()) return { type: 'directory', mode: modeBits(info.mode) }; + return { type: 'other', mode: modeBits(info.mode) }; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return { type: 'absent', mode: 0 }; + throw error; + } +} + +/** Resolve every existing symlink component, including for a path that is currently absent. */ +export function canonicalizeProtectedPath(filePath: string): string { + const absolute = path.resolve(filePath); + try { + return realpathSync.native(absolute); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; + } + + const missing: string[] = []; + let cursor = absolute; + while (true) { + const parent = path.dirname(cursor); + if (parent === cursor) return absolute; + missing.unshift(path.basename(cursor)); + cursor = parent; + try { + return path.join(realpathSync.native(cursor), ...missing); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; + } + } +} + +function captureNode(filePath: string, ancestors = new Set()): ProtectedNode { + const meta = nodeType(filePath); + if (meta.type === 'absent') { + return { type: 'absent', mode: 0, sha256: digest(['absent']) }; + } + + if (meta.type === 'symlink') { + const canonicalPath = canonicalizeProtectedPath(filePath); + if (ancestors.has(canonicalPath)) { + return { + type: 'cycle', + mode: meta.mode, + linkTarget: meta.linkTarget, + canonicalPath, + sha256: digest(['cycle', meta.linkTarget ?? '', canonicalPath]), + }; + } + const nextAncestors = new Set(ancestors); + nextAncestors.add(canonicalPath); + const target = captureNode(canonicalPath, nextAncestors); + return { + type: 'symlink', + mode: meta.mode, + bytes: Buffer.from(meta.linkTarget ?? ''), + linkTarget: meta.linkTarget, + canonicalPath, + entries: [{ name: '', node: target }], + sha256: digest(['symlink', String(meta.mode), meta.linkTarget ?? '', canonicalPath, target.sha256]), + }; + } + + if (meta.type === 'file') { + const bytes = readFileSync(filePath); + return { + type: 'file', + mode: meta.mode, + bytes, + sha256: digest(['file', String(meta.mode), bytes]), + }; + } + + if (meta.type === 'directory') { + const entries = readdirSync(filePath) + .sort((left, right) => left.localeCompare(right)) + .map((name) => ({ name, node: captureNode(path.join(filePath, name), ancestors) })); + return { + type: 'directory', + mode: meta.mode, + entries, + sha256: digest([ + 'directory', + String(meta.mode), + ...entries.flatMap(({ name, node }) => [name, node.type, node.sha256]), + ]), + }; + } + + return { type: 'other', mode: meta.mode, sha256: digest(['other', String(meta.mode)]) }; +} + +function restoreNode(filePath: string, node: ProtectedNode, restoredTargets = new Set()): void { + if (node.type === 'absent') { + rmSync(filePath, { recursive: true, force: true }); + return; + } + + if (node.type === 'cycle') return; + + if (node.type === 'symlink') { + const targetNode = node.entries?.[0]?.node; + if (node.canonicalPath && targetNode && !restoredTargets.has(node.canonicalPath)) { + restoredTargets.add(node.canonicalPath); + restoreNode(node.canonicalPath, targetNode, restoredTargets); + } + rmSync(filePath, { recursive: true, force: true }); + mkdirSync(path.dirname(filePath), { recursive: true }); + symlinkSync(node.linkTarget ?? '', filePath); + return; + } + + rmSync(filePath, { recursive: true, force: true }); + mkdirSync(path.dirname(filePath), { recursive: true }); + if (node.type === 'file') { + writeFileSync(filePath, node.bytes ?? Buffer.alloc(0)); + chmodSync(filePath, node.mode); + return; + } + if (node.type === 'directory') { + mkdirSync(filePath, { recursive: true }); + for (const entry of node.entries ?? []) { + restoreNode(path.join(filePath, entry.name), entry.node, restoredTargets); + } + chmodSync(filePath, node.mode); + return; + } + throw new Error(`Cannot restore unsupported protected path type at ${filePath}`); +} + +function capturePath(requestedPath: string): ProtectedPathSnapshot { + const absolute = path.resolve(requestedPath); + const logical = nodeType(absolute); + const canonicalPath = canonicalizeProtectedPath(absolute); + return { + requestedPath: absolute, + canonicalPath, + logicalType: logical.type, + logicalMode: logical.mode, + logicalLinkTarget: logical.linkTarget, + node: captureNode(canonicalPath), + }; +} + +export class RepairProtectionSnapshot { + private constructor(private readonly paths: ProtectedPathSnapshot[]) {} + + static capture(protectedPaths: string[]): RepairProtectionSnapshot { + const uniquePaths = [...new Set(protectedPaths.map((filePath) => path.resolve(filePath)))]; + return new RepairProtectionSnapshot(uniquePaths.map(capturePath)); + } + + verifyAndRestore(): RepairScopeViolation[] { + const violations: RepairScopeViolation[] = []; + for (const expected of this.paths) { + const actual = capturePath(expected.requestedPath); + const reasons: string[] = []; + if (expected.logicalType !== actual.logicalType) { + reasons.push(`type changed from ${expected.logicalType} to ${actual.logicalType}`); + } + if (expected.logicalMode !== actual.logicalMode) { + reasons.push( + `mode changed from ${expected.logicalMode.toString(8)} to ${actual.logicalMode.toString(8)}` + ); + } + if (expected.logicalLinkTarget !== actual.logicalLinkTarget) { + reasons.push('symlink target changed'); + } + if (expected.canonicalPath !== actual.canonicalPath) { + reasons.push(`canonical target changed from ${expected.canonicalPath} to ${actual.canonicalPath}`); + } + if (expected.node.sha256 !== actual.node.sha256) reasons.push('SHA-256 changed'); + if (reasons.length === 0) continue; + + const violation: RepairScopeViolation = { + path: expected.requestedPath, + reason: reasons.join('; '), + expectedType: expected.logicalType, + actualType: actual.logicalType, + expectedSha256: expected.node.sha256, + actualSha256: actual.node.sha256, + expectedCanonicalPath: expected.canonicalPath, + actualCanonicalPath: actual.canonicalPath, + }; + try { + restoreNode(expected.canonicalPath, expected.node); + if (expected.logicalType === 'absent') { + rmSync(expected.requestedPath, { recursive: true, force: true }); + } else if (expected.logicalType === 'symlink') { + rmSync(expected.requestedPath, { recursive: true, force: true }); + mkdirSync(path.dirname(expected.requestedPath), { recursive: true }); + symlinkSync(expected.logicalLinkTarget ?? '', expected.requestedPath); + } else if ( + actual.logicalType === 'symlink' || + expected.canonicalPath !== actual.canonicalPath + ) { + restoreNode(expected.requestedPath, expected.node); + } + } catch (error) { + violation.restoreError = error instanceof Error ? error.message : String(error); + } + violations.push(violation); + } + return violations; + } +} diff --git a/packages/core/src/runner.ts b/packages/core/src/runner.ts index 7344a0d..a91e0d8 100644 --- a/packages/core/src/runner.ts +++ b/packages/core/src/runner.ts @@ -64,6 +64,10 @@ import { ensureRelayfileMount, type MountHandle } from '@relayfile/sdk/workspace import { collectCliSession, type CliSessionReport } from './cli-session-collector.js'; import { executeApiStep } from './api-executor.js'; import { BudgetExceededError, BudgetTracker } from './budget-tracker.js'; +import { + RepairProtectionSnapshot, + RepairScopeViolationError, +} from './repair-protection.js'; import { ChannelMessenger, formatObserverGuidance, @@ -557,6 +561,12 @@ interface DeterministicRepairContext { exitSignal?: string; } +interface RepairProtectionPlan { + protectedPaths: string[]; + unresolvedLocalGateCode: string[]; + explicitlyProtected: boolean; +} + interface AgentStepRepairContext { step: WorkflowStep; agentDef: AgentDefinition; @@ -878,6 +888,8 @@ export class WorkflowRunner { private readonly relayOptions: RuntimeSpawnOptions; private readonly cwd: string; private workflowFileDir?: string; + /** Absolute path of the loaded YAML, when configuration came from disk. */ + private workflowFilePath?: string; private cwdResolution: NonNullable = 'process'; private readonly summaryDir: string; private executor?: RunnerStepExecutor; @@ -3068,6 +3080,7 @@ export class WorkflowRunner { const absPath = path.resolve(this.cwd, filePath); const raw = await readFile(absPath, 'utf-8'); this.workflowFileDir = path.dirname(absPath); + this.workflowFilePath = absPath; return this.parseYamlString(raw, absPath); } @@ -3075,6 +3088,10 @@ export class WorkflowRunner { parseYamlString(raw: string, source = ''): RelayYamlConfig { if (source !== '' && path.isAbsolute(source)) { this.workflowFileDir = path.dirname(source); + this.workflowFilePath = source; + } else if (source === '') { + this.workflowFileDir = undefined; + this.workflowFilePath = undefined; } const parsed = parseYaml(raw); this.validateConfig(parsed, source); @@ -3641,6 +3658,11 @@ export class WorkflowRunner { `${source}: terminalSuccessExitCodes is only valid on deterministic steps ("${s.name}")` ); } + if (s.repairProtection !== undefined && s.type !== 'deterministic') { + throw new Error( + `${source}: repairProtection is only valid on deterministic steps ("${s.name}")` + ); + } // Deterministic steps require type and command if (s.type === 'deterministic') { @@ -3664,6 +3686,23 @@ export class WorkflowRunner { ); } } + if (s.repairProtection !== undefined) { + const protection = s.repairProtection; + if ( + typeof protection !== 'object' || + protection === null || + Array.isArray(protection) || + !Array.isArray((protection as Record).protectedPaths) || + ((protection as Record).protectedPaths as unknown[]).length === 0 || + ((protection as Record).protectedPaths as unknown[]).some( + (entry) => typeof entry !== 'string' || entry.trim().length === 0 + ) + ) { + throw new Error( + `${source}: deterministic step "${s.name}" repairProtection.protectedPaths must be a non-empty array of non-empty strings` + ); + } + } } else if (s.type === 'worktree') { if (typeof s.branch !== 'string' || s.branch.trim().length === 0) { throw new Error(`${source}: worktree step "${s.name}" must have a "branch" string field`); @@ -4840,6 +4879,7 @@ export class WorkflowRunner { const verificationResult = step.verification ? this.runVerification(step.verification, output, step.name, undefined, { exitCode: executorResult.exitCode, + cwd: stepCwd, }) : undefined; return { @@ -4956,6 +4996,7 @@ export class WorkflowRunner { const verificationResult = step.verification ? this.runVerification(step.verification, output, step.name, undefined, { exitCode: lastExitCode, + cwd: stepCwd, }) : undefined; lastCommandOutput = [commandStdout || output, commandStderr].filter(Boolean).join('\n'); @@ -5067,6 +5108,160 @@ export class WorkflowRunner { return score; } + private tokenizeShellCommand(command: string): string[][] { + const commands: string[][] = []; + let currentCommand: string[] = []; + let currentToken = ''; + let quote: "'" | '"' | undefined; + let escaped = false; + const flushToken = () => { + if (currentToken.length > 0) currentCommand.push(currentToken); + currentToken = ''; + }; + const flushCommand = () => { + flushToken(); + if (currentCommand.length > 0) commands.push(currentCommand); + currentCommand = []; + }; + + for (const char of command) { + if (escaped) { + currentToken += char; + escaped = false; + continue; + } + if (char === '\\' && quote !== "'") { + escaped = true; + continue; + } + if (quote) { + if (char === quote) quote = undefined; + else currentToken += char; + continue; + } + if (char === "'" || char === '"') { + quote = char; + continue; + } + if (/\s/.test(char)) { + if (char === '\n') flushCommand(); + else flushToken(); + continue; + } + if (char === ';' || char === '|' || char === '&') { + flushCommand(); + continue; + } + currentToken += char; + } + flushCommand(); + return commands; + } + + private resolveProtectedPath(rawPath: string, cwd: string): string { + const expanded = WorkflowRunner.resolveEnvVars(rawPath); + const tildeExpanded = expanded === '~' ? homedir() : expanded.startsWith('~/') ? path.join(homedir(), expanded.slice(2)) : expanded; + return path.resolve(cwd, tildeExpanded); + } + + private findDirectLocalScripts( + command: string, + cwd: string, + source: 'gate' | 'verification' + ): { paths: string[]; unresolved: string[] } { + const paths: string[] = []; + const unresolved: string[] = []; + const interpreters = new Set([ + 'sh', + 'bash', + 'dash', + 'zsh', + 'node', + 'python', + 'python3', + 'bun', + 'deno', + 'tsx', + 'ts-node', + ]); + const packageRunners = new Set(['npm', 'npx', 'pnpm', 'yarn']); + const scriptExtension = /\.(?:[cm]?js|ts|tsx|py|sh|bash|zsh)$/i; + + for (const originalTokens of this.tokenizeShellCommand(command)) { + const tokens = [...originalTokens]; + while (tokens[0] && /^[A-Za-z_][A-Za-z0-9_]*=/.test(tokens[0])) tokens.shift(); + if (tokens[0] === 'env') { + tokens.shift(); + while (tokens[0] && (tokens[0].startsWith('-') || /^[A-Za-z_][A-Za-z0-9_]*=/.test(tokens[0]))) { + tokens.shift(); + } + } + if (tokens[0] === 'sudo') tokens.shift(); + const executable = tokens[0]; + if (!executable) continue; + const executableName = path.basename(executable).toLowerCase(); + let candidate: string | undefined; + + if (interpreters.has(executableName)) { + const args = tokens.slice(1); + for (let index = 0; index < args.length; index += 1) { + const arg = args[index]; + if (['-c', '-e', '--eval', '-p', '--print', '-m'].includes(arg)) { + candidate = undefined; + break; + } + if (['-r', '--require', '--loader', '--import'].includes(arg)) { + index += 1; + continue; + } + if (arg === '--') { + candidate = args[index + 1]; + break; + } + if (!arg.startsWith('-')) { + candidate = arg; + break; + } + } + } else if ( + executable.startsWith('.') || + executable.startsWith('/') || + executable.includes('/') || + scriptExtension.test(executable) + ) { + candidate = executable; + } else if (packageRunners.has(executableName)) { + unresolved.push(`${source} command uses indirect package runner: ${tokens.join(' ')}`); + } + + if (!candidate) continue; + const resolved = this.resolveProtectedPath(candidate, cwd); + if (existsSync(resolved)) paths.push(resolved); + else unresolved.push(`${source} script could not be resolved: ${candidate}`); + } + return { paths, unresolved }; + } + + private resolveRepairProtection(context: DeterministicRepairContext): RepairProtectionPlan { + const explicit = context.step.repairProtection?.protectedPaths ?? []; + const protectedPaths = explicit.map((entry) => this.resolveProtectedPath(entry, context.cwd)); + if (this.workflowFilePath) protectedPaths.push(this.workflowFilePath); + + const gateScripts = this.findDirectLocalScripts(context.command, context.cwd, 'gate'); + protectedPaths.push(...gateScripts.paths); + const verificationScripts = + context.step.verification?.type === 'custom' + ? this.findDirectLocalScripts(context.step.verification.value, context.cwd, 'verification') + : { paths: [], unresolved: [] }; + protectedPaths.push(...verificationScripts.paths); + + return { + protectedPaths: [...new Set(protectedPaths.map((entry) => path.resolve(entry)))], + unresolvedLocalGateCode: [...gateScripts.unresolved, ...verificationScripts.unresolved], + explicitlyProtected: explicit.length > 0, + }; + } + private async runDeterministicRepairAgent(context: DeterministicRepairContext): Promise { if (!context.agentDef.cli) { throw new Error(`Repair agent "${context.agentDef.name}" must be a raw CLI agent`); @@ -5076,7 +5271,23 @@ export class WorkflowRunner { cli: context.agentDef.cli, interactive: false, }; - const repairPrompt = this.buildDeterministicRepairPrompt(context); + const protection = this.resolveRepairProtection(context); + if (protection.unresolvedLocalGateCode.length > 0 && !protection.explicitlyProtected) { + const detail = + `[${context.step.name}] REPAIR PROTECTION WARNING: repair agent skipped because local gate code ` + + `could not be resolved safely (${protection.unresolvedLocalGateCode.join('; ')}). ` + + `The deterministic gate will be rerun without repair. Configure repairProtection.protectedPaths to proceed.`; + this.log(detail); + this.postToChannel(`**[${context.step.name}] REPAIR PROTECTION WARNING:** ${detail}`); + this.recordStepToolSideEffect(context.step.name, { + type: 'custom', + detail, + raw: { unresolvedLocalGateCode: protection.unresolvedLocalGateCode }, + }); + return; + } + + const repairPrompt = this.buildDeterministicRepairPrompt(context, protection.protectedPaths); const repairStep: WorkflowStep = { name: `${context.step.name}-repair-${context.attempt}`, type: 'agent', @@ -5107,9 +5318,11 @@ export class WorkflowRunner { }, }); + const snapshot = RepairProtectionSnapshot.capture(protection.protectedPaths); + let repairOutput: string | undefined; + let repairError: unknown; try { this.ensureBudgetAllowsSpawn(context.step.name, repairAgent.name); - let repairOutput: string; if (this.executor) { repairOutput = await this.executor.executeAgentStep(repairStep, repairAgent, repairPrompt, timeoutMs); } else if (repairAgent.cli === 'api') { @@ -5126,30 +5339,60 @@ export class WorkflowRunner { const result = await this.execNonInteractive(repairAgent, repairStep, timeoutMs); repairOutput = result.output; } - - this.recordStepToolSideEffect(context.step.name, { - type: 'custom', - detail: `Repair agent ${repairAgent.name} completed before deterministic retry`, - raw: { repairAgent: repairAgent.name, output: repairOutput.slice(0, 1000) }, - }); } catch (error) { - if (error instanceof BudgetExceededError || this.abortController?.signal.aborted) { - throw error; + repairError = error; + } finally { + const violations = snapshot.verifyAndRestore(); + if (violations.length > 0) { + const evidence = violations + .map( + (violation) => + `${violation.path}: ${violation.reason}; expected sha256=${violation.expectedSha256}, ` + + `actual sha256=${violation.actualSha256}; ` + + (violation.restoreError ? `restore FAILED: ${violation.restoreError}` : 'snapshot restored') + ) + .join('\n'); + this.log(`[${context.step.name}] Repair scope violation; terminating run FAILED:\n${evidence}`); + this.postToChannel( + `**[${context.step.name}] REPAIR SCOPE VIOLATION — RUN FAILED**\n\`\`\`\n${evidence}\n\`\`\`` + ); + this.recordStepToolSideEffect(context.step.name, { + type: 'custom', + detail: 'Repair scope violation; protected snapshot restored and run terminated', + raw: { violations }, + }); + throw new RepairScopeViolationError(violations); } - const message = error instanceof Error ? error.message : String(error); - this.log(`[${context.step.name}] Repair agent "${repairAgent.name}" failed: ${message}`); - this.postToChannel( - `**[${context.step.name}]** Repair agent \`${repairAgent.name}\` failed; retrying gate anyway` - ); + } + + if (repairError === undefined) { this.recordStepToolSideEffect(context.step.name, { type: 'custom', - detail: `Repair agent ${repairAgent.name} failed before deterministic retry: ${message}`, - raw: { repairAgent: repairAgent.name, error: message }, + detail: `Repair agent ${repairAgent.name} completed before deterministic retry`, + raw: { repairAgent: repairAgent.name, output: (repairOutput ?? '').slice(0, 1000) }, }); + return; } + + if (repairError instanceof BudgetExceededError || this.abortController?.signal.aborted) { + throw repairError; + } + const message = repairError instanceof Error ? repairError.message : String(repairError); + this.log(`[${context.step.name}] Repair agent "${repairAgent.name}" failed: ${message}`); + this.postToChannel( + `**[${context.step.name}]** Repair agent \`${repairAgent.name}\` failed; retrying gate anyway` + ); + this.recordStepToolSideEffect(context.step.name, { + type: 'custom', + detail: `Repair agent ${repairAgent.name} failed before deterministic retry: ${message}`, + raw: { repairAgent: repairAgent.name, error: message }, + }); } - private buildDeterministicRepairPrompt(context: DeterministicRepairContext): string { + private buildDeterministicRepairPrompt( + context: DeterministicRepairContext, + protectedPaths: string[] = [] + ): string { const output = context.output.trim(); const clippedOutput = output.length > 4000 ? output.slice(-4000) : output; return ( @@ -5161,6 +5404,10 @@ export class WorkflowRunner { `Exit code: ${context.exitCode ?? 'unknown'}\n` + `Exit signal: ${context.exitSignal ?? 'none'}\n\n` + `Command output:\n${clippedOutput || '(no output captured)'}\n\n` + + (protectedPaths.length > 0 + ? `Protected gate files (enforced by snapshot; do not edit, delete, replace, chmod, or execute them):\n` + + `${protectedPaths.map((entry) => `- ${entry}`).join('\n')}\n\n` + : '') + `Repair only what is needed for this gate to pass. Preserve unrelated user changes. ` + `After making the fix, report the files changed and the reason the gate should pass.` ); @@ -10903,7 +11150,7 @@ export class WorkflowRunner { output, stepName, injectedTaskText, - { ...options, cwd: this.cwd }, + { ...options, cwd: options?.cwd ?? this.cwd }, { recordStepToolSideEffect: (name, effect) => this.recordStepToolSideEffect(name, effect), getOrCreateStepEvidenceRecord: (name) => this.getOrCreateStepEvidenceRecord(name), diff --git a/packages/core/src/schema.json b/packages/core/src/schema.json index 844a0a8..b492a17 100644 --- a/packages/core/src/schema.json +++ b/packages/core/src/schema.json @@ -910,6 +910,19 @@ }, "description": "Explicit exit codes that end the workflow with completed_early and skip remaining work" }, + "repairProtection": { + "type": "object", + "additionalProperties": false, + "required": ["protectedPaths"], + "properties": { + "protectedPaths": { + "type": "array", + "minItems": 1, + "items": { "type": "string", "minLength": 1 }, + "description": "Files a repair agent must not change, resolved relative to the deterministic step cwd" + } + } + }, "workdir": { "type": "string", "description": "Sets this step's working directory to a named entry from the top-level paths array." @@ -1191,6 +1204,19 @@ }, "description": "Explicit exit codes that end the workflow with completed_early and skip remaining work" }, + "repairProtection": { + "type": "object", + "additionalProperties": false, + "required": ["protectedPaths"], + "properties": { + "protectedPaths": { + "type": "array", + "minItems": 1, + "items": { "type": "string", "minLength": 1 }, + "description": "Files a repair agent must not change, resolved relative to the deterministic step cwd" + } + } + }, "timeoutMs": { "type": "integer", "minimum": 0, diff --git a/packages/core/src/schema.ts b/packages/core/src/schema.ts index d3ea554..54d4481 100644 --- a/packages/core/src/schema.ts +++ b/packages/core/src/schema.ts @@ -368,6 +368,12 @@ export interface HumanAssistanceConfig { file?: FileHumanAssistanceConfig; } +/** Files a deterministic repair agent must never change. */ +export interface RepairProtectionConfig { + /** Paths resolved relative to the deterministic step's effective cwd. */ + protectedPaths: string[]; +} + /** * A single step within a workflow. * @@ -424,6 +430,8 @@ export interface WorkflowStep { * remaining work. The run is reported as completed_early, not completed. */ terminalSuccessExitCodes?: number[]; + /** Enforce an immutable scope around every repair attempt for this gate. */ + repairProtection?: RepairProtectionConfig; // ── Integration step fields ──────────────────────────────────────────────── /** Integration name: 'github', 'linear', 'slack' (required for integration steps). */ diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index b944d13..e92b5d1 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -341,6 +341,8 @@ export interface CustomStepDefinition { captureOutput?: boolean; /** Exit codes that end the workflow with the distinct completed_early status. */ terminalSuccessExitCodes?: number[]; + /** Files a deterministic repair agent must never change. */ + repairProtection?: import('./schema.js').RepairProtectionConfig; /** Timeout in milliseconds. */ timeoutMs?: number; /** Human-readable description of this step. */ From d0afb7ac4c19c9f60f66d3346476a4a1535de063 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Wed, 26 Aug 2026 14:46:04 +0200 Subject: [PATCH 2/5] fix: close symlink-cycle, parent-swap, eval-mode, and custom-step gaps in the repair guard Four review findings on the repair-scope guard, each with a regression test: - a planted symlink cycle (ELOOP) now records a violation and restores instead of throwing past the restore path - restoration rebuilds a real parent chain so a parent directory swapped for a symlink cannot redirect the restore to an external target - interpreter eval modes resolve their nested command (bash -c scripts become protected paths) and fail closed when inline code references a local script - custom-step resolution preserves the invoking step's cwd/workdir so repairProtection paths resolve against the gate's actual directory Co-Authored-By: Claude Opus 4.7 --- .../src/__tests__/repair-scope-guard.test.ts | 99 ++++++++++++++++++- .../src/__tests__/yaml-validation.test.ts | 14 +++ packages/core/src/custom-steps.ts | 4 + packages/core/src/repair-protection.ts | 51 ++++++++-- packages/core/src/runner.ts | 36 +++++++ 5 files changed, 197 insertions(+), 7 deletions(-) diff --git a/packages/core/src/__tests__/repair-scope-guard.test.ts b/packages/core/src/__tests__/repair-scope-guard.test.ts index 2c64fd4..7f35400 100644 --- a/packages/core/src/__tests__/repair-scope-guard.test.ts +++ b/packages/core/src/__tests__/repair-scope-guard.test.ts @@ -1,4 +1,13 @@ -import { mkdtempSync, readFileSync, rmSync, symlinkSync, unlinkSync, writeFileSync } from 'node:fs'; +import { + lstatSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + symlinkSync, + unlinkSync, + writeFileSync, +} from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -205,6 +214,94 @@ describe('deterministic repair scope guard', () => { expect(readFileSync(verificationPath, 'utf8')).toBe('#!/bin/sh\nexit 1\n'); }); + it('treats a planted symlink cycle as a violation and restores the file', async () => { + const protectedPath = path.join(cwd, 'gate.js'); + writeFileSync(protectedPath, 'original\n'); + const runner = new WorkflowRunner({ + cwd, + executor: { + executeAgentStep: vi.fn(async () => { + unlinkSync(protectedPath); + symlinkSync(protectedPath, protectedPath); + return 'planted cycle'; + }), + }, + }); + + await expect( + (runner as any).runDeterministicRepairAgent( + repairContext(cwd, { protectedPaths: ['gate.js'] }) + ) + ).rejects.toBeInstanceOf(RepairScopeViolationError); + expect(readFileSync(protectedPath, 'utf8')).toBe('original\n'); + }); + + it('restores through a real parent chain when a parent directory is swapped for a symlink', async () => { + const subDir = path.join(cwd, 'sub'); + mkdirSync(subDir); + const protectedPath = path.join(subDir, 'gate.js'); + writeFileSync(protectedPath, 'original\n'); + const outsideDir = path.join(cwd, 'outside'); + mkdirSync(outsideDir); + const outsideFile = path.join(outsideDir, 'gate.js'); + writeFileSync(outsideFile, 'external\n'); + const runner = new WorkflowRunner({ + cwd, + executor: { + executeAgentStep: vi.fn(async () => { + rmSync(subDir, { recursive: true, force: true }); + symlinkSync(outsideDir, subDir); + return 'swapped parent for symlink'; + }), + }, + }); + + await expect( + (runner as any).runDeterministicRepairAgent( + repairContext(cwd, { protectedPaths: ['sub/gate.js'] }) + ) + ).rejects.toBeInstanceOf(RepairScopeViolationError); + expect(lstatSync(subDir).isDirectory()).toBe(true); + expect(lstatSync(subDir).isSymbolicLink()).toBe(false); + expect(readFileSync(protectedPath, 'utf8')).toBe('original\n'); + expect(readFileSync(outsideFile, 'utf8')).toBe('external\n'); + }); + + it('protects a script invoked through bash -c by resolving the nested command', async () => { + const gateScript = path.join(cwd, 'gate.sh'); + writeFileSync(gateScript, 'exit 1\n', { mode: 0o755 }); + const runner = new WorkflowRunner({ + cwd, + executor: { + executeAgentStep: vi.fn(async () => { + writeFileSync(gateScript, 'exit 0\n'); + return 'edited nested gate'; + }), + }, + }); + + await expect( + (runner as any).runDeterministicRepairAgent( + repairContext(cwd, { command: "bash -c './gate.sh'" }) + ) + ).rejects.toBeInstanceOf(RepairScopeViolationError); + expect(readFileSync(gateScript, 'utf8')).toBe('exit 1\n'); + }); + + it('skips repair when inline eval references a local script it cannot inspect', async () => { + writeFileSync(path.join(cwd, 'helper.js'), 'module.exports = 1;\n'); + const executeAgentStep = vi.fn(async () => 'unexpected repair'); + const runner = new WorkflowRunner({ cwd, executor: { executeAgentStep } }); + const log = vi.spyOn(runner as any, 'log'); + + await (runner as any).runDeterministicRepairAgent( + repairContext(cwd, { command: 'node -e "require(\'./helper.js\')"' }) + ); + + expect(executeAgentStep).not.toHaveBeenCalled(); + expect(log).toHaveBeenCalledWith(expect.stringContaining('REPAIR PROTECTION WARNING')); + }); + it('skips repair loudly when indirect gate code is unresolvable and no explicit protection exists', async () => { const executeAgentStep = vi.fn(async () => 'unexpected repair'); const runner = new WorkflowRunner({ cwd, executor: { executeAgentStep } }); diff --git a/packages/core/src/__tests__/yaml-validation.test.ts b/packages/core/src/__tests__/yaml-validation.test.ts index a602c89..cfd5811 100644 --- a/packages/core/src/__tests__/yaml-validation.test.ts +++ b/packages/core/src/__tests__/yaml-validation.test.ts @@ -601,6 +601,20 @@ describe('Custom Step Resolution', () => { expect(resolved.terminalSuccessExitCodes).toEqual([78]); }); + it('preserves the invoking step cwd and workdir through custom-step resolution', () => { + const step = { + name: 'build', + use: 'docker-build', + image: 'myapp:latest', + cwd: 'services/api', + workdir: 'api-checkout', + } as WorkflowStep; + const resolved = resolveCustomStep(step, customSteps); + + expect(resolved.cwd).toBe('services/api'); + expect(resolved.workdir).toBe('api-checkout'); + }); + it('should resolve custom step with all params', () => { const step = { name: 'build', diff --git a/packages/core/src/custom-steps.ts b/packages/core/src/custom-steps.ts index c80f115..475f69b 100644 --- a/packages/core/src/custom-steps.ts +++ b/packages/core/src/custom-steps.ts @@ -455,6 +455,10 @@ export function resolveCustomStep( type: stepType as 'deterministic' | 'worktree', dependsOn: step.dependsOn, timeoutMs: step.timeoutMs ?? customDef.timeoutMs, + // The invoking step's effective directory must survive resolution: + // repairProtection paths and the command itself resolve against it. + cwd: step.cwd, + workdir: step.workdir, }; if (stepType === 'deterministic') { diff --git a/packages/core/src/repair-protection.ts b/packages/core/src/repair-protection.ts index 346c5c9..4f51ebf 100644 --- a/packages/core/src/repair-protection.ts +++ b/packages/core/src/repair-protection.ts @@ -58,6 +58,12 @@ interface ProtectedPathSnapshot { } const modeBits = (mode: number): number => mode & 0o7777; + +// Codes that mean "this path cannot be resolved as it stands" rather than a +// real I/O failure. A repair that plants a symlink cycle (ELOOP) or replaces a +// parent directory with a file (ENOTDIR) must surface as a violation to +// restore, not as a thrown error that skips the restore path entirely. +const UNRESOLVABLE_PATH_CODES = new Set(['ENOENT', 'ELOOP', 'ENOTDIR', 'ENAMETOOLONG']); const digest = (chunks: Array): string => { const hash = createHash('sha256'); for (const chunk of chunks) hash.update(chunk); @@ -74,18 +80,48 @@ function nodeType(filePath: string): { type: ProtectedNodeType; mode: number; li if (info.isDirectory()) return { type: 'directory', mode: modeBits(info.mode) }; return { type: 'other', mode: modeBits(info.mode) }; } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') return { type: 'absent', mode: 0 }; + if (UNRESOLVABLE_PATH_CODES.has((error as NodeJS.ErrnoException).code ?? '')) { + return { type: 'absent', mode: 0 }; + } throw error; } } +/** Make every ancestor of `filePath` a real directory, removing any symlink or + * non-directory a repair may have planted, so restore/removal operations act + * on the canonical location instead of following a mutable parent elsewhere. */ +function ensureRealParentChain(filePath: string): void { + const parentPath = path.dirname(filePath); + if (parentPath === filePath) return; + const root = path.parse(parentPath).root; + const parts = parentPath.slice(root.length).split(path.sep).filter(Boolean); + let cursor = root; + for (const part of parts) { + cursor = path.join(cursor, part); + let info: ReturnType | undefined; + try { + info = lstatSync(cursor); + } catch (error) { + if (!UNRESOLVABLE_PATH_CODES.has((error as NodeJS.ErrnoException).code ?? '')) throw error; + } + if (!info) { + mkdirSync(cursor); + continue; + } + if (info.isSymbolicLink() || !info.isDirectory()) { + rmSync(cursor, { recursive: true, force: true }); + mkdirSync(cursor); + } + } +} + /** Resolve every existing symlink component, including for a path that is currently absent. */ export function canonicalizeProtectedPath(filePath: string): string { const absolute = path.resolve(filePath); try { return realpathSync.native(absolute); } catch (error) { - if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; + if (!UNRESOLVABLE_PATH_CODES.has((error as NodeJS.ErrnoException).code ?? '')) throw error; } const missing: string[] = []; @@ -98,7 +134,7 @@ export function canonicalizeProtectedPath(filePath: string): string { try { return path.join(realpathSync.native(cursor), ...missing); } catch (error) { - if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; + if (!UNRESOLVABLE_PATH_CODES.has((error as NodeJS.ErrnoException).code ?? '')) throw error; } } } @@ -164,6 +200,10 @@ function captureNode(filePath: string, ancestors = new Set()): Protected } function restoreNode(filePath: string, node: ProtectedNode, restoredTargets = new Set()): void { + // A repair may have swapped an ancestor directory for a symlink; every + // rm/write below would silently follow it to an external target otherwise. + ensureRealParentChain(filePath); + if (node.type === 'absent') { rmSync(filePath, { recursive: true, force: true }); return; @@ -178,13 +218,11 @@ function restoreNode(filePath: string, node: ProtectedNode, restoredTargets = ne restoreNode(node.canonicalPath, targetNode, restoredTargets); } rmSync(filePath, { recursive: true, force: true }); - mkdirSync(path.dirname(filePath), { recursive: true }); symlinkSync(node.linkTarget ?? '', filePath); return; } rmSync(filePath, { recursive: true, force: true }); - mkdirSync(path.dirname(filePath), { recursive: true }); if (node.type === 'file') { writeFileSync(filePath, node.bytes ?? Buffer.alloc(0)); chmodSync(filePath, node.mode); @@ -258,10 +296,11 @@ export class RepairProtectionSnapshot { try { restoreNode(expected.canonicalPath, expected.node); if (expected.logicalType === 'absent') { + ensureRealParentChain(expected.requestedPath); rmSync(expected.requestedPath, { recursive: true, force: true }); } else if (expected.logicalType === 'symlink') { + ensureRealParentChain(expected.requestedPath); rmSync(expected.requestedPath, { recursive: true, force: true }); - mkdirSync(path.dirname(expected.requestedPath), { recursive: true }); symlinkSync(expected.logicalLinkTarget ?? '', expected.requestedPath); } else if ( actual.logicalType === 'symlink' || diff --git a/packages/core/src/runner.ts b/packages/core/src/runner.ts index a91e0d8..aaf4687 100644 --- a/packages/core/src/runner.ts +++ b/packages/core/src/runner.ts @@ -5207,6 +5207,42 @@ export class WorkflowRunner { for (let index = 0; index < args.length; index += 1) { const arg = args[index]; if (['-c', '-e', '--eval', '-p', '--print', '-m'].includes(arg)) { + // Inline eval / module mode. The inline code itself lives in the + // workflow YAML, which is always protected — but any LOCAL SCRIPT + // it invokes is gate code we cannot see. Resolve shell payloads + // recursively; for other payloads, fail closed on any reference + // to an existing local script. + const payload = args[index + 1]; + const shellEval = + ['sh', 'bash', 'dash', 'zsh'].includes(executableName) && arg === '-c'; + if (shellEval && payload) { + const nested = this.findDirectLocalScripts(payload, cwd, source); + paths.push(...nested.paths); + unresolved.push(...nested.unresolved); + } else if (arg === '-m' && payload) { + const moduleFile = this.resolveProtectedPath(`${payload.replace(/\./g, '/')}.py`, cwd); + if (existsSync(moduleFile)) { + unresolved.push( + `${source} command runs local module ${payload}; nested code cannot be inspected: ${tokens.join(' ')}` + ); + } + } else if (payload) { + const referenced = + payload.match(/[A-Za-z0-9_./~-]+\.(?:[cm]?js|ts|tsx|py|sh|bash|zsh)\b/gi) ?? []; + for (const ref of referenced) { + const resolvedRef = this.resolveProtectedPath(ref, cwd); + if (existsSync(resolvedRef)) { + unresolved.push( + `${source} command evaluates inline code referencing local script ${ref}; it cannot be inspected: ${tokens.join(' ')}` + ); + break; + } + } + } else { + unresolved.push( + `${source} command uses interpreter ${arg} mode with no inspectable payload: ${tokens.join(' ')}` + ); + } candidate = undefined; break; } From 94be27b49d6ba7d6768f3e6a3d385fb88e2d6935 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Wed, 26 Aug 2026 15:01:09 +0200 Subject: [PATCH 3/5] fix: close round-two review gaps in the repair guard - an absent protected path masked by a parent directory replaced with a file is now a violation: snapshots track parent-chain traversability and restore rebuilds the real chain - nested shell parsing models cd (directory state), source/. (protects the sourced script), exec (recurses), and eval (fail closed) - inline eval payloads probe extension-less import/require specifiers against local module layouts (x.py, pkg/__init__.py, pkg/__main__.py, x.js, index.js) - python -m probes pkg/__main__.py as well as pkg.py - custom-step validation no longer warns that supported cwd/workdir fields will be ignored Co-Authored-By: Claude Opus 4.7 --- .../src/__tests__/repair-scope-guard.test.ts | 75 +++++++++++++++++ .../src/__tests__/yaml-validation.test.ts | 16 ++++ packages/core/src/custom-steps.ts | 2 +- packages/core/src/repair-protection.ts | 36 ++++++++ packages/core/src/runner.ts | 84 +++++++++++++++++-- 5 files changed, 205 insertions(+), 8 deletions(-) diff --git a/packages/core/src/__tests__/repair-scope-guard.test.ts b/packages/core/src/__tests__/repair-scope-guard.test.ts index 7f35400..2353f4a 100644 --- a/packages/core/src/__tests__/repair-scope-guard.test.ts +++ b/packages/core/src/__tests__/repair-scope-guard.test.ts @@ -288,6 +288,81 @@ describe('deterministic repair scope guard', () => { expect(readFileSync(gateScript, 'utf8')).toBe('exit 1\n'); }); + it('detects a parent directory replaced by a file masking an absent protected path', async () => { + const subDir = path.join(cwd, 'sub'); + mkdirSync(subDir); + const runner = new WorkflowRunner({ + cwd, + executor: { + executeAgentStep: vi.fn(async () => { + rmSync(subDir, { recursive: true, force: true }); + writeFileSync(subDir, 'not a directory\n'); + return 'replaced parent with file'; + }), + }, + }); + + await expect( + (runner as any).runDeterministicRepairAgent( + repairContext(cwd, { protectedPaths: ['sub/output.txt'] }) + ) + ).rejects.toBeInstanceOf(RepairScopeViolationError); + expect(lstatSync(subDir).isDirectory()).toBe(true); + }); + + it('protects a script reached through cd and one loaded through source', async () => { + const subDir = path.join(cwd, 'sub'); + mkdirSync(subDir); + const cdGate = path.join(subDir, 'gate.js'); + writeFileSync(cdGate, 'process.exit(1)\n'); + writeFileSync(path.join(cwd, 'env.sh'), 'exit 1\n'); + const runner = new WorkflowRunner({ + cwd, + executor: { + executeAgentStep: vi.fn(async () => { + writeFileSync(cdGate, 'process.exit(0)\n'); + return 'edited gate behind cd'; + }), + }, + }); + + await expect( + (runner as any).runDeterministicRepairAgent( + repairContext(cwd, { command: 'source ./env.sh; cd sub && node gate.js' }) + ) + ).rejects.toBeInstanceOf(RepairScopeViolationError); + expect(readFileSync(cdGate, 'utf8')).toBe('process.exit(1)\n'); + }); + + it('skips repair when inline python imports a local module without an extension', async () => { + writeFileSync(path.join(cwd, 'helper.py'), 'VALUE = 1\n'); + const executeAgentStep = vi.fn(async () => 'unexpected repair'); + const runner = new WorkflowRunner({ cwd, executor: { executeAgentStep } }); + const log = vi.spyOn(runner as any, 'log'); + + await (runner as any).runDeterministicRepairAgent( + repairContext(cwd, { command: 'python3 -c "import helper"' }) + ); + + expect(executeAgentStep).not.toHaveBeenCalled(); + expect(log).toHaveBeenCalledWith(expect.stringContaining('REPAIR PROTECTION WARNING')); + }); + + it('skips repair when python -m targets a local package entrypoint', async () => { + mkdirSync(path.join(cwd, 'pkg')); + writeFileSync(path.join(cwd, 'pkg', '__main__.py'), 'raise SystemExit(1)\n'); + const executeAgentStep = vi.fn(async () => 'unexpected repair'); + const runner = new WorkflowRunner({ cwd, executor: { executeAgentStep } }); + const log = vi.spyOn(runner as any, 'log'); + + await (runner as any).runDeterministicRepairAgent( + repairContext(cwd, { command: 'python3 -m pkg' }) + ); + + expect(executeAgentStep).not.toHaveBeenCalled(); + expect(log).toHaveBeenCalledWith(expect.stringContaining('REPAIR PROTECTION WARNING')); + }); + it('skips repair when inline eval references a local script it cannot inspect', async () => { writeFileSync(path.join(cwd, 'helper.js'), 'module.exports = 1;\n'); const executeAgentStep = vi.fn(async () => 'unexpected repair'); diff --git a/packages/core/src/__tests__/yaml-validation.test.ts b/packages/core/src/__tests__/yaml-validation.test.ts index cfd5811..3dd3fcd 100644 --- a/packages/core/src/__tests__/yaml-validation.test.ts +++ b/packages/core/src/__tests__/yaml-validation.test.ts @@ -731,6 +731,22 @@ describe('Custom Step Validation', () => { expect(result.missingSteps).toContain('unknown-step'); }); + it('does not warn that supported step fields like cwd/workdir will be ignored', () => { + const steps: WorkflowStep[] = [ + { + name: 'build', + use: 'docker-build', + image: 'myapp:latest', + cwd: 'services/api', + workdir: 'api-checkout', + } as WorkflowStep, + ]; + + const result = validateCustomStepsUsage(steps, customSteps); + + expect(result.warnings.filter((w) => w.includes('cwd') || w.includes('workdir'))).toEqual([]); + }); + it('should report missing required parameters', () => { const steps: WorkflowStep[] = [ { name: 'build', use: 'docker-build' } as WorkflowStep, // missing 'image' diff --git a/packages/core/src/custom-steps.ts b/packages/core/src/custom-steps.ts index 475f69b..44a63b2 100644 --- a/packages/core/src/custom-steps.ts +++ b/packages/core/src/custom-steps.ts @@ -361,7 +361,7 @@ export function validateCustomStepsUsage( // Check for extra parameters that aren't defined const definedParams = new Set((customDef.params ?? []).map((p) => p.name)); const stepKeys = Object.keys(stepAny).filter( - (k) => !['name', 'use', 'dependsOn', 'timeoutMs'].includes(k) + (k) => !['name', 'use', 'dependsOn', 'timeoutMs', 'cwd', 'workdir'].includes(k) ); for (const key of stepKeys) { if (!definedParams.has(key)) { diff --git a/packages/core/src/repair-protection.ts b/packages/core/src/repair-protection.ts index 4f51ebf..85e9e4a 100644 --- a/packages/core/src/repair-protection.ts +++ b/packages/core/src/repair-protection.ts @@ -54,6 +54,9 @@ interface ProtectedPathSnapshot { logicalType: ProtectedNodeType; logicalMode: number; logicalLinkTarget?: string; + /** True when an existing ancestor component is not a traversable directory, + * i.e. the path reads as absent only because its parent chain is blocked. */ + parentChainBlocked: boolean; node: ProtectedNode; } @@ -87,6 +90,29 @@ function nodeType(filePath: string): { type: ProtectedNodeType; mode: number; li } } +/** Does some existing ancestor of `filePath` block traversal (a file or other + * non-directory where a directory component is required)? Distinguishes a + * genuinely absent path from one masked by a replaced parent. */ +function isParentChainBlocked(filePath: string): boolean { + let cursor = path.dirname(path.resolve(filePath)); + while (true) { + let info: ReturnType | undefined; + try { + info = lstatSync(cursor); + } catch (error) { + if (!UNRESOLVABLE_PATH_CODES.has((error as NodeJS.ErrnoException).code ?? '')) throw error; + } + if (info) { + // The nearest existing ancestor decides: directories traverse, and + // symlinks are judged by canonical-path comparison instead. + return !info.isDirectory() && !info.isSymbolicLink(); + } + const parent = path.dirname(cursor); + if (parent === cursor) return false; + cursor = parent; + } +} + /** Make every ancestor of `filePath` a real directory, removing any symlink or * non-directory a repair may have planted, so restore/removal operations act * on the canonical location instead of following a mutable parent elsewhere. */ @@ -249,6 +275,7 @@ function capturePath(requestedPath: string): ProtectedPathSnapshot { logicalType: logical.type, logicalMode: logical.mode, logicalLinkTarget: logical.linkTarget, + parentChainBlocked: isParentChainBlocked(absolute), node: captureNode(canonicalPath), }; } @@ -281,6 +308,15 @@ export class RepairProtectionSnapshot { reasons.push(`canonical target changed from ${expected.canonicalPath} to ${actual.canonicalPath}`); } if (expected.node.sha256 !== actual.node.sha256) reasons.push('SHA-256 changed'); + if (expected.parentChainBlocked !== actual.parentChainBlocked) { + // An "absent" reading can be an artifact of a parent directory being + // replaced by a file; the two absences must not compare equal. + reasons.push( + actual.parentChainBlocked + ? 'parent chain replaced by a non-directory' + : 'parent chain traversability changed' + ); + } if (reasons.length === 0) continue; const violation: RepairScopeViolation = { diff --git a/packages/core/src/runner.ts b/packages/core/src/runner.ts index aaf4687..eb26367 100644 --- a/packages/core/src/runner.ts +++ b/packages/core/src/runner.ts @@ -5187,6 +5187,7 @@ export class WorkflowRunner { const packageRunners = new Set(['npm', 'npx', 'pnpm', 'yarn']); const scriptExtension = /\.(?:[cm]?js|ts|tsx|py|sh|bash|zsh)$/i; + let effectiveCwd = cwd; for (const originalTokens of this.tokenizeShellCommand(command)) { const tokens = [...originalTokens]; while (tokens[0] && /^[A-Za-z_][A-Za-z0-9_]*=/.test(tokens[0])) tokens.shift(); @@ -5202,6 +5203,34 @@ export class WorkflowRunner { const executableName = path.basename(executable).toLowerCase(); let candidate: string | undefined; + // Shell constructs that change what later tokens resolve against, or + // execute local code without looking like a script invocation. + if (executableName === 'cd') { + if (tokens[1]) effectiveCwd = this.resolveProtectedPath(tokens[1], effectiveCwd); + continue; + } + if (executableName === 'source' || executableName === '.') { + // `source x.sh` executes x.sh in the current shell: protect it. + const sourced = tokens[1]; + if (sourced) { + const resolvedSource = this.resolveProtectedPath(sourced, effectiveCwd); + if (existsSync(resolvedSource)) paths.push(resolvedSource); + else unresolved.push(`${source} script could not be resolved: ${sourced}`); + } + continue; + } + if (executableName === 'exec') { + const nested = this.findDirectLocalScripts(tokens.slice(1).join(' '), effectiveCwd, source); + paths.push(...nested.paths); + unresolved.push(...nested.unresolved); + continue; + } else if (executableName === 'eval') { + unresolved.push( + `${source} command uses shell eval; nested code cannot be inspected: ${tokens.join(' ')}` + ); + continue; + } + if (interpreters.has(executableName)) { const args = tokens.slice(1); for (let index = 0; index < args.length; index += 1) { @@ -5216,22 +5245,63 @@ export class WorkflowRunner { const shellEval = ['sh', 'bash', 'dash', 'zsh'].includes(executableName) && arg === '-c'; if (shellEval && payload) { - const nested = this.findDirectLocalScripts(payload, cwd, source); + const nested = this.findDirectLocalScripts(payload, effectiveCwd, source); paths.push(...nested.paths); unresolved.push(...nested.unresolved); } else if (arg === '-m' && payload) { - const moduleFile = this.resolveProtectedPath(`${payload.replace(/\./g, '/')}.py`, cwd); - if (existsSync(moduleFile)) { + // `python -m pkg` executes pkg.py OR pkg/__main__.py. + const moduleBase = payload.replace(/\./g, '/'); + const moduleProbes = [`${moduleBase}.py`, `${moduleBase}/__main__.py`]; + if ( + moduleProbes.some((probe) => existsSync(this.resolveProtectedPath(probe, effectiveCwd))) + ) { unresolved.push( `${source} command runs local module ${payload}; nested code cannot be inspected: ${tokens.join(' ')}` ); } } else if (payload) { + // References to local code inside the inline payload: explicit + // script paths, plus extension-less import/require specifiers + // that resolve to a local module file. const referenced = payload.match(/[A-Za-z0-9_./~-]+\.(?:[cm]?js|ts|tsx|py|sh|bash|zsh)\b/gi) ?? []; - for (const ref of referenced) { - const resolvedRef = this.resolveProtectedPath(ref, cwd); - if (existsSync(resolvedRef)) { + const specifiers: string[] = []; + for (const m of payload.matchAll(/\b(?:import|from)\s+([A-Za-z_][\w.]*)/g)) { + specifiers.push(m[1]); + } + for (const m of payload.matchAll(/\b(?:require|import)\s*\(\s*['"]([^'"]+)['"]/g)) { + specifiers.push(m[1]); + } + for (const m of payload.matchAll(/\bfrom\s+['"]([^'"]+)['"]/g)) { + specifiers.push(m[1]); + } + const probes = new Set(referenced); + for (const spec of specifiers) { + const base = spec.replace(/\./g, '/'); + for (const suffix of [ + '', + '.py', + '/__init__.py', + '/__main__.py', + '.js', + '.mjs', + '.cjs', + '.ts', + '/index.js', + ]) { + probes.add(`${base}${suffix}`); + } + probes.add(spec); + } + for (const ref of probes) { + const resolvedRef = this.resolveProtectedPath(ref, effectiveCwd); + let refExists = false; + try { + refExists = existsSync(resolvedRef) && !statSync(resolvedRef).isDirectory(); + } catch { + refExists = false; + } + if (refExists) { unresolved.push( `${source} command evaluates inline code referencing local script ${ref}; it cannot be inspected: ${tokens.join(' ')}` ); @@ -5271,7 +5341,7 @@ export class WorkflowRunner { } if (!candidate) continue; - const resolved = this.resolveProtectedPath(candidate, cwd); + const resolved = this.resolveProtectedPath(candidate, effectiveCwd); if (existsSync(resolved)) paths.push(resolved); else unresolved.push(`${source} script could not be resolved: ${candidate}`); } From 3eebc230224fab19e6d0551fd0f836ac58dd134d Mon Sep 17 00:00:00 2001 From: kjgbot Date: Wed, 26 Aug 2026 15:16:05 +0200 Subject: [PATCH 4/5] fix: close round-three review gaps in gate-code resolution - parent-chain check resolves symlink ancestors: a dangling link or cycle planted over a parent now reads as blocked, not as genuine absence - the shell tokenizer carries separators; cd only threads the effective directory across proven-sequential positions (start, ;, &&, newline) and fails closed behind ||, |, or & - builtins match on the raw token, so a script literally named ./source is protected as a script - sourced files are protected AND inspected recursively (depth-capped, cycle-safe); a cd inside a sourced file marks later resolution unresolved - quoted side-effect imports are scanned; bare-identifier import matching is python-only so JS import bindings stop false-positiving; relative-path specifiers are probed without dotted-module mangling Co-Authored-By: Claude Opus 4.7 --- .../src/__tests__/repair-scope-guard.test.ts | 94 +++++++++++- packages/core/src/repair-protection.ts | 16 +- packages/core/src/runner.ts | 139 ++++++++++++++---- 3 files changed, 219 insertions(+), 30 deletions(-) diff --git a/packages/core/src/__tests__/repair-scope-guard.test.ts b/packages/core/src/__tests__/repair-scope-guard.test.ts index 2353f4a..4e46364 100644 --- a/packages/core/src/__tests__/repair-scope-guard.test.ts +++ b/packages/core/src/__tests__/repair-scope-guard.test.ts @@ -315,13 +315,15 @@ describe('deterministic repair scope guard', () => { mkdirSync(subDir); const cdGate = path.join(subDir, 'gate.js'); writeFileSync(cdGate, 'process.exit(1)\n'); - writeFileSync(path.join(cwd, 'env.sh'), 'exit 1\n'); + const sourcedEnv = path.join(cwd, 'env.sh'); + writeFileSync(sourcedEnv, 'export GATE_MODE=strict\n'); const runner = new WorkflowRunner({ cwd, executor: { executeAgentStep: vi.fn(async () => { writeFileSync(cdGate, 'process.exit(0)\n'); - return 'edited gate behind cd'; + writeFileSync(sourcedEnv, 'export GATE_MODE=lenient\n'); + return 'edited gate behind cd and sourced env'; }), }, }); @@ -332,6 +334,94 @@ describe('deterministic repair scope guard', () => { ) ).rejects.toBeInstanceOf(RepairScopeViolationError); expect(readFileSync(cdGate, 'utf8')).toBe('process.exit(1)\n'); + expect(readFileSync(sourcedEnv, 'utf8')).toBe('export GATE_MODE=strict\n'); + }); + + it('detects a parent directory swapped for a dangling symlink masking an absent path', async () => { + const subDir = path.join(cwd, 'sub'); + mkdirSync(subDir); + const runner = new WorkflowRunner({ + cwd, + executor: { + executeAgentStep: vi.fn(async () => { + rmSync(subDir, { recursive: true, force: true }); + symlinkSync(path.join(cwd, 'nowhere'), subDir); + return 'replaced parent with dangling symlink'; + }), + }, + }); + + await expect( + (runner as any).runDeterministicRepairAgent( + repairContext(cwd, { protectedPaths: ['sub/output.txt'] }) + ) + ).rejects.toBeInstanceOf(RepairScopeViolationError); + expect(lstatSync(subDir).isDirectory()).toBe(true); + }); + + it('protects a gate script literally named source', async () => { + const gateScript = path.join(cwd, 'source'); + writeFileSync(gateScript, 'exit 1\n', { mode: 0o755 }); + const runner = new WorkflowRunner({ + cwd, + executor: { + executeAgentStep: vi.fn(async () => { + writeFileSync(gateScript, 'exit 0\n'); + return 'edited script named source'; + }), + }, + }); + + await expect( + (runner as any).runDeterministicRepairAgent(repairContext(cwd, { command: './source' })) + ).rejects.toBeInstanceOf(RepairScopeViolationError); + expect(readFileSync(gateScript, 'utf8')).toBe('exit 1\n'); + }); + + it('fails closed when cd sits behind a non-sequential operator', async () => { + const subDir = path.join(cwd, 'sub'); + mkdirSync(subDir); + writeFileSync(path.join(subDir, 'gate.js'), 'process.exit(1)\n'); + writeFileSync(path.join(cwd, 'gate.js'), 'process.exit(1)\n'); + const executeAgentStep = vi.fn(async () => 'unexpected repair'); + const runner = new WorkflowRunner({ cwd, executor: { executeAgentStep } }); + const log = vi.spyOn(runner as any, 'log'); + + await (runner as any).runDeterministicRepairAgent( + repairContext(cwd, { command: 'true || cd sub; node gate.js' }) + ); + + expect(executeAgentStep).not.toHaveBeenCalled(); + expect(log).toHaveBeenCalledWith(expect.stringContaining('REPAIR PROTECTION WARNING')); + }); + + it('does not mistake a JS import binding for a local module reference', async () => { + writeFileSync(path.join(cwd, 'helper.js'), 'module.exports = 1;\n'); + const executeAgentStep = vi.fn(async () => { + writeFileSync(path.join(cwd, 'state.txt'), 'fixed\n'); + return 'repaired mutable state'; + }); + const runner = new WorkflowRunner({ cwd, executor: { executeAgentStep } }); + + await (runner as any).runDeterministicRepairAgent( + repairContext(cwd, { command: 'node -e "import helper from \'lodash\'; process.exit(1)"' }) + ); + + expect(executeAgentStep).toHaveBeenCalled(); + }); + + it('skips repair when a quoted side-effect import references a local extensionless module', async () => { + writeFileSync(path.join(cwd, 'helper.js'), 'module.exports = 1;\n'); + const executeAgentStep = vi.fn(async () => 'unexpected repair'); + const runner = new WorkflowRunner({ cwd, executor: { executeAgentStep } }); + const log = vi.spyOn(runner as any, 'log'); + + await (runner as any).runDeterministicRepairAgent( + repairContext(cwd, { command: 'node --input-type=module -e "import \'./helper\'"' }) + ); + + expect(executeAgentStep).not.toHaveBeenCalled(); + expect(log).toHaveBeenCalledWith(expect.stringContaining('REPAIR PROTECTION WARNING')); }); it('skips repair when inline python imports a local module without an extension', async () => { diff --git a/packages/core/src/repair-protection.ts b/packages/core/src/repair-protection.ts index 85e9e4a..73d666c 100644 --- a/packages/core/src/repair-protection.ts +++ b/packages/core/src/repair-protection.ts @@ -8,6 +8,7 @@ import { readdirSync, realpathSync, rmSync, + statSync, symlinkSync, writeFileSync, } from 'node:fs'; @@ -103,9 +104,18 @@ function isParentChainBlocked(filePath: string): boolean { if (!UNRESOLVABLE_PATH_CODES.has((error as NodeJS.ErrnoException).code ?? '')) throw error; } if (info) { - // The nearest existing ancestor decides: directories traverse, and - // symlinks are judged by canonical-path comparison instead. - return !info.isDirectory() && !info.isSymbolicLink(); + // The nearest existing ancestor decides. A symlink ancestor counts as + // blocked unless it resolves to a real directory — a dangling link or + // cycle would otherwise read identically to a genuinely absent path. + if (info.isSymbolicLink()) { + try { + return !statSync(cursor).isDirectory(); + } catch (error) { + if (UNRESOLVABLE_PATH_CODES.has((error as NodeJS.ErrnoException).code ?? '')) return true; + throw error; + } + } + return !info.isDirectory(); } const parent = path.dirname(cursor); if (parent === cursor) return false; diff --git a/packages/core/src/runner.ts b/packages/core/src/runner.ts index eb26367..243902d 100644 --- a/packages/core/src/runner.ts +++ b/packages/core/src/runner.ts @@ -880,6 +880,10 @@ function resolveWorkflowTokenSigningKey(env: NodeJS.ProcessEnv): LocalJwksSignin }; } +/** Separator preceding a tokenized shell command; only ';', '&&', newline, and + * 'start' prove sequential execution in the parent shell. */ +type ShellCommandSeparator = ';' | '&&' | '||' | '|' | '&' | 'start'; + // ── WorkflowRunner ────────────────────────────────────────────────────────── export class WorkflowRunner { @@ -5108,23 +5112,29 @@ export class WorkflowRunner { return score; } - private tokenizeShellCommand(command: string): string[][] { - const commands: string[][] = []; + private tokenizeShellCommand( + command: string + ): Array<{ tokens: string[]; sep: ShellCommandSeparator }> { + const commands: Array<{ tokens: string[]; sep: ShellCommandSeparator }> = []; let currentCommand: string[] = []; let currentToken = ''; let quote: "'" | '"' | undefined; let escaped = false; + // The separator that preceded the command currently being collected. + let pendingSep: ShellCommandSeparator = 'start'; const flushToken = () => { if (currentToken.length > 0) currentCommand.push(currentToken); currentToken = ''; }; - const flushCommand = () => { + const flushCommand = (nextSep: ShellCommandSeparator) => { flushToken(); - if (currentCommand.length > 0) commands.push(currentCommand); + if (currentCommand.length > 0) commands.push({ tokens: currentCommand, sep: pendingSep }); currentCommand = []; + pendingSep = nextSep; }; - for (const char of command) { + for (let i = 0; i < command.length; i += 1) { + const char = command[i]; if (escaped) { currentToken += char; escaped = false; @@ -5144,17 +5154,35 @@ export class WorkflowRunner { continue; } if (/\s/.test(char)) { - if (char === '\n') flushCommand(); + if (char === '\n') flushCommand(';'); else flushToken(); continue; } - if (char === ';' || char === '|' || char === '&') { - flushCommand(); + if (char === ';') { + flushCommand(';'); + continue; + } + if (char === '&') { + if (command[i + 1] === '&') { + flushCommand('&&'); + i += 1; + } else { + flushCommand('&'); + } + continue; + } + if (char === '|') { + if (command[i + 1] === '|') { + flushCommand('||'); + i += 1; + } else { + flushCommand('|'); + } continue; } currentToken += char; } - flushCommand(); + flushCommand('start'); return commands; } @@ -5167,7 +5195,9 @@ export class WorkflowRunner { private findDirectLocalScripts( command: string, cwd: string, - source: 'gate' | 'verification' + source: 'gate' | 'verification', + depth = 0, + visited: Set = new Set() ): { paths: string[]; unresolved: string[] } { const paths: string[] = []; const unresolved: string[] = []; @@ -5188,7 +5218,7 @@ export class WorkflowRunner { const scriptExtension = /\.(?:[cm]?js|ts|tsx|py|sh|bash|zsh)$/i; let effectiveCwd = cwd; - for (const originalTokens of this.tokenizeShellCommand(command)) { + for (const { tokens: originalTokens, sep } of this.tokenizeShellCommand(command)) { const tokens = [...originalTokens]; while (tokens[0] && /^[A-Za-z_][A-Za-z0-9_]*=/.test(tokens[0])) tokens.shift(); if (tokens[0] === 'env') { @@ -5204,27 +5234,71 @@ export class WorkflowRunner { let candidate: string | undefined; // Shell constructs that change what later tokens resolve against, or - // execute local code without looking like a script invocation. - if (executableName === 'cd') { + // execute local code without looking like a script invocation. Matched + // on the RAW token: a script literally named ./source is not a builtin. + if (executable === 'cd') { + // A cd behind ||, |, or & runs conditionally or in a subshell; the + // effective directory of later commands cannot be proven. + if (sep === '||' || sep === '|' || sep === '&') { + unresolved.push( + `${source} command changes directory in a non-sequential position; effective directory cannot be proven: ${tokens.join(' ')}` + ); + continue; + } if (tokens[1]) effectiveCwd = this.resolveProtectedPath(tokens[1], effectiveCwd); continue; } - if (executableName === 'source' || executableName === '.') { - // `source x.sh` executes x.sh in the current shell: protect it. + if (executable === 'source' || executable === '.') { + // `source x.sh` executes x.sh in the current shell: protect it and + // inspect its contents, since they run with the gate's authority. const sourced = tokens[1]; if (sourced) { const resolvedSource = this.resolveProtectedPath(sourced, effectiveCwd); - if (existsSync(resolvedSource)) paths.push(resolvedSource); - else unresolved.push(`${source} script could not be resolved: ${sourced}`); + if (existsSync(resolvedSource)) { + paths.push(resolvedSource); + if (depth < 3 && !visited.has(resolvedSource)) { + visited.add(resolvedSource); + try { + const contents = readFileSync(resolvedSource, 'utf8'); + const nested = this.findDirectLocalScripts( + contents, + effectiveCwd, + source, + depth + 1, + visited + ); + paths.push(...nested.paths); + unresolved.push(...nested.unresolved); + // A cd inside the sourced file changes the OUTER shell's + // directory; later commands in this gate cannot be resolved. + if (/(^|[\n;&|])\s*cd(\s|$)/.test(contents)) { + unresolved.push( + `${source} sourced script ${sourced} changes the working directory; later commands cannot be resolved` + ); + } + } catch { + unresolved.push(`${source} sourced script could not be read: ${sourced}`); + } + } + } else { + unresolved.push(`${source} script could not be resolved: ${sourced}`); + } } continue; } - if (executableName === 'exec') { - const nested = this.findDirectLocalScripts(tokens.slice(1).join(' '), effectiveCwd, source); + if (executable === 'exec') { + const nested = this.findDirectLocalScripts( + tokens.slice(1).join(' '), + effectiveCwd, + source, + depth + 1, + visited + ); paths.push(...nested.paths); unresolved.push(...nested.unresolved); continue; - } else if (executableName === 'eval') { + } + if (executable === 'eval') { unresolved.push( `${source} command uses shell eval; nested code cannot be inspected: ${tokens.join(' ')}` ); @@ -5245,7 +5319,13 @@ export class WorkflowRunner { const shellEval = ['sh', 'bash', 'dash', 'zsh'].includes(executableName) && arg === '-c'; if (shellEval && payload) { - const nested = this.findDirectLocalScripts(payload, effectiveCwd, source); + const nested = this.findDirectLocalScripts( + payload, + effectiveCwd, + source, + depth + 1, + visited + ); paths.push(...nested.paths); unresolved.push(...nested.unresolved); } else if (arg === '-m' && payload) { @@ -5266,18 +5346,27 @@ export class WorkflowRunner { const referenced = payload.match(/[A-Za-z0-9_./~-]+\.(?:[cm]?js|ts|tsx|py|sh|bash|zsh)\b/gi) ?? []; const specifiers: string[] = []; - for (const m of payload.matchAll(/\b(?:import|from)\s+([A-Za-z_][\w.]*)/g)) { - specifiers.push(m[1]); + // Bare-identifier import syntax is Python's; matching it in JS + // payloads would capture import BINDINGS (import x from 'pkg') + // and false-positive on same-named local files. + if (executableName.startsWith('python')) { + for (const m of payload.matchAll(/\b(?:import|from)\s+([A-Za-z_][\w.]*)/g)) { + specifiers.push(m[1]); + } } for (const m of payload.matchAll(/\b(?:require|import)\s*\(\s*['"]([^'"]+)['"]/g)) { specifiers.push(m[1]); } - for (const m of payload.matchAll(/\bfrom\s+['"]([^'"]+)['"]/g)) { + // Quoted sources: static side-effect imports and from-clauses. + for (const m of payload.matchAll(/\b(?:from|import)\s+['"]([^'"]+)['"]/g)) { specifiers.push(m[1]); } const probes = new Set(referenced); for (const spec of specifiers) { - const base = spec.replace(/\./g, '/'); + // Dotted segments only denote module packages in bare + // specifiers; './helper' must stay a relative path. + const isPathLike = spec.startsWith('.') || spec.startsWith('/') || spec.includes('/'); + const base = isPathLike ? spec : spec.replace(/\./g, '/'); for (const suffix of [ '', '.py', From 1368302400d61150927d2d49cdae8b2cf461dd3a Mon Sep 17 00:00:00 2001 From: kjgbot Date: Wed, 26 Aug 2026 15:25:45 +0200 Subject: [PATCH 5/5] fix: prove cd execution before threading it; inspect sourced chains fully - cd now threads the effective directory only from provably-executed positions (start, ;, newline); behind &&, ||, |, or & it fails closed - sourced-script inspection relies on the visited set alone, so arbitrarily deep source chains are inspected instead of silently cut off at depth 3 Co-Authored-By: Claude Opus 4.7 --- .../src/__tests__/repair-scope-guard.test.ts | 41 +++++++++++++++---- packages/core/src/runner.ts | 13 ++++-- 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/packages/core/src/__tests__/repair-scope-guard.test.ts b/packages/core/src/__tests__/repair-scope-guard.test.ts index 4e46364..31b49d4 100644 --- a/packages/core/src/__tests__/repair-scope-guard.test.ts +++ b/packages/core/src/__tests__/repair-scope-guard.test.ts @@ -383,16 +383,41 @@ describe('deterministic repair scope guard', () => { mkdirSync(subDir); writeFileSync(path.join(subDir, 'gate.js'), 'process.exit(1)\n'); writeFileSync(path.join(cwd, 'gate.js'), 'process.exit(1)\n'); - const executeAgentStep = vi.fn(async () => 'unexpected repair'); - const runner = new WorkflowRunner({ cwd, executor: { executeAgentStep } }); - const log = vi.spyOn(runner as any, 'log'); + for (const command of ['true || cd sub; node gate.js', 'false && cd sub; node gate.js']) { + const executeAgentStep = vi.fn(async () => 'unexpected repair'); + const runner = new WorkflowRunner({ cwd, executor: { executeAgentStep } }); + const log = vi.spyOn(runner as any, 'log'); - await (runner as any).runDeterministicRepairAgent( - repairContext(cwd, { command: 'true || cd sub; node gate.js' }) - ); + await (runner as any).runDeterministicRepairAgent(repairContext(cwd, { command })); - expect(executeAgentStep).not.toHaveBeenCalled(); - expect(log).toHaveBeenCalledWith(expect.stringContaining('REPAIR PROTECTION WARNING')); + expect(executeAgentStep).not.toHaveBeenCalled(); + expect(log).toHaveBeenCalledWith(expect.stringContaining('REPAIR PROTECTION WARNING')); + } + }); + + it('inspects sourced scripts beyond three levels of nesting', async () => { + const deepGate = path.join(cwd, 'deep-gate.js'); + writeFileSync(deepGate, 'process.exit(1)\n'); + writeFileSync(path.join(cwd, 'd.sh'), 'node deep-gate.js\n'); + writeFileSync(path.join(cwd, 'c.sh'), 'source ./d.sh\n'); + writeFileSync(path.join(cwd, 'b.sh'), 'source ./c.sh\n'); + writeFileSync(path.join(cwd, 'a.sh'), 'source ./b.sh\n'); + const runner = new WorkflowRunner({ + cwd, + executor: { + executeAgentStep: vi.fn(async () => { + writeFileSync(deepGate, 'process.exit(0)\n'); + return 'edited deeply sourced gate'; + }), + }, + }); + + await expect( + (runner as any).runDeterministicRepairAgent( + repairContext(cwd, { command: 'source ./a.sh' }) + ) + ).rejects.toBeInstanceOf(RepairScopeViolationError); + expect(readFileSync(deepGate, 'utf8')).toBe('process.exit(1)\n'); }); it('does not mistake a JS import binding for a local module reference', async () => { diff --git a/packages/core/src/runner.ts b/packages/core/src/runner.ts index 243902d..8e0f3be 100644 --- a/packages/core/src/runner.ts +++ b/packages/core/src/runner.ts @@ -5237,9 +5237,11 @@ export class WorkflowRunner { // execute local code without looking like a script invocation. Matched // on the RAW token: a script literally named ./source is not a builtin. if (executable === 'cd') { - // A cd behind ||, |, or & runs conditionally or in a subshell; the - // effective directory of later commands cannot be proven. - if (sep === '||' || sep === '|' || sep === '&') { + // A cd behind &&, ||, |, or & runs conditionally or in a subshell; + // whether it executes cannot be proven statically, so neither can the + // effective directory of later commands. Only a cd at the start of a + // sequence (start, ;, newline) provably runs. + if (sep === '&&' || sep === '||' || sep === '|' || sep === '&') { unresolved.push( `${source} command changes directory in a non-sequential position; effective directory cannot be proven: ${tokens.join(' ')}` ); @@ -5256,7 +5258,10 @@ export class WorkflowRunner { const resolvedSource = this.resolveProtectedPath(sourced, effectiveCwd); if (existsSync(resolvedSource)) { paths.push(resolvedSource); - if (depth < 3 && !visited.has(resolvedSource)) { + // The visited set terminates cycles; every distinct sourced file + // is inspected exactly once, with no arbitrary depth cutoff that + // would silently skip deep dependencies. + if (!visited.has(resolvedSource)) { visited.add(resolvedSource); try { const contents = readFileSync(resolvedSource, 'utf8');