Cut the updater's cache, file server and API comments back #69
Loading…
Reference in a new issue
No description provided.
Delete branch "docs/updater-cache-and-api-comments"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
A comment pass over
updater/firmware.py,updater/file_server.py,updater/api.pyand their three test files. Narration and restatement go;rationale, spec citations and the traversal warning stay, cut to a line or two.
No behaviour change. One small readability edit in
firmware._check: the twocomments explaining why a
Nonefromstoreis not always a failure are nowtwo named conditions instead, and the existing tests already pin both branches.
file_server.py's module docstring loses its copy of the manifest JSON. Theshape is the specification's (section 11.1) and
manifest()already builds it,so a second copy here is the stale duplicate CLAUDE.md warns against.
Worth knowing:
ruff'sDrules are on with no per-file ignores, so D100-D103require a docstring on every module, class, public function and test. Docstrings
could only be shortened here, never deleted.
Scope, tabs, spelling and commits are clean; no test body changed (AST comparison with docstrings stripped shows only
firmware._checkdiffers). Findings inline, plus two that sit off the diff:updater/api.py:196_route,updater/api.py:335_nameandupdater/firmware.py:256_checkstill carry docstrings that only re-spell the name. Ruff's D102/D103 exempt_-prefixed functions and methods (verified against the repo's own config), so these three could have gone entirely — that was the remaining headroom the pass walked past.updater/file_server.py:97is where the traversal defence actually lives, and{image_path(offer): offer for offer in offered(cache)}.get(path)reads as an ordinary dict lookup. The only warning is ~90 lines up in the module docstring. Worth one line at the lookup itself, since a future editor "simplifying" it to a join onto the cache root breaks it silently.@ -28,3 +28,2 @@# Bound here rather than looked up on the module, because the suite replaces# that name with a fake to keep every other test off a socket.# Bound here rather than looked up on the module, which the suite fakes out.Over-cut. "which the suite fakes out" reads as the module being faked; it is the name on it (
tests/conftest.py:91patchesfile_server.FirmwareFileServerautouse). The why is gone too. Something like:# Bound here, not looked up on the module: conftest patches that name to keep the rest of the suite off a socket.@ -106,3 +90,4 @@"""One check at a time: two racing share a part-file and overwrite each other."""FETCH_STOP = threading.Event()"""Set once the service is stopping, so a fetch gives up instead of finishing.Lost "Nothing clears it: the process is on its way out, and the next one starts with an event of its own." That is a real invariant, not narration:
updater/service.py:74is the only.set()and nothing inupdater/ever clears it — only the tests do, to fake a fresh process. Without the line a reader sees a module-globalEventthat is set and never reset and cannot tell whether that is deliberate or a bug. Put it back as one line.@ -314,1 +282,3 @@if node_for_environment(environment) is not None:we_stopped_it = FETCH_STOP.is_set()no_node_wants_it = node_for_environment(environment) is Noneif not we_stopped_it and not no_node_wants_it:not we_stopped_it and not no_node_wants_itis a double negative, which costs back most of what the named booleans bought.a_node_wants_it = node_for_environment(environment) is not Noneandif not we_stopped_it and a_node_wants_itreads straight.Behaviour is preserved either way —
node_for_environmentis pure (prefix check plus an enum lookup withKeyErrorcaught), so evaluating it unconditionally when the stop is set only costs a lookup.One thing left.
@ -119,18 +94,18 @@ class NodeRequestHandler(BaseHTTPRequestHandler):body = json.dumps(manifest(cache), indent=2, sort_keys=True).encode()self.respond(HTTPStatus.OK, "application/json", body + b"\n")return# Matched against what was advertised, never joined onto the cache root:Yes, this is now a duplicate. Keep it here and cut the first sentence of the last paragraph of the module docstring — this is where an editor about to break it will be standing, and the paragraph still reads whole starting at "There is deliberately no authentication".
Clean. Nothing outstanding.