Skip to content

feat(openrouter): tag client log lines with the review phase - #120

Merged
aliasunder merged 5 commits into
mainfrom
feat/client-log-phase-context
Sep 30, 2026
Merged

aliasunder merged 5 commits into
mainfrom
feat/client-log-phase-context

Conversation

@aliasunder

@aliasunder aliasunder commented Sep 30, 2026 •

Copy link
Copy Markdown
Owner

Summary

With phases: parallel, three review phases call the same model at the same time. Until now, the OpenRouter client's log lines carried the model but no phase. A failed attempt, a fallback advance, or a request that settled after its deadline could not be tied to the phase that sent it. generationId only appears on accepted responses, so it doesn't help with failures.

Every line the client logs for a phase now carries phase: <id>. This includes the late-settlement line, which is logged after requestReview has already returned.

Changes

  • src/openrouter/client.ts
    • requestReview(params, logger) takes the caller's logger, and createOpenRouterClient no longer takes one.
    • attemptOnce and lookupGenerationCost pass withDeadline a child of that logger. The child binds the operation (operation plus model or generationId), so withDeadline no longer takes a separate logContext record.
    • The cost-lookup failure warnings log through the same child, so they now carry generationId as well.
    • All nine log call sites write through the request's logger, or through a child of it that adds the operation's fields:
      • review attempt failed
      • advancing to fallback model without same-model retry
      • retrying with an output ceiling…
      • review response accepted
      • request deadline elapsed
      • deadline-elapsed request settled
      • generation cost lookup failed (two sites)
      • unexpected generation response shape
  • src/orchestrate.ts
    • createPromptedGenerateFindings builds logger.child({ phase }) for each call and passes it to requestReview.
    • Its own requesting review line logs through the same phase logger. The hand-set module: "generateFindings" tag on that line is gone, because the auto-captured source field already names the file.
  • src/main.ts: builds the client without a logger.

Tests

  • Client tests: each requestReview call passes the same captured logger its assertions read.
  • New client test: a caller-bound prop reaches deadline-elapsed request settled after the request has already rejected. The test also asserts the exact rejection message.
  • New createPromptedGenerateFindings tests:
    • Two phases produce two client log entries, each with its own phase.
    • The requesting review line is asserted whole: phase, model, and fallbackModel.
  • Cost-lookup tests: the warning assertions now include generationId.
  • New parallel integration test with the real client:
    • The first-dispatched phase (correctness-security) fails once with a retryable 500.
    • Its review attempt failed line names that phase.
    • All three review response accepted lines name their own phase.
  • Mutation checks, each run on a commit and reverted afterwards. The first three ran on ab4f290, and the last three ran after the logger-context refactor:
Mutation Failing tests
Pass the unscoped logger instead of the phase child Both new orchestrate tests
Store the per-call logger on the client, so the last caller wins The parallel integration test: the failure line reads subtle-bugs
Log the late settlement through the module-level logger The new client test and 3 existing late-settlement tests
Log requesting review through the unscoped logger The requesting review test (missing phase)
Pass withDeadline the plain logger instead of the chat-request child 5 client deadline and late-settlement tests
Pass withDeadline the plain logger instead of the cost-lookup child 2 cost-lookup deadline tests

Verification

  • npm test: 804 passed
  • npm run lint and npm run build are clean, and docker build . succeeds.
  • Live: this PR's self-review runs combined, and its log shows review response accepted with "phase":"combined" and "source":"client.js:…".

🤖 Generated with Claude Code

requestReview now takes the caller's logger, and the prompted strategy
passes a child bound to the phase id. Every attempt, fallback-advance,
ceiling-retry, acceptance, cost-lookup, and late-settlement line the
client logs carries `phase`, so concurrent parallel phases on the same
model can be told apart. The client factory no longer takes a logger.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@umm-actually

umm-actually Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

umm-actually re-reviewed at fec6d29

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


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

aliasunder and others added 4 commits September 30, 2026 12:39
- withDeadline takes its operation context from a child logger instead of
  a parallel logContext record; the log output is unchanged.
- requestReview takes its type from OpenRouterClient instead of repeating
  the params shape.
- createPromptedGenerateFindings builds the phase logger once and derives
  its own module-tagged line from it; the comment now gives the real reason
  client lines skip the module tag.
- Doc comments explain the late-settlement line, the logger-free factory,
  attemptOnce's logging, and drop an undefined roadmap reference.

Ship-Check: code-quality · claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… rejection

The "requesting review" line had no assertion, so a change to how its
phase and module tags are bound could pass the suite. It is now pinned
whole. The late-settlement test asserts the ladder-exhaustion message
instead of only the error class. Two multiline client test callbacks
get block bodies, and the stub queue guard is a truthy check.

Ship-Check: test-audit · claude-opus-5-5

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…o doc claims

The cost-lookup failure warnings logged through the request logger, so
they carried no generationId while the lookup's deadline lines did. All
lookup lines now go through one child logger that binds it.

withDeadline's doc said it logs only timing, but it also logs how the
abandoned call settled. createPromptedGenerateFindings's doc said one
request, but the client's retry and fallback ladder can send several.

Ship-Check: bug-check · claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…equest log

The requesting-review line already carries the phase and an auto-captured source file, so the manual module tag added nothing and needed a comment explaining why client lines lack it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread src/orchestrate.ts
@aliasunder
aliasunder added this pull request to stack #122 September 30, 2026 18:12
@aliasunder
aliasunder merged commit dd2a7d7 into main Sep 30, 2026
9 checks passed
@aliasunder
aliasunder deleted the feat/client-log-phase-context branch September 30, 2026 18:36
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