Skip to content

Fix reflection fallback flow and dedupe duplicate integrated hooks#390

Closed
yeyeyeason wants to merge 1 commit intoCortexReach:masterfrom
yeyeyeason:fix/reflection-recovery-and-hook-dedupe
Closed

Fix reflection fallback flow and dedupe duplicate integrated hooks#390
yeyeyeason wants to merge 1 commit intoCortexReach:masterfrom
yeyeyeason:fix/reflection-recovery-and-hook-dedupe

Conversation

@yeyeyeason
Copy link
Copy Markdown

Summary

  • remove synchronous CLI full fallback from memory reflection and switch to minimal fallback + queued recovery bundles
  • add manual/queue/worker recovery paths for HQ reflection regeneration
  • add lightweight supersede/final semantics for recovered HQ reflections
  • write recovered HQ reflections into LanceDB so retrieval can prefer final outputs
  • prevent duplicate integrated memory-reflection hook registration within the same process/runtime via a global registration guard

Why

On /new, embedded reflection failures were falling into a heavier CLI fallback path that could amplify latency and retries. In addition, the same integrated memory-reflection hooks could be registered multiple times in one process, causing a single /new event to trigger serial duplicate reflection runs.

What changed

  • fallback path now writes a minimal reflection plus a recovery bundle instead of synchronously re-running a full CLI reflection
  • added recovery commands / worker flow for generating HQ recovered reflections
  • added file/header supersede markers so recovered HQ reflections are clearly preferred over provisional fallback reflections
  • recovered HQ reflections are stored back into LanceDB to bias future retrieval toward final versions
  • added process-global integrated hook registration dedupe for memory reflection

Notes

  • This PR is intentionally pragmatic and file-based for supersede semantics; it does not introduce a deep DB/schema-level supersede chain.
  • In the local environment where this was developed, the recovery worker still needed a valid plugin LLM OAuth file to actually generate HQ recoveries. That environment/config issue is separate from the hook dedupe fix in this PR.

@AliceLJY
Copy link
Copy Markdown
Collaborator

@yeyeyeason Thanks for the work, but this PR is too large to review safely — +4805 -4554 across index.ts, reflection-store.ts, and cli.ts is essentially a rewrite.

For the reflection loop problem specifically, #369 solves it with a targeted +41 line fix (global re-entrant guard + serial cooldown). We're going with that approach.

If you have additional improvements beyond the loop fix (recovery bundles, fallback path changes, CLI recovery commands), please split them into separate, focused PRs:

  1. Dedupe hook registration — if fix: prevent reflection loop with global cross-instance re-entrant guard #369 doesn't fully cover this
  2. Recovery bundle / queued regeneration — standalone feature PR
  3. CLI recovery commands — standalone feature PR

Each PR should be reviewable independently (<200 lines ideally). We can't merge a 5000-line PR that touches core registration and reflection logic in one shot.

@rwmjhb
Copy link
Copy Markdown
Collaborator

rwmjhb commented Mar 29, 2026

The issue is not just size. This PR currently mixes several separate changes into one review surface: reflection loop / hook dedupe, fallback flow rewrite, recovery bundles, and CLI recovery commands. That is already beyond what can be reviewed safely in a single pass.

There is also regression risk in the current implementation. At minimum, I was able to confirm that the existing sessionMemory hook contract is changed here, and that behavior change is not called out as a separate intended change in the PR description.

For the original reflection loop problem, I would strongly prefer the smaller and more targeted approach in #369. If the other improvements are still worth pursuing, please split them into focused follow-up PRs, for example:

  • hook dedupe / loop prevention
  • recovery bundle / queued regeneration
  • CLI recovery commands
  • fallback flow rewrite, if still needed

That will make each change independently reviewable and testable. My recommendation is to close this PR and resubmit the piecesseparately.

@rwmjhb
Copy link
Copy Markdown
Collaborator

rwmjhb commented Mar 29, 2026

I'm inclined to close this PR.

The issue is not just size. This PR currently mixes several separate changes into one review surface: reflection loop / hook dedupe, fallback flow rewrite, recovery bundles, and CLI recovery commands. That is already beyond what can be reviewed safely in a single pass.

There is also regression risk in the current implementation. At minimum, I was able to confirm that the existing sessionMemory hook contract is changed here, and that behavior change is not called out as a separate intended change in the PR description.

For the original reflection loop problem, I would strongly prefer the smaller and more targeted approach in #369. If the other improvements are still worth pursuing, please split them into focused follow-up PRs, for example:

  • hook dedupe / loop prevention
  • recovery bundle / queued regeneration
  • CLI recovery commands
  • fallback flow rewrite, if still needed

That will make each change independently reviewable and testable. My recommendation is to close this PR and resubmit the pieces separately.

@rwmjhb rwmjhb closed this Mar 29, 2026
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.

3 participants