test(datagrid): assert the contracts the app actually has, not the ones it used to

[datlechin]

Ten more entries out of the quarantine file, 32 to 22, after #2377 took the first ten.

Half of them share one shape: the test holds a handle that used to steer the code and no longer does.

Cases	The dead handle
3	conn.sshConfig.enabled = true, when resolvedSSHConfig reads sshTunnelMode. The formatter produced a plain mysql:// URL and the test asserted it contained ssh://
4	reloadVersion after an edit, when the manager only bumps it on a reload
1	DatabaseType.scylladb.iconName == "cassandra-icon", when ScyllaDB has its own registry entry and its own asset
2	a read-only guard inside saveChanges, when the block lives in the execution gate

reloadVersion is a reload signal, not a change counter

reloadVersion tells the grid to throw away what it is showing and fetch again. It increments in exactly three places: clearChanges, discardChanges and configureForTable. It deliberately does not increment when an edit is recorded, because a reload there would discard the edit the user just made.

Four cases asserted the opposite. Each now pins the contract from both sides: recording does not bump it, and the reload path does.

manager.recordCellChange(...)
#expect(manager.reloadVersion == initialVersion)
manager.clearChanges()
#expect(manager.reloadVersion == initialVersion + 1)

Read-only is enforced, in one place

saveChanges carries no read-only guard, and that is correct. DefaultExecutionGate denies a write with "Safe Mode is set to read-only for this connection", and MainWindowToolbar+Validation disables Save. ExecutionGateTests.readOnlyBlocksWrites asserts exactly that, is not quarantined, and passes.

The two SaveCompletionTests cases were asserting the same rule at a layer that does not implement it, so they are gone rather than duplicated.

What is still quarantined, and why

The three remaining SaveCompletionTests cases fail earlier than the behaviour they assert: saveChanges returns at guard let scope = parent.selectedTabScope long before it clears the inout parameters, so they need a connected scope the harness does not build.

The three StructureChangeManagerUndoTests cases are an undo-grouping artifact, not drift. applyColumnDeleteUndo touches exactly one key, so a single undo clearing two pending changes can only be NSUndoManager grouping them: groupsByEvent is on, and operations called without a run loop turn between them land in one group. The app gets a turn between user actions and the test does not.

I could not close the group from the test. RunLoop.current.run(until: Date()) does not do it, and neither does a 20ms interval. Both dead ends are written into the quarantine file so the next attempt starts from a seam on the manager rather than repeating them. I removed these three from the file before proving the fix worked, which would have sent them to CI marked as passing; they are back in.

Verification

build                                          PASS
ConnectionURLFormatterSSHProfileTests + Cassandra   21 of 21
reloadVersion rewrites                         4 of 4, each confirmed by name
SaveCompletionTests                            8 pass, 3 fail (all three still quarantined)

The quarantine file records where every removed case went and why each remaining one stays.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[datlechin]

Pushed two more commits. The quarantine is now 14, down from 42 when this started, and every remaining entry has been read and carries a verified reason.

What the last batch cleared

Case	Finding
sqliteIndexes_isNoOp, sqliteIndexes_deleteIsNoOp	SQLite's curated snapshot overrides neither supportsAddIndex nor supportsDropIndex, so both default to true, which is right for an engine with CREATE INDEX
testMySQLProviderTypes, testProviderAcceptsDatabaseType	The provider keeps a cachedDialect and answers as generic SQL without one. Built with a databaseType but no dialect, they asked a generic provider for ENUM and JSONB
allMaxBytes	Unconstructible input: prefixRangeEnd takes a String, and no Swift String has 0xFF in its UTF-8, so the path it named is unreachable from that entry point
addsRequiredWhenMissing	sanitizeObject never invents a required key, and an absent one already means "nothing is required" in JSON Schema
insertQueryFromAI_appendsToExisting	git log -S found 3ab129793 fix(ai-chat): insert generated SQL into a new query tab instead of appending (#1257). The test pinned the behaviour that fix removed
addNewRowUsesNilForNoDefaults	A column with no default gets an explicit .null, not a Swift nil. An absent value and a SQL NULL are different things to the statement generator

Two invariants the tests had backwards

insertQueryFromAI is the clearer one. aiInsertReusesSelectedQueryTab is true only when the selected query tab is empty, so generated SQL takes over a blank tab and otherwise opens its own. A tab you have typed into is never rewritten. My first rewrite got this wrong too, asserting the query became the generated one; reading the property showed the real contract, and both directions are now pinned, including an empty-tab case nothing covered.

reloadVersion, in the previous commit, is the same shape: it does not increment on an edit because a grid reload would discard the edit you just made.

In both, "fix the code to match the test" would have introduced data loss.

The 14 left, by cause

Count	Cause
5	NSUndoManager grouping. Operations called without a run loop turn land in one group and one undo reverts all of them. RunLoop.current.run(until: Date()) does not close it and neither does a 20ms interval, so a seam on the manager is what these need
3	Environment-coupled. KeychainHelper.write genuinely upserts (SecItemAdd, then SecItemUpdate on duplicate), so these need a real login keychain and installed apps rather than a code change
3	saveChanges returns at guard let scope = parent.selectedTabScope before the behaviour they assert
2	Plugin bundles do not register in the xctest host, so the duplicate-ID check has nothing to collide with. PluginManager.loadPlugins() from the test does not fill the table either
1	The test's own schema setup through MockDatabaseDriver; comma-separated FROM is supported on purpose in SQLContextAnalyzer.fromListRegex

The four headings this file started with were mostly wrong. "Bundled .tableplugin drivers do not activate in the headless test host" covered 20 entries, and the test host is TablePro.app with all 14 bundled plugins in its Contents/PlugIns. Six distinct causes were hiding behind them.

One defect in the suite itself, reported not fixed: TestEtcdPrefixRange in EtcdHttpClientUtilityTests is a byte-for-byte copy of EtcdHttpClient.prefixRangeEnd, so those cases have never exercised the shipped function. That is documented on the helper now.

[krobchai32659-jpg]

เมื่อ เสาร์ 22 ส.ค. 2026 เวลา 1:20 PM Ngô Quốc Đạt ***@***.***> เขียนว่า:

…

Merged #2378 <#2378> into main. — Reply to this email directly, view it on GitHub <#2378?email_source=notifications&email_token=B64CZPHFVLTH3NOEGI2A2SD5LE3T5A5CNFSNUABQM5UWIORPF5TWS5BNNB2WEL2JONZXKZKFOZSW45CON52GSZTJMNQXI2LPNYXTEOJYGQYDGNBRHE2DPJTSMVQXG33OVJZXKYTTMNZGSYTFMSSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#event-29840341947>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/B64CZPFA2KOG4XBBCS6ZHVL5LE3T5AVCNFSNUABGKJSXA33TNF2G64TZHMYTCMJXHA4TCMBUGQ5US43TOVSTWNJSGIYDIMRUGEYTHILWAI> . Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS <https://github.com/notifications/mobile/ios/B64CZPDXM5IUNQ3CWWQOCAT5LE3T5A5CNFSNUABQM5UWIORPF5TWS5BNNB2WEL2JONZXKZKFOZSW45CON52GSZTJMNQXI2LPNYXTEOJYGQYDGNBRHE2DPJTSMVQXG33OVJZXKYTTMNZGSYTFMSSWK5TFNZ2KUZTPN52GK4S7NFXXG> and Android <https://github.com/notifications/mobile/android/B64CZPAIUKPNAYQEX7TRDST5LE3T5A5CNFSNUABQM5UWIORPF5TWS5BNNB2WEL2JONZXKZKFOZSW45CON52GSZTJMNQXI2LPNYXTEOJYGQYDGNBRHE2DPJTSMVQXG33OVJZXKYTTMNZGSYTFMSSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>. Download it today! You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>