diff --git a/findings-J.jsonl b/findings-J.jsonl new file mode 100644 index 0000000..215a132 --- /dev/null +++ b/findings-J.jsonl @@ -0,0 +1,6 @@ +{"review":"J4-implementation-reviewer-A","epic":"PM-86","task":"PM-83","target_commit":"66015acd","parent_commit":"bc35ccf9","verdict":"CLEAN","summary":"Architecture/cross-task/replace-vs-edit review of J4 commit 'harden generated preview lifecycle'. Single registry authority preserved. Metadata-only draft persistence with verified server-side re-acquisition. No second authority, resolver, route (beyond app-internal /app/staged), or state machine. J2 fixes intact. No browser/renderer duplication. Sibling task boundaries (A/C/F/G/H/I) respected. No critical/high/medium findings."} +{"id":"J4-A-01","severity":"low","category":"architecture","subject":"Preview generation Map replaces monotonic counter with content-addressed identity","evidence":["src/publish.rs:186 preview_generations: Signal>","src/publish.rs:2585-2620 preview_identity hashes draft_scope+model_id+primary_id+sorted resource metadata","src/publish.rs:2286-2291 preview_is_current checks identity match before installing"],"assessment":"The old preview_generation: Signal was a simple monotonic counter that could be satisfied by stale bytes if the counter happened to align. The new HashMap uses a SHA-256 identity over resource_id, source_name, byte_length, and digest for all staged resources — a content-addressed staleness check. This is a well-justified hardening that prevents stale-pixel races. The approach is proportionate, not over-engineered: it reuses sha2 already in the dependency tree and does not introduce a new state machine or lifecycle suite.","recommendation":"None — correct and proportionate."} +{"id":"J4-A-02","severity":"low","category":"replace-vs-edit","subject":"DraftStagedResource correctly replaced: bytes removed, verified metadata retained","evidence":["Old (removed): DraftStagedResource { name, mime_type, bytes, digest: None, byte_length }","New: DraftStagedResource { resource_id, upload_id, source_name, mime_type, digest (32-byte required), byte_length, format }","src/publish/draft.rs:104-119","src/publish.rs:2574-2582 staged_resource() requires 32-byte digest"],"assessment":"The old struct serialized geometry bytes into draft JSON with an unverified digest (None). The new struct is metadata-only: bytes are reacquired through the authenticated /app/staged boundary and re-verified client-side via acquire_preview_resources(). This is a correct replace decision — the ad-hoc bytes-in-draft pattern was a security/integrity gap. The new AcquiredDraftResource ephemeral pairing preserves verified bytes only in memory.","recommendation":"None — correct replacement."} +{"id":"J4-A-03","severity":"low","category":"ownership-boundaries","subject":"New /app/staged/{upload_id} route is app-internal, authenticated, owner-scoped","evidence":["src/appview/mod.rs:159-162: .route(\"/app/staged/{upload_id}\", axum::routing::get(drafts::get_staged_resource))","src/appview/drafts.rs:255-293: ExtractOAuthSession + authenticated_did + load_upload_file (owner-scoped) + validate_file_manifest + fetch_staged_bytes","src/publish/draft_client.rs:153-160: wasm client gated to target_family=wasm"],"assessment":"The new route follows the existing /app/drafts* and /app/images pattern: app-internal, not federated, OAuth-session-authenticated, owner-scoped. It validates the file manifest before streaming, enforces MAX_PREVIEW_RESOURCE_BYTES, and uses the existing CID/SHA-256 verification path. No new XRPC, no new lexicon, no security boundary expansion. Consistent with the plan's 'server-side drafts are app-internal' convention.","recommendation":"None — correct ownership."} +{"id":"J4-A-04","severity":"low","category":"cross-task-overlap","subject":"fetch_staged_bytes shares pattern with fetch_verified_bytes but is not problematic duplication","evidence":["src/appview/ldraw.rs:506-586 fetch_verified_bytes (C/G compound load from published records)","src/appview/ldraw.rs:588-664 fetch_staged_bytes (draft preview re-acquisition from staging metadata)"],"assessment":"Both functions resolve DID doc → PDS endpoint → GetBlob → stream chunks → CID verify → SHA-256 verify. However, their inputs differ fundamentally: fetch_verified_bytes takes a ResolvedResource (from published ATProto records), while fetch_staged_bytes takes raw blob_cids/length/digest (from upload_staging). Factoring out the shared streaming core would over-abstract given the different ownership semantics. The ~20 lines of shared streaming logic are small enough to be acceptable.","recommendation":"None — acceptable design choice."} +{"id":"J4-A-05","severity":"low","category":"cross-task-overlap","subject":"Sibling task boundaries A/C/F/G/H/I respected — no new resolver, state machine, or second authority","evidence":["No new LoadReducer, epoch counter, or readiness route (A preserved)","No new routes beyond /app/staged for C/G existing compound/load bundle paths (C/G preserved)","MeshFormat enum order unchanged, LengthUnit::LdrawUnit still appended after Unknown (F preserved)","plan_draft_ldraw_bundle uses ModelLoadBundle + polymodel_ldraw_core::extract_references (G/H consumed, not reimplemented)","No TEXMAP/Studio PE assumptions imported (I seam preserved)","thing_detail.rs unchanged in J4 — already registry-backed from prior J commits"],"assessment":"J4 is a focused hardening commit that replaces the browser-local bytes-in-draft pattern with server-verified re-acquisition. It does not alter the registry, lexicons, generated API, thing_detail dispatch, viewer bridge, download paths, or compound load infrastructure. The only new server surface is /app/staged, which is app-internal draft infrastructure.","recommendation":"None — boundaries clean."}