Repository navigation
fix: install Codex context skills under the account home - #624
Conversation
Gandy2025
left a comment
There was a problem hiding this comment.
Reviewed the immutable range 71e1d172f628a2df5c2b00bec12af8cef0ccb1be...8d6bcd17315fe0e46db5fdf9185265a94a0bafde as a high-risk provider-contract change.
Standards
The code range is focused, and the two manifests, lockfile, documentation, and exact-version assertions agree. The PR body is empty, however, so it does not document the user-visible skill-path migration or its validation as required by CONTRIBUTING.md; please fill that in before merge. I am treating that as a non-blocking process gap.
Spec
Blocking — the supported custom CODEX_HOME path reports ready after installing skills where Codex will not discover them.
packages/client/src/runtime/context-tree.ts:296-299 runs the 0.1.13 installer with HOME = dirname(codexHome), so a custom /opt/opentag/.codex receives its skills under /opt/opentag/.agents/skills. But the spawned Codex environment in client-runtime-composition.ts:450-474 preserves the original process HOME; Codex discovers user skills from $HOME/.agents/skills, independently of CODEX_HOME (official OpenAI documentation). The new test at context-tree.integration.test.ts:155-178 asserts only that the file exists beside the custom config home and explicitly asserts it is absent from the account home, so it proves the opposite location from the one the runtime scans.
I reproduced this with Codex 0.153.4 through the app-server skills/list method: with distinct HOME and CODEX_HOME=/custom/.codex, a marker skill under $HOME/.agents/skills was listed, while a different marker under /custom/.agents/skills was not. This means an OpenTag Computer using the currently accepted custom .codex config home can start successfully and expose Context Tree: ready, yet its Codex Sessions have none of the eight Context Tree skills.
Install the user-scoped skills under the same resolved HOME passed to the spawned Codex process, independent of CODEX_HOME (or use another explicit Codex-supported skill root). Then remove the obsolete basename coupling and replace the file-presence-only custom-home test with a real skills/list discovery assertion. Also update the stale .codex/skills comment at context-tree.ts:283.
Validation: the 0.1.12/0.1.13 published tarballs and the 0.1.13 installed package were inspected; 48 affected client tests and all 19 portable dependency tests pass. Those green tests do not exercise Codex skill discovery. Current CI is green aside from the expected ownership gate while review is pending. The previously accepted portable-assets and managed scheduling gaps remain tracked in #586 and are not new blockers in this review.
Gandy2025
left a comment
There was a problem hiding this comment.
Incremental re-review of the immutable range 8d6bcd17315fe0e46db5fdf9185265a94a0bafde...2137c77166c96467c527ecb20b34d33cb6516386 on base 71e1d172f628a2df5c2b00bec12af8cef0ccb1be, focused on the prior high-risk provider-contract finding.
Standards
The new delta is focused, the PR description now documents the user-visible migration and current validation honestly, and the design document and tests agree on the intended account-home destination.
Non-blocking: packages/client/src/runtime/context-tree.ts:167 still says the CLI installs skills beside the Codex config home under .agents. That is the old contract; please update it to say codexHome is used for host detection while skills install under the account home.
Spec
The OpenTag-side logic now addresses the original path bug: the Codex install receives the same resolved account HOME used for skill placement and receives CODEX_HOME independently for host detection. The basename restriction is gone, and the unit/integration cases cover both a custom .codex path and an arbitrary custom directory name.
This head is still not approvable, as the PR correctly states:
- It still pins
@first-tree-ai/context-tree@0.1.13. That release ignoresCODEX_HOMEduring host detection, so both new custom-home integration cases returnunavailablebecause$HOME/.codexdoes not exist. I reproduced the same two failures locally; the current GitHub Checks and Node compatibility jobs fail on them as well. Upstream context-tree#20 contains the required change and is green, but it remains open and unreleased. - The requested provider-boundary regression is still missing. The new integration cases prove where files land, but do not invoke Codex
skills/list, so they do not directly prove that a Session discovers the installed skills. I independently rechecked Codex 0.153.4 with an arbitrary customCODEX_HOME: it discoverscontext-tree-readfrom the account$HOME/.agents/skills, confirming the implementation target. Please preserve that boundary with a real discovery assertion, or an equivalent controlled provider check tied to release validation.
Keep the PR draft and the changes-requested state. After the upstream fix is released, update both manifests, the lockfile, and portable-runtime exact-version assertions; then make the focused and full CI gates green and re-request review. The next review can stay scoped to that release/pin delta, the provider-discovery coverage, and the affected tests.
Gandy2025
left a comment
There was a problem hiding this comment.
Approved the exact head b60a1cb460bfad83225e786806919cd75e9500cc after a proportional re-review of 2137c77166c96467c527ecb20b34d33cb6516386...b60a1cb460bfad83225e786806919cd75e9500cc. This closes my earlier changes-requested findings.
Standards
The release delta is focused. Both manifests, the lockfile, and the portable-runtime exact-version assertions agree on @first-tree-ai/context-tree@0.1.14; the lock integrity matches the npm registry. The PR description now covers the user-visible change, validation, and breaking behavior. All exact-head CI gates are green except the ownership gate, which is expected to update from review state and still requires the repository's separate second owner approval.
Non-blocking: packages/client/src/runtime/context-tree.ts:167 still describes the obsolete config-home-adjacent skill destination. Please update that comment when convenient so the option documentation matches the implemented account-home contract.
Spec
The previous custom-CODEX_HOME blocker is resolved end to end. Published 0.1.14 has gitHead=a286e5694731fa07ae31d4da7b55f8377bdecd14, the merge commit of upstream context-tree#20. Its installed code detects Codex through the independently supplied CODEX_HOME while writing skills under account $HOME/.agents/skills.
Validation on this head:
- 47 focused Context Tree tests pass against the installed registry package with no override.
- All 19 portable-runtime dependency tests pass.
- With Codex 0.153.4, I installed the published package twice, once with a custom
.codexdirectory and once with an arbitrarycodex-homedirectory. In both cases, app-serverskills/listdiscovered all eight packagedcontext-tree-*skills from account$HOME/.agents/skills. - Exact-head CI is green across Checks (including PostgreSQL integration and Agent Runtime coverage), Node 22/24/26 compatibility, patch coverage, browser/container/CLI pack smokes, build, typecheck, lint, and security/workflow gates.
The repository does not add a test that shells out to a real Codex binary. Given the repository rule that unit tests must not depend on external providers, the hermetic environment/install assertions plus the controlled exact-release skills/list validation above provide the appropriate boundary evidence; a permanent provider-binary check is not a merge blocker for this PR.
yuezengwu
left a comment
There was a problem hiding this comment.
Approved exact head b60a1cb460bfad83225e786806919cd75e9500cc. My earlier hold is resolved by independent verification of this release: all eight skills were discovered through real Codex skills/list using the current OpenTag manager and production launch arguments in three isolated layouts (default, custom .codex, and arbitrary custom directory). The wrong-root controls were ignored.
48 affected client tests, 19 portable dependency tests, pnpm check, pnpm build, and pnpm typecheck passed. Current-head technical CI is green.
One additional non-blocking documentation correction: docs/design/context-tree-integration.md:377-378 still describes installation beside the custom config home with the account home untouched. Please align that test-coverage paragraph with the account-home assertions now implemented.
liuchao-001
left a comment
There was a problem hiding this comment.
Approving exact head b60a1cb460bfad83225e786806919cd75e9500cc.
- The Context Tree install and the spawned Codex process now agree on both paths.
ContextTreeManagerreceives the samesourceEnvironmentand realpath'dcodexHomethatclient-runtime-composition.tspasses tocodexAgentRuntimeEnvironment. The install usesHOME = resolveAccountHome(sourceEnvironment), which is the realpath of the sameHOMEthat Codex inherits, and setsCODEX_HOMEexplicitly. Skills therefore land in the$HOME/.agents/skillsdirectory that Codex scans, whatever the config-home basename. - The
0.1.14pin is consistent across both manifests, the lockfile, and the portable-runtime assertions. The lock integrity matches the registry, and the publishedgitHeadis the merge commit of context-tree#20. - All exact-head CI checks are green apart from
ownership-gate, which is waiting on this second owner approval.
Non-blocking, as already noted: the codexHome option comment at packages/client/src/runtime/context-tree.ts:167 still says skills install "beside it under .agents". It would be worth fixing in a follow-up.
Summary
Custom
CODEX_HOMEpreviously redirected Context Tree installation beside the custom configuration directory, outside the account skill directory Codex scans. Pass accountHOMEand resolvedCODEX_HOMEindependently, remove the basename restriction, and retain unavailable reporting for skipped installations.Bump the CLI and client Context Tree dependencies to the published
0.1.14, which includes first-tree-ai/context-tree#20. Update the lockfile, portable-runtime dependency assertions, design documentation, and regression tests for both.codexand arbitrary custom names.Testing
pnpm install,pnpm check,pnpm build,pnpm typecheck.0.1.14package, with no local package override.pnpm --filter @opentag/server test:integration— 334 tests.pnpm test: Context Tree tests pass, but the client suite fails on the unchanged Codex probe fixture (login-startedmissing) and Claude probe timeout cases (1,268 client tests pass; 2 fail). Turbo stops on that failure, so this is not a full green repository run.Breaking changes
Custom Codex homes no longer return
CODEX_HOME_UNSUPPORTED. Skills install under account$HOME/.agents/skills, independently of the configuration directory.Checklist
pnpm check,pnpm build,pnpm typecheck, andpnpm testpass.