Repository navigation
build: generate the RPC params catalog and bundled skill guides at build time - #30644
brennanb2025 wants to merge 30 commits into
Conversation
…ild time Both files were derived from in-repo source, checked in, and compared by CI, so forgetting to regenerate cost a CI round and every guide edit conflicted on one JSON-string line. build:packages (postinstall, every test, typecheck, dev and build script, and CI's install action) now writes them; they are gitignored. The cross-version harness derives the catalog for release trees that lack it.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 6 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
| Description check | |
The description explains the problem, implementation, validation, scope, and checklist in detail. It does not follow the required template fully: it omits the required Linked Issue entry, the Visual P… | Add an actual issue reference in a Fixes #<issue> entry, add the required Visual Proof section with N/A and a reason if no visual change exists, and add the Testing section with completed or explained test checkboxes. Reconcile the vali… |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: generating the RPC parameter catalog and bundled skill guides during builds. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Description check
-
Explanation
The description explains the problem, implementation, validation, scope, and checklist in detail. It does not follow the required template fully: it omits the required Linked Issue entry, the Visual Proof section with an explicit N/A statement, and the Testing section with the required test checkboxes. The description also reports current-head success, while the provided objectives report an unresolved current workflow failure and merge conflicts.
Resolution
Add an actual issue reference in a
Fixes #<issue>entry, add the required Visual Proof section withN/Aand a reason if no visual change exists, and add the Testing section with completed or explained test checkboxes. Reconcile the validation claims with the latest CI status and document the current merge-conflict and end-to-end typecheck-ratchet state.
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
The path-literal ratchet rejects src/ paths that name no tracked file. oxlint already skips gitignored files.
…owaway cache Deriving loads the release's whole RPC registry, so it runs in its own process rather than the test worker. The HEAD test uses a temporary cache so it doesn't leave one extracted tree per commit.
… the catalog A concurrent typecheck, test run or dev watcher now never reads a half-written module. The cross-version harness derives the catalog only for releases whose tree has its directory, since both arrived together.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
ee82c60c-61f0-494a-8507-0f4d097ee684
⛔ Files ignored due to path filters: 1
src/shared/rpc-contract/rpc-params-catalog.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing: 15
.gitattributes.github/workflows/pr.yml.gitignore.oxlintrc.jsonconfig/oxlint-anti-slop.jsonconfig/scripts/generate-bundled-skill-guides.mjsconfig/scripts/generate-bundled-skill-guides.test.mjsconfig/scripts/generate-rpc-params-catalog.mjsconfig/scripts/pr-preflight-gates.test.mjsmobile/README.mdmobile/package.jsonpackage.jsonsrc/cli/bundled-skill-guides.tstests/e2e/cross-version-wire/release-checkout.tstests/e2e/cross-version-wire/release-checkout.unit.test.ts
💤 Files with no reviewable changes: 5
- .oxlintrc.json
- config/scripts/pr-preflight-gates.test.mjs
- config/oxlint-anti-slop.json
- .github/workflows/pr.yml
- .gitattributes
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.
|
Two checked-in generated modules duplicated their source inputs, causing drift failures and merge conflicts. This PR generates the RPC parameter catalog and embedded CLI guides before supported commands, so developers no longer commit those copies. Packaged guide output and RPC behavior keep their existing contracts. Current state: review is complete on Fixed: nine root and four mobile entry points prepare missing or stale output through the existing build owner while retaining arguments, working directories and failure behavior. The aggregate prepares once before 14 raw children. Both producers publish through temporary sibling files and rename; identical content retains its modification time. Installable guide projections stay tracked and checked. Cross-version tests derive missing catalogs from the extracted release's own declarations, preserving existing catalogs and releases from before the catalog existed. CI passed on the current head: full PR workflow, 23 successful jobs and seven scoped skips. The complete current-head check snapshot has 51 successes and 11 policy skips, none pending or failing; dependency security and WSL also pass. The synthetic CI tree exactly equals Packaged QA: fresh normal unsigned arm64 packages on exact QA carried forward: the tested branch is QA observations: both refs lose the displayed Low label after the reply without changing controls; effective effort and cause remain unverified. Main alone logs an at-rest slash-discovery SQLite initialization warning while the app and successful provider are alive; no slash-functionality pass is claimed. Real config changes are hash-only observations without writer attribution. The branch's optional post-driver backend snapshot failed in the collector; earlier live backend isolation and named closure are retained separately. A QA scratch file initially entered the preliminary package; that package is excluded from final evidence, and a normal corrected package ran the complete assigned flow. No product fix or extra provider flow was added. Deferred and limits: raw tools/editors need build preparation after checkout or input changes; supported package commands prepare automatically. Unreproduced external Windows sharing, abrupt-temp cleanup and cancellation concerns remain deferred after bounded source traces. Generation overhead was source-reviewed, not benchmarked. No all-provider, resume/upgrade, phone/remote UI, signed installation or updater-action coverage is claimed. This PR adds no agent/native-chat implementation change. For Brennan's review The same fixture, prompt and defaults on pinned main
|
…2025/generated-files-build-time # Conflicts: # src/shared/rpc-contract/rpc-params-catalog.generated.ts
…2025/generated-files-build-time
…2025/generated-files-build-time # Conflicts: # src/shared/rpc-contract/rpc-params-catalog.generated.ts
…2025/generated-files-build-time # Conflicts: # src/shared/rpc-contract/rpc-params-catalog.generated.ts
…2025/generated-files-build-time # Conflicts: # src/cli/bundled-skill-guides.ts # src/shared/rpc-contract/rpc-params-catalog.generated.ts
…2025/generated-files-build-time # Conflicts: # src/shared/rpc-contract/rpc-params-catalog.generated.ts
|
Closing in favor of #33712. Two independent reviewers, one Claude and one Codex, looked at whether this was still worth merging, and over two rounds they agreed it is not. Why not:
#33712 keeps both generated files committed, along with their CI check. It writes each guide one line per markdown line, with the strings byte-identical, so concurrent guide edits merge the way their markdown sources do. |
Problem and result
The RPC parameter catalog and embedded CLI skill guides duplicated source already in the repository. Developers had to regenerate and commit those copies; omissions failed CI, and large generated lines caused merge conflicts. This PR generates the copies before supported build and validation commands, so developers no longer commit them. Shipped guide output, RPC messages, saved data and user defaults retain their existing contracts.
The common pattern is to generate derived build inputs before their consumers. Raw tools and editor sessions still require
pnpm run build:packagesafter checkout or input changes (pnpm --dir .. run build:packagesfrom mobile); supported package commands prepare automatically. Installable skill guides stay tracked and checked.Changes
build:packagesowner, including installation and CI installation.Validation and review
Full review path: two rounds and the KEEP architecture ruling are complete. The final independent mechanical-merge review found no proven P0/P1/P2 issue on
69ed57f086978449efbf4873bf84d712bbc13e6c, including main89e93cfa387188ac63b0ec711da4bf02873225bf. Incoming non-PR source and lockfiles match main; no dependency fix is authored here.Current-head full PR workflow passed all 23 executed jobs with seven scoped skips. The complete check snapshot has 51 successes and 11 policy skips, including successful dependency security and WSL checks. Its synthetic tree equals the pushed head. CI owns full lint/typecheck. Focused tests cover missing/stale output, unchanged modification times, invalid-input publication, command arguments/working directories and failure propagation.
The successful packaged QA ran on
1adf535936e18c23d6bcf50d937831de0623d80dagainst main54acda1d127b076abd00cafe86e361051f5ff7af: both normal unsigned arm64 packages passed startup, four byte-equal shipped guide commands, authenticated schema-validated RPC, normal Terminal 1 isolation, rendered Settings and one new real native Codex reply each. The catalog matched pinned main after only the approved header and formatter normalization. All 788 sanitized evidence records were independently verified, package inventories stayed unchanged, and recorded process/listener cleanup passed.Only five mechanical main merges followed that QA head. The October 23 ruling carries this evidence forward; current-head CI and independent merge review cover integration, with no merge-broken flow found. The later collector failure was a rig bug; it does not invalidate the earlier successful run. A redundant October 23 build attempt was stopped by the lane owner before any app or driver launched. The single review comment names all five merges and retains the 18 original whole-renderer screenshots.
Scope and observations
Both QA refs lose the displayed Low label after replying without changing controls; effective effort and cause are unverified. Main alone logged an at-rest slash-discovery SQLite warning while its app and successful provider were alive; slash functionality was not tested. The branch’s optional post-driver backend snapshot failed in its collector; earlier live isolation and named closure were independently verified. A preliminary package containing QA scratch was excluded, and the complete assigned flow passed on the corrected normal package.
Generation work was source-reviewed, not benchmarked. Unreproduced external Windows sharing, abrupt-temp cleanup and cancellation concerns remain deferred after bounded traces. Rendered QA is macOS only; cross-platform and mixed-version contracts passed CI. This PR changes no agent/native-chat implementation. No all-provider, resume/upgrade, phone/remote UI, signed installation or updater coverage is claimed.
Checklist
Agent skill upstream boundary