[codex] Add host adapter and Codex plugin metadata - #73
Conversation
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (20)
WalkthroughAdds a Codex native plugin manifest ( ChangesDual-host adapter and plugin metadata follow-ups
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BinValidate as bin/validate
participant PluginCheck as codex-plugin-manifest-check
participant OpenAiCheck as validate-openai-agent-metadata
participant HostLint as validate-host-adapter-syntax
BinValidate->>PluginCheck: run against .codex-plugin/plugin.json
PluginCheck-->>BinValidate: PASS/FAIL
BinValidate->>OpenAiCheck: run against skills/*/agents/openai.yaml
OpenAiCheck-->>BinValidate: PASS/FAIL
BinValidate->>HostLint: run against skills/*/SKILL.md, workflows/*.md
HostLint-->>BinValidate: PASS/FAIL
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![claude[bot]](https://avatars.github.com/in/1236702?s=60&v=4){kind=small}
| "/goal" | ||
| ].freeze | ||
|
|
||
| AVAILABLE_TOOL_TOKENS = [ |
There was a problem hiding this comment.
AVAILABLE_TOOL_TOKENS only checks `codex review, codex review --, and /simplify, but docs/host-adapter/contract.md ("Availability Checks" section) explicitly lists /address-review and /code-review alongside /simplify as Claude Code slash commands that must be availability-checked before use. Because those two tokens aren't in this list, a future skill/workflow edit could hardcode /address-review or /code-review without a host-branch: available-tool marker or nearby fallback language, and bin/validate-host-adapter-syntax would pass it silently — undermining exactly the portability guarantee this PR introduces. Consider adding `/address-review` and `/code-review` (or the un-backticked forms actually used in prose) to this list.
| CODEX_ONLY_END = "<!-- host-branch: codex-only end -->" | ||
| AVAILABLE_TOOL_START = "<!-- host-branch: available-tool start -->" | ||
| AVAILABLE_TOOL_END = "<!-- host-branch: available-tool end -->" | ||
| ALLOW_CODEX = "<!-- host-allow: codex-only -->" |
There was a problem hiding this comment.
ALLOW_CODEX (``) is wired into codex_only_lines but isn't exercised anywhere: it's not used in any current `skills//SKILL.md` or `workflows/.md` file, and `bin/host-adapter-syntax-test.rb` only tests the paired `codex-only start/end` markers, not this single-line marker. Minor, but it's an untested code path — either add a test fixture for it or drop it until it's needed.
| # Host Adapter Contract | ||
|
|
||
| Date: 2026-07-02 | ||
| Status: proposed |
There was a problem hiding this comment.
Nit: this contract is marked Status: proposed, but ADR 0001 (docs/adr/0001-identical-skill-text-across-hosts.md), which this contract operationalizes, is Status: accepted — and bin/validate-host-adapter-syntax already enforces this contract's rules in CI via bin/validate. Worth bumping this to accepted (or clarifying why it's still proposed) so readers don't wonder whether the enforced rules are provisional.
|
Reviewed this PR (host adapter contract + Codex plugin metadata). Overall this is solid: the Codex plugin manifest, its validator (
Left 3 inline comments, all minor/nit level:
No blocking correctness, security, or portability bugs found. |
![chatgpt-codex-connector[bot]](https://avatars.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbb3fbf80c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 2. The loaded skill's own base directory, when the host exposes it for an | ||
| installed skill. | ||
| 3. A repo-local pinned copy such as `.agents/skills/<name>`. |
There was a problem hiding this comment.
In repos that pin .agents/skills/<name> for compatibility, this order makes an agent launched from a globally installed skill use the global helper before checking the pinned helper, even though the same contract says pinned repo-local copies may carry compatibility changes and should be resolved before the installed home. That can run stale or incompatible preflight helpers for a consumer checkout; put the repo-local pinned copy ahead of the loaded installed skill after explicit env overrides.
Useful? React with 👍 / 👎.
| ].freeze | ||
|
|
||
| AVAILABLE_TOOL_PATTERNS = [ | ||
| { pattern: /`codex review|codex review --/, label: "codex review" }, |
There was a problem hiding this comment.
The alternation here isn't grouped, so it doesn't do what the other three patterns in this list do.
{ pattern: /`codex review|codex review --/, label: "codex review" },This parses as two independent alternatives: `codex review (backtick immediately followed by the text) OR codex review -- (no backtick required). A plain-prose mention like Run codex review now (no surrounding backticks, no -- flag) matches neither branch, so it silently bypasses the "needs availability-check" gate this script exists to enforce.
Compare with the other three entries, which correctly use a non-capturing group so the prefix/suffix boundary conditions apply to the whole alternation:
{ pattern: %r{(?:`|^|\s)/address-review(?:\s|`|$)}, label: "/address-review" },Likely fix: /`codex review(?:`| --)/ or similar, so any backtick-quoted or ---flagged mention is required to sit inside an available-tool branch or carry fallback language — matching the intent already demonstrated by the sibling patterns and by the test fixtures (which only ever exercise the backtick+no-flag case, so this gap isn't covered by bin/host-adapter-syntax-test.rb).
| next if allowed.include?(index) | ||
|
|
||
| window_start = [index - 3, 0].max | ||
| window_end = [index + 3, lines.length - 1].min |
There was a problem hiding this comment.
The "needs availability-check or fallback language" heuristic just checks whether any word from AVAILABILITY_WORDS (e.g. available, fallback) appears anywhere in the ±3-line window around the flagged mention — it doesn't check that the word is actually talking about the flagged tool.
This produces a real false pass in this very PR's diff: skills/address-review/SKILL.md:874 reads
- If this skill conflicts with broader agent defaults, this file wins only for `/address-review` workflow behavior; ...
- Resolve the review thread after replying when the concern is actually addressed and a thread ID is **available**
The check treats line 875's "available" (about thread-ID availability, unrelated to whether the /address-review slash command exists on the host) as satisfying the fallback-language requirement for line 874's `/address-review` mention. It passes for the wrong reason.
Since this validator is the mechanism enforcing the new Host Adapter Contract's "availability-check host-specific tools before use" rule (docs/host-adapter/contract.md), a proximity-only keyword window gives false confidence — a future edit could remove real fallback language elsewhere in a paragraph and this check would still pass as long as an unrelated "available"/"fallback" word happens to sit within 3 lines. Worth tightening (e.g., require the availability word in the same sentence/line, or reference the specific tool label) or at least being aware this is a soft check, not a precise one.
Review summaryReviewed the host adapter contract, Codex plugin manifest, OpenAI picker metadata, and the new host-syntax/manifest validation scripts, plus the path-resolution wording sweep across Overall: this is solid, well-documented infra work. The ADR + contract doc give a clear rationale for keeping shared skill text byte-identical across hosts, the I posted two inline findings on
Neither finding blocks merge — the current doc content passes correctly either way — but both are real gaps in the new enforcement mechanism that could let a future edit slip past silently. Everything else (installer tests, symlink-mode coverage for |
| ].freeze | ||
|
|
||
| AVAILABLE_TOOL_PATTERNS = [ | ||
| { pattern: /`codex review|codex review --/, label: "codex review" }, |
There was a problem hiding this comment.
The alternation here isn't grouped, so it doesn't do what the other three patterns in this list do.
{ pattern: /`codex review|codex review --/, label: "codex review" },This parses as two independent alternatives: `codex review (backtick immediately followed by the text) OR codex review -- (no backtick required). A plain-prose mention like Run codex review now (no surrounding backticks, no -- flag) matches neither branch, so it silently bypasses the "needs availability-check" gate this script exists to enforce.
Compare with the other three entries, which correctly use a non-capturing group so the prefix/suffix boundary conditions apply to the whole alternation:
{ pattern: %r{(?:`|^|\s)/address-review(?:\s|`|$)}, label: "/address-review" },Likely fix: /`codex review(?:`| --)/ or similar, so any backtick-quoted or ---flagged mention is required to sit inside an available-tool branch or carry fallback language — matching the intent already demonstrated by the sibling patterns and by the test fixtures (which only ever exercise the backtick+no-flag case, so this gap isn't covered by bin/host-adapter-syntax-test.rb).
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/update-changelog/SKILL.md (1)
112-117: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWire
UPDATE_CHANGELOG_SKILL_DIRthrough the host base first.The current default still drops straight to
.agents/skills/update-changelog, so an installed skill will ignore the host-exposed base directory and may fail to findbin/changelog-merged-prs. Please use the same env-var → loaded-skill-base → pinned-copy order here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/update-changelog/SKILL.md` around lines 112 - 117, The UPDATE_CHANGELOG_SKILL_DIR fallback is skipping the host-exposed base directory and going straight to the repo-local copy, which can break installed skills. Update the lookup order in the update-changelog skill script to follow the same env-var → loaded-skill-base → pinned-copy precedence used elsewhere, and ensure the call to changelog-merged-prs resolves from that computed base rather than assuming .agents/skills/update-changelog first.
🧹 Nitpick comments (3)
bin/codex-plugin-manifest-check (2)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor:
EXPECTED_SKILLS_PATHconstant isn't reused for the literal path check.
validate_skills_pathhardcodes"./skills/"instead of interpolatingEXPECTED_SKILLS_PATH(declared at line 12), so the two need to stay manually in sync if the expected path ever changes.Also applies to: 130-135
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/codex-plugin-manifest-check` at line 12, The skills path check in validate_skills_path is hardcoding "./skills/" instead of reusing EXPECTED_SKILLS_PATH, which can drift if the constant changes. Update validate_skills_path to build the check from EXPECTED_SKILLS_PATH and apply the same change anywhere else the literal path is repeated, so the constant is the single source of truth.
114-128: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOnly the first line of the README summary paragraph is compared.
body.each { ... return stripped unless stripped.empty? }returns as soon as it hits the first non-empty line after the title. If the README's summary paragraph wraps across multiple lines (common Markdown authoring style), the comparison invalidate_manifest_metadata(line 100) will only ever see that first line, causing spurious "description must match README summary" failures for a semantically-unchanged, just-reformatted paragraph.♻️ Proposed fix: join contiguous non-empty lines into one paragraph
body.each do |line| stripped = line.strip - return stripped unless stripped.empty? + next if stripped.empty? + paragraph = [stripped] + body[(body.index(line) + 1)..].each do |next_line| + next_stripped = next_line.strip + break if next_stripped.empty? + paragraph << next_stripped + end + return paragraph.join(" ") end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/codex-plugin-manifest-check` around lines 114 - 128, `read_readme_description` only returns the first non-empty line after the README title, so wrapped summary paragraphs are truncated before `validate_manifest_metadata` compares them. Update `read_readme_description` to collect and join the contiguous non-empty lines of the first paragraph after the `# ` heading, then return that full paragraph string instead of a single line. Keep the existing error handling for missing summary text.bin/validate (1)
51-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale banner label: "seam doctor unit tests" now covers unrelated suites.
Three new, unrelated test files (
codex-plugin-manifest-check-test.rb,validate-openai-agent-metadata-test.rb,host-adapter-syntax-test.rb) were appended under the== seam doctor unit tests ==banner, making the label misleading for anyone scanning CI output.✏️ Suggested rename
-echo "== seam doctor unit tests ==" +echo "== validator unit tests =="🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/validate` around lines 51 - 55, The `bin/validate` test section banner is outdated because `== seam doctor unit tests ==` now precedes unrelated suites. Update the echoed label to match the full group of tests run in this block, and keep the banner aligned with the test commands such as `bin/agent-workflow-seam-doctor-test.rb`, `bin/codex-plugin-manifest-check-test.rb`, `bin/validate-openai-agent-metadata-test.rb`, and `bin/host-adapter-syntax-test.rb`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/codex-plugin-manifest-check`:
- Around line 20-34: The hardcoded branch-name matcher in
CONSUMER_POLICY_PATTERNS is inconsistent because the branch-name regex in
codex-plugin-manifest-check is case-sensitive while the sibling patterns are
not. Update the /\b(main|master)\b/ entry to use the same case-insensitive
behavior as the other policy patterns so references like Main or MASTER are
matched, and keep the change localized to the CONSUMER_POLICY_PATTERNS list.
In `@bin/validate-host-adapter-syntax`:
- Around line 138-149: The availability guard in validate-host-adapter-syntax is
too loose because the surrounding 3-line context in the main scan can let a
generic availability/fallback mention satisfy the check even when the tool
reference itself is unguarded. Tighten the logic in the lines.each_with_index
loop by requiring the availability/fallback words to appear in the same bullet
or paragraph as the matched tool pattern, using the existing
AVAILABLE_TOOL_PATTERNS and AVAILABILITY_WORDS symbols to locate and validate
the reference more precisely.
In `@bin/validate-openai-agent-metadata`:
- Around line 44-46: The validation in validate-openai-agent-metadata is too
broad because the current default_prompt scan pattern can mistakenly treat
dollar amounts like $20 as skill references. Tighten the matching logic in the
prompt_refs check so it only recognizes real skill IDs or directly compares
against the expected $token, and update the validation flow around
default_prompt and expected_skill accordingly. Add a regression test covering a
prompt that mentions money to ensure dollar values no longer fail validation.
In `@skills/plan-pr-batch/SKILL.md`:
- Around line 98-100: The PLAN_PR_BATCH_SKILL_DIR fallback is resolving too
early, which bypasses the host-exposed loaded-skill base and can miss
bin/pr-file-touch-map. Update the shell logic in the skill bootstrap to resolve
PLAN_PR_BATCH_SKILL_DIR by checking the explicit env var first, then the
loaded-skill base, then the repo-local pinned copy, and only then falling back
to .agents; keep the existing pr-file-touch-map invocation wired to that
resolved path.
---
Outside diff comments:
In `@skills/update-changelog/SKILL.md`:
- Around line 112-117: The UPDATE_CHANGELOG_SKILL_DIR fallback is skipping the
host-exposed base directory and going straight to the repo-local copy, which can
break installed skills. Update the lookup order in the update-changelog skill
script to follow the same env-var → loaded-skill-base → pinned-copy precedence
used elsewhere, and ensure the call to changelog-merged-prs resolves from that
computed base rather than assuming .agents/skills/update-changelog first.
---
Nitpick comments:
In `@bin/codex-plugin-manifest-check`:
- Line 12: The skills path check in validate_skills_path is hardcoding
"./skills/" instead of reusing EXPECTED_SKILLS_PATH, which can drift if the
constant changes. Update validate_skills_path to build the check from
EXPECTED_SKILLS_PATH and apply the same change anywhere else the literal path is
repeated, so the constant is the single source of truth.
- Around line 114-128: `read_readme_description` only returns the first
non-empty line after the README title, so wrapped summary paragraphs are
truncated before `validate_manifest_metadata` compares them. Update
`read_readme_description` to collect and join the contiguous non-empty lines of
the first paragraph after the `# ` heading, then return that full paragraph
string instead of a single line. Keep the existing error handling for missing
summary text.
In `@bin/validate`:
- Around line 51-55: The `bin/validate` test section banner is outdated because
`== seam doctor unit tests ==` now precedes unrelated suites. Update the echoed
label to match the full group of tests run in this block, and keep the banner
aligned with the test commands such as `bin/agent-workflow-seam-doctor-test.rb`,
`bin/codex-plugin-manifest-check-test.rb`,
`bin/validate-openai-agent-metadata-test.rb`, and
`bin/host-adapter-syntax-test.rb`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6dfacafd-f9d6-4ed9-a870-a948becae441
📒 Files selected for processing (33)
.codex-plugin/plugin.jsonREADME.mdbin/codex-plugin-manifest-checkbin/codex-plugin-manifest-check-test.rbbin/host-adapter-syntax-test.rbbin/install-agent-workflows-test.bashbin/validatebin/validate-host-adapter-syntaxbin/validate-openai-agent-metadatabin/validate-openai-agent-metadata-test.rbdocs/adr/0001-identical-skill-text-across-hosts.mddocs/host-adapter/contract.mddocs/installation-and-upgrades.mdskills/address-review/SKILL.mdskills/address-review/agents/openai.yamlskills/adversarial-pr-review/agents/openai.yamlskills/autoreview/SKILL.mdskills/autoreview/agents/openai.yamlskills/plan-pr-batch/SKILL.mdskills/post-merge-audit/SKILL.mdskills/post-merge-audit/agents/openai.yamlskills/pr-batch/SKILL.mdskills/pr-batch/agents/openai.yamlskills/replicate-ci/agents/openai.yamlskills/spec/agents/openai.yamlskills/triage/agents/openai.yamlskills/update-changelog/SKILL.mdskills/update-changelog/agents/openai.yamlskills/verify/agents/openai.yamlworkflows/address-review.mdworkflows/adversarial-pr-review.mdworkflows/continuous-evaluation-loop.mdworkflows/pr-processing.md
| `PLAN_PR_BATCH_SKILL_DIR="${PLAN_PR_BATCH_SKILL_DIR:-.agents/skills/plan-pr-batch}"; "${PLAN_PR_BATCH_SKILL_DIR}/bin/pr-file-touch-map" N --repo OWNER/REPO --cross-check` | ||
| Resolve `PLAN_PR_BATCH_SKILL_DIR` with the explicit env-var, loaded skill | ||
| base, repo-local pinned-copy chain before using the fallback assignment. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Resolve the helper dir before falling back to .agents.
This assignment still skips the host-exposed loaded-skill base and goes straight to the repo-local pinned copy, so installed/shared copies can miss bin/pr-file-touch-map. Please thread the env-var → loaded-skill-base → pinned-copy chain through the actual shell code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/plan-pr-batch/SKILL.md` around lines 98 - 100, The
PLAN_PR_BATCH_SKILL_DIR fallback is resolving too early, which bypasses the
host-exposed loaded-skill base and can miss bin/pr-file-touch-map. Update the
shell logic in the skill bootstrap to resolve PLAN_PR_BATCH_SKILL_DIR by
checking the explicit env var first, then the loaded-skill base, then the
repo-local pinned copy, and only then falling back to .agents; keep the existing
pr-file-touch-map invocation wired to that resolved path.
Review SummaryThis is a well-structured PR that formalizes host-adapter conventions (Codex vs. Claude Code) with genuinely good self-validation: Two things worth a look (posted as inline comments):
Nice touches: the symlink-escape test/guard for the plugin manifest's skills path, the ADR explaining why install-time text rewriting was rejected, and the |
| `codex review --base origin/<base>` or the PR's real base, before PR creation or | ||
| update. | ||
| primary local/adversarial self-review gate, normally available `codex review --base origin/<base>` or the PR's real base, before PR creation or update. | ||
|
|
There was a problem hiding this comment.
This paragraph now says the primary review gate runs "when that CLI is available" but doesn't say what to do otherwise. Compare with skills/autoreview/SKILL.md (also touched by this PR), which explicitly chains: verify Codex is available → fall back to Claude review tooling → if neither exists, stop and report which engines are missing. The very next paragraph here (L263-264) already delegates the /simplify half of this same gate to the canonical "Pre-Push AI Review And Simplify Gate" section in pr-processing.md — this review-gate paragraph could do the same (or explicitly state the fallback) instead of silently leaving "not available" unhandled for a high-risk/hosted-CI-labeled PR.
Same gap exists in the near-duplicate paragraph in workflows/pr-processing.md around line 654.
| end | ||
|
|
||
| expected = skill_names(File.join(root, EXPECTED_SKILLS_PATH)) | ||
| actual = skill_names(skills_root) |
There was a problem hiding this comment.
resolve_relative_path's traversal guard (path.absolute? || path.each_filename.any? { |part| part == ".." }) is unreachable in practice: its only caller, validate_skills_path, already returns early with "skills must be "./skills/"" unless manifest["skills"] == "./skills/" exactly — so raw_path is always that fixed relative literal by the time this method runs, and cleanpath on it never has an absolute path or a .. segment to catch. The symlink-escape check just below it (via File.realpath) is still reachable and is exercised by test_skills_path_must_not_escape_plugin_root_through_symlink, but this specific guard isn't covered by any test and can't be exercised given the current call site. Not harmful, just worth trimming or leaving a comment noting it's intentional defense-in-depth for a future looser caller.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e64961505
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| unless value.is_a?(String) && !value.strip.empty? | ||
| failures << "#{relative_path}: interface.#{field} must be a non-empty string" |
There was a problem hiding this comment.
The validator only rejects empty strings, so bin/validate passes the new openai.yaml files even when interface.short_description is below the documented 25-character minimum for Codex picker metadata; this commit already adds too-short values such as skills/pr-batch/agents/openai.yaml (23 chars), plus adversarial-pr-review, post-merge-audit, triage, and verify. Please enforce the 25–64 char constraint here so invalid picker metadata cannot ship through the new validation gate.
Useful? React with 👍 / 👎.
|
|
||
| require "pathname" | ||
|
|
||
| ROOT = Pathname.new(ARGV[0] || ".").expand_path |
There was a problem hiding this comment.
Unlike the two sibling validators added in this PR (codex-plugin-manifest-check resolves its root via File.expand_path("..", __dir__), validate-openai-agent-metadata via File.join(__dir__, "..")), this script defaults ROOT to "." — the process's current working directory — when no argument is given.
bin/validate happens to cd to the repo root before calling this script with no args, so the gate itself is fine. But if anyone runs bin/validate-host-adapter-syntax directly from another directory (e.g. from inside skills/, or after cd-ing into an installed pack subdirectory), ROOT.glob("skills/*/SKILL.md") + ROOT.glob("workflows/*.md") silently returns zero files and the script still prints PASS host adapter syntax — a false pass with no files actually checked, rather than an error.
Consider defaulting to File.expand_path("..", __dir__) like the other two validators for CWD-independent, fail-safe behavior.
| AVAILABLE_TOOL_PATTERNS = [ | ||
| { pattern: /(?:`|^|\s)codex review(?:`|\s+--|\s|$)/, label: "codex review" }, | ||
| { pattern: %r{(?:`|^|\s)/address-review(?:\s|`|$)}, label: "/address-review" }, | ||
| { pattern: %r{(?:`|^|\s)/code-review(?:\s|`|$)}, label: "/code-review" }, | ||
| { pattern: %r{(?:`|^|\s)/simplify(?:\s|`|$)}, label: "/simplify" } | ||
| ].freeze |
There was a problem hiding this comment.
Minor completeness gap: these patterns only match when the tool phrase is immediately followed by a backtick, `\s+--`, whitespace, or end-of-line. A mention followed by other punctuation (comma, period, closing paren) slips through undetected — e.g. the pre-existing workflows/pr-processing.md lines "...Cursor Bugbot, Codex review, and any repo-specific reviewer bot" and "...Greptile, Codex review, or another AI reviewer..." aren't flagged even though they lack adjacent availability/fallback language, simply because "review" is followed by a comma rather than whitespace/backtick.
Not a blocker (those two lines predate this PR and describe a GitHub-bot listing rather than the CLI), but it does mean the new host-adapter-syntax gate gives incomplete coverage for prose that mentions these tools with trailing punctuation. Might be worth tightening the lookaheads (e.g. [\s,.)]|$) so bin/validate doesn't give false confidence here.
Review SummaryReviewed this PR (Host Adapter Contract, Codex plugin metadata, OpenAI picker metadata, host-syntax linting, manifest validation) against portability rules, shell/Ruby helper safety, correctness, and security. Overall: solid, well-scoped work. The three new Ruby validators ( Findings (posted inline):
No security, correctness, or portability issues found beyond the above. Nice attention to detail on the ADR and Host Adapter Contract docs. |
| **For parallel batch scheduling, always pass `--cross-check`** so the local | ||
| diff and the Files API must independently agree on the path set — a | ||
| fail-safe against a silent under-report scheduling two colliding items into | ||
| the same wave: | ||
| Resolve `PLAN_PR_BATCH_SKILL_DIR` with the explicit env-var, loaded skill | ||
| base, repo-local pinned-copy chain before using the fallback assignment. | ||
| Then run: | ||
| `PLAN_PR_BATCH_SKILL_DIR="${PLAN_PR_BATCH_SKILL_DIR:-.agents/skills/plan-pr-batch}"; "${PLAN_PR_BATCH_SKILL_DIR}/bin/pr-file-touch-map" N --repo OWNER/REPO --cross-check` |
There was a problem hiding this comment.
Bug: broken sentence — a clause was deleted instead of appended.
The edit removed the line the same wave: (which completed "...a fail-safe against a silent under-report scheduling two colliding items into the same wave:") and replaced it with the new PLAN_PR_BATCH_SKILL_DIR resolution guidance, but didn't preserve the original clause. The merged text now reads:
...a fail-safe against a silent under-report scheduling two colliding items into Resolve
PLAN_PR_BATCH_SKILL_DIRwith the explicit env-var, loaded skill base, repo-local pinned-copy chain before using the fallback assignment. Then run:PLAN_PR_BATCH_SKILL_DIR=...
This is ungrammatical and drops the explanation of why --cross-check matters (colliding items scheduled into the same wave). Suggest restoring the dropped clause, e.g.:
| **For parallel batch scheduling, always pass `--cross-check`** so the local | |
| diff and the Files API must independently agree on the path set — a | |
| fail-safe against a silent under-report scheduling two colliding items into | |
| the same wave: | |
| Resolve `PLAN_PR_BATCH_SKILL_DIR` with the explicit env-var, loaded skill | |
| base, repo-local pinned-copy chain before using the fallback assignment. | |
| Then run: | |
| `PLAN_PR_BATCH_SKILL_DIR="${PLAN_PR_BATCH_SKILL_DIR:-.agents/skills/plan-pr-batch}"; "${PLAN_PR_BATCH_SKILL_DIR}/bin/pr-file-touch-map" N --repo OWNER/REPO --cross-check` | |
| the same wave. Resolve `PLAN_PR_BATCH_SKILL_DIR` with the explicit env-var, loaded skill | |
| base, repo-local pinned-copy chain before using the fallback assignment. | |
| Then run: |
Review SummaryReviewed the diff for portability (per Overall: the change is well-scoped and internally consistent. The new One real bug found — left as an inline comment on Minor observations (non-blocking):
I was unable to execute |
ReviewOverall this is a careful, well-tested change — the new Ruby validators ( Two things worth addressing before/after merge:
Nit: |
1 participant
Summary
Closes #42
Closes #58
Closes #36
Closes #60
Closes #43
QA Evidence
workflows/pr-processing.md; bounded coordination doctor/status recovery degraded toUNKNOWN, while direct claim refresh succeeded for Follow-up: Define Codex vs Claude host adapter contract #42/Follow-up: Resolve skill helper paths when running as installed skills #58/Follow-up: Add native plugin manifests for agent-workflows distribution #36/Follow-up: Expand agents/openai.yaml Codex picker metadata coverage #60/Follow-up: Add host-specific install validation fixtures #43 andqa/host-adapteruntil 2026-07-04T07:23Z.ruby bin/codex-plugin-manifest-check-test.rbruby bin/host-adapter-syntax-test.rbruby bin/validate-openai-agent-metadata-test.rbruby bin/codex-plugin-manifest-checkbin/validate-host-adapter-syntaxruby bin/validate-openai-agent-metadatapython3 /Users/justin/.codex/skills/.system/plugin-creator/scripts/validate_plugin.py /Users/justin/.codex/worktrees/7c73/agent-workflowsgit diff --checkbin/validatecodex review --base origin/mainexposed concrete hardening issues earlier; those were fixed. A final boundedtimeout 180 codex review --base origin/mainattempt timed out with exit 124 after rerunning validation internally, so terminal review result remainsUNKNOWN/tool-stalled.n/a.Batch Split State
qa/host-adapter: local validation complete; broad coordination and terminal AI-review state remainUNKNOWNas noted above.Merge Authority
merge_authority: ask. Not merged.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation