feat(rebuild_log): instrument UserPromptSubmit path (#288 phase-1a) - #358
Merged
Conversation
Reviewer's GuideAdds rebuild_log instrumentation for the high-frequency UserPromptSubmit retrieval path, sharing schema and config with the existing PreCompact/rebuild_v14 logging, and verifies it with focused tests and doc updates. Sequence diagram for UserPromptSubmit rebuild_log instrumentationsequenceDiagram
actor User
participant GitHook as user_prompt_submit
participant Search as search_for_prompt
participant Retrieve as retrieve
participant HookLog as _emit_user_prompt_submit_rebuild_log
participant Ctx as record_user_prompt_submit_log
participant FS as rebuild_logs_dir
User->>GitHook: invoke user_prompt_submit(prompt)
GitHook->>Search: search_for_prompt(prompt)
Search->>Retrieve: retrieve(prompt)
Retrieve-->>GitHook: hits (pre_dedup)
GitHook->>GitHook: compute n_returned, n_unique, n_l0, n_l1
GitHook->>GitHook: hits_pre_dedup = list(hits)
alt config.collapse_duplicate_hashes
GitHook->>GitHook: hits = _dedup_by_content_hash(hits)
end
GitHook->>HookLog: _emit_user_prompt_submit_rebuild_log(prompt, session_id, hits_pre_dedup, hits_post_dedup=hits)
HookLog->>HookLog: if not session_id or db_path()==":memory:": return
HookLog->>HookLog: log_path = _rebuild_log_dir_for_db(db_path()) / session_id.jsonl
HookLog->>Ctx: record_user_prompt_submit_log(prompt, session_id, hits_pre_dedup, hits_post_dedup, log_path, enabled, stderr)
Ctx->>Ctx: check enabled, env opt-out, log_path, hits_pre_dedup
Ctx->>Ctx: surviving_ids, survivor_by_hash from hits_post_dedup
Ctx->>Ctx: build candidates with packed/dropped and reasons
Ctx->>Ctx: build pack_summary
Ctx->>Ctx: synthetic_turn = RecentTurn(role=user, text=prompt, session_id)
Ctx->>Ctx: record = _build_rebuild_log_record([...synthetic_turn...], candidates, pack_summary)
Ctx->>FS: _append_rebuild_log_record(log_path, record)
FS-->>Ctx: append ok
Ctx-->>HookLog: return
HookLog-->>GitHook: return (non-fatal on error)
GitHook->>GitHook: total_chars, body = _format_hits(hits)
GitHook-->>User: formatted response
Class diagram for new UPS rebuild_log helpers and related typesclassDiagram
class ContextRebuilder {
_append_rebuild_log_record(log_path, record, stderr)
_rebuild_log_disabled_via_env() bool
_empty_scores() dict
_belief_lock_level_for_log(belief) int
_build_rebuild_log_record(recent_turns, session_id, candidates, pack_summary) dict
record_user_prompt_submit_log(prompt, session_id, hits_pre_dedup, hits_post_dedup, log_path, enabled, stderr) void
}
class Hook {
user_prompt_submit(prompt, config, session_id, stderr) int
_dedup_by_content_hash(hits) list~Belief~
_emit_user_prompt_submit_rebuild_log(prompt, session_id, hits_pre_dedup, hits_post_dedup, stderr) void
db_path() Path
}
class RecentTurn {
+role: str
+text: str
+session_id: str
}
class Belief {
+id: str
+content: str
+lock_level: int
}
class RebuilderConfig {
+rebuild_log_enabled: bool
}
class Filesystem {
+rebuild_logs_dir: Path
}
Hook --> "*" Belief : uses hits_pre_dedup / hits_post_dedup
Hook --> Hook : _dedup_by_content_hash
Hook --> ContextRebuilder : calls record_user_prompt_submit_log
Hook --> RebuilderConfig : load_rebuilder_config
Hook --> Filesystem : _rebuild_log_dir_for_db, log_path
ContextRebuilder --> RecentTurn : constructs synthetic_turn
ContextRebuilder --> Belief : inspects id, content, lock_level
ContextRebuilder --> Filesystem : _append_rebuild_log_record
RebuilderConfig --> ContextRebuilder : rebuild_log_enabled
Flow diagram for record_user_prompt_submit_log decision and record buildingflowchart TD
A["record_user_prompt_submit_log<br/>(prompt, session_id,<br/>hits_pre_dedup, hits_post_dedup,<br/>log_path, enabled, stderr)"] --> B{enabled?}
B -- no --> Z1[return]
B -- yes --> C{"env opt-out<br/>_rebuild_log_disabled_via_env()?"}
C -- yes --> Z2[return]
C -- no --> D{log_path is None?}
D -- yes --> Z3[return]
D -- no --> E{"hits_pre_dedup empty?"}
E -- yes --> Z4[return]
E -- no --> F[build surviving_ids from hits_post_dedup]
F --> G[build survivor_by_hash from hits_post_dedup]
G --> H[init candidates list and n_dropped_by_dedup]
H --> I[for each belief in hits_pre_dedup with rank]
I --> J{belief.id in surviving_ids?}
J -- yes --> K["decision = packed<br/>reason = None"]
J -- no --> L["decision = dropped<br/>reason = content_hash_collision*<br/>increment n_dropped_by_dedup"]
K --> M[append candidate dict with empty scores and lock_level]
L --> M[append candidate dict with empty scores and lock_level]
M --> N{more beliefs?}
N -- yes --> I
N -- no --> O["compute pack_summary<br/>n_candidates, n_packed,<br/>n_dropped_by_floor=0,<br/>n_dropped_by_dedup,<br/>n_dropped_by_budget=0,<br/>total_chars_packed"]
O --> P["synthetic_turn = RecentTurn(role=user,<br/>text=prompt, session_id=session_id)"]
P --> Q["record = _build_rebuild_log_record(<br/>recent_turns=[synthetic_turn],<br/>session_id, candidates, pack_summary)"]
Q --> R["_append_rebuild_log_record(log_path, record, stderr)"]
R --> Z5[return]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 43 minutes and 49 seconds.Comment |
robotrocketscience
added
the
attn:review
|
This PR is now behind Auto-rebase was removed because the bot has no signing key; rebasing as the bot strips author signatures and the |
Owner
Author
|
[claim:review:Toug:2026-05-02T21:33:48Z] |
Owner
Author
|
[release:review:Toug:2026-05-02T21:34:14Z] |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
record_user_prompt_submit_log, you recompute a SHA1 digest ofb.contentboth when buildingsurvivor_by_hashand again in the pre-dedup loop; consider computing it once per belief (or reusingb.content_hashif it’s already a content-based digest) to avoid redundant work and reduce the risk of future divergence betweencontentand the hash source.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `record_user_prompt_submit_log`, you recompute a SHA1 digest of `b.content` both when building `survivor_by_hash` and again in the pre-dedup loop; consider computing it once per belief (or reusing `b.content_hash` if it’s already a content-based digest) to avoid redundant work and reduce the risk of future divergence between `content` and the hash source.
## Individual Comments
### Comment 1
<location path="src/aelfrice/hook.py" line_range="665-685" />
<code_context>
n_unique = len(unique_hashes)
n_l0 = sum(1 for h in hits if h.lock_level == LOCK_USER)
n_l1 = n_returned - n_l0
+ hits_pre_dedup = list(hits)
# AC6: optional dedup before formatting.
if config.collapse_duplicate_hashes:
hits = _dedup_by_content_hash(hits)
+ # #288 phase-1a extension: emit one rebuild_log row per
+ # UPS retrieval. Without this the high-frequency rebuild
+ # call site produces no log; phase-1b operator-week data
+ # collection depends on it.
+ _emit_user_prompt_submit_rebuild_log(
+ prompt=prompt,
+ session_id=session_id,
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid copying hits when dedup is disabled to reduce overhead on the hot path.
`hits_pre_dedup = list(hits)` runs even when `collapse_duplicate_hashes` is false, adding avoidable overhead on high-throughput paths. Consider only creating this list inside the `if config.collapse_duplicate_hashes:` block (passing `hits_post_dedup=hits` and `hits_pre_dedup=hits` when dedup is disabled), or let `_emit_user_prompt_submit_rebuild_log` treat `hits_post_dedup` as identical to `hits_pre_dedup` when one of them is `None`.
```suggestion
n_unique = len(unique_hashes)
n_l0 = sum(1 for h in hits if h.lock_level == LOCK_USER)
n_l1 = n_returned - n_l0
# AC6: optional dedup before formatting.
# Avoid copying hits on the hot path when dedup is disabled.
hits_pre_dedup = hits
if config.collapse_duplicate_hashes:
hits_pre_dedup = list(hits)
hits = _dedup_by_content_hash(hits)
# #288 phase-1a extension: emit one rebuild_log row per
# UPS retrieval. Without this the high-frequency rebuild
# call site produces no log; phase-1b operator-week data
# collection depends on it.
_emit_user_prompt_submit_rebuild_log(
prompt=prompt,
session_id=session_id,
hits_pre_dedup=hits_pre_dedup,
hits_post_dedup=hits,
stderr=serr,
)
# total_chars measured post-collapse (what is actually injected).
total_chars = sum(len(h.content) for h in hits)
body = _format_hits(hits)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
665
to
685
| n_unique = len(unique_hashes) | ||
| n_l0 = sum(1 for h in hits if h.lock_level == LOCK_USER) | ||
| n_l1 = n_returned - n_l0 | ||
| hits_pre_dedup = list(hits) | ||
| # AC6: optional dedup before formatting. | ||
| if config.collapse_duplicate_hashes: | ||
| hits = _dedup_by_content_hash(hits) | ||
| # #288 phase-1a extension: emit one rebuild_log row per | ||
| # UPS retrieval. Without this the high-frequency rebuild | ||
| # call site produces no log; phase-1b operator-week data | ||
| # collection depends on it. | ||
| _emit_user_prompt_submit_rebuild_log( | ||
| prompt=prompt, | ||
| session_id=session_id, | ||
| hits_pre_dedup=hits_pre_dedup, | ||
| hits_post_dedup=hits, | ||
| stderr=serr, | ||
| ) | ||
| # total_chars measured post-collapse (what is actually injected). | ||
| total_chars = sum(len(h.content) for h in hits) | ||
| body = _format_hits(hits) |
There was a problem hiding this comment.
suggestion (performance): Avoid copying hits when dedup is disabled to reduce overhead on the hot path.
hits_pre_dedup = list(hits) runs even when collapse_duplicate_hashes is false, adding avoidable overhead on high-throughput paths. Consider only creating this list inside the if config.collapse_duplicate_hashes: block (passing hits_post_dedup=hits and hits_pre_dedup=hits when dedup is disabled), or let _emit_user_prompt_submit_rebuild_log treat hits_post_dedup as identical to hits_pre_dedup when one of them is None.
Suggested change
| n_unique = len(unique_hashes) | |
| n_l0 = sum(1 for h in hits if h.lock_level == LOCK_USER) | |
| n_l1 = n_returned - n_l0 | |
| hits_pre_dedup = list(hits) | |
| # AC6: optional dedup before formatting. | |
| if config.collapse_duplicate_hashes: | |
| hits = _dedup_by_content_hash(hits) | |
| # #288 phase-1a extension: emit one rebuild_log row per | |
| # UPS retrieval. Without this the high-frequency rebuild | |
| # call site produces no log; phase-1b operator-week data | |
| # collection depends on it. | |
| _emit_user_prompt_submit_rebuild_log( | |
| prompt=prompt, | |
| session_id=session_id, | |
| hits_pre_dedup=hits_pre_dedup, | |
| hits_post_dedup=hits, | |
| stderr=serr, | |
| ) | |
| # total_chars measured post-collapse (what is actually injected). | |
| total_chars = sum(len(h.content) for h in hits) | |
| body = _format_hits(hits) | |
| n_unique = len(unique_hashes) | |
| n_l0 = sum(1 for h in hits if h.lock_level == LOCK_USER) | |
| n_l1 = n_returned - n_l0 | |
| # AC6: optional dedup before formatting. | |
| # Avoid copying hits on the hot path when dedup is disabled. | |
| hits_pre_dedup = hits | |
| if config.collapse_duplicate_hashes: | |
| hits_pre_dedup = list(hits) | |
| hits = _dedup_by_content_hash(hits) | |
| # #288 phase-1a extension: emit one rebuild_log row per | |
| # UPS retrieval. Without this the high-frequency rebuild | |
| # call site produces no log; phase-1b operator-week data | |
| # collection depends on it. | |
| _emit_user_prompt_submit_rebuild_log( | |
| prompt=prompt, | |
| session_id=session_id, | |
| hits_pre_dedup=hits_pre_dedup, | |
| hits_post_dedup=hits, | |
| stderr=serr, | |
| ) | |
| # total_chars measured post-collapse (what is actually injected). | |
| total_chars = sum(len(h.content) for h in hits) | |
| body = _format_hits(hits) |
Owner
Author
|
[claim:review:Gylf:2026-05-02T21:35:39Z] |
Owner
Author
|
Review (Gylf): diff is clean — discretion grep empty, both commits signed, CI green, no text conflict per Blocked only on rebase: Releasing review claim. |
Owner
Author
|
[release:review:Gylf:2026-05-02T21:36:55Z] |
Owner
Author
|
[claim:review:Setr:2026-05-02T21:56:10Z] |
Owner
Author
|
[claim:review:Kulili:2026-05-02T21:56:40Z] |
Owner
Author
|
[release:review:Kulili:2026-05-02T21:56:44Z] |
Owner
Author
|
[claim:review:Toug:2026-05-02T21:57:03Z] |
Owner
Author
|
Review (Setr): code LGTM, blocked on rebase. Substantive review
Blocker (not author's diff — branch state)
|
Owner
Author
|
[release:review:Setr:2026-05-02T21:57:41Z] |
Owner
Author
|
Review notes (Toug):
Blocker: branch is not fast-forward against Action requested: rebase onto current |
Owner
Author
|
[release:review:Toug:2026-05-02T21:57:59Z] |
Owner
Author
|
[claim:review:Setr:2026-05-02T22:06:48Z] |
Owner
Author
|
[claim:review:Gylf:2026-05-02T22:07:00Z] |
Owner
Author
|
Setr review pass — diff is clean (discretion grep empty), CI green on the latest staging-gate retry, but branch is non-FF against |
Owner
Author
|
[release:review:Setr:2026-05-02T22:07:56Z] |
robotrocketscience
removed
the
attn:review
Owner
Author
|
[release:review:Gylf:2026-05-02T22:08:41Z] |
Phase-1a wired the per-rebuild diagnostic log only into rebuild_v14, which fires from PreCompact. The high-frequency rebuild call site is user_prompt_submit, which calls search_for_prompt -> retrieve() directly and never reaches rebuild_v14. Result: rebuild_logs/ stayed empty under normal session load and phase-1b operator-week data collection could not begin. Add record_user_prompt_submit_log in context_rebuilder.py: synthesise a single RecentTurn from the prompt so the existing schema helpers apply unchanged, mark survivors of content-hash dedup as 'packed' and the dropped duplicates as 'dropped' with reason 'content_hash_collision_with:<survivor>'. Score fields are None per _empty_scores -- the BM25 / posterior decomposition is not exposed at this layer, locking the schema in phase-1a means phase-2 ranker work fills the same fields without a log-format migration. Wire from hook.user_prompt_submit() after dedup. Same 5MB cap, same AELFRICE_REBUILD_LOG=0 / [rebuild_log] enabled=false opt-outs as the PreCompact path. Fail-soft: any path-resolution or write error logs to stderr and never breaks the hook.
Update 'Where the write hook lives' to list both rebuild_v14 and user_prompt_submit, and call out that the original spec assumed all rebuild call sites went through rebuild_v14 -- which the UserPromptSubmit path does not. Records that bypass turned phase-1a into a no-op under normal session load and is what motivated the UPS wiring.
robotrocketscience
force-pushed
the
feat/issue-288-rebuild-log-ups
branch
from
May 2, 2026 22:16
8db9421 to
565980a
Compare
robotrocketscience
added
attn:review
Owner
Author
|
Rebased onto current main (was ~12 commits behind). Force-pushed ( — Setr |
Owner
Author
|
[claim:review:Gylf:2026-05-02T22:18:46Z] |
Owner
Author
|
[claim:review:Kulili:2026-05-02T22:18:49Z] |
Owner
Author
|
[claim:review:Toug:2026-05-02T22:18:53Z] |
Owner
Author
|
[release:review:Kulili:2026-05-02T22:18:54Z] |
Owner
Author
|
Merged via FF push (565980a). Schema parity verified, fail-soft wrapper good, opt-outs honored, all CI green. Reviewer: Gylf. |
Owner
Author
|
[release:review:Gylf:2026-05-02T22:19:34Z] |
Owner
Author
|
[release:review:Toug:2026-05-02T22:20:10Z] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase-1a of #288 wired the per-rebuild diagnostic log only into
rebuild_v14, which fires frompre_compact(). The high-frequency rebuild call site isuser_prompt_submit(), which callssearch_for_prompt()→retrieve()directly and never reachesrebuild_v14. Result: norebuild_logs/<session>.jsonlfiles appeared anywhere on disk under normal use, so phase-1b (operator-week of captured logs) was unreachable.Verified empty before this PR:
find ~ -path '*/aelfrice/rebuild_logs/*.jsonl'returned nothing despite weeks of UPS hook activity.Changes
context_rebuilder.py— newrecord_user_prompt_submit_loghelper. Synthesises a singleRecentTurnfrom the prompt, marks survivors of content-hash dedup aspacked, dropped duplicates asdroppedwithcontent_hash_collision_with:<survivor>. Score fields areNone(_empty_scores) — BM25 / posterior decomposition is not exposed at this layer; locking the schema in phase-1a means phase-2 ranker work fills the same fields without a log-format migration. Honors the sameAELFRICE_REBUILD_LOG=0env opt-out and[rebuild_log] enabled = falseTOML opt-out as the PreCompact path; reuses_append_rebuild_log_recordfor size-cap behaviour.hook.py— call the helper fromuser_prompt_submit()after dedup, before format. Path resolution mirrors_rebuild_and_format:<git-common-dir>/aelfrice/rebuild_logs/<session_id>.jsonl. Fail-soft: any path-resolution or write error logs to stderr and never breaks the hook.tests/test_rebuild_log_user_prompt_submit.py— 9 tests covering schema parity with PreCompact, dedup-drop visibility, env opt-out, TOML opt-out, missing-session-id no-op, in-memory store no-op, and lock-level passthrough.docs/rebuild_eval_harness.md— record both call sites; note that the original spec assumed all rebuild call sites went throughrebuild_v14, which the UPS path does not.Verification
uv run pytest tests/test_rebuild_log_user_prompt_submit.py— 9 passeduv run pytest tests/test_rebuild_log.py tests/test_hook_user_prompt_submit.py tests/test_hook_pre_compact.py tests/test_audit_rebuild_log.py— 43 passed (no regressions in the existing rebuild_log / UPS / PreCompact / audit suites)G(good signature) undergit log --format='%h %G? %s'.Test plan
scripts/audit_rebuild_log.pyfrom feat(scripts): rebuild_log audit script (#288 phase-1c) #350 with no changes).Closes #288 phase-1a follow-up. Phase-1b (operator-week wait) and phase-1c audit script are unchanged.
Summary by Sourcery
Instrument the high-frequency UserPromptSubmit retrieval path to emit rebuild_log entries consistent with the existing PreCompact logging schema, enabling phase-1b data collection.
New Features:
Enhancements:
Tests: