-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
.pr_agent_accepted_suggestions
| PR 2610 (2026-08-08) |
[correctness] Falsy config bypasses validation
Falsy config bypasses validation
`_resolve_cache_control_injection_points()` returns `None` for any falsy value before validating type, so malformed configs like `0`, `false`, or `{}` are silently treated as “unset” instead of raising `ValueError` as documented. This can hide operator misconfiguration and make caching unexpectedly not apply._resolve_cache_control_injection_points() currently uses if not cache_control_injection_points: return None, which treats any falsy value as “unset”. This bypasses the method’s own contract (“Raises ValueError on a malformed value”) for invalid-but-falsy types (e.g., 0, False, {}).
We still want backwards-compatible behavior for truly-unset/disabled values (e.g., None, "", []). But other falsy non-list values should be rejected deterministically.
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[517-526]
- Replace
if not cache_control_injection_points:with an explicit disabled-value check, e.g.: if cache_control_injection_points in (None, "", []): return None- Keep the existing JSON-string parsing and
listtype enforcement so invalid types always raiseValueError.
[reliability] Caching kwarg applied broadly
Caching kwarg applied broadly
When configured, `chat_completion()` injects `cache_control_injection_points` into `kwargs` for every model/provider, even though the feature is Anthropic-specific. In multi-provider deployments this broad injection can lead to provider-compatibility issues (the same function already avoids injecting other unsupported params to prevent provider-side rejection when `litellm.drop_params` is off).cache_control_injection_points is intended for Anthropic prompt caching, but it is currently added for any request as long as the config is set. This differs from other params in the same function that are guarded to avoid passing unsupported parameters to providers.
The codebase already contains logic acknowledging that some providers reject unsupported params when litellm.drop_params is off (see the user field handling). This setting should be scoped similarly to reduce unintended cross-provider impact.
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[708-744]
- Only inject
cache_control_injection_pointswhen the target model/provider is Anthropic-compatible. - Minimal heuristic:
if isinstance(model, str) and "claude" in model: ... - More robust: use
litellm.get_supported_openai_params(model=model)(already used nearby) and inject only if it reports support forcache_control_injection_points. - Optionally log a debug message when configured but skipped due to model/provider incompatibility.
| PR 2599 (2026-08-06) |
[performance] Eager comment materialization
Eager comment materialization
PRReviewer._get_user_answers() converts get_issue_comments() to a list before reversing, forcing full materialization of all issue comments even though it breaks as soon as it finds the latest Q/A. With paginated providers (e.g., GitHub), this can cause unnecessary API pagination, latency, and memory use on long threads.PRReviewer._get_user_answers() currently does reversed(list(discussion_messages)), which eagerly consumes the entire comment iterable before scanning newest-first. This defeats early-exit (break) and can force full pagination/network calls on providers that return lazy paginated iterables.
- The loop breaks as soon as it finds both the newest question and answer, so the iteration should ideally avoid fetching/allocating the entire history.
- Some providers return a true sequence (already reversible), while others return lazy/paginated iterables.
- pr_agent/tools/pr_reviewer.py[304-316]
- Prefer reversing without copying when possible:
- If the returned object supports
__reversed__, usereversed(discussion_messages). - Otherwise fall back to
reversed(list(discussion_messages)). - Optionally (if needed for compatibility with PyGithub shapes), check for a
.reversedattribute first and use it when present. Example pattern:
| PR 2598 (2026-08-05) |
[maintainability] `DEFAULT_DIAGRAM_DIRECTION*` duplicates defaults
`DEFAULT_DIAGRAM_DIRECTION*` duplicates defaults
`pr_diagram_direction` and `pr_diagram_direction_threshold` defaults are hardcoded in Python (`DEFAULT_DIAGRAM_DIRECTION*`) even though they are already defined in `pr_agent/settings/configuration.toml`, creating duplicated sources of truth that can drift. Prefer sourcing defaults only from Dynaconf settings to keep runtime behavior single-sourced.The defaults for pr_description.pr_diagram_direction and pr_description.pr_diagram_direction_threshold are defined both in pr_agent/settings/configuration.toml and again in Python (DEFAULT_DIAGRAM_DIRECTION, DEFAULT_DIAGRAM_DIRECTION_THRESHOLD). This duplication risks configuration drift and violates the single-source-of-truth configuration approach.
The Dynaconf settings already provide authoritative defaults via pr_agent/settings/configuration.toml. The Python code should not re-declare those same defaults; instead it should read the values from settings and pass them through.
- pr_agent/tools/pr_description.py[473-482]
- pr_agent/tools/pr_description.py[814-907]
- pr_agent/settings/configuration.toml[133-135]
[correctness] Edge labels parsed as nodes
Edge labels parsed as nodes
_strip_diagram_labels() only removes quoted and pipe-form labels, so an unquoted edge label between connectors can survive and be treated as a node ID by _parse_diagram_edges(). This can inflate the computed longest chain and cause apply_diagram_direction() to flip diagrams incorrectly (especially near the threshold).The edge parser strips only quoted ("...") and pipe (|...|) labels. If a diagram uses an unquoted edge label chunk between connectors, _parse_diagram_edges() can interpret that label text as a node ID, skewing longest-chain measurement and direction selection.
This PR’s adaptive direction feature depends on _parse_diagram_edges() producing a faithful edge list. Mis-parsing label text as nodes changes the path length calculation.
- pr_agent/tools/pr_description.py[824-864]
- Extend label stripping to remove/ignore unquoted edge-label segments between connectors (so they don’t become
node_groups). - Alternatively, replace the “split on connectors + search token” approach with a more explicit edge matcher that extracts only endpoint node IDs around connectors.
- Add a regression unit test for a line shaped like
A -- calls --> Bto ensure it yields only('A','B')and does not affect direction selection.
| PR 2587 (2026-08-01) |
[correctness] Token breakdown prints zeros
Token breakdown prints zeros
show_run_details() always renders "{prompt} in / {completion} out / {total} total" once any token usage exists, even when the provider only reported total_tokens (or omitted prompt/completion). This can display misleading "0 in / 0 out" values instead of omitting unreported fields, contradicting the new feature’s stated behavior.show_run_details() prints a full token breakdown (prompt/completion/total) whenever any token usage is present. Because RunDetails stores token counters as ints defaulting to 0, the code cannot distinguish “provider did not report prompt/completion tokens” from a real 0. If a provider reports only total_tokens, the output becomes 0 in / 0 out / N total.
This PR’s docs and RunDetails docstring say fields the provider does not report should be omitted rather than shown as zero. Current storage/renderer logic loses “presence” information.
- pr_agent/algo/run_details.py[20-108]
- pr_agent/algo/utils.py[1301-1325]
- Preserve per-field presence, e.g.:
- Change
prompt_tokens,completion_tokens,total_tokenstoOptional[int](or keep ints but addseen_prompt_tokens/seen_completion_tokens/seen_total_tokensbooleans). - Update
add_token_usage()to only accumulate fields that were actually present and mark them as seen. - Update
show_run_details()rendering: - If only total is known, render
- Tokens: {total:,} total. - If prompt+completion are known (with or without total), render the breakdown.
- If only one component is known, omit the missing component rather than printing 0.
| PR 2584 (2026-07-31) |
[reliability] `capture_logs` may raise AttributeError
`capture_logs` may raise AttributeError
`_get_request_user_field()` dereferences `record.get("extra", None).get(...)`, which can raise if `extra` is missing, breaking request attribution and potentially the request flow when enabled. This violates the robust error-handling requirement for edge cases in runtime logging context extraction._get_request_user_field() uses record.get("extra", None).get(...), which can raise AttributeError if extra is absent/None.
This code runs when config.add_user_to_requests is enabled and is meant to be a safe, optional, non-breaking enhancement.
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[436-443]
[reliability] Logger sink removal not guaranteed
Logger sink removal not guaranteed
`_get_request_user_field()` adds a temporary loguru sink but does not use `try/finally` to ensure `get_logger().remove(handler_id)` always runs. If an exception occurs between `add()` and `remove()`, the sink could leak and capture unrelated logs, impacting reliability/performance.Temporary loguru sink added in _get_request_user_field() is removed without a try/finally, so exceptions can leave the sink installed.
This method is called from chat_completion() when config.add_user_to_requests is enabled; it should never leave global logging state altered.
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[445-447]
[security] Cross-request attribution leak
Cross-request attribution leak
LiteLLMAIHandler._get_request_user_field temporarily attaches a process-wide Loguru sink and then uses the first captured log record, so concurrent requests can win the race and cause another request’s command/pr_url to be sent as this request’s OpenAI-compatible "user" field. This breaks attribution correctness and can disclose a different PR URL/command to the model provider when the feature is enabled._get_request_user_field() adds a global Loguru sink and then reads captured_extra[0]. In concurrent webhook/server usage, unrelated log records from other requests can be captured first, causing the wrong command/pr_url to be serialized and sent to the provider as the request attribution.
-
get_logger()returns the global Loguru logger (shared across concurrent requests). - Multiple servers run request handlers concurrently (async/background tasks) while using
logger.contextualize().
Make the capture request-scoped so it can only capture the record emitted by this method:
- Add a
filter=toget_logger().add(...)that only accepts a uniquely-identifiable record (e.g., match the message text, or bind a unique marker inextraand filter on it). - Prefer selecting the matching captured record (or the last one) rather than
captured_extra[0]. - Wrap
remove(handler_id)intry/finallyto avoid leaking the sink if anything raises.
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[427-460]
[maintainability] Single quotes in `_get_request_user_field`
Single quotes in `_get_request_user_field`
New Python code uses single-quoted strings and mixed quoting, which violates the repo style requirement to prefer double quotes and may trigger Ruff lint failures.New code in LiteLLMAIHandler._get_request_user_field() uses single quotes (and mixed quote styles) for string literals and dict keys, conflicting with the repo’s Ruff/formatting conventions that prefer double quotes.
This is a style/lint compliance issue and may introduce new Ruff findings.
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[439-442]
[maintainability] Overlong debug f-string line
Overlong debug f-string line
A newly added debug log line appears to exceed the 120 character limit, which violates the repository Ruff line-length policy.A newly added debug log message is very long and likely exceeds the configured Ruff line length limit (120).
The repository compliance checklist requires line length 120 for Python code.
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[663-664]
[correctness] Invalid JSON truncation
Invalid JSON truncation
LiteLLMAIHandler._get_request_user_field truncates the serialized JSON with `[:256]`, which can cut mid-string and produce invalid JSON when command/pr_url are long. This breaks the documented/tested expectation that the provider attribution value is a compact JSON string._get_request_user_field() currently does json.dumps(... )[:256]. If the JSON exceeds 256 characters, slicing can remove closing quotes/braces and yield an invalid JSON string.
The feature is documented as sending a compact JSON string in the OpenAI-compatible user field.
Tests also parse the value with json.loads(...), reinforcing the expectation that it remains valid JSON.
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[427-452]
- Do not slice the serialized JSON.
- Instead, enforce the 256-character cap by truncating individual values (e.g.,
command,pr_url) before serialization, thenjson.dumpsthe final dict. - Ensure the final output is always valid JSON (and ideally add a unit test that uses an artificially long
pr_urlto assert:len(user_field) <= 256andjson.loads(user_field)succeeds).
| PR 2570 (2026-07-27) |
[correctness] Markers ignore description flag
Markers ignore description flag
With `use_description_markers` enabled and `enable_pr_description=false`, the prompt no longer requests `description`, but `_prepare_pr_answer_with_markers` only replaces `pr_agent:summary` when `self.data['description']` exists and never removes/suppresses the marker otherwise. This can publish raw `pr_agent:summary` text (or an AI summary if the model returns `description` anyway) despite the flag explicitly disabling the summary section.When pr_description.enable_pr_description=false, the prompt template stops requesting a description field. In marker mode (use_description_markers=true), _prepare_pr_answer_with_markers() only replaces pr_agent:summary if self.data contains description, and it does not check the flag or remove the marker token. This can leave pr_agent:summary visible in the published PR description (and can also insert a summary if the model outputs description anyway).
The non-marker path (_prepare_pr_answer) already pops description when the flag is off, but marker mode is a separate rendering path and needs equivalent suppression/removal behavior.
- pr_agent/tools/pr_description.py[507-556]
- pr_agent/tools/pr_description.py[121-137]
| PR 2569 (2026-07-27) |
[correctness] None last_commit_id deref
None last_commit_id deref
`GiteaProvider._set_pr_commits()` can now set `last_commit`/`last_commit_id` to `None` when the PR commits endpoint returns an empty list, but downstream paths assume a valid commit object and either dereference `last_commit_id.sha` (crashing with `AttributeError`) or submit `commit_id=""` to Gitea’s reviews endpoint (causing inline review creation to fail or be unanchored despite `self.sha` being available).GiteaProvider._set_pr_commits() can leave last_commit/last_commit_id as None when the PR commits endpoint returns an empty list. Downstream code paths assume a commit object exists: some dereference last_commit_id.sha (triggering AttributeError), and publish_inline_comments() may submit commit_id="" to Gitea’s reviews endpoint, causing inline reviews to fail or be unanchored even though the PR head SHA is available in self.sha.
This PR intentionally changes behavior from raising an IndexError during initialization to allowing an empty commit list; the provider must remain compatible with existing consumers that expect .sha to exist. The provider already has the PR head SHA in self.sha, and at least one other method (e.g. _get_file_content_from_latest_commit) correctly falls back to self.sha when last_commit is missing, suggesting a consistent fallback strategy should be applied.
- pr_agent/git_providers/gitea_provider.py[111-127]
- pr_agent/git_providers/gitea_provider.py[336-344]
[maintainability] Single quotes in new tests
Single quotes in new tests
New test code introduces single-quoted strings, which deviates from the repo’s Python style preference for double quotes. This creates inconsistent formatting and may trigger Ruff/formatting churn across the file.The newly added tests use single quotes for string literals, but the repository style requires double quotes.
Rule requires aligning new/changed Python code with Ruff/style conventions, including preferring double quotes.
- tests/unittest/test_gitea_provider.py[373-383]
[reliability] Unvalidated commits payload
Unvalidated commits payload
`_set_pr_commits()` assumes `RepoApi.get_pr_commits()` returns a list of dicts, but `get_pr_commits()` returns raw `json.loads()` output without validating type; if a non-list/non-dict shape is returned, `reversed(raw_commits)` and `_GiteaCommitAdapter(raw).get(...)` can raise at runtime._set_pr_commits() iterates reversed(raw_commits) and passes each element to _GiteaCommitAdapter which calls .get(). RepoApi.get_pr_commits() returns json.loads() output directly with no validation.
While the endpoint is expected to return a list, defensive validation prevents hard crashes if the response is an object, an error payload, or contains unexpected elements.
- pr_agent/git_providers/gitea_provider.py[111-120]
- pr_agent/git_providers/gitea_provider.py[1086-1110]
- In
_set_pr_commits(): ifraw_commitsis not alist, log error and treat as empty. - Filter/skip any non-mapping elements before adapting.
- Optionally harden
_GiteaCommitAdapterto accept only mappings (else treat as{}).
| PR 2563 (2026-07-26) |
[reliability] Global flag state leak
Global flag state leak
dispatch._run_pr_agent applies "--config.propagate_tool_errors=true" via update_settings_from_args; if invoked without a request-scoped starlette_context, this writes into global_settings and can persistently enable error propagation for later non-MOSAICO runs in the same process. This PR increases the impact of such contextless calls by globally flipping tool error-handling semantics until settings are manually reset._run_pr_agent() relies on CLI-style --config.* args, which are applied by update_settings_from_args() directly onto get_settings(). When no Starlette request context is active, get_settings() resolves to global_settings, so _run_pr_agent() can unintentionally persist CONFIG.PROPAGATE_TOOL_ERRORS=True (and other overrides) beyond the call.
In the MOSAICO server path, the executor installs a request-scoped deepcopy into starlette_context, so this is safe. The problem is any direct/contextless invocation (tests already call the router without request_cycle_context, and other potential library callers could too).
- pr_agent/mosaico/dispatch.py[309-327] Suggested implementation direction:
- Detect when
get_settings()isglobal_settings(or when no context-scoped settings exist) and wrap the tool run in a temporary request context that uses a deepcopy ofglobal_settings, OR - Snapshot and restore at least the config keys you override (including
CONFIG.PROPAGATE_TOOL_ERRORS,CONFIG.PUBLISH_OUTPUT,CONFIG.PUBLISH_OUTPUT_PROGRESS, and any other keys dispatch mutates) in atry/finallyaroundPRAgent().handle_request(...).
| PR 2562 (2026-07-26) |
[correctness] Unknown flag swallowed as value
Unknown flag swallowed as value
`needval()` now accepts dash-prefixed values for *all* options, so a typo/unknown flag (e.g. `--github-dir --badflag`) is consumed as the directory value and won’t hit the `unknown argument` branch, only failing later (or comparing the wrong directory if such a path exists). This is a regression in CLI validation/diagnostics introduced by loosening the dash-value guard globally rather than just for `--ref`..github/scripts/mosaico_bundle_parity.sh relaxed needval() so it no longer rejects dash-prefixed option values, except for a small allowlist of the script’s own option tokens. This change allows unknown --* tokens to be swallowed as values for --github-dir/--gitlab-dir, bypassing the unknown argument handler and potentially producing misleading errors or unintended comparisons.
The PR’s motivation is valid for --ref (git refs may start with -), but the same relaxation is not necessary for directory arguments and reduces argument-validation quality.
- .github/scripts/mosaico_bundle_parity.sh[17-34]
- Keep allowing dash-leading values for
--ref. - Restore stricter validation for
--github-dirand--gitlab-dir(e.g., reject values starting with-for those two), or refactorneedvalto accept a mode parameter (allow/disallow dash-leading values) so each option can choose the appropriate policy. - Ensure an unknown
--badflagtoken is reported asunknown argumentinstead of being consumed as a value.
| PR 2561 (2026-07-26) |
[correctness] Diff prose stripping incomplete
Diff prose stripping incomplete
_diff_prose() stops stripping as soon as it encounters a line not matched by _DIFF_BODY_LINE_RE, but that regex omits valid git diff metadata like "new file mode" and "rename from/to". This can leak parts of the patch into the prose used for intent detection, potentially flipping routing (e.g., a leaked '?' causing /ask)._diff_prose() is used to remove raw diff bodies so verb/question detection isn't influenced by patch content. The current _DIFF_BODY_LINE_RE does not match several common git diff metadata lines (e.g., new file mode, deleted file mode, rename from, rename to, similarity index, GIT binary patch). Encountering any such line ends in_diff mode and causes subsequent patch lines to be kept as prose.
The leaked patch text is fed into _explicit_verb() / _reads_as_question() via _resolve_verb(), so routing can change based on patch contents rather than user intent.
- pr_agent/mosaico/dispatch.py[55-57]
- pr_agent/mosaico/dispatch.py[201-223]
- pr_agent/mosaico/dispatch.py[156-175]
- pr_agent/mosaico/diff_provider.py[59-70]
Expand _DIFF_BODY_LINE_RE to include common diff metadata lines (at least those handled by parse_unified_diff, like rename/new/deleted modes), and consider keeping in_diff=True until a clear terminator rather than turning it off on the first unknown line.
[correctness] Indented role diff missed
Indented role diff missed
_ROLE_LINE_RE consumes only one whitespace after the role label, so a turn like "user: diff --git ..." leaves the diff header indented and can prevent both diff detection and parsing (parse_unified_diff requires a column-0 "diff --git"). This can make a valid raw diff route as no-context or produce an empty-parse fallback.Turn splitting leaves leading indentation in the first content line when there are multiple spaces/tabs after the role label (e.g., user: diff --git ...). That indentation can prevent _looks_like_diff() from recognizing a raw diff header and can also make parse_unified_diff() return [] because it expects diff --git at column 0.
This only impacts forwarded conversation blobs (turn-splitting path). The reference agent uses "{role}: {content}", but real content can begin with whitespace (or some forwarders may emit multiple spaces after the colon).
- pr_agent/mosaico/dispatch.py[63-64]
- pr_agent/mosaico/dispatch.py[114-119]
- pr_agent/mosaico/dispatch.py[185-188]
Prefer a targeted normalization that only affects the role-label separator or raw diff markers, e.g.:
- Change the role label regex to consume all whitespace after
:(:[ \t]*), or - After extracting the first-line remainder,
lstrip()only when it would reveal a raw diff marker (diff --git/@@).
[correctness] Diff prose stripping incomplete
Diff prose stripping incomplete
_diff_prose() stops stripping as soon as it encounters a line not matched by _DIFF_BODY_LINE_RE, but that regex omits valid git diff metadata like "new file mode" and "rename from/to". This can leak parts of the patch into the prose used for intent detection, potentially flipping routing (e.g., a leaked '?' causing /ask)._diff_prose() is used to remove raw diff bodies so verb/question detection isn't influenced by patch content. The current _DIFF_BODY_LINE_RE does not match several common git diff metadata lines (e.g., new file mode, deleted file mode, rename from, rename to, similarity index, GIT binary patch). Encountering any such line ends in_diff mode and causes subsequent patch lines to be kept as prose.
The leaked patch text is fed into _explicit_verb() / _reads_as_question() via _resolve_verb(), so routing can change based on patch contents rather than user intent.
- pr_agent/mosaico/dispatch.py[55-57]
- pr_agent/mosaico/dispatch.py[201-223]
- pr_agent/mosaico/dispatch.py[156-175]
- pr_agent/mosaico/diff_provider.py[59-70]
Expand _DIFF_BODY_LINE_RE to include common diff metadata lines (at least those handled by parse_unified_diff, like rename/new/deleted modes), and consider keeping in_diff=True until a clear terminator rather than turning it off on the first unknown line.
[correctness] Indented role diff missed
Indented role diff missed
_ROLE_LINE_RE consumes only one whitespace after the role label, so a turn like "user: diff --git ..." leaves the diff header indented and can prevent both diff detection and parsing (parse_unified_diff requires a column-0 "diff --git"). This can make a valid raw diff route as no-context or produce an empty-parse fallback.Turn splitting leaves leading indentation in the first content line when there are multiple spaces/tabs after the role label (e.g., user: diff --git ...). That indentation can prevent _looks_like_diff() from recognizing a raw diff header and can also make parse_unified_diff() return [] because it expects diff --git at column 0.
This only impacts forwarded conversation blobs (turn-splitting path). The reference agent uses "{role}: {content}", but real content can begin with whitespace (or some forwarders may emit multiple spaces after the colon).
- pr_agent/mosaico/dispatch.py[63-64]
- pr_agent/mosaico/dispatch.py[114-119]
- pr_agent/mosaico/dispatch.py[185-188]
Prefer a targeted normalization that only affects the role-label separator or raw diff markers, e.g.:
- Change the role label regex to consume all whitespace after
:(:[ \t]*), or - After extracting the first-line remainder,
lstrip()only when it would reveal a raw diff marker (diff --git/@@).
| PR 2560 (2026-07-26) |
[maintainability] Brittle LiteLLM internal test
Brittle LiteLLM internal test
The new regression test reaches into LiteLLM private internals (`_in_memory_loggers`, `_init_custom_logger_compatible_class`) and asserts an implementation class name string, so it can fail on LiteLLM internal refactors/upgrades unrelated to MOSAICO tracing correctness. This increases ongoing maintenance cost and can produce CI failures even when the externally relevant behavior (callback registration/tracing) remains functional.The test test_mosaico_observability_deps.py depends on LiteLLM private/internal APIs (_in_memory_loggers, _init_custom_logger_compatible_class) and asserts type(logger).__name__ == "LangfuseOtelLogger". This makes the test brittle: any LiteLLM internal refactor (even with unchanged callback behavior) can break the suite.
The goal is to ensure the environment includes the missing transitive dependency so the langfuse_otel integration path is importable/constructible.
- tests/unittest/test_mosaico_observability_deps.py[42-77]
- Prefer asserting through a more stable/public surface where possible:
- Keep the environment-level import checks (
pydantic_settingsandlitellm.integrations.otelimportability). - Replace the private factory call and class-name assertion with a behavior/interface assertion (e.g., that constructing/obtaining the callback does not raise and returns an object with expected callable methods), or route via the same public path MOSAICO uses (env bridge + handler wiring) and assert that callbacks are enabled/registered without requiring private registry access.
- If you must use internals, reduce coupling:
- Avoid asserting on the concrete class name string; assert on properties/behavior instead.
- Avoid mutating
_in_memory_loggersdirectly; encapsulate cleanup with safer guards (e.g., check attribute existence before snapshot/restore) to prevent hard failures if LiteLLM changes internals.
| PR 2559 (2026-07-26) |
[reliability] `comm` failures not checked
`comm` failures not checked
The script captures `comm` output into variables without validating `comm`’s exit status. If `comm` fails (e.g., due to input/IO issues), the script can proceed with incomplete/empty sets and produce misleading parity results instead of exiting with a clear error..github/scripts/mosaico_bundle_parity.sh assigns only_gh, only_gl, and shared using comm inside command substitutions but does not check whether comm succeeded.
This script is intended to “fail closed” (exit 2 on operational errors). A non-zero exit from comm should be treated as a usage/fetch/operational error, not silently converted into empty/partial sets that can distort drift detection.
- .github/scripts/mosaico_bundle_parity.sh[198-212]
[reliability] `list_files()` masks `emit_raw` errors
`list_files()` masks `emit_raw` errors
`list_files()` uses `|| true` after a pipeline, which can hide failures from `emit_raw` and lead to misleading drift results instead of a clear error. This violates the requirement to handle error scenarios rather than ignoring them.list_files() currently uses || true after a pipeline: n_lines="$(emit_raw ... | ... | grep -c '' || true)". This construction forces the whole pipeline to succeed, masking upstream failures (e.g., emit_raw failing), and can cause the script to report drift (exit 1) or other misleading output instead of failing with a usage/fetch error (exit 2).
This script is a CI parity gate; incorrect exit codes or reports undermine its ability to reliably detect real drift vs. operational failures.
- .github/scripts/mosaico_bundle_parity.sh[118-127]
[reliability] Unvalidated token encoding
Unvalidated token encoding
When GITLAB_TOKEN is set, the script builds the Authorization header via a pipeline inside an `export` but never checks whether `base64`/`tr` succeeded, and the script is not running with `set -e`. If that encoding fails, the script still attempts `git clone` with a malformed/empty header, weakening the documented “token must work” invariant and producing harder-to-diagnose failures.When GITLAB_TOKEN is provided, the script constructs http.extraHeader using a command-substitution pipeline inside an export and does not validate the pipeline’s exit status before calling git clone.
The script explicitly documents that supplying a token must be exercised and must fail if broken; token-header construction failures should therefore be treated as deterministic errors before attempting the clone.
- .github/scripts/mosaico_bundle_parity.sh[102-107]
Compute the base64 value in a separate assignment and check it, e.g.
[reliability] Token not exported
Token not exported
`mosaico_bundle_parity.sh` checks `GITLAB_TOKEN` in the parent shell, but the credential helper expands `${GITLAB_TOKEN}` in a child process; if the token was set but not exported, `git clone` will authenticate with an empty password and fail. This primarily breaks local/manual runs using `GITLAB_TOKEN=...; ./mosaico_bundle_parity.sh` (as opposed to `GITLAB_TOKEN=... ./mosaico_bundle_parity.sh` or an exported env var).The script relies on ${GITLAB_TOKEN} being available to the shell spawned by git’s credential.helper, but it never exports the variable. If a caller sets GITLAB_TOKEN without exporting it, the script passes the “token is set” check yet git clone fails with empty credentials.
This affects local/manual invocations and any other caller that doesn’t export the token. GitHub Actions env: variables are exported, so CI is typically unaffected.
- .github/scripts/mosaico_bundle_parity.sh[69-83]
After verifying GITLAB_TOKEN is non-empty (and before invoking git clone), add export GITLAB_TOKEN so the credential-helper subprocess can reliably read it.
[reliability] `diff -u` errors ignored
`diff -u` errors ignored
When a content difference is found, the script runs `diff -u ... 2>/dev/null | sed ...` without checking whether `diff` errored (exit 2). This can mask operational failures (e.g., unreadable files) as normal drift output and prevents a clear exit-2 error path.diff -u is executed with stderr suppressed and without checking its exit code, so real diff errors (exit 2) can be misreported as drift output.
This script distinguishes outcomes by exit code (0 parity, 1 drift, 2 usage/fetch error). diff can return 2 for operational errors (permission issues, IO errors), which should map to exit 2 with a clear message.
- .github/scripts/mosaico_bundle_parity.sh[215-223]
[reliability] Grep failures are suppressed
Grep failures are suppressed
drop_allowlisted() uses `grep ... || true`, which suppresses operational grep failures (exit 2) the same way it suppresses “no matches” (exit 1), potentially producing an incorrect filtered file list. That can cascade into wrong GH-ONLY/GL-ONLY/shared sets or misleading downstream results when grep errors occur.drop_allowlisted() currently runs grep -vxF ... || true, which treats all grep nonzero statuses as success. This hides real grep errors (status 2) and can yield a truncated/empty list without surfacing the underlying failure.
For a verification script whose purpose is to provide a trustworthy parity verdict, masking operational failures can lead to confusing or incorrect results.
- .github/scripts/mosaico_bundle_parity.sh[140-155]
Capture grep output + exit status and accept only statuses 0 and 1:
- if status==0: keep output
- if status==1: output is empty (valid)
- else: fail with a clear error
If you make
drop_allowlistedable to fail, also propagate its exit status at call sites similarly to the existinglist_filespropagation pattern.
[security] Implicit token permissions
Implicit token permissions
The new workflow does not declare a `permissions:` block, so the job inherits whatever repository/org default GITHUB_TOKEN permissions are configured, which can be broader than this checkout-and-compare job needs. This makes the security boundary configuration-dependent and harder to audit over time.The workflow .github/workflows/mosaico-bundle-parity.yaml does not set explicit permissions, so it inherits repository/org defaults for GITHUB_TOKEN permissions. For least privilege and future-proofing against default changes, explicitly set only the permissions needed (typically contents: read).
This workflow checks out the repo and runs a local bash script; it does not need write permissions.
- .github/workflows/mosaico-bundle-parity.yaml[1-45]
[correctness] Locale mismatch in comm
Locale mismatch in comm
The script sorts file lists with LC_ALL=C but runs comm under the runner’s default locale, so for filenames whose collation differs from C this can produce wrong GH-ONLY/GL-ONLY/shared sets (false drift or missed drift). This contradicts the script’s explicit intent to correctly handle non-ASCII/escaped filenames.list_files() forces a deterministic byte-order sort via LC_ALL=C sort, but subsequent comm calls run under whatever locale the runner happens to have (often C.UTF-8). If locale collation differs, comm may consider inputs “not sorted” (or compute wrong set results), which can yield incorrect GH-ONLY/GL-ONLY/shared lists.
The script explicitly targets tricky filenames (non-ASCII and quote-bearing), which are exactly where locale collation differences are most likely to matter.
- .github/scripts/mosaico_bundle_parity.sh[152-165]
- .github/scripts/mosaico_bundle_parity.sh[102-108]
- .github/scripts/mosaico_bundle_parity.sh[127-128]
Prefix the three comm invocations with LC_ALL=C (or export LC_ALL=C for the comparison section), e.g.:
only_gh="$(LC_ALL=C comm -23 ...)"only_gl="$(LC_ALL=C comm -13 ...)"shared="$(LC_ALL=C comm -12 ...)"
[reliability] Workflow push path omission
Workflow push path omission
The workflow’s `push` trigger omits `.github/workflows/mosaico-bundle-parity.yaml` from `paths`, so a push to `main` that changes only this workflow file will not run the parity job. This creates a coverage gap where workflow-only updates can land without immediate validation from the workflow itself.The push trigger is path-filtered but does not include the workflow YAML itself, so workflow-only pushes to main won’t execute this workflow.
PR runs cover normal reviewed changes, but this gap affects direct pushes and any automation that updates only the workflow file.
- .github/workflows/mosaico-bundle-parity.yaml[23-29]
Add the workflow file to on.push.paths:
.github/workflows/mosaico-bundle-parity.yaml
[correctness] Option value misparsed
Option value misparsed
needval() only checks that $2 is non-empty, so `--github-dir --ref main` is accepted and shifts arguments incorrectly, producing a misleading “unknown argument” error instead of “--github-dir requires a value”. This can confuse debugging of CI/local invocations even though the script still exits 2.needval() validates only that $2 exists and is non-empty. If the user forgets to provide a value and the next token is another recognized flag, the parser will consume that flag as the value and shift, leading to a confusing downstream error.
Example bad invocation:
-
./mosaico_bundle_parity.sh --github-dir --ref mainCurrent behavior: - Treats
--refas the--github-dirvalue, then later errors onmainas an unknown argument. Desired behavior: - Immediately fail with
option --github-dir requires a value.
- .github/scripts/mosaico_bundle_parity.sh[45-58] Suggested implementation direction:
- Extend
needval()to also reject$2when it matches a known option (e.g.,--github-dir|--gitlab-dir|--ref|-h|--help). - Keep current exit code behavior (usage errors must remain exit 2).
| PR 2556 (2026-07-26) |
[reliability] Healthcheck needs PORT set
Healthcheck needs PORT set
The compose healthcheck probes `http://localhost:$PORT/health` but this overlay does not set `PORT` and the `mosaico_agent` image also does not define it, so `$PORT` can expand empty and the service never becomes healthy (blocking the registration container via `depends_on: condition: service_healthy`). Fixing this only addresses the port-expansion bug; `/health` can still intentionally return 503 when LLM creds/model aren’t configured.The healthcheck command uses $PORT ($${PORT} in compose-escaped form), but neither the compose overlay nor the Docker image defines PORT. When PORT is unset, the healthcheck URL becomes invalid (http://localhost:/health), keeping the agent unhealthy and preventing the registration service from starting.
- The server binds to
PORTif set, otherwise defaults to9000. - The healthcheck should not depend on
PORTbeing injected by an external compose base.
- docker/mosaico/docker-compose.pr-agent.yml[15-23]
Either:
- Add
PORT: 9000to the service environment so$PORTis always present, or - Change the healthcheck URL to use a fixed
9000or a shell fallback${PORT:-9000}(escaped appropriately for compose).
[reliability] Smoke test ignores advertised URL
Smoke test ignores advertised URL
The new `smoke_test.sh` maps host `${PORT}` to container `9000` but does not ensure the container advertises that mapped port in `supportedInterfaces` (and it never asserts the advertised URL), so `PORT=19000 ./smoke_test.sh` can pass while the agent card still advertises `http://localhost:9000/`.smoke_test.sh allows overriding the host port via PORT=..., but it does not set AGENT_CARD_HOST/AGENT_CARD_PORT (unless the user happens to put them in .env) and does not validate supportedInterfaces[0].url. This can report a successful smoke test even though the card advertises an endpoint that doesn’t match the actual exposed host port.
The agent card URL is computed from AGENT_CARD_HOST and AGENT_CARD_PORT (or PORT, or default 9000). If the container is started without those env vars, it will default to localhost:9000 regardless of the host port mapping.
- docker/mosaico/smoke_test.sh[19-70]
- pr_agent/mosaico/card.py[19-26]
- When launching the container, set
AGENT_CARD_HOST=localhostandAGENT_CARD_PORT=$PORTunless those keys are already provided in the.envfile. - Extend the Python card validation to assert
supportedInterfaces[0]['url'] == f'http://localhost:{PORT}/'(or derived from$BASE/).
[reliability] Smoke test deletes containers
Smoke test deletes containers
docker/mosaico/smoke_test.sh hard-codes the container name and installs an EXIT trap that always runs `docker rm -f` on that name, so a failure (including `docker run` failing due to name collision) can delete a pre-existing, unrelated container.smoke_test.sh uses a fixed container name and unconditionally force-removes it in an EXIT trap. If a container with that name already exists, or if the script exits early after a failure, the trap can delete a container that was not created by this run.
The script sets CONTAINER="pr-agent-mosaico-test", then docker run --name "$CONTAINER" ... and trap cleanup EXIT, where cleanup does docker rm -f "$CONTAINER".
- docker/mosaico/smoke_test.sh[19-60]
- Generate a unique container name per run (e.g., include PID + random suffix).
- Track whether this invocation successfully started a container (e.g.,
started=1afterdocker runsucceeds) and onlydocker rm -fwhenstarted==1. - Alternatively, label the container (e.g.,
--label pr-agent.smoke=1) and in cleanup remove by ID captured fromdocker run/docker psfiltering for that label, avoiding name-based deletion.
[reliability] Smoke-test `curl` lacks timeouts
Smoke-test `curl` lacks timeouts
`smoke_test.sh` uses `curl` for `/health` and `SendMessage` without any connect/overall timeouts, so the script can hang indefinitely on a stalled network or unresponsive container. This is missing edge-case handling for a test script intended to provide fast feedback.The smoke test's curl invocations have no explicit timeouts, which can cause the script to hang indefinitely.
This script is intended to be a quick validation (smoke/full). Adding --connect-timeout and --max-time makes failures deterministic and easier to diagnose.
- docker/mosaico/smoke_test.sh[94-112]
[security] Temp-file clobber in smoke
Temp-file clobber in smoke
docker/mosaico/smoke_test.sh writes /health and SendMessage responses to fixed filenames under /tmp and never removes them, so concurrent runs can overwrite each other and stale data can be read by later runs. Because /health includes exception text in its JSON body when unhealthy, the leftover file can also retain internal diagnostic details on disk (permissions depend on umask).docker/mosaico/smoke_test.sh writes responses to hard-coded /tmp/mosaico_health.json and /tmp/mosaico_resp.json and never deletes them. This can cause (1) clobbering between concurrent/overlapping runs and (2) leaving behind diagnostic response bodies (including exception strings from /health).
- The script already has an
EXITtrap for container cleanup; reuse that trap to also remove any temp files/dirs. -
/healthresponses can embed exception text when unhealthy.
- docker/mosaico/smoke_test.sh[26-28]
- docker/mosaico/smoke_test.sh[88-107]
- Create a per-run temp directory (e.g.,
tmpdir=$(mktemp -d)), storehealth.jsonandresp.jsoninside it, and extend theEXITtrap torm -rf "$tmpdir". - Optionally set restrictive permissions (e.g.,
umask 077) before writing files, if you want to minimize local disclosure on shared hosts/CI runners.
[reliability] Healthcheck needs PORT set
Healthcheck needs PORT set
The compose healthcheck probes `http://localhost:$PORT/health` but this overlay does not set `PORT` and the `mosaico_agent` image also does not define it, so `$PORT` can expand empty and the service never becomes healthy (blocking the registration container via `depends_on: condition: service_healthy`). Fixing this only addresses the port-expansion bug; `/health` can still intentionally return 503 when LLM creds/model aren’t configured.The healthcheck command uses $PORT ($${PORT} in compose-escaped form), but neither the compose overlay nor the Docker image defines PORT. When PORT is unset, the healthcheck URL becomes invalid (http://localhost:/health), keeping the agent unhealthy and preventing the registration service from starting.
- The server binds to
PORTif set, otherwise defaults to9000. - The healthcheck should not depend on
PORTbeing injected by an external compose base.
- docker/mosaico/docker-compose.pr-agent.yml[15-23]
Either:
- Add
PORT: 9000to the service environment so$PORTis always present, or - Change the healthcheck URL to use a fixed
9000or a shell fallback${PORT:-9000}(escaped appropriately for compose).
[reliability] Smoke test ignores advertised URL
Smoke test ignores advertised URL
The new `smoke_test.sh` maps host `${PORT}` to container `9000` but does not ensure the container advertises that mapped port in `supportedInterfaces` (and it never asserts the advertised URL), so `PORT=19000 ./smoke_test.sh` can pass while the agent card still advertises `http://localhost:9000/`.smoke_test.sh allows overriding the host port via PORT=..., but it does not set AGENT_CARD_HOST/AGENT_CARD_PORT (unless the user happens to put them in .env) and does not validate supportedInterfaces[0].url. This can report a successful smoke test even though the card advertises an endpoint that doesn’t match the actual exposed host port.
The agent card URL is computed from AGENT_CARD_HOST and AGENT_CARD_PORT (or PORT, or default 9000). If the container is started without those env vars, it will default to localhost:9000 regardless of the host port mapping.
- docker/mosaico/smoke_test.sh[19-70]
- pr_agent/mosaico/card.py[19-26]
- When launching the container, set
AGENT_CARD_HOST=localhostandAGENT_CARD_PORT=$PORTunless those keys are already provided in the.envfile. - Extend the Python card validation to assert
supportedInterfaces[0]['url'] == f'http://localhost:{PORT}/'(or derived from$BASE/).
[reliability] Healthcheck needs PORT set
Healthcheck needs PORT set
The compose healthcheck probes `http://localhost:$PORT/health` but this overlay does not set `PORT` and the `mosaico_agent` image also does not define it, so `$PORT` can expand empty and the service never becomes healthy (blocking the registration container via `depends_on: condition: service_healthy`). Fixing this only addresses the port-expansion bug; `/health` can still intentionally return 503 when LLM creds/model aren’t configured.The healthcheck command uses $PORT ($${PORT} in compose-escaped form), but neither the compose overlay nor the Docker image defines PORT. When PORT is unset, the healthcheck URL becomes invalid (http://localhost:/health), keeping the agent unhealthy and preventing the registration service from starting.
- The server binds to
PORTif set, otherwise defaults to9000. - The healthcheck should not depend on
PORTbeing injected by an external compose base.
- docker/mosaico/docker-compose.pr-agent.yml[15-23]
Either:
- Add
PORT: 9000to the service environment so$PORTis always present, or - Change the healthcheck URL to use a fixed
9000or a shell fallback${PORT:-9000}(escaped appropriately for compose).
[reliability] Smoke test ignores advertised URL
Smoke test ignores advertised URL
The new `smoke_test.sh` maps host `${PORT}` to container `9000` but does not ensure the container advertises that mapped port in `supportedInterfaces` (and it never asserts the advertised URL), so `PORT=19000 ./smoke_test.sh` can pass while the agent card still advertises `http://localhost:9000/`.smoke_test.sh allows overriding the host port via PORT=..., but it does not set AGENT_CARD_HOST/AGENT_CARD_PORT (unless the user happens to put them in .env) and does not validate supportedInterfaces[0].url. This can report a successful smoke test even though the card advertises an endpoint that doesn’t match the actual exposed host port.
The agent card URL is computed from AGENT_CARD_HOST and AGENT_CARD_PORT (or PORT, or default 9000). If the container is started without those env vars, it will default to localhost:9000 regardless of the host port mapping.
- docker/mosaico/smoke_test.sh[19-70]
- pr_agent/mosaico/card.py[19-26]
- When launching the container, set
AGENT_CARD_HOST=localhostandAGENT_CARD_PORT=$PORTunless those keys are already provided in the.envfile. - Extend the Python card validation to assert
supportedInterfaces[0]['url'] == f'http://localhost:{PORT}/'(or derived from$BASE/).
| PR 2552 (2026-07-25) |
[correctness] Gitea tag docs conflict
Gitea tag docs conflict
The new rolling-tag list includes `gitea_app`, but the Gitea installation guide tells users to push `pragent/pr-agent:gitea_webhook` after building `gitea_app`, so the documented commands won’t push the image they just built without retagging. This contradicts the publish workflow, which uses `gitea_app` as the rolling tag, and can cause users to pin/push the wrong tag or fail to push at all.The installation landing page documents gitea_app as the rolling tag, but docs/docs/installation/gitea.md instructs pushing gitea_webhook even though it builds gitea_app. This is inconsistent and the push command won’t reference the built image tag.
The Docker publish workflow’s matrix declares gitea_app as the rolling tag and uses it when computing pushed tags.
- docs/docs/installation/gitea.md[26-31]
- .github/workflows/publish.yml[90-150]
- .github/workflows/publish.yml[174-188]
Update docs/docs/installation/gitea.md to use a consistent repository+tag, e.g.:
- Build directly as
pragent/pr-agent:gitea_appand push that tag, or - Add an explicit
docker tag pr-agent:gitea_app pragent/pr-agent:gitea_appbefore pushing. Also verify there are no remaining references togitea_webhookifgitea_appis the canonical tag per the workflow.
| PR 2551 (2026-07-25) |
[observability] Unretrieved task exceptions
Unretrieved task exceptions
`drain_litellm_callbacks()` uses `asyncio.wait()` but discards the completed tasks set, so exceptions raised by callback tasks are never retrieved and can surface as `Task exception was never retrieved` warnings (or be silently ignored). This reduces diagnosability of callback failures and can create noisy shutdown logs.drain_litellm_callbacks() waits for pending tasks via asyncio.wait(), but it ignores the returned done tasks and never calls task.result() / task.exception(). If any drained callback task raises, asyncio can emit Task exception was never retrieved, and the root cause is not logged.
This drain is best-effort telemetry and already intentionally swallows errors, but it should still observe task exceptions to avoid asyncio warnings and to log failures in callbacks.
- pr_agent/algo/ai_handlers/litellm_helpers.py[172-205]
After done, still_pending = await asyncio.wait(...), iterate done and call task.result() inside a try/except (or call task.exception()), logging exceptions at debug/warning level but not raising. Alternatively, replace asyncio.wait() with asyncio.gather(*pending, return_exceptions=True) (bounded by the same timeout) and log any returned exceptions.
[reliability] Drain waits unrelated tasks
Drain waits unrelated tasks
`drain_litellm_callbacks()` builds its wait set from `asyncio.all_tasks()` with only two exclusions, so any unrelated background task still running at teardown can delay CLI/Action exit up to the timeout. If any such task remains pending, the function returns before `worker.flush()`, which can re-drop callbacks that already reached the worker queue.The drain waits on almost all loop tasks (asyncio.all_tasks()), not just LiteLLM callback-related ones. Any unrelated pending task can consume the whole timeout. Additionally, on timeout the function returns before attempting worker.flush(), which can drop callbacks that were already enqueued.
This function is invoked at teardown in both the CLI and the GitHub Action runner. It should focus on LiteLLM callback tasks and be robust in the presence of unrelated pending tasks.
- pr_agent/algo/ai_handlers/litellm_helpers.py[172-205]
- pr_agent/cli.py[139-155]
- pr_agent/servers/github_action_runner.py[347-361]
- Restrict the drained task set to LiteLLM-related tasks (e.g., tasks whose coroutine
__module__starts withlitellmand/or whose qualname matches known logging helpers), instead ofasyncio.all_tasks(). - If a timeout occurs, log it but do not return early; proceed to attempt
worker.flush()with any remaining time (even if 0) so queued callbacks get a best-effort chance to complete. - Optionally, snapshot tasks present at drain start and only wait on tasks created after that snapshot to further reduce unrelated-task impact.
| PR 2550 (2026-07-25) |
[reliability] Unnecessary secret client init
Unnecessary secret client init
In gitlab_webhook(), get_fork_safe_secret_provider() is called before checking whether the request will actually use token-based secret-provider auth, so a cloud client can be constructed (or raise) even when X-Gitlab-Token is absent or a shared-secret path is used. This adds avoidable overhead and can turn a secret-provider misconfiguration into runtime 500s instead of a startup failure.gitlab_webhook() currently calls get_fork_safe_secret_provider() unconditionally at the start of request handling, even though the secret-provider is only needed when X-Gitlab-Token is present and the secret-provider path is taken.
This can:
- Build a cloud secret client even for requests that don’t need it.
- Move provider initialization failures (e.g., missing creds / SDK init errors) from startup-time to request-time, causing runtime 500s.
The per-process PID cache is correct for fork safety under preload_app, but the call-site should be moved so the provider is only built when the request actually needs secret-provider lookup.
- pr_agent/servers/gitlab_webhook.py[212-218]
[reliability] Frozen GC blocks cyclic cleanup
Frozen GC blocks cyclic cleanup
`when_ready()` freezes the preloaded heap via `gc.freeze()`, but `get_fork_safe_secret_provider()` later overwrites the inherited secret provider instance after fork (PID mismatch). If the provider/client object graph contains reference cycles, the overwritten pre-fork instance can’t be reclaimed by cyclic GC, potentially retaining extra memory/resources per worker until process exit.gc.freeze() is executed after preload and before worker fork. The GitLab webhook code intentionally replaces the inherited (pre-fork) secret provider object inside workers when it detects a PID change. If that inherited provider/client graph has reference cycles, freezing it can prevent cyclic GC from ever reclaiming it after replacement, leaving unnecessary memory/resources attached to each worker for its lifetime.
- The gunicorn config freezes the preloaded heap to preserve copy-on-write.
- The GitLab webhook module constructs a cloud secret provider at import time, then overwrites it in each worker on first use.
- The in-repo AWS/GCS secret providers construct long-lived cloud client objects and do not expose an explicit close/cleanup path.
- pr_agent/servers/gunicorn_config.py[267-287]
- pr_agent/servers/gitlab_webhook.py[28-46]
- pr_agent/secret_providers/aws_secrets_manager_provider.py[10-26]
- pr_agent/secret_providers/google_cloud_storage_secret_provider.py[9-19]
-
Avoid creating the secret provider pre-fork: don’t construct the cloud client at import time under
preload_app; instead, build it lazily in each worker (and optionally perform lightweight config validation at startup to preserve “fail fast” without instantiating the client). -
Avoid freezing objects you plan to replace: move the
gc.freeze()strategy so that replaceable per-worker clients are created after fork (or ensure they aren’t part of the frozen set before replacement). -
Explicit cleanup before replacement (larger change): add a
close()/cleanup API on secret providers and invoke it on the inherited provider before overwriting it in the worker.
[correctness] Affinity ignored under quota
Affinity ignored under quota
available_cpus() returns the cgroup CPU quota whenever present and ignores a potentially stricter CPU affinity/cpuset mask, so worker sizing can exceed the CPUs the process is actually allowed to run on. This can cause unnecessary worker processes (and memory use) on pinned pods, despite the max-worker cap.available_cpus() claims to honor both cgroup quota and CPU affinity, but currently returns the cgroup quota whenever it exists and only consults os.sched_getaffinity() when the quota is None. On systems where both quota and cpuset pinning are active, this overestimates usable CPUs.
This affects compute_workers() which uses available_cpus() to set gunicorn workers.
- pr_agent/servers/gunicorn_config.py[112-142]
- Compute both limits when possible and use the stricter one:
- If cgroup limit is present, compute
quota_cpus(considermath.ceil(limit)if you want fractional quotas to count as an additional worker, or keep current truncation if intentional). - Also compute
affinity_cpus = len(os.sched_getaffinity(0))when available. - Return
max(1, min(quota_cpus, affinity_cpus))when both are available. - Add/adjust unit tests to cover “quota=4 + affinity=2 => available_cpus()==2”.
[correctness] Docs contradict `preload_app`
Docs contradict `preload_app`
The new GitLab installation docs state that gunicorn workers "do not share memory" and imply a linear `250MB × workers` startup requirement, but the PR explicitly enables `preload_app = True` to share the imported heap copy-on-write. This can mislead users and does not clearly establish a minimum startup memory request/limit for v0.36.1+ as required.The GitLab install docs state that gunicorn workers do not share memory and recommend budgeting 250MB × workers, but the server now uses preload_app = True specifically to share the imported heap copy-on-write. The docs should reflect the new memory model and explicitly document (or reference) a tested minimum memory request/limit (or range) for reliable startup on v0.36.1+.
This PR’s goal is to prevent startup OOMKills; inaccurate sizing guidance can cause users to over/under-provision and fails the "minimum memory" documentation requirement.
- docs/docs/installation/gitlab.md[100-117]
- pr_agent/servers/gunicorn_config.py[147-151]
[observability] Forked workers share log file
Forked workers share log file
With `preload_app = True`, `setup_logger()` runs during master preload, and if `CONFIG.ANALYTICS_FOLDER` is set it opens `pr-agent..log` using the master PID; forked workers inherit that same file sink/FD so per-worker log file separation is lost and concurrent writes may interleave in the same file.preload_app=True causes import-time logging setup to run only in the Gunicorn master. When CONFIG.ANALYTICS_FOLDER is enabled, setup_logger() creates a file sink named with os.getpid(), which becomes the master PID. Workers then inherit the sink and write to the same master-named file.
This is triggered by the new preload_app = True setting in the Gunicorn config and the fact that the webhook apps call setup_logger(...) at module import time.
- pr_agent/servers/gunicorn_config.py[145-154]
- pr_agent/log/init.py[30-60]
- pr_agent/servers/gitlab_webhook.py[16-30]
- pr_agent/servers/github_app.py[24-40]
- pr_agent/servers/gitea_app.py[18-25]
Add a Gunicorn post_fork(server, worker) hook (or equivalent per-worker hook) in gunicorn_config.py that re-runs setup_logger(...) inside each worker process (so the file sink uses the worker PID). Keep stdout logging behavior unchanged; only ensure analytics file sinks are opened per worker when CONFIG.ANALYTICS_FOLDER is set.
[reliability] Flaky gunicorn_config tests
Flaky gunicorn_config tests
The tests import `pr_agent.servers.gunicorn_config` before the autouse fixture deletes `GUNICORN_*` env vars and patches cgroup paths, but `gunicorn_config` computes `workers = compute_workers()` at import time, so `gunicorn_config.workers` can depend on the CI environment (especially if `GUNICORN_WORKERS` is set).gunicorn_config.workers is computed at module import. The test file imports gunicorn_config at top-level, but its autouse fixture (which deletes GUNICORN_* and patches cgroup file paths) runs after import. This makes assertions that depend on gunicorn_config.workers potentially environment-dependent.
The fixture’s docstring says it detaches tests from host env/cgroup files, but it does not affect module import-time side effects.
- tests/unittest/test_gunicorn_config.py[1-13]
- tests/unittest/test_gunicorn_config.py[108-110]
- pr_agent/servers/gunicorn_config.py[136-146]
Option A (preferred): avoid importing gunicorn_config at module scope; import it inside tests after env/path monkeypatching.
Option B: in isolated_env, after setting env/path patches, call import importlib; importlib.reload(gunicorn_config) so workers = compute_workers() is recalculated under the isolated environment before assertions like test_module_level_workers_within_bounds.
| PR 2548 (2026-07-25) |
[security] Stale api_key injected
Stale api_key injected
When `LiteLLMAIHandler.__init__` takes the keyless OpenAI branch, it now sets only `litellm.openai_key` and leaves the process-global `litellm.api_key` unchanged, so a real provider key from a prior handler init can persist. `chat_completion()` will then inject that stale key into later calls via `kwargs["api_key"]`, potentially authenticating requests with the wrong credential or leaking it to a keyless OpenAI-compatible endpoint.The keyless-OpenAI init branch now writes the placeholder to litellm.openai_key but does not reset litellm.api_key. Since litellm.api_key is process-global and chat_completion() forwards any truthy, non-placeholder value, a real key set by a prior handler init can be forwarded into later unrelated calls.
LiteLLMAIHandler.__init__ mutates multiple LiteLLM globals (litellm.api_key, litellm.openai_key). Provider inits (Groq/XAI/OpenRouter/Azure AD/etc.) can set litellm.api_key, and chat_completion() will forward it whenever it is set.
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[53-63]
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[679-686]
In the elif 'OPENAI_API_KEY' not in os.environ: branch, explicitly clear the global default key (e.g., litellm.api_key = None) before setting litellm.openai_key = DUMMY_LITELLM_API_KEY. This preserves the placeholder behavior for OpenAI-compatible endpoints while preventing stale real keys from being forwarded by chat_completion() in subsequent handler instances.
| PR 2547 (2026-07-25) |
[correctness] Wrong similar_issue CLI flag
Wrong similar_issue CLI flag
`docs/docs/tools/index.md` documents `similar_issue` as runnable with `--pr_url=`, but the CLI and tool implementation require an issue URL via `--issue_url` and will raise when given a PR URL.The Usage examples table documents the similar_issue tool with --pr_url=<PR_URL>, but the CLI/tool expect an issue URL (--issue_url). This makes the documented command fail when copied.
-
similar_issueis routed toPRSimilarIssue, which parses the input as a GitHub issue URL. - The CLI explicitly documents
--issue_url=... similar_issue.
- docs/docs/tools/index.md[20-37]
- Change the
Similar Issuesrow CLI example to:python -m pr_agent.cli --issue_url=<ISSUE_URL> similar_issue. - Adjust the surrounding text if needed to clarify that some tools use
--issue_urlrather than--pr_url. (Keep the table compact, but make the command correct.)
| PR 2545 (2026-07-24) |
[reliability] `unresolve_comment_thread` failure duplicates review
`unresolve_comment_thread` failure duplicates review
`publish_persistent_comment_full()` can post a new review comment after successfully editing the existing one if `unresolve_comment_thread()` raises, because the broad outer `except` triggers the fallback publish. This can cause duplicated review comments/threads and inconsistent persistent-review behavior.publish_persistent_comment_full() edits an existing persistent comment and then calls unresolve_comment_thread(comment). If unresolve_comment_thread throws, the surrounding try/except catches it and falls back to publishing a new comment, which can duplicate the review output.
This risk was introduced by adding the as_thread flow and calling unresolve_comment_thread inside the same try that controls the persistent-comment update and fallback behavior.
- pr_agent/git_providers/git_provider.py[376-387]
[reliability] Unguarded `discussion.attributes['notes'][0]`
Unguarded `discussion.attributes['notes'][0]`
When publishing a comment as a GitLab thread, the code assumes `discussion.attributes['notes'][0]['id']` always exists and is well-formed, so an unexpected or empty `notes` payload can raise (IndexError/KeyError/TypeError) and abort publishing with no graceful fallback. Because this exception can occur after the thread is created, it can also prevent downstream cleanup (e.g., leaving temporary "Preparing review..." comments behind).GitLabProvider.publish_comment(..., as_thread=True) assumes the created discussion always contains attributes['notes'][0]['id']; if notes is missing/empty or shaped differently, the code raises (KeyError/IndexError/TypeError) and review publishing fails. Because this can happen after the discussion is created and before the caller finishes, it can also prevent cleanup (e.g., leaving temporary "Preparing review..." comments behind).
This is a (new) code path used for the final /review output when GITLAB.PUBLISH_REVIEW_AS_THREAD=true. Failures here should degrade gracefully without duplicating the review comment (e.g., avoid creating a second plain note as a fallback), and the caller (PRReviewer) currently removes temporary progress comments only after publishing succeeds.
- pr_agent/git_providers/gitlab_provider.py[522-540]
- pr_agent/tools/pr_reviewer.py[163-200]
[performance] Thread reopen scans discussions
Thread reopen scans discussions
GitLabProvider.unresolve_comment_thread() fetches all MR discussions (get_all=True) and linearly scans their attributes to find the discussion owning a note ID, adding a full discussions listing on every persistent threaded review update. On discussion-heavy MRs this increases API traffic and can noticeably slow review refreshes.unresolve_comment_thread() calls mr.discussions.list(get_all=True) and scans every discussion to find the one containing comment.id. This adds an O(N) scan plus potentially multiple paginated API calls each time the persistent review is updated as a thread.
This is invoked from GitProvider.publish_persistent_comment_full() after editing the existing persistent review comment when as_thread=True.
- pr_agent/git_providers/gitlab_provider.py[546-558]
- pr_agent/git_providers/git_provider.py[363-387]
- Prefer a direct lookup path when possible (e.g., if the note object exposes a
discussion_id/discussionreference, usemr.discussions.get(...)rather than listing all). - If direct lookup is not possible, consider limiting the scan (stop early, avoid
get_all=Trueif the API/library can search by note id, or cache the discussion id when creating the thread in the same run). - Keep the existing soft-fail behavior (don’t raise from reopen attempts).
[maintainability] Single quotes in new code
Single quotes in new code
New/modified Python code uses single-quoted string literals (e.g., `{'as_thread': True}`), which diverges from the repository compliance requirement to prefer double quotes. This increases stylistic inconsistency across the modified modules.| PR 2528 (2026-07-16) |
[maintainability] `default_indicators` hardcoded in code
`default_indicators` hardcoded in code
`is_bot_user()` embeds the default bot-indicator list in Python instead of sourcing defaults from Dynaconf configuration, making runtime behavior harder to manage via `.pr_agent.toml`/defaults. This also duplicates the default list between code and `configuration.toml` comments, risking drift over time.is_bot_user() hardcodes the GitLab bot-indicator default list in code via default_indicators, instead of defining it as a Dynaconf default in pr_agent/settings/configuration.toml.
Compliance requires configurable behavior to be controlled via .pr_agent.toml or Dynaconf defaults (and to avoid duplicating the same “truth” across files).
- pr_agent/servers/gitlab_webhook.py[63-70]
- pr_agent/settings/configuration.toml[274-292]
[security] `bot_indicators` lacks type validation
`bot_indicators` lacks type validation
`gitlab.bot_user_indicators` is used without validating/normalizing its type and contents, so a misconfigured value (e.g., a string or non-string list items) can silently break bot detection or trigger exceptions. This treats configuration input as trusted despite it being a security boundary for webhook processing logic.gitlab.bot_user_indicators is consumed without validation/normalization. If it is set to an unexpected type (e.g., a string, or a list containing non-strings), bot detection can behave incorrectly (e.g., iterating characters) or raise exceptions.
Configuration is a security boundary; inputs should be validated and normalized with targeted error handling.
- pr_agent/servers/gitlab_webhook.py[63-74]
[correctness] Case-sensitive indicators
Case-sensitive indicators
`is_bot_user()` lowercases only `sender_name` but does not lowercase configured `gitlab.bot_user_indicators`, so user-provided indicators like "Renovate" won’t match even though the config comment says matching is case-insensitive. This can cause bot senders to be processed unexpectedly and trigger webhook handling that operators intended to suppress.is_bot_user() lowercases the sender display name but compares it against indicators without normalizing them. This makes matching effectively case-sensitive for configured values, contradicting the documented “case-insensitive match”.
- Defaults are already lowercase, so the bug mainly affects operator-supplied indicator lists.
- pr_agent/servers/gitlab_webhook.py[66-71]
- Normalize configured indicators before the
any()check, e.g. coerce to strings and lowercase each indicator: - Retrieve indicators from settings
- Build
bot_indicators = [str(i).lower() for i in bot_indicators] - Then run
indicator in sender_nameagainst the normalized list - Optionally (recommended) guard against non-list values (e.g. a single string) to avoid iterating over characters.
[maintainability] Ambiguous override semantics
Ambiguous override semantics
The configuration comment says the setting can “extend or replace” the defaults, but the implementation/tests enforce full replacement, so users who set only extra values (expecting extension) will silently drop default indicators. This can lead to bot users no longer being skipped after configuration changes.The configuration comment implies the override may “extend” defaults, but the actual behavior is replacement-only (as verified by the test).
Replacement semantics are fine, but the comment should explicitly state that operators must include the default entries themselves if they want to keep them.
- pr_agent/settings/configuration.toml[287-290]
Update the comment to something like:
- “When set, this list replaces the defaults. To extend, include the defaults plus your additional indicators.”
| PR 2526 (2026-07-15) |
[maintainability] `test_edit_pull_request_*` uses single-quotes
`test_edit_pull_request_*` uses single-quotes
The newly added unit tests use single-quoted strings, which diverges from the repo’s stated Python formatting standard to prefer double quotes. This can create style inconsistency and churn in future formatting/lint passes.New test code uses single-quoted strings, but the repository standard requires preferring double quotes.
This PR added two new tests in tests/unittest/test_gitea_provider.py that introduce multiple single-quoted string literals (e.g., in @patch(...), from ... import ..., and function calls/asserts).
- tests/unittest/test_gitea_provider.py[504-530]
| PR 2519 (2026-07-10) |
[correctness] Suffix-only language matching
Suffix-only language matching
LocalGitProvider.get_languages() matches languages using only Path.suffix, so settings rules that are full filenames ("Dockerfile", "Makefile") or multi-part extensions (".cmake.in") can never match and are excluded from the computed language percentages. This under-reports those languages and can skew prioritisation/file ordering in repos where such file types are common.LocalGitProvider.get_languages() builds an extension→language lookup from language_extension_map_org, but it looks up using Path.suffix only. language_extension_map_org contains:
- full filename patterns (e.g.
"Dockerfile","Makefile") - multi-part extensions (e.g.
".cmake.in") Those entries will never match a suffix-only lookup and therefore are excluded from the language percentage calculation.
The settings map already encodes these filename and multi-part-extension cases, so the local provider should respect them if it wants to approximate hosted-provider language detection.
- pr_agent/git_providers/local_git_provider.py[156-183]
- Keep building a normalized lookup from
language_extension_map_org, but normalize keys into two match modes:
-
Exact filename keys (no leading dot): match against
filepath.name.lower(). -
Suffix keys (leading dot, possibly multi-part like
.cmake.in): match against the longest combined suffix fromfilepath.suffixes(e.g. forfoo.cmake.in, try.cmake.infirst, then.in).
- Prefer longest suffix match to avoid
.inshadowing.cmake.in. - Continue excluding truly-unmatched files so they fall through to
Otherdownstream.
| PR 2495 (2026-07-02) |
[maintainability] Missing final newline
Missing final newline
`tests/unittest/test_pr_reviewer_core.py` is added/modified without a final trailing newline, which can trigger the repository’s `end-of-file-fixer` pre-commit hook, fail hygiene checks, and create noisy cross-platform diffs or unnecessary churn.tests/unittest/test_pr_reviewer_core.py is missing a final newline at end-of-file, violating formatting/hygiene expectations and potentially causing pre-commit failures or noisy diffs.
- The PR diff explicitly indicates
\ No newline at end of filefortests/unittest/test_pr_reviewer_core.py. - The repository’s pre-commit configuration includes the
end-of-file-fixerhook, which enforces a trailing newline and may rewrite files that don’t comply.
- tests/unittest/test_pr_reviewer_core.py[122-125]
- .pre-commit-config.yaml[7-15]
[maintainability] Brittle source-code assertion
Brittle source-code assertion
`test_get_user_answers_return_order_matches_init_destructuring` asserts on an exact `inspect.getsource(PRReviewer.__init__)` substring, so harmless refactors (line wrapping, helper extraction, renames) can break tests even when behavior is correct. This also doesn’t actually validate the runtime `/answer` initialization path because the test constructs the object via `__new__` and never runs `__init__`.A unit test checks PRReviewer.__init__ by searching for a specific source-code string via inspect.getsource(...). This is brittle and can fail due to formatting/refactors without any functional regression. Also, the test doesn’t exercise the actual __init__ path because the fixture uses PRReviewer.__new__.
The bug is about swapped question/answer flowing into self.vars in PRReviewer.__init__. A regression test should validate observable behavior (e.g., reviewer.vars['question_str'] / reviewer.vars['answer_str']) rather than a specific line of implementation.
- tests/unittest/test_pr_reviewer_core.py[95-125]
- tests/unittest/test_pr_reviewer_core.py[8-13]
- pr_agent/tools/pr_reviewer.py[35-103]
- Remove the
inspect.getsource(...)assertion. - Add a behavior-based test that instantiates
PRReviewer(..., is_answer=True)while monkeypatching/mocking: -
get_git_provider_with_contextto return a stubgit_providerwith required methods/attrs (pr.title,get_languages,get_files,get_issue_comments,is_supported,get_pr_description, etc.). -
TokenHandler(and possiblyLiteLLMAIHandler) to a lightweight dummy to avoid network/LLM. - Assert that after construction:
reviewer.vars['question_str'] == <expected question>reviewer.vars['answer_str'] == <expected answer>
| PR 2494 (2026-07-02) |
[reliability] Workflow_run tests leak settings
Workflow_run tests leak settings
The new workflow_run unit tests call run_action(), which mutates global Dynaconf flags (config.is_auto_command and pr_description.final_update_message), but the tests don’t restore those flags afterward. This can leak state into later tests and make the overall test suite order-dependent or flaky.The new workflow_run tests invoke github_action_runner.run_action(), which mutates global settings (settings.config.is_auto_command, settings.pr_description.final_update_message). The tests only use restore_github_settings, which restores GITHUB / GITHUB_ACTION_CONFIG but not these mutated flags, causing cross-test state leakage.
run_action() is designed for a one-shot GitHub Action process, so it doesn't reset global settings at the end. In unit tests, however, the same process runs many tests, so global settings must be restored to keep tests isolated.
- tests/unittest/test_github_action_runner_core.py[108-126]
- tests/unittest/test_github_action_runner_core.py[233-283]
- pr_agent/servers/github_action_runner.py[280-289]
- Option A (preferred): extend the
restore_github_settingsfixture to also snapshot+restore: settings.config.is_auto_commandsettings.pr_description.final_update_message- (optionally)
settings.config.response_language(for symmetry with the existing pull_request test) - Option B: in each new workflow_run test, follow the pattern used in
test_run_action_invokes_enabled_auto_tools_for_pull_request_event(try/finally restoring the mutated flags).
[security] Broad `except Exception` in `_inject_artifact_context()`
Broad `except Exception` in `_inject_artifact_context()`
`_inject_artifact_context()` uses a broad `except Exception` and continues execution, which can mask unexpected failures in a security-relevant config/file-loading path. This reduces visibility into malformed configuration or unexpected runtime conditions._inject_artifact_context() wraps its full logic in except Exception, which can unintentionally mask programming errors and security-relevant failures during config/env parsing and artifact loading.
Artifact injection reads local files and mutates tool prompt settings, so failures should be handled with targeted exception handling and clear behavior.
- pr_agent/servers/github_action_runner.py[38-77]
[correctness] Injection lost after repo settings
Injection lost after repo settings
In workflow_run events, run_action() injects artifact context before the PR URL is known, then apply_repo_settings(pr_url) can overwrite tool sections (including extra_instructions), and the later workflow_run injection attempt is skipped due to the ARTIFACTS._INJECTED guard. This can silently drop artifact context from prompts in the workflow_run flow._inject_artifact_context() is called unconditionally before event dispatch. For workflow_run, repo settings are applied later (once the PR URL is extracted), and apply_repo_settings() can replace tool sections (including extra_instructions), effectively removing the previously injected artifact context. Because _inject_artifact_context() sets ARTIFACTS._INJECTED, the later call in the workflow_run branch becomes a no-op, so artifact context never gets re-applied.
This primarily impacts the new workflow_run trigger, where the PR URL is not available when the early injection runs.
- pr_agent/servers/github_action_runner.py[33-37]
- pr_agent/servers/github_action_runner.py[157-159]
- pr_agent/servers/github_action_runner.py[243-270]
- pr_agent/git_providers/utils.py[306-316]
- Do not call
_inject_artifact_context()before event dispatch for all events. - Instead, call it only in branches where repo settings are already applied:
-
pull_request/pull_request_target: after the existingapply_repo_settings(pr_url)block. -
workflow_run: afterapply_repo_settings(pr_url)in theworkflow_runbranch. - Alternatively, make
_inject_artifact_context()resilient to repo settings reloads by: - delaying setting
ARTIFACTS._INJECTEDuntil after repo settings are finalized, or - re-injecting when repo settings were applied after the first injection (e.g., reset
_INJECTEDafterapply_repo_settingsfor workflow_run), while ensuring it still remains idempotent per run.
[performance] Truncation exceeds `max_artifact_size`
Truncation exceeds `max_artifact_size`
`_read_and_truncate()` truncates to `max_size` but then appends a truncation marker, causing injected artifact content to exceed the configured maximum. This undermines the compliance goal of keeping injected artifact content within a strict size bound to protect prompt size and performance._read_and_truncate() is intended to enforce a maximum artifact size (characters) before the artifact is injected into tool extra_instructions. However, when truncation happens it slices the content to max_size and then appends a truncation marker, making the returned string longer than max_size.
This violates the requirement that artifact content be truncated to at most the configured maximum size before injection.
- pr_agent/algo/artifacts.py[52-54]
| PR 2492 (2026-07-02) |
[reliability] Issue-context check-run crash
Issue-context check-run crash
GithubProvider._publish_check_run() assumes self.last_commit_id and self.pr exist; for issue URLs (or providers instantiated without a PR), last_commit_id is never initialized, so `if not self.last_commit_id` can raise AttributeError and prevent the intended fallback-to-comment behavior when github.publish_as_check_run=true._publish_check_run() directly dereferences self.last_commit_id and later self.pr._requester. In issue-only flows (or any GithubProvider created without a PR), last_commit_id is not set, so accessing it can raise AttributeError and abort publishing before the caller can fall back to PR comments.
-
GithubProvider.__init__only setsself.last_commit_idinside the PR-url branch. -
publish_persistent_comment()routes to_publish_check_run()whenevergithub.publish_as_check_runis enabled. - Some flows call
publish_persistent_comment()from generic utility code, not just PR-review publishing.
- pr_agent/git_providers/github_provider.py[33-62]
- pr_agent/git_providers/github_provider.py[373-452]
- pr_agent/git_providers/utils.py[340-365]
- Change the first guard to use
last_commit = getattr(self, "last_commit_id", None)and also ensureself.prexists before using_requester. - If either is missing, log once and
return Falseso the caller falls back topublish_persistent_comment_full(). - Consider adding a small unit test for the “issue/no-PR context” case (provider without
last_commit_idattr) to ensure no exception is raised and it returns False.
[correctness] Improve bypasses check runs
Improve bypasses check runs
The github.publish_as_check_run behavior is only implemented in GithubProvider.publish_persistent_comment(), but the /improve tool (PRCodeSuggestions) publishes via publish_persistent_comment_with_history() which edits/creates PR comments directly, so output still lands in PR comments even when publish_as_check_run=true.github.publish_as_check_run only affects GithubProvider.publish_persistent_comment(). The /improve implementation uses a separate persistence mechanism (publish_persistent_comment_with_history) that never calls publish_persistent_comment(), so it never reaches the check-run publishing hook.
- PRAgent routes
improvetoPRCodeSuggestions. - In persistent mode,
PRCodeSuggestionscallspublish_persistent_comment_with_history, which usesedit_comment/publish_commentrather thanpublish_persistent_comment.
- pr_agent/git_providers/github_provider.py[373-381]
- pr_agent/agent/pr_agent.py[23-35]
- pr_agent/tools/pr_code_suggestions.py[156-170]
- pr_agent/tools/pr_code_suggestions.py[251-355]
- Add a check-run-aware branch in
PRCodeSuggestionspersistent publishing: - If
get_settings().github.publish_as_check_runis true and provider is GitHub, publish viagit_provider.publish_persistent_comment(pr_body, initial_header=..., name=...)(or call a newgit_provider.publish_check_run(...)capability). - Skip or adapt the comment-history logic for this mode (otherwise it will still create/update PR comments).
- Add/adjust a unit test that asserts /improve’s persistent path calls
publish_persistent_comment(or the new check-run method) when the setting is enabled.
[correctness] Check-run output clobbered
Check-run output clobbered
With `github.publish_as_check_run=true`, `GithubProvider.publish_persistent_comment()` selects the target check run solely from the `name` parameter (default `'review'`). Existing callers like `handle_configurations_errors()` invoke `publish_persistent_comment()` without `name`, so their output will be published into (and overwrite) the same `PR Agent - Review` check run used for the actual PR review.When github.publish_as_check_run is enabled, the check-run identity is derived from name (default review). Any caller that omits name will publish into the Review check run and overwrite review results.
At least one existing call site (handle_configurations_errors) calls publish_persistent_comment(...) without passing name, which becomes problematic only after this PR’s new check-run publishing path.
- pr_agent/git_providers/utils.py[340-364]
- pr_agent/git_providers/github_provider.py[373-389]
- Update
handle_configurations_errors()to pass an explicit, non-reviewname(e.g.,name="config"orname=f"repo-settings-{config_type}"). - (Optional hardening) In
GithubProvider.publish_persistent_comment(), consider deriving a safer check-run key whennameis omitted/defaulted (e.g., include a sanitizedinitial_headerfragment) to prevent future collisions from other callers that forget to setname.
[reliability] Check-run failures drop output
Check-run failures drop output
When `github.publish_as_check_run` is enabled, `publish_persistent_comment()` returns immediately after calling `_publish_check_run()`, but `_publish_check_run()` swallows API exceptions and does not provide a fallback to publish a PR comment, so review output can be silently lost on GitHub API errors. This makes the overall publish operation potentially become a no-op without signaling failure.When publish_as_check_run is enabled, failures in the GitHub Checks API path can result in no output being published because _publish_check_run() catches exceptions and only logs warnings, while publish_persistent_comment() returns immediately without falling back to publishing a persistent PR comment.
-
publish_persistent_comment()returns right after calling_publish_check_run(pr_comment, name)when check-run mode is enabled. -
_publish_check_run()catches broadExceptionduring PATCH/POST and only logs warnings, masking failures. - There is no fallback to
publish_persistent_comment_full(...)(or equivalent) when the check-run publish fails, so the publish operation can become a no-op.
- pr_agent/git_providers/github_provider.py[374-422]
[reliability] `_check_run_ids` shared across instances
`_check_run_ids` shared across instances
`_check_run_ids` is defined as a mutable class-level dictionary keyed only by tool `name`, so check-run IDs can leak across `GithubProvider` instances and requests in the same process and cause updates to target the wrong check run. This can lead to cross-PR contamination in long-running processes/tests when multiple PRs are handled by the same worker._check_run_ids is currently a mutable class attribute shared across all GithubProvider instances, and it is keyed only by the tool/check name; in server/worker modes that handle multiple PRs in the same process, this can leak check-run IDs across requests and cause updates to be applied to the wrong PR’s check run.
The mapping is mutated during _publish_check_run() and is intended to be per-provider/per-PR (or otherwise scoped to a specific PR context), not global across all instances. In long-running processes/tests, get_git_provider_with_context can create new provider instances for different PR URLs while the class-level _check_run_ids persists, enabling ID reuse across PRs when self._check_run_ids.get(name) is used.
- pr_agent/git_providers/github_provider.py[34-61]
- pr_agent/git_providers/github_provider.py[372-422]
- pr_agent/git_providers/init.py[40-65]
[maintainability] `_publish_check_run()` does too much
`_publish_check_run()` does too much
The new `_publish_check_run()` function combines formatting/truncation, check-run lookup, and API update/create behavior in a single method, making it harder to reason about and maintain. This violates the single-responsibility guideline and increases change-risk for future edits.GithubProvider._publish_check_run() currently mixes multiple responsibilities (content preparation, existing check-run discovery, and update/create API operations) in one method, reducing maintainability.
This method was added as part of supporting GitHub Check Runs as an output mechanism.
- pr_agent/git_providers/github_provider.py[383-440]
[reliability] Check-run lookup not paginated
Check-run lookup not paginated
GithubProvider._find_existing_check_run() fetches check runs for a commit only once and scans only that response page. If the commit has enough check runs to paginate, the PR Agent check run may not be found and _publish_check_run() will create a new check run instead of updating the existing one._find_existing_check_run() calls the GitHub check-runs list endpoint once and searches only the returned check_runs array. On commits with many check runs, the matching check run may be on a subsequent page, so the lookup returns None and _publish_check_run() creates a duplicate check run instead of updating in-place.
This impacts the new github.publish_as_check_run path, where persistence relies on finding the existing check run by name + head SHA.
- pr_agent/git_providers/github_provider.py[441-452]
- pr_agent/git_providers/github_provider.py[383-418]
- Implement pagination when listing check runs (e.g., request
per_page=100and followLinkheaders until exhausted), and stop early once a match is found. - Alternatively (or additionally), use query params supported by the endpoint to narrow results by check name if available, to reduce payload size and paging needs.
| PR 2491 (2026-07-02) |
[reliability] Brittle push-support unit test
Brittle push-support unit test
`test_run_without_push_support` tries to simulate a provider without `create_or_update_pr_file` by `delattr`-ing the attribute on a bare `MagicMock`, but `PRUpdateChangelog` uses `hasattr()` for detection and the mock may still appear to have the attribute, causing the test to execute the wrong path. This can make the test fail to assert the intended early-exit behavior (or become brittle) and can let regressions in the skip logic slip through.test_run_without_push_support uses a plain MagicMock() and then calls delattr(mock_git_provider, 'create_or_update_pr_file') to simulate a provider that does not support pushing. Because the production code checks support via hasattr(self.git_provider, "create_or_update_pr_file"), the test double should behave like a real object for attribute existence; a bare MagicMock is not a reliable stand-in.
The goal of the test is to ensure /update_changelog exits early with a clear comment when push is requested but the provider cannot push.
- tests/unittest/test_pr_update_changelog.py[142-157]
- pr_agent/tools/pr_update_changelog.py[27-37]
In the test, replace the bare MagicMock() provider with either:
- A small stub class/object that implements only the fields/methods the early-exit path touches (e.g.,
publish_comment,get_pr_branch,get_pr_description,get_commit_messages, andpr.title) and does not definecreate_or_update_pr_file. or - A
MagicMock(spec_set=[...])that explicitly omitscreate_or_update_pr_fileso thathasattr(mock, 'create_or_update_pr_file')correctly returnsFalse. This keeps the test aligned with the production feature-detection mechanism and ensures it reliably exercises the intended branch.
[security] Logs `dict(get_settings())` configs
Logs `dict(get_settings())` configs
`PRUpdateChangelog.run()` logs full configuration dictionaries via `artifacts=relevant_configs`, which can leak sensitive configuration values into logs. This violates the requirement to treat logs as a security boundary and avoid logging full settings/secrets.get_logger().debug("Relevant configs", artifacts=relevant_configs) logs full dict(get_settings().config) / dict(get_settings().pr_update_changelog) which can unintentionally expose sensitive values.
Compliance requires treating logs as a security boundary and never logging full settings dictionaries or secret-bearing values.
- pr_agent/tools/pr_update_changelog.py[86-88]
[maintainability] Restricted mode permissions undocumented
Restricted mode permissions undocumented
The new Restricted Mode docs instruct setting `config.restricted_mode = true` but do not explicitly state the minimal GitHub permissions required in restricted mode (e.g., whether `contents` should be omitted/none/read). This leaves permissions requirements ambiguous and undermines least-privilege setup guidance.Restricted mode documentation mentions skipping operations that need contents: write, but it does not explicitly list the minimal GitHub permissions for restricted mode and whether contents is required at all (none/read/write).
Compliance requires documentation to clearly state the GitHub App/workflow permissions for both normal mode and restricted mode, especially clarifying the contents permission.
- docs/docs/installation/github.md[462-470]
- docs/docs/usage-guide/additional_configurations.md[284-293]
[correctness] Misleading commit hint
Misleading commit hint
When `push_changelog_changes=true` but pushing is skipped (`push_skipped_reason` set), `commit_changelog` becomes `False` and `_prepare_changelog_update()` appends instructions to rerun with `push_changelog_changes=true` even though that will not enable pushing. This produces contradictory guidance in the comment output and can send users into a loop.PRUpdateChangelog now sets commit_changelog to False not only when push wasn’t requested, but also when push was requested and then skipped due to restricted_mode or missing push support. _prepare_changelog_update() currently treats all commit_changelog == False cases the same and always appends a “rerun with push_changelog_changes=true” instruction, which is misleading when pushing is impossible.
-
commit_changelogis derived from both the user request and push feasibility. -
_prepare_changelog_update()appends the rerun instruction solely based onnot self.commit_changelog.
- Change
_prepare_changelog_update()to append the “rerun to commit” instruction only whenself.push_changelog_changesisFalse(i.e., push wasn’t requested). - If
self.push_skipped_reasonis set, replace that instruction with a message explaining what needs to change (e.g., disablerestricted_mode/ grant permissions / use a provider that supports pushing), or omit the instruction entirely since the comment already includes a “not pushed” note.
- pr_agent/tools/pr_update_changelog.py[22-41]
- pr_agent/tools/pr_update_changelog.py[139-155]
[correctness] Skip path drops changelog output
Skip path drops changelog output
When `_skip_push` is set (restricted/unsupported push), `PRUpdateChangelog.run()` returns before generating the changelog entry, so users only get a “pushing is …” message and no proposed changelog content. The tool already has a non-push flow (publish updates as a comment when `commit_changelog` is false), but the skip path never reaches it.When pushing changelog changes is blocked (provider doesn’t support it, or restricted mode disables push_code), PRUpdateChangelog.run() exits immediately and never produces the changelog update content. This loses useful output even though generating/publishing the proposed changelog entry does not require a repository write.
The tool already supports a non-push workflow (commit_changelog == False) that publishes the generated changelog updates as a PR comment.
- pr_agent/tools/pr_update_changelog.py[27-49]
- pr_agent/tools/pr_update_changelog.py[81-112]
- For
_skip_push == "restricted"(and potentially also for "not supported"), setself.commit_changelog = Falseand continue the normal generation path. - Publish a clear note alongside the generated changelog output indicating that pushing was skipped due to restriction/unsupported provider.
- Keep the early-return only for cases where even generation can’t proceed (if any), otherwise prefer fallback-to-comment behavior.
| PR 2490 (2026-07-02) |
[correctness] Synchronize bypasses pr_actions
Synchronize bypasses pr_actions
The new `if action == "synchronize"` branch returns early when `handle_push_trigger` is disabled, and `synchronize` never reaches the `elif action in pr_actions:` branch at all. As a result, auto tools cannot run on `synchronize` even if users add `"synchronize"` to `github_action_config.pr_actions` as documented.synchronize is handled in a dedicated branch that prevents the pr_actions flow from ever running for synchronize events. This contradicts the docs added in this PR stating that adding "synchronize" to pr_actions enables automatic tools on new commits.
Current structure:
-
if action == "synchronize": ... returnon disabled push-trigger -
elif action in pr_actions:auto tools Because of theif/elifstructure, once thesynchronizeblock executes, it can’t fall through to thepr_actionspath.
- pr_agent/servers/github_action_runner.py[116-190]
- docs/docs/usage-guide/automations_and_usage.md[187-195]
Refactor the condition to only intercept synchronize when push-trigger is enabled, e.g.:
- compute
push_triggerand useif action == "synchronize" and is_true(push_trigger): ... - otherwise allow
elif action in pr_actions:to handlesynchronizewhen the user explicitly included it. (Alternatively, if the intended behavior is thatsynchronizeis never part of the auto-tools path, update the docs accordingly.)
[correctness] Fallback push trigger blocked
Fallback push trigger blocked
`run_action()` checks `github_action_config.handle_push_trigger` first, and because the default config now always defines it as `false`, Dynaconf will never fall back to `github_app.handle_push_trigger` even if users enabled it there. This causes `synchronize` events to be skipped unexpectedly and defeats the documented fallback behavior.run_action() intends to fall back to github_app.handle_push_trigger when github_action_config.handle_push_trigger is not set. However, the PR adds handle_push_trigger = false to [github_action_config] in the default configuration.toml, which makes the key always present and therefore prevents the fallback from ever being used.
Dynaconf’s get(key, default) only uses default when key is missing; it does not use it when the key exists but is set to false.
- pr_agent/settings/configuration.toml[236-239]
- pr_agent/servers/github_action_runner.py[118-124]
- Remove (or comment out)
handle_push_trigger = falsefrom the[github_action_config]defaults so the key is absent by default and the code can truly fall back togithub_app.handle_push_trigger. - Keep the effective default behavior as
falseby relying on the code’s final default (False) when neither key is set.
| PR 2482 (2026-06-29) |
[security] Line prompt forces answering always
Line prompt forces answering always
The line-level `/ask` system prompt still includes unconditional “must always answer” language that can conflict with `extra_instructions` intended to require refusals for disallowed questions (e.g., PR rating requests). This direct prompt conflict can undermine policy enforcement by causing the model to answer when it should refuse.The /ask system prompt (including the line-level prompt) contains unconditional language requiring the model to always answer questions / not avoid answering, which can contradict extra_instructions that are meant to restrict behavior and require refusal for disallowed question types.
PR Compliance ID 6 expects extra_instructions to be enforceable and to steer behavior, including refusing disallowed requests (e.g., “rate this PR 1–10”) with an explanation. Because the current /ask prompt includes “must answer” instructions alongside an extra_instructions block, the unconditional “always answer” requirement can override or conflict with refusal requirements, undermining compliance for line-level questions.
- pr_agent/settings/pr_line_questions_prompts.toml[4-16]
- pr_agent/settings/pr_questions_prompts.toml[4-15]
| PR 2479 (2026-06-26) |
[correctness] Unencoded spaces in commit URL
Unencoded spaces in commit URL
After decoding `%20` into literal spaces in `_parse_pr_url`, `get_latest_commit_url()` builds a URL by string concatenation using `self.workspace_slug`/`self.repo_slug`, which can now contain spaces; this yields URLs with raw spaces that can be malformed when embedded in markdown output (e.g., PR description update comments).AzureDevopsProvider._parse_pr_url now decodes percent-encoded workspace/repo segments, which is correct for Azure DevOps API identifiers. However, other code later constructs URLs using these decoded values via raw string concatenation, producing URLs that may contain literal spaces (invalid in URL paths) and can break when embedded in markdown.
Example after this PR:
- workspace_slug = "Dev Project"
- repo_slug = "repo name"
- get_latest_commit_url() returns
.../Dev Project/_git/repo name/commit/<sha>(contains spaces)
Downstream code embeds latest_commit_url directly in markdown strings.
- pr_agent/git_providers/azuredevops_provider.py[641-645]
When constructing URLs (at least in get_latest_commit_url), percent-encode path segments, e.g.:
- import
quotefromurllib.parse - build:
.../{quote(self.workspace_slug, safe='')}/_git/{quote(self.repo_slug, safe='')}/commit/...(Keep the decoded values for API calls; only re-encode for URL generation.)
| PR 2475 (2026-06-26) |
[correctness] Stale title fallback
Stale title fallback
`BitbucketServerProvider.publish_description()` falls back to the cached `self.pr.title` when `pr_title` is `None`, so a manual title edit made while `/describe` is running can still be reverted on publish. This undermines the intended fix for Bitbucket Server because `self.pr` is only fetched once in `set_pr()` and not refreshed before update.Bitbucket Server uses self.pr.title as a fallback when pr_title is None, but self.pr is fetched once at set_pr() time and can be stale by the time /describe publishes. If a user edits the PR title during the describe run, the publish call can still send the old title and revert the user’s change.
-
/describenow passespr_title=Nonewhengenerate_ai_titleis false. - Bitbucket Server cannot omit
titlein the update payload (omitted fields get wiped), so it must supply a title value. - To actually “leave title unchanged”, the provider should fetch the latest PR state right before updating, and use that latest title/version/reviewers as the fallback.
- pr_agent/git_providers/bitbucket_server_provider.py[197-200]
- pr_agent/git_providers/bitbucket_server_provider.py[514-529]
In publish_description, if pr_title is None, re-fetch the PR (latest_pr = self._get_pr()), and build the payload using latest_pr.title, latest_pr.version, and latest_pr.reviewers (and optionally update self.pr = latest_pr) so the update preserves the most recent server-side title rather than a cached snapshot.
[correctness] Local output drops title
Local output drops title
LocalGitProvider.publish_description() writes only pr_body when pr_title is None, which removes the title line from the persisted description.md output and violates the new "leave existing title unchanged" contract. With generate_ai_title=false (default), PRDescription now passes None, so local /describe publishes will lose the title content.LocalGitProvider.publish_description() treats pr_title=None as "write only body", which drops the title line from description.md. Under the new contract, None means "do not change the existing title".
PRDescription.run() now passes pr_title=None when generate_ai_title is false (default). For the local provider, the output file is the only persisted representation, so omitting the title effectively erases it.
- pr_agent/git_providers/local_git_provider.py[112-116]
When pr_title is None, preserve the existing title for local output (e.g., use self.pr.title, or read the first line of the existing description.md if present) and write f"{existing_title}\n{pr_body}" instead of writing body-only.
| PR 2470 (2026-06-22) |
[maintainability] Misrendered list note
Misrendered list note
The new note under step 1 is not indented as part of the numbered list, and the second line is not part of the blockquote, so many Markdown renderers will display the note outside the list item (or inconsistently), reducing readability of the installation steps.In docs/docs/installation/gitlab.md, the note added after step 1 is not properly nested under the ordered list item: the > **Note:** ... line is unindented (top-level) and the continuation line is not prefixed with >.
This can cause the rendered documentation to show the note outside the numbered list item (or render inconsistently across Markdown engines).
This repo already uses MkDocs-style admonitions (e.g., !!! note ...) and also uses blockquote notes (> **Note**:). When a note is intended to belong to a list item, it should be indented to be part of that list item, and blockquotes should prefix all note lines with >.
- docs/docs/installation/gitlab.md[50-52]
Option A (nested blockquote):
[maintainability] GitLab docs line too long
GitLab docs line too long
The updated GitLab installation step is written as a single very long Markdown line, exceeding the repo’s 120-character line-length convention. This can trigger style/tooling issues and reduces readability of the docs diff.The updated GitLab installation instruction is a single line that exceeds the repository’s 120-character line-length convention.
The long inline parenthetical note should be wrapped across multiple lines (or converted into a nested bullet / separate note block) to match repo formatting/tooling expectations and improve readability.
- docs/docs/installation/gitlab.md[50-50]
| PR 2467 (2026-06-22) |
[correctness] Ask bypasses diff provider
Ask bypasses diff provider
PRQuestions constructs the provider via get_git_provider()(pr_url), which lacks the plain-diff override, so `--stdin/--diff-file ask` can instantiate a hosted provider after apply_repo_settings() overwrites config.git_provider and break tokenless mode.ask is documented as supported in plain-diff mode, but PRQuestions uses get_git_provider()(pr_url) instead of get_git_provider_with_context(pr_url). Only get_git_provider_with_context() contains the new “force plain-diff when plain_diff.content is set” logic.
Because PRAgent._handle_request() calls apply_repo_settings(pr_url) before tool instantiation, repo/extra config can overwrite config.git_provider away from plain-diff, and ask then routes to the wrong provider.
This is specific to the new plain-diff routing behavior added in this PR.
- pr_agent/tools/pr_questions.py[18-24]
- pr_agent/agent/pr_agent.py[55-58]
- pr_agent/git_providers/init.py[30-67]
- Change
PRQuestionsto construct the provider viaget_git_provider_with_context(pr_url)(matching reviewer/describe/improve). - (Optional) Apply the same change to
pr_line_questions.pyif you want parity forask_line.
| PR 2464 (2026-06-21) |
[correctness] Azure prefix bypasses Databricks
Azure prefix bypasses Databricks
When Azure mode is enabled, chat_completion() rewrites the model string to start with "azure/", so the new databricks/* checks never trigger and Databricks calls may be routed/authenticated using the wrong provider settings. This breaks Databricks usage in multi-provider configs that also enable Azure (OPENAI.API_TYPE=azure or AZURE_AD).LiteLLMAIHandler.chat_completion() prepends azure/ to all model names when self.azure is true. The new Databricks logic keys off model.startswith("databricks/") to (a) select api_base from DATABRICKS_API_BASE and (b) avoid forwarding litellm.api_key. If the model is rewritten first (e.g. databricks/foo -> azure/databricks/foo), both guards are bypassed.
This shows up specifically in multi-provider configurations where Azure is enabled but the request model is a Databricks model.
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[418-421]
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[471-482]
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[553-559]
Compute is_databricks = model.startswith("databricks/") (or extract provider prefix) before any Azure rewriting, then:
- Only apply
model = "azure/" + modelwhen notis_databricks(and ideally when the model isn’t already provider-qualified). - Use
is_databricksfor both theapi_baseselection and theapi_keyforwarding guard, rather than re-checkingstartswith()on a potentially mutatedmodel.
[correctness] Databricks api_base overridden
Databricks api_base overridden
`LiteLLMAIHandler.chat_completion()` always includes `api_base=self.api_base` in the LiteLLM call, so if another provider configured `self.api_base` during `__init__`, `databricks/*` calls can be sent to the wrong base URL despite `DATABRICKS_API_BASE` being set. The PR only guards `api_key` forwarding for Databricks, leaving `api_base` unguarded and able to override Databricks endpoint selection in multi-provider configs.Databricks is configured via DATABRICKS_API_KEY/DATABRICKS_API_BASE, but chat_completion() always forwards api_base=self.api_base. When self.api_base was set by another provider (e.g., OpenRouter/Azure AD/Ollama), Databricks calls may be routed to the wrong host.
The PR added a Databricks-specific guard for api_key, but api_base is still unconditionally included in the request kwargs.
For model.startswith("databricks/"), do not pass a foreign api_base:
- either omit
api_baseentirely fromkwargsfor Databricks models, or - set it explicitly from
DATABRICKS_API_BASE(if present) and otherwise remove it. Add a unit test mirroring the existingapi_keyguard test to assertapi_baseis not forwarded fordatabricks/*whenself.api_baseis set to another provider.
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[472-478]
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[549-556]
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[224-235]
- tests/unittest/test_litellm_api_key_guard.py[323-348]
[correctness] Wrong key forwarded
Wrong key forwarded
`LiteLLMAIHandler.chat_completion` forwards `litellm.api_key` for all models when it is set, so configuring another provider (e.g., GROQ/OPENROUTER) can cause that key to be passed for `databricks/*` calls and override the intended `DATABRICKS_API_KEY` env-var auth. This can make Databricks-hosted models fail authentication in multi-provider configurations.Databricks support exports credentials via DATABRICKS_API_KEY/DATABRICKS_API_BASE env vars, but the request path may still pass kwargs["api_key"] = litellm.api_key whenever any provider populated litellm.api_key. This can accidentally send (and prefer) a non-Databricks key for databricks/* models.
-
__init__setslitellm.api_keyfor some providers (Groq/SambaNova/XAI/OpenRouter/etc.) without regard to which model is being called. -
chat_completioninjectsapi_keyintoacompletion()kwargs based solely on whetherlitellm.api_keyis set. - Databricks auth is intended to come from
DATABRICKS_API_KEYenv var, so passing an explicitapi_keyfrom another provider can break Databricks auth.
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[549-553]
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[157-165]
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[203-209]
Update the api_key injection guard to be provider-aware (e.g., only inject for providers that require api_key in kwargs), and explicitly avoid injecting for databricks/* models so LiteLLM can use DATABRICKS_API_KEY/DATABRICKS_API_BASE.
[reliability] Test state leakage
Test state leakage
The new Databricks provider unit tests instantiate `LiteLLMAIHandler()`, which mutates global `litellm.api_key` (dummy fallback) but the autouse fixture only cleans environment variables. This can leak global state into later tests and make the suite order-dependent/flaky.LiteLLMAIHandler.__init__ mutates global litellm module state (notably litellm.api_key when OPENAI_API_KEY is absent). The new test file only isolates/cleans os.environ, so litellm.api_key can remain changed after the test and affect other tests.
This repo already has tests that explicitly reset litellm.api_key to avoid cross-test pollution; this new test should do the same.
- tests/unittest/test_litellm_databricks_provider.py[48-57]
- tests/unittest/test_litellm_databricks_provider.py[59-89]
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[53-57]
In the autouse fixture, snapshot and restore litellm.api_key (and any other mutated litellm globals you touch/observe) or explicitly set them to a known value before/after LiteLLMAIHandler().
| PR 2460 (2026-06-21) |
[reliability] Missing A2A-Version guard
Missing A2A-Version guard
PRAgentExecutor.execute constructs TaskUpdater using context.task_id/context_id before entering its try/except, so any request path that yields a RequestContext without these A2A 1.0 fields will raise before the task can be initialised and failed cleanly. The app does not enforce the required A2A-Version: 1.0 header at the Starlette boundary, even though tests document that the header is required to avoid non-1.0 handling.PRAgentExecutor.execute() assumes A2A 1.0 RequestContext fields (task_id, context_id) are always present and uses them before the try/except. If a request is handled without the required A2A-Version: 1.0 header (or otherwise produces a context without those fields), the executor can crash before emitting the required initial TaskArtifactUpdateEvent / failure status.
Tests explicitly document that A2A-Version: 1.0 is required to avoid non-1.0 handling, but build_app() does not enforce this requirement.
- Add explicit request validation for
A2A-Version: 1.0on the JSON-RPC route (reject early with a clear JSON-RPC error / HTTP 400). - Make
PRAgentExecutor.execute()robust iftask_id/context_idare missing (e.g., validate and raise a controlled error inside thetry, or provide a backward-compatible fallback). - pr_agent/mosaico/server.py[78-93]
- pr_agent/mosaico/executor.py[38-70]
[maintainability] Floating a2a-sdk version
Floating a2a-sdk version
requirements.txt changes a2a-sdk from a fixed pin to a version range, making installs non-reproducible relative to the otherwise pinned dependency set and increasing the risk of unexpected behavior changes on new 1.0.x releases. This can cause CI/runtime drift where the same commit resolves different a2a-sdk patch versions.a2a-sdk[http-server] is specified as a floating range (>=1.0.0,<1.1.0) while most other dependencies are pinned, which can lead to non-reproducible builds and patch-version drift.
This repo appears to prefer deterministic dependency resolution (many == pins). Allowing a moving target for a core protocol SDK can introduce hard-to-debug runtime differences.
- Decide on a specific known-good
1.0.xversion and pin it (or adopt a lockfile approach if intentional). - requirements.txt[29-33]
| PR 2459 (2026-06-21) |
[security] SSRF via diff fetch
SSRF via diff fetch
_fetch_public_diff() performs an unauthenticated aiohttp GET to a user-supplied PR URL (only path-pattern validated) and follows redirects, allowing external users to induce outbound HTTP requests to arbitrary/internal hosts from the MOSAICO server._fetch_public_diff() builds diff_url from user input and fetches it with redirects enabled. Because _PR_URL_RE does not restrict the hostname (only the path shape), this allows SSRF (including to internal IP ranges or attacker-controlled hosts), and redirects further expand reachable targets.
This code path is reachable from route_and_run_result() which is fed from MOSAICO user text, so the URL is attacker-controlled input.
- pr_agent/mosaico/dispatch.py[30-34]
- pr_agent/mosaico/dispatch.py[116-137]
- Parse the URL and enforce an allowlist of supported public hosts (e.g.,
github.com,gitlab.com, plus an explicit configurable allowlist for enterprise domains). - Block non-HTTPS schemes.
- Disable redirects (
allow_redirects=False) or only allow redirects that stay on the same hostname and scheme. - Optionally add IP safety checks (reject private/loopback/link-local/reserved ranges after DNS resolution), but keep the hostname allowlist as the primary control.
[maintainability] Duplicate Langfuse callbacks
Duplicate Langfuse callbacks
_register_langfuse_callback() appends `langfuse_otel` but does not replace/remove an existing `langfuse` callback, so environments already configured with `langfuse` can end up with both callbacks set and may double-instrument LLM calls.The Langfuse callback migration can leave both langfuse and langfuse_otel configured at the same time. Since LiteLLMAIHandler copies the settings callback lists directly onto litellm.*_callback, both entries will be active if present.
This is most likely in deployments that previously set LITELLM.SUCCESS_CALLBACK=["langfuse"] / LITELLM.FAILURE_CALLBACK=["langfuse"] in config/env; after this change, apply_mosaico_env() will append langfuse_otel without removing the legacy entry.
- pr_agent/mosaico/env_bridge.py[63-69]
- When registering, treat
langfuseas deprecated and normalize the list to only containlangfuse_otel(e.g., removelangfuseif present, then ensurelangfuse_otelexists). - Alternatively, if both are present, remove the legacy one to prevent double-callback execution.
| PR 2454 (2026-06-17) |
[maintainability] Trailing whitespace in model list
Trailing whitespace in model list
The newly added "claude-fable-5" entry in `NO_SUPPORT_TEMPERATURE_MODELS` contains trailing whitespace, which can fail the repo’s trailing-whitespace pre-commit hygiene checks and cause unnecessary diff churn. While minor, it can block local commits/CI when hooks are enforced.A newly added line in NO_SUPPORT_TEMPERATURE_MODELS for "claude-fable-5" contains trailing whitespace at the end of the line, which violates the repo’s no-trailing-whitespace hygiene requirements and can fail the configured trailing-whitespace pre-commit hook.
Trailing whitespace can trigger pre-commit failures (and potentially block local commits/CI when hooks are enforced) and creates avoidable formatting/diff churn. Remove the extra spaces so the line ends immediately after the comma.
- pr_agent/algo/init.py[318-320]
- .pre-commit-config.yaml[7-16]
[maintainability] Duplicate model entry
Duplicate model entry
NO_SUPPORT_TEMPERATURE_MODELS contains "anthropic/claude-opus-4-7" twice, which is confusing and error-prone if the list is later iterated or rendered. This duplication was introduced by adding a second copy rather than reusing the existing entry.NO_SUPPORT_TEMPERATURE_MODELS includes "anthropic/claude-opus-4-7" twice.
This list is used for membership checks today, so the duplicate is mostly harmless at runtime, but it increases confusion and makes future edits riskier.
- pr_agent/algo/init.py[314-323]
- Remove one of the two
"anthropic/claude-opus-4-7"entries (keep a single instance). - (Optional) Consider a quick dedup pass (e.g., comment or enforcing uniqueness) if this list is edited frequently.