⚡ Bolt: O(1) Blockchain Verification for Officer Visits#601
⚡ Bolt: O(1) Blockchain Verification for Officer Visits#601RohanExploit wants to merge 4 commits intomainfrom
Conversation
This change implements a cryptographically chained blockchain for Field Officer Visit records, enabling O(1) single-record integrity verification.
Key improvements:
- Added `previous_visit_hash` to `FieldOfficerVisit` model for chaining.
- Implemented O(1) chain-head retrieval using `visit_last_hash_cache`.
- Updated `officer_check_in` to automatically link new visits to the previous record's hash.
- Added `/api/field-officer/{visit_id}/blockchain-verify` for fast, efficient integrity checks.
- Normalized timestamps to ensure deterministic hashing across different database backends.
- Optimized database migration to add new columns and indexes safely.
|
👋 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. |
✅ Deploy Preview for fixmybharat canceled.
|
🙏 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. |
📝 WalkthroughWalkthroughAdds chained cryptographic hashing for field-officer visits: timestamps are normalized, visit hashes now include Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Router as API Router
participant Cache as visit_last_hash_cache
participant DB as Database
participant HashGen as Hash Generator
Client->>Router: POST /field-officer/check-in
Router->>Cache: get(last_hash for officer)
alt Cache Hit
Cache-->>Router: previous_visit_hash
else Cache Miss
Router->>DB: query last visit for officer
DB-->>Router: last visit (previous_visit_hash)
end
Router->>HashGen: generate_visit_hash(visit_data + previous_visit_hash)
HashGen-->>Router: visit_hash
Router->>DB: insert FieldOfficerVisit with visit_hash & previous_visit_hash
DB-->>Router: saved visit
Router->>Cache: set(last_hash = visit_hash)
Router-->>Client: response with visit_hash & previous_visit_hash
Client->>Router: GET /field-officer/{visit_id}/blockchain-verify
Router->>DB: fetch visit (data, previous_visit_hash, visit_hash)
DB-->>Router: visit record
Router->>HashGen: compute_hash(visit_data + previous_visit_hash)
HashGen-->>Router: computed_hash
Router->>Router: compare computed_hash == stored visit_hash
Router-->>Client: BlockchainVerificationResponse (is_valid, computed_hash, stored_hash)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/geofencing_service.py">
<violation number="1" location="backend/geofencing_service.py:114">
P1: Datetime normalization is incomplete: only `+` timezone offsets are stripped, so `-HH:MM`/`Z` formats can generate different hashes for the same instant.</violation>
</file>
<file name="backend/routers/field_officer.py">
<violation number="1" location="backend/routers/field_officer.py:104">
P1: Race condition: concurrent check-ins both read the same `prev_hash` from the cache before either commits, forking the blockchain chain. The read → compute → commit → cache-update sequence is not atomic. Consider using a database-level lock (e.g., `SELECT … FOR UPDATE` on PostgreSQL) or an application-level mutex around the entire chain-append operation.</violation>
<violation number="2" location="backend/routers/field_officer.py:563">
P1: Missing `except HTTPException: raise` before the generic `except Exception` handler. The 404 for a missing visit will be swallowed and returned as a 500 error to the client.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if '+' in check_in_time_str: | ||
| check_in_time_str = check_in_time_str.split('+')[0] |
There was a problem hiding this comment.
P1: Datetime normalization is incomplete: only + timezone offsets are stripped, so -HH:MM/Z formats can generate different hashes for the same instant.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/geofencing_service.py, line 114:
<comment>Datetime normalization is incomplete: only `+` timezone offsets are stripped, so `-HH:MM`/`Z` formats can generate different hashes for the same instant.</comment>
<file context>
@@ -105,11 +105,14 @@ def generate_visit_hash(visit_data: dict) -> str:
+ check_in_time_str = check_in_time.strftime('%Y-%m-%dT%H:%M:%S')
else:
check_in_time_str = str(check_in_time) if check_in_time else ""
+ if '+' in check_in_time_str:
+ check_in_time_str = check_in_time_str.split('+')[0]
</file context>
| if '+' in check_in_time_str: | |
| check_in_time_str = check_in_time_str.split('+')[0] | |
| if check_in_time_str.endswith('Z'): | |
| check_in_time_str = check_in_time_str[:-1] | |
| else: | |
| for tz_sep in ['+', '-']: | |
| if tz_sep in check_in_time_str[10:]: | |
| check_in_time_str = check_in_time_str.rsplit(tz_sep, 1)[0] | |
| break |
|
|
||
| # Blockchain feature: retrieve previous hash for chaining | ||
| # Use cache for O(1) retrieval | ||
| prev_hash = visit_last_hash_cache.get("last_hash") |
There was a problem hiding this comment.
P1: Race condition: concurrent check-ins both read the same prev_hash from the cache before either commits, forking the blockchain chain. The read → compute → commit → cache-update sequence is not atomic. Consider using a database-level lock (e.g., SELECT … FOR UPDATE on PostgreSQL) or an application-level mutex around the entire chain-append operation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/routers/field_officer.py, line 104:
<comment>Race condition: concurrent check-ins both read the same `prev_hash` from the cache before either commits, forking the blockchain chain. The read → compute → commit → cache-update sequence is not atomic. Consider using a database-level lock (e.g., `SELECT … FOR UPDATE` on PostgreSQL) or an application-level mutex around the entire chain-append operation.</comment>
<file context>
@@ -93,18 +96,29 @@ def officer_check_in(request: OfficerCheckInRequest, db: Session = Depends(get_d
+
+ # Blockchain feature: retrieve previous hash for chaining
+ # Use cache for O(1) retrieval
+ prev_hash = visit_last_hash_cache.get("last_hash")
+ if prev_hash is None:
+ # Cache miss: Fetch from DB
</file context>
| message=message | ||
| ) | ||
|
|
||
| except Exception as e: |
There was a problem hiding this comment.
P1: Missing except HTTPException: raise before the generic except Exception handler. The 404 for a missing visit will be swallowed and returned as a 500 error to the client.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/routers/field_officer.py, line 563:
<comment>Missing `except HTTPException: raise` before the generic `except Exception` handler. The 404 for a missing visit will be swallowed and returned as a 500 error to the client.</comment>
<file context>
@@ -478,9 +501,65 @@ def verify_visit(
+ message=message
+ )
+
+ except Exception as e:
+ logger.error(f"Error verifying visit blockchain for {visit_id}: {e}", exc_info=True)
+ raise HTTPException(status_code=500, detail="Failed to verify visit integrity")
</file context>
| except Exception as e: | |
| except HTTPException: | |
| raise | |
| except Exception as e: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/test_visit_blockchain.py (2)
13-13: Test database file not cleaned up after tests.The SQLite test database file
test_blockchain.dbis created on disk but not removed after tests complete. Consider using an in-memory database or adding cleanup.♻️ Use in-memory SQLite for faster, cleaner tests
-TEST_SQLALCHEMY_DATABASE_URL = "sqlite:///./test_blockchain.db" +TEST_SQLALCHEMY_DATABASE_URL = "sqlite:///:memory:"Note: If using in-memory DB, you'll need to ensure the same connection is reused. Alternatively, add cleanup in the fixture:
`@pytest.fixture` def test_db(): engine = create_engine(TEST_SQLALCHEMY_DATABASE_URL, connect_args={"check_same_thread": False}) TestingSessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine) Base.metadata.drop_all(bind=engine) Base.metadata.create_all(bind=engine) db = TestingSessionLocal() try: yield db finally: db.close() # Cleanup file if using file-based SQLite import os if os.path.exists("./test_blockchain.db"): os.remove("./test_blockchain.db")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_visit_blockchain.py` at line 13, The tests create a file-backed SQLite DB via TEST_SQLALCHEMY_DATABASE_URL ("sqlite:///./test_blockchain.db") and never clean it up; change the test fixture that creates the DB (e.g., the test_db fixture which calls create_engine/TestingSessionLocal and Base.metadata.create_all) to use an in-memory SQLite URL ("sqlite:///:memory:") with a single shared connection OR add teardown logic to close the session and remove "./test_blockchain.db" after tests complete (use os.path.exists + os.remove) to ensure the file is deleted; ensure connect_args={"check_same_thread": False} and session/engine reuse if switching to in-memory so tests see the same schema.
9-9: Unused import.
hashlibis imported but not used in the test file.♻️ Remove unused import
-import hashlib🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_visit_blockchain.py` at line 9, The file imports hashlib but never uses it; remove the unused import statement (the "import hashlib" line) from tests/test_visit_blockchain.py to clean up unused dependencies and satisfy the linter.backend/routers/field_officer.py (1)
563-565: Exception chaining missing — useraise ... from efor better traceability.The static analysis hint B904 is valid here. When re-raising as
HTTPException, the original exception context is lost, making debugging harder.♻️ Proposed fix
except Exception as e: logger.error(f"Error verifying visit blockchain for {visit_id}: {e}", exc_info=True) - raise HTTPException(status_code=500, detail="Failed to verify visit integrity") + raise HTTPException(status_code=500, detail="Failed to verify visit integrity") from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routers/field_officer.py` around lines 563 - 565, The except block that logs errors in the visit verification flow currently loses the original exception context when re-raising an HTTPException; update the handler in the function/method where visit_id is used (the except Exception as e block that calls logger.error(f"Error verifying visit blockchain for {visit_id}: {e}", exc_info=True)) to re-raise the HTTPException using exception chaining (raise HTTPException(status_code=500, detail="Failed to verify visit integrity") from e) so the original traceback is preserved for diagnostics.
🤖 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/geofencing_service.py`:
- Around line 114-115: The current timezone-stripping logic in
geofencing_service.py only handles '+' offsets for check_in_time_str, so
negative offsets like '-05:00' are left intact and cause hash mismatches; update
the code that processes check_in_time_str to remove both positive and negative
timezone offsets (e.g., detect either '+' or '-' offset and strip the trailing
offset) or, even better, normalize the timestamp using a robust parser (e.g.,
dateutil.parser) to a consistent timezone/naive UTC before hashing; locate the
check_in_time_str handling and replace the single '+' split with logic or a
regex that removes / normalizes any [+-]HH:MM or 'Z' suffix.
In `@backend/routers/field_officer.py`:
- Around line 511-565: The verify_visit_blockchain_integrity endpoint currently
catches all exceptions and will turn deliberate HTTPException(404) into a 500;
update the error handling in verify_visit_blockchain_integrity by adding an
explicit except HTTPException: raise (or re-raise the caught HTTPException)
before the generic except Exception block so that HTTP errors (like the "Visit
not found" 404) propagate unchanged, leaving the final except Exception to
handle unexpected errors and log/raise a 500.
---
Nitpick comments:
In `@backend/routers/field_officer.py`:
- Around line 563-565: The except block that logs errors in the visit
verification flow currently loses the original exception context when re-raising
an HTTPException; update the handler in the function/method where visit_id is
used (the except Exception as e block that calls logger.error(f"Error verifying
visit blockchain for {visit_id}: {e}", exc_info=True)) to re-raise the
HTTPException using exception chaining (raise HTTPException(status_code=500,
detail="Failed to verify visit integrity") from e) so the original traceback is
preserved for diagnostics.
In `@tests/test_visit_blockchain.py`:
- Line 13: The tests create a file-backed SQLite DB via
TEST_SQLALCHEMY_DATABASE_URL ("sqlite:///./test_blockchain.db") and never clean
it up; change the test fixture that creates the DB (e.g., the test_db fixture
which calls create_engine/TestingSessionLocal and Base.metadata.create_all) to
use an in-memory SQLite URL ("sqlite:///:memory:") with a single shared
connection OR add teardown logic to close the session and remove
"./test_blockchain.db" after tests complete (use os.path.exists + os.remove) to
ensure the file is deleted; ensure connect_args={"check_same_thread": False} and
session/engine reuse if switching to in-memory so tests see the same schema.
- Line 9: The file imports hashlib but never uses it; remove the unused import
statement (the "import hashlib" line) from tests/test_visit_blockchain.py to
clean up unused dependencies and satisfy the linter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b19a45e-b720-4893-9284-83ace5889f36
📒 Files selected for processing (7)
backend/cache.pybackend/geofencing_service.pybackend/init_db.pybackend/models.pybackend/routers/field_officer.pybackend/schemas.pytests/test_visit_blockchain.py
| if '+' in check_in_time_str: | ||
| check_in_time_str = check_in_time_str.split('+')[0] |
There was a problem hiding this comment.
Incomplete timezone offset handling — negative UTC offsets use -, not +.
The current logic only strips positive timezone offsets (e.g., +05:30). Negative offsets like -05:00 (UTC-5) won't be stripped, causing hash mismatches for datetimes with negative timezone offsets.
🛠️ Proposed fix to handle both positive and negative offsets
check_in_time_str = str(check_in_time) if check_in_time else ""
- if '+' in check_in_time_str:
- check_in_time_str = check_in_time_str.split('+')[0]
+ # Strip timezone offset (both + and - formats)
+ for sep in ('+', '-'):
+ if sep in check_in_time_str and 'T' in check_in_time_str:
+ # Only strip if it looks like a timezone suffix after time
+ parts = check_in_time_str.rsplit(sep, 1)
+ if len(parts) == 2 and ':' in parts[1]:
+ check_in_time_str = parts[0]
+ breakAlternatively, use a more robust approach with regex or dateutil parsing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/geofencing_service.py` around lines 114 - 115, The current
timezone-stripping logic in geofencing_service.py only handles '+' offsets for
check_in_time_str, so negative offsets like '-05:00' are left intact and cause
hash mismatches; update the code that processes check_in_time_str to remove both
positive and negative timezone offsets (e.g., detect either '+' or '-' offset
and strip the trailing offset) or, even better, normalize the timestamp using a
robust parser (e.g., dateutil.parser) to a consistent timezone/naive UTC before
hashing; locate the check_in_time_str handling and replace the single '+' split
with logic or a regex that removes / normalizes any [+-]HH:MM or 'Z' suffix.
| @router.get("/field-officer/{visit_id}/blockchain-verify", response_model=BlockchainVerificationResponse) | ||
| def verify_visit_blockchain_integrity(visit_id: int, db: Session = Depends(get_db)): | ||
| """ | ||
| Verify the cryptographic integrity of a visit record using O(1) single-record verification. | ||
| """ | ||
| try: | ||
| visit = db.query( | ||
| FieldOfficerVisit.issue_id, | ||
| FieldOfficerVisit.officer_email, | ||
| FieldOfficerVisit.check_in_latitude, | ||
| FieldOfficerVisit.check_in_longitude, | ||
| FieldOfficerVisit.check_in_time, | ||
| FieldOfficerVisit.visit_notes, | ||
| FieldOfficerVisit.visit_hash, | ||
| FieldOfficerVisit.previous_visit_hash | ||
| ).filter(FieldOfficerVisit.id == visit_id).first() | ||
|
|
||
| if not visit: | ||
| raise HTTPException(status_code=404, detail="Visit not found") | ||
|
|
||
| # Determine previous hash (O(1) from stored column) | ||
| prev_hash = visit.previous_visit_hash or "" | ||
|
|
||
| # Reconstruct visit data for hash verification | ||
| # Normalization of time must match the generation logic | ||
| visit_data = { | ||
| 'issue_id': visit.issue_id, | ||
| 'officer_email': visit.officer_email, | ||
| 'check_in_latitude': visit.check_in_latitude, | ||
| 'check_in_longitude': visit.check_in_longitude, | ||
| 'check_in_time': visit.check_in_time, # Pass datetime object | ||
| 'visit_notes': visit.visit_notes or '', | ||
| 'previous_visit_hash': prev_hash | ||
| } | ||
|
|
||
| # Verify integrity using the service helper | ||
| is_valid = verify_visit_integrity(visit_data, visit.visit_hash) | ||
| computed_hash = generate_visit_hash(visit_data) | ||
|
|
||
| message = ( | ||
| "Integrity verified. This visit record is cryptographically sealed and part of an immutable chain." | ||
| if is_valid | ||
| else "Integrity check failed! The record data does not match its cryptographic seal." | ||
| ) | ||
|
|
||
| return BlockchainVerificationResponse( | ||
| is_valid=is_valid, | ||
| current_hash=visit.visit_hash, | ||
| computed_hash=computed_hash, | ||
| message=message | ||
| ) | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Error verifying visit blockchain for {visit_id}: {e}", exc_info=True) | ||
| raise HTTPException(status_code=500, detail="Failed to verify visit integrity") |
There was a problem hiding this comment.
Missing HTTPException re-raise — 404 errors will incorrectly return 500.
The endpoint raises HTTPException(status_code=404) at line 529, but unlike other endpoints in this file (e.g., lines 183-184, 260-261), there's no except HTTPException: raise clause. The generic except Exception at line 563 will catch the 404 and convert it to a 500 error.
🐛 Proposed fix
return BlockchainVerificationResponse(
is_valid=is_valid,
current_hash=visit.visit_hash,
computed_hash=computed_hash,
message=message
)
+ except HTTPException:
+ raise
except Exception as e:
logger.error(f"Error verifying visit blockchain for {visit_id}: {e}", exc_info=True)
- raise HTTPException(status_code=500, detail="Failed to verify visit integrity")
+ raise HTTPException(status_code=500, detail="Failed to verify visit integrity") from e🧰 Tools
🪛 Ruff (0.15.7)
[warning] 512-512: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
[warning] 565-565: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/routers/field_officer.py` around lines 511 - 565, The
verify_visit_blockchain_integrity endpoint currently catches all exceptions and
will turn deliberate HTTPException(404) into a 500; update the error handling in
verify_visit_blockchain_integrity by adding an explicit except HTTPException:
raise (or re-raise the caught HTTPException) before the generic except Exception
block so that HTTP errors (like the "Visit not found" 404) propagate unchanged,
leaving the final except Exception to handle unexpected errors and log/raise a
500.
There was a problem hiding this comment.
Pull request overview
This PR introduces blockchain-style, cryptographically chained integrity protection for FieldOfficerVisit records, aiming to make visit check-in hashing and per-record integrity verification performant (O(1)) and tamper-evident.
Changes:
- Added
previous_visit_hashtoFieldOfficerVisit(model + migration) and included it in visit hashing for chaining. - Implemented an in-memory
visit_last_hash_cacheto avoid DB lookups for the chain head during check-in. - Added
/api/field-officer/{visit_id}/blockchain-verifyplus tests covering chaining and tamper detection.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
backend/models.py |
Adds previous_visit_hash column (indexed) to persist chain linkage. |
backend/init_db.py |
Migrates existing DBs by adding the column and creating an index. |
backend/geofencing_service.py |
Extends generate_visit_hash to include previous_visit_hash and normalizes datetime input. |
backend/cache.py |
Introduces visit_last_hash_cache for caching the chain head hash. |
backend/routers/field_officer.py |
Updates check-in/out responses to include hashes; adds O(1) blockchain verification endpoint; uses cache during check-in. |
backend/schemas.py |
Extends FieldOfficerVisitResponse to expose visit_hash and previous_visit_hash. |
tests/test_visit_blockchain.py |
Adds tests for chaining, tamper detection, and cache-miss DB fallback behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Blockchain feature: retrieve previous hash for chaining | ||
| # Use cache for O(1) retrieval | ||
| prev_hash = visit_last_hash_cache.get("last_hash") | ||
| if prev_hash is None: | ||
| # Cache miss: Fetch from DB |
There was a problem hiding this comment.
Using an in-memory cache for the chain head without any DB-level locking allows concurrent check-ins to read the same prev_hash and create forks (or link to a stale head), breaking the intended single linear chain. Consider serializing head updates (e.g., via a dedicated chain-head row with SELECT ... FOR UPDATE / transactional compare-and-swap) and/or using a shared cache with atomic operations if you want to keep the O(1) optimization.
| # Update cache after successful commit | ||
| visit_last_hash_cache.set(data=visit_hash, key="last_hash") |
There was a problem hiding this comment.
The cache update after commit doesn’t prevent races/staleness across concurrent requests (or other app instances), so subsequent check-ins can still chain to an outdated head. If integrity depends on a strict chain order, the head update needs to be part of an atomic, serialized operation (DB transaction/lock or shared atomic store).
| @@ -119,6 +122,7 @@ def generate_visit_hash(visit_data: dict) -> str: | |||
| f"{visit_data.get('check_in_longitude')}" | |||
There was a problem hiding this comment.
The hash input is built by concatenating fields with no separators, which can create ambiguous strings (different field tuples producing the same concatenation) and undermine integrity guarantees. Use an unambiguous serialization (e.g., delimiter-separated with escaping, or canonical JSON with explicit field names) before HMACing.
| import hashlib | ||
| from datetime import datetime, timezone |
There was a problem hiding this comment.
Unused imports (hashlib, datetime, timezone) add noise and can trigger lint failures if enforced. Remove them or use them in assertions as intended.
| import hashlib | |
| from datetime import datetime, timezone |
| app.dependency_overrides[get_db] = override_get_db | ||
| with TestClient(app) as c: | ||
| yield c |
There was a problem hiding this comment.
Because visit_last_hash_cache is a module-level singleton, its state can leak between tests and make results order-dependent. Clear the cache in fixture setup/teardown (e.g., in client before creating TestClient) so each test starts from a known state.
| # Verify integrity using the service helper | ||
| is_valid = verify_visit_integrity(visit_data, visit.visit_hash) | ||
| computed_hash = generate_visit_hash(visit_data) |
There was a problem hiding this comment.
verify_visit_integrity() already recomputes the hash internally; calling generate_visit_hash() again immediately after duplicates work. Compute the hash once here and derive is_valid from that to reduce overhead and keep the logic in one place.
| # Verify integrity using the service helper | |
| is_valid = verify_visit_integrity(visit_data, visit.visit_hash) | |
| computed_hash = generate_visit_hash(visit_data) | |
| # Compute hash once and derive integrity from comparison with stored hash | |
| computed_hash = generate_visit_hash(visit_data) | |
| is_valid = computed_hash == visit.visit_hash |
| check_in_time_str = check_in_time.strftime('%Y-%m-%dT%H:%M:%S') | ||
| else: | ||
| check_in_time_str = str(check_in_time) if check_in_time else "" | ||
| if '+' in check_in_time_str: | ||
| check_in_time_str = check_in_time_str.split('+')[0] |
There was a problem hiding this comment.
check_in_time_str normalization only strips offsets when a '+' is present. ISO-8601 strings may contain a 'Z' suffix or a negative offset (e.g., '-05:00'), which would produce a different hash for the same instant depending on representation. Normalize by parsing to a datetime (or strip both '+' and '-' offsets / 'Z') so equivalent timestamps hash identically.
| check_in_time_str = check_in_time.strftime('%Y-%m-%dT%H:%M:%S') | |
| else: | |
| check_in_time_str = str(check_in_time) if check_in_time else "" | |
| if '+' in check_in_time_str: | |
| check_in_time_str = check_in_time_str.split('+')[0] | |
| # If timezone-aware, convert to UTC, then drop tzinfo for a stable naive representation | |
| if check_in_time.tzinfo is not None: | |
| check_in_time_utc = check_in_time.astimezone(timezone.utc).replace(tzinfo=None) | |
| else: | |
| check_in_time_utc = check_in_time | |
| check_in_time_str = check_in_time_utc.strftime('%Y-%m-%dT%H:%M:%S') | |
| else: | |
| raw_time_str = str(check_in_time) if check_in_time else "" | |
| if raw_time_str: | |
| # Try to parse as ISO-8601, handling 'Z' and +/- offsets so equivalent instants hash identically | |
| iso_str = raw_time_str.rstrip() | |
| if iso_str.endswith('Z'): | |
| iso_str = iso_str[:-1] + '+00:00' | |
| try: | |
| parsed_dt = datetime.fromisoformat(iso_str) | |
| if parsed_dt.tzinfo is not None: | |
| parsed_dt = parsed_dt.astimezone(timezone.utc).replace(tzinfo=None) | |
| check_in_time_str = parsed_dt.strftime('%Y-%m-%dT%H:%M:%S') | |
| except ValueError: | |
| # Fallback to previous behavior for non-ISO strings | |
| check_in_time_str = raw_time_str | |
| if '+' in check_in_time_str: | |
| check_in_time_str = check_in_time_str.split('+')[0] | |
| elif check_in_time_str.endswith('Z'): | |
| check_in_time_str = check_in_time_str[:-1] | |
| else: | |
| check_in_time_str = "" |
| from datetime import datetime, timezone | ||
|
|
||
| # Setup test DB | ||
| TEST_SQLALCHEMY_DATABASE_URL = "sqlite:///./test_blockchain.db" |
There was a problem hiding this comment.
Using a fixed on-disk SQLite path (sqlite:///./test_blockchain.db) can pollute the repo working directory and make test runs interfere with each other. Prefer a per-test temporary path via tmp_path (or an in-memory DB with StaticPool) and clean up the file after the test.
| if not visit: | ||
| raise HTTPException(status_code=404, detail="Visit not found") | ||
|
|
There was a problem hiding this comment.
HTTPException raised for missing visits (e.g., 404) will be caught by the broad except Exception and converted into a 500 response. Add an explicit except HTTPException: raise (like other endpoints in this router) before the generic exception handler so client errors propagate correctly.
This change implements a cryptographically chained blockchain for Field Officer Visit records, enabling O(1) single-record integrity verification.
Key improvements:
- Added `previous_visit_hash` to `FieldOfficerVisit` model for chaining.
- Implemented O(1) chain-head retrieval using `visit_last_hash_cache`.
- Updated `officer_check_in` to automatically link new visits to the previous record's hash.
- Added `/api/field-officer/{visit_id}/blockchain-verify` for fast, efficient integrity checks.
- Normalized timestamps and stripped microseconds to ensure deterministic hashing across different database backends.
- Updated `backend/requirements-render.txt` to include `python-magic` for file validation.
- Cleaned up temporary test artifacts and scripts.
This change implements a cryptographically chained blockchain for Field Officer Visit records, enabling O(1) single-record integrity verification.
Key improvements:
- Added `previous_visit_hash` to `FieldOfficerVisit` model for chaining.
- Implemented O(1) chain-head retrieval using `visit_last_hash_cache`.
- Updated `officer_check_in` to automatically link new visits to the previous record's hash.
- Added `/api/field-officer/{visit_id}/blockchain-verify` for fast, efficient integrity checks.
- Normalized timestamps and stripped microseconds to ensure deterministic hashing.
- Updated `backend/requirements-render.txt` to include `python-magic` for file validation.
- Enabled database migrations in `backend/main.py` to ensure schema updates are applied.
- Cleaned up temporary test artifacts.
🔍 Quality Reminder |
- Implemented O(1) blockchain verification for officer visits. - Added `previous_visit_hash` for cryptographic chaining. - Fixed Render deployment by adding `python-magic` to requirements. - Re-enabled automatic database migrations on startup. - Normalized timestamp hashing for cross-system consistency.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test_imports.py">
<violation number="1" location="test_imports.py:8">
P2: Move this import smoke check into a test function and raise an assertion instead of exiting the process.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| # Add current directory to path | ||
| sys.path.append(os.getcwd()) | ||
|
|
||
| try: |
There was a problem hiding this comment.
P2: Move this import smoke check into a test function and raise an assertion instead of exiting the process.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test_imports.py, line 8:
<comment>Move this import smoke check into a test function and raise an assertion instead of exiting the process.</comment>
<file context>
@@ -0,0 +1,27 @@
+# Add current directory to path
+sys.path.append(os.getcwd())
+
+try:
+ print("Testing imports...")
+ from backend.main import app
</file context>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test_imports.py (1)
5-6: Consider usingsys.path.insert(0, ...)for consistency withmain.py.
sys.path.append()adds to the end of the search path, whereasbackend/main.pyusessys.path.insert(0, str(repo_root))to prepend. This inconsistency could cause different import resolution behavior if abackendpackage exists elsewhere in the path.♻️ Suggested fix
# Add current directory to path -sys.path.append(os.getcwd()) +sys.path.insert(0, os.getcwd())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_imports.py` around lines 5 - 6, Replace the sys.path.append(os.getcwd()) call in test_imports.py with a prepend using sys.path.insert(0, os.getcwd()) so imports resolve consistently with backend/main.py (which uses sys.path.insert(0, str(repo_root))). Locate the sys.path.append usage and change it to use sys.path.insert at index 0, ensuring the current working directory is placed first in the import search order to avoid conflicts with similarly named packages elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test_imports.py`:
- Around line 5-6: Replace the sys.path.append(os.getcwd()) call in
test_imports.py with a prepend using sys.path.insert(0, os.getcwd()) so imports
resolve consistently with backend/main.py (which uses sys.path.insert(0,
str(repo_root))). Locate the sys.path.append usage and change it to use
sys.path.insert at index 0, ensuring the current working directory is placed
first in the import search order to avoid conflicts with similarly named
packages elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d9545f1-b477-4fd2-a0cb-fc476d1b5284
⛔ Files ignored due to path filters (1)
test_blockchain.dbis excluded by!**/*.db
📒 Files selected for processing (2)
backend/main.pytest_imports.py
⚡ Bolt: implement O(1) blockchain verification for officer visits
Implemented a cryptographically chained blockchain for
FieldOfficerVisitrecords to ensure data integrity and prevent tampering.💡 What:
previous_visit_hashcolumn and index to thefield_officer_visitstable.generate_visit_hashto include the previous record's hash in the HMAC-SHA256 calculation.visit_last_hash_cache(ThreadSafeCache) to eliminate the need for a database query when retrieving the previous hash during check-in (O(1) performance)./api/field-officer/{visit_id}/blockchain-verifythat performs O(1) integrity validation using only the record's data and its storedprevious_visit_hash.🎯 Why:
Previously, there was no chaining between visit records, and verifying integrity would have required scanning the entire history. This implementation provides an immutable audit trail for government officer visits with high-performance verification.
📊 Impact:
🔬 Measurement:
Verified with
tests/test_visit_blockchain.pywhich covers chaining logic, cache performance, and tamper detection.PR created automatically by Jules for task 7836674297544090238 started by @RohanExploit
Summary by cubic
Adds a chained HMAC-SHA256 blockchain to
FieldOfficerVisitwith an O(1) verification endpoint to make visits tamper-evident and fast to verify. Startup now runs migrations to add and indexprevious_visit_hash, and timestamps are normalized for deterministic hashing; also fixes Render deploy by addingpython-magic.New Features
previous_visit_hashcolumn and index; visit hash now includes the previous hash.visit_last_hash_cachewith DB fallback; check-ins update the cache./api/field-officer/{visit_id}/blockchain-verifyreturnsis_valid,current_hash,computed_hash, and a message.visit_hashandprevious_visit_hash.Dependencies
python-magictobackend/requirements-render.txt.Written for commit 53a2852. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores