Repository navigation
Phase 2a part 1: depth primitives (dial, resume-entry, candidate sha) - #129
Conversation
…ndidate sha) The primitives the cumulative-depth loop needs, landed safely ahead of the live_climb surgery (part 2), all additive + no behavior change: - Benchmark.depth_k dial (default 1 = today's single pass; per-benchmark so 'does depth pay here?' is answerable per benchmark). - climb_once resume-entry: with resume_session_id + improve_prompt it resumes the prior session with the improve prompt instead of a fresh brief — so a depth pass builds on, and sees the measured result of, its own last pass (cumulative, per Mengye). Default off -> identical to today. - ClimbResult.candidate_sha: the sealed candidate snapshot an improved result was measured on, so the caller can publish THAT tree and a depth loop can select the best across passes by it. Part 2 (next, careful): the live_climb fixed-k depth loop that drives these + selects/publishes the best gate+panel-clean candidate. It needs a real change to the inline publish (today it commits the live workspace; best-of-k requires publishing the best sealed candidate_sha, as the wake path already does) — a change to the LIVE climb-publish path, hence its own reviewed PR. The improve prompt stays a seam (the recursive-self-improvement hook: a future agent rewrites its own depth prompt). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Round 1 — reviewed head 781e3d8a — reviewer hermes/gpt-5.6-terra.
second opinion — terra
Advisory findings from autoresearch — the code owner decides. Reply to disagree; the autoresearch:no-review label opts this PR out.
Verdict: no defects found.
No defects found in the diff.
There was a problem hiding this comment.
Round 1 — reviewed head 781e3d8a — reviewer claude/claude-opus-5.
Advisory findings from autoresearch — the code owner decides. Reply to disagree; the autoresearch:no-review label opts this PR out.
Verdict: nothing blocking — 4 advisory notes.
2 findings attached to the lines below.
Advisory (non-blocking):
- Wake path returns an improved result without candidate_sha (
src/autoresearch/orchestrator.py:916; medium) - Last assertion in the resume test can never fail (
tests/test_orchestrator.py:145; medium)
depth_k is added to the Benchmark model but is not read anywhere in src/ yet and has no test; that is consistent with the stated "part 1 primitives only" scope. The improve_prompt is passed verbatim to the harness while panel wake text is data-fenced in panel.py; whether that matters depends on how part 2 builds the prompt, which is not in this diff.
…conditions Advisory findings from #129 review (Opus 5; terra found none): - Wake path (resume_run) built an 'improved' ClimbResult without candidate_sha now that the field exists -> it left the sealed sha blank. Set it, so BOTH the in-job and wake improved results carry the tree they were measured on. - Resume-entry had no precondition: a resume_session_id with an empty improve_prompt sent an empty prompt to the harness (a burned promptless turn), and a resume on a no-resume backend ended the climb as session-error. Reject both loudly up front (same getattr optional-attr idiom as the panel policy). Loud, not a silent fresh-brief fallback: that would turn a depth pass into a fresh attempt behind the caller's back. The depth loop (caller) still owns WHEN to resume; this validates that choice. - Dropped a dead test assertion (the exact-equality above it already pins the prompt) + added two tests for the new preconditions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Round 1 addressed (90b83e0). Thanks both.
On the two scope notes: |
There was a problem hiding this comment.
Round 2 — reviewed head 90b83e0c — reviewer hermes/gpt-5.6-terra.
second opinion — terra
Advisory findings from autoresearch — the code owner decides. Reply to disagree; the autoresearch:no-review label opts this PR out.
Verdict: nothing blocking — 1 advisory note.
1 finding attached to the lines below.
There was a problem hiding this comment.
Round 2 — reviewed head 90b83e0c — reviewer claude/claude-opus-5.
Advisory findings from autoresearch — the code owner decides. Reply to disagree; the autoresearch:no-review label opts this PR out.
Verdict: nothing blocking — 4 advisory notes.
1 finding attached to the lines below.
Advisory (non-blocking):
- depth_k is accepted in contracts but nothing reads it (
src/autoresearch/contract.py:101; high) - New contract field has no CHANGELOG entry (
CHANGELOG.md; medium) - improve_prompt goes to the model unfenced and uncapped (
src/autoresearch/orchestrator.py:1074; low)
I verified the diff against the checked-out files: candidate_sha is always assigned before the improved return in climb_once's loop and before the improved return in resume_climb; all ClimbResult constructions in src use keyword args, so inserting the new field is safe; FakeHarness has a supports_resume field and records (prompt, workspace, resume_id) tuples, so the new tests exercise real behavior. I could not run the test suite or the gate, so the "699 green / mypy clean" claim is unverified here.
…condition Round 2 (Opus 5 + terra, both nothing-blocking): - depth_k was an unread contract field (part 2 is what reads it) -> a standing dead-field flag + the only user-visible change. Moved it to part 2 (the depth loop lands the dial together with the code that consumes it). Part 1 is now purely the two exercised primitives (resume-entry, candidate_sha), so no CHANGELOG entry is needed (both are internal API). - Resume precondition made SYMMETRIC (both-or-neither): a lone improve_prompt (e.g. a backend that returned no session_id) would have been silently discarded by the fresh-brief branch -- the exact 'depth pass becomes a fresh attempt' the guard claims to prevent. Now either alone raises. (Opus 5, medium.) - terra's candidate_sha 'stale tree' note: the sha is a SEALED, immutable commit (snapshot_tree write-tree); the wake path already publishes that sealed sha (climb.py:397, 'never the live tree'). The 'measured fresh' comment is the existing single-pass inline invariant (contained eval doesn't mutate tracked files), not a new bug -- no code change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Round 2 addressed (e587c2a).
|
There was a problem hiding this comment.
Round 3 — reviewed head e587c2a6 — reviewer hermes/gpt-5.6-terra.
second opinion — terra
Advisory findings from autoresearch — the code owner decides. Reply to disagree; the autoresearch:no-review label opts this PR out.
Verdict: 1 blocking, 0 advisory.
The depth_k benchmark setting is not implemented. Benchmark forbids extra fields and has no depth_k, so a contract that sets the promised per-benchmark dial fails to load and cannot configure depth. (src/autoresearch/contract.py:64; high confidence)
There was a problem hiding this comment.
Round 3 — reviewed head e587c2a6 — reviewer claude/claude-opus-5.
Advisory findings from autoresearch — the code owner decides. Reply to disagree; the autoresearch:no-review label opts this PR out.
Verdict: nothing blocking — 3 advisory notes.
1 finding attached to the lines below.
Advisory (non-blocking):
- The new candidate_sha points at a commit whose retaining ref the inline caller has already dropped (
src/autoresearch/orchestrator.py:412; medium) - Question: improve_prompt is sent to the author with no cap or data fence (
src/autoresearch/orchestrator.py:1075; low)
The PR description advertises a Benchmark.depth_k dial, but no depth_k exists anywhere in the checked-out head (grep over src/tests/docs finds nothing); the head commit message says it was moved to part 2, so the description is stale relative to the code. Verified: ClimbResult is only ever built with keyword args and is not serialized by field order, so the inserted field breaks no caller; candidate_sha at the final return is always bound because the loop body runs before any break; FakeHarness already has a supports_resume field and a 3-tuple calls log, so the new tests exercise real behavior and would fail if the resume-entry or the field were removed.
…ewer split The two advisory reviewers split on depth_k: terra (round 3) flags its ABSENCE as blocking; Opus 5 flagged its presence-unread (round 2) but is fine with it gone (round 3). Resolved by the code owner toward RESTORING it, which aligns with the Mengye-approved Phase 2a design (research-loop-buildout.md), this repo's precedent of landing primitives before their consumer (the codex backend), and terra's request -- while adding what Opus 5 wanted for quality: - depth_k back on Benchmark (default 1, bounded [1, 8]); a contract can declare the dial now, the depth loop that reads it lands in part 2. - New contract test: default is 1, accepts in-range, rejects 0 and 9. - CHANGELOG [Unreleased] entry (it is a user-visible contract field). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Round 3 addressed (2e69bbd). The two reviews split on
terra — the dial is now implemented; no contract fails to load (existing contracts omit it and default to 1). |
There was a problem hiding this comment.
Round 4 — reviewed head 2e69bbdd — reviewer hermes/gpt-5.6-terra.
second opinion — terra
Advisory findings from autoresearch — the code owner decides. Reply to disagree; the autoresearch:no-review label opts this PR out.
Verdict: no defects found.
No defects found in the diff.
There was a problem hiding this comment.
Round 4 — reviewed head 2e69bbdd — reviewer claude/claude-opus-5.
Advisory findings from autoresearch — the code owner decides. Reply to disagree; the autoresearch:no-review label opts this PR out.
Verdict: nothing blocking — 4 advisory notes.
2 findings attached to the lines below.
Advisory (non-blocking):
- candidate_sha returned by the inline path names a commit whose ref is dropped (
src/autoresearch/orchestrator.py:1259; medium) - depth_k is accepted but never read (
src/autoresearch/contract.py:101; medium)
I read contract.py, orchestrator.py (climb_once, resume_climb, _panel_revise_policy), role_runner.run_role, harness.FakeHarness and climb.py's inline/wake paths. The new params are keyword-only in effect and no caller constructs ClimbResult positionally, so inserting candidate_sha into the dataclass is safe. The coupling and no-resume-backend checks and the candidate_sha assignments match the described behavior. I cannot execute the suite, so the "702 green" claim is unverified from here.
…ume + tighten test regex Two legitimate inline findings surfaced in rounds 3-4 (Opus 5 suggestions): - The resume path skips build_brief, so task_hypothesis/lessons/recent_reports would be silently dropped on a resume -- the same silent-discard hazard the coupling check prevents. Reject them loudly (a resume pass is lean by design; the session already carries that context from its first pass). contract_text is NOT brief-only (scope/gate use it either way), so it stays allowed. - Backend-support test matched 'resume', which the coupling error also contains, so a mis-firing precondition could false-pass. Tightened to 'does not support resume' (unique to that check). Added a test for the brief-only-input guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Round 5 — reviewed head c5641156 — reviewer hermes/gpt-5.6-terra.
second opinion — terra
Advisory findings from autoresearch — the code owner decides. Reply to disagree; the autoresearch:no-review label opts this PR out.
Verdict: nothing blocking — 1 advisory note.
Advisory (non-blocking):
- The candidate SHA test misses the wake path (
tests/test_orchestrator.py:279; high)
There was a problem hiding this comment.
Round 5 — reviewed head c5641156 — reviewer claude/claude-opus-5.
Advisory findings from autoresearch — the code owner decides. Reply to disagree; the autoresearch:no-review label opts this PR out.
Verdict: nothing blocking — 2 advisory notes.
1 finding attached to the lines below.
Advisory (non-blocking):
- depth_k is accepted by the contract loader but no code reads it (
src/autoresearch/contract.py:102; medium)
I read climb_once, resume_climb, run_role, FakeHarness, and climb.py's publish/wake paths. candidate_sha is not consumed anywhere yet (only the local variable in climb.py is used), so adding it to ClimbResult changes no behavior; all ClimbResult constructions use keyword args, so the new field's position is harmless. The new params are appended and all callers/tests use keywords. improve_prompt reaches the author session unfenced, but in this codebase fencing is done by the producer (panel.py builds wake_text), so a part-2 caller fencing it would match existing practice; I could not verify the part-2 caller since it is not in this diff. The new tests fail without the change (default candidate_sha is "", and the preconditions do not exist), so they are not vacuous.
terra (round 5): the candidate_sha test covered only the in-job improved return, not the wake path (resume_climb), which round 1 also taught to set candidate_sha. Assert out.candidate_sha == CAND in the existing wake-improved test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
brief_baseline is also brief-only (it feeds build_brief in the else branch), so a resume silently discarded it while the other three raised. Include it for completeness (brief_baseline is not None -> reject; 0.0 is a real ledger value). Extended the test to cover it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Round 6 — reviewed head f84d8aa9 — reviewer hermes/gpt-5.6-terra.
second opinion — terra
Advisory findings from autoresearch — the code owner decides. Reply to disagree; the autoresearch:no-review label opts this PR out.
Verdict: no defects found.
No defects found in the diff.
There was a problem hiding this comment.
Round 6 — reviewed head f84d8aa9 — reviewer claude/claude-opus-5.
Advisory findings from autoresearch — the code owner decides. Reply to disagree; the autoresearch:no-review label opts this PR out.
Verdict: nothing blocking — 2 advisory notes.
1 finding attached to the lines below.
Advisory (non-blocking):
depth_kis accepted but nothing reads it (src/autoresearch/contract.py:102; high)
Verified against the checked-out files: run_role accepts resume_session_id, FakeHarness records (text, workspace, resume_id) and has a supports_resume field, resume_climb's improved return has candidate_sha in scope, and candidate_sha is always bound before the improved return in climb_once (assigned each loop pass before any break). The tests match the code paths they claim. The snapshot-ref lifetime problem (live_climb's finally drops every snapshot ref, so a returned candidate_sha would be unreferenced) is real in climb.py:1444-1450 but nothing consumes candidate_sha yet, and the PR description already records it as a part-2 constraint. I could not run tests or the gate.
created is the last optional brief-only param (feeds only build_brief); a resume silently dropped it. Add it, completing the EXHAUSTIVE set of optional brief-only inputs (task_hypothesis/lessons/recent_reports/created/brief_baseline) with a comment so a future build_brief input gets added here too. ruler is also brief-only but REQUIRED, so it cannot be omitted -- unavoidably passed and ignored on resume (documented, not guarded). run_climb now omits created on a resume (a resume is lean); test covers created too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Round 7 — reviewed head d1b392dd — reviewer claude/claude-opus-5.
Advisory findings from autoresearch — the code owner decides. Reply to disagree; the autoresearch:no-review label opts this PR out.
Verdict: nothing blocking — 1 advisory note.
1 finding attached to the lines below.
I checked the rest of the diff against the current sources. candidate_sha is assigned before every break in climb_once's loop, so the improved return is always populated, and the panel-revision path leaves it pointing at the last snapshot that was measured; resume_climb sets the same field for the wake path. All ClimbResult(...) constructions in src/ use keyword arguments, so inserting the new field is safe. depth_k is validated but nothing reads it yet, so a contract setting it above 1 gets no depth — the CHANGELOG says this explicitly, so I did not raise it. improve_prompt is passed to run_role unfenced, but that matches the existing split where the producer fences (panel.py _render_wake), and no caller in the repo constructs one yet.
The guard test covered 3 of the 5 guarded inputs, so dropping lessons or recent_reports from the guard would still pass. Cover all five, so removing any one regresses the suite. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Round 8 — reviewed head 2a0e288a — reviewer hermes/gpt-5.6-terra.
second opinion — terra
Advisory findings from autoresearch — the code owner decides. Reply to disagree; the autoresearch:no-review label opts this PR out.
Verdict: no defects found.
No defects found in the provided diff.
There was a problem hiding this comment.
Round 8 — reviewed head 2a0e288a — reviewer claude/claude-opus-5.
Advisory findings from autoresearch — the code owner decides. Reply to disagree; the autoresearch:no-review label opts this PR out.
Verdict: nothing blocking — 3 advisory notes.
Advisory (non-blocking):
- candidate_sha comment says the caller publishes that tree, but the in-job caller does not (
src/autoresearch/orchestrator.py:412; medium) - the "exhaustive" brief-only guard misses config.budget (
src/autoresearch/orchestrator.py:1054; low) - no way to pin run_seed across depth passes (
src/autoresearch/orchestrator.py:1069; low)
I verified the diff against the checked-out sources: FakeHarness has a supports_resume field and records (prompt, workspace, resume_id) tuples, so the resume test's assertions are real; ClimbResult is never built positionally or serialized field-by-field, so adding candidate_sha mid-dataclass is safe; load_contract lets pydantic ValidationError escape, so the depth_k range test is valid. depth_k is read nowhere yet, which the CHANGELOG and the field comment both state, so I did not raise it as a defect. improve_prompt is forwarded to the harness unfenced, but it is caller-supplied here and the repo fences at the construction site (panel.py:97-105), so that is a part-2 concern, not a defect in this diff. I could not run the test suite (no execute tool), so the "702 green" claim is unverified.
…guard note
Two comment-accuracy fixes (Opus 5, no behavior change):
- candidate_sha comment claimed 'the caller publishes THIS tree', but the in-job
caller does not yet (part 2 wires that); reword to capability ('a caller CAN
publish it; the wake path already does').
- the 'exhaustive' brief-only guard comment now notes required params with
brief-only fields (ruler, config.budget) are excluded because a caller cannot
omit them, not because they were missed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Round 8 addressed (8c55503): the two comment-accuracy notes are fixed (candidate_sha 'caller publishes' → capability wording; the guard comment now explains ruler/config.budget are excluded as required-can't-omit, not missed). The third — no way to pin run_seed across depth passes — is a part-2 constraint (the depth loop threads one seed across passes for a fair best-of-k; adding it as an unused param here would just be dead code), captured in the PR-body constraints + noted for part 2. |
There was a problem hiding this comment.
Round 9 — reviewed head 8c55503f — reviewer claude/claude-opus-5.
Advisory findings from autoresearch — the code owner decides. Reply to disagree; the autoresearch:no-review label opts this PR out.
Verdict: nothing blocking — 3 advisory notes.
Advisory (non-blocking):
- candidate_sha comment overstates that a caller can publish that tree on the in-job path (
src/autoresearch/orchestrator.py:409; medium) - depth_k is validated but ignored, with no runtime signal (
src/autoresearch/contract.py:102; medium) - improve_prompt reaches the harness with no length cap and no data fence (
src/autoresearch/orchestrator.py:1087; low)
I could not run the test suite (no execute tool), so the "703 green" claim is unverified; I checked the new tests by reading them and the helpers they use (FakeHarness records (text, workspace, resume_id) and has a supports_resume field, so the resume assertions are meaningful). The three preconditions in climb_once are consistent with today's optional brief-only params: BriefInputs has task/contract_text/ruler/lessons/recent_reports/budget and build_brief takes created, all covered by the guard or required. The new params are keyword-defaulted and the only production caller (climb.py:1387) passes neither, so behavior at defaults is unchanged.
What
The primitives the cumulative-depth loop (Phase 2a,
research-loop-buildout.md) needs — landed ahead of thelive_climbsurgery, all additive + no behavior change at today's defaults:Benchmark.depth_kdial (default 1 = today's single pass; bounded[1, 8]; per-benchmark, so "does depth pay here?" is answerable per benchmark). The declared knob — a contract can set it now; the depth loop that reads it lands in part 2.resume_session_id+improve_promptresume the prior session with the improve prompt instead of a fresh brief — a depth pass builds on, and sees the measured result of, its own last pass (cumulative depth). The two are a coupled pair (both-or-neither, validated loudly); a resume also rejects the brief-only inputs it would silently drop (task_hypothesis/lessons/recent_reports/created/brief_baseline) and a no-resume backend. Default off → identical to today.ClimbResult.candidate_sha: the sealed (immutable) candidate commit animprovedresult was measured on — so the caller publishes that tree and a depth loop selects the best across passes by it. Set on both the in-job and wake improved returns.Why split from the loop
Part 2 is the
live_climbfixed-kdepth loop that drives these and publishes the best gate+panel-clean candidate. It touches the live climb-publish path (today it commits the live workspace; best-of-kmust publish the best sealedcandidate_sha, as the wake path already does —climb.py:397), so it gets its own reviewed PR.Part-2 constraints surfaced in review (captured, not part-1 bugs):
finallydrops snapshot refs — cf. the park path'skept_ref). In part 1 nothing dereferencescandidate_sha, so it's latent until part 2.improve_promptfrom measured result text, it must data-fence it like the panel wake path (panel.py). In part 1improve_promptis a dormant caller-supplied param.run_seedacross passes for a fair best-of-k(the resume-entry draws a fresh seed per call today; a shared seed belongs where it's consumed, not as an unused part-1 param).Test plan
depth_k: default 1, accepts in-range, rejects out-of-range.candidate_shaonimproved(in-job and wake path); resume-entry skips the brief and resumes with the improve prompt; the coupled-pair, no-resume-backend, and all five brief-only-input preconditions raise.No secrets / no large files
Confirmed.