[Fable 5 Findings] Phase 3 — Correctness bugs, silent failures & misleading contracts

[suraj-ranganath]

🤖 Auto-generated from the thermo-nuclear whole-codebase code-quality review at commit 8ba958e (tip of PR #190). Part of the Fable 5 Findings epic #193.

Rationale

Bugs where the running program produces a wrong result, crashes, silently does the wrong thing, or silently ignores user input. This includes off-by-one / indexing errors in core signal ops, crashes on documented code paths, except Exception blocks that mask real load/clean failures, options that are validated and recorded but never take effect, and self-describing CLI schemas that contradict the actual CLI. Each erodes researcher trust because the failure is invisible.

How to approach

Fixes: dispatch on real conditions instead of catch-all except; make ignored options either work or raise; correct the index/boundary math against the EEGLAB oracle with a focused test; and make CLI/schema/exit-code contracts match behavior. Several of these also violate AGENTS.md's explicit ban on over-protective try/except and silent fallbacks.

Findings in this phase (38)

Every item below was confirmed by an independent adversarial verification pass against the working tree. Check items off as they land. Full per-finding detail (with verifier notes and full evidence) is in THERMO_NUCLEAR_REVIEW.md on the review branch.

1. eeg_compare: docstring/return contract mismatch, print-based output, broad except, dead code

• functions/popfunc/eeg_compare.py (lines 13-31, 96-390)
  • Why: This is a runtime-exposed function (registered in src/eegprep/init.py:87 and wrapped in functions/redefine_functions.py:44-45), not test-only. Its docstring states 'Returns bool — True if comparison completed', but every top-level return path returns summary (a str) (l.107, 144, 390). It also drives output via 31 print() calls split between stdout and stderr (AGENTS.md prefers logging), and…
  • Failure mode: Callers relying on the documented bool return get a string truthy value instead of a comparison result; the function can't be used programmatically for pass/fail without parsing prose. Broad excepts mask real comparison bugs (e.g. dtype errors) as 'not equal'. Output is unsuppressable print spam.
  • Fix: Decide a single contract (e.g. return (ok: bool, summary: str) or just the summary) and align the docstring; replace prints with logging or an accumulated report string; narrow the excepts to the specific numpy/type errors expected; delete…
  • Evidence: Docstring 'Returns bool / True if comparison completed' (l.28-31) vs return summary (l.390); except Exception: pass (l.51,57,63,68); commented # import matplotlib.pyplot as plt (l.327).

2. eeg_eegrej wrapper drops first/last events by latency value regardless of type

• functions/popfunc/eeg_eegrej.py (lines 332-335)
  • Why: After the backend already inserts/merges boundaries, the wrapper does ad-hoc edge cleanup with exact-equality latency checks that ignore event type, duplicating and overreaching the backend's own boundary handling.
  • Failure mode: if EEG['event'][-1].get('latency') == EEG['pnts']: EEG['event'] = EEG['event'][:-1] deletes the last event whenever its latency equals pnts (the final sample) even when it is a legitimate non-boundary stimulus event — silent data loss. The symmetric latency == 0 check at 332 has the same type-agnostic flaw.
  • Fix: Guard these edge drops on _is_boundary_event(EEG['event'][...]) like EEGLAB's eeg_eegrej, or remove them and rely on the backend's boundary insertion/merge which already runs.
  • Evidence: Lines 332-335: if len(EEG["event"]) > 1 and EEG["event"][0].get("latency") == 0: EEG["event"] = EEG["event"][1:] and ... EEG["event"][-1].get("latency") == EEG["pnts"]: EEG["event"] = EEG["event"][…

3. Epoched latency edit path is mislabeled and asymmetric vs display, corrupting values on edit

• functions/popfunc/pop_editeventvals.py (lines 176-192, 195-206, 209-214)
  • Why: For continuous data the display/edit pair is symmetric: display converts points→seconds (202) and _internal_event_value converts seconds→points (188). For epoched data they diverge: _display_event_value returns the raw stored latency in points (200-201) while the field is labeled 'Latency (ms)' (211) and _internal_event_value interprets typed input as ms via eeg_lat2point (178-187).
  • Failure mode: On an epoched dataset, the GUI shows a latency in points but labels it ms; if the user edits that value it is converted as if it were ms, producing a wrong sample latency. The displayed value also can never equal what the edit path expects, so even an intended no-op edit is interpreted as a unit change.
  • Fix: In _display_event_value, convert the epoched latency to ms (mirroring _internal_event_value's eeg_lat2point round-trip) so the displayed value matches the 'ms' label and the edit conversion.
  • Evidence: Line 200-201 if int(EEG.get("trials", 1) or 1) > 1: return value; line 178-187 epoched branch wraps eeg_lat2point(float(value), event.get("epoch",1), srate, [xmin*1000, xmax*1000], 1e-3); label at…

4. Silent .mat fallback chain in pop_fileio swallows real .set load errors

• functions/popfunc/pop_fileio.py (lines 24-30)
  • Why: For .mat inputs, pop_fileio first tries pop_loadset and on except Exception retries as a raw pop_importdata MATLAB array. Because pop_loadset itself already swallows errors into an HDF5 retry (finding above), a genuinely broken .set silently lands in the 'import as anonymous matlab array' branch, producing an EEG dict with synthetic channel labels and no events instead of surfacing the loa…
  • Failure mode: A valid-but-slightly-malformed EEGLAB .set silently loads as a bare data matrix (wrong nbchan orientation, lost chanlocs/events), and the user/console cannot tell from history that it was not loaded as a dataset.
  • Fix: Dispatch on declared dataformat/content rather than catch-all except Exception; only fall back to matlab-array import when the variable layout proves it is not an EEGLAB struct, and record which path was used.
  • Evidence: try: eeg = pop_loadset(str(path)) except Exception: eeg = pop_importdata("data", str(path), "setname", path.stem, "dataformat", "matlab", **kwargs)

5. Bare except masks .set load failures behind an HDF5 retry

• functions/popfunc/pop_loadset.py (lines 84-93)
  • Why: The v7 scipy.io.loadmat path is wrapped in try/except Exception whose sole fallback is pop_loadset_h5(file_path). Any failure during scipy load OR the new_check post-processing (a malformed struct, an unsupported dtype, a bug in conversion) is swallowed and reinterpreted as 'this must be a v7.3/HDF5 file'. If the file genuinely is not HDF5, h5py raises its own unrelated error, and the orig…
  • Failure mode: A corrupt or partially-written v7 .set raises a confusing h5py 'file signature not found' error instead of the real parse error; a real bug in new_check is hidden because every exception silently triggers the HDF5 branch.
  • Fix: Dispatch on the actual format (sniff the file header / catch only NotImplementedError from scipy for v7.3) rather than except Exception, and re-raise non-format errors with context.
  • Evidence: try: EEG = scipy.io.loadmat(...) EEG = new_check(EEG) ... except Exception: EEG = pop_loadset_h5(file_path) loaded_with_h5 = True

6. Hard-coded test fixture (emoji byte sequence) embedded in the runtime HDF5 loader

• functions/popfunc/pop_loadset_h5.py (lines 33-37)
  • Why: convert_to_string contains a literal special case that matches one exact uint16 byte sequence and returns the string 'hello 👖'. This is a test fixture baked into a production decoder; it makes the loader's behavior depend on a magic constant rather than on a correct general decoding rule, and signals the general uint16->UTF-8 path below it is not trusted for this input.
  • Failure mode: The general decoder silently mis-handles any other multibyte/surrogate string the same way the emoji case would have, but only the one fixture sequence is correct; the dead-looking branch hides whatever real bug motivated it and will rot when the test changes.
  • Fix: Remove the literal-match branch and fix the general uint16->bytes->UTF-8 decode (handle surrogate pairs / endianness) so all strings decode by one rule; move the emoji expectation into the test, not the loader.
  • Evidence: if len(filecontent) == 10 and np.array_equal( filecontent, np.array([104, 101, 108, 108, 111, 32, 240, 159, 146, 150]) ): return 'hello 👖'

7. chantype / rmchantype selection crashes: eeg_decodechan return value not unpacked

• functions/popfunc/pop_select.py (lines 210-217 (vs correct 188, 202))
  • Why: eeg_decodechan always returns a 2-tuple (chaninds, chanlist_out) (eeg_decodechan.py:73-74,113-114). The label paths unpack correctly (inds, _ = eeg_decodechan(...) at 188 and 202), but the chantype/rmchantype paths assign the whole tuple: inds = eeg_decodechan(EEG, g['chantype'], 'type', True).
  • Failure mode: With a chantype-only selection, np.array(inds, dtype=int) at line 213/217 receives ([0,1,...], ['EEG','EEG',...]) and raises ValueError (can't cast type-label strings to int). So pop_select(EEG, chantype=['EEG']) raises instead of selecting EEG channels.
  • Fix: Unpack like the label branches: inds, _ = eeg_decodechan(EEG, g['chantype'], 'type', True) on lines 211 and 216.
  • Evidence: Line 211 inds = eeg_decodechan(EEG, g['chantype'], 'type', True) then 213 chan_selected_flag[np.array(inds, dtype=int)] = True. No test exercises chantype-only: tests/test_pop_select.py:282 pairs…

8. pop_select numeric channel API is 0-based but GUI/history emits 1-based — replay is off-by-one

• functions/popfunc/pop_select.py (lines 170-174, 717-727, 769-775)
  • Why: _numeric_channel_indices validates against [0, nbchan) and indexes chan_selected_flag 0-based; tests/test_pop_select.py:332 confirms channel=[0,2] means Fz/Pz. But _run_gui keeps the user's 1-based GUI numbers in options, _gui_options_for_apply builds a separate apply dict with value-1 (727) while _history_command(options) (45,769) emits the un-decremented 1-based values. So the emitted histor…
  • Failure mode: A GUI selection of channels 1-3 emits pop_select(EEG, 'channel', [1 2 3]); re-running that string in eegprep-console selects 0-based channels 1,2,3 (= EEGLAB channels 2,3,4). The console converter does not reconcile this for pop_select (tests/test_console_workspace.py:1537 asserts channel=[1, 2] is passed through unchanged) even though it explicitly decr…
  • Fix: Make pop_select's contract consistent: either emit 0-based channel indices from _history_command (so it round-trips through the 0-based API) or decrement GUI channel values before storing them in options so history is 1-based AND the cons…
  • Evidence: Line 724 apply_options[key] = [value - 1 if isinstance(value, int) else value ...] (apply only); line 718 apply_options = dict(options) leaves options 1-based; line 774 formats raw value. Test…

9. pop_spectopo silently drops the icamode and plotchan component controls

• functions/popfunc/pop_spectopo.py (lines 51, 184-192, 238-249)
  • Why: The component dialog exposes plotchan ('Electrode number to analyze ([]=elec with max power; 0=whole scalp)') and icamode ('Compute comp spectra vs (data-comp) spectra'). _run_gui collects both into options (l.187, 191), but pop_spectopo strips them from history (history_options excludes them, l.51) and from the topoplot call (_split_spectopo_options pops them, l.248), and never uses t…
  • Failure mode: A user who sets plotchan to a specific electrode or unchecks icamode gets whole-component spectra regardless, with no error and no history record, diverging from EEGLAB behavior the dialog promises. Silent no-op of user-facing controls.
  • Fix: Either implement plotchan/icamode in the component spectra path, or raise a clear ValueError (matching pop_erpimage's raise_for_unsupported* pattern) when a non-default value is supplied, so the limitation is explicit rather than silent.
  • Evidence: history_options = {key: value for key, value in options.items() if key not in {"plotchan", "icamode"}} (l.51); for unused in {"icacomps", "icamaps", "nicamaps", "plotchan", "icamode"}: topoplot.pop…

10. Worker.run mutates the process-global 'eegprep' logger level and handler list from a background thread

• functions/guifunc/long_task.py (lines 54-72)
  • Why: Worker.run (running on the QThread) calls eegprep_logger.setLevel(...) and eegprep_logger.addHandler(handler) on the shared logging.getLogger('eegprep') instance to capture progress, then restores in finally. The progress dialog is WindowModal (line 46), which blocks the parent window but still spins the main-thread event loop, so other main-thread code that logs through the 'eegprep' logger runs…
  • Failure mode: If a second long task is started (modality only blocks the parent window; a dialog with a different/None parent, or a non-modal path, is not prevented) or main-thread code changes the eegprep logger level while a task is running, the finally-block restore writes back the level this worker captured at entry, clobbering the other change. The user can also ob…
  • Fix: Avoid mutating the shared logger's level: attach the temporary handler at INFO and rely on the handler's own level instead of lowering the logger level, or scope progress capture to a child logger/QueueHandler created per task. If the logge…
  • Evidence: long_task.py:58-62 eegprep_logger = logging.getLogger("eegprep") ... eegprep_logger.setLevel(logging.INFO); eegprep_logger.addHandler(handler) and long_task.py:70-71 eegprep_logger.removeHandler(ha…

11. IMPLEMENTED_ACTIONS is a hand-maintained third registry that can silently disagree with the dispatch table

• functions/guifunc/menu_actions.py (lines 28-146, 246-481, 1836-1847)
  • Why: IMPLEMENTED_ACTIONS (28-146, ~118 literals) is the source of truth for action_kind()=='implemented' (1841), which dispatch() consults at line 249 to decide whether to short-circuit into show_coming_soon, and which the menu/tests use to gate enable state. But nothing ties an entry in this set to the existence of a matching dispatch branch; the set is a manually curated mirror of the if/elif chain.…
  • Failure mode: A name listed in IMPLEMENTED_ACTIONS but lacking a dispatch arm reports action_kind=='implemented' (menu item shown enabled, no coming-soon suffix), yet clicking it falls through to show_coming_soon at line 481 — an enabled menu item that does nothing useful. Conversely a real dispatch branch missing from the set is classified 'unknown'/placeholder. No test…
  • Fix: Derive implemented-action membership from the actual dispatch routing (e.g. a single action->handler mapping that both dispatch() and action_kind() read), or add a test that asserts every IMPLEMENTED_ACTIONS entry reaches a non-coming-soon…
  • Evidence: if action_kind(action, extension_runtime=self.extension_runtime) == "placeholder":\n self.show_coming_soon(action, parent)\n return

12. _commit_processed_dataset_from_gui splits one dataset commit across eegh + add_history + apply_workspace_state, with a non-atomic abort path

• functions/guifunc/menu_actions.py (lines 1560-1588)
  • Why: This commit hand-rolls the session update instead of going through a single EEGPrepSession transaction: it mutates dataset history directly via eegh(command, dataset) at 1571, then (only if pop_newset returns a command) calls add_history(command) at 1586 followed by a separate apply_workspace_state(command=newset_command) at 1588. If newset_command is falsy it abandons the work and calls session.r…
  • Failure mode: User runs a filter/select via the GUI; pop_newset declines to create a set (cancel/no-op). The dataset's history field now contains the processing command while session ALLCOM does not, so saved-set history and console history disagree — exactly the GUI/console history desync AGENTS.md warns against.
  • Fix: Route the whole commit through one EEGPrepSession helper (apply_workspace_state already supports append_dataset_history=True) so dataset-level history is appended in the same transaction that updates ALLEEG/CURRENTSET, and not written befor…
  • Evidence: for dataset in eeg if isinstance(eeg, list) else [eeg]:\n if isinstance(dataset, dict):\n eegh(command, dataset)\n... if not newset_command:\n if old_selection:\n self.session.retrieve(...)\n return

13. Headplot menu action mutates session EEG without store_current/notify_changed

• functions/guifunc/menu_actions.py (lines 1560-1563, 1600-1601)
  • Why: The GUI handler calls pop_headplot(selection, ..., return_com=True) where selection is the live session dataset, then only runs self._add_history_from_gui(command) and self._refresh(). Because pop_headplot mutates selection['splinefile'] in place (see pop_headplot finding), the session's in-memory EEG changes, but the change never goes through store_current/notify_changed, and the re…
  • Failure mode: GUI and console desync: after a headplot, the console workspace EEG has splinefile set but no history line records it; if the user re-runs history on a freshly loaded dataset the recomputed/attached spline file is lost, and a later pop_saveset may or may not persist a field that was set outside the session's change tracking. State that EEGLAB treats as a com…
  • Fix: Treat pop_headplot's spline-file assignment as a dataset edit: have pop_headplot return the EEG and have the menu handler commit it via store_current/notify_changed (and emit an EEG = pop_headplot(...)-style history), or explicitly persis…
  • Evidence: _result, command = pop_headplot(selection, typeplot=..., return_com=True) (l.1563) followed only by self._add_history_from_gui(command) / self._refresh() (l.1600-1601); no store_current.

14. Reassigning only CURRENTSET in the console overwrites the target dataset with the previously-selected one

• functions/adminfunc/console.py (lines 357-376, 487-488)
  • Why: EEGPrepConsoleWorkspace.after_execute is the single point where console-side workspace edits are pushed back into the session, and switching the current dataset via CURRENTSET is an explicitly-supported flow (the ALLEEG/CURRENTSET branch resolves EEG from the new index). A wrong store here silently corrupts loaded datasets in ALLEEG.
  • Failure mode: With two loaded datasets (CURRENTSET=[1], EEG=dataset1), typing CURRENTSET = 2: (1) the "CURRENTSET" in targets branch calls apply_workspace_state(currentset=[2]) which sets session.EEG=dataset2; (2) _namespace_eeg_changed({CURRENTSET}) then returns True because "EEG" not in targets but namespace["EEG"] is still the stale dataset1 (pull_from_sessio…
  • Fix: Make the EEG-store branch fire only on a real EEG edit. Capture session.EEG before the ALLEEG/CURRENTSET apply and compare against that pre-apply object (or gate on "EEG" in targets or namespace EEG is not <pre_apply_eeg>), so a CURRENTSE…
  • Evidence: def _namespace_eeg_changed(self, targets): return "EEG" in targets or self.namespace.get("EEG") is not self.session.EEG and in after_execute: if self._namespace_eeg_changed(targets): eeg = self.nam…

15. eegrej() shifts boundary latencies by augmented (nested-boundary) durations instead of base region spans

• functions/sigprocfunc/eegrej.py (lines 138-143)
  • Why: EEGLAB eegrej.m computes the cumulative latency offset for later boundary events using only the base region span: boundevents(iRegion2) -= (regions(i,2)-regions(i,1)+1) (eegrej.m L139). The Python port instead subtracts np.cumsum(durations[:-1]) where durations has already had nested-boundary-event durations added in (line 136 durations[i_region] += extra). So the placement of boundary events…
  • Failure mode: When eegrej is run on data that already contains boundary events with duration (e.g. rejecting more segments from already-cleaned continuous data), the inserted boundary latencies for the 2nd+ regions are pulled too far left by the accumulated nested durations, desynchronizing boundary positions from the actually-excised samples. Event/boundary latency bookk…
  • Fix: Shift boundary latencies using base_durations (already computed at line 123), not the augmented durations: build cums from base_durations[:-1]. Keep the augmented durations only for the inserted boundary events' duration field, matching…
  • Evidence: Python L123 base_durations = (r[:, 1] - r[:, 0] + 1), L136 durations[i_region] += extra, L141 cums = np.concatenate([[0.0], np.cumsum(durations[:-1])]), L142 boundevents = boundevents - cums.…

16. epoch() data-boundary check is off by one vs EEGLAB; admits negative-index slice

• functions/sigprocfunc/epoch.py (lines 94-103)
  • Why: epoch() is the numeric core behind pop_epoch(). EEGLAB epoch.m treats posinit=pos0+reallim(1) as a 1-based MATLAB index and rejects events where posinit<1 (slice would start before the first sample). The Python port computes the identical posinit (line 89) but then double-shifts: the slice uses start = posinit - 1 (treating posinit as already 1-based, line 102 — this matches MATLAB), while the bou…
  • Failure mode: An event near the recording start with lim[0]=0 such that pos0+reallim[0]==0 passes the check (posinit_1based=1 >= 1) but yields start = posinit - 1 = -1. data[:, -1:end_excl] wraps to the last column / returns an empty slice, so epochdat[:,:,index] = tmpdata either raises a broadcast error or writes a malformed epoch. EEGLAB would have rejected that event a…
  • Fix: Make the boundary check use the same coordinate as the slice. Since start = posinit - 1 is the correct 0-based slice start matching MATLAB, the check should be on posinit/posend directly in MATLAB coordinates: within_bounds = (posinit >= 1)…
  • Evidence: Python L89 posinit = pos0 + reallim[0], L94 posinit_1based = posinit + 1, L97 within_bounds = (posinit_1based >= 1) and (posend_1based <= datawidth), L102 start = posinit - 1. MATLAB epoch.m L…

17. runamica() leaks its temp directory when the AMICA run or output load fails

• functions/sigprocfunc/runamica.py (lines 802-858)
  • Why: When outdir is None, runamica creates tempfile.mkdtemp(prefix='amica_') (line 804) and only deletes it inside the cleanup block at the very end (lines 849-858), which is reached only on the success path. _run_amica (line 831, uses check=True and raises RuntimeError on nonzero exit) and _load_amica_output (line 834, can raise FileNotFoundError on missing 'W') both run before cleanup with no try/fin…
  • Failure mode: Every failed AMICA invocation (segfault, bad params, missing output) leaves an amica_* temp directory containing the full float32 .fdt copy of the data on disk. Repeated GUI retries on a failing binary accumulate large orphaned temp dirs.
  • Fix: Wrap the run/load in try/finally so a temp dir created here is removed on failure too (only when cleanup and tmp_created), or use a tempfile.TemporaryDirectory context for the tmp_created case.
  • Evidence: L802-805 tmp_created = False / if outdir is None: outdir = tempfile.mkdtemp(prefix='amica_'); tmp_created = True; L831 _run_amica(binary, param_file); L849-857 cleanup if cleanup: ... if tmp_crea…

18. std_makedesign 'delfiles' option is strictly validated and recorded but has no effect; cached measures are cleared unconditionally regardless of its value

• functions/studyfunc/std_makedesign.py (lines 44, 74, 78-79, 108, 115)
  • Why: delfiles is parsed, validated against {'on','off','limited'}, and written into the history command, but it never influences behavior. At line 108 std_makedesign always calls std_selectdesign, which always calls clear_study_data_fields (std_selectdesign.py line 18). So cached measure arrays in changrp/cluster are wiped on every std_makedesign call, whether delfiles is 'on', 'off', or 'limited'. The…
  • Failure mode: A user calls std_makedesign(..., delfiles='off') intending to keep precomputed measures attached to the redefined design (the EEGLAB meaning). The measures are silently discarded anyway via std_selectdesign->clear_study_data_fields. The strict validation gives a false impression the flag is meaningful, and the recorded history command preserves an option tha…
  • Fix: Make delfiles actually gate clearing: only clear cached data fields when delfiles != 'off', or pass the desired clearing behavior into std_selectdesign. If clearing on design change is intended to always happen, remove delfiles (and its val…
  • Evidence: line 78-79: if delfiles not in {"on", "off", "limited"}: raise ValueError(...); line 108: study = std_selectdesign(study, datasets, design_index); std_selectdesign.py line 18: study = clear_study…

19. std_precomp 'recompute' flag is parsed and recorded but never gates recomputation (silent no-op, inconsistent with std_pac)

• functions/studyfunc/std_precomp.py (lines 66, 84-110, 116, 638-639)
  • Why: EEGLAB's std_precomp passes 'recompute' down so that 'off' skips measures whose cache already exists. Here the flag is accepted, stored in STUDY.etc.eegprep.study_measures and the history command, but the precompute path (_precompute_channels / _precompute_components) unconditionally overwrites study['changrp'] and study['cluster'][0]. The sibling std_pac STUDY mode DOES honor it ('if "pacdata" in…
  • Failure mode: A user calls std_precomp(..., recompute='off') expecting existing cached ERP/spec/ERSP/ITC arrays to be preserved (matching EEGLAB and matching std_pac). Instead every measure is recomputed from scratch every call. For large STUDYs this silently burns large amounts of compute (newtimef per dataset/component) and the recorded history command is misleading: re…
  • Fix: Either honor recompute='off' by short-circuiting when the target measure field already exists on the group/cluster (mirroring the std_pac pattern), or, if standalone EEGPrep intentionally always recomputes in-memory, drop the flag entirely…
  • Evidence: line 66: recompute = options.pop("recompute", recompute); line 116: "recompute": "on" if is_on(recompute) else "off",; grep shows no use of recompute inside _precompute_channels/_precompute_c…

20. compute_pac strictly rejects unknown kwargs but silently drops accepted title/vert/newfig

• functions/timefreqfunc/_pac_support.py (lines 88-96)
  • Why: compute_pac raises TypeError for any unrecognized kwarg (lines 92-94) and raises NotImplementedError for unsupported alpha (line 96), establishing a 'fail loudly on unimplemented options' contract. Yet title, vert, and newfig are accepted as named parameters and then discarded via _ = title, vert, newfig (line 91).
  • Failure mode: Inconsistent contract: a typo'd option name fails loudly, an unsupported alpha fails loudly, but three plotting-related options vanish without effect. A user supplying vert= markers for a PAC plot gets a silently plotless, marker-less result, contradicting the function's own strictness elsewhere.
  • Fix: Treat title/vert/newfig the same way as alpha for the no-plot epoched path: either drop them from the signature or raise NotImplementedError when a non-default plotting value is supplied.
  • Evidence: _pac_support.py:91 _ = title, vert, newfig; :93-94 if unsupported: raise TypeError(f"Unsupported pac option(s): ..."); :96 if alpha is not None: raise NotImplementedError(PAC_UNSUPPORTED_MESSAGE)…

21. Local _is_on uses opposite (blacklist) semantics from canonical is_on (whitelist)

• functions/timefreqfunc/newtimef.py (lines 700-701)
  • Why: Canonical popfunc._pop_utils.is_on returns True only for the whitelist {1,on,true,yes}. The timefreqfunc-local _is_on returns True for anything NOT in {0,false,off,no,none}. These are inverted defaults for unrecognized values.
  • Failure mode: For an unexpected toggle value (e.g. plot='display', plotitc='yes-please', or a numpy array), newtimef/newcrossf treat it as ON while the rest of EEGPrep (pop_* dialogs/history) treats the same value as OFF. Because newtimef defaults plot/plotersp/plotitc to 'on', the happy path agrees, masking the divergence until a non-canonical value flows in, at which po…
  • Fix: Use the canonical is_on from popfunc._pop_utils in newtimef.py and newcrossf.py instead of the local blacklist variant.
  • Evidence: newtimef.py:701 return str(value).lower() not in {"0", "false", "off", "no", "none"} (byte-identical in newcrossf.py:417) vs _pop_utils.py:102 return value.strip().lower() in {"1", "on", "true", "y…

22. newtimef accepts dead parameters overlap and plotphase that silently affect nothing

• functions/timefreqfunc/newtimef.py (lines 38-87)
  • Why: newtimef declares ~48 parameters; overlap and plotphase are consumed only by line 87 _ = overlap, plotphase and never reach any computation or plotting. compute_time_frequency has the same dead overlap (line 245).
  • Failure mode: A user (or EEGLAB migrant) calling newtimef(..., plotphase='on') reasonably expects a phase panel and gets nothing, with no error or warning. The signature advertises capability the implementation does not have, and there is no marker distinguishing 'not yet ported' from 'intentionally ignored'.
  • Fix: Either implement these options or remove them from the public signature; if they are intentional EEGLAB-parity placeholders, raise NotImplementedError for non-default values (as compute_pac does for alpha) instead of silently discarding the…
  • Evidence: newtimef.py:87 _ = overlap, plotphase; params declared at newtimef.py:52 overlap: Any = None, and :74 plotphase: Any = "off",; no other reference to either name in the module body.

23. gen_derived_fpath documents ${root} but only substitutes {root}, and mixes os.path.join with hardcoded '/'

• plugins/EEG_BIDS/bids.py (lines 71-126)
  • Why: The default value in the signature is outputdir='${root}/derivatives/clean_artifacts', but the substitution logic only handles the literal '{root}' (line 116 'if "{root}" in outputdir: outputdir = outputdir.format(root=root)'). A caller relying on the documented '${root}' form would get the placeholder passed through verbatim. Separately the final path is assembled with an f-string '/' (line 125)…
  • Failure mode: Anyone copying the documented default or passing '${root}/...' gets a literal '${root}' directory created instead of the BIDS root. The '/'-vs-os.path.join inconsistency produces forward-slash paths on Windows. bids_preproc itself uses '{root}' (line 454 default '{root}/derivatives/eegprep') so the bug is dormant for the primary caller, but the public helper…
  • Fix: Pick one placeholder form ('{root}') in the signature default and docstring, and build out_path with os.path.join for portability.
  • Evidence: Default: "outputdir: str = '${root}/derivatives/clean_artifacts',". Substitution: "if '{root}' in outputdir: outputdir = outputdir.format(root=root)". Final assembly: 'out_path = f"{outputdir}/{root_r…

24. icawinv emptiness/ICA validation runs after it is already dereferenced

• plugins/ICLabel/ICL_feature_extractor.py (lines 29-50)
  • Why: The guard meant to produce a clear 'You must have an ICA decomposition' error is placed after the code that already requires icawinv to exist and be 2-D, so the guard is dead for the case it was written for.
  • Failure mode: Line 32 ncomp = EEG['icawinv'].shape[1] executes before the line 35 check if 'icawinv' not in EEG.keys() or EEG['icawinv'].size == 0. If icawinv is missing the dict access raises KeyError; if it is an empty/0-d array .shape[1] raises IndexError. Either way the user gets an opaque error instead of the intended ValueError. Also line 39 reads EEG['ref']…
  • Fix: Move the icawinv existence/size check above line 32, and guard EEG.get('ref') before comparing it.
  • Evidence: ncomp = EEG['icawinv'].shape[1] (line 32) precedes if 'icawinv' not in EEG.keys() or EEG['icawinv'].size == 0: raise ValueError('You must have an ICA decomposition to use ICLabel') (lines 35-36).

25. asr_calibrate silently substitutes a trivial filter for unsupported sample rates

• plugins/clean_rawdata/asr_calibrate.py (lines 358-368)
  • Why: For any srate not in the precomputed table (e.g. 1000 Hz, 1024 Hz, 333 Hz), the spectral-shaping IIR is replaced by a degenerate first-difference filter B=[1,-1], A=[1] with only a logger.warning. The MATLAB reference raises an error here ('asr_calibrate:NoYulewalk') rather than silently proceeding.
  • Failure mode: ASR thresholds are computed on data shaped by a wildly different filter than intended, so the calibration covariance and per-component thresholds are wrong. The user gets plausible-looking but quietly miscalibrated artifact rejection with no error and only a log line that is easy to miss, on common sample rates like 1000/1024 Hz.
  • Fix: Either implement a yulewalk-based filter design to cover arbitrary srate (matching MATLAB), or raise a clear ValueError naming the unsupported srate and the supported set, instead of silently degrading to a difference filter.
  • Evidence: logger.warning(f"No pre-computed spectral filter for srate {srate}. Using a simple default (may be suboptimal).") then B = np.array([1.0, -1.0]) # Simple high-pass/difference filter as a basic fall…

26. Bare-broad except around component selection silently disables artifact removal

• plugins/clean_rawdata/asr_process.py (lines 196-203)
  • Why: The per-update component-selection block wraps threshold computation in except Exception that, on any error, sets keep = all True and trivial = True, i.e. reconstructs nothing for that window. Combined with the eigendecomposition fallback above (lines 190-193) and the pinv fallback (218-221), three layers of catch-and-continue can turn ASR into a no-op for affected windows while logging only…
  • Failure mode: A shape/contract bug in T/V handling becomes an invisible 'this window was left uncleaned' rather than a surfaced failure; the cleaned output silently contains unrepaired bursts. Because it is per-update inside a hot loop, sporadic numerical issues degrade results non-deterministically with no aggregate signal to the user.
  • Fix: Let unexpected exceptions propagate (or fail the call) and reserve fallbacks for the specific, expected numerical-singularity cases (np.linalg.LinAlgError), which are already handled separately.
  • Evidence: except Exception as e: logger.error(f"Error in component selection: {e}") keep = np.ones(C, dtype=bool) trivial = True

27. Bare/broad except masks real failures in channel selection and reinsertion

• plugins/clean_rawdata/clean_artifacts.py (lines 156-181, 224-233)
  • Why: Several try/except Exception blocks swallow all errors and silently fall back to a different code path, hiding genuine bugs (e.g. a malformed chanlocs entry) behind the documented 'pop_select missing' fallback.
  • Failure mode: Lines 158-168 and 171-182: except Exception: after from eegprep import pop_select catches not only ImportError but any error raised inside pop_select (bad channel labels, shape mismatch), then runs a manual label-index fallback that can produce a differently-selected dataset than pop_select would, with no warning. Lines 224-233: `except Exception as e:…
  • Fix: Narrow the channel-selection excepts to ImportError (the only documented fallback trigger) and let other exceptions propagate; for the clean_channels fallback, restrict to the location-related error it is meant to handle (e.g. the ValueErro…
  • Evidence: try:\n from eegprep import pop_select\n EEG = pop_select(EEG, channel=list(Channels))\nexcept Exception: and except Exception as e:\n logger.warning(f'clean_channels failed ({e}); falling back to c…

28. Documented MATLAB behavior (re-insert ignored channels) silently dropped

• plugins/clean_rawdata/clean_artifacts.py (lines 294-301)
  • Why: When Channels/Channels_ignore restrict the dataset, MATLAB clean_artifacts re-inserts the previously excluded channels into the final output. The port explicitly skips this 'for simplicity' and earlier wipes EEG['event'] = [] (lines 169,182) intending to restore later — but there is no restore.
  • Failure mode: A user who passes Channels_ignore (e.g. to skip ECG/EMG during cleaning) gets back a dataset that permanently lost those channels AND lost all events, diverging from EEGLAB output and from the function's own docstring which lists no such limitation. The 'will be restored later' comment is a false promise in code.
  • Fix: Either implement the channel/event re-insertion to match MATLAB, or raise NotImplementedError when Channels/Channels_ignore are supplied so the silent data loss cannot occur; update the docstring accordingly.
  • Evidence: # The full MATLAB logic is complicated; the Python port currently skips the # re‑insertion of previously excluded channels for simplicity. Users can # merge channels back manually if needed. return EE…

29. Broad except on clean_windows calibration silently falls back to using all data

• plugins/clean_rawdata/clean_asr.py (lines 107-127)
  • Why: Automatic calibration-data selection wraps clean_windows in except Exception, logs a warning, and proceeds with the entire (possibly heavily artifacted) dataset as ASR calibration. The empty/short-result branches above (lines 114-121) also silently fall back. ASR calibration quality directly sets the rejection thresholds, so a swallowed error here degrades cleaning without surfacing the cause.
  • Failure mode: A real bug or bad input in clean_windows (or fit_eeg_distribution) is masked as a benign 'falling back to entire data' warning; the user gets ASR thresholds computed on dirty data and silently weaker artifact removal, with no error to debug. Programming errors and legitimate data problems are indistinguishable.
  • Fix: Narrow the except to the expected calibration-failure exceptions (e.g. ValueError from insufficient windows) and let unexpected exceptions propagate, so genuine failures are visible.
  • Evidence: except Exception as e: logger.error(f"An error occurred during clean_windows: {e}") logger.warning( "Could not automatically identify clean calibration data. Falling back to using the entire data for…

30. Walrus precedence bug corrupts clean_channel_mask update in fallback path

• plugins/clean_rawdata/clean_flatlines.py (lines 70-71)
  • Why: The := binds the comparison result, not the dict value, so CCM becomes a bool and the subsequent subscript is invalid. This is reachable in the no-pop_select fallback when a prior clean_channel_mask exists.
  • Failure mode: if CCM := EEG['etc'].get('clean_channel_mask') is not None: parses as CCM := (EEG['etc'].get(...) is not None), making CCM True/False. Then CCM[CCM] = ~removed_channels raises TypeError: 'bool' object is not subscriptable whenever a previous clean_channel_mask is present and pop_select import failed. The intended mask compositing (mask[mask] = ~remov…
  • Fix: Split into CCM = EEG['etc'].get('clean_channel_mask') then if CCM is not None: CCM[CCM] = ~removed_channels. The sibling functions clean_channels.py (line 175) and clean_channels_nolocs.py (lines 133-139) already do this correctly; cons…
  • Evidence: if CCM := EEG['etc'].get('clean_channel_mask') is not None: followed by CCM[CCM] = ~removed_channels.

31. BIDS .set import silently falls back by sniffing an IndexError message string

• cli/commands/bids.py (lines 296-319)
  • Why: _import_bids_file calls pop_importbids; on IndexError it inspects str(exc) for the substring 'only integers' and the file suffix, and if both match it silently retries with load_dataset and synthesizes a fake history line. This is exactly the kind of bare-string-sniff fallback that masks the real failure.
  • Failure mode: The branch is coupled to the exact wording of a NumPy/indexing error. If the upstream message changes (NumPy version bump, refactor of pop_importbids), the 'only integers' not in str(exc) check flips and the IndexError re-raises as an opaque crash; or an unrelated IndexError containing that phrase gets silently swallowed and the BIDS sidecars are skipped w…
  • Fix: Detect the unsupported-.set case explicitly before/inside pop_importbids (e.g. a typed exception or an up-front capability check) rather than catching IndexError and matching message text.
  • Evidence: bids.py L300-302 except IndexError as exc: if "only integers" not in str(exc) or eeg_file.suffix.lower() != ".set": raise.

32. bids validate can never report status:error or can_continue:False (dead branch / misleading contract)

• cli/commands/bids.py (lines 91-107)
  • Why: errors is initialized to [] (L91) and nothing is ever appended; only warnings are populated (L93-96). So status = "error" if errors else ... and "can_continue": not errors are statically "warning"/"ok" and True forever.
  • Failure mode: The schema_version eegprep.bids.validate.v1 advertises an error/can_continue contract that the implementation cannot produce. A downstream agent that branches on can_continue == False to abort will never abort even on a genuinely unusable BIDS root, and a reader cannot tell whether error reporting was intended-but-unfinished or deliberately omitted.
  • Fix: Either populate errors for hard failures (e.g. missing required BIDS structure) or drop the unreachable errors/status==error/can_continue machinery so the contract matches reality.
  • Evidence: bids.py L91 errors: list[dict[str, str]] = [] then L97 status = "error" if errors else ... with no intervening errors.append.

33. pipeline run declares --quiet/--verbose/--no-progress flags that are never read

• cli/commands/pipeline.py (lines 178-204)
  • Why: register adds --quiet, --verbose, --no-progress to the pipeline run parser (L184-186), but handle_registered (L191-204) only forwards config/dry_run/manifest/overwrite to run_pipeline_config, and run_pipeline_config does no logging configuration. Unlike transforms.py, pipeline never calls _configure_logging or redirect_stdout(sys.stderr).
  • Failure mode: A researcher running eegprep pipeline run cfg.yaml --json --quiet gets full INFO/DEBUG logging anyway, and any library logging that lands on stdout can interleave with the single JSON line that agents parse from --json. The flags advertise progress control that does not exist.
  • Fix: Either wire these flags into run_pipeline_config (configure logging level + redirect non-JSON chatter to stderr as transforms.py does) or remove the flags from the parser so the contract matches behavior.
  • Evidence: transforms.py L95 with redirect_stdout(sys.stderr): return _run_transform_command(args) and L697 _configure_logging; pipeline.py has neither, yet L184-186 add --quiet/--verbose/--no-progress.

34. Filter step implemented twice with divergent validation (negative-edge guard missing in pipeline)

• cli/commands/transforms.py (lines 236-293, 511-538 (pipeline.py))
  • Why: transforms.py:_filter and pipeline.py:_apply_filter both translate the same {highpass, lowpass, notch, notch_width} params into the same pair of pop_eegfiltnew calls. They have already drifted: transforms.py guards the notch lower edge (if lower_edge <= 0: raise CliTransformError(... '--notch minus half --notch-width must be positive.'), lines 264-265) while pipeline.py passes `locutoff=fl…
  • Failure mode: A pipeline YAML with e.g. notch: 1, notch_width: 4 yields locutoff=-1 handed to pop_eegfiltnew; the CLI filter subcommand rejects the identical request. Same input, two answers, depending on which entry point the researcher used. Any future fix to one branch silently leaves the other wrong.
  • Fix: Extract one shared step-applier (e.g. apply_filter(EEG, *, highpass, lowpass, notch, notch_width, order=None, ...) -> (EEG, history)) in a single module and have both the transforms subcommand and the pipeline step call it, so validat…
  • Evidence: transforms.py L264-265: if lower_edge <= 0: raises; pipeline.py L530: locutoff=float(notch) - width / 2, with no positivity check.

35. command_error drops EEGPrepCLIError.exit_code, decoupling the structured-result path from exception exit codes

• cli/core.py (lines 97-115, 44-57)
  • Why: Commands that return structured results (pipeline/qc/report) wrap failures via command_error(command, exc), which copies code/message/path/suggestion/details but NOT exc.exit_code. The returned dict also has no exit_code key, so main.py's int(result.get("exit_code", 1) or 1) always yields 1 for these. By contrast EEGPrepCLIError raised through the exception path (and bids/batch's hand-roll…
  • Failure mode: Today it is latent because pipeline/qc/report happen to raise only default exit_code=1 errors. But the abstraction is unsound: the moment any of these commands raises a CONFIG_SCHEMA_ERROR with exit_code=2 (as the argparse path and bids already do for usage errors), the structured path will silently downgrade it to exit 1, giving agents an inconsistent e…
  • Fix: Carry error.exit_code into the command_error payload and have emit_command_result/main.py honor it, so structured-result and exception paths produce the same exit code for the same error.
  • Evidence: core.py command_error L97-115 builds payload from code/message/path/suggestion/details only; EEGPrepCLIError defines exit_code (L34) used by main.py L69 but never read by command_error.

36. command_schema lists required params that are absent from its own properties block

• cli/discovery.py (lines 298-310, 99-104)
  • Why: _command_schema is called with required=["input","freq","output"] (resample), required=["input","event_type","tmin","tmax","output"] (epoch), etc., but the properties dict it returns only describes input/output/manifest/overwrite/json (L302-309). So eegprep schema command epoch --json reports required: [input, event_type, tmin, tmax, output] while properties has no `event_type/tmin…
  • Failure mode: This schema is the agent-facing contract (the epilog tells agents to trust schema command <name> over flag docs). An agent that introspects the schema to construct a call cannot learn the type or even existence of the required freq/event_type/tmin/tmax params, and a strict JSON-Schema consumer sees required keys with no property definitions — an in…
  • Fix: Make _command_schema accept and emit the real per-command properties (or build the schema from the argparse parser), so required and properties stay consistent.
  • Evidence: discovery.py L302-309 properties = only input/output/manifest/overwrite/json; L103 required=["input", "event_type", "tmin", "tmax", "output"].

37. Self-describing schema marks --output required, but transforms allow omitting it with --overwrite

• cli/discovery.py (lines 32-37, 99-104, 287-295)
  • Why: _write_capability sets requires_output: True (L294) and command_schema lists output in required for resample/rereference/filter/clean/epoch/ica. But transforms.py _resolve_output_path (L552-561) explicitly permits omitting --output when --overwrite is passed (returns the input path), raising only when neither is given.
  • Failure mode: The machine-readable contract overstates the requirement. An agent or wrapper that validates against the schema will reject the legitimate in-place --overwrite workflow, or conversely will not learn that --overwrite is the alternative to --output. Contract drift between the discovery layer and the executor.
  • Fix: Express the real rule (one of --output/--overwrite required) in the schema, e.g. via anyOf/a documented note, instead of an unconditional output requirement.
  • Evidence: discovery.py L99 required=["input", "freq", "output"]; transforms.py L555-556 if args.overwrite: return input_path.

38. EEGobj.getattr returns a callable for any unknown name, swallowing field-name typos

• functions/eegobj/eegobj.py (lines 115-130)
  • Why: getattr returns eeg[name] only when name is a key present in the EEG dict; for every other attribute it unconditionally returns a wrapper closure that will try to resolve name as an eegprep function. There is no distinction between 'a function I can dispatch' and 'a misspelled field'. The AttributeError only fires later, inside _call_eegprep, and only if the wrapper is actually CALLED.
  • Failure mode: Accessing a field that is unset/misspelled (e.g. obj.icawnv instead of obj.icaweights, or obj.srate on a dict that lacks it) returns a function object instead of raising AttributeError. Code like if obj.icaweights is not None: is always truthy (a function is truthy), and obj.some_typo silently yields a callable, so a researcher's typo passes a wrong obje…
  • Fix: In getattr, only return a dispatch wrapper when name resolves to a known eegprep function (reuse _resolve); otherwise raise AttributeError. Or restrict bare-attribute access to declared EEG fields and require explicit method calls.
  • Evidence: Lines 124-130: if isinstance(eeg, dict) and name in eeg: return eeg[name] then unconditionally def wrapper(...): ...; return wrapper for any other name.

---

Generated by automated review; verify each finding from first principles before fixing.

[claude]

🤖 Plan:

• Read epic [Epic] Fable 5 Findings — Whole-Codebase Code-Quality Review #193 and phase issue body
• Duplicate-work preflight (no open PRs reference Phase 3 / epic [Epic] Fable 5 Findings — Whole-Codebase Code-Quality Review #193)
• Spot-verify a sample of findings against working tree
• Propose batching strategy and await maintainer direction before opening any PR

Scope assessment

This is a tracking epic with 38 distinct findings spanning 22+ modules across functions/popfunc, functions/guifunc, functions/adminfunc, functions/sigprocfunc, functions/studyfunc, functions/timefreqfunc, plugins/clean_rawdata, plugins/ICLabel, plugins/EEG_BIDS, cli/, and functions/eegobj. Fixing them as one PR would cross the 100-line threshold many times over and bundle unrelated semantic changes (history-command contract, off-by-one numeric math, CLI schema, GUI/console atomicity, ASR fallback policy) into one squash commit. Per the fix-issue scope rule I am stopping here and proposing how to batch instead of opening a single mega-PR.

Spot-check (sample of 4)

I confirmed against HEAD that these are real and the line numbers are accurate:

• src/eegprep/functions/popfunc/pop_select.py#L207,L212 — inds = eeg_decodechan(EEG, g['chantype'], 'type', True) does not unpack the (chaninds, chanlist_out) tuple; the next line np.array(inds, dtype=int) will raise on the string labels (finding EDF testing and MATLAB modification #7). Sibling label paths at L184,L198 unpack correctly.
• src/eegprep/plugins/clean_rawdata/clean_flatlines.py#L70-L71 — if CCM := EEG['etc'].get('clean_channel_mask') is not None: parses as CCM := (… is not None), binding a bool; CCM[CCM] = ~removed_channels then raises TypeError: 'bool' object is not subscriptable (finding Creating documentation and update eegprep functions #30).
• src/eegprep/functions/popfunc/pop_loadset_h5.py#L33-L37 — hardcoded hello 👖 literal-match still present in the runtime decoder (finding coords not defined #6).
• src/eegprep/plugins/ICLabel/ICL_feature_extractor.py#L32 vs L35-L36 — ncomp = EEG['icawinv'].shape[1] precedes the guard intended to catch a missing/empty icawinv (finding Map release process based on EEGDash #24).

I did not re-verify every line in the remaining 34 findings; the issue body notes the reviewer pass was read-only and asks fixers to validate from first principles. I will do that per-finding when each lands.

Proposed batching

I recommend opening one PR per logical theme below, each scoped to <300 lines and with its own regression test. Numbers in parentheses are finding ids from the issue body.

A. pop_select correctness and history contract — chantype unpack crash + numeric channel 0/1-based history desync (7, 8). One PR; both touch pop_select.py and share tests in tests/test_pop_select.py / tests/test_console_workspace.py.

B. Silent .set load fallbacks — pop_fileio → pop_loadset → pop_loadset_h5 chain plus the hardcoded emoji branch (4, 5, 6). One PR replacing except Exception with sniffed-format dispatch and removing the test fixture from the decoder.

C. Numeric/index correctness in sigprocfunc — eegrej boundary-shift, epoch off-by-one, eeg_eegrej type-agnostic edge drop (2, 15, 16). One PR; all three need MATLAB parity tests under tests/matlab/ and align with EEGLAB oracle.

D. ICA/ICLabel guards and AMICA temp cleanup — ICL feature guard ordering, runamica leak, headplot session commit (13, 17, 24). One PR; small isolated changes per module.

E. ASR / clean_rawdata broad-except cleanup — asr_calibrate srate, asr_process per-window, clean_artifacts channel select, clean_asr calibration, clean_artifacts re-insertion contract (25, 26, 27, 28, 29). One PR; consistent narrowing of except Exception to specific numerical/ImportError cases, plus an explicit NotImplementedError for the Channels_ignore re-insertion until implemented.

F. STUDY / time-frequency dead/inverted options — std_makedesign delfiles, std_precomp recompute, compute_pac title/vert/newfig, newtimef _is_on inversion, newtimef overlap/plotphase (18, 19, 20, 21, 22). One PR; either honor the option or raise NotImplementedError consistently.

G. CLI contract drift — pipeline duplicated filter validation, --quiet/--verbose/--no-progress dead flags, bids IndexError sniff, bids validate dead error branch, command_error exit_code, command_schema required vs properties, --output vs --overwrite (31, 32, 33, 34, 35, 36, 37). One PR; all in cli/, share testing surface.

H. GUI/console atomicity — eegprep logger mutation in QThread, IMPLEMENTED_ACTIONS drift guard, _commit_processed_dataset_from_gui transaction, CURRENTSET reassignment store (10, 11, 12, 14). One PR; touches functions/guifunc/ and functions/adminfunc/console.py and needs tests/test_console_workspace.py coverage per AGENTS.md.

I. EEGobj typo trap + small popfunc cleanups — __getattr__ wrapper for unknown names, eeg_compare contract, pop_editeventvals epoched display unit, pop_spectopo dropped plotchan/icamode, BIDS gen_derived_fpath placeholder (1, 3, 9, 23, 38). One PR each is probably overkill; group as a single small-cleanups PR or split if any one grows.

That is 9 PRs covering 38 findings, ordered roughly by blast radius (A–C user-visible numerics first, D–F option contracts, G CLI, H GUI atomicity, I residual cleanup). Each is independently revertable and CI-greenable.

Asks before I start

1. Confirm 9-PR batching is acceptable, or specify a different grouping.
2. Confirm I should pick up batch A first (pop_select is the most user-visible and already has a chantype crash on a documented code path).
3. Confirm whether MATLAB parity tests for batch C should be added under tests/matlab/ (preferred per AGENTS.md) or guarded behind EEGPREP_SKIP_MATLAB.

I will not open any PR against #196 until directed.

[suraj-ranganath]

🤖 Closing because Phase 3 was completed by merged PR #203 and is included in epic PR #206.