Skip to content

feat: enrich slack create response metadata#160

Open
i-am-thor[bot] wants to merge 17 commits into
mainfrom
feat/enrich-created-resource-responses
Open

feat: enrich slack create response metadata#160
i-am-thor[bot] wants to merge 17 commits into
mainfrom
feat/enrich-created-resource-responses

Conversation

@i-am-thor
Copy link
Copy Markdown
Contributor

@i-am-thor i-am-thor Bot commented May 27, 2026

Summary

  • return normalized identity and continuation metadata from
  • return file identity and known locators from
  • align tests, e2e expectations, and Slack docs/plan with the created-resource response contract

Testing

  • pnpm test -- packages/remote-cli/src/slack-post-message.test.ts packages/remote-cli/src/mcp-handler.test.ts
  • pnpm --filter @thor/remote-cli typecheck
  • Core E2E and Unit Tests GitHub checks on the pushed branch

AI-generated — verify before acting. View Thor context

Co-authored-by: Son Dao <son.dao@katalon.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR enriches Slack create-operation responses so agents receive normalized identity and continuation metadata after posting messages or uploading files.

Changes:

  • Adds normalized Slack message response metadata for slack-post-message.
  • Expands slack-upload success output with file identity and known locators.
  • Updates tests, e2e assertions, and Slack usage documentation for the new response contract.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/remote-cli/src/slack-post-message.ts Builds and returns normalized created-message metadata.
packages/remote-cli/src/slack-post-message.test.ts Updates expectations and adds fallback-channel coverage.
docker/opencode/bin/slack-upload Emits normalized file identity and continuation metadata.
scripts/test-e2e.sh Validates enriched slack-upload output in e2e flow.
docker/opencode/config/skills/slack/SKILL.md Documents response handling for Slack create helpers.
docs/plan/2026050501_slack-post-message.md Updates the implementation plan and decision log for enriched responses.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 77 to 79
4. remote-cli appends the alias via `appendCorrelationAlias(sessionId, correlationKey)` when a Thor session ID is present, where `correlationKey` is `slack:thread:${aliasTs}`:
- `aliasTs = thread_ts` for replies
- `aliasTs = response.ts` for new top-level messages
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I updated the plan wording to describe the channel-qualified primary key (slack:thread:<channel>/<ts>) plus the legacy fallback and pushed 44e2fe0.


AI-generated — verify before acting. View Thor context

| D6 | Register aliases in remote-cli after Slack `ok: true` without mutating stdout | remote-cli has the session/call IDs and existing alias writer. Keeping raw Slack JSON on stdout preserves agent/API expectations and avoids response rewriting. |
| D6 | Register aliases in remote-cli after Slack `ok: true` without making aliasing part of the visible response contract | remote-cli has the session/call IDs and existing alias writer. Alias registration should remain a side effect of a successful Slack write; visible stdout is governed by the created-resource response contract in D20. |
| D7 | Treat alias registration failure as logged side-effect failure after a successful Slack post | Retrying a successful post solely because bookkeeping failed can duplicate user-visible Slack messages. Operators need logs, but users should not get duplicate replies. |
| D8 | Keep channel ID out of alias storage in this branch | Existing resolver keys are `slack:thread:{ts}`. Redesigning alias metadata would widen the feature; the CLI can require `--channel` explicitly for now. |
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated. I revised the plan/decision-log wording to reflect the current channel-qualified alias model with ts-only fallback for back-compat, and pushed 44e2fe0.


AI-generated — verify before acting. View Thor context

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.


### 2026-05-27 scope update — created resource response enrichment

This branch should also stop collapsing successful Slack create/write responses to `{"ok":true}` or a bare `{ ts }`. The controlled Slack paths now need to return enough structured identity and locator data for agents and follow-up Thor code to continue in the created resource without scraping logs or relying on hidden alias state.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks — I see the mismatch. This follow-up came from Copilot rather than the original requester, so I’m pausing before changing scope/code again. I’ll confirm with the original requester whether they want this branch to (a) include the approval-card helper too, or (b) narrow the plan text to the agent-facing commands only, then follow up here.


AI-generated — verify before acting. View Thor context

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. I updated the approval path so approval_required now includes the enriched Slack notification metadata (channel, threadTs, messageTs, continuation), the approval store keeps the same locator data, and the plan text now explicitly includes approval-card Slack posts. Pushed in 0c7f26d with targeted test coverage and remote-cli typecheck.


AI-generated — verify before acting. View Thor context

Co-authored-by: Son Dao <son.dao@katalon.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.

daohoangson and others added 7 commits May 27, 2026 23:39
Move slack-upload's logic out of an inline-node shell script into
packages/opencode-cli/src/slack-upload.ts so it goes through tsc and
zod-validates the Slack response shapes. The bin entry stays as a thin
wrapper exec'ing the bundled .mjs, matching the git/mcp pattern.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Drop fields callers can derive or don't act on: `continuation`, `ok`,
`channel`, and `message_ts` from `SlackCreatedMessageResponse`; the
event-payload `notification` from `approval_required`. Source
`thread_ts` from Slack's `message.thread_ts` so threaded replies
report the truth (and degrade to top-level if Slack downgrades the
post) instead of echoing the request.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Drop the bespoke output object (`ok`/`file_id`/`file`/`files`/`channel`/
`thread_ts`) and write Slack's `files.completeUploadExternal` response
to stdout verbatim. Removes the normalize/synthesis layer and the
echoed inputs the caller already knows. Update the e2e assertion to
read `files.0.id` from the upstream shape; the reply-fetching loop
right after still verifies thread placement.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Swap the hand-rolled fetch + per-field `as` casts in
`postSlackMessageApi` for `WebClient.chat.postMessage`. Inject the
client as a dep so tests can mock `{ chat: { postMessage } }` directly
instead of stubbing Slack HTTP responses. Rename mcp-handler's
`fetchImpl` dep to `slackClient`. slack-upload stays on curl: it runs
behind mitmproxy which injects auth on the wire, and the SDK would
require a dummy token the CLI deliberately never holds.

Record D21–D24 in the plan covering the SDK switch, the response-shape
trim, the truthful `thread_ts` source, and the slack-upload passthrough.
Drop a redundant `slackPostMessage.toHaveBeenCalledTimes` assertion that
duplicated the persisted notification check.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@daohoangson
Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Copy link
Copy Markdown
Contributor

Copilot AI commented May 31, 2026

@copilot resolve the merge conflicts in this pull request

Resolved in fe17910.

Copilot AI requested a review from daohoangson May 31, 2026 14:53
@daohoangson
Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

…esource-responses

# Conflicts:
#	packages/remote-cli/src/mcp-handler.test.ts
Copy link
Copy Markdown
Contributor

Copilot AI commented Jun 2, 2026

@copilot resolve the merge conflicts in this pull request

Resolved in 83cad19.

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.

3 participants