Skip to content

Remove legacy memory push/inject in report building#4455

Draft
Hmbown wants to merge 2 commits into
mainfrom
jules-legacy-memory-push-removal-3120618009560725208
Draft

Remove legacy memory push/inject in report building#4455
Hmbown wants to merge 2 commits into
mainfrom
jules-legacy-memory-push-removal-3120618009560725208

Conversation

@Hmbown

@Hmbown Hmbown commented Jul 17, 2026

Copy link
Copy Markdown
Owner

This PR removes the legacy memory push/inject blocks and related properties that were guarded by a TODO indicating their planned removal. The UserMemory and ConfigEnabled tokens have also been cleaned up along with unused properties like App::moraine_fallback.


PR created automatically by Jules for task 3120618009560725208 started by @Hmbown

Removed the legacy memory push/inject logic in `build_headless_context_report` and `add_app_runtime_entries` when Moraine recall is stable. Cleaned up unused `SourceKind::UserMemory`, `ActivationReason::ConfigEnabled`, `App::moraine_fallback` states, and associated tests.

Co-authored-by: Hmbown <101357273+Hmbown@users.noreply.github.com>
@google-labs-jules

Copy link
Copy Markdown
Contributor

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

Copilot AI review requested due to automatic review settings July 17, 2026 01:50
@cursor

cursor Bot commented Jul 17, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 2m 58s —— View job


Review: Remove legacy memory push/inject in report building

  • Gather context (read diff, changed files)
  • Understand the request (code review)
  • Review crates/tui/src/context_report.rs changes
  • Review crates/tui/src/tui/app.rs changes
  • Check for dangling references to removed code (compile safety)
  • Post final review

Summary

This PR deletes the UserMemory source-entry blocks (and the now-unused SourceKind::UserMemory / ActivationReason::ConfigEnabled enum variants, App::moraine_fallback, and the two tests covering that report behavior) from the diagnostic context report (context_report.rs). I verified there are no dangling references left anywhere in the workspace — SourceKind::UserMemory, ActivationReason::ConfigEnabled, and App::moraine_fallback (as a field) don't appear outside this diff, PathBuf was correctly dropped from the test-module imports since it's now unused, and both build_context_report/build_headless_context_report are left with no leftover unused locals. This should compile cleanly.

Correctness concern: report now diverges from actual system-prompt content

context_report.rs is explicitly a "Diagnostic prompt source map for context pressure reports" (module doc, line 1) — it's what backs the /context/tokens debug command (crates/tui/src/commands/groups/debug/tokens.rs:135) and the headless doctor report (main.rs:4872), i.e. it's meant to describe what's actually in the model's context and why.

This PR only removes the memory entry from the report. The actual <user_memory> legacy push/inject path is untouched and still runs at request time:

  • crates/tui/src/core/engine.rs:1014-1015 and :3665-3666crate::memory::compose_block(config.memory_enabled && !config.moraine_fallback, ...)
  • crates/tui/src/tui/ui.rs:1696-1697 — same pattern, feeding build_app_system_prompt_with_goal

So for any user with [memory] enabled = true and moraine_fallback not set (the default), the <user_memory> block is still injected into the real system prompt and consumes real tokens, but the diagnostic report will now silently omit it from both the entry list and the summed active_context_estimated_tokens/budget_used_percent. That's a step backward for the report's stated purpose (accurately explaining context pressure) — it will under-count usage and can no longer tell a user "here's why you're near your context limit" if legacy memory is a contributor.

Given the PR title scopes this to "in report building," this may be intentional as a first slice ahead of removing the underlying push/inject in engine.rs/ui.rs/tool_setup.rs (which still carry the same TODO(v0.8.71) markers and are unmodified here). If that's the plan, it'd help to note it explicitly in the PR description so reviewers/maintainers know the remaining call sites are intentionally out of scope rather than missed. If the intent was actually to fully retire the legacy path, the compose_block calls in engine.rs and ui.rs and the moraine_fallback/memory_enabled config plumbing should be removed in the same pass — otherwise the codebase now has two independent copies of the same gating logic (memory_enabled && !moraine_fallback) that can drift, one of which no longer has any test coverage confirming it matches the report.

Nits

  • No new tests were added to assert that memory-related entries are never emitted from build_context_report/build_headless_context_report — the two deleted tests were the only coverage exercising the memory branch in this file, and they're deleted outright rather than replaced with a smaller "memory never shows up" assertion.

Fix this →

@Hmbown
Hmbown marked this pull request as draft July 17, 2026 01:52
@Hmbown Hmbown added this to the v0.9.4 milestone Jul 17, 2026
@Hmbown

Hmbown commented Jul 17, 2026

Copy link
Copy Markdown
Owner Author

Thanks — this belongs with the Moraine migration in v0.9.4, not the v0.9.1 release train, and I have moved it there and marked it draft.

This cannot land as an isolated cleanup yet. It removes UserMemory reporting and the App::moraine_fallback projection while legacy memory is still active in the engine, quick-add interception, remember-tool registration, runtime-thread configuration, and CLI surface. That would make the context report stop disclosing memory that can still be injected, while #3495 still requires the Codewhale Moraine adapter, MCP wiring, and end-to-end recall proof before the legacy path is deprecated.

Please hold this as part of one coherent #3495 cutover. The eventual patch needs to remove or migrate every legacy consumer together, preserve truthful context/runtime reporting during the transition, and prove Codewhale session ingest plus MCP recall end to end. It also overlaps the held Moraine cleanup PRs #4442, #4444, #4446, and #4447, so those should be consolidated rather than merged independently.

@google-labs-jules

Copy link
Copy Markdown
Contributor

Thanks — this belongs with the Moraine migration in v0.9.4, not the v0.9.1 release train, and I have moved it there and marked it draft.

This cannot land as an isolated cleanup yet. It removes UserMemory reporting and the App::moraine_fallback projection while legacy memory is still active in the engine, quick-add interception, remember-tool registration, runtime-thread configuration, and CLI surface. That would make the context report stop disclosing memory that can still be injected, while #3495 still requires the Codewhale Moraine adapter, MCP wiring, and end-to-end recall proof before the legacy path is deprecated.

Please hold this as part of one coherent #3495 cutover. The eventual patch needs to remove or migrate every legacy consumer together, preserve truthful context/runtime reporting during the transition, and prove Codewhale session ingest plus MCP recall end to end. It also overlaps the held Moraine cleanup PRs #4442, #4444, #4446, and #4447, so those should be consolidated rather than merged independently.

Understood. Acknowledging that this work is now obsolete and stopping work on this task.

This reverts the changes as requested since the PR should be part of a consolidated transition later on.

Co-authored-by: Hmbown <101357273+Hmbown@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants