fix(compact): address PR 110 review findings
Honor cancellation and disposal around async pre-step setup before the loop can open a step or call the model. Route compaction summarization through agent/request so router agents can select the model, and remove the stale model argument from agent/pre-step. Document serial events and the approximate convergence bound, regenerate the Cordis catalog, and add regression coverage for router compaction, HMR cleanup, and assembly/pre-step interruption.
This commit is contained in:
@@ -30,7 +30,7 @@ This is not a coupling smell — it is the contract's domain. The "only cordis"
|
||||
|
||||
An earlier draft put the full algorithm (the retention walk, token-summing, text extraction) as concrete methods on the interface, with only `estimateContentTokens()` and `summarize()` abstract. That recouples the contract to one strategy: a backend that wants a different retention policy or a different event-sequencing would have to fight inherited concrete code. Making both core methods abstract puts every *how* decision in the backend, where it belongs, and keeps the interface a pure statement of *what*. The backend remains internally factored — `estimateContentTokens()` and `summarize()` are `protected` hooks a sub-backend can override without reimplementing the walk — but that factoring is the backend's private concern, not the contract's.
|
||||
|
||||
`compactIfNeeded(session, system, model, signal)` takes **required** parameters (not the original all-optional shape). The auto-compaction seam (below) always supplies all four — the assembled system prompt (counted toward the estimate), the model (summarization fallback), and the turn's abort signal — so optionality would only invite a hidden default at the seam. `compactRegion(session, start, end, model, signal?)` keeps an optional signal (a manual caller may omit it).
|
||||
`compactIfNeeded(agent, turn, step, fullSystemPrompt, signal)` takes **required** parameters (not the original all-optional shape). The auto-compaction seam (below) always supplies the agent, lifecycle context, assembled system prompt (counted toward the estimate), and the turn's abort signal, so optionality would only invite a hidden default at the seam. The session being compacted comes from the agent context. `compactRegion(session, start, end, agent, turn, step, signal?)` keeps an optional signal (a manual caller may omit it). Passing lifecycle context rather than a concrete model keeps router agents honest: the backend's summarization request can run through `agent/request`, where model-routing plugins already choose the actual model.
|
||||
|
||||
### Auto-compaction runs on `agent/pre-step`, a dedicated surface-mutation seam
|
||||
|
||||
@@ -40,7 +40,7 @@ The fix is a dedicated loop seam, **`agent/pre-step`** (`@mode serial`), fired b
|
||||
|
||||
```
|
||||
assembly = ctx.systemPrompt.assemble()
|
||||
await ctx.serial('agent/pre-step', agent, turn, step, system, model, signal) ⟵ compaction mutates the surface here
|
||||
await ctx.serial('agent/pre-step', agent, turn, step, system, signal) ⟵ compaction mutates the surface here
|
||||
session('step/start') ⟵ the step opens AFTER the seam
|
||||
messages = session.deriveMessages() ⟵ single derive, reflects the compaction
|
||||
request = waterfall agent/request ⟵ pure request transform (hooks, model switch)
|
||||
@@ -64,9 +64,9 @@ A runaway turn thus compacts exactly like any other history: its early *closed*
|
||||
|
||||
`compactIfNeeded` always anchors the compacted range at the surface **head** (`nodes[0]`). After a first compaction lands a summary node at the head, the *second* compaction's range starts at that summary node and re-summarizes it together with the steps accumulated since — so the surface holds **at most one** auto-generated checkpoint, always at the head, re-consolidated each cycle (the backend's checkpoint-merge prompt makes this a cheap incremental merge — see below). This is *why* `CompactionResult.shadowedRange` is a **surface-position span, not a numeric seq interval**: after a replace lands a fresh high-seq summary node at an older range's position, `start` can be numerically **greater** than `end`. The range is resolved positionally (index into the ordered node list and slice), and `shadowedSeqs` is the authoritative set in surface order. (Manual `compactRegion` may target any aligned mid-range and so *can* leave several checkpoints; the checkpoint framing does not claim everything after it is recent.)
|
||||
|
||||
### Single-pass convergence invariant
|
||||
### Approximate convergence invariant
|
||||
|
||||
`resolveConfig` **rejects** (throws at construction) any config where `summarizationMaxTokens + retainTokens >= contextWindow * thresholdRatio`. The invariant guarantees the post-compaction history — the bounded summary plus the retained recent tail — is structurally below the threshold, so a compaction never immediately triggers another: consecutive re-compaction is impossible by construction, with no thrash throttle needed. The bound is **strict** (`>=` rejects, not `>`): the token-pressure gate declines only when the estimate is `< threshold`, so a post-compaction history sitting *exactly* at the threshold would re-trigger on the very next check — equality is a leak, not a safe boundary. `summarizationMaxTokens` stays an explicit *quality* knob (terse summaries); the invariant only forbids setting it so high it breaks convergence. The sole residual is the single-unit-overflow case above (a backward-rounded oversized step can push the retained tail over budget) — which is exactly the out-of-scope concern, not a thrash bug. Per the pre-release reject-don't-migrate stance, a config that cannot guarantee convergence is a bug at the call site, not something to silently clamp.
|
||||
`resolveConfig` **rejects** (throws at construction) any config where `summarizationMaxTokens + retainTokens >= contextWindow * thresholdRatio`. The invariant bounds the two variable parts of post-compaction history — the bounded summary plus the retained recent tail — but it is intentionally approximate: checkpoint framing, per-message role overhead, system-prompt size, and the char/4 estimator's error can still leave a narrow accepted config near the threshold. The bound is **strict** (`>=` rejects, not `>`): the token-pressure gate declines only when the estimate is `< threshold`, so a post-compaction history sitting *exactly* at the threshold would re-trigger on the very next check — equality is a leak, not a safe boundary. `summarizationMaxTokens` stays an explicit *quality* knob (terse summaries); the invariant only forbids setting it so high it breaks the structural budget. The sole residual is the single-unit-overflow case above (a backward-rounded oversized step can push the retained tail over budget) — which is exactly the out-of-scope concern, not a thrash bug. Per the pre-release reject-don't-migrate stance, a config that cannot satisfy the structural bound is a bug at the call site, not something to silently clamp.
|
||||
|
||||
### Surface replacement: `compact/*` events are log-only; one `user/message` carries the summary
|
||||
|
||||
|
||||
@@ -20,7 +20,7 @@ Pure generation is correct here because the codebase is disciplined enough that
|
||||
|
||||
Specific choices:
|
||||
|
||||
- **`@mode` tag, cross-checked.** Each harness event's JSDoc carries an explicit `@mode emit|waterfall|parallel` tag; the generator hard-errors on a missing tag. Where the signature shape is conclusive — a trailing `next: () => …` parameter is structurally a waterfall — it asserts the tag agrees and hard-errors on a contradiction. The emit-vs-parallel distinction is not structurally visible (`session/flush` returns `Promise<void> | void` with no `next`), so it is trusted from the tag. The authoring rule lives in [AGENTS.md](../../../../AGENTS.md).
|
||||
- **`@mode` tag, cross-checked.** Each harness event's JSDoc carries an explicit `@mode emit|waterfall|parallel|serial` tag; the generator hard-errors on a missing tag. Where the signature shape is conclusive — a trailing `next: () => …` parameter is structurally a waterfall — it asserts the tag agrees and hard-errors on a contradiction. The emit/parallel/serial distinction is not structurally visible (`session/flush` returns `Promise<void> | void` with no `next`, as does the ordered `agent/pre-step` checkpoint), so it is trusted from the tag. The authoring rule lives in [AGENTS.md](../../../../AGENTS.md).
|
||||
- **Tiered scope.** The harness tier (the 8 `@deepseek-ai/dsh-*` services + their events) is rendered in full from source. The inherited tier (cordis-core `ctx.on/emit/effect/provide/…` + the `internal/*` events + loader/hmr/timer) is pinned vendor source a plugin also sees; it is rendered tersely (name + one-line + source pointer) from a curated table in the generator, NOT walked from the vendor AST — the cordis-core `Context` mixes true ctx members with non-service fields (`root`, `baseUrl`, `logger`), and the vendor surface changes only on a deliberate vendor sync.
|
||||
- **Cross-links to the data-structure catalog.** A type name in a signature (`GenerateOptions`, `StreamChunk`, `ToolDefinition`, …) links to the core-data-structures page that documents it. The map is a small hand-curated const in the generator — NOT `type-equiv.manifest.json`, which documents the `…Map` symbols while signatures reference the derived union names, and lists a few symbols on two pages.
|
||||
- **A dedicated fence.** Signature blocks use a ` ```ts cordis-catalog ` info string that `doc-typecheck` recognizes and skips (a bare signature fragment is not standalone-compilable), excluded from the opt-out ratio — the same treatment `type-equiv` blocks get.
|
||||
|
||||
Reference in New Issue
Block a user