Repository navigation
feat(apps): manage Slack apps through their manifests - #210
Conversation
Add `slck apps` with get, validate, diff, create, update and delete on
the apps.manifest.* methods, authenticated with an app configuration
token pair stored under a new keyring key, app_config_token.
The pair enters through `slck set-credential --key app_config_token
--stdin` as {"token", "refresh_token", "exp"} JSON. slck rotates it with
tooling.tokens.rotate when the access token is within five minutes of
expiry or Slack rejects it, saving the new pair before using it, and
`config show` reports its expiry and team without the tokens.
`apps diff` exits 0 when the live manifest matches the file, 1 when it
differs and 2 when it could not compare; the root command now honors an
exit code an error carries. `apps create` never prints the new app's
secrets and writes them only to a new 0600 file when asked.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: ef7e0bf0d31b
Profile: claude-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| harness-engineering:repo-health | 1 |
| policies:conventions | 0 |
| security:code-auditor | 0 |
| architecture:solid-reviewer-agnostic | 3 |
go:implementation-tests (1 finding)
Minor - internal/cmd/apps/apps.go:264
runCreate's cleanup-on-failure branch (close and os.Remove(credentialsFile) when CreateApp fails after the file was already opened) is only reachable when the create call errors post-open, but apps_test.go never exercises that path: TestCreateRefusesAnExistingCredentialsFileBeforeCreating only covers the earlier os.OpenFile(O_EXCL) failure, and the other create tests all succeed. Add a case where newFakeSlack returns an error/invalid_manifest response for apps.manifest.create with credentialsFile set, asserting the file no longer exists afterward. Without it, a regression that drops the os.Remove or leaves a stale empty 0600 file on the API-failure path would pass silently.
harness-engineering:repo-health (1 finding)
Major - internal/cmd/apps/apps.go:105
docs/development.md states the project-wide contract: "Resource and mutation-success commands emit text or table output. JSON is reserved for local control-plane carve-outs, currently
slck config show --json."apps getnow prints indented JSON unconditionally ("printed as JSON whatever --output says"), a second JSON-always resource command that the documented carve-out list does not mention. A future agent reading ARCHITECTURE.md/development.md to decide whether a new command may default to JSON will be misled by a now-inaccurate list, and the undocumented precedent (resource output can be JSON when it's 'the document itself') is left for the next command to reinvent or ignore inconsistently. Fix: update the carve-out line in docs/development.md (and/or ARCHITECTURE.md) to nameapps getalongsideconfig show --json, with the rationale (manifest is the artifact, not a rendered view).
architecture:solid-reviewer-agnostic (3 findings)
Major - internal/cmd/apps/apps.go:180
U-L1 (documented contract):
apps diffdocuments "0 when they match, 1 when they differ, 2 when the comparison could not be made", but argument and flag errors never reach runDiff, so they take the root's default exit 1. Reproduced with a branch build:slck apps diff(no app id) exits 1, andslck apps diff A1 --bogusexits 1. A script that reads exit 1 as "drift detected" will treat a mistyped invocation as drift, and this is the case the 1/2 split exists to prevent. Since this is a new public exit-code contract that scripts will depend on, fix it now. On the diff command, wrap Args so it returnsusageOrFailure(cobra.ExactArgs(1)(cmd, args)), and setcmd.SetFlagErrorFunc(func(_ *cobra.Command, err error) error { return usageOrFailure(err) }). Add a test that runs the cobra command with no args and with an unknown flag and asserts exit code 2. Usage text still prints because quiet() runs only inside RunE.
Minor - internal/client/apps.go:45
U-L2 / naming of new public surface:
client.APIErroris the package's first typed Slack error, but only AppsClient returns it. The existingClientstill returnsfmt.Errorf("slack API error: %s")(client.go:171, 215), and callers branch on that withIsSlackErrorsubstring matching. The package-wide name suggests thaterrors.As(err, &*client.APIError)works for every Slack call. It silently returns false for every bot or user client call, which is the kind of mismatch the next contributor will hit. Either rename it to the scope it covers (e.g.ManifestAPIError) or add a doc line saying only AppsClient returns it today. The better follow-up is to have Client return it too, soIsSlackErrorand the string-matchingWrapErrorcan move toerrors.As.
Nits - internal/cmd/apps/apps.go:39
U-S1 (naming):
usageOrFailurebuilds the code-2 error that means "could not compare" for diff. Every call site passes a runtime failure (a missing file, an export error, a decode error), not a usage error. A name likecannotCompare(err)would match the documented meaning and keep later commands from reusing it for something else.
Reviewer Coverage
go:implementation-tests— complete (broad); inspected 15 assigned files (27 inspected across reviewers):internal/apptoken/apptoken.go,internal/apptoken/apptoken_test.go,internal/client/apps.go,internal/cmd/apps/apps.go,internal/cmd/apps/apps_test.go,internal/cmd/apps/diff.go,internal/cmd/apps/manifest.go,internal/cmd/apps/session.go,internal/cmd/config/show.go,internal/cmd/root/root.go,internal/cmd/root/root_test.go,internal/cmd/setcred/setcred.go,internal/cmd/setcred/setcred_test.go,internal/keychain/keychain.go,internal/noleak/noleak_test.go; skipped: none; constraints: noneharness-engineering:repo-health— complete (constrained); inspected 25 assigned files (27 inspected across reviewers):internal/apptoken/apptoken.go,internal/apptoken/apptoken_test.go,internal/client/apps.go,internal/cmd/apps/apps.go,internal/cmd/apps/apps_test.go,internal/cmd/apps/diff.go,internal/cmd/apps/manifest.go,internal/cmd/apps/session.go,internal/cmd/apps/testdata/create-response.json,internal/cmd/apps/testdata/invalid-manifest-response.json,internal/cmd/apps/testdata/manifest-export.json,internal/cmd/apps/testdata/manifest-more-scopes.json,internal/cmd/apps/testdata/manifest.json,internal/cmd/apps/testdata/manifest.yaml,internal/cmd/apps/testdata/ok-response.json,internal/cmd/apps/testdata/rotate-response.json,internal/cmd/apps/testdata/token-expired-response.json,internal/cmd/apps/testdata/update-response.json,internal/cmd/config/show.go,internal/cmd/root/root.go,internal/cmd/root/root_test.go,internal/cmd/setcred/setcred.go,internal/cmd/setcred/setcred_test.go,internal/keychain/keychain.go,internal/noleak/noleak_test.go; skipped: none; constraints: nonepolicies:conventions— complete (broad); inspected 11 assigned files (27 inspected across reviewers):README.md,internal/cmd/apps/apps.go,internal/cmd/apps/apps_test.go,internal/cmd/apps/diff.go,internal/cmd/apps/manifest.go,internal/cmd/apps/session.go,internal/cmd/config/show.go,internal/cmd/root/root.go,internal/cmd/root/root_test.go,internal/cmd/setcred/setcred.go,internal/cmd/setcred/setcred_test.go; skipped: none; constraints: No sibling ../cli-common/docs or ../.github convenience copies are checked out in this workspace, so review relied on repo-local CONTRIBUTING.md/ARCHITECTURE.md and existing internal/cmd packages as the available convention source.security:code-auditor— complete (broad); inspected 9 assigned files (27 inspected across reviewers):integration-tests.md,internal/apptoken/apptoken.go,internal/client/apps.go,internal/cmd/apps/apps.go,internal/cmd/apps/session.go,internal/cmd/config/show.go,internal/cmd/setcred/setcred.go,internal/keychain/keychain.go,internal/noleak/noleak_test.go; skipped: none; constraints: nonearchitecture:solid-reviewer-agnostic— complete (broad); inspected 8 assigned files (27 inspected across reviewers):internal/apptoken/apptoken.go,internal/client/apps.go,internal/cmd/apps/apps.go,internal/cmd/apps/diff.go,internal/cmd/apps/manifest.go,internal/cmd/apps/session.go,internal/cmd/root/root.go,internal/keychain/keychain.go; skipped: none; constraints: Ran go build ./... and go test for apptoken, cmd/apps, cmd/root, cmd/setcred and noleak at the PR head; all pass. Did not run golangci-lint.
Inspected files (27)
README.mdintegration-tests.mdinternal/apptoken/apptoken.gointernal/apptoken/apptoken_test.gointernal/client/apps.gointernal/cmd/apps/apps.gointernal/cmd/apps/apps_test.gointernal/cmd/apps/diff.gointernal/cmd/apps/manifest.gointernal/cmd/apps/session.gointernal/cmd/apps/testdata/create-response.jsoninternal/cmd/apps/testdata/invalid-manifest-response.jsoninternal/cmd/apps/testdata/manifest-export.jsoninternal/cmd/apps/testdata/manifest-more-scopes.jsoninternal/cmd/apps/testdata/manifest.jsoninternal/cmd/apps/testdata/manifest.yamlinternal/cmd/apps/testdata/ok-response.jsoninternal/cmd/apps/testdata/rotate-response.jsoninternal/cmd/apps/testdata/token-expired-response.jsoninternal/cmd/apps/testdata/update-response.jsoninternal/cmd/config/show.gointernal/cmd/root/root.gointernal/cmd/root/root_test.gointernal/cmd/setcred/setcred.gointernal/cmd/setcred/setcred_test.gointernal/keychain/keychain.gointernal/noleak/noleak_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 5m 23s | ~$4.51 (est.) | claude-sonnet-5, claude-opus-5-5 | cr 0.10.319
| Field | Value |
|---|---|
| Model | claude-sonnet-5, claude-opus-5-5 |
| Reviewers | go:implementation-tests, harness-engineering:repo-health, policies:conventions, security:code-auditor, architecture:solid-reviewer-agnostic |
| Engine | claude_cli · claude-sonnet-5, claude-opus-5-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 5m 23s wall · 12m 29s compute |
| Cost | ~$4.51 (est.) |
| Pricing basis | anthropic-public-2026-09-23 |
| Tokens | 198 in / 41.2k out |
Per-workstream usage
orchestrator-selection— claude-sonnet-5- In: 6
- Out: 7.6k
- Cache read: 96.8k
- Cache create: 48.0k
- Cost: ~$0.29 (est.)
- Duration: 1m 17s
go:implementation-tests— claude-sonnet-5- In: 52
- Out: 10.4k
- Cache read: 2.1M
- Cache create: 91.2k
- Cost: ~$0.88 (est.)
- Duration: 3m 27s
harness-engineering:repo-health— claude-sonnet-5- In: 34
- Out: 6.5k
- Cache read: 1.2M
- Cache create: 183.2k
- Cost: ~$1.04 (est.)
- Duration: 1m 53s
policies:conventions— claude-sonnet-5- In: 52
- Out: 7.5k
- Cache read: 1.8M
- Cache create: 66.2k
- Cost: ~$0.70 (est.)
- Duration: 2m 44s
security:code-auditor— claude-sonnet-5- In: 28
- Out: 3.7k
- Cache read: 899.7k
- Cache create: 67.3k
- Cost: ~$0.49 (est.)
- Duration: 1m 12s
architecture:solid-reviewer-agnostic— claude-opus-5-5- In: 20
- Out: 4.5k
- Cache read: 548.0k
- Cache create: 86.2k
- Cost: ~$0.89 (est.)
- Duration: 1m 29s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 933
- Cache read: 168.2k
- Cache create: 45.0k
- Cost: ~$0.22 (est.)
- Duration: 22s
A mistyped apps diff invocation exited 1, which scripts read as drift. Argument and flag errors now exit 2. The typed error is renamed AppsAPIError and documented as AppsClient's only, the exit-2 helper is cannotCompare, the JSON carve-out list in docs/development.md names apps get, and a test covers removing the credentials file when create fails.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 3a7b4082cee8
Profile: claude-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| harness-engineering:repo-health | 0 |
| policies:conventions | 0 |
| security:code-auditor | 0 |
| architecture:solid-reviewer-agnostic | 0 |
Reviewer Coverage
go:implementation-tests— complete (constrained); inspected 15 assigned files (28 inspected across reviewers):internal/apptoken/apptoken.go,internal/apptoken/apptoken_test.go,internal/client/apps.go,internal/cmd/apps/apps.go,internal/cmd/apps/apps_test.go,internal/cmd/apps/diff.go,internal/cmd/apps/manifest.go,internal/cmd/apps/session.go,internal/cmd/config/show.go,internal/cmd/root/root.go,internal/cmd/root/root_test.go,internal/cmd/setcred/setcred.go,internal/cmd/setcred/setcred_test.go,internal/keychain/keychain.go,internal/noleak/noleak_test.go; skipped: none; constraints: noneharness-engineering:repo-health— complete (constrained); inspected 25 assigned files (28 inspected across reviewers):internal/apptoken/apptoken.go,internal/apptoken/apptoken_test.go,internal/client/apps.go,internal/cmd/apps/apps.go,internal/cmd/apps/apps_test.go,internal/cmd/apps/diff.go,internal/cmd/apps/manifest.go,internal/cmd/apps/session.go,internal/cmd/apps/testdata/create-response.json,internal/cmd/apps/testdata/invalid-manifest-response.json,internal/cmd/apps/testdata/manifest-export.json,internal/cmd/apps/testdata/manifest-more-scopes.json,internal/cmd/apps/testdata/manifest.json,internal/cmd/apps/testdata/manifest.yaml,internal/cmd/apps/testdata/ok-response.json,internal/cmd/apps/testdata/rotate-response.json,internal/cmd/apps/testdata/token-expired-response.json,internal/cmd/apps/testdata/update-response.json,internal/cmd/config/show.go,internal/cmd/root/root.go,internal/cmd/root/root_test.go,internal/cmd/setcred/setcred.go,internal/cmd/setcred/setcred_test.go,internal/keychain/keychain.go,internal/noleak/noleak_test.go; skipped: none; constraints: nonepolicies:conventions— complete (constrained); inspected 12 assigned files (28 inspected across reviewers):README.md,docs/development.md,internal/cmd/apps/apps.go,internal/cmd/apps/apps_test.go,internal/cmd/apps/diff.go,internal/cmd/apps/manifest.go,internal/cmd/apps/session.go,internal/cmd/config/show.go,internal/cmd/root/root.go,internal/cmd/root/root_test.go,internal/cmd/setcred/setcred.go,internal/cmd/setcred/setcred_test.go; skipped: none; constraints: No sibling ../cli-common/docs or ../.github convenience copies are checked out in this workspace, so review relied on repo-local CONTRIBUTING.md/ARCHITECTURE.md/docs/development.md and existing internal/cmd packages as the available convention source.security:code-auditor— complete (constrained); inspected 9 assigned files (28 inspected across reviewers):integration-tests.md,internal/apptoken/apptoken.go,internal/client/apps.go,internal/cmd/apps/apps.go,internal/cmd/apps/session.go,internal/cmd/config/show.go,internal/cmd/setcred/setcred.go,internal/keychain/keychain.go,internal/noleak/noleak_test.go; skipped: none; constraints: nonearchitecture:solid-reviewer-agnostic— complete (constrained); inspected 8 assigned files (28 inspected across reviewers):internal/apptoken/apptoken.go,internal/client/apps.go,internal/cmd/apps/apps.go,internal/cmd/apps/diff.go,internal/cmd/apps/manifest.go,internal/cmd/apps/session.go,internal/cmd/root/root.go,internal/keychain/keychain.go; skipped: none; constraints: Pre-existing and out of scope: argument errors print twice (cobra's own 'Error:' line plus root's print). This already happens for other commands such aschannels get; the diff does not cause it. Re-review at 3a7b408. The prior findings (diff exit code on invocation errors, the APIError name, the usageOrFailure name) are fixed. A branch build exits 2 forapps diffwith no app id, and the tests for apps, root and client pass. golangci-lint was not run.
Inspected files (28)
README.mddocs/development.mdintegration-tests.mdinternal/apptoken/apptoken.gointernal/apptoken/apptoken_test.gointernal/client/apps.gointernal/cmd/apps/apps.gointernal/cmd/apps/apps_test.gointernal/cmd/apps/diff.gointernal/cmd/apps/manifest.gointernal/cmd/apps/session.gointernal/cmd/apps/testdata/create-response.jsoninternal/cmd/apps/testdata/invalid-manifest-response.jsoninternal/cmd/apps/testdata/manifest-export.jsoninternal/cmd/apps/testdata/manifest-more-scopes.jsoninternal/cmd/apps/testdata/manifest.jsoninternal/cmd/apps/testdata/manifest.yamlinternal/cmd/apps/testdata/ok-response.jsoninternal/cmd/apps/testdata/rotate-response.jsoninternal/cmd/apps/testdata/token-expired-response.jsoninternal/cmd/apps/testdata/update-response.jsoninternal/cmd/config/show.gointernal/cmd/root/root.gointernal/cmd/root/root_test.gointernal/cmd/setcred/setcred.gointernal/cmd/setcred/setcred_test.gointernal/keychain/keychain.gointernal/noleak/noleak_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 1m 58s | ~$2.93 (est.) | claude-sonnet-5, claude-opus-5-5 | cr 0.10.319
| Field | Value |
|---|---|
| Model | claude-sonnet-5, claude-opus-5-5 |
| Reviewers | go:implementation-tests, harness-engineering:repo-health, policies:conventions, security:code-auditor, architecture:solid-reviewer-agnostic |
| Engine | claude_cli · claude-sonnet-5, claude-opus-5-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 1m 58s wall · 4m 30s compute |
| Cost | ~$2.93 (est.) |
| Pricing basis | anthropic-public-2026-09-23 |
| Tokens | 68 in / 12.7k out |
Per-workstream usage
go:implementation-tests— claude-sonnet-5- In: 14
- Out: 2.4k
- Cache read: 774.6k
- Cache create: 96.6k
- Cost: ~$0.57 (est.)
- Duration: 1m 02s
harness-engineering:repo-health— claude-sonnet-5- In: 14
- Out: 2.9k
- Cache read: 802.0k
- Cache create: 54.4k
- Cost: ~$0.41 (est.)
- Duration: 47s
policies:conventions— claude-sonnet-5- In: 12
- Out: 3.3k
- Cache read: 516.7k
- Cache create: 69.5k
- Cost: ~$0.41 (est.)
- Duration: 1m 04s
security:code-auditor— claude-sonnet-5- In: 10
- Out: 1.5k
- Cache read: 423.1k
- Cache create: 70.7k
- Cost: ~$0.38 (est.)
- Duration: 27s
architecture:solid-reviewer-agnostic— claude-opus-5-5- In: 12
- Out: 2.1k
- Cache read: 543.5k
- Cache create: 112.3k
- Cost: ~$1.05 (est.)
- Duration: 53s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 439
- Cache read: 220.5k
- Cache create: 16.3k
- Cost: ~$0.11 (est.)
- Duration: 15s
Summary
slck appsmanages Slack apps through their manifests (apps.manifest.*):getprints a manifest as indented JSON with sorted keys (or--to-file),validate --file [--app]checks one,diff <app-id> --filecompares the live manifest with a file one line per JSON pointer,create --filecreates an app,update <app-id> --filereplaces a manifest and says when the scopes changed and a reinstall is needed, anddelete <app-id>deletes one after a prompt (--forceskips it; without a terminal it refuses unless--force).yaml/.ymlapp_config_token, holding the access token, refresh token and expiry as one JSON value so a rotation replaces both in one writeslck set-credential --key app_config_token --stdinwith{"token": "xoxe.xoxp-...", "refresh_token": "xoxe-...", "exp": <unix seconds>}; anything else is rejected without echoing ittooling.tokens.rotatewhen the access token is within five minutes of expiry, or once when Slack answerstoken_expired/invalid_auth, and saves the new pair before using itconfig show(text and--json) reports the pair's presence, the access token's expiry and the team, never the tokensapps createnever prints the new app's client secret, signing secret or verification token;--credentials-filewrites them to a new file with mode 0600, opened before the app is created so a bad path fails firstapps diffexits 0 when they match, 1 when they differ and 2 when it could not compare;Executenow honors an exit code an error carries, and every other error still exits 1internal/cmd/apps/testdataInstalling an app and reading its bot token need a person in a browser, so they are not here.
Verified against a live workspace with a branch build, read-only: stored a pair whose access token had expired, then
apps getrotated it before the first call (config showthen reported a new 12 hour expiry and the team), exported three apps' manifests,validate --apppassed for each,diffagainst each export reported no differences and exited 0, an added scope showed as one~line with exit 1, a missing file exited 2, a YAML copy of a manifest diffed clean, a manifest with an event missing its scope came backinvalid_manifestwith the pointer and message, anddeletewith stdin from/dev/nullrefused without calling Slack.Type of Change
Checklist
make test)