feat(gate2): real Claude analyzer completes hn-monitor analyze-story - #130
Conversation
|
Warning Review limit reachedNext included review available in 1 minute. View limit detailsLimit details: You’ve used the included review currently available. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (16)
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds a Claude-backed ChangesClaude analyzer flow
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The PR adds the real analyzer execution path and supporting path resolution, with no actionable merge-blocking risk remaining after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant LiveTest
participant Kernel
participant ClaudeCLI
participant Claude
LiveTest->>ClaudeCLI: Run auth status probe
ClaudeCLI->>Claude: Perform authenticated round trip
Claude-->>ClaudeCLI: Return readiness result
LiveTest->>Kernel: Submit story event
Kernel->>ClaudeCLI: Analyze wake context
ClaudeCLI->>Claude: Request structured story analysis
Claude-->>ClaudeCLI: Return JSON analysis
ClaudeCLI-->>Kernel: Emit validated story fields
Kernel-->>LiveTest: Return schema pass and completed run
Poem
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Essentials by visiting https://app.coderabbit.ai/settings/billing. Comment |
maintainability lens — UNCLEARThere's an issue with the selected model (fable). It may not exist or you may not have access to it. Run --model to pick a different model. |
history lens — FAILBlocker
Concerns — non-blocking
Notes
REVIEW_FAILED |
structure lens — PASS→ Read docs/RFC-0001-everything-is-a-relayflow.md Structure review — PR #130Boundaries. Nothing here reaches into the Rust kernel or the journal protocol; the change is confined to Primitives vs. helpers. Good discipline here. Fail-closed / completionReason discipline. The test asserts Concerns.
Notes. No blockers. REVIEW_PASSED |
|
🎯 review-swarm: FAILED (M:unclear H:fail S:pass) Lens transcripts posted as sibling comments above. |
43c27b0 to
78a8b19
Compare
Review round: 3-lens pre-swarm relayflow (
|
A CLI that inherits whatever model the host pins produces two failures: runs whose model cannot be recovered from the journal, and hard failure on a host pinning an alias the CLI cannot resolve. Both are live on this machine — ~/.claude/settings.json pins "fable", and `claude -p` without an explicit model answers "There's an issue with the selected model (fable)". That has bitten four separate places in this repo, including the review swarm's own maintainability lens, whose entire review body on PR #130 was that error message rather than a verdict. So `model` becomes a declared, journaled property of an agent step, carried the same way `cli` already is: - sdk/src/spec.ts: model on AgentStepSpec and KernelAgentStep. - sdk/src/validate.ts: allow-listed and type-checked. The empty string is rejected — it would reach the CLI as RELAYFLOW_MODEL='', which reads as "declared, and declared as nothing"; absent and empty must not look alike. - sdk/src/compile.ts: carried through all four sites cli uses, including the kernel->authoring inverse, so a round trip is lossless. - kernel/relayflowd-core/src/spec.rs: model on StepKind::Agent and its field allow-list. The kernel never calls a model and never interprets this; it carries and journals it so the choice is part of the run's record, then hands it to the worker. - sdk/src/worker.ts: surfaced to the CLI as RELAYFLOW_MODEL, and explicitly UNSET when the step declares none — same undefined-vs-empty discipline as RELAYFLOW_WAKE_CONTEXT. Without the explicit unset a leaked parent variable would silently pin a step that declared nothing. - sdk/src/preflight.ts + cli/check.ts: the probe now runs with the declared model in scope, so readiness answers "can this CLI use THIS model" instead of the weaker "is this CLI authenticated at all". Model is part of the probe cache key, or a model the CLI cannot resolve would inherit an earlier model's pass. There is deliberately no flow- or project-level default: inheriting a model from two levels up is the ambient-state problem this field removes. NOTE ON SCOPE: this edits kernel/, which ops/NEXT.md lists under explicit non-goals and which my earlier commits on this branch said they avoided. That instruction came from Khaliq, who owns these gates. Recording it here so the history does not read as a quiet violation. Also fixes a Windows bug the review swarm caught in resolveSpecCliPaths: it detected bare command names with `cli.includes('/')`, so a relative `preflight\analyzer` would be misread as a PATH command and left unresolved. Both separators are now checked. Tests, all with fail-first evidence captured in the PR body: - SDK/kernel parity: hello-agent now declares a model, so the existing bit-for-bit parity pair covers it. Canonical JSON and sha256 regenerated by the compiler. Reverting the kernel allow-list fails the Rust parity test, proving both halves are pinned to one dialect. - Worker set/unset pair against a live kernel, using a probe CLI that reports UNSET and EMPTY as distinct answers. The unset case pollutes process.env first, so it fails if the explicit unset is removed. Live acceptance, launched with ANTHROPIC_MODEL and RELAYFLOW_MODEL both unset so the declared value is the only thing that can work on this host: 5 real Hacker News stories, 5 completed, 0 failed, every step verdict pass / completionReason success, and run.spawned journals model=claude-haiku-4-5-20251001. `flows check` refuses the flow when the declared model is bogus, which is the probe doing real work. Session-Id: d4302017-1150-4ce8-8b77-01a5248b1414
Scope addition at
|
208def8 to
2e51800
Compare
Closes RFC-0001 gate 2 follow-up B from ops/reviews/20260901-1050-gate2-live-run.md. The merged live run (PR #121, 5835cba) ended all 9 analyzer attempts in worker_error -> step_failed: analyze-story declared a schema but no CLI. The worker and trigger planes were already merged; the analyzer program was the gap. Squashed from four working commits. Two of those messages made evidence claims that did not hold — one quoted an analyzer sha256 that a later edit in the same branch invalidated, and one said fail-first evidence was in the PR body when it was in a PR comment. The review swarm's history lens caught both. They are removed rather than annotated, because an acknowledgement elsewhere does not repair a false statement in an immutable commit message. This message therefore states what was verified and leaves the captured commands and outputs to the PR body, which is regenerated against this exact tree. What ships: - testdata/preflight/analyze-story-claude-cli. Reads the story from RELAYFLOW_WAKE_CONTEXT (PR #125, 7b115bd), asks Claude to judge it, and emits one JSON object carrying only the three schema-declared fields, so no unvalidated model chatter reaches the journal (PR #124, 3855099). `auth status` performs a live round-trip rather than checking the binary exists: presence says nothing about the model resolving or the session being authenticated, and a false "ready" would let a broken box emit a skip that reads like acceptance. - The canonical hn-monitor spec DECLARES that CLI, in both the YAML and the compiled JSON. Declaring it in a test copy only would have left `flows hn-monitor start` shipping a spec with no CLI — a green test over a dead workload. - resolveSpecCliPaths, because the two halves of the system disagreed about what a relative cli path means. `flows check` resolves it against the SPEC's directory (sdk/src/cli/check.ts probeCli), while AgentWorker ends at spawn(cli, ...), which resolves against the WORKER PROCESS's cwd. They coincide only when the runner starts from the spec's directory, so a spec that passed `flows check` could still die with ENOENT once launched. It returns a copy, and it tests for both path separators — a Windows `preflight\analyzer` would otherwise be misread as a bare PATH command. - `model` as a declared, journaled property of an agent step, carried exactly where `cli` already is: SDK authoring and kernel dialects, validation, all four compiler sites including the kernel->authoring inverse, StepKind::Agent and its field allow-list in the kernel, and the worker, which surfaces it to the CLI as RELAYFLOW_MODEL and leaves it UNSET when the step declares none. Preflight probes with the declared model in scope, so readiness answers "can this CLI use THIS model" rather than "is this CLI authenticated at all", and the model is part of the probe cache key. There is deliberately no flow- or project-level default; inheriting a model from two levels up is the ambient-state problem the field removes. Why it is needed: a CLI inheriting whatever the host pins gives runs whose model cannot be recovered from the journal, and hard failure on a host pinning an unresolvable alias. This machine pins "fable", and that has broken four things here, including the review swarm's own maintainability lens, whose entire review body on this PR was that error message instead of a verdict. An unavailable analyzer FAILS the acceptance test by default; skipping is opt-in via RELAYFLOWS_ALLOW_ANALYZER_SKIP=1. The rule it implements: a skip is diagnostics and never acceptance evidence, so strict has to be the default rather than a convention a reader has to know about. That rule comes from the gate-2 brief this task was given, which lives outside this branch. The ops/NEXT.md committed here is a DIFFERENT, older brief about building the worker, so the two do not share item numbers. An earlier version of this message, and a comment in live-kernel.test.ts, cited "ops/NEXT.md item 3" for the rule above, which is false against the committed copy — item 3 there is the worker-attach rule. Both citations are removed rather than renumbered. SCOPE: this edits kernel/, and BOTH briefs forbid that. The gate-2 brief lists "Editing kernel/" under explicit non-goals, and the ops/NEXT.md committed here lists "Changes to the kernel" under "Explicitly OUT of scope" (line 78). Khaliq, who owns these gates, directed the kernel work anyway, so it ships against both briefs deliberately rather than by oversight. An earlier version of this message claimed the committed ops/NEXT.md had no non-goals section; that was wrong — I had grepped for "non-goal" and missed the "OUT of scope" wording. The kernel change is inert: it carries and journals the field and never interprets it. Stated here so the history does not read as a quiet violation. The kernel change is inert — it carries and journals the field and never interprets it. No retry, scheduling, dedupe or lease logic was added. Session-Id: d4302017-1150-4ce8-8b77-01a5248b1414 Session-Id: d4302017-1150-4ce8-8b77-01a5248b1414 Session-Id: d4302017-1150-4ce8-8b77-01a5248b1414
2e51800 to
82be45f
Compare
maintainability lens — PASSExact head Reading the charter and RFC... This PR adds a Blockers: NoneThe plumbing works, tests are solid, and the change aligns with the RFC's identity + credentials covenant (gate 8). Concerns1. resolveSpecCliPaths coupling & re-entrancy 2. Env-var deletion pattern isn't DRY delete env[MODEL_ENV];
if (model !== undefined) env[MODEL_ENV] = model;With identical reasoning in both comments ("explicit unset prevents inherited pollution"). This is a silent contract that a future editor of one site won't see the other and might get it wrong. Extract to a helper: 3. Empty-string validation not covered by visible tests 4. Test pollution is risky 5. Cache-key inclusion is correct but not obviously so 6. Analyzer CLI requires Node ≥ 22 silently Notes
REVIEW_PASSED |
history lens — PASSExact head Blockers None. The diff neither repeats a DRIVE-LOG mistake nor contradicts a settled RFC decision, and the squashed commit message accurately describes the final tree. Concerns — non-blocking
Notes
REVIEW_PASSED |
structure lens — PASSExact head
→ Read docs/RFC-0001-everything-is-a-relayflow.md STRUCTURE review — PR #130 (
|
|
🎯 review-swarm: PASSED (M:pass H:pass S:pass) @ 82be45f Exact-head run against |
1 participant
What this closes
Follow-up B in
ops/reviews/20260901-1050-gate2-live-run.md. The merged live run (PR #121,5835cba) ended all 9 analyzer attempts inworker_error → step_failed:analyze-storydeclared a schema but no CLI.What ships
testdata/preflight/analyze-story-claude-cli— reads the story fromRELAYFLOW_WAKE_CONTEXT(PR feat(sdk): expose wake_context to agent CLIs via RELAYFLOW_WAKE_CONTEXT #125,7b115bd), asks Claude to judge it, emits one JSON object carrying only the three schema-declared fields so no unvalidated model chatter reaches the journal (PR feat(sdk): AgentWorker promotes CLI JSON output for json_schema verification #124,3855099).auth statusdoes a live round-trip, not an existence check: presence says nothing about the model resolving or the session being authenticated, and a false "ready" would let a broken box emit a skip that reads like acceptance.The canonical spec declares it — YAML and compiled JSON. Declaring it only in a test copy would leave
flows hn-monitor startshipping a spec with no CLI: a green test over a dead workload.resolveSpecCliPaths—flows checkresolves a relativecliagainst the spec's directory (check.ts probeCli);AgentWorkerends atspawn(cli, …), which resolves against the worker process's cwd. They coincide only when the runner starts from the spec's directory, so a spec that passedflows checkcould still die with ENOENT once launched. Returns a copy; tests both separators, since a Windowspreflight\analyzerwould otherwise be misread as a bare PATH command.modeldeclared on agent steps, end to end — carried exactly wherecliis: both SDK dialects, validation (empty string rejected), all four compiler sites incl. the kernel→authoring inverse,StepKind::Agent+ its kernel field allow-list, and the worker, which surfaces it asRELAYFLOW_MODELand leaves it unset when the step declares none. Preflight probes with the declared model in scope and keys its cache on it. No flow- or project-level default: inheriting a model from two levels up is the ambient-state problem the field removes.Needed because a CLI inheriting whatever the host pins gives runs whose model cannot be recovered from the journal, and hard failure on a host pinning an unresolvable alias. This box pins
fable, which has broken four things here — including the review swarm's own maintainability lens, whose entire review body on this PR was that error instead of a verdict.Scope: this edits
kernel/, forbidden by BOTH briefs: the gate-2 brief listsEditing kernel/under non-goals, and theops/NEXT.mdcommitted here listsChanges to the kernelunderExplicitly OUT of scope(line 78). Khaliq directed it anyway, so it ships against both deliberately rather than by oversight. That instruction came from Khaliq, who owns these gates. Stated plainly rather than left for a reader to discover. The kernel change is inert — it carries and journals the field, never interprets it. No retry, scheduling, dedupe or lease logic.Tree identity
Fail-first evidence — four mutations, each restored
1. Analyzer absent. No
RELAYFLOWS_*variable set, because strict is the default:2. Worker drops the model pass-through (
runCli(..., spec.model)→runCli(...)):3. Remove the explicit
delete env[MODEL_ENV]— the parent-inheritance leak. The test pollutesprocess.envfirst precisely to catch it:Without that one line a step declaring no model silently runs pinned to whatever the launching shell exported.
4. Remove
"model"from the kernel'sSTEP_AGENT_FIELDS— SDK/kernel parity:hello-agentnow declaresmodel: test-model-v1, so the existing bit-for-bit parity pair covers the field. Canonical JSON and sha256 were regenerated by the compiler, not hand-edited.Green after restore
The title returns carrying
wake-nonce-7f3a91c4. Nothing else on the machine or in training data contains that string, so only an analyzer that received this event's wake context can echo it.Preflight really probes the declared model
Live launched workload
flows hn-monitor starton the canonical spec against real Hacker News — not the test harness — withANTHROPIC_MODELandRELAYFLOW_MODELboth unset, so the declared spec value is the only thing that can work on a box pinningfable:Five real stories, five completed, zero failed — the inverse of the recorded 9-runs-9-failed. The model is recoverable from
run.spawned, which is the point of declaring it. Scores discriminate: 1 for a Commodore 64 anniversary post, 8 for a model release.Known tensions, not fixed here
resolveSpecCliPathsputs a host-specific absolute path into journaled state, in tension with RFC decision 14's digest/provenance model. Structure lens raised it as non-blocking; correct fix is resolving at the worker/ingestion boundary so every runner inherits it, rather than per-runner. Out of gate-2 scope.resolveSpecCliPaths,probeCli, the barespawn), so a future runner submitting a relativeclire-inherits the ENOENT bug.Toolchain defects found, reported not hidden
npmhangs here —npm -vexits 124;~/.npmrcsymlinks into a Dropbox path whose read hangs. All npm use needsNPM_CONFIG_USERCONFIG=/dev/null.npm testis not runnable on this host —test:prepdies withmise ERROR cargo is not a valid shimbefore reaching vitest.ops/cargo.shis broken here — it forcesCARGO_HOME/RUSTUP_HOMEto~/.relayflows-toolchain, which is empty, so the mise shim fails and the rustup proxy reportscould not choose a version of cargo to run. I built withCARGO_TARGET_DIRset to the same per-worktree path the script computes rather than edit a gate script.locateRelayflowdcan test the wrong binary — it picks the most recently built relayflowd across all worktree keys. This worktree is1914866954; tests had been resolving3076306992, built from a different worktree. Harmless until a PR toucheskernel/— as this one does. Every kernel-dependent run above pinsRELAYFLOWD_BINto this worktree's own build.testdata/hn-monitor.spec.canonical.jsonhas never passedflows check— snake_case trigger keys vs the checker's camelCase. Verified identical refusal at7728565. Pre-existing, untouched.claude -pwith no--model(ops/preswarm-check/lens-runner.sh:205,review-swarm-loop.sh:145), so on a host pinning an unresolvable alias the lens returns the model error as its review body — an environment failure shaped like a verdict. That produced the earlierM:unclearaggregate here.review-swarm-loop.shcannot be run as its own usage line documents — it sayssh …, but line 278 uses bash process substitution.I edited none of these: AGENTS.md says never edit a gate that judges your own work.