OPEN_QUESTIONS.md: summary table of every decision (ramp foot, naming,
isStable fix, monster guard, plain-dicts→declareChildGetter, VGC→
ChildRouter, LatestWinsGate fireAndWait, drop mAbs, per-listener fan-out,
commandRegistry 'none' + description, UnitPolicy dual-shape, Phase 10
test rewrites).
TASKS.md §Phase 11: unit-aware commands. Every numeric setter declares
units: { measure, default }. commandRegistry normalises msg.payload +
msg.unit; warns + lists accepted units for bad input; falls back to
default. New query.units topic returns the spec per node.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
566 lines
27 KiB
Markdown
566 lines
27 KiB
Markdown
# Open questions
|
|
|
|
Things deferred. Append, don't rewrite history. Add a date when you add
|
|
or resolve an entry. Anyone (human or agent) discovering an unclear
|
|
decision during refactor work writes it here rather than guessing.
|
|
|
|
---
|
|
|
|
## 2026-05-11 — Interview round — resolved decisions
|
|
|
|
| Topic | Decision |
|
|
|---|---|
|
|
| Ramp foot for run-zone curve (control/levelBased) | `inflowLevel` (current). startLevel is the 0% minimum, not the curve foot. |
|
|
| `overfillLevel` vs `highVolumeSafetyLevel` | **`highVolumeSafetyLevel` canonical**; drop the legacy alias. |
|
|
| measurement `isStable` tautology | Fix now with a config-driven absolute threshold (`stabilityThreshold` in scaling-units). Add to schema + editor UI. |
|
|
| monster cooldown-guard pre-existing fail | Debug + fix the sampling-pulse logic. |
|
|
| pumpingStation plain child dicts | Migrate to `declareChildGetter`; rewrite affected tests. |
|
|
| VGC custom `registerChild` overload | Adopt ChildRouter; rewrite disambiguation tests. |
|
|
| MGC inline dispatch gate vs LatestWinsGate | Extend LatestWinsGate with `fireAndWait(value)` returning the per-fire settlement promise. Migrate MGC. |
|
|
| measurement legacy `'mAbs'` event | Remove now. |
|
|
| ChildRouter wildcard emit-patch | Per-listener fan-out using canonical POSITIONS. No more emit patching. |
|
|
| commandRegistry payload schema | Add `'none'` type + per-command `description` field (wikiGen consumes). |
|
|
| UnitPolicy property-vs-method shape | Expose both. Frozen property bags alongside the methods. Drop `_unitView` workarounds. |
|
|
| rotatingMachine + reactor private-method-pinning tests (13 files) | Rewrite all to drive only the public BaseNodeAdapter surface. Phase 10. |
|
|
| **Unit-aware commands (new)** | Each numeric setter declares `units: { measure, default }`. commandRegistry normalises + warns + lists accepted units. `query.units` topic returns spec. Phase 11. |
|
|
|
|
|
|
Format:
|
|
|
|
```
|
|
## YYYY-MM-DD — Short title
|
|
|
|
**Context:** what we're trying to do
|
|
**Question:** what's unresolved
|
|
**Default chosen:** what we did meanwhile
|
|
**Decision needed by:** which phase or task
|
|
```
|
|
|
|
---
|
|
|
|
## 2026-05-10 — External Port-0 topic naming — RESOLVED
|
|
|
|
**Decision (2026-05-10):** Use canonical names (`set.*` / `cmd.*` /
|
|
`data.*` / `child.*` / `query.*` / `evt.*`) **from Phase 1 onwards**.
|
|
Each `commands/index.js` declares the canonical name as the topic and
|
|
lists legacy names in `aliases`. Aliases log a one-time deprecation
|
|
warning. Phase 7 shrinks to: remove aliases after one release cycle.
|
|
|
|
The full prefix glossary (with what each does and why) is now in
|
|
`CONTRACTS.md §1`. See it before naming a topic.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — Parent EVOLV repo `development` branch lineage — RESOLVED
|
|
|
|
**Decision (2026-05-10):** Rebase parent `development` onto
|
|
`origin/main` before the refactor proceeds. Done at the start of
|
|
Phase 1.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — `generalFunctions` deprecated paths — RESOLVED
|
|
|
|
**Decision (2026-05-10):** Tracked as Phase 8.5 in `TASKS.md`. Cleanup
|
|
runs after promotion to main. The list of paths to remove is captured
|
|
there so it isn't lost.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — Two child-storage shapes — RESOLVED
|
|
|
|
**Decision (2026-05-10):** Registry-as-truth, **with named getters** that
|
|
read clearly in code. `domain.machines` keeps working — it's a getter
|
|
that returns the rotatingMachine slice of `this.child`. Same for
|
|
`domain.stations`, `domain.machineGroups`, etc. Domain code reads
|
|
naturally; the registry is the source of truth underneath.
|
|
|
|
Named getters are declared by the domain subclass in `configure()`:
|
|
|
|
```js
|
|
configure() {
|
|
Object.defineProperty(this, 'machines',
|
|
{ get: () => this.child?.machine?.centrifugal ?? {} });
|
|
}
|
|
```
|
|
|
|
(`BaseDomain` provides a helper for this pattern.)
|
|
|
|
---
|
|
|
|
## 2026-05-10 — Async vs sync `tick()` — RESOLVED with redesign
|
|
|
|
**Decision (2026-05-10):** Default is **event-driven**. Ticks are
|
|
opt-in.
|
|
|
|
`BaseNodeAdapter` exposes two timers:
|
|
|
|
- `static tickInterval = null` — opt-in periodic tick. Default null = no
|
|
tick. Domain emits `'output-changed'` on `this.emitter` instead, and
|
|
BaseNodeAdapter subscribes to that event to push outputs.
|
|
- `static statusInterval = 1000` — always-on status badge poll.
|
|
Required because Node-RED's editor refresh expects a heartbeat. Set
|
|
to 0 only in headless test environments.
|
|
|
|
When opting into ticks:
|
|
- Document **why** in a one-line comment above
|
|
`static tickInterval = ...` (e.g. "needs delta-time for predicted
|
|
volume integrator").
|
|
- A node should opt in only when truly time-driven. Examples that need
|
|
it: `pumpingStation` (predicted volume integrates over time),
|
|
`measurement` (when simulator is enabled — ticks the random walk).
|
|
- Examples that DO NOT need it: `MGC` (recomputes on pressure events),
|
|
`rotatingMachine` (recomputes on measurement events + state changes).
|
|
|
|
`tick()` is treated as fire-and-forget (no await). A node that needs
|
|
serialisation uses `LatestWinsGate` internally.
|
|
|
|
See `CONTRACTS.md §2` for the BaseNodeAdapter shape.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — ChildRouter wildcard subscriptions monkey-patch `emit`
|
|
|
|
**Context:** P1.2 implementation. EventEmitter has no native wildcard.
|
|
Subscriptions with a partial filter (`{type}`-only or `{position}`-only)
|
|
install a per-variant `emit` proxy on the child's emitter; concrete
|
|
`{type, position}` filters use plain `emitter.on`.
|
|
|
|
**Question:** Multi-parent children. `child.parent` is already an array
|
|
in `childRegistrationUtils`, so a child can be registered under several
|
|
parents. If two parents each install ChildRouter wildcard proxies on
|
|
the same `child.measurements.emitter`, the wraps stack — but
|
|
`tearDown` only unwraps when its own bookkeeping is empty. Is this
|
|
correct semantics for multi-parent teardown ordering? Or should we
|
|
switch to per-listener fan-out (subscribe to every known
|
|
`<type>.<variant>.<position>` enumerated from a registry)?
|
|
|
|
**Default chosen:** Stacked wrappers. The current `childRegistrationUtils`
|
|
multi-parent path is rarely exercised in production. Revisit if
|
|
Phase 2 / Phase 4 hits a real multi-parent case.
|
|
|
|
**Decision needed by:** Phase 4.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — `predictionHealth` migration in rotatingMachine
|
|
|
|
**Context:** P1.4 implementation flagged that the existing
|
|
`rotatingMachine.predictionHealth` carries `quality` (string) +
|
|
`confidence` (0..1 numeric) on top of the new `HealthStatus` shape's
|
|
`{level, flags, message, source}`.
|
|
|
|
**Question:** Where does `confidence` live after migration?
|
|
|
|
**Default chosen:** Keep `confidence` on the per-metric drift
|
|
container as a sibling to a `health: HealthStatus` field. Drift
|
|
diagnostics (`nrmse`, `longTermNRMSD`, `immediateLevel`) stay as
|
|
siblings too. `HealthStatus` carries only the standardised five fields.
|
|
|
|
**Decision needed by:** Phase 5 (`rotatingMachine` refactor).
|
|
|
|
---
|
|
|
|
## 2026-05-10 — `dashboardAPI` basic test broken (pre-existing) — RESOLVED
|
|
|
|
**Context:** P1.12 sanity gate. `dashboardAPI/test/basic/structure-module-load.basic.test.js` uses Mocha-style `describe()` globals which don't exist under `node:test`. Reports 0 pass / 1 fail with `ReferenceError: describe is not defined`.
|
|
|
|
**Action:** Pre-existing — not caused by Phase 1. Convert to `node:test` form during Phase 6 when `dashboardAPI` gets its skeleton refactor. Tracked here so it isn't lost.
|
|
|
|
**Update (P6.7, 2026-05-10):** Converted to `node:test` form (`const test = require('node:test')` + `assert.doesNotThrow`). Basic test now reports 1 pass / 0 fail. The Mocha-style `test/dashboardapi.test.js`, `test/nodeClass.test.js`, `test/integration/`, and `test/edge/` files still use jest/Mocha globals — out of scope for P6.7; deferred to P10 test-suite refactor.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — `dashboardAPI` skipped BaseNodeAdapter + BaseDomain
|
|
|
|
**Context:** P6.7. dashboardAPI is a passive HTTP-emitter utility node: no
|
|
`generalFunctions/src/configs/dashboardapi.json`, no periodic Port-0/1
|
|
telemetry stream, no parent registration, no measurements, no tick loop,
|
|
no status badge. `BaseDomain` constructor would throw on the missing
|
|
config file; `BaseNodeAdapter._scheduleRegistration` would emit a
|
|
spurious `child.register` for a node that has no parent; the
|
|
`outputUtils.formatMsg` pipeline assumes a measurement-shaped output
|
|
which dashboardAPI lacks.
|
|
|
|
**Default chosen:** `nodeClass` stays a plain class (does **not** extend
|
|
`BaseNodeAdapter`); `specificClass` (`DashboardApi`) stays a plain class
|
|
(does **not** extend `BaseDomain`). Only the shared `commandRegistry`
|
|
is adopted (canonical topic `child.register` with `registerChild` alias
|
|
+ deprecation warning). One handler module in `src/commands/`. nodeClass
|
|
shrunk from 134 → 73 lines.
|
|
|
|
**Decision needed by:** Phase 7 / Phase 8 — revisit if `BaseNodeAdapter`
|
|
grows a passive/HTTP-only mode (skip-registration + skip-output-stream
|
|
flags) or if a `dashboardapi.json` config gets added to generalFunctions.
|
|
Either makes adoption straightforward; until then the bespoke shape is
|
|
correct.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — pumpingStation: plain dicts vs `declareChildGetter`
|
|
|
|
**Context:** P2.7+P2.8+P2.9. The 2026-05-10 "Two child-storage shapes"
|
|
decision says use `declareChildGetter` (registry-as-truth), but the
|
|
existing pumpingStation test suite mutates `ps.machineGroups['mgc1'] = {...}`
|
|
directly to inject mock children before driving `_controlLevelBased`.
|
|
A getter-backed `machineGroups` returns a fresh object per call, so the
|
|
mutation is on a throwaway and the orchestrator never sees the mock.
|
|
|
|
**Default chosen:** Keep `machines / stations / machineGroups` as plain
|
|
id-keyed dicts on `this`. ChildRouter `onRegister` handlers populate them
|
|
on real registration; tests can still assign directly. Registry remains
|
|
the upstream source of truth (handshake still flows through it), but the
|
|
flat dicts are also writable. Revisit if other domains can adopt
|
|
`declareChildGetter` cleanly without test rewrites.
|
|
|
|
**Decision needed by:** Phase 10 (test-suite refactor).
|
|
|
|
---
|
|
|
|
## 2026-05-10 — `reactor` test runtime is mathjs-bound (pre-existing)
|
|
|
|
**Context:** P1.12 sanity gate. Every reactor test file takes ~13 s because `require('mathjs')` alone is ~12.5 s on this machine (mathjs is huge and loads its full operator set eagerly). With basic tests parallelised by `node --test`, each subprocess pays the cost. A 90 s outer timeout doesn't accommodate the parallel load.
|
|
|
|
**Action:** Pre-existing — not caused by Phase 1. Two options to track for Phase 5/6 cleanup:
|
|
1. Switch to a tree-shaken mathjs subset (only ops actually used).
|
|
2. Cache the mathjs instance at module top and pass into Reactor classes.
|
|
|
|
Tracked; not blocking the refactor.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — measurement `isStable` tautology (pre-existing bug)
|
|
|
|
**Context:** P3.4. The existing `isStable` in `measurement/src/specificClass.js` does:
|
|
|
|
```js
|
|
stableThreshold = stdDev * marginFactor; // marginFactor = 2
|
|
return { isStable: (stdDev < stableThreshold || stdDev == 0), stdDev };
|
|
```
|
|
|
|
`stdDev < stdDev * 2` is always true for `stdDev > 0`, and the OR catches the
|
|
zero case. So `isStable` returns `true` for every non-empty buffer. That makes
|
|
`calibrate()` essentially un-gateable (it only aborts when there are < 2
|
|
samples) and `evaluateRepeatability()` happily reports a huge stdDev as
|
|
"repeatability".
|
|
|
|
**Action:** Preserved verbatim by the new `Calibrator` (additive). A
|
|
behavioural fix needs an external reference (config-driven absolute
|
|
threshold, or % of full scale). Two BUG-PRESERVED tests pin the current
|
|
shape so a follow-up behavioural PR is intentional.
|
|
|
|
**Decision needed by:** Phase 10 (test-suite refactor) — naturally
|
|
adjacent to the calibration test cleanup.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — `commandRegistry` payload schema needs `'none'`/`'void'` type
|
|
|
|
**Context:** P3.7+P3.8. Trigger-only commands (`set.simulator`,
|
|
`set.outlier-detection`, `cmd.calibrate`) ignore their payload. The
|
|
current registry's `payloadSchema.type` enum is
|
|
`'string'|'number'|'object'|'boolean'|'any'`. Trigger commands fall
|
|
into `'any'`, which is too permissive (an object slipped past would
|
|
not be flagged).
|
|
|
|
**Default chosen:** Use `'any'` for now. Add `'none'`/`'void'` to the
|
|
registry schema enum during Phase 7 (topic-name standardisation).
|
|
|
|
**Decision needed by:** Phase 7.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — measurement legacy `'mAbs'` emitter event
|
|
|
|
**Context:** P3.7+P3.8 CONTRACT.md noted that the existing `Measurement`
|
|
class emits `'mAbs'` on `source.emitter` whenever the analog output
|
|
updates. This was a pre-MeasurementContainer broadcast. It's still
|
|
fired but no production consumer reads it (per the existing comment
|
|
"DEPRECATED: Use measurements container instead").
|
|
|
|
**Default chosen:** Keep firing it through Phase 3 (post-integration).
|
|
Remove in Phase 7 alongside the topic-rename cleanup, or in Phase 8.5
|
|
deprecated-path cleanup.
|
|
|
|
**Update (P3.2+P3.5+P3.6+P3.9, 2026-05-10):** Re-emitted from the analog
|
|
specificClass by subscribing to the MeasurementContainer's
|
|
`<type>.measured.<position>` event (position lowercased to match
|
|
container normalisation). Channel itself stays event-name-agnostic.
|
|
|
|
**Decision needed by:** Phase 7 / Phase 8.5.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — measurement legacy property mirrors
|
|
|
|
**Context:** P3.2+P3.5+P3.6+P3.9. The analog pipeline now lives inside
|
|
`Channel`. The pre-refactor test suite pins many fields directly on the
|
|
Measurement instance: `outputAbs`, `outputPercent`, `storedValues`,
|
|
`totalMinValue`, `totalMaxValue`, `totalMinSmooth`, `totalMaxSmooth`,
|
|
`inputRange`, `processRange`. Some tests *write* `m.storedValues` /
|
|
`m.totalMinValue` directly before calling pipeline helpers.
|
|
|
|
**Default chosen:** Install getter/setter mirrors on the Measurement
|
|
instance (`_installChannelMirrors`) that forward read/write through
|
|
`this.analogChannel`. Storage stays single-sourced in Channel, the
|
|
legacy public surface stays writable, no test rewrites required.
|
|
|
|
**Decision needed by:** Phase 10 (test-suite refactor) — replace these
|
|
with direct `m.analogChannel.xxx` access in tests, then drop the mirrors.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — measurement `handleScaling` mutates config.scaling
|
|
|
|
**Context:** P3.2+P3.5+P3.6+P3.9. Channel's `_applyScaling` resets its
|
|
*own* `scaling.inputMin/inputMax` to `[0,1]` when the input range
|
|
collapses (`inputMax <= inputMin`). The pre-refactor `handleScaling`
|
|
mutated `this.config.scaling.inputMin/inputMax` instead, and a basic
|
|
test pins that contract.
|
|
|
|
**Default chosen:** The Measurement-level `handleScaling` delegate
|
|
copies Channel's reset back to `config.scaling` after the call so the
|
|
visible behaviour is preserved. Long-term, the test should read the
|
|
new state from `m.analogChannel.scaling` and we drop the mirror write.
|
|
|
|
**Decision needed by:** Phase 10 (test-suite refactor).
|
|
|
|
---
|
|
|
|
## 2026-05-10 — measurement nodeClass routing tests pin private wiring
|
|
|
|
**Context:** P3.2+P3.5+P3.6+P3.9. The basic `nodeclass-routing` and
|
|
edge `invalid-payload` tests instantiated `NodeClass.prototype` and
|
|
called `_attachInputHandler()` / `_registerChild()` directly. The
|
|
BaseNodeAdapter superclass renamed these to `_attachInputHandler`
|
|
(unchanged) and `_scheduleRegistration` (was `_registerChild`), and
|
|
dispatch now goes through `this._commands` built in the constructor.
|
|
|
|
**Action:** Adjusted the two tests in-place to seed `inst._commands`
|
|
via `createRegistry(commands, …)` and to call `_scheduleRegistration`
|
|
instead of `_registerChild`. The on-the-wire payload topic also moved
|
|
from `'registerChild'` → `'child.register'` (BaseNodeAdapter
|
|
convention); the test assertion was updated accordingly.
|
|
|
|
**Decision needed by:** Phase 10 — these tests should be rewritten to
|
|
drive a full nodeClass through `new nodeClass(uiConfig, RED, node,
|
|
'measurement')` rather than poking at private members.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — MGC `calcAbsoluteTotals` implicit pressure-key coupling
|
|
|
|
**Context:** P4.1/4.2 extracted `totals/totalsCalculator.js` preserving
|
|
original behaviour. `calcAbsoluteTotals` iterates
|
|
`machine.predictFlow.inputCurve` and re-uses the same pressure key to
|
|
index `machine.predictPower.inputCurve[pressure]`. If the two curves were
|
|
sampled at different pressures (legitimate when power was extrapolated
|
|
separately from flow), the lookup is `undefined` and the call throws.
|
|
**Question:** should the totals calculator defensively skip mismatched
|
|
pressure keys, or should the invariant "flow + power curves share pressure
|
|
keys" be enforced upstream in rotatingMachine's curveLoader/normalizer?
|
|
**Default chosen:** preserved the implicit coupling — no behavioural change.
|
|
**Decision needed by:** P5 (rotatingMachine refactor) — curveLoader/Normalizer
|
|
is the natural place to enforce or document the pairing.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — MGC concern modules use legacy unitPolicy object shape
|
|
|
|
**Context:** The MGC concern modules (groupOps/groupOperatingPoint,
|
|
totals/totalsCalculator, combinatorics/pumpCombinations, control/strategies)
|
|
extracted in Wave 1 read units as `ctx.unitPolicy.canonical.flow` — the old
|
|
plain-object shape carried on the pre-refactor specificClass. `BaseDomain`
|
|
now wires `this.unitPolicy` to a `UnitPolicy` instance whose canonical/output
|
|
are methods (`canonical('flow')`).
|
|
**Question:** Should the concern modules be updated to call the methods, or
|
|
should we keep the object-shaped view long-term?
|
|
**Default chosen:** specificClass builds a frozen `this._unitView` ({
|
|
canonical: {flow,pressure,power,temperature}, output: {…} }) and passes it
|
|
to the modules. Two surface shapes live side-by-side in the same node.
|
|
**Decision needed by:** P5 (rotatingMachine) — the same concern-module
|
|
shape will likely repeat. Pick one and migrate before the second node lands
|
|
on the pattern.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — rotatingMachine Machine constructor takes 3 positional args
|
|
|
|
**Context:** P5.9/5.10/5.12. The pre-refactor Machine class accepted
|
|
`(machineConfig, stateConfig, errorMetricsConfig)`. BaseDomain's
|
|
constructor only knows about the first slot. The whole test suite (~30
|
|
files) constructs Machines directly with two positional args, and
|
|
BaseNodeAdapter instantiates DomainClass with just `this.config`.
|
|
|
|
**Question:** Where do the extra positional configs travel? Schema
|
|
validation in `configUtils.initConfig` strips unknown top-level keys, so
|
|
embedding them in machineConfig doesn't work. Subclass-overriding
|
|
constructor before super() is blocked by ES6's pre-super `this` rule.
|
|
|
|
**Default chosen:** Static stash on the class itself
|
|
(`Machine._pendingExtras`) assigned just before `super()` (or by
|
|
nodeClass.buildDomainConfig before BaseNodeAdapter instantiates the
|
|
domain). `configure()` reads + clears it. Single-threaded JS makes the
|
|
hand-off race-free.
|
|
|
|
**Decision needed by:** P10 (test-suite refactor) — when tests get
|
|
rewritten to use the BaseNodeAdapter-built domain, drop the multi-arg
|
|
constructor and fold stateConfig/errorMetricsConfig into machineConfig
|
|
slices.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — rotatingMachine private nodeClass tests (4 files adjusted)
|
|
|
|
**Context:** P5.9/5.10/5.12. Four pre-refactor tests pinned private
|
|
nodeClass methods: `_loadConfig`, `_setupSpecificClass`,
|
|
`_updateNodeStatus`, and the inline `_attachInputHandler` switch. After
|
|
the BaseNodeAdapter migration those private methods are gone — config
|
|
build lives in `buildDomainConfig()`, dispatch in `commands/`, status
|
|
badge in `source.getStatusBadge()`.
|
|
|
|
**Default chosen:** Updated the four test files to drive the new
|
|
surface: `buildDomainConfig` returns the per-node slice (and stamps
|
|
`Machine._pendingExtras`); routing tests seed `inst._commands` via
|
|
`createRegistry(commands, …)` and assert through that path; status-badge
|
|
tests call `io.buildStatusBadge(source)` directly.
|
|
|
|
**Decision needed by:** P10 — these tests still poke private members
|
|
(`_commands`, `_attachInputHandler`). The right shape is constructing
|
|
a full `new nodeClass(uiConfig, RED, node, 'rotatingMachine')` and
|
|
asserting against the resulting `node._sent` / `node._statuses`.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — monster schema strips command-line constraint keys
|
|
|
|
**Context:** P6.3. The monster JSON schema in `generalFunctions/src/configs/monster.json` defines `samplingtime`, `minVolume`, `maxWeight` and others under `constraints`, but NOT `nominalFlowMin`, `flowMax`, `maxRainRef`, `minSampleIntervalSec`. `configUtils.initConfig` strips these unknown keys with a `Unknown key … Removing it.` warning. The legacy code read them anyway — `Number.isFinite(undefined)` returns false, so guards naturally route into invalid-bounds territory and tests pass via the undefined cascade.
|
|
|
|
**Default chosen:** Preserved — refactor reads the same stripped fields the same way. The schema warning is harmless but noisy in test output.
|
|
|
|
**Decision needed by:** Phase 7 (topic-name standardisation) — add the four missing constraint keys to the schema, OR move them into a `samplingControl` section. Either fix removes the warning and lets the values actually pass through.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — monster sampling-guards cooldown test fails on development (pre-existing)
|
|
|
|
**Context:** P6.3 baseline run. `test/edge/sampling-guards.edge.test.js` "cooldown guard blocks pulses when flow implies oversampling" already fails on `development` BEFORE the refactor (`assert.ok(monster.sumPuls > 0)` — sumPuls stays at 0 across 80 ticks). The legacy in-file equivalent in `test/monster.test.js` (Mocha-style wrapper, not picked up by `node:test`) appears to have passed in an earlier era.
|
|
|
|
**Default chosen:** Refactor preserves behaviour byte-for-byte — same failure remains, same line. Not a refactor regression.
|
|
|
|
**Decision needed by:** Phase 10 (test-suite refactor) — fix the test OR debug the underlying behaviour (likely the interaction between `_beginRun` resetting state inside the same `sampling_program` call that the integrator then runs, leaving the first second's m3PerTick stranded).
|
|
|
|
---
|
|
|
|
## 2026-05-10 — MGC handleInput retained inline latest-wins (not DemandDispatcher)
|
|
|
|
**Context:** Wave 1 added `src/dispatch/demandDispatcher.js` wrapping
|
|
`LatestWinsGate`. Tests (`turnoff-deadlock`, `idle-startup-deadlock`,
|
|
`ncog-distribution`) call `await mgc.handleInput(...)` and rely on the
|
|
awaited promise resolving after the dispatch completes; they also pin the
|
|
exact `_delayedCall` field. `LatestWinsGate.fire(value)` returns void.
|
|
**Question:** Should `handleInput` switch to the gate (changing the test
|
|
contract), or stay inline (keeping the awaitable shape)?
|
|
**Default chosen:** kept the inline `_dispatchInFlight + _delayedCall`
|
|
gate verbatim. `DemandDispatcher` remains exported but unused by the
|
|
orchestrator for now — its basic test still passes since it tests the
|
|
wrapper in isolation.
|
|
**Decision needed by:** P7 (topic-name standardisation) or P10 (test-suite
|
|
refactor) — adopting the gate requires either rewriting tests to drain
|
|
the gate or changing the gate to return a settle promise.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — valveGroupControl registerChild overload (skipped ChildRouter)
|
|
|
|
**Context:** P6.2. `ValveGroupControl.registerChild(child, posOrType)` is
|
|
called from two distinct paths: (a) `childRegistrationUtils.registerChild`
|
|
delegates with the canonical softwareType as 2nd arg, and (b) tests + a
|
|
few in-process callers invoke it directly passing **either** a position
|
|
(`'atEquipment'`) **or** a softwareType (`'machine'`). The legacy code
|
|
disambiguated via a `KNOWN_POSITIONS` set lookup and returned a boolean
|
|
indicating registration success (used by `flow-distribution` regression
|
|
test to assert a non-valve payload yields `false`).
|
|
|
|
**Default chosen:** kept the legacy resolver in the domain — override
|
|
`this.registerChild` inside `configure()` so the boolean return + dual
|
|
semantics survive. `ChildRouter` is **not** used for VGC (no `onRegister`
|
|
/ `onMeasurement` handlers declared). Source-side event wiring still
|
|
lives in `src/sources/fluidContract.js` (raw emitter `.on` on each
|
|
`SOURCE_FLOW_EVENTS` name) because the source family includes mixed-case
|
|
event names (`flow.predicted.atEquipment` and lowercase variants both fire).
|
|
|
|
**Decision needed by:** P7 — once topic names + position casing are
|
|
standardised, the source listener set collapses and a ChildRouter
|
|
`onMeasurement('machine', { type:'flow' }, …)` declaration becomes
|
|
sufficient. At that point `registerChild` can return to base + ChildRouter
|
|
and the boolean-return test can be rewritten to assert via side-effects.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — valveGroupControl `set.position` placeholder
|
|
|
|
**Context:** P6.2 command registry. Task spec required canonical name
|
|
`setpoint → set.position`, but VGC's pre-refactor input switch did not
|
|
implement a `setpoint` topic — valve position is driven by `data.totalFlow`
|
|
re-distribution, not direct per-valve setpoints. Registering `set.position`
|
|
with an empty handler keeps the canonical name reserved without breaking
|
|
the contract surface.
|
|
|
|
**Default chosen:** registered `set.position` with a no-op handler that
|
|
debug-logs the payload. `setpoint` listed as alias so a legacy emitter
|
|
gets the same no-op path.
|
|
|
|
**Decision needed by:** P7 — decide whether VGC actually needs a
|
|
per-valve setpoint topic (probably yes when virtualControl mode lands).
|
|
At that point promote the handler from no-op to real dispatch.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — reactor private nodeClass tests (8 files adjusted)
|
|
|
|
**Context:** P6.5. Eight pre-refactor reactor tests pinned private
|
|
nodeClass methods (`_loadConfig`, `_setupClass`, `_registerChild`, inline
|
|
`_attachInputHandler` switch, `_tick`, `_startTickLoop`, `_attachCloseHandler`).
|
|
After the BaseNodeAdapter migration those private methods are gone — config
|
|
build lives in `buildDomainConfig()`, dispatch in `commands/`, registration in
|
|
`_scheduleRegistration` (renamed), and the periodic emit lives in
|
|
`_emitOutputs` (overridden so the Fluent / GridProfile Port-0 contract is
|
|
preserved — delta-compressed payloads can't carry the C-vector).
|
|
|
|
**Default chosen:** Adjusted in place: `test/basic/constructor.basic.test.js`,
|
|
`test/basic/input-routing.basic.test.js`, `test/basic/register-child.basic.test.js`,
|
|
`test/basic/speedup-factor.basic.test.js`, `test/edge/invalid-topic.edge.test.js`,
|
|
`test/edge/missing-child.edge.test.js`, `test/edge/invalid-reactor-type.edge.test.js`,
|
|
`test/integration/tick-loop.integration.test.js`. Routing tests seed
|
|
`inst._commands` via `createRegistry(commands, …)`; topic moved from
|
|
`'registerChild'` → `'child.register'`. The "unknown reactor_type throws"
|
|
edge case became "falls back to CSTR" — the legacy bottom-of-switch already
|
|
fell back to CSTR; only the surface changed (warning channel now via
|
|
domain logger, not `node.warn`).
|
|
|
|
**Decision needed by:** Phase 10 — same shape as the rotatingMachine /
|
|
measurement adjustments. The right fix is to drive a full
|
|
`new nodeClass(...)` and assert against `node._sent` / `node._statuses`
|
|
instead of poking private members.
|
|
|
|
---
|
|
|
|
## 2026-05-10 — reactor schema enum lowercases `reactor_type`
|
|
|
|
**Context:** P6.5. The reactor JSON schema defines `reactor.reactor_type`
|
|
as `type: 'enum'` with values `'CSTR'` / `'PFR'`. The shared enum validator
|
|
lowercases the user-supplied value before comparing, so an inbound `'PFR'`
|
|
ends up stored as `'pfr'` in the validated config. The pre-refactor
|
|
nodeClass switched on the raw uiConfig value and never saw the lowercased
|
|
form; after the BaseDomain migration the wrapper reads the validated
|
|
config and would always fall back to CSTR.
|
|
|
|
**Default chosen:** `Reactor._buildEngine` upper-cases the value before
|
|
switching. The schema is left intact so external Phase-7 enum-casing
|
|
work can decide whether to preserve original casing globally.
|
|
|
|
**Decision needed by:** Phase 7 (topic-name + schema standardisation) —
|
|
once enums standardise on a canonical casing, drop the `.toUpperCase()`
|
|
guard here.
|