Feature: Add cleanup strategy for old database triggers #433 #460
Conversation
|
🪄 Scanning in progress... This comment will be updated with the results once the scan is complete. |
|
Caution Review failedFailed to post review comments Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an asynchronous cron trigger processor that atomically reserves and executes due triggers, marks statuses, creates next cron triggers with TTL, and runs configurable worker loops in UTC. Adds a periodic cleanup controller to delete old CANCELLED/TRIGGERED triggers. Includes unit tests for the cleanup query. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant S as Scheduler
participant CR as trigger_cron
participant W as Worker
participant DB as DatabaseTriggers
participant G as Graph Controller
S->>CR: trigger_cron() (UTC)
CR->>W: spawn workers (N)
loop worker loop
W->>DB: findOneAndUpdate(filter: PENDING & trigger_time <= cron_time, set TRIGGERING) -> trigger
alt trigger found
W->>G: call_trigger_graph(trigger, request_id)
alt success
W->>DB: update trigger_status = TRIGGERED
else failure
W->>DB: update trigger_status = FAILED
end
W->>DB: create_next_triggers(trigger, cron_time) (insert with expires_at TTL)
else none
W-->>W: exit
end
end
sequenceDiagram
autonumber
participant M as Maintenance
participant CL as start_periodic_cleanup
participant DB as DatabaseTriggers
M->>CL: start_periodic_cleanup()
loop every interval
CL->>DB: deleteMany({ trigger_status: {$in: [CANCELLED, TRIGGERED]}, trigger_time: {$lte: cutoffUTC} })
CL-->>CL: sleep(interval)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
✨ Finishing touches
🧪 Generate unit tests
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 |
Summary of ChangesHello @ak4shravikumar, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a crucial database maintenance feature by introducing an automated cleanup mechanism for old and inactive triggers. The primary goal is to prevent the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for cleaning up old database triggers, which will help manage database size and performance over the long term. The implementation of a periodic, configurable cleanup task is well-structured. My review includes a critical fix for an issue that would prevent new triggers from being created, along with suggestions to improve configuration management and test robustness.
| trigger_time=next_trigger_time, | ||
| trigger_status=TriggerStatusEnum.PENDING | ||
| trigger_status=TriggerStatusEnum.PENDING, | ||
| expires_at=datetime.utcnow() + timedelta(days=ttl_days) |
There was a problem hiding this comment.
The DatabaseTriggers model does not have an expires_at field. Adding it here will cause a pydantic.ValidationError at runtime when DatabaseTriggers is instantiated. The cleanup logic implemented in this PR uses the trigger_time field, so this expires_at field seems unnecessary and will break the creation of new triggers. If a TTL index is intended, the model and database index configuration need to be updated accordingly.
|
|
||
| # Check cutoff_time is UTC-aware | ||
| assert query["trigger_time"]["$lte"].tzinfo is not None | ||
| assert query["trigger_time"]["$lte"] <= datetime.now(timezone.utc) |
There was a problem hiding this comment.
This assertion is not effective, as a datetime from 30 days ago will always be less than the current time. This doesn't properly verify the cutoff_time calculation. A more robust test would check if the calculated cutoff time is close to the expected value, accounting for minor execution delays.
| assert query["trigger_time"]["$lte"] <= datetime.now(timezone.utc) | |
| assert abs(query["trigger_time"]["$lte"] - (datetime.now(timezone.utc) - timedelta(days=retention_days))) < timedelta(seconds=1) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
state-manager/app/tasks/trigger_cron.py(6 hunks)state_manager/app/controller/trigger_cleanup.py(1 hunks)state_manager/app/tasks/trigger_cron.py(1 hunks)state_manager/tests/unit/controller/test_trigger_cleanup.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
state_manager/tests/unit/controller/test_trigger_cleanup.py (2)
state_manager/app/controller/trigger_cleanup.py (1)
cleanup_old_triggers(12-24)state-manager/app/models/trigger_models.py (1)
TriggerStatusEnum(9-14)
state-manager/app/tasks/trigger_cron.py (5)
state-manager/app/models/db/trigger.py (1)
DatabaseTriggers(9-36)state-manager/app/controller/trigger_graph.py (1)
trigger_graph(31-115)state-manager/app/models/trigger_graph_model.py (1)
TriggerGraphRequestModel(4-7)state-manager/app/models/trigger_models.py (1)
TriggerStatusEnum(9-14)state-manager/app/config/settings.py (1)
get_settings(32-36)
state_manager/app/tasks/trigger_cron.py (6)
state-manager/app/models/db/trigger.py (1)
DatabaseTriggers(9-36)state-manager/app/models/trigger_models.py (2)
TriggerStatusEnum(9-14)TriggerTypeEnum(6-7)state-manager/app/singletons/logs_manager.py (2)
LogsManager(9-66)get_logger(65-66)state-manager/app/controller/trigger_graph.py (1)
trigger_graph(31-115)state-manager/app/models/trigger_graph_model.py (1)
TriggerGraphRequestModel(4-7)state-manager/app/config/settings.py (1)
get_settings(32-36)
state_manager/app/controller/trigger_cleanup.py (4)
state-manager/app/models/db/trigger.py (1)
DatabaseTriggers(9-36)state-manager/app/models/trigger_models.py (1)
TriggerStatusEnum(9-14)state-manager/app/singletons/logs_manager.py (2)
LogsManager(9-66)get_logger(65-66)state-manager/app/config/settings.py (1)
get_settings(32-36)
| while True: | ||
| try: | ||
| await cleanup_old_triggers() | ||
| except Exception as e: | ||
| logger.error(f"Error during trigger cleanup: {e}") | ||
| await asyncio.sleep(interval * 60) |
There was a problem hiding this comment.
Preserve cancellation of the periodic task.
The bare except Exception block will also catch asyncio.CancelledError, so when the app shuts down and cancels this background task the cancellation is swallowed, the loop keeps running, and shutdown hangs. Re-raise CancelledError (ideally log at INFO) before the generic handler, e.g.
- except Exception as e:
- logger.error(f"Error during trigger cleanup: {e}")
+ except asyncio.CancelledError:
+ logger.info("Trigger cleanup task cancelled; stopping periodic cleanup.")
+ raise
+ except Exception:
+ logger.exception("Error during trigger cleanup")🤖 Prompt for AI Agents
In state_manager/app/controller/trigger_cleanup.py around lines 33 to 38, the
generic "except Exception" handler swallows asyncio.CancelledError causing
shutdown to hang; modify the try/except so that if a CancelledError is raised
you log it at INFO and re-raise it immediately (or specifically catch
CancelledError and re-raise) before the generic exception handler that logs
other exceptions, preserving task cancellation while still logging
non-cancellation errors.
| {"_id": trigger.id}, | ||
| {"$set": {"trigger_status": TriggerStatusEnum.TRIGGERED}} | ||
| ) | ||
|
|
||
| async def handle_trigger(cron_time: datetime): | ||
| while(trigger:= await get_due_triggers(cron_time)): | ||
| try: | ||
| await call_trigger_graph(trigger) | ||
| await mark_as_triggered(trigger) |
There was a problem hiding this comment.
Add expires_at field + TTL index to the model.
create_next_triggers sets expires_at, but DatabaseTriggers doesn’t declare that field (and no TTL index exists), so Pydantic drops it on insert and Mongo never schedules TTL eviction. Pending triggers therefore never expire despite the new retention setting. Please add expires_at: datetime | None (or similar) to DatabaseTriggers and register a TTL index, e.g.
expires_at: datetime | None = Field(default=None)
IndexModel([("expires_at", 1)], expireAfterSeconds=0, name="ttl_expires_at")Without these additions, the core retention feature is broken.
🤖 Prompt for AI Agents
In state_manager/app/tasks/trigger_cron.py around lines 70 to 78, the
DatabaseTriggers model is missing an expires_at field and no TTL index is
registered, so create_next_triggers' expires_at is dropped and Mongo TTL
eviction never runs; add an expires_at: datetime | None = Field(default=None)
(or equivalent Optional[datetime]) to the DatabaseTriggers Pydantic model and
register a TTL index on that field (IndexModel([("expires_at", 1)],
expireAfterSeconds=0, name="ttl_expires_at")) when creating the collection or
indexes so MongoDB will automatically remove expired trigger documents.
| trigger_status=TriggerStatusEnum.PENDING, | ||
| expires_at=datetime.utcnow() + timedelta(days=ttl_days) | ||
| ).insert() |
There was a problem hiding this comment.
Prevent future cron triggers from expiring before they run.
expires_at is computed from datetime.utcnow(), so low-frequency crons (monthly/quarterly) will be deleted by TTL before their scheduled next_trigger_time, causing those triggers never to execute. Tie expires_at to the scheduled run time instead.
- expires_at=datetime.utcnow() + timedelta(days=ttl_days)
+ expires_at=next_trigger_time + timedelta(days=ttl_days)📝 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.
| trigger_status=TriggerStatusEnum.PENDING, | |
| expires_at=datetime.utcnow() + timedelta(days=ttl_days) | |
| ).insert() | |
| trigger_status=TriggerStatusEnum.PENDING, | |
| expires_at=next_trigger_time + timedelta(days=ttl_days) | |
| ).insert() |
🤖 Prompt for AI Agents
In state-manager/app/tasks/trigger_cron.py around lines 75 to 77, the code sets
expires_at using datetime.utcnow(), which lets low-frequency cron triggers
expire before their scheduled run; change expires_at to be based on the
trigger's scheduled next_trigger_time (e.g., set expires_at = next_trigger_time
+ timedelta(days=ttl_days) or at minimum max(next_trigger_time,
datetime.utcnow()) + timedelta(days=ttl_days)) so that TTL is applied relative
to when the job is supposed to run rather than when the record was created;
update the insert to use this computed value.
There was a problem hiding this comment.
Actionable comments posted: 126
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
state_manager/test_dotenv.py (1)
1-13: Remove or relocate debug script in state_manager/test_dotenv.pystate_manager/test_dotenv.py is a standalone debug script (no pytest assertions, not under tests/); remove it or move it to a proper scripts/ or tests/manual/ directory and document its purpose.
state_manager/app/config/settings.py (1)
36-49: Avoid import-time settings instantiation; add caching and explicit reloadInstantiating settings at import time can raise ValidationError if env is incomplete and makes tests brittle. Also, get_settings() reloads on every call. Cache once and expose an explicit reload.
Apply:
-# Global settings instance - will be updated when get_settings() is called -_settings = None +_settings: Settings | None = None @@ -def get_settings() -> Settings: - """Get the global settings instance, reloading from environment if needed.""" - global _settings - _settings = Settings.from_env() - return _settings +def get_settings() -> Settings: + """Get cached Settings (lazy init).""" + global _settings + if _settings is None: + _settings = Settings.from_env() + return _settings + +def reload_settings() -> Settings: + """Force reload Settings from environment.""" + global _settings + _settings = Settings.from_env() + return _settings @@ -# Initialize settings -settings = get_settings() +# Deliberately avoid module-level instantiation to prevent import-time failures
♻️ Duplicate comments (2)
state_manager/tests/unit/controller/test_trigger_cleanup.py (1)
41-42: Weak assertion: always passes.The assertion
cutoff_time <= datetime.now(timezone.utc)will always be true for any past datetime, making it ineffective at verifying thecutoff_timecalculation. A more robust test would verify that the cutoff is within a small tolerance of the expected value to account for execution delays.Apply this diff to strengthen the assertion:
- # Optional: Ensure cutoff is in the past - assert cutoff_time <= datetime.now(timezone.utc) + # Verify cutoff is approximately 30 days ago (default retention_days) + expected_cutoff = datetime.now(timezone.utc) - timedelta(days=30) + assert abs((cutoff_time - expected_cutoff).total_seconds()) < 1state_manager/app/controller/trigger_cleanup.py (1)
32-40: Don’t swallow task cancellation; clamp/parse interval minutes.Re-raise asyncio.CancelledError to allow clean shutdown; also parse interval and enforce a sane minimum to avoid tight loops.
Apply:
- interval = getattr(get_settings(), "cleanup_interval_minutes", 60) # default 1 hour - logger.info(f"Starting periodic trigger cleanup every {interval} minutes.") + settings = get_settings() + try: + interval_minutes = max(1, int(getattr(settings, "cleanup_interval_minutes", 60))) + except (TypeError, ValueError): + interval_minutes = 60 + logger.info(f"Starting periodic trigger cleanup every {interval_minutes} minutes.") + interval_seconds = interval_minutes * 60 @@ - try: - await cleanup_old_triggers() - except Exception as e: - logger.error(f"Error during trigger cleanup: {e}") - await asyncio.sleep(interval * 60) + try: + await cleanup_old_triggers() + except asyncio.CancelledError: + logger.info("Trigger cleanup task cancelled; stopping periodic cleanup.") + raise + except Exception: + logger.exception("Error during trigger cleanup") + await asyncio.sleep(interval_seconds)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
state_manager/poetry.lockis excluded by!**/*.lockstate_manager/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (107)
state_manager/.dockerignore(1 hunks)state_manager/.gitignore(1 hunks)state_manager/.python-version(1 hunks)state_manager/Dockerfile(1 hunks)state_manager/app/config/__init__.py(1 hunks)state_manager/app/config/cors.py(1 hunks)state_manager/app/config/settings.py(1 hunks)state_manager/app/config/test_settings.py(1 hunks)state_manager/app/controller/enqueue_states.py(1 hunks)state_manager/app/controller/errored_state.py(1 hunks)state_manager/app/controller/executed_state.py(1 hunks)state_manager/app/controller/get_graph_structure.py(1 hunks)state_manager/app/controller/get_graph_template.py(1 hunks)state_manager/app/controller/get_node_run_details.py(1 hunks)state_manager/app/controller/get_runs.py(1 hunks)state_manager/app/controller/get_secrets.py(1 hunks)state_manager/app/controller/list_graph_templates.py(1 hunks)state_manager/app/controller/list_namespaces.py(1 hunks)state_manager/app/controller/list_registered_nodes.py(1 hunks)state_manager/app/controller/manual_retry_state.py(1 hunks)state_manager/app/controller/prune_signal.py(1 hunks)state_manager/app/controller/re_queue_after_signal.py(1 hunks)state_manager/app/controller/register_nodes.py(1 hunks)state_manager/app/controller/trigger_cleanup.py(1 hunks)state_manager/app/controller/trigger_graph.py(1 hunks)state_manager/app/controller/upsert_graph_template.py(1 hunks)state_manager/app/main.py(1 hunks)state_manager/app/middlewares/request_id_middleware.py(1 hunks)state_manager/app/middlewares/unhandled_exceptions_middleware.py(1 hunks)state_manager/app/models/db/base.py(1 hunks)state_manager/app/models/db/graph_template_model.py(1 hunks)state_manager/app/models/db/registered_node.py(1 hunks)state_manager/app/models/db/run.py(1 hunks)state_manager/app/models/db/state.py(1 hunks)state_manager/app/models/db/store.py(1 hunks)state_manager/app/models/db/trigger.py(1 hunks)state_manager/app/models/dependent_string.py(1 hunks)state_manager/app/models/enqueue_request.py(1 hunks)state_manager/app/models/enqueue_response.py(1 hunks)state_manager/app/models/errored_models.py(1 hunks)state_manager/app/models/executed_models.py(1 hunks)state_manager/app/models/graph_models.py(1 hunks)state_manager/app/models/graph_structure_models.py(1 hunks)state_manager/app/models/graph_template_validation_status.py(1 hunks)state_manager/app/models/list_models.py(1 hunks)state_manager/app/models/manual_retry.py(1 hunks)state_manager/app/models/node_run_details_models.py(1 hunks)state_manager/app/models/node_template_model.py(1 hunks)state_manager/app/models/register_nodes_request.py(1 hunks)state_manager/app/models/register_nodes_response.py(1 hunks)state_manager/app/models/retry_policy_model.py(1 hunks)state_manager/app/models/run_models.py(1 hunks)state_manager/app/models/secrets_response.py(1 hunks)state_manager/app/models/signal_models.py(1 hunks)state_manager/app/models/state_status_enum.py(1 hunks)state_manager/app/models/store_config_model.py(1 hunks)state_manager/app/models/trigger_graph_model.py(1 hunks)state_manager/app/models/trigger_models.py(1 hunks)state_manager/app/routes.py(1 hunks)state_manager/app/singletons/SingletonDecorator.py(1 hunks)state_manager/app/singletons/logs_manager.py(1 hunks)state_manager/app/tasks/create_next_states.py(1 hunks)state_manager/app/tasks/verify_graph.py(1 hunks)state_manager/app/utils/check_database_health.py(1 hunks)state_manager/app/utils/check_secret.py(1 hunks)state_manager/app/utils/encrypter.py(1 hunks)state_manager/docker-compose.yml(1 hunks)state_manager/pyproject.toml(1 hunks)state_manager/pytest.ini(1 hunks)state_manager/run.py(1 hunks)state_manager/test_dotenv.py(1 hunks)state_manager/tests/README.md(1 hunks)state_manager/tests/__init__.py(1 hunks)state_manager/tests/unit/__init__.py(1 hunks)state_manager/tests/unit/config/test_cors.py(1 hunks)state_manager/tests/unit/controller/__init__.py(1 hunks)state_manager/tests/unit/controller/pending_test_get_graph_structure.py(1 hunks)state_manager/tests/unit/controller/test_enqueue_states.py(1 hunks)state_manager/tests/unit/controller/test_enqueue_states_comprehensive.py(1 hunks)state_manager/tests/unit/controller/test_errored_state.py(1 hunks)state_manager/tests/unit/controller/test_executed_state.py(1 hunks)state_manager/tests/unit/controller/test_get_graph_structure.py(1 hunks)state_manager/tests/unit/controller/test_get_graph_template.py(1 hunks)state_manager/tests/unit/controller/test_get_node_run_details.py(1 hunks)state_manager/tests/unit/controller/test_get_runs.py(1 hunks)state_manager/tests/unit/controller/test_get_secrets.py(1 hunks)state_manager/tests/unit/controller/test_get_states_by_run_id.py(1 hunks)state_manager/tests/unit/controller/test_list_graph_templates.py(1 hunks)state_manager/tests/unit/controller/test_list_registered_nodes.py(1 hunks)state_manager/tests/unit/controller/test_manual_retry_state.py(1 hunks)state_manager/tests/unit/controller/test_prune_signal.py(1 hunks)state_manager/tests/unit/controller/test_re_queue_after_signal.py(1 hunks)state_manager/tests/unit/controller/test_register_nodes.py(1 hunks)state_manager/tests/unit/controller/test_trigger_cleanup.py(1 hunks)state_manager/tests/unit/controller/test_trigger_graph.py(1 hunks)state_manager/tests/unit/controller/test_upsert_graph_template.py(1 hunks)state_manager/tests/unit/middlewares/__init__.py(1 hunks)state_manager/tests/unit/middlewares/test_request_id_middleware.py(1 hunks)state_manager/tests/unit/middlewares/test_unhandled_exceptions_middleware.py(1 hunks)state_manager/tests/unit/models/test_base.py(1 hunks)state_manager/tests/unit/models/test_dependent_string.py(1 hunks)state_manager/tests/unit/models/test_graph_template_model.py(1 hunks)state_manager/tests/unit/models/test_manual_retry.py(1 hunks)state_manager/tests/unit/models/test_node_template_model.py(1 hunks)state_manager/tests/unit/models/test_retry_policy_model.py(1 hunks)state_manager/tests/unit/models/test_retry_policy_model_extended.py(1 hunks)state_manager/tests/unit/models/test_signal_models.py(1 hunks)
⛔ Files not processed due to max files limit (12)
- state_manager/tests/unit/models/test_store.py
- state_manager/tests/unit/models/test_store_config_model.py
- state_manager/tests/unit/singletons/init.py
- state_manager/tests/unit/singletons/test_logs_manager.py
- state_manager/tests/unit/singletons/test_singleton_decorator.py
- state_manager/tests/unit/tasks/test_create_next_states.py
- state_manager/tests/unit/tasks/test_verify_graph.py
- state_manager/tests/unit/test_logs_manager.py
- state_manager/tests/unit/test_logs_manager_simple.py
- state_manager/tests/unit/test_main.py
- state_manager/tests/unit/test_routes.py
- state_manager/tests/unit/utils/init.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-03T18:20:45.910Z
Learnt from: NiveditJain
PR: exospherehost/exospherehost#343
File: state-manager/app/tasks/verify_graph.py:74-77
Timestamp: 2025-09-03T18:20:45.910Z
Learning: Store field validation for ${store.field} references in GraphTemplate is already handled by Pydantic model validators in the GraphTemplate class, checking against store_config.required_keys and store_config.default_values during template creation.
Applied to files:
state_manager/app/models/store_config_model.py
🧬 Code graph analysis (3)
state_manager/tests/unit/controller/test_trigger_cleanup.py (1)
state_manager/app/controller/trigger_cleanup.py (1)
cleanup_old_triggers(13-25)
state_manager/app/tasks/verify_graph.py (1)
python-sdk/exospherehost/statemanager.py (1)
trigger(37-91)
state_manager/app/main.py (2)
python-sdk/exospherehost/statemanager.py (1)
trigger(37-91)state_manager/app/controller/trigger_cleanup.py (1)
start_periodic_cleanup(28-40)
🪛 Checkov (3.2.334)
state_manager/Dockerfile
[low] 1-14: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[low] 1-14: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🪛 markdownlint-cli2 (0.18.1)
state_manager/tests/README.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
27-27: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
34-34: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
41-41: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
49-49: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
57-57: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
66-66: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
76-76: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
84-84: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
98-98: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
154-154: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
157-157: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
164-164: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
171-171: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Open Source Software Supply Chain Security Scanner
🔇 Additional comments (74)
state_manager/tests/__init__.py (1)
1-2: LGTM — harmless tests package marker.No side effects; fine as-is.
state_manager/tests/unit/controller/__init__.py (1)
1-2: Confirm package intent for pytest discovery/importsAdding init.py turns tests/controller into a package; this can change pytest collection and relative import behavior. If intentional, fine; otherwise consider removing it.
state_manager/.gitignore (1)
13-15: Verify breadth oflib/andlib64/ignores.If
state_manager/libcontains source code, these rules will ignore it. Keep only if you never place app code underlib*/here.state_manager/.python-version (1)
1-1: Pin looks good; verify cross-environment consistency.3.12 aligns with the Docker base. Ensure CI, pyproject/uv/poetry, and linters (e.g., pyright/mypy) also target 3.12 to avoid mismatch.
state_manager/pytest.ini (1)
1-9: LGTM: pytest-asyncio >=0.21 requirement satisfied (pins include ≥0.24.0 and ≥1.1.0).state_manager/tests/unit/middlewares/__init__.py (1)
1-1: LGTM!Standard Python package initializer with a descriptive comment. No issues.
state_manager/tests/unit/__init__.py (1)
1-3: LGTM!Standard Python package initializer with a descriptive comment. The extra blank lines are harmless.
state_manager/app/config/__init__.py (1)
1-2: LGTM!Standard Python package initializer with a descriptive comment. No issues.
state_manager/docker-compose.yml (1)
4-12: Explicitly set build context pathsIn
state_manager/docker-compose.yml, change:build: - context: . + context: ./state_manager dockerfile: DockerfileIf you’re using
state-manager/docker-compose.yml, similarly update:build: - context: . + context: ./state-manager dockerfile: DockerfileThis ensures
docker-compose upworks when run from the repo root.Likely an incorrect or invalid review comment.
state_manager/tests/unit/controller/test_trigger_cleanup.py (1)
11-40: Test logic and mock setup are correct.The test properly:
- Mocks the pymongo collection and delete operation
- Validates that
delete_manyis called with the correct query filtering bytrigger_statusandtrigger_time- Ensures the cutoff datetime is timezone-aware
state_manager/app/models/secrets_response.py (1)
1-6: LGTM!The
SecretsResponseModelis a simple, well-defined Pydantic model with appropriate typing and field description.state_manager/tests/unit/models/test_dependent_string.py (1)
5-86: Test coverage is comprehensive and correct.The test methods thoroughly exercise the
DependentStringmodel:
- Error handling when dependent values are unset
- Idempotency of mapping construction
- Multi-dependent value updates
- Identifier-field extraction
- Store dependency parsing
state_manager/app/models/graph_template_validation_status.py (1)
1-9: LGTM!The
GraphTemplateValidationStatusenum is correctly defined with appropriate string values for validation states.Note: The file is missing a trailing newline, which some linters and style guides recommend, but this is a minor formatting preference.
state_manager/app/models/state_status_enum.py (1)
1-20: LGTM!The
StateStatusEnumis well-organized with clear lifecycle groupings (Pending, Errored, Success, Retry) and proper string enum implementation.Note: The file is missing a trailing newline, which some linters and style guides recommend, but this is a minor formatting preference.
state_manager/app/models/errored_models.py (1)
1-11: LGTM!Both
ErroredRequestModelandErroredResponseModelare well-defined with appropriate field types and descriptions.Note: The file is missing a trailing newline, which some linters and style guides recommend, but this is a minor formatting preference.
state_manager/app/middlewares/unhandled_exceptions_middleware.py (1)
10-23: Exception handling and logging are well-implemented.The middleware correctly:
- Catches all unhandled exceptions
- Logs detailed context for debugging (error, traceback, request metadata)
- Returns a generic error message to clients (preventing information leakage)
- Preserves the request ID for tracing
state_manager/app/models/manual_retry.py (1)
5-11: LGTM — simple, clear Pydantic modelsFields and descriptions look good. No issues found.
state_manager/tests/unit/models/test_graph_template_model.py (1)
211-227: Duplicate test for polling interval coercionBoth tests validate the same path. Keep one.
- @pytest.mark.asyncio - async def test_get_valid_coerces_small_polling_interval(self): - """Test get_valid method coerces very small polling interval to 0.1""" - from unittest.mock import MagicMock - - with patch.object(GraphTemplate, 'get') as mock_get, \ - patch('time.monotonic', side_effect=[0, 1, 2]), \ - patch('asyncio.sleep') as _: - - mock_template = MagicMock() - mock_template.is_valid.return_value = True - mock_get.return_value = mock_template - - result = await GraphTemplate.get_valid("test_ns", "test_graph", polling_interval=0.01) - - assert result == mock_templateLikely an incorrect or invalid review comment.
state_manager/app/middlewares/request_id_middleware.py (1)
10-54: LGTM!The middleware implementation is sound. It correctly handles request ID validation, generation, state storage, header propagation, and observability logging. The response time calculation and logging are accurate.
state_manager/app/models/register_nodes_request.py (1)
5-14: LGTM!The Pydantic models are well-defined and appropriately typed. The use of
Field(...)with descriptions ensures clear API documentation. No validation issues detected.state_manager/app/controller/get_graph_template.py (1)
9-48: LGTM!The function correctly queries and returns the graph template. The namespace filtering is appropriate, and the 404 error handling is correct. The secrets transformation (converting keys to
secret_name: Truemapping) aligns with the intended API response structure.state_manager/tests/unit/controller/test_get_secrets.py (1)
9-239: LGTM!The test suite is comprehensive and covers success, error, and edge-case scenarios. The use of fixtures, mocking, and assertions is correct. The tests validate the controller's behavior effectively.
state_manager/app/models/trigger_graph_model.py (1)
4-11: LGTM!The Pydantic models are well-defined and appropriately typed. The validation for
start_delay(ge=0) ensures non-negative values. No issues detected.state_manager/pyproject.toml (2)
15-15: cryptography version confirmed Version 45.0.5 exists on PyPI (latest stable is 46.0.2); the specified dependency bound is valid.
19-19: pytest-cov≥6.2.1 is valid
Version 6.2.1 exists on PyPI and is stable; latest release is 7.0.0.state_manager/app/models/graph_structure_models.py (1)
3-3: Import path for StateStatusEnum is correct. Thestate_manager/app/models/db/state.pymodule imports and exposesStateStatusEnum, sofrom .db.state import StateStatusEnumwill succeed at runtime.state_manager/tests/unit/models/test_retry_policy_model.py (1)
1-377: Comprehensive coverage — looks solid.Good defaults, strategy behaviors, jitter bounds, caps, and validation checks. No issues spotted.
state_manager/app/models/graph_models.py (1)
8-8: Import path is correct. Verified thatstate_manager/app/models/trigger_models.pydefinesTrigger.state_manager/tests/unit/controller/test_get_node_run_details.py (1)
39-41: Existing patch target is correct.Stateis imported inapp.controller.get_node_run_details, so no change required.state_manager/tests/unit/controller/test_enqueue_states.py (1)
11-13: Comprehensive coverage.Good breadth across success, empty, mixed, and exception paths; assertions and patching look correct.
state_manager/tests/unit/middlewares/test_request_id_middleware.py (1)
10-16: Strong, realistic middleware tests.Covers key paths (UUID handling, logging, timing, exceptions) cleanly.
state_manager/app/models/register_nodes_response.py (1)
5-14: LGTM.Models are clear and typed appropriately for responses.
state_manager/tests/unit/controller/test_errored_state.py (1)
67-107: Good coverage and mocking strategyCovers key paths (queued, executed, retry creation) with clear assertions. LGTM.
state_manager/app/controller/get_secrets.py (1)
37-40: Verified GraphTemplate field names
GraphTemplate definesnameandnamespace, soGraphTemplate.name == state.graph_nameandGraphTemplate.namespace == namespace_nameare correct.state_manager/tests/unit/models/test_signal_models.py (2)
8-63: LGTM! Comprehensive test coverage for PruneRequestModel.The test class properly validates:
- Valid data (simple, empty, complex nested structures)
- Required field validation (missing and None values)
- Error messages in ValidationError
Tests follow AAA pattern and use appropriate pytest patterns.
133-272: LGTM! Excellent test coverage for SignalResponseModel.The test class provides thorough validation including:
- All enum values coverage (lines 223-243)
- Boundary cases (zero, large numbers)
- Type coercion (string to int, string to enum)
- JSON serialization/deserialization round-trip
- Required field validation
Tests are well-structured and follow pytest best practices.
state_manager/tests/unit/controller/test_enqueue_states_comprehensive.py (1)
14-222: LGTM! Comprehensive test suite for enqueue_states.The test suite provides excellent coverage including:
- Success path with proper result validation
- Empty results and edge cases (no states, empty nodes list)
- Error resilience (database errors, partial batch failures)
- Multiple scenarios (large batches, multiple nodes)
Tests properly mock the State model and database operations, and use appropriate async patterns.
state_manager/tests/unit/controller/test_list_registered_nodes.py (2)
11-322: LGTM! Thorough test coverage for list_registered_nodes.The test suite provides excellent coverage including:
- Success and empty result scenarios
- Error propagation (database errors, find errors)
- Filter criteria validation
- Logging verification with request IDs
- Different namespace values
- Large result sets (500 nodes)
- Return type validation
- Concurrent request handling
Tests properly use fixtures, mocks, and async patterns.
206-220: Empty namespace is valid input. Thelist_registered_nodesfunction applies no namespace validation and correctly returns an empty list for"". Tests are accurate.state_manager/tests/unit/controller/pending_test_get_graph_structure.py (1)
79-282: Good test coverage despite the filename and assertion issues.Once the filename issue is resolved, this test suite provides solid coverage:
- Success path with proper graph structure validation
- Empty result handling
- Error states with execution summary
- Complex parent-child relationships
- Exception propagation
The use of actual State instances in
test_get_graph_structure_complex_relationships(lines 190-251) is a good practice for testing complex object relationships.state_manager/app/models/store_config_model.py (1)
1-61: LGTM! Solid validation logic for store configuration.The
StoreConfigmodel provides robust validation:
- Enforces non-empty, unique keys without '.' characters
- Properly trims whitespace and normalizes data
- Provides clear error messages
- Aligns with store field reference syntax mentioned in retrieved learnings
The validators correctly handle edge cases and maintain data integrity for the store configuration.
state_manager/app/controller/get_graph_structure.py (1)
55-61: Verify GraphNode.status typeEnsure GraphNode.status expects an enum vs str. If it expects str, pass state.status.value to avoid serialization mismatches.
state_manager/tests/unit/controller/test_register_nodes.py (1)
1-435: Solid coverage and clear intentTests exercise create/update paths, mixed ops, error handling, payload echoes, and logging. Looks good.
If not already configured globally, ensure pytest-asyncio is enabled so async tests run.
state_manager/app/controller/register_nodes.py (1)
11-71: LGTM — clear upsert flow with structured logging.Create-or-update logic is sound; secrets aren’t logged; responses are minimal and safe.
state_manager/app/controller/upsert_graph_template.py (1)
64-71: Scope trigger deletion by namespace and skip empty cron expressions
In state_manager/app/controller/upsert_graph_template.py (L64–71), first buildcron_expressionsand only calldelete_manyif non-empty. Also include the correct namespace filter (verify which field onDatabaseTriggerscorresponds to namespace).state_manager/tests/unit/controller/test_trigger_graph.py (1)
18-55: Good breadth of success and error‑path coverageMocks and assertions validate core interactions and responses well.
state_manager/tests/unit/config/test_cors.py (1)
52-76: Clarify spec: empty vs whitespace‑only CORS_ORIGINSOne test expects empty string → defaults, another expects whitespace‑only → []. Ensure this difference is intentional; otherwise, align both.
Would you like me to adjust tests/implementation to make both inputs behave the same (e.g., treat whitespace‑only as empty)?
state_manager/app/controller/errored_state.py (2)
54-56: Confirm delay units (ms vs s) to avoid scheduling drift.enqueue_after is ms since epoch. Ensure compute_delay returns milliseconds; otherwise multiply by 1000.
65-74: Response/status mismatch — verify contract.You set state.status=RETRY_CREATED but return ErroredResponseModel(status=ERRORED). Confirm this is intentional for clients; otherwise return the actual status.
state_manager/app/singletons/logs_manager.py (1)
38-44: Verify desired dev log level.Setting WARNING in dev is atypical; confirm this was intentional.
state_manager/app/models/db/state.py (1)
56-63: Good call overriding insert_many to ensure fingerprints.Precomputing covers bulk inserts where Beanie may skip per-doc events.
state_manager/app/models/db/registered_node.py (1)
1-44: Note: File appears unrelated to PR objectives.This file introduces a
RegisteredNodemodel for node registration, which does not align with the PR objectives of adding a cleanup strategy for old database triggers (PR #460). Please verify that the correct files were included in this review request.state_manager/app/controller/enqueue_states.py (3)
32-69: LGTM! Concurrent state claiming is correctly implemented.The function correctly implements parallel state claiming:
- Creates
batch_sizeconcurrent tasks that compete to atomically claim available states- Uses
asyncio.gatherwithreturn_exceptions=Trueto handle partial success- Logs exceptions and continues processing successful results
- Returns a response with the count of successfully claimed states
The exception handling pattern (log and re-raise) provides good observability while preserving error propagation.
1-69: Note: File appears unrelated to PR objectives.This file introduces state enqueuing functionality, which does not align with the PR objectives of adding a cleanup strategy for old database triggers (PR #460). Please verify that the correct files were included in this review request.
15-30: Timestamp consistency verified. Theenqueue_afterfield is defined and used as Unix milliseconds across models and controllers.state_manager/tests/unit/middlewares/test_unhandled_exceptions_middleware.py (2)
1-381: LGTM! Comprehensive test coverage for UnhandledExceptionsMiddleware.The test suite thoroughly covers:
- Successful dispatch without exceptions
- Various exception types (generic, runtime, value, key, attribute errors)
- Missing and empty request IDs
- Different HTTP methods and paths
- Response structure validation
- Actual traceback formatting
- Error handling robustness (logging failures)
- Exception type preservation in logs
The parametric testing pattern (lines 347-381) is particularly well-designed for verifying consistent behavior across multiple exception types.
1-381: Note: File appears unrelated to PR objectives.This test file validates
UnhandledExceptionsMiddlewarefunctionality, which does not align with the PR objectives of adding a cleanup strategy for old database triggers (PR #460). Please verify that the correct files were included in this review request.state_manager/app/controller/get_runs.py (3)
31-86: LGTM! MongoDB aggregation pipeline correctly computes run statistics.The aggregation pipeline:
- Filters states by run IDs using
$inoperator- Groups by run_id and computes counts for each status category:
success_count: SUCCESS and PRUNED statespending_count: CREATED, QUEUED, and EXECUTED stateserrored_count: ERRORED and NEXT_CREATED_ERROR statesretried_count: RETRY_CREATED states- Uses conditional sums with
$condand$in/$eqoperators correctlyThe status categorization is appropriate for deriving run-level status.
1-137: Note: File appears unrelated to PR objectives.This file implements run listing with state aggregation, which does not align with the PR objectives of adding a cleanup strategy for old database triggers (PR #460). Please verify that the correct files were included in this review request.
110-125: Verify default status for runs without statesIn
state_manager/app/controller/get_runs.py(lines 110–125), runs not in the aggregation results are assignedstatus=RunStatusEnum.FAILED, which signals an error. If these runs haven’t started yet, should they default toPENDINGinstead? Confirm the intended behavior.state_manager/app/models/db/trigger.py (2)
1-30: This file aligns with PR objectives.The
DatabaseTriggersmodel with the TTL index onexpires_atis a key component of the trigger cleanup strategy described in PR #460. The TTL index enables MongoDB to automatically remove expired trigger documents, addressing the unbounded growth issue mentioned in the PR objectives.However, note that this model file alone does not implement the complete cleanup strategy. The PR objectives mention:
- A periodic cleanup function to remove old CANCELLED/TRIGGERED triggers
- Background task integration with FastAPI lifecycle
These additional components (setting
expires_atwhen status changes, periodic cleanup controller, FastAPI lifecycle integration) should be present in other files for the cleanup strategy to be complete.
17-30: No further action needed for expires_at handling
expires_atis correctly set on document creation (trigger_cron.py:76), and completed triggers are cleaned up via the scheduleddelete_manyjob.Likely an incorrect or invalid review comment.
state_manager/app/models/retry_policy_model.py (2)
1-69: LGTM! Well-structured retry policy implementation.The retry policy model is well-designed with comprehensive strategy support and proper validation. The code correctly implements:
- Nine retry strategies covering exponential, linear, and fixed delays with jitter variants
- Proper field validation (ge=0 for max_retries, gt=0 for backoff_factor/exponent/max_delay)
- Capping logic via
_caphelper- Clear error messages for invalid retry_count and strategy
38-66: Integer division loses precision in jitter calculations.Lines 44, 55, and 66 use integer division (
base/2) within jitter calculations, which can lose precision. For example, ifbase=2001, thenbase/2 = 1000.5, but the expressionint(base/2 + random.uniform(0, base/2))truncates toint(1000.5 + ...) = int(1000 + ...), losing 0.5ms from the calculation.Apply this diff to preserve precision:
elif self.strategy == RetryStrategy.EXPONENTIAL_EQUAL_JITTER: base = self.backoff_factor * (self.exponent ** (retry_count - 1)) - return _cap(int(base/2 + random.uniform(0, base / 2))) + half_base = base / 2 + return _cap(int(half_base + random.uniform(0, half_base))) elif self.strategy == RetryStrategy.LINEAR: return _cap(self.backoff_factor * retry_count) elif self.strategy == RetryStrategy.LINEAR_FULL_JITTER: base = self.backoff_factor * retry_count return _cap(int(random.uniform(0, base))) elif self.strategy == RetryStrategy.LINEAR_EQUAL_JITTER: base = self.backoff_factor * retry_count - return _cap(int(base/2 + random.uniform(0, base / 2))) + half_base = base / 2 + return _cap(int(half_base + random.uniform(0, half_base))) elif self.strategy == RetryStrategy.FIXED: return _cap(self.backoff_factor) elif self.strategy == RetryStrategy.FIXED_FULL_JITTER: base = self.backoff_factor return _cap(int(random.uniform(0, base))) elif self.strategy == RetryStrategy.FIXED_EQUAL_JITTER: base = self.backoff_factor - return _cap(int(base/2 + random.uniform(0, base / 2))) + half_base = base / 2 + return _cap(int(half_base + random.uniform(0, half_base)))Likely an incorrect or invalid review comment.
state_manager/tests/unit/controller/test_get_runs.py (1)
1-572: LGTM! Comprehensive test coverage.The test suite is exceptionally thorough, covering:
- Happy path with aggregation data
- Empty results
- Pagination variations
- Missing state aggregations with default handling
- Status calculation logic (PENDING/FAILED/SUCCESS)
- Exception handling and error logging
- Multiple namespaces
- Large page sizes (1000 runs)
- Edge cases (page=0, size=0)
- Aggregation pipeline structure verification
The mock setup correctly simulates the async query chains and cursor behavior.
state_manager/tests/unit/controller/test_list_graph_templates.py (1)
1-437: LGTM! Thorough test coverage for list_graph_templates.The test suite comprehensively validates:
- Successful template retrieval with proper logging
- Empty result handling
- Database and find operation errors with error logging
- Filter criteria application
- Multiple namespace handling
- Large result sets (200 templates)
- Return type validation
- Mixed validation statuses (VALID, INVALID, PENDING)
- Concurrent request handling with asyncio.gather
- Single template results
- Complex templates with nodes and secrets
The mock setup and assertions are correct and thorough.
state_manager/app/controller/trigger_graph.py (2)
18-25: LGTM! Clear validation logic.The
check_required_store_keysfunction correctly validates that all required store keys are provided, with a clear error message listing missing keys.
27-29: LGTM! Simple input merging.The
construct_inputsfunction correctly merges provided inputs with node defaults, preferring provided values.state_manager/tests/unit/controller/test_manual_retry_state.py (2)
59-116: Comprehensive happy-path and arg assertions look solidCovers status transition, field preservation/reset, and insert call. Good use of AsyncMock/MagicMock.
142-173: Good conflict path coverage (DuplicateKeyError → 409)Ensures original state is not saved on duplicate insert. This matches expected idempotency behavior.
state_manager/tests/unit/controller/test_get_graph_template.py (1)
1-10: Tests are clear and cover main branchesSuccess, not-found, invalid/pending, empty nodes, DB error, and complex secrets are all covered.
state_manager/app/main.py (1)
8-9: Verify AsyncMongoClient import path
Ensure yourpymongoversion supportsAsyncMongoClient(PyMongo ≥ 4.0) or switch to Motor’s async client:from motor.motor_asyncio import AsyncIOMotorClientA mismatched import will fail at runtime.
state_manager/app/tasks/verify_graph.py (2)
16-25: Node existence check looks good.Set-based diff is clear and efficient.
27-46: Secrets presence validation is reasonable.Keys-only presence matches GraphTemplate’s own secret validation.
|
|
||
| # Other | ||
| .env | ||
| Dockerfile |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
Trailing space in pattern; confirm intent to ignore Dockerfile.
“Dockerfile ” has a trailing space, likely not matching the Dockerfile. Also, ignoring the Dockerfile is usually not desired.
Option A (keep ignoring, fix whitespace):
-Dockerfile
+DockerfileOption B (recommended): do not ignore the Dockerfile; remove the line.
-Dockerfile 🏁 Script executed:
#!/bin/bash
sed -n '20,30p' state_manager/.dockerignore
find state_manager -maxdepth 1 -type f -name 'Dockerfile'Length of output: 122
Remove unnecessary whitespace and redundant patterns in .dockerignore
- The line
Dockerfilehas an extra space and matches nothing—remove it or trim the space. - The pattern
state_manager/Dockerfileis redundant here—remove it.
🤖 Prompt for AI Agents
In state_manager/.dockerignore around line 26, the line "Dockerfile " contains
trailing whitespace and the pattern "state_manager/Dockerfile" is redundant;
remove the trailing space from "Dockerfile" (or delete the line if unused) and
delete the redundant "state_manager/Dockerfile" entry so the file contains only
necessary, correctly formatted ignore patterns.
| *.iml | ||
| *.ipr | ||
|
|
||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add common Python tool/cache/coverage ignores.
These are standard and will reduce accidental noise in diffs.
+
+# Test & coverage caches
+.pytest_cache/
+.mypy_cache/
+.ruff_cache/
+.coverage
+.coverage.*
+htmlcov/
+.tox/
+.nox/
+
+# Python tooling
+.python-version
+pip-wheel-metadata/📝 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.
| # Test & coverage caches | |
| .pytest_cache/ | |
| .mypy_cache/ | |
| .ruff_cache/ | |
| .coverage | |
| .coverage.* | |
| htmlcov/ | |
| .tox/ | |
| .nox/ | |
| # Python tooling | |
| .python-version | |
| pip-wheel-metadata/ |
🤖 Prompt for AI Agents
state_manager/.gitignore around line 39: the file is missing common Python
ignores; add standard entries to reduce noise by appending ignores such as
__pycache__/, *.py[cod], *.pyo, .pytest_cache/, .mypy_cache/, .venv/, venv/,
env/, pip-wheel-metadata/, .coverage, coverage.xml, htmlcov/, .cache/, and
.ropeproject/ so typical Python bytecode, virtualenv, test and coverage
artifacts are excluded from git.
| #logs | ||
| *.log | ||
| logs/*.* | ||
| !logs/.gitkeep | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Make logs ignore recursive and robust.
logs/*.* misses files without extensions and nested dirs. Prefer recursive ignore and keep .gitkeep.
-#logs
-*.log
-logs/*.*
-!logs/.gitkeep
+# Logs
+*.log
+logs/**
+!logs/.gitkeep📝 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.
| #logs | |
| *.log | |
| logs/*.* | |
| !logs/.gitkeep | |
| # Logs | |
| *.log | |
| logs/** | |
| !logs/.gitkeep |
🤖 Prompt for AI Agents
In state_manager/.gitignore around lines 59-63, the current ignore rules use
"logs/*.*" which misses files without extensions and nested directories; replace
that line with a recursive pattern such as "logs/**" (or "logs/**" plus "logs/*"
if you prefer explicit top-level no-ext matches) and retain the existing
"!logs/.gitkeep" entry so .gitkeep is not ignored; ensure any redundant "*.log"
or "logs/*.*" lines are removed to avoid duplication.
|
|
||
| # local files | ||
| files/ | ||
| !files/.gitkeep No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add trailing newline at EOF.
POSIX-style text files should end with a newline. Improves diffs and tooling compatibility.
🤖 Prompt for AI Agents
In state_manager/.gitignore at line 66, the file lacks a trailing newline at
EOF; add a single POSIX newline character at the end of the file so the last
line ("!files/.gitkeep") is terminated with '\n' to satisfy tooling and diff
expectations.
| from app.config.settings import get_settings | ||
|
|
||
| def test_settings(): | ||
| settings = get_settings() | ||
| print("✅ Loaded Settings:") | ||
| print("Mongo URI:", settings.mongo_uri) | ||
| print("Database:", settings.mongo_database_name) | ||
| print("Trigger Workers:", settings.trigger_workers) | ||
| print("Trigger Retention Days:", settings.trigger_retention_days) | ||
| print("Cleanup Interval (minutes):", settings.cleanup_interval_minutes) | ||
|
|
||
| if __name__ == "__main__": | ||
| test_settings() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify the purpose of this file and consider relocation or renaming.
This file has a test_ prefix but doesn't integrate with the pytest suite (no assertions, no test class). It appears to be a manual verification utility for developers to inspect settings values.
This can cause confusion:
- Developers may expect it to be an automated test.
- Test discovery tools may attempt to run it as a test (though it won't fail).
- The location (
app/config/) is unconventional for test utilities.
Consider one of the following:
-
If this is a developer utility, rename it to make the purpose clear (e.g.,
inspect_settings.pyorverify_settings.py) and add a docstring explaining its purpose. -
If this should be an automated test, relocate it to
tests/unit/config/test_settings.pyand add proper assertions:
from app.config.settings import get_settings
def test_settings_loaded():
"""Verify settings can be loaded from environment."""
settings = get_settings()
assert settings.mongo_uri is not None
assert settings.mongo_database_name is not None
assert settings.trigger_workers > 0
assert settings.trigger_retention_days > 0
assert settings.cleanup_interval_minutes > 0| @@ -0,0 +1,85 @@ | |||
| import pytest | |||
There was a problem hiding this comment.
Remove BOM character.
Line 1 contains a UTF-8 BOM (Byte Order Mark) character which can cause issues with some tools and editors. Python source files should not include a BOM.
Apply this diff to remove the BOM:
-import pytest
+import pytest📝 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.
| import pytest | |
| import pytest |
🤖 Prompt for AI Agents
In state_manager/tests/unit/models/test_dependent_string.py around lines 1 to 1,
the file currently starts with a UTF-8 Byte Order Mark (BOM) character before
the import statement; remove the BOM so the first character is the 'i' of
"import pytest" and save the file as UTF-8 without BOM to avoid tooling/editor
issues.
| def test_validate_secret_value_valid(self): | ||
| """Test validation of valid secret value""" | ||
| # Create a valid base64 string that decodes to at least 12 bytes and is long enough | ||
| valid_bytes = b"x" * 20 | ||
| valid_base64 = base64.urlsafe_b64encode(valid_bytes).decode() | ||
| # Pad to make it at least 32 characters | ||
| padded_base64 = valid_base64 + "x" * (32 - len(valid_base64)) | ||
|
|
||
| # Should not raise any exception | ||
| GraphTemplate._validate_secret_value(padded_base64) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use a truly valid base64 string of length ≥ 32
Padding a base64 string by appending "x" can change the decoded bytes unpredictably. Encode enough bytes to naturally reach 32 chars.
Apply:
- # Create a valid base64 string that decodes to at least 12 bytes and is long enough
- valid_bytes = b"x" * 20
- valid_base64 = base64.urlsafe_b64encode(valid_bytes).decode()
- # Pad to make it at least 32 characters
- padded_base64 = valid_base64 + "x" * (32 - len(valid_base64))
-
- # Should not raise any exception
- GraphTemplate._validate_secret_value(padded_base64)
+ # Create a valid base64 string that decodes to ≥12 bytes and encodes to exactly 32 chars
+ valid_bytes = b"x" * 24 # 24 bytes -> 32 base64 chars
+ valid_base64 = base64.urlsafe_b64encode(valid_bytes).decode()
+ GraphTemplate._validate_secret_value(valid_base64)📝 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.
| def test_validate_secret_value_valid(self): | |
| """Test validation of valid secret value""" | |
| # Create a valid base64 string that decodes to at least 12 bytes and is long enough | |
| valid_bytes = b"x" * 20 | |
| valid_base64 = base64.urlsafe_b64encode(valid_bytes).decode() | |
| # Pad to make it at least 32 characters | |
| padded_base64 = valid_base64 + "x" * (32 - len(valid_base64)) | |
| # Should not raise any exception | |
| GraphTemplate._validate_secret_value(padded_base64) | |
| def test_validate_secret_value_valid(self): | |
| """Test validation of valid secret value""" | |
| # Create a valid base64 string that decodes to ≥12 bytes and encodes to exactly 32 chars | |
| valid_bytes = b"x" * 24 # 24 bytes -> 32 base64 chars | |
| valid_base64 = base64.urlsafe_b64encode(valid_bytes).decode() | |
| GraphTemplate._validate_secret_value(valid_base64) |
| def test_validate_secret_value_base64_decode_exception(self): | ||
| """Test validation when base64 decoding raises exception""" | ||
| invalid_base64 = "invalid_base64_string_that_is_long_enough_but_not_valid_base64" | ||
|
|
||
| with pytest.raises(ValueError, match="Value is not valid URL-safe base64 encoded"): | ||
| GraphTemplate._validate_secret_value(invalid_base64) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Duplicate of invalid base64 test
This repeats the earlier invalid base64 test. Remove or parametrize with the previous one to reduce redundancy.
- def test_validate_secret_value_base64_decode_exception(self):
- """Test validation when base64 decoding raises exception"""
- invalid_base64 = "invalid_base64_string_that_is_long_enough_but_not_valid_base64"
-
- with pytest.raises(ValueError, match="Value is not valid URL-safe base64 encoded"):
- GraphTemplate._validate_secret_value(invalid_base64)📝 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.
| def test_validate_secret_value_base64_decode_exception(self): | |
| """Test validation when base64 decoding raises exception""" | |
| invalid_base64 = "invalid_base64_string_that_is_long_enough_but_not_valid_base64" | |
| with pytest.raises(ValueError, match="Value is not valid URL-safe base64 encoded"): | |
| GraphTemplate._validate_secret_value(invalid_base64) |
🤖 Prompt for AI Agents
In state_manager/tests/unit/models/test_graph_template_model.py around lines 91
to 97, this test duplicates an earlier invalid base64 test; either remove this
redundant test function, or merge it into the existing invalid-base64 test by
adding this invalid_base64 string to that test's parametrized inputs and
deleting this function; ensure the remaining parametrized test asserts the same
ValueError message ("Value is not valid URL-safe base64 encoded") for all
invalid cases.
| def test_validate_secret_value_decoded_less_than_12_bytes(self): | ||
| """Test validation with decoded value less than 12 bytes""" | ||
| # This test was removed due to regex pattern mismatch issues | ||
| pass | ||
|
|
||
| # Removed failing tests that require get_collection mocking | ||
| # These tests were causing AttributeError issues with Beanie ODM | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Placeholder test should be implemented or skipped
Avoid pass-only tests. Mark as skipped with a reason, or implement.
- def test_validate_secret_value_decoded_less_than_12_bytes(self):
- """Test validation with decoded value less than 12 bytes"""
- # This test was removed due to regex pattern mismatch issues
- pass
+ @pytest.mark.skip(reason="Implement with controlled base64 that decodes to <12 bytes")
+ def test_validate_secret_value_decoded_less_than_12_bytes(self):
+ ...📝 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.
| def test_validate_secret_value_decoded_less_than_12_bytes(self): | |
| """Test validation with decoded value less than 12 bytes""" | |
| # This test was removed due to regex pattern mismatch issues | |
| pass | |
| # Removed failing tests that require get_collection mocking | |
| # These tests were causing AttributeError issues with Beanie ODM | |
| @pytest.mark.skip(reason="Implement with controlled base64 that decodes to <12 bytes") | |
| def test_validate_secret_value_decoded_less_than_12_bytes(self): | |
| ... | |
| # Removed failing tests that require get_collection mocking | |
| # These tests were causing AttributeError issues with Beanie ODM |
🤖 Prompt for AI Agents
In state_manager/tests/unit/models/test_graph_template_model.py around lines 109
to 116, the placeholder test
test_validate_secret_value_decoded_less_than_12_bytes currently contains only
pass which is not allowed; either implement the validation scenario or mark the
test as skipped with a reason. Fix by replacing the pass with a
pytest.skip("Reason: placeholder test - implement later") or add a real
assertion: construct the minimal GraphTemplateModel (or call the validator) with
a secret value that decodes to fewer than 12 bytes, assert the validator raises
the expected ValidationError, and import pytest at the top if not already
present.
| def test_is_valid_valid_status(self): | ||
| """Test is_valid method with valid status""" | ||
| # This test doesn't require GraphTemplate instantiation | ||
| assert GraphTemplate.is_valid.__name__ == "is_valid" | ||
|
|
||
| def test_is_valid_invalid_status(self): | ||
| """Test is_valid method with invalid status""" | ||
| # This test doesn't require GraphTemplate instantiation | ||
| assert GraphTemplate.is_valid.__name__ == "is_valid" | ||
|
|
||
| def test_is_validating_ongoing_status(self): | ||
| """Test is_validating method with ongoing status""" | ||
| # This test doesn't require GraphTemplate instantiation | ||
| assert GraphTemplate.is_validating.__name__ == "is_validating" | ||
|
|
||
| def test_is_validating_pending_status(self): | ||
| """Test is_validating method with pending status""" | ||
| # This test doesn't require GraphTemplate instantiation | ||
| assert GraphTemplate.is_validating.__name__ == "is_validating" | ||
|
|
||
| def test_is_validating_invalid_status(self): | ||
| """Test is_validating method with invalid status""" | ||
| # This test doesn't require GraphTemplate instantiation | ||
| assert GraphTemplate.is_validating.__name__ == "is_validating" | ||
|
|
||
| # Removed failing tests that require GraphTemplate instantiation | ||
| # These tests were causing get_collection AttributeError issues | ||
|
|
||
| def test_get_valid_success(self): | ||
| """Test get_valid method with successful validation""" | ||
| # This test doesn't require GraphTemplate instantiation | ||
| assert GraphTemplate.get_valid.__name__ == "get_valid" | ||
|
|
||
| def test_get_valid_ongoing_then_valid(self): | ||
| """Test get_valid method with ongoing then valid status""" | ||
| # This test doesn't require GraphTemplate instantiation | ||
| assert GraphTemplate.get_valid.__name__ == "get_valid" | ||
|
|
||
| def test_get_valid_invalid_status(self): | ||
| """Test get_valid method with invalid status""" | ||
| # This test doesn't require GraphTemplate instantiation | ||
| assert GraphTemplate.get_valid.__name__ == "get_valid" | ||
|
|
||
| def test_get_valid_timeout(self): | ||
| """Test get_valid method with timeout""" | ||
| # This test doesn't require GraphTemplate instantiation | ||
| assert GraphTemplate.get_valid.__name__ == "get_valid" | ||
|
|
||
| def test_get_valid_exception_handling(self): | ||
| """Test get_valid method exception handling""" | ||
| # This test doesn't require GraphTemplate instantiation | ||
| assert GraphTemplate.get_valid.__name__ == "get_valid" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove no-op method name assertions (no real coverage)
Tests asserting name do not validate behavior. Delete or replace with meaningful unit tests, or mark skipped.
- def test_is_valid_valid_status(self):
- """Test is_valid method with valid status"""
- # This test doesn't require GraphTemplate instantiation
- assert GraphTemplate.is_valid.__name__ == "is_valid"
-
- def test_is_valid_invalid_status(self):
- """Test is_valid method with invalid status"""
- # This test doesn't require GraphTemplate instantiation
- assert GraphTemplate.is_valid.__name__ == "is_valid"
-
- def test_is_validating_ongoing_status(self):
- """Test is_validating method with ongoing status"""
- # This test doesn't require GraphTemplate instantiation
- assert GraphTemplate.is_validating.__name__ == "is_validating"
-
- def test_is_validating_pending_status(self):
- """Test is_validating method with pending status"""
- # This test doesn't require GraphTemplate instantiation
- assert GraphTemplate.is_validating.__name__ == "is_validating"
-
- def test_is_validating_invalid_status(self):
- """Test is_validating method with invalid status"""
- # This test doesn't require GraphTemplate instantiation
- assert GraphTemplate.is_validating.__name__ == "is_validating"
+ # Replace these placeholders with real status matrix tests, or mark them skipped.📝 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.
| def test_is_valid_valid_status(self): | |
| """Test is_valid method with valid status""" | |
| # This test doesn't require GraphTemplate instantiation | |
| assert GraphTemplate.is_valid.__name__ == "is_valid" | |
| def test_is_valid_invalid_status(self): | |
| """Test is_valid method with invalid status""" | |
| # This test doesn't require GraphTemplate instantiation | |
| assert GraphTemplate.is_valid.__name__ == "is_valid" | |
| def test_is_validating_ongoing_status(self): | |
| """Test is_validating method with ongoing status""" | |
| # This test doesn't require GraphTemplate instantiation | |
| assert GraphTemplate.is_validating.__name__ == "is_validating" | |
| def test_is_validating_pending_status(self): | |
| """Test is_validating method with pending status""" | |
| # This test doesn't require GraphTemplate instantiation | |
| assert GraphTemplate.is_validating.__name__ == "is_validating" | |
| def test_is_validating_invalid_status(self): | |
| """Test is_validating method with invalid status""" | |
| # This test doesn't require GraphTemplate instantiation | |
| assert GraphTemplate.is_validating.__name__ == "is_validating" | |
| # Removed failing tests that require GraphTemplate instantiation | |
| # These tests were causing get_collection AttributeError issues | |
| def test_get_valid_success(self): | |
| """Test get_valid method with successful validation""" | |
| # This test doesn't require GraphTemplate instantiation | |
| assert GraphTemplate.get_valid.__name__ == "get_valid" | |
| def test_get_valid_ongoing_then_valid(self): | |
| """Test get_valid method with ongoing then valid status""" | |
| # This test doesn't require GraphTemplate instantiation | |
| assert GraphTemplate.get_valid.__name__ == "get_valid" | |
| def test_get_valid_invalid_status(self): | |
| """Test get_valid method with invalid status""" | |
| # This test doesn't require GraphTemplate instantiation | |
| assert GraphTemplate.get_valid.__name__ == "get_valid" | |
| def test_get_valid_timeout(self): | |
| """Test get_valid method with timeout""" | |
| # This test doesn't require GraphTemplate instantiation | |
| assert GraphTemplate.get_valid.__name__ == "get_valid" | |
| def test_get_valid_exception_handling(self): | |
| """Test get_valid method exception handling""" | |
| # This test doesn't require GraphTemplate instantiation | |
| assert GraphTemplate.get_valid.__name__ == "get_valid" | |
| # Replace these placeholders with real status matrix tests, or mark them skipped. | |
| # Removed failing tests that require GraphTemplate instantiation | |
| # These tests were causing get_collection AttributeError issues | |
| def test_get_valid_success(self): | |
| """Test get_valid method with successful validation""" | |
| # This test doesn't require GraphTemplate instantiation | |
| assert GraphTemplate.get_valid.__name__ == "get_valid" | |
| def test_get_valid_ongoing_then_valid(self): | |
| """Test get_valid method with ongoing then valid status""" | |
| # This test doesn't require GraphTemplate instantiation | |
| assert GraphTemplate.get_valid.__name__ == "get_valid" | |
| def test_get_valid_invalid_status(self): | |
| """Test get_valid method with invalid status""" | |
| # This test doesn't require GraphTemplate instantiation | |
| assert GraphTemplate.get_valid.__name__ == "get_valid" | |
| def test_get_valid_timeout(self): | |
| """Test get_valid method with timeout""" | |
| # This test doesn't require GraphTemplate instantiation | |
| assert GraphTemplate.get_valid.__name__ == "get_valid" | |
| def test_get_valid_exception_handling(self): | |
| """Test get_valid method exception handling""" | |
| # This test doesn't require GraphTemplate instantiation | |
| assert GraphTemplate.get_valid.__name__ == "get_valid" |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Hi @ak4shravikumar |
3 participants
Problem:
The DatabaseTriggers collection currently keeps all triggers indefinitely. Over time, this can lead to unbounded growth in the database, impacting performance and storage.
Solution:
Benefits:
Testing:
Related Issues:
Related Issues: