Conversation
added 2 commits
February 18, 2026 21:44
- Consolidate objective JSON files into daily.json, weekly.json, achievements.json - Remove redundant files (test_objectives, basic_objectives, assign, collect) - Add assign_correct objective type for SPECIAL-matched room assignments - Add DWELLER_ASSIGNED_CORRECTLY event emitted when dweller's best stat matches room ability - Add item table migration and fix objective.category to use String type - Add rewardtype enum values (STIMPAK, RADAWAY, LUNCHBOX) - Add ObjectiveAssignmentService for daily/weekly objective management - Update frontend to support new objective types and categories
- Add category field to get_multi_for_vault response - Remove default value from category field to prevent confusion - Update tests to include category in all objective test data
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds objective categories and datasets (daily/weekly/achievement), services and evaluators to assign/refresh objectives, Celery tasks/schedules to refresh them, DB migrations (item table + objective.category and enum additions), RustFS bucket policy tooling and scripts, frontend/type/UI updates, and related tests. Changes
Sequence DiagramsequenceDiagram
actor Beat as Celery Beat
participant Task as Refresh Task
participant Service as ObjectiveAssignmentService
participant DB as Database
participant EventBus as Event Bus
Beat->>Task: trigger (daily/weekly)
Task->>Service: refresh_*_objectives(vault_id)
Service->>DB: BEGIN transaction / delete VaultObjectiveProgressLink (clear)
DB-->>Service: deletion result
Service->>DB: query objectives for category
DB-->>Service: objectives list
Service->>DB: insert VaultObjectiveProgressLink entries (assign)
DB-->>Service: insert result / COMMIT
Service->>EventBus: emit assignment events (optional)
Service-->>Task: return summary
Task->>Task: log completion
Note over Task,Service: on error -> Task.self.retry(countdown=3600)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/services/reward_service.py (1)
212-213:⚠️ Potential issue | 🟡 MinorComment says "random dweller" but code always picks
dwellers[0].Lines 212 and 234 comment "Grant to random dweller" but the implementation uses
dwellers[0]. This appears to be a pre-existing issue, but since the surrounding lines changed (the query method), it's worth flagging.Also applies to: 234-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/reward_service.py` around lines 212 - 213, The comment says "Grant to random dweller" but the code always picks dwellers[0]; update both places where the variable dwellers is used to select the beneficiary (the blocks with the "Grant to random dweller" comment) to choose a random element instead: import and use Python's random.choice(dwellers) (or equivalent utility) and keep the existing handling for empty lists (ensure you still guard when dwellers is empty). Replace dweller = dwellers[0] with dweller = random.choice(dwellers) (and add the random import) in the functions/methods that perform the reward grant.
🧹 Nitpick comments (16)
backend/app/alembic/versions/2026_02_18_2104-5934c2bdf3e1_add_item_table_objective_category_and_.py (1)
43-45: Downgrade leavesrewardtypeenum values in place — document the limitation.PostgreSQL has no
ALTER TYPE ... DROP VALUE, so rolling back to before this migration will leaveSTIMPAK,RADAWAY, andLUNCHBOXin therewardtypeenum. The current downgrade silently omits any attempt to undo this. Add a comment making the limitation explicit so operators are aware that a database-level manual intervention is required for a complete rollback.📝 Suggested comment in downgrade
def downgrade() -> None: + # NOTE: The rewardtype enum values STIMPAK/RADAWAY/LUNCHBOX added in upgrade() + # cannot be removed by downgrade — PostgreSQL does not support DROP VALUE. + # Manual intervention is required if a full rollback is needed. op.drop_index(op.f("ix_objective_category"), table_name="objective")Also applies to: 48-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/alembic/versions/2026_02_18_2104-5934c2bdf3e1_add_item_table_objective_category_and_.py` around lines 43 - 45, The downgrade currently does nothing to remove the enum values added with op.execute("ALTER TYPE rewardtype ADD VALUE IF NOT EXISTS 'STIMPAK'"), op.execute("ALTER TYPE rewardtype ADD VALUE IF NOT EXISTS 'RADAWAY'"), and op.execute("ALTER TYPE rewardtype ADD VALUE IF NOT EXISTS 'LUNCHBOX'"); add a clear comment inside the downgrade() function stating that PostgreSQL does not support DROP VALUE for enum types, so these values will remain and manual DB-level steps are required to fully revert (include the enum name 'rewardtype' and the three values in the comment to make the limitation explicit to operators).backend/app/core/celery.py (1)
38-42: Interval-based weekly schedule won't align with calendar weeks.
schedule: 604800.0counts seconds from when Celery Beat starts, not from Monday midnight (or any wall-clock anchor). A Beat worker restart resets the countdown. For true weekly cadence usecrontab:♻️ Suggested crontab-based schedules
+from celery.schedules import crontab ... - "refresh-daily-objectives": { - "task": "refresh_daily_objectives", - "schedule": 86400.0, - "options": {"expires": 82800}, - }, - "refresh-weekly-objectives": { - "task": "refresh_weekly_objectives", - "schedule": 604800.0, - "options": {"expires": 518400}, - }, + "refresh-daily-objectives": { + "task": "refresh_daily_objectives", + "schedule": crontab(hour=0, minute=0), # daily at midnight UTC + "options": {"expires": 82800}, + }, + "refresh-weekly-objectives": { + "task": "refresh_weekly_objectives", + "schedule": crontab(hour=0, minute=0, day_of_week=1), # Mondays at midnight UTC + "options": {"expires": 518400}, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/core/celery.py` around lines 38 - 42, The current "refresh-weekly-objectives" entry uses an interval seconds schedule (schedule: 604800.0) which resets on Beat restart; replace it with a crontab-based schedule so it runs at a fixed wall-clock time (e.g., crontab(minute=0, hour=0, day_of_week='monday') to run at Monday midnight). Update the "refresh-weekly-objectives" task definition in celery.py to import and use Celery's crontab instead of the numeric interval while preserving existing "task" and "options" keys (adjust or remove "expires" if it no longer applies).backend/app/tests/test_utils/test_seed_objectives.py (2)
195-196: Test for schema validation only omitsreward— consider also testing missingcategory.Since
categoryis a new required field, it would be valuable to verify that an objective with a missingcategoryis also rejected by the seeding logic.💡 Suggested addition
invalid_data = [ {"challenge": "Missing reward field", "category": "achievement"}, # Missing 'reward' {"challenge": "Missing category", "reward": "50 caps"}, # Missing 'category' {"challenge": "Valid objective", "reward": "50 caps", "category": "achievement"}, # Valid ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_utils/test_seed_objectives.py` around lines 195 - 196, Update the test's invalid_data test vector to also include an objective missing the required "category" field so schema validation is asserted for both missing reward and missing category; specifically, in the test that builds invalid_data (variable name invalid_data in test_seed_objectives.py) add an entry like {"challenge": "Missing category", "reward": "50 caps"} before the valid objective so the seeding logic rejects objects without a category as well.
24-26: All test data uses"achievement"category — consider adding coverage for"daily"and"weekly".Every test fixture in this file uses
"category": "achievement". While this validates the field is persisted, it doesn't exercise the other enum values. A minor gap, but something to consider for more thorough coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_utils/test_seed_objectives.py` around lines 24 - 26, The test fixtures in test_seed_objectives.py only create objectives with "category": "achievement", so add at least one test objective with "category": "daily" and one with "category": "weekly" in the seeded test data (e.g., extend the list of dicts that currently contains {"challenge": "Collect 3 outfits", ...} entries) and update the related test assertions (or parameterize the assertions) to verify those categories are persisted and handled correctly; look for the seeded data structure in test_seed_objectives.py and the assertions that inspect objective.category to add checks for "daily" and "weekly".backend/app/crud/dweller.py (2)
248-248: Redundant inline import —GameEventandevent_busare already imported at the module level.Line 23 already imports these:
from app.services.event_bus import GameEvent, event_bus🧹 Remove the redundant import
# Emit dweller assigned event for objective tracking - from app.services.event_bus import GameEvent, event_bus - await event_bus.emit(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/crud/dweller.py` at line 248, Remove the redundant inline import of GameEvent and event_bus inside the function in backend/app/crud/dweller.py—there's already a module-level import "from app.services.event_bus import GameEvent, event_bus" (near the top), so delete the duplicate "from app.services.event_bus import GameEvent, event_bus" at line 248 and use the existing GameEvent and event_bus references directly.
256-273: Tie-breaking on equal SPECIAL stats is insertion-order-dependent; also duplicates logic fromauto_assign_to_best_room.When multiple SPECIAL stats are tied for the highest value,
max()returns the first key in dict insertion order (i.e., always "strength"). This means a dweller with equal strength and intelligence assigned to an intelligence room would not triggerDWELLER_ASSIGNED_CORRECTLY. The same pattern exists inauto_assign_to_best_room(lines 462-472), so this is at least consistent, but worth noting as a design choice.Additionally, the SPECIAL stats dict construction and max-finding logic is duplicated between these two methods. Consider extracting a helper, e.g.:
♻️ Extract shared helper
`@staticmethod` def _get_highest_special(dweller_obj: Dweller) -> str: """Return the name of the dweller's highest SPECIAL stat.""" special_stats = { "strength": dweller_obj.strength, "perception": dweller_obj.perception, "endurance": dweller_obj.endurance, "charisma": dweller_obj.charisma, "intelligence": dweller_obj.intelligence, "agility": dweller_obj.agility, "luck": dweller_obj.luck, } return max(special_stats, key=special_stats.get)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/crud/dweller.py` around lines 256 - 273, The current logic for picking the dweller's highest SPECIAL uses dict insertion order so ties always pick "strength" and the same code is duplicated in auto_assign_to_best_room; extract a single helper (e.g., a static/class method named _get_highest_special(dweller_obj, preferred: Optional[str]=None) used by both places) that builds the SPECIAL map once, resolves ties deterministically (prefer a provided preferred stat like the room's ability when present, otherwise pick by a stable rule such as alphabetical order or explicit priority list), and replace the inline max(...) usage in both the DWELLER_ASSIGNED_CORRECTLY check and auto_assign_to_best_room with calls to this helper so tie behavior is predictable and duplication is removed.frontend/src/modules/progression/views/ObjectivesView.vue (1)
26-31:activeObjectivesis now unused dead code.With the switch to category-based filtering (
dailyObjectives,weeklyObjectives,achievementObjectives), the genericactiveObjectivescomputed property on line 30 is no longer referenced in the template. ThefilterObjectiveshelper is only used forcompletedObjectivesnow.🧹 Remove unused code
-const filterObjectives = (status: boolean) => { - return objectivesStore.objectives.filter((objective) => objective.is_completed === status) -} - -const activeObjectives = computed(() => filterObjectives(false)) -const completedObjectives = computed(() => filterObjectives(true)) +const completedObjectives = computed(() => + objectivesStore.objectives.filter((objective) => objective.is_completed) +)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/modules/progression/views/ObjectivesView.vue` around lines 26 - 31, Remove the now-unused activeObjectives computed declaration: delete the line "const activeObjectives = computed(() => filterObjectives(false))" (and any direct references to activeObjectives), leaving filterObjectives and completedObjectives as-is (or inline filterObjectives into completedObjectives if you prefer). Ensure there are no remaining template or script references to activeObjectives so there are no unused-symbol warnings for activeObjectives or stale bindings on objectivesStore.objectives.backend/app/tests/test_services/test_objective_evaluators.py (1)
631-740: Addcategoryfield to allObjectiveconstructions for test resilience.While these in-memory tests work without
category, the earlier persisted-Objective tests (e.g., line 64) also omit it. Sincecategoryis a required field in the model definition, settingcategory="achievement"across all testObjectiveinstances ensures tests remain valid if database constraints become stricter and makes test data more realistic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_services/test_objective_evaluators.py` around lines 631 - 740, Add the missing required category field to every Objective instance in these tests (e.g., within TestExpeditionEvaluator and TestLevelUpEvaluator) by setting category="achievement" on each Objective constructor call (the Objective(...) usages inside test_matches_any_quest_type, test_matches_specific_quest_type, test_does_not_match_wrong_quest_type, test_matches_wildcard_quest_type, test_matches_min_level_met, test_matches_exceeds_min_level, test_does_not_match_below_min_level, and test_matches_no_min_level_requirement) so the model's required field is present and tests remain valid under stricter persistence constraints.backend/app/services/objective_assignment_service.py (3)
13-14: DeadTYPE_CHECKINGblock.The
if TYPE_CHECKING: passblock imports nothing and can be removed along with theTYPE_CHECKINGimport fromtyping.♻️ Suggested fix
-from typing import TYPE_CHECKING - from pydantic import UUID4 from sqlmodel import select from sqlmodel.ext.asyncio.session import AsyncSession from app.models.objective import Objective from app.models.vault_objective import VaultObjectiveProgressLink from app.schemas.common import ObjectiveCategoryEnum - -if TYPE_CHECKING: - pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/objective_assignment_service.py` around lines 13 - 14, Remove the dead TYPE_CHECKING block and its unused import: delete the "if TYPE_CHECKING: pass" block in objective_assignment_service.py and remove TYPE_CHECKING from the typing import list, ensuring no other references to TYPE_CHECKING remain in the file.
74-80: Non-atomic refresh: clear and assign commit separately.
refresh_daily_objectivescallsclear_daily_objectives(which commits) thenassign_daily_objectives(which also commits). If the process crashes between the two, the vault is left with zero daily objectives. Consider managing the transaction at the refresh level instead of committing inside each sub-method, or at minimum documenting this as a known trade-off.♻️ One approach: defer commit to the caller
Add an optional
auto_commit: bool = Trueparameter to_clear_category_objectivesand_assign_category_objectives, and passauto_commit=Falsewhen called fromrefresh_*, then commit once after both operations complete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/objective_assignment_service.py` around lines 74 - 80, The refresh methods perform clear and assign as separate commits which can leave a vault empty if a crash occurs; update the clear and assign methods (clear_daily_objectives, assign_daily_objectives, clear_weekly_objectives, assign_weekly_objectives and their shared helpers _clear_category_objectives/_assign_category_objectives) to accept an optional auto_commit: bool = True flag, have their default behavior unchanged, and when called from refresh_daily_objectives and refresh_weekly_objectives pass auto_commit=False and perform a single commit (or explicit transaction commit) after both operations complete so the clear+assign are atomic; alternatively wrap the two calls inside a single transaction at the refresh_* level using the same DB session/transaction.
82-113: N+1 queries in_assign_category_objectives.Each candidate objective triggers an individual
_objective_already_assignedquery. For small counts (5 daily / 3 weekly) this is tolerable, but could become noticeable with larger achievement pools. A single query fetching all assigned objective IDs for the vault + category would collapse this to one round-trip.♻️ Suggested optimization
async def _assign_category_objectives( self, vault_id: UUID4, category: ObjectiveCategoryEnum, count: int ) -> list[Objective]: query = select(Objective).where(Objective.category == category) result = await self._db_session.execute(query) all_objectives = list(result.scalars().all()) if not all_objectives: logger.warning(f"No {category} objectives found in database") return [] - available = [obj for obj in all_objectives if not await self._objective_already_assigned(vault_id, obj.id)] + # Fetch all assigned objective IDs in one query + assigned_query = select(VaultObjectiveProgressLink.objective_id).where( + VaultObjectiveProgressLink.vault_id == vault_id, + ) + assigned_result = await self._db_session.execute(assigned_query) + assigned_ids = {row[0] for row in assigned_result.all()} + + available = [obj for obj in all_objectives if obj.id not in assigned_ids]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/objective_assignment_service.py` around lines 82 - 113, The method _assign_category_objectives currently calls _objective_already_assigned for each Objective causing N+1 queries; instead run a single query up-front to fetch all assigned objective IDs for the given vault (and category if needed) and use that set to filter all_objectives into available before sampling; update the logic that builds VaultObjectiveProgressLink and adds them to the session the same way, then commit once as before. Target symbols: _assign_category_objectives, _objective_already_assigned (remove per-object awaits), Objective, and VaultObjectiveProgressLink.frontend/src/modules/progression/components/QuestCard.vue (2)
2-7: Import order: relative import should come after@/aliases.Line 4 uses a relative import (
'../models/quest') but it's placed before the@/alias imports on Lines 5-7. The guideline specifies: Vue/core → third-party →@/→ relative.♻️ Suggested reorder
import { computed, ref, onMounted, onUnmounted, watch } from 'vue' import { Icon } from '@iconify/vue' -import type { VaultQuest, QuestPartyMember } from '../models/quest' import type { DwellerShort } from '@/modules/dwellers/models/dweller' import { UCard, UBadge, UButton } from '@/core/components/ui' import { useQuestStore } from '@/stores/quest' +import type { VaultQuest, QuestPartyMember } from '../models/quest'As per coding guidelines, "Prefer @ aliases for imports (frontend/tsconfig.app.json paths); order imports: Vue/core → third-party →
@/→ relative".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/modules/progression/components/QuestCard.vue` around lines 2 - 7, The import order is incorrect: move the relative import '../models/quest' so it comes after the '@/...' alias imports (UCard, UBadge, UButton, useQuestStore) to follow the Vue/core → third-party → `@/` → relative rule; specifically reorder so computed, ref, onMounted, onUnmounted, watch and Icon remain first, then the '@/...' imports (DwellerShort, UCard, UBadge, UButton, useQuestStore), and then the relative types VaultQuest and QuestPartyMember are imported last.
142-179:formatRewardsignature exceeds 100-character line width.Line 143 is well over the 100-character limit. Consider breaking the parameter type into a named interface or multi-line parameter.
♻️ Suggested fix
-const formatReward = (reward: { reward_type: string; reward_data: Record<string, unknown>; reward_chance: number }) => { +const formatReward = (reward: { + reward_type: string + reward_data: Record<string, unknown> + reward_chance: number +}) => {As per coding guidelines, "Use 100 character line width for frontend code (enforced by oxlint.json)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/modules/progression/components/QuestCard.vue` around lines 142 - 179, The formatReward function signature is over the 100-char line width; refactor its parameter typing by creating a dedicated interface (e.g., interface Reward { reward_type: string; reward_data: Record<string, unknown>; reward_chance: number }) and then change the signature to use that type (e.g., const formatReward = (reward: Reward) => { ... }) or split the parameter across multiple lines so the line length is under 100 chars; update any imports/exports and keep the function body unchanged (refer to formatReward and the reward parameter).frontend/src/modules/progression/models/objective.ts (1)
7-21: New type definitions and interface extensions look good.The new
ObjectiveCategory,ObjectiveType, and the additionalObjectivefields are well-structured and align with the backend enums.One minor note: Line 9 is ~103 characters, which slightly exceeds the 100-character line width guideline.
♻️ Suggested wrap
-export type ObjectiveType = 'collect' | 'build' | 'train' | 'assign' | 'reach' | 'expedition' | 'level_up' +export type ObjectiveType = + | 'collect' + | 'build' + | 'train' + | 'assign' + | 'reach' + | 'expedition' + | 'level_up'As per coding guidelines, "Use 100 character line width for frontend code (enforced by oxlint.json)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/modules/progression/models/objective.ts` around lines 7 - 21, The ObjectiveType union on ObjectiveType is longer than 100 chars; split the union into multiple lines (one union member per line or grouped logically) or refactor into separate type aliases so the ObjectiveType declaration and its line(s) stay within the 100-character width guideline; update the ObjectiveType type definition referenced in frontend/src/modules/progression/models/objective.ts accordingly and ensure the file still exports ObjectiveType for use by the Objective interface.backend/app/services/reward_service.py (1)
204-206: Switched toget_multi_by_vault— correct, but the post-filter foris_deletedon Lines 206/228 is now redundant.
get_multi_by_vaultalready filters out soft-deleted dwellers by default (seebackend/app/crud/dweller.py, lines 85-87:include_deleted: bool = False→query = query.where(self.model.is_deleted == False)). The subsequent list comprehension[d for d in dwellers if not d.is_deleted]is a no-op.Also applies to: 226-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/reward_service.py` around lines 204 - 206, The list-comprehension post-filtering on dwellers is redundant because dweller_crud.get_multi_by_vault already excludes soft-deleted records; remove the redundant filtering expressions that reassign dwellers (the “[d for d in dwellers if not d.is_deleted]” lines) in reward_service.py so the code simply uses the returned dwellers from get_multi_by_vault (refer to the dwellers variable and get_multi_by_vault call to locate the spots, including the second occurrence later in the file).backend/app/api/celery_task.py (1)
194-237: Near-identical tasks — extract shared logic to reduce duplication.
refresh_daily_objectives_taskandrefresh_weekly_objectives_taskdiffer only in the service method called and the log/task name. The boilerplate for engine/session creation, vault iteration, and error handling is duplicated ~40 lines each.♻️ Suggested refactor: extract a shared helper
+async def _refresh_objectives_for_all_vaults(refresh_method_name: str) -> dict: + from sqlalchemy import select + from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine + + from app.core.config import settings + from app.models.vault import Vault + from app.services.objective_assignment_service import ObjectiveAssignmentService + + engine = create_async_engine( + str(settings.ASYNC_DATABASE_URI), + echo=False, + future=True, + pool_pre_ping=True, + ) + session_maker = async_sessionmaker(engine, expire_on_commit=False) + + try: + async with session_maker() as session: + result = await session.execute(select(Vault.id).where(Vault.is_deleted == False)) + vault_ids = [row[0] for row in result.all()] + + total_assigned = 0 + for vault_id in vault_ids: + service = ObjectiveAssignmentService(session) + assigned = await getattr(service, refresh_method_name)(vault_id) + total_assigned += len(assigned) + + return {"vaults_processed": len(vault_ids), "objectives_assigned": total_assigned} + finally: + await engine.dispose() + + `@celery_app.task`(name="refresh_daily_objectives", bind=True) def refresh_daily_objectives_task(self): """Refresh daily objectives for all vaults. Scheduled to run daily via Celery Beat.""" try: logger.info("Starting daily objectives refresh") - - async def run_refresh(): - ... - - result = asyncio.run(run_refresh()) + result = asyncio.run(_refresh_objectives_for_all_vaults("refresh_daily_objectives")) except Exception as e: logger.exception("Daily objectives refresh failed") raise self.retry(exc=e, countdown=3600) from e else: logger.info(f"Daily objectives refresh completed: {result}") return resultAlso applies to: 240-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/celery_task.py` around lines 194 - 237, Both refresh_daily_objectives_task and refresh_weekly_objectives_task duplicate engine/session setup, vault iteration, and error/retry handling; extract a shared helper (e.g., _run_objective_refresh) that creates the async engine via create_async_engine, builds async_sessionmaker, queries Vault.id, iterates vault_ids, constructs ObjectiveAssignmentService(session) and invokes a passed-in coroutine method (like service.refresh_daily_objectives or service.refresh_weekly_objectives) for each vault, accumulates results, disposes the engine, and returns the summary; then call that helper from refresh_daily_objectives_task and refresh_weekly_objectives_task and preserve the same try/except that logs via logger.exception and calls self.retry(exc=e, countdown=3600).
🤖 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/app/alembic/versions/2026_02_18_2104-5934c2bdf3e1_add_item_table_objective_category_and_.py`:
- Line 9: Replace the outdated typing imports to satisfy Ruff: change imports
using Sequence and Union so Sequence is imported from collections.abc and Union
uses the modern | syntax; specifically update the import line that currently
reads "from typing import Sequence, Union" to import Sequence from
collections.abc and remove Union usage in type annotations (e.g., replace
occurrences of "Union[A, B]" with "A | B"), and apply the same changes to the
other occurrences referenced around the file (lines ~17-19) including any
function signatures or variable annotations that use Sequence or Union.
- Around line 37-40: The migration adds the "category" column to the "objective"
table with server_default="ACHIEVEMENT" which mismatches the serialized enum
values (ObjectiveCategoryEnum uses "achievement"); update the op.add_column call
for the "objective" table (the sa.Column named "category") to use
server_default="achievement" (lowercase) so defaults in the DB match the enum
string values and won’t be missed by case-sensitive queries.
In `@backend/app/api/celery_task.py`:
- Around line 222-225: Loop over vault_ids currently calls
ObjectiveAssignmentService(session).refresh_daily_objectives (and
refresh_weekly_objectives) without isolating errors, so a raise for one vault
aborts the whole task; change the per-vault invocation inside the for vault_id
in vault_ids loops to wrap the call to service.refresh_daily_objectives and
service.refresh_weekly_objectives in a try/except that catches Exception, logs
the vault_id and error (include traceback/context), and continues so
total_assigned accumulates only successful assignments; do the same for the
other loop (the block calling refresh_weekly_objectives) and ensure you still
use the same session/service variables and update total_assigned as before.
- Around line 216-226: The current queries call select(Vault.id) and include
soft-deleted vaults; update both daily and weekly refresh blocks to filter out
soft-deleted records by adding a WHERE clause that only selects vaults with no
deletion timestamp (e.g., filter where Vault.deleted_at is NULL or use the
model's non-deleted helper if available). Locate the select(Vault.id) usages in
the async session blocks that call
ObjectiveAssignmentService(session).refresh_daily_objectives and
refresh_weekly_objectives and modify those selects to exclude soft-deleted
Vaults so only active vault_ids are iterated.
In `@backend/app/models/objective.py`:
- Around line 19-22: The DB column for the "category" field is currently
nullable which conflicts with the non-optional Python type
ObjectiveCategoryEnum; update the Field's sa_column for the category attribute
so Column(String(50), index=True) includes nullable=False to enforce NOT NULL at
the DB level and keep schema consistent with the ObjectiveCategoryEnum type hint
(update the sa_column in the category Field declaration in objective.py).
In `@backend/app/services/objective_evaluators.py`:
- Around line 364-407: LevelUpEvaluator._matches currently assumes
objective.target_entity["min_level"] is an int and may raise TypeError if it's a
string; update validation by coercing and validating min_level before comparison
(e.g., read target = objective.target_entity or {}, get raw_min =
target.get("min_level", 1), attempt to convert with int(raw_min) inside a
try/except and fallback to 1 on failure or log/raise a clear validation error),
then compare new_level >= min_level; alternatively enforce the shape in the
ObjectiveCreate/Pydantic schema so target_entity.min_level is always an int —
implement one of these fixes referencing LevelUpEvaluator, _matches, and
min_level.
In `@backend/app/services/reward_service.py`:
- Around line 271-274: The rarity weights currently sum to 0.9 in the
random.choices call that assigns rarity (the block using RarityEnum.COMMON,
RARE, LEGENDARY and the local variable rarity); update the weights so they sum
to 1.0 to avoid the apparent typo—either set explicit intended probabilities
(e.g., weights=[0.6, 0.3, 0.1]) or evenly redistribute the missing 0.1 (e.g.,
weights=[0.6333, 0.2333, 0.1333]) and replace the existing weights array in the
random.choices call accordingly.
---
Outside diff comments:
In `@backend/app/services/reward_service.py`:
- Around line 212-213: The comment says "Grant to random dweller" but the code
always picks dwellers[0]; update both places where the variable dwellers is used
to select the beneficiary (the blocks with the "Grant to random dweller"
comment) to choose a random element instead: import and use Python's
random.choice(dwellers) (or equivalent utility) and keep the existing handling
for empty lists (ensure you still guard when dwellers is empty). Replace dweller
= dwellers[0] with dweller = random.choice(dwellers) (and add the random import)
in the functions/methods that perform the reward grant.
---
Nitpick comments:
In
`@backend/app/alembic/versions/2026_02_18_2104-5934c2bdf3e1_add_item_table_objective_category_and_.py`:
- Around line 43-45: The downgrade currently does nothing to remove the enum
values added with op.execute("ALTER TYPE rewardtype ADD VALUE IF NOT EXISTS
'STIMPAK'"), op.execute("ALTER TYPE rewardtype ADD VALUE IF NOT EXISTS
'RADAWAY'"), and op.execute("ALTER TYPE rewardtype ADD VALUE IF NOT EXISTS
'LUNCHBOX'"); add a clear comment inside the downgrade() function stating that
PostgreSQL does not support DROP VALUE for enum types, so these values will
remain and manual DB-level steps are required to fully revert (include the enum
name 'rewardtype' and the three values in the comment to make the limitation
explicit to operators).
In `@backend/app/api/celery_task.py`:
- Around line 194-237: Both refresh_daily_objectives_task and
refresh_weekly_objectives_task duplicate engine/session setup, vault iteration,
and error/retry handling; extract a shared helper (e.g., _run_objective_refresh)
that creates the async engine via create_async_engine, builds
async_sessionmaker, queries Vault.id, iterates vault_ids, constructs
ObjectiveAssignmentService(session) and invokes a passed-in coroutine method
(like service.refresh_daily_objectives or service.refresh_weekly_objectives) for
each vault, accumulates results, disposes the engine, and returns the summary;
then call that helper from refresh_daily_objectives_task and
refresh_weekly_objectives_task and preserve the same try/except that logs via
logger.exception and calls self.retry(exc=e, countdown=3600).
In `@backend/app/core/celery.py`:
- Around line 38-42: The current "refresh-weekly-objectives" entry uses an
interval seconds schedule (schedule: 604800.0) which resets on Beat restart;
replace it with a crontab-based schedule so it runs at a fixed wall-clock time
(e.g., crontab(minute=0, hour=0, day_of_week='monday') to run at Monday
midnight). Update the "refresh-weekly-objectives" task definition in celery.py
to import and use Celery's crontab instead of the numeric interval while
preserving existing "task" and "options" keys (adjust or remove "expires" if it
no longer applies).
In `@backend/app/crud/dweller.py`:
- Line 248: Remove the redundant inline import of GameEvent and event_bus inside
the function in backend/app/crud/dweller.py—there's already a module-level
import "from app.services.event_bus import GameEvent, event_bus" (near the top),
so delete the duplicate "from app.services.event_bus import GameEvent,
event_bus" at line 248 and use the existing GameEvent and event_bus references
directly.
- Around line 256-273: The current logic for picking the dweller's highest
SPECIAL uses dict insertion order so ties always pick "strength" and the same
code is duplicated in auto_assign_to_best_room; extract a single helper (e.g., a
static/class method named _get_highest_special(dweller_obj, preferred:
Optional[str]=None) used by both places) that builds the SPECIAL map once,
resolves ties deterministically (prefer a provided preferred stat like the
room's ability when present, otherwise pick by a stable rule such as
alphabetical order or explicit priority list), and replace the inline max(...)
usage in both the DWELLER_ASSIGNED_CORRECTLY check and auto_assign_to_best_room
with calls to this helper so tie behavior is predictable and duplication is
removed.
In `@backend/app/services/objective_assignment_service.py`:
- Around line 13-14: Remove the dead TYPE_CHECKING block and its unused import:
delete the "if TYPE_CHECKING: pass" block in objective_assignment_service.py and
remove TYPE_CHECKING from the typing import list, ensuring no other references
to TYPE_CHECKING remain in the file.
- Around line 74-80: The refresh methods perform clear and assign as separate
commits which can leave a vault empty if a crash occurs; update the clear and
assign methods (clear_daily_objectives, assign_daily_objectives,
clear_weekly_objectives, assign_weekly_objectives and their shared helpers
_clear_category_objectives/_assign_category_objectives) to accept an optional
auto_commit: bool = True flag, have their default behavior unchanged, and when
called from refresh_daily_objectives and refresh_weekly_objectives pass
auto_commit=False and perform a single commit (or explicit transaction commit)
after both operations complete so the clear+assign are atomic; alternatively
wrap the two calls inside a single transaction at the refresh_* level using the
same DB session/transaction.
- Around line 82-113: The method _assign_category_objectives currently calls
_objective_already_assigned for each Objective causing N+1 queries; instead run
a single query up-front to fetch all assigned objective IDs for the given vault
(and category if needed) and use that set to filter all_objectives into
available before sampling; update the logic that builds
VaultObjectiveProgressLink and adds them to the session the same way, then
commit once as before. Target symbols: _assign_category_objectives,
_objective_already_assigned (remove per-object awaits), Objective, and
VaultObjectiveProgressLink.
In `@backend/app/services/reward_service.py`:
- Around line 204-206: The list-comprehension post-filtering on dwellers is
redundant because dweller_crud.get_multi_by_vault already excludes soft-deleted
records; remove the redundant filtering expressions that reassign dwellers (the
“[d for d in dwellers if not d.is_deleted]” lines) in reward_service.py so the
code simply uses the returned dwellers from get_multi_by_vault (refer to the
dwellers variable and get_multi_by_vault call to locate the spots, including the
second occurrence later in the file).
In `@backend/app/tests/test_services/test_objective_evaluators.py`:
- Around line 631-740: Add the missing required category field to every
Objective instance in these tests (e.g., within TestExpeditionEvaluator and
TestLevelUpEvaluator) by setting category="achievement" on each Objective
constructor call (the Objective(...) usages inside test_matches_any_quest_type,
test_matches_specific_quest_type, test_does_not_match_wrong_quest_type,
test_matches_wildcard_quest_type, test_matches_min_level_met,
test_matches_exceeds_min_level, test_does_not_match_below_min_level, and
test_matches_no_min_level_requirement) so the model's required field is present
and tests remain valid under stricter persistence constraints.
In `@backend/app/tests/test_utils/test_seed_objectives.py`:
- Around line 195-196: Update the test's invalid_data test vector to also
include an objective missing the required "category" field so schema validation
is asserted for both missing reward and missing category; specifically, in the
test that builds invalid_data (variable name invalid_data in
test_seed_objectives.py) add an entry like {"challenge": "Missing category",
"reward": "50 caps"} before the valid objective so the seeding logic rejects
objects without a category as well.
- Around line 24-26: The test fixtures in test_seed_objectives.py only create
objectives with "category": "achievement", so add at least one test objective
with "category": "daily" and one with "category": "weekly" in the seeded test
data (e.g., extend the list of dicts that currently contains {"challenge":
"Collect 3 outfits", ...} entries) and update the related test assertions (or
parameterize the assertions) to verify those categories are persisted and
handled correctly; look for the seeded data structure in test_seed_objectives.py
and the assertions that inspect objective.category to add checks for "daily" and
"weekly".
In `@frontend/src/modules/progression/components/QuestCard.vue`:
- Around line 2-7: The import order is incorrect: move the relative import
'../models/quest' so it comes after the '@/...' alias imports (UCard, UBadge,
UButton, useQuestStore) to follow the Vue/core → third-party → `@/` → relative
rule; specifically reorder so computed, ref, onMounted, onUnmounted, watch and
Icon remain first, then the '@/...' imports (DwellerShort, UCard, UBadge,
UButton, useQuestStore), and then the relative types VaultQuest and
QuestPartyMember are imported last.
- Around line 142-179: The formatReward function signature is over the 100-char
line width; refactor its parameter typing by creating a dedicated interface
(e.g., interface Reward { reward_type: string; reward_data: Record<string,
unknown>; reward_chance: number }) and then change the signature to use that
type (e.g., const formatReward = (reward: Reward) => { ... }) or split the
parameter across multiple lines so the line length is under 100 chars; update
any imports/exports and keep the function body unchanged (refer to formatReward
and the reward parameter).
In `@frontend/src/modules/progression/models/objective.ts`:
- Around line 7-21: The ObjectiveType union on ObjectiveType is longer than 100
chars; split the union into multiple lines (one union member per line or grouped
logically) or refactor into separate type aliases so the ObjectiveType
declaration and its line(s) stay within the 100-character width guideline;
update the ObjectiveType type definition referenced in
frontend/src/modules/progression/models/objective.ts accordingly and ensure the
file still exports ObjectiveType for use by the Objective interface.
In `@frontend/src/modules/progression/views/ObjectivesView.vue`:
- Around line 26-31: Remove the now-unused activeObjectives computed
declaration: delete the line "const activeObjectives = computed(() =>
filterObjectives(false))" (and any direct references to activeObjectives),
leaving filterObjectives and completedObjectives as-is (or inline
filterObjectives into completedObjectives if you prefer). Ensure there are no
remaining template or script references to activeObjectives so there are no
unused-symbol warnings for activeObjectives or stale bindings on
objectivesStore.objectives.
…et policies - Add dweller-images, dweller-thumbnails, and other buckets to RUSTFS_PUBLIC_BUCKET_WHITELIST - Implement _ensure_bucket_policy() method in RustFSAdapter to set public read policies - Fix public URL generation for newly uploaded files - Fixes issue where dweller images returned 403 Forbidden
- Add fix_dweller_image_urls.py to convert filenames to full URLs in database - Add set_rustfs_bucket_policies.py to apply public policies to existing buckets
Backend fixes: - Alembic migration: fix typing imports, server_default case, add enum downgrade comment - celery_task.py: add error handling per vault, filter soft-deleted vaults - objective.py: make category column non-nullable - objective_evaluators.py: coerce min_level to int in LevelUpEvaluator - reward_service.py: fix rarity weights sum, use random.choice, remove redundant filtering - celery.py: change weekly schedule from interval to crontab - dweller.py: remove redundant event_bus import - objective_assignment_service.py: remove TYPE_CHECKING, make refresh atomic, fix N+1 query - test_objective_evaluators.py: add missing category field to test objectives Frontend fixes: - QuestCard.vue: fix import order - objective.ts: split long ObjectiveType union - ObjectivesView.vue: remove unused activeObjectives computed
for more information, see https://pre-commit.ci
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
backend/app/services/objective_evaluators.py (1)
407-408:new_levelis compared uncoerced against the defensively castmin_level.
min_levelis safely cast viaint()+ try/except, butnew_levelis used as-is from event data. If the event payload delivers a non-numeric value (e.g., a JSON-decoded string"5"), the>=comparison raises aTypeError. Consistent defensive coercion would eliminate the asymmetry.♻️ Proposed fix
new_level = data.get("new_level", data.get("level", 1)) - return new_level >= min_level + try: + new_level = int(new_level) + except (TypeError, ValueError): + return False + return new_level >= min_level🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/objective_evaluators.py` around lines 407 - 408, The comparison uses new_level from the event payload without coercion while min_level is defensively cast; change new_level handling so it mirrors min_level: fetch it via data.get("new_level", data.get("level", 1)), attempt to coerce to int inside a try/except (e.g., new_level = int(raw_new_level)), and on failure fall back to a safe default (such as 1 or data.get("level", 1)); then perform return new_level >= min_level. Ensure you reference the new_level and min_level variables and keep the same defaulting semantics.frontend/src/modules/progression/components/QuestCard.vue (1)
174-175:lunchboxdisplay text is hardcoded and ignoresreward_data.If the lunchbox composition ever changes on the backend, this label will silently become stale. Consider reading from
reward_data(e.g.data.item_count,data.dweller_count) with the current string as a fallback.♻️ Suggested improvement
case 'lunchbox': { - return 'Lunchbox (3 items + 1 dweller)' + const itemCount = Number(data.item_count) || 3 + const dwellerCount = Number(data.dweller_count) || 1 + return `Lunchbox (${itemCount} item${itemCount !== 1 ? 's' : ''} + ${dwellerCount} dweller${dwellerCount !== 1 ? 's' : ''})` + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/modules/progression/components/QuestCard.vue` around lines 174 - 175, The 'lunchbox' branch in QuestCard.vue currently returns a hardcoded string; update that case to construct the label from the quest's reward_data (use reward_data.item_count and reward_data.dweller_count) and fall back to the existing "Lunchbox (3 items + 1 dweller)" string when those fields are missing or invalid; locate the switch/case that returns 'Lunchbox (3 items + 1 dweller)' (the 'lunchbox' case) and replace the hardcoded return with logic that reads reward_data, validates numeric values, and formats the same display text as the fallback.backend/app/services/storage/rustfs_adapter.py (1)
85-87: Policy is applied only on first bucket creation; pre-existing buckets needset_rustfs_bucket_policies.py.If a bucket already exists when
upload_fileis first called (e.g., after a deployment to a new env where buckets were pre-created),_ensure_bucket_policywill never be invoked through this path. The standalone script is the intended remedy—worth a clarifying comment here so operators know to run it for existing buckets.💡 Suggested comment
# Set public policy for whitelisted buckets (only after creation or if needed) if bucket_created: self._ensure_bucket_policy(bucket_name) + # NOTE: Pre-existing buckets are not covered here. + # Run backend/scripts/set_rustfs_bucket_policies.py to backfill policies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/storage/rustfs_adapter.py` around lines 85 - 87, The current upload_file path only calls _ensure_bucket_policy when bucket_created is True, so pre-existing buckets won't get policies applied automatically; add a concise comment next to the bucket_created check (in the upload_file method) explaining that existing buckets must be handled by the separate operator script set_rustfs_bucket_policies.py and that operators should run that script when migrating or deploying into environments with pre-created buckets so policies are applied for those buckets.backend/scripts/set_rustfs_bucket_policies.py (3)
16-51:async defwith noawait— drop the async wrapper.
set_bucket_policiesuses the synchronousboto3client exclusively; there are noawaitexpressions. Declaring itasyncand wrapping it withasyncio.run()creates an event loop unnecessarily and is misleading to future contributors. Usingasync deffor functions that perform blocking operations—such as synchronous network requests—will block the entire event loop; only useasync deffunctions for non-blocking operations.♻️ Proposed fix
-async def set_bucket_policies(): +def set_bucket_policies(): """Set public read policies on all whitelisted buckets.""" ... if __name__ == "__main__": - asyncio.run(set_bucket_policies()) + set_bucket_policies()You can also remove the now-unused
import asyncio.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/scripts/set_rustfs_bucket_policies.py` around lines 16 - 51, The function set_bucket_policies is declared async but uses the synchronous boto3 client (boto3.client) and has no await usage; change its signature from async def set_bucket_policies() to a regular def set_bucket_policies(), remove the asyncio.run(...) call at the bottom and invoke set_bucket_policies() directly, and delete the now-unused asyncio import; keep the existing try/except and boto3 usage unchanged.
8-8: Replaceos.pathhelpers withpathlib.Path(Ruff PTH118/PTH120).♻️ Proposed fix
-import os -import sys +import sys +from pathlib import Path -sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) +sys.path.insert(0, str(Path(__file__).parent.parent))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/scripts/set_rustfs_bucket_policies.py` at line 8, Replace the os.path usage that modifies sys.path with pathlib.Path: change sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) to use Path(__file__).resolve().parent.parent (converted to str) and add from pathlib import Path to imports; update any other os.path.join/dirname calls in this file similarly or remove unused os imports. For example, use str(Path(__file__).resolve().parent.parent) when inserting into sys.path and replace other os.path helpers with Path methods like parent / "subdir".
46-47: Narrow theexceptclause (Ruff BLE001).Catching bare
Exceptionsilently swallows unexpected errors (e.g., auth failures, network timeouts). Usebotocore.exceptions.ClientErrorto let real surprises surface.♻️ Proposed fix
- except Exception as e: + except ClientError as e: print(f"❌ Failed to set policy for {bucket}: {e}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/scripts/set_rustfs_bucket_policies.py` around lines 46 - 47, The except block in set_rustfs_bucket_policies.py currently catches a bare Exception; change it to catch botocore.exceptions.ClientError instead (import ClientError from botocore.exceptions) so only AWS client errors are handled in the block that prints "❌ Failed to set policy for {bucket}: {e}"; let other unexpected exceptions propagate (or re-raise them) so they aren't silently swallowed—update the except clause and imports accordingly, and if you want to keep a fallback log, catch Exception as e2 only to log then re-raise.backend/app/services/reward_service.py (1)
295-303: Move theStoragelookup outside the item-generation loop.
select(Storage).where(Storage.vault_id == vault_id)is executed on every iteration offor _ in range(3), issuing three identical DB queries. The result never changes within the loop.♻️ Proposed fix
+ # Get storage once, before the item loop + result = await db_session.execute(select(Storage).where(Storage.vault_id == vault_id)) + storage = result.scalar_one_or_none() + granted_items = [] for _ in range(3): name, wtype, subtype, stat = random.choice(item_configs) ... - # Get storage - result = await db_session.execute(select(Storage).where(Storage.vault_id == vault_id)) - storage = result.scalar_one_or_none() if storage:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/reward_service.py` around lines 295 - 303, The Storage lookup is being performed inside the item-generation loop; move the select(Storage).where(Storage.vault_id == vault_id) query out of the loop so it runs once and its result is reused; assign storage = result.scalar_one_or_none() before the for _ in range(3) loop, then inside the loop only check if storage and set item.storage_id = storage.id, add item to db_session, and append to granted_items (preserving the existing type/rarity logic) so you avoid issuing identical DB queries each iteration.backend/scripts/fix_dweller_image_urls.py (1)
12-12: Sameos.path→pathlib.Pathrefactor as inset_rustfs_bucket_policies.py(Ruff PTH118/PTH120).♻️ Proposed fix
-import os -import sys +import sys +from pathlib import Path -sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) +sys.path.insert(0, str(Path(__file__).parent.parent))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/scripts/fix_dweller_image_urls.py` at line 12, Replace the os.path usage in the sys.path.insert call with pathlib.Path equivalents: instead of os.path.join(os.path.dirname(__file__), "..") compute the parent directory via Path(__file__).resolve().parent.parent and insert its string into sys.path (i.e., update the sys.path.insert(...) that currently calls sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) to use Path to avoid PTH118/PTH120 warnings).backend/app/api/celery_task.py (1)
194-291: Extract a shared helper to eliminate copy-paste betweenrefresh_daily_objectives_taskandrefresh_weekly_objectives_task.Both tasks are structurally identical (engine init, vault ID query, per-vault loop, retry). Only the task name, log label, and service method name differ. Introducing a small shared async helper removes the duplication and means the rollback fix (and any future changes) only need to be made once.
♻️ Suggested refactor
async def _run_objectives_refresh(refresh_method_name: str, label: str) -> dict: from sqlalchemy import select from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine from app.core.config import settings from app.models.vault import Vault from app.services.objective_assignment_service import ObjectiveAssignmentService engine = create_async_engine(str(settings.ASYNC_DATABASE_URI), echo=False, future=True, pool_pre_ping=True) session_maker = async_sessionmaker(engine, expire_on_commit=False) try: async with session_maker() as session: result = await session.execute(select(Vault.id).where(Vault.deleted_at.is_(None))) vault_ids = [row[0] for row in result.all()] total_assigned = 0 for vault_id in vault_ids: try: service = ObjectiveAssignmentService(session) assigned = await getattr(service, refresh_method_name)(vault_id) total_assigned += len(assigned) except Exception: logger.exception(f"Failed to refresh {label} objectives for vault {vault_id}") await session.rollback() continue return {"vaults_processed": len(vault_ids), "objectives_assigned": total_assigned} finally: await engine.dispose() `@celery_app.task`(name="refresh_daily_objectives", bind=True) def refresh_daily_objectives_task(self): """Refresh daily objectives for all vaults. Scheduled to run daily via Celery Beat.""" try: logger.info("Starting daily objectives refresh") result = asyncio.run(_run_objectives_refresh("refresh_daily_objectives", "daily")) except Exception as e: logger.exception("Daily objectives refresh failed") raise self.retry(exc=e, countdown=3600) from e else: logger.info(f"Daily objectives refresh completed: {result}") return result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/celery_task.py` around lines 194 - 291, Both refresh_daily_objectives_task and refresh_weekly_objectives_task duplicate the same async engine/session/vault-loop logic; extract a shared async helper (e.g. _run_objectives_refresh(refresh_method_name: str, label: str) -> dict) that initializes create_async_engine/async_sessionmaker, queries Vault.id, iterates vault_ids, instantiates ObjectiveAssignmentService(session) and calls getattr(service, refresh_method_name)(vault_id), accumulates totals, logs per-vault exceptions (using logger.exception with the label) and performs await session.rollback() on per-vault failures, ensures await engine.dispose() in finally, then have each task call asyncio.run(_run_objectives_refresh("refresh_daily_objectives","daily")) or the weekly equivalent and keep the existing retry/logging in refresh_daily_objectives_task and refresh_weekly_objectives_task.backend/app/services/objective_assignment_service.py (2)
70-86:refresh_*operations are silent — add a post-commit log.With
auto_commit=False, neither_clear_category_objectivesnor_assign_category_objectiveslogs. The finalcommit()on line 76/85 is silent, making it hard to trace refresh activity per vault.♻️ Proposed fix
async def refresh_daily_objectives(self, vault_id: UUID4) -> list[Objective]: # Atomic clear + assign: do not commit between operations - await self._clear_category_objectives(vault_id, ObjectiveCategoryEnum.DAILY, auto_commit=False) + cleared = await self._clear_category_objectives(vault_id, ObjectiveCategoryEnum.DAILY, auto_commit=False) assigned = await self._assign_category_objectives( vault_id, ObjectiveCategoryEnum.DAILY, self.DAILY_COUNT, auto_commit=False ) await self._db_session.commit() + logger.info(f"Refreshed daily objectives for vault {vault_id}: cleared={cleared}, assigned={len(assigned)}") return assigned🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/objective_assignment_service.py` around lines 70 - 86, The refresh_daily_objectives and refresh_weekly_objectives functions perform clear+assign with auto_commit=False and then call await self._db_session.commit() but emit no log, making refreshes opaque; after the commit in both refresh_daily_objectives and refresh_weekly_objectives add an info-level post-commit log (e.g., via self._logger.info or existing logger) that records the vault_id, the category (ObjectiveCategoryEnum.DAILY / WEEKLY), and the number of objectives assigned (len(assigned)) and/or cleared so the operation is traceable; ensure the log runs after await self._db_session.commit() and mirror the same logging pattern in both methods.
128-147: Consider a bulkDELETEinstead of per-row ORM deletes.The loop at lines 140–141 issues one
DELETEper link row. A single bulk-delete statement would achieve the same result in one round-trip.♻️ Proposed refactor
- result = await self._db_session.execute(query) - links = result.scalars().all() - - for link in links: - await self._db_session.delete(link) - - if links and auto_commit: - await self._db_session.commit() - logger.info(f"Cleared {len(links)} {category} objectives for vault {vault_id}") - - return len(links) + from sqlalchemy import delete, func + + # Count first so we can return and log the actual number deleted + count_result = await self._db_session.execute( + select(func.count()).select_from(VaultObjectiveProgressLink) + .where(VaultObjectiveProgressLink.vault_id == vault_id) + .where(VaultObjectiveProgressLink.objective_id.in_(subquery)) + ) + count = count_result.scalar() or 0 + + await self._db_session.execute( + delete(VaultObjectiveProgressLink) + .where(VaultObjectiveProgressLink.vault_id == vault_id) + .where(VaultObjectiveProgressLink.objective_id.in_(subquery)) + ) + + if count and auto_commit: + await self._db_session.commit() + logger.info(f"Cleared {count} {category} objectives for vault {vault_id}") + + return count🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/objective_assignment_service.py` around lines 128 - 147, The loop in _clear_category_objectives issues one ORM delete per VaultObjectiveProgressLink row; replace it with a single bulk DELETE: build a delete statement targeting VaultObjectiveProgressLink rows where vault_id == vault_id and objective_id in the Objective subquery, execute it via self._db_session.execute(delete_stmt), commit if auto_commit, and use the execute() result.rowcount (or a prior COUNT query if your DB driver doesn't populate rowcount) to determine the number removed for the logger.info message and the return value; keep references to _clear_category_objectives, VaultObjectiveProgressLink, and Objective so the change is local and obvious.
🤖 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/app/alembic/versions/2026_02_18_2104-5934c2bdf3e1_add_item_table_objective_category_and_.py`:
- Around line 44-46: The migration adds uppercase enum members to the PostgreSQL
type rewardtype while the Python enum RewardType (used in reward_service.py)
defines lowercase strings; update the Python RewardType definition to use the
matching UPPERCASE values (e.g., "CAPS", "ITEM", "STIMPAK", "RADAWAY",
"LUNCHBOX") and audit usages of RewardType (including reward_service.py and any
tests/serializers) to ensure comparisons/inserts use the updated enum names so
they match the DB enum; alternatively, if you prefer changing the DB, modify the
op.execute calls that alter TYPE rewardtype to use lowercase values to match
RewardType, but do one consistent change across both the migration
(op.execute("ALTER TYPE rewardtype ...")) and the RewardType declaration.
In `@backend/app/api/celery_task.py`:
- Around line 222-229: The per-vault except blocks around
ObjectiveAssignmentService.refresh_daily_objectives are missing an explicit
rollback, leaving the async SQLAlchemy session in a requires-rollback state and
causing subsequent iterations to fail; after catching the exception (currently
logging via logger.exception in the loop that calls
service.refresh_daily_objectives for each vault_id) add an await
session.rollback() immediately after the logger.exception call and before
continue to restore session usability, and apply the identical change to the
refresh_weekly_objectives_task loop that handles refresh_weekly_objectives so
each per-vault failure cleans up the session before the next iteration.
In `@backend/app/services/objective_assignment_service.py`:
- Around line 33-45: The loop in assign_achievement_objectives currently calls
_objective_already_assigned for every objective causing an N+1 query; change it
to mirror _assign_category_objectives by fetching all existing assigned
objective IDs for the vault once (e.g., query VaultObjectiveProgressLink where
vault_id == vault_id and collect objective_id into a set), then iterate
all_achievements and only create/add a VaultObjectiveProgressLink for objectives
whose id is not in that set; keep usage of VaultObjectiveProgressLink,
self._db_session.add(link), and the assigned.append(objective) logic but remove
per-object existence calls to _objective_already_assigned.
In `@backend/app/services/objective_evaluators.py`:
- Around line 381-382: The code calling .lower() on event_quest_type can raise
AttributeError when data["quest_type"] is present but null; change the
expression in the function handling this logic (the variable event_quest_type
and the data.get("quest_type", "") call) to safely coerce None to an empty
string before lowercasing (for example use (event_quest_type or "").lower() or
explicitly str(event_quest_type or "").lower()), then compare that to
target_quest_type.lower() to avoid errors when quest_type is null.
In `@frontend/src/modules/progression/components/QuestCard.vue`:
- Line 143: The function signature for formatReward is over the 100-char limit
because it uses an inline parameter type; extract that inline type into a named
type or interface (e.g., type Reward = { reward_type: string; reward_data:
Record<string, unknown>; reward_chance: number } or interface Reward) and update
the function to use formatReward(reward: Reward) to shorten the line while
preserving types; modify any other references to the same shape to use the new
Reward type if applicable.
---
Nitpick comments:
In `@backend/app/api/celery_task.py`:
- Around line 194-291: Both refresh_daily_objectives_task and
refresh_weekly_objectives_task duplicate the same async
engine/session/vault-loop logic; extract a shared async helper (e.g.
_run_objectives_refresh(refresh_method_name: str, label: str) -> dict) that
initializes create_async_engine/async_sessionmaker, queries Vault.id, iterates
vault_ids, instantiates ObjectiveAssignmentService(session) and calls
getattr(service, refresh_method_name)(vault_id), accumulates totals, logs
per-vault exceptions (using logger.exception with the label) and performs await
session.rollback() on per-vault failures, ensures await engine.dispose() in
finally, then have each task call
asyncio.run(_run_objectives_refresh("refresh_daily_objectives","daily")) or the
weekly equivalent and keep the existing retry/logging in
refresh_daily_objectives_task and refresh_weekly_objectives_task.
In `@backend/app/services/objective_assignment_service.py`:
- Around line 70-86: The refresh_daily_objectives and refresh_weekly_objectives
functions perform clear+assign with auto_commit=False and then call await
self._db_session.commit() but emit no log, making refreshes opaque; after the
commit in both refresh_daily_objectives and refresh_weekly_objectives add an
info-level post-commit log (e.g., via self._logger.info or existing logger) that
records the vault_id, the category (ObjectiveCategoryEnum.DAILY / WEEKLY), and
the number of objectives assigned (len(assigned)) and/or cleared so the
operation is traceable; ensure the log runs after await
self._db_session.commit() and mirror the same logging pattern in both methods.
- Around line 128-147: The loop in _clear_category_objectives issues one ORM
delete per VaultObjectiveProgressLink row; replace it with a single bulk DELETE:
build a delete statement targeting VaultObjectiveProgressLink rows where
vault_id == vault_id and objective_id in the Objective subquery, execute it via
self._db_session.execute(delete_stmt), commit if auto_commit, and use the
execute() result.rowcount (or a prior COUNT query if your DB driver doesn't
populate rowcount) to determine the number removed for the logger.info message
and the return value; keep references to _clear_category_objectives,
VaultObjectiveProgressLink, and Objective so the change is local and obvious.
In `@backend/app/services/objective_evaluators.py`:
- Around line 407-408: The comparison uses new_level from the event payload
without coercion while min_level is defensively cast; change new_level handling
so it mirrors min_level: fetch it via data.get("new_level", data.get("level",
1)), attempt to coerce to int inside a try/except (e.g., new_level =
int(raw_new_level)), and on failure fall back to a safe default (such as 1 or
data.get("level", 1)); then perform return new_level >= min_level. Ensure you
reference the new_level and min_level variables and keep the same defaulting
semantics.
In `@backend/app/services/reward_service.py`:
- Around line 295-303: The Storage lookup is being performed inside the
item-generation loop; move the select(Storage).where(Storage.vault_id ==
vault_id) query out of the loop so it runs once and its result is reused; assign
storage = result.scalar_one_or_none() before the for _ in range(3) loop, then
inside the loop only check if storage and set item.storage_id = storage.id, add
item to db_session, and append to granted_items (preserving the existing
type/rarity logic) so you avoid issuing identical DB queries each iteration.
In `@backend/app/services/storage/rustfs_adapter.py`:
- Around line 85-87: The current upload_file path only calls
_ensure_bucket_policy when bucket_created is True, so pre-existing buckets won't
get policies applied automatically; add a concise comment next to the
bucket_created check (in the upload_file method) explaining that existing
buckets must be handled by the separate operator script
set_rustfs_bucket_policies.py and that operators should run that script when
migrating or deploying into environments with pre-created buckets so policies
are applied for those buckets.
In `@backend/scripts/fix_dweller_image_urls.py`:
- Line 12: Replace the os.path usage in the sys.path.insert call with
pathlib.Path equivalents: instead of os.path.join(os.path.dirname(__file__),
"..") compute the parent directory via Path(__file__).resolve().parent.parent
and insert its string into sys.path (i.e., update the sys.path.insert(...) that
currently calls sys.path.insert(0, os.path.join(os.path.dirname(__file__),
"..")) to use Path to avoid PTH118/PTH120 warnings).
In `@backend/scripts/set_rustfs_bucket_policies.py`:
- Around line 16-51: The function set_bucket_policies is declared async but uses
the synchronous boto3 client (boto3.client) and has no await usage; change its
signature from async def set_bucket_policies() to a regular def
set_bucket_policies(), remove the asyncio.run(...) call at the bottom and invoke
set_bucket_policies() directly, and delete the now-unused asyncio import; keep
the existing try/except and boto3 usage unchanged.
- Line 8: Replace the os.path usage that modifies sys.path with pathlib.Path:
change sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) to use
Path(__file__).resolve().parent.parent (converted to str) and add from pathlib
import Path to imports; update any other os.path.join/dirname calls in this file
similarly or remove unused os imports. For example, use
str(Path(__file__).resolve().parent.parent) when inserting into sys.path and
replace other os.path helpers with Path methods like parent / "subdir".
- Around line 46-47: The except block in set_rustfs_bucket_policies.py currently
catches a bare Exception; change it to catch botocore.exceptions.ClientError
instead (import ClientError from botocore.exceptions) so only AWS client errors
are handled in the block that prints "❌ Failed to set policy for {bucket}: {e}";
let other unexpected exceptions propagate (or re-raise them) so they aren't
silently swallowed—update the except clause and imports accordingly, and if you
want to keep a fallback log, catch Exception as e2 only to log then re-raise.
In `@frontend/src/modules/progression/components/QuestCard.vue`:
- Around line 174-175: The 'lunchbox' branch in QuestCard.vue currently returns
a hardcoded string; update that case to construct the label from the quest's
reward_data (use reward_data.item_count and reward_data.dweller_count) and fall
back to the existing "Lunchbox (3 items + 1 dweller)" string when those fields
are missing or invalid; locate the switch/case that returns 'Lunchbox (3 items +
1 dweller)' (the 'lunchbox' case) and replace the hardcoded return with logic
that reads reward_data, validates numeric values, and formats the same display
text as the fallback.
Comment on lines
+44
to
+46
| op.execute("ALTER TYPE rewardtype ADD VALUE IF NOT EXISTS 'STIMPAK'") | ||
| op.execute("ALTER TYPE rewardtype ADD VALUE IF NOT EXISTS 'RADAWAY'") | ||
| op.execute("ALTER TYPE rewardtype ADD VALUE IF NOT EXISTS 'LUNCHBOX'") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where rewardtype and rarityenum enums were originally created
# to determine whether their values are uppercase or lowercase.
echo "=== Searching for rewardtype enum creation in migrations ==="
rg -n "rewardtype" backend/app/alembic/versions/ -B2 -A5 --type=py
echo ""
echo "=== Checking RewardType enum definition ==="
rg -n "class RewardType" backend/ -A20 --type=py
echo ""
echo "=== Checking RarityEnum definition ==="
rg -n "class RarityEnum" backend/ -A10 --type=py
echo ""
echo "=== Checking reward_service.py for value usage ==="
rg -n "rarity\.value|reward_service" backend/ --type=py -A3 -B3 | head -50Repository: ElderEvil/falloutProject
Length of output: 11401
Case mismatch between PostgreSQL ENUM values and Python enum values requires correction.
The PostgreSQL rewardtype enum was originally created with uppercase values ("CAPS", "ITEM", "DWELLER", "RESOURCE", "EXPERIENCE"), and the new migration adds values in uppercase ('STIMPAK', 'RADAWAY', 'LUNCHBOX'). However, the Python RewardType enum defines all these values in lowercase ("caps", "item", "stimpak", etc.).
When reward_service.py or any code path inserts or compares RewardType values against the database, the constraint will fail because PostgreSQL expects uppercase while Python provides lowercase. Unlike RarityEnum, which uses CaseInsensitiveEnum, the RewardType enum is a standard StrEnum with no case-insensitive handling.
Either the PostgreSQL ENUM definition must be altered to use lowercase values, or the Python enum values must be changed to uppercase to match the database constraint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/app/alembic/versions/2026_02_18_2104-5934c2bdf3e1_add_item_table_objective_category_and_.py`
around lines 44 - 46, The migration adds uppercase enum members to the
PostgreSQL type rewardtype while the Python enum RewardType (used in
reward_service.py) defines lowercase strings; update the Python RewardType
definition to use the matching UPPERCASE values (e.g., "CAPS", "ITEM",
"STIMPAK", "RADAWAY", "LUNCHBOX") and audit usages of RewardType (including
reward_service.py and any tests/serializers) to ensure comparisons/inserts use
the updated enum names so they match the DB enum; alternatively, if you prefer
changing the DB, modify the op.execute calls that alter TYPE rewardtype to use
lowercase values to match RewardType, but do one consistent change across both
the migration (op.execute("ALTER TYPE rewardtype ...")) and the RewardType
declaration.
Comment on lines
+222
to
+229
| for vault_id in vault_ids: | ||
| try: | ||
| service = ObjectiveAssignmentService(session) | ||
| assigned = await service.refresh_daily_objectives(vault_id) | ||
| total_assigned += len(assigned) | ||
| except Exception: | ||
| logger.exception(f"Failed to refresh daily objectives for vault {vault_id}") | ||
| continue |
Contributor
There was a problem hiding this comment.
Missing await session.rollback() after per-vault exception — isolation guarantee is broken.
When refresh_daily_objectives raises (e.g., the internal commit() fails, or an autoflush error occurs), SQLAlchemy's async session enters a requires-rollback state. Without an explicit rollback, the very next session.execute() call in the next vault's iteration raises sqlalchemy.exc.PendingRollbackError, which is immediately swallowed by the same except block. The net result: every vault after the first failure silently errors, and total_assigned counts none of them — defeating the entire purpose of the per-vault try/except.
The same defect exists in refresh_weekly_objectives_task at lines 273–279.
🐛 Proposed fix — rollback the session after a per-vault failure
for vault_id in vault_ids:
try:
service = ObjectiveAssignmentService(session)
assigned = await service.refresh_daily_objectives(vault_id)
total_assigned += len(assigned)
except Exception:
logger.exception(f"Failed to refresh daily objectives for vault {vault_id}")
+ await session.rollback()
continueApply the same fix to the refresh_weekly_objectives_task loop (lines 273–279).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/celery_task.py` around lines 222 - 229, The per-vault except
blocks around ObjectiveAssignmentService.refresh_daily_objectives are missing an
explicit rollback, leaving the async SQLAlchemy session in a requires-rollback
state and causing subsequent iterations to fail; after catching the exception
(currently logging via logger.exception in the loop that calls
service.refresh_daily_objectives for each vault_id) add an await
session.rollback() immediately after the logger.exception call and before
continue to restore session usability, and apply the identical change to the
refresh_weekly_objectives_task loop that handles refresh_weekly_objectives so
each per-vault failure cleans up the session before the next iteration.
Comment on lines
+33
to
+45
| assigned = [] | ||
| for objective in all_achievements: | ||
| exists = await self._objective_already_assigned(vault_id, objective.id) | ||
| if not exists: | ||
| link = VaultObjectiveProgressLink( | ||
| vault_id=vault_id, | ||
| objective_id=objective.id, | ||
| progress=0, | ||
| total=objective.target_amount or 1, | ||
| is_completed=False, | ||
| ) | ||
| self._db_session.add(link) | ||
| assigned.append(objective) |
Contributor
There was a problem hiding this comment.
N+1 query pattern: _objective_already_assigned is called per achievement in a loop.
_assign_category_objectives already demonstrates the correct approach — fetch all assigned IDs for the vault in a single query (lines 100–104) then filter in Python. assign_achievement_objectives skips that optimization and issues one SELECT per achievement. This is especially costly when this method is invoked from a task that iterates all vaults.
♻️ Proposed fix — batch the existence check
async def assign_achievement_objectives(self, vault_id: UUID4) -> list[Objective]:
query = select(Objective).where(Objective.category == ObjectiveCategoryEnum.ACHIEVEMENT)
result = await self._db_session.execute(query)
all_achievements = list(result.scalars().all())
+ if not all_achievements:
+ logger.warning(f"No {ObjectiveCategoryEnum.ACHIEVEMENT} objectives found in database")
+ return []
+
+ assigned_ids_query = select(VaultObjectiveProgressLink.objective_id).where(
+ VaultObjectiveProgressLink.vault_id == vault_id
+ )
+ assigned_result = await self._db_session.execute(assigned_ids_query)
+ assigned_ids = {row[0] for row in assigned_result.all()}
+
assigned = []
- for objective in all_achievements:
- exists = await self._objective_already_assigned(vault_id, objective.id)
- if not exists:
+ for objective in all_achievements:
+ if objective.id not in assigned_ids:
link = VaultObjectiveProgressLink(
vault_id=vault_id,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/objective_assignment_service.py` around lines 33 - 45,
The loop in assign_achievement_objectives currently calls
_objective_already_assigned for every objective causing an N+1 query; change it
to mirror _assign_category_objectives by fetching all existing assigned
objective IDs for the vault once (e.g., query VaultObjectiveProgressLink where
vault_id == vault_id and collect objective_id into a set), then iterate
all_achievements and only create/add a VaultObjectiveProgressLink for objectives
whose id is not in that set; keep usage of VaultObjectiveProgressLink,
self._db_session.add(link), and the assigned.append(objective) logic but remove
per-object existence calls to _objective_already_assigned.
Comment on lines
+381
to
+382
| event_quest_type = data.get("quest_type", "") | ||
| return event_quest_type.lower() == target_quest_type.lower() |
Contributor
There was a problem hiding this comment.
None.lower() will raise AttributeError if event data has quest_type: null.
data.get("quest_type", "") returns the value stored under the key, which may be None even though a default of "" is provided (the default only applies when the key is absent). Calling .lower() on None raises AttributeError.
🛡️ Proposed fix
- event_quest_type = data.get("quest_type", "")
- return event_quest_type.lower() == target_quest_type.lower()
+ event_quest_type = data.get("quest_type") or ""
+ return event_quest_type.lower() == target_quest_type.lower()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/objective_evaluators.py` around lines 381 - 382, The
code calling .lower() on event_quest_type can raise AttributeError when
data["quest_type"] is present but null; change the expression in the function
handling this logic (the variable event_quest_type and the
data.get("quest_type", "") call) to safely coerce None to an empty string before
lowercasing (for example use (event_quest_type or "").lower() or explicitly
str(event_quest_type or "").lower()), then compare that to
target_quest_type.lower() to avoid errors when quest_type is null.
| }) | ||
|
|
||
| // Format reward details for display | ||
| const formatReward = (reward: { reward_type: string; reward_data: Record<string, unknown>; reward_chance: number }) => { |
Contributor
There was a problem hiding this comment.
Line 143 exceeds the 100-character line limit.
The inline parameter type makes the signature 122 characters long. Extract it to a named type or interface.
♻️ Proposed fix
+interface RewardParam {
+ reward_type: string
+ reward_data: Record<string, unknown>
+ reward_chance: number
+}
+
-const formatReward = (reward: { reward_type: string; reward_data: Record<string, unknown>; reward_chance: number }) => {
+const formatReward = (reward: RewardParam) => {As per coding guidelines, "Use 100 character line width for frontend code (enforced by oxlint.json)".
📝 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.
Suggested change
| const formatReward = (reward: { reward_type: string; reward_data: Record<string, unknown>; reward_chance: number }) => { | |
| interface RewardParam { | |
| reward_type: string | |
| reward_data: Record<string, unknown> | |
| reward_chance: number | |
| } | |
| const formatReward = (reward: RewardParam) => { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/modules/progression/components/QuestCard.vue` at line 143, The
function signature for formatReward is over the 100-char limit because it uses
an inline parameter type; extract that inline type into a named type or
interface (e.g., type Reward = { reward_type: string; reward_data:
Record<string, unknown>; reward_chance: number } or interface Reward) and update
the function to use formatReward(reward: Reward) to shorten the line while
preserving types; modify any other references to the same shape to use the new
Reward type if applicable.
- Add category='achievement' to all Objective model instantiations in test_objective_evaluators.py - Add category='achievement' to ObjectiveCreate calls in test_reward_service.py - Fixes NOT NULL constraint failures after making category field non-nullable
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
backend/app/tests/test_services/test_reward_service.py (1)
66-68: UseObjectiveCategoryEnum.ACHIEVEMENTinstead of the raw string literal for consistency.Both
ObjectiveCreatecalls passcategory="achievement"as a raw string. The AI summary fortest_crud/test_objective.py(also in this PR) explicitly usesObjectiveCategoryEnum.ACHIEVEMENT. While Pydantic'sCaseInsensitiveEnumwill coerce the string at runtime, the string literal bypasses static type checking and will silently survive any future enum value renames.♻️ Proposed fix
+from app.schemas.common import ObjectiveCategoryEnum objective = await objective_crud.create( - async_session, ObjectiveCreate(challenge="Test", reward="100 caps", category="achievement") + async_session, ObjectiveCreate(challenge="Test", reward="100 caps", category=ObjectiveCategoryEnum.ACHIEVEMENT) )Apply the same change on Line 99.
Also applies to: 98-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_services/test_reward_service.py` around lines 66 - 68, Replace the raw string "achievement" used as the category in the ObjectiveCreate calls with the enum constant ObjectiveCategoryEnum.ACHIEVEMENT so the tests use the enum type rather than a string; locate the ObjectiveCreate invocation(s) passed into objective_crud.create in test_reward_service.py (and the similar call around lines 98–100) and change the category argument to ObjectiveCategoryEnum.ACHIEVEMENT to ensure static type safety and consistency with the other tests.backend/app/tests/test_services/test_objective_evaluators.py (3)
70-70: UseObjectiveCategoryEnum.ACHIEVEMENTfor thecategoryfield across allObjectivemodel instantiations.All
Objective(...)constructions throughout this file passcategory="achievement"as a raw string literal. The model'scategoryfield is typed asObjectiveCategoryEnum, so a bare string is a type mismatch that static analyzers (mypy/pyright) will flag. Use the enum constant for consistency and type safety. This applies to every changed line withcategory="achievement"(Lines 70, 113, 156, 201, 252, 303, 352, 396, 439, 482, 525, 561, 576, 591, 606, 621, 636, 651, and all instances insideTestExpeditionEvaluator/TestLevelUpEvaluator).♻️ Proposed fix (representative diff)
+from app.schemas.common import ObjectiveCategoryEnum objective = Objective( challenge="Collect 100 caps", reward="50 caps", objective_type="collect", target_entity={"resource_type": "caps"}, target_amount=100, - category="achievement", + category=ObjectiveCategoryEnum.ACHIEVEMENT, )Apply the same substitution to every
category="achievement"occurrence in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_services/test_objective_evaluators.py` at line 70, Replace all raw string literals category="achievement" used when constructing Objective(...) with the enum constant ObjectiveCategoryEnum.ACHIEVEMENT to satisfy the Objective.category type; update every Objective(...) instantiation in the test module (including within TestExpeditionEvaluator and TestLevelUpEvaluator and other test cases) to pass ObjectiveCategoryEnum.ACHIEVEMENT instead of "achievement" so type checkers (mypy/pyright) accept the assignments.
657-714:TestExpeditionEvaluatoronly covers_matches()— the event-to-DB-progress path is untested.Every other evaluator (Collect, Build, Train, Assign, Reach) has at least one async integration test that emits an event via
fresh_event_busand asserts DB progress updates.ExpeditionEvaluatorhas none: if the event subscription or progress-increment logic is broken, no test in this file catches it.Consider adding an async integration test analogous to
test_collect_evaluator_resource_collected:`@pytest.mark.asyncio` async def test_expedition_evaluator_quest_completed( async_session: AsyncSession, fresh_event_bus, patched_session_maker, ) -> None: """Test ExpeditionEvaluator updates progress on quest completed.""" # ... create user, vault, objective(objective_type="expedition"), link ... ExpeditionEvaluator(fresh_event_bus) await fresh_event_bus.emit(GameEvent.QUEST_COMPLETED, vault.id, {"quest_type": "main"}) await async_session.refresh(link) assert link.progress == 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_services/test_objective_evaluators.py` around lines 657 - 714, Add an async integration test that verifies the end-to-end event-to-DB progress flow for ExpeditionEvaluator: create a user and vault, create an Objective with objective_type="expedition" and link it to the user's vault (same pattern as test_collect_evaluator_resource_collected), instantiate ExpeditionEvaluator(fresh_event_bus), emit the event await fresh_event_bus.emit(GameEvent.QUEST_COMPLETED, vault.id, {"quest_type": "main"}) using the provided fresh_event_bus, refresh the link via await async_session.refresh(link) and assert link.progress == 1; use fixtures async_session, fresh_event_bus, and patched_session_maker to match other evaluator tests and ensure the test is decorated with `@pytest.mark.asyncio`.
717-774:TestLevelUpEvaluatoronly covers_matches()— add an async integration test for the event-to-DB path.Same gap as
TestExpeditionEvaluator: no test exercisesLevelUpEvaluatorend-to-end (event emission → progress update in DB). Add a test parallel totest_train_evaluator_dweller_trained:`@pytest.mark.asyncio` async def test_level_up_evaluator_dweller_leveled_up( async_session: AsyncSession, fresh_event_bus, patched_session_maker, ) -> None: """Test LevelUpEvaluator updates progress on dweller level-up.""" # ... create user, vault, objective(objective_type="level_up", target_entity={"min_level": 5}), link ... LevelUpEvaluator(fresh_event_bus) await fresh_event_bus.emit(GameEvent.DWELLER_LEVEL_UP, vault.id, {"new_level": 5}) await async_session.refresh(link) assert link.progress == 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_services/test_objective_evaluators.py` around lines 717 - 774, Add an async integration test named test_level_up_evaluator_dweller_leveled_up that mirrors test_train_evaluator_dweller_trained: set up async_session, fresh_event_bus, and patched_session_maker, create a user, vault, an Objective with objective_type="level_up" and target_entity={"min_level": 5}, create and persist the ObjectiveLink for that vault/user, instantiate LevelUpEvaluator(fresh_event_bus), emit GameEvent.DWELLER_LEVEL_UP via fresh_event_bus with the vault.id and data {"new_level": 5}, refresh the link from async_session and assert link.progress == 1 to verify the event-to-DB path.
🤖 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/app/tests/test_services/test_objective_evaluators.py`:
- Line 70: Replace all raw string literals category="achievement" used when
constructing Objective(...) with the enum constant
ObjectiveCategoryEnum.ACHIEVEMENT to satisfy the Objective.category type; update
every Objective(...) instantiation in the test module (including within
TestExpeditionEvaluator and TestLevelUpEvaluator and other test cases) to pass
ObjectiveCategoryEnum.ACHIEVEMENT instead of "achievement" so type checkers
(mypy/pyright) accept the assignments.
- Around line 657-714: Add an async integration test that verifies the
end-to-end event-to-DB progress flow for ExpeditionEvaluator: create a user and
vault, create an Objective with objective_type="expedition" and link it to the
user's vault (same pattern as test_collect_evaluator_resource_collected),
instantiate ExpeditionEvaluator(fresh_event_bus), emit the event await
fresh_event_bus.emit(GameEvent.QUEST_COMPLETED, vault.id, {"quest_type":
"main"}) using the provided fresh_event_bus, refresh the link via await
async_session.refresh(link) and assert link.progress == 1; use fixtures
async_session, fresh_event_bus, and patched_session_maker to match other
evaluator tests and ensure the test is decorated with `@pytest.mark.asyncio`.
- Around line 717-774: Add an async integration test named
test_level_up_evaluator_dweller_leveled_up that mirrors
test_train_evaluator_dweller_trained: set up async_session, fresh_event_bus, and
patched_session_maker, create a user, vault, an Objective with
objective_type="level_up" and target_entity={"min_level": 5}, create and persist
the ObjectiveLink for that vault/user, instantiate
LevelUpEvaluator(fresh_event_bus), emit GameEvent.DWELLER_LEVEL_UP via
fresh_event_bus with the vault.id and data {"new_level": 5}, refresh the link
from async_session and assert link.progress == 1 to verify the event-to-DB path.
In `@backend/app/tests/test_services/test_reward_service.py`:
- Around line 66-68: Replace the raw string "achievement" used as the category
in the ObjectiveCreate calls with the enum constant
ObjectiveCategoryEnum.ACHIEVEMENT so the tests use the enum type rather than a
string; locate the ObjectiveCreate invocation(s) passed into
objective_crud.create in test_reward_service.py (and the similar call around
lines 98–100) and change the category argument to
ObjectiveCategoryEnum.ACHIEVEMENT to ensure static type safety and consistency
with the other tests.
- Use Path instead of os.path for path operations - Add ruff noqa for INP001 (scripts are not packages) - Use specific boto3 exceptions instead of bare Exception - Remove unnecessary comments
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/scripts/set_rustfs_bucket_policies.py (2)
43-45: Pre-serialize the policy string once, outside the loop.
json.dumps(policy)is called on the same static dict on every loop iteration. Serialize it once before theforloop.♻️ Proposed refactor
+ policy_str = json.dumps(policy) + for bucket in buckets: try: - bucket_policy = json.dumps(policy).replace("{bucket}", bucket) + bucket_policy = policy_str.replace("{bucket}", bucket)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/scripts/set_rustfs_bucket_policies.py` around lines 43 - 45, The code calls json.dumps(policy) inside the for loop each iteration; pre-serialize the policy once before iterating to avoid repeated work by computing serialized_policy = json.dumps(policy) (or similarly named variable) and then inside the for loop use serialized_policy.replace("{bucket}", bucket) to produce bucket_policy; update references to policy/json.dumps in the loop (functionally around the for bucket in buckets: block) to use the precomputed serialized string.
18-53:set_bucket_policiesshould be a plain synchronous function.The function is declared
asyncand run withasyncio.run, but the entire body is synchronous:boto3.clientandclient.put_bucket_policyare both blocking, and there is not a singleawaitin the function. This is misleading — readers will expect non-blocking I/O — and wastes an event loop that does no async work. Either make it a plaindefor switch to an async S3 client (e.g.,aiobotocore).♻️ Proposed refactor — drop async / asyncio.run
-async def set_bucket_policies(): +def set_bucket_policies(): """Set public read policies on all whitelisted buckets.""" ... if __name__ == "__main__": - asyncio.run(set_bucket_policies()) + set_bucket_policies()And the unused
import asyncioat line 4 can be removed as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/scripts/set_rustfs_bucket_policies.py` around lines 18 - 53, The function set_bucket_policies is incorrectly declared async and invoked via asyncio.run even though it performs only synchronous boto3 calls; change set_bucket_policies to a plain def (remove async) and replace asyncio.run(set_bucket_policies()) with a simple direct call to set_bucket_policies(); also remove the now-unused asyncio import, keeping references to boto3.client and client.put_bucket_policy as-is (or switch to an async S3 client like aiobotocore only if you intend to make the function truly async).backend/scripts/fix_dweller_image_urls.py (1)
39-41: Add error handling around the commit; thebreakis also redundant.Two related points:
If
db_session.commit()raises, the exception propagates unhandled and the migration ends mid-run with no diagnostic message. Wrapping the whole loop body in atry/exceptlets you print a useful error and decide whether to re-raise.
get_async_session()yields exactly once (it has a singleyieldinsideasync with async_session_maker()), so theasync forloop naturally exhausts after the first iteration. Thebreakis unreachable by a second iteration and only adds noise.♻️ Proposed refactor
async for db_session in get_async_session(): - result = await db_session.execute(select(Dweller).where(Dweller.image_url.is_not(None))) - dwellers = result.scalars().all() - - updated_count = 0 - for dweller in dwellers: - ... - - await db_session.commit() - print(f"Updated {updated_count} URL(s) for {len(dwellers)} dwellers") - break + try: + result = await db_session.execute(select(Dweller).where(Dweller.image_url.is_not(None))) + dwellers = result.scalars().all() + + updated_count = 0 + for dweller in dwellers: + ... + + await db_session.commit() + print(f"Updated {updated_count} URL(s) for {len(dwellers)} dwellers") + except Exception as e: + print(f"Migration failed: {e}", file=sys.stderr) + raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/scripts/fix_dweller_image_urls.py` around lines 39 - 41, Wrap the commit and related per-iteration work in a try/except around db_session.commit() inside the async for loop that iterates get_async_session(); on exception log/print a clear diagnostic including updated_count and the dwellers context and then re-raise or exit as appropriate; remove the redundant break after the print since get_async_session() yields only once and the break is unreachable.
🤖 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/scripts/fix_dweller_image_urls.py`:
- Line 23: The current query only selects rows where Dweller.image_url IS NOT
NULL (see the select(Dweller).where(Dweller.image_url.is_not(None)) call), which
will skip rows that have a NULL image_url but a non-NULL thumbnail_url; change
the WHERE to select dwellers where either image_url OR thumbnail_url is not NULL
(e.g., use an OR clause around Dweller.image_url.is_not(None) and
Dweller.thumbnail_url.is_not(None)) and add the necessary import (sqlalchemy.or_
or equivalent) so db_session.execute(select(Dweller).where(...)) returns all
rows needing update.
- Around line 26-40: The current loop increments updated_count per-field
(image_url and thumbnail_url) which misleads the log into implying updated_count
is number of dwellers updated; change the logic to track both per-field updates
(keep updated_count) and per-record updates (add a new dweller_updated_count or
similar) by detecting if either field was modified for a given dweller (compare
original_image/original_thumbnail vs new values or use a boolean flag inside the
loop) and increment the record counter only once per dweller; after awaiting
db_session.commit() update the print to show both counts (e.g., "Updated X
URL(s) across Y dwellers" or similar) and reference updated_count, the new
dweller-level counter, dwellers, image_url, thumbnail_url, and db_session.commit
in your change.
---
Nitpick comments:
In `@backend/scripts/fix_dweller_image_urls.py`:
- Around line 39-41: Wrap the commit and related per-iteration work in a
try/except around db_session.commit() inside the async for loop that iterates
get_async_session(); on exception log/print a clear diagnostic including
updated_count and the dwellers context and then re-raise or exit as appropriate;
remove the redundant break after the print since get_async_session() yields only
once and the break is unreachable.
In `@backend/scripts/set_rustfs_bucket_policies.py`:
- Around line 43-45: The code calls json.dumps(policy) inside the for loop each
iteration; pre-serialize the policy once before iterating to avoid repeated work
by computing serialized_policy = json.dumps(policy) (or similarly named
variable) and then inside the for loop use serialized_policy.replace("{bucket}",
bucket) to produce bucket_policy; update references to policy/json.dumps in the
loop (functionally around the for bucket in buckets: block) to use the
precomputed serialized string.
- Around line 18-53: The function set_bucket_policies is incorrectly declared
async and invoked via asyncio.run even though it performs only synchronous boto3
calls; change set_bucket_policies to a plain def (remove async) and replace
asyncio.run(set_bucket_policies()) with a simple direct call to
set_bucket_policies(); also remove the now-unused asyncio import, keeping
references to boto3.client and client.put_bucket_policy as-is (or switch to an
async S3 client like aiobotocore only if you intend to make the function truly
async).
added 4 commits
February 19, 2026 00:07
- Replace raw string category='achievement' with ObjectiveCategoryEnum.ACHIEVEMENT - Add migration to add QUESTING to dwellerstatusenum (fixes DB error) - Add async integration test for ExpeditionEvaluator - Add async integration test for LevelUpEvaluator
- fix_dweller_image_urls: use OR clause to select dwellers with either image_url or thumbnail_url, track per-dweller vs per-field updates, add try/except around commit with diagnostics - set_rustfs_bucket_policies: pre-serialize JSON once before loop, remove unnecessary async/asyncio (boto3 is synchronous)
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
backend/app/tests/test_services/test_reward_service.py (1)
65-65: Move inlineobjective_crudimport to module level.
from app.crud.objective import objective_crudis declared inside each test function body. Since this import is now used alongside the newly modifiedObjectiveCreatecalls, it's a good opportunity to lift it to the module-level import block alongside the otherapp.*imports.♻️ Proposed refactor
At the top of the file, add:
from app.schemas.common import ObjectiveCategoryEnum from app.schemas.objective import ObjectiveCreate +from app.crud.objective import objective_crudThen remove the two inline import statements:
- from app.crud.objective import objective_crud - objective = await objective_crud.create(Also applies to: 97-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_services/test_reward_service.py` at line 65, Move the inline import "from app.crud.objective import objective_crud" out of individual test functions and add it to the module-level imports at the top of the file so tests reuse the same import; then remove the two in-function import statements where objective_crud is currently imported (references: objective_crud used alongside ObjectiveCreate in the tests). Ensure the module-level import appears with the other app.* imports and leave test bodies unchanged except for deleting the inline import lines.backend/app/alembic/versions/2026_02_18_2358-ca055b638262_add_questing_to_dwellerstatusenum.py (1)
24-25: Consider documenting whydowngrade()is a no-op.PostgreSQL has no
ALTER TYPE … REMOVE VALUEDDL, so this enum addition cannot be automatically reversed. A brief comment prevents future maintainers from wondering whether the no-op is intentional.✏️ Suggested comment
def downgrade() -> None: - pass + # PostgreSQL does not support removing enum values; manual intervention required to roll back. + pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/alembic/versions/2026_02_18_2358-ca055b638262_add_questing_to_dwellerstatusenum.py` around lines 24 - 25, The downgrade() function is currently a no-op; add a brief in-code comment inside downgrade() stating that this is intentional because PostgreSQL does not support removing enum values (no ALTER TYPE … REMOVE VALUE), so the enum addition cannot be automatically reversed and any rollback must be performed manually (e.g., create a new type, migrate data, drop old type) — reference the downgrade() function name so maintainers see why it’s left empty.backend/scripts/fix_dweller_image_urls.py (2)
52-56: Add explicit rollback before re-raising on commit failure.After a failed
commit(), the session may hold dirty state. The generator cleanup eventually callsclose(), butclose()does not guarantee a rollback in all SQLAlchemy configurations. Explicitly rolling back is safer and makes intent clear.♻️ Proposed fix
try: await db_session.commit() print(f"Updated {updated_count} URL(s) across {dweller_updated_count} dwellers") except Exception as e: + await db_session.rollback() print( f"Failed to commit updates: {e}. Updated {updated_count} URL(s) across {dweller_updated_count} dwellers" ) raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/scripts/fix_dweller_image_urls.py` around lines 52 - 56, The except block that catches exceptions from commit() should explicitly rollback the SQLAlchemy session before re-raising to avoid leaving dirty state; update the except handler around commit() to call session.rollback() (or the specific Session instance used) immediately after printing the failure message and before raise, keeping the existing log that references updated_count and dweller_updated_count and leaving session.close() untouched.
20-21: Hardcoded fallback URL silently targets a specific production host.If
settings.RUSTFS_PUBLIC_URLis not set (e.g., in a dev or staging environment), the script silently rewrites all URLs to point athttps://s3-api.evillab.dev. Consider raising an explicit error when the setting is absent instead of falling back.♻️ Proposed fix
- base_url = settings.RUSTFS_PUBLIC_URL or "https://s3-api.evillab.dev" - base_url = base_url.rstrip("/") + if not settings.RUSTFS_PUBLIC_URL: + raise RuntimeError("RUSTFS_PUBLIC_URL must be configured before running this script") + base_url = settings.RUSTFS_PUBLIC_URL.rstrip("/")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/scripts/fix_dweller_image_urls.py` around lines 20 - 21, The code currently falls back to a hardcoded host by setting base_url = settings.RUSTFS_PUBLIC_URL or "https://s3-api.evillab.dev", which can silently point non-production environments at a production host; modify the logic in fix_dweller_image_urls.py to require settings.RUSTFS_PUBLIC_URL be present and raise a clear exception (e.g., ValueError or RuntimeError) if it's missing, remove the hardcoded fallback, and ensure base_url is still normalized with base_url.rstrip("/") after validating the setting so callers of base_url get a validated, normalized URL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/scripts/fix_dweller_image_urls.py`:
- Around line 29-51: No change required: the script now correctly differentiates
per-field and per-record counts—ensure the loop logic around updated_count,
dweller_updated_count and dweller_modified remains as shown (increment
updated_count for each field change, set dweller_modified True for any change,
then increment dweller_updated_count once per dweller), commit with await
db_session.commit(), and keep the final print that references updated_count and
dweller_updated_count to report both dimensions.
- Around line 23-27: The previous query skipped rows with only thumbnail URLs;
fix it by selecting dwellers with either non-null image_url or thumbnail_url
using or_(Dweller.image_url.is_not(None), Dweller.thumbnail_url.is_not(None));
ensure the or_ symbol is imported from sqlalchemy and that this select is
executed using the existing async session iterator (get_async_session, select,
Dweller, or_) so thumbnail-only rows are included.
---
Nitpick comments:
In
`@backend/app/alembic/versions/2026_02_18_2358-ca055b638262_add_questing_to_dwellerstatusenum.py`:
- Around line 24-25: The downgrade() function is currently a no-op; add a brief
in-code comment inside downgrade() stating that this is intentional because
PostgreSQL does not support removing enum values (no ALTER TYPE … REMOVE VALUE),
so the enum addition cannot be automatically reversed and any rollback must be
performed manually (e.g., create a new type, migrate data, drop old type) —
reference the downgrade() function name so maintainers see why it’s left empty.
In `@backend/app/tests/test_services/test_reward_service.py`:
- Line 65: Move the inline import "from app.crud.objective import
objective_crud" out of individual test functions and add it to the module-level
imports at the top of the file so tests reuse the same import; then remove the
two in-function import statements where objective_crud is currently imported
(references: objective_crud used alongside ObjectiveCreate in the tests). Ensure
the module-level import appears with the other app.* imports and leave test
bodies unchanged except for deleting the inline import lines.
In `@backend/scripts/fix_dweller_image_urls.py`:
- Around line 52-56: The except block that catches exceptions from commit()
should explicitly rollback the SQLAlchemy session before re-raising to avoid
leaving dirty state; update the except handler around commit() to call
session.rollback() (or the specific Session instance used) immediately after
printing the failure message and before raise, keeping the existing log that
references updated_count and dweller_updated_count and leaving session.close()
untouched.
- Around line 20-21: The code currently falls back to a hardcoded host by
setting base_url = settings.RUSTFS_PUBLIC_URL or "https://s3-api.evillab.dev",
which can silently point non-production environments at a production host;
modify the logic in fix_dweller_image_urls.py to require
settings.RUSTFS_PUBLIC_URL be present and raise a clear exception (e.g.,
ValueError or RuntimeError) if it's missing, remove the hardcoded fallback, and
ensure base_url is still normalized with base_url.rstrip("/") after validating
the setting so callers of base_url get a validated, normalized URL.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
UI
Data
Improvements
Scripts / Ops