Conversation
yordis
commented
Aug 4, 2026
yordis
commented
Member
- The agent platform had no human-facing surface: every path to an agent started at a machine webhook, so there was no way for a person to simply talk to one.
- Telegram is the first surface, but a person, a chat, and an agent session are named channel-neutrally on purpose, so adding a second platform does not mean relearning who is talking to whom.
- A long-lived chat needs a way to start over without abandoning the chat itself, which is why resetting a session belongs to the surface rather than to an operator.
- An update larger than the NATS max payload reaches the stream as an empty body with claim headers, and nothing on the consuming side redeemed it, so real messages were being destroyed while the log blamed the parser.
…astructure Squashes the telegram branch history into one commit so it lands as a single reviewable unit rather than 113 incremental commits accumulated during development. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
A conversation outlives the ephemeral session behind it, so a user needs a way to abandon accumulated context without losing their binding to an agent. Rotating on any prompt failure also discarded live conversations that were merely unreachable for a moment. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
The data model and the architecture doc both call this axis a channel, while only the crate and three type names said chat. That read the boundary as chat-app-specific when it covers email, SMS, and push just as well, and it gave no signal about the surfaces that genuinely do not belong. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…surface Every crate directory in the workspace shares one vocabulary with the crates inside it, and chat/ became the sole exception the moment the shared crate stopped being called chat. A half-finished rename reads worse than none: the crate doc described its own sibling as a channel binary named chat-bridge. Deriving the durable consumer identity from the crate name meant any future rename could silently reset a deployment's position in the stream, and a first run replayed history the agent had no business answering. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…space does Conversation ids are KV keys, so a random v4 threw away the creation ordering every other durable id in the workspace gets for free. The v4 here was the last one outside the correlation-token cases where ordering genuinely does not matter. Taking the generator as an argument keeps the crate honest about the clock discipline it already documents one field away. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Internal notes were only staying out of the index because of a machine-local global ignore, so the repo did not protect itself on a fresh clone or in CI. Ignoring .dockerignore was the inverse mistake: build context config belongs in version control. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
The eager claim-check the routing design assumed was never built, and it forced every turn to pay for media the agent may never read while pushing platform credentials toward a component whose value is being generic. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…t uses it Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
… of identity The group semantics were only discoverable by reading the pipeline, and the one place they surprise a reader is authorization: linking a room authorizes everyone in it, present and future. The routing page was also unreachable from the site nav, so none of this vocabulary was findable. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
The document carried the shipped topology and a speculative one side by side, both in the present tense, so a reader could not tell which one runs. That ambiguity produced the same question twice about subjects that do not exist. Rejected alternatives belong in the decision list, not in a second diagram. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…k hole The rewrite described streaming edit-in-place output that the Telegram outbound surface has no method for, restating the kind of unverified claim it was meant to remove. Separately, both this doc and ADR#0044 asserted the gateway carries no object-store dependency while its publish path has claim-checked since before either was written, which hid a consumer that drops oversized updates and blames the parser. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
An update over the NATS max payload arrives with an empty body and claim headers, so the bridge read it as unparseable and acked it: the only copy of a real user message was destroyed silently, and the log blamed the parser. A redeem failure now leaves the message for redelivery, because the bytes exist and the failure is recoverable. The consumer is bound to the bucket it reads so a wrong bucket reports itself instead of masquerading as data loss, and the bucket default moved next to the headers it travels with so the two sides cannot drift apart. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
PR SummaryMedium Risk Overview Shared logic lives in Docs add Multi-Channel Agent Routing, glossary entries, and ADR#0044 (dedicated downloader + Reviewed by Cursor Bugbot for commit 15fc0d2. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds a channel-neutral Rust routing library, a Telegram ACP bridge, JetStream-backed channel and claim storage, runtime integration, and documentation for multi-channel routing and inbound media handling. ChangesChannel-neutral contracts
Telegram channel bridge
Documentation and integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Telegram
participant Gateway
participant JetStream
participant TelegramBridge
participant ChannelStore
participant ACPAgent
Telegram->>Gateway: Publish inbound update
Gateway->>JetStream: Store claim and publish message
TelegramBridge->>JetStream: Consume inbound message
TelegramBridge->>ChannelStore: Resolve principal and conversation
TelegramBridge->>ACPAgent: Create or prompt session
ACPAgent-->>TelegramBridge: Stream response chunks
TelegramBridge->>Telegram: Send chunked response
TelegramBridge->>JetStream: Acknowledge message
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds a human-facing “channel” surface (Telegram first) so a person can hold a long-lived conversation with an agent, including session reset semantics at the surface layer, and fixes claim-check redemption so oversized NATS messages are not silently destroyed.
Changes:
- Introduces the channel-neutral routing/identity/state crate (
trogon-channel) plus a Telegram bridge binary (channel-bridge-telegram) that consumes raw gateway updates, routes to an agent via ACP, and renders replies back to Telegram. - Adds a consumer-side claim-check abstraction (
ClaimResolver) and a protocol-level default claim bucket constant to ensure oversized messages are redeemed before parsing. - Documents the multi-channel routing architecture and adds container/build wiring for the new bridge service.
Reviewed changes
Copilot reviewed 43 out of 45 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rsworkspace/crates/platform/trogon-telemetry/src/service_name.rs | Adds ServiceName::ChannelBridgeTelegram for telemetry attribution. |
| rsworkspace/crates/platform/trogon-telemetry/src/service_name/tests.rs | Extends service-name string/display tests for the new service. |
| rsworkspace/crates/platform/trogon-nats/src/jetstream/object_store.rs | Adds NatsObjectStore::bind for opening an existing object-store bucket without provisioning. |
| rsworkspace/crates/platform/trogon-nats/src/jetstream/mod.rs | Re-exports DEFAULT_CLAIM_BUCKET and new ClaimResolver. |
| rsworkspace/crates/platform/trogon-nats/src/jetstream/claim_check.rs | Introduces ClaimResolver and bucket-mismatch error to make claim redemption safer for consumers. |
| rsworkspace/crates/platform/trogon-nats/src/jetstream/claim_check/integration_tests.rs | Adds integration tests covering claim resolution and bucket mismatch behavior. |
| rsworkspace/crates/platform/trogon-nats/src/constants.rs | Adds DEFAULT_CLAIM_BUCKET as the protocol-level default bucket name. |
| rsworkspace/crates/platform/trogon-gateway/src/constants.rs | Removes gateway-local claim bucket constant in favor of DEFAULT_CLAIM_BUCKET. |
| rsworkspace/crates/platform/trogon-gateway/src/main.rs | Updates gateway claim-check bucket provisioning/publishing to use DEFAULT_CLAIM_BUCKET. |
| rsworkspace/crates/channel/trogon-channel/Cargo.toml | New crate manifest for channel-neutral routing/state types. |
| rsworkspace/crates/channel/trogon-channel/src/lib.rs | Declares channel-neutral modules and re-exports the public API surface. |
| rsworkspace/crates/channel/trogon-channel/src/agent_port.rs | Defines the protocol-neutral AgentPort trait and session lifecycle types. |
| rsworkspace/crates/channel/trogon-channel/src/command.rs | Implements bridge-owned command parsing (/new, /reset, configurable). |
| rsworkspace/crates/channel/trogon-channel/src/conversation.rs | Defines conversation/session identity and durable conversation record. |
| rsworkspace/crates/channel/trogon-channel/src/endpoint.rs | Defines endpoint/principal identity with safe-token validation and tests. |
| rsworkspace/crates/channel/trogon-channel/src/event.rs | Defines the channel-neutral inbound event and attachment handle types. |
| rsworkspace/crates/channel/trogon-channel/src/render.rs | Defines the channel-neutral render command vocabulary. |
| rsworkspace/crates/channel/trogon-channel/src/store.rs | Implements JetStream KV-backed registries for principals, endpoints, bindings, and conversations. |
| rsworkspace/crates/channel/channel-bridge-telegram/Cargo.toml | New Telegram bridge binary manifest. |
| rsworkspace/crates/channel/channel-bridge-telegram/src/main.rs | Bridge entrypoint: config, NATS/JetStream wiring, claim resolver binding, consumer loop, ACP client tasks. |
| rsworkspace/crates/channel/channel-bridge-telegram/src/config.rs | Env-driven bridge configuration (stream names, claim bucket, triggers, seed principals, ACP config). |
| rsworkspace/crates/channel/channel-bridge-telegram/src/parse.rs | Normalizes Telegram Update into channel-neutral InboundEvent. |
| rsworkspace/crates/channel/channel-bridge-telegram/src/pipeline.rs | End-to-end message handling: redeem claim-checked bodies, authorize, manage sessions, prompt agent, render reply. |
| rsworkspace/crates/channel/channel-bridge-telegram/src/pipeline_tests.rs | End-to-end tests using testcontainers NATS to validate routing, resets, and claim-check redemption behavior. |
| rsworkspace/crates/channel/channel-bridge-telegram/src/outbound.rs | Defines the outbound Telegram API seam used by the pipeline. |
| rsworkspace/crates/channel/channel-bridge-telegram/src/render.rs | Buffers streamed ACP session notifications and chunks long text to Telegram’s size limit. |
| rsworkspace/crates/channel/channel-bridge-telegram/src/acp_port.rs | Implements AgentPort over ACP, including session capability-gating and prompt metadata shaping. |
| rsworkspace/Cargo.toml | Adds workspace members/deps for trogon-channel, channel-bridge-telegram, and teloxide. |
| docs/architecture/multi-channel-agent-routing.md | New architecture doc describing the multi-channel routing model and operational characteristics. |
| docs/glossary/index.md | Adds “Channels and conversations” glossary section and entries list. |
| docs/glossary/channel.md | Defines “Channel” glossary entry. |
| docs/glossary/endpoint.md | Defines “Endpoint” glossary entry. |
| docs/glossary/principal.md | Defines “Principal” glossary entry. |
| docs/glossary/conversation.md | Defines “Conversation” glossary entry. |
| docs/glossary/binding.md | Defines “Binding” glossary entry. |
| docs/adr/index.md | Adds ADR#0044 to ADR index. |
| docs/adr/0044-inbound-media-fetch-out-of-band.md | New ADR draft specifying out-of-band inbound media download design. |
| docs/.vitepress/helpers.ts | Adds glossary section ordering for channels/conversations. |
| docs/.vitepress/config.mts | Adds nav link for the multi-channel routing architecture doc. |
| devops/docker/compose/services/channel-bridge-telegram/Dockerfile | Adds Docker build for the Telegram bridge binary. |
| devops/docker/compose/compose.yml | Adds channel-bridge-telegram service to compose profile wiring. |
| .github/workflows/canary-container-images.yml | Includes channel-bridge-telegram in canary container build matrix. |
| .github/canary-container-services.json | Adds canary container service metadata for channel-bridge-telegram. |
| .gitignore | Adds ignores for editor backups, logs, and NATS creds. |
Suppressed comments (1)
rsworkspace/crates/channel/trogon-channel/src/store.rs:46
ensure_bucketcurrently falls back to creating the bucket on anyget_key_valueerror. That means timeouts/permission errors (or other JetStream failures) get misinterpreted as "missing bucket" and may lead to an attempted create (which can update existing resources). It should only create on a definite not-found, and otherwise return an error (e.g.ChannelStoreError::OpenBucket).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The branch had fallen behind far enough that a dependency bump on main and the branch's own added crates were resolving the lockfile differently. Taking main's resolution now keeps the review focused on the channel work instead of on a lockfile argument. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 45 changed files in this pull request and generated no new comments.
Suppressed comments (3)
rsworkspace/crates/channel/channel-bridge-telegram/src/pipeline_tests.rs:489
- This uses
error.to_string().contains(...)to assert on an error message. The workspace’sERROR_STRING_COMPARISONlint flags string probes derived fromError::to_string(). If you want to keep a message assertion in this test, format the error viaDisplayinstead of callingto_string().
let error = pipeline.handle_message(&msg).await.expect_err("must not be acked");
assert!(error.to_string().contains("failed to redeem claim-checked update"));
rsworkspace/crates/platform/trogon-nats/src/jetstream/claim_check/integration_tests.rs:361
- This assertion probes
ClaimResolveError’sDisplaytext viaerror.to_string().contains(...). The workspace has a deny-by-default lint (ERROR_STRING_COMPARISON) that flags string probes on values derived fromError::to_string(), and the check is redundant with the typedBucketMismatchmatch above.
rsworkspace/crates/channel/channel-bridge-telegram/src/pipeline.rs:97 map_err(|e| anyhow!("... {e}"))stringifies the underlying claim-resolution error, which discards its typed context and nudges callers/tests toward message matching. Sinceanyhow::Contextis already in scope, prefer.context(...)so the original error remains available as the source.
let body = self
.claims
.resolve(msg.headers.as_ref(), msg.payload.clone())
.await
.map_err(|e| anyhow::anyhow!("failed to redeem claim-checked update: {e}"))?;
…session No protocol carries a distinct "no such session" code, so the code that rejects an unknown session id is also the code an agent uses to reject a prompt it simply will not answer. Betting the conversation's session pointer on that guess meant an ordinary refusal rotated the conversation onto a fresh session and failed anyway, leaving the user talking to an agent that had lost the thread. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
"Unset" reaches a process as the empty string more often than as an absent variable: Compose renders it that way, and so does a Kubernetes secret reference to a missing key. Reading that as configured meant the bridge started, reported nothing wrong, and then failed on its first Bot API call with nothing at startup to point at the cause. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
The reference linter requires every ADR mention to be a markdown link, so a bare one fails the branch on a formatting rule rather than on anything about the design. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 46 changed files in this pull request and generated 3 comments.
Suppressed comments (2)
rsworkspace/crates/channel/trogon-channel/src/store.rs:45
ensure_bucketcurrently creates the bucket wheneverget_key_valuereturnsErr, which can mask real JetStream failures (e.g. auth/network issues) as "bucket missing" and then report the create error instead of the original cause. It should only create when the get failed specifically because the bucket does not exist, and otherwise surface the get error.
rsworkspace/crates/channel/trogon-channel/src/store.rs:13ensure_bucketcurrently treats anyget_key_valueerror as "bucket is missing" and goes on to create it, butChannelStoreErrorhas no variant to represent an "open existing bucket" failure. Adding a dedicatedGetBucketvariant makes non-not-found failures reportable and letsensure_bucketdistinguish missing buckets from real JetStream errors (permission/network/etc.).
The service was left naming a Dockerfile the tree no longer had, so the profile that exists to run this bridge locally could not bring it up at all. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rsworkspace/crates/channel/trogon-channel/src/store.rs (1)
381-392: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftConsider a revision-checked write for the session pointer.
update_conversationusesput, so the last writer wins.create_conversationdeliberately usescreateandupdatewith a revision to avoid exactly this. If two bridge replicas process redelivered messages for the same conversation, onecurrent_sessionwrite can overwrite the other. The overwritten session is then held at the agent with nothing pointing at it, which is the conditionReleaseReason::Replacedexists to avoid.If a single-writer-per-conversation deployment is guaranteed, record that assumption here instead.
🤖 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 `@rsworkspace/crates/channel/trogon-channel/src/store.rs` around lines 381 - 392, Update update_conversation to perform a revision-checked conditional write for current_session updates, reusing the same create/update revision mechanism as create_conversation so concurrent replica writes cannot overwrite each other; otherwise explicitly document the guaranteed single-writer-per-conversation assumption at this method.
🤖 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 `@docs/adr/0044-inbound-media-fetch-out-of-band.md`:
- Around line 181-185: Before documenting the readiness and terminal-record
invariants in this ADR, add a durable retry or reconciliation path for writing
terminal failed channel_media records, including cases where the downloader
acknowledges or stops after final delivery and where the initial KV write fails.
Ensure failed-record persistence completes or is retried independently of
JetStream redelivery so channel_media is not left absent.
In `@rsworkspace/crates/platform/trogon-nats/src/jetstream/claim_check.rs`:
- Around line 142-147: Correct the documentation for UnnamableBucket to state
that the header was present but contained a value that is not a valid bucket
name, rather than saying it lacked a bucket name. Leave resolve’s absent-header
handling and the existing error text unchanged.
---
Nitpick comments:
In `@rsworkspace/crates/channel/trogon-channel/src/store.rs`:
- Around line 381-392: Update update_conversation to perform a revision-checked
conditional write for current_session updates, reusing the same create/update
revision mechanism as create_conversation so concurrent replica writes cannot
overwrite each other; otherwise explicitly document the guaranteed
single-writer-per-conversation assumption at this method.
🪄 Autofix
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: b6cfd2a1-267e-446e-b8be-630b68f9919e
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.gitignoredevops/docker/compose/services/channel-bridge-telegram/Dockerfiledocs/adr/0044-inbound-media-fetch-out-of-band.mddocs/architecture/multi-channel-agent-routing.mdrsworkspace/crates/channel/channel-bridge-telegram/Cargo.tomlrsworkspace/crates/channel/channel-bridge-telegram/src/acp_port.rsrsworkspace/crates/channel/channel-bridge-telegram/src/acp_port_tests.rsrsworkspace/crates/channel/channel-bridge-telegram/src/config.rsrsworkspace/crates/channel/channel-bridge-telegram/src/config_tests.rsrsworkspace/crates/channel/channel-bridge-telegram/src/pipeline_tests.rsrsworkspace/crates/channel/trogon-channel/src/agent_port.rsrsworkspace/crates/channel/trogon-channel/src/agent_port_tests.rsrsworkspace/crates/channel/trogon-channel/src/conversation_tests.rsrsworkspace/crates/channel/trogon-channel/src/event.rsrsworkspace/crates/channel/trogon-channel/src/event_tests.rsrsworkspace/crates/channel/trogon-channel/src/lib.rsrsworkspace/crates/channel/trogon-channel/src/store.rsrsworkspace/crates/channel/trogon-channel/src/store_tests.rsrsworkspace/crates/platform/trogon-nats/src/jetstream/claim_bucket.rsrsworkspace/crates/platform/trogon-nats/src/jetstream/claim_check.rsrsworkspace/crates/platform/trogon-nats/src/jetstream/claim_check/integration_tests.rsrsworkspace/crates/platform/trogon-nats/src/jetstream/mod.rs
💤 Files with no reviewable changes (1)
- rsworkspace/crates/channel/channel-bridge-telegram/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (10)
- devops/docker/compose/services/channel-bridge-telegram/Dockerfile
- rsworkspace/crates/channel/trogon-channel/src/conversation_tests.rs
- rsworkspace/crates/channel/trogon-channel/src/lib.rs
- rsworkspace/crates/channel/channel-bridge-telegram/src/acp_port_tests.rs
- rsworkspace/crates/platform/trogon-nats/src/jetstream/mod.rs
- rsworkspace/crates/channel/channel-bridge-telegram/src/pipeline_tests.rs
- rsworkspace/crates/channel/trogon-channel/src/event.rs
- rsworkspace/crates/channel/trogon-channel/src/event_tests.rs
- rsworkspace/crates/platform/trogon-nats/src/jetstream/claim_check/integration_tests.rs
- .gitignore
These crates were the last place where finding a module's tests meant following a path attribute to a sibling file, instead of the layout every other crate in the workspace already uses. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Nothing builds this image at pull request time and the canary workflow only fires on a push to main, so a build that cannot succeed lands as a red main rather than as a failed check. The Compose service goes with the file it built from, since a profile naming a Dockerfile the tree does not have cannot come up either. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
The check had to be spelled as a constant, which meant widening the repo's own rule about where constants may live in order to accept it, and being evaluated at compile time it left the file reporting lines no test can reach. What it guaranteed is one assertion wide. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
The variant read as though the header had been left out, which is a different deployment problem from the one it is raised for. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
The decision read as though every abandoned handle ends in a record, so a reader could conclude that waiting on one is safe. Liveness rests on the reader's deadline, and only the explanation an agent receives depends on the record. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
One of the two unwind paths could not be reached from any test, because the create it follows performs that same read internally and cannot report a conflict unless it already succeeded. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Nothing showed that an agent handing over an id too long for a subject token is told which rule it broke, or that an id sitting exactly at the limit can still address a session. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Matching a fragment of the message left the half that names the variable to edit unasserted, which is the half an operator needs. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/adr/0044-inbound-media-fetch-out-of-band.md (1)
135-151: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not promise a terminal record after documenting cases that produce none.
The ADR says the downloader writes before acknowledgement. It also says a KV outage or downloader crash can exhaust delivery attempts with no record. Therefore, “ends as a terminal record” and “Every handle ... leaves a terminal record” are not guaranteed. Add durable reconciliation, or qualify the invariant and consequence so the reader deadline is the only guaranteed fallback when the record write fails.
🤖 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 `@docs/adr/0044-inbound-media-fetch-out-of-band.md` around lines 135 - 151, Revise the ADR’s terminal-record claims to distinguish the intended outcome from the guaranteed invariant: remove or qualify statements such as “ends as a terminal record” and “Every handle ... leaves a terminal record” to acknowledge that KV failures or downloader crashes may leave no record after redelivery exhaustion. State that the reader deadline is the guaranteed liveness fallback when terminal-record persistence fails, and avoid promising durable reconciliation unless it is actually implemented.
🧹 Nitpick comments (1)
rsworkspace/crates/channel/channel-bridge-telegram/src/pipeline/tests.rs (1)
612-618: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the error variant so the test proves the claim failed.
Line 613 asserts only
is_err(). This test seeds no principal, so several later stages would also fail, and the assertion would still pass if the claim resolved and authorization refused instead. The sibling tests already pin the variant at lines 871, 1117, and 1127. PinPipelineError::Claimhere for the same reason.♻️ Proposed tightening
let msg = next_message(&mut messages).await; - assert!(pipeline.handle_message(&msg).await.is_err(), "must not be acked"); + let error = pipeline + .handle_message(&msg) + .await + .expect_err("an unredeemable claim must not be acked"); + assert!( + matches!(error, PipelineError::Claim(_)), + "the claim failure must surface, got {error:?}" + );🤖 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 `@rsworkspace/crates/channel/channel-bridge-telegram/src/pipeline/tests.rs` around lines 612 - 618, Update the failure assertion in the test around next_message and pipeline.handle_message to match the specific PipelineError::Claim variant instead of only checking is_err(). Preserve the existing “must not be acked” assertion and subsequent empty prompt/outbound checks.
🤖 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 `@docs/adr/0044-inbound-media-fetch-out-of-band.md`:
- Around line 196-201: Update the readiness lifecycle contract in the ADR by
choosing either persisted pending records at ingestion or absence-as-unresolved
with only ready and failed as explicit records. Apply the chosen model
consistently to the downloader lifecycle, reader deadline behavior, and agent
download tool, removing contradictory statements about absent records and
explicit readiness states.
---
Duplicate comments:
In `@docs/adr/0044-inbound-media-fetch-out-of-band.md`:
- Around line 135-151: Revise the ADR’s terminal-record claims to distinguish
the intended outcome from the guaranteed invariant: remove or qualify statements
such as “ends as a terminal record” and “Every handle ... leaves a terminal
record” to acknowledge that KV failures or downloader crashes may leave no
record after redelivery exhaustion. State that the reader deadline is the
guaranteed liveness fallback when terminal-record persistence fails, and avoid
promising durable reconciliation unless it is actually implemented.
---
Nitpick comments:
In `@rsworkspace/crates/channel/channel-bridge-telegram/src/pipeline/tests.rs`:
- Around line 612-618: Update the failure assertion in the test around
next_message and pipeline.handle_message to match the specific
PipelineError::Claim variant instead of only checking is_err(). Preserve the
existing “must not be acked” assertion and subsequent empty prompt/outbound
checks.
🪄 Autofix
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: 79aa5eac-afa4-4e63-b456-885ab1d256b3
📒 Files selected for processing (30)
docs/adr/0044-inbound-media-fetch-out-of-band.mdrsworkspace/crates/channel/channel-bridge-telegram/src/acp_port.rsrsworkspace/crates/channel/channel-bridge-telegram/src/acp_port/tests.rsrsworkspace/crates/channel/channel-bridge-telegram/src/config.rsrsworkspace/crates/channel/channel-bridge-telegram/src/config/tests.rsrsworkspace/crates/channel/channel-bridge-telegram/src/parse.rsrsworkspace/crates/channel/channel-bridge-telegram/src/parse/tests.rsrsworkspace/crates/channel/channel-bridge-telegram/src/pipeline.rsrsworkspace/crates/channel/channel-bridge-telegram/src/pipeline/tests.rsrsworkspace/crates/channel/channel-bridge-telegram/src/render.rsrsworkspace/crates/channel/channel-bridge-telegram/src/render/tests.rsrsworkspace/crates/channel/trogon-channel/src/agent_port.rsrsworkspace/crates/channel/trogon-channel/src/agent_port/tests.rsrsworkspace/crates/channel/trogon-channel/src/command.rsrsworkspace/crates/channel/trogon-channel/src/command/tests.rsrsworkspace/crates/channel/trogon-channel/src/command_trigger_input.rsrsworkspace/crates/channel/trogon-channel/src/command_trigger_input/tests.rsrsworkspace/crates/channel/trogon-channel/src/conversation.rsrsworkspace/crates/channel/trogon-channel/src/conversation/tests.rsrsworkspace/crates/channel/trogon-channel/src/endpoint.rsrsworkspace/crates/channel/trogon-channel/src/endpoint/tests.rsrsworkspace/crates/channel/trogon-channel/src/event.rsrsworkspace/crates/channel/trogon-channel/src/event/tests.rsrsworkspace/crates/channel/trogon-channel/src/safe_token.rsrsworkspace/crates/channel/trogon-channel/src/safe_token/tests.rsrsworkspace/crates/channel/trogon-channel/src/store.rsrsworkspace/crates/channel/trogon-channel/src/store/tests.rsrsworkspace/crates/platform/trogon-nats/src/jetstream/claim_bucket.rsrsworkspace/crates/platform/trogon-nats/src/jetstream/claim_bucket/tests.rsrsworkspace/crates/platform/trogon-nats/src/jetstream/claim_check.rs
🚧 Files skipped from review as they are similar to previous changes (13)
- rsworkspace/crates/channel/channel-bridge-telegram/src/acp_port.rs
- rsworkspace/crates/channel/trogon-channel/src/command_trigger_input.rs
- rsworkspace/crates/channel/channel-bridge-telegram/src/parse.rs
- rsworkspace/crates/platform/trogon-nats/src/jetstream/claim_bucket/tests.rs
- rsworkspace/crates/channel/channel-bridge-telegram/src/render.rs
- rsworkspace/crates/channel/trogon-channel/src/agent_port.rs
- rsworkspace/crates/channel/trogon-channel/src/endpoint.rs
- rsworkspace/crates/channel/channel-bridge-telegram/src/config.rs
- rsworkspace/crates/channel/trogon-channel/src/safe_token.rs
- rsworkspace/crates/platform/trogon-nats/src/jetstream/claim_bucket.rs
- rsworkspace/crates/channel/trogon-channel/src/store.rs
- rsworkspace/crates/channel/trogon-channel/src/event.rs
- rsworkspace/crates/channel/trogon-channel/src/conversation.rs
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 188716d. Configure here.
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
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
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.