PlantAI-dev/PLAN-REVIEW-LOG.md

# Plan Review Log: PPI stage-local STRING ID resolution

Act 1 discussion complete — plan locked with the user. MAX_ROUNDS=5.

## Phase 0 evidence consolidation

### Local code inventory

- `skills/ppi/run.py` currently strips versions in both Ortholog and direct paths, has no `get_string_ids`, writes `probe_status=not_run`, and conflates network API errors with `[]`.
- `apps/web/pipeline_backend.py` creates `pep_cds_idmap.tsv`, but stores `LOC...` in `gene_id`; numeric NCBI GeneID exists only inside `cds_header`.
- PPI main and PPI Reset are separate backend entry points and both must pass the same ID evidence.
- Existing AlphaFold link generation already prefers `string_id`, so a populated resolution report can be reused.
- Existing frontend can preview the TSV; direct metric labels are an enhancement required by the approved acceptance criteria.

### Official STRING API evidence

- Official docs: https://string-db.org/help/api/ and fixed v12 docs: https://version-12-0.string-db.org/help/api/.
- `get_string_ids` returns one best STRING match per input; `limit` is not a supported parameter.
- Use `queryIndex` rather than response order; unresolved inputs are omitted.
- All-unresolved `get_string_ids` has been observed/documented as HTTP 200 with `[]` or HTTP 404; network can also return 404 for unusable inputs. Endpoint, response shape, and request state—not status alone—must drive classification.
- `network` should explicitly set `add_nodes=0` for input-only family networks.
- Validate score, network type, taxon, and non-empty identifiers client-side.
- Version response currently uses `stable_address`; client should also tolerate documented `string_stable_address`.
- Official guidance recommends POST for lists, `caller_identity`, stable-version URLs, and at least one second between requests.

## Round 1 — Codex

The biological direction is sound—canonical IDs remain immutable, Ortholog remains a relationship, GeneID is a valid job-local bridge, partial mapping is distinguishable from an empty network, and the scope excludes unrelated Ortholog/KaKs redesign. However, the plan is not implementation-ready.

Blocking changes:

1. Close the PPI Reset identity and scope path.
   - Reset currently reloads Ft canonical IDs but does not carry the Ft→soe Ortholog relationship; the soe alias map alone is insufficient.
   - Carry the exact scoped Ortholog file and verified ID map into Reset, select the exact scoped source manifest, constrain client-supplied paths, declare Reset input namespace, and test real Ft→soe Reset wiring.
2. Make provenance cardinality explicit before deduplication.
   - The real job has 24 Ft canonical members, 12 unique soe reference queries, and many-to-one relationships. Preserve every canonical→reference path and deduplicate only final STRING network queries.
   - Cover conflicting references, versionless collisions, conflicting GeneIDs, and mixed-species ID-map rows.
3. Define metric denominators and exact manifest locations.
   - Distinguish canonical members, Ortholog relationships, unique reference queries, resolved STRING queries, returned network nodes, and edges.
   - Main, scoped, and Reset manifests must expose the same scientific metrics; frontend tests must assert values, not label presence only.
4. Reconcile partial mapping with `evaluate_string_mapping_quality()`.
   - Partial nonzero resolution must be nonterminal; zero resolution must fail; ambiguous entries must be excluded.
5. Lock the STRING HTTP state machine.
   - Handle all-unresolved `404` and `200 []`, validate `queryIndex` and taxon, test throttling, always send `add_nodes=0`, and distinguish stable scientific endpoints from optional visualization endpoints.
6. Specify header-only empty-network artifacts and persisted failure manifests, including CLI exit codes.
7. Expand validation to scoped Reset, exact manifest selection, cross-job/species rejection, Reset APIs, stage status, 24→12 provenance, and manifest equivalence.

VERDICT: REVISE

### Codex's response

Accepted all seven blockers because each is supported by current code or real-job evidence and remains inside the approved PPI-first boundary. The revised plan will:

- make Reset canonical-target-only, scope-bound, and dependent on both the exact Ortholog relationship and species-matched alias map;
- introduce an explicit per-canonical relationship record and retain 24 provenance rows before 12-query deduplication;
- replace ambiguous metrics with nested, denominator-defined ID-resolution and network metrics shared by main/scoped/Reset manifests;
- retire the old terminal low-rate threshold in favor of complete/partial/failed resolution states;
- define the STRING response validator, request pacing, empty-network TSV schema, failure manifests, exit codes, and expanded API/real-data validation.

The optional STRING image/link endpoint will remain explicitly non-authoritative visualization evidence if it cannot use the same stable version endpoint; it will not determine scientific stage status.

## Round 2 — Codex

The revision closes the original seven blocker groups in substance. Four material issues remain:

1. `query_mapping_rate` uses the wrong numerator when multiple reference queries converge on one STRING entity. Add `resolved_reference_query_count`; keep `selected_string_query_count` as the deduplicated network-query count and test two references converging on one STRING ID.
2. Reset needs race-safe evidence snapshots and a separate direct-target policy. Snapshot and re-hash canonical input, Ortholog mapping, and ID map inside the isolated Reset directory before launch; validate direct-target IDs against the recorded canonical member set.
3. Cross-process STRING pacing is not yet testably specified. PPI uses a light queue with concurrency four, so choose a shared interprocess gate or a dedicated single-concurrency queue and test it.
4. Contract/failure validation is incomplete. Explicitly test persisted failed manifests and backend/status exposure for CLI exits 2/3/4; synchronize `skills/ppi/config.yaml`; decide whether `network_type` is fixed or configurable; replace the stale flat metric schema in the project record.

VERDICT: REVISE

### Codex's response

Accepted all four findings.

- Add `resolved_reference_query_count` and use it for query mapping rate/status; network-query deduplication remains a separate count.
- Record the canonical input hash and make Reset execute from verified snapshots copied into its isolated directory. Projected Reset validates against Ortholog sources; direct-target Reset validates against the recorded canonical member set.
- Use a host-shared `fcntl.flock` + timestamp file gate for all stable STRING version/mapping/network requests across main and Reset processes; production interval remains one second and a short-interval multiprocessing test proves serialization.
- Add backend/status failure-manifest tests, synchronize `skills/ppi/config.yaml`, and keep this phase's PPI analysis fixed to `network_type=functional` to avoid unrelated request/UI expansion. Update the original project-record metric section to the nested schema.

## Round 3 — Codex

Round 2 is almost fully closed. Two material blockers remain:

1. The shared STRING rate gate excludes automatic image requests; route every fallback attempt through the same interprocess gate or disable retrieval, and test a scientific request concurrent with an image request.
2. The direct-target Reset rule conflicts with a following blanket rule requiring all added IDs to be Ortholog sources; qualify that rule as projected-only.

VERDICT: REVISE

### Codex's response

Accepted both findings. The detailed plan now places the host-shared gate in `gfa_core.string_rate_gate` and requires every active PPI-path STRING HTTP call—including each optional image/SVG fallback—to use it with the same caller identity. The multiprocessing suite combines scientific and image callers. The Ortholog-source requirement is explicitly projected-Reset-only; direct-target Reset accepts only IDs present in the recorded canonical snapshot.

## Round 4 — Codex

All prior blockers are closed.

- `gfa_core.string_rate_gate` is the single host-shared gate for version, mapping, network, and every image/SVG fallback in main and Reset, with aligned caller identity and mixed scientific-plus-image multiprocessing coverage.
- Projected Reset additions must be Ortholog sources; direct-target Reset accepts only IDs from its recorded canonical snapshot. The rules and tests no longer conflict.
- The earlier metric, snapshot, failure-manifest, configuration, network-type, and validation findings remain closed.

Non-blocking caution: the `fcntl` gate is intentionally host-local; a future multi-host worker deployment would require a distributed gate.

VERDICT: APPROVED