R7.4a second pass: fix 3 defects the first fix pass introduced, plus 5 it missed#143
Merged
Merged
Conversation
…5 it missed A user-requested re-audit aimed specifically at the R7.4a FIX PASS (rather than the original feature) found that the fix pass itself introduced new bugs and shipped two regression tests that could not fail. Root cause: the first pass fixed "coerce untyped wire input" for exactly one argument in one handler instead of applying it as a rule, so the two new code paths that same pass added each re-opened the identical hole. Introduced by the first fix pass, now fixed: - _redact() was handed the RAW wire api_key, so a non-string key raised TypeError inside an except block - where its own try cannot catch it - escaping the handler and pinning that provider's catalog at "loading" for the process lifetime, permanently disabling the Load button in every tab. Coerced at the top of load_api_models + isinstance guard in _redact. - api_catalog_state[provider] used the raw provider as a dict key, so an unhashable JSON value crashed the handler where the pre-fix flat cells could not. - That dict was written on the unsupported-provider reject path before returning, growing without bound on a client-controlled key (200 junk providers left 200 entries; now 0). Tests that could not fail, now mutation-verified: - The F1 concurrency test injected its "concurrent" write inside the mocked initialize_api, which sits BEFORE the racy read, so it passed against the very ordering it was meant to catch. Rewritten to inject inside _apply - the actual read/write window. - The F2 stale-provider guard had zero coverage; deleting it left the suite green. Now covered. Missed by the first review entirely: - The write-only-key design left an already-configured user with a blank key field, and load_api_models had no server-side fallback, so Load ALWAYS failed for them with "API key not configured" while the same payload reported apiKeyConfigured=true. Now falls back to the stored key (still never sent to the client), with a clean message when genuinely unconfigured - which also closes a hole where a blank-key Load could succeed off an ambient env var and silently repoint the live provider. - The OpenAI "Base URL must not be empty" guard both legacy predecessors had was dropped on load and save. On save a persisted "" is read back verbatim (SettingsManager's default only covers a missing key), silently re-pointing the saved key at api.openai.com. Restored on both paths. - Reset API Settings wiped every provider's key on a single click, dropping legacy's "This cannot be undone" gate. Restored as the same two-step confirm the legacy island and ChatLibraryDialog already use. Verification: 2438 backend pytest (was 2430), 1066 frontend vitest, compileall/tsc/eslint/build clean, burn-down pin unchanged at 152/84/68, both rewritten tests mutation-verified, and a second live WS drive against a real backend covering all five new guard paths (isolated scratch settings file; the real ~/.graphlink/session.dat was never touched). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #142 (already merged). A re-audit aimed specifically at the fix pass rather than the original feature found that the fix pass itself introduced new defects and shipped two regression tests that could not fail.
Root cause, stated plainly: the first pass fixed "coerce untyped wire input" for exactly one argument in one handler instead of applying it as a rule — so the two brand-new code paths that same pass added (
_redact, and the per-providerapi_catalog_statedict) each re-opened the identical hole.Introduced by the first fix pass — now fixed
_redact()was handed the raw wireapi_key. A non-string key raisedTypeErrorinside anexceptblock, where its owntrycannot catch it — escaping the handler and pinning that provider's catalog at"loading"for the process lifetime, which permanently disables the Load button in every tab. Reproduced directly (TypeError: replace() argument 1 must be str, not int), then fixed by coercing at the top ofload_api_modelsplus anisinstanceguard in_redact.api_catalog_state[provider]used the raw provider as a dict key, so an unhashable JSON value crashed the handler where the pre-fix flat cells could not. Reproduced, fixed by coercion.Tests that could not fail — now mutation-verified
initialize_api, which sits before the racy read — so it passed against the very ordering it was written to catch. Rewritten to inject inside_apply(the real read/write window) and mutation-verified to fail against that ordering.Missed by the first review entirely
load_api_modelshad no server-side fallback — so Load always failed with "API key not configured" while the same payload advertisedapiKeyConfigured: true. Now falls back to the stored key (still never sent to the client), with a clean message when genuinely unconfigured. This also closes a related hole where a blank-key Load could succeed off an ambient env var and silently repoint the process-global live provider to one the user was only browsing.""is read straight back (SettingsManager's default only covers a missing key, not a stored empty string), silently re-pointing the saved key atapi.openai.comfrom then on. Restored on both paths.ChatLibraryDialogalready use.Validation
python -m compileall -q .passes~/.graphlink/session.datwas never touched)Additional Context
Process lesson worth carrying into R7.4b/R7.4c: when a review finds an input-validation bug, fix the class across every new entry point, not the single reported instance.