Fix review findings: IAM, routing guards, past-date check, roster safety

- Ring scheduler: use DynamoDBCrudPolicy (resolve_shift needs Query)
- Button pickup: update 3CX for active shift type, not just night
- Pick/drop/swap commands: only update 3CX when shift type is active
- Swap command: add missing past-date guard
- add_roster_entry: reject if extension already exists
- Apply ruff formatting

src/shared/python/shared/blocks.py

@@ -130,7 +130,9 @@ def build_shift_change_message(
     """Build a channel notification for a shift change."""
     dt = datetime.strptime(date_str, "%Y-%m-%d")
     day_label = dt.strftime("%A, %b %-d")
-    type_label = f" ({SHIFT_LABELS.get(shift_type, shift_type)})" if shift_type == "day" else ""
+    type_label = (
+        f" ({SHIFT_LABELS.get(shift_type, shift_type)})" if shift_type == "day" else ""
+    )
 
     if action == "picked_up":
         text = f":white_check_mark: <@{user_id}> picked up the *{day_label}*{type_label} shift (Ext {ext})"

src/shared/python/shared/ring_scheduler.py

@@ -27,7 +27,5 @@ def update_queue_routing(
         closed_destination=extension,
         holiday_destination=extension,
     )
-    logger.info(
+    logger.info("Updated queue %s to forward to Ext %s", queue_number, extension)
-        "Updated queue %s to forward to Ext %s", queue_number, extension
-    )
     return {"extension": extension, "queue": queue_number}

src/shared/python/shared/schedule.py

@@ -145,9 +145,7 @@ class ShiftSchedule:
     # ── Schedule post tracking ───────────────────────────────────────────
 
     def get_schedule_post(self, channel_id: str) -> dict | None:
-        resp = self.table.get_item(
+        resp = self.table.get_item(Key={"PK": "SCHEDULE_POST", "SK": channel_id})
-            Key={"PK": "SCHEDULE_POST", "SK": channel_id}
-        )
         return resp.get("Item")
 
     def save_schedule_post(
@@ -183,16 +181,22 @@ class ShiftSchedule:
         config = self.get_config()
         return config.get("admin_users", [])
 
-    def add_roster_entry(self, extension: str, name: str) -> None:
+    def add_roster_entry(self, extension: str, name: str) -> bool:
-        self.table.put_item(
+        """Add a new roster entry. Returns False if extension already exists."""
-            Item={
+        try:
-                "PK": "ROSTER",
+            self.table.put_item(
-                "SK": extension,
+                Item={
-                "name": name,
+                    "PK": "ROSTER",
-                "extension": extension,
+                    "SK": extension,
-                "slack_user_id": "",
+                    "name": name,
-            }
+                    "extension": extension,
-        )
+                    "slack_user_id": "",
+                },
+                ConditionExpression="attribute_not_exists(PK)",
+            )
+            return True
+        except self.table.meta.client.exceptions.ConditionalCheckFailedException:
+            return False
 
     def remove_roster_entry(self, extension: str) -> None:
         self.table.delete_item(Key={"PK": "ROSTER", "SK": extension})

src/slack-bot/app.py

@@ -88,6 +88,18 @@ def is_today(date_str: str) -> bool:
     return date_str == datetime.now(EASTERN).strftime("%Y-%m-%d")
 
 
+WEEKEND_DAYS = {"Saturday", "Sunday"}
+
+
+def _is_active_shift_type(shift_type: str) -> bool:
+    """Check if the given shift type is the currently active one."""
+    now = datetime.now(EASTERN)
+    day_name = now.strftime("%A")
+    if day_name in WEEKEND_DAYS and now.hour < 17:
+        return shift_type == "day"
+    return shift_type == "night"
+
+
 def create_app(
     bot_token: str, signing_secret: str, schedule_channel: str | None = None
 ) -> App:
@@ -186,11 +198,15 @@ def create_app(
             )
             return
 
-        if is_today(date_str) and shift_type == "night":
+        if is_today(date_str) and _is_active_shift_type(shift_type):
             _update_3cx_routing(employee["extension"])
 
         blocks = build_shift_change_message(
-            user_id, date_str, "picked_up", employee["extension"], employee["name"],
+            user_id,
+            date_str,
+            "picked_up",
+            employee["extension"],
+            employee["name"],
             shift_type=shift_type,
         )
         respond(
@@ -375,7 +391,7 @@ def create_app(
 
         schedule.set_override(date_str, employee["extension"], employee["name"])
 
-        if is_today(date_str):
+        if is_today(date_str) and _is_active_shift_type("night"):
             _update_3cx_routing(employee["extension"])
 
         respond(text=f"You picked up the shift for *{date.strftime('%A, %b %-d')}*.")
@@ -427,7 +443,7 @@ def create_app(
 
         schedule.mark_open(date_str)
 
-        if is_today(date_str):
+        if is_today(date_str) and _is_active_shift_type("night"):
             _update_3cx_routing(FALLBACK_EXTENSION)
 
         respond(
@@ -465,6 +481,10 @@ def create_app(
             return
 
         date_str = date.strftime("%Y-%m-%d")
+        if date_str < datetime.now(EASTERN).strftime("%Y-%m-%d"):
+            respond(text="You can't swap a shift in the past.")
+            return
+
         day_name = date.strftime("%A")
         ext, name, source = schedule.resolve_shift(date_str, day_name)
 
@@ -493,7 +513,7 @@ def create_app(
 
         schedule.set_override(date_str, target["extension"], target["name"])
 
-        if is_today(date_str):
+        if is_today(date_str) and _is_active_shift_type("night"):
             _update_3cx_routing(target["extension"])
 
         blocks = build_shift_change_message(
@@ -597,7 +617,12 @@ def create_app(
                     return
                 ext = parts[3]
                 name = " ".join(parts[4:])
-                schedule.add_roster_entry(ext, name)
+                added = schedule.add_roster_entry(ext, name)
+                if not added:
+                    respond(
+                        text=f"Extension `{ext}` already exists. Use `roster rename` to change the name."
+                    )
+                    return
                 respond(text=f"Added *{name}* (Ext {ext}) to the roster.")
 
             elif roster_cmd == "remove":
@@ -630,7 +655,9 @@ def create_app(
                 )
 
             else:
-                respond(text="Unknown roster command. Use `add`, `remove`, or `rename`.")
+                respond(
+                    text="Unknown roster command. Use `add`, `remove`, or `rename`."
+                )
 
         else:
             respond(text=f"Unknown admin command: `{subcmd}`. Try `/oncall help`.")

src/weekly-post/app.py

@@ -212,7 +212,9 @@ def handler(event, context):
     schedule.save_schedule_post(
         channel_id, result["ts"], this_monday.strftime("%Y-%m-%d")
     )
-    logger.info("Posted weekly schedule to channel %s (ts=%s)", channel_id, result["ts"])
+    logger.info(
+        "Posted weekly schedule to channel %s (ts=%s)", channel_id, result["ts"]
+    )
     return {
         "posted": True,
         "channel": channel_id,

template.yaml

@@ -195,12 +195,9 @@ Resources:
           QUEUE_NUMBER: !Ref QueueNumber
           TZ: !Ref Timezone
       Policies:
+        - DynamoDBCrudPolicy:
+            TableName: !Ref ShiftTable
         - Statement:
-            - Effect: Allow
-              Action:
-                - dynamodb:GetItem
-              Resource:
-                - !GetAtt ShiftTable.Arn
             - Effect: Allow
               Action:
                 - secretsmanager:GetSecretValue