feat: sentry DSN wiring + metrics shim audit (dead set() removed, 11 new tests)#58
Merged
Merged
Conversation
|
🤖 OpenBot received this PR and review is queued. v0.1 alpha: only the ACK is automated so far. Full review (diff scan, structured findings, severity filter) lands in an upcoming commit. |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughIntroduces domain enums and metrics, expands ports and GitHub adapter, adds ingest_webhook orchestration, refactors the webhook route to use a dependency and use case, enhances observability (Sentry logs/profiling, JSON logging, LangSmith), adds Docker/dev tooling, DB init CLI, and updates tests/docs accordingly. ChangesWebhook ingestion and domain/ports reshaping
Sequence Diagram(s)sequenceDiagram
participant Client as GitHub
participant API as FastAPI App
participant Dep as verified_github_event
participant UseCase as ingest_webhook
participant Adapter as GitHubAdapter
participant Queue as Redis Stream + Worker
Client->>API: POST /webhook/github
API->>Dep: Read body + headers
Dep->>Adapter: verify_signature + parse_event
Dep-->>API: UnifiedEvent
API->>UseCase: event + ports (dedup/runs/lock/cancel/queue)
UseCase->>Adapter: optional reads (labels/comments/file) / check-run update
UseCase->>Queue: enqueue(event, feature, task_id,…)
Queue-->>UseCase: entry_id
UseCase-->>API: accepted/ignored/duplicate + ids
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
268-270:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove accidental trailing text from README.
Line 268–Line 270 look like leftover scratch text and make the documentation appear broken to readers.
Proposed fix
-# E2E Test -# Fixed -Final Test🤖 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 `@README.md` around lines 268 - 270, Remove the accidental trailing scratch text at the end of README by deleting the stray lines containing "E2E Test", "Fixed", and "Final Test" so the file ends cleanly; ensure no leftover headings or placeholder text remain and that the README content ends with the intended final section or newline.
🧹 Nitpick comments (2)
obser.txt (1)
1-1: ⚡ Quick winRemove stray artifact file before merge.
Line 1 appears to be a temporary test artifact and adds noise to the repo history.
🤖 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 `@obser.txt` at line 1, Remove the stray temporary artifact by deleting the file obser.txt (or removing its single-line content "obser test") before merging; ensure no other commits reintroduce this test artifact and update the branch so the repository history no longer contains this noisy temporary file.tests/application/use_cases/test_triage.py (1)
116-117: ⚡ Quick winAlign remaining
caploglogger names withuse_cases.triage.Three tests still target
openbot.application.workflows.triage; after the move, this makes log-level scoping brittle and inconsistent.Suggested patch
- with caplog.at_level(logging.WARNING, logger="openbot.application.workflows.triage"): + with caplog.at_level(logging.WARNING, logger="openbot.application.use_cases.triage"): @@ - with caplog.at_level(logging.WARNING, logger="openbot.application.workflows.triage"): + with caplog.at_level(logging.WARNING, logger="openbot.application.use_cases.triage"): @@ - with caplog.at_level(logging.ERROR, logger="openbot.application.workflows.triage"): + with caplog.at_level(logging.ERROR, logger="openbot.application.use_cases.triage"):Also applies to: 126-127, 177-178
🤖 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 `@tests/application/use_cases/test_triage.py` around lines 116 - 117, Update the caplog logger names used when setting log level in the triage tests: replace occurrences of "openbot.application.workflows.triage" with the new module path "openbot.application.use_cases.triage" where caplog.at_level is used (e.g., the test blocks that call caplog.at_level(logging.WARNING, logger="openbot.application.workflows.triage") around maybe_run_triage(_ctx(_event(...))) and similar calls); ensure all three remaining instances (the blocks wrapping maybe_run_triage and related assertions) use "openbot.application.use_cases.triage" so log-level scoping matches the moved module.
🤖 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 @.env.example:
- Line 26: The commit modifies the `.env.example` entry for
CLAUDE_SWITCH_GLM_BASE_URL which violates the rule disallowing committed .env*
files; either revert the change to keep `.env.example` untouched or move the
template line for CLAUDE_SWITCH_GLM_BASE_URL into a non-.env filename (e.g.,
rename to env.example.template or config/claude-proxy.template) and update
references accordingly, and if you truly intend to commit this template, update
the policy/ignore rules to explicitly allow `env.example.template` (not
`.env*`), ensuring no `.env*` file is added to the repo or commit history.
In `@Dockerfile`:
- Around line 1-14: The Dockerfile currently runs as root; create and switch to
a non-root user by adding a user/group (e.g., 1000:1000 or a named user like
"appuser"), chown the application files and workdir to that user after COPY, and
add a USER instruction before CMD so the container drops privileges at runtime;
update any ENV (e.g., HOME) if needed and ensure commands that require root
(like package installs) remain before switching. Reference the Dockerfile's
WORKDIR, COPY, RUN uv sync --frozen --no-dev, and CMD ["uv", "run", ...] when
placing the user creation, chown, and USER directives.
In `@docs/superpowers/plans/2026-05-19-sentry-dsn-metrics-audit.md`:
- Around line 1-2: The file titled "Sentry DSN + Metrics Audit Implementation
Plan" is in the wrong location; move its content into the repository's
single-context docs layout by converting the plan into an ADR-style entry and
placing it under the repository's docs/adr directory, then update the root
CONTEXT.md to reference the new ADR (or remove the stray plan file); ensure the
ADR uses the same title and metadata so reviewers can find it and delete the
original plan file from docs/superpowers/plans to avoid duplication.
- Around line 40-41: Replace the real Sentry DSN value currently assigned to the
OPENBOT_SENTRY_DSN example with a non-sensitive placeholder string; locate the
OPENBOT_SENTRY_DSN line in the docs and change the value to a generic example
(e.g. a clearly marked placeholder like
"https://<PUBLIC_KEY>@<HOST>/<PROJECT_ID>" or "your_sentry_dsn_here") so no live
DSN is committed.
In `@docs/superpowers/specs/2026-05-19-sentry-dsn-metrics-audit-design.md`:
- Around line 60-62: Replace the hard-coded Sentry DSN example string for
OPENBOT_SENTRY_DSN with a sanitized placeholder; locate the OPENBOT_SENTRY_DSN
example entry and swap the live DSN value (the long URL starting with
https://...ingest...) for a non-functional placeholder such as
https://<PUBLIC_KEY>`@sentry.io/`<PROJECT_ID> or REDACTED_SENTRY_DSN so the spec
no longer contains real credentials.
In `@openbot/application/middleware/security.py`:
- Around line 226-228: When calling the delegated adapter methods (specifically
ctx.adapter.get_pr_comments invoked where ctx.cache[cache_key] = result and the
nearby call around line 232), wrap the call in a try/except that catches adapter
exceptions and on failure logs the error and returns a safe default (empty list
for comments or "none" for role checks) instead of letting the exception
propagate; also ensure you populate ctx.cache[cache_key] with the default value
so the gate remains fail-closed on adapter failures.
In `@openbot/application/ports/channel_adapter.py`:
- Around line 54-56: The current signature in
openbot.application.ports.channel_adapter where parameters status and conclusion
default to status="completed" and conclusion=None violates GitHub Checks API;
change the default status to a non-completed state (e.g., "in_progress" or
"queued") and keep conclusion optional, and also add runtime validation inside
the same function (referencing the status and conclusion parameters) to
raise/return a clear error if status == "completed" but conclusion is still None
so callers get immediate feedback; update the function/type hints and any
docstring for the function in channel_adapter.py to reflect the corrected
contract.
In `@openbot/core/metrics.py`:
- Around line 93-107: The WorkflowCounter and CostCounter currently mutate
self._kwargs in labels(), causing shared singleton state bugs; change labels()
to return a new immutable bound handle object (e.g., WorkflowCounterBound /
CostCounterBound) that stores the provided kwargs internally and whose
inc(amount) uses that stored kwargs to call
_workflow_total.labels(**kwargs).inc(amount) and metrics.incr("workflow_total",
amount, tags=kwargs); do not write to the singleton's attributes. Apply the same
refactor to the other singleton metric wrappers (the other classes using
labels()/inc()) so labels() returns a bound instance instead of mutating the
singleton.
In `@openbot/infrastructure/adapters/github.py`:
- Line 421: Replace the lax Base64 decode call so invalid payloads fail fast:
change the call using base64.b64decode(encoded, validate=False) to use
validate=True (i.e., base64.b64decode(encoded, validate=True)) in the repository
file decoding function where the variable encoded is decoded; also add handling
for binascii.Error around that call to raise or propagate a clear decoding error
(so callers can surface a useful message) and ensure binascii is imported if
needed.
In `@openbot/infrastructure/agents/deepagents_chat.py`:
- Around line 75-83: The _extract_reply function assumes messages[-1] is an
object with a .content attribute and drops valid replies when the final message
is a dict; update _extract_reply to handle both dict and object shapes by
retrieving content via messages[-1].get("content") if isinstance(messages[-1],
dict) otherwise getattr(messages[-1], "content", None), then pass that content
to _extract_message_text to avoid spurious deepagents_result_missing_text errors
(refer to _extract_reply and messages[-1]).
In `@openbot/infrastructure/queue/worker.py`:
- Around line 93-109: The Sentry tags are currently set globally which can leak
between concurrent consumers; update _attach_sentry_tags to create an explicit
per-dispatch Sentry scope and set tags inside it (use sentry_sdk.push_scope() as
a context manager, or sentry_sdk.isolation_scope() if available), keeping the
existing try/except so missing sentry_sdk is still safe, and ensure the
dispatch/handler that currently calls _attach_sentry_tags invokes it within that
scope (or let _attach_sentry_tags itself manage the scope) so tags (delivery_id,
feature, repo) are isolated per job and cannot bleed across asyncio tasks.
In `@tests/infrastructure/agents/test_deepagents_chat.py`:
- Around line 47-51: The test currently asserts exact prompt wording (checks for
"GitHub maintainer bot" in seen["kwargs"]["system_prompt"] and several
substrings in prompt = seen["payload"]["messages"][0]["content"] like
"YiAgent/openbot", "issue `#42`", "summarize this"); remove those literal
prompt-content assertions from
tests/infrastructure/agents/test_deepagents_chat.py and instead assert only
deterministic contract properties (e.g., that seen contains kwargs.system_prompt
and payload.messages is a non-empty list or has expected message count/type)
using the existing variables seen, system_prompt, payload and messages; move the
exact prompt-quality checks into an eval under evals/ that verifies the prompt
wording and intent.
In `@tests/infrastructure/test_config_blank_coercion.py`:
- Around line 53-54: The test asserts the default for
Settings().debug_echo_enabled but can be influenced by the environment var
OPENBOT_DEBUG_ECHO_ENABLED; modify the test to ensure environment isolation by
clearing or unsetting OPENBOT_DEBUG_ECHO_ENABLED before instantiating Settings
(e.g., using monkeypatch.delenv or os.environ.pop) so Settings() reads a clean
environment, then assert debug_echo_enabled is False; ensure the env change
applies only for the test scope so other tests aren’t affected.
---
Outside diff comments:
In `@README.md`:
- Around line 268-270: Remove the accidental trailing scratch text at the end of
README by deleting the stray lines containing "E2E Test", "Fixed", and "Final
Test" so the file ends cleanly; ensure no leftover headings or placeholder text
remain and that the README content ends with the intended final section or
newline.
---
Nitpick comments:
In `@obser.txt`:
- Line 1: Remove the stray temporary artifact by deleting the file obser.txt (or
removing its single-line content "obser test") before merging; ensure no other
commits reintroduce this test artifact and update the branch so the repository
history no longer contains this noisy temporary file.
In `@tests/application/use_cases/test_triage.py`:
- Around line 116-117: Update the caplog logger names used when setting log
level in the triage tests: replace occurrences of
"openbot.application.workflows.triage" with the new module path
"openbot.application.use_cases.triage" where caplog.at_level is used (e.g., the
test blocks that call caplog.at_level(logging.WARNING,
logger="openbot.application.workflows.triage") around
maybe_run_triage(_ctx(_event(...))) and similar calls); ensure all three
remaining instances (the blocks wrapping maybe_run_triage and related
assertions) use "openbot.application.use_cases.triage" so log-level scoping
matches the moved module.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 59098e62-67e7-4d45-95b1-4a29cf317663
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (95)
.dockerignore.env.example.github/workflows/ci.yml.gitignore.importlinter.pre-commit-config.yamlDockerfileMakefileREADME.mdapp.jsondocker-compose.ymldocs/deploy/heroku.mddocs/superpowers/plans/2026-05-19-sentry-dsn-metrics-audit.mddocs/superpowers/specs/2026-05-19-sentry-dsn-metrics-audit-design.mdobser.txtopenbot/application/dispatcher.pyopenbot/application/handlers/__init__.pyopenbot/application/middleware/audit.pyopenbot/application/middleware/budget.pyopenbot/application/middleware/cancel.pyopenbot/application/middleware/feature_toggle.pyopenbot/application/middleware/preflight.pyopenbot/application/middleware/rate_limit.pyopenbot/application/middleware/sanitize.pyopenbot/application/middleware/security.pyopenbot/application/ports/channel_adapter.pyopenbot/application/ports/config_loader.pyopenbot/application/ports/dedup.pyopenbot/application/ports/queue.pyopenbot/application/router.pyopenbot/application/use_cases/__init__.pyopenbot/application/use_cases/_lifecycle.pyopenbot/application/use_cases/_tracing.pyopenbot/application/use_cases/chat.pyopenbot/application/use_cases/chat_parser.pyopenbot/application/use_cases/debug_echo.pyopenbot/application/use_cases/fix.pyopenbot/application/use_cases/ingest_webhook.pyopenbot/application/use_cases/review.pyopenbot/application/use_cases/triage.pyopenbot/application/workflows/__init__.pyopenbot/core/logging.pyopenbot/core/metrics.pyopenbot/core/sentry_metrics.pyopenbot/core/settings.pyopenbot/domain/config_schema.pyopenbot/domain/dedup.pyopenbot/domain/workflows.pyopenbot/entrypoints/api/app.pyopenbot/entrypoints/api/deps.pyopenbot/entrypoints/api/routes/github_webhook.pyopenbot/entrypoints/api/routes/health.pyopenbot/entrypoints/cli/db_init.pyopenbot/entrypoints/worker/__main__.pyopenbot/infrastructure/adapters/github.pyopenbot/infrastructure/agents/__init__.pyopenbot/infrastructure/agents/deepagents_chat.pyopenbot/infrastructure/config_loader.pyopenbot/infrastructure/llm/complete.pyopenbot/infrastructure/llm/model_router.pyopenbot/infrastructure/observability.pyopenbot/infrastructure/persistence/db.pyopenbot/infrastructure/persistence/dedup.pyopenbot/infrastructure/persistence/models.pyopenbot/infrastructure/queue/enqueue.pyopenbot/infrastructure/queue/worker.pypyproject.tomlscripts/doctor.pyscripts/heroku-monitoring-bootstrap.shscripts/hooks/import-linter.shtests/_fakes/channel_adapter.pytests/_fakes/queue.pytests/application/middleware/conftest.pytests/application/middleware/test_cancel.pytests/application/middleware/test_preflight.pytests/application/middleware/test_security.pytests/application/ports/test_queue_port_contract.pytests/application/test_no_raw_user_input.pytests/application/test_router.pytests/application/use_cases/__init__.pytests/application/use_cases/test_chat.pytests/application/use_cases/test_chat_parser.pytests/application/use_cases/test_triage.pytests/core/__init__.pytests/core/test_sentry_metrics.pytests/e2e/conftest.pytests/e2e/test_spec_demos.pytests/entrypoints/api/test_app_boot.pytests/entrypoints/api/test_ready.pytests/entrypoints/api/test_webhook_endpoint.pytests/entrypoints/cli/test_db_init.pytests/infrastructure/agents/test_deepagents_chat.pytests/infrastructure/test_config_blank_coercion.pytests/infrastructure/test_config_repo.pytests/infrastructure/test_obs.py
💤 Files with no reviewable changes (2)
- openbot/application/workflows/init.py
- openbot/application/handlers/init.py
| # ANTHROPIC_API_KEY= # review/fix → claude-opus-4-7 | ||
| # # triage/chat → claude-sonnet-4-6 | ||
| # OPENAI_API_KEY= # fallback model | ||
| # CLAUDE_SWITCH_GLM_BASE_URL=https://open.bigmodel.cn/api/anthropic # GLM Anthropic Proxy |
There was a problem hiding this comment.
Avoid changing committed .env* files under current policy.
This PR modifies .env.example, which matches the prohibited .env* pattern. Please either move this template to a non-.env* filename or explicitly carve out .env.example in policy before merge.
As per coding guidelines .env*: Do not commit .env* files.
🤖 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 @.env.example at line 26, The commit modifies the `.env.example` entry for
CLAUDE_SWITCH_GLM_BASE_URL which violates the rule disallowing committed .env*
files; either revert the change to keep `.env.example` untouched or move the
template line for CLAUDE_SWITCH_GLM_BASE_URL into a non-.env filename (e.g.,
rename to env.example.template or config/claude-proxy.template) and update
references accordingly, and if you truly intend to commit this template, update
the policy/ignore rules to explicitly allow `env.example.template` (not
`.env*`), ensuring no `.env*` file is added to the repo or commit history.
Comment on lines
+1
to
+14
| FROM ghcr.io/astral-sh/uv:python3.12-bookworm-slim | ||
|
|
||
| WORKDIR /app | ||
|
|
||
| ENV PYTHONUNBUFFERED=1 \ | ||
| UV_LINK_MODE=copy | ||
|
|
||
| COPY . . | ||
|
|
||
| RUN uv sync --frozen --no-dev | ||
|
|
||
| EXPOSE 8080 | ||
|
|
||
| CMD ["uv", "run", "uvicorn", "openbot.entrypoints.api.app:app", "--host", "0.0.0.0", "--port", "8080"] |
There was a problem hiding this comment.
Container runs as root; drop privileges before runtime.
Add a non-root user and switch to it before CMD.
Suggested hardening patch
FROM ghcr.io/astral-sh/uv:python3.12-bookworm-slim
WORKDIR /app
ENV PYTHONUNBUFFERED=1 \
UV_LINK_MODE=copy
COPY . .
RUN uv sync --frozen --no-dev
+RUN addgroup --system openbot && adduser --system --ingroup openbot openbot
+RUN chown -R openbot:openbot /app
+USER openbot
EXPOSE 8080
CMD ["uv", "run", "uvicorn", "openbot.entrypoints.api.app:app", "--host", "0.0.0.0", "--port", "8080"]📝 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.
Suggested change
| FROM ghcr.io/astral-sh/uv:python3.12-bookworm-slim | |
| WORKDIR /app | |
| ENV PYTHONUNBUFFERED=1 \ | |
| UV_LINK_MODE=copy | |
| COPY . . | |
| RUN uv sync --frozen --no-dev | |
| EXPOSE 8080 | |
| CMD ["uv", "run", "uvicorn", "openbot.entrypoints.api.app:app", "--host", "0.0.0.0", "--port", "8080"] | |
| FROM ghcr.io/astral-sh/uv:python3.12-bookworm-slim | |
| WORKDIR /app | |
| ENV PYTHONUNBUFFERED=1 \ | |
| UV_LINK_MODE=copy | |
| COPY . . | |
| RUN uv sync --frozen --no-dev | |
| RUN addgroup --system openbot && adduser --system --ingroup openbot openbot | |
| RUN chown -R openbot:openbot /app | |
| USER openbot | |
| EXPOSE 8080 | |
| CMD ["uv", "run", "uvicorn", "openbot.entrypoints.api.app:app", "--host", "0.0.0.0", "--port", "8080"] |
🧰 Tools
🪛 Trivy (0.69.3)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🤖 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 `@Dockerfile` around lines 1 - 14, The Dockerfile currently runs as root;
create and switch to a non-root user by adding a user/group (e.g., 1000:1000 or
a named user like "appuser"), chown the application files and workdir to that
user after COPY, and add a USER instruction before CMD so the container drops
privileges at runtime; update any ENV (e.g., HOME) if needed and ensure commands
that require root (like package installs) remain before switching. Reference the
Dockerfile's WORKDIR, COPY, RUN uv sync --frozen --no-dev, and CMD ["uv", "run",
...] when placing the user creation, chown, and USER directives.
Comment on lines
+1
to
+2
| # Sentry DSN + Metrics Audit Implementation Plan | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Move this design/plan record into the repository’s single-context docs layout.
This new domain-planning doc is under docs/superpowers/plans/, which conflicts with the documented single-context structure (CONTEXT.md + docs/adr/).
As per coding guidelines, "Use a single-context layout for domain docs: root CONTEXT.md + docs/adr/ (lazily created — silent if absent)".
🤖 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/superpowers/plans/2026-05-19-sentry-dsn-metrics-audit.md` around lines 1
- 2, The file titled "Sentry DSN + Metrics Audit Implementation Plan" is in the
wrong location; move its content into the repository's single-context docs
layout by converting the plan into an ADR-style entry and placing it under the
repository's docs/adr directory, then update the root CONTEXT.md to reference
the new ADR (or remove the stray plan file); ensure the ADR uses the same title
and metadata so reviewers can find it and delete the original plan file from
docs/superpowers/plans to avoid duplication.
Comment on lines
+75
to
+83
| def _extract_reply(result: dict[str, Any]) -> str: | ||
| messages = result.get("messages") | ||
| if not isinstance(messages, list) or not messages: | ||
| raise ValueError("deepagents_result_missing_messages") | ||
| content = getattr(messages[-1], "content", None) | ||
| reply = _extract_message_text(content) | ||
| if not reply: | ||
| raise ValueError("deepagents_result_missing_text") | ||
| return reply |
There was a problem hiding this comment.
Handle dict-shaped final messages in reply extraction.
On Line 79, reply extraction assumes an object with .content. If messages[-1] is a dict, valid replies can be dropped and trigger deepagents_result_missing_text.
🔧 Proposed compatibility patch
def _extract_reply(result: dict[str, Any]) -> str:
messages = result.get("messages")
if not isinstance(messages, list) or not messages:
raise ValueError("deepagents_result_missing_messages")
- content = getattr(messages[-1], "content", None)
+ last = messages[-1]
+ content = last.get("content") if isinstance(last, dict) else getattr(last, "content", None)
reply = _extract_message_text(content)
if not reply:
raise ValueError("deepagents_result_missing_text")
return replyFor deepagents 0.6.1, what is the returned structure of create_deep_agent(...).ainvoke(...), specifically the type/shape of result["messages"] items?
🤖 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 `@openbot/infrastructure/agents/deepagents_chat.py` around lines 75 - 83, The
_extract_reply function assumes messages[-1] is an object with a .content
attribute and drops valid replies when the final message is a dict; update
_extract_reply to handle both dict and object shapes by retrieving content via
messages[-1].get("content") if isinstance(messages[-1], dict) otherwise
getattr(messages[-1], "content", None), then pass that content to
_extract_message_text to avoid spurious deepagents_result_missing_text errors
(refer to _extract_reply and messages[-1]).
Comment on lines
+93
to
+109
| def _attach_sentry_tags(payload: QueuePayload) -> None: | ||
| """Push delivery context to the Sentry scope for the current dispatch. | ||
|
|
||
| Tags appear in the Sentry UI as filterable dimensions, allowing ops to | ||
| jump from a Sentry issue directly to the originating GitHub delivery_id. | ||
| Wrapped in try/except so a missing sentry_sdk dep (slim CI image) never | ||
| crashes the consumer loop. | ||
| """ | ||
| try: | ||
| import sentry_sdk | ||
|
|
||
| sentry_sdk.set_tag("delivery_id", payload.delivery_id) | ||
| sentry_sdk.set_tag("feature", payload.feature) | ||
| sentry_sdk.set_tag("repo", payload.repo) | ||
| except Exception: | ||
| pass # Sentry tagging is best-effort; never crash the dispatch loop | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For sentry-sdk Python 2.x, is sentry_sdk.set_tag(...)on the current scope shared across asyncio tasks unlesspush_scope() is used? Please cite the official scope/isolation docs.
💡 Result:
In the Sentry Python SDK (version 2.0 and later), sentry_sdk.set_tag(...) writes data to the current isolation scope [1][2]. Because the isolation scope is implemented using contextvars (or thread-local storage in older environments), it is designed to be isolated per "concurrency unit," such as an asyncio task [3][1][2]. When using asyncio, the AsyncioIntegration automatically manages the propagation of these scopes [3][4]. If your application is correctly configured with this integration, the SDK ensures that each asyncio task has its own independent isolation scope [3]. Consequently, tags set within one task will not leak into other concurrent tasks [3][1]. If you need to manually isolate data for a specific block of code (e.g., a background task or a specific operation), you should use sentry_sdk.isolation_scope() [3][5]. This creates a new, forked isolation scope that is active only for the duration of the with block [3][1][6]. Once the block exits, the SDK restores the previous isolation scope, ensuring that any tags added within the block do not persist or bleed into the surrounding execution context [1][5][6]. In summary, you do not need to manually manage scope isolation for standard asyncio tasks if the AsyncioIntegration is enabled, as the SDK handles this automatically via contextvars [3][4]. You should use isolation_scope() only when you need to explicitly create a temporary, isolated context for a specific operation [3][1]. References: [3] https://docs.sentry.io/platforms/python/troubleshooting [4] https://docs.sentry.io/platforms/python/integrations/asyncio/ [1] https://develop.sentry.dev/sdk/telemetry/scopes [2] https://github.com/getsentry/sentry-python/blob/master/sentry_sdk/scope.py [5] getsentry/sentry#73423
Citations:
- 1: https://develop.sentry.dev/sdk/telemetry/scopes
- 2: https://github.com/getsentry/sentry-python/blob/master/sentry_sdk/scope.py
- 3: https://docs.sentry.io/platforms/python/troubleshooting
- 4: https://docs.sentry.io/platforms/python/integrations/asyncio/
- 5: Migrate
sentry_sdk.push_scopecalls to SDK 2.0 API getsentry/sentry#73423 - 6: (1) Add new scopes related API (but not yet calling it) getsentry/sentry-python#2609
🏁 Script executed:
# Find where sentry_sdk is initialized and configured
find . -type f -name "*.py" -exec grep -l "sentry_sdk\|AsyncioIntegration" {} \; | head -20Repository: YiAgent/openbot
Length of output: 273
🏁 Script executed:
# Check dependencies to see sentry-sdk version
find . -type f \( -name "requirements*.txt" -o -name "pyproject.toml" -o -name "setup.py" -o -name "setup.cfg" \) | xargs cat 2>/dev/null | grep -i sentryRepository: YiAgent/openbot
Length of output: 350
🏁 Script executed:
# Check the actual worker.py file to understand the execution model
head -150 openbot/infrastructure/queue/worker.py | tail -100Repository: YiAgent/openbot
Length of output: 3661
🏁 Script executed:
# Check sentry initialization and integration setup
cat openbot/infrastructure/observability.pyRepository: YiAgent/openbot
Length of output: 6138
🏁 Script executed:
# Also check sentry_metrics.py for any integration details
cat openbot/core/sentry_metrics.pyRepository: YiAgent/openbot
Length of output: 2222
🏁 Script executed:
# Check how consume_loop is invoked and whether multiple concurrent tasks are created
grep -n "consume_loop\|asyncio\|gather\|Task" openbot/infrastructure/queue/worker.py | head -40Repository: YiAgent/openbot
Length of output: 468
🏁 Script executed:
# Also check the runner.py or main entry point to see how many workers/consumers are spawned
find . -type f -name "*runner*" | grep -E "(queue|infrastructure)" | head -5Repository: YiAgent/openbot
Length of output: 41
🏁 Script executed:
# Find the runner or main entry point
find . -type f -name "*.py" -path "*/queue/*" | xargs grep -l "runner\|main\|__main__" | head -5Repository: YiAgent/openbot
Length of output: 272
🏁 Script executed:
# Also check if there's a runner module
find . -type f -name "*runner*" -o -name "*main*" | grep -v __pycache__ | head -10Repository: YiAgent/openbot
Length of output: 323
🏁 Script executed:
# Look for where consume_loop is called and if multiple are spawned
rg "consume_loop" --type py -B2 -A2Repository: YiAgent/openbot
Length of output: 7241
🏁 Script executed:
# Check the worker entry point to confirm multiple concurrent consume_loop tasks
cat openbot/entrypoints/worker/__main__.py | grep -A20 "asyncio.create_task\|asyncio.gather"Repository: YiAgent/openbot
Length of output: 1948
🏁 Script executed:
# Check if isolation_scope is mentioned or used anywhere in the codebase
rg "isolation_scope" --type pyRepository: YiAgent/openbot
Length of output: 41
🏁 Script executed:
# Check if push_scope is mentioned in the codebase
rg "push_scope" --type pyRepository: YiAgent/openbot
Length of output: 172
🏁 Script executed:
# Look at the full dispatch call site to understand the context better
sed -n '340,380p' openbot/infrastructure/queue/worker.pyRepository: YiAgent/openbot
Length of output: 1786
Scope Sentry tags per dispatch to prevent cross-job tag bleed.
The code at line 358 calls _attach_sentry_tags(payload), which uses sentry_sdk.set_tag(...) without a scope, yet the preceding comment (line 356–357) claims tags "don't bleed into concurrent dispatches" via scoping. This is incorrect:
- No AsyncioIntegration enabled: The sentry initialization (observability.py) registers only
StarletteIntegration,FastApiIntegration, andHttpxIntegration. WithoutAsyncioIntegration, contextvars-based isolation is not guaranteed. - Concurrent dispatches: The worker spawns N consumers via
asyncio.create_task(...) for i in range(settings.worker_concurrency)and runs them withasyncio.gather(), so tags set in one dispatch can leak into another.
Wrap the dispatch in an explicit scope (e.g., sentry_sdk.push_scope() or sentry_sdk.isolation_scope()) to isolate tags per handler.
♻️ Proposed fix
+from contextlib import contextmanager
+
+@contextmanager
+def _sentry_scope(payload: QueuePayload):
+ try:
+ import sentry_sdk
+ except Exception:
+ yield
+ return
+
+ with sentry_sdk.push_scope() as scope:
+ scope.set_tag("delivery_id", payload.delivery_id)
+ scope.set_tag("feature", payload.feature)
+ scope.set_tag("repo", payload.repo)
+ yield
-
-def _attach_sentry_tags(payload: QueuePayload) -> None:
- ...- _attach_sentry_tags(payload)
-
try:
- try:
- await run_dispatch(
- adapter=adapter,
- event=event,
- dispatch=new_dispatch,
- session_factory=session_factory,
- redis=redis,
- check_run_id=payload.check_run_id,
- )
+ with _sentry_scope(payload):
+ try:
+ await run_dispatch(
+ adapter=adapter,
+ event=event,
+ dispatch=new_dispatch,
+ session_factory=session_factory,
+ redis=redis,
+ check_run_id=payload.check_run_id,
+ )
+ except Exception:
+ ...
- except Exception:
- ...Also applies to: 355–360
🤖 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 `@openbot/infrastructure/queue/worker.py` around lines 93 - 109, The Sentry
tags are currently set globally which can leak between concurrent consumers;
update _attach_sentry_tags to create an explicit per-dispatch Sentry scope and
set tags inside it (use sentry_sdk.push_scope() as a context manager, or
sentry_sdk.isolation_scope() if available), keeping the existing try/except so
missing sentry_sdk is still safe, and ensure the dispatch/handler that currently
calls _attach_sentry_tags invokes it within that scope (or let
_attach_sentry_tags itself manage the scope) so tags (delivery_id, feature,
repo) are isolated per job and cannot bleed across asyncio tasks.
Comment on lines
+47
to
+51
| assert "GitHub maintainer bot" in seen["kwargs"]["system_prompt"] | ||
| prompt = seen["payload"]["messages"][0]["content"] | ||
| assert "YiAgent/openbot" in prompt | ||
| assert "issue #42" in prompt | ||
| assert "summarize this" in prompt |
There was a problem hiding this comment.
Move prompt-content assertions out of tests/ and into evals/.
Line 47-Line 51 assert prompt wording/content in unit tests, which violates the test policy and makes the suite brittle to prompt iteration. Keep deterministic contract assertions here and move prompt-quality/content checks to evals.
As per coding guidelines, "Do not put LLM-behavior or prompt-quality assertions in tests/ — those belong in evals/."
🤖 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 `@tests/infrastructure/agents/test_deepagents_chat.py` around lines 47 - 51,
The test currently asserts exact prompt wording (checks for "GitHub maintainer
bot" in seen["kwargs"]["system_prompt"] and several substrings in prompt =
seen["payload"]["messages"][0]["content"] like "YiAgent/openbot", "issue `#42`",
"summarize this"); remove those literal prompt-content assertions from
tests/infrastructure/agents/test_deepagents_chat.py and instead assert only
deterministic contract properties (e.g., that seen contains kwargs.system_prompt
and payload.messages is a non-empty list or has expected message count/type)
using the existing variables seen, system_prompt, payload and messages; move the
exact prompt-quality checks into an eval under evals/ that verifies the prompt
wording and intent.
Comment on lines
+53
to
+54
| def test_debug_echo_is_disabled_by_default() -> None: | ||
| assert Settings().debug_echo_enabled is False |
There was a problem hiding this comment.
Make the default-value assertion environment-isolated.
Line 53 can fail if OPENBOT_DEBUG_ECHO_ENABLED is set in the runner environment. Explicitly clear it in-test to keep this deterministic.
Suggested fix
-def test_debug_echo_is_disabled_by_default() -> None:
+def test_debug_echo_is_disabled_by_default(monkeypatch: pytest.MonkeyPatch) -> None:
+ monkeypatch.delenv("OPENBOT_DEBUG_ECHO_ENABLED", raising=False)
assert Settings().debug_echo_enabled is False📝 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.
Suggested change
| def test_debug_echo_is_disabled_by_default() -> None: | |
| assert Settings().debug_echo_enabled is False | |
| def test_debug_echo_is_disabled_by_default(monkeypatch: pytest.MonkeyPatch) -> None: | |
| monkeypatch.delenv("OPENBOT_DEBUG_ECHO_ENABLED", raising=False) | |
| assert Settings().debug_echo_enabled is False |
🤖 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 `@tests/infrastructure/test_config_blank_coercion.py` around lines 53 - 54, The
test asserts the default for Settings().debug_echo_enabled but can be influenced
by the environment var OPENBOT_DEBUG_ECHO_ENABLED; modify the test to ensure
environment isolation by clearing or unsetting OPENBOT_DEBUG_ECHO_ENABLED before
instantiating Settings (e.g., using monkeypatch.delenv or os.environ.pop) so
Settings() reads a clean environment, then assert debug_echo_enabled is False;
ensure the env change applies only for the test scope so other tests aren’t
affected.
|
🤖 OpenBot received this PR and review is queued. v0.1 alpha: only the ACK is automated so far. Full review (diff scan, structured findings, severity filter) lands in an upcoming commit. |
2 similar comments
|
🤖 OpenBot received this PR and review is queued. v0.1 alpha: only the ACK is automated so far. Full review (diff scan, structured findings, severity filter) lands in an upcoming commit. |
|
🤖 OpenBot received this PR and review is queued. v0.1 alpha: only the ACK is automated so far. Full review (diff scan, structured findings, severity filter) lands in an upcoming commit. |
- redact live Sentry DSN from plan/spec docs (rotate in Sentry dashboard) - delete obser.txt scratch artifact - fix .env.example: OPENBOT_SENTRY_PROFILES_SAMPLE_RATE → OPENBOT_SENTRY_PROFILE_SESSION_SAMPLE_RATE - fix comments: anthropic_api_base alias description, environment field proximity - add SSRF validator to Settings.anthropic_api_base (rejects non-https and private IPs) - extend _blank_to_none to cover anthropic_api_base (blank env var → None) - add LiteLLMCompleter.complete() GLM proxy injection (mirrors module-level complete()) - add debug logging to sentry_profiler_start/stop_failed except blocks - tighten test_obs.py: assert LoggingIntegration breadcrumb/event levels (WARNING/ERROR) - add tests/core/test_settings_anthropic_base.py: 8 tests covering SSRF validator
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
tests/core/test_settings_anthropic_base.py (1)
57-79: ⚡ Quick winUse precise exception assertions for invalid URL cases.
pytest.raises(Exception)is too broad here; it can pass on unrelated errors. Assert the concrete validation exception used by settings loading.Proposed tightening
import pytest +from pydantic import ValidationError @@ - with pytest.raises(Exception, match="https://"): + with pytest.raises(ValidationError, match="https://"): Settings(_env_file=None) # type: ignore[call-arg] @@ - with pytest.raises(Exception, match="private"): + with pytest.raises(ValidationError, match="private"): Settings(_env_file=None) # type: ignore[call-arg] @@ - with pytest.raises(Exception, match=r"private|loopback"): + with pytest.raises(ValidationError, match=r"private|loopback"): Settings(_env_file=None) # type: ignore[call-arg] @@ - with pytest.raises(Exception, match=r"169\.254\.169\.254|metadata"): + with pytest.raises(ValidationError, match=r"169\.254\.169\.254|metadata"): Settings(_env_file=None) # type: ignore[call-arg]🤖 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 `@tests/core/test_settings_anthropic_base.py` around lines 57 - 79, Replace the broad pytest.raises(Exception) assertions with the concrete validation exception raised by the Settings loader (e.g., ValidationError or SettingsError used by your config code) so tests only pass for the intended validation failures; update the asserts in test_anthropic_api_base_rejects_private_ip, test_anthropic_api_base_rejects_loopback, test_anthropic_api_base_rejects_metadata_endpoint (and the earlier URL test) to use that specific exception type when constructing Settings(_env_file=None) after monkeypatching CLAUDE_SWITCH_GLM_BASE_URL and keep the same match patterns for the error message.tests/infrastructure/test_obs.py (1)
74-79: ⚡ Quick winMock
LoggingIntegrationdirectly to assert constructor arguments instead of private SDK internals.Lines 74-79 test unstable private attributes (
_breadcrumb_handler,_handler), which can change across Sentry SDK versions. Instead, mockLoggingIntegrationand assert it was called with the correct args:level=logging.WARNINGandevent_level=logging.ERROR(as you pass in the production code atopenbot/infrastructure/observability.py).🤖 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 `@tests/infrastructure/test_obs.py` around lines 74 - 79, Replace the fragile assertions against private SDK internals by mocking the Sentry SDK LoggingIntegration constructor in the test and asserting it was called with level=logging.WARNING and event_level=logging.ERROR (matching how LoggingIntegration is constructed in observability.py); remove the checks of logging_int._breadcrumb_handler and logging_int._handler and instead verify the mock's call args for LoggingIntegration to ensure correct constructor parameters were passed.
🤖 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/superpowers/plans/2026-05-19-sentry-continuous-profiling.md`:
- Around line 1-25: The plan file for Sentry continuous profiling is placed in
the per-domain plans folder instead of the repo's single-context docs layout;
move the content of this plan (the Sentry Continuous Profiling Implementation
Plan / 2026-05-19 spec) out of the domain-specific docs and into the repository
single-context structure by adding or updating the root CONTEXT.md to reference
it and placing the plan under the centralized ADR/docs area (docs/adr or
equivalent), ensuring the spec link in the plan still points to
docs/superpowers/specs/2026-05-19-sentry-continuous-profiling-design.md and that
any references to Settings, init_sentry(), start_profiler(), and stop_profiler()
remain intact.
In `@docs/superpowers/plans/2026-05-19-sentry-dsn-metrics-audit.md`:
- Around line 35-37: The fenced code blocks that currently contain the
environment snippet (e.g. the block with "# OPENBOT_SENTRY_DSN=") are unlabeled
and trigger MD040; update each opening triple-backtick to include an appropriate
language identifier (for example use ```bash or ```text for environment variable
snippets) for the block containing "# OPENBOT_SENTRY_DSN=" and the other similar
unlabeled blocks referenced nearby so the markdownlint rule is satisfied.
In `@docs/superpowers/plans/2026-05-19-sentry-logs.md`:
- Around line 185-192: In the docs section containing the dependency snippet
(the fenced block with the line "sentry-sdk[fastapi]>=2.35.0"), the code fences
are unlabeled and triggering MD040; edit the fenced code blocks in this section
to add a language tag (e.g., "text" or "bash") immediately after the opening
backticks so the block that currently shows "sentry-sdk[fastapi]>=2.35.0"
becomes a labeled fence (for example ```text or ```bash), which will satisfy the
linter.
In `@docs/superpowers/specs/2026-05-19-sentry-continuous-profiling-design.md`:
- Around line 130-145: The fenced data-flow block is missing a language tag and
triggers MD040; update the opening fence from ``` to ```text for the block that
contains the process flow (the block showing init_sentry(), start_profiler(),
stop_profiler() and SDK flush behavior) so the markdownlint rule is satisfied
and the snippet is treated as plain text.
In `@docs/superpowers/specs/2026-05-19-sentry-dsn-metrics-audit-design.md`:
- Around line 30-35: The fenced code blocks containing the metric function
signatures (count, distribution, gauge, time) are unlabeled and trigger MD040;
update each block to declare a language (e.g., use "text") so the blocks become
"```text" instead of "```"; apply the same change to the other unlabeled blocks
referenced (around the blocks showing similar function signatures at the later
occurrences) so all fenced blocks include a language label.
In `@docs/superpowers/specs/2026-05-19-sentry-logs-design.md`:
- Around line 31-33: The markdown fenced code blocks containing dependency lines
like "sentry-sdk[fastapi]>=2.35.0" (and the other fenced block around the later
section) are unlabeled and trigger MD040; add an explicit language tag to each
fence (e.g., ```text or ```bash or ```pip depending on intent) so the fences
become annotated (for example, change ``` to ```text) and ensure both
occurrences (the shown dependency snippet and the block in the 89-100 region)
are updated.
In `@openbot/core/settings.py`:
- Around line 204-221: The validator currently only rejects private IPs but
allows empty or "localhost" hostnames; update the check in the
anthropic_api_base validation (the block that computes hostname from parsed and
checks addr/is_private/etc.) to reject empty hostnames and literal "localhost"
(case-insensitive) before doing IP parsing — e.g., if hostname == "" or
hostname.lower() == "localhost": raise ValueError(...) with the same style
message — and also add "localhost" to the _blocked set that currently contains
"169.254.169.254" and "metadata.google.internal" so cloud metadata and loopback
hostnames are both explicitly blocked.
---
Nitpick comments:
In `@tests/core/test_settings_anthropic_base.py`:
- Around line 57-79: Replace the broad pytest.raises(Exception) assertions with
the concrete validation exception raised by the Settings loader (e.g.,
ValidationError or SettingsError used by your config code) so tests only pass
for the intended validation failures; update the asserts in
test_anthropic_api_base_rejects_private_ip,
test_anthropic_api_base_rejects_loopback,
test_anthropic_api_base_rejects_metadata_endpoint (and the earlier URL test) to
use that specific exception type when constructing Settings(_env_file=None)
after monkeypatching CLAUDE_SWITCH_GLM_BASE_URL and keep the same match patterns
for the error message.
In `@tests/infrastructure/test_obs.py`:
- Around line 74-79: Replace the fragile assertions against private SDK
internals by mocking the Sentry SDK LoggingIntegration constructor in the test
and asserting it was called with level=logging.WARNING and
event_level=logging.ERROR (matching how LoggingIntegration is constructed in
observability.py); remove the checks of logging_int._breadcrumb_handler and
logging_int._handler and instead verify the mock's call args for
LoggingIntegration to ensure correct constructor parameters were passed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 40fedb50-c62c-4767-aaca-11bb081f3b51
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.env.exampledocs/superpowers/plans/2026-05-19-sentry-continuous-profiling.mddocs/superpowers/plans/2026-05-19-sentry-dsn-metrics-audit.mddocs/superpowers/plans/2026-05-19-sentry-logs.mddocs/superpowers/specs/2026-05-19-sentry-continuous-profiling-design.mddocs/superpowers/specs/2026-05-19-sentry-dsn-metrics-audit-design.mddocs/superpowers/specs/2026-05-19-sentry-logs-design.mdopenbot/core/settings.pyopenbot/entrypoints/api/app.pyopenbot/entrypoints/worker/__main__.pyopenbot/infrastructure/llm/complete.pyopenbot/infrastructure/observability.pypyproject.tomltests/core/test_settings_anthropic_base.pytests/infrastructure/test_obs.py
🚧 Files skipped from review as they are similar to previous changes (4)
- .env.example
- openbot/entrypoints/worker/main.py
- openbot/infrastructure/llm/complete.py
- openbot/entrypoints/api/app.py
Comment on lines
+1
to
+25
| # Sentry Continuous Profiling Implementation Plan | ||
|
|
||
| > **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. | ||
|
|
||
| **Goal:** Replace transaction-based profiling (`profiles_sample_rate`) with continuous profiling (`profile_session_sample_rate` + `start_profiler()`) in both the webapp and worker entrypoints. | ||
|
|
||
| **Architecture:** `sentry_sdk.init()` receives `profile_session_sample_rate` (configured via Settings); the entrypoints own the profiler lifecycle — `start_profiler()` at startup, `stop_profiler()` in the webapp's shutdown block (worker relies on process-exit flush). Both profiler calls are wrapped in `try/except Exception: pass` for graceful degradation. | ||
|
|
||
| **Tech Stack:** Python 3.12, sentry-sdk ≥ 2.35.0 (already installed at 2.60.0), pytest, pydantic-settings | ||
|
|
||
| **Spec:** `docs/superpowers/specs/2026-05-19-sentry-continuous-profiling-design.md` | ||
|
|
||
| --- | ||
|
|
||
| ## File map | ||
|
|
||
| | Action | Path | Purpose | | ||
| |---|---|---| | ||
| | Modify | `openbot/core/settings.py` | Rename field + default 1.0 | | ||
| | Modify | `openbot/infrastructure/observability.py` | Swap `profiles_sample_rate` → `profile_session_sample_rate` | | ||
| | Modify | `openbot/entrypoints/api/app.py` | `start_profiler()` on startup, `stop_profiler()` in finally | | ||
| | Modify | `openbot/entrypoints/worker/__main__.py` | `start_profiler()` after `init_sentry()` | | ||
| | Modify | `tests/infrastructure/test_obs.py` | Swap assertion + Settings constructor | | ||
|
|
||
| --- |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Move this plan/spec documentation set into the repository’s single-context docs layout.
This file is added under docs/superpowers/plans/, which conflicts with the documented domain-doc structure for this repo.
As per coding guidelines, "Use a single-context layout for domain docs: root CONTEXT.md + docs/adr/ (lazily created — silent if absent)".
🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Ensure spelling is correct
Context: ...rate+start_profiler()`) in both the webapp and worker entrypoints. **Architecture...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_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 `@docs/superpowers/plans/2026-05-19-sentry-continuous-profiling.md` around
lines 1 - 25, The plan file for Sentry continuous profiling is placed in the
per-domain plans folder instead of the repo's single-context docs layout; move
the content of this plan (the Sentry Continuous Profiling Implementation Plan /
2026-05-19 spec) out of the domain-specific docs and into the repository
single-context structure by adding or updating the root CONTEXT.md to reference
it and placing the plan under the centralized ADR/docs area (docs/adr or
equivalent), ensuring the spec link in the plan still points to
docs/superpowers/specs/2026-05-19-sentry-continuous-profiling-design.md and that
any references to Settings, init_sentry(), start_profiler(), and stop_profiler()
remain intact.
Comment on lines
+35
to
+37
| ``` | ||
| # OPENBOT_SENTRY_DSN= | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
These fences are unlabeled and trigger markdownlint MD040; please label them (text, bash, etc.) to keep docs lint-clean.
Also applies to: 39-41, 54-56
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/superpowers/plans/2026-05-19-sentry-dsn-metrics-audit.md` around lines
35 - 37, The fenced code blocks that currently contain the environment snippet
(e.g. the block with "# OPENBOT_SENTRY_DSN=") are unlabeled and trigger MD040;
update each opening triple-backtick to include an appropriate language
identifier (for example use ```bash or ```text for environment variable
snippets) for the block containing "# OPENBOT_SENTRY_DSN=" and the other similar
unlabeled blocks referenced nearby so the markdownlint rule is satisfied.
Comment on lines
+185
to
+192
| ``` | ||
| "sentry-sdk[fastapi]>=2.18", | ||
| ``` | ||
|
|
||
| Replace with: | ||
| ``` | ||
| "sentry-sdk[fastapi]>=2.35.0", | ||
| ``` |
There was a problem hiding this comment.
Label the fenced code blocks with a language.
The blocks in this section are unlabeled and flagged by MD040; adding text (or a specific language) will resolve the lint warning.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 185-185: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 190-190: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/superpowers/plans/2026-05-19-sentry-logs.md` around lines 185 - 192, In
the docs section containing the dependency snippet (the fenced block with the
line "sentry-sdk[fastapi]>=2.35.0"), the code fences are unlabeled and
triggering MD040; edit the fenced code blocks in this section to add a language
tag (e.g., "text" or "bash") immediately after the opening backticks so the
block that currently shows "sentry-sdk[fastapi]>=2.35.0" becomes a labeled fence
(for example ```text or ```bash), which will satisfy the linter.
Comment on lines
+130
to
+145
| ``` | ||
| Process starts | ||
| └─ init_sentry() # configures SDK with profile_session_sample_rate=1.0 | ||
| └─ start_profiler() # begins continuous profiling session | ||
|
|
||
| Process runs | ||
| └─ profiler samples stack traces every ~10ms → buffered in SDK | ||
| └─ SDK flushes profile data to Sentry ingest on interval | ||
|
|
||
| webapp: SIGTERM → lifespan finally | ||
| └─ stop_profiler() # flush remaining profile data | ||
| └─ adapter/redis/db teardown | ||
|
|
||
| worker: SIGTERM → shutdown event → asyncio.run() returns | ||
| └─ SDK atexit hook flushes remaining profile data (no explicit stop needed) | ||
| ``` |
There was a problem hiding this comment.
Specify a language for the data-flow fenced block.
This block is currently unlabeled and triggers MD040. Use text to satisfy markdownlint.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 130-130: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/superpowers/specs/2026-05-19-sentry-continuous-profiling-design.md`
around lines 130 - 145, The fenced data-flow block is missing a language tag and
triggers MD040; update the opening fence from ``` to ```text for the block that
contains the process flow (the block showing init_sentry(), start_profiler(),
stop_profiler() and SDK flush behavior) so the markdownlint rule is satisfied
and the snippet is treated as plain text.
Comment on lines
+30
to
+35
| ``` | ||
| count(name, value, unit=None, attributes=None) | ||
| distribution(name, value, unit=None, attributes=None) | ||
| gauge(name, value, unit=None, attributes=None) | ||
| time(name, value, unit=None, attributes=None) | ||
| ``` |
There was a problem hiding this comment.
Fix unlabeled fenced code blocks (MD040).
These fenced blocks should declare a language (text is fine where not executable) to pass markdown lint checks.
Also applies to: 60-62, 99-105
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 30-30: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/superpowers/specs/2026-05-19-sentry-dsn-metrics-audit-design.md` around
lines 30 - 35, The fenced code blocks containing the metric function signatures
(count, distribution, gauge, time) are unlabeled and trigger MD040; update each
block to declare a language (e.g., use "text") so the blocks become "```text"
instead of "```"; apply the same change to the other unlabeled blocks referenced
(around the blocks showing similar function signatures at the later occurrences)
so all fenced blocks include a language label.
Comment on lines
+31
to
+33
| ``` | ||
| "sentry-sdk[fastapi]>=2.35.0", # was >=2.18 — Logs feature requires 2.35+ | ||
| ``` |
There was a problem hiding this comment.
Add language tags to fenced blocks.
The unlabeled fences here are flagged by MD040; please annotate them (text, python, etc.) to keep lint green.
Also applies to: 89-100
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/superpowers/specs/2026-05-19-sentry-logs-design.md` around lines 31 -
33, The markdown fenced code blocks containing dependency lines like
"sentry-sdk[fastapi]>=2.35.0" (and the other fenced block around the later
section) are unlabeled and trigger MD040; add an explicit language tag to each
fence (e.g., ```text or ```bash or ```pip depending on intent) so the fences
become annotated (for example, change ``` to ```text) and ensure both
occurrences (the shown dependency snippet and the block in the 89-100 region)
are updated.
Comment on lines
+204
to
+221
| hostname = parsed.hostname or "" | ||
| try: | ||
| addr = ipaddress.ip_address(hostname) | ||
| if addr.is_private or addr.is_loopback or addr.is_link_local: | ||
| raise ValueError( | ||
| f"anthropic_api_base must not target a private/loopback address: {hostname}" | ||
| ) | ||
| except ValueError as exc: | ||
| # Re-raise our own errors; ignore "not a valid IP" (i.e. hostname). | ||
| if "anthropic_api_base" in str(exc): | ||
| raise | ||
| # Block AWS/GCP/Azure instance metadata hostnames explicitly. | ||
| _blocked = {"169.254.169.254", "metadata.google.internal"} | ||
| if hostname in _blocked: | ||
| raise ValueError( | ||
| f"anthropic_api_base must not target a cloud metadata endpoint: {hostname}" | ||
| ) | ||
| return v |
There was a problem hiding this comment.
Block localhost/empty-host targets in anthropic_api_base validation.
https://localhost/... currently passes because only literal IPs are screened. That leaves a loopback SSRF path open.
🔒 Proposed fix
- hostname = parsed.hostname or ""
+ hostname = (parsed.hostname or "").lower()
+ if not hostname:
+ raise ValueError("anthropic_api_base must include a hostname")
+ if hostname == "localhost" or hostname.endswith(".localhost"):
+ raise ValueError(
+ "anthropic_api_base must not target localhost/loopback hostnames"
+ )
try:
addr = ipaddress.ip_address(hostname)
if addr.is_private or addr.is_loopback or addr.is_link_local:
raise ValueError(
f"anthropic_api_base must not target a private/loopback address: {hostname}"
)🤖 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 `@openbot/core/settings.py` around lines 204 - 221, The validator currently
only rejects private IPs but allows empty or "localhost" hostnames; update the
check in the anthropic_api_base validation (the block that computes hostname
from parsed and checks addr/is_private/etc.) to reject empty hostnames and
literal "localhost" (case-insensitive) before doing IP parsing — e.g., if
hostname == "" or hostname.lower() == "localhost": raise ValueError(...) with
the same style message — and also add "localhost" to the _blocked set that
currently contains "169.254.169.254" and "metadata.google.internal" so cloud
metadata and loopback hostnames are both explicitly blocked.
YiWang24
force-pushed
the
feat/observability-and-infra-updates
branch
from
4045521 to
b40e384
Compare
|
🤖 OpenBot received this PR and review is queued. v0.1 alpha: only the ACK is automated so far. Full review (diff scan, structured findings, severity filter) lands in an upcoming commit. |
YiWang24
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
OPENBOT_SENTRY_DSNis now read bySettingsand passed tosentry_sdk.init()in both the API and worker entrypoints. Set it in.env(see instructions below).Metrics.set()—sentry_sdk.metricshas nosetfunction in 2.x; the guardedhasattrcheck was silently dropping all calls. Replaced with a docstring note pointing callers todistribution().test_obs.pywithprofiles_sample_rateassertion +sentry_initialisedlog-line test.Files changed
openbot/core/sentry_metrics.pyMetrics.set(), fix module docstringtests/core/__init__.pyopenbot/core/mirrortests/core/test_sentry_metrics.pytests/infrastructure/test_obs.pydocs/superpowers/specs/…docs/superpowers/plans/…Setting the DSN (manual step — not committed)
.envfiles are gitignored and Semgrep blocks AI writes to them. Run once:Verify:
For Heroku production:
Test plan
uv run pytest tests/core/test_sentry_metrics.py -v— 7 passeduv run pytest tests/infrastructure/test_obs.py -v— 4 passedmake check— 697 passed, ruff + import-linter clean.env, runmake dev, confirmsentry_initialisedlog line at startupNeed help on this PR? Tag
@codesmithwith what you need.Summary by CodeRabbit
Release Notes
New Features
/readyhealth check endpoint with component status reportingmake db-init)Infrastructure & Operations
Documentation
Sentry Logs (added in follow-up commit)
sentry-sdk[fastapi]>=2.35.0(Logs feature requires 2.35+)enable_logs=Truetosentry_sdk.init()LoggingIntegration(level=WARNING, event_level=ERROR)— keeps INFO lines (e.g.http_request_completed) out of Sentry Logs while forwarding warnings and errorsenable_logs=TrueandLoggingIntegrationpresence in the existing obs test suiteSentry Continuous Profiling (added in follow-up commit)
profiles_sample_rate(transaction profiling) withprofile_session_sample_rate=1.0(continuous profiling)sentry_sdk.profiler.start_profiler()called in webapplifespanstartup and worker_mainafterinit_sentry()stop_profiler()called in webapplifespanfinally block; worker relies on SDK atexit flushtry/except Exception: pass— graceful degradation when SDK absent or DSN unsetprd: removedOPENBOT_SENTRY_PROFILES_SAMPLE_RATE, addedOPENBOT_SENTRY_PROFILE_SESSION_SAMPLE_RATE=1.0