Conversation
- Updated `backend/models.py` to add `VerificationStatus` and `EvidenceAuditLog` - Added missing columns to `ResolutionEvidence` and `ResolutionProofToken` in `backend/models.py` based on usage in `ResolutionProofService`. - Fixed dummy image tests (`test_captioning.py`, `test_smart_scan.py`, `test_graffiti_endpoint.py`) to generate valid PIL JPEG bytes. - Fixed `backend/pothole_detection.py` thread safety test failures by ensuring `reset_model()` doesn't automatically call `load_model()` unconditionally.
|
👋 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. |
📝 WalkthroughWalkthroughThe PR extends the data models by introducing a VerificationStatus enum and EvidenceAuditLog model for tracking evidence actions, adds cryptographic and location fields to ResolutionEvidence, refactors ResolutionProofToken with geofence and lifecycle management attributes, simplifies the model reset logic, and updates test infrastructure to mock cached detection functions with real PIL-generated images. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 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
This PR updates backend unit tests and ORM models to align with the current detection router caching wrappers and the ResolutionProofService data model, and removes problematic model reload behavior during pothole model resets.
Changes:
- Update detection endpoint tests to upload valid JPEG bytes (via Pillow) and patch router-level cached functions.
- Remove unintended model reload logic from
reset_model()inpothole_detection.py. - Extend
backend/models.pywith missing enums/models/columns needed byResolutionProofService.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_smart_scan.py | Uses a real generated JPEG payload and patches _cached_detect_smart_scan used by the endpoint. |
| tests/test_graffiti_endpoint.py | Uses a real generated JPEG payload and patches _cached_detect_graffiti used by the endpoint. |
| tests/test_captioning.py | Uses a real generated JPEG payload and patches _cached_generate_caption used by the endpoint. |
| backend/pothole_detection.py | Removes post-reset implicit reload logic to keep reset behavior consistent and thread-safe. |
| backend/models.py | Adds VerificationStatus, expands ResolutionEvidence/ResolutionProofToken, and adds EvidenceAuditLog to match ResolutionProofService. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| device_fingerprint_hash = Column(String, nullable=True) | ||
| metadata_bundle = Column(JSON, nullable=True) | ||
| server_signature = Column(String, nullable=True) | ||
| verification_status = Column(Enum(VerificationStatus), default=VerificationStatus.PENDING) |
There was a problem hiding this comment.
verification_status currently allows NULLs. Since the service logic treats verification status as a required state machine, consider setting nullable=False (and optionally indexing it if you commonly filter by status). This avoids records with an undefined verification state.
| verification_status = Column(Enum(VerificationStatus), default=VerificationStatus.PENDING) | |
| verification_status = Column(Enum(VerificationStatus), default=VerificationStatus.PENDING, nullable=False) |
| class ResolutionEvidence(Base): | ||
| __tablename__ = "resolution_evidence" | ||
| id = Column(Integer, primary_key=True, index=True) | ||
| grievance_id = Column(Integer, ForeignKey("grievances.id"), nullable=False) | ||
| file_path = Column(String, nullable=False) | ||
| token_id = Column(Integer, nullable=True) | ||
| evidence_hash = Column(String, nullable=False) | ||
| gps_latitude = Column(Float, nullable=True) | ||
| gps_longitude = Column(Float, nullable=True) | ||
| capture_timestamp = Column(DateTime, nullable=True) | ||
| device_fingerprint_hash = Column(String, nullable=True) | ||
| metadata_bundle = Column(JSON, nullable=True) | ||
| server_signature = Column(String, nullable=True) | ||
| verification_status = Column(Enum(VerificationStatus), default=VerificationStatus.PENDING) | ||
|
|
||
| file_path = Column(String, nullable=True) # made true to match earlier schema that had it but didn't require it in tests | ||
| media_type = Column(String, default="image") | ||
| description = Column(Text, nullable=True) | ||
| uploaded_at = Column(DateTime, default=lambda: datetime.datetime.now(datetime.timezone.utc)) | ||
| created_at = Column(DateTime, default=lambda: datetime.datetime.now(datetime.timezone.utc)) | ||
|
|
||
| # Relationship | ||
| grievance = relationship("Grievance", back_populates="resolution_evidence") | ||
|
|
||
| class ResolutionProofToken(Base): | ||
| __tablename__ = "resolution_proof_tokens" | ||
| id = Column(Integer, primary_key=True, index=True) | ||
| grievance_id = Column(Integer, ForeignKey("grievances.id"), nullable=False) | ||
| token = Column(String, unique=True, index=True) | ||
| generated_at = Column(DateTime, default=lambda: datetime.datetime.now(datetime.timezone.utc)) | ||
| expires_at = Column(DateTime, nullable=False) | ||
| token_id = Column(String, unique=True, index=True) | ||
| authority_email = Column(String, nullable=False) | ||
| geofence_latitude = Column(Float, nullable=False) | ||
| geofence_longitude = Column(Float, nullable=False) | ||
| geofence_radius_meters = Column(Float, nullable=False) | ||
| valid_from = Column(DateTime, nullable=False) | ||
| valid_until = Column(DateTime, nullable=False) | ||
| nonce = Column(String, nullable=False) | ||
| token_signature = Column(String, nullable=False) | ||
| is_used = Column(Boolean, default=False) | ||
| used_at = Column(DateTime, nullable=True) | ||
| generated_at = Column(DateTime, default=lambda: datetime.datetime.now(datetime.timezone.utc)) | ||
|
|
||
| # Relationship | ||
| grievance = relationship("Grievance", back_populates="resolution_tokens") | ||
|
|
||
|
|
||
| class EvidenceAuditLog(Base): | ||
| __tablename__ = "evidence_audit_logs" | ||
| id = Column(Integer, primary_key=True, index=True) | ||
| evidence_id = Column(Integer, ForeignKey("resolution_evidence.id"), nullable=False) | ||
| action = Column(String, nullable=False) | ||
| details = Column(String, nullable=True) | ||
| actor_email = Column(String, nullable=True) | ||
| timestamp = Column(DateTime, default=lambda: datetime.datetime.now(datetime.timezone.utc)) |
There was a problem hiding this comment.
These schema additions/renames for resolution_evidence, resolution_proof_tokens, and the new evidence_audit_logs table will require a migration for existing databases. The repo uses backend/init_db.py:migrate_db() with manual ALTER TABLE checks, but it currently has no migration steps for these new columns/tables, so deployments against an existing DB are likely to fail at runtime (missing columns). Consider extending migrate_db() accordingly (and handling enum type creation where applicable).
| server_signature = Column(String, nullable=True) | ||
| verification_status = Column(Enum(VerificationStatus), default=VerificationStatus.PENDING) | ||
|
|
||
| file_path = Column(String, nullable=True) # made true to match earlier schema that had it but didn't require it in tests |
There was a problem hiding this comment.
Inline comment # made true to match earlier schema... is confusing since this change makes the column nullable, not "true". Reword to clearly state the intention (e.g., "made nullable" / "optional for backward compatibility").
| file_path = Column(String, nullable=True) # made true to match earlier schema that had it but didn't require it in tests | |
| file_path = Column(String, nullable=True) # kept nullable to match earlier schema where this field was optional in tests |
| token_id = Column(String, unique=True, index=True) | ||
| authority_email = Column(String, nullable=False) | ||
| geofence_latitude = Column(Float, nullable=False) | ||
| geofence_longitude = Column(Float, nullable=False) | ||
| geofence_radius_meters = Column(Float, nullable=False) | ||
| valid_from = Column(DateTime, nullable=False) | ||
| valid_until = Column(DateTime, nullable=False) | ||
| nonce = Column(String, nullable=False) | ||
| token_signature = Column(String, nullable=False) |
There was a problem hiding this comment.
ResolutionProofToken.token_id is implicitly nullable here. Since token_id is used as the primary lookup key and should never be NULL, make it nullable=False (and consider adding a DB-level constraint/server_default if needed). Note that unique=True does not prevent multiple NULLs on some DBs (and can cause confusing duplicates in SQLite).
| id = Column(Integer, primary_key=True, index=True) | ||
| grievance_id = Column(Integer, ForeignKey("grievances.id"), nullable=False) | ||
| file_path = Column(String, nullable=False) | ||
| token_id = Column(Integer, nullable=True) |
There was a problem hiding this comment.
ResolutionEvidence.token_id appears to store the DB PK of resolution_proof_tokens (see usages that query ResolutionProofToken.id == evidence.token_id). It should be declared as a ForeignKey("resolution_proof_tokens.id") (and ideally indexed) to enforce referential integrity and make joins/queries safer.
| token_id = Column(Integer, nullable=True) | |
| token_id = Column(Integer, ForeignKey("resolution_proof_tokens.id"), nullable=True, index=True) |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 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/models.py">
<violation number="1" location="backend/models.py:268">
P1: Missing `ForeignKey` constraint on `token_id`. This column is used as a reference to `resolution_proof_tokens.id` (confirmed by service code that assigns `token.id` to it and joins on it), but without the FK declaration the database won't enforce referential integrity — orphaned or invalid `token_id` values can be inserted.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| id = Column(Integer, primary_key=True, index=True) | ||
| grievance_id = Column(Integer, ForeignKey("grievances.id"), nullable=False) | ||
| file_path = Column(String, nullable=False) | ||
| token_id = Column(Integer, nullable=True) |
There was a problem hiding this comment.
P1: Missing ForeignKey constraint on token_id. This column is used as a reference to resolution_proof_tokens.id (confirmed by service code that assigns token.id to it and joins on it), but without the FK declaration the database won't enforce referential integrity — orphaned or invalid token_id values can be inserted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/models.py, line 268:
<comment>Missing `ForeignKey` constraint on `token_id`. This column is used as a reference to `resolution_proof_tokens.id` (confirmed by service code that assigns `token.id` to it and joins on it), but without the FK declaration the database won't enforce referential integrity — orphaned or invalid `token_id` values can be inserted.</comment>
<file context>
@@ -258,10 +265,21 @@ class ResolutionEvidence(Base):
id = Column(Integer, primary_key=True, index=True)
grievance_id = Column(Integer, ForeignKey("grievances.id"), nullable=False)
- file_path = Column(String, nullable=False)
+ token_id = Column(Integer, nullable=True)
+ evidence_hash = Column(String, nullable=False)
+ gps_latitude = Column(Float, nullable=True)
</file context>
| token_id = Column(Integer, nullable=True) | |
| token_id = Column(Integer, ForeignKey("resolution_proof_tokens.id"), nullable=True) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/models.py (1)
308-315: Add a composite index for audit history reads.If this table is queried by evidence timeline, indexing
(evidence_id, timestamp)will avoid expensive scans as records grow.Index addition
class EvidenceAuditLog(Base): __tablename__ = "evidence_audit_logs" + __table_args__ = ( + Index("ix_evidence_audit_logs_evidence_timestamp", "evidence_id", "timestamp"), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/models.py` around lines 308 - 315, Add a composite index on (evidence_id, timestamp) to the EvidenceAuditLog model to speed timeline queries; modify the EvidenceAuditLog class (the ORM class and its __table_args__) to include an Index on the columns evidence_id and timestamp (e.g., Index("ix_evidence_audit_logs_evidence_id_timestamp", EvidenceAuditLog.evidence_id, EvidenceAuditLog.timestamp)) so the database creates a multi-column index for audit-history reads.tests/test_captioning.py (1)
16-20: Consider extracting shared image-bytes test helper.The same PIL/BytesIO setup is duplicated across multiple detection tests; a small fixture/helper would reduce maintenance churn.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_captioning.py` around lines 16 - 20, Extract the repeated PIL/BytesIO setup (creating Image.new('RGB', (100,100)), saving to BytesIO, and returning the bytes stored in file_content) into a shared test helper or pytest fixture (e.g., make_image_bytes or sample_image_bytes) and replace usages of the inline variables img, img_bytes, and file_content in tests with calls to that helper/fixture; ensure the helper returns raw bytes so existing assertions that consume file_content remain unchanged.
🤖 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/models.py`:
- Around line 268-269: There are two different "token_id" fields (one Integer,
one String) and a nullable string token that create ambiguity; change the
evidence linkage token column to an explicit FK (e.g., token_id ->
Column(Integer, ForeignKey('tokens.id', name='fk_evidence_token_id'),
nullable=False) on the evidence model referencing the tokens table) and rename
the other string identifier to a distinct name (e.g., token_identifier or
token_hash) and make it nullable=False as well so both token identifiers are
non-null and unambiguous; ensure evidence_hash remains non-null and update any
references/usages to the renamed string token field in code and queries.
---
Nitpick comments:
In `@backend/models.py`:
- Around line 308-315: Add a composite index on (evidence_id, timestamp) to the
EvidenceAuditLog model to speed timeline queries; modify the EvidenceAuditLog
class (the ORM class and its __table_args__) to include an Index on the columns
evidence_id and timestamp (e.g.,
Index("ix_evidence_audit_logs_evidence_id_timestamp",
EvidenceAuditLog.evidence_id, EvidenceAuditLog.timestamp)) so the database
creates a multi-column index for audit-history reads.
In `@tests/test_captioning.py`:
- Around line 16-20: Extract the repeated PIL/BytesIO setup (creating
Image.new('RGB', (100,100)), saving to BytesIO, and returning the bytes stored
in file_content) into a shared test helper or pytest fixture (e.g.,
make_image_bytes or sample_image_bytes) and replace usages of the inline
variables img, img_bytes, and file_content in tests with calls to that
helper/fixture; ensure the helper returns raw bytes so existing assertions that
consume file_content remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f155b1f-3296-4a92-b7c4-cd25ebf27dba
📒 Files selected for processing (5)
backend/models.pybackend/pothole_detection.pytests/test_captioning.pytests/test_graffiti_endpoint.pytests/test_smart_scan.py
💤 Files with no reviewable changes (1)
- backend/pothole_detection.py
| token_id = Column(Integer, nullable=True) | ||
| evidence_hash = Column(String, nullable=False) |
There was a problem hiding this comment.
Disambiguate and constrain token linkage fields.
Line 268 and Line 291 currently define two different token_id concepts (Integer vs String), and the String identifier is nullable. This is prone to data-integrity drift and ambiguous query behavior. Use an explicit FK name for evidence linkage and make token identifiers non-null.
Suggested schema adjustment
class ResolutionEvidence(Base):
@@
- token_id = Column(Integer, nullable=True)
+ resolution_proof_token_id = Column(Integer, ForeignKey("resolution_proof_tokens.id"), nullable=True, index=True)
@@
- grievance = relationship("Grievance", back_populates="resolution_evidence")
+ grievance = relationship("Grievance", back_populates="resolution_evidence")
+ resolution_proof_token = relationship("ResolutionProofToken", back_populates="evidence_items")
class ResolutionProofToken(Base):
@@
- token_id = Column(String, unique=True, index=True)
+ token_id = Column(String, unique=True, index=True, nullable=False)
@@
- grievance = relationship("Grievance", back_populates="resolution_tokens")
+ grievance = relationship("Grievance", back_populates="resolution_tokens")
+ evidence_items = relationship("ResolutionEvidence", back_populates="resolution_proof_token")Also applies to: 291-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/models.py` around lines 268 - 269, There are two different "token_id"
fields (one Integer, one String) and a nullable string token that create
ambiguity; change the evidence linkage token column to an explicit FK (e.g.,
token_id -> Column(Integer, ForeignKey('tokens.id',
name='fk_evidence_token_id'), nullable=False) on the evidence model referencing
the tokens table) and rename the other string identifier to a distinct name
(e.g., token_identifier or token_hash) and make it nullable=False as well so
both token identifiers are non-null and unambiguous; ensure evidence_hash
remains non-null and update any references/usages to the renamed string token
field in code and queries.
This PR fixes several broken unit tests in the backend, primarily around thread safety in
pothole_detection.py, missing database models/schemas inmodels.pyresulting from incomplete PRs forResolutionProofService, and invalid image byte strings being used in detection endpoint tests. All tests have been verified to pass successfully.PR created automatically by Jules for task 12811041833142274400 started by @RohanExploit
Summary by cubic
Fixes failing backend tests by making model reset thread-safe and using real JPEG inputs. Updates the models schema to support ResolutionProofService with new evidence/token fields and an EvidenceAuditLog.
Bug Fixes
Migration
Written for commit 1d4833d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests