* fix(skill-extractor): walk all brace candidates so stray braces in prose do not swallow valid JSON
The extractor sliced from the FIRST brace to the LAST brace to recover
JSON embedded in surrounding commentary. When the model emits stray
braces before the JSON object, the slice produces invalid JSON,
json.loads raises, and the exception is swallowed -- the skill is
silently lost.
Fix: walk each brace candidate left-to-right and attempt json.loads on
each slice. The first candidate that parses successfully wins. If none
parse, json.loads on the original text raises and the existing
JSONDecodeError handler logs and returns None as before.
Tested locally -- 8/8 tests passed:
tests/test_extract_skill_json_nonstring.py (2 passed)
tests/test_skill_extractor_rows.py (1 passed)
tests/test_search_content_extraction_parity.py (2 passed)
tests/test_deep_research_search_error.py (3 passed)
Closes#2199
* test(skill-extractor): add focused repro for stray-brace JSON recovery
* test(skill-extractor): add regression test for leading invalid-brace fragment
Addresses the remaining edge case from review: a response that *starts*
with a brace but the leading fragment isn't valid JSON (e.g.
'{not json}\n{"title": "Valid later", ...}') still needs to recover
the valid skill object that follows.
_extract_json_object (already on dev) handles this correctly — it tries
the whole de-fenced string first, then walks each '{' candidate left-to-
right regardless of whether the response begins with '{', so the leading
invalid fragment no longer short-circuits recovery of the real object.
Updates the comment at the call site to call this out explicitly and adds
a regression test covering exactly the scenario described in review.
This file documents the shared test helpers and the review expectations that go
with them. The suite is being refactored incrementally, so this is a working
reference for that effort — not a claim that the suite is already fully
organized. Read it before adding a new helper or before reviewing a PR that
touches tests/helpers/.
Core principles
Keep PRs small and homogeneous: one kind of change per PR.
Prefer explicit local setup over hidden global fixtures.
Avoid expanding the root conftest.py unless absolutely necessary.
Do not mix file moves with logic changes in the same PR.
Do not weaken tests with skip/xfail just to make CI pass.
Validate the focused files you changed, plus any neighboring or
order-sensitive groups they interact with.
Helper conventions
The helpers below live under tests/helpers/. They exist to remove repeated
boilerplate that already appeared across multiple tests. Reach for one only when
your test matches its intended use; do not stretch a helper to cover a new case.
tests.helpers.cli_loader.load_script
Use when a test needs to import a script under scripts/ without repeating
SourceFileLoader / importlib.util boilerplate.
Intended for script/CLI tests that load a single file from scripts/.
Not for arbitrary package imports — use a normal import for those.
When migrating an existing test to it, keep the existing stubs and assertions
unchanged. Any sys.modules stubs the script needs at import time must still
be injected (e.g. via monkeypatch) before calling load_script.
tests.helpers.import_state.clear_module
Use when a test must drop one cached module and its parent-package attribute
before a fresh import.
Clears sys.modules[name].
Clears the parent-package attribute when present.
Good replacement for local sys.modules.pop(...) + delattr(parent, child)
blocks.
tests.helpers.import_state.preserve_import_state
Use when a test temporarily installs stubs into sys.modules and needs
deterministic cleanup afterward.
Context manager: restores both sys.modules entries and parent-package
attributes on exit (normal or exception).
Useful around module-level stubs or temporary imports.
Prefer narrow, explicit module names over broad ones.
Lucas Daniel
73315e6ddc