Validate channels status timeout values

[roytong9]

Summary

Validate channels status --timeout values using the shared CLI timeout parser.

What changed

• Switched channels status from raw Number(opts.timeout ?? 10_000) parsing to parseTimeoutMsWithFallback(...)
• Added a focused test covering invalid timeout input

Why

channels status was parsing --timeout with a raw Number(...) conversion, unlike adjacent commands that already use the shared timeout parsing helper.

That made invalid inputs less consistently validated. This change brings channels status in line with the existing CLI timeout behavior and ensures invalid values such as 0 fail clearly.

Manual verification

1. Added a focused test in src/commands/channels.status.command-flow.test.ts
2. Updated src/commands/channels/status.ts to use parseTimeoutMsWithFallback(..., { invalidType: "error" })
3. Ran:
   • pnpm vitest run src/commands/channels.status.command-flow.test.ts -t "throws for invalid timeout values"
   • pnpm vitest run src/commands/channels.status.command-flow.test.ts
4. Ran the repo checks during commit

[greptile-apps]

Greptile Summary

Replaces the raw Number(opts.timeout ?? 10_000) call in channelsStatusCommand with parseTimeoutMsWithFallback(..., { invalidType: "error" }), aligning it with adjacent commands and ensuring zero/invalid values are rejected with a clear error. A focused test is added to verify the new validation path.

Confidence Score: 5/5

• Safe to merge — change is minimal, correct, and well-tested.
• The only finding is a P2 style note about test placement; no logic, correctness, or behavioral issues were identified. The helper function correctly rejects <= 0 values and falls back to 10 000 ms for missing input, preserving prior valid behavior while fixing silent acceptance of bad inputs.
• No files require special attention.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: src/commands/channels.status.command-flow.test.ts
Line: 175-181

Comment:
**Test placed in semantically unrelated describe block**

This timeout validation test lives inside `channelsStatusCommand SecretRef fallback flow`, but it has nothing to do with SecretRef resolution. It happens to work fine because `parseTimeoutMsWithFallback` throws before any mocked gateway/config calls, but a reader scanning for timeout-related tests won't find it here. Consider moving it to a more general or dedicated describe block (e.g., `channelsStatusCommand option validation`).

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "Validate channels status timeout values" | Re-trigger Greptile}

[roytong9]

The remaining failure appears unrelated to this change and is currently coming from src/channels/plugins/contracts/channel-import-guardrails.test.ts, specifically a matrix helper import guardrail around extensions/matrix/src/matrix/outbound-media-runtime.ts. Happy to wait for a rerun if helpful.

[vincentkoc]

ProjectClownfish pushed a narrow repair to this branch so the original contributor path can stay canonical.

Source PR: #59382
Validation: pnpm check:changed
Contributor credit is preserved in the branch history and PR context.

[clawsweeper]

Codex review: needs real behavior proof before merge.

Summary
The PR switches channels status --timeout from raw Number(...) parsing to the shared timeout parser and adds command-flow coverage for invalid timeout handling and the probe default.

Reproducibility: yes. by source inspection. Current main passes raw Number(opts.timeout...) into the channels.status gateway call, so --timeout 0 or a nonnumeric value is not rejected at the command boundary.

Real behavior proof
Needs real behavior proof before merge: Missing after-fix real CLI proof; the contributor should add redacted terminal output, logs, a screenshot/recording, or a linked artifact to the PR body for re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, ask a maintainer to comment @clawsweeper re-review.

Next step before merge
Manual follow-up is needed because external real behavior proof is missing and the branch needs a rebase plus assertion update before normal review can proceed.

Security
Cleared: The diff only changes CLI timeout parsing and colocated tests, with no dependency, workflow, package, secret, permission, or artifact-execution changes.

Review findings

• [P2] Match the timeout assertion to the parser message — src/commands/channels.status.command-flow.test.ts:318

Review details

Best possible solution:

Rebase the branch on current main, keep the command-boundary parser change, preserve channel filtering and probe defaults, assert the current parser message, and add redacted real CLI output proof.

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

Yes, by source inspection. Current main passes raw Number(opts.timeout...) into the channels.status gateway call, so --timeout 0 or a nonnumeric value is not rejected at the command boundary.

Is this the best way to solve the issue?

Mostly yes. Reusing parseTimeoutMsWithFallback(..., { invalidType: "error" }) is the narrow maintainable fix, but this branch needs a rebase, a current parser-message assertion, and contributor proof before merge.

Full review comments:

• [P2] Match the timeout assertion to the parser message — src/commands/channels.status.command-flow.test.ts:318
  After rebasing onto current main, parseTimeoutMsWithFallback throws Invalid --timeout... Received: "not-a-number", but this test expects the old lower-case text. Assert the current message or a stable substring such as Received: "not-a-number" so the command fix can pass on current main.
  Confidence: 0.91

Overall correctness: patch is incorrect
Overall confidence: 0.89

Acceptance criteria:

• node scripts/run-vitest.mjs src/commands/channels.status.command-flow.test.ts
• node scripts/run-vitest.mjs src/cli/parse-timeout.test.ts
• node scripts/crabbox-wrapper.mjs run ... --shell -- "pnpm check:changed"

What I checked:

• current_main_raw_timeout_parse: Current main still computes timeoutMs with Number(opts.timeout ?? (opts.probe ? 30_000 : 10_000)), so invalid command input is not rejected at the channels status command boundary before the gateway request is built. (src/commands/channels/status.ts:209, 3b7d01b63f02)
• current_shared_parser_contract: The shared parser now throws Invalid --timeout... Received: "<value>" for non-positive and nonnumeric values, which is the contract any rebased assertion should match. (src/cli/parse-timeout.ts:20, 3b7d01b63f02)
• parser_contract_test: Current parser tests assert the stable Received: "0" and Received: "-1" substrings for invalid values. (src/cli/parse-timeout.test.ts:39, 3b7d01b63f02)
• pr_head_uses_helper: The PR head uses parseTimeoutMsWithFallback(..., { invalidType: "error" }) and preserves the 10s default plus the 30s probe default through constants. (src/commands/channels/status.ts:173, 51504d9ea1ee)
• pr_head_assertion_mismatch_after_rebase: The PR head still asserts the older lower-case parser message invalid --timeout: not-a-number, which will not match current main after rebase. (src/commands/channels.status.command-flow.test.ts:318, 51504d9ea1ee)
• live_pr_state_and_proof: Live PR metadata reports the branch as CONFLICTING; the PR body and comments list tests/checks but no redacted terminal output, logs, screenshot, recording, or linked artifact showing after-fix real CLI behavior. (51504d9ea1ee)

Likely related people:

• steipete: GitHub commit history shows this account introduced parseTimeoutMsWithFallback and its tests, which define the helper this PR reuses. (role: shared parser feature introducer; confidence: high; commits: 4e055d8df2fb, cfeaa34c1623; files: src/cli/parse-timeout.ts, src/cli/parse-timeout.test.ts)
• vincentkoc: Current-main history updated the parser error examples, and the PR discussion/commits show this account pushed the narrow repair commits to the branch. (role: recent parser and branch repair contributor; confidence: medium; commits: 23d5e630cd9a, f35a3819f2ce, 51504d9ea1ee; files: src/cli/parse-timeout.ts, src/cli/parse-timeout.test.ts, src/commands/channels/status.ts)
• omarshahine: Recent current-main history added channels status --channel filtering through the same command and tests, which the rebased branch must preserve. (role: recent channels-status feature contributor; confidence: medium; commits: efc864139340; files: src/commands/channels/status.ts, src/commands/channels.status.command-flow.test.ts, src/cli/channels-cli.ts)
• shakkernerd: Recent current-main history tightened command-flow coverage for the channels.status timeout request shape. (role: recent channels-status test contributor; confidence: medium; commits: cbc620be377d; files: src/commands/channels.status.command-flow.test.ts)

Remaining risk / open question:

• The branch is merge-conflicting and must be rebased carefully to preserve current --channel filtering and the 30s probe default.
• No after-fix real CLI behavior proof has been posted; tests and CI are not sufficient for the external PR proof gate.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 3b7d01b63f02.