Repository navigation
docs(skill): add the repo-owned ocx operating skill #2788
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
3fc82f6
docs(skill): add the repo-owned ocx operating skill with a generated …
lidge-jun 72e5c91
ci: name the skill-surface regeneration in its own step
lidge-jun 368d85b
docs(devlog): record wp9 and the audit finding that disproved my own …
lidge-jun 8490718
merge dest before skill contract fixes
04fcbc3
fix(skill): stop teaching false exit-0 and JSON-error contracts
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
122 changes: 122 additions & 0 deletions
122
devlog/_plan/260828_ocx_agentic_control/071_wp8_implementation_record.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,122 @@ | ||
| # 071 — wp8 implementation record: the `ocx` agent skill | ||
|
|
||
| Branch: `codex/ocx-agent-skill`, stacked on `codex/ocx-gui-parity`. | ||
| Plan: `070_phase_agent_skill.md`. All five accept criteria met. | ||
|
|
||
| ## What shipped | ||
|
|
||
| ``` | ||
| skills/ocx/ | ||
| SKILL.md 112 lines — orientation, exit codes, consent, routing | ||
| references/ | ||
| 01_management_surface.md 485 lines — GENERATED from the capability table | ||
| 02_json_shapes.md 125 lines — envelopes and which field to read | ||
| 03_recipes.md 168 lines — eight verified task sequences | ||
| 04_failure_semantics.md 84 lines — exit codes, 503 classes, retry policy | ||
| scripts/generate-ocx-skill-surface.ts the generator, with a --check mode | ||
| tests/skill-ocx.test.ts 11 tests | ||
| ``` | ||
|
|
||
| Repo-owned rather than `$CODEX_HOME/skills`, because the skill describes *this* repository's CLI | ||
| contract and has to version with it. A user-directory copy goes stale the moment the CLI changes. | ||
|
|
||
| ## The generated half is the point | ||
|
|
||
| `01_management_surface.md` is rendered from `src/cli/capabilities.ts` by | ||
| `scripts/generate-ocx-skill-surface.ts`, and `tests/skill-ocx.test.ts` asserts the committed file | ||
| matches regeneration byte for byte. `bun run skill:surface` writes it; `bun run skill:surface:check` | ||
| is what CI asserts. | ||
|
|
||
| Hand-writing it would have recreated exactly the defect this unit removed: a second description of | ||
| the CLI surface, free to drift from the first. Now a capability added without regenerating fails a | ||
| test instead of silently shipping a skill that describes an older build. | ||
|
|
||
| Driven red both ways: adding a comment to a capability left the check green (it renders no output), | ||
| and changing one `summary` string failed both the `--check` and the test. So the gate tracks content, | ||
| not incidental edits. | ||
|
|
||
| ## The command-existence gate found a real error — in the plan | ||
|
|
||
| One test extracts every `ocx <command>` presented as a command across all five pages and asserts each | ||
| exists in `CLI_COMMANDS`. It immediately failed on **`ocx request-history`**, which the plan's recipe | ||
| section named and which does not exist. The route-decision view is `ocx logs explain <request-id>`. | ||
|
|
||
| A skill that documents a command nobody can run is worse than no skill: an agent tries it, gets | ||
| `Unknown command`, and concludes the tool is broken. | ||
|
|
||
| Two more errors surfaced the same way, from checking against source rather than assuming: | ||
|
|
||
| | I wrote | Reality | | ||
| |---|---| | ||
| | `ocx access key add --label X` | `ocx access key create <name>` — positional | | ||
| | `ocx access key remove --id X` | `ocx access key remove <id> --yes` — positional | | ||
| | `ocx provider default X` | `ocx provider set-default X` | | ||
|
|
||
| ### The extractor needed a second pass | ||
|
|
||
| A raw prose scan produced two false positives worth recording: "driving ocx **programmatically**" | ||
| (an ordinary sentence) and "there is no `ocx request-history` command" — a line whose entire purpose | ||
| is to say the command does *not* exist. A gate that fails on documentation warning you about a | ||
| missing command is measuring the wrong thing. | ||
|
|
||
| The extractor now reads only fenced blocks and inline code spans, skipping spans on lines that | ||
| negate them. A companion test asserts it still finds `capabilities`, `ready`, `status`, `logs`, | ||
| `usage`, `account`, `storage`, and `inspect` — without that, narrowing the extractor could have made | ||
| the main assertion vacuously true. | ||
|
|
||
| ## Everything in the recipes was executed | ||
|
|
||
| Not transcribed from source. `ready`, `status`, `logs --jsonl`, `logs explain`, `access key list`, | ||
| `storage report`, `storage cleanup --percent 1`, `inspect star`, `inspect pacing`, | ||
| `inspect client-config`, `integration native list`, and `agent request-user-input` were all run | ||
| against the live proxy, and the field names in `02_json_shapes.md` were read off those responses. | ||
|
|
||
| `logs explain` is where that mattered: the real payload nests everything under `routeDecision` with | ||
| `candidates[].exclusions` and `selected.reason`, which is the part an operator actually needs and is | ||
| not obvious from the route name. | ||
|
|
||
| ## Consent, stated rather than implied | ||
|
|
||
| `SKILL.md` says plainly not to star the repository on the user's behalf, and says why: the POST | ||
| spends *their* GitHub identity and the server requires a dashboard session precisely so an agent | ||
| cannot answer that question for them. It also names the workarounds and forbids them — `gh`, a raw | ||
| HTTP call, a minted session. | ||
|
|
||
| Three tests hold that line: the prohibition must be present, no page may contain `gh api`, and no | ||
| page may end a line with a bare `POST /api/github/star`. The failure mode being guarded is a skill | ||
| that mentions a boundary and then hands over the workaround anyway. | ||
|
|
||
| ## Docs-site | ||
|
|
||
| `reference/cli.md` gains the exact exit-code table (0/2/4/5/1), the preview-first rule for | ||
| `storage cleanup`, an agent-orientation section pointing at `ocx capabilities --json` and | ||
| `skills/ocx/`, and a behavior-change list covering this unit: `doctor`/`sync-cache` exiting non-zero, | ||
| the 404→4 and 409→5 mapping, position-independent `--json`, `logs --model` actually filtering, and | ||
| `ocx storage` gaining subcommands while its bare form is unchanged. | ||
|
|
||
| Changes are **additive**, so the seven translated locales are less complete but do not contradict the | ||
| English source — which is the plan's stated requirement. Leaving them untranslated is preferred over | ||
| machine-translating a contract page. | ||
|
|
||
| ## AGENTS.md | ||
|
|
||
| One pointer added, distinguishing the three audiences: `skills/ocx/` operates a proxy, | ||
| `AGENTS_INSTALL.md` installs one, `AGENTS.md` changes the codebase. It also names the regeneration | ||
| commands, because a generated file whose generator is undiscoverable gets hand-edited. | ||
|
|
||
| ## Verification | ||
|
|
||
| ``` | ||
| bun test tests/skill-ocx.test.ts tests/repo-hygiene.test.ts tests/cli-capabilities.test.ts | ||
| → 34 pass, 0 fail across 3 files | ||
|
|
||
| bun run skill:surface:check → current | ||
| ./node_modules/.bin/tsc --noEmit → clean | ||
| bun run privacy:scan → passed | ||
| ``` | ||
|
|
||
| ## Subagent dispatch | ||
|
|
||
| Sol-tier spawns continued to return 429. The command verification, the three corrected recipe | ||
| errors, and the extractor false-positive analysis were done directly. | ||
|
|
129 changes: 129 additions & 0 deletions
129
devlog/_plan/260828_ocx_agentic_control/081_wp9_implementation_record.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,129 @@ | ||
| # 081 — wp9 implementation record: rebase no-op, CI triage, and an audit that disproved me | ||
|
|
||
| Closes the unit. Five fixes across three branches, each traced to a specific commit in | ||
| this stack before being changed. | ||
|
|
||
| ## The rebase was a no-op, and that was verified rather than assumed | ||
|
|
||
| `080.1` planned a parent-first rebase of eight branches onto a moved `dev`. `dev` had not | ||
| moved: it sat at `50e955604` when this unit opened, the same commit the stack was built | ||
| on. Rather than skip the step, the ancestry was proved link by link — | ||
| `git merge-base --is-ancestor` for each of the nine consecutive pairs from `origin/dev` | ||
| to `codex/ocx-agent-skill`, all OK. A restack still happened, because the fixes below | ||
| landed on lower branches and had to be carried up. | ||
|
|
||
| Pre-rewrite SHAs were snapshotted to `/tmp/wp9snap/pre-rebase-shas.txt` before any | ||
| history was rewritten, and every push used `--force-with-lease`. | ||
|
|
||
| ## Three CI failures, three distinct causes | ||
|
|
||
| CI was read for all nine PRs at once, as `080.3` requires. Grouping by cause first was | ||
| what made this cheap: two of the three failures appeared in every PR from a given depth | ||
| upward, which located them in the lowest PR showing them rather than the one where they | ||
| were noticed. | ||
|
|
||
| | Failure | Cause | Fixed in | | ||
| |---|---|---| | ||
| | `hygiene`: `empty_catch` on a docs-only PR | scanner matches added lines textually; two devlog lines quote the construct they argue against | `codex/ocx-agentic-control-roadmap` | | ||
| | `cli-native-profile`: expected 1, got 5 | 409 now maps to exit 5; the old assertion pinned the pre-mapping behaviour | `codex/ocx-transport-honesty` | | ||
| | `codex-retained-root-serialization`: expected 0, got 1 | a contended catalog lock was classified as a failure | `codex/ocx-uniform-contract` | | ||
|
|
||
| The 409 case was the only one where an existing test was edited, so it needed the | ||
| strongest justification. `025` line 51 declares the vocabulary — 0 ok, 2 usage, 4 not | ||
| found, 5 conflict — and the test asserted 1 only because every account-family failure | ||
| used to exit 1 regardless of status. It was pinning the defect. What the test exists to | ||
| cover, the idempotent cancel fallback and the absence of a spurious cleanup warning, is | ||
| untouched and still asserted. | ||
|
|
||
| Two CI entries that looked like failures were not: a `ci` job failing in 2–4s on every | ||
| PR was `needed job(s) did not pass: changes=cancelled`, the older run superseded by the | ||
| force-push, and a `windows-schtasks` failure timestamped before the push belonged to the | ||
| pre-push commit. Neither was chased as a defect. | ||
|
|
||
| ## The audit disproved a premise I had asserted | ||
|
|
||
| The A-gate reviewer returned **fail** with two blocking findings, and both were correct. | ||
| Recorded here in full because the value of the gate is precisely that it caught something | ||
| my own scan could not. | ||
|
|
||
| **Finding 1 — the stack did add a real empty catch.** I had scanned every changed file | ||
| with the gate's own exported `hasEmptyCatch` and got zero matches, and concluded the | ||
| hygiene failure was purely a prose match. The scan was sound and the conclusion was | ||
| wrong: `tests/management-route-registry.test.ts:138` wrapped `Bun.writeSync?.(0, "")` in a | ||
| catch whose body is only a comment, and the regex at `pr-hygiene.cjs:113` does not match a | ||
| comment-only body. So the gate would have gone green while a genuinely swallowed failure | ||
| shipped. The call was dead anyway — the probe writes its fixture with `node:fs` on the | ||
| next line — so the block was removed rather than given a handler. | ||
|
|
||
| Re-scanning with a comment-aware pattern found seven more files, all pre-existing on | ||
| `dev` (zero added catch lines in each), so they are out of this unit's scope. | ||
|
|
||
| **Finding 2 — the first sync-cache fix masked real failures.** `ok = wrote || | ||
| desiredDisabled || contended` treated Codex-integration-off as automatic success. But the | ||
| call passes `allowWhenDesiredDisabled: true`, so the OFF gate inside the refresh never | ||
| fires and the work is genuinely attempted. A falsy result with integration off therefore | ||
| means the refresh *failed*, and the expression returned 0 with `skipped: true` for exactly | ||
| that case — asserting a false reason, which is worse than the always-0 behaviour this unit | ||
| set out to fix. Now only `busy` is a skip; `database`, `unsafe-path`, and a falsy | ||
| `completed` all exit 1. `--json` gained `reason`, because `outcome` alone cannot separate a | ||
| contended lock from a hard serialization failure — both report `unavailable`. | ||
|
|
||
| ## Verification | ||
|
|
||
| Local runs were focused, per the operator's no-full-suite constraint; the three-platform | ||
| full suite is CI's job and is what settles wp6's deferred validation (`050` | ||
| §Verification exception). | ||
|
|
||
| - `tsc --noEmit` clean. | ||
| - 41 tests pass across `codex-composed-acceptance`, `local-management-direct-transport`, | ||
| `codex-retained-root-serialization`, and `cli-json-contract`; 37 across | ||
| `management-route-registry` and `cli-native-profile` in the first round. | ||
| - Red-first proof for the contended-lock fix: dropping `contended` from the success set | ||
| turns exactly one test red and leaves the other five passing, so the assertion is not | ||
| vacuous. | ||
| - Detector integrity proof: `hasEmptyCatch` still returns true for a catch with a literally | ||
| empty body and false for one that handles the error, so the reword did not weaken the | ||
| guard. (Phrased rather than quoted, because a literal example of the construct in this | ||
| file is itself an added line the scanner matches -- which is how this very sentence | ||
| failed the gate once.) | ||
|
|
||
| ## Second triage round: two more live failures, and a case both I and the audit missed | ||
|
|
||
| The first round's fixes exposed two failures that the earlier cancelled runs had hidden. | ||
| Both reproduced locally, so neither was treated as CI flake. | ||
|
|
||
| | Failure | Cause | Fixed in | | ||
| |---|---|---| | ||
| | `local-management-direct-transport`: extra `version` in identity | the version-skew fix (#2701) widened the identity payload; the exact-match assertion predates it | `codex/ocx-capability-registry` | | ||
| | `codex-composed-acceptance`: expected 0, got 1 | an absent catalog was classified as a failure | `codex/ocx-uniform-contract` | | ||
|
|
||
| The second one matters more than its diff suggests, because it shows the audit's finding 2 | ||
| was directionally right and still not the whole answer. | ||
| `invalidateCodexModelsCacheWithPermit` returns a bare boolean for four distinct | ||
| situations -- wrote the cache, no catalog file exists, the OFF gate fired, or it threw. | ||
| So `false` cannot be read as failure any more than it could be read as success. My first | ||
| version read it as success when integration was off (masking real failures, which the | ||
| audit caught); my second read it as failure (inventing one in a native home, which the | ||
| audit did not catch because the reviewer reasoned from the reason matrix and the | ||
| `existsSync` early return is not in it). | ||
|
|
||
| `!existsSync(catalogPath)` is now checked at the call site and joins a contended lock as a | ||
| benign skip, with `skippedReason` in `--json` so `skipped: true` is never opaque. The check | ||
| lives at the call site rather than in the shared function because that boolean is consumed | ||
| by a dozen management routes with no use for the distinction. | ||
|
|
||
| The lesson worth keeping: a boolean that means four things will be misread by whoever | ||
| reads it next, in whichever direction their test happens to cover. Three consecutive | ||
| attempts got a different subset right. | ||
|
|
||
| Both new assertions were driven red. Reverting the `version` expectation fails exactly that | ||
| test; dropping `noCatalog` from the success set fails exactly the composed-toggle | ||
| acceptance test. | ||
|
|
||
|
|
||
| ## Honest note on subagent substitution | ||
|
|
||
| Earlier phases in this unit recorded that subagent dispatch was failing with 429 rate | ||
| limits and that audits were performed directly. That constraint lifted here: the A-gate | ||
| reviewer for wp9 was a real dispatched `explorer` (gpt-5.6-sol, high effort), and its | ||
| two blocking findings are the substance of this record. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new check is skipped when a pull request changes only
skills/ocx/**: thegatesjob requiresneeds.changes.outputs.ci == 'true', but thecifilter at lines 176–193 does not includeskills/**; the push trigger omits it as well. Consequently, direct edits to the generated surface or invalid commands added to the handwritten references can receive a green aggregate check without running either this step ortests/skill-ocx.test.ts. Add the skill directory to both path allowlists.AGENTS.md reference: AGENTS.md:L184-L191
Useful? React with 👍 / 👎.