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

Task 3 independent spec and code-quality review

Verdicts

• Spec compliance: FAIL
• Code quality: FAIL — changes required
• Checkpoint recommendation: NOT APPROVED
• Findings: 0 Critical, 4 Important, 2 Minor

The implementation covers the named happy paths and every mandatory test name is present and green. The checkpoint is not approvable because unsupported species can silently query Arabidopsis, malformed taxon values can be accepted, the stable-version URL is not validated as an authoritative STRING URL, and production metrics omit canonical members that have no Ortholog relationship.

Review basis

Reviewed in full:

• AGENTS.md, PLAN.md, PLAN-REVIEW-LOG.md.
• docs/superpowers/plans/2026-07-10-ppi-id-resolution.md, including all of Task 3.
• .superpowers/sdd/task-3-brief.md, task-3-report.md, and task-3-review-package.md.
• Complete baseline/current diffs for skills/ppi/run.py, apps/web/pipeline_backend.py, and tests/unit/test_ppi_string_policy.py.
• Complete new files gfa_core/string_rate_gate.py and tests/unit/test_ppi_id_resolution.py.
• CodeGraph call paths for the STRING client, resolver, rate gate, image helper, and ReferenceAliasIndex integration.

No production or repository test file was edited during this review. Adversarial checks were kept in /tmp/test_task3_review_edges.py.

Findings

Important 1 — Unsupported species silently become Arabidopsis taxon 3702

StringAPI.__init__() calls string_taxon_id(species, "3702") at skills/ppi/run.py:85. Therefore an empty, misspelled, or unsupported species becomes taxon 3702 rather than failing before HTTP. This contradicts the plan's client-side taxon validation and can produce scientifically incorrect but superficially valid Arabidopsis mappings.

Reproduction: StringAPI("not_a_species", ...) constructs successfully with species_id == "3702"; the adversarial test expecting pre-HTTP rejection failed.

Required correction: resolve with an empty fallback, reject an unknown/empty species before creating the client or sending a request, and add a no-HTTP regression test. Keep explicit support for recognized aliases such as Spinacia oleracea -> soe -> 3562.

Important 2 — Stable-version addresses are concatenated, not parsed or validated

_normalize_api_base() at skills/ppi/run.py:98-103 strips slashes and appends /api. get_version() at lines 166-175 accepts either field but does not validate scheme, hostname, credentials, port, path, query, or fragment. Values such as javascript:alert(1), http://evil.example, and https://string-db.org.evil.example become request bases instead of typed protocol failures.

This undermines the stable-version provenance boundary and can send submitted identifiers to a non-STRING endpoint if the discovery payload is malformed or compromised.

Required correction: parse with urllib.parse.urlsplit, require HTTPS and an exact string-db.org host or genuine *.string-db.org subdomain, reject credentials/query/fragment and unexpected paths, normalize only an empty path or /api, and raise StringApiError(kind="protocol", endpoint="version"). Add accepted tests for host-only, trailing slash, and /api, plus rejected deceptive-host and malformed-URI cases.

Important 3 — Mapping response validation accepts malformed taxon and preferredName fields

At skills/ppi/run.py:214-225, ncbiTaxonId is coerced with int(taxon). JSON 3562.5 becomes 3562 and is accepted for Spinacia. The mandatory wrong-taxon test covers 3702, but it does not cover malformed values that truncate to the expected taxon.

Reproduction: a mapping row with ncbiTaxonId: 3562.5 passed get_string_ids() instead of raising StringApiError(kind="protocol").

The same validator does not require preferredName to be a string. Both a missing field and preferredName: ["A"] pass validation; the resolver later converts arbitrary values with str(...) at line 639. That violates the explicit JSON-shape boundary and weakens exact mapping provenance.

Required correction: accept only a non-boolean integer or an all-digit integer string, then compare exactly. Add malformed cases for floats, booleans, empty strings, and mixed numeric text. Require preferredName to be a string and reject a missing or non-string value rather than coercing arbitrary JSON later.

Important 4 — Production resolution metrics drop canonical inputs without Ortholog relationships

The production run() path filters origins to matching canonical IDs at skills/ppi/run.py:1052-1065, logs missing members, and then passes only retained origins into the resolver. _resolution_metrics() derives input_member_count from those origins at lines 750-766. Consequently, a canonical input without an Ortholog relationship disappears from input_member_count, unresolved_member_count, and all resolution evidence.

Reproduction: two canonical inputs (Ft1, Ft2) with only Ft1 -> XP_1.1 produce input_member_count == 1, resolved_member_count == 1, and unresolved_member_count == 0, despite logging 1/2 missing.

This violates canonical-input provenance and makes partial-resolution metrics overstate success. It is also incompatible with the approved denominator definition: input_member_count is the distinct canonical members presented to PPI.

Required correction: carry the complete canonical member set separately from relationship origins. Count absent relationships as unresolved members without fabricating an Ortholog ID; preserve relationship_count as the number of actual relationship rows. Add production-path tests for one missing relationship and for all relationships missing.

Minor 1 — The typed HTTP boundary can leak an untyped ValueError

status_code = int(...) at skills/ppi/run.py:126 is outside validation/translation. A malformed response status raises raw ValueError, not StringApiError. A real requests.Response normally supplies an integer, so this is lower risk, but it weakens the stated typed boundary and test-double contract.

Wrap status extraction/validation in the protocol boundary and raise a typed protocol error for a malformed response object.

Minor 2 — Compatibility dispatch is coupled to runtime signature inspection

At skills/ppi/run.py:1049-1050, deterministic resolution is enabled only when inspect.signature(self.analyze_network) contains the literal parameter origins. This preserves the existing monkeypatched legacy test, but a wrapper, decorator, subclass, or mock can silently switch production behavior back to the old lossy normalization/report path.

Prefer an explicit compatibility seam or update legacy callers/tests to invoke a named legacy helper. Deterministic resolver use should not depend on introspecting an overridable method's parameter names.

Mandatory Task 3 test audit

Every exact mandatory case exists in tests/unit/test_ppi_id_resolution.py and passed:

1. raw XP failure -> GeneID fallback -> STRING ID;
2. raw and GeneID convergence;
3. one biological query resolving to two STRING IDs -> ambiguous;
4. partial resolution with an explicit unresolved row;
5. zero selected IDs -> ID-resolution failure;
6. network [] -> empty_network success;
7. network HTTP failure remains external API failure;
8. response attribution by queryIndex, not response order;
9. both stable_address and string_stable_address fields;
10. invalid network parameters rejected before HTTP;
11. 24 origins -> 12 reference queries without provenance loss, with 5 raw and 7 GeneID-only resolutions;
12. many canonical members sharing one reference retain separate rows;
13. conflicting Ortholog references are excluded;
14. mapping 404 and 200 [] both mean zero resolution;
15. duplicate, negative, out-of-range, non-integer, wrong-taxon, wrong-echo, and missing-string-ID mapping rows are rejected;
16. injected clock/sleeper pacing;
17. two references converging on one STRING ID retain two resolved-query counts but one selected network query;
18. real multiprocessing serialization between scientific clients;
19. real multiprocessing serialization between scientific and optional image callers.

The existing mandatory tests are necessary but not sufficient: they miss unsupported-species fallback, fractional taxon coercion, stable-URL authority checks, and the incomplete-Ortholog production denominator.

Positive assessment by review area

• Production integration: PpiSkill.run() invokes the resolver, submits only deduplicated selected STRING IDs, writes one row per retained relationship, short-circuits zero resolution, and emits a header-only network TSV for a valid empty network. Internal id_map_file wiring is present; CLI/backend --id-map wiring correctly remains Task 4 scope.
• 404/empty semantics: mapping 404 and 200 [] are zero mappings; network [] is valid empty science; network HTTP/transport/JSON/protocol errors are not converted to [].
• Candidate convergence: raw, normalized, versionless, and local candidates are probed once; all successful candidates must converge. Final STRING IDs alone are deduplicated.
• Ambiguous Ortholog handling: multiple references for one canonical source are allowed only when every reference resolves and all converge on one STRING entity; otherwise every associated row is ambiguous_ortholog and excluded.
• Relationship provenance: versioned reference accessions are preserved. relationship_source remains independent of the chosen alias evidence_source. The production integration test confirms exact two-row provenance, and the real artifact statically confirms 24 distinct canonical/reference pairs.
• Metrics: the complete real-shape case correctly reports 24 members, 24 relationships, 12 unique queries, 5 raw, 7 GeneID fallback, 12 resolved queries, 12 selected STRING IDs, and complete rates. The missing-relationship denominator remains the blocking gap above.
• Rate gate: one fcntl.flock gate serializes version, mapping, network, and each active image/SVG attempt. Timestamp replacement is atomic, the default interval is one second, BBAV5_STRING_RATE_GATE is honored, and clock/sleeper/path/gate injection makes tests deterministic. The approved host-local limitation remains explicit and acceptable.
• Compatibility: all prior policy/backend tests pass. The CLI flag and final manifest/failure taxonomy are correctly deferred to Tasks 4 and 5.
• Surgical scope: production changes are restricted to the Task 3 allowlist; the backend diff is limited to the shared gate/caller identity plus injection hooks. test_ppi_string_policy.py is unchanged from baseline.

Verification evidence

• Required focused suite: 43 passed in 0.61s.
• Relevant backend PPI/image selection: 26 passed, 5 deselected in 5.63s.
• Combined regression suite: 86 passed in 6.58s.
• py_compile for all three changed production files: exit 0.
• Static real-job shape (no network): 24 origins, 24 canonical members, 24 exact canonical/reference pairs, 12 unique reference queries, and GeneID candidates for all 12.
• Adversarial /tmp suite: 9 failed in 0.31s, reproducing the four findings above (three malicious/malformed stable-address variants and two preferredName variants are parameterized).

No live STRING request or full stage execution was performed; those are explicitly Task 6. No service or Celery restart was required for this read-only review.