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:
📝 WalkthroughWalkthroughAdds Mattermost as a new output channel: new orchestrator, output service, schema and tenant config support, HTTP and Kafka handlers, templates, docs, and sample config; wiring and orchestrator activation are conditional on presence of a Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Web as WebHandler
participant Validator as JSONValidator
participant Orch as SendMattermostOrchestrator
participant Jinja as JinjaService
participant Output as MattermostOutputService
Client->>Web: PUT /send_mattermost (JSON)
Web->>Validator: Validate with mattermost_schema
alt valid
Validator-->>Web: Valid
Web->>Orch: send_to_mattermost(msg, tenant)
Orch->>Jinja: render body.template with params
Orch->>Jinja: render nested props (recursively)
Orch->>Output: send(payload, channel_id/username)
Output->>Output: resolve tenant config / call Mattermost API
Output-->>Orch: success or ASABIrisError
Orch-->>Web: result / error
Web-->>Client: 200 OK or mapped error
else invalid
Validator-->>Web: Validation error
Web-->>Client: 400 Bad Request
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@asabiris/app.py`:
- Around line 137-145: The code references non-existent Mattermost modules
(MattermostOutputService, SendMattermostOrchestrator, and mattermost_schema);
either implement those modules or remove all Mattermost usage—prefer removal for
now: delete any imports of asabiris.output.mattermost and
asabiris.orchestration.sendmattermost and any import of mattermost_schema,
remove the conditional block that instantiates
MattermostOutputService/SendMattermostOrchestrator (or replace it with setting
self.MattermostOutputService = None and self.SendMattermostOrchestrator = None
unconditionally), and search for and remove or guard any other references to
MattermostOutputService, SendMattermostOrchestrator, or mattermost_schema in the
codebase (including webhandler-related code) so no ImportError occurs at
runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b37def2-6e5b-456b-93ce-6a24cd1de6f9
📒 Files selected for processing (2)
asabiris/app.pyasabiris/handlers/webhandler.py
| if 'mattermost' in asab.Config.sections(): | ||
| self.MattermostOutputService = MattermostOutputService(self) | ||
| if not self.MattermostOutputService.IsConfigured: | ||
| self.SendMattermostOrchestrator = None | ||
| else: | ||
| self.SendMattermostOrchestrator = SendMattermostOrchestrator(self) | ||
| else: | ||
| self.MattermostOutputService = None | ||
| self.SendMattermostOrchestrator = None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify MattermostOutputService has IsConfigured property
# Search for IsConfigured in mattermost output service
fd -t f "mattermost.py" --exec cat {} \; | grep -n "IsConfigured"
# Also check the class definition
ast-grep --pattern 'class MattermostOutputService($_) {
$$$
}'Repository: TeskaLabs/asab-iris
Length of output: 45
🏁 Script executed:
# Search for MattermostOutputService across all Python files
fd -t f -e py . | head -20Repository: TeskaLabs/asab-iris
Length of output: 665
🏁 Script executed:
# Look for imports and MattermostOutputService references in app.py
rg "MattermostOutputService" -B 2 -A 2Repository: TeskaLabs/asab-iris
Length of output: 931
🏁 Script executed:
# Search for the class definition of MattermostOutputService
ast-grep --pattern 'class MattermostOutputService'Repository: TeskaLabs/asab-iris
Length of output: 45
🏁 Script executed:
# Search for IsConfigured property across the codebase
rg "IsConfigured" -A 2 -B 2Repository: TeskaLabs/asab-iris
Length of output: 356
🏁 Script executed:
# Find the mattermost output service file
fd -t f -path "*/output/mattermost*"Repository: TeskaLabs/asab-iris
Length of output: 231
🏁 Script executed:
# Check the output directory structure
fd -t d -path "*/output" | head -5Repository: TeskaLabs/asab-iris
Length of output: 231
🏁 Script executed:
# List files in output directory
ls -la asabiris/output/ 2>/dev/null || find . -name "output" -type dRepository: TeskaLabs/asab-iris
Length of output: 565
🏁 Script executed:
# Search entire repository for MattermostOutputService definition
rg "class MattermostOutputService" -lRepository: TeskaLabs/asab-iris
Length of output: 45
🏁 Script executed:
# Search for any mattermost references
rg "mattermost" -iRepository: TeskaLabs/asab-iris
Length of output: 1674
🏁 Script executed:
# Check if mattermost module exists anywhere
find . -name "*mattermost*" -type fRepository: TeskaLabs/asab-iris
Length of output: 45
🏁 Script executed:
# Check for mattermost schema
fd -t f "mattermostschema*"Repository: TeskaLabs/asab-iris
Length of output: 45
🏁 Script executed:
# Check orchestration directory
ls -la asabiris/orchestration/Repository: TeskaLabs/asab-iris
Length of output: 615
🏁 Script executed:
# Check schemas directory
ls -la asabiris/schemas/ 2>/dev/null || find . -name "schemas" -type dRepository: TeskaLabs/asab-iris
Length of output: 616
Remove Mattermost service references or implement missing modules.
The code references MattermostOutputService (line 138), SendMattermostOrchestrator (line 141), and mattermost_schema, but their implementations do not exist:
- Missing:
asabiris/output/mattermost/directory and module - Missing:
asabiris/orchestration/sendmattermost.py - Missing:
asabiris/schemas/mattermostschema.py
These imports in app.py and webhandler.py will cause ImportError at runtime. Either implement these modules or remove Mattermost support entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@asabiris/app.py` around lines 137 - 145, The code references non-existent
Mattermost modules (MattermostOutputService, SendMattermostOrchestrator, and
mattermost_schema); either implement those modules or remove all Mattermost
usage—prefer removal for now: delete any imports of asabiris.output.mattermost
and asabiris.orchestration.sendmattermost and any import of mattermost_schema,
remove the conditional block that instantiates
MattermostOutputService/SendMattermostOrchestrator (or replace it with setting
self.MattermostOutputService = None and self.SendMattermostOrchestrator = None
unconditionally), and search for and remove or guard any other references to
MattermostOutputService, SendMattermostOrchestrator, or mattermost_schema in the
codebase (including webhandler-related code) so no ImportError occurs at
runtime.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
asabiris/handlers/kafkahandler.py (1)
138-163:⚠️ Potential issue | 🟠 Major
handle_push()is still unreachable from this dispatcher.
dispatch()never branches onmsg_type == "push", so Kafka push messages still fall into the unsupported-type warning even thoughhandle_push()exists later in the same class. Either add the missing branch here or remove the dead handler if push is not meant to be Kafka-driven.Proposed fix
elif msg_type == "sms": if self.App.SendSMSOrchestrator is None: L.warning("SMS service is not configured. Discarding notification.") return await self.handle_sms(msg) + elif msg_type == "push": + if getattr(self.App, "SendPushOrchestrator", None) is None: + L.warning("Push service is not configured. Discarding notification.") + return + await self.handle_push(msg) else: L.warning( - "Notification sending failed: Unsupported message type '{}'. Supported types are 'email', 'mattermost', 'slack', 'msteams', and 'sms'.".format(msg_type) + "Notification sending failed: Unsupported message type '{}'. Supported types are 'email', 'mattermost', 'slack', 'msteams', 'sms', and 'push'.".format(msg_type) )Also applies to: 293-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asabiris/handlers/kafkahandler.py` around lines 138 - 163, The dispatcher in dispatch() is missing a branch for msg_type == "push", so Kafka push messages fall through to the unsupported-type warning although handle_push() exists; either add an elif branch that mirrors the other services (check self.App.SendPushOrchestrator is not None, warn+return if missing, otherwise await self.handle_push(msg)) or remove the dead handle_push method and any SendPushOrchestrator usage; apply the same fix to the other dispatcher occurrence noted (the second dispatch block referenced in the review) so both places consistently handle "push".
🧹 Nitpick comments (1)
asabiris/handlers/kafkahandler.py (1)
375-385: Reuse the original Mattermost envelope in the fallback path.This branch hardcodes a second payload shape and always emits
channel_id,username, andtenant, even when those keys are absent. Copying the originalmsgand only replacingbodykeeps the error path aligned with the normal Mattermost schema and avoids injectingNonefields.Refactor sketch
try: L.log(asab.LOG_NOTICE, "Sending error notification via Mattermost Orchestrator.") - await self.App.SendMattermostOrchestrator.send_to_mattermost({ - "body": { - "template": tpl_mattermost, - "params": params - }, - "channel_id": msg.get("channel_id"), - "username": msg.get("username"), - "tenant": msg.get("tenant") - }) + mattermost_msg = { + key: value + for key, value in msg.items() + if key != "body" and value is not None + } + mattermost_msg["body"] = { + "template": tpl_mattermost, + "params": params, + } + await self.App.SendMattermostOrchestrator.send_to_mattermost(mattermost_msg) except ASABIrisError as e: L.info("Error notification to Mattermost unsuccessful: Explanation: {}".format(e.TechMessage))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@asabiris/handlers/kafkahandler.py` around lines 375 - 385, The fallback Mattermost notification currently constructs a new payload that always injects channel_id, username, and tenant and can insert None values; instead, reuse the original msg envelope by copying msg (e.g., new_msg = dict(msg) or msg.copy()), set new_msg["body"] = {"template": tpl_mattermost, "params": params}, and pass that to self.App.SendMattermostOrchestrator.send_to_mattermost; keep the L.log call and await semantics and do not add channel_id/username/tenant keys when they were absent in the original msg.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@asabiris/handlers/kafkahandler.py`:
- Around line 138-163: The dispatcher in dispatch() is missing a branch for
msg_type == "push", so Kafka push messages fall through to the unsupported-type
warning although handle_push() exists; either add an elif branch that mirrors
the other services (check self.App.SendPushOrchestrator is not None, warn+return
if missing, otherwise await self.handle_push(msg)) or remove the dead
handle_push method and any SendPushOrchestrator usage; apply the same fix to the
other dispatcher occurrence noted (the second dispatch block referenced in the
review) so both places consistently handle "push".
---
Nitpick comments:
In `@asabiris/handlers/kafkahandler.py`:
- Around line 375-385: The fallback Mattermost notification currently constructs
a new payload that always injects channel_id, username, and tenant and can
insert None values; instead, reuse the original msg envelope by copying msg
(e.g., new_msg = dict(msg) or msg.copy()), set new_msg["body"] = {"template":
tpl_mattermost, "params": params}, and pass that to
self.App.SendMattermostOrchestrator.send_to_mattermost; keep the L.log call and
await semantics and do not add channel_id/username/tenant keys when they were
absent in the original msg.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72bda40a-c56e-49b0-a83b-06c1e2419a37
📒 Files selected for processing (1)
asabiris/handlers/kafkahandler.py
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@asabiris/orchestration/sendmattermost.py`:
- Around line 107-109: The string rendering in sendmattermost.py uses
self.JinjaService.Environment.from_string(...).render(context) directly which
bypasses the template error handling; replace this direct render with the
existing safe path by calling self.JinjaService.format(value, context) (or, if
you must keep from_string, wrap the render call in try/except Exception as e and
re-raise an ASABIrisError carrying the original exception) so that any broken
props expressions are translated into the standard ASABIrisError flow; refer to
JinjaService.Environment.from_string, self.JinjaService.format, and
ASABIrisError when making the change.
In `@asabiris/output/mattermost/service.py`:
- Around line 156-163: The log call in MattermostOutputService.send is
persisting full message content via L.log (asab.LOG_NOTICE) with struct_data
including post_payload; remove or redact post_payload to avoid logging rendered
notification bodies/PII—replace it with safe metadata (e.g., only channel_id,
username, payload_size, or a whitelist of non-sensitive keys) or omit payload
entirely in the struct_data passed to L.log so the rendered message and props
are not retained.
- Around line 79-83: The tenant-specific lookup assumes the ConfigService has a
functional ZooKeeper backend and calls
self.ConfigService.get_mattermost_config(tenant), which raises AttributeError
when ZK is disabled; to fix, only attempt tenant-scoped resolution when
ConfigService exists and its ZK backend is present (e.g., check
getattr(self.ConfigService, "ZK", None) is not None) before calling
get_mattermost_config, otherwise skip to the global config path (or
alternatively expand the except to catch AttributeError alongside KeyError) so
tenant overrides are not attempted when ZooKeeper is disabled; refer to the
tenant variable and get_mattermost_config / self.ConfigService symbols to locate
the change.
- Around line 260-272: The except block handling aiohttp.ClientError around the
POST request does not catch aiohttp timeouts (asyncio.TimeoutError), so wrap
timeout exceptions in the same ASABIrisError: import asyncio if missing and
change the except to catch both aiohttp.ClientError and asyncio.TimeoutError
(e.g., except (aiohttp.ClientError, asyncio.TimeoutError) as e), preserving the
existing ASABIrisError construction and the "from e" chaining so timeouts are
reported with ErrorCode.SERVER_ERROR and the same tech_message/error_dict.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1c5822a-ea88-4f19-a9e2-c5dbda7045b6
📒 Files selected for processing (8)
README.mdasabiris/orchestration/sendmattermost.pyasabiris/output/__init__.pyasabiris/output/mattermost/__init__.pyasabiris/output/mattermost/service.pyasabiris/schemas/__init__.pyasabiris/tenantconfiguration/tenant_config.pyetc/asab-iris.conf
✅ Files skipped from review due to trivial changes (4)
- asabiris/schemas/init.py
- asabiris/output/init.py
- asabiris/output/mattermost/init.py
- etc/asab-iris.conf
| if isinstance(value, str): | ||
| template = self.JinjaService.Environment.from_string(value) | ||
| return template.render(context) |
There was a problem hiding this comment.
Route body.props rendering failures through ASABIrisError too.
The main message on Line 69 goes through JinjaService.format(), but string leaves in props are rendered directly here. A broken props expression will therefore skip the existing template-error translation and surface as a generic failure instead of the normal IRIS error response.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@asabiris/orchestration/sendmattermost.py` around lines 107 - 109, The string
rendering in sendmattermost.py uses
self.JinjaService.Environment.from_string(...).render(context) directly which
bypasses the template error handling; replace this direct render with the
existing safe path by calling self.JinjaService.format(value, context) (or, if
you must keep from_string, wrap the render call in try/except Exception as e and
re-raise an ASABIrisError carrying the original exception) so that any broken
props expressions are translated into the standard ASABIrisError flow; refer to
JinjaService.Environment.from_string, self.JinjaService.format, and
ASABIrisError when making the change.
| if tenant and self.ConfigService is not None: | ||
| try: | ||
| tenant_config = self.ConfigService.get_mattermost_config(tenant) | ||
| except KeyError: | ||
| L.warning("Tenant-specific Mattermost configuration not found for '%s'. Using global config.", tenant) |
There was a problem hiding this comment.
Skip tenant overrides when ZooKeeper is disabled.
asabiris/tenantconfiguration/tenant_config.py explicitly allows a no-ZooKeeper mode on Lines 15-31, but Line 81 assumes the backend is usable as soon as the service exists. In that mode, get_mattermost_config() eventually hits self.ZK.exists(...) and raises AttributeError, so tenant-scoped sends fail instead of falling back to the global [mattermost] settings.
🛡️ Suggested guard
- if tenant and self.ConfigService is not None:
+ if (
+ tenant
+ and self.ConfigService is not None
+ and getattr(self.ConfigService, "ZK", None) is not None
+ and getattr(self.ConfigService, "TenantConfigPath", None)
+ ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@asabiris/output/mattermost/service.py` around lines 79 - 83, The
tenant-specific lookup assumes the ConfigService has a functional ZooKeeper
backend and calls self.ConfigService.get_mattermost_config(tenant), which raises
AttributeError when ZK is disabled; to fix, only attempt tenant-scoped
resolution when ConfigService exists and its ZK backend is present (e.g., check
getattr(self.ConfigService, "ZK", None) is not None) before calling
get_mattermost_config, otherwise skip to the global config path (or
alternatively expand the except to catch AttributeError alongside KeyError) so
tenant overrides are not attempted when ZooKeeper is disabled; refer to the
tenant variable and get_mattermost_config / self.ConfigService symbols to locate
the change.
| L.log( | ||
| asab.LOG_NOTICE, | ||
| "MattermostOutputService.send -> channel_id=%s, username=%s, payload=%r", | ||
| struct_data={ | ||
| "channel_id": channel_id, | ||
| "username": username, | ||
| "payload": post_payload, | ||
| } |
There was a problem hiding this comment.
Do not persist rendered notification bodies in logs.
post_payload contains the full Mattermost message and optional props; the examples in README.md already include usernames and IP addresses. Emitting that object at NOTICE level turns every notification into log-retained message content/PII.
🧹 Suggested redaction
L.log(
asab.LOG_NOTICE,
- "MattermostOutputService.send -> channel_id=%s, username=%s, payload=%r",
- struct_data={
- "channel_id": channel_id,
- "username": username,
- "payload": post_payload,
- }
+ "MattermostOutputService.send -> channel_id=%s, username=%s",
+ channel_id,
+ username,
+ struct_data={
+ "channel_id": channel_id,
+ "username": username,
+ },
)📝 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.
| L.log( | |
| asab.LOG_NOTICE, | |
| "MattermostOutputService.send -> channel_id=%s, username=%s, payload=%r", | |
| struct_data={ | |
| "channel_id": channel_id, | |
| "username": username, | |
| "payload": post_payload, | |
| } | |
| L.log( | |
| asab.LOG_NOTICE, | |
| "MattermostOutputService.send -> channel_id=%s, username=%s", | |
| channel_id, | |
| username, | |
| struct_data={ | |
| "channel_id": channel_id, | |
| "username": username, | |
| }, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@asabiris/output/mattermost/service.py` around lines 156 - 163, The log call
in MattermostOutputService.send is persisting full message content via L.log
(asab.LOG_NOTICE) with struct_data including post_payload; remove or redact
post_payload to avoid logging rendered notification bodies/PII—replace it with
safe metadata (e.g., only channel_id, username, payload_size, or a whitelist of
non-sensitive keys) or omit payload entirely in the struct_data passed to L.log
so the rendered message and props are not retained.
| timeout = aiohttp.ClientTimeout(total=self.Timeout) | ||
|
|
||
| try: | ||
| async with aiohttp.ClientSession(timeout=timeout) as session: | ||
| async with session.post(url, headers=headers, json=payload) as resp: | ||
| body = await resp.text() | ||
| except aiohttp.ClientError as e: | ||
| raise ASABIrisError( | ||
| ErrorCode.SERVER_ERROR, | ||
| tech_message="Mattermost request failed: {}".format(e), | ||
| error_i18n_key="Error occurred while calling Mattermost. Reason: '{{error_message}}'.", | ||
| error_dict={"error_message": str(e)}, | ||
| ) from e |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In aiohttp, does exceeding ClientTimeout(total=...)raiseasyncio.TimeoutErrororaiohttp.ClientError?
💡 Result:
Exceeding ClientTimeout(total=...) in aiohttp raises asyncio.TimeoutError.
Citations:
- 1: aiohttp request cancels the current task when raising a TimeoutError, leads to incompatibility with asyncio.timeout aio-libs/aiohttp#7479
- 2: https://aiohttp.readthedocs.org/en/stable/_modules/aiohttp/client_exceptions.html
- 3: https://aiohttp.readthedocs.org/en/stable/_modules/aiohttp/helpers.html
- 4: https://github.com/aio-libs/aiohttp/blob/7a324fd46ff7dc9bb0bb1bc5afb326e04cf7cef0/aiohttp/client_proto.py
- 5: https://aiohttp.readthedocs.org/en/stable/_modules/aiohttp/client.html
🏁 Script executed:
# Find and read the service.py file around the mentioned lines
find . -type f -name "service.py" -path "*/mattermost/*" | head -5Repository: TeskaLabs/asab-iris
Length of output: 102
🏁 Script executed:
# Search for the mattermost service.py file
fd -t f "service\.py" -p "*mattermost*"Repository: TeskaLabs/asab-iris
Length of output: 162
🏁 Script executed:
# Check the structure first
git ls-files | grep -i mattermost | head -20Repository: TeskaLabs/asab-iris
Length of output: 180
🏁 Script executed:
# Read the service.py file with line numbers, focusing on the relevant section and imports
head -30 ./asabiris/output/mattermost/service.pyRepository: TeskaLabs/asab-iris
Length of output: 883
🏁 Script executed:
# Read lines around 260-272 with context
sed -n '250,280p' ./asabiris/output/mattermost/service.pyRepository: TeskaLabs/asab-iris
Length of output: 1172
🏁 Script executed:
# Check if asyncio is already imported
grep -n "^import asyncio\|^from asyncio" ./asabiris/output/mattermost/service.pyRepository: TeskaLabs/asab-iris
Length of output: 45
🏁 Script executed:
# Search for all exception handlers in the file to understand full context
grep -n "except " ./asabiris/output/mattermost/service.pyRepository: TeskaLabs/asab-iris
Length of output: 265
🏁 Script executed:
# Read more context after line 272 to check if there's a broader exception handler
sed -n '272,310p' ./asabiris/output/mattermost/service.pyRepository: TeskaLabs/asab-iris
Length of output: 1190
Add asyncio.TimeoutError to the exception handler to catch timeout failures.
When ClientTimeout(total=...) expires, aiohttp raises asyncio.TimeoutError, not aiohttp.ClientError. The current handler at line 266 only catches aiohttp.ClientError, so timeout exceptions escape the ASABIrisError wrapper and propagate uncaught.
Fix
+import asyncio
import json
import logging
import configparser
@@
- except aiohttp.ClientError as e:
+ except (aiohttp.ClientError, asyncio.TimeoutError) as e:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@asabiris/output/mattermost/service.py` around lines 260 - 272, The except
block handling aiohttp.ClientError around the POST request does not catch
aiohttp timeouts (asyncio.TimeoutError), so wrap timeout exceptions in the same
ASABIrisError: import asyncio if missing and change the except to catch both
aiohttp.ClientError and asyncio.TimeoutError (e.g., except (aiohttp.ClientError,
asyncio.TimeoutError) as e), preserving the existing ASABIrisError construction
and the "from e" chaining so timeouts are reported with ErrorCode.SERVER_ERROR
and the same tech_message/error_dict.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qa.md`:
- Line 1740: Update the test label string "Access non existant template" to
correct the typo; replace it with "Access non-existent template" (or "Access
nonexistent template") wherever that exact label appears so test names are
consistent and readable.
- Line 1421: Add language identifiers ("json") to the new fenced code blocks in
qa.md to satisfy markdownlint MD040; replace plain fences ``` with ```json for
the request/response JSON examples (e.g., the blocks containing
{"channel_id":"security_channel_id", ...} and {"type":"mattermost",
"channel_id":"security_channel_id", ...}) and apply the same change for the
other occurrences noted (lines referenced in the review). Ensure every JSON
example fence begins with ```json and ends with ``` so the content is properly
highlighted.
|
|
||
| `PUT /send_mattermost` | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add language identifiers to new fenced code blocks.
New fences use plain ``` and trigger markdownlint MD040. Please mark request/response examples as `json`.
Suggested fix
- ```
+ ```json
{
"channel_id":"security_channel_id",
...
}
- ```
+ ```
- ```
+ ```json
{"type":"mattermost", "channel_id":"security_channel_id", ...}
- ```
+ ```Also applies to: 1443-1443, 1464-1464, 1479-1479, 1525-1525, 1542-1542, 1562-1562, 1585-1585, 1603-1603, 1732-1732
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 1421-1421: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qa.md` at line 1421, Add language identifiers ("json") to the new fenced code
blocks in qa.md to satisfy markdownlint MD040; replace plain fences ``` with
```json for the request/response JSON examples (e.g., the blocks containing
{"channel_id":"security_channel_id", ...} and {"type":"mattermost",
"channel_id":"security_channel_id", ...}) and apply the same change for the
other occurrences noted (lines referenced in the review). Ensure every JSON
example fence begins with ```json and ends with ``` so the content is properly
highlighted.
| 'Bad template path' | ||
| {"type":"mattermost", "channel_id":"security_channel_id", "body":{"template":"/Templates/Mattermosts/message.md", "params":{"user.name":"alice" }}} | ||
|
|
||
| 'Access non existant template' |
There was a problem hiding this comment.
Fix typo in test label.
“non existant” should be “non existent” (or “nonexistent”) for consistency/readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qa.md` at line 1740, Update the test label string "Access non existant
template" to correct the typo; replace it with "Access non-existent template"
(or "Access nonexistent template") wherever that exact label appears so test
names are consistent and readable.
1 participant
Summary by CodeRabbit