Repository navigation
fix(profiles): rebuild_profiles creates a domain from a Codex-only history (#726, slice 1) - #881
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude finished @cdeust's task in 2m 37s —— View job Review of PR #881: done
No Blockers. The review has three Notes, none needed before merge:
I could not run pytest or ruff in this job; the review comes from reading the code. Verdict: approve |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f88d5ccae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isinstance(record, dict): | ||
| records.append(record) | ||
| return records |
There was a problem hiding this comment.
Stream rollout records instead of retaining the entire transcript
For large Codex sessions, this retains every decoded record—including tool outputs, reasoning, and injected context—in memory until parsing finishes, although most of those payloads are never used. Because rebuild_profiles now scans Codex history unconditionally, a single sufficiently large rollout can exhaust the server's memory and prevent rebuilding even existing Claude profiles. The Claude scanner bounds metadata reads with head/tail sampling, and rollout_activity already streams its input; extract the metadata and capped prompt text incrementally here rather than accumulating the complete transcript.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Vrai sur le fond : _read_records garde tout le rollout en mémoire, y compris pour un thread rejeté dès la première ligne, puis rollout_activity le relit. Ce n'est pas un bloqueur au sens d'ADR-1119 (aucun échec reproduit, P2), donc ce n'est pas poussé dans cette PR : une lecture en un seul passage (session_meta, prompts plafonnés à MAX_USER_TEXT_CHARS, tours et outils) ira dans une PR de raffinement de #726, avec la note de la revue Claude sur PRIVACY.md.
Generated by Claude Code
Review 1 of 3: PR #881 (Codex history in
|
…story (#726, slice 1) rebuild_profiles walked ~/.claude/projects only, and scanner_parse reads top-level user/assistant records, so a Codex rollout parsed to zero messages and a Codex-only history created no domain. infrastructure/codex_history.py reads every user-thread rollout under $CODEX_HOME/sessions (default ~/.codex/sessions) into the conversation record scanner_parse.build_conversation_record builds for a Claude transcript: project from session_meta.cwd through cwd_to_project_id, typed prompts from event_msg user_message / item_completed UserMessage, turns and tool order from transcript_rollout.rollout_activity (ADR-1081), source "codex". Subagent, guardian and consolidation threads are skipped. Locations and record shapes: openai/codex@e7f32cf. The test suite now redirects CODEX_HOME with the other real-data roots. Refs #726 Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018kyZYtKhqTifXyihe2MEcp
…unit, run 42137170027) The unit job's diff-scoped mutation gate (ADR-1122) failed on 9ad593b: surviving mutants in the changed functions of rebuild_profiles.py, codex_history.py and scanner_parse.py. - scanner_parse.py leaves the diff: codex_history imports the existing _MAX_USER_TEXT_CHARS instead of renaming it, so extract_message_stats (76 survivors, untouched logic) is no longer a changed function. - codex_history reads rollouts as bytes and lets json.loads decode them: a non-UTF-8 line is skipped like any malformed line, and the encoding arguments whose mutants no test could tell apart are gone. CODEX_HOME is read without a default ("" and None were the same falsy value). - test_codex_history_edges.py: a malformed or non-UTF-8 line does not end the read; an empty file, a non-session_meta first record, a non-object meta payload, a non-string cwd, non-string or non-object prompt payloads and non-text content blocks are skipped; an unreadable rollout is reported on stderr and the scan goes on. - test_rebuild_profiles_wiring.py: the handler passes force and replace_accumulated as False, the discovered memories, brain index and both scans to the assembler, and reports duration in milliseconds and [] for a build without domains. Local: GITHUB_BASE_REF=main scripts/ci_mutation_diff.py -> 2 file(s), 0 failed, 0 untested. Refs #726 Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018kyZYtKhqTifXyihe2MEcp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5711763c4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try: | ||
| record = parse_rollout_conversation(path) | ||
| except OSError as exc: |
There was a problem hiding this comment.
Isolate parsing failures to the affected rollout
If a rollout contains valid JSON with a malformed timestamp, parsing can raise an exception that this OSError handler does not catch. For example, a string timestamp on session_meta followed by timestamp: 1 on a user event reproducibly raises TypeError in extract_metadata_fields. Since rebuild_profiles now scans Codex history unconditionally, that single bad record aborts the entire rebuild, including unrelated Claude profiles. Validate metadata types or catch and report per-file parsing failures before continuing, as the Claude scanner already does in _scan_project_conversations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reproduit (TypeError: '<' not supported between instances of 'int' and 'str'), ainsi que la variante RecursionError d'une ligne trop imbriquée relevée par la revue Claude 2. Corrigé dans 35a7e93 : l'intervalle de temps ne lit plus que les timestamp de type chaîne (_time_span, sans extract_metadata_fields), et discover_codex_conversations signale et saute un rollout qui lève RecursionError comme un rollout illisible. Tests : test_a_timestamp_that_is_not_a_string_is_skipped, test_no_string_timestamp_gives_no_time_span, test_a_too_deeply_nested_line_skips_its_rollout_only.
Generated by Claude Code
5711763 to
d7289db
Compare
|
Claude finished @cdeust's task in 2m 52s —— View job Review 2 of 3: PR #881, done
No Blockers. The review has three Notes, none needed before merge:
I could not run pytest or ruff in this job; the review comes from reading the code. Verdict: approve |
|
CI autopilot : correctif poussé pour Diagnostic : les 11773 tests passaient ; c'est la gate de mutation limitée aux fonctions modifiées (ADR-1122, arrivée sur Commits : Vérifié localement : Generated by Claude Code |
Review 2 of 3: PR #881 (Codex history in
|
…only history (#726, slice 1)
|
Claude finished @cdeust's task in 2m 11s —— View job Review 3 of 3: PR #881, done
No Blockers. The fixup fixes the open P2: a non-string
The whole PR is accepted. I could not run pytest or ruff in this job; the review comes from reading the code. Verdict: approve |
Review 3 of 3: PR #881 (Codex history in
|
Summary
First slice of #726:
rebuild_profilesnow creates a domain from a Codex-only history. It walked~/.claude/projectsonly, andscanner_parse.extract_message_statsreads top-leveluser/assistantrecords, so a Codex rollout (session_meta,response_item,event_msg) parsed to zero messages and created nothing (the issue's offline replay:user_count=0,assistant_count=0).New
mcp_server/infrastructure/codex_history.pyreads every rollout under$CODEX_HOME/sessions/**/rollout-*.jsonl(CODEX_HOMEdefaults to~/.codex) into the recordscanner_parse.build_conversation_recordalready builds for a Claude transcript:session_meta.payload.cwdthroughshared.project_ids.cwd_to_project_id, the function that names Claude project directories, so one repository worked on from both hosts is one domain;event_msguser_message(legacy history) andevent_msgitem_completedwith aUserMessageitem (paginated history), the two shapes Codex's own thread preview reads.response_itemmessages with roleuserare not read: they also carry the injected<environment_context>/ instruction blocks, which no user typed;transcript_rollout.rollout_activity(ADR-1081), the reader SessionEnd already uses for the same file;source: "codex"on the record (provenance);session_meta.thread_sourceis notuser(subagent, guardian review, memory consolidation) are skipped, as the Claude scan skips subagent transcripts; a rollout without that field (older Codex) counts as a user thread.Sources for locations and shapes, read in openai/codex@e7f32cf:
codex-rs/utils/home-dir/src/lib.rs(find_codex_home),codex-rs/rollout/src/lib.rs(SESSIONS_SUBDIR),codex-rs/rollout/src/policy.rs(persisted events),codex-rs/rollout/src/list.rs(event_msg_preview),codex-rs/protocol/src/protocol.rs(ThreadSource,UserMessageEvent),codex-rs/protocol/src/items.rs(TurnItem::UserMessage).handlers/rebuild_profiles.pyconcatenates both scans; its tool description names the Codex location.scanner_parse._MAX_USER_TEXT_CHARSbecomes publicMAX_USER_TEXT_CHARSso both readers cap prompt text identically (ADR-0596). The test suite redirectsCODEX_HOMEwith the other real-data roots (tests_py/conftest.py, and thehermetic_claude_dirsfixture), so a developer's real Codex history never reaches a rebuild under test.Refs #726 (not
Closes: remaining slices below).Type of change
Test plan
tests_py/infrastructure/test_codex_history.py(14 tests): legacy and paginated prompts give exact counts (2 prompts, 2 turns,["exec", "apply_patch"], duration 120000 ms); injected<environment_context>is not a prompt; the Claude reader returns 0/0 on the same records (the defect, pinned); the committed ADR-1081 excerpt maps to-Users-example-projectwith 2 turns; subagent / guardian_review / memory_consolidation threads skipped; missingthread_sourcekept; nosession_meta, no cwd, or no message → None;CODEX_HOMEset / empty / unset; discovery readsrollout-*.jsonlonly, in path order, identically on a second scan.tests_py/handlers/test_rebuild_profiles_codex.py: an isolated Codex-only history (two rollouts, one cwd, no Claude project) →totalSessions == 2, one domain, outcomecreatedwith 2 scanned / 2 resulting sessions; a secondforcerebuild → stored 2, scanned 2, resulting 2 (no duplication).pytest tests_py/handlers tests_py/infrastructure→ 3022 passed, 220 skipped, 3 failed: the threetests_py/infrastructure/test_embedding_engine.pytests that importtorch, which this container cannot install (the PyTorch wheel host is blocked by its proxy). Unrelated to this diff. The existing Claude fixtures (test_rebuild_profiles.py,test_rebuild_profiles_accumulation.py,test_scanner.py) pass unchanged.ruff checkon the changed files: clean;ruff format --check: the only non-conforming files are 3 undertests_py/scripts, identical onmain.python scripts/check_craftsmanship.py: OK (tests_py/conftest.pystays at 300 lines).Not verified here: a real Codex history (none in this container); the PostgreSQL path (no PostgreSQL here; PG-gated tests skipped); Windows
cwdforms from Codex (the mapping is the existingcwd_to_project_id, already tested for Windows paths).Audit notes
infrastructure/codex_history.pyimportsinfrastructuresiblings andshared.project_idsonly; the handler stays the composition root.hooks/session_lifecycle.py:363still logs only); (3) a Codex session recorded by SessionEnd and later rescanned is counted once across both paths (today the rescan is a full rebuild, so it does not duplicate, but there is no shared session identity between the two); (4)$CODEX_HOME/archived_sessionsis not scanned — whether archived threads belong in a profile is an owner decision.Coding-standards compliance
Breaking changes
None for Claude-only users. A user with Codex history gets new domains (or more sessions in an existing one) on the next
rebuild_profiles.Reviewer checklist
Generated by Claude Code