feat: add contributes.fileMenuItems app-manifest contribution - #7955
Conversation
a8dec9f to
9e27b8a
Compare
UX Review (Fable 5, fork) — 🟡 CONCERNSUX-level review of I've read the full patch (backend manifest field, three render sites, resolver module, i18n strings, tests) and the PR description. The PR declares UX-Verdict: CONCERNS Activating a contributed row is fire-and-forget — the menu closes and success is silent — and no first-time reader has seen any new surface. Watch
Evidence gaps
[UX-REVIEWED] 2ed981b |
Design Review (Fable 5, fork) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound seam that follows the Watch
Suggestions
[DESIGN-REVIEWED] 2ed981b |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed |
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of First-Principles-Verdict: CONCERNS The shared endpoint helper silently tightens publish-provider validation, and a stock-build CSS rider contradicts the "no user-visible change" claim. Not justified as shipped
What this change shipsIntent: let an installed app add rows to the file, tree, and folder menus from its manifest alone, without editing core — an ADDITION.
No duplicate mechanism exists: base has zero Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 2ed981b |
GPT 5.6 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
bolichen97
left a comment
There was a problem hiding this comment.
Thanks Suhas — the seam shape is precedent-faithful (reportSeamCollision, MENU_ITEM_ID_RE mirroring PROVIDER_ID_RE, data-option + role="menuitem" rows), and the diff is clean of any product-specific naming. Requesting changes for two reasons: a few real defects on this head, and a direction change on my side that affects who this registry must serve.
Direction (affects the contract, not just this PR)
I've decided Pippin should ship as a standalone app (own repo, catalog entry) end to end — backend, page, and the in-chat pieces — rather than "app backend + edition-owned thin frontend". That means the upstream seam has to be reachable by an installed app, not only by the edition build.
As written it is edition-only: website/src/app-sdk/shared-modules.ts publishes a closed module set and menuContributions is not re-exported from the app-sdk barrel, so an app bundle has no import path to registerFileOverflowMenuItems / registerTreeContextMenuItems. The existing app-reachable precedents are messageRenderers (barrel-exported "so an app can add a row type") and the manifest-declared publishProviders / contributes.commands (core reads a declaration and calls the app's endpoint, never imports app code). Please pick one of those shapes:
- Declarative (preferred, matches
contributes.commands):contributes.fileMenuItems[]inapp.jsonwithid,label,icon,surfaces: ['file-overflow'|'tree-context'|'folder-row'],endpoint(orcommand), andwhenfilters; core renders the rows and POSTs the file context to the app. No live callbacks cross the app boundary. - Programmatic: export the registry through the
app-sdkbarrel and addunregister(a runtime-loaded contributor must be able to withdraw rows on unmount).
Defects on 9e27b8a
website/src/apps/menuContributions.tsx:83— validation coverslabel/onSelectbut notvisible/disabled. Avisible: truedescriptor passes registration and throws atmi.visible(mctx)in the render loop, into the app-shell error boundary. GPT's blocking finding is correct; validate all four callbacks.website/src/pierre/PierreWorkspaceTreeImpl.tsx:129-146— the tree render never readsmi.disabled, whileMarkdownPanel.tsx:513does. Same descriptor is gated in one menu and live in the other, andonSelectfires in a state the contributor declared invalid.PierreWorkspaceTreeImpl.tsx:107-147—TreeContextMenunever returnsnull; with noonAddToContextand every item'svisible()false, right-click opens an empty bordered popup and thequerySelector('[role="menuitem"]')fallback has nothing to focus.PierreWorkspaceTreeImpl.tsx:168-170— theonAddToContextdoc ("Absent → still shown but inert") was already false in base and is still wrong here; fix it while touching the file.menuContributions.tsx:41-44—grouphas zero readers;ordercannot deliver its documented semantics because built-ins are hardcoded JSX outside the registry at both sites (order < 0cannot land before them). Drop both or implement them.website/docs/extension-seams.md:14— "extensionSeams.test.tsxexercises each one except the source-provider seam" is now false; add the block or amend the sentence.- Scope: the PR description promises three surfaces;
FolderPanel.tsxis untouched, so the FolderPanel row buttons still need a core edit. Also "one non-additive line" is +43/-13 across three edit sites inPierreWorkspaceTreeImpl.tsx(:87-88,:112-124,:386) — rendered stock output is unchanged, so fine, but please state it accurately. - Tests:
menuContributions.test.tscovers registration/sort/collision but has no malformedvisible/disabledcase (the blocking bug) and no render test that registers an item and asserts the row appears,onSelectfires, anddisabledis honored. - CI: Screenshot Evidence is red — add the
<!-- no-visual-delta -->+**Why no screenshot:**markers or a screenshot.
Happy to pair on the app-reachable shape.
|
Thanks @bolichen97 — adopting the direction. Reshaping this from the edition-only programmatic registry to a declarative, app-reachable contribution, so an installed standalone app (not just the edition build) can add these rows and the edition registers nothing. Plan for this PR
One grounding note vs. the review wording
Defects I'll fix regardless of shape
Will push the reshaped revision shortly. |
9e27b8a to
1d86835
Compare
|
Pushed the declarative reshape (
One thing worth your eyes: row icons currently reuse |
bolichen97
left a comment
There was a problem hiding this comment.
Thanks for the reshape — the seam is now the right shape: declarative, manifest-owned, endpoint-dispatched, edition registers nothing, and every prior code-level finding except the test gap is genuinely fixed. The app-reachability trace is correct by design (app.json contributes.fileMenuItems → AppManifest → list_apps() → GET /api/file-menu-items → useFileMenuItems → three render sites → POST to the app's own endpoint), and with no contributing app the stock output is byte-identical to main.
It is blocked by one fact that invalidates the PR's central premise, plus two things that follow from it.
Blocking
1. contributes is not net-new — this PR redefines a class that already exists on main, and the merge silently deletes contributes.commands.
#7423 (147010ce, merged 2026-09-03, ~3h after your commit) landed class Contributes with commands: list[CommandContribution] at src/kiro_crew/apps/manifest.py:1556, plus "contributes" in _KNOWN_FIELDS (:1675) and the contributes: field on AppManifest (:1744). This PR adds a second class Contributes (~1146, fileMenuItems only), a second _KNOWN_FIELDS entry, and a second AppManifest field. Post-merge the module defines the name twice; whichever wins, one of commands / fileMenuItems stops parsing and serializing, to_dict() drops it, and discovery.py:64-68 writes that truncated dict into the persisted app.json — no error anywhere. The new validation loop (for item in self.contributes.fileMenuItems) raises AttributeError against main's class on every manifest parse. The publishProviders mirroring rationale in the docstring no longer applies; Contributes is the precedent now.
Fix: rebase onto main; add fileMenuItems: list[FileMenuItemConfig] to the existing Contributes, extend its to_dict/from_dict/validate; delete the new class, the duplicate _KNOWN_FIELDS entry, and the duplicate AppManifest field. Good news: the backend→persisted-manifest→/api/apps→frontend exposure path already exists and is proven, so this is mostly deletion.
2. mergeable_state: dirty, and no CI lane has run.
Head 1d86835: combined status pending, 4 queued + 1 skipped fork-gate checks, none of build / typecheck / vitest / pytest / eslint ceiling / coverage / error-code ratchet, none of the 5 review bots. With no local build on your side either, nothing here has been compiled by anyone yet. Rebase (resolving #1 as an extension, not a merge of two classes), push, and get a green run.
3. No caps, where the sibling field's caps are load-bearing.
Main's Contributes carries _MAX_COMMANDS_PER_APP = 20, _MAX_KEYWORDS, MAX_TITLE = 120, each explicitly mirrored by a constant in contributedCommands.ts. fileMenuItems caps nothing: array length, label, icon are unbounded, and every row renders in three menus.
Fix: _MAX_FILE_MENU_ITEMS_PER_APP + a label-length cap, mirrored in fileMenuContributions.tsx.
Should fix
4. Silent coercion the neighbouring code was hardened against. Contributes.from_dict does items_raw if isinstance(items_raw, list) else [] and FileMenuItemConfig.from_dict drops non-dict entries. Main's comment at manifest.py:1567-1580 states exactly why that is wrong (indistinguishable from a deliberate empty list; author sees no error and no rows). Carry bad_file_menu_items / dropped_file_menu_items and report them from validate(), as commands does.
5. File content reaches an app with no declared permission. MarkdownPanel.tsx ~508 POSTs the whole file content; PierreWorkspaceTreeImpl.tsx ~142 POSTs absolute path + root. The manifest has a permissions block and nothing here requires one — activating a row hands an app arbitrary file content with no consent step and no install-time declaration. Gate content behind a declared permission, or POST the path only and let the app read through its own permitted route.
6. Bespoke endpoint duplicating an existing data flow. GET /api/file-menu-items (routes.py ~645) + a second query in client.ts ~3662. contributedCommands.ts:26 documents itself as "the subset of GET /api/apps this module reads" — contributes already reaches the frontend through /api/apps. This adds a second per-session request and a second list_apps() disk walk for data already on the wire. Read contributes.fileMenuItems off the existing apps query.
7. SSRF allowlist copy-pasted, not extracted. _app_endpoint_allowed (routes.py ~560) is logic-identical to the inline check in collect_publish_providers (routes.py:412-432); main is left untouched, so there are now two copies of one security control free to drift. Extract once, call from both.
8. Two of three surfaces have no render test. fileMenuContributions.test.tsx covers the when predicate and FolderRowActions (0 and 1 rows). Untested: file-overflow rows in MarkdownPanel.tsx ~498; tree-context rows plus the if (!onAddToContext && rows.length === 0) return null early return (PierreWorkspaceTreeImpl.tsx ~109), the firstItemRef ?? querySelector focus fallback (~85-90), the renderContextMenu gate (~393); the useFileMenuItems surface filter; any N>1 case; a route-level test; and an "empty registry is inert" assertion. This is the one prior finding (#9) still only partially addressed.
Nits
9. Documented icon example "icon": "Send" is not in AppIcon's ICON_MAP (Shield, Bot, Search, Tag, Users, Zap, Star, Package, Cat), so the canonical example silently renders Package. Use a real name and document the allowed set.
10. PierreWorkspaceTreeImpl.tsx ~84 still says "with exactly one item the arrows have nothing to move between", which this PR makes false — same stale-comment class flagged last round.
11. MarkdownPanel.tsx ~498 inserts app rows above the core "View options" section; render them last or in their own trailing group.
Prior comments
1 (visible/disabled), 2, 5 (group/order) — moot, removed. 3 (TreeContextMenu null return), 4 (stale onAddToContext doc), 7 (FolderPanel surface), 8 (PR body accuracy), 10 (Screenshot Evidence markers) — addressed. 6 — moot. 9 (tests) — partial, see #8.
Not verified by me
Nothing was built or run; #1 and #3 are read from source, and "which Contributes wins" is reasoned from definition order in the merged file, not observed. Whether the error-code baseline or route census moves for the new endpoint is unconfirmed.
1d86835 to
137c227
Compare
|
Rebased onto Blocking
Should fix Nits — 9: example is now Also, unprompted: Your unconfirmed item resolves to "nothing moves": |
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
1 similar comment
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
|
Description updated for the template gate — added a Why it matters section, and renamed That description gate is also the answer to the "no CI lane has run" finding — the real lanes (build / tsc / vitest / pytest / review bots) were never approved to start, rather than having run and been skipped. Once they execute I'll drive whatever they report to green. |
fc6b3a3 to
b27d340
Compare
b27d340 to
1cfe14c
Compare
9eb10b4
1cfe14c to
9eb10b4
Compare
Add fileMenuItems as a second field on the existing Contributes block, alongside commands, so an installed app can declare rows for the file editor's overflow menu, the workspace-tree context menu, and folder rows. Core resolves the declarations off the shared /api/apps query and POSTs the file path to the app's own endpoint; no app code is imported and no file content crosses the boundary. Caps and malformed-input reporting mirror the commands precedent, and the endpoint allowlist is enforced at install time by a single shared helper now used by publishProvider too.
Problem / Motivation
contributeson main carriescommands, so a manifest-only app can already reach the Command Bar — but nothing lets an app contribute a row to the file editor's overflow (⋮) menu, the workspace-tree context menu, or a folder-panel row. Those three menus are hardcoded inMarkdownPanel/PierreWorkspaceTreeImpl/FolderPanel, so today the only way to add a row is to edit core.What changed
contributes.fileMenuItems[]— a second field on the existingContributes, alongsidecommands:apps/manifest.py) —FileMenuItemConfig+FileMenuWhen+fileMenuItemsonContributes, threaded through itsto_dict/from_dict/validate.commandsis untouched: both fields parse, serialize and validate side by side. Caps and malformed-input reporting mirror thecommandsprecedent (bad_file_menu_items/dropped_file_menu_items/bad_surfaces,_MAX_FILE_MENU_ITEMS_PER_APP, label bounded by_MAX_TITLE)./api/apps/<app>/allowlist is now one shared helper,manifest.app_endpoint_allowed(), called by manifest validation and bycollect_publish_providers(its inline copy is removed, so there is one implementation of the control rather than two free to drift). It is enforced at install time, so a row naming/api/shutdownnever reaches the dashboard.signing_payloadcoversfileMenuItems, becauseendpointdecides where a chosen path is sent — tampering must break the signature.apps/fileMenuContributions.tsx, added) — rows resolve off the shared['apps']query as a non-fetching cache subscriber (enabled: false, the same posture as the Command Bar), so there is no extra per-session request and no secondlist_apps()disk walk.whenis evaluated by core; no live callback crosses the app boundary.MarkdownPanel(file-overflow, rows in a trailing group after the core options),PierreWorkspaceTreeImpl(tree-context, returningnullwhen no row surviveswhen),FolderPanel(folder-row).{item_id, surface, path, kind?, root?}to the app's own endpoint. File content is never sent — an app that needs bytes reads them through its own permitted route.Why it matters
An installed app can add per-file and per-folder actions — sending a file to an external document store, pulling one back — from its manifest alone, without any core edit and without the edition having to register anything on its behalf. That removes the copy-and-shadow maintenance a downstream fork otherwise re-applies on every sync, and it keeps the contribution honest at the boundary: core reads a declaration and POSTs a path to the app's own endpoint, so no app code is imported into the shell and no file content leaves the host without the app reading it through its own permitted route. With no app declaring
fileMenuItems, the resolver returns empty and the stock build is byte-identical to main.Addressing the review
contributesalready exists on main — correct, and the earlier revision was wrong to add a second class.fileMenuItemsis now a field on main'sContributes;commandskeeps parsing, serializing and validating, pinned by a round-trip test asserting both survive together. Rebased ontomain.content.GET /api/file-menu-items, its aggregator, handler, route registration and the second client query are all deleted;routes.pyis net negative.bad_*/dropped_*flags reported fromvalidate(), mirroringcommands. No test asserts silent degradation.MarkdownPanel.test.tsx(row renders, path-only POST, rows render last,data-optionpresent,whenhonoured, N>1, disabled app, inert empty case) and tree-context tests inPierreWorkspaceTreeImpl.test.tsx(therenderContextMenugate, thenullreturn when no row survives, thequerySelectorfocus fallback with no built-in row, path+root dispatch, disabled app), plus surface-filter and resolver coverage.AppIconname and the allowed set is documented; the stale "exactly one item" comment is rewritten; app rows render last.Tests
22/22 backend tests pass locally.
flake8clean;mypy --platform linuxclean on the changed files;blackgate passes;scrub-lint's working-tree scan is clean.error-code-baseline.jsonneeds no change and there is no route census — the endpoint is deleted, not added.Frontend
tsc/vitestcould not run on the authoring host (no npm egress), so CI is the gate for them.Screenshots / video
Why no screenshot: No user-visible change in the stock build — no app on main declares
contributes.fileMenuItems, so the resolver returns empty and all three render sites are inert. Contributed rows appear only when an installed app declares them, which is out of scope for this PR.