Skip to content

fix(types): AbsorbConfig type surface — optional value fields + ResolvedAbsorbConfig - #503

Open
ranxianglei wants to merge 1 commit into
masterfrom
2026-10-03_absorb-config-type-surface
Open

ranxianglei wants to merge 1 commit into
masterfrom
2026-10-03_absorb-config-type-surface

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

Closes #502

What changed

Facet 1 (main): AbsorbConfig's declared surface was stricter than actual usage — all five fields were required, while both the kernel (resolveAbsorbConfig / defaultConfig spread merges, src/absorb.ts:42-44, src/config.ts:70-72) and consumers tolerate absence per field. Reproduced on v0.0.100 master: defaultConfig(200000, { absorb: { enabled: true } }) fails with TS2739 although it is runtime-valid.

  • src/types.ts: toolName / minToolTokens / contextThresholdPct / excludeTools now optional; enabled stays required as the activation toggle (matches the ImageCompressionConfig precedent; the issue allows either). Field docs carry the defaults.
  • src/types.ts: new export ResolvedAbsorbConfig = Required<AbsorbConfig> — the fully-populated shape resolveAbsorbConfig / defaultConfig produce. DEFAULT_ABSORB_CONFIG, the resolveAbsorbConfig return, EvaluateCrushInput.absorb, and internal isAcpOrConfiguredTool now use it, so entry points consuming a resolved config cannot silently accept partials after the loosening (they would compare against undefined).
  • src/config.ts: validateConfig absorb checks treat undefined as "default applies" (same pattern as the imageCompression checks directly below); explicitly invalid values are still rejected with identical messages.
  • tests/absorb.test.ts: three regression tests (partial→defaults resolution, per-field override merge, validation accepts partials but rejects explicit bad values).

The issue's secondary ask — "export a default-merge function consistent with resolveAbsorbSettings" — is already satisfied by the kernel itself: resolveAbsorbConfig + DEFAULT_ABSORB_CONFIG are exported from acp-kernel in current master (src/absorb.ts, re-exported via index.ts). No new function needed; the remaining gap was purely the required-fields surface.

Facet 2 (prompt_cache_key): no change, per the issue's own recommendation ① — keep string (follows the OpenAI spec; non-string wire values are client anomalies, and the consumer-side defense at billion-context src/session-id.ts:214-222 is already in the right place).

Behavior changes (disclosed)

  1. Type-surface widening (backward compatible): partial AbsorbConfig literals now typecheck; existing full literals unaffected. New type export is purely additive.
  2. validateConfig: an enabled absorb block missing toolName used to be reported as "absorb.toolName must be a non-empty string when enabled" (only reachable via JS/cast callers, since absence was unrepresentable under the old types); it now passes because the default "absorb" applies. Explicit empty string "" is still rejected. Old → new: missing = error → missing = OK (default fills in); explicit bad values unchanged. Reason: absence is merged over defaults everywhere in the kernel, so flagging it as invalid was a false positive.
  3. EvaluateCrushInput.absorb (public input interface): now requires the resolved shape (ResolvedAbsorbConfig). Callers passing fully-populated configs (all current call sites, incl. bili) are unaffected; partials that previously slipped through with undefined comparisons now fail at compile time.

No runtime-default, prompt-text, wire-byte, token-count, or pipeline-ordering changes. No new config keys or env vars.

Verification (local, targeted)

  • Repro probe: defaultConfig(200000, { absorb: { enabled: true } }) — before: TS2739 (4 missing props); after: clean.
  • npm run typecheck, npm run format:check, npm run build — green; built dist/types.d.ts shows the optional fields + ResolvedAbsorbConfig alias.
  • node --import tsx --test tests/absorb.test.ts tests/crush.test.ts tests/sync-config.test.ts — 80/80 pass. Full suite deferred to CI.

…e fields + ResolvedAbsorbConfig (#502)

The declared surface demanded all five AbsorbConfig fields while both the kernel (resolveAbsorbConfig, defaultConfig spread merges) and consumers tolerate absence per field. Make toolName/minToolTokens/contextThresholdPct/excludeTools optional (enabled stays required as the activation toggle, matching ImageCompressionConfig precedent), add ResolvedAbsorbConfig = Required<AbsorbConfig> for entry points consuming resolved configs (EvaluateCrushInput.absorb, DEFAULT_ABSORB_CONFIG, resolveAbsorbConfig return), and make validateConfig undefined-tolerant like the imageCompression checks. prompt_cache_key wire type intentionally unchanged (issue facet 2, option 1).
@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

📦 Built Package Artifact

Branch: 2026-10-03_absorb-config-type-surface (54cbab6)

Option A — Install from npm PR tag (recommended)

In your adapter project:

npm install acp-kernel@pr-503

Each push to this PR publishes a new version under the pr-503 npm tag.

Option B — Download artifact

  1. Download the artifact from the Actions run
  2. Extract the tarball and install:
tar xzf acp-kernel-pr503.tgz
npm install ./package

This comment is automatically updated on each push.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 收到 PR #503(对应 issue #502)。这是已完成工作的 [PR] 跟踪单,按规则我不重做,改为独立 review:拉取分支核对代码 + 独立确认 CI 状态,然后给出结构化七问结论。开始核对中。

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 PR #503 独立 review 完成(拉分支核对代码 + 独立确认 CI + 本地定向复验)。

核对过程

  • Base 新鲜:merge-base == origin/master tip(633b8c8),单 commit 54cbab6,diff 与描述一致(5 文件 +78/−19);无跑题文件、无空白/重排 churn —— diff 整洁 ✅
  • CI 独立确认(check-runs @ 54cbab6):build-artifact / test(22) / test(24) / pr-validation 全部 success ✅
  • 本地定向复验(pr-503 分支):npm run typecheck clean、format:check green、tests/absorb.test.ts tests/crush.test.ts tests/sync-config.test.ts 80/80 pass(全量以 CI 为准)
  • 回归清点(diff 触碰路径的既有行为,逐项):
    • defaultConfig absorb 合并(src/config.ts:70-72):字节级未变,输出仍 fully-populated;
    • resolveAbsorbConfig / DEFAULT_ABSORB_CONFIG(src/absorb.ts:42-44):spread 语义未变,仅类型标注放宽;
    • 内核所有运行时消费点都经 resolveAbsorbConfig(src/absorb.ts:93/136/262、src/crush.ts:1795)或只读必填字段 enabled(src/compress.ts:892/916)——放宽后不存在「字段缺席 → undefined 比较」的路径,TS strict + typecheck 绿在编译期兜底;
    • validateConfig 三条错误消息逐字未变,显式非法值拒绝语义不变;
    • prompt 文本 / wire 字节 / token 计数 / pipeline 顺序:均未触碰。

结构化七问

  1. 是什么问题:AbsorbConfig 声明五字段全必填,但内核与消费方运行时一直逐字段合入默认值——类型表面比实际契约严,任何 partial 构造场景(fixture、配置解析中间态)被迫填齐全部字段或 cast。
  2. 什么原因导致的:声明与实现漂移。运行时消费路径一直是 {...DEFAULT_ABSORB_CONFIG, ...(config.absorb ?? {})} 合并(src/absorb.ts:42-44、src/config.ts:70-72),字段缺席本就合法;类型却按「完整对象」声明。是声明-实现不一致,不是行为 bug。已在 v0.0.100 master 复现(TS2739)。
  3. 怎么修复的:把声明放宽到与实现一致(toolName/minToolTokens/contextThresholdPct/excludeTools 可选,enabled 保留必填作激活开关,与 ImageCompressionConfig 先例一致);新增 ResolvedAbsorbConfig = Required<AbsorbConfig> 钉住所有「消费已解析配置」的入口(DEFAULT_ABSORB_CONFIG、resolveAbsorbConfig 返回、EvaluateCrushInput.absorb、内部 isAcpOrConfiguredTool),防止放宽后 partial 从这些入口静默回流(否则会对 undefined 做比较);validateConfig 三条检查改「undefined = 默认生效」,与相邻 nudge.minPressureBenefitTokens / truncate.terminalEscapeAfter 同一模式。
  4. 是否有回归问题:无意回归未发现。一处有意变更(PR 已披露,我验证成立):enabled 但缺 toolName 的 absorb 块——旧代码报 "absorb.toolName must be a non-empty string when enabled"(旧类型下只能经 JS/cast 到达)→ 新代码通过,默认 "absorb" 生效;原判定是假阳性(缺席在内核各处都被合入默认)。显式空串 "" 仍报错,消息逐字不变。EvaluateCrushInput.absorb 收紧为 resolved 形态属编译期收紧:现有调用方(含 bili)传完整配置不受影响,传 partial 的潜在调用方本就会 undefined 比较,现在编译期暴露是收益。
  5. 是否破坏缓存:不涉及——prompt 文本、wire 字节、token 计数、渲染快照均未变,纯类型表面修复。
  6. 是否改了设计方向:不涉及——架构/数据流不变;ResolvedAbsorbConfig 纯增量导出(经 src/index.ts:1 export * from "./types.js" 进入公共 API),不触碰 §7.3 任何契约(wire 格式、ref 不可变性、pipeline 顺序、Prompts 规则均未动)。
  7. 是否引入新配置:无——无新 env/config key,默认值不变(toolName "absorb"、minToolTokens 4000、contextThresholdPct 0、excludeTools [])。

测试有效性:3 个新回归测试是真测试——断言精确解析值(4000 / 0 / [] / ABSORB_TOOL_NAME)而非恒真式,分别钉住 partial→defaults 解析、逐字段 override 合并、validateConfig「接受 partial / 拒绝显式坏值(-1 / 2 / "")」边界;若字段回退必填或 resolve 合并被破坏都能抓住。小缺口(不拦):minToolTokens: NaN(isFinite 分支)无用例,-1 用例覆盖了 <0 分支。

一项发现(非阻断):CcrConfig 存在与本单主 facet 同机制族的缺陷,本 PR 未覆盖——src/types.ts:236-245 内层字段全必填(types.ts:415 容器 ccr? 可选)+ src/ccr.ts:59-60 resolveCcrConfig 同为逐字段合入默认 + src/config.ts:235-249 validateConfig 无条件检查缺席字段(partial {enabled:true} ccr 块在 current master 会报 3 条假阳性错误)。#502 明确「同一机制的全部 facet 在本单内统一管理」,已在 #502 留评论请 owner 决定处置(reopen 补 facet 或另立);不影响本 PR 范围的正确性。

判定:可合并(CI 全绿 + 代码核对通过 + 无意回归未发现 + diff 干净;按仓库规则由人工 merge)。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant