feat(canvas): add canvas create, edit, and delete commands by rianjs · Pull Request #137 · open-cli-collective/slack-chat-api

rianjs · 2026-03-28T20:29:41Z

Summary

New slck canvas command group (cv alias) for managing Slack Canvases
canvas create — standalone (--title required) or channel canvases (--channel), content via --text, --file, or stdin (--file -)
canvas edit <canvas-id> — replace canvas content
canvas delete <canvas-id> — delete a canvas
Client methods for canvases.create, conversations.canvases.create, canvases.edit, canvases.delete

Closes #116

Test plan

Client tests: CreateCanvas, CreateChannelCanvas, EditCanvas, DeleteCanvas
Command tests: resolveContent (text, stdin, file, error cases), runCreate validation
make build, make test, make lint all pass

Adds a new `slck canvas` command group for managing Slack Canvases: - `canvas create` — standalone or channel canvases from --text, --file, or stdin - `canvas edit` — replace canvas content - `canvas delete` — delete a canvas Client methods map to canvases.create, conversations.canvases.create, canvases.edit, and canvases.delete Slack API endpoints. Closes #116

rianjs · 2026-03-28T20:30:52Z

TDD Assessment: canvas command group

Summary

Coverage is functional but has several gaps worth addressing before merge.

Client tests (`internal/client/client_test.go`)

All 4 methods have happy-path tests. Missing:

No API error tests ("ok": false) for any of the 4 new methods. Every other command in the codebase that posts has a matching _APIError test (e.g. TestClient_SearchMessages_APIError). The canvas methods skip this entirely.
No request body validation in any of the 4 new client tests. The tests only check the URL path. Other tests in the file decode r.Body and assert fields like canvas_id, document_content.type, changes[0].operation, etc. For EditCanvas in particular, the nested changes array structure is non-trivial and worth asserting.

Command tests (`internal/cmd/canvas/canvas_test.go`)

resolveContent is well-tested. Gaps:

Bug: editOptions has no stdin field. runEdit calls resolveContent(opts.text, opts.file, nil) — the third argument is hardcoded nil, not a field on editOptions. The --file - (stdin) path shown in the long description is broken for edit: stdin will fall through to os.Stdin at runtime but is untestable and untested. createOptions has the stdin io.Reader field; editOptions does not.
No runEdit tests at all. There are 2 tests for runCreate but zero for runEdit or runDelete. Missing cases:
- runEdit with no content should error
- runEdit with a mock client (success path)
- runDelete with a mock client (success path)
TestRunCreate_StandaloneNoTitle tests the wrong thing. The comment says it exercises title validation, but the function returns early on client.New() failure before reaching the title check. The if opts.title == "" guard is never reached. A proper test would inject a mock client and assert the --title is required error.
No runCreate test for the channel canvas path. The if opts.channel != "" branch (which calls ResolveChannel then CreateChannelCanvas) has no test coverage.
No JSON output tests. The output.IsJSON() branches exist in all three commands but are not exercised anywhere.

Minor

TestResolveContent_Empty is valid at the resolveContent level, but both callers treat the empty-string return as an error. A comment explaining the caller-level guard would reduce confusion.

monit-reviewer

Automated PR Review

Reviewed commit: f5ea9ab

Summary

Reviewer	Findings
harness-engineering:architecture-reviewer	2
harness-engineering:enforcement-reviewer	2
harness-engineering:legibility-reviewer	1
security:code-auditor	1

harness-engineering:architecture-reviewer (2 findings)

⚠️ Should Fix - internal/cmd/canvas/edit.go:12

editOptions lacks a stdin io.Reader field, unlike createOptions which has one for testing. runEdit passes nil to resolveContent for stdin (line 41), so --file - works at runtime by falling back to os.Stdin, but this code path cannot be unit tested. Add a stdin field to editOptions and wire it through the RunE closure, matching the create command pattern.

💡 Suggestion - internal/cmd/canvas/canvas_test.go:1

No command-level tests exist for runEdit or runDelete, and runCreate only covers validation failures — no success-path tests with a mock client. The resolveContent and client-layer tests are solid, but adding mock-client command tests (following the pattern in CLAUDE.md) would close the gap between the test plan and actual coverage.

harness-engineering:enforcement-reviewer (2 findings)

⚠️ Should Fix - internal/cmd/canvas/create.go:83

Title validation for standalone canvases happens after client.New() (line 60). A user running slck canvas create --text foo without --title and without a valid token gets a confusing token error instead of the helpful --title is required message. Move the standalone title check before client initialization. This is also the root cause of TestRunCreate_StandaloneNoTitle (canvas_test.go:78) not actually testing title validation — the test comment acknowledges client.New() fails first, leaving the title guard at line 83 completely untested.

💡 Suggestion - internal/cmd/canvas/create.go:67

When --channel is provided alongside --title, the title is silently ignored since channel canvases don't use it. Consider either warning the user or noting in the --title help text that it only applies to standalone canvases.

harness-engineering:legibility-reviewer (1 findings)

💡 Suggestion - internal/cmd/canvas/edit.go:46

Error message says content required: use --text or --file but the command help text documents stdin support via --file -. For consistency with create.go:57, update to: content required: use --text, --file, or --file -.

security:code-auditor (1 findings)

💡 Suggestion - internal/cmd/canvas/create.go:123

readStdin uses bufio.Scanner which has a default 64KB max token (line) size. Markdown content with a single line exceeding 64KB (e.g. base64-encoded images) will error with bufio.Scanner: token too long. Consider using io.ReadAll(r) instead — it's simpler and has no per-line size limit.

Completed in 7m 18s | $0.99

Field	Value
Reviewers	hybrid-synthesis, database:sql-reviewer, security:code-auditor, harness-engineering:knowledge-reviewer, harness-engineering:enforcement-reviewer, harness-engineering:architecture-reviewer, harness-engineering:legibility-reviewer
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	7m 18s (Reviewers: 2m 12s · Synthesis: 1m 57s)
Cost	$0.99
Tokens	374.7k in / 31.5k out
Turns	30

monit-reviewer · 2026-03-28T20:34:11Z

internal/cmd/canvas/create.go

+		return nil
+	}
+
+	if opts.title == "" {


🟡 Medium (harness-engineering:enforcement-reviewer): Title validation for standalone canvases happens after client.New() (line 60). A user running slck canvas create --text foo without --title and without a valid token gets a confusing token error instead of the helpful --title is required message. Move the standalone title check before client initialization. This is also the root cause of TestRunCreate_StandaloneNoTitle (canvas_test.go:78) not actually testing title validation — the test comment acknowledges client.New() fails first, leaving the title guard at line 83 completely untested.

Fixed — title validation moved before client.New(). TestRunCreate_StandaloneNoTitle now properly tests this path.

monit-reviewer · 2026-03-28T20:34:11Z

internal/cmd/canvas/edit.go

+	"github.com/open-cli-collective/slack-chat-api/internal/output"
+)
+
+type editOptions struct {


🟡 Medium (harness-engineering:architecture-reviewer): editOptions lacks a stdin io.Reader field, unlike createOptions which has one for testing. runEdit passes nil to resolveContent for stdin (line 41), so --file - works at runtime by falling back to os.Stdin, but this code path cannot be unit tested. Add a stdin field to editOptions and wire it through the RunE closure, matching the create command pattern.

Fixed — added stdin field to editOptions. TestRunEdit_Stdin covers this path.

monit-reviewer · 2026-03-28T20:34:11Z

internal/cmd/canvas/create.go

+	return "", nil
+}
+
+func readStdin(r io.Reader) (string, error) {


🔵 Low (security:code-auditor): readStdin uses bufio.Scanner which has a default 64KB max token (line) size. Markdown content with a single line exceeding 64KB (e.g. base64-encoded images) will error with bufio.Scanner: token too long. Consider using io.ReadAll(r) instead — it's simpler and has no per-line size limit.

Fixed — replaced bufio.Scanner with io.ReadAll. Simpler and no per-line size limit.

monit-reviewer · 2026-03-28T20:34:11Z

internal/cmd/canvas/edit.go

+		return err
+	}
+	if markdown == "" {
+		return fmt.Errorf("content required: use --text or --file")


🔵 Low (harness-engineering:legibility-reviewer): Error message says content required: use --text or --file but the command help text documents stdin support via --file -. For consistency with create.go:57, update to: content required: use --text, --file, or --file -.

Fixed — error message now says: content required: use --text, --file, or --file -

monit-reviewer · 2026-03-28T20:34:11Z

internal/cmd/canvas/create.go

+		}
+	}
+
+	if opts.channel != "" {


🔵 Low (harness-engineering:enforcement-reviewer): When --channel is provided alongside --title, the title is silently ignored since channel canvases don't use it. Consider either warning the user or noting in the --title help text that it only applies to standalone canvases.

Fixed — now returns an error if --title and --channel are both set. TestRunCreate_TitleWithChannel covers this.

monit-reviewer · 2026-03-28T20:34:11Z

internal/cmd/canvas/canvas_test.go

@@ -0,0 +1,91 @@
+package canvas


🔵 Low (harness-engineering:architecture-reviewer): No command-level tests exist for runEdit or runDelete, and runCreate only covers validation failures — no success-path tests with a mock client. The resolveContent and client-layer tests are solid, but adding mock-client command tests (following the pattern in CLAUDE.md) would close the gap between the test plan and actual coverage.

Fixed — added mock-client success tests for create (standalone + channel), edit (text + stdin), and delete.

- Move title validation before client.New() for clear error messages - Error when --title used with --channel (mutually exclusive) - Add stdin field to editOptions for testability - Replace bufio.Scanner with io.ReadAll (no 64KB line limit) - Consistent error messages mentioning --file - for stdin - Add mock-client success tests for create, edit, delete commands

rianjs · 2026-03-28T20:42:15Z

All 6 review findings addressed in f0af8e6. Please approve the PR.

monit-reviewer

Automated PR Review

Reviewed commit: f0af8e6

Human Override

Approved at the request of @rianjs (comment).

All 6 review findings addressed in f0af8e6. Please approve the PR.

No automated reviewers were run.

- README: add canvas command section, --since flag on thread, canvas alias - CHANGELOG: add entries for canvas, --since, edited metadata, truncation fix - integration-tests: add canvas tests (Part 10), thread --since and [edited] indicator verification to existing sections

…138) ## Summary - **README**: Added canvas command section, `--since` flag on thread, canvas alias in aliases table - **CHANGELOG**: Added entries for canvas (#137), `--since` (#135), edited metadata (#136), truncation fix (#134) - **integration-tests.md**: Added Part 10 (Canvas Tests), enhanced Part 3.4 with `--since` and full-text verification, enhanced Part 3.5 with `[edited]` indicator checks ## Test plan - [x] Docs-only change, no code modified

monit-reviewer suggested changes Mar 28, 2026

View reviewed changes

monit-reviewer approved these changes Mar 28, 2026

View reviewed changes

rianjs merged commit ed6c21a into main Mar 28, 2026
2 checks passed

rianjs deleted the feat/116-canvas-support branch March 28, 2026 20:42

rianjs mentioned this pull request Mar 28, 2026

docs: update README, CHANGELOG, and integration tests for #134-#137 #138

Merged

1 task

Conversation

rianjs commented Mar 28, 2026

Summary

Test plan

Uh oh!

rianjs commented Mar 28, 2026

TDD Assessment: canvas command group

Summary

Client tests (internal/client/client_test.go)

Command tests (internal/cmd/canvas/canvas_test.go)

Minor

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rianjs commented Mar 28, 2026

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Human Override

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Client tests (`internal/client/client_test.go`)

Command tests (`internal/cmd/canvas/canvas_test.go`)