A Codex review pass on the draft caught four real gaps and two solid
suggestions; all addressed except one pushed back on the merits:
- hooks-claude/hooks-codex: stderrSummaryMaxChars was the one new knob
with NO range validation — a negative/NaN cap would silently
misbehave inside slice(). Both bridges now assert a positive integer
at the TOP of apply() (before the config-file parse's early return,
so a bad value fails the load loudly), with rejection tests.
- tool-fs: the read caps count lines/chars/bytes, so positive-FINITE
was too loose (a fractional readLimit would flow into windowing
arithmetic and the schema description). All four now require a
positive integer, matching tool-web's cap.
- Doc drift the gates cannot catch: tool-web's README tools table
still named WEB_SEARCH_MAX_RESULTS as the mechanism; compact-basic's
README/module doc and the compaction-capability-seam RFC still
described estimation as fixed char/4 rather than the charsPerToken
default.
- subagent-acp: the dispose graces were tested only at the
startAcpRun level, so a regression that stopped threading plugin
config into AcpRunSpec would have survived. A provider-path test now
drives the trap-escalation scenario through ctx.subagents.start with
small config graces and bounds dispose at 4s.
Pushed back on: converting compact-basic's charsPerToken to a
schemastery field. The package's whole config is deliberately
hand-rolled (resolveConfig, every threshold REQUIRED with no default —
a documented design posture); one schemastery field beside it would be
incoherent. The knob is cordis.yml-reachable, defaulted, and validated,
which is what the convention requires; migrating the package to
schemastery wholesale is pre-existing config-surface hygiene out of
this change's scope.
The audit swept every packages/*/* plugin for the new AGENTS.md
convention (no hardcoded tunables in plugins) and exposes each finding
as a defaulted, validated Config field. Defaults are the previously
hardcoded values throughout, so no deployment or golden changes.
- tool-fs (had NO Config): readLimit, readMaxLineLength, readMaxBytes,
readStreamMinSize. The caps thread through ReadToolCaps/ReadWindow —
read-render already documented that the consumer applies the caps, so
they become explicit per-request fields.
- tool-web: searchMaxResults (WEB_SEARCH_MAX_RESULTS stays as the
schemastery default). Also fixes the stale GREP_LIMIT references in
search.ts and the web-capability-seam RFC (no such constant exists).
- bash-local: graceMs (SIGTERM->SIGKILL escalation grace). The
RunInternals.graceMs test seam is gone: graceMs is now a required
SpawnSpec field filled from config, so tests exercise the real
config path and the defaults live in exactly one place.
- subagent-acp: disposeEofGraceMs / disposeGraceMs. The AcpRunSpec
fields become required for the same one-defaulting-layer reason.
- session-persistence-sqlite: journalMode ('wal' default; the
rollback-journal modes serve filesystems where WAL's shared-memory
files do not work, e.g. network mounts).
- hooks-claude + hooks-codex: stderrSummaryMaxChars for the persisted
hook/result stderr summary. The duplicated summarize() helpers merge
into hook-protocol's summarizeStderr(stderr, maxChars), beside the
HookResultRecord field it feeds, with the bound parameterized the
same way runHook's defaultTimeoutMs already is.
- compact-basic: charsPerToken for the token estimator (default 4, the
English-text heuristic; CJK-heavy deployments need ~1-2 or compaction
fires far too late). Also corrects the BasicCompactService class doc,
which claimed defaults the required-field config never had.
- fs-local: deletes the dead STREAM_MIN_SIZE constant and the dead
FsIoInternals.streamMinSize seam — the read-routing bound lives in
the consumer (tool-fs), where it is now config. This is item 1 of
the proposed prune-write-only-fs-surface RFC, annotated accordingly.
Every new field gets range validation (following the existing
assertPositiveFinite pattern), a README row, and tests covering the
configured behavior, the schema default, and load-time rejection.
ImageBlock had no production producer and every consumer dropped it:
the deepseek serializer skipped it, the pi-ai converter skipped it as
unrepresentable, the ACP bridge neither advertises image prompt
capability nor forwards image blocks, and compact-basic charged a flat
85-token estimate and rendered an [image] placeholder. A block
constructed today would silently vanish from the wire — the vocabulary
advertised a capability no path honors, the silent-data-loss shape the
defensive patterns warn against. The only constructors were tests
pinning the skip/estimate branches.
Remove ImageBlock and its ContentBlockMap entry (its cache?: CacheHint
field leaves with it; CacheHint itself and the other two cache? fields
are out of scope). compact-basic loses its explicit image estimate and
placeholder arms (the merge-extensible default arms absorb the case);
the deepseek serializer, pi-ai converter, and ACP codec already handled
image in their default arms, so only their image-naming comments
change. The codec's inbound rejection of ACP-protocol image prompt
content stays — that guards wire content a client can send regardless
of our vocabulary.
Tests that constructed harness image blocks to pin the removed branches
are dropped (the 85-token estimate pin) or retargeted onto plugin-added
block types / other non-text blocks, which the surviving default arms
own. Docs, the type-equiv pastes, and the content-block vocabulary
RFC's block list and multimodal-home consequence are updated in the
same change; the RFC moves to implemented/ and the index is
regenerated. A real multimodal feature reintroduces image via
declaration merging together with the adapter mapping, ACP
advertisement, and compaction pricing that honor it.
Address @tianyicui's minor-revision review on PR #110:
- Make every BasicCompactConfig knob required except `auto` (defaults
true): there is no data yet to justify default thresholds/budgets, so
a consumer states each value explicitly. Drop the DEFAULTS export and
the constructor's `= {}` default; example cordis.yml, the compaction
e2e, the README, and every test construction site now pass a complete
config (tests route through a `cfg()` helper).
- Add a TODO on estimateContentTokens: char/4 is coarse; replace with a
real tokenizer or post-response usage feedback in a follow-up.
- Add a TODO on the agent/pre-step `fullSystemPrompt` param flagging it
as a smell on a generic per-step seam (compaction is its sole
consumer); a `//` line comment so it stays out of the generated catalog.
Use maxTokens as the provider generation cap and remove the confusing stored-summary max config.
Strip reasoning blocks before storing compaction summaries, reject non-shrinking summaries, and retry bounded re-compaction when the surface remains over threshold.
Add config validation for numeric and type-shaped knobs plus unit and real-API e2e coverage for reasoning-capable summarization.
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.
Codex round 1 CBR-003: several docs still described compaction as an
`agent/request` waterfall concern, and the implemented compaction RFC
claimed "No changes to dsh-session or dsh-invariants" while the diff
changed both.
- Package READMEs / JSDoc (agent, agent-loop, system-prompt, compact,
compact-basic): compaction now lives on the serial `agent/pre-step`
seam (fired after turn/start, before step/start); the structural guard
is tool-pairing balance (`isToolPairingBalanced`), not step-alignment;
the convergence bound is strict (`>=` rejects).
- architecture.md / core-data-structures/compaction.md: same seam +
predicate + dispatch-mode updates; regenerated cordis catalog.
- Implemented compaction RFC, updated in place to describe shipped
reality: the seam is `agent/pre-step` (@mode serial) fired before
step/start; alignment is surface tool-pairing balance; the convergence
invariant rejects `>=`; and the "no dsh-session/dsh-invariants changes"
claim is corrected — dsh-session gains the tool-pairing predicate and
dsh-invariants drops its `start <= end` replace assertion (a positional
replace makes start > end normal).
Reform the compaction blueprint so a runaway turn survives and the design
stops drifting across review rounds:
- Drop in-flight-turn protection ("layer 2"). Retention is a uniform tail→head
whole-unit walk; the only structural guard is step-alignment. A single turn
that alone exceeds the window now compacts its own early closed steps instead
of being retained verbatim (the failure mode that motivated this).
- Move auto-compaction off the agent/request waterfall onto a new awaited
agent/pre-request loop seam, fired before history derivation. Compaction
mutates the surface; the loop derives once from the result — no double-derive,
and a listener structurally cannot act on not-yet-derived messages.
- Tighten compactIfNeeded to required (session, system, model, signal).
- Enforce a single-pass convergence invariant in resolveConfig: reject configs
where summarizationMaxTokens + retainTokens exceeds the threshold, so a
compaction can never immediately re-trigger.
- Document the crash vs recoverable failure taxonomy; core session repair stays
compaction-agnostic (a log-only orphaned compact/start is inert).
- Wire dsh-compact-basic into examples/coding-agent and add a with-key
compaction e2e (compaction's first real-world exercise + runaway net).
- Rewrite the RFC to encode the blueprint and move it to implemented/.
The runaway-turn snapshot is a named deferred follow-up: dsh-llm-replay cannot
yet serve the interleaved summarization model call.
Collapses the per-round review churn of the prior compact-basic branch into a
single clean baseline on top of compact-interface, so the upcoming retention
refactor lands as fresh, well-scoped commits rather than stacking on a history
of fixes that are being superseded.
Tianyi Cui
Hypatia May