Skip to content

feat(hn-monitor): unify Slack + Telegram into single dual-transport agent#88

Merged
khaliqgant merged 4 commits into
mainfrom
feat/unified-hn-monitor
Jun 23, 2026
Merged

feat(hn-monitor): unify Slack + Telegram into single dual-transport agent#88
khaliqgant merged 4 commits into
mainfrom
feat/unified-hn-monitor

Conversation

@khaliqgant

@khaliqgant khaliqgant commented Jun 23, 2026

Copy link
Copy Markdown
Member

Summary

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).

Depends on workforce#250 for the @agentworkforce/delivery package.

Changes

  • Single agent.ts — imports createDelivery from @agentworkforce/delivery, auto-detects configured transports
  • Non-blocking parentRef threading — header published with 0ms timeout, body threaded via parentRef (x-reply-radar pattern)
  • Backported Telegram improvements: fetchWithTimeout (8s AbortController), withTimeout for LLM, pending thread body recovery
  • Shared helpers (input, list, withTimeout, fetchWithTimeout) now imported from @agentworkforce/delivery
  • Persona declares both slack and telegram integrations, both inputs optional: true

What this enables

Once @agentworkforce/delivery is published, the same unification pattern can be applied to the remaining 3 paired agents (inbox-buddy, joke-bot, spotify-releases).


Summary by cubic

Unifies hn-monitor into one agent that sends Hacker News digests to Slack, Telegram, or both using the published @agentworkforce/delivery. Adds non‑blocking header publish, transport‑scoped Q&A (relay and Telegram), and reliable recovery for threaded posts.

  • New Features

    • Single dual-transport agent.ts using createDelivery from @agentworkforce/delivery; auto-detects SLACK_CHANNEL/TELEGRAM_CHAT.
    • Posts digests to Slack and/or Telegram; Q&A via Telegram and relay; replies scoped to the origin transport.
    • Non-blocking header publish with threaded body via parentRef/replyTo; bounded timeouts for HN fetch (8s) and LLM.
  • Bug Fixes

    • Scoped Q&A replies to the origin using createDelivery(onlyTargets) to prevent cross‑post leakage.
    • Built and persisted pending thread bodies before send; retry now checks all target refs and clears stale records if targets change. Added recovery tests and exported retryPendingThreadBody.

Written for commit 05d6135. Summary will update on new commits.

Review in cubic

…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.
@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The HN monitor now uses a transport-agnostic delivery abstraction to send digests and answers to multiple Slack and/or Telegram targets simultaneously. Q&A is handled via provider-specific message parsing (Telegram events and relay inbox DMs), with answers routed only to the originating transport. Fresh story posting uses non-blocking delivery to thread digests under published headers, and stores pending thread bodies in durable state when sends partially fail, enabling retry on subsequent cron ticks. Digest generation is wrapped with timeout-backed LLM calls that fall back to plain story titles on error. Tests now cover success, fallback, header-failure rollback, partial-failure recovery, and Q&A routing across both posting and Q&A flows.

Changes

HN monitor multi-target delivery

Layer / File(s) Summary
Shared delivery infrastructure
shared/delivery/types.ts, shared/delivery/helpers.ts, shared/delivery/delivery.ts, shared/delivery/index.ts
New transport-agnostic delivery module supporting Slack and Telegram concurrently, blocking and non-blocking send modes, reply threading via ref matching, timeout helpers for fetch and LLM calls, and configuration resolution from persona inputs or environment variables.
Agent configuration and persona updates
hn-monitor/persona.ts, hn-monitor/agent.ts, package.json
Persona integrations expanded to include Telegram scope, inputs made optional for both SLACK_CHANNEL and new TELEGRAM_CHAT, system prompt generalized from Slack-specific to short-digest format, @agentworkforce/delivery added as local file dependency, agent imports delivery factories and utilities.
Q&A message handling
hn-monitor/agent.ts
New handleQaMessage function parses Telegram and relay inbox Q&A messages, recalls stored post digests, builds LLM prompts grounded in context and user questions, routes answers back only to the originating transport (Slack for relay origin, Telegram for Telegram origin) to avoid mirroring.
Story posting, storage, and recovery
hn-monitor/agent.ts
postFreshStories refactored to use DeliveryClient for multi-target posting, claims story IDs as seen before publish to prevent duplicate cron re-posts, persists post digests on success, saves PendingThreadBody records on partial thread-send failure for retry on next cron tick, summarize wrapped with withTimeout and fallback to plain story titles, loadPosts tolerates malformed JSON and sorts by recency, durable-memory recovery helpers serialize and retry failed pending threads when targets match.
Test coverage
tests/hn-monitor.test.mjs
Test harness updated with fakeCtx() tracking memory/LLM calls and fakeDelivery() modeling blocking/non-blocking publish-and-thread semantics, postFreshStories tests cover success with persisted digest, LLM fallback, header-publish failure with rollback, and partial thread-send failure with recovery record persistence, handleQaMessage tests verify digest recall, LLM prompt composition, and delivery-based answer routing, plus no-op behavior on empty question input.

Sequence Diagram(s)

sequenceDiagram
  participant Cron
  participant Agent
  participant Memory
  participant LLM
  participant Delivery
  
  Cron->>Agent: scheduled scan
  Agent->>Memory: load pending thread body
  Agent->>Delivery: retry pending body if present
  Agent->>Memory: claim seen IDs before publish
  Agent->>LLM: summarize fresh stories with timeout
  LLM-->>Agent: header and body or fallback
  Agent->>Delivery: publish header
  Delivery-->>Agent: header ref
  Agent->>Delivery: send body replyTo header (nonBlocking)
  alt all targets succeed
    Agent->>Memory: persist post digest
  else partial failure
    Agent->>Memory: save pending thread body record
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~55 minutes

Possibly related PRs

  • AgentWorkforce/agents#51: Both PRs address the same cron double-post issue in hn-monitor by reordering story-claim persistence before sending the digest, with this PR further generalizing via DeliveryClient and adding pending-thread recovery.

Poem

🐇 A digest springs forth in two quick hops,
To Slack and Telegram the story pops.
If one hop fails mid-thread I weave,
I tuck the scraps in memory's sleeve,
And finish the journey next cycle's night! 🌙

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% 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
Title check ✅ Passed The title clearly and concisely summarizes the main objective: unifying Slack and Telegram into a single dual-transport agent, which matches the primary change in the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description clearly explains the consolidation of two separate agents into one unified agent with dual-transport support, specific configuration changes, and technical improvements including non-blocking threading and timeout handling.

✏️ 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 feat/unified-hn-monitor

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the Hacker News monitor to support multi-platform delivery (Slack, Telegram, and Relay) using the @agentworkforce/delivery package, and introduces a pending thread body recovery mechanism to handle partial delivery failures. Feedback on the changes highlights a bug in parseRelayMessage where accessing payload.data is redundant and breaks Relay Q&A, and notes that routing for Relay inbox DMs is missing from the main handler. Additionally, it is recommended to store and pass the replyTo references in the pending thread body state to ensure retried bodies are correctly threaded under their original headers.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread hn-monitor/agent.ts Outdated
Comment on lines +82 to +88
function parseRelayMessage(payload: unknown): ParsedMessage | null {
const data = asRecord((payload as { data?: Record<string, unknown> })?.data);
const nested = (data?.message && typeof data.message === 'object' ? data.message : {}) as Record<string, unknown>;
const text = str(data?.text) ?? str(nested.text) ?? '';
if (!text.trim()) return null;
return { text: text.trim(), provider: 'relay' };
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

In handleQaMessage, payload.data is passed directly to parseRelayMessage. Therefore, the payload parameter in parseRelayMessage is already the inner data record, not the outer event payload. Accessing payload.data again results in undefined, which completely breaks the Relay Q&A path. We should parse payload directly as the record.

Suggested change
function parseRelayMessage(payload: unknown): ParsedMessage | null {
const data = asRecord((payload as { data?: Record<string, unknown> })?.data);
const nested = (data?.message && typeof data.message === 'object' ? data.message : {}) as Record<string, unknown>;
const text = str(data?.text) ?? str(nested.text) ?? '';
if (!text.trim()) return null;
return { text: text.trim(), provider: 'relay' };
}
function parseRelayMessage(payload: unknown): ParsedMessage | null {
const data = asRecord(payload);
if (!data) return null;
const nested = asRecord(data.message) ?? {};
const text = str(data.text) ?? str(nested.text) ?? '';
if (!text.trim()) return null;
return { text: text.trim(), provider: 'relay' };
}

Comment thread hn-monitor/agent.ts
Comment on lines +112 to +118
// Q&A path: telegram message
if (typeof event.type === 'string' && event.type.startsWith('telegram.')) {
await handleQaMessage(ctx, event as unknown as AgentEvent, 'telegram');
return;
}
// Cron path
if (!isCronTickEvent(event as unknown as AgentEvent)) return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The routing for Relay inbox DM messages (relaycast.message) is missing from the handler, meaning the agent will silently ignore any incoming Relay DMs. We should route relaycast.message events to handleQaMessage with the 'relay' provider.

    // Q&A path: telegram message or relay DM
    if (typeof event.type === 'string' && event.type.startsWith('telegram.')) {
      await handleQaMessage(ctx, event as unknown as AgentEvent, 'telegram');
      return;
    }
    if (event.type === 'relaycast.message') {
      await handleQaMessage(ctx, event as unknown as AgentEvent, 'relay');
      return;
    }
    // Cron path
    if (!isCronTickEvent(event as unknown as AgentEvent)) return;

Comment thread hn-monitor/agent.ts
Comment on lines +47 to +53
interface PendingThreadBody {
targets: string;
header: string;
body: string;
createdAt: string;
stories: Array<{ title: string; url: string; points: number }>;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To ensure that the retried body is correctly threaded under the original header on recovery, we should store the header's delivery references (replyTo) in the PendingThreadBody state.

Suggested change
interface PendingThreadBody {
targets: string;
header: string;
body: string;
createdAt: string;
stories: Array<{ title: string; url: string; points: number }>;
}
interface PendingThreadBody {
targets: string;
header: string;
body: string;
createdAt: string;
stories: Array<{ title: string; url: string; points: number }>;
replyTo?: any;
}

Comment thread hn-monitor/agent.ts Outdated
Comment on lines +246 to +252
pending = {
targets: delivery.targets.join(','),
header,
body,
createdAt: new Date().toISOString(),
stories: fresh.map((s) => ({ title: s.title, url: s.url, points: s.points }))
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Include the heads delivery result as replyTo in the saved pending state so that the retry attempt can thread the body correctly.

Suggested change
pending = {
targets: delivery.targets.join(','),
header,
body,
createdAt: new Date().toISOString(),
stories: fresh.map((s) => ({ title: s.title, url: s.url, points: s.points }))
};
pending = {
targets: delivery.targets.join(','),
header,
body,
createdAt: new Date().toISOString(),
stories: fresh.map((s) => ({ title: s.title, url: s.url, points: s.points })),
replyTo: heads
};

Comment thread hn-monitor/agent.ts Outdated
const configuredTargets = delivery.targets.join(',');
if (pending.targets !== configuredTargets) return false;

const bodyResult = await delivery.send(pending.body, { nonBlocking: true });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Pass the stored replyTo reference to delivery.send so that the retried body is threaded under the original header instead of being posted as a standalone message.

Suggested change
const bodyResult = await delivery.send(pending.body, { nonBlocking: true });
const bodyResult = await delivery.send(pending.body, { replyTo: pending.replyTo, nonBlocking: true });

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 48da94e8b6

ℹ️ 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".

Comment thread package.json Outdated
"evals:live": "npm run compile && node scripts/evals/run-evals.mjs --live --judge"
},
"dependencies": {
"@agentworkforce/delivery": "file:../workforce/packages/delivery",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Depend on a resolvable delivery package

This adds a dependency on a sibling directory outside the repository. In a fresh checkout of this agents repo, ../workforce/packages/delivery is not present, so installs/CI/deploys cannot resolve @agentworkforce/delivery and hn-monitor/agent.ts cannot compile. Please depend on a published package version or include the package in this workspace instead of a local file path.

Useful? React with 👍 / 👎.

Comment thread hn-monitor/agent.ts
return;
}
// Cron path
if (!isCronTickEvent(event as unknown as AgentEvent)) return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Route relay inbox messages before dropping non-cron events

When this persona receives a relay inbox DM, the event is a relaycast.message while hn-monitor/persona.ts still declares relay: { inbox: ['@self'] }. The new handler only routes telegram.* events and then silently returns for every non-cron event here, so the existing DM Q&A path advertised by the persona stops working, especially for Slack-only deployments. Please keep a relay inbox branch before this guard.

Useful? React with 👍 / 👎.

Comment thread hn-monitor/agent.ts Outdated
// Deliver answer to all configured targets (non-blocking for Q&A replies)
const delivery = createDelivery(ctx);
if (delivery.targets.length > 0) {
await delivery.publish(answer.trim() || 'No answer available.');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reply only to the Telegram chat that asked

For a Telegram message from any chat in the mounted /telegram/chats/** scope, this publishes the generated answer to all configured delivery targets instead of verifying the message came from TELEGRAM_CHAT and replying to that source message. Because parseTelegramMessage drops the chat/message id, a question in an unrelated Telegram chat can be copied into the configured Slack/Telegram digest destinations. Please parse the chat id/message id, apply the TELEGRAM_CHAT guard, and reply to that message rather than broadcasting.

Useful? React with 👍 / 👎.

@agent-relay-code

Copy link
Copy Markdown
Contributor

pr-reviewer could not complete review for #88 in AgentWorkforce/agents.
The review harness exited with code 1.
No review was posted; this needs operator attention.

agent-relay-code Bot added a commit that referenced this pull request Jun 23, 2026
@agent-relay-code

Copy link
Copy Markdown
Contributor

pr-reviewer could not complete review for #88 in AgentWorkforce/agents.
The review harness exited with code 1.
No review was posted; this needs operator attention.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (1)
tests/hn-monitor.test.mjs (1)

139-173: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Add a recovery-path test that asserts threaded retry uses preserved replyTo.

Current suite stops at pending-state persistence. Please add a follow-up test for retry behavior (same targets) that verifies the body is re-sent under the original header and that pending state is cleared only after successful retry.

🤖 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 139 - 173, The current tests cover
only saving pending state after a partial failure, but they do not verify the
recovery path for a retry. Add a follow-up test around postFreshStories using
the same delivery target flow to assert that the threaded body resend preserves
the original replyTo from the published header, and that the pending-thread-body
state is removed only after the retry succeeds. Use the existing fakeCtx,
delivery.publish, and delivery.send patterns to verify the retry behavior end to
end.
🤖 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 108-119: The Agent handler in agent.ts only routes Q&A traffic
when event.type starts with telegram., so relay/slack Q&A events never reach
handleQaMessage or the later parsing branches. Update the dispatcher in handler
to recognize the relay/slack event types alongside telegram before the cron
check, and make sure the existing Q&A parsing path in handleQaMessage is invoked
for those events rather than being dropped as non-cron noise.
- Around line 202-206: The Q&A reply path in createDelivery currently publishes
to every configured target, which causes in-origin questions to be mirrored
elsewhere. Update the answer delivery flow so the publish call only sends back
to the originating transport/context for the current request, and keep the
non-blocking behavior for replies; use createDelivery and delivery.publish as
the main places to adjust the routing logic.
- Around line 247-248: The pending-vs-configured target comparison is
order-sensitive because `delivery.targets.join(',')` preserves array order,
which can cause false retry mismatches. Update the target key generation in the
relevant `agent.ts` flow (where `targets` and `header` are assembled, and the
same logic is reused later) to canonicalize the targets before storing or
comparing them, such as by sorting or otherwise normalizing the collection
first. Ensure both the pending and configured target keys use the same
normalized representation so `slack,telegram` and `telegram,slack` compare
equal.
- Around line 47-53: The PendingThreadBody interface is missing the thread
linkage references needed for recovery. Add `heads` and `replyTo` fields to the
PendingThreadBody interface to preserve the thread context when pending body
state is persisted after header success. Then update the pending body state
creation (in the sections around lines 243-253 and 292-303) to include these
thread references when saving, and ensure that when delivery.send(...) is called
during retry, it receives the replyTo information from the recovered pending
body so the body can be properly attached to the original header message.

---

Nitpick comments:
In `@tests/hn-monitor.test.mjs`:
- Around line 139-173: The current tests cover only saving pending state after a
partial failure, but they do not verify the recovery path for a retry. Add a
follow-up test around postFreshStories using the same delivery target flow to
assert that the threaded body resend preserves the original replyTo from the
published header, and that the pending-thread-body state is removed only after
the retry succeeds. Use the existing fakeCtx, delivery.publish, and
delivery.send patterns to verify the retry behavior end to end.
🪄 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: 6c7e6275-27be-4797-ad94-87fad5ad4958

📥 Commits

Reviewing files that changed from the base of the PR and between aedc47b and f23b1d4.

⛔ Files ignored due to path filters (2)
  • package-lock.json is excluded by !**/package-lock.json
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (4)
  • hn-monitor/agent.ts
  • hn-monitor/persona.ts
  • package.json
  • tests/hn-monitor.test.mjs

Comment thread hn-monitor/agent.ts
Comment thread hn-monitor/agent.ts
Comment on lines +108 to +119
triggers: {
telegram: [{ on: 'message' }]
},
handler: async (ctx, event) => {
// Chat path: a relay DM arrived — answer questions about what we've posted.
if (isRelaycastMessageEvent(event)) {
await handleInboxMessage(ctx, event);
// Q&A path: telegram message
if (typeof event.type === 'string' && event.type.startsWith('telegram.')) {
await handleQaMessage(ctx, event as unknown as AgentEvent, 'telegram');
return;
}
// Cron path
if (!isCronTickEvent(event as unknown as AgentEvent)) return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Relay/Slack Q&A events are currently unreachable.

The handler only dispatches handleQaMessage(...) for telegram.*; any non-telegram, non-cron event is dropped. That makes the relay/slack parsing branches dead for live traffic.

Suggested fix
   handler: async (ctx, event) => {
-    // Q&A path: telegram message
-    if (typeof event.type === 'string' && event.type.startsWith('telegram.')) {
-      await handleQaMessage(ctx, event as unknown as AgentEvent, 'telegram');
-      return;
-    }
+    if (typeof event.type === 'string') {
+      if (event.type.startsWith('telegram.')) {
+        await handleQaMessage(ctx, event as unknown as AgentEvent, 'telegram');
+        return;
+      }
+      if (event.type.startsWith('relay.')) {
+        await handleQaMessage(ctx, event as unknown as AgentEvent, 'relay');
+        return;
+      }
+      if (event.type.startsWith('slack.')) {
+        await handleQaMessage(ctx, event as unknown as AgentEvent, 'slack');
+        return;
+      }
+    }
     // Cron path
     if (!isCronTickEvent(event as unknown as AgentEvent)) return;

Also applies to: 151-163

🤖 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 108 - 119, The Agent handler in agent.ts
only routes Q&A traffic when event.type starts with telegram., so relay/slack
Q&A events never reach handleQaMessage or the later parsing branches. Update the
dispatcher in handler to recognize the relay/slack event types alongside
telegram before the cron check, and make sure the existing Q&A parsing path in
handleQaMessage is invoked for those events rather than being dropped as
non-cron noise.

Comment thread hn-monitor/agent.ts Outdated
Comment thread hn-monitor/agent.ts Outdated
@agent-relay-code

Copy link
Copy Markdown
Contributor

Clean — node_modules is gitignored and I made no source edits. The PR cannot be auto-fixed mechanically; the blocker requires human authorship.

Review of PR #88feat/unified-hn-monitor

This PR unifies the Slack and Telegram hn-monitor into a single dual-transport agent, replacing direct @relayfile/relay-helpers Slack calls with a new @agentworkforce/delivery abstraction (createDelivery, publish, send, plus re-exported input/list/withTimeout/fetchWithTimeout).

Blocking issue (build fails in a clean checkout) — NOT auto-fixable

The new dependency @agentworkforce/delivery is declared as a local file path that does not exist in this repo.

  • package.json:17, package-lock.json, and pnpm-lock.yaml all pin it to file:../workforce/packages/delivery — a sibling-monorepo directory outside this repo.

  • npm install succeeds but only creates a dangling symlink (node_modules/@agentworkforce/delivery -> ../../../workforce/packages/delivery); the target does not exist in the checkout, and the Pullfrog CI uses a plain actions/checkout with no sibling workforce repo.

  • I reproduced the failure with the canonical commands:

    npx tsc --noEmit and npm test both fail with:

    hn-monitor/agent.ts(32,8): error TS2307: Cannot find module '@agentworkforce/delivery' or its corresponding type declarations.
    hn-monitor/agent.ts(130,52): error TS7006: Parameter 't' implicitly has an 'any' type.
    hn-monitor/agent.ts(134,56): error TS7006: Parameter 't' implicitly has an 'any' type.
    

    The two implicit-any errors are downstream of the same root cause: list()/input() come from the unresolvable module, so their return types collapse to any.

The test command never even reaches the test files — compilation aborts. Every other agent in this repo uses published npm versions (^4.0.0) plus the in-repo shared/telegram.ts + @relayfile/relay-helpers; this PR is the only consumer of @agentworkforce/delivery.

Why I did not edit anything: This is an architecture/dependency-resolution decision, not a mechanical fix. The correct resolution is a human call between (a) publishing @agentworkforce/delivery to npm and pinning a real version, (b) vendoring it into the repo (e.g. under shared/), or (c) using the existing shared/telegram.ts pattern. I cannot fabricate the missing package or perform a large semantic rewrite of the handler, and changing the file: path would not make it resolve. The 3 type errors are symptoms of this one blocker — there is no mechanical edit that makes the build green.

Non-blocking observations (advisory — left unchanged)

  • DeliveryResult import is unused (agent.ts:31). Normally a mechanical removal, but I cannot verify the change compiles end-to-end while the module itself is unresolvable (the whole file is a compile error), so I'm not editing it in isolation — fold it in with the dependency fix.
  • hn-monitor-telegram/ still exists. If this PR's intent is to unify Slack+Telegram into one agent, the standalone hn-monitor-telegram/ agent may now be redundant/duplicative. Not touched by this diff — flagging for the author to confirm whether it should be removed in this PR or a follow-up.

Impact trace (no external breakage found)

  • Removing the exported handleInboxMessage is safe: the only other handleInboxMessage definitions (daytona-monitor, gcp-watcher, neon-monitor) are independent local functions, not imports from hn-monitor. The test correctly dropped that import.
  • tests/hn-monitor.test.mjs was updated to the new delivery-based signatures and is internally consistent with the new postFreshStories. (I did not modify tests.)
  • No other repo files import from hn-monitor/agent.

Addressed comments

  • No prior bot or human review comments were available in the harness context (.workforce/context.json contains only PR metadata; no comment/review threads were provided), so there are none to reconcile. All findings above are my own, validated against the current checkout.

Advisory Notes

  • Consider removing the now-redundant hn-monitor-telegram/ agent if the unified agent supersedes it. This is outside this PR's changed-file set and should not be folded in without author intent — recording as advisory only.

This PR is not ready: the canonical build/test command fails in a clean checkout because @agentworkforce/delivery resolves to a non-existent local path, and the fix requires human judgment (publish/vendor the package or rework the transport) rather than a mechanical edit. I made no code changes and left the working tree clean. I am therefore not printing READY.

@agent-relay-code

Copy link
Copy Markdown
Contributor

I made no edits to the working tree (nothing to discard — git status is clean). Here is my review.


PR #88 Review — feat(hn-monitor): unify Slack + Telegram into single dual-transport agent

Verdict: Changes requested — the PR does not build.

I reproduced the repo's canonical CI command (npm install then npm test, which runs tsc --noEmit false --outDir .test-build && node --test tests/*.test.mjs) on the current checkout. The TypeScript compile fails before any test runs:

hn-monitor/agent.ts(32,8): error TS2307: Cannot find module '@agentworkforce/delivery' or its corresponding type declarations.
hn-monitor/agent.ts(130,52): error TS7006: Parameter 't' implicitly has an 'any' type.
hn-monitor/agent.ts(134,56): error TS7006: Parameter 't' implicitly has an 'any' type.

npx tsc --noEmit (the typecheck script) fails identically.

Blocking finding (not auto-fixable — needs human decision)

@agentworkforce/delivery is an unresolvable dependency. The PR adds it to package.json/package-lock.json/pnpm-lock.yaml as:

"@agentworkforce/delivery": "file:../workforce/packages/delivery"
  • That path (../workforce/packages/delivery) is a sibling repo that does not exist in this checkout. After npm install, node_modules/@agentworkforce/delivery is a dangling symlink to a missing target, so the module cannot be imported or typechecked.
  • The package is not published to npm (the only resolution is the local file: path).
  • No other agent in the repo uses it — I grepped the whole tree; createDelivery/DeliveryClient/@agentworkforce/delivery appear only in hn-monitor/agent.ts (and the three dependency manifests).
  • The established in-repo convention for the exact same job is shared/telegram.ts (hn-monitor-telegram/agent.ts:24-31, plus spotify-releases-telegram, joke-bot-telegram, inbox-buddy-telegram all import from ../shared/telegram.js). The Slack path used @relayfile/relay-helpers.

The two TS7006 implicit-any errors at agent.ts:130 and :134 are downstream of the missing module: list/input come from @agentworkforce/delivery, so list(...) has type any and .map((t) => ...) infers t: any. They will most likely clear once the module resolves with real types, but they cannot be verified until it does.

I did not auto-fix this. Pointing the dependency at a published version, vendoring the package, or porting the handler onto the existing shared/telegram.ts transport are all semantic/architecture decisions that change runtime wiring and are out of scope for mechanical cleanup. This needs the PR author to make the @agentworkforce/delivery package resolvable in CI (publish it, or commit/vendor it into the repo, or switch to shared/telegram.ts).

Non-blocking observations (advisory — left unchanged)

  • Unused import DeliveryResult (agent.ts:31) — imported, never referenced. Normally a mechanical removal, but I did not touch it: (a) the module is unresolvable so I cannot compile-verify the edit, and (b) tsconfig has no noUnusedLocals, so it is not a build error and removing it does not unblock CI. Leave it for the author to drop alongside the dependency fix.
  • Unused import envelopeToAgentEvent (tests/hn-monitor.test.mjs:4) — the old inboxEvent helper that used it was removed. Harmless (the .mjs test isn't type-checked by the tsconfig include set), and I do not modify tests per policy. Author may remove it.
  • Lockfile/manifest drift: package.json bumps @types/node to ^22.20.0, but pnpm-lock.yaml still records the specifier as ^22 (pnpm-lock.yaml importer block). npm install tolerated this; pnpm install --frozen-lockfile may flag it depending on how CI installs. Worth reconciling when the dependency situation is sorted.

Safety review of the behavior change (no regression found)

The refactor of postFreshStories preserves the at-least-once dedup guard: the seen claim is still saved before posting, the provisional claim is only released (saveSeen(ctx, seen)) when !headerPosted, and once the header lands the claim is kept on body failure (now also persisting a pending-thread-body record for next-tick retry) rather than rethrowing and risking a duplicate header. This does not turn any fail-closed path into fail-open. The new retryPendingThreadBody only retries when configured targets match. No lifecycle/reaper/dispatch code is touched. These parts are sound — they just can't compile.

Addressed comments

  • No human reviews and no bot/reviewer comments were present in .workforce/context.json, and no separate review-comments file was provided in .workforce/. There were no prior threads to validate or resolve.

Advisory Notes

  • Consider aligning hn-monitor's dual-transport implementation with the existing shared/telegram.ts pattern used by the other unified telegram agents, rather than introducing a new @agentworkforce/delivery dependency — but this is a design-direction call for the author/maintainers, not something to fold into this review.

I made no edits to the working tree (it remains clean); the blocker is a missing dependency that only a human can resolve, and the cascading type errors and lockfile drift cannot be safely or mechanically fixed without it. Because a required build/typecheck step is currently failing, I am not printing READY.

@khaliqgant khaliqgant force-pushed the feat/unified-hn-monitor branch from f23b1d4 to 48da94e Compare June 23, 2026 17:00
- 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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
shared/delivery/delivery.ts (1)

121-136: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Non-blocking partial failures are silently swallowed.

In non-blocking mode ok = refs.length > 0, so when one target succeeds and another throws (errors populated, ok=true), the warning at line 127 (if (!ok && errors.length > 0)) never fires. The consumer in postFreshStories detects the missing ref via refs.length < targets.length, but the underlying error reason is never logged anywhere, making partial-delivery debugging difficult.

Consider emitting the warning whenever errors.length > 0, independent of ok.

♻️ Proposed change
-    if (!ok && errors.length > 0) {
+    if (errors.length > 0) {
       this.ctx.log?.('warn', 'delivery.partial-failure', { errors, nonBlocking });
     }
🤖 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 `@shared/delivery/delivery.ts` around lines 121 - 136, In non-blocking mode,
partial failures where some targets succeed and others throw are not being
logged because the warning condition checks both !ok and errors.length > 0, but
in non-blocking mode ok evaluates to true when refs.length > 0 regardless of
errors. Remove the !ok condition from the warning check on the line with
this.ctx.log?.('warn', 'delivery.partial-failure', { errors, nonBlocking }) so
that the warning is emitted whenever errors.length > 0, independent of the ok
flag, ensuring partial delivery failures are always logged for debugging.
🤖 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 `@shared/delivery/delivery.ts`:
- Around line 218-243: In the sendTelegram method, the condition on line 233
skips error handling for non-blocking sends, allowing execution to continue to
line 241 where messageId becomes an empty string when result.ok is false (which
occurs in non-blocking mode due to writebackTimeoutMs: 0). This breaks reply
threading when the returned ref is used as a parent. Unlike the Slack
implementation (lines 191-217) which embeds parentRef.draftRef directly in the
message body to ensure threading works independently of receipt timing, Telegram
relies entirely on messageId from the receipt. You need to either embed the
parent reference information directly in the message text similar to the Slack
approach, or ensure that a valid messageId is returned for non-blocking sends
that can be used for subsequent reply threading instead of returning an empty
string.

In `@shared/delivery/types.ts`:
- Around line 17-22: The TelegramRef interface is missing a draftRef field that
is needed for cloud-side threading support in non-blocking mode. Add a draftRef
field to the TelegramRef interface (optional string type similar to how SlackRef
has it), then update the sendTelegram function to populate this draftRef field
when creating the TelegramRef object in the same way that sendSlackNonBlocking
populates draftRef. This will ensure that threading works correctly in
non-blocking mode when messageId is initially empty, by allowing the draftRef to
be used as the parent reference instead of relying on the empty messageId.

---

Nitpick comments:
In `@shared/delivery/delivery.ts`:
- Around line 121-136: In non-blocking mode, partial failures where some targets
succeed and others throw are not being logged because the warning condition
checks both !ok and errors.length > 0, but in non-blocking mode ok evaluates to
true when refs.length > 0 regardless of errors. Remove the !ok condition from
the warning check on the line with this.ctx.log?.('warn',
'delivery.partial-failure', { errors, nonBlocking }) so that the warning is
emitted whenever errors.length > 0, independent of the ok flag, ensuring partial
delivery failures are always logged for debugging.
🪄 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: a538aedf-060e-4355-8f3e-9d1af58af628

📥 Commits

Reviewing files that changed from the base of the PR and between f23b1d4 and b37c092.

📒 Files selected for processing (6)
  • hn-monitor/agent.ts
  • shared/delivery/delivery.ts
  • shared/delivery/helpers.ts
  • shared/delivery/index.ts
  • shared/delivery/types.ts
  • tests/hn-monitor.test.mjs
✅ Files skipped from review due to trivial changes (2)
  • shared/delivery/index.ts
  • shared/delivery/helpers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • hn-monitor/agent.ts

Comment thread shared/delivery/delivery.ts Outdated
Comment thread shared/delivery/types.ts Outdated
Comment on lines +17 to +22
export interface TelegramRef {
provider: 'telegram';
chatId: string;
/** Delivered message id (set after the writeback receipt arrives). */
messageId: string;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

fd -t f "types.ts" | grep -i delivery

Repository: AgentWorkforce/agents

Length of output: 184


🏁 Script executed:

fd -t f "delivery.ts"

Repository: AgentWorkforce/agents

Length of output: 187


🏁 Script executed:

# Find the shared directory structure
git ls-files shared/delivery/

Repository: AgentWorkforce/agents

Length of output: 264


🏁 Script executed:

cat -n shared/delivery/types.ts

Repository: AgentWorkforce/agents

Length of output: 5161


🏁 Script executed:

cat -n shared/delivery/delivery.ts | head -300

Repository: AgentWorkforce/agents

Length of output: 9816


🏁 Script executed:

# Check for replyTo and parentRef usage
rg -n "replyTo|parentRef" shared/delivery/ -A 2 -B 2

Repository: AgentWorkforce/agents

Length of output: 7011


TelegramRef lacks a draftRef field for cloud-side threading in non-blocking mode.

Unlike SlackRef (which carries draftRef for embedding in the message body as parentRef), TelegramRef only has messageId. In non-blocking mode, messageId is set to an empty string (see line 241: messageId: result.ok ? result.messageId : ''). When this empty messageId is later used as a parent ref, the condition at line 229 (parentRef?.messageId) fails, and replyToMessageId is never set. This breaks threading for Telegram in the non-blocking publish pattern, whereas Slack threading works correctly because it relies on draftRef embedded in the message body.

Add a draftRef field to TelegramRef and populate it in sendTelegram (similar to how sendSlackNonBlocking populates it) to support the documented non-blocking threading pattern.

🤖 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 `@shared/delivery/types.ts` around lines 17 - 22, The TelegramRef interface is
missing a draftRef field that is needed for cloud-side threading support in
non-blocking mode. Add a draftRef field to the TelegramRef interface (optional
string type similar to how SlackRef has it), then update the sendTelegram
function to populate this draftRef field when creating the TelegramRef object in
the same way that sendSlackNonBlocking populates draftRef. This will ensure that
threading works correctly in non-blocking mode when messageId is initially
empty, by allowing the draftRef to be used as the parent reference instead of
relying on the empty messageId.

- 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).
@khaliqgant khaliqgant merged commit a0577f5 into main Jun 23, 2026
2 checks passed
@khaliqgant khaliqgant deleted the feat/unified-hn-monitor branch June 23, 2026 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant