Greptile Summary

This PR extracts the two-step ingest flow (create investigation → attach URL) from generate_report() into a dedicated helper create_investigation_and_attach_url() in app/utils/ingest_delivery.py. The refactor preserves all existing behavior and error-handling semantics without modifying send_ingest()'s contract.

Confidence Score: 5/5

Safe to merge — pure refactoring with no behavioral changes; all findings are P2 style/cleanup items.

All issues found are P2: a cross-layer import that doesn't cause a runtime problem, missing tests that were claimed but not blocking the refactored logic (existing behavior is unchanged), and a stale comment. No functional regressions introduced.

app/utils/ingest_delivery.py — cross-layer import from app/nodes/ and missing tests for the new helper.

Important Files Changed

Sequence Diagram

sequenceDiagram
    participant N as node.py generate_report()
    participant H as ingest_delivery.py create_investigation_and_attach_url()
    participant S as send_ingest()
    participant API as Ingest API

    N->>H: create_investigation_and_attach_url(state, slack_message, summary)
    H->>S: send_ingest(state_with_report)
    S->>API: POST /api/investigations/ingest
    API-->>S: { data: { investigation_id } }
    S-->>H: investigation_id
    H->>H: get_investigation_url(org_slug, investigation_id)
    alt investigation_id present
        H->>S: send_ingest(state_with_url)
        S->>API: POST /api/investigations/ingest (with investigation_url)
        API-->>S: 200 OK
        S-->>H: ignored
    end
    H-->>N: (investigation_id, investigation_url)

_{Reviews (1): Last reviewed commit: "Merge branch 'Tracer-Cloud:main' into co..." | Re-trigger Greptile}

Hey @SB2318, this is a clean refactor — moving get_investigation_url to live alongside send_ingest in ingest_delivery.py makes sense since they share the same app.config dependency, and removing it from report.py keeps the formatter layer focused on presentation.

One small thing I noticed in the new create_investigation_and_attach_url function — looking at the test:

investigation_id, investigation_url = (
    ingest_delivery.create_investigation_and_attach_url(sample_state, "slack message")
)

It returns a tuple of (str | None, str | None), which is fine, but the function signature in the diff seems to be cut off so I couldn't verify the return type annotation. If it's not there yet, a -> tuple[str | None, str | None] annotation would make the contract clear for callers.

Also just noticed the cast import was removed from node.py — was that intentional (i.e. there are no more cast() calls after the refactor)? If so, the unused import removal is a nice cleanup bonus.

Good work on this one — the test coverage for the new function looks solid too! 🙏

Hey @SB2318, this is a clean refactor — moving get_investigation_url to live alongside send_ingest in ingest_delivery.py makes sense since they share the same app.config dependency, and removing it from report.py keeps the formatter layer focused on presentation.

One small thing I noticed in the new create_investigation_and_attach_url function — looking at the test:

investigation_id, investigation_url = (
    ingest_delivery.create_investigation_and_attach_url(sample_state, "slack message")
)

It returns a tuple of (str | None, str | None), which is fine, but the function signature in the diff seems to be cut off so I couldn't verify the return type annotation. If it's not there yet, a -> tuple[str | None, str | None] annotation would make the contract clear for callers.

Also just noticed the cast import was removed from node.py — was that intentional (i.e. there are no more cast() calls after the refactor)? If so, the unused import removal is a nice cleanup bonus.

Good work on this one — the test coverage for the new function looks solid too! 🙏

Thanks, I have some pending review tasks on these two PR.
Bot suggested me also to write unit test for the create_investigation_and_data_attach_url function.

But due natural calamity, I didnot fixed it yesterday.
please give me 1-2 hour, And please let me know any more suggestions from your side. 😇

@Ghraven, please review this PR, and let me know if it would be better to squash those commits, or you need any further changes.

I added corresponding test cases for the new methods and updated all usages to align with the project’s new send_ingest workflow changes.

Thanks for the updates @SB2318 — both points I raised look good now:

1. Return type annotation — -> tuple[str | None, str | None] is there on create_investigation_and_attach_url, type checkers can now infer the unpacked values correctly.

2. cast removal confirmed — the unused import is gone from node.py.

I also noticed get_investigation_url has been moved into ingest_delivery.py with the investigation_id=None → returns list page fallback preserved, which addresses the regression @yashksaini-coder flagged. That's the right call.

On squashing — I'd lean toward keeping the commits separate for now since the history is readable and the maintainers may want to see the progression. But that's ultimately muddlebee's call.

Looks solid to me! 🙏

Ran locally: 65/65 pass, lint clean. Refactor logic is correct and the move of get_investigation_url into ingest_delivery.py makes sense — it shares the app.config dependency with send_ingest and has no business being in the formatters layer.

One functional regression though:

investigation_url fallback is lost

Old node.py always computed a URL even when ingest failed:

investigation_url = get_investigation_url(state.get("organization_slug"), investigation_id)
# investigation_id=None → returns "{base}/investigations" (the list page)

The new helper only sets investigation_url when investigation_id is not None, so on first-ingest failure it returns (None, None). The call site then does:

build_action_blocks(investigation_url or "", investigation_id)

Slack's Block Kit rejects an empty string on the button's url field — the "View Details in Tracer" button will break whenever ingest is down or unreachable. The old behavior (fallback to the investigations list URL) should be preserved. Simplest fix is to always call get_investigation_url in the helper regardless of whether investigation_id is set:

investigation_url = get_investigation_url(state.get("organization_slug"), investigation_id)

and remove the investigation_url or "" guard at the call site.

Minor — test_node.py patches internals instead of the helper

test_masking_unmask.py patches create_investigation_and_attach_url wholesale, which is the right approach for an integration-level test. test_node.py still patches send_ingest and get_investigation_url at the utility module level — it works but locks the test to the helper's internal structure. Worth aligning to the same strategy.

The refactor itself is clean and the three new tests in test_ingest_delivery.py cover the right paths. Fix the URL fallback and this is good to merge.

With empty string removal, your test is falling.

Run python -m mypy app/
app/nodes/publish_findings/node.py:42: error: Argument 1 to "build_action_blocks" has incompatible type "str | None"; expected "str"  [arg-type]
Found 1 error in 1 file (checked 424 source files)
Error: Process completed with exit code 1.

```

@yashksaini-coder Please check my comment on these two issues.
#317 and #481

@SB2318 pls fix CI

@SB2318 pls fix CI

okay