SLM-317: do-no-harm AR→repair hybrid + advancement screen - #852
SLM-317: do-no-harm AR→repair hybrid + advancement screen#852Tyler-R-Kendrick wants to merge 9 commits into
Conversation
- coverage_class per frozen row (OBSERVABLE_PROMPT / OBSERVABLE_REQUEST_ONLY / UNKNOWN); all 51 rows are OBSERVABLE_REQUEST_ONLY pre-inventory. - Matched requests differ only by the production ensure_prompt_inventory suffix; request-derived, no hidden gold channel. - X22 deterministic arm: paired v2-strict delta +0.3125 (5/16 vs 0/16, Wilson [0.142, 0.556]) exceeds the predeclared 0.10 minimum; no regressions on observable rows. - AR tiny baseline: 6/32 decodes completed (all v2-fail partials), 26 decode_timeout (600-step checkpoint constrained-forest cost per SLM-294 evidence); reported unmeasured, not evidence. Rico rows not_run with SLM-294 cost evidence. - 100-record blind slot-observability audit: raw agreement 97%, kappa 0.0 (skew caveat); finding: v2 coverage detector misses inline slot enumeration -> false prompt_contract_unknown (append-only; no metric change). - 8 harness tests; new component harness.experiments.slm301_prompt_observability v1.
…ints - decode_outcome.py taxonomy with strict precedence (fallback never counts as model success); per-request budget/elapsed/forwards/ verifier/fallback/stop-reason records. - eval_runner additive integration: details[] gain decode_outcome, stop_reason, fallback_used; suite metrics gain decode_outcome_counts. - Census: only slm230 committed-SHA verifiable; hash-pinned the two remediated nonzero-timeout checkpoints; scoreboard classes separated (runtime_timeout/fallback/unmeasured/model_behavior). - Preregistered 1x vs 10x budget sweep: all 12 cells not_rerunnable (v0 checkpoints fail current output-contract check); the 10x-flip question is honestly UNANSWERED by re-decode. Recommendation: retrain the remediated recipe on contract v2, then re-eval at both budgets. - 25 new tests; harness.model_build.eval v50; component harness.experiments.slm303_decode_budget_audit v1; gates.ship v3 threshold mirror fixed.
- slm308_distance_oracle.py: bounded reverse-BFS over canonical AST fingerprints via the real extended 11-action transitions; EXACT / BOUNDED / UNKNOWN labels (budget never conflated with distance); cache keyed by action-schema version, grammar sha, inventory, target and state hashes. - tree_edit_diffusion: value_label_mode (bounded_distance default for new configs; pre-field checkpoints inject mutation_count for behavior parity, format stays 2); normalized oracle cost-to-go with UNKNOWN masked out of MSE; pairwise progress margin loss (independently tested); decode paths never read gold distance (audit test). - Matched fixture experiment (mutation_count vs bounded_distance, identical budgets, near-gold + seed-trajectory states): rank corr 0.433->0.505 (+0.072 < 0.10), beam regret 0.889->0.889 (+0.0 < 0.05), Brier 0.031->0.004, UNKNOWN coverage 0.175. Preregistered thresholds written before results; verdict honestly rejected at this budget. - 16 new tests; slm308 component v1; slm299 component v3.
- Proposal instrumentation: per enumerated candidate — action, factor score, applicability, rejection reason (23 machine-readable codes), budget consumption; deterministic order; additive evidence keys. - Distribution audit: training-target vs decode-demand action distributions by source/suite; dead-candidate rate ~0.98, applicable-ADD recall 0.10 baseline; preregistered reweighting rule. - corruption_action_distribution knob (gold corpus untouched; default off = historical uniform; parity tested) and stop_slot_accounting legacy|corrected arm (STOP consumes an expansion slot only when its frozen candidate is retained; deterministic, regression-tested). - Matched 2x2 (ADD-balanced x STOP arms, isolated levers): ADD target share 0.177 -> 0.328 (T1 +0.150 >= 0.10 both STOP arms), recall 0.10 -> 0.16, corrected STOP budget <= legacy; verdicts adopted per preregistered rules; loss reweighting deferred to preserve isolation. - 10 new tests; slm299 component v4; slm310 component v1.
- State-source harness: gold_only (existing corruption chain), seedward
(offline oracle-guided walk from the seed toward gold, strictly
distance-decreasing valid intermediates), on_policy (immutable
content-addressed beam-trajectory snapshots: wrong states, verifier
failures, abstentions); explicit provenance + gold-visibility policy
per row; sha256 rows + tamper-evident manifest; fail-closed leakage
guards (train vs held-out AST fingerprints).
- Matched {gold_only, seedward, on_policy, mixed} arms with predeclared
weights/caps, identical model/steps/optimizer/seeds; evaluated on
held-out seed trajectories. Verdict: rejected per preregistered
primary gate (beam regret improvement 0.0 < 0.05); secondary signal
(value rank corr -0.258 -> +0.258 for non-gold arms) recorded as
wiring evidence only.
- 9 new tests; slm312 component v1.
…training
- Frozen multi-mode dataset (8 prompts x 2 verifier-accepted modes;
canonical alpha-invariant AST fingerprints, sha256 rows + tamper-evident
manifest; prompt count declared below ship-gate scale).
- WTA/MCL loss: backpropagate only the min-loss mode per example +
preregistered floor term (eps=0.1) on losing modes; durable per-example
selection telemetry. No set-FTPO machinery.
- Two-mode synthetic fixture proves single-gold CE collapses
(p(mode B)=0.00004) while WTA retains both modes (p(B)=0.0083,
p(A)=0.826 dominant); multi-gold spreads 50% mass to invalid hybrids.
- Matched arms {single_gold, multi_gold, wta} identical budgets: coverage
0.750/0.500/0.500, hard-valid decode 1.0, v2 regression 0.000.
Verdict rejected per preregistered coverage gate (WTA gain -0.25);
at this scale single-gold lifts the sibling mode too.
- 16 new tests; slm314 component v1.
- Metamorphic invariance (alpha-rename, statement reorder, formatting,
AST roundtrip) tested before model evaluation.
- Hard do-no-harm commit rule: COMMIT on deterministic hard-evidence
improvement or calibrated soft improvement without hard regression;
RETAIN on regression/no-improvement; ABSTAIN on missing candidate;
source + candidate + exact reason durable per example; oracle commit
selector as non-promotable upper bound.
- Matched arms {ar_only, repair_only, ar_repair_historical,
ar_repair_improved, oracle_commit} on the frozen SLM-155 decision
corpus with paired per-example outcomes and explicit
invalid-over-valid counts per arm.
- Gates: Safety PASS (iov=0), Value FAIL (0/16 improvements, Wilson
[0.0, 0.194] vs 0.05), Reachability PASS. Disposition: inconclusive —
LAR3 remains open but NOT advanced; powered rerun required.
- 20 new tests; slm317 component v1.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds optional LangSmith/OTLP telemetry, an extended tree-edit language with checkpoint migration, decode-outcome taxonomy, and four experiment harnesses covering reachability, distance supervision, action alignment, state sources, multimode training, and repair decisions. ChangesObservability and evaluation
Extended tree-edit platform
Experiment harnesses
Design artifacts and registry
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
scripts/run_slm299_reachability_audit.py (1)
397-414: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winApply the canonical run cap to comparison runs.
--compareperforms a second audit, but this entry point has noMAX_RUN_MINUTES-derived deadline or interrupted-run handling. Add the standard levers-based cap and ensure timed-out/interrupted output is not treated as evidence. As per coding guidelines, “Every train, eval, benchmark, profile, telemetry, matrix, reproduction, or supporting shell command must obey the canonical run cap insrc/slm_training/levers.py.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/run_slm299_reachability_audit.py` around lines 397 - 414, The compare execution in the script’s main argument/report flow must honor the canonical run cap from src/slm_training/levers.py. Add the standard deadline/timeout and interruption handling around build_report (including the second audit), and ensure timed-out or interrupted runs are marked incomplete and excluded from evidence/output rather than reported as valid results.Source: Coding guidelines
src/slm_training/models/tree_edit_diffusion.py (1)
1429-1512: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftCandidate enumeration is now multiplicative per state and runs on every decode step.
INSERT_SUBTREEalone isn_stmts × n_comp × n_slots × |leaf_comps| × |CONTAINER_RESTS|, and the whole list is materialized and fully sorted each step even though onlyexpand_per_statecandidates are consumed. With realistic component counts andMAX_SLOTSthis becomes tens of thousands ofEditobjects and float conversions per beam row per step. Consider gating the subtree/statement families behind the action log-prob (skip families whose action score can't reach the current top-k bound) or usingheapq.nlargest(expand_per_state, ...)instead of a full sort.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/slm_training/models/tree_edit_diffusion.py` around lines 1429 - 1512, Reduce candidate-enumeration cost in the decode expansion around the scored list: avoid materializing and fully sorting all INSERT_SUBTREE and statement-family combinations when only expand_per_state candidates are consumed. Use action-score/top-k bounds to skip families that cannot qualify, and select the final candidates with a bounded top-k approach such as heap-based selection rather than a full sort. Preserve candidate ordering and existing Edit coverage for candidates that can reach the bound.src/slm_training/harnesses/model_build/ship_gates.py (1)
67-99: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winVersion bump to
v3is negated by_meaningful_metric_policy's stale hardcoded default.
_meaningful_metric_policy()still haspolicy_id = "openui_ship_gates_v2"hardcoded (line 86), and that value — notMEANINGFUL_METRIC_POLICY["threshold_version"]— is what ends up in the returned"threshold_version"whenevercustom=False(the common path). The bump on line 69 tov3therefore has no effect on this function's output. Based on versions.json's history forgates.ship, previous bumps (v1→v2) kept this hardcoded default in sync — this one didn't.🐛 Proposed fix
def _meaningful_metric_policy( policy: dict[str, dict[str, float]], *, custom: bool ) -> dict[str, Any]: - policy_id = "openui_ship_gates_v2" + policy_id = MEANINGFUL_METRIC_POLICY["threshold_version"] source = "DEFAULT_SHIP_GATES"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/slm_training/harnesses/model_build/ship_gates.py` around lines 67 - 99, Update _meaningful_metric_policy so its non-custom default policy_id reuses MEANINGFUL_METRIC_POLICY["threshold_version"] instead of the stale hardcoded v2 value, ensuring the returned threshold_version reflects the configured v3 bump while preserving custom policy IDs.
🟡 Minor comments (9)
tests/test_harnesses/experiments/test_slm308_distance_value.py-253-256 (1)
253-256: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
metrics["value_bounded"] + 1 >= 1is always true.The comment claims at least one oracle-labeled value and one improving pair exist, but the assertion holds for any non-negative count. Assert the intended property directly.
🐛 Suggested assertion
- assert metrics["value_bounded"] + 1 >= 1 - assert "pairwise_progress" in metrics + assert metrics["value_unknown_excluded"] < metrics.get("skipped", 0) + len(_RECORDS) + assert "value" in metrics # at least one row carried an oracle value target + assert "pairwise_progress" in metrics(Adjust to whatever invariant the fixture genuinely guarantees — the current form asserts nothing.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_harnesses/experiments/test_slm308_distance_value.py` around lines 253 - 256, Replace the tautological assertion on metrics["value_bounded"] with a direct assertion of the fixture’s intended invariant: require at least one oracle-labeled value, using the appropriate metric count and preserving the existing pairwise_progress presence check.src/slm_training/harnesses/experiments/slm312_state_sources.py-666-669 (1)
666-669: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSoften the "strictly distance-decreasing by construction" claim.
oracle_best_childskips the strict-improvement check when the parent's own distance is UNKNOWN (d_current is None, Line 572), which is exactly the case for the bare seed at these shallow budgets — the repo's own test notes this. So the first seedward step is not a proven improvement, and this docstring overstates the guarantee.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/slm_training/harnesses/experiments/slm312_state_sources.py` around lines 666 - 669, Soften the docstring for the seedward source to avoid claiming that every step is strictly distance-decreasing by construction. Describe the edits as oracle-guided legal actions toward each gold target, while preserving the existing statement that valid intermediate states are emitted and the seed-side distribution is covered.docs/design/iter-slm305-edit-language-20260724.md-1-1 (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winHeading identifies SLM-299 while the file is named for SLM-305.
iter-slm305-edit-language-20260724.mdopens as "SLM-299 (LAR1-03): X22 edit-space reachability audit". Since the doc also carries the SLM-305 old-vs-extended comparison, retitle it to name both issues (e.g. "SLM-305 extended edit language — SLM-299 reachability audit") so lookups by issue id land on the right artifact.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/iter-slm305-edit-language-20260724.md` at line 1, Update the document’s opening heading to identify both SLM-305 and SLM-299, while preserving the existing extended edit-language and X22 reachability-audit context so searches for either issue locate the correct artifact.scripts/run_slm312_state_sources.py-201-206 (1)
201-206: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winEmpty
arm_rowscrashes withValueErrorfromrandrange(0).
build_arm_rowsreturns[]when a source pool is empty (e.g.acquire_on_policyyields nothing because every held-out decode returned an emptyfinal_text), andorder.randrange(len(arm_rows))then raises. Fail fast with a clear message instead, so an acquisition gap is reported rather than surfacing as an opaque stdlib error.🐛 Proposed guard
model.train() + if not arm_rows: + raise ValueError( + "arm has no acquired rows; acquisition produced an empty pool " + "for this state source" + ) for _ in range(steps):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/run_slm312_state_sources.py` around lines 201 - 206, Add an early guard before the training loop that checks whether arm_rows is empty and raises a clear acquisition-gap error. Keep the existing sampling and optimization flow unchanged for non-empty arm_rows, covering the build_arm_rows output used by this loop.scripts/run_slm308_distance_value.py-369-390 (1)
369-390: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSTOP candidate gets a looser witness than it deserves.
witness = entry["witness"] + 1is applied to every option, including the STOP option where the "child" is the parent state itself.witness + 1is still a valid upper bound there, but it is one edit looser than the provenentry["witness"], so aBOUNDEDlabel for the STOP candidate can score farther than it is and bias the regret in favor of edit candidates.🐛 Proposed fix
for child in options: # A candidate child of a witness-backed state is one edit further from # gold along the same witness path: hi = witness + 1 is proven. - witness = ( - entry["witness"] + 1 if entry["witness"] is not None else None - ) + if entry["witness"] is None: + witness = None + else: + # STOP keeps the parent state, so its proven witness is unchanged. + witness = entry["witness"] + (1 if child is not None else 0)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/run_slm308_distance_value.py` around lines 369 - 390, Update the option loop in the regret calculation so the STOP option, identified by child being None, passes entry["witness"] unchanged to distance_to_target, while non-STOP children continue using entry["witness"] + 1 when a witness exists. Preserve the existing None behavior and effective_distance handling.src/slm_training/runtime/telemetry/trace.py-295-298 (1)
295-298: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winExit-time manifest rewrite erases
last_export_errorfrom OTLP mirror failures.
_mirror()records a failed remote export via_write_manifest(export_error=str(exc)), but__exit__now calls_write_manifest()with no argument, so the final manifest always persists"last_export_error": null. Failed OTLP exports become invisible in the committed artifact. Persist the last error on the instance instead of passing it through per call.🐛 Proposed fix
def _mirror(self, signal: str, payload: dict[str, Any]) -> None: @@ except OSError as exc: - self._write_manifest(export_error=str(exc)) + self._export_error = str(exc) + self._write_manifest() - def _write_manifest(self, *, export_error: str | None = None) -> None: + def _write_manifest(self) -> None: @@ - "last_export_error": export_error, + "last_export_error": getattr(self, "_export_error", None),Also applies to: 419-441
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/slm_training/runtime/telemetry/trace.py` around lines 295 - 298, Persist the latest OTLP mirror failure on the runtime instance rather than passing export_error only to individual _write_manifest calls. Initialize the error state, update it in _mirror when an export fails, and have _write_manifest use the stored value by default so __exit__ preserves last_export_error instead of resetting it to null.tests/test_runtime_trace.py-15-19 (1)
15-19: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFixture only neutralizes
LANGSMITH_TRACING; otherLANGSMITH_*values still leak in.
_LangSmithTrace.__init__calls_load_local_env(), which loads the repo.env, and readsLANGSMITH_PROJECT,LANGSMITH_ENDPOINT,LANGSMITH_WORKSPACE_ID. A developer with those set (env or.env) failsassert manifest["langsmith"]["project"] == "slm-training"at Line 91. Clear them and point_ENV_PATHat a nonexistent file by default.💚 Proposed fix
`@pytest.fixture`(autouse=True) -def _disable_real_langsmith_exports(monkeypatch: pytest.MonkeyPatch) -> None: +def _disable_real_langsmith_exports( + monkeypatch: pytest.MonkeyPatch, tmp_path_factory: pytest.TempPathFactory +) -> None: """Focused tests use fakes when exercising the remote publisher.""" monkeypatch.setenv("LANGSMITH_TRACING", "false") + for name in ( + "LANGSMITH_API_KEY", + "LANGSMITH_PROJECT", + "LANGSMITH_ENDPOINT", + "LANGSMITH_WORKSPACE_ID", + ): + monkeypatch.delenv(name, raising=False) + monkeypatch.setattr( + trace_module, "_ENV_PATH", tmp_path_factory.mktemp("noenv") / ".env" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_runtime_trace.py` around lines 15 - 19, Update the autouse fixture _disable_real_langsmith_exports to isolate tests from all LangSmith configuration: clear LANGSMITH_PROJECT, LANGSMITH_ENDPOINT, and LANGSMITH_WORKSPACE_ID, and redirect _LangSmithTrace._ENV_PATH to a nonexistent file before tests run. Preserve the existing LANGSMITH_TRACING=false setup so the manifest assertions use deterministic test values.scripts/run_slm314_winner_take_all.py-288-296 (1)
288-296: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winEnforce or record the canonical run cap for the SLM314 runner.
scripts/run_slm314_winner_take_all.pyrunsbuild_report()end-to-end with nosrc/slm_training/levers.pycap (MAX_RUN_MINUTES=3), interrupt signal (INTERRUPT_AFTER_SECONDS), or kill-grace handling. Add derived budget usage in this CLI or wire it through the documented wrapper so this eval/training command complies with the repository run policy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/run_slm314_winner_take_all.py` around lines 288 - 296, Update build_report and its CLI execution path to enforce or record the repository’s canonical run budget, including the MAX_RUN_MINUTES=3 cap, INTERRUPT_AFTER_SECONDS handling, and kill-grace behavior. Reuse the documented wrapper or existing policy symbols from src/slm_training/levers.py rather than implementing a separate budget model, and ensure the end-to-end runner reports derived budget usage.Source: Coding guidelines
src/slm_training/harnesses/experiments/slm317_repair_hybrid.py-208-211 (1)
208-211: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
float(None)crash risk iflearned_marginis explicitlyNone.
evidence.get("learned_margin", 0.0)only falls back to0.0when the key is absent; if a future caller passeslearned_margin=Noneexplicitly,float(None)raisesTypeError. No current caller hits this, but it's cheap to close.🛡️ Proposed fix
- margin = float(evidence.get("learned_margin", 0.0)) + raw_margin = evidence.get("learned_margin") + margin = float(raw_margin) if raw_margin is not None else 0.0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/slm_training/harnesses/experiments/slm317_repair_hybrid.py` around lines 208 - 211, Update the learned margin initialization near soft_s, soft_c, and margin so an explicitly provided None is treated as 0.0 before converting to float. Preserve conversion of valid numeric values and the existing default for missing learned_margin entries.
🧹 Nitpick comments (14)
tests/test_harnesses/experiments/test_slm308_distance_value.py (1)
344-344: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead assignment:
mdis overwritten at line 356 before use.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_harnesses/experiments/test_slm308_distance_value.py` at line 344, Remove the unused `md` assignment in the test flow around `render_markdown(first)`, since the variable is overwritten before any read. Preserve the later assignment and its existing behavior.src/slm_training/models/tree_edit_diffusion.py (1)
1349-1368: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Edit.target/Edit.payloadare enumerated but never supervised.The
comp/slotheads got the new actions, but no head predicts the container-rest index or the leaf-comp/template payload, so_enumerate_editsscores all payload/rest variants identically and their relative order is decided purely by insertion order. If that is a deliberate simplification, aponytail:note naming the limitation and upgrade path would keep the decision auditable.As per coding guidelines: "If a deliberate simplification accepts a known limitation, add a
ponytail:comment naming the limitation and its upgrade path."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/slm_training/models/tree_edit_diffusion.py` around lines 1349 - 1368, Update the loss computation around the comp/slot supervision and _enumerate_edits so Edit.target and Edit.payload receive dedicated supervision and payload/rest variants are not scored identically; otherwise add a nearby ponytail: comment explicitly documenting this deliberate limitation and naming the upgrade path for supervising container-rest indices and leaf-comp/template payloads.Source: Coding guidelines
src/slm_training/harnesses/experiments/slm299_edit_reachability.py (1)
365-407: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider validating
modein_enumerate_childrentoo (or narrowing the type).
analyze_reachabilityrejects unknown modes, but the helper silently treats any non-"v1"string as extended. ALiteral["v1", "extended"]annotation would make the contract explicit for the other callers (e.g.slm308_distance_oracle._build_target_map).Also applies to: 434-551
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/slm_training/harnesses/experiments/slm299_edit_reachability.py` around lines 365 - 407, Validate the mode argument at the start of _enumerate_children so only "v1" and "extended" are accepted, matching analyze_reachability; alternatively narrow its annotation to Literal["v1", "extended"] and enforce invalid values at runtime. Ensure unknown modes cannot silently follow the extended-action path, including callers such as _build_target_map.tests/test_models/test_tree_edit_diffusion.py (1)
133-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the coverage assertion for extended inverses.
seen_actions - {0}passes as soon as any non-STOP inverse appears (and STOP is never an inverse), so it does not actually prove the extended actions were exercised. Assert at least one of the SLM-305 inverse ids is present.♻️ Suggested assertion
- # The seeded loop exercised the extended inverse-edit supervision. - assert seen_actions - {0} + # The seeded loop exercised the extended inverse-edit supervision. + assert seen_actions & { + ACTION_ADD_CONTAINER, + ACTION_REMOVE_CONTAINER, + ACTION_INSERT_SUBTREE, + ACTION_REPLACE_SUBTREE, + ACTION_REPLACE_STATEMENT, + ACTION_BIND_PLACEHOLDER, + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_models/test_tree_edit_diffusion.py` around lines 133 - 150, The assertion in test_extended_sample_mutation_loop_restores should verify that seen_actions contains at least one SLM-305 extended inverse action ID, rather than merely any non-STOP action. Preserve the existing seeded mutation loop and use the defined SLM-305 inverse identifiers so the test specifically proves extended actions were exercised.src/slm_training/harnesses/experiments/slm310_action_alignment.py (2)
241-246: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse
zip(..., strict=True)so a records/evidence length mismatch fails loudly.Silent truncation here would quietly drop records from the audit totals.
♻️ Diff
- for record, ev in zip(records, evidence): + for record, ev in zip(records, evidence, strict=True):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/slm_training/harnesses/experiments/slm310_action_alignment.py` around lines 241 - 246, Update the iteration in the record/evidence aggregation flow to use strict zipping between records and evidence, ensuring any length mismatch raises an error instead of silently truncating audit totals. Preserve the existing processing body and aggregation behavior.
246-257: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winApplicable-ADD recall is recomputed three times per record.
_add_recallre-parses every logged state and re-runsapplicable_add_exists(a fullstmts × leaf_comps × slotssweep of parser-backedspace.apply) once foroverall, once forby_source, and once forby_suite. Compute the per-record(states_with_applicable_add, states_recalled)once and add the two counters into each bucket.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/slm_training/harnesses/experiments/slm310_action_alignment.py` around lines 246 - 257, Update the aggregation loop around _add_recall to compute each record’s applicable-ADD recall counters once, then add those two counters to overall, the source bucket, and the suite bucket. Refactor _add_recall or introduce a helper so the expensive state parsing and applicable_add_exists sweep is not repeated for each bucket, while preserving existing bucket totals.src/slm_training/models/checkpoint_migrate.py (1)
343-373: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWarm-start writes rely on
state_dict()aliasing model storage.
new_state[key][: tensor.shape[0]] = tensormutates the live parameter storage in place (state_dict()returns detached views), so the model is already modified beforeload_state_dict. It works, but aclone()of the destination rows (or building a fresh dict of clones) makes the intent explicit and keeps the function side-effect-free if the model is ever reused by the caller.♻️ Explicit clone
- new_state = model.state_dict() + new_state = {k: v.clone() for k, v in model.state_dict().items()}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/slm_training/models/checkpoint_migrate.py` around lines 343 - 373, Update the partial-copy branches in the state migration loop around policy.action_head.weight and policy.action_head.bias so assignments operate on cloned destination tensors or a freshly cloned state dictionary rather than detached views returned by model.state_dict(). Preserve the existing warm-start row-copy behavior and subsequent strict load_state_dict call while avoiding any mutation of the live model before loading.src/slm_training/harnesses/experiments/slm308_distance_oracle.py (1)
239-250: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
visited = set(result.dist)is rebuilt for every expanded node.That copy is O(|dist|) per node, so the BFS is quadratic in the number of keyed states for a pure pruning hint. Hoist it out (or pass a live view) —
result.distkeys already are the visited set.♻️ Reuse the live key set
- visited = set(result.dist) for child, _action in _enumerate_children( - space, statements, inventory, mode="extended", visited=visited + space, statements, inventory, mode="extended", + visited=result.dist.keys(), ):(
_enumerate_childrenonly needsinsupport; widen its annotation toCollection[str]if you take this.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/slm_training/harnesses/experiments/slm308_distance_oracle.py` around lines 239 - 250, Update the BFS around the expansion loop to stop rebuilding visited state for each node: reuse result.dist directly as the visited membership set, or create one shared live key view outside the loop. If passing the live view into _enumerate_children, widen its visited annotation to Collection[str] while preserving the existing duplicate-key checks and traversal behavior.scripts/run_slm310_action_alignment.py (2)
111-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFixture corpus is duplicated verbatim from
scripts/run_slm308_distance_value.py.
FIXTURE_PROGRAMSandbuild_fixture_records()are byte-identical to lines 80–104 ofscripts/run_slm308_distance_value.py;scripts/run_slm312_state_sources.pyalready importsFIXTURE_PROGRAMSfrom that module. Importing here too keeps the three matched experiments provably on the same corpus.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/run_slm310_action_alignment.py` around lines 111 - 134, The fixture corpus is duplicated in FIXTURE_PROGRAMS and build_fixture_records; remove these local definitions and import/reuse them from scripts.run_slm308_distance_value. Keep the existing record construction behavior and ensure the matched experiments share the single canonical fixture corpus.
315-322: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRetraining the baseline for the audit doubles cell-A cost.
train_cell(records, None, "legacy", steps=steps, ...)reproducescell_aexactly (same seed, same levers, deterministic batch order). Keep the cell-A model from the loop above and pass it torun_distribution_auditinstead — it removes a full training pass from the wall-clock budget and guarantees the audit is on the same weights the cell metrics describe.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/run_slm310_action_alignment.py` around lines 315 - 322, Reuse the existing cell-A model from the preceding training loop for the distribution audit instead of calling train_cell again. Update the run_distribution_audit invocation to pass that retained model, preserving the current audit parameters and ensuring the audit uses the same weights as the reported cell-A metrics.docs/design/iter-slm312-state-sources-20260724.md (1)
15-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSurface
beam_regret_n/beam_regret_excludedin the headline table.The JSON records
beam_regret_n: 1withbeam_regret_excluded: 3for every arm, so the reported0.0000regret rests on a single measurable state. Rendering those counts (viarender_markdowninscripts/run_slm312_state_sources.py) keeps the doc as self-limiting as the payload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/iter-slm312-state-sources-20260724.md` around lines 15 - 22, Update the headline results table in the design document and its generating render_markdown flow to include beam_regret_n and beam_regret_excluded for each arm. Preserve the existing beam regret values while sourcing and rendering the counts from the JSON records so the table exposes the measurement coverage.src/slm_training/resources/versions.json (1)
4538-4555: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
decode_outcome.pyisn't a watched path in any component.
src/slm_training/harnesses/model_build/decode_outcome.pyis new, imported byeval_runner.py(tracked underharness.model_build.eval), and implements the SLM-303 taxonomy — but it's absent from everypathsarray. Only its test file is tracked (underharness.experiments.slm303_decode_budget_audit). Add the source module to one of these components so future edits to the classification logic are forced through the bump/no-bump:discipline the registry's own_readmedescribes.As per coding guidelines, "Maintain component versions, watched paths, and newest-first append-only history in the version registry."
Also applies to: 4574-4595
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/slm_training/resources/versions.json` around lines 4538 - 4555, Update the version registry’s paths arrays so src/slm_training/harnesses/model_build/decode_outcome.py is watched by the appropriate component, preferably the existing harness.model_build.eval component that tracks eval_runner.py; preserve the registry’s component-version and newest-first history conventions.Source: Coding guidelines
scripts/run_agentv_eval.mjs (1)
30-59: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win500 ms abort budget is likely too tight for a cold HTTPS POST.
TLS handshake plus round trip to
api.smith.langchain.comwill frequently exceed 500 ms, so the publication will mostly abort and emit warning noise instead of exporting. Consider aligning with the Python-side flush budget (SLM_LANGSMITH_FLUSH_SECONDS, default 2 s) and making it env-configurable.♻️ Suggested change
- signal: AbortSignal.timeout(500), + signal: AbortSignal.timeout( + Math.min(5000, Math.max(100, Number(process.env.SLM_LANGSMITH_FLUSH_SECONDS ?? 2) * 1000)), + ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/run_agentv_eval.mjs` around lines 30 - 59, Update the AbortSignal.timeout call in publishLangSmithSummary to use an environment-configurable timeout aligned with the Python flush budget, using SLM_LANGSMITH_FLUSH_SECONDS and a 2-second default converted to milliseconds. Preserve the existing 500 ms behavior only as a fallback if the environment value is absent or invalid.src/slm_training/harnesses/experiments/slm317_repair_hybrid.py (1)
330-426: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo test exercises
repair_decode's fail-closed branches.The
source_unparseable/source_invalidearly-returns (lines 346-351) are the safety-critical part of this function per the module docstring ("do-no-harm... fail-closed"), but no test intest_slm317_repair_hybrid.pycallsrepair_decodeat all — only the deterministic transform/commit logic is unit-tested. The script exercises the happy path only (AR source is always valid there), so the fail-closed guarantee itself is unverified by any runnable check.A small fixture-model test (e.g. a stub with just enough surface for
_encode_context/policy/_enumerate_edits) exercisingrepair_decode("", ...)andrepair_decode(INVALID_UNPARSEABLE, ...)would close this gap.
Based on path instructions: "Non-trivial logic must leave one runnable check behind, such as an assert-based self-check or one small test file."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/slm_training/harnesses/experiments/slm317_repair_hybrid.py` around lines 330 - 426, Add a runnable test for repair_decode that covers both fail-closed branches: an empty source must return an empty result with failure "source_unparseable", and an unparseable-but-parsed-invalid source must return failure "source_invalid". Use a minimal fixture model exposing only the interfaces needed by repair_decode, and assert the returned failure metadata and kind without exercising the happy path.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/iter-slm305-edit-language-20260724.json`:
- Around line 306-314: Update the version_stamp for
harness.experiments.slm299_edit_reachability from v2 to the current v4,
preferably by regenerating the artifact so its reachability verdicts match the
shipping harness. If regeneration is not possible, add the required append-only
no-bump/staleness note and ensure the version-stamp contract passes
verification.
In `@docs/design/iter-slm310-action-alignment-20260724.json`:
- Around line 238-241: Update the verifier-call aggregation path so each decoded
evidence record increments its source/suite bucket, including the train suite,
in addition to the existing overall total. Regenerate the design artifact and
verify that train’s verifier_calls reflects its records rather than remaining
zero.
- Around line 5-10: The decision rule in
docs/design/iter-slm310-action-alignment-20260724.json lines 5-10 must add a
preregistered train/demand gap or recall criterion, require it alongside the
existing thresholds, and mark alignment inconclusive under the current results
rather than adopting it from target-share movement. Update
docs/design/iter-slm310-action-alignment-20260724.md lines 3-11 to scope the
conclusion to sampler target distribution, not adopted action alignment, and
state that alignment requires a rerun using the new criterion.
In `@docs/design/iter-slm312-state-sources-20260724.json`:
- Line 210: Align the origin-label contract between build_heldout_eval_states
and score_arm: ensure the SLM-312 builder’s heldout states use an origin label
recognized by score_arm, or update score_arm to include the observed
heldout_seed_trajectory label alongside existing origins. Then regenerate this
JSON artifact and its companion Markdown so all arm by_origin breakdowns are
populated.
In `@scripts/run_slm299_reachability_audit.py`:
- Around line 244-269: Prevent invalid comparison output in the corpus-summary
flow by rejecting or overriding `mode="v1"` when `compare` is enabled. Ensure
the primary `suites` results are labeled and computed as extended while
`suites_v1` remains v1, preserving the existing non-comparison behavior.
In `@scripts/run_slm303_decode_budget_audit.py`:
- Around line 292-312: Update _design_scoreboards to skip the audit’s own output
artifacts, including DEFAULT_JSON_OUT (and its corresponding report if
applicable), before walking iter*.json files. Keep discovering scoreboard-shaped
dictionaries from other design iteration artifacts unchanged so rerunning
run_report does not ingest its previously generated census.scoreboards or
sweep.cells.
- Around line 508-534: Update the multiprocessing handoff around proc.start(),
proc.join(), and queue.get() so the parent drains the worker result with a
blocking queue read and timeout before joining the process, rather than relying
on queue.empty(). Preserve decode_timeout only when no result arrives within the
budget, and classify an available worker result as successful even if the
child’s feeder thread delays process termination.
In `@scripts/run_slm314_winner_take_all.py`:
- Around line 400-404: Update the build_version_stamp call that constructs
payload["version_stamp"] to replace harness.experiments.slm299_edit_reachability
with harness.experiments.slm312_state_sources, preserving the existing slm314
component and return flow.
In `@scripts/run_slm317_repair_hybrid.py`:
- Around line 251-357: Make each entry in seeds produce an independent replicate
in the evaluation loop: build model_hist and model_imp with the current seed
inside the for seed in seeds loop, and ensure all seed-sensitive
training/decoding paths use that seed. Preserve the existing arm comparisons and
commit recording, while preventing identical rows from being counted as
independent paired trials; alternatively remove the seeds loop and rely on
increasing n_eval_decisions, updating the associated naming and documentation.
In `@src/slm_training/harnesses/experiments/slm308_distance_oracle.py`:
- Around line 354-361: Update the BOUNDED label construction in the
upper_bound_witness branch and the corresponding depth_bound branch so an
upper-bound witness is used as hi only when it is at least the proven lower
bound (deepest_full_depth + 2 or max_depth + 1). For inconsistent witnesses,
omit hi or fail explicitly; never create a label with lo greater than hi or
allow effective_distance to average contradictory bounds.
- Around line 285-286: Update the `_TargetMap` cache-key construction around
`map_key` and the corresponding lookup logic near the later map-cache handling
to include `node_budget`, ensuring each budget produces an independently
computed map. Preserve the existing `label_key` structure and prevent
budget-exhausted maps from being reused for richer queries.
In `@src/slm_training/harnesses/experiments/slm312_state_sources.py`:
- Around line 1016-1019: Update the target construction around supervised_edit
so Edit(ACTION_STOP) is appended only when the oracle proves distance 0; for
other rows with no supervised_edit, provide value supervision without an action
target. Restrict the action, statement, component, and slot cross-entropy row
selections to action_mask rows, preserving value-loss inclusion for unproven
states.
In `@src/slm_training/harnesses/model_build/decode_outcome.py`:
- Around line 32-40: Make FALLBACK_COUNTER_NAMES the single source of truth for
fallback attribution by updating eval_runner.py’s fallback_count computation to
reuse this tuple instead of maintaining its own hardcoded five-name list.
Preserve the existing counter filtering and fallback-count behavior, and update
imports or references as needed to use the existing symbol from
decode_outcome.py.
In `@src/slm_training/harnesses/model_build/eval_runner.py`:
- Around line 1062-1116: The per-record outcome classification currently uses
chunk-level DecodeStats, causing fallback activity from one batched record to be
attributed to every record. Update the _score_one flow and
_decode_outcome_fields to pass and use record-specific decode metadata or
fallback counters, preserving timeout and parse handling while ensuring
fallback_used and decode_outcome reflect only that record’s decoding.
In `@src/slm_training/runtime/telemetry/trace.py`:
- Line 89: Make RunTrace.__post_init__ resilient to malformed trace IDs by
validating or safely parsing trace.trace_id before assigning self.run_id, and
fall back to a newly generated valid ID when parsing fails. Ensure telemetry
initialization never raises from invalid persisted trace.json data, including
when LangSmith is disabled, while preserving valid existing IDs.
---
Outside diff comments:
In `@scripts/run_slm299_reachability_audit.py`:
- Around line 397-414: The compare execution in the script’s main
argument/report flow must honor the canonical run cap from
src/slm_training/levers.py. Add the standard deadline/timeout and interruption
handling around build_report (including the second audit), and ensure timed-out
or interrupted runs are marked incomplete and excluded from evidence/output
rather than reported as valid results.
In `@src/slm_training/harnesses/model_build/ship_gates.py`:
- Around line 67-99: Update _meaningful_metric_policy so its non-custom default
policy_id reuses MEANINGFUL_METRIC_POLICY["threshold_version"] instead of the
stale hardcoded v2 value, ensuring the returned threshold_version reflects the
configured v3 bump while preserving custom policy IDs.
In `@src/slm_training/models/tree_edit_diffusion.py`:
- Around line 1429-1512: Reduce candidate-enumeration cost in the decode
expansion around the scored list: avoid materializing and fully sorting all
INSERT_SUBTREE and statement-family combinations when only expand_per_state
candidates are consumed. Use action-score/top-k bounds to skip families that
cannot qualify, and select the final candidates with a bounded top-k approach
such as heap-based selection rather than a full sort. Preserve candidate
ordering and existing Edit coverage for candidates that can reach the bound.
---
Minor comments:
In `@docs/design/iter-slm305-edit-language-20260724.md`:
- Line 1: Update the document’s opening heading to identify both SLM-305 and
SLM-299, while preserving the existing extended edit-language and X22
reachability-audit context so searches for either issue locate the correct
artifact.
In `@scripts/run_slm308_distance_value.py`:
- Around line 369-390: Update the option loop in the regret calculation so the
STOP option, identified by child being None, passes entry["witness"] unchanged
to distance_to_target, while non-STOP children continue using entry["witness"] +
1 when a witness exists. Preserve the existing None behavior and
effective_distance handling.
In `@scripts/run_slm312_state_sources.py`:
- Around line 201-206: Add an early guard before the training loop that checks
whether arm_rows is empty and raises a clear acquisition-gap error. Keep the
existing sampling and optimization flow unchanged for non-empty arm_rows,
covering the build_arm_rows output used by this loop.
In `@scripts/run_slm314_winner_take_all.py`:
- Around line 288-296: Update build_report and its CLI execution path to enforce
or record the repository’s canonical run budget, including the MAX_RUN_MINUTES=3
cap, INTERRUPT_AFTER_SECONDS handling, and kill-grace behavior. Reuse the
documented wrapper or existing policy symbols from src/slm_training/levers.py
rather than implementing a separate budget model, and ensure the end-to-end
runner reports derived budget usage.
In `@src/slm_training/harnesses/experiments/slm312_state_sources.py`:
- Around line 666-669: Soften the docstring for the seedward source to avoid
claiming that every step is strictly distance-decreasing by construction.
Describe the edits as oracle-guided legal actions toward each gold target, while
preserving the existing statement that valid intermediate states are emitted and
the seed-side distribution is covered.
In `@src/slm_training/harnesses/experiments/slm317_repair_hybrid.py`:
- Around line 208-211: Update the learned margin initialization near soft_s,
soft_c, and margin so an explicitly provided None is treated as 0.0 before
converting to float. Preserve conversion of valid numeric values and the
existing default for missing learned_margin entries.
In `@src/slm_training/runtime/telemetry/trace.py`:
- Around line 295-298: Persist the latest OTLP mirror failure on the runtime
instance rather than passing export_error only to individual _write_manifest
calls. Initialize the error state, update it in _mirror when an export fails,
and have _write_manifest use the stored value by default so __exit__ preserves
last_export_error instead of resetting it to null.
In `@tests/test_harnesses/experiments/test_slm308_distance_value.py`:
- Around line 253-256: Replace the tautological assertion on
metrics["value_bounded"] with a direct assertion of the fixture’s intended
invariant: require at least one oracle-labeled value, using the appropriate
metric count and preserving the existing pairwise_progress presence check.
In `@tests/test_runtime_trace.py`:
- Around line 15-19: Update the autouse fixture _disable_real_langsmith_exports
to isolate tests from all LangSmith configuration: clear LANGSMITH_PROJECT,
LANGSMITH_ENDPOINT, and LANGSMITH_WORKSPACE_ID, and redirect
_LangSmithTrace._ENV_PATH to a nonexistent file before tests run. Preserve the
existing LANGSMITH_TRACING=false setup so the manifest assertions use
deterministic test values.
---
Nitpick comments:
In `@docs/design/iter-slm312-state-sources-20260724.md`:
- Around line 15-22: Update the headline results table in the design document
and its generating render_markdown flow to include beam_regret_n and
beam_regret_excluded for each arm. Preserve the existing beam regret values
while sourcing and rendering the counts from the JSON records so the table
exposes the measurement coverage.
In `@scripts/run_agentv_eval.mjs`:
- Around line 30-59: Update the AbortSignal.timeout call in
publishLangSmithSummary to use an environment-configurable timeout aligned with
the Python flush budget, using SLM_LANGSMITH_FLUSH_SECONDS and a 2-second
default converted to milliseconds. Preserve the existing 500 ms behavior only as
a fallback if the environment value is absent or invalid.
In `@scripts/run_slm310_action_alignment.py`:
- Around line 111-134: The fixture corpus is duplicated in FIXTURE_PROGRAMS and
build_fixture_records; remove these local definitions and import/reuse them from
scripts.run_slm308_distance_value. Keep the existing record construction
behavior and ensure the matched experiments share the single canonical fixture
corpus.
- Around line 315-322: Reuse the existing cell-A model from the preceding
training loop for the distribution audit instead of calling train_cell again.
Update the run_distribution_audit invocation to pass that retained model,
preserving the current audit parameters and ensuring the audit uses the same
weights as the reported cell-A metrics.
In `@src/slm_training/harnesses/experiments/slm299_edit_reachability.py`:
- Around line 365-407: Validate the mode argument at the start of
_enumerate_children so only "v1" and "extended" are accepted, matching
analyze_reachability; alternatively narrow its annotation to Literal["v1",
"extended"] and enforce invalid values at runtime. Ensure unknown modes cannot
silently follow the extended-action path, including callers such as
_build_target_map.
In `@src/slm_training/harnesses/experiments/slm308_distance_oracle.py`:
- Around line 239-250: Update the BFS around the expansion loop to stop
rebuilding visited state for each node: reuse result.dist directly as the
visited membership set, or create one shared live key view outside the loop. If
passing the live view into _enumerate_children, widen its visited annotation to
Collection[str] while preserving the existing duplicate-key checks and traversal
behavior.
In `@src/slm_training/harnesses/experiments/slm310_action_alignment.py`:
- Around line 241-246: Update the iteration in the record/evidence aggregation
flow to use strict zipping between records and evidence, ensuring any length
mismatch raises an error instead of silently truncating audit totals. Preserve
the existing processing body and aggregation behavior.
- Around line 246-257: Update the aggregation loop around _add_recall to compute
each record’s applicable-ADD recall counters once, then add those two counters
to overall, the source bucket, and the suite bucket. Refactor _add_recall or
introduce a helper so the expensive state parsing and applicable_add_exists
sweep is not repeated for each bucket, while preserving existing bucket totals.
In `@src/slm_training/harnesses/experiments/slm317_repair_hybrid.py`:
- Around line 330-426: Add a runnable test for repair_decode that covers both
fail-closed branches: an empty source must return an empty result with failure
"source_unparseable", and an unparseable-but-parsed-invalid source must return
failure "source_invalid". Use a minimal fixture model exposing only the
interfaces needed by repair_decode, and assert the returned failure metadata and
kind without exercising the happy path.
In `@src/slm_training/models/checkpoint_migrate.py`:
- Around line 343-373: Update the partial-copy branches in the state migration
loop around policy.action_head.weight and policy.action_head.bias so assignments
operate on cloned destination tensors or a freshly cloned state dictionary
rather than detached views returned by model.state_dict(). Preserve the existing
warm-start row-copy behavior and subsequent strict load_state_dict call while
avoiding any mutation of the live model before loading.
In `@src/slm_training/models/tree_edit_diffusion.py`:
- Around line 1349-1368: Update the loss computation around the comp/slot
supervision and _enumerate_edits so Edit.target and Edit.payload receive
dedicated supervision and payload/rest variants are not scored identically;
otherwise add a nearby ponytail: comment explicitly documenting this deliberate
limitation and naming the upgrade path for supervising container-rest indices
and leaf-comp/template payloads.
In `@src/slm_training/resources/versions.json`:
- Around line 4538-4555: Update the version registry’s paths arrays so
src/slm_training/harnesses/model_build/decode_outcome.py is watched by the
appropriate component, preferably the existing harness.model_build.eval
component that tracks eval_runner.py; preserve the registry’s component-version
and newest-first history conventions.
In `@tests/test_harnesses/experiments/test_slm308_distance_value.py`:
- Line 344: Remove the unused `md` assignment in the test flow around
`render_markdown(first)`, since the variable is overwritten before any read.
Preserve the later assignment and its existing behavior.
In `@tests/test_models/test_tree_edit_diffusion.py`:
- Around line 133-150: The assertion in
test_extended_sample_mutation_loop_restores should verify that seen_actions
contains at least one SLM-305 extended inverse action ID, rather than merely any
non-STOP action. Preserve the existing seeded mutation loop and use the defined
SLM-305 inverse identifiers so the test specifically proves extended actions
were exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 43fb036b-4b6d-4235-8acf-bde5ee6a4ce6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (54)
README.mddocs/design/agentv-evaluation.mddocs/design/iter-slm303-decode-budget-audit-20260724.jsondocs/design/iter-slm303-decode-budget-audit-20260724.mddocs/design/iter-slm305-edit-language-20260724.jsondocs/design/iter-slm305-edit-language-20260724.mddocs/design/iter-slm308-distance-value-20260724.jsondocs/design/iter-slm308-distance-value-20260724.mddocs/design/iter-slm310-action-alignment-20260724.jsondocs/design/iter-slm310-action-alignment-20260724.mddocs/design/iter-slm312-state-sources-20260724.jsondocs/design/iter-slm312-state-sources-20260724.mddocs/design/iter-slm314-winner-take-all-20260724.jsondocs/design/iter-slm314-winner-take-all-20260724.mddocs/design/iter-slm317-repair-hybrid-20260724.jsondocs/design/iter-slm317-repair-hybrid-20260724.mddocs/design/langsmith-telemetry-smoke-20260724.jsonpyproject.tomlscripts/run_agentv_eval.mjsscripts/run_slm299_reachability_audit.pyscripts/run_slm303_decode_budget_audit.pyscripts/run_slm308_distance_value.pyscripts/run_slm310_action_alignment.pyscripts/run_slm312_state_sources.pyscripts/run_slm314_winner_take_all.pyscripts/run_slm317_repair_hybrid.pysrc/slm_training/evals/agentv.pysrc/slm_training/harnesses/experiments/slm299_edit_reachability.pysrc/slm_training/harnesses/experiments/slm308_distance_oracle.pysrc/slm_training/harnesses/experiments/slm310_action_alignment.pysrc/slm_training/harnesses/experiments/slm312_state_sources.pysrc/slm_training/harnesses/experiments/slm314_winner_take_all.pysrc/slm_training/harnesses/experiments/slm317_repair_hybrid.pysrc/slm_training/harnesses/model_build/decode_outcome.pysrc/slm_training/harnesses/model_build/eval_runner.pysrc/slm_training/harnesses/model_build/ship_gates.pysrc/slm_training/models/checkpoint_migrate.pysrc/slm_training/models/tree_edit_diffusion.pysrc/slm_training/resources/data/slm314_multimode/modes.jsonlsrc/slm_training/resources/data/slm314_multimode/modes.jsonl.manifest.jsonsrc/slm_training/resources/versions.jsonsrc/slm_training/runtime/telemetry/trace.pytests/test_evals/test_agentv.pytests/test_harnesses/experiments/test_slm299_edit_reachability.pytests/test_harnesses/experiments/test_slm308_distance_value.pytests/test_harnesses/experiments/test_slm310_action_alignment.pytests/test_harnesses/experiments/test_slm312_state_sources.pytests/test_harnesses/experiments/test_slm314_winner_take_all.pytests/test_harnesses/experiments/test_slm317_repair_hybrid.pytests/test_harnesses/model_build/test_decode_outcome.pytests/test_harnesses/model_build/test_eval_gates.pytests/test_models/test_tree_edit_diffusion.pytests/test_runtime_trace.pytests/test_scripts/test_run_slm303_decode_budget_audit.py
| "version_stamp": { | ||
| "stamp_schema": "version_stamp/v1", | ||
| "code_commit": "5d35b90d7fc58c094604c23d702ab65593f98004", | ||
| "code_dirty": true, | ||
| "components": { | ||
| "harness.experiments.slm299_edit_reachability": "v2" | ||
| }, | ||
| "stamped_at": "2026-07-24T20:22:57.129778+00:00" | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Stale component version: this artifact stamps slm299_edit_reachability at v2 while a sibling artifact in the same PR stamps v4.
docs/design/iter-slm312-state-sources-20260724.json (Lines 462–465) records harness.experiments.slm299_edit_reachability: "v4", and both were generated on 2026-07-24 from dirty trees. This artifact predates the later bumps, so its reachability verdicts were produced against an older reachability harness than the one shipping in this PR. Regenerate it (or add the append-only no-bump:/staleness note) so the version-stamp contract holds.
As per coding guidelines, "Maintain component versions, watched paths, and newest-first append-only history in the version registry; use verify_version_stamps --check and inspect stale results after bumps."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/iter-slm305-edit-language-20260724.json` around lines 306 - 314,
Update the version_stamp for harness.experiments.slm299_edit_reachability from
v2 to the current v4, preferably by regenerating the artifact so its
reachability verdicts match the shipping harness. If regeneration is not
possible, add the required append-only no-bump/staleness note and ensure the
version-stamp contract passes verification.
Source: Coding guidelines
| "preregistered_thresholds": { | ||
| "t1_add_target_share_gain_min": 0.1, | ||
| "t2_valid_final_no_regression": true, | ||
| "t3_stop_budget_not_worse_and_valid_final_no_regression": true, | ||
| "t4_lever_isolation_structural": true, | ||
| "verdict_rule": "per lever: adopted iff its thresholds hold in both cells of the matched pair, else rejected; levers are never combined" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not present target-share movement as action-alignment adoption.
T1 passes because ADD target share rises, but the reported absolute ADD train/demand gap worsens from 0.1763 (cell A) to 0.3264 (cell B). The current rule therefore cannot answer the stated “close the train/demand action gap” question. Mark alignment as inconclusive and rerun with a preregistered gap/recall criterion.
docs/design/iter-slm310-action-alignment-20260724.json#L5-L10: add an alignment metric to the decision rule and update the current conclusion accordingly.docs/design/iter-slm310-action-alignment-20260724.md#L3-L11: scope the current result to sampler target distribution, not adopted action alignment.
📍 Affects 2 files
docs/design/iter-slm310-action-alignment-20260724.json#L5-L10(this comment)docs/design/iter-slm310-action-alignment-20260724.md#L3-L11
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/iter-slm310-action-alignment-20260724.json` around lines 5 - 10,
The decision rule in docs/design/iter-slm310-action-alignment-20260724.json
lines 5-10 must add a preregistered train/demand gap or recall criterion,
require it alongside the existing thresholds, and mark alignment inconclusive
under the current results rather than adopting it from target-share movement.
Update docs/design/iter-slm310-action-alignment-20260724.md lines 3-11 to scope
the conclusion to sampler target distribution, not adopted action alignment, and
state that alignment requires a rerun using the new criterion.
| "verifier_calls": 0, | ||
| "states_with_applicable_add": 58, | ||
| "states_recalled": 6, | ||
| "applicable_add_recall": 0.10344827586206896 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fix per-suite verifier-call aggregation.
train contains every decoded record here, yet reports verifier_calls: 0 while overall reports 11,092. The aggregation path only increments overall; increment the source/suite buckets from each evidence record and regenerate this artifact.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/iter-slm310-action-alignment-20260724.json` around lines 238 -
241, Update the verifier-call aggregation path so each decoded evidence record
increments its source/suite bucket, including the train suite, in addition to
the existing overall total. Regenerate the design artifact and verify that
train’s verifier_calls reflects its records rather than remaining zero.
| "beam_regret_mean": 0.0, | ||
| "beam_regret_n": 1, | ||
| "beam_regret_excluded": 3, | ||
| "by_origin": {} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
by_origin is empty in every arm — the origin label doesn't match the consumer.
Root cause is a cross-file contract mismatch: scripts/run_slm312_state_sources.py (build_heldout_eval_states, Lines 145 and 168) tags states "heldout_seed_trajectory", while score_arm in scripts/run_slm308_distance_value.py (Lines 428–433) only buckets ("near_gold", "seed_trajectory"). So the per-origin breakdown is silently dropped for all four arms here (Lines 210, 285, 352, 417). Either emit "seed_trajectory" from the SLM-312 builder or make score_arm bucket by the observed origins, then regenerate this artifact and the companion .md.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/iter-slm312-state-sources-20260724.json` at line 210, Align the
origin-label contract between build_heldout_eval_states and score_arm: ensure
the SLM-312 builder’s heldout states use an origin label recognized by
score_arm, or update score_arm to include the observed heldout_seed_trajectory
label alongside existing origins. Then regenerate this JSON artifact and its
companion Markdown so all arm by_origin breakdowns are populated.
| mode: str = "extended", | ||
| compare: bool = False, | ||
| ) -> dict[str, Any]: | ||
| suites: dict[str, Any] = {} | ||
| suites_v1: dict[str, Any] = {} | ||
| comparisons: dict[str, Any] = {} | ||
| for suite, records in corpora.items(): | ||
| if not records: | ||
| suites[suite] = { | ||
| "status": "corpus_unavailable", | ||
| "n_cases": 0, | ||
| "reachable_fraction": None, | ||
| } | ||
| if compare: | ||
| suites_v1[suite] = dict(suites[suite]) | ||
| continue | ||
| if limit is not None: | ||
| records = records[:limit] | ||
| cases = [ | ||
| ( | ||
| str(record.get("id", f"{suite}_{index}")), | ||
| analyze_record(record, max_edits=max_edits, node_budget=node_budget), | ||
| suites[suite] = _summarize_records( | ||
| records, max_edits=max_edits, node_budget=node_budget, mode=mode | ||
| ) | ||
| if compare: | ||
| suites_v1[suite] = _summarize_records( | ||
| records, max_edits=max_edits, node_budget=node_budget, mode="v1" | ||
| ) | ||
| for index, record in enumerate(records) | ||
| ] | ||
| suites[suite] = {"status": "ok", **summarize_suite(cases)} | ||
| comparisons[suite] = _compare_summaries(suites_v1[suite], suites[suite]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject --mode v1 --compare or force extended mode.
With mode="v1", both suites and suites_v1 are v1 results, but the payload and Markdown present the former as “extended.” Fail fast unless mode == "extended" when comparing, or always compute the primary side in extended mode.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/run_slm299_reachability_audit.py` around lines 244 - 269, Prevent
invalid comparison output in the corpus-summary flow by rejecting or overriding
`mode="v1"` when `compare` is enabled. Ensure the primary `suites` results are
labeled and computed as extended while `suites_v1` remains v1, preserving the
existing non-comparison behavior.
| if upper_bound_witness is not None and target_map.deepest_full_depth >= 0: | ||
| label = DistanceLabel( | ||
| kind=DistanceKind.BOUNDED, | ||
| reason=REASON_BUDGET, | ||
| lo=target_map.deepest_full_depth + 2, | ||
| hi=upper_bound_witness, | ||
| nodes_expanded=target_map.nodes_expanded, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A witness smaller than the proven lower bound yields contradictory BOUNDED(lo > hi).
hi=upper_bound_witness is taken verbatim while lo is deepest_full_depth + 2 / max_depth + 1. With e.g. max_depth=8, upper_bound_witness=3 the label is lo=9, hi=3, and effective_distance happily returns midpoint 6 — an estimate that violates both proven bounds and silently feeds value supervision. Since lo is a proof, a witness below it is inconsistent input and should be rejected (drop the hi, or fail loudly) rather than averaged.
🐛 Only use a witness that is consistent with the proven lower bound
- if upper_bound_witness is not None and target_map.deepest_full_depth >= 0:
+ lo_budget = target_map.deepest_full_depth + 2
+ if (
+ upper_bound_witness is not None
+ and target_map.deepest_full_depth >= 0
+ and upper_bound_witness >= lo_budget
+ ):
label = DistanceLabel(
kind=DistanceKind.BOUNDED,
reason=REASON_BUDGET,
- lo=target_map.deepest_full_depth + 2,
+ lo=lo_budget,
hi=upper_bound_witness,
nodes_expanded=target_map.nodes_expanded,
)and mirror the same guard for the depth_bound branch (hi only when upper_bound_witness >= max_depth + 1).
Also applies to: 380-389
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/slm_training/harnesses/experiments/slm308_distance_oracle.py` around
lines 354 - 361, Update the BOUNDED label construction in the
upper_bound_witness branch and the corresponding depth_bound branch so an
upper-bound witness is used as hi only when it is at least the proven lower
bound (deepest_full_depth + 2 or max_depth + 1). For inconsistent witnesses,
omit hi or fail explicitly; never create a label with lo greater than hi or
allow effective_distance to average contradictory bounds.
| if row.supervised_edit is not None: | ||
| targets.append(edit_from_dict(row.supervised_edit)) | ||
| else: | ||
| targets.append(Edit(ACTION_STOP)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Rows without a supervised edit are always trained as STOP, contradicting the documented rule.
The docstring (Lines 980–982) says such rows are "supervised as STOP when the oracle proves distance 0, else they carry value supervision only", but the code unconditionally appends Edit(ACTION_STOP) and includes the row in the action cross-entropy. acquire_on_policy sets supervised_edit=None whenever oracle_best_child finds no improving child — including UNKNOWN/plateau states that are not at distance 0 — so those states teach the policy to stop while still far from gold.
🐛 Proposed fix: only supervise STOP when the oracle proves distance 0
- if row.supervised_edit is not None:
- targets.append(edit_from_dict(row.supervised_edit))
- else:
- targets.append(Edit(ACTION_STOP))
+ if row.supervised_edit is not None:
+ targets.append(edit_from_dict(row.supervised_edit))
+ action_mask.append(True)
+ else:
+ proven_done = (
+ row.distance_label.get("kind") == DistanceKind.EXACT.value
+ and row.distance_label.get("distance") == 0
+ )
+ targets.append(Edit(ACTION_STOP))
+ action_mask.append(proven_done)Then restrict the action/stmt/comp/slot CE row sets to action_mask rows so unproven rows contribute value supervision only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/slm_training/harnesses/experiments/slm312_state_sources.py` around lines
1016 - 1019, Update the target construction around supervised_edit so
Edit(ACTION_STOP) is appended only when the oracle proves distance 0; for other
rows with no supervised_edit, provide value supervision without an action
target. Restrict the action, statement, component, and slot cross-entropy row
selections to action_mask rows, preserving value-loss inclusion for unproven
states.
| # DecodeStats counter names that mean "the emitted output came from a | ||
| # non-model fallback path". Mirrors the eval_runner fallback_count set. | ||
| FALLBACK_COUNTER_NAMES: tuple[str, ...] = ( | ||
| "unconstrained_retries", | ||
| "compiler_fallbacks", | ||
| "seeded_fallbacks", | ||
| "template_fallback_count", | ||
| "certified_fallbacks", | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Deduplicate FALLBACK_COUNTER_NAMES instead of mirroring it in eval_runner.py.
This tuple is explicitly documented as a mirror of the hardcoded 5-name list in eval_runner.py's fallback_count computation (around line 1362). Two independently-maintained copies of the same "which counters mean fallback" list will silently diverge if either is changed without updating the other — undermining exactly the goal this taxonomy exists for (consistent, non-laundered fallback attribution).
♻️ Proposed fix: single source of truth
- fallback_count = sum(
- int(getattr(row, name, 0) or 0)
- for row in decode_stats_rows
- for name in (
- "unconstrained_retries",
- "compiler_fallbacks",
- "seeded_fallbacks",
- "template_fallback_count",
- "certified_fallbacks",
- )
- )
+ fallback_count = sum(
+ int(getattr(row, name, 0) or 0)
+ for row in decode_stats_rows
+ for name in FALLBACK_COUNTER_NAMES
+ )As per coding guidelines, "Before writing code, apply the YAGNI and reuse ladder: first determine whether the feature is needed, then reuse existing code... before writing new code."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/slm_training/harnesses/model_build/decode_outcome.py` around lines 32 -
40, Make FALLBACK_COUNTER_NAMES the single source of truth for fallback
attribution by updating eval_runner.py’s fallback_count computation to reuse
this tuple instead of maintaining its own hardcoded five-name list. Preserve the
existing counter filtering and fallback-count behavior, and update imports or
references as needed to use the existing symbol from decode_outcome.py.
Source: Coding guidelines
| def _decode_outcome_fields( | ||
| pred: str, | ||
| *, | ||
| parse_ok: bool | None, | ||
| error: str | None, | ||
| decode_meta: dict[str, Any] | None, | ||
| ) -> dict[str, Any]: | ||
| """SLM-303 additive per-record decode-outcome classification. | ||
|
|
||
| Computed from the same evidence the runner already has (chunk timeout | ||
| fact, chunk DecodeStats fallback counters, parse verdict); never | ||
| changes any existing field. | ||
| """ | ||
| from slm_training.harnesses.model_build.decode_outcome import ( | ||
| MODEL_VALID, | ||
| classify_decode_outcome, | ||
| fallback_counter_total, | ||
| ) | ||
|
|
||
| meta = decode_meta or {} | ||
| stats = meta.get("stats") | ||
| fallbacks = fallback_counter_total(stats) | ||
| timed_out = bool(meta.get("timed_out")) | ||
| outcome = classify_decode_outcome( | ||
| parse_ok=parse_ok, | ||
| error=error, | ||
| fallback_counters=fallbacks, | ||
| timed_out=timed_out, | ||
| abstained=not pred.strip(), | ||
| harness_exception=False, | ||
| ) | ||
| if timed_out: | ||
| stop_reason = "decode_timeout" | ||
| else: | ||
| stop_reason = "" | ||
| for name in ( | ||
| "compiler_lattice_termination_reason", | ||
| "solver_terminal_status", | ||
| ): | ||
| value = str(getattr(stats, name, "") or "") if stats is not None else "" | ||
| if value: | ||
| stop_reason = value | ||
| break | ||
| if not stop_reason: | ||
| stop_reason = "empty_prediction" if not pred.strip() else "completed" | ||
| detail = None | ||
| if outcome == MODEL_VALID and parse_ok is None: | ||
| detail = "parse_not_evaluated" | ||
| return { | ||
| "decode_outcome": outcome, | ||
| "stop_reason": stop_reason, | ||
| "fallback_used": fallbacks > 0, | ||
| "decode_outcome_detail": detail, | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Per-record decode_outcome/fallback_used is chunk-scoped, not record-scoped, under batched generation.
_decode_outcome_fields derives fallback_counters from the single DecodeStats row collected for the whole chunk (chunk_decode_meta[-1]["stats"]), then applies that same aggregate to every record in the chunk via _score_one (lines 1320-1327). With the default generate_batch_size = 8, one record's fallback in an 8-record batch will mark all 8 records fallback_output/fallback_used=True, even the 7 that never fell back. This is exactly the "laundering runtime/harness artifacts into model-quality claims" failure mode the taxonomy's own docstring says it exists to prevent — just in the opposite direction (overcounting fallback instead of hiding it).
Since this currently only feeds the new, non-gating decode_outcome_counts metric, it won't break existing gates today, but it will produce misleading per-record evidence for any audit (e.g. SLM-303's decode-budget audit) that reads details[].decode_outcome under batched decoding.
Also applies to: 1320-1327
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/slm_training/harnesses/model_build/eval_runner.py` around lines 1062 -
1116, The per-record outcome classification currently uses chunk-level
DecodeStats, causing fallback activity from one batched record to be attributed
to every record. Update the _score_one flow and _decode_outcome_fields to pass
and use record-specific decode metadata or fallback counters, preserving timeout
and parse handling while ensuring fallback_used and decode_outcome reflect only
that record’s decoding.
| "endpoint": os.getenv("LANGSMITH_ENDPOINT") or None, | ||
| "workspace_id_configured": bool(os.getenv("LANGSMITH_WORKSPACE_ID")), | ||
| } | ||
| self.run_id = uuid.UUID(hex=trace.trace_id) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
uuid.UUID(hex=...) runs unconditionally and can abort a run whose trace.json is malformed.
RunTrace.__post_init__ adopts trace_id verbatim from an existing run_dir/trace.json (str(existing.get("trace_id") or _hex_id(16))), so any non-32-hex value (hand-edited file, schema drift, id from another tracer) makes this constructor raise ValueError — inside __post_init__, before the run even starts, and even when LangSmith is disabled. Telemetry setup should never be able to fail a run.
🛡️ Proposed fix
- self.run_id = uuid.UUID(hex=trace.trace_id)
+ try:
+ self.run_id = uuid.UUID(hex=trace.trace_id)
+ except ValueError:
+ self.run_id = None
+ self.config["enabled"] = False
+ self.error = "InvalidTraceId"start()/summary()/finish() already no-op when disabled / client is None.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.run_id = uuid.UUID(hex=trace.trace_id) | |
| try: | |
| self.run_id = uuid.UUID(hex=trace.trace_id) | |
| except ValueError: | |
| self.run_id = None | |
| self.config["enabled"] = False | |
| self.error = "InvalidTraceId" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/slm_training/runtime/telemetry/trace.py` at line 89, Make
RunTrace.__post_init__ resilient to malformed trace IDs by validating or safely
parsing trace.trace_id before assigning self.run_id, and fall back to a newly
generated valid ID when parsing fails. Ensure telemetry initialization never
raises from invalid persisted trace.json data, including when LangSmith is
disabled, while preserving valid existing IDs.
Tyler-R-Kendrick
left a comment
Tyler-R-Kendrick
left a comment
There was a problem hiding this comment.
Not merged — this PR is stale relative to main and cannot be auto-resolved without semantic decisions (review, not a rubber stamp).
Findings:
- Merge-conflicting (DIRTY) on 13 files, most of them code-semantics, not just versions.json:
models/tree_edit_diffusion.py,models/checkpoint_migrate.py,harnesses/model_build/eval_runner.py,harnesses/model_build/ship_gates.py,evals/agentv.py,runtime/telemetry/trace.py,harnesses/experiments/slm299_edit_reachability.py,scripts/run_slm299_reachability_audit.py,tests/test_tree_edit_diffusion.py,tests/test_runtime_trace.py, plus docs/agentv/langsmith/versions.json. - Large parts of the stacked branch are already on main in newer form (this is not a pure duplicate of the SLM-317 core, but the stack beneath it is superseded):
tree_edit_diffusion.py: PR carries SLM-305-era format 2 (action_head4→N rows); main is already atCHECKPOINT_FORMAT = 3/N_ACTIONS = 12(SLM-425). Merging the PR side would regress the checkpoint format.ship_gates.py: PR bumpsthreshold_versiontoopenui_ship_gates_v3; main is already atopenui_ship_gates_v4. PR side would be a gate-version downgrade.checkpoint_migrate.py:migrate_tree_edit_checkpointalready exists on main (v3-aware).- SLM-303 / SLM-305 iter docs from this branch are already on main;
eval_runner.pyandagentv.pyhave 400+/40 lines of parallel evolution on main from other merged PRs.
- Genuinely new content (not on main): the SLM-308/310/312/314/317 experiment harnesses + iter docs + tests. The SLM-317 suite (
tests/test_harnesses/experiments/test_slm317_repair_hybrid.py) passes 20/20 on the PR head, and the do-no-harm commit rule / inconclusive disposition read honest.
Recommended path: rebase onto current main, drop the superseded stack (tree_edit_diffusion / checkpoint_migrate / ship_gates / eval_runner / agentv / trace / slm299 changes — take main's versions), and keep only the new SLM-308/310/312/314/317 files plus a fresh harness.experiments.* versions.json bump on top of main's registry. Happy to re-review after the rebase.
|
Closing as superseded — content already landed on main via other PRs (see review comments for superseding commits). Reopen only with a rebased, still-unique delta. |
1 participant
Summary
SLM-317 / LAR2-06: do-no-harm AR→repair hybrid + valid-state repair advancement screen.
inconclusive— LAR3 explicitly remains OPEN but NOT advanced; powered rerun required.Verification
harness.experiments.slm317_repair_hybridv1); repo_policy ok; git diff --check clean; ruff clean.Closes SLM-317.
Summary by CodeRabbit
New Features
Documentation