fix(skills): bundle split oo skill references#67
Conversation
Why: The oo skill now reads focused reference files instead of one monolithic contract, so bundled installs would miss required docs and drift from the checked-in ownership content. What: Split the oo guidance into dedicated reference documents for Codex and Claude, updated the Codex agent prompt, and registered each new reference in the embedded skill asset bundle. How: Refreshed the embedded asset tests and the ownership assertion so the packaged skill output matches the new reference layout. Signed-off-by: Kevin Cui <bh@bugs.cc>
Summary by CodeRabbitRelease Notes
WalkthroughThis PR replaces a single Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
contrib/skills/claude/oo/SKILL.md (1)
90-90: Consider simplifying "outside of" to "outside".The phrase "outside of" can be simplified to "outside" for conciseness. While both are grammatically correct, "outside" is more direct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/skills/claude/oo/SKILL.md` at line 90, The phrase "outside of" in SKILL.md should be simplified to "outside" for conciseness; locate the occurrence near the line containing "**end the response** after reporting the outcome. Never continue with work" and replace "outside of" with "outside", preserving surrounding punctuation and capitalization and ensuring no change to meaning or formatting in the SKILL.md content.contrib/skills/codex/oo/SKILL.md (1)
88-88: Consider simplifying "outside of" to "outside".The phrase "outside of" can be simplified to "outside" for conciseness. While both are grammatically correct, "outside" is more direct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/skills/codex/oo/SKILL.md` at line 88, Replace each occurrence of the phrase "outside of" with the simpler "outside" in the SKILL.md content; search for the token "outside of" (e.g., within sentences like "end the response after reporting the outcome. Never continue with work") and update them to "outside" to improve conciseness while preserving original meaning and punctuation.contrib/skills/codex/oo/references/package-execution.md (1)
118-121: Consider rephrasing to focus on observable behavior rather than implementation details.Lines 118-121 mention "validation is patched" and describe "current local validation" behavior, which reveals internal implementation mechanics. Based on learnings, documentation should focus on user-facing contract (observable behavior) rather than internal implementation details like validation patching.
Consider rephrasing to describe the observable validation requirements without mentioning the patching mechanism. For example:
- "File widget inputs require URI strings; local paths or binary payloads are not supported."
- "Input handles with
contentMediaType(exceptoomol/secret) are rejected during validation."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/skills/codex/oo/references/package-execution.md` around lines 118 - 121, Rewrite the two sentences to describe only observable behavior: replace "File widget validation is patched to require URI strings instead of local paths or binary payloads." with something like "File widget inputs require URI strings; local file paths and binary payloads are not supported." and replace "If an input handle schema contains `contentMediaType` and the value is not `oomol/secret`, current local validation rejects it." with "Input handles that include `contentMediaType` are rejected unless the value is `oomol/secret`." Ensure you remove words implying internal mechanics (e.g., "patched", "current local validation") and keep the references to File widget, `contentMediaType`, and `oomol/secret` so the contract is clear.contrib/skills/codex/oo/references/file-transfer.md (1)
25-25: Consider whether mentioning SQLite implementation is necessary.Line 25 states "Successful uploads persist a local sqlite record," which reveals an internal storage implementation detail. Based on learnings, documentation should focus on user-facing contract rather than internal mechanics.
If the SQLite detail provides value to users (e.g., for troubleshooting or understanding persistence guarantees), it can remain. Otherwise, consider rephrasing to focus on observable behavior: "Successful uploads are recorded locally for tracking purposes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/skills/codex/oo/references/file-transfer.md` at line 25, Update the documentation sentence that currently reads "Successful uploads persist a local sqlite record" to avoid leaking implementation details; either remove the mention of SQLite and rephrase to a user-facing behavior such as "Successful uploads are recorded locally for tracking purposes" or, if the SQLite detail is needed for troubleshooting, expand it to justify why and how users can inspect that record; change the exact string mentioned above accordingly.contrib/skills/claude/oo/references/package-execution.md (1)
118-121: Consider rephrasing to focus on observable behavior rather than implementation details.Lines 118-121 mention "validation is patched" and describe "current local validation" behavior, which reveals internal implementation mechanics. Based on learnings, documentation should focus on user-facing contract (observable behavior) rather than internal implementation details like validation patching.
Consider rephrasing to describe the observable validation requirements without mentioning the patching mechanism. For example:
- "File widget inputs require URI strings; local paths or binary payloads are not supported."
- "Input handles with
contentMediaType(exceptoomol/secret) are rejected during validation."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/skills/claude/oo/references/package-execution.md` around lines 118 - 121, Replace the implementation-focused phrases "validation is patched" and "current local validation rejects it" with user-facing, observable behavior statements; specifically, change the two bullets to say: "File widget inputs require URI strings; local filesystem paths and binary payloads are not supported." and "Input handles that include contentMediaType (except oomol/secret) are rejected by validation." Ensure you remove any mention of patching or local/internal validation mechanics and keep the statements as contract-level requirements.contrib/skills/codex/oo/references/connector-execution.md (1)
1-108: Consider reducing doc duplication between Codex and Claude connector references.This file is effectively identical to
contrib/skills/claude/oo/references/connector-execution.md; consider generating both from one source (or enforcing sync checks) to prevent drift as behavior evolves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/skills/codex/oo/references/connector-execution.md` around lines 1 - 108, The two docs contrib/skills/codex/oo/references/connector-execution.md and contrib/skills/claude/oo/references/connector-execution.md are near-identical and risk drifting; consolidate into a single source-of-truth and update build/CI to prevent divergence by either (a) extracting the shared canonical content into one file (e.g., contrib/skills/shared/oo/references/connector-execution.md) and making the codex and claude files include or symlink that file, or (b) keep one canonical file and add a CI sync check that fails the PR when the two files differ; modify the repo layout and CI scripts accordingly and update any doc references to point at the canonical symbol so future edits are made in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@contrib/skills/claude/oo/references/package-execution.md`:
- Around line 118-121: Replace the implementation-focused phrases "validation is
patched" and "current local validation rejects it" with user-facing, observable
behavior statements; specifically, change the two bullets to say: "File widget
inputs require URI strings; local filesystem paths and binary payloads are not
supported." and "Input handles that include contentMediaType (except
oomol/secret) are rejected by validation." Ensure you remove any mention of
patching or local/internal validation mechanics and keep the statements as
contract-level requirements.
In `@contrib/skills/claude/oo/SKILL.md`:
- Line 90: The phrase "outside of" in SKILL.md should be simplified to "outside"
for conciseness; locate the occurrence near the line containing "**end the
response** after reporting the outcome. Never continue with work" and replace
"outside of" with "outside", preserving surrounding punctuation and
capitalization and ensuring no change to meaning or formatting in the SKILL.md
content.
In `@contrib/skills/codex/oo/references/connector-execution.md`:
- Around line 1-108: The two docs
contrib/skills/codex/oo/references/connector-execution.md and
contrib/skills/claude/oo/references/connector-execution.md are near-identical
and risk drifting; consolidate into a single source-of-truth and update build/CI
to prevent divergence by either (a) extracting the shared canonical content into
one file (e.g., contrib/skills/shared/oo/references/connector-execution.md) and
making the codex and claude files include or symlink that file, or (b) keep one
canonical file and add a CI sync check that fails the PR when the two files
differ; modify the repo layout and CI scripts accordingly and update any doc
references to point at the canonical symbol so future edits are made in one
place.
In `@contrib/skills/codex/oo/references/file-transfer.md`:
- Line 25: Update the documentation sentence that currently reads "Successful
uploads persist a local sqlite record" to avoid leaking implementation details;
either remove the mention of SQLite and rephrase to a user-facing behavior such
as "Successful uploads are recorded locally for tracking purposes" or, if the
SQLite detail is needed for troubleshooting, expand it to justify why and how
users can inspect that record; change the exact string mentioned above
accordingly.
In `@contrib/skills/codex/oo/references/package-execution.md`:
- Around line 118-121: Rewrite the two sentences to describe only observable
behavior: replace "File widget validation is patched to require URI strings
instead of local paths or binary payloads." with something like "File widget
inputs require URI strings; local file paths and binary payloads are not
supported." and replace "If an input handle schema contains `contentMediaType`
and the value is not `oomol/secret`, current local validation rejects it." with
"Input handles that include `contentMediaType` are rejected unless the value is
`oomol/secret`." Ensure you remove words implying internal mechanics (e.g.,
"patched", "current local validation") and keep the references to File widget,
`contentMediaType`, and `oomol/secret` so the contract is clear.
In `@contrib/skills/codex/oo/SKILL.md`:
- Line 88: Replace each occurrence of the phrase "outside of" with the simpler
"outside" in the SKILL.md content; search for the token "outside of" (e.g.,
within sentences like "end the response after reporting the outcome. Never
continue with work") and update them to "outside" to improve conciseness while
preserving original meaning and punctuation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b07e14e2-877d-49fa-88e5-decdf67c7d46
📒 Files selected for processing (20)
contrib/skills/claude/oo/SKILL.mdcontrib/skills/claude/oo/references/auth-and-billing.mdcontrib/skills/claude/oo/references/connector-execution.mdcontrib/skills/claude/oo/references/file-transfer.mdcontrib/skills/claude/oo/references/oo-cli-contract.mdcontrib/skills/claude/oo/references/package-execution.mdcontrib/skills/claude/oo/references/search-and-selection.mdcontrib/skills/claude/oo/references/task-lifecycle.mdcontrib/skills/codex/oo/SKILL.mdcontrib/skills/codex/oo/agents/openai.yamlcontrib/skills/codex/oo/references/auth-and-billing.mdcontrib/skills/codex/oo/references/connector-execution.mdcontrib/skills/codex/oo/references/file-transfer.mdcontrib/skills/codex/oo/references/oo-cli-contract.mdcontrib/skills/codex/oo/references/package-execution.mdcontrib/skills/codex/oo/references/search-and-selection.mdcontrib/skills/codex/oo/references/task-lifecycle.mdsrc/application/commands/skills/embedded-assets.test.tssrc/application/commands/skills/embedded-assets.tssrc/application/commands/skills/index.test.ts
💤 Files with no reviewable changes (2)
- contrib/skills/claude/oo/references/oo-cli-contract.md
- contrib/skills/codex/oo/references/oo-cli-contract.md
The Windows CI job can fail while Bun downloads or extracts the baseline executable used by the compiled binary test. Retry only that known transient error and assert the retry behavior so real compile failures still surface immediately. Signed-off-by: Kevin Cui <bh@bugs.cc>
GitHub Actions on Windows can keep failing to download or extract the Bun baseline executable even after multiple retries. Treat only that known CI infrastructure error as skippable there so unrelated changes do not fail while real compile errors still surface everywhere else. Signed-off-by: Kevin Cui <bh@bugs.cc>
The compile smoke test exercises the published binary flow, but Windows CI was unstable because Bun compile downloads were flaky. Skip that path on Windows, remove the platform-specific fallback, and keep the slower compile coverage on Linux and macOS. Signed-off-by: Kevin Cui <bh@bugs.cc>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
contrib/ci/npm-packages.test.ts (2)
525-533: Allow sync test doubles for the injected retry hooks.Lines 528-529 force callers to wrap trivial stubs in Promise-returning functions even though
awaitalready handles non-Promise values. Widening these hooks tovoid | Promise<void>/unknown | Promise<unknown>would make the helper easier to stub and remove the need forasyncno-op callbacks in the retry tests.♻️ Proposed refactor
async function compileExecutableWithTransientRetry(options: { outputPath: string; runCompile: () => CompileSpawnResult; - removeOutput?: (outputPath: string) => Promise<void>; - sleep?: (delayMs: number) => Promise<unknown>; + removeOutput?: (outputPath: string) => void | Promise<void>; + sleep?: (delayMs: number) => void | Promise<unknown>; }): Promise<void> {As per coding guidelines, "Never mark a function
asyncif it contains noawait; removeasyncwhen all code paths are synchronous."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/ci/npm-packages.test.ts` around lines 525 - 533, The compileExecutableWithTransientRetry helper currently types the injected hooks removeOutput and sleep as returning Promises, forcing callers to wrap sync stubs; update the options parameter in compileExecutableWithTransientRetry so removeOutput is typed as (outputPath: string) => void | Promise<void> and sleep is typed as (delayMs: number) => unknown | Promise<unknown>, and keep the same default assignments (removeOutput => rm(...), sleep => Bun.sleep(...)) so both synchronous and Promise-returning test doubles are accepted without extra async wrappers.
90-90: Drop the unreachable Windows branch inside the skipped test.Line 90 already turns this case into
test.skipon Windows, so the condition at Line 322 is never false when this body runs. Makingchmod()unconditional here keeps the success path flatter.♻️ Proposed simplification
- if (process.platform !== "win32") { - await chmod(executablePath, 0o755); - } + await chmod(executablePath, 0o755);As per coding guidelines, "Eliminate dead code: remove unused functions, parameters, type fields, unreachable branches, redundant error rethrows, and
JSON.stringify() === undefinedchecks."Also applies to: 322-324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/ci/npm-packages.test.ts` at line 90, The test uses compiledBinaryInstallTest (set to test.skip on win32) so the Windows branch inside the test body is unreachable; remove the conditional branch that checks platform and instead call chmod(...) unconditionally in the test. Locate the test that uses compiledBinaryInstallTest and the inner platform check around chmod (the conditional branch that only runs chmod when not win32) and delete that branch, leaving a single, unconditional fs.chmod(...) call to simplify the success path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@contrib/ci/npm-packages.test.ts`:
- Around line 525-533: The compileExecutableWithTransientRetry helper currently
types the injected hooks removeOutput and sleep as returning Promises, forcing
callers to wrap sync stubs; update the options parameter in
compileExecutableWithTransientRetry so removeOutput is typed as (outputPath:
string) => void | Promise<void> and sleep is typed as (delayMs: number) =>
unknown | Promise<unknown>, and keep the same default assignments (removeOutput
=> rm(...), sleep => Bun.sleep(...)) so both synchronous and Promise-returning
test doubles are accepted without extra async wrappers.
- Line 90: The test uses compiledBinaryInstallTest (set to test.skip on win32)
so the Windows branch inside the test body is unreachable; remove the
conditional branch that checks platform and instead call chmod(...)
unconditionally in the test. Locate the test that uses compiledBinaryInstallTest
and the inner platform check around chmod (the conditional branch that only runs
chmod when not win32) and delete that branch, leaving a single, unconditional
fs.chmod(...) call to simplify the success path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5795626c-7e2a-461c-aa20-c3455af25581
📒 Files selected for processing (1)
contrib/ci/npm-packages.test.ts
1 participant
Why: The oo skill now reads focused reference files instead of one monolithic contract, so bundled installs would miss required docs and drift from the checked-in ownership content.
What: Split the oo guidance into dedicated reference documents for Codex and Claude, updated the Codex agent prompt, and registered each new reference in the embedded skill asset bundle.
How: Refreshed the embedded asset tests and the ownership assertion so the packaged skill output matches the new reference layout.