Skip to content

Honor summary_timeout_seconds in explain --generate; raise default to 5m#1204

Merged
peyton-alt merged 2 commits into
entireio:mainfrom
numman-ali:fix/summary-timeout-setting
May 13, 2026
Merged

Honor summary_timeout_seconds in explain --generate; raise default to 5m#1204
peyton-alt merged 2 commits into
entireio:mainfrom
numman-ali:fix/summary-timeout-setting

Conversation

@numman-ali
Copy link
Copy Markdown
Contributor

@numman-ali numman-ali commented May 13, 2026

Fixes #1198.

Summary

summary_timeout_seconds was accepted by settings and a SummaryTimeoutValue() helper existed, but entire checkpoint explain --generate ignored both and used a hardcoded 30s deadline — exactly what the field's own doc comment said was a TODO ("Not yet consumed by the generate path; present so settings round-trip for a follow-up change that wires it into the deadline selection.").

This wires it up, adds a per-run override and a configure flag, and raises the package default.

Changes

  • Wire the setting. New resolveSummaryTimeout(ctx, flagSeconds) in cmd/entire/cli/explain.go picks the effective deadline with precedence: per-run flag > summary_timeout_seconds setting > package default. Zero or negative at any layer means "unset; consult the next layer down" — matching the existing SummaryTimeoutValue() semantics.

  • Per-run flag. entire checkpoint explain --generate <id> --summary-timeout-seconds N overrides the setting for a single invocation, no JSON editing required. Validated to be non-negative and rejected when used without --generate.

  • Configure flag. entire configure --summarize-timeout-seconds N persists the value, mirroring the existing --summarize-provider / --summarize-model family in setup.go. Passing 0 clears; negatives rejected.

  • Default bumped 30s → 5 min. SOTA models on large transcripts routinely exceed 30s; the old default made --generate look broken on otherwise normal runs. Users wanting faster failure can set summary_timeout_seconds: 30 or pass --summary-timeout-seconds 30. Locked in via TestDefaultCheckpointSummaryTimeout_IsFiveMinutes so a casual edit can't silently regress.

  • Doc comment on SummaryTimeoutSeconds updated to reflect the wired-up behavior.

Signature change

generateCheckpointAISummary and the runExplain* plumbing chain take the new value as a positional arg, matching the existing positional-arg style in this file (provider/model already flow as positional fields from a resolver-returned struct). Not refactoring to an options struct here — that's a broader cleanup across all the existing positional bools and would be scope creep on a bug-fix PR.

The package-level checkpointSummaryTimeout var stays so the existing test-injection pattern (explain_test.go lines 754, 845, 903, 938) keeps working and continues to serve as the single source of truth for the fallback default.

Forward-compat with #964

The stream-driven explain --generate work in #964 proposes a different deadline model (idle watchdog + hard cap, TTY-gated). This change touches only the deadline resolution, not the application model — if #964 lands, the same setting/flag surface can be re-wired to feed the new model. The user-facing API stays valid.

Tests

New:

  • TestResolveSummaryTimeout_FlagOverridesSetting
  • TestResolveSummaryTimeout_SettingHonoredWhenFlagUnset
  • TestResolveSummaryTimeout_DefaultWhenBothUnset
  • TestResolveSummaryTimeout_NegativeSettingTreatedAsUnset
  • TestDefaultCheckpointSummaryTimeout_IsFiveMinutes (regression lock-in)
  • TestConfigureCmd_SummarizeTimeoutSeconds_WritesProjectSettings
  • TestConfigureCmd_SummarizeTimeoutSeconds_WritesLocalSettings
  • TestConfigureCmd_SummarizeTimeoutSeconds_ClearsValue
  • TestConfigureCmd_SummarizeTimeoutSeconds_RejectsNegative

Existing four TestGenerateCheckpointAISummary_* tests updated to pass timeout as a positional arg — the package-var injection pattern is preserved for the fallback-default path.

Test plan

  • mise run check (fmt + lint + unit + integration + Vogon E2E canary) passes locally
  • Repro from the issue: with a large checkpoint and summary_timeout_seconds: 3600, entire checkpoint explain --checkpoint <id> --generate now honors the 3600s instead of timing out at 30s
  • Per-run override: entire checkpoint explain --generate <id> --summary-timeout-seconds 120 uses 120s regardless of setting
  • Configure persistence: entire configure --summarize-timeout-seconds 600 writes summary_timeout_seconds: 600 to settings
  • Default behavior: with no setting and no flag, --generate now uses 5 min instead of 30s

Copilot AI review requested due to automatic review settings May 13, 2026 21:04
@numman-ali numman-ali requested a review from a team as a code owner May 13, 2026 21:04
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

The summary_timeout_seconds setting and SummaryTimeoutValue() helper
existed in settings but were never consumed by `explain --generate`,
which used a hardcoded 30s deadline. Fixes entireio#1198.

Changes:

- Wire SummaryTimeoutValue() into the generate path via a new
  resolveSummaryTimeout helper with precedence:
  per-run flag > settings > package default.
- Add --summary-timeout-seconds flag on `entire checkpoint explain`
  for one-off overrides without editing JSON.
- Add `entire configure --summarize-timeout-seconds` matching the
  existing --summarize-provider / --summarize-model family for
  persistent configuration.
- Raise the package default from 30s to 5 minutes. SOTA models on
  large transcripts routinely exceed 30s; users wanting faster
  failure can set summary_timeout_seconds: 30 or pass
  --summary-timeout-seconds 30.
@numman-ali numman-ali force-pushed the fix/summary-timeout-setting branch from bbba8aa to dce46d3 Compare May 13, 2026 21:05
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR wires summary_timeout_seconds into entire checkpoint explain --generate, adds CLI/configure overrides, and raises the default summary-generation timeout to 5 minutes.

Changes:

  • Adds timeout resolution precedence: per-run flag > settings value > package default.
  • Adds --summary-timeout-seconds for explain --generate and --summarize-timeout-seconds for configure.
  • Updates settings documentation and adds/updates tests for timeout resolution and persistence.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
cmd/entire/cli/explain.go Applies resolved summary timeout through the explain/generate call chain.
cmd/entire/cli/explain_test.go Updates call sites and adds timeout resolution/default tests.
cmd/entire/cli/setup.go Adds configure support for persisting summary timeout settings.
cmd/entire/cli/setup_test.go Adds tests for configure timeout persistence and validation.
cmd/entire/cli/settings/settings.go Updates documentation for the now-consumed timeout setting.

Comment thread cmd/entire/cli/explain.go Outdated
The flag validation block sat after the export-mode dispatch, so
invocations like `explain --json --summary-timeout-seconds 10` or
`explain --transcript --summary-timeout-seconds -5` returned via
runExplainExport before the check ran, silently ignoring the flag
(and accepting negative values).

Move the validation alongside --session-index and --limit, which
were already correctly placed above the export-mode return. Add
regression coverage spanning the prose path, --json, --transcript,
and --raw-transcript --session-index export modes.
Copy link
Copy Markdown
Contributor

@peyton-alt peyton-alt left a comment

Choose a reason for hiding this comment

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

Approved, looks great! Thanks for the contribution!

@peyton-alt peyton-alt enabled auto-merge (squash) May 13, 2026 22:56
@peyton-alt peyton-alt merged commit cc178b5 into entireio:main May 13, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

summary_timeout_seconds is ignored by checkpoint explain --generate

3 participants