Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions docs/FLOWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -710,8 +710,7 @@ parseSubAgentOutput(stdout):

resolveTimeout(options):
├── if options.timeout provided → options.timeout
├── else if MADZ_SUBAGENT_TIMEOUT env var → parseInt(env)
├── else → config.process.subAgent.timeout (default 600000)
└── else → config.process.subAgent.timeout (default 600000)
```

**Process tracking:** Sub-agents share the `processTracker` Map from `terminal.js` for PID tracking and lifecycle management. Each sub-agent gets a unique PID that can be polled, waited on, or killed via the `process` tool.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
schema: spec-driven
created: 2026-06-24
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
## Context

The subAgent tool in `src/tools/subAgent.js` manages child process spawning with configurable timeouts and session isolation. Two environment variables were introduced as configuration paths: `MADZ_SUBAGENT_TIMEOUT` (a per-process timeout override) and `MADZ_SESSION_ID` (for session isolation). However, `MADZ_SUBAGENT_TIMEOUT` creates a third configuration tier that bypasses the centralized config system, and `MADZ_SESSION_ID` is passed to child processes but never consumed by either parent or child.

The `config.yaml` already defines `process.subAgent.timeout: 600000` which is read correctly by the tool for maxConcurrent, defaultStrategy, and defaultOnError — only timeout has the env var detour.

## Goals / Non-Goals

**Goals:**
- Remove `MADZ_SUBAGENT_TIMEOUT` env var check from `resolveTimeout()`, restoring the intended priority chain: per-call → config.yaml → hardcoded default
- Remove `MADZ_SESSION_ID` from child process environment — it's never read
- Update tool description to remove env var reference
- Remove tests that validate the dead env var behavior

**Non-Goals:**
- No changes to config.yaml structure
- No new configuration mechanisms
- No migration path for users relying on MADZ_SUBAGENT_TIMEOUT (they'll silently fall back to config.yaml default, which has the same value)

## Decisions

1. **Remove env var entirely rather than deprecate first** — This is dead code, not a deprecated feature. The env var was never documented as a public API, and the config.yaml default has always been the intended source of truth. A deprecation cycle would add unnecessary noise.

2. **No config.yaml changes needed** — The timeout is already defined in `process.subAgent.timeout: 600000`. The env var was a redundant override path, not a replacement for config.yaml.

3. **Session isolation future-proofing** — If session isolation is needed in the future, it should be implemented through the config system (consistent with maxConcurrent, defaultStrategy, defaultOnError), not ad-hoc environment variables.

## Risks / Trade-offs

- [Risk] Users setting MADZ_SUBAGENT_TIMEOUT in their environment will silently fall back to config.yaml default → **Mitigation**: The config.yaml default (600000ms) is the same value the env var would have used, so behavior is unchanged for anyone using the default. Only users with a custom env var value will see a change, and that value was never documented as a supported configuration path.
- [Risk] Tests explicitly validate env var priority behavior → **Mitigation**: Remove these tests and ensure remaining coverage is adequate for timeout resolution.

## Migration Plan

No migration needed. This is a cleanup of dead code. The config.yaml default is the source of truth and has always been the intended path.

## Open Questions

None. This is a straightforward cleanup with no ambiguity.
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
## Why

The subAgent tool contains two pieces of dead code that create confusion and technical debt: the `MADZ_SUBAGENT_TIMEOUT` environment variable check (which bypasses the centralized config system) and the `MADZ_SESSION_ID` environment variable (which is passed to child processes but never consumed). These were introduced as configuration paths that no longer serve a purpose, creating a third configuration tier that contradicts the intended priority chain and adding noise to the process environment.

## What Changes

- Remove `MADZ_SUBAGENT_TIMEOUT` env var check from `resolveTimeout()` in `src/tools/subAgent.js` — fall through directly from per-call timeout to config.yaml default
- Remove `MADZ_SESSION_ID` from child process environment in `src/tools/subAgent.js` — it's never read by parent or child
- Update tool description to remove "Overrides MADZ_SUBAGENT_TIMEOUT env var" reference
- Remove tests that validate env var priority behavior in `tests/unit/tools/subAgent.test.js`
- No new capabilities or API changes — this is a cleanup of dead code only

## Capabilities

### New Capabilities
<!-- None — this is a cleanup task, no new capabilities -->

### Modified Capabilities
- `subagent`: Remove env var priority behavior from timeout resolution; remove unused session ID env var

## Impact

- `src/tools/subAgent.js` — resolveTimeout() function and child process spawn
- `tests/unit/tools/subAgent.test.js` — env var priority tests
- `config.yaml` — no changes needed (timeout already defined in process.subAgent section)
- No API changes, no breaking changes for users using config.yaml (which is the intended path)
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
## ADDED Requirements

### Requirement: Child process environment
The subAgent tool SHALL pass only necessary environment variables to child processes. No unused or dead environment variables SHALL be included in the child process environment.

#### Scenario: Child process receives necessary env vars
- **WHEN** a sub-agent is spawned
- **THEN** it inherits the parent process's environment variables (API keys, config paths) without unused variables like MADZ_SESSION_ID

#### Scenario: No dead env vars passed to child
- **WHEN** a sub-agent is spawned
- **THEN** the child process environment does not include MADZ_SESSION_ID

## MODIFIED Requirements

### Requirement: Timeout enforcement
The subAgent tool SHALL enforce timeouts with priority: per-call `timeout` parameter > `config.yaml` default.

#### Scenario: Per-call timeout overrides config
- **WHEN** subAgent is called with timeout 30000 and config default is 60000
- **THEN** the sub-agent uses 30000ms timeout

#### Scenario: Config default is used when no per-call override
- **WHEN** no per-call timeout is provided and config.yaml defines process.subAgent.timeout as 600000
- **THEN** the sub-agent uses 600000ms timeout

#### Scenario: Timeout kills process
- **WHEN** sub-agent exceeds its timeout
- **THEN** the process receives SIGTERM, then SIGKILL after 5 seconds

### Requirement: Configuration
The subAgent tool SHALL be configured via `config.yaml` under `process.subAgent` with settings for timeout, maxConcurrent, sessionMode, defaultStrategy, and defaultOnError.

#### Scenario: Config section exists
- **WHEN** config.yaml is loaded
- **THEN** `process.subAgent` section contains timeout, maxConcurrent, sessionMode, defaultStrategy, defaultOnError

#### Scenario: Config defaults are applied
- **WHEN** no per-call overrides are provided
- **THEN** config defaults are used for timeout, maxConcurrent, sessionMode, defaultStrategy, defaultOnError
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
## 1. Remove MADZ_SUBAGENT_TIMEOUT env var

- [ ] 1.1 Remove `process.env.MADZ_SUBAGENT_TIMEOUT` check from `resolveTimeout()` in `src/tools/subAgent.js`
- [ ] 1.2 Verify the fallback chain works: per-call → config.yaml `process.subAgent.timeout` → hardcoded default (600000ms)
- [ ] 1.3 Update tool description to remove "Overrides MADZ_SUBAGENT_TIMEOUT env var" reference

## 2. Remove MADZ_SESSION_ID from child process env

- [ ] 2.1 Remove `MADZ_SESSION_ID` from child process environment in `src/tools/subAgent.js`
- [ ] 2.2 Search entire codebase for any other references to `MADZ_SESSION_ID` and remove them
- [ ] 2.3 Verify child process still receives all other necessary env vars

## 3. Update tests

- [ ] 3.1 Remove tests that validate `MADZ_SUBAGENT_TIMEOUT` env var priority in `tests/unit/tools/subAgent.test.js`
- [ ] 3.2 Remove any tests that set or check `MADZ_SESSION_ID`
- [ ] 3.3 Ensure remaining tests still provide adequate coverage for timeout and session behavior

## 4. Verify

- [ ] 4.1 Run `npm run test` and verify all tests pass
- [ ] 4.2 Run `npm run lint` and verify no lint errors
- [ ] 4.3 Run `npm start` and verify application starts without crashing
11 changes: 0 additions & 11 deletions openspec/specs/subagent-session-id/spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,6 @@ The system SHALL generate a unique session ID (UUID v4) for each subAgent invoca
- **WHEN** multiple subAgents are invoked concurrently or sequentially
- **THEN** each invocation receives a distinct session ID with no collisions

### Requirement: Session ID Propagation
The system SHALL pass the session ID to the sub-agent child process via the `MADZ_SESSION_ID` environment variable.

#### Scenario: Session ID is passed to child process
- **WHEN** a subAgent process is spawned
- **THEN** the child process receives `MADZ_SESSION_ID` in its environment with the correct session ID value

#### Scenario: Session ID survives in child process
- **WHEN** the child process accesses `process.env.MADZ_SESSION_ID`
- **THEN** it receives the same session ID that was generated by the parent process

### Requirement: Session ID-Based Log File Naming
The system SHALL use the session ID for log file naming instead of the child process PID.

Expand Down
21 changes: 16 additions & 5 deletions openspec/specs/subagent/spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -101,15 +101,15 @@ The subAgent tool SHALL support fan-out mode with sequential strategy, running t
- **THEN** remaining tasks are cancelled; partial results are returned

### Requirement: Timeout enforcement
The subAgent tool SHALL enforce timeouts with priority: per-call `timeout` parameter > `MADZ_SUBAGENT_TIMEOUT` env var > `config.yaml` default.
The subAgent tool SHALL enforce timeouts with priority: per-call `timeout` parameter > `config.yaml` default.

#### Scenario: Per-call timeout overrides config
- **WHEN** subAgent is called with timeout 30000 and config default is 60000
- **THEN** the sub-agent uses 30000ms timeout

#### Scenario: Env var overrides config
- **WHEN** MADZ_SUBAGENT_TIMEOUT is set to 45000 and config default is 60000
- **THEN** the sub-agent uses 45000ms timeout (no per-call override)
#### Scenario: Config default is used when no per-call override
- **WHEN** no per-call timeout is provided and config.yaml defines process.subAgent.timeout as 600000
- **THEN** the sub-agent uses 600000ms timeout

#### Scenario: Timeout kills process
- **WHEN** sub-agent exceeds its timeout
Expand Down Expand Up @@ -160,6 +160,17 @@ The subAgent tool SHALL be configured via `config.yaml` under `process.subAgent`
- **THEN** `process.subAgent` section contains timeout, maxConcurrent, sessionMode, defaultStrategy, defaultOnError

#### Scenario: Config defaults are applied
- **WHEN** no per-call or env var overrides are provided
- **WHEN** no per-call overrides are provided
- **THEN** config defaults are used for timeout, maxConcurrent, sessionMode, defaultStrategy, defaultOnError

### Requirement: Child process environment
The subAgent tool SHALL pass only necessary environment variables to child processes. No unused or dead environment variables SHALL be included in the child process environment.

#### Scenario: Child process receives necessary env vars
- **WHEN** a sub-agent is spawned
- **THEN** it inherits the parent process's environment variables (API keys, config paths) without unused variables like MADZ_SESSION_ID

#### Scenario: No dead env vars passed to child
- **WHEN** a sub-agent is spawned
- **THEN** the child process environment does not include MADZ_SESSION_ID

14 changes: 3 additions & 11 deletions src/tools/subAgent.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,7 @@ export function spawnSubAgentProcess(prompt, sessionsDir, timeout) {
// timeout sends SIGTERM first, then SIGKILL after --kill-after delay
const child = spawn("timeout", ["--kill-after=10", timeoutSeconds.toString(), "node", "index.js", prompt, sessionsDir], {
stdio: ["pipe", "pipe", "pipe"],
env: {
...process.env,
MADZ_SESSION_ID: sessionId,
},
env: process.env,
});

const logPath = `/tmp/sub-agent-${sessionId}.log`;
Expand Down Expand Up @@ -250,7 +247,7 @@ async function executeFanOut(tasks, strategy, maxConcurrent, onError, sessionsDi
}

/**
* Resolve timeout with priority: per-call > env var > config default.
* Resolve timeout with priority: per-call > config default.
* @param {number | undefined} perCallTimeout - Per-call timeout parameter
* @param {object} config - Resolved config object
* @returns {number} Resolved timeout in milliseconds
Expand All @@ -260,11 +257,6 @@ function resolveTimeout(perCallTimeout, config) {
return perCallTimeout;
}

const envTimeout = process.env.MADZ_SUBAGENT_TIMEOUT;
if (envTimeout !== undefined && envTimeout !== "") {
return parseInt(envTimeout, 10);
}

const configTimeout = config?.process?.subAgent?.timeout;
if (configTimeout !== undefined && configTimeout !== null) {
return configTimeout;
Expand Down Expand Up @@ -407,7 +399,7 @@ export function createSubAgentTool(options = {}) {
.positive()
.optional()
.describe(
"Timeout in milliseconds for this sub-agent execution. Overrides MADZ_SUBAGENT_TIMEOUT env var and config default.",
"Timeout in milliseconds for this sub-agent execution. Overrides config default.",
),
}),
},
Expand Down
29 changes: 0 additions & 29 deletions tests/unit/tools/subAgent.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,21 +74,6 @@ describe("resolveTimeout", () => {
assert.strictEqual(resolveTimeout(30000, config), 30000);
});

it("should use env var when per-call is not provided", () => {
const original = process.env.MADZ_SUBAGENT_TIMEOUT;
process.env.MADZ_SUBAGENT_TIMEOUT = "45000";
assert.strictEqual(resolveTimeout(undefined, {}), 45000);
process.env.MADZ_SUBAGENT_TIMEOUT = original;
});

it("should use env var over config default", () => {
const original = process.env.MADZ_SUBAGENT_TIMEOUT;
process.env.MADZ_SUBAGENT_TIMEOUT = "45000";
const config = { process: { subAgent: { timeout: 600000 } } };
assert.strictEqual(resolveTimeout(undefined, config), 45000);
process.env.MADZ_SUBAGENT_TIMEOUT = original;
});

it("should use config default when no per-call or env var", () => {
const config = { process: { subAgent: { timeout: 120000 } } };
assert.strictEqual(resolveTimeout(undefined, config), 120000);
Expand Down Expand Up @@ -164,20 +149,6 @@ describe("msToSeconds", () => {
});

describe("spawnSubAgentProcess integration", () => {
it("should pass session ID to child process via MADZ_SESSION_ID env var", async () => {
const prompt = "# SubAgent\n\n{ ok: true, result: \"test\" }";
const sessionsDir = join(__dirname, "../../../memory/sessions/");

const result = await spawnSubAgentProcess(prompt, sessionsDir, 10000);

assert.strictEqual(result.ok, true);
assert.ok(result.sessionId, "Result should include sessionId");
assert.strictEqual(typeof result.sessionId, "string");
// Verify sessionId is a valid UUID v4 format
const uuidV4Regex = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i;
assert.ok(uuidV4Regex.test(result.sessionId), `Expected UUID v4 format, got: ${result.sessionId}`);
});

it("should create log file with session ID naming", async () => {
const prompt = "# SubAgent\n\n{ ok: true, result: \"test\" }";
const sessionsDir = join(__dirname, "../../../memory/sessions/");
Expand Down