[PR-07b-code] Review feedback, CIs, Validation Report build-out, NeoIPC-Tools integration (code only)#63
Merged
Merged
Conversation
…PC-Tools integration (code only)
Second Surveillance-Toolkit PR in the cross-repo PartnerReport
upstreaming effort. Picks up the post-feedback report iteration that
followed PR-07a-code's JSON-output milestone: integrates the partner-
side review feedback into Reference and Partner reports, finishes the
Validation Report build-out, wires confidence intervals through the
report layer, and integrates the NeoIPC-Tools PowerShell module set.
Code-only scope: this PR ships the source-file deltas (`.qmd` /
`.Rmd` / `.R` / `.ps1` / `*.yml` / `*.yaml` / `*.lua` / `*.tex` /
workflow / module). The translation-artefact deltas produced by po4a
land separately as PR-07b-translations.
What the PR delivers
--------------------
* Reference-Report iteration. Post-real-life-testing rewrite of the
Reference-Report tables (secondary-BSI rates now properly
localized; surgical-procedure intro rewritten; per-table headers
hooked through the `sR$<tbl-...>` namespace); title-page +
single-country edge-case fixes; methods-texts toggle.
* Partner-Report iteration. Layout + fine-grained CI control + methods
texts; partner-feedback fixes for the antibiotic-resistance,
infectious-agent-detection, and risk-density tables. Final-touches
pass after partner real-life rendering of multiple sites.
* Validation Report. Revived for the current neoipcr API shape;
department / hospital / country resolution wired through the
metadata cascade; exclusion-of-test-data default; explicit
`-IncludeTestData` switch; `Get-NeoipcDepartments
-ExcludeTestUnits` plumbed through. Per-rule mapping +
formatter scaffolding for the rule cohort introduced in PR-08.
* Confidence intervals at the report layer. Both Reference and
Partner reports now render CIs alongside the point estimates from
neoipcr's rate-table builders; the `includeConfidenceIntervals`
parameter switches between full / pooled-only / off so output
size stays in scope when CIs aren't needed.
* NeoIPC-Tools PowerShell module set. Integrated under
`scripts/modules/NeoIPC-Tools/` as a vendored module: cmdlets for
reading event info, departments, hospitals, infectious-agent
metadata; `Find-NextFreeInfectiousAgentId` for the
infectious-agents authoring workflow; localized message
string-resources per script.
* Multi-DHIS2-instance polish. Wrapper scripts and `_setup.qmd`
files accept the `dhis2Scheme` / `Hostname` / `Port` / `Path`
parameter family consistently; `Resolve-NeoipcLocaleQmd`
centralises the per-locale wrapper discovery; auth flow is
routed through neoipcr's env-var contract.
* Toolchain polish. Several PowerShell wrappers consolidate on
the `Invoke-WithNeoipcAuth` + `Invoke-QuartoRender` helpers
introduced in PR-07a-code; `Update-Po4aYamlKeys.ps1`
empty-keys + comment-anchor hardening; `Test-PoPlaceholders.ps1`
introduced for placeholder validation.
The deferred concerns flagged in PR-07a-code's review cycles are
addressed here — `_tbl-secondary-bsi-rates.qmd` is rewritten with full
`sR$` localization; `_tbl-intro-surgical-procedure.qmd` is removed (it
was misleading prose, replaced with corrected text in the table chunk);
the Partner-Certificate `\selectlanguage{british}` hardcode is
replaced by a babel-language-name lookup. See
`tmp/upstreaming-review-log.md` for the cumulative deferral set.
There was a problem hiding this comment.
Pull request overview
This PR continues the Surveillance-Toolkit upstreaming effort by iterating on report rendering (incl. Validation Report build-out and localization/CI plumbing) and integrating vendored PowerShell tooling modules (NeoIPC-Tools / NeoIPC-BuildTools) into the scripts/ workflow.
Changes:
- Updated multiple PowerShell wrappers to import the vendored NeoIPC-Tools module, and added/adjusted parameters (e.g., locale handling, combined Validation Report mode).
- Added/updated NeoIPC-Tools + NeoIPC-BuildTools module content (DHIS2 HTTP helpers, user query cmdlet, module manifest), plus metadata tooling integration.
- Localization/tooling updates (po4a YAML key updater improvements, Greek locale code
gr → elin Python tooling) and Validation/Reference report content tweaks.
Reviewed changes
Copilot reviewed 166 out of 251 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/Update-Po4aYamlKeys.ps1 | Adds # manual-keys support while parsing po4a YAML entries (key regeneration skip). |
| scripts/Update-NeoipcSiteCache.ps1 | Switches from helper dot-sourcing to importing vendored NeoIPC-Tools. |
| scripts/update-glossary-po.py | Updates default language list / language names (Greek code el). |
| scripts/sync-html-to-po-v2.py | Updates default locales list (Greek code el). |
| scripts/New-ValidationReports.ps1 | Adds combined/per-site mode parameters and switches to module-based helpers. |
| scripts/New-PatientDataReport.ps1 | Switches to NeoIPC-Tools imports; renames Language → Locale and adds -JsonReport. |
| scripts/ConvertFrom-JsonMetadata.ps1 | Imports NeoIPC-BuildTools for metadata conversion pipeline. |
| scripts/modules/NeoIPC-Tools/Public/UserInfo.ps1 | Introduces Read-UserInfo cmdlet for querying DHIS2 users with filtering. |
| scripts/modules/NeoIPC-Tools/Private/DHIS2Http.ps1 | Adds centralized DHIS2 GET/DELETE helpers used by NeoIPC-Tools cmdlets. |
| scripts/modules/NeoIPC-BuildTools/NeoIPC-BuildTools.psd1 | Adds NeoIPC-BuildTools module manifest and export list. |
| reports/Reference-Report/content/_nosocomial_infections.Rmd | Updates Reference Report narrative content and string-resource insertion. |
| reports/Validation-Report/rules/_rule_0033.Rmd | Adds/updates Validation Report rule implementation for rule 33. |
| reports/Validation-Report/en/_problem_detail_0014.Rmd | Updates English problem-detail guidance text. |
| reports/Validation-Report/de/_problem_detail_0014.Rmd | Updates German problem-detail guidance text. |
| reports/Validation-Report/de/_problem_detail_0012.Rmd | Updates German problem-detail guidance text. |
…ed flag, inline R spacing - scripts/Update-Po4aYamlKeys.ps1: the inner `# manual-keys` match overwrote `$Matches` before the YAML path was captured. Hoisted the path-capture above the manual-keys check. - scripts/New-ValidationReports.ps1: rewrote the dispatch. The param is `-Combined` but the code referenced `$isCombined`, so the per-site path always ran. The render loop also still used `$Language` after the parameter rename to `$Locale` and called `Resolve-NeoipcLocaleQmd -Language ...` after the helper's signature changed to `-Locale`. Now derives the language from the full locale via `Split-NeoipcLocale`, branches cleanly on `$Combined`, and skips the per-site Get-NeoipcDepartments call in the combined path. - scripts/New-PatientDataReport.ps1: completed the same Language → Locale rename. Added the missing `$timestamp` and `$language` helpers (`$timestamp` was referenced before being defined; the language part of `$Locale` is needed for `--profile` and the output filename). - scripts/ConvertFrom-JsonMetadata.ps1: removed the `# Dev mode` block that hardcoded `$ForExcel = $true`, overriding the user-passed switch. - scripts/modules/NeoIPC-Tools/Private/DHIS2Http.ps1: `$uriBuilder.Query = '?' + (...)` produced `??` in the resulting URL because `UriBuilder.Query` prepends `?` automatically. Drop the leading literal. - reports/Reference-Report/content/_nosocomial_infections.Rmd: added the missing spaces around two inline R expressions (`In`r ...`` → `In `r ...``, `"in"` → `"in "`).
Member
Author
Round 1 —
|
| # | Location | Finding | Disposition |
|---|---|---|---|
| 1 | Update-Po4aYamlKeys.ps1:101 |
Inner # manual-keys regex overwrote $Matches before the YAML path was captured |
Fixed — hoisted the path capture above the manual-keys check |
| 2 | New-ValidationReports.ps1:94 |
$sites = Get-NeoipcDepartments ran twice unconditionally; $isCombined was undefined (param is $Combined) |
Fixed — branch on $Combined once, single Get-NeoipcDepartments call only on the per-site path |
| 3 | New-ValidationReports.ps1:128 |
$Language referenced after rename to $Locale; Resolve-NeoipcLocaleQmd -Language after helper signature changed to -Locale; -Combined not implemented |
Fixed as part of #2's rewrite — derives $language via Split-NeoipcLocale, calls helper with -Locale $Locale, implements $Combined (renders once without -P departmentFilter:...) |
| 4 | New-PatientDataReport.ps1:70 |
Same Language → Locale rename incomplete; $timestamp not defined |
Fixed — added $timestamp = [datetime]::Now.ToString(...) and $language = (Split-NeoipcLocale -Locale $Locale).Language; updated all references |
| 5 | ConvertFrom-JsonMetadata.ps1:16 |
# Dev mode block forced $ForExcel = $true, overriding the user-passed switch |
Fixed — removed the override |
| 6 | DHIS2Http.ps1:47 |
$uriBuilder.Query = '?' + (...) produced ?? in URL (setter prepends ? automatically) |
Fixed — $uriBuilder.Query = (...) |
| 7 | UserInfo.ps1:22 |
ArgumentCompleter reads data/local/<serverKey>/site-codes.txt, flagged as guardrail-pattern issue |
Deferred — the cache pattern is used by 9 sites across 7 scripts (consumers + the Update-NeoipcSiteCache.ps1 writer); fixing only UserInfo.ps1 would create asymmetric paths. Filed as a coordinated follow-up to move all consumers + the writer to $env:LOCALAPPDATA / $env:XDG_CACHE_HOME. Also clarifying: the data/local/ guardrail is an agent-behavior restriction (no Claude/Copilot reads/writes), not a runtime constraint on the shipped scripts — moving the cache is a workflow improvement, not a guardrail-violation fix |
| 8 | NeoIPC-BuildTools.psd1:20 |
Author = '...' flagged as personal-names guardrail violation |
False positive — the workspace + submodule guardrails both have an explicit exception: "except in copyright statements and file-header attribution lines (e.g. Author:, @author, Copyright (c) fields)". The .psd1 Author = '...' field is exactly this case |
| 9 | _nosocomial_infections.Rmd:1 |
Missing spaces around inline R expressions — would render InTable 4, found inSEC1.4.2. |
Fixed |
All 9 threads resolved.
…+ inline R spacing
- scripts/New-PatientDataReport.ps1 and scripts/New-ValidationReports.ps1:
the rebase --onto -X ours dropped the per-script build-report
machinery that PR-07b's chronological commits introduced —
Write-NeoipcBuildReport at the end, PSCmdlet.ShouldProcess() gates,
LC_ALL territory handling, Invoke-Rscript for the JSON branch,
proper error/outputFiles accumulation, Write-Progress, and the
JsonReport switch actually being consulted. Restored both files to
their dev-tip shape and re-applied the PR-07a-code round-2
Dhis2Port-null guards on top (which the dev tip lacks).
- reports/Partner-Report/figures/_fig-{ga,bw}.qmd: replaced the
undefined sR$outlier$heading lookup with the existing
sR$figure_interpretation$heading key. The callout was rendering
with an empty heading because the outlier section in the cascade
doesn't define a heading sub-key.
- reports/Reference-Report/content/_infectious_agents.Rmd: added
the missing spaces around two inline R expressions
(`it`r dR$refAgentPerInfTable`` -> `it `r ...``, and the
`infections`r dR$refDetectionTable`` mirror).
Member
Author
Round 2 — 9fe5dccFive unique findings (Copilot duplicated each 2-3 times, so 14 inline comments — the duplicates were resolved without per-thread replies since the canonical thread for each finding has the detailed disposition).
The two New-PatientDataReport.ps1 findings (#1, #2) — plus the corresponding New-ValidationReports.ps1 omissions — are the same class of rebase-artefact damage that surfaced on PR-08 round 2: the rebase --onto -X ours strategy aggressively chose ours-side resolution where PR-07a-code's earlier fixes touched lines that PR-07b chronologically rewrote, dropping the chronological rewrite. The fix here was a wholesale dev-tip restore plus targeted re-application of the still-valid earlier-round fix on top. New-ValidationReports.ps1 had the same damage; restored in the same commit even though Copilot didn't separately flag it. All 14 threads resolved (5 canonical + 9 duplicates). |
Member
Author
|
Round 3 summary (head 7e1cd8b) Fixed (2 unique findings, 6 duplicate threads):
Note: both findings predate PR-07b at the dev tip — these are bugs the upstreaming squash-merged main will start out without. |
Member
Author
|
Round 4 summary (head 01224fc) Fixed:
Deferred:
|
…idation-Report content
Member
Author
|
Round 5 summary (head e328dc2) Copilot's round 5 review reported 'no new comments' but actually generated 18 stored comments (9 unique findings posted twice) that failed to upload to the PR API — recovered via the Running Copilot Code Review Actions run log (run 26776267817) per the CLAUDE.md fallback recipe, and confirmed verbatim against the agent-task session. Fixed:
No threads to resolve since the inline comments never reached the PR API. Recording the upload-failure pattern in the upstreaming review log for future reference. |
…throw on failed delete, fix UserInfo -Path semantics
Member
Author
|
Round 6 summary (head 9290d0c) 5 unique findings on this round (each posted 3 times = 15 inline threads, all replied + resolved). Fixed:
Pushed back (workspace guardrail exception):
|
…rences in _nosocomial_infections.Rmd
Member
Author
|
Round 7 summary (head 0001607) Fixed:
|
…TE (was admission_occurredAt)
Member
Author
|
Round 7 followup (head a8b43e2) — recovered missing finding Copilot stored 2 findings this round but only 1 (the _nosocomial_infections.Rmd spacing issue, addressed in 0001607) reached the PR API. Recovered the second from the Running Copilot Code Review Actions run (id 26778645600) per the CLAUDE.md fallback recipe, then verified verbatim against the agent-task session bodies the user pasted. Fixed (critical):
No inline thread to resolve — the upload failed silently on this finding. Second occurrence of the partial-upload failure pattern documented under PR-07b-code round 5 in the upstreaming review log. |
Member
Author
|
Round 8 followup (head 8a4cde1) — recovery + false-positive batch The new workspace guardrail's PR/run-log count cross-check caught a 20-of-21 partial upload failure. The PR review body claimed 'generated 1 comment'; Actions run 26779380886 stored 21 unique findings. Of those 21:
Why false positive: knitr's chunk-label parsing is permissive. Empirical test of No code changes from the 20 dropped findings. Recording the false-positive pattern in the upstreaming review log. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 175 out of 251 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
reports/Reference-Report/tables/_tbl-abr-infection-rate.qmd:66
fmt_number()is usingcolumns = !sR$table_headers$n, which is fragile/ambiguous becausesR$table_headers$nis a string (set via the earlierrename()), and!<character>can be interpreted as a logical negation rather than a tidyselect negation. This can break table rendering at runtime. Use tidyselect explicitly to exclude the N column (consistent with the Partner Report tables).
reports/Reference-Report/tables/_tbl-abr-infection-rate.qmd:66fmt_number()is usingcolumns = !sR$table_headers$n, which is fragile/ambiguous becausesR$table_headers$nis a string (set via the earlierrename()), and!<character>can be interpreted as a logical negation rather than a tidyselect negation. This can break table rendering at runtime. Use tidyselect explicitly to exclude the N column (consistent with the Partner Report tables).
reports/Reference-Report/tables/_tbl-abr-infection-rate.qmd:33stringr::str_extract()only accepts(string, pattern). Passing a third positional argument (the capture-group index) will raise an "unused argument" error at runtime, so this table chunk will fail to render. Usestringr::str_match()(orstr_match_first()) to access capture groups instead.
Member
Author
|
Round 9 cross-check (head f07cd67) — direct postfilter evidence + 1 resolved fix The PR review body says 'generated no new comments' but includes an explicit 'Comments suppressed due to low confidence (3)' section. Actions run 26780871467 stored 5 events (3 unique findings × dedup duplicates). Suppressed-due-to-low-confidence (2 unique findings shown by Copilot, both documented false positives):
This is direct empirical confirmation of the confidence-based postfilter — and Copilot got both calls correct. Silently dropped (1 unique finding, NOT listed in suppressed section) — now fixed:
|
Member
Author
|
Round 10 summary (head f07cd67) Pre-flight per the count cross-check guardrail: Actions run 26781907657 stored 1 unique finding; PR review body claims 2 comments (inline threads); no 'Comments suppressed due to low confidence' section. Mismatch is the opposite direction this time — 1 unique store event produced 2 inline threads (Copilot's known dedup-duplication quirk; same body posted twice on PR-API). Run-log count 1 = unique findings; PR-API count 2 = dedup-inflated thread count. Pushed back (no code change):
|
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
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.
Second Surveillance-Toolkit PR in the cross-repo PartnerReport
upstreaming effort. Picks up the post-feedback report iteration that
followed PR-07a-code's JSON-output milestone: integrates the partner-
side review feedback into Reference and Partner reports, finishes the
Validation Report build-out, wires confidence intervals through the
report layer, and integrates the NeoIPC-Tools PowerShell module set.
Code-only scope: this PR ships the source-file deltas (
.qmd/.Rmd/.R/.ps1/*.yml/*.yaml/*.lua/*.tex/workflow / module). The translation-artefact deltas produced by po4a
land separately as PR-07b-translations.
What the PR delivers
Reference-Report iteration. Post-real-life-testing rewrite of the
Reference-Report tables (secondary-BSI rates now properly
localized; surgical-procedure intro rewritten; per-table headers
hooked through the
sR$<tbl-...>namespace); title-page +single-country edge-case fixes; methods-texts toggle.
Partner-Report iteration. Layout + fine-grained CI control + methods
texts; partner-feedback fixes for the antibiotic-resistance,
infectious-agent-detection, and risk-density tables. Final-touches
pass after partner real-life rendering of multiple sites.
Validation Report. Revived for the current neoipcr API shape;
department / hospital / country resolution wired through the
metadata cascade; exclusion-of-test-data default; explicit
-IncludeTestDataswitch;Get-NeoipcDepartments -ExcludeTestUnitsplumbed through. Per-rule mapping +formatter scaffolding for the rule cohort introduced in PR-08.
Confidence intervals at the report layer. Both Reference and
Partner reports now render CIs alongside the point estimates from
neoipcr's rate-table builders; the
includeConfidenceIntervalsparameter switches between full / pooled-only / off so output
size stays in scope when CIs aren't needed.
NeoIPC-Tools PowerShell module set. Integrated under
scripts/modules/NeoIPC-Tools/as a vendored module: cmdlets forreading event info, departments, hospitals, infectious-agent
metadata;
Find-NextFreeInfectiousAgentIdfor theinfectious-agents authoring workflow; localized message
string-resources per script.
Multi-DHIS2-instance polish. Wrapper scripts and
_setup.qmdfiles accept the
dhis2Scheme/Hostname/Port/Pathparameter family consistently;
Resolve-NeoipcLocaleQmdcentralises the per-locale wrapper discovery; auth flow is
routed through neoipcr's env-var contract.
Toolchain polish. Several PowerShell wrappers consolidate on
the
Invoke-WithNeoipcAuth+Invoke-QuartoRenderhelpersintroduced in PR-07a-code;
Update-Po4aYamlKeys.ps1empty-keys + comment-anchor hardening;
Test-PoPlaceholders.ps1introduced for placeholder validation.
The deferred concerns flagged in PR-07a-code's review cycles are
addressed here —
_tbl-secondary-bsi-rates.qmdis rewritten with fullsR$localization;_tbl-intro-surgical-procedure.qmdis removed (itwas misleading prose, replaced with corrected text in the table chunk);
the Partner-Certificate
\selectlanguage{british}hardcode isreplaced by a babel-language-name lookup. See
tmp/upstreaming-review-log.mdfor the cumulative deferral set.