LostSynapse/box-legend
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Delta 25: code review remediation - 16 findings fixed #1

Merged
sysop merged 14 commits from claude/code-review-98469c into main 2026-09-08 17:08:41 +00:00
Conversation 0 Commits 14 Files changed 23 +1377 -109
sysop commented 2026-09-08 17:05:54 +00:00
Owner
Copy link

Full-codebase review; 16 defects found and fixed. No behaviour added, no page changed shape — every commit either makes something do what its own docstring already promised, or stops it silently doing the wrong thing.

Most were invisible in normal use. Several reported success while doing nothing, and the two most serious corrupted or lost data while PRAGMA integrity_check still answered "ok".

Highlights

  • DB restore silently reverted to pre-restore data. SQLite locates a WAL by filename, so replacing the .db left the old sidecar to replay over it. Reproduced end to end. Purge is safe by contrast because it unlinks rather than replaces — that asymmetry is the crux, and is why purge was deliberately left untouched.
  • Both archival jobs could lose a box/report. WAL disables the multi-database super-journal, and main commits first, so a torn commit deleted from the source without writing the archive. Now a two-phase, resumable move verified by row counts.
  • Both scan pages were inert whenever the session's mode ≠ the page's mode — no scanning, no printing, no Generate, page looking entirely normal. Triggered by any restart (SECRET_KEY is per-process, so every restart invalidates all sessions), bookmark, or restored tab. Clicking through the UI could never reproduce it, which is why it went unpinned for so long.

Also fixed

printing._run() swallowing subprocess failures past every "raises ConfigError" contract (which could kill create_app() at boot) · check_reachable() raising on a malformed device_uri port · duplicate Proposal # 500ing the xlsx import · unescaped Content-Disposition filenames · CSV formula injection in both exports · non-deterministic PPD margin fallback · user rename/delete reporting success on a no-op · archival cron logging that never printed · an unclosed read-only openpyxl workbook · stale project autofill attaching one project's manager to another.

Verification

Tests 696 → 741, all passing. Every new test mutation-checked — revert the fix, confirm the test fails. That caught two of my own vacuous tests and one fix of mine that would itself have caused data loss (a guard that skipped the archive copy on any existing row, then deleted the source anyway; the existing continues_after_one_box_fails test rejected it).

Three fixes the Python suite cannot pin were verified directly instead: the WAL replay against real SQLite files, the cron logging by running the module against a throwaway data dir, and both JS fixes by driving the running app in a browser.

Full write-up, including reasoning and the places a first attempt was wrong: spec/shipped/box-legend-delta25-code-review-remediation.md.

🤖 Generated with Claude Code

Full-codebase review; 16 defects found and fixed. No behaviour added, no page changed shape — every commit either makes something do what its own docstring already promised, or stops it silently doing the wrong thing. Most were invisible in normal use. Several reported success while doing nothing, and the two most serious corrupted or lost data while `PRAGMA integrity_check` still answered "ok". ## Highlights - **DB restore silently reverted to pre-restore data.** SQLite locates a WAL by filename, so replacing the `.db` left the old sidecar to replay over it. Reproduced end to end. Purge is safe by contrast because it *unlinks* rather than *replaces* — that asymmetry is the crux, and is why purge was deliberately left untouched. - **Both archival jobs could lose a box/report.** WAL disables the multi-database super-journal, and `main` commits first, so a torn commit deleted from the source without writing the archive. Now a two-phase, resumable move verified by row counts. - **Both scan pages were inert whenever the session's mode ≠ the page's mode** — no scanning, no printing, no Generate, page looking entirely normal. Triggered by any restart (`SECRET_KEY` is per-process, so every restart invalidates all sessions), bookmark, or restored tab. Clicking through the UI could never reproduce it, which is why it went unpinned for so long. ## Also fixed `printing._run()` swallowing subprocess failures past every "raises ConfigError" contract (which could kill `create_app()` at boot) · `check_reachable()` raising on a malformed `device_uri` port · duplicate Proposal # 500ing the xlsx import · unescaped `Content-Disposition` filenames · CSV formula injection in both exports · non-deterministic PPD margin fallback · user rename/delete reporting success on a no-op · archival cron logging that never printed · an unclosed read-only openpyxl workbook · stale project autofill attaching one project's manager to another. ## Verification Tests 696 → 741, all passing. Every new test **mutation-checked** — revert the fix, confirm the test fails. That caught two of my own vacuous tests and one fix of mine that would itself have caused data loss (a guard that skipped the archive copy on any existing row, then deleted the source anyway; the existing `continues_after_one_box_fails` test rejected it). Three fixes the Python suite cannot pin were verified directly instead: the WAL replay against real SQLite files, the cron logging by running the module against a throwaway data dir, and both JS fixes by driving the running app in a browser. Full write-up, including reasoning and the places a first attempt was wrong: `spec/shipped/box-legend-delta25-code-review-remediation.md`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) 
Every database runs in WAL mode, and SQLite locates a database's WAL by
filename rather than by inode or content. restore_database() and
restore_system() replaced the .db file but left <db>-wal/-shm beside it,
so on the next open init_db()'s `PRAGMA journal_mode=WAL` recovered the
orphaned WAL and replayed its old pages over the restored file --
PRAGMA integrity_check still reporting "ok" afterwards. The restore came
back up holding the pre-restore data and looked in every observable way
like it had worked.

_discard_stale_wal() now unlinks both sidecars as part of the atomic
swap in _atomic_write()/_atomic_copy(). That is the only moment the
deletion is sound: replacing the file underneath a sidecar is the
instant it stops describing it, and the last point anything can tell a
stale WAL from a legitimate post-crash one. Doing it at startup instead
would throw away real crash recovery.

build_system_backup() also excludes sidecars from its raw-copy loop.
They normally do not exist -- SQLite deletes them when the last
connection closes, and this app holds none open between calls -- but a
request running concurrently with that loop made them briefly appear,
and the filter matched only exact .db names.

Not full mutual exclusion: a connection opened before the swap and still
open after it keeps writing old pages into a freshly recreated sidecar,
and restart_and_exit()'s os._exit(0) never closes connections. Closing
that window would mean quiescing every other request, which this app has
no mechanism for; the residual case is documented in the docstring.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
archive_boxes_older_than() and archive_reports_older_than() each wrapped
an INSERT into the attached archive and a DELETE from the live tables in
one `with conn:`. SQLite only writes the super-journal that makes a
multi-database commit atomic for journal modes that need one, and WAL is
not one of them -- with both files in WAL mode the commit is atomic per
file but not across the pair. Main commits first, so an interruption
during the weekly supercronic run could leave a box or report deleted
from the source and never written to the archive: silent, permanent loss.

Split into two separately committed phases. Phase 1 copies into the
archive, writing that file only, so it really is atomic. Phase 2 deletes
from the live tables. The worst case is now an entry present in both
files, which the next run detects and finishes, rather than one that is
gone.

Between the phases, _row_counts() compares per-table row counts on both
sides and the source is deleted only when they match. An earlier version
of this change skipped the copy whenever the archive held any row for the
id, which the existing "continues after one box fails" tests correctly
rejected: a stray or partial archive row read as "already copied", so the
copy was skipped and the source deleted anyway. Counting instead keeps
the three cases distinct -- (0,0,0) means copy, equal means safe to
delete, anything else raises and lands the entry in `failed` with its
source rows intact.

Adds a resume test to each module covering the post-crash state. Both
were mutation-checked: with the guard disabled they fail, box_log by
duplicating and checkout with a UNIQUE constraint violation on
reports.report_id that would otherwise strand an interrupted report on
every retry.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two related spots where a function raised something its own docstring
said it would not.

_run() is the single subprocess choke point, and every caller documents
"raises ConfigError on failure" -- but only a non-zero exit code produced
one. A missing CUPS binary (OSError) or a wedged cupsd (TimeoutExpired)
escaped past all of them. At startup that was the worst case:
_reprovision_printer_queue() catches ConfigError specifically, so an
exception of any other type propagated out of create_app() and the app
failed to boot over a printer problem, in direct contradiction of that
function's stated best-effort contract. On POST /config the same escape
turned an intended 400 into an unhandled 500. Converting at the choke
point fixes every caller at once rather than widening each except clause.
print_label() still relabels whatever comes out of here as PrintError,
which stays correct: a send that could not be attempted is a failed send.

_reprovision_printer_queue() now also catches Exception outright, matching
its sibling _register_self(). Its contract is that nothing about the
printer stops the app from starting, and an except clause narrower than
that contract is a bug waiting for the next exception type.

check_reachable() promises it never raises, but urlsplit() defers port
parsing to attribute access, so a device_uri with a non-numeric or
out-of-range port -- free-text operator config, stored unvalidated --
raised ValueError from .port. /maintenance/printer-check has no handler
for that, so the modal got a 500 HTML page it could not parse and
reported a network error, pointing the operator at the LAN instead of the
malformed URI.

Tests capture printing._run before conftest's autouse fixture swaps it
out, so they exercise the real implementation rather than the stand-in.
All mutation-checked.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
project_number is the PRIMARY KEY of ephemera's active_projects table,
and parse_xlsx() skipped only blank Proposal # rows -- it never deduped.
A workbook listing the same proposal number twice, routine when a project
has several entries, reached replace_active_projects() and failed the
whole import on a UNIQUE constraint. active_projects_import() guards only
ValueError from the parse, so that surfaced as an uncaught IntegrityError
-- a raw 500 instead of the clean 400 every other rejection in this flow
produces, with the footer still showing the previous import's date and
nothing explaining why.

Deduped in parse_xlsx() rather than in storage or the route:
replace_active_projects() is deliberately pure storage with no business
logic of its own, and this is the parse/validate layer its docstring
already delegates that to. Last occurrence wins, matching col_index's own
handling of a duplicated header and plain dict-assignment semantics; the
row keeps its first-seen position, since assigning an existing key does
not reorder a dict.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both weekly scripts end with logging.info("archived %d ... %d failure(s)")
but neither ever configured logging, so the root logger's default WARNING
level discarded it on every run. supercronic captured the output
faithfully -- there was simply never any. That line is the only thing
separating "archived 400 boxes" from "archiving has been silently off for
months", and the only place the failed-id count from a partially
completed archive move is ever reported.

basicConfig() sits under __main__ rather than in main(), so importing
either module -- the tests call main() directly -- never reconfigures
logging for anything else.

Verified by running the module against a temp data dir: the run now emits
"<timestamp> INFO archive_old_boxes: archived 0 box(es), 0 failure(s)".

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The filenames reaching content_disposition() are user input: Checkout's
_pdf_filename() embeds the operator-typed Project Number, and Snapshot
passes a report name straight through. encode("ascii", "replace") only
maps non-ASCII to "?", so two ASCII characters survived into a header
built by f-string. A literal double quote closed the quoted-string early
and truncated the filename the browser saved; a newline landed in a
header value Werkzeug rejects outright, turning a download into a 500.

Control characters are replaced here -- no escaping makes them legal in
a quoted-string -- and the rest is handed to werkzeug's
dump_options_header(), the same helper flask's send_file() uses
internally and therefore the same handling /maintenance's backup routes
already get for free.

quote(..., safe="") rather than the default: it percent-encodes every
non-token character, "/" included, so the RFC 5987 ext-value stays
unquoted as that spec requires. Left as safe="/", a name containing a
slash would make dump_options_header() quote the whole parameter and
corrupt it -- a test pins this for every name shape.

Also adds csv_safe() here, used by the export routes in the next commit.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every free-text column in these exports -- project numbers, POs, part
names, and the admin-configured header-field labels and values -- is text
an operator typed and the database stored verbatim. Written raw, a part
name beginning with "=" is a live formula the moment the file is opened,
which is the normal way these exports get used. Excel and LibreOffice
evaluate it, and nothing in the app hints anything unusual was stored.

csv_safe() prefixes a single quote only when a value begins with one of
= + - @ TAB CR. Ordinary rows come out byte-for-byte, which matters
because these files are consumed by other systems, not only read; and
the integer quantity columns are passed through untouched, since they
come straight from SQL and reading back as '1 would break exactly those
consumers.

Snapshot's per-report CSV had the identical exposure via the same
catalog-sourced names, so the helper lives in the shared export_utils and
both writers use it -- fixing only the one the review flagged would have
left the same hole one route over.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
_parse_ppd_margin() preferred the PPD's own *DefaultImageableArea, but
when that key was missing or unpaired it fell back to next(iter(common))
-- iteration over a set of strings, whose order depends on PYTHONHASHSEED
and is therefore re-randomised at every process start.

Margins are not constant across sizes; this function's own docstring
records that circular/die-cut and 54mm entries carry different left/top
margins from the continuous ones. So for a custom printer whose PPD lacks
that key, two saves of an identical config could persist two different
margin_mm values and silently shift where the driver clips every
subsequent label, with nothing in the config to explain the change.

Now takes the first entry that has paired dimension data in the PPD's own
order. `areas` is built by finditer, so it preserves file ordering, and a
PPD generally lists its primary size first -- stable and meaningful
rather than merely stable. Verified identical output across six
PYTHONHASHSEED values.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rename_user() and delete_user() scope their UPDATE/DELETE to
role = 'operator', so both can legitimately match nothing -- but neither
returned anything, and the routes answered ok=True regardless. Renaming
or deleting an account that does not exist reported success to the admin
and refreshed a list that had not changed.

Fixed in db.py rather than by adding an existence check to the routes:
the write is the thing that knows whether it happened, and returning
rowcount > 0 is the same answer delete() already gives for a catalog
entry. The routes then 404 with "no such user", matching
users_set_password() -- which was already the only one of the three that
checked before reporting success, and the one that needed it least.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
parse_xlsx() opens the workbook with read_only=True, which keeps the
underlying zip handle and a per-sheet reader open for the workbook's
lifetime -- openpyxl documents that callers must close it. Nothing did,
so each import leaked those handles until a garbage collection pass
happened to run in a waitress worker that stays up for weeks between
restarts.

Wrapped in contextlib.closing() around the whole body rather than a
close() before the return, because every exit matters: the
no-header-row ValueError leaks exactly as readily as a clean return, and
a future early exit would too.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Active Projects autofill fired on every keystroke, so typing a longer
project number dragged a shorter one's details along with it. With a
project "12" on file and "1234" not on file, typing 1-2-3-4 filled in
project 12's name/manager/lead at the second keystroke and then left them
attached to project 1234 -- silently mis-attributed data, since
box_details stores these as permanent columns and Reports rolls up by
PM/Lead. Checkout had the same code and put the wrong manager on the
generated Check-Out Sheet.

selectActiveProject() now records what it wrote, and
clearAutofilledProjectFields() undoes it when the number stops matching.
Deliberately narrow: a field is cleared only while it still holds exactly
what was autofilled, so a correction made afterwards survives -- and a
name/manager/lead the operator typed themselves is never touched at all,
since nothing is recorded unless an autofill actually ran. Blanking the
number, and the Clear button, both reset the record too.

Verified against the running app in a browser on both pages: exact match
still autofills, typing past a shorter match clears, a longer exact match
autofills, hand-typed values survive, and an edited field survives while
its untouched siblings are cleared.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
inject_mode() read current_mode from session["active_mode"], and
_nav.html publishes it as window.CURRENT_MODE. Both scan pages pick their
pinned-field inputs off that, so whenever the session disagreed with the
URL the JS looked up an input the served template never renders -- po- on
Checkout, shelf- on Receiving -- dereferenced null, and threw at the top
level of its IIFE.

That throw lands before every behavioural wire-up: the scan keydown
handler, the Print/Generate click handler, createScanTable(),
loadHeaderFields() and loadActiveProjects() were all never reached. The
button labels are set at line 8, before the crash, so the page rendered
looking entirely normal and did nothing at all. The nav rendered the
other mode's links too.

The trigger is any route to a mode's URL that isn't the mode switcher,
because switch_mode sets the session and redirects to that same mode's
page in one step -- which is why clicking through the UI could never
reproduce it. SECRET_KEY is regenerated every process start, so every
restart invalidates all sessions and resets active_mode to "receiving":
after any restart, purge, restore or container recreate, the first load
of /checkout/scan was dead until someone touched the switcher. A logout,
a bookmark, a restored tab or a second device did the same.

A mode blueprint now names its own mode, and only genuinely shared
`common` pages keep following the session -- the same rule
auth.enforce_require_login() already applies to gating, and its docstring
already spells out why. The session is written back so the mode sticks
once a mode-owned page has been reached directly, or the next shared page
would snap the nav back to the stale value.

Also narrows activePinnedFields to keys this page actually renders an
input for. Defence in depth, not the fix: one missing element should
degrade to "that field is absent", never cost the page scanning,
printing and Generate.

Verified in a browser against the running app: straight to /checkout/scan
on a fresh session now reports mode checkout, renders Checkout's nav,
wires shelf-, clears the scan input and POSTs /resolve -- with no console
errors on either page, where before both were inert.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Records the full-codebase review and the 16 defects fixed on this branch,
grouped by theme rather than by finding number, with the reasoning that
would otherwise only exist in commit messages.

Written because most of these were invisible in normal use: several
reported success while doing nothing, and the two most serious corrupted
or lost data while integrity_check still answered "ok". The document
keeps the non-obvious parts -- why purge was safe where restore was not,
why WAL disables the multi-database super-journal, why clicking through
the UI could never reproduce the mode bug -- and is explicit about the
places a first attempt was wrong, including a guard that would itself
have caused data loss had the existing suite not rejected it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sysop merged commit 4bda82dd8e into main 2026-09-08 17:08:41 +00:00
sysop deleted branch claude/code-review-98469c 2026-09-08 17:08:42 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
LostSynapse/box-legend!1
No description provided.