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

Task 3 final authoritative spec and code-quality review

Verdicts

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

The implementation closes the previously reported species, URL-authority, redirect, mapping-shape, taxon, endpoint-shape, canonical-denominator, signature-dispatch, and rate-gate defects. All 19 named mandatory Task 3 cases are present, and the current focused/backend/combined suites pass with live sockets and DNS disabled. The checkpoint nevertheless remains unapprovable: the network validator does not prove that returned edges satisfy the submitted identifier set or requested score threshold, and direct-target resolution records false Ortholog provenance. A lower-severity legacy compatibility fallback also weakens optional image response authority validation.

Findings

Important 1 — network responses are not checked against the submitted identifiers or requested score threshold

StringAPI.get_network() sends add_nodes=0 and required_score, but its response validation checks only taxon, nonblank endpoint fields, STRING-ID syntax, and whether score is finite in [0, 1] (skills/ppi/run.py:329-389). It never requires stringId_A and stringId_B to belong to the submitted proteins, and it never requires the returned score to meet required_score / 1000.

Fresh socket-blocked probes against current production code accepted both:

request identifiers: 3562.A, 3562.B
response endpoints:   3562.X, 3562.Y
result:                accepted

request required_score: 700
response score:         0.1
result:                 accepted

This violates the approved input-only family-network policy (add_nodes=0) and the plan-review requirement to validate the network score client-side. A malformed or inconsistent upstream response can therefore become a successful scientific edge and inflate network artifacts/metrics.

The current positive test reinforces part of the gap: it requests only ["3562.A"] while accepting an A-B response (tests/unit/test_ppi_id_resolution.py:785-802). No test covers a score below the requested threshold.

Required correction:

• retain the exact submitted STRING-ID set;
• after existing syntax/taxon validation, reject either endpoint unless it is in that set;
• reject scores below the requested threshold (with a deliberate numeric comparison policy); and
• add offline regressions for an unsubmitted same-taxon endpoint and a finite but below-threshold score. Update the existing positive row test to submit both endpoints.

Important 2 — direct-target resolution falsely labels canonical input as Ortholog evidence

Direct mode correctly creates origins with an empty ortholog_reference_id and the canonical member file as relationship_source (skills/ppi/run.py:1165-1173). However, _query_candidates() unconditionally labels every raw candidate's evidence_source as ortholog_relationship (skills/ppi/run.py:577-617), regardless of origin mode.

A fresh direct production PpiSkill.run() probe produced:

ortholog_reference_id = <empty>
relationship_source   = /tmp/.../members.id
evidence_source       = ortholog_relationship

That is factually false provenance for direct-target PPI and contradicts the core distinction between canonical inputs and Ortholog relationships. The TSV contains an internally contradictory record even though canonical IDs are not mutated.

Required correction: carry origin/mode-specific raw evidence into candidate construction. Projected raw candidates may cite the exact Ortholog relationship; direct raw candidates must cite the canonical member input (or an explicit canonical_input method/source). Add a production direct-mode TSV regression asserting the empty Ortholog field and truthful raw evidence source.

Minor 1 — optional image validation accepts legacy responses with no status or final URL

The optional image helper validates the final URL only when the response has a url attribute and defaults a missing status_code to 200 (apps/web/pipeline_backend.py:3256-3265). The two authorized backend fakes omit both attributes (tests/unit/test_pipeline_backend_execution_env.py:1022-1039,1060-1076), so the compatibility fallback is exercised by green tests.

A fresh socket-blocked fake response with image content but no url and no status_code was accepted and written successfully:

LEGACY_RESPONSE_WITHOUT_STATUS_OR_URL_ACCEPTED True b'legacy-image' 1

Real requests.Response objects normally provide both fields, and this image is explicitly non-authoritative, so the issue is Minor rather than Important. It still weakens the claimed fail-closed authority boundary.

Required correction: require an authoritative final response URL and an explicit non-boolean integer 2xx status. Update the two backend fakes to expose url=request_url and status_code=200; do not retain a production downgrade path for stale test doubles.

Mandatory Task 3 test audit

All exact cases required by Task 3 Step 1 exist in tests/unit/test_ppi_id_resolution.py and passed in the fresh socket-blocked focused run:

1. raw XP failure to NCBI GeneID fallback;
2. raw/GeneID convergence;
3. divergent candidate STRING IDs become ambiguous;
4. partial resolution preserves unresolved rows;
5. zero resolution is ID-resolution failure;
6. network [] is successful empty_network;
7. network HTTP failure remains an external failure;
8. mapping attribution uses queryIndex;
9. both stable-address field names are accepted;
10. invalid request parameters fail before HTTP;
11. 24 origins collapse to 12 queries while retaining 24 provenance rows and the 5-raw/7-GeneID fallback shape;
12. shared reference queries retain separate canonical rows;
13. conflicting Ortholog references are excluded;
14. mapping 404 and 200 [] both mean zero resolution;
15. malformed/duplicate/out-of-range/wrong-echo/wrong-taxon mapping rows are rejected;
16. injected clock/sleeper pacing works;
17. convergent references retain two resolved-query counts but one selected network ID;
18. real forked scientific/scientific callers serialize; and
19. real forked scientific/image callers serialize.

These cases are necessary but do not cover the two Important findings above.

Historical-finding closure audit

• Unknown/empty species fallback: closed. Unknown values fail before HTTP; every declared canonical species code self-resolves.
• Stable address and redirect authority: closed for scientific requests. Stable hosts are restricted to authoritative STRING HTTPS URLs, automatic redirects are disabled, and present non-STRING final URLs are rejected.
• Mapping protocol: closed. Query index, echoed query, strict scalar taxon, nonblank preferred name, and taxon-prefixed STRING ID coherence fail closed.
• Network row shape: prior missing/blank/wrong-taxon endpoint, taxon, and finite-range score findings are closed. Request/response membership and requested-threshold coherence remain open as Important 1.
• Canonical denominators and empty Ortholog inputs: closed. Missing, empty, and comment-only projected relationships retain factual canonical denominators and do not fall into direct mode.
• Signature fail-open dispatch: closed. The active run path always invokes deterministic resolution.
• Relationship provenance before deduplication: closed for projected mode; the 24-to-12 path retains 24 rows. Direct-mode provenance remains open as Important 2.
• Rate-gate safety: closed. The shared host gate serializes scientific and image calls, rejects non-finite clock/interval/timestamp state, fails closed on corrupt/unreadable state, holds the flock through update, and atomically replaces the timestamp.
• Empty network versus external failure: closed. Valid [] remains success with a header-only network TSV; HTTP/transport/JSON/protocol failures are not converted to empty science.

Production integration, scope, and maintainability

• The active PpiSkill.run() path constructs explicit origins, retains the complete canonical denominator, resolves before network submission, and submits deduplicated selected STRING IDs only.
• The internal id_map_file seam exists; CLI/backend discovery is correctly deferred to Task 4 by the authoritative Task 3 brief.
• Reset identity/snapshots, final manifests/contracts/diagnosis/frontend labels, and live/full-real-job execution remain outside Task 3. None was pulled into this patch.
• The backend production diff is confined to the active optional STRING image helper and shared gate import. The policy production/test expansions and two backend fake-signature changes are documented as parent-authorized.
• No live STRING call, backend service execution, Celery restart, or Task 6 full-job run was performed.

Review basis and fresh verification

Read and inspected in full:

• AGENTS.md, PLAN.md, PLAN-REVIEW-LOG.md, and approved Task 3 in docs/superpowers/plans/2026-07-10-ppi-id-resolution.md;
• the current Task 3 brief, report, review package, progress record, and all prior verification/anti-pattern/spec-quality reviews;
• complete baseline/current diffs for skills/ppi/run.py, apps/web/pipeline_backend.py, and tests/unit/test_ppi_string_policy.py;
• complete current/new gfa_core/string_rate_gate.py, gfa_core/ppi_string_policy.py, and tests/unit/test_ppi_id_resolution.py; and
• the two authorized backend test-fake changes.

The workspace has no functional Git repository (.git contains only info/), so the review-package baselines under /tmp/ppi-id-resolution-task3-baseline/ were used as instructed.

Before every pytest run, socket.socket.connect, connect_ex, socket.create_connection, and socket.getaddrinfo were replaced with a function that raises. Fresh results:

Focused Task 3 suite:
116 passed in 0.72s (exit 0)

Relevant backend image/PPI selection:
26 passed, 5 deselected in 5.64s (exit 0)

Combined regression:
159 passed in 6.59s (exit 0)

py_compile gfa_core/ppi_string_policy.py gfa_core/string_rate_gate.py
           skills/ppi/run.py apps/web/pipeline_backend.py:
exit 0, no output

The focused run includes the two real multiprocessing gate tests. Pytest emitted the pre-existing RequestsDependencyWarning for the environment's urllib3/chardet combination; it did not affect collection or assertions.

No production or repository test source was edited during this review. This review document is the only repository file created.