Repository navigation
Hand on the reporting classification with the infectious-agent code sets - #185
Conversation
Get-NeoIPCInfectiousAgentCodeSet now also reads the reporting classification beside the list, or the file -ClassificationPath names, and returns one row per Id with the reporting group and the value of each of the classification's attributes that the node takes, resolved as the classification's checks resolve them, with the classification's SHA-256. Export-NeoIPCInfectiousAgentCodeSet writes them into its JSON, so that another language's reading of the classification, such as neoipcr's, can be held to the toolkit's resolution; an attribute whose Key would take the place of another column of a row, in any letter case, is refused. The export refuses to write over the classification, by its path or by its bytes, as it refuses the list, and both files are read through one helper that refuses a byte order mark, invalid UTF-8, and a path that names no readable file. Both cmdlets now end at any refusal or failure, whatever the caller's error preference, so that an unparsable or unreadable file never leaves a partial export behind, and a caller that silences errors can no longer make the export write over the list it has just refused to replace.
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
Brar
left a comment
There was a problem hiding this comment.
Review at 065c422
The change does what it says on the committed files: 3,453 rows, one per Id, with no finding from the structure, member, or attribute checks, and an independent nearest-placement walk over the list agrees with every row's group and both attributes. The refusals in the description hold as probed (below). The findings are about what the rows do with a file that is not what it should be, and about tests that pass for the wrong reason.
Findings (10 low, 6 nits)
- low: a file that is no classification (an empty file, a stray mapping, the list itself) is taken as one; every row comes back without a group, and without the attribute columns.
- low: the rows repeat the checks' resolution rather than sharing it, and no test ties the two together.
- low: "cannot be read into rows" also wraps failures from the list, naming the classification.
- low:
''rather than$nullfor a missingValueor groupKey; a key-less group's member hands on its ownValue. - low: a path on a drive of another provider is checked against the location and read from the process directory.
- low: the YAML refusals keep the
Exception calling "Load"wrapper and drop the line and column. - low: no test for a clash between two attributes in different letter case; a case-sensitive mutant survives.
- low: the two
ends at a <Case>child-process runs pass when the child never reaches the cmdlet. - low: no test for
-ClassificationPathon a PowerShell drive; a.ProviderPath→.Pathmutant survives. - low: the help's list of refusals leaves out the attribute without a Key.
- nit: the "cannot be read" refusal keeps the
ReadAllByteswrapper. - nit: a 186-column help line, with a clause attached to the wrong noun.
- nit: the Ids 0 and 3561 come out
$nullonly in a classification that passes its checks. - nit: a whitespace or non-text Key is handed on under a made-up name.
- nit: a test name promises rows it does not assert.
- nit:
Convert-InfectiousAgentList.ps1now needs a classification it does not use.
Checked and holding
- The test file.
Tests/InfectiousAgents.Tests.ps1on Linux (Pester 6.2.0, powershell-yaml 0.4.12): 71 tests, 69 passed, 2 skipped as root (the unreadable classification, which root can read, and the Windows-only letter-case test). Run without root's file-access override, as CI's runner is: 70 passed, 1 skipped, and the unreadable-classification test goes red without thetryaroundReadAllBytes. Againstmain's production files, every new test fails (34 passed, 35 failed). - The whole NeoIPC-Tools suite, excluding the
yaml12andruameltags as CI does: 2,629 tests at this head (2,576 passed, 48 skipped, 2 not run, 3 failed) against 2,597 at the base. The same three fail at both, for reasons of this environment (two need a.gitdirectory, one an R summary table), and the one extra skip is the root-only test. PSScriptAnalyzer gives the same 8 diagnostics at base and head; the 8 changed files are LF, UTF-8 without BOM, andgit diff --checkis clean. - Refusal 4. Missing, BOM, invalid-UTF-8, directory, keyless-attribute, and invalid-YAML classifications, and the list-path, classification-path, and identical-bytes refusals, each end the call with their message and write nothing, under a global
SilentlyContinueorIgnore, under-ErrorAction SilentlyContinueorIgnoreon the call, and under$PSDefaultParameterValues['*:ErrorAction']. The comment at lines 213 to 215 holds: without thetryaround the classification parse, the export writes under the caller'sContinue. - Refusal 2. The path check refuses the output as the classification by absolute, relative,
sub/..,.//, and drive path; the byte check refuses it through a symbolic link, a symlinked directory, and an identical copy. Directories namedw[1],w*x,w?y, andw`zwork throughout, and a directory given as list or classification is refused as "no file". - JSON shape. A list without Ids gives
"Classification": [], a single Id an array of one,$nullstays JSONnull, Ids are numbers, and the rows sort numerically. - Mutants. The description's 34 are caught. Of the further mutants tried, those in items 7 and 9 survive and matter; the other survivors are equivalent, or touch only a case the structure check reports.
- CI at this head: every check green (release-assets skipped).
Question
The description says neoipcr's parity test holds its reading to this export, but neoipcr's main (c6de50b13254b162484427adeeffe08b3d98331e) has no reader of this JSON yet, so until that test lands, the cmdlets' own refusals are the only guard against item 1. Will it read the committed classification itself, or the file the export was given, and will it run against a released list? infectious-agents-v0.1.0-alpha ships no classification, so Get and Export now refuse that list, as the README says.
This comment is the sign-off; a formal Approve is not available on one's own pull request. No merge approval from me, and I hold no instruction to merge this.
🤖 Reviewed by Claude Code
Generated by Claude Code
…what no row can come from The code sets' rows now come from the very resolution the classification's member and attribute checks judge, built by one function for both, so the two cannot drift apart. Each row takes its Id as the list's walker reads it, so that a form of an Id the walker accepts is never blamed on the classification; it hands on null where the classification leaves a group without a Key or an assignment without a Value; and a member always takes its group's Key, never a Value of its own. Get-NeoIPCInfectiousAgentCodeSet refuses a file that is no reporting classification, such as an empty file, the list itself, or one whose Groups hold no group, which would have handed on every Id in no group or every row without the attributes' columns; and an attribute that is no mapping, whose Key is no text with a word in it, or whose Key turns into a name PowerShell reserves, PSTypeName among them, which took a row's type name in place of its column. A path on a drive of a provider other than the file system, whose provider path .NET takes for a file relative to the process's directory, is refused wherever the infectious-agent cmdlets, the text-file reader, and the module's write point hand a path to .NET: an output path on the Variable drive had written a file into the process's directory. A path that cannot be resolved, such as one on a drive that does not exist, is refused with its reason, and a refused path is never handed on, so that nothing is written where a caller's SilentlyContinue drops the refusal. Add-NeoIPCInfectiousAgentUid runs under Stop, as the code-set cmdlets do, so that its refusals end the call whatever the caller's preference: under SilentlyContinue it had written the sidecar it refused. A refusal of a file that is not valid YAML names the line and the column where the parser stopped, or the end of the file, and neither it nor the refusal of a file that cannot be read carries PowerShell's wrapper around the .NET exception. The help lists every refusal, and Convert-InfectiousAgentList.ps1's help names the classification its directory must hold.
|
All sixteen threads are addressed in 23a728f and resolved. Beyond the threads, the same commit:
The four other places in the module that hand a path to .NET in the same way (the release check's read of the list's CHANGELOG, the country plan's two, and the onboarding's metadata directory) lie outside this change and are recorded with the module's other path defects. On the question: neoipcr's parity test reads neither YAML file. Its Tests: |
There was a problem hiding this comment.
🔵 Needs a closer look
Human confirmation is appropriate because the change relies on Windows-specific provider resolution, YAML parser exception details, and cross-process error-preference behavior that cannot be independently reproduced here.
No open issues 🎉
Review effort: Lite
The entry for Get-NeoIPCInfectiousAgentCodeSet promised a reporting group, a Gram reaction, and an oxygen requirement for every Id. It now says, as the list's README does, that the attributes concern the bacteria, that a value is null where the classification gives none, and that the cmdlet reads the classification beside the list unless -ClassificationPath names another and does not run without one.
|
Copilot's review of
All checks passed on |
The test on the committed files held every node to a leaf group and to values that exist, which a resolution placing nodes in the wrong ones would still pass. Six well-known infectious agents, their groups and values read from the two files by hand, now pin what the code sets hand on: Staphylococcus aureus, Bacteroides fragilis, Escherichia coli, Pseudomonas aeruginosa, Candida albicans, and Streptococcus agalactiae.
|
Copilot's review of Read as that test checking only that every node takes a leaf group and values that exist, it is addressed in 4d7ecfb: a mutant that gives every node the first leaf group's |
|
Copilot's review of It was probed through |
What This Changes
Get-NeoIPCInfectiousAgentCodeSetnow also reads the reporting classification beside the list(
NeoIPC-Infectious-Agent-Reporting-Groups.yaml), or the file-ClassificationPathnames, and returns, for everyId, its reporting group and the value of each of the classification's attributes that the node takes (its Gramreaction and oxygen requirement), resolved through the very functions the classification's own checks use, with the
classification's SHA-256.
Export-NeoIPCInfectiousAgentCodeSetwrites them into its JSON.neoipcr is moving from its legacy tables to the released infectious-agent list and its reporting classification, and
its parity test holds its own reading of both to the toolkit's through this export. The infectious-agents release
0.2.0-alphafollows this change, since its CHANGELOG entry belongs to that release; the list's YAML is not touched.Refusals
Groupshold nogroup, is refused, with the file named.
Keyis refused, and so is one whoseKeyis no text with a word in it,would take the place of another column of a row, in any letter case, or turns into a name PowerShell reserves,
such as
PSObjectorPSTypeName.Read-NeoIPCTextFile, which refuses a byte order mark, invalid UTF-8, anda path that names no readable file. A file that is not valid YAML is refused with the line and the column where
the parser stopped, or the end of the file; neither that refusal nor the refusal of an unreadable file carries
PowerShell's wrapper around the .NET exception.
relative to the process's directory, or a path that cannot be resolved, is refused wherever these cmdlets, the
text-file reader, and the module's write point hand a path to .NET, and a refused path is never handed on.
Add-NeoIPCInfectiousAgentUid, end at any refusal or failure, whatever the caller's errorpreference, so an unparsable or unreadable file leaves no partial export behind, and a caller that silences errors
can no longer make the export write over the list it has just refused to replace, or make
Add-NeoIPCInfectiousAgentUidwrite a sidecar it has refused.Testing
Tests/InfectiousAgents.Tests.ps1: 106 tests pass on Windows, among them the cases for each refusal, a relative-ClassificationPathbeside a decoy in the process directory, directory paths, an unreadable classification,drives of other providers and drives that do not exist, and child-process runs under
ContinueandSilentlyContinue;Tests/RelativePathWrites.Tests.ps1: 11, among them the module's write point refusing a pathoutside the file system, under
SilentlyContinuetoo. The new tests were shown failing first.For Reviewers
The documentation changes are the module README's rows for both cmdlets and for the text-file helpers, the
infectious-agent list's README and CHANGELOG, the metadata generation design note's paragraph on the code sets, and
Convert-InfectiousAgentList.ps1's help, which names the classification its directory must hold.