feat(actions): HPA/VPA right-sizing audit with Datadog percentiles and Slack review card - #592
feat(actions): HPA/VPA right-sizing audit with Datadog percentiles and Slack review card#592isiddharthsingh wants to merge 13 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds Datadog ChangesDatadog metric statistics
HPA/VPA recommendation lifecycle
GitHub API integration documentation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![arvo-ai-staging[bot]](https://avatars.githubusercontent.com/in/4088260?s=60&v=4){kind=small}
There was a problem hiding this comment.
Aurora Risk Review
Verdict: RISKY
This is a well-engineered, large feature addition with thoughtful failure handling throughout. One concrete CI signal stands out: the Linters workflow is currently failing on this PR's head SHA (run 30500910222), which is a gate that should pass before merge. All other infrastructure concerns — table creation ordering across containers, import-time assertions, connection handling, and the read-only mode access control — are correctly handled. The feature is seeded disabled, so no scheduled execution risk exists on deploy.
Findings
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | MEDIUM | server/chat/backend/agent/tools/hpa_vpa_card_tool.py:1 |
Linter CI gate is currently failing on this PR's head SHA |
Aurora reviews PRs for incident prevention. This is advisory only and does not block merge.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/chat/backend/agent/skills/integrations/datadog/SKILL.md`:
- Line 30: Add a blank line immediately after the headings Resource Types, The
interval Parameter, Percentiles, and Reconciliation vs Sizing in the Datadog
skill documentation, before their following content.
- Around line 42-47: Correct the interval documentation in the “The interval
Parameter” section to state that automatic granularity selection applies only to
metric_stats. Document that omitting interval for metrics uses Datadog’s default
resolution, while retaining the existing millisecond units and clamping guidance
where applicable.
In `@server/chat/backend/agent/tools/datadog_tool.py`:
- Line 530: Update query_datadog’s handler invocation to forward its accepted
**kwargs, including source, alongside interval so _query_metric_stats can
perform its source dispatch and unsupported-source validation; alternatively
remove the unused **kwargs contract, but keep the function signature and
behavior consistent.
- Around line 548-555: The truncated-all branch in the result-building flow must
preserve existing series context in result["note"]. Update the assignment under
“if was_truncated and not truncated_results and results_list” to append the
oversized-item message to any existing note rather than replacing it, while
retaining the truncated_all flag and current guidance.
- Around line 371-382: Update the window_note construction in the
expected_points cap check to distinguish auto-picked intervals from
caller-supplied intervals. When the interval was explicitly supplied, explain
that using a coarser interval can avoid the point cap; retain the
coarsest-supported-interval wording and 250-day guidance only for auto-picked
intervals.
In `@server/chat/backend/agent/tools/hpa_vpa_card_tool.py`:
- Around line 100-102: Update the `vcs_provider` schema and `_validate` flow to
match the providers supported by `_CLOSERS`: either remove GitLab and Bitbucket
from the field description or reject them during validation before any row is
claimed. Ensure unsupported providers fail early instead of allowing cards whose
later Dismiss operation cannot close the PR.
In `@server/routes/slack/slack_events.py`:
- Around line 705-710: Update the dismissal flow containing close_pull_request
to acknowledge Slack within the 3-second budget by moving the close and
card-rewrite work off the request thread through the existing Celery actions
mechanism; at minimum, ensure the close_pull_request invocation uses a short
timeout rather than its 30-second default. Preserve the committed dismissal
transition and existing degraded behavior when the GitHub close times out.
In `@server/services/actions/hpa_vpa_recommendations.py`:
- Around line 381-384: Validate repo_full_name against the expected
owner/repository format before constructing the GitHub URL in the relevant
PR-closing function. Add the required re import and reject invalid values,
including path traversal or extra slash segments, before requests.patch is
called; preserve the existing behavior for valid repository names.
In `@server/services/actions/system_actions.py`:
- Around line 73-76: Replace the assert-based invariant check near
SYSTEM_ACTIONS and _DEFAULT_INSTRUCTIONS with an explicit exception raise when
any system keys lack default instructions, preserving the existing missing-key
calculation and error message so the validation remains enforced under
optimization.
In `@server/tests/services/test_hpa_vpa_recommendations.py`:
- Around line 176-178: Update
test_missing_pr_reference_is_rejected_before_any_network_call to monkeypatch
_CLOSERS["github"] with a sentinel closer that records invocations, then assert
it remains uncalled after both invalid close_pull_request calls while retaining
the existing error assertions.
- Around line 91-100: Update compute_severity_score() to reject boolean metric
values in dimensions before numeric processing, since booleans pass numeric
validation. Extend the parameterized invalid-dimensions tests with boolean
current/recommended values and verify they produce the existing invalid/neutral
severity result without affecting cooldown gate logic.
- Around line 122-128: Update both pytest.mark.parametrize decorators at
server/tests/services/test_hpa_vpa_recommendations.py lines 122-128 and 134-140
to use tuple-form parameter names instead of comma-delimited strings, preserving
the existing test cases and values.
In `@server/utils/db/db_utils.py`:
- Around line 1458-1459: Update the DDL for idx_hpa_vpa_recs_pr in the database
initialization or migration flow to restrict uniqueness to rows with status
proposed, while preserving the existing org_id, repo_full_name, and pr_number
columns and pr_number non-null condition. Add the necessary migration step to
drop the existing broader index before recreating it, since IF NOT EXISTS will
not change an existing definition; leave idx_hpa_vpa_recs_live unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c6cb5dfd-4701-43c5-86d8-de1d806f09a3
📒 Files selected for processing (13)
server/chat/backend/agent/access/mode_access_controller.pyserver/chat/backend/agent/skills/integrations/datadog/SKILL.mdserver/chat/backend/agent/tools/cloud_tools.pyserver/chat/backend/agent/tools/datadog_tool.pyserver/chat/backend/agent/tools/github_rca_tool.pyserver/chat/backend/agent/tools/hpa_vpa_card_tool.pyserver/routes/slack/slack_events.pyserver/services/actions/hpa_vpa_action.pyserver/services/actions/hpa_vpa_recommendations.pyserver/services/actions/system_actions.pyserver/tests/chat/test_datadog_metric_stats.pyserver/tests/services/test_hpa_vpa_recommendations.pyserver/utils/db/db_utils.py
| `query_datadog(resource_type=TYPE, query=QUERY, time_from=START, time_to=END, limit=N)` | ||
| `query_datadog(resource_type=TYPE, query=QUERY, time_from=START, time_to=END, limit=N, interval=MS)` | ||
|
|
||
| ### Resource Types |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
markdownlint MD022: add a blank line after these headings.
### Resource Types, ### The interval Parameter, ### Percentiles, and ### Reconciliation vs Sizing are immediately followed by content.
Also applies to: 42-42, 49-49, 70-70
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 30-30: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/chat/backend/agent/skills/integrations/datadog/SKILL.md` at line 30,
Add a blank line immediately after the headings Resource Types, The interval
Parameter, Percentiles, and Reconciliation vs Sizing in the Datadog skill
documentation, before their following content.
Source: Linters/SAST tools
| ### The `interval` Parameter | ||
| `interval` is the rollup granularity in **milliseconds**, and applies to `'metrics'` and | ||
| `'metric_stats'`. Omit it and a granularity is auto-picked that keeps each series under | ||
| ~1000 points -- a 30-day window auto-picks `3600000` (1 hour, 720 points). Values are | ||
| clamped to `60000`..`14400000`. Datadog caps a series at 1500 points, so a long window | ||
| with a fine interval returns less than you asked for; prefer the auto-pick. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Auto-pick claim is wrong for 'metrics'.
_query_metrics deliberately does not auto-pick — omitting interval leaves Datadog's own default resolution (see datadog_tool.py Lines 151-156 and test_plain_metrics_does_not_auto_pick_an_interval). Only metric_stats auto-picks. As written, the agent will assume a ~1000-point rollup for plain metrics queries.
📝 Proposed doc fix
`interval` is the rollup granularity in **milliseconds**, and applies to `'metrics'` and
-`'metric_stats'`. Omit it and a granularity is auto-picked that keeps each series under
-~1000 points -- a 30-day window auto-picks `3600000` (1 hour, 720 points). Values are
-clamped to `60000`..`14400000`. Datadog caps a series at 1500 points, so a long window
-with a fine interval returns less than you asked for; prefer the auto-pick.
+`'metric_stats'`. Values are clamped to `60000`..`14400000`.
+
+- `'metric_stats'`: omit it and a granularity is auto-picked that keeps each series under
+ ~1000 points -- a 30-day window auto-picks `3600000` (1 hour, 720 points).
+- `'metrics'`: omitting it leaves Datadog's own default resolution in place; no auto-pick.
+
+Datadog caps a series at 1500 points, so a long window with a fine interval returns less
+than you asked for; for `metric_stats` prefer the auto-pick.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### The `interval` Parameter | |
| `interval` is the rollup granularity in **milliseconds**, and applies to `'metrics'` and | |
| `'metric_stats'`. Omit it and a granularity is auto-picked that keeps each series under | |
| ~1000 points -- a 30-day window auto-picks `3600000` (1 hour, 720 points). Values are | |
| clamped to `60000`..`14400000`. Datadog caps a series at 1500 points, so a long window | |
| with a fine interval returns less than you asked for; prefer the auto-pick. | |
| ### The `interval` Parameter | |
| `interval` is the rollup granularity in **milliseconds**, and applies to `'metrics'` and | |
| `'metric_stats'`. Values are clamped to `60000`..`14400000`. | |
| - `'metric_stats'`: omit it and a granularity is auto-picked that keeps each series under | |
| ~1000 points -- a 30-day window auto-picks `3600000` (1 hour, 720 points). | |
| - `'metrics'`: omitting it leaves Datadog's own default resolution in place; no auto-pick. | |
| Datadog caps a series at 1500 points, so a long window with a fine interval returns less | |
| than you asked for; for `metric_stats` prefer the auto-pick. |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 42-42: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/chat/backend/agent/skills/integrations/datadog/SKILL.md` around lines
42 - 47, Correct the interval documentation in the “The interval Parameter”
section to state that automatic granularity selection applies only to
metric_stats. Document that omitting interval for metrics uses Datadog’s default
resolution, while retaining the existing millisecond units and clamping guidance
where applicable.
| @pytest.mark.parametrize("new,prior", [ | ||
| (None, 0.60), # unknown new score | ||
| (0.90, None), # unknown prior score | ||
| (0.90, 0.0), # unusable prior | ||
| (0.90, -1.0), | ||
| ("0.9", 0.60), # non-numeric | ||
| ]) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^|/)test_hpa_vpa_recommendations\.py$|pyproject\.toml|pysnooper|ruff' || true
echo
echo "== target occurrences =="
python3 - <<'PY'
from pathlib import Path
p=Path('server/tests/services/test_hpa_vpa_recommendations.py')
if p.exists():
lines=p.read_text().splitlines()
for start,end in [(122,128),(134,140)]:
print(f'-- {p}:{start}-{end}')
for i in range(start, min(end, len(lines))+1):
print(f'{i:4}: {lines[i-1]}')
else:
print('missing target file')
PY
echo
echo "== ruff config =="
for f in pyproject.toml setup.cfg tox.ini.cfg; do
if [ -f "$f" ]; then
echo "-- $f"
sed -n '1,220p' "$f" | rg -n "ruff|flake8-pytest|PT006|select|ignore|lint" -C 2 || true
fi
done
echo
echo "== all annotated `@pytest.mark.parametrize` declarations =="
python3 - <<'PY'
from pathlib import Path
roots=['server/tests']
for root in roots:
for path in Path(root).rglob('*.py'):
text=path.read_text(errors='replace')
if '`@pytest.mark.parametrize`' not in text:
continue
for n,line in enumerate(text.splitlines(),1):
if '`@pytest.mark.parametrize`' in line or (n>1 and 'parametrize' in line):
for i,l in enumerate(text.splitlines()[max(0,n-3):n+5], max(0,n-3)+1):
print(f'{path}:{i}: {l}')
print('---')
PYRepository: Arvo-AI/aurora
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Ruff availability and PT006 semantics probe =="
if command -v ruff >/dev/null 2>&1; then
ruff --version
cat >/tmp/ruff_pt006_probe.py <<'PY'
import pytest
# Ruff's PT006 only flags invalid parameter name patterns.
# It does not prohibit values like None, floats, strings, inf/nan.
def test_none(v):
pass
def test_float(v):
pass
def test_annotated(v: float):
pass
PY
tmpdir="$(mktemp -d)"
cat > "$tmpdir/pyproject.toml" <<'TOML'
[project]
name = "probe"
requires-python = ">=3.10"
[tool.ruff]
select = ["PT006"]
TOML
(cd "$tmpdir" && ruff check /tmp/ruff_pt006_probe.py --config pyproject.toml || true)
else
echo "ruff not installed"
fi
echo
echo "== repository pytest/ruff related dependency declarations =="
git ls-files | rg '(^|/)(requirements.*\.txt|pyproject\.toml|setup\.cfg|setup\.py|Pipfile|poetry\.lock|uv\.lock)' | while read -r f; do
echo "-- $f"
rg -n "pytest|ruff|flake8-pytest|select|ignore" "$f" -C 1 || true
doneRepository: Arvo-AI/aurora
Length of output: 644
Fix the parameter naming format in both decorators. Convert the comma-delimited parameter names to tuple form to match the expected lint/style format.
@server/tests/services/test_hpa_vpa_recommendations.py:122
@server/tests/services/test_hpa_vpa_recommendations.py:134
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 122-122: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
📍 Affects 1 file
server/tests/services/test_hpa_vpa_recommendations.py#L122-L128(this comment)server/tests/services/test_hpa_vpa_recommendations.py#L134-L140
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/tests/services/test_hpa_vpa_recommendations.py` around lines 122 -
128, Update both pytest.mark.parametrize decorators at
server/tests/services/test_hpa_vpa_recommendations.py lines 122-128 and 134-140
to use tuple-form parameter names instead of comma-delimited strings, preserving
the existing test cases and values.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/tests/conftest.py`:
- Around line 149-160: Change the stub setup loop to evaluate installation
status for each entry in _STUBBED_SUBMODULES rather than skipping the entire
namespace based on _is_installed(_ns). Ensure missing google.* submodules are
stubbed even when the google namespace is provided by another installed
distribution, while preserving the existing real-package and
_STUB_WHEN_GOOGLE_ABSENT behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3ecccc74-4305-45c5-acdf-a4a6d295d2b1
📒 Files selected for processing (1)
server/tests/conftest.py
| for _ns in _STUBBED_NAMESPACES: | ||
| if _is_installed(_ns): | ||
| continue # real package installed -- never mask it | ||
| for _mod in _STUBBED_SUBMODULES: | ||
| if _mod.split(".")[0] == _ns: | ||
| _stub(_mod) | ||
| if _ns == "google": | ||
| # These wrappers import google.* at module scope, so they cannot load | ||
| # against stubs even when they are themselves installed. | ||
| for _wrapper in _STUB_WHEN_GOOGLE_ABSENT: | ||
| sys.modules.pop(_wrapper, None) | ||
| sys.modules[_wrapper] = _StubModule(_wrapper) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Namespace-level _is_installed check may mask missing sub-packages under google.*.
_is_installed("google") returns True as soon as any google.* distribution (e.g., google-auth) provides the namespace, causing the whole submodule loop to be skipped — even when a different distribution under the same namespace (e.g., google-genai providing google.genai, or google-cloud-bigquery providing google.cloud.bigquery) is actually absent. Unlike google.generativeai in google_provider.py (which has its own try/except ImportError), other google.* consumers may not be guarded and would hit a real ModuleNotFoundError in that partial-install scenario.
Consider gating per-submodule instead of per-namespace-root:
🔧 Proposed fix for per-submodule gating
for _ns in _STUBBED_NAMESPACES:
- if _is_installed(_ns):
- continue # real package installed -- never mask it
for _mod in _STUBBED_SUBMODULES:
- if _mod.split(".")[0] == _ns:
- _stub(_mod)
+ if _mod.split(".")[0] == _ns and not _is_installed(_mod):
+ _stub(_mod)
if _ns == "google":
- # These wrappers import google.* at module scope, so they cannot load
- # against stubs even when they are themselves installed.
- for _wrapper in _STUB_WHEN_GOOGLE_ABSENT:
- sys.modules.pop(_wrapper, None)
- sys.modules[_wrapper] = _StubModule(_wrapper)
+ # These wrappers import google.genai.* at module scope, so they cannot
+ # load if that specific submodule is missing, even when `google` itself
+ # resolves via another distribution.
+ if not _is_installed("google.genai"):
+ for _wrapper in _STUB_WHEN_GOOGLE_ABSENT:
+ sys.modules.pop(_wrapper, None)
+ sys.modules[_wrapper] = _StubModule(_wrapper)If the slim CI environment always installs/omits all google.* distributions atomically (never partially), this gap is theoretical — worth confirming.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _ns in _STUBBED_NAMESPACES: | |
| if _is_installed(_ns): | |
| continue # real package installed -- never mask it | |
| for _mod in _STUBBED_SUBMODULES: | |
| if _mod.split(".")[0] == _ns: | |
| _stub(_mod) | |
| if _ns == "google": | |
| # These wrappers import google.* at module scope, so they cannot load | |
| # against stubs even when they are themselves installed. | |
| for _wrapper in _STUB_WHEN_GOOGLE_ABSENT: | |
| sys.modules.pop(_wrapper, None) | |
| sys.modules[_wrapper] = _StubModule(_wrapper) | |
| for _ns in _STUBBED_NAMESPACES: | |
| for _mod in _STUBBED_SUBMODULES: | |
| if _mod.split(".")[0] == _ns and not _is_installed(_mod): | |
| _stub(_mod) | |
| if _ns == "google": | |
| # These wrappers import google.genai.* at module scope, so they cannot | |
| # load if that specific submodule is missing, even when `google` itself | |
| # resolves via another distribution. | |
| if not _is_installed("google.genai"): | |
| for _wrapper in _STUB_WHEN_GOOGLE_ABSENT: | |
| sys.modules.pop(_wrapper, None) | |
| sys.modules[_wrapper] = _StubModule(_wrapper) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/tests/conftest.py` around lines 149 - 160, Change the stub setup loop
to evaluate installation status for each entry in _STUBBED_SUBMODULES rather
than skipping the entire namespace based on _is_installed(_ns). Ensure missing
google.* submodules are stubbed even when the google namespace is provided by
another installed distribution, while preserving the existing real-package and
_STUB_WHEN_GOOGLE_ABSENT behavior.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/routes/slack/slack_events.py (3)
634-693: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftEnforce centralized RBAC for dismissal authorization.
This flow authorizes the action through manual Slack-user and organization checks, but does not apply the required centralized permission check. Any linked member of the organization can therefore dismiss a recommendation and trigger PR closure regardless of application permissions. Protect the invoking route with
@require_permissionor the repository’s centralized Slack-compatible RBAC equivalent, while retaining the RLS/org check as defense in depth.As per path instructions, routes under
server/routes/**must use@require_permissionand must not perform manual auth checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/routes/slack/slack_events.py` around lines 634 - 693, Apply the centralized `@require_permission` decorator to the Slack dismissal route containing the shown flow, using the permission required for dismissing recommendations. Remove the manual Slack-user authentication and related warning logic from this route, while retaining the RLS/org ownership check as defense in depth; preserve the existing dismissal transition behavior for authorized callers.Source: Path instructions
745-796: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAdd recovery for failed dismissed-card rewrites.
If
update_messagefails, the database row is already dismissed and this helper only logs the failure. A later click hits the idempotent no-op path, so the original card can retain a live Dismiss button permanently without any way to repair it. Persist a rewrite-needed state or enqueue a retry; alternatively, make the already-dismissed path idempotently rebuild the card.Based on the
status = 'proposed'transition contract, subsequent clicks cannot re-enter the rewrite path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/routes/slack/slack_events.py` around lines 745 - 796, The dismissed-card flow must recover when _rewrite_dismissed_card or its client.update_message call fails. Persist a rewrite-needed state or enqueue a retry, and ensure the already-dismissed/idempotent click path can rebuild the button-free card; do not rely solely on logging because status='proposed' prevents subsequent clicks from re-entering the rewrite path.
720-734: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDo not claim that no cooldown was applied when
mark_mergedfails.The initial dismissal transition has already committed the cooldown. If
mark_mergedraises, the row remains dismissed/cooling down, but the card still says “No cooldown applied.” Track whether the merge-state update succeeded and show an indeterminate/error status when it did not.Based on the committed dismissal transition, the recommendation already carries cooldown state when this branch begins.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/routes/slack/slack_events.py` around lines 720 - 734, Update the already_merged branch around mark_merged to track whether the merge-state update succeeds. Since the dismissal transition has already committed the cooldown, retain that state and change status_line to report an indeterminate/error outcome when mark_merged raises instead of claiming no cooldown was applied; keep the existing success message only for a successful mark_merged call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/chat/backend/agent/tools/datadog_tool.py`:
- Around line 377-392: Update the window_note branching around
_clamp_interval(interval) so a caller-pinned interval that clamps to
_MAX_INTERVAL_MS receives the existing “coarsest supported interval” guidance
instead of the coarser-interval suggestion. Preserve the current pinned-interval
guidance only when a smaller supported interval remains available, and keep the
auto-selected path unchanged.
In `@server/routes/slack/slack_events.py`:
- Around line 39-44: Move the close-pull-request and message-rewrite operations
out of the Slack interaction request handler and enqueue them on the existing
Celery task mechanism before returning the acknowledgement response. Ensure the
handler returns 200 immediately after the dismissal transition is committed,
while the background task performs close_pull_request and update_message with
the existing failure behavior; update the _GITHUB_CLOSE_TIMEOUT usage only as
needed for that asynchronous flow.
- Line 652: Update the logger.exception calls in the unauthenticated Slack-user
warning paths around the relevant handlers to use lazy percent-style formatting,
passing the sanitized slack_user_id as a logging argument. Remove the appended
caught exception because logger.exception includes it automatically, and apply
the same change to all occurrences at the referenced locations.
In `@server/tests/services/test_hpa_vpa_recommendations.py`:
- Around line 182-189: In
test_missing_pr_reference_is_rejected_before_any_network_call, update the
sentinel lambda registered in _CLOSERS so its unused keyword-argument parameter
is named _kwargs or captured with **_. Apply the same unused-parameter rename to
the other sentinel lambda in this test file, without changing behavior.
---
Outside diff comments:
In `@server/routes/slack/slack_events.py`:
- Around line 634-693: Apply the centralized `@require_permission` decorator to
the Slack dismissal route containing the shown flow, using the permission
required for dismissing recommendations. Remove the manual Slack-user
authentication and related warning logic from this route, while retaining the
RLS/org ownership check as defense in depth; preserve the existing dismissal
transition behavior for authorized callers.
- Around line 745-796: The dismissed-card flow must recover when
_rewrite_dismissed_card or its client.update_message call fails. Persist a
rewrite-needed state or enqueue a retry, and ensure the
already-dismissed/idempotent click path can rebuild the button-free card; do not
rely solely on logging because status='proposed' prevents subsequent clicks from
re-entering the rewrite path.
- Around line 720-734: Update the already_merged branch around mark_merged to
track whether the merge-state update succeeds. Since the dismissal transition
has already committed the cooldown, retain that state and change status_line to
report an indeterminate/error outcome when mark_merged raises instead of
claiming no cooldown was applied; keep the existing success message only for a
successful mark_merged call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8032225e-9aef-44d3-b789-ffe154f211b1
📒 Files selected for processing (9)
server/chat/backend/agent/tools/datadog_tool.pyserver/chat/backend/agent/tools/hpa_vpa_card_tool.pyserver/routes/slack/slack_events.pyserver/services/actions/hpa_vpa_recommendations.pyserver/services/actions/system_actions.pyserver/tests/chat/test_datadog_metric_stats.pyserver/tests/conftest.pyserver/tests/services/test_hpa_vpa_recommendations.pyserver/utils/db/db_utils.py
| # Slack expects an interaction response within ~3s. The dismissal transition is | ||
| # already committed before this call, so a timeout only costs the automatic PR | ||
| # close (surfaced on the card as "could not be closed automatically"), never the | ||
| # cooldown. Kept short for that reason; moving the close and card rewrite onto a | ||
| # Celery task would remove the constraint entirely and is the better long-term fix. | ||
| _GITHUB_CLOSE_TIMEOUT = 5 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
Five seconds still exceeds Slack’s acknowledgement budget.
The handler waits for close_pull_request and then performs update_message before returning 200. A slow provider can still exceed Slack’s roughly 3-second response window, causing retries or a failed interaction after the dismissal is already committed. Move close/rewrite work off the request thread; a shorter timeout alone is only a mitigation.
This is the same unresolved Slack acknowledgement-budget issue noted in the previous review.
Also applies to: 712-718
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/routes/slack/slack_events.py` around lines 39 - 44, Move the
close-pull-request and message-rewrite operations out of the Slack interaction
request handler and enqueue them on the existing Celery task mechanism before
returning the acknowledgement response. Ensure the handler returns 200
immediately after the dismissal transition is committed, while the background
task performs close_pull_request and update_message with the existing failure
behavior; update the _GITHUB_CLOSE_TIMEOUT usage only as needed for that
asynchronous flow.
|
Worked through the SonarQube and CodeRabbit findings. Fixed the valid ones; below are the ones I'm deliberately leaving, with reasons. Fixed in
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/routes/slack/slack_events.py (3)
709-742: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle missing and non-GitHub PRs in the failure status.
When
pr_numberis absent, this code intentionally creates a “No PR recorded” result but later rendersPR#None``. It also always tells users to close the change on GitHub even whenvcs_provideris GitLab. Render a no-PR-specific message and derive the provider label from the stored provider.The lifecycle is provider-dispatched per the PR objectives.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/routes/slack/slack_events.py` around lines 709 - 742, Update the failure-status branch around close_result and the dismissed provider so missing pr_number renders a specific “No PR recorded” message instead of PR `#None`, while existing PR failures include the number. Derive the manual-close destination from dismissed.get("vcs_provider") (defaulting consistently to GitHub) and use the appropriate provider label, preserving the provider-dispatched lifecycle.
635-685: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftEnforce dismissal authorization through centralized RBAC.
This handler authorizes the action by checking Slack identity and organization ownership only, so any authenticated organization member can dismiss and close a recommendation. Route authorization must go through the centralized permission policy rather than this inline check; use the RBAC decorator at the route boundary or an equivalent centralized adapter for Slack identities.
As per path instructions, routes must use RBAC decorators (
@require_permission), never manual auth checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/routes/slack/slack_events.py` around lines 635 - 685, Replace the inline authorization logic in the Slack dismissal route, including the clicker identity and organization checks around get_user_id_from_slack_user and set_rls_context, with the centralized RBAC route authorization using `@require_permission`. Ensure Slack identities are adapted through the established centralized RBAC mechanism and enforce the dismissal permission at the route boundary; do not retain manual authorization checks.Source: Path instructions
719-733: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not report “No cooldown applied” when merge reconciliation fails.
dismiss_recommendationhas already committed the dismissal beforemark_mergedruns. Ifmark_mergedraises, the record can retain its dismissal cooldown, but the card still claims that no cooldown was applied. Make the merge transition retryable/durable and only emit the no-cooldown status after the state change succeeds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/routes/slack/slack_events.py` around lines 719 - 733, Update the already-merged branch in dismiss_recommendation so mark_merged succeeds durably before reporting “No cooldown applied.” Track whether the merge transition committed successfully, preserve the existing exception logging, and emit a failure/retryable status when mark_merged raises instead of claiming no cooldown was applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/services/actions/hpa_vpa_recommendations.py`:
- Around line 84-96: Replace the value parameter annotation in _is_real_number
and _dimension_score from Any to object, preserving their existing runtime
checks and behavior while satisfying ANN401.
- Around line 84-93: Update _is_real_number to handle oversized integer values
without propagating OverflowError: preserve rejection of booleans, non-numeric
values, NaN, and infinities, while treating integers that cannot be safely
evaluated by math.isfinite as invalid. Ensure the scoring flow and any later
float conversions treat these malformed values as unusable rather than aborting.
---
Outside diff comments:
In `@server/routes/slack/slack_events.py`:
- Around line 709-742: Update the failure-status branch around close_result and
the dismissed provider so missing pr_number renders a specific “No PR recorded”
message instead of PR `#None`, while existing PR failures include the number.
Derive the manual-close destination from dismissed.get("vcs_provider")
(defaulting consistently to GitHub) and use the appropriate provider label,
preserving the provider-dispatched lifecycle.
- Around line 635-685: Replace the inline authorization logic in the Slack
dismissal route, including the clicker identity and organization checks around
get_user_id_from_slack_user and set_rls_context, with the centralized RBAC route
authorization using `@require_permission`. Ensure Slack identities are adapted
through the established centralized RBAC mechanism and enforce the dismissal
permission at the route boundary; do not retain manual authorization checks.
- Around line 719-733: Update the already-merged branch in
dismiss_recommendation so mark_merged succeeds durably before reporting “No
cooldown applied.” Track whether the merge transition committed successfully,
preserve the existing exception logging, and emit a failure/retryable status
when mark_merged raises instead of claiming no cooldown was applied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d597e6a7-5711-4746-91c1-2e9e08a4290c
📒 Files selected for processing (5)
server/chat/backend/agent/tools/datadog_tool.pyserver/routes/slack/slack_events.pyserver/services/actions/hpa_vpa_recommendations.pyserver/tests/chat/test_datadog_metric_stats.pyserver/tests/services/test_hpa_vpa_recommendations.py
|
There was a problem hiding this comment.
Looks like chats can now post a slack card. The posting of the slack card should be exclusive to the seeded templated action being ran
| row = _summarize_series( | ||
| meta.get("group_tags"), | ||
| meta.get("unit"), | ||
| values[idx] if idx < len(values) else [], |
There was a problem hiding this comment.
When values is shorter than series, the substituted [] produces points: 0, note: "no non-null points in window", indistinguishable from a genuinely idle workload, which is the exact failure mode this resource type exists to prevent. Fix: Emit a distinct marker (e.g. note: "series metadata present but no values row returned") so the agent can tell a malformed response from real absence.
| dismissed_at TIMESTAMP, | ||
| cooldown_until TIMESTAMP, | ||
| created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, | ||
| updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP |
There was a problem hiding this comment.
cooldown_until is written as aware UTC but compared with NOW(), the 30-day window drifts on a non-UTC session. TIMESTAMPTZ instead?
| if client: | ||
| _send_ephemeral(client, channel_id, slack_user_id, | ||
| "Unauthorized: that recommendation does not belong to your organization.") | ||
| return jsonify({"text": ""}), 200 | ||
|
|
||
| dismissed = dismiss_recommendation(cursor, rec_id, clicker_org_id, slack_user_id) | ||
| # Commit before the GitHub call so a slow API cannot hold the row lock. | ||
| conn.commit() | ||
|
|
||
| if not dismissed: | ||
| logger.info(f"Recommendation {rec_id} was already dismissed; no-op") | ||
| if client: | ||
| _send_ephemeral(client, channel_id, slack_user_id, | ||
| "That recommendation was already dismissed.") | ||
| return jsonify({"text": ""}), 200 |
There was a problem hiding this comment.
The DB transition and PR close are unconditional, but every piece of feedback — ephemerals and _rewrite_dismissed_card — is gated on if client: / an early return when get_slack_client_for_user yields None, so the workload gets a 30-day cooldown and the PR gets closed while the card keeps its live Dismiss button and no message appears.
| status_line = (f"*Status:* Dismissed by <@{slack_user_id}> -- PR #{pr_number} could not be " | ||
| "closed automatically; please close it on GitHub.") |
There was a problem hiding this comment.
status_line says "could not be closed automatically; please close it on GitHub", but the rewrite passes with_actions=False, which drops the entire actions block including the View PR button — so the one message asking for manual work deletes the only link to the PR.
There was a problem hiding this comment.
Invalid UUID, unresolvable org, and unauthenticated-without-workspace-token all return an empty 200 with no ephemeral, so the click is indistinguishable from a broken button
| "Unauthorized: that recommendation does not belong to your organization.") | ||
| return jsonify({"text": ""}), 200 | ||
|
|
||
| dismissed = dismiss_recommendation(cursor, rec_id, clicker_org_id, slack_user_id) |
There was a problem hiding this comment.
maybe there should be a ui confirmation for users if they want to mute it for 30 days or not
| def mark_superseded(cursor, rec_id: str) -> None: | ||
| """Retire a row whose mis-size worsened, or that a newer rec took over.""" | ||
| cursor.execute( | ||
| """UPDATE hpa_vpa_recommendations | ||
| SET status = %s, cooldown_until = NULL, updated_at = NOW() | ||
| WHERE id = %s::uuid""", | ||
| (STATUS_SUPERSEDED, rec_id), | ||
| ) |
There was a problem hiding this comment.
mark_superseded is committed before the Slack post, but the post's compensation path only deletes the new claim, the superseded dismissal is never restored, so the remaining anti-nag window is gone for good.
| if body.get("merged"): | ||
| return {"success": True, "already_merged": True, "state": body.get("state")} | ||
| return {"success": True, "state": body.get("state", "closed")} | ||
| if resp.status_code == 404: |
There was a problem hiding this comment.
i think this should be treated as a error or reqiures more code to tell if its realyl a success. Maybe a use toast saying not reachable
There was a problem hiding this comment.
severity_score is an LLM-supplied argument. Someone clicks Dismiss and gets 30 days of quiet but the AI is allowed to break that silence early if the problem got 25% worse, but the AI is the one reporting how bad it is so this sort of doesnt make sense
3 participants
Adds a scheduled action that compares real Datadog usage against the CPU/memory requests, limits, and HPA
maxReplicasdeclared in a team's IaC, opens one PR per materially mis-sized workload, and posts a Slack card with View PR and Dismiss. Aurora never applies anything — the PR is the change and a human merges it.Seeded
enabled=False, so nothing runs on a schedule until someone turns it on.What's here
metric_statsDatadog resource type — Datadog has no time-percentile capability (every documented route is rejected or silently returns zero series, and both target metrics are gauges), so p95 is computed in Python from rolled-up points, behind a_P95_BY_SOURCEdispatch seam with an import-time guard. Returns one compact row per series rather than a single fat dict:_truncate_resultskeeps a prefix, so today a 973 KB metrics payload becomescount: 0, which is indistinguishable from "no data" and would lead the agent to cut an idle-looking workload. Budgeted at 30 KB againstPASS_THROUGH_CHARS(40 K), notMAX_OUTPUT_SIZE(120 K) — above that an LLM summarizer paraphrases percentiles into plausible fiction.hpa_vpa_recommendationstable + RLS, with a partial unique index enforcing at most one live proposal per workload.close_pull_requestas a provider dispatch (GitHub implemented; GitLab/Bitbucket are a table entry each, not a migration — hence thevcs_providercolumn).send_hpa_vpa_recommendation(first agent-callable Slack write tool in the repo) andlist_hpa_vpa_recommendations, so the prompt checks cooldown state before opening a PR.Design decisions worth review
merged, no cooldown, or the next genuine drift goes unreported.requests.patchinserver/. MCPupdate_pull_requestis allowlisted inmcp_tools.pybut absent fromtool_registry.py, sogate_actiondenies it in background context. Direct REST is the only reliable path.tool_registry.pyentry and nogate_actioncall on the card tool. It writes only to Aurora's own RLS table and the channel Aurora created; a registry key would be a dead switch (_is_org_tool_permittedis only consulted insidegate_action, which this native tool never calls), andgate_actiondenies unconditionally in scheduled runs. The working kill switch isenabled=Falseon the action.resource_type='metrics'behaviour is unchanged. Its output contract predates this work and the RCA prompts depend on it; onlymetric_statsauto-picks an interval. A test pins that boundary.max(not p95) because the failure mode is an OOM-kill; CPU is symmetric because throttling is recoverable.Verification
tests/architectural/unchanged at 67).successruns, and the agent independently builtsum:kubernetes.{cpu.usage.total,memory.usage}{*} by {kube_deployment}— the intendedsum-across-pods shape — then wrote the living document and correctly issued no recommendations rather than inventing any.dismissed_atunchanged; merge clears the cooldown; a duplicate live claim is blocked by the partial unique index./slack/interactions: valid signature → 200, bad signature → 403, stale timestamp → 403, malformed UUID → 200 with a clean message.Two bugs found in review and fixed with regression tests: a NaN metric point silently broke
list.sort()and understatedmax(which guards the OOM rule), and Dismiss closed the PR as the clicker rather than the account that opened it, which would have failed for any org-mate without their own GitHub connection.Summary by CodeRabbit
metric_statssupport for percentile-style summaries (p50/p95/p99/max/mean) with interval auto-selection/clamping.metric_stats/interval guidance and truncation rules.metric_statsand the HPA/VPA recommendation lifecycle.