PlantAI-dev/.superpowers/sdd/task-2-review.md

Task 2 Independent Review — Reference-bundle ID evidence schema

Verdicts

• Spec compliance: PASS. The implementation adds the requested numeric NCBI GeneID evidence while preserving the LOC-style gene_id, existing row selection rules, and legacy empty-value behavior.
• Code quality: PASS WITH MINOR FINDING. The production change is small, readable, and confined to the two approved functions. Focused coverage proves the principal happy path, but does not directly pin every stated compatibility edge case.

Findings

Critical

None.

Important

None.

Minor

1. The focused regression does not directly assert the missing/malformed metadata behavior or the complete TSV schema/order.

   The new test at tests/unit/test_pipeline_backend_execution_env.py:62-86 checks only a CDS header containing a valid exact GeneID tag and reads values through csv.DictReader. It does not assert the full field-name sequence, an empty ncbi_gene_id for a header with no matching tag, a malformed/non-numeric tag, or PEP fallback when CDS lacks the tag. The pre-existing reference-bundle download test does exercise a no-GeneID bundle without a crash, but it never reads the generated cell. The current implementation is deterministic for these cases (else "" at apps/web/pipeline_backend.py:1538 and CDS-first, PEP-fallback at line 1583), so this is a coverage weakness rather than a production correctness failure. A small parameterized test would lock down the requirement and exact schema order.

Compliance assessment

• Parsing correctness: apps/web/pipeline_backend.py:1532 uses an exact, numeric-only pattern for [db_xref=GeneID:<digits>]; line 1538 stores only the digit payload. Non-matching metadata produces "" rather than changing gene_id or raising unexpectedly.
• LOC compatibility: Existing [gene=...] extraction is unchanged at lines 1531 and 1537. The TSV still selects CDS gene_id first and PEP gene_id second at line 1582. The regression proves LOC110806180 remains intact while 110806180 is emitted separately.
• TSV schema and order: Existing columns retain their relative order and meanings. ncbi_gene_id is inserted immediately after gene_id, yielding species_code, protein_id, pep_id, cds_id, gene_id, ncbi_gene_id, pep_present, cds_present, pep_header, cds_header. Repository search found no positional consumer of the shifted trailing columns; the intended ReferenceAliasIndex consumer uses csv.DictReader by name and accepts the optional field.
• Missing/legacy bundles: Missing PEP/CDS records continue to use empty dictionaries and empty strings. Missing or malformed GeneID metadata yields an empty ncbi_gene_id, and the already-existing columns are still written. Task 1's parser continues to accept both legacy maps without the new field and new maps with it.
• Precedence: The new field follows the established CDS-first, PEP-fallback rule. No canonical member ID, PPI query, Ortholog relationship, frontend, manifest, or stage behavior was changed.
• Surgical scope: The baseline/current production diff contains only the regex extraction, record field, TSV field name, and TSV row value in the two approved functions. The test diff contains one import and one focused test. No unrelated production or test edits are present in the provided baseline comparisons.

Fresh verification

Commands run from the repository root on 2026-07-10:

/home/xia-lab/miniconda3/envs/test4py311/bin/python -m pytest \
  tests/unit/test_pipeline_backend_execution_env.py \
  -k 'reference_bundle or ppi' -q

Result: 25 passed, 6 deselected, exit code 0.

/home/xia-lab/miniconda3/envs/test4py311/bin/python -m pytest \
  tests/unit/test_id_resolution.py -q

Result: 12 passed, exit code 0. This verifies the downstream parser accepts the explicit ncbi_gene_id schema while retaining legacy-header behavior.

/home/xia-lab/miniconda3/envs/test4py311/bin/python -m py_compile \
  apps/web/pipeline_backend.py

Result: exit code 0, no output.

Both pytest runs emitted the pre-existing RequestsDependencyWarning about the installed requests dependency versions; it did not affect test results and is outside Task 2 scope.

Review basis and limits

The review compared both provided /tmp/ppi-id-resolution-task2-baseline/ copies with the current production/test files, inspected the approved Task 2 plan and surrounding producer/consumer code, and ran fresh focused tests. The reported RED step is historical TDD evidence and cannot be independently replayed against the current production file without modifying/reverting the workspace; the baseline diff is consistent with the reported missing-column failure. No real-data or API execution was required by the accepted Task 2 checkpoint.