perf(ci): add a no-marker fast path to extract_model_prose - #1811
Merged
Conversation
Salvaged from .github#1416 ("Bolt: extract_model_prose 빠른 반환 경로
추가"), whose branch bundled three changes against a stale base:
- extract_model_prose's fast path: still valid, applied here.
- max_tokens: REVIEW_MAX_OUTPUT_TOKENS -> 16 in the preflight request:
superseded by ADR-0005's own escalation design already on main
(REVIEW_PREFLIGHT_BASE_TOKENS = 16, escalating to
REVIEW_PREFLIGHT_ESCALATED_TOKENS only when a response is truncated)
-- not carried over.
- curl --max-time 30 -> 60 in the sidecar preflight: superseded by a
later main-side change that removed the timeout entirely, per
ADR-0003 ("model inference forbids a wall-clock timeout"), replacing
it with a bounded-attempt retry loop instead -- carrying this over
would reintroduce exactly what ADR-0003 forbids.
For the one idea that's still current: most model responses contain
neither SENTINEL_PREFIX nor CONTROL_START, so scanning every line for
both prefixes is wasted work. Bolt's own version of this fast path
(`return raw_output.strip()`) wasn't quite behavior-preserving --
splitlines()/join("\n") normalizes CRLF and other exotic line
separators the plain .strip() path leaves untouched, so a raw model
response using non-LF line endings would come back different from
before. Fixed to `"\n".join(raw_output.splitlines()).strip()`, which
is byte-for-byte identical to the slow path's output while still
skipping its per-line prefix-matching loop.
.github#1416 will be closed as fully addressed once this merges.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 1 minute. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
seonghobae
added a commit
that referenced
this pull request
Sep 3, 2026
As discussed in PR comments, this entire PR was closed as obsolete. The valid parts (`extract_model_prose` fast path) have already been merged into `main` via #1811. The other ideas (`max_tokens=16`, `curl --max-time 60`) have been superseded by `main`'s adherence to ADR-0005 and ADR-0003, making this implementation redundant or regressive. This commit acknowledges that feedback and stops further work on this branch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.github#1416("⚡ Bolt: extract_model_prose 빠른 반환 경로 추가") bundled three changes, but its branch was forked before two significant redesigns landed onmain. Checked each of the three against currentmainbefore deciding what to carry over:extract_model_prose's fast path — still valid, applied here.max_tokens: REVIEW_MAX_OUTPUT_TOKENS -> 16in the preflight request — superseded.mainalready hasADR-0005's own escalation design (REVIEW_PREFLIGHT_BASE_TOKENS = 16, escalating toREVIEW_PREFLIGHT_ESCALATED_TOKENS— i.e.REVIEW_MAX_OUTPUT_TOKENS— only when a response comes back truncated). Not carried over; Bolt's blunt always-16 version would have been a regression against a smarter mechanism already in place.curl --max-time 30 -> 60in the sidecar preflight — superseded. A latermain-side change removed the timeout from that curl call entirely, with an explicit comment citingADR-0003("model inference forbids a wall-clock timeout"), replacing it with a bounded-attempt retry loop. Carrying this over would reintroduce exactly what that ADR forbids. Not carried over.For the one idea that's still current: most model responses contain neither
SENTINEL_PREFIXnorCONTROL_START, so scanning every line for both prefixes on every review is wasted work when neither is present at all. Bolt's own version of the fast path (return raw_output.strip()) wasn't quite behavior-preserving, though:str.splitlines()/"\n".join(...)normalizes CRLF and a handful of other line-separator characters that a plain.strip()leaves untouched, so a raw model response using non-LF line endings would come back subtly different from the un-optimized path. Fixed to"\n".join(raw_output.splitlines()).strip(), which is byte-for-byte identical to the slow path's output on any input, while still skipping its per-line prefix-matching loop for the common case. Added a test proving the two paths agree exactly, including on a CRLF input..github#1416will be closed as fully addressed (its one still-current idea absorbed here, its other two explained as superseded) once this merges.Test plan
coverage run -m pytest tests→ 2772 passed, 1 skippedcoverage report→ 100%interrogate→ 100%🤖 Generated with Claude Code