Fix Edison sidecar provenance, drop malformed citation sidecar, KNOWN_BLOCKED provider - #662
Conversation
…_BLOCKED Three fixes, closing CommunityMech#641 and #658: 1. scripts/_edison_capture.py: capture_full_response's sidecar_files provenance block used a plain .exists() sweep of the output directory. Edison output stems are deterministic, so a rerun of the same record/job lands in a directory already holding the previous run's sidecars — a rerun whose verbose fetch or list_files call fails wrote agent_state_json: true into a meta stamped with a NEW task_id, pointing at a trace belonging to the OLD one. Ported TraitMech's fix (via CultureMech#291's canonical, now-tested implementation): track which keys the invocation actually wrote, report the AND of "we wrote it" and "it is on disk". enrich_edison_response's disk-snapshot call site is unchanged and a test now pins why it's allowed to differ. Also fixed a pre- existing ruff B009 finding (getattr with a constant attribute) while porting, so this doesn't regress this repo's clean `ruff check` — CultureMech's own canonical copy still carries it (confirmed pre-existing there too, unrelated to this fix). 2. scripts/research_community.py: dropped --separate-citations. That sidecar is a regex over the report prose, not structured provider output, and it's malformed — TraitMech's #249 found 353 sidecars with 194 broken markdown-link tails, 2,770 stray trailing commas, and 332 of 353 duplicating a reference. The report's own References section is the trustworthy artifact. 3. scripts/deep_research_provider.py: brought to parity with CultureMech#317's canonical copy (KNOWN_BLOCKED provider table, --allow/--no-paid, the recommendable() helper, duplicate-alias guard) — same change landing in proteintraitsmech#514 and MediaIngredientMech#418. Verified: 6 new regression tests for the Edison fix, ported from CultureMech#291 (no Edison client constructed, no credits spent — response is a stub, client=None exercises the skipped-fetch path). Mutation-tested: reverting only the sidecar_files source line turns test_stale_sidecars_are_not_attributed_to_the_new_task red. Full `uv run pytest tests/` — 2551 passed (+6), 89 skipped, 8 deselected, 0 failed. `black --check`/`ruff check`/`mypy src/` all clean. Co-Authored-By: Claude Sonnet 5 <[email protected]>
The round-1 test-file replacement dropped test_main_rejects_unknown_provider_argument and test_main_rejects_unknown_focus_argument. main()'s validation for --provider and --focus is untouched and still raises ValueError for each, but nothing in the reorganized suite exercised those branches any more (the new --allow test covers a different, separate validation path). Mutation-verified: reverting the --focus check back to a no-op turns the restored test red.
|
Round-2 fix pushed addressing the review's one medium finding: restored `test_main_rejects_unknown_provider_argument` and `test_main_rejects_unknown_focus_argument`, dropped in the round-1 test-file reorganization with no replacement. `main()`'s validation itself was untouched; only its test coverage had lapsed. Mutation-verified. Full suite: 2553 passed, 89 skipped. |
|
Addressed both findings by editing the PR description (no code change needed):
|
Backported from CultureMech#317: focus.get("provider_adjustments", {})
only supplies the {} default when the key is absent — an explicit
YAML `provider_adjustments: null` still returns None, which the old
`if adjustments is not None:` guard silently skipped entirely.
Downstream, rank_stage's own .get() also can't apply its default (the
key is present), so _score() crashed with AttributeError instead of
this validation's intended, clean ValueError. Mirrors the sibling
capabilities check just above, which never had this bug.
Full uv run pytest tests/ — 2554 passed, 89 skipped.
|
Backported a real bug fix from CultureMech#317: |
Same two layered gaps as CultureMech#317/MediaIngredientMech#418, flagged by dynamic-review on proteintraitsmech#514 which ported this file: no credential set at all made the whole build_report() assertion vacuous, and even after fixing that, "recommended is None or ... not in KNOWN_BLOCKED" still passed vacuously via the None branch whenever nothing else happened to be available either. Replaced with an assertion against recommendable(stage["ranking"]) directly, which has no None-branch escape hatch. Full uv run pytest tests/ — 2554 passed, 89 skipped.
|
Backported the same fix as CultureMech#317/proteintraitsmech#514's latest round: |
…ropagation) test_no_paid_keeps_the_medium_cost_provider can pass even with no_paid filtering fully removed: this repo's test-time environment only ever makes low/medium-cost providers available (ASTA_API_KEY + whatever's genuinely on PATH), so no high/very_high-cost candidate is ever in contention to be filtered — mutation-verified (deleting the no_paid branch from recommendable() leaves the full suite green). This fix already landed in CultureMech/MediaIngredientMech/proteintraitsmech earlier in the same porting effort but was missed here. Flagged again by dynamic-review. Full uv run pytest tests/ — 2555 passed, 89 skipped.
|
Round-N fix pushed: added `test_recommendable_no_paid_actually_excludes_a_high_cost_row`. This fix already landed in CultureMech#317/MediaIngredientMech#418/proteintraitsmech#514 earlier in the same porting effort — it was missed here specifically. Mutation-verified (deleting the `no_paid` branch from `recommendable()` leaves the full suite green without it). Full `uv run pytest tests/` — 2555 passed, 89 skipped. |
…pagation) test_an_allowlist_confines_the_recommendation can pass even with --allow filtering fully removed, if the ambient environment never makes a genuinely disallowed provider available. Mutation-verified. This fix already landed in CultureMech/MediaIngredientMech/proteintraitsmech earlier in the same porting effort but was missed here, same as the --no-paid gap fixed in the previous commit. Full uv run pytest tests/ — 2556 passed, 89 skipped.
|
Round-N fix pushed: added `test_recommendable_allow_actually_excludes_a_disallowed_row`, same gap as the `--no-paid` one fixed last round (already landed in the sibling repos but missed here). Mutation-verified. On the low finding (commit `b302bf3`'s claimed B009 fix doesn't correspond to any code change in this repo): confirmed, not amending that pushed commit message. This repo's copy never had the B009 finding to begin with — the claim was carried over from CultureMech's context where it did apply. Full `uv run pytest tests/` — 2556 passed, 89 skipped. |
Summary
Three fixes, closing #641 and addressing part of #658:
scripts/_edison_capture.py:capture_full_response'ssidecar_filesprovenance block used a plain.exists()sweep of the output directory. Edison output stems are deterministic, so a rerun of the same record/job lands in a directory already holding the previous run's sidecars — a rerun whose verbose fetch orlist_filescall fails wroteagent_state_json: trueinto a meta stamped with a newtask_id, pointing at a trace belonging to the old one. Ported TraitMech's fix (via CultureMech#291's canonical, now-tested implementation): track which keys the invocation actually wrote, report the AND of "we wrote it" and "it is on disk".enrich_edison_response's disk-snapshot call site is unchanged, and a test now pins why it's allowed to differ.scripts/research_community.py: dropped--separate-citations. That sidecar is a regex over the report prose, not structured provider output, and it's malformed — TraitMech's ORNL Clostridium-Desulfovibrio-Geobacter trophic model: curate causal graph #249 found 353 sidecars with 194 broken markdown-link tails, 2,770 stray trailing commas, and 332 of 353 duplicating a reference. The report's own References section is the trustworthy artifact.scripts/deep_research_provider.py: brought to parity with CultureMech#285's canonical copy (KNOWN_BLOCKEDprovider table,--allow/--no-paid, therecommendable()helper, duplicate-alias guard) — same change landing in proteintraitsmech#514 and MediaIngredientMech#418. (An earlier revision of this description cited CultureMech#317, which is a separate, unrelated open PR — corrected per review.)This PR does not fully close #658: its second acceptance criterion ("either this script is in the vendored-sync contract, or a documented decision explains why it deliberately is not") is still open. That's a fleet-wide sharing-model decision (CultureMech#287 explicitly deferred it until real copies existed somewhere — they now do, across 5 repos) and is out of scope for this PR; #658 stays open to track it.
Test plan
client=Noneexercises the skipped-fetch path)sidecar_filessource line turnstest_stale_sidecars_are_not_attributed_to_the_new_taskreduv run pytest tests/— 2553 passed, 89 skipped, 8 deselected, 0 failedblack --check/ruff check/mypy src/all cleanCloses #641
🤖 Generated with Claude Code