Completes the context-complete tool path - #142
Merged
wpak-ai merged 4 commits intoJun 9, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThis PR threads ServerContext into URL builtins and query-result formatting, updates tool callers to pass ctx, and adds tests covering suggestion-flow (TTL, disable bypass), degradation/timeouts, and context isolation. Vitest branch coverage threshold increased to 65%. ChangesServerContext Threading and Test Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
![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
🧹 Nitpick comments (1)
vitest.config.ts (1)
4-19: ⚡ Quick winThreshold may be too conservative per documented policy.
The comment on lines 4–6 states that thresholds should sit "~2–3% below the last measured…totals" to catch meaningful regressions. However, if the measured branch coverage is 74.64% (per PR objectives), the documented policy would suggest a threshold of approximately 71–72%, not 65%.
The current 65% threshold creates a ~9.6 percentage point gap, allowing branch coverage to drop significantly before CI fails—potentially missing regressions the policy aims to catch.
Consider either:
- Raising the threshold to ~71–72% to align with the documented policy, or
- Updating the comment to explain why 65% was chosen (e.g., "Phase 3 milestone threshold; will incrementally raise to measured-minus-slack in future PRs").
🤖 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 `@vitest.config.ts` around lines 4 - 19, The branch coverage threshold in the vitest config (test.coverage.thresholds.branches) is lower (65) than your documented policy (~2–3% below measured 74.64%), so either raise test.coverage.thresholds.branches to ~71–72 to match the policy or update the nearby comment above defineConfig to justify the 65% "phase" choice; locate the thresholds object (thresholds: { lines, statements, branches }) and modify the branches value or the comment accordingly.
🤖 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 `@src/alliance/url-builtins.ts`:
- Around line 83-89: isServerContext currently checks getConfig and
generateUrlForNamespace but not registerUrlGenerator, yet code later calls
registerUrlGenerator; strengthen the type guard by adding a function check for
(value as ServerContext).registerUrlGenerator so the guard verifies all methods
you actually call (getConfig, generateUrlForNamespace, registerUrlGenerator)
before narrowing to ServerContext. Update the isServerContext function to
include this additional typeof ... === 'function' check so partially-shaped
objects cannot pass and cause runtime errors.
In `@src/core/server/server-context.test.ts`:
- Around line 150-166: The test "requireSuggested returns expiry message after
TTL on instance context" enables fake timers with vi.useFakeTimers() but only
calls vi.useRealTimers() on the success path; wrap the fake-timer usage in a
try/finally (or add an afterEach that calls vi.useRealTimers()) so
vi.useRealTimers() is always executed even if assertions throw; modify the test
around the ServerContext instantiation/vi.advanceTimersByTime and calls to
ctx.requireSuggested to ensure vi.useRealTimers() is invoked in the finally
block (or configure a global afterEach) to prevent leaking fake timers to other
tests.
---
Nitpick comments:
In `@vitest.config.ts`:
- Around line 4-19: The branch coverage threshold in the vitest config
(test.coverage.thresholds.branches) is lower (65) than your documented policy
(~2–3% below measured 74.64%), so either raise test.coverage.thresholds.branches
to ~71–72 to match the policy or update the nearby comment above defineConfig to
justify the 65% "phase" choice; locate the thresholds object (thresholds: {
lines, statements, branches }) and modify the branches value or the comment
accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4afcd158-3a43-45b1-bcc3-98156082a1e3
📒 Files selected for processing (17)
src/alliance/setup.tssrc/alliance/tools/guided-query-tool.context.test.tssrc/alliance/tools/guided-query-tool.tssrc/alliance/tools/isolated-context.context.test.tssrc/alliance/tools/suggest-query-params-tool.context.test.tssrc/alliance/url-builtins.context.test.tssrc/alliance/url-builtins.tssrc/core/server/format-query-result.context.test.tssrc/core/server/format-query-result.tssrc/core/server/server-context.test.tssrc/core/server/suggestion-flow.test.tssrc/core/server/tools/count-tool.context.test.tssrc/core/server/tools/keyword-search-tool.tssrc/core/server/tools/query-tool.context.test.tssrc/core/server/tools/query-tool.tssrc/core/server/tools/test-helpers.tsvitest.config.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #142 +/- ##
=======================================
Coverage ? 85.95%
=======================================
Files ? 39
Lines ? 1445
Branches ? 485
=======================================
Hits ? 1242
Misses ? 201
Partials ? 2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
AuraMindNest
approved these changes
Jun 9, 2026
wpak-ai
approved these changes
Jun 9, 2026
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
3 participants
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.
PR body (paste below the title on GitHub)
Summary
Phase 3 completes the context-complete tool path: formatters and Alliance URL builtins use the injected
ServerContexton the instance path, and new.context.test.ts/suggestion-flow.test.tscoverage exercises suggest-flow TTL, degradation, timeouts, and isolated-context URL enrichment without relying on process-default facades.Issues addressed
Changes
formatSearchResultAsRow/formatQueryResultRowsaccept optionalctxand callctx.generateUrlForNamespace()when enriching URLsquery,keyword_search, andguided_querypassctxthrough to formattersregisterBuiltinUrlGenerators(ctx?, options?)registers per-context viaWeakSet;setupAllianceServerpasses the samectxas tool registrationregisterBuiltinUrlGenerators({ reinstallBuiltins: true })call sitesisolateFromDefaultContext()test helper for phase 3 exit criterionAcceptance criteria
#120
ctx; tools passctxto formattersregisterBuiltinUrlGenerators(ctx?)+setupAllianceServerwiringguided_queryinstance path usesctxfor client, cache, suggest-flow, URL enrichmentgetDefaultServerContext()imports outside facade modulesisolated-context.context.test.ts)createTestServerContext()/.context.test.ts, no facade mocks#138
suggest_query_paramsupdates state, not duplicatesquerysucceeds, no FLOW_GATEguided_querydoes not block subsequent explicitsuggest_query_paramsdisableSuggestFlowonquery+countinstance pathsuggestion-flow.tsfacade fully exercised (suggestion-flow.test.ts; file has no branch points in v8 report)#133
format-query-resultuses context-owned URL registry whenctxprovidedguided-querydelegates toctxsuggest-flow / cache / client on instance path*.test.tsunchanged and still passTest plan
npm test— 207 tests passnpm run test:coverage— branches 74.64% (gate 65%),suggestion-flow.ts100% statementsnpm run build— compiles cleanlyguided_query+enrich_urlson isolatedServerContext, zero default contextsetupAllianceServerdefault-context path unchanged (manual)Notes for #133 reviewers
Dual-path facade imports in
guided-query-tool.tsremain for the legacy no-ctxpath. The instance path is fully context-driven. Legacy*.test.tsfiles still mock module facades by design; new.context.test.tsfiles cover the instance path without facade mocks.Summary by CodeRabbit
New Features
Bug Fixes
Tests