Skip to content

[WIP] Add read-only Codex CLI history support#150

Draft
Copilot wants to merge 3 commits into
dev/yuazha/codex-sessionfrom
copilot/wta-add-read-only-codex-cli-history-support
Draft

[WIP] Add read-only Codex CLI history support#150
Copilot wants to merge 3 commits into
dev/yuazha/codex-sessionfrom
copilot/wta-add-read-only-codex-cli-history-support

Conversation

Copilot AI commented May 31, 2026

Copy link
Copy Markdown

Thanks for the feedback on #98. I've created this new PR, which merges into #98, to address your comment. I will work on the changes and keep this PR's description up to date as I make progress.

Original PR: #98
Triggering comment (#98 (comment)):

Thanks for the careful review @copilot. I pushed fixes for every actionable comment to a follow-up branch: dev/yeelam/pr-98-codex-session (commit 43d719167). The branch also resolves the current merge conflict with main (in tools/wta/src/app.rs, around the NotResumable / phantom-load arms — combined the new known_cli_id helper with main's i18n flow so the i18n machinery is preserved and Codex is covered by lookup_profile_by_id("codex")).

Per-comment status:

tools/wta/src/history_loader.rs

  • parse_iso_to_systemtime overflow panic → switched the final construction to SystemTime::UNIX_EPOCH.checked_add(...) AND added an upper-bound year guard (year > 9999) so the intermediate days_before_year / total_days * 86400 arithmetic also can't overflow.
  • TZ offset HH/MM not range-checked → now rejects out-of-range offsets (hh ∉ 0..=23 or mm ∉ 0..=59) before they can skew the timestamp.
  • Stale docstring → rewritten to document both the Z and ±HH:MM shapes, plus the "never panics, fails closed" contract.

tools/wta/src/agent_hooks_installer.rs

  • parse_codex_marketplace_list leading-whitespace bug → now line.trim() (both ends) before splitn, so column-aligned rows with leading spaces parse correctly.
  • parse_codex_plugin_list "anything other than not" → now rest[0].starts_with("installed"), matching the comment's intent (statuses like available are no longer misreported as installed; existing tests with installed, installed, enabled, not installed still pass).
  • Stale "Codex placeholders" header → rewritten to describe what the real implementations do.

src/cascadia/TerminalSettingsEditor/Resources/<locale>/Resources.resw

  • AIAgents_HooksRemovingCodexSummary only in en-US → added to all 15 other locales that already had the parallel Copilot / Claude / Gemini entries, with the English source value so translators pick it up.

tools/wta/wt-agent-hooks/codex/plugins/wt-agent-hooks/hooks/send-event.ps1

  • Header still referenced Claude / Copilot → rewritten to refer to Codex.
  • CLI-source identification comment → now lists claude / codex / copilot / gemini and explicitly notes this bundle passes -CliSource codex.

Check-spelling errors (fanin / feb / stat'ing / systemtime / yuazha) were all addressed by renaming identifiers (parse_iso_to_system_time, parse_iso_accepts_february_29_leap_year, codex-fan-in-test) or rewording prose (<user> placeholder, "running stat on") — no new entries in .github/actions/spelling/expect/. Spell-check now passes on the follow-up branch.

Strictly bug-fix scope, no behavior changes beyond what the reviewer flagged. Happy to cherry-pick or open a separate PR if that's easier — let me know what works best.

Copilot AI mentioned this pull request May 31, 2026
Merged
9 tasks
Yee Lam Allen Tang and others added 2 commits May 31, 2026 07:52
fix(wta): address PR #98 review comments
1bac82f 
Without changing slice-A scope, fix the issues raised in the PR review:

history_loader.rs (Copilot review):
* Use SystemTime::UNIX_EPOCH.checked_add(...) so malformed / far-future
  timestamps fail closed (return None) instead of panicking on overflow.
* Range-check timezone offsets HH (0-23) / MM (0-59) so strings like
  '...+99:99' are rejected instead of silently skewing the timestamp.
* Update parse_iso_to_system_time docstring to match behavior (it also
  supports the +/-HH:MM offset variant, not only Z).

agent_hooks_installer.rs (Copilot review):
* parse_codex_marketplace_list: trim both ends of each line so the
  first whitespace-delimited column parses correctly when 'codex plugin
  marketplace list' emits column-aligned rows with leading spaces.
* parse_codex_plugin_list: match the documented behavior by checking
  for an 'installed*' status prefix instead of just '!= "not"', so
  statuses like 'available' don't get misreported as installed.
* Drop the stale 'Codex placeholders' section header — the functions
  below are real implementations.

Resources.resw: add AIAgents_HooksRemovingCodexSummary to every locale
that already had the Copilot/Claude/Gemini variants (16 .resw files
total), so Codex doesn't silently fall back to en-US for translators.

wt-agent-hooks/codex/.../send-event.ps1: update the bundle header and
the CliSource-identification comment to refer to Codex instead of
Claude/Copilot — the bundle is Codex-specific.

Check-spelling fixes (rewording / renaming, no dictionary changes):
* parse_iso_to_systemtime -> parse_iso_to_system_time (snake_case
  matches the SystemTime type).
* parse_iso_accepts_feb_29_leap_year -> parse_iso_accepts_february_29_leap_year.
* codex-fanin-test -> codex-fan-in-test (session_registry.rs test key).
* superpowers slice-B docs: replace author handle in worktree paths
  with <user> placeholder; reword "stat'ing" to "running stat on".

No behavior change beyond the targeted bug fixes Copilot called out;
no new features, no signature changes, no test additions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
docs(wta): clarify ISO offset format in parser comment
c5f4d56
Copilot AI requested a review from yeelam-gordon May 31, 2026 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yeelam-gordon yeelam-gordon Awaiting requested review from yeelam-gordon

Assignees

@yeelam-gordon yeelam-gordon

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yeelam-gordon