Fix transient thread title inference failures - #1330
Conversation
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this pull request for security, code quality, performance, architecture, and end-to-end behavior. |
1209b9e to
a10b91e
Compare
| "Thread metadata inference timed out; retrying", | ||
| "Thread metadata inference failed transiently; retrying", | ||
| ); | ||
| await delay(THREAD_METADATA_RETRY_DELAY_MS); |
There was a problem hiding this comment.
🚨 slopcop/review — The fixed delay can increase load during provider limits.
A 429 or 503 retry always waits 250 ms. The daemon does not pass Retry-After, so concurrent requests can retry too early.
Please preserve a bounded provider delay. Use a random delay when the provider gives no value.
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5
The app asks a helper to name each new thread. This change gives the helper more time and another try.
It is like one extra turn to name a toy. Some failure notes still make the next try late or unsuccessful.
Findings
- Medium: A
server_errorcode can disappear when its event also has a message. A neutral message then skips the service retry. - Medium: The reader continues after
response.failed. An open stream can consume the deadline and hide the retryable service error. - Medium: A 429 or 503 retry always waits 250 ms. This delay ignores
Retry-Afterand can align concurrent retries.
I left inline comments for these findings.
Snapshot note
The requested SHA used protocol 98. Current main also used protocol 98, so that snapshot could not force a daemon update.
The PR moved to a10b91eb during this review. That head uses protocol 99 and fixes the protocol issue.
The other feature files have the same patch on both heads.
Architecture and security
The server owns retry policy. The daemon owns provider requests, deadlines, and error translation. This boundary is correct.
I found no harmful duplicate logic. Three response-fetch interfaces still contain an unused command field and should remove it.
I found no new authorization bypass, credential leak, or injection path.
Validation
git diff --checkpassed.- Turbo type checks passed for the daemon, server, and contract.
- The contract passed 49 tests.
- The daemon passed 524 tests.
- The server passed 1,404 tests.
- A browser test does not apply because this change has no UI route.
The GPT-5.6 review gate found the three medium issues above.
3af711e to
88a3016
Compare
Summary\n\n- add BB_INFERENCE_FALLBACK as a live-reloadable bb-app config/environment setting, defaulting to codex/gpt-5.4-mini\n- use the configured primary model first and switch every thread-title path and commit-subject inference to the fallback only after timeouts, rate limits, or service-unavailable failures\n- apply the same two-attempt policy to asynchronous personal/reused-workspace titles without delaying thread startup\n- give Luna title inference a realistic 5-second attempt window and classify provider HTTP/stream failures as retryable\n- preserve structured Codex stream error codes even when a human-readable message is present\n- terminate and cancel SSE reads immediately when an error or response.failed event arrives instead of waiting for EOF\n- enforce one end-to-end daemon deadline across fetch and SSE reads, with host RPC delivery grace\n- bump the host-daemon protocol to 100 after rebasing over independent protocol changes on main\n\n## Validation\n\n- reproduced the missed personal-thread fallback from thr_fy866594ed: Luna returned codex_service_unavailable after 1.74s and the old async path made only one attempt\n- regression test drives that exact async lifecycle, verifies the fallback model is selected, applies the title after the thread becomes idle, and queues the provider rename\n- fresh personal-thread live probe on the restarted branch generated the title "Test title generation"\n- live configured fallback probe: Luna hit the forced transient timeout path; GPT-5.4 mini returned a structured title in 1.18s\n- affected Turbo typechecks pass for config, server, bb-app, templates, host daemon, host-daemon contract, and integration tests\n- full server suite passes: 160 files / 1,409 tests\n- full host-daemon suite passes: 45 files / 526 tests\n- 35-test host-daemon contract suite passes\n- generated guide templates are current and git diff --check passes