fix(iterate): ship real vendor manifest dirs for Codex install - #607
onatozmenn wants to merge 4 commits into
Conversation
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
5 similar comments
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
There was a problem hiding this comment.
🟡 Changes recommended
Existing plugin-loading tests still require vendor paths to be symlinks, so the full test suite will fail for iterate.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR fixes Codex installation by replacing iterate vendor manifest symlinks with real mirrored directories and updating synchronization checks. It also includes an unrelated GitHub repository monitor profile-resolution change.
Changes:
- Add real
.codex-pluginand.claude-pluginmanifest copies. - Validate and refresh real vendor manifests in
sync_extensions.py. - Add manifest/install tests and profile-resolution tests.
File summaries
| File | Description |
|---|---|
scripts/sync_extensions.py |
Supports symlinked or mirrored vendor manifests. |
skills/iterate/.codex-plugin/plugin.json |
Adds Codex manifest copy. |
skills/iterate/.claude-plugin/plugin.json |
Adds Claude manifest copy. |
tests/test_sync_extensions.py |
Tests vendor manifest synchronization and lossy installs. |
skills/github-repo-monitor/scripts/main.py |
Adds LLM profile resolution. |
tests/test_github_repo_monitor.py |
Tests profile resolution behavior. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for vendor in (".codex-plugin", ".claude-plugin"): | ||
| path = iterate / vendor | ||
| assert not path.is_symlink(), f"{vendor} must not be a symlink" | ||
| assert path.is_dir(), f"{vendor} must be a real directory" | ||
| assert (path / "plugin.json").read_bytes() == canon |
| profile = os.environ.get("AUTOMATION_MODEL") | ||
| if profile: | ||
| profile_llm = _fetch_profile_llm(agent_url, api_key, profile) | ||
| if profile_llm is not None: | ||
| llm = profile_llm | ||
| llm["usage_id"] = f"profile:{profile}" |
Co-authored-by: openhands <openhands@all-hands.dev>
a44d970 to
c1ee8ba
Compare
Co-authored-by: openhands <openhands@all-hands.dev>
|
Follow-up on the review: the branch has been cleaned to just this fix, and the valid point about plugin-loading tests is addressed — |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Assessment
The core fix is sound: replacing the skills/iterate/.codex-plugin (and .claude-plugin) symlinks with real directories whose plugin.json mirrors .plugin/plugin.json does survive the symlink-dropping copy that Codex performs, and the added sync_symlinks handling correctly accepts a real mirror directory, flags a stale copy, and refreshes it in fix mode. The test_lossy_install_copy_keeps_codex_manifest regression test genuinely reproduces the reported failure.
However, the new unit test TestVendorManifests.test_missing_vendor_still_gets_symlink fails on Linux, which is the platform the Tests workflow runs on (.github/workflows/tests.yml runs uv run --group test pytest tests/ on ubuntu-latest). This contradicts the PR's "How to Test" section, which reports 4 passed, 2 skipped. Reproduced against the exact head:
$ uv run --with pytest --with pyyaml python -m pytest tests/test_sync_extensions.py -q -k VendorManifests
.....F [100%]
FAILED tests/test_sync_extensions.py::TestVendorManifests::test_missing_vendor_still_gets_symlink
AssertionError: assert ['missing: sk...codex-plugin'] == []
1 failed, 5 passed, 34 deselected
The 4 passed, 2 skipped figure is the Windows-without-privileges result: there, creating the symlink raises OSError and the test skips. On Linux symlinks are created without error, so sync_symlinks(check=False) returns the two missing: ... problem strings (fix mode reports everything it fixed) and the assertion fails. The except OSError guard never triggers on Linux, so the test cannot pass there. The repository's own test suite is therefore red on the current head.
Non-blocking design note
The fix is applied only to skills/iterate; the other ~64 skill/plugin directories with a .plugin/ manifest still use vendor symlinks on this head, and sync_symlinks keeps creating symlinks for any directory missing a vendor path. Since the reported root cause (Codex plugin add dropping symlinked directories) is not specific to iterate, those plugins would hit the same "missing or invalid plugin.json" failure when installed as Codex plugins. Consider whether the sync script should emit real mirror directories by default (or document why iterate is special) so this class of regression cannot recur for new skills.
🔄 CHANGES REQUESTED
| skill = _make_plugin_skill(tmp_path) | ||
| _point_sync_at(tmp_path, monkeypatch) | ||
| try: | ||
| assert sync_symlinks(check=False) == [] |
There was a problem hiding this comment.
This assertion fails on Linux (the platform CI uses), so the PR's own test suite is red on the current head. In fix mode sync_symlinks(check=False) returns the missing: skills/demo/.claude-plugin / missing: skills/demo/.codex-plugin problem strings for the symlinks it just created, so == [] is never true when symlink creation succeeds. except OSError only catches the Windows-without-privileges path (where the skip makes the test green); on Linux it reaches the assert and fails. Reproduced with uv run --with pytest --with pyyaml python -m pytest tests/test_sync_extensions.py -k VendorManifests (1 failed, 5 passed). Assert on the created symlinks instead - drop the == [] check and keep the is_symlink() assertions below, or expect the two missing: entries.
|
Fixed the Linux failure: |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Assessment
The change is correctly scoped to this repository: it fixes a Codex plugin-loading defect in the skills/ registry that this repo owns, and touches only the sync tooling, the skills/iterate vendor manifests, and the related tests.
The fix is sound. Replacing the skills/iterate/.codex-plugin and .claude-plugin symlinks with real directories whose plugin.json is byte-identical to .plugin/plugin.json survives the symlink-dropping copy that Codex plugin add performs, and the added sync_symlinks branch correctly treats a real vendor directory as valid when its manifest mirrors the canonical one, flags a stale copy, and refreshes it in fix mode. Missing vendors still get symlinks as before.
The previously reported failure on TestVendorManifests.test_missing_vendor_still_gets_symlink is resolved by commit 7d657fb, which stops asserting sync_symlinks(check=False) == [] (fix mode returns the missing: problem strings for paths it created) and instead verifies the created symlinks plus sync_symlinks(check=True) == [].
Verification on this head
uv run --with pytest --with pyyaml python -m pytest tests/test_sync_extensions.py -q-> 40 passed.uv run --with pytest --with pyyaml --with openhands-sdk python -m pytest tests/test_skill_plugin_loading.py -q-> 78 passed.python scripts/sync_extensions.py --checkexits 0 (only the non-blocking pre-existing coverage warning for./plugins/issue-duplicate-checker, unrelated to this PR).- GitHub Actions for head
7d657fb: Tests, Check Extensions, PR Artifacts, PR Description Check, Deprecation deadlines andprallcompleted/success. - The lossy-copy regression
test_lossy_install_copy_keeps_codex_manifest(#257) is present and passes.
No blocking or non-blocking findings remain.
✅ APPROVED
HUMAN:
I ran the targeted VendorManifests tests and verified the lossy-copy case fails on base but passes with the fix, with no new failures or ruff issues beyond the existing ones.
Why
skills/iterate/.codex-pluginis a symlink to.plugin/, and Codexplugin adddrops symlinked directories when materializing its install cache. The installed tree then has no.codex-plugin/plugin.json, so Codex fails with "missing or invalid plugin.json" and/iteratenever registers (#257).Summary
skills/iterate/.codex-pluginand.claude-pluginsymlinks with real directories containing byte-identicalplugin.jsoncopies (mode120000→100644).sync_symlinksnow accepts a real vendor dir whoseplugin.jsonmirrors the canonical one, flags stale copies, and refreshes them in fix mode; missing vendors still get symlinks as before.TestVendorManifeststests, including a lossy-install-copy simulation of the reported failure.Issue Number
Fixes #257
How to Test
python -m pytest tests/test_sync_extensions.py -q -k VendorManifests→ 4 passed, 2 skipped (symlink-creation tests skip on Windows without privileges; they run on Linux CI).test_sync_extensions.py: the 7 remaining failures are identical on base (Windows-env frontmatter/catalog issues), no new failures.ruff checkon touched files: no new findings (2 pre-existing F541s on base).Type
Bug fix