feat(compaction): configurable keep-recent/truncation knobs on the compaction agent#857
Conversation
…tter Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ation Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…on agent Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThree optional compaction tuning fields ( ChangesPer-agent compaction knobs end-to-end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/harnx-core/src/agent_config.rs (1)
601-619:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRefactor
AgentFrontMatter::is_emptyto clear the current quality gate failure.Line 601 now carries an even longer boolean chain, and the added checks at Lines 616-618 increase method complexity in the same hotspot already failing CodeScene for this file. Please extract grouped predicates (for example, compaction-related emptiness) into helper methods to bring this method below the gate threshold.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/harnx-core/src/agent_config.rs` around lines 601 - 619, The is_empty method in AgentFrontMatter has become too complex with a very long boolean chain, especially after adding the compaction-related checks at lines 616-618, causing a CodeScene quality gate failure. Extract logically grouped predicates into separate private helper methods to reduce the method's cyclomatic complexity. Specifically, create a helper method for compaction-related emptiness checks (compaction_agent, compaction_keep_recent_turns, compaction_keep_recent_tokens, and compaction_tool_output_max_chars), and consider extracting other logical groupings such as model-related checks or conversation-related checks into their own helper methods. Replace the corresponding conditions in is_empty with calls to these new helper methods to bring the method below the complexity threshold.Source: Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/harnx-runtime/src/config/tests.rs`:
- Around line 617-714: Extract helper functions from the
test_compact_session_honors_compaction_keep_recent_turns test to reduce its size
and assertion density. Create a setup helper that encapsulates the creation of
the temporary directory, agent files, main agent configuration, session with
four turns, and mock client setup (everything from the start through
TestStateGuard creation). Create separate assertion helpers for each major
assertion block: one helper to verify the conversation history contains the
expected compacted transcript with the first three user turns but not the
fourth, and another helper to verify the session messages contain the summary
plus the two kept verbatim messages. Replace the inline setup and assertion code
in the test with calls to these helpers so the main test function becomes
concise and focused on orchestrating the test flow.
- Around line 622-629: The test function uses blocking std::fs operations which
violates the project guideline requiring Tokio async I/O in async tests. Replace
std::fs::create_dir_all with tokio::fs::create_dir_all and std::fs::File::create
with tokio::fs::File::create, then use tokio::io::AsyncWriteExt to perform the
async write operation. Ensure all these filesystem operations are awaited since
the test is async.
---
Outside diff comments:
In `@crates/harnx-core/src/agent_config.rs`:
- Around line 601-619: The is_empty method in AgentFrontMatter has become too
complex with a very long boolean chain, especially after adding the
compaction-related checks at lines 616-618, causing a CodeScene quality gate
failure. Extract logically grouped predicates into separate private helper
methods to reduce the method's cyclomatic complexity. Specifically, create a
helper method for compaction-related emptiness checks (compaction_agent,
compaction_keep_recent_turns, compaction_keep_recent_tokens, and
compaction_tool_output_max_chars), and consider extracting other logical
groupings such as model-related checks or conversation-related checks into their
own helper methods. Replace the corresponding conditions in is_empty with calls
to these new helper methods to bring the method below the complexity threshold.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db9ff911-965b-44ef-b37a-cb7704f6a19e
📒 Files selected for processing (4)
crates/harnx-core/src/agent_config.rscrates/harnx-runtime/src/config/compaction.rscrates/harnx-runtime/src/config/session_ops_split.rscrates/harnx-runtime/src/config/tests.rs
…pers Address CodeScene complexity on AgentFrontMatter::is_empty by grouping the boolean chain into model/content/compaction helpers, and extract the keep-recent-turns compaction test into a setup fixture plus two assertion helpers to lower its size and assertion density. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ion density The earlier helper extraction added net lines and concentrated assertions into a single helper, which CodeScene flags as a new large assertion block and worsens this file's lines-of-code. Inline the test instead and spread its assertions (max two consecutive), keeping the same end-to-end coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Move the compact_session tests and their helper out of the oversized config/tests.rs into config/compaction_tests.rs, and share EnvGuard via a new config/test_support.rs. Reduces tests.rs lines-of-code, responsibilities, and assertion-block count to improve CodeScene health; behavior unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Pull the temp-dir/agent-file/config/session/mock setup into shared helpers so each compact_session test drops below CodeScene's 70-line Large Method threshold, and the tests share helpers (raising the module's cohesion). Behavior and assertions unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/harnx-runtime/src/config/compaction_tests.rs (1)
15-21: 💤 Low valueSync I/O in test helper may violate the async-everywhere guideline.
The
write_agenthelper usesstd::fs::create_dir_allandstd::fs::File::create(synchronous I/O), but the coding guidelines state "All I/O must be async via Tokio using async fn and .await." While test utilities often use sync I/O for simplicity, verify whether the project's strict async policy applies to test code.♻️ Optional refactor to use async I/O
-fn write_agent(dir: &std::path::Path, name: &str, content: &str) { - std::fs::create_dir_all(dir).unwrap(); - std::fs::File::create(dir.join(format!("{name}.md"))) - .unwrap() - .write_all(content.as_bytes()) - .unwrap(); +async fn write_agent(dir: &std::path::Path, name: &str, content: &str) { + tokio::fs::create_dir_all(dir).await.unwrap(); + tokio::fs::write(dir.join(format!("{name}.md")), content.as_bytes()) + .await + .unwrap(); }Then call with
.awaitfrom test functions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/harnx-runtime/src/config/compaction_tests.rs` around lines 15 - 21, The write_agent helper function uses synchronous I/O operations (std::fs::create_dir_all and std::fs::File::create) which conflicts with the project's async-everywhere guideline requiring all I/O to be performed via Tokio with async fn and await. Convert the write_agent function to an async function, replace the synchronous file system calls with their Tokio async equivalents (tokio::fs::create_dir_all and tokio::fs::File), update the write_all call to use Tokio's async write method, and ensure all call sites of write_agent are updated to use await when invoking the function.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/harnx-runtime/src/config/test_support.rs`:
- Around line 12-27: The test functions
`info_theme_reports_custom_theme_path_and_name` and
`info_theme_reports_custom_theme_without_name` invoke `EnvGuard::new()` without
acquiring the required global test lock, violating the safety contract
documented in the `EnvGuard` implementation. Fix this by acquiring the lock
(using `env_lock()` or similar synchronization mechanism) before instantiating
`EnvGuard` in both test functions, storing the lock guard to keep it alive for
the duration of the test. Alternatively, refactor `EnvGuard::new()` to accept a
reference to a lock guard (such as `&TestStateGuard`) as a parameter, making the
lock requirement compile-time enforced.
---
Nitpick comments:
In `@crates/harnx-runtime/src/config/compaction_tests.rs`:
- Around line 15-21: The write_agent helper function uses synchronous I/O
operations (std::fs::create_dir_all and std::fs::File::create) which conflicts
with the project's async-everywhere guideline requiring all I/O to be performed
via Tokio with async fn and await. Convert the write_agent function to an async
function, replace the synchronous file system calls with their Tokio async
equivalents (tokio::fs::create_dir_all and tokio::fs::File), update the
write_all call to use Tokio's async write method, and ensure all call sites of
write_agent are updated to use await when invoking the function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f8ebc1c-37a8-419a-b734-120bed892792
📒 Files selected for processing (5)
crates/harnx-core/src/agent_config.rscrates/harnx-runtime/src/config/compaction_tests.rscrates/harnx-runtime/src/config/mod.rscrates/harnx-runtime/src/config/test_support.rscrates/harnx-runtime/src/config/tests.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/harnx-core/src/agent_config.rs
- crates/harnx-runtime/src/config/tests.rs
| pub(super) fn new(key: &'static str, value: &std::path::Path) -> Self { | ||
| let prev = std::env::var_os(key); | ||
| // SAFETY: test-only; concurrent env mutation is prevented by the | ||
| // global test lock held by the caller while the guard is alive. | ||
| unsafe { std::env::set_var(key, value) }; | ||
| Self { key, prev } | ||
| } | ||
| } | ||
| impl Drop for EnvGuard { | ||
| fn drop(&mut self) { | ||
| match &self.prev { | ||
| Some(v) => unsafe { std::env::set_var(self.key, v) }, | ||
| None => unsafe { std::env::remove_var(self.key) }, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify EnvGuard is only instantiated while TestStateGuard is held.
# Search for EnvGuard::new call sites
echo "=== EnvGuard::new call sites ==="
rg -n -A5 -B5 'EnvGuard::new' --type rust
# Search for patterns where EnvGuard might be used without guard
echo "=== Checking for TestStateGuard in same scope ==="
rg -n -B10 'EnvGuard::new' --type rust | rg -C3 'TestStateGuard'Repository: dobesv/harnx
Length of output: 50368
🏁 Script executed:
# Check EnvGuard implementation in test_support.rs
head -50 crates/harnx-runtime/src/config/test_support.rsRepository: dobesv/harnx
Length of output: 1020
🏁 Script executed:
# Check the tokio::test call sites in commands.rs more carefully
sed -n '1330,1410p' crates/harnx-runtime/src/commands.rsRepository: dobesv/harnx
Length of output: 2534
🏁 Script executed:
# Check if there's a TestStateGuard and how it's defined
rg -n 'struct TestStateGuard' --type rustRepository: dobesv/harnx
Length of output: 130
🏁 Script executed:
# Check if tests in commands.rs are somehow isolated/serialized
rg -n -B2 'info_theme_reports_custom_theme' crates/harnx-runtime/src/commands.rs | head -30Repository: dobesv/harnx
Length of output: 340
🏁 Script executed:
# Check what env_lock is and how it works
rg -n 'fn env_lock' --type rust -A5Repository: dobesv/harnx
Length of output: 2564
🏁 Script executed:
# Check TestStateGuard implementation
sed -n '44,100p' crates/harnx-runtime/src/client/mod.rsRepository: dobesv/harnx
Length of output: 1698
🏁 Script executed:
# Check if there's a test harness or something that enforces serial execution
rg -n 'serial|SERIAL|Mutex|test_lock' crates/harnx-runtime/src/commands.rsRepository: dobesv/harnx
Length of output: 919
🏁 Script executed:
# Check if any other async test in the codebase uses EnvGuard without a lock
rg -n 'async fn.*test' crates/harnx-runtime/src/commands.rs | head -20Repository: dobesv/harnx
Length of output: 177
🏁 Script executed:
# Let's check if there's a global lock mechanism in the test module
rg -n 'lazy_static|once_cell|static.*LOCK|static.*MUTEX' crates/harnx-runtime/src/commands.rsRepository: dobesv/harnx
Length of output: 38
🏁 Script executed:
# Double-check: are env_lock() calls in commands.rs at all?
rg -n 'env_lock' crates/harnx-runtime/src/commands.rsRepository: dobesv/harnx
Length of output: 38
🏁 Script executed:
# Let's see the full context around the two problem test functions
sed -n '1332,1345p' crates/harnx-runtime/src/commands.rs
sed -n '1402,1415p' crates/harnx-runtime/src/commands.rsRepository: dobesv/harnx
Length of output: 1247
🏁 Script executed:
# Check if there's any ENV_MUTEX in commands.rs module
rg -n 'ENV_MUTEX|LOCK' crates/harnx-runtime/src/commands.rs | head -20Repository: dobesv/harnx
Length of output: 38
🏁 Script executed:
# Verify: how many test functions use EnvGuard in commands.rs without visible lock?
rg -B5 'EnvGuard::new.*HARNX_CONFIG_DIR' crates/harnx-runtime/src/commands.rs | rg -E '(async fn|let _lock|let _guard|env_lock)'Repository: dobesv/harnx
Length of output: 453
🏁 Script executed:
# Check if commands.rs has any module-level test setup or teardown
grep -n '#\[cfg(test)\]' crates/harnx-runtime/src/commands.rsRepository: dobesv/harnx
Length of output: 74
🏁 Script executed:
# Look for all #[tokio::test] tests in commands.rs and check if any hold locks
rg -n '#\[tokio::test\]' crates/harnx-runtime/src/commands.rs -A10 | grep -E '(tokio::test|let _|env_lock|LOCK|MUTEX|Guard)' | head -40Repository: dobesv/harnx
Length of output: 344
🏁 Script executed:
# Count EnvGuard calls in commands.rs vs calls with locks visible
echo "=== EnvGuard calls in commands.rs ==="
rg -c 'EnvGuard::new' crates/harnx-runtime/src/commands.rs || echo "0"
echo ""
echo "=== Context around these calls ==="
rg -B10 'EnvGuard::new' crates/harnx-runtime/src/commands.rs | grep -E '(fn |let _lock|env_lock|LOCK)'Repository: dobesv/harnx
Length of output: 257
🏁 Script executed:
# Check if tokio::test provides implicit serialization or isolation
rg -n 'tokio::test' crates/harnx-runtime/src/commands.rs | wc -lRepository: dobesv/harnx
Length of output: 58
Two EnvGuard instantiations in commands.rs omit the required lock, creating potential data races.
The tests info_theme_reports_custom_theme_path_and_name and info_theme_reports_custom_theme_without_name (lines 1335, 1405) call EnvGuard::new() without holding the global test lock. The EnvGuard documentation explicitly requires "callers must hold the global test lock while it is alive," but these #[tokio::test] functions have no lock acquisition visible in their scope.
Most other call sites (in harnx-runtime/tests/package_loading.rs, harnx-sandbox-common/, etc.) correctly hold env_lock(), ENV_MUTEX, or TestStateGuard before instantiating EnvGuard. The pattern works in practice because test harnesses often serialize, but it remains fragile without type-system enforcement.
Acquire the lock before EnvGuard::new():
let _lock = env_lock(); // or similar synchronization
let _env = EnvGuard::new("HARNX_CONFIG_DIR", temp.path());Alternatively, refactor EnvGuard::new to require a &TestStateGuard parameter, making the lock requirement compile-time enforced.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/harnx-runtime/src/config/test_support.rs` around lines 12 - 27, The
test functions `info_theme_reports_custom_theme_path_and_name` and
`info_theme_reports_custom_theme_without_name` invoke `EnvGuard::new()` without
acquiring the required global test lock, violating the safety contract
documented in the `EnvGuard` implementation. Fix this by acquiring the lock
(using `env_lock()` or similar synchronization mechanism) before instantiating
`EnvGuard` in both test functions, storing the lock guard to keep it alive for
the duration of the test. Alternatively, refactor `EnvGuard::new()` to accept a
reference to a lock guard (such as `&TestStateGuard`) as a parameter, making the
lock requirement compile-time enforced.
1 participant
Summary
Makes compaction's three tuning values configurable via the compaction agent's frontmatter only — no global setting, no calling-agent setting, no per-session override. The role is unambiguous: these fields only take effect when the agent is used as a compaction agent.
The built-in constants (renamed
DEFAULT_*) remain the per-field fallback, so when nocompaction_agentis configured — or one is configured but leaves a knob unset — behavior is unchanged.New
AgentConfigfrontmatter fields (allOption<usize>)compaction_keep_recent_turnscompaction_keep_recent_tokenscompaction_tool_output_max_charsExample compaction agent:
How it works
compaction_agentplumbing (struct field,from_markdown,AgentFrontMatter,from_config,is_empty, getter).compaction.rsgains aCompactionParamsstruct and acompaction_params(&AgentConfig)resolver (per-fieldgetter().unwrap_or(DEFAULT_*)).compact_sessionis reordered so the summarizer agent is built before the read-lock snapshot block; its resolved params feedsplit_index(turns/tokens) andrender_transcript(tool-output cap).compaction_agent→ syntheticAgentConfig::from_prompt(DEFAULT_COMPACT_SYSTEM_PROMPT)→ all knobsNone→ defaults → existing behavior preserved.Testing
agent_config.rs: frontmatter parse + absent→None+ serialize/reparse round-trip.compaction.rs:compaction_paramsper-field override-then-default resolution.tests.rs: end-to-endcompact_sessiontest provingcompaction_keep_recent_turns: 1keeps fewer turns verbatim than the default of 3.cargo build,cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,cargo nextest run --workspace --stress-count=5(1541 passed × 5).Out of scope (deliberate)
No global
Config/settings.json/env wiring, no per-session override, no reading from the calling agent.DEFAULT_COMPACT_SYSTEM_PROMPTunchanged.🤖 Generated with Claude Code
Summary by CodeRabbit