feat(v0.2): add LinearAdapter, plugins, dedup, audit CLI, ConfigApprovalGate#103
feat(v0.2): add LinearAdapter, plugins, dedup, audit CLI, ConfigApprovalGate#103YiWang24 wants to merge 7 commits into
Conversation
- show <task_id>: full audit trail + cost breakdown for a single task - export --format csv|json: export cost_meter rows - budget reset --repo|--global: emergency cost_meter cleanup - 9 new unit tests for parser, rendering, export
- openbot_plugins/__init__.py: plugin registry (register/get_all_plugins) - reproduce_python_issue: extract traceback, identify error, suggest repro - reproduce_js_issue: extract JS/TS stack trace, suggest repro - summarize_pr_diff: parse unified diff, group by added/modified/deleted - 11 unit tests for all 3 plugins
…GraphQL reply - LinearAdapter: ChannelAdapter implementation for Linear - verify_signature: HMAC-SHA256 of raw body - parse_event: Issue.create/update → ISSUE_OPENED/EDITED - reply/update_comment: GraphQL mutation to Linear API - Stub methods for PR/label/code operations - FastAPI route: POST /webhook/linear (same pattern as GitHub) - deps.py: verified_linear_event dependency - 8 unit tests for signature verification + event parsing
…label - ConfigApprovalGate middleware: checks if PR modifies .openbot/config.yaml without config-approved label, blocks with informative comment - 5 unit tests: non-PR, no config mention, missing label, approved, other labels
…case - dedup.py: use case with embed → search → rerank → comment pipeline - embedding.py: EmbeddingService (Voyage primary, OpenAI fallback) + search_similar_issues via pgvector cosine similarity - 003 migration: pgvector extension + issue_embeddings table + IVFFlat index - 7 unit tests for render_dedup_comment + check_duplicates flows
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? |
|
Warning Review limit reached
More reviews will be available in 57 minutes and 15 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (26)
✨ Finishing Touches🧪 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 |
![openbot-dev[bot]](https://avatars.githubusercontent.com/u/279808688?s=60&v=4){kind=small}
There was a problem hiding this comment.
Large feature PR (v0.2) adding ConfigApprovalGate, LinearAdapter, Audit CLI extensions, embedding/dedup pipeline, and plugin system — with a critical signature-verification bypass, several high-risk issues (pgvector type mismatch, unsized app-scoped HTTP client, unscoped budget reset, missing rerank pipeline), and medium/low concerns.
|
|
||
| def _get_http(self) -> httpx.AsyncClient: | ||
| if self._http is None: | ||
| headers = {"Content-Type": "application/json"} |
There was a problem hiding this comment.
critical — hmac.new should be hmac.HMAC (or hmac.new is not a real function — the actual function is hmac.new, but the correct call is hmac.new(key, msg, digestmod). However, the real bug is that hmac.new is lowercase — Python's module-level function is hmac.new(). Actually, hmac.new IS valid. But the signature format mismatch is the real problem: Linear docs send the signature as sha256=<hex>, but the code compares the raw header value directly against the computed hexdigest, meaning any real Linear webhook will be rejected as invalid. This effectively blocks all legitimate webhooks.
expected = hmac.new(self._webhook_secret, body, hashlib.sha256).hexdigest()
if not hmac.compare_digest(signature, expected):
| sa.UniqueConstraint("repo", "issue_number", name="uq_issue_embeddings_repo_issue"), | ||
| ) | ||
|
|
||
| # Create IVFFlat index for fast cosine similarity search |
There was a problem hiding this comment.
high — The embedding column is defined as sa.Text() but the pgvector index uses vector_cosine_ops on it. pgvector requires the column type to be vector, not text. This migration will fail at the CREATE INDEX step or silently create a broken index.
sa.Column("embedding", sa.Text(), nullable=False), # stored as vector via raw SQL
| 3. OPENBOT_POSTGRES_URL configured | ||
|
|
||
| Returns list of dicts: {issue_number, title, similarity} | ||
| """ |
There was a problem hiding this comment.
high — create_async_engine is called inside search_similar_issues() on every invocation but is never disposed. Each call leaks a connection pool / engine. The engine should be reused or explicitly disposed after use.
engine = create_async_engine(postgres_url)
async with engine.connect() as conn:
|
|
||
| def _get_http(self) -> httpx.AsyncClient: | ||
| if self._http is None: | ||
| headers = {"Content-Type": "application/json"} |
There was a problem hiding this comment.
high — The _http httpx.AsyncClient is lazily created and stored on self but never closed. Since LinearAdapter is typically an app-lifetime singleton, there should be an explicit aclose() / shutdown hook, or the client should be used as an async context manager. On shutdown or adapter replacement, this leaks connections.
self._http = httpx.AsyncClient(base_url=_GRAPHQL_URL, headers=headers, timeout=10.0)
| "feature": str(r.feature), | ||
| "model": r.model, | ||
| "cost_usd": str(r.cost_usd), | ||
| "cost_status": str(r.cost_status), |
There was a problem hiding this comment.
high — reset_budget() deletes ALL cost_meter rows when --global is passed, with no safety net beyond a --yes prompt. This is irreversible data destruction of audit/billing records. Consider soft-delete (mark as reset) or requiring a backup/export first. The session_scope commits automatically, so there's no rollback path.
if global_reset:
result = await session.execute(delete(CostMeter))
| return MiddlewareDecision.proceed() | ||
|
|
||
| # Check if the PR touches .openbot/config.yaml via raw payload | ||
| pr_body = event.raw.get("pull_request", {}).get("body", "") |
There was a problem hiding this comment.
high — The gate only checks if .openbot/config.yaml is mentioned in the PR title or body — it doesn't actually inspect the PR's changed files. A PR that modifies the config file without mentioning it in the title/body will bypass the gate entirely.
config_mentioned = _CONFIG_PATH in pr_body or _CONFIG_PATH in pr_title
if not config_mentioned:
return MiddlewareDecision.proceed()
| """ | ||
| if embedding_func is None or search_func is None: | ||
| _logger.info("dedup_skipped_not_configured") | ||
| return DedupResult(candidates=[], comment="") |
There was a problem hiding this comment.
medium — The rerank_func parameter is accepted but never called — the pipeline skips directly from search results to rendering. The docstring says 'LLM rerank → top-3 candidates' but the code just takes similar[:rerank_top] without reranking.
# Step 3: Rerank (optional)
candidates = [
DedupCandidate(...)
for s in similar[:rerank_top]
]
| async def get_issue(self, event: UnifiedEvent, issue_number: int) -> dict[str, Any]: | ||
| """Fetch Linear issue details via GraphQL.""" | ||
| query = """ | ||
| query Issue($id: String!) { |
There was a problem hiding this comment.
medium — get_issue() queries by internal Linear UUID (issue.id from the webhook payload) but the public issue_number parameter is ignored. If called with a different issue number than the event's issue, it will return the wrong issue.
async def get_issue(self, event: UnifiedEvent, issue_number: int) -> dict[str, Any]:
...
issue_id = event.raw.get("data", {}).get("id")
| @@ -1,9 +1,11 @@ | |||
| """FastAPI dependencies for the API entrypoint. | |||
There was a problem hiding this comment.
medium — The module docstring still says 'At the moment this module only exposes the GitHub webhook ingress boundary' in the first paragraph, but the diff was supposed to update it. The old docstring lines (1–5) remain unchanged because the edit only touched lines 3–8.
At the moment this module only exposes the GitHub webhook ingress boundary:
read raw bytes, verify the signature, parse the event, and surface a single
There was a problem hiding this comment.
Large feature PR adding v0.2 features (LinearAdapter, audit CLI, dedup, config approval gate, plugins). Several correctness and robustness issues found: config approval gate is trivially bypassable, migration creates a vector index on a text column, budget reset deletes data without committing, and LinearAdapter doesn't populate required pr_number field.
Repo-wide notes:
- high —
openbot/application/middleware/config_approval.py: The config approval gate checks only PR title/body for the string.openbot/config.yamlinstead of actually fetching the PR's changed files. A PR that renames/deletes config.yaml, or usesgit mv, or simply doesn't mention the path in title/body, bypasses the gate entirely. This is a security-sensitive check that must validate the actual file list from the API. - high —
openbot/infrastructure/persistence/migrations/versions/003_add_issue_embeddings.py: The migration creates an IVFFlat vector index on theembeddingcolumn, but the column is defined assa.Text(). The index usesembedding vector_cosine_opswhich requires the column to be of typevector. Mixing a plain text column with vector operators will fail at runtime. Either change the column to usevectortype (e.g. via raw DDLembedding vector(1024)) or remove the index until the column type is correct. - medium —
openbot/entrypoints/cli/audit.py: Thereset_budgetfunction executesDELETEstatements but the surroundingsession_scopecontext manager may not auto-commit. Ifsession_scopeis a read-only or rollback-on-exit context manager, the deletes will be silently lost. Additionally,result.rowcountis accessed after the session may have been closed, which can raiseMissingGreenletorDetachedInstanceErrordepending on the async session configuration. - medium —
openbot/infrastructure/adapters/linear.py: Linear issue numbers use theidentifierfield (e.g. "ENG-123") and the adapter extracts only the numeric portion. Butissue_numberisintonUnifiedEvent— if the identifier parsing fails (no hyphen),issue_numberremainsNone. More importantly, Linear identifiers are team-scoped ("ENG-123" vs "OPS-123" could be the same number), so using just the integer creates ambiguity when multiple Linear teams send webhooks. - medium —
openbot/infrastructure/llm/embedding.py:search_similar_issuescreates a newAsyncEngineon every call (create_async_engine(postgres_url)) but never disposes it. This leaks connection pools. The engine should be reused (e.g. passed as a parameter or obtained from a shared factory), matching the pattern used in the audit CLI. - medium —
openbot/application/middleware/config_approval.py: The_HIGH_RISK_PREFIXEStuple is defined but never used anywhere in the gate logic. The comment says it checks for 'high-risk fields' but the implementation only checks if the config path is mentioned in PR title/body — it doesn't inspect which fields actually changed. Dead code suggests incomplete implementation.
YiWang24
left a comment
YiWang24
left a comment
There was a problem hiding this comment.
Review — v0.2 feature batch
Reviewed the 5 features locally against main. The structure is clean and well-tested at the unit level, but a few items need attention before this is runtime-ready. Prioritized below.
🔴 Blocking / correctness
-
LinearAdapteris unreachable dead code at runtime. Thelinear_webhookrouter is never registered inopenbot/entrypoints/api/app.py(only_webhook_router(GitHub) and_health_routerareinclude_router-ed), andapp.state.linear_adapteris never assigned anywhere. Soverified_linear_eventalways raises503andPOST /webhook/linearis never mounted. The 322-line adapter + route + deps function are only exercised by unit tests — nothing wires them into the live app. Either wire it up (register the router + construct the adapter in the lifespan/startup like the GitHub one) or land it explicitly behind an "experimental, not wired" note. -
search_similar_issuesleaks a DB engine on every call (openbot/infrastructure/llm/embedding.py). It callscreate_async_engine(postgres_url)per invocation and neverawait engine.dispose(). Each call opens a fresh connection pool that's never closed → pool/file-descriptor exhaustion under any real traffic. Reuse a module-level/shared engine, or wrap intry/finally: await engine.dispose().
🟠 Should fix
-
ConfigApprovalGateis trivially bypassable (openbot/application/middleware/config_approval.py). It only fires when.openbot/config.yamlis mentioned in the PR title or body (config_mentioned); it never inspects the PR's changed files or diff. A PR that editsconfig.yamlwithout naming the path in its title/body passes straight through. Also_HIGH_RISK_PREFIXESis defined but never used — the gate never actually checks which fields changed, contradicting the docstring ("changes high-risk fields … the bot blocks"). For a security gate, key off the changed-files list (and ideally the diffed keys), not free-text mention. -
check_duplicatesacceptsrerank_funcbut never calls it (openbot/application/use_cases/dedup.py). Step 3 is commented "Rerank (optional)" but just slicessimilar[:rerank_top]; the LLM rerank described in the docstring and PRD §2.3 isn't implemented. Either implement the rerank call or drop the parameter so the signature doesn't over-promise.
🟡 Minor / notes
-
LinearAdapterneveraclose()s its cachedhttpx.AsyncClient, andget_actor_rolereturns"write"unconditionally — any actor on an authenticated Linear webhook is treated as authorized. Fine as a v0.2 stub, but worth a# TODOso it isn't mistaken for a real permission check. -
Scope vs locked boundary:
CLAUDE.mdlists "Channel — GitHub only (v0.1). Do not add … Linear adapter code." This PR is labeled v0.2 and shipsdocs/superpowers/specs/v02-overall-spec.md, so I'm treating it as a deliberate scope expansion — just confirming that's intended and that the v0.2 PRD slice is approved. -
Rebase before merge: base is
1e5e7f6, which is well behind currentmain(now past#121/#104plus 11 dependency bumps). Please update the branch so CI runs against the current toolchain before merging.
Happy to push fixes for #1–#4 if you'd like — say the word.
Generated by Claude Code
…gate Addresses review findings on #103: - Wire LinearAdapter into the live app: register the /webhook/linear router and construct app.state.linear_adapter from new OPENBOT_LINEAR_WEBHOOK_SECRET / OPENBOT_LINEAR_OAUTH_TOKEN settings (mirrors the GitHub receive-only fallback). Previously the adapter + route + deps were unreachable dead code. Add LinearAdapter.aclose() and call it on shutdown. - search_similar_issues: stop creating (and leaking) a new async engine per call; cache one engine per Postgres URL. - ConfigApprovalGate: key off the PR diff (real changed files) instead of a free-text mention of the config path in the title/body, which was trivially bypassable. Surface the changed high-risk keys in the block comment; fail open on diff-fetch errors. - check_duplicates: actually invoke rerank_func when supplied (was accepted but ignored), falling back to similarity order otherwise. Tests: diff-based config-gate cases, rerank invocation, LinearAdapter.aclose, and route-registration guards for both webhooks.
|
Pushed fixes for the review findings ( #1 — LinearAdapter now wired into the live app: registered the #2 — engine leak: #3 — ConfigApprovalGate hardened: now keys off the PR diff (real changed files) instead of a free-text mention of the config path in the title/body, which was trivially bypassable. The block comment surfaces the changed high-risk keys; fails open on diff-fetch errors. #4 — dedup rerank: Added tests: diff-based config-gate cases (incl. fail-open), rerank invocation, I did not touch finding #6 (the Generated by Claude Code |
YiWang24
changed the title
|
Test Coverage ReviewThis is a well-tested PR — 577 tests pass and every new module has a corresponding test file. The coverage is strong for the happy paths. The gaps below are focused on areas that aren't touched by the existing suite. What's already covered ✅
Unit tests needed1. Embedding service — async def test_voyage_provider_builds_correct_request(httpx_mock):
httpx_mock.add_response(json={"data": [{"embedding": [0.1, 0.2]}]})
result = await embed_texts(["hello"], provider="voyage")
assert len(result[0]) > 0
assert httpx_mock.get_request().url.host == "api.voyageai.com"
async def test_openai_fallback_when_voyage_fails(httpx_mock):
# First call (Voyage) fails, second call (OpenAI) succeeds
httpx_mock.add_response(status_code=500) # Voyage
httpx_mock.add_response(json={"data": [{"embedding": [0.3]}]}) # OpenAI
result = await embed_texts(["hello"]) # should use fallback
assert result is not None
def test_empty_input_returns_empty_list():
result = embed_texts_sync([])
assert result == []2. The async def test_valid_linear_signature_returns_event(test_client):
body = b'{"action": "create", "type": "Issue"}'
sig = hmac_sign(body, LINEAR_SECRET)
resp = test_client.post("/webhook/linear", content=body, headers={"linear-signature": sig})
assert resp.status_code == 202
async def test_tampered_body_returns_403(test_client):
body = b'{"action": "create"}'
sig = hmac_sign(b'{"action": "delete"}', LINEAR_SECRET) # signed different body
resp = test_client.post("/webhook/linear", content=body, headers={"linear-signature": sig})
assert resp.status_code == 403
async def test_missing_signature_header_returns_422(test_client):
resp = test_client.post("/webhook/linear", content=b'{}')
assert resp.status_code in (400, 422)
async def test_linear_route_503_when_secret_not_configured(unconfigured_client):
resp = unconfigured_client.post("/webhook/linear", content=b'{}')
assert resp.status_code == 5033. Plugin registry — add to existing plugin test files or new def test_register_adds_tool_to_registry():
from openbot_plugins import _REGISTRY, register
initial_count = len(_REGISTRY)
dummy = StructuredTool.from_function(lambda x: x, name="dummy_test", description="test")
register(dummy)
assert len(_REGISTRY) == initial_count + 1
def test_get_all_plugins_returns_sequence():
from openbot_plugins import get_all_plugins
plugins = get_all_plugins()
assert isinstance(plugins, (list, tuple))
assert all(isinstance(p, StructuredTool) for p in plugins)Integration tests needed
Edge cases to cover
Summary: 6/7 new modules have unit tests. The embedding service ( Posted by automated test-coverage routine. |
2 participants
v0.2 Features (5 commits)
Test plan
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.