Skip to content

feat(rules): acp_rule persistent rules — reconciled API (state, limits, hard protection) - #332

Merged
ranxianglei merged 2 commits into
masterfrom
2026-09-20_acp-rule-reconcile
Sep 20, 2026
Merged

ranxianglei merged 2 commits into
masterfrom
2026-09-20_acp-rule-reconcile

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

Closes #282. Supersedes #283 and #284 — they are competing implementations of the same spec (6 shared files, mutual conflict); one must win and this branch reconciles them against the only concrete consumer code that exists today.

Why a third branch

#283 and #284 agree on the core (state field, helpers, ALWAYS_PROTECTED_TOOLS += acp_rule) but diverge on API shape — and neither matches billion-context#750 (the port of the feature origin billion-context-pi#433), whose src/rules-feature.ts imports addRule, listRules, formatRulesList, resolveRuleLimits from the kernel:

#283 #284 this PR (matches #750)
limits source Config.rules (configurable) frozen defaults resolveRuleLimits(config) merging Config.rules over defaults
limit field names maxRules/maxRuleChars maxCount/maxChars maxRules/maxRuleChars
list rendering — (formatRulesForPrompt only) formatRulesForPrompt both formatRulesForPrompt + formatRulesList(rules)
id allocation plain nextRuleId monotonic + never-reuse guard #284's (kept)
state mutation immutable returns in-place mutation returning `{ok:true;rule}
persistence carry not handled state-merge + sync + cloneState #284's (kept)

The in-place mutation choice is deliberate: hosts hold one live CompressionState per session and #750's executeRule calls addRule(state, text, limits) and discards any returned state — an immutable-only helper would silently lose the rule.

Changes

  • src/rules.ts (new): RULE_TOOL_NAME, DEFAULT_RULE_LIMITS ({maxRules: 50, maxRuleChars: 300}, frozen), RULES_USAGE_PROMPT, listRules, allocateRuleId (pure, never-reuse, hand-crafted-state backfill), resolveRuleLimits(config?), addRule/removeRule/clearRules (validating: non-empty → per-rule char cap → identical-content dedup → count cap), formatRulesForPrompt ("" when empty — zero token cost), formatRulesList
  • src/types.ts: RuleRecord {id, text}, CompressionState.rules? + nextRuleId? (optional — pre-rules persisted states load unchanged), Config.rules?: RulesConfig {enabled?, maxRules?, maxRuleChars?}. Documented deviation from acp_rule 持久规则:状态字段、辅助函数与硬保护(跨仓功能内核侧) #282's original "no Config field" stance: the host gates on config.rules.enabled === true and carries limit overrides through the resolved Config.
  • src/config.ts: validateConfig rejects rules.maxRules < 1 / rules.maxRuleChars < 1
  • src/protected.ts: ALWAYS_PROTECTED_TOOLS = ["compress", "acp_rule"] — unconditional; side-effect-free when the adapter never emits acp_rule messages (feature flag defaults off in pi acp.json / bili three-level config)
  • src/state.ts, src/compress.ts (cloneState), src/sync.ts, src/persist/state-merge.ts: rules + nextRuleId flow through every state-copy/merge site with fresh-state fallbacks
  • src/index.ts: barrel exports
  • NOT added to ACP_TOOL_NAMES (kernel-side wire never injects it — opt-in adapter surface, same stance as acp_absorb)

Pre-flight

  • npm run typecheck: PASS
  • npm test: 861/861 (846 existing + 15 new in tests/rules.test.ts: mutation+id sequence, empty/over-length/duplicate/cap rejections, never-reuse after remove/clear, hand-crafted-state backfill, resolveRuleLimits defaults/overrides, renderers, validateConfig, compression-exclusion integration (direct + effective coverage, Bug-39 shape), state-merge fallbacks)
  • npm run build: PASS

Follow-ups (after this merges + releases)

  1. billion-context bumps its kernel pin; #750's src/rules-feature.ts then compiles as-is (imports already match this API).
  2. Close feat(rules): acp_rule persistent rules — state field, helpers, hard protection #283/feat(rules): acp_rule persistent rule records with hard compression protection #284 in favor of this PR (either one's author can port any missing nuance — I found none beyond what is absorbed above).

…s, hard protection)

Reconciles #283/#284 into one implementation matching the concrete
consumer contract (billion-context#750 rules-feature.ts):

- src/rules.ts: RULE_TOOL_NAME, DEFAULT_RULE_LIMITS {maxRules:50,
  maxRuleChars:300}, RULES_USAGE_PROMPT, listRules, allocateRuleId
  (pure, never-reuse, hand-crafted-state guard from #284),
  resolveRuleLimits(config) (Config.rules overrides, from #283),
  addRule/removeRule/clearRules — state-mutating, returning
  {ok:true;rule}|{ok:false;error} (host executeRule discards returned
  state), formatRulesForPrompt (system-prompt section, '' when empty),
  formatRulesList (tool-result rendering)
- types: RuleRecord, CompressionState.rules?/nextRuleId? (optional,
  pre-rules states load unchanged), Config.rules? RulesConfig
  {enabled?,maxRules?,maxRuleChars?} — deviates from #282's original
  'no Config field' stance because the host gates on
  config.rules.enabled and passes the resolved Config through
- config: validateConfig rejects rules.maxRules/maxRuleChars < 1
- protected: ALWAYS_PROTECTED_TOOLS += acp_rule (unconditional,
  side-effect-free when the adapter never emits acp_rule messages)
- state/compress(cloneState)/sync(state carry)/persist(state-merge
  fallbacks): rules + nextRuleId flow through every state copy site
- tests/rules.test.ts: 15 tests incl. compression-exclusion
  integration (direct+effective coverage) and state-merge fallbacks

Closes #282. Supersedes #283 and #284 (competing implementations of
the same spec — one of them must be closed in favor of this one).
@github-actions

github-actions Bot commented Sep 20, 2026

Copy link
Copy Markdown

📦 Built Package Artifact

Branch: 2026-09-20_acp-rule-reconcile (04e97cd)

Option A — Install from npm PR tag (recommended)

In your adapter project:

npm install acp-kernel@pr-332

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

Option B — Download artifact

  1. Download the artifact from the Actions run
  2. Extract the tarball and install:
tar xzf acp-kernel-pr332.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 #332 review — verified, ready for human merge.

This is a completed agent PR (ework-agent-pr marker), so per protocol I pulled the branch, audited the code, and checked CI instead of redoing the work.

Branch / CI

  • Head: 2026-09-20_acp-rule-reconcile @ c81aee7 — single commit on current master (043cc68); GitHub reports mergeable/clean, not draft.
  • CI on c81aee7: pr-validation ✅ · test (22) ✅ · build-artifact ✅ · test (24) ✅
  • Local re-run on the same SHA: typecheck PASS · 861/861 tests (846 existing + 15 new, as claimed) · build PASS.

Code audit (diff vs base: 10 files, +431/−2, every hunk feature-relevant)

  • API matches the billion-context#750 consumer table exactly: addRule / listRules / formatRulesList / resolveRuleLimits all exported from the barrel with matching signatures; field names maxRules / maxRuleChars; both renderers present; in-place mutation returning {ok:true;rule} | {ok:false;error}.
  • Never-reuse id invariant: monotonic nextRuleId + backfill from the highest surviving id; allocateRuleId is pure; covered by tests incl. hand-crafted-state backfill and post-remove/clear sequences.
  • State flow: rules + nextRuleId carried through every construction site — createInitialState, cloneState (src/compress.ts:1568), syncBlocks (src/sync.ts:35), mergeCompressionState (src/persist/state-merge.ts:23); rebuildCompressionState spreads {...state} so it flows implicitly. Grepped all state-construction sites; none missed.
  • Hard protection: ALWAYS_PROTECTED_TOOLS = ["compress", "acp_rule"] unconditional; integration test asserts the acp_rule call+result are excluded from both directMessageIds and effectiveMessageIds (Bug-39 shape). Side-effect-free when the adapter never emits acp_rule messages.
  • Not in ACP_TOOL_NAMES — same documented opt-in stance as absorb (src/compress-tools.ts:558-567).
  • validateConfig rejects rules.maxRules / rules.maxRuleChars < 1 or non-finite.
  • Backward compat: both state fields optional; legacy persisted states load unchanged (JSON round-trip test with the fields deleted → fresh-state fallbacks).
  • Diff hygiene: git diff --check clean, no unrelated or reformatted hunks.

Non-blocking observations (flagging only — CI is green, no changes pushed)

  1. The never-reuse expression Math.max(state.nextRuleId ?? 1, highestRuleNumber(state) + 1) is duplicated in allocateRuleId (src/rules.ts:67) and addRule (src/rules.ts:100). A shared private helper would keep that core invariant single-sourced against future drift; behavior is identical today. Happy to push a small refactor if you want it folded in before merge.
  2. format:check (default prettier, no .prettierrc) flags the two new files plus src/config.ts, but 117 files across the repo already fail it and CI doesn't run it — pre-existing convention, not a regression from this PR.

Related threads: #282 (spec) closes on merge; #283 / #284 remain open — closing them in favor of this PR plus the billion-context pin bump are the listed post-merge follow-ups. Merge itself stays human-only per repo rules: #332

中文摘要:审查了 PR #332——代码已核对(API 与消费者端 #750 的导入完全对齐,状态流转四个构造点全覆盖,硬保护含 Bug-39 有效覆盖测试),CI 全绿且本地复跑 861/861 通过;仅发现一处非阻塞问题(id 分配表达式在 allocateRuleId/addRule 两处重复,建议抽公共函数防漂移),可以合并。

@ranxianglei
ranxianglei merged commit db7a123 into master Sep 20, 2026
4 checks passed
@ranxianglei ranxianglei mentioned this pull request Sep 20, 2026
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.

acp_rule 持久规则:状态字段、辅助函数与硬保护(跨仓功能内核侧)

1 participant