Skip to content

fix(workbuddy): keep prompt hooks from stalling on scope reads - #1729

Merged
Teingi merged 2 commits into
oceanbase:masterfrom
AlexStocks:codex/workbuddy-scope-read-deadline
Sep 23, 2026
Merged

Teingi merged 2 commits into
oceanbase:masterfrom
AlexStocks:codex/workbuddy-scope-read-deadline

Conversation

@AlexStocks

@AlexStocks AlexStocks commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Which issue or RFC does this PR close?

N/A - follow-up from local WorkBuddy hook timeout diagnosis.

Rationale for this change

The WorkBuddy scope client bounded the size of a response body but not the time it could take, and only one of the hook's four read paths was covered. Two gaps remain in the first revision of this PR:

  1. The deadline was not enforced inside a single read call. HTTPResponse.read(n) delegates to io.BufferedReader.read(n), which keeps issuing underlying raw reads until it has n bytes or hits EOF. Recomputing the socket timeout once per 64 KiB block read therefore cannot stop a server that trickles data faster than that timeout: the caller stays inside one read() call and the absolute deadline is never re-evaluated. Measured against a loopback server dripping one byte per 50 ms with a 0.3 s per-request timeout, the first revision aborted after 3.19 s (body drip) and 3.17 s (header drip) instead of 0.3 s, i.e. it was bounded by the drip length, not by the deadline. With production defaults the same shape of server reaches the host's UserPromptSubmit timeout — the Hook timed out after 30000ms symptom this PR set out to remove.

    Note on the original description: a body read that stalls was already bounded (urllib applies its timeout to each individual socket read, so a hard stall trips it), so "response body reads were not bounded" overstated the gap. The genuinely unbounded case is a trickling response, at the body and at the header exchange, since the socket is unreachable until open() returns.

  2. Only resolve_scope_id was covered. The hook's own recall, capture, and flush reads (workbuddy_powercontext_hook._read_response, used by /v1/context/prepare, /v1/sources/content, and /v1/memory/flush) still used the pre-feat(integrations): show token savings in Claude Code, OpenCode, and Codex #1580 pattern, so the PR title's promise ("keep prompt hooks from stalling") applied to one of four read paths.

The primitives needed to close both gaps already exist in this repository. claude-code and codex introduced open_bounded + _DeadlineSocket/bind_response_deadline in #1580; _read_bounded in the codex plugin even has a test asserting the drip case today. This PR reuses them instead of inventing a workbuddy-specific variant.

What changes are included in this PR?

  • integrations/workbuddy/plugins/powercontext/scripts/workspace_scope.py
    • Add open_bounded, which runs one request in a daemon worker so connect and the header exchange stay inside the caller's budget.
    • Add _DeadlineSocket + bind_response_deadline, which recompute the remaining time before every recv/recv_into, framing included.
    • Call bind_response_deadline at the top of _read_bounded and keep the 64 KiB block reads (block size and deadline enforcement are orthogonal; one byte per call would cap a 1 MiB body at roughly 0.4 MB/s).
    • Read the socket in _set_response_timeout through a guarded getattr chain, and drop the fp member the first revision added to the _Response protocol.
  • integrations/workbuddy/plugins/powercontext/hooks/powercontext_scope_binding.py
    • Forward bind_response_deadline and open_bounded next to resolve_scope_id. The installer replaces this file with a full copy of scripts/workspace_scope.py, so both layouts (source tree shim and installed copy) expose the same names to the hook.
  • integrations/workbuddy/plugins/powercontext/hooks/workbuddy_powercontext_hook.py
    • Extend the import guard to the two new names, bind the deadline in _read_response, and route _post_json through open_bounded.
    • Drop the hook-local _RejectRedirects/_URL_OPENER and the now-unused override shim: the bounded opener is shared from the scope module, which keeps the redirect refusal the hook relies on.
    • _set_response_timeout uses the guarded getattr chain, so a missing fp can no longer raise AttributeError outside the hook's error contract.
  • tests/workbuddy_plugin/
    • Add __init__.py, matching the tests/codex_plugin and tests/claude_code_plugin layout.
    • Replace the fake-socket test with real loopback property tests that assert wall-clock behavior through the public resolve_scope_id: body drip, header drip, a large healthy response inside its budget, and an expired deadline that must not contact the server.
    • Add a hook test asserting a trickling /v1/context/prepare keeps main() inside the shared HTTP budget, plus a redirect-refusal guard for the shared opener.

Are there any user-facing changes?

Yes. A WorkBuddy prompt hook now fails open inside its configured HTTP budget when the PowerContext Server trickles response headers or a response body, instead of waiting for WorkBuddy's outer UserPromptSubmit timeout. Healthy responses are unaffected, and a slow but healthy response is still allowed to finish inside its budget (covered by a test).

No public API, persisted format, or configuration semantics change. The behaviour is now identical to the claude-code and codex integrations.

Known residual, unchanged from the other hosts: bind_response_deadline and _set_response_timeout reach the socket through CPython's response.fp.raw._sock chain. If a future transport drops a link in that chain the deadline degrades to the per-socket-op timeout instead of failing loudly; that case is not observable in the loopback tests and is left as parity with the existing hosts.

How was this change tested?

Red/green, with the implementation reverted to the first revision of this PR and the new tests left in place (git apply of the fix patch afterwards):

uv run pytest tests/workbuddy_plugin -q      # 3 failed, 2 passed on the first revision
uv run pytest tests/workbuddy_plugin -q      # 6 passed with the fix
tests/workbuddy_plugin/test_scope_binding.py:91   assert 3.19 < 1.0   (body drip)
tests/workbuddy_plugin/test_scope_binding.py:132  assert 3.17 < 1.0   (header drip)
tests/workbuddy_plugin/test_hook.py:107           assert 3.20 < 1.5   (hook recall read)

The two tests that pass in both revisions are the reverse guards (a 1 MB healthy response must still succeed; an expired deadline must not issue a request) and are documented as such.

Commands run on this branch:

  • uv run pytest tests/workbuddy_plugin tests/test_cli_workbuddy.py tests/test_integration_manifest.py tests/test_doctor_integrations.py tests/codex_plugin tests/claude_code_plugin -q → 350 passed, 4 skipped
  • uv run ruff check . / uv run ruff format --check . → clean, 844 files formatted
  • uv run ty check → no diagnostics in the changed files
  • uv lock --check → resolved
  • Installed-layout smoke check: with scripts/workspace_scope.py copied to powercontext_scope_binding.py and no workspace_scope.py present (what the installer produces), the hook imports bind_response_deadline/open_bounded from powercontext_scope_binding.

Not covered here: a cross-host contract test that asserts the same drip property for the claude-code and codex read paths. Their scope modules still read one byte per call, which caps a large body at roughly 0.4 MB/s; that is a separate, low-priority alignment.

AI usage statement

The initial revision and its diagnosis were prepared by Codex with GPT-5. The follow-up commit was prepared by WorkBuddy (Claude) after an independent review of the first revision: it reproduced the remaining drip case with real loopback servers, ported the existing open_bounded/_DeadlineSocket primitives instead of the workbuddy-specific variant, extended coverage to the hook's own read path, replaced the mock-based test with wall-clock property tests, and verified the red/green transition reported above. A human reviewed the diff and the test evidence.

WorkBuddy prompt submission should fail open within the scope binding HTTP budget even when a server response stalls after headers arrive.

Constraint: WorkBuddy enforces an outer UserPromptSubmit hook timeout

Scope-risk: narrow

Tested: uv run pytest tests/workbuddy_plugin/test_scope_binding.py -q

Tested: uv run ruff check integrations/workbuddy/plugins/powercontext/scripts/workspace_scope.py tests/workbuddy_plugin/conftest.py tests/workbuddy_plugin/test_scope_binding.py

Tested: uv run ruff format --check integrations/workbuddy/plugins/powercontext/scripts/workspace_scope.py tests/workbuddy_plugin/conftest.py tests/workbuddy_plugin/test_scope_binding.py

Co-authored-by: OmX <omx@oh-my-codex.dev>
f42a2bf added an absolute deadline around the scope resolver's response body,
but it still tightened urllib's socket timeout only once per 64 KiB block read.
http.client consumes many socket reads inside a single read call, so a server
that trickles a byte faster than that timeout keeps the caller inside one read
and past its deadline; the observed worst case is the host's 30s
UserPromptSubmit timeout, which is exactly what the change set out to remove.

Reuse the primitives the claude-code and codex scope modules already ship:

- open_bounded runs the request in a daemon worker, so connect and header
  exchange stay inside the caller's budget.
- bind_response_deadline recomputes the remaining time before every receive,
  framing included.

Both now cover the scope resolver and the hook's own recall, capture, and flush
reads. The hook shares them through the installed powercontext_scope_binding
shim instead of carrying its own opener, so the shim forwards them too.
_set_response_timeout reads the socket through a guarded getattr chain, so a
missing fp can no longer raise AttributeError outside the scope client's error
contract.

The deadline test replaces its fake socket with real loopback properties: body
drip, header drip, a large healthy response, an expired deadline, and the hook's
own recall read. The three drip cases fail on f42a2bf (3.19s, 3.17s, 3.20s
against a 1.0s assertion) and pass here.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Response reads can still outlast the request deadline, and the regression test does not exercise multiple socket receives.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
What changed in this PR

Updates WorkBuddy scope binding to bound response-body reads within the configured HTTP deadline.

Changes:

  • Applies socket timeouts during response reads.
  • Adds test fixtures and stalled-response regression coverage.
File Summary
tests/​workbuddy_plugin/​test_scope_binding.py Tests stalled response handling.
tests/​workbuddy_plugin/​conftest.py Loads the WorkBuddy scope module for tests.
integrations/​workbuddy/​plugins/​powercontext/​scripts/​workspace_scope.py Applies request deadlines while reading responses.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +53 to +55
def read(self, amount: int = -1) -> bytes:
assert self.sock.timeouts
raise TimeoutError
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants