Skip to content

Lint: reject a manifest naming an engine that is not configured - #105

Open
dudarenok-maker wants to merge 1 commit into
NateBJones-Projects:mainfrom
dudarenok-maker:fix/lint-reject-unconfigured-engine
Open

Lint: reject a manifest naming an engine that is not configured#105
dudarenok-maker wants to merge 1 commit into
NateBJones-Projects:mainfrom
dudarenok-maker:fix/lint-reject-unconfigured-engine

Conversation

@dudarenok-maker

Copy link
Copy Markdown

Burned a whole dispatch discovering a typo in an engine name. lint said
clean, run then failed at spawn time — after the run row, the worktree and
the dashboard entry already existed. The manifest was generated by a tool of
mine, so the same wrong name would have come back every time until I read the
spawn error closely enough to recognise it as a name problem rather than a
missing binary.

lint is the cheap gate that exists to catch exactly this before a dispatch is
paid for, and on this input it certified nothing:

$ ./ringer.py lint probe.json          # main @ a1a91b8
lint: clean (1 tasks)
$ echo $?
0

That manifest names no-such-engine-xyz. It cannot run.

Two holes, and the second is what made the first invisible

1. lint_manifest never resolved task.engine against the config. The only
code that touches the name is noncanonical_route_findings, and an unknown name
takes the quiet path out of it — config.engines.get(task.engine) returns
None, model_key falls through to identity_registry.defaults.get(...)"",
noncanonical_routes.get((engine, ""))Nonecontinue (ringer.py
6376-6382). No finding, by construction.

2. The lint CLI branch returned before the shared AppConfig.load. The
branch ends at 11090/11092; the shared load is at 11097. So lint_manifest was
always called with config=None — the parameter was already in its signature
and was simply never fed from the CLI. A correct check added in (1) alone would
still have found nothing.

That second hole also silently degraded a check that already shipped: with
config=None, noncanonical_route_findings falls back to the identity
registry's defaults instead of the engine's real model_default, for every task
that does not name a model explicitly.

What this does

  • Resolves each task's engine against config.engines, and reports an unknown
    one naming the engines that are available — so the finding is diagnosable on
    its own, without a second round-trip to the config.
  • Loads the config in the lint branch, via the same suppressed --config
    argument run, hud, db, models and demo already take (the main parser
    supplies the default; argparse.SUPPRESS on the subparser keeps it from being
    clobbered). No new flag surface.
  • If the config cannot be loaded, says so as the first finding rather than
    degrading quietly — a bare clean would otherwise claim more than was
    actually checked.

Same manifest, with this change:

$ ./ringer.py lint probe.json
lint: ERROR: one: engine 'no-such-engine-xyz' is not configured; engines available here: claude, cline, codex, copilot.
$ echo $?
1

On "one fix per PR" — this has an and, deliberately

The check and the config wiring cannot be split into two mergeable PRs: the
check without the wiring is dead code that can never fire, and the wiring
without the check has no observable effect on its own. They are one defect —
lint runs configless — seen from two ends. Splitting would mean landing a
provably vacuous guard first, which is the failure mode the second half exists
to prevent.

Proof

tests/test_lint.py gains five tests, each guard exercised in both
directions, because a guard only shown to block is indistinguishable from one
welded shut:

Test Pins
test_unknown_engine_is_reported the finding fires and names what is available
test_configured_engine_is_not_reported a valid engine is not flagged
test_check_is_vacuous_without_config documents why the wiring is required — with config=None the check cannot fire at all
test_cli_lint_loads_the_config the wiring itself: fails if the CLI goes back to config=None
test_cli_lint_stays_clean_on_a_configured_engine the CLI path exits 0 on a good manifest

Mutation-proved in both halves: neutering the check reddens two tests, dropping
the config load reddens one, restoring both goes green. The before/after above
is the real CLI against a real config, not a fixture.

Notes for review

  • Rebased on a1a91b8, current main.
  • Overlap with Lint: reject a check no shell can parse #95. That PR touches the same two files for a different
    concern (check parseability vs. engine identity). A textual conflict in
    lint_manifest's findings block and in test_lint.py's import line is
    likely. Happy to rebase whichever lands second — no objection to Lint: reject a check no shell can parse #95 going
    first.
  • Related to Warn at config load when an engine bin is not resolvable — PATH problems currently surface mid-run #43 ("warn at config load when an engine bin is not
    resolvable") but at a different layer: Warn at config load when an engine bin is not resolvable — PATH problems currently surface mid-run #43 is a configured engine whose binary
    will not resolve, this is a name that is not a configured engine at all. They
    are complementary; neither catches the other's case.
  • Platform honesty. Developed and run on Windows. tests/test_lint.py has
    two failures on this box — test_verifier_expands_user_expect_files and
    test_w6_write_collision — which reproduce identically on pristine
    a1a91b8
    (14 tests / 2 failures before, 19 tests / the same 2 after), so
    they are pre-existing and path-shaped rather than anything this change did. I
    make no POSIX claim beyond what the macOS and Linux jobs prove; the new tests
    are stdlib-only, spawn no shell, and call main() in-process with
    RINGER_NO_SELF_UPDATE=1 set.
  • Scope is one concern, two files, additions only. It inspects the manifest the
    operator wrote, not the worker — squarely "verify outputs", not confinement.

lint called a manifest naming a nonexistent engine clean, so the error
cost a whole dispatch to discover: run then failed at spawn time, once
the run row, the worktree and the dashboard entry already existed.

Two holes, and the second is what made the first invisible.

lint_manifest never resolved task.engine against the config. The only
code that touches the name is noncanonical_route_findings, and an
unknown name takes the quiet path out of it: config.engines.get returns
None, model_key falls through to the identity registry's defaults, the
route lookup misses and it continues. No finding, by construction.

The lint CLI branch also returned before the shared AppConfig.load, so
lint_manifest was always called with config=None - the parameter was
already in its signature and simply never fed. A correct check alone
would still have found nothing. That hole also degraded a check that
already shipped: with config=None, noncanonical_route_findings falls
back to the identity registry's defaults instead of the engine's real
model_default for every task that does not name a model.

Resolve each task's engine against config.engines and name the engines
that are available, so the finding is diagnosable on its own. Load the
config in the lint branch via the same suppressed --config argument run,
hud, db, models and demo already take. If the config cannot be loaded,
say so as the first finding rather than degrade quietly - a bare clean
would claim more than was actually checked.

Five tests, each guard exercised in both directions, because a guard
only shown to block is indistinguishable from one welded shut, plus a
wiring test that fails if the CLI goes back to config=None.

Co-Authored-By: Claude <noreply@anthropic.com>
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