docs(api): correct the retained sections (NEH-196) - #259
Merged
Conversation
Thirteen defects in the sections NEH-194 kept on the premise that they were the accurate part. Twelve were confirmed against code; one item in the issue was struck as wrong (see below). Two that caused harm rather than confusion: - The Live Scan example fetched GET /records/?limit=1, called it "the latest record" and PATCHed it. Project-wide listings order by id ASCENDING (records.py:126, deliberately and with a comment), so it targeted the OLDEST record in the database and would overwrite an unrelated page's metadata. Now uses record_id from the capture response, which CaptureResponse has carried all along (cameras.py:103-111). - "reviewer: read-only views only" was wrong. allow_read_only is RoleChecker(["admin","operator","reviewer"]) (auth.py:171) and guards annotations and record status transitions; reviewers are the role the QA workflow exists for. Performance figures replaced with the 23–24 July bench measurements: 7–8 s per pair on picamera2 and ~4 s on gphoto2, against a documented "5–7 s" and "1760 pph"; and 560–780 KB per medium-preset JPEG against a documented 4–5 MB. The old numbers were roughly double the real throughput and six to eight times the real file sizes. Also: modified_at exists only on Record; migrations are automatic only in the dev container; the session token is a custom HMAC string and not a JWT; POST /records/ returns 200, not 201; logout must clear auth_user as well as access_token; email validation is a deliberate basic regex; and live_scan_workflow no longer defaults to a project name that 422s. STRUCK from NEH-196: the claim that GET /collections/?project_id= returns nested sub-collections. It does not. check_collection_parent means a nested collection has project_id NULL, so it cannot match the filter — the doc and the endpoint docstring were both already correct.
Seven findings on the twelve corrections in the previous commit. All verified against code; all valid. The pattern is consistent — every one is a claim true of one code path and asserted of all of them. - "The client treats a token without auth_user as logged out" is true of the root route (+page.svelte:24) and false of the store, whose isAuthenticated checks token !== null alone (auth.ts:113-114,149-151). The advice to clear both keys was right for the wrong reason: the real hazard is a leftover auth_user leaving the app authenticated with a stale user, and the keys hydrate independently so that state is reachable. - The 422 for an unknown project_name only fires when collection_id is omitted; with a collection_id, _resolve_capture_target never reads project_name (cameras.py:43-56). - The 560-780 KB storage figure is picamera2-only. No sizes were measured on the DSLR path, and the resolution presets do not apply there at all — gphoto2_backend.py:443-446 silently ignores most CameraConfig fields. Sizing a Canon unit from it would under-provision badly. Arithmetic also corrected: 56-78 MB, not 60-80. - "created_at on every model" is false: ProjectMember uses added_at (project_member.py:14). And Collection does have a modification timestamp, named updated_at (collection.py:27), so "modified_at only on Record" misled in the other direction. - The ordering rule holds only for unfiltered listings. With ?collection_id= the order is sequence-then-id (records.py:125), so limit=1 returns whatever the operator ranked first. Line citation was also off by one. - allow_read_only is defined six times, once per router. records.py:30 governs the annotation and status endpoints; the auth.py:171 I cited governs only auth. Editing it would have changed nothing. Reviewers are also not read-only on collections — they can trigger a BagIt export (collections.py:443) — and cannot return a record to captured. - The ThreadPool and stagger are at service.py:388-393; :320 is the function signature. That citation was inherited verbatim from the bench report, which has the same imprecision.
There was a problem hiding this comment.
Pull request overview
Corrects inaccurate API reference guidance for capture workflows, reviewer permissions, authentication, performance, storage, and architecture.
Changes:
- Uses capture response IDs to update the correct record.
- Replaces estimated hardware metrics with measured results.
- Corrects role, token, migration, validation, and timestamp documentation.
Comment on lines
+357
to
+359
| Measured on a Pi 5 during hardware validation, 23–24 July 2026, at the medium | ||
| preset. Earlier figures in this section were estimates and were roughly double | ||
| the real throughput and six to eight times the real file sizes. |
Comment on lines
+546
to
+549
| project_name must already exist — but only when collection_id is omitted. | ||
| _resolve_capture_target (cameras.py:43-56) resolves the path from the | ||
| collection when collection_id is given and never reads project_name at | ||
| all; otherwise an unmatched name returns 422. |
Comment on lines
+690
to
+694
| Clearing only the token is not safe: isAuthenticated() checks | ||
| token !== null alone (stores/auth.ts:113-114, 149-151), so a leftover | ||
| auth_user leaves the app authenticated with a stale user, while the | ||
| root route requires both and bounces to login (routes/+page.svelte:24). | ||
| The two keys hydrate independently, so half-cleared state is reachable. |
Three review findings, all valid. The serious one: I had the half-cleared session backwards. isAuthenticated() tests token !== null alone (stores/auth.ts:113-114,149-151), so clearing the token makes it false whatever auth_user holds. The hazard is the reverse — a retained token with auth_user gone still reads as authenticated, with userRole null. The advice to clear both keys was right either way, but the reason given was the opposite of the behaviour. Worth recording how that happened: the adversarial review stated this correctly, and I inverted it while writing the correction. The failure was comprehension, not verification. Also: - The performance preamble said "at the medium preset" over a table covering both backends, contradicting the storage note below it, which says gphoto2 ignores the presets entirely. Scoped to picamera2, with the DSLR condition stated. - The live_scan_workflow docstring explained the collection_id branch, which that example never takes — it sends no collection_id. Reduced to the path the example actually exercises, with the other case parenthetical.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
docs/developers/API_REFERENCE.md:359
- This scopes both benchmark rows to the medium preset, but the same section later correctly states that gphoto2 ignores resolution presets. As written, readers may treat the DSLR result as a reproducible medium-preset measurement even though it reflects the camera body's settings. Scope the preset only to picamera2.
Measured on a Pi 5 during hardware validation, 23–24 July 2026. The picamera2
figures are at the medium preset; the presets do not apply to gphoto2, which
shot at whatever the camera bodies were set to (see the storage note below).
docs/developers/API_REFERENCE.md:549
- The example neither accepts nor sends a
collection_id, so this branch of_resolve_capture_targetis unreachable from the documented workflow. The qualification makes it sound as though callers can omitproject_namewhen using this function, but its required argument and request body do not allow that. Document the behavior this example can actually exercise, or add a collection parameter and send it.
"""Workflow for the live scan page: capture → auto-save → optional metadata update.
project_name must already exist: this example sends no collection_id, and
an unmatched name returns 422. (If a capture does send collection_id,
docs/developers/API_REFERENCE.md:693
- The stated authentication outcome is reversed: after only
access_tokenis removed, bothisAuthenticated()implementations evaluate to false because they check the token, and the root route goes to/welcome. The real problem is thatcurrentUser/userRoleremain hydrated from staleauth_user, leaving inconsistent state—not that the app remains authenticated.
localStorage.removeItem("auth_user")
The client stores the user alongside the token (stores/auth.ts:78-79),
and the two keys hydrate independently, so half-cleared state is
reachable. The hazard is a leftover TOKEN, not a leftover user:
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
docs/developers/API_REFERENCE.md:361
- This comparison is only true for picamera2: the previous 1760 pph estimate is about twice picamera2's ~900 pph, but is nearly identical to the newly documented gphoto2 throughput of 1800 pph. Please qualify the statement so it does not incorrectly characterize the DSLR measurement.
Earlier figures in this section were estimates, and were roughly double the
real throughput and six to eight times the real file sizes.
docs/developers/API_REFERENCE.md:386
- The certainty here is unsupported by the preceding facts: no DSLR sizes were measured, and output depends on the camera body's settings. The picamera2 figure may under-provision a DSLR setup, but that cannot be guaranteed without measuring the intended settings.
at whatever the body is set to. Sizing storage for a DSLR unit from the number
above will under-provision badly. The low and high picamera2 presets are also
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes NEH-196.
NEH-194 trimmed this file on the premise that the retained sections — Code Examples, Frontend Integration, File Storage Architecture, Architecture Overview — were the accurate part. They were not; they had simply never been audited. This fixes them.
Two that caused harm rather than confusion
The Live Scan example patched the wrong record. It fetched
GET /records/?limit=1, called the result "the latest record", andPATCHed its title, typology and author. Unfiltered listings order by id ascending (records.py:127), so that is the oldest record in the database — running the example against a populated appliance silently overwrote an unrelated page's metadata. It now usesrecord_idfrom the capture response, whichCaptureResponsehas carried all along (cameras.py:103-111).Reviewers were documented as read-only. They are the role the QA workflow exists for:
allow_read_onlyincludes reviewer, and it guards annotations andin_review → approved/rejected(schemas/record.py:11-18). A frontend built to the old line would have hidden review from reviewers.Figures replaced with measurements
The performance section was estimates. Against the 23–24 July bench run: 7–8 s per pair on picamera2 and ~4 s on gphoto2, versus a documented "5–7 s" and "1760 pph"; and 560–780 KB per medium-preset JPEG versus a documented "4–5 MB". Roughly double the real throughput and six to eight times the real file sizes.
The storage figure is now explicitly picamera2-only. No sizes were measured on the DSLR path and the resolution presets do not apply there —
gphoto2_backend.py:443-446silently ignores mostCameraConfigfields — so sizing a Canon unit from that number would under-provision badly. Unmeasured presets are named as unmeasured rather than extrapolated.Also corrected
modified_at/created_atare not uniform (ProjectMemberusesadded_at;Collectionhasupdated_at); migrations are automatic only in the dev container; the session token is a custom HMAC string, not a JWT;POST /records/returns 200, not 201; logout must clearauth_useras well asaccess_token; email validation is a deliberate basic regex;live_scan_workflowno longer defaults to a project name that 422s; and the 422 for an unknownproject_nameis scoped to the case wherecollection_idis omitted.Process
Two commits: the fixes, then an adversarial review pass over those fixes. The review found seven further errors in the corrections themselves, every one of the same shape — a claim true of one code path and asserted of all of them. The
allow_read_onlyI cited (auth.py:171) is one of six definitions and governs nothing in records; the ordering rule I stated holds only for unfiltered listings; theauth_useradvice was right for the wrong reason. Those are fixed in the second commit.One item in NEH-196 was struck as wrong and is not acted on here:
GET /collections/?project_id=does not return nested sub-collections.check_collection_parentmeans a nested collection hasproject_idNULL and cannot match the filter. The documentation and the endpoint docstring were both already correct. Retraction recorded on the issue.Docs-only. All four Python examples parse under
ast.parse.