feat(agent): add provider-specific agent detail inventories - #1692
feat(agent): add provider-specific agent detail inventories#1692techotaku39 wants to merge 7 commits into
Conversation
…tails # Conflicts: # web/src/components/SessionChat.tsx
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
-
[Major] Select the session model before persisting Claude's context window —
modelUsageis a model-keyed record that can include the main model and Task/subagent models, but the new fallback takes the first object value. Existing handling explicitly distinguishes the resolved session model from subagent entries (cli/src/claude/utils/sdkToLogConverter.ts:394). If a 200k subagent entry precedes a 1M session entry, persisted metadata can overwrite the correct window and the web fallback will calculate the wrong remaining budget. Evidencecli/src/agent/contextDetails.ts:109
Suggested fix:const modelUsage = asRecord(result?.modelUsage ?? result?.model_usage) const entries = modelUsage ? Object.entries(modelUsage) : [] const selectedUsage = model && modelUsage ? asRecord(modelUsage[model]) : entries.length === 1 ? asRecord(entries[0][1]) : null
Use
selectedUsageforcontextWindow, and pass the last rawsystem/initmodel when a result has nomodel. -
[Minor] Allow authoritative empty inventories to clear persisted entries —
buildCodexContextDetailsomitsskills, commands, and MCP arrays when they are empty, whilemergeContextDetailsretains omitted previous fields. Consequently, the addedskills/changedrefresh atcli/src/codex/codexRemoteLauncher.ts:3498cannot remove the final deleted/disabled skill, so archived and live dialogs continue showing stale capabilities. Evidencecli/src/agent/contextDetails.ts:209
Suggested fix:const codex: CodexContextDetails = { ...(args.slashCommands !== undefined ? { slashCommands: normalizeCommands(args.slashCommands) } : {}), ...(args.skills !== undefined ? { skills: args.skills.filter((skill) => skill.enabled).map(({ name }) => ({ name })) } : {}), ...(args.mcpServers !== undefined ? { mcpServers } : {}) }
Questions
- None.
Summary
- Review mode: initial. Two introduced metadata correctness issues found: ambiguous Claude model selection and stale inventories that cannot be cleared.
Testing
- Not run (automation). Suggested coverage: a multi-entry Claude
modelUsagecase with the main model not first, and a Codexskills/changedrefresh from one skill to an empty list.
HAPI Bot
| const contextUsage = asRecord(args.contextUsage) | ||
| const system = asRecord(args.system) | ||
| const result = asRecord(args.result) | ||
| const resultModelUsage = firstRecord(result?.modelUsage ?? result?.model_usage) |
There was a problem hiding this comment.
[MAJOR] Select the session model before persisting contextWindow
modelUsage can contain both the current session model and Task/subagent models. The fallback below eventually returns the first object value, unlike the existing converter which resolves the main model explicitly (cli/src/claude/utils/sdkToLogConverter.ts:394). A 200k subagent entry can therefore overwrite a 1M main-session window.
Suggested fix:
const modelUsage = asRecord(result?.modelUsage ?? result?.model_usage)
const entries = modelUsage ? Object.entries(modelUsage) : []
const selectedUsage = model && modelUsage
? asRecord(modelUsage[model])
: entries.length === 1
? asRecord(entries[0][1])
: nullUse selectedUsage for the result fallback, and have callers pass the last raw system/init model when result.model is absent.
| })) | ||
| : undefined | ||
| const codex: CodexContextDetails = { | ||
| ...(args.slashCommands && args.slashCommands.length > 0 |
There was a problem hiding this comment.
[MINOR] Preserve explicit empty inventories so refreshes can clear stale data
These conditions omit fields when an authoritative inventory is empty. mergeContextDetails then retains the previous arrays, so the new skills/changed refresh cannot remove the last deleted/disabled skill.
Suggested fix:
const codex: CodexContextDetails = {
...(args.slashCommands !== undefined
? { slashCommands: normalizeCommands(args.slashCommands) }
: {}),
...(args.skills !== undefined
? { skills: args.skills.filter((skill) => skill.enabled).map(({ name }) => ({ name })) }
: {}),
...(args.mcpServers !== undefined ? { mcpServers } : {})
}Add a refresh test that changes skills from one entry to [] and asserts the persisted list is cleared.
There was a problem hiding this comment.
Findings
-
[Major] Merge against the metadata value inside the serialized updater —
publishContextDetailssnapshots and merges beforeupdateMetadataruns. The real client queues update handlers and only advances its cached metadata after the hub ACK, while the local Claude scanner can publish several events synchronously from one scan. Those calls can therefore merge from the same old value; a later usage-only event can overwrite the system/skill inventory queued immediately before it. Evidencecli/src/agent/contextDetails.ts:308
Suggested fix:export function publishContextDetails(client: ContextDetailsClient, next: ContextDetails): void { if (!client.updateMetadata) return client.updateMetadata((metadata) => { const current = metadata.contextDetails const merged = mergeContextDetails(current, next) return merged === current ? metadata : { ...metadata, contextDetails: merged } }) }
-
[Major] Publish the local Codex inventory independently of token events and include skills — the added local path only calls
publishContextDetailsfrom atoken_countevent, and passes commands/MCP but never a skill catalog. Terminal-started Codex uses local mode by default, so Agent details is empty until the first usage record and its Skills section stays empty for the whole local session. Evidencecli/src/codex/codexLocalLauncher.ts:227
Suggested fix:const [commands, discoveredSkills] = await Promise.all([ listSlashCommands('codex', effectiveCodexCwd), listSkills(effectiveCodexCwd, { flavor: 'codex' }) ]) availableSlashCommands = commands.map(({ name }) => name) availableSkills = discoveredSkills.map(({ name }) => ({ name, enabled: true })) publishContextDetails(session.client, buildCodexContextDetails({ slashCommands: availableSlashCommands, skills: availableSkills, mcpServers }))
Widen the builder's skill input to the minimal
{ name; enabled }shape so local discovery does not need fake path/scope fields.
Questions
- None.
Summary
- Review mode: follow-up after new commits. The two prior findings are fixed; two newly identified correctness gaps remain in metadata update ordering and the default local Codex inventory path.
Testing
- Not run (automation; PR code was not executed). Add a queued-updater test that applies two publishers after both are scheduled, plus a local Codex launcher test asserting commands, skills, and MCP metadata exists before any
token_count.
HAPI Bot
|
|
||
| export function publishContextDetails(client: ContextDetailsClient, next: ContextDetails): void { | ||
| if (!client.updateMetadata) return | ||
| const current = client.getMetadata?.()?.contextDetails |
There was a problem hiding this comment.
[MAJOR] Merge inside the serialized metadata updater
This reads and merges contextDetails before updateMetadata runs. ApiSession.updateMetadata queues handlers and updates its cache only after the hub ACK, while the local Claude scanner can invoke this publisher several times synchronously from one scan. Multiple calls can therefore snapshot the same old metadata; a later usage-only event can overwrite the inventory queued immediately before it.
Suggested fix:
client.updateMetadata((metadata) => {
const current = metadata.contextDetails
const merged = mergeContextDetails(current, next)
return merged === current ? metadata : { ...metadata, contextDetails: merged }
})Add a test with a fake client that queues two updater functions, then applies them in order and verifies the final value contains fields from both publishers.
| flushPendingExecWrapper(message.callId, message); | ||
| } | ||
| } else { | ||
| if (message.type === 'token_count') { |
There was a problem hiding this comment.
[MAJOR] Publish a complete local inventory before the first token event
This is the only local Codex publish site. It waits for token_count, and the payload includes commands/MCP but no skills. Because terminal-started Codex defaults to local mode, Agent details is empty before the first response and never shows local Codex skills.
Suggested fix:
const [commands, discoveredSkills] = await Promise.all([
listSlashCommands('codex', effectiveCodexCwd),
listSkills(effectiveCodexCwd, { flavor: 'codex' })
])
availableSlashCommands = commands.map(({ name }) => name)
availableSkills = discoveredSkills.map(({ name }) => ({ name, enabled: true }))
publishContextDetails(session.client, buildCodexContextDetails({
slashCommands: availableSlashCommands,
skills: availableSkills,
mcpServers
}))Widen the builder's skill input to the minimal { name; enabled } shape, and reuse availableSkills in later token updates.
There was a problem hiding this comment.
Findings
- [Major] Publish the resolved Codex MCP inventory, not only HAPI's injected bridge - the current payload omits user/project-configured Codex MCP servers.
- [Major] Normalize the actual Codex token-usage shape - standard
last_token_usage.input_tokensevents do not populate persisted context usage. - [Minor] Preserve connection state in the trigger's accessible name - the explicit label masks offline, permission, thinking, and background-task status.
Questions
- None.
Summary
Review mode: follow-up after new commits. The two findings from the prior HAPI Bot review are fixed. Three issues remain: incomplete Codex MCP inventory, dropped Codex usage for the repository's standard event shape, and an accessibility regression in the connection-status trigger.
Testing
Not run (automation; PR code was not executed). GitHub's test job passed. The runner integration job failed, but the same base commit's main workflow is also failing, so it is not attributed to this diff. Add coverage for a configured non-HAPI MCP server, snake_case last_token_usage.input_tokens, and an accessible name containing the live connection state.
HAPI Bot
| publishContextDetails(session.client, buildCodexContextDetails({ | ||
| slashCommands: slashCommandsLoaded ? availableSlashCommands : undefined, | ||
| skills: skillsLoaded ? availableSkills : undefined, | ||
| mcpServers |
There was a problem hiding this comment.
[MAJOR] Publish a complete Codex MCP inventory
mcpServers here comes directly from buildHapiMcpBridge, which returns only the injected hapi server (buildHapiMcpBridge.ts:126). Codex still loads user/project MCP servers from its normal configuration, so both local and remote launchers persist an incomplete list while the UI labels it "MCP servers". Any non-HAPI server is silently missing from the new capability inventory.
Suggested fix:
const resolvedMcpServers = await loadCodexMcpServers(effectiveCodexCwd)
const contextMcpServers = { ...resolvedMcpServers, ...mcpServers }
publishContextDetails(session.client, buildCodexContextDetails({
slashCommands: slashCommandsLoaded ? availableSlashCommands : undefined,
skills: skillsLoaded ? availableSkills : undefined,
mcpServers: contextMcpServers
}))Use the provider's resolved runtime/config inventory in the remote path too. If a complete inventory is unavailable, omit or explicitly rename this section rather than presenting only the HAPI bridge as authoritative.
|
|
||
| function buildCodexUsage(value: unknown, fallbackContextTokens?: number): ContextUsageSnapshot | undefined { | ||
| const usage = normalizeUsageSnapshot(value) | ||
| if (!usage) return undefined |
There was a problem hiding this comment.
[MAJOR] Accept Codex input_tokens as the current context
The repository's app-server fixture emits last_token_usage: { input_tokens, output_tokens } without context_tokens (codexRemoteLauncher.test.ts:482). normalizeUsageSnapshot returns undefined for that standard shape, and this early return drops usage entirely. Consequently the persisted contextDetails fallback never receives the current Codex context for those events.
Suggested fix:
function buildCodexUsage(value: unknown, fallbackContextTokens?: number): ContextUsageSnapshot | undefined {
const record = asRecord(value)
const normalized = normalizeUsageSnapshot(value)
const contextTokens = normalized?.contextTokens
?? fallbackContextTokens
?? asTokenCount(record?.inputTokens ?? record?.input_tokens)
const cacheReadTokens = normalized?.cacheReadTokens
return contextTokens === undefined && cacheReadTokens === undefined
? undefined
: {
...(contextTokens !== undefined ? { contextTokens } : {}),
...(cacheReadTokens !== undefined ? { cacheReadTokens } : {})
}
}Add a test using the exact snake_case event shape already used by the remote-launcher harness.
| </div> | ||
| <ContextDetailsDialog | ||
| details={displayContextDetails} | ||
| triggerAriaLabel={t('misc.contextAgentDetails')} |
There was a problem hiding this comment.
[MINOR] Keep the connection state in the accessible name
An explicit aria-label overrides the visible child text, so screen readers now hear only "Agent context details" and lose the previous live state: offline, permission required, thinking, or background tasks. That makes the status conversion into a button an accessibility regression.
Suggested fix:
triggerAriaLabel={`${connectionStatus.text}. ${t('misc.contextAgentDetails')}`}Add an assertion that the button's accessible name contains both the current state and the action.
There was a problem hiding this comment.
Findings
- [Major] Keep capability inventory off the Codex startup path — local mode waits for filesystem discovery before
launcher.run(), and remote mode awaitsmcpServerStatus/listwith a 30-second timeout before continuing initialization. These inventories only feed persisted UI metadata, so slow project/CODEX_HOME I/O or a slow app server can delay the actual coding session. Evidence:cli/src/codex/codexLocalLauncher.ts:492,cli/src/codex/codexRemoteLauncher.ts:3641,cli/src/codex/codexAppServerClient.ts:307.
Suggested fix:// Make the CLI inventory helper asynchronous, then publish all inventories in the background. void Promise.all([ listConfiguredCodexMcpServers(effectiveCodexCwd), listSlashCommands('codex', effectiveCodexCwd), listSkills(effectiveCodexCwd, { flavor: 'codex' }) ]).then(([mcpServerInventory, commands, skills]) => { publishContextDetails(session.client, buildCodexContextDetails({ slashCommands: commands.map((command) => command.name), skills: skills.map((skill) => ({ name: skill.name, enabled: true })), mcpServers, mcpServerInventory })) }) return await launcher.run() // Remote status enrichment should also be detached from startup. void appServerClient.listMcpServerStatuses() .then((response) => { const inventory = parseCodexMcpStatusResponse(response) if (inventory.length === 0) return codexMcpServerInventory = mergeCodexMcpInventories(codexMcpServerInventory, inventory) publishCodexInventoryContext?.() }) .catch((error) => { logger.debug(`[Codex] mcpServerStatus/list failed: ${errorMessage(error)}`) })
Questions
- None.
Summary
Review mode: follow-up after new commits. The prior MCP completeness, snake_case usage, and accessible-name findings are resolved. One startup-path regression remains: best-effort inventory collection can delay both local and remote Codex sessions.
Testing
Not run (automation; PR code was not executed). GitHub test and fixture drift checks passed. The runner integration job failed on the unchanged runner suite, matching the base commit failure and not attributed to this diff. Add a launcher test with unresolved inventory promises to assert that Codex starts without waiting.
HAPI Bot
| }); | ||
|
|
||
| try { | ||
| const statusInventory = parseCodexMcpStatusResponse(await appServerClient.listMcpServerStatuses()); |
There was a problem hiding this comment.
[MAJOR] Do not await presentation-only inventory during startup
This request is awaited before the launcher reaches its normal session loop, and listMcpServerStatuses() allows 30 seconds before timing out. A slow app server therefore delays every remote Codex session even though this data only enriches the Agent details dialog. Local mode has the same gating pattern at codexLocalLauncher.ts:492, plus a synchronous codex mcp list probe.
Suggested fix:
void appServerClient.listMcpServerStatuses()
.then((response) => {
const inventory = parseCodexMcpStatusResponse(response)
if (inventory.length === 0) return
codexMcpServerInventory = mergeCodexMcpInventories(codexMcpServerInventory, inventory)
publishCodexInventoryContext?.()
})
.catch((error) => {
logger.debug(`[Codex] mcpServerStatus/list failed: ${errorMessage(error)}`)
})Also make the CLI inventory helper asynchronous and detach the local inventoryTask before calling launcher.run().
There was a problem hiding this comment.
Findings
-
[Major] Keep remote slash-command discovery off the startup path —
listSlashCommands()recursively reads command files from the user and project trees before the app-server is connected, so slow or network-backed filesystems still delay every remote Codex session even after the follow-up detached MCP discovery. Evidence:cli/src/codex/codexRemoteLauncher.ts:3562,cli/src/modules/common/slashCommands.ts:146,cli/src/codex/codexRemoteLauncher.ts:3637.
Suggested fix:void listSlashCommands('codex', session.path) .then((commands) => { if (this.shouldExit) return availableSlashCommands = commands.map((command) => command.name) publishCodexInventoryContext?.() }) .catch((error) => { logger.debug(`[Codex] failed to list slash commands: ${errorMessage(error)}`) })
-
[Minor] Preserve authoritative empty Claude inventories — the normalizers turn explicit empty arrays into
undefined, and the merge logic interprets omitted fields as “keep the previous value.” After the final skill, MCP tool, system tool, or command disappears, archived metadata and the Agent details dialog therefore continue showing stale capabilities. Evidence:cli/src/agent/contextDetails.ts:39,cli/src/agent/contextDetails.ts:93,cli/src/agent/contextDetails.ts:107,cli/src/agent/contextDetails.ts:328.
Suggested fix:// Preserve [] when the source supplied an array; undefined means unavailable. return values // buildClaudeSkills: return skills // buildClaudeMcpTools: return tools
Questions
- None.
Summary
Review mode: follow-up after new commits. The new commit detaches local capability discovery and remote MCP status enrichment, but remote slash-command discovery still gates startup. Claude inventory clearing also remains incorrect for explicit empty lists.
Testing
Not run (automation; PR code was not executed). GitHub test and fixture drift-gate checks passed. The unchanged runner integration suite currently fails four assertions, as in the prior bot pass. Add a remote-launcher test with an unresolved slash-command promise, plus a merge test proving empty Claude inventories replace prior non-empty values.
HAPI Bot
| }); | ||
| this.happyServer = happyServer; | ||
| try { | ||
| availableSlashCommands = (await listSlashCommands('codex', session.path)).map((command) => command.name); |
There was a problem hiding this comment.
[MAJOR] Keep slash-command discovery off remote startup
This await occurs before appServerClient.connect() and listSlashCommands() recursively reads every command markdown file in the user/project command trees. Slow or network-backed storage can therefore still delay the coding session for presentation-only metadata; the follow-up test only covers the detached MCP status request.
Suggested fix:
void listSlashCommands('codex', session.path)
.then((commands) => {
if (this.shouldExit) return
availableSlashCommands = commands.map((command) => command.name)
publishCodexInventoryContext?.()
})
.catch((error) => {
logger.debug(`[Codex] failed to list slash commands: ${errorMessage(error)}`)
})| const values = value | ||
| .map(asString) | ||
| .filter((value): value is string => value !== undefined) | ||
| return values.length > 0 ? values : undefined |
There was a problem hiding this comment.
[MINOR] Preserve explicit empty Claude inventories
Returning undefined for an explicit [] makes mergeContextDetails() retain the previous list. The same pattern exists for skills and MCP tools, so removing the final command/tool/skill leaves stale capabilities persisted and displayed.
Suggested fix:
// Explicit [] is authoritative; only non-arrays mean unavailable.
return values
// buildClaudeSkills:
return skills
// buildClaudeMcpTools:
return toolsAdd a merge test where non-empty Claude lists are followed by explicit empty arrays.
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
Review mode: follow-up after new commits. No high-confidence issues found on the current head. The previous startup-blocking slash-command discovery and stale-empty-inventory findings are resolved. Residual risk remains around live Codex MCP status payload compatibility and cross-platform subprocess cleanup, which are covered only by mocked/unit paths in this PR.
Testing
Not run (automation; PR code was not executed). GitHub drift-gate passed. The integration job fails the same four unchanged runner assertions recorded in the prior bot review; this PR does not modify runner source or integration tests. The main test workflow was pending at review time.
HAPI Bot
1 participant
Summary
User impact
Users can inspect the capabilities available to Claude Code and Codex from the connection status entry. Saved details remain available after a session is archived.
This change currently covers Claude Code and Codex only. Other Agent providers are unchanged and do not publish
contextDetails.Validation
bun typecheck— passedterminal-wrap-fidelity.spec.ts— 2 passedStatusBar.popover.test.tsx— 13 passedbun run build— passedgit diff --check— passedtest,drift-gate, and Codex PR review checks — passedData and migration
No database migration is required. Existing metadata is compacted when a later
contextDetailsupdate is published.Related Issues
None
AI assistance
OpenAI Codex (GPT-5.6) was used for implementation, testing, investigation, and PR preparation.