Skip to content

Reject signed Retry-After delays in the CLI - #2939

Merged
freeqaz merged 1 commit into
mainfrom
fix/cli-retry-after-decimal-only
Oct 19, 2026
Merged

freeqaz merged 1 commit into
mainfrom
fix/cli-retry-after-decimal-only

Conversation

@freeqaz-openai

Copy link
Copy Markdown
Collaborator

With Retry-After: +3600, following Agent logs could pause for an hour instead of falling back to the normal two-second polling interval. HTTP delay-seconds are decimal digits with no sign. The CLI now parses that form as unsigned; valid values such as 05 and 3600, the one-hour limit, and HTTP-date handling are unchanged.

The regression test exercises the log-follow command against a local HTTP server and records the requested wait without sleeping. Before the fix it recorded 1h0m0s; after, 2s. All 129 CLI tests and subtests passed.

@freeqaz-openai
freeqaz-openai requested a review from a team as a code owner October 19, 2026 13:27
@freeqaz-openai freeqaz-openai added the agent-created PR opened by an AI agent; does not identify the reviewer or approver. label Oct 19, 2026
@clawsweeper

clawsweeper Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 19, 2026
@clawsweeper

clawsweeper Bot commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

The shared CLI client rejects signed Retry-After seconds while preserving valid seconds, HTTP dates, and the existing one-hour bound.

Example: An HTTP 429 response to occ agent logs --follow contains Retry-After: +3600.

  • Before: The CLI selects a one-hour wait.
  • After: The CLI ignores the malformed delay and selects its normal two-second interval.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) A focused, source-supported repair with useful HTTP-backed command coverage; confidence remains bounded by harness-only behavioral evidence.
Proof confidence 🌊 off-meta tidepool Not applicable: This collaborator-authored PR is outside the ordinary contributor proof gate; its supplied command-harness comparison exercises the real HTTP client with recorded waits, while the earlier date-fix binary proof covers a different scenario.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: A malformed signed Retry-After header can pause followed Agent logs for an hour.
Reason: This restores the existing invalid-header fallback through a minimal shared-owner repair, and collaborator authorship supplies owner direction.

Merge readiness

✅ Ready for maintainer review

This PR fixes a source-confirmed parsing bug with a minimal change and meaningful command-level coverage; no introduced defect was found.

Priority: P2
Reviewed head: e5da3466aee194fa6b9a1c8ad39b88ff0da574d0

Before merge

None.

Findings

None.

Tests

  • Missing end-to-end proof: The supplied comparison observes selected waits through the command harness; it does not demonstrate elapsed retry timing through the compiled CLI.
Agent review details

How this fits together

The OCC client's HTTP transport converts error-response headers into APIError metadata, which the Agent log-follow command consumes to choose its next polling delay.

flowchart LR
  A[Agent log-follow command] --> B[Shared OCC HTTP client]
  B --> C[HTTP error response]
  C --> D[Retry-After parser]
  D --> E[APIError delay]
  E --> F[Polling wait]
Loading

Technical review

Best possible solution:

Keep the unsigned parsing change in the existing shared transport owner and retain the command-level regression cases.

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

Main parses +3600 as a positive integer, and log-follow takes the maximum of that delay and its two-second interval; the supplied regression comparison reports this exact failure without a reviewer-run reproduction.

Is this the best way to solve the issue?

The one-line parser replacement fixes the shared cause without adding a parallel path, changing valid header handling, or expanding configuration.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 61ec42252eb8.

Provenance checked

  • Retry-After seconds parser keeps the original intent (refactor(cli): separate OCC transport from resource methods #1104: The transport refactor carried the existing integer parser unchanged; it does not establish the parser's original introduction.)
  • HTTP-date fallback and command coverage keeps the original intent (e01a939: Honor supported future HTTP dates while retaining invalid-header fallback and the one-hour bound.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

  • Minimal introduced fix: The production change replaces Atoi with base-10 ParseUint; positive and one-hour checks and the HTTP-date fallback remain unchanged. (internal/occclient/transport.go:278, e5da3466aee1)
  • Observable command behavior: The existing HTTP-backed command test adds signed-positive rejection, signed-negative rejection, leading-zero acceptance, and upper-bound controls. Its existing sleep replacement observes the selected delay without waiting. (internal/occcli/retry_after_test.go:56, e5da3466aee1)
  • Prior behavior and ownership: fix(cli): honor HTTP-date Retry-After delays #2480 introduced HTTP-date support and deliberately preserved invalid-header fallback and the existing delay bound; this PR respects those choices. refactor(cli): separate OCC transport from resource methods #1104 moved the parser without changing its behavior. (internal/occclient/transport.go:280, e01a93962130)
  • Discussion and owner direction: The full current body reports a before/after command-harness comparison of one hour versus two seconds; comments contain only review-status messages. The author is a collaborator, so direction is already owner-approved and closing is prohibited. (e5da3466aee1)
  • Current validation state: CI Required passed for the reviewed head; First Agent Smoke remains in progress, with no failed check reported. (e5da3466aee1)
  • Review boundaries: Read the complete root policy; no nested internal AGENTS.md, maintainer-notes directory, or pr-simplify skill was present. Commit-tree history and targeted blame supported ownership, although content-history searches encountered unavailable historical blobs. No tests or review helpers ran, and the checkout remained clean. (AGENTS.md, e5da3466aee1)

Likely related people:

  • Free Wortley: Raw commit 846c86a adds internal/occclient/transport.go:272 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 846c86a3a81b; files: internal/occclient/transport.go)
  • Eric Cai: Raw commit e01a939 adds internal/occcli/retry_after_test.go:52 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: e01a93962130; files: internal/occcli/retry_after_test.go)
  • Steven Lee: Raw commit 92538a1 adds internal/occclient/transport.go:248 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: 92538a1995de; files: internal/occclient/transport.go)

Labels

Label changes:

  • add P2: Malformed signed response headers can stall advanced CLI log-follow polling, with limited blast radius and no demonstrated urgent regression.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Label justifications:

  • P2: Malformed signed response headers can stall advanced CLI log-follow polling, with limited blast radius and no demonstrated urgent regression.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 19, 2026, 9:39 AM ET / 13:39 UTC.

@freeqaz-openai

Copy link
Copy Markdown
Collaborator Author

Free’s clanker here. I checked the review’s remaining proof question with the compiled CLI and its real timer, comparing parent 61ec4225 with this PR’s e5da3466. A loopback HTTP server returned 429, then a unique log line on the next request. Times below are measured at the server, not inferred from the notice.

Retry-After Parent This PR
+4 (malformed) Retried after 4.005 s Retried after 2.002 s
4 (valid control) Retried after 4.004 s Retried after 4.004 s
+3600 (reported case) Printed retrying in 1h0m0s; no second request during a 5 s observation Retried after 2.003 s and printed the log line

I did not wait an hour or replace the sleep function. This was a local HTTP fixture with a dummy key, not a production service. CI Required and First Agent Smoke also passed on this head. No source changed after review; maintainer review is still required.

@freeqaz
freeqaz merged commit 13be531 into main Oct 19, 2026
84 checks passed
@freeqaz
freeqaz deleted the fix/cli-retry-after-decimal-only branch October 19, 2026 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-created PR opened by an AI agent; does not identify the reviewer or approver. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants