Skip to content

Accept a node that cannot name its illuminator - #385

Merged
Babissimo merged 1 commit into
mainfrom
feat/nodes-nullable-tx-callsign
Sep 15, 2026
Merged

Babissimo merged 1 commit into
mainfrom
feat/nodes-nullable-tx-callsign

Conversation

@Babissimo

@Babissimo Babissimo commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

tx_callsign was the last required field of a node's configuration with no way to say "unknown", so both producers invent one: retina-gui substitutes the literal 'Tower' when the tower it selected has no callsign (setup.js:1139), and retina-simulation's tower resolver falls back to an empty string. Both are the objection 1.1.3 made of a substituted coordinate: the server cannot later tell the placeholder apart from a name an owner gave it.

The field becomes nullable on the coordinates' reasoning and by the same route.

  • Validator — a _NULLABLE_CALLSIGN set folded into the published schema's nullable union, and None let through the check in validate_config.
  • Columnnode_configs.tx_callsign becomes nullable, via 0009_nullable_tx_callsign. Graded destructive for 0005's reason: code predating this revision has no null-handling for the column, so a rollback across it must reach a human rather than be served as safe. Existing rows keep the names they declared.
  • ContractNODE_API_VERSION 1.2.1 → 1.2.2 and contracts/nodes-v1.openapi.yaml regenerated in the same commit. A patch on the test 1.1.3 applied to this same change on the coordinates: the document gains no field a client can read, and what a client generating from it sees is a widened type on a field it already had.

The empty string stays refused. Null is the one spelling of "unknown", which is what keeps a stored name distinguishable from its absence, and since the validator has always refused "" there is no legacy sentinel to grandfather the way (0, 0) had to be.

Scope

Nothing in this repo reads the callsign: it is absent from node_pipeline's _WIRE_FIELDS, no lib under libs/ reads it, and no frontend or dashboard file references it. So no consumer gains null-handling here, and the change is confined to the door, the column and the document.

Each producer needs its own change before it stops inventing a name. Out of scope here; the retina-simulation half is tracked in 123zgec2zcz.

Verification

  • pytest tests/ (minus test_mlat_history.py): 3516 passed. test_migrations_produce_the_schema_create_all_produces compares notnull per column, so it is what proves the migration and the model agree.
  • scripts.generate_openapi --check: current.
  • pre-commit run --all-files: clean.

🤖 Generated with Claude Code

@claude

This comment has been minimized.

`tx_callsign` was the last required field of a node's configuration with no way
to say "unknown", which left its producers inventing one: retina-gui substitutes
the literal 'Tower' when the tower it selected has no callsign, and
retina-simulation's tower resolver falls back to an empty string. Both are the
objection 1.1.3 made of a substituted coordinate, that the server cannot later
tell the placeholder apart from a name an owner gave it.

So the field becomes nullable, on the coordinates' reasoning and by the same
route: the validator, the column, and the published schema, which the contract
is regenerated from in this commit.

The empty string stays refused. Null is the one spelling of "unknown", which is
what keeps a stored name distinguishable from its absence, and since the
validator has always refused "" there is no legacy sentinel to grandfather the
way (0, 0) had to be.

A patch rather than a minor bump, on the test 1.1.3 applied to this same change
on the coordinates: the document gains no field a client can read, and what a
client generating from it sees is a widened type on a field it already had.

Nothing in this repo reads the callsign, so no consumer gains null-handling.
The producers need their own changes before either stops inventing a name, and
those are out of scope here.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Babissimo
Babissimo force-pushed the feat/nodes-nullable-tx-callsign branch from 2be4eba to 467e0f4 Compare September 15, 2026 11:42
@claude

claude Bot commented Sep 15, 2026

Copy link
Copy Markdown

Claude finished @Babissimo's task in 1m 53s —— View job


Reviewing PR #385

  • Gather context (CLAUDE.md, ONBOARDING.md, diff)
  • Review validator changes (backend/services/node_config.py)
  • Review migration (backend/migrations/versions/0009_nullable_tx_callsign.py)
  • Review model/column change (backend/core/nodes.py)
  • Review route changes (backend/routes/nodes.py)
  • Review contract regeneration (contracts/nodes-v1.openapi.yaml)
  • Review tests
  • Post final review feedback

Review

Clean, well-scoped change that mirrors the precedent set for the coordinate fields in migration 0005/_NULLABLE, and every seam that precedent touches is updated consistently.

  • Validator (backend/services/node_config.py:83-88, 234-242) — _NULLABLE_CALLSIGN folds into the published schema's nullable union (config_json_schema, line 135), and validate_config passes None through while still refusing "" and any other non-string. Traced the ordering by hand: the missing check at line 218-220 runs before payload["tx_callsign"] is read at line 236, so a payload that omits the key entirely still raises ConfigInvalid("tx_callsign", "missing") rather than KeyError — nullable and optional stay distinct, as intended. The new _typed_branch/string_branch helper (line 153-173) generalizes numeric_branch correctly: PUBLISHED_NUMERIC_FIELDS in test_node_config_validation.py still excludes tx_callsign since its anyOf branches are string/null, and numeric_branch on that schema falls through to published.get("type") being None, not "number".
  • Column/migrationNodeConfig.tx_callsign becomes str | None (backend/core/nodes.py:105), and 0009_nullable_tx_callsign.py follows 0005's exact shape: batch_alter_table for SQLite, rollback_safety = "destructive" with the matching justification, and a downgrade() that fails loudly instead of inventing a name. Confirmed only one migration file claims down_revision = "0008", so the chain (00080009) has no branch.
  • ContractNODE_API_VERSION 1.2.1 → 1.2.2 (patch, consistent with the coordinates' reasoning: no new field, just a type widening via anyOf: [string, null] on tx_callsign), and contracts/nodes-v1.openapi.yaml reflects it exactly along with the schema description addition.
  • Tests — endpoint, store, validation and OpenAPI tests each get a null-callsign case; test_a_non_string_callsign_is_rejected correctly drops None from its rejected-values parametrization now that it's accepted, "" stays refused with a comment explaining why, and test_the_published_callsign_length_is_where_the_validator_refuses is updated to read through string_branch now that the schema is an anyOf.

Scope claim re-verified: grepped for tx_callsign outside tests — it only appears in node_config.py, node_config_store.py (a plain field-name list used for equality-based diffing, so None == None round-trips with no special-casing needed), core/nodes.py, routes/nodes.py, vulture_whitelist.py, and the migrations (0002 original NOT NULL, 0009 here). Nothing in node_pipeline, libs/, or any frontend file references it, matching the PR description.

No bugs, security issues, or inconsistencies found. I don't have a working Python venv in this environment to run pytest/generate_openapi --check (backend/.venv isn't present), so I traced the validator's control flow by hand instead of executing it; the logic checks out and is internally consistent with the schema and contract. The PR description states the full suite (3516 passed), the OpenAPI check, and pre-commit run --all-files were run and passed.

@Babissimo
Babissimo merged commit b114652 into main Sep 15, 2026
39 of 40 checks passed
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