Conversation
f9af168 to
db15896
Compare
github-actions
Bot
added
the
contract:chat
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Thanks for wiring the Discord adapter from #1484 into the docs — the links to tools/chat-discord/ resolve once #1484 lands, and the stack is in order. Two things need fixing before this merges: a wrong tracking link in the adapter registry (inline), and spec text that now contradicts itself.
The contributor-growth spec now contradicts itself
The specs in
tools/spec-loop/specs/are the source of truth and must not fall behind the code.
—AGENTS.md
In tools/spec-loop/specs/contributor-growth.md the frontmatter source: and the Known-gaps bullet now say chat covers Slack and Discord, but two passages the diff doesn't touch still say otherwise:
- Where it lives → Tools bullet:
`tools/chat-slack` (Slack MCP adapter, public channels only, never posts). Discord is an extension point with no adapter yet. - Behaviour & contract → Community signals:
chat answers through `contract:chat` (Slack adapter; public channels only).
Please add tools/chat-discord (Discord MCP adapter, public channels only, never posts) to the Tools bullet, drop the "Discord is an extension point" sentence, and change "(Slack adapter; …)" to "(Slack or Discord adapter; …)".
Smaller observations
- See inline on
specs/adapters.mdand the project template (chat.guild_id). tools/chat/README.md→ Prerequisites → Credentials / auth still describes only the Slack connector; please add a clause for the Discord adapter.- The PR description lists
docs/labels-and-capabilities.mdchanges, but those are in #1484 — please correct it so the stack's descriptions match their contents. And consider "Closes #1421" on whichever stack PR lands last, so the adapter issue closes when the work ships. - Any change to #1484's coverage (see the review there) flows into the "shipping" wording here.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. After you've
addressed the points above and pushed an update, an Apache Magpie
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
CONTRIBUTING.md.
| | [`tools/vcs`](../../tools/vcs/) | Git, Mercurial, Fossil | Subversion [\#602](https://github.com/apache/magpie/issues/602), Jujutsu [\#603](https://github.com/apache/magpie/issues/603), Perforce [\#605](https://github.com/apache/magpie/issues/605) | | ||
| | Forge / tracker | [`github`](../../tools/github/), [`jira`](../../tools/jira/), [`bitbucket`](../../tools/bitbucket/) `partial-read-only` foundation, [`sourcehut`](../../tools/sourcehut/), [`fossil`](../../tools/fossil/), [`gitlab`](../../tools/gitlab/) `partial-read-only` foundation | Forgejo/Gitea [\#310](https://github.com/apache/magpie/issues/310), Pagure [\#312](https://github.com/apache/magpie/issues/312), deeper Bitbucket/Jira coverage [\#606](https://github.com/apache/magpie/issues/606), GitLab [\#305](https://github.com/apache/magpie/issues/305), Bugzilla [\#302](https://github.com/apache/magpie/issues/302) | | ||
| | [`tools/chat`](../../tools/chat/) | [`chat-slack`](../../tools/chat-slack/) | Discord [#1421](https://github.com/apache/magpie/issues/1421) | | ||
| | [`tools/chat`](../../tools/chat/) | [`chat-slack`](../../tools/chat-slack/), [`chat-discord`](../../tools/chat-discord/) | Matrix [#1422](https://github.com/apache/magpie/issues/1422), Zulip | |
There was a problem hiding this comment.
major — #1422 is a merged contributor-growth PR ("small penalty for automated work that drew maintainer pushback"), not a Matrix issue. The page defines the column as "extension point = a documented, labelled slot with a tracking issue", and the existing tracking issues are #309 (Matrix / Element) and #308 (Zulip):
| | [`tools/chat`](../../tools/chat/) | [`chat-slack`](../../tools/chat-slack/), [`chat-discord`](../../tools/chat-discord/) | Matrix [#1422](https://github.com/apache/magpie/issues/1422), Zulip | | |
| | [`tools/chat`](../../tools/chat/) | [`chat-slack`](../../tools/chat-slack/), [`chat-discord`](../../tools/chat-discord/) | Matrix [#309](https://github.com/apache/magpie/issues/309), Zulip [#308](https://github.com/apache/magpie/issues/308) | |
| It never calls a tool that sends, schedules, drafts, or edits a message. | ||
| `discord` and `none` are placeholders in the contract's adapter table | ||
| (Discord tracked in #1421). | ||
| - `tools/chat-discord/` — the shipping `contract:chat` adapter: a mapping |
There was a problem hiding this comment.
minor — Both this and the Slack bullet above now call themselves "the shipping contract:chat adapter" — use "a shipping adapter" (or "the Slack adapter" / "the Discord adapter"). Please also add tools/chat-discord/ to this spec's frontmatter source: list, which still ends tools/chat/, tools/chat-slack/.. The trailing "none is a placeholder …" sentence now reads as part of the Discord bullet; it fits better under the tools/chat/ bullet.
| | Security cross-ref | `osv` | [`../../tools/osv/`](../../../tools/osv/) | `security_cross_ref.tool`, `security_cross_ref.ecosystem` | | ||
| | Project metadata (rosters / people / releases) | *org-level* — inherited from `organizations/<org>/organization.md → project_metadata.kind`; for ASF: `apache-projects` ([`tools/apache-projects/`](../../../tools/apache-projects/)), for independent: `none` | — | override in [Project metadata](#project-metadata) only if this project differs from its org | | ||
| | Project chat | TODO: `slack`, `discord`, or `none` | [`../../tools/chat/`](../../../tools/chat/) (abstract) + adapter dirs (`tools/chat-slack/`) | `chat.kind`, `chat.channels` — see [Project chat](#project-chat) below; read-only, public channels only | | ||
| | Project chat | TODO: `slack`, `discord`, or `none` | [`../../tools/chat/`](../../../tools/chat/) (abstract) + adapter dirs (`tools/chat-slack/`, `tools/chat-discord/`) | `chat.kind`, `chat.channels` — see [Project chat](#project-chat) below; read-only, public channels only | |
There was a problem hiding this comment.
minor — #1484's adapter README adds an optional chat.guild_id key (and operations.md builds message URLs from <guild_id>), but neither this knobs column, the template's ## Project chat YAML block, nor the contract's ## Configuration block in tools/chat/README.md mentions it — an adopter filling in the template won't learn it exists. Please add it as an optional, Discord-only line.
…oject template Integrate tools/chat-discord into the chat contract documentation, adapter registry, labels-and-capabilities taxonomy, and adopter project template. Updates spec-loop specs to reflect Discord as a shipping adapter under contract:chat alongside Slack. Generated-by: Antigravity
…rd adapter Update vendor-neutrality scoring for contract:chat with Discord adapter. The contract now has 2 distinct backend vendors (Discord, Slack), advancing the framework's overall vendor-neutrality score to 11/12 capability contracts (92%). Updates docs/vendor-neutrality.md and adds regression test in test_vendor_neutrality_score.py. Generated-by: Antigravity
…pecs, and scorer test Address review feedback on PR #1485: - docs/adapters/registry.md: fix tracking issues for Matrix (#309) and Zulip (#308) - tools/spec-loop/specs/contributor-growth.md: list tools/chat-discord, remove extension point text, and note Slack or Discord in community signals - tools/spec-loop/specs/adapters.md: add tools/chat-discord/ to frontmatter, align Slack/Discord adapter wording, and move none placeholder to tools/chat/ - plugins/magpie-setup/templates/project.md: document chat.guild_id in summary table and yaml example - tools/chat/README.md: document chat.guild_id in yaml example and mention Discord bot token under $HOME in prerequisites - tools/vendor-neutrality-score/tests/test_vendor_neutrality_score.py: test live repository tools loaded from disk rather than synthetic objects Generated-by: Antigravity
d0c0a8e to
2caa40f
Compare
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Thanks — all three earlier threads, the contributor-growth spec contradictions, and the Credentials clause are fixed. Two small config-wording issues remain inline (guild_id shipping as an active "..." value, and "channel names or IDs" promised contract-wide while the Slack adapter resolves names only), plus a nit. Merge still waits on #1484.
The PR description still says this PR marks contract:chat vendor-neutral and moves the score to 11/12 — that change now lives in #1484; please refresh the description (and "taxonomy" in the title) to match what this PR contains.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The findings
below are observations, not blockers; an Apache Magpie
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
CONTRIBUTING.md.
| chat: | ||
| kind: none # slack | discord | none | ||
| channels: [] # channel names to read; empty = every public channel — | ||
| guild_id: "..." # optional Discord server (guild) ID when kind: discord and the bot joins multiple servers |
There was a problem hiding this comment.
minor — The template's default block is kind: none, but guild_id: "..." ships as an active key with a literal "..." value. An adopter who copies the block and later switches to kind: discord keeps a guild ID of "...". The key is optional and Discord-only, so please ship it commented out:
# guild_id: "" # Discord only, optional: server (guild) ID when the bot joins several serversSame change in tools/chat/README.md (the guild_id line in ## Configuration), where the example is kind: slack and an active guild_id contradicts its own "when kind: discord" comment.
| kind: slack # slack | discord | none | ||
| channels: [] # channel names; empty = every public channel | ||
| guild_id: "..." # optional Discord server (guild) ID when kind: discord and the bot joins multiple servers | ||
| channels: [] # channel names or IDs; empty = every public channel |
There was a problem hiding this comment.
minor — This is the contract-level config, so "channel names or IDs" now applies to every adapter. tools/chat-slack/operations.md resolves chat.channels only as names (slack_search_channels, one call per configured channel name as keywords), and its README still says "channel names"; the list_channels row above also still says "the configured channel names". Please keep the contract at "channel names", or say "names (Discord also accepts IDs)" here and in the template, so a Slack adopter doesn't list IDs that never match.
| - **Chat evidence is Slack-only.** `contract:chat` has one adapter | ||
| (`tools/chat-slack`); Discord and Matrix answers are not collected | ||
| - **Chat evidence covers Slack and Discord.** `contract:chat` has two shipping adapters | ||
| (`tools/chat-slack`, `tools/chat-discord`); Matrix answers are not collected |
There was a problem hiding this comment.
nit — adapters.md now says "Matrix and Zulip remain extension points"; please name both here too: "Matrix and Zulip answers are not collected until an adapter lands".
2 participants
Part 2 of 2 in the
contract:chatDiscord adapter stack. Depends on #1484. Closes #1421.Summary
discordfrom placeholder to shipping adapter undercontract:chat:tools/chat/README.md: documentsdiscordshipping status, mentions bot token location under$HOME(~/.config/apache-magpie/discord-tokenor$DISCORD_BOT_TOKEN), and adds optionalchat.guild_idto configuration example.docs/adapters/registry.md: markschat-discordas shipping alongsidechat-slack, citing open tracking issues for Matrix (#309) and Zulip (#308).plugins/magpie-setup/templates/project.md: documents Discord chat adapter and optionalchat.guild_idin the tools table and configuration example.tools/spec-loop/specs/adapters.md: addstools/chat-discord/to frontmatter source list, aligns Slack and Discord adapter descriptions, and places thenoneplaceholder undertools/chat/.tools/spec-loop/specs/contributor-growth.md: liststools/chat-discord/in tools inventory, removes extension point phrasing, and refers to Slack or Discord adapter in community signals.contract:chatvendor-neutral with 2 distinct backend vendors (Discord, Slack), advancing the framework's overall vendor-neutrality score to 11/12 capability contracts (92%).test_chat_contract_is_green_with_slack_and_discord()intools/vendor-neutrality-score/tests/test_vendor_neutrality_score.pytesting live repository tools viavns.load_tools(vns.find_repo_root()).Type of change
docs/,README.md,CONTRIBUTING.md)projects/_template/)tests/)Test plan
uv run --project tools/vendor-neutrality-score python -m pytest tools/vendor-neutrality-score/tests -k test_chat_contract_is_green_with_slack_and_discord(passed against live repository).docs/adapters/registry.md,tools/chat/README.md, and spec-loop specs.RFC-AI-0004 compliance
contract:chatto vendor-neutral status (2 vendors: Discord, Slack).Linked issues
Closes #1421
🤖 Generated with Antigravity