Skip to content

fix(claude-sdk-oauth): canonicalize eval summary continuity - #1498

Open
Altairpaca wants to merge 1 commit into
code-yeongyu:mainfrom
Altairpaca:fix/1472-eval-summary-continuity
Open

Altairpaca wants to merge 1 commit into
code-yeongyu:mainfrom
Altairpaca:fix/1472-eval-summary-continuity

Conversation

@Altairpaca

@Altairpaca Altairpaca commented Sep 8, 2026

Copy link
Copy Markdown

Summary

Fixes #1472.

eval.prepareArguments normalizes run summaries before schema validation. For summaries longer than the schema limit, the provider-final assistant can therefore differ from the committed assistant only by harness-owned normalization. AssistantCommitBoundary previously fingerprinted the raw tool arguments and classified that normalization as assistant_rewritten, invalidating Claude SDK continuity and making the next turn fork or cold-flatten.

This patch canonicalizes the effective eval run summary at the continuity fingerprint boundary while keeping every other tool and argument fail-closed.

Maintainer review follow-up

Addressed the requested deduplication: continuity no longer carries a second implementation of the eval summary clamp.

  • clampEvalSummary has one implementation owned by @code-yeongyu/senpi-codemode.
  • eval-request.ts continues to export that same helper, and types.ts continues to export the same EVAL_SUMMARY_MAX_LENGTH constant.
  • The narrow runtime leaf is published as @code-yeongyu/senpi-codemode/eval-summary so compiled Node code does not statically import a .ts file from the source-only package under node_modules.
  • AssistantCommitBoundary imports that canonical helper and preserves the eval-only, peek/stop, string-summary, and fail-closed guards.
  • The regression now drives the committed summary through the real eval tool prepareArguments path instead of hand-writing an 80-character expected value.

Changes

  • Canonicalize only eval run summary values for assistantContentHash using the tool-owned normalizer.
  • Leave peek / stop arguments untouched.
  • Keep text, thinking, tool id/name, all non-summary eval arguments, and every other tool argument rewrite-sensitive.
  • Add a focused [Bug]: eval summary clamping triggers assistant_rewritten with claude-sdk-oauth #1472 regression using real eval.prepareArguments, plus negative controls for real summary and code rewrites.
  • Expose the codemode normalizer through a runtime-safe ESM leaf while preserving its existing TypeScript exports and single limit constant.

Independent runtime evidence

The issue has an independent macOS arm64 reproduction on omo-ai 5.0.0-0.beta.51 / bundled Senpi 2026.9.9-2 / claude-sdk-oauth/claude-fable-5-1: the committed eval summary was exactly 80 characters ending in ..., the binding invalidation reason was assistant_rewritten, and the following request flattened 72 delta messages into a 943,217-byte payload. See #1472 for the measured details.

Verification status

Focused regression:

packages/coding-agent/test/suite/regressions/1472-claude-sdk-oauth-eval-summary-clamp.test.ts

Final head fc00aceef3be605d6a349afa62eff499d74a3a4a is a single commit on current upstream main c68ec4d3c0a2dc5fd105da69b2ab4d4e898e7e4d.

Pre-submit audit checked:

  • final diff scope: 7 expected files, no unrelated main drift;
  • one clamp implementation / one summary-length constant;
  • real prepareArguments is used by the continuity regression;
  • source-only codemode packaging includes the .mjs runtime leaf and .d.mts declaration;
  • Node ESM + TypeScript conditional-export resolution and npm pack --dry-run were exercised in an isolated package fixture.

This environment cannot execute the repository's installed dependency graph, so I am not claiming a local full-suite green run. The PR-triggered CI, Changelog gate, and binary-parity workflows were created for this head but are currently action_required pending repository approval; any executable type/test failure remains blocking evidence.

Risk

Low and bounded to the continuity fingerprint seam. Two provider/committed tool calls compare equal only when their eval run summaries normalize to the same effective value that the harness executes. Semantic summary changes within the limit and changes to other eval arguments remain assistant_rewritten.

No release changelog entry is included in this contributor patch; repository-side changelog policy/label handling remains with the maintainer workflow.

Copy link
Copy Markdown
Author

Rebased/synchronized onto current main (464f1a54e8569cdfe8e4620583e19d594a6b96e9); the PR still reduces to one production file plus the focused regression and remains mergeable. Issue #1472 now also has an independent macOS arm64 reproduction matching this exact 80-character clamp -> assistant_rewritten boundary, including a subsequent 943,217-byte flatten request. I updated the PR body with that evidence and kept the negative-control scope explicit. Current CI/Changelog runs are action_required before jobs execute, so there is no failing test result to address yet. When convenient, a workflow approval/review pass would unblock the next evidence step; I’ll own any executable CI or review follow-up.

@code-yeongyu

Copy link
Copy Markdown
Owner

Reviewed against a field report of the same symptom (a Claude session re-sending the whole conversation every turn, hundreds of KB per turn). Your diagnosis is right and the fix is in the right place: the harness-owned clamp lands after the provider-final boundary was hashed, so the fingerprint has to compare the effective tool input. The three tests are the right shape too — the clamp is clean, a real summary rewrite is still rewritten, and a change to another argument is still rewritten.

One change requested before merge: do not duplicate the clamp — import it.

The normalization you re-implemented in semanticToolCallArguments is already exported by the package that owns it, and that package is already a dependency of packages/coding-agent:

  • @code-yeongyu/senpi-codemodesrc/tool/eval-request.ts exports clampEvalSummary(value: unknown): string | undefined, with exactly the semantics your copy reproduces: trim(), collapse \s+ (unicode), empty becomes undefined, otherwise truncate to EVAL_SUMMARY_MAX_LENGTH - "...".length and append the ellipsis.
  • EVAL_SUMMARY_MAX_LENGTH = 80 is exported from that package's src/tool/types.ts.
  • packages/coding-agent/package.json already declares @code-yeongyu/senpi-codemode.

As written, the invariant now lives in two places across a package boundary with nothing binding them. If the eval tool ever changes its limit or its ellipsis, continuity silently starts classifying clean turns as rewritten again — and the regression test added here would still pass, because it hardcodes the same two numbers on both sides of the assertion. That is the same failure class this PR is fixing, one release later.

Concretely: call clampEvalSummary (and delete the local EVAL_SUMMARY_MAX_LENGTH / ELLIPSIS constants and the normalization body), keep the eval-only and peek/stop guards and the fail-closed behavior for every other tool exactly as you have them. Then one of the tests should drive the real tool's prepareArguments output rather than a hand-written 80-character string, so the two sides cannot drift without a red test.

If you would rather not extend this PR, say so and I will land the import swap as a small follow-up right after this merges — I do not want a review round trip to hold a correct fix. The behavior you shipped is what we want either way.

@Altairpaca
Altairpaca force-pushed the fix/1472-eval-summary-continuity branch from 3a809ba to fc00ace Compare September 16, 2026 05:32

Copy link
Copy Markdown
Author

Addressed the requested review change in fc00aceef3be605d6a349afa62eff499d74a3a4a.

  • Removed the duplicated continuity-side clamp and now call the codemode-owned clampEvalSummary.
  • Preserved the existing eval-only, peek/stop, string-summary and fail-closed guards.
  • Changed the regression to derive the committed summary through the real eval tool prepareArguments path rather than a hand-written 80-character value.
  • Kept the source-only package safe for the compiled Node path by exposing the canonical helper as a narrow .mjs leaf with a .d.mts declaration; eval-request.ts re-exports the same helper and types.ts re-exports the same limit, so there is still one implementation and one constant.
  • Rebased/squashed the PR to one commit on current main (c68ec4d3c0a2dc5fd105da69b2ab4d4e898e7e4d).

I also checked the final 7-file diff and exercised the package export with isolated Node ESM / TypeScript resolution plus npm pack --dry-run. I am not claiming a repository-suite green run: the new CI, Changelog gate and binary-parity runs are all currently action_required pending repository approval.

One repository-side gate detail: this GitHub App connection cannot apply the no-changelog label (403). I left release changelog handling unchanged rather than manufacturing a contributor-side release entry.

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.

[Bug]: eval summary clamping triggers assistant_rewritten with claude-sdk-oauth

2 participants