Skip to content

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

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

cailmdaley wants to merge 4 commits into
mainfrom
smoke-findings

Conversation

@cailmdaley

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 now false whenever check mode finds planned work, so the JSON and the exit code agree, and --help states the contract. A check with planned work still prints "would be made" rather than "did not finish". Normal (non-check) runs are unchanged, and up_to_date keeps its value in every case, which matters because eval.yml reads it.

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 7
Tool calls 5
Cost $0.12
Agent wall time 0m44s
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        965f194
  · current   baseline/hubble_diagram  965f194
  · current   baseline/residuals       965f194

3 current
Confusion & pain points (Claude analysis)

Confusion & pain points

  • The run was essentially clean, with little friction. There were no errored tool calls. The agent went from spec to validated, materialized outputs in 5 calls, and astra validate, lc materialize --check and the cluster teardown all passed. The points below are the only friction.

  • The license warning cost a second pass. The first lc materialize local ended with ! no [project].license …, so no crate was written. The agent then added license = "CC-BY-4.0" with a sed edit, committed it, and re-ran materialize. The cause is that the scaffold deliberately omits a license, so publication intent is derived from its presence. This is by design. The task prompt or the astra skill could say that a crate needs [project].license, which would avoid the extra commit and run. The sed anchor ^version = "0.0.1" is also fragile, since it depends on the scaffold's exact version string.

  • Dependency setup was done blind. The agent ran uv add numpy scipy matplotlib >/dev/null 2>&1 and only echoed the exit code (0). That is not a product gap, but it hides any resolution warning, including the tool_warnings() output about uv cache and filesystem mismatches. If lc surfaced the lock and environment state directly, the agent would not need to trust a silent exit code.

  • The agent wrote the spec by string replacement. A Python s.replace(...) script patched astra.yaml. It matched on the exact text of the scaffold's recipe: command: python scripts/... stubs, and it had no way to confirm the spec's shape before writing. It worked here, but it would break silently if the skeleton text changed. An astra verb or documented pattern for filling in format:, inputs: and decisions: on existing outputs would be sturdier. The agent also had to infer the recipe placeholder grammar ({inputs.X}, {decisions.X}, {output}) from the skill and the existing spec alone.

  • The cluster lifecycle is a manual step. The agent had to chain lc compute launch --wait && lc materialize local, then remember lc compute down local at the end. Materialize requires an explicit cluster name by design. A local run therefore costs three verbs and an easy-to-forget cleanup, although the 30-minute idle timeout bounds the damage if it is forgotten. The guide or skill should show this three-step sequence for local runs.

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.

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