Bridge YAMLs: fail CI when MATLAB has moved a file since matlab_last_sync_hash #191

Activity

1. stevevanhooser commented on Sep 7, 2026

   

   stevevanhooser

   on Sep 7, 2026 – with Claude

   ContributorAuthor

   More actions

   It bit, and here is the sweep number

   This issue parks itself on "has not bitten in practice — if a MATLAB sync goes badly for lack of it, that is the signal to prioritise." That happened on #203.

   Merging main into that branch conflicted in two bridge YAMLs, and the only thing that decided the conflict was which side's matlab_last_sync_hash was current. Both sides looked equally plausible; resolving it meant checking each hash against NDI-matlab by hand. Three were stale in ways nothing would have caught:

   entry	recorded	actual
   GeneIngest	9e956f76	eae346a
   fromGEF	6ae50870	a178e46
   tilePath	e217f367	b9972ab

   The check this issue proposes would have answered that in one command instead of a manual sweep.

   The initial-sweep number

   This issue notes "somebody has to run the check to know." I ran it — exactly the proposed git log <hash>..HEAD -- <path>, against main (12dfe24) and NDI-matlab main (114e034f8):

   entries with matlab_path AND matlab_last_sync_hash: 375
     current:                          286  (76%)
     STALE (MATLAB moved since):        84  (22%)
     unresolvable hash:                  4
     path no longer in NDI-matlab:       1
   

   Stale entries cluster, so the sweep is less daunting than 84 suggests — five files hold two thirds of them:

   15  src/ndi/fun/ndi_matlab_python_bridge.yaml
   13  src/ndi/ndi_matlab_python_bridge_database_fun.yaml
   12  src/ndi/cloud/ndi_matlab_python_bridge.yaml
    8  src/ndi/cloud/api/ndi_matlab_python_bridge.yaml
    7  src/ndi/cloud/sync/ndi_matlab_python_bridge.yaml
   

   Worst by commits behind: url (11), profile (8), extract_doc_files (6), dir (6), downloadGenericFiles (5), session (5).

   The "wrong kind of hash" prediction is confirmed

   The side-benefit section is right. Four entries carry 234c356, which is not a commit in NDI-matlab at all — git cat-file reports "Not a valid object name": epoch (docTable), treatment (docTable), readtablechar, writetablechar. Exactly the PR #61 failure mode, and the commits-behind check catches them for free.

   Two things the check as scoped would still miss

   Both surfaced while running the sweep, and both are cheap to fold in:

   1. 143 entries have a matlab_path but no matlab_last_sync_hash at all. That is more than the 84 stale ones. An entry with no hash cannot be stale and cannot be checked — it is permanently invisible to a freshness guard, which is a weaker claim than "current" but reads the same to a reviewer. Worth deciding whether a missing hash should fail, warn, or be recorded deliberately.

   2. The root-level src/ndi/ndi_matlab_python_bridge_database*.yaml files are outside the completeness guard entirely. read_bridge_index rglobs from each BridgedPackage.python_dir, and those three files sit directly in src/ndi/, under no package. Consequence: copy_session_to_dataset still carries an entry and a hash for +ndi/+database/+fun/copy_session_to_dataset.m, which no longer exists in NDI-matlab — it was superseded by convertLinkedSessionToIngested. test_every_recorded_matlab_path_points_at_a_real_file does not catch it because it never walks that file. That is a live stale path today, not a hypothetical.

   Related

   #208 adds the sibling guard on the same file — one MATLAB file gets one entry, because eleven were recorded twice and four of the pairs disagreed with each other about whether the thing was ported. Same failure shape as this issue: the bridge answering one question two ways, with nothing checking. A freshness check and a duplicate check are the two halves of "an entry can be trusted."

   Happy to do the sweep and the check if it is wanted — the script above is throwaway but the numbers are reproducible.

   ---

   Generated by Claude Code
2. stevevanhooser commented on Sep 7, 2026

   

   stevevanhooser

   on Sep 7, 2026 – with Claude

   ContributorAuthor

   More actions

   Correction to the numbers above, and a way to fix most of it exactly

   My "84 stale / 4 unresolvable" split was wrong. git log <blob>..HEAD -- <path> does not error on a blob hash — it silently returns commits — so entries carrying a blob hash were counted as stale. git diff does reject them, which is how it surfaced.

   Typing every hash with git cat-file -t gives the real breakdown:

   	count
   entries with a matlab_path and a hash	375
   → commit hashes	336 — 286 current, 50 stale
   → blob hashes (PR #61 bug)	35
   → resolve to nothing	4 (all 234c356)

   So 39 of 375 (10%) are not commit hashes at all, not 4. The "wrong kind of hash" side-benefit in the issue body is the larger half of the problem, not an incidental one.

   The 35 blob hashes are exactly recoverable

   A blob hash is the file's content at the moment somebody examined it, so the commit it came from can be recovered rather than guessed — walk the file's history and compare git rev-parse <commit>:<path> to the blob:

   makePyramid   8fff9ff6  ->  261816e8d  "Give a pyramid the subject it was measured from"
   readTileFile  6636493c  ->  f5d4baaec
   extract_doc_files 9dc96e22 -> fe64a9f53
   ...
   recovered 35/35, unrecoverable 0
   

   All 35 recovered, and 24 of them turn out to be current — the port was fine, only the field was the wrong kind of hash. That matters for sequencing: converting them is mechanical and verifiable (each result is checked by construction against the recorded content), it needs no porting judgement, and it shrinks the problem rather than papering over it. It also preserves the real examined-at point instead of bumping to HEAD, which would assert an examination nobody did.

   The 4 234c356 entries (epoch (docTable), treatment (docTable), readtablechar, writetablechar) are not recoverable this way — they resolve to no object at all, so somebody has to say what they were meant to be.

   Corrected staleness, and what it needs

   Once the blobs are converted, genuinely stale is 61 (50 + the 11 recovered-but-behind). Triaging those by what actually changed in the MATLAB file:

   what changed	count	what it needs
   signature / arguments block	16	port review — the recorded input_arguments / output_arguments may now be wrong
   body	43	read the diff; mostly bump-with-examined_at
   comments/docs only	2	safe bump

   The 16 signature-changed are all status: ported, so there is live Python behind each: profile (166 changed code lines), session (69), stimulator (63), dir (58), sessiontable (48), dataset (39), dir (28), updateFileInfoForLocalFiles (22), ndiquery / ndiqueryAll (20 each), advanceSession, startSession, setup.lab, setup.rayolab, clocktype, RayoLabStims.

   A blanket bump of all 61 would be worse than doing nothing — it would convert "unknown" into a false claim of "examined and current," which is exactly the signal this issue exists to create. Only 2 of the 61 are safe to bump unread.

   Suggested order: convert the 35 blobs (mechanical, exact) → land the check so it cannot regress → work the 16 signature-changed as real port review → sweep the remaining 45.

   ---

   Generated by Claude Code
3. stevevanhooser commented on Sep 8, 2026

   

   stevevanhooser

   on Sep 8, 2026 – with Claude

   ContributorAuthor

   More actions

   This is done — every point in the issue, checked against the code rather than assumed

   Built as tests/test_matlab_bridge_hashes.py under #211, which took this issue's proposal cross-repo. Point by point:

   The check itself. has_drifted() runs git log <matlab_last_sync_hash>..HEAD -- <matlab_path> in the NDI-matlab checkout — path-filtered, exactly as proposed — and TestNothingDriftsFromMatlab::test_no_entry_has_drifted fails on a non-empty result, listing the commits.

   Commits-behind, not days-behind. As argued here. Never a timer.

   Gated on NDI_BRIDGE_CHECK_STRICT. And the CI job names the file explicitly (ci.yml:108), against a fetch-depth: 0 checkout — a shallow clone collapses history and makes git log <hash>..HEAD lie.

   The "wrong kind of hash" side benefit. Caught, and better than incidentally: TestEverySyncHashIsACommit::test_no_bridge_entry_records_a_blob checks the object type directly, and TestTheGuardWouldActuallyCatchOne is a positive-control class proving the guard discriminates — a blob is reported, an unresolvable hash is reported, a real commit passes. Those exist because a check that silently stops checking is the failure mode this whole family is about (#77).

   The rollout. Done as a ratchet rather than a big bang, per #211's decision 3:

   	when the gate went up	now
   drifted	60	1
   matlab_path with no hash	147	0

   The one remaining line is downloadNdiDocuments, held open deliberately for #215 (no file-series support in the cloud path, so the drift is pointing at a real missing port). TestTheAllowlistOnlyShrinks fails if a listed entry stops violating, so the list cannot rot.

   The companion rule. test_every_matlab_path_entry_carries_a_hash — an entry with a path and no hash can never show drift, so it claimed to be current forever. That was 147 entries; it is now zero.

   Two things beyond what this issue asked for

   • TestAHashChangeIsJustified, ported from Require a written reason when a hash moves without a port VH-Lab/NDR-python#24. NDR-python#23 observed that merely telling an agent "the hash does not match" tempts it to bump the hash rather than fix the thing. So if a hash changes and none of that entry's python_path files change in the same diff, the decision_log must change too and name the new commit.
   • The sweep found real defects, not just staleness. Roughly one entry in three: startSession missing a now-required organizationId, setup.lab duplicating every DAQ system on a second run, waitForAllBulkUploads ported and never called, a transfer URL never checked for https. The issue predicted bookkeeping; it was more than that.

   One deviation worth recording

   This issue proposed the convention examined_at: <newhash>; deferred: <reason> for a bump where nothing needed porting. What shipped uses the existing decision_log field instead, and #211 decision 4 settled that a regular port needs no prose at all — the bumped hash is itself the record of "examined as of this commit". Substantively the same rule, one fewer field.

   Closable as far as I can tell — every acceptance point is implemented and enforced. Leaving the close to you since you filed it.

   ---

   Generated by Claude Code
4. stevevanhooser

   closed this as completedon Sep 8, 2026