hook: deprecate inert PreCompact FTS5 scorer, keep as no-op stub - #215
Conversation
📝 WalkthroughWalkthroughThe PreCompact hook is now documented as deprecated and unsupported, and its implementation is a no-op. Legacy parsing and relevance helpers remain compiled with dead-code allowances, while existing configuration wiring continues to avoid errors. ChangesPreCompact hook deprecation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components.rs (1)
542-565: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the “pin to ultra” check require
ultra.A config with
defaultMode: "off","lite", or"full"is reported as satisfied, andwrite_pinned_modethen refuses to update it. Either enforceultrawhile preserving the existing JSON document, or rename this component so it does not promise pinning.🤖 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 `@src/components.rs` around lines 542 - 565, Update the check closure for the “optimize-code-mode” component to return true only when the parsed config’s defaultMode value is exactly “ultra”, while continuing to handle missing or invalid configuration as unsatisfied. Preserve the existing JSON document behavior and keep write_pinned_mode responsible for applying the setting.
🧹 Nitpick comments (1)
AGENTS.md (1)
38-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
ctx_globmapping.The strict contract covers search, symbols, call graphs, composition, reads, and shell output, but omits file/glob discovery.
src/rule_text.rsstill definesctx_globas the lean-ctx replacement for Glob, while Line 50 prohibits nativefind; add an explicitctx_globrule so agents have an allowed discovery path.As per coding guidelines, code intelligence must be routed through lean-ctx rather than native tools.
🤖 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 `@AGENTS.md` around lines 38 - 43, Update the tool-mapping guidance in AGENTS.md to explicitly document ctx_glob as the lean-ctx replacement for native Glob/file discovery. Add this rule near the existing ctx_search and ctx_callgraph mappings, while preserving the prohibition on native find and routing discovery through ctx_glob.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 `@src/auth_runner.rs`:
- Line 44: Update the comment immediately above the backoff logic in auth_runner
to use the canonical optimize-code prefix instead of flare-code, preserving the
remainder of the comment unchanged.
In `@src/init.rs`:
- Around line 206-215: Update the Claude Code branch in the initialization flow
around confirm_ponytail_migration so Optimize hooks are not wired while Ponytail
remains installed. Make the migration check return false until removal is
confirmed, and prompt the user to rerun initialization after uninstalling
Ponytail; preserve hook installation only for confirmed migrations.
- Around line 601-607: The initialization logic must replace substring-based
idempotency checks with exact per-hook inspection. In src/init.rs lines 601-607,
update the Claude Code managed hook/status-line setup to inspect each configured
command or event and add any missing Optimize hook or status line; in
src/init.rs lines 649-655, apply the same exact managed-command/event checks for
Cursor and add any missing Optimize hook. Preserve existing entries and avoid
treating an unrelated command containing “optimize” as already configured.
In `@src/mcp_prompts.rs`:
- Around line 46-49: Update the descriptions for the “optimize” prompt and its
corresponding handler message to document every mode accepted by
normalize_config_mode, including review, audit, debt, gain, help, playbook, and
no-hallucination, while retaining the existing lite, full, ultra, off, and
status modes.
In `@src/mcp_server.rs`:
- Around line 671-674: The sizing contract should consistently use metadata.size
rather than size labels. In src/mcp_server.rs lines 671-674, update the
unestimated guidance to instruct setting metadata.size; in
.claude/skills/pm/reference/rubric.md lines 11-15, update the later guidance to
recommend metadata={"size":"S"|"M"|"L"}.
In `@src/mcp_server/item.rs`:
- Around line 799-800: Align PM documentation with completed_at semantics: in
src/mcp_server/item.rs lines 799-800, remove the velocity updated_at-proxy
claim; in .claude/skills/pm/SKILL.md lines 32-38, distinguish exact completion
timing from approximate stale/stuck activity timing; in
.claude/skills/pm/SKILL.md lines 87-96, remove the old proxy caveat from
velocity; and in .claude/skills/pm/reference/read-recipe.md lines 15-20, update
the caveat to describe standup’s completed_at behavior.
In `@src/worktree.rs`:
- Around line 171-173: Update the successful already-isolated branch around
already_isolated_for to pass repo_root to isolate_worktree_target_dir and return
repo_root instead of the derived worktree_path, ensuring the actual linked
worktree is isolated and reclaimed.
- Around line 133-140: Update the configuration setup around the config_path
existence check and std::fs::write call to also detect the legacy .cargo/config
file. If that legacy file exists, merge the target-dir setting into it or warn
and skip writing config.toml, ensuring Cargo does not receive an ineffective
configuration.
- Around line 563-574: Remove the process-wide environment mutation from
warn_if_ambient_target_dir_warns_when_set, including the unsafe
set_var/remove_var calls and the non-assertive invocations. Either delete this
test or refactor warn_if_ambient_target_dir to accept an injectable environment
reader, then assert the warning behavior for a configured CARGO_TARGET_DIR
without changing global process state.
---
Outside diff comments:
In `@src/components.rs`:
- Around line 542-565: Update the check closure for the “optimize-code-mode”
component to return true only when the parsed config’s defaultMode value is
exactly “ultra”, while continuing to handle missing or invalid configuration as
unsatisfied. Preserve the existing JSON document behavior and keep
write_pinned_mode responsible for applying the setting.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 38-43: Update the tool-mapping guidance in AGENTS.md to explicitly
document ctx_glob as the lean-ctx replacement for native Glob/file discovery.
Add this rule near the existing ctx_search and ctx_callgraph mappings, while
preserving the prohibition on native find and routing discovery through
ctx_glob.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a012e120-5c99-4ae1-a0d2-196392fece76
📒 Files selected for processing (20)
.claude/skills/pm/SKILL.md.claude/skills/pm/reference/read-recipe.md.claude/skills/pm/reference/rubric.mdAGENTS.mdcrates/agentflare-backend/src/item.rsopencode.jsonsrc/auth_runner.rssrc/cli/caveman.rssrc/cli/hook.rssrc/cli/mod.rssrc/cli/ponytail.rssrc/components.rssrc/hook.rssrc/init.rssrc/mcp_prompts.rssrc/mcp_server.rssrc/mcp_server/item.rssrc/optimize/code.rssrc/worktree.rstests/caveman_cli.rs
💤 Files with no reviewable changes (3)
- src/cli/ponytail.rs
- src/cli/caveman.rs
- src/cli/mod.rs
| eprintln!("retrying with new profile ({remaining} retries left)..."); | ||
| } | ||
| // ponytail: short backoff, linear increase if rate limits persist | ||
| // flare-code: short backoff, linear increase if rate limits persist |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the canonical optimize-code label.
Line 44 says flare-code, but the repository's current command and documentation use optimize code. Rename the comment prefix to keep terminology consistent.
🤖 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 `@src/auth_runner.rs` at line 44, Update the comment immediately above the
backoff logic in auth_runner to use the canonical optimize-code prefix instead
of flare-code, preserving the remainder of the comment unchanged.
| "claude-code" => { | ||
| wire_claude_code(); | ||
| if confirm_ponytail_migration(agent, yes) { | ||
| wire_ponytail_hooks(agent); | ||
| wire_optimize_hooks(agent); | ||
| } | ||
| } | ||
| "cursor" => { | ||
| wire_cursor(); | ||
| if confirm_ponytail_migration(agent, yes) { | ||
| wire_ponytail_hooks(agent); | ||
| wire_optimize_hooks(agent); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not wire Optimize hooks before Ponytail is actually removed.
For Claude Code, confirm_ponytail_migration() only prints manual uninstall instructions but returns true; this immediately installs Optimize hooks despite the same function stating the remaining Ponytail plugin conflicts. Return false until removal is confirmed and ask the user to rerun initialization.
🤖 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 `@src/init.rs` around lines 206 - 215, Update the Claude Code branch in the
initialization flow around confirm_ponytail_migration so Optimize hooks are not
wired while Ponytail remains installed. Make the migration check return false
until removal is confirmed, and prompt the user to rerun initialization after
uninstalling Ponytail; preserve hook installation only for confirmed migrations.
| let already_wired = settings | ||
| .get("hooks") | ||
| .and_then(|h| h.get("SessionStart")) | ||
| .map(|v| v.to_string().contains("ponytail")) | ||
| .map(|v| v.to_string().contains("optimize")) | ||
| .unwrap_or(false); | ||
| if already_wired { | ||
| println!(" skip ponytail hooks already wired in ~/.claude/settings.json"); | ||
| println!(" skip optimize code hooks already wired in ~/.claude/settings.json"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use exact per-hook idempotency checks instead of a substring sentinel.
src/init.rs#L601-L607: inspect the managed Claude Code commands/events and add any missing Optimize hook or status line.src/init.rs#L649-L655: inspect Cursor’s managed commands/events and add any missing Optimize hook.
📍 Affects 1 file
src/init.rs#L601-L607(this comment)src/init.rs#L649-L655
🤖 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 `@src/init.rs` around lines 601 - 607, The initialization logic must replace
substring-based idempotency checks with exact per-hook inspection. In
src/init.rs lines 601-607, update the Claude Code managed hook/status-line setup
to inspect each configured command or event and add any missing Optimize hook or
status line; in src/init.rs lines 649-655, apply the same exact
managed-command/event checks for Cursor and add any missing Optimize hook.
Preserve existing entries and avoid treating an unrelated command containing
“optimize” as already configured.
| "optimize", | ||
| Some("Switch or report flare code lazy-dev mode"), | ||
| Some(vec![PromptArgument::new("mode") | ||
| .with_description("lite|full|ultra|off|status (omit to report current mode)")]), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document every mode accepted by this handler.
review, audit, debt, gain, help, playbook, and no-hallucination are accepted by normalize_config_mode, but both messages imply only lite|full|ultra|off|status are valid.
Also applies to: 131-133
🤖 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 `@src/mcp_prompts.rs` around lines 46 - 49, Update the descriptions for the
“optimize” prompt and its corresponding handler message to document every mode
accepted by normalize_config_mode, including review, audit, debt, gain, help,
playbook, and no-hallucination, while retaining the existing lite, full, ultra,
off, and status modes.
| if config_path.exists() { | ||
| return; // don't clobber an intentional worktree-local override | ||
| } | ||
| let content = "[build]\n# Isolated per worktree (see item #133). Registry deps are\n\ | ||
| # better shared via sccache (RUSTC_WRAPPER + SCCACHE_BASEDIRS),\n\ | ||
| # not a shared CARGO_TARGET_DIR, which leaks artifacts across worktrees.\n\ | ||
| target-dir = \"target\"\n"; | ||
| if let Err(e) = std::fs::write(&config_path, content) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
# Use lean-ctx to locate existing Cargo-config handling before choosing
# whether to merge or explicitly warn on legacy `.cargo/config`.
ctx_search ".cargo/config"
ctx_read src/worktree.rs --start-line 129 --end-line 146Repository: getappz/agentflare
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and nearby config handling.
git ls-files src/worktree.rs .cargo .cargo/config .cargo/config.toml Cargo.toml
echo '--- src/worktree.rs around the reported lines ---'
nl -ba src/worktree.rs | sed -n '120,170p'
echo '--- search for cargo config handling in the repo ---'
rg -n --hidden --glob '!target' --glob '!node_modules' '\.cargo/config|config\.toml|CARGO_TARGET_DIR|target-dir' .Repository: getappz/agentflare
Length of output: 273
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read any repo docs or tests that describe Cargo config precedence or worktree setup.
rg -n --hidden --glob '!target' --glob '!node_modules' 'worktree|cargo config|target-dir|config.toml|\.cargo/config' README* . github . || trueRepository: getappz/agentflare
Length of output: 33529
🌐 Web query:
Cargo config precedence .cargo/config.toml .cargo/config if both exist
💡 Result:
If both .cargo/config.toml and .cargo/config exist in the same directory, Cargo will ignore the .toml extension and use the file without the extension (i.e., .cargo/config) [1][2]. While .cargo/config.toml is the modern, preferred format introduced in Cargo 1.39, this specific precedence rule—where the extensionless file takes priority—is maintained to ensure backward compatibility [1][2]. For general configuration precedence across different directories, Cargo searches for files by walking up the directory tree from the current working directory to the filesystem root, and finally checks the global $CARGO_HOME/config.toml [1][3]. Values defined in files closer to the project directory take precedence over those in parent or global directories [1][2]. Environment variables and command-line --config arguments generally take the highest precedence [4][5][3].
Citations:
- 1: https://doc.rust-lang.org/stable/cargo/reference/config.html
- 2: https://doc.rust-lang.org/1.63.0/cargo/reference/config.html
- 3: https://deepwiki.com/rust-lang/cargo/1.2-configuration-system
- 4: https://doc.rust-lang.org/cargo/reference/config.html
- 5: https://blog.rust-lang.org/inside-rust/2023/08/24/cargo-config-merging/
Handle legacy .cargo/config too. Cargo ignores .cargo/config.toml when an extensionless .cargo/config exists beside it, so this can write a config that never takes effect. Detect the legacy file and merge into it or warn/skip.
🤖 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 `@src/worktree.rs` around lines 133 - 140, Update the configuration setup
around the config_path existence check and std::fs::write call to also detect
the legacy .cargo/config file. If that legacy file exists, merge the target-dir
setting into it or warn and skip writing config.toml, ensuring Cargo does not
receive an ineffective configuration.
| warn_if_ambient_target_dir(); | ||
| isolate_worktree_target_dir(&worktree_path); | ||
| return Some(worktree_path); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Isolate and return the actual reclaimed worktree.
When already_isolated_for succeeds, repo_root is the linked worktree. worktree_path was derived as repo_root/.worktrees/task/<id>, so this creates .cargo under a nonexistent nested path and returns that wrong path. Use repo_root here.
Proposed fix
- isolate_worktree_target_dir(&worktree_path);
- return Some(worktree_path);
+ isolate_worktree_target_dir(repo_root);
+ return Some(repo_root.to_path_buf());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| warn_if_ambient_target_dir(); | |
| isolate_worktree_target_dir(&worktree_path); | |
| return Some(worktree_path); | |
| warn_if_ambient_target_dir(); | |
| isolate_worktree_target_dir(repo_root); | |
| return Some(repo_root.to_path_buf()); |
🤖 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 `@src/worktree.rs` around lines 171 - 173, Update the successful
already-isolated branch around already_isolated_for to pass repo_root to
isolate_worktree_target_dir and return repo_root instead of the derived
worktree_path, ensuring the actual linked worktree is isolated and reclaimed.
| #[test] | ||
| fn warn_if_ambient_target_dir_warns_when_set() { | ||
| // Just asserts the function runs without panicking whether or not the | ||
| // var is set; the warning is an ephemeral eprintln, not assertable here. | ||
| unsafe { | ||
| std::env::set_var("CARGO_TARGET_DIR", "/tmp/shared"); | ||
| } | ||
| warn_if_ambient_target_dir(); | ||
| unsafe { | ||
| std::env::remove_var("CARGO_TARGET_DIR"); | ||
| } | ||
| warn_if_ambient_target_dir(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
# Inspect all test-side process-environment mutations before removing or
# refactoring this coverage.
ctx_search "set_var"
ctx_search "remove_var"
ctx_read src/worktree.rs --start-line 563 --end-line 575Repository: getappz/agentflare
Length of output: 290
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== search for process env mutations ==\n'
grep -RIn --exclude-dir=.git -E 'std::env::(set_var|remove_var)\(' .
printf '\n== inspect the test block ==\n'
nl -ba src/worktree.rs | sed -n '548,586p'Repository: getappz/agentflare
Length of output: 3981
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
path = Path("src/worktree.rs")
lines = path.read_text().splitlines()
for start, end in [(500, 620), (1, 120)]:
print(f"\n== {path} lines {start}-{end} ==")
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: getappz/agentflare
Length of output: 11322
Don’t mutate process-wide env in this test — set_var/remove_var on CARGO_TARGET_DIR is unsafe under concurrent test execution, and this case doesn’t assert any observable behavior. Drop it or inject an env reader and assert the warning path instead.
🤖 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 `@src/worktree.rs` around lines 563 - 574, Remove the process-wide environment
mutation from warn_if_ambient_target_dir_warns_when_set, including the unsafe
set_var/remove_var calls and the non-assertive invocations. Either delete this
test or refactor warn_if_ambient_target_dir to accept an injectable environment
reader, then assert the warning behavior for a configured CARGO_TARGET_DIR
without changing global process state.
Claude Code's PreCompact hook is blocking-only and never consumes hookSpecificOutput.additionalContext, so pre_compact's scored-lines output was always discarded unread. Compaction-survival is now handled end-to-end by the lean-ctx sidecar instead. pre_compact becomes a no-op stub (like session_end) rather than being deleted, since old settings.json installs still wire it and crate::compact::score_lines is still used by coaching::store for the rule digest. parse_pre_compact/relevance_query stay in place (marked #[allow(dead_code)]) for their existing unit tests and in case compaction-survival is ever reactivated here.
d6ecc1e to
42aa13e
Compare
1 participant
Summary
hookSpecificOutput.additionalContext, sopre_compact's scored-lines output was always discarded unread — the feature has never done anything.pre_compactbecomes a no-op stub (likesession_end) instead of being deleted, since existingsettings.jsoninstalls still wire it andcrate::compact::score_linesis still used bycoaching::storefor the rule digest.parse_pre_compact/relevance_query/PreCompactInputare kept (#[allow(dead_code)]) rather than removed, so their unit tests still cover the parsing/ranking logic and the code is available if compaction-survival is ever reactivated here.Addresses item #91 in our internal agentflare project tracker (not a GitHub issue — unrelated to GH #91).
Test plan
cargo build --workspace --all-featuresRUSTFLAGS="-D warnings" cargo build --workspace --all-featurescargo test -p agentflare hook::— all 22hook::testspass, includingparse_pre_compact_*/relevance_query_*