[spark-compete] Narrow bare except in read_json + read_toml helpers in system_map.py to their exhaustive parse + OS error classes#532
Conversation
QA write-up — for reviewer eyesTL;DR. Beforedef read_json(path: Path) -> tuple[Any | None, str | None]:
if not path.exists():
return None, "missing"
try:
return json.loads(path.read_text(encoding="utf-8-sig")), None
except Exception as exc:
return None, f"read_json_failed: {type(exc).__name__}: {exc}"
def read_toml(path: Path) -> tuple[dict[str, Any] | None, str | None]:
if not path.exists():
return None, "missing"
try:
return tomllib.loads(path.read_text(encoding="utf-8")), None
except Exception as exc:
return None, f"read_toml_failed: {type(exc).__name__}: {exc}"If a typo lands in either try body ( After except (json.JSONDecodeError, OSError) as exc:
return None, f"read_json_failed: {type(exc).__name__}: {exc}" except (tomllib.TOMLDecodeError, OSError) as exc:
return None, f"read_toml_failed: {type(exc).__name__}: {exc}"The diagnostic is preserved for the documented failure conditions (malformed JSON, malformed TOML, unreadable file). Programming bugs propagate so the operator gets a real traceback during compile. SmokeWhat didn't changeOne file, two lines changed. Except body byte-identical pre/post for both helpers. The missing-file fast-path and the valid-file success-path are unchanged. The diagnostic string still names the exception class and message; it just can no longer name a class that has nothing to do with file IO or parsing. Pattern-match: same shape as our own #163 (json-load helpers in spark-intelligence-builder) and #526 (sqlite-introspection helpers in the same file). |
…to (json.JSONDecodeError, OSError) / (tomllib.TOMLDecodeError, OSError) In src/spark_cli/system_map.py, read_json (L301) and read_toml (L310) are paired helpers that load a config-style file from disk and return (parsed_payload, error_string). Both catch bare 'except Exception:' and serialize the exception type into the failure-detail string. The bare Exception folds any unrelated runtime error (AttributeError on a typo, NameError from a refactor, KeyboardInterrupt) into a synthetic 'read_json_failed: ...' / 'read_toml_failed: ...' diagnostic that lies to the operator about what actually broke -- it looks like the file was malformed when really the surrounding code has a bug. Narrow each to its exhaustive disk + parse class: read_json: (json.JSONDecodeError, OSError) read_toml: (tomllib.TOMLDecodeError, OSError) Smoke: corrupt/valid/missing trio confirmed for both helpers, success and documented-failure paths byte-identical pre/post.
8e567a9 to
be716ad
Compare
|
Thanks, this was useful. We maintainer-adopted this exception-boundary improvement in #609 and merged it into What shipped:
Verification on the adopted PR:
Source credit is recorded for this PR, and #609 itself should not receive participant credit. Points are still gated because this PR body had Closing this PR as adopted into #609. No code action is needed unless you believe #609 missed part of the issue. |
|
R22/R23 final scoring update: this PR did receive final R22/R23 public leaderboard credit. Earlier gate, credit-review, or points-lock wording described the pre-final review state, not a final rejection or permanent zero. Final R22/R23 credit for this PR: 48 points. Thanks for the contribution. |
{
"schema": "spark-compete-hotfix-v1",
"event": "spark-compete-first-event",
"submission_mode": "public_repo_pr",
"submission_target_url": "#532",
"team": {
"name": "SparkThisUp",
"members": ["ValHallaBuilder", "Baz707", "DanFireDash"],
"github_accounts": ["4gjnbzb4zf-sudo"],
"llm_device_holder": "ValHallaBuilder",
"device_holder_github": "4gjnbzb4zf-sudo"
},
"target_repo": {
"id": "vibeforge1111/spark-cli",
"source": "https://github.com/vibeforge1111/spark-cli",
"owner_surface": "spark-cli"
},
"issue": {
"type": "bug",
"severity": "low",
"title": "read_json and read_toml helpers in system_map.py lie about which class of failure triggered the diagnostic when the bare Exception catches a programming bug",
"actual_behavior": "src/spark_cli/system_map.py exposes two paired helpers used throughout the system-map compile pipeline:
read_json(path)(L301) andread_toml(path)(L310). Both wrap a single load operation inexcept Exception as exc: return None, f'read_json_failed: {type(exc).__name__}: {exc}'(or the toml variant). The diagnostic string asserts the file failed to load -- but the bare Exception folds any unrelated runtime error (AttributeError on a typo, NameError from a refactor, KeyboardInterrupt, MemoryError) into the sameread_json_failed:/read_toml_failed:synthetic detail. The operator sees a 'failed to read' line in their system-map output and assumes the file is malformed; the real cause is a programming bug in the helper.","expected_behavior": "The diagnostic is preserved for the documented failure conditions (malformed JSON / malformed TOML / unreadable file) and only those. Programming bugs propagate naturally so the operator sees them as a real traceback during compile.",
"repro_steps": [
"gh pr checkout ",
"cd into a working copy and:
PYTHONPATH=src python -c \"from spark_cli.system_map import read_json, read_toml; from pathlib import Path; import tempfile, os; d=Path(tempfile.mkdtemp()); (d/'t.json').write_text('not json'); print(read_json(d/'t.json'))\"-> still returns(None, 'read_json_failed: JSONDecodeError: ...')exactly as before","Same trio for read_toml: missing file ->
(None, 'missing'), malformed ->(None, 'read_toml_failed: TOMLDecodeError: ...'), valid ->(payload, None)","Inverse: edit a typo into the read_json try body (e.g.
path.read_textZZZ) -- before the change, the AttributeError gets repackaged asread_json_failed: AttributeError: .... After the change, the AttributeError propagates so the operator sees the real bug instead of a misleading 'malformed JSON' line."],
"affected_workflow": "system_map.py is the system-map compile module driven by
spark report system-mapand the underlying compile pipeline. read_json and read_toml are paired helpers consumed across the file (manifest probes, repo-board snapshots, capability cards, etc.). When the diagnostic lies about which class of failure happened, downstream compile-failure triage points the operator at the wrong file or assumes data corruption when the actual cause is a code bug in the helper."},
"evidence": {
"safe_links_only": true,
"before_after_proof": "Two paired try/except patterns. Before:
except Exception as exc: return None, f'read_json_failed: {type(exc).__name__}: {exc}'. After:except (json.JSONDecodeError, OSError) as exc: return None, f'read_json_failed: {type(exc).__name__}: {exc}'. read_toml's narrow tuple is(tomllib.TOMLDecodeError, OSError). The except body is byte-identical -- only the catch tuple changes. Each try body is one line:json.loads(path.read_text(encoding=...))ortomllib.loads(path.read_text(encoding=...)). JSONDecodeError / TOMLDecodeError are the parse-time class for each library; OSError covers FileNotFoundError, PermissionError, IsADirectoryError, UnicodeDecodeError reasons for read_text to fail. Diff: 2 insertions, 2 deletions, one file.","links": ["https://github.com//pull/532"],
"forbidden": ["pdf","zip","exe","unknown downloads","shortened links","archives","binaries","tokens","browser cookies","wallet material","raw logs","raw conversations","raw memory","raw patches","private repo maps","private scoring details"]
},
"proposed_fix": {
"approach": "Replace
except Exception as exc:withexcept (json.JSONDecodeError, OSError) as exc:in read_json, and withexcept (tomllib.TOMLDecodeError, OSError) as exc:in read_toml. Both narrow tuples are exhaustive for the single-operation try body. The except body is byte-identical -- the synthetic 'failed' diagnostic string still names the exception type and message viatype(exc).__name__: {exc}, only now it can only be one of the narrow classes, so the diagnostic is no longer a lie.","files_expected": ["src/spark_cli/system_map.py"],
"tests_or_smoke": "Manual smoke verified the missing/corrupt/valid trio for both helpers: read_json returns (None,'missing'), (None,'read_json_failed: JSONDecodeError: ...'), (payload, None); read_toml returns the analogous trio. AST parse of system_map.py is clean. Diff is 2 insertions, 2 deletions."
},
"pr": {
"branch": "sentinel/exception-narrow/system-map-read-json-toml-helpers",
"title_prefix": "[spark-compete]",
"author_github": "4gjnbzb4zf-sudo",
"body_must_include": ["packet","team","pr_author","repo","actual_behavior","expected_behavior","repro_steps","before_after_proof","tests_or_smoke","duplicate_notes","risk_notes","review_claim"],
"url": "#532"
},
"review_claim": {
"impact_claim": "low",
"evidence_types": ["redacted_terminal_excerpt"],
"duplicate_notes": "Pre-flight
gh pr list --search 'read_json read_toml narrow'returned zero hits.gh pr list --search 'system_map narrow'shows our own #526 (six different sqlite-introspection helpers in the same file). This PR touches L306/L314, two distinct helpers from #526's L651/L941/L983/L2576/L2684/L2742. No competitor PR touches these helpers. Same shape as our own #163 (json-load helpers in spark-intelligence-builder) and #526.","risk_notes": "Local scope: one file, two lines changed. The except body is byte-identical pre/post for both helpers. The missing-file fast-path and the valid-file fast-path are unchanged. The synthetic diagnostic string is preserved for the documented failure conditions (malformed JSON, malformed TOML, unreadable file). The only observable behavior change is that programming bugs in the helpers themselves no longer get mis-attributed to file corruption -- which is the goal of the fix.",
"review_state_requested": "pr_review"
}
}