Fix: config persistence, stale footer, and REPL rendering/timing#2806
Fix: config persistence, stale footer, and REPL rendering/timing#2806aniruddhaadak80 wants to merge 2 commits into
Conversation
Greptile code reviewThis repo uses Greptile for automated review. Before merge, aim for Confidence Score: 5/5 with zero unresolved review threads — see CONTRIBUTING.md. Run a review — add a PR comment with: Give it ~5-10 minutes (sometimes longer) for results, then fix feedback and re-trigger until you reach Confidence Score: 5/5. Optional: automate with the greploop skill. |
Greptile SummaryThis PR fixes four independent issues: config path resolution under PyInstaller, a stale Rich progress footer, and several REPL rendering/timing problems around confirmation dialogs and spinner elapsed time.
Confidence Score: 3/5The config and spinner-timing changes are safe, but the dispatch routing refactor has a likely regression that could corrupt user input. The simplification of dispatch_needs_exclusive_stdin groups /watches with genuinely async background commands and returns False, removing the guard specifically called out in comments as preventing CPR byte leakage into the next prompt buffer. Every other change in the PR is well-scoped and low-risk. app/cli/interactive_shell/runtime/dispatch.py — the /watches classification and the catch-all True logic need a second look before merging. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant RunLoop as run_interactive
participant Dispatch as dispatch.py
participant State as ReplState
participant Spinner as SpinnerState
participant Prompt as prompt_surface.py
User->>RunLoop: submit text
RunLoop->>State: reset total_confirm_duration and confirm_start_time
RunLoop->>Dispatch: dispatch_needs_exclusive_stdin
Note over Dispatch: new_alert route True, watch/unwatch False, all else True
RunLoop->>Dispatch: dispatch_one_turn with confirm_fn
alt confirmation needed
Dispatch->>State: begin_confirmation and record confirm_start_time
Dispatch->>Prompt: set _is_awaiting_confirm True on app
Prompt-->>User: placeholder hidden
Spinner-->>User: spinner row hidden during confirm
User->>Dispatch: deliver answer
Dispatch->>State: accumulate total_confirm_duration and clear start_time
Dispatch->>Prompt: clear _is_awaiting_confirm on app
end
Spinner->>State: inline_spinner_ansi reads accumulated confirm duration
Note over Spinner: elapsed excludes user wait time
Spinner-->>User: shows LLM processing time only
Reviews (1): Last reviewed commit: "Fix: config persistence, stale footer, a..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # /watch commands run asynchronously in the background, so they don't need exclusive stdin | ||
| if name in {"/watch", "/watches", "/unwatch"}: | ||
| return False |
There was a problem hiding this comment.
/watches incorrectly removed from exclusive-stdin guard
/watches (list active watches) was previously in _EXCLUSIVE_STDIN_MENU_COMMANDS under the explicit comment "Table-outputting commands: must complete before the next prompt_async() starts, otherwise patch_stdout redraws trigger ESC[6n DSR queries whose CPR responses land as literal keystrokes in the incoming prompt buffer." The new code lumps it with /watch and /unwatch — which genuinely start background tasks — and returns False, removing that protection. Running /watches after a streaming investigation will now allow prompt_async() to start before the table finishes rendering, which can corrupt the next input buffer with stale CPR bytes — exactly the race the original comment described.
| # /tests run/synthetic/cloudopsbench are run as background tasks, others are synchronous | ||
| if name == "/tests": | ||
| _BACKGROUND_TEST_SUBCOMMANDS = {"run", "synthetic", "cloudopsbench"} | ||
| return not (args and args[0] in _BACKGROUND_TEST_SUBCOMMANDS) |
There was a problem hiding this comment.
_BACKGROUND_TEST_SUBCOMMANDS is a set literal allocated on every call to dispatch_needs_exclusive_stdin. It should be a module-level constant like the existing frozen-set siblings above it.
| # /tests run/synthetic/cloudopsbench are run as background tasks, others are synchronous | |
| if name == "/tests": | |
| _BACKGROUND_TEST_SUBCOMMANDS = {"run", "synthetic", "cloudopsbench"} | |
| return not (args and args[0] in _BACKGROUND_TEST_SUBCOMMANDS) | |
| # /tests run/synthetic/cloudopsbench are run as background tasks, others are synchronous | |
| if name == "/tests": | |
| return not (args and args[0] in _BACKGROUND_TEST_SUBCOMMANDS) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| app = get_app_or_none() | ||
| if app is not None: | ||
| setattr(app, "_is_awaiting_confirm", True) # noqa: B010 |
There was a problem hiding this comment.
Application object
setattr(app, "_is_awaiting_confirm", True) attaches a custom attribute directly to a prompt_toolkit Application instance. While getattr(..., False) in prompt_surface.py provides a safe read-side default, naming it with a leading underscore implies it is an internal prompt_toolkit attribute and risks a future collision if prompt_toolkit adds its own _is_awaiting_confirm. Exposing the flag through the ReplState object (already threaded through the same call site) would avoid touching the third-party instance entirely.
…greSQL blocking queries, unified /agents commands & autocomplete, and EmbeddingsClient
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| val = int(arg) | ||
| if val > 0: | ||
| is_positive_int = True | ||
| except ValueError: |
| @property | ||
| def model_name(self) -> str: | ||
| """The model name used for embeddings.""" | ||
| ... |
| @property | ||
| def dim(self) -> int: | ||
| """The dimensionality of the embedding vectors.""" | ||
| ... |
|
|
||
| def embed(self, texts: list[str]) -> list[list[float]]: | ||
| """Generate embeddings for the given list of texts.""" | ||
| ... |
Fixes #2058, #2117, #2230, #2116, #2115
This PR addresses the first batch of issues: