Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion review/persona.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,18 @@ export default definePersona({

integrations: {
github: {},
slack: {}
slack: {
// Cloud mounts an integration's relayfile subtree only from `scope`
// (or from triggers β€” and this persona has github triggers only). A
// scope-less `slack: {}` mounts nothing, so slackClient().post() wrote
// its draft to unmounted local disk and the writeback worker never saw
// it: every Slack ping was a silent no-op. The channel is picked at
// deploy time (SLACK_CHANNEL input), so the scope can't name one
// statically β€” mount the channels subtree, which covers the
// `/slack/channels/{channelId}/messages` writeback path for any picked
// channel (and excludes DMs/users).
scope: { paths: '/slack/channels/**' }
}
},

inputs: {
Expand Down
20 changes: 20 additions & 0 deletions tests/review-agent.test.mjs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import assert from 'node:assert/strict';
import test from 'node:test';

import { parseIntegrations } from '@agentworkforce/persona-kit';

import {
labelNames,
readPr,
Expand Down Expand Up @@ -98,3 +100,21 @@ test('labelNames normalizes github label arrays defensively', () => {
]), ['no-agent-relay-review']);
assert.deepEqual(labelNames(undefined), []);
});

// Cloud only mounts an integration's relayfile subtree from its `scope` (or
// from triggers β€” and this persona has github triggers only). A scope-less
// `slack: {}` mounts nothing, so every slackClient() post was written to
// unmounted local disk and silently dropped. persona-kit also discards empty
// scope objects client-side, so the scope must survive parsing as a non-empty
// string map covering the `/slack/channels/{channelId}/messages` writeback
// path. This pins both halves.
test('persona declares a slack scope that survives persona-kit parsing and covers the messages writeback path', async () => {
const { default: persona } = await import('../.test-build/review/persona.js');
const parsed = parseIntegrations(persona.integrations, 'integrations');
const scope = parsed?.slack?.scope;
assert.ok(scope && Object.keys(scope).length > 0, 'slack integration must declare a non-empty scope or cloud mounts no /slack paths');
const covers = Object.values(scope).some(
(value) => typeof value === 'string' && value.startsWith('/slack/channels'),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: The coverage assertion is too permissive: checking startsWith('/slack/channels') will also accept invalid scopes like /slack/channels-private/**, so this test can pass even when the writeback path is not actually mounted. Tighten the predicate to require the real channels subtree pattern (for example exact /slack/channels/** or at least a /slack/channels/ boundary) so regressions are caught. [incorrect condition logic]

Severity Level: Major ⚠️
- ❌ Slack ready-for-review notifications may silently fail after regression.
- ❌ Slack failure/merge pings can break without tests detecting it.
- ⚠️ Test suite gives false confidence about Slack scope mounting correctness.
Steps of Reproduction βœ…
1. Open `review/persona.ts` and observe the Slack integration scope at
`review/persona.ts:17-27`, where `scope: { paths: '/slack/channels/**' }` is configured to
mount the `/slack/channels/{channelId}/messages` writeback path.

2. Open the persona integration test at `tests/review-agent.test.mjs:111-119`. This test
imports the built persona (`../.test-build/review/persona.js`), parses integrations via
`parseIntegrations`, and then computes `covers` using `Object.values(scope).some((value)
=> typeof value === 'string' && value.startsWith('/slack/channels'))` on line 117.

3. Note that the predicate on line 117 only checks `startsWith('/slack/channels')`. In
JavaScript, a value such as `'/slack/channels-private/**'` also satisfies this condition
(`'/slack/channels-private/**'.startsWith('/slack/channels') === true`), even though it
does not match the documented `/slack/channels/{channelId}/messages` subtree described in
the comment at `review/persona.ts:24-26` and `tests/review-agent.test.mjs:104-110`.

4. If a future change mistakenly sets the Slack scope in `review/persona.ts:27` to an
invalid pattern like `'/slack/channels-private/**'`, `parseIntegrations` would produce a
scope whose value is that invalid path; the test at `tests/review-agent.test.mjs:111-119`
would still pass because `startsWith('/slack/channels')` remains true, but the actual
writeback path `/slack/channels/{channelId}/messages` would no longer be guaranteed to be
mounted in cloud, allowing the original "Slack pings are silently dropped" regression to
reoccur without being caught by this test.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent πŸ€–
This is a comment left during a code review.

**Path:** tests/review-agent.test.mjs
**Line:** 117:117
**Comment:**
	*Incorrect Condition Logic: The coverage assertion is too permissive: checking `startsWith('/slack/channels')` will also accept invalid scopes like `/slack/channels-private/**`, so this test can pass even when the writeback path is not actually mounted. Tighten the predicate to require the real channels subtree pattern (for example exact `/slack/channels/**` or at least a `/slack/channels/` boundary) so regressions are caught.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
πŸ‘ | πŸ‘Ž

);
Comment on lines +116 to +118

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The current assertion checks if any scope value starts with /slack/channels. However, if the scope was accidentally set to /slack/channels (without a trailing slash or wildcards), the test would still pass, but it would not actually cover the subpaths like /slack/channels/{channelId}/messages required for the writeback worker.

Updating the check to require a trailing slash (i.e., /slack/channels/) ensures that the scope correctly targets subresources within the channels directory.

Suggested change
const covers = Object.values(scope).some(
(value) => typeof value === 'string' && value.startsWith('/slack/channels'),
);
const covers = Object.values(scope).some(
(value) => typeof value === 'string' && value.startsWith('/slack/channels/'),
);

assert.ok(covers, 'slack scope must cover /slack/channels/** so slackClient() drafts reach the writeback worker');
});