fix(typescript): honor timeoutSeconds in DockerSandboxProvider executeCode

[jlaportebot]

Summary

Fixes #3118 - The TypeScript DockerSandboxProvider was ignoring the timeoutSeconds config parameter when executing code.

Changes

• Store session config (including timeoutSeconds) in createSession()
• Use stored timeout in executeCode() via timeout command inside container
• Add timeout detection (exit code 124) and proper killReason
• Clean up sessionConfigs map in destroySession()
• Add regression test for custom timeout behavior

Testing

All sandbox tests pass:

• creates, executes, and destroys a session
• uses custom timeoutSeconds from session config (new test)

The fix uses the timeout command inside the container (available in python:3.11-slim) to properly enforce execution time limits. The Node.js execFile timeout is set to timeoutSeconds + 5 seconds as a safety margin.

[github-actions]

🤖 AI Agent: security-scanner — View details

AI-generated review output. Treat it as untrusted analysis and verify before acting.

No security issues found.

[github-actions]

🤖 AI Agent: breaking-change-detector — API Compatibility

AI-generated review output. Treat it as untrusted analysis and verify before acting.

API Compatibility

Severity	Change	Impact
High	Added sessionConfigs map to DockerSandboxProvider class to store session configurations.	Potentially breaking for any code that relies on the internal structure of DockerSandboxProvider or directly manipulates its properties.
High	Modified executeCode method to use the timeout command with a session-specific timeoutSeconds value.	Potentially breaking for any code that relies on the previous behavior of executeCode without timeouts or with a fixed timeout.
High	Changed the executeCode method's behavior to include timeout detection and update the killReason field.	Potentially breaking for consumers of executeCode who rely on the previous killReason behavior or do not expect the timeout exit code handling.
High	Updated destroySession to also delete entries from the sessionConfigs map.	Potentially breaking for any code that assumes sessionConfigs entries persist beyond session destruction.

[github-actions]

🤖 AI Agent: docs-sync-checker — Docs Sync

AI-generated review output. Treat it as untrusted analysis and verify before acting.

Docs Sync

• DockerSandboxProvider in agent-governance-typescript/src/sandbox.ts -- missing docstrings for new sessionConfigs property and updated methods (createSession, executeCode, and destroySession).
• README.md -- no updates found related to the new timeoutSeconds functionality in DockerSandboxProvider.
• CHANGELOG.md -- missing entry for the behavioral change in DockerSandboxProvider to honor timeoutSeconds.

[github-actions]

🤖 AI Agent: test-generator — `agent-governance-typescript/src/sandbox.ts`

AI-generated review output. Treat it as untrusted analysis and verify before acting.

agent-governance-typescript/src/sandbox.ts

• test_invalid_timeoutSeconds -- Validate behavior when timeoutSeconds is set to an invalid value (e.g., negative or non-numeric).
• test_missing_timeoutSeconds -- Ensure default timeout is applied when timeoutSeconds is not provided in the session config.
• test_timeout_not_enforced -- Verify behavior when the timeout command is unavailable or fails inside the container.
• test_large_timeoutSeconds -- Test behavior with an extremely large timeoutSeconds value to ensure no overflow or unexpected behavior.

agent-governance-typescript/tests/sandbox.test.ts

• test_timeout_exit_code_handling -- Validate that non-timeout exit codes (other than 124) are handled correctly and do not trigger a timeout-related killReason.

[github-actions]

🤖 AI Agent: code-reviewer — View details

AI-generated review output. Treat it as untrusted analysis and verify before acting.

TL;DR: 0 blockers, 1 warning. The change improves timeout handling but introduces a potential resource cleanup issue.

#	Sev	Issue	Where
1	Warn	Potential resource leak if destroySession fails.	agent-governance-typescript/src/sandbox.ts

Action items:

1. Ensure robust error handling in destroySession() to prevent resource leaks if cleanup operations fail.

Warnings (fine as follow-up PRs):

#	Issue	Where
1	Potential resource leak if destroySession fails.	agent-governance-typescript/src/sandbox.ts

[github-actions]

🔴 Contributor Check: HIGH

Check	Result
Profile	HIGH
Credential	LOW
Overall	HIGH

Automated check by AGT Contributor Check.

[github-actions]

PR Review Summary

Check	Status	Details
🔍 Code Review	⚠️ Missing	No current-run comment
🛡️ Security Scan	⚠️ Missing	No current-run comment
🔄 Breaking Changes	⚠️ Missing	No current-run comment
📝 Docs Sync	⚠️ Missing	No current-run comment
🧪 Test Coverage	⚠️ Missing	No current-run comment

Verdict: ⚠️ AI review incomplete; ready for human review

AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims.

[imran-siddique]

Legit bug fix -- timeoutSeconds was hardcoded to 60000ms regardless of session config. Clean implementation: stores config per session, passes timeout to docker exec, detects exit code 124 for killReason. Approved.

[imran-siddique]

@MohammadHaroonAbuomar -- this PR is fully approved and all required checks are green. Could you merge it when you get a chance?

Side note: my merge access stopped working -- looks like I was removed from the microsoft/agent-governance-toolkit maintainer team when my Microsoft account was offboarded, so my permissions dropped from maintain to push. If you can add me back to the team (or re-add as a direct collaborator with maintain role), that would unblock me from merging on my own going forward. Otherwise I'll need to route these through you for now.

[imran-siddique]

Clean fix. The three-part timeout model is correct: (1) timeout <N> python3 inside the container enforces the wall-clock limit at the kernel level, (2) the outer execFile timeout of (timeoutSeconds + 5) * 1000 ms serves as a safety net if timeout itself hangs, and (3) exit code 124 is the GNU coreutils timeout contract for a killed child.

The killed: timedOut || killed and killReason: timedOut ? 'timeout' : killed ? 'signal' : '' correctly distinguishes between the timeout binary firing (124) vs the outer execFile wrapper killing the process for another reason. Previous code always set killReason = 'timeout' for any killed=true, which was wrong when the process was killed by signal rather than by time limit.

Storing config per session in sessionConfigs and deleting in the finally block of destroySession is correct; no leak risk.

One question worth noting for follow-up: defaultSandboxConfig().timeoutSeconds should be 60 to preserve the previous hardcoded 60_000 ms behavior. Not blocking since the test exercises the contract rather than a specific default, but worth a comment or constant.

LGTM.

[imran-siddique]

Clean fix for the missing timeout enforcement. The approach is correct:

• Storing SandboxConfig per session and retrieving it in executeCode is the right place to thread the timeout through.
• Using the timeout command inside the container to enforce the wall-clock limit is better than relying solely on Node's execFile timeout, which would leave the container process running.
• The execFile timeout of (timeoutSeconds + 5) * 1000 as a safety margin is sensible -- it ensures Node has a backstop if the container-level timeout somehow fails.
• Exit code 124 detection is correct (timeout uses 124 per POSIX convention).
• sessionConfigs.delete in the finally block prevents a memory leak.
• The regression test covers the new custom timeout path.

Only the maintainer gate is failing.

[imran-siddique]

@MohammadHaroonAbuomar can you take a look and merge when you get a chance? @imran-siddique has reviewed and approved this one.