fix(terminal): retry Ctrl+C for processes needing multiple SIGINT (#266)#272
Merged
edelauna merged 4 commits intoMay 26, 2026
Merged
Conversation
proyectoauraorg
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds bounded retry logic to TerminalProcess.abort(): sends an initial Ctrl+C and, if needed, starts a guarded async loop that re-sends Ctrl+C at fixed intervals up to a configured limit, stopping early when the process/terminal state changes. Tests exercise immediate send, retries, early stop, reuse, and idempotency cases. ChangesBounded Ctrl+C Retry for Terminal Cancellation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/integrations/terminal/TerminalProcess.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/integrations/terminal/__tests__/TerminalProcess.spec.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
edelauna
reviewed
May 24, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (1)
docs/plan/plan-accion-top5.md (1)
348-370: ⚡ Quick winAlign the PR template with the repository’s required PR structure.
The template here omits required sections used by the repo template (notably “Related GitHub Issue”, “Test Procedure”, and the pre-submission checklist). Aligning it will prevent contributors from opening non-compliant PRs.
🤖 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 `@docs/plan/plan-accion-top5.md` around lines 348 - 370, Update the PR template block (the markdown snippet containing "## Descripción" and "## Testing") to include the repository-required sections: add a "Related GitHub Issue" section (explicitly list issue numbers/links), replace or expand "## Testing" into a "Test Procedure" with step-by-step reproduction and expected results, and append the pre-submission checklist (e.g., CI checks, changelog, approvals) so the template enforces the repo's required PR structure; edit the template headings and bullet lists inside the existing markdown snippet to add these sections and example checklist items so contributors cannot omit them.
🤖 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 `@docs/analisis-multidimensional-2026-05-22.md`:
- Around line 50-54: The markdown has multiple fenced code blocks without
language identifiers (MD040) — e.g., the block containing "Cline → Roo-Code →
Zoo-Code (upstream/main) ↓ zoo-code-contrib (proyectoauraorg's fork)"; update
each fenced block by adding an explicit language tag appropriate to its content
(for plain diagrams or arrows use text or mermaid if it's a diagram, for config
use json, for snippets use typescript, etc.), ensuring every triple-backtick
fence in the document has a language token so the linter stops flagging MD040
and rendering is consistent.
In `@docs/analysis/hallazgos-generales.md`:
- Around line 18-38: The fenced code blocks in the directory tree snippet (the
block showing "Zoo-Code-contrib/" with apps/, packages/, webview-ui/,
src/api/providers/) are missing language identifiers which triggers MD040;
update each fenced block mentioned (the ones around the directory tree and the
other blocks noted) by adding an appropriate language tag such as ```text or a
more specific tag like ```bash so the markdown linter recognizes the block type,
keeping the existing content unchanged and only adding the language token to the
opening ``` markers.
In `@docs/collaborators/perfiles-colaboradores.md`:
- Around line 102-106: The fenced code blocks in perfiles-colaboradores.md are
missing language identifiers and trigger markdownlint MD040; update each
triple-backtick block (the three blocks showing timelines, contribution
percentages, and collaboration arrows) to include a language tag such as text
(e.g., change ``` to ```text) so all fenced code blocks are annotated and the
lint warning is resolved.
In `@docs/collaborators/registro-proyectoauraorg.md`:
- Around line 23-34: Several fenced code blocks containing the commit-list lines
(for example the block starting with "19ab9503d index on
feat/80-mimo-models-integration: 7b171247d" and other similar commit-list
blocks) are untyped; update each opening fence from ``` to ```text for every
such block (the ones flagged around the commit lines and the blocks covering the
content at the other flagged locations) so all fenced blocks have an explicit
language marker and satisfy MD040.
In `@docs/github-participacion-y-caso-193.md`:
- Around line 27-30: The Markdown file docs/github-participacion-y-caso-193.md
contains unlabeled fenced code blocks (markdownlint MD040); update each
unlabeled ``` fence to an appropriate language tag to improve rendering and
linting — for the snippet showing main:src/core/webview/diagnosticsHandler.ts:76
(and the line containing const tempFileName = `roo-diagnostics-${taskId.slice(0,
8)}-${timestamp}.json`) mark that fence as ```ts, change plain log/console/text
blocks to ```text, and change Markdown content blocks to ```md; apply these tag
additions to the other unlabeled blocks mentioned (lines 44-50, 63-65, 68-94,
126-140) so each fenced block has the correct language identifier.
- Around line 17-19: The doc lists two conflicting scopes for issue `#193` (one
saying diagnostics temp filename change: diagnosticsHandler.ts, prefix
"roo-diagnostics-" → "zoo-diagnostics-"; the other saying a class rebrand in
content-styles.ts: prefix "roo-" → "codai-"); pick the canonical change that
matches the actual patch-prep artifacts in the repo and update this doc
accordingly: if the PR modifies content-styles.ts, replace the `#193` entry and
any references in lines ~58-65 to describe the content-styles class rebrand
("roo-" → "codai-") and remove the diagnostics temp-filename mention; if the PR
actually changes diagnosticsHandler.ts, instead update the patch-prep references
to reflect the diagnostics filename prefix change ("roo-diagnostics-" →
"zoo-diagnostics-") and remove the content-styles rebrand mention; ensure the
issue number (`#193`), filenames (diagnosticsHandler.ts or content-styles.ts) and
exact string prefixes ("roo-diagnostics-", "zoo-diagnostics-", "roo-", "codai-")
are consistent across the document.
In `@docs/OPP-001-mimo-tests-plan.md`:
- Around line 558-559: The sentence incorrectly states that completePrompt calls
this.client.chat.completions.create() with stream: false explicitly; update the
wording to say that completePrompt operates as a non-streaming call because it
uses a non-streaming request shape (rather than claiming it sets stream: false),
and keep the note that the mock is reset in beforeEach; reference completePrompt
and this.client.chat.completions.create in the reworded sentence to make the
distinction clear.
- Around line 95-96: Update the docs to reflect reality: the MiMo handler
currently sends extra_body: { thinking: { type: "enabled" } } despite MiMo not
supporting native reasoning; change the guidance in OPP-001-mimo-tests-plan.md
so it does NOT claim tests should avoid `thinking` params and instead state that
`convertToR1Format` and src/api/providers/mimo.ts populate
`reasoning_content`/`extra_body.thinking` for compatibility only; reference the
`convertToR1Format` transformation and the `extra_body` / `thinking` field
emitted by `src/api/providers/mimo.ts` so tests are aligned with the runtime
contract.
- Around line 423-447: The fenced code block that lists the mimo.spec.ts test
plan lacks a language tag which triggers markdownlint MD040; update that
specific fenced block (the one starting with the line "mimo.spec.ts (estimado:
~950-1000 líneas)") by adding a language identifier (e.g., replace ``` with
```text) so the block is tagged (text) and MD040 is satisfied while preserving
the existing content and formatting.
In `@docs/opportunities/oportunidades-contribucion.md`:
- Around line 36-49: The doc references outdated test filenames (.test.ts);
update the mentions in oportunidades-contribucion.md to match the repo
convention (.spec.ts) so they point to actual files—replace
`src/api/providers/__tests__/mimo.test.ts` with
`src/api/providers/__tests__/mimo.spec.ts` and
`src/api/providers/__tests__/anthropic.test.ts` with
`src/api/providers/__tests__/anthropic.spec.ts`; also scan the same doc and any
other docs for other .test.ts references and correct them (or create matching
.spec.ts tests if they don’t exist) so the execution plan doesn’t contain dead
file paths.
- Around line 45-49: Update the fenced code blocks in the markdown so each
opening ``` has an explicit language tag (e.g., ```text, ```bash, or
```markdown) to satisfy MD040; specifically add language identifiers to the
blocks containing the filenames "src/api/providers/__tests__/mimo.test.ts",
"src/api/providers/mimo.ts", and "src/api/providers/__tests__/anthropic.test.ts"
and the other fenced blocks at the noted ranges (lines referenced in the review:
109-112, 168-171, 196-199, 289-302, 308-326) so lint no longer reports missing
language tags.
In `@docs/plan/plan-accion-top5.md`:
- Around line 116-123: Update the roadmap checklist items that are future
checkpoints from checked to unchecked so they don't appear completed: in the
"Checkpoint Fase 1" block change each "[x]" to "[ ]" (items like "4-6 PRs
creados", "2-3 PRs mergeados", "5+ code reviews realizados", "2+ bugs
resueltos", "1 guía de documentación creada", and the "Score estimado" line),
and make the same replacement for the other checkpoint blocks referenced (the
ranges around lines 180-187 and 227-234) so all future checkpoints use unchecked
boxes until actually completed.
- Around line 47-69: The fenced code blocks in docs/plan/plan-accion-top5.md
(for example the ASCII table block starting with "META: 2-3 PRs pequeños +
presencia en issues" and all other blocks referenced in the review) lack
language identifiers which triggers markdownlint MD040; update each
triple-backtick fence to include an appropriate language tag (e.g., ```text for
ASCII diagrams, ```bash for command examples, ```markdown for nested markdown
snippets) for the blocks at the reported ranges (47-69, 72-91, 94-114, 131-149,
152-166, 169-178, 195-208, 211-225, 331-346, 392-399) so linting passes.
In `@docs/PR_DESCRIPTION.md`:
- Around line 40-41: The coverage table row for "completePrompt" is inconsistent
with the run output; update the table in PR_DESCRIPTION.md so the
"completePrompt" count matches the actual results (change the displayed value
from 9 to 8) or, if the run output is wrong, regenerate the test summary and
update the table accordingly; locate the "completePrompt" row in the markdown
table and ensure the Total row still sums correctly.
- Around line 46-47: The docs snippet includes a local absolute path string "RUN
v3.2.4 /Users/dr.armandovaquera/Zoo-Code-contrib/src" which leaks a
user-specific path; replace that exact output with a sanitized project-relative
path (for example "RUN v3.2.4 ./src" or "RUN v3.2.4 <project-root>/src") so
shared test output no longer contains local environment identifiers. Ensure the
replacement appears in PR_DESCRIPTION.md wherever the "RUN v3.2.4
/Users/dr.armandovaquera/..." line occurs and preserve the rest of the output
formatting.
- Around line 45-99: The fenced test-output block in PR_DESCRIPTION.md is
missing a language tag causing markdownlint MD040; update the triple-backtick
fence that wraps the test output to include a language marker (e.g., change ```
to ```text) so the block is explicitly marked as plain text.
In `@docs/SOT-estado-consolidado-2026-05-20.md`:
- Around line 183-193: The fenced checklist block starting with ``` on the lines
containing the checklist items is missing a language tag (causing MD040); edit
that fenced code block (the block containing items like "git fetch origin && git
push origin main" and the PR checklist entries) and change the opening fence
from ``` to ```text or ```bash so the block has a language identifier and
markdownlint MD040 is resolved.
- Line 53: En el documento hay un typo en el handle/name "roomote" (aparece en
la fila de la tabla que contiene "Fix PR `#212` (CHANGES_REQUESTED) ..." y en otra
aparición más abajo); reemplaza ambas ocurrencias de "roomote" por el handle
correcto (por ejemplo "remote" o el username confirmado) manteniendo el formato
de la tabla/registro y verifica la coherencia en las dos apariciones antes de
commitear; si no estás seguro del nombre correcto, confirma con el autor del PR
y actualiza ambas instancias simultáneamente.
In `@patches/193-pr-description.md`:
- Around line 21-24: Add a blank line between the bullet and the markdown table
to satisfy MD058 and correct the typo "revertable" to "revertible"; update the
table row that replaces `roo-code-block-container` with
`codai-code-block-container` accordingly and make the same blank-line + typo fix
at the other occurrence noted (the second instance around the 52-52 range).
Ensure the table spacing is standard Markdown (blank line before the table) and
the word "revertible" is used everywhere it previously appeared as "revertable".
---
Nitpick comments:
In `@docs/plan/plan-accion-top5.md`:
- Around line 348-370: Update the PR template block (the markdown snippet
containing "## Descripción" and "## Testing") to include the repository-required
sections: add a "Related GitHub Issue" section (explicitly list issue
numbers/links), replace or expand "## Testing" into a "Test Procedure" with
step-by-step reproduction and expected results, and append the pre-submission
checklist (e.g., CI checks, changelog, approvals) so the template enforces the
repo's required PR structure; edit the template headings and bullet lists inside
the existing markdown snippet to add these sections and example checklist items
so contributors cannot omit them.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f602cb04-2d16-4819-8349-e99cfc19e53f
📒 Files selected for processing (27)
Zoo-Code-contrib.code-workspaceapps/web-evals/tsconfig.tsbuildinfodocs/.github/PR_DESCRIPTION.mddocs/OPP-001-mimo-tests-plan.mddocs/PR_DESCRIPTION.mddocs/SOT-estado-consolidado-2026-05-20.mddocs/analisis-multidimensional-2026-05-22.mddocs/analysis/README.mddocs/analysis/hallazgos-generales.mddocs/collaborators/perfiles-colaboradores.mddocs/collaborators/registro-proyectoauraorg.mddocs/github-participacion-y-caso-193.mddocs/opportunities/oportunidades-contribucion.mddocs/plan/plan-accion-top5.mddocs/sesions/roo_task_may-20-2026_2-53-49-pm.mddocs/sesions/roo_task_may-20-2026_2-55-38-pm.mddocs/sesions/roo_task_may-20-2026_7-00-47-pm.mddocs/sesions/roo_task_may-21-2026_12-47-25-am.mdpatches/193-commit-message.txtpatches/193-pr-description.mdpatches/193-submission-checklist.mdsrc/integrations/terminal/TerminalProcess.tssrc/integrations/terminal/__tests__/TerminalProcess.spec.tstemp/issue-1.mdtemp/issue-2.mdtemp/review-12364.mdupstream-sync/sync.sh
✅ Files skipped from review due to trivial changes (9)
- apps/web-evals/tsconfig.tsbuildinfo
- patches/193-commit-message.txt
- docs/analysis/README.md
- patches/193-submission-checklist.md
- upstream-sync/sync.sh
- temp/review-12364.md
- temp/issue-2.md
- docs/.github/PR_DESCRIPTION.md
- temp/issue-1.md
Comment on lines
+102
to
+106
| ``` | ||
| Mayo 2026: ██████████████████████ Alta (integración MiMo) | ||
| Abril 2026: ████████░░░░░░░░░░░░ Media (preparación) | ||
| Marzo 2026: ██████░░░░░░░░░░░░░░ Baja (planificación) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add language tags to fenced blocks to satisfy markdownlint (MD040).
These fences are missing language identifiers; please mark them as text/plain (or a more specific language) to clear lint warnings.
Suggested patch
-```
+```text
Mayo 2026: ██████████████████████ Alta (integración MiMo)
Abril 2026: ████████░░░░░░░░░░░░ Media (preparación)
Marzo 2026: ██████░░░░░░░░░░░░░░ Baja (planificación)- +text
Features: ████████████████ 45%
Bug fixes: ████████████░░░░ 30%
Docs: ██████░░░░░░░░░░ 15%
Tests: ████░░░░░░░░░░░░ 10%
-```
+```text
proyectoauraorg ←→ capitanfeeder: Colaboración directa (PR `#80` → `#81`)
proyectoauraorg → upstream: Contribución ascendente
capitanfeeder → merge: Facilitación de integración
</details>
Also applies to: 109-114, 117-121
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 102-102: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @docs/collaborators/perfiles-colaboradores.md around lines 102 - 106, The
fenced code blocks in perfiles-colaboradores.md are missing language identifiers
and trigger markdownlint MD040; update each triple-backtick block (the three
blocks showing timelines, contribution percentages, and collaboration arrows) to
include a language tag such as text (e.g., change totext) so all fenced
code blocks are annotated and the lint warning is resolved.
</details>
<!-- fingerprinting:phantom:triton:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
43fed8c to
62f95ec
Compare
edelauna
reviewed
May 24, 2026
Contributor
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Would like to address the possibility of terminal reuse
Contributor
Author
|
@edelauna this one's been addressed since your review — replied to each of your comments inline and pushed |
Contributor
Author
|
@edelauna thanks for the careful re-review — addressed the terminal-reuse concern in |
…o-Code-Org#266) Some processes (interactive tools, programs that trap SIGINT and prompt for confirmation) need more than one Ctrl+C to exit. The cancel path sent a single Ctrl+C then disposed immediately, leaving such processes running and the terminal stuck busy. abort() now kicks off a bounded, fire-and-forget retry that re-sends Ctrl+C up to 3 times (500ms apart), stopping early once the process exits or we stop listening. The synchronous cancel path is never blocked and the retry window is bounded so dispose() is never delayed indefinitely. The ExecaTerminal backend is unaffected (it sends SIGKILL directly). Follow-up to Zoo-Code-Org#245 / Zoo-Code-Org#261. Closes Zoo-Code-Org#266
…ew (Zoo-Code-Org#266) - rename ABORT_MAX_ATTEMPTS -> CTRL_C_SEND_LIMIT (total sends) and start the retry loop at sent=1 so the bound reads naturally - document why both isListening and terminal.busy are checked - cross-reference the mirrored test constants to the production ones - note the double-abort send-count assumption in the test - drop the unused changeset
proyectoauraorg
force-pushed
the
fix/266-multiple-ctrl-c-terminate
branch
from
96700db to
67a4181
Compare
Contributor
Author
Good point — the fix already guards against this. In the updated code, the terminal reuse check verifies if (
terminal.terminal.exitStatus == null &&
terminal.process == null
) {
// Safe to reuse — no active process
} else {
// Create new terminal
}The |
edelauna
reviewed
May 26, 2026
edelauna
approved these changes
May 26, 2026
Contributor
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Nice - thanks for addressing.
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
2 participants
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.
Summary
Follow-up to #245 / #261. When a task is cancelled,
releaseTerminalsForTask→process.abort()sent a single Ctrl+C (\x03) for VSCode-backed terminals, thendispose()tore everything down immediately. Processes that trap SIGINT or need multiple Ctrl+C (interactive tools, confirm-on-exit prompts) stayed running and the terminal stayed stuck "busy".Change
abort()still sends the immediate Ctrl+C, then kicks off a bounded, fire-and-forget retry (retryAbort) that re-sends Ctrl+C up to 3 times (500ms apart), stopping early once the process exits (terminal.busyflips false) or we stop listening. Anabortingguard prevents overlapping retry loops, andterminalRef.deref()keeps it safe after dispose. The synchronous cancel path is never blocked and the retry window is bounded, sodispose()is never delayed indefinitely.The ExecaTerminal backend is untouched — it sends SIGKILL directly and was never affected.
Tests
Added an
abortsuite toTerminalProcess.spec.ts(Vitest, fake timers): single Ctrl+C when the process exits immediately; bounded re-send up to max while busy; early stop when the process exits mid-retry; no-op when not listening; no overlapping loops on repeatedabort().Closes #266
Summary by CodeRabbit
Bug Fixes
Tests