fix(sessions): derive root agent labels from identity#259
Open
DrJsPBs wants to merge 4 commits intodaggerhashimoto:masterfrom
Open
fix(sessions): derive root agent labels from identity#259DrJsPBs wants to merge 4 commits intodaggerhashimoto:masterfrom
DrJsPBs wants to merge 4 commits intodaggerhashimoto:masterfrom
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 (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSession labeling now prefers durable agent identities: identity names are parsed from IDENTITY.md, cached per-root in SessionContext, used to override top-level session labels via a new Changes
Sequence Diagram(s)sequenceDiagram
participant SessionCtx as SessionContext (effect)
participant Cache as rootIdentityNames / misses
participant FetchAPI as /api/workspace/identity
participant Consumer as UI / Tests
rect rgba(200,200,255,0.5)
SessionCtx->>Cache: derive missing rootIds from sessions
end
rect rgba(200,255,200,0.5)
SessionCtx->>FetchAPI: fetch identity?agentId=<rootId> (concurrent, AbortController)
FetchAPI-->>SessionCtx: identity content or error
SessionCtx->>Cache: extractIdentityName() -> store hit or miss
end
rect rgba(255,200,200,0.5)
SessionCtx->>Consumer: expose displaySessions (override identityName)
Consumer-->>SessionCtx: may call refreshSessions()
SessionCtx->>FetchAPI: on refresh, re-evaluate missing roots (skip cached misses)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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: 1
🧹 Nitpick comments (3)
src/features/sessions/sessionKeys.test.ts (1)
24-28: Add one non-bulletedName:parser test for robustness.
extractIdentityNameis currently tested for bulleted formats; adding a plain-line variant (e.g.,Name: Reviewer Prime) would harden coverage against commonIDENTITY.mdformatting differences.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/sessions/sessionKeys.test.ts` around lines 24 - 28, Add a new unit test case for extractIdentityName that covers a non-bulleted plain-line format (e.g., "Name: Reviewer Prime") so the parser handles unlisted Name: lines; update the test suite in sessionKeys.test.ts by adding an it(...) that calls extractIdentityName with a string containing "Name: Reviewer Prime" and asserts the result equals "Reviewer Prime", referencing the existing extractIdentityName function to locate where to add this case.src/features/sessions/sessionKeys.ts (1)
133-136: Broaden identity-name parsing to accept non-bulletedName:lines.
Current regex only matches list-item forms. Supporting both list and plain-line variants will reduce false fallbacks to root id whenIDENTITY.mdformatting differs.💡 Suggested patch
export function extractIdentityName(content: string): string | null { const normalized = content.replace(/\*\*/g, ''); - const match = normalized.match(/^\s*[-*]\s*Name\s*:\s*(.+?)\s*$/im); + const match = normalized.match(/^\s*(?:[-*]\s*)?Name\s*:\s*(.+?)\s*$/im); return match?.[1]?.trim() || null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/sessions/sessionKeys.ts` around lines 133 - 136, The extractIdentityName function's regex only matches bulleted lines and misses plain "Name: ..." lines; update the regex in extractIdentityName to accept an optional list marker so it matches both bulleted and non-bulleted forms (e.g. allow an optional leading "- " or "* " before "Name:"), keep the normalization step, capture the same group, trim it and return or null as before; target the extractIdentityName function to replace the current /^\s*[-*]\s*Name\s*:\s*(.+?)\s*$/im with a pattern that allows the optional bullet.src/features/kanban/lib/assigneeOptions.test.ts (1)
23-24: Consider adding one identity-present option test to lock the primary label path.
These updates validate root-id fallback well, but this suite now lacks a direct assertion for the preferredidentityName-driven label path in assignee options. Adding one case would prevent regressions where options silently fall back to root id despite identity hydration.Also applies to: 53-53, 66-66, 80-80, 98-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/kanban/lib/assigneeOptions.test.ts` around lines 23 - 24, Add a unit test in the assigneeOptions.test.ts suite that exercises getAssigneeOptions (or the function that builds assignee options) with an identity object that includes identityName present; assert that the resulting option for that identity uses the identity-driven label path (e.g., value starts with "identity:" and label equals the identityName like "designer") rather than falling back to root-id format. Add similar assertions alongside the existing cases around the current entries ({ value: 'agent:designer', label: 'designer' }, { value: 'agent:reviewer', label: 'reviewer' }) and replicate for the other mentioned spots so the preferred identityName path is explicitly locked in.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/contexts/SessionContext.tsx`:
- Around line 173-213: The effect repeatedly refetches identities for roots that
have no parseable "Name:" because those misses are never recorded; add a
miss-cache so negative lookups are treated as resolved and won't be retried
every refresh. Concretely, introduce a lightweight miss set (e.g.,
rootIdentityMisses: Set<string> or a Map<string, number> for TTL) and, inside
the Promise.all resolution, when extractIdentityName returns null record the
rootId into that miss set (or set a timestamp), and change the early-filtering
that builds rootAgentIds to exclude ids present in rootIdentityMisses (or
not-yet-expired entries). Also, when a subsequent successful identity is found
update setRootIdentityNames as before and remove that rootId from the miss set;
if you choose TTL, expire entries based on timestamps to allow retries.
Reference: rootAgentIds, extractIdentityName, setRootIdentityNames, sessions,
rootIdentityNames, and the AbortController handling in the effect.
---
Nitpick comments:
In `@src/features/kanban/lib/assigneeOptions.test.ts`:
- Around line 23-24: Add a unit test in the assigneeOptions.test.ts suite that
exercises getAssigneeOptions (or the function that builds assignee options) with
an identity object that includes identityName present; assert that the resulting
option for that identity uses the identity-driven label path (e.g., value starts
with "identity:" and label equals the identityName like "designer") rather than
falling back to root-id format. Add similar assertions alongside the existing
cases around the current entries ({ value: 'agent:designer', label: 'designer'
}, { value: 'agent:reviewer', label: 'reviewer' }) and replicate for the other
mentioned spots so the preferred identityName path is explicitly locked in.
In `@src/features/sessions/sessionKeys.test.ts`:
- Around line 24-28: Add a new unit test case for extractIdentityName that
covers a non-bulleted plain-line format (e.g., "Name: Reviewer Prime") so the
parser handles unlisted Name: lines; update the test suite in
sessionKeys.test.ts by adding an it(...) that calls extractIdentityName with a
string containing "Name: Reviewer Prime" and asserts the result equals "Reviewer
Prime", referencing the existing extractIdentityName function to locate where to
add this case.
In `@src/features/sessions/sessionKeys.ts`:
- Around line 133-136: The extractIdentityName function's regex only matches
bulleted lines and misses plain "Name: ..." lines; update the regex in
extractIdentityName to accept an optional list marker so it matches both
bulleted and non-bulleted forms (e.g. allow an optional leading "- " or "* "
before "Name:"), keep the normalization step, capture the same group, trim it
and return or null as before; target the extractIdentityName function to replace
the current /^\s*[-*]\s*Name\s*:\s*(.+?)\s*$/im with a pattern that allows the
optional bullet.
🪄 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: 9fff8476-cc84-4b2f-bd0a-022bd22317a1
📒 Files selected for processing (8)
src/contexts/SessionContext.test.tsxsrc/contexts/SessionContext.tsxsrc/features/kanban/CreateTaskDialog.test.tsxsrc/features/kanban/TaskDetailDrawer.test.tsxsrc/features/kanban/lib/assigneeOptions.test.tssrc/features/sessions/sessionKeys.test.tssrc/features/sessions/sessionKeys.tssrc/types.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/contexts/SessionContext.test.tsx (1)
461-531: Add one regression assertion for staleidentityNameon miss path.This test validates no-refetch behavior, but it doesn’t verify rendered fallback when
sessions.listincludes a staleidentityNameand/api/workspace/identityreturns no parseableName:. Please add a case asserting the top-level label falls back toreviewer(rootId), not stale identity text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contexts/SessionContext.test.tsx` around lines 461 - 531, Add one assertion after the refresh that checks the rendered label falls back to the rootId "reviewer" (not the stale identity text). After the existing await waitFor that confirms rpcMock called 'sessions.list', query the rendered output (e.g., using getByText or getByTestId from the same render result) to assert that "reviewer" is present and that the stale label "stale reviewer label" is not rendered; reference the test helpers/getters already in scope (fetchSpy, rpcMock, getByTestId, SessionProvider, SessionRefreshProbe) to locate where to insert the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/contexts/SessionContext.tsx`:
- Around line 244-246: The code currently returns the original session when a
non-`main` root's name is unresolved, which leaves a stale
`session.identityName`; update the `identityName` handling in the block that
computes identityName (using `rootId`, `rootIdentityNames`, and `session`) so
that when `rootId !== 'main'` and `rootIdentityNames[rootId]` is falsy you
explicitly clear `session.identityName` (e.g., return a new session object with
`identityName` set to undefined/null) instead of returning the original session;
this ensures `getSessionDisplayLabel` will fall back to the `<rootId>` label
rather than showing stale text.
---
Nitpick comments:
In `@src/contexts/SessionContext.test.tsx`:
- Around line 461-531: Add one assertion after the refresh that checks the
rendered label falls back to the rootId "reviewer" (not the stale identity
text). After the existing await waitFor that confirms rpcMock called
'sessions.list', query the rendered output (e.g., using getByText or getByTestId
from the same render result) to assert that "reviewer" is present and that the
stale label "stale reviewer label" is not rendered; reference the test
helpers/getters already in scope (fetchSpy, rpcMock, getByTestId,
SessionProvider, SessionRefreshProbe) to locate where to insert the assertion.
🪄 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: 218aa2ab-4181-4f98-a72d-23673487d499
📒 Files selected for processing (5)
src/contexts/SessionContext.test.tsxsrc/contexts/SessionContext.tsxsrc/features/kanban/lib/assigneeOptions.test.tssrc/features/sessions/sessionKeys.test.tssrc/features/sessions/sessionKeys.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/features/kanban/lib/assigneeOptions.test.ts
- src/features/sessions/sessionKeys.test.ts
- src/features/sessions/sessionKeys.ts
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
Fix root-agent labeling so non-
maintop-level agents resolve from durable agent identity instead of transient session metadata.Why
This fixes root-agent rows showing stale
label/displayNamevalues from session metadata instead of the actual agent identity.mainalready used a dedicated identity path, but other root agents did not.Closes #258
How
mainroot sessions withidentityNamefrom/api/workspace/identityName:from each agent'sIDENTITY.md<identityName> (<rootId>)Type of Change
Checklist
npm run lintpassesnpm run build && npm run build:serversucceedsnpm test -- --runpassesSummary by CodeRabbit
New Features
Bug Fixes
Tests