test(chat-filter): add coverage for ALLOWED_CHAT_IDS propagation

[prekshivyas]

Summary

• Extracts chat ID parsing/filtering from telegram-bridge.js into a testable bin/lib/chat-filter.js module
• Adds 8 tests covering the full ALLOWED_CHAT_IDS path: shell env → nohup → node process → parsing → filtering
• Fixes minor bug where trailing commas (e.g. "111,,222,") produced empty entries in the allowlist

Fixes #896

Test plan

• npm test — 32/32 passing
• All pre-commit hooks pass
• Integration test: env var survives nohup in start-services.sh
• E2E test: nohup child parses + filters using chat-filter module
• Source assertion: telegram-bridge.js uses the module with correct env var name
• Unit tests: whitespace trimming, single ID, trailing commas, empty/undefined input

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added configurable chat-ID allowlist for the Telegram bridge, allowing admins to restrict access via an environment variable; parsing handles commas, whitespace, and empty entries, and unset values disable filtering.

• Tests

  • Added unit and end-to-end tests verifying environment propagation, parsing behavior, trimming/empty-entry handling, and allow/deny enforcement.

[coderabbitai]

📝 Walkthrough

Walkthrough

A new chat-filter module is added to parse and enforce ALLOWED_CHAT_IDS; the Telegram bridge now uses it instead of inline parsing. Tests are added to verify parsing, allowlist behavior, and that ALLOWED_CHAT_IDS is propagated into child processes started by start-services.sh.

Changes

Cohort / File(s)	Summary
Chat Filter Module bin/lib/chat-filter.js	New module exporting parseAllowedChatIds(raw) which returns `string[]
Telegram Bridge Integration scripts/telegram-bridge.js	Replaced inline CSV parsing of process.env.ALLOWED_CHAT_IDS with parseAllowedChatIds(...) and changed access check to isChatAllowed(ALLOWED_CHATS, chatId); added import from bin/lib/chat-filter.js.
Tests test/service-env.test.js	Added unit tests for parsing/filtering functions, source-level assertions that telegram-bridge.js uses the new module, and end-to-end tests validating ALLOWED_CHAT_IDS propagation and enforcement in a nohup child process.

Sequence Diagram

sequenceDiagram
    participant User as User/Admin
    participant StartServices as start-services.sh
    participant ChildProcess as telegram-bridge.js (child)
    participant ChatFilter as chat-filter module
    participant TelegramAPI as Telegram API
    participant IncomingMsg as Incoming Message

    User->>StartServices: Set ALLOWED_CHAT_IDS env var
    StartServices->>ChildProcess: Start child (nohup) with env vars
    ChildProcess->>ChatFilter: parseAllowedChatIds(process.env.ALLOWED_CHAT_IDS)
    ChatFilter-->>ChildProcess: parsed array or null
    IncomingMsg->>TelegramAPI: User sends message
    TelegramAPI->>ChildProcess: Deliver message with chatId
    ChildProcess->>ChatFilter: isChatAllowed(ALLOWED_CHATS, chatId)
    alt allowed (whitelist match or null)
        ChatFilter-->>ChildProcess: true
        ChildProcess->>ChildProcess: process message
    else denied
        ChatFilter-->>ChildProcess: false
        ChildProcess->>ChildProcess: ignore/reject message
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I nibbled env vars, tidy and neat,
Split the commas and trimmed every seat.
Into the bridge they hop with delight,
Allowing the right chats, denying the slight.
Hooray — the rabbit says, "All systems set and light!"

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main changes: extracting chat filtering logic into a new module and adding comprehensive test coverage for ALLOWED_CHAT_IDS propagation.
Linked Issues check	✅ Passed	The PR addresses issue #896 by extracting chat-filter logic into a reusable module, updating telegram-bridge to use it, and adding comprehensive tests verifying ALLOWED_CHAT_IDS propagation and enforcement.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the ALLOWED_CHAT_IDS propagation issue and improving test coverage; no out-of-scope modifications detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch test/allowed-chat-ids-896

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/service-env.test.js`:
- Around line 191-193: The PATH in the test wrapper still includes system
directories (/usr/bin, /bin, /usr/local/bin) which exposes real
openshell/cloudflared; update the PATH construction used in both wrapper blocks
(the lines setting NODE_DIR and export PATH) to exclude those system directories
and only include the computed NODE_DIR plus the test fake-bridge/bin directory
(or explicitly prepend the fake-bridge path and NODE_DIR), ensuring both
occurrences (around the current diff and the second block at lines ~266-267) are
changed so the fake wrappers are isolated from real system binaries.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 698edb18-81e9-4630-accb-9a80126b60d7

📥 Commits

Reviewing files that changed from the base of the PR and between b999c0e and 0ac135c.

📒 Files selected for processing (3)

• bin/lib/chat-filter.js
• scripts/telegram-bridge.js
• test/service-env.test.js

[coderabbitai]

♻️ Duplicate comments (1)

test/service-env.test.js (1)

157-160: ⚠️ Potential issue | 🟠 Major

Coverage is still one layer short of the reported regression path.

Line 160 and Line 233 run patched start-services.sh directly; issue #896 reproduces from nemoclaw start. These tests can still pass if the outer CLI layer drops ALLOWED_CHAT_IDS. Please add one end-to-end assertion through the real nemoclaw start entrypoint (or its launcher module).

Also applies to: 233-234

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/service-env.test.js` around lines 157 - 160, The tests for
"ALLOWED_CHAT_IDS propagation (issue `#896`)" only invoke the patched
start-services.sh directly and miss the outer CLI path; update the test in
test/service-env.test.js to call the real nemoclaw start entrypoint (or require
and invoke the launcher module used by the CLI) and assert end-to-end that
ALLOWED_CHAT_IDS is present in the environment of the spawned nohup child (for
example by spawning the CLI via child_process.exec/spawn or calling the launcher
function, then inspecting the child process env or the nohup output file for the
variable). Keep the existing direct-start-services.sh tests, but add this single
end-to-end assertion using the CLI entrypoint name "nemoclaw start" (or the
launcher module) and the same ALLOWED_CHAT_IDS value used elsewhere in the
suite.

🧹 Nitpick comments (1)

test/service-env.test.js (1)

224-230: Source-string assertions here are brittle to harmless refactors.

These checks are tightly coupled to exact formatting/import text. Consider regex/semantic assertions (e.g., behavior-level checks only) so equivalent refactors don’t cause false failures.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/service-env.test.js` around lines 224 - 230, The assertions in
test/service-env.test.js are brittle because they assert exact source strings;
update the checks that reference bridgeSrc to use regex or behavior-level
assertions instead of exact text: replace
expect(bridgeSrc).toContain('require("../bin/lib/chat-filter")') and the exact
parseAllowedChatIds/process.env and isChatAllowed/ALLOWED_CHATS checks with
regex matches (e.g., match
parseAllowedChatIds\s*\(\s*process\.env\.ALLOWED_CHAT_IDS\s*\) and
isChatAllowed\s*\(\s*ALLOWED_CHATS\s*,\s*chatId\s*\)) and replace the
inline-split negative assertion with a regex that ignores whitespace/quote
variations (e.g., ensure no /\.split\(\s*['"].+?['"]\s*\)\.map\(/).
Alternatively, convert these source-string checks into behavior-level assertions
by importing/execing the module and asserting parseAllowedChatIds and
isChatAllowed are called/used correctly when given ALLOWED_CHAT_IDS.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@test/service-env.test.js`:
- Around line 157-160: The tests for "ALLOWED_CHAT_IDS propagation (issue `#896`)"
only invoke the patched start-services.sh directly and miss the outer CLI path;
update the test in test/service-env.test.js to call the real nemoclaw start
entrypoint (or require and invoke the launcher module used by the CLI) and
assert end-to-end that ALLOWED_CHAT_IDS is present in the environment of the
spawned nohup child (for example by spawning the CLI via
child_process.exec/spawn or calling the launcher function, then inspecting the
child process env or the nohup output file for the variable). Keep the existing
direct-start-services.sh tests, but add this single end-to-end assertion using
the CLI entrypoint name "nemoclaw start" (or the launcher module) and the same
ALLOWED_CHAT_IDS value used elsewhere in the suite.

---

Nitpick comments:
In `@test/service-env.test.js`:
- Around line 224-230: The assertions in test/service-env.test.js are brittle
because they assert exact source strings; update the checks that reference
bridgeSrc to use regex or behavior-level assertions instead of exact text:
replace expect(bridgeSrc).toContain('require("../bin/lib/chat-filter")') and the
exact parseAllowedChatIds/process.env and isChatAllowed/ALLOWED_CHATS checks
with regex matches (e.g., match
parseAllowedChatIds\s*\(\s*process\.env\.ALLOWED_CHAT_IDS\s*\) and
isChatAllowed\s*\(\s*ALLOWED_CHATS\s*,\s*chatId\s*\)) and replace the
inline-split negative assertion with a regex that ignores whitespace/quote
variations (e.g., ensure no /\.split\(\s*['"].+?['"]\s*\)\.map\(/).
Alternatively, convert these source-string checks into behavior-level assertions
by importing/execing the module and asserting parseAllowedChatIds and
isChatAllowed are called/used correctly when given ALLOWED_CHAT_IDS.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ab462a35-970a-4a11-bc37-e7b0a2bb7604

📥 Commits

Reviewing files that changed from the base of the PR and between 0ac135c and c1ae639.

📒 Files selected for processing (1)

• test/service-env.test.js