feat(npqs): batch/parallel-split phytosanitary certificate workflow - #75
Open
lokewate wants to merge 32 commits into
Open
feat(npqs): batch/parallel-split phytosanitary certificate workflow#75lokewate wants to merge 32 commits into
lokewate wants to merge 32 commits into
Conversation
Redesigns the NPQS export flow from one monolithic per-consignment pass into a graph that forks into three concurrent tracks (lab, visual, treatment) via PARALLEL_SPLIT, each partitioning the consignment down to just the items that need it via BATCH_SPLIT/ BATCH_JOIN pairs keyed on the officer's per-item routing decision (lab_required / visual_required / treatment_required), before rejoining for document review, payment, certificate issuance, and ePhyto transmission. Within that: the lab track's result gate (lab_result_split / lab_result_join) is itself a nested per-item BATCH_SPLIT/BATCH_JOIN — one item failing final doesn't hold up others in the same batch, and only items marked for resubmission loop back through a dedicated redraw/retest pair (kept structurally separate from the initial draw/test nodes so the nested batch region stays topologically closed, per the engine's BATCH_SPLIT region-closure validation). The treatment-request and certificate-issuance steps each carry a per-item picker (treatment specs per item; which items make the final certificate) instead of one decision for the whole consignment. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
These are the forms officers actually see (task_code -> taskconfig -> forms.review), separate from the tnsw/npqs_v2 spec copies — several were missing the per-item breakdown the spec copies already had, and none of the treatment-track ones showed which items a batch decision actually covered. Adds a read-only/editable per-item array (id + commodity name, plus track-specific fields) to each, marked addable:false/removable:false now that ArrayControl supports it, and adds the missing item context to npqs_treatment_review's view form (previously showed zero commodity context at all — just the trader's raw uploaded cert URLs). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Pre-existing uncommitted local changes, not authored this session — committing as-is to track them (large diffs, not independently reviewed or verified here). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Pre-existing uncommitted local changes, not authored this session and unrelated to the NPQS workflow — committing as-is to track them, not independently reviewed or verified here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…on tracks
The trader-side status markdown for all three visual-inspection tracks
(sample, consignment, system) referenced flat top-level fields like
.visual_result, .comments, .system_reference_id. Those values are never
flat: TaskManager.CompleteTaskStep namespaces every EXTERNAL_REVIEW/
USER_INPUT submission under the subtask template's own output_namespace
(record.Data[namespace] = payload) before it reaches the render layer, so
the officer's actual submission lives at .reviewerform.visual_result etc.
Since the flat key never existed, requireDataKey: "visual_result" never
matched post-completion either, so the whole view rendered as {}.
Fixes:
- render.json: requireDataKey now checks "reviewerform" (the namespace
key that's actually present once the officer completes their review),
not the never-populated flat "visual_result".
- status_markdown.json: templates now read .reviewerform.* for
officer-submitted fields, and .traderinput.* (already correct) for
trader-submitted scheduling fields.
- Every state (pending, scheduled, completed) now lists the batch's
commodities up top and, once reviewed, a per-item pass/fail/reason
breakdown from .reviewerform.items - not just an aggregate result -
so both officer and trader can see exactly which items a decision
covers.
- 4-1-visual_sample: status_message was ungated (always rendered) while
assessment_outcome only rendered once reviewerform existed, causing
the same content to render twice post-completion; gated status_message
to QUEUED_EXTERNALLY to match the other two tracks.
Verified by executing all 6 templates (npqs_v2 + deployed npqs mirrors)
through Go's text/template with realistic pending/scheduled/completed
data shapes.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…facing status views Same root cause as the visual-inspection fix: every EXTERNAL_REVIEW step in NPQS namespaces the officer's submission under record.Data.reviewerform (TaskManager.CompleteTaskStep writes record.Data[output_namespace] = payload), but these six trader-facing status templates still referenced the pre-fix flat field names, so their result sections stayed permanently hidden/empty once an officer actually completed a review: - 3-lab_testing: sample_test_result, lab_comments, lab_report_attachment_url - 5-5-treatment_review_certs: treatment_review_result, review_comments, post_treatment_visual_required - 6-2-review_docs: docs_review_outcome, review_comments - 5-6-treatment_supervisor_report: supervisor_notes, supervision_report_url - 5-3-treatment_cert_issue: treatment_certificate_id, treatment_outcome, issue_comments - 8-issue_certificate: certificate_id, certificate_url, comments, and the certificate_items per-item inclusion list (was reading the pre-decision dispatch-time array, never the officer's actual include_in_certificate choice at .reviewerform.certificate_items) render.json's requireDataKey gates updated the same way (now check for the "reviewerform" namespace key, which is reliably present once the officer completes their review, instead of a flat field that's never populated at that path). Lab testing's per-item breakdown now ranges over the officer's actual submitted .reviewerform.items instead of the static dispatch-time .commodities list, so per-item pass/fail actually reflects what was reviewed. Left unchanged (confirmed correct on inspection): commodity/treatment item context lists that are self-mapped at dispatch time (.commodities, .treatment_items, .certificate_items as pre-decision context, reference_number, sample_number, ephyto_required, treatment_supervision) - those are genuinely flat in record.Data and were never part of this bug. 2-sample_collection was also checked and needs no change: its results_summary section already uses dataKey: "officer_receive" to scope the projector to the right sub-object, which is the same fix applied a different way. Verified by executing all 12 changed template files (npqs_v2 + deployed npqs mirrors) through Go's text/template with realistic pending/completed data shapes for each track. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… npqs_v2 tnsw/npqs is the old, single-item workflow mirror and is actively being tested right now — don't touch it while that's in progress. Reverts the render.json/status_markdown.json edits from the last two commits for this directory only; tnsw/npqs_v2's equivalent fixes stay in place. See the npqs_v2 top-level split in the next commit for how batch-workflow testing is now fully decoupled from this tree going forward. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fully decouples the batch-workflow (tnsw/npqs_v2) officer-facing forms from the old single-item flow's forms, which are still being actively tested against the shared top-level npqs/ tree. Root cause: every EXTERNAL_REVIEW task's plugin_properties.task_code is resolved against the agency backend's artifact registry by task_code as a flat id (npqs/manifest.json). tnsw/npqs and tnsw/npqs_v2 used byte-identical task codes (e.g. npqs_lab_testing_v1) for the same conceptual review step, so both workflow versions resolved to the exact same officerinput_jsonform.json under npqs/ - any edit there (per-item context, form fields, anything) hit both flows at once, with no way to change one in isolation. npqs_v2/ is a full copy of npqs/ (so it starts from the same, already per-item-aware forms) with every id renamed to a distinct "_v2"/"-v2" suffix - task codes, taskconfig filenames, and every generic_template (jsonform) id referenced from forms.view/forms.review - so registering both trees into one registry can't collide. tnsw/npqs_v2/**/*.json's task_code fields (13 EXTERNAL_REVIEW/PAYMENT tracks) now point at the new _v2 codes; tnsw/npqs's task codes are untouched and still resolve into npqs/ exactly as before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…-sample inspection Selecting "sample" as the visual inspection approach routed straight to the officer's inspection review with no way for a physical sample to actually reach NPQS first - no trader submission step, no officer acknowledgement, and n_visual_sample_inspection's own sample_number input silently depended on the (unrelated) lab track's npqs.sample_number variable, which is only ever set when an item also needs lab testing. Adds tnsw/npqs_v2/4-0-visual_sample_collection, a small two-step sub-workflow mirroring 2-sample_collection's shape: trader_submit (USER_INPUT, "confirm you dropped off a sample") -> officer_receive (EXTERNAL_REVIEW, "verify & register the received sample, assign a sample number"). Wired into the parent workflow as a new node n_visual_sample_collect between gw_visual_split's 'sample' edge and n_visual_sample_inspection, with n_visual_sample_inspection now reading sample_number from its own dedicated npqs.visual_sample_number variable instead of the lab track's. Registers the new officer-facing review form + task config (npqs_visual_sample_receive_v2) under the split top-level npqs_v2/ artifact root added last commit - doesn't touch npqs/ or tnsw/npqs at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…; simplify to pure status flow; fix output_mapping
Two real bugs found by live-testing the previous commit against the running
dev stack:
1. tnsw/manifest.json (nsw-srilanka's own artifact catalog, loaded once at
process startup, separate from the agency-side npqs_v2/manifest.json
added earlier) never got entries for the new
npqs-v2-visual-sample-collection task_template/workflow/subtask/render
artifacts. The parent workflow's activity kept failing with "artifact
not found: npqs-v2-visual-sample-collection/task_template" and retrying
forever - no task record was ever created, so the trader saw a
permanently blank screen. Fixed by adding the 6 missing rows and
restarting tnsw-api to reload the manifest (loaded once at boot, not
live like individual artifact content).
2. officer_receive's own output_mapping used "reviewerform.sample_number"
as its left-hand key. That's wrong: output_mapping's LHS reads the RAW,
un-namespaced activity result (CompleteTaskStep passes the officer's flat
reviewerResponse straight through) - the "reviewerform." nesting only
applies to record.Data, used for rendering, not to workflow variable
mapping. Every other correct EXTERNAL_REVIEW node in this codebase (e.g.
2-sample_collection's own officer_receive) uses a flat key. This bug
didn't error loudly - the graph interpreter "parks" a node on a
mapTaskOutputs failure instead of failing the Temporal workflow task, so
the workflow just silently stopped forever after the officer submitted,
with no error surfaced anywhere. Fixed to a flat "sample_number":
"sample_number", matching the working pattern.
Also simplifies the design per feedback: the trader doesn't need to
actively confirm dropping off the sample. Removed the trader_submit
USER_INPUT step entirely - the trader now just sees a pure status message
("please drop off a sample" -> "sample received") with no form to fill,
exactly matching how a physical drop-off actually works and matching the
established pattern used by the other read-only-for-trader tracks (visual
system/lab testing) already fixed earlier this session.
Added test/npqs/npqs_visual_sample_collection_test.go in nsw-srilanka,
which drives this sub-workflow's own workflow.json directly (the existing
full-simulation test mocks at too high a level - it treats the whole
sub-workflow as one opaque activity call and can't see output_mapping bugs
inside it) with the exact flat payload shape NSW Agency actually posts.
Confirmed it fails (times out, matching the real parked-workflow symptom)
against the old output_mapping and passes against the fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…s view assessment_outcome's section is dataKey-scoped to "reviewerform" (its render.json), so .lab_required/.visual_required/.treatment_required/ .lab_sample_method/.visual_approach resolved against reviewerform directly - but those are per-item fields the officer sets inside reviewerform.commodities[], not direct properties of reviewerform itself. They were always undefined, so the trader always saw "Not Required" for every inspection path regardless of what was actually configured per item. Only .review_outcome, .reference_number, .ephyto_required, and .clarification_reason are genuinely batch-level reviewerform fields and stay as direct references. Replaced the single aggregate line per path with a per-item breakdown (ranging over .commodities, which resolves to reviewerform.commodities under this section's dataKey), so the trader sees exactly which items need lab testing, visual inspection, or treatment. Verified against this exact bug's real trigger data (a live consignment where item-1/item-2 have visual_required: true) via Go's text/template - correctly shows "Required" for those items now, where it silently showed "Not Required" for everything before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
status_message (the section carrying the item list + narrative status text) was gated to states: [QUEUED_EXTERNALLY] only - the window after the trader submits, while waiting on the officer. But this sub-workflow's FIRST step (trader_schedule) runs in PENDING_USER, before that gate opens, so a trader landing on "schedule your consignment inspection" saw only the bare date/time/contact form - no indication of which items the inspection even covers, since assessment_outcome (the other item-list-carrying section) is also hidden at that point (gated on reviewerform, which doesn't exist yet either). Added PENDING_USER to status_message's states gate. Its own template already handles this correctly (falls through to the "Schedule Your Consignment Inspection" branch, which lists items, when neither reviewerform.visual_result nor traderinput.inspection_date exist yet) - this was purely a visibility gate gap, not a template content bug. Checked every other track with the same trader-form-then-officer-review shape (upload_docs, ephyto, treatment_upload_certs, treatment_request) for the same gap: none have it. treatment_request has a separate, always-visible items_summary section that already covers this; the other three already include PENDING_USER in their form-carrying section's own gate. Verified live against a completed real task from this session (n_visual_consignment_flow) that the assessment_outcome branch (post- completion) already renders items + per-item pass/fail correctly - this fix only closes the earlier, pre-submission gap. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e-collection and lab-testing markdown Two related gaps in the lab track's trader-side status views: 1. The officer's "NPQS Receive Sample" review form (agency-side) had no context at all about which commodities the batch's sample is supposed to cover - just a bare sample-number/comments form. Added a read-only items array (id, commodity name, botanical name) sourced from the sub-workflow's own dispatch data, addable:false/removable:false since this step is all-or-nothing (no per-item outcome tracking needed here, just "here's what to expect"). Renamed the officer_receive node's local input key from "commodities" to "items" to match the established convention (and the trader-app's ApplicationDetailScreen prefill whitelist, which already includes "items" but not "commodities") - updated 2-sample_collection/status_markdown.json's own reference to match, and mirrored the officer form onto the split npqs_v2/ agency-side tree. 2. n2_1_lab_testing's (3-lab_testing) trader-facing status markdown only listed items in the post-completion per-item-results branch - while testing was in progress, the trader had no way to tell which items were actually in the lab. Moved the item list (id, botanical name, quantity) to always render at the top, above both the pending and completed branches, matching the pattern already used for every other track fixed this session. Verified via Go's text/template with realistic pending/completed payloads, and the nsw-srilanka workflow test suite still passes with the 2-sample_collection input_mapping rename. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ine from treatment-request approval gw_treatment_split only partitions on item.treatment_required - it doesn't sub-partition by provider, so a single n3_treatment_request execution can (and does, per a live report) cover items with different treatment_provider/ treatment_supervision preferences (set per-item back at n1_apply). The officer's own review form for this step captures ONE aggregate provider/ supervision decision, not a per-item one, so the "Treatment Provider" / "Supervision" lines in the approval markdown only ever showed one value - misleadingly implying a single uniform decision when items can differ, and duplicating (with different, conflicting values) what items_summary already shows correctly per item. There's no per-item outcome captured at this step to display instead (the review form has no items array), so per the simpler of the two options this was reported with: removed the two lines, kept the "what happens next" guidance (still accurate - the workflow really does take one single path forward from here, driven by the same treatment_provider variable, regardless of what any individual item's original preference was). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rkflow-wide decision
Reported bug: selecting NPQS-station treatment for one item and external-
provider treatment for two others in the same batch, only the external
task was ever created - all three items went down it. Root cause:
treatment_provider_split was an EXCLUSIVE_SPLIT on a single workflow
variable (npqs.treatment_provider), set by the officer's own ONE decision
on n3_treatment_request's review form - completely ignoring each item's
own treatment_provider (already set per-item back at n1_apply, and
already shown correctly, per item, in the always-visible items_summary
section). The same bug existed one level down: n3_3_1_supervision_split
routed supervision-report on a single npqs.treatment_supervision value,
so a mixed npqs-provider batch (some items needing supervision, some not)
also couldn't route correctly.
Converts both to per-item BATCH_SPLIT/JOIN gateways (mirroring
lab_result_split's existing pattern), reading item.treatment_provider and
item.treatment_supervision directly instead of the single workflow
variable:
treatment_provider_split (now BATCH_SPLIT on item.treatment_provider)
-> npqs items: n3_2_treatment_payment -> n3_3_treatment_cert_issue
-> n3_3_1_supervision_split (BATCH_SPLIT on
item.treatment_supervision)
-> with_supervision: n3_3_2_treatment_supervisor_report
-> without: (straight through)
-> n3_3_1_supervision_join
-> external items: n3_4_treatment_upload_certs (direct)
both partitions converge at n3_4_treatment_upload_certs, continue
through the shared review/post-treatment-visual chain, and exit through
the new treatment_provider_join before rejoining gw_treatment_join.
Since the real per-item provider/supervision already exists from n1_apply,
n3_treatment_request's own treatment_provider/treatment_supervision/
supervision_officer_type fields were pure duplication of that (and the
source of the bug, since the workflow trusted this single redundant
decision over the real per-item data) - removed them from the review form
(tnsw/npqs_v2 spec copy and the split npqs_v2/ agency copy), and from
n3_treatment_request's own output_mapping. Fixed the three trader-facing
status views that depended on the now-removed workflow variables
(5-1-treatment_request, 5-3-treatment_cert_issue,
5-4-treatment_upload_certs) - replaced single-value provider/supervision
claims (which could never be accurate for a mixed batch, and are
redundant with the item lists already shown) with guidance that doesn't
assume batch-wide uniformity.
Verified: extended the workflow test suite with item-7 (external
provider) and item-8 (npqs provider, with_supervision) alongside the
existing item-3 (npqs, without_supervision) - all in the SAME
treatment-required batch. Confirmed via the mock's actual execution
counts that npqs-v2-pay-for-treatment/issue-treatment-cert run once for
item-3+item-8 only, npqs-v2-treatment-supervisor-report runs once for
item-8 only, and npqs-v2-upload-treatment-certs/review-treatment-certs
each run twice (once per provider partition) before both converge back
into the shared PARALLEL_JOIN steps. Also confirmed the test fails
(BATCH_JOIN references non-existent BATCH_SPLIT) when
treatment_provider_split is reverted to the old EXCLUSIVE_SPLIT, proving
it's a real regression guard for this exact bug.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ment sub-tasks BATCH_SPLIT only re-scopes its configured items_variable (commodities); npqs.treatment_items was a separate flat variable set once, before treatment_provider_split, from the trader's full treatment submission — so both the npqs and external partitions carried the *combined* item list downstream, unfiltered, into cert issue, supervisor report, upload certs, and review certs. Repoint all four to the already correctly-scoped `commodities` variable instead, dropping the per-item treatment_type column those screens showed (already visible to both trader and officer once, at treatment-request time) in favor of correct per-partition item lists. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…aggregate visual_sample_result_split, visual_consignment_result_split, visual_system_result_split, and visual_sample_escalated_result_split were EXCLUSIVE_SPLIT gateways reading a single workflow-level "worst outcome across items" value the officer picked for the whole batch — so if one item in a batch failed or needed escalation, every item in that batch was forced down the same path, even ones that individually passed. Convert all four to per-item BATCH_SPLIT/BATCH_JOIN pairs (mirroring the lab and treatment tracks), reading item.visual_result instead. This also required propagating the officer's per-item result array back into `commodities` (via items->commodities output_mapping on each inspection sub-workflow's officer node) since nothing wrote it back before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… clobbering
gw_par_join now carries parallel_join.merge_by_id: {"commodities": "id"},
wiring NPQS into the shared engine's new PARALLEL_SPLIT/JOIN isolation
and merge fix (core commit a81c35e). Previously all three tracks ran as
in-process coroutines sharing the same commodities variable directly;
whichever track's BATCH_SPLIT/JOIN cycle finished last would overwrite
the WHOLE array from its own pre-split snapshot, silently reverting the
other tracks' contributions — even sample_test_result and visual_result
on items each track itself had just processed. Proven with a live item
that's both lab_required and visual_required in nsw-srilanka's test
suite: both fields now survive to certificate issuance regardless of
which track finishes first.
Also renames the one field lab and visual could genuinely both write to
the same item — failure_reason -> lab_failure_reason / visual_failure_reason
— since no merge, however correct, can arbitrate two different values for
the identical field. Everything else the two tracks write is already
disjointly named (sample_test_result vs visual_result), so this closes
the last real collision surface.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The lab/visual failure_reason -> lab_failure_reason/visual_failure_reason rename only touched the tnsw/ spec copies, missing these 4 agency-side mirrors that officers actually submit through in production — leaving the exact field-name collision the rename was meant to eliminate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ore_info loops None of the three resubmission loops (treatment request, docs upload, treatment cert upload) carried the trader's own prior submission forward on a needs_more_info loop-back — each showed a blank or pristine-reset form, forcing the trader to redo everything even for items the officer never flagged. Treatment request loops within a single sub-workflow instance, so it's fixed by seeding and reading the same persistent path (traderinput.treatment_items) the trader's own output already writes to - pristine on first entry, the trader's latest answer on every resubmit, with no fallback logic needed. Docs upload and treatment cert upload re-enter their outer task node fresh each time (a new sub-workflow instance), so their prior submission is relayed in field-by-field from where the outer graph already stores it (npqs.docs_traderinput / npqs.treatment_traderinput) via prior_<field> pass-through variables. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ent request
Both are regressions from the loop-back prefill fix: n3_treatment_request's
outer input_mapping now seeds traderinput.treatment_items instead of a bare
commodities key, but two templates were never updated to match.
- items_summary_markdown.json still read .commodities (now gone entirely,
so the item list rendered empty). Switched to .traderinput.treatment_items,
and gated the section to PENDING_USER only in render.json - its own text
("please complete the specification below") only makes sense pre-submission,
and traderinput.treatment_items loses several fields once the trader's own
submission overwrites it, so showing it after submission was never going to
render right anyway.
- status_markdown.json's "awaiting review" branch read .traderinput.treatment_type
and .traderinput.treatment_duration as flat scalars, but they're per-item
fields nested inside traderinput.treatment_items[] - always <no value>.
Replaced with a per-item breakdown of what the trader actually submitted.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Matches the established pattern already used everywhere else in NPQS (7-payment's own certificate-fee step) and in CDA's payment flow - 5-2-treatment_payment was the only payment step in the whole artifact tree still offering LankaPay. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ion view Both NPQS payment steps (7-payment, 5-2-treatment_payment) rendered their PENDING_PAYMENT fee details as a read-only jsonform, plus a "PAYMENT" projector section the trader-app has no renderer for (frontend only supports FORM/MARKDOWN/REDIRECT — it was silently showing "No renderer for component type: PAYMENT"). Replace both with a single MARKDOWN section matching CDA's payment_details_markdown.json pattern; drop the now-dead instructions_wrapper/payment jsonform files. upload_docs's render.json had no section at all for the COMPLETED state, so the page went blank once the trader submitted documents. Unlike the other trader-facing tasks in this flow, officer review here happens in a separate outer node (n5_2_review_docs), not bundled into this task's own sub-workflow, so this task's own record never learns the review verdict. Add a COMPLETED-state confirmation section showing the reference number and submitted document links instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ndant siblings The "submission" reserved key + sibling-merge convention (introduced this session in nsw-srilanka's external_review.go) was unnecessary complexity for a problem input_mapping's own nested dot-path support already solves. Every "commodities?"/"items?"/"certificate_items?" sibling here can instead write straight into a nested submission.<field> path via the engine's existing maputil.SetNestedKey, with no plugin-level merge step needed. Split the 11 affected nodes two ways: - 6 nodes (2-sample_collection, 3-lab_testing, 4-1-visual_sample, 4-2-visual_consignment, 4-3-visual_system, 8-issue_certificate) map a genuinely different per-branch-partitioned array under a destination name (items/certificate_items) that userform/traderinput doesn't already carry — converted to "submission.items"/"submission.certificate_items" directly. - 5 nodes (1-apply, 4-0-visual_sample_collection, 5-3-treatment_cert_issue, 5-5-treatment_review_certs, 5-6-treatment_supervisor_report) had a sibling destined for "commodities", which userform/traderinput already independently carries (the frozen apply-time full commodity list, which is what the shared officer-view-form-v2 schema is actually built to display). The sibling was always redundant here — for 3 of the 5 it was silently discarded by submission's existing precedence anyway; for the other 2 it referenced a field name that doesn't even appear in their own view form's schema. Converting it to a nested path would have introduced a genuine Go-map-iteration-order race against userform's own field instead of fixing anything, so these are simply deleted. Verified deterministic across 200 randomized map-iteration-order runs using the real maputil package. external_review.go's merge code is intentionally untouched — reference_number and other siblings across every agency still depend on it; that's a separate, out-of-scope cleanup. Note: 5-5-treatment_review_certs's view form expects a "treatment_items" field that nothing currently populates (5-4-treatment_upload_certs's output_mapping never sets it) — a separate, pre-existing display gap, not something this change touches. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… sibling convention Companion to edd6dde: convert every remaining sibling across npqs_v2's external_review nodes now that nsw-srilanka's external_review.go no longer merges siblings into submission at all (see that repo's 0478ccd). Two kinds of cleanup: - reference_number, sample_number (3-lab_testing, 4-1-visual_sample), and userform (5-1-treatment_request) are genuine data that userform/traderinput doesn't already carry — converted to direct "submission.<field>" input_mapping paths. - nppo_office_location (2-sample_collection, 4-0-visual_sample_collection), inspection_date/inspection_time/contact_name/contact_phone (4-2-visual_consignment), treatment_certificate_url/ supervision_report_url/upload_remarks (5-5-treatment_review_certs), and invoice_file_url/packing_list_file_url/additional_file_url (6-2-review_docs) were always redundant: each reads from a nested path already inside whatever gets wholesale-copied into submission, so the sibling was dead weight regardless of merge behavior. Deleted rather than converted. Every external_review node in npqs_v2 now relies on nothing beyond input_mapping's own nested dot-path support. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Companion to nsw-srilanka's cec1510: the officer can no longer submit this form without touching the item picker. npqs is being deprecated this week in favor of npqs_v2, so there's no longer a reason to let this slide gracefully all the way through to ephyto's SOAP builder — better to reject it right here, at the form the officer is looking at. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…p v2 naming npqs (old, single-item) is deprecated this week in favor of the batch workflow built under npqs_v2 this cycle. Consolidate to one tree at the plain npqs/ (agency) and tnsw/npqs/ (trader) paths, and strip the now purely historical "v2"/"V2" marker from every artifact id, task_code, form id, and file name across both trees (152 ids renamed, 14 taskconfig files renamed, 14 dead trader-side files removed that turned out to be unreferenced leftovers from before agency review forms had their own copies). Verified via nsw-srilanka's full test/npqs suite, including the static cross-check between every tnsw node's wiring and its agency form schema (catches any missed id/reference automatically) — all green, plus a raw sweep confirming zero remaining "v2" occurrences anywhere in either tree. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Sep 9, 2026
…duplicates The npqs_v2 -> npqs consolidation (5fb86fc) never updated tnsw/manifest.json — the real, production id->path registry nsw-srilanka's artifact loader actually reads (internal/bootstrap/app.go). It still had 134 entries under the now-nonexistent npqs_v2/ path (100% broken) alongside its original npqs/ section, which itself had drifted: 17 entries pointed at files deleted during cleanup, and 9 newly-added files (4-0-visual_sample_collection, plus this session's new markdown templates) were never registered at all. Rebuilt it to exactly match the actual tree: dropped the dead npqs_v2/ section, removed the 17 stale entries, added the 9 missing ones. Restored x-search on 1-apply/userinput_jsonform.json's importing_country and commodity_common_name fields — these had been replaced with a fully inlined static oneOf list at some point during npqs_v2's development, which is what actually inflated this one file from 715 to 2,471 lines (all the world's countries enumerated inline instead of referenced by id). Restored the two backing static-data files (importing_countries.data.json, commodity_common_names.data.json) that had been deleted along with old npqs, and ported forward the genuinely new content (the per-item id field, added for BATCH_SPLIT tracking) onto the original compact formatting rather than starting from the bloated version. Deleted 9 agency-side userinput_jsonform.json files that all declared the identical id (npqs-apply-phyto-cert--user-form) as the one legitimate tnsw-side copy, registered in npqs/manifest.json, but referenced by zero taskconfigs — ~21,500 lines of pure duplicate dead weight, apparently copy-pasted into each new task directory during npqs_v2 scaffolding and never cleaned up. Net effect: the npqs tree is now 11,068 lines, smaller than the 14,177 lines old npqs itself was — despite genuinely new batch-workflow content. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tore Restoring x-search (83ffa19) made commodity_common_name an object ({value, label}) again, but the commodities.example array I ported forward still had it as a plain string — whatever in the trader app autofills from this example was feeding a bare string into a field that now requires an object, surfacing as "must be object" on the live form. Fix the 4 example entries to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ata flows Restoring x-search (25dcbe1) made commodity_common_name and importing_country {value, label} objects again at the point of entry (1-apply), but every downstream consumer of that same data — 12 agency review/view schemas, 2 more trader-side forms, and 14 status markdown templates — still declared/rendered them as plain strings. In practice this broke injection outright: the agency rejected any submission with "type: ... has type \"object\", want \"string\"" the moment an officer task tried to validate the trader's data against its own schema. Fixed all schema declarations to the same {value, label} object shape, and all `{{.commodity_common_name}}`/`{{.importing_country}}` markdown references to `.label` so they render the human name instead of Go's default map formatting. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The npqs -> npqs_v2 -> npqs consolidation (83ffa19) never touched tnsw/trade/, so its HS-code-to-workflow mapping still offered a "npqs-v2-export-phytosanitary-reg" option alongside the real "npqs-export-phytosanitary-reg" — a dead id nothing produces anymore, left selectable in the hscode_selection form, the split-items transform mapping, and the summary markdown's display branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…orting_country field d7a3498 converted these fields to {value, label} objects everywhere they flow, but that alone isn't enough for ui-packages' renderer set to actually render them: SearchSelectControlTester only matches a type: "object" field if it also carries x-search.service — a plain object with no x-search hits zero renderers, surfacing as "No applicable renderer found" on every officer-side form displaying batch item context. Add the same x-search config used on the original 1-apply input field to all 18 downstream occurrences (they're all read-only context displays or already-submitted values, so this only needs to satisfy the renderer's tester and let it show the already-known label — SearchSelectControl does that without touching the search service at all when data already carries {value, label}). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
lokewate
marked this pull request as ready for review
September 9, 2026 11:29
ginaxu1
requested changes
Sep 10, 2026
ginaxu1
left a comment
ginaxu1
left a comment
Contributor
There was a problem hiding this comment.
Hold for now, nsw-srilanka still pins core from Sep 4th. Until that pin includes OpenNSW/core#188, an item on lab and visual loses both tracks’ fields at join. Wait for the go.mod bump, then merge this in the same deploy as OpenNSW/nsw-srilanka#410
Note: in-flight NPQS applications use the old graph under the same workflow id. Replacing it in place will strand them. Version/cut over first.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces the single-item NPQS workflow with a batch/parallel-split one: a consignment's items can now route independently through lab, visual, and treatment tracks, merge back together by item ID, and reach certificate issuance with a per-item picker so a failed item never rides along on a certificate covering the rest of the batch.
Highlights
input_mappingwrites context directly into nested paths instead of relying on a separate merge step (see companion nsw-srilanka PR).Testing: manually verified end-to-end through the TNSW trader app and NPQS agency app. Automated test coverage (in nsw-srilanka) is pending a proper way to reference this repo from tests — see companion nsw-srilanka PR.
Related PRs & Merge Order
Part of the NPQS batch workflow rollout — deploy together:
Blocking, not yet merged: OpenNSW/core#188 (isolate PARALLEL_SPLIT
branches). Without it, an item routed through two tracks concurrently
(e.g. lab + visual) loses both tracks' contributions at merge — a real
data-loss bug, not hypothetical.
nsw-srilanka'sgo.modalso needsbumping to a
coreversion that includes this fix once it merges. Don'tdeploy the batch workflow before this lands.
Non-blocking, UX-only once published: OpenNSW/ui-packages#44 (ArrayControl
addable/removable) — needs a release and apackage.jsonbump in thensw-srilanka/nsw-agency frontends before
addable: false/removable: falseactually takes effect.