fix(compat): the floor was already in the matrix, and I wrote a different number - #19
Conversation
…rent number `MIN_NODE_VERSION` shipped as 0.5.190 in PR #17. It should be 0.5.168, and the repo has said so since 0.5.185. I took CIRISServer#497's `ciris-client>=0.5.190,<0.6` and used it here. That range answers WHICH CLIENTS THE SERVER SUPPORTS. This constant answers WHICH NODES THIS CLIENT SUPPORTS — the opposite direction — and `compat/matrix.json` has carried that as `node_min: 0.5.168` in every row since 0.5.185, each with a written reason. At 0.5.190 the client would have shown "update recommended" against every node between 0.5.168 and 0.5.190: nodes the matrix calls supported. That is the permanent nag the decoupling existed to remove, moved to a different boundary. I also told this repo's owner the floor was "not earned the way the server earned theirs". That was wrong in a more useful way than it looked. The floor IS earned — stated, reasoned and append-only in the matrix. What was missing was never the evidence; it was anything keeping the Kotlin copy equal to it. I asserted a constant beside a file that already held the answer. `CompatibilityFloorMatchesMatrixTest` reads `compat/matrix.json`, finds the row for `CLIENT_VERSION`, and fails if `node_min` and `MIN_NODE_VERSION` disagree. It reads the matrix rather than restating it: a test hard-coding 0.5.168 would prove the constant equals a literal in a test file, which is not the claim. The release gate already refuses a version with no matrix row, so the row and the constant now move together or the build stops. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EGE52kPzjGFiPzGcs63ZC1
![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: 4929dc4d6c
ℹ️ 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".
| * anything keeping the two copies equal. That is the test. | ||
| */ | ||
| const val MIN_NODE_VERSION: String = "0.5.190" | ||
| const val MIN_NODE_VERSION: String = "0.5.168" |
There was a problem hiding this comment.
Lowering the default floor to 0.5.168 makes the existing ClientModeTest.version_mismatch_fires_on_a_node_below_the_floor assertion assertTrue(isVersionMismatch("0.5.175", "0.5.176")) return false, because 0.5.175 is now above the floor. The inspected .github/workflows/build.yml runs :shared:desktopTest, which includes this common test, so the required desktop test job will fail until the test uses a node version below 0.5.168 (and updates its stale explanation).
Useful? React with 👍 / 👎.
Correcting MIN_NODE_VERSION to 0.5.168 left three cases asserting the behaviour of the number I removed. Codex named the first; the suite found the other two the moment the constant moved, which is the drift test doing its job on its first run. version_mismatch_fires_on_a_node_below_the_floor 0.5.175 is ABOVE 0.5.168 version_mismatch_ignores_the_v_prefix 0.5.176 likewise a_node_that_declares_nothing_... 0.5.186 likewise Each asserted the client NAGS at a node the compatibility matrix calls supported. Left in place they would have preserved the exact defect the change removes, inside the tests written to prove it gone — a green suite certifying the wrong floor. Below-floor cases now use 0.5.167 and 0.5.100, which are genuinely below 0.5.168, and the supported-but-older versions are asserted SILENT alongside them, so the boundary is pinned from both sides rather than only from beneath. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EGE52kPzjGFiPzGcs63ZC1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4dfbf08f81
ℹ️ 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 (File(d, "compat/matrix.json").isFile) return d | ||
| dir = d.parentFile | ||
| } | ||
| error("could not find compat/matrix.json above ${File(".").absolutePath}") |
There was a problem hiding this comment.
When client/ is built standalone using the supported -PclientVersion override, or this test is merged into the still-vendored CIRISServer/CIRISAgent copies, no ancestor is required to contain CIRISClient's root-level compat/matrix.json; this search therefore reaches error(...) and fails the required :shared:desktopTest task before any assertion runs. The inspected .github/workflows/build.yml hides the regression because its full CIRISClient checkout happens to place the matrix above client/; pass or stage the matrix explicitly, or keep this cross-repository check outside the standalone client test suite.
AGENTS.md reference: AGENTS.md:L65-L67
Useful? React with 👍 / 👎.
| val idx = text.indexOf("\"client_version\": \"$version\"") | ||
| if (idx < 0) return null | ||
| return Regex("\"node_min\"\\s*:\\s*\"([^\"]+)\"") | ||
| .find(text.substring(idx))?.groupValues?.get(1) |
There was a problem hiding this comment.
If a valid matrix row places node_min before client_version, this unbounded search starts after the row's floor and reads the first node_min from a later release instead. When that later value equals MIN_NODE_VERSION, the new consistency check passes even though the current release's row disagrees—the exact drift this test is intended to catch. Parse the matched JSON object or otherwise bound extraction to that object rather than searching the remainder of the file.
Useful? React with 👍 / 👎.
…e could not fail Two findings on the drift test, and acting on the first produced a better answer than patching it would have. IT DOES NOT BELONG IN THE CLIENT'S TEST SUITE. `client/` builds standalone with `-PclientVersion`, and this tree is vendored into two other repos — none of which are required to have `compat/` above them. A Kotlin test that walks up looking for `compat/matrix.json` fails the whole `:shared:desktopTest` task there, for a reason that has nothing to do with the client. CI hid it, because a full checkout happens to place the matrix above `client/` (Codex, PR #19). The matrix is the thing being compared against, so the comparison now lives beside the matrix in `compat/validate.py`, which already runs as a release gate and where the file is guaranteed to exist. THAT ALSO DISSOLVED THE SECOND FINDING rather than fixing it. The Kotlin version scanned text after the matched `client_version`, so a row that ever placed `node_min` first would send the scan into the NEXT release's floor — and if that value happened to equal the constant, the check would pass WHILE DRIFTING. A gate silently failing to fail. `validate.py` already parses the row as JSON, so key order cannot mean anything. AND THE FIRST VERSION OF THIS GATE COULD NOT FIRE AT ALL. I appended the call next to the wrong `return problems` — it landed inside `check_kotlin_floor` itself, a recursive line nothing ever reached — and referenced a `REPO` constant that exists in `check_pins.py` and not here. Three mutations passed, including deleting `MIN_NODE_VERSION` outright. I only know because I ran them; the gate reported OK on every one, which is precisely the failure this session keeps naming, committed by the person naming it. Mutation-tested after the fix: constant drifts to 0.5.190 FAIL, naming both values and which is source constant deleted FAIL, "parsed no MIN_NODE_VERSION" node_min reordered before PASS — JSON parsing does not care about key client_version in the row order, which was the point Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EGE52kPzjGFiPzGcs63ZC1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e862bbe373
ℹ️ 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".
| MIN_NODE_RE = re.compile( | ||
| r'const val MIN_NODE_VERSION:\s*String\s*=\s*"([^"]+)"' | ||
| ) |
There was a problem hiding this comment.
When ClientMode.kt retains the old declaration in a comment before changing the live constant, MIN_NODE_RE.search captures the commented value and the compatibility check passes despite the compiled MIN_NODE_VERSION disagreeing with the matrix. I reproduced this with // const val MIN_NODE_VERSION: String = "0.5.168" followed by a live 9.9.9 declaration; validate() returned no problems, so this new drift gate can report green while the banner uses the wrong floor. Anchor the match to an uncommented declaration or strip comments before parsing.
AGENTS.md reference: AGENTS.md:L44-L44
Useful? React with 👍 / 👎.
| row = next((r for r in rows if r.get("client_version") == version), None) | ||
| if row is None: | ||
| return [] # the "exactly one row for VERSION" check already reports this | ||
| if m.group(1) != row["node_min"]: |
There was a problem hiding this comment.
When the current-version row is missing node_min, the main loop already records the missing required field, but this unconditional lookup then raises KeyError instead of returning that problem; similarly, a non-object row that the loop can reject reaches r.get(...) and raises AttributeError. Both the CI command and the imported compat-matrix gate therefore produce a traceback/error rather than the validator's actionable failure list for malformed matrices. Skip the cross-check unless a validated current row and string node_min are available.
AGENTS.md reference: AGENTS.md:L52-L52
Useful? React with 👍 / 👎.
| MIN_NODE_RE = re.compile( | ||
| r'const val MIN_NODE_VERSION:\s*String\s*=\s*"([^"]+)"' | ||
| ) | ||
| CLIENT_MODE = "client/shared/src/commonMain/kotlin/ai/ciris/mobile/shared/models/ClientMode.kt" |
There was a problem hiding this comment.
When python -m readiness --client-tree <CIRISServer-or-CIRISAgent>/client grades one of the other supported trees, compat_matrix() still calls validate() with the CIRISClient repository root and this hard-coded path reads CIRISClient's own client/. A stale or divergent MIN_NODE_VERSION in the selected consumer tree therefore reports green while the board labels that consumer tree as its subject. Pass client_tree(ctx) into the floor check and select the matrix row for that tree's client version so this gate actually detects cross-tree drift.
AGENTS.md reference: AGENTS.md:L49-L49
Useful? React with 👍 / 👎.
| f"exactly one row must match VERSION ({version}); found {len(matches)} — " | ||
| f"a release without its matrix row does not merge (FSD §6)" | ||
| ) | ||
| problems.extend(check_kotlin_floor(repo_root, rows, version)) |
There was a problem hiding this comment.
Adding check_kotlin_floor here changes the imported compat-matrix readiness gate from checking only the matrix row to also checking the Kotlin runtime constant, but its decorator question, MISSION.md §3 entry, and README table still describe only the former contract. Consequently python -m readiness gates and the documented board omit a newly enforced failure condition; update both required tables and the registered question in this commit.
AGENTS.md reference: AGENTS.md:L56-L56
Useful? React with 👍 / 👎.
| * anything keeping the two copies equal. That is the test. | ||
| */ | ||
| const val MIN_NODE_VERSION: String = "0.5.190" | ||
| const val MIN_NODE_VERSION: String = "0.5.168" |
There was a problem hiding this comment.
This commit changes ClientMode.kt and two test files under client/, but its client/VENDORING.md edit only refreshes the state digest and adds no §3 delta row explaining those departures. That leaves the modified client files without the provenance entry required for every vendored-file change; add the ledger row, or reconcile the applicable repository rule in the same commit if retiring §3 was intended.
AGENTS.md reference: AGENTS.md:L26-L26
Useful? React with 👍 / 👎.
…tract was unwritten Five findings on the gate I added, and two of them say the gate lied. A COMMENT DEFEATED IT. `MIN_NODE_RE` matched the first occurrence anywhere in the file, so an old value left commented above a changed live one was what the gate compared — green while the compiled constant disagreed with the matrix. Codex reproduced it. That is the second time in two commits that this gate reported OK on exactly the drift it exists to catch. Now anchored to a live declaration at line start, and mutation-tested with the decoy in place. IT THREW INSTEAD OF REPORTING. A row missing `node_min`, or a non-object row, reached past the loop that had already recorded the problem and raised. Both CI and the imported readiness gate got a traceback instead of the validator's actionable list. Now it defers: the loop's finding is the answer. IT GRADED THE WRONG TREE. `readiness --client-tree <CIRISServer>/client` points the board at a consumer's vendored copy, and the path was hardcoded to ours — so a drifted constant in the tree actually under grade reported green while the board named that tree as its subject. `validate()` now takes the tree. ITS CONTRACT WAS UNWRITTEN. Adding this check changed what `compat-matrix` enforces, while the decorator question, MISSION.md §3 and the README table still described only the row check. A gate whose stated contract omits a condition it enforces under-reports what it checked, which is the same defect as not checking. All three updated here, in the commit that changed the behaviour. AND THE FIFTH IS A STALE RULE, RECONCILED RATHER THAN OBEYED. Codex asked for a `VENDORING.md` §3 delta row for the changed client files. §3 was RETIRED at the three-way merge — git history is the declaration, the §1 digest is the seal — but AGENTS.md still instructed every file under `client/` to be byte-identical to CIRISAgent@6083bdf or carry a §3 row, and claimed `check_vendoring.py` asserts that. It does not and never has: it asserts the digest and the never-vendor classes. The finding offered reconciliation as the alternative and that is the correct branch — the rule was describing a gate that does not exist, so the instruction is fixed rather than a row added to a retired section. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EGE52kPzjGFiPzGcs63ZC1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ffa3a7bbe
ℹ️ 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".
| r'^(?!\s*(?://|\*|/\*))\s*(?:internal\s+|public\s+)?const val MIN_NODE_VERSION' | ||
| r':\s*String\s*=\s*"([^"]+)"', |
There was a problem hiding this comment.
When an old declaration is retained inside a normal multiline comment whose interior lines do not begin with *, this negative lookahead treats that declaration as live and captures its value instead of the compiled constant below it. I reproduced this with /* old floor\nconst val MIN_NODE_VERSION: String = "0.5.168"\n*/ followed by a live 9.9.9 declaration; the final regex still returns 0.5.168, so the drift check can report green. This is fresh evidence beyond the earlier // case because the revised regex only examines the matched line and does not track block-comment spans; strip comments before matching or exclude declarations inside those spans.
AGENTS.md reference: AGENTS.md:L52-L52
Useful? React with 👍 / 👎.
| row = next( | ||
| (r for r in rows | ||
| if isinstance(r, dict) and r.get("client_version") == version), | ||
| None, |
There was a problem hiding this comment.
When --client-tree grades a consumer copy whose client version belongs to a different matrix row and the floors differ, this still searches using the CIRISClient repository's VERSION; a correct stale consumer can therefore fail against a newer release's floor, while a wrong consumer constant can pass if it happens to equal that newer floor. Fresh evidence after the earlier report is that the final implementation now passes the selected tree but still compares client_version to the root-derived version; derive the selected tree's version using the same two-layout logic as version_alignment before choosing its row.
AGENTS.md reference: AGENTS.md:L49-L50
Useful? React with 👍 / 👎.
…must match the graded tree Two more on the same gate. The line-anchored exclusion caught `//` and a `*` continuation, but a declaration parked inside `/* ... */` need not start its line with anything. Block comments are stripped before matching now, and the decoy is mutation-tested in that form as well as the line form. And the row was selected by THIS repo's VERSION even when `--client-tree` pointed readiness at a consumer's vendored copy, which can sit at a different release — comparing that tree's constant against a row describing a release it is not. The graded tree's own generated `ClientVersion.kt` answers it; absent one, it falls back to ours rather than inventing a version. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EGE52kPzjGFiPzGcs63ZC1
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
MIN_NODE_VERSIONshipped as 0.5.190 in #17. It should be 0.5.168, and the repo has said so since 0.5.185.I took CIRISServer#497's
ciris-client>=0.5.190,<0.6and used it here. That range answers which clients the server supports. This constant answers which nodes this client supports — the opposite direction — andcompat/matrix.jsonhas carried it asnode_min: 0.5.168in every row since 0.5.185, each with a written reason.At 0.5.190 the client would show "update recommended" against every node between 0.5.168 and 0.5.190 — nodes the matrix calls supported. That's the permanent nag the decoupling existed to remove, relocated to a different boundary.
The part worth correcting out loud
I told @CIRISAI the floor was "not earned the way the server earned theirs," and offered to build a gate to earn it. That was wrong in a more useful way than it looked. The floor is earned — stated, reasoned, append-only in the matrix, one row per release. What was missing was never the evidence. It was anything keeping the Kotlin copy equal to it.
I asserted a constant beside a file that already held the answer.
The check
CompatibilityFloorMatchesMatrixTestreadscompat/matrix.json, finds the row forCLIENT_VERSION, and fails ifnode_minandMIN_NODE_VERSIONdisagree. It reads the matrix rather than restating it — a test hard-coding0.5.168would prove the constant equals a literal in a test file, which isn't the claim.The release gate already refuses a version with no matrix row, so the row and the constant now move together or the build stops.
🤖 Generated with Claude Code
https://claude.ai/code/session_01EGE52kPzjGFiPzGcs63ZC1