[R2 mono] wave-1 + scaling fixes + citable delivery — fork review rounds - #175
Conversation
Fork main CI has been red since the R1 merge: 4 trajectory-stack tests failed in clean CI while appearing to pass in dev environments — test_registered_tool_uses_the_product_compiler_path, test_code_owns_host_envelope_and_selector_proposes_semantics_only, test_named_facet_delta_is_selected_before_the_only_semantic_call, and test_pre_llm_hook_selective_compiler_uses_existing_auxiliary_seam_and_fails_open. Root cause (corrected from the earlier provider-gate hypothesis): these fixtures pin question_date="2026-07-20" but append their source messages WITHOUT a timestamp, so the store records observation at wall-clock "now". reasoning._ground_one enforces the as_of boundary (falling back to ingested_at when observed_at is absent), so once the clock passed 2026-07-20 every grounding call rejected with "source was observed after the question-date boundary" -> operand_grounding_failed -> status "fallback" / state "unknown" / the pre-llm hook failing open without the lcm-selective-evidence block. A time bomb, not a provider gate: with question_date moved past today the same paths compile fine in a clean pytest+numpy venv with no embedding/LLM provider at all. Fix: pin observed_at/timestamp to 2026-07-19T09:00Z (before the pinned question_date) on the appended sources, exactly the idiom the passing tests in the same files already use (e.g. test_exact_span_entity_date_value_unit_role_and_key_are_product_validated). This keeps the real code path exercised — grounding now validates the observation boundary instead of short-circuiting — and stays deterministic forever. No provider fixtures were needed (the selectors and retrieval in these tests are already deterministic fixtures) and no skip markers were added: nothing in these paths requires a provider. Verified in a CI-faithful venv (pytest+numpy only, agent stub, no provider env): the 4 tests pass both with VOYAGE_API_KEY/OPENAI_API_KEY/ ANTHROPIC_API_KEY unset and with dummy values set; full-suite failure set is byte-identical to the pre-fix baseline minus exactly these 4; ruff clean.
MessageStore.search()'s FTS-path SQL still SELECTed the old 12 message columns + rank + snippet, while _row_to_dict() grew to expect 15 message columns (..., ingested_at, observed_at, observed_at_source). The zip() truncation misread the FTS row's rank as ingested_at and snippet as observed_at, and silently dropped observed_at_source. The LIKE fallback path was unaffected. Add the three sidecar columns to the FTS SELECT and derive the rank/snippet column offset from _MESSAGE_SELECT_COLUMNS instead of a hardcoded 12, so a future column addition can't silently desync the two again. Regression: tests/test_runtime_time_contract.py::test_fts_search_preserves_time_contract_fields (fails on 0cb7f37, passes after this fix). Codex finding: stephenschoettler#436 discussion_r3641730742
In _persist_compiled_view(), claim_build() could succeed and then
publish_ready() could still raise (e.g. a rejected manifest). The
generic `except Exception:` handler only recorded
query_view_publish_failed in the result payload and never called
mark_failed() on the claimed token, so the row stayed status='building'
for the full 300s lease. An immediate, correct retry for the same
identity got query_view_build_in_progress ("busy") instead of
rebuilding.
Track the token outside the try block and call store.mark_failed(token,
str(exc)) in the except-Exception branch, mirroring the pattern already
used in adaptive_retrieval.py's _persist_view().
Regression: tests/test_evidence_compiler.py::test_persist_compiled_view_releases_lease_on_publish_failure
(fails on 0cb7f37 -- row stays 'building' and the retry reports busy;
passes after this fix).
Codex finding: stephenschoettler#436 discussion_r3641730749
classify_version_mismatch() treats any extra table not matching a known feature-family prefix as a genuinely-newer-build signature. The prefix list stopped at lcm_assertion, so a profile with an interim schema_version stamp whose only extras are the new query-view (lcm_query_*) or trajectory (lcm_trajectory_*) sidecar tables was misclassified as genuinely_newer instead of an interim stamp -- sending `/lcm doctor repair schema-stamp` down the refusal path even though these are opt-in, marker-gated features that don't bump the core schema. Add "lcm_query" and "lcm_trajectory" to _KNOWN_FEATURE_TABLE_PREFIXES. Both families are new in this PR with no prior deployed shape to drift from, so (like the existing prefix-only families before their own verifiers existed) they intentionally get no early-variant verifier yet -- that stays a separate, future addition if these tables ever need drop-and-rebuild remediation. Regression: tests/test_schema_stamp_remediation.py::test_classify_interim_stamp_with_query_view_and_trajectory_marker_tables (fails on 0cb7f37 -- classifies genuinely_newer; passes after this fix). Codex finding: stephenschoettler#436 discussion_r3641730753
LCM_COMPILE_EVIDENCE's public proposal schema required "operation", but _validate_proposal() rejects any proposal containing an "operation" key (_PROPOSAL_KEYS deliberately excludes it) because operation is a deterministic, code-derived value computed from the question in prepare_evidence_selector() and only ever surfaced to the selector as input (selector_request["operation"]) -- it is never a selector output. A caller who honestly followed the schema's required-field list got selector_schema_invalid on every call; the proposal-mode path was only reachable by silently violating the advertised contract. Remove "operation" from the proposal object's required list and properties -- the schema's additionalProperties:false now correctly rejects it too, matching the runtime exactly. Regression: tests/test_evidence_compiler.py::test_public_proposal_schema_does_not_require_code_derived_operation (fails on 0cb7f37 -- a schema-compliant proposal is rejected with selector_schema_invalid; passes after this fix). Codex finding: stephenschoettler#436 discussion_r3641730756
requirements_digest() hashed only slot_id and minimum_refs, omitting the requirement description that defines what evidence a slot is supposed to satisfy. Two unrelated questions sharing a QueryViewIdentity (same subject/predicate/scope, plausible for a coarse-grained intent bucket) and the same slot_id/minimum_refs -- but a different description -- produced an identical digest, so start() treated a cached view built for one meaning as a hit for the other and pre-filled the new requirement with unrelated cached evidence. Confirmed end to end: a "Who is the CFO of Acme?" retrieval was served the CEO's cached citation. Fold the (already whitespace-normalized) description into the hashed identity alongside slot_id/minimum_refs. This intentionally narrows one existing warm-reuse case: test_exact_slot_closure_compute_finish_and_warm_reuse previously varied both the literal question text AND the requirement description on its "warm" call and still asserted a hit. That was exercising the exact gap being closed here. Updated it to keep the description constant while still rewording the question, which is what that test is actually meant to demonstrate (warm reuse survives rephrasing, not a description change); the description-varies case now has its own dedicated coverage. Regressions: - tests/test_adaptive_retrieval.py::test_requirements_digest_distinguishes_descriptions - tests/test_adaptive_retrieval.py::test_cached_view_is_not_reused_across_different_requirement_descriptions (both fail on 0cb7f37 -- digests collide and the CFO retrieval reuses the CEO's evidence; pass after this fix). Codex finding: stephenschoettler#436 discussion_r3641730764
Add lexical_floor:int=0 kwarg to TrajectoryStore.query(). When >0, reserve K nucleus slots for the top pure-BM25 states (via _select_with_floor) before filling the rest from the fused order, honouring the same 5/trajectory cap. Default 0 reproduces the historical fused-only selection byte-for-byte (candidate-composition repair for the semantic-magnet SOURCE_MISS bucket).
Add arm_quota:tuple|None=None kwarg to TrajectoryStore.query(). When set, _merge_arms round-robins a pure-BM25 arm and the semantic/fused arm into the nucleus by a q_lex:q_sem quota (dedup + backfill, arm order as tie-break), strictly generalising Policy A. Default None reproduces the historical selection byte-for-byte.
Component instrument for the candidate-composition repair. Injects the frozen H1.3 semantic source ranks into TrajectoryStore.query() (no provider/model call), replays from read-only frozen DB copies, and: - GOLDEN GATE: defaults reproduce recorded delivered_evidence_refs byte-for-byte; - RECOVERY: vanished genuine-loss refs re-admitted (by h2 bucket); - CEILING: vanished refs absent from global_rows top-128 (un-recoverable at seam); - PRESERVATION: stable-correct delivered refs dropped (ref-level + source-level); - latency p95 over a 50-query replay. Emits the full A/K + D/quota sweep table + JSON; does not pick a shipping knob.
Reproduce the semantic-magnet SOURCE_MISS displacement synthetically and assert: default byte-compat (magnet displacement preserved), Policy A/D re-admit the displaced lexical winner, _merge_arms dedup+backfill order, and both policies no-op without semantic ranks.
… from scored path per #127 (2nd powered fail +7 vs ≥+8); preserved default-off; golden 451/451 byte-identical
Promote adjacency from a delivery-only ±1 backfill to a POOL-stage expansion: for every lexical seed hit in the candidate pool, the states at sequence_ordinal ±1..adjacency_radius within the same source are admitted as a QUOTA-CAPPED ADDITIVE arm through the existing _merge_arms machinery (q_lex=len(rows), q_sem=adjacency_quota), giving a non-lexical recall path for states that carry no query term of their own. Controls (anti-magnet, per H5-design-options option (b) + SPEC-H5b): - strictly additive tail: expanded neighbors earn NO semantic boost and no BM25 rank; nucleus selection -- and therefore delivery -- changes only when the ranked pool cannot fill the nucleus (gate (ii) additive proof); - deterministic distance-major / seed-pool-rank / ordinal arm order; - 5-per-trajectory diversity cap at selection untouched; - adjacency_radius=0 / adjacency_quota=0 defaults skip the path entirely (current bytes; telemetry key present only when active); - batched row-value IN lookups on the (source_id, sequence_ordinal) index. tests: synthetic invisible-neighbor corpus -- default byte-identity incl. half-open knobs, pool entry, radius bound + arm order, quota cap, pool dedup, delivery-unchanged on a full (magnet) pool, selection cap, tail-only placement. Full suite: failure set identical to base 65f679b under the same invocation (pre-existing env-dependent failures only).
…135) The arm probe is now index-only (state_id/source_id/sequence_ordinal); the candidate-shaped full rows (large state text) are fetched solely for the <=quota admitted states. Measured on the frozen enterprise corpus: ~400-pair full-row probe cost ~63ms/chunk vs 0.07ms light probe + ~5ms admitted fetch -- keeps the knob inside the p95 +10% latency gate (iv). Behavior unchanged (41 trajectory-suite tests green; golden 451/451 re-verified).
Extends the H3.1 composition replay instrument: same frozen assets, same injected semantic ranks, same mandatory golden gate (451/451 before any sweep). Sweeps adjacency radius x quota under TWO compositions (base + quarantined hybrid lexical_floor=1/arm_quota=(7,4)) and reports per knob: pool-entry recovery over the 30 verified H32 targets (h5-targets.json; EXACT pool membership, not 64-truncated telemetry), Delivered-Recall@16 vs the same composition's no-adjacency delivery, the 8 loss-ids' delivered-set anti-filler check, preservation disturbance over the 154 stable-correct questions, and paired-pass p95 latency (external-drive I/O variance makes unpaired reads incomparable). Also emits the zero/weak-seed split of the 30. The component gate is FROZEN: this instrument does not pick the knob.
#142) Add an additive per-STATE embedding space (the existing lcm_trajectory_embeddings is per-SOURCE / one coarse vector per trajectory, which cannot surface a lexically-invisible answer state). New idempotent tables lcm_trajectory_state_embedding{_profiles,s} created lazily via _ensure_state_semantic_schema (mirrors _ensure_semantic_schema; no existing-table change), a resumable build_state_semantic_index backfill (32-item/72K-token packing, chunked mean-pool path for over-cap states, skip-embedded resume, progress ledger callback), and a default-off state_semantic_quota query arm that admits the query's semantic nearest-neighbour states as a STRICTLY ADDITIVE _merge_arms tail (no boost, 5/traj cap intact, telemetry only when active). Defaults reproduce current bytes. 10 tests mirror the 8 adjacency tests + backfill resume/chunk/inert cases.
state_embedding_backfill.py: resumable metered CLI over a working-copy lcm.db (32-item/72K packing via build_state_semantic_index, JSONL spend ledger, projected-cost cost-cap abort, 50%% checkpoint). h5_state_semantic_replay.py: sibling of the adjacency sweep -- opens the BACKFILLED copies with a cached real Voyage query provider (source ranks still injected -> golden byte-identical), sweeps state_semantic_quota and reports the four frozen #142 gate metrics (pool-entry/30, delivered-recall@16, loss8 preservation, 154-set preservation, p95). Query embeds cached off the timed path so the latency delta isolates the arm's added cost.
Bootstrap the hermes_lcm package before constructing the query provider (the provider import needs it), and disable the interactive per-minute call-rate guard on the sweep's query embedder (the 451-query warm pass is the bulk pattern for_backfill exempts, and tripped ProviderRateLimited otherwise). Sweep runs clean end-to-end: golden 451/451, quotas 4-64.
…trieval knobs (#143) Knob G (HERMES_LCM_ANTIBOILERPLATE, default-off): inside C1's per-trajectory MMR survivor selection, penalize a candidate by its mean lexical similarity to the other pooled states of its own trajectory and reward query-term density, so a trajectory's seats go to query-relevant states not repeated task headers. Knob H (HERMES_LCM_TITLE_BOOST, default-off): at the lexical candidate stage, stable-reorder states whose title/heading/field-label text contains an exact (case/punct-normalized) 2-4 gram question phrase ahead of same-band peers. Both deterministic, no model calls, omitted from query() when off. Golden 451/451 byte-identical on base and W3a stores; full-suite failure-set parity (+3 new passing tests, identical 60 pre-existing bad node ids); ruff clean.
FTS5 barewords accept only alphanumerics, so a natural-language question (`?`, apostrophes, commas, `&`, `$`) was a syntax error at MATCH. The query fell through to the LIKE full-scan, which blows recall_query_timeout_s at scale and returns EMPTY -- 100% of queries at 8,000+ sessions (FINDING-F31 §3). The sanitization belonged in the product, not in a benchmark harness that happened to know the trick. sanitize_fts5_query now maps every non-alphanumeric character outside a balanced phrase quote to a separator (the transform the Phase 1A A3 arm used), matching how the default unicode61 tokenizer splits the INDEXED text -- so no term is lost. LIKE stays the fallback for what FTS cannot express (CJK/emoji/compound tokens) and for a query with nothing left after sanitization; that path now scores punctuation-only queries on the raw text so its literal substring behavior is unchanged.
…ndow (#167) recall_scan_rows was a HARD bound on the brute-force scan, so at 185,175 vectors lcm_recall scored only the 25,000 most-recent and 86% of memory was invisible to semantic retrieval. All-gold recall@25 collapsed 0.82 -> 0.00 across the ladder exactly as gold content aged out of the window, and the no-deadline arm reproduced it: coverage, not time (FINDING-F31 §2). The "all conversations, all time" promise was false at scale. The scan now covers the WHOLE corpus in batches with a running top-k across them. recall_scan_rows becomes the BATCH SIZE, so it bounds vectors resident per batch (peak memory) instead of the corpus reached; the arithmetic is trivial either way (185k x 384-dim is ~0.14 GFLOPs). No ANN index here -- that is deferred until profiling shows the full scan too slow. lcm_grep is untouched: full_scan is off by default, and a single batch of bounded_scan_rows candidates is byte-for-byte the previous behavior. The degraded/degraded_reason machinery is preserved and now means what it says: coverage degrades to 'bounded' only when an explicit bound truncates the scan -- recall_scan_max_rows (0 = unlimited, for a pathological corpus) or recall_scan_budget_s (0 = no early stop). Both default to no early stop, so a default recall discloses nothing because nothing is hidden.
… the raw one
requires_like_fallback() tested the RAW query, so a compound token still forced
the full-table LIKE scan even though sanitization turns it into ordinary terms
the index answers: requires_like_fallback("art-related") was True while
sanitize_fts5_query("art-related") == "art related". Six of the fifty fixed
Phase 1B questions carry a hyphen, so 12% of the rerun would have taken the
same full-scan path this branch exists to remove.
The predicate now asks what sanitization LOSES, not what the FTS5 query grammar
cannot spell. Compounds ride the index. The genuine losses still route to LIKE
and are tested against the RAW query, because sanitization is exactly what
removes them: unicode61 does not segment CJK and drops emoji from the index, and
a query that sanitizes to nothing has no term left to match.
Four existing tests used a hyphen purely as their LIKE trigger; they now trigger
on an emoji and keep the hyphen in the raw query, so the SQL-limit, conversational
-vs-tool ranking, and risky-ASCII repetition collapse each still assert exactly
what they did before.
The 4-entry matrix cache and a sequential batch sweep are actively hostile to each other. The 185,175-vector corpus needs eight 25k batches; after a sweep the cache holds batches 4-7, and the next sweep starts at batch 0 and evicts each entry before reaching it — zero hits on every measured repetition, pure reload cost. Worse, it RETAINED four float32 matrices (153.6MB on the chunk arm alone), which is exactly the peak-memory bound the batching was introduced to provide. A sweep that needs more than one batch now loads each batch straight through (_load_matrix / _load_chunk_matrix, no cache interaction), so peak memory really is one batch. A single-batch scan — every lcm_grep call, and any corpus under the batch size — takes the cached path unchanged, so the pooled-store warm path keeps its F2-matrix-cache behavior byte for byte. Cache semantics for the Phase 1B curve, so F34 can read it: rungs whose corpus fits one batch are WARM across repetitions; any rung above the batch size is DETERMINISTICALLY COLD. At the default 25k batch that is the 19,829-session rung (185k vectors) cold and the rest warm.
…path too A fully-synced binary-prescreen identity returned coverage='full_approx' before _scan_limits() was ever consulted, so recall_scan_max_rows and recall_scan_budget_s were silently ignored on those profiles — on both the summary and chunk arms. An operator capping a pathological corpus got an uncapped scan and no disclosure that the cap did nothing. The two-stage path can honor neither knob: it is one matmul over the whole binary mirror, with no candidate window to cap and no batch boundary to stop at. So a requested bound now declines that path and takes the exact batched scan, which enforces the bound and reports the bounded coverage with scanned/total. Both knobs default to 0, so an unbounded recall keeps the two-stage path and its full_approx disclosure byte for byte.
Sharing the FTS term form with the LIKE fallback deleted the very characters that fallback exists to find. `launch 🚀` routes to LIKE precisely BECAUSE unicode61 does not index the emoji — and then sanitized to `launch`, searching only %launch%. A row containing just 🚀 stopped matching. Regression against the base sanitizer, on both message and summary search. The LIKE path now has its own weaker sanitizer (sanitize_like_query), which strips only genuine FTS5 query-syntax operators — the punctuation a user typed FOR the index — and preserves every other character, because a character the index cannot spell is still a character a substring match can find. That is the pre-Phase-1B behavior, restored for the one path that wants it, while MATCH keeps the strict term form. Both walk the same quote-preserving scanner. This also retires the `or query` empty-sanitize workaround: sanitize_like_query never empties a punctuation-only query in the first place.
…query Uppercase AND/OR/NOT/NEAR survived sanitization, so a raw question silently ACQUIRED boolean semantics it never asked for. `Portland, OR hotel` sanitized to `Portland OR hotel` and broadened into a disjunction; a leading `NOT ready` was an outright FTS syntax error that dumped the query onto the LIKE full-scan — the exact failure mode this branch exists to close. Raw and deliberate queries share one unmarked entry point, so the raw reading has to be the safe one: bare operators outside quoted phrases are lowercased, which turns them back into ordinary barewords (FTS5 operators are operators only in uppercase). An explicit "NEAR" phrase is already a literal and is untouched. The two hyphenated-operator tests asserted the OLD contract, where a stray OR broadened the query. They now assert the new one — the messy query resolves conjunctively with no syntax error and no full-table fallback — plus a positive check that the hyphenated compounds themselves reach the target through the index, which is what finding 1 bought.
str.isalnum() is not unicode61's token boundary. A combining mark is not alphanumeric, so a decomposed "naïve" (nai + U+0308) sanitized to "nai ve" while unicode61 folds and indexes the word as "naive" — zero rows for a query that matched raw. That directly contradicted the tokenizer-parity invariant this sanitizer is justified by. sanitize_fts5_query now composes to NFC first, so a decomposed accent is the one alphanumeric character the tokenizer folds, and any mark that does NOT compose (a virama, stacked diacritics) is kept inside its token rather than treated as a separator. The LIKE path deliberately does not normalize: it is a literal substring match against stored bytes and must not re-spell the query.
…ad of guessing Neutralizing bare operators broke the ONE in-repo caller that composes FTS5 syntax on purpose: benchmarking/longmemeval.build_fts_query joins its barewords with OR, and lowercasing them turned the harness's disjunction into a conjunction that also required the literal word "or" — the FTS arm's recall@10 fell from 1.0 to 0.0. That is the Phase 1B instrument itself, so leaving it broken would have silently gutted the rerun this branch exists to enable. The reviewer named the real defect: raw and deliberate queries shared one unmarked mode. So mark it. search() and sanitize_fts5_query() take an explicit allow_operators flag, default False — raw prose keeps the safe reading from the previous commit — and the harness, which knows it wrote operator syntax, opts in. Caught by tests/test_longmemeval_harness.py, which is why the arm assertion is worth keeping.
…d scan cache=False kept NEW batches out of the LRU but did nothing about what was already in it, and the retrieval-core pool keeps a store alive for the whole process. The delta-review probe measured cache_before=4 / cache_after=4 across a two-batch scan: four matrices warmed by earlier calls — ~192MB of summary float32 at a 25k batch, before the separate chunk cache — sitting resident alongside the streamed batch. The one-batch peak claim was false in exactly the long-lived-process case that motivates it. A multi-batch sweep now releases every cached matrix before allocating its first batch. All four caches go: both float32 LRUs and both binary-prescreen caches, since all of them are retained scan state and the binary ones are additive to the same peak. Same probe after the fix: cache_before=4, cache_after=0, and 0 resident at EVERY batch allocation. Safe on a shared pooled store: clearing only drops dict entries. A caller mid-scan holds its own reference to the tuple it was handed, so its matrix stays alive until it returns and is freed normally after. Single-batch scans are untouched and keep their warm-cache behavior, pinned by the existing test.
evaOS review status: completedPR: #175 - [R2 mono] wave-1 + scaling fixes + citable delivery — fork review rounds evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #175 Review URL: #175 (review) |
![evaos-code-review-bot[bot]](https://avatars.githubusercontent.com/in/4184532?s=60&v=4){kind=small}
There was a problem hiding this comment.
Walkthrough
PR: #175 - [R2 mono] wave-1 + scaling fixes + citable delivery — fork review rounds
Head: 060a0df5824a741b9c7cfde5f6c9b5acffb549e2 into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
.github/ISSUE_TEMPLATE/bug_report.yml |
modified | +1/-1 | Changed file | Low |
CHANGELOG.md |
modified | +8/-0 | Documentation | Low |
DELTA-REVIEW-PACKET.md |
added | +24/-0 | Documentation | Low |
FINDINGS-VERDICTS-R2.md |
added | +19/-0 | Documentation | Low |
FINDINGS-VERDICTS-R3.md |
added | +21/-0 | Documentation | Low |
FINDINGS-VERDICTS-R4.md |
added | +17/-0 | Documentation | Low |
FINDINGS-VERDICTS-R4B.md |
added | +19/-0 | Documentation | Low |
FINDINGS-VERDICTS-R6.md |
added | +8/-0 | Documentation | Low |
FINDINGS-VERDICTS.md |
added | +22/-0 | Documentation | Low |
README.md |
modified | +1/-1 | Documentation | Low |
REGRESSION-REPORT.md |
added | +56/-0 | Documentation | Low |
REVIEW-PACKET.md |
added | +47/-0 | Documentation | Low |
adaptive_retrieval.py |
modified | +47/-7 | Changed file | Low |
benchmarking/h3_composition_replay.py |
added | +462/-0 | Changed file | Elevated: large change |
benchmarking/h5_recall_replay.py |
added | +425/-0 | Changed file | Elevated: large change |
benchmarking/h5_state_semantic_replay.py |
added | +291/-0 | Changed file | Elevated: large change |
benchmarking/longmemeval.py |
modified | +5/-1 | Changed file | Low |
benchmarking/state_embedding_backfill.py |
added | +266/-0 | Changed file | Elevated: large change |
benchmarking/stress.py |
modified | +45/-13 | Changed file | Low |
benchmarking/w3b_diversity_replay.py |
added | +456/-0 | Changed file | Elevated: large change |
benchmarking/w3b_it4_replay.py |
added | +188/-0 | Changed file | Low |
config.py |
modified | +32/-5 | Configuration | Low |
dag.py |
modified | +52/-2 | Changed file | Low |
db_bootstrap.py |
modified | +32/-2 | Changed file | Low |
docs/operator-guide.md |
modified | +1/-1 | Documentation | Low |
32 additional changed files omitted from this walkthrough.
Review Signal
No validated inline findings.
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
No finding categories.
Validation and Proof
No required validation recommendation selected; rely on existing GitHub checks and human review.
Proof status: not_applicable - No required behavior proof selected for this changed surface.
Profile validation hints: Prefer correctness, security, data-loss, release, and regression findings over style-only feedback.
Profile proof expectations: Look for focused validation, rollback notes, and evidence appropriate to the changed surface.
Related Context
Related issues/PRs: #167, #168, #164, #174, #171, #172.
Suggested labels: docs, tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Review status comment (sticky_status)
- Path instructions: none
- Label suggestions: none
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
trajectory_store.py (1)
3233-3301: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winSkip the source-semantic embed for termless queries
scoped_rowsis always[]whenexpressionis empty, so_semantic_source_ranks(query)can only affect telemetry here while still billing tokens under an active embedding provider. Gate it behindif expression:and keep the state-semantic path unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@trajectory_store.py` around lines 3233 - 3301, Update the semantic source-ranking block around _semantic_source_ranks so it is invoked only when expression is non-empty; use empty semantic ranks and skip related embedding/telemetry work for termless queries. Preserve the existing state-semantic path and the current scoped_rows behavior for queries with an expression.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@adaptive_retrieval.py`:
- Around line 176-183: Update EvidenceRequirement.parse() validation to reject
descriptions containing surrogate code points, including lone surrogates, before
constructing the requirement; preserve the existing length and empty-description
checks, and add a regression test covering the rejected input.
In `@tests/test_h5_state_semantic_replay.py`:
- Around line 8-19: Add an explicit assertion in
test_output_parent_exists_before_provider_work that CachedVoyageQueryProvider is
not constructed, such as monkeypatching its constructor with a failing spy
before invoking h5_state_semantic_replay.main. Preserve the existing exit-code,
parent-directory, and absent-output assertions.
---
Outside diff comments:
In `@trajectory_store.py`:
- Around line 3233-3301: Update the semantic source-ranking block around
_semantic_source_ranks so it is invoked only when expression is non-empty; use
empty semantic ranks and skip related embedding/telemetry work for termless
queries. Preserve the existing state-semantic path and the current scoped_rows
behavior for queries with an expression.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1f634614-3d43-44d6-8e6d-79baa24a9897
📒 Files selected for processing (7)
FINDINGS-VERDICTS-R6.mdadaptive_retrieval.pybenchmarking/h5_state_semantic_replay.pytests/test_adaptive_retrieval.pytests/test_h5_state_semantic_replay.pytests/test_trajectory_state_semantic_expansion.pytrajectory_store.py
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
- GitHub Check: test (3.14)
- GitHub Check: test (3.12)
🧰 Additional context used
🪛 ast-grep (0.45.0)
adaptive_retrieval.py
[warning] 47-49: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.compile(
rf"^[a-z][a-z0-9_.-]{{0,{MAX_REQUIREMENT_SLOT_ID_CHARS - 1}}}$"
)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
[info] 110-120: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"slot_id": "s" * MAX_REQUIREMENT_SLOT_ID_CHARS,
"description": "\0" * MAX_REQUIREMENT_DESCRIPTION_CHARS,
"minimum_refs": MAX_CANDIDATE_REFS,
},
ensure_ascii=False,
sort_keys=True,
separators=(",", ":"),
allow_nan=False,
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🔇 Additional comments (6)
FINDINGS-VERDICTS-R6.md (1)
1-8: LGTM!tests/test_adaptive_retrieval.py (1)
14-17: LGTM!Also applies to: 244-254, 275-356
adaptive_retrieval.py (1)
39-50: LGTM!Also applies to: 107-130
trajectory_store.py (1)
1808-1827: Spend ledgered before failure-prone persistence — matches Round-6 intent.Moving
stats["provider_calls"]/billed_tokensaccounting and_emit_progress()to immediately afterembed_documents()(before the vector-count check and_persist()) correctly ensures the ledger reflects real spend even if persistence later fails. Verified againsttest_normal_batch_persist_failure_still_ledgers_spend.tests/test_trajectory_state_semantic_expansion.py (1)
19-19: LGTM!Also applies to: 263-281, 717-760
benchmarking/h5_state_semantic_replay.py (1)
149-149: LGTM!
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 060a0df582
ℹ️ 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".
| if batch_docs and ( | ||
| len(batch_docs) >= batch_max_items | ||
| or batch_tokens + tokens > batch_token_budget | ||
| ): | ||
| _flush_normal() | ||
| batch_ids.append(state_id) |
There was a problem hiding this comment.
When a caller configures batch_token_budget below document_token_budget, a normal document can be larger than the batch budget, but this condition only flushes an already-populated batch and then appends the oversized document unchanged. The subsequent embed_documents() request therefore exceeds the explicitly configured request cap and may be rejected by the provider; classify or split documents using the smaller of the two budgets, or reject this parameter combination.
Useful? React with 👍 / 👎.
| "SELECT state_id FROM lcm_trajectory_state_embeddings " | ||
| "WHERE profile_digest = ?", | ||
| (profile_digest,), |
There was a problem hiding this comment.
When resume=False forces a rebuild using the same provider/model/profile digest as an existing complete index, the old rows are left in place while batches overwrite them. If that rebuild is interrupted or intentionally stopped by the cost cap, a subsequent default resume selects every old and newly written row here as already embedded, performs zero remaining provider work, passes the row-count completeness check, and reactivates a stale or mixed index. Delete or generation-scope the same-profile rows when starting a forced rebuild so a later resume can distinguish unfinished work.
Useful? React with 👍 / 👎.
| top = ctx.global_top_state_ids(qid, 128) | ||
| for ref in info["vanished"]: | ||
| state_id = ctx.ref_to_state_id(qid, ref) | ||
| per_ref.setdefault(qid, {})[ref] = state_id is not None and state_id in top |
There was a problem hiding this comment.
For an arm_quota sweep cell, Policy D selects its semantic arm from the fused rows pool, which includes FTS hits scoped to the injected semantic-top trajectories, but this ceiling labels a vanished reference recoverable only when it is in the global FTS top 128. A target present only in the scoped pool is therefore excluded from the denominator and evaluate_knob() skips it even if Policy D actually delivers it, under-reporting recovery and potentially invalidating the composition experiment's conclusion. Compute a policy-appropriate ceiling from the same fused candidate pool used by the arm.
Useful? React with 👍 / 👎.
|
@coderabbitai review |
✅ Action performedReview finished.
|
- trajectory: forced same-profile rebuild (resume=False) clears prior rows before refill — no stale vectors from an earlier fill survive - trajectory: document chunk-routing honors the SMALLER of document/ request token budgets - trajectory: termless queries skip the source-semantic embed (scoped rows are structurally empty there); state-semantic seeding intact - adaptive_retrieval: requirement descriptions reject surrogate code points before canonical UTF-8 digesting - h3 replay: Policy D arm-quota cells measure recoverability from their fused candidate pool (scoped semantic rows included) - h5 test: out-dir contract now proves provider construction is skipped All items quota-gated / bench-side / test-side — no delivery-path change. Author: codex sol-high (R7); review: orchestrator. Full-suite exact-name parity; 6 focused regressions green. STOPPING RULE (declared): this closes the in-train fix cycle. Subsequent non-delivery-path findings are filed as next-train issues.
Round 7 response — 6/6 closed at
|
evaOS review status: completedPR: #175 - [R2 mono] wave-1 + scaling fixes + citable delivery — fork review rounds evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #175 Review URL: #175 (review) |
There was a problem hiding this comment.
Walkthrough
PR: #175 - [R2 mono] wave-1 + scaling fixes + citable delivery — fork review rounds
Head: 93a3adefcab8b0a3cf7bda3196586694f1fea71f into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
.github/ISSUE_TEMPLATE/bug_report.yml |
modified | +1/-1 | Changed file | Low |
CHANGELOG.md |
modified | +8/-0 | Documentation | Low |
DELTA-REVIEW-PACKET.md |
added | +24/-0 | Documentation | Low |
FINDINGS-VERDICTS-R2.md |
added | +19/-0 | Documentation | Low |
FINDINGS-VERDICTS-R3.md |
added | +21/-0 | Documentation | Low |
FINDINGS-VERDICTS-R4.md |
added | +17/-0 | Documentation | Low |
FINDINGS-VERDICTS-R4B.md |
added | +19/-0 | Documentation | Low |
FINDINGS-VERDICTS-R6.md |
added | +8/-0 | Documentation | Low |
FINDINGS-VERDICTS-R7.md |
added | +13/-0 | Documentation | Low |
FINDINGS-VERDICTS.md |
added | +22/-0 | Documentation | Low |
README.md |
modified | +1/-1 | Documentation | Low |
REGRESSION-REPORT.md |
added | +56/-0 | Documentation | Low |
REVIEW-PACKET.md |
added | +47/-0 | Documentation | Low |
adaptive_retrieval.py |
modified | +51/-7 | Changed file | Low |
benchmarking/h3_composition_replay.py |
added | +493/-0 | Changed file | Elevated: large change |
benchmarking/h5_recall_replay.py |
added | +425/-0 | Changed file | Elevated: large change |
benchmarking/h5_state_semantic_replay.py |
added | +291/-0 | Changed file | Elevated: large change |
benchmarking/longmemeval.py |
modified | +5/-1 | Changed file | Low |
benchmarking/state_embedding_backfill.py |
added | +266/-0 | Changed file | Elevated: large change |
benchmarking/stress.py |
modified | +45/-13 | Changed file | Low |
benchmarking/w3b_diversity_replay.py |
added | +456/-0 | Changed file | Elevated: large change |
benchmarking/w3b_it4_replay.py |
added | +188/-0 | Changed file | Low |
config.py |
modified | +32/-5 | Configuration | Low |
dag.py |
modified | +52/-2 | Changed file | Low |
db_bootstrap.py |
modified | +32/-2 | Changed file | Low |
34 additional changed files omitted from this walkthrough.
Review Signal
No validated inline findings.
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
No finding categories.
Validation and Proof
No required validation recommendation selected; rely on existing GitHub checks and human review.
Proof status: not_applicable - No required behavior proof selected for this changed surface.
Profile validation hints: Prefer correctness, security, data-loss, release, and regression findings over style-only feedback.
Profile proof expectations: Look for focused validation, rollback notes, and evidence appropriate to the changed surface.
Related Context
Related issues/PRs: #167, #168, #164, #174, #171, #172.
Suggested labels: docs, tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Review status comment (sticky_status)
- Path instructions: none
- Label suggestions: none
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
benchmarking/h3_composition_replay.py (1)
190-224: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
deliver()andfused_candidate_state_ids()duplicate the query-and-inject boilerplate.Both methods build
store.injectedand callstore.query(...)with identical kwargs, only diverging in what they extract from telemetry afterward. Extract a private_run(qid, **kwargs)that performs the injection + query call once, and have both public methods pull their respective telemetry field from the result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarking/h3_composition_replay.py` around lines 190 - 224, Extract the shared injection and query logic from deliver() and fused_candidate_state_ids() into a private _run(qid, **kwargs) helper that returns the query telemetry. Update both public methods to call _run and extract only their respective telemetry fields, preserving the existing query arguments and result conversions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarking/h3_composition_replay.py`:
- Around line 210-224: The fused candidate lookup in fused_candidate_state_ids
currently reads the truncated state_candidate_pool; update the query telemetry
usage to retrieve the uncapped state_candidate_pool_ids list, following the
existing diversity_cap pattern. Preserve integer state-ID conversion and return
the full candidate-limit pool so measure_ceiling can evaluate every arm_quota
cell.
---
Outside diff comments:
In `@benchmarking/h3_composition_replay.py`:
- Around line 190-224: Extract the shared injection and query logic from
deliver() and fused_candidate_state_ids() into a private _run(qid, **kwargs)
helper that returns the query telemetry. Update both public methods to call _run
and extract only their respective telemetry fields, preserving the existing
query arguments and result conversions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 653540e6-c0ad-4e37-a381-680bbc87dfe6
📒 Files selected for processing (8)
FINDINGS-VERDICTS-R7.mdadaptive_retrieval.pybenchmarking/h3_composition_replay.pytests/test_adaptive_retrieval.pytests/test_h3_composition_replay.pytests/test_h5_state_semantic_replay.pytests/test_trajectory_state_semantic_expansion.pytrajectory_store.py
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
- GitHub Check: test (3.14)
🧰 Additional context used
🪛 markdownlint-cli2 (0.23.1)
FINDINGS-VERDICTS-R7.md
[warning] 1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (8)
trajectory_store.py (1)
1727-1737: LGTM!Also applies to: 1771-1785, 1852-1854, 3273-3308
tests/test_trajectory_state_semantic_expansion.py (1)
263-283: LGTM!Also applies to: 566-611, 723-753
tests/test_h3_composition_replay.py (1)
1-39: LGTM!adaptive_retrieval.py (2)
176-187: LGTM!
107-130: 🎯 Functional CorrectnessNo issue:
_REQUIREMENTS_DIGEST_MAX_CHARSis derived from the max canonical item length, includingdescription, and covers the 12-item payload with one-item headroom.tests/test_adaptive_retrieval.py (1)
288-295: LGTM!tests/test_h5_state_semantic_replay.py (1)
8-28: LGTM!FINDINGS-VERDICTS-R7.md (1)
1-14: LGTM!
| def fused_candidate_state_ids( | ||
| self, qid: str, **kwargs: Any | ||
| ) -> set[int]: | ||
| domain, text = self.questions[qid] | ||
| store = self.stores[domain] | ||
| store.injected = self._injected_ranks(qid) | ||
| store.query( | ||
| text, candidate_limit=128, limit=16, image_limit=0, | ||
| include_adjacent=True, text_char_limit=2000, **kwargs, | ||
| ) | ||
| return { | ||
| int(row["state_id"]) | ||
| for row in store.last_query_telemetry()["state_candidate_pool"] | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and inspect the surrounding code.
git ls-files | rg '(^|/)(benchmarking/h3_composition_replay.py|trajectory_store\.py|.*diversity_cap.*|.*trajectory.*)$'
echo '--- benchmarking/h3_composition_replay.py (around fused_candidate_state_ids and measure_ceiling) ---'
sed -n '180,340p' benchmarking/h3_composition_replay.py
echo '--- trajectory_store.py (search for state_candidate_pool / survivor_state_ids / telemetry) ---'
rg -n -A8 -B8 'state_candidate_pool|survivor_state_ids|last_query_telemetry|candidate_limit|include_adjacent' trajectory_store.pyRepository: 100yenadmin/hermes-lcm
Length of output: 22497
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- trajectory_store.py (around candidate selection and telemetry) ---'
sed -n '3440,3665p' trajectory_store.py
echo '--- tests touching diversity_cap / candidate telemetry ---'
rg -n -A6 -B6 'diversity_cap|survivor_state_ids|state_candidate_pool|fused_candidate_state_ids|arm_quota' tests benchmarkingRepository: 100yenadmin/hermes-lcm
Length of output: 50378
Expose an uncapped fused candidate-id list for the ceiling sweep.
state_candidate_pool is truncated to 64 rows, so fused_candidate_state_ids() can miss state IDs that are present in the full candidate_limit=128 pool. That makes measure_ceiling() undercount recoverable vanished refs for every arm_quota cell. Reuse the same pattern as diversity_cap and return a full state_candidate_pool_ids list instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarking/h3_composition_replay.py` around lines 210 - 224, The fused
candidate lookup in fused_candidate_state_ids currently reads the truncated
state_candidate_pool; update the query telemetry usage to retrieve the uncapped
state_candidate_pool_ids list, following the existing diversity_cap pattern.
Preserve integer state-ID conversion and return the full candidate-limit pool so
measure_ceiling can evaluate every arm_quota cell.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93a3adefca
ℹ️ 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".
| embedded = max(1, stats["states_embedded"]) | ||
| pending = max(1, stats["pending"]) | ||
| projected_tokens = stats["billed_tokens"] / embedded * pending |
There was a problem hiding this comment.
When the first normal embedding request contains multiple states, build_state_semantic_index() invokes the callback before persisting them, so states_embedded is still zero and this denominator becomes 1. A 32-document first batch is therefore projected as though its billed tokens covered one state, inflating the estimated total by roughly 32×; a valid --cost-cap can abort the backfill after a paid request even when the true projected run cost is below the cap. Track documents dispatched in the current request or defer the projection until that batch's completed-state count is available.
Useful? React with 👍 / 👎.
| if len(vectors) != len(sub): | ||
| raise ValueError("chunk embedding count does not match batch size") | ||
| for vector in vectors: | ||
| chunk_vectors.append(_normalized_vector(vector, expected_dim=dim)) |
There was a problem hiding this comment.
When an oversized-state embed_documents() call returns a malformed vector count or a vector with the wrong dimension, these validations raise before provider_calls, billed_tokens, and the progress ledger are updated. The provider request was still dispatched and may be billable, so the CLI loses it from its audit trail and cost-cap accounting; record and emit the request usage immediately after the provider returns, before failure-prone validation, as the normal path now does.
Useful? React with 👍 / 👎.
| for k, v in _COMPOSITIONS.items()}, | ||
| "sweep": results, | ||
| } | ||
| args.out.write_text(json.dumps(payload, indent=2)) |
There was a problem hiding this comment.
When --out names a file in a directory that does not already exist, this final write raises FileNotFoundError only after the golden gate, baseline measurements, and every radius/quota replay have completed. The experiment then consumes its full runtime without producing an artifact; create args.out.parent before starting the sweep, as the sibling replay drivers do.
Useful? React with 👍 / 👎.
| source_profile = self._semantic_profile() | ||
| dim: int | None = None | ||
| probe_provider_calls = 0 | ||
| probe_billed_tokens = 0 | ||
| if ( | ||
| source_profile is not None | ||
| and str(source_profile["provider"]) == provider_name | ||
| and str(source_profile["model_name"]) == model_name | ||
| ): | ||
| dim = int(source_profile["dim"]) |
There was a problem hiding this comment.
When a state-only index is already complete but no source-level semantic profile exists, a default resume still reaches the dimension probe because this lookup consults only _semantic_profile(), not the matching active state profile. The rerun then pays for embed_query() even though every state is skipped, contradicting the zero-provider-call idempotency contract and allowing a tight cost cap to abort an otherwise no-op resume; reuse the dimension from a matching state profile after checking its provider, model, document version, and manifest.
Useful? React with 👍 / 👎.
| _bootstrap_package(_REPO_ROOT) | ||
| provider = CachedVoyageQueryProvider() | ||
| ctx = StateReplayContext( | ||
| args.run_root, args.h1_artifacts, args.h31_artifacts, args.db_dir, provider |
There was a problem hiding this comment.
When --db-dir points to copies backfilled with any supported model other than voyage-4, this hard-coded provider identity does not match the active state profile. _semantic_state_ranks() then silently returns no candidates, while the knob-off golden gate still passes and the script writes an apparently valid sweep reporting zero state-semantic recovery. Resolve the model from the active profiles or fail before the golden gate when both database copies do not match the requested provider identity.
Useful? React with 👍 / 👎.
1 participant
The R2 consolidated train, opened for the fork's multi-bot review rounds (gate 5 of the consolidation checklist)
Head:
2edb8fc— wave-1 (V2 trajectory subsystem) + the Phase-1A scaling fixes (#167 batched full scan, #168 in-product FTS5-safe sanitization with the allow_operators mode split) + #164a summary-hit store_id + the citable-delivery engine (PR #174: reference-strict delivery, entry-lifecycle ledger, derived resume — 8 review rounds, 15 findings closed, property-tested).Measured so far (full provenance in
bench/on docs/program-architecture):Review guide: highest-leverage areas are
search_query.py(sanitizer; adversarially reviewed ×2),vector_store.py(batched scan + cache semantics),tools.pyrecall paths (the citable-delivery ledger — the 8-round review history with per-finding commits is on PR #174), and thelcm_trajectory_*subsystem. Known open items tracked separately: #171 (ANN), #172 (FTS-arm NL rescue), summary-lineage-at-ingest (unfiled — nested provenance data gap).Requesting review rounds from the configured bots (Codex / CodeRabbit / evaOS Code Review Bot). Merge is gated on: clean rounds per the checklist (min 2, cap 5) + the full-500 confirm number landing in the release notes. Do not merge before both.
Summary by CodeRabbit