refactor(publish): extract GitLab MR write-back into dedicated helper module by udit-rawat · Pull Request #1026 · Tracer-Cloud/opensre

udit-rawat · 2026-04-28T07:46:40Z

Problem

app/nodes/publish_findings/node.py was doing too many things in one file — rendering, ingest, Slack, Discord, Telegram, and GitLab MR write-back all inline. The GitLab branch was 20+ lines including a _build_mr_note() helper, making the node hard to test and maintain.

Solution

Extracted _build_mr_note() and the entire GitLab write-back branch into app/nodes/publish_findings/gitlab_writeback.py behind a single post_gitlab_mr_writeback() function. The publish node now calls that one function instead of inlining the logic. GITLAB_MR_WRITEBACK env-flag behavior and failure handling are unchanged.

Tests

Added tests/nodes/publish_findings/test_gitlab_writeback.py covering:

env flag off → no-op
missing MR IID → no-op
missing project ID → no-op
write-back failure → does not propagate
happy path → post_gitlab_mr_note called with correct args

Closes #869

… module

greptile-apps · 2026-04-28T07:50:05Z

Greptile Summary

This PR extracts the GitLab MR write-back logic from node.py into a dedicated gitlab_writeback.py module, reducing the publish node to a single post_gitlab_mr_writeback(state, slack_message) call. Behavior, env-flag handling, and failure suppression are preserved exactly; five new unit tests cover the key no-op and happy-path scenarios.

Confidence Score: 5/5

Safe to merge — pure refactoring with no functional changes and good test coverage.

All findings are P2 (style/naming/test-quality suggestions). The extraction is mechanically correct, the env-flag guard and failure-suppression semantics are unchanged, and the new tests exercise the important paths.

tests/nodes/publish_findings/test_gitlab_writeback.py — the failure-propagation test has no assertions.

Important Files Changed

Filename	Overview
app/nodes/publish_findings/gitlab_writeback.py	New module correctly extracts _build_mr_note and post_gitlab_mr_writeback; behavior is functionally identical to the inlined original, though the import is now eager instead of lazy.
app/nodes/publish_findings/node.py	Correctly swaps the 26-line inline GitLab block for a single post_gitlab_mr_writeback call; removes the now-unused os import.
tests/nodes/publish_findings/test_gitlab_writeback.py	Good coverage of env-flag, missing-ID, happy-path, and truncation cases; failure test lacks an assertion that the warning is actually logged.

Sequence Diagram

sequenceDiagram
    participant N as node.py::generate_report
    participant GW as gitlab_writeback.py::post_gitlab_mr_writeback
    participant ENV as os.getenv
    participant GL as app.integrations.gitlab

    N->>GW: post_gitlab_mr_writeback(state, slack_message)
    GW->>ENV: getenv("GITLAB_MR_WRITEBACK")
    alt flag not truthy
        GW-->>N: return (no-op)
    else flag truthy
        GW->>GW: extract merge_request_iid, project_id from state
        alt missing iid or project_id
            GW-->>N: return (no-op)
        else both present
            GW->>GL: build_gitlab_config(...)
            GW->>GL: post_gitlab_mr_note(config, project_id, mr_iid, body)
            alt success
                GL-->>GW: note dict
                GW->>GW: logger.info(...)
            else exception
                GW->>GW: logger.warning(...)
            end
            GW-->>N: return
        end
    end

_{Reviews (1): Last reviewed commit: "refactor(publish): extract GitLab MR wri..." | Re-trigger Greptile}

greptile-apps · 2026-04-28T07:50:09Z

+import logging
+import os
+
+from app.integrations.gitlab import build_gitlab_config, post_gitlab_mr_note


Eager top-level import replaces formerly lazy in-try import

In the original node.py, the build_gitlab_config / post_gitlab_mr_note imports lived inside the try block and would therefore be caught by the except Exception handler if the module or its transitive dependencies failed to load. Now they are a top-level import of gitlab_writeback.py, which in turn is imported at the top of node.py. Any import-time failure in app.integrations.gitlab will now surface as an unhandled ImportError at process startup rather than a logged warning at call-time.

In practice app.integrations.gitlab only uses httpx and pydantic (both core deps), so this is unlikely to bite today, but it's a subtle erosion of the defensive isolation the original pattern provided.

greptile-apps · 2026-04-28T07:50:10Z

+    return f"### RCA Finding\n\n<details>\n<summary>Investigation summary</summary>\n\n{body}\n\n</details>"
+
+
+def post_gitlab_mr_writeback(state: InvestigationState, slack_message: str) -> None:


slack_message parameter name is misleading inside a GitLab module

The parameter carries the Slack-formatted report string and is named slack_message throughout the call chain. Naming it report or report_body in this module would make the public API of the helper self-explanatory without requiring callers to know that the GitLab note body originates from the Slack formatter.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

greptile-apps · 2026-04-28T07:50:11Z

+def test_failure_does_not_propagate(state_with_gitlab):
+    with (
+        patch("app.nodes.publish_findings.gitlab_writeback.os.getenv", return_value="true"),
+        patch(
+            "app.nodes.publish_findings.gitlab_writeback.post_gitlab_mr_note",
+            side_effect=RuntimeError("network error"),
+        ),
+        patch(
+            "app.nodes.publish_findings.gitlab_writeback.build_gitlab_config",
+            return_value=MagicMock(),
+        ),
+    ):
+        post_gitlab_mr_writeback(state_with_gitlab, "report")


Failure test has no assertions

test_failure_does_not_propagate verifies only that no exception escapes — it doesn't check that the warning is actually emitted. A future refactor could accidentally silence the log (e.g. change logger.warning → pass) and the test would still pass. Consider adding:

with patch("app.nodes.publish_findings.gitlab_writeback.logger") as mock_logger: post_gitlab_mr_writeback(state_with_gitlab, "report") mock_logger.warning.assert_called_once()

muddlebee · 2026-04-28T07:50:48Z

+    _gl = (state.get("available_sources") or {}).get("gitlab", {})
+    _mr_iid = _gl.get("merge_request_iid", "")
+    _project_id = _gl.get("project_id", "")
+


Underscore prefix on local variables (_gl, _mr_iid, _project_id) is noise — that convention is for module-level private names, not locals. Drop the underscores.

muddlebee · 2026-04-28T07:51:00Z

+    assert "x" * 3997 + "..." in note
+    assert "x" * 3998 not in note
+
+


Patching os.getenv globally replaces every getenv call in the module for the duration of the test. Use patch.dict(os.environ, {"GITLAB_MR_WRITEBACK": "false"}) instead — it only touches the one key and is safer if more env-var reads get added later.

muddlebee · 2026-04-28T07:51:12Z

+    state = {"available_sources": {"gitlab": {"project_id": "99"}}}
+    with (
+        patch("app.nodes.publish_findings.gitlab_writeback.os.getenv", return_value="true"),
+        patch("app.nodes.publish_findings.gitlab_writeback.post_gitlab_mr_note") as mock_post,


Same os.getenv patching concern as the first test. Applies to all five test functions — swap all of them to patch.dict for consistency.

…atch.dict for env, assert logger.warning on failure

udit-rawat · 2026-04-28T08:04:17Z

@muddlebee all review comments addressed — dropped underscore locals, renamed param to report, switched all env patches to patch.dict, and added the logger.warning assertion to the failure test. Happy to make any further adjustments if needed.

muddlebee · 2026-04-28T08:21:44Z

@udit-rawat thank you

refactor(publish): extract GitLab MR write-back into dedicated helper…

0e084f9

… module

greptile-apps Bot reviewed Apr 28, 2026

View reviewed changes

fix test_node.py import: _build_mr_note moved to gitlab_writeback module

f8d544c

muddlebee reviewed Apr 28, 2026

View reviewed changes

udit-rawat added 2 commits April 28, 2026 13:26

address review: drop underscore locals, rename param to report, use p…

a8d2151

…atch.dict for env, assert logger.warning on failure

fix ruff formatting in test_node.py

960ea22

muddlebee merged commit 566acf0 into Tracer-Cloud:main Apr 28, 2026
7 checks passed

udit-rawat mentioned this pull request Apr 28, 2026

Extract repeated _truncate() implementations into a shared app/utils/truncation.py helper #1056

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(publish): extract GitLab MR write-back into dedicated helper module#1026

refactor(publish): extract GitLab MR write-back into dedicated helper module#1026
muddlebee merged 4 commits intoTracer-Cloud:mainfrom
udit-rawat:fix/extract-gitlab-mr-writeback

udit-rawat commented Apr 28, 2026

Uh oh!

greptile-apps Bot commented Apr 28, 2026

Uh oh!

greptile-apps Bot Apr 28, 2026

Uh oh!

greptile-apps Bot Apr 28, 2026

Uh oh!

greptile-apps Bot Apr 28, 2026

Uh oh!

muddlebee Apr 28, 2026

Uh oh!

muddlebee Apr 28, 2026

Uh oh!

muddlebee Apr 28, 2026

Uh oh!

udit-rawat commented Apr 28, 2026

Uh oh!

Uh oh!

muddlebee commented Apr 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		return f"### RCA Finding\n\n<details>\n<summary>Investigation summary</summary>\n\n{body}\n\n</details>"


		def post_gitlab_mr_writeback(state: InvestigationState, slack_message: str) -> None:

		assert "x" * 3997 + "..." in note
		assert "x" * 3998 not in note

Conversation

udit-rawat commented Apr 28, 2026

Problem

Solution

Tests

Uh oh!

greptile-apps Bot commented Apr 28, 2026

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps Bot Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

muddlebee Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

muddlebee Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

muddlebee Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

udit-rawat commented Apr 28, 2026

Uh oh!

Uh oh!

muddlebee commented Apr 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants