The second audit's blocking half

.claude/skills/audit/SKILL.md

@@ -26,7 +26,7 @@ Split by what the code shares, not by directory size. One area per run.
 
 Read `CLAUDE.md`, `CONTRIBUTING.md` and `src/__tests__/conformance.test.ts`.
 
-The suite already asserts nine conventions. **Do not report what it asserts** —
+The suite already asserts thirteen conventions. **Do not report what it asserts** —
 it is green, so those are closed. Audit what a test cannot see.
 
 ## The eight criteria

CHANGELOG.md

@@ -9,6 +9,60 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **The transaction log now shows the address of a coil read and of a write.**
+  The Addr column was blank for every coil read, every discrete input read and
+  every write Modbux sends, so a log of coil traffic said nothing about which
+  address each request went to.
+- **The unit ID scan no longer asks for more than a device can answer.** Its
+  Length field took anything up to 65535 and sent it as the quantity, which is
+  past what the protocol allows. Every device on the bus then refused the
+  request or said nothing, so what the scan told you was about the request
+  rather than about the device. The field now stops at 125 registers, or 2000
+  bits when only bit types are selected.
+- **A 64-bit server register now shows the value a master wrote.** A master
+  writing an INT64, UINT64 or DATETIME register sends four words, and the server
+  folded them through a number that carries 53 bits, so anything above that came
+  back rounded. Writing one more word to that register afterwards collapsed it
+  to the last word alone. A DATETIME at the far end of the range read back one
+  millisecond late for the same reason.
+- **A configuration from a newer Modbux is now checked before it is loaded.** It
+  was the one file that went in unread: the fields were taken as they came, so a
+  register map a newer version writes differently was written straight into the
+  app. The warning said some features may not work correctly. The file is now
+  read against what this version understands, what matches is kept, and the
+  message says which parts did not come across.
+- **A file that is not a configuration is now refused instead of opened.**
+  Picking a JSON file holding a number, a string or a list emptied the server
+  and reported that the configuration had been updated from an older format. A
+  file holding nothing at all emptied it too and then failed with a message
+  about a property of null. A file claiming a version that is not a number got
+  one of those or the newer-version warning, depending on the value.
+- **Opening the wrong file no longer empties the server.** Picking a client
+  config, or any file Modbux cannot read as a server config, cleared the setup
+  that was on screen before anything had read the file, and then reported the
+  failure. The file is read first now, and a refused one costs nothing.
+- **The server view no longer comes up blank and stays that way.** One thing main
+  refused while the setup was being handed over left the whole view faded out,
+  on that launch and on every launch after it, with no message and nothing on
+  screen to clear the configuration with. A refusal now costs the one server it
+  belonged to.
+- **A server added in the split out window can be changed when that window
+  closes.** Its port, unit id and byte order each refused every change with no
+  message until Modbux was restarted.
+- **A server register the encoder cannot serve is now refused by name.** A
+  config file could put a fixed value outside the range of its own data type, a
+  string wider than the register map, a register running past the last address,
+  a generator drawing between two numbers its data type cannot hold, or a
+  generator whose interval was so long it fired every millisecond instead.
+  Opening it left the server view blank for that launch and every launch after
+  it, with no message and nothing on screen to clear it with. The file is now
+  refused with the register and the field named, and one already saved is
+  dropped on the next launch with the rest of the setup kept.
+- **Splitting the server into its own window no longer turns Read
+  Configuration off.** The toggle stayed on while the reading went back to the
+  address and length in the toolbar, so every configured register above the
+  tenth stopped being read and its value left the grid. The split out window now
+  leaves the client alone.
 - **A DATETIME write now carries the date you typed.** The write dialog took a
   value field whose range was stated in seconds while the register is encoded
   from milliseconds, so every value it accepted fell before the year 2000 and

CONTRIBUTING.md

@@ -145,6 +145,21 @@ like a guard. The caller has to be in `src/renderer`, because a channel only the
 e2e suite drives is one the app does not use, and that is a decision to take
 rather than to let happen.
 
+**Every channel that reaches main's client is placed.** `main/index.ts`
+constructs one `ModbusClient` and no client channel carries an addressee, so any
+window can aim one at it, and both windows load the same renderer bundle.
+`CLIENT_CHANNELS` in `main/ipc.ts` is the list `createIpcHandle` refuses from a
+window that is not `windows.main`, and it is written by hand. A channel added
+later is a name, a spec entry and a handler, and every other rule here stays
+green while it escapes that list. The signal the test reads is the handler's own
+body: a listener naming `client` or `state` reaches what one window owns.
+Everything else drives a server, which is addressed by uuid, or asks `app` or a
+Linux helper. Three reach the client and are deliberately not refused, and the
+test names them with the reason: `list_serial_ports` and `validate_serial_port`
+enumerate hardware and the RTU server's COM field asks for the first of the two
+from the server window, and `get_client_state` answers a value rather than
+`undefined`, so a refusal would have nothing to hand back.
+
 **Every configured path alias is imported through.** `@renderer/*` and `@shared`
 are the two, in the tsconfigs and in `electron.vite.config.ts` alike. An alias
 nobody imports through resolves whatever it points at, including a directory

e2e/specs/01-main/05-file-io.spec.ts

@@ -48,6 +48,36 @@ test.describe.serial('File I/O — open, save, clear server and client configs',
     await mainPage.waitForTimeout(300)
   })
 
+  // ─── A file the server cannot read ────────────────────────────────────
+
+  // `useOpen` awaited `resetServer` above its `try`, and `resetServer` reaches
+  // main and then clears the store. `migrateServerConfig` throws on both shapes
+  // a user picks by accident, a client config file and malformed JSON, and the
+  // `catch` only enqueues a snackbar, so the configuration on screen was gone
+  // before anything had read the file.
+  test('opening a client config into the server says so and keeps the config', async ({
+    mainPage
+  }) => {
+    const fileInput = mainPage.getByTestId('server-open-file-input')
+    await fileInput.setInputFiles(CONFIG_FILES.clientBasic)
+
+    await expect(mainPage.locator('.notistack-SnackbarContainer')).toContainText(
+      'Failed to load config',
+      { timeout: 5000 }
+    )
+
+    await expect(mainPage.getByTestId('server-name-input').locator('input')).toHaveValue(
+      'Basic Server'
+    )
+    await expect(mainPage.getByTestId('section-holding_registers')).toContainText('(2)')
+    await expect(mainPage.getByTestId('section-input_registers')).toContainText('(1)')
+  })
+
+  test('the buttons come back after a file it could not read', async ({ mainPage }) => {
+    await expect(mainPage.getByTestId('server-clear-btn')).toBeEnabled()
+    await expect(mainPage.getByTestId('server-save-btn')).toBeEnabled()
+  })
+
   // ─── Server config clear ──────────────────────────────────────────────
 
   test('clear server config — verify all sections show (0)', async ({ mainPage }) => {
@@ -138,7 +168,10 @@ test.describe.serial('File I/O — open, save, clear server and client configs',
     const content = await fs.readFile(savePath, 'utf-8')
     const config = JSON.parse(content)
 
-    expect(config.version).toBe(2)
+    // `CURRENT_SERVER_CONFIG_VERSION`, written out because importing `@shared`
+    // here pulls the barrel and with it modules that need Electron. It moved to
+    // 3 when a 64 bit value became a decimal string.
+    expect(config.version).toBe(3)
     expect(config.name).toBe('Main Server')
     expect(config.littleEndian).toBe(false)
     expect(config.serverRegistersPerUnit).toBeDefined()

e2e/specs/01-main/12-scan-unitids.spec.ts

@@ -95,6 +95,46 @@ test.describe.serial('Scan Unit IDs', () => {
     await expect(input).toHaveValue('500')
   })
 
+  // ─── Length field ──────────────────────────────────────────────────
+
+  // The field passed no `max`, so `UintInput`'s default of 65535 was typeable
+  // and `modbus-serial` wrote it into the quantity field. A device answers
+  // illegal-data-value or says nothing, and the answer reads as a fact about
+  // the bus rather than about the request. FC03 answers at most 125 registers.
+  test('the length field stops at what one read of a register answers', async ({ mainPage }) => {
+    const input = mainPage.getByTestId('scan-unitid-length-input').locator('input')
+    await input.click()
+    await input.selectText()
+    await input.pressSequentially('65535')
+    await input.blur()
+
+    await expect(input).toHaveValue('125')
+  })
+
+  // FC01 answers 2000 bits, so selecting a bit type widens the same field.
+  test('and at 2000 once a bit type is selected on its own', async ({ mainPage }) => {
+    const modal = mainPage.locator('.MuiModal-root')
+    await modal.getByTestId('scan-unitid-type-coils').click()
+    await modal.getByTestId('scan-unitid-type-holding-registers').click()
+
+    const input = mainPage.getByTestId('scan-unitid-length-input').locator('input')
+    await input.click()
+    await input.selectText()
+    await input.pressSequentially('65535')
+    await input.blur()
+
+    await expect(input).toHaveValue('2000')
+
+    // Back to the defaults the tests below read.
+    await modal.getByTestId('scan-unitid-type-holding-registers').click()
+    await modal.getByTestId('scan-unitid-type-coils').click()
+    await input.click()
+    await input.selectText()
+    await input.pressSequentially('2')
+    await input.blur()
+    await expect(input).toHaveValue('2')
+  })
+
   // ─── Register type toggle buttons ──────────────────────────────────
 
   test('Holding Registers is selected by default', async ({ mainPage }) => {

e2e/specs/01-main/15-client-toolbar.spec.ts

@@ -255,6 +255,29 @@ test.describe.serial('Client toolbar — display options and utilities', () => {
     await expect(logPanel).not.toBeVisible()
   })
 
+  // modbus-serial files `nextDataAddress` on two of its twelve transaction
+  // records, so FC3, FC4 and FC6 carried an address and FC1, FC2, FC5, FC15 and
+  // FC16 did not: the Addr cell was blank for every coil read, every discrete
+  // input read and every write Modbux sends.
+  test('the log shows the address of a coil read, which FC1 files none for', async ({
+    mainPage
+  }) => {
+    await selectRegisterType(mainPage, 'Coils')
+    await mainPage.getByTestId('show-log-btn').click()
+
+    await readRegisters(mainPage, '4', '2')
+
+    const logPanel = mainPage.getByTestId('transaction-log-panel')
+    const addressCell = logPanel
+      .locator('.MuiDataGrid-row')
+      .first()
+      .locator('[data-field="address"]')
+    await expect(addressCell).toHaveText('4', { timeout: 5000 })
+
+    await mainPage.getByTestId('show-log-btn').click()
+    await selectRegisterType(mainPage, 'Holding Registers')
+  })
+
   // ─── Register read config toggle ──────────────────────────────────
 
   for (const regType of ['Holding Registers', 'Input Registers']) {

e2e/specs/01-main/31-split-view-client-state.spec.ts

@@ -0,0 +1,85 @@
+import { test, expect } from '../../fixtures/electron-app'
+import {
+  navigateToClient,
+  navigateToHome,
+  connectClient,
+  disconnectClient,
+  enableAdvancedMode,
+  enableReadConfiguration,
+  disableReadConfiguration,
+  cleanServerState,
+  clearData,
+  loadServerConfig,
+  loadClientConfig,
+  selectRegisterType,
+  splitOutServerWindow,
+  expectCell
+} from '../../fixtures/helpers'
+import { resolve } from 'path'
+import { type Page } from '@playwright/test'
+
+const CONFIG_DIR = resolve(__dirname, '../../fixtures/config-files')
+const SERVER_CONFIG = resolve(CONFIG_DIR, 'server-large-config.json')
+const CLIENT_CONFIG = resolve(CONFIG_DIR, 'client-large-config.json')
+
+let serverPage: Page
+
+// Both windows load the one renderer bundle, so both evaluate
+// `client.zustand`'s module scope, and `init` there called
+// `setReadConfiguration(false)` with no addressee. The main window's toggle
+// still read on, and the next read went out as one flat `[address, length]`
+// block instead of the mapping's groups: the length field holds 10, so every
+// configured register above address 9 stopped being read. Only two windows can
+// see it.
+test.describe.serial('Read configuration survives the split out server window', () => {
+  test.afterAll(async ({ electronApp }) => {
+    await electronApp.evaluate(({ BrowserWindow }) => {
+      BrowserWindow.getAllWindows()
+        .filter((w) => w.getTitle() === 'Server')
+        .forEach((w) => w.close())
+    })
+  })
+
+  test('clean server state', async ({ mainPage }) => {
+    await cleanServerState(mainPage)
+  })
+
+  test('load 120 holding registers into the server and the matching mapping', async ({
+    mainPage
+  }) => {
+    await loadServerConfig(mainPage, SERVER_CONFIG)
+    await navigateToClient(mainPage)
+    await enableAdvancedMode(mainPage)
+    await selectRegisterType(mainPage, 'Holding Registers')
+    await loadClientConfig(mainPage, CLIENT_CONFIG)
+  })
+
+  test('connected, read configuration fills every configured register', async ({ mainPage }) => {
+    await connectClient(mainPage, '127.0.0.1', '502', '0')
+    await enableReadConfiguration(mainPage)
+
+    await expectCell(mainPage, 0, 'word_uint16', '1')
+    await expectCell(mainPage, 50, 'word_uint16', '51')
+  })
+
+  test('the split out window leaves the mapping reading', async ({ electronApp, mainPage }) => {
+    await navigateToHome(mainPage)
+    serverPage = await splitOutServerWindow(electronApp, mainPage)
+    await expect(serverPage.getByTestId('section-holding_registers')).toBeVisible()
+
+    // The rows the previous test read already satisfy the assertion below, and
+    // the read that would empty them lands after it. Clear them first, so the
+    // grid has to be filled again rather than left alone.
+    await clearData(mainPage)
+    await expect(mainPage.locator('.MuiDataGrid-row[data-id="50"]')).toHaveCount(0)
+
+    await mainPage.getByTestId('read-btn').click()
+
+    await expectCell(mainPage, 50, 'word_uint16', '51')
+  })
+
+  test('cleanup', async ({ mainPage }) => {
+    await disableReadConfiguration(mainPage)
+    await disconnectClient(mainPage)
+  })
+})

src/__tests__/conformance.test.ts

@@ -698,6 +698,83 @@ describe('the lists that name a channel agree', () => {
   })
 })
 
+//
+// ─── Every channel that reaches main's client is placed ──────────────────────
+//
+// `main/index.ts` constructs one `ModbusClient`, and no client channel carries
+// an addressee, so any window can aim one at it. `CLIENT_CHANNELS` in
+// `main/ipc.ts` is the list `createIpcHandle` refuses from a window that is not
+// `windows.main`, and it is hand written. A channel added later is three lines,
+// a name, a spec entry and a handler, and every one of the rules above stays
+// green while it escapes the guard, which is the defect this list was added to
+// close.
+//
+// The signal is the handler's own body: a listener naming `client` or `state`
+// reaches what one window owns. Everything else drives a server, which is
+// addressed by uuid, or asks `app` or a Linux helper.
+
+describe('every channel that reaches main’s client is placed', () => {
+  /**
+   * The channels that reach the client and are deliberately not refused.
+   *
+   * The two serial ones enumerate hardware rather than touch the client, and
+   * `server.zustand.ts refreshSerialPorts` calls the first of the two for the
+   * RTU server's COM field. `get_client_state` answers a `ClientState` rather
+   * than `undefined`, so a refusal has nothing to hand back, and reading what
+   * main is doing changes nothing about it.
+   */
+  const ALLOWED_FROM_EITHER_WINDOW = [
+    'list_serial_ports',
+    'validate_serial_port',
+    'get_client_state'
+  ]
+
+  const ipc = parse(join(repoRoot, 'src/main/ipc.ts'))
+
+  /** Every channel whose listener body names `client` or `state`. */
+  const reachesTheClient = new Set<string>()
+  eachNode(ipc, (node) => {
+    if (!ts.isCallExpression(node) || !ts.isIdentifier(node.expression)) return
+    if (node.expression.text !== 'ipcHandle') return
+    const [channel, listener] = node.arguments
+    if (!channel || !ts.isStringLiteral(channel) || !listener) return
+    if (/\b(client|state)\./.test(listener.getText(ipc))) reachesTheClient.add(channel.text)
+  })
+
+  /** The names `CLIENT_CHANNELS` holds. */
+  const refused = new Set<string>()
+  eachNode(ipc, (node) => {
+    if (!ts.isVariableDeclaration(node)) return
+    if (node.name.getText(ipc) !== 'CLIENT_CHANNELS') return
+    eachNode(node as unknown as ts.SourceFile, (child) => {
+      if (ts.isStringLiteral(child)) refused.add(child.text)
+    })
+  })
+
+  it('finds channels to check', () => {
+    expect(reachesTheClient.size).toBeGreaterThan(10)
+    expect(refused.size).toBeGreaterThan(10)
+  })
+
+  it('refuses every one of them from the server window, or says why not', () => {
+    const unplaced = [...reachesTheClient]
+      .filter((channel) => !refused.has(channel))
+      .filter((channel) => !ALLOWED_FROM_EITHER_WINDOW.includes(channel))
+      .sort()
+    expect(unplaced).toEqual([])
+  })
+
+  it('names no channel the handlers do not reach the client through', () => {
+    const stale = [...refused].filter((channel) => !reachesTheClient.has(channel)).sort()
+    expect(stale).toEqual([])
+  })
+
+  it('allows only channels that are there', () => {
+    const gone = ALLOWED_FROM_EITHER_WINDOW.filter((channel) => !reachesTheClient.has(channel))
+    expect(gone).toEqual([])
+  })
+})
+
 //
 // ─── Every channel carrying an object declares a schema ──────────────────────
 //