Skip to content

refactor(components): remove offloaders that delete by heuristic - #474

Open
OsherElhadad wants to merge 7 commits into
mainfrom
feat/remove-heuristic-deletion
Open

OsherElhadad wants to merge 7 commits into
mainfrom
feat/remove-heuristic-deletion

Conversation

@OsherElhadad

@OsherElhadad OsherElhadad commented Oct 12, 2026 •

Copy link
Copy Markdown
Collaborator

Part of #471 (compaction cleanup).

What changes

A component may now remove only content that is provably redundant (duplicates, superseded copies, formatting, boilerplate), or content it parks behind an expand handle where the choice of what to park comes from the content itself. Components that deleted by age, size, run order, or a prediction about the future are removed:

failed_run, collapse, mask, linecap, smartcrush, readlifecycle, coref, agentdiet, skeleton (and the tree-sitter dependency that only skeleton used).

Removed from: code, presets (balanced, aggressive, coding, mcp, agent, general, codesmart, codesafe, and the agentdiet preset), stats fields, docs and nav, the plugin component list, and the benchmark sweep arms. internal/coref stays: extract_llm_sweep passes its index to the model as evidence.

Kept after review: format, textclean, searchfold, toon, dedup, extract, cmdfilter, extract_llm, extract_llm_sweep, summarize, cache_aware_summarizer, cachesplit, cacheinject, toolschema, toolfilter. The default house pipeline never contained any of the removed components, so its behaviour is unchanged.

Compatibility

A stored config that still names a removed component, or preset: agentdiet, loads, logs a warning and runs without it. It does not stop the proxy. config.LoadBytes and Config.Build both drop the names, so library callers are covered. Test: TestRemovedComponentsAreIgnoredNotFatal. TestEveryPresetBuilds now fails if a preset names a removed or unregistered component.

Evidence

The per-component audit and the replay figures are kept in the private research notes. Summary: each removed component failed the pre-declared check that the agent does not need removed text, or had no supporting measurement. The default house pipeline is byte-identical in replay, with no warm-cache busts, format failures or reverts for any preset. Presets that carried a removed component lose the savings those components made; none of them is the service default.

Tests

go test ./... passes (pure-Go build). Tests that used mask, collapse or linecap as a generic offloader fixture were moved to extract or cmdfilter; tests whose subject was a removed component were deleted. mkdocs build --strict passes.

Not in this PR: marker_mode: summary|off (no expand handle, in no preset) is left as is and flagged in #471.

Osher-Elhadad added 2 commits October 12, 2026 00:20
A component may now remove only content that is provably redundant
(duplicates, superseded copies, formatting, boilerplate) or that it parks
behind an expand handle chosen from the content itself. The following
deleted by age, size, run order or prediction and are removed:
failed_run, collapse, mask, linecap, smartcrush, readlifecycle, coref,
agentdiet and skeleton (with the tree-sitter dependency).

Presets, docs, the plugin component list, stats fields and benchmark
arms no longer name them. A stored config that still does loads, logs a
warning and runs without the component (the agentdiet preset likewise),
so an old document cannot stop the proxy at boot.

Refs #471

Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com>
Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com>
@OsherElhadad

Copy link
Copy Markdown
Collaborator Author

CI is green. Task-success check: the default house pipeline is byte-identical before and after this change (replay of 2,123 requests), so the existing real-agent SWE-bench results for house apply unchanged. A paired run of the cleaned general preset against the same task list is queued with the benchmark owner and will be reported on #471.

@OsherElhadad

Copy link
Copy Markdown
Collaborator Author

Review of 20057e6 + c6b6242. Verified in a fresh worktree: go build, go vet clean; go test ./... passes except dash (SQLITE_BUSY, load-dependent) and one proxy test (database is locked); both fail the same way on origin/main under the current host load (load avg ~40), neither touches code this PR changes. TestRemovedComponentsAreIgnoredNotFatal and TestEveryPresetBuilds pass.

Blocking

  1. The PR description publishes per-component experiment results (retained-% table with unit counts, per-preset token-share changes, "pre-declared bound", replay sizes). This repo is public and the mission rule is that method and measured numbers about features go to the private research repo. Replace the Evidence section with a one-line pointer ("evidence is in the private research notes") and keep only the compatibility and test statements. The commit messages are fine.

Non-blocking
2. Stale references to removed components remain in non-test code comments: components/offload/common.go:45,56,219 (collapse.max_tokens, mask, smartcrush), components/dsl/dsl.go:286 (linecap), components/offload/cmdfilter_agentcmd.go:40 (skeleton.go), components/offload/extract_llm.go:942 ("unlike mask/failed_run"), components/component.go:39 ("code skeleton"). Reword or drop in this PR since it is the cleanup PR.
3. The /compact override path (cmd/context-guru-proxy/main.go:598) now answers ?preset=agentdiet with "unknown preset" (error) while a stored config with preset: agentdiet loads. Acceptable, but say so in the PR text, or have PresetPipeline return an empty pipeline for it.
4. Pipeline-name handling is correct on every path I traced (LoadBytes, Build for struct callers, Validate which goes through LoadBytes, applyPreset). Note Validate therefore accepts a removed name silently at settings-save time; the warning only shows in the log. Fine for compatibility; consider rejecting it for new saves later.

Safety: house pipeline unchanged; no defaults changed; go.mod drops the tree-sitter deps only. Privacy: nothing found in the diff. Attribution: no AI/Co-Authored-By lines in the two commits. Not labelled ready-to-merge until item 1 is fixed.

Osher-Elhadad added 2 commits October 12, 2026 08:55
Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com>
…e on /compact too

Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com>
@OsherElhadad

Copy link
Copy Markdown
Collaborator Author

Re-review of 03b92fe: blocker 1 is resolved (the PR body carries no measurements). Item 3 is fixed and covered by TestRemovedPresetResolvesToEmptyPipeline. Build and vet are clean; config, cmd and components tests pass. The one failure, TestExtractLLMDoesNotBookASingleFlightFollowersSaving, is a flake that fails the same way on origin/main (29 of 40 runs on main, 31 of 40 here) and is unrelated.

Remaining nit, not blocking: comments in components/offload/state.go (lines 59, 258, 287, 378, 429) and config/form.go (65, 509) still name mask, failed_run and linecap. Historical wording, fine to leave or tidy later.

Approving; labelled ready-to-merge.

Osher-Elhadad added 2 commits October 14, 2026 06:48
Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com>

# Conflicts:
#	context-guru-plugin/skills/insights-components/SKILL.md
…sk is removed

Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com>

@OsherElhadad OsherElhadad left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: APPROVE at f0a0237. (Posted as a comment: GitHub does not let the author account approve.)

  • Merge 83be4b8. Its only conflict was context-guru-plugin/skills/insights-components/SKILL.md, and it was resolved to main's version: the file has no diff against main and names none of the removed components.
  • Removed components. failed_run, collapse, mask, linecap, smartcrush, readlifecycle, coref, agentdiet and skeleton are no longer referenced by any preset, shipped config, plugin script or doc except in historical notes. A stored config that still names one keeps loading: dropRemoved drops it with a warning, and preset: agentdiet warns and runs the pipeline as written.
  • Fixtures. f0a0237 drives the thread-cache fixtures with extract, and the test still fails if extract acts on nothing ("the comparison proves little"), so the fixtures still test something.
  • Checks. gofmt and go vet ./... are clean. go test passes for ./config, ./components/..., ./apply, ./context-guru-plugin, ./proxy and ./dash. CI is green. All new commits, including the merge, are signed off with no AI attribution. It merges cleanly with #475 and #500.

Non-blocking:

  • The removed TestKeptVerbatimNotRecompacted was the only end-to-end pipeline test of the kept-verbatim guarantee, and it drove mask. The shared skipReduce path still enforces it, but porting the test to extract would keep it pinned.
  • Some deploy/harbor/cfg-iter02*.yaml experiment configs still list collapse and coref. They load with a warning; delete or annotate them as historical.

Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com>

# Conflicts:
#	components/offload/skeleton_measure_test.go
#	components/offload/testdata/read_lifecycle.json
#	docs/components/offload-reducers.md
#	docs/components/skeleton.md

@OsherElhadad OsherElhadad left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: APPROVE at 4e22725 (main merged; last approved f0a0237).

  • Conflicts. git show --remerge-diff 4e22725c shows four, all on files this PR deletes:
    • skeleton_measure_test.go, testdata/read_lifecycle.json and docs/components/skeleton.md were modify/delete conflicts. Main's only edits to them were path placeholders from #529, so dropping those edits is correct.
    • In docs/components/offload-reducers.md, the resolution keeps the intro and the dedup/extract sections. It removes only the blocks for removed components: failed_run, linecap, smartcrush, mask, collapse and read_lifecycle.
    • Nothing else from main is lost.
  • Leftovers. No build-time or config reference to the removed components remains. What does remain is historical comments, a CI comment about the old preset breakage, internal/coref (still used by extract_llm_sweep), and test stubs that use the names only as strings. mkdocs build --strict passes.
  • Tests. On a local merge with today's main, gofmt and go vet ./... are clean. ./config, ./components/..., ./apply, ./context-guru-plugin, ./proxy and ./dash all pass. CI is green, and the merge commit is signed off.

The earlier non-blocking note (port the end-to-end "kept verbatim" test to extract) still stands. This approval covers 4e22725.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge Program PR approved by the reviewer, CI green; needs maintainer approval

Projects

Status: New/ToDo

Development

Successfully merging this pull request may close these issues.

2 participants