fix: move-codex-session should refuse sessions whose goals cite source attachments #147
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/move-codex-session-goal-attachments"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Codex 0.160.1's TUI saves long goal text (over 4,000 characters), pasted text and images under
<codex_home>/attachments/<uuid>/and cites them by absolute path inthread_goals.objective(codex-rs/tui/src/goal_files.rsatrust-v0.160.1). The script moves goal rows but not those files, so a moved goal cites files that are lost once the source home is deleted.Preflight now refuses, before any lock or write, when a moved thread's objective contains
/attachments/<entry>/for an entry of the real source home'sattachments/. It checks again once the script holds the writer locks, because a source writer can change a goal until then. The error names the thread and file. Matching the random UUID directory names doesn't depend on how the home is spelled. Codex doesn't resolve symlinks in the default~/.codex, so an earlier path match missed aliases.SKILL.mdlists the refusal with its remedy:/goal clear, or replace the goal with one of at most 4,000 characters with no pasted text or images, then rerun. It also separatesthread_attachmentsrows (client JSON, which is moved) from attachment files (not moved), and unifies the placeholder names.Rejected: copying the files and rewriting the objective. It needs a new verification exception, and none of the 170 Codex homes checked has an
attachments/directory.Not checked: old citations in rollout
ThreadGoalUpdatedevents and inthreads.preview. Codex restores goals only fromthread_goals.The new tests fail on
main. They cover each Codex reference form, four alias spellings, a goal that changes after the preflight, no modification on refusal, and unrelated citations still moving. Six mutants of the first version were all caught.🤖 Generated with Claude Code
move-codex-sessionshould refuse sessions whose goals reference source attachments c157481130move-codex-sessionshould detect goal attachment citations however the source home is spelledReview
01M4JEJM2EZVMYD0793EKH734D— head7943249911174577f22012e7da296e2cfc12bdb9Review — j4k-oss/agent-skills @
5b1aa6fb3bScope: diff against base tree
aaa8e581a81eStatus: dispatched — coverage complete (5/5 slots terminal)
Facts: current review-wide projection
Computed under:
Findings (1)
low — Goals citing another home’s attachment are refused when its directory ID collides
01M4JEMG7GYE1MAM4ZAAQE0DAFskills/move-codex-session/scripts/move-codex-session.ts(snippet)01M4JEQ3BYBD6WD1BF7F0C2MWM· valid: The exact-grounded excerpt constructs /attachments/${directory}/ and searches goal.objective with indexOf, so the match itself does not distinguish source-home paths from destination or other-home paths. The reviewer supplies a coherent trace that requireNoGoalAttachmentReferences aborts on that match at both preflight and locked checks, and reports that the documented blocker is a reference under the source attachments directory. With the same directory name in both homes, the cited destination path satisfies the source-entry marker despite not referring to a source attachment. No supplied guard refutes this conditional false refusal; an executed reproduction is unnecessary to establish the string-match mechanism. Low severity is appropriate. Resolve the referenced path against the source attachment location while preserving supported lexical and symlink aliases.Other claims
Coverage
Coverage pass: 01M4JEK05QA8K75E0GDQYQPE2D
Accounting: complete
Slot health: healthy
@ -2138,2 +2169,4 @@);assertThreadDatabaseEmpty(threadDatabase.destination, threadDatabase, threadIds);if (threadDatabase.prefix === "goals") {requireNoGoalAttachmentReferences(threadDatabase.source, threadIds, args.sourceHome);medium — Recheck goal attachment references after acquiring writer locks
lens
general-bug· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M4JDMG4F7WYW36NH45HCBMHQof review01M4JDJ45QDW5VDSD17DTE9MR8Fixed in
2aa306d. The goal check now runs again after the script takes the writer locks, next to the rollout lineage recheck. A new test edits the source goal after the preflight and expects the refusal; without the fix the move exited 0 and copied the citation.move-codex-sessionshould recheck goal attachment citations under the writer locks@ -1613,0 +1635,4 @@return `Read the Codex goal objective file at ${home}/attachments/${GOAL_FILE_ID}/goal-objective.md before continuing.`;}// Every file's content hash and every directory below `home`, apart from SQLite's transient WAL companions.medium — The snapshot helper comment duplicates its traversal inventory
lens
restated-sets· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M4JE356G7D2GZHZ41MTWAQ0Hof review01M4JE0RED3FNQY1WDMN81VMQFFixed in
7943249. The helper comment is gone; the reason for skipping the WAL companions now sits beside the skip.@ -1613,0 +1717,5 @@assert.equal(refused.stdout, "", name);assert.equal(refused.stderr,`thread ${ROOT_ID} has a goal that references ${fixture.sourceHome}/attachments/${GOAL_FILE_ID}/goal-objective.md; goal attachment files are not moved\n`,name,low — The alias cases expect the physical path instead of the path passed to the script
lens
test-trimming· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M4JE5MY5836NR93CHEAM55RWof review01M4JE0RED3FNQY1WDMN81VMQFNot a defect:
maincanonicalizes both homes withrealpathSync(args.sourceHome)before the check (move-codex-session.ts, line 2115), so forpassed: "alias-a"the diagnostic names the physical path, and the test'sfixture.sourceHomeis right. All four alias cases pass in the PR'sNode testscheck and locally. The claim assumedresolve()inparseArgumentsis the only normalization; the lsof failure kept the reviewer from running the case.@ -941,0 +958,6 @@const goal = select.get(id) as { objective: string } | undefined;if (goal === undefined) continue;for (const directory of directories) {const marker = `/attachments/${directory}/`;const start = goal.objective.indexOf(marker);if (start === -1) continue;medium — Unrelated attachment paths can block valid session moves
lens
general-bug· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M4JE4GCC5WRJ3AMK0HGXN22Zof review01M4JE0RED3FNQY1WDMN81VMQFDeliberate. The first version matched on the source home's path, and review reproduced a miss: Codex does not resolve symlinks in the default
~/.codex, so a goal can cite the source home by an alias that matches neither--source-homenor its real path, and the files were then lost silently. Matching on the attachment directory names avoids that. Those names are random UUIDs, so one appears under two homes only when a home is a copy of another, and the refusal is loud: the error names the thread and file, andSKILL.mdgives the remedy (/goal clear, or a shorter goal). A false refusal costs one rerun; a false pass loses files. Resolving the cited prefix would mean extracting a path from free text, where spaces make its start ambiguous.move-codex-sessionsnapshot helper should explain only its WAL exclusion@ -941,0 +958,5 @@const goal = select.get(id) as { objective: string } | undefined;if (goal === undefined) continue;for (const directory of directories) {const marker = `/attachments/${directory}/`;const start = goal.objective.indexOf(marker);low — Goals citing another home’s attachment are refused when its directory ID collides
lens
general-bug· armdefault· tally 1 valid / 0 invalid / 0 uncertainclaim
01M4JEMG7GYE1MAM4ZAAQE0DAFof review01M4JEJM2EZVMYD0793EKH734DDeliberate. The first version matched on the source home's path, and review reproduced a miss: Codex does not resolve symlinks in the default
~/.codex, so a goal can cite the source home by an alias that matches neither--source-homenor its real path, and the files were then lost silently. Matching on the attachment directory names avoids that. Those names are random UUIDs, so one appears under two homes only when a home is a copy of another, and the refusal is loud: the error names the thread and file, andSKILL.mdgives the remedy (/goal clear, or a shorter goal). A false refusal costs one rerun; a false pass loses files. Resolving the cited prefix would mean extracting a path from free text, where spaces make its start ambiguous.