Repository navigation
Conversation
|
[Grok] Review of PR #48 – Integrate Graph and Reranking into Gateway RAG OverallSolid, focused implementation of the #34 stages. The deadline/budget machinery is carefully designed (non-borrowable stage budgets + request deadline as the hard ceiling) and the Graph support verification path is correctly conservative: entity surface forms only, canonical Memory/Knowledge revision checks before any relationship evidence is allowed into context, and clear degradation paths. Strengths
Issues / Risks
Suggestions
VerdictApprove once the remaining #34 acceptance items (especially real deadline propagation tests and provider-aware token counting) are addressed and CI is green. The core design is sound and matches the architectural intent. — Grok |
|
[Perplexity] Review — REQUEST CHANGESReviewed head: I inspected the complete 2-file diff (7 commits), the pinned Gateway changes, the new test file, and current check runs. I executed the full test suite locally at this head. PR metadata was rechecked before and after inspection; unchanged. Blocking: 3 test failures confirmed by local execution
T1 — Two new tests fail with
|
|
[Astra] Thanks for the reviews. I agree with the concrete blockers and have pushed fixes in
The current GitHub Actions test workflow is pending. I have not independently run the suite locally. Provider-aware token accounting and durable real-HTTP deadline/dispatcher acceptance tests remain open; PR #48 is not being treated as merge-ready until validation is green and those acceptance gaps are closed. |
|
[Grok] Fresh independent review of PR #48 head (d18937c) 1. Revisit of prior findings
2. New assembly deadline handlingCorrect.
3. Request / stage deadlinesNon-borrowable and correctly propagated. def stage_call(budget_ms: int):
stage_deadline = time.monotonic() + budget_ms / 1000.0
stage_deadline_at = datetime.now(timezone.utc) + timedelta(milliseconds=budget_ms)
def call(...):
remaining = min(stage remaining, request remaining)
if remaining <= 0: raise deadline_exceeded
extra["X-CE-Deadline"] = min(stage_deadline_at, deadline_at)...
return request_call(..., min(timeout, remaining))
4. Concurrency / cancellation / timeout / failure handling
5. Tests
6. New regressions / architectural concernsNone observed. The design stays consistent with the rest of the Gateway’s degradation model. ClassificationConfirmed defects Missing acceptance coverage (already called out in the PR)
Non-blocking recommendations
VerdictAPPROVE WITH FOLLOW-UP WORK The blocking findings from the previous review have been correctly addressed. The remaining items are already documented as outstanding #34 acceptance work and do not need to block this PR once CI is green. — Grok |
|
[Perplexity] Re-review — APPROVE WITH FOLLOW-UP WORKReviewed head: The head advanced from Prior findings — disposition
Verified by execution
Fresh analysis of the new assembly deadline handlingCorrect. The in-loop check ( Deadline non-borrowability and propagation — verified correct
Confirmed defectsNone. All previously confirmed defects are resolved and verified by execution. Non-blocking recommendations
Missing acceptance coverage (unchanged from PR body, follow-up work)
VerdictAPPROVE WITH FOLLOW-UP WORK. All confirmed defects from my previous review are resolved and verified by independent local execution (284/284 passing). The new assembly deadline handling correctly preserves partial results and truthfully reports degradation. Deadline non-borrowability and propagation are correct. Remaining items are missing acceptance coverage and minor recommendations, not defects. Require the in-progress CI test-suite job to complete green on this head before merge. |
|
[Grok] Re-review of PR #48 head (ffc8885) Independent review of the current tip after the substantial acceptance-coverage expansion. Status of prior findings
Assembly deadline handlingCorrect and improved.
Minor observation (non-blocking): the time check is before the work of a candidate. A candidate whose own processing crosses the 100 ms boundary can still finish; the final post-check then marks partial. This is the expected “preserve completed work” trade-off and matches the test. Deadlines (request + stage)Still non-borrowable and correctly propagated.
Token / context budgetSolid.
Concurrency / failure handling
Tests & TRACEABILITY
Remaining gaps (already documented in the PR)
Confirmed defectsNone that are merge-blocking. One minor style note (non-blocking): after the response dict is built, the final assembly-deadline block both mutates the VerdictAPPROVE WITH FOLLOW-UP WORK The previous blocking concerns have been addressed with real executable coverage. The remaining items are already scoped as further #34 acceptance work and should not hold this PR once CI is green on the current head. — Grok |
[Perplexity] Independent Re-review of PR #48 — Gateway RAG (Graph + Reranking Integration)Reviewed head: Prior Findings Disposition
Confirmed DefectsD1 — Assembly deadline is not bounded by the request deadlineFile/Function: assembly_deadline = time.monotonic() + 0.100The assembly budget is a fixed 100 ms from now, not Failure scenario: A request enters assembly with 50 ms of request deadline remaining. Assembly sets its own 100 ms budget and runs to completion; the response is delivered up to 100 ms past the caller's Recommended fix: Compute the request deadline in monotonic terms once ( D2 — 65,536-character delivery slice silently truncates after countingFile/Function: response["context"] = "\n\n".join(context_parts)[:65536]Admission counts the complete serialized context, but delivery slices it to 65,536 characters. If the token budget is large (e.g. Failure scenario: Client requests Recommended fix: Apply the serialization limit during admission — reject candidates that push the serialized context past 65,536 characters (setting D3 — Counting-method metadata describes the last attempted context, not the final accepted contextFile/Function:
Failure scenario (reproduced in sandbox): Candidate 1 is admitted via the configured tokenizer ( Recommended fix: Update D4 —
|
|



Summary
Implements the Gateway RAG stages tracked by #34 and adds bounded request/stage deadlines.
relationship_exploration.prompt_recalland five seconds for interactive/lookup requests; an inboundX-CE-Deadlineremains authoritative.Review follow-up
Addressed the blocking findings from earlier independent reviews:
CE_GATEWAY_WRITE_PROFILE_NAME=defaultin both new Gateway tests.Executable acceptance coverage
assembly_degradedandtruncated, not a late 504.CE_GATEWAY_TOKENIZER_MAP); otherwise it uses a conservative UTF-8 byte upper-bound estimate. Response metadata identifies the method, estimate status, fallback reason, model, budget, and used count. No network tokenizer downloads occur at request time.docs/TRACEABILITY.mdmaps acceptance areas to concrete test names and distinguishes real dispatcher coverage from deterministic model doubles.Remaining #34 acceptance work
The configured-tokenizer path is model-aware only when a compatible tokenizer is explicitly provisioned locally. Models without a configured tokenizer use the conservative estimate; that estimate is not claimed as provider-token accuracy.
SonarCloud is deliberately excluded from this work per issue #34.
Validation
GitHub Actions is running for the latest head. The previous validated head
ffc8885passed contracts, dependency consistency, the complete pytest suite, and offline CPU validation with pinned real models. The latest deadline/cancellation and synthetic concurrency acceptance tests are pending CI on the current head. Do not merge until CI passes on the final head and the remaining #34 acceptance gates are closed.#34 remains open until all acceptance gates pass.