Skip to content

Make --check JSON agree with its exit code; show recipe-less outputs in status - #241

Open
cailmdaley wants to merge 7 commits into
mainfrom
smoke-findings
Open

cailmdaley wants to merge 7 commits into
mainfrom
smoke-findings

Conversation

@cailmdaley

@cailmdaley cailmdaley commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

Cheap agents running a five-task smoke suite against the agent-skills lightcone plugin, on lc 0.5.0rc5, hit two lc behaviours that mislead them. Each is fixed here, with a test that fails on main.

lc materialize --check --json said "ok": true and exited 1 when outputs were still to build. An agent reading the JSON concluded that it was done. ok is removed from the materialize report. In a real run it meant "nothing failed or was blocked", which check mode can never violate. Redefining it as the exit code would have made it identical to up_to_date in check mode. The report now starts with up_to_date:

  • A run succeeded when failed and blocked are empty.
  • A check passes when up_to_date is true.
  • The exit code is 1 when any of failed, blocked or planned is non-empty.

--help states the contract. A check with planned work still prints "would be made" rather than "did not finish". eval.yml reads only up_to_date and current, so it is unaffected. The decision is recorded in CLAUDE.md. Follow-up outside this repo: docs (commands/materialize.mdx, concepts.mdx) still names ok as a key to branch on.

A declared output with no recipe vanished from lc status. An agent asked to wire a missing recipe couldn't see what was missing. Such outputs now appear in both human and --json status with the state no recipe, and counts["no recipe"] counts them. Re-exports (from:) are not listed this way, since their target output is already listed. The empty-report message now reads "The analysis declares no output."

Tests: the full suite minus test_container_smoke.py gives 1152 passed and 3 skipped. The 4 container smoke tests fail identically on main locally ("No such image: sha256:…" on Docker 27.3.1), which looks environmental. A reviewer copied the new tests onto origin/main, saw them fail for the stated reasons, and ran both commands by hand on a mixed spec.

Two questions for review:

  • Should lc materialize --check also fail while some declared output has no recipe? The lightcone skill calls --check "the gate" and tells agents they're done when it passes, so today an unwired output passes the gate.
  • The agent-skills lightcone skill documents counts{current,behind,stale} and three states. It needs a no recipe row once this lands; that follow-up goes in agent-skills.

Not included: the smoke suite also reported that the local cluster sizes itself from host CPUs instead of the container limit. That doesn't reproduce. Inside the smoke image, lc compute resources reports 2 CPUs under docker --cpus=2 and 3 under --cpuset-cpus=0-2, because dask's CPU_COUNT already honours affinity and cgroup quotas. The original observation came from a Harbor run on Docker Desktop that didn't apply the quota.

Claude Opus 5.5 on behalf of Cail

🤖 Generated with Claude Code

https://claude.ai/code/session_01UJCxD1csvG8NnxWLtew9uB

cailmdaley and others added 3 commits October 3, 2026 00:10
A check with planned outputs fails the gate, so report ok as false.

Co-Authored-By: GPT-6 Luna <noreply@openai.com>
The plan preserves non-executable declarations so users can see what still needs a recipe.

Co-Authored-By: GPT-6 Luna <noreply@openai.com>
A `from:` re-export stands for an output that is already reported, and
the nine-character `no recipe` state pushed its row out of the table.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

✅ Eval

Metric Value
Outputs check success
Agent run success
Turns 8
Tool calls 6
Cost $0.15
Agent wall time 0m56s
Model claude-sonnet-5-5
lc status
  mode:    direct
  sandbox: landlock (fs: declared, network: allowed)
  crate:   up to date with the outputs

  · current   baseline/best_fit        a17d288
  · current   baseline/hubble_diagram  a17d288
  · current   baseline/residuals       a17d288

3 current
Confusion & pain points (Claude analysis)

Confusion & pain points

  • Heredocs failed because scripts/ didn't exist. The agent wrote scripts/*.py in the same Bash call as uv add and astra.yaml patching. Every cat > scripts/... failed with "No such file or directory". The agent then ran the same five heredocs a second time after mkdir scripts. Because it was one chained command, the later steps (astra validate, git commit, lc materialize) ran against an incomplete tree. The cause is that the scaffold doesn't create a place for analysis code, which is deliberate. The hint in the spec or skill output is thin, and the failure cost a full redo of about 150 lines. Suggested fix: have the astra skill, or lc init output, say where recipe scripts are expected to live.

  • The agent inferred the data schema and never checked it. It read only head/sed of SCPUnion2.1_mu_vs_z.txt, with no header. It guessed the columns as name, z, μ, stat error, sys error. It also guessed that stat_and_sys means adding them in quadrature. The final reduced χ² of 0.37 looks suspicious, and the agent said so but didn't investigate. The spec's input or decision descriptions don't document the column layout or the meaning of error_model. Suggested fix: have astra.yaml inputs carry a schema or column note, or have the eval prompt state the error-model semantics.

  • The agent launched compute with a bare lc compute launch --wait, with no inspection first. It piped the output through tail -3 and then used the name local in materialize without confirming it. That worked, because the built-in local shortcut defaults the name to local. But the agent never ran lc compute resources or status. It relied on prior knowledge of the shortcut's behavior, so a different default would have failed silently behind tail. Minor: print the launched name in the --wait output.

  • format: and inputs:/decisions: had to be hand-added to every output with a Python string-replace patch. The scaffolded or provided spec left these out, and plan.build refuses outputs without format. The agent edited YAML by regex, which is fragile and was not validated until the end. Suggested fix: have astra validate flag a missing format on executable outputs, or have the skill cover required recipe fields.

  • The run was otherwise clean. There were no sandbox denials, no source-code reverse-engineering, and materialize succeeded on the first real attempt. The friction above is the whole of it.

Full trace: agent-trace artifact on this run.

@EiffL

EiffL commented Oct 3, 2026

Copy link
Copy Markdown
Member

@claude can you show me what the output of lc materialize would look like in the 2 situations mentioned in this PR before and after the fix, grounded in the code

@claude

claude Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Claude finished @EiffL's task in 24s —— View job


Before/after output for the two situations in this PR

These are derived from the diff (origin/main...HEAD) and the render code in src/lightcone/cli/commands.py. I did not run them. Task names, the why text and the commit sha below are illustrative. The shape and flags come from the code.

One correction to the framing: situation 2 is a lc status change, not lc materialize. materialize never listed recipe-less outputs before or after the PR, because plan.build only turns outputs with a command into tasks.


1. lc materialize --check when outputs still have to be built

Assume one output would run, baseline/fit. In check mode materialize.check() puts it in report.planned.

Before (main)

MaterializeReport.ok was not failed and not blocked, so it ignored planned:

$ lc materialize --check --json
{
  "ok": true,            <-- says everything is fine
  "up_to_date": false,
  "planned": {"baseline/fit": "never materialized"},
  ...
}
$ echo $?
1                        <-- the exit code came from `check_only and not report.up_to_date`

The human output was:

  · would run baseline/fit — never materialized

! 1 output(s) would be made

The JSON said ok: true while the exit code was 1. An agent that reads only the JSON concludes it is done.

After (this PR)

ok is now not failed and not blocked and not planned. The extra exit-code clause is gone, so the exit code is simply not report.ok:

$ lc materialize --check --json
{
  "ok": false,           <-- agrees with the exit code
  "up_to_date": false,
  "planned": {"baseline/fit": "never materialized"},
  ...
}
$ echo $?
1

The human output is unchanged. _render_materialize_output now tests report.failed or report.blocked, not not report.ok. Without that change the planned-work case would have printed "did not finish":

  · would run baseline/fit — never materialized

! 1 output(s) would be made

Cases that don't change:

  • Up to date: ok: true, up_to_date: true, exit 0.
  • Real failure: ok: false, "did not finish", exit 1.
  • Normal (non-check) run: planned is empty, so ok is as before.
  • up_to_date: it is now ok and not made. That is equal to the old ok and not made and not planned, so eval.yml sees the same values.

2. lc status with a declared output that has no recipe

Assume baseline/fit is current and baseline/summary is declared with no recipe.

Before (main)

plan._tasks hit if not out.command: continue, so summary never reached the graph and was not reported at all:

  mode:    direct
  sandbox: landlock (fs: declared, network: allowed)
  crate:   up to date with the outputs

  · current baseline/fit  f394d64

1 current

--json had no entry for it, and counts was {"current": 1, "behind": 0, "stale": 0}. A spec with only recipe-less outputs printed "The analysis declares no output with a recipe."

After (this PR)

plan._tasks now records the key in Graph.no_recipe. status() appends an OutputStatus(status="no recipe", ...) for each one, and the {o.status:<9} column was widened to fit the new label:

  mode:    direct
  sandbox: landlock (fs: declared, network: allowed)
  crate:   up to date with the outputs

  · current   baseline/fit      f394d64
  · no recipe baseline/summary  —

1 current, ...

The exact summary-line wording sits in a part of status I didn't print, so I haven't reproduced it here.

--json gains an entry for baseline/summary with status: "no recipe", empty why, git_sha and data_version. counts is now {"current": 1, "behind": 0, "stale": 0, "no recipe": 1}. The empty-report message is now "The analysis declares no output."

Re-exports are excluded. The if out.reexports is None guard means a from: output is not listed. Its target output is already listed, and listing both would count one set of bytes twice.


Observations tied to your open questions

  • --check and no-recipe outputs. The check path (check() → _classified) ignores graph.no_recipe. An unwired output therefore still passes the gate, with ok: true and exit 0, exactly as the PR description says. Only status() reads no_recipe. Making --check fail on it would mean check() consuming graph.no_recipe as well.
  • Test coverage. The diff has no test for the all-recipe-less case in status that is visible in the part I read. tests/test_plan.py (+30) and tests/test_cli.py (+61) cover the rest, but I didn't re-run them.

@EiffL

EiffL commented Oct 3, 2026

Copy link
Copy Markdown
Member

@claude review this PR and see why the tests fail

@claude

claude Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Claude finished @EiffL's task in 29s —— View job


Review of #241 and why the tests fail

I read the diff and the CI logs for the failed run, and I did not re-run the suite myself.

Why CI fails

Only tests/test_container_smoke.py fails: 7 failed, 1150 passed, 1 skipped, on ubuntu 3.11 and 3.13. The ubuntu 3.12 and macOS jobs pass.

The cause is not in this PR. The image build runs uv python install <patch>, where the patch is the runner's own interpreter, and fails with:

error: No download found for request: cpython-3.13.16-linux-x86_64-gnu

(3.11.17 on the 3.11 job). The image pins ghcr.io/astral-sh/uv:0.12.5. That uv's bundled python-build-standalone index doesn't know the newest 3.11 and 3.13 patch releases that the runners picked up. 3.12 happens to still be in the index, and macOS never builds the image.

This is a time-dependent environmental failure, so main should hit it on a rerun too. It also fits the PR author's local failure on those four tests. Their message was different ("No such image"), so that part is unconfirmed.

Possible fixes, each outside this PR:

  • Bump the uv pin.
  • Make .python-version scaffolding or the image's interpreter pin tolerate a patch version the pinned uv can't download.
  • Pin the CI Python patch version.

None of the failures touch --check or status.

Review of the change

The change is correct and small. Findings, in order of importance:

  1. MaterializeReport.ok changes meaning in non-check mode too, in principle. ok now includes not planned. This is harmless today because only check() fills planned. It makes ok mean "gate passed" and not "nothing failed". Tests that used report.ok as "no failures" had to be rewritten (tests/test_materialize.py:588). Fine, but the docstring on ok could say that planned only exists in check mode.

  2. status mark colour: no recipe is yellow like stale, with a different glyph. That is fine. The {o.status:<9} width works for no recipe, which is exactly 9 characters.

  3. Re-export handling is correct but subtle. plan._tasks skips outputs where out.reexports is not None, relying on the target being listed. test_a_reexport_is_not_reported_as_missing_a_recipe covers the nested case, and it asserts the target is sub.made. A dotted id in no_recipe reaches OutputStatus without going through output_path, so the sidecar-ambiguity guard isn't hit. That is fine because it's report-only.

  4. Formatting and lint:

    • The marks dict in commands.py puts two entries per line.
    • The new test signature in tests/test_cli.py has a trailing comma after monkeypatch on the same line.

    Run uv run ruff check src/ tests/ (and the formatter if one is configured) to confirm. The line length is 100.

  5. Coverage gap: no test covers a mixed spec in the engine layer. The recipe-less output is only tested in an all-recipe-less spec (test_cli.py) and in plan. engine.status with one recipe task plus one no recipe output is not exercised, and neither is the counts tally against both. Consider adding one test_materialize.py case.

Your open question

Should --check also fail on recipe-less outputs? I lean no for this PR. --check answers "would a run change anything?", and an unwired output isn't work a run could do. It is a spec-completeness problem, which lc status now surfaces as no recipe. If the skill treats --check as the done-gate, the better fix is for the skill to also require counts["no recipe"] == 0. Changing the gate's meaning would be a separate, deliberate decision.

@cailmdaley

Copy link
Copy Markdown
Member Author

I have read the Developer Certificate of Origin and I hereby sign the DCO for this PR

Docs moved to LightconeResearch/docs (#251); the four doc edits this branch
made are carried there.
@cailmdaley

Copy link
Copy Markdown
Member Author

@EiffL i think the failing tests also fail on main, and aren't related to this PR. so I think this can be merged if you're happy with it

EiffL and others added 2 commits October 15, 2026 14:12
`ok` meant "nothing failed or was blocked" in a run, which check mode can
never violate, so `--check --json` reported `"ok": true` while exiting 1.
Redefining it as the exit code left it identical to `up_to_date` in
check mode: a second name for one fact, and the name agents misread as
"done". The report now carries only keys that say what they mean: a run
succeeded when `failed` and `blocked` are empty, a check passes when
`up_to_date` is true, and the exit code is nonzero when any of `failed`,
`blocked` or `planned` is non-empty.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@EiffL

EiffL commented Oct 15, 2026

Copy link
Copy Markdown
Member

On the open question, whether lc materialize --check should fail while a declared output has no recipe: no, decided.

  • --check predicts lc materialize. Its contract is "exit 1 if a run would do something". A run skips a recipe-less output and exits 0. A check that failed on one could never be cleared by running the verb it gates, which is the same kind of contradiction this PR removes with ok.
  • It's the same reason behind doesn't count against up_to_date. --check doesn't demand work that running materialize can't do. Here no compute could help at all; the fix belongs to whoever writes the spec.
  • A missing recipe is a legitimate state. ASTRA validates a spec whose outputs have no recipe yet, and that's the normal shape of an analysis mid-scoping. A red gate there would flag a state that isn't a fault.

The smoke-suite problem was that "--check passes" was taught as "the analysis is done". That's fixed where it was taught. "Done" now means --check passes and lc status lists no no recipe output:

Both are drafts until a CLI release includes this PR.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants