Skip to content

fix(core): recover truncated session briefs with bounded retries - #424

Merged
Sun-sunshine06 merged 2 commits into
mainfrom
fix/generation-feedback-reliability
Sep 17, 2026
Merged

Sun-sunshine06 merged 2 commits into
mainfrom
fix/generation-feedback-reliability

Conversation

@Sun-sunshine06

Copy link
Copy Markdown
Collaborator

Summary

Recover background design-session briefs that stop at the output-token ceiling, without accepting incomplete JSON or replacing the previous brief on failure.

  • Add a typed CompletionLengthError, preserving the existing PROVIDER_ERROR code. Only usage metadata is retained; partial response content is not exposed as a successful completion.
  • Make the brief prompt explicitly concise. On a typed length stop only, retry once with an output ceiling of 4,000 tokens instead of 2,000. Existing transport retry behavior is unchanged.
  • Include reported usage from both attempts after successful recovery. Authentication, cancellation and other errors do not enter the length-recovery path.
  • Move cold OAuth module loading into bounded suite setup, rather than charging provider-module transformation against the first status assertion's 5-second timeout. Actual handler assertions keep their original timeout.
  • Add regression tests and a core/providers changeset. No new dependencies.

Upstream reconciliation

During validation, main advanced from b94d715 to c8a179f. This branch was rebased before publication. Upstream already fixed repeated done diagnostic compaction and retiring obsolete preview errors, so those overlapping changes were removed rather than overwritten or duplicated. This PR now touches only the remaining brief-recovery and test-setup scope.

Refs #423.

Validation

On the final branch rebased onto c8a179f:

  • pnpm lint — passed (588 files)
  • pnpm typecheck — passed (10 tasks)
  • pnpm exec turbo run test --continue=always — 2,899 passed, 2 skipped, 213 test files, no cached test results
  • pnpm --filter @open-codesign/desktop build — passed
  • Live smoke through updateDesignSessionBrief and the normal provider layer with gemini-3.5-flash — valid normalized JSON in about 11 seconds, 1,029 reported input tokens and 451 output tokens. This live call completed on its first attempt; forced truncation/recovery is verified by deterministic tests.
  • Windows, Node 24.19.0, pnpm 10.33.4, Electron 39.8.10.

Deterministic coverage includes successful length recovery, exhausting the one-retry limit, malformed JSON after recovery, preserving the prior brief, aggregating usage, and not retrying other failures as truncation.

Checklist

  • Checked the linked issue and repository contribution requirements
  • Added/updated tests
  • Added a changeset
  • Documented the bounded recovery behavior
  • No dependency additions
  • No credentials, private endpoint addresses, logs or generated test artifacts included

No visual UI changes; screenshots are not applicable. This does not implement the placeholder Resources page, disable generation timeouts, or broadly accept invalid model tool arguments.

Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com>
Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com>
@github-actions github-actions Bot added docs Documentation area:desktop apps/desktop (Electron shell, renderer) area:core packages/core (generation orchestration) area:providers packages/providers (pi-ai adapter, model calls) labels Sep 17, 2026

@github-actions github-actions 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.

Findings

  • [Minor] The length-recovery path is only verified with a mocked completeWithRetry, so the one integration that the feature actually depends on is untested — packages/core/src/design-context.ts catches err instanceof CompletionLengthError from completeWithRetry, while packages/core/src/design-context.test.ts replaces completeWithRetry with completeWithRetryMock and rejects with a hand-built CompletionLengthError. If the real wrapper remaps/wraps provider errors (or its retry predicate re-classifies the new PROVIDER_ERROR-coded length stop as transport-retriable), the recovery either never fires in production or fires after redundant transport retries — and the deterministic tests would still pass. The live smoke in the PR body completed on its first attempt, so it does not exercise this either.
    Suggested fix / next action: add one providers-level or integration test that throws a real stopReason: 'length' result through complete → completeWithRetry and asserts the caller receives CompletionLengthError unchanged (same usage), confirming the wrapper rethrows it as-is and does not re-retry it. If the wrapper does wrap, the instanceof check in design-context.ts needs to key off something that survives the wrapper (for example the error code/name).

Questions

  • Does completeWithRetry rethrow this error instance unchanged rather than reconstructing it as a generic CodesignError? And is PROVIDER_ERROR part of its retriable set, given CompletionLengthError keeps that code? Confirming both is what makes the recovery bounded to a single explicit retry rather than an extra layer of transport retries.

Summary

  • Review mode: initial
  • The change is well-scoped and directionally sound: CompletionLengthError preserves PROVIDER_ERROR and carries only usage, the recovery is a single bounded retry (BRIEF_MAX_OUTPUT_TOKENS → BRIEF_RETRY_OUTPUT_TOKENS), usage from both attempts is summed on success, and the previous brief is retained on any failure (packages/providers/src/index.ts:488, packages/core/src/design-context.ts:518). No direct provider-SDK imports, no new dependencies, and no silent fallback were introduced; the changeset covers both touched packages.
  • Residual risk: on total failure the thrown error carries only the second attempt's usage, so tokens/cost from the first (truncated) attempt are not surfaced to the caller. Low impact since the operation failed and the prior brief is kept, but worth noting if callers aggregate usage for display.
  • The codex-oauth-ipc.test.ts beforeAll warm-up is a reasonable test-infra fix and unrelated to the brief logic.
  • Not found in the public repo: the completeWithRetry body and issue #423 acceptance criteria (PR uses Refs #423, so no closure claim to validate).

Testing

  • Not run (automation). Suggested addition: an integration test at the completeWithRetry boundary asserting CompletionLengthError propagates unchanged, plus a check that the wrapper does not transport-retry a length stop.

Open-CoDesign Bot

@Sun-sunshine06
Sun-sunshine06 merged commit e70430d into main Sep 17, 2026
6 checks passed
@Sun-sunshine06
Sun-sunshine06 deleted the fix/generation-feedback-reliability branch September 17, 2026 17:08
hqhq1025 added a commit that referenced this pull request Sep 17, 2026
Fix independently reproduced lifecycle, artifact, chat, comment, and tweak defects. Required CI, production main-chain validation, and three independent adversarial re-reviews passed. Preserve upstream #424.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:core packages/core (generation orchestration) area:desktop apps/desktop (Electron shell, renderer) area:providers packages/providers (pi-ai adapter, model calls) docs Documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant