Add Sharkey (Misskey-family) interoperability smoke tests#659
Merged
dahlia merged 3 commits intofedify-dev:mainfrom Apr 10, 2026
Merged
Add Sharkey (Misskey-family) interoperability smoke tests#659dahlia merged 3 commits intofedify-dev:mainfrom
dahlia merged 3 commits intofedify-dev:mainfrom
Conversation
|
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 a Sharkey interoperability smoke-test suite: CI workflow, Docker Compose test stack with Caddy TLS proxies, cert generation and provisioning scripts, Deno harness/orchestrator updates for async recipient resolution and robust account lookup, and related test artifacts for running smoke tests. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as "GitHub Actions\n(smoke-sharkey)"
participant Runner as "Runner (ubuntu-latest)"
participant Docker as "Docker Compose"
participant DB as "Postgres"
participant Redis as "Redis"
participant Caddy as "Caddy TLS Proxies"
participant Sharkey as "Sharkey web backend"
participant Harness as "Fedify harness (Deno)"
participant Orchestrator as "orchestrator.ts"
GH->>Runner: trigger workflow (push/cron/manual)
Runner->>Docker: generate certs, pull images, start stack
Docker->>DB: start & healthcheck
Docker->>Redis: start & healthcheck
Docker->>Sharkey: start web backend
Docker->>Caddy: start TLS proxies
Runner->>Sharkey: run provision.sh (create users/tokens/follows)
Runner->>Harness: run Deno harness tests (deno run orchestrator)
Orchestrator->>Sharkey: lookup accounts, poll inboxes, validate activities
Orchestrator->>Harness: verify harness-side expectations
Runner->>Docker: on failure collect compose logs
Runner->>Docker: teardown stack (down -v)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces smoke tests for Sharkey, including a Docker Compose environment with Caddy for TLS termination, provisioning scripts, and configuration files. The test harness has been updated to support WebFinger resolution with caching and improved account lookup logic that falls back to Sharkey-specific endpoints. Additionally, error handling was added to harness inbox fetches, and the unfollow test assertion order was refined. Feedback was provided regarding the lack of timeouts in the new fetch calls within the WebFinger resolution logic, which could cause tests to hang.
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/smoke-sharkey.yml:
- Around line 19-26: Add an explicit minimal GITHUB_TOKEN permissions block to
the workflow so it doesn't inherit repo defaults: under the top-level workflow
definition (near the existing concurrency / jobs: smoke block), add a
permissions section that grants only read access to the repository contents
(e.g., permissions: contents: read) to limit the token to the minimal scope
required by the smoke job.
In `@test/smoke/harness/backdoor.ts`:
- Around line 62-64: The fallback constructs an inboxId with the variable scheme
but hardcodes https for actorId, causing mismatched http/https URIs; update the
actorId construction (actorId) to use the same scheme variable (scheme) as
inboxId (e.g., `${scheme}://${domain}/users/${user}`) so both inboxId and
actorId share the same scheme.
In `@test/smoke/orchestrator.ts`:
- Around line 159-205: The empty catch blocks swallow all errors; change them to
catch the error (e.g., catch (err)) and only suppress expected "endpoint
unsupported" cases (HTTP 404 from serverGet or fetch) while rethrowing
everything else; specifically update the serverGet try/catch blocks around
`/api/v1/accounts/search` and `/api/v1/accounts/lookup` to inspect the caught
error (check for err.status === 404 or err instanceof Response with status 404
or the presence of "404" in err.message) and rethrow non-404 errors, and do the
same for the POST to `/api/users/show` (catch errors from fetch and only swallow
a 404/not-found network response, otherwise rethrow).
In `@test/smoke/sharkey/default.yml`:
- Around line 19-22: The allowedPrivateNetworks block currently uses overly
broad catch-alls (0.0.0.0/0 and ::/0) which effectively disables the
restriction; update the allowedPrivateNetworks entry to list only the specific
private CIDR ranges the smoke stack actually uses (replace 0.0.0.0/0 and ::/0 in
allowedPrivateNetworks) — e.g. the project’s Docker/compose subnets (like the
specific 172.x.x.x/16 or 192.168.x.x/16 ranges your smoke stack uses) and any
required internal IPv6 prefix, or better yet reference the smoke stack’s
configured subnet variables instead of wildcarding everything. Ensure loopback
(::1/128, 127.0.0.0/8) is not inadvertently allowed unless explicitly needed.
In `@test/smoke/sharkey/docker-compose.yml`:
- Line 87: Replace the inline mapping "fedify-harness-backend: { condition:
service_healthy }" with a normalized block mapping so YAML linter accepts it;
locate the depends_on entries (the "fedify-harness-backend" mapping) and change
each inline form to a two-line mapping with "fedify-harness-backend:" on one
line and an indented "condition: service_healthy" on the next, preserving
surrounding indentation and aligning with the existing depends_on structure
(also update the other occurrences noted around the same stanza).
In `@test/smoke/sharkey/provision.sh`:
- Line 19: Remove printing of raw credential-bearing responses and tokens in
provision.sh: stop echoing variables like ADMIN_RAW (and similar echoes at the
other mentioned locations) that may contain bearer tokens, passwords, or reset
links; instead log only non-sensitive status messages (e.g., "admin creation
succeeded") or a sanitized summary (no tokens, no passwords, no reset URLs), and
if you must indicate truncated content, ensure you redact token-like patterns
rather than printing the raw body. Update the echo statements that reference
ADMIN_RAW and the other echoed response variables to either omit the variable
entirely or replace it with a safe, redacted summary so no secret material
appears in CI logs.
- Around line 124-132: The follow request currently silences failures with "||
true" and stores output in FOLLOW_RAW, so failures are ignored; remove the "||
true" and instead check curl's exit/status and fail fast: run the POST to
"$SHARKEY_URL/api/following/create" into FOLLOW_RAW (keep TEST_TOKEN and
FEDIFY_USER_ID), capture curl's exit code or use curl -f, and if non-zero print
a clear error including FOLLOW_RAW and exit non-zero so the bootstrap stops on
failed follow attempts.
- Around line 134-141: The .env.test heredoc writes unescaped assignments (e.g.,
SERVER_ACCESS_TOKEN=$TOKEN) which can allow unwanted shell expansion when later
sourced; instead emit shell-safe assignments by escaping values with printf '%q'
(or similar) for each variable before writing—replace the heredoc with lines
like printf "SERVER_ACCESS_TOKEN=%q\n" "$TOKEN" (and do the same for
SERVER_BASE_URL, SERVER_INTERNAL_HOST, HARNESS_BASE_URL, HARNESS_ORIGIN) into
test/smoke/.env.test so the file contains safe, quoted/escaped assignments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a6954926-bbe5-4084-8c3c-1907704b872d
📒 Files selected for processing (11)
.github/workflows/smoke-sharkey.yml.gitignoretest/smoke/harness/backdoor.tstest/smoke/orchestrator.tstest/smoke/sharkey/Caddyfile.fedify-harnesstest/smoke/sharkey/Caddyfile.sharkeytest/smoke/sharkey/default.ymltest/smoke/sharkey/docker-compose.ymltest/smoke/sharkey/docker.envtest/smoke/sharkey/generate-certs.shtest/smoke/sharkey/provision.sh
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/smoke/harness/backdoor.ts`:
- Around line 65-70: The fallback Mastodon-convention result (inboxId, actorId
stored in result) should not be cached indefinitely: remove the
recipientCache.set(handle, result) call in the fallback branch so transient
failures don't persistently poison the cache, or alternatively implement a TTL
for fallback entries by storing a timestamp with the fallback result and
updating recipientCache lookup logic to reject/refresh entries older than the
TTL; update the code around recipientCache, handle, inboxId, actorId, and result
accordingly.
- Around line 61-63: The empty catch in the WebFinger/actor fetch block swallows
all errors; update the catch to accept an error parameter and handle expected
"not found" cases while surfacing unexpected failures (e.g., log error details
or re-throw). Specifically, in the try that performs WebFinger lookup (the block
surrounding the current empty catch in backdoor.ts), inspect the caught error
(HTTP status, network/DNS error, or JSON parse error) and only suppress when the
response indicates WebFinger not supported or 404; otherwise call the existing
logger (or console.error) with the error details or re-throw so TLS/DNS/JSON
failures are visible during smoke tests.
In `@test/smoke/orchestrator.ts`:
- Around line 158-203: The fallback chain currently ignores non-OK HTTP
responses (e.g., 5xx) and silently proceeds, so add logging for unexpected
non-OK responses in the poll handler: when checking searchRes, lookupRes and
misskeyRes, if !res.ok and res.status !== 404 log the endpoint, status and
response body (or statusText) before falling through; update the blocks around
searchRes, lookupRes and misskeyRes (the variables named searchRes, lookupRes,
misskeyRes inside the poll("Fedify user resolvable", async () => { ... })
function) to await and log response.text() or statusText for non-404 errors so
server-side errors are visible in test output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 38b521ef-c224-4475-abf4-cf6692c1d7f8
📒 Files selected for processing (2)
test/smoke/harness/backdoor.tstest/smoke/orchestrator.ts
There was a problem hiding this comment.
♻️ Duplicate comments (3)
test/smoke/sharkey/provision.sh (3)
135-141:⚠️ Potential issue | 🟠 MajorWrite
.env.testusing shell-escaped assignments.This file is sourced later; raw assignments at Line 138 can execute unintended shell expansion if values contain metacharacters. Emit escaped values (
printf %q) for each variable.Suggested patch
-cat > test/smoke/.env.test <<EOF -SERVER_BASE_URL=http://localhost:3000 -SERVER_INTERNAL_HOST=sharkey -SERVER_ACCESS_TOKEN=$TOKEN -HARNESS_BASE_URL=http://localhost:3001 -HARNESS_ORIGIN=https://fedify-harness -EOF +{ + printf 'SERVER_BASE_URL=%q\n' "http://localhost:3000" + printf 'SERVER_INTERNAL_HOST=%q\n' "sharkey" + printf 'SERVER_ACCESS_TOKEN=%q\n' "$TOKEN" + printf 'HARNESS_BASE_URL=%q\n' "http://localhost:3001" + printf 'HARNESS_ORIGIN=%q\n' "https://fedify-harness" +} > test/smoke/.env.test🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/smoke/sharkey/provision.sh` around lines 135 - 141, The here-doc that writes test/smoke/.env.test uses raw assignments which can be subject to shell expansion for values like SERVER_ACCESS_TOKEN; update the block that creates the file (the cat > test/smoke/.env.test <<EOF ... EOF fragment) to emit shell-escaped assignments by using printf '%q' for each value (e.g., printf "SERVER_BASE_URL=%q\n" "$SERVER_BASE_URL" and similarly for SERVER_INTERNAL_HOST, SERVER_ACCESS_TOKEN, HARNESS_BASE_URL, HARNESS_ORIGIN) so the file is safe to source later without unintended expansion.
34-34:⚠️ Potential issue | 🟠 MajorStop logging token material, even truncated.
Line 34, Line 86, and Line 143 still expose token prefixes in CI logs. Please log only success/failure status without any token bytes.
Suggested patch
-echo " admin token: ${ADMIN_TOKEN:0:8}..." +echo " admin token acquired" ... -echo " testuser token: ${TEST_TOKEN:0:8}..." +echo " testuser token acquired" ... -echo "✓ Provisioning complete (token: ${TOKEN:0:8}...)" +echo "✓ Provisioning complete"Also applies to: 86-86, 143-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/smoke/sharkey/provision.sh` at line 34, Remove all printing of token material (even truncated) in the provision script: stop echoing ADMIN_TOKEN and any other TOKEN variables; instead log only a non-sensitive status message like "admin token created" or "admin token set" on success/failure. Update the occurrences that echo "${ADMIN_TOKEN:0:8}" (and similar token-prefix echoes at the other occurrences) to emit a generic success/failure message and ensure no token substring is written to stdout/stderr or CI logs.
52-77:⚠️ Potential issue | 🟠 MajorDon’t swallow transport failures in signin/reset fallback calls.
Line 52, Line 61, Line 67, and Line 73 use
|| true, which masks network/runtime failures and turns them into generic auth flow failures. Keep fallback for expected auth outcomes, but let transport errors fail fast.Suggested direction
-SIGN_IN_RAW=$(curl -sf -X POST "$SHARKEY_URL/api/signin" \ +SIGN_IN_RAW=$(curl -sS -X POST "$SHARKEY_URL/api/signin" \ -H "Content-Type: application/json" \ - -d '{"username": "testuser", "password": "testpassword123"}' 2>&1) || true + -d '{"username": "testuser", "password": "testpassword123"}')Use HTTP/body checks to decide fallback, but do not blanket-ignore curl execution errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/smoke/sharkey/provision.sh` around lines 52 - 77, The curl invocations that populate SIGN_IN_RAW, TESTUSER_INFO, RESET_RAW and the second SIGN_IN_RAW are swallowing transport failures via "|| true"; change them to preserve curl's exit status (remove "|| true") and explicitly check curl's exit code after each call: if curl returns non-zero (network/transport/runtime error) exit the script/report error immediately, but if curl succeeded and the response body lacks the expected fields (e.g., jq checks on '.i', '.id', '.password'), continue with the auth fallback logic; apply this to the SIGN_IN_RAW initial call, TESTUSER_INFO, RESET_RAW, and the signin-after-reset block so only real transport failures fail fast while expected auth failures still trigger fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/smoke/sharkey/provision.sh`:
- Around line 135-141: The here-doc that writes test/smoke/.env.test uses raw
assignments which can be subject to shell expansion for values like
SERVER_ACCESS_TOKEN; update the block that creates the file (the cat >
test/smoke/.env.test <<EOF ... EOF fragment) to emit shell-escaped assignments
by using printf '%q' for each value (e.g., printf "SERVER_BASE_URL=%q\n"
"$SERVER_BASE_URL" and similarly for SERVER_INTERNAL_HOST, SERVER_ACCESS_TOKEN,
HARNESS_BASE_URL, HARNESS_ORIGIN) so the file is safe to source later without
unintended expansion.
- Line 34: Remove all printing of token material (even truncated) in the
provision script: stop echoing ADMIN_TOKEN and any other TOKEN variables;
instead log only a non-sensitive status message like "admin token created" or
"admin token set" on success/failure. Update the occurrences that echo
"${ADMIN_TOKEN:0:8}" (and similar token-prefix echoes at the other occurrences)
to emit a generic success/failure message and ensure no token substring is
written to stdout/stderr or CI logs.
- Around line 52-77: The curl invocations that populate SIGN_IN_RAW,
TESTUSER_INFO, RESET_RAW and the second SIGN_IN_RAW are swallowing transport
failures via "|| true"; change them to preserve curl's exit status (remove "||
true") and explicitly check curl's exit code after each call: if curl returns
non-zero (network/transport/runtime error) exit the script/report error
immediately, but if curl succeeded and the response body lacks the expected
fields (e.g., jq checks on '.i', '.id', '.password'), continue with the auth
fallback logic; apply this to the SIGN_IN_RAW initial call, TESTUSER_INFO,
RESET_RAW, and the signin-after-reset block so only real transport failures fail
fast while expected auth failures still trigger fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6e1c0cad-49cf-450c-b674-ed26329f6e41
📒 Files selected for processing (1)
test/smoke/sharkey/provision.sh
There was a problem hiding this comment.
♻️ Duplicate comments (5)
test/smoke/harness/backdoor.ts (1)
63-64:⚠️ Potential issue | 🟠 MajorScheme mismatch between
inboxIdandactorIdin fallback path.Line 63 constructs
inboxIdusing theschemevariable, but line 64 hardcodeshttps://foractorId. WhenSTRICT_MODEis off, this produces anhttpinbox URL paired with anhttpsactor URI, breaking non-TLS smoke stacks.Suggested fix
const inboxId = new URL(`${scheme}://${domain}/users/${user}/inbox`); - const actorId = new URL(`https://${domain}/users/${user}`); + const actorId = new URL(`${scheme}://${domain}/users/${user}`); const result = { inboxId, actorId };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/smoke/harness/backdoor.ts` around lines 63 - 64, The fallback constructs create inboxId with the variable scheme but build actorId with a hardcoded "https://", causing mismatched schemes when STRICT_MODE is off; update the actorId construction to use the same scheme variable (replace the `https://` literal with `scheme + '://'`) so both inboxId and actorId are created consistently (refer to the inboxId and actorId variables and the scheme value in backdoor.ts).test/smoke/sharkey/provision.sh (2)
135-141:⚠️ Potential issue | 🟠 MajorWrite
.env.testwith shell-escaped assignments before sourcing.
SERVER_ACCESS_TOKEN=$TOKENis emitted raw, then sourced by the workflow. Escape each value when writing to prevent unintended shell expansion.🛡️ Suggested fix
-echo "→ Writing test env..." -cat > test/smoke/.env.test <<EOF -SERVER_BASE_URL=http://localhost:3000 -SERVER_INTERNAL_HOST=sharkey -SERVER_ACCESS_TOKEN=$TOKEN -HARNESS_BASE_URL=http://localhost:3001 -HARNESS_ORIGIN=https://fedify-harness -EOF +echo "→ Writing test env..." +{ + printf 'SERVER_BASE_URL=%q\n' "http://localhost:3000" + printf 'SERVER_INTERNAL_HOST=%q\n' "sharkey" + printf 'SERVER_ACCESS_TOKEN=%q\n' "$TOKEN" + printf 'HARNESS_BASE_URL=%q\n' "http://localhost:3001" + printf 'HARNESS_ORIGIN=%q\n' "https://fedify-harness" +} > test/smoke/.env.test#!/bin/bash # Verify sourcing path and unsafe raw env emission pattern. rg -n -C2 'source test/smoke/.env.test|\. test/smoke/.env.test' .github/workflows rg -n -C3 'cat > test/smoke/.env.test|SERVER_ACCESS_TOKEN=\$TOKEN|printf .*%q' test/smoke/sharkey/provision.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/smoke/sharkey/provision.sh` around lines 135 - 141, The here-doc in provision.sh writes SERVER_ACCESS_TOKEN=$TOKEN raw into test/smoke/.env.test which will be expanded when sourced; update the block that writes the .env.test (the cat > test/smoke/.env.test <<EOF ... EOF section) to shell-escape or quote each value before emitting (e.g., use printf '%s=%q\n' or wrap values in single quotes) so SERVER_ACCESS_TOKEN and other variables are written safely (refer to the here-doc writing of SERVER_BASE_URL, SERVER_INTERNAL_HOST, SERVER_ACCESS_TOKEN, HARNESS_BASE_URL, HARNESS_ORIGIN).
34-34:⚠️ Potential issue | 🟠 MajorStop logging token material (even prefixes) in CI output.
These lines still expose token fragments in logs. Replace with non-sensitive status messages only.
🧯 Suggested fix
-echo " admin token: ${ADMIN_TOKEN:0:8}..." +echo " admin token acquired" @@ -echo " testuser token: ${TEST_TOKEN:0:8}..." +echo " testuser token acquired" @@ -echo "✓ Provisioning complete (token: ${TOKEN:0:8}...)" +echo "✓ Provisioning complete"Also applies to: 86-86, 143-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/smoke/sharkey/provision.sh` at line 34, Remove token material from CI logs by replacing any echo that prints ADMIN_TOKEN (e.g., the echo using ${ADMIN_TOKEN:0:8} at the lines shown) with a non-sensitive status message such as "admin token created" or "admin token set" and ensure the same change is applied to the other occurrences referenced (the echo statements that currently print token fragments at the other locations). Do not print any prefix or substring of secrets; if a debug mode is required, output that tokens are hidden and write the actual token only to a secure file or environment variable instead..github/workflows/smoke-sharkey.yml (1)
19-23:⚠️ Potential issue | 🟠 MajorDeclare least-privilege
GITHUB_TOKENpermissions explicitly.This workflow currently inherits default token permissions. Pin it to read-only repo contents at workflow level.
🔐 Suggested change
concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true +permissions: + contents: read + jobs: smoke:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/smoke-sharkey.yml around lines 19 - 23, The workflow currently uses default token permissions; explicitly scope GITHUB_TOKEN to least privilege by adding a top-level permissions mapping with "contents: read" (i.e., set permissions: contents: read) so the workflow (including jobs and steps) only has read-only access to repo contents rather than broader default permissions; update the workflow YAML near the top-level (alongside concurrency/group and before jobs) to include this permissions entry referencing GITHUB_TOKEN.test/smoke/sharkey/docker-compose.yml (1)
87-87:⚠️ Potential issue | 🟡 MinorNormalize inline
depends_onmappings to pass YAMLlint.These mappings still include spaces inside
{}and will keep the lint gate failing.🧹 Suggested fix
- fedify-harness-backend: { condition: service_healthy } + fedify-harness-backend: {condition: service_healthy} @@ - db: { condition: service_healthy } - redis: { condition: service_healthy } + db: {condition: service_healthy} + redis: {condition: service_healthy} @@ - sharkey-web-backend: { condition: service_healthy } + sharkey-web-backend: {condition: service_healthy}Also applies to: 110-111, 134-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/smoke/sharkey/docker-compose.yml` at line 87, Normalize the inline depends_on YAML mappings by removing spaces inside the curly braces so they conform to YAMLlint; specifically update the entries like the "fedify-harness-backend: { condition: service_healthy }" (and the other duplicate occurrences) to use a compact inline mapping (e.g., {condition: service_healthy}) or convert them to the equivalent block mapping under depends_on, ensuring the key "fedify-harness-backend" and its mapping use the corrected format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/smoke-sharkey.yml:
- Around line 19-23: The workflow currently uses default token permissions;
explicitly scope GITHUB_TOKEN to least privilege by adding a top-level
permissions mapping with "contents: read" (i.e., set permissions: contents:
read) so the workflow (including jobs and steps) only has read-only access to
repo contents rather than broader default permissions; update the workflow YAML
near the top-level (alongside concurrency/group and before jobs) to include this
permissions entry referencing GITHUB_TOKEN.
In `@test/smoke/harness/backdoor.ts`:
- Around line 63-64: The fallback constructs create inboxId with the variable
scheme but build actorId with a hardcoded "https://", causing mismatched schemes
when STRICT_MODE is off; update the actorId construction to use the same scheme
variable (replace the `https://` literal with `scheme + '://'`) so both inboxId
and actorId are created consistently (refer to the inboxId and actorId variables
and the scheme value in backdoor.ts).
In `@test/smoke/sharkey/docker-compose.yml`:
- Line 87: Normalize the inline depends_on YAML mappings by removing spaces
inside the curly braces so they conform to YAMLlint; specifically update the
entries like the "fedify-harness-backend: { condition: service_healthy }" (and
the other duplicate occurrences) to use a compact inline mapping (e.g.,
{condition: service_healthy}) or convert them to the equivalent block mapping
under depends_on, ensuring the key "fedify-harness-backend" and its mapping use
the corrected format.
In `@test/smoke/sharkey/provision.sh`:
- Around line 135-141: The here-doc in provision.sh writes
SERVER_ACCESS_TOKEN=$TOKEN raw into test/smoke/.env.test which will be expanded
when sourced; update the block that writes the .env.test (the cat >
test/smoke/.env.test <<EOF ... EOF section) to shell-escape or quote each value
before emitting (e.g., use printf '%s=%q\n' or wrap values in single quotes) so
SERVER_ACCESS_TOKEN and other variables are written safely (refer to the
here-doc writing of SERVER_BASE_URL, SERVER_INTERNAL_HOST, SERVER_ACCESS_TOKEN,
HARNESS_BASE_URL, HARNESS_ORIGIN).
- Line 34: Remove token material from CI logs by replacing any echo that prints
ADMIN_TOKEN (e.g., the echo using ${ADMIN_TOKEN:0:8} at the lines shown) with a
non-sensitive status message such as "admin token created" or "admin token set"
and ensure the same change is applied to the other occurrences referenced (the
echo statements that currently print token fragments at the other locations). Do
not print any prefix or substring of secrets; if a debug mode is required,
output that tokens are hidden and write the actual token only to a secure file
or environment variable instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5291212d-9711-4f3b-ad4d-defdce64f284
📒 Files selected for processing (11)
.github/workflows/smoke-sharkey.yml.gitignoretest/smoke/harness/backdoor.tstest/smoke/orchestrator.tstest/smoke/sharkey/Caddyfile.fedify-harnesstest/smoke/sharkey/Caddyfile.sharkeytest/smoke/sharkey/default.ymltest/smoke/sharkey/docker-compose.ymltest/smoke/sharkey/docker.envtest/smoke/sharkey/generate-certs.shtest/smoke/sharkey/provision.sh
Adds interoperability smoke tests for Sharkey, a Misskey fork. Sharkey
requires HTTPS for all ActivityPub federation lookups (hard-coded in
checkHttps() and WebFingerService), so the setup uses Caddy TLS proxies
with self-signed certificates.
Key changes:
- New GitHub Actions workflow (smoke-sharkey) that boots a Sharkey
stack with Caddy TLS proxies, provisions users, and runs E2E
scenarios via the shared orchestrator
- Test harness backdoor now resolves recipients via WebFinger + actor
document fetch instead of hardcoding Mastodon URL patterns, with a
cache that is cleared on /_test/reset
- Orchestrator account lookup falls back through Mastodon-compat
search, lookup, and Misskey-native /api/users/show endpoints
- res.ok checks before JSON parsing in inbox poll helpers
- poll() catches transient errors from callbacks and retries until
timeout
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace raw JSON response echoes with safe ok/failed status messages to avoid logging unnecessary API internals. Make the follow request fail the script immediately on error instead of silently continuing with `|| true`, which would cause confusing downstream test failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
test/smoke/harness/backdoor.ts (2)
62-66:⚠️ Potential issue | 🟠 MajorKeep
actorIdon the same scheme asinboxIdin the fallback path.When
STRICT_MODEis off, this builds anhttpinbox and anhttpsactor for the same account, which breaks plain-HTTP stacks whenever resolution falls back to the URL convention.💡 Proposed fix
const inboxId = new URL(`${scheme}://${domain}/users/${user}/inbox`); - const actorId = new URL(`https://${domain}/users/${user}`); + const actorId = new URL(`${scheme}://${domain}/users/${user}`); const result = { inboxId, actorId };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/smoke/harness/backdoor.ts` around lines 62 - 66, Fallback constructs inboxId with variable scheme but creates actorId always with https; change actorId to use the same scheme variable so both URLs share scheme (use scheme, domain, user to build actorId), then store the pair in recipientCache as before (symbols: inboxId, actorId, recipientCache, handle, scheme, domain, user).
58-60:⚠️ Potential issue | 🟡 MinorSurface unexpected WebFinger failures instead of silently falling back.
404/unsupported cases already flow throughwfRes.ok === false; anything reaching thiscatchis an unexpected transport/TLS/parse failure. Swallowing it makes smoke-test failures much harder to diagnose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/smoke/harness/backdoor.ts` around lines 58 - 60, The catch block in backdoor.ts that currently swallows WebFinger errors should surface unexpected transport/TLS/parse failures instead of silently falling back: change the bare catch to catch (err) { throw err } or at minimum log the error (e.g., processLogger.error or console.error with the error and context) and then rethrow so callers of the WebFinger lookup (the code around the try/catch handling wfRes.ok) will see the real failure rather than proceeding to the Mastodon fallback.test/smoke/sharkey/default.yml (1)
19-22:⚠️ Potential issue | 🟠 Major
allowedPrivateNetworksis effectively unrestricted here.
0.0.0.0/0and::/0admit every address, so this no longer limits Sharkey to Docker-internal targets. Replace these catch-alls with the specific private subnets the smoke stack actually needs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/smoke/sharkey/default.yml` around lines 19 - 22, The allowedPrivateNetworks entry currently uses catch-all CIDRs (0.0.0.0/0 and ::/0) which removes any restriction; replace those with the specific private network CIDRs required by the smoke stack (e.g., IPv4 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16 and the appropriate IPv6 ULA range like fc00::/7 or the actual internal prefixes in use) by updating the allowedPrivateNetworks list so Sharkey only permits the intended Docker-internal targets.test/smoke/orchestrator.ts (1)
158-205:⚠️ Potential issue | 🟠 MajorDo not treat every lookup failure as an unsupported endpoint.
The empty
catchblocks aroundsearch/lookup, plus the silent non-okpath for/api/users/show, turn auth failures, 5xxs, and transport errors intonull. That makespoll()burn the full timeout without surfacing the real cause. Only fall through on explicit “unsupported/not found” responses; rethrow or log everything else.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/smoke/orchestrator.ts` around lines 158 - 205, The try/catch blocks treating all failures as "unsupported" must be tightened: in the serverGet attempts (the first search block and the lookup block using serverGet) only swallow/continue on explicit not-supported/not-found responses (e.g., response status 404/410 or other explicit endpoint-missing indicators returned by serverGet); for any other error (network error, 401/403 auth, 5xx) rethrow or surface it (or call processLogger.error with the error and context) so poll() fails fast. Likewise, for the Misskey POST to `${SERVER_URL}/api/users/show` check res.ok and only fall through when res.status indicates not-found/unsupported (e.g., 404); for non-ok statuses that are not "unsupported" (401/403/5xx) throw or log the response status and body. Update the catch handlers to inspect the caught error (or the serverGet response) instead of always swallowing, and include the handle/SERVER_URL and error details in the log when rethrowing or logging..github/workflows/smoke-sharkey.yml (1)
19-24:⚠️ Potential issue | 🟠 MajorAdd explicit minimal
GITHUB_TOKENpermissions.This workflow still inherits default token scopes. Pin to least privilege (
contents: read) at the workflow top level.🔐 Suggested fix
concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true +permissions: + contents: read + jobs: smoke:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/smoke-sharkey.yml around lines 19 - 24, The workflow currently relies on inherited GITHUB_TOKEN scopes; add an explicit top-level permissions block to pin least privilege (e.g., set permissions: contents: read) right alongside the existing top-level keys (e.g., after the concurrency block and before jobs) so the workflow uses only the minimal token scope needed.test/smoke/sharkey/docker-compose.yml (1)
87-87:⚠️ Potential issue | 🟡 MinorNormalize
depends_onmappings to pass YAMLlint.Current inline mappings violate the configured braces rule and will keep lint red.
🧩 Suggested fix
depends_on: - fedify-harness-backend: { condition: service_healthy } + fedify-harness-backend: + condition: service_healthy @@ depends_on: - db: { condition: service_healthy } - redis: { condition: service_healthy } + db: + condition: service_healthy + redis: + condition: service_healthy @@ depends_on: - sharkey-web-backend: { condition: service_healthy } + sharkey-web-backend: + condition: service_healthyAlso applies to: 110-111, 134-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/smoke/sharkey/docker-compose.yml` at line 87, The inline mapping for the depends_on entry "fedify-harness-backend: { condition: service_healthy }" violates the project's braces rule; replace it with a normalized YAML mapping using a nested key (e.g., under the depends_on mapping set fedify-harness-backend: on its own line and indent a nested condition: service_healthy beneath it) and apply the same change to the other occurrences noted (lines referenced around the file). Locate the depends_on blocks that contain "fedify-harness-backend" and convert any inline "{ condition: service_healthy }" to the multi-line form so YAMLlint passes.test/smoke/sharkey/provision.sh (2)
135-141:⚠️ Potential issue | 🟠 MajorWrite
.env.testwith shell-escaped assignments.These values are sourced by shell at
.github/workflows/smoke-sharkey.ymlLine 79. Raw heredoc assignments allow expansion/substitution if token content is unsafe.🛡️ Suggested fix
echo "→ Writing test env..." -cat > test/smoke/.env.test <<EOF -SERVER_BASE_URL=http://localhost:3000 -SERVER_INTERNAL_HOST=sharkey -SERVER_ACCESS_TOKEN=$TOKEN -HARNESS_BASE_URL=http://localhost:3001 -HARNESS_ORIGIN=https://fedify-harness -EOF +{ + printf 'SERVER_BASE_URL=%q\n' "http://localhost:3000" + printf 'SERVER_INTERNAL_HOST=%q\n' "sharkey" + printf 'SERVER_ACCESS_TOKEN=%q\n' "$TOKEN" + printf 'HARNESS_BASE_URL=%q\n' "http://localhost:3001" + printf 'HARNESS_ORIGIN=%q\n' "https://fedify-harness" +} > test/smoke/.env.test🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/smoke/sharkey/provision.sh` around lines 135 - 141, The .env.test heredoc writes raw assignments that allow unsafe shell expansion; update test/smoke/provision.sh so .env.test is written with shell-escaped values (e.g., ensure SERVER_ACCESS_TOKEN is escaped from the TOKEN variable) instead of raw heredoc expansion; locate the block that writes .env.test and replace the heredoc approach with generating each assignment using a shell-escaping method (for the TOKEN and any other potentially unsafe values) so sourcing .env.test in the workflow cannot perform unintended substitutions.
34-35:⚠️ Potential issue | 🟠 MajorRemove sensitive token/response material from CI logs.
Token prefixes and raw API payload snippets are still being printed. Keep only non-sensitive status logs.
🧹 Suggested fix
-echo " admin token: ${ADMIN_TOKEN:0:8}..." +echo " admin auth ready" -echo " testuser token: ${TEST_TOKEN:0:8}..." +echo " testuser auth ready" - echo " ap/show attempt $i: ${RESOLVE_RAW:0:300}" + echo " ap/show attempt $i: $(echo "$RESOLVE_RAW" | jq -r 'if .object.id then "resolved" elif .error then (.error.message // .error // "error") else "not-resolved" end' 2>/dev/null || echo "non-JSON response")" -echo "✓ Provisioning complete (token: ${TOKEN:0:8}...)" +echo "✓ Provisioning complete"Also applies to: 86-87, 109-110, 143-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/smoke/sharkey/provision.sh` around lines 34 - 35, The script prints sensitive token prefixes and raw API payloads (e.g., the echo that outputs "admin token: ${ADMIN_TOKEN:0:8}..." and similar echoes at other locations), so remove any direct token/payload output and replace it with non-sensitive status messages; for example, change echoes that reference ADMIN_TOKEN or other token/response variables to a neutral message such as "admin token: [REDACTED]" or "admin token: present" and stop echoing raw API payload snippets—update the echo lines found in provision.sh (the one using ADMIN_TOKEN and the similar lines around the other reported locations) to only log status without secrets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/smoke/sharkey/generate-certs.sh`:
- Around line 16-21: Remove the stderr redirections that hide OpenSSL errors: in
generate-certs.sh, update the openssl invocations (e.g., the openssl genrsa and
openssl req commands shown and the other occurrences around the later blocks) to
omit "2>/dev/null" so OpenSSL error output is preserved in CI logs; ensure any
remaining redirections still allow stderr to be visible for debugging.
---
Duplicate comments:
In @.github/workflows/smoke-sharkey.yml:
- Around line 19-24: The workflow currently relies on inherited GITHUB_TOKEN
scopes; add an explicit top-level permissions block to pin least privilege
(e.g., set permissions: contents: read) right alongside the existing top-level
keys (e.g., after the concurrency block and before jobs) so the workflow uses
only the minimal token scope needed.
In `@test/smoke/harness/backdoor.ts`:
- Around line 62-66: Fallback constructs inboxId with variable scheme but
creates actorId always with https; change actorId to use the same scheme
variable so both URLs share scheme (use scheme, domain, user to build actorId),
then store the pair in recipientCache as before (symbols: inboxId, actorId,
recipientCache, handle, scheme, domain, user).
- Around line 58-60: The catch block in backdoor.ts that currently swallows
WebFinger errors should surface unexpected transport/TLS/parse failures instead
of silently falling back: change the bare catch to catch (err) { throw err } or
at minimum log the error (e.g., processLogger.error or console.error with the
error and context) and then rethrow so callers of the WebFinger lookup (the code
around the try/catch handling wfRes.ok) will see the real failure rather than
proceeding to the Mastodon fallback.
In `@test/smoke/orchestrator.ts`:
- Around line 158-205: The try/catch blocks treating all failures as
"unsupported" must be tightened: in the serverGet attempts (the first search
block and the lookup block using serverGet) only swallow/continue on explicit
not-supported/not-found responses (e.g., response status 404/410 or other
explicit endpoint-missing indicators returned by serverGet); for any other error
(network error, 401/403 auth, 5xx) rethrow or surface it (or call
processLogger.error with the error and context) so poll() fails fast. Likewise,
for the Misskey POST to `${SERVER_URL}/api/users/show` check res.ok and only
fall through when res.status indicates not-found/unsupported (e.g., 404); for
non-ok statuses that are not "unsupported" (401/403/5xx) throw or log the
response status and body. Update the catch handlers to inspect the caught error
(or the serverGet response) instead of always swallowing, and include the
handle/SERVER_URL and error details in the log when rethrowing or logging.
In `@test/smoke/sharkey/default.yml`:
- Around line 19-22: The allowedPrivateNetworks entry currently uses catch-all
CIDRs (0.0.0.0/0 and ::/0) which removes any restriction; replace those with the
specific private network CIDRs required by the smoke stack (e.g., IPv4
10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16 and the appropriate IPv6 ULA range
like fc00::/7 or the actual internal prefixes in use) by updating the
allowedPrivateNetworks list so Sharkey only permits the intended Docker-internal
targets.
In `@test/smoke/sharkey/docker-compose.yml`:
- Line 87: The inline mapping for the depends_on entry "fedify-harness-backend:
{ condition: service_healthy }" violates the project's braces rule; replace it
with a normalized YAML mapping using a nested key (e.g., under the depends_on
mapping set fedify-harness-backend: on its own line and indent a nested
condition: service_healthy beneath it) and apply the same change to the other
occurrences noted (lines referenced around the file). Locate the depends_on
blocks that contain "fedify-harness-backend" and convert any inline "{
condition: service_healthy }" to the multi-line form so YAMLlint passes.
In `@test/smoke/sharkey/provision.sh`:
- Around line 135-141: The .env.test heredoc writes raw assignments that allow
unsafe shell expansion; update test/smoke/provision.sh so .env.test is written
with shell-escaped values (e.g., ensure SERVER_ACCESS_TOKEN is escaped from the
TOKEN variable) instead of raw heredoc expansion; locate the block that writes
.env.test and replace the heredoc approach with generating each assignment using
a shell-escaping method (for the TOKEN and any other potentially unsafe values)
so sourcing .env.test in the workflow cannot perform unintended substitutions.
- Around line 34-35: The script prints sensitive token prefixes and raw API
payloads (e.g., the echo that outputs "admin token: ${ADMIN_TOKEN:0:8}..." and
similar echoes at other locations), so remove any direct token/payload output
and replace it with non-sensitive status messages; for example, change echoes
that reference ADMIN_TOKEN or other token/response variables to a neutral
message such as "admin token: [REDACTED]" or "admin token: present" and stop
echoing raw API payload snippets—update the echo lines found in provision.sh
(the one using ADMIN_TOKEN and the similar lines around the other reported
locations) to only log status without secrets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bff83842-9ce5-482c-be04-b323962b7387
📒 Files selected for processing (11)
.github/workflows/smoke-sharkey.yml.gitignoretest/smoke/harness/backdoor.tstest/smoke/orchestrator.tstest/smoke/sharkey/Caddyfile.fedify-harnesstest/smoke/sharkey/Caddyfile.sharkeytest/smoke/sharkey/default.ymltest/smoke/sharkey/docker-compose.ymltest/smoke/sharkey/docker.envtest/smoke/sharkey/generate-certs.shtest/smoke/sharkey/provision.sh
dahlia
previously approved these changes
Apr 10, 2026
dahlia
added
type/test
Remove 2>/dev/null redirections from openssl commands so that errors are visible in CI logs when certificate generation fails. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dahlia
approved these changes
Apr 10, 2026
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
2 participants
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.
Background
Fedify currently has smoke tests for Mastodon, but lacks coverage for Misskey-family servers which use different API conventions and URL patterns. Sharkey is a popular Misskey fork that represents this server family well.
A key challenge is that Sharkey requires HTTPS for all ActivityPub federation lookups —
checkHttps()inApPersonServicerejectshttp://URIs outright, andWebFingerService.genUrl()hardcodeshttps://(see misskey-dev/misskey#10716). This means the test infrastructure needs TLS termination, unlike the plain HTTP setup used for Mastodon non-strict tests.Additionally, Sharkey's Mastodon-compatible API layer has gaps (e.g.
/api/v1/accounts/searchreturns 404 for remote users,/api/v1/accounts/lookupthrowsTypeError), so the orchestrator needs Misskey-native API fallbacks to work reliably.Related: #654
Summary
smoke-sharkey) that boots a Sharkey stack with Caddy TLS proxies and self-signed certificates, provisions users, and runs E2E scenarios via the shared orchestratorsharkey-web-backend) to avoid DNS collision with the TLS proxy aliases (sharkey,fedify-harness)/_test/reset/api/users/showendpointsres.okchecks before JSON parsing in inbox poll helpers🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com