diff --git a/.ai/contexts/subagent-observability.md b/.ai/contexts/subagent-observability.md index 653e3e43..a27cdf63 100644 --- a/.ai/contexts/subagent-observability.md +++ b/.ai/contexts/subagent-observability.md @@ -167,6 +167,46 @@ an agent tracked from its first line. The renderer's 60 s TTL has the same shape. Anything that needs true liveness would have to come from the parent process, not from mtime. +## Subagent children inside a slug group (issue #128 ask 4, rehab-plan.md A3) + +- **The bug**: `buildSlugGroup()` used to append its sessions via the raw + `buildSessionItem(session)`, never through `appendSubagentChildren()` — only + the ungrouped/top-level render path (`buildSessionsList`) called that + helper. Any session rendered inside a slug group (i.e. any second-or-later + rerun of a schedule sharing a slug — the only real producer, see + `schedule-runner.js:createScheduleSession()`) silently lost its subagent + caret/children. +- **The fix**: `appendSubagentChildren()` was hoisted from a closure inside + `renderProjects()` to module scope (it never captured any of that + function's locals) so `buildSlugGroup(slug, sessions, subagentIndex)` can + call it directly for each session it renders, exactly like + `buildSessionsList` does for ungrouped sessions. +- **The knock-on bug this caused**: a slug-group `
` carries no + `dataset.sessionId` of its own, so the "orphan subagents" pass in + `buildSessionsList` — which built `allTopLevelIds` from + `item.element.dataset.sessionId` — never counted the sessions grouped + inside it as accounted-for. Their subagents were treated as parentless and + duplicated into the project's "Orphan subagents" bucket even after the fix + above attached them correctly inside the group. `collectTopLevelSessionIds(el)` + fixes this by walking into `el` for nested `[data-session-id]` session-items + (excluding subagent ones) when `el` itself isn't a session item. +- **Coverage**: `test/dom-slug-group-subagent-nesting.test.js` seeds two + schedule-rerun-shaped sessions sharing a slug plus a subagent parented to + one of them, and pins both the caret-attachment fix and the + no-duplicate-orphan fix (failed on both before the fix, confirmed by + reverting it locally during development). +- **A second knock-on bug (PR #134 review F1)**: nesting the subagent's + caret/children as DOM siblings inside the group means every DOM query + scoped to `.slug-group` that matches on `.session-item` alone now also + matches the nested subagent item (`buildSubagentItem` includes + `session-item` in its className for shared styling). The "Archive all + sessions in group" handler (`rebindSidebarEvents`, `.slug-group-archive-btn`) + had exactly this query and, unguarded, called `archiveSession`/`stopSession` + on the subagent's id. Fixed with the same `:not([data-subagent])` guard + already used elsewhere in this file (e.g. the per-item click wiring). Any + *new* query scoped to a slug-group's subtree must apply this guard too — + it's not automatic. + ## If you change this, also check - `eslint.config.js` `rendererCrossFileGlobals` — must list any new renderer-global functions (e.g. `showSubagentTranscript`, `drainViewerWatches`) or lint fails on `no-undef` diff --git a/public/sidebar.js b/public/sidebar.js index 1af1c0ab..5ebcf744 100644 --- a/public/sidebar.js +++ b/public/sidebar.js @@ -226,7 +226,56 @@ function buildSubagentItem(session) { return item; } -function buildSlugGroup(slug, sessions) { +// Shared by buildSessionsList and buildSlugGroup — see .ai/contexts/subagent-observability.md +function appendSubagentChildren(parentEl, parentSessionId, subagentIndex) { + const children = subagentIndex && subagentIndex.get(parentSessionId); + if (!children || children.length === 0) return; + + const expandedSet = getExpandedSubagents(); + const caretId = caretIdFor(parentSessionId); + const isExpanded = expandedSet.has(parentSessionId); + + const caret = document.createElement('div'); + caret.className = 'sidebar-children-caret js-stateful'; + caret.id = caretId; + if (isExpanded) caret.classList.add('expanded'); + if (parentHasActiveSubagent(parentSessionId)) caret.classList.add('has-running-child'); + caret.innerHTML = `▶ ${children.length} subagent${children.length !== 1 ? 's' : ''}`; + + const childrenContainer = document.createElement('div'); + childrenContainer.className = 'sidebar-subagents-container js-stateful'; + childrenContainer.id = 'subc-' + parentSessionId.replace(/[^a-zA-Z0-9_-]/g, '_'); + childrenContainer.style.display = isExpanded ? '' : 'none'; + + for (const child of children) { + childrenContainer.appendChild(buildSubagentItem(child)); + } + + caret.addEventListener('click', (e) => { + e.stopPropagation(); + const open = childrenContainer.style.display !== 'none'; + childrenContainer.style.display = open ? 'none' : ''; + caret.classList.toggle('expanded', !open); + const set = getExpandedSubagents(); + if (open) { set.delete(parentSessionId); } else { set.add(parentSessionId); } + saveExpandedSubagents(set); + }); + + parentEl.after(caret); + caret.after(childrenContainer); +} + +// See .ai/contexts/subagent-observability.md (slug-group orphan-detection fix) +function collectTopLevelSessionIds(el) { + const ids = []; + if (el.dataset && el.dataset.sessionId && !el.dataset.subagent) ids.push(el.dataset.sessionId); + el.querySelectorAll('[data-session-id]').forEach((child) => { + if (!child.dataset.subagent) ids.push(child.dataset.sessionId); + }); + return ids; +} + +function buildSlugGroup(slug, sessions, subagentIndex) { const group = document.createElement('div'); const id = slugId(slug); const expanded = getExpandedSlugs().has(id); @@ -290,7 +339,9 @@ function buildSlugGroup(slug, sessions) { if (promoted.length > 0) { group.classList.add('has-promoted'); for (const session of promoted) { - sessionsContainer.appendChild(buildSessionItem(session)); + const sessionEl = buildSessionItem(session); + sessionsContainer.appendChild(sessionEl); + appendSubagentChildren(sessionEl, session.sessionId, subagentIndex); } if (rest.length > 0) { const moreBtn = document.createElement('div'); @@ -302,7 +353,9 @@ function buildSlugGroup(slug, sessions) { olderDiv.className = 'slug-group-older js-stateful'; olderDiv.id = 'sgo-' + id; for (const session of rest) { - olderDiv.appendChild(buildSessionItem(session)); + const sessionEl = buildSessionItem(session); + olderDiv.appendChild(sessionEl); + appendSubagentChildren(sessionEl, session.sessionId, subagentIndex); } sessionsContainer.appendChild(moreBtn); @@ -310,7 +363,9 @@ function buildSlugGroup(slug, sessions) { } } else { for (const session of sessions) { - sessionsContainer.appendChild(buildSessionItem(session)); + const sessionEl = buildSessionItem(session); + sessionsContainer.appendChild(sessionEl); + appendSubagentChildren(sessionEl, session.sessionId, subagentIndex); } } @@ -416,7 +471,7 @@ function renderProjects(projects, resort) { const mostRecentTime = Math.max(...sessions.map(s => new Date(s.modified).getTime())); const hasRunning = sessions.some(s => activePtyIds.has(s.sessionId) || pendingSessions.has(s.sessionId)); const hasPinned = sessions.some(s => s.starred); - const element = sessions.length === 1 ? buildSessionItem(sessions[0]) : buildSlugGroup(slug, sessions); + const element = sessions.length === 1 ? buildSessionItem(sessions[0]) : buildSlugGroup(slug, sessions, subagentIndex); allItems.push({ sortTime: mostRecentTime, pinned: hasPinned, running: hasRunning, element }); } @@ -466,45 +521,6 @@ function renderProjects(projects, resort) { }; } - // Append subagent children beneath a session item element. - function appendSubagentChildren(parentEl, parentSessionId, subagentIndex) { - const children = subagentIndex && subagentIndex.get(parentSessionId); - if (!children || children.length === 0) return; - - const expandedSet = getExpandedSubagents(); - const caretId = caretIdFor(parentSessionId); - const isExpanded = expandedSet.has(parentSessionId); - - const caret = document.createElement('div'); - caret.className = 'sidebar-children-caret js-stateful'; - caret.id = caretId; - if (isExpanded) caret.classList.add('expanded'); - if (parentHasActiveSubagent(parentSessionId)) caret.classList.add('has-running-child'); - caret.innerHTML = `▶ ${children.length} subagent${children.length !== 1 ? 's' : ''}`; - - const childrenContainer = document.createElement('div'); - childrenContainer.className = 'sidebar-subagents-container js-stateful'; - childrenContainer.id = 'subc-' + parentSessionId.replace(/[^a-zA-Z0-9_-]/g, '_'); - childrenContainer.style.display = isExpanded ? '' : 'none'; - - for (const child of children) { - childrenContainer.appendChild(buildSubagentItem(child)); - } - - caret.addEventListener('click', (e) => { - e.stopPropagation(); - const open = childrenContainer.style.display !== 'none'; - childrenContainer.style.display = open ? 'none' : ''; - caret.classList.toggle('expanded', !open); - const set = getExpandedSubagents(); - if (open) { set.delete(parentSessionId); } else { set.add(parentSessionId); } - saveExpandedSubagents(set); - }); - - parentEl.after(caret); - caret.after(childrenContainer); - } - // Build the sessions list DOM (shared between projects and worktrees) function buildSessionsList(fId, visible, older, subagentIndex, projectPath) { const sessionsList = document.createElement('div'); @@ -534,9 +550,10 @@ function renderProjects(projects, resort) { sessionsList.appendChild(olderList); } - // Orphan subagents: children whose parentSessionId has no top-level session in this project + // Orphan subagents: children whose parentSessionId has no top-level session in this project. + // See .ai/contexts/subagent-observability.md for why collectTopLevelSessionIds is needed here. if (subagentIndex) { - const allTopLevelIds = new Set([...visible, ...older].map(i => i.element.dataset && i.element.dataset.sessionId).filter(Boolean)); + const allTopLevelIds = new Set([...visible, ...older].flatMap(i => collectTopLevelSessionIds(i.element))); const orphans = []; for (const [parentId, kids] of subagentIndex) { if (!allTopLevelIds.has(parentId)) { @@ -892,7 +909,7 @@ function rebindSidebarEvents(projects) { archiveBtn.onclick = async (e) => { e.stopPropagation(); const group = header.parentElement; - const sessionItems = group.querySelectorAll('.session-item'); + const sessionItems = group.querySelectorAll('.session-item:not([data-subagent])'); for (const item of sessionItems) { const sid = item.dataset.sessionId; const session = sessionMap.get(sid); diff --git a/test/dom-slug-group-subagent-nesting.test.js b/test/dom-slug-group-subagent-nesting.test.js new file mode 100644 index 00000000..8e8db37d --- /dev/null +++ b/test/dom-slug-group-subagent-nesting.test.js @@ -0,0 +1,208 @@ +// Regression coverage for issue #128 (ask 4) / rehab-plan.md A3+A4 — subagent +// children rendered inside a slug group used to vanish from the sidebar. +// +// buildSlugGroup() appended its sessions via the raw buildSessionItem(session), +// never through appendSubagentChildren — only the top-level (non-grouped) +// render path called that helper. Any session rendered inside a slug group +// therefore lost its subagent caret/children, and — because the group element +// itself carries no data-session-id — the orphan-subagents pass in +// buildSessionsList() didn't recognize the grouped session as "already +// accounted for" either, so its subagents were duplicated into the project's +// "Orphan subagents" bucket instead. +// +// The fixture below reproduces the one real producer of shared slugs across +// top-level sessions in this app: schedule-runner.js's createScheduleSession() +// writes `slug: schedule.slug` on the JSONL's first line for every rerun of the +// same schedule (schedule-runner.js:176). Two reruns of the same schedule share +// a slug and land in session_cache (session-cache.js:buildProjectsFromCache) +// with identical `slug`, distinct `sessionId`/`modified`, and no +// `parentSessionId` — the shape asserted here. + +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const { setupSidebarDom } = require('./dom-setup'); + +function scheduleRerunProject({ subagentParentId = 'sched-run-2', ...overrides } = {}) { + const baseTime = Date.parse('2026-08-20T09:00:00Z'); + const t = (offsetMs) => new Date(baseTime + offsetMs).toISOString(); + // summary as read-session-file.js derives it for a createScheduleSession() + // JSONL: the first user message is `'Scheduled Task: ' + schedule.prompt`, + // which doesn't match the tag pattern, so it + // falls through to text.slice(0, 120). + const summary = 'Scheduled Task: Check Hacker News for the current top article and any posts related to AI docume'; + + return { + projectPath: '/home/dev/hn-watcher', + sessions: [ + { + sessionId: 'sched-run-1', + slug: 'hn-top-articles', + summary, + firstPrompt: summary, + modified: t(-120000), // an earlier rerun + messageCount: 1, + starred: false, + archived: 0, + }, + { + sessionId: 'sched-run-2', + slug: 'hn-top-articles', + summary, + firstPrompt: summary, + modified: t(0), // the current (running) rerun + messageCount: 1, + starred: false, + archived: 0, + }, + { + sessionId: `sub:${subagentParentId}:agent-1`, + parentSessionId: subagentParentId, + agentId: 'agent-1', + subagentType: 'explore', + description: 'explore subagent', + modified: t(-1000), + messageCount: 1, + }, + ], + ...overrides, + }; +} + +test('a session rendered inside a slug group still gets its subagent children attached', () => { + const ctx = setupSidebarDom(); + try { + ctx.window.activePtyIds.add('sched-run-2'); + ctx.sidebar.renderProjects([scheduleRerunProject()], true); + + const group = ctx.document.getElementById('slug-hn-top-articles'); + assert.ok(group, 'the two schedule reruns must be grouped by shared slug'); + assert.ok(group.classList.contains('slug-group'), 'grouped element must carry slug-group'); + + const groupedSessionItems = group.querySelectorAll('.session-item:not([data-subagent])'); + assert.equal(groupedSessionItems.length, 2, 'both reruns render as session-items inside the group'); + + const caret = ctx.document.getElementById('sub-caret-sched-run-2'); + assert.ok(caret, 'subagent caret for the grouped, running rerun must exist'); + assert.ok(group.contains(caret), 'the caret must be nested inside the slug group, next to its parent session-item'); + + const subagentItem = ctx.document.getElementById('si-sub:sched-run-2:agent-1'); + assert.ok(subagentItem, 'the subagent item must be rendered'); + assert.ok(group.contains(subagentItem), 'the subagent item must be nested inside the slug group, not dropped to the orphan bucket'); + } finally { + ctx.destroy(); + } +}); + +test('the grouped session\'s subagent is not duplicated into the project\'s orphan-subagents bucket', () => { + const ctx = setupSidebarDom(); + try { + ctx.window.activePtyIds.add('sched-run-2'); + ctx.sidebar.renderProjects([scheduleRerunProject()], true); + + const orphanGroup = ctx.document.querySelector('.sidebar-orphan-subagents'); + assert.ok(!orphanGroup, 'a subagent whose parent is a grouped session is not an orphan — no orphan bucket should render'); + } finally { + ctx.destroy(); + } +}); + +test('the slug-group dot lights up while one of the grouped, nested sessions has an active PTY', () => { + const ctx = setupSidebarDom(); + try { + ctx.window.activePtyIds.add('sched-run-2'); + ctx.sidebar.renderProjects([scheduleRerunProject()], true); + + const dot = ctx.document.querySelector('#slug-hn-top-articles .slug-group-dot'); + assert.ok(dot, 'slug-group-dot must be rendered'); + assert.ok(dot.classList.contains('running'), 'group dot must reflect that sched-run-2 has an active PTY'); + } finally { + ctx.destroy(); + } +}); + +// PR #134 review F1 — nesting the subagent's caret/children as DOM siblings +// inside the group (the fix above) means group.querySelectorAll('.session-item') +// in the "Archive all sessions in group" handler now also matches the nested +// subagent item (buildSubagentItem's className includes 'session-item' for +// shared styling), so the handler must exclude it the same way every other +// subagent-item consumer in this file does (:not([data-subagent])). +test('"Archive all sessions in group" only archives the two top-level reruns, not the nested subagent', async () => { + const ctx = setupSidebarDom(); + try { + ctx.window.activePtyIds.add('sched-run-2'); + ctx.window.sessionMap.set('sched-run-1', { sessionId: 'sched-run-1', archived: 0 }); + ctx.window.sessionMap.set('sched-run-2', { sessionId: 'sched-run-2', archived: 0 }); + ctx.window.sessionMap.set('sub:sched-run-2:agent-1', { sessionId: 'sub:sched-run-2:agent-1', archived: 0 }); + + const archiveCalls = []; + const stopCalls = []; + ctx.window.api.archiveSession = (sid, val) => { archiveCalls.push([sid, val]); return Promise.resolve({ ok: true }); }; + ctx.window.api.stopSession = (sid) => { stopCalls.push(sid); return Promise.resolve({ ok: true }); }; + + ctx.sidebar.renderProjects([scheduleRerunProject()], true); + + const archiveBtn = ctx.document.querySelector('#slug-hn-top-articles .slug-group-archive-btn'); + assert.ok(archiveBtn, 'slug-group-archive-btn must be rendered'); + await archiveBtn.onclick({ stopPropagation: () => {} }); + + const archivedIds = archiveCalls.map(([sid]) => sid).sort(); + assert.deepEqual(archivedIds, ['sched-run-1', 'sched-run-2'], + 'only the two top-level reruns must be archived — the nested subagent must not reach archiveSession'); + assert.ok(!stopCalls.includes('sub:sched-run-2:agent-1'), + 'the nested subagent must not reach stopSession either'); + } finally { + ctx.destroy(); + } +}); + +// Adversarial review of #134 found buildSlugGroup() calling appendSubagentChildren +// from three separate loops (promoted / rest-under-"more" / all-sessions-else) and +// only the first was exercised — see .ai/contexts/subagent-observability.md. +test('a session in the "+N more" (non-promoted) part of a slug group still gets its subagent children attached', () => { + const ctx = setupSidebarDom(); + try { + ctx.window.activePtyIds.add('sched-run-2'); // only sched-run-2 is promoted; sched-run-1 lands in "rest" + ctx.sidebar.renderProjects([scheduleRerunProject({ subagentParentId: 'sched-run-1' })], true); + + const group = ctx.document.getElementById('slug-hn-top-articles'); + assert.ok(group, 'the two schedule reruns must be grouped by shared slug'); + assert.ok(group.classList.contains('has-promoted'), 'sched-run-2 being active must promote it, pushing sched-run-1 into rest'); + + const olderDiv = ctx.document.getElementById('sgo-slug-hn-top-articles'); + assert.ok(olderDiv, 'the "+N more" container must be rendered since rest.length > 0'); + + const caret = ctx.document.getElementById('sub-caret-sched-run-1'); + assert.ok(caret, 'subagent caret for the non-promoted rerun must exist'); + assert.ok(olderDiv.contains(caret), 'the caret must be nested inside the "+N more" container, next to its parent session-item'); + + const subagentItem = ctx.document.getElementById('si-sub:sched-run-1:agent-1'); + assert.ok(subagentItem, 'the subagent item must be rendered'); + assert.ok(olderDiv.contains(subagentItem), 'the subagent item must be nested inside the "+N more" container, not dropped to the orphan bucket'); + } finally { + ctx.destroy(); + } +}); + +test('a session in a slug group with no active PTY at all still gets its subagent children attached', () => { + const ctx = setupSidebarDom(); + try { + // No session in this group has an active PTY, so buildSlugGroup() takes + // the plain "all sessions" branch rather than the promoted/rest split. + ctx.sidebar.renderProjects([scheduleRerunProject({ subagentParentId: 'sched-run-2' })], true); + + const group = ctx.document.getElementById('slug-hn-top-articles'); + assert.ok(group, 'the two schedule reruns must be grouped by shared slug'); + assert.ok(!group.classList.contains('has-promoted'), 'no session is active, so the group must not be in the promoted state'); + + const caret = ctx.document.getElementById('sub-caret-sched-run-2'); + assert.ok(caret, 'subagent caret must exist even when no session in the group is running'); + assert.ok(group.contains(caret), 'the caret must be nested inside the slug group, next to its parent session-item'); + + const subagentItem = ctx.document.getElementById('si-sub:sched-run-2:agent-1'); + assert.ok(subagentItem, 'the subagent item must be rendered'); + assert.ok(group.contains(subagentItem), 'the subagent item must be nested inside the slug group, not dropped to the orphan bucket'); + } finally { + ctx.destroy(); + } +});