β.3+β.5: AGENTS.md + vetted template + chat→solve→inspector slice (closes #3) - #9
Conversation
|
Partially reviewed — holding substantive feedback until #7 and #8 are reconciled and merged. I've gone through this in the context of the stack and it reads as coherent with the β.3/β.5 plan, but I don't want to give line-level feedback yet, for two reasons:
Once #7 and #8 are addressed, green, and merged, I'll do the full pass — correctness, the chat→solve→inspector slice, and a local run — against the settled base. |
…ate to package root Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… plot; slow-tier AC vet (F=0.99999998 through amico-run) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…o opencode session Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ead spike_solve wiring; opencode_paths helpers Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s amico-run); add amicode.runsRoot config Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…manifest identity, unbounded iter digits) Pure vscode-free ingestRunDir in run_dir_reader.ts (unit-tested); RunsRootWatcher adds incremental fs.watch + run.log tailing over it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…/aborted + fidelity display Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s (no reach-through); rollout+subspace fidelity (F=0.99994, ≤1) Re-vetted against Piccolo 1.19 (what Pkg.add installs today, per Jack's #7 note). Kills the methods(solve!) IpoptSolverExt reach-through — Callbacks is public in 1.19. Fidelity now = unitary_fidelity over the computational subspace from a fresh unitary_rollout (was reading >1 from raw-propagator integrator drift). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4bb031b to
26d9018
Compare
76efc8d to
259cdf6
Compare
|
Both threads you flagged as blocking the full #9 pass are resolved — ready when you are:
The base is still #8 → retargets to |
jack-champagne
left a comment
There was a problem hiding this comment.
Re-reviewed against the issue ACs (#3, #5) and pulled the branch locally (build/typecheck green, fast tier 45+26). The slice is well-built and the run-dir-contract consumption is solid — but two issue-AC items don't hold yet, both small fixes:
- #3 AC2 (warm-start): the documented
load_trajidiom can't reload a template-produced pulse — the template saves JLD2 keytrajectorybutload_trajreads keytraj, so the reload throws. - #5 AC3 (status bar):
solving (iter N)is never shown — the status bar only updates at completion; live iters go only to the inspector.
A few should-fixes are inline: the live-stats regex silently drops Inf/NaN (the stagnation/blow-up case those stats exist to flag), a replay→tail seam can drop a burst of iter lines, runs lose param/lab provenance, and the cold-start promote prompt can re-fire for an old run. There's also no test covering the live status-bar / incremental inspector path, which is why the status-bar gap slips past CI — one watcher test would catch it.
Solidly right: FINISHED-keyed completion reusing amico-run's validators, runs-root agreement with amico-run's default, no env propagation, no MCP/HTTP, the CSP fix, and scope stays in lane (no scheduler / sizeClass / HDF5 / multi-run). The promote→catalog prompt is a pre-existing stub from the β.2 base, not new here.
Requesting changes scoped to the two AC items (both tiny); everything else is should-fix / nit. (Minor, not inline-able since the file isn't touched here: server_manager.ts's ServerOptions.env comment still references a removed AMICODE_EXTENSION_URL / MCP-callback env — worth deleting.)
| # ensure at least one PNG even if the solve stopped before PLOT_EVERY | ||
| isfile(@sprintf("iter_%04d.png", iters[])) || save_control_plot(iters[]) | ||
|
|
||
| JLD2.save("pulse.jld2", "trajectory", prob.trajectory) |
There was a problem hiding this comment.
Saves JLD2 key trajectory, but the documented load_traj reads key traj, so warm-start reload throws (#3 AC2) — use JLD2.save("pulse.jld2", prob.trajectory).
There was a problem hiding this comment.
Fixed in a5121a9 — the template now saves JLD2.save("pulse.jld2", "traj", prob.trajectory), so the documented load_traj (which reads key traj) reloads it. Warm-start round-trips.
| getInspector()?.setImageSource(fsPath, iter); | ||
| } | ||
| iter(rec: IterRecord): void { | ||
| getInspector()?.postIterationRecord(rec); |
There was a problem hiding this comment.
Live iters go only to the inspector here — setRun fires only at completion, so the status bar never shows solving (iter N) (#5 AC3).
| // isolation; the vscode-coupled RunsRootWatcher (file_watcher.ts) consumes it. | ||
| // ============================================================================ | ||
|
|
||
| export const AMICODE_ITER_RE = /^AMICODE_ITER\s+iter=(\d+)\s+f=(-?\d+(?:\.\d+)?(?:[eE][-+]?\d+)?)\s+inf_pr=(-?\d+(?:\.\d+)?(?:[eE][-+]?\d+)?)\s+inf_du=(-?\d+(?:\.\d+)?(?:[eE][-+]?\d+)?)\s*$/; |
There was a problem hiding this comment.
This float regex rejects Inf/NaN, so stagnation/blow-up iters are silently dropped (#5 AC2) — and amico-run's own classifier keeps them, so the two disagree.
There was a problem hiding this comment.
Fixed in a5121a9 — the NUM group now accepts Inf|-Inf|NaN, matching amico-run's classifier, plus parseAmicoNum maps them to JS Infinity/NaN. Stagnation/blow-up iters are kept. Guard test in watcher_contract.test.ts.
| private attach(): void { | ||
| if (this.disposed) return; | ||
| // Skip the body already replayed by ingestRunDir; tail only appended lines. | ||
| try { this.offset = fs.statSync(this.opts.path).size; } catch { this.offset = 0; } |
There was a problem hiding this comment.
Setting offset to the current size can skip AMICODE_ITER lines appended between ingestRunDir's read and this attach.
There was a problem hiding this comment.
Fixed in a5121a9 — ingestRunDir now returns the run.log byte count it consumed, and the LogTailer attaches at that startOffset (not current EOF), so lines appended between the replay read and the tail attach aren't skipped. Covered by the 'returns the run.log byte offset' test.
| anharmonicity `δ` (GHz), `levels`, the target gate, gate time `T` (ns), | ||
| timesteps `N`, `max_iter`. **Parameters live in the script — never in this | ||
| file.** If the user gives a `lab.toml` path, read it in the script. | ||
| 3. Run it via the `bash` tool: `amico-run --project <JULIA_PROJECT> solve.jl` |
There was a problem hiding this comment.
Invocation omits --lab and the template writes no params, so a run can't report which lab/params it actually solved.
There was a problem hiding this comment.
|
|
||
| // --------------------------------------------------------------------- | ||
|
|
||
| private followLatest(): void { |
There was a problem hiding this comment.
On launch this re-reveals and can re-pop the promote prompt for a prior session's completed run, since the promote-once guard is per-run and rebuilt each switch.
There was a problem hiding this comment.
Fixed in a5121a9 — on launch the watcher baselines a prior finished run by setting activeRunDir without a sink (so it renders nothing and can't re-pop promote), and the promotedRuns Set is now shared across run-switches so a run promotes at most once. Only a fresh live completion prompts.
| /** Local resource roots the inspector webview may load images from. Includes | ||
| * the runs-root so live iter_*.png under ~/.amico/runs/... are not CSP-blocked (Q69). */ | ||
| export function inspectorResourceRootDirs(extensionRoot: string, runsRoot: string): string[] { | ||
| return [join(extensionRoot, "dist"), join(extensionRoot, "media"), runsRoot, "/tmp"]; |
There was a problem hiding this comment.
Hardcoded /tmp is now redundant with runsRoot and needlessly broadens the webview's allowed roots.
There was a problem hiding this comment.
Fixed in a5121a9 — inspectorResourceRootDirs no longer includes /tmp; the inspector only loads frames under runsRoot, so the webview's allowed roots are now just dist/media/runsRoot.
- warm-start (#3 AC2): template saves JLD2 key 'traj' so load_traj reloads it. - status bar (#5 AC3): show 'running · iter N' live (sink.iter → setRun), not only at completion. - live-stats (#5 AC2): AMICODE_ITER regex + parseAmicoNum accept Inf/-Inf/NaN so blow-up/stagnation iters aren't silently dropped (matches amico-run). - log tailer: ingestRunDir returns the run.log bytes consumed; the tailer attaches at that offset (no skipped iters between replay and attach). - promote-once: a run promotes at most once across run-switches; an already-finished run (launch-follows-latest / switch-back) never re-pops. - inspector roots: drop the blanket /tmp webview root (redundant with runsRoot). - result.toml records the scalar solve params (self-describing run). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks Jack — all addressed in the latest push (force-pushed; #10–#22 rebased on top). Must-fix
Should-fix
Disposition: |
…ck review nits The real "plots never display" bug: LiveRunSink shared one counter between image() and iter(). run.log AMICODE_ITER lines stream once per iteration and race the counter to max BEFORE the lagging PNG frames land (solver logs iter=k, then writes iter_k.png), so every image() hit `k <= latest` and was dropped — blank inspector for the whole solve. Split into SinkDedup: frames dedup only against frames; log lines advance a separate high-water mark for the status bar. Extracted SinkDedup to the pure, vscode-free run_dir_reader layer so the live path is finally unit-tested (Jack #9: "no test covering the live status-bar / incremental inspector path" — the exact gap this regression slipped through). Also folds in the remaining non-blocking review nits (kept here rather than restacking the 7-deep chain to place one-liners on their home branches): - #13: #runlabel was styled but never populated → setRunLabel + webview handler. - #12: resolveJuliaProject now expands a leading ~ (parity with resolveRunsRoot). - #9: server_manager ServerOptions.env comment no longer cites the removed AMICODE_EXTENSION_URL/MCP-callback env. - #9/#11: AGENTS.md step-2 author path now matches step-3 run path (/tmp/amicode-work/solve.jl) and the invocation passes --lab default (run provenance — amico-run supports --lab). Plus the cold-start display polish already in flight on this branch: launch stays idle for a prior finished run (no stale plot), and the placeholder toggles via style.display (the [hidden] attr was overridden by .placeholder{display:flex}). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for the thorough pass, @jack-champagne. Both AC blockers and all should-fixes are addressed: Blockers (a5121a9)
Should-fixes (a5121a9): Inf/NaN regex kept; replay→tail Your top-level callout — 'no test covering the live status-bar / incremental inspector path': now covered. I extracted the live newest-wins dedup into a pure Re-requesting review. 47 tests green. |
|
Re-checked at a5121a9 — the two AC blockers from my last review are resolved:
One gap remained on #5 AC1 ("display refreshes for the whole solve"): the live plot frame never advanced during a solve. Fixed in #23 (SinkDedup splits frame dedup from the log high-water mark), now validated end-to-end on the live vertical. Recording the link for history; resolved in the unit. Worth a follow-up: a watcher test that appends to a run dir over time so this path is covered going forward. |
jack-champagne
left a comment
There was a problem hiding this comment.
Approving as a unit member — my two blockers are resolved here; the live-plot gap is fixed in #23 (validated end-to-end). Details in the comment above.
Closes #3. Stacked on #8 (β.2) → base retargets to
mainwhen #7/#8 merge.Implements
spec-20260615-113328-agents-template-inspector: the β.3 AGENTS.md+template merged with the β.5 single-run inspector into the full chat→solve→inspector vertical slice.What this is (7 units)
amico-run <script>. Relocated topackages/extension/(the repo-root copy was invisible to the bundled extension → opencode was silently getting the stub fallback; fixed).templates/solve_template.jl— vetted single-transmon X-gate template:TransmonSystem→EmbeddedOperator→SmoothPulseProblem, Ipopt callback emittingAMICODE_ITER+ per-iter control-amplitude PNG, atomicresult.toml,pulse.jld2,DONE.prepareOpencodeProjectcopies AGENTS.md (with the Julia project substituted in) + the template into the opencode session.amico-runon PATH — esbuild stages the launcher+bundle intobin/(VSIX-self-contained); dev resolves the workspace sibling..vscodeignoreships it./tmp/amicode-runs→~/.amico/runs/default, matchingamico-run's β.1 default, so the watcher sees what the agent's solve writes.FINISHED(previously regex-scrapedresult.tomland never read FINISHED), identity frommanifest.toml, unboundediter_<N>.pngdigits. Pure vscode-freeingestRunDir(unit-tested) + incremental live path.localResourceRootsnow includes the runs-root (Q69, else blank inspector); status-bar renders failed/aborted and shows fidelity (was showing iter count mislabeled as F).AC → evidence
agents_md.test.tsguardopencode_config.test.tsopencode_paths.test.ts+ AGENTS substitution~/.amico/runs/default; FINISHED-keyed; manifest identity; unbounded digitswatcher_contract.test.ts(completed/failed/aborted/no-promote) +opencode_paths.test.tsinspectorResourceRootDirstest + watcher contract testFast tier: 44 amico-run + 26 extension tests green from a clean install. Slow tier (
AMICO_TEST_JULIA_PROJECT=… pnpm --filter amicode-v2 test:slow) passed on the dev machine.Notes
load_pulse→load_traj; vetted on Piccolo 1.19 (was 0.11 in dev); fidelity measured via fresh rollout over the computational subspace (unitary_fidelity(U, op.operator; subspace=op.subspace)) — full-space fidelity caps at 4/9 for an embedded gate and was the initial red herring. Theamico:transmonskill + Piccolo's canonicalmultilevel_transmon.jldrove the fix (drive_bounds 0.2, T=10).🤖 Generated with Claude Code