Skip to content

feat(cli): post partial results when the process is signalled - #367

Merged
MattJColes merged 2 commits into
mainfrom
claude/lgtmaybe-perf-signal-3j8akh
Aug 3, 2026
Merged

feat(cli): post partial results when the process is signalled#367
MattJColes merged 2 commits into
mainfrom
claude/lgtmaybe-perf-signal-3j8akh

Conversation

@MattJColes

Copy link
Copy Markdown
Owner

What

On SIGINT/SIGTERM, lgtmaybe stops dispatching new model calls and posts whatever the review has already produced, with a notice naming the interruption — instead of dying with nothing on the PR.

Why

Two routine ways a review currently posts nothing at all — no findings, no failure comment:

  1. The job exceeds its timeout-minutes. max_review_seconds defaults to 3600s, so a workflow with timeout-minutes: 25 gets killed long before the soft deadline can fire and post partial findings. The workaround has been hand-tuning max_review_seconds under the job timeout — fragile config discipline every user has to rediscover (our own .github/workflows/lgtmaybe.yml carries a comment explaining the layering).
  2. concurrency: cancel-in-progress: true cancels an in-flight review when a new push lands.

In both cases the runner signals the process before killing it. We ignored the signal.

How

Reuses the deadline mechanism rather than building a parallel one. The engine already degrades an over-running review to partial-results-with-a-notice when max_review_seconds passes. A signal now sets the same state:

  • engine.request_interrupt() sets a process-global flag that _skip_reason checks alongside the deadline and the token budget, so queued lens calls are skipped at execution time exactly as they are past the deadline. In-flight calls finish and their findings post.
  • Everything downstream is shared and unchanged: the "N of M review calls failed" notice, the hidden incomplete marker (so the notice also posts as its own PR comment), the skipped-reflection notice, and the ReviewIncompleteError path when every call was skipped — an interruption can never become a silent 👍 LGTM.
  • Only the reason wording differs (review interrupted (termination signal) vs the deadline's), so nobody goes hunting for a max_review_seconds they never hit.

Handler installation (cli.graceful_interrupt), against the constraints:

  • Installed by the CLI entrypoint only, held open for the subcommand via the click context — importing lgtmaybe as a library never touches a host application's handlers (guarded by a fresh-interpreter test).
  • signal.signal() only works on the main thread; off it (and on a platform missing a signal) the context manager degrades to a no-op rather than raising — the engine runs a ThreadPoolExecutor, so this matters.
  • The previous handler is restored on the way out and as the handler fires, so a second signal still kills the process if the wind-down itself hangs. lgtmaybe never becomes unkillable, and it never permanently steals the signal.
  • Windows-safe: signals are looked up with getattr and signal.signal failures are tolerated, and the tests parametrise over whichever signals the platform actually has.

Tests

TDD, red first (both files failed to import before the implementation existed).

tests/engine/test_interrupt.py

  • test_flag_starts_clear_and_round_trips
  • test_queued_calls_are_skipped_and_partial_findings_still_post
  • test_notice_names_the_interruption_not_the_deadline
  • test_reflection_is_skipped_with_an_honest_notice
  • test_every_call_skipped_still_fails_loud

tests/cli/test_signal.py

  • test_installs_and_restores_the_previous_handler (per available signal)
  • test_first_signal_requests_the_wind_down (per available signal — also asserts the previous handler is already back)
  • test_off_the_main_thread_is_a_no_op
  • test_main_wraps_the_command_in_the_wind_down
  • test_importing_the_library_installs_no_handler

Also

  • Spec: new cli.graceful-interrupt requirement + anchor in openspec/specs/cli-and-local/ (the review-pipeline section it would otherwise extend is already at the 40-line cap). uv run pytest tests/specs -q and openspec validate --specs green.
  • Docs: action.yml's max_review_seconds description, the Action input table, docs/explanation/architecture.md, ARCHITECTURE.md, CLAUDE.md, regenerated docs/llms*.txt.
  • .github/workflows/lgtmaybe.yml: the timeout-layering comment now names the wind-down as the backstop. Timeout layering is still the calm path — the signal handler can only skip queued calls, so a job cut short mid-call is still racing the runner's grace period. No workflow if: conditions touched.

Gate: ruff check / ruff format --check / mypy / pytest -q (1839 passed, 3 skipped) all green.


🤖 Generated with Claude Code


Generated by Claude Code

A GitHub Action job killed for exceeding `timeout-minutes`, or cancelled by
`cancel-in-progress` on a new push, posted nothing at all — no findings, no
failure comment. The runner signals the process before killing it; we ignored
the signal.

The first SIGINT/SIGTERM now sets exactly the state the `max_review_seconds`
deadline sets, so the whole downstream path is shared: queued lens calls are
skipped, in-flight ones finish, the skipped calls raise the existing
partial-results notice plus the hidden incomplete marker, reflection is skipped
with an honest notice, and a run where everything was skipped still fails loud.
Only the reason wording differs, so nobody hunts for a ceiling they never hit.

The handler is installed by the CLI entrypoint only — importing lgtmaybe as a
library never touches a host's handlers — held open for the subcommand via the
click context, restored on the way out and as it fires (so a second signal
still kills the process), and a no-op off the main thread or where the platform
lacks the signal.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lgtmaybe

lgtmaybe Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

⚠️ 3 of 4 review calls failed (ProviderTruncated: response hit the 32768-token max_tokens ceiling (34012 reasoning) before finishing — raise max_tokens, or lower max_input_tokens so each call covers less); results may be incomplete.

⏱️ 1 batch was too big for one call (timed out, or ran past the max_tokens ceiling) and was reviewed in smaller pieces instead. Consider a lower max_input_tokens, a higher max_tokens, or a faster model.

2 findings · provider openrouter · model ~deepseek/deepseek-v4-flash-latest · lgtmaybe 1.12.2

@lgtmaybe lgtmaybe Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 2 of 4 review calls failed (ProviderTruncated: response hit the 32768-token max_tokens ceiling (33849 reasoning) before finishing — raise max_tokens, or lower max_input_tokens so each call covers less); results may be incomplete.

⏱️ 1 batch was too big for one call (timed out, or ran past the max_tokens ceiling) and was reviewed in smaller pieces instead. Consider a lower max_input_tokens, a higher max_tokens, or a faster model.

💬 2 earlier lgtmaybe conversations are still unresolved on this PR — this run's count covers what it reviewed now, not those.

0 findings · provider openrouter · model ~deepseek/deepseek-v4-flash-latest · lgtmaybe 1.12.2

Incremental review of the changes since 8403343 — earlier findings stay open until fixed.

Comment thread tests/cli/test_signal.py
Comment thread src/lgtmaybe/cli/__init__.py
Both from review of #367.

The off-main-thread test joined the worker with a timeout but never checked
`is_alive()`, so a `graceful_interrupt` that hung off the main thread would
have left `errors` empty and passed — a hang reading as success.

The partial-install path — SIGINT installs, then SIGTERM refuses — was
untested: only the first-call-raises case was covered, so a regression that
left the already-installed SIGINT handler behind would have gone unnoticed.
Characterisation test (green from the start; the restore was already there),
verified non-vacuous by removing the `_restore()` in the except arm and
watching it fail.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lgtmaybe

lgtmaybe Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

⚠️ 2 of 4 review calls failed (ProviderTruncated: response hit the 32768-token max_tokens ceiling (33849 reasoning) before finishing — raise max_tokens, or lower max_input_tokens so each call covers less); results may be incomplete.

⏱️ 1 batch was too big for one call (timed out, or ran past the max_tokens ceiling) and was reviewed in smaller pieces instead. Consider a lower max_input_tokens, a higher max_tokens, or a faster model.

💬 2 earlier lgtmaybe conversations are still unresolved on this PR — this run's count covers what it reviewed now, not those.

0 findings · provider openrouter · model ~deepseek/deepseek-v4-flash-latest · lgtmaybe 1.12.2

Incremental review of the changes since 8403343 — earlier findings stay open until fixed.

@MattJColes
MattJColes merged commit 1819896 into main Aug 3, 2026
8 checks passed
MattJColes added a commit that referenced this pull request Aug 3, 2026
… drop a param in silence (#369)

`reasoning_effort` was configured and discarded. litellm's openrouter
transformation only adds the param to its supported list when
`supports_reasoning()` is true — i.e. when the model is in litellm's capability
map — and with `drop_params = True` an unmapped model loses it with no warning.
The models a reasoning budget is set for are exactly the ones the map does not
know yet. Measured on this repo: one lens on #367 spent 34,012 reasoning tokens
against a 32,768 ceiling with `reasoning_effort: low` set, and truncated.

Two changes:

- `dropped_params` reads litellm's own capability map and OpenAI-param
  vocabulary to name, once at startup, every configured param the resolved model
  will not accept. Keyed off the map rather than a per-param special case, so a
  param added later is covered; provider-native options (ollama's `num_ctx`) are
  not judged, since they are not litellm's to drop.
- On openrouter only, a budget litellm will not forward is sent as OpenRouter's
  own top-level `reasoning` object via `extra_body`. Never beside the flat param
  — OpenRouter answers a request carrying both with a 400. `default` has no
  equivalent in OpenRouter's effort enum, so it is reported, not translated.

The openrouter branch is a deliberate, narrowly scoped exception to the
litellm-normalises-everything decision in CLAUDE.md, documented as such in the
code — not licence for general per-provider plumbing.

Also corrects `.lgtmaybe.yml`, whose comments concluded `reasoning_effort` was
"the separate lever" against a knob that was never connected. The measured table
stays; the conclusion drawn from it does not.

Refs #348

Co-authored-by: Claude <noreply@anthropic.com>
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