Skip to content

fix: warn once per distinct config-validation error set (#346) - #347

Open
ranxianglei wants to merge 1 commit into
masterfrom
2026-09-22_dedup-config-warn-once
Open

ranxianglei wants to merge 1 commit into
masterfrom
2026-09-22_dedup-config-warn-once

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

Closes #346

Problem

processTurn unconditionally called console.warn on every turn whenever validateConfig returned errors. With nudge.minContextLimitPct > nudge.maxContextLimitPct (a valid soft-target setup exposed by billion-context — ranxianglei/billion-context#1122), that is one log line per upstream request in proxy deployments: unbounded noise for a configuration that works fine.

Verified: the two limits gate independent code paths in decideNudge (max → pressure zone at src/compress.ts:1222; min → first-sight mass bypass + tier-count usage floor at src/compress.ts:1291,1304). Nothing depends on [min, max) being non-empty, so thresholds do fire correctly — the warning is informational only.

Fix

Deduplicate per session/state, as suggested in the issue: remember the last emitted validation-error set on CompressionState.lastConfigWarnings (new optional field) and re-warn only when the set actually changes (config edit). Warning text unchanged.

  • src/compress.ts — processTurn compares the new error set against the stored one (sameWarningSet, order-insensitive) before emitting; writes the updated set back to state every turn ([] when valid).
  • src/types.ts — new optional field CompressionState.lastConfigWarnings?: string[].
  • src/state.ts — createInitialState() initializes it to [].
  • src/sync.ts, cloneState in src/compress.ts, mergeCompressionState in src/persist/state-merge.ts — all three explicit state constructors carry the field, so dedup memory survives both the per-turn sync clone and host persist/load cycles (the exact proxy scenario from validation: minContextLimitPct > maxContextLimitPct warns once per processTurn (log spam) #346). The sync clone was dropping it initially — caught by the new round-trip test.

Behavior change (old → new)

  • Before: one console.warn per processTurn call while any config-validation error exists (log spam proportional to request volume).
  • After: at most one console.warn per change of the validation-error set — once at first detection, again if the config is edited into a different invalid shape, and again across an invalid→valid→invalid transition. Silent for valid configs.
  • New optional state field: old persisted states load fine (field absent → first invalid turn warns once, same as before).
  • Caveat: hosts that discard/rebuild state from scratch every turn will still see one warning per turn — inherent to fully stateless usage; matches the "per session/state" semantics proposed in the issue.

Verification

  • 5 new tests in tests/validation.test.ts: warn-once across repeated turns (the validation: minContextLimitPct > maxContextLimitPct warns once per processTurn (log spam) #346 repro), re-warn on error-set change, re-warn after valid→invalid transition, silent + empty-set storage for valid configs, dedup memory surviving a JSON round-trip (host persist/load).
  • Full suite: 771/771 pass; tsc --noEmit clean; npm run build OK.
  • Note: npm run format:check fails repo-wide without this change too (no prettier config committed; ~107 files flagged incl. untouched ones like tsconfig.json). This diff follows the surrounding code style.

@github-actions

github-actions Bot commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

📦 Built Package Artifact

Branch: 2026-09-22_dedup-config-warn-once (7a6f88e)

Option A — Install from npm PR tag (recommended)

In your adapter project:

npm install acp-kernel@pr-347

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

Option B — Download artifact

  1. Download the artifact from the Actions run
  2. Extract the tarball and install:
tar xzf acp-kernel-pr347.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] 🏷 Verified PR #347 (fix: warn once per distinct config-validation error set, closes #346). I did not redo the work — I pulled the branch, reviewed the code, checked CI, and fixed one blocker that was preventing merge.

Code review — passes. Implementation is correct, minimal, well-targeted:

  • src/compress.ts:459 — processTurn warns only when configErrors.length > 0 && !sameWarningSet(input.state.lastConfigWarnings, configErrors), then writes the current set back every turn ([] when valid).
  • sameWarningSet (src/compress.ts:1336) — order-insensitive compare, handles undefined (pre-dedup states).
  • All four state constructors carry the new optional field — createInitialState (src/state.ts:19), cloneState (src/compress.ts:1665), syncBlocks (src/sync.ts:38), mergeCompressionState (src/persist/state-merge.ts:26) — so dedup memory survives both the per-turn sync clone and host persist/load (the exact validation: minContextLimitPct > maxContextLimitPct warns once per processTurn (log spam) #346 proxy scenario).
  • 5 regression tests in tests/validation.test.ts: warn-once (validation: minContextLimitPct > maxContextLimitPct warns once per processTurn (log spam) #346 repro), re-warn-on-set-change, valid→invalid→valid, silent-valid, JSON round-trip. Diff is clean (+143/-1, 6 files), no unrelated churn.

Blocker found & fixed: stale-base merge conflict. The PR branched from an older master; master had since advanced through releases v0.0.79–v0.0.82 and the acp_rule persistent-rules feature, which added terminalStreak/rules/nextRuleId to the same state constructors this PR touches. mergeable_state was dirty. I rebased onto current master (b30723f) and resolved all 5 conflicts as a pure union of field additions (kept both sides — no semantic overlap).

Verification after rebase (head fe70885):

  • Local: tsc --noEmit clean · full suite 874/874 pass · build OK.
  • Pushed to the PR branch; GitHub CI green: test (22), test (24), pr-validation, build-artifact — all success.
  • PR is now mergeable: true; mergeable_state: blocked = only the required-review/approval gate remains.

Not merging — merge is human-only per repo rules. Please approve + merge: #347

Notes:

  • Behavior change (already disclosed in PR body): one console.warn per change of the validation-error set instead of per turn; silent for valid configs; old persisted states load fine (field absent → first invalid turn warns once). Fully-stateless hosts still see one warn/turn (inherent).
  • npm run format:check failing repo-wide is pre-existing (no prettier config committed); my conflict resolution matched surrounding style, adds no new formatting delta.

中文摘要:修复了 PR #347 因落后于 master(期间合入 v0.0.79–v0.0.82 与 acp_rule 功能)导致的合并冲突——已 rebase 到最新 master 并按字段并集解决 5 处冲突;本地 typecheck + 874 项测试 + 构建全绿,GitHub CI(test 22/24、pr-validation、build-artifact)也全绿。代码实现正确、测试充分,diff 干净,现只差人工审批,可以合并。

@ranxianglei

Copy link
Copy Markdown
Owner Author

@ework-daemon 与 master 冲突了,请 rebase 到当前 master 后重推,CI 绿后待合。

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 已完成 rebase + 重推,CI 全绿,待合。

做了什么:

  1. master 又前进了(我上次 rebase 的基点 v0.0.82 → 现在已到 v0.0.89,期间合入 compress 端点落在被 orphan-hiding 隐藏的失败 compress 调用上时整条区间被拒为 cannot be anchored(应吸附到相邻可见消息) #396/fix: snap compress range endpoints onto hidden orphan calls (#396) #397/fix: preserve non-text blocks in Anthropic tool_result via sidecar #367/fix(persist): fence async commits against late-landing stale writes (#373) #409/fix: drop top-level anyOf from COMPRESS_PARAMETERS (Anthropic 400, #404) #405 等,新增了 imageFullRestored/imageShrinks/hiddenOrphanRefs 状态字段和 processTurn 里的 contentStore)→ 确实产生了新冲突。
  2. 把 PR 分支 rebase 到当前 master(615347a),4 处冲突全部按字段并集解决(两边各自新增的字段都保留,无语义重叠):src/state.ts、src/persist/state-merge.ts、src/types.ts、src/compress.ts(processTurn 里同时保留 lastConfigWarnings 写入与 contentStore;cloneState 自动合并且仍带该字段)。
  3. 重推:fe70885 → d00e96b(force-with-lease)。

验证:

  • 本地 pre-flight(rebased head d00e96b):tsc --noEmit 干净 · 全量测试 1086/1086 通过 · build OK。diff 相对 master 仍是干净的 +143/-1(6 文件),未混入 master 新代码。
  • GitHub CI 全绿:test (22) ✅ · test (24) ✅ · build-artifact ✅ · pr-validation ✅。
  • 分支已与当前 master 对齐(head 的父提交即 master 最新 tip 615347a,mergeable: true)。GitHub 页面若短暂显示 "behind" 是 force-push 后的异步计算延迟,git 侧已确认最新。

待合: 只差人工审批/合并(合并为人工操作,我不代合)。链接: #347

中文摘要:已将 PR #347 rebase 到当前 master(v0.0.89)并按字段并集解决 4 处冲突后重推(d00e96b),本地 typecheck+1086 测试+构建全绿,GitHub CI(test 22/24、build-artifact、pr-validation)也全绿,分支已对齐最新 master,可以合并。

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 说明:本条 rebase 请求发出约 40 分钟后,本 PR 的专属会话仍无任何进展(远端分支未动、无新提交),由 #412 审计会话代为执行 rebase。若本 PR 会话随后恢复运行,看到已推的 rebase 结果即可停止,无需重复。rebase 完成后我会在此回报(含门禁结果)。

@ranxianglei
ranxianglei force-pushed the 2026-09-22_dedup-config-warn-once branch from d00e96b to 7a6f88e Compare September 25, 2026 09:20
@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 Rebase 完成,分支已更新,可合。

  • 新 head:7a6f88e(原 d00e96b → rebase 到 master b86c064,含 fix: unify tool-message accounting across status report surfaces (issue #390) #177/test(persist): retarget missed fsp.rename mock from #409 (#410) #411 合并)。
  • 冲突 4 处,全部为同模式并集解(master 侧新增的 state 字段 vs 本 PR 新增字段,互不覆盖):
    • src/compress.ts processTurn():lastConfigWarnings = configErrors.length > 0 ? [...configErrors] : [](PR)+ const contentStore = input.contentStore ?? createContentStore();(master)
    • src/persist/state-merge.ts:master 三个 image 字段 + PR lastConfigWarnings: parsed.lastConfigWarnings ?? fresh.lastConfigWarnings
    • src/state.ts fresh-state:imageFullRestored/imageShrinks/lastConfigWarnings 均初始化
    • src/types.ts CompressionState:两侧可选字段全保留(含各自 doc comment)
    • src/sync.ts、tests/validation.test.ts 自动合并干净(master 未动过该测试)
  • diffstat 与原 PR 完全一致(+143/−1),无内容丢失。
  • 门禁(本地真实 Node):typecheck 干净 ✅ / 1093/1093 tests ✅ / build 成功 ✅。
  • 行为语义与 PR 描述一致:warn-once 去重按「错误集变化」触发,不触压缩决策/wire/state schema 契约(state 仅加一个 optional 字段,旧持久化状态缺省安全)。

CI 将在 force-push 后重跑;合并请人工执行(PR merges are human-only)。

一句话总结:#347 已 rebase 到当前 master 并四处并集解冲突,本地三门禁全绿(1093/1093),可以合并。

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

validation: minContextLimitPct > maxContextLimitPct warns once per processTurn (log spam)

1 participant