Conversation
|
@codex review this pull request at head Three things deserve the sharpest look:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cc6510168
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # refusal of the whole batch. | ||
| self._unbound_channel_rows += 1 | ||
| self._say_channel_is_unbound(reading.channel, absent) | ||
| return None |
There was a problem hiding this comment.
When an absent-channel row is later consumed through descriptor replay, this bare NULL is not reported as DESCRIPTOR_HASH_MISSING: ArchiveReader._read_sqlite_bounded maps it to resolve_legacy_descriptor (archive_reader.py:1361-1366), and DescriptorReplayReader._from_query_result only reports missing identity when row.descriptor is None (broker_replay.py:162-189). The result is therefore marked complete with a fabricated legacy=True identity; descriptor reporting can then exclude the row from alarm classification and emit the clean “no alarms” result. Exports preserve the NULL, but the bounded reader/report path treats a newly unbound row as pre-catalog legacy history, so the stored representation needs explicit unbound provenance (preserved through cold rotation) or the reader must otherwise surface the missing-identity issue.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
| self._unbound_channel_rows += 1 | ||
| self._say_channel_is_unbound(reading.channel, absent) |
There was a problem hiding this comment.
If an absent reading precedes a described-channel unit/instrument mismatch, or the subsequent SQLite transaction fails, these lines increment the counter and emit “readings are being saved” before any transaction begins; the later error rejects the whole batch, leaving zero stored rows while the counter says one and the rate limiter suppresses another notice for five minutes. Defer the count/rate-limit update until the transaction containing that unbound row commits, or make the message and counter explicitly describe attempted rather than persisted rows.
AGENTS.md reference: AGENTS.md:L16-L18
Useful? React with 👍 / 👎.
| if key not in self._unbound_channel_said and len(self._unbound_channel_said) >= _MAX_REMEMBERED_UNBOUND_CHANNELS: | ||
| # Forget the channel that spoke longest ago. Saying one line twice costs a | ||
| # log line; remembering every name a driver ever emitted costs the week. | ||
| self._unbound_channel_said.pop( | ||
| min(self._unbound_channel_said, key=self._unbound_channel_said.__getitem__), |
There was a problem hiding this comment.
If a faulty driver cycles through more than 256 distinct channel labels within five minutes, each new label evicts the oldest timestamp; with 257 rotating labels, every label has been evicted by the time it reappears, so every reading emits a warning instead of once per channel per interval. This defeats the stated week-long log bound and permits sustained log growth despite the bounded dictionary; use a global/aggregate limiter or an eviction scheme that does not immediately forget still-rate-limited labels.
Useful? React with 👍 / 👎.
Three review findings, each verified against the code before acting. P1, and it changed the design. The claim that the bounded reader already reported a null descriptor hash as DESCRIPTOR_HASH_MISSING was false. It resolved the null through resolve_legacy_descriptor, which hands the row a fabricated pre-catalog identity and leaves the read marked complete; descriptor reporting would then carry a newly unbound row as ordinary described history. Storing the row honestly and reading it back dishonestly is worse than either alone, so the reader is fixed in the same slice: in an archive that itself carries a descriptor catalog -- an installed catalog in the hot database, a positive descriptor-row count in the rotated index -- a null identity is reported as missing rather than synthesised. A database written before catalogs existed carries no catalog and still reads as legacy, and that boundary has its own test. The discriminator is deliberately NOT descriptor_map: that map is filtered to the hashes the rows reference, so a day whose rows are all unbound would leave it empty in a database that does carry a catalog. P2, counting. The count and the notice moved out of the row builder and into the commit path. A batch can still be destroyed after its rows are built -- a described channel whose unit disagrees raises, a disk-full fault returns False -- and announcing readings as saved that were then discarded is a false statement, made worse by the rate limiter staying silent about the next interval of real ones. The retry loop reaches the record point once, so a retried transaction cannot count the same rows twice. P2, log bound. A per-channel limit is not a bound: a driver cycling through more labels than are remembered evicts each one before it returns, so every label looks new and speaks. There is now a hard ceiling of named lines per interval plus one aggregate line naming the row and channel counts, which rotation cannot defeat. Every property was falsified: the hot guard, the cold guard, counting at build time instead of at commit, and the named-line ceiling each redden exactly their own tests. Residual scope, stated rather than left to be found: resolve_sqlite_descriptor still resolves a null as legacy. It has no production caller in src/ -- read_sqlite_reading is a tool and test surface -- and exports preserve the null without synthesising identity.
|
@codex review this pull request at head All three findings are answered, and the first one changed the design. Your P1 was right, and it exposed a false claim of mine. I had written that the bounded The reader is fixed here, in both the hot and the cold path, discriminating on whether the archive The pre-catalog boundary has its own test, built by driving the real writer with no catalog rather P2 counting: moved to the commit path. Please confirm the three discard paths — the raise, the P2 log bound: a hard named-line ceiling per interval plus one aggregate line. Residual scope stated in the description: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d16e18590
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except ChannelDescriptorStorageError as exc: | ||
| raise _DescriptorReadError(BoundedReadIssueCode.DESCRIPTOR_INDEX_MISMATCH) from exc | ||
| catalog_is_installed = ( | ||
| conn.execute("SELECT 1 FROM main.channel_descriptors LIMIT 1").fetchone() is not None |
There was a problem hiding this comment.
When a pre-catalog daily database is reopened by a catalog-bearing writer on the same day, initialize_descriptor_storage deliberately migrates the existing rows without rewriting their NULL hashes, and the next write installs descriptor rows beside them. This database-wide test then classifies those genuine legacy rows as DESCRIPTOR_HASH_MISSING; because the bounded reader rejects every staged row from an incomplete source, the query loses both the legacy rows and valid newly described rows. Cold rotation preserves the same misclassification through its positive descriptor-row count, so distinguishing unbound data requires per-row provenance rather than catalog presence.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
| cold_catalog_rows = (source.index_entry or {}).get("channel_descriptors_rows") | ||
| cold_catalog_is_installed = type(cold_catalog_rows) is int and cold_catalog_rows > 0 |
There was a problem hiding this comment.
When every reading in a catalog-bearing day is unbound, real cold rotation finds no non-NULL referenced hashes and therefore writes no descriptor sidecar or channel_descriptors_rows index field. This test consequently evaluates false after rotation, so every genuinely unbound row is again synthesized as legacy and the bounded read reports complete—the exact false classification fixed for the hot path. Rotation needs an explicit catalog-present marker independent of the referenced-descriptor sidecar.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
| if not unbound: | ||
| return |
There was a problem hiding this comment.
If an unbound gap is transient—such as one large batch followed only by described readings—this early return prevents the elapsed window from ever reaching _close_unbound_window, and shutdown does not flush it either. The log therefore contains only the first at-most-eight named warnings, whose running totals were captured early, while the promised aggregate line reporting the remaining persisted rows and channel count is never emitted; check and close an elapsed nonempty window even when the current commit has no unbound rows, and flush it during stop.
Useful? React with 👍 / 👎.
CI runs the linter over the CHANGED files only, which is why master stays green with three pre-existing errors while this branch went red on one. I had not run ruff locally at all; that is the whole cause, and it cost two red pull requests.
CI checks formatting of the CHANGED Python files as a separate step from Lint, and it caught these after the lint fix went green. Running the formatter is now part of what I do before a push, alongside ruff check; the tests were re-run AFTER the formatter reflowed the code, because a green run before it proves nothing about the bytes that ship.
|
@codex review this pull request at head My omission, stated plainly: the last two commits on this branch were a lint fix and a Nothing about the substance changed in those two commits — one long line and one There are no live findings at this head, and all required checks succeeded against it. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f86df02b14
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # A database written before catalogs existed reaches | ||
| # this line with catalog_is_installed False and is | ||
| # resolved as legacy exactly as before. | ||
| raise _DescriptorReadError(BoundedReadIssueCode.DESCRIPTOR_HASH_MISSING) |
There was a problem hiding this comment.
At exact head f86df02, this formatter-authored hunk comes from commit 2aebc10 (style: apply ruff format to the files this branch changed), while repository policy prohibits invoking Ruff in any mutation-capable mode and requires formatting corrections to be explicit reviewed patches. Recreate the formatter-produced hunks as manually authored patches and rerun only the sanctioned read-only Ruff checks.
AGENTS.md reference: AGENTS.md:L144-L150
Useful? React with 👍 / 👎.
| nor ``write_immediate``, which logs CRITICAL and re-raises, nor the scheduler, | ||
| which counts an error and refuses to publish. So ONE unbound channel abandoned | ||
| the whole batch -- every good reading beside it -- on every acquisition cycle, |
There was a problem hiding this comment.
When the shipped engine encounters the rewired/new-channel scenario described here, it constructs SQLiteWriter with a LiveChannelDescriptorCatalog, so the scheduler selects begin_committed/write_committed; LiveChannelDescriptorCatalog.bind then rejects the unknown label before _write_day_batch can reach this new catch. The added tests use a plain ChannelCatalog and write_immediate, while the existing production-path integration test still requires the unknown label to reject the whole batch, so this change does not fix the acquisition path it claims to fix; exercise and reconcile the descriptor-authoritative admission path, or narrow the behavior and operator-facing claims to the legacy API.
AGENTS.md reference: AGENTS.md:L380-L386
Useful? React with 👍 / 👎.
Correction: this change does not fix the path I said it fixesBoth findings at this head are confirmed against the code, and the first one is a The write path. I wrote that the defect sits on "the ordinary acquisition path". It does The shipped engine hands the writer the live catalog, so How I got it wrong, since that matters more than the error. I searched for So the honest status of this change: the row-level handling, the reserved identity, and the Two options, and I would rather you judged which.
The second finding is also right: the elapsed window can never close when an unbound gap @codex I am not asking for a verdict at |
Summary
Testing
|
The production path loses the whole batch too — measured, with a controlMy previous comment said this change fixes the wrong path. That stands: the handler is on A The described reading is lost with it, and no database file is created at all. The So the disease is exactly what I described in the pull-request body. I aimed at the wrong What this means for the fix. The registered decision that an unknown raw label must not I will redirect this pull request accordingly rather than open another: same disease, same @codex no verdict wanted at this head — I am changing the aim, not asking you to bless the |
Status
Checks
|
Where the fix actually belongs — located in the code, ready to implementRecording this before implementing so the aim survives an interruption, and so it can be The exact line that destroys the batch is the admission step: One generator over the whole batch. If And this is what
Those two are not the same case, and the difference is the whole design. The second is The change
The reserved descriptor and its pinned identity are already in this pull request and need no The contract this reverses, named before I touch it
Its stated purpose is that "duck typing or unit/name matching can never grant canonical The risk I already measured oncePutting the reserved descriptor into the plain catalog earlier reddened seven tests across @codex no verdict wanted at this head; this is the plan, not the change. |
Status
Checks
|
The redirect works on the real path — and here is what it costsImplemented and measured, same probe and same control as before: Both rows on disk, the batch intact, the receipt issued, and the log naming the channel to What the change is
The cost, stated before anyone else measures it
I sampled two before drawing any conclusion, and both are expectation updates rather than
That is the shape I expect for most of the rest, but expect is not measured, and "make it
@codex no verdict wanted yet. This is the measured position, not a finished change. |
Status
Checks
|
…e engine uses
The disease was named correctly and the location was not. Review found it and I
confirmed it: the shipped engine hands the writer a LIVE catalog
(engine.py:6627), so the scheduler takes write_committed and the refusal happens
at ADMISSION, before any row is built. My earlier handler was on the legacy API,
which no shipped engine reaches. I had read the wrong search hit -- engine.py:6904
constructs the sensor diagnostics, not the writer -- and built on it.
Measured on the real path, with a control:
control, all described -> a receipt, rows: ['sensor.main']
one undescribed -> ChannelDescriptorStorageError,
rows on disk: no database file at all
The described reading dies with the undescribed one, and the scheduler catches
that, counts an error and publishes nothing -- every acquisition cycle, for as
long as the catalog gap lasts. After this change the same batch yields a receipt
and both rows.
WHAT CHANGED. `bind` is untouched and keeps its behaviour for every direct
caller. A sibling `admit` is what the write path uses: a channel with no binding
at all is admitted against the reserved catalog entry, which grants no canonical
identity and states that the channel is not described; a channel described under
another instrument still raises, because that reading may not be the quantity
the descriptor names. `begin_committed` admits instead of binding -- its
comprehension covered the whole batch, which is why one refusal killed every
reading in it.
The reserved entry joins THE CATALOG AND NOT THE BINDINGS. The bindings are what
the tracked manifest declares; the reserved entry declares nothing and exists so
a reading with no declaration has a real descriptor to reference, which the
foreign key requires. An earlier attempt put it in both, and the manifest guards
correctly reported that the live catalog was claiming a channel the manifest
never declared. That correction also removed the need to filter it out of
`instrument_ids`, so the design got smaller.
A reading admitted against the reserved entry KEEPS THE LABEL THE INSTRUMENT
EMITTED. A described reading adopts the canonical channel id, which is what makes
the row, the receipt entry and the descriptor agree; this one has no canonical
identity to adopt, and the emitted label is the only truth left about where the
value came from.
THREE REGISTERED CONTRACTS ARE REVERSED DELIBERATELY, each argued at the line it
is reversed: the unknown-channel case leaves two parametrized refusal lists, and
the end-to-end test is renamed and rewritten rather than deleted -- it still
proves its original purpose, that no canonical identity is granted, and now also
proves the row survives carrying the emitted label.
Nineteen tests reddened when this began. All nineteen are answered: sixteen were
expectation updates, each with its reason written at the line, and three are the
contract reversals above. No guard was weakened; two objections that looked like
bookkeeping were not, and changed the design instead.
877 tests pass across storage, channels and the F35 end-to-end module. Style and
formatting were checked in the READ-ONLY forms the repository requires, and the
three patches the formatter proposed were applied by hand after reading them --
all three were my own insertion mistakes, which a blind mutating run would have
hidden.
|
@codex review this pull request at head |
|
@codex review Head under review: This round answers your findings on the prior head. The coordinator verified the The derived pair was regenerated to a fixed point as the last commit and the docs |
Resolved by keeping master's content and re-applying this branch's own entries on top. Two files conflicted: `src/cryodaq/storage/channel_descriptors.py`, resolved to the reviewed content of this branch's fix, and the derived metrics file, taken from master and regenerated in the following commit. After the merge the branch differs from master in exactly ten paths: the nine the fix touches, and the claim-corrections row that carries the moving changed-Python count. Each of the nine is byte-identical to the tree the reversion control was run against.
|
@codex review Head The merge needed care and is worth checking. The earlier attempt to merge master here was What the change does. A reading on a channel the descriptor catalog does not describe used to Four things I want challenged:
Control I ran myself: restoring the old strict behaviour in place — |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d35046b1db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except ChannelDescriptorStorageError as refusal: | ||
| if "unavailable in the explicit descriptor catalog bindings" not in str(refusal): | ||
| raise |
There was a problem hiding this comment.
When a descriptor maps an emitted alias to another canonical ID, a reading arriving under that canonical ID is absent from the emitted-label side of _bindings, so bind raises the message accepted here and admit assigns the reserved identity—even when the canonical ID belongs to another instrument. Fresh evidence at this head is that, when an aliased reading precedes this canonical-label reading at the same timestamp, the writer persists both with the same (timestamp, instrument_id, channel) but different hashes; _BoundedReadingCollector then replaces the valid described row with the later unbound row. Treat catalog canonical IDs as already described and apply the instrument/binding mismatch refusal instead of this fallback.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
| except ChannelDescriptorStorageError as refusal: | ||
| if "unavailable in the explicit descriptor catalog bindings" not in str(refusal): | ||
| raise |
There was a problem hiding this comment.
If this refusal is reworded, localized, or wrapped while preserving its semantics, the substring check stops recognizing an absent channel and rethrows, restoring the whole-batch loss on every acquisition cycle. ChannelNotDescribedError already exists and is used by the non-live resolver, so make bind raise that type for this case and catch the type here rather than coupling persistence behavior to diagnostic prose.
AGENTS.md reference: AGENTS.md:L20-L22
Useful? React with 👍 / 👎.
Reserved admission was reached by two paths it should never have taken. A channel named by its canonical id, bypassing its declared alias, was treated as undescribed and admitted against the reserved entry. The batch then committed two rows sharing one public key with different descriptor hashes, and the bounded reader replaced the valid described value with the later unbound one. That is a described reading being overwritten by an unbound one, which is the opposite of what reserved admission is for. Bind now raises the binding-mismatch error for a canonical id and keeps the absence error only for a channel nothing describes. The fallback also sorted refusals by matching a substring of the error message. Rewording a refusal would have silently changed which readings are admitted. It now catches ChannelNotDescribedError, a symbol. The new guard drives the real catalog, writer, SQLite and bounded reader, because the defect lives in the interaction between them and no unit-level double can observe it. A second guard pins both sides of the decision: a truly absent label is still admitted, a cross-instrument disagreement is still refused. Control: with the previous admission restored in place, both guards fail at pytest exit 1, the collision test observing 999.0 with descriptor None in place of the described 4.2. Restored byte-identically afterwards.
|
Both findings from the previous round are closed at this head. P1 — a canonical id was treated as undescribed. A channel named by its canonical id, bypassing the alias declared for it, fell through to reserved admission. The batch then committed two rows sharing one public key with different descriptor hashes, and the bounded reader replaced the valid described value with the later unbound one. P2 — refusals were sorted by a message substring. Rewording a refusal would have silently changed which readings are admitted. The fallback now catches Guards added. Control, run at this head. With the previous admission restored in place so the module still imports (blob Local evidence at this head.
Written with AI assistance; every measurement above was run and read by a human-directed session, and nothing is reported that the evidence does not support. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cde6fa3e04
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except ChannelNotDescribedError: | ||
| pass | ||
| owned = _own_live_reading(reading) |
There was a problem hiding this comment.
When an undescribed emitted label is 257–1024 UTF-8 bytes, bind accepts it because _own_live_reading permits 1024 bytes and this fallback admits and persists it, but _discover_sqlite_channels rejects every stored channel over 256 bytes as INVALID_ROW before materialization. I reproduced this at the reviewed head with one described reading and one 257-byte unknown label: both rows committed, yet the bounded reader returned zero rows, hiding the valid described measurement too. Align reserved admission with the archive reader's durable field bounds, or extend the reader so every admitted label remains readable.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
| selected = tuple( | ||
| dict.fromkeys((*sorted(discovered), *sorted(unbound_seen)[:_MAX_MATERIALIZED_UNBOUND_CHANNELS])) |
There was a problem hiding this comment.
When the live writer commits the first row for a new undescribed label after _discover_sqlite_channels returns but before _read_sqlite_bounded opens the hot source, selected excludes that label and unbound_any remains false. The materialization query then filters out the committed row, while the separate per-open identity checks never compare the discovery and read snapshots, so the result can report complete=True; I reproduced this by inserting one valid reserved row at that boundary, after which the reader returned only the described row with no issues. Pin discovery and materialization to one SQLite snapshot, or detect a changed hot source and retry or mark the result incomplete.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
An unknown channel label between 257 and 1024 bytes was accepted by the writer and rejected by the reader. `_own_live_reading` permits 1024 UTF-8 bytes, so reserved admission persisted the row; `_discover_sqlite_channels` rejects any stored channel over 256 bytes as an invalid row, before materialization. Both rows then committed and the bounded reader returned nothing, so a valid described measurement was hidden by the presence of an unknown one beside it. That is the whole-batch loss reserved admission exists to prevent, surviving in a size range nobody had checked. Refusing the over-long label here would have refused its whole acquisition batch and recreated the same loss, and a plain prefix truncation could alias two physical channels onto one row. The durable form keeps a UTF-8-safe prefix and appends the full digest of the exact emitted label, so it fits every durable reader boundary and still identifies the channel. Second defect, in the reader. A row committed for a new undescribed label after `_discover_sqlite_channels` returned but before `_read_sqlite_bounded` opened its source was filtered out of the materialization, while the per-open identity checks never compared the discovery snapshot against the read snapshot. The result reported complete while a committed row was missing. Discovery now captures MAX(id) inside its own snapshot, materialization checks for rows appended past that boundary, and any newly committed unselected channel in range marks the result incomplete. Explicit caller filtering is unaffected and stays intentional. Controls, each restored byte-identically afterwards. With truncation disabled in place, the size guard fails at pytest exit 1 with "an unknown label must never quarantine the described row", asserting 4.2 in an empty mapping. With the discovery watermark removed at the production call site, the race guard fails at exit 1 with "a row excluded by the discovery snapshot makes the read incomplete", asserting True is False on the result's own complete flag. No test was weakened. The query-plan assertion in the bounded-reader guard was widened to cover the watermark and post-discovery plans, and now also requires the post-discovery query to use the integer primary key.
|
Both findings raised at Finding 1 — a long unknown label hid the good measurement too
Refusing the over-long label was rejected as a fix: it would refuse the whole acquisition batch and recreate exactly the loss reserved admission exists to prevent. A plain prefix truncation was rejected too, because it can alias two physical channels onto one row. The durable form keeps a UTF-8-safe prefix and appends the full SHA-256 of the exact emitted label, so it fits every durable reader boundary and still identifies the channel uniquely. Finding 2 — the reader called an incomplete result completeA row committed for a new undescribed label after Controls, run at this head, each restored byte-identically
An earlier attempt at the second control silently failed to apply its mutation and reported a pass; that pass was discarded rather than read as evidence, and the control was re-run against the exact call site. No test was weakenedOne assertion changed in Local evidence at this head
Written with AI assistance. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41e14293a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| owned = _own_live_reading(reading) | ||
| bounded_channel = _durable_unbound_channel_label(owned.channel) | ||
| if bounded_channel != owned.channel: | ||
| owned = replace(owned, channel=bounded_channel) |
There was a problem hiding this comment.
When an undescribed reading has a 257–1024-byte instrument_id or a 65–1024-byte unit, _own_live_reading accepts it and this branch shortens only channel, so the live writer commits the oversized field. _read_sqlite_bounded still limits instruments to 256 bytes and units to 64 bytes; I reproduced both cases with a valid described reading beside the unknown one, and each bounded read returned zero rows with INVALID_ROW, quarantining the good measurement too. Normalize or otherwise reconcile every admitted field with the durable reader grammar, not only the channel label.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
| if len(encoded) <= MAX_PERSISTED_READING_ID_BYTES: | ||
| return label | ||
| suffix = f"~sha256:{hashlib.sha256(encoded).hexdigest()}" |
There was a problem hiding this comment.
When one valid short label exactly equals the generated prefix~sha256:<digest> form of an overlong label, the short label passes through unchanged while the long label maps onto it. I reproduced two such undescribed readings from the same instrument and timestamp: both rows committed under the same durable channel, and the bounded collector returned only the later value because their identity keys were identical. Reserve or escape the encoded namespace so distinct emitted labels cannot collapse onto one channel.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
|
@codex review exact head 42163b9 Please review this exact commit. The laboratory claim is that one undescribed channel must not destroy or hide the described readings beside it. This head keeps distinct unbound physical channels distinct through SQLite, archive union, and CSV history; bounds instrument, channel, and unit text on both write paths; preserves already-bounded durable labels during upgrade; normalizes only oversized legacy reserved rows; and prevents an empty or malformed descriptor hash from silently collapsing different channels. Local exact-head evidence: 38 focused storage tests passed. Read-only Ruff check, Ruff format check, and git diff check passed. Three production mutations were asserted, each made its named regression fail, and each file was restored byte-for-byte: restoring the empty-hash collapse, bypassing legacy oversized-field normalization, and removing the typed non-string guard. A separate cold-context GLM-5.3 review of this exact commit returned CLEAN. Please bind any verdict or finding to the full SHA above. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42163b9441
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if descriptor_hash == _UNBOUND_DESCRIPTOR_HASH: | ||
| instrument, raw_channel, unit = normalize_persisted_unbound_reading_fields( | ||
| instrument, | ||
| raw_channel, | ||
| unit, | ||
| ) |
There was a problem hiding this comment.
When upgrading a database containing a pre-contract reserved row whose channel exceeds 256 UTF-8 bytes, this normalization is never reached: _discover_sqlite_channels first passes the raw channel to _bounded_text at lines 1306-1310 and rejects the entire source as INVALID_ROW; _discover_parquet_channels has the same ordering. I reproduced the hot case with a valid described neighbor, and the bounded result returned zero rows. Discovery and materialization need to retain a mapping from the raw oversized reserved label to its normalized identity so the row can be selected without hiding the described readings beside it.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
| digest = base64.urlsafe_b64encode(hashlib.sha256(encoded).digest()).rstrip(b"=").decode("ascii") | ||
| header = f"{_DURABLE_UNBOUND_TEXT_NAMESPACE}{digest}:" | ||
| prefix_budget = maximum - len(header.encode("ascii")) | ||
| if prefix_budget < 0: | ||
| raise AssertionError("durable unbound text bound cannot carry the reserved namespace") | ||
| visible_prefix = encoded[:prefix_budget].decode("utf-8", errors="ignore") | ||
| return header + visible_prefix |
There was a problem hiding this comment.
At exact head 42163b944199aa212f8d85ef3c6250d56fcb6f5c, a short undescribed label beginning with ~sha256b64: is encoded into another bounded label that can itself be a valid catalog channel ID; admission checks the original label before this transformation and therefore does not notice the collision. I reproduced a catalog descriptor using that generated ID and wrote its described reading beside the prefixed unknown reading at the same timestamp and instrument: both persisted, but _BoundedReadingCollector saw identical (timestamp, instrument, channel) keys and returned only the unbound value, hiding the described measurement. Reserve the generated namespace from descriptor IDs or otherwise ensure the post-encoding label cannot equal any authoritative channel ID.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
| digest = base64.urlsafe_b64encode(hashlib.sha256(encoded).digest()).rstrip(b"=").decode("ascii") | ||
| header = f"{_DURABLE_UNBOUND_TEXT_NAMESPACE}{digest}:" | ||
| prefix_budget = maximum - len(header.encode("ascii")) | ||
| if prefix_budget < 0: | ||
| raise AssertionError("durable unbound text bound cannot carry the reserved namespace") | ||
| visible_prefix = encoded[:prefix_budget].decode("utf-8", errors="ignore") | ||
| return header + visible_prefix |
There was a problem hiding this comment.
When an earlier deployment has already persisted an oversized unknown channel, parent 41e14293a229644abcddd43dbe7cdf316db752b4 stores it as the UTF-8-safe prefix plus ~sha256:<hex>, whereas exact head 42163b944199aa212f8d85ef3c6250d56fcb6f5c stores the same emitted label as ~sha256b64:<digest>:<prefix>. The upgrade reader deliberately preserves the already-bounded old spelling, so after restart the same physical channel acquires a second durable identity; bounded history, archive union, and CSV then split one sensor's time series across two channel labels. Keep new writes compatible with the prior durable encoding or migrate and alias existing rows atomically so identity remains stable across the upgrade.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
|
@codex review Please review exact head This head closes the current durable-identity findings without changing the described-channel contract. It preserves the deployed hexadecimal identity for long unbound channel labels. It normalizes pre-contract oversized reserved labels before bounded hot and cold reads. It also keeps generated identities outside the authoritative descriptor-ID grammar. I verified the central claim independently. I kept the new tests and restored only The pull request remains a draft. Hosted CI and the exact-head Codex verdict are still open gates. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db9c38a09c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| unbound_query_channels=( | ||
| unbound_query_channels_by_source.get(source.token, {}) if explicit_channels is None else {} | ||
| ), |
There was a problem hiding this comment.
When channels is explicit, this branch discards the normalized-to-raw mapping, so a pre-contract oversized reserved label can be found by auto-discovery but not fetched again using the normalized channel it returned. Fresh evidence at this head is that I reproduced a hot source returning the legacy row for channels=None but returning zero rows with complete=True for channels=(normalized_label,); the cold-history path in sqlite_writer.py also uses explicit single-channel queries, so it can silently omit the archived portion of that identity. Apply legacy normalization/mapping to explicit SQLite and Parquet reads as well.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
| if len(channel.encode("utf-8")) <= MAX_PERSISTED_READING_ID_BYTES: | ||
| return channel | ||
| return _durable_unbound_channel_text(channel) |
There was a problem hiding this comment.
If an existing unbound channel is a valid short label ending in ~sha256:<64 hex characters>, prior versions persisted that literal unchanged, but new admission now hashes it again while this upgrade normalizer preserves the old bounded spelling. Fresh evidence beyond the earlier namespace-collision finding is that the corrected head therefore returns the same physical channel under two identities before and after restart; migrate or alias legacy literal namespace occupants so the collision fix does not split their history.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
| if len(unbound_channels) <= _MAX_MATERIALIZED_UNBOUND_CHANNELS: | ||
| unbound_channels.setdefault(channel, raw_channel) |
There was a problem hiding this comment.
When an upgrade occurs during a day that already contains a pre-contract oversized reserved label, that source can contain both the old raw spelling and the new bounded spelling for the same channel. This setdefault retains only one raw spelling, and materialization queries only that value; I reproduced the exact-head hot path with both rows in one daily database and the auto-discovered bounded read examined and returned only the older row, hiding the post-upgrade measurement. The rotated Parquet discovery uses the same one-to-one map, so retain and query every persisted spelling that normalizes to the selected identity.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
|
@codex review this pull request at exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33e345eaf9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if encoded_length <= MAX_PERSISTED_READING_ID_BYTES: | ||
| return channel |
There was a problem hiding this comment.
When a pre-upgrade unbound label is 129–256 UTF-8 bytes and ends in ~sha256:<64 hex>, this branch preserves the old literal spelling, while current admission passes the same label through _durable_unbound_channel_text and hashes it again. Fresh evidence at this exact head is that a 132-byte label remains unchanged here but receives a distinct 204-byte identity on its next write, so one physical channel is still split across pre- and post-upgrade history; the 128-byte test covers only authoritative-width labels even though live unbound admission accepts wider labels. Alias or migrate these wider legacy literals as well.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
| for channel in channels: | ||
| query_channel_set = tuple(sorted(unbound_query_channels.get(channel, {channel}))) | ||
| query_placeholders = ",".join("?" for _ in query_channel_set) |
There was a problem hiding this comment.
When an explicit request names a valid authoritative channel that itself ends in the reserved suffix and the source also has a pre-upgrade unbound row under that raw spelling, discovery normalizes the unbound row to its escaped identity and therefore does not add it to this requested channel's alias set, but the {channel} fallback still queries the raw spelling. Materialization then returns the escaped unbound channel even though it was not requested; the analogous Parquet fallback has the same behavior, and descriptor-specific history can become incomplete because an unrelated unbound identity entered its result. Apply the explicit-channel predicate after reserved-row normalization, not only to the persisted spelling.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
| channel in unbound_channels | ||
| or len(unbound_channels) <= _MAX_MATERIALIZED_UNBOUND_CHANNELS | ||
| ): | ||
| unbound_channels.setdefault(channel, set()).add(raw_channel) |
There was a problem hiding this comment.
When a malformed or manually restored source contains reserved rows with very large channel strings, normalization bounds only the returned identity while this set retains each complete raw_channel across discovery pages and row groups. SQLite permits cells up to the reader's 1 MiB connection limit, and Parquet permits a raw label approaching the per-batch 32 MiB cap; retaining 33 distinct labels can therefore consume tens of MiB on the hot path or roughly a GiB on the cold path outside max_retained_bytes, defeating the hard-bounded query contract. Reject raw spellings beyond the valid pre-contract live grammar or charge their aggregate bytes to an explicit cap before retaining them.
Useful? React with 👍 / 👎.
| channel_id = self._bindings.get((owned.instrument_id, owned.channel)) | ||
| if channel_id is None: | ||
| if any(emitted_channel == owned.channel for _, emitted_channel in self._bindings): | ||
| canonical_descriptor = self._catalog.by_channel_id.get(owned.channel) |
There was a problem hiding this comment.
When an otherwise undescribed driver channel is literally named cryodaq.unbound, this lookup finds the synthetic persistence descriptor that was appended to _catalog and classifies the reading as an instrument/binding mismatch instead of absent. admit consequently refuses the entire live acquisition batch before any database file is created; I reproduced the same whole-batch refusal on the immediate writer path because its persistence catalog also exposes this synthetic entry to ordinary resolution. Keep the reserved persistence entry outside normal emitted/canonical lookup so this valid unknown spelling receives unbound admission like every other catalog gap.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
|
@codex review this pull request at head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63493ab383
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # bound belongs. | ||
| admitted_is_unbound = is_reserved_descriptor(item.descriptor) | ||
| if admitted_is_unbound: | ||
| stable = reading |
There was a problem hiding this comment.
When an undescribed reading has a 65–1024-byte unit or a 257–1024-byte instrument ID, admission has already normalized that field in item.reading, so retaining this reading makes the commit receipt differ from the original batch. The production scheduler permits channel canonicalization but requires instrument IDs and units to match exactly at scheduler.py:1453-1457; it therefore treats the successful commit as ambiguous, publishes none of the batch, and increments the instrument error count. Fresh evidence beyond the earlier durable-field-bound finding is a reproduction through Scheduler._process_readings at this head where the row reached SQLite while the broker remained empty. Teach receipt comparison about authenticated normalization or preserve the original receipt fields separately.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
| if requested_unbound: | ||
| for source in sources: |
There was a problem hiding this comment.
When an explicit query names a generated or suffix-form channel, this new alias-discovery path invokes _discover_sqlite_channels, whose legacy-timestamp probe scans the entire readings table rather than the requested aliases. An unrelated channel with a text timestamp consequently makes the requested valid channel return zero rows with LEGACY_TIMESTAMP_UNSUPPORTED; ordinary explicit queries scope that check to selected channels. Restrict the discovery probe to candidate aliases or defer it to the channel-scoped materialization step.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
| snapshot = snapshot_catalog(catalog) | ||
| if _carries_reserved_entry(snapshot): | ||
| return snapshot | ||
| return snapshot_catalog(ChannelCatalog([*snapshot.descriptors, unbound_channel_descriptor()])) |
There was a problem hiding this comment.
When a valid catalog already contains MAX_CATALOG_DESCRIPTORS entries, appending the reserved descriptor creates a 4097th entry and makes writer construction fail with ChannelDescriptorError: descriptors exceeds 4096 descriptors; the live catalog constructor has the same failure when it performs its second append. Reserve capacity without reducing the documented caller limit, or keep the persistence-only descriptor outside the bounded caller catalog.
Useful? React with 👍 / 👎.
| if _carries_reserved_entry(snapshot): | ||
| return snapshot |
There was a problem hiding this comment.
When a plain ChannelCatalog contains a noncanonical descriptor whose channel ID is cryodaq.unbound, this shortcut leaves the canonical reserved hash absent even though SQLiteWriter records it as the hash for undescribed readings. Any other undescribed reading is consequently assigned a foreign-key reference that was never installed, so the insert aborts the entire batch; I reproduced this through write_immediate with IntegrityError: FOREIGN KEY constraint failed. Validate the carried entry exactly as LiveChannelDescriptorCatalog does instead of treating channel-ID equality as sufficient.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
| if now - self._unbound_window_start >= _UNBOUND_CHANNEL_LOG_INTERVAL_S: | ||
| self._close_unbound_window(now) |
There was a problem hiding this comment.
When a finite burst contains more than eight distinct undescribed channels and no later unbound reading arrives after the five-minute boundary, this is the only place that closes the accounting window, while stop() never flushes it. Channels beyond the named-line cap are therefore neither named nor covered by the promised aggregate warning; I reproduced a nine-channel batch followed by writer.stop(), which emitted eight named warnings, no mention of the ninth channel, and no aggregate. Flush a nonempty window during shutdown or close it independently of a future unbound write.
Useful? React with 👍 / 👎.
| ) | ||
| else: | ||
| descriptor_hash = self._descriptor_hash_or_none(r, unbound) | ||
| if descriptor_hash == self._unbound_descriptor_hash: |
There was a problem hiding this comment.
When no descriptor catalog is configured, both descriptor_hash and self._unbound_descriptor_hash are None, so this equality sends every catalogless legacy reading through the reserved-field codec. Values in its generated namespace are consequently renamed even though their rows retain NULL pre-catalog provenance: I reproduced an existing reader-valid 78-byte channel ending in ~sha256:<64 hex> followed by write_immediate of the same reading, and the new NULL row was stored under a different 150-byte channel. Reserved-row alias handling cannot reunify these catalogless rows, so distinguish NULL provenance and preserve or alias its previously valid spellings.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
1 participant
The defect
A channel the descriptor catalog does not describe destroyed the whole write batch.
The shipped engine gives the writer a live catalog (
src/cryodaq/engine.py:6627), so thescheduler takes
write_committedand the refusal happens at admission, inbegin_committed, before any row is built:One comprehension over the whole batch. If
bindrefuses a single reading, every readingacquired in that cycle dies with it.
Measured on that path, with a control:
The scheduler catches that exception, counts an error and publishes nothing. So the
laboratory loses every reading of every channel, on every acquisition cycle, for as long
as the catalog gap lasts — and the gap is ordinary: a re-wired sensor, or one added in the
middle of a campaign.
After this change the same batch yields a receipt and both rows.
The change
bindis untouched. Every direct caller keeps its registered fail-closed behaviour. Asibling
admitis what the write path uses, and the difference is only the cost of arefusal:
The reserved entry is a real catalog entry whose meaning is this channel is not described.
It grants no canonical identity and nothing infers it from a name or a unit. Its hash is
pinned by test, because rows on disk reference it and a field edit would orphan them
silently.
It joins the catalog and not the bindings. The bindings are what the tracked manifest
declares — which emitted label belongs to which channel of which instrument. The reserved
entry declares nothing; it exists so that a reading with no declaration has a real descriptor
to reference, which the readings foreign key requires. An earlier attempt put it in both, and
the manifest guards correctly reported that the live catalog was claiming a channel the
manifest never declared. Correcting that also removed the need to filter it out of
instrument_ids, so the design got smaller.A reading admitted against the reserved entry keeps the label the instrument emitted. A
described reading adopts the canonical channel id, which is what makes the row, the receipt
entry and the descriptor agree. This one has no canonical identity to adopt, and the emitted
label is the only truth left about where the value came from.
Reading it back
A row carrying the reserved identity is returned without a descriptor, and the source is
not marked failed. That second part is not a detail: the collector quarantines every row from
a failed source, so reporting at that level cost the entire rotated day — measured, with a
control, a day holding one such row returned no rows at all. The report belongs one layer up,
in
DescriptorReplayReader, which withholds a descriptor-less reading and marks its batchincomplete. That chain is asserted against the production function.
Three registered contracts are reversed deliberately
All three are the same case — an unknown channel — recorded in three places. Each is argued
at the line it is reversed, not edited quietly:
test_unknown_raw_label_fails_closed_not_legacy_synthesisis renamed and rewritten,not deleted. It still proves its original purpose — that no canonical identity is granted
to an unknown channel — and now also proves the row survives carrying the emitted label.
The purpose those contracts state is preserved exactly. What changed is that a refusal no
longer destroys every reading acquired in the same cycle, which their purpose never required.
What this cost, and what it changed
Nineteen tests reddened when this began. Sixteen were expectation updates, each with its
reason written at the line; three are the reversals above. No guard was weakened, and two
objections that looked like bookkeeping were not:
removed;
correction simplified the design.
The repository-wide spelling sweep then found four membership tests over identity in my own
code. It was right, and that guard had already caught me once in this work. Selecting by
looking a spelling up in a mapping is how a channel comes to be identified by its name
instead of by its declaration; the code now compares opaque values for exact equality.
Verification
tests/storage,tests/channels, the spelling sweep and the F35end-to-end module.
reserved entry's presence, the reader's recognition, and the counting rules each redden
exactly their own tests.
patches the formatter proposed were applied by hand after reading them — all three were my
own insertion mistakes, which a blind mutating run would have hidden.
Stated plainly
An earlier head on this branch was pushed with the documentation gate red. The script
that pushed it printed the failure and continued, because nothing checked the result. The
push tool now runs the gate itself and refuses on failure.
An earlier version of this description said the defect was on the ordinary acquisition path
via
write_immediate. That was wrong: I read a search hit atengine.py:6904, whichconstructs the sensor diagnostics rather than the writer, and built on it. The row-level
handler from that attempt remains as defence in depth for the tools and tests that use the
legacy API.
This change was prepared with AI assistance (Claude).