refactor: simplify the repository and resolve every finding it surfaced - #27
Merged
Merged
Conversation
`allow_implicit_invocation: true` restates the documented default, so the `policy` block on coordinate-codex-work carried no behavior. Verified against learn.chatgpt.com/docs/build-skills and against Codex's own loader, where an absent block and an explicit `true` both resolve through `unwrap_or(true)`. The sibling find-skill-candidates keeps its `false`, which is load-bearing. This moves the repo from 6-of-8 to 7-of-8 openai.yaml files declaring only deviations, matching the upstream scaffold, which emits interface-only. npm test 35/35, npm run validate PASS. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QH54tukcgvdxqwaTb5cdNB
The indexed plugin-builder "Build skills" page documents only the `dependencies` block of `agents/openai.yaml`; it covers neither `interface` nor `policy`. A reader routed there for skill UI or invocation metadata finds nothing, which AGENTS.md's preflight requires this index to prevent. Adds the Codex "Build skills" page, which documents all three keys plus local skill discovery and plugin distribution. Purely additive: the existing pointer is retained unaltered, since the doc-contract test pins 15 exact URLs. npm test 35/35, npm run validate PASS. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QH54tukcgvdxqwaTb5cdNB
… tests
Collector: reduce SKILL_WORD_PATTERN to `\bskills?\b` (the `SKILL\.md` and
`agents[/\\]skills` alternatives were subsumed, since their word boundaries
coincide with the alternation's); drop a `.lower()` that cannot fire, as
SKILL_PATTERN has no IGNORECASE and matches `[a-z0-9-]` only; drop a redundant
`list()` and correct the return annotation it obscured; replace safe_text's
per-node `len(" ".join(chunks))` with an incrementally maintained counter,
turning an O(n^2) budget check into O(n).
Tests: hoist findPython() to module scope (up to 9 probes per run down to 3),
extract makeTempDir/writeSession/runCollector, add the `u` flag to the 6 regex
literals this file was missing, and replace doesNotMatch(new RegExp(secret))
with an includes() check that does not depend on the fixture being free of
regex metacharacters.
Behavior preservation was proven, not assumed: stdout is byte-identical across
678 subprocess pairs covering the budget boundary, all secret patterns,
malformed JSONL, and every argparse path; the regex reduction was checked over
all 1,112,064 Unicode codepoints; and mutation tests confirm the redaction
assertions still fail when redaction is broken.
npm test 35/35 with 0 skipped, npm run validate PASS.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QH54tukcgvdxqwaTb5cdNB
…-dir leak
Validator: `includes("[TODO:") || includes("TODO:")` is provably equal to its
right operand, since "[TODO:" contains "TODO:" at offset 1. Hoist a single
`const name = plugin?.name` in place of eight mixed optional/non-optional
reads, and fold two adjacent guards on it into one block, preserving push
order. No error code, message, or file field changes.
Test: register `t.after` cleanup in createFixture, matching the convention in
tests/codex-operations.test.mjs. Each `npm test` previously leaked five
/tmp/codex-marketplace-test-* directories; now zero. Also declare the
marketplace entry once and add a `codesOf` helper for four inline repeats.
All 41 error codes are treated as frozen public contract, including the 34 no
test covers and the unreachable remote-source branches, since this file ships
to consumers who point it at their own marketplaces. Equivalence was proven
over 212 fixtures emitting all 41 codes plus 3,500 fuzz cases, comparing full
ordered result objects rather than code sets, and the CLI surface is
byte-identical.
npm test 35/35, npm run validate PASS.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QH54tukcgvdxqwaTb5cdNB
The five contract test files carried the same helper trio verbatim: `read` in all five, `readJson` and `compact` in four. Four "marketplace exposes X" tests were ~22-line near-clones differing only in name, version and category, and seven skill tests repeated the same frontmatter and default_prompt patterns. Adds tests/helpers.mjs with read/readJson/compact/escapeRegExp/once plus assertExists, skillPaths, assertMarketplacePlugin and readSkillContract, generalizing the path-builder pattern plugin-ops already used. Net -212/+114 across the five files. documentation-contract.test.mjs imports `read` and `escapeRegExp` only. It still does NOT use `compact`: its assertions are deliberately wrap-sensitive, and compacting them would silently weaken them. A comment records why. Test count stays 35: no test() was collapsed, so a failure still names the plugin that broke. The coverage asymmetries were left exactly as found rather than normalized, since fixing them ADDS assertions; they are filed separately. Preservation was proven, not inferred from a green suite: instrumenting node:assert to log every assertion with its pattern and a hash of its subject yields byte-identical multisets across all five files (596 assertions, zero differences), and 43 independent mutations produce identical failing-test sets before and after. npm test 35/35 with 0 skipped, npm run validate PASS. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QH54tukcgvdxqwaTb5cdNB
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.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: e1555450ff
ℹ️ 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".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
kyle-sexton
enabled auto-merge (squash)
August 30, 2026 22:06
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
Resolves every finding the simplification sweep deferred, in place rather than by filing: each is fixed, or closed as refuted/impossible with the primary source that settles it. Test gate: adds tests/test-harness.test.mjs, asserting package.json's hand-enumerated test list equals the *.test.mjs files actually on disk, in both directions. This closes a real fail-open gap — `node --test <missing>` exits 0 silently on Node 24, so a renamed test file previously made CI green while running fewer tests, and a newly added one was never run at all. Collector: rejects NaN/infinite --hours with a clean argparse error instead of an unhandled traceback, and likewise a finite-but-unrepresentable window (the real limit depends on `now`, so the window is now built inside parse_args rather than guarded by a magic constant); breaks skill-mention ties deterministically so output no longer varies with PYTHONHASHSEED; separates the `often`/`repeated` overlap that made one word increment two counters the report presents as independent; drops pipe-escaping that protected nothing outside a table and corrupted quoted evidence; records the real 3.11 floor. Validator: one defect now yields one error in the `apps` branch, symmetric with `mcpServers` by construction; reports the manifest path rather than the repo root for invalid-skills-path; distinguishes an unreadable file from invalid JSON; and no longer crashes on ENOTDIR paths. `duplicate-skill-name` is now scoped target-wide, so it is independently reachable as intended. Adds `unreadable-file` and `unreadable-path`; drops no existing code. Tests: closes the copy-paste coverage asymmetries (display_name is now a required parameter, so the gap cannot silently reopen), tightens the unanchored URL assertions, and relaxes the taskRefs assertion to a shape check so it stops freezing another host's absolute paths while still rejecting a wrong or swapped run. Validator coverage goes from 7 of 41 codes asserted to all 43. Docs: corrected against the Codex CLI source rather than a docs page. The `<plugin>@<marketplace>` and `--available --json` forms are confirmed correct (the docs page was incomplete); `--available` additionally requires `--json`. Plugin-provided skills resolve only under their plugin namespace, so MIGRATION-PLAYBOOK's bare `$migrate-plugin` was wrong and is now qualified. README gives codex-operations its own heading. The cheat sheet's verification basis is split per claim class so a re-verified date cannot be read as covering rows that remain unverifiable. Manifests are deliberately unchanged: `$schema` is REJECTED by OpenAI's own validate_plugin.py closed allowlist, and the duplicated author/homepage/ repository/license blocks have no inheritance mechanism in the Codex source. The JWT redaction fixture is assembled from its segments at run time rather than written as a literal, so the repository carries no credential-shaped string. npm test 114/114 with 0 skipped, npm run validate PASS, validator plugin-root mode PASS, SHA-256-pinned files byte-unchanged. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QH54tukcgvdxqwaTb5cdNB
kyle-sexton
force-pushed
the
claude/repo-code-tidying-simplify-bq22lo
branch
from
August 30, 2026 22:23
3446e3b to
112b918
Compare
All eight skills seeded `default_prompt` with a bare `$skill-name` mention that Codex cannot resolve. Codex qualifies a plugin-provided skill's name with its plugin namespace — `SkillNamespaceResolver::qualify()` produces `plugin:skill`, applied to `skill.name` in the skills loader — and explicit mention selection matches `mentions.contains_plain_name(skill.name)` against that qualified name, with no base-name alias in the selector, the name counter, or the TUI mirror. `default_prompt` itself is stored verbatim: the interface resolver only collapses whitespace and enforces a length cap, and the namespace pass touches `skill.name` without touching `skill.interface`. So the seeded text reaches mention resolution unqualified and matches nothing. Qualified is correct-or-neutral on every surface: where a client attaches a structured skill item, resolution happens by path and the text form is irrelevant; everywhere else the bare form was simply failing. The repo's own docs already used the qualified form, so these eight values were the drift. Also qualifies the `$verify-plugin` mention in the migrate-plugin skill body, which had the same defect and no test pinning it. The contract is now pinned rather than left to convention: `skillPaths` exposes its plugin name and `readSkillContract` asserts the qualified mention, so the drift cannot silently return. Verified by mutation — restoring a bare mention fails with "humanize must seed the qualified mention $humanize:humanize". npm test 114/114 with 0 skipped, npm run validate PASS. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QH54tukcgvdxqwaTb5cdNB
kyle-sexton
changed the title
This PR added a required pointer row to docs/OFFICIAL-DOCS.md without adding the URL to `requiredOfficialPointers`, so the test named "retains every required live OpenAI pointer" stayed green whether or not that row survived. Deleting or mistyping the new pointer would have regressed the documentation contract silently — the same fail-open shape this PR fixes elsewhere. Verified by mutation: with the URL listed, changing the linked target to `…/build-skillsX` now fails with "docs/OFFICIAL-DOCS.md must link exactly https://learn.chatgpt.com/docs/build-skills". Before this change that mutation passed. Raised by the Codex review bot on this PR. npm test 114/114 with 0 skipped, npm run validate PASS. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QH54tukcgvdxqwaTb5cdNB
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
No linked issue — repository-wide maintenance sweep, not tied to a tracked change.
Summary
A whole-repository simplification sweep, plus a second pass that resolves every finding the sweep surfaced. Nothing is deferred and nothing is filed for later: each finding is fixed here, or closed as refuted/impossible with the primary source that settles it.
Every behavior-preserving change was cleared by a fresh-context verifier whose job was to refute preservation rather than confirm it. Three proposed changes were refuted and reverted; they are recorded below rather than hidden.
Context inputs and precedence.
AGENTS.mdpreflight first, then the live pages named indocs/OFFICIAL-DOCS.md, then verified behavior, then repository conventions. Where a docs page and the Codex CLI source disagreed, the source won — several findings were only settleable by readingopenai/codexdirectly, and doing so overturned four conclusions drawn from documentation alone.Test coverage goes from 35 tests to 114, and validator error-code coverage from 7 of 41 asserted to all 43.
Fix
Simplification (behavior-preserving). Removed a redundant
policyblock restating a documented default; dead regex alternatives, a no-op.lower(), a redundantlist()and an O(n²) budget check in the session collector; a provably dead disjunct and mixed optional-chaining in the validator; and the helper trio duplicated across five test files plus four ~22-line near-clone tests, extracted intotests/helpers.mjs.Correctness fixes found along the way.
package.jsonhand-enumerates the test files and is the sole CI gate, butnode --test <missing>exits 0 silently on Node 24 — so a renamed test file made CI green while running fewer tests, and a newly added one never ran at all.tests/test-harness.test.mjsnow asserts the manifest and the on-disk set match, in both directions.plugin:skilland matches explicit mentions against that qualified name with no base-name alias, whiledefault_promptis stored verbatim and never rewritten. All eightdefault_promptvalues used the bare form and could not resolve. Now qualified, and pinned by the shared helper so the drift cannot return.--hourscrashed with a traceback instead of a clean argparse error (the real limit depends onnow, so the window is built insideparse_argsrather than guarded by a constant). Skill-mention ties followed set-iteration order, so output varied withPYTHONHASHSEED.often/repeatedsat in both the friction and workflow patterns, so one word incremented two counters the report presents as independent.appsbranch, symmetric withmcpServersby construction;invalid-skills-pathreports the manifest rather than the repo root; an unreadable file is distinguished from invalid JSON; and three ENOTDIR paths that crashed the CLI now degrade to errors.duplicate-skill-namewas scoped so narrowly it could never fire alone, and is now target-wide. Addsunreadable-fileandunreadable-path; drops no existing code.OFFICIAL-DOCS.mdrouted readers to a page documenting neitherinterfacenorpolicy(fixed additively — the existing pointer is test-pinned).MIGRATION-PLAYBOOK's bare$migrate-pluginwas wrong.READMEgavecodex-operationsno heading of its own.Findings closed as refuted or impossible, with the source that settles each.
$schemamust NOT be added. The Agent Plugins spec requires it, but OpenAI's ownplugin-creator/scripts/validate_plugin.pyuses a closed top-level allowlist that omits it. Injecting it yieldsplugin.json field $schema is not accepted by plugin validation, exit 1 — it would turn four passing plugins into four failing ones.author/homepage/repository/licenseblocks cannot be factored out. No inheritance orextendsexists in the Codex source (zero hits across the manifest and marketplace modules). The one near-miss, a#[serde(flatten)]fallback, is a strict either/or for sources without a readable manifest and is unreachable for all four local plugins.interfaceis contract-required. (This corrects an earlier revision of this description, which called it optional.)validate_plugin.pyrequires the object and five string fields, independently of this repo's own validator.<plugin>@<marketplace>and--available --jsonare correct (plugin_cmd.rs); the docs page was merely incomplete.--availableadditionally requires--json, which the cheat sheet now records.taskRefsis now checked by shape and still rejects a wrong or swapped run.melodic-software/standards(melodic-standards-sync[bot]; the commit bodies say "Do not hand-edit these managed files downstream"). Two carry real defects — twelve default-valued keys in.editorconfig-checker.jsonand a mangled comment in.editorconfig— which belong in a PR against that repository, since a local edit here is reverted by the next sync.Verification
Beyond the gates, each group was proven rather than assumed:
node:assertinstrumented at run time to log every assertion with its pattern and a hash of its subject. Before/after multisets were byte-identical: 596 assertions, zero differences. 43 independent mutations produced identical failing-test sets before and after.The two SHA-256-pinned files,
plugins/humanize/skills/humanize/SKILL.mdandreferences/revision-rubric.md, are byte-unchanged.Three changes were refuted and reverted. (1)
node --testwithout the explicit file list: it passes CI, butnpm test -- <path>changes meaning (the argument becomes the file list, running 9 tests instead of 35), Node's default discovery descends into dot-directories on Node 20 but not 22+ so it is unstable across the declaredengines.node: ">=20", and it converts a fail-closed manifest into fail-open discovery. (2) A first docs attempt asserted the two "Build skills" pages cover "disjoint halves" ofopenai.yaml; the relationship is nested, not disjoint. (3)$schema, as above.One security finding, fixed at the root. GitGuardian flagged a JWT-shaped fixture. It was synthetic (its signature decodes to
not-a-real-signature), but the fix is to assemble the value from its segments at run time so the repository carries no credential-shaped literal at all, rather than to split the string past the scanner. Because the scan covers every commit in the PR, the two commits carrying the literal were collapsed so no commit contains it; the tree hash was verified identical before and after, so history changed and content did not. The neighbouringghp_fixture was not flagged and is deliberately left as a plain literal, since splitting it would be evasion rather than a fix.Coverage gaps that remain, stated plainly.
collect_recent_sessions.pystill has no unit tests — its coverage is black-box subprocess tests, now substantially expanded. Some rows of the capability cheat sheet describe a run-time, account-dependent environment that no source read can confirm; those are labelled as not re-verified rather than covered by a refreshed date, because asserting a verification that was not performed would be false.Related
Live upstream sources, all verified 2026-08-30:
openai.yamlinterface/policy/dependenciesdependenciesblock$schema; refuted above)openai/codex@main—cli/src/plugin_cmd.rs,ext/skills/src/loader/{namespace,host,metadata}.rs,skills/src/{selection,mentions,interface,model}.rs,core-plugins/src/{manifest,marketplace}.rs, andplugin-creator/scripts/validate_plugin.pyKnown upstream follow-up, outside this repository: the TUI's local mention codec excludes
:from mention name characters while core's includes it, so a qualified mention typed in the TUI is not chipped locally even though core resolves it correctly. The bare form fails at both layers, so the qualified form here is strictly better.