feat(benchmark): compare Kimi coding protocols - #1451

Merged
Astro-Han merged 1 commit into
apache:mainfrom
Nyvo-io:feat/1269-kimi-protocol-ab
Jul 28, 2026
Merged

feat(benchmark): compare Kimi coding protocols#1451
Astro-Han merged 1 commit into
apache:mainfrom
Nyvo-io:feat/1269-kimi-protocol-ab

Conversation

@Nyvo-io

@Nyvo-ioNyvo-io commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • keep this PR as the benchmark-harness layer requested in the maintainer split
  • compare the existing Kimi Coding Plan connection over Anthropic Messages and OpenAI Chat while changing only the selected protocol
  • run a paired smoke preflight before the remaining paid matrix, validate complete continuation-aware request traces, and retain immutable resumable artifacts
  • keep recommendations conservative: require unchanged correctness, a successful pair, complete request telemetry, material improvement, and no regression in the other measured dimensions
  • preserve normal multi-version Harbor caches by selecting the newest requested task version
  • fail closed when invalid host protocol overrides cross the Harbor host-cell boundary

Stack

Verification

  • npm ci
  • npm run build:test
  • npm run test:dist
  • focused launcher and Headless benchmark suites
  • npm run typecheck
  • npm run lint
  • npm run format:check
  • git diff --check
  • complete local diff review and independent Codex review: no findings
  • GitHub CI: typecheck, test, and e2e all passed on 6a9a8b89

No paid/account-plan benchmark was run. The Anthropic-compatible default remains unchanged.

Refs #1269.

@Astro-Han

Copy link
Copy Markdown
Contributor

I do not think this PR is ready to merge in its current shape. The concern is not the line count by itself: the PR combines three independently owned seams—runtime protocol support, request-trace evidence, and the benchmark harness—and the concrete failures found during review align with those boundaries.

I recommend converting this PR back to draft and splitting it before continuing review.

Blocking findings

Runtime protocol support

  • Empty reasoning is not preserved. model-adapter.ts drops reasoning_content: "" / reasoning: "" because it only emits a thinking event when the string is non-empty. Preserved-thinking backends require the field to round-trip even when empty, so a valid tool-call response can make the next request fail.
  • The detected OpenAI reasoning dialect has the wrong lifetime. createKimiOpenAiTransport() stores it in a model-instance closure, while AiSdkBackend.send() resolves a new model for every user turn. An endpoint that returns reasoning works for later steps in the first turn, then the next turn starts again with reasoning_content.
  • The new product-loop test covers multiple provider steps inside one send(), but not two user turns or an empty reasoning field, so both lifecycle failures remain green.

Request-trace evidence

  • requestPayloadWithoutProviderOptionsHash was added as a required field while the capture remains schema version 1. The new reader silently drops valid traces created before this PR; reading a legacy v1 capture now produces captures: [].
  • The comparison validates capture/attempt consistency internally, but does not bind the trace’s run/session/turn identity to the task execution that returned the path. The new success fixture itself uses task-specific runtime refs but fixed run-1 / session-1 trace events and still passes.
  • Malformed or torn attempt records are silently skipped. If another attempt still references the capture, validation passes and request count, failures, latency, and token usage can be understated.

Benchmark harness and recommendation

  • The immutable/resume fingerprint does not cover the full environment inherited by Harbor. Variables such as MAKA_CONTEXT_*, continuation settings, and stream/command timeouts can change between resumes, mixing different runtime policies into one supposedly single-variable comparison.
  • The smoke comparison runs only after every paid task, repetition, and arm has completed. A shared request-shape drift therefore consumes the full run before failing instead of acting as a preflight gate.
  • The documented kimi-for-coding option is currently guaranteed to fail that smoke check: Anthropic and OpenAI captures expose different SDK-level maxOutputTokens values even though the final wire limit is the same.
  • The recommendation gate can produce openai_candidate when both arms fail every task, provided there are enough paired failures to pass the non-inferiority calculation. It also treats any one-metric improvement as sufficient, including a 1 ms latency difference while another metric regresses without bound.
  • A terminal event without a trace poisons the immutable run: resume retrieves the same event and throws again instead of producing an invalid/not-cleared report.
  • The default Harbor task cache root fails when a task has multiple cached versions, which is a normal local-cache state. The documented dry-run is reproducibly blocked by duplicate task IDs.
  • The documented dry-run and paid-run commands do not set a stable run ID, so they validate and execute different manifests by default.

CI is green, and the happy-path product-loop coverage is useful, but the current tests do not establish the core experiment invariants: cross-turn reasoning replay, empty reasoning, legacy trace compatibility, execution identity, resume under environment changes, real cross-protocol captures, or complete ordered tool-call arguments/results.

Suggested split

  1. Runtime protocol support

    • Existing Kimi provider protocol selection
    • Model/runtime/factory and OpenAI transport changes
    • Cross-turn reasoning, empty reasoning, tool pairing, and usage contract tests
  2. Request-trace evidence

    • Protocol-independent request payload hash
    • Schema/version compatibility
    • Trace identity and fail-closed completeness validation
    • Reader and artifact compatibility tests
  3. Benchmark harness

    • Kimi protocol A/B runner and Harbor wiring
    • Immutable manifest/resume behavior
    • Smoke preflight and recommendation policy
    • Launcher tests and documentation

The first two seams can be reviewed as independent flat PRs from main. The benchmark PR can then be stacked on them because it genuinely depends on both. Tests should remain with the behavior they verify rather than being split into separate test-only PRs.

This split is about ownership, verification, and independent rollback—not splitting for size alone. It should also make the remaining fixes substantially easier to reason about and review.

@Nyvo-io
Nyvo-io marked this pull request as draft July 24, 2026 11:59
@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch 3 times, most recently from fc001e0 to 8ed7c03CompareJuly 26, 2026 15:30
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Rebuilt this PR according to the requested split. The runtime protocol layer is in merged #1462, the request-trace layer is in #1464, and this branch now contains the benchmark harness stacked on #1464 (c1917e8f) rather than the previous combined implementation.

The benchmark findings from the original review are covered: inherited runtime policy is fingerprinted, the paired smoke check runs before the remaining matrix, equivalent output limits are normalized, recommendations require successful comparable evidence and material non-regressing improvement, terminal missing-trace outcomes resume as stable invalid results, multi-version caches select the newest requested task, and dry/paid runs share an explicit immutable run ID. The host-cell protocol boundary also now preserves invalid values so Kimi validation fails closed.

Local full headless: 1394 tests, 1393 passed, 1 Pier-Python skip, 0 failed. Typecheck/lint/format/diff checks pass, the independent Codex review reported no findings, and GitHub typecheck/test/e2e are all green on 8ed7c034.

I am leaving #1451 as draft while #1464 remains its open dependency.

@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch from 8ed7c03 to 6a9a8b8CompareJuly 28, 2026 08:35
@Nyvo-io
Nyvo-io marked this pull request as ready for review July 28, 2026 10:21
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Ready for re-review at 6a9a8b8. The branch is now one benchmark-only commit on current main after #1462 and #1464 merged. The original harness findings were rechecked against the final diff; local build, full serial workspace tests, typecheck, lint, format, focused benchmark tests, and an independent Codex review all passed with no findings. GitHub typecheck, test, and e2e are green. No paid benchmark was run, and the Anthropic-compatible default is unchanged.

@Astro-Han

Copy link
Copy Markdown
Contributor

Reviewed 6a9a8b89. The benchmark-only split looks right, but one experiment-validity issue remains:

  • [P1] compareKimiProtocolSmokeTrace() checks only the first step === 0 capture and always reports requestCount: 1. Continuation traces can contain several provider requests, so a later request can change its shared history or non-protocol parameters without failing the smoke gate. That can produce an openai_candidate result from a confounded run. Please compare every capture in order, require matching request/step counts, and add a test where a request after the first one differs.

The earlier split, resume, and recommendation-gate concerns otherwise look addressed.

@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch from 6a9a8b8 to 730c630CompareJuly 28, 2026 14:39
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Ready for re-review at 730c6302. The smoke gate now compares every capture in original order, requires matching request counts and step sequences, checks shared/model/non-protocol evidence per request, and verifies both arms use the same provider connection while differing in provider_options. Continuation regressions cover later-history drift plus count and step mismatches. Local repository build, full serial workspace tests, typecheck, lint, format, diff checks, and independent Codex review all passed; GitHub typecheck, test, and e2e are green. No paid benchmark was run.

@Astro-HanAstro-Han 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.

Rechecked 730c6302. The earlier P1 is fixed: the smoke gate now compares every provider request in order and fails on count, step, shared-history, model, connection, non-protocol parameter, or provider-options drift. The continuation regressions cover the previously missed later-request case. Focused tests pass 26/26, CI is green, and I found no remaining P0–P3 issues.

@Astro-Han
Astro-Han merged commit 43de24f into apache:mainJul 28, 2026
3 checks passed
Sign up for freeto 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.

2 participants

@Nyvo-io@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

feat(benchmark): compare Kimi coding protocols - #1451

Merged
Astro-Han merged 1 commit into
apache:mainfrom
Nyvo-io:feat/1269-kimi-protocol-ab
Jul 28, 2026
Merged

feat(benchmark): compare Kimi coding protocols#1451
Astro-Han merged 1 commit into
apache:mainfrom
Nyvo-io:feat/1269-kimi-protocol-ab

Conversation

@Nyvo-io

@Nyvo-ioNyvo-io commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • keep this PR as the benchmark-harness layer requested in the maintainer split
  • compare the existing Kimi Coding Plan connection over Anthropic Messages and OpenAI Chat while changing only the selected protocol
  • run a paired smoke preflight before the remaining paid matrix, validate complete continuation-aware request traces, and retain immutable resumable artifacts
  • keep recommendations conservative: require unchanged correctness, a successful pair, complete request telemetry, material improvement, and no regression in the other measured dimensions
  • preserve normal multi-version Harbor caches by selecting the newest requested task version
  • fail closed when invalid host protocol overrides cross the Harbor host-cell boundary

Stack

Verification

  • npm ci
  • npm run build:test
  • npm run test:dist
  • focused launcher and Headless benchmark suites
  • npm run typecheck
  • npm run lint
  • npm run format:check
  • git diff --check
  • complete local diff review and independent Codex review: no findings
  • GitHub CI: typecheck, test, and e2e all passed on 6a9a8b89

No paid/account-plan benchmark was run. The Anthropic-compatible default remains unchanged.

Refs #1269.

@Astro-Han

Copy link
Copy Markdown
Contributor

I do not think this PR is ready to merge in its current shape. The concern is not the line count by itself: the PR combines three independently owned seams—runtime protocol support, request-trace evidence, and the benchmark harness—and the concrete failures found during review align with those boundaries.

I recommend converting this PR back to draft and splitting it before continuing review.

Blocking findings

Runtime protocol support

  • Empty reasoning is not preserved. model-adapter.ts drops reasoning_content: "" / reasoning: "" because it only emits a thinking event when the string is non-empty. Preserved-thinking backends require the field to round-trip even when empty, so a valid tool-call response can make the next request fail.
  • The detected OpenAI reasoning dialect has the wrong lifetime. createKimiOpenAiTransport() stores it in a model-instance closure, while AiSdkBackend.send() resolves a new model for every user turn. An endpoint that returns reasoning works for later steps in the first turn, then the next turn starts again with reasoning_content.
  • The new product-loop test covers multiple provider steps inside one send(), but not two user turns or an empty reasoning field, so both lifecycle failures remain green.

Request-trace evidence

  • requestPayloadWithoutProviderOptionsHash was added as a required field while the capture remains schema version 1. The new reader silently drops valid traces created before this PR; reading a legacy v1 capture now produces captures: [].
  • The comparison validates capture/attempt consistency internally, but does not bind the trace’s run/session/turn identity to the task execution that returned the path. The new success fixture itself uses task-specific runtime refs but fixed run-1 / session-1 trace events and still passes.
  • Malformed or torn attempt records are silently skipped. If another attempt still references the capture, validation passes and request count, failures, latency, and token usage can be understated.

Benchmark harness and recommendation

  • The immutable/resume fingerprint does not cover the full environment inherited by Harbor. Variables such as MAKA_CONTEXT_*, continuation settings, and stream/command timeouts can change between resumes, mixing different runtime policies into one supposedly single-variable comparison.
  • The smoke comparison runs only after every paid task, repetition, and arm has completed. A shared request-shape drift therefore consumes the full run before failing instead of acting as a preflight gate.
  • The documented kimi-for-coding option is currently guaranteed to fail that smoke check: Anthropic and OpenAI captures expose different SDK-level maxOutputTokens values even though the final wire limit is the same.
  • The recommendation gate can produce openai_candidate when both arms fail every task, provided there are enough paired failures to pass the non-inferiority calculation. It also treats any one-metric improvement as sufficient, including a 1 ms latency difference while another metric regresses without bound.
  • A terminal event without a trace poisons the immutable run: resume retrieves the same event and throws again instead of producing an invalid/not-cleared report.
  • The default Harbor task cache root fails when a task has multiple cached versions, which is a normal local-cache state. The documented dry-run is reproducibly blocked by duplicate task IDs.
  • The documented dry-run and paid-run commands do not set a stable run ID, so they validate and execute different manifests by default.

CI is green, and the happy-path product-loop coverage is useful, but the current tests do not establish the core experiment invariants: cross-turn reasoning replay, empty reasoning, legacy trace compatibility, execution identity, resume under environment changes, real cross-protocol captures, or complete ordered tool-call arguments/results.

Suggested split

  1. Runtime protocol support

    • Existing Kimi provider protocol selection
    • Model/runtime/factory and OpenAI transport changes
    • Cross-turn reasoning, empty reasoning, tool pairing, and usage contract tests
  2. Request-trace evidence

    • Protocol-independent request payload hash
    • Schema/version compatibility
    • Trace identity and fail-closed completeness validation
    • Reader and artifact compatibility tests
  3. Benchmark harness

    • Kimi protocol A/B runner and Harbor wiring
    • Immutable manifest/resume behavior
    • Smoke preflight and recommendation policy
    • Launcher tests and documentation

The first two seams can be reviewed as independent flat PRs from main. The benchmark PR can then be stacked on them because it genuinely depends on both. Tests should remain with the behavior they verify rather than being split into separate test-only PRs.

This split is about ownership, verification, and independent rollback—not splitting for size alone. It should also make the remaining fixes substantially easier to reason about and review.

@Nyvo-io
Nyvo-io marked this pull request as draft July 24, 2026 11:59
@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch 3 times, most recently from fc001e0 to 8ed7c03CompareJuly 26, 2026 15:30
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Rebuilt this PR according to the requested split. The runtime protocol layer is in merged #1462, the request-trace layer is in #1464, and this branch now contains the benchmark harness stacked on #1464 (c1917e8f) rather than the previous combined implementation.

The benchmark findings from the original review are covered: inherited runtime policy is fingerprinted, the paired smoke check runs before the remaining matrix, equivalent output limits are normalized, recommendations require successful comparable evidence and material non-regressing improvement, terminal missing-trace outcomes resume as stable invalid results, multi-version caches select the newest requested task, and dry/paid runs share an explicit immutable run ID. The host-cell protocol boundary also now preserves invalid values so Kimi validation fails closed.

Local full headless: 1394 tests, 1393 passed, 1 Pier-Python skip, 0 failed. Typecheck/lint/format/diff checks pass, the independent Codex review reported no findings, and GitHub typecheck/test/e2e are all green on 8ed7c034.

I am leaving #1451 as draft while #1464 remains its open dependency.

@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch from 8ed7c03 to 6a9a8b8CompareJuly 28, 2026 08:35
@Nyvo-io
Nyvo-io marked this pull request as ready for review July 28, 2026 10:21
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Ready for re-review at 6a9a8b8. The branch is now one benchmark-only commit on current main after #1462 and #1464 merged. The original harness findings were rechecked against the final diff; local build, full serial workspace tests, typecheck, lint, format, focused benchmark tests, and an independent Codex review all passed with no findings. GitHub typecheck, test, and e2e are green. No paid benchmark was run, and the Anthropic-compatible default is unchanged.

@Astro-Han

Copy link
Copy Markdown
Contributor

Reviewed 6a9a8b89. The benchmark-only split looks right, but one experiment-validity issue remains:

  • [P1] compareKimiProtocolSmokeTrace() checks only the first step === 0 capture and always reports requestCount: 1. Continuation traces can contain several provider requests, so a later request can change its shared history or non-protocol parameters without failing the smoke gate. That can produce an openai_candidate result from a confounded run. Please compare every capture in order, require matching request/step counts, and add a test where a request after the first one differs.

The earlier split, resume, and recommendation-gate concerns otherwise look addressed.

@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch from 6a9a8b8 to 730c630CompareJuly 28, 2026 14:39
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Ready for re-review at 730c6302. The smoke gate now compares every capture in original order, requires matching request counts and step sequences, checks shared/model/non-protocol evidence per request, and verifies both arms use the same provider connection while differing in provider_options. Continuation regressions cover later-history drift plus count and step mismatches. Local repository build, full serial workspace tests, typecheck, lint, format, diff checks, and independent Codex review all passed; GitHub typecheck, test, and e2e are green. No paid benchmark was run.

@Astro-HanAstro-Han 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.

Rechecked 730c6302. The earlier P1 is fixed: the smoke gate now compares every provider request in order and fails on count, step, shared-history, model, connection, non-protocol parameter, or provider-options drift. The continuation regressions cover the previously missed later-request case. Focused tests pass 26/26, CI is green, and I found no remaining P0–P3 issues.

@Astro-Han
Astro-Han merged commit 43de24f into apache:mainJul 28, 2026
3 checks passed
Sign up for freeto 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.

2 participants

@Nyvo-io@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(benchmark): compare Kimi coding protocols - #1451

Merged
Astro-Han merged 1 commit into
apache:mainfrom
Nyvo-io:feat/1269-kimi-protocol-ab
Jul 28, 2026
Merged

feat(benchmark): compare Kimi coding protocols#1451
Astro-Han merged 1 commit into
apache:mainfrom
Nyvo-io:feat/1269-kimi-protocol-ab

Conversation

@Nyvo-io

@Nyvo-ioNyvo-io commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • keep this PR as the benchmark-harness layer requested in the maintainer split
  • compare the existing Kimi Coding Plan connection over Anthropic Messages and OpenAI Chat while changing only the selected protocol
  • run a paired smoke preflight before the remaining paid matrix, validate complete continuation-aware request traces, and retain immutable resumable artifacts
  • keep recommendations conservative: require unchanged correctness, a successful pair, complete request telemetry, material improvement, and no regression in the other measured dimensions
  • preserve normal multi-version Harbor caches by selecting the newest requested task version
  • fail closed when invalid host protocol overrides cross the Harbor host-cell boundary

Stack

Verification

  • npm ci
  • npm run build:test
  • npm run test:dist
  • focused launcher and Headless benchmark suites
  • npm run typecheck
  • npm run lint
  • npm run format:check
  • git diff --check
  • complete local diff review and independent Codex review: no findings
  • GitHub CI: typecheck, test, and e2e all passed on 6a9a8b89

No paid/account-plan benchmark was run. The Anthropic-compatible default remains unchanged.

Refs #1269.

@Astro-Han

Copy link
Copy Markdown
Contributor

I do not think this PR is ready to merge in its current shape. The concern is not the line count by itself: the PR combines three independently owned seams—runtime protocol support, request-trace evidence, and the benchmark harness—and the concrete failures found during review align with those boundaries.

I recommend converting this PR back to draft and splitting it before continuing review.

Blocking findings

Runtime protocol support

  • Empty reasoning is not preserved. model-adapter.ts drops reasoning_content: "" / reasoning: "" because it only emits a thinking event when the string is non-empty. Preserved-thinking backends require the field to round-trip even when empty, so a valid tool-call response can make the next request fail.
  • The detected OpenAI reasoning dialect has the wrong lifetime. createKimiOpenAiTransport() stores it in a model-instance closure, while AiSdkBackend.send() resolves a new model for every user turn. An endpoint that returns reasoning works for later steps in the first turn, then the next turn starts again with reasoning_content.
  • The new product-loop test covers multiple provider steps inside one send(), but not two user turns or an empty reasoning field, so both lifecycle failures remain green.

Request-trace evidence

  • requestPayloadWithoutProviderOptionsHash was added as a required field while the capture remains schema version 1. The new reader silently drops valid traces created before this PR; reading a legacy v1 capture now produces captures: [].
  • The comparison validates capture/attempt consistency internally, but does not bind the trace’s run/session/turn identity to the task execution that returned the path. The new success fixture itself uses task-specific runtime refs but fixed run-1 / session-1 trace events and still passes.
  • Malformed or torn attempt records are silently skipped. If another attempt still references the capture, validation passes and request count, failures, latency, and token usage can be understated.

Benchmark harness and recommendation

  • The immutable/resume fingerprint does not cover the full environment inherited by Harbor. Variables such as MAKA_CONTEXT_*, continuation settings, and stream/command timeouts can change between resumes, mixing different runtime policies into one supposedly single-variable comparison.
  • The smoke comparison runs only after every paid task, repetition, and arm has completed. A shared request-shape drift therefore consumes the full run before failing instead of acting as a preflight gate.
  • The documented kimi-for-coding option is currently guaranteed to fail that smoke check: Anthropic and OpenAI captures expose different SDK-level maxOutputTokens values even though the final wire limit is the same.
  • The recommendation gate can produce openai_candidate when both arms fail every task, provided there are enough paired failures to pass the non-inferiority calculation. It also treats any one-metric improvement as sufficient, including a 1 ms latency difference while another metric regresses without bound.
  • A terminal event without a trace poisons the immutable run: resume retrieves the same event and throws again instead of producing an invalid/not-cleared report.
  • The default Harbor task cache root fails when a task has multiple cached versions, which is a normal local-cache state. The documented dry-run is reproducibly blocked by duplicate task IDs.
  • The documented dry-run and paid-run commands do not set a stable run ID, so they validate and execute different manifests by default.

CI is green, and the happy-path product-loop coverage is useful, but the current tests do not establish the core experiment invariants: cross-turn reasoning replay, empty reasoning, legacy trace compatibility, execution identity, resume under environment changes, real cross-protocol captures, or complete ordered tool-call arguments/results.

Suggested split

  1. Runtime protocol support

    • Existing Kimi provider protocol selection
    • Model/runtime/factory and OpenAI transport changes
    • Cross-turn reasoning, empty reasoning, tool pairing, and usage contract tests
  2. Request-trace evidence

    • Protocol-independent request payload hash
    • Schema/version compatibility
    • Trace identity and fail-closed completeness validation
    • Reader and artifact compatibility tests
  3. Benchmark harness

    • Kimi protocol A/B runner and Harbor wiring
    • Immutable manifest/resume behavior
    • Smoke preflight and recommendation policy
    • Launcher tests and documentation

The first two seams can be reviewed as independent flat PRs from main. The benchmark PR can then be stacked on them because it genuinely depends on both. Tests should remain with the behavior they verify rather than being split into separate test-only PRs.

This split is about ownership, verification, and independent rollback—not splitting for size alone. It should also make the remaining fixes substantially easier to reason about and review.

@Nyvo-io
Nyvo-io marked this pull request as draft July 24, 2026 11:59
@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch 3 times, most recently from fc001e0 to 8ed7c03CompareJuly 26, 2026 15:30
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Rebuilt this PR according to the requested split. The runtime protocol layer is in merged #1462, the request-trace layer is in #1464, and this branch now contains the benchmark harness stacked on #1464 (c1917e8f) rather than the previous combined implementation.

The benchmark findings from the original review are covered: inherited runtime policy is fingerprinted, the paired smoke check runs before the remaining matrix, equivalent output limits are normalized, recommendations require successful comparable evidence and material non-regressing improvement, terminal missing-trace outcomes resume as stable invalid results, multi-version caches select the newest requested task, and dry/paid runs share an explicit immutable run ID. The host-cell protocol boundary also now preserves invalid values so Kimi validation fails closed.

Local full headless: 1394 tests, 1393 passed, 1 Pier-Python skip, 0 failed. Typecheck/lint/format/diff checks pass, the independent Codex review reported no findings, and GitHub typecheck/test/e2e are all green on 8ed7c034.

I am leaving #1451 as draft while #1464 remains its open dependency.

@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch from 8ed7c03 to 6a9a8b8CompareJuly 28, 2026 08:35
@Nyvo-io
Nyvo-io marked this pull request as ready for review July 28, 2026 10:21
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Ready for re-review at 6a9a8b8. The branch is now one benchmark-only commit on current main after #1462 and #1464 merged. The original harness findings were rechecked against the final diff; local build, full serial workspace tests, typecheck, lint, format, focused benchmark tests, and an independent Codex review all passed with no findings. GitHub typecheck, test, and e2e are green. No paid benchmark was run, and the Anthropic-compatible default is unchanged.

@Astro-Han

Copy link
Copy Markdown
Contributor

Reviewed 6a9a8b89. The benchmark-only split looks right, but one experiment-validity issue remains:

  • [P1] compareKimiProtocolSmokeTrace() checks only the first step === 0 capture and always reports requestCount: 1. Continuation traces can contain several provider requests, so a later request can change its shared history or non-protocol parameters without failing the smoke gate. That can produce an openai_candidate result from a confounded run. Please compare every capture in order, require matching request/step counts, and add a test where a request after the first one differs.

The earlier split, resume, and recommendation-gate concerns otherwise look addressed.

@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch from 6a9a8b8 to 730c630CompareJuly 28, 2026 14:39
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Ready for re-review at 730c6302. The smoke gate now compares every capture in original order, requires matching request counts and step sequences, checks shared/model/non-protocol evidence per request, and verifies both arms use the same provider connection while differing in provider_options. Continuation regressions cover later-history drift plus count and step mismatches. Local repository build, full serial workspace tests, typecheck, lint, format, diff checks, and independent Codex review all passed; GitHub typecheck, test, and e2e are green. No paid benchmark was run.

@Astro-HanAstro-Han 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.

Rechecked 730c6302. The earlier P1 is fixed: the smoke gate now compares every provider request in order and fails on count, step, shared-history, model, connection, non-protocol parameter, or provider-options drift. The continuation regressions cover the previously missed later-request case. Focused tests pass 26/26, CI is green, and I found no remaining P0–P3 issues.

@Astro-Han
Astro-Han merged commit 43de24f into apache:mainJul 28, 2026
3 checks passed
Sign up for freeto 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.

2 participants

@Nyvo-io@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(benchmark): compare Kimi coding protocols - #1451

Merged
Astro-Han merged 1 commit into
apache:mainfrom
Nyvo-io:feat/1269-kimi-protocol-ab
Jul 28, 2026
Merged

feat(benchmark): compare Kimi coding protocols#1451
Astro-Han merged 1 commit into
apache:mainfrom
Nyvo-io:feat/1269-kimi-protocol-ab

Conversation

@Nyvo-io

@Nyvo-ioNyvo-io commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • keep this PR as the benchmark-harness layer requested in the maintainer split
  • compare the existing Kimi Coding Plan connection over Anthropic Messages and OpenAI Chat while changing only the selected protocol
  • run a paired smoke preflight before the remaining paid matrix, validate complete continuation-aware request traces, and retain immutable resumable artifacts
  • keep recommendations conservative: require unchanged correctness, a successful pair, complete request telemetry, material improvement, and no regression in the other measured dimensions
  • preserve normal multi-version Harbor caches by selecting the newest requested task version
  • fail closed when invalid host protocol overrides cross the Harbor host-cell boundary

Stack

Verification

  • npm ci
  • npm run build:test
  • npm run test:dist
  • focused launcher and Headless benchmark suites
  • npm run typecheck
  • npm run lint
  • npm run format:check
  • git diff --check
  • complete local diff review and independent Codex review: no findings
  • GitHub CI: typecheck, test, and e2e all passed on 6a9a8b89

No paid/account-plan benchmark was run. The Anthropic-compatible default remains unchanged.

Refs #1269.

@Astro-Han

Copy link
Copy Markdown
Contributor

I do not think this PR is ready to merge in its current shape. The concern is not the line count by itself: the PR combines three independently owned seams—runtime protocol support, request-trace evidence, and the benchmark harness—and the concrete failures found during review align with those boundaries.

I recommend converting this PR back to draft and splitting it before continuing review.

Blocking findings

Runtime protocol support

  • Empty reasoning is not preserved. model-adapter.ts drops reasoning_content: "" / reasoning: "" because it only emits a thinking event when the string is non-empty. Preserved-thinking backends require the field to round-trip even when empty, so a valid tool-call response can make the next request fail.
  • The detected OpenAI reasoning dialect has the wrong lifetime. createKimiOpenAiTransport() stores it in a model-instance closure, while AiSdkBackend.send() resolves a new model for every user turn. An endpoint that returns reasoning works for later steps in the first turn, then the next turn starts again with reasoning_content.
  • The new product-loop test covers multiple provider steps inside one send(), but not two user turns or an empty reasoning field, so both lifecycle failures remain green.

Request-trace evidence

  • requestPayloadWithoutProviderOptionsHash was added as a required field while the capture remains schema version 1. The new reader silently drops valid traces created before this PR; reading a legacy v1 capture now produces captures: [].
  • The comparison validates capture/attempt consistency internally, but does not bind the trace’s run/session/turn identity to the task execution that returned the path. The new success fixture itself uses task-specific runtime refs but fixed run-1 / session-1 trace events and still passes.
  • Malformed or torn attempt records are silently skipped. If another attempt still references the capture, validation passes and request count, failures, latency, and token usage can be understated.

Benchmark harness and recommendation

  • The immutable/resume fingerprint does not cover the full environment inherited by Harbor. Variables such as MAKA_CONTEXT_*, continuation settings, and stream/command timeouts can change between resumes, mixing different runtime policies into one supposedly single-variable comparison.
  • The smoke comparison runs only after every paid task, repetition, and arm has completed. A shared request-shape drift therefore consumes the full run before failing instead of acting as a preflight gate.
  • The documented kimi-for-coding option is currently guaranteed to fail that smoke check: Anthropic and OpenAI captures expose different SDK-level maxOutputTokens values even though the final wire limit is the same.
  • The recommendation gate can produce openai_candidate when both arms fail every task, provided there are enough paired failures to pass the non-inferiority calculation. It also treats any one-metric improvement as sufficient, including a 1 ms latency difference while another metric regresses without bound.
  • A terminal event without a trace poisons the immutable run: resume retrieves the same event and throws again instead of producing an invalid/not-cleared report.
  • The default Harbor task cache root fails when a task has multiple cached versions, which is a normal local-cache state. The documented dry-run is reproducibly blocked by duplicate task IDs.
  • The documented dry-run and paid-run commands do not set a stable run ID, so they validate and execute different manifests by default.

CI is green, and the happy-path product-loop coverage is useful, but the current tests do not establish the core experiment invariants: cross-turn reasoning replay, empty reasoning, legacy trace compatibility, execution identity, resume under environment changes, real cross-protocol captures, or complete ordered tool-call arguments/results.

Suggested split

  1. Runtime protocol support

    • Existing Kimi provider protocol selection
    • Model/runtime/factory and OpenAI transport changes
    • Cross-turn reasoning, empty reasoning, tool pairing, and usage contract tests
  2. Request-trace evidence

    • Protocol-independent request payload hash
    • Schema/version compatibility
    • Trace identity and fail-closed completeness validation
    • Reader and artifact compatibility tests
  3. Benchmark harness

    • Kimi protocol A/B runner and Harbor wiring
    • Immutable manifest/resume behavior
    • Smoke preflight and recommendation policy
    • Launcher tests and documentation

The first two seams can be reviewed as independent flat PRs from main. The benchmark PR can then be stacked on them because it genuinely depends on both. Tests should remain with the behavior they verify rather than being split into separate test-only PRs.

This split is about ownership, verification, and independent rollback—not splitting for size alone. It should also make the remaining fixes substantially easier to reason about and review.

@Nyvo-io
Nyvo-io marked this pull request as draft July 24, 2026 11:59
@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch 3 times, most recently from fc001e0 to 8ed7c03CompareJuly 26, 2026 15:30
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Rebuilt this PR according to the requested split. The runtime protocol layer is in merged #1462, the request-trace layer is in #1464, and this branch now contains the benchmark harness stacked on #1464 (c1917e8f) rather than the previous combined implementation.

The benchmark findings from the original review are covered: inherited runtime policy is fingerprinted, the paired smoke check runs before the remaining matrix, equivalent output limits are normalized, recommendations require successful comparable evidence and material non-regressing improvement, terminal missing-trace outcomes resume as stable invalid results, multi-version caches select the newest requested task, and dry/paid runs share an explicit immutable run ID. The host-cell protocol boundary also now preserves invalid values so Kimi validation fails closed.

Local full headless: 1394 tests, 1393 passed, 1 Pier-Python skip, 0 failed. Typecheck/lint/format/diff checks pass, the independent Codex review reported no findings, and GitHub typecheck/test/e2e are all green on 8ed7c034.

I am leaving #1451 as draft while #1464 remains its open dependency.

@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch from 8ed7c03 to 6a9a8b8CompareJuly 28, 2026 08:35
@Nyvo-io
Nyvo-io marked this pull request as ready for review July 28, 2026 10:21
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Ready for re-review at 6a9a8b8. The branch is now one benchmark-only commit on current main after #1462 and #1464 merged. The original harness findings were rechecked against the final diff; local build, full serial workspace tests, typecheck, lint, format, focused benchmark tests, and an independent Codex review all passed with no findings. GitHub typecheck, test, and e2e are green. No paid benchmark was run, and the Anthropic-compatible default is unchanged.

@Astro-Han

Copy link
Copy Markdown
Contributor

Reviewed 6a9a8b89. The benchmark-only split looks right, but one experiment-validity issue remains:

  • [P1] compareKimiProtocolSmokeTrace() checks only the first step === 0 capture and always reports requestCount: 1. Continuation traces can contain several provider requests, so a later request can change its shared history or non-protocol parameters without failing the smoke gate. That can produce an openai_candidate result from a confounded run. Please compare every capture in order, require matching request/step counts, and add a test where a request after the first one differs.

The earlier split, resume, and recommendation-gate concerns otherwise look addressed.

@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch from 6a9a8b8 to 730c630CompareJuly 28, 2026 14:39
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Ready for re-review at 730c6302. The smoke gate now compares every capture in original order, requires matching request counts and step sequences, checks shared/model/non-protocol evidence per request, and verifies both arms use the same provider connection while differing in provider_options. Continuation regressions cover later-history drift plus count and step mismatches. Local repository build, full serial workspace tests, typecheck, lint, format, diff checks, and independent Codex review all passed; GitHub typecheck, test, and e2e are green. No paid benchmark was run.

@Astro-HanAstro-Han 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.

Rechecked 730c6302. The earlier P1 is fixed: the smoke gate now compares every provider request in order and fails on count, step, shared-history, model, connection, non-protocol parameter, or provider-options drift. The continuation regressions cover the previously missed later-request case. Focused tests pass 26/26, CI is green, and I found no remaining P0–P3 issues.

@Astro-Han
Astro-Han merged commit 43de24f into apache:mainJul 28, 2026
3 checks passed
Sign up for freeto 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.

2 participants

@Nyvo-io@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

feat(benchmark): compare Kimi coding protocols - #1451

Merged
Astro-Han merged 1 commit into
apache:mainfrom
Nyvo-io:feat/1269-kimi-protocol-ab
Jul 28, 2026
Merged

feat(benchmark): compare Kimi coding protocols#1451
Astro-Han merged 1 commit into
apache:mainfrom
Nyvo-io:feat/1269-kimi-protocol-ab

Conversation

@Nyvo-io

@Nyvo-ioNyvo-io commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • keep this PR as the benchmark-harness layer requested in the maintainer split
  • compare the existing Kimi Coding Plan connection over Anthropic Messages and OpenAI Chat while changing only the selected protocol
  • run a paired smoke preflight before the remaining paid matrix, validate complete continuation-aware request traces, and retain immutable resumable artifacts
  • keep recommendations conservative: require unchanged correctness, a successful pair, complete request telemetry, material improvement, and no regression in the other measured dimensions
  • preserve normal multi-version Harbor caches by selecting the newest requested task version
  • fail closed when invalid host protocol overrides cross the Harbor host-cell boundary

Stack

Verification

  • npm ci
  • npm run build:test
  • npm run test:dist
  • focused launcher and Headless benchmark suites
  • npm run typecheck
  • npm run lint
  • npm run format:check
  • git diff --check
  • complete local diff review and independent Codex review: no findings
  • GitHub CI: typecheck, test, and e2e all passed on 6a9a8b89

No paid/account-plan benchmark was run. The Anthropic-compatible default remains unchanged.

Refs #1269.

@Astro-Han

Copy link
Copy Markdown
Contributor

I do not think this PR is ready to merge in its current shape. The concern is not the line count by itself: the PR combines three independently owned seams—runtime protocol support, request-trace evidence, and the benchmark harness—and the concrete failures found during review align with those boundaries.

I recommend converting this PR back to draft and splitting it before continuing review.

Blocking findings

Runtime protocol support

  • Empty reasoning is not preserved. model-adapter.ts drops reasoning_content: "" / reasoning: "" because it only emits a thinking event when the string is non-empty. Preserved-thinking backends require the field to round-trip even when empty, so a valid tool-call response can make the next request fail.
  • The detected OpenAI reasoning dialect has the wrong lifetime. createKimiOpenAiTransport() stores it in a model-instance closure, while AiSdkBackend.send() resolves a new model for every user turn. An endpoint that returns reasoning works for later steps in the first turn, then the next turn starts again with reasoning_content.
  • The new product-loop test covers multiple provider steps inside one send(), but not two user turns or an empty reasoning field, so both lifecycle failures remain green.

Request-trace evidence

  • requestPayloadWithoutProviderOptionsHash was added as a required field while the capture remains schema version 1. The new reader silently drops valid traces created before this PR; reading a legacy v1 capture now produces captures: [].
  • The comparison validates capture/attempt consistency internally, but does not bind the trace’s run/session/turn identity to the task execution that returned the path. The new success fixture itself uses task-specific runtime refs but fixed run-1 / session-1 trace events and still passes.
  • Malformed or torn attempt records are silently skipped. If another attempt still references the capture, validation passes and request count, failures, latency, and token usage can be understated.

Benchmark harness and recommendation

  • The immutable/resume fingerprint does not cover the full environment inherited by Harbor. Variables such as MAKA_CONTEXT_*, continuation settings, and stream/command timeouts can change between resumes, mixing different runtime policies into one supposedly single-variable comparison.
  • The smoke comparison runs only after every paid task, repetition, and arm has completed. A shared request-shape drift therefore consumes the full run before failing instead of acting as a preflight gate.
  • The documented kimi-for-coding option is currently guaranteed to fail that smoke check: Anthropic and OpenAI captures expose different SDK-level maxOutputTokens values even though the final wire limit is the same.
  • The recommendation gate can produce openai_candidate when both arms fail every task, provided there are enough paired failures to pass the non-inferiority calculation. It also treats any one-metric improvement as sufficient, including a 1 ms latency difference while another metric regresses without bound.
  • A terminal event without a trace poisons the immutable run: resume retrieves the same event and throws again instead of producing an invalid/not-cleared report.
  • The default Harbor task cache root fails when a task has multiple cached versions, which is a normal local-cache state. The documented dry-run is reproducibly blocked by duplicate task IDs.
  • The documented dry-run and paid-run commands do not set a stable run ID, so they validate and execute different manifests by default.

CI is green, and the happy-path product-loop coverage is useful, but the current tests do not establish the core experiment invariants: cross-turn reasoning replay, empty reasoning, legacy trace compatibility, execution identity, resume under environment changes, real cross-protocol captures, or complete ordered tool-call arguments/results.

Suggested split

  1. Runtime protocol support

    • Existing Kimi provider protocol selection
    • Model/runtime/factory and OpenAI transport changes
    • Cross-turn reasoning, empty reasoning, tool pairing, and usage contract tests
  2. Request-trace evidence

    • Protocol-independent request payload hash
    • Schema/version compatibility
    • Trace identity and fail-closed completeness validation
    • Reader and artifact compatibility tests
  3. Benchmark harness

    • Kimi protocol A/B runner and Harbor wiring
    • Immutable manifest/resume behavior
    • Smoke preflight and recommendation policy
    • Launcher tests and documentation

The first two seams can be reviewed as independent flat PRs from main. The benchmark PR can then be stacked on them because it genuinely depends on both. Tests should remain with the behavior they verify rather than being split into separate test-only PRs.

This split is about ownership, verification, and independent rollback—not splitting for size alone. It should also make the remaining fixes substantially easier to reason about and review.

@Nyvo-io
Nyvo-io marked this pull request as draft July 24, 2026 11:59
@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch 3 times, most recently from fc001e0 to 8ed7c03CompareJuly 26, 2026 15:30
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Rebuilt this PR according to the requested split. The runtime protocol layer is in merged #1462, the request-trace layer is in #1464, and this branch now contains the benchmark harness stacked on #1464 (c1917e8f) rather than the previous combined implementation.

The benchmark findings from the original review are covered: inherited runtime policy is fingerprinted, the paired smoke check runs before the remaining matrix, equivalent output limits are normalized, recommendations require successful comparable evidence and material non-regressing improvement, terminal missing-trace outcomes resume as stable invalid results, multi-version caches select the newest requested task, and dry/paid runs share an explicit immutable run ID. The host-cell protocol boundary also now preserves invalid values so Kimi validation fails closed.

Local full headless: 1394 tests, 1393 passed, 1 Pier-Python skip, 0 failed. Typecheck/lint/format/diff checks pass, the independent Codex review reported no findings, and GitHub typecheck/test/e2e are all green on 8ed7c034.

I am leaving #1451 as draft while #1464 remains its open dependency.

@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch from 8ed7c03 to 6a9a8b8CompareJuly 28, 2026 08:35
@Nyvo-io
Nyvo-io marked this pull request as ready for review July 28, 2026 10:21
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Ready for re-review at 6a9a8b8. The branch is now one benchmark-only commit on current main after #1462 and #1464 merged. The original harness findings were rechecked against the final diff; local build, full serial workspace tests, typecheck, lint, format, focused benchmark tests, and an independent Codex review all passed with no findings. GitHub typecheck, test, and e2e are green. No paid benchmark was run, and the Anthropic-compatible default is unchanged.

@Astro-Han

Copy link
Copy Markdown
Contributor

Reviewed 6a9a8b89. The benchmark-only split looks right, but one experiment-validity issue remains:

  • [P1] compareKimiProtocolSmokeTrace() checks only the first step === 0 capture and always reports requestCount: 1. Continuation traces can contain several provider requests, so a later request can change its shared history or non-protocol parameters without failing the smoke gate. That can produce an openai_candidate result from a confounded run. Please compare every capture in order, require matching request/step counts, and add a test where a request after the first one differs.

The earlier split, resume, and recommendation-gate concerns otherwise look addressed.

@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch from 6a9a8b8 to 730c630CompareJuly 28, 2026 14:39
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Ready for re-review at 730c6302. The smoke gate now compares every capture in original order, requires matching request counts and step sequences, checks shared/model/non-protocol evidence per request, and verifies both arms use the same provider connection while differing in provider_options. Continuation regressions cover later-history drift plus count and step mismatches. Local repository build, full serial workspace tests, typecheck, lint, format, diff checks, and independent Codex review all passed; GitHub typecheck, test, and e2e are green. No paid benchmark was run.

@Astro-HanAstro-Han 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.

Rechecked 730c6302. The earlier P1 is fixed: the smoke gate now compares every provider request in order and fails on count, step, shared-history, model, connection, non-protocol parameter, or provider-options drift. The continuation regressions cover the previously missed later-request case. Focused tests pass 26/26, CI is green, and I found no remaining P0–P3 issues.

@Astro-Han
Astro-Han merged commit 43de24f into apache:mainJul 28, 2026
3 checks passed
Sign up for freeto 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.

2 participants

@Nyvo-io@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(benchmark): compare Kimi coding protocols - #1451

Merged
Astro-Han merged 1 commit into
apache:mainfrom
Nyvo-io:feat/1269-kimi-protocol-ab
Jul 28, 2026
Merged

feat(benchmark): compare Kimi coding protocols#1451
Astro-Han merged 1 commit into
apache:mainfrom
Nyvo-io:feat/1269-kimi-protocol-ab

Conversation

@Nyvo-io

@Nyvo-ioNyvo-io commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • keep this PR as the benchmark-harness layer requested in the maintainer split
  • compare the existing Kimi Coding Plan connection over Anthropic Messages and OpenAI Chat while changing only the selected protocol
  • run a paired smoke preflight before the remaining paid matrix, validate complete continuation-aware request traces, and retain immutable resumable artifacts
  • keep recommendations conservative: require unchanged correctness, a successful pair, complete request telemetry, material improvement, and no regression in the other measured dimensions
  • preserve normal multi-version Harbor caches by selecting the newest requested task version
  • fail closed when invalid host protocol overrides cross the Harbor host-cell boundary

Stack

Verification

  • npm ci
  • npm run build:test
  • npm run test:dist
  • focused launcher and Headless benchmark suites
  • npm run typecheck
  • npm run lint
  • npm run format:check
  • git diff --check
  • complete local diff review and independent Codex review: no findings
  • GitHub CI: typecheck, test, and e2e all passed on 6a9a8b89

No paid/account-plan benchmark was run. The Anthropic-compatible default remains unchanged.

Refs #1269.

@Astro-Han

Copy link
Copy Markdown
Contributor

I do not think this PR is ready to merge in its current shape. The concern is not the line count by itself: the PR combines three independently owned seams—runtime protocol support, request-trace evidence, and the benchmark harness—and the concrete failures found during review align with those boundaries.

I recommend converting this PR back to draft and splitting it before continuing review.

Blocking findings

Runtime protocol support

  • Empty reasoning is not preserved. model-adapter.ts drops reasoning_content: "" / reasoning: "" because it only emits a thinking event when the string is non-empty. Preserved-thinking backends require the field to round-trip even when empty, so a valid tool-call response can make the next request fail.
  • The detected OpenAI reasoning dialect has the wrong lifetime. createKimiOpenAiTransport() stores it in a model-instance closure, while AiSdkBackend.send() resolves a new model for every user turn. An endpoint that returns reasoning works for later steps in the first turn, then the next turn starts again with reasoning_content.
  • The new product-loop test covers multiple provider steps inside one send(), but not two user turns or an empty reasoning field, so both lifecycle failures remain green.

Request-trace evidence

  • requestPayloadWithoutProviderOptionsHash was added as a required field while the capture remains schema version 1. The new reader silently drops valid traces created before this PR; reading a legacy v1 capture now produces captures: [].
  • The comparison validates capture/attempt consistency internally, but does not bind the trace’s run/session/turn identity to the task execution that returned the path. The new success fixture itself uses task-specific runtime refs but fixed run-1 / session-1 trace events and still passes.
  • Malformed or torn attempt records are silently skipped. If another attempt still references the capture, validation passes and request count, failures, latency, and token usage can be understated.

Benchmark harness and recommendation

  • The immutable/resume fingerprint does not cover the full environment inherited by Harbor. Variables such as MAKA_CONTEXT_*, continuation settings, and stream/command timeouts can change between resumes, mixing different runtime policies into one supposedly single-variable comparison.
  • The smoke comparison runs only after every paid task, repetition, and arm has completed. A shared request-shape drift therefore consumes the full run before failing instead of acting as a preflight gate.
  • The documented kimi-for-coding option is currently guaranteed to fail that smoke check: Anthropic and OpenAI captures expose different SDK-level maxOutputTokens values even though the final wire limit is the same.
  • The recommendation gate can produce openai_candidate when both arms fail every task, provided there are enough paired failures to pass the non-inferiority calculation. It also treats any one-metric improvement as sufficient, including a 1 ms latency difference while another metric regresses without bound.
  • A terminal event without a trace poisons the immutable run: resume retrieves the same event and throws again instead of producing an invalid/not-cleared report.
  • The default Harbor task cache root fails when a task has multiple cached versions, which is a normal local-cache state. The documented dry-run is reproducibly blocked by duplicate task IDs.
  • The documented dry-run and paid-run commands do not set a stable run ID, so they validate and execute different manifests by default.

CI is green, and the happy-path product-loop coverage is useful, but the current tests do not establish the core experiment invariants: cross-turn reasoning replay, empty reasoning, legacy trace compatibility, execution identity, resume under environment changes, real cross-protocol captures, or complete ordered tool-call arguments/results.

Suggested split

  1. Runtime protocol support

    • Existing Kimi provider protocol selection
    • Model/runtime/factory and OpenAI transport changes
    • Cross-turn reasoning, empty reasoning, tool pairing, and usage contract tests
  2. Request-trace evidence

    • Protocol-independent request payload hash
    • Schema/version compatibility
    • Trace identity and fail-closed completeness validation
    • Reader and artifact compatibility tests
  3. Benchmark harness

    • Kimi protocol A/B runner and Harbor wiring
    • Immutable manifest/resume behavior
    • Smoke preflight and recommendation policy
    • Launcher tests and documentation

The first two seams can be reviewed as independent flat PRs from main. The benchmark PR can then be stacked on them because it genuinely depends on both. Tests should remain with the behavior they verify rather than being split into separate test-only PRs.

This split is about ownership, verification, and independent rollback—not splitting for size alone. It should also make the remaining fixes substantially easier to reason about and review.

@Nyvo-io
Nyvo-io marked this pull request as draft July 24, 2026 11:59
@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch 3 times, most recently from fc001e0 to 8ed7c03CompareJuly 26, 2026 15:30
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Rebuilt this PR according to the requested split. The runtime protocol layer is in merged #1462, the request-trace layer is in #1464, and this branch now contains the benchmark harness stacked on #1464 (c1917e8f) rather than the previous combined implementation.

The benchmark findings from the original review are covered: inherited runtime policy is fingerprinted, the paired smoke check runs before the remaining matrix, equivalent output limits are normalized, recommendations require successful comparable evidence and material non-regressing improvement, terminal missing-trace outcomes resume as stable invalid results, multi-version caches select the newest requested task, and dry/paid runs share an explicit immutable run ID. The host-cell protocol boundary also now preserves invalid values so Kimi validation fails closed.

Local full headless: 1394 tests, 1393 passed, 1 Pier-Python skip, 0 failed. Typecheck/lint/format/diff checks pass, the independent Codex review reported no findings, and GitHub typecheck/test/e2e are all green on 8ed7c034.

I am leaving #1451 as draft while #1464 remains its open dependency.

@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch from 8ed7c03 to 6a9a8b8CompareJuly 28, 2026 08:35
@Nyvo-io
Nyvo-io marked this pull request as ready for review July 28, 2026 10:21
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Ready for re-review at 6a9a8b8. The branch is now one benchmark-only commit on current main after #1462 and #1464 merged. The original harness findings were rechecked against the final diff; local build, full serial workspace tests, typecheck, lint, format, focused benchmark tests, and an independent Codex review all passed with no findings. GitHub typecheck, test, and e2e are green. No paid benchmark was run, and the Anthropic-compatible default is unchanged.

@Astro-Han

Copy link
Copy Markdown
Contributor

Reviewed 6a9a8b89. The benchmark-only split looks right, but one experiment-validity issue remains:

  • [P1] compareKimiProtocolSmokeTrace() checks only the first step === 0 capture and always reports requestCount: 1. Continuation traces can contain several provider requests, so a later request can change its shared history or non-protocol parameters without failing the smoke gate. That can produce an openai_candidate result from a confounded run. Please compare every capture in order, require matching request/step counts, and add a test where a request after the first one differs.

The earlier split, resume, and recommendation-gate concerns otherwise look addressed.

@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch from 6a9a8b8 to 730c630CompareJuly 28, 2026 14:39
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Ready for re-review at 730c6302. The smoke gate now compares every capture in original order, requires matching request counts and step sequences, checks shared/model/non-protocol evidence per request, and verifies both arms use the same provider connection while differing in provider_options. Continuation regressions cover later-history drift plus count and step mismatches. Local repository build, full serial workspace tests, typecheck, lint, format, diff checks, and independent Codex review all passed; GitHub typecheck, test, and e2e are green. No paid benchmark was run.

@Astro-HanAstro-Han 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.

Rechecked 730c6302. The earlier P1 is fixed: the smoke gate now compares every provider request in order and fails on count, step, shared-history, model, connection, non-protocol parameter, or provider-options drift. The continuation regressions cover the previously missed later-request case. Focused tests pass 26/26, CI is green, and I found no remaining P0–P3 issues.

@Astro-Han
Astro-Han merged commit 43de24f into apache:mainJul 28, 2026
3 checks passed
Sign up for freeto 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.

2 participants

@Nyvo-io@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(benchmark): compare Kimi coding protocols - #1451

Merged
Astro-Han merged 1 commit into
apache:mainfrom
Nyvo-io:feat/1269-kimi-protocol-ab
Jul 28, 2026
Merged

feat(benchmark): compare Kimi coding protocols#1451
Astro-Han merged 1 commit into
apache:mainfrom
Nyvo-io:feat/1269-kimi-protocol-ab

Conversation

@Nyvo-io

@Nyvo-ioNyvo-io commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • keep this PR as the benchmark-harness layer requested in the maintainer split
  • compare the existing Kimi Coding Plan connection over Anthropic Messages and OpenAI Chat while changing only the selected protocol
  • run a paired smoke preflight before the remaining paid matrix, validate complete continuation-aware request traces, and retain immutable resumable artifacts
  • keep recommendations conservative: require unchanged correctness, a successful pair, complete request telemetry, material improvement, and no regression in the other measured dimensions
  • preserve normal multi-version Harbor caches by selecting the newest requested task version
  • fail closed when invalid host protocol overrides cross the Harbor host-cell boundary

Stack

Verification

  • npm ci
  • npm run build:test
  • npm run test:dist
  • focused launcher and Headless benchmark suites
  • npm run typecheck
  • npm run lint
  • npm run format:check
  • git diff --check
  • complete local diff review and independent Codex review: no findings
  • GitHub CI: typecheck, test, and e2e all passed on 6a9a8b89

No paid/account-plan benchmark was run. The Anthropic-compatible default remains unchanged.

Refs #1269.

@Astro-Han

Copy link
Copy Markdown
Contributor

I do not think this PR is ready to merge in its current shape. The concern is not the line count by itself: the PR combines three independently owned seams—runtime protocol support, request-trace evidence, and the benchmark harness—and the concrete failures found during review align with those boundaries.

I recommend converting this PR back to draft and splitting it before continuing review.

Blocking findings

Runtime protocol support

  • Empty reasoning is not preserved. model-adapter.ts drops reasoning_content: "" / reasoning: "" because it only emits a thinking event when the string is non-empty. Preserved-thinking backends require the field to round-trip even when empty, so a valid tool-call response can make the next request fail.
  • The detected OpenAI reasoning dialect has the wrong lifetime. createKimiOpenAiTransport() stores it in a model-instance closure, while AiSdkBackend.send() resolves a new model for every user turn. An endpoint that returns reasoning works for later steps in the first turn, then the next turn starts again with reasoning_content.
  • The new product-loop test covers multiple provider steps inside one send(), but not two user turns or an empty reasoning field, so both lifecycle failures remain green.

Request-trace evidence

  • requestPayloadWithoutProviderOptionsHash was added as a required field while the capture remains schema version 1. The new reader silently drops valid traces created before this PR; reading a legacy v1 capture now produces captures: [].
  • The comparison validates capture/attempt consistency internally, but does not bind the trace’s run/session/turn identity to the task execution that returned the path. The new success fixture itself uses task-specific runtime refs but fixed run-1 / session-1 trace events and still passes.
  • Malformed or torn attempt records are silently skipped. If another attempt still references the capture, validation passes and request count, failures, latency, and token usage can be understated.

Benchmark harness and recommendation

  • The immutable/resume fingerprint does not cover the full environment inherited by Harbor. Variables such as MAKA_CONTEXT_*, continuation settings, and stream/command timeouts can change between resumes, mixing different runtime policies into one supposedly single-variable comparison.
  • The smoke comparison runs only after every paid task, repetition, and arm has completed. A shared request-shape drift therefore consumes the full run before failing instead of acting as a preflight gate.
  • The documented kimi-for-coding option is currently guaranteed to fail that smoke check: Anthropic and OpenAI captures expose different SDK-level maxOutputTokens values even though the final wire limit is the same.
  • The recommendation gate can produce openai_candidate when both arms fail every task, provided there are enough paired failures to pass the non-inferiority calculation. It also treats any one-metric improvement as sufficient, including a 1 ms latency difference while another metric regresses without bound.
  • A terminal event without a trace poisons the immutable run: resume retrieves the same event and throws again instead of producing an invalid/not-cleared report.
  • The default Harbor task cache root fails when a task has multiple cached versions, which is a normal local-cache state. The documented dry-run is reproducibly blocked by duplicate task IDs.
  • The documented dry-run and paid-run commands do not set a stable run ID, so they validate and execute different manifests by default.

CI is green, and the happy-path product-loop coverage is useful, but the current tests do not establish the core experiment invariants: cross-turn reasoning replay, empty reasoning, legacy trace compatibility, execution identity, resume under environment changes, real cross-protocol captures, or complete ordered tool-call arguments/results.

Suggested split

  1. Runtime protocol support

    • Existing Kimi provider protocol selection
    • Model/runtime/factory and OpenAI transport changes
    • Cross-turn reasoning, empty reasoning, tool pairing, and usage contract tests
  2. Request-trace evidence

    • Protocol-independent request payload hash
    • Schema/version compatibility
    • Trace identity and fail-closed completeness validation
    • Reader and artifact compatibility tests
  3. Benchmark harness

    • Kimi protocol A/B runner and Harbor wiring
    • Immutable manifest/resume behavior
    • Smoke preflight and recommendation policy
    • Launcher tests and documentation

The first two seams can be reviewed as independent flat PRs from main. The benchmark PR can then be stacked on them because it genuinely depends on both. Tests should remain with the behavior they verify rather than being split into separate test-only PRs.

This split is about ownership, verification, and independent rollback—not splitting for size alone. It should also make the remaining fixes substantially easier to reason about and review.

@Nyvo-io
Nyvo-io marked this pull request as draft July 24, 2026 11:59
@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch 3 times, most recently from fc001e0 to 8ed7c03CompareJuly 26, 2026 15:30
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Rebuilt this PR according to the requested split. The runtime protocol layer is in merged #1462, the request-trace layer is in #1464, and this branch now contains the benchmark harness stacked on #1464 (c1917e8f) rather than the previous combined implementation.

The benchmark findings from the original review are covered: inherited runtime policy is fingerprinted, the paired smoke check runs before the remaining matrix, equivalent output limits are normalized, recommendations require successful comparable evidence and material non-regressing improvement, terminal missing-trace outcomes resume as stable invalid results, multi-version caches select the newest requested task, and dry/paid runs share an explicit immutable run ID. The host-cell protocol boundary also now preserves invalid values so Kimi validation fails closed.

Local full headless: 1394 tests, 1393 passed, 1 Pier-Python skip, 0 failed. Typecheck/lint/format/diff checks pass, the independent Codex review reported no findings, and GitHub typecheck/test/e2e are all green on 8ed7c034.

I am leaving #1451 as draft while #1464 remains its open dependency.

@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch from 8ed7c03 to 6a9a8b8CompareJuly 28, 2026 08:35
@Nyvo-io
Nyvo-io marked this pull request as ready for review July 28, 2026 10:21
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Ready for re-review at 6a9a8b8. The branch is now one benchmark-only commit on current main after #1462 and #1464 merged. The original harness findings were rechecked against the final diff; local build, full serial workspace tests, typecheck, lint, format, focused benchmark tests, and an independent Codex review all passed with no findings. GitHub typecheck, test, and e2e are green. No paid benchmark was run, and the Anthropic-compatible default is unchanged.

@Astro-Han

Copy link
Copy Markdown
Contributor

Reviewed 6a9a8b89. The benchmark-only split looks right, but one experiment-validity issue remains:

  • [P1] compareKimiProtocolSmokeTrace() checks only the first step === 0 capture and always reports requestCount: 1. Continuation traces can contain several provider requests, so a later request can change its shared history or non-protocol parameters without failing the smoke gate. That can produce an openai_candidate result from a confounded run. Please compare every capture in order, require matching request/step counts, and add a test where a request after the first one differs.

The earlier split, resume, and recommendation-gate concerns otherwise look addressed.

@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch from 6a9a8b8 to 730c630CompareJuly 28, 2026 14:39
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Ready for re-review at 730c6302. The smoke gate now compares every capture in original order, requires matching request counts and step sequences, checks shared/model/non-protocol evidence per request, and verifies both arms use the same provider connection while differing in provider_options. Continuation regressions cover later-history drift plus count and step mismatches. Local repository build, full serial workspace tests, typecheck, lint, format, diff checks, and independent Codex review all passed; GitHub typecheck, test, and e2e are green. No paid benchmark was run.

@Astro-HanAstro-Han 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.

Rechecked 730c6302. The earlier P1 is fixed: the smoke gate now compares every provider request in order and fails on count, step, shared-history, model, connection, non-protocol parameter, or provider-options drift. The continuation regressions cover the previously missed later-request case. Focused tests pass 26/26, CI is green, and I found no remaining P0–P3 issues.

@Astro-Han
Astro-Han merged commit 43de24f into apache:mainJul 28, 2026
3 checks passed
Sign up for freeto 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.

2 participants

@Nyvo-io@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

feat(benchmark): compare Kimi coding protocols - #1451

Merged
Astro-Han merged 1 commit into
apache:mainfrom
Nyvo-io:feat/1269-kimi-protocol-ab
Jul 28, 2026
Merged

feat(benchmark): compare Kimi coding protocols#1451
Astro-Han merged 1 commit into
apache:mainfrom
Nyvo-io:feat/1269-kimi-protocol-ab

Conversation

@Nyvo-io

@Nyvo-ioNyvo-io commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • keep this PR as the benchmark-harness layer requested in the maintainer split
  • compare the existing Kimi Coding Plan connection over Anthropic Messages and OpenAI Chat while changing only the selected protocol
  • run a paired smoke preflight before the remaining paid matrix, validate complete continuation-aware request traces, and retain immutable resumable artifacts
  • keep recommendations conservative: require unchanged correctness, a successful pair, complete request telemetry, material improvement, and no regression in the other measured dimensions
  • preserve normal multi-version Harbor caches by selecting the newest requested task version
  • fail closed when invalid host protocol overrides cross the Harbor host-cell boundary

Stack

Verification

  • npm ci
  • npm run build:test
  • npm run test:dist
  • focused launcher and Headless benchmark suites
  • npm run typecheck
  • npm run lint
  • npm run format:check
  • git diff --check
  • complete local diff review and independent Codex review: no findings
  • GitHub CI: typecheck, test, and e2e all passed on 6a9a8b89

No paid/account-plan benchmark was run. The Anthropic-compatible default remains unchanged.

Refs #1269.

@Astro-Han

Copy link
Copy Markdown
Contributor

I do not think this PR is ready to merge in its current shape. The concern is not the line count by itself: the PR combines three independently owned seams—runtime protocol support, request-trace evidence, and the benchmark harness—and the concrete failures found during review align with those boundaries.

I recommend converting this PR back to draft and splitting it before continuing review.

Blocking findings

Runtime protocol support

  • Empty reasoning is not preserved. model-adapter.ts drops reasoning_content: "" / reasoning: "" because it only emits a thinking event when the string is non-empty. Preserved-thinking backends require the field to round-trip even when empty, so a valid tool-call response can make the next request fail.
  • The detected OpenAI reasoning dialect has the wrong lifetime. createKimiOpenAiTransport() stores it in a model-instance closure, while AiSdkBackend.send() resolves a new model for every user turn. An endpoint that returns reasoning works for later steps in the first turn, then the next turn starts again with reasoning_content.
  • The new product-loop test covers multiple provider steps inside one send(), but not two user turns or an empty reasoning field, so both lifecycle failures remain green.

Request-trace evidence

  • requestPayloadWithoutProviderOptionsHash was added as a required field while the capture remains schema version 1. The new reader silently drops valid traces created before this PR; reading a legacy v1 capture now produces captures: [].
  • The comparison validates capture/attempt consistency internally, but does not bind the trace’s run/session/turn identity to the task execution that returned the path. The new success fixture itself uses task-specific runtime refs but fixed run-1 / session-1 trace events and still passes.
  • Malformed or torn attempt records are silently skipped. If another attempt still references the capture, validation passes and request count, failures, latency, and token usage can be understated.

Benchmark harness and recommendation

  • The immutable/resume fingerprint does not cover the full environment inherited by Harbor. Variables such as MAKA_CONTEXT_*, continuation settings, and stream/command timeouts can change between resumes, mixing different runtime policies into one supposedly single-variable comparison.
  • The smoke comparison runs only after every paid task, repetition, and arm has completed. A shared request-shape drift therefore consumes the full run before failing instead of acting as a preflight gate.
  • The documented kimi-for-coding option is currently guaranteed to fail that smoke check: Anthropic and OpenAI captures expose different SDK-level maxOutputTokens values even though the final wire limit is the same.
  • The recommendation gate can produce openai_candidate when both arms fail every task, provided there are enough paired failures to pass the non-inferiority calculation. It also treats any one-metric improvement as sufficient, including a 1 ms latency difference while another metric regresses without bound.
  • A terminal event without a trace poisons the immutable run: resume retrieves the same event and throws again instead of producing an invalid/not-cleared report.
  • The default Harbor task cache root fails when a task has multiple cached versions, which is a normal local-cache state. The documented dry-run is reproducibly blocked by duplicate task IDs.
  • The documented dry-run and paid-run commands do not set a stable run ID, so they validate and execute different manifests by default.

CI is green, and the happy-path product-loop coverage is useful, but the current tests do not establish the core experiment invariants: cross-turn reasoning replay, empty reasoning, legacy trace compatibility, execution identity, resume under environment changes, real cross-protocol captures, or complete ordered tool-call arguments/results.

Suggested split

  1. Runtime protocol support

    • Existing Kimi provider protocol selection
    • Model/runtime/factory and OpenAI transport changes
    • Cross-turn reasoning, empty reasoning, tool pairing, and usage contract tests
  2. Request-trace evidence

    • Protocol-independent request payload hash
    • Schema/version compatibility
    • Trace identity and fail-closed completeness validation
    • Reader and artifact compatibility tests
  3. Benchmark harness

    • Kimi protocol A/B runner and Harbor wiring
    • Immutable manifest/resume behavior
    • Smoke preflight and recommendation policy
    • Launcher tests and documentation

The first two seams can be reviewed as independent flat PRs from main. The benchmark PR can then be stacked on them because it genuinely depends on both. Tests should remain with the behavior they verify rather than being split into separate test-only PRs.

This split is about ownership, verification, and independent rollback—not splitting for size alone. It should also make the remaining fixes substantially easier to reason about and review.

@Nyvo-io
Nyvo-io marked this pull request as draft July 24, 2026 11:59
@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch 3 times, most recently from fc001e0 to 8ed7c03CompareJuly 26, 2026 15:30
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Rebuilt this PR according to the requested split. The runtime protocol layer is in merged #1462, the request-trace layer is in #1464, and this branch now contains the benchmark harness stacked on #1464 (c1917e8f) rather than the previous combined implementation.

The benchmark findings from the original review are covered: inherited runtime policy is fingerprinted, the paired smoke check runs before the remaining matrix, equivalent output limits are normalized, recommendations require successful comparable evidence and material non-regressing improvement, terminal missing-trace outcomes resume as stable invalid results, multi-version caches select the newest requested task, and dry/paid runs share an explicit immutable run ID. The host-cell protocol boundary also now preserves invalid values so Kimi validation fails closed.

Local full headless: 1394 tests, 1393 passed, 1 Pier-Python skip, 0 failed. Typecheck/lint/format/diff checks pass, the independent Codex review reported no findings, and GitHub typecheck/test/e2e are all green on 8ed7c034.

I am leaving #1451 as draft while #1464 remains its open dependency.

@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch from 8ed7c03 to 6a9a8b8CompareJuly 28, 2026 08:35
@Nyvo-io
Nyvo-io marked this pull request as ready for review July 28, 2026 10:21
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Ready for re-review at 6a9a8b8. The branch is now one benchmark-only commit on current main after #1462 and #1464 merged. The original harness findings were rechecked against the final diff; local build, full serial workspace tests, typecheck, lint, format, focused benchmark tests, and an independent Codex review all passed with no findings. GitHub typecheck, test, and e2e are green. No paid benchmark was run, and the Anthropic-compatible default is unchanged.

@Astro-Han

Copy link
Copy Markdown
Contributor

Reviewed 6a9a8b89. The benchmark-only split looks right, but one experiment-validity issue remains:

  • [P1] compareKimiProtocolSmokeTrace() checks only the first step === 0 capture and always reports requestCount: 1. Continuation traces can contain several provider requests, so a later request can change its shared history or non-protocol parameters without failing the smoke gate. That can produce an openai_candidate result from a confounded run. Please compare every capture in order, require matching request/step counts, and add a test where a request after the first one differs.

The earlier split, resume, and recommendation-gate concerns otherwise look addressed.

@Nyvo-io
Nyvo-ioforce-pushed the feat/1269-kimi-protocol-ab branch from 6a9a8b8 to 730c630CompareJuly 28, 2026 14:39
@Nyvo-io

Copy link
Copy Markdown
ContributorAuthor

Ready for re-review at 730c6302. The smoke gate now compares every capture in original order, requires matching request counts and step sequences, checks shared/model/non-protocol evidence per request, and verifies both arms use the same provider connection while differing in provider_options. Continuation regressions cover later-history drift plus count and step mismatches. Local repository build, full serial workspace tests, typecheck, lint, format, diff checks, and independent Codex review all passed; GitHub typecheck, test, and e2e are green. No paid benchmark was run.

@Astro-HanAstro-Han 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.

Rechecked 730c6302. The earlier P1 is fixed: the smoke gate now compares every provider request in order and fails on count, step, shared-history, model, connection, non-protocol parameter, or provider-options drift. The continuation regressions cover the previously missed later-request case. Focused tests pass 26/26, CI is green, and I found no remaining P0–P3 issues.

@Astro-Han
Astro-Han merged commit 43de24f into apache:mainJul 28, 2026
3 checks passed
Sign up for freeto 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.

2 participants

@Nyvo-io@Astro-Han