Claude/cpu guard script dn d8u - #129
Merged
Merged
Conversation
…eout Telegraf's long-polling uses getUpdates with timeout=50 (Telegram holds the connection for up to 50 seconds waiting for new updates). The callApi patch was wrapping ALL methods — including getUpdates — with globalThrottle, which has a hard 15-second race. This fired on every poll cycle, throwing 'rateLimiter: API call timed out after 15000ms', causing bot.launch() to reject and systemd to enter an infinite restart loop. Fix: add getUpdates (and Telegraf startup/lifecycle calls: getMe, deleteWebhook, setWebhook, getWebhookInfo, close, logOut) to the _MANAGED bypass set so they call the original callApi directly without the timeout wrapper. All 60 unit tests pass. Bot loads cleanly in CI mode. https://claude.ai/code/session_01WpEqAiDbpqnBywMjYXJHf4
…hod sets Two fixes: 1. index.js — ensureQaArtifacts(), persistQaProviderState(), ensureQaDirs(): All QA file writes are now wrapped in try/catch and treated as non-fatal. The bot service runs as the 'runewager' system user but qa/ is owned by root, so writeFileSync was throwing EACCES on every startup, which was caught by startBot()'s outer try/catch and called process.exit(1). 2. telegramSafe.js — callApi bypass list refactored (code review feedback): The inline _MANAGED set is replaced with three named, exported constants: - LONG_POLL_METHODS : getUpdates (must bypass 15 s timeout) - LIFECYCLE_METHODS : getMe, deleteWebhook, setWebhook, etc. - INDIVIDUALLY_PATCHED_METHODS : sendMessage, sendPhoto, etc. Combined into CALLAPI_BYPASS_METHODS used by the callApi patch. All sets are exported so test suites can assert bypass membership without requiring a live Telegraf instance. All 60 unit tests pass. Module loads cleanly in CI mode. https://claude.ai/code/session_01WpEqAiDbpqnBywMjYXJHf4
|
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 GuideRefines Telegram callApi rate-limiting bypass logic using categorized method sets and exposes them for testing, and hardens QA artifact directory/file handling so permission issues never crash the process and are logged as warnings instead. Sequence diagram for updated Telegram callApi rate-limiting and bypass logicsequenceDiagram
actor User
participant Bot
participant tg
participant globalThrottle
participant TelegramAPI
User->>Bot: triggerTelegramAction
Bot->>tg: callApi sendMessage, data, signal
Note over tg: sendMessage is in CALLAPI_BYPASS_METHODS
tg->>TelegramAPI: callApi sendMessage, data, signal
TelegramAPI-->>tg: response
tg-->>Bot: response
Bot-->>User: success
User->>Bot: triggerOtherAction
Bot->>tg: callApi unknownMethod, data, signal
Note over tg: unknownMethod not in CALLAPI_BYPASS_METHODS
tg->>globalThrottle: schedule _callApi unknownMethod, data, signal
globalThrottle->>TelegramAPI: callApi unknownMethod, data, signal
TelegramAPI-->>globalThrottle: response or error
globalThrottle-->>tg: response
tg-->>Bot: response
Bot-->>User: success or failure
Flow diagram for QA artifacts creation with non-fatal error handlingflowchart TD
A[start ensureQaArtifacts] --> B[try ensureQaDirs]
B --> C[try write qaRepoInfoFile]
C --> D[try write qaCapabilitiesFile]
D --> E[call persistQaProviderState]
E --> F{persistQaProviderState success?}
F -->|yes| G[ensureQaArtifacts complete]
F -->|no - exception| G
B -.on error.-> H[catch error in ensureQaArtifacts]
C -.on error.-> H
D -.on error.-> H
E -.on error.-> I[catch error in persistQaProviderState]
I --> G
H --> J[logEvent warn
ensureQaArtifacts skipping QA artifact write]
J --> G
subgraph persistQaProviderState
K[start persistQaProviderState] --> L[try ensureQaDirs]
L --> M[try write qaProviderStateFile]
M --> N[persistQaProviderState return]
L -.on error.-> O[catch and ignore error]
M -.on error.-> O
O --> N
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChanges enhance resilience by wrapping QA directory creation, state persistence, and artifact generation in try-catch blocks, and refactor the API rate-limiter bypass logic in telegramSafe.js by centralizing bypass method definitions into exported constant sets. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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:M
![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 1 issue, and left some high level feedback:
- The empty catch blocks around
fs.mkdirSyncinensureQaDirsandpersistQaProviderStatefully swallow any filesystem issues; consider at least logging at a debug level or restricting the catch to expected error codes (e.g. EEXIST, EACCES) so that unexpected failures don't go unnoticed. - In
ensureQaArtifacts, the catch block assumes a permission issue; it might be more robust to branch onerror.code(e.g. only suppress EACCES/EPERM) and rethrow or log differently for other error types to avoid masking genuine bugs like invalid paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The empty catch blocks around `fs.mkdirSync` in `ensureQaDirs` and `persistQaProviderState` fully swallow any filesystem issues; consider at least logging at a debug level or restricting the catch to expected error codes (e.g. EEXIST, EACCES) so that unexpected failures don't go unnoticed.
- In `ensureQaArtifacts`, the catch block assumes a permission issue; it might be more robust to branch on `error.code` (e.g. only suppress EACCES/EPERM) and rethrow or log differently for other error types to avoid masking genuine bugs like invalid paths.
## Individual Comments
### Comment 1
<location path="index.js" line_range="4884-4888" />
<code_context>
}
function persistQaProviderState() {
- ensureQaDirs();
- fs.writeFileSync(qaProviderStateFile, JSON.stringify(qaRuntime.providerStatus, null, 2));
+ try {
+ ensureQaDirs();
+ fs.writeFileSync(qaProviderStateFile, JSON.stringify(qaRuntime.providerStatus, null, 2));
+ } catch (_) { /* non-fatal — qa/ dir may not be writable by the service user */ }
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Swallowing all errors in persistQaProviderState makes diagnosing real issues difficult.
Making this non‑fatal is reasonable, but the empty catch masks all failures (including unexpected ones like JSON serialization issues). Please either log the error (even at debug/info) or restrict the catch to the expected cases (e.g., permission errors) so unexpected issues remain visible.
Suggested implementation:
```javascript
function persistQaProviderState() {
try {
ensureQaDirs();
fs.writeFileSync(qaProviderStateFile, JSON.stringify(qaRuntime.providerStatus, null, 2));
} catch (err) {
// Non-fatal — qa/ dir may not be writable by the service user.
// Only swallow expected filesystem permission issues; rethrow unexpected errors
// (including JSON serialization issues) so they remain visible.
if (err && (err.code === 'EACCES' || err.code === 'EROFS' || err.code === 'EPERM')) {
// eslint-disable-next-line no-console
console.warn('Unable to persist QA provider state; qa/ directory not writable for service user.', err);
return;
}
throw err;
}
}
```
If the codebase has a centralized logger (e.g., `logger.warn` or similar), replace `console.warn` with that to be consistent with existing logging practices.
You may also want to audit the other empty `catch` blocks around `fs.mkdirSync` and apply a similar pattern (log and/or narrow to expected error codes) for better diagnosability.
</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 🔍
|
|
CodeAnt AI finished reviewing your PR. |
…e conflict Addresses code review feedback: index.js: - Added QA_FS_SUPPRESS constant (EACCES, EPERM, EROFS, EEXIST) — the set of expected error codes when qa/ is owned by root but the service runs as the runewager user. Defined once, shared by all three QA helpers. - ensureQaDirs: replace silent catch-all with per-directory loop; unexpected error codes (e.g. ENOENT, ENOMEM) are logged as warnings so they remain visible; permission codes are silently skipped. - persistQaProviderState: catch checks err.code — returns silently for permission errors, logs a warning for any other code (e.g. JSON failure). - ensureQaArtifacts: same pattern — unexpected errors are logged at warn level; permission errors (EACCES/EPERM/EROFS) are intentionally silent. telegramSafe.js: - Resolved merge conflict: kept the CALLAPI_BYPASS_METHODS module-level constants introduced in the previous commit; dropped the inline _MANAGED block that arrived from main during the merge. All 60 unit tests pass. https://claude.ai/code/session_01WpEqAiDbpqnBywMjYXJHf4
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
index.js (1)
4885-4888: Avoid fully silent QA provider-state write failures.Catching-and-ignoring here can mask persistent write problems during runtime. Consider at least a throttled
logEvent('warn', ...)so ops can detect broken QA persistence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.js` around lines 4885 - 4888, The try/catch around ensureQaDirs() and fs.writeFileSync(qaProviderStateFile, JSON.stringify(qaRuntime.providerStatus, null, 2)) swallows all errors; change the catch to capture the error (e) and emit a throttled warning via the existing log mechanism (e.g., call logEvent('warn', ... ) or a rate-limited logger) that includes the qaProviderStateFile path and error message so ops can detect persistent write failures while preserving the non-fatal behavior.telegramSafe.js (1)
329-334: Consider exporting immutable snapshots instead of liveSetreferences.Line 329 through Line 334 currently exports mutable
Setobjects by reference; external mutation can silently alter runtime bypass behavior.Optional hardening diff
module.exports = { init, sendMessage, editMessageText, answerCallbackQuery, sendPhoto, sendDocument, // Exported for testing — allows test suites to assert which methods bypass // rate-limiting without requiring a live Telegraf instance. - LONG_POLL_METHODS, - LIFECYCLE_METHODS, - INDIVIDUALLY_PATCHED_METHODS, - CALLAPI_BYPASS_METHODS, + LONG_POLL_METHODS: Object.freeze([...LONG_POLL_METHODS]), + LIFECYCLE_METHODS: Object.freeze([...LIFECYCLE_METHODS]), + INDIVIDUALLY_PATCHED_METHODS: Object.freeze([...INDIVIDUALLY_PATCHED_METHODS]), + CALLAPI_BYPASS_METHODS: Object.freeze([...CALLAPI_BYPASS_METHODS]), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@telegramSafe.js` around lines 329 - 334, The export currently exposes live mutable Set instances (LONG_POLL_METHODS, LIFECYCLE_METHODS, INDIVIDUALLY_PATCHED_METHODS, CALLAPI_BYPASS_METHODS) which allows external code to mutate runtime behavior; replace those exports with immutable snapshots by exporting frozen copies (e.g., Object.freeze([...theSet]) or a new Set created from the original and frozen) so callers receive an immutable array/collection instead of the original Set reference, and update any internal usage to continue working with the original Sets where mutation is required while keeping exported symbols read-only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@index.js`:
- Around line 4885-4888: The try/catch around ensureQaDirs() and
fs.writeFileSync(qaProviderStateFile, JSON.stringify(qaRuntime.providerStatus,
null, 2)) swallows all errors; change the catch to capture the error (e) and
emit a throttled warning via the existing log mechanism (e.g., call
logEvent('warn', ... ) or a rate-limited logger) that includes the
qaProviderStateFile path and error message so ops can detect persistent write
failures while preserving the non-fatal behavior.
In `@telegramSafe.js`:
- Around line 329-334: The export currently exposes live mutable Set instances
(LONG_POLL_METHODS, LIFECYCLE_METHODS, INDIVIDUALLY_PATCHED_METHODS,
CALLAPI_BYPASS_METHODS) which allows external code to mutate runtime behavior;
replace those exports with immutable snapshots by exporting frozen copies (e.g.,
Object.freeze([...theSet]) or a new Set created from the original and frozen) so
callers receive an immutable array/collection instead of the original Set
reference, and update any internal usage to continue working with the original
Sets where mutation is required while keeping exported symbols read-only.
External code or tests could accidentally mutate the exported Sets (LONG_POLL_METHODS, LIFECYCLE_METHODS, etc.) which would silently alter runtime bypass behavior in callApi. Export frozen array snapshots instead — callers can still iterate and use .includes() for membership checks; the internal module-level Sets remain mutable for runtime use by the callApi patch. All 60 unit tests pass. https://claude.ai/code/session_01WpEqAiDbpqnBywMjYXJHf4
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
Summary by Sourcery
Guard Telegram API rate limiting around callApi and make QA telemetry file writes non-fatal to avoid impacting core service operation.
Enhancements:
CodeAnt-AI Description
Prevent Telegram long-poll timeout and make QA file writes non-fatal
What Changed
Impact
✅ Prevents bot restart loops caused by long-poll timeouts✅ Fewer startup crashes when QA files are unwritable✅ Easier testing of Telegram rate-limit bypasses💡 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
Bug Fixes
Refactor