Skip to content

refactor: Guido simplification + testing.md compliance pass#151

Merged
aorumbayev merged 8 commits into
mainfrom
refactor/guido-simplification
May 6, 2026
Merged

refactor: Guido simplification + testing.md compliance pass#151
aorumbayev merged 8 commits into
mainfrom
refactor/guido-simplification

Conversation

@aorumbayev

Copy link
Copy Markdown
Member

Summary

  • Dead-code excision in core: deleted unused private wrappers and the legacy dict API on kagan.core (get_backend, to_legacy_config, guidance_hints, _acp_startup_timeout_seconds, _friendly_startup_error_message, _acp_process_exit_hint); fixed fake asyncio.StreamReader inheritance in _ByteCountingStreamReader and JsonRpcObjectStreamReader; consolidated process termination through a shared terminate_process() helper.
  • Sessions.run() split and async-ified DB mutation helpers.
  • TUI cosmetics: settings, context_footer, workspace, kanban, chat — pure cleanups, no behavior change.
  • task_screen.py split (760 lines) into task_screen.py + _task_chat.py + _task_review.py + _task_stream.py.
  • Final acp aliases gone: deleted _acp_handshake_timeout_seconds / _friendly_acp_error_message shims in cli/chat/acp.py; callers use the public kagan.core functions directly.
  • Test layout aligned with docs/internal/testing.md:
    • 15 unit tests moved tests/core/unit/tests/unit/core/
    • 4 widget/screen unit tests moved tests/tui/tests/unit/tui/
    • All unit-test files now carry pytest.mark.unit
    • RateLimitMiddleware._safe_log_key and macOS env-sanitization unit tests pulled out of behavioral suites into tests/unit/server/test_middleware.py and tests/unit/cli/test_env_sanitization.py
    • Deleted tests/core/unit/test_transitions.py (asserted on locally defined helpers; the real state machine is covered by the comprehensive tests/unit/core/test_transitions.py)
    • Replaced asyncio.sleep polling in test_chat_first_turn_invokes_title_generator with an asyncio.Event set by the title generator + a yield-only loop
    • Added parametrized error-classification coverage for friendly_acp_error_message

Test plan

  • uv run poe lint
  • uv run poe typecheck — 0 errors (74 suppressed)
  • uv run pytest tests/unit/ — 945 passed, 1 skipped
  • uv run pytest tests/core/ tests/mcp/ tests/integrations/ -m "not slow" — 1513 passed, 1 skipped, 1 xpassed
  • uv run pytest tests/tui/ -m "not snapshot and not slow" — 147 passed, 1 skipped
  • CI green across the matrix

@greptile-apps

greptile-apps Bot commented May 6, 2026

Copy link
Copy Markdown

Greptile Summary

This PR is a broad refactoring and cleanup pass: dead private wrapper functions are removed, fake asyncio.StreamReader inheritance is replaced with explicit composition, DB mutation helpers are promoted from sync to async, Sessions.run() is split into _run_detached / _run_attached, and the 760-line task_screen.py is split into three focused mixin modules. The test suite is reorganised to match the layout documented in testing.md.

  • Core cleanup: _terminate_acp_process / _terminate_stdio_process are consolidated into a shared terminate_process() helper; _ByteCountingStreamReader and JsonRpcObjectStreamReader drop asyncio.StreamReader inheritance and replace __getattr__ with explicit delegation stubs.
  • Sessions async-ification: _update_session_pid, _mark_session_running, _complete_session, and _fail_session are promoted to async methods using _db_async, removing all asyncio.to_thread wrapping.
  • Test hygiene: 19 unit tests moved to tests/unit/, pytest.mark.unit added to all unit files, asyncio.Event-driven polling replaces the unbounded sleep loop in test_chat_first_turn_invokes_title_generator.

Confidence Score: 5/5

Safe to merge — all changes are refactoring with no behaviour modification to the production path.

The diff consolidates three previously-duplicated process-termination helpers, promotes sync DB helpers to async, and replaces fake inheritance with explicit composition. Every changed call site has a corresponding test fix, and both previous-thread concerns are directly addressed. No correctness regressions were identified.

No files require special attention.

Important Files Changed

Filename Overview
src/kagan/core/_subprocess.py Adds terminate_process() as a shared helper consolidating three previously-duplicate process shutdown implementations.
src/kagan/core/_acp_streams.py Drops fake asyncio.StreamReader inheritance; replaces getattr with explicit readline, readuntil, read, readexactly, at_eof stubs.
src/kagan/core/_sessions.py DB mutation helpers converted from sync+asyncio.to_thread to native async; run() split into _run_detached / _run_attached with proper -> Session return annotation.
src/kagan/core/_agent.py _ByteCountingStreamReader now uses composition instead of asyncio.StreamReader inheritance; getattr removed; all four read methods explicitly delegated.
src/kagan/core/_acp.py Private alias wrappers removed; all three _terminate_acp_process call sites replaced with terminate_process().
tests/core/test_chat_lifecycle.py Unbounded sleep poll replaced with asyncio.Event-driven synchronisation and asyncio.wait_for timeout guard.
.github/workflows/ci.yml Unit test path updated from tests/core/unit/ to tests/unit/core/ to match the new test layout.
tests/unit/test_sessions_shutdown.py Monkeypatched sync lambdas replaced with proper async functions to match the newly-async method signatures.

Reviews (2): Last reviewed commit: "refactor(core): address greptile review ..." | Re-trigger Greptile

Comment thread src/kagan/core/_acp_streams.py
Comment thread tests/core/test_chat_lifecycle.py Outdated
aorumbayev and others added 8 commits May 6, 2026 16:30
- Delete dead private wrappers in _acp.py (_acp_startup_timeout_seconds,
  _friendly_startup_error_message, _acp_process_exit_hint)
- Delete legacy dict API (BackendSpec.to_legacy_config, guidance_hints, get_backend)
- Remove get_backend from __init__.py exports
- Update callers in cli/tools.py and _environment_checks.py to use typed API
- Fix _ByteCountingStreamReader: remove asyncio.StreamReader inheritance,
  remove __getattr__ catch-all, use explicit forward methods
- Fix JsonRpcObjectStreamReader: remove asyncio.StreamReader inheritance
- Consolidate duplicate process termination into _subprocess.terminate_process()
- Update tests to remove legacy API coverage
- Extract _run_detached() and _run_attached() from run()
- Compute shared setup (db_path_str, backend_spec) once before branching
- Convert _update_session_pid, _mark_session_running, _complete_session,
  _fail_session from sync to async using _db_async directly
- Update all callers to use await self._method() instead of
  await asyncio.to_thread(self._method, ...)
- Keep _set_status as externally-injected sync callable
- delete _acp_handshake_timeout_seconds and _friendly_acp_error_message
  aliases in cli/chat/acp.py; call the public functions directly
- relocate 15 unit tests from tests/core/unit/ into tests/unit/core/ and
  4 widget/screen unit tests from tests/tui/ into tests/unit/tui/, the
  layout testing.md prescribes
- normalize markers so every unit-test file carries pytest.mark.unit
- pull RateLimitMiddleware._safe_log_key contract into
  tests/unit/server/test_middleware.py and macOS env-sanitization into
  tests/unit/cli/test_env_sanitization.py instead of leaking unit-shaped
  assertions into behavioral suites
- drop tests/core/unit/test_transitions.py: it asserted on locally
  defined helpers, the real state machine is covered by the 397-line
  tests/unit/core/test_transitions.py
- replace asyncio.sleep polling in test_chat_first_turn_invokes_title_generator
  with an asyncio.Event set by the title generator plus a yield-only loop
- extend friendly_acp_error_message coverage with a parametrized
  classification test (rate limit, quota, network, overloaded)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous commit moved tests from tests/core/unit/ to tests/unit/core/
to comply with docs/internal/testing.md, but the CI fast-gate and CD
release-smoke step still pointed at the old path and selected zero tests.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- JsonRpcObjectStreamReader: add explicit read/readuntil/readexactly
  delegating stubs so an SDK call to one of those methods raises a clear
  error rather than being silently swallowed by the broad AttributeError
  handler in run_acp_session, mirroring _ByteCountingStreamReader.
- test_chat_first_turn_invokes_title_generator: bound the post-event
  label-observation loop with asyncio.wait_for(timeout=3.0) so a regression
  in the engine's label write surfaces as a TimeoutError instead of hanging
  the test indefinitely.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@aorumbayev aorumbayev force-pushed the refactor/guido-simplification branch from e866ae0 to 0181f25 Compare May 6, 2026 14:30
@aorumbayev

Copy link
Copy Markdown
Member Author

@greptileai

@aorumbayev aorumbayev merged commit f34286c into main May 6, 2026
18 checks passed
@aorumbayev aorumbayev deleted the refactor/guido-simplification branch May 6, 2026 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants