Make --emit-yaml output pasteable, and refuse a --refresh that would do nothing (#380) - #457
Conversation
Review round 1 — the test that mattered was a tautologyThe reviewer confirmed the code is sound on every probe, and then found that my headline test proved nothing.
It now applies a block to a real record and reads back what landed, comparing key-for-key against Two smaller ones from the same review:
What the review cleared
|
test_both_render_paths_agree rebuilt emit_block's body character-for-character and compared it to emit_block. It passed for any DUMP_WIDTH and never touched apply_to_community at all - whose dump is a different call, on the inner block, without default_flow_style and with a hand-prefixed indent. Changing the apply side left every test green, which is precisely the regression the file's docstring promises to catch. It now applies a block to a real record and reads back what landed, comparing key-for-key against what --emit-yaml prints and asserting no line wrapped. Verified: against main's script four tests fail, this one among them, where before only the orphan check did. Two smaller ones from the same review. The fixture used a genus-rank lineage, which has no whitespace and so cannot wrap - leaving gtdb_lineage, the scalar the module comment names as a wrapping risk, untested; it is a species lineage now, whose final segment carries a space. And the orphan detector keyed on the absence of a colon, which a wrapped scalar can contain; it keys on indentation instead. Also dropped a no-op assertion I left in the rewrite. The review found nothing wrong with the code: no repo invocation of --refresh lacks --apply or --community, so the guards break no workflow; args.refresh is read at exactly one other place, the sole apply_to_community call, so the no-op diagnosis holds; and width=4096 on the emit path cannot reach the KB except by paste, where matching apply is the point. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Cause identified: a GitHub Actions platform outageNot this repo, not this PR. GitHub's own status API: Repo and org Actions settings are both Nothing to do here but wait. This PR is ready: |
CI caught what no local run could. Two tests shell out to gtdb_ground.py, which exits 1 with 'NCBI2GTDB mapping not found' without the kg-microbe crosswalk. Locally resolve_kg_microbe_dir always finds it by walking to a sibling directory, so the failure is invisible here and certain in CI. They now take the mapping fixture that seven other test files already use for the same reason. The two guard tests stay unguarded on purpose: the argparse checks run before the mapping is resolved. Worth stating, because a missing crosswalk also exits nonzero - so asserting the message and not just the exit code is what stops those passing for the wrong reason. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…do nothing (#380) apply_to_community dumped blocks at width=4096, one key per line, while --emit-yaml used PyYAML's default. So gtdb_lineage and mapping_source - the two long scalars - wrapped onto continuation lines indented deeper than the block keys. --emit-yaml exists to be pasted into a record, so the two paths disagreeing is how wrapped blocks reached the KB in the first place. Not cosmetic: it is the root cause of the corruption #378 had to fix. A line-level editor then has to handle both shapes, and that PR's first version matched exactly six spaces and orphaned the continuations into duplicate keys. One constant, DUMP_WIDTH, used by both. What keeps it that way is a test that renders the same grounding down each path and compares bytes - a future change to either dump call reintroduces #380 silently otherwise. The issue's secondary point, also real: --refresh only modifies how --apply treats existing blocks, so on its own, or with --ncbi-id/--name where there is no stored block, it ran the ordinary report and exited 0 - leaving the caller believing a re-ground had happened. Argparse enforced neither. Both are now refused with a message saying what --refresh actually does, and the one combination that works is unaffected. No KB records change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
test_both_render_paths_agree rebuilt emit_block's body character-for-character and compared it to emit_block. It passed for any DUMP_WIDTH and never touched apply_to_community at all - whose dump is a different call, on the inner block, without default_flow_style and with a hand-prefixed indent. Changing the apply side left every test green, which is precisely the regression the file's docstring promises to catch. It now applies a block to a real record and reads back what landed, comparing key-for-key against what --emit-yaml prints and asserting no line wrapped. Verified: against main's script four tests fail, this one among them, where before only the orphan check did. Two smaller ones from the same review. The fixture used a genus-rank lineage, which has no whitespace and so cannot wrap - leaving gtdb_lineage, the scalar the module comment names as a wrapping risk, untested; it is a species lineage now, whose final segment carries a space. And the orphan detector keyed on the absence of a colon, which a wrapped scalar can contain; it keys on indentation instead. Also dropped a no-op assertion I left in the rewrite. The review found nothing wrong with the code: no repo invocation of --refresh lacks --apply or --community, so the guards break no workflow; args.refresh is read at exactly one other place, the sole apply_to_community call, so the no-op diagnosis holds; and width=4096 on the emit path cannot reach the KB except by paste, where matching apply is the point. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pushes during the outage (Actions was in major_outage from ~14:50 to ~22:00 UTC) produced no workflow run. Empty commit to queue one now that events are being accepted again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CI caught what no local run could. Two tests shell out to gtdb_ground.py, which exits 1 with 'NCBI2GTDB mapping not found' without the kg-microbe crosswalk. Locally resolve_kg_microbe_dir always finds it by walking to a sibling directory, so the failure is invisible here and certain in CI. They now take the mapping fixture that seven other test files already use for the same reason. The two guard tests stay unguarded on purpose: the argparse checks run before the mapping is resolved. Worth stating, because a missing crosswalk also exits nonzero - so asserting the message and not just the exit code is what stops those passing for the wrong reason. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9efc9b1 to
c42aca1
Compare
Closes #380.
The divergence
apply_to_communitydumped blocks atwidth=4096, one key per line.--emit-yamlused PyYAML's default, so the two long scalars wrapped:--emit-yamlexists to be pasted into a record, so the two paths disagreeing is how wrapped blocks reached the KB.This is not cosmetic — it is the root cause of the corruption #378 had to fix. Once both shapes exist, a line-level editor has to handle both, and that PR's first version matched exactly six spaces and orphaned the continuations into duplicate keys.
The fix
One constant,
DUMP_WIDTH, used by both call sites. What keeps it fixed is a test that renders the same grounding down each path and compares bytes — otherwise a future change to either dump call (a width, a flow style, a sort) reintroduces #380 silently.The secondary point, also real
--refreshonly modifies how--applytreats existing blocks. On its own, or with--ncbi-id/--namewhere there is no stored block, it ran the ordinary report and exited 0 — leaving the caller believing a re-ground had happened. Argparse enforced neither:Both are now refused with a message saying what
--refreshactually does.--community X --refresh --applyis unaffected, and a test pins that so the guard can't over-reach.Scope
scripts/gtdb_ground.pyonly, +22/−2. No KB record changes. Six new tests.just qcgreen.just qc(lint + full suite + every offline validator) is green on this tree.🤖 Generated with Claude Code