Repository navigation
Add the improve skill and split oversized files - #601
Merged
Merged
Conversation
jserv
added
the
explained
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
jserv
force-pushed
the
rust-refactor
branch
2 times, most recently
from
November 6, 2026 18:25
6651a9a to
ec00523
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The size ceilings say which Rust files are over 1500 lines but not which to split first. A big file nobody touches costs nothing; one every change goes through costs on each change. scripts/ceiling-hotspots.py prints, per directory, each file's lines, ceiling, commits in a window and their product. --under ranks tracked files that have no ceiling, such as web. Lines are counted as check-rust-style.py counts them, and history is read with --full-history and NUL-separated names so reverted branches and non-ASCII names are not lost. Change-Id: I592debfa709204383036bd9843aa32b779ce27b9
The name "change" says only that code moved. The skill is the order of work that leaves quality higher than it found it, and it now takes in the Rust rules, so the name should say what the work is for. The skill directory is renamed and the Rust skill becomes rust.md beside it, read when Rust files or the style checker are touched. References to either name are updated. The description names the kinds of change and carries the Rust triggers as plain YAML, since a colon and space or a space before a hash makes a loader fail or cut every trigger after it. Routing for the skill and its neighbours still passes. Change-Id: Id576dced221aa38d1ddb4eb7c2c5835598196ddd
Refactoring several functions, splitting a file or working through a list of findings kept going wrong the same ways: a fix changed what the next one measured, a rename rode in with a logic change, and a smaller total hid a replaced finding. The skill had one line on it. It now has a section: scope as an audit or a fix, test current behavior first, diagnose with the checks that exist, classify, record a baseline, batch one intent at a time, report each claim with a label, and stop when findings grow or a contract must change. The description names restructuring, splitting and new features, and eight routing queries pin it. Change-Id: I9eaa2cba7c431876eae5e1109f72304e26b185bb
A skill is routed from its description alone, and the loader reads an unquoted one as plain YAML. A space before a hash starts a comment and drops the rest, which is how the review skill reached the router as "... re-review PR". A colon and a space is a YAML error in the same place. The checker stripped the comment and asked only that something was left. The review description now puts the pull request number in backticks, the gate, review and browser skills each point at codetrial-improve, and check-skill-refs.py reports both shapes in an unquoted description while leaving quoted, folded and literal ones alone. Change-Id: I3a4fa8f03b034acff8ce58cd6b43cfc1b2ae275f
The tree recovers a poisoned Mutex in 42 places over 14 locks with nothing saying why. Reading each critical section shows all 14 guard state that is written whole, and Slot::drop and Blocked::drop take their locks while a task unwinds, where a panic would abort the process. Five comments sit at the declarations whose reason is not obvious, and rust.md states the rule once for the rest and for any new lock. The Gemini socket fields are covered by the rule only: a comment there would push src/gemini.rs past its size ceiling. Change-Id: I3af942bf1291706b78cd661e37efc1a0f527711f
dispatch_or_release_consent took Option<&String>, which forces a caller holding a &str to allocate. It takes Option<&str> now, and its one caller passes as_deref(). recipes/rust.md records this and the sites that stay as they are: pending_candidate_line, because the session code needs a &String and a str would add an allocation, and two closures that call as_deref first. Web tests that dispatch with an interview id run the changed code. Change-Id: Ied566ca871f2162517a901e90cb02e7fc33d2d6d
The skill had a Rust profile and a sentence saying JavaScript had none, yet the browser half is 60 files and four of them are over 1500 lines. javascript.md covers the gates, how a new file reaches the browser, what catches a missing import, the file and function sizes, and where to start: web/task.js, with few commits, no importers and the least coverage. It also says why web/ has no size ceiling: the baseline check refuses a file that gains an entry. SKILL.md points at it and one routing query pins it. Change-Id: I8554e75919a79f4c06f515c4c76bef400d275ca9
src/agent/prompts.rs held every string the model is shown in 3944 lines, the most edited file with no open pull request touching it. It is now a 70-line facade and six modules under src/agent/prompts/: material, instructions, progress, nudges, recovery and assessment. The facade re-exports the same names, so no caller changes. The text is unchanged apart from whitespace and visibility, and the golden prompts and the Rust lane pass. The report evaluation hashed the old file for its identity, so it now hashes all seven. rust.md gains a Splitting a file section with the order of work, so the next split does not rediscover it. Change-Id: I3e5fad8f4af8d2c17dc5eaa4179ba0dee9879eb5
The skill draws on seven tools under externals/ that nobody can open from a fresh clone, so what each taught it, and what was left out and why, has to be in the skill. SKILL.md gains rows for fastmod, reformat, repo-audit and splice; rust.md extends the style-audit row and adds splitrs and refactoring-rust. Each cell says only what the skill contains. No tracked file links externals/, and the reference checker passes with it moved aside. Change-Id: I8a1a5c720c4fc8a1cd38283164b9e8a6f36f28fe
web/task.js cannot be imported by a node test, so none of its logic had a unit test and the only coverage was a browser suite that skips without Chromium. Three blocks of it are pure. web/task-calibration.js holds the calibration line, its checks and the message for missing face files. web/task-cases.js holds the text of a run's cases and takes the required cases as an argument. web/task-review.js holds the result wording, as a generator so a malformed assessment still throws after the head was shown. Each test was written first. The page suite did not pin the calibration line, the face message or the unavailable sentences, so the tests are the only guard for those. Change-Id: Ia913f971dc444909336d338d2a117f7213d68a2e
web/task.js kept 45 mutable variables at module level, so code could not leave it: a function in another module cannot reach a variable in this one. They are now fields of a sealed object, st, in web/task-state.js, and every reference reads or writes st.name. A misspelled name throws on a write instead of creating a field nobody reads. A script did the rewrite, resolving each reference with eslint-scope so a local with the same name is untouched. Three tests pin the seal and the defaults. Change-Id: Icbf31889d5f161a5aec5326cf63c8a698ca3e450
Joining the room, what follows when it ends, and what pressing Start does are about 300 lines of async flow in web/task.js that touch 14 variables and 32 functions, and no node test could reach them. They move to web/task-room.js and web/task-start.js, with $ and id in web/task-dom.js and three predicates in task-state.js. task-state.js also gains host and lend: task.js lends what it still owns by name, as getters read at the call, so a split-out module never imports task.js, which cannot be imported without the page. Twenty-two node tests give the code a stubbed document and a fake host. Change-Id: Iccc7551d7864a6efe67ee2f56adf75daa55fecbb
src/livekit.rs, src/agent.rs and src/gemini.rs were 3572, 3520 and 2815 lines, the most edited files in src/. Parts that need only the state they sit beside move by line span into modules: livekit gains side_calls, candidate, open, restart and replacement; agent gains framework, phase_judge, code_work and transcript; gemini gains report_calls and http_calls. The text is unchanged apart from whitespace and visibility, and each file re-exports the same names, so no caller changes. Six tests read livekit source as text and the report evaluation hashes gemini sources for identity, so both follow the code. The ceilings fall to 1935, 2004 and 1870, and rust.md records what the splits turned up. The mutation lane judges moved lines as changed, and found the candidate and duplicate-agent checks and the owed-reply hold untested. Those rules now take plain facts and have tests; the wrappers over an SDK participant stay excluded in mutants.toml, each with its reason. Change-Id: Iaeb565ca193ff253e7f0ffe54f9fafca292cb8d0
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
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.
Problem and behavior
Four files that most changes go through were far over the 1500-line limit (src/agent/prompts.rs 3944, src/livekit.rs 3572, src/agent.rs 3520, src/gemini.rs 2815 lines) and web/task.js had no unit-testable logic. The change-and-Rust skills also overlapped and had no method for refactors. After this change those Rust files are 71, 1935, 2004 and 1870 lines, web/task.js is 1550 lines with seven node-testable modules, and one skill, codetrial-improve, carries the method and the Rust and JavaScript profiles. No behavior changes.
Approach
Rust: whole items move by line span into sibling modules and the old file keeps re-exporting the same names, so no caller changes; only visibility (to pub(super)), imports and the ceilings in scripts/rust-style-ceilings.json change. Tests that read source as text and the report evaluation's source digest follow the moved code. Browser: the 45 mutable module-level variables of web/task.js become one sealed state object, and the pure text, room connection and Start handler move to modules that reach what task.js still owns through a lazy host seam, so none imports the page. Skills: codetrial-change and codetrial-rust merge into codetrial-improve with a refactor method, a ceiling-hotspot ranking (scripts/ceiling-hotspots.py), and a check that refuses a skill description a YAML loader would cut. The series is 13 commits, one intent each.
Summary by cubic
Splits the oversized Rust and browser files into smaller, testable modules and merges the change and Rust guidance into
codetrial-improve. The refactors preserve behavior: the Rust files re-export the same names, so no caller changes.web/task.jsis split into seven Node-testable modules sharing one sealed state object.scripts/ceiling-hotspots.pyto rank oversized files by churn times size, plus Node tests for the extracted browser logic.codetrial-improvecarries the refactor method, Rust and JavaScript profiles, and reference-tool notes; routing and skill references are updated, and unquoted descriptions are checked for YAML hazards.Option<&str>.-D warnings; Playwright, the desktop build and cargo-deny were not run.Written for commit 65d44db. Summary will update on new commits.