Repository navigation
fix(codex): a Codex core gets the Sutando skills and CLAUDE_CONFIG_DIR - #5599
Conversation
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 91%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
akag2
left a comment
There was a problem hiding this comment.
Reviewed head d01078b252af658c36353e37f4fdde886f3b7948.
Read all 8 changed files end-to-end (docs/catalog.json, docs/codex-core.md, scripts/sutando-config.sh, skills/collaboration-intelligence/SKILL.md, skills/install.sh, src/agent/codex/cli/start-cli.sh, tests/codex-core-launcher.test.py, tests/skills-install-codex.test.py).
link_skills()extraction inskills/install.shis a straight de-duplication of the two previously-separate repo/workspace linking loops; the per-skill relink/skip/warn logic is byte-for-byte unchanged, just called twice (Claude target, then Codex target whencore-runtimeiscodex)..system/and any pre-existing same-named real directory are correctly left untouched (never replaced).codex-home-pathinsutando-config.shmirrors the existingclaude-home-pathresolution order (launcher-exported value →$CODEX_HOME→ Codex default), with the same trailing sub-path-join behavior.CLAUDE_CONFIG_DIRforwarding into the tmux core session (start-cli.sh) is a one-line additive change with a test for both the set and unset case.- New tests (
tests/skills-install-codex.test.py) cover the two directions that matter: Codex-runtime run links repo + workspace skills into the Codex root (idempotent on a second run), Claude-runtime run leaves it alone. - PR body's before/after regression evidence (failing at parent
b5b0e9b2, passing at HEAD) matches what the diff implements.
CI: all green except the author's own license/cla (pending — external, not a code finding). review-checks.sh hardcoded-path/root-artifact scan: clean. Its prose-cap sub-check was skipped locally only because this checkout's current tree doesn't carry the two new test files at my working revision — a local tooling limitation, not a PR defect; CI's own classify the diff / lint jobs are authoritative and pass.
No correctness issues found. Approving; the CLA is the only thing standing between this and mergeable, and it's on the author, not the review.
skills/install.sh linked skills only into Claude's skills directory, so a Codex core never saw room-commons or any other Sutando skill (#5587). With core.runtime codex it now also links every repo and workspace skill into $CODEX_HOME/skills, resolved by a new `sutando-config.sh codex-home-path` from the same core_config_dirs entry the Codex launcher exports. Codex follows the symlinks; its bundled .system/ skills and any same-named entry are left alone, and a re-run links nothing new. The Codex launcher now forwards CLAUDE_CONFIG_DIR into the core session, so room_commons.py finds the channel access.json there, as it does on Claude. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
d01078b to
8964994
Compare
|
@cla-assistant check |
sonichi
left a comment
There was a problem hiding this comment.
Diff reads correctly: the install.sh refactor into a shared link_skills() is idempotent (never replaces an existing dir or re-links a working symlink), codex-home-path mirrors claude-home-path's resolution order, and the CLAUDE_CONFIG_DIR forward into the Codex core session has its own regression test. Ran tests/skills-install-codex.test.py and the targeted tests/codex-core-launcher.test.py locally (env -i, to avoid my own pool-worker env vars leaking in — see sibling note below): 73/74 pass. The one local failure (test_launches_codex_and_managed_task_notifier) reproduces identically on origin/main, so it's pre-existing and unrelated to this diff.
Not approving yet, though: the live CI run shows a REAL failure — tests/health-check-task-watcher.test.py (cases s, t3, u, u2-u4), unrelated to anything in this diff on its face. I reproduced it 3/3 clean locally on both this branch and origin/main, which points to a CI-environment flake (PID-reuse/timing) rather than a regression, but I'm not certifying that from three local runs alone. Recommend re-running the failed job before merging; I'll approve once it's green (author is qingyun-wu, not my shared login, so I can formally approve here).
sonichi
left a comment
There was a problem hiding this comment.
Upgrading my earlier COMMENT to a formal APPROVE: same head (89b5eaf) I reviewed before — the diff itself hasn't changed, only CI has. At that time tests/health-check-task-watcher.test.py had failed; I reproduced it 3/3 clean locally on both this branch and main and called it a likely CI-environment flake, recommending a re-run rather than approving over a visible red X. The re-run (run 38871684186) is now fully green, 34/34 checks pass, confirming that read. The install.sh link_skills() refactor, codex-home-path helper, and CLAUDE_CONFIG_DIR forwarding all check out as before.
marklysze
left a comment
There was a problem hiding this comment.
Approve — ready to merge (recommendation; the repo's merge conditions still apply). Reviewed head 89b5eafb6c7b81a78647aed07f22464d35eed8ba.
Verified, not read off the description:
skills-install-codex(2 tests) andskills-install-workspace-skills(3) pass at head. The new codex test errors on the merge base (ffc79966) with this PR's test file dropped in, so it does exercise the failure (a codex-runtime install links nothing). Control for the negative: the same test passes at head.codex-core-launcherhas 7test_notifier_*failures on my VM, identical at the merge base (72 tests at base, 74 at head), so host-specific and not from this diff. CI is 33 pass / 1 skipping.install.sh: the two loops becamelink_skills <target>with the bodies moved unchanged, including the shipped-wins-collision rule, broken-symlink relink and skip-existing-dir.local TARGETshadows the global only inside the function. The Claude path runs first and prints the same lines.
Worst case for existing users: none for Claude-core hosts, since the new branch is gated on core-runtime == codex. On a Codex host it adds symlinks under $CODEX_HOME/skills, never replacing an existing dir or .system/; codex-home-path resolves the same core_config_dirs entry the launcher exports.
Non-blocking notes:
- 16 of 82 shipped
SKILL.mdfiles have no YAML front-matter, includingschedule-crons,session-recapandtask-progress. Codex reports them as load errors (15 in the PR's run) and won't load them, so a Codex core still lacks/schedule-cronsand/session-recap. The PR body says this predates it. It is worth its own issue, and the startup flow depends on those two. CLAUDE_CONFIG_DIRis forwarded only when set at launch; if unset,room_commons.pystill can't findaccess.json. That matches the CODEX_HOME line next to it.
Review by Mark's agent (posted under marklysze).
Fixes the skills half of #5587. First of two PRs for "Room Commons works on a Codex core"; the second (standing instruction + MCP setup doc) is #5598. Independent: either can merge first.
What changed, and why?
skills/install.shlinked skills only intoclaude-home-path skills; nothing wrote a directory Codex reads, and the Codex launcher passes no skills path. So a Codex core in an AG2 Space room had noroom-commonsskill and produced an OpenAI artifact instead (Codex-based document creation defaults to OpenAI artifact instead of Room Commons #5587).scripts/sutando-config.sh codex-home-path [sub…](new): thecore_config_dirscodexvaluethe Codex launcher already exports asCODEX_HOME, else$CODEX_HOME, else Codex's default. Same sub-path joining asclaude-home-path.skills/install.sh: the two link loops (repo skills,<workspace>/skills) become onelink_skills <target>function, unchanged in behavior. It runs for Claude's directory as before and, whencore-runtimeiscodex, again forcodex-home-path skills.app-serverskills/listreports$CODEX_HOME/skillsentries as scopeuser, follows symlinked skill folders, and keeps its bundled skills in$CODEX_HOME/skills/.system/(evidence below)..system/is never a skill name here, an existing directory or symlink with a Sutando skill's name is skipped (only a broken symlink is relinked, as on the Claude side), and a re-run links nothing new.CLAUDE_CONFIG_DIRnever reached a Codex core session.room_commons.py(access_file) looks for the channelaccess.jsonunder$CLAUDE_CONFIG_DIR. tmux hands a new session the server's environment, not the launcher's, so the Codex launcher now forwards it with-e, likeCODEX_HOME.SUTANDO_WORKSPACE_DIRis not added here: open fix(start-cli): forward SUTANDO_WORKSPACE_DIR into the core tmux session #4895 forwards it from both launchers and this PR does not duplicate it.skills/collaboration-intelligence/SKILL.mdsaid Codex was "discoverable in principle, not installed in practice" and named this installer change as the missing step; it now says Codex installs happen withcore.runtime: codex.docs/codex-core.mdgains the install step.Files to look at first
skills/install.sh, thenscripts/sutando-config.sh(codex-home-path), then the one-line forward insrc/agent/codex/cli/start-cli.sh.Before/after evidence
Same script for both: a throwaway
CODEX_HOMEandCLAUDE_CONFIG_DIR(the real~/.codexand~/.claudeare untouched), set through a gitignoredsutando.config.local.jsonwithcore.runtime: codex. TheCODEX_HOMEis seeded with a stand-in bundled.system/imagegen, a user-installedlinear, and a user directory namedtask-progress, which collides with a shipped skill.room-commonsis a workspace skill (workspace/skills/room-commons, linked to an ag2-space/room-commons checkout). The last step asks the real Codex CLI, throughcodex app-server'sskills/listwith thatCODEX_HOME, which skills it discovers. The repeated✓ <skill>lines are elided.Before (
b5b0e9b2, main):After (this branch):
75 links: 74 repo skills (all but
task-progress, kept as the user's own directory) plusroom-commonsfrom the workspace. Codex loads 60 of them; withlinearandtask-progressthat makes the 62 it lists. The 15 load errors are pre-existing: thoseSKILL.mdfiles have no YAML front-matter, which Claude Code tolerates and Codex does not. That is a follow-up, not this PR.Regression tests at the parent commit (the new test file and the two new launcher tests, copied onto
b5b0e9b2):At HEAD:
Feature/documentation consistency
docs/codex-core.md(canonical Codex-core doc) gains the install step and theCLAUDE_CONFIG_DIRforward; itslast_verifiedindocs/catalog.jsonis bumped.Tests
Run with an environment stripped of this host's
SUTANDO_*/CLAUDE_CONFIG_DIR/CODEX_*variables:tests/skills-install-codex.test.py(Codex runtime links into the configured Codex root, workspace skills too,.system/and a same-named directory untouched, second run links nothing; Claude runtime leaves the Codex root alone). It runs on a fixture repo copy with its ownsutando.config.json, so it never touches a real Codex home.tests/codex-core-launcher.test.py: the corenew-sessioncarries-e CLAUDE_CONFIG_DIR=…, and leaves it out when unset.sutando-config.sh,install.shorcodex-core.md, plus the skill-symlink health checks andskill-roots: 45 distinct test files, 43 PASS.channel-bridge-launchd-supervisorfailed once under load and passes when rerun alone, both atb5b0e9b2and at HEAD.codex-core-launchersuite has load-sensitive failures on this host at main as well. Main: 72 tests,failures=2, errors=5. HEAD: 74 tests,failures=2, errors=4. HEAD's set is a subset of main's: the same two FAILs plus four of main's five ERRORs. The two new tests pass. CI is authoritative for that suite.git diff origin/main | bash scripts/review-checks.sh:PASS (hardcoded-paths + root-artifacts + prose-cap clean).Live path?
Startup-adjacent:
install.shand the Codex launcher's env. The evidence above is a real Codex CLI reading the installed links. A post-restart round trip on a live Codex core (a room task that ends in a Commons page) was not run; that needs a host whose core runtime is Codex. The Windows twinskills/install.ps1is unchanged.🤖 Generated with Claude Code