Skip to content

refactor: clear readability pauses in the client, orchestrator, and entrypoint - #121

Merged
aliasunder merged 9 commits into
mainfrom
refactor/pr120-readability-cleanup
Sep 30, 2026
Merged

aliasunder merged 9 commits into
mainfrom
refactor/pr120-readability-cleanup

Conversation

@aliasunder

@aliasunder aliasunder commented Sep 30, 2026 •

Copy link
Copy Markdown
Owner

Summary

Two stranger reads of src/openrouter/client.ts, src/orchestrate.ts, and src/main.ts found places where a reader new to the code had to stop and trace. This PR fixes the ones in code these files own: comments that didn't match the code, names that meant two things, duplicated logic, silent branches, and a production branch that existed only for test stubs.

One review output changes: a finding re-posted after GitHub rejects the inline review no longer says it sits beyond the diff. Log output changes are listed below.

Changes

src/openrouter/client.ts

  • ModelAttempt.model doc. It defines the ladder (the primary model, then the fallback), and says the routed model is reported only for the accepted attempt, as StructuredReviewResult.modelUsed.
  • One name for the key-rejected fact. "Abort" also named the AbortSignal that cancels an HTTP call, so the fact an auth or credit status (401, 402, 403) ended the ladder is now keyRejected everywhere:
    • the failed attempt's flag (was abort)
    • ReviewRequestError's field (was aborted)
    • the status set, KEY_REJECTED_STATUSES
    • the stage runner's check, isKeyRejected (was isAbortedRequest)
  • getCallTimeout. It computes a call's timeout and its timeout summary once. The chat request and the cost lookup used to compute both separately.
  • Cost lookup. lookupGenerationCost logs at debug when the review deadline has already passed, where it used to return silently.
  • OpenRouterLike.generations is required. The real SDK always has it, so the optional member and its no-client skip existed only for test stubs.
  • Counter names. The per-model counter is now modelAttemptNumber, and the total across the ladder is now totalAttemptCount. Before, attemptNumber and attemptCount sat side by side and were easy to confuse.
  • Comments now state:
    • that Promise.try forwards the abort signal to start
    • why a context overflow continues to the fitted-ceiling retry
    • that requestReview checks the review budget before each attempt
    • which variables are reassigned across attempts

src/orchestrate.ts

  • Comments. Step-numbered comments ("Step 4", "Step 12.5") pointed to a list that doesn't exist. They are now plain sentences, and the ones that only restated the next line are gone.
  • Diff budget comment. It said the diff "gets half the budget", but half is only the diff's cap: context files get whatever the diff leaves. budgetHalf is now diffTokenLimit.
  • changedPaths. A deleted file returns nothing, instead of a null that a later filter removed. A rename returns its new and old paths.
  • Findings posted as issue comments. These are beyond-diff findings plus every in-diff finding when GitHub rejects the inline review. The group was called "standalone", "unanchored", and "beyond-diff" in different places, and its log lines called every member "beyond-diff". It is now issueCommentPosts, which pairs each finding with its rendered body, and the log text matches.
  • Smaller changes:
    • The related-files token total uses sumBy.
    • describePipelineFailure takes includeCostSummary.
    • renderCostSummary sits beside the other return values.
    • Comments state that the anchored/unanchored split compares object identity, and that the GitHub client already logs a 422.

src/config.ts and src/main.ts

  • An empty fallback_model parses to null in config.ts. It used to be normalized in two different ways, in main.ts and in the settings log.
  • main.ts uses describeError for unhandled rejections.
  • Its comments now agree that at most one cancellation cleanup is registered, and name the priority_docs exception to "empty selects the default".
  • The cancellation warning no longer claims to close a check run that doesn't exist.

src/github/client.ts: its doc for the issue-comment group now names both members.

src/review/comment-mapping.ts

  • renderBeyondDiffFinding and renderReroutedFinding replace renderStandaloneFinding. They share one body and differ only in the location line.
  • A re-routed in-diff finding used to say it sat beyond the diff's line ranges. It now says it is at or near a changed line, posted as its own comment because GitHub rejected the inline review.

Log output changes

Line Before After
review attempt failed attemptNumber modelAttemptNumber
review response accepted, review phase completed attemptCount totalAttemptCount
Issue-comment post summary beyond-diff findings posted findings posted as issue comments, with locations
Inline review post findings review posted with inlineCount, reviewUrl adds locations
Issue-comment post failure failed to post beyond-diff finding — … failed to post finding as an issue comment — …
Cancellation cancellation signal received — closing the check run cancellation signal received — closing any open check run before exit, with checkRunOpen
Cost lookup after the deadline (nothing) generation cost lookup skipped at debug, with reason

Tests

  • The existing log assertions follow the renamed fields and messages.
  • New or strengthened tests:
    • changedPaths is asserted as the exact ordered path list, which catches a dropped rename path or a dropped /dev/null guard.
    • The issue-comment summary logs the count of posts that landed, excluding a failed one, and is absent when none land.
    • The stage runner skips later stages only when keyRejected is true.
    • The key-rejection skip warning names the skipped phases.
    • The deadline cost-lookup skip asserts its debug line.
    • A configured fallback_model keeps its value, and an empty one parses to null.
    • The failure summary leaves out the cost table when cost_summary is off.
    • Both findings-post summaries assert their locations.
    • A re-routed finding renders with the in-diff location line, and a 422 run with mixed findings keeps the beyond-diff line only on the beyond-diff finding.
  • One test, which covered only the removed no-generations-client branch, is deleted.
  • Mutation checks were run on each new behavior and reverted, and each broke the intended test.

Verification

  • npm test: 810 passed
  • npm run lint and npm run build are clean.

🤖 Generated with Claude Code

@aliasunder
aliasunder added this pull request to stack #122 September 30, 2026 18:12
Comment thread src/openrouter/__tests__/client.test.ts
Comment thread src/openrouter/client.ts
@umm-actually

umm-actually Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

umm-actually re-reviewed at 47f5106

1 new finding(s) posted (6 tracked finding(s) across all runs).


umm-actually · deepseek/deepseek-v4.1-flash

Base automatically changed from feat/client-log-phase-context to main September 30, 2026 18:36
aliasunder and others added 5 commits September 30, 2026 14:36
…ntrypoint

- client.ts: define the ladder on ModelAttempt.model, rename the failed
  attempt's `abort` flag to `keyRejected` (and its status set to match),
  compute the call deadline and its timeout summary once, debug-log the two
  skipped cost-lookup paths, rename the per-model attempt counter to
  `modelAttemptNumber`, and state why a context overflow continues.
- orchestrate.ts: replace step-numbered comments with sentences, make the
  rename check an early return, use sumBy, name the cost-summary flag for
  what it gates, and state the identity check and the 422 logging owner.
- config.ts: an empty fallback_model parses to null once; main.ts and the
  settings log read it as is.
- main.ts: use describeError and name what keeps the cancellation cleanup
  from throwing.

Log output changes: "review attempt failed" reports `modelAttemptNumber`
instead of `attemptNumber`, and a skipped cost lookup logs at debug.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s match the code

- ReviewRequestError.aborted becomes keyRejected, matching SingleAttempt; run-stages reads it through isKeyRejected
- getCallDeadline becomes getCallTimeout, and its doc states the tie
- The success log's attemptCount becomes totalAttemptCount beside the failure log's modelAttemptNumber, in the client and the stage runner
- Issue-comment posting names its group for both members (beyond-diff and rerouted findings), and its log lines say issue comment
- The diff-budget, dedup, deleted-path, cancellation, and empty-input comments state what the code does
- The cancellation warning reports whether a check run was open

Ship-Check: code-quality · claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The fetchBotIssueComments and renderStandaloneFinding docs called every issue-comment finding beyond-diff, but in-diff findings whose inline anchors GitHub rejected post the same way.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The real @openrouter/sdk always exposes generations, so the optional member and the no-client skip existed only for test stubs. Stubs now provide it, the skip branch and its debug line are gone, and the test that covered only that branch is removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…jected skip

- readChangedFiles gets its whole changedPaths list, so a dropped rename path, an added /dev/null, or a deleted file's path fails a dedicated test.
- The "findings posted as issue comments" info line is asserted with a count that leaves out a failed post, and it stays silent when no post lands.
- runStages skips later stages only when keyRejected is true. A failure carrying keyRejected: false runs them, and the skip warning names the skipped phases.

Ship-Check: test-audit · claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@aliasunder
aliasunder force-pushed the refactor/pr120-readability-cleanup branch from 470b2d7 to 9a7935d Compare September 30, 2026 18:36
GitHub rejects the inline review whole on a 422, so every in-diff finding in
it posts as an issue comment, not only the one with the bad anchor. The
renderStandaloneFinding doc, the rerouted field, and the issue-comment group
comment now say so. The diff-exclusion comment names every excluded file, not
only generated ones, and ModelAttempt.model points at where the routed model
is actually reported.

Ship-Check: bug-check · claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread src/orchestrate.ts
Every failure-path test ran with cost_summary on, so dropping describePipelineFailure's includeCostSummary guard left the suite green.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread src/orchestrate.ts Outdated
The inline-review and issue-comment summary lines logged only counts, so an operator could not tell which findings went where without diffing the PR against the model output. Both now carry file:line locations.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread src/review/comment-mapping.ts Outdated
A finding re-posted as an issue comment after GitHub rejects the inline
review rendered the beyond-diff location note, which misstated a finding
on a changed line. Beyond-diff and re-routed findings now render through
separate functions that share one body, and the orchestrator pairs each
finding with its renderer where it builds the post list.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread src/orchestrate.ts
@aliasunder
aliasunder merged commit d4609f2 into main Sep 30, 2026
9 checks passed
@aliasunder
aliasunder deleted the refactor/pr120-readability-cleanup branch September 30, 2026 21:14
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.

1 participant