[MISSION #80-82] Elite Search & Infrastructure Upgrade (T1 Full Suite)#797
[MISSION #80-82] Elite Search & Infrastructure Upgrade (T1 Full Suite)#797denaev-dev wants to merge 61 commits intoSolFoundry:mainfrom
Conversation
…lassmorphism (T1-012)
- Added project_name to DB and models - Implemented Hot-Relevance ranking - Fixed deadline_before router bug - Added AND/OR skills logic support - Configured Pydantic camelCase aliasing
|
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 mission/spec markdowns and multiple backend and frontend changes: implements a comprehensive async Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 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 unit tests (beta)
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: 40
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
backend/app/api/bounties.py (2)
144-180: 🧹 Nitpick | 🔵 TrivialInput validation on
sortparameter.The
sortparameter (line 155) accepts any string and passes it to the search service. Whilebounty_search_service.pyfalls back to "newest" for unknown values (line 97), consider adding explicit validation with allowed values for clearer API documentation and earlier error feedback.Suggested improvement
- sort: str = Query("newest"), + sort: str = Query("newest", pattern=r"^(newest|reward_high|reward_low|deadline|submissions|best_match)$"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/bounties.py` around lines 144 - 180, The search_bounties endpoint currently accepts any string for the sort parameter and forwards it to BountySearchService; add explicit validation for sort by restricting it to the allowed values used by the service (e.g. the same set handled in BountySearchService.search/fallback logic) — update the search_bounties signature to use a constrained type (Literal[...] or an Enum) or validate the string at the start of the function, return a 400 on invalid values, and then pass the validated sort value into BountySearchParams so API docs and early errors reflect the service's accepted sort options.
154-155:⚠️ Potential issue | 🟠 MajorAdd explicit datetime parsing or validation for
deadline_beforeparameter.
deadline_beforeis declared asOptional[str]in the API endpoint (line 154) but passed directly toBountySearchParams(line 175), where it's typed asOptional[datetime](bounty.py line 417). Pydantic v2 automatically coerces valid ISO datetime strings to datetime objects, and the SQL binding receives the correct type. However, this implicit type conversion at the API boundary should be explicit—either by adding a@field_validatorondeadline_beforein BountySearchParams similar to thevalidate_sortvalidator, or by parsing the string in the endpoint before constructing the params object. This improves API contract clarity and makes validation behavior explicit rather than relying on Pydantic's automatic coercion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/bounties.py` around lines 154 - 155, The deadline_before query is currently declared as Optional[str] in the API endpoint and passed into BountySearchParams which expects Optional[datetime]; make the conversion/validation explicit by either (A) adding a `@field_validator` on BountySearchParams.deadline_before (mirroring validate_sort) to parse ISO strings into datetime and raise a clear error on invalid input, or (B) parse the string to a datetime in the endpoint before constructing BountySearchParams (in the function that declares deadline_before and calls BountySearchParams), ensuring you use a strict ISO parser and surface validation errors as HTTP 400.backend/tests/test_health.py (1)
76-80:⚠️ Potential issue | 🟠 MajorUpdate status code expectations for degraded health states to 503.
The implementation correctly returns 503 for degraded states (when either database or redis is disconnected), as specified in the health.py contract (line 301:
http_status = 200 if core_healthy else 503). However, three tests incorrectly expectstatus_code == 200when reporting degraded state:
test_health_check_db_down(line 76): Database disconnected, Redis uptest_health_check_redis_down(line 99): Database up, Redis disconnectedtest_health_check_both_down(line 128): Both services disconnectedSince
core_healthyis defined as(db_result["status"] == "healthy" and redis_result["status"] == "healthy"), any degraded scenario setscore_healthy = False, which triggers the 503 response. Update all three assertions to expectstatus_code == 503instead of 200.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_health.py` around lines 76 - 80, The tests in backend/tests/test_health.py are asserting 200 for degraded health responses but the health handler sets http_status = 200 if core_healthy else 503; update the three failing tests—test_health_check_db_down, test_health_check_redis_down, and test_health_check_both_down—to assert response.status_code == 503 (leave the rest of their assertions about data["status"] and services unchanged) so they expect the correct status code when database or redis (or both) are disconnected.backend/app/api/payouts.py (2)
304-327:⚠️ Potential issue | 🔴 CriticalMissing authentication on payout execution endpoint.
The
execute_payoutendpoint triggers on-chain SPL token transfers without authentication. Any caller who knows apayout_idcan execute the transfer. This should be protected by admin authentication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/payouts.py` around lines 304 - 327, The execute_payout endpoint lacks authentication — update the router function to require admin authentication by adding a dependency (e.g., Depends(get_current_admin_user) or your existing admin check) to the execute_payout signature or decorator so only authorized admin users can call it; ensure the dependency raises appropriate HTTP 401/403 on failure and leave the existing exception handling (PayoutNotFoundError, InvalidPayoutTransitionError), process_payout call, invalidate_cache(), and PayoutResponse behavior unchanged.
279-297:⚠️ Potential issue | 🔴 CriticalMissing authentication/authorization AND critical import failures on admin approval endpoint.
The
admin_approve_payoutendpoint has two critical issues:
Missing authentication/authorization: The endpoint accepts
admin_iddirectly from the request body without verifying the caller's identity or authorization. Any client can submit an arbitraryadmin_id. This should require authenticated admin credentials viaDepends(get_current_user_id)(already defined inapp.api.auth), similar to how other sensitive endpoints like those inbounties.pyare protected.Broken imports — code cannot run: The endpoint attempts to import
approve_payoutandreject_payoutfromapp.services.payout_service, but these functions do not exist in that file. Only 7 of 13 expected functions are defined (create_buyback,create_payout,list_buybacks,list_payouts, and helper functions exist; missing:approve_payout,reject_payout,get_payout_by_id,get_payout_by_tx_hash,get_total_paid_out,process_payout). This causes anImportErrorat module initialization, making the entire payouts API router non-functional.The service layer implementation for payout approval/rejection logic is incomplete and must be implemented before the endpoint can be used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/payouts.py` around lines 279 - 297, The admin_approve_payout endpoint is insecure and currently breaks at import time; fix it by (1) replacing the untrusted admin_id from AdminApprovalRequest with a dependency on the authenticated user (use Depends(get_current_user_id) from app.api.auth) and enforce admin authorization in admin_approve_payout (accept current_user_id via Depends, remove or ignore body.admin_id, and pass current_user_id to approve_payout/reject_payout), and (2) resolve the ImportError by adding the missing service functions (approve_payout and reject_payout) to app.services.payout_service with the expected signatures used by admin_approve_payout (or adjust the imports to the correct function names if they exist elsewhere); ensure the endpoint imports the implemented functions so the module can initialize.backend/app/services/pg_store.py (1)
37-57:⚠️ Potential issue | 🟠 MajorThe PostgreSQL mapping drops fields that
PayoutResponsenow exposes.Lines 42-56 and 99-110 only persist and hydrate the legacy payout columns plus
created_at.PayoutResponsenow includesretry_count,failure_reason, andupdated_at, butbackend/app/models/tables.pyhas no backing columns for them andload_payoutsnever populates them, so loaded payouts return synthetic defaults instead of stored state.As per coding guidelines,
backend/**: "Analyze thoroughly: API contract consistency with spec".Also applies to: 92-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/pg_store.py` around lines 37 - 57, The Payout mapping is missing new fields (retry_count, failure_reason, updated_at): update backend/app/models/tables.py to add corresponding columns on PayoutTable, then extend persist_payout to pass record.retry_count, record.failure_reason, and record.updated_at into the _upsert call (use _to_uuid for bounty_id as done) and ensure session.commit remains; finally update load_payouts to hydrate PayoutResponse with retry_count, failure_reason, and updated_at from the DB rows so loaded payouts reflect stored state.backend/app/models/bounty.py (1)
402-420:⚠️ Potential issue | 🟠 Major
skills_logicis documented as a two-value switch, but the model accepts any string.Line 409 says this field is
anyorall, yet there is no enum/pattern validation. Invalid values can now flow into the search-selection path, which at best silently changes semantics and at worst becomes a query-construction hazard in the search service.As per coding guidelines,
backend/**: "Analyze thoroughly: Input validation and SQL injection vectors".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/models/bounty.py` around lines 402 - 420, The skills_logic field on BountySearchParams currently allows any string; tighten validation by replacing the loose Field("any", ...) with a constrained type (e.g., a Literal["any", "all"] or a small Enum) and adjust the annotation to that type so Pydantic enforces only "any" or "all"; update any consumers that assume str and add a small unit test to assert invalid values raise validation errors; reference BountySearchParams.skills_logic to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/api/bounties.py`:
- Line 169: The /search endpoint handler references skills_logic but that
variable is not declared, causing a NameError; add a new query parameter named
skills_logic to the function signature of the handler (the function that defines
the /search endpoint) matching the style of the other query params (same type
and default/annotation pattern used in lines 144-158) so the variable is
available where used (skills_logic=skills_logic in the return/constructor call);
ensure the parameter name exactly matches skills_logic so downstream usage stays
correct.
In `@backend/app/api/buybacks.py`:
- Around line 3-4: Remove the unused imports in backend/app/api/buybacks.py:
delete Optional and Depends from the import line so only APIRouter and
HTTPException are imported; ensure no other code in the module relies on
Optional or Depends (e.g., in any route functions or dependency declarations)
before removing them.
- Around line 15-18: The list_buybacks endpoint (function list_buybacks)
currently accepts unbounded skip and limit params; add FastAPI Query validators
to constrain them (import Query) and prevent abuse — e.g., change parameters to
skip: int = Query(0, ge=0) and limit: int = Query(100, ge=1, le=1000) (adjust
max as policy requires), then pass them through to
payout_service.list_buybacks(skip=skip, limit=limit); this ensures bounds
validation is enforced at the HTTP layer.
- Around line 10-13: The record_buyback endpoint currently allows
unauthenticated access and lacks a return type annotation; update the router
decorator and function signature to require an admin auth dependency (e.g., add
a Depends(get_current_admin) or whatever admin auth dependency your project
uses) so only authorized admins can call record_buyback, and add an explicit
return type annotation (-> BuybackResponse) to the async def
record_buyback(buyback_in: BuybackCreate) signature; ensure you pass the
authenticated admin principal into the handler if needed and continue to call
payout_service.create_buyback(buyback_in) inside the function.
In `@backend/app/api/health.py`:
- Around line 253-302: The code currently sets http_status based only on
core_healthy, which can leave a non-healthy overall_status in the response body
while returning 200; change the logic that computes http_status (variable
http_status used in the final JSONResponse) to reflect overall_status as well
(e.g., return 503 whenever overall_status is "degraded" or "unavailable",
otherwise 200) while preserving any existing special-casing for core_healthy if
needed; update the assignment of http_status near the return (the variables
core_healthy, overall_status, http_status and the final JSONResponse) so the
HTTP status code is consistent with the body.
- Around line 80-106: The health endpoints are returning raw exception strings
(e.g., str(exc)) to unauthenticated callers; update all handlers (notably the
SQLAlchemyError and generic Exception branches in the database check and the
Redis handlers in _check_redis) to return a generic, non-sensitive error value
(e.g., {"status":"unavailable","error":"infrastructure_error"} or
{"error":"unavailable"}), and ensure the full exception details are kept only in
server logs by calling logger.exception or logger.error with the exc info; do
this for the SQLAlchemyError branch, the generic Exception branches shown around
the database check and the Redis _check_redis function, and any other similar
handlers mentioned in the comment.
- Around line 121-150: The Solana/GitHub health checks use
httpx.AsyncClient(timeout=CHECK_TIMEOUT) but currently catch
asyncio.TimeoutError and return raw str(exc), which leaks internals; change the
timeout handling to use httpx.Timeout(total=CHECK_TIMEOUT) (or
httpx.Timeout(CHECK_TIMEOUT) depending on your httpx version) when constructing
httpx.AsyncClient and explicitly catch httpx.TimeoutException (and its
subclasses) instead of asyncio.TimeoutError in the exception blocks, and
sanitize responses by returning a generic error string like "timeout" or
"unavailable" (avoid returning str(exc)); update the exception handlers
surrounding httpx.AsyncClient usage (references: httpx.AsyncClient,
CHECK_TIMEOUT, httpx.TimeoutException, httpx.Timeout) to implement these
changes.
In `@backend/app/api/payouts.py`:
- Line 58: The import get_total_paid_out is unused in this module; remove
get_total_paid_out from the import list in backend/app/api/payouts.py (where the
current from ... import ... list includes get_total_paid_out) to eliminate the
unused import, or if you intended to use it, call get_total_paid_out in the
appropriate handler (e.g., within the payout endpoints) and ensure tests/linter
pass; after the change, run the formatter and linter to confirm no remaining
unused imports.
In `@backend/app/main.py`:
- Around line 36-42: The CORS middleware call using app.add_middleware with
CORSMiddleware sets allow_credentials=True while using wildcards
(allow_origins=["*"], allow_methods=["*"], allow_headers=["*"]) which is
invalid; fix by either (A) providing explicit origin(s) in allow_origins and
replacing wildcard allow_methods/allow_headers with explicit lists (e.g.,
["GET","POST",...]) while keeping allow_credentials=True, or (B) if you must
allow any origin, set allow_credentials=False; update the call site in main.py
(the app.add_middleware(CORSMiddleware, ... ) invocation) accordingly so the
configuration matches FastAPI/Starlette rules.
In `@backend/app/middleware/rate_limit.py`:
- Around line 59-64: The _get_client_id function currently trusts the first
X-Forwarded-For value which allows clients to spoof rate-limit buckets; change
it to only trust XFF after validating trusted proxy hops: accept a configured
TRUSTED_PROXIES set (or CIDRs) and compute the client IP by splitting
X-Forwarded-For into a list, appending request.client.host as the last hop, then
iterating from the rightmost hop removing entries that match TRUSTED_PROXIES
until a non-trusted hop remains — use that hop as the client id; also validate
entries are well-formed IPs and fall back to request.client.host or "unknown"
when validation fails (update _get_client_id to implement this logic and
reference TRUSTED_PROXIES where appropriate).
In `@backend/app/middleware/security.py`:
- Around line 35-50: The dispatch method trusts the first X-Forwarded-For hop
(client_ip) allowing spoofing; change client IP resolution to only accept/parse
X-Forwarded-For when the immediate peer (request.client.host) is a trusted
proxy. Add a trusted_proxies configuration (e.g., set or list) and in dispatch:
if request.client.host is in trusted_proxies, parse X-Forwarded-For safely
(choose appropriate hop—usually the left-most original client), otherwise use
request.client.host (or "unknown"); then call redis.sismember("ip_blocklist",
client_ip) as before. Ensure references: dispatch, client_ip,
request.headers["X-Forwarded-For"], request.client.host, trusted_proxies, and
redis.sismember("ip_blocklist") are updated accordingly.
- Around line 20-27: The Content-Security-Policy set in
SecurityMiddleware.dispatch is too restrictive and breaks FastAPI's /docs
Swagger UI; update the CSP header logic in dispatch to either skip/relax the
policy for FastAPI docs paths or add explicit script-src and style-src
directives that allow the Swagger UI resources (e.g., include
https://cdn.jsdelivr.net and the necessary inline script allowance via a safer
mechanism such as a nonce/hash or, if not feasible, 'unsafe-inline' as a
temporary measure) and permit fonts/images as needed (e.g., font-src and
img-src); locate the dispatch method in SecurityMiddleware and modify the
response.headers["Content-Security-Policy"] assignment to implement the
relaxed/conditional policy (or conditional bypass) so /docs loads correctly
while keeping strict defaults for other routes.
In `@backend/app/models/bounty.py`:
- Around line 246-251: The camel-case aliasing is only applied in BountyBase so
models that inherit BaseModel (BountyUpdate, BountyListItem, BountySearchResult,
BountySearchParams) remain snake_case in the API contract; to fix, either have
these models inherit from BountyBase or add the same model_config
("populate_by_name": True and the alias_generator lambda) to each of those
classes (BountyUpdate, BountyListItem, BountySearchResult, BountySearchParams)
so incoming/outgoing fields use the expected camelCase aliases consistently with
the frontend contract.
- Around line 210-214: The project_name Field in the Pydantic model accepts
unbounded strings but the DB column is String(100), so update the project_name
Field in the Bounty model (symbol: project_name in the Bounty model in
backend/app/models/bounty.py) to enforce a 100-character limit (e.g., add
max_length=100 or use a constrained string type) and apply the same change to
the other occurrence referenced (the second project_name definition around line
288); keep the description/examples the same and ensure validation prevents
>100-char values before persistence to match
backend/app/models/bounty_table.py's String(100).
In `@backend/app/services/health.py`:
- Around line 34-44: Move the `_running` check inside the `track_request` lock
to avoid a TOCTOU race: acquire `self._lock` first, then check `self._running`
before updating metrics in `track_request`; reference `_running`, `_lock`, and
`track_request` to locate the change. Also prevent unbounded growth of
`self._path_counts` by normalizing or bounding keys: either canonicalize dynamic
segments (e.g., strip/replace IDs from `path` before using it) or enforce a
fixed-size container (LRU/eviction) for `_path_counts` so new unique/random
paths evict old entries; reference `_path_counts` when applying the
normalization or replacement eviction strategy. Ensure error counting and status
code updates remain inside the same locked region.
In `@backend/app/services/payout_service.py`:
- Around line 110-126: The current duplicate-check in create_buyback (using
pg_store.load_buybacks limited to 1000) can miss older or concurrent duplicates
and lets the DB unique constraint on BuybackTable.tx_hash surface as a 500;
change create_buyback to treat the pre-scan as best-effort but wrap the persist
call (pg_store.persist_buyback) in an exception handler that catches the DB
unique-violation/IntegrityError thrown for BuybackTable.tx_hash and raises an
HTTPException(status_code=400, detail="Buyback with this tx_hash already
exists.") instead; keep or expand the pre-scan if desired but ensure the
DB-level uniqueness error is translated to a 400 to handle races and older
duplicates.
- Around line 47-72: The duplicate-check is racy and limited by
load_payouts(limit=1000); update create_payout to stop scanning and instead
query for an existing payout by bounty_id (e.g., add/use a
pg_store.get_payout_by_bounty(bounty_id) or equivalent) and rely on a
database-level uniqueness constraint on bounty_id in
backend/app/models/tables.py (add a UNIQUE index/constraint for the bounty_id
column). Make the persistence robust by adding the UNIQUE constraint and keeping
the insert via pg_store.persist_payout(PayoutRecord...) but wrap the persist
call to catch the DB integrity error (e.g., IntegrityError) and convert it to
HTTPException(400, ...) for duplicate bounty; this ensures concurrent requests
cannot create duplicate payouts and removes the fragile load_payouts scanning
logic in create_payout.
- Around line 75-108: The code currently truncates reads by calling
pg_store.load_payouts(limit=10000) in list_payouts and get_payout which hides
older payouts; update these functions to use proper store-level operations: in
list_payouts call a pg_store method that implements server-side filtering and
pagination (e.g., pg_store.load_payouts with skip and limit parameters or a new
pg_store.query_payouts(status, recipient, skip, limit)) instead of forcing
limit=10000, and in get_payout call a direct lookup on the store (e.g.,
pg_store.get_payout_by_id(payout_id)) rather than loading all payouts; modify
list_payouts to forward skip/limit and filters to the store and modify
get_payout to fetch the single payout by id and raise 404 if not found.
In `@backend/app/services/pg_store.py`:
- Around line 180-200: persist_bounty is not persisting the bounty.category
field so category data accepted by the API is lost; update the call to _upsert
inside the persist_bounty function to include category=bounty.category (or
bounty.category.value if it's an enum) alongside the other kwargs so
BountyTable's category column is written; reference the persist_bounty function
and BountyTable/BountyBase types to locate where to add the category parameter
and ensure any enum/value handling matches how status is handled.
In `@backend/migrations/V2__add_project_name_and_rebuild_search_vector.sql`:
- Around line 43-44: The migration's full-table UPDATE ("UPDATE bounties SET
updated_at = now()") will lock the entire bounties table; change it to perform
the update in small batches (e.g., loop over primary key ranges or use
LIMIT/ctid batches) or run it in a maintenance window so each batch updates a
subset and commits between batches, ensuring the search_vector rebuild is
triggered without a long-running transaction; update the migration SQL in
V2__add_project_name_and_rebuild_search_vector.sql to implement the batched
UPDATE approach targeting the bounties table.
- Around line 17-26: Remove the dead/overwritten first definition of the trigger
function bounties_search_trigger (the CREATE OR REPLACE FUNCTION block that sets
search_vector using json_array_elements_text) so only the intended/final
definition remains; also eliminate the invalid use of the set-returning function
json_array_elements_text in a scalar assignment (ensure the retained function
builds search_vector from project_name/title/description/skills using a safe
scalar approach instead of directly calling json_array_elements_text).
- Around line 9-13: The second CREATE OR REPLACE FUNCTION that builds the
search_vector is missing aggregation of the skills JSON array and is overwriting
the earlier version; update the active function (the CREATE OR REPLACE FUNCTION
that defines the trigger for jobs/job postings) to aggregate skills via
something like array_to_string(ARRAY(SELECT
jsonb_array_elements_text(NEW.skills)), ' ') and include it as a weight D
component: add setweight(to_tsvector('simple', coalesce(skills_text, '')), 'D')
to the concatenation used for NEW.search_vector (or merge into the existing
tsvector combination), and remove or replace the duplicate earlier function so
only the corrected function is defined and executed.
In `@backend/requirements.txt`:
- Around line 20-22: The three new requirements lack upper bounds and the
suggested constraints were incorrect; update the dependency specs for anthropic,
openai, and psutil to include conservative upper bounds: change
"anthropic>=0.40.0" to "anthropic>=0.40.0,<1.0.0", change "openai>=1.50.0" to
"openai>=1.50.0,<3.0.0", and change "psutil>=5.9.0" to "psutil>=5.9.0,<8.0.0" so
they remain consistent with the project's pinned-style requirements while
allowing the current stable releases.
In `@backend/scripts/setup.sh`:
- Around line 79-81: The syntax check command currently targets "../main.py"
(the line with python3 -m py_compile ../main.py); update that argument to point
to the FastAPI app file at "../app/main.py" so the py_compile step verifies
backend/app/main.py instead of backend/main.py, keeping the surrounding echo and
color variables (GREEN/NC) intact.
- Around line 58-69: The virtual environment is being created using the VENV_DIR
value (referenced as "$VENV_DIR") in the scripts block but the requirements are
installed from "../requirements.txt", which indicates the venv is being created
in the wrong directory; update the script so VENV_DIR points to the backend root
(e.g., set VENV_DIR to "../.venv" or create the venv at the parent dir) and then
source "$VENV_DIR/bin/activate" and run pip install -r
"$VENV_DIR/../requirements.txt" (or simply pip install -r ../requirements.txt
after creating the venv in the parent) to ensure the virtualenv location and the
requirements file path are consistent (modify the VENV_DIR initialization and
the venv creation/activation lines that reference "$VENV_DIR").
- Around line 44-53: Make the path handling in setup.sh robust by defining
SCRIPT_DIR (e.g., resolve the script's directory via BASH_SOURCE) and use it to
build absolute paths for ENV_EXAMPLE and ENV_FILE instead of literal
"../../$ENV_EXAMPLE" and "../$ENV_FILE"; update the existence checks and the cp
invocation that reference ENV_EXAMPLE and ENV_FILE to use the
SCRIPT_DIR-prefixed absolute paths so the script works regardless of the current
working directory, leaving the conditional logic and messages (the if checks and
echo lines) intact.
In `@backend/submit_bounty.py`:
- Around line 1-7: Remove the unused import of the os module at the top of the
file; in the import block that currently includes "import os", keep only the
actually used imports ("import subprocess" and "import logging") so there is no
dead code left, and verify imports around logging.basicConfig and logger =
logging.getLogger("stark_submit") remain unchanged.
- Around line 25-36: The current automated git steps (logger.info and the
subprocess.run calls that perform "git add .", "git commit ...", and "git push
... --force") are dangerous: replace the indiscriminate git add with explicit
staged paths or validate and filter changed files (don't use "git add ."),
remove automatic "--force" pushes and require an explicit flag/confirmation to
allow force, and avoid hardcoding the branch by deriving the target branch from
an argument or environment variable and aborting if it doesn't match the
expected PR source (add a dry-run mode and interactive/CLI confirmation in the
script before running subprocess.run for commit/push). Ensure these safeguards
are enforced around the subprocess.run calls and log via
logger.info/logger.error when aborting.
- Around line 1-39: The file backend/submit_bounty.py should be removed from
this PR because it is unrelated and risky; delete backend/submit_bounty.py from
the branch (or if this automation is intentionally needed, move it to a
dedicated scripts/ or tools/ directory), remove unused imports/variables
(pr_body, os) and replace unsafe git commands in run() (subprocess.run(["git",
"add", "."]), subprocess.run(["git", "push", "origin",
"bounty-169-final-certified", "--force"])) with explicit staged file paths,
interactive/confirmed push (no --force), and parameterized inputs (no hardcoded
wallet or bounty references), keeping logger and run() only if the script is
moved and sanitized.
In `@backend/tests/test_security_mission.py`:
- Around line 10-24: The test_ip_blocklist is a no-op because it never asserts
the response and patches redis after the app/middleware were instantiated;
remove the trailing pass, ensure the Redis mock is applied to the actual
middleware instance before making the request (either patch the module used by
IPBlocklistMiddleware earlier or set IPBlocklistMiddleware.redis = mock_redis
before creating AsyncClient), then perform the request and assert the middleware
behavior (e.g., response.status_code == 403 and the JSON payload contains the
IP_BLOCKED detail). Target symbols: test_ip_blocklist, IPBlocklistMiddleware,
app, AsyncClient, and the patched redis.from_url/mock_redis.
In `@backend/verify_middleware.py`:
- Around line 23-29: The test in the Payload Size Limit block posts to
"/api/sync" which may not exist and will produce 404 instead of 413; update the
check in the test that builds large_body and calls ac.post("/api/sync", ...) to
first assert/examine resp_size.status_code for 404 and handle that case (e.g.,
skip the 413 assertion or log that the endpoint is missing), or switch the
request to an endpoint known to exist in the suite; specifically modify the code
around the ac.post("/api/sync", content=large_body) call and the
resp_size.status_code handling so the test treats 404 as “endpoint missing” and
only treats 413 as the payload-size enforcement success.
- Around line 31-34: The rate-limit audit is currently reading headers from resp
(the /health GET response) instead of the most recent /api/sync POST response;
change the header reads in the Rate Limit Header Audit block to use
resp_size.headers (e.g., replace resp.headers.get(...) with
resp_size.headers.get(...)) so X-RateLimit-Limit and X-RateLimit-Remaining
reflect the POST response that followed the payload test; keep the same header
keys and fallback values.
- Line 4: Remove the unused import by deleting the line "import json" from
backend/verify_middleware.py (or, if JSON functionality is intended, replace it
with actual usage in the relevant function such as any verify middleware
functions), ensuring no other code references the json symbol; this removes the
unused import warning and keeps the module clean.
In `@frontend/src/components/bounties/SkillTags.tsx`:
- Around line 10-13: The mapped BountyTag instances in SkillTags.tsx use
key={skill}, which can collide when skills repeat; update the map call to
include the index (e.g., visible.map((skill, i) => ...)) and set a unique key
like key={`${skill}-${i}`} or, if each skill has an identifier, use that id
(e.g., key={skill.id}) so keys are stable and unique for BountyTag components.
- Around line 5-7: Normalize the maxVisible prop before using it: convert it to
a number, floor it to an integer, clamp it to a minimum of 0 (and optionally to
skills.length), then use that normalized value everywhere (replace uses of
maxVisible in the computations for visible and overflow and the other occurrence
around lines 17-20). For example, compute a single const like normalizedMax =
Math.max(0, Math.floor(Number(maxVisible))) (or clamp to Math.min(skills.length,
... ) if desired) and use normalizedMax for skills.slice(...) and for
calculating overflow so negative or fractional inputs can't produce incorrect UI
badges; update all references (e.g., visible, overflow, and any rendering logic)
to use normalizedMax.
In `@frontend/src/components/common/BountyTag.tsx`:
- Around line 39-46: The BountyTag component currently attaches onClick to a
<span> making it inaccessible to keyboard users; update the render so when
onClick is provided the element uses proper interactive semantics (e.g., render
a <button> or add role="button" + tabIndex={0} and keyDown handler) to support
Enter/Space activation and focus styles; ensure to keep existing
className/styleClasses and preserve cursor/hover/active behavior and update any
handlers in BountyTag to call onClick(label) for both mouse and keyboard
activation.
- Around line 23-25: The current substring checks in BountyTag (variable
"lower") use includes('api') and includes('ui') which cause false positives;
change the logic to match whole tokens instead (e.g., split the tag string into
tokens on non-word characters or whitespace and check token equality, or use
word-boundary regex like /\bapi\b/ and /\bui\b/), update the conditional checks
that reference "lower" accordingly so only exact tag tokens (api, ui, frontend,
backend, security, audit) trigger their respective style returns, and keep the
existing return strings unchanged.
- Around line 33-34: The fallback currently returns invalid Tailwind tokens like
`bg-hsl(${hue}, ...)` (the function that builds/returns these style class
strings and the runtime check `styleClasses.includes('hsl')` must be updated);
replace the malformed classes with Tailwind arbitrary-value syntax (e.g.
`bg-[hsl(${hue}_70%_50%_/_0.1)] text-[hsl(${hue}_70%_70%)]
border-[hsl(${hue}_70%_50%_/_0.3)]`) OR remove the dynamic class fallback
entirely and apply the computed HSL/HSLA values via the component’s inline
`style` prop (compute hsla strings from `hue` and set backgroundColor, color,
borderColor), and remove the fragile regex/string-detection logic that checks
`styleClasses.includes('hsl')`.
---
Outside diff comments:
In `@backend/app/api/bounties.py`:
- Around line 144-180: The search_bounties endpoint currently accepts any string
for the sort parameter and forwards it to BountySearchService; add explicit
validation for sort by restricting it to the allowed values used by the service
(e.g. the same set handled in BountySearchService.search/fallback logic) —
update the search_bounties signature to use a constrained type (Literal[...] or
an Enum) or validate the string at the start of the function, return a 400 on
invalid values, and then pass the validated sort value into BountySearchParams
so API docs and early errors reflect the service's accepted sort options.
- Around line 154-155: The deadline_before query is currently declared as
Optional[str] in the API endpoint and passed into BountySearchParams which
expects Optional[datetime]; make the conversion/validation explicit by either
(A) adding a `@field_validator` on BountySearchParams.deadline_before (mirroring
validate_sort) to parse ISO strings into datetime and raise a clear error on
invalid input, or (B) parse the string to a datetime in the endpoint before
constructing BountySearchParams (in the function that declares deadline_before
and calls BountySearchParams), ensuring you use a strict ISO parser and surface
validation errors as HTTP 400.
In `@backend/app/api/payouts.py`:
- Around line 304-327: The execute_payout endpoint lacks authentication — update
the router function to require admin authentication by adding a dependency
(e.g., Depends(get_current_admin_user) or your existing admin check) to the
execute_payout signature or decorator so only authorized admin users can call
it; ensure the dependency raises appropriate HTTP 401/403 on failure and leave
the existing exception handling (PayoutNotFoundError,
InvalidPayoutTransitionError), process_payout call, invalidate_cache(), and
PayoutResponse behavior unchanged.
- Around line 279-297: The admin_approve_payout endpoint is insecure and
currently breaks at import time; fix it by (1) replacing the untrusted admin_id
from AdminApprovalRequest with a dependency on the authenticated user (use
Depends(get_current_user_id) from app.api.auth) and enforce admin authorization
in admin_approve_payout (accept current_user_id via Depends, remove or ignore
body.admin_id, and pass current_user_id to approve_payout/reject_payout), and
(2) resolve the ImportError by adding the missing service functions
(approve_payout and reject_payout) to app.services.payout_service with the
expected signatures used by admin_approve_payout (or adjust the imports to the
correct function names if they exist elsewhere); ensure the endpoint imports the
implemented functions so the module can initialize.
In `@backend/app/models/bounty.py`:
- Around line 402-420: The skills_logic field on BountySearchParams currently
allows any string; tighten validation by replacing the loose Field("any", ...)
with a constrained type (e.g., a Literal["any", "all"] or a small Enum) and
adjust the annotation to that type so Pydantic enforces only "any" or "all";
update any consumers that assume str and add a small unit test to assert invalid
values raise validation errors; reference BountySearchParams.skills_logic to
locate the change.
In `@backend/app/services/pg_store.py`:
- Around line 37-57: The Payout mapping is missing new fields (retry_count,
failure_reason, updated_at): update backend/app/models/tables.py to add
corresponding columns on PayoutTable, then extend persist_payout to pass
record.retry_count, record.failure_reason, and record.updated_at into the
_upsert call (use _to_uuid for bounty_id as done) and ensure session.commit
remains; finally update load_payouts to hydrate PayoutResponse with retry_count,
failure_reason, and updated_at from the DB rows so loaded payouts reflect stored
state.
In `@backend/tests/test_health.py`:
- Around line 76-80: The tests in backend/tests/test_health.py are asserting 200
for degraded health responses but the health handler sets http_status = 200 if
core_healthy else 503; update the three failing tests—test_health_check_db_down,
test_health_check_redis_down, and test_health_check_both_down—to assert
response.status_code == 503 (leave the rest of their assertions about
data["status"] and services unchanged) so they expect the correct status code
when database or redis (or both) are disconnected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8f498ba0-fc48-4c9f-a91b-23b81d671cb9
📒 Files selected for processing (28)
Mission_80_Prompt.mdMission_80_Specs.mdMission_82_Prompt.mdMission_82_Specs.mdbackend/app/api/bounties.pybackend/app/api/buybacks.pybackend/app/api/health.pybackend/app/api/payouts.pybackend/app/constants.pybackend/app/main.pybackend/app/middleware/rate_limit.pybackend/app/middleware/security.pybackend/app/models/bounty.pybackend/app/models/bounty_table.pybackend/app/services/bounty_search_service.pybackend/app/services/health.pybackend/app/services/payout_service.pybackend/app/services/pg_store.pybackend/migrations/V2__add_project_name_and_rebuild_search_vector.sqlbackend/requirements.txtbackend/scripts/setup.shbackend/submit_bounty.pybackend/tests/test_health.pybackend/tests/test_security_mission.pybackend/verify_middleware.pyfrontend/src/components/bounties/SkillTags.tsxfrontend/src/components/common/BountyTag.tsxfrontend/tsconfig.json
backend/app/api/buybacks.py
Outdated
| from typing import Optional | ||
| from fastapi import APIRouter, Depends, HTTPException |
There was a problem hiding this comment.
Unused imports: Optional and Depends are imported but never used.
Proposed fix
-from typing import Optional
-from fastapi import APIRouter, Depends, HTTPException
+from fastapi import APIRouter📝 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.
| from typing import Optional | |
| from fastapi import APIRouter, Depends, HTTPException | |
| from fastapi import APIRouter, HTTPException |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/buybacks.py` around lines 3 - 4, Remove the unused imports in
backend/app/api/buybacks.py: delete Optional and Depends from the import line so
only APIRouter and HTTPException are imported; ensure no other code in the
module relies on Optional or Depends (e.g., in any route functions or dependency
declarations) before removing them.
| @router.post("/", response_model=BuybackResponse, summary="Record a buyback") | ||
| async def record_buyback(buyback_in: BuybackCreate): | ||
| """Record a manual SOL -> FNDRY buyback event and update treasury stats.""" | ||
| return await payout_service.create_buyback(buyback_in) |
There was a problem hiding this comment.
Missing authentication on buyback recording endpoint.
The record_buyback endpoint allows anyone to record buyback events without authentication. This is a treasury-impacting operation that should require admin authorization.
Additionally, the function lacks a return type annotation for consistency with FastAPI conventions.
Proposed fix for type annotation
`@router.post`("/", response_model=BuybackResponse, summary="Record a buyback")
-async def record_buyback(buyback_in: BuybackCreate):
+async def record_buyback(buyback_in: BuybackCreate) -> BuybackResponse:
"""Record a manual SOL -> FNDRY buyback event and update treasury stats."""
return await payout_service.create_buyback(buyback_in)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/buybacks.py` around lines 10 - 13, The record_buyback
endpoint currently allows unauthenticated access and lacks a return type
annotation; update the router decorator and function signature to require an
admin auth dependency (e.g., add a Depends(get_current_admin) or whatever admin
auth dependency your project uses) so only authorized admins can call
record_buyback, and add an explicit return type annotation (-> BuybackResponse)
to the async def record_buyback(buyback_in: BuybackCreate) signature; ensure you
pass the authenticated admin principal into the handler if needed and continue
to call payout_service.create_buyback(buyback_in) inside the function.
| @router.get("/", response_model=BuybackListResponse, summary="List buybacks") | ||
| async def list_buybacks(skip: int = 0, limit: int = 100): | ||
| """Retrieve a paginated list of all recorded buyback events.""" | ||
| return await payout_service.list_buybacks(skip=skip, limit=limit) |
There was a problem hiding this comment.
Missing input validation bounds on pagination parameters.
The skip and limit parameters lack validation constraints. Without bounds, a malicious client could request limit=999999999 causing performance issues or memory exhaustion.
Proposed fix with validation
`@router.get`("/", response_model=BuybackListResponse, summary="List buybacks")
-async def list_buybacks(skip: int = 0, limit: int = 100):
+async def list_buybacks(
+ skip: int = Query(0, ge=0, description="Number of records to skip"),
+ limit: int = Query(100, ge=1, le=1000, description="Maximum records per page"),
+) -> BuybackListResponse:
"""Retrieve a paginated list of all recorded buyback events."""
return await payout_service.list_buybacks(skip=skip, limit=limit)Note: You'll need to import Query from fastapi.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/buybacks.py` around lines 15 - 18, The list_buybacks endpoint
(function list_buybacks) currently accepts unbounded skip and limit params; add
FastAPI Query validators to constrain them (import Query) and prevent abuse —
e.g., change parameters to skip: int = Query(0, ge=0) and limit: int =
Query(100, ge=1, le=1000) (adjust max as policy requires), then pass them
through to payout_service.list_buybacks(skip=skip, limit=limit); this ensures
bounds validation is enforced at the HTTP layer.
| const visible = skills.slice(0, maxVisible); | ||
| const overflow = skills.length - maxVisible; | ||
|
|
There was a problem hiding this comment.
maxVisible should be normalized before slicing/counting overflow.
Lines 5-7 compute visibility/overflow directly from maxVisible (typed as number). Negative or fractional values can produce invalid UI output (e.g., incorrect +overflow badge values).
As per coding guidelines, frontend/**: “React/TypeScript frontend. Check: Component structure and state management”.
Also applies to: 17-20
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/bounties/SkillTags.tsx` around lines 5 - 7, Normalize
the maxVisible prop before using it: convert it to a number, floor it to an
integer, clamp it to a minimum of 0 (and optionally to skills.length), then use
that normalized value everywhere (replace uses of maxVisible in the computations
for visible and overflow and the other occurrence around lines 17-20). For
example, compute a single const like normalizedMax = Math.max(0,
Math.floor(Number(maxVisible))) (or clamp to Math.min(skills.length, ... ) if
desired) and use normalizedMax for skills.slice(...) and for calculating
overflow so negative or fractional inputs can't produce incorrect UI badges;
update all references (e.g., visible, overflow, and any rendering logic) to use
normalizedMax.
| {visible.map(skill => ( | ||
| <BountyTag | ||
| key={skill} | ||
| label={skill} |
There was a problem hiding this comment.
Duplicate key risk in mapped tags.
Line 12 uses key={skill}, which is not unique when skills repeat. This can cause incorrect tag reuse/reordering during re-renders.
As per coding guidelines, frontend/**: “React/TypeScript frontend. Check: Component structure and state management”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/bounties/SkillTags.tsx` around lines 10 - 13, The
mapped BountyTag instances in SkillTags.tsx use key={skill}, which can collide
when skills repeat; update the map call to include the index (e.g.,
visible.map((skill, i) => ...)) and set a unique key like key={`${skill}-${i}`}
or, if each skill has an identifier, use that id (e.g., key={skill.id}) so keys
are stable and unique for BountyTag components.
| if (lower.includes('backend') || lower.includes('api')) return 'bg-blue-500/20 text-blue-400 border-blue-500/30'; | ||
| if (lower.includes('frontend') || lower.includes('ui')) return 'bg-pink-500/20 text-pink-400 border-pink-500/30'; | ||
| if (lower.includes('security') || lower.includes('audit')) return 'bg-red-500/20 text-red-400 border-red-500/30'; |
There was a problem hiding this comment.
Over-broad substring matching causes false style categorization.
Lines 23-25 use includes('api') and especially includes('ui'), which can match unrelated words and assign incorrect color categories.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/common/BountyTag.tsx` around lines 23 - 25, The
current substring checks in BountyTag (variable "lower") use includes('api') and
includes('ui') which cause false positives; change the logic to match whole
tokens instead (e.g., split the tag string into tokens on non-word characters or
whitespace and check token equality, or use word-boundary regex like /\bapi\b/
and /\bui\b/), update the conditional checks that reference "lower" accordingly
so only exact tag tokens (api, ui, frontend, backend, security, audit) trigger
their respective style returns, and keep the existing return strings unchanged.
| return `bg-hsl(${hue}, 70%, 50%, 0.1) text-hsl(${hue}, 70%, 70%) border-hsl(${hue}, 70%, 50%, 0.3)`; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Tailwind CSS v4, are class names like bg-hsl(210, 70%, 50%, 0.1)valid utilities, or must HSL colors be expressed via arbitrary value syntax (e.g.,bg-[hsl(...)]) or inline styles?
💡 Result:
bg-hsl(210, 70%, 50%, 0.1) is not a valid Tailwind utility in v4.
Tailwind v4 only made some utilities accept “dynamic” unbracketed values (like grid-cols-15), but color utilities still expect a named color token (like bg-sky-500) unless you use arbitrary values. (tailwindcss.com)
Use one of these instead:
- Arbitrary value (recommended for one-offs):
bg-[hsl(210_70%_50%_/_0.1)](underscores stand in for spaces in class names) - Or define a theme variable (then you can use
bg-yourcolor) (tailwindcss.com) - Or inline styles (especially if the value is truly dynamic at runtime) (tailwindcss.com)
Citations:
- 1: https://tailwindcss.com/blog/tailwindcss-v4
- 2: https://tailwindcss.com/docs/customizing-colors
- 3: https://tailwindcss.com/docs/customizing-colors
🏁 Script executed:
cat -n frontend/src/components/common/BountyTag.tsx | head -60Repository: SolFoundry/solfoundry
Length of output: 2895
Fallback color styling uses invalid Tailwind tokens with runtime string-parsing fallback.
Lines 33 generate invalid Tailwind class tokens (bg-hsl(${hue}, 70%, 50%, 0.1) etc.) that do not match Tailwind v4 syntax. While lines 48–52 work around this with a runtime string detection (styleClasses.includes('hsl')) and extract the hue value via regex to apply inline hsla styles, this pattern is fragile and relies on string parsing rather than proper styling declarations.
Instead, use one of these approaches:
- Arbitrary values syntax: Return
bg-[hsl(${hue}_70%_50%_/_0.1)] text-[hsl(${hue}_70%_70%)] border-[hsl(${hue}_70%_50%_/_0.3)](underscores replace spaces in class names) - Pure inline styles: Remove the className fallback entirely and apply all dynamic colors via the
styleprop
The current workaround avoids build-time errors but creates runtime fragility and string-manipulation overhead that reduces maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/common/BountyTag.tsx` around lines 33 - 34, The
fallback currently returns invalid Tailwind tokens like `bg-hsl(${hue}, ...)`
(the function that builds/returns these style class strings and the runtime
check `styleClasses.includes('hsl')` must be updated); replace the malformed
classes with Tailwind arbitrary-value syntax (e.g.
`bg-[hsl(${hue}_70%_50%_/_0.1)] text-[hsl(${hue}_70%_70%)]
border-[hsl(${hue}_70%_50%_/_0.3)]`) OR remove the dynamic class fallback
entirely and apply the computed HSL/HSLA values via the component’s inline
`style` prop (compute hsla strings from `hue` and set backgroundColor, color,
borderColor), and remove the fragile regex/string-detection logic that checks
`styleClasses.includes('hsl')`.
| <span | ||
| onClick={() => onClick?.(label)} | ||
| className={` | ||
| inline-flex items-center px-2.5 py-0.5 rounded-full text-[10px] font-medium | ||
| border backdrop-blur-sm transition-all duration-200 | ||
| ${styleClasses} | ||
| ${onClick ? 'cursor-pointer hover:scale-105 active:scale-95' : ''} | ||
| ${className} |
There was a problem hiding this comment.
Interactive tag is not keyboard-accessible.
Lines 39-46 attach click behavior to a <span> without button semantics/focus handling. When onClick is provided, this blocks keyboard-only interaction for tag actions.
As per coding guidelines, frontend/**: “React/TypeScript frontend. Check: Component structure and state management”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/common/BountyTag.tsx` around lines 39 - 46, The
BountyTag component currently attaches onClick to a <span> making it
inaccessible to keyboard users; update the render so when onClick is provided
the element uses proper interactive semantics (e.g., render a <button> or add
role="button" + tabIndex={0} and keyDown handler) to support Enter/Space
activation and focus styles; ensure to keep existing className/styleClasses and
preserve cursor/hover/active behavior and update any handlers in BountyTag to
call onClick(label) for both mouse and keyboard activation.
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
backend/app/api/payouts.py (1)
107-144:⚠️ Potential issue | 🔴 CriticalFinancial mutation endpoints remain publicly callable.
record_payout,record_buyback,admin_approve_payout, andexecute_payoutexpose treasury-changing operations without any auth or admin check. An unauthenticated caller can create records, approve/reject payouts, or trigger payout execution.As per coding guidelines,
backend/**: "Analyze thoroughly: Authentication/authorization gaps".Also applies to: 177-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/payouts.py` around lines 107 - 144, The financial endpoints (record_payout, record_buyback, admin_approve_payout, execute_payout) are publicly callable; add an authentication/authorization dependency that enforces admin privileges before executing mutations. For each function (record_payout, record_buyback, admin_approve_payout, execute_payout) add a FastAPI dependency parameter (e.g., current_user: User = Depends(get_current_user) or admin: User = Depends(require_admin)) and either call a shared require_admin() dependency or explicitly check current_user.is_admin and raise HTTPException(403) on failure; keep existing error handling and invalidate_cache() calls intact so only authenticated admins can perform these treasury-changing operations.backend/tests/test_health.py (1)
57-69:⚠️ Potential issue | 🟠 MajorThe health contract test still encodes the pre-upgrade schema.
The added assertions only cover metadata. Lines 60-61 still pin database/redis to
connected/disconnected, and there are no checks for the new Solana/GitHub probes or the Mission#80timeout behavior. This can either fail against the upgraded contract or let an off-spec health response pass.As per coding guidelines,
backend/**: "Analyze thoroughly: API contract consistency with spec".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_health.py` around lines 57 - 69, The test in backend/tests/test_health.py currently hardcodes service statuses (data["services"]["database"] == "connected" and data["services"]["redis"] == "connected") and misses new probes and mission-timeout behavior; update the assertions to validate the health contract per spec by: checking data["services"] exists and contains keys for "database", "redis", "solana", and "github"; assert each service value is one of the allowed status tokens (e.g., "connected", "disconnected", "degraded", "unknown") rather than exact strings; keep the metadata checks (version, uptime_seconds >= 0, timestamp endswith "Z"); and add an assertion for the Mission `#80` timeout indicator (e.g., presence and boolean status like data.get("mission_timeout") or the spec-defined key) so the test will correctly accept upgraded contract responses while still validating required fields.backend/app/api/bounties.py (1)
210-214:⚠️ Potential issue | 🟠 MajorAdd input validation for
deadline_beforeandskills_logicquery parameters at the API boundary.
deadline_beforeis typed asOptional[str]in the handler parameter butOptional[datetime]inBountySearchParams. This means validation is deferred untilBountySearchParams()is constructed inside the handler (line 221), causing malformed datetimes to raiseValidationErroras 500 errors instead of 422 responses.
skills_logiclacks any validation. It defaults to"any"but has no field validator. In the search service, any value other than exactly"all"silently triggers OR semantics (?|operator, line 62 of bounty_search_service.py). Invalid values should be rejected at the boundary, not silently degraded.Compare with
sortandcategory, which use@field_validatorto enforce allowed values. Apply the same pattern toskills_logicand changedeadline_before: Optional[str]todeadline_before: Optional[datetime]in the handler signature to validate at the FastAPI boundary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/bounties.py` around lines 210 - 214, Change the handler parameter deadline_before from Optional[str] to Optional[datetime] so FastAPI performs ISO datetime validation at the API boundary (update the handler signature for the endpoint in bounties.py and import datetime), and add a field validator for skills_logic inside BountySearchParams (the same pattern used for sort/category) that only allows "any" or "all" and raises ValueError for other values so invalid skills_logic values are rejected at input rather than silently treated as OR in bounty_search_service.py.backend/app/services/pg_store.py (4)
232-246:⚠️ Potential issue | 🟡 Minorpersist_buyback uses _upsert, risking tx_hash unique constraint violation.
Similar to payouts,
BuybackTable.tx_hashhas a unique constraint. If an upsert changes thetx_hashto one that already exists on another row, anIntegrityErrorwill be raised but not handled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/pg_store.py` around lines 232 - 246, persist_buyback currently calls _upsert which can raise an IntegrityError if BuybackTable.tx_hash uniqueness is violated; wrap the _upsert + session.commit() in a try/except that catches sqlalchemy.exc.IntegrityError, perform session.rollback() on error, and handle the conflict deterministically (e.g., query BuybackTable by tx_hash to skip/update the existing row or log and return) so the IntegrityError is not unhandled; reference persist_buyback, _upsert, and BuybackTable.tx_hash when implementing the catch and conflict-resolution logic.
189-208:⚠️ Potential issue | 🟡 Minorpersist_payout uses _upsert but doesn't handle unique constraint on tx_hash.
PayoutTable.tx_hashhas aunique=Trueconstraint (perbackend/app/models/tables.py). If_upsertupdates an existing row with atx_hashthat conflicts with another row'stx_hash, the database will raise anIntegrityError. This exception is not caught and will propagate as a 500 error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/pg_store.py` around lines 189 - 208, persist_payout calls _upsert which can trigger a unique constraint violation on PayoutTable.tx_hash; wrap the _upsert/commit in a try/except that catches sqlalchemy.exc.IntegrityError, and inside the except check whether the error is a tx_hash uniqueness conflict (use the exception message or inspect orig) and handle it gracefully—either look up the existing PayoutTable row by tx_hash and update that row instead of inserting (using the session and _to_uuid/record.id logic) or raise a controlled application error; ensure you import IntegrityError from sqlalchemy.exc and maintain session rollback/cleanup when catching the exception so the DB session isn’t left in a bad state.
283-302:⚠️ Potential issue | 🟡 Minorload_reputation has no pagination, loading all history into memory.
Line 288 selects all
ReputationHistoryTablerows without limit. For systems with extensive history, this could cause memory exhaustion and slow response times.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/pg_store.py` around lines 283 - 302, The load_reputation function loads all ReputationHistoryTable rows into memory which can OOM; change it to iterate in paginated/batched streaming mode using the DB session instead of loading all scalars at once. Inside load_reputation (and within the async with get_db_session() block) replace the single await session.execute(stmt)).scalars() call with a batched fetch/streaming loop (e.g., use session.stream/select with execution_options or explicit LIMIT/OFFSET or cursor-based paging) that yields rows in chunks and processes each chunk into ReputationHistoryEntry before appending to out; keep the same field mappings (entry_id, contributor_id, bounty_id, bounty_title, bounty_tier, review_score, earned_reputation, anti_farming_applied, created_at) and preserve the order_by ReputationHistoryTable.created_at.desc() while ensuring the loop stops when no more rows are returned.
21-30: 🧹 Nitpick | 🔵 Trivial_upsert lacks conflict handling for concurrent updates.
The get-then-update pattern (lines 24-30) is not atomic. Two concurrent calls for the same
pk_valuecould both seeobj is None, leading to duplicate insert attempts. While PostgreSQL will reject the duplicate PK, this surfaces as an unhandled exception.For true upsert semantics, consider using SQLAlchemy's
insert().on_conflict_do_update()or PostgreSQL'sINSERT ... ON CONFLICT.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/pg_store.py` around lines 21 - 30, The _upsert function uses a non-atomic get-then-add pattern that can race on concurrent calls; replace it with a true DB-level upsert by using SQLAlchemy Core's insert().on_conflict_do_update() against the model class table: convert pk_value via _to_uuid, build an insert statement from model_cls.__table__ (include id=pk_value and the columns), call on_conflict_do_update with the primary key/index (e.g., index_elements=[model_cls.id] or the actual PK column) and set_ to the incoming columns, then execute the statement with await session.execute(...) (and flush/commit as currently expected) instead of session.get/session.add to avoid duplicate-insert errors.
♻️ Duplicate comments (8)
backend/requirements.txt (1)
27-29:⚠️ Potential issue | 🟠 MajorMissing upper bounds on new dependencies creates upgrade instability.
Line 27, Line 28, and Line 29 add
anthropic,openai, andpsutilwith only lower bounds, while the rest of this file consistently uses capped ranges. This can allow silent major-version jumps and break runtime/API compatibility in production.#!/bin/bash set -euo pipefail echo "1) Confirm current specs in backend/requirements.txt" rg -n '^(anthropic|openai|psutil)' backend/requirements.txt echo echo "2) Fetch latest published versions from PyPI (for cap selection)" for pkg in anthropic openai psutil; do echo "== $pkg ==" curl -fsSL "https://pypi.org/pypi/${pkg}/json" | jq -r '.info.version' done echo echo "3) Quick policy consistency check: show bounded vs unbounded entries" python - <<'PY' from pathlib import Path import re p = Path("backend/requirements.txt") lines = p.read_text().splitlines() for i,l in enumerate(lines,1): if re.match(r'^(anthropic|openai|psutil)>=', l): bounded = '<' in l print(f"Line {i}: {l} | upper_bound={'yes' if bounded else 'no'}") PYAs per coding guidelines, "
**: Be DETAILED and SPECIFIC ... for automated LLM pipeline."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/requirements.txt` around lines 27 - 29, The three new dependency lines for anthropic, openai, and psutil in backend/requirements.txt lack upper bounds and must be changed to match the project's bounded-range style; update each entry (anthropic, openai, psutil) to use a capped range like ">=current_min,<next_major" (or "<=latest_patch" if the file uses minor/patch caps) so upgrades cannot jump major versions silently—identify the current minimum versions you added and set the corresponding upper-bound cap consistent with other entries in requirements.txt, then run the same PyPI/version check used in the review to verify the chosen caps.backend/app/models/bounty.py (2)
236-240:⚠️ Potential issue | 🟠 Major
project_namestill accepts values the table cannot store.
backend/app/models/bounty_table.pycaps the column atString(100), but bothBountyBaseandBountyUpdateaccept unbounded strings. Oversize payloads will clear model validation and fail only at persistence time.As per coding guidelines,
backend/**: "Analyze thoroughly: Input validation and SQL injection vectors".Also applies to: 316-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/models/bounty.py` around lines 236 - 240, Bounty models accept unbounded project_name while the DB column is String(100); update the Field definition(s) named project_name in both BountyBase and BountyUpdate to enforce max length 100 (e.g., Field(..., max_length=100) or use pydantic.constr(max_length=100)) so validation fails early and matches backend/app/models/bounty_table.py's String(100).
272-277:⚠️ Potential issue | 🟠 MajorCamelCase aliasing still stops at
BountyBase.
BountyUpdate,BountyListItem/BountySearchResult, andBountySearchParamsstill inherit directly fromBaseModel, so patch/search/list flows keep exposing snake_case fields likeproject_name,deadline_before, andskills_logic. The frontend contract is only partially migrated.As per coding guidelines,
backend/**: "Analyze thoroughly: API contract consistency with spec".Also applies to: 305-317, 408-425, 464-483
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/models/bounty.py` around lines 272 - 277, Several Bounty-related models (BountyUpdate, BountyListItem, BountySearchResult, BountySearchParams) still inherit directly from BaseModel and thus do not pick up the camelCase aliasing defined in the shared model_config; update each of those classes to inherit from BountyBase (the class that defines model_config with populate_by_name and alias_generator) so they automatically use camelCase aliases, or alternatively copy the same model_config into each class if inheritance is not possible; update BountyUpdate, BountyListItem, BountySearchResult, and BountySearchParams accordingly so fields like project_name, deadline_before, and skills_logic are exposed as camelCase to the frontend.backend/app/main.py (1)
128-134:⚠️ Potential issue | 🟠 MajorThis CORS configuration is still invalid for credentialed requests.
Starlette/FastAPI do not support
allow_credentials=Truewith wildcard origins, methods, and headers. Browsers will block credentialed cross-origin calls even though the server boots.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/main.py` around lines 128 - 134, The CORS setup in app.add_middleware using CORSMiddleware is invalid because allow_credentials=True cannot be used with wildcard origins; update the configuration for CORSMiddleware by replacing allow_origins=["*"] with an explicit list of allowed origins (or use allow_origin_regex with a safe regex), or set allow_credentials=False if wildcards are required; keep allow_methods and allow_headers as needed but ensure allow_origins is a concrete list (or regex) whenever allow_credentials=True so credentialed requests are not blocked by browsers.backend/app/middleware/security.py (1)
35-43:⚠️ Potential issue | 🟠 MajorThe CSP is still incorrect for production.
script-srcandstyle-srcstill omit the CDN hosts FastAPI’s default/docsUI depends on, so Swagger/ReDoc remain broken. At the same time, the global'unsafe-inline' 'unsafe-eval'allowances weaken the policy for every other route instead of isolating that relaxation to docs/static pages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/middleware/security.py` around lines 35 - 43, Update the global CSP constants in middleware/security.py so the default CSP_SCRIPT_SRC and CSP_STYLE_SRC include the CDN hosts used by FastAPI docs (e.g., cdn.jsdelivr.net, unpkg.com, and cdnjs.cloudflare.com and keep fonts.googleapis.com/font sources) but remove the global "'unsafe-inline' 'unsafe-eval'" allowances; then add dedicated CSP_DOCS_SCRIPT_SRC and CSP_DOCS_STYLE_SRC (or similar) that include the CDN hosts plus the relaxed "'unsafe-inline' 'unsafe-eval'" only for the docs/static endpoints, and update the docs/static response middleware or route that sets the Content-Security-Policy header to use these docs-specific variables while leaving CSP_SCRIPT_SRC and CSP_STYLE_SRC strict for all other routes (reference CSP_SCRIPT_SRC, CSP_STYLE_SRC, CSP_DOCS_SCRIPT_SRC/CSP_DOCS_STYLE_SRC and the middleware/security.py header-setting logic).backend/app/services/payout_service.py (3)
63-75:⚠️ Potential issue | 🔴 CriticalDuplicate-check race condition and incomplete scan remain unaddressed.
The bounty-level duplicate check at lines 70-75 loads only 5000 payouts, missing older records. Combined with the non-atomic check-then-insert pattern, concurrent requests can race past each other. The
PayoutTable.bounty_idcolumn lacks a unique constraint (perbackend/app/models/tables.py), so the DB won't catch duplicates either.Additionally, the in-memory
tx_hashcheck (lines 63-68) only scans_payout_store, which may be stale or incomplete relative to the database.Recommended fix: Add a
UNIQUEconstraint onPayoutTable.bounty_id(for non-null values) and wrappg_store.persist_payoutin a try/except to catchIntegrityError, converting it to anHTTPException(400). Remove the fragile pre-scan logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/payout_service.py` around lines 63 - 75, Remove the fragile in-memory and limited pre-scan duplicate checks (the loop over _payout_store under _lock and the partial pg_store.load_payouts scan) and instead enforce uniqueness at the DB level by adding a UNIQUE constraint on PayoutTable.bounty_id (for non-null values); then make persist calls use the DB as source-of-truth by wrapping pg_store.persist_payout in a try/except that catches the DB IntegrityError (or the specific SQLAlchemy/asyncpg integrity exception used) and raises HTTPException(status_code=400, detail="Bounty already has a payout" or "Payout with tx_hash already exists" as appropriate); keep references to data.tx_hash and data.bounty_id but remove the pre-insert duplicate scans so concurrent requests rely on the DB constraint and the IntegrityError-to-HTTPException conversion.
142-145:⚠️ Potential issue | 🟠 MajorPayout lookups silently truncate at fixed limits, causing false 404s.
get_payout_by_id(line 142) andget_payout_by_tx_hash(line 149) load only 5000/10000 records respectively. Once the table exceeds these limits, valid older payouts will not be found, returningNoneand causing false "not found" responses to callers.Recommended fix: Implement direct database lookups (
session.get()for ID, indexed query fortx_hash) instead of loading all records and filtering in Python.Also applies to: 149-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/payout_service.py` around lines 142 - 145, get_payout_by_id and get_payout_by_tx_hash currently call pg_store.load_payouts(limit=...) and filter in Python which truncates results and yields false 404s; change get_payout_by_id to load the single payout by primary key via the DB session (use session.get(Payout, payout_id) or the equivalent store method) and change get_payout_by_tx_hash to run a direct indexed query (e.g. session.query(Payout).filter_by(tx_hash=tx_hash).one_or_none() or the equivalent store method) instead of iterating over pg_store.load_payouts; return _payout_to_response(...) when a row is found and preserve the None return when not found.
243-259:⚠️ Potential issue | 🟠 MajorBuyback duplicate check has same race condition and truncation issues.
Line 244 loads only 1000 buybacks for duplicate detection.
BuybackTable.tx_hashhas aUNIQUEconstraint (perbackend/app/models/tables.py), so duplicates outside the scan window—or concurrent inserts—will cause an unhandled databaseIntegrityErrorsurfacing as a 500 error instead of the intended 400.Recommended fix: Wrap
pg_store.persist_buyback(line 256) in a try/except to catch the unique violation and convert toHTTPException(400).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/payout_service.py` around lines 243 - 259, The duplicate-check using load_buybacks(limit=1000) is insufficient; wrap the call to pg_store.persist_buyback(record) in a try/except that catches the DB unique-constraint/IntegrityError (or the specific DB driver unique-violation exception) and re-raise HTTPException(status_code=400, detail="Buyback already exists"); only add the record to _buyback_store under _lock after persist_buyback succeeds (so use BuybackRecord, pg_store.persist_buyback, BuybackTable.tx_hash, _buyback_store, and _lock to locate and update the code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/api/payouts.py`:
- Around line 155-163: The success branch of validate_wallet currently omits the
computed is_program flag so known program addresses are returned as ordinary
valid wallets; update the success return in validate_wallet (and ensure both
branches use the same response shape) to include is_program_address=is_program
in the WalletValidationResponse so callers can detect program accounts;
reference validate_wallet and WalletValidationResponse to locate and modify the
response construction.
In `@backend/app/main.py`:
- Around line 190-208: The contributors_router endpoints were never included in
the app registration block; add app.include_router(contributors_router,
prefix="/api", tags=["contributors"]) to the router registration sequence in
main.py (near the other app.include_router calls such as auth_router,
bounties_router, and contributor_webhooks_router) so the imported
contributors_router is actually mounted and its endpoints become available at
the /api prefix.
- Around line 138-140: The health-check endpoint moved to /api/v2/health but the
middleware exemptions in the IPBlocklistMiddleware, RateLimitMiddleware, and
RateLimiterMiddleware still only whitelist /health or /api/health, causing
probes to be blocked; update each middleware's exemption logic (in the classes
IPBlocklistMiddleware, RateLimitMiddleware, and RateLimiterMiddleware) to also
treat "/api/v2/health" as a permitted path (and preserve the existing /health
and /api/health checks), ensuring the middlewares skip blocking/rate-limiting
for that exact route so health probes are not 403/429'd.
- Around line 136-161: The add_request_id_and_timing middleware must reuse any
request ID already set by prior middleware (LoggingMiddleware) instead of
minting a new one; modify add_request_id_and_timing to first check
request.state.request_id and use it if present, otherwise fall back to
request.headers.get("X-Request-ID") or generate uuid.uuid4(), then assign that
value to request.state.request_id and set response.headers["X-Request-ID"] (but
only if not already set) so LoggingMiddleware, add_request_id_and_timing,
monitor.track_request, and any error payloads all reference the same ID.
In `@backend/app/services/bounty_search_service.py`:
- Around line 61-64: The SQL branch uses "?&" for skills_logic == "all" but the
in-memory fallback still does OR-only matching; update the in-memory filtering
that examines params.skills and params.skills_logic (the code that iterates over
bounty 'b' and checks b.skills in the fallback near the current binds/conditions
logic) to perform AND semantics when params.skills_logic == "all" (e.g., require
all requested skills are present via subset check) and OR semantics otherwise
(require any overlap), so results match the PostgreSQL path for both
skills_logic modes.
In `@backend/app/services/payout_service.py`:
- Around line 181-190: The reject_payout function updates the DB via
pg_store.persist_payout but fails to update the in-memory cache (_payout_store),
causing inconsistency; after awaiting pg_store.persist_payout(record) in
reject_payout, update the _payout_store entry for payout_id to reflect the
changed record (same approach used in approve_payout), including status,
failure_reason, and updated_at so the in-memory cache matches the persisted
payout.
- Around line 288-290: The get_total_paid_out function currently calls
pg_store.load_payouts(limit=10000) which truncates results; change it to compute
the total across all payouts instead of only the first 10k—either by (A)
invoking a DB-side aggregate (preferred) such as adding/using a method like
pg_store.sum_payouts_by_status(status=PayoutStatus.CONFIRMED) to return a single
summed value, or (B) implementing pagination in get_total_paid_out that
repeatedly calls pg_store.load_payouts(...) (removing the fixed 10000 cap) and
accumulates sum(p.amount for p in payouts.values() if p.status ==
PayoutStatus.CONFIRMED) until no more pages; update get_total_paid_out to use
the chosen approach and reference PayoutStatus.CONFIRMED and
pg_store.load_payouts or the new pg_store.sum_payouts_by_status method.
- Around line 261-270: The total returned by list_buybacks is wrong because it
derives total from the truncated items list; update list_buybacks (and/or
pg_store.load_buybacks) so you obtain the true total count rather than
len(items): either call a dedicated count method (e.g.,
pg_store.count_buybacks()) and set total to that value, or change
pg_store.load_buybacks to return a tuple/object with (items_map, total) and use
that total when constructing BuybackListResponse; ensure you still slice the
returned items for pagination and map with _buyback_to_response.
- Around line 272-282: The startup hydration in hydrate_from_database currently
uses hardcoded limits (pg_store.load_payouts(limit=5000),
load_buybacks(limit=1000)) which can miss records; change it to fetch all rows
by paginating or streaming instead of a single fixed-limit call: replace the
single calls with a loop that repeatedly calls pg_store.load_payouts(batch_size,
offset/last_id) and pg_store.load_buybacks(batch_size, offset/last_id) until no
more rows are returned, updating _payout_store and _buyback_store inside the
existing _lock on each batch, keep the final log.info to report total counts,
and preserve the try/except and log.error behavior in hydrate_from_database.
- Around line 203-211: The code currently calls
pg_store.load_payouts(limit=5000) and then checks all_p.get(payout_id) which can
return a false 404 if the payout is beyond the limit, and it persists status
change with pg_store.persist_payout(record) but delays the cache update until
later, leaving a window of stale cache; change the DB fetch to load the specific
payout by id (e.g., add or use a pg_store.load_payout(payout_id) or a filtered
query instead of load_payouts(limit=5000)), keep the existing validation of
record.status, set record.status = PayoutStatus.PROCESSING and
record.updated_at, call await pg_store.persist_payout(record), and immediately
update the cache (the same cache update logic from lines 233-234) right after
persist_payout to eliminate the stale-window before continuing the rest of
process_payout.
- Around line 110-127: The code currently fetches an unbounded set with
pg_store.load_payouts(limit=limit + skip + 1000) and then applies in-memory
filters (recipient, status, bounty_id, token, start_date, end_date), causing
incorrect total counts and memory/performance issues; update
pg_store.load_payouts to accept filter parameters (recipient, status, bounty_id,
token, start_date, end_date, skip, limit) and have it perform the WHERE clauses
and a separate COUNT(*) query so that total reflects the true DB count, then use
the returned page results to populate page and total instead of filtering items
in-memory.
- Around line 157-172: The code currently operates on a possibly stale cached
record from _payout_store (locked by _lock) and persists it without reconciling
with the DB; instead, fetch the authoritative DB record (via
pg_store.load_payouts(...) and .get(payout_id) or a dedicated loader) and use
that for the status check and mutation (PayoutStatus checks and setting
record.status/updated_at), then await pg_store.persist_payout(db_record) and
finally update the in-memory cache _payout_store inside the same _lock so the
cache reflects the persisted state; reference symbols: _lock, _payout_store,
payout_id, pg_store.load_payouts, persist_payout, PayoutStatus, updated_at.
In `@backend/app/services/pg_store.py`:
- Around line 277-281: The save_last_sync function currently always inserts a
new SyncStateTable row (session.add(SyncStateTable(last_sync=dt))) causing
unbounded growth; change it to perform an upsert/update instead: locate the
existing SyncStateTable row (by its single known key/unique constraint or
primary id) and update its last_sync value, or use your ORM's upsert support
(e.g., insert().on_conflict_do_update or session.merge) to atomically set
last_sync = dt, then commit; update the code paths inside save_last_sync and
keep using get_db_session() for the transaction.
- Around line 224-226: The code currently constructs
PayoutStatus(row.status.lower()) which will raise ValueError for unexpected
values; wrap that conversion in a try/except ValueError, log the invalid status
along with identifying fields from the row (e.g., row.tx_hash or row.id), and
assign a safe fallback (use PayoutStatus.UNKNOWN if that enum member exists,
otherwise assign None or a designated default) so the load operation doesn't
crash; also consider adding a note to validate/normalize status values when
persisting.
- Around line 16-19: The helper _to_uuid currently swallows
ValueError/AttributeError and returns the original value on failure; change it
to surface conversion failures by raising a clear exception (e.g., ValueError
with the offending value and context) instead of returning the original, or at
minimum log a warning before returning; update the except block in _to_uuid to
raise a descriptive ValueError (or call the logger) so callers know the UUID
conversion failed.
---
Outside diff comments:
In `@backend/app/api/bounties.py`:
- Around line 210-214: Change the handler parameter deadline_before from
Optional[str] to Optional[datetime] so FastAPI performs ISO datetime validation
at the API boundary (update the handler signature for the endpoint in
bounties.py and import datetime), and add a field validator for skills_logic
inside BountySearchParams (the same pattern used for sort/category) that only
allows "any" or "all" and raises ValueError for other values so invalid
skills_logic values are rejected at input rather than silently treated as OR in
bounty_search_service.py.
In `@backend/app/api/payouts.py`:
- Around line 107-144: The financial endpoints (record_payout, record_buyback,
admin_approve_payout, execute_payout) are publicly callable; add an
authentication/authorization dependency that enforces admin privileges before
executing mutations. For each function (record_payout, record_buyback,
admin_approve_payout, execute_payout) add a FastAPI dependency parameter (e.g.,
current_user: User = Depends(get_current_user) or admin: User =
Depends(require_admin)) and either call a shared require_admin() dependency or
explicitly check current_user.is_admin and raise HTTPException(403) on failure;
keep existing error handling and invalidate_cache() calls intact so only
authenticated admins can perform these treasury-changing operations.
In `@backend/app/services/pg_store.py`:
- Around line 232-246: persist_buyback currently calls _upsert which can raise
an IntegrityError if BuybackTable.tx_hash uniqueness is violated; wrap the
_upsert + session.commit() in a try/except that catches
sqlalchemy.exc.IntegrityError, perform session.rollback() on error, and handle
the conflict deterministically (e.g., query BuybackTable by tx_hash to
skip/update the existing row or log and return) so the IntegrityError is not
unhandled; reference persist_buyback, _upsert, and BuybackTable.tx_hash when
implementing the catch and conflict-resolution logic.
- Around line 189-208: persist_payout calls _upsert which can trigger a unique
constraint violation on PayoutTable.tx_hash; wrap the _upsert/commit in a
try/except that catches sqlalchemy.exc.IntegrityError, and inside the except
check whether the error is a tx_hash uniqueness conflict (use the exception
message or inspect orig) and handle it gracefully—either look up the existing
PayoutTable row by tx_hash and update that row instead of inserting (using the
session and _to_uuid/record.id logic) or raise a controlled application error;
ensure you import IntegrityError from sqlalchemy.exc and maintain session
rollback/cleanup when catching the exception so the DB session isn’t left in a
bad state.
- Around line 283-302: The load_reputation function loads all
ReputationHistoryTable rows into memory which can OOM; change it to iterate in
paginated/batched streaming mode using the DB session instead of loading all
scalars at once. Inside load_reputation (and within the async with
get_db_session() block) replace the single await
session.execute(stmt)).scalars() call with a batched fetch/streaming loop (e.g.,
use session.stream/select with execution_options or explicit LIMIT/OFFSET or
cursor-based paging) that yields rows in chunks and processes each chunk into
ReputationHistoryEntry before appending to out; keep the same field mappings
(entry_id, contributor_id, bounty_id, bounty_title, bounty_tier, review_score,
earned_reputation, anti_farming_applied, created_at) and preserve the order_by
ReputationHistoryTable.created_at.desc() while ensuring the loop stops when no
more rows are returned.
- Around line 21-30: The _upsert function uses a non-atomic get-then-add pattern
that can race on concurrent calls; replace it with a true DB-level upsert by
using SQLAlchemy Core's insert().on_conflict_do_update() against the model class
table: convert pk_value via _to_uuid, build an insert statement from
model_cls.__table__ (include id=pk_value and the columns), call
on_conflict_do_update with the primary key/index (e.g.,
index_elements=[model_cls.id] or the actual PK column) and set_ to the incoming
columns, then execute the statement with await session.execute(...) (and
flush/commit as currently expected) instead of session.get/session.add to avoid
duplicate-insert errors.
In `@backend/tests/test_health.py`:
- Around line 57-69: The test in backend/tests/test_health.py currently
hardcodes service statuses (data["services"]["database"] == "connected" and
data["services"]["redis"] == "connected") and misses new probes and
mission-timeout behavior; update the assertions to validate the health contract
per spec by: checking data["services"] exists and contains keys for "database",
"redis", "solana", and "github"; assert each service value is one of the allowed
status tokens (e.g., "connected", "disconnected", "degraded", "unknown") rather
than exact strings; keep the metadata checks (version, uptime_seconds >= 0,
timestamp endswith "Z"); and add an assertion for the Mission `#80` timeout
indicator (e.g., presence and boolean status like data.get("mission_timeout") or
the spec-defined key) so the test will correctly accept upgraded contract
responses while still validating required fields.
---
Duplicate comments:
In `@backend/app/main.py`:
- Around line 128-134: The CORS setup in app.add_middleware using CORSMiddleware
is invalid because allow_credentials=True cannot be used with wildcard origins;
update the configuration for CORSMiddleware by replacing allow_origins=["*"]
with an explicit list of allowed origins (or use allow_origin_regex with a safe
regex), or set allow_credentials=False if wildcards are required; keep
allow_methods and allow_headers as needed but ensure allow_origins is a concrete
list (or regex) whenever allow_credentials=True so credentialed requests are not
blocked by browsers.
In `@backend/app/middleware/security.py`:
- Around line 35-43: Update the global CSP constants in middleware/security.py
so the default CSP_SCRIPT_SRC and CSP_STYLE_SRC include the CDN hosts used by
FastAPI docs (e.g., cdn.jsdelivr.net, unpkg.com, and cdnjs.cloudflare.com and
keep fonts.googleapis.com/font sources) but remove the global "'unsafe-inline'
'unsafe-eval'" allowances; then add dedicated CSP_DOCS_SCRIPT_SRC and
CSP_DOCS_STYLE_SRC (or similar) that include the CDN hosts plus the relaxed
"'unsafe-inline' 'unsafe-eval'" only for the docs/static endpoints, and update
the docs/static response middleware or route that sets the
Content-Security-Policy header to use these docs-specific variables while
leaving CSP_SCRIPT_SRC and CSP_STYLE_SRC strict for all other routes (reference
CSP_SCRIPT_SRC, CSP_STYLE_SRC, CSP_DOCS_SCRIPT_SRC/CSP_DOCS_STYLE_SRC and the
middleware/security.py header-setting logic).
In `@backend/app/models/bounty.py`:
- Around line 236-240: Bounty models accept unbounded project_name while the DB
column is String(100); update the Field definition(s) named project_name in both
BountyBase and BountyUpdate to enforce max length 100 (e.g., Field(...,
max_length=100) or use pydantic.constr(max_length=100)) so validation fails
early and matches backend/app/models/bounty_table.py's String(100).
- Around line 272-277: Several Bounty-related models (BountyUpdate,
BountyListItem, BountySearchResult, BountySearchParams) still inherit directly
from BaseModel and thus do not pick up the camelCase aliasing defined in the
shared model_config; update each of those classes to inherit from BountyBase
(the class that defines model_config with populate_by_name and alias_generator)
so they automatically use camelCase aliases, or alternatively copy the same
model_config into each class if inheritance is not possible; update
BountyUpdate, BountyListItem, BountySearchResult, and BountySearchParams
accordingly so fields like project_name, deadline_before, and skills_logic are
exposed as camelCase to the frontend.
In `@backend/app/services/payout_service.py`:
- Around line 63-75: Remove the fragile in-memory and limited pre-scan duplicate
checks (the loop over _payout_store under _lock and the partial
pg_store.load_payouts scan) and instead enforce uniqueness at the DB level by
adding a UNIQUE constraint on PayoutTable.bounty_id (for non-null values); then
make persist calls use the DB as source-of-truth by wrapping
pg_store.persist_payout in a try/except that catches the DB IntegrityError (or
the specific SQLAlchemy/asyncpg integrity exception used) and raises
HTTPException(status_code=400, detail="Bounty already has a payout" or "Payout
with tx_hash already exists" as appropriate); keep references to data.tx_hash
and data.bounty_id but remove the pre-insert duplicate scans so concurrent
requests rely on the DB constraint and the IntegrityError-to-HTTPException
conversion.
- Around line 142-145: get_payout_by_id and get_payout_by_tx_hash currently call
pg_store.load_payouts(limit=...) and filter in Python which truncates results
and yields false 404s; change get_payout_by_id to load the single payout by
primary key via the DB session (use session.get(Payout, payout_id) or the
equivalent store method) and change get_payout_by_tx_hash to run a direct
indexed query (e.g.
session.query(Payout).filter_by(tx_hash=tx_hash).one_or_none() or the equivalent
store method) instead of iterating over pg_store.load_payouts; return
_payout_to_response(...) when a row is found and preserve the None return when
not found.
- Around line 243-259: The duplicate-check using load_buybacks(limit=1000) is
insufficient; wrap the call to pg_store.persist_buyback(record) in a try/except
that catches the DB unique-constraint/IntegrityError (or the specific DB driver
unique-violation exception) and re-raise HTTPException(status_code=400,
detail="Buyback already exists"); only add the record to _buyback_store under
_lock after persist_buyback succeeds (so use BuybackRecord,
pg_store.persist_buyback, BuybackTable.tx_hash, _buyback_store, and _lock to
locate and update the code).
In `@backend/requirements.txt`:
- Around line 27-29: The three new dependency lines for anthropic, openai, and
psutil in backend/requirements.txt lack upper bounds and must be changed to
match the project's bounded-range style; update each entry (anthropic, openai,
psutil) to use a capped range like ">=current_min,<next_major" (or
"<=latest_patch" if the file uses minor/patch caps) so upgrades cannot jump
major versions silently—identify the current minimum versions you added and set
the corresponding upper-bound cap consistent with other entries in
requirements.txt, then run the same PyPI/version check used in the review to
verify the chosen caps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7fc91ab-c701-406d-bcb1-b171db510d65
📒 Files selected for processing (11)
backend/app/api/bounties.pybackend/app/api/payouts.pybackend/app/main.pybackend/app/middleware/security.pybackend/app/models/bounty.pybackend/app/models/bounty_table.pybackend/app/services/bounty_search_service.pybackend/app/services/payout_service.pybackend/app/services/pg_store.pybackend/requirements.txtbackend/tests/test_health.py
backend/app/api/payouts.py
Outdated
| @router.post("/validate-wallet", response_model=WalletValidationResponse) | ||
| async def validate_wallet(body: WalletValidationRequest) -> WalletValidationResponse: | ||
| """Check base-58 format and reject known program addresses. | ||
|
|
||
| Returns a structured response indicating whether the address is | ||
| valid for receiving payouts. | ||
| """ | ||
| address = body.wallet_address | ||
| is_program = address in KNOWN_PROGRAM_ADDRESSES | ||
| try: | ||
| validate_solana_wallet(address) | ||
| return WalletValidationResponse( | ||
| wallet_address=address, | ||
| valid=True, | ||
| message="Valid Solana wallet address", | ||
| ) | ||
| return WalletValidationResponse(wallet_address=address, valid=True, message="Valid Solana wallet address") | ||
| except ValueError as exc: | ||
| return WalletValidationResponse( | ||
| wallet_address=address, | ||
| valid=False, | ||
| is_program_address=is_program, | ||
| message=str(exc), | ||
| ) | ||
| return WalletValidationResponse(wallet_address=address, valid=False, is_program_address=is_program, message=str(exc)) |
There was a problem hiding this comment.
Valid program accounts lose the is_program_address flag.
validate_wallet() computes is_program, but the success response drops it. A known program address that passes base58 validation comes back as an ordinary valid wallet, so downstream callers lose the signal they need to block non-user payout targets.
As per coding guidelines, backend/**: "Analyze thoroughly: API contract consistency with spec".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/payouts.py` around lines 155 - 163, The success branch of
validate_wallet currently omits the computed is_program flag so known program
addresses are returned as ordinary valid wallets; update the success return in
validate_wallet (and ensure both branches use the same response shape) to
include is_program_address=is_program in the WalletValidationResponse so callers
can detect program accounts; reference validate_wallet and
WalletValidationResponse to locate and modify the response construction.
| app.add_middleware(LoggingMiddleware) | ||
|
|
||
| # Layer 4: Input sanitization — blocks XSS and SQL injection patterns | ||
| app.add_middleware(InputSanitizationMiddleware) | ||
|
|
||
| # Layer 3: Redis-backed token bucket rate limiter (upstream) | ||
| app.add_middleware(RateLimiterMiddleware) | ||
|
|
||
| # Layer 2: IP blocklist — blocks banned IPs via Redis set | ||
| app.add_middleware(RateLimitMiddleware) | ||
| app.add_middleware(IPBlocklistMiddleware) | ||
|
|
||
| # Layer 1 (outermost): Security headers — HSTS, CSP, X-Frame-Options, etc. | ||
| app.add_middleware(SecurityHeadersMiddleware) | ||
|
|
||
| @app.middleware("http") | ||
| async def add_request_id_and_timing(request: Request, call_next: Callable): | ||
| request_id = request.headers.get("X-Request-ID", str(uuid.uuid4())) | ||
| request.state.request_id = request_id | ||
|
|
||
| start_time = time.time() | ||
| response = await call_next(request) | ||
| process_time = time.time() - start_time | ||
|
|
||
| response.headers["X-Request-ID"] = request_id | ||
| response.headers["X-Process-Time"] = str(process_time) | ||
|
|
||
| monitor.track_request( | ||
| path=request.url.path, | ||
| method=request.method, | ||
| status_code=response.status_code, | ||
| duration=process_time | ||
| ) | ||
| return response |
There was a problem hiding this comment.
Two middlewares now mint independent request IDs.
LoggingMiddleware already sets request.state.request_id and X-Request-ID, and the new add_request_id_and_timing middleware generates another value from the incoming header instead of reusing the first one. That makes correlation order-dependent and can leave logs, error payloads, and response headers referring to different IDs for the same request.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/main.py` around lines 136 - 161, The add_request_id_and_timing
middleware must reuse any request ID already set by prior middleware
(LoggingMiddleware) instead of minting a new one; modify
add_request_id_and_timing to first check request.state.request_id and use it if
present, otherwise fall back to request.headers.get("X-Request-ID") or generate
uuid.uuid4(), then assign that value to request.state.request_id and set
response.headers["X-Request-ID"] (but only if not already set) so
LoggingMiddleware, add_request_id_and_timing, monitor.track_request, and any
error payloads all reference the same ID.
| app.add_middleware(RateLimiterMiddleware) | ||
|
|
||
| # Layer 2: IP blocklist — blocks banned IPs via Redis set | ||
| app.add_middleware(RateLimitMiddleware) | ||
| app.add_middleware(IPBlocklistMiddleware) |
There was a problem hiding this comment.
/api/v2/health is now behind the blocking middlewares.
The route moved to /api/v2, but backend/app/middleware/ip_blocklist.py, backend/app/middleware/rate_limit.py, and backend/app/middleware/rate_limiter.py still only exempt /health or /api/health. Health probes can now be 403/429'd, and because both rate-limit middlewares are mounted here the failure mode is doubled.
As per coding guidelines, backend/**: "Analyze thoroughly: API contract consistency with spec".
Also applies to: 205-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/main.py` around lines 138 - 140, The health-check endpoint moved
to /api/v2/health but the middleware exemptions in the IPBlocklistMiddleware,
RateLimitMiddleware, and RateLimiterMiddleware still only whitelist /health or
/api/health, causing probes to be blocked; update each middleware's exemption
logic (in the classes IPBlocklistMiddleware, RateLimitMiddleware, and
RateLimiterMiddleware) to also treat "/api/v2/health" as a permitted path (and
preserve the existing /health and /api/health checks), ensuring the middlewares
skip blocking/rate-limiting for that exact route so health probes are not
403/429'd.
| app.include_router(auth_router, prefix="/api", tags=["auth"]) | ||
| app.include_router(bounties_router, prefix="/api", tags=["bounties"]) | ||
| app.include_router(notifications_router, prefix="/api", tags=["notifications"]) | ||
| app.include_router(leaderboard_router, prefix="/api", tags=["leaderboard"]) | ||
| app.include_router(payouts_router, prefix="/api", tags=["payouts"]) | ||
| app.include_router(buybacks.router, prefix="/api/buybacks", tags=["treasury"]) | ||
| app.include_router(github_webhook_router, prefix="/api/webhooks", tags=["webhooks"]) | ||
|
|
||
| # WebSocket: /ws/* | ||
| app.include_router(websocket_router) | ||
|
|
||
| # Agents: /api/agents/* | ||
| app.include_router(agents_router, prefix="/api") | ||
|
|
||
| # Disputes: /api/disputes/* | ||
| app.include_router(disputes_router, prefix="/api") | ||
|
|
||
| # Escrow: /api/escrow/* | ||
| app.include_router(escrow_router, prefix="/api") | ||
|
|
||
| # Stats: /api/stats (public endpoint) | ||
| app.include_router(stats_router, prefix="/api") | ||
|
|
||
| # Open Graph previews: /og/* | ||
| app.include_router(og_router) | ||
| app.include_router(websocket_router, tags=["websocket"]) | ||
| app.include_router(agents_router, prefix="/api", tags=["agents"]) | ||
| app.include_router(disputes_router, prefix="/api", tags=["disputes"]) | ||
| app.include_router(escrow_router, prefix="/api", tags=["escrow"]) | ||
| app.include_router(stats_router, prefix="/api", tags=["stats"]) | ||
| app.include_router(og_router, tags=["og"]) | ||
| app.include_router(contributor_webhooks_router, prefix="/api") | ||
| app.include_router(siws_router, prefix="/api") | ||
|
|
||
| # System Health: /health, Prometheus: /metrics | ||
| app.include_router(health_router) | ||
| app.include_router(metrics_router) | ||
|
|
||
| # Admin Dashboard: /api/admin/* (protected by ADMIN_API_KEY) | ||
| app.include_router(admin_router) | ||
|
|
||
|
|
||
| @app.post("/api/sync", tags=["admin"]) | ||
| async def trigger_sync(): | ||
| """Manually trigger a GitHub to bounty and leaderboard sync. | ||
|
|
||
| This endpoint should be protected by admin authentication in production. | ||
| It forces an immediate resync of all bounty and contributor data from | ||
| the GitHub Issues API. | ||
|
|
||
| Returns: | ||
| dict: Sync results including counts of updated bounties and contributors. | ||
| """ | ||
| result = await sync_all() | ||
| return result | ||
| app.include_router(health_router, prefix="/api/v2", tags=["system"]) | ||
| app.include_router(metrics_router, tags=["system"]) | ||
| app.include_router(admin_router, tags=["admin"]) | ||
|
|
There was a problem hiding this comment.
The contributor API is no longer registered.
contributors_router is imported at Line 32, but the new registration block never includes it. That removes those endpoints from the running app even though the module still ships.
As per coding guidelines, backend/**: "Analyze thoroughly: API contract consistency with spec".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/main.py` around lines 190 - 208, The contributors_router
endpoints were never included in the app registration block; add
app.include_router(contributors_router, prefix="/api", tags=["contributors"]) to
the router registration sequence in main.py (near the other app.include_router
calls such as auth_router, bounties_router, and contributor_webhooks_router) so
the imported contributors_router is actually mounted and its endpoints become
available at the /api prefix.
| async def hydrate_from_database() -> None: | ||
| """Startup hydration from PostgreSQL into memory caches.""" | ||
| try: | ||
| payouts = await pg_store.load_payouts(limit=5000) | ||
| buybacks = await pg_store.load_buybacks(limit=1000) | ||
| with _lock: | ||
| _payout_store.update(payouts) | ||
| _buyback_store.update(buybacks) | ||
| log.info("Hydrated %d payouts and %d buybacks from PostgreSQL", len(payouts), len(buybacks)) | ||
| except Exception as e: | ||
| log.error("Hydration failed: %s", e) |
There was a problem hiding this comment.
Startup hydration uses fixed limits, potentially missing data.
Lines 275-276 hydrate only 5000 payouts and 1000 buybacks. If the database contains more records, the in-memory cache will be incomplete from startup, causing inconsistent behavior between cache hits and DB fallbacks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/payout_service.py` around lines 272 - 282, The startup
hydration in hydrate_from_database currently uses hardcoded limits
(pg_store.load_payouts(limit=5000), load_buybacks(limit=1000)) which can miss
records; change it to fetch all rows by paginating or streaming instead of a
single fixed-limit call: replace the single calls with a loop that repeatedly
calls pg_store.load_payouts(batch_size, offset/last_id) and
pg_store.load_buybacks(batch_size, offset/last_id) until no more rows are
returned, updating _payout_store and _buyback_store inside the existing _lock on
each batch, keep the final log.info to report total counts, and preserve the
try/except and log.error behavior in hydrate_from_database.
backend/app/services/pg_store.py
Outdated
| def _to_uuid(val: Any) -> Any: | ||
| """Coerce a string value to uuid.UUID for ORM lookups on UUID PK columns. | ||
|
|
||
| Args: | ||
| val: The value to coerce, typically a string UUID. | ||
|
|
||
| Returns: | ||
| A uuid.UUID instance if conversion succeeds, otherwise the original value. | ||
| """ | ||
| if isinstance(val, _uuid.UUID): | ||
| return val | ||
| try: | ||
| return _uuid.UUID(str(val)) | ||
| except (ValueError, AttributeError): | ||
| return val | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Generic helpers | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| async def _upsert( | ||
| session: AsyncSession, model_cls: type, pk_value: Any, **columns: Any | ||
| ) -> None: | ||
| """Insert or update a row using merge (session-level upsert). | ||
| if isinstance(val, _uuid.UUID): return val | ||
| try: return _uuid.UUID(str(val)) | ||
| except (ValueError, AttributeError): return val |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
_to_uuid silently returns invalid input on conversion failure.
Lines 18-19 catch ValueError/AttributeError and return the original value unchanged. If an invalid UUID string is passed, subsequent database operations expecting a UUID type will fail with confusing errors deeper in the stack, or worse, cause type mismatches.
Consider raising an explicit error or logging a warning when conversion fails for values that are expected to be UUIDs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/pg_store.py` around lines 16 - 19, The helper _to_uuid
currently swallows ValueError/AttributeError and returns the original value on
failure; change it to surface conversion failures by raising a clear exception
(e.g., ValueError with the offending value and context) instead of returning the
original, or at minimum log a warning before returning; update the except block
in _to_uuid to raise a descriptive ValueError (or call the logger) so callers
know the UUID conversion failed.
| bounty_title=row.bounty_title, | ||
| tx_hash=row.tx_hash, | ||
| status=PayoutStatus(row.status), | ||
| solscan_url=row.solscan_url, | ||
| created_at=row.created_at, | ||
| ) | ||
| log.info("Loaded %d payouts from PostgreSQL", len(out)) | ||
| return out | ||
|
|
||
|
|
||
| async def load_buybacks(*, offset: int = 0, limit: int = 10000) -> dict[str, Any]: | ||
| """Load buyback records from PostgreSQL into a dict keyed by ID string. | ||
|
|
||
| Args: | ||
| offset: Number of rows to skip. | ||
| limit: Maximum rows to return. | ||
|
|
||
| Returns: | ||
| Dict mapping buyback ID strings to BuybackRecord instances. | ||
| """ | ||
| from app.models.payout import BuybackRecord | ||
| from app.models.tables import BuybackTable | ||
|
|
||
| out: dict[str, Any] = {} | ||
| async with get_db_session() as session: | ||
| stmt = ( | ||
| select(BuybackTable) | ||
| .order_by(BuybackTable.created_at.desc()) | ||
| .offset(offset) | ||
| .limit(limit) | ||
| ) | ||
| for row in (await session.execute(stmt)).scalars(): | ||
| out[str(row.id)] = BuybackRecord( | ||
| id=str(row.id), | ||
| amount_sol=float(row.amount_sol), | ||
| amount_fndry=float(row.amount_fndry), | ||
| price_per_fndry=float(row.price_per_fndry), | ||
| tx_hash=row.tx_hash, | ||
| status=PayoutStatus(row.status.lower()), |
There was a problem hiding this comment.
Status parsing assumes lowercase format.
Line 226 calls PayoutStatus(row.status.lower()). If the database contains an unexpected status value (e.g., mixed case not matching enum values after lowercasing, or a completely invalid value), this will raise a ValueError and crash the entire load operation.
Consider wrapping in a try/except with logging for invalid statuses, or validating/normalizing status values during persistence.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/pg_store.py` around lines 224 - 226, The code currently
constructs PayoutStatus(row.status.lower()) which will raise ValueError for
unexpected values; wrap that conversion in a try/except ValueError, log the
invalid status along with identifying fields from the row (e.g., row.tx_hash or
row.id), and assign a safe fallback (use PayoutStatus.UNKNOWN if that enum
member exists, otherwise assign None or a designated default) so the load
operation doesn't crash; also consider adding a note to validate/normalize
status values when persisting.
| async def save_last_sync(dt: datetime) -> None: | ||
| from app.models.tables import SyncStateTable | ||
| async with get_db_session() as session: | ||
| await _insert_if_absent( | ||
| session, | ||
| ReputationHistoryTable, | ||
| entry.entry_id, | ||
| contributor_id=entry.contributor_id, | ||
| bounty_id=entry.bounty_id, | ||
| bounty_title=entry.bounty_title, | ||
| bounty_tier=entry.bounty_tier, | ||
| review_score=entry.review_score, | ||
| earned_reputation=entry.earned_reputation, | ||
| anti_farming_applied=entry.anti_farming_applied, | ||
| created_at=entry.created_at, | ||
| ) | ||
| session.add(SyncStateTable(last_sync=dt)) | ||
| await session.commit() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
save_last_sync always inserts a new row.
Line 280 unconditionally adds a new SyncStateTable row on every call. Over time, this creates unbounded growth in the sync_state table. Consider using upsert with a single known key, or deleting old rows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/pg_store.py` around lines 277 - 281, The save_last_sync
function currently always inserts a new SyncStateTable row
(session.add(SyncStateTable(last_sync=dt))) causing unbounded growth; change it
to perform an upsert/update instead: locate the existing SyncStateTable row (by
its single known key/unique constraint or primary id) and update its last_sync
value, or use your ORM's upsert support (e.g., insert().on_conflict_do_update or
session.merge) to atomically set last_sync = dt, then commit; update the code
paths inside save_last_sync and keep using get_db_session() for the transaction.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/e2e/conftest.py (1)
38-48:⚠️ Potential issue | 🟡 MinorDuplicate comment block.
The section header "Test user for dependency override" appears twice (lines 38-41 and 46-48).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/e2e/conftest.py` around lines 38 - 48, Remove the duplicated section header comment block labeled "Test user for dependency override" so the file only contains a single header; locate the two identical comment blocks surrounding the imports (near the imports of UserResponse and DEFAULT_WALLET) and delete the redundant one, keeping the single intended header and the imports intact (references: UserResponse import and DEFAULT_WALLET import).
♻️ Duplicate comments (18)
backend/app/api/buybacks.py (2)
9-12:⚠️ Potential issue | 🔴 Critical
record_buybackis still unauthenticated for a treasury-mutating operation.Line 10 exposes a state-changing treasury endpoint without any authentication/authorization dependency, so any caller can submit buyback events.
As per coding guidelines
backend/**: Python FastAPI backend. Analyze thoroughly: Authentication/authorization gaps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/buybacks.py` around lines 9 - 12, The endpoint record_buyback currently allows unauthenticated callers to mutate treasury state; require and enforce authentication/authorization by adding a FastAPI dependency to the function signature (e.g., add a parameter like current_user: User = Depends(get_current_user) or current_admin: User = Depends(require_admin) depending on your auth utilities) and validate admin/treasury scope before calling payout_service.create_buyback; update the router declaration/signature in backend/app/api/buybacks.py (record_buyback) to use the appropriate Depends helper (get_current_user, require_admin, or require_scope) and return 403 on insufficient privileges.
15-17:⚠️ Potential issue | 🟠 MajorPagination parameters remain unbounded at the API layer.
Line 15 accepts unconstrained
skipandlimit. Extremely large values can be used to force excessive DB/data processing.As per coding guidelines
backend/**: Python FastAPI backend. Analyze thoroughly: Input validation and SQL injection vectors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/buybacks.py` around lines 15 - 17, The list_buybacks endpoint accepts unconstrained skip and limit which allows very large values; validate and clamp these inputs in the API layer (function list_buybacks) before calling payout_service.list_buybacks: ensure skip is an integer >= 0 and limit is an integer within an allowed maximum (e.g. <= 1000) and a sensible default (e.g. 100), returning a 400 on invalid input or silently clamping values; implement this using FastAPI Query parameter constraints or explicit runtime checks and pass only the sanitized values to payout_service.list_buybacks to prevent excessive DB load and potential abuse.backend/app/middleware/rate_limit.py (2)
59-64:⚠️ Potential issue | 🔴 CriticalClient identity is still spoofable via
X-Forwarded-For.Lines 61-64 trust the first XFF hop directly, allowing callers to rotate identities and bypass bucket enforcement.
As per coding guidelines
backend/**: Python FastAPI backend. Analyze thoroughly: Authentication/authorization gaps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/middleware/rate_limit.py` around lines 59 - 64, The current _get_client_id trusts the first value in the X-Forwarded-For header which is easily spoofed; change it to only honor X-Forwarded-For when the immediate peer (request.client.host) is a configured trusted proxy, otherwise ignore XFF and use request.client.host; when honoring XFF, take the right-most untrusted IP (or the left-most if your infra documents it) from XFF consistently and document/configure trusted proxies via a TRUSTED_PROXIES config/env used by _get_client_id to validate request.client.host before parsing XFF.
94-102:⚠️ Potential issue | 🟠 MajorRate-limit response headers do not match enforced bucket semantics.
Lines 95 and 101 always advertise limit
2while the bucket is configured withburst=10and refill behavior from Line 66;Retry-Afteris also hardcoded, so clients receive inconsistent throttling metadata.As per coding guidelines
backend/**: Python FastAPI backend. Analyze thoroughly: API contract consistency with spec.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/middleware/rate_limit.py` around lines 94 - 102, The response headers in the rate-limiting middleware are hardcoded and inconsistent with the token bucket config: replace the fixed "2" and hardcoded "Retry-After" with values derived from the actual bucket settings (use the configured burst capacity and current token count) and compute Retry-After from the token refill/reset time; specifically, in the code paths around call_next(request) where response.headers and remaining are set, set "X-RateLimit-Limit" to the bucket's burst capacity, "X-RateLimit-Remaining" to the current remaining tokens (the existing remaining variable), and set "Retry-After" to the calculated seconds until a token becomes available based on the refill/reset logic used earlier (referencing the burst and refill/reset variables used in the token bucket implementation).backend/app/services/health.py (1)
34-56:⚠️ Potential issue | 🟠 MajorRequest tracking still has TOCTOU and unbounded cardinality risks.
Line 36 reads
_runningoutside the lock, while updates happen under lock, which permits racey accounting. Also Line 42 stores raw path keys without bounds/normalization, so high-cardinality paths can grow memory unbounded and degrade Line 55 sorting cost.As per coding guidelines
backend/**: Python FastAPI backend. Analyze thoroughly: Error handling and edge case coverage.backend/logs/application.log.2026-03-21 (1)
1-15:⚠️ Potential issue | 🟠 MajorThis is another committed runtime log artifact with request metadata.
Lines 1-15 include request/trace telemetry (request IDs, paths, client IP, timings) that should not live in version-controlled source.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/logs/application.log.2026-03-21` around lines 1 - 15, The committed runtime log file application.log.2026-03-21 contains request/trace telemetry and must be removed from the repo; remove it from version control (git rm --cached application.log.2026-03-21 and commit), add a rule to .gitignore to prevent re-adding logs, and if any sensitive data may have been exposed rewrite history using git filter-repo or BFG (or coordinate with the security team) to purge the file from prior commits; finally push the cleaned branch and notify reviewers that the log file was removed and .gitignore updated.backend/app/main.py (4)
190-207:⚠️ Potential issue | 🟠 Major
contributors_routeris imported but never registered.Line 32 imports
contributors_routerfromapp.api.bounties, but the router registration block (lines 190-207) does not include it. This removes contributor endpoints from the API.app.include_router(auth_router, prefix="/api", tags=["auth"]) app.include_router(bounties_router, prefix="/api", tags=["bounties"]) +app.include_router(contributors_router, prefix="/api", tags=["contributors"]) app.include_router(notifications_router, prefix="/api", tags=["notifications"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/main.py` around lines 190 - 207, The contributors_router imported as contributors_router (from app.api.bounties) is not registered with the FastAPI app; add a registration line in the router block using app.include_router(contributors_router, prefix="/api", tags=["contributors"]) (place it alongside the other app.include_router calls near auth_router/bounties_router) so contributor endpoints are exposed.
143-161:⚠️ Potential issue | 🟠 MajorDuplicate request ID generation with LoggingMiddleware.
Both
LoggingMiddleware(seebackend/app/middleware/logging_middleware.pylines 14-22) andadd_request_id_and_timinggenerate and setX-Request-ID. The middleware execution order meansLoggingMiddlewareruns first (added earlier), setsrequest.state.request_id, thenadd_request_id_and_timingoverwrites it with a new value from the header or a fresh UUID, causing log correlation mismatches.Consolidate request ID handling into one location:
`@app.middleware`("http") async def add_request_id_and_timing(request: Request, call_next: Callable): - request_id = request.headers.get("X-Request-ID", str(uuid.uuid4())) + # Reuse ID from LoggingMiddleware if already set + request_id = getattr(request.state, "request_id", None) or request.headers.get("X-Request-ID") or str(uuid.uuid4()) request.state.request_id = request_id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/main.py` around lines 143 - 161, add_request_id_and_timing is overwriting the request ID that LoggingMiddleware already sets; update add_request_id_and_timing to respect an existing request.state.request_id (use that if present) and only fall back to request.headers.get("X-Request-ID") or a new uuid when request.state.request_id is missing, then set response.headers["X-Request-ID"] to that chosen ID and use it in monitor.track_request; this consolidates request ID handling so LoggingMiddleware remains the single source of truth and prevents log correlation issues.
205-206:⚠️ Potential issue | 🟠 MajorHealth endpoint at
/api/v2/healthis not exempt from rate limiting.The health router is mounted at
/api/v2(line 205), making the health check available at/api/v2/health. However,RateLimitMiddlewareandRateLimiterMiddlewareonly exempt/healthand/api/healthpaths. Health probes from load balancers and orchestrators may be rate-limited or blocked.#!/bin/bash # Check rate limiter exempt paths rg -n "EXEMPT_PATHS|/health" backend/app/middleware/rate_limit*.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/main.py` around lines 205 - 206, The health endpoint is mounted via health_router at the "/api/v2" prefix (health accessible at "/api/v2/health") but the rate limiters only exempt "/health" and "/api/health"; update the rate limiting exemption logic by adding "/api/v2/health" (or better: canonicalize and include any router prefix + "/health") to the EXEMPT_PATHS used by RateLimitMiddleware and RateLimiterMiddleware, or change their path check to allow any path that endswith "/health" (or startswith any registered health prefix) so requests to health_router (health_router, RateLimitMiddleware, RateLimiterMiddleware, EXEMPT_PATHS) are not rate limited.
128-134:⚠️ Potential issue | 🟠 MajorCORS configuration violates Starlette/FastAPI specification.
Setting
allow_credentials=Truewithallow_origins=["*"],allow_methods=["*"], andallow_headers=["*"]is explicitly prohibited by Starlette'sCORSMiddleware. Per official documentation, whenallow_credentials=True, none of these can use wildcards. This will cause authenticated cross-origin requests to fail.app.add_middleware( CORSMiddleware, - allow_origins=["*"], - allow_credentials=True, - allow_methods=["*"], - allow_headers=["*"], + allow_origins=["https://solfoundry.org", "http://localhost:3000"], # Explicit origins + allow_credentials=True, + allow_methods=["GET", "POST", "PUT", "PATCH", "DELETE", "OPTIONS"], + allow_headers=["Authorization", "Content-Type", "X-Request-ID"], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/main.py` around lines 128 - 134, The CORSMiddleware configuration in app.add_middleware using CORSMiddleware sets allow_credentials=True while using wildcards for origins/methods/headers, which violates Starlette/FastAPI rules; either set allow_credentials=False if you need global wildcards, or replace the wildcard origins/methods/headers with an explicit allow_origins list (and explicit allow_methods/allow_headers if required) so authenticated requests succeed. Update the CORSMiddleware call in the app.add_middleware block: choose one of two fixes—(a) set allow_credentials=False to keep allow_origins=["*"], allow_methods=["*"], allow_headers=["*"], or (b) provide a concrete list for allow_origins (and non-wildcard allow_methods/allow_headers if desired) while leaving allow_credentials=True—so the middleware conforms to Starlette/FastAPI expectations.backend/app/api/payouts.py (1)
154-162:⚠️ Potential issue | 🟠 MajorValid program accounts still lose the
is_program_addressflag on success.The success path at line 160 omits
is_program_addressfrom the response, while the failure path at line 162 includes it. A known program address that passes base58 validation returnsis_program_address=False(the default) instead ofTrue, causing downstream callers to lose the signal needed to block non-user payout targets.- return WalletValidationResponse(wallet_address=address, valid=True, message="Valid Solana wallet address") + return WalletValidationResponse(wallet_address=address, valid=True, is_program_address=is_program, message="Valid Solana wallet address")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/payouts.py` around lines 154 - 162, In validate_wallet, the success path currently omits the is_program_address flag so known program addresses lose that signal; update the WalletValidationResponse returned in the try block inside validate_wallet to include is_program_address=is_program (keeping wallet_address, valid=True and message unchanged) so both success and except paths consistently populate is_program_address; reference validate_wallet, is_program, and WalletValidationResponse to locate and change the return in the try block.backend/app/services/payout_service.py (7)
135-153:⚠️ Potential issue | 🟠 MajorPayout lookups truncate at 5000/10000 records.
get_payout_by_id(line 142) andget_payout_by_tx_hash(line 149) load limited records. Once the table exceeds this size, valid lookups return false 404s.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/payout_service.py` around lines 135 - 153, The lookup functions get_payout_by_id and get_payout_by_tx_hash currently call pg_store.load_payouts with hard limits (limit=5000/10000) which truncates results and causes false negatives; change them to perform targeted DB queries instead of loading a limited batch: for get_payout_by_id call a pg_store.load_payout_by_id(payout_id) (or equivalent single-row query) and for get_payout_by_tx_hash call pg_store.load_payout_by_tx_hash(tx_hash), falling back to the in-memory _payout_store only if those targeted queries return nothing; update the service to rely on these new/single-row pg_store methods (or modify load_payouts to accept no limit/streaming) so lookups are complete.
155-179:⚠️ Potential issue | 🟡 Minorapprove_payout and reject_payout do not update in-memory cache after persist.
After persisting state changes to the database, neither function updates
_payout_store. Subsequent reads from cache return stale status values.await pg_store.persist_payout(record) + with _store_lock: + _payout_store[record.id] = recordAlso applies to: 181-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/payout_service.py` around lines 155 - 179, approve_payout and reject_payout update the DB but never refresh the in-memory cache (_payout_store), causing stale reads; after calling pg_store.persist_payout(record) in both approve_payout and reject_payout, acquire _lock and update _payout_store[payout_id] = record (or set the status and updated_at on the cached object if present) so the in-memory entry reflects the persisted PayoutStatus and timestamp; ensure this uses the same _lock used when reading the cache to avoid races and handle the case where the cache entry didn't exist by inserting the record.
110-133:⚠️ Potential issue | 🟠 MajorIn-memory filtering with
limit + skip + 1000cap produces incorrect totals.Line 110 fetches a bounded set, then line 126 sets
total = len(items)from the filtered subset. For datasets exceeding this threshold, pagination metadata is wrong and records are missing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/payout_service.py` around lines 110 - 133, The current logic calls pg_store.load_payouts(limit=limit + skip + 1000) then computes total = len(items) after doing in-memory filters, which causes incorrect totals and missing records for large datasets; update the implementation so filtering and counting happen at the data source (use pg_store.load_payouts with appropriate filter parameters or add a new pg_store.count_payouts(filters) method) and then fetch only the paginated rows (apply skip/limit at DB) before mapping with _payout_to_response into PayoutListResponse, ensuring total reflects the full count of filtered records and page contains only the requested slice.
261-270:⚠️ Potential issue | 🟠 Majorlist_buybacks reports incorrect total count.
Line 267 sets
total=len(items)from the fetched subset rather than the true database count.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/payout_service.py` around lines 261 - 270, The total returned by list_buybacks is wrong because it uses total=len(items) after paging; change it to compute the true total before slicing (e.g., all_items = list(all_records.values()); total = len(all_items)) or call a dedicated count method (e.g., pg_store.count_buybacks()) and use that value for BuybackListResponse.total; update list_buybacks (function name) to set total to the full count and keep paging logic (page = all_items[skip: skip+limit]) and then return items from the page.
279-284:⚠️ Potential issue | 🟡 Minorget_total_paid_out truncates at 10000 records.
For systems with more than 10000 confirmed payouts, this returns an understated total.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/payout_service.py` around lines 279 - 284, get_total_paid_out currently calls pg_store.load_payouts(limit=10000) which truncates results and undercounts totals; update get_total_paid_out to fetch all payouts (either by removing the hard limit if load_payouts supports unlimited, or implement pagination/streaming using pg_store.load_payouts in a loop with offset/limit until no more rows) and then sum PayoutStatus.CONFIRMED amounts for token == "FNDRY" and token == "SOL" (keep existing variables fndry and sol and the filtering logic). Ensure the implementation avoids loading an unbounded result into memory at once by using a reasonable page size and aggregating per page.
242-259:⚠️ Potential issue | 🟠 MajorBuyback duplicate check has same race/truncation issues.
Line 244 loads only 1000 buybacks. The
UNIQUEconstraint onBuybackTable.tx_hashwill throw an unhandled database error for duplicates outside this window or during races, resulting in 500 errors instead of controlled 400s.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/payout_service.py` around lines 242 - 259, The duplicate-check using pg_store.load_buybacks(limit=1000) in create_buyback is racy and truncated; replace it by either a single DB lookup for the given tx_hash (e.g., add/use pg_store.get_buyback_by_tx_hash(tx_hash) and raise HTTPException(400) if found) or keep a single insert but wrap the call to pg_store.persist_buyback in a try/except that catches the DB unique-constraint error (e.g., IntegrityError/UniqueViolation from your DB driver) and convert it to HTTPException(status_code=400, detail="Buyback already exists"); after successful persist, continue to update _buyback_store under _store_lock and return _buyback_to_response(record).
61-94:⚠️ Potential issue | 🔴 CriticalDouble-pay check remains racy and limited to 5000 records.
The bounty-level duplicate check (lines 71-75) loads only 5000 payouts. This misses older payouts and the check-then-persist sequence is not atomic. Concurrent requests can race past the check. Without a database-level
UNIQUEconstraint onbounty_id, double payments remain possible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/payout_service.py` around lines 61 - 94, create_payout currently does an in-memory/dataset-level double-pay check (load_payouts with limit=5000 and loop) which is racy and incomplete; instead add a DB-level UNIQUE constraint/index on the payouts.bounty_id column and rely on the database for atomic enforcement, update the persistence path to handle that constraint violation: modify the schema/migration to add UNIQUE(bounty_id) (allowing nulls if needed), update pg_store.persist_payout to catch the DB IntegrityError/unique-violation for bounty_id and translate it into HTTPException(status_code=400, detail="Bounty {bounty_id} already has a payout."), and remove or keep but not rely on the current load_payouts/loop check in create_payout so concurrent requests cannot create double payouts; references: create_payout, load_payouts, pg_store.persist_payout, PayoutRecord, bounty_id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/api/payouts.py`:
- Around line 187-216: The execute_payout handler is using unnecessary hasattr
checks for attributes that always exist on PayoutResponse and is referencing a
non‑existent contributor_id; replace the hasattr usages by directly reading
fields from the PayoutResponse returned by process_payout (e.g. use
result.bounty_id, str(result.amount), result.tx_hash) and remove the
contributor_id logic entirely; if you need to fall back to payout_id when
bounty_id is explicitly None, do an explicit None check (bounty_id =
result.bounty_id if result.bounty_id is not None else payout_id) rather than
getattr(..., None) or truthy "or" fallbacks, and call
ContributorWebhookService.dispatch_event with user_id omitted or None.
In `@backend/app/middleware/rate_limit.py`:
- Around line 54-57: The Redis client in RateLimitMiddleware.__init__
initializes self.redis via redis.from_url(redis_url, decode_responses=True)
without socket timeouts; update that call to pass explicit
socket_connect_timeout and socket_timeout (e.g., short connect timeout and
reasonable read timeout) so socket operations fail fast during network
degradation, then retain the existing fail-open error handling around
_lua_script and request processing; apply the same change to other Redis
initializations that use redis.from_url in websocket_manager (WebsocketManager),
core/redis (module-level client factory), api/health (health check client), and
api/admin (admin Redis client) to ensure consistent timeout behavior across the
codebase.
- Around line 82-83: The health-check bypass currently only matches
"/api/health" and misses the registered "/api/v2/health"; update the check in
backend/app/middleware/rate_limit.py (the branch that returns await
call_next(request)) to also allow "/api/v2/health" — better: replace the
exact-path check with a more robust match (e.g.,
request.url.path.endswith("/health") or a contains "/health" rule) so any
prefixed health route passes through; also update the alternate
RateLimiterMiddleware exemption in backend/app/middleware/rate_limiter.py (the
logic around paths starting with "/health") to use the same robust matching
(endswith or wildcard) so both middleware implementations consistently bypass
all health endpoints.
In `@backend/app/middleware/security.py`:
- Around line 35-43: The CSP is currently too permissive globally via
CSP_SCRIPT_SRC and CSP_STYLE_SRC (they include 'unsafe-inline' and
'unsafe-eval') and is applied in the global middleware in security.py; change
the global CSP to remove 'unsafe-inline' and 'unsafe-eval' (tighten
CSP_DEFAULT_SRC/CSP_SCRIPT_SRC/CSP_STYLE_SRC/CSP_IMG_SRC/CSP_CONNECT_SRC/CSP_FONT_SRC)
and then disable FastAPI's default /docs and /redoc routes, creating a custom
/docs handler that injects per-request nonces into inline scripts/styles and
applies a relaxed CSP header only for that route; update the middleware that
sets the response header to use the strict env-based values by default and add
route-level logic to set the relaxed CSP when serving the custom docs endpoint.
In `@backend/app/services/payout_service.py`:
- Around line 65-68: The code in create_payout references an undefined lock
variable `_lock`, causing a NameError; replace `_lock` with the correctly
defined module-level lock `_store_lock` (or create an alias) so the critical
section protecting `_payout_store` uses the right synchronization primitive;
update the block that iterates `_payout_store.values()` (inside create_payout)
to use `_store_lock` to prevent race conditions when checking existing.tx_hash.
In `@backend/app/services/treasury_service.py`:
- Around line 16-20: The module redefines _count_confirmed_payouts and
_count_buybacks as synchronous functions, shadowing the async helpers imported
earlier and causing await _count_confirmed_payouts() / await _count_buybacks()
in get_treasury_stats to fail at runtime; fix by renaming the local synchronous
implementations (e.g., to _count_confirmed_payouts_sync and
_count_buybacks_sync) or otherwise removing the name collision so the async
helpers imported on lines where _count_confirmed_payouts and _count_buybacks are
brought into the module remain the callable symbols used by get_treasury_stats,
and update any internal calls that referenced the local names accordingly.
In `@backend/logs/error.log.2026-03-21`:
- Around line 1-2: This commit accidentally includes runtime error logs (entries
mentioning "RateLimiter", "Blocklist", "Failsafe: ALLOW", and the Starlette
taskName "starlette.middleware.base.BaseHTTPMiddleware.__call__...coro"); remove
that log file from the repo, add the log filename pattern (e.g., error.log.* or
the specific runtime log name) to .gitignore, and revert the file from the
commit history so operational logs are not tracked; additionally, ensure your
logging config writes to a non-source-control sink (rotating file, syslog, or
cloud logs) and add a pre-commit/CI check to prevent committed runtime logs in
the future.
In `@backend/tests/e2e/conftest.py`:
- Around line 85-93: The current _get_test_loop function and module-level
_test_loop global create shared mutable event loop state causing test pollution;
replace this pattern with a pytest fixture that creates a fresh event loop per
test instead of using _test_loop/_get_test_loop. Implement a fixture (e.g.,
async def or regular def) that calls asyncio.new_event_loop(), sets it via
asyncio.set_event_loop(loop), yields the loop to the test, then on teardown
closes the loop and clears the event loop (asyncio.set_event_loop(None)) to
ensure no stale state; remove the _test_loop global and any callers of
_get_test_loop (or update callers to depend on the fixture) so tests use
pytest-asyncio/fixture-managed loops.
- Around line 29-30: The test hard-codes TEST_DB_PATH causing non-portable
tests; change conftest.py to create a temporary database path instead of the
absolute path by replacing TEST_DB_PATH and the os.environ["DATABASE_URL"]
assignment with a portable approach (e.g., use
tempfile.mkstemp()/tempfile.NamedTemporaryFile or pytest's tmp_path fixture) to
generate a temp file path and set os.environ["DATABASE_URL"] to
f"sqlite+aiosqlite:///{temp_db_path}" so tests run on CI and other machines.
- Around line 99-125: The module-level call inside _create_test_app() that runs
_get_test_loop().run_until_complete(_async_init()) performs schema creation
(using engine and Base) during import and forces asyncio.set_event_loop(), which
conflicts with pytest-asyncio; move the schema creation out of module import and
into a session-scoped pytest fixture (or perform lazy initialization) so that
_async_init() is awaited within a fixture using the pytest event loop instead of
calling _get_test_loop() at import time; update references to
_create_test_app(), _async_init(), engine and Base so schema setup runs inside
that session fixture before tests start.
---
Outside diff comments:
In `@backend/tests/e2e/conftest.py`:
- Around line 38-48: Remove the duplicated section header comment block labeled
"Test user for dependency override" so the file only contains a single header;
locate the two identical comment blocks surrounding the imports (near the
imports of UserResponse and DEFAULT_WALLET) and delete the redundant one,
keeping the single intended header and the imports intact (references:
UserResponse import and DEFAULT_WALLET import).
---
Duplicate comments:
In `@backend/app/api/buybacks.py`:
- Around line 9-12: The endpoint record_buyback currently allows unauthenticated
callers to mutate treasury state; require and enforce
authentication/authorization by adding a FastAPI dependency to the function
signature (e.g., add a parameter like current_user: User =
Depends(get_current_user) or current_admin: User = Depends(require_admin)
depending on your auth utilities) and validate admin/treasury scope before
calling payout_service.create_buyback; update the router declaration/signature
in backend/app/api/buybacks.py (record_buyback) to use the appropriate Depends
helper (get_current_user, require_admin, or require_scope) and return 403 on
insufficient privileges.
- Around line 15-17: The list_buybacks endpoint accepts unconstrained skip and
limit which allows very large values; validate and clamp these inputs in the API
layer (function list_buybacks) before calling payout_service.list_buybacks:
ensure skip is an integer >= 0 and limit is an integer within an allowed maximum
(e.g. <= 1000) and a sensible default (e.g. 100), returning a 400 on invalid
input or silently clamping values; implement this using FastAPI Query parameter
constraints or explicit runtime checks and pass only the sanitized values to
payout_service.list_buybacks to prevent excessive DB load and potential abuse.
In `@backend/app/api/payouts.py`:
- Around line 154-162: In validate_wallet, the success path currently omits the
is_program_address flag so known program addresses lose that signal; update the
WalletValidationResponse returned in the try block inside validate_wallet to
include is_program_address=is_program (keeping wallet_address, valid=True and
message unchanged) so both success and except paths consistently populate
is_program_address; reference validate_wallet, is_program, and
WalletValidationResponse to locate and change the return in the try block.
In `@backend/app/main.py`:
- Around line 190-207: The contributors_router imported as contributors_router
(from app.api.bounties) is not registered with the FastAPI app; add a
registration line in the router block using
app.include_router(contributors_router, prefix="/api", tags=["contributors"])
(place it alongside the other app.include_router calls near
auth_router/bounties_router) so contributor endpoints are exposed.
- Around line 143-161: add_request_id_and_timing is overwriting the request ID
that LoggingMiddleware already sets; update add_request_id_and_timing to respect
an existing request.state.request_id (use that if present) and only fall back to
request.headers.get("X-Request-ID") or a new uuid when request.state.request_id
is missing, then set response.headers["X-Request-ID"] to that chosen ID and use
it in monitor.track_request; this consolidates request ID handling so
LoggingMiddleware remains the single source of truth and prevents log
correlation issues.
- Around line 205-206: The health endpoint is mounted via health_router at the
"/api/v2" prefix (health accessible at "/api/v2/health") but the rate limiters
only exempt "/health" and "/api/health"; update the rate limiting exemption
logic by adding "/api/v2/health" (or better: canonicalize and include any router
prefix + "/health") to the EXEMPT_PATHS used by RateLimitMiddleware and
RateLimiterMiddleware, or change their path check to allow any path that
endswith "/health" (or startswith any registered health prefix) so requests to
health_router (health_router, RateLimitMiddleware, RateLimiterMiddleware,
EXEMPT_PATHS) are not rate limited.
- Around line 128-134: The CORSMiddleware configuration in app.add_middleware
using CORSMiddleware sets allow_credentials=True while using wildcards for
origins/methods/headers, which violates Starlette/FastAPI rules; either set
allow_credentials=False if you need global wildcards, or replace the wildcard
origins/methods/headers with an explicit allow_origins list (and explicit
allow_methods/allow_headers if required) so authenticated requests succeed.
Update the CORSMiddleware call in the app.add_middleware block: choose one of
two fixes—(a) set allow_credentials=False to keep allow_origins=["*"],
allow_methods=["*"], allow_headers=["*"], or (b) provide a concrete list for
allow_origins (and non-wildcard allow_methods/allow_headers if desired) while
leaving allow_credentials=True—so the middleware conforms to Starlette/FastAPI
expectations.
In `@backend/app/middleware/rate_limit.py`:
- Around line 59-64: The current _get_client_id trusts the first value in the
X-Forwarded-For header which is easily spoofed; change it to only honor
X-Forwarded-For when the immediate peer (request.client.host) is a configured
trusted proxy, otherwise ignore XFF and use request.client.host; when honoring
XFF, take the right-most untrusted IP (or the left-most if your infra documents
it) from XFF consistently and document/configure trusted proxies via a
TRUSTED_PROXIES config/env used by _get_client_id to validate
request.client.host before parsing XFF.
- Around line 94-102: The response headers in the rate-limiting middleware are
hardcoded and inconsistent with the token bucket config: replace the fixed "2"
and hardcoded "Retry-After" with values derived from the actual bucket settings
(use the configured burst capacity and current token count) and compute
Retry-After from the token refill/reset time; specifically, in the code paths
around call_next(request) where response.headers and remaining are set, set
"X-RateLimit-Limit" to the bucket's burst capacity, "X-RateLimit-Remaining" to
the current remaining tokens (the existing remaining variable), and set
"Retry-After" to the calculated seconds until a token becomes available based on
the refill/reset logic used earlier (referencing the burst and refill/reset
variables used in the token bucket implementation).
In `@backend/app/services/payout_service.py`:
- Around line 135-153: The lookup functions get_payout_by_id and
get_payout_by_tx_hash currently call pg_store.load_payouts with hard limits
(limit=5000/10000) which truncates results and causes false negatives; change
them to perform targeted DB queries instead of loading a limited batch: for
get_payout_by_id call a pg_store.load_payout_by_id(payout_id) (or equivalent
single-row query) and for get_payout_by_tx_hash call
pg_store.load_payout_by_tx_hash(tx_hash), falling back to the in-memory
_payout_store only if those targeted queries return nothing; update the service
to rely on these new/single-row pg_store methods (or modify load_payouts to
accept no limit/streaming) so lookups are complete.
- Around line 155-179: approve_payout and reject_payout update the DB but never
refresh the in-memory cache (_payout_store), causing stale reads; after calling
pg_store.persist_payout(record) in both approve_payout and reject_payout,
acquire _lock and update _payout_store[payout_id] = record (or set the status
and updated_at on the cached object if present) so the in-memory entry reflects
the persisted PayoutStatus and timestamp; ensure this uses the same _lock used
when reading the cache to avoid races and handle the case where the cache entry
didn't exist by inserting the record.
- Around line 110-133: The current logic calls pg_store.load_payouts(limit=limit
+ skip + 1000) then computes total = len(items) after doing in-memory filters,
which causes incorrect totals and missing records for large datasets; update the
implementation so filtering and counting happen at the data source (use
pg_store.load_payouts with appropriate filter parameters or add a new
pg_store.count_payouts(filters) method) and then fetch only the paginated rows
(apply skip/limit at DB) before mapping with _payout_to_response into
PayoutListResponse, ensuring total reflects the full count of filtered records
and page contains only the requested slice.
- Around line 261-270: The total returned by list_buybacks is wrong because it
uses total=len(items) after paging; change it to compute the true total before
slicing (e.g., all_items = list(all_records.values()); total = len(all_items))
or call a dedicated count method (e.g., pg_store.count_buybacks()) and use that
value for BuybackListResponse.total; update list_buybacks (function name) to set
total to the full count and keep paging logic (page = all_items[skip:
skip+limit]) and then return items from the page.
- Around line 279-284: get_total_paid_out currently calls
pg_store.load_payouts(limit=10000) which truncates results and undercounts
totals; update get_total_paid_out to fetch all payouts (either by removing the
hard limit if load_payouts supports unlimited, or implement pagination/streaming
using pg_store.load_payouts in a loop with offset/limit until no more rows) and
then sum PayoutStatus.CONFIRMED amounts for token == "FNDRY" and token == "SOL"
(keep existing variables fndry and sol and the filtering logic). Ensure the
implementation avoids loading an unbounded result into memory at once by using a
reasonable page size and aggregating per page.
- Around line 242-259: The duplicate-check using
pg_store.load_buybacks(limit=1000) in create_buyback is racy and truncated;
replace it by either a single DB lookup for the given tx_hash (e.g., add/use
pg_store.get_buyback_by_tx_hash(tx_hash) and raise HTTPException(400) if found)
or keep a single insert but wrap the call to pg_store.persist_buyback in a
try/except that catches the DB unique-constraint error (e.g.,
IntegrityError/UniqueViolation from your DB driver) and convert it to
HTTPException(status_code=400, detail="Buyback already exists"); after
successful persist, continue to update _buyback_store under _store_lock and
return _buyback_to_response(record).
- Around line 61-94: create_payout currently does an in-memory/dataset-level
double-pay check (load_payouts with limit=5000 and loop) which is racy and
incomplete; instead add a DB-level UNIQUE constraint/index on the
payouts.bounty_id column and rely on the database for atomic enforcement, update
the persistence path to handle that constraint violation: modify the
schema/migration to add UNIQUE(bounty_id) (allowing nulls if needed), update
pg_store.persist_payout to catch the DB IntegrityError/unique-violation for
bounty_id and translate it into HTTPException(status_code=400, detail="Bounty
{bounty_id} already has a payout."), and remove or keep but not rely on the
current load_payouts/loop check in create_payout so concurrent requests cannot
create double payouts; references: create_payout, load_payouts,
pg_store.persist_payout, PayoutRecord, bounty_id.
In `@backend/logs/application.log.2026-03-21`:
- Around line 1-15: The committed runtime log file application.log.2026-03-21
contains request/trace telemetry and must be removed from the repo; remove it
from version control (git rm --cached application.log.2026-03-21 and commit),
add a rule to .gitignore to prevent re-adding logs, and if any sensitive data
may have been exposed rewrite history using git filter-repo or BFG (or
coordinate with the security team) to purge the file from prior commits; finally
push the cleaned branch and notify reviewers that the log file was removed and
.gitignore updated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 909726b6-e8cd-4c1e-98cc-832cb02eb893
📒 Files selected for processing (16)
backend/app/api/buybacks.pybackend/app/api/payouts.pybackend/app/database.pybackend/app/main.pybackend/app/middleware/rate_limit.pybackend/app/middleware/security.pybackend/app/models/bounty.pybackend/app/services/auth_service.pybackend/app/services/bounty_service.pybackend/app/services/health.pybackend/app/services/payout_service.pybackend/app/services/treasury_service.pybackend/logs/application.log.2026-03-21backend/logs/audit.log.2026-03-21backend/logs/error.log.2026-03-21backend/tests/e2e/conftest.py
| @router.post("/{payout_id}/execute", response_model=PayoutResponse) | ||
| async def execute_payout(payout_id: str, db: AsyncSession = Depends(get_db)) -> PayoutResponse: | ||
| try: | ||
| result = await process_payout(payout_id) | ||
| invalidate_cache() | ||
|
|
||
| # Notify contributor webhooks: bounty paid | ||
| try: | ||
| wh_service = ContributorWebhookService(db) | ||
| bounty_id = result.bounty_id if hasattr(result, "bounty_id") else payout_id | ||
| contributor_id = result.contributor_id if hasattr(result, "contributor_id") else None | ||
| await wh_service.dispatch_event( | ||
| "bounty.paid", | ||
| str(bounty_id), | ||
| { | ||
| "payout_id": payout_id, | ||
| "amount": str(result.amount) if hasattr(result, "amount") else None, | ||
| "tx_hash": result.tx_hash if hasattr(result, "tx_hash") else None, | ||
| }, | ||
| user_id=str(contributor_id) if contributor_id else None, | ||
| ) | ||
| except Exception as e: | ||
| logger.error("Failed to dispatch payout webhook: %s", e) | ||
|
|
||
| return result | ||
|
|
||
| except PayoutNotFoundError as exc: | ||
| raise HTTPException(status_code=404, detail=str(exc)) from exc | ||
| except InvalidPayoutTransitionError as exc: | ||
| raise HTTPException(status_code=409, detail=str(exc)) from exc |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check PayoutResponse fields
ast-grep --pattern 'class PayoutResponse($$$) {
$$$
}'
rg -n "class PayoutResponse" -A 30 backend/app/models/payout.pyRepository: SolFoundry/solfoundry
Length of output: 1682
🏁 Script executed:
#!/bin/bash
# Find process_payout function definition
rg -n "def process_payout\|async def process_payout" backend/ -A 15
# Also search for where ContributorWebhookService.dispatch_event is defined
rg -n "class ContributorWebhookService" backend/ -A 30 | head -50Repository: SolFoundry/solfoundry
Length of output: 2852
🏁 Script executed:
#!/bin/bash
# Find dispatch_event method signature
rg -n "async def dispatch_event|def dispatch_event" backend/app/services/contributor_webhook_service.py -A 10
# Find process_payout function
rg -n "async def process_payout|def process_payout" backend/ -A 20Repository: SolFoundry/solfoundry
Length of output: 2280
🏁 Script executed:
#!/bin/bash
# Check the full process_payout function to see how PayoutResponse is constructed
rg -n "async def process_payout" backend/app/services/payout_service.py -A 50 | head -70
# Also check if there are any other return paths or exceptions that might not be PayoutResponse
rg -n "return.*PayoutResponse\|return PayoutResponse" backend/app/services/payout_service.pyRepository: SolFoundry/solfoundry
Length of output: 2236
🏁 Script executed:
#!/bin/bash
# Find the _payout_to_response function to understand the conversion
rg -n "_payout_to_response" backend/app/services/payout_service.py -B 5 -A 20Repository: SolFoundry/solfoundry
Length of output: 5093
Remove unnecessary hasattr checks and fix attribute access pattern.
The code uses defensive hasattr() checks (lines 196-206) even though process_payout() always returns a PayoutResponse instance. The attributes bounty_id, amount, and tx_hash are guaranteed to exist as defined fields.
Additionally, contributor_id does not exist in PayoutResponse and will always be None — this field should be removed or the logic reconsidered.
Instead of the suggested getattr(result, "bounty_id", None) or payout_id pattern (which would incorrectly fall back when bounty_id is legitimately None), directly access the attributes:
- bounty_id = result.bounty_id if hasattr(result, "bounty_id") else payout_id
- contributor_id = result.contributor_id if hasattr(result, "contributor_id") else None
+ bounty_id = result.bounty_id or payout_id # or use bounty_id directly if None is valid
+ contributor_id = None # PayoutResponse has no contributor_id field🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/payouts.py` around lines 187 - 216, The execute_payout
handler is using unnecessary hasattr checks for attributes that always exist on
PayoutResponse and is referencing a non‑existent contributor_id; replace the
hasattr usages by directly reading fields from the PayoutResponse returned by
process_payout (e.g. use result.bounty_id, str(result.amount), result.tx_hash)
and remove the contributor_id logic entirely; if you need to fall back to
payout_id when bounty_id is explicitly None, do an explicit None check
(bounty_id = result.bounty_id if result.bounty_id is not None else payout_id)
rather than getattr(..., None) or truthy "or" fallbacks, and call
ContributorWebhookService.dispatch_event with user_id omitted or None.
backend/app/middleware/rate_limit.py
Outdated
| def __init__(self, app, redis_url: str = os.getenv("REDIS_URL", "redis://localhost:6379")): | ||
| super().__init__(app) | ||
| self.redis = redis.from_url(redis_url, decode_responses=True) | ||
| self._lua_script = None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect Redis client initialization and timeout-related options in middleware.
rg -n -C3 'redis\.from_url|socket_timeout|socket_connect_timeout|health_check_interval' backend/app/middleware/rate_limit.py
# Find other Redis client constructions for consistency comparison.
rg -n --type=py 'from_url\(|Redis\(' backend/appRepository: SolFoundry/solfoundry
Length of output: 902
🏁 Script executed:
# Get full context of the rate_limit.py middleware to see line 77 and error handling
head -100 backend/app/middleware/rate_limit.py | cat -nRepository: SolFoundry/solfoundry
Length of output: 4410
🏁 Script executed:
# Check if there's more content in the file and see the fail-open mechanism mentioned at line 77
wc -l backend/app/middleware/rate_limit.py
sed -n '70,85p' backend/app/middleware/rate_limit.py | cat -nRepository: SolFoundry/solfoundry
Length of output: 978
🏁 Script executed:
# Search for timeout handling throughout the middleware
rg -n "timeout|except|except.*redis|try" backend/app/middleware/rate_limit.py | head -30Repository: SolFoundry/solfoundry
Length of output: 157
🏁 Script executed:
# Check if there are any socket_timeout or connection pool timeout configurations elsewhere in codebase
rg -n "socket_timeout|connect_timeout|timeout|ConnectionPool|pool" backend/app --type=py | grep -i redis | head -20Repository: SolFoundry/solfoundry
Length of output: 194
🏁 Script executed:
# Check the redis library version and configuration patterns
cat backend/requirements.txt 2>/dev/null || cat setup.py 2>/dev/null | grep -i redisRepository: SolFoundry/solfoundry
Length of output: 785
🏁 Script executed:
# Look for any timeout configuration in environment or config files
find backend -name "*.py" -o -name "*.env*" -o -name "*.yaml" -o -name "*.yml" | xargs grep -l "REDIS\|timeout" 2>/dev/null | head -10Repository: SolFoundry/solfoundry
Length of output: 344
🏁 Script executed:
# Check core config for timeout settings
cat -n backend/app/core/config.py | head -50Repository: SolFoundry/solfoundry
Length of output: 1763
🏁 Script executed:
# Check redis.py initialization
cat -n backend/app/core/redis.pyRepository: SolFoundry/solfoundry
Length of output: 1319
🏁 Script executed:
# Check .env.example for REDIS configuration
cat -n backend/.env.example | grep -i redisRepository: SolFoundry/solfoundry
Length of output: 166
Redis client initialization lacks explicit socket timeouts, causing request delays during network degradation.
Line 56 initializes the Redis client with redis.from_url(redis_url, decode_responses=True) without explicit timeout parameters. During degraded Redis/network conditions, async operations can hang indefinitely on socket operations until the connection is forcibly closed by the OS or network, even though the exception handler at lines 76-78 eventually provides a fail-open fallback. This causes unnecessary request latency and potential resource exhaustion in production.
The redis>=5.0 library supports timeout configuration via socket_timeout and socket_connect_timeout parameters in from_url(). This same pattern of missing timeouts is replicated across the codebase in: backend/app/services/websocket_manager.py:78, backend/app/core/redis.py:20, backend/app/api/health.py:52, and backend/app/api/admin.py:900.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/middleware/rate_limit.py` around lines 54 - 57, The Redis client
in RateLimitMiddleware.__init__ initializes self.redis via
redis.from_url(redis_url, decode_responses=True) without socket timeouts; update
that call to pass explicit socket_connect_timeout and socket_timeout (e.g.,
short connect timeout and reasonable read timeout) so socket operations fail
fast during network degradation, then retain the existing fail-open error
handling around _lua_script and request processing; apply the same change to
other Redis initializations that use redis.from_url in websocket_manager
(WebsocketManager), core/redis (module-level client factory), api/health (health
check client), and api/admin (admin Redis client) to ensure consistent timeout
behavior across the codebase.
| if request.url.path == "/api/health": | ||
| return await call_next(request) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify registered health routes and prefixes.
rg -n -C3 '@router\.get\("/health"|@app\.(get|api_route)\("/health"|include_router\(|prefix=' backend/app --type=py
# Check if middleware skip logic covers all health endpoints.
rg -n -C2 'request\.url\.path == "/api/health"|/health' backend/app/middleware/rate_limit.py backend/app/main.py backend/app/api/health.pyRepository: SolFoundry/solfoundry
Length of output: 8574
🏁 Script executed:
# Check the health_router definition in health.py
head -100 backend/app/api/health.py | grep -A 5 "router ="
# Also check for any other health-related endpoints or configurations
rg -n "health|Health" backend/app/api/health.py | head -20
# Verify the actual routes and their full paths
rg -n "@router\.|@app\." backend/app/api/health.pyRepository: SolFoundry/solfoundry
Length of output: 1587
🏁 Script executed:
# Search for any other health endpoint definitions
rg -n "@router\.get.*health|@app\.get.*health|@router\.post.*health" backend/app --type=py
# Check for any route prefixing or URL rewriting that might affect health endpoints
rg -n "url\.path|request\.path|/api/health|/health" backend/app/middleware/ backend/app/main.py --type=py | head -30
# Verify if there's a root /health endpoint
rg -n "@app\.get\(\"/health" backend/app --type=pyRepository: SolFoundry/solfoundry
Length of output: 1500
🏁 Script executed:
# Check the rate_limiter.py EXEMPT_PATHS definition and context
sed -n '60,75p' backend/app/middleware/rate_limiter.py
# Check app middleware registration to see which is actually used
rg -n "add_middleware|RateLimit" backend/app/main.py --type=py -A 2 -B 2
# Verify if there's a root-level /health endpoint
rg -n "^@app\.get.*health|^app\.add_route.*health" backend/app/main.py --type=pyRepository: SolFoundry/solfoundry
Length of output: 1395
🏁 Script executed:
# Get the full context of rate_limiter.py to understand the startswith check
sed -n '295,310p' backend/app/middleware/rate_limiter.py
# Verify the actual registered health endpoint path by checking main.py again
sed -n '200,210p' backend/app/main.py
# Check if there's any other route or middleware serving /health at root
rg -n "prefix=|/health" backend/app/api/health.py | head -5Repository: SolFoundry/solfoundry
Length of output: 1309
🏁 Script executed:
# Check if there's a root-level /health endpoint or app.get decorator
rg -n "^@app\.get|^@app\.post|^@app\.api_route.*health" backend/app --type=py
# Check for any middleware or routing that might serve /health at root
rg -n "app\.get\(\"/health|app\.post\(\"/health" backend/app --type=py
# Verify the complete path: health_router prefix + endpoint path
grep -n "health_router" backend/app/main.pyRepository: SolFoundry/solfoundry
Length of output: 191
🏁 Script executed:
# Verify the logs mentioned in the review to understand what paths are being used
ls -la backend/logs/ 2>/dev/null || echo "logs directory not found"
# Look at the log file mentioned in the review
head -20 backend/logs/application.log.2026-03-21 2>/dev/null || echo "log file not found"
# Search for any alternative health endpoint definitions
rg -n "get\(.*health|post\(.*health|health.*route" backend/app --type=py -iRepository: SolFoundry/solfoundry
Length of output: 3713
🏁 Script executed:
# Check the exact middleware bypass logic in rate_limiter.py - look at the full dispatch method
sed -n '275,320p' backend/app/middleware/rate_limiter.py
# Search for any app-level health endpoint or root /health serving
rg -n "def.*health|class.*health" backend/app --type=py -i | grep -v "Comprehensive\|health check\|health\|Health"
# Check if there's a livenessProbe or readinessProbe endpoint (common Kubernetes patterns)
rg -n "liveness|readiness|alive|ready" backend/app --type=py -iRepository: SolFoundry/solfoundry
Length of output: 7438
Health-check rate-limit bypass checks both fail for the registered /api/v2/health endpoint.
The middleware at line 82 bypasses only the non-existent /api/health path, but the actual health endpoint is registered at /api/v2 prefix (main.py:205: app.include_router(health_router, prefix="/api/v2")), making the endpoint accessible at /api/v2/health. Additionally, the alternate RateLimiterMiddleware (rate_limiter.py:298) exempts paths starting with /health, which also fails to match /api/v2/health. Logs confirm /health requests traverse rate-limit middleware and encounter Redis errors, confirming health endpoints are not properly isolated from rate-limiting.
Fix: Update both middleware bypass checks to include the actual endpoint path (/api/v2/health), or use wildcard matching (**/health) to cover all future health endpoint variations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/middleware/rate_limit.py` around lines 82 - 83, The health-check
bypass currently only matches "/api/health" and misses the registered
"/api/v2/health"; update the check in backend/app/middleware/rate_limit.py (the
branch that returns await call_next(request)) to also allow "/api/v2/health" —
better: replace the exact-path check with a more robust match (e.g.,
request.url.path.endswith("/health") or a contains "/health" rule) so any
prefixed health route passes through; also update the alternate
RateLimiterMiddleware exemption in backend/app/middleware/rate_limiter.py (the
logic around paths starting with "/health") to use the same robust matching
(endswith or wildcard) so both middleware implementations consistently bypass
all health endpoints.
| # CSP directives | ||
| CSP_DEFAULT_SRC: str = os.getenv("CSP_DEFAULT_SRC", "'self'") | ||
| CSP_SCRIPT_SRC: str = os.getenv("CSP_SCRIPT_SRC", "'self'") | ||
| CSP_STYLE_SRC: str = os.getenv("CSP_STYLE_SRC", "'self' 'unsafe-inline'") | ||
| CSP_IMG_SRC: str = os.getenv("CSP_IMG_SRC", "'self' data: https:") | ||
| CSP_SCRIPT_SRC: str = os.getenv("CSP_SCRIPT_SRC", "'self' 'unsafe-inline' 'unsafe-eval'") | ||
| CSP_STYLE_SRC: str = os.getenv("CSP_STYLE_SRC", "'self' 'unsafe-inline' https://fonts.googleapis.com") | ||
| CSP_IMG_SRC: str = os.getenv("CSP_IMG_SRC", "'self' data: https: https://solfoundry.org") | ||
| CSP_CONNECT_SRC: str = os.getenv( | ||
| "CSP_CONNECT_SRC", "'self' https://api.mainnet-beta.solana.com" | ||
| ) | ||
| CSP_FONT_SRC: str = os.getenv("CSP_FONT_SRC", "'self'") | ||
| CSP_FONT_SRC: str = os.getenv("CSP_FONT_SRC", "'self' https://fonts.gstatic.com") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
❓ Verification inconclusive
FastAPI custom Swagger UI with CSP nonce 2025
To run FastAPI Swagger UI under a strict CSP (with nonces) you generally need to (1) disable FastAPI’s built-in /docs, (2) serve a custom /docs HTML where you add nonce="..." to the inline <script> / <style> tags, and (3) set a per-request CSP header that includes those nonces. FastAPI supports custom Swagger UI routes via get_swagger_ui_html. [1]
Key gotcha (Swagger UI + CSP)
Even if you nonce all inline scripts, Swagger UI has historically required unsafe-eval in some configurations/versions (due to runtime code generation), so a CSP that forbids eval may still result in a blank/broken UI unless you allow unsafe-eval (at least on the docs route) or use a Swagger UI build/version that avoids it. [2], [3]
FastAPI example: custom /docs with CSP nonces
import secrets
from fastapi import FastAPI
from fastapi.openapi.docs import get_swagger_ui_html
from starlette.responses import HTMLResponse
app = FastAPI(docs_url=None, redoc_url=None)
`@app.get`("/docs", include_in_schema=False)
def custom_swagger_ui():
nonce = secrets.token_urlsafe(16)
html = get_swagger_ui_html(
openapi_url=app.openapi_url,
title="API docs",
# swagger_ui_parameters={...}, # optional
).body.decode("utf-8")
# Add nonce to inline tags (FastAPI’s docs HTML includes inline JS/CSS)
html = html.replace("<script>", f'<script nonce="{nonce}">')
html = html.replace("<style>", f'<style nonce="{nonce}">')
csp = (
"default-src 'self'; "
f"script-src 'self' 'nonce-{nonce}' 'unsafe-eval'; "
f"style-src 'self' 'nonce-{nonce}'; "
"img-src 'self' data:; "
"font-src 'self' data:; "
"connect-src 'self'; "
"base-uri 'none'; "
"frame-ancestors 'none'"
)
return HTMLResponse(html, headers={"Content-Security-Policy": csp})Practical advice: keep this relaxed CSP only on /docs, and use a stricter CSP elsewhere (Swagger UI’s needs are often incompatible with a fully locked-down global CSP). [2], [3]
Sources: FastAPI docs customization entry point [1]; Swagger UI CSP/eval problem reports [2], [3].
[1] (fastapi.tiangolo.com)
[2] (github.com)
[3] (community.smartbear.com)
Citations:
- 1: https://fastapi.tiangolo.com/how-to/configure-swagger-ui/?utm_source=openai
- 2: Swagger-ui requires 'unsafe-eval' in CSP Headers for SVG swagger-api/swagger-ui#7540
- 3: https://community.smartbear.com/discussions/swaggerostools/swagger-ui-requires-unsafe-eval-or-unsafe-inline-as-part-of-csp-header-load/253249?utm_source=openai
Script executed:
# Find and read the security middleware file
find . -type f -name "security.py" -path "*/middleware/*" | head -5Repository: SolFoundry/solfoundry
Repository: SolFoundry/solfoundry
Exit code: 0
stdout:
./backend/app/middleware/security.py
Script executed:
# Once found, read the security.py file with context around lines 35-43 and check the full middleware
cd backend && find . -type f -name "security.py" -path "*/middleware/*" -exec cat -n {} \; | head -150Repository: SolFoundry/solfoundry
Repository: SolFoundry/solfoundry
Exit code: 0
stdout:
1 """HTTP security headers middleware for production hardening.
2
3 Implements comprehensive security headers following OWASP recommendations:
4 - Strict-Transport-Security (HSTS) with preload
5 - Content-Security-Policy (CSP) restricting resource origins
6 - X-Frame-Options preventing clickjacking
7 - X-Content-Type-Options preventing MIME sniffing
8 - Referrer-Policy limiting referrer information leakage
9 - Permissions-Policy restricting browser feature access
10 - Cache-Control headers for sensitive endpoints
11 - Request body size enforcement to prevent resource exhaustion
12 """
13
14 import logging
15 import os
16 from typing import Callable
17
18 from fastapi import Request, Response
19 from starlette.middleware.base import BaseHTTPMiddleware
20 from starlette.responses import JSONResponse
21
22 logger = logging.getLogger(__name__)
23
24 # Maximum request body size in bytes (default 1 MB)
25 MAX_REQUEST_BODY_SIZE: int = int(
26 os.getenv("MAX_REQUEST_BODY_SIZE", str(1 * 1024 * 1024))
27 )
28
29 # Whether to enforce HTTPS (disable in local dev)
30 ENFORCE_HTTPS: bool = os.getenv("ENFORCE_HTTPS", "true").lower() == "true"
31
32 # HSTS max-age in seconds (default: 1 year)
33 HSTS_MAX_AGE: int = int(os.getenv("HSTS_MAX_AGE", "31536000"))
34
35 # CSP directives
36 CSP_DEFAULT_SRC: str = os.getenv("CSP_DEFAULT_SRC", "'self'")
37 CSP_SCRIPT_SRC: str = os.getenv("CSP_SCRIPT_SRC", "'self' 'unsafe-inline' 'unsafe-eval'")
38 CSP_STYLE_SRC: str = os.getenv("CSP_STYLE_SRC", "'self' 'unsafe-inline' https://fonts.googleapis.com")
39 CSP_IMG_SRC: str = os.getenv("CSP_IMG_SRC", "'self' data: https: https://solfoundry.org")
40 CSP_CONNECT_SRC: str = os.getenv(
41 "CSP_CONNECT_SRC", "'self' https://api.mainnet-beta.solana.com"
42 )
43 CSP_FONT_SRC: str = os.getenv("CSP_FONT_SRC", "'self' https://fonts.gstatic.com")
44 CSP_FRAME_ANCESTORS: str = os.getenv("CSP_FRAME_ANCESTORS", "'none'")
45
46 # Paths considered sensitive (no caching)
47 SENSITIVE_PATH_PREFIXES: tuple[str, ...] = (
48 "/auth/",
49 "/api/payouts",
50 "/api/treasury",
51 )
52
53 def _build_csp_header() -> str:
54 directives = [
55 f"default-src {CSP_DEFAULT_SRC}",
56 f"script-src {CSP_SCRIPT_SRC}",
57 f"style-src {CSP_STYLE_SRC}",
58 f"img-src {CSP_IMG_SRC}",
59 f"connect-src {CSP_CONNECT_SRC}",
60 f"font-src {CSP_FONT_SRC}",
61 f"frame-ancestors {CSP_FRAME_ANCESTORS}",
62 "base-uri 'self'",
63 "form-action 'self'",
64 "object-src 'none'",
65 "upgrade-insecure-requests",
66 ]
67 return "; ".join(directives)
68
69 def _build_permissions_policy() -> str:
70 policies = [
71 "camera=()", "microphone=()", "geolocation=()", "payment=()",
72 "usb=()", "magnetometer=()", "gyroscope=()", "accelerometer=()",
73 ]
74 return ", ".join(policies)
75
76 class SecurityHeadersMiddleware(BaseHTTPMiddleware):
77 """OWASP-recommended security headers and request body size enforcement."""
78
79 def __init__(self, app: Callable) -> None:
80 super().__init__(app)
81 self.csp_header = _build_csp_header()
82 self.permissions_policy = _build_permissions_policy()
83
84 async def dispatch(self, request: Request, call_next: Callable) -> Response:
85 # Enforce request body size limit
86 content_length = request.headers.get("content-length")
87 if content_length and int(content_length) > MAX_REQUEST_BODY_SIZE:
88 return JSONResponse(
89 status_code=413,
90 content={"detail": "Request body too large", "code": "PAYLOAD_TOO_LARGE"}
91 )
92
93 response = await call_next(request)
94
95 # Apply Headers
96 if ENFORCE_HTTPS:
97 response.headers["Strict-Transport-Security"] = f"max-age={HSTS_MAX_AGE}; includeSubDomains; preload"
98
99 response.headers["Content-Security-Policy"] = self.csp_header
100 response.headers["X-Frame-Options"] = "DENY"
101 response.headers["X-Content-Type-Options"] = "nosniff"
102 response.headers["Referrer-Policy"] = "strict-origin-when-cross-origin"
103 response.headers["Permissions-Policy"] = self.permissions_policy
104
105 if any(request.url.path.startswith(p) for p in SENSITIVE_PATH_PREFIXES):
106 response.headers["Cache-Control"] = "no-store, no-cache, must-revalidate"
107 response.headers["Pragma"] = "no-cache"
108
109 if "Server" in response.headers:
110 del response.headers["Server"]
111
112 return response
113
114 # Alias for backward compatibility with existing tests
115 SecurityMiddleware = SecurityHeadersMiddleware
Script executed:
# Check if /docs is disabled or customized in main app
find . -type f \( -name "main.py" -o -name "app.py" -o -name "__init__.py" \) -path "*/backend/*" -exec grep -l "docs_url\|redoc_url\|swagger_ui_html" {} \;Repository: SolFoundry/solfoundry
Repository: SolFoundry/solfoundry
Exit code: 0
CSP relaxation applies globally and should be route-specific to minimize XSS exposure.
The CSP header at lines 37–39 permits 'unsafe-inline' and 'unsafe-eval' on scripts and is applied globally to every response (line 99 in the middleware). This weakens XSS protections across the entire API, not just for the /docs endpoint.
While 'unsafe-eval' is historically necessary for Swagger UI to function, the current global application is overly permissive. FastAPI's /docs should use a custom route with nonce-based CSP (as documented in FastAPI's configuration guide), keeping the global CSP strict and restrictive for all other endpoints.
Recommended fix: Disable the default /docs and /redoc routes, implement a custom /docs handler that injects nonces into inline scripts/styles, and apply a relaxed CSP only to that route. Keep the global middleware CSP strict (removing 'unsafe-inline' and 'unsafe-eval').
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/middleware/security.py` around lines 35 - 43, The CSP is
currently too permissive globally via CSP_SCRIPT_SRC and CSP_STYLE_SRC (they
include 'unsafe-inline' and 'unsafe-eval') and is applied in the global
middleware in security.py; change the global CSP to remove 'unsafe-inline' and
'unsafe-eval' (tighten
CSP_DEFAULT_SRC/CSP_SCRIPT_SRC/CSP_STYLE_SRC/CSP_IMG_SRC/CSP_CONNECT_SRC/CSP_FONT_SRC)
and then disable FastAPI's default /docs and /redoc routes, creating a custom
/docs handler that injects per-request nonces into inline scripts/styles and
applies a relaxed CSP header only for that route; update the middleware that
sets the response header to use the strict env-based values by default and add
route-level logic to set the relaxed CSP when serving the custom docs endpoint.
| {"message": "RateLimiter Redis Error: Error Multiple exceptions: [Errno 61] Connect call failed ('::1', 6379, 0, 0), [Errno 61] Connect call failed ('127.0.0.1', 6379) connecting to localhost:6379. - Failsafe: ALLOW", "taskName": "starlette.middleware.base.BaseHTTPMiddleware.__call__.<locals>.call_next.<locals>.coro"} | ||
| {"message": "RateLimiter/Blocklist Redis Error: Error 1 connecting to localhost:6379. Operation not permitted. - Failsafe: ALLOW", "taskName": "starlette.middleware.base.BaseHTTPMiddleware.__call__.<locals>.call_next.<locals>.coro"} |
There was a problem hiding this comment.
Runtime error logs should not be committed to source control.
Lines 1-2 contain environment-specific operational failures and fail-open security behavior details. Keeping these artifacts in the repo increases leakage risk and creates non-deterministic diff noise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/logs/error.log.2026-03-21` around lines 1 - 2, This commit
accidentally includes runtime error logs (entries mentioning "RateLimiter",
"Blocklist", "Failsafe: ALLOW", and the Starlette taskName
"starlette.middleware.base.BaseHTTPMiddleware.__call__...coro"); remove that log
file from the repo, add the log filename pattern (e.g., error.log.* or the
specific runtime log name) to .gitignore, and revert the file from the commit
history so operational logs are not tracked; additionally, ensure your logging
config writes to a non-source-control sink (rotating file, syslog, or cloud
logs) and add a pre-commit/CI check to prevent committed runtime logs in the
future.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/tests/e2e/conftest.py (1)
88-96:⚠️ Potential issue | 🟠 MajorShared import-time event loop is still a suite-wide reliability hazard.
Lines 88-96 reintroduce a module-global loop via
_get_test_loop(), and Lines 124-128 driveengine.begin()throughrun_until_complete()whileconftest.pyis importing. That makes test collection depend on async database I/O before fixture setup and leaves initialization running on a different loop from the one later used bypytest-asyncio/anyio inasync_client. The failure mode is suite-wide: if that loop is stale, blocked, or incompatible with later async fixtures, collection/import breaks before the tests even start. This is the same unresolved loop-pollution/import-order issue already raised on earlier revisions.Also applies to: 102-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/e2e/conftest.py` around lines 88 - 96, The module-global _test_loop and the import-time call driving engine.begin() cause loop pollution and run async DB I/O during import; remove the _test_loop/_get_test_loop usage and stop calling engine.begin() at import time in conftest.py, and instead perform database initialization inside an async fixture (e.g., a pytest-asyncio/anyio session-scoped fixture used by async_client) so engine.begin() is awaited on the test-run event loop; update any places referencing _get_test_loop to use the fixture-provided loop or run engine.begin() inside that fixture so all async work uses the same pytest-managed loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/tests/e2e/conftest.py`:
- Around line 30-33: The tests currently use a single temp-file SQLite DB
(TEST_DB_PATH, DATABASE_URL) and call Base.metadata.create_all() while
clear_stores() only resets in-memory stores, which causes persistent state and
fails to validate PostgreSQL-specific behaviors; update conftest.py to spin up
an ephemeral PostgreSQL 15+ instance for E2E (use a test container or in-memory
PG harness), set DATABASE_URL to that instance instead of TEST_DB_PATH, run
Alembic migrations (not Base.metadata.create_all()) during async initialization,
and extend the teardown/clear_stores() fixture to truncate all DB tables and
destroy the ephemeral PG instance to ensure full isolation and correct full-text
search semantics validation.
---
Duplicate comments:
In `@backend/tests/e2e/conftest.py`:
- Around line 88-96: The module-global _test_loop and the import-time call
driving engine.begin() cause loop pollution and run async DB I/O during import;
remove the _test_loop/_get_test_loop usage and stop calling engine.begin() at
import time in conftest.py, and instead perform database initialization inside
an async fixture (e.g., a pytest-asyncio/anyio session-scoped fixture used by
async_client) so engine.begin() is awaited on the test-run event loop; update
any places referencing _get_test_loop to use the fixture-provided loop or run
engine.begin() inside that fixture so all async work uses the same
pytest-managed loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fa065a82-b930-431d-9dcc-ebbf143c3369
📒 Files selected for processing (4)
.github/workflows/ci.ymlbackend/app/services/payout_service.pybackend/app/services/treasury_service.pybackend/tests/e2e/conftest.py
| # Ensure the test database path is absolute and platform-agnostic | ||
| TEST_DB_PATH = os.path.join(tempfile.gettempdir(), "test_e2e.db") | ||
| DATABASE_URL = f"sqlite+aiosqlite:///{TEST_DB_PATH}" | ||
| os.environ["DATABASE_URL"] = DATABASE_URL |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's view the conftest.py file to see the actual code
cat -n backend/tests/e2e/conftest.py | head -200Repository: SolFoundry/solfoundry
Length of output: 9332
🏁 Script executed:
# Check for migration/alembic setup
find backend -name "alembic.ini" -o -name "migrations" -o -name "alembic" -type d 2>/dev/null | head -20Repository: SolFoundry/solfoundry
Length of output: 146
🏁 Script executed:
# Search for database initialization patterns in the codebase
rg -A 5 "Base.metadata.create_all|alembic|migration" backend --type py | head -60Repository: SolFoundry/solfoundry
Length of output: 3793
🏁 Script executed:
# Check the actual Mission_82_Specs.md to see test environment requirements
cat Mission_82_Specs.md 2>/dev/null | head -100Repository: SolFoundry/solfoundry
Length of output: 2015
🏁 Script executed:
# Check if there's any database cleanup in conftest or test setup
rg "truncate|delete from|drop table|rm.*\.db" backend/tests/e2e --type pyRepository: SolFoundry/solfoundry
Length of output: 47
Persistent SQLite test DB prevents proper E2E validation of Mission #82 PostgreSQL requirements and creates cross-test contamination.
Lines 30–33 pin all E2E tests to the same temp-file SQLite database, lines 124–128 use Base.metadata.create_all() instead of running Alembic migrations, and the clear_stores() fixture at lines 177–180 clears only in-memory stores—it never truncates database tables or removes the temp file. This means rows and schema persist across test runs, preventing test isolation. More critically, Mission #82's acceptance criteria explicitly require PostgreSQL 15+ with full-text search semantics (weighted TSVECTOR, ?| vs ?& operators for skill filtering, specific ranking behavior), but this harness exercises only SQLite. E2E tests therefore cannot validate PostgreSQL-specific search behavior, weight ordering, or the migration history—only whether the code compiles against SQLAlchemy's generic schema layer. That is a material acceptance-path gap.
Also applies to: 88–96 (shared event loop), 124–128 (async initialization), 177–180 (cleanup scope).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/e2e/conftest.py` around lines 30 - 33, The tests currently use
a single temp-file SQLite DB (TEST_DB_PATH, DATABASE_URL) and call
Base.metadata.create_all() while clear_stores() only resets in-memory stores,
which causes persistent state and fails to validate PostgreSQL-specific
behaviors; update conftest.py to spin up an ephemeral PostgreSQL 15+ instance
for E2E (use a test container or in-memory PG harness), set DATABASE_URL to that
instance instead of TEST_DB_PATH, run Alembic migrations (not
Base.metadata.create_all()) during async initialization, and extend the
teardown/clear_stores() fixture to truncate all DB tables and destroy the
ephemeral PG instance to ensure full isolation and correct full-text search
semantics validation.
denaev-dev
force-pushed
the
bounty-health-check-v2
branch
from
d410c07 to
851d3af
Compare
denaev-dev
force-pushed
the
bounty-health-check-v2
branch
from
851d3af to
560b781
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/app/services/pg_store.py (2)
209-229:⚠️ Potential issue | 🟠 Major
load_payoutsdoes not loadupdated_at,failure_reason,retry_count,admin_approved_byfields.Lines 216-228 construct
PayoutRecordbut omit these fields. Combined withpersist_payoutnot saving them, this creates a complete data loss loop for payout lifecycle metadata.Additionally, line 225
PayoutStatus(row.status.lower())will raiseValueErrorfor unexpected status values, crashing the entire load operation.🔧 Proposed fix for missing fields
out[str(row.id)] = PayoutRecord( id=str(row.id), recipient=row.recipient, recipient_wallet=row.recipient_wallet, amount=float(row.amount), token=row.token, bounty_id=str(row.bounty_id) if row.bounty_id else None, bounty_title=row.bounty_title, tx_hash=row.tx_hash, status=PayoutStatus(row.status.lower()), solscan_url=row.solscan_url, created_at=row.created_at, + updated_at=getattr(row, "updated_at", row.created_at), + failure_reason=getattr(row, "failure_reason", None), + retry_count=getattr(row, "retry_count", 0), + admin_approved_by=getattr(row, "admin_approved_by", None), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/pg_store.py` around lines 209 - 229, The load_payouts function is missing several payout lifecycle fields and uses an unsafe enum conversion; update the PayoutRecord construction in load_payouts to include updated_at, failure_reason, retry_count, and admin_approved_by by reading them from the PayoutTable row (e.g., row.updated_at, row.failure_reason, row.retry_count, row.admin_approved_by), and ensure persist_payout also persists those same fields; additionally replace the brittle PayoutStatus(row.status.lower()) call with a safe parse (normalize the DB value, attempt to construct the PayoutStatus and wrap it in a try/except or use a mapping/fallback to a sensible default like PayoutStatus.UNKNOWN or PayoutStatus.PENDING) so unexpected status values do not raise ValueError and crash load_payouts.
188-207:⚠️ Potential issue | 🔴 Critical
persist_payoutis missing critical fields that are set during state transitions:updated_at,failure_reason,retry_count,admin_approved_by.The PayoutRecord model defines all four fields and the service layer actively sets them (especially
updated_aton every state change andfailure_reasonon failures), but PayoutTable lacks the corresponding columns andpersist_payoutomits them from the upsert. This causes silent data loss when records are persisted and reloaded from the database.The docstring in payout.py includes a migration template showing these fields were intended for the schema:
admin_approved_by VARCHAR(100), retry_count INT NOT NULL DEFAULT 0, failure_reason TEXT, updated_at TIMESTAMPTZ NOT NULL DEFAULT now()Add all four fields to the upsert call at lines 192-206:
await _upsert( session, PayoutTable, record.id, recipient=record.recipient, recipient_wallet=record.recipient_wallet, amount=record.amount, token=record.token, bounty_id=_to_uuid(record.bounty_id) if record.bounty_id else None, bounty_title=record.bounty_title, tx_hash=record.tx_hash, status=status, solscan_url=record.solscan_url, created_at=record.created_at, + updated_at=getattr(record, "updated_at", record.created_at), + failure_reason=getattr(record, "failure_reason", None), + retry_count=getattr(record, "retry_count", 0), + admin_approved_by=getattr(record, "admin_approved_by", None), )Note: PayoutTable must first be updated to include these columns before this fix is applied.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/pg_store.py` around lines 188 - 207, persist_payout omits fields that are set during state transitions—add updated_at, failure_reason, retry_count, and admin_approved_by to the _upsert call in persist_payout so these values from the PayoutRecord are persisted; specifically, pass updated_at=record.updated_at, failure_reason=record.failure_reason, retry_count=record.retry_count, and admin_approved_by=record.admin_approved_by into the existing _upsert(...) invocation in the persist_payout function (ensure PayoutTable schema has corresponding columns before applying).backend/tests/e2e/conftest.py (1)
101-157:⚠️ Potential issue | 🟠 MajorThe E2E clients are not exercising the real application stack.
_create_test_app()rebuilds a bareFastAPIinstance with routers only; it does not wire the production lifespan or app-level middleware, and it also adds a stub/healthroute at Lines 155-157 that returns the stale{"status": "ok"}contract. Because bothclientandasync_clientbind to this synthetic app, this suite cannot validate Mission#80behaviors that live in the real request pipeline: actual health telemetry, unifiedhealthy|degraded|unavailablestatuses, request ID / process-time headers, and failsafe behavior when dependencies degrade.As per coding guidelines,
backend/logs/application.log.2026-03-21expects/healthrequest instrumentation with correlatedrequest_id, andbackend/logs/error.log.2026-03-21requires Redis failures to stay failsafe rather than break health availability.Also applies to: 195-256
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/e2e/conftest.py` around lines 101 - 157, The test builds a synthetic FastAPI app in _create_test_app (creating test_app, adding routers and a stub health_check) which bypasses production lifespan, middleware, and real /health behavior; replace this by instantiating the real application (use the production app factory or module that wires lifespan/middleware/telemetry) instead of creating test_app, remove the inline health_check stub, and keep only dependency_overrides[get_current_user] = _mock_get_current_user (or apply the override on the real app) so E2E clients exercise the real request pipeline (request_id headers, real health handler that returns healthy|degraded|unavailable, failsafe Redis behavior) while preserving authentication override for tests.
♻️ Duplicate comments (13)
backend/submit_bounty.py (3)
9-22:⚠️ Potential issue | 🟠 MajorDead payload includes hardcoded submission metadata and wallet identifier.
pr_bodyis never consumed, so Lines 9-22 are dead code. It also embeds hardcoded operational metadata (including a wallet address), which is unnecessary in backend source and increases compliance/audit noise if committed long-term.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/submit_bounty.py` around lines 9 - 22, Remove the unused hardcoded pr_body string in submit_bounty.py: delete the dead variable definition (pr_body) and any unreachable block that defines it, ensuring no operational metadata or wallet identifiers remain in backend source; if the text is required for documentation, move it to a safe external doc (e.g., a markdown changelog) or a non-production config, and run tests to confirm no references to pr_body remain.
27-33:⚠️ Potential issue | 🔴 CriticalDestructive git workflow is unsafe (
git add .+push --force) with hardcoded target branch.Line 27 stages everything indiscriminately, and Line 33 force-pushes to a hardcoded branch unrelated to this PR branch. This combination can leak unintended files and rewrite remote history, making it a high-risk automation path inside repository code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/submit_bounty.py` around lines 27 - 33, The script uses unsafe git operations: the subprocess.run calls that execute ["git", "add", "."] and ["git", "push", "origin", "bounty-169-final-certified", "--force"] in submit_bounty.py should be changed to stage only intended files (avoid "git add ."), determine or accept the target branch dynamically instead of hardcoding "bounty-169-final-certified", and avoid --force; update the code around the subprocess.run invocations (the git add/commit/push calls) to accept a whitelist or explicit file list, use the current branch name or a function parameter for the target branch, and replace --force with a safe push strategy (e.g., normal push or --force-with-lease) and/or require confirmation before destructive pushes.
1-38:⚠️ Potential issue | 🔴 CriticalCritical scope violation: this file is unrelated to PR
#797backend deliverables.
backend/submit_bounty.pyis a git automation script for a different submission context (Bounty 169 / PR#410), not part of the FastAPI backend work for Missions#80-82. Keeping this inbackend/introduces non-product automation into production backend scope and breaks API-contract-focused review boundaries.
As per coding guidelines, "backend/**: Python FastAPI backend. Analyze thoroughly: ... API contract consistency with spec."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/submit_bounty.py` around lines 1 - 38, This file backend/submit_bounty.py is an unrelated git-automation script (contains run(), logger, and git subprocess calls) and must be removed or relocated out of the backend FastAPI package: either delete backend/submit_bounty.py from this branch/PR, or move it into a dedicated tooling/automation repo or a top-level tools/ directory (preserving run() if you need the script), then amend the commit(s) to remove the file from PR `#797`, ensure no imports/reference to submit_bounty.py remain, update any CI or packaging manifests so it isn't packaged with the backend, and run the test suite/linters to confirm nothing breaks.backend/app/services/payout_service.py (7)
155-179:⚠️ Potential issue | 🟡 Minor
approve_payoutdoes not update the in-memory cache after persisting.After persisting the updated record at line 172, the
_payout_storecache is not refreshed. Subsequent reads from the cache will return stale status, causing inconsistency between cache and database.🔧 Proposed fix
record.status = PayoutStatus.APPROVED record.updated_at = datetime.now(timezone.utc) await pg_store.persist_payout(record) + with _store_lock: + _payout_store[record.id] = record return AdminApprovalResponse(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/payout_service.py` around lines 155 - 179, approve_payout updates the DB but never refreshes the in-memory cache, causing stale reads; after await pg_store.persist_payout(record) you should re-acquire the _store_lock and update _payout_store[payout_id] = record (or replace the existing entry) so the cache reflects the new status, ensuring you modify the same record object or a copy while holding the lock to avoid race conditions with concurrent readers/writers.
261-270:⚠️ Potential issue | 🟡 Minor
list_buybacksreports incorrect total count.Line 262 fetches
limit + skip + 100records, but line 267 reportstotal=len(items)which only reflects the fetched subset. For pagination beyond this window, totals will be incorrect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/payout_service.py` around lines 261 - 270, The total returned by list_buybacks is incorrect because it uses total=len(items) (the sliced subset) instead of the full set; update list_buybacks to compute the total from the full result (e.g., use len(all_records) or call a proper count method on pg_store) and pass that value into BuybackListResponse (keep the existing paging using items/page and mapping via _buyback_to_response).
181-197:⚠️ Potential issue | 🟡 Minor
reject_payoutdoes not update the in-memory cache after persisting.Same issue as
approve_payout— after persisting the rejection at line 190, the in-memory_payout_storeis not updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/payout_service.py` around lines 181 - 197, reject_payout currently persists the updated record with pg_store.persist_payout but never updates the in-memory cache, mirroring the same bug in approve_payout; after the persist call in reject_payout, update the module-level cache (e.g., _payout_store) with the new record (for example _payout_store[payout_id] = record or update the existing entry's fields) so the in-memory state matches the DB before returning the AdminApprovalResponse.
242-259:⚠️ Potential issue | 🟠 MajorBuyback duplicate detection is limited and lacks DB constraint error handling.
Lines 243-247 only scan the newest 1000 buybacks for duplicate
tx_hash.BuybackTable.tx_hashhas a UNIQUE constraint, so duplicates outside this window—or concurrent inserts—will raise an unhandled database integrity error (500) instead of the controlled 400.Wrap
pg_store.persist_buybackto catchIntegrityErrorand convert toHTTPException(400, ...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/payout_service.py` around lines 242 - 259, The duplicate-check in create_buyback currently only scans the latest 1000 records and does not handle DB UNIQUE violations; wrap the call to pg_store.persist_buyback in a try/except that catches the DB integrity exception (e.g., IntegrityError from your DB library) and raise HTTPException(status_code=400, detail="Buyback already exists") when tx_hash uniqueness is violated (referencing the BuybackTable.tx_hash constraint), then continue to update _buyback_store[record.id] and return _buyback_to_response(record) on success; keep the existing pre-check loop but add this DB-level error handling around persist_buyback to handle duplicates outside the scanned window or concurrent inserts.
110-127:⚠️ Potential issue | 🟠 MajorIn-memory filtering produces incorrect pagination totals.
Line 110 fetches a bounded slice (
limit + skip + 1000), then applies filters in-memory. Line 126 reportstotal = len(items)which only reflects the filtered subset of fetched records, not the true database count. For datasets exceeding this window:
- Pagination metadata will be incorrect
- Records beyond the fetch window are invisible
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/payout_service.py` around lines 110 - 127, The pagination is incorrect because the code fetches a bounded slice via pg_store.load_payouts(limit=limit + skip + 1000) then applies filters in-memory (variables items, recipient, status, bounty_id, token, start_date, end_date) and sets total = len(items), which only counts the filtered subset within the fetch window; fix by moving filtering into the database layer and using a proper count query: extend or replace pg_store.load_payouts to accept filter parameters (recipient, status, bounty_id, token, start_date, end_date) and an offset/limit, then call a new pg_store.count_payouts(filters) to get the true total, and finally retrieve the paged rows with pg_store.load_payouts(filters, limit, skip) so total reflects the DB count and page contains correct records.
272-294:⚠️ Potential issue | 🟠 MajorAggregate functions truncate at 10,000 records.
get_total_buybacks,get_total_paid_out,_count_confirmed_payouts, and_count_buybacksall callpg_store.load_*withlimit=10000. Once the tables grow past this size, aggregates will be understated.Use database-side aggregation (
SUM,COUNT) instead of loading records into memory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/payout_service.py` around lines 272 - 294, The four service functions get_total_buybacks, get_total_paid_out, _count_confirmed_payouts, and _count_buybacks currently call pg_store.load_buybacks/load_payouts with limit=10000 which truncates results; replace in-memory aggregation with database-side aggregates by adding and calling new pg_store methods (e.g., sum_buybacks(), sum_confirmed_payouts_by_token(token), count_confirmed_payouts(), count_buybacks()) that run SQL SUM/COUNT queries (with proper WHERE for PayoutStatus.CONFIRMED and token="FNDRY"/"SOL"), then update get_total_buybacks, get_total_paid_out, _count_confirmed_payouts, and _count_buybacks to await those pg_store aggregate methods and return the aggregated numbers.
61-94:⚠️ Potential issue | 🟠 MajorDuplicate-pay prevention is still racy and limited to 5000 records.
The bounty-level duplicate check at lines 71-75 loads only 5000 payouts from the database. This:
- Misses older payouts beyond the limit window
- Is not atomic with the subsequent insert, allowing concurrent requests to race past each other
- Does not rely on database-level uniqueness constraints
The tx_hash check at lines 65-68 only scans the in-memory cache, not the database, so duplicates in DB but not in cache will be missed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/payout_service.py` around lines 61 - 94, The duplicate-prevention in create_payout is racy and incomplete: stop relying on load_payouts(limit=5000) and the in-memory _payout_store scan and instead enforce uniqueness at the DB and handle conflicts atomically—add DB unique constraints/indexes for tx_hash (unique if non-null) and for bounty_id (unique or unique partial depending on nullable semantics), modify pg_store.persist_payout to perform the insert and surface IntegrityError/unique-constraint violations, and in create_payout call persist_payout directly (or call a new pg_store.insert_payout_if_not_exists) and catch the DB integrity exception to raise HTTPException(400) for duplicate tx_hash or bounty_id; keep the in-memory _payout_store updates as caching only, and remove/replace load_payouts usage in create_payout (references: create_payout, pg_store.load_payouts, pg_store.persist_payout, PayoutRecord, PayoutCreate).backend/app/services/pg_store.py (1)
276-280: 🧹 Nitpick | 🔵 Trivial
save_last_syncalways inserts a new row, causing unbounded table growth.Line 279 unconditionally adds a new
SyncStateTablerow on every call. Over time, this creates unbounded growth in thesync_statetable. Consider using upsert with a fixed key or deleting old rows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/pg_store.py` around lines 276 - 280, The save_last_sync function currently always inserts a new SyncStateTable row causing unbounded table growth; change save_last_sync to update or upsert the single canonical sync row instead of inserting every time: locate save_last_sync and replace the unconditional session.add(SyncStateTable(last_sync=dt)) with an upsert/update flow (e.g., query for the existing SyncStateTable row by its unique key or primary id and update its last_sync, or use the ORM/session.merge or DB-level ON CONFLICT upsert to set last_sync = :dt), or alternatively delete old rows before inserting so only one row remains; ensure this uses get_db_session() and commits as before.backend/tests/e2e/conftest.py (2)
87-95:⚠️ Potential issue | 🟠 MajorImport-time loop bootstrap is still fighting
pytest-asyncio.Line 94 globally installs a shared manual event loop, Line 127 uses it to run async schema initialization, and Line 171 does all of that during module import before pytest fixtures are active.
async_clientat Lines 247-256 then runs requests on pytest-managed loops, so the harness mixes resources created on one loop with tests executing on another. That is the same cross-loop/test-pollution failure mode already flagged earlier, and the shared loop is still never closed or cleared.Also applies to: 123-127, 171-171, 247-256
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/e2e/conftest.py` around lines 87 - 95, The code installs and reuses a global event loop at import time via _test_loop/_get_test_loop and calls asyncio.set_event_loop and runs async schema initialization before pytest fixtures, causing cross-loop pollution with pytest-asyncio and never closing the loop; fix by removing import-time loop creation and asyncio.set_event_loop usage, move any async schema initialization into an async pytest fixture (or use pytest-asyncio's built-in event_loop/async_client fixtures) so initialization runs on pytest-managed loops, ensure the created loop (if any) is closed and cleared in teardown, and update references from _get_test_loop/_test_loop to rely on the pytest event loop instead of a global loop.
30-33:⚠️ Potential issue | 🟠 MajorShared SQLite +
create_all()makes Mission#82E2E results non-representative.Line 33 overwrites
DATABASE_URLunconditionally to one temp-file SQLite database, Lines 124-125 build schema withBase.metadata.create_all(), and Lines 180-187 never truncate tables or remove that file. That reuses stale rows/schema across runs,create_all()will not evolve an existingtest_e2e.db, and this harness cannot be pointed at PostgreSQL to exercise the search behavior this PR adds (ts_rank_cd,?|/?&, trigger-drivensearch_vectorrebuild,project_namemigration/backfill). A green E2E run here does not validate the shipped Mission#82path.As per coding guidelines,
backend/migrations/V2__add_project_name_and_rebuild_search_vector.sqlrequires PostgreSQL trigger/backfill logic and weightedto_tsvectortokens fortitle,project_name,description, andskills.Also applies to: 123-127, 173-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/e2e/conftest.py` around lines 30 - 33, The test harness currently forces a single temp-file SQLite DB by unconditionally setting TEST_DB_PATH and os.environ["DATABASE_URL"] and calling Base.metadata.create_all(), which reuses stale schema/data and prevents running tests against PostgreSQL (breaking Mission `#82` validation); change conftest.py so DATABASE_URL is only set to the tempfile path when no DATABASE_URL exists in env (respect existing env to allow PostgreSQL), avoid unconditionally calling Base.metadata.create_all() for production migrations (use a per-test ephemeral DB or transactional fixtures and ensure tests truncate tables or remove the temp file between runs), and add teardown logic to delete TEST_DB_PATH or drop all tables after tests complete; refer to TEST_DB_PATH, DATABASE_URL, and Base.metadata.create_all() to locate where to implement these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/services/payout_service.py`:
- Around line 186-190: The PayoutRecord fields failure_reason and updated_at are
only set in memory by reject_payout and process_payout but never persisted
because persist_payout (and the PayoutTable schema) don't include those columns;
update the DB schema/migration to add failure_reason (text) and updated_at
(timestamp with timezone) to PayoutTable, update persist_payout to upsert these
two fields, and ensure load_payouts maps those columns back into PayoutRecord so
rejected/failed payouts retain their failure_reason and timestamps; touch the
functions persist_payout, PayoutTable definition, and load_payouts to implement
this change.
In `@backend/submit_bounty.py`:
- Around line 27-33: The three sequential subprocess.run operations (git add,
git commit, git push) can leave partial repo state if a later step fails; wrap
them in a try/except and either perform the operations on a temporary branch or
implement a rollback path: create a branch first (e.g.,
subprocess.run(["git","checkout","-b","bounty-169-final-certified"],...)), run
git add/commit/push, and on any exception run recovery commands (e.g.,
subprocess.run(["git","reset","--soft","HEAD~1"],...) to undo the commit and
subprocess.run(["git","restore","--staged","."]) to clear staging, optionally
delete the temp branch with subprocess.run(["git","checkout","-"],...) and
subprocess.run(["git","branch","-D","bounty-169-final-certified"],...), log the
error and re-raise; ensure all subprocess.run calls still use check=True and
capture output for diagnostics.
In `@backend/tests/e2e/conftest.py`:
- Around line 44-45: The clear_stores() fixture currently resets service stores
but misses resetting the WebSocket singleton; modify clear_stores() to call and
await manager.shutdown() (the imported singleton `manager` from
websocket_manager) so that
_connections/_subscriptions/_rate_buckets/_event_buffer are cleared before each
test, ensuring route-level E2E tests use a fresh WebSocketManager instance; no
changes to the separate websocket_manager fixture are needed.
---
Outside diff comments:
In `@backend/app/services/pg_store.py`:
- Around line 209-229: The load_payouts function is missing several payout
lifecycle fields and uses an unsafe enum conversion; update the PayoutRecord
construction in load_payouts to include updated_at, failure_reason, retry_count,
and admin_approved_by by reading them from the PayoutTable row (e.g.,
row.updated_at, row.failure_reason, row.retry_count, row.admin_approved_by), and
ensure persist_payout also persists those same fields; additionally replace the
brittle PayoutStatus(row.status.lower()) call with a safe parse (normalize the
DB value, attempt to construct the PayoutStatus and wrap it in a try/except or
use a mapping/fallback to a sensible default like PayoutStatus.UNKNOWN or
PayoutStatus.PENDING) so unexpected status values do not raise ValueError and
crash load_payouts.
- Around line 188-207: persist_payout omits fields that are set during state
transitions—add updated_at, failure_reason, retry_count, and admin_approved_by
to the _upsert call in persist_payout so these values from the PayoutRecord are
persisted; specifically, pass updated_at=record.updated_at,
failure_reason=record.failure_reason, retry_count=record.retry_count, and
admin_approved_by=record.admin_approved_by into the existing _upsert(...)
invocation in the persist_payout function (ensure PayoutTable schema has
corresponding columns before applying).
In `@backend/tests/e2e/conftest.py`:
- Around line 101-157: The test builds a synthetic FastAPI app in
_create_test_app (creating test_app, adding routers and a stub health_check)
which bypasses production lifespan, middleware, and real /health behavior;
replace this by instantiating the real application (use the production app
factory or module that wires lifespan/middleware/telemetry) instead of creating
test_app, remove the inline health_check stub, and keep only
dependency_overrides[get_current_user] = _mock_get_current_user (or apply the
override on the real app) so E2E clients exercise the real request pipeline
(request_id headers, real health handler that returns
healthy|degraded|unavailable, failsafe Redis behavior) while preserving
authentication override for tests.
---
Duplicate comments:
In `@backend/app/services/payout_service.py`:
- Around line 155-179: approve_payout updates the DB but never refreshes the
in-memory cache, causing stale reads; after await
pg_store.persist_payout(record) you should re-acquire the _store_lock and update
_payout_store[payout_id] = record (or replace the existing entry) so the cache
reflects the new status, ensuring you modify the same record object or a copy
while holding the lock to avoid race conditions with concurrent readers/writers.
- Around line 261-270: The total returned by list_buybacks is incorrect because
it uses total=len(items) (the sliced subset) instead of the full set; update
list_buybacks to compute the total from the full result (e.g., use
len(all_records) or call a proper count method on pg_store) and pass that value
into BuybackListResponse (keep the existing paging using items/page and mapping
via _buyback_to_response).
- Around line 181-197: reject_payout currently persists the updated record with
pg_store.persist_payout but never updates the in-memory cache, mirroring the
same bug in approve_payout; after the persist call in reject_payout, update the
module-level cache (e.g., _payout_store) with the new record (for example
_payout_store[payout_id] = record or update the existing entry's fields) so the
in-memory state matches the DB before returning the AdminApprovalResponse.
- Around line 242-259: The duplicate-check in create_buyback currently only
scans the latest 1000 records and does not handle DB UNIQUE violations; wrap the
call to pg_store.persist_buyback in a try/except that catches the DB integrity
exception (e.g., IntegrityError from your DB library) and raise
HTTPException(status_code=400, detail="Buyback already exists") when tx_hash
uniqueness is violated (referencing the BuybackTable.tx_hash constraint), then
continue to update _buyback_store[record.id] and return
_buyback_to_response(record) on success; keep the existing pre-check loop but
add this DB-level error handling around persist_buyback to handle duplicates
outside the scanned window or concurrent inserts.
- Around line 110-127: The pagination is incorrect because the code fetches a
bounded slice via pg_store.load_payouts(limit=limit + skip + 1000) then applies
filters in-memory (variables items, recipient, status, bounty_id, token,
start_date, end_date) and sets total = len(items), which only counts the
filtered subset within the fetch window; fix by moving filtering into the
database layer and using a proper count query: extend or replace
pg_store.load_payouts to accept filter parameters (recipient, status, bounty_id,
token, start_date, end_date) and an offset/limit, then call a new
pg_store.count_payouts(filters) to get the true total, and finally retrieve the
paged rows with pg_store.load_payouts(filters, limit, skip) so total reflects
the DB count and page contains correct records.
- Around line 272-294: The four service functions get_total_buybacks,
get_total_paid_out, _count_confirmed_payouts, and _count_buybacks currently call
pg_store.load_buybacks/load_payouts with limit=10000 which truncates results;
replace in-memory aggregation with database-side aggregates by adding and
calling new pg_store methods (e.g., sum_buybacks(),
sum_confirmed_payouts_by_token(token), count_confirmed_payouts(),
count_buybacks()) that run SQL SUM/COUNT queries (with proper WHERE for
PayoutStatus.CONFIRMED and token="FNDRY"/"SOL"), then update get_total_buybacks,
get_total_paid_out, _count_confirmed_payouts, and _count_buybacks to await those
pg_store aggregate methods and return the aggregated numbers.
- Around line 61-94: The duplicate-prevention in create_payout is racy and
incomplete: stop relying on load_payouts(limit=5000) and the in-memory
_payout_store scan and instead enforce uniqueness at the DB and handle conflicts
atomically—add DB unique constraints/indexes for tx_hash (unique if non-null)
and for bounty_id (unique or unique partial depending on nullable semantics),
modify pg_store.persist_payout to perform the insert and surface
IntegrityError/unique-constraint violations, and in create_payout call
persist_payout directly (or call a new pg_store.insert_payout_if_not_exists) and
catch the DB integrity exception to raise HTTPException(400) for duplicate
tx_hash or bounty_id; keep the in-memory _payout_store updates as caching only,
and remove/replace load_payouts usage in create_payout (references:
create_payout, pg_store.load_payouts, pg_store.persist_payout, PayoutRecord,
PayoutCreate).
In `@backend/app/services/pg_store.py`:
- Around line 276-280: The save_last_sync function currently always inserts a
new SyncStateTable row causing unbounded table growth; change save_last_sync to
update or upsert the single canonical sync row instead of inserting every time:
locate save_last_sync and replace the unconditional
session.add(SyncStateTable(last_sync=dt)) with an upsert/update flow (e.g.,
query for the existing SyncStateTable row by its unique key or primary id and
update its last_sync, or use the ORM/session.merge or DB-level ON CONFLICT
upsert to set last_sync = :dt), or alternatively delete old rows before
inserting so only one row remains; ensure this uses get_db_session() and commits
as before.
In `@backend/submit_bounty.py`:
- Around line 9-22: Remove the unused hardcoded pr_body string in
submit_bounty.py: delete the dead variable definition (pr_body) and any
unreachable block that defines it, ensuring no operational metadata or wallet
identifiers remain in backend source; if the text is required for documentation,
move it to a safe external doc (e.g., a markdown changelog) or a non-production
config, and run tests to confirm no references to pr_body remain.
- Around line 27-33: The script uses unsafe git operations: the subprocess.run
calls that execute ["git", "add", "."] and ["git", "push", "origin",
"bounty-169-final-certified", "--force"] in submit_bounty.py should be changed
to stage only intended files (avoid "git add ."), determine or accept the target
branch dynamically instead of hardcoding "bounty-169-final-certified", and avoid
--force; update the code around the subprocess.run invocations (the git
add/commit/push calls) to accept a whitelist or explicit file list, use the
current branch name or a function parameter for the target branch, and replace
--force with a safe push strategy (e.g., normal push or --force-with-lease)
and/or require confirmation before destructive pushes.
- Around line 1-38: This file backend/submit_bounty.py is an unrelated
git-automation script (contains run(), logger, and git subprocess calls) and
must be removed or relocated out of the backend FastAPI package: either delete
backend/submit_bounty.py from this branch/PR, or move it into a dedicated
tooling/automation repo or a top-level tools/ directory (preserving run() if you
need the script), then amend the commit(s) to remove the file from PR `#797`,
ensure no imports/reference to submit_bounty.py remain, update any CI or
packaging manifests so it isn't packaged with the backend, and run the test
suite/linters to confirm nothing breaks.
In `@backend/tests/e2e/conftest.py`:
- Around line 87-95: The code installs and reuses a global event loop at import
time via _test_loop/_get_test_loop and calls asyncio.set_event_loop and runs
async schema initialization before pytest fixtures, causing cross-loop pollution
with pytest-asyncio and never closing the loop; fix by removing import-time loop
creation and asyncio.set_event_loop usage, move any async schema initialization
into an async pytest fixture (or use pytest-asyncio's built-in
event_loop/async_client fixtures) so initialization runs on pytest-managed
loops, ensure the created loop (if any) is closed and cleared in teardown, and
update references from _get_test_loop/_test_loop to rely on the pytest event
loop instead of a global loop.
- Around line 30-33: The test harness currently forces a single temp-file SQLite
DB by unconditionally setting TEST_DB_PATH and os.environ["DATABASE_URL"] and
calling Base.metadata.create_all(), which reuses stale schema/data and prevents
running tests against PostgreSQL (breaking Mission `#82` validation); change
conftest.py so DATABASE_URL is only set to the tempfile path when no
DATABASE_URL exists in env (respect existing env to allow PostgreSQL), avoid
unconditionally calling Base.metadata.create_all() for production migrations
(use a per-test ephemeral DB or transactional fixtures and ensure tests truncate
tables or remove the temp file between runs), and add teardown logic to delete
TEST_DB_PATH or drop all tables after tests complete; refer to TEST_DB_PATH,
DATABASE_URL, and Base.metadata.create_all() to locate where to implement these
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ccc03247-c012-42f5-aea3-239b74812f03
📒 Files selected for processing (5)
backend/app/services/payout_service.pybackend/app/services/pg_store.pybackend/submit_bounty.pybackend/tests/e2e/conftest.pybackend/verify_middleware.py
|
|
||
| record.status = PayoutStatus.FAILED | ||
| record.admin_approved_by = admin_id | ||
| record.failure_reason = reason | ||
| record.updated_at = datetime.now(timezone.utc) | ||
| await pg_store.persist_payout(record) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what fields persist_payout writes vs what PayoutRecord/PayoutTable have
echo "=== Fields written by persist_payout ==="
rg -A 20 'async def persist_payout' backend/app/services/pg_store.py | grep -E '^\s+\w+=' | head -20
echo ""
echo "=== PayoutTable columns ==="
ast-grep --pattern 'class PayoutTable($_) {
$$$
}'
rg -A 40 'class PayoutTable' backend/app/models/tables.py | grep -E '^\s+\w+\s*=\s*Column'
echo ""
echo "=== PayoutRecord fields ==="
rg -A 50 'class PayoutRecord' backend/app/models/payout.py | grep -E '^\s+\w+:' | head -20Repository: SolFoundry/solfoundry
Length of output: 1781
🏁 Script executed:
#!/bin/bash
# Get complete PayoutTable definition
echo "=== Complete PayoutTable definition ==="
rg -A 30 'class PayoutTable' backend/app/models/tables.py
echo ""
echo "=== Complete PayoutRecord definition ==="
rg -A 30 'class PayoutRecord' backend/app/models/payout.py
echo ""
echo "=== payout_service.py lines 186-230 (context around the issue) ==="
sed -n '186,230p' backend/app/services/payout_service.py
echo ""
echo "=== persist_payout full implementation ==="
rg -A 40 'async def persist_payout' backend/app/services/pg_store.pyRepository: SolFoundry/solfoundry
Length of output: 5912
failure_reason and updated_at are set but never persisted to PostgreSQL.
Lines 188–189 in reject_payout set record.failure_reason and record.updated_at, and lines 224, 227, and 231 in process_payout do the same. However, persist_payout only upserts 10 fields: recipient, recipient_wallet, amount, token, bounty_id, bounty_title, tx_hash, status, solscan_url, and created_at.
Critically, the PayoutTable schema itself has no updated_at or failure_reason columns—these fields exist only on PayoutRecord in memory. When either function calls persist_payout, the in-memory updates are lost, and load_payouts cannot reconstruct them from the database.
This causes rejected/failed payouts to have NULL failure reasons when reloaded, and all status transition timestamps are lost. The issue affects all three persist_payout calls: after rejection, after status transition to PROCESSING, and after on-chain transfer completion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/payout_service.py` around lines 186 - 190, The
PayoutRecord fields failure_reason and updated_at are only set in memory by
reject_payout and process_payout but never persisted because persist_payout (and
the PayoutTable schema) don't include those columns; update the DB
schema/migration to add failure_reason (text) and updated_at (timestamp with
timezone) to PayoutTable, update persist_payout to upsert these two fields, and
ensure load_payouts maps those columns back into PayoutRecord so rejected/failed
payouts retain their failure_reason and timestamps; touch the functions
persist_payout, PayoutTable definition, and load_payouts to implement this
change.
backend/tests/e2e/conftest.py
Outdated
| from app.services.websocket_manager import manager as ws_manager, WebSocketManager, InMemoryPubSubAdapter # noqa: E402 | ||
| from tests.e2e.factories import DEFAULT_WALLET # noqa: E402 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the conftest.py file to see the imports and fixture setup
fd conftest.py -t f | head -20Repository: SolFoundry/solfoundry
Length of output: 120
🏁 Script executed:
# Let's find and read the conftest.py file
find . -name conftest.py -path "*/tests/e2e/*" -type fRepository: SolFoundry/solfoundry
Length of output: 96
🏁 Script executed:
# Check if the file exists at the expected location
ls -la backend/tests/e2e/conftest.py 2>/dev/null || echo "File not found"Repository: SolFoundry/solfoundry
Length of output: 144
🏁 Script executed:
# Find any conftest.py files
git ls-files | grep conftestRepository: SolFoundry/solfoundry
Length of output: 120
🏁 Script executed:
# Read the e2e test conftest file
cat -n backend/tests/e2e/conftest.py | head -100Repository: SolFoundry/solfoundry
Length of output: 4582
🏁 Script executed:
# Read more of conftest.py to see the fixtures and clear_stores() function
cat -n backend/tests/e2e/conftest.py | sed -n '100,200p'Repository: SolFoundry/solfoundry
Length of output: 4907
🏁 Script executed:
# Read the rest of conftest.py
cat -n backend/tests/e2e/conftest.py | sed -n '200,350p'Repository: SolFoundry/solfoundry
Length of output: 5737
🏁 Script executed:
# Also read the websocket_manager.py to understand the state
cat -n backend/app/services/websocket_manager.py | sed -n '1,100p'Repository: SolFoundry/solfoundry
Length of output: 4363
🏁 Script executed:
# Read more of websocket_manager.py to understand the state variables and manager structure
cat -n backend/app/services/websocket_manager.py | sed -n '100,250p'Repository: SolFoundry/solfoundry
Length of output: 5939
🏁 Script executed:
# Continue reading to find the shutdown method and confirm state variables
cat -n backend/app/services/websocket_manager.py | sed -n '250,400p'Repository: SolFoundry/solfoundry
Length of output: 6704
🏁 Script executed:
# Check if ws_manager is used in the app routes
grep -r "ws_manager\|websocket_manager" backend/app/api/ --include="*.py" | head -20Repository: SolFoundry/solfoundry
Length of output: 337
🏁 Script executed:
# Check the websocket router to see which manager instance is used
cat -n backend/app/api/websocket.py | head -100Repository: SolFoundry/solfoundry
Length of output: 3843
🏁 Script executed:
# Search for shutdown calls in test conftest files
grep -r "shutdown" backend/tests/ --include="*.py"Repository: SolFoundry/solfoundry
Length of output: 47
🏁 Script executed:
# Check how the manager singleton is instantiated
grep -n "^manager\s*=" backend/app/services/websocket_manager.pyRepository: SolFoundry/solfoundry
Length of output: 97
🏁 Script executed:
# Check if there are WebSocket E2E tests that use the manager
grep -r "websocket\|ws\|manager" backend/tests/e2e/ --include="*.py" | grep -E "(def test_|async def test_)" | head -20Repository: SolFoundry/solfoundry
Length of output: 959
🏁 Script executed:
# Search for test files that might test WebSocket functionality
find backend/tests/e2e -name "*.py" -type f | xargs grep -l "websocket\|/ws"Repository: SolFoundry/solfoundry
Length of output: 137
🏁 Script executed:
# Check if there's a lifespan or startup/shutdown in the test app
cat -n backend/tests/e2e/conftest.py | sed -n '150,175p'Repository: SolFoundry/solfoundry
Length of output: 1117
🏁 Script executed:
# Read the WebSocket test file to see how tests use the manager
cat -n backend/tests/e2e/test_websocket_events.py | head -150Repository: SolFoundry/solfoundry
Length of output: 6414
🏁 Script executed:
# Check if websocket_manager fixture is actually used in tests
grep -A 5 "def test_" backend/tests/e2e/test_websocket_events.py | head -80Repository: SolFoundry/solfoundry
Length of output: 3229
🏁 Script executed:
# Look at the route-level WebSocket tests that use client fixture
cat -n backend/tests/e2e/test_websocket_events.py | grep -A 30 "def test_websocket_connect_and_subscribe"Repository: SolFoundry/solfoundry
Length of output: 1644
🏁 Script executed:
# See all route-level tests (using client) vs unit tests (using websocket_manager fixture)
grep -E "def test_.*\(self, (client|websocket_manager)" backend/tests/e2e/test_websocket_events.py | head -30Repository: SolFoundry/solfoundry
Length of output: 550
🏁 Script executed:
# Check what the client-based tests actually test
cat -n backend/tests/e2e/test_websocket_events.py | sed -n '200,350p'Repository: SolFoundry/solfoundry
Length of output: 6543
🏁 Script executed:
# Check if there are route-level tests that might be affected
grep -n "def test_websocket" backend/tests/e2e/test_websocket_events.py | tail -20Repository: SolFoundry/solfoundry
Length of output: 485
🏁 Script executed:
# Verify the actual singleton manager is used by the app and not overridable
grep -n "manager\|ws_manager" backend/app/api/websocket.pyRepository: SolFoundry/solfoundry
Length of output: 533
🏁 Script executed:
# Check if manager.shutdown() is ever called anywhere
grep -rn "shutdown()" backend/tests/ --include="*.py"Repository: SolFoundry/solfoundry
Length of output: 47
WebSocket singleton state is not reset by clear_stores(), causing test isolation failures for route-level E2E tests.
The singleton manager imported at line 44 and used directly by routes in backend/app/api/websocket.py (lines 28, 31, 35, 42, 78, 115) retains _connections, _subscriptions, _rate_buckets, and _event_buffer between tests. The clear_stores() fixture (lines 173-188) only resets service-specific stores and never calls manager.shutdown(), which is the only method that clears singleton state (websocket_manager.py lines 197-209).
The websocket_manager fixture at lines 264-273 creates a separate instance and does not affect the singleton used by the actual routes. This leaves route-level WebSocket tests—test_websocket_connect_and_subscribe, test_websocket_reject_invalid_token, test_websocket_ping_pong, test_websocket_subscribe_and_broadcast, test_websocket_events_status_endpoint, test_websocket_events_type_endpoint—vulnerable to cross-test contamination from prior connection or subscription state.
Fix: Add await manager.shutdown() to clear_stores() to reset the singleton before each test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/e2e/conftest.py` around lines 44 - 45, The clear_stores()
fixture currently resets service stores but misses resetting the WebSocket
singleton; modify clear_stores() to call and await manager.shutdown() (the
imported singleton `manager` from websocket_manager) so that
_connections/_subscriptions/_rate_buckets/_event_buffer are cleared before each
test, ensuring route-level E2E tests use a fresh WebSocketManager instance; no
changes to the separate websocket_manager fixture are needed.
…olation and pydantic schema
The UNIQUE constraint on tx_hash was failing because previous test data persisted in the shared SQLite database. Added real table deletion to clear_stores fixture.
…s tests and services
…and ruff linting in main.py
…, postgres credentials)
…aware dates, robust UUID handling, and test-compatibility fixes (PR SolFoundry#797)
…configuration, and stabilize test environment (PR SolFoundry#797)
…ed Anchor workspace, and zero-hardcode audit (PR SolFoundry#797)
…ructure, fix backend test leakage, and clear Ruff lint
1 participant
Mission #80-82: SolFoundry Elite Infrastructure Upgrade (T1)
This PR completes three Tier 1 bounties in a single comprehensive sweep, upgrading the platform's health monitoring, visual labeling system, and search capabilities to Elite standards.
🚀 Overview of Changes
🟢 Mission #80: Health Check Upgrade
healthy,degraded,unavailable.🟢 Mission #81: Frontend Labels Upgrade
BountyTagcomponent.SkillTagsandBountyCard.🔍 Mission #82: Elite Search Engine
project_nameto DB and Models, syncing with frontend expectations.skills_logic(AND/OR) support via PostgreSQL?&and?|.deadline_beforefiltering in the API router.🛠 Verification
Wallet:
BeQcMrhy5ujhakN96FDbjHnV5f844yZHq1s5AyapQSek