Refactor Develop Continued

[tsmarvin]

Description

Continued refactor of the in progress updates to PR#274

[Copilot]

Pull request overview

Continues the refactor work from PR#274 by updating deployment/docs for the new worker/Redis-based architecture, modernizing container startup/build tooling, and expanding automated tests for newly introduced components.

Changes:

• Updated deployment/configuration docs for installation scripts, Redis, ATProto lexicons, and the Discord worker architecture.
• Refactored container build + entrypoint flow to publish multiple projects, run the AppHost, and add Redis to docker-compose.
• Added substantial unit/integration test coverage and new test infrastructure (shared Redis Testcontainer, antiforgery helpers, controller/service tests).

Reviewed changes

Copilot reviewed 75 out of 397 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
docs/DISCORD_BOT.md	Documents Discord worker architecture separation/scaling concept.
docs/DEPLOYMENT.md	Adds install-script quick start and updates sharding/deployment guidance.
docs/CONFIGURATION.md	Documents Redis + Discord worker settings and updates appsettings examples.
docs/ATPROTO_LEXICON.md	Expands lexicon docs to include playlist lexicon and updated paths.
containers/setup-secrets.sh	Removes old secrets setup script in favor of new installation approach.
containers/entrypoint.sh	Refactors startup to generate appsettings, configure Aspire dashboard, Redis, and run AppHost.
containers/docker-compose.yml	Adds Redis service, new secret mounts, DP key persistence, and dependency wiring.
containers/Test-LockFileChanges.ps1	New lock-file diff validator intended for cross-platform restore/publish consistency.
containers/Dockerfile.caddy	Updates pinned Caddy base image digests.
containers/Dockerfile	Reworks container build to publish all projects and include Aspire tools/dashboard.
containers/Caddyfile	Proxies Aspire Dashboard to in-container dashboard port rather than separate container.
containers/Build-Publish.ps1	New publish orchestrator script to standardize output layout and lock regeneration.
containers/.env.example	Updates env example to remove SQLite cache config and document required secrets.
Tests/Unit/WellKnownControllerTests.cs	Adds unit tests for ATProto OAuth well-known endpoints.
Tests/Unit/StatisticsControllerTests.cs	Adds controller unit tests for stats rendering/refresh behavior.
Tests/Unit/SpotifyArtistGenreServiceTests.cs	Adds constructor validation tests for Spotify genre service.
Tests/Unit/RetryAfterExceededExceptionTests.cs	Adds tests for rate-limit exception behavior and provider detection.
Tests/Unit/RecordKeyGeneratorTests.cs	Updates tests to new namespaces and MSTest assertions, adds XML docs.
Tests/Unit/QrCodeServiceTests.cs	Adds unit tests for QR code data URI generation.
Tests/Unit/OpenGraphExtensionsTests.cs	Updates namespace and test naming for theme-color expectations.
Tests/Unit/OpenGraphCardServiceTests.cs	Updates namespace and expands card service constructor checks.
Tests/Unit/LookupStatisticsTests.cs	Adds DTO default/initializer behavior tests.
Tests/Unit/LoggingConfigurationTests.cs	Updates logging config tests to LogDirPath and adds documentation.
Tests/Unit/HashUtilityTests.cs	Adds tests for hashing utilities and URL normalization.
Tests/Unit/DataProtectionLookupProtectorTests.cs	Adds tests for DP-based lookup protection roundtrips and failure modes.
Tests/Unit/DataProtectionKeyRingTests.cs	Adds tests for DP key ring behavior.
Tests/Unit/ConnectionStringCompatibilityTests.cs	Removes tests tied to the removed SQLite link-cache connection string.
Tests/Unit/ConfigurationValidationTests.cs	Updates tests to reflect removed link-cache connection string usage.
Tests/Unit/CacheBootstrapBackgroundServiceTests.cs	Adds background service tests for ATProto→cache bootstrap logic.
Tests/Unit/AppleMusicLinkParserTests.cs	Adds coverage for Apple Music URL parsing edge cases.
Tests/Unit/AppleJwtHandlerTests.cs	Updates namespace and adds more documentation around JWT generation tests.
Tests/Unit/AppSettingsTests.cs	Updates binding tests for renamed logging setting and removed settings.
Tests/Unit/ATProtoSigningKeyProviderTests.cs	Adds extensive JWKS/JWK key provider tests.
Tests/TestUtilities.cs	Removes old custom factory utilities replaced by new infra.
Tests/TestConfiguration.cs	Adds shared config loader and helper for Aspire parameter args.
Tests/SharedTestInfrastructure.cs	Adds shared Redis Testcontainer lifecycle for integration tests.
Tests/RateLimitTestHelper.cs	Adds helper to mark tests inconclusive when rate limited.
Tests/Integration/RedisRequestDeduplicatorTests.cs	Adds Redis integration tests for request de-dup + pub/sub completion.
Tests/Integration/RedisRateLimitTrackerTests.cs	Adds Redis integration tests for rate limit state/TTL behavior.
Tests/Integration/QueueMetricsIntegrationTests.cs	Adds integration tests validating observable gauge metrics from Redis streams.
Tests/Integration/HealthEndpointAuthorizationTests.cs	Re-enables health endpoint checks and updates config overrides.
Tests/Integration/DashboardAuthorizationTests.cs	Re-enables dashboard auth tests with a test auth scheme/factory.
Tests/EndToEnd/OpenGraphCardControllerTests.cs	Updates E2E tests to direct-provider mode + improved rate-limit handling.
Tests/EndToEnd/MusicLookupControllerTests.cs	Uses antiforgery helper and adjusts multi-URL test expectations.
Tests/EndToEnd/HomeControllerTests.cs	Adds antiforgery handling for POST endpoints and expands docs.
Tests/CustomWebApplicationFactory.cs	New factory wiring Redis + direct-mode overrides for WebApplicationFactory tests.
Tests/BridgeBeats.Tests.csproj	Adds Testcontainers/Aspire testing deps, removes FluentAssertions, tightens build settings.
Tests/AntiforgeryTestHelper.cs	Adds helper for antiforgery token acquisition and POST wrapper.
.github/workflows/tests.yml	Switches restore workflow to RID-specific restore + lock validation script.
.github/workflows/publish-caddy-cloudflare.yml	Updates pinned docker/login-action version SHA.
.github/workflows/docs.yml	Updates setup-dotnet action SHA.
.github/workflows/docker-publish.yml	Updates pinned docker/login-action version SHA.
.editorconfig	Tightens analyzer severities and adds additional analyzer config.

[Copilot]

The Dockerfile uses $APP_UID for chown and USER selection, but this diff removed the earlier USER $APP_UID and does not define ARG/ENV APP_UID in the shown stages. If APP_UID is unset at build time, these lines can fail (or run as an empty user). Define APP_UID explicitly (e.g., ARG with a default + matching user creation) or switch to a known existing user.

[Copilot]

The assertion is reversed: suggestedRetryTime should be later than now when retryAfterValue is positive. This test will fail as written; assert that suggestedRetryTime is greater than now.

Suggested change

	Assert.IsGreaterThan(now, suggestedRetryTime);
	Assert.IsGreaterThan(suggestedRetryTime, now);

[Copilot]

The script defines LOG_DIR_PATH but always creates /app/data/logs instead of "$LOG_DIR_PATH". If LOG_DIR_PATH is overridden, the target directory may not exist and file logging can fail. Create the directory using the configured LOG_DIR_PATH.

Suggested change

	LOG_DIR_PATH="${LOG_DIR_PATH:-/app/data/logs}"
	LOG_DIR_PATH="${LOG_DIR_PATH:-/app/data/logs}"
	mkdir -p "$LOG_DIR_PATH"

[Copilot]

This section mixes BridgeBeats__NodeNumber (used in the example) with NODE_NUMBER (called out as the uniqueness requirement). Pick one configuration name and use it consistently throughout the sharding docs (including the unique-shard warning) to avoid misconfiguration.

[Copilot]

This section mixes BridgeBeats__NodeNumber (used in the example) with NODE_NUMBER (called out as the uniqueness requirement). Pick one configuration name and use it consistently throughout the sharding docs (including the unique-shard warning) to avoid misconfiguration.

[Copilot]

This section mixes BridgeBeats__NodeNumber (used in the example) with NODE_NUMBER (called out as the uniqueness requirement). Pick one configuration name and use it consistently throughout the sharding docs (including the unique-shard warning) to avoid misconfiguration.

Suggested change

	**Important**: Each shard must have a unique `NODE_NUMBER`. Running multiple instances with the same node number will cause duplicate responses to the same messages.
	**Important**: Each shard must have a unique `BridgeBeats__NodeNumber`. Running multiple instances with the same node number will cause duplicate responses to the same messages.

[Copilot]

Comparing ConvertTo-Json output from hashtables can be unstable because hashtable key enumeration order isn’t guaranteed, which can cause false positives when the underlying data is the same. A more robust approach is to compare specific fields (e.g., resolved/type/contentHash) or parse into ordered objects (avoid -AsHashtable) and canonicalize/sort keys before comparison.

[Copilot]

Comparing ConvertTo-Json output from hashtables can be unstable because hashtable key enumeration order isn’t guaranteed, which can cause false positives when the underlying data is the same. A more robust approach is to compare specific fields (e.g., resolved/type/contentHash) or parse into ordered objects (avoid -AsHashtable) and canonicalize/sort keys before comparison.

[Copilot]

Pull request overview

Copilot reviewed 75 out of 397 changed files in this pull request and generated 9 comments.

[Copilot]

The mcr.microsoft.com/dotnet/sdk base image typically doesn't include PowerShell (pwsh). This RUN pwsh ... step will fail during Docker build unless PowerShell is explicitly installed. Consider either installing PowerShell in the build stage (apt-get) or rewriting the publish pipeline in bash/dotnet so it runs without pwsh.

[Copilot]

These Assert.IsGreaterThan(...) calls have the expected/actual arguments reversed, which will make the tests fail (e.g., you want result.Length > 100, largeResult.Length > smallResult.Length, and bytes.Length > 8). Swap the argument order (or use a boolean assertion) so the comparisons reflect the intended expectations.

[Copilot]

These Assert.IsGreaterThan(...) calls have the expected/actual arguments reversed, which will make the tests fail (e.g., you want result.Length > 100, largeResult.Length > smallResult.Length, and bytes.Length > 8). Swap the argument order (or use a boolean assertion) so the comparisons reflect the intended expectations.

[Copilot]

These Assert.IsGreaterThan(...) calls have the expected/actual arguments reversed, which will make the tests fail (e.g., you want result.Length > 100, largeResult.Length > smallResult.Length, and bytes.Length > 8). Swap the argument order (or use a boolean assertion) so the comparisons reflect the intended expectations.

Suggested change

	Assert.IsGreaterThan( 8, bytes.Length, "PNG file should have more than 8 bytes" );
	Assert.IsGreaterThan( bytes.Length, 8, "PNG file should have more than 8 bytes" );

[Copilot]

This section mixes BridgeBeats__NodeNumber (in the examples) with NODE_NUMBER (in the Important note). Align on a single variable name (and match docs/CONFIGURATION.md) so operators don’t configure the wrong setting.

[Copilot]

The compose default NODE_NUMBER is 100, while the documentation/config references indicate a default of 0. If NODE_NUMBER is still meaningful (e.g., sharding), consider making the compose default consistent with the documented default or add a brief note explaining why 100 is chosen here.

Suggested change

	- NODE_NUMBER=${NODE_NUMBER:-100}
	- NODE_NUMBER=${NODE_NUMBER:-0}

[Copilot]

Using .ToDictionary() without selectors is unusual and may not compile unless you’re relying on a very specific framework/library overload. For clarity and compatibility, prefer an explicit form like .ToDictionary(kv => kv.Key, kv => kv.Value) (this pattern appears in multiple test files in the PR).

Suggested change

	.ToDictionary( );
	.ToDictionary( kv => kv.Key, kv => kv.Value );

[Copilot]

The lock validation script is being called without -FromPlatform, so it assumes the backup lock file was also linux-x64. If a checked-in lock file uses a different RID, this can produce false failures. Consider detecting the original RID per lock file (similar to Build-Publish.ps1) and passing it as -FromPlatform in the workflow.

Suggested change

	pwsh containers/Test-LockFileChanges.ps1 -BackupPath "$backup" -CurrentPath "$lockfile" -ToPlatform "linux-x64"
	fromRid=$(grep -m1 '"runtimeIdentifier"' "$backup" | sed -E 's/.*"runtimeIdentifier"[[:space:]]*:[[:space:]]*"([^"]*)".*/\1/')
	if [ -z "$fromRid" ]; then
	fromRid="linux-x64"
	fi
	pwsh containers/Test-LockFileChanges.ps1 -BackupPath "$backup" -CurrentPath "$lockfile" -FromPlatform "$fromRid" -ToPlatform "linux-x64"

[Copilot]

LOG_DIR_PATH is configurable, but directory creation is hardcoded to /app/data/logs. Create the directory using $LOG_DIR_PATH (and optionally also create /app/data/logs for backward compatibility) so non-default log paths work reliably.

Suggested change

	# Ensure the configured log directory exists (and keep /app/data/logs for backward compatibility)
	if [ ! -d "$LOG_DIR_PATH" ]; then
	mkdir -p "$LOG_DIR_PATH"
	fi
	if [ "$LOG_DIR_PATH" != "/app/data/logs" ]; then
	mkdir -p "/app/data/logs"
	fi

[Copilot]

Pull request overview

Copilot reviewed 75 out of 397 changed files in this pull request and generated 7 comments.

Comments suppressed due to low confidence (1)

Tests/Unit/HashUtilityTests.cs:1

• This likely asserts the opposite of what the message says (it reads as “8 is greater than bytes.Length”). To match “PNG file should have more than 8 bytes”, the assertion should validate bytes.Length > 8.

using BridgeBeats.Core.Infrastructure.Utilities;

[Copilot]

LOG_DIR_PATH is configurable, but the script always creates /app/data/logs instead of creating the directory specified by $LOG_DIR_PATH. If LOG_DIR_PATH is set to a different path, log writing can fail. Create "$LOG_DIR_PATH" (and consider also keeping /app/data/logs only if it’s still required elsewhere).

Suggested change

	# 0) Create required directories
	# 0) Create required directories
	mkdir -p "$LOG_DIR_PATH"

[Copilot]

The docs instruct users to set REDIS_CONNECTION_STRING, but the container configuration/entrypoint wiring in this PR uses REDIS_HOST/REDIS_PORT (+ a Redis password secret) to construct ConnectionStrings:redis. Either document the REDIS_HOST/REDIS_PORT setup (and secret) here, or update the container entrypoint/compose to accept REDIS_CONNECTION_STRING directly so docs and deployment match.

Suggested change

	| `REDIS_CONNECTION_STRING` | Redis connection string for caching and queuing | `localhost:6379` (provided by Aspire) |
	| `REDIS_HOST` | Redis host for caching and queuing | `redis` (provided by Aspire/container) |
	| `REDIS_PORT` | Redis port for caching and queuing | `6379` |
	| `REDIS_PASSWORD` | Redis password for caching and queuing (if required) | *(empty – no auth by default)* |

[Copilot]

The docs instruct users to set REDIS_CONNECTION_STRING, but the container configuration/entrypoint wiring in this PR uses REDIS_HOST/REDIS_PORT (+ a Redis password secret) to construct ConnectionStrings:redis. Either document the REDIS_HOST/REDIS_PORT setup (and secret) here, or update the container entrypoint/compose to accept REDIS_CONNECTION_STRING directly so docs and deployment match.

Suggested change

	Redis is used for distributed caching, request queuing, and rate limit tracking. In development, Aspire provides and configures Redis automatically. In production:
	1. Deploy a Redis instance (standalone or cluster)
	2. Set `REDIS_CONNECTION_STRING` to your Redis endpoint
	3. Recommended: Use Redis with persistence (RDB or AOF) for queue durability
	4. Recommended: Enable TLS and authentication for production deployments
	Redis is used for distributed caching, request queuing, and rate limit tracking. In development, Aspire provides and configures Redis automatically. In production (containerized deployment):
	1. Deploy a Redis instance (standalone or cluster).
	2. Configure the BridgeBeats container with:
	- `REDIS_HOST` – the hostname or service name of your Redis instance
	- `REDIS_PORT` – the TCP port for Redis (usually `6379`)
	- A Redis password provided via environment variable or secret (for example, `REDIS_PASSWORD`), which the container entrypoint uses together with `REDIS_HOST`/`REDIS_PORT` to build `ConnectionStrings:redis`
	3. Recommended: Use Redis with persistence (RDB or AOF) for queue durability.
	4. Recommended: Enable TLS and authentication for production deployments.

[Copilot]

The docs instruct users to set REDIS_CONNECTION_STRING, but the container configuration/entrypoint wiring in this PR uses REDIS_HOST/REDIS_PORT (+ a Redis password secret) to construct ConnectionStrings:redis. Either document the REDIS_HOST/REDIS_PORT setup (and secret) here, or update the container entrypoint/compose to accept REDIS_CONNECTION_STRING directly so docs and deployment match.

Suggested change

	2. Set `REDIS_CONNECTION_STRING` to your Redis endpoint
	3. Recommended: Use Redis with persistence (RDB or AOF) for queue durability
	4. Recommended: Enable TLS and authentication for production deployments
	2. When running in containers (e.g., via Aspire or Docker), configure Redis using `REDIS_HOST` and `REDIS_PORT` (and the associated Redis password secret defined in your deployment). The container entrypoint combines these into `ConnectionStrings:redis` for the application.
	3. When running outside of containers, you may instead set `REDIS_CONNECTION_STRING` directly to a full Redis connection string if your hosting model prefers a single connection string value.
	4. Recommended: Use Redis with persistence (RDB or AOF) for queue durability
	5. Recommended: Enable TLS and authentication for production deployments

[Copilot]

The deployment guide promotes curl | bash, which is risky because it executes remote code without an integrity check. Consider documenting a safer flow (download to a file, review, optionally verify a checksum/signature, then execute), or at minimum call out the trust/security tradeoff explicitly.

[Copilot]

Enumerable.ToDictionary() does not have a parameterless overload in common LINQ APIs; this is likely a compile error. Use an explicit key/value selector (e.g., ToDictionary(kv => kv.Key, kv => kv.Value)) or construct a Dictionary<string, string?> from the enumeration. The same pattern appears in other updated test files in this PR (e.g., DashboardAuthorizationTests, OpenGraphCardControllerTests, MusicLookupControllerTests, HomeControllerTests) and should be corrected consistently.

[Copilot]

The comparison assertions appear to have their operands reversed relative to the intent expressed in the messages. For example, “should be substantial” implies result.Length > 100, and “larger image” implies largeResult.Length > smallResult.Length. Fix the operand ordering (or use explicit boolean assertions) so the tests validate the intended behavior.