Skip to content

feat(pins): add pins add, remove and list - #218

Merged
piekstra merged 2 commits into
mainfrom
piekstra/pins
Nov 3, 2026
Merged

feat(pins): add pins add, remove and list#218
piekstra merged 2 commits into
mainfrom
piekstra/pins

Conversation

@piekstra

@piekstra piekstra commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

Adds slck pins add, slck pins remove and slck pins list, over Slack's pins.add, pins.remove and pins.list. slck had no way to pin a message.

slck pins add <channel> <timestamp>
slck pins remove <channel> <timestamp>
slck pins list <channel>
  • The channel resolves as in the other message commands (ID, name, or a user ID or @handle for a DM). The timestamp takes the API form, the p-prefixed form or the message's Slack URL.
  • add on a pinned message (already_pinned) and remove on an unpinned one (no_pin or not_pinned) exit 0 and say so, as messages react and unreact do.
  • A missing_scope error names the scope the command needs, pins:write for add and remove and pins:read for list, and where to add it.
  • list prints TS | PINNED | PINNED_BY | AUTHOR | TEXT, most recently pinned first; TS is what remove takes.
  • Runtime errors print the error and its hint once, without the usage text, as the slck apps commands do. The helper that does this moves from apps to internal/cmd/cmdutil, so both packages share it, and docs/development.md tells new commands to use it.
  • --as-user and --as-bot pick the token as on every other command.
  • The README, integration-tests.md and the extended manifest (which gains pins:write) are updated.

Verification

  • make test and make lint pass. The tests cover each command's request, the already-pinned and not-pinned outcomes, the missing-scope hints, and the list rendering, against fixture payloads.
  • Read-only against a real workspace: pins list --as-user with a user token that lacks pins:read printed the missing-scope hint and exited 1; pins list --as-bot with a bot token that has it printed No pins in <channel>.

Model: Claude Opus 5.5 (1M context, effort unknown) in Claude Code.

@piekstra
feat(pins): add pins add, remove and list
fa40e62 
slck can now pin and unpin a message and list what is pinned in a
channel, through pins.add, pins.remove and pins.list. Pinning an
already pinned message, or unpinning one that is not pinned, succeeds
and says so. A missing_scope error names the scope the command needs
(pins:write or pins:read). The extended manifest gains pins:write.
monit-reviewer
monit-reviewer approved these changes Nov 3, 2026
View reviewed changes

@monit-reviewer monit-reviewer 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.

Automated PR Review

Reviewed commit: fa40e6210ae6
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer Findings
go:implementation-tests 0
harness-engineering:repo-health 1
policies:conventions 0
documentation:docs 0
architecture:solid-reviewer-agnostic 1
security:code-auditor 0
harness-engineering:repo-health (1 finding)

Nits - internal/cmd/pins/pins.go:35

quiet() is a verbatim copy of the helper in internal/cmd/apps/apps.go (same body, slightly different doc comment). The PR describes "print the runtime error and hint once, without usage" as a convention shared with slck apps, so it is becoming a cross-package pattern, but each new command package now copies it locally and the two comments already disagree on the reason (apps mentions the silent exitError). The next command to adopt the behavior will copy it a third time. Suggested fix: move it into one shared place (for example a small exported helper beside the root command, or set SilenceUsage/SilenceErrors in a PersistentPreRun on the subcommands that opt in) and call it from both apps and pins. Also add a line to the Command Patterns section of docs/development.md so future agents use it instead of copying it.

architecture:solid-reviewer-agnostic (1 finding)

Minor - internal/client/errors.go:64

U-L2 (Errors are contracts): the new exported WrapScopeError decides behaviour by substring-matching the error message (IsSlackError → strings.Contains(err.Error(), code)). The new AddPin/RemovePin in internal/client/pins.go use the same check for already_pinned/no_pin/not_pinned. The generic Slack path in client.go already returns fmt.Errorf("slack API error: %s"), so this follows existing practice, and the codes cannot collide today. Still, it adds three more call sites and one more public helper built on message matching, while the apps path in the same package already has a typed AppsAPIError{Code} that apps.wrap matches with errors.As. This is non-blocking because it copies an existing pattern. A follow-up (or this PR, if cheap) could give post/get a typed *SlackAPIError{Code string} and have IsSlackError and WrapScopeError use errors.As and compare Code exactly. Everything that calls IsSlackError would then use the typed check without changes.

Reviewer Coverage

Complete coverage details are retained in the local coverage.json artifact.

  • go:implementation-tests — complete (broad); scope: 19 files; examples: internal/client/errors.go, internal/client/errors_test.go, internal/client/pins.go, internal/cmd/pins/add.go, internal/cmd/pins/list.go; 14 omitted; body-inspected: 19 files; examples: internal/client/errors.go, internal/client/errors_test.go, internal/client/pins.go, internal/cmd/pins/add.go, internal/cmd/pins/list.go; 14 omitted; skipped: none; context (not coverage): 5 files; examples: internal/client/channel_resolver.go, internal/client/client.go, internal/cmd/apps/apps.go, internal/cmd/messages/react.go, internal/output/output.go; constraints: Lack of context.Context propagation and substring-based IsSlackError matching are pre-existing client patterns that the new code follows; not flagged as diff-introduced drift. Static review only; tests were not executed.
  • harness-engineering:repo-health — complete (constrained); scope: 19 files; examples: internal/client/errors.go, internal/client/errors_test.go, internal/client/pins.go, internal/cmd/pins/add.go, internal/cmd/pins/list.go; 14 omitted; body-inspected: 19 files; examples: internal/client/errors.go, internal/client/errors_test.go, internal/client/pins.go, internal/cmd/pins/add.go, internal/cmd/pins/list.go; 14 omitted; skipped: none; context (not coverage): 7 files; examples: ARCHITECTURE.md, CLAUDE.md, README.md, docs/development.md, integration-tests.md; 2 omitted; constraints: Test fixture JSON bodies were not read one by one. Their behavior was checked through the assertions in pins_test.go. Tests and lint were not run. The review was a static read only. Tests were not run. The fixtures were checked only by reading them, comparing them with the assertions in pins_test.go and with the existing fixture layout in internal/cmd/apps/testdata.
  • policies:conventions — complete (broad); scope: 8 files; examples: README.md, docs/development.md, internal/client/errors.go, internal/cmd/pins/add.go, internal/cmd/pins/list.go; 3 omitted; body-inspected: 8 files; examples: README.md, docs/development.md, internal/client/errors.go, internal/cmd/pins/add.go, internal/cmd/pins/list.go; 3 omitted; skipped: none; context (not coverage): 5 files; examples: AGENTS.md, CLAUDE.md, internal/cmd/apps/apps.go, internal/cmd/search/render.go, internal/output/output.go; constraints: Shared cli-common docs (../cli-common/docs) were not available locally; review used repo-local docs/development.md, README conventions, and existing command patterns (apps quiet(), search SearchTable, text/table-only output contract).
  • documentation:docs — complete (broad); scope: 3 files; examples: README.md, docs/development.md, integration-tests.md; body-inspected: 3 files; examples: README.md, docs/development.md, integration-tests.md; skipped: none; context (not coverage): 5 files; examples: internal/cmd/pins/add.go, internal/cmd/pins/list.go, internal/cmd/pins/pins.go, internal/cmd/pins/remove.go, internal/render/render.go; constraints: Docs checked against the pins command source (aliases, output strings, list columns, file-pin dash handling, scope requirements); the commands were not run against a live workspace.
  • architecture:solid-reviewer-agnostic — complete (broad); scope: 7 files; examples: internal/client/errors.go, internal/client/pins.go, internal/cmd/pins/add.go, internal/cmd/pins/list.go, internal/cmd/pins/pins.go; 2 omitted; body-inspected: 7 files; examples: internal/client/errors.go, internal/client/pins.go, internal/cmd/pins/add.go, internal/cmd/pins/list.go, internal/cmd/pins/pins.go; 2 omitted; skipped: none; context (not coverage): 5 files; examples: ARCHITECTURE.md, docs/development.md, internal/client/client.go, internal/cmd/apps/apps.go, internal/cmd/messages/react.go; constraints: Ran go test ./internal/cmd/pins/ ./internal/client/ (both pass); did not run make lint. Shared cli-common standards referenced by docs/development.md were not available locally.
  • security:code-auditor — complete (constrained); scope: 6 files; examples: internal/client/errors.go, internal/client/pins.go, internal/cmd/pins/add.go, internal/cmd/pins/list.go, internal/cmd/pins/pins.go; 1 omitted; body-inspected: 6 files; examples: internal/client/errors.go, internal/client/pins.go, internal/cmd/pins/add.go, internal/cmd/pins/list.go, internal/cmd/pins/pins.go; 1 omitted; skipped: none; context (not coverage): 3 files; examples: internal/cmd/root/root.go, internal/output/output.go, internal/validate/validate.go; constraints: Static review only; tests were not executed. Checked that token selection uses the shared root --as-user/--as-bot PersistentPreRunE, that timestamps are regex-validated before any API call, and that Slack-sourced table cells pass through sanitizeSearchCell.

0 PR discussion threads considered. 0 summarized; 0 resolved.

Completed in 2m 59s | ~$2.30 (est.) | claude-opus-5-5 | cr 0.10.354
Field Value
Model claude-opus-5-5
Reviewers go:implementation-tests, harness-engineering:repo-health, policies:conventions, documentation:docs, architecture:solid-reviewer-agnostic, security:code-auditor
Engine claude_cli · claude-opus-5-5
Reviewed by cr · monit-reviewer
Duration 2m 59s wall · 6m 36s compute
Cost ~$2.30 (est.)
Pricing basis baseline-2026-10-02/pricing
Tokens 110 in / 22.3k out

Per-workstream usage

  • orchestrator-selection — claude-opus-5-5
    • In: 6
    • Out: 2.7k
    • Cache read: 25.2k
    • Cache create: 14.5k
    • Cost: ~$0.18 (est.)
    • Duration: 27s
  • go:implementation-tests — claude-opus-5-5
    • In: 12
    • Out: 3.0k
    • Cache read: 114.5k
    • Cache create: 29.0k
    • Cost: ~$0.31 (est.)
    • Duration: 54s
  • harness-engineering:repo-health — claude-opus-5-5
    • In: 26
    • Out: 4.6k
    • Cache read: 371.2k
    • Cache create: 41.3k
    • Cost: ~$0.50 (est.)
    • Duration: 1m 17s
  • policies:conventions — claude-opus-5-5
    • In: 16
    • Out: 2.9k
    • Cache read: 154.0k
    • Cache create: 27.1k
    • Cost: ~$0.31 (est.)
    • Duration: 58s
  • documentation:docs — claude-opus-5-5
    • In: 18
    • Out: 2.8k
    • Cache read: 170.2k
    • Cache create: 23.7k
    • Cost: ~$0.28 (est.)
    • Duration: 58s
  • architecture:solid-reviewer-agnostic — claude-opus-5-5
    • In: 16
    • Out: 3.8k
    • Cache read: 206.6k
    • Cache create: 36.7k
    • Cost: ~$0.41 (est.)
    • Duration: 1m 03s
  • security:code-auditor — claude-opus-5-5
    • In: 10
    • Out: 1.6k
    • Cache read: 81.9k
    • Cache create: 25.1k
    • Cost: ~$0.25 (est.)
    • Duration: 38s
  • orchestrator-rollup — claude-opus-5-5
    • In: 6
    • Out: 944
    • Cache read: 71.8k
    • Cache create: 4.7k
    • Cost: ~$0.07 (est.)
    • Duration: 17s

Comment thread internal/client/errors.go Resolved
Comment thread internal/cmd/pins/pins.go Resolved
@piekstra
refactor(cmd): share the quiet helper between apps and pins
114e9ea 
apps and pins each had their own copy of the helper that stops cobra
printing a runtime error twice with the usage text. It now lives in
internal/cmd/cmdutil, and docs/development.md tells new commands to use
it.
monit-reviewer
monit-reviewer approved these changes Nov 3, 2026
View reviewed changes

@monit-reviewer monit-reviewer 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.

Automated PR Review

Reviewed commit: 114e9ead1543
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer Findings
go:implementation-tests 0
harness-engineering:repo-health 0
policies:conventions 0
documentation:docs 0
architecture:solid-reviewer-agnostic 0
security:code-auditor 0

Reviewer Coverage

Complete coverage details are retained in the local coverage.json artifact.

  • go:implementation-tests — complete (constrained); scope: 21 files; examples: internal/client/errors.go, internal/client/errors_test.go, internal/client/pins.go, internal/cmd/apps/apps.go, internal/cmd/cmdutil/cmdutil.go; 16 omitted; body-inspected: 21 files; examples: internal/client/errors.go, internal/client/errors_test.go, internal/client/pins.go, internal/cmd/apps/apps.go, internal/cmd/cmdutil/cmdutil.go; 16 omitted; skipped: none; context (not coverage): 5 files; examples: docs/development.md, internal/client/channel_resolver.go, internal/client/client.go, internal/cmd/messages/react.go, internal/output/output.go; constraints: Static review only; tests were not executed. Substring IsSlackError matching and missing context.Context are pre-existing client patterns; the typed-error migration was settled in discussion as a follow-up. The 114e9ea delta is a mechanical move of quiet() into cmdutil.Quiet; pins behavior is still covered by TestRuntimeErrorsPrintNoUsage.
  • harness-engineering:repo-health — complete (constrained); scope: 19 files; examples: internal/client/errors.go, internal/client/errors_test.go, internal/client/pins.go, internal/cmd/pins/add.go, internal/cmd/pins/list.go; 14 omitted; body-inspected: 19 files; examples: internal/client/errors.go, internal/client/errors_test.go, internal/client/pins.go, internal/cmd/pins/add.go, internal/cmd/pins/list.go; 14 omitted; skipped: none; context (not coverage): 5 files; examples: ARCHITECTURE.md, CLAUDE.md, docs/development.md, internal/cmd/apps/apps.go, internal/cmd/cmdutil/cmdutil.go; constraints: Tests and lint were not run. The review was a static read only. The substring-matching IsSlackError thread is settled and deferred to a follow-up, so it is not raised again. This pass also checked commit 114e9ea against the earlier finding about the duplicated quiet() helper. The helper is now consolidated in internal/cmd/cmdutil and documented in docs/development.md, so the finding is not raised again.
  • policies:conventions — complete (constrained); scope: 8 files; examples: README.md, docs/development.md, internal/client/errors.go, internal/cmd/pins/add.go, internal/cmd/pins/list.go; 3 omitted; body-inspected: 8 files; examples: README.md, docs/development.md, internal/client/errors.go, internal/cmd/pins/add.go, internal/cmd/pins/list.go; 3 omitted; skipped: none; context (not coverage): 4 files; examples: AGENTS.md, CLAUDE.md, internal/cmd/apps/apps.go, internal/cmd/cmdutil/cmdutil.go; constraints: Incremental commit 114e9ea (shared cmdutil.Quiet, documented in Command Patterns) checked; root exitCode prints silenced errors once, consistent with the new doc rule. Shared cli-common docs (../cli-common/docs) were not available locally; review relied on repo-local docs/development.md, README conventions, and existing command patterns.
  • documentation:docs — complete (constrained); scope: 3 files; examples: README.md, docs/development.md, integration-tests.md; body-inspected: 3 files; examples: README.md, docs/development.md, integration-tests.md; skipped: none; context (not coverage): 7 files; examples: internal/cmd/cmdutil/cmdutil.go, internal/cmd/pins/add.go, internal/cmd/pins/list.go, internal/cmd/pins/pins.go, internal/cmd/pins/remove.go; 2 omitted; constraints: Docs checked against the pins command source, cmdutil.Quiet and root error printing; the commands were not run against a live workspace.
  • architecture:solid-reviewer-agnostic — complete (constrained); scope: 7 files; examples: internal/client/errors.go, internal/client/pins.go, internal/cmd/pins/add.go, internal/cmd/pins/list.go, internal/cmd/pins/pins.go; 2 omitted; body-inspected: 7 files; examples: internal/client/errors.go, internal/client/pins.go, internal/cmd/pins/add.go, internal/cmd/pins/list.go, internal/cmd/pins/pins.go; 2 omitted; skipped: none; context (not coverage): 5 files; examples: docs/development.md, internal/client/client.go, internal/cmd/apps/apps.go, internal/cmd/cmdutil/cmdutil.go, internal/cmd/messages/react.go; constraints: Prior threads settled: substring-based IsSlackError matching is deferred to a typed-error follow-up; duplicated quiet() was consolidated into internal/cmd/cmdutil.Quiet (outside assigned scope). Neither is re-raised. Ran go test ./internal/cmd/... ./internal/client/ at 114e9ea (all pass); did not run make lint.
  • security:code-auditor — complete (constrained); scope: 6 files; examples: internal/client/errors.go, internal/client/pins.go, internal/cmd/pins/add.go, internal/cmd/pins/list.go, internal/cmd/pins/pins.go; 1 omitted; body-inspected: 6 files; examples: internal/client/errors.go, internal/client/pins.go, internal/cmd/pins/add.go, internal/cmd/pins/list.go, internal/cmd/pins/pins.go; 1 omitted; skipped: none; context (not coverage): 4 files; examples: internal/cmd/cmdutil/cmdutil.go, internal/cmd/root/root.go, internal/output/output.go, internal/validate/validate.go; constraints: Static review only; tests not executed. Delta since fa40e62 is the quiet()->cmdutil.Quiet refactor with no security impact. Token selection uses the shared root --as-user/--as-bot handling, timestamps are regex-validated before API calls, and pins list cells pass through sanitizeSearchCell.

0 PR discussion threads considered. 0 summarized; 0 resolved.

Completed in 2m 17s | ~$0.99 (est.) | claude-opus-5-5 | cr 0.10.354
Field Value
Model claude-opus-5-5
Reviewers go:implementation-tests, harness-engineering:repo-health, policies:conventions, documentation:docs, architecture:solid-reviewer-agnostic, security:code-auditor
Engine claude_cli · claude-opus-5-5
Reviewed by cr · monit-reviewer
Duration 2m 17s wall · 5m 55s compute
Cost ~$0.99 (est.)
Pricing basis baseline-2026-10-02/pricing
Tokens 58 in / 10.8k out

Per-workstream usage

  • go:implementation-tests — claude-opus-5-5
    • In: 6
    • Out: 1.4k
    • Cache read: 107.0k
    • Cache create: 8.6k
    • Cost: ~$0.12 (est.)
    • Duration: 48s
  • harness-engineering:repo-health — claude-opus-5-5
    • In: 6
    • Out: 1.7k
    • Cache read: 143.1k
    • Cache create: 9.0k
    • Cost: ~$0.13 (est.)
    • Duration: 45s
  • policies:conventions — claude-opus-5-5
    • In: 10
    • Out: 2.0k
    • Cache read: 182.8k
    • Cache create: 10.5k
    • Cost: ~$0.16 (est.)
    • Duration: 52s
  • documentation:docs — claude-opus-5-5
    • In: 12
    • Out: 1.8k
    • Cache read: 201.2k
    • Cache create: 11.6k
    • Cost: ~$0.17 (est.)
    • Duration: 1m 11s
  • architecture:solid-reviewer-agnostic — claude-opus-5-5
    • In: 10
    • Out: 2.1k
    • Cache read: 230.9k
    • Cache create: 11.1k
    • Cost: ~$0.18 (est.)
    • Duration: 1m 13s
  • security:code-auditor — claude-opus-5-5
    • In: 10
    • Out: 1.5k
    • Cache read: 183.8k
    • Cache create: 15.3k
    • Cost: ~$0.19 (est.)
    • Duration: 51s
  • orchestrator-rollup — claude-opus-5-5
    • In: 4
    • Out: 422
    • Cache read: 54.7k
    • Cache create: 2.8k
    • Cost: ~$0.04 (est.)
    • Duration: 12s

@piekstra
piekstra marked this pull request as ready for review November 3, 2026 13:44
@piekstra
piekstra merged commit 2993028 into main Nov 3, 2026
9 checks passed
@piekstra
piekstra deleted the piekstra/pins branch November 3, 2026 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@monit-reviewer monit-reviewer monit-reviewer approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@piekstra @monit-reviewer