Skip to content

Commit 425492e

Browse files
Kasper JungeRalphify
authored andcommitted
workspace: record _call_safely extraction + seed _agent.py coverage note
Co-authored-by: Ralphify <noreply@ralphify.co>
1 parent cb61477 commit 425492e

3 files changed

Lines changed: 62 additions & 1 deletion

File tree

workspace/ralphs/improve-codebase/backlog.md

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,12 @@ only when they land in a commit.
88
- Audit `_console_emitter.py` for unused private helpers / constants (grep
99
each `_foo` name for other references inside the module and tests).
1010
- Audit `_agent.py` for parallel streaming/blocking helpers that reference
11-
the same constants but define their own copies.
11+
the same constants but define their own copies. (cb61477 — extracted
12+
`_call_safely` for the 3× best-effort observer-callback pattern; no
13+
remaining obvious dup after that pass. Streaming's `_readline_pump` and
14+
blocking's `_pump_stream` look similar but do genuinely different work:
15+
the queue-based pump feeds a main-thread loop that parses JSON, while
16+
the list-based pump does its callback work inline on its own thread.)
1217
- Check `cli.py` validators for unreachable error branches after recent
1318
TypedDict refactors.
1419
- Confirm every `from typing import ...` import in `src/ralphify/` is used.
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
# `_agent.py` coverage
2+
3+
Valid at: cb61477
4+
5+
## Recent changes
6+
7+
- cb61477 — added `_call_safely(callback, *args)` helper next to the
8+
callback type aliases. Replaces three copies of the
9+
`if cb is not None: try: cb(...); except Exception: pass` pattern
10+
(two in `_read_agent_stream`, one in `_pump_stream`) with single-line
11+
calls. Behavior preserved — identical None guard, identical broad
12+
`Exception` suppression, identical argument-once semantics.
13+
14+
## Shape of the module
15+
16+
- Two execution paths: `_run_agent_streaming` (JSON line stream, used for
17+
`claude`) and `_run_agent_blocking` (subprocess.Popen with optional
18+
capture, used for all other agents).
19+
- `execute_agent` is the single public entry point; selects mode via
20+
`_supports_stream_json(cmd)` (checks `Path(cmd[0]).stem == CLAUDE_BINARY`).
21+
- Shared shutdown sequence is centralized in `_cleanup_agent`:
22+
1. `_ensure_process_dead` (SIGTERM → SIGKILL via `_try_graceful_group_kill`,
23+
then `proc.kill()`).
24+
2. `_close_pipes` (raw `os.close` on stdout/stderr fds to unblock readers).
25+
3. `_drain_readers` (bounded join on reader/writer threads).
26+
4. `_finalize_pipes` (Python-level `pipe.close()` for GC hygiene).
27+
- Thread spawning uses `_start_writer_thread` / `_start_pump_thread` to
28+
centralize the `Thread(..., daemon=True); .start()` boilerplate.
29+
30+
## Verified live (grepped, confirmed used)
31+
32+
- `CLAUDE_BINARY` — public; imported by `_console_emitter.py` for display
33+
logic (see backlog note about consolidating `_is_claude_command` /
34+
`_supports_stream_json`; deferred until a third caller appears).
35+
- `_STDOUT`, `_STDERR` — used in `_run_agent_streaming` /
36+
`_run_agent_blocking` stderr pump calls and inside `_read_agent_stream`.
37+
- `_SIGTERM_GRACE_PERIOD`, `_THREAD_JOIN_TIMEOUT`, `_PROCESS_WAIT_TIMEOUT`
38+
— each referenced exactly once; constants kept near usage as the
39+
project convention prefers.
40+
- `AgentResult`, `_StreamResult` — returned from streaming/blocking paths
41+
and consumed by `engine.py`.
42+
43+
## Potential future wins (not yet taken)
44+
45+
- `_run_agent_streaming` and `_run_agent_blocking` both finish with the
46+
same "`stdout = "".join(...); stderr = "".join(...); log_file =
47+
_write_log(...); return AgentResult(...)`" tail, but the shape of the
48+
intermediate state differs (tuple vs list|None), so extracting would
49+
mostly move arguments around. Revisit only if a third execution path
50+
appears.
51+
- The two `if proc.stdin/stdout/stderr is None: raise RuntimeError(...)`
52+
guards just after `Popen` could use a single helper, but `subprocess`
53+
guarantees these are non-None when `PIPE` is passed — the guards exist
54+
mainly to narrow for the type checker, and a helper would make the
55+
narrow less explicit. Leave as-is.

workspace/ralphs/improve-codebase/iterations.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
One line per iteration: `<sha> <summary>`.
44

5+
cb61477 refactor: extract `_call_safely` helper — dedupes 3× best-effort callback guards in `_agent.py`
56
4ccfa9a refactor: drop redundant `if parts else ""` in `_format_params` (empty join already returns "")
67
3e9627b refactor: extract `_stop_compact_live_unlocked` to dedupe compact-Live teardown across 3 call sites
78
0900aad refactor: drop redundant `_iteration_order` list — dict insertion order suffices

0 commit comments

Comments
 (0)