Conversation
Proposes evaluating SkillAdam (ruc-datalab/SkillAdam, MIT, (C) 2026 Tencent)
as an out-of-tree optimizer for this repo's SKILL.md files, with the
accept/reject authority kept in our own dataset harness.
Records four facts the design turns on, all verified against this tree:
- only `description` is guaranteed to reach a model. The dispatch context
(knowledge.py:120 -> dispatch.py:70) sends {name, description}; the body
reaches a model only when it calls skill_search, truncated at 1500 chars
(knowledge.py:163).
- the three skills whose bodies exceed that cap are exactly the three the
eval dataset does not measure -- they describe maintaining this copilot's
own source, not vllm-omni review.
- skills/pr-review-breaking-changes/SKILL.md cites ground truth from held-out
dataset items: #4810 (pr_review/val), #4834 and #4849 (pr_review/test),
#4891 and #4905 (issue_answer/val). Pre-existing, not introduced here, and
it taints every val/test number produced by an arm carrying that skill. A
scan of the other six skills found only model-adaptation-review -> #5003,
an issue_answer/train item, which is legitimate. The trial target therefore
moves to model-adaptation-review, and repairing the leak gets its own issue
-- the dataset README requires retiring and redrawing leaked items, not
editing them in place.
- the gate is eval/dataset/run_copilot_arm.py (generation, ARM_OUT isolates
configurations) plus judge_val.py (blind pairwise, claude-sonnet-5 as a
third model). eval/run_eval_v3.py and run_replicates.sh are the legacy
three-PR benchmark over cached reviews and know nothing about the dataset
splits. judge_val.py's REPLICATES re-judges the SAME saved outputs, so it
measures judge noise only; generation variance needs independent
run_copilot_arm.py passes into distinct ARM_OUT directories, and the RFC
specifies three per configuration. ARM_B_DIR defaults to the historical
Opus 4.8 baseline and must be set explicitly.
Also records the prior from eval/dataset/judgments/T3_FORENSICS.md -- ~90% of
judge penalties were mechanical delivery problems, not weak analysis -- as a
reason to keep the trial bounded.
No code, dependency, or fork is proposed here. The first implementation step
is a default-off `skills_body_chars` knob measured as an ablation arm, in the
idiom config.py already uses for every other capacity constant.
Doc-only change. Validators run: check_doc_links, check_doc_citations,
check_spec_freshness --strict, all green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011TVowBZsVPW1cv6Hbsi4iU
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
1 participant
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.
RFC only — no code, no dependency, no fork. It asks for agreement on a sequence before PR #1.
What it proposes
Evaluating SkillAdam (MIT, © 2026 Tencent) as an out-of-tree optimizer for this repo's
SKILL.mdfiles. It runs in a scratch tree, proposes candidate text, and never writes an active skill or enterssrc/infermatrix_copilot/. The accept/reject authority stays ineval/dataset/.Sequence: a default-off injection knob (PR #1) → an ablation arm → the trial → a skill-promotion PR if a candidate survives val.
Four findings recorded while scoping it
All verified against this tree; the first three were surprises.
1. Only
descriptionis guaranteed to reach a model. The dispatch context (knowledge.py:120→dispatch.py:70) sends{name, description}. The body reaches a model only if it callsskill_search, truncated at 1500 chars (knowledge.py:163). Any run that scores a wholeSKILL.mdis scoring text this runtime may never send.2. The three skills that overflow that cap are the three the eval cannot see.
code-quality-review(4203),knowledge-base-contribution(3570),docstring-conventions(2551) all describe maintaining this copilot's own source, whicheval/dataset/does not measure. The three on the measured path are all under the cap.3.
pr-review-breaking-changescites held-out ground truth. Pre-existing, not introduced here:#4810 → issue #4891pr_review/val,issue_answer/val#4834 broke merge CI → #4905/#4912pr_review/test,issue_answer/valGT #4849pr_review/testA model carrying this skill has been told in prose what two test items and two val items are supposed to find, so every val/test number from an arm carrying it is contaminated to that degree. A scan of the other six skills found only
model-adaptation-review→#5003, anissue_answer/train item, which is legitimate.The trial target moved to
model-adaptation-reviewas a result. Repairing the leak needs its own issue — the dataset README requires retiring and redrawing leaked items, never editing them in place, so rewriting the skill would not restore the holdout.4.
REPLICATES=3does not cover generation variance.judge_val.pyre-judges the same saved arm outputs, andrun_copilot_arm.pyskips existing outputs on re-run. Sampling generation variance requires independent passes into distinctARM_OUTdirectories; the RFC specifies three per configuration. Related:ARM_B_DIRdefaults to the historical Opus 4.8 baseline, so leaving it unset silently judges against the wrong reference.Also recorded, as a reason to keep the trial bounded:
eval/dataset/judgments/T3_FORENSICS.mdfound ~90% of judge penalties were mechanical delivery problems, not weak analysis — so skill prose is the smaller of the two terms.Decisions the RFC records
External optimizer first, copilot capability deferred · plumbing fixed before optimizing · shipped as a default-off knob measured by an ablation arm, not a default flip · SkillAdam is a candidate generator, the dataset harness holds authority · no pre-registered threshold, so the output supports our own investment decision and is not evidence that the method works.
Checks
Doc-only change.
check_doc_links,check_doc_citations,check_spec_freshness --strictall green. Offlinepytestwas run; its 5 failures are environmental (system Python resolvesmcp2.x while the project pinsmcp<2) and unrelated to a docs diff.🤖 Generated with Claude Code
https://claude.ai/code/session_011TVowBZsVPW1cv6Hbsi4iU