fix(ingestion): wire memory persistence contract to ingestion hook

[zachg-devops]

Summary

• Connects the MemoryPersistenceContractState to the SpacebotHook in the ingestion process_chunk function
• Adds .with_memory_persistence_contract(contract_state.clone()) to match how branch.rs wires the same state (line 71)

Problem

The ingestion pipeline creates a MemoryPersistenceContractState and passes it to the tool server, but never connects it to the SpacebotHook. Because memory_persistence_contract is None on the hook, the on_tool_call_result handler never calls set_terminal_outcome() — even when the model correctly calls memory_persistence_complete and receives a success response.

This causes has_terminal_outcome() to always return false, making every ingestion chunk fail with:

completed without memory_persistence_complete signal

Root Cause

In ingestion.rs line 517-523, the hook is constructed via SpacebotHook::new(...) without calling .with_memory_persistence_contract(). Compare with branch.rs line 70-71 which correctly wires the contract state.

Fix

One line: add .with_memory_persistence_contract(contract_state.clone()) to the hook construction in process_chunk.

Testing

Tested with 4 different models via OpenRouter (MiniMax M2.7, Step 3.5 Flash, GPT-4.1 nano, Mistral Small 3.2). All correctly call memory_persistence_complete but all fail the has_terminal_outcome() check. This fix resolves the check by ensuring the hook records the terminal outcome when the tool succeeds.

Fixes #538

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 379e68f1-86cc-4a60-b72c-08596dfaf6b9

📥 Commits

Reviewing files that changed from the base of the PR and between 9f8fee0 and d2950f2.

📒 Files selected for processing (1)

• src/hooks/spacebot.rs

---

Walkthrough

The ingestion chunk flow now wires the memory-persistence contract into the SpacebotHook and adds two Tokio regression tests validating that the hook records terminal outcomes only when the contract is attached.

Changes

Cohort / File(s)	Summary
Ingestion hook wiring src/agent/ingestion.rs	Chained .with_memory_persistence_contract(contract_state.clone()) onto SpacebotHook in process_chunk so the hook receives the memory-persistence contract state for completion signaling.
Spacebot hook tests src/hooks/spacebot.rs	Added two #[tokio::test] regression tests: one asserting a SpacebotHook without the contract does not record a terminal outcome, and one asserting a hook wired with the contract does record the terminal outcome after on_tool_result for memory_persistence_complete.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Linked Issues check	❓ Inconclusive	The PR addresses the primary symptom in issue #538 (the missing hook contract wiring) but does not implement the full set of objectives outlined in the issue.	This PR implements only step 1 (wiring the contract); consider follow-up PRs for retry logic, deduplication, and broader tool enforcement coverage to fully resolve issue #538.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main fix: wiring memory persistence contract to the ingestion hook, matching the core change in ingestion.rs.
Description check	✅ Passed	The description clearly explains the problem, root cause, fix, and testing validation; it is directly related to the changeset.
Out of Scope Changes check	✅ Passed	All changes are scoped to wiring the memory persistence contract in the ingestion hook and adding regression tests; no extraneous modifications are present.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

src/agent/ingestion.rs (1)

523-524: Please add a focused regression test for this wiring.

Given this is a stateful terminal-signal path, a targeted test would help prevent regressions where memory_persistence_complete succeeds but has_terminal_outcome() remains false.

Based on learnings: For changes in async/stateful paths (worker lifecycle, cancellation, retrigger, recall cache behavior), include explicit race/terminal-state reasoning in the PR summary and run targeted tests in addition to just gate-pr.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/agent/ingestion.rs` around lines 523 - 524, Add a focused regression test
that exercises the stateful terminal-signal path wired by
with_memory_persistence_contract(contract_state.clone()): simulate/drive
memory_persistence_complete on the contract_state (or call the same code paths
that would trigger it), then assert that has_terminal_outcome() becomes true for
the associated worker/operation; ensure the test reproduces the async lifecycle
(await necessary futures, use timers or synchronization to force potential
races) so it fails if persistence completes but has_terminal_outcome() is not
set.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/agent/ingestion.rs`:
- Around line 523-524: Add a focused regression test that exercises the stateful
terminal-signal path wired by
with_memory_persistence_contract(contract_state.clone()): simulate/drive
memory_persistence_complete on the contract_state (or call the same code paths
that would trigger it), then assert that has_terminal_outcome() becomes true for
the associated worker/operation; ensure the test reproduces the async lifecycle
(await necessary futures, use timers or synchronization to force potential
races) so it fails if persistence completes but has_terminal_outcome() is not
set.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ea78a699-c2bb-45c4-bac5-a2ec56b748cb

📥 Commits

Reviewing files that changed from the base of the PR and between fb5c0f3 and 9f8fee0.

📒 Files selected for processing (1)

• src/agent/ingestion.rs

[jamiepine]

Oh this is a great catch for a long time bug, thank you!

[zachg-devops]

Thanks @jamiepine! Happy to contribute.

Quick note on scope: the CodeRabbit pre-merge check flagged that issue #538 outlines four objectives but this PR only addresses one (the contract wiring). After testing, the contract wiring fix was the critical missing piece — once we applied it, ingestion completed successfully across 13 files with multiple models (Mistral Small 3.2, GPT-4.1 nano, MiniMax M2.7, Step 3.5 Flash).

The other items in #538 (retry loop for ingestion, deduplication on retry, ToolUseEnforcement widening) are separate concerns that could improve robustness further, but this one-line fix resolved the immediate blocker. Happy to submit separate PRs for those if there's interest.

The regression tests in the second commit verify both the broken path (hook without contract → has_terminal_outcome() stays false) and the fixed path (hook with contract → has_terminal_outcome() returns true).