Skip to content

feat: provider-agnostic push outputs for review (stdout/file/webhook/slack)#2510

Open
shmulc8 wants to merge 3 commits into
The-PR-Agent:mainfrom
shmulc8:feat/push-outputs-slack
Open

feat: provider-agnostic push outputs for review (stdout/file/webhook/slack)#2510
shmulc8 wants to merge 3 commits into
The-PR-Agent:mainfrom
shmulc8:feat/push-outputs-slack

Conversation

@shmulc8

@shmulc8 shmulc8 commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Description

Resolves #2025 — push PR-Agent review output to external sinks (Slack, a file, or a generic webhook) without calling any git-provider API.

This is a leaner take on the same idea as #2043: it drops the bundled FastAPI relay server. Slack Incoming Webhooks accept {"text": ...} directly, so a dedicated "slack" channel POSTs to the webhook itself — no extra service to deploy.

What changed

  • pr_agent/algo/utils.pypush_outputs(message_type, payload, markdown). No-op unless [push_outputs].enable is true. Channels:

    • stdout — one JSON line
    • file — append a JSONL record
    • webhook — POST the generic record to webhook_url
    • slack — POST {"text": markdown} to a Slack Incoming Webhook (slack_webhook_url)

    Non-fatal: any failure is logged as a warning and never interrupts the run.

  • pr_agent/settings/configuration.toml — new [push_outputs] section, disabled by default.

  • pr_agent/tools/pr_reviewer.py — emits the review after the markdown is built, independent of config.publish_output (so you still get a Slack ping even when the PR comment is suppressed, e.g. "no major issues").

  • tests/unittest/test_push_outputs.py — disabled-by-default no-op, file channel, slack channel payload, and non-fatal error handling.

Meets the issue requirements

  • Provider-agnostic — no provider APIs touched; works across GitHub/GitLab/Bitbucket/Azure/etc.
  • Minimal API calls — zero for stdout/file; one POST for webhook/slack.
  • Generic presentation — the record carries type, ISO-UTC timestamp, structured payload, and human-readable markdown.
  • Off by default — no behavior change unless explicitly enabled.

Configuration example

[push_outputs]
enable = true
channels = ["slack"]
slack_webhook_url = "https://hooks.slack.com/services/T.../B.../..."

Scope / follow-ups

  • Wired into /review (the issue's primary motivation). Extending to /describe, /improve, /ask is a one-line push_outputs(...) call at each tool's final-output site — happy to add if maintainers want it in this PR.

🤖 Generated with Claude Code

…slack)

Emit /review output to external sinks without calling any git-provider API,
gated behind a disabled-by-default [push_outputs] config section. Channels:
stdout, file (JSONL), generic webhook, and slack (POST {"text": ...} to a
Slack Incoming Webhook — no relay service needed). Non-fatal on error.

Resolves The-PR-Agent#2025

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VcpPDpXKuSesj5xoFqJyfS
@github-actions github-actions Bot added the feature 💡 label Jul 6, 2026
@shmulc8 shmulc8 requested a review from naorpeled July 6, 2026 20:30
@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Add provider-agnostic push outputs for /review (stdout/file/webhook/slack)

✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 20-40 Minutes

Grey Divider

AI Description

• Add push_outputs helper to emit review results to stdout, file, webhook, or Slack.
• Introduce disabled-by-default [push_outputs] configuration to control channels and destinations.
• Emit /review markdown to configured sinks and add unit tests for channels and failures.
Diagram

graph TD
  A["pr_reviewer.py (/review)"] --> B["utils.push_outputs"]
  C["configuration.toml"] --> D["Dynaconf settings"] --> B["utils.push_outputs"]
  B["utils.push_outputs"] --> E["stdout (JSON line)"]
  B["utils.push_outputs"] --> F["file (JSONL append)"]
  B["utils.push_outputs"] --> G{{"Webhook endpoint"}}
  B["utils.push_outputs"] --> H{{"Slack Incoming Webhook"}}
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Output-sink plugin/registry interface
  • ➕ Keeps utils.py minimal by delegating channel implementations to dedicated classes/modules
  • ➕ Makes it easier to add new sinks (e.g., S3, Kafka, Teams) without growing a single function
  • ➖ More upfront abstraction/boilerplate for a small number of sinks
  • ➖ Harder to keep configuration simple and discoverable
2. Leverage Python logging + handlers
  • ➕ Reuses a mature sink ecosystem (file, HTTP, syslog, etc.)
  • ➕ Centralizes retry/backoff/formatting concerns in logging configuration
  • ➖ Harder to guarantee a stable structured schema (type/timestamp/payload/markdown)
  • ➖ Slack-specific payload needs custom handler anyway; more ops-heavy configuration

Recommendation: The current approach (single best-effort push_outputs(...) gated by [push_outputs]) is a good fit for the stated goal: provider-agnostic, minimal moving parts, and no extra service to deploy. If additional tools/sinks are planned soon, consider evolving this into a small sink registry to prevent utils.push_outputs from becoming a monolith.

Files changed (4) +119 / -2

Enhancement (2) +50 / -2
utils.pyAdd best-effort 'push_outputs' utility and UTC timestamps +45/-1

Add best-effort 'push_outputs' utility and UTC timestamps

• Introduces 'push_outputs(message_type, payload, markdown)' to emit structured tool output records to stdout, JSONL file, generic webhook, or Slack webhook. Uses UTC ISO timestamps and wraps all sink work in a non-fatal try/except to avoid breaking runs when outputs fail.

pr_agent/algo/utils.py

pr_reviewer.pyEmit '/review' markdown to external sinks after rendering +5/-1

Emit '/review' markdown to external sinks after rendering

• Calls 'push_outputs("review", payload=..., markdown=...)' after the final markdown is built (and after optional relevant-config output), enabling Slack/webhook/file/stdout emission independent of git-provider comment publishing behavior.

pr_agent/tools/pr_reviewer.py

Tests (1) +62 / -0
test_push_outputs.pyAdd unit tests for push outputs channels and failure behavior +62/-0

Add unit tests for push outputs channels and failure behavior

• Covers disabled-by-default no-op behavior, JSONL file append semantics, Slack webhook payload shape, and verifies network errors are non-fatal by ensuring exceptions are swallowed.

tests/unittest/test_push_outputs.py

Other (1) +7 / -0
configuration.tomlAdd disabled-by-default '[push_outputs]' configuration section +7/-0

Add disabled-by-default '[push_outputs]' configuration section

• Defines the new 'push_outputs' config with 'enable', 'channels', and per-channel settings (file path, webhook URL, Slack webhook URL). Defaults keep behavior off unless explicitly enabled.

pr_agent/settings/configuration.toml

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (4) 📎 Requirement gaps (1) 📜 Skill insights (0)

Context used

Grey Divider


Action required

1. push_outputs unvalidated webhook URLs 📘 Rule violation ⛨ Security
Description
push_outputs sends HTTP POST requests to webhook_url/slack_webhook_url from config without
validating scheme/host or disabling redirects, and then masks all exceptions. This creates SSRF risk
and can also leak secret-bearing URLs via exception text in logs.
Code

pr_agent/algo/utils.py[R1307-1315]

+        if "webhook" in channels and cfg.get('webhook_url'):
+            requests.post(cfg['webhook_url'], json=record, timeout=5)
+
+        # Slack Incoming Webhooks accept {"text": ...} directly — no relay service needed.
+        if "slack" in channels and cfg.get('slack_webhook_url'):
+            text = markdown if markdown is not None else json.dumps(payload or {}, ensure_ascii=False)
+            requests.post(cfg['slack_webhook_url'], json={"text": text}, timeout=5)
+    except Exception as e:
+        get_logger().warning(f"push_outputs failed: {e}")
Evidence
Rule 23 requires validating configuration inputs used in security-sensitive network paths, avoiding
broad exception masking, and not logging secrets. The code posts to
cfg['webhook_url']/cfg['slack_webhook_url'] without validation and logs Exception text, which
may include the full URL (Slack webhook URLs are secret-bearing).

pr_agent/algo/utils.py[1307-1315]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`push_outputs()` performs network calls to config-provided URLs without validation/constraints and catches all exceptions while logging the raw exception message, which can contain secret-bearing URLs.
## Issue Context
This feature intentionally reads webhook URLs from configuration, which should be treated as a security boundary. Network egress to arbitrary URLs should be constrained/validated and logging should not expose sensitive URL paths.
## Fix Focus Areas
- pr_agent/algo/utils.py[1307-1315]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Tests leak global settings ✓ Resolved 📘 Rule violation ☼ Reliability
Description
The new tests mutate the global Dynaconf settings via get_settings().set(...) without
snapshotting/restoring prior values. This can cause order-dependent/flaky behavior across the test
suite.
Code

tests/unittest/test_push_outputs.py[R8-22]

+    def test_disabled_by_default_is_noop(self, monkeypatch, tmp_path):
+        get_settings().set('PUSH_OUTPUTS.ENABLE', False)
+        get_settings().set('PUSH_OUTPUTS.CHANNELS', ['file'])
+        get_settings().set('PUSH_OUTPUTS.FILE_PATH', str(tmp_path / 'out.jsonl'))
+
+        push_outputs("review", payload={"a": 1}, markdown="hi")
+
+        assert not (tmp_path / 'out.jsonl').exists()
+
+    def test_file_channel_appends_jsonl(self, monkeypatch, tmp_path):
+        out = tmp_path / 'nested' / 'out.jsonl'
+        get_settings().set('PUSH_OUTPUTS.ENABLE', True)
+        get_settings().set('PUSH_OUTPUTS.CHANNELS', ['file'])
+        get_settings().set('PUSH_OUTPUTS.FILE_PATH', str(out))
+
Evidence
Rule 22 requires snapshotting/restoring mutated global/module state in tests. These tests set
multiple PUSH_OUTPUTS.* keys but never restore previous values after each test.

tests/unittest/test_push_outputs.py[8-22]
tests/unittest/test_push_outputs.py[33-54]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`tests/unittest/test_push_outputs.py` mutates global settings and does not restore them, which can leak state into other tests.
## Issue Context
`get_settings()` returns a shared `global_settings` Dynaconf instance when no request context exists, so `.set(...)` persists across tests unless explicitly restored.
## Fix Focus Areas
- tests/unittest/test_push_outputs.py[8-62]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. String "false" enables outputs ✓ Resolved 🐞 Bug ⛨ Security
Description
push_outputs() treats cfg['enable'] as a boolean without normalizing string values, so an env/config
value like "false" is truthy and can unexpectedly emit review data to stdout/file/webhook/slack.
This can cause unintended data disclosure when operators believe outputs are disabled.
Code

pr_agent/algo/utils.py[R1282-1284]

+        cfg = get_settings().get('push_outputs', {}) or {}
+        if not cfg.get('enable', False):
+            return
Evidence
push_outputs() gates only on cfg.get('enable') with no string coercion, while
github_action_output() explicitly coerces string booleans (and tests rely on this env-string
behavior). This demonstrates that env-loaded values can be strings and that a missing coercion here
can flip the feature on unintentionally.

pr_agent/algo/utils.py[1274-1285]
pr_agent/algo/utils.py[1258-1264]
tests/unittest/test_github_action_output.py[45-59]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`push_outputs()` checks `cfg['enable']` directly; if the value is a non-empty string (e.g. loaded from env), Python treats it as truthy and the feature turns on even when set to "false".
### Issue Context
There is an established pattern in this codebase to normalize string booleans before gating output emission.
### Fix Focus Areas
- pr_agent/algo/utils.py[1274-1285]
- pr_agent/algo/utils.py[1258-1264]
### Suggested fix
Mirror `github_action_output()` logic:
- Read `enable = cfg.get('enable', False)`
- If `isinstance(enable, str)`, coerce via `.lower().strip()` and treat ("false", "0", "no", "") as disabled
- Gate on the coerced boolean

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

4. Hardcoded timeout=5 and path 📘 Rule violation ⚙ Maintainability ⭐ New
Description
push_outputs() hardcodes network timeouts and a file output default path in code, duplicating
behavioral defaults that should be centralized in settings to support documented overrides and avoid
scattered constants.
Code

pr_agent/algo/utils.py[R1302-1317]

+            file_path = cfg.get('file_path', 'pr-agent-outputs/reviews.jsonl')
+            folder = os.path.dirname(file_path)
+            if folder:
+                os.makedirs(folder, exist_ok=True)
+            with open(file_path, 'a', encoding='utf-8') as fh:
+                fh.write(json.dumps(record, ensure_ascii=False) + "\n")
+
+        # ponytail: local channels first, network last, so a failed POST can't lose a file write.
+        # allow_redirects=False: never follow a redirect from a configured sink to another host.
+        if "webhook" in channels and cfg.get('webhook_url'):
+            requests.post(cfg['webhook_url'], json=record, timeout=5, allow_redirects=False)
+
+        # Slack Incoming Webhooks accept {"text": ...} directly — no relay service needed.
+        if "slack" in channels and cfg.get('slack_webhook_url'):
+            text = markdown if markdown is not None else json.dumps(payload or {}, ensure_ascii=False)
+            requests.post(cfg['slack_webhook_url'], json={"text": text}, timeout=5, allow_redirects=False)
Evidence
Rule 11 requires new behavior/tunables to be controlled via documented configuration sources rather
than hardcoded literals. The new code introduces hardcoded defaults for file_path and timeout=5
directly in push_outputs().

AGENTS.md: Do Not Hardcode Configuration Values; Use .pr_agent.toml or pr_agent/settings
pr_agent/algo/utils.py[1302-1317]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`push_outputs()` contains hardcoded operational defaults (file path fallback and HTTP timeout) instead of sourcing these values from the documented configuration system.

## Issue Context
The project expects tunables/defaults to live in `pr_agent/settings/configuration.toml` (and be overrideable via Dynaconf sources) rather than being duplicated as literals in business logic.

## Fix Focus Areas
- pr_agent/algo/utils.py[1302-1317]
- pr_agent/settings/configuration.toml[408-413]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Unlocked JSONL append 🐞 Bug ☼ Reliability ⭐ New
Description
The "file" channel in push_outputs() appends JSONL records without any inter-process/thread
coordination, so concurrent runs can race and occasionally produce partial/interleaved lines that
break JSONL parsing. This is plausible in PR-Agent’s webhook servers where multiple requests can be
processed concurrently and share the same output file path.
Code

pr_agent/algo/utils.py[R1301-1307]

+        if "file" in channels:
+            file_path = cfg.get('file_path', 'pr-agent-outputs/reviews.jsonl')
+            folder = os.path.dirname(file_path)
+            if folder:
+                os.makedirs(folder, exist_ok=True)
+            with open(file_path, 'a', encoding='utf-8') as fh:
+                fh.write(json.dumps(record, ensure_ascii=False) + "\n")
Evidence
The file sink appends JSON text to a shared path using a plain open(..., 'a') + write(...) with
no locking. Separately, PR-Agent’s webhook server processes incoming events asynchronously (FastAPI
BackgroundTasks), making concurrent executions of review generation—and thus
push_outputs()—plausible against the same output file.

pr_agent/algo/utils.py[1301-1307]
pr_agent/servers/github_app.py[38-54]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`push_outputs()` writes JSONL by opening the configured file in append mode and calling `fh.write(...)` with no locking. In concurrent deployments (webhook servers / multiple workers), multiple writers can contend for the same file, which can lead to occasionally corrupted JSONL (partial/interleaved lines) and downstream ingestion failures.

## Issue Context
PR-Agent includes FastAPI webhook servers that can process multiple webhook events concurrently; if `push_outputs.channels` includes `"file"`, all concurrent runs will append to the same configured path.

## Fix Focus Areas
- pr_agent/algo/utils.py[1301-1307]

## Suggested fix
Implement a concurrency-safe append strategy for the JSONL sink, e.g.:
- Use a cross-process file lock (best-effort `fcntl.flock` on Unix, `msvcrt.locking` on Windows), or
- Write via a single low-level `os.open(..., O_APPEND)` + single `os.write(...)` of UTF-8 bytes (minimizes interleaving risk), or
- Optionally support per-run/per-request output files to avoid contention.

Keep failures non-fatal, but ensure the file sink produces valid JSONL under concurrent writers.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. Slack message hardcoded to markdown 📎 Requirement gap ⚙ Maintainability
Description
The Slack channel always posts {"text": markdown} (or JSON-dumped payload) with no configurable
template/blocks/fields, making the notification format hard to adapt without code changes.
Code

pr_agent/algo/utils.py[R1313-1316]

+        # Slack Incoming Webhooks accept {"text": ...} directly — no relay service needed.
+        if "slack" in channels and cfg.get('slack_webhook_url'):
+            text = markdown if markdown is not None else json.dumps(payload or {}, ensure_ascii=False)
+            requests.post(cfg['slack_webhook_url'], json={"text": text}, timeout=5)
Evidence
PR Compliance ID 8 requires a generic, easily adaptable Slack message format. The implementation
posts a fixed Slack payload ({"text": text}) derived directly from markdown/payload with no
configuration-driven template or structure customization.

Slack message format is generic and easy to adapt
pr_agent/algo/utils.py[1313-1316]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Slack notifications are currently hardcoded to `{"text": ...}` derived from `markdown` (or a JSON dump of `payload`). This makes it difficult for teams to customize wording/fields/layout without changing code.
## Issue Context
Compliance requires Slack message format to be generic and easy to adapt.
## Fix Focus Areas
- pr_agent/algo/utils.py[1313-1316]
- pr_agent/settings/configuration.toml[408-413]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (4)
7. channels config not validated 📘 Rule violation ⛨ Security
Description
channels is consumed directly from configuration without validating its type/values, which can
lead to unexpected behavior (e.g., a string behaving as an iterable) and makes the feature more
error-prone to misconfiguration.
Code

pr_agent/algo/utils.py[R1289-1290]

+        channels = cfg.get('channels', []) or []
+        record = {
Evidence
PR Compliance ID 26 requires validating/normalizing configuration inputs. The code reads `channels =
cfg.get('channels', []) without type-checking or normalizing to list[str]` before using it to
drive output behavior.

pr_agent/algo/utils.py[1289-1290]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`push_outputs()` reads `channels` from config and immediately performs membership checks without validating/normalizing the value. Mis-typed configuration (e.g., `channels = "slack"` as a string, unknown channel names, non-list values) can produce surprising behavior.
## Issue Context
Compliance requires treating configuration inputs as a security boundary, including validation/normalization of config values.
## Fix Focus Areas
- pr_agent/algo/utils.py[1289-1290]
- pr_agent/settings/configuration.toml[408-413]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


8. Channel failure stops others 🐞 Bug ☼ Reliability
Description
push_outputs() wraps all channel emissions in a single try/except, so an exception in an earlier
sink (stdout/file/webhook) aborts the rest and prevents other configured sinks from receiving the
same review. This undermines multi-channel “best-effort” delivery (e.g., a failing webhook can also
prevent Slack from being attempted).
Code

pr_agent/algo/utils.py[R1281-1318]

+    try:
+        cfg = get_settings().get('push_outputs', {}) or {}
+        enable = cfg.get('enable', False)
+        if isinstance(enable, str):  # env vars arrive as strings; treat "false"/"0"/"no"/"" as off
+            enable = enable.lower().strip() not in ("false", "0", "no", "")
+        if not enable:
+            return
+
+        channels = cfg.get('channels', []) or []
+        record = {
+            "type": message_type,
+            "timestamp": datetime.now(timezone.utc).isoformat(),
+            "payload": payload or {},
+        }
+        if markdown is not None:
+            record["markdown"] = markdown
+
+        if "stdout" in channels:
+            print(json.dumps(record, ensure_ascii=False))
+
+        if "file" in channels:
+            file_path = cfg.get('file_path', 'pr-agent-outputs/reviews.jsonl')
+            folder = os.path.dirname(file_path)
+            if folder:
+                os.makedirs(folder, exist_ok=True)
+            with open(file_path, 'a', encoding='utf-8') as fh:
+                fh.write(json.dumps(record, ensure_ascii=False) + "\n")
+
+        # ponytail: local channels first, network last, so a failed POST can't lose a file write.
+        if "webhook" in channels and cfg.get('webhook_url'):
+            requests.post(cfg['webhook_url'], json=record, timeout=5)
+
+        # Slack Incoming Webhooks accept {"text": ...} directly — no relay service needed.
+        if "slack" in channels and cfg.get('slack_webhook_url'):
+            text = markdown if markdown is not None else json.dumps(payload or {}, ensure_ascii=False)
+            requests.post(cfg['slack_webhook_url'], json={"text": text}, timeout=5)
+    except Exception as e:
+        get_logger().warning(f"push_outputs failed: {e}")
Evidence
The code shows a single try covering all four sinks; any exception in one sink prevents execution
of later sink blocks. The comment about ordering sinks (local before network) further suggests sinks
are intended to be attempted independently, which the current control-flow does not guarantee.

pr_agent/algo/utils.py[1274-1319]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`push_outputs()` uses one outer `try/except` around stdout/file/webhook/slack blocks. In Python, the first exception exits the `try` body immediately, so later channels are skipped.
### Issue Context
The implementation comments indicate an intent for independent best-effort delivery (local sinks first, network last), but a failure in any earlier sink currently prevents subsequent sinks from running.
### Fix Focus Areas
- pr_agent/algo/utils.py[1274-1319]
### Suggested fix
- Keep record construction under one try (or validate inputs up front), then wrap **each channel** (`stdout`, `file`, `webhook`, `slack`) in its own `try/except`.
- On per-channel failure, log a warning that includes the channel name (and ideally the exception info), then continue to the next channel.
- Optionally, keep a final broad `except` as a last resort, but avoid having it cover the whole multi-channel emission path.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


9. push_outputs ignores HTTP failures 📘 Rule violation ☼ Reliability
Description
The webhook/Slack POST requests in push_outputs() ignore the returned HTTP response and never
check response.status_code or call raise_for_status(), so 4xx/5xx failures can be silently
missed. This reduces observability, violates the requirement to explicitly handle error scenarios,
and breaks the intended behavior that any delivery failure is logged as a warning while remaining
non-fatal.
Code

pr_agent/algo/utils.py[R1307-1313]

+        if "webhook" in channels and cfg.get('webhook_url'):
+            requests.post(cfg['webhook_url'], json=record, timeout=5)
+
+        # Slack Incoming Webhooks accept {"text": ...} directly — no relay service needed.
+        if "slack" in channels and cfg.get('slack_webhook_url'):
+            text = markdown if markdown is not None else json.dumps(payload or {}, ensure_ascii=False)
+            requests.post(cfg['slack_webhook_url'], json={"text": text}, timeout=5)
Evidence
Rule 3 requires explicit handling of error scenarios, but the cited code path performs
requests.post(...) calls and discards the returned Response, relying only on an outer exception
handler for warnings. Since requests does not raise exceptions for non-2xx HTTP statuses by
default, a 4xx/5xx response will neither throw nor be inspected, so no warning is emitted and
delivery failures can pass without any signal.

Rule 3: Robust Error Handling
pr_agent/algo/utils.py[1307-1313]
pr_agent/algo/utils.py[1307-1315]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`requests.post(...)` return values are ignored in `push_outputs()`, so non-2xx HTTP responses (4xx/5xx) won’t be detected or logged; only exceptions are currently logged, and HTTP error statuses don’t raise by default.
## Issue Context
The feature is intended to be non-fatal, but delivery failures must still be surfaced via warnings (per the stated expectation that failures are logged and per the requirement to handle error scenarios explicitly). Ensure missed webhook/Slack notifications are observable by logging warnings on HTTP error responses while continuing execution.
## Fix Focus Areas
- pr_agent/algo/utils.py[1307-1315]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


10. Slack webhook URL in tests ✓ Resolved 📘 Rule violation ⛨ Security
Description
The test commits a Slack Incoming Webhook-shaped URL (https://hooks.slack.com/services/...), which
can be treated as a secret-like pattern by scanners and violates the no-committed-secrets
requirement. Use a non-secret placeholder domain (e.g., https://example.invalid/...) instead.
Code

tests/unittest/test_push_outputs.py[36]

+        get_settings().set('PUSH_OUTPUTS.SLACK_WEBHOOK_URL', 'https://hooks.slack.com/services/T/B/X')
Evidence
Rule 12 prohibits committing secrets or secret-like credential patterns. The test includes a URL
matching Slack Incoming Webhook format, which is commonly treated as sensitive and flagged by secret
scanners.

AGENTS.md, CLAUDE.md: No Hardcoded or Committed Secrets; Use Environment Variables or Secrets Files: AGENTS.md, CLAUDE.md: No Hardcoded or Committed Secrets; Use Environment Variables or Secrets Files: AGENTS.md, CLAUDE.md: No Hardcoded or Committed Secrets; Use Environment Variables or Secrets Files: AGENTS.md, CLAUDE.md: No Hardcoded or Committed Secrets; Use Environment Variables or Secrets Files: AGENTS.md, CLAUDE.md: No Hardcoded or Committed Secrets; Use Environment Variables or Secrets Files: AGENTS.md, CLAUDE.md: No Hardcoded or Committed Secrets; Use Environment Variables or Secrets Files: AGENTS.md, CLAUDE.md: No Hardcoded or Committed Secrets; Use Environment Variables or Secrets Files: AGENTS.md, CLAUDE.md: No Hardcoded or Committed Secrets; Use Environment Variables or Secrets Files: AGENTS.md, CLAUDE.md: No Hardcoded or Committed Secrets; Use Environment Variables or Secrets Files: AGENTS.md, CLAUDE.md: No Hardcoded or Committed Secrets; Use Environment Variables or Secrets Files: AGENTS.md, CLAUDE.md: No Hardcoded or Committed Secrets; Use Environment Variables or Secrets Files: AGENTS.md, CLAUDE.md: No Hardcoded or Committed Secrets; Use Environment Variables or Secrets Files: AGENTS.md, CLAUDE.md: No Hardcoded or Committed Secrets; Use Environment Variables or Secrets Files: AGENTS.md, CLAUDE.md: No Hardcoded or Committed Secrets; Use Environment Variables or Secrets Files: AGENTS.md, CLAUDE.md: No Hardcoded or Committed Secrets; Use Environment Variables or Secrets Files: AGENTS.md, CLAUDE.md: No Hardcoded or Committed Secrets; Use Environment Variables or Secrets Files
tests/unittest/test_push_outputs.py[34-37]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A Slack webhook-shaped URL is committed in a test, which can be flagged as a secret pattern.
## Issue Context
Even if the value is fake, secret scanners often alert on Slack webhook URL formats.
## Fix Focus Areas
- tests/unittest/test_push_outputs.py[34-37]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

  • Author self-review: I have reviewed the code review findings, and addressed the relevant ones.

Qodo Logo

Comment thread pr_agent/algo/utils.py Outdated
Comment thread tests/unittest/test_push_outputs.py
Comment thread pr_agent/algo/utils.py
- Treat env-supplied "false"/"0"/"no"/"" as disabled (mirrors github_action_output),
  so push_outputs can't be turned on by a string-valued enable flag.
- Add teardown fixture to restore global settings after each test.
- Use an obviously-fake webhook host in the slack test to avoid secret-scanner noise.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VcpPDpXKuSesj5xoFqJyfS
@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit bce2968

A repo's .pr_agent.toml could otherwise enable push_outputs and set
webhook_url/slack_webhook_url/file_path, allowing a malicious repo on a
shared PR-Agent host to exfiltrate review data to an arbitrary host, reach
internal endpoints (SSRF), or append to arbitrary host files.

- Add [push_outputs] to the host-only allowlist with an empty key set, so
  the whole section can only be configured by the operator (same mechanism
  as skills.paths).
- allow_redirects=False on the webhook/slack POSTs.
- Log only the exception type on failure; requests errors embed the
  secret-bearing URL in their message.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VcpPDpXKuSesj5xoFqJyfS
@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 9d4616b

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable Slack Notifications for Newly Reviewed PRs or Any PR-Agent Response

2 participants