Conversation
Codecov Report❌ Patch coverage is
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
DRC-3296. Adds info.json + lineage_diff.json to the cloud-mode precompute artifacts so Cloud's /info and /select endpoints can serve responses directly from S3 without re-computing lineage from raw dbt artifacts on every request. Both files are derived purely from the adapter's already-loaded manifests and catalogs via build_merged_lineage (same helper the OSS /api/info endpoint uses) and adapter.get_lineage_diff() — no SQL, no extra I/O, no new dependencies. Upload is keyed by info_url / lineage_diff_url in get_upload_urls_by_session_id. Missing keys produce a warning and continue (exit 0), keeping old-CLI + new-Cloud and vice-versa compatible. Written as raw JSON (no compression, per DRC-3296 scope). Uses a distinct recce-metadata-* tempdir prefix (distinct from A1's recce-cll-*) and shutil.rmtree's it on successful upload. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: even-wei <evenwei@infuseai.io>
0aaeea4 to
10db78a
Compare
Code Review: PR #1336Files reviewed: 4 ( Validation ResultsPass A: Correctness & Logic — PASS (minor)
Pass C: Cross-Reference Consistency — PASS
Pass D: Error Handling & Edge Cases — FAILISSUE — NOTE — Pass E: Test Coverage & Quality — PASS (one gap)NOTE — No test exercises the partial-emit failure path (first file written, second write raises). The existing
Pass F: Diff-Specific Checks — PASS
Pass G: Performance — PASS
Pass H: Async/Concurrency — N/A
Verification Results
Verdict: NEEDS-CHANGESIssues
Notes
What I could not verify
What I looked for and did not find
|
even-wei
left a comment
There was a problem hiding this comment.
Claude Code Review: 1 ISSUE + 3 NOTEs (NEEDS-CHANGES). Partial-emit failure silently reports 'Cloud upload complete.' — see review comment for details.
Addresses self-review finding on #1336 (DRC-3296) — previously, partial failures of emit_info_and_lineage_diff silently reported "Cloud upload complete.". When the emitter wrote info.json but then raised before lineage_diff.json was written, both local paths were reset to None. The upload loop's if/elif then fell through — upload_failures stayed empty and the success banner printed, masking the real failure. Add an explicit "URL present + local file missing" arm to the metadata upload loop. On that path we append the artifact to upload_failures and print a warning, so the end-of-run summary reports the partial failure. Chose approach B (upload-loop guard) over approach A (broader try/except around the emit) so emit exceptions stay separate from upload errors and the emit-time warning still fires. New test covers the partial-emit failure path: mock emit to write info.json then raise, assert no false success banner, both artifacts listed in upload_failures, and the "completed with warnings" summary fires. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: even-wei <evenwei@infuseai.io>
Updated Review — PR #1336SummaryRe-review after Findings[Resolved] Misleading "Metadata saved to …" path is deleted on the way outFile: [Unaddressed — low priority] Two
|
gcko
left a comment
There was a problem hiding this comment.
Claude Code Review: No critical issues found.
Address PR review feedback (gcko, Nit 1): both ``metadata_scratch`` and ``per_node_scratch`` are unconditionally cleaned up in the surrounding ``finally`` block, so printing "saved to <path>" pointed users at a directory that no longer existed by the time the command exited. Drop the path from both messages and keep only the load-bearing info (file sizes + elapsed time). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: even-wei <evenwei@infuseai.io>
|
@gcko Pushed a24c1cf addressing review nits. Nit 1 (misleading "Metadata saved to {metadata_scratch}") — fixed. Dropped the path from the success message; kept the load-bearing info (file sizes + elapsed). Applied the same fix to the Nit 2 (eager Verification: 16/16 tests pass ( |
PR checklist
What type of PR is this?
feat
What this PR does / why we need it:
Adds
info.jsonandlineage_diff.jsonto the cloud-mode precompute artifacts emitted byrecce init --cloud. These serve Cloud's/infoand/selectendpoints directly from S3 without re-computing lineage from raw dbt artifacts on every request.Both files are derived purely from the adapter's already-loaded manifests and catalogs:
info.json— the artifact-sourced portion of the/inforesponse (adapter_type + merged lineage), built viabuild_merged_lineage(same helper the OSS/api/infoendpoint uses). Cloud-specific fields (org_id, project_id, cloud_mode, pull_request, etc.) are injected by the Cloud API at request time.lineage_diff.json— the rawLineageDiff(base / current / diff) emitted byadapter.get_lineage_diff().Both are raw JSON (no compression, per scope decision). No new dependencies; no SQL execution beyond what
recce initalready does.Upload contract:
info.jsonuploaded viainfo_url,lineage_diff.jsonvialineage_diff_urlkeys onget_upload_urls_by_session_id. Graceful degradation: if either key is missing in the upload URL response, we log a warning and continue (exit 0) — keeps old CLI + new Cloud (and vice-versa) compatible during the rollout.Scratch dir: cloud mode uses a
recce-metadata-*tempdir (distinct prefix from A1'srecce-cll-*), cleaned up viashutil.rmtreeon successful upload, preserved on failure for debugging.Which issue(s) this PR fixes:
Resolves DRC-3296 — https://linear.app/recce/issue/DRC-3296
Special notes for your reviewer:
recce/cli.pyrebase conflict in the cloud-upload block when one merges first — trivial to resolve (both PRs append upload branches side by side).info_urlandlineage_diff_urlkeys need to be added toget_upload_urls_by_session_idin the cloud backend. This PR ships the CLI-side emit+upload logic with graceful missing-key handling; the cloud-side work is tracked separately and not in this PR's scope.recce/util/info_emitter.py) reusesbuild_merged_lineageandadapter.get_lineage_diff()so the JSON shapes stay in sync with the live-instance/api/infoand/api/selectresponses.Tests:
tests/test_info_emitter.py, 9 tests) — adapter-type resolution, payload shape/contract, atomic write, .tmp cleanup on failure, both-files write, parent-dir creation, pydantic round-trip.tests/test_cli_info_emitter.py, 6 tests) — happy path both-URLs-present, missing info_url warn+continue, missing lineage_diff_url warn+continue, both-URLs-missing warn+no-upload, local-mode non-regression (no emit, no upload), metadata-scratch prefix distinctness + cleanup.test_spa_route_*failures remain (SPA build missing in OSS test env, unchanged from baseline).Does this PR introduce a user-facing change?:
NONE