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
1 participant
Summary
Tests