feat(named-modes): IRCv3 draft/named-modes (PROP command, 96X numerics)#8
Conversation
ChannelList changes: - api-isupport.c: CHANTYPES advertises "#^$" so clients (and the new CHANTYPES-driven validators below) recognise stream channels. - channel.c: valid_channelname() now reads CHANTYPES at runtime instead of hardcoding the prefix list, with "#" as a bootstrap fallback for the config preprocessor's pre-ISupport call path. - struct.h: IsChannelName() macro likewise CHANTYPES-driven. Was hardcoded to '#' || '^', which was rejecting JOIN $foo with ERR_NOSUCHCHANNEL even though valid_channelname() accepted it. - targetfloodprot.c: extends the existing ^ exemption to $ so the WebRTC signaling firehose isn't throttled on stream channels. - voice-channels.c: is_voice_or_stream_channel() helper, so the CAN_JOIN cap gate, PRE_CHANMSG signaling forward, LOCAL_PART bridge notify, and emit_outbound_signal channel-target detection all cover both prefixes uniformly.
… prep)
Adds char *name to Cmode + Umode and changes the registration APIs:
- CmodeAdd(): rejects registrations missing CmodeInfo.name with a
USER-visible ULOG_ERROR. Also detects duplicate-by-name to mirror
the existing duplicate-by-letter check.
- UmodeAdd(): signature gains const char *name as the second
parameter. Same validation + duplicate-by-name guard.
- cm->letter / um->letter are now optional -- a future name-only
mode can register with letter=0 once we wire the executor path.
All 33 chanmodes/*.c and 11 usermodes/*.c modules supply a name. The
spec table (op/voice/ban/topiclock/etc.) gets unprefixed names; every
UnrealIRCd-specific mode gets vendored under obby.world/ to avoid
clashing with future IRCv3 standardisations. obbyscript.c's
JS-driven mode registration also requires a name now.
This commit doesn't add the PROP/RPL_CHMODELIST machinery -- that
lives in the next commit's named-modes module. The data plumbing
ships first because every single mode registration on this branch
depends on it.
Adds src/modules/named-modes.c with the full client-facing surface:
- draft/named-modes capability (CAP LS / REQ / ACK)
- PROP <chan> -> RPL_PROPLIST 961 / RPL_ENDOF 960
- PROP <chan> <listmode> -> RPL_LISTPROPLIST 963 / RPL_ENDOF 962
(v1 supports ban/banex/invex; vendored list-modes will land later)
- PROP <chan> +foo=bar -baz... -> translated to MODE for the existing
cmd_mode dispatcher; cm->letter is
required for now (name-only modes
are a v2 enhancement)
- RPL_CHMODELIST 964 + RPL_UMODELIST 965 sent right after RPL_ISUPPORT
so cap-aware clients can render long-form mode names alongside the
classic letters
MODE -> PROP relay path:
src/modules/mode.c filters cap-holders OUT of its sendto_channel for
the legacy MODE broadcast (clicap | CAP_INVERT, looked up at runtime
via ClientCapabilityBit so this is a no-op when named-modes isn't
loaded). The same chanmode change then triggers
HOOKTYPE_LOCAL_CHANMODE in named-modes.c, which reassembles
(modebuf, parabuf) into "+op=alice -topiclock +ban=*!*@spam" form
and broadcasts that to cap-holders only. Net result: any given
client sees exactly one of the two forms, never both.
ISUPPORT:
api-isupport.c now advertises MAXMODES alongside MODES, mirroring
the existing per-line item budget. The spec REQUIRES MAXMODES.
Modules.default.conf gets `loadmodule "named-modes"` slotted next to
the other obsidianirc/ modules so a default install picks it up.
Known v1 limitations (tracked for follow-up commits):
- User MODE -> PROP for self isn't translated (only channel modes).
- PROP for a name-only mode (cm->letter == 0) errors with
ERR_UNKNOWNMODE; the executor path needs a non-letter dispatch.
- PROP listing for vendored list-modes (e.g. obby.world/timedban)
isn't wired -- only ban/banex/invex.
|
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 IRCv3 named-modes (PROP, numerics, filtering), names all mode handlers, introduces member-roles and channel registration persistence, supports $ stream channels, extends auth to multiple schemes with opportunistic rehash, adds MEMO, config/build updates, and a Go migration tool (readers/writers/CLI/TUI/tests). ChangesIRCv3 named-modes, mode APIs, and module naming
Member-roles, channeldb v101, auth schemes, memo, and configs
Voice and stream channels support
Go-based migration CLI/TUI, readers, writers, and tests
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/channel.c (1)
1446-1449:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInvalid-prefix error message is now stale.
Line 1446 still assumes
#as the only valid prefix, but validation now usesCHANTYPES. This can mislead users when other prefixes are configured.Suggested fix
- if (*channelname != '#') + ISupport *is_chantypes = ISupportFind("CHANTYPES"); + const char *chantypes = (is_chantypes && is_chantypes->value) ? is_chantypes->value : "#"; + if (!*channelname || !strchr(chantypes, *channelname)) { - reason = "Channel name must start with a hash mark (#)"; + static char reasonbuf[128]; + snprintf(reasonbuf, sizeof(reasonbuf), "Channel name must start with one of: %s", chantypes); + reason = reasonbuf; } else🤖 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 `@src/channel.c` around lines 1446 - 1449, The error message hardcodes '#' even though validation now uses CHANTYPES; update the branch in the function that sets reason (the block that checks channelname and assigns reason) to build a message using the CHANTYPES configuration instead of the literal '#'. Use the CHANTYPES symbol (and channelname/reason variables) to produce a message like "Channel name must start with one of: <CHANTYPES>" (or the equivalent localized string), ensuring the message reflects the actual valid prefixes at runtime.src/api-usermode.c (1)
197-225:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResurrection path can leave stale duplicates of letter or name.
The loop returns/resurrects on the first match by letter OR name, and then lines 262–263 unconditionally write both
um->letter = ch;andsafe_strdup(um->name, name);to the resurrected entry. Two concrete failure modes:
- Resurrect-by-letter, mismatched name: an unloaded mode
(ch='x', name="cloak")is matched bych, thenum->nameis overwritten to the new name. If another live mode already has that name, we now have two live modes sharing the same name with no error reported.- Resurrect-by-name, mismatched letter: an unloaded mode
(ch='X', name="foo")is matched byname, thenum->letteris overwritten to the caller'sch. If another live mode already uses that letter (orumode_letter_to_handler[ch]is occupied), we silently steal it via line 259 (umode_letter_to_handler[um->letter] = um).After deciding to resurrect, do a second pass to ensure neither the new letter nor the new name is currently held by a different live mode, and bail with
MODERR_EXISTSif so.Also:
existing(line 181) is set but never read.🤖 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 `@src/api-usermode.c` around lines 197 - 225, The resurrection logic in the usermode loop (iterating um over usermodes) can resurrect by letter or by name and then unconditionally overwrite both um->letter and um->name, which may create duplicate live entries or steal a letter slot in umode_letter_to_handler; change the flow so that when you find an unloaded match (um->unloaded), do NOT immediately modify the entry—instead perform a second scan to verify that no OTHER live mode already has the requested letter (ch) or name (name) and that umode_letter_to_handler[ch] is free or points to this exact unloaded um; if either conflict exists set module->errorcode = MODERR_EXISTS and return NULL; only after both checks succeed, clear um->unloaded and safely assign um->letter and um->name and update umode_letter_to_handler; also remove the unused variable existing (it is set but never read).
🧹 Nitpick comments (2)
src/modules/chanmodes/regonly.c (1)
51-51: ⚡ Quick winPrefer vendor-prefixed name for this non-standard mode.
Line [51] uses an unprefixed
"regonly". To stay consistent with the stated vendoring convention and avoid future collisions, consider namespacing it (for exampleobby.world/regonly).Suggested change
- req.name = "regonly"; + req.name = "obby.world/regonly";🤖 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 `@src/modules/chanmodes/regonly.c` at line 51, Change the non-standard chanmode name from the unprefixed literal used in req.name (currently "regonly") to a vendor-prefixed identifier to avoid collisions (for example "obby.world/regonly"); update the assignment in the module initialization where req.name is set so that the module advertises the namespaced mode string instead of the bare "regonly".src/modules/obbyscript.c (1)
5510-5519: ⚡ Quick winExpose the optional-letter registration path here too.
These bindings still hard-fail unless
config.letteris a single character, so ObbyScript cannot register name-only modes even though the underlying mode APIs now supportletter=0. That leaves the scripting API out of sync with the new named-modes contract.Also applies to: 5588-5597, 5692-5701
🤖 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 `@src/modules/obbyscript.c` around lines 5510 - 5519, The binding currently requires config.letter to be a single-character string and rejects an absent/empty value; change the logic around duk_get_prop_string(ctx, 0, "letter")/letter_str/letter so that if the property is missing or the value is null/empty string you set letter = 0 (to represent a name-only mode), but if a string is present enforce strlen(letter_str) == 1 and assign letter = letter_str[0]; otherwise throw the existing DUK_ERR_TYPE_ERROR. Apply the same change at the other two occurrences that the reviewer called out (the blocks that use duk_get_prop_string + letter_str + letter around the other registration paths).
🤖 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 `@src/api-channelmode.c`:
- Around line 317-329: The code currently allows name-only chanmode registration
(req.letter == 0) but other internals (e.g., checks on m->letter and cm->letter)
assume a non-zero letter; to avoid instability, change CmodeAdd so it rejects
name-only modes for now: add a guard that treats req.letter == 0 as an error
(log a clear message, set module->errorcode = MODERR_INVALID if module present,
and return NULL) similar to the existing BadPtr(req.name) path; reference
CmodeAdd, req.letter, m->letter and cm->letter to locate the change.
In `@src/api-usermode.c`:
- Around line 178-195: Reject registration attempts with ch==0 in UmodeAdd: add
a defensive check after the BadPtr(name) block that treats ch == '\0' as invalid
for now, call unreal_log with a clear message, set module->errorcode =
MODERR_INVALID if module is non-NULL, and return NULL; this prevents downstream
usermode_add_sorted() and the umode_letter_to_handler[0] slot from being
corrupted by a name-only mode until full support is implemented.
In `@src/channel.c`:
- Around line 946-951: The validation currently uses strchr(chantypes, *cname)
without checking for an empty channel name, allowing "" to pass; modify the
check that uses ISupportFind/chantypes and cname so you first verify the name is
non-empty (e.g., ensure *cname != '\0' / cname[0] != '\0') and return 0 for
empty names before calling strchr for prefix validation.
In `@src/modules/chanmodes/permanent.c`:
- Line 79: Update the vendor prefix for the permanent channel mode: in
src/modules/chanmodes/permanent.c where the request name is set (the assignment
to req.name), change the unprefixed literal "permanent" to the namespaced token
"obby.world/permanent" so this UnrealIRCd-specific +P mode follows the PR's
vendor-prefix convention and avoids colliding with spec-defined names exposed
via PROP.
In `@src/modules/chanmodes/secureonly.c`:
- Line 53: The mode registration uses an unprefixed name; change the req.name
assignment in secureonly.c to use the vendor namespaced string
"obby.world/secureonly" instead of "secureonly" so it matches the convention
used for other UnrealIRCd-specific modes (see noknock.c) and avoids future
collisions; update the req.name initializer in the registration code that
currently sets req.name = "secureonly" to req.name = "obby.world/secureonly".
In `@src/modules/named-modes.c`:
- Around line 177-189: The code in named_modes_connect currently sends
RPL_CHMODELIST/RPL_UMODELIST to every local user after numeric 005, breaking
backwards-compat for clients that didn't negotiate the draft; modify
named_modes_connect to only call send_chmodelist/send_umodelist when the client
both is a user (MyUser(client)) and has the draft/named-modes capability enabled
(add a capability check, e.g. client_has_capability(client, "draft/named-modes")
or the project-specific CapIsSet/has_cap method). Update the conditional around
send_chmodelist/send_umodelist in named_modes_connect to require that capability
before emitting the numerics.
- Around line 74-76: The module only registers HOOKTYPE_LOCAL_CHANMODE but must
also handle remote-originated mode changes; add a HookAdd registration for
HOOKTYPE_REMOTE_CHANMODE (using the same handler or a shared handler) on
modinfo->handle so remote chanmode events are processed and relayed to
cap-enabled clients. Specifically, register HOOKTYPE_REMOTE_CHANMODE with
named_modes_local_chanmode (or factor the logic into a new named_modes_chanmode
and register that for both HOOKTYPE_LOCAL_CHANMODE and HOOKTYPE_REMOTE_CHANMODE)
so PROP relays are emitted for remote mode changes too.
- Around line 540-545: The current code builds one concatenated parameter string
(parabuf) and passes it as a single argv element to do_mode(), which breaks
multi-parameter PROP updates; change the logic that fills parabuf and mode_argv
so each PROP parameter becomes its own argv entry: instead of appending into
parabuf, allocate/point separate null-terminated strings for each parameter and
push them into mode_argv (incrementing mode_argc) so do_mode() sees the same
argv shape as a parsed MODE command; keep using modebuf for the mode string and
ensure you stop at the mode_argv capacity (mode_argv[8]) and validate lengths
(modelen/paralen) per entry, applying the same fix in the second block
referenced (around the other occurrence).
- Around line 357-393: The loop skips unknown legacy mode letters without
advancing param_idx, which misaligns subsequent params; fix by detecting whether
the skipped mode would have consumed a parameter and, if so, advance param_idx
(consuming params[param_idx]) before continuing. Concretely, when you hit the
final else branch (no named mapping), look up/compute the mode classification
(use the same logic as with cm/prop_classify_chanmode and wants_param) for *p
and if that indicates a parameter (and param_idx < n_params) increment param_idx
to consume that parameter, then continue; keep using param_idx, n_params,
params, cm, prop_classify_chanmode and wants_param to locate and implement the
change.
In `@src/modules/usermodes/bot.c`:
- Line 49: UmodeAdd("bot", 'B', UMODE_GLOBAL...) return value is not checked, so
if registration fails UMODE_BOT remains 0 and the module continues initializing;
update the init path to capture the return of UmodeAdd(modinfo->handle, "bot",
'B', UMODE_GLOBAL, 0, NULL, &UMODE_BOT), check for failure, log an error via the
module/process logger including the fact that bot mode registration failed and
the returned value/state, and abort module initialization (e.g., return
MODULE_FAIL or call the appropriate ModuleFailed/cleanup routine used elsewhere)
so the module does not load with UMODE_BOT == 0. Ensure any subsequent code
using UMODE_BOT only runs after successful registration.
In `@src/modules/usermodes/noctcp.c`:
- Line 46: Remove the unused dead variable CmodeInfo req from the module
initialization; it’s a leftover copy-paste and not referenced anywhere in
MOD_INIT() or by UmodeAdd, so delete the declaration `CmodeInfo req;` (leaving
UmodeAdd and the rest of MOD_INIT untouched) to avoid stale/unreferenced code.
In `@src/modules/usermodes/privacy.c`:
- Line 42: The user mode registration call UmodeAdd(modinfo->handle,
"hidechans", 'p', UMODE_GLOBAL, 0, umode_allow_all, &UMODE_PRIVACY) uses an
unprefixed vendor-specific name; change the mode name string to include the
project's vendor prefix (e.g. "obby.world/hidechans" or the correct vendor
namespace used elsewhere) so it matches the codebase convention, leaving the
rest of the UmodeAdd arguments (modinfo->handle, 'p', UMODE_GLOBAL,
umode_allow_all, &UMODE_PRIVACY) unchanged.
In `@src/modules/usermodes/regonlymsg.c`:
- Line 42: UmodeAdd(...) return value is ignored; change the module init to
check the result of UmodeAdd(modinfo->handle, "obby.world/regonlymsg", 'R',
UMODE_GLOBAL, 0, umode_allow_all, &UMODE_REGONLYMSG) and if it fails, log an
error (including modinfo->handle and UMODE_REGONLYMSG for context) and abort
module initialization by returning a failure code so the module does not remain
loaded with an unset mode bit.
---
Outside diff comments:
In `@src/api-usermode.c`:
- Around line 197-225: The resurrection logic in the usermode loop (iterating um
over usermodes) can resurrect by letter or by name and then unconditionally
overwrite both um->letter and um->name, which may create duplicate live entries
or steal a letter slot in umode_letter_to_handler; change the flow so that when
you find an unloaded match (um->unloaded), do NOT immediately modify the
entry—instead perform a second scan to verify that no OTHER live mode already
has the requested letter (ch) or name (name) and that
umode_letter_to_handler[ch] is free or points to this exact unloaded um; if
either conflict exists set module->errorcode = MODERR_EXISTS and return NULL;
only after both checks succeed, clear um->unloaded and safely assign um->letter
and um->name and update umode_letter_to_handler; also remove the unused variable
existing (it is set but never read).
In `@src/channel.c`:
- Around line 1446-1449: The error message hardcodes '#' even though validation
now uses CHANTYPES; update the branch in the function that sets reason (the
block that checks channelname and assigns reason) to build a message using the
CHANTYPES configuration instead of the literal '#'. Use the CHANTYPES symbol
(and channelname/reason variables) to produce a message like "Channel name must
start with one of: <CHANTYPES>" (or the equivalent localized string), ensuring
the message reflects the actual valid prefixes at runtime.
---
Nitpick comments:
In `@src/modules/chanmodes/regonly.c`:
- Line 51: Change the non-standard chanmode name from the unprefixed literal
used in req.name (currently "regonly") to a vendor-prefixed identifier to avoid
collisions (for example "obby.world/regonly"); update the assignment in the
module initialization where req.name is set so that the module advertises the
namespaced mode string instead of the bare "regonly".
In `@src/modules/obbyscript.c`:
- Around line 5510-5519: The binding currently requires config.letter to be a
single-character string and rejects an absent/empty value; change the logic
around duk_get_prop_string(ctx, 0, "letter")/letter_str/letter so that if the
property is missing or the value is null/empty string you set letter = 0 (to
represent a name-only mode), but if a string is present enforce
strlen(letter_str) == 1 and assign letter = letter_str[0]; otherwise throw the
existing DUK_ERR_TYPE_ERROR. Apply the same change at the other two occurrences
that the reviewer called out (the blocks that use duk_get_prop_string +
letter_str + letter around the other registration paths).
🪄 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: 4881d355-a350-4832-a5a3-fe01e261bddf
📒 Files selected for processing (57)
doc/conf/modules.default.confinclude/modules.hinclude/struct.hsrc/api-channelmode.csrc/api-isupport.csrc/api-usermode.csrc/channel.csrc/modules/Makefile.insrc/modules/chanmodes/censor.csrc/modules/chanmodes/chanadmin.csrc/modules/chanmodes/chanop.csrc/modules/chanmodes/chanowner.csrc/modules/chanmodes/delayjoin.csrc/modules/chanmodes/floodprot.csrc/modules/chanmodes/halfop.csrc/modules/chanmodes/history.csrc/modules/chanmodes/inviteonly.csrc/modules/chanmodes/isregistered.csrc/modules/chanmodes/issecure.csrc/modules/chanmodes/key.csrc/modules/chanmodes/limit.csrc/modules/chanmodes/link.csrc/modules/chanmodes/moderated.csrc/modules/chanmodes/nocolor.csrc/modules/chanmodes/noctcp.csrc/modules/chanmodes/noexternalmsgs.csrc/modules/chanmodes/noinvite.csrc/modules/chanmodes/nokick.csrc/modules/chanmodes/noknock.csrc/modules/chanmodes/nonickchange.csrc/modules/chanmodes/nonotice.csrc/modules/chanmodes/operonly.csrc/modules/chanmodes/permanent.csrc/modules/chanmodes/private.csrc/modules/chanmodes/regonly.csrc/modules/chanmodes/regonlyspeak.csrc/modules/chanmodes/secret.csrc/modules/chanmodes/secureonly.csrc/modules/chanmodes/stripcolor.csrc/modules/chanmodes/topiclimit.csrc/modules/chanmodes/voice.csrc/modules/mode.csrc/modules/named-modes.csrc/modules/obbyscript.csrc/modules/targetfloodprot.csrc/modules/usermodes/bot.csrc/modules/usermodes/censor.csrc/modules/usermodes/noctcp.csrc/modules/usermodes/nokick.csrc/modules/usermodes/privacy.csrc/modules/usermodes/privdeaf.csrc/modules/usermodes/regonlymsg.csrc/modules/usermodes/secureonlymsg.csrc/modules/usermodes/servicebot.csrc/modules/usermodes/showwhois.csrc/modules/usermodes/wallops.csrc/modules/voice-channels.c
| /* IRCv3 draft/named-modes: name is required at registration so | ||
| * every chanmode is reachable via PROP. The legacy MODE letter | ||
| * (req.letter) is optional now -- 0 means a name-only mode. */ | ||
| if (BadPtr(req.name)) | ||
| { | ||
| unreal_log(ULOG_ERROR, "module", "CHANNEL_MODE_MISSING_NAME", NULL, | ||
| "CmodeAdd(): name is required (letter '$letter') from $module_name", | ||
| log_data_char("letter", req.letter ? req.letter : '?'), | ||
| log_data_string("module_name", module ? module->header->name : "<core>")); | ||
| if (module) | ||
| module->errorcode = MODERR_INVALID; | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
req.letter == 0 is accepted too early for current internals.
Lines 317-329 allow name-only registration now, but internals still assume letters exist (eg, Line 277 aborts on m->letter == '\0', and Line 613 skips unload when !cm->letter). This can cause unstable behavior for name-only modes.
Minimal safe guard for v1
if (BadPtr(req.name))
{
unreal_log(ULOG_ERROR, "module", "CHANNEL_MODE_MISSING_NAME", NULL,
"CmodeAdd(): name is required (letter '$letter') from $module_name",
log_data_char("letter", req.letter ? req.letter : '?'),
log_data_string("module_name", module ? module->header->name : "<core>"));
if (module)
module->errorcode = MODERR_INVALID;
return NULL;
}
+ if (!req.letter)
+ {
+ unreal_log(ULOG_ERROR, "module", "CHANNEL_MODE_NAME_ONLY_UNSUPPORTED", NULL,
+ "CmodeAdd(): name-only channel modes are not supported yet (name '$name')",
+ log_data_string("name", req.name));
+ if (module)
+ module->errorcode = MODERR_INVALID;
+ return NULL;
+ }🤖 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 `@src/api-channelmode.c` around lines 317 - 329, The code currently allows
name-only chanmode registration (req.letter == 0) but other internals (e.g.,
checks on m->letter and cm->letter) assume a non-zero letter; to avoid
instability, change CmodeAdd so it rejects name-only modes for now: add a guard
that treats req.letter == 0 as an error (log a clear message, set
module->errorcode = MODERR_INVALID if module present, and return NULL) similar
to the existing BadPtr(req.name) path; reference CmodeAdd, req.letter, m->letter
and cm->letter to locate the change.
| Umode *UmodeAdd(Module *module, const char *name, char ch, int global, int unset_on_deoper, int (*allowed)(Client *client, int what), long *mode) | ||
| { | ||
| Umode *um; | ||
| int existing = 0; | ||
|
|
||
| /* IRCv3 draft/named-modes: name is required, letter is optional | ||
| * (0 = mode reachable only by name via PROP). Refuse missing or | ||
| * empty names so we never silently advertise a mode without one. */ | ||
| if (BadPtr(name)) | ||
| { | ||
| unreal_log(ULOG_ERROR, "module", "USER_MODE_MISSING_NAME", NULL, | ||
| "UmodeAdd: name is required (letter '$letter') from $module_name", | ||
| log_data_char("letter", ch ? ch : '?'), | ||
| log_data_string("module_name", module ? module->header->name : "<core>")); | ||
| if (module) | ||
| module->errorcode = MODERR_INVALID; | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
Defensively reject ch == 0 until name-only modes are actually supported.
The new prototype allows ch=0 (PR description: "letter=0 reserved for future name-only modes"), but downstream code is not ready: usermode_add_sorted() aborts the process if it encounters any mode with letter == '\0' in the list, and umode_letter_to_handler[0] would be clobbered. If a third-party module misuses the new API and passes 0, the next UmodeAdd call brings the daemon down via abort().
Until the v1 limitation is lifted, fail closed at registration time with a clear log + MODERR_INVALID:
🛡️ Suggested guard
if (BadPtr(name))
{
unreal_log(ULOG_ERROR, "module", "USER_MODE_MISSING_NAME", NULL,
"UmodeAdd: name is required (letter '$letter') from $module_name",
log_data_char("letter", ch ? ch : '?'),
log_data_string("module_name", module ? module->header->name : "<core>"));
if (module)
module->errorcode = MODERR_INVALID;
return NULL;
}
+ /* Name-only user modes (ch == 0) are not yet supported by the
+ * registration/sort path -- usermode_add_sorted() will abort() on
+ * letter=='\0'. Reject here until that's lifted. */
+ if (ch == 0)
+ {
+ unreal_log(ULOG_ERROR, "module", "USER_MODE_NAME_ONLY_UNSUPPORTED", NULL,
+ "UmodeAdd: name-only user modes (no letter) are not yet supported (name '$name')",
+ log_data_string("name", name));
+ if (module)
+ module->errorcode = MODERR_INVALID;
+ return NULL;
+ }🤖 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 `@src/api-usermode.c` around lines 178 - 195, Reject registration attempts with
ch==0 in UmodeAdd: add a defensive check after the BadPtr(name) block that
treats ch == '\0' as invalid for now, call unreal_log with a clear message, set
module->errorcode = MODERR_INVALID if module is non-NULL, and return NULL; this
prevents downstream usermode_add_sorted() and the umode_letter_to_handler[0]
slot from being corrupted by a name-only mode until full support is implemented.
| ISupport *is_chantypes = ISupportFind("CHANTYPES"); | ||
| const char *chantypes = is_chantypes && is_chantypes->value | ||
| ? is_chantypes->value | ||
| : "#"; | ||
| if (!strchr(chantypes, *cname)) | ||
| return 0; |
There was a problem hiding this comment.
Empty channel names can now pass validation.
Line 950 uses strchr(chantypes, *cname) without guarding for *cname == '\0', so an empty name can be accepted. Add an explicit non-empty check before prefix validation.
Suggested fix
{
+ if (!cname || !*cname)
+ return 0;
+
ISupport *is_chantypes = ISupportFind("CHANTYPES");
const char *chantypes = is_chantypes && is_chantypes->value
? is_chantypes->value
: "#";
- if (!strchr(chantypes, *cname))
+ if (!strchr(chantypes, cname[0]))
return 0;
}📝 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.
| ISupport *is_chantypes = ISupportFind("CHANTYPES"); | |
| const char *chantypes = is_chantypes && is_chantypes->value | |
| ? is_chantypes->value | |
| : "#"; | |
| if (!strchr(chantypes, *cname)) | |
| return 0; | |
| { | |
| if (!cname || !*cname) | |
| return 0; | |
| ISupport *is_chantypes = ISupportFind("CHANTYPES"); | |
| const char *chantypes = is_chantypes && is_chantypes->value | |
| ? is_chantypes->value | |
| : "#"; | |
| if (!strchr(chantypes, cname[0])) | |
| return 0; | |
| } |
🤖 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 `@src/channel.c` around lines 946 - 951, The validation currently uses
strchr(chantypes, *cname) without checking for an empty channel name, allowing
"" to pass; modify the check that uses ISupportFind/chantypes and cname so you
first verify the name is non-empty (e.g., ensure *cname != '\0' / cname[0] !=
'\0') and return 0 for empty names before calling strchr for prefix validation.
| memset(&req, 0, sizeof(req)); | ||
| req.paracount = 0; | ||
| req.letter = 'P'; | ||
| req.name = "permanent"; |
There was a problem hiding this comment.
"permanent" should be "obby.world/permanent" — vendor prefix missing.
The +P permanent channel mode is an UnrealIRCd-specific extension not defined in any IRC or IRCv3 specification. The PR's stated naming convention is "spec-defined names unprefixed; Unreal-specific modes namespaced under obby.world/". All other Unreal-specific modes in this PR use the vendor prefix (obby.world/operonly, obby.world/issecure, obby.world/stripcolor, etc.). Leaving "permanent" unprefixed risks a future collision with a spec-defined name and exposes a bare unregistered token to cap-aware clients via PROP.
🐛 Proposed fix
- req.name = "permanent";
+ req.name = "obby.world/permanent";📝 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.
| req.name = "permanent"; | |
| req.name = "obby.world/permanent"; |
🤖 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 `@src/modules/chanmodes/permanent.c` at line 79, Update the vendor prefix for
the permanent channel mode: in src/modules/chanmodes/permanent.c where the
request name is set (the assignment to req.name), change the unprefixed literal
"permanent" to the namespaced token "obby.world/permanent" so this
UnrealIRCd-specific +P mode follows the PR's vendor-prefix convention and avoids
colliding with spec-defined names exposed via PROP.
| memset(&req, 0, sizeof(req)); | ||
| req.paracount = 0; | ||
| req.letter = 'z'; | ||
| req.name = "secureonly"; |
There was a problem hiding this comment.
"secureonly" should be "obby.world/secureonly" to match the vendor-namespacing convention.
+z (secureonly) is an UnrealIRCd-specific extension, not defined in any standard RFC. Within this very PR, the equivalent Unreal-specific +K (noknock) is correctly registered as "obby.world/noknock" in noknock.c. Using an unprefixed name for secureonly is inconsistent with that convention and risks collision with any future spec-defined mode named "secureonly".
🐛 Proposed fix
- req.name = "secureonly";
+ req.name = "obby.world/secureonly";📝 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.
| req.name = "secureonly"; | |
| req.name = "obby.world/secureonly"; |
🤖 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 `@src/modules/chanmodes/secureonly.c` at line 53, The mode registration uses an
unprefixed name; change the req.name assignment in secureonly.c to use the
vendor namespaced string "obby.world/secureonly" instead of "secureonly" so it
matches the convention used for other UnrealIRCd-specific modes (see noknock.c)
and avoids future collisions; update the req.name initializer in the
registration code that currently sets req.name = "secureonly" to req.name =
"obby.world/secureonly".
| char modebuf[64] = ""; | ||
| char parabuf[BUFSIZE] = ""; | ||
| char what = '+'; | ||
| const char *mode_argv[8]; | ||
| int mode_argc = 0; | ||
| size_t modelen = 0, paralen = 0; |
There was a problem hiding this comment.
Pass MODE params as separate argv entries.
do_mode() needs the same argv shape as a parsed MODE command, but this path concatenates every PROP parameter into one parabuf and passes it as a single argument. Multi-parameter updates like +op=alice +op=bob or mixed param modes will execute incorrectly because only the first parameter can be parsed in the right slot.
Also applies to: 631-655
🤖 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 `@src/modules/named-modes.c` around lines 540 - 545, The current code builds
one concatenated parameter string (parabuf) and passes it as a single argv
element to do_mode(), which breaks multi-parameter PROP updates; change the
logic that fills parabuf and mode_argv so each PROP parameter becomes its own
argv entry: instead of appending into parabuf, allocate/point separate
null-terminated strings for each parameter and push them into mode_argv
(incrementing mode_argc) so do_mode() sees the same argv shape as a parsed MODE
command; keep using modebuf for the mode string and ensure you stop at the
mode_argv capacity (mode_argv[8]) and validate lengths (modelen/paralen) per
entry, applying the same fix in the second block referenced (around the other
occurrence).
| MOD_INIT() | ||
| { | ||
| UmodeAdd(modinfo->handle, 'B', UMODE_GLOBAL, 0, NULL, &UMODE_BOT); | ||
| UmodeAdd(modinfo->handle, "bot", 'B', UMODE_GLOBAL, 0, NULL, &UMODE_BOT); |
There was a problem hiding this comment.
Handle UmodeAdd() failure before completing init.
At Line 49, mode registration failure is not handled. If this fails, the module still loads with UMODE_BOT == 0, silently disabling bot-mode behavior.
Suggested fix
MOD_INIT()
{
- UmodeAdd(modinfo->handle, "bot", 'B', UMODE_GLOBAL, 0, NULL, &UMODE_BOT);
+ Umode *um = UmodeAdd(modinfo->handle, "bot", 'B', UMODE_GLOBAL, 0, NULL, &UMODE_BOT);
+ if (!um)
+ {
+ config_error("bot: Could not add usermode 'B': %s", ModuleGetErrorStr(modinfo->handle));
+ return MOD_FAILED;
+ }
ISupportAdd(modinfo->handle, "BOT", "B");📝 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.
| UmodeAdd(modinfo->handle, "bot", 'B', UMODE_GLOBAL, 0, NULL, &UMODE_BOT); | |
| MOD_INIT() | |
| { | |
| Umode *um = UmodeAdd(modinfo->handle, "bot", 'B', UMODE_GLOBAL, 0, NULL, &UMODE_BOT); | |
| if (!um) | |
| { | |
| config_error("bot: Could not add usermode 'B': %s", ModuleGetErrorStr(modinfo->handle)); | |
| return MOD_FAILED; | |
| } | |
| ISupportAdd(modinfo->handle, "BOT", "B"); | |
| } |
🤖 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 `@src/modules/usermodes/bot.c` at line 49, UmodeAdd("bot", 'B',
UMODE_GLOBAL...) return value is not checked, so if registration fails UMODE_BOT
remains 0 and the module continues initializing; update the init path to capture
the return of UmodeAdd(modinfo->handle, "bot", 'B', UMODE_GLOBAL, 0, NULL,
&UMODE_BOT), check for failure, log an error via the module/process logger
including the fact that bot mode registration failed and the returned
value/state, and abort module initialization (e.g., return MODULE_FAIL or call
the appropriate ModuleFailed/cleanup routine used elsewhere) so the module does
not load with UMODE_BOT == 0. Ensure any subsequent code using UMODE_BOT only
runs after successful registration.
| @@ -45,7 +45,7 @@ MOD_INIT() | |||
| { | |||
| CmodeInfo req; | |||
There was a problem hiding this comment.
Remove stale CmodeInfo req; — dead copy-paste artifact.
This is a usermode module; CmodeInfo is the channel-mode registration struct and is never referenced anywhere in this MOD_INIT(). The UmodeAdd call at line 48 uses no local struct at all. This was almost certainly copied from a chanmodes/ module and left behind.
🐛 Proposed fix
MOD_INIT()
{
-CmodeInfo req;
-
UmodeAdd(modinfo->handle, "obby.world/noctcp", 'T', UMODE_GLOBAL, 0, NULL, &UMODE_NOCTCP);📝 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.
| CmodeInfo req; | |
| MOD_INIT() | |
| { | |
| UmodeAdd(modinfo->handle, "obby.world/noctcp", 'T', UMODE_GLOBAL, 0, NULL, &UMODE_NOCTCP); |
🤖 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 `@src/modules/usermodes/noctcp.c` at line 46, Remove the unused dead variable
CmodeInfo req from the module initialization; it’s a leftover copy-paste and not
referenced anywhere in MOD_INIT() or by UmodeAdd, so delete the declaration
`CmodeInfo req;` (leaving UmodeAdd and the rest of MOD_INIT untouched) to avoid
stale/unreferenced code.
| MOD_INIT() | ||
| { | ||
| UmodeAdd(modinfo->handle, 'p', UMODE_GLOBAL, 0, umode_allow_all, &UMODE_PRIVACY); | ||
| UmodeAdd(modinfo->handle, "hidechans", 'p', UMODE_GLOBAL, 0, umode_allow_all, &UMODE_PRIVACY); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
IRCv3 draft named-modes specification user mode names hidechans privacy
💡 Result:
IRCv3 “named-modes” draft and the “hidechans” privacy user mode: 1) IRCv3 named-modes draft (user mode names) - There is an IRCv3-specifications repository pull request titled “Add named-modes specification” (branch “named-modes”, PR #484) that describes a “named-modes” specification intended to use long-form mode names in place of short mode characters. [1] - The PR is shown as “State: open” in the source. [1] - The PR discusses server translation of MODE messages to PROP messages (i.e., the mechanism used to carry long-form/named mode operations), indicating that this draft is about interoperable named-mode semantics rather than just documentation. [1] 2) hidechans privacy - The InspIRCd “hidechans” module adds user mode I (hidechans) whose purpose is to hide the channels a user is in from their /WHOIS response. [2] - InspIRCd’s “hidechans” documentation (v4) describes the same behavior: user mode I hides the channels users are in from their /WHOIS response (with configuration options such as affectsopers and hideservices). [3] Relationship between them (what your query is asking) - “hidechans” is a specific server implementation’s user mode (InspIRCd) that affects privacy of channel lists in WHOIS. [2][3] - The “named-modes” draft is about long-form/named mode identifiers (PROP/RPL_* structures, translating MODE to PROP), but the sources found here do not state that it specifically targets the hidechans user mode or that it provides a standardized long name for “I hidechans”. [1] Answering the core of your query - The “named-modes” work you’re looking for exists as the open IRCv3-specifications pull request #484 (“Add named-modes specification”). [1] - The privacy user mode “hidechans” (mode character I) is defined in InspIRCd as hiding channels from a user’s /WHOIS response. [2][3]
Citations:
- 1: Add named-modes specification ircv3/ircv3-specifications#484
- 2: https://docs.inspircd.org/3/modules/hidechans
- 3: https://docs.inspircd.org/4/modules/hidechans/
🏁 Script executed:
# Identify the repository/IRCd
git remote -vRepository: ObsidianIRC/ObbyIRCd
Length of output: 1901
🏁 Script executed:
# Check the file and surrounding context to understand vendor prefix patterns
head -50 src/modules/usermodes/privacy.c | cat -nRepository: ObsidianIRC/ObbyIRCd
Length of output: 1901
🏁 Script executed:
# Look for other user mode definitions and their naming patterns
rg "UmodeAdd" src/modules/usermodes/ -A 1 -B 1 | head -40Repository: ObsidianIRC/ObbyIRCd
Length of output: 1901
🏁 Script executed:
# Search for vendor prefix examples in codebase
rg '"[a-z0-9]+\.[a-z0-9]+/' src/ --max-count=5Repository: ObsidianIRC/ObbyIRCd
Length of output: 1901
🌐 Web query:
IRCv3 draft named-modes specification standard user modes list
💡 Result:
IRCv3 “named-modes” (PROP/RPL_*PROPLIST) does not define a fixed “standard user modes list”. Instead, it standardizes the protocol syntax/semantics for setting modes by their long names, and how those modes are represented/listed using IRCv3 numerics/messages. 1) What “named-modes” standardizes - The open IRCv3 “named-modes specification” is a draft meant to let clients use long mode names (not only mode letters) when interacting with an IRC server [1]. - In InspIRCd, the corresponding module exposes a command named PROP that “allows users to add, remove, and view the modes of a specific target” using names (long names) and optional values [2]. 2) Why there is no “standard user modes list” in the draft - IRC has a general concept of user modes where the actual mode set (letters/meaning/availability) is not universally defined by IRCv3; it depends on the IRC server/daemon’s implementation and/or network. As a result, a named-modes draft cannot sensibly publish a single authoritative “standard user modes list” the way it could for its own protocol elements. - The closest “standardization” in IRCv3 that explicitly ties to “user modes” is example extensions like BOT MODE, which standardizes how bots can mark themselves as bots using a user mode character provided by the BOT ISUPPORT token (i.e., the mode character is server-provided via ISUPPORT) [3]. This is not a complete user-mode catalog; it’s one standardized behavior that uses a user mode. 3) What you can treat as “the list” related to IRCv3 in practice - For IRCv3 named modes syntax, the operational “listing” is defined via the PROP-related numerics/commands in the named-modes draft and implementations (e.g., PROP in InspIRCd) [2]. - For specific “standard” user-mode-like behavior, consult individual IRCv3 specs/extensions (e.g., BOT MODE) rather than expecting a global user mode letter→meaning list [3]. Answer to your query: There is no authoritative IRCv3 draft “named-modes specification” standard user-modes list; named-modes is about a standardized way to represent/set/list modes by long name (via PROP and related listing), while the actual user modes and their meanings are defined by the underlying IRC daemon/network implementation [1][2][3].
Citations:
- 1: Add named-modes specification ircv3/ircv3-specifications#484
- 2: https://docs.inspircd.org/3/modules/namedmodes
- 3: https://ircv3.github.io/specs/extensions/bot-mode.html
🌐 Web query:
IRCv3 named-modes "hidechans" standard mode
💡 Result:
IRCv3 does not define a standard named-modes mode called “hidechans”. The string “hidechans” refers to an InspIRCd user-mode module that implements user mode “I” (hidechans), which hides the channels the user is in from their /WHOIS response. [1][2] While IRCv3 has a “named-modes” work-in-progress proposal/specification referenced in the ircv3/ircv3-specifications repository, the sources found here do not indicate that “hidechans” is part of any ratified IRCv3 named-modes standard. [3][4] So, “IRCv3 named-modes ‘hidechans’ standard mode” appears to be a category error: it’s an ircd-specific module-mode name (InspIRCd) rather than an IRCv3 standard named mode.
Citations:
- 1: https://docs.inspircd.org/4/modules/hidechans/
- 2: https://docs.inspircd.org/3/modules/hidechans
- 3: Add named-modes specification ircv3/ircv3-specifications#484
- 4: https://ircv3.github.io/irc/
🌐 Web query:
IRCv3 user mode names privacy "draft-hardy-irc-isupport-parameter"
💡 Result:
There is no single IRCv3 “user mode names privacy” setting in draft-hardy-irc-isupport-00. What draft-hardy-irc-isupport-00 actually is: an IRC RPL_ISUPPORT (005) mechanism for servers to advertise supported features (tokens) to clients; it does not define a general privacy mode name mechanism. The draft describes that RPL_ISUPPORT is a list of “token SP …:are supported by this server” entries using the 005 numeric format [1][2]. It also defines how token/value pairs (e.g., “PARAMETER=VALUE”) are represented [1][2]. What “privacy” relates to in ISUPPORT/user modes: 1) ISUPPORT tokens are named server features (including some related to information hiding), not a privacy framework. For example, the ISUPPORT token “BOT” exists to standardize the user mode character used for bot marking (BOT=) [3][4]. 2) Information hiding behavior is implemented via server-specific user modes (e.g., modes that hide channels/WHOIS info). Those are not standardized by draft-hardy-irc-isupport-00 as a generic “privacy user mode names” registry. (For instance, UnrealIRCd documents a user mode “p” described as hiding channels a user is in from /WHOIS) [5]. So: searching specifically for “IRCv3 user mode names privacy draft-hardy-irc-isupport-parameter” does not yield a defined parameter or capability in the draft; instead, privacy-related user mode letters/meanings are defined by ircds, while ISUPPORT provides a way to advertise which features are supported and, for some extensions, which mode characters to use (like BOT) [3][4]. If you tell me which ircd (e.g., InspIRCd, UnrealIRCd, ircd-hybrid) and which exact privacy behavior you mean (hide WHOIS channels? cloak hostnames? hide oper status?), I can map it to the corresponding user mode(s) and—where standardized—any ISUPPORT tokens used to advertise them.
Citations:
- 1: https://datatracker.ietf.org/doc/html/draft-hardy-irc-isupport-00
- 2: https://www.ietf.org/archive/id/draft-hardy-irc-isupport-00.txt
- 3: https://ircv3.net/specs/extensions/bot-mode
- 4: https://ircv3.net/registry.html
- 5: https://www.unrealircd.org/docs/User_modes
Add vendor prefix to "hidechans" user mode name.
The "hidechans" mode is not defined in the IRCv3 draft/named-modes specification. It is a vendor-specific implementation. Per the codebase convention of prefixing vendor modes (e.g., "obby.world/..."), this mode name should use the appropriate vendor prefix rather than the bare unprefixed form.
🤖 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 `@src/modules/usermodes/privacy.c` at line 42, The user mode registration call
UmodeAdd(modinfo->handle, "hidechans", 'p', UMODE_GLOBAL, 0, umode_allow_all,
&UMODE_PRIVACY) uses an unprefixed vendor-specific name; change the mode name
string to include the project's vendor prefix (e.g. "obby.world/hidechans" or
the correct vendor namespace used elsewhere) so it matches the codebase
convention, leaving the rest of the UmodeAdd arguments (modinfo->handle, 'p',
UMODE_GLOBAL, umode_allow_all, &UMODE_PRIVACY) unchanged.
| MOD_INIT() | ||
| { | ||
| UmodeAdd(modinfo->handle, 'R', UMODE_GLOBAL, 0, umode_allow_all, &UMODE_REGONLYMSG); | ||
| UmodeAdd(modinfo->handle, "obby.world/regonlymsg", 'R', UMODE_GLOBAL, 0, umode_allow_all, &UMODE_REGONLYMSG); |
There was a problem hiding this comment.
Check UmodeAdd() result before proceeding.
At Line 42, failure from UmodeAdd() is ignored. This can leave the module loaded with an unset mode bit and degraded behavior.
Suggested fix
MOD_INIT()
{
- UmodeAdd(modinfo->handle, "obby.world/regonlymsg", 'R', UMODE_GLOBAL, 0, umode_allow_all, &UMODE_REGONLYMSG);
+ Umode *um = UmodeAdd(modinfo->handle, "obby.world/regonlymsg", 'R', UMODE_GLOBAL, 0, umode_allow_all, &UMODE_REGONLYMSG);
+ if (!um)
+ {
+ config_error("regonlymsg: Could not add usermode 'R': %s", ModuleGetErrorStr(modinfo->handle));
+ return MOD_FAILED;
+ }
HookAdd(modinfo->handle, HOOKTYPE_CAN_SEND_TO_USER, 0, regonlymsg_can_send_to_user);📝 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.
| UmodeAdd(modinfo->handle, "obby.world/regonlymsg", 'R', UMODE_GLOBAL, 0, umode_allow_all, &UMODE_REGONLYMSG); | |
| MOD_INIT() | |
| { | |
| Umode *um = UmodeAdd(modinfo->handle, "obby.world/regonlymsg", 'R', UMODE_GLOBAL, 0, umode_allow_all, &UMODE_REGONLYMSG); | |
| if (!um) | |
| { | |
| config_error("regonlymsg: Could not add usermode 'R': %s", ModuleGetErrorStr(modinfo->handle)); | |
| return MOD_FAILED; | |
| } | |
| HookAdd(modinfo->handle, HOOKTYPE_CAN_SEND_TO_USER, 0, regonlymsg_can_send_to_user); |
🤖 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 `@src/modules/usermodes/regonlymsg.c` at line 42, UmodeAdd(...) return value is
ignored; change the module init to check the result of UmodeAdd(modinfo->handle,
"obby.world/regonlymsg", 'R', UMODE_GLOBAL, 0, umode_allow_all,
&UMODE_REGONLYMSG) and if it fails, log an error (including modinfo->handle and
UMODE_REGONLYMSG for context) and abort module initialization by returning a
failure code so the module does not remain loaded with an unset mode bit.
Spec: clients MUST negotiate draft/named-modes before receiving the new numerics. Previously the welcome hook fired 964/965 to every client unconditionally, leaking them to plain IRC clients that have no idea what those numerics mean.
Vendor rename:
Every chanmode + usermode that we vendored under "obby.world/"
now lives under "obsidianirc/" (cmodes: floodprot, nokick,
nonickchange, delayjoin{,-rejoinhide}, link, history, ..., umodes:
bot, deaf, hideoper, hideidle, regnick, sethost, secureconn,
privdeaf, censor, regonlymsg, secureonlymsg, servicebot,
showwhois, noctcp). strip_vendor() in named-modes.c follows.
v1 limitation fixes:
- User-mode self-MODE -> PROP. send_umode_out() in src/user.c now
checks ClientCapabilityBit("draft/named-modes"); cap-holders see
":nick PROP nick +invisible -wallops" instead of the legacy
":nick MODE nick :+i-w". Letter->name lookup walks the live
usermodes list since umode_letter_to_handler[] is file-local.
- Name-only mode executor. cmd_prop now applies type-4 (flag)
CMODE_NORMAL modes that have cm->letter == 0 directly: it runs
the cm->is_ok permission check, skips no-op transitions, toggles
channel->mode.mode, and broadcasts ":src PROP #chan +foo" to
cap-holders only (no MODE-letter equivalent exists). List/param
name-only modes still ERR_UNKNOWNMODE -- nobody has registered
one yet, so cross that bridge when we get there.
- List-mode listing is now table-driven via offsetof(Channel,
banlist|exlist|invexlist), so future vendored list-modes plug in
with one line. Vendor-prefixed forms (e.g. "obsidianirc/ban")
are recognised via strip_vendor().
Numeric defines move out of the named-modes module:
- 960-965 RPL_*PROPLIST / RPL_CHMODELIST / RPL_UMODELIST land
next to the SASL block in include/numeric.h, the canonical
home for IRC numerics.
- STR_RPL_ENDOFPROPLIST / STR_RPL_LISTPROPLIST /
STR_RPL_ENDOFLISTPROPLIST get matching STR_* entries for any
sendnumeric() callers that want them. The three variable-
length lists (961/964/965) stay direct-formatted in the
module because they need custom buffering.
Mode-by-name lookup is genuinely useful beyond named-modes (think
scripts, future RPC handlers, anything that needs to resolve
"obsidianirc/floodprot" -> Cmode*). Promote the helpers to the
core mode APIs:
- find_channel_mode_handler_by_name() in api-channelmode.c
- find_user_mode_handler_by_name() in api-usermode.c
- both extern'd in struct.h next to CmodeAdd / UmodeAdd
The named-modes module's local copies are deleted; cmd_prop calls
the public API. Module-local helpers that ARE named-modes-specific
(prop_classify_chanmode, strip_vendor, extra_chmodes[] table) stay
where they are -- they encode spec-table mappings, not general
infrastructure.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/modules/named-modes.c (2)
547-709:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass translated MODE params as separate argv entries.
do_mode()is called with every translated PROP parameter packed into oneparabufargument. A request likePROP#chan+op=alice +op=bobtherefore reaches MODE as+oowith one"alice bob"parameter, so anything after the first parameter is parsed in the wrong slot. Reusemode_argvto push each parameter as its own argv element before callingdo_mode().🤖 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 `@src/modules/named-modes.c` around lines 547 - 709, The translated MODE parameters are being combined into a single parabuf string so do_mode receives one argv with all params (causing misaligned parameter parsing); instead, for each param (where eq && eq[1]) append the parameter as a separate entry into the existing mode_argv array (use mode_argv[mode_argc++] = strdup or a safe pointer into parabuf after copying, ensure bounds like mode_argc < 7 and parabuf space checks using paralen and modelen), null-terminate mode_argv[mode_argc] and then call do_mode(channel, client, recv_mtags, mode_argc - 1, mode_argv + 1, 0, 0) (remove the fake_argv usage), so each translated name/value becomes its own argv element for do_mode to parse correctly.
69-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRelay remote chanmode updates too.
Only
HOOKTYPE_LOCAL_CHANMODEis registered. Server-originated channel mode changes can still be hidden fromdraft/named-modesclients by the legacy MODE filter, so local cap-holders on this server miss remote updates unless the same relay runs for remote events as well.🤖 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 `@src/modules/named-modes.c` around lines 69 - 71, The module only registers HOOKTYPE_LOCAL_CHANMODE, so server-originated (remote) channel mode changes aren't relayed; add a HookAdd registration for the server chanmode hook (e.g., HOOKTYPE_CHANMODE) using the same handler used for local changes (named_modes_local_chanmode) on modinfo->handle so remote MODE events are also passed to the draft/named-modes relay; update the hook registration block that currently calls HookAdd(..., HOOKTYPE_LOCAL_CHANMODE, ..., named_modes_local_chanmode) to additionally call HookAdd(..., HOOKTYPE_CHANMODE, 0, named_modes_local_chanmode).
🤖 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 `@src/modules/named-modes.c`:
- Around line 626-676: The code currently applies name-only channel modes by
directly mutating channel->mode.mode (in the block where cm->letter == 0), which
bypasses the replicated MODE pipeline and S2S propagation; instead, either route
the change through the core mode executor or reject name-only modes until that
executor exists: replace the direct bit-twiddling and local PROP emission (the
code manipulating channel->mode.mode, new_message/recv_mtags, sendto_channel
with CAP_NAMED_MODES and free_message_tags) with a call into the replicated mode
handling path (the same executor used for lettered modes) so the access check
(cm->is_ok/EXCHK_ACCESS_ERR), MODE_ADD/MODE_DEL handling, and propagation are
preserved, or return ERR_UNKNOWNMODE/another appropriate error for cm->letter ==
0 to disallow name-only channel modes until a replicated executor is
implemented.
In `@src/user.c`:
- Around line 448-517: The current named-modes PROP logic rebuilds changes by
reparsing buf from build_umode_string and therefore fails when a Umode uses
letter=0; instead, compute the delta by comparing the old umode state (old)
against the current client->umodes set and emit name-based +/− entries for modes
whose presence changed. In practice: in the branch that checks
ClientCapabilityBit("draft/named-modes")/HasCapabilityFast, iterate the global
usermodes list (usermodes) and for each Umode with a non-NULL name compare
whether it was set in old versus set in client->umodes, choose '+' or '-'
accordingly, append "±name" items to propbuf and flush/send PROP lines exactly
as the existing code does (reusing propbuf, plen, sendto_one) — do not rely on
buf or mode letters and handle empty-name/letter cases per the UmodeAdd(...,
letter = 0) contract.
---
Duplicate comments:
In `@src/modules/named-modes.c`:
- Around line 547-709: The translated MODE parameters are being combined into a
single parabuf string so do_mode receives one argv with all params (causing
misaligned parameter parsing); instead, for each param (where eq && eq[1])
append the parameter as a separate entry into the existing mode_argv array (use
mode_argv[mode_argc++] = strdup or a safe pointer into parabuf after copying,
ensure bounds like mode_argc < 7 and parabuf space checks using paralen and
modelen), null-terminate mode_argv[mode_argc] and then call do_mode(channel,
client, recv_mtags, mode_argc - 1, mode_argv + 1, 0, 0) (remove the fake_argv
usage), so each translated name/value becomes its own argv element for do_mode
to parse correctly.
- Around line 69-71: The module only registers HOOKTYPE_LOCAL_CHANMODE, so
server-originated (remote) channel mode changes aren't relayed; add a HookAdd
registration for the server chanmode hook (e.g., HOOKTYPE_CHANMODE) using the
same handler used for local changes (named_modes_local_chanmode) on
modinfo->handle so remote MODE events are also passed to the draft/named-modes
relay; update the hook registration block that currently calls HookAdd(...,
HOOKTYPE_LOCAL_CHANMODE, ..., named_modes_local_chanmode) to additionally call
HookAdd(..., HOOKTYPE_CHANMODE, 0, named_modes_local_chanmode).
🪄 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: a6566cf8-3bf3-4128-87d4-962b4644a98f
📒 Files selected for processing (32)
include/modules.hinclude/numeric.hinclude/struct.hsrc/api-channelmode.csrc/api-usermode.csrc/modules/chanmodes/censor.csrc/modules/chanmodes/delayjoin.csrc/modules/chanmodes/floodprot.csrc/modules/chanmodes/history.csrc/modules/chanmodes/isregistered.csrc/modules/chanmodes/issecure.csrc/modules/chanmodes/link.csrc/modules/chanmodes/nocolor.csrc/modules/chanmodes/noinvite.csrc/modules/chanmodes/nokick.csrc/modules/chanmodes/noknock.csrc/modules/chanmodes/nonickchange.csrc/modules/chanmodes/nonotice.csrc/modules/chanmodes/operonly.csrc/modules/chanmodes/regonlyspeak.csrc/modules/chanmodes/stripcolor.csrc/modules/named-modes.csrc/modules/obbyscript.csrc/modules/usermodes/censor.csrc/modules/usermodes/noctcp.csrc/modules/usermodes/nokick.csrc/modules/usermodes/privdeaf.csrc/modules/usermodes/regonlymsg.csrc/modules/usermodes/secureonlymsg.csrc/modules/usermodes/servicebot.csrc/modules/usermodes/showwhois.csrc/user.c
✅ Files skipped from review due to trivial changes (1)
- include/modules.h
🚧 Files skipped from review as they are similar to previous changes (19)
- src/modules/usermodes/servicebot.c
- src/modules/chanmodes/noinvite.c
- src/modules/chanmodes/nokick.c
- src/modules/chanmodes/noknock.c
- src/modules/usermodes/regonlymsg.c
- src/modules/chanmodes/history.c
- src/modules/chanmodes/delayjoin.c
- src/modules/chanmodes/link.c
- src/modules/usermodes/privdeaf.c
- src/modules/chanmodes/nonotice.c
- src/modules/chanmodes/regonlyspeak.c
- include/struct.h
- src/modules/chanmodes/nocolor.c
- src/modules/usermodes/censor.c
- src/modules/chanmodes/isregistered.c
- src/api-channelmode.c
- src/modules/chanmodes/stripcolor.c
- src/api-usermode.c
- src/modules/chanmodes/nonickchange.c
| if (!cm->letter) | ||
| { | ||
| /* Name-only mode (no legacy letter). The | ||
| * cmd_mode dispatcher is letter-keyed so we | ||
| * can't route through it; instead apply the | ||
| * change directly here. Only flag (type 4) | ||
| * modes are handled this way -- list/param | ||
| * modes still need a letter or a richer | ||
| * non-letter executor (TODO if/when we | ||
| * register name-only param modes). */ | ||
| int cls = prop_classify_chanmode(cm); | ||
| int mode_change_what = | ||
| (sign == '+') ? MODE_ADD : MODE_DEL; | ||
| if (cls != 4 || cm->type != CMODE_NORMAL) | ||
| { | ||
| sendnumeric(client, ERR_UNKNOWNMODE, '?'); | ||
| continue; | ||
| } | ||
| if (cm->is_ok) | ||
| { | ||
| int access = cm->is_ok(client, channel, 0, | ||
| NULL, EXCHK_ACCESS_ERR, | ||
| mode_change_what); | ||
| if (access != EX_ALLOW) | ||
| continue; | ||
| } | ||
| /* No-op? */ | ||
| if (mode_change_what == MODE_ADD && | ||
| (channel->mode.mode & cm->mode)) | ||
| continue; | ||
| if (mode_change_what == MODE_DEL && | ||
| !(channel->mode.mode & cm->mode)) | ||
| continue; | ||
| if (mode_change_what == MODE_ADD) | ||
| channel->mode.mode |= cm->mode; | ||
| else | ||
| channel->mode.mode &= ~cm->mode; | ||
| /* Echo PROP back to all cap-holders directly | ||
| * (no MODE-letter equivalent exists). */ | ||
| { | ||
| MessageTag *mt = NULL; | ||
| new_message(client, recv_mtags, &mt); | ||
| sendto_channel(channel, client, NULL, 0, | ||
| CAP_NAMED_MODES, SEND_LOCAL, | ||
| mt, | ||
| ":%s PROP %s %c%s", | ||
| client->name, channel->name, | ||
| sign, cm->name); | ||
| free_message_tags(mt); | ||
| } | ||
| continue; |
There was a problem hiding this comment.
Don't apply name-only channel modes by mutating channel->mode.mode directly.
This path bypasses the normal mode pipeline and only emits a local PROP. That skips server-to-server propagation and any side effects wired off regular MODE handling, so channel state can diverge as soon as a letter == 0 mode is used. Route this through a core replicated executor, or reject name-only channel modes until that path exists.
🤖 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 `@src/modules/named-modes.c` around lines 626 - 676, The code currently applies
name-only channel modes by directly mutating channel->mode.mode (in the block
where cm->letter == 0), which bypasses the replicated MODE pipeline and S2S
propagation; instead, either route the change through the core mode executor or
reject name-only modes until that executor exists: replace the direct
bit-twiddling and local PROP emission (the code manipulating channel->mode.mode,
new_message/recv_mtags, sendto_channel with CAP_NAMED_MODES and
free_message_tags) with a call into the replicated mode handling path (the same
executor used for lettered modes) so the access check
(cm->is_ok/EXCHK_ACCESS_ERR), MODE_ADD/MODE_DEL handling, and propagation are
preserved, or return ERR_UNKNOWNMODE/another appropriate error for cm->letter ==
0 to disallow name-only channel modes until a replicated executor is
implemented.
| build_umode_string(client, old, ALL_UMODES, buf); | ||
| if (*buf) | ||
| sendto_one(client, NULL, ":%s MODE %s :%s", client->name, client->name, buf); | ||
| { | ||
| /* IRCv3 draft/named-modes: cap-holders see PROP with | ||
| * the long-form mode names instead of MODE. The umode | ||
| * name lookup walks the live registry so vendored | ||
| * names ("obsidianirc/...") come through correctly. */ | ||
| long named_modes_bit = | ||
| ClientCapabilityBit("draft/named-modes"); | ||
| if (named_modes_bit && | ||
| HasCapabilityFast(client, named_modes_bit)) | ||
| { | ||
| char propbuf[BUFSIZE]; | ||
| const char *p = buf; | ||
| char what = '+'; | ||
| size_t plen = 0; | ||
| propbuf[0] = '\0'; | ||
| for (; *p; p++) | ||
| { | ||
| Umode *um; | ||
| char item[80]; | ||
| size_t ilen; | ||
| if (*p == '+' || *p == '-') | ||
| { | ||
| what = *p; | ||
| continue; | ||
| } | ||
| /* Walk usermodes list to find handler by | ||
| * letter (umode_letter_to_handler[] is | ||
| * file-local in api-usermode.c). The | ||
| * list is short -- one pass is fine. */ | ||
| { | ||
| Umode *cand; | ||
| um = NULL; | ||
| for (cand = usermodes; cand; cand = cand->next) | ||
| { | ||
| if (!cand->unloaded && cand->letter == *p) | ||
| { | ||
| um = cand; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| if (!um || !um->name) | ||
| continue; | ||
| snprintf(item, sizeof(item), "%c%s", | ||
| what, um->name); | ||
| ilen = strlen(item); | ||
| if (plen + ilen + 2 >= sizeof(propbuf)) | ||
| { | ||
| sendto_one(client, NULL, | ||
| ":%s PROP %s %s", | ||
| client->name, | ||
| client->name, | ||
| propbuf); | ||
| propbuf[0] = '\0'; | ||
| plen = 0; | ||
| } | ||
| if (plen) | ||
| propbuf[plen++] = ' '; | ||
| strlcpy(propbuf + plen, item, | ||
| sizeof(propbuf) - plen); | ||
| plen += ilen; | ||
| } | ||
| if (plen) | ||
| sendto_one(client, NULL, | ||
| ":%s PROP %s %s", | ||
| client->name, | ||
| client->name, propbuf); | ||
| } |
There was a problem hiding this comment.
Don't derive named-mode PROP from the letter-only delta.
This branch still reparses buf from build_umode_string(). With the new UmodeAdd(..., letter = 0) contract, a name-only umode change collapses to just "+" / "-" here, so no PROP item is emitted and the client misses its own update. Build the named delta by comparing old against client->umodes directly instead of translating back from letters.
🤖 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 `@src/user.c` around lines 448 - 517, The current named-modes PROP logic
rebuilds changes by reparsing buf from build_umode_string and therefore fails
when a Umode uses letter=0; instead, compute the delta by comparing the old
umode state (old) against the current client->umodes set and emit name-based +/−
entries for modes whose presence changed. In practice: in the branch that checks
ClientCapabilityBit("draft/named-modes")/HasCapabilityFast, iterate the global
usermodes list (usermodes) and for each Umode with a non-NULL name compare
whether it was set in old versus set in client->umodes, choose '+' or '-'
accordingly, append "±name" items to propbuf and flush/send PROP lines exactly
as the existing code does (reusing propbuf, plen, sendto_one) — do not rely on
buf or mode letters and handle empty-name/letter cases per the UmodeAdd(...,
letter = 0) contract.
…GISTER
This swaps the upstream UnrealIRCd prefix chanmodes (chanowner/admin/op/
halfop/voice + topiclimit) for a single configurable third/member-roles
module ported from Valware's upstream, and wires the +P channeldb to
persist channel-registration metadata so /CREGISTER can make a channel
durable + grant chanowner via extban.
Pieces:
* src/modules/third/member-roles.c -- port of valware-unrealircd-mods/
member-roles, with three obbyircd-specific changes:
1. CmodeAdd is called inside CONFIGRUN (per role) instead of
MOD_LOAD, so PREFIX/STATUSMSG/CHANMODES advertise the modes
from the first connection. Upstream MOD_LOAD timing meant the
linked list was still empty.
2. CmodeInfo.name is set from the role's config block name so
RPL_CHMODELIST advertises the IRCv3 named-modes long name
(e.g. "+q owner") -- pairs with feat/named-modes.
3. CREGISTER <#chan>: requires a logged-in services account,
sets +P, stamps the channeldb-owned obsidianirc/channel-
registration ModData, grants the highest-rank role to the
caller, and adds +e ~automode:<mode>:~account:<owner> so
re-joins re-promote.
* src/modules/channeldb.c -- bumped CHANNELDB_VERSION 100 -> 101,
added a MODDATATYPE_CHANNEL named obsidianirc/channel-registration
(account name + timestamp), and persisted the two new fields per
+P channel after the existing mode_lock. v100 dbs still load fine
(the new fields are gated on version >= 101).
* doc/conf/examples/member-roles.conf -- default config defining
the five obbyircd prefix roles (q ~ owner=4000, a & admin=3000,
o @ op=2000, h % halfop=1000, v + voice=-1) with permissions that
line up with the legacy chanmodes those roles replace, plus a
"default" block restricting list-viewing for users with no role.
* doc/conf/modules.default.conf -- commented out the five prefix
loadmodule lines + chanmodes/topiclimit, added "loadmodule
third/member-roles" + include of the example config above.
* doc/conf/examples/example.conf -- modes-on-join "+nt" -> "+n"
(topiclimit is gone; topic permissioning is now via member-roles
can_topic).
Smoke-tested on obby.t3ks.com: PREFIX=(qaohv)~&@%+, STATUSMSG=~&@%+,
MODE +q correctly denied for non-owners (482), CREGISTER reports
"must be logged into services account first" for unauthed users.
End-to-end CREGISTER will run once an authed account hits it.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/modules/third/member-roles.c`:
- Around line 165-177: The code currently caches channel_registration_md in
MOD_LOAD using findmoddata_byname(CHANREG_MODDATA_NAME, MODDATATYPE_CHANNEL)
which can run before channeldb is initialized, leaving channel_registration_md
NULL and breaking /CREGISTER; instead, lazily re-resolve the ModData at use time
in cmd_cregister() by calling findmoddata_byname(CHANREG_MODDATA_NAME,
MODDATATYPE_CHANNEL) if channel_registration_md is NULL (or verify and reload it
each time before accessing it), ensuring you handle a still-missing result
gracefully; keep MOD_LOAD unchanged except for removing the one-time cache if
present.
- Around line 927-965: The fallback return value in member_role_can_kick and
member_role_can_set_topic incorrectly uses EX_ALLOW; change both fallback
returns to the module's "no decision" sentinel (i.e., replace the trailing
"return EX_ALLOW;" in member_role_can_kick and the final "return EX_ALLOW;" in
member_role_can_set_topic with the appropriate no-decision constant such as
EX_NEUTRAL/EX_NO_DECISION used by this codebase) so the hooks defer to
default/other-module logic rather than granting permission.
- Around line 631-804: member_roles_configrun currently appends roles on every
rehash causing duplicates and re-registration errors; fix it by invoking
free_all_roles() at the start of member_roles_configrun when type == CONFIG_MAIN
to clear previous state before parsing, and also reset have_default_permissions
= 0 and memset(&default_permissions, 0, sizeof(default_permissions)) so default
state is cleared before rebuilding; refer to member_roles_configrun,
free_all_roles, have_default_permissions, and default_permissions when making
the change.
🪄 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: f3f576c5-34ed-46f9-b16e-97c0c84a9502
📒 Files selected for processing (6)
.gitignoredoc/conf/examples/example.confdoc/conf/examples/member-roles.confdoc/conf/modules.default.confsrc/modules/channeldb.csrc/modules/third/member-roles.c
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- doc/conf/examples/member-roles.conf
| MOD_LOAD() | ||
| { | ||
| /* Locate the channel-registration ModData owned by channeldb. | ||
| * channeldb registers it in its MOD_INIT, which runs before us | ||
| * because channeldb is in core/. If channeldb isn't loaded | ||
| * (rare in obbyircd), CREGISTER will fail gracefully. */ | ||
| channel_registration_md = findmoddata_byname(CHANREG_MODDATA_NAME, MODDATATYPE_CHANNEL); | ||
|
|
||
| /* Note: chanmode registration happens in member_roles_configrun | ||
| * (see "obbyircd: register the cmode now" comment), NOT here. | ||
| * MOD_LOAD fires immediately after MOD_INIT, before config blocks | ||
| * are processed, so member_roles is empty at this point. */ | ||
| return MOD_SUCCESS; |
There was a problem hiding this comment.
Resolve the channeldb ModData lazily instead of caching it once at module load.
This lookup runs before channeldb is loaded in the default config, so channel_registration_md stays NULL and /CREGISTER always falls into the “storage is unavailable” path on a stock install. Re-resolve it in cmd_cregister() when needed, or enforce the dependency/load order explicitly.
Suggested fix
MOD_LOAD()
{
- /* Locate the channel-registration ModData owned by channeldb.
- * channeldb registers it in its MOD_INIT, which runs before us
- * because channeldb is in core/. If channeldb isn't loaded
- * (rare in obbyircd), CREGISTER will fail gracefully. */
- channel_registration_md = findmoddata_byname(CHANREG_MODDATA_NAME, MODDATATYPE_CHANNEL);
+ /* Best-effort lookup; channeldb may not be loaded yet. */
+ channel_registration_md = findmoddata_byname(CHANREG_MODDATA_NAME, MODDATATYPE_CHANNEL);
/* Note: chanmode registration happens in member_roles_configrun
* (see "obbyircd: register the cmode now" comment), NOT here.
@@
account = client->user->account;
- if (!channel_registration_md)
+ if (!channel_registration_md)
+ channel_registration_md = findmoddata_byname(CHANREG_MODDATA_NAME, MODDATATYPE_CHANNEL);
+
+ if (!channel_registration_md)
{
sendnotice(client, "*** CREGISTER: channel-registration storage is unavailable (channeldb module not loaded).");
return;
}🤖 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 `@src/modules/third/member-roles.c` around lines 165 - 177, The code currently
caches channel_registration_md in MOD_LOAD using
findmoddata_byname(CHANREG_MODDATA_NAME, MODDATATYPE_CHANNEL) which can run
before channeldb is initialized, leaving channel_registration_md NULL and
breaking /CREGISTER; instead, lazily re-resolve the ModData at use time in
cmd_cregister() by calling findmoddata_byname(CHANREG_MODDATA_NAME,
MODDATATYPE_CHANNEL) if channel_registration_md is NULL (or verify and reload it
each time before accessing it), ensuring you handle a still-missing result
gracefully; keep MOD_LOAD unchanged except for removing the one-time cache if
present.
| /* Parse role properties */ | ||
| for (cepp = cep->items; cepp; cepp = cepp->next) | ||
| { | ||
| if (!strcmp(cepp->name, "prefix")) | ||
| { | ||
| role->prefix = cepp->value[0]; | ||
| role->sjoin_prefix = cepp->value[0]; | ||
| } | ||
| else if (!strcmp(cepp->name, "rank")) | ||
| { | ||
| role->rank = atoi(cepp->value); | ||
| } | ||
| else if (!strcmp(cepp->name, "mode")) | ||
| { | ||
| role->mode = cepp->value[0]; | ||
| } | ||
| else if (!strcmp(cepp->name, "permissions")) | ||
| { | ||
| for (ceppp = cepp->items; ceppp; ceppp = ceppp->next) | ||
| { | ||
| if (!strcmp(ceppp->name, "can_kick")) | ||
| { | ||
| role->permissions.can_kick = !strcmp(ceppp->value, "yes"); | ||
| role->direct.can_kick = 1; | ||
| } | ||
| else if (!strcmp(ceppp->name, "can_topic")) | ||
| { | ||
| role->permissions.can_topic = !strcmp(ceppp->value, "yes"); | ||
| role->direct.can_topic = 1; | ||
| } | ||
| else if (!strcmp(ceppp->name, "can_invite")) | ||
| { | ||
| role->permissions.can_invite = !strcmp(ceppp->value, "yes"); | ||
| role->direct.can_invite = 1; | ||
| } | ||
| else if (!strcmp(ceppp->name, "can_override_bans")) | ||
| { | ||
| role->permissions.can_override_bans = !strcmp(ceppp->value, "yes"); | ||
| role->direct.can_override_bans = 1; | ||
| } | ||
| else if (!strcmp(ceppp->name, "is_voice")) | ||
| { | ||
| role->permissions.is_voice = !strcmp(ceppp->value, "yes"); | ||
| role->direct.is_voice = 1; | ||
| } | ||
| else if (!strcmp(ceppp->name, "is_unkickable")) | ||
| { | ||
| role->permissions.is_unkickable = !strcmp(ceppp->value, "yes"); | ||
| role->direct.is_unkickable = 1; | ||
| } | ||
| else if (!strcmp(ceppp->name, "can_see_bans")) | ||
| { | ||
| role->permissions.can_see_bans = !strcmp(ceppp->value, "yes"); | ||
| role->direct.can_see_bans = 1; | ||
| } | ||
| else if (!strcmp(ceppp->name, "can_see_invex")) | ||
| { | ||
| role->permissions.can_see_invex = !strcmp(ceppp->value, "yes"); | ||
| role->direct.can_see_invex = 1; | ||
| } | ||
| else if (!strcmp(ceppp->name, "can_see_excepts")) | ||
| { | ||
| role->permissions.can_see_excepts = !strcmp(ceppp->value, "yes"); | ||
| role->direct.can_see_excepts = 1; | ||
| } | ||
| else if (!strcmp(ceppp->name, "can_set")) | ||
| { | ||
| safe_strdup(role->permissions.can_set, ceppp->value); | ||
| role->direct.can_set = 1; | ||
| } | ||
| else if (!strcmp(ceppp->name, "can_unset")) | ||
| { | ||
| safe_strdup(role->permissions.can_unset, ceppp->value); | ||
| role->direct.can_unset = 1; | ||
| } | ||
| else if (!strcmp(ceppp->name, "inherit")) | ||
| safe_strdup(role->inherit, ceppp->value); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /* Add to linked list */ | ||
| AddListItem(role, member_roles); | ||
| /* obbyircd: register the cmode now so PREFIX/STATUSMSG in | ||
| * ISUPPORT and CHANMODES name-table reflect it before the | ||
| * server starts accepting connections. (MOD_LOAD is too | ||
| * early -- runs before configrun.) */ | ||
| register_role_cmode(role); | ||
| } /* end of non-default role block */ | ||
| } | ||
|
|
||
| /* After all roles are loaded, resolve inheritance */ | ||
| struct MemberRole *role; | ||
| for (role = member_roles; role; role = role->next) | ||
| { | ||
| resolve_role_inheritance(role, 0); | ||
| } | ||
|
|
||
| return 1; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
echo "== member-roles lifecycle =="
rg -n -C3 'member_roles_configrun|free_all_roles|register_role_cmode|AddListItem\(role, member_roles\)' src/modules/third/member-roles.c
echo
echo "== CONFIGRUN hook usage/callers =="
rg -n -C3 'HOOKTYPE_CONFIGRUN|CONFIGRUN' src includeRepository: ObsidianIRC/ObbyIRCd
Length of output: 39705
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Checking free_all_roles implementation ==="
sed -n '853,900p' src/modules/third/member-roles.c
echo
echo "=== Checking register_role_cmode implementation ==="
sed -n '182,210p' src/modules/third/member-roles.c
echo
echo "=== Checking for any cleanup/rehash handling in member-roles.c ==="
rg -n 'REHASH|free_all|UNLOAD|clear' src/modules/third/member-roles.c
echo
echo "=== Checking if member_roles list is cleared at start of configrun ==="
sed -n '631,680p' src/modules/third/member-roles.cRepository: ObsidianIRC/ObbyIRCd
Length of output: 4559
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Checking if CONFIGRUN fires on rehash ==="
rg -n -B5 'HOOKTYPE_CONFIGRUN.*confptr' src/conf.c | head -40
echo
echo "=== Checking rehash flow ==="
rg -n 'REHASH' src/conf.c | head -20
echo
echo "=== Checking AddListItem macro ==="
rg -n -A3 'define.*AddListItem' src includeRepository: ObsidianIRC/ObbyIRCd
Length of output: 1972
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Full AddListItem macro definition ==="
sed -n '749,765p' include/struct.h
echo
echo "=== Rehash sequence: when does CONFIGRUN happen? ==="
sed -n '11417,11500p' src/conf.c | head -60
echo
echo "=== Check if config is re-parsed on rehash ==="
rg -n -B2 -A5 'rehash_conf\|load_conf' src/conf.c | head -50Repository: ObsidianIRC/ObbyIRCd
Length of output: 2464
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Tracing config_run path ==="
rg -n 'config_run\(' src/conf.c | head -5
echo
echo "=== Examining config_run function ==="
rg -n -A20 '^config_run\(' src/conf.c | head -40
echo
echo "=== Check if there's any REHASH hook in member-roles ==="
rg -n 'HOOKTYPE_REHASH' src/modules/third/member-roles.c
echo
echo "=== Search for module cleanup on rehash ==="
rg -n 'HOOKTYPE_REHASH' src/modules/*.c | grep -E 'member|role' || echo "No rehash hooks for member-roles"Repository: ObsidianIRC/ObbyIRCd
Length of output: 243
🏁 Script executed:
#!/bin/bash
set -e
echo "=== config_run function where HOOKTYPE_CONFIGRUN is called ==="
sed -n '10363,10380p' src/conf.c
echo
echo "=== Look for HOOKTYPE_CONFIGRUN execution in config_run ==="
sed -n '10363,10600p' src/conf.c | grep -n -A2 -B2 'HOOKTYPE_CONFIGRUN'
echo
echo "=== Verify member_roles is a persistent global ==="
rg -n 'member_roles' src/modules/third/member-roles.c | head -15Repository: ObsidianIRC/ObbyIRCd
Length of output: 578
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Search for where HOOKTYPE_CONFIGRUN hooks are actually invoked ==="
rg -n 'Hooks\[HOOKTYPE_CONFIGRUN\]' src/conf.c
echo
echo "=== Get context around those invocations ==="
rg -n -B3 -A3 'Hooks\[HOOKTYPE_CONFIGRUN\]' src/conf.c | head -60Repository: ObsidianIRC/ObbyIRCd
Length of output: 2508
Clear the member_roles list before rebuilding during configrun to make rehash idempotent.
On every rehash, member_roles_configrun() executes and appends new roles to the linked list without clearing the previous configuration, causing memory leaks of the old state and duplicate role entries. Additionally, register_role_cmode() attempts to re-register mode letters that were already registered in the prior config, triggering config errors. Since free_all_roles() is only called in MOD_UNLOAD(), the module state persists incorrectly across rehashes. Call free_all_roles() at the beginning of member_roles_configrun() when type == CONFIG_MAIN to ensure a clean rebuild.
🤖 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 `@src/modules/third/member-roles.c` around lines 631 - 804,
member_roles_configrun currently appends roles on every rehash causing
duplicates and re-registration errors; fix it by invoking free_all_roles() at
the start of member_roles_configrun when type == CONFIG_MAIN to clear previous
state before parsing, and also reset have_default_permissions = 0 and
memset(&default_permissions, 0, sizeof(default_permissions)) so default state is
cleared before rebuilding; refer to member_roles_configrun, free_all_roles,
have_default_permissions, and default_permissions when making the change.
| int member_role_can_kick(Client *client, Client *victim, Channel *channel, const char *comment, | ||
| const char *client_member_modes, const char *victim_member_modes, const char **reject_reason) | ||
| { | ||
| struct MemberRolePermissions *client_perms, *victim_perms; | ||
| static char errmsg[NICKLEN+256]; | ||
|
|
||
| /* Check if victim has is_unkickable permission */ | ||
| victim_perms = get_effective_permissions(victim, channel); | ||
| if (victim_perms && victim_perms->is_unkickable && !IsULine(client)) | ||
| { | ||
| ircsnprintf(errmsg, sizeof(errmsg), ":%s %d %s %s :%s", | ||
| me.name, ERR_CANNOTDOCOMMAND, client->name, "KICK", | ||
| "user has a role that makes them unkickable"); | ||
| *reject_reason = errmsg; | ||
|
|
||
| sendnotice(victim, | ||
| "*** %s tried to kick you from channel %s (%s)", | ||
| client->name, channel->name, comment); | ||
|
|
||
| return EX_ALWAYS_DENY; | ||
| } | ||
|
|
||
| /* Check if client has can_kick permission */ | ||
| client_perms = get_effective_permissions(client, channel); | ||
| if (client_perms && client_perms->can_kick) | ||
| return EX_ALLOW; | ||
|
|
||
| return EX_ALLOW; /* Let other modules/default logic handle it */ | ||
| } | ||
|
|
||
| int member_role_can_set_topic(Client *client, Channel *channel, const char *topic, const char **errmsg) | ||
| { | ||
| struct MemberRolePermissions *perms; | ||
|
|
||
| perms = get_effective_permissions(client, channel); | ||
| if (perms && perms->can_topic) | ||
| return EX_ALLOW; | ||
|
|
||
| return EX_ALLOW; /* Let other modules/default logic handle it */ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
echo "== Hook contracts and comparable implementations =="
rg -n -C3 'HOOKTYPE_CAN_KICK|HOOKTYPE_CAN_SET_TOPIC|EX_ALLOW|EX_ALWAYS_DENY|HOOK_CONTINUE|HOOK_DENY' src include
echo
echo "== Local implementations =="
rg -n -C3 'member_role_can_kick|member_role_can_set_topic' src/modules/third/member-roles.cRepository: ObsidianIRC/ObbyIRCd
Length of output: 50377
Return a value indicating "no decision" rather than EX_ALLOW when deferring to default logic.
Both member_role_can_kick and member_role_can_set_topic return EX_ALLOW in their fallback paths, but these hooks treat EX_ALLOW as an affirmative authorization. Users who lack the can_kick or can_topic permissions will incorrectly bypass the default permission checks rather than deferred to them. When the module has no specific permission grant, it should not return EX_ALLOW.
🤖 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 `@src/modules/third/member-roles.c` around lines 927 - 965, The fallback return
value in member_role_can_kick and member_role_can_set_topic incorrectly uses
EX_ALLOW; change both fallback returns to the module's "no decision" sentinel
(i.e., replace the trailing "return EX_ALLOW;" in member_role_can_kick and the
final "return EX_ALLOW;" in member_role_can_set_topic with the appropriate
no-decision constant such as EX_NEUTRAL/EX_NO_DECISION used by this codebase) so
the hooks defer to default/other-module logic rather than granting permission.
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
tools/obbyircd-migrate/internal/tui/tui.go (1)
193-206: ⚡ Quick winRun migrations via a
tea.Cmdinstead of insideUpdate.
migrate.Run(...)does disk I/O and can take a while. Calling it synchronously here blocks Bubble Tea’s event loop, so the UI freezes during preview/commit and cannot repaint or react cleanly.Also applies to: 216-227
🤖 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 `@tools/obbyircd-migrate/internal/tui/tui.go` around lines 193 - 206, The code currently calls migrate.Run(...) synchronously inside the Update flow (setting m.err, m.bundle, m.report, m.preview, m.scr), which blocks Bubble Tea; change this to run migrate.Run in a background tea.Cmd: create a result message type (e.g., type migrateResultMsg struct { bundle migrate.BundleType; report migrate.ReportType; err error }) and replace the direct call with return of a tea.Cmd that executes migrate.Run(migrate.PlanInputs{...}) in a goroutine and sends migrateResultMsg to the program; update the Update switch to handle migrateResultMsg by assigning m.err, m.bundle, m.report, m.preview = renderPreview(bundle, report), and m.scr = screenPreview; apply the same pattern for the commit path referenced around lines 216-227 so both preview and commit run asynchronously.
🤖 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 `@tools/obbyircd-migrate/internal/migrate/migrate.go`:
- Around line 57-63: The call to Read can return (nil, nil) and the code
dereferences bundle (bundle.Accounts) causing a panic; after the Read(in.Source,
in.SourcePath) call in migrate.go, add a guard that checks if bundle == nil when
err == nil and return a descriptive error (e.g. fmt.Errorf("read %s: empty
bundle", in.Source)) or otherwise handle the nil bundle before constructing rep
or accessing bundle.Accounts; reference the Read call, the bundle variable and
subsequent uses (bundle.Accounts) and ensure the writer.Report/Options logic
only runs when bundle is non-nil.
- Around line 62-63: Validate the incoming OnConflict value before constructing
writer.Options: check in.OnConflict against the allowed set/enum used by writer
(e.g., the valid constants in writer.OnConflict or accepted string values) and
return a clear error from the orchestration function if it is unknown, rather
than letting writer.Options fallback later; update the code around opts :=
writer.Options{...} (and the surrounding logic in migrate.go) to perform this
validation and return an error early when in.OnConflict is invalid.
In `@tools/obbyircd-migrate/internal/readers/atheme/reader.go`:
- Around line 219-228: The ME branch is using the status field as the memo body
and only checks for 5 fields; update the guard and assignment in the ME handler
so it requires the text field and uses it as the body: change the length check
in the case "ME" handling to ensure the text field exists (e.g. require
len(fields) >= 6) and set ir.Memo.Body to the text field (fields[5]) while
keeping Recipient: fields[1], Sender: fields[2], SentAt: parseUnix(fields[3])
and optionally retain Status from fields[4] if needed; update references to
b.Memos and ir.Memo in this block accordingly.
In `@tools/obbyircd-migrate/internal/readers/ergo/reader.go`:
- Around line 21-25: Add a preflight file existence check before calling
buntdb.Open in Read: use os.Stat(path) and return a wrapped error if the file
does not exist (or Stat fails for non-existence) so we don't silently create an
empty DB; then proceed to call buntdb.Open only after Stat succeeds and add "os"
to the imports. Ensure the error messages reference the path and wrap the
underlying error.
In `@tools/obbyircd-migrate/internal/writer/channeldb_parser.go`:
- Around line 68-81: readOneChannel (and readListMode) currently ignore errors
from r.str()/r.uint64(), letting truncated/malformed channel.db parse as
zero-values; replace the blind assignments (e.g., ch.Name, _ = r.str(), ct, _ :=
r.uint64(), ch.Topic, _ = r.str(), etc., including the version>=101 block for
RegisteredBy/RegisteredAt and the similar fields around lines 105-117) with
error-aware reads: capture the returned error for each call, and if err != nil
return that error up the stack (or wrap it with context), so parsing stops on
malformed input and the caller can handle the failure.
In `@tools/obbyircd-migrate/internal/writer/channeldb.go`:
- Around line 200-214: buildModeLock currently omits ModeLock.Key and
ModeLock.Limit from the serialized string; update buildModeLock to append "+k"
followed by ModeLock.Key when ModeLock.Key != "" and "+l" followed by the
decimal string of ModeLock.Limit when ModeLock.Limit != 0 (use strconv.Itoa for
the limit) in the same strings.Builder flow alongside the existing On/Off
handling so channels with only key/limit locks are preserved; keep the initial
early-return check but ensure the builder branches for Key and Limit are added
(function: buildModeLock, struct fields: ModeLock.Key, ModeLock.Limit,
ModeLock.On, ModeLock.Off).
In `@tools/obbyircd-migrate/internal/writer/end_to_end_test.go`:
- Around line 133-136: The test currently ignores errors from sql.Open and the
QueryRow scan, letting count default to zero; update the end_to_end_test.go
dry-run assertion to check and handle errors from sql.Open("sqlite", dbPath) and
from db.QueryRow(...).Scan(&count) (and ensure db.Close is deferred after
successful open), failing the test with t.Fatalf or t.Fatalf-like assertion if
either returns an error, so the test cannot pass when the DB open or COUNT query
fails.
In `@tools/obbyircd-migrate/internal/writer/obsidian.go`:
- Around line 75-81: The function WriteObsidian currently dereferences the
bundle parameter b when iterating accounts and can panic if b is nil; add an
explicit nil check for b at the start of WriteObsidian (similar to the existing
checks for path and rep) and either return a clear error (e.g., "WriteObsidian:
bundle is required") or initialize b to an empty ir.Bundle before any use,
ensuring the subsequent accounts iteration (where b.Accounts or similar is
accessed) never dereferences a nil pointer.
- Around line 157-162: Replace the silent skip for ConflictMerge with a
fail-fast behavior: when encountering the ConflictMerge case, set ar.Action =
"failed" and ar.Notes = "merge not yet implemented", append ar to rep.Accounts,
and then return or propagate an error (rather than continue) so the caller knows
the operation failed; update the ConflictMerge switch arm handling in
obsidian.go accordingly (referencing ConflictMerge, ar.Action, ar.Notes,
rep.Accounts).
- Around line 279-289: After the rows.Next() loop that populates the out map,
call rows.Err() and return any error it reports before returning out;
specifically, after the loop that uses rows.Next() and rows.Scan(...), check if
err := rows.Err(); err != nil { return nil, err } so iteration/driver errors are
not silently ignored.
---
Nitpick comments:
In `@tools/obbyircd-migrate/internal/tui/tui.go`:
- Around line 193-206: The code currently calls migrate.Run(...) synchronously
inside the Update flow (setting m.err, m.bundle, m.report, m.preview, m.scr),
which blocks Bubble Tea; change this to run migrate.Run in a background tea.Cmd:
create a result message type (e.g., type migrateResultMsg struct { bundle
migrate.BundleType; report migrate.ReportType; err error }) and replace the
direct call with return of a tea.Cmd that executes
migrate.Run(migrate.PlanInputs{...}) in a goroutine and sends migrateResultMsg
to the program; update the Update switch to handle migrateResultMsg by assigning
m.err, m.bundle, m.report, m.preview = renderPreview(bundle, report), and m.scr
= screenPreview; apply the same pattern for the commit path referenced around
lines 216-227 so both preview and commit run asynchronously.
🪄 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: 6260c98b-ad14-4f74-b8cc-88b0e1628d7a
⛔ Files ignored due to path filters (1)
tools/obbyircd-migrate/go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
tools/obbyircd-migrate/cmd/obbyircd-migrate/main.gotools/obbyircd-migrate/go.modtools/obbyircd-migrate/internal/ir/ir.gotools/obbyircd-migrate/internal/migrate/migrate.gotools/obbyircd-migrate/internal/readers/anope/reader.gotools/obbyircd-migrate/internal/readers/anope/reader_test.gotools/obbyircd-migrate/internal/readers/atheme/reader.gotools/obbyircd-migrate/internal/readers/atheme/reader_test.gotools/obbyircd-migrate/internal/readers/ergo/reader.gotools/obbyircd-migrate/internal/readers/ergo/reader_test.gotools/obbyircd-migrate/internal/tui/tui.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/obsidian.go
✅ Files skipped from review due to trivial changes (5)
- tools/obbyircd-migrate/go.mod
- tools/obbyircd-migrate/internal/readers/anope/reader_test.go
- tools/obbyircd-migrate/internal/readers/atheme/reader_test.go
- tools/obbyircd-migrate/internal/readers/ergo/reader_test.go
- tools/obbyircd-migrate/internal/ir/ir.go
| bundle, err := Read(in.Source, in.SourcePath) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("read %s: %w", in.Source, err) | ||
| } | ||
| rep := &writer.Report{} | ||
| opts := writer.Options{DryRun: in.DryRun, OnConflict: in.OnConflict} | ||
| if in.ObsidianPath != "" && len(bundle.Accounts) > 0 { |
There was a problem hiding this comment.
Handle nil bundle from reader before dereferencing.
If Read returns (*ir.Bundle)(nil), nil, Lines 63/68/73 panic. Add an explicit guard for resilience.
Proposed fix
bundle, err := Read(in.Source, in.SourcePath)
if err != nil {
return nil, nil, fmt.Errorf("read %s: %w", in.Source, err)
}
+ if bundle == nil {
+ return nil, nil, fmt.Errorf("read %s: returned nil bundle", in.Source)
+ }
rep := &writer.Report{}Also applies to: 68-73
🤖 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 `@tools/obbyircd-migrate/internal/migrate/migrate.go` around lines 57 - 63, The
call to Read can return (nil, nil) and the code dereferences bundle
(bundle.Accounts) causing a panic; after the Read(in.Source, in.SourcePath) call
in migrate.go, add a guard that checks if bundle == nil when err == nil and
return a descriptive error (e.g. fmt.Errorf("read %s: empty bundle", in.Source))
or otherwise handle the nil bundle before constructing rep or accessing
bundle.Accounts; reference the Read call, the bundle variable and subsequent
uses (bundle.Accounts) and ensure the writer.Report/Options logic only runs when
bundle is non-nil.
| opts := writer.Options{DryRun: in.DryRun, OnConflict: in.OnConflict} | ||
| if in.ObsidianPath != "" && len(bundle.Accounts) > 0 { |
There was a problem hiding this comment.
Validate OnConflict at the orchestration boundary.
Rejecting unknown values early gives callers a clear error instead of writer-specific fallback behavior later.
Proposed fix
rep := &writer.Report{}
+ switch in.OnConflict {
+ case writer.ConflictSkip, writer.ConflictFail, writer.ConflictMerge:
+ default:
+ return bundle, rep, fmt.Errorf("invalid on-conflict policy: %d", in.OnConflict)
+ }
opts := writer.Options{DryRun: in.DryRun, OnConflict: in.OnConflict}🤖 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 `@tools/obbyircd-migrate/internal/migrate/migrate.go` around lines 62 - 63,
Validate the incoming OnConflict value before constructing writer.Options: check
in.OnConflict against the allowed set/enum used by writer (e.g., the valid
constants in writer.OnConflict or accepted string values) and return a clear
error from the orchestration function if it is unknown, rather than letting
writer.Options fallback later; update the code around opts :=
writer.Options{...} (and the surrounding logic in migrate.go) to perform this
validation and return an error early when in.OnConflict is invalid.
| case "ME": | ||
| // ME <dest> <sender> <sent> <status> <text-spans> | ||
| if len(fields) < 5 { | ||
| continue | ||
| } | ||
| b.Memos = append(b.Memos, ir.Memo{ | ||
| Recipient: fields[1], | ||
| Sender: fields[2], | ||
| SentAt: parseUnix(fields[3]), | ||
| Body: fields[4], |
There was a problem hiding this comment.
Use the memo text field, not the status field.
ME rows decode as dest sender sent status text, so fields[4] is the status flag. This currently imports "0"/"1" as the memo body and drops the actual message text. The length guard should also require the text field to be present.
💡 Proposed fix
case "ME":
// ME <dest> <sender> <sent> <status> <text-spans>
- if len(fields) < 5 {
+ if len(fields) < 6 {
continue
}
b.Memos = append(b.Memos, ir.Memo{
Recipient: fields[1],
Sender: fields[2],
SentAt: parseUnix(fields[3]),
- Body: fields[4],
+ Body: fields[5],
})📝 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.
| case "ME": | |
| // ME <dest> <sender> <sent> <status> <text-spans> | |
| if len(fields) < 5 { | |
| continue | |
| } | |
| b.Memos = append(b.Memos, ir.Memo{ | |
| Recipient: fields[1], | |
| Sender: fields[2], | |
| SentAt: parseUnix(fields[3]), | |
| Body: fields[4], | |
| case "ME": | |
| // ME <dest> <sender> <sent> <status> <text-spans> | |
| if len(fields) < 6 { | |
| continue | |
| } | |
| b.Memos = append(b.Memos, ir.Memo{ | |
| Recipient: fields[1], | |
| Sender: fields[2], | |
| SentAt: parseUnix(fields[3]), | |
| Body: fields[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 `@tools/obbyircd-migrate/internal/readers/atheme/reader.go` around lines 219 -
228, The ME branch is using the status field as the memo body and only checks
for 5 fields; update the guard and assignment in the ME handler so it requires
the text field and uses it as the body: change the length check in the case "ME"
handling to ensure the text field exists (e.g. require len(fields) >= 6) and set
ir.Memo.Body to the text field (fields[5]) while keeping Recipient: fields[1],
Sender: fields[2], SentAt: parseUnix(fields[3]) and optionally retain Status
from fields[4] if needed; update references to b.Memos and ir.Memo in this block
accordingly.
| func Read(path string) (*ir.Bundle, error) { | ||
| db, err := buntdb.Open(path) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("open ergo buntdb: %w", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file and the specific lines
cat -n tools/obbyircd-migrate/internal/readers/ergo/reader.go | head -40Repository: ObsidianIRC/ObbyIRCd
Length of output: 1457
🏁 Script executed:
# Check the imports in the file to understand current dependencies
head -20 tools/obbyircd-migrate/internal/readers/ergo/reader.goRepository: ObsidianIRC/ObbyIRCd
Length of output: 579
🏁 Script executed:
# Search for buntdb usage in the codebase to understand the library better
rg "buntdb\.Open|buntdb" --type goRepository: ObsidianIRC/ObbyIRCd
Length of output: 930
🌐 Web query:
buntdb.Open create missing file behavior documentation
💡 Result:
BuntDB’s buntdb.Open creates the database file if it does not already exist (e.g., Open("data.db") creates data.db when missing). [1] What it does NOT do (based on available documentation): there is no documented behavior that it also creates missing parent directories; if you pass a path whose directory hierarchy doesn’t exist, you should expect buntdb.Open to fail with an underlying filesystem error rather than silently creating directories (the official docs only describe creation of a missing file, not directory creation). [1] Grounding example from the docs: “Open the data.db file. It will be created if it doesn’t exist.” [1]
Citations:
🏁 Script executed:
# Also check if there are any tests or usage patterns in the codebase
# that show how buntdb behaves with missing files
cat tools/obbyircd-migrate/internal/readers/ergo/reader_test.go | head -50Repository: ObsidianIRC/ObbyIRCd
Length of output: 1516
Add preflight file existence check before opening the Ergo database.
The buntdb.Open() API creates missing database files. If the path contains a typo, this silently creates an empty database and the migration succeeds with no data loaded instead of failing fast. Add an os.Stat() preflight check to catch missing files before opening, and add os to the imports.
Proposed fix
func Read(path string) (*ir.Bundle, error) {
+ if _, err := os.Stat(path); err != nil {
+ return nil, fmt.Errorf("stat ergo buntdb %s: %w", path, err)
+ }
db, err := buntdb.Open(path)
if err != nil {
return nil, fmt.Errorf("open ergo buntdb: %w", err)
}Also add os to the imports.
🤖 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 `@tools/obbyircd-migrate/internal/readers/ergo/reader.go` around lines 21 - 25,
Add a preflight file existence check before calling buntdb.Open in Read: use
os.Stat(path) and return a wrapped error if the file does not exist (or Stat
fails for non-existence) so we don't silently create an empty DB; then proceed
to call buntdb.Open only after Stat succeeds and add "os" to the imports. Ensure
the error messages reference the path and wrap the underlying error.
| ch.Name, _ = r.str() | ||
| ct, _ := r.uint64() | ||
| ch.CreationTime = int64(ct) | ||
| ch.Topic, _ = r.str() | ||
| ch.TopicNick, _ = r.str() | ||
| tt, _ := r.uint64() | ||
| ch.TopicTime = int64(tt) | ||
| ch.Modes1, _ = r.str() | ||
| ch.Modes2, _ = r.str() | ||
| ch.ModeLock, _ = r.str() | ||
| if version >= 101 { | ||
| ch.RegisteredBy, _ = r.str() | ||
| ra, _ := r.uint64() | ||
| ch.RegisteredAt = int64(ra) |
There was a problem hiding this comment.
Propagate parse errors from every field read.
readOneChannel and readListMode ignore the errors from r.str()/r.uint64(). A truncated or misframed channel.db can therefore parse as zero-values and still let the round-trip test pass.
💡 Proposed fix
- ch.Name, _ = r.str()
- ct, _ := r.uint64()
+ if ch.Name, err = r.str(); err != nil {
+ return ch, err
+ }
+ ct, err := r.uint64()
+ if err != nil {
+ return ch, err
+ }
ch.CreationTime = int64(ct)
- ch.Topic, _ = r.str()
- ch.TopicNick, _ = r.str()
- tt, _ := r.uint64()
+ if ch.Topic, err = r.str(); err != nil {
+ return ch, err
+ }
+ if ch.TopicNick, err = r.str(); err != nil {
+ return ch, err
+ }
+ tt, err := r.uint64()
+ if err != nil {
+ return ch, err
+ }
ch.TopicTime = int64(tt)
- ch.Modes1, _ = r.str()
- ch.Modes2, _ = r.str()
- ch.ModeLock, _ = r.str()
+ if ch.Modes1, err = r.str(); err != nil {
+ return ch, err
+ }
+ if ch.Modes2, err = r.str(); err != nil {
+ return ch, err
+ }
+ if ch.ModeLock, err = r.str(); err != nil {
+ return ch, err
+ }
if version >= 101 {
- ch.RegisteredBy, _ = r.str()
- ra, _ := r.uint64()
+ if ch.RegisteredBy, err = r.str(); err != nil {
+ return ch, err
+ }
+ ra, err := r.uint64()
+ if err != nil {
+ return ch, err
+ }
ch.RegisteredAt = int64(ra)
}
@@
for i := uint32(0); i < n; i++ {
var e ParsedListEntry
- e.BanStr, _ = r.str()
- e.Who, _ = r.str()
- w, _ := r.uint64()
+ if e.BanStr, err = r.str(); err != nil {
+ return nil, err
+ }
+ if e.Who, err = r.str(); err != nil {
+ return nil, err
+ }
+ w, err := r.uint64()
+ if err != nil {
+ return nil, err
+ }
e.When = int64(w)
out = append(out, e)
}Also applies to: 105-117
🤖 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 `@tools/obbyircd-migrate/internal/writer/channeldb_parser.go` around lines 68 -
81, readOneChannel (and readListMode) currently ignore errors from
r.str()/r.uint64(), letting truncated/malformed channel.db parse as zero-values;
replace the blind assignments (e.g., ch.Name, _ = r.str(), ct, _ := r.uint64(),
ch.Topic, _ = r.str(), etc., including the version>=101 block for
RegisteredBy/RegisteredAt and the similar fields around lines 105-117) with
error-aware reads: capture the returned error for each call, and if err != nil
return that error up the stack (or wrap it with context), so parsing stops on
malformed input and the caller can handle the failure.
| func buildModeLock(ch *ir.Channel) string { | ||
| if ch.ModeLock.On == "" && ch.ModeLock.Off == "" && ch.ModeLock.Limit == 0 && ch.ModeLock.Key == "" { | ||
| return "" | ||
| } | ||
| var b strings.Builder | ||
| if ch.ModeLock.On != "" { | ||
| b.WriteByte('+') | ||
| b.WriteString(ch.ModeLock.On) | ||
| } | ||
| if ch.ModeLock.Off != "" { | ||
| b.WriteByte('-') | ||
| b.WriteString(ch.ModeLock.Off) | ||
| } | ||
| return b.String() | ||
| } |
There was a problem hiding this comment.
buildModeLock currently drops +k and +l locks.
This helper checks ModeLock.Key/ModeLock.Limit in the early return, but it never writes them into the serialized lock string. Atheme imports those fields, so channels with only key/limit locks lose their mode-locks on export.
💡 Proposed fix
func buildModeLock(ch *ir.Channel) string {
if ch.ModeLock.On == "" && ch.ModeLock.Off == "" && ch.ModeLock.Limit == 0 && ch.ModeLock.Key == "" {
return ""
}
- var b strings.Builder
- if ch.ModeLock.On != "" {
- b.WriteByte('+')
- b.WriteString(ch.ModeLock.On)
- }
+ var b strings.Builder
+ var params []string
+ if ch.ModeLock.On != "" || ch.ModeLock.Key != "" || ch.ModeLock.Limit > 0 {
+ b.WriteByte('+')
+ b.WriteString(ch.ModeLock.On)
+ if ch.ModeLock.Key != "" {
+ b.WriteByte('k')
+ params = append(params, ch.ModeLock.Key)
+ }
+ if ch.ModeLock.Limit > 0 {
+ b.WriteByte('l')
+ params = append(params, fmt.Sprintf("%d", ch.ModeLock.Limit))
+ }
+ }
if ch.ModeLock.Off != "" {
b.WriteByte('-')
b.WriteString(ch.ModeLock.Off)
}
+ if len(params) > 0 {
+ b.WriteByte(' ')
+ b.WriteString(strings.Join(params, " "))
+ }
return b.String()
}📝 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.
| func buildModeLock(ch *ir.Channel) string { | |
| if ch.ModeLock.On == "" && ch.ModeLock.Off == "" && ch.ModeLock.Limit == 0 && ch.ModeLock.Key == "" { | |
| return "" | |
| } | |
| var b strings.Builder | |
| if ch.ModeLock.On != "" { | |
| b.WriteByte('+') | |
| b.WriteString(ch.ModeLock.On) | |
| } | |
| if ch.ModeLock.Off != "" { | |
| b.WriteByte('-') | |
| b.WriteString(ch.ModeLock.Off) | |
| } | |
| return b.String() | |
| } | |
| func buildModeLock(ch *ir.Channel) string { | |
| if ch.ModeLock.On == "" && ch.ModeLock.Off == "" && ch.ModeLock.Limit == 0 && ch.ModeLock.Key == "" { | |
| return "" | |
| } | |
| var b strings.Builder | |
| var params []string | |
| if ch.ModeLock.On != "" || ch.ModeLock.Key != "" || ch.ModeLock.Limit > 0 { | |
| b.WriteByte('+') | |
| b.WriteString(ch.ModeLock.On) | |
| if ch.ModeLock.Key != "" { | |
| b.WriteByte('k') | |
| params = append(params, ch.ModeLock.Key) | |
| } | |
| if ch.ModeLock.Limit > 0 { | |
| b.WriteByte('l') | |
| params = append(params, fmt.Sprintf("%d", ch.ModeLock.Limit)) | |
| } | |
| } | |
| if ch.ModeLock.Off != "" { | |
| b.WriteByte('-') | |
| b.WriteString(ch.ModeLock.Off) | |
| } | |
| if len(params) > 0 { | |
| b.WriteByte(' ') | |
| b.WriteString(strings.Join(params, " ")) | |
| } | |
| return b.String() | |
| } |
🤖 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 `@tools/obbyircd-migrate/internal/writer/channeldb.go` around lines 200 - 214,
buildModeLock currently omits ModeLock.Key and ModeLock.Limit from the
serialized string; update buildModeLock to append "+k" followed by ModeLock.Key
when ModeLock.Key != "" and "+l" followed by the decimal string of
ModeLock.Limit when ModeLock.Limit != 0 (use strconv.Itoa for the limit) in the
same strings.Builder flow alongside the existing On/Off handling so channels
with only key/limit locks are preserved; keep the initial early-return check but
ensure the builder branches for Key and Limit are added (function:
buildModeLock, struct fields: ModeLock.Key, ModeLock.Limit, ModeLock.On,
ModeLock.Off).
| db, _ := sql.Open("sqlite", dbPath) | ||
| defer db.Close() | ||
| var count int | ||
| _ = db.QueryRow(`SELECT COUNT(*) FROM accounts`).Scan(&count) |
There was a problem hiding this comment.
Don't ignore DB errors in the dry-run assertion.
If sql.Open or the count query fails, count stays zero and this test can pass without ever verifying the database state.
💡 Proposed fix
- db, _ := sql.Open("sqlite", dbPath)
+ db, err := sql.Open("sqlite", dbPath)
+ if err != nil {
+ t.Fatal(err)
+ }
defer db.Close()
var count int
- _ = db.QueryRow(`SELECT COUNT(*) FROM accounts`).Scan(&count)
+ if err := db.QueryRow(`SELECT COUNT(*) FROM accounts`).Scan(&count); err != nil {
+ t.Fatalf("count query: %v", err)
+ }📝 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.
| db, _ := sql.Open("sqlite", dbPath) | |
| defer db.Close() | |
| var count int | |
| _ = db.QueryRow(`SELECT COUNT(*) FROM accounts`).Scan(&count) | |
| db, err := sql.Open("sqlite", dbPath) | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| defer db.Close() | |
| var count int | |
| if err := db.QueryRow(`SELECT COUNT(*) FROM accounts`).Scan(&count); err != nil { | |
| t.Fatalf("count query: %v", 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 `@tools/obbyircd-migrate/internal/writer/end_to_end_test.go` around lines 133 -
136, The test currently ignores errors from sql.Open and the QueryRow scan,
letting count default to zero; update the end_to_end_test.go dry-run assertion
to check and handle errors from sql.Open("sqlite", dbPath) and from
db.QueryRow(...).Scan(&count) (and ensure db.Close is deferred after successful
open), failing the test with t.Fatalf or t.Fatalf-like assertion if either
returns an error, so the test cannot pass when the DB open or COUNT query fails.
| func WriteObsidian(path string, b *ir.Bundle, opts Options, rep *Report) error { | ||
| if path == "" { | ||
| return errors.New("WriteObsidian: path is required") | ||
| } | ||
| if rep == nil { | ||
| rep = &Report{} | ||
| } |
There was a problem hiding this comment.
Guard against nil bundle before iterating accounts.
Line 135 dereferences b without validation; a nil *ir.Bundle will panic the migration.
Proposed fix
func WriteObsidian(path string, b *ir.Bundle, opts Options, rep *Report) error {
if path == "" {
return errors.New("WriteObsidian: path is required")
}
+ if b == nil {
+ return errors.New("WriteObsidian: bundle is required")
+ }
if rep == nil {
rep = &Report{}
}Also applies to: 135-135
🤖 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 `@tools/obbyircd-migrate/internal/writer/obsidian.go` around lines 75 - 81, The
function WriteObsidian currently dereferences the bundle parameter b when
iterating accounts and can panic if b is nil; add an explicit nil check for b at
the start of WriteObsidian (similar to the existing checks for path and rep) and
either return a clear error (e.g., "WriteObsidian: bundle is required") or
initialize b to an empty ir.Bundle before any use, ensuring the subsequent
accounts iteration (where b.Accounts or similar is accessed) never dereferences
a nil pointer.
| case ConflictMerge: | ||
| // Fall through to update path -- not yet implemented for v1. | ||
| ar.Action = "skipped" | ||
| ar.Notes = "merge not yet implemented" | ||
| rep.Accounts = append(rep.Accounts, ar) | ||
| continue |
There was a problem hiding this comment.
ConflictMerge should fail fast until implemented.
Lines 157-162 currently convert merge into a silent skip. That can look like success while dropping intended updates.
Proposed fix
if existingID != 0 {
switch opts.OnConflict {
case ConflictFail:
return fmt.Errorf("account %q already exists in obsidian.db (--on-conflict=fail)", a.Name)
case ConflictSkip:
ar.Action = "skipped"
ar.Notes = "exists"
rep.Accounts = append(rep.Accounts, ar)
continue
case ConflictMerge:
- // Fall through to update path -- not yet implemented for v1.
- ar.Action = "skipped"
- ar.Notes = "merge not yet implemented"
- rep.Accounts = append(rep.Accounts, ar)
- continue
+ return fmt.Errorf("account %q already exists in obsidian.db (--on-conflict=merge not implemented)", a.Name)
}
}📝 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.
| case ConflictMerge: | |
| // Fall through to update path -- not yet implemented for v1. | |
| ar.Action = "skipped" | |
| ar.Notes = "merge not yet implemented" | |
| rep.Accounts = append(rep.Accounts, ar) | |
| continue | |
| case ConflictMerge: | |
| return fmt.Errorf("account %q already exists in obsidian.db (--on-conflict=merge not implemented)", a.Name) |
🤖 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 `@tools/obbyircd-migrate/internal/writer/obsidian.go` around lines 157 - 162,
Replace the silent skip for ConflictMerge with a fail-fast behavior: when
encountering the ConflictMerge case, set ar.Action = "failed" and ar.Notes =
"merge not yet implemented", append ar to rep.Accounts, and then return or
propagate an error (rather than continue) so the caller knows the operation
failed; update the ConflictMerge switch arm handling in obsidian.go accordingly
(referencing ConflictMerge, ar.Action, ar.Notes, rep.Accounts).
| for rows.Next() { | ||
| var cid int | ||
| var name, typ string | ||
| var notnull, pk int | ||
| var dflt sql.NullString | ||
| if err := rows.Scan(&cid, &name, &typ, ¬null, &dflt, &pk); err != nil { | ||
| return nil, err | ||
| } | ||
| out[strings.ToLower(name)] = true | ||
| } | ||
| return out, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the specific file
cat -n tools/obbyircd-migrate/internal/writer/obsidian.go | sed -n '270,295p'Repository: ObsidianIRC/ObbyIRCd
Length of output: 1010
🏁 Script executed:
# Also check the broader function context to understand the function signature
cat -n tools/obbyircd-migrate/internal/writer/obsidian.go | sed -n '260,300p'Repository: ObsidianIRC/ObbyIRCd
Length of output: 1364
🏁 Script executed:
# Search for other rows.Next() patterns in the codebase to see if rows.Err() is consistently checked
rg 'rows\.Next\(\)' -A 10 --type goRepository: ObsidianIRC/ObbyIRCd
Length of output: 885
🏁 Script executed:
# Check if rows.Err() is used elsewhere in this file
rg 'rows\.Err\(\)' tools/obbyircd-migrate/internal/writer/obsidian.goRepository: ObsidianIRC/ObbyIRCd
Length of output: 46
🏁 Script executed:
# Search for all rows.Next() patterns in the entire codebase
rg 'rows\.Next\(\)' -A 15 --type go | head -100Repository: ObsidianIRC/ObbyIRCd
Length of output: 1234
🏁 Script executed:
# Look for rows.Err() usage patterns across the repo
rg 'rows\.Err\(\)' --type go -B 3 -A 1Repository: ObsidianIRC/ObbyIRCd
Length of output: 46
Check rows.Err() after the loop completes to catch iteration errors.
The function returns without calling rows.Err() after the rows.Next() loop. According to Go's database/sql package, iteration errors from the driver can be surfaced through rows.Err() and must be checked to avoid silently losing failures.
Proposed fix
for rows.Next() {
var cid int
var name, typ string
var notnull, pk int
var dflt sql.NullString
if err := rows.Scan(&cid, &name, &typ, ¬null, &dflt, &pk); err != nil {
return nil, err
}
out[strings.ToLower(name)] = true
}
+ if err := rows.Err(); err != nil {
+ return nil, err
+ }
return out, nil
}📝 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.
| for rows.Next() { | |
| var cid int | |
| var name, typ string | |
| var notnull, pk int | |
| var dflt sql.NullString | |
| if err := rows.Scan(&cid, &name, &typ, ¬null, &dflt, &pk); err != nil { | |
| return nil, err | |
| } | |
| out[strings.ToLower(name)] = true | |
| } | |
| return out, nil | |
| for rows.Next() { | |
| var cid int | |
| var name, typ string | |
| var notnull, pk int | |
| var dflt sql.NullString | |
| if err := rows.Scan(&cid, &name, &typ, ¬null, &dflt, &pk); err != nil { | |
| return nil, err | |
| } | |
| out[strings.ToLower(name)] = true | |
| } | |
| if err := rows.Err(); err != nil { | |
| return nil, err | |
| } | |
| return out, nil |
🤖 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 `@tools/obbyircd-migrate/internal/writer/obsidian.go` around lines 279 - 289,
After the rows.Next() loop that populates the out map, call rows.Err() and
return any error it reports before returning out; specifically, after the loop
that uses rows.Next() and rows.Scan(...), check if err := rows.Err(); err != nil
{ return nil, err } so iteration/driver errors are not silently ignored.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/modules/account-registration.c (1)
138-146:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove or lock down
DEBUGLMAObefore release.This is registered as
CMD_USER, so any authenticated user can enumerateclient_listand see internal client IDs. That's production debug surface and an avoidable information leak.Also applies to: 276-276
🤖 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 `@src/modules/account-registration.c` around lines 138 - 146, The cmd_debuglmao handler (CMD_FUNC(cmd_debuglmao)) leaks client_list and internal client IDs via sendnotice and must be removed or restricted: either unregister the CMD_USER binding and delete the CMD_FUNC entirely, or restrict the command to privileged users by changing its registration from CMD_USER to an admin-only command and adding an authorization check (e.g., is_admin or has_capability) before iterating client_list; alternatively guard the function with a compile-time debug flag so it is not present in release builds — ensure no public path calls sendnotice with c->id in production.
♻️ Duplicate comments (1)
tools/obbyircd-migrate/internal/writer/obsidian.go (1)
158-163:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast for
ConflictMergeuntil merge semantics exist.
--on-conflict=mergecurrently degrades into a silent skip here. That makes the migration look successful while dropping the caller's requested behavior.🤖 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 `@tools/obbyircd-migrate/internal/writer/obsidian.go` around lines 158 - 163, The ConflictMerge branch currently silently skips requested merges; instead fail fast: in the case for ConflictMerge set ar.Action to "failed" and ar.Notes to something like "on-conflict=merge not implemented", append ar to rep.Accounts, and then return an error (or propagate a failure) from the enclosing handler so the caller sees the migration did not succeed; update the ConflictMerge case in obsidian.go (the switch handling ar/rep) to perform these steps rather than falling through to a skip.
🤖 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 `@src/modules/account-registration.c`:
- Around line 649-666: authenticate_attempt() currently only checks
account_2fa_credentials WHERE type='external', ignoring the new account_certfps
table where migrated cert fingerprints are stored; update authenticate_attempt()
to also consult account_certfps (fingerprint column) for SASL EXTERNAL
logins—either by adding a LEFT JOIN to account_certfps and including its
fingerprint in the matching predicate or by UNIONing credential fingerprints
with account_certfps so both sources are considered when type='external',
ensuring migrated certfps can authenticate.
- Around line 1202-1219: b64_decode_any currently passes the raw input to
EVP_DecodeBlock but must first normalize base64url and fix missing padding;
update b64_decode_any to copy `in` into a local buffer, replace '-' with '+' and
'_' with '/', append '=' characters until the length is a multiple of 4, then
call EVP_DecodeBlock on the normalized buffer, validate against outcap, and
adjust returned length for any '=' padding stripped by EVP; reference the
function name b64_decode_any and ensure pbkdf2v2_verify callers keep using it
unchanged.
In `@src/modules/memo.c`:
- Around line 299-305: Replace the loose strtol usage that allows trailing
garbage by calling strtol with an end-pointer (e.g. char *endptr) and require
that endptr points to the terminating NUL (and that parv[2] is non-empty) before
accepting the value; if the end pointer does not indicate full-string
consumption or the converted value is <= 0, return the same error via
sendto_one. Update the memo_id assignment (currently memo_id = strtol(parv[2],
NULL, 10)), validate using endptr and then keep the existing sendto_one
INVALID_PARAMS branch for failure.
In `@tools/obbyircd-migrate/internal/migrate/migrate.go`:
- Around line 64-68: The current condition only calls writer.WriteObsidian when
in.ObsidianPath != "" AND len(bundle.Accounts) > 0, which skips persisting memos
when there are no accounts; remove the accounts length check so that
writer.WriteObsidian(in.ObsidianPath, bundle, opts, rep) is invoked whenever
in.ObsidianPath != "" (i.e., always call WriteObsidian when --obsidian is
provided), leaving the parameters (bundle, opts, rep) and error handling
unchanged.
In `@tools/obbyircd-migrate/internal/writer/end_to_end_test.go`:
- Around line 84-93: The queries in TestWriteObsidianMemos currently ignore
errors from db.QueryRow(...).Scan(...) which can mask failures; update the two
Scan calls (the COUNT(*) scan that sets rows and the SELECT sender, body scan
that sets sender/body) to capture and check the returned error and call t.Fatalf
(or t.Fatalf) with the error if non-nil so the test fails loudly instead of
proceeding with zero values. Ensure you reference the same
db.QueryRow(...).Scan(...) invocations when adding the error handling.
In `@tools/obbyircd-migrate/internal/writer/tkldb.go`:
- Around line 33-39: WriteTKLDB currently validates path but may dereference a
nil *ir.Bundle (b) in the ban loop and later code (see ban loop around lines
56-57); add a guard at the start of WriteTKLDB to check if b == nil and return a
clear error (e.g. "WriteTKLDB: bundle is required") so the function fails
gracefully instead of panicking, and ensure any subsequent places that
dereference b (the ban loop and other uses) can assume b is non-nil.
In `@tools/obbyircd-migrate/README.md`:
- Around line 77-82: The fenced code blocks containing the ASCII diagram (the
block starting with the box diagram "┌────────────┐ ┌──────────────┐ ...")
and the later block referenced in the comment must include a language tag to
satisfy markdownlint MD040; update those triple-backtick fences to use a neutral
tag like ```text or ```ascii (i.e., change the opening ``` to ```text) so the
README.md fenced blocks are annotated without altering their contents.
- Around line 114-123: Update the "Known limitations" section to reflect the
code changes: remove or reword the bullet that claims bcrypt / pbkdf2v2 /
crypt-sha256 / crypt-sha512 verification is unavailable (these are now handled
by the verifier dispatch implemented in account-registration.c), and remove the
claim that memos are not written (the Obsidian writer now inserts memos into the
memos table); keep or adjust the X-line/gecos-ban note if still accurate and
update any PLAN.md cross-references to the correct section or remove if no
longer relevant.
---
Outside diff comments:
In `@src/modules/account-registration.c`:
- Around line 138-146: The cmd_debuglmao handler (CMD_FUNC(cmd_debuglmao)) leaks
client_list and internal client IDs via sendnotice and must be removed or
restricted: either unregister the CMD_USER binding and delete the CMD_FUNC
entirely, or restrict the command to privileged users by changing its
registration from CMD_USER to an admin-only command and adding an authorization
check (e.g., is_admin or has_capability) before iterating client_list;
alternatively guard the function with a compile-time debug flag so it is not
present in release builds — ensure no public path calls sendnotice with c->id in
production.
---
Duplicate comments:
In `@tools/obbyircd-migrate/internal/writer/obsidian.go`:
- Around line 158-163: The ConflictMerge branch currently silently skips
requested merges; instead fail fast: in the case for ConflictMerge set ar.Action
to "failed" and ar.Notes to something like "on-conflict=merge not implemented",
append ar to rep.Accounts, and then return an error (or propagate a failure)
from the enclosing handler so the caller sees the migration did not succeed;
update the ConflictMerge case in obsidian.go (the switch handling ar/rep) to
perform these steps rather than falling through to a skip.
🪄 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: c3dc2ee0-757c-45eb-9db5-a007aeec1116
📒 Files selected for processing (16)
doc/conf/examples/member-roles.confdoc/conf/modules.default.confinclude/obsidian.hsrc/modules/Makefile.insrc/modules/account-registration.csrc/modules/member-roles.csrc/modules/memo.ctools/obbyircd-migrate/README.mdtools/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/end_to_end_test.gotools/obbyircd-migrate/internal/writer/obsidian.gotools/obbyircd-migrate/internal/writer/tkldb.gotools/obbyircd-migrate/internal/writer/tkldb_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- tools/obbyircd-migrate/internal/readers/ergo/reader_test.go
- tools/obbyircd-migrate/cmd/obbyircd-migrate/main.go
- doc/conf/modules.default.conf
- doc/conf/examples/member-roles.conf
- tools/obbyircd-migrate/internal/readers/ergo/reader.go
| /* Phase 0 (§6.1) sibling tables for SASL EXTERNAL + nick aliases. | ||
| * Created up-front; the migration tool will populate rows here | ||
| * when the source carries certfps / additional registered nicks. */ | ||
| "CREATE TABLE IF NOT EXISTS account_certfps (" | ||
| " id INTEGER PRIMARY KEY AUTOINCREMENT," | ||
| " account_id INTEGER NOT NULL," | ||
| " fingerprint TEXT NOT NULL," | ||
| " added_at INTEGER NOT NULL," | ||
| " UNIQUE(account_id, fingerprint)," | ||
| " FOREIGN KEY (account_id) REFERENCES accounts(id)" | ||
| ");" | ||
| "CREATE TABLE IF NOT EXISTS account_aliases (" | ||
| " id INTEGER PRIMARY KEY AUTOINCREMENT," | ||
| " account_id INTEGER NOT NULL," | ||
| " alias TEXT NOT NULL COLLATE NOCASE," | ||
| " UNIQUE(alias)," | ||
| " FOREIGN KEY (account_id) REFERENCES accounts(id)" | ||
| ");" |
There was a problem hiding this comment.
account_certfps is orphaned by the current auth path.
These new rows are never consulted during SASL EXTERNAL login: the lookup in authenticate_attempt() still joins account_2fa_credentials on type = 'external'. The migration tool now writes certfps into account_certfps, so migrated certificate identities will not authenticate unless this module reads that table too.
🤖 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 `@src/modules/account-registration.c` around lines 649 - 666,
authenticate_attempt() currently only checks account_2fa_credentials WHERE
type='external', ignoring the new account_certfps table where migrated cert
fingerprints are stored; update authenticate_attempt() to also consult
account_certfps (fingerprint column) for SASL EXTERNAL logins—either by adding a
LEFT JOIN to account_certfps and including its fingerprint in the matching
predicate or by UNIONing credential fingerprints with account_certfps so both
sources are considered when type='external', ensuring migrated certfps can
authenticate.
| /* base64 decoding shim using OpenSSL EVP. Returns number of decoded | ||
| * bytes, or -1 on failure. `out` must be at least len(in) bytes. | ||
| * Atheme's pbkdf2v2 module emits "url-safe" b64 with no padding for | ||
| * the salt, but standard b64 with padding for the hash. We try both. */ | ||
| static int b64_decode_any(const char *in, unsigned char *out, int outcap) | ||
| { | ||
| int n = EVP_DecodeBlock(out, (const unsigned char *)in, strlen(in)); | ||
| if (n <= 0 || n > outcap) | ||
| return -1; | ||
| /* EVP_DecodeBlock returns the number of bytes decoded BEFORE | ||
| * stripping pad nuls; figure out actual length by walking back | ||
| * over '=' padding in the input. */ | ||
| int pad = 0; | ||
| int inlen = strlen(in); | ||
| if (inlen >= 1 && in[inlen-1] == '=') pad++; | ||
| if (inlen >= 2 && in[inlen-2] == '=') pad++; | ||
| return n - pad; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the actual function and check the context
fd account-registration.cRepository: ObsidianIRC/ObbyIRCd
Length of output: 99
🏁 Script executed:
# Look at the function and its context
cat -n src/modules/account-registration.c | sed -n '1195,1230p'Repository: ObsidianIRC/ObbyIRCd
Length of output: 1644
🏁 Script executed:
# Search for usage of b64_decode_any to understand how it's used
rg 'b64_decode_any' src/Repository: ObsidianIRC/ObbyIRCd
Length of output: 379
🏁 Script executed:
# Search for pbkdf2v2 references to understand the context better
rg 'pbkdf2v2' src/Repository: ObsidianIRC/ObbyIRCd
Length of output: 628
🏁 Script executed:
# Check OpenSSL EVP_DecodeBlock documentation behavior
# Also search for any test files or documentation about base64url handling
fd -e c -e h | xargs rg 'EVP_DecodeBlock' -lRepository: ObsidianIRC/ObbyIRCd
Length of output: 99
🏁 Script executed:
# Get the complete pbkdf2v2_verify function
cat -n src/modules/account-registration.c | sed -n '1221,1280p'Repository: ObsidianIRC/ObbyIRCd
Length of output: 2399
🏁 Script executed:
# Continue reading the function
cat -n src/modules/account-registration.c | sed -n '1280,1320p'Repository: ObsidianIRC/ObbyIRCd
Length of output: 2208
🌐 Web query:
OpenSSL EVP_DecodeBlock base64url support and behavior with non-standard base64 characters
💡 Result:
OpenSSL's EVP_DecodeBlock does not natively support base64url encoding. It uses the standard base64 alphabet: A-Z, a-z, 0-9, +, /, with = for padding [1][2][3][4]. Characters outside this alphabet, such as - (hyphen) or _ (underscore) used in base64url, are treated as invalid and cause the function to return -1 (error) [1][5]. The hyphen (-) is recognized only as a soft end-of-input in EVP_DecodeUpdate (for PEM compatibility), not as a valid base64 character in EVP_DecodeBlock [1][6]. There is an optional SRP_ALPHABET flag for EVP_ENCODE_CTX that changes the table, but it is not enabled by default for EVP_DecodeBlock (which uses NULL ctx) and uses a different custom alphabet, not base64url [5]. For non-standard characters like - or _, EVP_DecodeBlock will fail decoding [5]. Note that EVP_DecodeBlock always produces output as a multiple of 3 bytes, treating padding as zero bits, so callers must trim trailing bytes based on padding count [7][2][8]. For proper handling including padding length, use EVP_DecodeInit/EVP_DecodeUpdate/EVP_DecodeFinal instead [7][8]. No built-in base64url support exists in EVP_DecodeBlock as of OpenSSL 3.x (master branch) [2][5]. To decode base64url, applications must translate - to +, _ to /, and add padding before calling EVP_DecodeBlock.
Citations:
- 1: https://manpages.debian.org/experimental/libssl-doc/EVP_DecodeBlock.3ssl.en.html
- 2: https://openssl.org/docs/manmaster/man3/EVP_EncodeBlock.html
- 3: https://docs.openssl.org/3.4/man3/EVP_EncodeInit/
- 4: https://wiki.openssl.org/index.php/Base64
- 5: https://github.com/openssl/openssl/blob/master/crypto/evp/encode.c
- 6: https://manpages.opensuse.org/Leap-16.0/openssl-3-doc/EVP_DecodeFinal.33ssl.en.html
- 7: base64 decoding using EVP_DecodeBlock returns wrong result: one byte (0x10) too much openssl/openssl#23863
- 8: EVP_DecodeBlock return value is incorrect for standard Base64 openssl/openssl#17197
b64_decode_any() does not actually handle base64url encoding or missing padding.
The function comment claims it accepts Atheme's url-safe, unpadded PBKDF2v2 fields and says "we try both," but the implementation only calls EVP_DecodeBlock() directly. According to OpenSSL documentation, EVP_DecodeBlock() uses only the standard base64 alphabet (A-Z, a-z, 0-9, +, /) and will fail with any base64url character (such as - or _). Additionally, the current logic only strips trailing = from the output count rather than adding missing padding before decoding. This means any salt or hash containing base64url characters or omitted padding will fail to decode (returning -1), causing pbkdf2v2_verify() to fail and preventing migrated Atheme accounts from verifying their passwords.
Suggested fix
static int b64_decode_any(const char *in, unsigned char *out, int outcap)
{
- int n = EVP_DecodeBlock(out, (const unsigned char *)in, strlen(in));
+ char buf[256];
+ int inlen = strlen(in);
+ int pad = (4 - (inlen % 4)) % 4;
+ if (inlen + pad >= (int)sizeof(buf))
+ return -1;
+ for (int i = 0; i < inlen; i++)
+ buf[i] = (in[i] == '-') ? '+' : (in[i] == '_') ? '/' : in[i];
+ for (int i = 0; i < pad; i++)
+ buf[inlen + i] = '=';
+ buf[inlen + pad] = '\0';
+
+ int n = EVP_DecodeBlock(out, (const unsigned char *)buf, inlen + pad);
if (n <= 0 || n > outcap)
return -1;
- /* EVP_DecodeBlock returns the number of bytes decoded BEFORE
- * stripping pad nuls; figure out actual length by walking back
- * over '=' padding in the input. */
- int pad = 0;
- int inlen = strlen(in);
- if (inlen >= 1 && in[inlen-1] == '=') pad++;
- if (inlen >= 2 && in[inlen-2] == '=') pad++;
return n - pad;
}📝 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.
| /* base64 decoding shim using OpenSSL EVP. Returns number of decoded | |
| * bytes, or -1 on failure. `out` must be at least len(in) bytes. | |
| * Atheme's pbkdf2v2 module emits "url-safe" b64 with no padding for | |
| * the salt, but standard b64 with padding for the hash. We try both. */ | |
| static int b64_decode_any(const char *in, unsigned char *out, int outcap) | |
| { | |
| int n = EVP_DecodeBlock(out, (const unsigned char *)in, strlen(in)); | |
| if (n <= 0 || n > outcap) | |
| return -1; | |
| /* EVP_DecodeBlock returns the number of bytes decoded BEFORE | |
| * stripping pad nuls; figure out actual length by walking back | |
| * over '=' padding in the input. */ | |
| int pad = 0; | |
| int inlen = strlen(in); | |
| if (inlen >= 1 && in[inlen-1] == '=') pad++; | |
| if (inlen >= 2 && in[inlen-2] == '=') pad++; | |
| return n - pad; | |
| } | |
| static int b64_decode_any(const char *in, unsigned char *out, int outcap) | |
| { | |
| char buf[256]; | |
| int inlen = strlen(in); | |
| int pad = (4 - (inlen % 4)) % 4; | |
| if (inlen + pad >= (int)sizeof(buf)) | |
| return -1; | |
| for (int i = 0; i < inlen; i++) | |
| buf[i] = (in[i] == '-') ? '+' : (in[i] == '_') ? '/' : in[i]; | |
| for (int i = 0; i < pad; i++) | |
| buf[inlen + i] = '='; | |
| buf[inlen + pad] = '\0'; | |
| int n = EVP_DecodeBlock(out, (const unsigned char *)buf, inlen + pad); | |
| if (n <= 0 || n > outcap) | |
| return -1; | |
| return n - pad; | |
| } |
🤖 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 `@src/modules/account-registration.c` around lines 1202 - 1219, b64_decode_any
currently passes the raw input to EVP_DecodeBlock but must first normalize
base64url and fix missing padding; update b64_decode_any to copy `in` into a
local buffer, replace '-' with '+' and '_' with '/', append '=' characters until
the length is a multiple of 4, then call EVP_DecodeBlock on the normalized
buffer, validate against outcap, and adjust returned length for any '=' padding
stripped by EVP; reference the function name b64_decode_any and ensure
pbkdf2v2_verify callers keep using it unchanged.
| memo_id = strtol(parv[2], NULL, 10); | ||
| if (memo_id <= 0) | ||
| { | ||
| sendto_one(client, NULL, | ||
| ":%s FAIL MEMO INVALID_PARAMS :Memo id must be a positive integer.", | ||
| me.name); | ||
| return; |
There was a problem hiding this comment.
Reject memo IDs with trailing garbage.
strtol(parv[2], NULL, 10) will accept inputs like 12foo as memo 12, so the command can act on a different memo than the user actually typed. Use an end pointer and require full-string consumption.
Suggested fix
- memo_id = strtol(parv[2], NULL, 10);
- if (memo_id <= 0)
+ char *end = NULL;
+ memo_id = strtol(parv[2], &end, 10);
+ if (memo_id <= 0 || !end || *end != '\0')
{
sendto_one(client, NULL,
":%s FAIL MEMO INVALID_PARAMS :Memo id must be a positive integer.",
me.name);
return;🤖 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 `@src/modules/memo.c` around lines 299 - 305, Replace the loose strtol usage
that allows trailing garbage by calling strtol with an end-pointer (e.g. char
*endptr) and require that endptr points to the terminating NUL (and that parv[2]
is non-empty) before accepting the value; if the end pointer does not indicate
full-string consumption or the converted value is <= 0, return the same error
via sendto_one. Update the memo_id assignment (currently memo_id =
strtol(parv[2], NULL, 10)), validate using endptr and then keep the existing
sendto_one INVALID_PARAMS branch for failure.
| if in.ObsidianPath != "" && len(bundle.Accounts) > 0 { | ||
| if err := writer.WriteObsidian(in.ObsidianPath, bundle, opts, rep); err != nil { | ||
| return bundle, rep, fmt.Errorf("write obsidian: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't gate Obsidian writes on accounts only.
WriteObsidian() also persists memos, but this condition only invokes it when bundle.Accounts is non-empty. A source with memos and no accounts will silently skip memo migration even when --obsidian was provided.
Suggested fix
- if in.ObsidianPath != "" && len(bundle.Accounts) > 0 {
+ if in.ObsidianPath != "" && (len(bundle.Accounts) > 0 || len(bundle.Memos) > 0) {
if err := writer.WriteObsidian(in.ObsidianPath, bundle, opts, rep); err != nil {
return bundle, rep, fmt.Errorf("write obsidian: %w", err)
}
}📝 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.
| if in.ObsidianPath != "" && len(bundle.Accounts) > 0 { | |
| if err := writer.WriteObsidian(in.ObsidianPath, bundle, opts, rep); err != nil { | |
| return bundle, rep, fmt.Errorf("write obsidian: %w", err) | |
| } | |
| } | |
| if in.ObsidianPath != "" && (len(bundle.Accounts) > 0 || len(bundle.Memos) > 0) { | |
| if err := writer.WriteObsidian(in.ObsidianPath, bundle, opts, rep); err != nil { | |
| return bundle, rep, fmt.Errorf("write obsidian: %w", 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 `@tools/obbyircd-migrate/internal/migrate/migrate.go` around lines 64 - 68, The
current condition only calls writer.WriteObsidian when in.ObsidianPath != "" AND
len(bundle.Accounts) > 0, which skips persisting memos when there are no
accounts; remove the accounts length check so that
writer.WriteObsidian(in.ObsidianPath, bundle, opts, rep) is invoked whenever
in.ObsidianPath != "" (i.e., always call WriteObsidian when --obsidian is
provided), leaving the parameters (bundle, opts, rep) and error handling
unchanged.
| var rows int | ||
| _ = db.QueryRow(`SELECT COUNT(*) FROM memos`).Scan(&rows) | ||
| if rows != 1 { | ||
| t.Errorf("memos written = %d, want 1", rows) | ||
| } | ||
| var sender, body string | ||
| _ = db.QueryRow(`SELECT sender, body FROM memos`).Scan(&sender, &body) | ||
| if sender != "Bob" || body != "hi alice" { | ||
| t.Errorf("memo row = (%s, %q)", sender, body) | ||
| } |
There was a problem hiding this comment.
Don't ignore DB errors in TestWriteObsidianMemos queries.
Same anti-pattern as previously flagged for the dry-run test: if Scan fails, rows/sender/body keep their zero values and the assertions can pass without ever observing the database. A failing test should fail loudly, not silently succeed because the query errored.
💡 Proposed fix
var rows int
- _ = db.QueryRow(`SELECT COUNT(*) FROM memos`).Scan(&rows)
+ if err := db.QueryRow(`SELECT COUNT(*) FROM memos`).Scan(&rows); err != nil {
+ t.Fatalf("count memos: %v", err)
+ }
if rows != 1 {
t.Errorf("memos written = %d, want 1", rows)
}
var sender, body string
- _ = db.QueryRow(`SELECT sender, body FROM memos`).Scan(&sender, &body)
+ if err := db.QueryRow(`SELECT sender, body FROM memos`).Scan(&sender, &body); err != nil {
+ t.Fatalf("scan memo: %v", err)
+ }
if sender != "Bob" || body != "hi alice" {
t.Errorf("memo row = (%s, %q)", sender, body)
}📝 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.
| var rows int | |
| _ = db.QueryRow(`SELECT COUNT(*) FROM memos`).Scan(&rows) | |
| if rows != 1 { | |
| t.Errorf("memos written = %d, want 1", rows) | |
| } | |
| var sender, body string | |
| _ = db.QueryRow(`SELECT sender, body FROM memos`).Scan(&sender, &body) | |
| if sender != "Bob" || body != "hi alice" { | |
| t.Errorf("memo row = (%s, %q)", sender, body) | |
| } | |
| var rows int | |
| if err := db.QueryRow(`SELECT COUNT(*) FROM memos`).Scan(&rows); err != nil { | |
| t.Fatalf("count memos: %v", err) | |
| } | |
| if rows != 1 { | |
| t.Errorf("memos written = %d, want 1", rows) | |
| } | |
| var sender, body string | |
| if err := db.QueryRow(`SELECT sender, body FROM memos`).Scan(&sender, &body); err != nil { | |
| t.Fatalf("scan memo: %v", err) | |
| } | |
| if sender != "Bob" || body != "hi alice" { | |
| t.Errorf("memo row = (%s, %q)", sender, body) | |
| } |
🤖 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 `@tools/obbyircd-migrate/internal/writer/end_to_end_test.go` around lines 84 -
93, The queries in TestWriteObsidianMemos currently ignore errors from
db.QueryRow(...).Scan(...) which can mask failures; update the two Scan calls
(the COUNT(*) scan that sets rows and the SELECT sender, body scan that sets
sender/body) to capture and check the returned error and call t.Fatalf (or
t.Fatalf) with the error if non-nil so the test fails loudly instead of
proceeding with zero values. Ensure you reference the same
db.QueryRow(...).Scan(...) invocations when adding the error handling.
| func WriteTKLDB(path string, b *ir.Bundle, opts Options, rep *Report) error { | ||
| if path == "" { | ||
| return errors.New("WriteTKLDB: path is required") | ||
| } | ||
| if rep == nil { | ||
| rep = &Report{} | ||
| } |
There was a problem hiding this comment.
Guard the exported writer against a nil bundle.
WriteTKLDB validates path but immediately dereferences b in the ban loop. A nil *ir.Bundle will panic here instead of returning a regular migration error.
Suggested fix
func WriteTKLDB(path string, b *ir.Bundle, opts Options, rep *Report) error {
if path == "" {
return errors.New("WriteTKLDB: path is required")
}
+ if b == nil {
+ return errors.New("WriteTKLDB: bundle is required")
+ }
if rep == nil {
rep = &Report{}
}Also applies to: 56-57
🤖 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 `@tools/obbyircd-migrate/internal/writer/tkldb.go` around lines 33 - 39,
WriteTKLDB currently validates path but may dereference a nil *ir.Bundle (b) in
the ban loop and later code (see ban loop around lines 56-57); add a guard at
the start of WriteTKLDB to check if b == nil and return a clear error (e.g.
"WriteTKLDB: bundle is required") so the function fails gracefully instead of
panicking, and ensure any subsequent places that dereference b (the ban loop and
other uses) can assume b is non-nil.
| ``` | ||
| ┌────────────┐ ┌──────────────┐ ┌────────────┐ | ||
| │ reader │ ──> │ intermediate │ ──> │ writer │ | ||
| │ (per src) │ │ (IR) │ │ (per dest) │ | ||
| └────────────┘ └──────────────┘ └────────────┘ | ||
| ``` |
There was a problem hiding this comment.
Add language tags to the fenced blocks.
The code fences starting at Lines 77 and 89 are missing a language identifier, which is enough to trip MD040 in markdownlint.
Also applies to: 89-104
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 77-77: 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 `@tools/obbyircd-migrate/README.md` around lines 77 - 82, The fenced code
blocks containing the ASCII diagram (the block starting with the box diagram
"┌────────────┐ ┌──────────────┐ ...") and the later block referenced in the
comment must include a language tag to satisfy markdownlint MD040; update those
triple-backtick fences to use a neutral tag like ```text or ```ascii (i.e.,
change the opening ``` to ```text) so the README.md fenced blocks are annotated
without altering their contents.
| - The bcrypt / pbkdf2v2 / crypt-sha256 / crypt-sha512 verifier paths | ||
| in `account-registration.c` are still TODO (PLAN.md §6.3 follow-up). | ||
| Migrated accounts using those schemes will be readable by the | ||
| writer (column populated, hash preserved), but obbyircd can't yet | ||
| *verify* against them — only argon2id works for live login. | ||
| - X-line (gecos ban) doesn't have a native UnrealIRCd equivalent. | ||
| The TKL writer drops X-lines with a warning; create a manual | ||
| spamfilter rule if needed. | ||
| - Memos are not currently written anywhere — the IR carries them but | ||
| no obsidian.db `memos` table exists yet (deferred per PLAN.md §6.2). |
There was a problem hiding this comment.
Refresh the "Known limitations" section to match the current code.
This section still says bcrypt/pbkdf2/crypt verification is unavailable and memos are not written anywhere, but this PR now adds verifier dispatch in account-registration.c and memo inserts in the Obsidian writer. Leaving it as-is will send operators the wrong migration guidance.
🤖 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 `@tools/obbyircd-migrate/README.md` around lines 114 - 123, Update the "Known
limitations" section to reflect the code changes: remove or reword the bullet
that claims bcrypt / pbkdf2v2 / crypt-sha256 / crypt-sha512 verification is
unavailable (these are now handled by the verifier dispatch implemented in
account-registration.c), and remove the claim that memos are not written (the
Obsidian writer now inserts memos into the memos table); keep or adjust the
X-line/gecos-ban note if still accurate and update any PLAN.md cross-references
to the correct section or remove if no longer relevant.
Per review feedback that channel-registration is core obbyircd functionality and shouldn't live under third/. The module's behaviour is unchanged; this just moves the .c file, lists it in src/modules/Makefile.in's MODULES, switches the loadmodule line in modules.default.conf from "third/member-roles" to "member-roles", updates the example conf comment to match, and renames the MOD_HEADER name field accordingly.
ValwareIRC
force-pushed
the
feat/named-modes
branch
from
d8ec5bd to
60679e3
Compare
feat(named-modes): IRCv3 draft/named-modes (PROP command, 96X numerics)
1 participant
Summary
Implements the WIP IRCv3 named-modes spec — long-form mode names (
op,topiclock,ban, …) reachable via a newPROPcommand, with full backwards-compat with the classicMODEletter syntax.Spec referenced: progval/ircv3-specifications @ e28f44f
What's in the box
CmodeAdd/UmodeAddnow refuse registrations without aname. Every mode in core is named (33 chanmodes + 11 usermodes touched). The legacyletteris now optional (0reserved for future name-only modes).op,voice,ban,topiclock, …); UnrealIRCd-specific ones are vendored underobby.world/(obby.world/floodprot,obby.world/nokick, …) so they don't clash with future IRCv3 standardisations.draft/named-modescapability +PROPcommand +RPL_PROPLIST(961) /RPL_ENDOFPROPLIST(960) /RPL_LISTPROPLIST(963) /RPL_ENDOFLISTPROPLIST(962) /RPL_CHMODELIST(964) /RPL_UMODELIST(965).mode.cfilters cap-holders out of itsMODEbroadcast (clicap | CAP_INVERT); thenamed-modesmodule'sHOOKTYPE_LOCAL_CHANMODEthen reassembles the same change asPROP +foo=bar -bazand ships it to cap-holders. Each client sees exactly one form.MAXMODES=N(mirrorsMODES), required by the spec.Backwards compat
MODElines exactly as before.PROPinstead ofMODE(per the spec'sMUST translate).PROP +foo=barfrom a cap-aware client gets translated back intoMODE +X barinternally so the existingcmd_modemachinery does the actual work — no parallel state machine.Known v1 limitations
MODEisn't translated toPROP(channel modes only).PROP +<name-only-mode>errors withERR_UNKNOWNMODE(no non-letter executor yet).PROP <chan> <listmode>only handlesban/banex/invex; vendored list-modes will land in a follow-up.Test plan
Configuration test passed OK)MODE #chan +o nick— recipient still seesMODEPROP #chan +op=nick— recipient seesPROPPROP #chanreturns 961/960PROP #chan banreturns 963/962Summary by CodeRabbit
New Features
Improvements
$)