ENG-815 - Three collateral smells from PR #82's blind startswith-sweep

[druks-operator]

Linear ticket: ENG-815

Plan

Implementation plan

1. In backend/druks/harnesses/subprocess.py, restore the two purpose-built prefix checks in read_result_json() verbatim to text.startswith("```") and lines[0].startswith("```"). Leave closing-fence stripping, JSON parsing, and HarnessInvalidOutputError behavior untouched.

2. In backend/druks/harnesses/registry.py, remove the module-level from druks.user_settings.models import HarnessSettings dependency. Import HarnessSettings inside get_harness_for_model() immediately before the settings query, preserving the current iteration over HarnessSettings.all(), bare-harness-name match, allowed_models ID match, returned row.harness, and exact miss error. This is the narrow cycle seam already used by HarnessSettings.harness, which resolves back through druks.harnesses.registry.

3. In backend/druks/user_settings/routes.py, consolidate the two invalid-model branches in update_harness_settings(). Keep resolution within one try block, translate a resolved harness whose name differs from the requested harness into HarnessError, and let one except HarnessError raise the existing HTTP 422 with the exact detail f"{updates['model']!r} is not a {harness.name} model." and exception chaining. Do not alter successful updates or effort/timeout validation.

4. In backend/tests/test_harness_model_routing.py, expand the existing harness-update rejection test to exercise both failure sources—another harness's registered model and an unregistered model—and assert the exact 422 detail for each. Retain the existing routing coverage for shipped lists, fetched lists, bare harness names, and misses.

The preserved HTTP behavior is:

PATCH /api/settings/harnesses/claude
Content-Type: application/json

{"model":"gpt-5.5"}

{"detail":"'gpt-5.5' is not a claude model."}

and an unknown model such as {"model":"llama-3-70b"} returns status 422 with {"detail":"'llama-3-70b' is not a claude model."}.

Out of scope: changing model-routing semantics, fetched or shipped model data, endpoint schemas, status codes, user-facing error text, or the repository's named author-surface boundary list.

Acceptance criteria

AC1

Description: Both opening-fence checks in backend/druks/harnesses/subprocess.py use .startswith("```"); neither uses a three-character slice comparison.

Verification: Inspect read_result_json: the outer text check and first-line check both call .startswith("```").

AC2

Description: backend/druks/harnesses/registry.py has no module-level import from druks.user_settings; get_harness_for_model() imports HarnessSettings at function scope and retains the existing fetched-or-shipped model-list lookup and HarnessError miss behavior.

Verification: Inspect the module imports and get_harness_for_model, then read the existing routing tests covering shipped models, fetched models, bare harness names, and unknown models.

AC3

Description: Model validation in update_harness_settings() contains one HTTPException raise for both an unknown model and a model belonging to another harness, preserving status 422 and the exact detail template {model!r} is not a {harness.name} model.

Verification: Inspect the if "model" in updates branch and confirm focused tests assert exact status and detail for both failure sources.

AC4

Description: backend/tests/test_harness_model_routing.py covers both invalid update cases: gpt-5.5 when updating claude, and an unregistered model such as llama-3-70b; each produces the corresponding exact 422 detail without updating settings.

Verification: Read the parametrized or equivalent route-level tests and their exact HTTPException.status_code and detail assertions.

Ruled out

• Add druks.harnesses to the named architectural boundary rule while retaining the module-level user_settings import: this would legitimize the cross-concern import instead of removing it and would leave the registry coupled to model initialization at import time.
• Move model resolution out of the harness registry into a new user-settings service or model API: that would require changing established call sites and ownership for a three-line dependency seam, increasing regression surface without changing routing behavior.
• Collapse the route branches by storing None or a boolean after catching HarnessError: this would turn a typed resolution failure into sentinel state and discard the original exception cause.
• Add tests that parse production source to enforce .startswith or import placement: those tests would pin implementation syntax rather than behavior; these two code-shape requirements are directly inspectable acceptance criteria.

Acceptance Criteria

• AC1: Both opening-fence checks in backend/druks/harnesses/subprocess.py use .startswith("```"); neither uses a three-character slice comparison.
  • Verification: Inspect read_result_json: the outer text check and first-line check both call .startswith("```").
• AC2: backend/druks/harnesses/registry.py has no module-level import from druks.user_settings; get_harness_for_model() imports HarnessSettings at function scope and retains the existing fetched-or-shipped model-list lookup and HarnessError miss behavior.
  • Verification: Inspect the module imports and get_harness_for_model, then read the existing routing tests covering shipped models, fetched models, bare harness names, and unknown models.
• AC3: Model validation in update_harness_settings() contains one HTTPException raise for both an unknown model and a model belonging to another harness, preserving status 422 and the exact detail template {model!r} is not a {harness.name} model.
  • Verification: Inspect the if "model" in updates branch and confirm focused tests assert exact status and detail for both failure sources.
• AC4: backend/tests/test_harness_model_routing.py covers both invalid update cases: gpt-5.5 when updating claude, and an unregistered model such as llama-3-70b; each produces the corresponding exact 422 detail without updating settings.
  • Verification: Read the parametrized or equivalent route-level tests and their exact HTTPException.status_code and detail assertions.

[druks-reviewer]

Verdict: approve

All four acceptance criteria are satisfied by 500d0dd, and the change stays inside the stated behavior-preserving scope.

AC1 — backtick checks (backend/druks/harnesses/subprocess.py:19,21). Both opening-fence checks are .startswith("```") again; no slice comparison remains in the file. Closing-fence stripping, JSON parsing, and HarnessInvalidOutputError are untouched.

AC2 — registry import seam (backend/druks/harnesses/registry.py). The module-level from druks.user_settings.models import HarnessSettings is gone; the import now sits at function scope inside get_harness_for_model() immediately before the HarnessSettings.all() loop. Bare-name match, allowed_models ID match, row.harness return, and the loud HarnessError miss are all preserved. test_shipped_tuple_fallback_routes_shipped_models, test_fetched_list_routes_provider_models, test_bare_harness_name_routes, and test_unknown_model_raises_harness_error still cover the four routing paths and pass.

AC3 — single raise (backend/druks/user_settings/routes.py:71-81). Resolution and the harness-mismatch check now live in one try; the mismatch translates into HarnessError and a single except HarnessError raises HTTP 422 with the exact detail f"{updates['model']!r} is not a {harness.name} model." and from exc. Effort/timeout validation and the success path are unchanged.

AC4 — test coverage (backend/tests/test_harness_model_routing.py:47-68). test_settings_reject_invalid_model_returns_422 is parametrized over gpt-5.5 (another harness's registered model) and llama-3-70b (unregistered), asserting exact status 422 and the exact detail string for each, plus settings.model == original_model so a rejected update leaves the row alone. The _validate_model test for the distinct "No installed harness runs model ..." detail is retained.

Verification

• uv run ruff check backend — pass (local: "All checks passed!").
• uv run pytest backend/ — pass, 1116 passed (local, against deploy/compose.dev.yaml Postgres + Redis).
• uv pip install -e backend/tests/druks-field_notes + the three proof-extension test files — pass, 5 passed.
• GitHub check checks — success on 500d0dd.
• npm --prefix frontend run lint / run build / test — not run: no Node toolchain in this sandbox (npm: command not found), and no frontend check ran on this commit because the diff is backend-only and the frontend workflow's path filters exclude it. Not a blocker for a backend-only change.

Open findings

• backend/druks/harnesses/routes.py:14 and backend/druks/harnesses/models.py:14 still import druks.user_settings at module level. Pre-existing, outside this ticket's scope (ENG-815 names only registry.py), and untouched by this diff — noting it because the layering concern the ticket raises applies there too, and it may be worth a follow-up that either moves those seams or adds druks.harnesses to the boundary rule's named list. No change requested here.
• raise HarnessError inside the try makes the synthetic exception the __cause__ rather than the original miss, which slightly shortens the traceback for the unregistered-model case. This is exactly what the plan specified ("translate a resolved harness whose name differs from the requested harness into HarnessError"), so it is intended, not a defect.

If you read either open finding differently — particularly if you think the __cause__ shape deserves a distinct sentinel subclass — push back; neither is blocking and I'd rather hear the counter-argument than have you work around a note.

[druks-operator]

Code review: Clean, tightly-scoped fix for all three ENG-815 smells — startswith restored in subprocess.py, the harnesses/user_settings import cycle broken with a function-scoped import (consistent with the existing precedent in HarnessSettings.harness), and the duplicate 422 branches in routes.py collapsed into one, with a new parametrized test covering both rejection paths and asserting the settings row is left untouched on failure.