Skip to content

Accept Auto-Calibrate outcomes from the nodes at POST /api/feedback/tower-outcome - #32

Merged
jehanazad merged 1 commit into
mainfrom
feat/node-feedback-ingest
Sep 13, 2026
Merged

jehanazad merged 1 commit into
mainfrom
feat/node-feedback-ingest

Conversation

@jehanazad

Copy link
Copy Markdown
Contributor

Stands up the towers API's side of the calibration feedback loop so the nodes can start reporting. This is the fleet-feedback ingest from #30, split out so it can merge and accumulate rows while the ranking change in #30 is reviewed. #30 will need a rebase onto this once it lands; the ranking is what consumes the store.

What the nodes post

POST /api/feedback/tower-outcome, bearer TOWER_FINDER_FEEDBACK_TOKEN, one row or up to 100. The row shape was checked against retina-gui's calibrator (src/calibrator.py) rather than designed in the abstract: each row flattens one entry of a run's history (tower_namecallsign, fcfc_hz, final_gain_*gain_*, dwell_secondsduration_s), the outcome vocabulary is the calibrator's verbatim, callsign uses the node's own 32-character limit, and a not_reached entry with no final gains validates. The README carries the field-by-field mapping for the retina-gui follow-up.

Two additions over #30's shape

  • run_id with a partial unique index on (node, run, tower). A node that times out on the post and retries would otherwise land its run twice, doubling that run's weight in the correction. A retry now answers 200 {"stored": 0, "ignored": n}, which tells the node to stop. Rows without a run_id (archive job, older nodes) are never deduplicated.
  • device_error, which the calibrator sets when the SDR wedged rather than reported a clean overload. Kept apart from outcome so a later fit can weight them differently.

Unchanged from #30

Own secret, fail-closed, not interchangeable with the admin token; extra="forbid" so a misspelled field is a 422 rather than a weightless row; SQLite store on the runtime volume capped at 200k rows; GET /api/feedback/summary for admins; apply_feedback ships but nothing calls it yet.

Verification

  • 441 backend tests (58 for feedback, 13 new for the node-shaped row and idempotent retries), ruff check and format clean.
  • Live against a local server: no token 401, admin token 401, node-shaped row stored, retry {"stored": 0, "ignored": 1}, misspelled field 422, summary 401 on the feedback token and 200 on the admin token, unknown /api/feedback/* path 404 rather than the SPA.

Deploy

Set TOWER_FINDER_FEEDBACK_TOKEN in backend/.env on each droplet (documented in the bootstrap step). The same value goes to retina-node's .env when retina-gui grows the posting side. Nodes hold no tower-finder credential today, so that distribution is new.

🤖 Generated with Claude Code

@claude

claude Bot commented Sep 11, 2026

Copy link
Copy Markdown

Claude finished @jehanazad's task in 2m 20s —— View job


Code review

  • Gather context (PR body, changed files list)
  • Read repo CLAUDE.md for repo-specific guidelines (none present)
  • Review changed files for correctness, security, style
  • Post review feedback

No CLAUDE.md exists in this repo, so no repo-specific conventions to check beyond what's visible in the code itself.

Summary

This is a careful, well-tested piece of work. Auth (backend/core/auth.py), validation (backend/models/feedback.py), storage (backend/services/tower_feedback.py), and routing (backend/routes/feedback.py) all hang together cleanly, and the two additions over #30 (the run_id partial-unique-index dedupe and device_error) are implemented exactly as described. I traced the SQL parameter binding (_COLUMNS + received_at/observed_at/tower_key/rx_cell ordering in record_many, services/tower_feedback.py:230-249) and it lines up correctly; everything is parameterized, so no injection risk. hmac.compare_digest is used correctly for both token guards. Fail-closed-when-unset is tested for both tokens and for token interchangeability in both directions.

I don't have anything blocking. Two minor, non-blocking observations:

  1. backend/models/feedback.py:111-115 — the model validator only checks one direction: a source="calibration" row can't use outcome="observed", but a source="archive" row is free to use a calibration-only outcome (e.g. "confirmed_track"). It's harmless today since _residual() in tower_feedback.py:283-302 branches on source first and never reads outcome for archive rows, but it does mean the schema accepts a combination that shouldn't exist per the documented vocabulary split. Could tighten with a symmetric check if you want the schema to fully match the two-producer contract described in the module docstring.
  2. services/tower_feedback.py:140-144_LOCK is a threading.Lock, which only serializes writers within one process. This is fine under the stated single-worker deployment assumption (called out in the route docstring), but if this service is ever run with multiple Uvicorn/Gunicorn workers, concurrent writers would hit SQLite's own locking instead (busy errors under timeout=5.0) rather than being serialized by _LOCK. Worth a one-line note near the deploy docs if that assumption could change.

Verified independently while reading:

  • The INSERT OR IGNORE + partial unique index dedupe logic is correct, including the "partial retry" and "same run, different node" cases (test_a_run_is_stored_once_per_tower, test_feedback_store.py:122-134).
  • executemany(...).rowcount does correctly accumulate stored-row count across the batch for SQLite (confirmed against CPython's sqlite3 implementation), so {"stored": n, "ignored": m} is accurate.
  • The generic SPA catch-all 404 for unknown /api/* paths (app.py:74-92) is prefix-generic, not feedback-specific, so /api/feedback/* 404s are already covered by the existing test_app_surface.py catch-all tests — no gap there despite it only being called out as manually verified in the PR body.
  • apply_feedback is genuinely unused elsewhere in the tree, matching the PR's claim that nothing calls it yet.

Nice touches worth calling out: the -0.0 normalization in tower_key (_norm, tower_feedback.py:150-154), the antimeridian/pole handling in _bounding_box, and the test_import_creates_nothing subprocess test guarding against accidental file creation at import time.

…ower-outcome

The fleet feedback ingest from the expected-area ranking branch, on its own so
it can merge and start collecting rows while that ranking is reviewed. The
ranking does not read the store yet; apply_feedback ships here and the ranking
branch wires it in.

Checked against retina-gui's calibrator rather than designed in the abstract:
a row is a flattening of one entry of a run's history, the outcome vocabulary
is the calibrator's verbatim, callsign's limit is the node's own
TX_NAME_MAX_LENGTH, and a not_reached entry (no final gains) validates.

Two additions over the ranking branch's shape:

- run_id, and a partial unique index on (node_id, run_id, tower_key). A node
  that times out on the post and retries would otherwise land its run twice,
  and every duplicate doubles that run's weight. Retries now answer 200 with
  the rows under `ignored`, so the node knows to stop.
- device_error, which the calibrator sets when the SDR wedged rather than
  reported a clean overload; kept apart from outcome so a later fit can weight
  the two differently.

Own bearer secret (TOWER_FINDER_FEEDBACK_TOKEN), fail-closed like the admin
one and deliberately not interchangeable with it: every node holds this one.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@jehanazad
jehanazad force-pushed the feat/node-feedback-ingest branch from 9551803 to a049703 Compare September 13, 2026 20:08
@claude

claude Bot commented Sep 13, 2026

Copy link
Copy Markdown

Claude finished @jehanazad's task in 3m 7s —— View job


I'll analyze this and get back to you.

@jehanazad
jehanazad merged commit 04afd6b into main Sep 13, 2026
7 checks passed
jehanazad added a commit that referenced this pull request Sep 13, 2026
Reconciles the ranking redesign with what landed on main since it branched:

- The fleet feedback ingest merged separately as #32, with run_id dedupe and
  device_error on top of what this branch carried. Its files are taken from
  main wholesale; this branch keeps the ranking-side wiring (apply_feedback
  before the sort) and the README paragraph on how rows enter the ranking,
  folded into the newer section.
- #33's terrestrial path loss and under-beam derating now shape
  received_power_dbm, which the sweep calibration reads. tower_scoring still
  models its own direct path with free space plus the horizon term, so the two
  direct-path figures differ; pointing scoring at path_loss/underbeam_loss for
  the direct path (and keeping free space for the target echo) is the
  follow-up #33 named.
- Address lookup (#31) and the marker stacking fix (#29) merge cleanly apart
  from adjacent lines in app.py, the test helpers and the README API table.

Co-Authored-By: Claude Fable 5.1 <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