Uh oh!
There was an error while loading. Please reload this page.
fix(slack): answer in a thread, and make the manifest readable - #121
Conversation
Two problems seen in production on the first live @mention. Replies landed in the channel, not a thread. `postReply` only threaded when the incoming message was ALREADY in one, so a top-level `@Agent Relay …` got a channel-level answer. That is worst exactly where agents are useful: every agent shares one Slack identity, so one mention draws a reply from each agent watching the channel and they all pile into the main channel. Now the reply threads under the incoming message, and stays in the existing thread when there is one. `capabilities --json` dumped ~4KB of minified JSON as plain text, which Slack renders as an unreadable wall. Pretty-print it inside a fenced block and lead with one line naming the agent and saying you can just ask a question in plain language — nobody should have to parse JSON to discover that. The threading change is in shared/slack.ts, so it applies to every agent using that helper. That is intended: an agent answering a channel mention should thread. The existing test asserting a channel-level `post` encoded the old behaviour and now asserts the thread. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQpuAYyuSG3MWSZYnw5o1x
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Warning Review limit reachedNext included review available in 11 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe capabilities command now delegates formatted JSON output to an exported renderer. Slack replies now always use a thread root, including for top-level messages. ChangesCapabilities JSON rendering
Slack threaded replies
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk:⚪ Minimal · up to Replies now stay under the relevant Slack message and capability output is easier to read without changing its content; no actionable merge-blocking risk remains after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:2cf284a9cc
ℹ️ 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".
| const threadTs = msg.threadTs ?? msg.ts; | ||
| const result = threadTs | ||
| ? await slack.reply(chanId, threadTs, text) |
There was a problem hiding this comment.
When Inbox Buddy handles a top-level message, conversationKeyForSlack stores that turn under the channel key, but this change posts the answer beneath msg.ts; a user's natural follow-up in that new thread then arrives with threadTs and is loaded under channel:threadTs. Consequently, the initial question and answer are absent from the follow-up prompt, breaking the multi-turn continuity implemented in inbox-buddy/agent.ts. Ensure the initial turn and its resulting thread use the same conversation key.
Useful? React with 👍 / 👎.
| 'GTM Signal Scout — machine-readable capability manifest.', | ||
| 'You can also just ask a GTM question in plain language, or send' | ||
| + ' \u201cwhat can you tell me?\u201d for the short version.', | ||
| '```json', |
There was a problem hiding this comment.
When an agent or catalog invokes capabilities --json over Relay, handleRelayMessage now returns these prose and fence lines around the payload, so parsing the response directly with JSON.parse fails. This affects Relay as well as Slack because both transports share handleInteractiveCommand, despite the documented contract in ASKABLE_AGENTS.md describing this command as a machine-readable relay response; apply Slack-only presentation formatting or leave the relay response as standalone JSON.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="shared/slack.ts">
<violation number="1" location="shared/slack.ts:138">
P3: The `postReply` JSDoc now understates the behavior: this line also threads top-level messages under their own timestamp. Update the docstring to describe both top-level and existing-thread replies.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| // buries the conversation in the main channel, which is worst exactly where | ||
| // agents are useful — several agents share one Slack identity, so a single | ||
| // `@Agent Relay` mention can draw a reply from each of them. | ||
| const threadTs = msg.threadTs ?? msg.ts; |
There was a problem hiding this comment.
P3: The postReply JSDoc now understates the behavior: this line also threads top-level messages under their own timestamp. Update the docstring to describe both top-level and existing-thread replies.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At shared/slack.ts, line 138:
<comment>The `postReply` JSDoc now understates the behavior: this line also threads top-level messages under their own timestamp. Update the docstring to describe both top-level and existing-thread replies.</comment>
<file context>
@@ -130,10 +130,16 @@ export async function postReply(
+ // buries the conversation in the main channel, which is worst exactly where
+ // agents are useful — several agents share one Slack identity, so a single
+ // `@Agent Relay` mention can draw a reply from each of them.
+ const threadTs = msg.threadTs ?? msg.ts;
+ const result = threadTs
+ ? await slack.reply(chanId, threadTs, text)
</file context>
Both P1s from review, each flagged independently by both reviewers.
Threading was applied in shared/slack.ts for every agent, which would have
broken inbox-buddy's multi-turn context: `conversationKeyForSlack` keys a
top-level message on the CHANNEL, so answering it in a new thread stranded the
opening turn under `channel` while the follow-up loaded under `channel:ts` —
the prompt would lose the question it was answering.
Threading is now opt-in via `postReply(..., { startThread: true })`, and
`conversationKeyForSlack` takes the same option so an agent that opts in keys
continuity on the thread it created. Every existing caller passes nothing and
is byte-for-byte unchanged; only askable-gtm opts in, and it keeps no
cross-turn Slack context so moving the conversation unit costs nothing.
`capabilities --json` is documented in ASKABLE_AGENTS.md as a MACHINE-readable
relay response, and both transports share `handleInteractiveCommand` — so
fencing the payload broke `JSON.parse` for any agent or catalog calling it over
relay. The payload is standalone JSON again; presentation moved to the
transport via an optional `presentJson` hook that only the Slack actor supplies.
Tests pin both: relay receives raw parseable JSON with no fences, Slack
receives the fenced form whose payload still round-trips, and an opted-in
top-level mention threads under its own ts.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SQpuAYyuSG3MWSZYnw5o1xkhaliqgant
commented
Sep 1, 2026
Both P1s addressed in P1 — "Preserve the conversation key when starting a reply thread" (@chatgpt-codex-connector, @cubic-dev-ai) Correct. Rather than patch the key, threading is now opt-in: P1 — "Keep relay capability responses valid JSON" (@chatgpt-codex-connector, @cubic-dev-ai) Correct, and I should have caught it:
P3 — Fixed; the docstring now describes both cases and says why threading is opt-in rather than default. Validation: 48 tests in @codex review |
@khaliqgant Both P1s check out against the diff — good fixes. P1 #1 (conversation key / opt-in threading). The shape you chose is right. Making P1 #2 (relay JSON). Correct call. Fencing in P3 (JSDoc). The updated docstring on One small thing worth a second glance: Nice catch on both — the threading one especially would have shipped a real inbox-buddy regression. Tip: get faster answers by chatting with cubic’s review copilot in the review UI |
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Uh oh!
There was an error while loading. Please reload this page.
The first live answer in Slack was a ~4,300-character wall. Reddit rows carry a `title`, LinkedIn rows do not, so the renderer fell back to `body_text` — and pasted five entire LinkedIn posts into one thread. `oneLine` collapses whitespace but never truncates. A citation is a pointer to a source, not a copy of it. Excerpt to 180 characters on a word boundary, and let the link carry the rest. The same answer is now ~1,200 characters and scannable. Also attribute LinkedIn rows by public author. Reddit rows show their community; LinkedIn has none, so those lines carried no indication of who was speaking — while `author.handle` was already parsed and is evidence the capability manifest promises to surface. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQpuAYyuSG3MWSZYnw5o1x
The test named "relay receives raw JSON while Slack receives the fenced form" only asserted the relay half. Removing `presentJson` from the Slack actor left it green — the exact regression it was written to catch. Now drives `handleSlackMessage` as well and asserts the reply is threaded, fenced, and carries byte-for-byte the same document relay received. Verified by ablation: deleting `presentJson: presentJsonForSlack` fails this test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQpuAYyuSG3MWSZYnw5o1x
khaliqgant
commented
Sep 1, 2026
@cubic-dev-ai P2 addressed in `e0a2b0f` — correct, and a fair hit. The test asserted a claim its own name made twice and only proved once: it captured It now drives Verified by ablation rather than assertion: deleting Worth noting this is the third leaf-only test gap caught by review on this branch and its siblings today, all the same shape: a helper tested directly while the wiring that reaches it went unasserted. I've started ablating rather than trusting green. |
@khaliqgant Agreed, that's a fair hit and The fix lands the right way: driving The pattern you're calling out is worth a habit, not just a patch: a test that names two sides and asserts one is a latent hole. Your "ablate rather than trust green" instinct is the right correction for this branch's whole cluster of same-shaped gaps. Tip: get faster answers by chatting with cubic’s review copilot in the review UI |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="askable-gtm/agent.ts">
<violation number="1" location="askable-gtm/agent.ts:1510">
P3: When a post has a non-BMP character at the excerpt boundary, `truncateEvidence` splits its surrogate pair and sends malformed text to Slack. Count and slice Unicode code points instead of UTF-16 code units.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| export function truncateEvidence(value: string, max = EVIDENCE_EXCERPT_MAX): string { | ||
| const single = oneLine(value); | ||
| if (single.length <= max) return single; | ||
| const clipped = single.slice(0, max); | ||
| const lastSpace = clipped.lastIndexOf(' '); | ||
| // Only honour the word boundary if it does not gut the excerpt. | ||
| const body = lastSpace > max * 0.6 ? clipped.slice(0, lastSpace) : clipped; | ||
| return `${body.replace(/[\s.,;:!?—-]+$/u, '')}…`; |
There was a problem hiding this comment.
P3: When a post has a non-BMP character at the excerpt boundary, truncateEvidence splits its surrogate pair and sends malformed text to Slack. Count and slice Unicode code points instead of UTF-16 code units.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At askable-gtm/agent.ts, line 1510:
<comment>When a post has a non-BMP character at the excerpt boundary, `truncateEvidence` splits its surrogate pair and sends malformed text to Slack. Count and slice Unicode code points instead of UTF-16 code units.</comment>
<file context>
@@ -1488,6 +1499,24 @@ function oneLine(value: string): string {
+ * to a source, not a copy of it: pasting five full LinkedIn posts into a Slack
+ * thread buries the very signal the answer exists to surface.
+ */
+export function truncateEvidence(value: string, max = EVIDENCE_EXCERPT_MAX): string {
+ const single = oneLine(value);
+ if (single.length <= max) return single;
</file context>
| exportfunctiontruncateEvidence(value: string,max=EVIDENCE_EXCERPT_MAX): string{ | |
| constsingle=oneLine(value); | |
| if(single.length<=max)returnsingle; | |
| constclipped=single.slice(0,max); | |
| constlastSpace=clipped.lastIndexOf(' '); | |
| // Only honour the word boundary if it does not gut the excerpt. | |
| constbody=lastSpace>max*0.6 ? clipped.slice(0,lastSpace) : clipped; | |
| return`${body.replace(/[\s.,;:!?—-]+$/u,'')}…`; | |
| exportfunctiontruncateEvidence(value: string,max=EVIDENCE_EXCERPT_MAX): string{ | |
| constcharacters=[...oneLine(value)]; | |
| if(characters.length<=max)returncharacters.join(''); | |
| constclipped=characters.slice(0,max); | |
| constlastSpace=clipped.lastIndexOf(' '); | |
| // Only honour the word boundary if it does not gut the excerpt. | |
| constbody=lastSpace>max*0.6 | |
| ? clipped.slice(0,lastSpace).join('') | |
| : clipped.join(''); | |
| return`${body.replace(/[\s.,;:!?—-]+$/u,'')}…`; | |
| } |
Four lines of ceremony sat above every result: the question restated, the fetch timestamp and raw coverage map, the credential/endpoint disclosure, and a standing caveat about inference. All of it was either something the reader had just typed or something they could see, and it pushed the actual findings below the fold. Two things survive the trim, because dropping them makes the answer dishonest rather than merely terse: - a missing source, now stated as a compact `(reddit unavailable this request)` rather than a sentence. Returning LinkedIn-only results while Reddit is 403 and saying nothing presents a partial view as a complete one, which is the failure this persona exists to avoid. - the access disclosure, but only when the credential is NOT the user's own. The manifest marks managed access `disclosureRequired` because it is metered and billable, and `unknown` discloses too — the gateway did not say which path it used, and staying quiet about a possible charge is the wrong way to be wrong. Same answer as the screenshot that prompted this: ~4,300 chars → ~680. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQpuAYyuSG3MWSZYnw5o1x
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Uh oh!
There was an error while loading. Please reload this page.
Dropping the query header was right for the interactive path — the reader just typed the question — but the renderer serves two callers, and I only reasoned about one. `runWatchSweep` sends the same output as an unsolicited DM that can land hours later beside other watches, so with the header gone the user could not tell which saved query fired, or which id to unwatch. The header belongs at the delivery site rather than in the renderer: the sweep knows it is delivering unsolicited, the interactive path knows it is not. Compact by design — query, cadence, id — so it identifies the update without reintroducing the preamble that buried the results. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQpuAYyuSG3MWSZYnw5o1x
khaliqgant
commented
Sep 1, 2026
@cubic-dev-ai P2 addressed in Dropping the query header was right for the interactive path (the reader just typed the question), but Fixed at the delivery site rather than in the renderer, since that is where the distinction actually lives: the sweep knows it is delivering unsolicited, the interactive path knows it is not. The label is compact by design — New test asserts both halves: a swept delivery carries query, cadence and id and is followed immediately by results; the interactive answer stays bare. 51 tests in this file, 327 in the full suite, 0 failures. |
@khaliqgant Confirmed — The shape reads well against the diff:
Good distinction between "who is reading this" and "does this renderer know that" — the sweep knows, the renderer doesn't, so the label lives at the delivery point. Nothing to add. Tip: get faster answers by chatting with cubic’s review copilot in the review UI |
Uh oh!
There was an error while loading. Please reload this page.
Both problems observed on the first live
@mentionafter cloud#3231 deployed.1. Replies went to the channel, not a thread
postReplyonly threaded when the incoming message was already in one:That is worst exactly where agents are useful. Every agent shares one Slack identity, so a single
@Agent Relay …wakes every agent watching that channel — the production dispatch for one message matched 8 deployments — and they all replied into the main channel. Now the reply threads under the incoming message, and stays in the existing thread when there is one.2.
capabilities --jsonwas an unreadable wall~4KB of minified JSON posted as plain text. Now pretty-printed inside a ```json fence and led by one line naming the agent and pointing at plain-language questions — nobody should have to parse JSON to learn they can just ask.
Blast radius, deliberately
The threading change is in
shared/slack.ts, so it applies to every agent using that helper (hn-monitor, joke-bot, inbox-buddy, review, …). That is the intent: an agent answering a channel mention should thread. Calling it out because it is broader than the askable-gtm title suggests.The existing test asserting a channel-level
postencoded the old behaviour; it now asserts the thread, with the reasoning in a comment so it is not "fixed" back later.Validation
npm run typecheck: passtests/askable-gtm.test.mjs: 46 pass, 2 new — fenced/parseable manifest, and an existing thread answered in that thread rather than a new oneNot fixed here
One
@Agent Relaymention still wakes every agent watching the channel, because they share a Slack identity — cloud#3234 gates on the bot being mentioned, which does not disambiguate which agent is being addressed. Threading makes that liveable, not solved. Worth a follow-up.🤖 Generated with Claude Code
Summary by cubic
Fixes four problems from the first live
@mention: replies thread under the message instead of landing in the channel,capabilities --jsonstays parseable for machines while Slack gets a readable fenced view, cited evidence is an excerpt rather than a whole post, and answers lead with results instead of a preamble.startThreadinpostReply/conversationKeyForSlack; only askable-gtm enables it because it keeps no cross-turn Slack context.capabilities --jsonstays standalone parseable JSON over relay; the Slack actor fences and pretty-prints it via a newpresentJsonhook.@Agent Relaymention still wakes every agent watching the channel since they share a Slack identity; threading makes that liveable, not solved.Written for commit 55f8d85. Summary will update on new commits.