Skip to content

feat(apps): add recipes seams for an edition-supplied recipes app - #4199

Open
czziemba wants to merge 1 commit into
kirodotdev:mainfrom
czziemba:feat/recipes-port
Open

feat(apps): add recipes seams for an edition-supplied recipes app#4199
czziemba wants to merge 1 commit into
kirodotdev:mainfrom
czziemba:feat/recipes-port

Conversation

@czziemba

@czziemba czziemba commented Aug 17, 2026

Copy link
Copy Markdown

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:

  • The routing write has to be the gateway's. slack.channels[id] lives in
    config.json, whose write lock is an in-process asyncio.Lock. A second
    process writing that file races every other config writer with no shared OS
    lock, so a lost update is rare but real.
  • The running gateway has to be told. _reload_orch_cfg is in-process, so
    routing written from outside does not take effect until the next restart.

An external app backend also runs in its own process with its own .venv and
cannot 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 one
brokered 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 much
larger 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.json itself, with core offering
only 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:

  1. recipes manifest vocabulary (apps/manifest.py) — SlackRecipe,
    CronRecipe, RecipesConfig, RecipeDependencies, with
    AppManifest._validate_recipes enforcing kebab-case names, uniqueness,
    required fields, valid activations, and the cron either/or rules
    (schedule xor everySecs, promptFile xor promptText). Parsed,
    validated, otherwise ignored.
  2. PUT /api/slack/channels/{channel_id}/routing
    (dashboard/handlers/slack_routing.py) — writes or removes
    slack.channels[id] under the gateway's existing _get_config_lock(), then
    refreshes in-memory routing. Handles teardown via {"remove": true}, so the
    lifecycle is symmetric. Channel id is validated against an anchored bounded
    pattern before it reaches the config document, activation against the values
    ChannelConfig accepts, and every call is attributed in the audit log.
  3. recipes-provider tag + install-time hint (apps/manager.py,
    cli_commands.py) — recipes_provider_installed() and
    recipes_provider_hint() tell the user at install time when an app declares
    recipes 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.
  4. kirocrew app install registry:<name> (cli.py, cli_commands.py) —
    install_app already accepted this form; only the CLI help and dispatch
    were missing.

Also removes a duplicate GET /api/slack/channels registration. It was
registered twice, and the refactor that split the route table into
dashboard/routes/ carried both copies into different modules
(sessions.py and taskrunner.py). register_all runs sessions before
taskrunner, so the taskrunner copy never resolved; this drops the dead one
and leaves the live registration and its website/src/api/client.ts caller
untouched.

Tests

test/test_app_manifest_recipes.py — 50 cases in 6 classes, covering the
schema this PR makes public:

  • Round-trip fidelity (TestSlackRecipeRoundTrip,
    TestCronRecipeRoundTrip, TestRecipesConfigRoundTrip,
    TestRecipeDependencies) — every field survives from_dictto_dict, and
    optional fields are omitted rather than emitted empty when unset
    (purpose, dependencies, persistentSession, silent). This is what
    keeps a manifest from growing keys nobody wrote.
  • Manifest integration (TestAppManifestRecipesIntegration) — a manifest
    with no recipes omits the section entirely, and recipes does not leak into
    extra.
  • Validation (TestRecipesValidation) — each rule is pinned by the case
    that 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.
  • Route table inspected after the dashboard/routes/ split: exactly one
    GET /api/slack/channels registration remains (routes/sessions.py), and the
    new PUT is registered once (routes/messaging.py). Confirmed no catch-all
    (:.*) route exists in the package and that the literal three-segment path
    cannot be shadowed by the five-segment pattern, so registration order is not
    load-bearing here.
  • Validation exercised directly against the real AppManifest: a well-formed
    recipes block validates to [], and malformed entries produce the expected
    messages.
  • recipes_provider_installed() / recipes_provider_hint() exercised across
    five 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: pytest and
cd website && npm run check. This checkout has no pytest, aiohttp, or
croniter, 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 from
config.json and 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 recipes block is a manifest contract third-party app authors
would write against. If maintainers want it written up, folding it into
rfc-federated-app-platform.md as a phase looks better than a new document.
Please rule on that before spending review time on the code.

Checklist

  • Single commit with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality — new
    tests added (50 cases); "existing tests pass" is unverified locally, see
    Manual verification. CI is the first run.
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable) — docs/system-specs/features/recipes.md
    plus its index entry; docs-lint.sh green
  • No secrets, credentials, or internal references in the diff — diff scanned
    for credential patterns and internal names

@czziemba
czziemba requested a review from a team as a code owner August 17, 2026 20:28
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention fork Pull request from a fork (external contributor) labels Aug 17, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 17, 2026
@rnoack1

rnoack1 commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Three of your red Backend Tests jobs are one gate firing on one line, and it is not a flake — so re-running will not clear it. Flagging it with the fix, since the failure message alone does not explain what the bucket means.

On the current head (a3c3ccb1db, run 32068039555), Backend Tests (Windows) (2), (3.10, 2) and (3.12, 2) all fail the same assertion — three jobs because that shard runs on three interpreter/OS combinations, not three separate problems:

FAILED test/test_error_code_contract.py::test_no_new_error_response_without_a_code
  src/kiro_crew/dashboard/handlers/slack_routing.py: dynamic_status 0 -> 1

What the gate is

test_no_new_error_response_without_a_code is a ratchet, not a sweep. error-code-baseline.json at the repo root holds a per-file count of existing non-compliant error responses; the test scans src/kiro_crew and fails only when a file goes above its recorded cap. There are ~1484 error responses with a literal status >= 400 in the tree and almost none carry a code today, so the gate freezes that debt where it is and fails only when a new one lands. Your file had a cap of 0 in the dynamic_status bucket and now scans as 1.

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 dynamic_status means

It is not "you forgot a code". It means the scanner could not classify your response at all: the status= you passed is an expression rather than a literal (something like status=code, or status=500 if ... else 400), so whether the response is even an error depends on runtime state and no static rule applies.

It is capped separately on purpose. Per the gate's own header, without a cap on this bucket the check would be trivially defeated by computing the status instead of writing it, and that escape would look like ordinary refactoring in review.

How to satisfy it

  • If the status is knowable at the call site, make it a literal. If it is >= 400, return {"error": "<English prose>", "code": "<lower_snake_id>"}code is the contract the client switches on, error is advisory.
  • If the status genuinely has to be computed, say so in the PR description. That turns it into a one-line review conversation instead of a silent bypass, which is the outcome the separate bucket exists to produce.

Please do not regenerate the baseline to clear this. The assertion says so explicitly. The --update path on that file exists for the opposite direction — a count that improved and needs re-snapshotting, which a companion test enforces so the ratchet cannot silently loosen.

Why it is worth the bother

From the gate's header: the dashboard renders server prose verbatim into a localized UI (setError(res.error)), so an English sentence produced in Python lands untranslated inside a non-English page and no amount of frontend i18n can reach it — the string never passes through a catalog. A machine-readable code is what lets the client supply its own text. error-code-baseline.json doubles as the worklist for converting the existing sites, one file at a time.

Two other failures on the same run, unrelated to this one

So they are not a surprise later — I have not diagnosed either, just noting they are separate gates rather than more of the above:

  • test/test_app_bridges.py::TestBuiltinDeclaredResourcesActuallyRegister::test_builtin_dict_carries_every_declarative_manifest_field (3 jobs)
  • test/test_spawn_audit.py::test_every_spawn_is_routed_or_allowlisted (3 jobs)

Both look like declarative-contract guards of the same family, which a PR adding new app seams would plausibly trip.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 18, 2026
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 18, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 18, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — 🟡 CONCERNS

Advisory design-level review of 71cc4ce091e7ee3b5073010ba9fa1d676c8c6d30 via the fork AI-review pipeline — updated in place on each push; does not block merge.

Design-Verdict: CONCERNS

The _owner provenance the design leans on for cleanup is silently erased by any typed config save, so the teardown story degrades to the app's own ledger.

Watch

  • _owner doesn't survive a config round-trip. The endpoint writes _owner raw into slack.channels[id], but every other writer goes through KiroCrewConfig.load()save(), and ChannelConfig.from_dict / asdict (config/loader.py:3894, :6855) carry only activation/agent/thread_follow — the first unrelated agent PATCH or sync rewrites the channels map and drops the stamp. The doc promises "an app can find and remove its own entries at uninstall" via _owner; that only holds until any other config write. Either persist it through ChannelConfig or drop _owner and state that the app ledger is the sole provenance record.
  • The recipes schema is a public contract frozen ahead of any reviewable consumer. Semantics like the {alias}-{part}-kc-{ts} template, the 15-char cap, and capabilityPackages (which public core answers 503 for) are pinned by an edition app that isn't in this repo, so course-correcting the vocabulary after third parties write against it is a one-way door — the author's own RFC question (Recipes: seams so an edition can install Slack-channel and cron recipes #4196) should be settled before merge, exactly as the PR asks.
  • The manifest.py block comment says "the built-in recipes app owns their install… via kirocrew recipes …", contradicting the shipped doc ("core ships no recipes implementation"); pick one story before authors read the wrong one.

Suggestions

  • Have the endpoint round-trip through KiroCrewConfig (or teach ChannelConfig to preserve unknown keys) so there is one config document shape instead of a raw-JSON writer coexisting with a typed one under the same lock.

[DESIGN-REVIEWED] 71cc4ce

@github-actions

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

Reviewed 71cc4ce091e7ee3b5073010ba9fa1d676c8c6d30 via the fork AI-review pipeline; updated in place on each push.

BLOCKING -- src/kiro_crew/dashboard/handlers/slack_routing.py:198 -- routing updates overwrite corrupt configuration
logger.warning("config.json unreadable, starting from empty: %s", exc)
Corrupt config.json -> routing PUT -> empty document written -> unrelated settings and credentials are destroyed.
Anchor: residual/crash-data-loss-corruption
Fix: Use fail-closed update_config_locked off-loop instead of resetting the document.

BLOCKING -- src/kiro_crew/apps/manifest.py:1027 -- null recipe arrays crash installation
SlackRecipe.from_dict(r) for r in data.get("slack", []) if isinstance(r, dict)
Manifest containing "recipes":{"slack":null} -> install_app -> iteration raises TypeError -> CLI aborts.
Anchor: residual/crash-data-loss-corruption
Fix: Normalize null recipe collections to empty lists before iterating.

FINDING -- src/kiro_crew/apps/manifest.py:994 -- the stated everySecs and persistentSession manifest fields are ignored because only "every_secs" and "persistent_session" are parsed -> Fix: parse and serialize the declared camelCase keys.

FINDING -- src/kiro_crew/dashboard/handlers/slack_routing.py:44 -- function-local imports here and in cli_commands.py:632 violate top-level-imports -> Fix: move them to module scope, retaining only documented circular imports locally.

[BLOCK-MERGE] 71cc4ce
[GPT-REVIEWED] 71cc4ce

@github-actions

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🔴 BLOCK (advisory)

Advisory premise-level review of 71cc4ce091e7ee3b5073010ba9fa1d676c8c6d30 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push; does not block merge.

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

capabilityPackages names a mechanism core itself stubs to 503, and the routing write re-implements an existing dual-locked writer.

What this change ships

Intent: let an installed app get its declared agents reachable (Slack channel + cron) without manual setup — an ADDITION, deliberately inert on main.

  1. Apps can declare recipes (Slack + cron) in manifests, validated at install — zero in-tree consumers by design
  2. New PUT /api/slack/channels/{id}/routing writes/removes routing + live refresh — duplicate of slack/handler.py:_persist_channel_config
  3. Per-recipe dependencies.capabilityPackages field — zero consumers, targets a dead mechanism
  4. Install prints a hint when recipes are declared but no provider app exists — justified
  5. kirocrew app install registry:<name> from the CLI — rides along; framing contradicts diff
  6. Dead duplicate GET /api/slack/channels registration removed — justified deletion
  7. _owner provenance block persisted into config.json — new persisted state, zero readers in base
  8. Spawn-audit allowlist entry for the CLI asyncio.run — justified (CI gate)

Blockers

RecipeDependencies.capabilityPackages is unreachable surface. Grep capabilityPackages across base src/: 0 hits. The PR's own doc says the only resolver, CapabilityManager, "reports available() == False, so /api/capability/* answers 503" and "an app must ship agents in-tree." A public schema field whose one consumer is stubbed off is pure "so we can later." Subtraction: delete RecipeDependencies and both dependencies fields from the schema and tests.

_write_routing_locked is a second spelling of an existing writer. _persist_channel_config (src/kiro_crew/slack/handler.py:1078, 2 call sites) already read-modify-writes slack.channels[id].agent/.activation and pairs with _reload_orch_cfg — and it runs under both config locks via update_config_locked + run_config_write (dashboard/chat_utils.py:63), whose docstring says every config.json RMW must hold both; the new writer holds only _get_config_lock(), undermining the PR's own single-writer rationale. Subtraction: have the endpoint call run_config_write(_persist_channel_config, ...), extended with remove/_owner.

Watch

Items 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 "install_app already accepted this form; only the CLI help and dispatch were missing," but the diff's own comment says "install_app only accepts a local directory, so the registry path … is a different entrypoint" — the dispatch is new capability, not missing help text.

Subtractions

  • Drop _CHANNEL_ID_RE in slack_routing.pyvalidation.py:141 CHANNEL_ID_RE already exists (9 consuming sites across 5 files); reuse it with CHANNEL_MAX_LEN.
  • Drop _activation_values() and manifest's _SLACK_ACTIVATION_VALUESconfig/loader.py:3865 _VALID_ACTIVATIONS is the existing set both hand-copy.

[FIRST-PRINCIPLES-REVIEWED] 71cc4ce

@github-actions

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 71cc4ce091e7ee3b5073010ba9fa1d676c8c6d30 via the fork AI-review pipeline; updated in place on each push.

Review details

I've verified the load-bearing facts. The removed GET /api/slack/channels in taskrunner.py is a genuine duplicate — sessions.py:75 still registers it, so no route is lost (in fact aiohttp would reject the duplicate, making removal correct). The loader defines five activation constants.

Now falsifying the sole candidate: the SlackRecipe.activation docstring (activation: str = "always" # ... (prompt overridable)) and the feature doc both state activation is prompted/overridable at install. The manifest value is only a default; the actual channel routing is written through the endpoint, which accepts all five modes. So restricting the manifest default to three modes produces no observable wrong outcome — a recipe's channel can still end up review or off. The only residue is that the comment's "Mirrors ChannelConfig.activation" claim is imprecise, which is a comment-accuracy matter with no runtime consequence — and the candidate itself concedes confidence "low" and cannot rule out that the three-value default is deliberate. It fails (c). Dropped.

No other hunk yields a grounded, reachable defect.

No findings.

[OPUS-REVIEWED] 71cc4ce

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 18, 2026
@iamwhatever iamwhatever added the needs-pr-triage PR scanner: awaiting automated triage label Aug 24, 2026
@chenmingwei23 chenmingwei23 added needs-author-decision PR blocked on author input and removed needs-pr-triage PR scanner: awaiting automated triage labels Aug 24, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor

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):

  • RFC / schema-surface ruling. First Principles Review (BLOCK) argues RecipeDependencies.capabilityPackages names a mechanism core stubs to 503 (zero in-tree consumers) and should be deleted from the public recipes schema, and that _write_routing_locked duplicates the existing _persist_channel_config writer. Both are subtraction/approach calls on a public manifest contract -- not something I can decide without changing your design direction. You already ask maintainers to rule on the Recipes: seams so an edition can install Slack-channel and cron recipes #4196 RFC question ("Please rule on that before spending review time on the code") -- that ruling gates this.
  • _owner provenance model. Design Review (CONCERNS) notes _owner written raw into slack.channels[id] is erased by the next typed ChannelConfig save, so the documented "app finds/removes its own entries at uninstall" only holds until any other config write. Choose: persist it through ChannelConfig (teach it to preserve the key) vs. drop _owner and document the app ledger as the sole provenance record.
  • Doc/comment contradiction. manifest.py says the built-in recipes app owns install via the recipes CLI while the shipped doc says "core ships no recipes implementation" -- pick one story.

Mechanical items (fixable once the above is settled): GPT 5.6 (BLOCKING) flags corrupt-config clobbering the whole document in slack_routing.py:198 (needs a fail-closed write), a "recipes":{"slack":null} install crash in manifest.py:1027, ignored camelCase everySecs/persistentSession keys at manifest.py:994, and function-local imports violating the top-level-imports gate. These have obvious fixes but resolving them before the schema/approach ruling risks churning code the design decision may remove.

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 pr-no-autofix label.

@bolichen97
bolichen97 enabled auto-merge (squash) August 24, 2026 06:59
@czziemba

Copy link
Copy Markdown
Author

On the suggestion to reuse the shared CHANNEL_ID_RE instead of the local pattern in slack_routing.py: I looked at how the shared one is actually used, and I think the evidence argues for keeping the local pattern. Pushing back with the reasoning rather than silently declining.

The shared pattern is never used on its own. CHANNEL_ID_RE (validation.py:148) is ^[CDGW][A-Z0-9]+$, which is unbounded in length. All four call sites in the repo pair it with an explicit length cap:

validation.py:2168   FieldSpec("channel", str, max_len=CHANNEL_MAX_LEN, pattern=CHANNEL_ID_RE)
validation.py:2462   FieldSpec("channel", str, max_len=CHANNEL_MAX_LEN, pattern=CHANNEL_ID_RE)
validation.py:2556   FieldSpec("channel", str, required=True, max_len=CHANNEL_MAX_LEN, pattern=CHANNEL_ID_RE)
validation.py:2742   FieldSpec("channel", str, max_len=CHANNEL_MAX_LEN, pattern=CHANNEL_ID_RE)

So the repo's actual convention is not "use this regex", it is "use this regex plus a length bound" (CHANNEL_MAX_LEN = 20, validation.py:149). This endpoint has no FieldSpec wrapper to carry that bound, and the value becomes a key in config.json (slack.channels[channel_id]). Dropping in the bare shared regex here would therefore be strictly less safe than what the PR has: it would accept an unbounded string as a persisted config key. The local ^[CG][A-Z0-9]{2,31}$ folds the character class and the length bound into a single compiled pattern, which is the same intent the FieldSpec pairs express.

The [CG] vs [CDGW] narrowing is deliberate, not an oversight. Recipes create and route private channels, so C and G are the only meaningful targets. D is a DM and W is an enterprise/workspace token; routing a recipe to either is not a supported operation, and accepting them would let a caller point routing at a target the installer never creates and cannot clean up.

One concession. My upper bound of 31 is more generous than the repo's CHANNEL_MAX_LEN of 20. I picked 31 as headroom for longer Enterprise Grid ids, but I do not feel strongly about it, and there is a fair argument that parity with the existing constant beats my speculative headroom. Happy to tighten it to 20, or to import CHANNEL_MAX_LEN and compose it explicitly, if you would prefer the shared constant to be the single source of that number.

auto-merge was automatically disabled August 25, 2026 18:34

Head branch was pushed to by a user without write access

@czziemba

Copy link
Copy Markdown
Author

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. RecipeDependencies / capabilityPackages comes out of the public recipes schema. The reasoning matches the First Principles argument: CapabilityManager.available() is False in OSS so /api/capability/* answers 503, and there are no in-tree consumers, so this is a one-way-door field naming a mechanism core does not implement. Better to add it later when something consumes it than to freeze it into a public manifest contract now.

2. Writer approach: route through the existing writer. _write_routing_locked is deleted rather than patched. The endpoint delegates to slack.handler._persist_channel_config via chat_utils.run_config_write, the helper that holds both the in-process asyncio lock and the sidecar flock. _persist_channel_config gains a remove: bool parameter and returns the list of fields it actually changed; the four existing callers ignore the return, so they are unaffected.

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 json.loads + atomic_write while holding only one of the two locks, which is precisely the race this endpoint was introduced to remove. Going through run_config_write also makes it fail closed, so an unreadable config.json returns a coded 500 instead of writing a {} baseline over every other setting.

3. _owner provenance: drop it. Of the two options, persisting it through ChannelConfig means adding a field to a shared core dataclass purely to serve an out-of-tree app, and the Design Review finding is correct that the raw key does not survive a typed save. The recipe-installing app already records channel_id per installed recipe in its own ledger, so that ledger is the sole provenance record. The feature doc now says core keeps no provenance, rather than promising one that decays on the next unrelated config write.

4. Doc/comment contradiction: the doc is right. Core ships no recipes implementation. The stale manifest.py comment claiming a built-in recipes app owns the install lifecycle predates the core/app split and is removed.

On the mechanical items, all four are handled: the "recipes": {"slack": null} crash (.get("slack") or [] rather than a .get default, since an explicit JSON null returns None and iterating it raises out of install_app), the camelCase keys (everySecs and persistentSession; note the pre-existing top-level crons block keeps its own snake_case names, since that is shipped contract and out of scope here), and the function-local imports.

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. handlers/_shared.py and handlers/files.py already import slack.handler at module scope, and handlers/__init__ imports both, so there is no cycle and no added startup cost. One nuance worth recording because it is not obvious: _persist_channel_config is reached through a module reference (from kiro_crew.slack import handler as slack_handler) rather than a bare from ... import, because binding the bare name at import time defeats patch("kiro_crew.slack.handler._persist_channel_config") and would have made the regression test that proves the delegation pass vacuously.

Separately, I have posted a reply above pushing back on the shared-CHANNEL_ID_RE suggestion with the reasoning, in case a maintainer wants to overrule it.

The RFC question in #4196 remains the one genuinely outstanding item, and is not mine to settle.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 25, 2026
@bolichen97
bolichen97 enabled auto-merge August 30, 2026 00:08
@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 3, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This 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

  • This PR is OVERLAPPING with PR #7249. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #4199: REBASE. Not duplicate work -- 7249 extends the existing cron declaration, 4199 introduces a parallel one -- but the duplicated-yet-renamed field set is a public-contract question worth resolving before the recipes vocabulary is frozen. Files: src/kiro_crew/apps/manifest.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.
  • This PR is OVERLAPPING with PR #7423. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #4199: REBASE. The merged PR does not implement any part of 4199's behavior -- it reshaped the same regions of apps/manifest.py and the same test fixture, producing a real content conflict, and it set a precedent (contributes namespace, signing_payload coverage) that 4199 should be reconciled with rather than merged past. Files: src/kiro_crew/apps/manifest.py, test/test_app_bridges.py.
  • This PR is OVERLAPPING with PR #7955. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #4199: REBASE. No duplicated code and no shared hunk, but three concurrent manifest-vocabulary additions (7423 merged, 7955, 7975) all extend contributes while 4199 does not; that placement decision belongs in one conversation rather than being decided by merge order. Files: src/kiro_crew/apps/manifest.py.
  • This PR is OVERLAPPING with PR #7975. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #4199: REBASE. Complementary features that collide only on manifest-schema placement and on textual proximity in from_dict/to_dict; worth aligning, not closing. Files: src/kiro_crew/apps/manifest.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.

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.
@bolichen97

Copy link
Copy Markdown
Collaborator

Rebased onto main 6fa5519c by a maintainer as part of the 2026-09-08 open-PR audit (was 1891 commits behind, mergeable_state: dirty).

Conflicts resolved:

Gates run locally on changed files: isort, flake8, pytest (test_app_bridges, test_app_manifest_recipes, test_slack_routing, test_spawn_audit, test_app_manifest, test_docs_lint_fact_checks) all pass. black reports these three files unformatted, which was already true on main (they are in .github/black-baseline.txt).

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 recipes belongs under contributes) is untouched. Reply if anything looks wrong.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor) needs-author-decision PR blocked on author input readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Recipes: seams so an edition can install Slack-channel and cron recipes

5 participants