⚡ Bolt: Implement blockchain integrity for ClosureConfirmation#655
⚡ Bolt: Implement blockchain integrity for ClosureConfirmation#655RohanExploit wants to merge 3 commits into
Conversation
- Added integrity_hash and previous_integrity_hash to ClosureConfirmation model.
- Implemented HMAC-SHA256 chaining in ClosureService.submit_confirmation.
- Added O(1) verification endpoint /api/closure-confirmation/{id}/blockchain-verify.
- Optimized chaining with closure_last_hash_cache.
- Added comprehensive integration test suite.
|
👋 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. |
📝 WalkthroughWalkthroughIntroduces blockchain integrity hash chaining for closure confirmations using HMAC-SHA256. Changes include adding cache management, extending the data model with integrity columns, implementing database migrations, computing and verifying chained hashes in the closure service, exposing a verification endpoint, and adding comprehensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API Server
participant Database
participant Cache
rect rgba(76, 175, 80, 0.5)
Note over Client,Cache: Closure Confirmation Submission
Client->>API Server: POST /submit-confirmation
API Server->>Cache: Fetch last_hash (prev_hash)
alt Cache Miss
Cache-->>API Server: None
API Server->>Database: Query latest ClosureConfirmation.integrity_hash
Database-->>API Server: Return integrity_hash or null
else Cache Hit
Cache-->>API Server: Return cached hash
end
API Server->>API Server: Compute HMAC-SHA256(data\|prev_hash)
API Server->>Database: INSERT ClosureConfirmation(integrity_hash, previous_integrity_hash)
Database-->>API Server: Commit success
API Server->>Cache: SET closure_last_hash_cache = new_integrity_hash
Cache-->>API Server: Cache updated
API Server-->>Client: 200 OK
end
rect rgba(33, 150, 243, 0.5)
Note over Client,Cache: Blockchain Verification
Client->>API Server: GET /closure-confirmation/{id}/blockchain-verify
API Server->>Database: Fetch ClosureConfirmation(id)
Database-->>API Server: Return integrity_hash, previous_integrity_hash, fields
API Server->>API Server: Compute HMAC-SHA256(fields\|previous_integrity_hash)
alt Hashes Match
API Server-->>Client: 200 OK, is_valid=true
else Hashes Mismatch
API Server-->>Client: 200 OK, is_valid=false
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 |
There was a problem hiding this comment.
Pull request overview
Implements blockchain-style (hash-chained) integrity sealing for ClosureConfirmation records and adds an API endpoint + tests to verify a record’s seal.
Changes:
- Add
integrity_hashandprevious_integrity_hashfields toClosureConfirmation, plus DB migration/indexing support. - Compute/store an HMAC-SHA256 integrity hash during closure confirmation submission, using an in-memory cache for the latest hash.
- Add
/api/closure-confirmation/{id}/blockchain-verifyendpoint and a new test covering chaining + tamper detection.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/closure_service.py | Computes and stores chained HMAC integrity hashes for new closure confirmations. |
| backend/routers/grievances.py | Adds endpoint to recompute/verify a closure confirmation’s chained HMAC seal. |
| backend/models.py | Adds integrity hash columns to ClosureConfirmation model. |
| backend/init_db.py | Adds migration steps + index for new closure confirmation integrity columns. |
| backend/cache.py | Adds a dedicated closure_last_hash_cache instance for O(1) prev-hash lookup. |
| backend/tests/test_closure_blockchain.py | Adds end-to-end test for chaining + verification endpoint behavior. |
| .jules/bolt.md | Documents learning about updating blockchain caches after DB commit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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/routers/grievances.py">
<violation number="1" location="backend/routers/grievances.py:604">
P2: Use constant-time digest comparison for HMAC verification instead of `==`.</violation>
</file>
<file name="backend/closure_service.py">
<violation number="1" location="backend/closure_service.py:95">
P1: Race condition: concurrent requests can read the same `prev_hash` before either updates the cache, producing two records with identical `previous_integrity_hash` and breaking the chain. The cache's internal lock only protects individual get/set calls — it does not make the full read→hash→commit→update sequence atomic. Consider wrapping this critical section with a dedicated lock (or using `SELECT ... FOR UPDATE` at the DB level) to serialize chain extension.</violation>
<violation number="2" location="backend/closure_service.py:103">
P2: `reason` is excluded from the integrity hash, so it can be modified in the DB without detection. Since the stated goal is tamper-proofing confirmation records, include `reason` (or a normalized form of it) in `hash_content`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/routers/grievances.py (1)
618-622: Useraise ... fromfor proper exception chaining.The static analyzer flagged that exceptions within
exceptclauses should useraise ... from errorraise ... from Noneto distinguish them from errors in exception handling.♻️ Proposed fix
except HTTPException: raise except Exception as e: logger.error(f"Error verifying closure confirmation blockchain for {confirmation_id}: {e}", exc_info=True) - raise HTTPException(status_code=500, detail="Failed to verify confirmation integrity") + raise HTTPException(status_code=500, detail="Failed to verify confirmation integrity") from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routers/grievances.py` around lines 618 - 622, The catch-all except block should chain the original exception when converting to an HTTPException: inside the except Exception as e handler (the block that logs "Error verifying closure confirmation blockchain for {confirmation_id}"), keep the logger.error call but change the raise to raise HTTPException(status_code=500, detail="Failed to verify confirmation integrity") from e so the original exception is preserved; update the except block that currently handles generic exceptions in the verification routine (the one referencing confirmation_id and logger) to use "raise ... from e".backend/closure_service.py (1)
93-100: Minor: Redundant cache set on miss may cause subtle race issues.Line 100 sets the cache immediately after a DB lookup (before commit), but line 125 will overwrite it after commit anyway. This is mostly harmless, but in concurrent scenarios where multiple confirmations are submitted simultaneously, both requests could read the same
prev_hashbefore either commits, resulting in two records with identicalprevious_integrity_hashvalues—breaking the strictly linear chain.This is likely an architectural limitation affecting all blockchain integrity features in this codebase. Consider whether concurrent confirmation submissions are expected; if so, serialization (e.g., database-level locking or a mutex) would be needed to guarantee chain linearity.
💡 Optional: Remove redundant cache set before commit
prev_hash = closure_last_hash_cache.get("last_hash") if prev_hash is None: # Cache miss: Fetch only the last hash from DB last_conf = db.query(ClosureConfirmation.integrity_hash).order_by(ClosureConfirmation.id.desc()).first() prev_hash = last_conf[0] if last_conf and last_conf[0] else "" - closure_last_hash_cache.set(data=prev_hash, key="last_hash")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/closure_service.py` around lines 93 - 100, The early cache set of prev_hash via closure_last_hash_cache.set on a cache miss can produce race conditions that allow two concurrent submissions to use the same previous_integrity_hash; remove the redundant cache.set on miss and instead ensure linearity by performing the prev_hash read and subsequent insert inside a serialized operation (e.g., a DB transaction with a row/table lock or SELECT ... FOR UPDATE) or by protecting the sequence with a process-level mutex around the read+insert; locate symbols closure_last_hash_cache, prev_hash and ClosureConfirmation to adjust logic and ensure the final cache update remains the single authoritative write after commit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/closure_service.py`:
- Around line 93-100: The early cache set of prev_hash via
closure_last_hash_cache.set on a cache miss can produce race conditions that
allow two concurrent submissions to use the same previous_integrity_hash; remove
the redundant cache.set on miss and instead ensure linearity by performing the
prev_hash read and subsequent insert inside a serialized operation (e.g., a DB
transaction with a row/table lock or SELECT ... FOR UPDATE) or by protecting the
sequence with a process-level mutex around the read+insert; locate symbols
closure_last_hash_cache, prev_hash and ClosureConfirmation to adjust logic and
ensure the final cache update remains the single authoritative write after
commit.
In `@backend/routers/grievances.py`:
- Around line 618-622: The catch-all except block should chain the original
exception when converting to an HTTPException: inside the except Exception as e
handler (the block that logs "Error verifying closure confirmation blockchain
for {confirmation_id}"), keep the logger.error call but change the raise to
raise HTTPException(status_code=500, detail="Failed to verify confirmation
integrity") from e so the original exception is preserved; update the except
block that currently handles generic exceptions in the verification routine (the
one referencing confirmation_id and logger) to use "raise ... from e".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9295bb93-64ff-4320-8104-62eb28ff5c69
📒 Files selected for processing (7)
.jules/bolt.mdbackend/cache.pybackend/closure_service.pybackend/init_db.pybackend/models.pybackend/routers/grievances.pybackend/tests/test_closure_blockchain.py
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
🔍 Quality Reminder |
💡 What: Implementation of HMAC-SHA256 blockchain chaining for community closure confirmations.
🎯 Why: Ensures cryptographic integrity and immutability of the community-driven closure process, preventing unauthorized tampering with confirmation records.
📊 Impact:
ThreadSafeCachefor the latest hash.previous_integrity_hashdirectly in the record, avoiding expensive recursive lookups.🔬 Measurement: Verified via
backend/tests/test_closure_blockchain.pyensuring that valid chains pass verification and tampered records (or their successors) fail correctly.PR created automatically by Jules for task 1527181114373412597 started by @RohanExploit
Summary by cubic
Implements HMAC-SHA256 chaining for closure confirmations to make records tamper-evident with O(1) verification. Adds a verify endpoint and a thread-safe cache (updated only after commit) for safe, fast appends. Aligns with task 1527181114373412597.
New Features
integrity_hashandprevious_integrity_hashtoClosureConfirmation; indexedprevious_integrity_hash.grievance_id|user_email|confirmation_type|reason|prev_hashinClosureService.submit_confirmationusingclosure_last_hash_cachefor O(1) appends./api/closure-confirmation/{id}/blockchain-verifyfor single-record integrity checks.backend/tests/test_closure_blockchain.pyfor valid chains and tampering.Migration
init_dbadds the new columns and index automatically; no backfill required.integrity_hashwill not verify until new records are created.Written for commit 70b54c9. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests
Documentation