fix(channelCheckboxes): report malformed values instead of crashing - #162
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bc5010a8b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # The front end sends either {'1': True, '2': True} or [1, 2]; both mean | ||
| # channels 1 and 2 are being blurred. | ||
| try: | ||
| channels = annotation_tools.get_selected_channels( | ||
| allChannels, 'All channels') |
There was a problem hiding this comment.
This changes the accepted interface behavior, but the corresponding DECONWOLF.md, GAUSSIAN_BLUR.md, HISTOGRAM_MATCHING.md, REGISTRATION.md, and ROLLING_BALL.md files remain untouched, so maintainers consulting those worker-specific contracts will not learn that checkbox values must be normalized from either mappings or lists. Update each affected worker document directly, as was done for the Cellpose-SAM workers.
AGENTS.md reference: AGENTS.md:L49-L54
Useful? React with 👍 / 👎.
Checked the NimbusImage side — there is a fix there, but it can't be what handled thisSurveyed at NimbusImage
So the 2026-07-24 normalization is real and useful, but it postdates the crash by two months and only covers the agent path — the worker-side normalization in this PR is still doing work. Still unidentified: what submitted Recorded in Generated by Claude Code |
A production cellposesam job received a bare list where the documented
channelCheckboxes mapping was expected:
{"Channel for Slot 1": [0], "Channel for Slot 2": [], "Channel for Slot 3": []}
Every worker read the value with .items(), so the job died with
`AttributeError: 'list' object has no attribute 'items'` before any of its own
validation could run. Six sibling workers had the identical latent bug;
cellposesam was just the first one a user happened to trigger.
New shared helper annotation_tools.get_selected_channels(value, field_name)
parses the documented {'0': True, '1': False} mapping into a sorted list of int
indices, returns [] for an unset field (None, '', {}), and raises ValueError on
any other shape. Every consumer routes through it and reports the ValueError via
sendError: cellposesam, cellposesam_train, registration, deconwolf,
histogram_matching, gaussian_blur, rolling_ball, and sample_interface (which now
demonstrates the canonical read for the field it previously only declared).
cellposesam's slot resolution is factored into get_slot_channels() mirroring the
one cellposesam_train already had, so the two stay in sync.
The list shape is rejected, not normalized. ChannelCheckboxGroup.vue has been
typed Record<number, boolean> since it was introduced, and the one upstream path
that accepts arrays (the AI panel's normalizeWorkerInterfaceValue) converts them
to the map before saving — so a list value comes from outside the UI and we
cannot confirm which channel it meant. Running a segmentation or deconvolution
against a guessed channel silently produces wrong results; failing with an
actionable message does not. Tolerating it would also keep a second wire format
alive indefinitely for a payload nothing is known to still emit.
Empty-selection behavior is unchanged per worker: the workers that already
errored on no channels still do, and gaussian_blur/rolling_ball still write out
an unmodified copy.
Tool configs that already hold list values now report "Could not read the
channel selection" until their channels are re-selected and re-saved. That
cleanup, and identifying the submitter that wrote them, is tracked as TODO-004 —
the AI panel landed 2026-07-06, five weeks after the 2026-05-29 crash, so it
cannot be the source of that payload.
Verification (Docker unavailable here, so run natively against the real
annotation_client, large_image, skimage and pystackreg): 200 tests pass across
annotation_utilities (37), worker_client (12), gaussian_blur (17), rolling_ball
(19), histogram_matching (18), registration (22), deconwolf (40),
sample_interface (5), cellposesam (13) and cellposesam_train (17). The exact
crash payload was replayed through get_slot_channels() and now yields the
sendError path. Each new per-worker test was confirmed to fail both against a
list-accepting helper and against master's pre-fix entrypoint.
4235f72 to
b7dac40
Compare
arjunrajlab
changed the title
1 participant
The bug
A production
cellposesamjob received a bare list where the documentedchannelCheckboxesmapping was expected:Every worker read the value with
.items(), so the job died withAttributeError: 'list' object has no attribute 'items'before any of its own validation could run. Six sibling workers had the identical latent bug (registration,deconwolf,histogram_matching,gaussian_blur,rolling_ball,cellposesam_train);cellposesamwas just the first one a user happened to trigger.About the other error in that log
Failed to get docker network/socket.gaierror: [Errno -3] Temporary failure in name resolutioncomes fromgirder_workeron the NimbusImage server, not from this repo, and it is non-fatal — it logs the failure and runs the container anyway ("Running container: ..." follows immediately). The job died from theAttributeError.The fix
New shared helper
annotation_tools.get_selected_channels(value, field_name):{"0": True, "1": False, "2": True}→[0, 2])[]for an unset field (None,'',{})ValueErroron any other shape, including a bare listEvery consumer routes through it and reports the
ValueErrorviasendError:cellposesam,cellposesam_train,registration,deconwolf,histogram_matching,gaussian_blur,rolling_ball, andsample_interface(which now demonstrates the canonical read for the field it previously only declared).cellposesam's slot resolution is factored into aget_slot_channels()helper mirroring the onecellposesam_trainalready had.Empty-selection behavior is deliberately unchanged per worker: the workers that already errored on no channels still do, and
gaussian_blur/rolling_ballstill write out an unmodified copy (their existing tests assert this).Why the list shape is rejected rather than normalized
An earlier revision of this PR accepted
[0]as "channel 0". It no longer does:ChannelCheckboxGroup.vuehas been typedRecord<number, boolean>since it was introduced (2025-01-31) and still is — the UI has never emitted a list.normalizeWorkerInterfaceValue(src/utils/workerInterface.ts), which converts them to the canonical map before saving.So a list value means the config was written by something outside the UI, and we cannot confirm which channel it intended. Running a segmentation or deconvolution against a guessed channel silently produces wrong results; failing with an actionable message does not. Tolerating the shape would also keep a second wire format alive indefinitely for a payload nothing is known to still emit.
Consequence to be aware of: tool configs that already hold list values now report "Could not read the channel selection" until their channels are re-selected and re-saved. That is intended, and the cleanup is tracked in TODO-004.
Verification
Docker isn't available in this environment, so the suites were run natively against the real
annotation_client,large_image,skimageandpystackreg— 200 tests pass:annotation_utilities/testsworker_client/testsgaussian_blurrolling_ballhistogram_matchingregistrationdeconwolfsample_interfacecellposesamcellposesam_trainThe exact
workerInterfacepayload from the crash log was replayed through the realget_slot_channels()and now yields thesendErrorpath with an actionable message. Each new per-worker test was confirmed to fail both against a list-accepting helper and against master's pre-fix entrypoint, so they pin the new behavior rather than passing vacuously.Worth re-running
./build_workers.sh --build-and-run-tests <worker>on a Docker host before merge, since that is the canonical path.Follow-up
The submitter that wrote
"Channel for Slot 1": [0]on 2026-05-29 is still unidentified. It was not the checkbox UI, and it was not the AI panel — that landed 2026-07-06, five weeks after the crash — which leaves a script or REST/girder_clientcall writingworkerInterfaceValuesdirectly. Tracked as TODO-004 (todo/channelcheckboxes-serialization.md) along with repairing the already-broken saved configs.Notes for review
.gitignorechanges already landed on master independently.Docs
The helper and the reject-don't-guess rationale are recorded in
CLAUDE.md/AGENTS.md, thenimbus-interfacereference, thenimbus-worker-hardeningandnimbus-worker-scaffoldskills (both.claude/and.agents/copies), and the affectedWORKERNAME.mdfiles.🤖 Generated with Claude Code