Skip to content

fix: cap implementation prompt size and runner timeout - #17

Open
Zauberio wants to merge 11 commits into
Jhacarreiro:mainfrom
Zauberio:fix/prompt-size-cap
Open

fix: cap implementation prompt size and runner timeout#17
Zauberio wants to merge 11 commits into
Jhacarreiro:mainfrom
Zauberio:fix/prompt-size-cap

Conversation

@Zauberio

@Zauberio Zauberio commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Summary

createImplementationRunnerSession wrote params.prompt to prompt.txt with no size gate, and timeoutSeconds had no upper bound.

Change

  • DEVELOPMENT_CYCLE_MAX_PROMPT_BYTES (default 2 MiB) → { ok:false, error:"prompt_too_large" }
  • DEVELOPMENT_CYCLE_MAX_RUNNER_TIMEOUT_SECONDS (default 24h) clamps effective timeout

Test plan

  • npm run build
  • npm test (jq baseline only on this host)
  • Grok 4.5 High: CONFIRMED

Validation

  • Head: d29586526715 (contains current main; base-gate compliant)
  • Full-suite run in bwrap sandbox at this exact head:
# evidence: Jhacarreiro/openclaw-development-cycle#17 @ d29586526715 (base -)

## head: npm ci (net, cached) — exit 0, 11s

added 312 packages in 11s


## head: npm run build (netless) — exit 0, 2s

> openclaw-plugin-development-cycle@0.1.0 build
> tsc -p tsconfig.json



## head: npm test (netless) — exit 0, 12s

> openclaw-plugin-development-cycle@0.1.0 test
> npm run build && node --test tests/*.test.mjs


> openclaw-plugin-development-cycle@0.1.0 build
> tsc -p tsconfig.json

✔ cleanId rejects path traversal tokens (2.414858ms)
✔ configuration uses portable, command-first defaults (3.676167ms)
✔ configuration accepts command and Octopus adapter overrides (0.919361ms)
✔ repository delivery is opt-in and configurable (0.968973ms)
✔ core modules do not import OpenClaw or runtime adapters (12.139532ms)
✔ accepts the three contractual decisions (2.825899ms)
✔ rejects ambiguous or accidental first tokens (0.627322ms)
✔ example command adapter consumes request v1 without changing the source checkout (50.384261ms)
✔ filesystem storage uses safe run paths and atomic status updates (31.416824ms)
✔ updateStatus serializes two processes onto one status file (418.528667ms)
✔ updateStatus recovers a stale lock left by a dead holder (43.658172ms)
✔ stale recovery claim abandoned before owner publication is recoverable (142.364727ms)
✔ a live holder is not evicted when the lock mtime is stale (132.67909ms)
✔ persistent lock setup errors respect the acquisition timeout (119.016949ms)
✔ stale-lock rename failure remains bounded and later acquire recovers (278.342463ms)
✔ stale ownerless lock from interrupted legacy publication is recoverable (4.635436ms)
✔ crash before owner publication leaves only a nonblocking candidate directory (4.551029ms)
✔ concurrent release calls cannot remove a replacement lock (7.117425ms)
✔ delayed release does not remove a replacement lock (11.844974ms)
✔ runner heartbeat interpolates a JSON-encoded observer session id (2.098947ms)
✔ generated runner verifies projectRoot inode identity before adapter execution (0.517505ms)
✔ generated runner stays shell-safe and emits parseable heartbeat JSON (90.875011ms)
✔ partial delivery opens normal PR and residual issues without merge (672.461749ms)
✔ successful delivery queues GitHub auto-merge (557.224468ms)
✔ delivery runner refuses sensitive changed paths (104.720524ms)
✔ status operation is read-only and reports pending PR (157.661496ms)
✔ status operation reports completed merge (155.618457ms)
✔ cleanId produces bounded path-safe identifiers (1.495997ms)
✔ newRunId is deterministic with an injected clock (3.731427ms)
✔ command adapter receives the stable request JSON path (3.162773ms)
✔ Octopus adapter translates the generic request into orchestrate.sh (0.552117ms)
✔ Octopus adapter omits timeout when the control plane delegates timeout policy (0.673211ms)
✔ shell rendering quotes executable, arguments and environment values (0.727859ms)
✔ jsonShellQuote encodes JSON then shell-quotes the result (2.524248ms)
✔ record_plan rejects planPath outside allowed roots (805.240044ms)
✔ run_final_validation ignores validationConfigPath outside allowed roots (103.474678ms)
✔ planPath via symlink outside roots is rejected (66.034162ms)
✔ malicious projectWikiPath does not expand planPath allowlist (107.175679ms)
✔ projectWikiPath that is a dir-symlink under wiki root does not write outside (43.162853ms)
✔ broad projectRoot does not make outside planPath readable (63.280319ms)
✔ resolveTru

@Zauberio

Zauberio commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Amended (OCL-K, Grok CONFIRMED): DEVELOPMENT_CYCLE_MAX_TOOL_TEXT_BYTES (2 MiB) for direction/planText/deliveryText/validationText via assertToolText. npm run build green.

@Jhacarreiro

Copy link
Copy Markdown
Owner

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@Zauberio, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 47 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 95bf2660-5031-4fce-b292-5e504ce8a99d

📥 Commits

Reviewing files that changed from the base of the PR and between 54a1049 and e19d248.

📒 Files selected for processing (3)
  • docs/configuration.md
  • src/index.ts
  • tests/prompt-size-cap.test.mjs

Comment @coderabbitai help to get the list of available commands.

@Jhacarreiro Jhacarreiro added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Aug 11, 2026
@Jhacarreiro

Jhacarreiro commented Aug 11, 2026

Copy link
Copy Markdown
Owner

Codex review: needs real behavior proof before merge. Reviewed August 22, 2026, 10:27 PM ET / August 23, 2026, 02:27 UTC.

ClawSweeper review

What this changes

The branch adds load-time prompt and tool-text byte caps, rejects non-finite per-call timeouts, clamps recorded timeout metadata, updates runner-limit documentation, and adds focused cap tests.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

Blocked until real behavior proof is added - 14 items remain

Keep this PR open. Against the exact merge result, current main still lacks the requested hardening, but the branch retains six actionable blockers from the prior review: incomplete cap parsing, uncapped direct/file handoffs, post-write rejection, no shared command-runner deadline, schema-invalid timeout values, and removed retry documentation. The visible evidence remains install/build/test output rather than a real configured runner run. Likely related people are Jhacarreiro, the current-main control-plane contributor, and Zauberio, a recent runner-path contributor.

Priority: P2
Reviewed head: a9d42af8dd39f349b1aea0ab7c9f267e561b2829
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The PR contains focused hardening and useful boundary tests, but six actionable contract gaps remain and real configured-run proof is absent.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The external PR shows sandboxed npm ci/build/test output only; it has no inspectable after-fix configured command or Octopus run with observed prompt rejection before writes and timeout enforcement. Add redacted terminal/live output, logs, a recording, or a linked artifact, and refresh the PR body at the exact head. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦪 silver shellfish (2/6) 6 actionable review findings remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The external PR shows sandboxed npm ci/build/test output only; it has no inspectable after-fix configured command or Octopus run with observed prompt rejection before writes and timeout enforcement. Add redacted terminal/live output, logs, a recording, or a linked artifact, and refresh the PR body at the exact head. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 12 items Exact merge result: The PR head is a merge commit whose result differs from current main only in docs/configuration.md, src/index.ts, and tests/prompt-size-cap.test.mjs; the stale-base package, adapter, README, and plan-test changes are already in main or not part of the resulting delta.
Current-main necessity: Current main contains no MAX_PROMPT_BYTES, MAX_TOOL_TEXT_BYTES, or MAX_RUNNER_TIMEOUT_SECONDS implementation, so the central hardening is not already merged.
Cap parsing defect: The new envCap helper uses Number.parseInt, which accepts a suffix such as 4096junk instead of applying the documented non-numeric fallback.
Findings 6 actionable findings [P2] Parse cap values as complete numeric tokens
[P2] Apply the text cap before direct handoff assembly
[P2] Reject oversized handoffs before writing them
Security None None.

How this fits together

This plugin receives plans and validation feedback, persists them as durable handoffs, and launches a portable command or Octopus implementation runner through a process-group supervisor. The reviewed guards sit between tool input, durable artifacts, runner metadata, and process execution.

flowchart LR
A[Plan or feedback input] --> B[Text and timeout checks]
B --> C[Durable handoff files]
B --> D[Runner request metadata]
D --> E[Process-group supervisor]
E --> F[Command or Octopus adapter]
F --> G[Run status and logs]
Loading

Decision needed

Question Recommendation
Should this PR replace the merged delegated-timeout contract with a positive server-wide deadline, and which boundary must enforce it for both adapters? Preserve delegation by default: Keep the existing zero/default delegation behavior, make any shared hard cap explicitly opt-in, and prove both delegated and bounded modes.

Why: Merged #32 documents and tests timeout delegation with a zero default, while this branch documents a positive default but does not change the source default or enforce the deadline for the command path; choosing the compatibility policy is maintainer intent, not a safe automated inference.

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The external PR shows sandboxed npm ci/build/test output only; it has no inspectable after-fix configured command or Octopus run with observed prompt rejection before writes and timeout enforcement. Add redacted terminal/live output, logs, a recording, or a linked artifact, and refresh the PR body at the exact head. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Parse cap values as complete numeric tokens (P2) - envCap uses Number.parseInt, so a value such as 4096junk is accepted as 4096 even though the new documentation says non-numeric values fall back to the default. Parse the complete trimmed token and cover suffix, empty, negative, and fractional inputs.
  • Apply the text cap before direct handoff assembly (P2) - The new assertToolText calls cover request_plan and the record actions, but start_implementation.planText, start_corrections.feedbackText, council correction feedback, and file-loaded text are consumed before this boundary. A caller can bypass the advertised tool-text limit or force an unbounded file read.
  • Reject oversized handoffs before writing them (P2) - start_implementation and start_corrections write implementation_request.md or corrections_request.md before createImplementationRunnerSession performs the prompt check. An oversized request is rejected only after the large durable handoff and observer setup have already occurred.
  • Enforce the timeout at the shared runner boundary (P2) - The new max is copied into request/status metadata and reaches Octopus as --timeout, but the portable command adapter gets only the request path and runner-supervisor.py launches without a deadline. A command run can therefore exceed the documented server-wide cap indefinitely.
  • Emit a positive schema-valid timeout value (P2) - With DEVELOPMENT_CYCLE_DEFAULT_TIMEOUT_SECONDS still defaulting to 0, the new expression produces timeoutSeconds: 0 in every default request even though the request schema requires any present timeout to be an integer of at least 1; fractional per-call values remain fractional as well. Normalize to a positive integer or omit the field when policy is delegated, and align the documented default.
  • Keep the supported Octopus retry setting documented (P3) - The patch removes DEVELOPMENT_CYCLE_LOOP_UNTIL_APPROVED from the configuration table, while src/config.ts still reads it and the Octopus adapter still exports LOOP_UNTIL_APPROVED. Restore the row so the supported compatibility setting remains discoverable.
  • Resolve merge risk (P2) - The advertised server-wide timeout is not enforced by the portable command path or runner-supervisor.py, and the current default remains 0 despite the branch documentation claiming a positive bounded default.
  • Resolve merge risk (P1) - Direct start and file-loaded text paths can bypass the tool-text boundary or leave durable handoff/observer artifacts before rejection.
  • Resolve merge risk (P1) - The current PR body and visible discussion do not provide exact-head real configured-run proof for prompt rejection and deadline enforcement.
  • Improve patch quality - Repair the six findings at the current head and add focused tests for every text ingress and both adapter timeout paths.
  • Improve patch quality - Attach redacted live command and Octopus traces showing prompt rejection before durable writes and actual deadline termination.
  • Improve patch quality - Refresh the PR body with validation from the exact current head.

Findings

  • [P2] Parse cap values as complete numeric tokens — src/index.ts:19-23
  • [P2] Apply the text cap before direct handoff assembly — src/index.ts:30-36
  • [P2] Reject oversized handoffs before writing them — src/index.ts:216-223
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Exact merge delta 3 files affected; src/index.ts +65/-8, docs/configuration.md +11/-2, tests/prompt-size-cap.test.mjs +148/-0 The resulting patch is concentrated in the control plane and adds tests, but it does not modify the shared supervisor or adapter implementation that would enforce the advertised deadline.

Merge-risk options

Maintainer options:

  1. Preserve the existing default and repair the boundary (recommended)
    Keep timeout delegation as the default, make the cap policy explicit, fix all six findings, and add exact-head real-run proof for both adapters.
  2. Adopt the documented bounded default
    Approve the user-visible timeout-policy change only after setting a positive source default, enforcing it in the shared supervisor, and proving fresh and upgraded setups.
  3. Pause the timeout-policy change
    Keep the PR open until maintainers decide whether the new global timeout contract belongs in this runner-agnostic core.

Technical review

Best possible solution:

Adopt one explicit timeout contract that preserves the existing delegated default unless maintainers intentionally opt into a bounded policy, enforce any bounded deadline at the shared process-group boundary for both adapters, validate every text ingress before reading or writing durable artifacts, restore supported configuration documentation, and attach redacted real-run traces.

Do we have a high-confidence way to reproduce the issue?

Yes for the remaining source-level review findings: the current head shows exact uncapped ingress, post-write rejection, metadata-only timeout, zero-default schema mismatch, and documentation regression paths. No live configured runner was executed in this read-only review.

Is this the best way to solve the issue?

No, not yet. The cap direction is useful, but the timeout ownership/default contract, complete ingress validation, schema-valid timeout handling, accurate documentation, and real configured-run proof still need alignment.

Full review comments:

  • [P2] Parse cap values as complete numeric tokens — src/index.ts:19-23
    envCap uses Number.parseInt, so a value such as 4096junk is accepted as 4096 even though the new documentation says non-numeric values fall back to the default. Parse the complete trimmed token and cover suffix, empty, negative, and fractional inputs.
    Confidence: 0.99
  • [P2] Apply the text cap before direct handoff assembly — src/index.ts:30-36
    The new assertToolText calls cover request_plan and the record actions, but start_implementation.planText, start_corrections.feedbackText, council correction feedback, and file-loaded text are consumed before this boundary. A caller can bypass the advertised tool-text limit or force an unbounded file read.
    Confidence: 0.98
  • [P2] Reject oversized handoffs before writing them — src/index.ts:216-223
    start_implementation and start_corrections write implementation_request.md or corrections_request.md before createImplementationRunnerSession performs the prompt check. An oversized request is rejected only after the large durable handoff and observer setup have already occurred.
    Confidence: 0.99
  • [P2] Enforce the timeout at the shared runner boundary — src/index.ts:212-215
    The new max is copied into request/status metadata and reaches Octopus as --timeout, but the portable command adapter gets only the request path and runner-supervisor.py launches without a deadline. A command run can therefore exceed the documented server-wide cap indefinitely.
    Confidence: 0.98
  • [P2] Emit a positive schema-valid timeout value — src/index.ts:212-215
    With DEVELOPMENT_CYCLE_DEFAULT_TIMEOUT_SECONDS still defaulting to 0, the new expression produces timeoutSeconds: 0 in every default request even though the request schema requires any present timeout to be an integer of at least 1; fractional per-call values remain fractional as well. Normalize to a positive integer or omit the field when policy is delegated, and align the documented default.
    Confidence: 0.99
  • [P3] Keep the supported Octopus retry setting documented — docs/configuration.md:27-28
    The patch removes DEVELOPMENT_CYCLE_LOOP_UNTIL_APPROVED from the configuration table, while src/config.ts still reads it and the Octopus adapter still exports LOOP_UNTIL_APPROVED. Restore the row so the supported compatibility setting remains discoverable.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning max; reviewed against fc524ecbbd45.

Labels

Label justifications:

  • P2: This is a normal-priority resource-hardening change with bounded but meaningful compatibility and runner-availability implications.
  • merge-risk: 🚨 compatibility: The branch changes timeout semantics and documentation while current main intentionally delegates a zero default and the request schema requires positive timeout values when present.
  • merge-risk: 🚨 availability: A long-running portable command can exceed the advertised cap because the shared supervisor has no deadline enforcement.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The external PR shows sandboxed npm ci/build/test output only; it has no inspectable after-fix configured command or Octopus run with observed prompt rejection before writes and timeout enforcement. Add redacted terminal/live output, logs, a recording, or a linked artifact, and refresh the PR body at the exact head. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Exact merge result: The PR head is a merge commit whose result differs from current main only in docs/configuration.md, src/index.ts, and tests/prompt-size-cap.test.mjs; the stale-base package, adapter, README, and plan-test changes are already in main or not part of the resulting delta. (src/index.ts:1, a9d42af8dd39)
  • Current-main necessity: Current main contains no MAX_PROMPT_BYTES, MAX_TOOL_TEXT_BYTES, or MAX_RUNNER_TIMEOUT_SECONDS implementation, so the central hardening is not already merged. (src/index.ts:18, fc524ecbbd45)
  • Cap parsing defect: The new envCap helper uses Number.parseInt, which accepts a suffix such as 4096junk instead of applying the documented non-numeric fallback. (src/index.ts:19, a9d42af8dd39)
  • Ingress and write ordering: start_implementation and start_corrections assemble and write durable handoff files before createImplementationRunnerSession performs the prompt check; direct plan/feedback and file-loaded text also bypass the new tool-text assertion before reading. (src/index.ts:216, a9d42af8dd39)
  • Timeout enforcement boundary: The branch only records the effective timeout and passes it to Octopus when positive; the command adapter receives only requestPath and runner-supervisor.py accepts only runner path and cwd, so no shared deadline terminates a long command run. (src/index.ts:212, a9d42af8dd39)
  • Timeout schema and default: Current config still defaults DEVELOPMENT_CYCLE_DEFAULT_TIMEOUT_SECONDS to 0, while the request schema permits timeoutSeconds only as an integer of at least 1; the new expression always includes the zero default and preserves fractional per-call values. (src/config.ts:148, a9d42af8dd39)

Likely related people:

  • Jhacarreiro: Merged history owns the timeout/retry contract, adapter boundary, packaged supervisor, and recent lifecycle changes around the reviewed paths. (role: current-main control-plane contributor; confidence: high; commits: d373f6265b75, 6c68783309f8, b8069b3968e6; files: src/index.ts, src/config.ts, src/adapters/implementation.ts)
  • Zauberio: Prior merged changes cover supervisor request bounds, process-group cleanup, path guards, and null-parameter handling; the submitted cap helper is also introduced in that history. (role: recent runner-path contributor; confidence: high; commits: 9c713f519984, 11d0fb48ffbe, f6564801aaff; files: src/index.ts, runner-supervisor.py, src/adapters/implementation.ts)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (3 earlier review cycles)
  • reviewed 2026-08-11T19:55:48.317Z sha 3d57a42 :: needs real behavior proof before merge. :: [P2] Keep configured caps finite | [P2] Reject non-finite timeout requests
  • reviewed 2026-08-14T00:02:32.680Z sha e19d248 :: needs real behavior proof before merge. :: [P2] Parse cap values as complete numbers | [P2] Apply the tool-text cap to direct handoffs | [P2] Check prompt size before writing handoff files | [P2] Enforce the documented timeout bound
  • reviewed 2026-08-19T22:33:55.415Z sha f656480 :: needs real behavior proof before merge. :: [P2] Parse cap values as complete numeric tokens | [P2] Apply the tool-text cap to direct handoffs | [P2] Reject oversized prompts before writing handoff files | [P2] Enforce the timeout in both adapter paths | [P2] Emit a schema-valid timeout value | [P3] Keep the existing Octopus retry setting documented

@Zauberio

Copy link
Copy Markdown
Contributor Author

I'll start by reading the full request and the repo context so the reply addresses every review finding with verified evidence.I'll inspect the PR code and test names against the review findings so the reply only claims what the captured runs actually show.I'll pull the captured evidence, the PR head source, and a prior author-style reply so the comment stays accurate and consistent.Agreed on the remaining gaps — the captured suite is green, but it does not cover complete cap parsing, direct handoff text, pre-write prompt checks, or command-adapter timeout enforcement, and there is still no live-run artifact.

I’ll repair those paths, add focused tests, attach after-fix proof, and keep the timeout as a shared control-plane deadline.


e19d248 is not merge-ready. Captured npm test is 27/27 pass, but that suite does not cover the four remaining boundary gaps, and we still have no inspectable live-run artifact.

What I verified (captured at e19d24878f19)

  • npm ci — exit 0, 11s: added 311 packages in 10s
  • npm run build — exit 0, 8s: tsc -p tsconfig.json completed with no errors
  • npm test — exit 0, 9s: tsc + node --test tests/*.test.mjs
✔ overflowing MAX_PROMPT_BYTES env falls back to a finite 2 MiB cap
✔ non-finite timeoutSeconds is rejected with timeout_not_finite
✔ tool-text cap rejects oversized request_plan direction
✔ request_plan still succeeds with default caps
ℹ tests 27
ℹ pass 27
ℹ fail 0

That is plugin-test proof only. It is not a live runner transcript, and it does not exercise the four remaining ingress/timeout contracts.

Decision needed — timeout ownership

Agree: enforce the shared deadline. DEVELOPMENT_CYCLE_MAX_RUNNER_TIMEOUT_SECONDS should stay a server-wide upper bound. I will make the control-plane supervisor honor request.timeoutSeconds for both the default command adapter and Octopus, rather than narrowing the docs.

Findings / before-merge items

Add real behavior proof

Not verified yet. Captured evidence is npm ci / npm run build / npm test only. There is no after-fix live-run output, log, recording, or linked artifact showing oversized-input rejection or timeout clamping on a real configured runner. I will attach a redacted transcript covering those two behaviors, update the PR body, then request @clawsweeper re-review.

[P2] Parse cap values as complete numbers (src/index.ts:20-22)

Confirmed. envCap still uses Number.parseInt, so a value like 4096junk is accepted as 4096 instead of falling back as the docs claim. Captured coverage only shows overflow fallback ("9".repeat(400) → finite 2 MiB cap), not a complete-token parse. I will parse the whole string (reject any leftover suffix), fall back on non-numeric input, and add a 4096junk / empty / negative case.

[P2] Apply the tool-text cap to direct handoffs (src/index.ts:29-35)

Confirmed. Captured test tool-text cap rejects oversized request_plan direction only covers request_plan. Direct start_implementation.planText and correction feedback still bypass assertToolText. I will validate every ingress path before assembling request artifacts, including start_implementation and start_corrections.

[P2] Check prompt size before writing handoff files (src/index.ts:178-184) / merge risk P1

Confirmed. The oversized start_implementation path in the captured suite asserts prompt_too_large after the helper runs; it does not prove implementation_request.md / corrections_request.md / council_corrections_request.md were never written. The check still happens after those writes. I will move the size check ahead of the writes and add a regression that an over-limit call leaves no large durable file.

[P2] Enforce the documented timeout bound (docs/configuration.md:64) / merge risk P2

Confirmed — not verified at runtime. Captured coverage is only non-finite timeoutSeconds is rejected with timeout_not_finite (Infinitytimeout_not_finite). That does not show the command adapter or runner-supervisor.py enforcing request.timeoutSeconds. Octopus still gets --timeout; the command path does not. I will enforce the effective timeout in the supervisor for both adapters and add tests for both launch paths. No live clamp was captured.

Resolve merge risk (P2) — tool-text + partial numeric env

Same two gaps as above. Confirmed; will fix both.

Resolve merge risk (P2) — no inspectable after-fix real-run

Same as the proof item. Confirmed absent.

Complete next step (P2)

Agreed — keep this PR open. I will repair the four boundary findings, add the missing tests, post the redacted live-run proof, then ask for re-review. No merge until that lands.


PR Army reply · evidence from sandboxed execution (bwrap, netless) — commands and outputs above are real captured runs

Clamp per-call timeoutSeconds to DEVELOPMENT_CYCLE_MAX_RUNNER_TIMEOUT_SECONDS
(24h, envCap floor 60s) and reject non-finite timeoutSeconds with
timeout_not_finite. Reject prompts over DEVELOPMENT_CYCLE_MAX_PROMPT_BYTES
(2 MiB, envCap floor 1 KiB) with prompt_too_large. Also cap tool text
payloads (direction/plan/delivery/validation) via MAX_TOOL_TEXT_BYTES
(2 MiB). Includes HEAD supervisor cleanup (spawn+terminateGroup) from
origin/main fc2c52a.

Rebased onto origin/main fc2c52a (was e19d248 on base 54a104).
@Zauberio
Zauberio force-pushed the fix/prompt-size-cap branch from e19d248 to f656480 Compare August 19, 2026 21:41
@Zauberio

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review — sandboxed bwrap evidence refreshed for head f6564801aaff and PR body updated with Validation section.

Evidence: ~/pr-army/evidence/Jhacarreiro__openclaw-development-cycle-17-f6564801aaff.txt — bwrap as nobody, scrubbed env, no network for build/test. See PR body Validation section for phase exits.

@Zauberio

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review — head 1e68038e9e7d now contains current main (cd08e3047077) (base-gate refresh merge). Bwrap evidence refreshed at this exact head: npm ci (net, cached) exit 0; npm run build (netless) exit 0; npm test (netless) exit 0. Full capture: pr-army/evidence/Jhacarreiro__openclaw-development-cycle-17-1e68038e9e7d.txt.

@Jhacarreiro

Copy link
Copy Markdown
Owner

@clawsweeper re-review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants