Add Codex import resume flow#976
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
- [Major] Codex transcript listing/import bypasses workspace roots — the RPC validates only the requested
cwd, then returns every transcript fromCODEX_HOME, including full message bodies whensessionIdsis present./api/codex/sessionscan also call it with nocwd, so a runner started with scopedworkspaceRootsstill exposes/imports transcripts from unrelated directories. Evidencecli/src/api/apiMachine.ts:226.
Suggested fix:const sessionIsAllowed = async (session: { cwd?: string | null }) => { if (!this.normalizedWorkspaceRoots?.length) return true if (!session.cwd?.trim()) return false const resolved = await this.resolveForWorkspaceCheck(session.cwd) return this.isWithinWorkspaceRoots(resolved) } const allSessions = requestedIds ? listLocalCodexSessionsWithMessages() : listLocalCodexSessionSummaries() const sessions = [] for (const session of allSessions) { if (requestedIds && !requestedIds.has(session.id)) continue if (!(await sessionIsAllowed(session))) continue sessions.push(session) } return { success: true, sessions }
- [Major] Archive accepts any local Codex session id —
archiveLocalCodexSession()is called with onlysessionId, so a client can move transcripts outside the runner workspace roots once it knows or guesses the id. This is a data-loss boundary issue independent of the list endpoint. Evidencecli/src/api/apiMachine.ts:239.
Suggested fix:const result = await archiveLocalCodexSession(sessionId, { canArchive: async (session) => { if (!this.normalizedWorkspaceRoots?.length) return true if (!session.cwd?.trim()) return false const resolved = await this.resolveForWorkspaceCheck(session.cwd) return this.isWithinWorkspaceRoots(resolved) } }) return result.success ? result : { success: false, error: result.error }
- [Minor] New scanner reintroduces subagent transcripts — the old hub parser skipped
session_meta.payload.sourceobjects that containsubagent, but the new shared scanner only copies stringsourceand never filters them. Those internal subagent threads can now appear in the import dialog as normal sessions. Evidencecli/src/modules/common/codexSessions.ts:290.
Suggested fix:function isSubagentSource(value: unknown): boolean { const record = asRecord(value) return record ? Object.prototype.hasOwnProperty.call(record, 'subagent') : false } if (record.type === 'session_meta') { const payload = asRecord(record.payload) if (isSubagentSource(payload?.source)) return null // existing field extraction... }
Summary
Review mode: initial. The main risk is that the new machine-side Codex transcript RPC moved local transcript access out to the runner but did not preserve workspace-root scoping for the returned sessions or archive operation. Also, the new scanner dropped the prior subagent filter.
Testing
Not run (automation). Suggested additions: RPC tests where CODEX_HOME has one transcript under an allowed cwd and one outside it, covering list-by-cwd, import-by-sessionId, and archive-by-sessionId; plus a scanner test for source: { subagent: ... }.
HAPI Bot
| ? new Set(params.sessionIds.filter((id): id is string => typeof id === 'string' && id.trim().length > 0)) | ||
| : null | ||
| const sessions = requestedIds | ||
| ? listLocalCodexSessionsWithMessages().filter((session) => requestedIds.has(session.id)) |
There was a problem hiding this comment.
[Major] Codex transcript listing/import bypasses workspace roots
The handler validates only params.cwd, but then listLocalCodexSessionsWithMessages() / listLocalCodexSessionSummaries() returns every transcript in CODEX_HOME. When /api/codex/sessions calls this with no cwd, there is no workspace check at all; when import calls it with sessionIds, the full messages for outside-root transcripts can be returned and imported.
Suggested fix:
const sessionIsAllowed = async (session: { cwd?: string | null }) => {
if (!this.normalizedWorkspaceRoots?.length) return true
if (!session.cwd?.trim()) return false
const resolved = await this.resolveForWorkspaceCheck(session.cwd)
return this.isWithinWorkspaceRoots(resolved)
}
const allSessions = requestedIds
? listLocalCodexSessionsWithMessages()
: listLocalCodexSessionSummaries()
const sessions = []
for (const session of allSessions) {
if (requestedIds && !requestedIds.has(session.id)) continue
if (!(await sessionIsAllowed(session))) continue
sessions.push(session)
}
return { success: true, sessions }| if (!sessionId) { | ||
| return { success: false, error: 'sessionId is required' } | ||
| } | ||
| return archiveLocalCodexSession(sessionId) |
There was a problem hiding this comment.
[Major] Archive accepts any local Codex session id
This archive RPC only checks that sessionId is non-empty, then moves the matching transcript from local Codex storage. That bypasses the runner workspace roots even if the list endpoint is fixed, because a client can submit an outside-root session id directly and cause data loss by moving that transcript.
Suggested fix:
const result = await archiveLocalCodexSession(sessionId, {
canArchive: async (session) => {
if (!this.normalizedWorkspaceRoots?.length) return true
if (!session.cwd?.trim()) return false
const resolved = await this.resolveForWorkspaceCheck(session.cwd)
return this.isWithinWorkspaceRoots(resolved)
}
})
return result.success ? result : { success: false, error: result.error }| if (!cwd && typeof payload?.cwd === 'string') cwd = payload.cwd | ||
| if (!originator && typeof payload?.originator === 'string') originator = payload.originator | ||
| if (!cliVersion && typeof payload?.cli_version === 'string') cliVersion = payload.cli_version | ||
| if (!source && typeof payload?.source === 'string') source = payload.source |
There was a problem hiding this comment.
[Minor] New scanner reintroduces subagent transcripts
The previous hub-side parser filtered session_meta.payload.source objects containing subagent; this new shared scanner only reads string source and continues, so subagent transcripts can appear as importable top-level sessions.
Suggested fix:
function isSubagentSource(value: unknown): boolean {
const record = asRecord(value)
return record ? Object.prototype.hasOwnProperty.call(record, subagent) : false
}
if (record.type === session_meta) {
const payload = asRecord(record.payload)
if (isSubagentSource(payload?.source)) return null
// existing field extraction...
}
There was a problem hiding this comment.
Findings
- [Major] Codex transcript listing/import bypasses workspace roots — the RPC validates only the requested
cwd, then returns every transcript fromCODEX_HOME, including full message bodies whensessionIdsis present. Evidencecli/src/api/apiMachine.ts:226.
Suggested fix:const sessionIsAllowed = async (session: { cwd?: string | null }) => { if (!this.normalizedWorkspaceRoots?.length) return true if (!session.cwd?.trim()) return false const resolved = await this.resolveForWorkspaceCheck(session.cwd) return this.isWithinWorkspaceRoots(resolved) } const allSessions = requestedIds ? listLocalCodexSessionsWithMessages() : listLocalCodexSessionSummaries() const sessions = [] for (const session of allSessions) { if (requestedIds && !requestedIds.has(session.id)) continue if (!(await sessionIsAllowed(session))) continue sessions.push(session) } return { success: true, sessions }
- [Major] Archive accepts any local Codex session id — the RPC only validates a non-empty id, then moves the matching transcript from local Codex storage. This bypasses runner workspace roots even if listing is fixed. Evidence
cli/src/api/apiMachine.ts:239.
Suggested fix:const result = await archiveLocalCodexSession(sessionId, { canArchive: async (session) => { if (!this.normalizedWorkspaceRoots?.length) return true if (!session.cwd?.trim()) return false const resolved = await this.resolveForWorkspaceCheck(session.cwd) return this.isWithinWorkspaceRoots(resolved) } }) return result.success ? result : { success: false, error: result.error }
- [Minor] New scanner reintroduces subagent transcripts — the previous parser skipped
session_meta.payload.sourceobjects containingsubagent, but this shared scanner only copies stringsource, so internal subagent transcripts can appear as importable top-level sessions. Evidencecli/src/modules/common/codexSessions.ts:290.
Suggested fix:function isSubagentSource(value: unknown): boolean { const record = asRecord(value) return record ? Object.prototype.hasOwnProperty.call(record, 'subagent') : false } if (record.type === 'session_meta') { const payload = asRecord(record.payload) if (isSubagentSource(payload?.source)) return null // existing field extraction... }
Summary
Review mode: follow-up after new commits. The follow-up commit only changed runtime active-state persistence and its test; the previously reported Codex transcript boundary issues remain present at the current head. The main risk is still that runner-scoped machines can expose/import/archive Codex transcripts outside configured workspace roots.
Testing
Not run (automation). Suggested additions: machine RPC tests with one allowed and one outside-root Codex transcript for list, import-by-id, and archive-by-id; scanner test for source: { subagent: ... }.
HAPI Bot
| ? new Set(params.sessionIds.filter((id): id is string => typeof id === 'string' && id.trim().length > 0)) | ||
| : null | ||
| const sessions = requestedIds | ||
| ? listLocalCodexSessionsWithMessages().filter((session) => requestedIds.has(session.id)) |
There was a problem hiding this comment.
[Major] Codex transcript listing/import bypasses workspace roots
The handler checks only params.cwd, then listLocalCodexSessionsWithMessages() / listLocalCodexSessionSummaries() returns every transcript in CODEX_HOME. /api/codex/sessions can call this without a cwd, and import-by-sessionIds returns full messages for matching outside-root transcripts.
Suggested fix:
const sessionIsAllowed = async (session: { cwd?: string | null }) => {
if (!this.normalizedWorkspaceRoots?.length) return true
if (!session.cwd?.trim()) return false
const resolved = await this.resolveForWorkspaceCheck(session.cwd)
return this.isWithinWorkspaceRoots(resolved)
}
const allSessions = requestedIds
? listLocalCodexSessionsWithMessages()
: listLocalCodexSessionSummaries()
const sessions = []
for (const session of allSessions) {
if (requestedIds && !requestedIds.has(session.id)) continue
if (!(await sessionIsAllowed(session))) continue
sessions.push(session)
}
return { success: true, sessions }| if (!sessionId) { | ||
| return { success: false, error: 'sessionId is required' } | ||
| } | ||
| return archiveLocalCodexSession(sessionId) |
There was a problem hiding this comment.
[Major] Archive accepts any local Codex session id
This RPC only checks that sessionId is non-empty before moving the matching transcript. A client that knows or guesses an outside-root Codex session id can archive it, which is a data-loss boundary issue independent of the list endpoint.
Suggested fix:
const result = await archiveLocalCodexSession(sessionId, {
canArchive: async (session) => {
if (!this.normalizedWorkspaceRoots?.length) return true
if (!session.cwd?.trim()) return false
const resolved = await this.resolveForWorkspaceCheck(session.cwd)
return this.isWithinWorkspaceRoots(resolved)
}
})
return result.success ? result : { success: false, error: result.error }| if (!cwd && typeof payload?.cwd === 'string') cwd = payload.cwd | ||
| if (!originator && typeof payload?.originator === 'string') originator = payload.originator | ||
| if (!cliVersion && typeof payload?.cli_version === 'string') cliVersion = payload.cli_version | ||
| if (!source && typeof payload?.source === 'string') source = payload.source |
There was a problem hiding this comment.
[Minor] New scanner reintroduces subagent transcripts
The old parser filtered session_meta.payload.source objects containing subagent; this new shared scanner only reads string source and continues. Internal subagent transcripts can therefore show up as importable top-level sessions.
Suggested fix:
function isSubagentSource(value: unknown): boolean {
const record = asRecord(value)
return record ? Object.prototype.hasOwnProperty.call(record, 'subagent') : false
}
if (record.type === 'session_meta') {
const payload = asRecord(record.payload)
if (isSubagentSource(payload?.source)) return null
// existing field extraction...
}
There was a problem hiding this comment.
Findings
- No issues found.
Summary
- Review mode: follow-up after new commits. Re-reviewed the full current diff at this head and used the prior HAPI Bot comments as follow-up context. The previously reported Codex transcript workspace-boundary and subagent-listing issues appear addressed in the current head (
cli/src/api/apiMachine.ts,cli/src/modules/common/codexSessions.ts). Residual risk: this is a large cross-package change around Codex transcript import/resume/archive flows, so end-to-end behavior still depends on runner/Web integration beyond static review.
Testing
- Not run (automation). Suggested coverage to keep:
bun typecheck && bun run test, plus the added Codex transcript/import route and web dialog tests.
HAPI Bot
sleepinginsummer
changed the title
…me-ui # Conflicts: # cli/src/api/apiMachine.test.ts
There was a problem hiding this comment.
Findings
- [Major] Preserve omitted model fields during manual Codex sync —
hub/src/web/routes/codexDesktop.ts:1530 - [Major] Do not cap explicit Codex session imports to the newest 200 transcripts —
cli/src/api/apiMachine.ts:228
Summary
- Review mode: follow-up after new commits. Re-reviewed the full latest diff at this head and used the prior HAPI Bot review as context. The previous workspace-boundary/subagent concerns appear addressed, but the current diff introduces two correctness regressions in the Codex import/sync path.
Testing
- Not run (automation). Suggested coverage: a manual sync test that omits
model/modelReasoningEffortand preserves existing preferences, plus an RPC/import test for a requested Codex session ID outside the default scan window.
HAPI Bot
| sessionIds: Array.from(new Set(sessionIds)), | ||
| cwd: typeof bodyRecord.cwd === 'string' && bodyRecord.cwd.trim() ? bodyRecord.cwd.trim() : null, | ||
| machineId: typeof bodyRecord.machineId === 'string' && bodyRecord.machineId.trim() ? bodyRecord.machineId.trim() : null, | ||
| model: typeof bodyRecord.model === 'string' && bodyRecord.model.trim() ? bodyRecord.model.trim() : null, |
There was a problem hiding this comment.
[MAJOR] parseSyncSessionRequest normalizes omitted model and modelReasoningEffort fields to null, but the existing-session import branch treats any non-undefined value as explicit and writes it back. Manual sync callers such as SessionHeader send only sessionIds/cwd/machineId, so syncing an existing Codex session clears its stored model and reasoning-effort preferences.
Suggested fix:
const hasModel = Object.prototype.hasOwnProperty.call(bodyRecord, 'model')
const hasModelReasoningEffort = Object.prototype.hasOwnProperty.call(bodyRecord, 'modelReasoningEffort')
return {
sessionIds: Array.from(new Set(sessionIds)),
cwd: typeof bodyRecord.cwd === 'string' && bodyRecord.cwd.trim() ? bodyRecord.cwd.trim() : null,
machineId: typeof bodyRecord.machineId === 'string' && bodyRecord.machineId.trim() ? bodyRecord.machineId.trim() : null,
model: hasModel
? (typeof bodyRecord.model === 'string' && bodyRecord.model.trim() ? bodyRecord.model.trim() : null)
: undefined,
modelReasoningEffort: hasModelReasoningEffort
? (typeof bodyRecord.modelReasoningEffort === 'string' && bodyRecord.modelReasoningEffort.trim() ? bodyRecord.modelReasoningEffort.trim() : null)
: undefined,
yolo: bodyRecord.yolo === true
}| ? new Set(params.sessionIds.filter((id): id is string => typeof id === 'string' && id.trim().length > 0)) | ||
| : null | ||
| const allSessions = requestedIds | ||
| ? listLocalCodexSessionsWithMessages().filter((session) => requestedIds.has(session.id)) |
There was a problem hiding this comment.
[MAJOR] Explicit imports by sessionIds still call listLocalCodexSessionsWithMessages() with its default 200-session limit before filtering. A manual sync/resume for an older Codex thread can therefore return an empty RPC result and fail with “Transcript not found” even though the caller provided the exact session ID.
Suggested fix:
const allSessions = requestedIds
? listLocalCodexSessionsWithMessages(Number.MAX_SAFE_INTEGER)
.filter((session) => requestedIds.has(session.id))
: listLocalCodexSessionSummaries()
There was a problem hiding this comment.
Findings
- [Major] Surface Codex history RPC failures instead of returning an empty success —
hub/src/web/routes/codexDesktop.ts:1824 - [Major] Send the selected machine when loading Codex import history —
web/src/components/NewSession/index.tsx:483
Summary
- Review mode: follow-up after new commits. Re-reviewed the full latest diff at this head. The previous bot findings are addressed, but the new Codex import listing path still has two correctness issues around runner selection/error reporting.
Testing
- Not run (automation). Suggested coverage:
GET /api/codex/sessionspropagating machine/RPC failures, and New Session Codex import history loading with two online machines where the selected machine must be used.
HAPI Bot
| namespace: c.get('namespace'), | ||
| cwd | ||
| }) | ||
| return c.json({ |
There was a problem hiding this comment.
[MAJOR] listCodexSessionsViaMachine can return remote.error for expected failure modes such as no online runner, ambiguous workspace ownership, or an RPC error, but this route always returns success: true with remote.sessions. The dialog then renders an empty history list, so users cannot distinguish “no Codex history” from “HAPI could not reach the machine that owns it,” and the New Session import flow is blocked without a useful error.
Suggested fix:
const remote = await listCodexSessionsViaMachine({
engine: options.getSyncEngine(),
namespace: c.get('namespace'),
cwd,
machineId
})
if (remote.error) {
return c.json({
success: false,
error: remote.error,
sessions: [],
...(remote.machineId ? { machineId: remote.machineId } : {})
}, 503)
}
return c.json({
success: true,
sessions: remote.sessions.map(({ messages: _messages, ...summary }) => summary),
...(remote.machineId ? { machineId: remote.machineId } : {})
})| setIsLoadingCodexImportSessions(true) | ||
| setCodexImportError(null) | ||
| try { | ||
| const result = await props.api.getCodexSessions(trimmedDirectory || null) |
There was a problem hiding this comment.
[MAJOR] New Session has an explicit selected machineId, but the Codex history load only sends cwd. In multi-runner setups with an empty directory, overlapping workspace roots, or a path that does not uniquely identify one runner yet, the hub can pick a different machine or return no machine even though the user already selected the target machine. That makes “Choose history” show the wrong/empty Codex history for the selected runner.
Suggested fix:
// caller
const result = await props.api.getCodexSessions(trimmedDirectory || null, machineId)
// API client
async getCodexSessions(cwd?: string | null, machineId?: string | null): Promise<CodexLocalSessionsResponse> {
const params = new URLSearchParams()
if (cwd?.trim()) params.set('cwd', cwd.trim())
if (machineId?.trim()) params.set('machineId', machineId.trim())
const query = params.size ? `?${params.toString()}` : ''
return await this.request<CodexLocalSessionsResponse>(`/api/codex/sessions${query}`)
}
// route
const machineId = c.req.query('machineId')?.trim() || null
const remote = await listCodexSessionsViaMachine({
engine: options.getSyncEngine(),
namespace: c.get('namespace'),
cwd,
machineId
})
There was a problem hiding this comment.
Findings
- No issues found.
Summary
- Review mode: follow-up after new commits. Re-reviewed the full latest diff at this head; the previous bot findings about Codex history RPC error handling and selected-machine propagation are addressed.
- Residual risk: this PR touches the Codex import/resume path across CLI, hub RPC, store metadata, and web UI. Coverage is broad, but there is still integration risk around real multi-runner setups and live Codex app-server transcript formats.
Testing
- Not run (automation). Suggested coverage: an end-to-end/manual pass with two online runners, importing a Codex Desktop thread from New Session, resuming it, then using manual sync after the resumed thread forks.
HAPI Bot
There was a problem hiding this comment.
Findings
- [Major] Persistent generated-image cache is unbounded across CLI restarts —
evictOldGeneratedImages()only sees the current process map, so every process restart forgets previously persisted files whilepersistGeneratedImage()keeps adding up to 25 MB per image underCODEX_HOME/generated-images. A user who displays/generated images over multiple sessions can grow this cache without hittingMAX_GENERATED_IMAGE_TOTAL_BYTES, eventually filling the HAPI/Codex data directory. Evidence:cli/src/modules/common/generatedImages.ts:85.
Suggested fix:function persistGeneratedImage(image: GeneratedImageMetadata): void { try { const cacheDir = getGeneratedImageCacheDir() mkdirSync(cacheDir, { recursive: true }) writeFileSync(join(cacheDir, `${image.id}.bin`), image.content) writeFileSync(join(cacheDir, `${image.id}.json`), JSON.stringify({ id: image.id, fileName: image.fileName, mimeType: image.mimeType, createdAt: image.createdAt })) evictPersistedGeneratedImages(cacheDir) } catch { // cache only; fall back to in-memory behavior } }
Summary
- Review mode: follow-up after new commits. Re-reviewed the full latest diff; the new generated-image persistence needs persisted-cache eviction, not just in-memory eviction.
Testing
- Not run (automation). Suggested coverage: restart/clear the in-memory cache after registering enough images to exceed
MAX_GENERATED_IMAGE_TOTAL_BYTES, then register another image and assert old persisted.bin/.jsonfiles are pruned.
HAPI Bot
| } | ||
| generatedImages.set(args.id, metadata) | ||
| generatedImageBytes += content.byteLength | ||
| persistGeneratedImage(metadata) |
There was a problem hiding this comment.
[Major] Persistent generated-image cache is unbounded across CLI restarts.
evictOldGeneratedImages() only tracks generatedImages and generatedImageBytes for the current process. After restart, those counters reset, but previously written .bin/.json files remain in CODEX_HOME/generated-images; this line then persists more images without applying MAX_GENERATED_IMAGE_TOTAL_BYTES to the on-disk cache. Repeated image display/generation can fill the user’s HAPI/Codex data directory.
Suggested fix:
function persistGeneratedImage(image: GeneratedImageMetadata): void {
try {
const cacheDir = getGeneratedImageCacheDir()
mkdirSync(cacheDir, { recursive: true })
writeFileSync(join(cacheDir, `${image.id}.bin`), image.content)
writeFileSync(join(cacheDir, `${image.id}.json`), JSON.stringify({
id: image.id,
fileName: image.fileName,
mimeType: image.mimeType,
createdAt: image.createdAt
}))
evictPersistedGeneratedImages(cacheDir)
} catch {
// cache only; fall back to in-memory behavior
}
}
sleepinginsummer
force-pushed
the
fix/codex-import-resume-ui
branch
from
bd3ef03 to
92c6a3a
Compare
There was a problem hiding this comment.
Findings
- [Major] Re-syncing an imported source clobbers the current fork thread id — the new
codexSourceSessionIdmatch lets the original transcript update the already-forked HAPI session, butbuildImportedSessionMetadata()still writescodexSessionId: data.id. For a session whose metadata is{ codexSessionId: 'fork-session-id', codexSourceSessionId: originalId }, syncingoriginalIdnow selects that session and rewritescodexSessionIdback to the original source. The next resume then treats the source as current and forks again instead of continuing the existing fork. Evidence:hub/src/web/routes/codexDesktop.ts:961with context athub/src/web/routes/codexDesktop.ts:840.
Suggested fix:const currentCodexSessionId = typeof existingMetadata?.codexSessionId === 'string' ? existingMetadata.codexSessionId : data.id return { ...(existingMetadata ?? {}), // ... codexSessionId: currentCodexSessionId, codexSourceSessionId: typeof existingMetadata?.codexSourceSessionId === 'string' ? existingMetadata.codexSourceSessionId : data.id, }
Questions
- None.
Summary
- Review mode: follow-up after new commits. Re-reviewed the full latest diff, using the prior bot review and compare diff as context. The previous generated-image cache finding is no longer in this diff, but the new source/fork import path can regress Codex resume by overwriting the fork id during re-sync.
Testing
- Not run (automation). Suggested coverage: extend
updates an existing forked import when syncing the original Codex session idto assert the storedcodexSessionIdremainsfork-session-idwhilecodexSourceSessionIdremains the original id.
HAPI Bot
| .filter((candidate) => candidate.metadata?.codexSessionId === codexSessionId) | ||
| .filter((candidate) => ( | ||
| candidate.metadata?.codexSessionId === codexSessionId | ||
| || candidate.metadata?.codexSourceSessionId === codexSessionId |
There was a problem hiding this comment.
[Major] Re-syncing an imported source clobbers the current fork thread id.
This added codexSourceSessionId match makes the original transcript select an already-forked HAPI session, but buildImportedSessionMetadata() still writes codexSessionId: data.id. For metadata like { codexSessionId: 'fork-session-id', codexSourceSessionId: originalId }, syncing originalId rewrites codexSessionId back to the source. The next Codex resume then sees the source as current and forks again instead of continuing the existing fork.
Suggested fix:
const currentCodexSessionId = typeof existingMetadata?.codexSessionId === 'string'
? existingMetadata.codexSessionId
: data.id
return {
...(existingMetadata ?? {}),
// ...
codexSessionId: currentCodexSessionId,
codexSourceSessionId: typeof existingMetadata?.codexSourceSessionId === 'string'
? existingMetadata.codexSourceSessionId
: data.id,
}
There was a problem hiding this comment.
Findings
- [Major] Duplicate cleanup misses forked imports by source thread id — this diff starts preserving the original Codex thread as
codexSourceSessionIdwhilecodexSessionIdbecomes the fork id, and import targeting was updated to match either id. The duplicate detector/merge path still groups only bymetadata.codexSessionId, so after an imported source is resumed/forked, re-importing the original source can update that session but/codex/duplicate-sessionswill not report duplicates for the selected original id. Users who already created a separate duplicate for the source lose the new cleanup path. Evidence:hub/src/web/routes/codexDesktop.ts:964and related grouping athub/src/web/routes/codexDesktop.ts:1017.
Suggested fix:function getCodexImportIds(metadata: Record<string, unknown> | null): string[] { return [metadata?.codexSessionId, metadata?.codexSourceSessionId] .filter((id): id is string => typeof id === 'string' && id.length > 0) } for (const candidate of collectImportCandidates(store, namespace, getSyncEngine)) { for (const codexSessionId of getCodexImportIds(candidate.metadata)) { if (!requestedSessionIds.has(codexSessionId)) continue const existing = groups.get(codexSessionId) if (existing) existing.push(candidate) else groups.set(codexSessionId, [candidate]) } }
Questions
- None.
Summary
- Review mode: follow-up after new commits. Re-reviewed the full latest diff after the prior bot finding was addressed. One remaining regression: the new source/fork identity model is only applied to import targeting, not duplicate detection/merge.
Testing
- Not run (automation). Suggested coverage: add a Codex desktop route test where one HAPI session has
{ codexSessionId: 'fork-session-id', codexSourceSessionId: originalId }and another hascodexSessionId: originalId;duplicate-sessionsfororiginalIdshould return both ids and merge should consolidate them.
HAPI Bot
| .filter((candidate) => candidate.metadata?.codexSessionId === codexSessionId) | ||
| .filter((candidate) => ( | ||
| candidate.metadata?.codexSessionId === codexSessionId | ||
| || candidate.metadata?.codexSourceSessionId === codexSessionId |
There was a problem hiding this comment.
[Major] Duplicate cleanup misses forked imports by source thread id.
This source-aware match updates re-imports for forked sessions, but the duplicate detector still groups candidates only by metadata.codexSessionId. Once the imported source is resumed and codexSessionId becomes the fork id, selecting the original source id can update this session but /codex/duplicate-sessions will not report it alongside any duplicate session still keyed by the original id.
Suggested fix:
function getCodexImportIds(metadata: Record<string, unknown> | null): string[] {
return [metadata?.codexSessionId, metadata?.codexSourceSessionId]
.filter((id): id is string => typeof id === 'string' && id.length > 0)
}
for (const candidate of collectImportCandidates(store, namespace, getSyncEngine)) {
for (const codexSessionId of getCodexImportIds(candidate.metadata)) {
if (!requestedSessionIds.has(codexSessionId)) continue
const existing = groups.get(codexSessionId)
if (existing) existing.push(candidate)
else groups.set(codexSessionId, [candidate])
}
}
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
Review mode: follow-up after new commits. Re-reviewed the full latest diff, including the new source/fork duplicate grouping fix added after the prior HAPI Bot review. No high-confidence issues found in the changed Codex import/resume/archive flow.
Testing
- Not run (automation):
bunis not installed in this runner (bun: command not found). Reviewed the added/updated tests statically, including Codex transcript import, duplicate source/fork grouping, CLI resume args, and dialog/menu coverage.
HAPI Bot
There was a problem hiding this comment.
Findings
- [Minor] Archive failures are dropped instead of shown to the user — the archive menu starts
handleArchivewithvoid, whilehandleArchiveawaitsonArchiveSessionwithout a catch. Both callers throw when/api/codex/archive-sessionreturns{ success: false }, so an RPC failure becomes an unhandled rejection and the user gets no error feedback while the row remains visible. Evidenceweb/src/components/CodexSessionSyncDialog.tsx:410.
Suggested fix:const [archiveError, setArchiveError] = useState<string | null>(null) const handleArchive = async (session: CodexLocalSessionSummary) => { if (!onArchiveSession || isPending || isLoading) return closeArchiveMenu() setArchiveError(null) try { await onArchiveSession(session) } catch (error) { setArchiveError(error instanceof Error ? error.message : t('codexSync.failed.body')) } }
Questions
- None.
Summary
Review mode: follow-up after new commits. Re-reviewed the full latest diff for the Codex import/resume/archive flow and found one minor error-handling regression in the new archive affordance.
Testing
- Not run (automation).
HAPI Bot
| <button | ||
| type="button" | ||
| className="block w-full rounded px-3 py-2 text-left text-sm text-[var(--app-fg)] hover:bg-[var(--app-subtle-bg)]" | ||
| onClick={() => void handleArchive(session)} |
There was a problem hiding this comment.
[MINOR] Archive failures are dropped here. handleArchive awaits onArchiveSession, and both current callers throw when the archive RPC returns { success: false }, but this click handler starts it with void, so failures become unhandled rejections with no user-visible feedback.
Suggested fix:
const [archiveError, setArchiveError] = useState<string | null>(null)
const handleArchive = async (session: CodexLocalSessionSummary) => {
if (!onArchiveSession || isPending || isLoading) return
closeArchiveMenu()
setArchiveError(null)
try {
await onArchiveSession(session)
} catch (error) {
setArchiveError(error instanceof Error ? error.message : t('codexSync.failed.body'))
}
}
There was a problem hiding this comment.
Findings
- [Major]
--existing-session-idis injected into every runner spawn —buildCliArgs()is shared by Claude/Cursor/Kimi/OpenCode/Pi as well as Codex, but this PR only teaches the Codex command parser about--existing-session-id. BecauseresumeSession()now passesexistingSessionIdthrough the generic spawn path, resumed non-Codex sessions receive an unknown/internal flag and can fail or stop attaching to the existing HAPI session. Evidencecli/src/runner/run.ts:1107.
Suggested fix:if (agent === 'codex') { const existingSessionId = options.existingSessionId ?? options.sessionId if (existingSessionId) { args.push('--existing-session-id', existingSessionId) } }
Questions
- None.
Summary
Review mode: follow-up after new commits. Re-reviewed the full latest diff for the Codex import/resume flow and found one Major regression in shared runner argument construction. The prior archive error-handling finding appears fixed in the new commits.
Testing
- Not run:
bunis not installed in this environment (bun: command not found).
HAPI Bot
| } | ||
| } | ||
| args.push('--hapi-starting-mode', 'remote', '--started-by', 'runner'); | ||
| if (options.existingSessionId) { |
There was a problem hiding this comment.
[MAJOR] This flag is now emitted for every agent, but only hapi codex parses --existing-session-id. buildCliArgs() is used for Claude/Cursor/Kimi/OpenCode/Pi spawns too, and resumeSession() passes existingSessionId through the generic spawn path, so non-Codex resumes can receive an unknown/internal flag instead of resuming cleanly or attaching to the existing HAPI session.
Suggested fix:
if (agent === 'codex') {
const existingSessionId = options.existingSessionId ?? options.sessionId
if (existingSessionId) {
args.push('--existing-session-id', existingSessionId)
}
}If all agents need this behavior, add parser support for this flag in each command before emitting it globally.
There was a problem hiding this comment.
Findings
- [Major] Imported Codex sessions can lose the selected runner binding —
/codex/sync-sessionresolves the exact runner viamachineIdand imports transcript data from it, but that resolvedremote.machineIdis not passed intoimportSelectedCodexSessions(). The created HAPI session then recomputesmachineIdfromtranscript.cwd; if multiple online runners own overlapping workspace roots,resolveImportMachineId()returnsundefined, andbuildImportedSessionMetadata()falls back to the hub hostname. The new-session import flow immediately callsresumeSession()for the imported HAPI session, and resume only targetsmetadata.machineIdormetadata.host, so the import can succeed but the session cannot start on the runner the user explicitly selected. Evidencehub/src/web/routes/codexDesktop.ts:1924.
Suggested fix:const result = await importSelectedCodexSessions({ codexSessionIds: parsed.sessionIds, store: options.store, namespace: c.get('namespace'), getSyncEngine: options.getSyncEngine, localSessions: remote.sessions, resolvedMachineId: remote.machineId, model: parsed.model, modelReasoningEffort: parsed.modelReasoningEffort, yolo: parsed.yolo }) // then thread resolvedMachineId through importSelectedCodexSessions/importSingleCodexSession // and prefer it when building metadata for newly imported sessions: const metadata = buildImportedSessionMetadata( transcript, asRecord(existingStored?.metadata), options.resolvedMachineId ?? resolveImportMachineId(transcript.cwd, options.namespace, engine), options.yolo ? 'yolo' : undefined )
Questions
- None.
Summary
Review mode: follow-up after new commits. Re-reviewed the full latest diff. The previous --existing-session-id runner issue is fixed, but the remote Codex import path can still create an imported session that is not resumable on the selected runner when workspace roots overlap.
Testing
- Not run (automation).
HAPI Bot
| namespace: c.get('namespace'), | ||
| getSyncEngine: options.getSyncEngine | ||
| getSyncEngine: options.getSyncEngine, | ||
| localSessions: remote.sessions, |
There was a problem hiding this comment.
[MAJOR] The sync route has already resolved the exact runner in remote.machineId, but the import path drops it here. importSingleCodexSession() later recomputes machineId from transcript.cwd; with overlapping workspace roots that returns undefined, so new imported sessions are stamped with the hub host instead of the selected runner. The new-session flow then immediately calls resumeSession(), which only targets metadata.machineId or metadata.host, so import can succeed while resume fails with no matching machine.
Suggested fix:
const result = await importSelectedCodexSessions({
codexSessionIds: parsed.sessionIds,
store: options.store,
namespace: c.get('namespace'),
getSyncEngine: options.getSyncEngine,
localSessions: remote.sessions,
resolvedMachineId: remote.machineId,
model: parsed.model,
modelReasoningEffort: parsed.modelReasoningEffort,
yolo: parsed.yolo
})
// Thread resolvedMachineId through importSelectedCodexSessions/importSingleCodexSession
// and prefer it when building metadata:
const metadata = buildImportedSessionMetadata(
transcript,
asRecord(existingStored?.metadata),
options.resolvedMachineId ?? resolveImportMachineId(transcript.cwd, options.namespace, engine),
options.yolo ? 'yolo' : undefined
)
There was a problem hiding this comment.
Findings
- No issues found.
Questions
- None.
Summary
Review mode: follow-up after new commits. Re-reviewed the full latest diff, including the previous bot finding and the new machine-binding fix. No high-confidence introduced issues found. Residual risk: local verification was limited because the runner does not have bun installed.
Testing
- Not run:
bunnot found in PATH, sobun typecheckand targeted tests could not be executed here.
HAPI Bot
1 participant
Summary
Tests