Skip to content

secrets: default exec no-output timeout to timeoutMs#32235

Merged
steipete merged 2 commits intoopenclaw:mainfrom
bmendonca3:bm/secrets-no-output-timeout
Mar 2, 2026
Merged

secrets: default exec no-output timeout to timeoutMs#32235
steipete merged 2 commits intoopenclaw:mainfrom
bmendonca3:bm/secrets-no-output-timeout

Conversation

@bmendonca3
Copy link

@bmendonca3 bmendonca3 commented Mar 2, 2026

Summary

Describe the problem and fix in 2–5 bullets:

  • Problem: branch contains a scoped bugfix that is not yet merged to .
  • Why it matters: this path can produce incorrect runtime behavior for users in affected flows.
  • What changed: minimal fix plus regression coverage aligned with existing local patterns.
  • What did NOT change (scope boundary): no unrelated refactors, no dependency updates, no behavior outside the touched module.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

  • Closes #
  • Related #

User-visible / Behavior Changes

  • Fixes behavior addressed by .

Security Impact (required)

  • New permissions/capabilities? (Yes/No) No
  • Secrets/tokens handling changed? (Yes/No) No
  • New/changed network calls? (Yes/No) No
  • Command/tool execution surface changed? (Yes/No) No
  • Data access scope changed? (Yes/No) No
  • If any Yes, explain risk + mitigation:

Repro + Verification

Environment

  • OS: macOS
  • Runtime/container: Node 22 + pnpm
  • Model/provider: N/A
  • Integration/channel (if any): module-specific
  • Relevant config (redacted): default test fixtures

Steps

  1. Check out branch .
  2. Run regression tests for touched boundary.
  3. Validate behavior in touched code path.

Expected

  • Targeted flow behaves correctly and regression tests pass.

Actual

  • Verified passing on this branch after fix.

Evidence

Attach at least one:

  • Failing test/log before + passing after
  • Trace/log snippets
  • Screenshot/recording
  • Perf numbers (if relevant)

Human Verification (required)

What you personally verified (not just CI), and how:

  • Verified scenarios: pnpm -s vitest run src/secrets/resolve.test.ts.
  • Edge cases checked: changed branch regression boundary only.
  • What you did not verify: full live multi-channel end-to-end deployment behavior.

Compatibility / Migration

  • Backward compatible? (Yes/No) Yes
  • Config/env changes? (Yes/No) No
  • Migration needed? (Yes/No) No
  • If yes, exact upgrade steps:

Failure Recovery (if this breaks)

  • How to disable/revert this change quickly: revert this PR commit.
  • Files/config to restore: src/secrets/resolve.test.ts,src/secrets/resolve.ts
  • Known bad symptoms reviewers should watch for: regression in the touched boundary.

Risks and Mitigations

List only real risks for this PR. Add/remove entries as needed. If none, write None.

  • Risk: subtle behavior differences around branch-specific edge inputs.
    • Mitigation: targeted regression tests in touched modules.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 2, 2026

Greptile Summary

This PR fixes a bug in the exec secret provider where a hardcoded DEFAULT_EXEC_NO_OUTPUT_TIMEOUT_MS of 2000ms would silently override a user-configured timeoutMs, causing processes that legitimately needed more than 2 seconds to produce initial output to be killed prematurely. The fix removes the hardcoded constant and instead defaults noOutputTimeoutMs to the resolved timeoutMs value, making the behavior consistent with user expectations.

Key changes:

  • DEFAULT_EXEC_NO_OUTPUT_TIMEOUT_MS (2000ms) constant removed from resolve.ts; noOutputTimeoutMs now defaults to timeoutMs when not explicitly configured
  • A new regression test in resolve.test.ts validates the fix by spawning a script that delays 2200ms before writing output — which would have triggered the old no-output timeout but correctly resolves with the new default
  • The overall timeout (timeoutTimer) still provides a hard cap, so no process can run beyond the configured timeoutMs regardless of how noOutputTimeoutMs is set

Confidence Score: 5/5

  • This PR is safe to merge — the change is minimal, well-scoped, and directly backed by a targeted regression test.
  • The fix is a single two-line change with clear intent: replacing a hardcoded 2000ms constant with the user-configured timeoutMs. The logic is sound — the hard timeoutTimer still provides the absolute cap, so there is no risk of processes running beyond the configured timeout. The new test directly exercises the previously broken path (2200ms delay vs. old 2000ms default) and confirms the fix. No unrelated code is touched.
  • No files require special attention.

Last reviewed commit: d0de192

@steipete steipete force-pushed the bm/secrets-no-output-timeout branch from d0de192 to c2d7a04 Compare March 2, 2026 22:47
@steipete steipete merged commit a78ec81 into openclaw:main Mar 2, 2026
@steipete
Copy link
Contributor

steipete commented Mar 2, 2026

Landed via temp rebase onto main.

  • Gate: pnpm -s vitest run src/secrets/resolve.test.ts
  • Land commit: c2d7a04
  • Merge commit: a78ec81

Thanks @bmendonca3!

dawi369 pushed a commit to dawi369/davis that referenced this pull request Mar 3, 2026
@steipete @dawi369
fix: align exec no-output timeout defaults (openclaw#32235) (thanks @…
8a3b9e2 
…bmendonca3)
OWALabuy pushed a commit to kcinzgg/openclaw that referenced this pull request Mar 4, 2026
@steipete @OWALabuy
fix: align exec no-output timeout defaults (openclaw#32235) (thanks @…
51a3d09 
…bmendonca3)
zooqueen pushed a commit to hanzoai/bot that referenced this pull request Mar 6, 2026
@steipete
fix: align exec no-output timeout defaults (openclaw#32235) (thanks @…
f0b68cd 
…bmendonca3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

size: XS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bmendonca3 @steipete