[AI-1887] Emit judge-fact guidelines at session start for all harnesses - #538
Merged
Merged
Conversation
PR Summary by QodoEmit session-start guidelines for all harnesses via composite context provider
AI Description
Diagram
High-Level Assessment
Files changed (24)
|
Code Review by Qodo
1. Cursor budget misaccounted
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
realtonyyoung
force-pushed
the
ai-1887-guideline-injection-all-vendors
branch
from
August 12, 2026 00:26
26c903b to
9651e22
Compare
Collaborator
Author
|
/agentic_review |
Comment on lines
+36
to
+39
| /// <summary> | ||
| /// Overload for the guidelines lane: the eight non-Claude harnesses | ||
| /// fetch <c>GET /api/repositories/{hash}/guidelines</c> directly and pass the | ||
| /// parsed rows here, rather than reading <c>top_clusters</c> from a hook |
There was a problem hiding this comment.
1. Verbose xml doc for overload 📘 Rule violation ⚙ Maintainability
The new BuildFragment(IReadOnlyList<GuidelineRow>?) overload includes an overly long XML doc comment describing endpoint behavior and size limits. This should be shortened to the essential contract and any non-obvious constraints.
Agent Prompt
## Issue description
New XML documentation is overly verbose and includes operational details that can live elsewhere.
## Issue Context
PR Compliance requires minimal comments and self-explanatory code.
## Fix Focus Areas
- src/Capacitor.Cli/SessionGuidelinesEmitter.cs[36-43]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+551
to
+554
| var activeProfile = await AppConfig.GetActiveProfileAsync(); | ||
| var disabled = activeProfile?.DisableMemoryIndex is true; | ||
| var guidelinesDisabled = activeProfile?.DisableSessionGuidelines is true; | ||
| if (disabled && guidelinesDisabled) return null; |
There was a problem hiding this comment.
2. Cursor budget misaccounted 🐞 Bug ☼ Reliability
CursorHookCommand snapshots memBudget before awaiting GetActiveProfileAsync(), so profile-read time is not deducted from the remaining session-start budget and the subsequent context fetch can run longer than the intended leftover window.
Agent Prompt
### Issue description
`RunMemoryOrchestrationAsync` computes `memBudget` before awaiting `AppConfig.GetActiveProfileAsync()`. The awaited profile load can consume non-trivial time, but `memBudget` is later used to bound `memCts.CancelAfter(memBudget)` and is passed into `SessionStartMemoryContextRequest`, so the network/context work can receive a larger budget than what is actually left.
### Issue Context
This method’s design intent (per comments) is that memory/guidelines work runs strictly on “whatever budget is left over”. That intent is currently undermined by doing the async profile read after capturing `memBudget`.
### Fix Focus Areas
- src/Capacitor.Cli/Commands/CursorHookCommand.cs[542-590]
### Suggested fix
Move the active-profile read earlier (before computing `memBudget`), or recompute `memBudget` after the `await` using an updated `sw.Elapsed` (preserving `memoryBudgetOverride` semantics if it’s intended to be absolute). Then keep the existing `memBudget <= 0` guard and continue using the recomputed budget for `CancelAfter` and `SessionStartMemoryContextRequest`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 9651e22 |
SessionStart guideline injection (## Known patterns / ## Guidance from past
sessions) reached only Claude, which reads top_clusters off its hook POST
response. The eight non-Claude harnesses got the team-memory index but no
guidelines.
This adds a parallel "guidelines lane" to the existing per-vendor memory
orchestration and composes ONE combined fragment (marker → ## Team memory →
guidelines) that rides each harness's existing delivery seam unchanged — no
plugin/extension/envelope changes.
- ISessionStartContextProvider seam on the orchestrator; SessionStartContextFetch
shares the authenticated-GET/401-refresh/bounded-read transport between lanes.
- SessionStartGuidelinesLane fetches GET /api/repositories/{hash}/guidelines and
maps 404 -> retryable (the visibility race: 404 = "not visible yet", not "no
facts"), unlike the memory lane's 404 -> empty.
- SessionStartCompositeContextProvider resolves scope once, runs both lanes in
parallel, and combines over ENABLED lanes only: any content commits; all empty
completes-without-context; no content + >=1 retryable holds for retry with the
max of the lanes' Retry-After hints. A disabled lane contributes nothing.
- The orchestrator's early return becomes both-disabled; every non-Claude adapter
routes through SessionStartMemoryHookSupport.CompositeProvider and passes the
guidelines opt-out. Claude keeps its memory-only provider (no guidelines GET);
its only change is reading disable_session_guidelines from the EFFECTIVE profile
(the KCAP_URL defect). Cursor gains effective-profile threading inside its 2s
dispatcher race + the same DisableMemoryIndex fix.
No CLI-side size cap (match Claude; the server clamps). Combined fragment still
opens with the kcap-memory-index marker so Pi/OpenCode capture is unchanged.
Tests: guidelines-lane status mapping incl. 404->retryable, composite disposition
matrix + lease/compose rules, Retry-After max aggregation, marker-first compose,
emitter row overload, and a Claude-issues-no-guidelines-GET isolation test.
Existing per-vendor memory suites unchanged and green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
realtonyyoung
force-pushed
the
ai-1887-guideline-injection-all-vendors
branch
from
August 12, 2026 00:33
9651e22 to
4045f67
Compare
Collaborator
Author
|
Addressed both qodo findings. (1) Uncancellable Cursor profile load: |
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.
What
SessionStart guideline injection (
## Known patterns/## Guidance from past sessions, evaluation-derived fact clusters) reached only Claude Code, which readstop_clustersoff its hook POST response. The other eight harnesses got the team-memory index but no guidelines.This adds a parallel guidelines lane to the existing per-vendor memory orchestration and composes one combined fragment (marker →
## Team memory→ guidelines) that rides each harness's existing delivery seam unchanged — no plugin/extension/envelope changes.How
ISessionStartContextProviderseam on the orchestrator;SessionStartContextFetchshares the authenticated-GET / 401-refresh / 256 KiB bounded-read transport between the two lanes (byte-for-byte what the memory lane did before).SessionStartGuidelinesLanefetchesGET /api/repositories/{hash}/guidelinesand maps 404 → retryable — the visibility race: a 404 means "not visible yet" (the endpoint gates on a projected caller-visible session), not "no facts". The memory lane keeps 404 → empty; this deliberate divergence is why it's a separate lane.SessionStartCompositeContextProviderresolves the repo/machine scope once (matters under Cursor's 2s budget) and runs both lanes in parallel, combining over enabled lanes only: any content ⇒ commit; all empty ⇒ complete-without-context; no content + ≥1 retryable ⇒ retry with the max of the lanes'Retry-Afterhints. A disabled lane contributes nothing and never blocks commit. Marker-first compose: the guidelines-only case prepends the sharedkcap-memory-indexmarker so Pi/OpenCode capture (stdout must OPEN with it) is unchanged.SessionStartMemoryHookSupport.CompositeProviderand passes the guidelines opt-out.disable_session_guidelinesfrom the effective profile (the pre-existingKCAP_URL/--server-urldefect whereResolvedProfile.Profileis null) — it keeps its memory-only provider and issues no guidelines GET (pinned by a test).HandleCoredeadline race (so a slow config read can't blow the 2s dispatcher contract —HandleCoreabandons the computation on the deadline), plus the sameDisableMemoryIndexeffective-profile fix.No CLI-side size cap (matches Claude; the server clamps row count + text length).
Server side
The endpoint
GET /api/repositories/{hash}/guidelinesalready exists. Its response contract + aGuidelineInjectionConfigsize-bound validator are hardened in the companion kcap-server PR (independent, non-blocking in either direction — this CLI change is well-defined against any server version/config: an over-256 KiB or missing response is a fail-open retryable lane failure, never a crash, and never blocks a delivered memory lane).Tests
New
GuidelinesLaneAndCompositeTests(17): guidelines-lane status mapping incl. 404 → retryable, composite disposition matrix + lease/compose rules,Retry-Aftermax aggregation, marker-first compose, disabled-lane isolation, emitter row overload. Plus aClaudeHookCommandTestsisolation test proving Claude issues no guidelines GET. All existing per-vendor memory suites (138) + foundation (31) + Cursor (42) + Codex source (16) unchanged and green.🤖 Generated with Claude Code