Fix blockchain integrity logic and map cache invalidation#384
Fix blockchain integrity logic and map cache invalidation#384RohanExploit wants to merge 3 commits intomainfrom
Conversation
- Added `previous_integrity_hash` to `Issue` model for robust blockchain verification. - Updated `create_issue` to populate `previous_integrity_hash` and clear `nearby_issues_cache`. - Updated `verify_blockchain_integrity` to perform robust two-step verification (internal + link). - Added database migration to `init_db.py`. Co-authored-by: RohanExploit <178623867+RohanExploit@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
🙏 Thank you for your contribution, @RohanExploit!PR Details:
Quality Checklist:
Review Process:
Note: The maintainers will monitor code quality and ensure the overall project flow isn't broken. |
✅ Deploy Preview for fixmybharat canceled.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 5 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/routers/issues.py">
<violation number="1" location="backend/routers/issues.py:626">
P1: Backward-incompatible change: pre-existing records have `previous_integrity_hash = NULL` (no backfill in migration), so `or ""` replaces the real predecessor hash with an empty string. This causes `computed_hash` to differ from `integrity_hash` for every old record that had a predecessor, producing false "tampered" results.
When `previous_integrity_hash` is `None`, fall back to querying the predecessor's hash from the DB (the old behavior).</violation>
</file>
<file name="tests/reproduce_blockchain_enhanced.py">
<violation number="1" location="tests/reproduce_blockchain_enhanced.py:85">
P1: Incorrect assertion: after deleting Issue B, Issue A (id=1) still exists as a predecessor of C (id=3). The predecessor query (`WHERE id < 3 ORDER BY id DESC LIMIT 1`) returns A, so the status will be `"Broken (Link Mismatch)"`, not `"Broken (Predecessor Missing)"`. This test will fail unconditionally.</violation>
</file>
<file name="backend/init_db.py">
<violation number="1" location="backend/init_db.py:137">
P3: Inconsistent logging: this index creation uses `print()` while nearly all other index creation blocks in this function use `logger.info()`. Use `logger.info()` for consistency.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| prev_hash = prev_issue_hash[0] if prev_issue_hash and prev_issue_hash[0] else "" | ||
| # Use stored previous hash to verify the chain | ||
| prev_hash = current_issue.previous_integrity_hash or "" |
There was a problem hiding this comment.
P1: Backward-incompatible change: pre-existing records have previous_integrity_hash = NULL (no backfill in migration), so or "" replaces the real predecessor hash with an empty string. This causes computed_hash to differ from integrity_hash for every old record that had a predecessor, producing false "tampered" results.
When previous_integrity_hash is None, fall back to querying the predecessor's hash from the DB (the old behavior).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/routers/issues.py, line 626:
<comment>Backward-incompatible change: pre-existing records have `previous_integrity_hash = NULL` (no backfill in migration), so `or ""` replaces the real predecessor hash with an empty string. This causes `computed_hash` to differ from `integrity_hash` for every old record that had a predecessor, producing false "tampered" results.
When `previous_integrity_hash` is `None`, fall back to querying the predecessor's hash from the DB (the old behavior).</comment>
<file context>
@@ -613,34 +615,59 @@ async def verify_blockchain_integrity(issue_id: int, db: Session = Depends(get_d
-
- prev_hash = prev_issue_hash[0] if prev_issue_hash and prev_issue_hash[0] else ""
+ # Use stored previous hash to verify the chain
+ prev_hash = current_issue.previous_integrity_hash or ""
# Recompute hash based on current data and previous hash
</file context>
| prev_hash = current_issue.previous_integrity_hash or "" | |
| if current_issue.previous_integrity_hash is not None: | |
| prev_hash = current_issue.previous_integrity_hash | |
| else: | |
| # Fallback for records created before the previous_integrity_hash column was added | |
| prev_issue_hash = await run_in_threadpool( | |
| lambda: db.query(Issue.integrity_hash).filter(Issue.id < issue_id).order_by(Issue.id.desc()).first() | |
| ) | |
| prev_hash = prev_issue_hash[0] if prev_issue_hash and prev_issue_hash[0] else "" |
| valid, status = verify_issue(id_c) | ||
| print(f" Valid: {valid}, Status: {status}") | ||
| assert valid == True | ||
| assert status == "Broken (Predecessor Missing)" |
There was a problem hiding this comment.
P1: Incorrect assertion: after deleting Issue B, Issue A (id=1) still exists as a predecessor of C (id=3). The predecessor query (WHERE id < 3 ORDER BY id DESC LIMIT 1) returns A, so the status will be "Broken (Link Mismatch)", not "Broken (Predecessor Missing)". This test will fail unconditionally.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/reproduce_blockchain_enhanced.py, line 85:
<comment>Incorrect assertion: after deleting Issue B, Issue A (id=1) still exists as a predecessor of C (id=3). The predecessor query (`WHERE id < 3 ORDER BY id DESC LIMIT 1`) returns A, so the status will be `"Broken (Link Mismatch)"`, not `"Broken (Predecessor Missing)"`. This test will fail unconditionally.</comment>
<file context>
@@ -0,0 +1,91 @@
+valid, status = verify_issue(id_c)
+print(f" Valid: {valid}, Status: {status}")
+assert valid == True
+assert status == "Broken (Predecessor Missing)"
+
+print("\nTest PASSED")
</file context>
| # Add index on previous_integrity_hash | ||
| try: | ||
| conn.execute(text("CREATE INDEX ix_issues_previous_integrity_hash ON issues (previous_integrity_hash)")) | ||
| print("Migrated database: Added index on previous_integrity_hash column.") |
There was a problem hiding this comment.
P3: Inconsistent logging: this index creation uses print() while nearly all other index creation blocks in this function use logger.info(). Use logger.info() for consistency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/init_db.py, line 137:
<comment>Inconsistent logging: this index creation uses `print()` while nearly all other index creation blocks in this function use `logger.info()`. Use `logger.info()` for consistency.</comment>
<file context>
@@ -124,6 +124,20 @@ def migrate_db():
+ # Add index on previous_integrity_hash
+ try:
+ conn.execute(text("CREATE INDEX ix_issues_previous_integrity_hash ON issues (previous_integrity_hash)"))
+ print("Migrated database: Added index on previous_integrity_hash column.")
+ except Exception:
+ pass
</file context>
| print("Migrated database: Added index on previous_integrity_hash column.") | |
| logger.info("Migrated database: Added index on previous_integrity_hash column.") |
- Made `python-magic` import optional in `backend/utils.py` to prevent crashes if libmagic is missing. - Removed `python-magic` from `requirements-render.txt` to fix Render deployment build failure. - Confirmed `google-generativeai` and `pywebpush` are present in requirements. Co-authored-by: RohanExploit <178623867+RohanExploit@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the backend’s “blockchain-style” integrity chaining for Issue records by persisting an explicit previous_integrity_hash, and adjusts cache invalidation so map-related caches refresh after issue creation.
Changes:
- Add
previous_integrity_hashto theIssuemodel and attempt to migrate existing DBs to include the new column + index. - Update issue creation to store
previous_integrity_hashand clear bothrecent_issues_cacheandnearby_issues_cache. - Update blockchain verification to validate internal integrity against the stored previous hash and detect broken/missing predecessor links.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
backend/models.py |
Adds previous_integrity_hash column (indexed) to Issue. |
backend/init_db.py |
Adds migration steps for the new column and its index. |
backend/routers/issues.py |
Stores previous_integrity_hash on creation; clears map cache; updates verification logic. |
tests/reproduce_blockchain_enhanced.py |
Adds a standalone reproduction script for the enhanced chaining behavior. |
test_blockchain_enhanced.db |
Adds a binary SQLite DB artifact (should not be committed). |
Comments suppressed due to low confidence (1)
backend/routers/issues.py:674
- PR description mentions reporting detailed chain status (Intact / Link Mismatch / Predecessor Missing), but the API response model only returns is_valid/current_hash/computed_hash/message. Since chain_status/link_valid are computed here but not returned, clients can’t reliably distinguish “intact” vs “broken link” beyond parsing message text. Either add an explicit status field to BlockchainVerificationResponse (and return it here) or update the PR description/behavior to match the existing response contract.
# Step 2: Chain Link Consistency Check
link_valid = True
chain_status = "Intact"
# Try to find the immediate predecessor
predecessor = await run_in_threadpool(
lambda: db.query(Issue.integrity_hash).filter(Issue.id < issue_id).order_by(Issue.id.desc()).first()
)
if predecessor:
# Predecessor exists, check if hash matches our stored previous hash
# Use index access for Row object consistency
if predecessor[0] != prev_hash:
chain_status = "Broken (Link Mismatch)"
link_valid = False
elif prev_hash:
# No predecessor found in DB, but we have a previous hash stored.
# This means the predecessor was deleted.
chain_status = "Broken (Predecessor Missing)"
link_valid = False
# Else: No predecessor and no prev_hash (Genesis block) -> Intact
if internal_valid and link_valid:
message = "Integrity verified. This report is cryptographically sealed and chain is intact."
elif internal_valid and not link_valid:
if chain_status == "Broken (Predecessor Missing)":
message = "Internal integrity verified. Previous link is missing (likely deleted), but this record is safe."
else:
message = "Internal integrity verified, but chain link is broken! Previous block hash mismatch."
else:
message = "Integrity check failed! The report data does not match its cryptographic seal."
# We return internal validity as primary 'is_valid' to avoid false negatives on deletions
return BlockchainVerificationResponse(
is_valid=internal_valid,
current_hash=current_issue.integrity_hash,
computed_hash=computed_hash,
message=message
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Use stored previous hash to verify the chain | ||
| prev_hash = current_issue.previous_integrity_hash or "" | ||
|
|
||
| # Recompute hash based on current data and previous hash | ||
| # Chaining logic: hash(description|category|prev_hash) | ||
| hash_content = f"{current_issue.description}|{current_issue.category}|{prev_hash}" | ||
| computed_hash = hashlib.sha256(hash_content.encode()).hexdigest() |
There was a problem hiding this comment.
verify_blockchain_integrity now recomputes using current_issue.previous_integrity_hash unconditionally. For any pre-migration rows (or tests/seed data) where previous_integrity_hash is NULL, this will recompute with an empty prev_hash and incorrectly fail integrity verification for all non-genesis blocks. Consider falling back to the legacy behavior (derive prev_hash from the DB predecessor) when previous_integrity_hash is NULL, and treat link validation as “unknown/legacy” in that case (or backfill the column during migration).
| print("Migrated database: Added index on previous_integrity_hash column.") | ||
| except Exception: | ||
| pass | ||
|
|
There was a problem hiding this comment.
The migration adds previous_integrity_hash but doesn’t backfill it for existing rows. Because verification now depends on this stored value, all existing non-genesis issues will fail integrity checks (or show link mismatches) until the column is populated. Add a one-time backfill step (e.g., iterate issues in id order and set previous_integrity_hash to the prior row’s integrity_hash where it’s NULL) or ensure the verification endpoint falls back for legacy rows.
| # Backfill previous_integrity_hash for existing issues | |
| try: | |
| result = conn.execute( | |
| text( | |
| "SELECT id, integrity_hash " | |
| "FROM issues " | |
| "WHERE integrity_hash IS NOT NULL " | |
| "ORDER BY id" | |
| ) | |
| ) | |
| previous_hash = None | |
| for row in result: | |
| current_hash = row.integrity_hash | |
| if previous_hash is not None: | |
| conn.execute( | |
| text( | |
| "UPDATE issues " | |
| "SET previous_integrity_hash = :prev_hash " | |
| "WHERE id = :id AND previous_integrity_hash IS NULL" | |
| ), | |
| {"prev_hash": previous_hash, "id": row.id}, | |
| ) | |
| previous_hash = current_hash | |
| print("Migrated database: Backfilled previous_integrity_hash for existing issues.") | |
| except Exception: | |
| # If backfill fails, leave existing data untouched; verification should handle missing values gracefully. | |
| pass |
| from datetime import datetime | ||
|
|
||
| # Setup test DB | ||
| DB_FILE = "test_blockchain_enhanced.db" | ||
| if os.path.exists(DB_FILE): | ||
| os.remove(DB_FILE) |
There was a problem hiding this comment.
This script imports datetime but never uses it, and it writes a fixed DB file name into the repo working directory. Using tempfile (or at least a unique temporary path) will avoid leaving behind files when the script fails midway and will prevent collisions when running multiple times in parallel.
| from datetime import datetime | |
| # Setup test DB | |
| DB_FILE = "test_blockchain_enhanced.db" | |
| if os.path.exists(DB_FILE): | |
| os.remove(DB_FILE) | |
| import tempfile | |
| # Setup test DB | |
| db_fd, DB_FILE = tempfile.mkstemp(prefix="test_blockchain_enhanced_", suffix=".db") | |
| os.close(db_fd) |
| try: | ||
| conn.execute(text("ALTER TABLE issues ADD COLUMN previous_integrity_hash VARCHAR")) | ||
| print("Migrated database: Added previous_integrity_hash column.") | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| try: | ||
| conn.execute(text("CREATE INDEX ix_issues_previous_integrity_hash ON issues (previous_integrity_hash)")) | ||
| print("Migrated database: Added index on previous_integrity_hash column.") | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/utils.py">
<violation number="1" location="backend/utils.py:129">
P2: Bug: `except Exception as pil_error` catches all exceptions including `HTTPException`, making the two subsequent `except` blocks (lines 132–137) unreachable dead code. If an `HTTPException` is raised inside this try block, it will be caught here and incorrectly re-wrapped with a generic "Invalid image file" message.
Add `except HTTPException: raise` before this catch-all, and remove the now-orphaned handlers below.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ) | ||
| return img | ||
|
|
||
| except Exception as pil_error: |
There was a problem hiding this comment.
P2: Bug: except Exception as pil_error catches all exceptions including HTTPException, making the two subsequent except blocks (lines 132–137) unreachable dead code. If an HTTPException is raised inside this try block, it will be caught here and incorrectly re-wrapped with a generic "Invalid image file" message.
Add except HTTPException: raise before this catch-all, and remove the now-orphaned handlers below.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/utils.py, line 129:
<comment>Bug: `except Exception as pil_error` catches all exceptions including `HTTPException`, making the two subsequent `except` blocks (lines 132–137) unreachable dead code. If an `HTTPException` is raised inside this try block, it will be caught here and incorrectly re-wrapped with a generic "Invalid image file" message.
Add `except HTTPException: raise` before this catch-all, and remove the now-orphaned handlers below.</comment>
<file context>
@@ -71,58 +74,64 @@ def _validate_uploaded_file_sync(file: UploadFile) -> Optional[Image.Image]:
- )
+ return img
+
+ except Exception as pil_error:
+ logger.error(f"PIL validation failed for {file.filename}: {pil_error}")
+ raise HTTPException(
</file context>
- Removed `python-magic` from `backend/requirements.txt` to prevent build failures if the user overrides the build command. - Verified key module imports (`gemini_services`, `tasks`, `hf_api_service`) work correctly in the build environment. Co-authored-by: RohanExploit <178623867+RohanExploit@users.noreply.github.com>
🔍 Quality Reminder |
1 participant
This PR addresses issues with the blockchain verification logic where deleting an intermediate record would break the verification of subsequent records. It introduces a
previous_integrity_hashcolumn to theIssuemodel, allowing verification to check the explicit link to the previous record's hash.Changes:
previous_integrity_hashcolumn toIssuemodel (backend/models.py).backend/init_db.pyto add the new column and index if missing.create_issueinbackend/routers/issues.pyto:previous_integrity_hash.nearby_issues_cacheto ensure map data is fresh.verify_blockchain_integrityinbackend/routers/issues.pyto:hash(data + stored_prev_hash) == stored_hash).stored_prev_hash == actual_predecessor_hash).Verified with
tests/reproduce_blockchain_final.py.PR created automatically by Jules for task 8952080502823438387 started by @RohanExploit
Summary by cubic
Fixes blockchain verification by linking each issue to the previous block and adds two-step integrity checks. Also clears the nearby map cache on issue creation and removes/relaxes python-magic to prevent builds failing when libmagic is missing.
Bug Fixes
Migration
Written for commit efd381b. Summary will update on new commits.