Feat/cli raw attach - #17
Merged
Merged
Conversation
A Finder/Launchpad-launched macOS app gets launchd's bare PATH
(/usr/bin:/bin:/usr/sbin:/sbin), so Command::new("tailscale") never
found the CLI even when it was installed and working from a terminal.
tailscale_ip()/tailscale_detect() silently fell back to the LAN IP, so
toggling "Usar Tailscale" never changed the shown URL. Check the known
install locations (/usr/local/bin, /opt/homebrew/bin) before falling
back to a bare PATH lookup. Verified live against the daemon: remote
auto-start and the stop/start toggle cycle both now resolve to the
Tailscale IP.
Replaced bento-cli's line-buffered attach() with a real interactive terminal session on unix: raw termios (no local echo/canonical mode, so Ctrl-C/Ctrl-D pass through to the remote process instead of the local shell), live window-resize forwarding via SIGWINCH, and UTF-8-safe byte streaming instead of newline-delimited lines. Falls back to the old line-based path when stdin isn't a real tty or on non-unix. Verified against a real pty harness: single echo (no local double-echo), SIGWINCH correctly reaches the remote PTY, Ctrl-C reaches the remote process without killing the local CLI, and termios is restored both on normal remote exit and on SIGTERM. Also fixed a real hang found during that verification: tokio's stdin reader runs on a blocking OS thread that the runtime waits for on shutdown, so returning normally left the process stuck until the next keystroke — now exits explicitly once the terminal is restored, matching the daemon's own daemon.shutdown pattern. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Extracted the HTTP /api/review/branches handler's core logic into a shared list_branches() fn so it's reusable without axum, then exposed it on the daemon's lightweight IPC socket as review.branches (no token/HTTP server needed, same trust level as the existing terminal.* commands) and added `bento review branches [--cwd <dir>]` as its CLI entry point. First iteration of Fase 2 (CLI parity for the review/PR workflow) — one command at a time, starting with the simplest. Verified output matches raw `git branch --sort=-committerdate` exactly, and confirmed the existing HTTP handler still works unchanged after the extraction. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same pattern as review.branches: extracted the /api/review/prs handler's gh_cmd call into a shared list_prs() fn and exposed it as review.prs on the IPC socket, with `bento review prs [--cwd <dir>]` as its CLI entry point. No unit test needed — list_prs is a one-line wrapper around gh_cmd with no branching logic of its own, matching this file's existing precedent (only pure validation fns like is_safe_branch are tested). Verified output matches `gh pr list --json ...` byte-for-byte. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Extracted the name-status/numstat merge from /api/review/files into a pure, TDD-covered build_file_list() (4 tests: path matching, renames with old_path, missing-numstat defaults, untracked-file appends), wrapped by list_files() which also carries the is_safe_branch() flag-injection guard so IPC callers get the same protection the HTTP handler always had, not just axum ones. Wired up as review.files on the IPC socket and `bento review files [--cwd <dir>] [--base <ref>]` on the CLI. Verified byte-for-byte against `git diff --name-status`/`--numstat` on this branch's real diff vs main, and confirmed the HTTP handler is unchanged. Also added workers/.gitignore for .wrangler/ — it was leaking into every diff as untracked cruft from earlier local TURN testing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same extraction pattern as prs/branches (thin gh_cmd wrappers, no unit test needed — no branching logic of their own). Added `bento review pr diff <n>` and `bento review pr comments <n>`. Also fixed a real bug found while verifying pr diff for real: it was printing the diff as a JSON-escaped string (literal \n, wrapped in quotes) instead of raw text, and panicked with "Broken pipe" when piped through `head`/`less` (extremely likely for a diff-sized command). Added print_text(), which writes raw bytes and exits quietly on a closed pipe instead of panicking — matches how well-behaved Unix text tools behave. Verified both commands byte-for-byte against `gh pr diff`/`gh pr view --json` directly, and confirmed the HTTP handlers are unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Extracted add_comment/update_comment/delete_comment/submit_review from their HTTP handlers, preserving the comment_belongs_to_pr ownership check (now shared via ensure_comment_belongs_to_pr) so IPC callers can't edit/delete a comment across PRs any more than the HTTP handlers could. Added `bento review pr comment/comment-update/comment-delete/submit`. Verified live against the real PR #10: posted a clearly-labeled test comment, edited it, deleted it, and confirmed via `gh api` at each step (including the final 404 after delete). `submit` (approve/request changes) was deliberately NOT tested live — unlike a comment, a submitted review isn't cleanly reversible, so that needs separate explicit sign-off before touching a real PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Extracted ask_handler's checkpoint-lookup + resume/fallback decision into a shared ask() fn (previously inline in the axum handler), reused by both the HTTP SSE endpoint and a new review.ask IPC command. The IPC command follows terminal.subscribe's established pattern: an immediate ack, then push events (review.output per chunk, review.done at the end) on the same connection — no new streaming mechanism needed. Added `bento review ask <question> [--cwd] [--base] [--agent]` with a new stream_review() helper in bento-cli that prints chunks live as they arrive instead of buffering the whole response. Verified the cheap path (no saved checkpoint for the given base) end to end on both CLI and HTTP — identical error, no agent spawned. The path that resumes/runs a real claude/codex/opencode process needs the user's go-ahead before testing live (real cost, real time), so it's implemented and compiles clean but not yet exercised for real. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Found by actually running the live agent path for review.ask (a real claude invocation against a saved checkpoint): the response text came back correct, but the literal string "[DONE]" was appended right after it with no separator, since ask()'s own end-of-stream sentinel chunk was being forwarded as a normal review.output event before the IPC layer's own review.done event fired. The IPC completion signal already exists independently, so the sentinel text itself is now filtered out instead of leaking into what the user sees. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Running `bento` with no arguments now opens a navigable panel (ratatui + crossterm) listing terminals/agents, instead of printing help. Selecting one and pressing Enter attaches inline; when that session ends, control returns to the list — the panel stays running, unlike the standalone `bento attach` (which intentionally hard-exits, see its own comment). This works because crossterm's EventStream (used for all panel input, list navigation and attached-session keystrokes alike) has none of the blocking-OS-thread problem tokio::io::stdin() has: its reader thread blocks on an interruptible mio poll, not an uninterruptible read(), so Drop can always unwind it promptly — confirmed by reading crossterm's own source before relying on it. Since crossterm delivers structured KeyEvents instead of raw bytes, added key_event_to_bytes() (TDD-covered: plain chars, Enter, Backspace, arrows, Ctrl+letter) to translate back to what a raw tty would have produced. Alternate-screen handling required care: a remote program's own alt- screen use (vim, htop) shares one non-ref-counted flag with the panel's, so the panel leaves its own alt-screen before attaching and reasserts it after, instead of assuming nested enter/exit is harmless (it isn't — confirmed against real terminal-emulator semantics before shipping this). Verified live end-to-end against a real vim session inside a pty harness, which caught two real bugs the type checker couldn't: 1. `Terminal::clear()` (called to force a repaint on returning to the list) queries the cursor position via a synchronous DSR read on stdin — racing the EventStream's own concurrent reader on the same fd and hanging until crossterm's read timeout, observed live as the panel dying with "cursor position could not be read within a normal duration" right after a real vim session ended. Fixed by using `Terminal::resize()` to the current size instead — same "next draw repaints everything" effect via a pure ANSI clear write, with no read involved (confirmed by reading ratatui's own source: for a Fullscreen viewport, resize() never touches cursor position at all). 2. (caught and fixed earlier in the same pass) confirmed raw mode is restored and the process exits cleanly on quit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Exposes the existing multi-agent/batched review pipeline (already used by the HTTP SSE endpoint) as `bento review run`, and moves base/branch validation into run_review itself so the IPC caller gets the same protection the HTTP handler already had. Also extracts the duplicated review-stream forwarding between review.ask and review.run into a shared spawn_review_stream helper, and filters the protocol's batch/synthesis/ session sentinels out of the CLI's printed report (into stderr) instead of leaking them as literal text — both issues were caught by running the new command against its own diff as a live test.
Tab from the terminals list into a Review view: browse files changed vs
main, run a full AI review inline with live streaming output (scrollable),
and ask follow-up questions — all through the existing review.* IPC
commands, no new backend surface beyond one addition (see below).
Split tui.rs into tui/{mod,terminals,review}.rs (mod.rs owns the Mode enum
and main loop; terminals.rs is the pre-existing list/attach code, moved
unchanged) — matches this branch's established per-concern-submodule
pattern. Extracted the review.run/review.ask streaming plumbing shared
between the one-shot CLI and the new tab into review_stream.rs (DRY: one
connect/ack/classify implementation instead of two).
Live pty testing against a real agent surfaced a real gap: review.run
never persisted a checkpoint, so a follow-up review.ask always failed with
"no hay análisis guardado" for any review started from the TUI (checkpoints
were previously only ever written by the web panel's own JS). Fixed by
extracting the existing PUT /api/review/checkpoint save logic into
save_checkpoint() and exposing it as a new review.checkpoint_save IPC
command, called once when a run finishes — verified end-to-end (run, then
a real follow-up ask resuming that session, no error).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… tab
Review tab was scoped down too far for a first pass (base fixed to main,
no PR browsing, no way to stop a running review) — the goal is parity with
Bento.app, not a reduced CLI subset, so filling these in now rather than
treating them as a deferred v2. All reuse existing review.* IPC commands,
no new backend surface for browsing:
- b: pick any local branch as the review base (review.branches)
- p: browse open PRs, Enter shows diff + reviews/comments (review.prs,
review.pr_diff, review.pr_comments)
- g: cycle the agent (claude/codex/opencode) used for run/ask
- c: cancel an in-flight run/ask
Cancel surfaced a real gap: aborting the TUI's own streaming task only
stops the client from listening — the daemon-side review.run/review.ask
task (and the real agent subprocess it spawns) kept running unseen and
unbilled-for-nothing. Fixed at both ends: the daemon now tracks each
connection's review task and aborts it when the connection closes (a
cancel just drops the TUI's socket), and every agent Command::new(...) in
review/{mod,ask}.rs now sets kill_on_drop(true) so aborting the task
actually kills the process (tokio doesn't do this by default). Verified
live: started a real review, confirmed the claude subprocess was running
via ps, cancelled, confirmed it was gone.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…w-quality fix Review tab: Enter on a file now shows its diff (r runs the full review, moved off Enter); PR detail gained a (comment), y/n/m (approve/request changes/comment-only review). All reuse existing review.* IPC commands except review.file, a new thin wrapper around the same file_diff logic the web panel's /api/review/file already used. While comparing against the desktop app's real "AI Review" to check for parity gaps, found that the daemon's review pipeline (used by bento review run/ask and now the TUI) was never actually equivalent in quality, not just interaction surface: - build_review_prompt was a simpler, unstructured prompt with no severity levels or verdict — only build_synthesis_prompt had been ported from techReview.ts, and it already referenced a "Veredicto, Resumen, ## Hallazgos" format the per-batch prompt never produced. Ported buildReviewPrompt properly: same 8 categories, severity scale (critical/high/medium/low), verdict (pass/needs_review/fail), and the "read the final state, not just the diff" rule. Lexis context isn't portable (it's an interactive-agent MCP tool), so the prompt instead leans on the agent's own Read/Grep access to the worktree. - That only works if the agent actually runs in the project's directory — run_claude_collecting/resume_claude (the default "claude" agent path) never received `cwd` at all, so every claude-agent review ran with whatever directory the daemon process happened to launch from, not the project being reviewed. Added .current_dir(cwd) to both. codex/opencode were already correct. Verified live: real review against the new prompt shows the format change directly (**Veredicto:** fail, ### [high] file:line — title, **Arreglo:** ...) where the old prompt produced generic unlabeled sections. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…resume Closes the remaining gaps against the desktop Review panel: - x: toggle "compare agents" in the Files view — runs claude+codex+opencode together and synthesizes their reports (review.run already supported a comma-separated agents list; the TUI just never exposed more than one). - c: edit the author context injected into the review prompt, mirroring desktop's "Contexto para la review" textarea (previously always sent ""). - h: browse saved checkpoints (one per base branch reviewed) with relative save time, open one to view without re-running, d to delete. New review.checkpoints/review.checkpoint_get/review.checkpoint_delete IPC commands wrap the same list/get/delete logic the HTTP checkpoint endpoints already used (extracted into list_checkpoint_metas/ get_checkpoint/delete_checkpoint so both transports share one implementation). - Fixed review.ask never actually resuming a codex session: resume_agent special-cased only "opencode" and silently fell back to resume_claude for codex, which can't resume a codex session id. Added resume_codex, args order matching the desktop's own confirmed codex resume invocation (src-tauri/src/agent/mod.rs's build_agent_invocation, verified against its existing test codex_resume_puts_session_and_prompt_last). Verified live: compare toggle and context both reflect in the Files title, checkpoint history lists/opens/deletes correctly against a real checkpoint file. Did not live-test the codex resume fix specifically — round-tripping it end-to-end needs two more real paid agent calls (a codex review, then a follow-up ask) for a narrow, low-risk fix whose only change is call routing plus an arg list already verified byte-for-byte against an existing tested implementation elsewhere in the repo. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replaced the previous "one full-screen view at a time" navigation (separate Files/Branches/Prs screens) with a persistent two-pane layout matching the desktop Tech Review panel: a sidebar (Base/Agent/Comparar status + Ramas/PRs/Historial tabs) on the left, changed files with per-file "revisado" checkboxes and a status filter (All/Added/Modified/ Deleted) on the right. Left/Right arrows move focus between panes; b/p/h switch the sidebar tab in place instead of navigating to a new screen. File diff, PR detail, and running/loaded review output stay as full-screen drill-downs (Esc returns to the split view). Fixed a real bug found live: the sidebar defaulted to the Ramas tab but never fetched branches on entry (only files), so the tab looked selected but stayed empty until `b` was pressed. Extracted the fetch into fetch_branches() and added ReviewState::enter(), called once when Tab-ing in from the terminals list, that populates both files and the default sidebar tab together. Verified live: branches/files/PRs all populate correctly on entry and tab switch, checkbox/filter/counter mechanics work, clean exit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The Review tab was stuck on the cwd `bento` was launched from. It now keeps its own `cwd` in `ReviewState` and a "Proyectos" sidebar tab (`o`) lists the directories of every open terminal/agent — the same source `/api/projects` already used for the phone remote, now shared through a new `projects.list` IPC command instead of a second copy. Also fixes two things the picker surfaced: daemon errors were swallowed into empty lists (an old daemon that doesn't know a `review.*` command looked like "this project has no branches"), and the sidebar header had a fixed height that clipped the agent/compare lines once "Proyecto" was added.
The review prompt existed twice — `src/core/ai/techReview.ts` for the desktop app and `remote/review/prompt.rs` for the daemon (phone remote + CLI) — and the two had already drifted: the desktop sent no project/base line, escaped the diff through JSON.stringify and lacked the trailing verdict kick-off that stops the agent from writing a preamble. Both now call one implementation in `daemon/bento-review`, a crate with no tauri/axum dependency so it stays usable from every transport. `ReviewPromptInput` covers both shapes: project/base/diff always, plus the optional blocks each caller can gather (author context from the daemon, Lexis context and inlined files from the desktop). The desktop reaches it through two new commands, `review_build_prompt` and `review_build_synthesis_prompt`. Prompt-text assertions moved to the crate's tests; the vitest ones now only check the frontend passes what it gathered.
`is_safe_branch` existed twice — once in `src-tauri/src/git/mod.rs` for the desktop's whole git panel, once in the daemon's review module — so hardening it in one place never reached the other. It now lives in `bento-review::vcs` together with `is_safe_relative_path`, the git/gh command runners and the changed-file/branch/diff functions built on them (`parse.rs` moved along as `bento_review::diff`, since `build_file_list` needs it). The shared runners resolve `git`/`gh` through a login shell when they are not on PATH, cached per process. That was a desktop-only workaround for macOS GUI apps not inheriting the shell PATH, and it only covered `git` — a `gh` installed via Homebrew was invisible to the packaged app.
The desktop app and the daemon each had their own `gh` layer, and the overlap
was not as clean as it looked: both had "update comment" and "delete comment"
but on different endpoints — the desktop edits inline *review* comments
(`pulls/comments/{id}`), the daemon edits *issue* comments (the PR
conversation). `bento-review::pr` now holds both families, named for what
they are, plus list/diff/discussion/submit and the inline create/reply.
Two behaviour changes fall out of it:
- The desktop's edit/delete/reply now check the comment belongs to the PR
being reviewed, which only the daemon did. A comment id from another PR in
the same repo was previously edited just as happily, so the commands (and
`ReviewCommentActions`) take the PR number now.
- `review.pr_comments` returns the REST payload (`user.login`,
`created_at`/`submitted_at`) instead of `gh pr view --json`'s. Only the REST
one carries the numeric comment ids that editing and deleting need — the
daemon exposed both operations while handing clients a payload with no ids
to call them with. The phone UI and the TUI formatter follow the new shape.
`git/pr.rs` drops from 445 to 200 lines, with one `blocking` helper instead of
the same spawn_blocking + double map_err in every command.
The sidebar is ~40 columns, so clipping the project line from the right left "/Users/romangomez/Desktop/roma" on screen — the half that says nothing about which project is loaded, and identical for every project under the same tree. It now drops leading segments instead: "…/roma/bento/daemon".
Each codebase drove claude/codex/opencode its own way and each knew things the other didn't. `bento-review::agents` is the union: one argv builder, one stateful parser per agent, one run loop. What each side gains: - The daemon (phone remote + CLI) now runs reviews read-only. It launched `claude -p` with default tools, so a review from the TUI or the phone could edit the code it was reviewing, while the desktop passed `--allowedTools Read,Glob,Grep`. Same for opencode's `--agent plan`. - The daemon resolves the agent binary outside PATH and caps line/total output size, both of which only the desktop did. - The desktop reads Claude's streaming deltas and Codex's `session_meta`, which only the daemon did, and resolves built-in agents instead of running them by bare name. Claude can report the same text twice — as deltas and again in the final assistant message — so the parser is stateful and drops the final message once deltas have been seen. Six near-identical spawn+parse loops in the daemon (three for reviews, three for follow-up questions) collapse into one call: review/mod.rs goes from 747 to 526 lines and ask.rs from 257 to 131. The desktop's five adapter files are gone; its tests moved to the crate, where they now cover both dialects.
review.rs had grown to 927 lines of implementation doing four unrelated jobs at once, and I had just made it worse by adding the project picker to it. The desktop panel it mirrors was already split this way (ReviewPanel / ReviewDiffView / reviewDataLoader / reviewFormat), so this follows the convention the repo already had. mod.rs 358 state and the review.* IPC calls input.rs 277 one key handler per view draw.rs 235 rendering format.rs 164 pure helpers, with their tests next to them No behaviour change: same tests, plus a smoke run of the real TUI against a live daemon (Tab → p still lists PR #10).
run_review lived in the daemon and spoke its transport's wire format directly, emitting "[BATCH:1/2]", "[SYNTHESIS]" and "[ERROR] …" strings from inside the orchestration. The engine now emits a typed ReviewEvent and the daemon translates those into the same markers, so the phone, the CLI and the TUI see exactly the protocol they saw before. Splitting it that way made the orchestration testable for the first time: - `plan_stages` is pure — one stage per agent when comparing, one per batch of the diff otherwise — so what each agent gets asked is now covered by tests instead of being implied by a 120-line function. - The agent call is behind an `AgentRunner` trait, so the batching order, the synthesis trigger and the failure paths are tested with a fake runner rather than by launching claude. Gathering the diff moved to `vcs::review_diff` (it was raw tokio Commands inline), and `parse_agents` came along with the engine. review/mod.rs is down to 385 lines, from 1083 when this started.
The desktop kept finished reviews in the browser's localStorage while the daemon kept them in ~/.bento/review-checkpoints, so a review run in the app did not exist for the TUI or the phone — and vice versa. Both now write the same store, which lives in `bento_review::checkpoints`; the daemon keeps only its HTTP handlers. The record gained two optional fields the desktop had and the daemon didn't (`branch` and `commit`, so a stale review can be spotted), and a checkpoint written before they existed still loads — there is a test for exactly that, and the daemon read a desktop-shaped record end to end. Reviews saved in the old localStorage are not lost: `loadReviewCheckpoint` reads the shared store first and falls back to the browser copy.
…eir folders Two things the file audit turned up. The `invoke` calls I added to `src/core/ai/techReview.ts` this morning broke the rule the rest of the codebase follows: `core/` holds logic, the panels and adapters talk to Tauri. `buildReviewPrompt`/`buildReviewSynthesisPrompt` moved to `panels/review/reviewPrompts.ts` and `loadReviewCheckpoint` to `panels/review/reviewCheckpoints.ts`; techReview.ts keeps what is actually TS logic (context provider, review document, checkpoint parsing) and no longer imports Tauri at all. And eleven test files sat outside the folder mirroring their source — tasks, db, terminal and tv panel tests flat in tests/panels/, the command palette test under tests/core/, the i18n catalog test under tests/core/. All 158 test files now mirror their source; 1253 tests still pass.
src/ui was 15 flat files where nothing said which ones render and which are utilities. Now the root holds only what draws something (agentStatusBar, aiChat, askAi, collapsibleSidebar, commandPalette, contextMenu, panelLauncher, resizablePane, windowControls), helpers/ the pure ones (icons, platform, zoom), and state/ the persisted UI state (activeProject, decorationsPreference). Their tests moved with them. aiKeys.ts left ui/ entirely: it reads and writes AI keys in the Vault through Tauri and has no UI in it, so it belongs with the other Tauri-backed access in adapters/. Also folded src-tauri/src/agent/tests.rs into its module as an inline `#[cfg(test)] mod tests` — it was the only separate test file among 51.
A file never gets big in one commit — it grows a bit at a time and nobody notices, which is exactly how tui/review.rs reached 927 lines with me adding to it this morning. `npm run check:size` (now part of ci:local) fails when a file crosses 400 lines of implementation. The 17 files already over it are recorded in file-size-baseline.json with their current size: they don't block, but they can't grow either. Splitting one means lowering its number or deleting the entry. Implementation lines exclude blanks, comments and — in Rust — the inline `#[cfg(test)]` module, so writing tests never counts against you.
The mark lived in a HashSet that refresh_files() cleared, so it vanished on every reload — useless for the one thing it exists for, working through a big diff over time. It is now filed per project and ref under ~/.bento/review-viewed, next to the checkpoints, through two IPC commands (review.viewed / review.viewed_set). `bento_review::store` came out of it: checkpoints already had the FNV-1a naming for a (cwd, ref) pair inline and the second store would have copied it. A corrupt or unreadable file reads as "nothing reviewed" — the cost of being wrong is re-reading a diff. Adding the two commands pushed ipc.rs past the size check added a commit ago, so the `review.*` arms moved to ipc/review.rs: 290 + 196 lines instead of 407 in one match. The check earned its keep on its first day. Verified live: marked a file, restarted the TUI, still 1/119.
`/` opens a search box in whatever you are looking at: it filters the branch list in the sidebar, and the lines of a file diff, a PR or a finished review. In a thousand-line diff that is the difference between reviewing and giving up. Both are pure functions on the state (`visible_branches`, `filtered`) and both have tests — including the case that matters, a search matching nothing leaving an empty list rather than the whole one.
Reviewing a branch created a worktree, mirrored the uncommitted work into it and made it read-only while the agent looked — but only in the desktop app. A review from the TUI or the phone ran against the live working tree: the agent could touch the code you are writing, and what it was reviewing moved under its feet. The worktree module moved from src-tauri into bento-review (the app now re-exports it for its commands) and the engine uses it: when a branch is given it prepares the isolated copy, runs there, and releases it afterwards whatever happened. `uuid` did not come along — a worktree's unique suffix is now nanoseconds plus the pid, which is enough for "two reviews of the same branch at once" and saves the crate a dependency.
…he agent used Two things the desktop had and the shared engine did not, so a review from the TUI or the phone was worse in ways that matter: - **Retry.** A rate limit or a dropped connection killed the whole stage. It now retries once. A timeout is not retried — it means the work did not fit the window, so repeating it just burns another one. `agents::is_retryable` is the same rule as the desktop's `isRetryableReviewError`, with its tests. - **Tool use.** The agents module already parsed tool events and the runner threw them away. They now reach the caller as `ReviewEvent::Tool`, which is the only thing you can see while an agent thinks — and the evidence of what it looked at before saying what it says. The TUI shows them as progress, the phone in its header, and neither lets them into the report text. This is the other half of "one engine": rather than ripping out the desktop's orchestration and losing its retries, evidence and parallelism, the engine grows to match it. What is still missing before the desktop can switch: parallel multi-agent runs and the Lexis context.
…ritten exposure policy `tasks.list` over IPC and `GET /api/tasks` answer "which branch is each task on, and on what commit" — the first thing you want to know from outside the app. Read-only: creating, deleting or rebasing a worktree stays in the app, where there are confirmations, history and backups. The worktree parsing moved to `bento_review::worktrees` with its tests, including the detached case that has no branch. docs/remote-exposure.md writes down what is exposed and what is not — vault, databases and task writes, each with the reason — plus the three questions to answer before adding anything to that surface. The list existed only in my head and in this conversation, which is not where it should live. Also fixed the unique suffix for isolated worktrees: nanoseconds plus pid collided when two ran in the same nanosecond (a test caught it), so it now carries a per-process counter too.
`bento tasks` lists each task (worktree) with its branch, what it has
uncommitted and how far it has drifted from its upstream. `new`, `rm`,
`commit`, `sync` and `push` do the rest. All of it goes through
`bento_review::tasks`, so the logic is the crate's, not the CLI's.
list worktrees + `status --porcelain` + ahead/behind, per task
create a branch in its own worktree next to the repo
remove refuses to drop unsaved work unless you say --force
commit stages everything and commits, --amend rewrites the last one
sync fetch + rebase --autostash onto the upstream (rebase, not merge:
a task branch is for a PR, and a merge back only adds noise)
push --force means --force-with-lease, which never overwrites what you
have not seen
Reads are exposed over HTTP too (the phone already lists tasks); writes only
over the local IPC socket, as docs/remote-exposure.md says.
**`is_safe_branch` let a bare flag through.** `--upload-pack=…` was rejected
for its `=`, but `--force` or `-n` passed and git reads those as options, not
as a branch name. Writing the task-name check is what surfaced it. Fixed with
its own test, and it protects every caller of the shared validator — review
and diffs included.
Verified end to end against a live daemon: created a task, committed in it,
listed it, removed it and cleaned up the branch.
The last two pieces of the tasks panel that only the desktop had, and the two
that matter most when something goes wrong.
bento_review::rebase plan validation, start, continue, abort, split,
preserve-merges, and where it stopped
bento_review::backup the automatic ref before anything that rewrites
history, the bounded per-branch history, and restore
`src-tauri/src/git/{rebase,backup}.rs` are now thin commands over them — 34 and
30 lines instead of 381 and 205 — and the crate carries their tests, plus the
test_support that builds real repos.
The plan validation got its own tests because it is the sharp edge: every line
of a rebase todo ends up inside git's, so an unknown action (`exec rm -rf /`),
a commit outside the range, or a newline smuggled into one instruction are each
rejected with a reason. The sequence-editor quoting too — a path with a space
or a quote cannot break out of the shell command git builds.
In the CLI: `bento tasks rebase <base>|status|continue|abort`, `tasks backups`
and `tasks restore [<ref>]`. Without an explicit plan the rebase is all picks,
which is what you want nine times out of ten.
Verified against a real repo: a rebase that conflicts pauses and `rebase
status` says where and on which file, aborting leaves the original content
untouched, the backup ref is there afterwards and restore lands on it.
Writes go over the local IPC socket only — docs/remote-exposure.md updated to
say so rather than "not exposed".
Moving BackupEntry, BackupStatus, RebaseStatus and WorktreeInfo into the crate quietly broke the binding: ts-rs only writes a type where the derive is, so the four .ts files stopped tracking their structs. Nothing failed — the files were still there, just frozen — which is the worst way for a guarantee to break. `ts-rs` is now an optional dependency of bento-review behind a `ts` feature that only the desktop app turns on: the daemon and the CLI do not build it. `bindings:generate` runs the crate's export tests too. Two details the move had eaten: the export path is relative to the workspace root, not the crate, and `created_at` needs `ts(type = "number")` or a u64 becomes a bigint in TypeScript and `new Date(...)` stops accepting it — which is exactly what tsc then said.
What the tasks panel still had of its own was already in bento-review, so it now calls it: worktree listing and parsing, the status counts, ahead/behind and the sync state, removing a worktree, and pushing. Two of them were better in the app than in the crate, so the crate grew rather than the app losing anything: - **Removing.** If the `.git` file inside a worktree broke, git refuses and the folder stays forever. The app repaired the link and retried; the crate now does too — and the trigger is wider than the app's was, because git says "validation failed", not only "not a working tree" (a test with a genuinely broken link is what showed it). What did not widen: unsaved work still blocks the removal, which has its own test so the repair path can never quietly force it. - **Pushing.** The first publish uses the branch name rather than HEAD, so the upstream points at something named, and a detached HEAD is refused. The two worktree parsing tests (bare entries, Windows CRLF with a space in the path) moved to the crate with the parser. src-tauri/src/git/worktree.rs is 60 lines, from 164.
A new crate, `bento-docker`, with what the Docker panel does and nothing of how it looks: resolving the binary (through a login shell, because a Homebrew docker is invisible to a GUI app on macOS), listing containers, parsing `docker ps` into typed ones, the start/stop/restart lifecycle, logs, and compose up/down. bento docker containers, running and stopped bento docker logs <name> [--tail n] bento docker start|stop|restart <name> GET /api/docker read-only, for the phone Reads go over HTTP; the lifecycle only over the local IPC socket. Stopping the production database from a phone should not be one token away — docs/remote-exposure.md says so now. **Same flag hole as `is_safe_branch`, found the same way.** `is_safe_container` allowed a leading dash, so `--volumes` passed for a container name and docker would read it as an option. A test asserting it should fail is what caught it. Docker itself does not allow a name to start with `-`, so rejecting it is also what the tool expects. `Container` is now defined once in Rust and exported to TypeScript, so the shape cannot drift; the panel keeps parsing the raw text it already receives. Adding the route pushed remote/mod.rs over the size check again, so the read-only handlers (projects, tasks, docker) moved to remote/inventory.rs — which is a better name for them than "the file with the server in it".
…ubnet into bento-docker
…top as wrappers branches, log, commit, edit, status, sync, recommend, pr status/view/create and worktree add/remove ya no tienen lógica en src-tauri: viven en la crate, con sus tests, y el escritorio solo envuelve. src-tauri/src/git pasa de ~1770 a 606 líneas. Se van paths.rs (a bento_review::edit) y test_support.rs (ya está en la crate).
tasks.status, tasks.diff, tasks.log y tasks.upstream salen ahora por el socket IPC (local), y el CLI trae `bento tasks status|diff|log`. Son lecturas: por HTTP sigue yendo solo la lista — ver docs/remote-exposure.md.
…to-review El markdown de la review, el resumen de la rama, la sesión de seguimiento y qué fallo merece reintento vivían en TS (y la lista de reintentos ya estaba duplicada en Rust). Ahora están en `bento_review::report` y `agents::is_retryable`, con sus tests; el panel los llama por `review_build_document`, `review_build_overview`, `review_follow_up_session` y `review_is_retryable`. De paso se borra buildReviewFileBatches, que ya no usaba nadie.
…e from the CLI Localizar la sesión de cada agente (Claude, Codex, OpenCode) y el scrollback que Bento guarda salen de src-tauri y pasan a la crate `bento-sessions`, con sus tests. El CLI trae `bento agent sessions` (lo que se puede retomar aquí) y `bento agent resume <agente> <id> [--attach]`, que abre el PTY por IPC. El id llega de fuera y acaba en una línea de comandos, así que se rechaza vacío, con guion inicial o con cualquier cosa que no sea [A-Za-z0-9-_]. De paso, `bento agent` se parte a commands/agent.rs.
Entrecomillar nombres y escapar literales estaba escrito dos veces, con reglas distintas por motor en cada lenguaje. Ahora `bento_db::query` arma el SQL que el panel enseña y ejecuta —consulta de ejemplo, relaciones, camino de JOINs, INSERT, vaciar celda y el LIMIT de seguridad— con 14 tests, y valida los nombres antes de concatenarlos (antes el lado TS no validaba nada). Se van sqlQuote.ts, pgIdents.ts, rowLimit.ts y joinPath.ts, y la tercera copia de la detección de LIMIT que vivía suelta en dbQueryExec.ts. Los tests del panel comprueban ahora que pide la sentencia correcta y ejecuta la que recibe.
Importar desde Rust (`memory_source_import`) solo miraba el `external_id`, mientras que el panel además detectaba duplicados semánticos y los fusionaba: la misma operación dejaba resultados distintos según por dónde entrara, y por el lado de Rust duplicados que el panel habría fundido. La regla vive ahora en `bento_memory::dedup` (clave semántica, contención, fusión y el plan skip/merge/create) con 9 tests, y la usan las dos. En TS se van memoryImportPlan.ts y la mitad de duplicados de normalize.ts; el panel entra por `memory_plan_import` y `memory_merge`. Los tests del runner y de la vista previa dictan ahora la decisión en vez de recalcularla.
Había tres criterios distintos para lo mismo: classifyPrCheck (panel de tareas), computeCiStatus (panel de review) y format_checks (TUI). Un action_required salía como "pasa" en uno, "pendiente" en otro; un skipped, "pasa" en el panel y "pendiente" en el TUI. Ahora lo decide `bento_review::pr::check_verdict`, con tests, y lo usan los tres. Un action_required cuenta como fallo: GitHub no deja mezclar con él, así que contarlo como que pasa era la lectura peligrosa. El informe (veredicto por check y recuento) viaja ya dentro de PrStatus, así que el panel de tareas no pregunta de más; el de review usa gh_pr_check_report. Se van core/git/prChecks.ts, computeCiStatus y core/git/reviewFiles.ts, que no lo usaba nadie.
El panel pedía el log, las dos recomendaciones y los ficheros de cada commit por separado (N+3 llamadas) y luego combinaba las tres señales con sus propios pesos. Ahora `bento_review::recommend::fixup_targets` lo hace de una vez, con tests, y el panel hace una sola llamada. Los códigos del porcelain que significan conflicto estaban en dos sitios: dentro de `rebase::status` y en conflictWorkflow.ts. Ahora los dice `bento_review::status::parse_conflicted`, que usan los dos. Se van changedPaths, matchingPaths y rankFixupCandidates de commitWorkflow.ts, y parseConflictFiles de conflictWorkflow.ts.
`tasks.fixup` sale por el socket local y el CLI trae `bento tasks fixup [--base <ref>] [<fichero>…]`: a qué commit propio le pega lo que tienes sin commitear, y por qué (ficheros en común, blame, historial). Sin parche explícito usa el diff del worktree, que es el caso normal. Probándolo salió un fallo del propio CLI: al recoger los ficheros sueltos se colaba el valor de `--cwd` como si fuera uno, y entonces el historial marcaba todos los commits. Ahora hay un `positional()` que sabe qué opciones se llevan su valor, con tests.
…ies it
El panel armaba el parche por trozos en TypeScript y Rust lo aplicaba con
`--unidiff-zero` justo porque esos trozos van tal cual salen del diff original:
media regla a cada lado de una pareja que tiene que ir junta. Ahora lo hace
`bento_review::diff::{parse_file_patch, build_selected_patch}`, con 6 tests.
Partir el fichero en trozos también pasa por ahí, y no solo por limpieza: quien
pinta las casillas y quien arma el parche cuentan los trozos con el mismo código,
así que no se puede marcar uno y commitear otro. commitWorkflow.ts se queda en 7
líneas.
… were loose El panel de Jira y el del móvil se quedaron fuera de la pasada de i18n: 65 de los 82 strings anotados eran suyos. Ahora salen del catálogo (`panels.jira` sube a 55 claves, `panels.remote` es nueva), en español y en inglés. De paso caen los sueltos de aiChat, lazyPanel, AgentsPanel y TV. Quedan 12 anotados y ahí se quedan: no son prosa. Son el nombre del producto, el literal SQL NULL, los valores de un filtro (all, commented), una rama por defecto y un árbol de ficheros en ASCII — traducir eso los rompe. Lo dice el comentario de audit-i18n.mjs, para no volver a mirarlo.
"claude/codex/opencode" estaba escrito en seis sitios: el catálogo del panel, el tipo y el validador de chatHistory, la constante del TUI, dos constantes del bundle del móvil y tres <select> del HTML. Ahora la lista es `bento_review::agents::AGENTS` y de ahí salen todos. Lo que impide que vuelvan a separarse: - El tipo `AgentType` de TypeScript lo genera ts-rs desde `AgentId`, así que añadir uno en Rust rompe la compilación del panel hasta que tenga etiqueta (comprobado añadiendo un `Gemini` de prueba: el typecheck falla). - Un test ata la lista al `match` de `invocation`: no se puede ofrecer un agente que luego no se sepa lanzar. - El daemon sirve /agents.js generado desde el catálogo, y un test comprueba que ni el HTML ni el bundle vuelven a escribir la lista. Al hacerlo salieron dos copias que no había contado: el tipo y el validador de sessionAgent en chatHistory.ts.
…behind Los procesos node que se acumulaban durante días eran este hook. El resumidor se mataba con SIGTERM y se esperaba a su evento `close`: un agente que ignora SIGTERM no lo emite nunca, así que la promesa no resolvía y el proceso se quedaba vivo hasta que saltara el temporizador de 300 s. Y cuando saltaba, era peor: `process.exit(0)` mataba a node y dejaba al agente suelto, con sus propios hijos, para siempre. Ahora el resumidor va en su propio grupo de procesos, se escala a SIGKILL tras una gracia y se resuelve pase lo que pase; y el temporizador del hook corta a los hijos antes de salir. Tres tests con agentes que se portan mal a propósito: uno que ignora SIGTERM y otro al que hay que cortar a media faena.
… not
Los tres tests del cajón de Jira buscaban botones por su texto en español
('Guardar', 'Comentar'). Mientras el panel estaba sin traducir daba igual el
idioma; desde que sale del catálogo, el panel se pinta en el que esté activo.
Y cuál estaba activo dependía del orden de los ficheros: varios hacen
stubGlobal del localStorage y eso se filtra entre ellos. Sin localStorage
utilizable, getAppLocale() devuelve español (en local pasaba); con uno vacío cae
a navigator.language, que en happy-dom es en-US (en CI fallaba).
Un tests/setup.ts fija el idioma antes de cada test, así que ya no depende del
orden. Las aserciones de Jira usan ahora el catálogo y pasan en los dos idiomas.
De paso salió otro test que heredaba el inglés de un test anterior sin pedirlo.
Comprobado con la suite entera en español, en inglés, y tres pasadas en orden
aleatorio.
…king target/ El test del resumidor lanzaba scripts .sh, que en Windows no arrancan (spawn UNKNOWN, errno -4094). Y el código de producción tampoco valía allí: `process.kill(-pid)` mata un grupo de procesos, que es de Unix. Ahora la decisión es una función pura por plataforma —taskkill /t en Windows, grupo en Unix— probada en las dos, como ya hacía crossPlatformProcess. Lo que lanza procesos de verdad usa agentes en Node en vez de shell, y se salta en Windows, donde no se puede ignorar un SIGTERM. Los busca por un nombre único: con el fijo, un resto de una ejecución anterior contaba como vivo y el test iba y venía. Y aparte: `npm run test:coverage` se quedaba sin memoria con 6 GB de heap. No era de esta rama —pasa en cualquiera— sino de `coverage.all`, que recorre el proyecto y se metía en target/, 80 GB de artefactos de cargo en cuanto compilas la parte Rust. En CI no se ve porque allí target/ está vacío.
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.
No description provided.