Skip to content

fix: adapt agent artifacts for Codex - #9

Merged
3metaJun merged 2 commits into
mainfrom
fix/runtime-artifact-portability
Sep 11, 2026
Merged

3metaJun merged 2 commits into
mainfrom
fix/runtime-artifact-portability

Conversation

@3metaJun

@3metaJun 3metaJun commented Sep 11, 2026 •

Copy link
Copy Markdown
Owner

The optional agents artifact was copied as Markdown beside each harness's skills directory, so Codex could not discover it. Codex installations now generate TOML under CODEX_HOME/agents, defaulting to ~/.codex/agents. SSH installs recognize both standard skill roots; custom remote layouts require an explicit artifact target.

Planning rejects shared targets with conflicting output formats before writes or connections. Both installers and repository validation reject invalid adapter settings. Agent conversion preserves the body, rejects unsupported metadata, and cannot overwrite a same-name TOML file. README and environment examples explain the separate Codex roots.

This PR also aligns the Codex plugin version with package.json, enforces version equality, and removes legacy delegation wording.

Validation: npm test passes with 96 tests and one expected Windows skip, including standard TOML parser checks and regression tests for remote paths, both collision orders, empty CODEX_HOME, invalid profiles, and preservation of an existing installation when conversion fails. Package and strict pinned-upstream checks pass.

Historical release tags remain unchanged. npm publishing and a matching GitHub Release are deferred until the remaining fixes are complete.


Model: GPT-6
Harness: Codex

Comment thread scripts/remote-install.mjs Outdated
Comment thread scripts/install.mjs
Comment thread scripts/install.mjs Outdated
Comment thread scripts/validate.mjs Outdated

@3metaJun 3metaJun left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial review (4 independent reviewers + first-hand reproduction)

Verdict: request changes. The local Codex TOML conversion, override validation, and version sync are solid, and the PR's validation claims check out empirically (npm test = 84 passed + 1 expected platform skip, npm run check-package clean, git diff --check clean). But the headline remote behavior has a confirmed bug, and the test suite never exercises the layout that breaks.

P1 — should fix before merge

  1. Remote agents double .codex for the conventional ~/.codex/skills target. scripts/remote-install.mjs:100-103 pops a level only when the skills parent is literally .agents, then unconditionally appends .codex. Reproduced via --dry-run: targets.codex = /home/dev/.codex/skills → artifact/agents -> /home/dev/.codex/.codex/agents. Contradicts the PR body's "map SSH environments to ~/.codex/agents" and diverges from local resolution; no mechanism honors a remote CODEX_HOME. → inline comment
  2. Order-dependent .md→.toml conversion on shared targets. scripts/install.mjs:605 + first-wins dedup: with colliding overrides, --harness claude,codex ships .md with no .toml (broken for Codex); codex,claude ships .toml only (broken for Claude, and pinned by the existing test). Remote is mirror-image last-writer-wins on upload order. → inline comment

P2 — worth addressing

  1. Set-but-empty CODEX_HOME silently writes agents into the process CWD — scripts/install.mjs:99 (configuredPath's ?? semantics; reproduced via --dry-run). → inline comment
  2. README artifact-table header is now false for the Codex row — README.md:62 still says "(beside the harness skills/ directory)" while $CODEX_HOME/agents is a second root; skills/agents now split across ~/.agents and ~/.codex with no explanation. Related undocumented behavior change: HARNESS_SKILLS_CODEX_DIR no longer relocates agents (they follow CODEX_HOME instead).
  3. format accepts any string; harnesses keys unchecked — scripts/validate.mjs:48; a typo silently reintroduces the raw-.md bug. Runtime re-validates path but not format/base. → inline comment
  4. Released v0.2.1 still carries the version drift this PR fixes. Tag v0.2.1 (23bae0a) ships package.json 0.2.1 vs plugin.json 0.2.0; the new version-integrity.test.mjs would fail at that tag. The alignment only lands in a future release — worth tracking against the release convention that every npm version gets a matching GitHub Release.
  5. Straggler legacy Task wording — skills/meta-mode/playbooks/orchestrate.md:16 ("full Task schema") survives the "remaining legacy Task wording" cleanup; multi-phase-plan.md:79 and capability-matrix.md:10 are arguable label uses.

P3 — nits / latent

  1. readAgentFrontmatter is not a YAML parser: \s* crosses newlines (a name: value can be captured from a later line), single-quoted YAML values keep their quotes into TOML, block scalars mangle, and a missing trailing newline after the closing --- is a hard install failure. Current agents/*.md all parse cleanly.
  2. JSON.stringify-as-TOML edge cases: raw U+007F and lone surrogates produce TOML-spec-invalid basic strings; no round-trip validation. Unreachable from current content.
  3. .md conversion is non-recursive (future subdirs pass through unadapted) and can silently clobber a same-basename shipped .toml.
  4. base: "codex-home" is not bound to the codex harness; the guard at install.mjs:170 still says "harness configuration directory" for what is now $CODEX_HOME.

Coverage gaps

  • No test for the local ~/.codex default (install.test.mjs fixture always sets CODEX_HOME).
  • No remote test for the conventional ~/.codex/skills target — the case that breaks in #1.
  • artifactPathParts harness-override path preference branch is untested and unused by any artifact.
  • Error paths (has no frontmatter, is not a valid Codex agent definition) and TOML escaping are untested.

Confirmed working (attacked, held)

Path containment (artifactPathParts rejects ../separators for base and override arrays; pathIsWithin backstop), relative/~-relative CODEX_HOME handling, env-var naming parity local↔remote, remote shell quoting on all new code paths, TOML structural injection impossible via JSON.stringify, staging/commit flow lands .toml and never the stale .md, dry-run safety of the new tests, and version sync at HEAD.

@3metaJun
3metaJun merged commit 983ddc8 into main Sep 11, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant