feat(hn-monitor): unify Slack + Telegram into single dual-transport agent#89
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
📝 WalkthroughWalkthroughThe ChangesUnified HN monitor transport flow
Sequence DiagramsequenceDiagram
actor User
participant Trigger as Telegram/Relay Trigger
participant handleQaMessage
participant memory as Durable Memory
participant LLM as LLM withTimeout
participant delivery as DeliveryClient
User->>Trigger: sends question
Trigger->>handleQaMessage: event with provider context
handleQaMessage->>memory: recall hn-monitor:post (limit 60)
memory-->>handleQaMessage: recent digests
handleQaMessage->>LLM: prompt built from digests + question
LLM-->>handleQaMessage: answer text (or fallback on timeout)
handleQaMessage->>delivery: send answer to origin-provider targets only
rect rgba(100, 180, 100, 0.5)
Note over delivery,memory: Cron posting path
delivery->>memory: save hn-monitor:seen
delivery->>delivery: publish header to all targets
delivery->>delivery: send threaded body (replyTo headerRefs)
alt all succeed
delivery->>memory: save hn-monitor:post
else partial failure
delivery->>memory: save hn-monitor:pending-thread-body
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6108a7e4c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Retain the digest so a user can DM the agent and ask about recent posts. | ||
| // ttlDays (30) on memory ages these out, giving a rolling ~30-day window. | ||
| // Thread the body under each header, also non-blocking. | ||
| const bodyResult = await delivery.send(body, { replyTo: heads, nonBlocking: true }); |
There was a problem hiding this comment.
When delivery.send rejects after the header has already been published, pending is still null, so the catch block logs hn-monitor.thread-incomplete but does not save hn-monitor:pending-thread-body; the stories remain claimed as seen and the threaded body is never retried. This affects hard delivery failures, which the comment immediately above this block explicitly expects to recover from.
Useful? React with 👍 / 👎.
| if (heads.refs.length === 0) { | ||
| throw new Error(`Header publish failed across all targets`); | ||
| } |
There was a problem hiding this comment.
When both Slack and Telegram are configured and delivery.publish(header) returns a ref for only one target, this check accepts the header as successful. The saved headerRefs then only contain the successful target, and recovery only retries the body, so the other target cannot receive a threaded digest under its missing header; the body path already treats refs.length < delivery.targets.length as a partial failure and the header needs the same handling.
Useful? React with 👍 / 👎.
| "evals:live": "npm run compile && node scripts/evals/run-evals.mjs --live --judge" | ||
| }, | ||
| "dependencies": { | ||
| "@agentworkforce/delivery": "^0.1.0", |
There was a problem hiding this comment.
This adds @agentworkforce/delivery to package.json but leaves package-lock.json unchanged (rg '@agentworkforce/delivery' package-lock.json finds nothing). The npm-ci docs state that if lockfile dependencies do not match package.json, npm ci exits with an error instead of updating the lock, so CI/deploy installs that use the checked-in lockfile will fail until the lockfile is regenerated and committed.
Useful? React with 👍 / 👎.
| @@ -1,353 +0,0 @@ | |||
| /** | |||
There was a problem hiding this comment.
Deleting this module leaves tests/telegram-agents.test.mjs:20 importing ../.test-build/hn-monitor-telegram/agent.js, while npm test runs every tests/*.test.mjs after compiling only the remaining */agent.ts files. In any environment with dependencies installed, the test suite will fail with a module-not-found error because this deleted agent is no longer emitted.
Useful? React with 👍 / 👎.
Review: PR #89 —
|
…gent Replaces the separate hn-monitor and hn-monitor-telegram directories with a single agent that delivers to Slack, Telegram, or both — configuration- driven via SLACK_CHANNEL and TELEGRAM_CHAT inputs (both optional). Uses the new @agentworkforce/delivery package for unified messaging with non-blocking parentRef threading (zero receipt round-trips, cloud-side ordering). Backports Telegram-specific improvements: - fetchWithTimeout (8s AbortController) - withTimeout for LLM calls - Pending thread body recovery on partial failure Shared helpers (input, list, withTimeout, fetchWithTimeout) now come from @agentworkforce/delivery instead of being copy-pasted.
- Fix sendToTargets cross-post leakage: now uses createDelivery(onlyTargets) - Fix threaded body permanently lost on hard throw (build pending before send) - Fix retryPendingThreadBody partial-failure inconsistency (check refs.length) - Fix orphaned pending body (clear on targets mismatch) - Add createDelivery(onlyTargets) parameter for transport-scoped delivery
- Remove @agentworkforce/delivery file: dependency from package.json (now vendored in shared/delivery/ — no external dep needed) - Remove pnpm-lock.yaml (was added by earlier commit, not tracked on main) - Export retryPendingThreadBody for testing - Add recovery-path tests: verify retry threads under original headerRefs, verifies orphaned pending body is cleared on targets mismatch
Replace vendored shared/delivery/ with npm dependency on @agentworkforce/delivery ^0.1.0 (now published).
… tests - Check heads.refs.length < targets.length for partial header publishes - Remove hn-monitor-telegram tests (superseded by hn-monitor.test.mjs) - Fix stale import in telegram-agents.test.mjs
khaliqgant
force-pushed
the
feat/unified-hn-monitor
branch
from
6f6e881 to
a618a40
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hn-monitor/agent.ts (1)
462-470: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate parsed post records before sorting.
A valid JSON value like
null,[], or a string bypasses the catch at Line 466 and can still crash Line 470 when sorting bypostedAt. Skip non-record values the same way malformed JSON is skipped.Proposed shape guard
for (const item of items) { try { - posts.push(JSON.parse(item.content) as PostRecord); + const parsed = JSON.parse(item.content); + if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { + posts.push(parsed as PostRecord); + } } catch { // skip malformed records } }🤖 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 `@hn-monitor/agent.ts` around lines 462 - 470, After successfully parsing JSON in the try block within the loop that processes items, add a validation check to ensure the parsed value is actually a valid PostRecord object before pushing it to the posts array. Currently, valid JSON values like null, empty arrays, or strings can pass the try-catch block but will cause issues when the sort operation on line 470 attempts to access the postedAt property. Add a type guard or shape validation that skips any parsed values that don't match the expected PostRecord structure, treating them the same way malformed JSON is handled in the catch block.
🧹 Nitpick comments (2)
tests/hn-monitor.test.mjs (2)
282-349: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCover retry partial-failure behavior explicitly.
Please add a test where
delivery.sendreturnsok: falseor fewer refs than targets, then assert pending state is not cleared andhn-monitor:postis not saved.Example test shape
+test('retryPendingThreadBody keeps pending when retry send fails', async () => { + // arrange pending recall + // delivery.send -> { ok: false, refs: [] } + const result = await retryPendingThreadBody(ctx, delivery); + assert.equal(result, true); + assert.equal( + saved.some((s) => s.content === 'null' && s.opts?.tags?.includes('hn-monitor:pending-thread-body')), + false + ); + assert.equal(saved.some((s) => s.opts?.tags?.includes('hn-monitor:post')), false); +});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/hn-monitor.test.mjs` around lines 282 - 349, Add a new test case after the existing retryPendingThreadBody test that covers the partial-failure scenario. In this new test, configure the delivery.send method to return ok: false or return fewer refs than expected targets, then assert that the pending state is NOT cleared (i.e., no saved call with content 'null' and tag 'hn-monitor:pending-thread-body') and the post is NOT saved (i.e., no saved call with tag 'hn-monitor:post'). Use the same context and memory setup structure as the existing test, but modify the delivery.send response to simulate a failure condition.
183-279: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd Telegram-path Q&A regression coverage.
The Q&A suite only exercises the relay branch. The Telegram branch has separate parsing/gating and should have at least one happy-path test plus one skip-path test (e.g., wrong chat or bot echo) to protect unified transport 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 `@tests/hn-monitor.test.mjs` around lines 183 - 279, Add two new regression tests for the Telegram branch of the Q&A functionality to match the existing relay coverage. First, create a happy-path test for Telegram similar to the existing relay test that calls handleQaMessage with 'telegram' as the channel parameter instead of 'relay', using a mock delivery configured for Telegram targets. Second, add a skip-path test for Telegram (such as a message from the bot itself or a message in the wrong chat) that verifies the function logs and returns early without calling memory.recall or llm.complete, using the same pattern as the existing "no text" skip-path test but with Telegram-specific conditions to trigger the early exit.
🤖 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 `@hn-monitor/agent.ts`:
- Around line 264-268: The error handling for the delivery.publish call does not
distinguish between complete failure and partial success. Currently, when
heads.refs.length is greater than zero but less than delivery.targets.length
(partial publish), the code throws an error before setting headerPosted to true,
causing the catch block to roll back the seen state and rethrow as if no refs
were published. This leads to duplicate header publishes on retry. Split the
condition to only throw an error when heads.refs.length equals zero (complete
failure), and set headerPosted to true whenever at least one header ref exists
(partial or complete success). This prevents the rollback/rethrow logic from
undoing progress on targets that already received the header. Also apply the
same fix to the equivalent code block that also appears around lines 301-305.
- Around line 357-362: The delivery.send call for pending.body is not wrapped in
exception handling, allowing transient failures to escape and terminate the cron
invocation instead of treating them as recoverable pending state. Wrap the const
bodyResult = await delivery.send(pending.body, bodyOpts) call in a try-catch
block to catch any exceptions thrown during the send operation. When an
exception is caught, log the error appropriately and return true to mark the
body as still pending for the next scheduled retry attempt, consistent with how
postFreshStories handles send failures.
- Around line 220-229: The relay DM handler currently falls back to all
configured delivery targets when Slack is not available, which violates
origin-transport isolation by potentially sending relay-origin questions to
Telegram. In the provider === 'relay' block, the targets assignment on line 223
should not spread all delivery targets as a fallback when Slack is absent.
Instead, ensure relay DMs are only delivered via Slack by keeping targets as
['slack'] regardless of what other targets are configured, or skip delivery
entirely if Slack is not available. Remove the fallback behavior
`[...delivery.targets]` and maintain strict origin-scoped routing for relay
messages.
- Around line 159-169: The code does not validate that TELEGRAM_CHAT is
configured before processing Telegram messages. When TELEGRAM_CHAT is empty or
undefined, telegramSkipReason is called with an undefined value, which fails to
properly filter wrong chat messages. Add an early return check after getting the
TELEGRAM_CHAT value with input(ctx, 'TELEGRAM_CHAT') to verify it is configured;
if it is not configured (empty or falsy), return immediately to skip the entire
Telegram Q&A processing path, ensuring the persona behavior of skipping Telegram
delivery when the config is not set.
In `@package.json`:
- Line 17: The dependency entry for `@agentworkforce/delivery` in package.json is
using a file protocol pointing to a relative sibling directory
(../workforce/packages/delivery), which breaks in CI/cloud environments and
contradicts the migration to a published package. Replace the file protocol
reference with a proper published semver version range (such as "^X.Y.Z") for
the `@agentworkforce/delivery` package, or use the workspace protocol if this
repository manages that package. Ensure the dependency resolves to the published
package rather than a local file path.
---
Outside diff comments:
In `@hn-monitor/agent.ts`:
- Around line 462-470: After successfully parsing JSON in the try block within
the loop that processes items, add a validation check to ensure the parsed value
is actually a valid PostRecord object before pushing it to the posts array.
Currently, valid JSON values like null, empty arrays, or strings can pass the
try-catch block but will cause issues when the sort operation on line 470
attempts to access the postedAt property. Add a type guard or shape validation
that skips any parsed values that don't match the expected PostRecord structure,
treating them the same way malformed JSON is handled in the catch block.
---
Nitpick comments:
In `@tests/hn-monitor.test.mjs`:
- Around line 282-349: Add a new test case after the existing
retryPendingThreadBody test that covers the partial-failure scenario. In this
new test, configure the delivery.send method to return ok: false or return fewer
refs than expected targets, then assert that the pending state is NOT cleared
(i.e., no saved call with content 'null' and tag
'hn-monitor:pending-thread-body') and the post is NOT saved (i.e., no saved call
with tag 'hn-monitor:post'). Use the same context and memory setup structure as
the existing test, but modify the delivery.send response to simulate a failure
condition.
- Around line 183-279: Add two new regression tests for the Telegram branch of
the Q&A functionality to match the existing relay coverage. First, create a
happy-path test for Telegram similar to the existing relay test that calls
handleQaMessage with 'telegram' as the channel parameter instead of 'relay',
using a mock delivery configured for Telegram targets. Second, add a skip-path
test for Telegram (such as a message from the bot itself or a message in the
wrong chat) that verifies the function logs and returns early without calling
memory.recall or llm.complete, using the same pattern as the existing "no text"
skip-path test but with Telegram-specific conditions to trigger the early exit.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 96a0c56e-9818-43eb-b1ac-703d6b1b99bc
⛔ Files ignored due to path filters (3)
hn-monitor-telegram/avatar.pngis excluded by!**/*.pnghn-monitor-telegram/banner.pngis excluded by!**/*.pnghn-monitor-telegram/card.pngis excluded by!**/*.png
📒 Files selected for processing (8)
hn-monitor-telegram/README.mdhn-monitor-telegram/agent.tshn-monitor-telegram/persona.tshn-monitor/agent.tshn-monitor/persona.tspackage.jsontests/hn-monitor.test.mjstests/telegram-agents.test.mjs
💤 Files with no reviewable changes (4)
- hn-monitor-telegram/README.md
- hn-monitor-telegram/agent.ts
- hn-monitor-telegram/persona.ts
- tests/telegram-agents.test.mjs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hn-monitor/agent.ts (1)
462-470: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate parsed post records before sorting.
A valid JSON value like
null,[], or a string bypasses the catch at Line 466 and can still crash Line 470 when sorting bypostedAt. Skip non-record values the same way malformed JSON is skipped.Proposed shape guard
for (const item of items) { try { - posts.push(JSON.parse(item.content) as PostRecord); + const parsed = JSON.parse(item.content); + if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { + posts.push(parsed as PostRecord); + } } catch { // skip malformed records } }🤖 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 `@hn-monitor/agent.ts` around lines 462 - 470, After successfully parsing JSON in the try block within the loop that processes items, add a validation check to ensure the parsed value is actually a valid PostRecord object before pushing it to the posts array. Currently, valid JSON values like null, empty arrays, or strings can pass the try-catch block but will cause issues when the sort operation on line 470 attempts to access the postedAt property. Add a type guard or shape validation that skips any parsed values that don't match the expected PostRecord structure, treating them the same way malformed JSON is handled in the catch block.
🧹 Nitpick comments (2)
tests/hn-monitor.test.mjs (2)
282-349: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCover retry partial-failure behavior explicitly.
Please add a test where
delivery.sendreturnsok: falseor fewer refs than targets, then assert pending state is not cleared andhn-monitor:postis not saved.Example test shape
+test('retryPendingThreadBody keeps pending when retry send fails', async () => { + // arrange pending recall + // delivery.send -> { ok: false, refs: [] } + const result = await retryPendingThreadBody(ctx, delivery); + assert.equal(result, true); + assert.equal( + saved.some((s) => s.content === 'null' && s.opts?.tags?.includes('hn-monitor:pending-thread-body')), + false + ); + assert.equal(saved.some((s) => s.opts?.tags?.includes('hn-monitor:post')), false); +});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/hn-monitor.test.mjs` around lines 282 - 349, Add a new test case after the existing retryPendingThreadBody test that covers the partial-failure scenario. In this new test, configure the delivery.send method to return ok: false or return fewer refs than expected targets, then assert that the pending state is NOT cleared (i.e., no saved call with content 'null' and tag 'hn-monitor:pending-thread-body') and the post is NOT saved (i.e., no saved call with tag 'hn-monitor:post'). Use the same context and memory setup structure as the existing test, but modify the delivery.send response to simulate a failure condition.
183-279: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd Telegram-path Q&A regression coverage.
The Q&A suite only exercises the relay branch. The Telegram branch has separate parsing/gating and should have at least one happy-path test plus one skip-path test (e.g., wrong chat or bot echo) to protect unified transport 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 `@tests/hn-monitor.test.mjs` around lines 183 - 279, Add two new regression tests for the Telegram branch of the Q&A functionality to match the existing relay coverage. First, create a happy-path test for Telegram similar to the existing relay test that calls handleQaMessage with 'telegram' as the channel parameter instead of 'relay', using a mock delivery configured for Telegram targets. Second, add a skip-path test for Telegram (such as a message from the bot itself or a message in the wrong chat) that verifies the function logs and returns early without calling memory.recall or llm.complete, using the same pattern as the existing "no text" skip-path test but with Telegram-specific conditions to trigger the early exit.
🤖 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 `@hn-monitor/agent.ts`:
- Around line 264-268: The error handling for the delivery.publish call does not
distinguish between complete failure and partial success. Currently, when
heads.refs.length is greater than zero but less than delivery.targets.length
(partial publish), the code throws an error before setting headerPosted to true,
causing the catch block to roll back the seen state and rethrow as if no refs
were published. This leads to duplicate header publishes on retry. Split the
condition to only throw an error when heads.refs.length equals zero (complete
failure), and set headerPosted to true whenever at least one header ref exists
(partial or complete success). This prevents the rollback/rethrow logic from
undoing progress on targets that already received the header. Also apply the
same fix to the equivalent code block that also appears around lines 301-305.
- Around line 357-362: The delivery.send call for pending.body is not wrapped in
exception handling, allowing transient failures to escape and terminate the cron
invocation instead of treating them as recoverable pending state. Wrap the const
bodyResult = await delivery.send(pending.body, bodyOpts) call in a try-catch
block to catch any exceptions thrown during the send operation. When an
exception is caught, log the error appropriately and return true to mark the
body as still pending for the next scheduled retry attempt, consistent with how
postFreshStories handles send failures.
- Around line 220-229: The relay DM handler currently falls back to all
configured delivery targets when Slack is not available, which violates
origin-transport isolation by potentially sending relay-origin questions to
Telegram. In the provider === 'relay' block, the targets assignment on line 223
should not spread all delivery targets as a fallback when Slack is absent.
Instead, ensure relay DMs are only delivered via Slack by keeping targets as
['slack'] regardless of what other targets are configured, or skip delivery
entirely if Slack is not available. Remove the fallback behavior
`[...delivery.targets]` and maintain strict origin-scoped routing for relay
messages.
- Around line 159-169: The code does not validate that TELEGRAM_CHAT is
configured before processing Telegram messages. When TELEGRAM_CHAT is empty or
undefined, telegramSkipReason is called with an undefined value, which fails to
properly filter wrong chat messages. Add an early return check after getting the
TELEGRAM_CHAT value with input(ctx, 'TELEGRAM_CHAT') to verify it is configured;
if it is not configured (empty or falsy), return immediately to skip the entire
Telegram Q&A processing path, ensuring the persona behavior of skipping Telegram
delivery when the config is not set.
In `@package.json`:
- Line 17: The dependency entry for `@agentworkforce/delivery` in package.json is
using a file protocol pointing to a relative sibling directory
(../workforce/packages/delivery), which breaks in CI/cloud environments and
contradicts the migration to a published package. Replace the file protocol
reference with a proper published semver version range (such as "^X.Y.Z") for
the `@agentworkforce/delivery` package, or use the workspace protocol if this
repository manages that package. Ensure the dependency resolves to the published
package rather than a local file path.
---
Outside diff comments:
In `@hn-monitor/agent.ts`:
- Around line 462-470: After successfully parsing JSON in the try block within
the loop that processes items, add a validation check to ensure the parsed value
is actually a valid PostRecord object before pushing it to the posts array.
Currently, valid JSON values like null, empty arrays, or strings can pass the
try-catch block but will cause issues when the sort operation on line 470
attempts to access the postedAt property. Add a type guard or shape validation
that skips any parsed values that don't match the expected PostRecord structure,
treating them the same way malformed JSON is handled in the catch block.
---
Nitpick comments:
In `@tests/hn-monitor.test.mjs`:
- Around line 282-349: Add a new test case after the existing
retryPendingThreadBody test that covers the partial-failure scenario. In this
new test, configure the delivery.send method to return ok: false or return fewer
refs than expected targets, then assert that the pending state is NOT cleared
(i.e., no saved call with content 'null' and tag
'hn-monitor:pending-thread-body') and the post is NOT saved (i.e., no saved call
with tag 'hn-monitor:post'). Use the same context and memory setup structure as
the existing test, but modify the delivery.send response to simulate a failure
condition.
- Around line 183-279: Add two new regression tests for the Telegram branch of
the Q&A functionality to match the existing relay coverage. First, create a
happy-path test for Telegram similar to the existing relay test that calls
handleQaMessage with 'telegram' as the channel parameter instead of 'relay',
using a mock delivery configured for Telegram targets. Second, add a skip-path
test for Telegram (such as a message from the bot itself or a message in the
wrong chat) that verifies the function logs and returns early without calling
memory.recall or llm.complete, using the same pattern as the existing "no text"
skip-path test but with Telegram-specific conditions to trigger the early exit.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 96a0c56e-9818-43eb-b1ac-703d6b1b99bc
⛔ Files ignored due to path filters (3)
hn-monitor-telegram/avatar.pngis excluded by!**/*.pnghn-monitor-telegram/banner.pngis excluded by!**/*.pnghn-monitor-telegram/card.pngis excluded by!**/*.png
📒 Files selected for processing (8)
hn-monitor-telegram/README.mdhn-monitor-telegram/agent.tshn-monitor-telegram/persona.tshn-monitor/agent.tshn-monitor/persona.tspackage.jsontests/hn-monitor.test.mjstests/telegram-agents.test.mjs
💤 Files with no reviewable changes (4)
- hn-monitor-telegram/README.md
- hn-monitor-telegram/agent.ts
- hn-monitor-telegram/persona.ts
- tests/telegram-agents.test.mjs
🛑 Comments failed to post (5)
hn-monitor/agent.ts (4)
159-169: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Skip Telegram Q&A when
TELEGRAM_CHATis not configured.With
TELEGRAM_CHATempty,telegramSkipReason(msg, undefined)does not reject wrong chats, so any non-bot Telegram message can be loaded into the LLM path even though the persona says empty Telegram config skips Telegram delivery.Proposed gating fix
const msg = readTelegramMessage(payload.data); if (!msg) return; + const telegramChat = input(ctx, 'TELEGRAM_CHAT'); + if (!telegramChat) { + ctx.log('info', 'hn-monitor.qa.skip', { reason: 'telegram not configured' }); + return; + } // Gate: skip bot echoes, wrong chat, empty text - const reason = telegramSkipReason(msg, input(ctx, 'TELEGRAM_CHAT')); + const reason = telegramSkipReason(msg, telegramChat);📝 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 (provider === 'telegram') { const payload = expanded as { data?: unknown }; if (!payload.data) return; const msg = readTelegramMessage(payload.data); if (!msg) return; const telegramChat = input(ctx, 'TELEGRAM_CHAT'); if (!telegramChat) { ctx.log('info', 'hn-monitor.qa.skip', { reason: 'telegram not configured' }); return; } // Gate: skip bot echoes, wrong chat, empty text const reason = telegramSkipReason(msg, telegramChat); if (reason) { ctx.log('info', `hn-monitor.qa.skip reason=${reason.replace(/\s+/g, '-')}`); 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 `@hn-monitor/agent.ts` around lines 159 - 169, The code does not validate that TELEGRAM_CHAT is configured before processing Telegram messages. When TELEGRAM_CHAT is empty or undefined, telegramSkipReason is called with an undefined value, which fails to properly filter wrong chat messages. Add an early return check after getting the TELEGRAM_CHAT value with input(ctx, 'TELEGRAM_CHAT') to verify it is configured; if it is not configured (empty or falsy), return immediately to skip the entire Telegram Q&A processing path, ensuring the persona behavior of skipping Telegram delivery when the config is not set.
220-229: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not fall back from relay DMs to Telegram.
Line 223 sends relay-origin Q&A replies to every configured target when Slack is absent, so a private relay inbox question can be answered in Telegram. That breaks the origin-transport isolation called out by this PR.
Proposed origin-scoped fix
if (provider === 'relay') { // Relay DMs: reply to Slack if configured (legacy behavior). - // If only Telegram is configured, reply there instead. - const targets: Array<'slack' | 'telegram'> = delivery.targets.includes('slack') ? ['slack'] : [...delivery.targets]; + // Do not mirror relay-origin questions into Telegram. + if (!delivery.targets.includes('slack')) { + ctx.log('warn', 'hn-monitor.qa.no-origin-target', { provider: 'relay' }); + return; + } + const targets: Array<'slack'> = ['slack'];🤖 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 `@hn-monitor/agent.ts` around lines 220 - 229, The relay DM handler currently falls back to all configured delivery targets when Slack is not available, which violates origin-transport isolation by potentially sending relay-origin questions to Telegram. In the provider === 'relay' block, the targets assignment on line 223 should not spread all delivery targets as a fallback when Slack is absent. Instead, ensure relay DMs are only delivered via Slack by keeping targets as ['slack'] regardless of what other targets are configured, or skip delivery entirely if Slack is not available. Remove the fallback behavior `[...delivery.targets]` and maintain strict origin-scoped routing for relay messages.
264-268: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Handle partial header publishes as landed state.
Line 265 throws on a partial header publish (
refs.length > 0 && refs.length < targets.length) beforeheaderPostedis set. The catch block then restoresseenand rethrows as if nothing landed, so a runtime retry can duplicate headers on targets that already returned refs. Split zero-ref failure from partial-ref failure, and avoid the rollback/rethrow path once any header ref exists.Also applies to: 301-305
🤖 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 `@hn-monitor/agent.ts` around lines 264 - 268, The error handling for the delivery.publish call does not distinguish between complete failure and partial success. Currently, when heads.refs.length is greater than zero but less than delivery.targets.length (partial publish), the code throws an error before setting headerPosted to true, causing the catch block to roll back the seen state and rethrow as if no refs were published. This leads to duplicate header publishes on retry. Split the condition to only throw an error when heads.refs.length equals zero (complete failure), and set headerPosted to true whenever at least one header ref exists (partial or complete success). This prevents the rollback/rethrow logic from undoing progress on targets that already received the header. Also apply the same fix to the equivalent code block that also appears around lines 301-305.
357-362: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Catch pending body retry send exceptions.
postFreshStoriestreats throwndelivery.send()calls as recoverable pending state, but the retry path lets the same exception escape. A transient send failure can fail the whole cron invocation and trigger runtime retries instead of leaving the pending body for the next scheduled attempt.Proposed retry guard
- const bodyResult = await delivery.send(pending.body, bodyOpts); + let bodyResult: DeliveryResult; + try { + bodyResult = await delivery.send(pending.body, bodyOpts); + } catch (error) { + ctx.log('error', 'hn-monitor.pending-body-retry-failed', { + targets: configuredTargets, + error: String(error) + }); + return true; + } // Match postFreshStories: ALL targets must receive refs for success.📝 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.let bodyResult: DeliveryResult; try { bodyResult = await delivery.send(pending.body, bodyOpts); } catch (error) { ctx.log('error', 'hn-monitor.pending-body-retry-failed', { targets: configuredTargets, error: String(error) }); return true; } // Match postFreshStories: ALL targets must receive refs for success. if (!bodyResult.ok || bodyResult.refs.length < delivery.targets.length) { ctx.log('error', 'hn-monitor.pending-body-retry-failed', { targets: configuredTargets }); return true; }🤖 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 `@hn-monitor/agent.ts` around lines 357 - 362, The delivery.send call for pending.body is not wrapped in exception handling, allowing transient failures to escape and terminate the cron invocation instead of treating them as recoverable pending state. Wrap the const bodyResult = await delivery.send(pending.body, bodyOpts) call in a try-catch block to catch any exceptions thrown during the send operation. When an exception is caught, log the error appropriately and return true to mark the body as still pending for the next scheduled retry attempt, consistent with how postFreshStories handles send failures.package.json (1)
17-17: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify the delivery dependency is not pinned to a sibling checkout. node - <<'NODE' const fs = require('fs'); const pkg = JSON.parse(fs.readFileSync('package.json', 'utf8')); const dep = pkg.dependencies?.['`@agentworkforce/delivery`'] ?? pkg.devDependencies?.['`@agentworkforce/delivery`']; console.log(`@agentworkforce/delivery=${dep ?? '<missing>'}`); if (typeof dep === 'string' && dep.startsWith('file:../')) { process.exitCode = 1; } NODERepository: AgentWorkforce/agents
Length of output: 220
Use the published delivery package instead of a sibling
file:link.Line 17 points outside this repository (
../workforce/packages/delivery), which causes dependency resolution to fail in CI/cloud environments without the adjacent../workforcecheckout. This also contradicts the PR objective of migrating to the published package. Use the published semver range, or a workspace protocol if this repository owns that package.Proposed fix
- "`@agentworkforce/delivery`": "file:../workforce/packages/delivery", + "`@agentworkforce/delivery`": "^0.1.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."`@agentworkforce/delivery`": "^0.1.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 `@package.json` at line 17, The dependency entry for `@agentworkforce/delivery` in package.json is using a file protocol pointing to a relative sibling directory (../workforce/packages/delivery), which breaks in CI/cloud environments and contradicts the migration to a published package. Replace the file protocol reference with a proper published semver version range (such as "^X.Y.Z") for the `@agentworkforce/delivery` package, or use the workspace protocol if this repository manages that package. Ensure the dependency resolves to the published package rather than a local file path.
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. Working tree is clean. No source edits made (no mechanical-only fix was safe/appropriate). Here is my review. Review: PR #89 —
|
1 participant
Summary
Replaces the separate
hn-monitorandhn-monitor-telegramagents with a single dual-transport agent. Delivers to Slack, Telegram, or both — configuration-driven viaSLACK_CHANNELandTELEGRAM_CHATinputs (both optional).Depends on workforce#250 for
@agentworkforce/delivery(now published as^0.1.0).Changes
createDeliveryfrom@agentworkforce/delivery, auto-detects configured transportsfetchWithTimeout(8s AbortController),withTimeoutfor LLM, pending thread body recovery withheaderRefsfor proper retry threadinginput,list,withTimeout,fetchWithTimeout) from@agentworkforce/deliveryslackandtelegramintegrations, both inputsoptional: truehn-monitor-telegram/directory (now superseded)Summary by cubic
Unifies the Slack and Telegram HN monitors into one agent that posts to Slack, Telegram, or both via
@agentworkforce/delivery. Adds non-blocking threading with robust recovery and prevents cross-post or partial-publish failures.New Features
SLACK_CHANNELandTELEGRAM_CHAT(optional) with unified, non-blocking parentRef threading; addsfetchWithTimeout(8s) andwithTimeoutto prevent hangs.headerRefsand retries threaded bodies on the next scan; unified Q&A for relay DMs and Telegram, replying only on the origin transport.Bug Fixes
hn-monitor-telegram/and superseded Telegram tests; updates imports; uses published@agentworkforce/delivery@^0.1.0(matchesmain).Written for commit 2784722. Summary will update on new commits.