Repository navigation
Merge main into V2_Update - #203
Merged
Merged
Conversation
…rals (audit 6.1-3)
Several sqlitedb queries interpolate branch_id / doc_id / document_id /
filename directly into SQL string literals delimited by double quotes, e.g.
['... WHERE doc_id="' document_id '"']. run_sql_query() does not forward bind
parameters to mksqlite (it calls do_run_sql_query without varargin), so these
values cannot be passed as '?' placeholders; a value containing a double quote
could break out of the literal and inject SQL (e.g. a crafted branch name or
document id).
Add a private escapeSqlLiteral helper that doubles embedded double quotes
(inside a double-quoted token SQLite reads "" as an escaped "), mirroring the
DID-python _sql_escape (which doubles single quotes for its single-quoted
literals), and apply it at all 15 value-interpolation sites
(do_add_branch / do_delete_branch / do_get_branch_parent / do_get_sub_branches
/ do_get_doc_ids / read / remove_doc / open_doc / file path lookups). The
existing parameterized run_sql_noOpen('... =?', val) call sites are unchanged.
Lockstep with the DID-python field-name validation (same audit item 6.1-3).
Authored without a local MATLAB runtime; needs MATLAB to validate/run.
Maps the §6.1 finding addressed here (SQL-literal escaping, 6.1-3) to its commit, notes that isa (6.1-1) and the sqlite indexes (6.1-4) needed no MATLAB change (MATLAB is the correct reference / already has them; Python caught up), and records the timestamp-format DECISION (§6.1-2/§7.3-13: datenum vs epoch seconds in the same column) as a cross-client choice deliberately NOT changed here, pending joint sign-off. Reiterates author-not-run + the lockstep merge with audit/did-python-2026-06.
Adversarial review counted 14 escapeSqlLiteral call sites in sqlitedb.m, not 15 (the earlier count of 15 included a docstring-example occurrence that was later reverted). All vulnerable double-quoted interpolations are still covered.
…6 ecosystem audit; lockstep + deferrals)
The bare-named tools/tasks/codecheckToolbox.m sits on the genpath'd
tools/ directory and shadowed matbox.tasks.codecheckToolbox, so the
check-code@v1 action's codecheckToolbox("FoldersToCheck","src") call
resolved to the local zero-arg wrapper and failed with "Too many input
arguments". Packaging it as didtools.codecheckToolbox removes the
shadow while keeping the developer convenience. Mirrors the fix already
made on the V2 branch.
Reverts the package-move approach (which let CI fall through to matbox with CreateBadge=true and fail in createBadgeSvg: 'Python commands require a supported version of CPython'). Instead keep the bare-named wrapper that the check-code@v1 action resolves to, but accept and forward the new name-value args (FoldersToCheck/src) while forcing CreateBadge=false. Fixes both the original 'Too many input arguments' error and the CPython badge error without changing the workflow.
Audit remediation: SQL-literal injection escaping (+ codecheck CI fix)
[CI] Update source of codespell action
The depends_on schema validation required every dependency defined in the schema to be present in the document, even when the schema marked the dependency as optional (mustbenotempty = false). Adding a document that omitted such a dependency raised DID:Database:ValidationDependsOn. Only dependencies flagged mustbenotempty are now required to be present. When an optional dependency defined in the schema is missing from the document, a DID:Database:MissingOptionalDependency warning is issued (including the document's class_name) instead of an error, so the condition can be tracked without blocking database adds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013A8gJ2chzebBsYMeTtrMpY
Prints the class name, superclass flag, whether the document has a depends_on field, and the dependency names found, each time the depends_on validation case runs. Temporary; to be removed once the missing-warning behavior is understood. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013A8gJ2chzebBsYMeTtrMpY
Appends each [DIAG depends_on] line to ~/Desktop/did_depends_on_diag.txt so the output can be collected and de-duplicated rather than scrolling past in the command window. Temporary; to be removed with the rest of the diagnostic. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013A8gJ2chzebBsYMeTtrMpY
The diagnostic identified that the DID:Database:MissingOptionalDependency
warning was being suppressed by a global warning('off') in NDI-matlab's
ndi.dataset.dir during bulk document add, not that the warning was
failing to fire. Reverting the debug instrumentation now that the cause
is understood; the optional-dependency fix and warning remain.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013A8gJ2chzebBsYMeTtrMpY
Callers such as NDI-matlab's ndi.dataset.dir wrap bulk database.add
calls in a global warning('off'), which was swallowing the new
DID:Database:MissingOptionalDependency warning. Temporarily enable the
specific warning identifier (a per-id state overrides the global off)
before issuing it, then restore the caller's prior warning state, so the
condition remains visible during dataset downloads.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013A8gJ2chzebBsYMeTtrMpY
The MissingOptionalDependency warning previously forced itself through
any global warning('off') unconditionally. Make that override opt-in so
it stays out of normal releases: the warning is now only forced to
display when the environment variable DID_FORCE_VALIDATION_WARNINGS is
set to a non-zero (or non-numeric) value. When the variable is unset or
0, the warning respects the caller's warning preferences as usual.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013A8gJ2chzebBsYMeTtrMpY
Previously the warning was always issued and only its forced display was
gated, so in a normal warnings-on session it would surface by default.
Gate the entire warning emission behind DID_FORCE_VALIDATION_WARNINGS so
the default (unset or 0) is fully silent, as intended for normal
releases; the warning is only issued (and forced through any global
warning('off')) when the variable is set to a non-zero value.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013A8gJ2chzebBsYMeTtrMpY
With missing optional dependencies (mustbenotempty:0) now allowed rather
than errored, four modification cases that TestInvalidModification
expected to be rejected are valid under the new contract:
- remover item1/item2/item3: drop one optional dependency from a demoC
document
- dependency_modifier 'invalid name': rename an optional dependency,
leaving the declared name absent plus an unrecognized (ignored) entry
Move these cases from TestInvalidModification (verifyTrue) to
TestValidModification (verifyFalse). 'invalid id' stays invalid because
it sets a dependency value to a non-existent document id, which still
fails the dependency-exists check.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013A8gJ2chzebBsYMeTtrMpY
Cover the DID_FORCE_VALIDATION_WARNINGS branch in
did.database.validate_doc_vs_schema that codecov flagged as untested:
- testWarningIssuedWhenEnabled: with the override on, adding a demoC
document missing an optional dependency (item1) issues the
DID:Database:MissingOptionalDependency warning and still adds the doc.
- testNoWarningWhenDisabled: with the override off, the same document
adds without raising that warning (the default, release-safe path).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013A8gJ2chzebBsYMeTtrMpY
The opt-in warning fired correctly, but add_docs then errored with
DID:Database:ValidationFieldInteger because a bare did.document('demoC')
leaves demoC.value as the template default empty string, which fails the
schema's integer type check. Construct the document with a numeric value
(as the other document helpers do) so the only notable condition is the
missing optional dependency.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013A8gJ2chzebBsYMeTtrMpY
…r-baaafe Distinguish required vs optional schema dependencies in validation
A file location hosted at an http(s) URL was rejected during validation, so a document whose schema marked that file mustbenotempty could not be added at all -- add_docs failed with "Missing file" and wrote nothing. Two things were wrong. The HEAD pre-check had never worked: it called req.send(url), and url is never assigned anywhere in database.m, so the error was swallowed by the bare catch and the branch could never set found. And singling out http was inconsistent with every other non-local scheme -- s3://, ftp:// and anything else fell through to the else branch and were accepted without a check. Removed rather than repaired, so a URL behaves as any other non-local location: not pre-checked here, evaluated when the file is read. Repairing it would put a network round-trip inside validation and would need a matching URL download path in DID-python. The shipped demoFile.json template carries location_type "url" entries, so remote locations are a first-class part of the document format and rejecting them was never the intent. DID-python's can_find_one_file is changed in lockstep on its claude/did-bridge-coverage-wx9pzu branch, with coverage in tests/test_validation.py:TestRemoteFileLocations. There is no MATLAB test for this path yet; worth adding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EtYz3hup2PyrHf9U6T9r3P
do_open_doc's 'url' branch called ndi.cloud.api.files.getFile directly, which made DID depend on NDI being on the path -- a lower-level package reaching for a higher-level one -- for a case NDI already covers. NDI's didsqlite passes 'customFileHandler', @download_file_from_cloud, which resolves ndic://<datasetId>/<fileUid> by minting a pre-signed URL and downloading it. Every non-'file' location type now goes to that handler, or errors with DID:SQLITEDB:FileRetrieval:UnsupportedType when none is supplied. Only ndic:// is supported; plain http(s) URLs are deliberately not handled by DID, which downloads nothing itself. DID-python's open_doc gains the matching custom_file_handler parameter on its claude/did-bridge-coverage-wx9pzu branch, so both languages have the same contract: fn(destPath, sourcePath) must produce a file at destPath, and both check that it did. Not addressed here: do_add_doc's ingestion path also calls ndi.cloud.api.files.getFile for a non-'file' location, so the NDI dependency is reduced but not gone. do_add_doc takes no handler parameter, so closing that one means plumbing one through add_docs and the abstract base. It is rarely reached, since ingest defaults to 0 for url and ndicloud locations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EtYz3hup2PyrHf9U6T9r3P
do_add_doc's ingestion path was the second place DID called ndi.cloud.api.files.getFile, so removing it from do_open_doc reduced the dependency on NDI without eliminating it. This closes it. add_docs gains a 'customFileHandler' name-value pair and forwards it to do_add_doc alongside OnDuplicate; the abstract declaration already takes varargin, and sqlitedb is the only implementer, so no other signature changes are needed. A non-'file' location marked for ingestion is now handed to that handler, called as HANDLER(DESTPATH, SOURCEPATH) and checked for having produced a file -- the same contract do_open_doc enforces. With no handler supplied the ingest fails with a clear message rather than an undefined function error, which is what happened when NDI was not on the path. Ingest failure stays non-fatal: the branch is inside the existing try/catch, so a throwing handler is converted to the same warning as before and the document is still added. delete_original now skips any source carrying a '://' scheme. A remote location is not ours to remove, and delete() on such a string would either do nothing or match something unintended on disk. It defaults to 0 for 'url' and 'ndicloud', so this was latent rather than live. Remote ingestion is expected to be rare -- ingest defaults to 0 for remote location types -- but it is supported rather than refused. NDI needs a matching one-line change to benefit: its didsqlite.do_add calls db.add_docs(ndi_document_obj,'a') and should pass 'customFileHandler', @download_file_from_cloud, as it already does at open_doc. Until then remote ingestion is inert rather than broken, and nothing in NDI implements do_add_doc or subclasses did.database, so the new name-value pair breaks nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EtYz3hup2PyrHf9U6T9r3P
… do_add_doc codecov/patch failed on PR #156: the retrieval branches added to do_open_doc and do_add_doc had no tests. DID-matlab had none for this path at all, which is how the ndi.cloud dependency sat there unexercised. Adds did.unittest.TestCustomFileHandler covering both sides of the contract: ingestion through a handler, ingestion with no handler supplied (warns, and the document is still added -- ingestion has always been non-fatal), a handler that produces no file, a handler that throws, open_doc retrieving through a handler, and open_doc erroring when none is supplied. Each test gets its own working folder and database. Also covers the delete_original guard: without it, delete() on a location carrying a '://' scheme warns that the file was not found, so verifying the add is warning-free is what shows the guard works. Separately, per review: the abstract do_add_doc declaration still took varargin, which this repo has otherwise moved away from in favour of arguments blocks. An abstract declaration is a signature only and cannot carry an arguments block itself, so the contract is stated by naming the trailing argument `options`, matching the implementation that declares it. sqlitedb is the only implementer and its arity already matches, so nothing else changes. The remaining abstract declarations in that block still use varargin; those were left alone since each needs its implementations checked separately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EtYz3hup2PyrHf9U6T9r3P
Retrieve remote files through customFileHandler only, removing DID's dependency on NDI
The symmetry suite covered buildDatabase only, whose demoA/demoB/demoC documents declare no files section, and it compares database summaries, which carry no file information. So nothing in it touched the file subsystem: no document had a file, nothing called open_doc, and the compared summary had nowhere to show a difference. That blind spot hid a complete break in the other direction: DID-python created the files table but never inserted into it, and do_open_doc here resolves a file only by selecting from docs, files -- it never reads locations back out of the document JSON -- so every file in a Python-written document was unreachable from MATLAB, including a plain local file sitting on disk. Fixed on the Python side in VH-Lab/DID-python#31; this is the coverage that would have caught it. Adds a fileDocument symmetry pair mirroring DID-python's. The maker builds a demoFile document with both declared files, each holding ten deterministic bytes (file i holds i*10..i*10+9, the same rule on both sides), writes a manifest recording the document id and expected bytes, and self-checks through open_doc before claiming anything. Note this exercises MATLAB's ingestion path too: by the time the self-check runs the originals have been ingested into FileDir and deleted, so it also confirms the files table is doing the work. The reader is parameterized over both artifact sources and uses assumeTrue rather than a hard failure when the artifact is absent. Each repository's symmetry job checks out the other's main, so failing would mean neither half could land first; assuming lets them merge independently and the cross-language check becomes real once both are in. This is the ordering trap that made #156 and NDI-matlab#894 sequential, avoided here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EtYz3hup2PyrHf9U6T9r3P
Exercise file-bearing documents in the symmetry suite
Neither class had any test coverage — the only tests are unconverted
script-style ones under tests/+did/+test/_old, which nothing runs. That
is how the bugs below survived. Both classes are on the document-open
path: fileCache is built on binaryTable, and sqlitedb.do_open_doc
consults the cache and touches what it finds there.
Found while porting both classes to DID-python, where the corresponding
behaviour is now covered from the other side.
fileCache:
- Reopening a cache without passing fileNameCharacters left the
binaryTable built at the default width of 32 over a file whose rows are
33 wide. The header carries the width, so the scalar properties came
back correct and only reading a row showed the damage — the catalog was
read at the wrong offsets. The table is now rebuilt once the stored
width is known.
- resizeAndAdd indexed its row cell by the loop variable rather than by
the column (data_here{i} where data_here{1} is meant), so the second
file's name was written into the last-accessed column and immediately
overwritten. Adding more than one file at a time catalogued every file
after the first under the first file's name.
- removeFile's "not in file cache manifest" error referenced an undefined
variable, so it raised "Unrecognized function or variable 'filename'"
instead of naming the missing file.
- setProperties could reach readHeader with binaryTable still 0.
binaryTable:
- readRow(Inf, col) reshaped by the row count, so an empty table divided
by zero. fileCache.fileList calls straight into this.
- insertRow accepted insertAfter == rows+1 and then took the copy branch,
writing the new row past the end of the data. deleteRow accepted
rows+1 and rewrote the file having deleted nothing.
sqlitedb.do_open_doc:
- A cache hit called touch() with the file's full path, but touch looks
the name up in the catalog, whose column holds the name inside the
cache. It never matched, so a hit never refreshed the access time and
eviction dropped exactly the files that are read most.
Tests: TestBinaryTable (24 tests) and TestFileCache (18) cover the typed
read/write path, the search and its insertion points, eviction order, and
each bug above. Both assert the bytes on disk with plain fread rather
than only round-tripping through the class, because that layout is a
cross-language contract — DID-python reads and writes the same file.
The first run of the new tests found this. headerSize is declared uint16,
and MATLAB gives an integer/double expression the integer class, so every
byte position computed from it was uint16 and saturated at 65535:
dataSize = sz - headerSize
beforeBytes = headerSize + insertAfter * rowSize()
fseek(file, headerSize + (row-1)*rowSize() + ..., 'bof')
Past 65535 bytes of data the row count stops rising, seeks land on the
same byte, and insertRow and deleteRow copy the wrong number of bytes.
With the file cache's 49-byte rows that ceiling arrives at about 1337
files -- well inside a cache whose default limit is 100 GB.
It also made findRow's return type depend on which branch answered: the
unsorted scan returned uint16 (its loop ran over a uint16 row count)
while the binary search returned double.
Every offset expression now works in doubles, and rowSize() states that
it returns one. testATableLargerThanTheOldUint16Ceiling builds a
2000-row table and checks the count, the last row's seek, a full-column
read and a binary search across it.
Also fixes three bugs in the tests themselves, all mine:
- makeSource multiplied a uint8 by ones(1,n), which MATLAB rejects as
mixed classes. This errored 9 TestFileCache tests -- and made two
others pass for the wrong reason, since the error was raised inside the
verifyError closure they were using to assert something else.
- makeTable reused one filename without clearing it, so a test that built
a table twice appended to the first one and searched a table twice the
size it expected.
- testRowSize expected uint16; sum() over an integer array returns double
unless 'native' is asked for.
The run did confirm the three fixes it could reach: resizeAndAdd now
gives each file its own row, fileList no longer divides by zero on an
empty cache, and removeFile names the file it could not find.
setProperties' "is the binaryTable built yet" check cannot fire. Reaching that branch means the info file exists, and every route to it -- the constructor, addFile, removeFile, clear, resizeAndAdd -- passes through getProperties or through the branch above first, and both build the table. getProperties keeps its guard, because that is the one that runs. Codecov flagged it: 4 of the 6 uncovered lines in this PR were here, and they are uncovered because nothing can reach them, not because a test is missing. Two guards for one condition, one of them dead, is worse than one guard in the place that needs it. The two remaining uncovered lines are the touch() call in sqlitedb.do_open_doc's file-cache hit. Covering them means putting a file in did.common.PathConstants.filecachepath, which on a developer machine is the real cache directory under userpath -- a test with a side effect outside its own working folder. Not worth two lines.
Pass document context to customFileHandler so NDI can batch-presign
A file series member whose bytes were absent could never be retrieved,
even when the caller supplied a customFileHandler that could reach them.
seriesMemberPath resolved a member as far as its uid and stopped, because
a member has no orig_location of its own -- the per-member record a series
deliberately does not keep. open_doc then reported it absent. That is the
last piece that makes a downloaded series readable: NDI's round trip
checks the manifest and the counts, so it looks complete while
open_doc(docId, 'chunkdata.bin_7') still fails.
A member does not need a location of its own. The SERIES MANIFEST has one,
and a handler that can reach the store the manifest came from can reach a
sibling object in it given the member's uid. So the manifest's row supplies
sourcePath and the member's uid travels in the handler context. DID
composes no URL and learns no scheme.
seriesMemberPath's retrieveManifest option becomes mayRetrieve, since it
now gates the member too, and step 4 calls the new protected
fetchSeriesMemberBytes. That helper reuses do_open_doc's retrieval
primitives -- the per-uid fetch lock, the unique '.part' name, the cache
addFile with its race fallback -- in the same order and with the same
arguments, and points at do_open_doc for the reasoning rather than
restating it. It never raises: seriesMemberPath is called speculatively by
check_exist_doc and by do_open_doc's fallback, and both need a plain miss.
check_exist_doc still passes mayRetrieve false and so still answers a
question about local state without going to the network.
seriesName now reaches a handler at open time, where it was populated only
at ingest, which is the other half of the issue.
TWO WAYS THIS WOULD BE SILENT WRONG BYTES, both refused. Neither is an
ordinary failure: the result is cached under the member's uid, where no
later read can tell it from the real thing.
* A manifest whose own location is an ordinary local file is never
offered to a handler, which might simply copy what it is given.
* sourcePath names the MANIFEST, so a handler that resolves it instead
of reading ctx.uid returns the manifest's bytes for every member.
NDI's download_file_from_cloud does exactly that today -- it parses
the uid out of the ndic:// sourcePath -- so this is the live
version-skew case, not a hypothetical. The result is compared against
the manifest we already hold and refused with a warning that names
the mistake. A handler declared with only two inputs is not asked at
all, since it receives no context and so cannot know which member it
is being asked for.
The arity rule dispatchCustomFileHandler dispatches by is named once, as
handlerTakesContext, so fetchSeriesMemberBytes can ask rather than
re-derive it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012YLWu9vpZz1zkDcCG2PgTx
Fetch a series member that is not on this machine (#188)
docs/notes/file_series_manifest.md stated that "nothing can retrieve one member's bytes from a remote store", deferring it to #173's step 3. That is exactly the claim #188 overturns, so on merge the repo's own design note would describe a feature as absent while the code implements it -- worse than an unwritten note, since a reader would trust it. Replaces that sentence with a section on how a member is fetched: the manifest's location as sourcePath, the member's uid in the context, the mayRetrieve gate that keeps check_exist_doc off the network, and the two silent-wrong-bytes mistakes that are refused. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012YLWu9vpZz1zkDcCG2PgTx
fetchSeriesMemberBytes refused to offer a manifest whose own location was an ordinary local file, so it never dispatched and the member read as absent. Two reasons were given, and neither survives. The first was that handing a handler a local path risks it copying the MANIFEST's bytes into the member's cache slot. That is real, and it is already caught: the byte comparison against the manifest refuses exactly that result, and does not care whether the path the handler was given was local. The type check was a second guard on a failure the first one covers. The second was that a local manifest implies a database with no remote store to fetch a member from. That is false, and false for the shape this mechanism exists to serve. ndi.cloud.downloadDataset syncs a dataset's document files to local paths and deliberately leaves the series MEMBERS on the cloud -- lazily, so a 28,000-member series does not arrive with it -- so the manifest is an ordinary file on disk while every member it names is remote. Under the old rule NDI's handler was never asked, and every member of a downloaded series was unreadable: the feature inert for its main caller, which is how VH-Lab/NDI-matlab#966 found it. Every location is now offered, remote ones first. The ordering is not eligibility -- a handler that can answer from a remote store should not first be handed a local path it might merely copy, since the guard makes that harmless but not free. testALocalFileManifestIsNeverHandedToAHandler asserted the old contract and is inverted rather than deleted: the local-file manifest case is now the one that must dispatch, and it verifies the member's bytes come back. The wrong-bytes case it was protecting against keeps its own test, testAHandlerThatReturnsTheManifestIsRefused, which is where that protection actually lives. Not tested: the remote-before-local ordering, which needs a manifest carrying two locations. Building one turns on how add_file treats a repeated filename and on remove_docs' branch argument, neither of which I could verify without running MATLAB, and a speculative test that fails would obscure the change it sits next to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VMPdbVkfEbjfEviv6dspHN
Correct the file series design note: a member IS retrievable now (#188)
…o-handler Offer a local-file manifest to the handler too (unblocks #188 for its main caller)
#191 removed fetchSeriesMemberBytes' type check, so every manifest location is offered to a handler now, local ones included. Two descriptions of the deleted rule survived it, both written before it went: * the design note's "Two mistakes ... Both are refused" bullets, which still say a local-file manifest is never offered and that a local manifest means no remote store to fetch from -- the exact reasoning #191 showed to be false, and the shape a lazily synced dataset has; * TestSeriesMemberFetch's class comment, which still summarises the suite as pinning that rule while the test below it, testALocalFileManifestIsAlsoHandedToAHandler, now pins the opposite. Neither is a code change. Both are the failure #190 existed to fix -- a note a reader has every reason to trust, describing behaviour the code does not have -- so they are corrected the same way, naming the byte comparison as the guard that made offering local paths safe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012YLWu9vpZz1zkDcCG2PgTx
Follow #191 in the two places that still describe the removed type rule
The symmetry suite covers '.fileCacheInfo' and '<file>-lock' -- the two things both languages already share. The file series manifest is the third and is not covered at all: DID-python#64 marks it a cross-language contract, and DID-python has no occurrence of "series" yet, so the port has nothing to check itself against. Two makeArtifacts halves, no readArtifacts halves. That is deliberate: the read suite asserts nSkipped == 0, so a reader parameterised over pythonArtifacts would go red the moment it landed and stay red until DID-python wrote its half. Generating first keeps CI green and puts the artifacts where the port can use them from day one; the reader halves go in on either side once Python has something to read. file/seriesManifest writes five manifests, each pinning a part of the format an ordinary one would not exercise: a dense baseline; a sparse one whose absent members are all-NUL records; a named one carrying the empty-name EDGES (first, an interior run, last), which is the boundary DID-matlab's own reader got wrong in 5780144 by converting to one-based before comparing; an all-empty name section, which is a different file from no section at all; and one with uid_width 40 fully used, since the width is a header field rather than the 33 did.ido.unique_id happens to produce. The self-check reads every slot through readSeriesManifestUid as well as through readSeriesManifest, so a port cannot pass the whole-file read while mis-computing 32 + i*uid_width and giving away the O(1) index the format was chosen for. database/fileSeriesDocument ingests a sparse series into a real database and pins what a port has to reproduce beyond the bytes: members at FileDir/<uid>; exactly one files-table row, the manifest's, because a row per member would put back the per-member records the manifest exists to remove; ingest_locations present but EMPTY in the stored document rather than removed (87ebcf6, where rmfield made adding a series to a stored document fail at runtime); and NAME_<i> one-based against the manifest's zero-based slots, sparse so an off-by-one lands on a gap or a neighbour rather than shifting everything uniformly. Follows fileDocument rather than INSTRUCTIONS item 4 in writing only manifest.json: a database summary carries no file information, which is why fileDocument does not write one either. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012YLWu9vpZz1zkDcCG2PgTx
Generate file series symmetry artifacts for the DID-python port
Both were noted while sweeping the file series work for #193 and left out of it to keep that change to one subject. file/fileCacheStaleRow is a SEPARATE artifact rather than an extension of file/fileCache, so the healthy pair keeps meaning what it means. It leaves a cache in the two states check() names: a stale row, where the index promises a file that is not on disk, and an orphan, a file on disk with no row. The stale row is the one that matters -- isFile consults the index, so it answers true while the bytes are gone, which is the uid poisoning 4a0f9a5 fixed by having addFile retract the promise. One cache directory and two languages means either can be the one that died mid-write, so whichever arrives next reconciles a state it did not create; they have to agree on what "stale" and "orphan" mean before they can agree what to do. The self-check calls check() with its read-only defaults, so the artifact survives being verified. common/seriesMemberNames is a rule rather than a format, which is why it sits beside pathAgreement. seriesMemberOf decides whether a filename names a series member and everything else rests on it, yet it is implemented per language from prose with nothing to check against. Thirteen vectors, each asserted against the live implementation before being written out: the last-underscore split (chunkdata.bin_1_2 is a miss, which a greedy match would resolve), a declared-but-not-a-series stem (plainfile.ext_1), case-insensitive matching that returns the DECLARED spelling, multi-digit and leading-zero indices, and the misses at the edges. Two things the table states rather than assumes. Validation lives in the CALLER: chunkdata.bin_0 and _-1 both parse, and seriesMemberPath is what rejects them, so a port that rejects in the parser agrees on the outcome while disagreeing here. And MATLAB parses the index with str2num, which evaluates -- so inputs like NAME_1+1 are deliberately absent, since pinning them would oblige DID-python to reproduce an eval it should not want. Every vector present is one where a strict integer parse and str2num agree. makeArtifacts only, for the same reason as #193: the read suite asserts nSkipped == 0, so a reader parameterised over pythonArtifacts goes red until DID-python writes its half. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012YLWu9vpZz1zkDcCG2PgTx
…e-vectors Two more symmetry artifacts: a divergent file cache, and the NAME_<i> parse rule
The half that was missing. Both languages have written series manifests since #173, and DID-python has read both languages' output since then -- but nothing read PYTHON's. So the shared binary format was proven in one direction only, and a DID-python writer that drifted would have been caught by nothing: its own reader agrees with it by construction. The symmetry job's honesty gates could not see the hole. They fail on a SKIP, or on an artifact missing for a test that exists; a reader nobody wrote is neither, so nSkipped == 0 and nPassed > 0 both passed while this leg compared nothing. The makeArtifacts half even anticipated this file -- its uidOf helper is a static "so a readArtifacts half can use the same one" -- and it was never written. Reads whichever language produced the artifacts, over the same five cases the make side builds: dense, sparse, named (the empty-name edges), allEmpty and wideUid. Checks the header fields, every uid, the name section, and that the O(1) slot read agrees with the whole-file read for every slot including absent ones -- that path is why this format was chosen over inline file_info, so a reader that has the two disagree has given the property away while still passing a whole-file read. One past the end must be a miss, not an error. Two shape hazards handled rather than assumed. jsondecode returns a cell array here rather than a struct array, because sourceNames is [] for some entries and a list for others; and uids are compared by CONTENT, since an absent member's uid is the empty string and jsondecode's 0x0 char is the same uid as the reader's 1x0 while verifyEqual would call them different. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Read the series manifests DID-python writes
Completes the four-leg matrix for file series. Two artifacts have been written on every CI run since #173 and nothing has ever opened them: +makeArtifacts/+database/fileSeriesDocument and +makeArtifacts/+common/seriesMemberNames. The symmetry gates could not see it -- they fail on a SKIP, or on an artifact missing for a test that EXISTS, and a reader nobody wrote is neither -- so nSkipped == 0 and nPassed > 0 both passed while two thirds of the series comparison happened nowhere. DID-python's halves landed in VH-Lab/DID-python#82. fileSeriesDocument is the one no single-language test can stand in for. A member has no files-table row and no file_info entry, so resolving one means reading the OTHER language's manifest and believing what it says about slots and uids -- a chain with a one-based/zero-based boundary in the middle. The series is sparse (2, 5, 6 of 6) with distinct bytes per member, so an off-by-one lands on a gap or a neighbour instead of shifting everything uniformly, and a wrong resolution fails rather than passes. It also pins the ABSENCE that is the whole design: one files-table row, the manifest's, never one per member; and ingest_locations present but EMPTY in a stored document, which is the defect 87ebcf6 fixed here and which a port using del rediscovers. seriesMemberNames is a RULE, not a format. seriesMemberOf is written once per language from prose with nothing shared to check against; too permissive turns every NAME_<n> into a member and sends resolution after files that do not exist, too strict makes real members unreachable, and both are quiet. The known deviation is named and CHECKED, not skipped. str2num evaluates, so '-1' parses here and seriesMemberOf returns ('chunkdata.bin', -1) with the index rejected downstream; DID-python's isdigit() declines, so the name is not a member there and the read fails as an ordinary missing file. Both languages refuse it, at different layers, and slots are one-based so nothing can mint it. The table asserts THIS language's answer, so a further drift fails -- and a deviation the other side stops recording fails too, rather than leaving a stale allowance. The artifact directory is copied whole before the database is opened: member bytes live in FileDir beside the database file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Read the file series artifacts this repo builds and never opened
is_in_file_list, seriesMemberOf and localValidateFileDeclarations all used str2num to decide whether the tail after the last underscore is a member index. str2num EVALUATES: '_pi' and '_i' come back as numbers, '_1+1' as 2, '_-1' as -1. That divergence had two costs -- Python's series_member_of uses str.isdigit(), so those names weren't members there; and localValidateFileDeclarations refused declarations that were valid in Python. Switch each site to a strict all-digits check (all(isstrprop(tail, 'digit'))) matching DID-python's parse. Both languages now accept NAME_<i> as a numbered entry only when <i> is a non-negative integer, so NAME_pi and NAME_-1 are literal names in both. The 'shadowedBy' error is no longer raised for a NAME_pi literal beside a NAME series, because the parse it depends on no longer resolves the name -- so the same declaration constructs in both languages. Update TestDocumentFileSeries: the "_pi / _i" case now verifies the declaration constructs rather than that it is refused. Clear the '_-1' deviation from the seriesMemberNames symmetry table, and rewrite the '_-1' vector in the make-artifacts half as a miss in both languages. The readArtifacts deviations() cell is now empty. See VH-Lab/DID-python#69. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SqTZnkGgPBgWKyAyNHGNuv
…allel-65fkut Tighten NAME_<i> parse to strict digits (str2num → isstrprop 'digit')
These tests are committed before the fix so CI -- the only MATLAB this work has -- records them failing against the current code. A test that passes both ways pins nothing. Two of them fail here: a required file declared in the document's file_list with nothing bound to it in file_info is either accepted outright (when file_info is a 0x0 struct array) or rejected with a message naming the file_list (when it is [], where dot-indexing throws and the caller's catch discards the file_list it had already read). A third fails on the name it reports: with the file_list discarded, every declared name looks absent from it, so the message names the first expected file rather than the one genuinely missing from the list. The rest are guards that must keep passing: an optional file need not be bound, a name truly absent from the file_list keeps its own message, a document with no files section still validates, and a document that binds what it requires still validates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WAMNh58UmUcrPcwsioEEN4
validate_doc_vs_schema read file_info and file_list under one try/catch
with file_list last. Dot-indexing an empty file_info throws -- [] is what
a document that binds nothing carries -- so the throw landed BEFORE
file_list was assigned, and the catch replaced a file_list that was
sitting there, readable and correct, with {}. checkfiles then reported
every required name as missing "from the file_list in document X",
naming the one field that was right and sending the reader away from the
actual absence, which is the bound file. Read the two fields
independently instead, so no readable value is lost to statement
ordering, and keep handling a document with no files section at all --
legitimate and common -- by asking whether the field is there rather
than by letting an error stand in for the answer.
"Nothing is bound" arrives in two shapes: [] and a 0x0 struct array from
reset_file_info, where {s.name} yields {} without throwing. They mean
the same thing and are now treated alike, rather than the verdict
depending on which shape a caller happened to produce.
Reading file_list correctly is not enough on its own: with the real
file_list, the reported document would have PASSED. checkfiles' step 3
loops over the match indices of each required name, and a name with
nothing bound has none -- an empty loop, falling through to isvalid = 1.
That is the same fail-open #182 closed for step 1, one step further
down. So checkfiles now separates the two absences: a name missing from
the document's file_list keeps its existing message, and a name that IS
in the file_list with nothing bound for it gets its own, naming the file
and file_info.
Two tests depended on that fail-open: TestCachedPath and
TestFileSeriesRoundTrip each added a demoFile document binding only
filename1.ext, though demoFile declares both files mustbenotempty.
Neither is about validation, so both now bind both files rather than the
check being weakened to admit them.
Hit through NDI-matlab, where ndi.fun.doc.gene.makeSourceFile created
generic_file documents without the generic_file.ext their schema
requires. Those documents were genuinely invalid, and that is fixed on
the NDI side (a new fileReference class for files the database describes
but does not hold). This half is only that the error misidentified which
field was at fault.
Closes #199
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WAMNh58UmUcrPcwsioEEN4
The read added a ischar/isstring -> cellstr normalization on its way to checkfiles. Nothing needs it: setdiff already reads a char list the same as the cell form, and the code this replaced passed file_list through as stored. It was a speculative branch on a validation path, untested and with no behaviour to test -- so it goes rather than gaining a test that would pass with or without it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WAMNh58UmUcrPcwsioEEN4
…agnosis Blame the unbound file, not the file_list, when a declared file has nothing bound
A series downloaded with SyncFiles=false lands with its manifest's file_info naming an ndic:// address and nothing on this machine. seriesMemberPath's local-only manifest lookup then returned '' and every member open reported the member as absent, even when a handler could have served the manifest. Add sqlitedb.fetchSeriesManifestBytes: one level up from fetchSeriesMemberBytes, same call shape. Reads the manifest's file_info straight from the document, so no SQL and no recursion into do_open_doc; offers every non-'file' location to the customFileHandler with the manifest's uid in the context and mode='open'; lands the bytes at filecachepath/<manifestUid> under the same single-flight lock the member fetch uses. The next cachedPathForUid is a hit, so a second member open on the same series pays no network for the manifest again -- whether the next call is this member, another member, or a later session. seriesMemberPath's fast local path is unchanged; retrieval sits behind the same mayRetrieve gate check_exist_doc already respects. When the handler cannot service the manifest's location, open_doc still reports the member as "not on this machine" through the current DID:SQLITEDB:open shape, so callers that pattern-match on the message keep working. localSeriesManifestPath and cachedPathForFile stay no-SQL, no-network -- the promise that lets a viewer resolve files from any thread and any process without a session per worker. Companion port: VH-Lab/DID-python#87. This unblocks the NDI-matlab workaround in ndi.cloud.sync.internal.updateFileInfoForRemoteFiles, which will retire in a follow-up NDI-matlab change once this lands. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015dKpDXCo8VTS4AHiibNnCp
… file"
The test I added on top of the previous commit failed on CI: when the
handler cannot serve the manifest, seriesMemberPath was returning with
memberOf='', so do_open_doc reported the member as a name it did not
recognise -- "does not include a file named chunkdata.bin_2" -- rather
than as a member of a declared series whose bytes could not be reached.
The current message shape that callers pattern-match on ("not on this
machine") was not raised, and #201 acceptance case (c) failed.
The fix is to set memberOf=stem when the filename parses as a member of
a declared series and the manifest cannot be read to rule the member out.
Case C, "manifest was read, slot is empty" (testMemberBeyondTheSeriesDoesNotExist,
testSparseSeriesResolvesOnlyThePresentSlots), still returns memberOf='':
we have positive evidence the slot is empty, so "no such file" is the
truthful message. Case B (manifest OK, bytes missing) and case D
(manifest unreachable) both give the "member of file series X, ..., not
on this machine" error do_open_doc already emits.
memberOf's docstring is updated to name the three states rather than
just the "bytes missing" one.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015dKpDXCo8VTS4AHiibNnCp
Fetch a remote-only series manifest through the customFileHandler (#201)
Contributor
Author
|
Merge landed directly on
Design follow-up filed as #204: make Closing this PR — it was preview-only. Generated by Claude Code |
Merged
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
4 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.
Bring
mainforward into a newV2_Updatestaging branch so V2 work can pick up fixes that have landed onmainsinceV2diverged.Branch setup
V2_Updatewas just created fromV2(8f58f88, 2026-07-05). This is the first main→V2 sync since V2 diverged from main, so there is no prior sync commit to build on (unlike NDI-matlab'sVnext_Update).Since divergence (2026-07-05 → 2026-09-14)
mainat8bea92c. Notable landings onmainsince V2 branched:customFileHandler(Fetch a remote-only series manifest through the customFileHandler (#201) #202):sqlitedb.fetchSeriesManifestBytessits one level up fromfetchSeriesMemberBytesand offers a series manifest'sndic://location to the custom handler, so a remote-only series can resolve members without the NDI-sideupdateFileInfoForRemoteFilesworkaround. Companion to NDI-matlab work retiring that workaround.file_infoandfile_listunder one try/catch collapsed a[]dot-index throw onto a goodfile_list, so the validator reported the wrong field as missing. Reads are now independent; also splits "name absent from file_list" from "name in file_list but nothing bound", which used to fail-open at step 3 ofcheckfiles.is_in_file_list,seriesMemberOf, andlocalValidateFileDeclarationsmoved offstr2num(which evaluates'_pi','_i','_1+1') toall(isstrprop(tail,'digit')), matching DID-python'sstr.isdigit(). Restores cross-language symmetry.Where conflicts are likely
The three DID PRs above all touch schema/validation and series-handling code that V2 has been redesigning. Expect real (not badge/lint) conflicts in:
+did/+sqlitedb/*— the series manifest fetch path.validate_doc_vs_schema/checkfilesin main; V2's may be reorganized.is_in_file_list/seriesMemberOf/ any equivalent V2 rename — the strict-digits parse should be preserved wherever the logic lives on V2.Not to merge as-is
For review only. Actual sync will be
git merge origin/mainonV2_Updatelocally, resolving conflicts, then push.Generated by Claude Code