fix: Stabilize 0.2.3 UX Runtime Edge Cases#118
Merged
Conversation
📝 WalkthroughWalkthroughThis PR updates CLI logs-root default resolution and validation to support both repo-local and user-owned logs directories, adds file-open safety gates (binary/large-file detection) with sticky error status in the core editor, updates status bar precedence, and introduces a startup input-buffer guard in KeyBinder. Corresponding tests are added for each area. ChangesCLI Logs Root Resolution
File-Open Safety Gates and Sticky Status
Startup Paste Guard
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant KeyBinder
participant Ecli
participant DrawScreen
KeyBinder->>Ecli: get_key_input first call flushes buffer, returns curses.ERR
Ecli->>Ecli: open_file(path) checks size, binary sample
alt permission error
Ecli->>Ecli: _set_sticky_error_message()
end
Ecli->>Ecli: _handle_input_dispatch clears sticky status on key action
Ecli->>DrawScreen: draw status bar using _sticky_status, status_message, or "Ready"
Compact metadata:
A guard at the gate, a sticky note held, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@src/ecli/cli.py`:
- Around line 329-335: The _safe_is_relative_to helper in cli.py duplicates
stdlib behavior and should be removed in favor of Path.is_relative_to, which is
available for the project’s Python 3.11 target. Update the call sites that use
_safe_is_relative_to to call the built-in Path.is_relative_to directly, and
delete the helper plus its outdated docstring note about Python 3.9+. Ensure any
related path checks in the CLI code continue to behave the same after the
replacement.
In `@src/ecli/core/Ecli.py`:
- Around line 5363-5388: The nonexistent-path buffer branch in Ecli.open should
reset syntax highlighting state instead of inheriting the previous file’s lexer.
In the block that sets up the new clean buffer, clear self._lexer and then call
self.detect_language() after self.filename is assigned so the new buffer gets
the correct language/highlighting rules before returning.
- Around line 5363-5388: Reset the file-origin state when creating a fresh
buffer for a nonexistent path in Ecli.open_file: this branch currently
initializes text, filename, modified, and history but leaves
_file_loaded_from_disk and _file_had_final_newline unchanged from the prior
file. Update that buffer setup so those flags are explicitly reset to the
correct defaults for a new unsaved file, alongside the existing new_buffer
initialization and logging.
In `@src/ecli/ui/KeyBinder.py`:
- Around line 169-191: The startup drain loop in _flush_startup_input_buffer can
block the UI thread indefinitely because it reads until curses.ERR with no upper
bound. Add a small iteration/byte cap to the while loop, following the bounded
patterns already used by _read_bracketed_paste and _complete_trailing_utf8, and
stop draining once that cap is reached even if input keeps arriving. Keep the
fix scoped to KeyBinder._flush_startup_input_buffer and preserve the existing
nodelay reset and debug logging behavior.
- Around line 169-191: The startup buffer drain in _flush_startup_input_buffer
currently discards every queued input, which can lose a KEY_RESIZE event
arriving during startup. Update the loop in
KeyBinder._flush_startup_input_buffer so it preserves KEY_RESIZE by requeueing
it for later handling instead of counting it as discarded, while still draining
and dropping all other pre-existing buffered bytes.
In `@tests/cli/test_phase1_service_cli.py`:
- Around line 270-279: The test is recomputing the repository logs path from
Path.cwd() instead of using the same repo-root source as _effective_logs_root(),
which can diverge depending on the working directory. Update
tests/cli/test_phase1_service_cli.py to anchor repo_logs by importing and using
_repo_logs_root() (the same helper used by _effective_logs_root() in
src/ecli/cli.py) and keep the existing assertions against in_repo and in_user
unchanged.
In `@tests/ui/test_startup_paste_guard.py`:
- Around line 219-230: The test in
test_startup_guard_deactivates_after_first_flush is tautological because it
manually sets _startup_guard_active to False after calling
_flush_startup_input_buffer and then asserts the same value. Rewrite it to
validate the actual effect of _flush_startup_input_buffer on the keybinder state
using make_keybinder and the _startup_guard_active flag, or remove this test if
get_key_input already covers the guard deactivation contract.
- Around line 300-324: The final assertion in
test_get_key_input_processes_normal_key_after_guard_fires is tautological
because kb._startup_guard_active is already False, so rewrite it to verify the
actual returned key behavior from kb.get_key_input. Use the concrete result from
the second call to assert that it matches the expected normal input path (or the
specific burst-drained outcome if that is what the test is meant to cover), and
remove the always-true fallback. Keep the focus on the behavior exercised by
get_key_input and _drain_text_burst rather than rechecking the guard flag.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fa0f71dc-bc60-430c-bd2e-9334f1d4fced
📒 Files selected for processing (7)
src/ecli/cli.pysrc/ecli/core/Ecli.pysrc/ecli/ui/DrawScreen.pysrc/ecli/ui/KeyBinder.pytests/cli/test_phase1_service_cli.pytests/core/test_file_open_safety.pytests/ui/test_startup_paste_guard.py
Comment on lines
+329
to
+335
| def _safe_is_relative_to(path: Path, parent: Path) -> bool: | ||
| """Return True when ``path`` is relative to ``parent`` (Python 3.9+).""" | ||
| try: | ||
| path.relative_to(parent) | ||
| return True | ||
| except ValueError: | ||
| return False |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Prefer the built-in Path.is_relative_to over a hand-rolled helper.
The project targets Python ≥ 3.11, where Path.is_relative_to() is available natively (the new tests in this PR already call it directly). This helper duplicates stdlib behavior, and the docstring's "Python 3.9+" note is misleading for a 3.11 target. Consider dropping the helper and calling the built-in.
♻️ Proposed simplification
resolved = path.resolve(strict=False)
allowed = [_repo_logs_root(), _user_logs_root()]
- if not any(_safe_is_relative_to(resolved, root) for root in allowed):
+ if not any(resolved.is_relative_to(root) for root in allowed):
raise ValueError(
"logs root must be under the repository logs/ or ~/.config/ecli/logs/"
)
-
-
-def _safe_is_relative_to(path: Path, parent: Path) -> bool:
- """Return True when ``path`` is relative to ``parent`` (Python 3.9+)."""
- try:
- path.relative_to(parent)
- return True
- except ValueError:
- return FalseAs per coding guidelines: "Use Python >= 3.11" and "prefer existing patterns and helpers in Python code".
🤖 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/ecli/cli.py` around lines 329 - 335, The _safe_is_relative_to helper in
cli.py duplicates stdlib behavior and should be removed in favor of
Path.is_relative_to, which is available for the project’s Python 3.11 target.
Update the call sites that use _safe_is_relative_to to call the built-in
Path.is_relative_to directly, and delete the helper plus its outdated docstring
note about Python 3.9+. Ensure any related path checks in the CLI code continue
to behave the same after the replacement.
Source: Coding guidelines
Comment on lines
+5363
to
5388
| # Nonexistent path → open a new clean buffer at that path. | ||
| # This matches vim-style behaviour: the file will be created | ||
| # on the first save. The buffer is clean (not modified). | ||
| self.text = [""] | ||
| self.filename = None | ||
| self.filename = actual_filename_to_open | ||
| self.clear_diagnostic_line_highlight() | ||
| self.modified = False | ||
| self.encoding = "utf-8" | ||
| self.history.clear() | ||
| self.history.add_action( | ||
| { | ||
| "type": "open_file_missing", | ||
| "attempted_path": actual_filename_to_open, | ||
| "type": "new_buffer", | ||
| "filename": actual_filename_to_open, | ||
| "content": [""], | ||
| "encoding": "utf-8", | ||
| } | ||
| ) | ||
| self.set_initial_cursor_position() | ||
| self._set_status_message( | ||
| f"Error: File not found '{os.path.basename(actual_filename_to_open)}'" | ||
| f"New file: '{os.path.basename(actual_filename_to_open)}' (not yet saved)" | ||
| ) | ||
| logging.warning( | ||
| f"Open file failed: file not found at '{actual_filename_to_open}'" | ||
| logging.info( | ||
| "New buffer created for nonexistent path: '%s'", | ||
| actual_filename_to_open, | ||
| ) | ||
| return True |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map relevant symbols in the file
ast-grep outline src/ecli/core/Ecli.py --view expanded || true
echo '--- SEARCH ---'
rg -n "apply_syntax_highlighting_with_pygments|detect_language|_lexer|new_buffer|nonexistent path|successful-open" src/ecli/core/Ecli.py
echo '--- TARGETED LINES AROUND CLAIM ---'
sed -n '5340,5415p' src/ecli/core/Ecli.py
echo '--- TARGETED LINES FOR HIGHLIGHTING LOGIC ---'
sed -n '1,220p' src/ecli/core/Ecli.py | sed -n '1,220p'Repository: SSobol77/ecli
Length of output: 23741
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("src/ecli/core/Ecli.py")
lines = path.read_text().splitlines()
def show(start, end):
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:5d}: {lines[i-1]}")
# Show the open-buffer branches around the cited lines.
show(5335, 5415)
print("\n--- detect_language references ---")
for i, line in enumerate(lines, 1):
if "def detect_language" in line or "detect_language(" in line or "_lexer" in line:
print(f"{i:5d}: {line}")
print("\n--- apply_syntax_highlighting_with_pygments ---")
for i, line in enumerate(lines, 1):
if "def apply_syntax_highlighting_with_pygments" in line:
show(i, i + 140)
break
PYRepository: SSobol77/ecli
Length of output: 14441
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
lines = Path("src/ecli/core/Ecli.py").read_text().splitlines()
def show(start, end):
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:5d}: {lines[i-1]}")
print("--- success-path reset ---")
show(5488, 5510)
print("\n--- new_file reset ---")
show(6398, 6432)
PYRepository: SSobol77/ecli
Length of output: 2513
Reset syntax state for nonexistent-path buffers.
This branch should clear self._lexer and call self.detect_language() so a newly created buffer picks up the correct highlighting rules instead of reusing the previous file’s lexer.
🤖 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/ecli/core/Ecli.py` around lines 5363 - 5388, The nonexistent-path buffer
branch in Ecli.open should reset syntax highlighting state instead of inheriting
the previous file’s lexer. In the block that sets up the new clean buffer, clear
self._lexer and then call self.detect_language() after self.filename is assigned
so the new buffer gets the correct language/highlighting rules before returning.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and relevant symbols before reading slices.
git ls-files src/ecli/core/Ecli.py
rg -n "_file_loaded_from_disk|_file_had_final_newline|def _write_file|nonexistent path|New buffer created" src/ecli/core/Ecli.py
# Show the relevant region around the reported lines.
sed -n '5300,5425p' src/ecli/core/Ecli.py
# Show the write path and any initialization in nearby code.
sed -n '5480,5615p' src/ecli/core/Ecli.pyRepository: SSobol77/ecli
Length of output: 12098
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '5904,5985p' src/ecli/core/Ecli.py
printf '\n---\n'
sed -n '5455,5505p' src/ecli/core/Ecli.py
printf '\n---\n'
rg -n "new_buffer|_file_loaded_from_disk = False|_file_had_final_newline = False" src/ecli/core/Ecli.pyRepository: SSobol77/ecli
Length of output: 6267
Reset the file-origin flags for a fresh buffer. This branch still leaves _file_loaded_from_disk / _file_had_final_newline carrying over from the previous file, so the first save of a newly created nonexistent-path buffer can use the wrong newline behavior.
🤖 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/ecli/core/Ecli.py` around lines 5363 - 5388, Reset the file-origin state
when creating a fresh buffer for a nonexistent path in Ecli.open_file: this
branch currently initializes text, filename, modified, and history but leaves
_file_loaded_from_disk and _file_had_final_newline unchanged from the prior
file. Update that buffer setup so those flags are explicitly reset to the
correct defaults for a new unsaved file, alongside the existing new_buffer
initialization and logging.
Comment on lines
+169
to
+191
| def _flush_startup_input_buffer(self, target: Any) -> None: | ||
| """Drain and discard any bytes already buffered in the terminal before ECLI opened. | ||
|
|
||
| Called exactly once (on the first get_key_input invocation) so that | ||
| residual shell output, type-ahead, or control sequences present in the | ||
| terminal input queue at startup are never replayed as user text. | ||
| """ | ||
| target.nodelay(True) | ||
| drained = 0 | ||
| try: | ||
| while True: | ||
| ch = target.getch() | ||
| if ch == curses.ERR: | ||
| break | ||
| drained += 1 | ||
| finally: | ||
| target.nodelay(False) | ||
| if drained: | ||
| logging.debug( | ||
| "startup guard: discarded %d pre-existing buffered terminal byte(s)", | ||
| drained, | ||
| ) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unbounded drain loop can hang the UI thread on continuous input.
_flush_startup_input_buffer loops until getch() returns curses.ERR with no iteration cap. Every other buffered-read loop in this file bounds itself: _read_bracketed_paste caps at max_bytes, _complete_trailing_utf8 bounds by needed. If stdin/the terminal keeps delivering bytes continuously (e.g., piped input, a misbehaving terminal, or a CI harness), this loop never observes curses.ERR and spins indefinitely at startup, blocking the UI thread before the editor renders.
As per coding guidelines, **/*.py: "Do not introduce UI-thread blocking I/O in Python code" and "Keep changes scoped; prefer existing patterns and helpers in Python code".
🛠️ Proposed fix: cap the drain iterations
def _flush_startup_input_buffer(self, target: Any) -> None:
...
target.nodelay(True)
drained = 0
+ max_drain = 65536 # hard cap to avoid spinning on a continuous stream
try:
- while True:
+ while drained < max_drain:
ch = target.getch()
if ch == curses.ERR:
break
drained += 1
finally:
target.nodelay(False)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _flush_startup_input_buffer(self, target: Any) -> None: | |
| """Drain and discard any bytes already buffered in the terminal before ECLI opened. | |
| Called exactly once (on the first get_key_input invocation) so that | |
| residual shell output, type-ahead, or control sequences present in the | |
| terminal input queue at startup are never replayed as user text. | |
| """ | |
| target.nodelay(True) | |
| drained = 0 | |
| try: | |
| while True: | |
| ch = target.getch() | |
| if ch == curses.ERR: | |
| break | |
| drained += 1 | |
| finally: | |
| target.nodelay(False) | |
| if drained: | |
| logging.debug( | |
| "startup guard: discarded %d pre-existing buffered terminal byte(s)", | |
| drained, | |
| ) | |
| def _flush_startup_input_buffer(self, target: Any) -> None: | |
| """Drain and discard any bytes already buffered in the terminal before ECLI opened. | |
| Called exactly once (on the first get_key_input invocation) so that | |
| residual shell output, type-ahead, or control sequences present in the | |
| terminal input queue at startup are never replayed as user text. | |
| """ | |
| target.nodelay(True) | |
| drained = 0 | |
| max_drain = 65536 # hard cap to avoid spinning on a continuous stream | |
| try: | |
| while drained < max_drain: | |
| ch = target.getch() | |
| if ch == curses.ERR: | |
| break | |
| drained += 1 | |
| finally: | |
| target.nodelay(False) | |
| if drained: | |
| logging.debug( | |
| "startup guard: discarded %d pre-existing buffered terminal byte(s)", | |
| drained, | |
| ) |
🤖 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/ecli/ui/KeyBinder.py` around lines 169 - 191, The startup drain loop in
_flush_startup_input_buffer can block the UI thread indefinitely because it
reads until curses.ERR with no upper bound. Add a small iteration/byte cap to
the while loop, following the bounded patterns already used by
_read_bracketed_paste and _complete_trailing_utf8, and stop draining once that
cap is reached even if input keeps arriving. Keep the fix scoped to
KeyBinder._flush_startup_input_buffer and preserve the existing nodelay reset
and debug logging behavior.
Source: Coding guidelines
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant methods.
ast-grep outline src/ecli/ui/KeyBinder.py --view expanded || true
printf '\n--- relevant lines ---\n'
sed -n '1,260p' src/ecli/ui/KeyBinder.py | nl -ba | sed -n '120,240p'
printf '\n--- resize handling references ---\n'
rg -n "KEY_RESIZE|ungetch|_drain_text_burst|_flush_startup_input_buffer" src/ecli/ui/KeyBinder.pyRepository: SSobol77/ecli
Length of output: 1438
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the surrounding input-handling code paths that may consume KEY_RESIZE.
rg -n "def get_key_input|def _drain_text_burst|def _flush_startup_input_buffer|KEY_RESIZE|ungetch|getch\(" src/ecli/ui/KeyBinder.pyRepository: SSobol77/ecli
Length of output: 786
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the startup flush, burst drain, and input loop around the relevant lines.
sed -n '169,260p' src/ecli/ui/KeyBinder.py | cat -n
printf '\n---\n'
sed -n '958,1015p' src/ecli/ui/KeyBinder.py | cat -n
printf '\n---\n'
sed -n '900,930p' src/ecli/ui/KeyBinder.py | cat -nRepository: SSobol77/ecli
Length of output: 9125
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the helper and call-site relationship, focusing on whether the flush
# window can re-enter the main input path before resize handling exists.
python3 - <<'PY'
from pathlib import Path
path = Path("src/ecli/ui/KeyBinder.py")
lines = path.read_text().splitlines()
for start, end in [(169, 260), (900, 930), (958, 1015)]:
print(f"\n--- {start}-{end} ---")
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: SSobol77/ecli
Length of output: 8986
Startup flush can drop KEY_RESIZE. _flush_startup_input_buffer() consumes every queued code on the first get_key_input() call, so a resize that lands during startup is lost before the normal KEY_RESIZE handler runs. Requeue that event instead of discarding 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/ecli/ui/KeyBinder.py` around lines 169 - 191, The startup buffer drain in
_flush_startup_input_buffer currently discards every queued input, which can
lose a KEY_RESIZE event arriving during startup. Update the loop in
KeyBinder._flush_startup_input_buffer so it preserves KEY_RESIZE by requeueing
it for later handling instead of counting it as discarded, while still draining
and dropping all other pre-existing buffered bytes.
Comment on lines
+270
to
+279
| repo_logs = (Path.cwd() / "logs").resolve(strict=False) | ||
| user_logs = home / ".config" / "ecli" / "logs" | ||
|
|
||
| root = _effective_logs_root().resolve(strict=False) | ||
|
|
||
| in_repo = root.is_relative_to(repo_logs) or str(root).startswith(str(repo_logs)) | ||
| in_user = root.is_relative_to(user_logs) or str(root).startswith(str(user_logs)) | ||
|
|
||
| assert in_repo or in_user, ( | ||
| f"_effective_logs_root() is neither under repo logs nor ~/.config/ecli/logs: {root}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant test and implementation symbols.
rg -n "_effective_logs_root|_repo_logs_root|repo_logs|Path.cwd\(\)|resolve\(strict=False\)" tests/cli/test_phase1_service_cli.py ecli 2>/dev/null || true
# Show the relevant sections with line numbers.
python3 - <<'PY'
from pathlib import Path
paths = [
Path("tests/cli/test_phase1_service_cli.py"),
]
for path in paths:
if path.exists():
print(f"\n### {path} ###")
lines = path.read_text().splitlines()
for start, end in [(240, 320)]:
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: SSobol77/ecli
Length of output: 5317
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the implementation of the log-root helpers.
rg -n "def _effective_logs_root|def _repo_logs_root|logs_root|user_config|site-packages|Path\\(__file__\\)" ecli tests/cli/test_phase1_service_cli.py
python3 - <<'PY'
from pathlib import Path
path = Path("ecli/cli.py")
if path.exists():
lines = path.read_text().splitlines()
for start, end in [(1, 220)]:
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: SSobol77/ecli
Length of output: 2036
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the repo root helper is file-based or cwd-based.
python3 - <<'PY'
from pathlib import Path
import re
for p in [Path("ecli/cli.py"), Path("tests/cli/test_phase1_service_cli.py")]:
if p.exists():
text = p.read_text()
for name in ["_effective_logs_root", "_repo_logs_root"]:
m = re.search(rf"def {name}\(\).*?(?=^def |\Z)", text, re.S | re.M)
if m:
print(f"\n### {p}::{name} ###")
print(m.group(0))
PYRepository: SSobol77/ecli
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the implementation of the helper under test.
python3 - <<'PY'
from pathlib import Path
path = Path("ecli/cli.py")
if not path.exists():
print("ecli/cli.py not found")
raise SystemExit(0)
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if "_effective_logs_root" in line or "_repo_logs_root" in line:
start = max(1, i - 25)
end = min(len(lines), i + 80)
print(f"\n### around line {i} ###")
for j in range(start, end + 1):
print(f"{j:4d}: {lines[j-1]}")
PYRepository: SSobol77/ecli
Length of output: 173
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any repo-root helper used by tests or CLI.
rg -n "_repo_root|_repo_logs_root|_effective_logs_root|Path.cwd\(\).*logs" ecli tests -g '!**/__pycache__/**'Repository: SSobol77/ecli
Length of output: 4284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
path = Path("ecli/cli.py")
if not path.exists():
print("ecli/cli.py not found")
raise SystemExit(0)
text = path.read_text()
for name in ["_effective_logs_root", "_repo_logs_root"]:
m = re.search(rf"def {name}\(.*?(?=^def |\Z)", text, re.S | re.M)
if m:
print(f"\n### {name} ###")
print(m.group(0))
PYRepository: SSobol77/ecli
Length of output: 173
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the package and the helper implementation file.
git ls-files | sed -n '1,200p'
printf '\n---\n'
fd -a 'cli.py' .
printf '\n---\n'
fd -a '_effective_logs_root' .Repository: SSobol77/ecli
Length of output: 6489
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [Path("src/ecli/cli.py"), Path("tests/cli/test_phase1_service_cli.py")]:
print(f"\n### {path} ###")
if not path.exists():
print("missing")
continue
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if "_effective_logs_root" in line or "_repo_logs_root" in line or "Path.cwd()" in line:
start = max(1, i - 20)
end = min(len(lines), i + 60)
for j in range(start, end + 1):
print(f"{j:4d}: {lines[j-1]}")
print()
PYRepository: SSobol77/ecli
Length of output: 50369
Anchor repo_logs to the same repo-root source as _effective_logs_root()
tests/cli/test_phase1_service_cli.py:270-279 uses Path.cwd(), but the implementation resolves the repo root from src/ecli/cli.py via Path(__file__).resolve().parents[2]. Import _repo_logs_root() here instead of recomputing the path from the current working directory.
🤖 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/cli/test_phase1_service_cli.py` around lines 270 - 279, The test is
recomputing the repository logs path from Path.cwd() instead of using the same
repo-root source as _effective_logs_root(), which can diverge depending on the
working directory. Update tests/cli/test_phase1_service_cli.py to anchor
repo_logs by importing and using _repo_logs_root() (the same helper used by
_effective_logs_root() in src/ecli/cli.py) and keep the existing assertions
against in_repo and in_user unchanged.
Comment on lines
+219
to
+230
| def test_startup_guard_deactivates_after_first_flush() -> None: | ||
| kb = make_keybinder() | ||
|
|
||
| window = MagicMock() | ||
| window.getch.return_value = curses.ERR # no buffered bytes | ||
|
|
||
| kb._flush_startup_input_buffer(window) | ||
|
|
||
| # Guard must be deactivated by the caller (get_key_input) after flushing. | ||
| # Here we deactivate manually to mirror that contract. | ||
| kb._startup_guard_active = False | ||
| assert kb._startup_guard_active is False |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test doesn't validate real guard-deactivation behavior.
This test calls _flush_startup_input_buffer but never asserts its actual effect on _startup_guard_active. Instead, it manually sets kb._startup_guard_active = False (line 229) and then asserts that same value (line 230) — the assertion is tautologically true no matter what _flush_startup_input_buffer does, so the test cannot fail even if the implementation regresses. The real contract (guard deactivation is get_key_input's responsibility) is already covered by test_get_key_input_returns_err_on_first_call_when_guard_active.
As per coding guidelines, "A mock-based test is invalid if it replaces the behavior being claimed as fixed" and "Tests that only assert mock calls, duplicate old behavior, or do not represent real ECLI behavior must be removed or rewritten."
🧪 Proposed fix: assert the pre-override state instead
def test_startup_guard_deactivates_after_first_flush() -> None:
kb = make_keybinder()
window = MagicMock()
window.getch.return_value = curses.ERR # no buffered bytes
kb._flush_startup_input_buffer(window)
- # Guard must be deactivated by the caller (get_key_input) after flushing.
- # Here we deactivate manually to mirror that contract.
- kb._startup_guard_active = False
- assert kb._startup_guard_active is False
+ # _flush_startup_input_buffer itself must NOT toggle the guard;
+ # deactivation is get_key_input's responsibility.
+ assert kb._startup_guard_active is True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_startup_guard_deactivates_after_first_flush() -> None: | |
| kb = make_keybinder() | |
| window = MagicMock() | |
| window.getch.return_value = curses.ERR # no buffered bytes | |
| kb._flush_startup_input_buffer(window) | |
| # Guard must be deactivated by the caller (get_key_input) after flushing. | |
| # Here we deactivate manually to mirror that contract. | |
| kb._startup_guard_active = False | |
| assert kb._startup_guard_active is False | |
| def test_startup_guard_deactivates_after_first_flush() -> None: | |
| kb = make_keybinder() | |
| window = MagicMock() | |
| window.getch.return_value = curses.ERR # no buffered bytes | |
| kb._flush_startup_input_buffer(window) | |
| # _flush_startup_input_buffer itself must NOT toggle the guard; | |
| # deactivation is get_key_input's responsibility. | |
| assert kb._startup_guard_active is True |
🤖 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/ui/test_startup_paste_guard.py` around lines 219 - 230, The test in
test_startup_guard_deactivates_after_first_flush is tautological because it
manually sets _startup_guard_active to False after calling
_flush_startup_input_buffer and then asserts the same value. Rewrite it to
validate the actual effect of _flush_startup_input_buffer on the keybinder state
using make_keybinder and the _startup_guard_active flag, or remove this test if
get_key_input already covers the guard deactivation contract.
Source: Coding guidelines
Comment on lines
+300
to
+324
| def test_get_key_input_processes_normal_key_after_guard_fires() -> None: | ||
| """After the guard fires once, subsequent calls behave normally.""" | ||
| kb = make_keybinder() | ||
|
|
||
| window = MagicMock() | ||
| # First call: guard fires, nothing in buffer → ERR | ||
| window.getch.return_value = curses.ERR | ||
| first = kb.get_key_input(window) | ||
| assert first == curses.ERR | ||
| assert kb._startup_guard_active is False | ||
|
|
||
| # Second call: guard is gone; a real keypress arrives. The single | ||
| # keystroke is followed by ERR so _drain_text_burst's non-blocking | ||
| # look-ahead terminates instead of looping forever on a fixed | ||
| # return_value. | ||
| window.getch.return_value = None | ||
| window.getch.side_effect = [ord("x"), curses.ERR] | ||
| # _drain_text_burst will be called; make nodelay a no-op | ||
| window.nodelay = MagicMock() | ||
| window.timeout = MagicMock() | ||
|
|
||
| second = kb.get_key_input(window) | ||
| # The result should be something other than the guard's ERR sentinel; | ||
| # it may be the character or a paste event if burst-drained. | ||
| assert second != curses.ERR or kb._startup_guard_active is False |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Tautological final assertion doesn't verify normal-key behavior.
kb._startup_guard_active is already False after the first call (asserted at line 309) and nothing re-activates it, so ... or kb._startup_guard_active is False is always True — the assertion passes regardless of what second is, defeating the stated purpose ("subsequent calls behave normally").
As per coding guidelines, "Tests that only assert mock calls, duplicate old behavior, or do not represent real ECLI behavior must be removed or rewritten."
🧪 Proposed fix: assert the concrete expected value
second = kb.get_key_input(window)
- # The result should be something other than the guard's ERR sentinel;
- # it may be the character or a paste event if burst-drained.
- assert second != curses.ERR or kb._startup_guard_active is False
+ # A single keystroke followed by ERR should be returned as-is (fast path),
+ # not swallowed as the guard's ERR sentinel.
+ assert second == ord("x")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_get_key_input_processes_normal_key_after_guard_fires() -> None: | |
| """After the guard fires once, subsequent calls behave normally.""" | |
| kb = make_keybinder() | |
| window = MagicMock() | |
| # First call: guard fires, nothing in buffer → ERR | |
| window.getch.return_value = curses.ERR | |
| first = kb.get_key_input(window) | |
| assert first == curses.ERR | |
| assert kb._startup_guard_active is False | |
| # Second call: guard is gone; a real keypress arrives. The single | |
| # keystroke is followed by ERR so _drain_text_burst's non-blocking | |
| # look-ahead terminates instead of looping forever on a fixed | |
| # return_value. | |
| window.getch.return_value = None | |
| window.getch.side_effect = [ord("x"), curses.ERR] | |
| # _drain_text_burst will be called; make nodelay a no-op | |
| window.nodelay = MagicMock() | |
| window.timeout = MagicMock() | |
| second = kb.get_key_input(window) | |
| # The result should be something other than the guard's ERR sentinel; | |
| # it may be the character or a paste event if burst-drained. | |
| assert second != curses.ERR or kb._startup_guard_active is False | |
| def test_get_key_input_processes_normal_key_after_guard_fires() -> None: | |
| """After the guard fires once, subsequent calls behave normally.""" | |
| kb = make_keybinder() | |
| window = MagicMock() | |
| # First call: guard fires, nothing in buffer → ERR | |
| window.getch.return_value = curses.ERR | |
| first = kb.get_key_input(window) | |
| assert first == curses.ERR | |
| assert kb._startup_guard_active is False | |
| # Second call: guard is gone; a real keypress arrives. The single | |
| # keystroke is followed by ERR so _drain_text_burst's non-blocking | |
| # look-ahead terminates instead of looping forever on a fixed | |
| # return_value. | |
| window.getch.return_value = None | |
| window.getch.side_effect = [ord("x"), curses.ERR] | |
| # _drain_text_burst will be called; make nodelay a no-op | |
| window.nodelay = MagicMock() | |
| window.timeout = MagicMock() | |
| second = kb.get_key_input(window) | |
| # A single keystroke followed by ERR should be returned as-is (fast path), | |
| # not swallowed as the guard's ERR sentinel. | |
| assert second == ord("x") |
🤖 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/ui/test_startup_paste_guard.py` around lines 300 - 324, The final
assertion in test_get_key_input_processes_normal_key_after_guard_fires is
tautological because kb._startup_guard_active is already False, so rewrite it to
verify the actual returned key behavior from kb.get_key_input. Use the concrete
result from the second call to assert that it matches the expected normal input
path (or the specific burst-drained outcome if that is what the test is meant to
cover), and remove the always-true fallback. Keep the focus on the behavior
exercised by get_key_input and _drain_text_burst rather than rechecking the
guard flag.
Source: Coding guidelines
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
1 participant
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.
This PR implements the ECLI 0.2.3 UX/runtime stabilization hotfix.
Fixes:
Additional regression fixes:
Validation:
Manual smoke:
Evidence:
Non-goals:
Summary by CodeRabbit
New Features
Bug Fixes