feat(cli): add --agent-timeout flag for execution time limit#1242
feat(cli): add --agent-timeout flag for execution time limit#1242
Conversation
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 82.50% | 81.96% | 📉 -0.54% |
| Statements | 82.50% | 81.90% | 📉 -0.60% |
| Functions | 82.69% | 81.51% | 📉 -1.18% |
| Branches | 74.78% | 74.31% | 📉 -0.47% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/docker-manager.ts |
84.0% → 82.9% (-1.07%) | 83.3% → 82.0% (-1.30%) |
src/cli.ts |
47.0% → 46.2% (-0.83%) | 47.5% → 46.6% (-0.83%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
There was a problem hiding this comment.
Pull request overview
Adds a configurable execution time limit for agent commands via a new --agent-timeout <minutes> CLI flag, wiring the option through the CLI workflow into container execution so long-running builds/tests can be bounded.
Changes:
- Extend
WrapperConfigwithagentTimeout(minutes) and document it. - Add
--agent-timeoutCLI option and pass it throughrunMainWorkflowtorunAgentCommand. - Implement timeout behavior in
runAgentCommandby racingdocker waitwith a timer and stopping the container on expiry (exit code 124).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Adds agentTimeout?: number to wrapper configuration with JSDoc. |
| src/cli.ts | Introduces --agent-timeout option and parses/validates it into config. |
| src/docker-manager.ts | Implements timeout race + graceful container stop + exit code 124. |
| src/cli-workflow.ts | Threads config.agentTimeout into runAgentCommand. |
| src/cli-workflow.test.ts | Adds tests ensuring agentTimeout is passed to runAgentCommand. |
| docs/usage.md | Documents the new CLI flag in usage options. |
Comments suppressed due to low confidence (1)
src/docker-manager.ts:1477
- When the timeout wins
Promise.race,waitPromiseis left running without any cancellation/handling. Ifdocker waitlater rejects (e.g., container removed), this can surface as an unhandled rejection. Consider abortingdocker waitvia an AbortController/execasignal, or at least attaching a.catch()towaitPromiseto ensure late failures are handled.
const waitPromise = execa('docker', ['wait', 'awf-agent']).then(result => ({
type: 'completed' as const,
exitCodeStr: result.stdout,
}));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/docker-manager.ts
Outdated
| const timeoutPromise = new Promise<{ type: 'timeout' }>(resolve => | ||
| setTimeout(() => resolve({ type: 'timeout' }), timeoutMs) | ||
| ); |
There was a problem hiding this comment.
The timeout implementation schedules a setTimeout but never clears/unrefs it when docker wait wins the race. In Node.js, that pending timer will keep the event loop alive, causing awf to hang until the timeout duration elapses even after the agent command finishes. Please keep a handle to the timer and clearTimeout() (or timeout.unref()) once the wait completes.
This issue also appears on line 1474 of the same file.
src/cli.ts
Outdated
| const timeoutMinutes = parseInt(options.agentTimeout, 10); | ||
| if (isNaN(timeoutMinutes) || timeoutMinutes <= 0) { | ||
| logger.error('--agent-timeout must be a positive integer (minutes)'); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
--agent-timeout is documented/validated as a "positive integer", but parseInt() will accept values like "30m" or "1.5" (parsed as 30 / 1) and pass the current checks. To match the error message and avoid surprising behavior, validate the raw string (e.g., ^[1-9]\d*$) before parsing.
| const timeoutMinutes = parseInt(options.agentTimeout, 10); | |
| if (isNaN(timeoutMinutes) || timeoutMinutes <= 0) { | |
| logger.error('--agent-timeout must be a positive integer (minutes)'); | |
| process.exit(1); | |
| } | |
| if (!/^[1-9]\d*$/.test(options.agentTimeout)) { | |
| logger.error('--agent-timeout must be a positive integer (minutes)'); | |
| process.exit(1); | |
| } | |
| const timeoutMinutes = parseInt(options.agentTimeout, 10); |
| if (agentTimeoutMinutes) { | ||
| const timeoutMs = agentTimeoutMinutes * 60 * 1000; | ||
| logger.info(`Agent timeout: ${agentTimeoutMinutes} minutes`); | ||
|
|
||
| // Race docker wait against a timeout | ||
| const waitPromise = execa('docker', ['wait', 'awf-agent']).then(result => ({ | ||
| type: 'completed' as const, | ||
| exitCodeStr: result.stdout, | ||
| })); | ||
|
|
||
| const timeoutPromise = new Promise<{ type: 'timeout' }>(resolve => | ||
| setTimeout(() => resolve({ type: 'timeout' }), timeoutMs) | ||
| ); | ||
|
|
||
| const raceResult = await Promise.race([waitPromise, timeoutPromise]); | ||
|
|
||
| const exitCode = parseInt(exitCodeStr.trim(), 10); | ||
| if (raceResult.type === 'timeout') { | ||
| logger.warn(`Agent command timed out after ${agentTimeoutMinutes} minutes, stopping container...`); | ||
| // Stop the container gracefully (10 second grace period before SIGKILL) | ||
| await execa('docker', ['stop', '-t', '10', 'awf-agent'], { reject: false }); | ||
| exitCode = 124; // Standard timeout exit code (same as coreutils timeout) | ||
| } else { | ||
| exitCode = parseInt(raceResult.exitCodeStr.trim(), 10); | ||
| } |
There was a problem hiding this comment.
This PR adds new timeout behavior in runAgentCommand, but src/docker-manager.test.ts already has unit tests for this function and none cover the timeout path (exit code 124 + docker stop call + log streaming completion). Adding a focused test would help prevent regressions around the new race/stop logic.
|
🤖 Smoke test results for ✅ GitHub MCP: #1232 Overall: PASS
|
Smoke Test Results
Overall: PASS
|
This comment has been minimized.
This comment has been minimized.
Chroot Version Comparison Results
Result: ❌ Not all versions match — Python and Node.js differ between host and chroot environments.
|
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 82.50% | 82.47% | 📉 -0.03% |
| Statements | 82.50% | 82.48% | 📉 -0.02% |
| Functions | 82.69% | 82.93% | 📈 +0.24% |
| Branches | 74.78% | 74.57% | 📉 -0.21% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/cli.ts |
47.0% → 46.2% (-0.83%) | 47.5% → 46.6% (-0.83%) |
src/docker-manager.ts |
84.0% → 84.8% (+0.87%) | 83.3% → 84.2% (+0.91%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
Smoke Test Results
Overall: PASS
|
|
🔬 Smoke test results for run
Overall: PASS | Author:
|
This comment has been minimized.
This comment has been minimized.
|
Merged PRs:
|
Chroot Version Comparison Results
Result: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot environments.
|
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 83.46% | 83.46% | ➡️ +0.00% |
| Statements | 83.45% | 83.46% | ➡️ +0.01% |
| Functions | 83.64% | 83.94% | 📈 +0.30% |
| Branches | 76.15% | 76.14% | 📉 -0.01% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/cli.ts |
54.3% → 54.0% (-0.32%) | 54.8% → 54.5% (-0.33%) |
src/docker-manager.ts |
85.1% → 85.9% (+0.83%) | 84.4% → 85.3% (+0.87%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
|
Smoke test results for run 22968551101 — ✅ GitHub MCP: #1240 "test(docker): verify capsh execution chain after PR #715", #1232 "fix(cli): clear LD_PRELOAD after one-shot-token library loads" Overall: PASS
|
Smoke Test ResultsLast 2 merged PRs:
Overall: PASS
|
Chroot Version Comparison Results
Result: FAILED — Python and Node.js versions differ between host and chroot environments.
|
|
PR titles: feat(cli): add --agent-timeout flag for execution time limit | feat(cli): organize help text with logical option groups
|
This comment has been minimized.
This comment has been minimized.
Mossaka
force-pushed
the
feat/025-agent-timeout
branch
from
23ed792 to
422ddfd
Compare
Smoke Test Results✅ GitHub MCP — Last 2 merged PRs: #1240 "test(docker): verify capsh execution chain after PR #715", #1163 "test: expand credential hiding tests to all 14 protected paths" Overall: PASS
|
|
Smoke Test Results (run #22969587337) ✅ GitHub MCP: Last 2 merged PRs retrieved — #1240 Overall: PASS —
|
|
Merged PRs reviewed: test: expand credential hiding tests to all 14 protected paths; test(docker): verify capsh execution chain after PR #715
|
This comment has been minimized.
This comment has been minimized.
Chroot Version Comparison Results
Result: ❌ Not all versions match — Go matches, but Python and Node.js differ between host and chroot environments.
|
|
Smoke test results
|
Adds --agent-timeout flag that sets a maximum execution time (in minutes) for the agent command. When the timeout is reached, the container is gracefully stopped and AWF exits with code 124 (matching coreutils timeout convention). This helps large projects like nushell, Dapper, and Polly that need more than 20 minutes for builds/tests. Fixes #948 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cover the timeout branch (exit code 124) and normal completion with timeout set to fix coverage regression. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract inline timeout validation into exported parseAgentTimeout() function and add 7 unit tests covering valid integers, zero, negative, non-numeric, empty string, and large values. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Mossaka
force-pushed
the
feat/025-agent-timeout
branch
from
dde87da to
078ed8c
Compare
Smoke Test Results ✅ PASS
PR author:
|
|
Smoke Test Results — PASS
|
|
GitHub MCP merged PRs: ✅ test: add logger/aggregator tests for blocked domain detection; feat(cli): organize help text with logical option groups
|
Chroot Version Comparison Results
Overall: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot environments.
|
Move agent timeout validation from inline action handler to a standalone exported function, enabling direct unit test coverage for all branches (undefined, valid, invalid paths). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add unit tests for the formatItem help formatter function to offset the minor branch coverage regression (-0.01%) from the agent timeout feature. Tests cover all three branches: term fits with description, term wraps to next line, and term without description. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (2 files)
Coverage comparison generated by |
1 similar comment
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (2 files)
Coverage comparison generated by |
Chroot Version Comparison Results
Result: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot environments.
|
Smoke Test Results
Overall: PASS
|
Smoke Test ResultsLast 2 merged PRs (
Overall: PASS
|
|
PRs: test: add logger/aggregator tests for blocked domain detection | ci: skip CI when only release.yml changes
|
This comment has been minimized.
This comment has been minimized.
- Clear setTimeout when docker wait wins the race to prevent event loop hang - Use regex validation (^[1-9]\d*$) instead of parseInt to reject values like "30m" or "1.5" that parseInt would silently accept - Add tests for stricter validation edge cases Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (2 files)
Coverage comparison generated by |
Smoke Test Results
Overall: PASS
|
|
Merged PRs reviewed: fix: add missing formatItem and program imports in cli.test.ts; test: add logger/aggregator tests for blocked domain detection
|
Smoke Test Results ✅ PASS
Last 2 merged PRs:
cc
|
Chroot Version Comparison Results
Result: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot environments.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
Summary
--agent-timeout <minutes>CLI flag to set a maximum execution time for agent commandstimeoutconvention)Changes
src/types.ts: AddedagentTimeoutfield toWrapperConfigsrc/cli.ts: Added--agent-timeoutCLI option with validation (must be positive integer)src/docker-manager.ts:runAgentCommandracesdocker waitagainst a timeout, stops container on expirysrc/cli-workflow.ts: Pass timeout through torunAgentCommanddocs/usage.md: Document new flagsrc/cli-workflow.test.ts: Tests for timeout parameter passingUsage
Fixes #948
Test plan
npm run buildpassesnpm testpasses (858 tests)npm run lintpasses (0 errors)🤖 Generated with Claude Code