h4ks: docker stack rebrand + image fixes#9
Merged
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:
📝 WalkthroughWalkthroughConsolidates Docker stack and compose into env-driven services (obbyircd, obby-api, optional web), adds reusable GitHub Actions for multi-platform Docker builds/pushes, updates entrypoint key generation and custom-module wiring, removes hosted-backend build artifacts, and introduces an end-to-end test suite with container harness and async IRC client tests. ChangesDocumentation Foundation
Docker Stack & Deployment
End-to-end tests
Sequence Diagram(s)sequenceDiagram
participant User as Push/Tag/PR
participant docker-obbyircd as docker-obbyircd.yml
participant build_push as docker-build-push.yml
participant DockerHub as Docker Hub
User->>docker-obbyircd: trigger build event
docker-obbyircd->>build_push: workflow_call(image_name=obbyircd, context=., dockerfile=./docker/Dockerfile)
build_push->>DockerHub: login, build & push multi-platform image
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
CLAUDE.md (1)
1-1: ⚡ Quick winMake the redirect explicit and clickable.
Line 1 currently shows only plain text. Use an explicit Markdown redirect (for example, “See AGENTS.md”) so readers immediately know what to do.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLAUDE.md` at line 1, Replace the plain filename on the first line of CLAUDE.md with an explicit, clickable Markdown redirect; e.g., change the current text "AGENTS.md" to a sentence like "See [AGENTS.md](AGENTS.md)" so readers have a clear, clickable link; update the first line in CLAUDE.md accordingly.AGENTS.md (2)
456-456: 💤 Low valueOptional: Improve grammar for clarity.
The phrase "as if it always was that way" would read more naturally as "as if it was always that way."
✍️ Optional refinement
-- **Write code as if it always was that way.** No patch-history +- **Write code as if it was always that way.** No patch-history🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` at line 456, Update the phrasing in AGENTS.md by changing the sentence "Write code as if it always was that way. No patch-history" to use the more natural grammar "Write code as if it was always that way. No patch-history" (locate the exact string "Write code as if it always was that way" and replace it accordingly).
27-38: 💤 Low valueOptional: Add language identifier to fenced code block.
The directory structure code block lacks a language identifier. Consider adding
textor leaving it empty with triple backticks to satisfy linters.📝 Optional refinement
-``` +```text src/ core ircd C source🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` around lines 27 - 38, Update the fenced directory-structure block in AGENTS.md to include a language identifier (e.g., use ```text) so linters recognize it; locate the triple-backtick block containing the directory listing in AGENTS.md and change the opening fence to include the identifier (for example, replace ``` with ```text) while keeping the block contents unchanged.ARCHITECTURE.md (1)
370-370: 💤 Low valueConsider version number staleness.
The documentation specifies exact versions "Coolify v4.0.0, Traefik v3.6" which may become outdated as the deployment evolves. Consider either:
- Using "as of [date]" notation
- Using ranges like "Coolify v4.x"
- Noting these are deployment-specific details
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ARCHITECTURE.md` at line 370, The line "h4ks is **arm64 (aarch64)**, Ubuntu 24.04, Coolify v4.0.0, Traefik" pins exact tool versions; update that sentence to avoid staleness by either appending "as of YYYY-MM-DD", changing specific versions to ranges like "Coolify v4.x, Traefik v3.x", or adding a parenthetical note that these are deployment-specific examples and may change; ensure the revised text still mentions architecture and OS (arm64, Ubuntu 24.04) and clearly labels the Coolify/Traefik entries as non-authoritative examples.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/docker-build-push.yml:
- Around line 39-43: Guard the Docker Hub authentication and push steps so they
don't run for forked pull requests: add an if-condition to the "Log in to Docker
Hub" step and the Docker push step (the unconditional push around line 65) to
skip execution when the workflow is triggered by a pull_request (e.g. use a
condition like github.event_name != 'pull_request' or another check that ensures
repository secrets are available) so the login and push won't run when secrets
are not provided by GitHub Actions.
In @.github/workflows/docker-obbyircd.yml:
- Around line 27-31: The pull_request.paths filter is missing many entries
present in push.paths; update the pull_request.paths section to match the
push.paths section exactly so PRs touching build/config/docs (e.g., include/**,
doc/conf/**, extras/**, autoconf/**, configure*, Config, Makefile.in,
BSDmakefile, obbyircd.in) trigger the workflow; locate the pull_request.paths
block in the same workflow and add the same list of path patterns used under
push.paths so both triggers are aligned.
In `@ARCHITECTURE.md`:
- Line 392: Replace the inaccurate table row text that currently reads '3478 /
UDP | Public | TURN. Cloudflare cannot proxy UDP — set turn DNS to "DNS only".'
with a clarified note: state that Cloudflare can proxy UDP for TURN via its
Managed Realtime/TURN service and via Spectrum (Enterprise) for self-hosted
TURN, but include the limitations (Spectrum lacks packet fragmentation support
and requires a custom header for client IP) and mention the plan-level
requirement (Spectrum = Enterprise); keep the port/protocol cell ('3478 / UDP')
and the short guidance about DNS only where appropriate but remove the absolute
"cannot proxy UDP" claim.
In `@compose.yaml`:
- Around line 21-22: Traefik's backend port is hardcoded to 8080 while WS_PORT
is made configurable (WS_PORT: "${WS_PORT:-8080}"), causing routing mismatches
when WS_PORT is overridden; update the Traefik service/backend configuration to
reference the same WS_PORT variable (use ${WS_PORT:-8080}) instead of a literal
8080 so Traefik always routes to the configured WebSocket port and keep SSL_PORT
handling unchanged.
- Line 60: The VOICE_TURN_SECRET environment entry currently allows empty/unset
values; update the VOICE_TURN_SECRET interpolation to use shell parameter
expansion with the :? form so docker-compose fails at render time if the
variable is missing (e.g. make VOICE_TURN_SECRET require a value and emit a
clear message). Locate the VOICE_TURN_SECRET env declaration and replace the
plain interpolation with the :? parameter expansion variant to enforce presence.
In `@README.md`:
- Around line 207-209: The README currently claims "no extra steps" to bring up
the full stack but omits required backend secrets; update the README section
that mentions the hosted-backend and mattfly/obby-api:latest (and the
compose.yaml pull) to explicitly require setting environment variables
JWT_SECRET and VOICE_TURN_SECRET before `docker compose up -d`, describe where
to set them (e.g., env file or compose environment) and warn against using
hard-coded/default values so operators must supply secure secrets when building
or running the hosted backend.
---
Nitpick comments:
In `@AGENTS.md`:
- Line 456: Update the phrasing in AGENTS.md by changing the sentence "Write
code as if it always was that way. No patch-history" to use the more natural
grammar "Write code as if it was always that way. No patch-history" (locate the
exact string "Write code as if it always was that way" and replace it
accordingly).
- Around line 27-38: Update the fenced directory-structure block in AGENTS.md to
include a language identifier (e.g., use ```text) so linters recognize it;
locate the triple-backtick block containing the directory listing in AGENTS.md
and change the opening fence to include the identifier (for example, replace ```
with ```text) while keeping the block contents unchanged.
In `@ARCHITECTURE.md`:
- Line 370: The line "h4ks is **arm64 (aarch64)**, Ubuntu 24.04, Coolify v4.0.0,
Traefik" pins exact tool versions; update that sentence to avoid staleness by
either appending "as of YYYY-MM-DD", changing specific versions to ranges like
"Coolify v4.x, Traefik v3.x", or adding a parenthetical note that these are
deployment-specific examples and may change; ensure the revised text still
mentions architecture and OS (arm64, Ubuntu 24.04) and clearly labels the
Coolify/Traefik entries as non-authoritative examples.
In `@CLAUDE.md`:
- Line 1: Replace the plain filename on the first line of CLAUDE.md with an
explicit, clickable Markdown redirect; e.g., change the current text "AGENTS.md"
to a sentence like "See [AGENTS.md](AGENTS.md)" so readers have a clear,
clickable link; update the first line in CLAUDE.md accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8cebb4af-5130-4fac-bca3-9ed710874bb6
📒 Files selected for processing (13)
.env.example.github/workflows/docker-build-push.yml.github/workflows/docker-obbyircd.ymlAGENTS.mdARCHITECTURE.mdCLAUDE.mdREADME.mdcompose.yamldocker/Dockerfiledocker/docker-entrypoint.shhosted-backend/.dockerignorehosted-backend/Dockerfilehosted-backend/compose.yaml
💤 Files with no reviewable changes (3)
- hosted-backend/.dockerignore
- hosted-backend/Dockerfile
- hosted-backend/compose.yaml
| environment: | ||
| PORT: "8080" | ||
| VOICE_BRIDGE_SOCKET: /run/obbyirc/voice.sock | ||
| VOICE_TURN_SECRET: ${VOICE_TURN_SECRET} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the compose.yaml file
fd -t f 'compose\.ya?ml' -x cat -n {} \;Repository: ObsidianIRC/ObbyIRCd
Length of output: 3978
🏁 Script executed:
# Get specific context around line 60
fd -t f 'compose\.ya?ml' -x sed -n '55,65p' {}Repository: ObsidianIRC/ObbyIRCd
Length of output: 488
🏁 Script executed:
# Check if there are other secret/credential environment variables that might have the same issue
fd -t f 'compose\.ya?ml' -x grep -n 'SECRET\|PASSWORD\|TOKEN' {}Repository: ObsidianIRC/ObbyIRCd
Length of output: 113
🏁 Script executed:
# Check if there's a .env.example or documentation about required variables
fd -t f '\.env' -o 'README*' -o '*.md' -x cat -n {} \; 2>/dev/null | head -100Repository: ObsidianIRC/ObbyIRCd
Length of output: 46
🌐 Web query:
Docker Compose variable expansion required parameter error handling syntax :?
💡 Result:
In Docker Compose, you can enforce mandatory environment variables during variable interpolation using the:? or? syntax [1][2][3]. This causes Docker Compose to exit with an error message if the specified variable is missing or empty [1][3]. The supported syntax options are: ${VARIABLE:?err} This syntax exits with an error message containing err if VARIABLE is unset or empty in the environment [1][2][3]. ${VARIABLE?err} This syntax exits with an error message containing err only if VARIABLE is unset in the environment (it will allow an empty string) [1][2][3]. These features are part of the standard variable interpolation supported in Compose files, allowing you to validate configuration requirements at runtime before the containers are created [1][3].
Citations:
- 1: https://compose-spec.github.io/compose-spec/12-interpolation.html
- 2: https://docs.docker.com/compose/how-tos/environment-variables/variable-interpolation/
- 3: https://docs.docker.com/reference/compose-file/interpolation/
Enforce VOICE_TURN_SECRET at compose render time instead of allowing empty value
Line 60 currently permits an empty or unset secret, which can leave TURN authentication misconfigured at runtime. Use the :? parameter expansion syntax to fail fast at compose rendering time.
Suggested fix
- VOICE_TURN_SECRET: ${VOICE_TURN_SECRET}
+ VOICE_TURN_SECRET: ${VOICE_TURN_SECRET:?VOICE_TURN_SECRET must be set}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@compose.yaml` at line 60, The VOICE_TURN_SECRET environment entry currently
allows empty/unset values; update the VOICE_TURN_SECRET interpolation to use
shell parameter expansion with the :? form so docker-compose fails at render
time if the variable is missing (e.g. make VOICE_TURN_SECRET require a value and
emit a clear message). Locate the VOICE_TURN_SECRET env declaration and replace
the plain interpolation with the :? parameter expansion variant to enforce
presence.
Comment on lines
+207
to
+209
| The hosted-backend (Go REST/JWT API + WebRTC SFU + TURN that works alongside the IRCd for account management, channel metadata, and image hosting) lives in its own repository: <https://github.com/ObsidianIRC/hosted-backend>. The published image `mattfly/obby-api:latest` is pulled by this repo's `compose.yaml`, so `docker compose up -d` brings up the full stack with no extra steps. | ||
|
|
||
| **Prerequisites:** Go 1.21+ with CGO enabled (for SQLite). | ||
|
|
||
| ```bash | ||
| cd hosted-backend | ||
| export JWT_SECRET=your_secret | ||
| export IRC_SERVER_KEY=your_key | ||
| CGO_ENABLED=1 go build -o backend . | ||
| ./backend | ||
| ``` | ||
|
|
||
| The backend listens on `http://localhost:8080` by default. The Docker Compose setup starts it automatically alongside the IRCd. See [hosted-backend/README.md](hosted-backend/README.md) for the full API reference. | ||
| To build the backend from source, clone its repo separately and follow the build instructions there. |
There was a problem hiding this comment.
Document required backend secrets before “full stack” startup claim.
The “no extra steps” wording should explicitly require setting backend secrets (JWT_SECRET, VOICE_TURN_SECRET) in environment config before startup; otherwise users may run insecure defaults or misconfigure auth/voice behavior.
Based on learnings: JWT and TURN secrets in hosted backend must be environment-driven (JWT_SECRET, VOICE_TURN_SECRET), never hard-coded defaults.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 207 - 209, The README currently claims "no extra
steps" to bring up the full stack but omits required backend secrets; update the
README section that mentions the hosted-backend and mattfly/obby-api:latest (and
the compose.yaml pull) to explicitly require setting environment variables
JWT_SECRET and VOICE_TURN_SECRET before `docker compose up -d`, describe where
to set them (e.g., env file or compose environment) and warn against using
hard-coded/default values so operators must supply secure secrets when building
or running the hosted backend.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Line 47: Split combined environment assignments into individual lines so each
KEY=VALUE pair is on its own line; specifically change the single line
containing RPC_PASSWORD and RPC_PORT into two lines (RPC_PASSWORD= and
RPC_PORT=8600) and do the same for the line that combines CLOAK_KEY with any
other variable so each of CLOAK_KEY and the other key are on separate KEY=VALUE
lines, preserving placeholder values or leaving values blank as in the rest of
the file.
In `@docker/docker-entrypoint.sh`:
- Line 67: The FILEHOST_CONFIG export uses literal single quotes causing SC2089;
replace the direct quoted assignment with a safe printf-based assignment (e.g.
use printf -v FILEHOST_CONFIG 'filehosts { host '\''%s'\''; };' "$FILEHOST_URL"
and then export FILEHOST_CONFIG) to correctly interpolate FILEHOST_URL; likewise
search for other similar quoted-literal assignments (the other SC2089/SC2090
instances) and convert them to printf -v or properly quoted parameter expansion,
fix SC2090 occurrences by avoiding concatenation of quoted strings and variables
(use printf or proper quoting), and resolve SC2046 by quoting command
substitutions or using arrays instead of unquoted word-splitting; run shellcheck
and apply the same patterns to the other flagged symbols so the script becomes
ShellCheck-clean.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2647c51-f2cb-4849-861c-1954240d7a35
📒 Files selected for processing (3)
.env.example.github/workflows/docker-build-push.ymldocker/docker-entrypoint.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/docker-build-push.yml
matheusfillipe
force-pushed
the
feat/h4ks-fixes
branch
from
00d64c0 to
216df7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
tests/e2e/test_cap.py (1)
14-14: ⚡ Quick winAdd a return type hint to the module-level async test.
Annotate this test with
-> None.As per coding guidelines, "`**/*.py`: Use type hints for module-level functions in Python code".Proposed fix
-async def test_cap_ls_advertises_obby_set(ircd): +async def test_cap_ls_advertises_obby_set(ircd) -> None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/test_cap.py` at line 14, The test function declaration for test_cap_ls_advertises_obby_set is missing a return type annotation; update the async def test_cap_ls_advertises_obby_set(ircd) signature to include an explicit return type hint -> None so it becomes async def test_cap_ls_advertises_obby_set(ircd) -> None, keeping the function name and parameter unchanged.tests/e2e/test_smoke.py (1)
4-32: ⚡ Quick winAnnotate module-level async test functions with
-> None.These are module-level functions and should be typed.
As per coding guidelines, "`**/*.py`: Use type hints for module-level functions in Python code".Proposed fix
-async def test_welcome(ircd): +async def test_welcome(ircd) -> None: @@ -async def test_network_name(ircd): +async def test_network_name(ircd) -> None: @@ -async def test_join_and_privmsg_echo(ircd): +async def test_join_and_privmsg_echo(ircd) -> None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/test_smoke.py` around lines 4 - 32, The three module-level async test functions (test_welcome, test_network_name, test_join_and_privmsg_echo) lack explicit return type annotations; update each function signature to include the return type "-> None" (e.g., async def test_welcome(...) -> None:) so they comply with the project's typing guideline for module-level functions.tests/e2e/lib/container.py (1)
13-14: ⚡ Quick winAdd a return type hint to the module-level helper.
_cloakshould declare its return type per Python guideline.As per coding guidelines, "`**/*.py`: Use type hints for module-level functions in Python code".Proposed fix
-def _cloak(): +def _cloak() -> str: return "".join(secrets.choice(ALNUM) for _ in range(96))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/lib/container.py` around lines 13 - 14, The helper function _cloak lacks a return type hint; update its signature (function _cloak) to annotate the return type as str and ensure imports/types still resolve (it uses secrets and ALNUM), e.g., add -> str to the def _cloak(...) declaration so the module-level function follows the project's type-hinting guideline.tests/e2e/conftest.py (1)
12-18: ⚡ Quick winAdd a type hint for the module-level fixture function.
The fixture should declare its generator return type.
As per coding guidelines, "`**/*.py`: Use type hints for module-level functions in Python code".Proposed fix
+from collections.abc import Generator + `@pytest.fixture` -def ircd(): +def ircd() -> Generator[IrcdContainer, None, None]: c = IrcdContainer().up()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/conftest.py` around lines 12 - 18, Add a generator return type annotation to the module-level pytest fixture function ircd: change its signature to def ircd() -> Generator[IrcdContainer, None, None]: and import Generator from typing (or collections.abc.Generator in Python 3.9+); this indicates the function yields an IrcdContainer (the instance returned by IrcdContainer().up()), with no sent value and no return value, leaving the body (calls to up(), wait_ready(), yield c, and down()) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 27: The fenced code block under the "Where things live" section in
AGENTS.md is missing a language tag which triggers markdownlint MD040; edit the
Markdown and add an appropriate language identifier (for example "text" or
"markdown") after the opening backticks of that fenced block so it becomes
```text (or ```markdown) to satisfy the linter and prevent MD040.
- Around line 14-15: The document currently contradicts itself about where the
hosted backend lives: one sentence ("ObsidianIRC modules and the hosted backend
live in-tree, not as third-party drops.") claims it is in-tree while other
passages (the lines referencing the hosted-backend repository) state it lives in
a separate repo; pick the correct single source-of-truth and make all mentions
consistent. Replace or remove the incorrect sentence about "ObsidianIRC modules
and the hosted backend live in-tree" and update the other references that
mention the separate repository so every occurrence of "hosted backend",
"hosted-backend", and the statement about ObsidianIRC modules reflects the
chosen location (in-tree OR separate repo) and, if appropriate, add a short
parenthetical pointing to the canonical repo/location.
In `@ARCHITECTURE.md`:
- Line 29: Several fenced code blocks in ARCHITECTURE.md are unlabeled and
trigger markdownlint MD040; update each triple-backtick fence to include an
appropriate language identifier (e.g., text or bash). Locate the unlabeled
blocks by their contents (the tree snippet starting with "$PREFIX/", the config
example containing "CONF_BIND=/srv/obbyircd/conf", the ASCII diagram that starts
with "┌──────────┐", and the lists like "draft/account-registration" and the
file tree beginning with "."), and change lines like ``` to ```text or ```bash
as appropriate so all fenced code blocks are tagged.
In `@tests/e2e/lib/irc.py`:
- Around line 49-50: Change the bare except handler to preserve exception
chaining: in the block that currently does "except asyncio.TimeoutError:"
(inside the method that raises TimeoutError with self.lines[-10:]), capture the
original exception (e.g., "except asyncio.TimeoutError as err:") and re-raise
the new TimeoutError using "raise TimeoutError(f\"never matched (last:
{self.lines[-10:]})\") from err" so the traceback retains the original
asyncio.TimeoutError as the __cause__.
- Around line 39-41: The current assignment predicate = lambda l: needle in l
uses an anonymous lambda with a single-letter parameter which triggers Ruff
warnings (E731, E741); replace the lambda with a clearly named function (e.g.
def contains_line(line): return needle in line) and assign predicate =
contains_line so you avoid the lambda and use a descriptive parameter name;
refer to the variables predicate, needle and the existing lambda l when making
the change.
In `@tests/e2e/README.md`:
- Around line 8-14: The fenced code block in tests/e2e/README.md lacks a
language identifier and fails markdown linting; update the triple-backtick fence
to include a language tag (e.g., "bash") for the command block so the linter
accepts it and commit the change along with any lint fixes; locate the block
shown in the diff and change the opening ``` to ```bash.
In `@tests/e2e/test_cap.py`:
- Line 21: Rename the ambiguous lambda parameter "l" used in the c.expect call
to "line" to satisfy Ruff E741; locate the expression awaiting c.expect(lambda
l: "CAP " in l and " LS " in l, change the lambda parameter to "line" and update
its usages inside the lambda so it becomes c.expect(lambda line: "CAP " in line
and " LS " in line).
In `@tests/e2e/test_smoke.py`:
- Line 8: Rename the anonymous lambda parameter named `l` to a clear name like
`line` in all uses of c.expect in tests/e2e/test_smoke.py (e.g., the call using
c.expect(lambda l: " 001 " in l, timeout=30) and the similar lambdas at the
other noted locations), so the predicate reads lambda line: " 001 " in line (and
analogous checks) to resolve Ruff E741 and improve readability; update every
occurrence (lines referenced: the lambdas at the other assertions) to use `line`
consistently.
---
Nitpick comments:
In `@tests/e2e/conftest.py`:
- Around line 12-18: Add a generator return type annotation to the module-level
pytest fixture function ircd: change its signature to def ircd() ->
Generator[IrcdContainer, None, None]: and import Generator from typing (or
collections.abc.Generator in Python 3.9+); this indicates the function yields an
IrcdContainer (the instance returned by IrcdContainer().up()), with no sent
value and no return value, leaving the body (calls to up(), wait_ready(), yield
c, and down()) unchanged.
In `@tests/e2e/lib/container.py`:
- Around line 13-14: The helper function _cloak lacks a return type hint; update
its signature (function _cloak) to annotate the return type as str and ensure
imports/types still resolve (it uses secrets and ALNUM), e.g., add -> str to the
def _cloak(...) declaration so the module-level function follows the project's
type-hinting guideline.
In `@tests/e2e/test_cap.py`:
- Line 14: The test function declaration for test_cap_ls_advertises_obby_set is
missing a return type annotation; update the async def
test_cap_ls_advertises_obby_set(ircd) signature to include an explicit return
type hint -> None so it becomes async def test_cap_ls_advertises_obby_set(ircd)
-> None, keeping the function name and parameter unchanged.
In `@tests/e2e/test_smoke.py`:
- Around line 4-32: The three module-level async test functions (test_welcome,
test_network_name, test_join_and_privmsg_echo) lack explicit return type
annotations; update each function signature to include the return type "-> None"
(e.g., async def test_welcome(...) -> None:) so they comply with the project's
typing guideline for module-level functions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 50816160-c823-4121-a412-c7252db87234
📒 Files selected for processing (24)
.env.example.github/workflows/docker-build-push.yml.github/workflows/docker-obbyircd.yml.github/workflows/e2e.yml.gitignoreAGENTS.mdARCHITECTURE.mdCLAUDE.mdREADME.mdcompose.yamldocker/Dockerfiledocker/docker-entrypoint.shdocker/obbyircd.conf.templatehosted-backend/.dockerignorehosted-backend/Dockerfilehosted-backend/compose.yamltests/e2e/README.mdtests/e2e/conftest.pytests/e2e/lib/__init__.pytests/e2e/lib/container.pytests/e2e/lib/irc.pytests/e2e/pyproject.tomltests/e2e/test_cap.pytests/e2e/test_smoke.py
💤 Files with no reviewable changes (3)
- hosted-backend/compose.yaml
- hosted-backend/.dockerignore
- hosted-backend/Dockerfile
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- README.md
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/docker-obbyircd.yml
- docker/Dockerfile
- .github/workflows/docker-build-push.yml
|
|
||
| ## 2. Where things live | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced block under “Where things live”.
This triggers markdownlint MD040 and is a trivial cleanup.
Based on learnings: "Lint clean on touch; fix errors introduced and inherited lint issues in the same commit".
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AGENTS.md` at line 27, The fenced code block under the "Where things live"
section in AGENTS.md is missing a language tag which triggers markdownlint
MD040; edit the Markdown and add an appropriate language identifier (for example
"text" or "markdown") after the opening backticks of that fenced block so it
becomes ```text (or ```markdown) to satisfy the linter and prevent MD040.
Comment on lines
+39
to
+41
| if isinstance(predicate, str): | ||
| needle = predicate | ||
| predicate = lambda l: needle in l |
There was a problem hiding this comment.
Avoid assigning a lambda and ambiguous single-letter parameter names.
This trips Ruff (E731, E741) and is easy to make explicit.
Proposed fix
if isinstance(predicate, str):
needle = predicate
- predicate = lambda l: needle in l
+ def _contains_needle(line: str) -> bool:
+ return needle in line
+ predicate = _contains_needle📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if isinstance(predicate, str): | |
| needle = predicate | |
| predicate = lambda l: needle in l | |
| if isinstance(predicate, str): | |
| needle = predicate | |
| def _contains_needle(line: str) -> bool: | |
| return needle in line | |
| predicate = _contains_needle |
🧰 Tools
🪛 Ruff (0.15.13)
[error] 41-41: Do not assign a lambda expression, use a def
Rewrite predicate as a def
(E731)
[error] 41-41: Ambiguous variable name: l
(E741)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/lib/irc.py` around lines 39 - 41, The current assignment predicate
= lambda l: needle in l uses an anonymous lambda with a single-letter parameter
which triggers Ruff warnings (E731, E741); replace the lambda with a clearly
named function (e.g. def contains_line(line): return needle in line) and assign
predicate = contains_line so you avoid the lambda and use a descriptive
parameter name; refer to the variables predicate, needle and the existing lambda
l when making the change.
Comment on lines
+8
to
+14
| ``` | ||
| docker build -t obbyircd:e2e -f docker/Dockerfile . | ||
| cd tests/e2e | ||
| uv venv && source .venv/bin/activate | ||
| uv pip install -e . | ||
| OBBYIRCD_IMAGE=obbyircd:e2e pytest -v | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the fenced command block.
Markdown lint will fail on this block without a language tag.
Proposed fix
-```
+```bash
docker build -t obbyircd:e2e -f docker/Dockerfile .
cd tests/e2e
uv venv && source .venv/bin/activate
uv pip install -e .
OBBYIRCD_IMAGE=obbyircd:e2e pytest -v</details>
Based on learnings, "Lint clean on touch; fix errors introduced and inherited lint issues in the same commit".
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 8-8: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/README.md` around lines 8 - 14, The fenced code block in
tests/e2e/README.md lacks a language identifier and fails markdown linting;
update the triple-backtick fence to include a language tag (e.g., "bash") for
the command block so the linter accepts it and commit the change along with any
lint fixes; locate the block shown in the diff and change the opening ``` to
```bash.
| await c.register() | ||
| for _ in range(8): | ||
| try: | ||
| await c.expect(lambda l: "CAP " in l and " LS " in l, timeout=5) |
There was a problem hiding this comment.
Use a non-ambiguous lambda parameter name.
Rename l to line to satisfy Ruff E741.
🧰 Tools
🪛 Ruff (0.15.13)
[error] 21-21: Ambiguous variable name: l
(E741)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/test_cap.py` at line 21, Rename the ambiguous lambda parameter "l"
used in the c.expect call to "line" to satisfy Ruff E741; locate the expression
awaiting c.expect(lambda l: "CAP " in l and " LS " in l, change the lambda
parameter to "line" and update its usages inside the lambda so it becomes
c.expect(lambda line: "CAP " in line and " LS " in line).
| port = ircd.host_port(6697) | ||
| async with IrcClient("127.0.0.1", port, nick="smoke") as c: | ||
| await c.register() | ||
| line = await c.expect(lambda l: " 001 " in l, timeout=30) |
There was a problem hiding this comment.
Rename lambda parameter from l to a clear name (e.g., line).
This currently hits Ruff E741 in multiple assertions.
Also applies to: 16-16, 25-25, 26-26, 29-29, 31-31
🧰 Tools
🪛 Ruff (0.15.13)
[error] 8-8: Ambiguous variable name: l
(E741)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/test_smoke.py` at line 8, Rename the anonymous lambda parameter
named `l` to a clear name like `line` in all uses of c.expect in
tests/e2e/test_smoke.py (e.g., the call using c.expect(lambda l: " 001 " in l,
timeout=30) and the similar lambdas at the other noted locations), so the
predicate reads lambda line: " 001 " in line (and analogous checks) to resolve
Ruff E741 and improve readability; update every occurrence (lines referenced:
the lambdas at the other assertions) to use `line` consistently.
216df7f to
bb25daf
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
tests/e2e/lib/irc.py (2)
39-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace the lambda assignment with a named predicate.
Line 41 still uses assigned lambda syntax; switch to a named local function to avoid lint churn and improve traceability.
Proposed fix
if isinstance(predicate, str): needle = predicate - predicate = lambda line: needle in line + def _contains_needle(line: str) -> bool: + return needle in line + predicate = _contains_needle🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/lib/irc.py` around lines 39 - 42, The current code assigns a lambda to predicate when predicate is a string (needle) which causes lint churn; replace that inline lambda with a named local function (e.g., def predicate_contains(line): return needle in line) and then set predicate = predicate_contains so the behavior is identical but traceable; update the block that checks isinstance(predicate, str) to create the local function referencing the captured needle and assign it to predicate.
49-50:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve exception cause when translating timeout errors.
Line 50 should chain the original
asyncio.TimeoutErrorso root-cause traceback is retained.Proposed fix
- except asyncio.TimeoutError: - raise TimeoutError(f"never matched (last: {self.lines[-10:]})") + except asyncio.TimeoutError as err: + raise TimeoutError(f"never matched (last: {self.lines[-10:]})") from err🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/lib/irc.py` around lines 49 - 50, The exception handler currently swallows the original asyncio.TimeoutError; update the except block to capture the original exception (e.g., "except asyncio.TimeoutError as e:") and re-raise the higher-level TimeoutError with chaining ("raise TimeoutError(f'never matched (last: {self.lines[-10:]})') from e") so the root-cause traceback is preserved; adjust the handler around the asyncio.TimeoutError catch in the method referencing self.lines.tests/e2e/test_smoke.py (1)
8-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRename ambiguous lambda parameter
lacross expectations.These lines still use
l; rename tolineconsistently to resolve lint noise.Proposed fix
- line = await c.expect(lambda l: " 001 " in l, timeout=30) + line = await c.expect(lambda line: " 001 " in line, timeout=30) @@ - await c.expect(lambda l: "NETWORK=TestNet" in l, timeout=30) + await c.expect(lambda line: "NETWORK=TestNet" in line, timeout=30) @@ - await c1.expect(lambda l: " 001 " in l) - await c2.expect(lambda l: " 001 " in l) + await c1.expect(lambda line: " 001 " in line) + await c2.expect(lambda line: " 001 " in line) @@ - await c2.expect(lambda l: "JOIN" in l and "`#room`" in l and "listener" in l) + await c2.expect(lambda line: "JOIN" in line and "`#room`" in line and "listener" in line) @@ - line = await c2.expect(lambda l: "PRIVMSG `#room` :hello" in l, timeout=10) + line = await c2.expect(lambda line: "PRIVMSG `#room` :hello" in line, timeout=10)Also applies to: 16-16, 25-26, 29-29, 31-31
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/test_smoke.py` at line 8, Rename the ambiguous lambda parameter `l` to `line` in all expectation callbacks in tests/e2e/test_smoke.py (e.g., the calls using c.expect(lambda l: " 001 " in l, and the other occurrences at the noted locations); update each lambda expression from `lambda l: ...` to `lambda line: ...` so the parameter name is consistent and linter-friendly across the test expectations.tests/e2e/test_cap.py (1)
21-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRename ambiguous lambda parameter
l.Line 21 still uses
l; rename it tolineto resolve the lint error and improve readability.Proposed fix
- await c.expect(lambda l: "CAP " in l and " LS " in l, timeout=5) + await c.expect(lambda line: "CAP " in line and " LS " in line, timeout=5)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/test_cap.py` at line 21, Rename the ambiguous lambda parameter in the test assertion: replace the short parameter name used in the await c.expect(lambda l: "CAP " in l and " LS " in l, timeout=5) expression with a clearer name (e.g., change lambda l to lambda line) so the predicate reads lambda line: "CAP " in line and " LS " in line to satisfy linting and improve readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/docker-build-push.yml:
- Line 34: The checkout steps currently use actions/checkout@v4 without
disabling token persistence; update each checkout invocation (the steps using
actions/checkout@v4 in the build and manifest jobs) to add persist-credentials:
false so the GITHUB_TOKEN is not written to the workspace and cannot be
inadvertently used by later steps or stored in artifacts.
In @.github/workflows/e2e.yml:
- Around line 23-25: Replace the unpinned action usages with their specific
commit SHAs and disable credential persistence on checkout: update the workflow
to use a pinned SHA for actions/checkout (replace actions/checkout@v4 with
actions/checkout@<commit-sha>) and add the key with: persist-credentials: false
under the checkout step, and similarly replace actions/setup-python@v5 with its
pinned commit SHA (actions/setup-python@<commit-sha>) so both actions are
referenced by fixed SHAs rather than tags.
In `@tests/e2e/conftest.py`:
- Around line 6-13: Remove the runtime sys.path mutation (delete the
sys.path.insert(...) line) and keep the explicit import from lib.container
as-is; then change the pytest fixture "ircd" to be a generator with an explicit
return type annotation (e.g., add "from typing import Generator" and declare
"def ircd() -> Generator[IrcdContainer, None, None]:") and yield the container
instance (create it with "c = IrcdContainer().up()" then "yield c") so the
fixture is properly typed and yields the container for tests.
In `@tests/e2e/test_smoke.py`:
- Line 4: Add explicit return type annotations "-> None" to the module-level
async test functions to satisfy typing conventions: update the signatures of
test_welcome, test_network_name, and test_join_and_privmsg_echo so each async
def includes "-> None" (e.g., change "async def test_welcome(ircd):" to "async
def test_welcome(ircd) -> None:") and ensure the same for the functions named
test_network_name and test_join_and_privmsg_echo.
---
Duplicate comments:
In `@tests/e2e/lib/irc.py`:
- Around line 39-42: The current code assigns a lambda to predicate when
predicate is a string (needle) which causes lint churn; replace that inline
lambda with a named local function (e.g., def predicate_contains(line): return
needle in line) and then set predicate = predicate_contains so the behavior is
identical but traceable; update the block that checks isinstance(predicate, str)
to create the local function referencing the captured needle and assign it to
predicate.
- Around line 49-50: The exception handler currently swallows the original
asyncio.TimeoutError; update the except block to capture the original exception
(e.g., "except asyncio.TimeoutError as e:") and re-raise the higher-level
TimeoutError with chaining ("raise TimeoutError(f'never matched (last:
{self.lines[-10:]})') from e") so the root-cause traceback is preserved; adjust
the handler around the asyncio.TimeoutError catch in the method referencing
self.lines.
In `@tests/e2e/test_cap.py`:
- Line 21: Rename the ambiguous lambda parameter in the test assertion: replace
the short parameter name used in the await c.expect(lambda l: "CAP " in l and "
LS " in l, timeout=5) expression with a clearer name (e.g., change lambda l to
lambda line) so the predicate reads lambda line: "CAP " in line and " LS " in
line to satisfy linting and improve readability.
In `@tests/e2e/test_smoke.py`:
- Line 8: Rename the ambiguous lambda parameter `l` to `line` in all expectation
callbacks in tests/e2e/test_smoke.py (e.g., the calls using c.expect(lambda l: "
001 " in l, and the other occurrences at the noted locations); update each
lambda expression from `lambda l: ...` to `lambda line: ...` so the parameter
name is consistent and linter-friendly across the test expectations.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffdf6145-03a8-49ba-9111-c074bd59553a
📒 Files selected for processing (16)
.env.example.github/workflows/docker-build-push.yml.github/workflows/e2e.yml.gitignoreARCHITECTURE.mdcompose.yamldoc/plans/docker-build-modernize.mddocker/docker-entrypoint.shtests/e2e/README.mdtests/e2e/conftest.pytests/e2e/lib/__init__.pytests/e2e/lib/container.pytests/e2e/lib/irc.pytests/e2e/pyproject.tomltests/e2e/test_cap.pytests/e2e/test_smoke.py
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- ARCHITECTURE.md
Comment on lines
+23
to
+25
| - uses: actions/checkout@v4 | ||
|
|
||
| - uses: actions/setup-python@v5 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the referenced workflow lines and nearby context
sed -n '1,120p' .github/workflows/e2e.yml | nl -ba | sed -n '1,120p'
# Specifically show around the reported line range (23-25) with numbers
nl -ba .github/workflows/e2e.yml | sed -n '15,40p'
# Check whether persist-credentials is set anywhere in this workflow
rg -n "persist-credentials" .github/workflows/e2e.yml || true
# Check if actions/checkout or setup-python are pinned to SHAs anywhere
rg -n "uses: actions/(checkout|setup-python)@" .github/workflows/e2e.yml || trueRepository: ObsidianIRC/ObbyIRCd
Length of output: 107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/e2e.yml"
test -f "$FILE" || { echo "Missing $FILE"; exit 1; }
echo "=== e2e.yml lines 15-40 ==="
awk 'NR>=15 && NR<=40 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo "=== persist-credentials occurrences ==="
rg -n "persist-credentials" "$FILE" || true
echo "=== checkout/setup-python uses lines ==="
rg -n "uses:\s*actions/(checkout|setup-python)@" "$FILE" || true
echo "=== actions/checkout@<40hex> (SHA) check ==="
rg -n "uses:\s*actions/checkout@[0-9a-f]{40}" "$FILE" || true
echo "=== actions/setup-python@<40hex> (SHA) check ==="
rg -n "uses:\s*actions/setup-python@[0-9a-f]{40}" "$FILE" || trueRepository: ObsidianIRC/ObbyIRCd
Length of output: 852
🌐 Web query:
actions/checkout v4 persist-credentials default value
💡 Result:
The default value for the persist-credentials input in actions/checkout v4 is true [1][2]. When set to true (the default), the action automatically persists the authentication token or SSH key in the local git configuration within the workspace [1][2]. This allows subsequent steps in the same job to execute authenticated git commands easily [1][2]. The token is removed during the post-job cleanup process [1][2]. If you wish to prevent this behavior—for instance, to avoid credentials being persisted in the.git/config file, which could pose a security risk if the repository is archived or uploaded as an artifact—you must explicitly set the input to false in your workflow file [3][4]: uses: actions/checkout@v4 with: persist-credentials: false
Citations:
- 1: https://github.com/actions/checkout/tree/v4
- 2: https://github.com/actions/checkout
- 3: Remove
persist-credentialsor change the default tofalseactions/checkout#485 - 4: https://yossarian.net/til/post/actions-checkout-can-leak-github-credentials/
🌐 Web query:
actions/checkout@v4 persist-credentials default
💡 Result:
In actions/checkout@v4, the default value for the persist-credentials input is true [1][2][3]. When persist-credentials is set to its default (true), the action configures the authentication token (such as the default GITHUB_TOKEN or a custom SSH key) into the local git configuration of the checked-out repository [1][4][3]. This allows subsequent steps in the workflow to execute authenticated git commands [1][3]. Security Implications: Because the credentials are added to the local.git/config file, any subsequent step or process in the same job—including third-party actions or scripts—can potentially read these credentials [5][4]. If a repository is archived and uploaded as a workflow artifact, these persisted credentials can inadvertently be leaked or exposed [4]. To avoid this, it is a recommended security practice to explicitly set persist-credentials to false if your workflow does not require authenticated git operations in subsequent steps [4][3]. To disable this behavior, use the following configuration in your workflow file [4]: - uses: actions/checkout@v4 with: persist-credentials: false
Citations:
- 1: https://github.com/actions/checkout/tree/v4
- 2: https://github.com/actions/checkout
- 3: https://spacelift.io/blog/github-actions-checkout
- 4: https://yossarian.net/til/post/actions-checkout-can-leak-github-credentials/
- 5: Remove
persist-credentialsor change the default tofalseactions/checkout#485
Harden e2e workflow actions (pin SHAs + disable checkout credential persistence).
- Replace
actions/checkout@v4andactions/setup-python@v5with pinned commit SHAs (no pins in.github/workflows/e2e.ymllines 23-27). - Add
with: persist-credentials: falsetoactions/checkout@v4to avoid the default credential persistence behavior (persist-credentialsdefaults totrue).
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 23-23: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 23-23: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 25-25: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e.yml around lines 23 - 25, Replace the unpinned action
usages with their specific commit SHAs and disable credential persistence on
checkout: update the workflow to use a pinned SHA for actions/checkout (replace
actions/checkout@v4 with actions/checkout@<commit-sha>) and add the key with:
persist-credentials: false under the checkout step, and similarly replace
actions/setup-python@v5 with its pinned commit SHA
(actions/setup-python@<commit-sha>) so both actions are referenced by fixed SHAs
rather than tags.
Comment on lines
+6
to
+13
| sys.path.insert(0, str(Path(__file__).resolve().parent)) | ||
|
|
||
| from lib.container import IrcdContainer | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def ircd(): | ||
| c = IrcdContainer().up() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la
sed -n '1,200p' tests/e2e/conftest.py
python3 - <<'PY'
import ast, pathlib
p = pathlib.Path("tests/e2e/conftest.py")
src = p.read_text()
mod = ast.parse(src)
# report top-level statements order
for i, stmt in enumerate(mod.body, 1):
t = type(stmt).__name__
if isinstance(stmt, (ast.Import, ast.ImportFrom)):
extra = getattr(stmt, "module", None) or ""
print(f"{i}: {t} {extra}")
else:
name = getattr(stmt, "name", "")
print(f"{i}: {t} {name}".strip())
# find ircd fixture function
for node in mod.body:
if isinstance(node, ast.FunctionDef) and node.name == "ircd":
print("ircd return annotation:", ast.unparse(node.returns) if node.returns else None)
# check for Return/Yield
has_return = any(isinstance(n, ast.Return) for n in ast.walk(node))
has_yield = any(isinstance(n, ast.Yield) for n in ast.walk(node))
print("ircd has Return:", has_return, "has Yield:", has_yield)
PYRepository: ObsidianIRC/ObbyIRCd
Length of output: 2578
Fix sys.path mutation and add ircd fixture type hint
tests/e2e/conftest.pymutatessys.pathat module import time before importinglib.container; remove runtime path mutation and rely on proper package/import setup.- The
ircdfixture is a generator (yield c) but has no return type annotation.
Suggested direction
-import sys
-from pathlib import Path
+from typing import Generator
import pytest
-
-sys.path.insert(0, str(Path(__file__).resolve().parent))
-
from lib.container import IrcdContainer
`@pytest.fixture`
-def ircd():
+def ircd() -> Generator[IrcdContainer, None, None]:
c = IrcdContainer().up()
try:
c.wait_ready()
yield c
finally:
c.down()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/conftest.py` around lines 6 - 13, Remove the runtime sys.path
mutation (delete the sys.path.insert(...) line) and keep the explicit import
from lib.container as-is; then change the pytest fixture "ircd" to be a
generator with an explicit return type annotation (e.g., add "from typing import
Generator" and declare "def ircd() -> Generator[IrcdContainer, None, None]:")
and yield the container instance (create it with "c = IrcdContainer().up()" then
"yield c") so the fixture is properly typed and yields the container for tests.
| from lib.irc import IrcClient | ||
|
|
||
|
|
||
| async def test_welcome(ircd): |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant portion of the file with line numbers
ls -la tests/e2e/test_smoke.py
sed -n '1,120p' tests/e2e/test_smoke.py | cat -n
# Show all async test function definitions in the file (module-level)
rg -n "^(async\s+def\s+test_[a-zA-Z0-9_]+\s*\()" tests/e2e/test_smoke.py
# Check whether any test files in the repo already use return type hints for async tests
rg -n "^(async\s+def\s+test_[a-zA-Z0-9_]+\s*\(.*\)\s*->\s*None\s*:)" tests -S || trueRepository: ObsidianIRC/ObbyIRCd
Length of output: 1675
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find async test defs without return annotations in Python files
rg -n --type py "^(async\s+def\s+test_[A-Za-z0-9_]+\s*\([^)]*\))\s*:" \
--glob '**/*.py' . || true
# Find async test defs with explicit return annotation (useful for style alignment)
rg -n --type py "^(async\s+def\s+test_[A-Za-z0-9_]+\s*\([^)]*\))\s*->\s*None\s*:" \
--glob '**/*.py' . || trueRepository: ObsidianIRC/ObbyIRCd
Length of output: 335
Add -> None return type hints for module-level async test functions.
In tests/e2e/test_smoke.py, declare -> None for test_welcome, test_network_name, and test_join_and_privmsg_echo (lines 4, 12, 19).
Proposed fix
-async def test_welcome(ircd):
+async def test_welcome(ircd) -> None:
@@
-async def test_network_name(ircd):
+async def test_network_name(ircd) -> None:
@@
-async def test_join_and_privmsg_echo(ircd):
+async def test_join_and_privmsg_echo(ircd) -> None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/test_smoke.py` at line 4, Add explicit return type annotations "->
None" to the module-level async test functions to satisfy typing conventions:
update the signatures of test_welcome, test_network_name, and
test_join_and_privmsg_echo so each async def includes "-> None" (e.g., change
"async def test_welcome(ircd):" to "async def test_welcome(ircd) -> None:") and
ensure the same for the functions named test_network_name and
test_join_and_privmsg_echo.
Read in MOD_LOAD (not MOD_INIT) so the metadata module's limit defaults are set first, and exempt local-server SETs from the per-entry rate limit. Without both, restored channel/user metadata was silently dropped on startup. First-load sentinel mirrors channeldb/tkldb so /REHASH doesn't double-replay.
New metadata.db writer (k4be format) for channel + user metadata; ergo reader pulls realname/email from scattered pre-metadata-2 keys. Shared UnrealDB string/magic helpers reused from channeldb.
metadata: persist + migrate channel/user metadata
voice-channels: external TURN via voice::turn conf
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/lib/container.py (1)
53-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPublish the configured container ports instead of fixed
6697/8080.This helper accepts
SSL_PORT/WS_PORToverrides viaenv, butup()still maps the hardcoded container ports. Any test that changes either listener port will start an unreachable container through this wrapper.Suggested fix
cmd = [ "docker", "run", "-d", "--name", self.name, - "-p", "127.0.0.1::6697", - "-p", "127.0.0.1::8080", + "-p", f"127.0.0.1::{self.env['SSL_PORT']}", + "-p", f"127.0.0.1::{self.env['WS_PORT']}", ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/lib/container.py` around lines 53 - 57, The port mappings in up() use hardcoded container ports 6697/8080; instead read the configured container ports from the environment (SSL_PORT and WS_PORT) and use those values when building the docker run command. Update the cmd construction in the up() method of the container helper to replace the literal "6697" and "8080" with self.env.get("SSL_PORT", "6697") and self.env.get("WS_PORT", "8080") (or equivalent) so the docker "-p" arguments publish the actual configured container listener ports.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker/docker-entrypoint.sh`:
- Around line 101-114: The loop appends literal newlines/quotes causing
ShellCheck SC2089 and building VOICE_TURN_CONFIG with embedded
backslashes/quotes via command substitution triggers SC2155/SC2090; change the
loop to accumulate lines using printf (e.g., printf '%s\n' "url '$u';" into
voice_turn_url_lines) and then construct VOICE_TURN_CONFIG with another printf
into a local variable (avoiding inline quoted command-substitution) before
exporting it (export VOICE_TURN_CONFIG). Use the existing voice_dq_escape,
VOICE_TURN_SHARED_SECRET and VOICE_TURN_TTL when formatting the printf arguments
so the values are correctly escaped and avoid embedding raw backslashes/quotes
in the assignment.
In `@src/modules/metadata-db.c`:
- Around line 142-158: MOD_LOAD currently gates read_metadatadb() behind
metadatadb_next_event which leaves module-static metadata_storage empty on
reload and causes write_metadatadb_evt to overwrite cfg.database losing offline
accounts' metadata; change MOD_LOAD so it always calls read_metadatadb() on
first load (regardless of metadatadb_next_event) to rebuild metadata_storage
from disk, but keep the gating only for the client-facing
replay/send_out_metadata/METADATA dispatch so that replay to connected users
still happens once; ensure metadatadb_next_event is still set to TStime() +
METADATADB_SAVE_EVERY after reading.
In `@src/modules/voice-channels.c`:
- Around line 667-701: The rehash path doesn't clear removed TURN settings
because free_turn_cfg() is only called when a 'turn' block is present; move or
add a call to free_turn_cfg() (and ensure it resets
cfg_turn_urls/cfg_turn_url_count/cfg_turn_secret/cfg_turn_ttl) at the start of
the CONFIG_MAIN handling before searching for a 'turn' entry so any previously
loaded TURN config is always cleared on rehash, then repopulate cfg_turn_urls,
cfg_turn_url_count, cfg_turn_secret and cfg_turn_ttl only if a 'turn' block is
found in the subsequent loop that processes ConfigEntry items.
In `@tests/e2e/lib/bridge_sidecar.py`:
- Line 8: The function main() lacks a return type annotation; update its
signature in tests/e2e/lib/bridge_sidecar.py to include an explicit return type
(-> None) so the module-level async function is properly type hinted (async def
main() -> None:), and run type checks to ensure no further signature issues.
In `@tests/e2e/test_metadata_persist.py`:
- Around line 102-106: The helper _restart_and_wait currently returns after
seeing the "Configuration test passed OK" log, which can occur before the server
accepts connections; after subprocess.check_call(["docker", "start", c.name])
call, add a call to c.wait_ready() to ensure the container is fully accepting
connections before returning (you can keep the existing marker-based wait
(_docker_logs and _wait_for_marker) but ensure c.wait_ready() is invoked
afterwards to avoid races).
In `@tools/obbyircd-migrate/internal/readers/ergo/reader_test.go`:
- Around line 279-295: The test indexes b.Accounts[0] without checking the slice
length; add a guard asserting len(b.Accounts) > 0 (e.g., t.Fatalf("no accounts
returned") or t.Fatalf("expected at least 1 account, got %d", len(b.Accounts)))
immediately after calling Read(path) to avoid a panic and produce a useful
failure message—mirror the len(b.Accounts) check pattern used in
TestReadErgoFallbackMetadata and then proceed to inspect a := b.Accounts[0] and
the metadata assertions.
---
Outside diff comments:
In `@tests/e2e/lib/container.py`:
- Around line 53-57: The port mappings in up() use hardcoded container ports
6697/8080; instead read the configured container ports from the environment
(SSL_PORT and WS_PORT) and use those values when building the docker run
command. Update the cmd construction in the up() method of the container helper
to replace the literal "6697" and "8080" with self.env.get("SSL_PORT", "6697")
and self.env.get("WS_PORT", "8080") (or equivalent) so the docker "-p" arguments
publish the actual configured container listener ports.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cacec73e-81d1-45c4-97ca-342cb720554d
📒 Files selected for processing (29)
.env.example.gitignoreAGENTS.mdARCHITECTURE.mdREADME.mdcompose.yamldoc/plans/docker-build-modernize.mddoc/plans/external-turn.mddocker/docker-entrypoint.shdocker/obbyircd.conf.templatesrc/modules/metadata-db.csrc/modules/metadata.csrc/modules/voice-channels.ctests/e2e/.gitignoretests/e2e/lib/bridge_sidecar.pytests/e2e/lib/bridge_stub.pytests/e2e/lib/container.pytests/e2e/test_metadata_persist.pytests/e2e/test_voice_turn.pytools/obbyircd-migrate/cmd/obbyircd-migrate/main.gotools/obbyircd-migrate/internal/migrate/migrate.gotools/obbyircd-migrate/internal/readers/ergo/reader.gotools/obbyircd-migrate/internal/readers/ergo/reader_test.gotools/obbyircd-migrate/internal/writer/channeldb.gotools/obbyircd-migrate/internal/writer/channeldb_parser.gotools/obbyircd-migrate/internal/writer/end_to_end_test.gotools/obbyircd-migrate/internal/writer/metadatadb.gotools/obbyircd-migrate/internal/writer/metadatadb_test.gotools/obbyircd-migrate/internal/writer/obsidian.go
✅ Files skipped from review due to trivial changes (6)
- tests/e2e/.gitignore
- README.md
- AGENTS.md
- doc/plans/docker-build-modernize.md
- ARCHITECTURE.md
- .env.example
Comment on lines
+101
to
+114
| voice_turn_url_lines="" | ||
| set -f | ||
| for u in $VOICE_TURN_EXTERNAL_URLS; do | ||
| voice_turn_url_lines="$voice_turn_url_lines | ||
| url '$u';" | ||
| done | ||
| set +f | ||
| export VOICE_TURN_CONFIG="voice { turn {${voice_turn_url_lines} | ||
| shared-secret \"$(voice_dq_escape "${VOICE_TURN_SHARED_SECRET}")\"; | ||
| ttl ${VOICE_TURN_TTL:-21600}; | ||
| }; };" | ||
| echo "External TURN: enabled (${VOICE_TURN_EXTERNAL_URLS})" | ||
| else | ||
| export VOICE_TURN_CONFIG="" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
shellcheck docker/docker-entrypoint.shRepository: obbyworld/ObbyIRCd
Length of output: 2373
🏁 Script executed:
#!/bin/sh
set -eu
shellcheck docker/docker-entrypoint.shRepository: obbyworld/ObbyIRCd
Length of output: 2373
🏁 Script executed:
#!/bin/sh
set -eu
shellcheck docker/docker-entrypoint.shRepository: obbyworld/ObbyIRCd
Length of output: 2373
🏁 Script executed:
shellcheck docker/docker-entrypoint.shRepository: obbyworld/ObbyIRCd
Length of output: 2373
Fix remaining ShellCheck warnings in docker/docker-entrypoint.sh TURN config rendering
ShellCheck still reports:
SC2089atvoice_turn_url_linesappend (line 104)SC2155+SC2090onexport VOICE_TURN_CONFIG="..."(lines 108/114)
Render VOICE_TURN_CONFIG without quote/backslash-bearing shell literals by building it with printf and separate assignments, as below.
Suggested fix
voice_turn_url_lines=""
set -f
for u in $VOICE_TURN_EXTERNAL_URLS; do
voice_turn_url_lines="$voice_turn_url_lines
url '$u';"
done
set +f
- export VOICE_TURN_CONFIG="voice { turn {${voice_turn_url_lines}
- shared-secret \"$(voice_dq_escape "${VOICE_TURN_SHARED_SECRET}")\";
- ttl ${VOICE_TURN_TTL:-21600};
-}; };"
+ voice_turn_secret_escaped=$(voice_dq_escape "${VOICE_TURN_SHARED_SECRET}")
+ VOICE_TURN_CONFIG=$(printf 'voice { turn {%s
+ shared-secret "%s";
+ ttl %s;
+}; };' \
+ "$voice_turn_url_lines" \
+ "$voice_turn_secret_escaped" \
+ "${VOICE_TURN_TTL:-21600}")
+ export VOICE_TURN_CONFIG
else
export VOICE_TURN_CONFIG=""In addition, this file still has other ShellCheck warnings elsewhere (SC2089/SC2090 in WS_CONFIG/FILEHOST_CONFIG/RPC_CONFIG, and SC2046 near the pkg-config call), so it won’t be ShellCheck-clean until those are addressed too.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| voice_turn_url_lines="" | |
| set -f | |
| for u in $VOICE_TURN_EXTERNAL_URLS; do | |
| voice_turn_url_lines="$voice_turn_url_lines | |
| url '$u';" | |
| done | |
| set +f | |
| export VOICE_TURN_CONFIG="voice { turn {${voice_turn_url_lines} | |
| shared-secret \"$(voice_dq_escape "${VOICE_TURN_SHARED_SECRET}")\"; | |
| ttl ${VOICE_TURN_TTL:-21600}; | |
| }; };" | |
| echo "External TURN: enabled (${VOICE_TURN_EXTERNAL_URLS})" | |
| else | |
| export VOICE_TURN_CONFIG="" | |
| voice_turn_url_lines="" | |
| set -f | |
| for u in $VOICE_TURN_EXTERNAL_URLS; do | |
| voice_turn_url_lines="$voice_turn_url_lines | |
| url '$u';" | |
| done | |
| set +f | |
| voice_turn_secret_escaped=$(voice_dq_escape "${VOICE_TURN_SHARED_SECRET}") | |
| VOICE_TURN_CONFIG=$(printf 'voice { turn {%s | |
| shared-secret "%s"; | |
| ttl %s; | |
| }; };' \ | |
| "$voice_turn_url_lines" \ | |
| "$voice_turn_secret_escaped" \ | |
| "${VOICE_TURN_TTL:-21600}") | |
| export VOICE_TURN_CONFIG | |
| echo "External TURN: enabled (${VOICE_TURN_EXTERNAL_URLS})" | |
| else | |
| export VOICE_TURN_CONFIG="" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 104-105: Quotes/backslashes will be treated literally. Rewrite using set/"$@" or functions.
(SC2089)
[warning] 108-108: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 114-114: Quotes/backslashes in this variable will not be respected.
(SC2090)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docker/docker-entrypoint.sh` around lines 101 - 114, The loop appends literal
newlines/quotes causing ShellCheck SC2089 and building VOICE_TURN_CONFIG with
embedded backslashes/quotes via command substitution triggers SC2155/SC2090;
change the loop to accumulate lines using printf (e.g., printf '%s\n' "url
'$u';" into voice_turn_url_lines) and then construct VOICE_TURN_CONFIG with
another printf into a local variable (avoiding inline quoted
command-substitution) before exporting it (export VOICE_TURN_CONFIG). Use the
existing voice_dq_escape, VOICE_TURN_SHARED_SECRET and VOICE_TURN_TTL when
formatting the printf arguments so the values are correctly escaped and avoid
embedding raw backslashes/quotes in the assignment.
| UNIX_SOCK = os.environ.get("STUB_UNIX_SOCK", "/run/obbyirc/voice.sock") | ||
|
|
||
|
|
||
| async def main(): |
There was a problem hiding this comment.
Add a return type annotation to main().
main() is a module-level function and should be annotated (-> None). As per coding guidelines: "Use type hints for module-level functions in Python".
Proposed change
-async def main():
+async def main() -> None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async def main(): | |
| async def main() -> None: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/lib/bridge_sidecar.py` at line 8, The function main() lacks a
return type annotation; update its signature in tests/e2e/lib/bridge_sidecar.py
to include an explicit return type (-> None) so the module-level async function
is properly type hinted (async def main() -> None:), and run type checks to
ensure no further signature issues.
Comment on lines
+102
to
+106
| def _restart_and_wait(c: IrcdContainer, timeout: int = 120): | ||
| pre = _docker_logs(c.name).count("Configuration test passed OK") | ||
| subprocess.check_call(["docker", "stop", "-t", "15", c.name]) | ||
| subprocess.check_call(["docker", "start", c.name]) | ||
| _wait_for_marker(c.name, "Configuration test passed OK", pre + 1, timeout) |
There was a problem hiding this comment.
Wait for full IRC readiness after restart.
This helper returns on the "Configuration test passed OK" log line, which can show up before the server is actually accepting connections again. The next metadata read can then race the restart and make both tests flaky. Reuse c.wait_ready() after docker start.
🧰 Tools
🪛 Ruff (0.15.14)
[error] 104-104: subprocess call: check for execution of untrusted input
(S603)
[error] 104-104: Starting a process with a partial executable path
(S607)
[error] 105-105: subprocess call: check for execution of untrusted input
(S603)
[error] 105-105: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/test_metadata_persist.py` around lines 102 - 106, The helper
_restart_and_wait currently returns after seeing the "Configuration test passed
OK" log, which can occur before the server accepts connections; after
subprocess.check_call(["docker", "start", c.name]) call, add a call to
c.wait_ready() to ensure the container is fully accepting connections before
returning (you can keep the existing marker-based wait (_docker_logs and
_wait_for_marker) but ensure c.wait_ready() is invoked afterwards to avoid
races).
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.
Docker image refs renamed to mattfly/obby-api + mattfly/obby. Compose/entrypoint/Dockerfile fixed (python3-dev, cloak keys). hosted-backend folder removed -- now its own repo.
Companion repos: https://github.com/ObsidianIRC/obby-stack | https://github.com/ObsidianIRC/obby-helm
Summary by CodeRabbit
New Features
Documentation
Chores
Tests
Removals