Skip to content

fix(DEVS-145): resolve three post-merge Codex findings on PR #78 - #80

Merged
lbruton merged 2 commits into
mainfrom
fix/devs-145-codex-followups
Oct 27, 2026
Merged

lbruton merged 2 commits into
mainfrom
fix/devs-145-codex-followups

Conversation

@lbruton

@lbruton lbruton commented Oct 27, 2026

Copy link
Copy Markdown
Owner

Codex posted three review threads on #78 seconds before it merged, so the /pr-resolve pass did not see them. This PR fixes all three. Tracked under DEVS-145 (CLOSE-8); it gates the E.3 deploy.

Fixes

  • P1: shared embed cooldown in both directions (embedding_control.py, memory_index.py)
    • EmbeddingBudget.before_batch also honors EmbedThrottle.remaining(), so a transcript batch waits out a memory embed's cooldown.
    • A memory embed queued on mlx-embed behind a transcript batch calls EmbedThrottle.wait_blocking() on that thread before it embeds, and stamps the throttle there.
  • P2: no vector search under another model (memory_index.py)
    • vector_search re-reads the stored memory model and raises MemoryModelMismatch before it queries Milvus.
    • MemoryService already drops a failed vector channel and answers from FTS; a new test pins that for this error.
  • P2: CLI audit project inventory (memory_service.py, memory_cli.py)
    • memory_service.project_inventory() reads memory.project (else tag) from each known root's .claude/project.json, sharing the lookup with resolve_project_roots.
    • memory_cli audit passes it, so unknown projects are reported.
    • The report carries inventory_checked; when it is false, the CLI says the check was skipped instead of printing a clean result.

Tests

  • 9 new tests, written first and failing before the fix: 3 throttle, 2 index, 1 service, 3 CLI.
  • Full suite: 725 passed, 0 skipped, with SESSIONFLOW_TEST_PG_DSN and the restore DSN set.
  • ruff check . clean; Vale clean on README.md, AGENTS.md, CLAUDE.md.
  • test_memory_embeds_run_on_mlx_embed_behind_the_throttle is unchanged: wait_blocking is optional through getattr, as batch already is.

Codex posted three review threads on #78 seconds before it merged, so the
/pr-resolve pass never saw them.

- Shared embed cooldown in both directions (P1). EmbeddingBudget.before_batch
  checked only its own last batch, so a transcript batch could start right
  after a memory embed. It now also honors EmbedThrottle.remaining(). A memory
  embed queued on mlx-embed behind a transcript batch could also start at once;
  _embed_after_cooldown now calls EmbedThrottle.wait_blocking() on mlx-embed
  and stamps the throttle there.
- No vector search under another model (P2). vector_search re-reads the
  stored memory model and raises MemoryModelMismatch instead of querying
  Milvus; same-size vectors from another model would otherwise match.
  MemoryService already answers from FTS when the vector channel fails.
- CLI audit project inventory (P2). memory_cli audit passes
  memory_service.project_inventory() (memory.project, else tag, from each
  known root's .claude/project.json), so unknown projects are reported. The
  report carries inventory_checked, and the CLI says when the check could
  not run instead of printing a clean result.

Tests: 9 new (3 throttle, 2 index, 1 service, 3 CLI), written first and
failing before the fix. Full suite 725 passed, 0 skipped with the test DSNs
set; ruff clean; Vale clean on the changed docs.
@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review 🔄 Running since 2026-10-27T18:56:22.197314Z 7d7e8d0 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@codacy-production

codacy-production Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 33 complexity · 4 duplication

Metric Results
Complexity ✅ 33 (≤ 100 complexity)
Duplication ✅ 4 (≤ 5 duplication)

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 611254d759

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread memory_index.py
Codex review on #80: _embed_after_cooldown stamps the throttle on mlx-embed,
and leaving EmbedThrottle.batch() stamped it again on the event loop. A
transcript batch already sleeping out the first stamp could then be denied
on add_turns' single retry, and the watcher advances the transcript offset
anyway, so those turns were skipped.

EmbedThrottle.batch takes stamp=False; embed_for_index uses it, so the only
stamp is the one on mlx-embed. Tests: batch(stamp=False) leaves no stamp,
and one memory embed stamps exactly once, off the event loop thread. Full
suite 727 passed, 0 skipped; ruff clean.
@lbruton
lbruton merged commit 3b5aee5 into main Oct 27, 2026
9 checks passed
@lbruton
lbruton deleted the fix/devs-145-codex-followups branch October 27, 2026 19:02
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.

1 participant