feat(v3.0): full production upgrade — menu lifecycle, pagination, security, 23 Block 1 fixes - #110
Merged
Merged
Conversation
…urity, 23 Block 1 fixes Menu lifecycle overhaul: - replyMenu() TTL auto-vanish via ttlMs parameter - clearStaleMenuIds() on startup (24h threshold) - mainMenuSentAt / adminMenuSentAt timestamps on user schema - sendHelpMenu() fixed: bare ctx.reply() → replyMenu() (panel stacking bug) - replaceCallbackPanel() fallback now tracks message ID in user state - Admin mode toggle double-fire fixed Feature upgrades: - User giveaway list: sendUserGiveawaysPage() 5/page + 2-min auto-vanish - Admin giveaway panel: activeGiveawaysKeyboard(page) 5/page pagination - pmenu_referral sub-menu with share deep-link button - Admin stats: active window indicator + Refresh button - Admin health panel: inline memory/errors/users/persist-age/uptime - Broadcast builder: Preview button sends to admin DM before mass send Block 1 security & logic fixes: - getStartAppLink(): encodeURIComponent + regex whitelist - referralShareHTML(): escapeHtml() on code param - unwrapTelegramUrl(): safe fallback, scheme whitelist, www.t.me support - getDiscordLink(): null when not configured (no hardcoded fallback) - evaluatePendingActionTimeout(): >= boundary, NaN guard, no createdAt mutation - getPlayLink(): legacy user.playMode fallback restored - Weighted winner pool: splice-after-pick guarantees termination - deploy_status + logs: exec() → execFile() (no shell spawn) - SSHV: rejects null bytes, backticks, $( and ${ before exec - Startup env warnings: ADMIN_IDS, TELEGRAM_CHANNEL_ID, TELEGRAM_GROUP_ID Centralized helpers: computeParticipantWeight(), getRealGiveaways(), isNewUserPromoEligible() Metrics: added runewager_menu_stale_recoveries, pending_actions_timed_out, uptime_seconds load_tooltips.sh: full rewrite — atomic write, --push/--pull flags, parameterized REPO_DIR, HTML-safe tooltips, --dry-run mode, permission checks, guarded git ops Tests: readdirSync wrapped in try/catch; isCatchAllRegexPattern expanded; extractCommandHandlerNames supports let/var/no-semicolon Infrastructure: pre-deploy-checks gate 3b (menu symbols), CHANGELOG.md created, package.json bumped to 3.0.0 All 60 tests pass. node --check clean. https://claude.ai/code/session_01VijmtzjN63WZJy5gYgJAKs
… during PR merges Restores 12 categories of codex changes that were incorrectly overwritten by "current" (main) when resolving merge conflicts across 6 merge commits. Security / TLS hardening (index.js): - Restore resolveTlsCertPathIfAllowed(): multi-directory fallback for TLS certs (PROJECT_DIR, certs/, /etc/ssl, /etc/letsencrypt) — was collapsed to PROJECT_DIR only - Restore isHealthTlsEnabled() to use resolveTlsCertPathIfAllowed() - Restore requestHealthPayload(): centralized HTTP/HTTPS health fetcher with dynamic protocol detection — was inlined with hardcoded https in two places - Update /health command to use requestHealthPayload() (shows protocol label) - Update pamenu_tools_health to use requestHealthPayload() - Update startHealthServer() TLS cert read to use resolveTlsCertPathIfAllowed() Command injection protection (index.js): - commandNeedsConfirmation(): restore pipe-to-bash/zsh detection, destructive-cmd-piped pattern, redirect pattern — main simplified to only catch "sh" - commandBlocked(): restore explicit pipe-to-shell blocker pattern Race condition fixes (index.js): - deleteEphemeralBonusPrompt(): restore runUserMutation guards for safe read-delete-write of ephemeralBonusMsgId/ChatId - sendEphemeralBonusPrompt(): restore guards (claimedPromo check, active promo lookup), per-user dynamic promo lookup via getActivePromoCodeForUser() (was hardcoded promoStore.code), "I Have Claimed" callback button, mutation-safe writes - Add promo_confirm_claimed_next callback handler (button was added, handler missing) Deploy/runtime hardening (deploy.sh, prod-run.sh, runewager.service): - deploy.sh: restore data/backups/ mkdir, chown -R APP_NAME, chmod 0750/0640/0600 permission hardening after npm install - prod-run.sh: restore chmod 0750 for data/data/backups/logs dirs, chmod 0640 for log+session files, chown -R to service user after dir creation - runewager.service: add UMask=0077 (prevents world-readable files from service) .gitignore: - Restore legacy root-level data file patterns: users.json, giveaways.json, promo.json, env.json, analytics*.json (pre-data/ directory structure) All 60 tests pass. node --check clean. https://claude.ai/code/session_01VijmtzjN63WZJy5gYgJAKs
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Reviewer's GuideImplements the v3.0 production upgrade with a menu lifecycle overhaul (stale menu cleanup, TTL-based auto-vanish, better tracking), paginated giveaway views for users and admins, a richer admin health/stats UX, safer URL/command handling and SSHV restrictions, centralized helpers and new metrics, along with a hardened load_tooltips.sh pipeline, improved tests, deployment checks, and file permission tightening. Sequence diagram for paginated user giveaways menu with TTL auto-vanishsequenceDiagram
actor User
participant Telegram
participant Bot
participant sendUserGiveawaysPage
participant replyMenu
User->>Telegram: Tap pmenu_giveaways button
Telegram->>Bot: Callback pmenu_giveaways
Bot->>Bot: getUser(ctx)
Bot->>Telegram: answerCbQuery()
Bot->>sendUserGiveawaysPage: ctx, user, page=1
alt no running giveaways
sendUserGiveawaysPage->>replyMenu: ctx, user, text("No giveaways"), extra(parse_mode, keyboard, ttlMs=120000)
replyMenu->>Bot: clearOldMenus(ctx, user)
Bot-->>replyMenu: menus cleared
replyMenu->>Telegram: ctx.reply(text, telegramExtra)
Telegram-->>replyMenu: sentMessage(message_id, chat.id)
replyMenu->>Bot: store user.lastMenuMsgId, lastMenuChatId
replyMenu->>Telegram: schedule delete after ttlMs
else running giveaways
sendUserGiveawaysPage->>sendUserGiveawaysPage: getRealGiveaways()
sendUserGiveawaysPage->>sendUserGiveawaysPage: compute totalPages, safePage
sendUserGiveawaysPage->>sendUserGiveawaysPage: slice current page giveaways
sendUserGiveawaysPage->>sendUserGiveawaysPage: build text + join buttons + nav buttons
sendUserGiveawaysPage->>replyMenu: ctx, user, text(paginated list), extra(parse_mode, keyboard, ttlMs=120000)
replyMenu->>Bot: clearOldMenus(ctx, user)
Bot-->>replyMenu: menus cleared
replyMenu->>Telegram: ctx.reply(text, telegramExtra)
Telegram-->>replyMenu: sentMessage(message_id, chat.id)
replyMenu->>Bot: store user.lastMenuMsgId, lastMenuChatId
replyMenu->>Telegram: schedule delete after ttlMs
end
%% Pagination callback
User->>Telegram: Tap Next or Prev page button
Telegram->>Bot: Callback user_giveaways_page_N
Bot->>Bot: getUser(ctx)
Bot->>Telegram: answerCbQuery()
Bot->>sendUserGiveawaysPage: ctx, user, page=N
sendUserGiveawaysPage->>replyMenu: ctx, user, text(new page), extra(parse_mode, keyboard, ttlMs=120000)
replyMenu->>Bot: clearOldMenus(ctx, user)
Bot-->>replyMenu: menus cleared
replyMenu->>Telegram: ctx.reply(text, telegramExtra)
Telegram-->>replyMenu: sentMessage(message_id, chat.id)
replyMenu->>Bot: update user.lastMenuMsgId, lastMenuChatId
replyMenu->>Telegram: schedule delete after ttlMs
Sequence diagram for admin health tools using requestHealthPayloadsequenceDiagram
actor Admin
participant Telegram
participant Bot
participant requestHealthPayload
participant HealthServer
Admin->>Telegram: Send /health command
Telegram->>Bot: Command health
Bot->>Bot: requireAdmin(ctx)
Bot->>requestHealthPayload:()
requestHealthPayload->>requestHealthPayload: read PORT, HTTPS_KEY_PATH, HTTPS_CERT_PATH
requestHealthPayload->>Bot: isHealthTlsEnabled()
Bot-->>requestHealthPayload: true/false
alt TLS enabled
requestHealthPayload->>HealthServer: HTTPS GET /health (rejectUnauthorized=false)
HealthServer-->>requestHealthPayload: statusCode, JSON body, protocol https
else TLS disabled
requestHealthPayload->>HealthServer: HTTP GET /health
HealthServer-->>requestHealthPayload: statusCode, JSON body, protocol http
end
requestHealthPayload-->>Bot: { data, statusCode, protocol }
Bot->>Telegram: ctx.reply(formatted JSON with protocol label)
%% Inline admin panel health check
Admin->>Telegram: Tap pamenu_tools_health button
Telegram->>Bot: Callback pamenu_tools_health
Bot->>Bot: requireAdmin(ctx)
Bot->>Telegram: answerCbQuery("Running health check...")
Bot->>requestHealthPayload:()
requestHealthPayload->>Bot: isHealthTlsEnabled()
Bot-->>requestHealthPayload: true/false
requestHealthPayload->>HealthServer: HTTP(S) GET /health
HealthServer-->>requestHealthPayload: data, statusCode, protocol
requestHealthPayload-->>Bot: { data, statusCode, protocol }
Bot->>Telegram: ctx.reply("Health Check (PROTOCOL):" + data slice)
Flow diagram for rewritten load_tooltips.sh pipelinegraph TD
A_start[Start load_tooltips.sh] --> B_resolve[Resolve SCRIPT_DIR and REPO_DIR]
B_resolve --> C_parse_flags[Parse flags --push --dry-run --pull]
C_parse_flags --> D_check_dry_run{DRY_RUN?}
D_check_dry_run -->|yes| E_dry_info[Print target path and tooltip JSON]
E_dry_info --> F_dry_validate[Validate JSON with python3 -m json.tool]
F_dry_validate --> G_dry_exit[Exit without writing files]
D_check_dry_run -->|no| H_perm[Ensure DATA_DIR exists and is writable]
H_perm --> I_pull_check{DO_PULL?}
I_pull_check -->|yes| J_git_pull[git -C REPO_DIR pull origin main]
I_pull_check -->|no| K_skip_pull[Skip git pull]
J_git_pull --> L_build_json
K_skip_pull --> L_build_json[Build TOOLTIP_JSON string]
L_build_json --> M_tmp[Write JSON to TMP_OUT]
M_tmp --> N_validate[Validate TMP_OUT with python3 -m json.tool]
N_validate --> O_atomic[Atomic mv TMP_OUT -> OUT]
O_atomic --> P_gitignore_check{.gitignore exists?}
P_gitignore_check -->|yes| Q_gitignore_entry{Has data/tooltips.json entry?}
Q_gitignore_entry -->|no| R_append_gitignore[Append data/tooltips.json to .gitignore]
Q_gitignore_entry -->|yes| S_skip_gitignore[Skip .gitignore change]
P_gitignore_check -->|no| T_no_gitignore[Skip .gitignore handling]
R_append_gitignore --> U_push_check
S_skip_gitignore --> U_push_check[Check DO_PUSH]
T_no_gitignore --> U_push_check{DO_PUSH?}
U_push_check -->|no| V_done[Print completion message and exit]
U_push_check -->|yes| W_stage[git add .gitignore in REPO_DIR]
W_stage --> X_diff{Has staged .gitignore changes?}
X_diff -->|no| Y_no_commit[No changes to commit]
X_diff -->|yes| Z_commit_push[Commit and push .gitignore]
Y_no_commit --> V_done
Z_commit_push --> V_done
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis PR implements version 3.0.0 with comprehensive enhancements: security hardening (TLS path validation, URL normalization, environment checks), observability improvements (health metrics, startup validations), giveaway system features (pagination, weighted selection), menu lifecycle management, deployment automation (permission controls, atomic writes), and extensive documentation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:XL
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
requestHealthPayload()you dynamicallyrequire('http')/require('https')on each call; consider lifting these requires to the module scope to avoid repeated synchronous requires in hot admin paths. - The SSHV executor comment mentions
spawnbut the implementation still usesexec; either switch tospawnas described or update the comment to reflect the actual execution strategy to avoid confusion for future maintainers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `requestHealthPayload()` you dynamically `require('http')`/`require('https')` on each call; consider lifting these requires to the module scope to avoid repeated synchronous requires in hot admin paths.
- The SSHV executor comment mentions `spawn` but the implementation still uses `exec`; either switch to `spawn` as described or update the comment to reflect the actual execution strategy to avoid confusion for future maintainers.
## Individual Comments
### Comment 1
<location path="index.js" line_range="557-566" />
<code_context>
+ if (!Number.isFinite(created) || !Number.isFinite(now)) {
+ const expiredType = ACTION_LABELS[user.pendingAction.type] || 'current action';
+ user.pendingAction = null;
+ pendingActionsTimedOut++;
+ return { hadPending: true, expired: true, expiredType };
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid incrementing `pendingActionsTimedOut` before it is initialized to prevent a temporal dead-zone ReferenceError.
Since `pendingActionsTimedOut` is declared with `let` later in the file, any future call to `evaluatePendingActionTimeout` that runs before the declaration will throw a `ReferenceError` due to the temporal dead zone. To make this robust against refactors, either move the `menuStaleRecoveries`/`pendingActionsTimedOut` declarations above all usages, or switch to `var` if you explicitly want hoisting semantics.
</issue_to_address>
### Comment 2
<location path="index.js" line_range="6862-6864" />
<code_context>
- const { exec } = require('child_process');
- const cmd = `journalctl -u runewager.service -n ${lines} --no-pager 2>/dev/null || tail -n ${lines} /tmp/runewager-3000.log 2>/dev/null || echo "No logs found."`;
- exec(cmd, { timeout: 8000 }, async (err, stdout) => {
+ const lineCount = String(Math.min(Number(parts[1]) || 50, 200));
+ // v3.0 fix: use execFile with shell:false — lineCount is a validated safe integer string
+ execFile('journalctl', ['-u', 'runewager.service', '-n', lineCount, '--no-pager'], { timeout: 8000 }, async (err, stdout) => {
const output = (stdout || '').trim() || (err ? `Error: ${err.message}` : 'No logs found.');
const chunks = [];
</code_context>
<issue_to_address>
**suggestion:** Consider preserving a non-journalctl fallback for the logs command while keeping `execFile`-based hardening.
Switching to `execFile('journalctl', ...)` drops the earlier `tail` fallback when `journalctl` is missing or the unit name is wrong. On systems without systemd or with different service names, this will now just return an error instead of any logs. To keep both the security hardening and resilience, consider calling `execFile('tail', ['-n', lineCount, '/tmp/runewager-3000.log'])` if the `journalctl` call fails and `stdout` is empty.
Suggested implementation:
```javascript
const { execFile } = require('child_process');
bot.command(
'logs',
safeAdminHandler(
'logs',
{ usage: '/logs [lines]', example: '/logs 50' },
async (ctx) => {
if (!requireAdmin(ctx)) return;
const parts = (ctx.message.text || '').trim().split(/\s+/);
const lineCount = String(Math.min(Number(parts[1]) || 50, 200));
// helper to split output into Telegram-safe chunks and send
const sendOutput = async (text) => {
const chunks = [];
for (let i = 0; i < text.length; i += 3900) {
chunks.push(text.slice(i, i + 3900));
}
for (const chunk of chunks) {
// adjust parse_mode/formatting if your bot uses a different convention
// Using MarkdownV2 code block to avoid log formatting issues.
await ctx
.reply(`\`\`\`\n${chunk}\n\`\`\``, { parse_mode: 'MarkdownV2' })
.catch(() => {});
}
};
await new Promise((resolve) => {
// Primary: systemd journal logs
execFile(
'journalctl',
['-u', 'runewager.service', '-n', lineCount, '--no-pager'],
{ timeout: 8000 },
(err, stdout) => {
const primaryError = err;
const journalOutput = (stdout || '').trim();
if (journalOutput) {
// We got logs from journalctl; no fallback needed.
sendOutput(journalOutput).finally(resolve);
return;
}
// Fallback: logs from the local file via tail
execFile(
'tail',
['-n', lineCount, '/tmp/runewager-3000.log'],
{ timeout: 8000 },
(tailErr, tailStdout) => {
const fallbackOutput = (tailStdout || '').trim();
const finalOutput =
fallbackOutput ||
(tailErr || primaryError
? `Error: ${(tailErr || primaryError).message}`
: 'No logs found.');
sendOutput(finalOutput).finally(resolve);
}
);
}
);
});
}
)
);
```
1. If `index.js` already imports from `child_process`, adjust the `SEARCH`/`REPLACE` so you either add `execFile` to the existing destructuring (e.g. `const { execFile } = require('child_process');` or `const { exec, execFile } = require('child_process');`), rather than introducing a duplicate `require`.
2. If the file already defines a common `sendOutput`/chunking helper for other commands, you may want to refactor the duplicated logic here to call that helper instead of inlining it.
3. If your bot does not use `MarkdownV2` / code blocks for logs, update the `ctx.reply` options to match your existing formatting conventions.
</issue_to_address>
### Comment 3
<location path="index.js" line_range="2530-2538" />
<code_context>
}
}
+ // v3.0 fix: reject null bytes, backticks, command substitution to reduce injection surface
+ if (/[\x00`]|\$\(|\$\{/.test(command)) {
+ session.buffer = '[SSHV] Command rejected: contains disallowed characters (null byte, backtick, or $( / ${).';
+ await renderSshvConsole(ctx, session, 'Command rejected.');
+ return;
+ }
await ctx.reply(`⏳ Running: \`${escapeMarkdownFull(command)}\``, { parse_mode: 'MarkdownV2' });
await new Promise((resolve) => {
+ // Use spawn with shell:true but command is admin-only and blocked list is enforced above
const child = exec(command, { cwd: session.cwd, timeout: 8000, maxBuffer: 2 * 1024 * 1024 }, async (err, stdout, stderr) => {
const out = `${stdout || ''}${stderr || ''}`.trim();
</code_context>
<issue_to_address>
**nitpick:** The comment mentions `spawn` but the code still uses `exec`, which can be misleading for future maintainers.
In `executeSshvCommand`, the inline comment says "Use spawn with shell:true" but the code still calls `exec(command, ...)`. Please either update the comment to match the current `exec` usage or switch to `spawn` if that’s what you intended, to avoid confusion in this security-sensitive path.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Nitpicks 🔍
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
index.js (3)
14-19:⚠️ Potential issue | 🟠 Major
ADMIN_IDSneeds strict numeric validation, not just non-empty checksLine 29 warns only on empty arrays, but
ADMIN_IDScan still be non-empty with invalid values (NaN) from Line 18. That silently breaks admin auth and admin-only workflows.Suggested fix
const ADMIN_IDS = (process.env.ADMIN_IDS || '') .split(',') .map((id) => id.trim()) .filter(Boolean) - .map((id) => Number(id)); + .map((id) => Number(id)) + .filter((id) => Number.isInteger(id) && id > 0); // Startup env var validation — warn on missing optional-but-important vars if (!ADMIN_IDS || ADMIN_IDS.length === 0) { console.error('[WARN] ADMIN_IDS is empty — no admins configured. Admin commands will be inaccessible.'); + throw new Error('Invalid ADMIN_IDS: provide a comma-separated list of numeric Telegram user IDs.'); }Based on learnings: Validate required environment variables are present and properly formatted before bot startup.
Also applies to: 27-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.js` around lines 14 - 19, ADMIN_IDS parsing currently coerces env values with Number(...) which can produce NaN and silently break admin checks; update the logic that builds ADMIN_IDS (using process.env.ADMIN_IDS and constant ADMIN_IDS) to validate each trimmed entry is a valid integer (e.g., use a numeric parse check and Number.isFinite/Number.isInteger) and either filter out invalid entries with a warning or throw an error on startup if any invalid IDs are present; ensure you log the offending raw values and fail fast when ADMIN_IDS is required and empty after validation so admin-only workflows don't silently break.
4215-4237:⚠️ Potential issue | 🟡 MinorAdmin giveaway text and action list are built from different datasets
buildActiveGiveawaysText()excludes test giveaways, but the paginated keyboard flow here is fed with unfiltered running giveaways. That can create page/action mismatches vs the visible text.Also applies to: 7763-7774
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.js` around lines 4215 - 4237, activeGiveawaysKeyboard is building the paginated keyboard from the unfiltered giveaways list, which can desynchronize it from the visible text produced by buildActiveGiveawaysText (which excludes test giveaways); fix by applying the same filtering used by buildActiveGiveawaysText to the giveaways array before paging (or accept a pre-filtered list) in activeGiveawaysKeyboard and the other duplicate implementation (lines noted around the second occurrence), so that functions like activeGiveawaysKeyboard, buildActiveGiveawaysText and the admin_gw_page callbacks operate on the identical filtered dataset and produce matching buttons and text.
66-78:⚠️ Potential issue | 🟠 MajorNullable Discord links can propagate into invalid URL buttons
At Line 68 and Line 77,
getDiscordLink()returnsnullfor invalid input. Several menu/command builders pass Discord URLs directly intoMarkup.button.url(...); passingnullcan fail at runtime when those flows render.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.js` around lines 66 - 78, getDiscordLink currently returns null for invalid inputs which then flows into UI builders (e.g., Markup.button.url) and causes runtime failures; change getDiscordLink (and its use sites) so it never passes null into URL buttons: make getDiscordLink return undefined for non-valid links (or a guaranteed-valid fallback string) and update all call sites that pass its result into Markup.button.url to guard on truthiness (e.g., const discord = getDiscordLink(...); if (discord) add Markup.button.url(discord) ). Also keep the parsing logic using unwrapTelegramUrl and URL intact and only return a URL string when parsed.protocol === 'https:' and hostname matches discord hosts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.js`:
- Around line 8835-8844: The health panel reads a nonexistent field
_errorRate.windowErrors so it always shows 0; update the template to use the
actual tracked property (use _errorRate.count) or compute the windowed error
total if intended—replace `_errorRate.windowErrors` with `(_errorRate && typeof
_errorRate.count === 'number' ? _errorRate.count : 0)` in the healthText
construction (or implement the proper window calculation using
_errorRate.windowStart/_errorRate.count if a time-windowed value is required).
- Around line 6862-6864: The current calculation for lineCount allows zero or
negative values (const lineCount = String(Math.min(Number(parts[1]) || 50,
200))) which can produce invalid journalctl behavior; replace this with a
clamped positive integer in the range [1,200] by parsing Number(parts[1]) into
an integer, forcing a default (e.g., 50) when NaN, then applying Math.max(1,
Math.min(parsed, 200)), and finally converting to a string before passing to
execFile (update the variable used in the execFile call that targets
'journalctl' with ['-u','runewager.service','-n', lineCount, '--no-pager']).
- Around line 561-563: The timeout boundary was flipped so an age exactly equal
to PENDING_ACTION_TIMEOUT_MS is treated as expired, conflicting with the
`/testall` checks; update the expiration check around (now - created) to treat
age === PENDING_ACTION_TIMEOUT_MS as NOT expired (use <= instead of < for the
non-expired branch) so the returned object from this check
(hadPending/expired/expiredType) preserves the `/testall` expectations and keeps
the final summary `TestAll complete — X passed, Y warnings, Z failures`
unchanged.
In `@load_tooltips.sh`:
- Around line 110-119: The conditional that checks for staged .gitignore changes
currently has "|| true", which forces the if to always succeed; remove the "||
true" from the if condition so the git diff --cached --quiet -- .gitignore
return code drives the branch (i.e. change the line to: if git -C "$REPO_DIR"
diff --cached --quiet -- .gitignore; then ...), or alternatively temporarily
disable errexit just for that check (e.g. run the diff under set +e / set -e) so
the info/warn/git commit/push logic in load_tooltips.sh runs correctly when
.gitignore has changes.
---
Outside diff comments:
In `@index.js`:
- Around line 14-19: ADMIN_IDS parsing currently coerces env values with
Number(...) which can produce NaN and silently break admin checks; update the
logic that builds ADMIN_IDS (using process.env.ADMIN_IDS and constant ADMIN_IDS)
to validate each trimmed entry is a valid integer (e.g., use a numeric parse
check and Number.isFinite/Number.isInteger) and either filter out invalid
entries with a warning or throw an error on startup if any invalid IDs are
present; ensure you log the offending raw values and fail fast when ADMIN_IDS is
required and empty after validation so admin-only workflows don't silently
break.
- Around line 4215-4237: activeGiveawaysKeyboard is building the paginated
keyboard from the unfiltered giveaways list, which can desynchronize it from the
visible text produced by buildActiveGiveawaysText (which excludes test
giveaways); fix by applying the same filtering used by buildActiveGiveawaysText
to the giveaways array before paging (or accept a pre-filtered list) in
activeGiveawaysKeyboard and the other duplicate implementation (lines noted
around the second occurrence), so that functions like activeGiveawaysKeyboard,
buildActiveGiveawaysText and the admin_gw_page callbacks operate on the
identical filtered dataset and produce matching buttons and text.
- Around line 66-78: getDiscordLink currently returns null for invalid inputs
which then flows into UI builders (e.g., Markup.button.url) and causes runtime
failures; change getDiscordLink (and its use sites) so it never passes null into
URL buttons: make getDiscordLink return undefined for non-valid links (or a
guaranteed-valid fallback string) and update all call sites that pass its result
into Markup.button.url to guard on truthiness (e.g., const discord =
getDiscordLink(...); if (discord) add Markup.button.url(discord) ). Also keep
the parsing logic using unwrapTelegramUrl and URL intact and only return a URL
string when parsed.protocol === 'https:' and hostname matches discord hosts.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitignoreCHANGELOG.mdCLAUDE.mddeploy.shindex.jsload_tooltips.shpackage.jsonprod-run.shrunewager.servicescripts/pre-deploy-checks.shtest/smoke.test.jstodolist.md
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+110
to
+119
| # Only commit if .gitignore actually changed | ||
| if git -C "$REPO_DIR" diff --cached --quiet -- .gitignore || true; then | ||
| info "No .gitignore changes to commit." | ||
| else | ||
| git -C "$REPO_DIR" commit -m "chore: ensure data/tooltips.json is in .gitignore" -- .gitignore \ | ||
| || warn "git commit failed (non-fatal)" | ||
| info "Pushing to origin..." | ||
| git -C "$REPO_DIR" push origin main || error "git push failed" | ||
| info "Push complete." | ||
| fi |
There was a problem hiding this comment.
Suggestion: The condition that checks whether .gitignore has staged changes is incorrectly written as if git diff ... || true; then, which always evaluates to success and therefore always executes the "no changes" branch, meaning the script will never actually commit or push .gitignore even when there are real changes; remove the || true and rely on the if conditional semantics so the commit/push path runs only when there are staged changes. [logic error]
Severity Level: Major ⚠️
- .gitignore auto-commit never runs when using --push.
- Remote repo may miss data/tooltips.json ignore rule.
- Other clones risk committing data/tooltips.json accidentally.
- --push flag behavior contradicts documented script usage.
Suggested change
| # Only commit if .gitignore actually changed | |
| if git -C "$REPO_DIR" diff --cached --quiet -- .gitignore || true; then | |
| info "No .gitignore changes to commit." | |
| else | |
| git -C "$REPO_DIR" commit -m "chore: ensure data/tooltips.json is in .gitignore" -- .gitignore \ | |
| || warn "git commit failed (non-fatal)" | |
| info "Pushing to origin..." | |
| git -C "$REPO_DIR" push origin main || error "git push failed" | |
| info "Push complete." | |
| fi | |
| # Only commit if .gitignore actually changed | |
| if git -C "$REPO_DIR" diff --cached --quiet -- .gitignore; then | |
| info "No .gitignore changes to commit." | |
| else | |
| git -C "$REPO_DIR" commit -m "chore: ensure data/tooltips.json is in .gitignore" -- .gitignore \ | |
| || warn "git commit failed (non-fatal)" | |
| info "Pushing to origin..." | |
| git -C "$REPO_DIR" push origin main || error "git push failed" | |
| info "Push complete." | |
| fi |
Steps of Reproduction ✅
1. From the repository root `/workspace/Runewager`, ensure `.gitignore` does not yet
contain the line `data/tooltips.json` (this is what the script is meant to add/guard at
`load_tooltips.sh:98-101`).
2. Run the script with push enabled: `./load_tooltips.sh --push` (entry point at
`/workspace/Runewager/load_tooltips.sh:1`, push logic at lines 105-120).
3. The script appends `data/tooltips.json` to `.gitignore` if missing
(`load_tooltips.sh:98-101`), then stages `.gitignore` via `git -C "$REPO_DIR" add
"$GITIGNORE"` (`line 108`).
4. The conditional at `load_tooltips.sh:111` executes: `if git -C "$REPO_DIR" diff
--cached --quiet -- .gitignore || true; then`. Because of `|| true`, this `if` branch
always evaluates as success, so the script always logs `info "No .gitignore changes to
commit."` (`line 112`) and never enters the `else` block.
5. As a result, the commit and push commands in the `else` block (`git commit` and `git
push` at lines 114-117) are never executed, even though `.gitignore` was actually changed
and staged. Verifying with `git status` shows `.gitignore` modified/staged locally but no
commit created; the remote repository remains without the updated ignore rule.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** load_tooltips.sh
**Line:** 110:119
**Comment:**
*Logic Error: The condition that checks whether `.gitignore` has staged changes is incorrectly written as `if git diff ... || true; then`, which always evaluates to success and therefore always executes the "no changes" branch, meaning the script will never actually commit or push `.gitignore` even when there are real changes; remove the `|| true` and rely on the `if` conditional semantics so the commit/push path runs only when there are staged changes.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
- TDZ fix: move pendingActionsTimedOut/menuStaleRecoveries declarations
before evaluatePendingActionTimeout to eliminate temporal dead-zone risk
- Timeout boundary: revert < to <= so age exactly equal to 15m is NOT
expired — aligns with /testall check and unit test expectations
- /logs line count: clamp to [1, 200] (adds Math.max(1,...) lower bound
to reject negative/zero values from user input)
- Health panel: fix _errorRate.windowErrors → _errorRate.count (field
did not exist; .count is the correct field on _errorRate object)
- /logs fallback: add execFile('tail') fallback when journalctl errors
with no output (non-systemd systems); uses BOT_LOG_FILE env or default
- executeSshvCommand: fix comment — said "spawn" but code uses exec();
updated to accurately describe exec with shell:true (admin-only)
- load_tooltips.sh: remove || true that defeated diff --cached check,
causing .gitignore changes to never be committed on --push
All 60 tests pass. node --check clean.
https://claude.ai/code/session_01VijmtzjN63WZJy5gYgJAKs
This was referenced Feb 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Menu lifecycle overhaul:
Feature upgrades:
Block 1 security & logic fixes:
Centralized helpers: computeParticipantWeight(), getRealGiveaways(), isNewUserPromoEligible()
Metrics: added runewager_menu_stale_recoveries, pending_actions_timed_out, uptime_seconds
load_tooltips.sh: full rewrite — atomic write, --push/--pull flags, parameterized REPO_DIR,
HTML-safe tooltips, --dry-run mode, permission checks, guarded git ops
Tests: readdirSync wrapped in try/catch; isCatchAllRegexPattern expanded;
extractCommandHandlerNames supports let/var/no-semicolon
Infrastructure: pre-deploy-checks gate 3b (menu symbols), CHANGELOG.md created,
package.json bumped to 3.0.0
All 60 tests pass. node --check clean.
https://claude.ai/code/session_01VijmtzjN63WZJy5gYgJAKs
Summary by Sourcery
Upgrade bot to v3.0 with a hardened menu lifecycle, safer link and command handling, richer admin/user panels, and improved deployment tooling and metrics.
New Features:
Bug Fixes:
Enhancements:
Build:
Deployment:
Documentation:
Tests:
Chores:
CodeAnt-AI Description
v3.0: menu lifecycle, paginated giveaways, referral submenu, admin health, TTL auto-vanish, and security fixes
What Changed
Impact
✅ Fewer stacked/duplicate menus for users✅ Clearer and safer referral/share links✅ Fewer admin command injection and unsafe-output risks💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Security
Bug Fixes
Documentation
Scripts/Tooling
Tests
Chores