Skip to content

refactor: Ingest flow cleanup & Fix ingest duplication streamline investigation URL handling#1191

Merged
muddlebee merged 4 commits intoTracer-Cloud:mainfrom
SB2318:inject_flow_fix_with_steps
May 2, 2026
Merged

refactor: Ingest flow cleanup & Fix ingest duplication streamline investigation URL handling#1191
muddlebee merged 4 commits intoTracer-Cloud:mainfrom
SB2318:inject_flow_fix_with_steps

Conversation

@SB2318
Copy link
Copy Markdown
Contributor

@SB2318 SB2318 commented May 2, 2026 •
edited
Loading

Fixes #867

Followed by PR #1086

Describe the changes you have made in this PR -

This PR refactors the ingest logic in generate_report() by extracting the two-step flow (create investigation → attach URL) into a dedicated helper.
Previously, generate_report() directly called send_ingest() twice, which made the write path harder to follow and test. This change introduces a single helper that encapsulates that flow without modifying the existing API contract

Key Changes

✔ generate_report() no longer has duplicate ingest logic
✔ Helper encapsulates the two-step flow clearly
✔ No change to send_ingest() contract
✔ Failure behavior unchanged

Updated this PR to address all review comments and CI issues:

Changes made

  • Fixed investigation_url fallback regression by always computing:
    get_investigation_url(state.get("organization_slug"), investigation_id)
    inside create_investigation_and_attach_url() even when ingest fails.

    This preserves previous behavior:

    • successful ingest → investigation details URL
    • failed ingest → investigations list URL fallback

  • Updated helper return contract:

    • investigation_id: str | None
    • investigation_url: always str

  • Removed investigation_url or "" from build_action_blocks() call site since Slack Block Kit rejects empty URL strings.

  • Refactored test_node.py to patch create_investigation_and_attach_url() directly instead of patching helper internals (send_ingest / get_investigation_url), aligning with integration-level test strategy.

  • Added/updated corresponding tests for helper behavior and fallback URL scenarios.

Refactor summary

✔ generate_report() no longer has duplicate ingest logic
✔ Helper encapsulates the two-step ingest flow clearly
✔ No change to send_ingest() contract
✔ Failure behavior unchanged

Ran locally:

  • tests passing
  • lint clean
  • mypy passing

Please let me know if any further changes are needed.

Demo/Screenshot for feature changes and bug fixes -

Code Understanding and AI Usage

Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?

  • No, I wrote all the code myself
  • Yes, I used AI assistance (continue below)

If you used AI assistance:

  • I have reviewed every single line of the AI-generated code
  • I can explain the purpose and logic of each function/component I added
  • I have tested edge cases and understand how the code handles them
  • I have modified the AI output to follow this project's coding standards and conventions

Explain your implementation approach:

What this solves:

  • Eliminates duplicated two-step ingest logic
  • Improves readability and debuggability

Implementation approach:

Ingest Flow Refactor

  • Extracted helper: create_investigation_and_attach_url()
  • Encapsulates:
    • create investigation (send_ingest)
    • attach investigation_url (conditional second call)
  • Replaced inline logic in generate_report()

Key design choices:

  • Kept send_ingest() unchanged (no API contract changes)

Behavior guarantees:

  • Same ingest behavior and failure handling (logged, non-blocking)

Test coverage:

  • Ingest helper:
    • success path
    • second call failure
    • missing investigation ID

Checklist before requesting a review

  • I have added proper PR title and linked to the issue
  • I have performed a self-review of my code
  • I can explain the purpose of every function, class, and logic block I added
  • I understand why my changes work and have tested them thoroughly
  • I have considered potential edge cases and how my code handles them
  • If it is a core feature, I have added thorough tests
  • My code follows the project's style guidelines and conventions

Note: Please check Allow edits from maintainers if you would like us to assist in the PR.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 2, 2026

Greptile Summary

This PR extracts the two-step ingest flow (create investigation → attach URL) from generate_report() into a dedicated create_investigation_and_attach_url() helper in ingest_delivery.py, and moves get_investigation_url alongside it. The refactor is clean and the PR description accurately captures the behavior guarantees.

  • P1 — test asserts impossible None: test_create_investigation_first_ingest_failure mocks get_investigation_url to return None and asserts investigation_url is None, contradicting the helper's documented tuple[str | None, str] return contract. On a real first-ingest failure the function always returns the fallback string URL.

Confidence Score: 4/5

Safe to merge after fixing the incorrect test assertion in the failure path.

One P1 in the test suite: the failure-path test asserts investigation_url is None, which is impossible given the real get_investigation_url always returns a string. Production behavior is unchanged; the test just gives false coverage for the failure scenario. No P0 issues found.

tests/utils/test_ingest_delivery.py — test_create_investigation_first_ingest_failure needs its mock return value and assertion corrected to reflect the fallback URL contract.

Important Files Changed

Filename Overview
app/utils/ingest_delivery.py New create_investigation_and_attach_url helper cleanly encapsulates the two-step ingest flow; get_investigation_url moved here from report.py. Minor: outer try/except blocks around send_ingest are unreachable because send_ingest never raises.
app/nodes/publish_findings/node.py Inline two-step ingest logic removed; replaced with single create_investigation_and_attach_url call. Cleaner and consistent with the refactor goal.
app/nodes/publish_findings/formatters/report.py get_investigation_url removed from this file and relocated to ingest_delivery.py; import of get_tracer_base_url dropped accordingly. No logic changes.
tests/utils/test_ingest_delivery.py New tests for the helper added, but test_create_investigation_first_ingest_failure mocks get_investigation_url to return None and asserts investigation_url is None, which contradicts the declared str return type and tests an impossible code path.
tests/nodes/publish_findings/test_node.py Patch target updated from individual helpers (send_ingest, get_investigation_url) to create_investigation_and_attach_url, aligning with integration-level testing strategy.
tests/nodes/publish_findings/test_masking_unmask.py Patch updated from send_ingest to create_investigation_and_attach_url with a proper return tuple. No issues.

Sequence Diagram

sequenceDiagram
    participant GR as generate_report()
    participant H as create_investigation_and_attach_url()
    participant SI as send_ingest()
    participant GU as get_investigation_url()

    GR->>H: state, slack_message, summary
    H->>SI: state_with_report (report_md)
    SI-->>H: investigation_id (str | None)
    H->>GU: org_slug, investigation_id
    GU-->>H: investigation_url (always str)
    alt investigation_id is not None
        H->>SI: state_with_url (report_md + investigation_url)
        SI-->>H: (ignored)
    end
    H-->>GR: (investigation_id, investigation_url)
Loading

Reviews (1): Last reviewed commit: "refactor: streamline investigation URL h..." | Re-trigger Greptile

greptile-apps[bot]
greptile-apps Bot reviewed May 2, 2026
View reviewed changes
Comment thread tests/utils/test_ingest_delivery.py
Comment thread app/utils/ingest_delivery.py Outdated
@SB2318
Copy link
Copy Markdown
Contributor Author

SB2318 commented May 2, 2026

@muddlebee @yashksaini-coder @VaibhavUpreti please review.

@muddlebee
Copy link
Copy Markdown
Collaborator

muddlebee commented May 2, 2026

@SB2318 with so many new PRs it's difficult to track the older review again and newer round of review again. It's not really ideal.

If you are having issues with rebase etc I would suggest to explore/learn and practice on a demo PR in your fork. If that helps.

Otherwise it's very difficult to keep track of your changes across different PRs.

@SB2318
Copy link
Copy Markdown
Contributor Author

SB2318 commented May 2, 2026 •
edited
Loading

@SB2318 with so many new PRs it's difficult to track the older review again and newer round of review again. It's not really ideal.

If you are having issues with rebase etc I would suggest to explore/learn and practice on a demo PR in your fork. If that helps.

Otherwise it's very difficult to keep track of your changes across different PRs.

okay, let me mention the previous PR name.

PR #1086

Actually, I am using github desktop to squash all commit, but somehow they don't allow me to squash merge commits, but I have to update the branch also.

That's why I have to create so many PRs

@muddlebee
Copy link
Copy Markdown
Collaborator

muddlebee commented May 2, 2026

You don't have to squash merge commits. You just rebase if needed for merge conflicts that's it.

@muddlebee
Copy link
Copy Markdown
Collaborator

muddlebee commented May 2, 2026

Unless there's a merge conflict, you don't have to squash or rebase? Why you doing it over again?

@SB2318
Copy link
Copy Markdown
Contributor Author

SB2318 commented May 2, 2026 •
edited
Loading

Unless there's a merge conflict, you don't have to squash or rebase? Why you doing it over again?

I have to update branch. and beside this there are also so many commits more than 17 sometimes

@muddlebee
Copy link
Copy Markdown
Collaborator

muddlebee commented May 2, 2026

That's not the way to do. I will suggest you to go through best git/PR practices and how to do it.

And going forward such multiple PRs won't be acceptable. It creates friction for us maintainers. Thank you for understanding.

@SB2318
Copy link
Copy Markdown
Contributor Author

SB2318 commented May 2, 2026

That's not the way to do. I will suggest you to go through best git/PR practices and how to do it.

And going forward such multiple PRs won't be acceptable. It creates friction for us maintainers. Thank you for understanding.

Okay, I will try from the next time. 😇

Thanks for considering. 🙏

@muddlebee muddlebee merged commit 2b82e03 into Tracer-Cloud:main May 2, 2026
10 checks passed
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 2, 2026

🤖 CI passed. Linter didn't scream. Reviewer typed LGTM. @SB2318, every machine in this pipeline just slow-clapped. 🖥️✨

👋 Join us on Discord - OpenSRE : hang out, contribute, or hunt for features and issues. Everyone's welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Collapse the current two-step ingest flow into one explicit helper

2 participants

@SB2318 @muddlebee