MPT-23854 Improve error handling - #96
Conversation
📝 WalkthroughWalkthroughChangesError handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@backend/mpt_usage_reporting_extension/services/execution_tracker.py`:
- Line 45: Update both tracker context-manager docstrings and
docs/error-handling.md to document that only escaping Exception subclasses
finalize rows as failed. Add coverage for asyncio.CancelledError and
KeyboardInterrupt in both context-manager test suites, while preserving existing
ordinary-exception and typer.Exit(code=1) behavior.
In `@docs/error-handling.md`:
- Line 75: Update the “Every tracked execution produces exactly one card”
statement in the execution-notification documentation to make the guarantee
conditional on notifications being enabled, while preserving the existing
behavior that disabled notifications send no card.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 4b83b969-cb30-4c42-ab35-d31aeff46eff
📒 Files selected for processing (23)
AGENTS.mdREADME.mdbackend/mpt_usage_reporting_extension/cli/commands/push_estimates_by_id.pybackend/mpt_usage_reporting_extension/exceptions.pybackend/mpt_usage_reporting_extension/mpt_client.pybackend/mpt_usage_reporting_extension/persistence/postgres/auth.pybackend/mpt_usage_reporting_extension/persistence/postgres/connection.pybackend/mpt_usage_reporting_extension/persistence/postgres/database.pybackend/mpt_usage_reporting_extension/persistence/postgres/insights.pybackend/mpt_usage_reporting_extension/services/bucket_delete.pybackend/mpt_usage_reporting_extension/services/charges.pybackend/mpt_usage_reporting_extension/services/execution_tracker.pybackend/mpt_usage_reporting_extension/services/statements.pybackend/tests/cli/commands/test_push_estimates_by_id.pybackend/tests/persistence/postgres/test_auth.pybackend/tests/persistence/postgres/test_connection.pybackend/tests/persistence/postgres/test_database.pybackend/tests/services/test_bucket_delete.pybackend/tests/services/test_charges.pybackend/tests/services/test_statements.pybackend/tests/test_mpt_client.pydocs/architecture.mddocs/error-handling.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
softwareone-platform/mpt-extension-skills(manual)
💤 Files with no reviewable changes (4)
- backend/mpt_usage_reporting_extension/services/statements.py
- backend/mpt_usage_reporting_extension/services/charges.py
- backend/mpt_usage_reporting_extension/cli/commands/push_estimates_by_id.py
- backend/mpt_usage_reporting_extension/services/bucket_delete.py
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: mpt-extension-usage-reporting (Build Build Image)
- GitHub Check: mpt-extension-usage-reporting (Prerequisites Create standard build artifact)
- GitHub Check: mpt-extension-usage-reporting (Prerequisites Set the version)
- GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (10)
README.md
📄 CodeRabbit inference engine (AGENTS.md)
README.md: When applicable, read README.md first for repository purpose, quick start, and the documentation map.
Keep README.md short and navigational; put topic-specific behavior under docs/.
Files:
README.md
**/*
⚙️ CodeRabbit configuration file
**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved
Files:
README.mddocs/architecture.mdbackend/mpt_usage_reporting_extension/persistence/postgres/connection.pybackend/mpt_usage_reporting_extension/persistence/postgres/auth.pybackend/tests/services/test_charges.pyAGENTS.mdbackend/tests/cli/commands/test_push_estimates_by_id.pybackend/mpt_usage_reporting_extension/mpt_client.pydocs/error-handling.mdbackend/tests/services/test_bucket_delete.pybackend/mpt_usage_reporting_extension/services/execution_tracker.pybackend/tests/test_mpt_client.pybackend/mpt_usage_reporting_extension/exceptions.pybackend/tests/services/test_statements.pybackend/tests/persistence/postgres/test_auth.pybackend/tests/persistence/postgres/test_connection.pybackend/mpt_usage_reporting_extension/persistence/postgres/insights.pybackend/tests/persistence/postgres/test_database.pybackend/mpt_usage_reporting_extension/persistence/postgres/database.py
docs/**/*.md
📄 CodeRabbit inference engine (docs/documentation.md)
Topic-specific behavior must live in the matching file under docs/ directory
Follow shared documentation standard for structure and naming in repository documentation
Files:
docs/architecture.mddocs/error-handling.md
docs/*.md
📄 CodeRabbit inference engine (docs/local-development.md)
Do not duplicate the deployment parameter reference outside
docs/deployment.md; link to that document instead.
Files:
docs/architecture.mddocs/error-handling.md
docs/{architecture,local-development,deployment,contributing,testing,error-handling,migrations,documentation}.md
📄 CodeRabbit inference engine (AGENTS.md)
When applicable, read the relevant repository documentation before proceeding, following the prescribed order: architecture, local development, deployment, contributing, testing, error handling, migrations, and documentation.
Files:
docs/architecture.mddocs/error-handling.md
docs/**
⚙️ CodeRabbit configuration file
docs/**: Review documentation changes againstdocs/documentation.mdand the linked repository'sstandards/documentation.md.
Use those documents as the source of truth for structure, topic boundaries, navigation updates, and when to link shared rules instead of copying them.
Files:
docs/architecture.mddocs/error-handling.md
backend/**/*.py
⚙️ CodeRabbit configuration file
backend/**/*.py: Follow the linting rules defined inbackend/pyproject.tomlunder[tool.ruff]and[tool.flake8].
For formatting, use Ruff instead of Black. Do not suggest Black formatting changes.
Review code against the linked repository'sstandards/python-coding.md.
Flag any code artifact not written in English: identifiers, comments, docstrings, log messages, error messages, or test names in any other language must be reported and translated to English.
Flag module-level docstrings in__init__.pyfiles, and redundant module-level docstrings that only restate the module name or path.
Verify modules are organized into cohesive packages instead of flat or grab-bagutils/helpersmodules, and flag inline linter or type-checker ignores (# noqa,# type: ignore) that are not a narrow, justified last resort.
Files:
backend/mpt_usage_reporting_extension/persistence/postgres/connection.pybackend/mpt_usage_reporting_extension/persistence/postgres/auth.pybackend/tests/services/test_charges.pybackend/tests/cli/commands/test_push_estimates_by_id.pybackend/mpt_usage_reporting_extension/mpt_client.pybackend/tests/services/test_bucket_delete.pybackend/mpt_usage_reporting_extension/services/execution_tracker.pybackend/tests/test_mpt_client.pybackend/mpt_usage_reporting_extension/exceptions.pybackend/tests/services/test_statements.pybackend/tests/persistence/postgres/test_auth.pybackend/tests/persistence/postgres/test_connection.pybackend/mpt_usage_reporting_extension/persistence/postgres/insights.pybackend/tests/persistence/postgres/test_database.pybackend/mpt_usage_reporting_extension/persistence/postgres/database.py
backend/**
⚙️ CodeRabbit configuration file
backend/**: Review backend changes againstAGENTS.md,docs/architecture.md,docs/contributing.md, anddocs/testing.md.
Use relevant linked shared standards and operational guidance when those local documents reference them.
If the change adds or alters behaviour, components, configuration, or commands, verify the corresponding documentation (docs/*,README.md,AGENTS.md) is updated perstandards/documentation.md.
Files:
backend/mpt_usage_reporting_extension/persistence/postgres/connection.pybackend/mpt_usage_reporting_extension/persistence/postgres/auth.pybackend/tests/services/test_charges.pybackend/tests/cli/commands/test_push_estimates_by_id.pybackend/mpt_usage_reporting_extension/mpt_client.pybackend/tests/services/test_bucket_delete.pybackend/mpt_usage_reporting_extension/services/execution_tracker.pybackend/tests/test_mpt_client.pybackend/mpt_usage_reporting_extension/exceptions.pybackend/tests/services/test_statements.pybackend/tests/persistence/postgres/test_auth.pybackend/tests/persistence/postgres/test_connection.pybackend/mpt_usage_reporting_extension/persistence/postgres/insights.pybackend/tests/persistence/postgres/test_database.pybackend/mpt_usage_reporting_extension/persistence/postgres/database.py
backend/tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Inspect the backend test suite when the task concerns backend code or tests.
Files:
backend/tests/services/test_charges.pybackend/tests/cli/commands/test_push_estimates_by_id.pybackend/tests/services/test_bucket_delete.pybackend/tests/test_mpt_client.pybackend/tests/services/test_statements.pybackend/tests/persistence/postgres/test_auth.pybackend/tests/persistence/postgres/test_connection.pybackend/tests/persistence/postgres/test_database.py
⚙️ CodeRabbit configuration file
backend/tests/**: Review backend test changes againstdocs/testing.mdand the linked repository'sstandards/unittests.md.
Verify that repository-specific test behavior and shared unit-test rules are followed.
Verify tests are written as functions (not classes), are grouped into packages mirroring the source, share setup through fixtures (splitting a large conftest into a fixtures package registered viapytest_plugins), keep fixture dependency depth at 3 or fewer levels, and usefreezeguninstead of patchingdatetime.
Files:
backend/tests/services/test_charges.pybackend/tests/cli/commands/test_push_estimates_by_id.pybackend/tests/services/test_bucket_delete.pybackend/tests/test_mpt_client.pybackend/tests/services/test_statements.pybackend/tests/persistence/postgres/test_auth.pybackend/tests/persistence/postgres/test_connection.pybackend/tests/persistence/postgres/test_database.py
AGENTS.md
📄 CodeRabbit inference engine (AGENTS.md)
AGENTS.md: For every task, identify the task type and select only the local repository files relevant to that task.
Read only the selected relevant local files before making changes.
If selected local files reference relevant shared standards or operational guidance, read those shared documents before proceeding.
Treat repository-local documents as repository-specific additions, restrictions, or overrides to shared guidance.
When repository-local rules conflict with shared rules, the repository-local rule takes precedence.
Files:
AGENTS.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: softwareone-platform/mpt-usage-reporting-extension
Timestamp: 2026-07-31T13:41:21.212Z
Learning: Prefer documented make targets over ad hoc Docker commands.
Learnt from: CR
Repo: softwareone-platform/mpt-usage-reporting-extension
Timestamp: 2026-07-31T13:41:21.212Z
Learning: Treat Docker as the default local execution model for the repository.
Learnt from: CR
Repo: softwareone-platform/mpt-usage-reporting-extension
Timestamp: 2026-07-31T13:41:21.212Z
Learning: For shared meaning of common make targets and validation flow, prefer shared knowledge documents instead of inferring semantics from target names alone.
Learnt from: CR
Repo: softwareone-platform/mpt-usage-reporting-extension
Timestamp: 2026-07-31T13:41:33.066Z
Learning: Keep the extension app bare: `app.py` should instantiate `ExtensionApp` without registering custom event, API, or plug routes.
Learnt from: CR
Repo: softwareone-platform/mpt-usage-reporting-extension
Timestamp: 2026-07-31T13:41:42.460Z
Learning: Do not add automatic retries. Estimate uploads may be re-pushed because they use absolute `PUT`s; do not re-run a failed `run` for the same window because additive accumulation can double-count—use `recalculate` instead.
🪛 LanguageTool
AGENTS.md
[style] ~19-~19: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....md) before changing code or tests. 7. [docs/error-handling.md](docs/error-handling....
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~20-~20: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ons, retries, or failure reporting. 8. docs/migrations.md when...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...mentions schema or data migrations. 9. [docs/documentation.md](docs/documentation.md...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/error-handling.md
[locale-violation] ~81-~81: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...tacktrace; the exception is re-raised afterwards, so the process still exits non-zero. -...
(AFTERWARDS_US)
🔇 Additional comments (18)
AGENTS.md (1)
19-21: LGTM!README.md (1)
45-45: LGTM!docs/architecture.md (1)
132-132: LGTM!backend/mpt_usage_reporting_extension/mpt_client.py (1)
5-6: LGTM!Also applies to: 17-17
backend/mpt_usage_reporting_extension/persistence/postgres/insights.py (1)
8-8: LGTM!Also applies to: 38-38
backend/tests/test_mpt_client.py (1)
4-4: LGTM!Also applies to: 23-23
backend/tests/services/test_bucket_delete.py (1)
128-133: LGTM!backend/tests/services/test_charges.py (1)
135-141: LGTM!backend/tests/services/test_statements.py (1)
123-123: LGTM!Also applies to: 135-135
backend/mpt_usage_reporting_extension/exceptions.py (1)
1-9: 🗄️ Data Integrity & IntegrationComment is incorrect; no compatibility issue exists.
Production code catches exceptions using either
except Exception(pipeline.py, execution_tracker.py, estimates_uploader.py) or specific custom types (e.g.,MPTError,UpstreamStatementError). No production code catchesRuntimeError, so changing the inheritance ofConfigurationErrorandUpstreamAPIErrorto inherit fromExtensionErrordoes not break any existing callers. The new hierarchy is documented indocs/error-handling.mdand fully compatible with existing exception handlers.> Likely an incorrect or invalid review comment.backend/mpt_usage_reporting_extension/persistence/postgres/auth.py (1)
9-9: LGTM!Also applies to: 75-77
backend/mpt_usage_reporting_extension/persistence/postgres/connection.py (1)
5-6: LGTM!Also applies to: 44-44
backend/mpt_usage_reporting_extension/persistence/postgres/database.py (2)
7-7: LGTM!
25-27: 🩺 Stability & AvailabilityThe exception contract is consistent.
ConfigurationErrorderives fromExtensionError. All database callers use the new exception types, and production code has noRuntimeErrordependency. The remainingRuntimeErrortests cover unrelated injected failures.backend/tests/persistence/postgres/test_auth.py (1)
5-5: LGTM!Also applies to: 87-93
backend/tests/persistence/postgres/test_connection.py (1)
3-3: LGTM!Also applies to: 51-51
backend/tests/persistence/postgres/test_database.py (1)
7-7: LGTM!Also applies to: 42-50, 140-140, 154-154, 166-166
backend/tests/cli/commands/test_push_estimates_by_id.py (1)
128-133: LGTM!
| try: | ||
| yield execution | ||
| except BaseException as exc: # noqa: WPS424 (includes typer.Exit) | ||
| except Exception as exc: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 'CancelledError|KeyboardInterrupt|typer\.Exit' backend/testsRepository: softwareone-platform/mpt-usage-reporting-extension
Length of output: 2631
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- execution_tracker.py outline ---'
ast-grep outline backend/mpt_usage_reporting_extension/services/execution_tracker.py || true
printf '%s\n' '--- execution_tracker.py ---'
cat -n backend/mpt_usage_reporting_extension/services/execution_tracker.py
printf '%s\n' '--- focused tests and usages ---'
rg -n -C 6 'ExecutionTracker|execution_tracker|\.track\(|\.record\(' backend/tests backend/mpt_usage_reporting_extension docs README.md AGENTS.md 2>/dev/null || true
printf '%s\n' '--- exception guidance ---'
if [ -f docs/error-handling.md ]; then
cat -n docs/error-handling.md
fi
if [ -f AGENTS.md ]; then
cat -n AGENTS.md
fi
printf '%s\n' '--- tracked test files ---'
git ls-files 'backend/tests/*execution*' 'backend/tests/*tracker*' 'backend/tests/*pipeline*'Repository: softwareone-platform/mpt-usage-reporting-extension
Length of output: 36014
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- focused tracker tests ---'
cat -n backend/tests/services/test_execution_tracker.py
printf '%s\n' '--- pipeline tracking implementation ---'
sed -n '250,325p' backend/mpt_usage_reporting_extension/pipeline.py
printf '%s\n' '--- pipeline exception and typer.Exit tests ---'
rg -n -C 12 'def _tracked|typer\.Exit|notify_failure|CancelledError|KeyboardInterrupt|Exception' backend/mpt_usage_reporting_extension/pipeline.py backend/tests/test_pipeline.py
printf '%s\n' '--- current change summary ---'
git status --short
git diff --stat
git diff -- backend/mpt_usage_reporting_extension/services/execution_tracker.py backend/tests/services/test_execution_tracker.py docs/error-handling.mdRepository: softwareone-platform/mpt-usage-reporting-extension
Length of output: 18338
🏁 Script executed on selected repositories:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- dependency declarations ---'
rg -n -C 3 '^(name = "typer"|name = "click"|typer|click)' backend/pyproject.toml backend/uv.lock 2>/dev/null || true
printf '%s\n' '--- local documentation guidance ---'
for file in docs/documentation.md docs/testing.md docs/contributing.md standards/documentation.md; do
if [ -f "$file" ]; then
printf '\n--- %s ---\n' "$file"
cat -n "$file"
fi
done
printf '%s\n' '--- shared error-handling guidance ---'
rg -n -C 8 'CancelledError|KeyboardInterrupt|BaseException|typer\.Exit|cancellation|termination' standards docs backend 2>/dev/null || true
printf '%s\n' '--- related repository standards ---'
rg -n -C 8 'CancelledError|KeyboardInterrupt|BaseException|typer\.Exit|cancellation|termination' . 2>/dev/null || true
printf '%s\n' '--- recent commits ---'
git log --oneline -8
Repositories: softwareone-platform/mpt-usage-reporting-extension, softwareone-platform/mpt-extension-skills
Length of output: 57064
🏁 Script executed (no clone):
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import asyncio
import ast
print("asyncio.CancelledError MRO:", asyncio.CancelledError.__mro__)
print("KeyboardInterrupt MRO:", KeyboardInterrupt.__mro__)
try:
import typer
except ImportError as exc:
print("typer unavailable:", exc)
else:
print("typer.Exit MRO:", typer.Exit.__mro__)
`@__import__`("contextlib").asynccontextmanager
async def exception_only_tracker(events):
events.append("start")
try:
yield
except Exception as exc:
events.append(("finish", type(exc).__name__))
raise
else:
events.append("success")
async def probe(exc):
events = []
try:
async with exception_only_tracker(events):
raise exc
except BaseException as caught:
return type(caught).__name__, events
async def main():
for exc in (ValueError("ordinary"), asyncio.CancelledError(), KeyboardInterrupt()):
print(type(exc).__name__, await probe(exc))
asyncio.run(main())
PY
Length of output: 540
🏁 Script executed (no clone):
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import asyncio
import ast
from contextlib import asynccontextmanager
print("asyncio.CancelledError MRO:", asyncio.CancelledError.__mro__)
print("KeyboardInterrupt MRO:", KeyboardInterrupt.__mro__)
try:
import typer
except ImportError as exc:
print("typer unavailable:", exc)
else:
print("typer.Exit MRO:", typer.Exit.__mro__)
`@asynccontextmanager`
async def exception_only_tracker(events):
events.append("start")
try:
yield
except Exception as exc:
events.append(("finish", type(exc).__name__))
raise
else:
events.append("success")
async def probe(exc):
events = []
try:
async with exception_only_tracker(events):
raise exc
except BaseException as caught:
return type(caught).__name__, events
async def main():
for exc in (ValueError("ordinary"), asyncio.CancelledError(), KeyboardInterrupt()):
print(type(exc).__name__, await probe(exc))
asyncio.run(main())
PY
Length of output: 540
Document the Exception boundary and cover termination signals.
Update both tracker docstrings and docs/error-handling.md to state that only escaping Exception subclasses finalize rows as failed. Add tests for asyncio.CancelledError and KeyboardInterrupt in both context managers. Existing tests cover ordinary exceptions and typer.Exit(code=1).
🤖 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 `@backend/mpt_usage_reporting_extension/services/execution_tracker.py` at line
45, Update both tracker context-manager docstrings and docs/error-handling.md to
document that only escaping Exception subclasses finalize rows as failed. Add
coverage for asyncio.CancelledError and KeyboardInterrupt in both
context-manager test suites, while preserving existing ordinary-exception and
typer.Exit(code=1) behavior.
Sources: Coding guidelines, Path instructions
Root all package exceptions in a single ExtensionError base and raise ConfigurationError for missing or invalid configuration instead of bare RuntimeError. Narrow the execution-tracker catches from BaseException to Exception, and drop the duplicate warning logs at the MPT API translation sites so each failure is reported once, at the boundary that handles it. Document the exception hierarchy, runtime error flow, and Teams notification triggers in docs/error-handling.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9046c46 to
40053f4
Compare
albertsola
changed the title
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/tests/services/test_execution_tracker.py (1)
50-55: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the original
typer.Exitcode.
pytest.raises(typer.Exit)checks only the exception type. Capture the exception and assert that its exit code remains1. This protects the re-raise behavior from a future replacement with a default-code exit.🤖 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 `@backend/tests/services/test_execution_tracker.py` around lines 50 - 55, Update test_track_finishes_failed_on_typer_exit to capture the raised typer.Exit exception with pytest.raises and assert its exit code is 1, while preserving the existing execution-status assertion.
🤖 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 `@docs/error-handling.md`:
- Around line 84-86: Update ExecutionNotifier.notify_failure to sanitize error
and stacktrace details before constructing the Adaptive Card and calling
send_card, removing secrets, customer identifiers, SQL, tokens, and local paths
while preserving useful failure context. Do not send raw exception diagnostics
to Teams.
---
Nitpick comments:
In `@backend/tests/services/test_execution_tracker.py`:
- Around line 50-55: Update test_track_finishes_failed_on_typer_exit to capture
the raised typer.Exit exception with pytest.raises and assert its exit code is
1, while preserving the existing execution-status assertion.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 719f9b25-0eef-4088-b2ba-f7677cbfe4b3
📒 Files selected for processing (24)
AGENTS.mdREADME.mdbackend/mpt_usage_reporting_extension/cli/commands/push_estimates_by_id.pybackend/mpt_usage_reporting_extension/exceptions.pybackend/mpt_usage_reporting_extension/mpt_client.pybackend/mpt_usage_reporting_extension/persistence/postgres/auth.pybackend/mpt_usage_reporting_extension/persistence/postgres/connection.pybackend/mpt_usage_reporting_extension/persistence/postgres/database.pybackend/mpt_usage_reporting_extension/persistence/postgres/insights.pybackend/mpt_usage_reporting_extension/services/bucket_delete.pybackend/mpt_usage_reporting_extension/services/charges.pybackend/mpt_usage_reporting_extension/services/execution_tracker.pybackend/mpt_usage_reporting_extension/services/statements.pybackend/tests/cli/commands/test_push_estimates_by_id.pybackend/tests/persistence/postgres/test_auth.pybackend/tests/persistence/postgres/test_connection.pybackend/tests/persistence/postgres/test_database.pybackend/tests/services/test_bucket_delete.pybackend/tests/services/test_charges.pybackend/tests/services/test_execution_tracker.pybackend/tests/services/test_statements.pybackend/tests/test_mpt_client.pydocs/architecture.mddocs/error-handling.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
softwareone-platform/mpt-extension-skills(manual)
💤 Files with no reviewable changes (4)
- backend/mpt_usage_reporting_extension/services/statements.py
- backend/mpt_usage_reporting_extension/cli/commands/push_estimates_by_id.py
- backend/mpt_usage_reporting_extension/services/bucket_delete.py
- backend/mpt_usage_reporting_extension/services/charges.py
🚧 Files skipped from review as they are similar to previous changes (17)
- backend/mpt_usage_reporting_extension/persistence/postgres/connection.py
- backend/mpt_usage_reporting_extension/exceptions.py
- backend/mpt_usage_reporting_extension/persistence/postgres/insights.py
- backend/tests/persistence/postgres/test_connection.py
- backend/tests/services/test_charges.py
- backend/tests/cli/commands/test_push_estimates_by_id.py
- backend/tests/services/test_bucket_delete.py
- backend/mpt_usage_reporting_extension/persistence/postgres/auth.py
- backend/tests/services/test_statements.py
- backend/mpt_usage_reporting_extension/persistence/postgres/database.py
- backend/tests/test_mpt_client.py
- backend/mpt_usage_reporting_extension/services/execution_tracker.py
- backend/mpt_usage_reporting_extension/mpt_client.py
- README.md
- backend/tests/persistence/postgres/test_database.py
- backend/tests/persistence/postgres/test_auth.py
- docs/architecture.md
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: mpt-extension-usage-reporting (Build Build Image)
- GitHub Check: mpt-extension-usage-reporting (Prerequisites Create standard build artifact)
- GitHub Check: mpt-extension-usage-reporting (Prerequisites Set the version)
- GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (7)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: For each task, identify its type and select only the local repository files relevant to that task before making changes.
Read selected local files before making changes, and read any referenced shared standards or operational guidance relevant to the task.
Treat repository-local documents as additions, restrictions, or overrides to shared guidance; local rules take precedence when they conflict with shared rules.
When applicable, read repository documentation in the prescribed order, beginning with README.md and then the relevant architecture, development, deployment, contributing, testing, error-handling, migration, or documentation guides.
Inspect code paths relevant to the task, including the backend application entry point, backend configuration, migrations, tests, make commands, container files, and CI workflow.
Prefer documented make targets over ad hoc Docker commands.
Treat Docker as the default local execution model for the repository.
For shared meaning of common make targets and validation flow, prefer shared knowledge documents instead of inferring local semantics from target names alone.
Files:
backend/tests/services/test_execution_tracker.pyAGENTS.mddocs/error-handling.md
⚙️ CodeRabbit configuration file
**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved
Files:
backend/tests/services/test_execution_tracker.pyAGENTS.mddocs/error-handling.md
backend/**/*.py
⚙️ CodeRabbit configuration file
backend/**/*.py: Follow the linting rules defined inbackend/pyproject.tomlunder[tool.ruff]and[tool.flake8].
For formatting, use Ruff instead of Black. Do not suggest Black formatting changes.
Review code against the linked repository'sstandards/python-coding.md.
Flag any code artifact not written in English: identifiers, comments, docstrings, log messages, error messages, or test names in any other language must be reported and translated to English.
Flag module-level docstrings in__init__.pyfiles, and redundant module-level docstrings that only restate the module name or path.
Verify modules are organized into cohesive packages instead of flat or grab-bagutils/helpersmodules, and flag inline linter or type-checker ignores (# noqa,# type: ignore) that are not a narrow, justified last resort.
Files:
backend/tests/services/test_execution_tracker.py
backend/**
⚙️ CodeRabbit configuration file
backend/**: Review backend changes againstAGENTS.md,docs/architecture.md,docs/contributing.md, anddocs/testing.md.
Use relevant linked shared standards and operational guidance when those local documents reference them.
If the change adds or alters behaviour, components, configuration, or commands, verify the corresponding documentation (docs/*,README.md,AGENTS.md) is updated perstandards/documentation.md.
Files:
backend/tests/services/test_execution_tracker.py
backend/tests/**
⚙️ CodeRabbit configuration file
backend/tests/**: Review backend test changes againstdocs/testing.mdand the linked repository'sstandards/unittests.md.
Verify that repository-specific test behavior and shared unit-test rules are followed.
Verify tests are written as functions (not classes), are grouped into packages mirroring the source, share setup through fixtures (splitting a large conftest into a fixtures package registered viapytest_plugins), keep fixture dependency depth at 3 or fewer levels, and usefreezeguninstead of patchingdatetime.
Files:
backend/tests/services/test_execution_tracker.py
docs/**/*.md
📄 CodeRabbit inference engine (docs/documentation.md)
Topic-specific behavior must live in the matching file under docs/ directory
Follow shared documentation standard for structure and naming in repository documentation
Files:
docs/error-handling.md
docs/*.md
📄 CodeRabbit inference engine (docs/local-development.md)
Do not duplicate the deployment parameter reference outside
docs/deployment.md; link to that document instead.
Files:
docs/error-handling.md
docs/**
⚙️ CodeRabbit configuration file
docs/**: Review documentation changes againstdocs/documentation.mdand the linked repository'sstandards/documentation.md.
Use those documents as the source of truth for structure, topic boundaries, navigation updates, and when to link shared rules instead of copying them.
Files:
docs/error-handling.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: softwareone-platform/mpt-usage-reporting-extension
Timestamp: 2026-07-31T16:15:18.910Z
Learning: Keep the product as a CLI batch job that selects billing statements, accumulates usage by subscription/agreement and month, persists totals to PostgreSQL, and pushes price estimates back to subscriptions.
Learnt from: CR
Repo: softwareone-platform/mpt-usage-reporting-extension
Timestamp: 2026-07-31T16:15:30.564Z
Learning: After a failed `run`, do not re-run the same window because additive accumulation upserts can double-count; use `recalculate`, which deletes the scope's buckets before refilling them.
🪛 LanguageTool
AGENTS.md
[style] ~19-~19: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ....md) before changing code or tests. 7. [docs/error-handling.md](docs/error-handling....
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~20-~20: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ons, retries, or failure reporting. 8. docs/migrations.md when...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...mentions schema or data migrations. 9. [docs/documentation.md](docs/documentation.md...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/error-handling.md
[grammar] ~68-~68: Ensure spelling is correct
Context: .... Only escaping Exception subclasses finalise rows as failed. BaseExceptions such a...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[locale-violation] ~86-~86: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...tacktrace; the exception is re-raised afterwards, so the process still exits non-zero. -...
(AFTERWARDS_US)
🔇 Additional comments (5)
AGENTS.md (1)
19-21: LGTM!docs/error-handling.md (1)
1-83: LGTM!Also applies to: 87-116
backend/tests/services/test_execution_tracker.py (3)
1-4: LGTM!
58-66: LGTM!
82-88: LGTM!
| - **Failure card (💣), unhandled exception** — an exception escaped the | ||
| command body. Includes the error message and the full stacktrace; the | ||
| exception is re-raised afterwards, so the process still exits non-zero. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'stacktrace|traceback|ExecutionNotifier|webhook|MPT_TEAMS|logger\.exception' \
backend/mpt_usage_reporting_extension/services/execution_notifier.py \
backend/mpt_usage_reporting_extension/pipeline.pyRepository: softwareone-platform/mpt-usage-reporting-extension
Length of output: 9982
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- notifier implementation ---'
cat -n backend/mpt_usage_reporting_extension/services/execution_notifier.py
printf '%s\n' '--- pipeline failure path ---'
sed -n '240,290p' backend/mpt_usage_reporting_extension/pipeline.py
printf '%s\n' '--- repository references to notifier and redaction ---'
rg -n -i -C 3 'AsyncTeamsNotifications|notify_failure|redact|sanitize|stacktrace|traceback|webhook' .
printf '%s\n' '--- dependency declarations ---'
rg -n -C 3 'mpt-extension-sdk|teams|notification' pyproject.toml poetry.lock requirements*.txt uv.lock 2>/dev/null || trueRepository: softwareone-platform/mpt-usage-reporting-extension
Length of output: 36208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -i -C 4 'AsyncTeamsNotifications|redact|sanitize|stacktrace|traceback|webhook|send_card' .Repository: softwareone-platform/mpt-extension-skills
Length of output: 50399
🌐 Web query:
"mpt-extension-contrib-custom-notifications" "AsyncTeamsNotifications"
💡 Result:
AsyncTeamsNotifications is a component provided by the mpt-extension-contrib-custom-notifications library, which facilitates sending notifications from SoftwareONE MPT (Managed Platform Tooling) extensions to Microsoft Teams [1]. Key details regarding AsyncTeamsNotifications include: Purpose: It is designed to send Adaptive Cards to a Microsoft Teams channel asynchronously using httpx.AsyncClient [1]. This approach ensures that notification operations do not block the application's event loop [1]. Location: It is located within the mpt_extension_contrib.custom_notifications.channels.teams_async module [1]. Usage: It acts as an asynchronous counterpart to the synchronous TeamsNotifications class [1]. To implement or resolve it, developers typically work with the AsyncTeamsNotifier base class, which is used to resolve the appropriate notification handler via the notification registry (e.g., ctx.notifications.get(AsyncTeamsNotifier)) [1]. The mpt-extension-contrib-custom-notifications library provides a unified registry (ctx.notifications) that allows MPT extensions to report to various notification channels without needing to re-implement sender logic for each extension [1][2].
Citations:
- 1: https://pypi.org/project/mpt-extension-contrib-custom-notifications/1.2.0/
- 2: https://pypi.org/project/mpt-extension-contrib-custom-notifications/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- tracked dependency files ---'
git ls-files | rg '(^|/)(pyproject\.toml|poetry\.lock|uv\.lock|requirements[^/]*|setup\.cfg|setup\.py|Pipfile)$' || true
printf '%s\n' '--- package metadata references ---'
rg -n -i -C 3 'mpt-extension-contrib-custom-notifications|custom-notifications|mpt-extension-contrib' --glob '!docs/**' --glob '!backend/tests/**' .
printf '%s\n' '--- notifier tests around raw content ---'
sed -n '60,105p' backend/tests/services/test_execution_notifier.pyRepository: softwareone-platform/mpt-usage-reporting-extension
Length of output: 4208
Redact failure details before sending them to Teams.
ExecutionNotifier.notify_failure inserts raw error and stacktrace values into the Adaptive Card. Redact secrets, customer identifiers, SQL, tokens, and local paths before send_card, or document that Teams receives unsanitized diagnostics.
🧰 Tools
🪛 LanguageTool
[locale-violation] ~86-~86: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...tacktrace; the exception is re-raised afterwards, so the process still exits non-zero. -...
(AFTERWARDS_US)
🤖 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 `@docs/error-handling.md` around lines 84 - 86, Update
ExecutionNotifier.notify_failure to sanitize error and stacktrace details before
constructing the Adaptive Card and calling send_card, removing secrets, customer
identifiers, SQL, tokens, and local paths while preserving useful failure
context. Do not send raw exception diagnostics to Teams.
1 participant
🤖 AI-generated PR — Please review carefully.
What was done
Improved and documented the backend's error handling:
ExtensionErrorbase. AddedConfigurationErrorfor missing or invalid configuration and re-rootedUpstreamAPIErrorunder the base.RuntimeError— replaced the six bareRuntimeErrorraises: MPT client env vars (mpt_client.py),MPT_DATABASE_URLresolution and the not-open guard (persistence/postgres/database.py), unsupported DSN parameters (persistence/postgres/connection.py), non-TLSsslmodewith Entra ID auth (persistence/postgres/auth.py), and the impossible-row guard (persistence/postgres/insights.py).ExecutionTrackerandStatementProcessingRecordernow catchExceptioninstead ofBaseException;typer.Exitis still caught (it subclassesRuntimeError).KeyboardInterrupt/asyncio.CancelledErrorpropagate without finalising the insight row, which is documented and covered by tests.logger.warningcalls at the four MPT API error-translation sites (services/charges.py,services/statements.py,services/bucket_delete.py,cli/commands/push_estimates_by_id.py); the boundary that handles the failure owns the single report.docs/error-handling.mddescribing the exception hierarchy, the runtime error flow across the recording/notification layers, when Teams notifications trigger, and the timeout/retry posture; linked it fromREADME.md,AGENTS.md, anddocs/architecture.md.No business behavior changes.
Testing
make teston all affected test files: 136 passed (includes newtyper.Exit,KeyboardInterrupt, andasyncio.CancelledErrorcoverage for both tracker context managers).make check(ruff, flake8, mypy): clean.Closes MPT-23854
ExtensionErrorandConfigurationErrorto define the package exception hierarchy.RuntimeErrorraises with domain-specific exceptions.BaseExceptiontoExceptionwhile preservingtyper.Exithandling.