feat(apps): add recipes seams for an edition-supplied recipes app - #4199
feat(apps): add recipes seams for an edition-supplied recipes app#4199czziemba wants to merge 1 commit into
Conversation
ec08fdc to
a3c3ccb
Compare
|
Three of your red On the current head ( What the gate is
Because it reads a checked-in file and statically scans source, it is deterministic — it cannot flake, and it will keep failing until the line changes. What
|
a3c3ccb to
949594e
Compare
949594e to
a9d6aba
Compare
a9d6aba to
71cc4ce
Compare
Design Review (Fable 5, fork) — 🟡 CONCERNSAdvisory design-level review of Design-Verdict: CONCERNS The Watch
Suggestions
[DESIGN-REVIEWED] 71cc4ce |
GPT 5.6 Review (fork) — 🔴 changes requested (blocking)Reviewed BLOCKING -- src/kiro_crew/dashboard/handlers/slack_routing.py:198 -- routing updates overwrite corrupt configuration BLOCKING -- src/kiro_crew/apps/manifest.py:1027 -- null recipe arrays crash installation FINDING -- src/kiro_crew/apps/manifest.py:994 -- the stated FINDING -- src/kiro_crew/dashboard/handlers/slack_routing.py:44 -- function-local imports here and in |
First Principles Review (Fable 5, fork) — 🔴 BLOCK (advisory)Advisory premise-level review of Reading the contract, the intent file, and the authoritative patch is done; I verified the claims against the trusted base (writer helpers, regexes, route tables, consumer counts). Final review follows. First-Principles-Verdict: BLOCK
What this change shipsIntent: let an installed app get its declared agents reachable (Slack channel + cron) without manual setup — an ADDITION, deliberately inert on
Blockers
WatchItems 1, 2, and 7 all have zero grep-counted in-tree consumers; the sole consumer is an out-of-tree edition app, and the truncated description says issue #4196 left open whether this interface should be ratified first. Public manifest schema is one-way surface once released. Description says " Subtractions
[FIRST-PRINCIPLES-REVIEWED] 71cc4ce |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed Review detailsI've verified the load-bearing facts. The removed Now falsifying the sole candidate: the No other hunk yields a grounded, reachable defect. No findings. [OPUS-REVIEWED] 71cc4ce |
|
Kiro Crew [operator: chenmingwei23#de330d0c]: This PR has been inactive for 7+ days. I reviewed the blockers -- the CI failures split into mechanical fixes plus design decisions that only you (and a maintainer) can settle, so it needs your input before automation can safely drive it: Needs a decision (blocking):
Mechanical items (fixable once the above is settled): GPT 5.6 (BLOCKING) flags corrupt-config clobbering the whole document in When you have addressed these (and the RFC question is ruled on), the pipeline will re-assess on its next cycle. If you would prefer no automation on this PR, add the |
|
On the suggestion to reuse the shared The shared pattern is never used on its own. So the repo's actual convention is not "use this regex", it is "use this regex plus a length bound" ( The One concession. My upper bound of 31 is more generous than the repo's |
Head branch was pushed to by a user without write access
71cc4ce to
c5ecd93
Compare
|
Thanks for the nudge, and for splitting it into decisions vs mechanical items. Answering the decisions here, since those are what the pipeline is waiting on. Flagging up front that the revision carrying the implementation is not pushed yet, so the diff above still shows the old code; treat this comment as the decision record, not as a claim about what is currently visible. 1. Schema surface: delete it. 2. Writer approach: route through the existing writer. This is the same fix as GPT 5.6's corrupt-config blocker, so that item is not separate: the old code did a raw 3. 4. Doc/comment contradiction: the doc is right. Core ships no recipes implementation. The stale On the mechanical items, all four are handled: the That last one deserves a correction rather than a silent fix: the comment in my file justified the lazy imports as circular-import avoidance, and that was wrong. Separately, I have posted a reply above pushing back on the shared- The RFC question in #4196 remains the one genuinely outstanding item, and is not mine to settle. |
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
Core validates a `recipes` manifest vocabulary and brokers the one
write an external app cannot do itself, the channel-routing write at
PUT /api/slack/channels/{id}/routing: it needs the gateway's config
lock and an in-memory routing refresh. No recipes implementation
ships here; an app tagged `recipes-provider` supplies it.
Also drops a duplicate GET /api/slack/channels registration.
c5ecd93 to
7073d46
Compare
|
Rebased onto main Conflicts resolved:
Gates run locally on changed files: isort, flake8, pytest ( Please review the resolution. A maintainer push makes the maintainer the last pusher, so a second approver is needed under the repo's last-push rule. The open contract question from #4196 (whether |
Problem / Motivation
An app can ship agents in-tree today, but it has no way to say "this agent
wants a Slack channel of its own" or "this agent should run on a schedule".
Getting an agent from installed to reachable is manual every time: create
the channel in Slack, route it to the agent, repeat. That is tolerable once and
tedious at three, and the tedium is what stops anyone shipping small
agent-plus-channel bundles as installable units.
Two of those steps are ones an app genuinely cannot perform for itself, however
much code it ships:
slack.channels[id]lives inconfig.json, whose write lock is an in-processasyncio.Lock. A secondprocess writing that file races every other config writer with no shared OS
lock, so a lost update is rare but real.
_reload_orch_cfgis in-process, sorouting written from outside does not take effect until the next restart.
An external app backend also runs in its own process with its own
.venvandcannot import
kiro_crew, so it has no in-process route to either one.Why it matters
This is the seam work that lets the agent-plus-channel bundle be a thing you
install rather than a runbook someone follows. Without it, every such bundle
costs manual Slack setup per recipe, which is precisely the friction that keeps
them from being shared.
It is deliberately inert on
main: core gains a validated vocabulary and onebrokered endpoint, and ships no recipes implementation. The cost of carrying it
is a schema plus 248 lines of handler; the benefit is that an installed app can
own the whole workflow without a second config writer racing the gateway.
What changed (motivation → approach → change)
Goal: let an installed app act on recipe declarations without core owning
the feature, and without a second process writing
config.json.Approach, and what it is not. The obvious design is to broker everything,
so core creates and archives Slack channels on the app's behalf. Rejected: an
app backend runs same-UID and can read the credential store directly, so
brokering the Slack calls buys no containment (see
src/kiro_crew/docs/app-platform-trust-model.md) while handing core a muchlarger and more dangerous surface, channel creation and archival, to serve a
workflow not every install wants. So the app does its own Slack calls, and core
brokers only the one write it must own, for the correctness reasons above. The
alternative of letting the app write
config.jsonitself, with core offeringonly a reload nudge, is smaller but puts a second writer on a file whose lock is
in-process; making that safe means giving every existing config writer an
OS-level lock, a bigger change than the endpoint it saves.
What was built:
recipesmanifest vocabulary (apps/manifest.py) —SlackRecipe,CronRecipe,RecipesConfig,RecipeDependencies, withAppManifest._validate_recipesenforcing kebab-case names, uniqueness,required fields, valid activations, and the cron either/or rules
(
schedulexoreverySecs,promptFilexorpromptText). Parsed,validated, otherwise ignored.
PUT /api/slack/channels/{channel_id}/routing(
dashboard/handlers/slack_routing.py) — writes or removesslack.channels[id]under the gateway's existing_get_config_lock(), thenrefreshes in-memory routing. Handles teardown via
{"remove": true}, so thelifecycle is symmetric. Channel id is validated against an anchored bounded
pattern before it reaches the config document, activation against the values
ChannelConfigaccepts, and every call is attributed in the audit log.recipes-providertag + install-time hint (apps/manager.py,cli_commands.py) —recipes_provider_installed()andrecipes_provider_hint()tell the user at install time when an app declaresrecipes but nothing is installed that would act on them. A manifest tag
rather than a schema field, so provider detection costs the schema nothing;
both helpers swallow every exception, because a hint must never break an
install.
kirocrew app install registry:<name>(cli.py,cli_commands.py) —install_appalready accepted this form; only the CLI help and dispatchwere missing.
Also removes a duplicate
GET /api/slack/channelsregistration. It wasregistered twice, and the refactor that split the route table into
dashboard/routes/carried both copies into different modules(
sessions.pyandtaskrunner.py).register_allrunssessionsbeforetaskrunner, so thetaskrunnercopy never resolved; this drops the dead oneand leaves the live registration and its
website/src/api/client.tscalleruntouched.
Tests
test/test_app_manifest_recipes.py— 50 cases in 6 classes, covering theschema this PR makes public:
TestSlackRecipeRoundTrip,TestCronRecipeRoundTrip,TestRecipesConfigRoundTrip,TestRecipeDependencies) — every field survivesfrom_dict→to_dict, andoptional fields are omitted rather than emitted empty when unset
(
purpose,dependencies,persistentSession,silent). This is whatkeeps a manifest from growing keys nobody wrote.
TestAppManifestRecipesIntegration) — a manifestwith no
recipesomits the section entirely, andrecipesdoes not leak intoextra.TestRecipesValidation) — each rule is pinned by the casethat violates it: missing name, non-kebab name, missing
description/agent/channelNamePart, over-long channel name part, invalid
activation, duplicate names, and the cron either/or pairs in both directions
(neither supplied, both supplied).
Not covered by unit tests: the routing endpoint itself. It is an aiohttp
handler whose behaviour is the config write plus the in-memory refresh, so a
meaningful test needs a gateway fixture; called out under Manual verification
rather than papered over.
Manual verification
Verified locally:
./scripts/docs-lint.sh— passes (213 files).python -m py_compile— clean across every touched module.dashboard/routes/split: exactly oneGET /api/slack/channelsregistration remains (routes/sessions.py), and thenew PUT is registered once (
routes/messaging.py). Confirmed no catch-all(
:.*) route exists in the package and that the literal three-segment pathcannot be shadowed by the five-segment pattern, so registration order is not
load-bearing here.
AppManifest: a well-formedrecipesblock validates to[], and malformed entries produce the expectedmessages.
recipes_provider_installed()/recipes_provider_hint()exercised acrossfive states (enabled provider, disabled provider, no provider, app declaring
no recipes, app absent) — the hint fires only in the intended one.
Still required, and I could not run it here:
pytestandcd website && npm run check. This checkout has no pytest, aiohttp, orcroniter, so CI is the first real execution of the new test file. Flagging that
rather than implying a green suite.
Worth a reviewer's eye: the routing endpoint against a live gateway — set
routing for a channel, confirm messages route to the named agent without a
restart, then
{"remove": true}and confirm the entry is gone fromconfig.jsonand routing stops.Related Issues
Closes #4196
That issue also asks whether this wants an RFC in
docs/request-for-change/first. CONTRIBUTING routes changes to a public interface, changes other parts of
the project build around, and changes that would be expensive to reverse through
an RFC, and the
recipesblock is a manifest contract third-party app authorswould write against. If maintainers want it written up, folding it into
rfc-federated-app-platform.mdas a phase looks better than a new document.Please rule on that before spending review time on the code.
Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)tests added (50 cases); "existing tests pass" is unverified locally, see
Manual verification. CI is the first run.
docs/system-specs/features/recipes.mdplus its index entry;
docs-lint.shgreenfor credential patterns and internal names