Repository navigation
Conversation
…text framing Adds the controls a reviewer asked of the novelty 2x2: - added_over_placebo when the placebo arm ran - no_artifact_repeat arm (new cache draw) -> noise_floor, added_net_of_noise, degraded_within_noise - ci95_added resamples atoms (ci_cluster: atom) - --context-framing reference|neutral on novelty analyze/probe Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…bo-as-with arm, docs) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🔍 OpenCodeReview found 3 issue(s) in this PR. CI at `b3d97e2`: 3 failing, 1 waiting for approval, 8 passing, 4 skipped
By severity: 3 medium
Review details
|
| arms=_arms(args.arms), concurrency=args.concurrency, | ||
| allow_self_judge=args.allow_self_judge, seed=args.seed, | ||
| placebo_text=placebo_text, | ||
| placebo_text=placebo_text, framing=args.context_framing, |
There was a problem hiding this comment.
Must-fix — Impact: saved probe results cannot distinguish the reference and neutral framing experiments, undermining comparison of the placebo-abstention control. Trigger: run novelty probe with --context-framing neutral and save its output. Evidence: framing is passed only to measurement; build_probe_record() writes no framing/config field, and the saved runs contain questions and responses, not the context preamble. Thus neither the output nor its artifact digest identifies the chosen framing. Persist the framing in the probe record (and test both choices). Workaround: use analyze, which records neutral framing, or track probe flags externally. Severity justification: medium because this loses experiment provenance for the newly supported probe control, without affecting unrelated measurements.
| fields["added_net_of_noise"] = round(rates["added"] - floor["added"], 4) | ||
| fields["degraded_within_noise"] = rates["degraded"] <= floor["degraded"] |
There was a problem hiding this comment.
Must-fix — Impact: these derived metrics can report noise-adjusted gains or dismiss degradation using unrelated probe populations. Trigger: asymmetric full/repeat arm errors. Evidence: paired_outcomes intersects baseline with full, whereas noise_floor independently intersects baseline with repeat. For two initially wrong probes, let full answer a correctly and error on b, while repeat errors on a and answers b wrongly: this reports added_net_of_noise=1.0 despite having no successfully answered three-arm comparison. The same population mismatch affects degraded_within_noise. Compute both compared rates on the common baseline/full/repeat intersection, and omit the derived metrics when that intersection is empty; retain the standalone floor if useful. Workaround: use these derived fields only when all three arms have the same usable probes. Severity justification: medium because this is a bounded but material measurement correctness failure. Add an asymmetric-error regression test; the current tests cover only complete populations.
There was a problem hiding this comment.
Agreed, and the empty case is worse: when every full answer errors, rates are the 0.0 placeholders and the output still says added_net_of_noise: -0.1, degraded_within_noise: true (n_paired 0, floor 0.1/0.05). That is a pass for a degradation nobody measured. Computing both on the common baseline/full/repeat probes and omitting the two fields when that set is empty fixes both cases.
| for atom in per_atom.values() for n in atom]), | ||
| # Atoms are resampled: the paraphrases of one atom are not independent probes. | ||
| "ci95_added": clustered_ci(added_by_atom), | ||
| "ci_cluster": "atom", |
There was a problem hiding this comment.
Must-fix — Impact: novelty analyze silently loses all new control metrics and writes atom-clustered intervals without their identifying marker. Trigger: any analyze run (including --arms no_artifact,full,placebo,no_artifact_repeat). Evidence: analyze() passes the measurement to report.build_novelty_artifact(), whose paired projection (report.py lines 100–110) copies only the old seven fields, dropping ci_cluster, added_over_placebo, noise_floor, added_net_of_noise, and degraded_within_noise. This also makes the documented rule that records without ci_cluster have probe-level intervals incorrect for new analyze outputs. Update that projection to preserve the new fields and test the persisted analyze artifact, not only paired_outcomes. Workaround: use the probe output or inspect paired_outcomes directly. Severity: medium because the advertised controls are absent from the main command's saved results, while the underlying measurement still runs.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
2 open issues
What changed in this PR
This PR adds reviewer-requested control metrics and prompt framing options to the novelty measurement pipeline, improving interpretability of the added signal by separating placebo/context effects, run-to-run noise, and correlated paraphrase uncertainty.
Changes:
- Introduces placebo gating, a baseline repeat “noise floor”, and atom-clustered bootstrap confidence intervals.
- Extends measurement/analyze/probe flows with
--context-framing reference|neutral, recording framing in run config and prompt hashing. - Updates docs/changelog and adds a focused test suite for the new controls.
| File | Description |
|---|---|
| dokimi/tests/test_novelty_controls.py | Adds tests covering placebo gating, noise floor, clustered CI, and neutral framing validation. |
| dokimi/scripts/growth_baseline.json | Updates growth baseline metadata to reflect novelty command growth from new flags/arm. |
| dokimi/dokimi/novelty/measure.py | Adds repeat arm + framing support; switches CI to atom-clustered; integrates control field reporting. |
| dokimi/dokimi/novelty/controls.py | Implements placebo-gated added, noise floor, and atom-clustered bootstrap CI helpers. |
| dokimi/dokimi/novelty/analyze.py | Records context_framing in config and passes it through to measurement. |
| dokimi/dokimi/commands/novelty.py | Adds --context-framing; adjusts cost estimation for the repeat arm. |
| dokimi/docs/skill-novelty.md | Documents placebo gating, noise floor outputs, clustered CI semantics, and framing usage. |
| dokimi/changelog.d/20261105_120000_napetrov_novelty-review-controls.md | Changelog entry for controls, clustered CI, framing, and prompt hash changes. |
Review effort: Lite
| n_arms = len(_arms(args.arms)) | ||
| estimate["expected_usd"] = _estimate_usd( | ||
| args, n_tokens, estimate["estimate"]["by_role"], settings.tier, | ||
| context_share=(n_arms - 1) / n_arms, | ||
| context_share=(n_arms - 1 - ("no_artifact_repeat" in args.arms)) / n_arms, |
| clusters = [values for values in per_atom.values() if values] | ||
| if len(clusters) < 5: | ||
| return None | ||
| rng = random.Random(f"novelty-ci-{seed}") | ||
| n = len(clusters) | ||
| means = [] | ||
| for _ in range(iterations): | ||
| picked = [clusters[rng.randrange(n)] for _ in range(n)] | ||
| total = sum(len(c) for c in picked) | ||
| means.append(sum(sum(c) for c in picked) / total) |
There was a problem hiding this comment.
Not worth the extra code: clustered_ci over 300 atoms of 3 paraphrases takes about 30 ms, and runs probe far fewer atoms.
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at b3d97e2. The controls are the right ones and the repeat arm really gets a new cache draw, but the new fields are not ready to read yet: novelty analyze drops them from its artifact, and two of them turn "not measured" into a number or a pass. CI is red for reasons from this PR; the details are in the line comments.
Agreed with @exai-bot: the analyze artifact loses the control fields and ci_cluster; novelty probe does not record the framing; the noise fields mix probe sets, and I added the empty case in its thread.
Agreed with @copilot-pull-request-reviewer on parsing --arms once; on precomputing cluster sums I disagree and replied in its thread.
What I ran, at b3d97e2:
make ci-local --offline: 15,796 passed, 3 failed, 63 skipped. The CLI snapshot, the flag registry (--context-framing) and the growth rationale fail in the tests; the strict docs build fails on the noise-floor list. All four come from this PR.- A repro of
paired_outcomeswith thefullarm all errored, and with a placebo that overlaps no other arm: the output showsdegraded_within_noise: trueandadded_over_placebo.rate: 0.0withn_paired: 0.
Checked, no issues
- Repeat arm:
draw=1reaches the default transport and so the response cache key; reversal probes andreversal_resistedstay on the with-artifact arm only. - Cost estimate: the repeat arm is priced without the artifact, which is right.
- Schema:
novelty.v1allows extra keys inpaired, so adding the control fields there needs no new version. - Framing:
referencegives the same prompt as before;neutralis inrun_configonly when set, so older reference cells keep the same config.
| { | ||
| "files": { | ||
| "dokimi/commands/novelty.py": {"lines": 1021, "rationale": "estimate-only projection and W1 routing cost output (#164) from main, plus the #172 routing field, --all-vs-all, tied/unscored caveat and blind-skill warning, --all-vs-all with --repeats (noise and cost block); routing output in commands/novelty_routing_report.py; file split tracked separately"}, | ||
| "dokimi/commands/novelty.py": {"lines": 1026, "rationale": "--context-framing and the no_artifact_repeat arm help (review controls); estimate-only projection and W1 routing cost output (#164) from main, plus the #172 routing field, --all-vs-all, tied/unscored caveat and blind-skill warning, --all-vs-all with --repeats (noise and cost block); routing output in commands/novelty_routing_report.py; file split tracked separately"}, |
There was a problem hiding this comment.
CI fails on this branch for five reasons, all from this PR.
docs/reference/cli-interface.jsonis stale: runmake cli-snapshot.test_every_spelling_on_the_tree_is_canonical_or_allowlisted:--context-framingis not incli_flags.pynor intests/data/cli_flags_allowlist.txt.test_growth_ratchet: this rationale is over 300 characters.- strict mkdocs: the nested list under Noise floor in
skill-novelty.mdneeds 4-space indent and a blank line before it. controls.pyandtest_novelty_controls.pyhave no SPDX header.
| return { | ||
| "n_paired": n, | ||
| "count": count, | ||
| "rate": round(count / n, 4) if n else 0.0, |
There was a problem hiding this comment.
added_over_placebo reports rate: 0.0 when no probe was answered in all three arms.
With full errored on the probes the placebo answered (or the reverse), paired_outcomes returns {"n_paired": 0, "count": 0, "rate": 0.0, "ci95": None}, which reads as "no gain beats the placebo". Nothing was measured. noise_floor already returns None for this case.
Suggestion: "rate": round(count / n, 4) if n else None (or omit the key), and a test with no three-arm overlap.
| A probe asks for one fact, so it cannot punish a scrambled document: expect this arm | ||
| near ``full``. The study's shuffled arm scored higher, but it injected more files | ||
| than ``full`` did, so that result does not carry over to this arm. |
There was a problem hiding this comment.
The PR retracts the shuffled-arm claim here, but users still see it elsewhere.
The order_independence gate message (gate/novelty_gate.py:635, "+0.022 (CI -0.027 to +0.072)"), the _order_independent docstring (report.py:189, "four of five models") and skill-novelty.md ("Investment in prose order") still cite it. The removed text also dropped the 71-task behavioural retest, which is a separate result.
Suggestion: say which part no longer holds, and update the gate message and the docs to match.
|
|
||
| ### Changed | ||
|
|
||
| - **`ci95_added` resamples atoms rather than probes** (`ci_cluster: "atom"`). Paraphrases of one atom are correlated, so the old interval was too narrow. |
There was a problem hiding this comment.
The clustered interval needs 5 atoms where the old one needed 5 probes, and the fragment does not say so.
A run of 4 atoms with 3 paraphrases each used to get ci95_added; now it gets null. The new intervals are also wider, so ci95_added from older cells is not comparable with new ones.
Suggestion: add both to this entry, so a reader knows what to re-run.
| framing is recorded in the run config and is covered by `prompts_sha256`. | ||
|
|
||
| ```bash | ||
| python cli.py novelty analyze --artifact skill:s --model M --arms no_artifact,full,placebo,no_artifact_repeat --placebo-text other-domain.md --context-framing neutral |
There was a problem hiding this comment.
Nit: The line breaks of this command were lost (--model M --arms ... --context-framing). Put it on separate lines with \ continuations, like the other examples.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Not ready to merge. The controls are the right ones, but the new fields are not yet safe to read, and CI is red for reasons from this PR. Two test gaps are inline.
I agree with @vlad-perevezentsev's points on the novelty analyze artifact dropping ci_cluster and the control fields (report.py:100-110), the mixed probe populations in the noise fields, added_over_placebo reporting rate: 0.0 for n_paired: 0, the shuffled-arm claim still cited in the gate message and docs, the changelog's missing 5-atom minimum, and the broken example line in the docs.
@vlad-perevezentsev's points on the CI failures hold at this head. The CLI snapshot is stale, --context-framing is neither in the flag registry nor the allowlist, the growth rationale in growth_baseline.json exceeds 300 characters, the strict docs build fails on the noise-floor list, and controls.py and test_novelty_controls.py have no SPDX header.
@napetrov's point that d1a87df and b3d97e2 have no Signed-off-by waits for the author; the DCO check stays red until then.
What I ran, on a Linux review host at b3d97e2:
make ci-local: 27 passed, 3 failed, 7 skipped. The failures are the CLI snapshot, the strict docs build andRun tests. In the test run, 15,792 passed. The failures from this PR are the snapshot test, the flag registry test and the growth ratchet. Two audit tests timed out and one hook test took 2.0 s against a 1 s limit; none of them touches the files this PR changes.- A mutation run over
controls.py,measure.pyandanalyze.py: 30 mutants, 15 killed by the new tests alone and 16 with the neighbouring novelty tests. Survivors are in the inline comments.
Open bot points I confirm:
- I agree with exai-bot that
novelty analyzedrops the new control fields andci_clusterfrom the saved artifact. New evidence:report.py:100-110copies onlyadded,redundant,degraded,still_missing,n_paired,n_unpaired_droppedandci95_added. - I agree with exai-bot that
noise_floorandpaired_outcomesuse different probe populations, soadded_net_of_noiseanddegraded_within_noisecan come from unrelated probes. Both should use the common baseline, full and repeat set and be omitted when it is empty. This is from readingcontrols.py; I did not rerun the empty-set repro. - I agree with exai-bot that
novelty probedoes not record--context-framing, so a saved probe result cannot say which framing produced it.
Checked, no issues
- Repeat arm:
draw=1reachesLLMClient.text, the transport and the cache key. Draw 0 leaves the key unchanged, so old cache entries still hit. - Reference framing: the
Reference material:prompt is byte-identical to before, andrun_configgains a key only forneutral. - Old bootstrap:
_bootstrap_cihas no remaining callers, andrandomis still used byshuffle_text.
Bot points I checked that no longer apply at this head (2)
- Copilot's point on the
args.armssubstring test does not apply:no_artifact_repeatis not contained in any other arm name, so the test gives the same answer as list membership. - Copilot's point on precomputing cluster sums does not apply:
clustered_ciover 300 atoms of 3 paraphrases takes about 30 ms, and the result would be identical.
| answerer=answerer, judge=judge, arms=config.arms, | ||
| concurrency=config.concurrency, allow_self_judge=config.allow_self_judge, | ||
| placebo_text=config.placebo_text, seed=config.seed, | ||
| placebo_text=config.placebo_text, seed=config.seed, framing=config.context_framing, |
There was a problem hiding this comment.
No test pins that analyze passes the framing to the measurement or records it in the run config.
Mutation check: removing , framing=config.context_framing here, and separately changing framing = {} if config.context_framing == "reference" else {...} (analyze.py:85) to if True, both pass the new tests and the 357 neighbouring novelty tests (analyze, measure, cli, report, gate). With the first mutant, novelty analyze --context-framing neutral silently runs the reference prompt. With the second, cells with different framings look comparable. The flag exists only for the placebo-abstention control.
Suggestion: add a test that runs analyze with context_framing="neutral" and a recording transport, asserts the prompt contains the neutral preamble, and asserts run_config holds context_framing: "neutral" while the default run has no such key.
|
|
||
| def _usable(runs, arm: str) -> dict: | ||
| return {_key(r): r for r in runs | ||
| if r.arm == arm and r.probe_kind != "reversal" and not r.errored} |
There was a problem hiding this comment.
The guards and the interval bounds that decide reported measurements are untested.
Mutation check, each passing the full new test file and the neighbouring novelty tests:
_usableno longer excludingerroredruns (controls.py:24): errored placebo and repeat answers then count as wrong and inflateadded_over_placebo.countandnoise_floor.added.- Dropping the
with_arm == PLACEBO_ARMguard (controls.py:58). - Flipping
degraded <= floorindegraded_within_noise: the test data hasdegraded == floor degraded, so only equality is pinned. - Swapping the floor's
ci95_addedflips to right-then-wrong. - Changing the
< 5cluster threshold to< 8, the lo/hi percentile indices, the seed, or the cluster weighting. The tests only check "wider than flat" andNonefor very few clusters.
Suggestion: add cases with an errored probe in the placebo and repeat arms, a case with degraded strictly below the floor, and one exact ci95 for a fixed small input, plus a 5-cluster case that must return an interval.
novelty: placebo-gated added, noise floor, atom-clustered CI, context framing
Problem: a review of the novelty study asked four things the 2x2 could not answer: is "added" above the placebo, above run-to-run noise, how wide is the CI when paraphrases of one atom are correlated, and is placebo abstention an artefact of the "Reference material" wording.
Change:
Evidence: tests/test_novelty_controls.py; reviewer rerun numbers in docs.
Local review: two self-review passes (design lens, finder and verifier with probes); the blocking parity finding is fixed. mypy, bandit and zizmor did not run locally, and some Windows-only test failures are the same on main.
🤖 Generated with Claude Code