Skip to content

fix(cli): plain status, inspect and why print a successful poll's warnings, and the e2e covers a sealed engine relaunch - #1209

Merged
aviggiano merged 3 commits into
mainfrom
claude/g1-observers-greptile-fixes
Sep 29, 2026
Merged

aviggiano merged 3 commits into
mainfrom
claude/g1-observers-greptile-fixes

Conversation

@aviggiano

@aviggiano aviggiano commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Change

  1. commandFromRuntime prints a successful result's diagnostics after the rendered value. Each one prints on stdout as <severity>: <code>: <message>, and the exit status stays 0. Failures still lead with them. The duplicate appenders in commands/resume.ts and commands/init.ts are deleted. Source is +5/−9 lines. The JSON envelope is unchanged. In the docs:
    • The status section of docs/reference/cli.md says where these lines appear and that a message can span several lines. It also says that warnings do not change the exit status, so scripts should read the --json diagnostics array.
    • The deadline advice in docs/reference/configuration.md now tells cron jobs to poll ultrafuzz status <run-id> --json and act on the warnings in its diagnostics. It notes that plain status prints them on stdout.
  2. The campaign e2e (packages/cli/test/e2e/campaign-resume.test.ts) first kills only the engine. Test code only, no production change:
    • While summarize is held, it SIGKILLs only the detached engine: the process whose argv has up and not supervise.
    • With no ultrafuzz command, it waits for the relaunched engine to hold summarize again in a new process. A new heldCall helper serves both waits.
    • Then it kills the whole controller, which is now the relaunched engine and its supervisor, and resumes. So ultrafuzz resume now follows a relaunched engine, not the engine ultrafuzz run started (see Risk).
    • The final checks change to match: summarize starts 3 times, the event log has 3 RunStarted, and it must contain a RunAutoResumed. The test title now names both kills.

Deliberately not fixed

  • The relaunched engine does not finish the run. Greptile's finding asked whether a sealed launch's run finishes after a relaunch.
    • What the new phase shows: the supervisor relaunches a sealed engine, and the relaunched engine re-runs the interrupted node.
    • What it does not show: the e2e SIGKILLs that engine while it holds summarize. RunFinished comes from the engine ultrafuzz resume starts, and that engine runs the project-relative workflow file recorded in run.json, not the sealed snapshot.
    • The gap: a regression that breaks a later node, or the run's completion, only in a sealed relaunch would stay green.
    • Why not close it here: resume runs from plain paths, so one campaign cannot both let the sealed relaunch finish and resume after a mid-node crash. A second campaign would roughly double this test's time, which was 329–611 s per run on this host.
  • The held stub is not killed separately. Smithers 0.35.0 spawns its CLI agents, the stub codex included, under @smthrs/agents/src/BaseCliAgent/parentDeathWatchdog.js (runCommandEffect → parentDeathCommand). The watchdog checks the engine's pid every 100 ms and SIGKILLs the agent's process group once the engine is gone. So killing the engine kills the held stub. origin/main's existing wait for "the controller and its agent to exit" already relies on this.
  • No sealed variant of smithers-supervisor-relaunch.integration.test.ts. Draft feat(runtime)!: run lifecycle commands from the pnpm-patched install instead of per-command npm installs (#921 step 1) #1201 rewrites that file. The e2e drives the CLI black-box, so it keeps guarding the sealed path through feat(runtime)!: run lifecycle commands from the pnpm-patched install instead of per-command npm installs (#921 step 1) #1201 and the Re-evaluate sealed execution snapshots: cost/benefit after repeated campaign losses #921 rewrites.
  • Some lines now appear twice, and I left them. This is cosmetic, and fixing it would change the renderers or the envelope. I observed both cases:
    • During launch preparation, plain status prints the WORKFLOW_CONTROL_SEAL_PENDING warning, and its message is also the Reason: line.
    • A run can be terminal in Ultrafuzz while the runner is still live. In the repro's scenario C, the deadline cancel succeeds while the fake runner still reports running. Plain status then prints both Lifecycle divergence: Ultrafuzz is terminal timed-out, but the workflow runner is running; ... and warning: RUN_WORKFLOW_STATUS_DIVERGED: .... From reading the code, a poll that fails but still returns a health value (a non-transient sync error) already printed both on origin/main, diagnostics first. A follow-up could delete lifecycleDivergenceLines in commands/status.ts (about −7 lines), because the same condition now also prints the warning, and point cli.test.ts's divergence assertion at the warning line.
  • RunAutoResumed must be present, but its count is not checked. A relaunch that needs a second supervisor attempt under load still finishes the campaign, and this test is not meant to measure activation latency. RunStarted is still required to be exactly 3.

Verification

  • New CLI test "plain status prints the warnings of a successful poll" (packages/cli/test/lifecycle-commands.test.ts). It sets a past workflow_deadline_at, and the fake runner answers cancel outside its status contract.
    • On origin/main (2cacf4c) it fails with AssertionError [ERR_ASSERTION]: The input did not match the regular expression /^warning: WORKFLOW_DEADLINE_CANCEL_FAILED: /mu. The input is the plain status text, which ends at Pace: 0 finished in the last 10m with no warning line. Both reviews reproduced this failure with main's text logic in packages/cli/dist. oclif loads commands from there, so rebuilding only dist-test does not test main.
    • It passes here.
  • Existing tests, passing here after the last change:
    • all of lifecycle-commands.test.ts (13/13);
    • 9 cli.test.ts tests selected by name, most of which run plain-text commands: the product-evidence test, plain init, ps text, status launch-incomplete, status lifecycle divergence, status quota parking, resume already-active, references status and symlinked --project.
    • A review also ran all of cli.test.ts plus audit-profile-commands (88/91):
      • The 3 failures were two report bundle --require-verified tests whose ultrafuzz run --json failed with WORKFLOW_SUBMISSION_FAILED (... changed while reading) on a dependency file. That is the launch-snapshot race that x03 fixes.
      • Both tests passed when rerun alone.
  • The verifier's repro, which I re-ran against this branch's build. It drives runCli with a fake runner. Every scenario exits 0, and the JSON is unchanged. On origin/main the same script printed no warning line in any scenario.
    • A, past deadline with a failing cancel: plain status now ends with warning: WORKFLOW_DEADLINE_CANCEL_FAILED: workflow runner command failed (exit 1), followed by the message's own stderr: runner busy line. Plain why prints the same warning.
    • B, diverged snapshot workflow file: both polls print warning: WORKFLOW_CONTROL_EVIDENCE_DIVERGED: ... and warning: WORKFLOW_STATE_SYNC_SKIPPED: ..., and plain events prints the first.
    • C, the deadline cancel succeeds: plain status prints the Lifecycle divergence: line and the RUN_WORKFLOW_STATUS_DIVERGED warning described above.
  • Campaign e2e. This change is test-only and covers behaviour that already works on main, so the new check passes on main's code. Two ablations show that it discriminates.
    • Passes here in 329 s (load average about 5–13). The two reviews ran the same test, and it passed again in 373 s and 611 s. An earlier run of the same checks, instrumented to print process argv and events, also passed (469 s, load average about 15–20). In that run:
      • the relaunched engine held summarize 45 s after the engine SIGKILL;
      • that engine ran from the sealed snapshot: its argv held /proc/self/fd/3/... paths, --log-dir <run>/smithers/logs and --resume;
      • the events were RunAutoResumed ×1, RunStarted ×3, summarize NodeStarted ×3, then RunFinished;
      • every status, stats and report check passed.
    • Ablation of the relaunch root. I reduced fix(runtime): the Smithers supervisor can relaunch a crashed engine #1199's supervisor_resume_root patch to upstream behaviour (return direct;) and rebuilt the runtime.
    • Sealed-only ablation, run by a review. It broke only the inherited-descriptor branch of fix(runtime): the Smithers supervisor can relaunch a crashed engine #1199's resume_snapshot_transfer patch, by pointing a sealed relaunch at a missing --config=/proc/self/fd/3/controls/ablated-bunfig.toml. The plain-path relaunch was left alone.
      • The new e2e fails with the workflow stopped while waiting for the relaunched engine to rerun summarize. The review reports that the supervisor log showed ENOENT for that config on attempts 1–3, after which the supervisor gave up.
      • origin/main's e2e passes under the same ablation (536 s). This is the gap the finding describes.
  • Gates, all passing after the last change:
    • npx prettier --check and npx eslint on the changed files;
    • CI=1 ESLINT_PLUGIN_DIFF_COMMIT=origin/main pnpm -w lint:strict:ci. It also passes with the merge base as the diff commit, which is what CI's pull-request diff sees; origin/main has two later commits that touch no file here;
    • pnpm -w lint;
    • pnpm --filter @ultrafuzz/cli typecheck;
    • pnpm -w knip;
    • pnpm -w docs:check, which includes node scripts/docs-check.mjs.

Risk / compatibility

Changelog entry

Plain-text ultrafuzz status (including each --watch poll), inspect, why, events, node, timeline, snapshots, run, ps, pause, cancel, replay, fork, clean, materialize, validate, doctor and references status|sync|update now print the warnings of a successful result on stdout after their output, as init and resume already did. Examples are WORKFLOW_DEADLINE_CANCEL_FAILED and WORKFLOW_STATE_SYNC_SKIPPED, which before only --json showed; the exit status is unchanged. The end-to-end campaign test now also checks that the supervisor relaunches a SIGKILLed engine of a fresh run, and that the relaunched engine re-runs the interrupted node.

Greptile follow-up

  • Kept as is: diagnostics are appended to a successful result's text without a separator (comment). Every renderer that reaches commandFromRuntime ends its text with a newline, so the output is correct. A separator helper would guard only a hypothetical renderer.

🤖 Generated with Claude Code

aviggiano and others added 3 commits September 29, 2026 12:30
…ssful poll

commandFromRuntime rendered a result's diagnostics only when the result
failed. getRunHealth returns ok unless a diagnostic is an error, and the
sync and deadline problems it reports are warnings
(WORKFLOW_DEADLINE_CANCEL_FAILED, WORKFLOW_STATE_SYNC_SKIPPED,
WORKFLOW_CONTROL_EVIDENCE_DIVERGED and the transient sync codes), so plain
`ultrafuzz status` exited 0 without them. inspect and why render through
the same helper. docs/reference/configuration.md tells operators to run
`ultrafuzz status` from cron and act on its warnings.

A successful result now prints its diagnostics after the rendered value,
and a failure still leads with them. resume and init appended successful
diagnostics themselves; those copies are gone. JSON output is unchanged.
The status reference now says where the warnings appear.

The new CLI test sets a past workflow_deadline_at and has the fake
runner answer cancel outside its status contract. On origin/main plain
status prints no warning line; with this change it prints
`warning: WORKFLOW_DEADLINE_CANCEL_FAILED: ...`.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
…aled engine re-runs the interrupted node

No test checked that a fresh `ultrafuzz run`, whose engine runs from the
sealed execution snapshot, recovers when only its engine dies. The
real-engine relaunch test runs a plain-path engine, the fd-transfer
harness runs a toy script, and this e2e killed the engine together with
its supervisor, then recovered through `ultrafuzz resume`.

The e2e now first SIGKILLs only the detached engine (the `up` process,
not `supervise`) while summarize is held, and waits, with no ultrafuzz
command, for the relaunched engine to hold summarize again in a new
process. It then kills the whole controller, which is now the relaunched
engine and its supervisor, and resumes, so summarize starts three times
and the event log has three RunStarted and a RunAutoResumed.

The relaunched engine does not finish the run: it is killed while it
holds summarize, and the engine `ultrafuzz resume` starts finishes it.
The resume phase therefore starts from a supervisor-relaunched engine
instead of the one `ultrafuzz run` started. Resume takes the workflow
path from Ultrafuzz's run metadata, so what differs is the state the
relaunched engine wrote to the Smithers database.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
…agnostics

A successful status poll prints its warnings on stdout without changing
the exit status, and a message can continue on lines without a severity
prefix. A cron job that discards stdout, or greps for `warning:` lines,
can therefore miss them or see only the first line of each.

The status reference now says so, and the deadline advice in the
configuration reference points scripts at the `diagnostics` of
`ultrafuzz status <run-id> --json`.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@aviggiano
aviggiano requested a review from a team as a code owner September 29, 2026 14:39
text: result.value
? `${result.ok ? "" : diagnosticsText(result.diagnostics)}${text(result.value)}`
? result.ok
? `${text(result.value)}${diagnosticsText(result.diagnostics)}`

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Warnings depend on renderer newlines The helper appends diagnostics without adding a separator. All current renderers end with a newline, so output is correct today, but a future renderer that does not would attach warning: to its final line and make the warning harder to recognize. Adding the separator here would keep that formatting requirement in one place.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli/src/command-shared.ts
Line: 137

Comment:
**Warnings depend on renderer newlines** The helper appends diagnostics without adding a separator. All current renderers end with a newline, so output is correct today, but a future renderer that does not would attach `warning:` to its final line and make the warning harder to recognize. Adding the separator here would keep that formatting requirement in one place.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code

@aviggiano
aviggiano merged commit 94c0fc6 into main Sep 29, 2026
28 of 30 checks passed
@aviggiano
aviggiano deleted the claude/g1-observers-greptile-fixes branch September 29, 2026 15:54
aviggiano added a commit that referenced this pull request Sep 29, 2026
…ptor once (#1215)

`packages/modal/test/deterministic-archive.test.ts` › "rejects a caller-visible output parent replaced after descriptor normalization" fails intermittently. It failed in #1209's package gates lane:
```
AssertionError: expected [Function] to throw error including 'deterministic archive output changed while it was written' but got 'EBADF: bad file descriptor, close'
```

Co-Authored-By: Claude Opus 5.5 <[email protected]>
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.

1 participant