Agent-authored code-health week — for review, not for merge - #110
Open
AlexisJanin wants to merge 11 commits into
Open
AlexisJanin wants to merge 11 commits into
AlexisJanin wants to merge 11 commits into
Conversation
These six packages were the last stratum of src/ still commenting the
what. They are also the files a contributor reads first when adding a
datasource, so narration here teaches the wrong habit.
8 comments deleted, 3 tightened, 24 kept — the majority are kept, which
is the expected shape: most of these comments already state a why.
Deleted: three __init__.py banners restating their own directory path,
and five comments restating the statement below them ("Compare extensions
by preference order", "Read the data starting from the row after units").
Docstrings: eight tightened from restating the method name to stating a
contract — the sample-time formula in _format_xml_waveform_data, the
column-naming rule in both mindray _load methods, and why parse_file's
signature is odd (only the first Servo U file carries the header, so its
start_time and rename_map must be fed into every later call).
Two corrections fall out of that: _is_float was documented as never
raising, but OverflowError propagates through its except clause; and
_remove_polluted_columns named two of the four device markers its regex
actually matches.
fluxmed_signals' `units_line_index + 6` is deliberately left unexplained.
What those six lines contain is not established by the code or by the
fixtures, so the comment now records the uncertainty and the asymmetry a
reader needs (under-skipping is caught by the numeric-row filter,
over-skipping silently drops data) rather than inventing a reason.
Comments and docstrings only: for all 18 files the AST with docstring
Expr nodes stripped is identical before and after, so no executable code
changed. Left untouched by instruction: the mindray_respi_waves
vectorisation and int64-ns rationales, and mindray_scope's "seems tz
aware" hedge, which belongs to a separate ticket.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This started as a fix: the per-file guard in the 'other' override reported
error_message="Unexpected error processing <file>" and discarded the
exception, so the actionable half of the diagnosis never reached the
Inspect modal. That fix landed on main first, in a better form -- it
prefixes the exception type and keeps only the first line, which matters
because pyarrow appends the file's whole schema to its message.
What survives is the coverage. Main's own tests reach the generic except
through a parquet with duplicate column names; these two reach it through
the failures a clinician actually hits -- an empty CSV ("No columns to
parse from file") and a file that is not parquet at all ("Parquet magic
bytes not found in footer"). A third pins status, datasource_name and
file_path unchanged on that path.
The earlier version of this commit also put the file name in the message,
reasoning that other::<stem> carries no extension. The inspect card
renders "File: <full path>" on the line directly above the error, so the
prefix only repeated what was already on screen.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"One corrupt file must not cost the whole run" is the most load-bearing resilience promise in the app, and not one of its except branches was exercised. Every site is a bare `except Exception: log; continue`, which is exactly the shape an innocent refactor breaks silently, two ways: the guard stops catching and one corrupt file takes down a visualization the clinician was mid-way through, or it catches too much and a datasource that loaded nothing is reported as fine. Six tests, no production code. A DataSourceBase subclass whose _load, _format or inspect raises is appended to DataSource.AVAILABLE for one test at a time, alongside a healthy neighbour, and the four halves of the contract are pinned: wrapper.main skips the raising source and still returns the neighbour's signals; wrapper.inspect reports load_error carrying the real exception text; DataSourceBase.extract answers None rather than propagating; and a _format failure keeps format_error distinct from load_error, which is what lets a caller tell "could not open the file" from "opened it and the contents made no sense". Each guard was verified to be genuinely pinned by neutering it in a throwaway copy of src/ and confirming exactly one test goes red. One test is noted in its own docstring as double-guarded: DataSourceBase .inspect and wrapper.inspect each catch the same failure onto the same status, so neutering either alone leaves it green. The two tests beside it are what pin each layer on its own. The registry is duck-typed — wrapper.main reads NAME/MAIN_MODULE and wrapper.inspect reads NAME/DATASOURCE_CLASS — so a fake entry needs no decorator. ALLOW_QUICK_LOAD = False on every fake keeps the parquet cache out of it, so the file writes nothing to disk; monkeypatch is function-scoped because AVAILABLE is also read by the session-scoped default_database_options fixture. Statuses are asserted as independent string literals, never cst.InspectionStatus. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The severity to log-level fan-out was byte-identical in three modules in three different layers -- wrapper.py, config/parsing.py and the Dash data_callbacks.py -- and the "database_options [%s]: %s" prefix had nine independent copies, three in each. validation.py already owned one cross-tier phrasing (ValidationIssue.unknown_keys), so it was the established home for how an issue is worded but not for how one is reported. A fourth severity, or a change of prefix, was three edits with nothing to catch a missed one. log_database_options_issues(issues, logger) now owns both. The name says database_options because the body hardcodes that prefix: every ValidationIssue producer in src/ is a database_options validator today, and a generic name over a specific message is a trap for the first caller that is not. Two things the extraction had to preserve and neither is visible in the suite's output. The logger is a parameter, never a module-level one here: a getLogger(__name__) inside validation.py would silently rewrite every record's origin from clinical_scope.wrapper to clinical_scope.validation. stacklevel=2 carries the rest of that origin, because the emitting frame moved into this module while funcName, filename and lineno should still name the tier that found the issue -- unobserved today, since both formatters in logger_config.py print only name, level, time and message, but a formatter is one edit from reading them. And the table lookup keeps the old else branch's catch-all: a severity outside the Literal reports at INFO rather than raising, since severity binds type-checkers and not runtime. Verified behaviour-preserving rather than argued: all three call sites were driven against a scratch copy of HEAD's src/ and the emitted (logger name, level, funcName, filename, message) tuples diffed identical. Each of the two guards is pinned by its own test -- removing stacklevel=2 reddens exactly one, re-parenting the logger reddens four. The three caller tests pass against unmodified HEAD as well, which is what makes them characterization tests rather than tests fitted to this implementation. Deliberately not done: no exported SEVERITY_* string constants and no sweep of the ~19 severity= construction sites in database_options_parser.py and plot_types/. A named constant at three of nineteen sites is a half-migration that leaves the vocabulary less consistent than either extreme. data_callbacks.py's severity filters and colour map are untouched for the same reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
load_database_options_from_path is what README.md tells library users to
call and what all three scripts/ entry points import, and no test had
ever executed it. Its .xlsx branch is the only route to
xlsx_to_database_options; the suite reached that module solely through
the bytes variant the Dash upload uses. So the file-path route into the
primary clinician-editable config format was untested end to end,
build_patient_options and load_options had no coverage anywhere, and the
sidecar the .xlsx branch writes into the user's own directory had never
been observed by anything.
Thirteen tests, no production code. Both formats load through the path
API, an unknown suffix raises, a missing file raises, and .JSON is
accepted because the dispatch lowercases -- a Windows-cased spreadsheet
is the realistic way that branch is reached.
Three behaviours are pinned deliberately rather than incidentally. The
<stem>_from_xlsx.json sidecar is asserted to land beside its input, to
parse back to the returned dict, and not to appear in a fixed location:
a load that writes into the caller's data folder is surprising enough
that it should be a decision on record, not a discovery. Its failure
path is pinned too -- a read-only directory still yields the parsed
config, and the test asserts the logged warning, because otherwise a
sidecar write that was never attempted passes as one that was attempted
and forgiven. And load_options answering {} for a missing file while
load_database_options_from_path raises FileNotFoundError is now a test
with the asymmetry in its name: an absent patient_options file is
optional, a config the caller named by path is not.
Each of those is verified load-bearing by neutering one guard at a time
in a throwaway copy of src/ and confirming which tests go red -- reading
the greens, not the reds. Two came back green on the first pass and both
were fixed rather than explained. Dropping the Path() coercion in
build_patient_options reddened nothing, because every test happened to
pass a Path while the signature promises str | Path; one test now passes
a str, and the neutering reddens it. Deleting the sidecar write call
outright left the read-only test green, since it passed on the file's
absence -- the state a removed call also produces; the caplog assertion
is what tells those two apart.
Fixtures are copied into tmp_path before loading, never read in place,
because of that sidecar write. Representative keys are asserted rather
than whole-dict json/xlsx equality: that parity oracle already lives in
test_example_assets.py, and a second copy would make both files fail
together on an unrelated config edit. Worth knowing for anyone extending
this file -- tests/data/option_files/example_database_options.{json,xlsx}
are not a parity pair despite the shared stem. The .json carries 13
top-level sections including edf; the .xlsx has two sheets and parses to
five, with different group and loop names. Nothing enforces parity there,
unlike the shipped example/demo_database pair.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
validate_database_options checked the keys inside a section but never the
section name, so "servo-u" or "mindray_respi_wave" discarded a whole
device's loading configuration in total silence: no issue, no log line,
just a run that reports 0 datasource(s). The one question a clinician
could ask — "why is this device missing?" — had no answer on any surface
the app offers. The Dash UI already renders validation issues in orange
on config upload, so a warning lands where they are looking, at the
moment they choose the file.
A top-level key that is not global, does not start with other::, and is
not a registered datasource name now yields exactly one warning naming
the key and the three spellings a section name may take. The valid names
are derived from DataSource.AVAILABLE rather than restated, so a new
datasource cannot make a valid config warn. The stem after other:: is a
user file name and stays unchecked — the validator never sees the patient
folder, so a misspelled stem is a separate problem for a later fix.
The message says no data is loaded for the section, and deliberately does
not say the section is ignored: plot_assembly._flatten_config iterates
every top-level section regardless of name, so a mis-named section's
grouped_fields and derived-plot entries do still reach assembly and can
still resolve. What is actually lost is the loading half — wrapper.main,
wrapper.inspect and extract_datasource each gate on membership of
DataSource.AVAILABLE.
The check runs before the isinstance guard, so {"servo-u": "hello"} warns
too, where it used to be silent, and continues past _validate_section, so
a section no datasource reads gets one message about its name instead of
a cascade about its keys. Nothing gates on the returned issues; every
caller only logs or renders them, so no file that loads today can stop
loading, and all four shipped configs were confirmed to stay warning-free
in both formats.
The demo config's guard lives in test_example_assets.py, the module that
already owns example/demo_database/ assertions — tests/README reserves
the reach out of tests/ for demo_patient/ alone. Each guard was verified
by neutering it in a throwaway copy of src/ and reading which tests stay
green; the accept-case tests are regression guards and are not counted as
coverage of the new behaviour.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
OtherDataSource is the only datasource that reads its own files, and it spelled the same normalization tail twice: set_datetime_index, the to_numeric coercion loop, then deduplicate_then_sort_index, once in main and once in inspect, 150 lines apart. The two had already drifted (df.columns against list(df.columns)), and the suite exercises main far more heavily than inspect, so the second copy is the one that would rot unnoticed. _normalize_on_time_axis now holds the sequence once. The read deliberately stays at each call site rather than moving into the helper. Every way the readers fail is a ValueError subclass: EmptyDataError for an empty CSV, pyarrow's ArrowInvalid for a corrupt parquet, and _load_single_file's own ValueError for an unsupported extension. A read pulled inside the helper would land in the caller's except ValueError, which exists to mean "this file carries no time axis", so an unreadable file would be reported as a missing datetime column: the inspect entry would lose the file-name prefix that "Preserve the real exception text in OtherDataSource.inspect" added, and the log line would name the wrong cause. Confirmed by neutering, not argued: moving the read into the caller's try reddens both TestInspectUnreadableFile cases and logs "no datetime index in 'corrupt_file.parquet'". main's loaded_files.append moves from between set_datetime_index and the coercion to just after the helper, still ahead of _format so a file the datetime window later empties is still recorded as read. That list drives the source symlink, which is other's only provenance. The move can change behaviour only if the coercion or the dedup raises, and neither can: to_numeric(errors="coerce") raised nothing over every column dtype these readers produce, its one raiser needs a duplicate column label (making df[column] a DataFrame) which raises TypeError and is unreachable anyway since read_csv mangles duplicate headers and read_parquet rejects duplicate fields, and deduplicate_then_sort_index only reads index predicates on the DatetimeIndex set_datetime_index guarantees. The no-time-axis path this change moves was untested in both callers. Four tests now pin it: main skips the file, still loads its sibling and leaves the skipped file out of the symlink set; inspect reports load_error carrying the reader's bare message with no file-name prefix, and still describes the sibling. All four pass against unmodified src/ at HEAD, so they pin the old contract rather than this implementation. Both callers were also driven over a fixture folder before and after and produce a byte-identical observable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
renderer.py decides which subplot a clinician's mark lands on and at what clock time; either being wrong looks entirely plausible on screen until someone re-reads the study. Its lines were already 95% executed by the callback tests, but almost nothing asserted what they built: no test in the repo called annotation_to_shapes, build_figure_overlays or normalize_annotation_for_display by name, and the pre-existing subplot fixtures carry no yaxis key at all, so the "y3 domain" branch was never once evaluated with a real axis. Adds 47 tests over shape geometry (type, coordinates, axis refs), subplot resolution including the removed-subplot skip, and timezone normalization asserted as literals across three zones plus a January/July pair that a fixed-offset implementation would fail. Mutation testing, not the coverage number, drove what to assert. Hardcoding the event or window xref, detaching the point dot's coordinates from its data, or making the window label ignore its subplot each left every test in the repo green beforehand; each now reddens the test that names it. The point case mattered most: Annotation.create hides a point's label, so the dot is the whole rendering of a default point, and none of it was pinned. The module docstring claimed the x-axis reference "is always x for time-series plots because all subplots share the same time axis". Both halves were wrong. Figures are built with shared_xaxes=False and time rows are linked with plotly's matches, so they pan and zoom together while keeping distinct refs, and every annotation stores the ref of the trace its click landed on. Corrected to what the code does; docstring only, no behaviour change. Assertions are limited to placement fields, so restyling an annotation cannot redden the file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
deduplicate_then_sort_index kept the first row per timestamp and said nothing about the rest. A CSV whose timestamp column has lower resolution than its sample rate — a second-resolution export of a 100 Hz channel, which is the tidy clinician CSV the other datasource exists for — lost everything but one sample per second in silence. Measured on a 6000-row file: 60 plotted points, 99% discarded, no message from main, from inspect, or in the figure, and the curve it draws looks entirely plausible at 1 Hz. inspect made it worse rather than catching it. other deduplicates before building its column table, so raw_point_count reported 60 for that file: the one surface a clinician would consult to check for data loss confirmed the loss had not happened. The warning goes in the function rather than at the call sites, so one line covers all eight and every datasource. other threads the file name through _normalize_on_time_axis, which since the per-file read was collapsed is the single dedup call site serving both main and inspect, so both paths name the file that lost rows. The other seven pass nothing and report counts only; naming their source is a separate change, since several concatenate many files before deduplicating and could not name one file if they wanted to. The dedup and sort logic is untouched. Verified rather than asserted: the returned frame is byte-identical across sorted, unsorted, duplicated, empty, single-row, all-identical and NaT-indexed frames, including the no-copy fast path and the input never being mutated. Noise was measured before the change, not predicted. Instrumenting every binding of the function and running the full demo patient and Patient_difficult_format through main against two option sets produced zero row drops, and a handler teed onto the logger across the whole suite catches nothing but the new tests. No file that loads today can stop loading: this adds a log call and a keyword-only parameter with a default. For a log line the prose is the whole contract — there is no return value or state to check it against — so the message's meaning is pinned, not just its numbers. The test fixture makes dropped (5), kept (3) and colliding stamps (2) three different numbers so "5 of 8" can only be the intended pair, and two assertions pin the direction of the count and the identity of the survivors: rewriting "Dropped" as "Kept", or describing the survivors as the rows that had no duplicate, each left the entire suite green beforehand and now reddens the test that names it. Word order, voice and phrasing stay free to change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The class docstring told a new-datasource author to implement `_find()`. It is not abstract, has a working default over `find_files`, and no datasource in the repo overrides it, so the instruction bought a method nobody needs. `_load()` is the only abstract method; the real unstated requirement is setting `OPTIONS_MODULE`, which `__init_subclass__` reads for the datasource name, the cache file name and the capability flags. Eight further docstrings restated their own method name. They now carry what the signature cannot: that a failed cache write is swallowed on purpose, that `_apply_time_shift` rebinds the index in place and returns the same frame, that `main` returns [] for absent data but raises on a genuine failure, that `filter_date=False` reads the bounds as times of day, and that `_find` yields one file per stem rather than every match. Also corrects the comment above inspect's field_display strip, which named `_load()` as the thing being narrowed. `_load()` takes only a path and cannot see field_display (ADR-0010); it is the parquet cache read that would otherwise be column-pruned, and `configured_columns_only` puts that narrowing back. Docstrings and comments only: the AST with docstring nodes stripped is byte-identical to HEAD's. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"Which datasources does this run touch" was answered three times: an
identical `requested_sources` comprehension in main, inspect and
extract_patient, each followed by a loop over the whole registry with an
`if name not in database_options_global: continue` guard re-applying the
same filter. The copies already differed in whether they reported
progress, so a reader could not tell which differences were intentional.
`_requested_datasources` answers it once and the three loops iterate its
result, which makes the in-loop guards dead. `AVAILABLE` is named in one
place. The two remaining `"data_folder"` subscript literals become
`cst.PatientOptions.PathDataFolder.NAME`, which the file already used
twice.
Behaviour-preserving, checked rather than assumed: HEAD and this change
were probed over 6 configs x {main, inspect} x {callback, none},
capturing the full (current, total, name) sequence and the return shape.
Identical throughout, including nothing requested, unregistered keys
only, and an `other::<stem>` key.
tests/unit/test_progress_reporting.py is new and pins what nothing
covered. `progress_callback` had no test anywhere in the suite, so a
refactor that counted over the registry instead of the requested set --
a three-datasource run stalling at "3/9" -- would have committed green.
It pins the count, the registry order, and the placement: the progress
bar labels the datasource being worked on, so the call has to precede
the read.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This branch has not been deployed
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
2 participants
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.
What this is
An experiment, not a feature branch. Eleven commits written by autonomous Claude Opus agents across eleven separate sessions, under a fixed protocol, on a branch that was never intended to merge as-is.
The deliverable was never the code. It was a capability map: which kinds of code-health task can an agent be trusted with unsupervised in this repo, and where is the ceiling? Committed code was defined up front as evidence, not as the goal.
This PR is a review surface, not a merge request. See What this PR is not.
How it was written
The protocol lived in
.scratch/agent-week/(gitignored — see Where the rest of it lives). Every run followed the same loop:ruff format --check .,ruff check .,pytest— all three, all green.Run 1 was an audit instead: four subagents in parallel (
comments/coverage/depth/robustness), merged into a ranked backlog of 12 one-session tasks, then stopped at a human approval gate. No code changes in that run — the gate was the point.The guardrails
tests/expected_results/or any use of--update-snapshots; the__init__.pyexports; annotation and extraction return shapes;README.mdanddocs/user_guide/**; any decided ADR;example/**andtests/data/**.Why it was run
To answer three questions with evidence rather than intuition:
The honest answer to (3) turned out to be the most valuable output, and it is not in this diff.
What landed
11 commits, suite green at 1459 passed, ruff clean.
database_options-section warning; duplicate-timestamp drop warningValidationIssuereporter;other's per-file read tail;wrapper's datasource dispatchsources/packages;DataSourceBase's stale subclass contract; renderer docstringconfig/parsing.py;inspect's reader failures+227/−116 in
src/, +1383 intests/(97 new test functions). ~86% of the diff is test code. That is the honest shape of the week.Both behaviour changes are warnings on paths that were already degrading — neither rejects input that loads today.
The tests were mutation-checked, not taken on faith
1,383 lines of new test could easily be coverage theatre. Eight independent mutations were applied to
src/and the new suites run against each:logger.debugvalidation.py:stacklevel=2removedvalidation.py: logger re-parented to this modulewrapper: progresstotalcounts the registry, not the requestpaper1→0.5Every mutation reddened. Two results — "removing
stacklevel=2reddens exactly one, re-parenting the logger reddens four" — reproduce verbatim what the agent's own log claimed at the time, which is a datum about the log's reliability as much as the tests'.What the experiment actually found
The audit and the eleven runs produced 39 findings deliberately left unimplemented — too large for one session, or needing a product decision. Several were verified by hand against
HEAD. The ones that matter most, none of which this branch fixes:extract_*returns unformatted data when_formatfails.datasource/base.pycatches, logs, and falls back todf_raw— handing the caller a normal-looking DataFrame in which the timezone, the time shift and the datetime window were all never applied, under a green success banner. The same frame is whatsave_pathwrites to disk.quick_loadnever checks the cache still matches the source. No mtime comparison, no schema check. A source file added after the cache was written is invisible; a corrupt cache is a permanent trap that nothing bypasses or deletes.patient_optionsis validated indash_apiand nowhere else. The library entry points accept the dict raw; a hand-editedtime_shift: nullyields an all-NaTtime axis under a green status line.tzdatais not a declared dependency and is not in the PyInstaller spec — unverified on Windows, but if correct it would break every timezone in the standalone Windows build. Flagged as the single entry most worth checking by hand.to_float_secondsis dead and wrong. On pandas 3 the naive-DatetimeIndexbranch returns epoch seconds 1000× too small, and the tz-aware branch raises before reaching its own code. Blast radius today is nil — the one production caller takes the safe branch.ruff.tomlexcludestests, so no test file has ever been linted — including in CI — and the[lint.extend-per-file-ignores]"tests/*"block is dead config describing ignores for files ruff never reaches.pd.concat(..., axis=1)inmindray_scopehas a pandas 4 deadline. The sort default flips; this is the only finding with a clock on it.What the experiment says about agents in this repo
Recorded plainly, including where it is unflattering:
OtherDataSource.inspectdiscarded the real exception. The same bug was found and fixed onmainin a better form (prefixes the exception type, keeps only the first line — pyarrow appends the whole schema). The rebase collapsed that commit to tests-only and its message was rewritten to say so.main@ 4944467; a later rebase onto Annotations: move, hide, and nudge an existing annotation #100 made task 08's stated premise void (renderer.pymeasured 95%, not the ticket's 29%). Caught by a pre-commission check, not by design — the remaining coverage premises are still unverified.git showby a later run, which marked the reviewer and fix-round columns**not recorded**rather than guessing them — the right call, since that column is the deliverable.Where the rest of it lives
.scratch/is gitignored, so the protocol, the 704-line run log, the 12-task backlog, the four audit reports and all 39 findings are not in this branch — they exist only on one machine. The highest-value findings are transcribed above so they survive; the full set still needs filing as issues.What this PR is not
mainand PR'd from there. The per-task human verdict column is still blank on all eleven entries.validate_and_collect, the gate on every Process and Inspect run) remains unworked and its premise should be re-measured before it is.It is open against
mainso the eleven commits can be read in a review UI, and so CI runs the 3.11 / 3.13 matrix — which this branch has never exercised, since CI does not run on it.🤖 Generated with Claude Code