refactor(agent): read the charter and the window's shrinking through the roles

[ypflll]

Summary

The agent loop already names four strategy roles (Memory, Planning, Capability, Action), but two judgements a turn makes still lived in the shell around them. This moves both onto the roles, in two dependent commits, with no behaviour change intended.

1. The charter reads through Memory and Action. A dispatched sub-agent's playbook (_meta["raven.playbook"]) was read from four places inside the loop and the tool registry. prompt and stopWhen are now read by DefaultMemory.assemble(), which fills task_brief and task_done_when on the TurnContext the identity segment renders. checks and code are read by DefaultAction.judge(), which ToolRegistry.execute asks once per proposed call, the way it already asks the permission gate. tools stays in withheld_names() and the registry still decides what a refusal does, so a replaced role withholds nothing it could not already withhold.

2. Mid-turn window shrinking seats on Memory. The five recoveries the loop ran inline go through one method, MemoryModule.shrink(messages, pressure, state, model), with WindowPressure, WindowState and ShrinkResult as the carriers:

• proactive compaction, once the last billed reading crosses the trigger
• the standing image window
• a provider's overflow error
• a picture refused inside a tool result
• pictures refused for size

The retry mechanism stays in the shell. The six continue sites and the iteration rollback did not move, so a hook sees a retried call exactly as it saw the first. WindowState is a per-turn object the shell owns and hands to each call, because the role outlives the turn; image_window has no default, since 0 is a real value meaning "no picture stays".

The pure pieces both sides need move to a new package, raven/agent/window/, which the import contracts treat as cargo that may not import the loop shell. REASONING_EFFORT_LADDER moves to contracts/llm_provider.py because the loop's empty-response descent and the window's head summary both read it and neither package may import the other.

Contract papers: CONTRACTS_VERSION 26 to 28, CONTRACTS_LINE_CEILING 3,040 to 3,200, each bump with its own review paragraph in tests/test_kernel_budget.py.

A follow-up PR carries the third step, which gives a sub-agent nine verbs in place of the six hook phases. It is kept separate because it touches every bundled plugin.

Type

• Fix
• Feature
• Docs
• CI / tooling
• Refactor
• Other

Verification

Gates, run at each of the two commits:

uv run ruff check --no-cache raven evolver agents plugins-dist tests scripts
uv run ruff format --check --no-cache raven evolver agents plugins-dist tests scripts
uv run lint-imports
uv run pytest -q
npx commitlint --from origin/main --to HEAD --config commitlint.config.cjs
uv run python scripts/check_commit_messages.py origin/main..HEAD
uv run python scripts/check_source_language.py origin/main..HEAD
uv run python scripts/check_large_files.py origin/main..HEAD

Import contracts: 10 kept, 0 broken at both commits. The suite is green at both; the failures this machine shows are the same ones it shows on an unmodified main (missing browser and optional packages), and neither commit adds one. The loop's own e2e files, which the default selection leaves out, were run too (charter, playbook, truncation, direct chat, fallback chain, ACP stdio): no failure that main does not also show.

Behaviour preservation. Each commit was run against a worktree of its own parent under the same scripted provider, outputs normalised for nonces and temp paths, then byte-compared. Four scenarios, eight comparisons, all identical:

• a whole turn with no playbook: host turn, spawned sub-agent, tool calls and refusals
• a playbook-on worker lane in process, including two charter refusals
• six window-shrink scenarios: proactive compaction, the standing image window, overflow, a refused picture, oversized pictures, the effort ladder
• the five-plugin hook seam, every phase's decision and metadata per iteration

Mutation check on the second commit: breaking each of the five moves inside DefaultMemory.shrink turns exactly its own scenario red, so the comparison is not vacuous.

Real model, Sonnet 4.5 through OpenRouter, at both commits:

• playbook on, in-process lane: both workers receive the charter, write their files and finish

• playbook on, ACP fork lane: the charter prompt crosses the process boundary, and a charter check blocks a write_file outside the fence

• Relevant tests pass locally

• Relevant lint / type checks pass locally

• User-facing docs or screenshots are updated when needed

Risk

No user-visible behaviour change is intended, and the comparisons above are byte-identical on every path they cover.

For anyone replacing a harness role: MemoryModule gains shrink, so a replacement written against the old protocol needs it. DefaultMemory.bind() names the missing member at assembly rather than failing inside a turn.

Rollback is a revert of the merge commit. Nothing persisted on disk changes shape.

• Security impact considered
• Backward compatibility considered
• Rollback path is clear for risky changes

Related Issues

N/A

[gloryfromca]

No blockers; this can merge as far as I am concerned.

Reviewed the current head after its merge from main. I covered the repository rules and canonical runtime vocabulary, the full diff, the affected charter/window callers, both feature commits and surrounding history, compatibility of the moved internal helpers and expanded harness contracts, test changes for weakening, and the cargo-versus-loop architecture boundary. The refactor preserves the prior retry/state transitions while moving policy behind Memory, and the charter prompt/check paths remain joined end to end.

Verification: the focused changed-area run passed 247 tests; the PPT ledger passed 13 tests under the all-extras environment; uv run lint-imports kept all 10 contracts. The full all-extras run reported 23,127 passed and 120 skipped, with one unrelated test_cli_theme failure caused by this runner's NO_COLOR=1; that exact test passed when rerun with NO_COLOR unset. GitHub's four Python test shards are also green on this head.

[LivXue]

Review: verified against f69e6cf7f vs merge-base 3b35e7d34

This is a high-quality refactor. turn_path.py loses 537 lines, the contract paper
is written with more care than most changes of this size get, and the verification
method in the description (per-commit byte-compare under a scripted provider, plus a
mutation check to prove the comparison is not vacuous) is the right one.

Two findings are, in my reading, blocking. Both are small to fix.

B1. The charter's checks/code enforcement went from unconditional to opt-in,
and one registry that dispatches inside a live charter scope did not get the opt-in.
This contradicts "no behaviour change intended", and it moves a security boundary in
the permissive direction. Detail inline at registry.py:407.

B2. Four of the five tests this PR adds live in a directory the default run excludes,
so the guard it wrote for its own new failure mode never executes. Detail inline at
test_subagent_charter_e2e.py:215.

Four more I would like resolved in the same change, each one or two lines:
contracts/context.py:80 (positional-slot shift on a contract-tier dataclass),
harness/__init__.py:52 (no bind_action beside bind_memory),
contracts/harness.py:13 (the paper contradicts itself), and CONTEXT.md:93
(AGENTS.md section 6: three new contract-tier terms and a new ruled package are
undefined, and the charter is renamed to "playbook", which CONTEXT.md already
defines as a different feature).

Nine further comments are non-blocking.

On the nine: five of them are pre-existing code this PR relocates verbatim
(images.py:77, images.py:90, memory.py:217, memory.py:262, shrink.py:237).
I verified them byte-identical to the base after normalising the _name -> name
de-privatisation. They are not defects introduced here; they show up as + lines
to anyone scanning added hunks, and creating the file is the one moment the fix is
free. Fixing them is your call, not a condition of merge.

One side note outside this diff: promoting REASONING_EFFORT_LADDER to contracts
did not reach rpc/methods/console.py:843, which still validates
agents.defaults.reasoningEffort against four of the ladder's six rungs, so the
console rejects xhigh/max while config files and the Anthropic transport accept
them. Worth a separate PR.

Nothing here argues against the shape of the refactor. The policy/mechanism split is
the right seam, and the shell keeping the six continue sites and the iteration
rollback is what makes it safe.

[gloryfromca]

Blocking: charter checks are bypassed by the Curator registry, and the new charter-path guards are outside default CI collection.

The new review evidence changes my prior stance. I verified that a bare ToolRegistry() under an active charter whose rule forbids the call still executed it and returned RAN:blocked; the Curator constructs that same bare registry and dispatches through it inside the turn's charter scope. I also verified that default pytest collection finds zero tests from test_subagent_charter_e2e.py, while the explicitly named file collects 30. tests/test_harness_modules.py remains green (25 passed), but does not cover the missing charter join.

The existing blocking threads already state the required fixes, so I am not duplicating inline findings or replying under another reviewer's name. My earlier clean stance is withdrawn until those open blockers are addressed.

[gloryfromca]

Blocking: the merge-only revision leaves the Curator charter-enforcement regression and excluded charter-path tests unchanged.

I reviewed the delta from f69e6cf7f0f1: this head only merges three unrelated main commits, so the PR's feature files and affected callers are unchanged. I also checked the full thread record; the blocking threads remain open with no replies, resolution, or maintainer deferral.

Reverified on this head: a charter-forbidden call through a bare ToolRegistry() still returned RAN:blocked; default pytest collection still found zero test_subagent_charter_e2e tests; and uv run --all-extras pytest tests/test_harness_modules.py -q passed 25 tests. I covered the revision delta and history, the relevant caller path, default test collection, the standing review record, and the repository's test/domain constraints. No new inline findings.

[gloryfromca]

Blocking: the unchanged charter-enforcement and default test-discovery blockers still have to be resolved.

This head only merges current main; compared with bb64cb073980, it changes the session manager and its tests but leaves the PR's own patch unchanged. I found no new defect in that merge delta, but the existing blockers still reproduce on the merged tree: a bare registry executes the charter-forbidden probe, and default collection still finds zero entries from test_subagent_charter_e2e.py.

Covered: the project rules (AGENTS.md/CLAUDE.md, CONTEXT-MAP.md, and CONTEXT.md), the full PR diff and merge delta, relevant callers and history, compatibility/API concerns, test placement and whether coverage was weakened, and the harness/role architecture. No new inline findings.

Verification: git diff --check github/main...HEAD passed; uv run pytest tests/test_harness_modules.py -q (25 passed); explicit charter integration suite (30 passed); merged session suite (37 passed); affected loop/contracts/window unit selection (192 passed). The default-collection diagnostic returned 0 charter integration entries.

[gloryfromca]

Blocking: move the Memory-to-engine charter join test into default collection and finish the required Charter terminology and cargo-documentation fixes.

This revision fixes the bare-registry enforcement bypass, positional compatibility, Action-role admission, recovery ownership prose, and several non-blocking observations. I found no new defect in the fix commit.

Two established issues remain. Default collection still finds zero entries from test_subagent_charter_e2e.py; the new default-suite tests cover _briefed and identity rendering separately, but not the DefaultMemory.assemble -> _briefed -> engine join. Removing self._briefed(turn) therefore still leaves the default tests green. Also, CONTEXT.md now defines the window terms and adds window to the cargo roster, but the roster still omits harness, while changed runtime prose continues to use playbook for the separately defined Charter concept.

Covered: AGENTS.md/CLAUDE.md and the Runtime context documents, the full final-comparison diff and this revision's delta, affected callers and history, positional/API compatibility, test-strength changes, and harness/import architecture. Patch-risk result: revise (impact high, likelihood high, protection partial, recovery easy, confidence high).

Verification at this head: focused default-suite selection, 130 passed; explicit charter integration file, 30 passed; default collection diagnostic, 0 charter-integration entries; import-linter, 10 contracts kept; git diff --check, passed.

[ypflll]

Thanks -- this found two real holes. Fourteen of the fifteen are addressed in ec24a4e; the three pre-existing ones I propose to send separately, reasoning at the end.

Both blocking items, fixed.

The registry early return was a genuine regression, and the curator path is what makes it bite. _verifier_refusals no longer returns early: with no provider injected it builds the default Action role, which is what the module-level helper it replaced already was. Two regressions cover it in tests/test_subagent_charter.py -- a bare ToolRegistry() inside a charter scope refuses what the rules refuse, and an injected role is the only thing asked when one is installed.

You are right that the tests/integration additions never ran. The two that need no external resource moved to tests/test_segments.py, which is in the default selection; the registry fallback is covered where the other charter tests live.

The four "fix before merge", fixed.

AssemblyContext's task_brief / task_done_when moved to the end of the dataclass, beside where the same pair already sits on TurnContext, with a comment saying why the position is load-bearing. The contract digest is re-pinned.

DefaultAction is admitted through a bind() that names a missing member, exactly as the Memory role is, and default_harness_modules goes through it. A test asserts a decide-only role is refused at assembly.

contracts/harness.py now draws the line where the code does: Memory answers what to give up, the shell owns the mechanism and the retry bounds, and a role answering shrink with an unchanged transcript is named as the failure that would cause.

CONTEXT.md defines WindowPressure, WindowState and ShrinkResult beside the Harness Modules entry, and the cargo-contract roster names window.

Six of the nine non-blocking, fixed.

Dispatch compares with == rather than is, with a comment naming the str-Enum reason. The withdrawal note reads image_budget is not None, matching the guard above it. window_images clamps a negative keep. The main.py comment no longer claims a hot apply that does not exist.

On the curator: I kept the two lines and rewrote the comment, which now says the rebuild is partial on purpose and that the brief reaches the identity through AssemblyContext, not through here. Dropping them would make the turn the curator hands on differ from the one the rest of the assembly saw, which is a worse thing for the next reader to discover than two fields nobody reads yet.

The misnamed test is split: one asserts what the standing pass does, and a new one drives IMAGES_TOO_LARGE and TOOL_IMAGES_REFUSED and asserts the counters and the caps -- deleting any increment now turns it red.

Three I propose to leave.

The filed-note regex truncation, the base64 copy in wire_image_bytes, and the double tokenisation in _compact_ahead are all byte-identical to base, as you note. This PR's claim is that the move changes nothing, and a byte-level A/B against the parent is what backs that claim; a behaviour fix inside the moved code would make "the move changed nothing" no longer checkable in one comparison. I would rather send them as their own PR against main, where each gets the test it deserves -- the regex one in particular is data loss in the session record and should not ride along as a footnote. Say the word if you would rather have them here.

Verification after the fixes: full suite 22898 passed, with no failure main does not also show; ruff, format and import contracts clean at both commits; the four scripted A/Bs against the pre-fix branch are byte-identical, so none of the above changed behaviour on a covered path.

[gloryfromca]

Blocking: the default suite still needs to exercise the Memory-to-engine charter join, and the remaining Section 6 documentation mismatches need correction.

Thanks for the detailed accounting. I agree that the bare-registry bypass and the other implementation/API issues named here are fixed, and I accept the filed-note truncation, base64-copy cost, and double-tokenisation as explicitly deferred non-blocking follow-ups.

Two parts do not hold yet:

1. test_the_brief_only_reaches_the_identity_through_memory is still only in the excluded integration file. The new default tests exercise DefaultMemory._briefed directly and identity rendering separately; neither calls DefaultMemory.assemble. Removing self._briefed(turn) from assemble therefore still leaves the default tests green. Direct invocation of the excluded join test passes, but default collection reports 0 entries for it.

2. The documentation fix addresses the new window terms and adds window, but the cited cargo roster still omits harness, and changed prose still uses playbook to mean the separately defined Charter concept. Those were two explicit parts of the Section 6 finding, not the three pre-existing items deferred at the end of your response.

Verification: uv run --all-extras pytest tests/test_segments.py tests/test_subagent_charter.py -q passed 94 tests; the named excluded join test passed; default collection found 0 copies of it. The initial focused run without extras had 93 passes and one environment failure (raven_everos was not installed), then the same command with the project extras passed.

[gloryfromca]

No blockers; suggestions only, and they are marked inline.

This revision closes the remaining concerns from my side. The default suite now exercises the real DefaultMemory.assemble -> _briefed -> engine join, the cargo roster includes harness, and the affected prose consistently uses Charter rather than the distinct Playbook term. I found no new defect in the delta.

The three explicitly deferred, pre-existing follow-ups remain named here: filed-note caption truncation, the base64 length copy, and duplicate transcript tokenisation in _compact_ahead. They do not block this change. LivXue owns the existing review threads, so I have not resolved or regraded them.

Covered: repository rules and Runtime context documents, the full final-comparison diff and new delta, affected callers/history, compatibility, test strength, and harness/import architecture. Patch-risk result: merge / human_review_required (impact high, likelihood low, protection strong, recovery easy, confidence high).

Verification at this head: focused default-suite selection, 131 passed; the new join regression is collected exactly once by default; import-linter, 10 contracts kept; git diff --check, passed.

[gloryfromca]

No blockers; this can merge as far as I am concerned.

This head only merges the latest main; the feature patch is unchanged, there is no manual conflict-resolution delta, and the newly combined trajectory/replay paths remain compatible with the registry changes. All fifteen review threads are now resolved by their author. The already named deferred, pre-existing follow-ups remain outside this change and are not merge conditions.

Covered: repository rules and Runtime context documents, the final-comparison diff and merge delta, affected callers and history, compatibility, test strength, and harness/import architecture. No new findings. Patch-risk result: merge / human_review_required (impact high, likelihood low, protection strong, recovery easy, confidence high).

Verification at this head: focused harness/contract selection, 131 passed; trajectory/replay/regression selection, 150 passed; import-linter, 10 contracts kept; git diff --check, passed.

[gloryfromca]

No blockers; this can merge as far as I am concerned.

This revision only merges the latest main commit. Its incoming changes are confined to the two READMEs and tests/test_cli_onboard_commands.py, with no overlap in the feature-modified files, so the reviewed feature patch is unchanged from a0db8b641eb2.

I rechecked the repository rules and terminology documentation, the final diff and its caller/history context, compatibility and test integrity, and the architecture boundaries. Verification: git diff --check github/main...HEAD; uv run --all-extras pytest tests/test_cli_onboard_commands.py tests/test_harness_modules.py tests/test_subagent_charter.py tests/test_segments.py tests/test_contracts_two_tier_ledger.py -q (463 passed); uv run --frozen --python 3.12 lint-imports (10 contracts kept). No new findings.