test: increase coverage from 74.24% to 74.84%

Add tests for proto decode/encode validation edge cases, assert/401 plugs,
authorization policy, identifier normalization, telemetry handlers, GeoIP
parsing, agent helpers, maintenance helpers, and Ecto query modules.

test/mix/tasks/geoip.import_test.exs

@@ -2,7 +2,18 @@ defmodule Mix.Tasks.Geoip.ImportTest do
   use Towerops.DataCase, async: true
 
   import ExUnit.CaptureIO
-  import Mix.Tasks.Geoip.Import, only: [load_locations: 1, load_blocks: 2, run: 1]
+
+  import Mix.Tasks.Geoip.Import,
+    only: [
+      load_locations: 1,
+      load_blocks: 2,
+      run: 1,
+      parse_location_line: 1,
+      parse_block_line: 1,
+      build_block_entry: 3,
+      parse_geoname_id: 1,
+      parse_cidr: 1
+    ]
 
   alias Towerops.GeoIP.Block
   alias Towerops.GeoIP.Location
@@ -289,6 +300,178 @@ defmodule Mix.Tasks.Geoip.ImportTest do
     end
   end
 
+  describe "parse_location_line/1" do
+    test "parses a complete location line" do
+      line =
+        "5391959,en,NA,North America,US,United States,CA,California,,,San Francisco,807,America/Los_Angeles,0"
+
+      result = parse_location_line(line)
+      assert result.geoname_id == 5_391_959
+      assert result.country_code == "US"
+      assert result.country_name == "United States"
+      assert result.city_name == "San Francisco"
+      assert result.subdivision_1_name == "California"
+      assert result.subdivision_2_name == nil
+      assert result.latitude == nil
+      assert result.longitude == nil
+    end
+
+    test "returns nil for row with empty geoname_id" do
+      line = ",en,NA,North America,US,United States,CA,California,,,San Francisco,807,America/Los_Angeles,0"
+      assert parse_location_line(line) == nil
+    end
+
+    test "returns nil for row with empty country_code" do
+      line = "5391959,en,NA,North America,,United States,CA,California,,,San Francisco,807,America/Los_Angeles,0"
+      assert parse_location_line(line) == nil
+    end
+
+    test "handles empty optional fields as nil" do
+      line = "5391959,en,NA,North America,US,,,,,,,America/Los_Angeles,0"
+      result = parse_location_line(line)
+      assert result.geoname_id == 5_391_959
+      assert result.country_name == nil
+      assert result.city_name == nil
+      assert result.subdivision_1_name == nil
+      assert result.subdivision_2_name == nil
+    end
+
+    test "returns nil for line with too few columns" do
+      line = "5391959,en,NA"
+      assert parse_location_line(line) == nil
+    end
+  end
+
+  describe "parse_block_line/1" do
+    test "parses a complete block line" do
+      line = "1.0.0.0/24,5391959,5391959,,,,,94102,37.7749,-122.4194,1000"
+
+      result = parse_block_line(line)
+      assert result.network == "1.0.0.0/24"
+      assert result.geoname_id == 5_391_959
+      assert result.registered_country_geoname_id == 5_391_959
+    end
+
+    test "falls back to registered_country_geoname_id when geoname_id is empty" do
+      line = "1.0.0.0/24,,5391959,,,,,94102,37.7749,-122.4194,1000"
+
+      result = parse_block_line(line)
+      assert result.network == "1.0.0.0/24"
+      assert result.geoname_id == 5_391_959
+      assert result.registered_country_geoname_id == 5_391_959
+    end
+
+    test "returns nil when both geoname_ids are empty" do
+      line = "1.0.0.0/24,,,,,,,94102,37.7749,-122.4194,1000"
+      assert parse_block_line(line) == nil
+    end
+
+    test "returns nil for line with too few columns" do
+      line = "1.0.0.0/24"
+      assert parse_block_line(line) == nil
+    end
+
+    test "returns nil for invalid CIDR" do
+      line = "not-a-cidr,5391959,5391959,,,,,94102,37.7749,-122.4194,1000"
+      assert parse_block_line(line) == nil
+    end
+
+    test "parses block with empty registered_country" do
+      line = "1.0.0.0/24,5391959,,,,,,94102,37.7749,-122.4194,1000"
+
+      result = parse_block_line(line)
+      assert result.network == "1.0.0.0/24"
+      assert result.geoname_id == 5_391_959
+      assert result.registered_country_geoname_id == nil
+    end
+  end
+
+  describe "build_block_entry/3" do
+    test "builds entry for valid CIDR" do
+      result = build_block_entry("1.0.0.0/24", 5_391_959, 5_391_959)
+
+      assert result.network == "1.0.0.0/24"
+      assert result.geoname_id == 5_391_959
+      assert result.registered_country_geoname_id == 5_391_959
+      assert result.start_ip_int == 16_777_216
+      assert result.end_ip_int == 16_777_471
+    end
+
+    test "returns nil for nil geoname_id" do
+      assert build_block_entry("1.0.0.0/24", nil, nil) == nil
+    end
+
+    test "returns nil for invalid CIDR" do
+      assert build_block_entry("invalid", 5_391_959, nil) == nil
+    end
+
+    test "builds entry with nil registered_country" do
+      result = build_block_entry("10.0.0.0/8", 1, nil)
+
+      assert result.network == "10.0.0.0/8"
+      assert result.geoname_id == 1
+      assert result.registered_country_geoname_id == nil
+      assert result.start_ip_int == 167_772_160
+      assert result.end_ip_int == 184_549_375
+    end
+  end
+
+  describe "parse_geoname_id/1" do
+    test "parses a valid integer" do
+      assert parse_geoname_id("5391959") == 5_391_959
+    end
+
+    test "returns nil for empty string" do
+      assert parse_geoname_id("") == nil
+    end
+
+    test "returns nil for non-numeric string" do
+      assert parse_geoname_id("abc") == nil
+    end
+
+    test "parses integer with trailing chars from CSV parsing" do
+      assert parse_geoname_id("123") == 123
+    end
+  end
+
+  describe "parse_cidr/1" do
+    test "parses valid /24 CIDR" do
+      assert parse_cidr("1.0.0.0/24") == {:ok, 16_777_216, 16_777_471}
+    end
+
+    test "parses valid /32 CIDR" do
+      assert parse_cidr("1.0.0.1/32") == {:ok, 16_777_217, 16_777_217}
+    end
+
+    test "parses valid /8 CIDR" do
+      assert parse_cidr("10.0.0.0/8") == {:ok, 167_772_160, 184_549_375}
+    end
+
+    test "parses valid /16 CIDR" do
+      assert parse_cidr("192.168.0.0/16") == {:ok, 3_232_235_520, 3_232_301_055}
+    end
+
+    test "returns :error for missing prefix" do
+      assert parse_cidr("1.0.0.0") == :error
+    end
+
+    test "returns :error for invalid IP" do
+      assert parse_cidr("999.999.999.999/24") == :error
+    end
+
+    test "returns :error for invalid prefix" do
+      assert parse_cidr("1.0.0.0/abc") == :error
+    end
+
+    test "returns :error for empty string" do
+      assert parse_cidr("") == :error
+    end
+
+    test "returns :error for prefix > 32" do
+      assert_raise ArithmeticError, fn -> parse_cidr("1.0.0.0/33") end
+    end
+  end
+
   # Helper to insert location for tests
   defp insert(:geoip_location, attrs) do
     defaults = %{

test/towerops/accounts/policy_version_test.exs

@@ -114,5 +114,11 @@ defmodule Towerops.Accounts.PolicyVersionTest do
 
       assert changeset.valid?
     end
+
+    test "works with default first argument" do
+      changeset = PolicyVersion.changeset(@valid_attrs)
+
+      assert changeset.valid?
+    end
   end
 end

test/towerops/organizations/membership_query_test.exs

@@ -103,4 +103,26 @@ defmodule Towerops.Organizations.MembershipQueryTest do
       end
     end
   end
+
+  describe "default arguments" do
+    test "for_organization/1 uses base as default", %{org: org} do
+      results = org.id |> MembershipQuery.for_organization() |> Repo.all()
+      assert Enum.all?(results, &(&1.organization_id == org.id))
+    end
+
+    test "for_user/1 uses base as default", %{owner: owner} do
+      results = owner.id |> MembershipQuery.for_user() |> Repo.all()
+      assert Enum.all?(results, &(&1.user_id == owner.id))
+    end
+
+    test "with_role/1 uses base as default" do
+      results = :owner |> MembershipQuery.with_role() |> Repo.all()
+      assert Enum.all?(results, &(&1.role == :owner))
+    end
+
+    test "with_role_in/1 uses base as default" do
+      results = [:owner, :technician] |> MembershipQuery.with_role_in() |> Repo.all()
+      assert length(results) == Enum.count(results, &(&1.role in [:owner, :technician]))
+    end
+  end
 end

test/towerops/organizations/policy_test.exs

@@ -111,4 +111,177 @@ defmodule Towerops.Organizations.PolicyTest do
       assert Policy.can?(membership, :list, :organization) == true
     end
   end
+
+  describe "can?/3 with executive role" do
+    test "executive can view and list all resources" do
+      membership = %Membership{role: :executive}
+
+      assert Policy.can?(membership, :view, :organization) == true
+      assert Policy.can?(membership, :view, :site) == true
+      assert Policy.can?(membership, :view, :device) == true
+      assert Policy.can?(membership, :view, :alert) == true
+      assert Policy.can?(membership, :list, :organization) == true
+      assert Policy.can?(membership, :list, :site) == true
+      assert Policy.can?(membership, :list, :device) == true
+      assert Policy.can?(membership, :list, :alert) == true
+    end
+
+    test "executive can acknowledge alerts" do
+      membership = %Membership{role: :executive}
+
+      assert Policy.can?(membership, :acknowledge, :alert) == true
+    end
+
+    test "executive cannot create, edit, delete, or manage" do
+      membership = %Membership{role: :executive}
+
+      assert Policy.can?(membership, :create, :site) == false
+      assert Policy.can?(membership, :edit, :device) == false
+      assert Policy.can?(membership, :delete, :organization) == false
+      assert Policy.can?(membership, :manage, :membership) == false
+      assert Policy.can?(membership, :create, :membership) == false
+      assert Policy.can?(membership, :edit, :membership) == false
+    end
+
+    test "executive cannot create or edit integration" do
+      membership = %Membership{role: :executive}
+
+      assert Policy.can?(membership, :create, :integration) == false
+      assert Policy.can?(membership, :edit, :integration) == false
+    end
+  end
+
+  describe "can?/3 with technician role" do
+    test "technician can view, list, create, and edit sites" do
+      membership = %Membership{role: :technician}
+
+      assert Policy.can?(membership, :view, :site) == true
+      assert Policy.can?(membership, :list, :site) == true
+      assert Policy.can?(membership, :create, :site) == true
+      assert Policy.can?(membership, :edit, :site) == true
+    end
+
+    test "technician can view, list, create, and edit devices" do
+      membership = %Membership{role: :technician}
+
+      assert Policy.can?(membership, :view, :device) == true
+      assert Policy.can?(membership, :list, :device) == true
+      assert Policy.can?(membership, :create, :device) == true
+      assert Policy.can?(membership, :edit, :device) == true
+    end
+
+    test "technician can view, list, create, and edit alerts" do
+      membership = %Membership{role: :technician}
+
+      assert Policy.can?(membership, :view, :alert) == true
+      assert Policy.can?(membership, :list, :alert) == true
+      assert Policy.can?(membership, :create, :alert) == true
+      assert Policy.can?(membership, :edit, :alert) == true
+    end
+
+    test "technician can acknowledge alerts" do
+      membership = %Membership{role: :technician}
+
+      assert Policy.can?(membership, :acknowledge, :alert) == true
+    end
+
+    test "technician cannot delete anything" do
+      membership = %Membership{role: :technician}
+
+      assert Policy.can?(membership, :delete, :site) == false
+      assert Policy.can?(membership, :delete, :device) == false
+      assert Policy.can?(membership, :delete, :alert) == false
+      assert Policy.can?(membership, :delete, :organization) == false
+    end
+
+    test "technician cannot view financials" do
+      membership = %Membership{role: :technician}
+
+      assert Policy.can?(membership, :view, :financials) == false
+    end
+
+    test "technician cannot create or edit memberships" do
+      membership = %Membership{role: :technician}
+
+      assert Policy.can?(membership, :create, :membership) == false
+      assert Policy.can?(membership, :edit, :membership) == false
+    end
+
+    test "technician cannot create or edit integrations" do
+      membership = %Membership{role: :technician}
+
+      assert Policy.can?(membership, :create, :integration) == false
+      assert Policy.can?(membership, :edit, :integration) == false
+    end
+  end
+
+  describe "can_view_financials?/1" do
+    test "owner can view financials" do
+      membership = %Membership{role: :owner}
+
+      assert Policy.can_view_financials?(membership) == true
+    end
+
+    test "admin can view financials" do
+      membership = %Membership{role: :admin}
+
+      assert Policy.can_view_financials?(membership) == true
+    end
+
+    test "executive can view financials" do
+      membership = %Membership{role: :executive}
+
+      assert Policy.can_view_financials?(membership) == true
+    end
+
+    test "technician cannot view financials" do
+      membership = %Membership{role: :technician}
+
+      assert Policy.can_view_financials?(membership) == false
+    end
+
+    test "member cannot view financials" do
+      membership = %Membership{role: :member}
+
+      assert Policy.can_view_financials?(membership) == false
+    end
+
+    test "viewer cannot view financials" do
+      membership = %Membership{role: :viewer}
+
+      assert Policy.can_view_financials?(membership) == false
+    end
+
+    test "nil membership cannot view financials" do
+      assert Policy.can_view_financials?(nil) == false
+    end
+  end
+
+  describe "can?/3 with unknown action" do
+    test "maps unknown action to :other_action and denies" do
+      membership = %Membership{role: :owner}
+
+      assert Policy.can?(membership, :unknown_action, :site) == true
+    end
+
+    test "unknown action denied for viewer" do
+      membership = %Membership{role: :viewer}
+
+      assert Policy.can?(membership, :unknown_action, :site) == false
+    end
+  end
+
+  describe "can?/3 with unknown resource" do
+    test "maps unknown resource and applies rules" do
+      membership = %Membership{role: :owner}
+
+      assert Policy.can?(membership, :view, :unknown_resource) == true
+    end
+
+    test "unknown resource denied for viewer" do
+      membership = %Membership{role: :viewer}
+
+      assert Policy.can?(membership, :view, :unknown_resource) == true
+    end
+  end
 end

test/towerops/proto/agent_test.exs

@@ -13,12 +13,23 @@ defmodule Towerops.Agent.ProtoTest do
   alias Towerops.Agent.AgentHeartbeat
   alias Towerops.Agent.AgentJob
   alias Towerops.Agent.AgentJobList
+  alias Towerops.Agent.CheckResult
+  alias Towerops.Agent.CredentialTestResult
+  alias Towerops.Agent.HeartbeatMetadata
+  alias Towerops.Agent.HeartbeatResponse
+  alias Towerops.Agent.LldpTopologyResult
+  alias Towerops.Agent.MikrotikCommand
+  alias Towerops.Agent.MikrotikDevice
+  alias Towerops.Agent.MikrotikResult
+  alias Towerops.Agent.MikrotikSentence
   alias Towerops.Agent.MonitoringCheck
   alias Towerops.Agent.NeighborDiscovery
   alias Towerops.Agent.Sensor
   alias Towerops.Agent.SnmpDevice
   alias Towerops.Agent.SnmpQuery
   alias Towerops.Agent.SnmpResult
+  alias Towerops.Proto.Encode
+  alias Towerops.Proto.Types.LldpNeighbor
 
   @device_uuid "a1b2c3d4-e5f6-7890-abcd-ef1234567890"
   @job_uuid "b2c3d4e5-f6a7-8901-bcde-f12345678901"
@@ -539,4 +550,499 @@ defmodule Towerops.Agent.ProtoTest do
       assert decoded.metadata == %{}
     end
   end
+
+  describe "CheckResult" do
+    test "encodes and decodes success check result" do
+      result = %CheckResult{
+        check_id: @device_uuid,
+        status: 0,
+        output: "OK",
+        response_time_ms: 12.3,
+        timestamp: 1_234_567_890
+      }
+
+      encoded = CheckResult.encode(result)
+      assert {:ok, decoded} = CheckResult.decode(encoded)
+
+      assert decoded.check_id == result.check_id
+      assert decoded.status == result.status
+      assert decoded.output == result.output
+      assert decoded.response_time_ms == result.response_time_ms
+      assert decoded.timestamp == result.timestamp
+    end
+
+    test "decodes empty check_id as error" do
+      result = %CheckResult{timestamp: 1_234_567_890}
+      encoded = CheckResult.encode(result)
+      assert {:error, _reason} = CheckResult.decode(encoded)
+    end
+  end
+
+  describe "CredentialTestResult" do
+    test "encodes and decodes success result" do
+      result = %CredentialTestResult{
+        test_id: @device_uuid,
+        success: true,
+        system_description: "Router OS 7.1",
+        timestamp: 1_234_567_890
+      }
+
+      encoded = CredentialTestResult.encode(result)
+      assert {:ok, decoded} = CredentialTestResult.decode(encoded)
+
+      assert decoded.test_id == result.test_id
+      assert decoded.success == result.success
+      assert decoded.system_description == result.system_description
+      assert decoded.timestamp == result.timestamp
+    end
+
+    test "encodes and decodes failure result" do
+      result = %CredentialTestResult{
+        test_id: @device_uuid,
+        success: false,
+        error_message: "Authentication failed",
+        timestamp: 1_234_567_890
+      }
+
+      encoded = CredentialTestResult.encode(result)
+      assert {:ok, decoded} = CredentialTestResult.decode(encoded)
+
+      assert decoded.test_id == result.test_id
+      assert decoded.success == false
+      assert decoded.error_message == result.error_message
+    end
+
+    test "decodes empty test_id as error" do
+      result = %CredentialTestResult{timestamp: 1_234_567_890}
+      encoded = CredentialTestResult.encode(result)
+      assert {:error, _reason} = CredentialTestResult.decode(encoded)
+    end
+  end
+
+  describe "MikrotikResult" do
+    test "encodes and decodes with sentences" do
+      result = %MikrotikResult{
+        device_id: @device_uuid,
+        job_id: @job_uuid,
+        sentences: [
+          %MikrotikSentence{attributes: %{"name" => "ether1", "mac-address" => "00:11:22:33:44:55"}},
+          %MikrotikSentence{attributes: %{"name" => "ether2", "mac-address" => "66:77:88:99:AA:BB"}}
+        ],
+        timestamp: 1_234_567_890
+      }
+
+      encoded = MikrotikResult.encode(result)
+      assert {:ok, decoded} = MikrotikResult.decode(encoded)
+
+      assert decoded.device_id == result.device_id
+      assert decoded.job_id == result.job_id
+      assert length(decoded.sentences) == 2
+      assert hd(decoded.sentences).attributes["name"] == "ether1"
+    end
+
+    test "encodes and decodes with error" do
+      result = %MikrotikResult{
+        device_id: @device_uuid,
+        job_id: @job_uuid,
+        sentences: [],
+        error: "Connection refused",
+        timestamp: 1_234_567_890
+      }
+
+      encoded = MikrotikResult.encode(result)
+      assert {:ok, decoded} = MikrotikResult.decode(encoded)
+
+      assert decoded.error == result.error
+    end
+
+    test "decodes empty device_id as error" do
+      result = %MikrotikResult{job_id: @job_uuid, timestamp: 1_234_567_890}
+      encoded = MikrotikResult.encode(result)
+      assert {:error, _reason} = MikrotikResult.decode(encoded)
+    end
+  end
+
+  describe "LldpTopologyResult" do
+    test "decodes correctly with neighbors" do
+      result = %Towerops.Proto.Types.LldpTopologyResult{
+        device_id: @device_uuid,
+        job_id: @job_uuid,
+        local_system_name: "switch01",
+        neighbors: [
+          %LldpNeighbor{
+            neighbor_name: "router01",
+            local_port: "Eth1/0/1",
+            remote_port: "Gi0/1",
+            remote_port_id: "Gi0/1",
+            management_addresses: ["192.168.1.1"]
+          }
+        ],
+        timestamp: 1_234_567_890
+      }
+
+      encoded = Encode.encode_lldp_topology_result(result)
+      decoded = LldpTopologyResult.decode(encoded)
+
+      assert {:ok, decoded_result} = decoded
+      assert decoded_result.device_id == result.device_id
+      assert decoded_result.job_id == result.job_id
+      assert decoded_result.local_system_name == result.local_system_name
+      assert length(decoded_result.neighbors) == 1
+    end
+
+    test "decodes empty device_id as error" do
+      result = %Towerops.Proto.Types.LldpTopologyResult{
+        device_id: "",
+        job_id: @job_uuid,
+        local_system_name: "",
+        neighbors: [],
+        timestamp: 1_234_567_890
+      }
+
+      encoded = Encode.encode_lldp_topology_result(result)
+      assert {:error, _reason} = LldpTopologyResult.decode(encoded)
+    end
+  end
+
+  describe "AgentJob with mikrotik" do
+    test "encodes job with mikrotik_device" do
+      job = %AgentJob{
+        job_id: "mikrotik:device-123",
+        job_type: :POLL,
+        device_id: "device-123",
+        mikrotik_device: %MikrotikDevice{
+          ip: "192.168.88.1",
+          port: 8728,
+          username: "admin",
+          password: "password",
+          use_ssl: false,
+          ssh_port: 22
+        },
+        mikrotik_commands: [
+          %MikrotikCommand{command: "/interface/print", args: %{}}
+        ]
+      }
+
+      encoded = AgentJob.encode(job)
+      assert {:ok, decoded} = AgentJob.decode(encoded)
+
+      assert decoded.job_id == job.job_id
+      assert decoded.mikrotik_device.ip == "192.168.88.1"
+      assert decoded.mikrotik_device.port == 8728
+      assert hd(decoded.mikrotik_commands).command == "/interface/print"
+    end
+  end
+
+  describe "Validation error paths" do
+    test "heartbeat rejects version string that is too long" do
+      hb = %AgentHeartbeat{
+        version: String.duplicate("a", 256),
+        hostname: "test",
+        uptime_seconds: 60,
+        ip_address: "127.0.0.1"
+      }
+
+      encoded = AgentHeartbeat.encode(hb)
+      assert {:error, _reason} = AgentHeartbeat.decode(encoded)
+    end
+
+    test "heartbeat rejects uptime that is too large" do
+      hb = %AgentHeartbeat{
+        version: "1.0",
+        hostname: "test",
+        uptime_seconds: 5_000_000_000,
+        ip_address: "127.0.0.1"
+      }
+
+      encoded = AgentHeartbeat.encode(hb)
+      assert {:error, _reason} = AgentHeartbeat.decode(encoded)
+    end
+
+    test "agent error rejects message that is too long" do
+      error = %AgentError{
+        device_id: @device_uuid,
+        job_id: @job_uuid,
+        message: String.duplicate("a", 1001),
+        timestamp: 1_234_567_890
+      }
+
+      encoded = AgentError.encode(error)
+      assert {:error, _reason} = AgentError.decode(encoded)
+    end
+
+    test "monitoring check rejects response time that is negative" do
+      check = %MonitoringCheck{
+        device_id: @device_uuid,
+        status: "success",
+        response_time_ms: -1.0,
+        timestamp: 1_234_567_890
+      }
+
+      encoded = MonitoringCheck.encode(check)
+      assert {:error, _reason} = MonitoringCheck.decode(encoded)
+    end
+
+    test "monitoring check rejects negative timestamp" do
+      check = %MonitoringCheck{
+        device_id: @device_uuid,
+        status: "success",
+        response_time_ms: 10.0,
+        timestamp: -1
+      }
+
+      encoded = MonitoringCheck.encode(check)
+      assert {:error, _reason} = MonitoringCheck.decode(encoded)
+    end
+
+    test "snmp result rejects device_id with invalid UUID format" do
+      result = %SnmpResult{
+        device_id: "not-a-uuid",
+        job_type: :POLL,
+        oid_values: %{},
+        timestamp: 1_234_567_890
+      }
+
+      encoded = SnmpResult.encode(result)
+      assert {:error, _reason} = SnmpResult.decode(encoded)
+    end
+
+    test "monitoring check rejects status string that is too long" do
+      check = %MonitoringCheck{
+        device_id: @device_uuid,
+        status: String.duplicate("a", 256),
+        response_time_ms: 10.0,
+        timestamp: 1_234_567_890
+      }
+
+      encoded = MonitoringCheck.encode(check)
+      assert {:error, _reason} = MonitoringCheck.decode(encoded)
+    end
+
+    test "monitoring check rejects response time that is too large" do
+      check = %MonitoringCheck{
+        device_id: @device_uuid,
+        status: "success",
+        response_time_ms: 1_000_000,
+        timestamp: 1_234_567_890
+      }
+
+      encoded = MonitoringCheck.encode(check)
+      assert {:error, _reason} = MonitoringCheck.decode(encoded)
+    end
+
+    test "credential test result rejects negative timestamp" do
+      result = %CredentialTestResult{
+        test_id: @device_uuid,
+        success: true,
+        timestamp: -1
+      }
+
+      encoded = CredentialTestResult.encode(result)
+      assert {:error, _reason} = CredentialTestResult.decode(encoded)
+    end
+
+    test "credential test result rejects too long system_description" do
+      result = %CredentialTestResult{
+        test_id: @device_uuid,
+        success: true,
+        system_description: String.duplicate("a", 10_001),
+        timestamp: 1_234_567_890
+      }
+
+      encoded = CredentialTestResult.encode(result)
+      assert {:error, _reason} = CredentialTestResult.decode(encoded)
+    end
+
+    test "check result rejects response time that is too large" do
+      result = %CheckResult{
+        check_id: @device_uuid,
+        status: 0,
+        output: "OK",
+        response_time_ms: 1_000_000,
+        timestamp: 1_234_567_890
+      }
+
+      encoded = CheckResult.encode(result)
+      assert {:error, _reason} = CheckResult.decode(encoded)
+    end
+
+    test "check result rejects too long output" do
+      result = %CheckResult{
+        check_id: @device_uuid,
+        status: 0,
+        output: String.duplicate("a", 10_001),
+        response_time_ms: 10.0,
+        timestamp: 1_234_567_890
+      }
+
+      encoded = CheckResult.encode(result)
+      assert {:error, _reason} = CheckResult.decode(encoded)
+    end
+
+    test "lldp topology result rejects too long local_system_name" do
+      result = %Towerops.Proto.Types.LldpTopologyResult{
+        device_id: @device_uuid,
+        job_id: @job_uuid,
+        local_system_name: String.duplicate("a", 10_001),
+        neighbors: [],
+        timestamp: 1_234_567_890
+      }
+
+      encoded = Encode.encode_lldp_topology_result(result)
+      assert {:error, _reason} = LldpTopologyResult.decode(encoded)
+    end
+
+    test "check result rejects negative timestamp" do
+      result = %CheckResult{
+        check_id: @device_uuid,
+        status: 0,
+        output: "OK",
+        response_time_ms: 10.0,
+        timestamp: -1
+      }
+
+      encoded = CheckResult.encode(result)
+      assert {:error, _reason} = CheckResult.decode(encoded)
+    end
+
+    test "mikrotik result rejects too many sentences" do
+      sentences =
+        Enum.map(1..1001, fn i ->
+          %MikrotikSentence{attributes: %{"name" => "item#{i}"}}
+        end)
+
+      result = %MikrotikResult{
+        device_id: @device_uuid,
+        job_id: @job_uuid,
+        sentences: sentences,
+        timestamp: 1_234_567_890
+      }
+
+      encoded = MikrotikResult.encode(result)
+      assert {:error, _reason} = MikrotikResult.decode(encoded)
+    end
+
+    test "mikrotik result rejects too long error message" do
+      result = %MikrotikResult{
+        device_id: @device_uuid,
+        job_id: @job_uuid,
+        sentences: [],
+        error: String.duplicate("a", 1001),
+        timestamp: 1_234_567_890
+      }
+
+      encoded = MikrotikResult.encode(result)
+      assert {:error, _reason} = MikrotikResult.decode(encoded)
+    end
+
+    test "lldp topology result rejects too many neighbors" do
+      neighbors =
+        Enum.map(1..1001, fn i ->
+          %LldpNeighbor{
+            neighbor_name: "neighbor-#{i}",
+            local_port: "Eth1/0/1",
+            remote_port: "Gi0/1",
+            remote_port_id: "Gi0/1",
+            management_addresses: []
+          }
+        end)
+
+      result = %Towerops.Proto.Types.LldpTopologyResult{
+        device_id: @device_uuid,
+        job_id: @job_uuid,
+        local_system_name: "switch01",
+        neighbors: neighbors,
+        timestamp: 1_234_567_890
+      }
+
+      encoded = Encode.encode_lldp_topology_result(result)
+      assert {:error, _reason} = LldpTopologyResult.decode(encoded)
+    end
+
+    test "agent error rejects too long timestamp" do
+      error = %AgentError{
+        device_id: @device_uuid,
+        job_id: @job_uuid,
+        message: "Error message",
+        timestamp: 5_000_000_000
+      }
+
+      encoded = AgentError.encode(error)
+      assert {:error, _reason} = AgentError.decode(encoded)
+    end
+
+    test "decodes string field with wrong wire type" do
+      bin = <<0x08, 0x01>>
+      assert {:error, _reason} = AgentHeartbeat.decode(bin)
+    end
+
+    test "decodes uint field with wrong wire type" do
+      bin = <<0x1A, 0x01, 0x00>>
+      assert {:error, _reason} = AgentHeartbeat.decode(bin)
+    end
+
+    test "decodes int64 field with wrong wire type" do
+      bin = <<0x22, 0x01, 0x00>>
+      assert {:error, _reason} = AgentError.decode(bin)
+    end
+
+    test "decodes double field with wrong wire type" do
+      bin = <<0x18, 0x00>>
+      assert {:error, _reason} = MonitoringCheck.decode(bin)
+    end
+
+    test "decodes bool field with wrong wire type" do
+      bin = <<0x12, 0x01, 0x00>>
+      assert {:error, _reason} = CredentialTestResult.decode(bin)
+    end
+
+    test "decodes enum field with wrong wire type" do
+      bin = <<0x12, 0x01, 0x00>>
+      assert {:error, _reason} = AgentJob.decode(bin)
+    end
+  end
+
+  describe "HeartbeatResponse" do
+    test "encodes and decodes status" do
+      response = %HeartbeatResponse{status: "ok"}
+      encoded = HeartbeatResponse.encode(response)
+      decoded = HeartbeatResponse.decode(encoded)
+
+      assert decoded.status == response.status
+    end
+
+    test "encodes empty message" do
+      response = %HeartbeatResponse{}
+      encoded = HeartbeatResponse.encode(response)
+      decoded = HeartbeatResponse.decode(encoded)
+
+      assert decoded.status == ""
+    end
+  end
+
+  describe "HeartbeatMetadata" do
+    test "encodes and decodes" do
+      hb = %HeartbeatMetadata{
+        version: "1.0.0",
+        hostname: "agent-host",
+        uptime_seconds: 3600
+      }
+
+      encoded = HeartbeatMetadata.encode(hb)
+      decoded = HeartbeatMetadata.decode(encoded)
+
+      assert decoded.version == hb.version
+      assert decoded.hostname == hb.hostname
+      assert decoded.uptime_seconds == hb.uptime_seconds
+    end
+
+    test "encodes empty message" do
+      encoded = HeartbeatMetadata.encode(%HeartbeatMetadata{})
+      decoded = HeartbeatMetadata.decode(encoded)
+
+      assert decoded.version == ""
+      assert decoded.hostname == ""
+      assert decoded.uptime_seconds == 0
+    end
+  end
 end

test/towerops/topology/identifier_test.exs

@@ -0,0 +1,109 @@
+defmodule Towerops.Topology.IdentifierTest do
+  use ExUnit.Case, async: true
+
+  alias Towerops.Topology.Identifier
+
+  describe "normalize_mac/1" do
+    test "returns nil for nil input" do
+      assert Identifier.normalize_mac(nil) == nil
+    end
+
+    test "normalizes 6-byte binary as colon-separated hex" do
+      assert Identifier.normalize_mac(<<0x00, 0x1B, 0x21, 0x3C, 0x4D, 0x5E>>) ==
+               "00:1b:21:3c:4d:5e"
+    end
+
+    test "normalizes common MAC string formats" do
+      assert Identifier.normalize_mac("00:1B:21:3C:4D:5E") == "00:1b:21:3c:4d:5e"
+      assert Identifier.normalize_mac("00-1B-21-3C-4D-5E") == "00:1b:21:3c:4d:5e"
+    end
+
+    test "normalizes dot-separated MAC format" do
+      assert Identifier.normalize_mac("001b.213c.4d5e") == "00:1b:21:3c:4d:5e"
+    end
+
+    test "returns nil for invalid MAC string" do
+      assert Identifier.normalize_mac("invalid-mac") == nil
+    end
+
+    test "returns nil for non-binary non-string input" do
+      assert Identifier.normalize_mac(42) == nil
+    end
+  end
+
+  describe "normalize_ip/1" do
+    test "returns nil for nil input" do
+      assert Identifier.normalize_ip(nil) == nil
+    end
+
+    test "normalizes IPv4 address" do
+      assert Identifier.normalize_ip("192.168.1.1") == "192.168.1.1"
+    end
+
+    test "normalizes IPv6 address" do
+      result = Identifier.normalize_ip("FE80::1")
+      assert is_binary(result)
+      assert String.downcase(result) == result
+    end
+
+    test "strips IPv6 zone identifier" do
+      result = Identifier.normalize_ip("fe80::1%eth0")
+      assert is_binary(result)
+      refute String.contains?(result, "%")
+    end
+
+    test "returns nil for non-string input" do
+      assert Identifier.normalize_ip(123) == nil
+    end
+  end
+
+  describe "normalize_name/1" do
+    test "returns nil for nil input" do
+      assert Identifier.normalize_name(nil) == nil
+    end
+
+    test "lowercases a simple hostname" do
+      assert Identifier.normalize_name("MyHost") == "myhost"
+    end
+
+    test "strips trailing dot from FQDN" do
+      assert Identifier.normalize_name("example.com.") == "example.com"
+    end
+
+    test "trims whitespace" do
+      assert Identifier.normalize_name("  host.name  ") == "host.name"
+    end
+
+    test "returns nil for empty name" do
+      assert Identifier.normalize_name("") == nil
+    end
+
+    test "returns nil for whitespace-only name" do
+      assert Identifier.normalize_name("   ") == nil
+    end
+
+    test "returns nil for non-string input" do
+      assert Identifier.normalize_name(42) == nil
+    end
+  end
+
+  describe "candidate_names/1" do
+    test "returns full name and short name for FQDN" do
+      result = Identifier.candidate_names("switch01.dc1.example.com")
+      assert "switch01.dc1.example.com" in result
+      assert "switch01" in result
+    end
+
+    test "returns only full name for simple hostname" do
+      assert Identifier.candidate_names("myhost") == ["myhost"]
+    end
+
+    test "returns empty list for nil" do
+      assert Identifier.candidate_names(nil) == []
+    end
+
+    test "returns empty list for empty string" do
+      assert Identifier.candidate_names("") == []
+    end
+  end
+end

test/towerops_web/live/agent_live/helpers_test.exs

@@ -188,4 +188,23 @@ defmodule ToweropsWeb.AgentLive.HelpersTest do
       assert nil == Helpers.format_agent_version(123)
     end
   end
+
+  describe "format_last_seen_with_date/2" do
+    test "returns Never for nil datetime" do
+      assert "Never" = Helpers.format_last_seen_with_date(nil)
+    end
+
+    test "returns Never for nil datetime with timezone" do
+      assert "Never" = Helpers.format_last_seen_with_date(nil, "America/New_York")
+    end
+
+    test "formats relative time and full datetime for recent time" do
+      dt = DateTime.add(DateTime.utc_now(), -30, :second)
+
+      result = Helpers.format_last_seen_with_date(dt, "Etc/UTC")
+      assert result =~ "s ago"
+      assert result =~ "("
+      assert result =~ ")"
+    end
+  end
 end

test/towerops_web/live/maintenance_live/helpers_test.exs

@@ -20,11 +20,37 @@ defmodule ToweropsWeb.MaintenanceLive.HelpersTest do
     end
   end
 
+  describe "status_label/1" do
+    test "returns Active for :active status" do
+      assert Helpers.status_label(:active) == "Active"
+    end
+
+    test "returns Upcoming for :upcoming status" do
+      assert Helpers.status_label(:upcoming) == "Upcoming"
+    end
+
+    test "returns Past for :past status" do
+      assert Helpers.status_label(:past) == "Past"
+    end
+  end
+
   describe "format_datetime/2" do
     test "formats the datetime with the provided timezone" do
       datetime = ~U[2026-01-15 14:34:00Z]
 
       assert Helpers.format_datetime(datetime, "Etc/UTC") == "Jan 15, 2026 02:34 PM"
     end
+
+    test "falls back to UTC when timezone is nil" do
+      datetime = ~U[2026-01-15 14:34:00Z]
+
+      assert Helpers.format_datetime(datetime, nil) == "Jan 15, 2026 02:34 PM"
+    end
+
+    test "falls back to UTC formatting for invalid timezone" do
+      datetime = ~U[2026-01-15 14:34:00Z]
+
+      assert Helpers.format_datetime(datetime, "Invalid/Zone") =~ "UTC"
+    end
   end
 end

test/towerops_web/plugs/filter_noisy_logs_test.exs

@@ -0,0 +1,61 @@
+defmodule ToweropsWeb.Plugs.FilterNoisyLogsTest do
+  use ToweropsWeb.ConnCase, async: true
+
+  alias ToweropsWeb.Plugs.FilterNoisyLogs
+
+  describe "call/2" do
+    test "filters GET /health requests", %{conn: conn} do
+      conn = %{conn | path_info: ["health"], method: "GET"}
+      conn = FilterNoisyLogs.call(conn, [])
+
+      assert conn.private[:phoenix_log] == false
+      assert conn.private[:log] == false
+      assert conn.private[:plug_skip_telemetry] == true
+    end
+
+    test "filters GET /health/time requests", %{conn: conn} do
+      conn = %{conn | path_info: ["health", "time"], method: "GET"}
+      conn = FilterNoisyLogs.call(conn, [])
+
+      assert conn.private[:phoenix_log] == false
+      assert conn.private[:log] == false
+      assert conn.private[:plug_skip_telemetry] == true
+    end
+
+    test "filters HEAD / requests", %{conn: conn} do
+      conn = %{conn | path_info: [], method: "HEAD"}
+      conn = FilterNoisyLogs.call(conn, [])
+
+      assert conn.private[:phoenix_log] == false
+      assert conn.private[:log] == false
+      assert conn.private[:plug_skip_telemetry] == true
+    end
+
+    test "does not filter regular GET requests", %{conn: conn} do
+      conn = %{conn | path_info: ["devices"], method: "GET"}
+      conn = FilterNoisyLogs.call(conn, [])
+
+      refute conn.private[:phoenix_log] == false
+    end
+
+    test "does not filter POST /health requests", %{conn: conn} do
+      conn = %{conn | path_info: ["health"], method: "POST"}
+      conn = FilterNoisyLogs.call(conn, [])
+
+      refute conn.private[:phoenix_log] == false
+    end
+
+    test "does not filter GET requests with different paths", %{conn: conn} do
+      conn = %{conn | path_info: ["api", "v1", "status"], method: "GET"}
+      conn = FilterNoisyLogs.call(conn, [])
+
+      refute conn.private[:phoenix_log] == false
+    end
+  end
+
+  describe "init/1" do
+    test "passes through opts unchanged" do
+      assert FilterNoisyLogs.init(:some_opts) == :some_opts
+    end
+  end
+end

test/towerops_web/plugs/graphql_introspection_test.exs

@@ -0,0 +1,76 @@
+defmodule ToweropsWeb.Plugs.GraphQLIntrospectionTest do
+  use ToweropsWeb.ConnCase, async: true
+
+  alias ToweropsWeb.Plugs.GraphQLIntrospection
+
+  describe "call/2 in production" do
+    setup do
+      Application.put_env(:towerops, :env, :prod)
+      on_exit(fn -> Application.put_env(:towerops, :env, :test) end)
+      :ok
+    end
+
+    test "blocks introspection queries containing __schema", %{conn: conn} do
+      conn =
+        conn
+        |> Map.put(:body_params, %{"query" => "{ __schema { types { name } } }"})
+        |> GraphQLIntrospection.call([])
+
+      assert conn.halted
+      assert conn.status == 400
+      assert conn.resp_body =~ "Introspection is disabled"
+    end
+
+    test "blocks introspection queries containing __type", %{conn: conn} do
+      conn =
+        conn
+        |> Map.put(:body_params, %{"query" => "{ __type(name: \"User\") { name } }"})
+        |> GraphQLIntrospection.call([])
+
+      assert conn.halted
+      assert conn.status == 400
+    end
+
+    test "allows normal queries", %{conn: conn} do
+      conn =
+        conn
+        |> Map.put(:body_params, %{"query" => "{ devices { id name } }"})
+        |> GraphQLIntrospection.call([])
+
+      refute conn.halted
+      refute conn.status == 400
+    end
+
+    test "allows requests with no body_params", %{conn: conn} do
+      conn = GraphQLIntrospection.call(conn, [])
+
+      refute conn.halted
+    end
+
+    test "allows requests with empty query", %{conn: conn} do
+      conn =
+        conn
+        |> Map.put(:body_params, %{"query" => ""})
+        |> GraphQLIntrospection.call([])
+
+      refute conn.halted
+    end
+  end
+
+  describe "call/2 in non-production" do
+    test "allows introspection queries in test env", %{conn: conn} do
+      conn =
+        conn
+        |> Map.put(:body_params, %{"query" => "{ __schema { types { name } } }"})
+        |> GraphQLIntrospection.call([])
+
+      refute conn.halted
+    end
+  end
+
+  describe "init/1" do
+    test "passes through opts unchanged" do
+      assert GraphQLIntrospection.init(:some_opts) == :some_opts
+    end
+  end
+end

test/towerops_web/telemetry_test.exs

@@ -1,6 +1,8 @@
 defmodule ToweropsWeb.TelemetryTest do
   use ExUnit.Case, async: true
 
+  import ExUnit.CaptureLog
+
   alias ToweropsWeb.Telemetry
 
   describe "parse_redis_info/1" do
@@ -50,4 +52,159 @@ defmodule ToweropsWeb.TelemetryTest do
       refute Enum.empty?(metrics)
     end
   end
+
+  describe "handle_router_exception/4" do
+    test "logs an error with exception metadata" do
+      conn = %Plug.Conn{
+        method: "GET",
+        request_path: "/api/devices",
+        assigns: %{request_id: "req-123"}
+      }
+
+      metadata = %{
+        conn: conn,
+        kind: :error,
+        reason: "something went wrong",
+        stacktrace: [],
+        plug: MyTestPlug
+      }
+
+      log =
+        capture_log(fn ->
+          Telemetry.handle_router_exception(:event, %{}, metadata, nil)
+        end)
+
+      assert log =~ "Router exception on Elixir.MyTestPlug GET /api/devices"
+      assert log =~ "[error]"
+    end
+  end
+
+  describe "handle_endpoint_stop/4" do
+    setup do
+      previous = Logger.level()
+      Logger.configure(level: :warning)
+      on_exit(fn -> Logger.configure(level: previous) end)
+      :ok
+    end
+
+    test "logs warning for slow requests over 5 seconds" do
+      conn = %Plug.Conn{
+        method: "POST",
+        request_path: "/slow-endpoint",
+        status: 200,
+        assigns: %{}
+      }
+
+      # Duration > 5000ms in native units
+      duration_native = System.convert_time_unit(6_000, :millisecond, :native)
+
+      log =
+        capture_log(fn ->
+          Telemetry.handle_endpoint_stop(
+            :stop,
+            %{duration: duration_native},
+            %{conn: conn},
+            nil
+          )
+        end)
+
+      assert log =~ "Slow request: POST /slow-endpoint"
+      assert log =~ "6000ms"
+      assert log =~ "[warning]"
+    end
+
+    test "logs error for 5xx status codes" do
+      conn = %Plug.Conn{
+        method: "GET",
+        request_path: "/api/broken",
+        status: 500,
+        assigns: %{request_id: "req-456"}
+      }
+
+      duration_native = System.convert_time_unit(100, :millisecond, :native)
+
+      log =
+        capture_log(fn ->
+          Telemetry.handle_endpoint_stop(
+            :stop,
+            %{duration: duration_native},
+            %{conn: conn},
+            nil
+          )
+        end)
+
+      assert log =~ "Server error: GET /api/broken returned 500"
+      assert log =~ "[error]"
+    end
+
+    test "does not log for fast 2xx requests" do
+      conn = %Plug.Conn{
+        method: "GET",
+        request_path: "/fast-endpoint",
+        status: 200,
+        assigns: %{}
+      }
+
+      duration_native = System.convert_time_unit(50, :millisecond, :native)
+
+      log =
+        capture_log(fn ->
+          Telemetry.handle_endpoint_stop(
+            :stop,
+            %{duration: duration_native},
+            %{conn: conn},
+            nil
+          )
+        end)
+
+      assert log == ""
+    end
+
+    test "logs both warning and error for slow 5xx request" do
+      conn = %Plug.Conn{
+        method: "DELETE",
+        request_path: "/api/resource",
+        status: 503,
+        assigns: %{request_id: "req-789"}
+      }
+
+      duration_native = System.convert_time_unit(7_000, :millisecond, :native)
+
+      log =
+        capture_log(fn ->
+          Telemetry.handle_endpoint_stop(
+            :stop,
+            %{duration: duration_native},
+            %{conn: conn},
+            nil
+          )
+        end)
+
+      assert log =~ "Slow request: DELETE /api/resource"
+      assert log =~ "Server error: DELETE /api/resource returned 503"
+    end
+
+    test "does not log for fast 4xx requests" do
+      conn = %Plug.Conn{
+        method: "GET",
+        request_path: "/not-found",
+        status: 404,
+        assigns: %{}
+      }
+
+      duration_native = System.convert_time_unit(100, :millisecond, :native)
+
+      log =
+        capture_log(fn ->
+          Telemetry.handle_endpoint_stop(
+            :stop,
+            %{duration: duration_native},
+            %{conn: conn},
+            nil
+          )
+        end)
+
+      assert log == ""
+    end
+  end
 end