OM-M1.1: the model takes the front door, protocol moves to pyquadcortex.protocol

[jonathanstokes]

Closes #9

What changed

import pyquadcortex now gives you the Quad Cortex itself. The message-level API
that used to live there moved down one level, to pyquadcortex.protocol.

Nothing about the protocol API changed except where it lives. Same classes, same
methods, same arguments, same results. It is a move, not a rewrite.

What you have to do

Change one import line.

# before
import pyquadcortex
from pyquadcortex import Scene, Setlist

with pyquadcortex.connect() as qc:
    qc.switch_scene(Scene.B)

# after
from pyquadcortex import protocol
from pyquadcortex.protocol import Scene, Setlist

with protocol.connect() as qc:
    qc.switch_scene(Scene.B)

That is the whole migration. qcctl behaves exactly as before. If you installed
the package in editable mode, reinstall it so the console script finds its new
home.

Why break it now

The library exists to give you an object model of the unit, and that model is now
being built. The name at the front door should be the thing people want, and that
is the model, not the wire. This library is at version 0.x with roughly no users,
so the break costs almost nothing today and would cost a lot later. The decision
was made and recorded ahead of time as ADR-0006.

The version is deliberately NOT cut in this pull request. It gets cut once the
model can actually read a preset, so that no published release ever has
connect() meaning two different things.

What the model gives you today

Not much yet, and it says so rather than pretending.

import pyquadcortex

with pyquadcortex.connect() as device:
    print(device.firmware, device.serial)

Presets, scenes and the grid are the next stories in the same Epic. Use the
protocol layer for anything the model does not cover. To use both layers in one
script, wrap a connection you already have with Device.from_client(qc) - it does
not take ownership, so closing the Device leaves your connection open.

How you can check the move was faithful

Four tests do the checking, so nobody has to take my word for it.

tests/test_namespace.py reads a committed copy of the old
pyquadcortex/__init__.py, taken verbatim from git at 0.40.0, and asserts that every single name it used to export is reachable
under pyquadcortex.protocol. The list of names is generated from that file, not
typed out, so a name dropped in the move cannot slip past by being forgotten from
a checklist. The same file also enforces the new layering rule: nothing under
pyquadcortex/protocol/ may import from pyquadcortex/model/.

tests/test_import_cleanliness.py walks every module in the package and imports
each one in a fresh process, checking that none of them pulls in hid. That rule
already existed, but it was enforced by people remembering it. Now it covers
anything anyone adds, in either namespace.

tests/test_packaging.py reads the qcctl target out of pyproject.toml and
imports it, the way the installed console script does. That string is the one
part of this change a user could notice, and nothing was checking it.

There is also a mechanical check on the diff itself: every changed line in the
moved modules contains the word pyquadcortex, which means every change is an
import path or a path inside a docstring. No logic was touched.

The one decision recorded here that is not about namespaces

ADR-0007: the model may represent a control whose wire path is still open, as long
as the operation it cannot perform refuses rather than guesses.

The case that raised it is TEMPO MODE. The unit's Tempo menu has a GLOBAL /
PRESET switch, and we know exactly what it does. What we have not found is the
message that drives it. Three tests watched for an announcement when the switch is
changed and saw nothing, and the docs had written that down as "not on the wire at
all". That was too strong: the unit not announcing something is not the same as
the unit refusing to tell you. Every one of those tests listened, and none of them
asked.

So the design doc now models the switch and refuses it, instead of omitting it or
letting a write through and guessing which scope it landed in. The last of those
is the failure this whole project is trying to avoid, because a wrong write on
this device looks exactly like a right one.

The correction lands in the protocol record too, not just the design doc:
protocol.md, manual-coverage.md, and capture.md, which had been using the
old conclusion as its worked example of a trustworthy negative result. It now
carries the other half of that lesson: a listener can only prove what a listener
measures.

Nothing about tempo ships here. It is an M3 surface.

Test results

Offline suite:

511 passed, 1 skipped in 6.51s

Hardware suite, on the connected unit:

5 passed, 1 skipped in 12.07s

The two scene echo failures reported here earlier are gone. They were the
no-presence trap fixed in #18, which has since been merged into this branch. No
restore failures, so the unit was left as it was found.

Things worth a second opinion

• The generated protobuf bindings moved too, to pyquadcortex/protocol/proto/.
  Leaving them at pyquadcortex.proto, right next to pyquadcortex.protocol,
  would have been a permanent trap for anyone reading an import. Nothing was
  regenerated; the files are byte-identical and git recorded them as renames.
  ADR-0001 is already decided and append-only, so its pyquadcortex/proto/ paths
  stay as written; the steering doc's change log records where the directory went
  and why.
• qcctl moved to pyquadcortex.protocol.cli and pyproject.toml follows it.
  The command is unchanged. This is the one part of the move a user could notice,
  and only until they reinstall.
• Device.client is a public way back down to the protocol layer. The story
  did not ask for it. Without it, anyone who calls pyquadcortex.connect() is
  stuck in the model, and they cannot open a second connection because the device
  allows only one.
• The version string moved to pyquadcortex/_version.py. Both namespaces
  publish __version__, and this is the way to do that without one importing the
  other.
• A closed Device now refuses reads. This bullet used to say the behaviour
  was undefined and consistent with a closed QuadCortex. Review showed that was
  wrong: a Device that had already read its identity kept answering firmware
  and serial from cache, with no unit behind it, while repr() said closed.
  firmware, serial and client now raise. That defines the explicit
  close() only - a connection that goes away on its own is still the reconnect
  story, OM-M1.7: Scripts survive the unit disconnecting, sleeping, and coming back #15.

Review round

The 30 review threads were worked in a follow-up commit. 25 are fixed and
resolved, 5 are left open for the owner:

• pyquadcortex/model/__init__.py - "model" already names an amp or pedal
  block in this codebase, so the package directory collides. Four options and
  their costs are on the thread. No rename made; this one is yours.
• Device.client lifetime and _owns_client visibility - partly closed
  (client raises after close, repr() says owns or borrows). The rest is an
  API decision that belongs with the reconnect code.
• ADR-0007 has no exception type or test yet - the reviewer agrees it is not
  a defect; whether to pin the exception and a supported predicate now is an
  owner call.
• tests/test_docs.py - the stale-row blind spot is fixed; running
  documentation examples and covering the model's surface are not, and are
  bigger than this change.

One thing found while working them: the venv's grpcio-tools 1.83.0 emits
gencode 5.29.3, while the committed bindings are 7.35.1 and pyproject.toml
pins protobuf>=7.35.1,<8. A regeneration today walks the gencode backwards
with no symptom, because the runtime only validates runtime >= gencode.
scripts/compile_protos.sh now prints what changed so it is visible, and
CLAUDE.md says to read the gencode version in the diff. The grpcio-tools>=1.68
floor in the dev extra probably wants raising - not touched here.

🤖 Generated with Claude Code

[jonathanstokes]

Reviewed the head of this branch (c31a24a), so this covers the fix batch in 818b891 as well as the original change.

Verdict: comments only, nothing here blocks a merge in my view. GitHub will not let an author approve their own pull request, so this is a COMMENT review rather than an APPROVE.

Two things are worth doing before the release cut, both about three lines each, and both in pyquadcortex/model/device.py: firmware and serial can hand back an empty string without saying so, and a closed Device keeps answering from cache while its own docstring says it cannot. Details are inline. They stand out because they are the exact failure this library is built to avoid, and because ADR-0007 in this same change writes down the rule they break.

What I checked rather than took on trust

• The offline suite really is 479 passed, 1 skipped.
• The move really is behaviour-preserving. All ten moved modules diff clean against main with every changed line an import path or a path inside a docstring. framing.py and hid_ids.py are byte-identical. Export parity is exact at 70 of 70 names.
• Packaging works end to end. A built wheel carries the _pb2 files and entry_points.txt reads qcctl = pyquadcortex.protocol.cli:main.
• The .gitignore edit is comment-only, and all three proto files are still tracked, so ADR-0001 holds.
• import hid appears exactly once, lazily, at pyquadcortex/protocol/session.py:48.
• The hid guard genuinely goes red when broken: 18 of 19 cases fail on an injected module-scope import, including one wrapped in try/except.

Three notes with no single line to attach them to

No py.typed marker anywhere in the package. Python's rule is that a package without that marker is treated as untyped, so every type annotation on Device, on connect(), and across the whole protocol surface is invisible to anyone running mypy or pyright against this library. The fix is one empty file at pyquadcortex/py.typed. packages = ["pyquadcortex"] in pyproject.toml would ship it as-is. Worth doing in this change specifically, because this is the pull request that restructured the package and added tests/test_packaging.py to pin exactly this kind of promise.

The transport's logger name changed. It was pyquadcortex.transport and it is now pyquadcortex.protocol.transport, because the logger takes its name from the module. Nothing breaks, and no documentation names a logger, but anyone who wrote logging.getLogger("pyquadcortex.transport") in their own setup will find that line silently stops doing anything. The changelog currently says nothing changed except where things are imported from, which technically covers it, but a reader configuring logging would not connect the two.

QuadCortex.close() swallows every exception with no log at all (pyquadcortex/protocol/client.py:228). This is older than this change and is outside the deliberate "the RX thread never dies" exemption, so I am not asking for it here. Flagging it because this change puts the library's documented front door directly on top of it: if releasing the USB handle fails, the unit stays held, and the user finds out on the next run or when Cortex Control refuses to attach, with nothing in this run's output. One log.warning in that handler would make it visible.

Worth saying plainly

The move is clean, and I went looking for drift rather than assuming. Every new parametrised guard carries its own check against passing vacuously, three for three, which is rarer than it should be. _reraise in tests/test_import_cleanliness.py is a genuinely subtle catch: noticing that walk_packages swallows import errors, so a broken subpackage would have quietly shrunk the list into a clean sweep of a package nobody ever opened.

The documentation correction is the best part of this change. Walking "MODE is not on the wire" back to "MODE is never broadcast" across three documents, and adding the second half of the lesson to the chapter that had been using the old claim as its worked example, fixes the overreach where it actually lived instead of only in the new design document. The four comments I left on that section are about finishing that job, not about the direction of it.

[jonathanstokes]

This pattern skips a stale row instead of flagging it, and stale rows are exactly what this change could leave behind.

The regex is anchored on a backtick, so a leftover row reading `pyquadcortex.blocks(` matches nothing at all: the optional protocol. group fails, then [a-z_][a-z0-9_]* swallows pyquadcortex and runs into a . where it needs a (. Before this change that row was the checked case. Now it is silently passed over.

I checked docs/api.md and it is fully converted, 0 stale rows against 17 converted ones and 106 names checked, so there is no live miss. It is a blind spot pointing in the direction this change moves.

Two related gaps while this is open: nothing here executes a documentation example, it only pattern-matches the table, so every snippet in README.md, architecture.md and capture.md is unchecked by CI. And the model's surface, pyquadcortex.connect, device.firmware, Device.from_client, is not covered at all, even though those are the new front-door names.

[jonathanstokes]

Confirmed the regex behaviour exactly: a row reading `pyquadcortex.blocks(`
produces no match at all, for the reason you give. Fixed.

A new test names stale rows instead of skipping them. Any `pyquadcortex.something(` in that table that is not protocol. or model.
fails, listing what it found. docs/api.md is clean today, so it starts green
and stays a guard pointing the direction this change moves.

Leaving this open, because I have not closed the two related gaps and I do not
want to resolve them on your behalf.

Executing a documentation example needs a fake stack the docs can be run
against, or a hardware run. Covering the model's surface means api.md growing
a model section, which is worth doing when the model has more than identity on
it rather than now. Both are bigger than this change. Say if you want either
filed as its own issue.

[jonathanstokes]

This rule has no code, no exception type and no test yet, which is consistent but leaves the first user of it with nothing to inherit.

I am not calling this a defect. The ADR says the exception shape stays open until M3, the pull request says no tempo surface ships here, and pyquadcortex/model/ contains no raise at all, so the three statements agree.

The thing worth noticing is that the four-test safety case in this change does not touch ADR-0007 at all, so nothing mechanical will stop the first M3 surface from guessing. Two decisions are much cheaper to pin now, while the reasoning is fresh, than to retrofit later:

The refusal wants its own exported exception. The pattern already exists here in BlockRefused at pyquadcortex/protocol/client.py:136, but reusing that one would be wrong: BlockRefused means the device said no, and this means we have not found the message, and merging them makes both impossible to catch separately. NotImplementedError is also a poor fit, since callers cannot tell it from a genuine unfinished-method bug and tooling reads it that way too.

There also needs to be a way to ask without touching. With a property that raises, "read it if you can, skip it if you cannot" forces a try/except around every attribute, and any generic state dump or inspector blows up on attribute access. The comment on Device.__repr__ shows this concern is already understood one level up; a raising property reintroduces it one level down. Pairing the exception with a supported predicate avoids that.

When the first refusing property does land, the test worth writing is that the refusal happens with a client whose send and request fail the test if called at all, so refusing-before-writing is pinned rather than assumed.

[jonathanstokes]

Agreed it is not a defect, and I have not turned it into code. Not resolving.

ADR-0007's Open Questions already parks the exception shape until the first such
control ships, M3 at the earliest, so the three statements you checked still
agree. Settling it now means designing an M3 API with no consumer in front of
us. The record is also Decided, so a new constraint would have to arrive as a
resolution of that open question or as a new record, not as an edit.

Your two constraints are the useful part and I think both are right. Reusing
BlockRefused would merge "the device said no" with "we have not found the
message", and then neither can be caught on its own. NotImplementedError reads
as an unfinished method to people and to tooling. The supported predicate is
the point I had not considered: a raising property does break a generic state
dump, which is the same concern the __repr__ comment is protecting one level
up.

I did correct one fact in the record while I was in it. It said all three tests
watched the switch "changed and committed", and the third test's action script
has only the toggle - no OK step before the BANK UP that ends the section. Two
of the three committed. ADR-0007 has not shipped, since this same pull request
introduces it, so fixing a fact in it is not rewriting a shipped decision.

Whether to pin the exception and the predicate now is your call.