READABILITY REVIEW, week of 2026-09-11 (read-only scan by the repair role on the maintainer's request: "code that adds unnecessary complexity, redundancy or anything else that would make it hard for a human reader to fully follow the functionality"). Findings are candidates for tasks, ordered by how much they cost a reader; nothing was changed by the scan. Pick the ones worth doing; each is one small unit. Several of these are the repair role's own additions this week (the remit builders, the digest's marker parsing, the prelude constants) and are owned as such.

# Ollama-Codex — readability review of the last seven days

## Summary

The week added about 14,000 lines across 113 files: a coordinator runner with a clock and digest, merge-gate cards and a review page, a git-HTTP mirror, a badge bridge, per-project sandbox secrets, and a crystal-robots CGI. Most of it works by adding a new poller, a new marker prefix, or a new copy of an existing helper next to the old one rather than extending it. A reader now meets the same idea (limit-banner parsing, "own checkout" resolution, the task prelude, the Needs-you card, config-blob writes) in two or three places with small differences and has to decide which one is real. The single biggest cost is that duplication, compounded by comments that explain the ticket history instead of the behaviour, so the reader cannot tell a deliberate difference from drift.

## Findings

**1. Two renderers for the Needs-you card, one of them only reachable from its spec**
`src/activity_report.cr:needs_you_card_html` and `src/ui.cr:render_needs_you_card`. The two are near-identical 40-line HTML builders (the activity_report one adds a no-session warning); production calls only the activity_report version, while `spec/ui_spec.cr` tests the UI one. A reader who fixes one will not know the other exists until the spec fails. Delete `UI.render_needs_you_card` and its spec, or make the dashboard call it.

**2. Provider-limit banner parsing exists twice with different fallbacks**
`src/task_worker.cr:task_limit_reset_time?` versus `src/session_agent.cr:classify_claude_output` + `parse_claude_reset_time`. Same substring list, same regex, same am/pm arithmetic, but the worker copy falls back to "now + 1 hour" and the session copy to nil, and its own comment says it "reuses the same pattern". Extract one pure `LimitBanner.reset_time?(text)` and call it from both.

**3. The isolated-task prelude is built in three files**
`src/isolated_task.cr:branch_instructions`, `src/agent_tool.cr:task_enqueue`, `src/chat_cgi_app.cr:enqueue_ticket_as_task`. All three emit the same seven bullet lines, and this week each was hand-extended with the same three constants (`SPEC_FOREGROUND_NOTE`, `SELF_CHECK`, `PEERS_NOTE`). A reader cannot tell whether the three preludes are meant to differ. Keep `IsolatedTask.branch_instructions` and call it from the other two.

**4. `dashboard` carries nine-field positional tuples that are then re-parsed as JSON**
`src/activity_report.cr:dashboard` (178 lines) and `schema_field_gates`. `needs` is `{String, String, String, String, Int32, String?, String, String, Bool}` with a comment line naming the slots; the same shape is destructured in three places, serialised to JSON, and read back by `UI.render_console_activity_report` and `TaskWorker.coordinator_digest` with `n["reason"]?.try(&.as_s?)` chains. Replace the tuple with a `record NeedsYou` that serialises itself, and split `dashboard` into "collect per repo" and "build JSON".

**5. Three ways to write a config-table row, chosen by `is_a?`**
`src/provider_env.cr:store_blob` (in-process, locked), `src/repo_bootstrap.cr:declare_journal_mode` (in-process, unlocked, because the caller holds the lock) and `src/repo_bootstrap.cr:apply_blob_setting` (a `fossil sqlite3` shell-out with hand `sql_quote` and `unixepoch()`, while the other two share `MTIME_NOW_SQL`). `RepoBootstrap.ensure` then dispatches with `if inv.is_a?(ExactBlobSetting)` inside a class hierarchy that exists to avoid exactly that. Give `Invariant` an `apply(repo, value)` method and route every config write through one function that takes an "already locked" flag.

**6. `has_live_session?` re-implements `SessionStore.get`**
`src/activity_report.cr:has_live_session?`. It sanitises the hash, builds the `ollama-session:` key, opens the DB and parses the JSON by hand, but reads only the legacy key while `SessionStore.get` also resolves the canonical root. A reader has to compare the two to see which is right. Replace with `SessionStore.get(tid, repo).try(&.status) == "open"`.

**7. `bridge_run` copies twenty lines of `job_start`'s ownership check**
`src/agent_tool.cr:bridge_run` and `job_start`. The `own_checkout` / `Paths.under?` / `clone_caller_binding` block is duplicated verbatim, with a comment saying "mirrors job_start". Extract `resolve_caller_owner(root, ctx, caller_checkout)` returning `{owner, enqueue_repo, runner_id}`.

**8. Four pollers over the same held-ticket set, with two dedupe caches**
`src/task_worker.cr:poll_merge_gate_landings`, `poll_decision_thread_posts`, `poll_held_ticket_responses`, `route_finished_task_to_gate`, plus `ungated_review_tickets`. Each tick queries `held_task_ticket_ids` up to three times, two instance sets (`@held_task_ids`, `@woken_decision_tasks`) must be kept consistent by hand, and `ungated_review_tickets` regex-parses the OCX-STATE JSON that `TaskQueue.latest_marker` already parses. Compute one `GateSnapshot` per tick and let each action read from it.

**9. `log_live_write` is guarded twice at nine call sites**
`src/config.cr:log_live_write` and its callers in `src/agent_tool.cr`. The function already returns unless `login.starts_with?("agent")`, yet every caller writes `user || ""` and appends `if user && !user.empty?`. Make the parameter `String?` and drop the call-site guards.

**10. Six remit builders and a hash-of-the-prompt version scheme**
`src/session_agent.cr:prompt_version`, `coordinator_remit`, `claude_milestone_remit`, `claude_trunk_ops_remit`, `codex_milestone_remit`, `codex_trunk_ops_remit`, `codex_coordinator_remit` (a one-line wrapper), `claude_priming_prompt_coordinator`. To learn what a session is told, a reader walks runner-type × role × priming-shell, and `prompt_version` re-renders the remit with an empty memory just to digest it. Collapse to `remit_for(session)` returning text plus its digest, and one priming template that takes the remit.

**11. Dead lookup chain and a second trunk-tip query in the peer deployer**
`src/served_peer_deploy.cr:peer_primary_checkout`, `set_peer_primary_checkout`, `trunk_tip`. The three-tier checkout lookup (env var, config key, cgi-bin symlink walk) has no caller — `TaskWorker.poll_served_peer_deploys` uses `ctx.deployment_checkout`. `trunk_tip` finds trunk by `tagtype=1 ORDER BY mtime` while `TaskQueue.commit_landed_on_trunk?` uses a `leaf`/`tagtype=2` CTE, and `run_deploy` repeats the record-failure/stamp/return block four times. Delete the lookup chain, share one trunk-tip query, and fold the failure block into a `fail(reason)` helper.

**12. Request dispatchers grew inline instead of gaining handlers**
`src/chat_cgi_app.cr:run` (1230 lines; this week added `approve_merge`, `badge_set_led`, `sessions.json`, `merge_review` inline) and `src/agent_tool_cli.cr:run` (954 lines; added `bridge-run`, `peer-add`, `peer-rm`, `badge-spec-run`, `peer-ticket-update`, each with its own `while i < ARGV.size` option loop). Move each action to a named method and use one `take_flag(ARGV, "--label")` helper.

Noted and moved past: `src/providers.cr` keeps two entries for the same LiteLLM gateway with a comment saying the maintainer must pick; `src/turn_watchdog.cr`'s channel rendezvous is explained by the race it closes.

## Recurring patterns

- **Helper copied instead of shared**: the fossil binary is resolved from `ENV["OLLAMA_CODEX_FOSSIL_BINARY"]` in 13 separate `fossil_executable`/`fossil_binary` definitions across 22 files (`src/served_peer_deploy.cr` inlines it four times); `Config.git_served_base` re-derives what `ForumWriter.base_url_for` already computes; `has_design_input_column?` and `has_human_verify_column?` are the same query with a different literal.
- **Comments narrate tickets, dates and redo numbers, not behaviour**: 128 added comment lines in `src/` cite a ticket hash, a "W1.10b"-style milestone id, or a date ("2026-09-15, three times", "FIX 2 from REDO", "occurrence 6"); the reader has to open the ticket to learn what the code guarantees.
- **Bare `rescue` returning nil/empty**: 29 new bare `rescue` clauses and 18 `rescue nil` expressions; `FossilChat.post_message` now returns nil on any failure, `ActivityReport.merge_gate_detail` returns nil for "no gate" and "database unreadable" alike, so callers cannot distinguish absence from breakage.

## Method

Read the full `fossil diff` for the range, then every added `src/` file whole (`served_peer_deploy`, `git_http_handler`, `bridge_jobs`, `plan_check`, `robots_*`, `badge_controller`, `glossary`, `sandbox_secrets`, `forgejo_mirror_survey`) and the touched regions of `task_worker`, `agent_tool`, `agent_tool_cli`, `activity_report`, `ui`, `session_agent`, `session_store`, `task_queue`, `repo_bootstrap`, `chat_cgi_app`, `config`, `isolation`, `mcp_server`, `mirror_sync`, `fossil_forum`, `forum_ipc`, plus the callees each new function reaches (`SessionStore.get`, `ForumWriter.base_url_for`, `job_start`, `classify_claude_output`), and grepped callers for every helper flagged as duplicated or dead.
Did not read `ui.cr`'s 740-line `chat_javascript` beyond its diff, `ollama_tool_agent.cr` beyond its diff, the spec files beyond `spec_helper` and `task_worker_integration_spec`, `docs/`, or `lib/`.
About forty minutes, read-only; no commands were run against the repository other than `fossil diff`/`status`.

PROGRESS 2026-09-19 (coordinator fable): findings are being worked one small unit at a time as their own tickets, referencing this review. Started with finding 3 (task prelude in three files); next in order of reader cost: 2 (limit-banner parsing), 1 (two Needs-you renderers), 6 (has_live_session re-implements SessionStore.get), 7 (bridge_run copies job_start), 5 (three config-row writers), 4 (nine-field tuples). Each closes here with its commit.