Skip to content

feat: add contributes.fileMenuItems app-manifest contribution - #7955

Merged
bolichen97 merged 1 commit into
kirodotdev:mainfrom
suhasaitham22:feat/menu-item-registry-seam
Sep 8, 2026
Merged

feat: add contributes.fileMenuItems app-manifest contribution#7955
bolichen97 merged 1 commit into
kirodotdev:mainfrom
suhasaitham22:feat/menu-item-registry-seam

Conversation

@suhasaitham22

@suhasaitham22 suhasaitham22 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

contributes on main carries commands, 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 in MarkdownPanel / PierreWorkspaceTreeImpl / FolderPanel, so today the only way to add a row is to edit core.

What changed

contributes.fileMenuItems[] — a second field on the existing Contributes, alongside commands:

"contributes": {
  "fileMenuItems": [{
    "id": "send-to-store",
    "label": "Send to store",
    "icon": "Package",
    "endpoint": "/api/apps/<app>/send",
    "surfaces": ["file-overflow", "tree-context", "folder-row"],
    "when": { "extensions": ["md"], "kinds": ["file"] }
  }]
}
  • Backend (apps/manifest.py) — FileMenuItemConfig + FileMenuWhen + fileMenuItems on Contributes, threaded through its to_dict / from_dict / validate. commands is untouched: both fields parse, serialize and validate side by side. Caps and malformed-input reporting mirror the commands precedent (bad_file_menu_items / dropped_file_menu_items / bad_surfaces, _MAX_FILE_MENU_ITEMS_PER_APP, label bounded by _MAX_TITLE).
  • Endpoint safety — the /api/apps/<app>/ allowlist is now one shared helper, manifest.app_endpoint_allowed(), called by manifest validation and by collect_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/shutdown never reaches the dashboard. signing_payload covers fileMenuItems, because endpoint decides where a chosen path is sent — tampering must break the signature.
  • Frontend (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 second list_apps() disk walk. when is evaluated by core; no live callback crosses the app boundary.
  • Render sitesMarkdownPanel (file-overflow, rows in a trailing group after the core options), PierreWorkspaceTreeImpl (tree-context, returning null when no row survives when), FolderPanel (folder-row).
  • Dispatch is path-only. Activating a row POSTs {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

  • contributes already exists on main — correct, and the earlier revision was wrong to add a second class. fileMenuItems is now a field on main's Contributes; commands keeps parsing, serializing and validating, pinned by a round-trip test asserting both survive together. Rebased onto main.
  • File content reaching an app with no declared permission — fixed by removing content from the contract entirely (path-only), which needs no new permission surface. Two tests assert the dispatched payload carries no content.
  • Bespoke endpoint duplicating an existing flowGET /api/file-menu-items, its aggregator, handler, route registration and the second client query are all deleted; routes.py is net negative.
  • SSRF allowlist copy-pasted — extracted to one helper, called from both paths.
  • No caps — added, and mirrored as constants in the frontend module (label length measured to match the renderer's UTF-16 count).
  • Silent coercionbad_* / dropped_* flags reported from validate(), mirroring commands. No test asserts silent degradation.
  • Tests (the prior finding still open) — added file-overflow tests in MarkdownPanel.test.tsx (row renders, path-only POST, rows render last, data-option present, when honoured, N>1, disabled app, inert empty case) and tree-context tests in PierreWorkspaceTreeImpl.test.tsx (the renderContextMenu gate, the null return when no row survives, the querySelector focus fallback with no built-in row, path+root dispatch, disabled app), plus surface-filter and resolver coverage.
  • Nits — the documented icon example is a real AppIcon name 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. flake8 clean; mypy --platform linux clean on the changed files; black gate passes; scrub-lint's working-tree scan is clean. error-code-baseline.json needs no change and there is no route census — the endpoint is deleted, not added.

Frontend tsc / vitest could 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.

@suhasaitham22
suhasaitham22 requested a review from a team September 2, 2026 18:24
@suhasaitham22
suhasaitham22 requested a review from a team as a code owner September 2, 2026 18:24
@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 Sep 2, 2026
@suhasaitham22
suhasaitham22 force-pushed the feat/menu-item-registry-seam branch from a8dec9f to 9e27b8a Compare September 2, 2026 19:31
@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 Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — 🟡 CONCERNS

UX-level review of 2ed981beb3ecae78509325dccdbc92e38d64d0f7 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I've read the full patch (backend manifest field, three render sites, resolver module, i18n strings, tests) and the PR description. The PR declares <!-- no-visual-delta --> because the stock build renders nothing, but the diff does add user-visible controls — contributed menu rows with an "App: {{app}}" attribution, a hover-revealed "App actions" kebab on folder rows, and two new dispatch-failure ErrorNotices — none of which any screenshot or blind reader has seen. My final review follows.

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

  • No pending/success feedback on activation: invokeFileMenuItem is called void … .catch(onError) and every surface closes its menu on select, so a successful "Send to store" produces nothing visible — a reader can't tell in-flight from done and may re-click, double-POSTing the path. Core rows in the same overflow menu (add-to-knowledge, promote) show success flags with delayedClose; contributed rows are the odd ones out. Frequent (every successful use) × friction/duplicate dispatch × every time. Fix: reuse the menu's existing success-flag pattern, or surface a brief success notice through the same channel failures use.
  • Raw-code fallback in the failure notice: throw new ApiError(r.status, errText || \HTTP ${r.status}`)` puts a bare "HTTP 502" in the ErrorNotice when the app returns no body — what happened but not what to do. Fix: wrap in a catalog string, e.g. "The app didn't accept this action (HTTP 502)".

Evidence gaps

  • Contributed row in the file-overflow menu (separator + icon + label + "App: doc-store" attribution) — screenshot with a demo app installed.
  • Tree-context menu with contributed rows, including the app-only menu (no "Add to chat") this PR newly makes reachable.
  • Folder-row "App actions" kebab (hover-revealed) and its open dropdown.
  • Dispatch-failure ErrorNotice in FolderPanel and above the workspace tree.
  • Long app label truncating inside the 420px/320px menu caps — narrow-viewport screenshot.
  • No blind read ran (fork lane), so first-time comprehension of "App actions" and the "App: {{app}}" attribution is unestablished.

[UX-REVIEWED] 2ed981b

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of 2ed981beb3ecae78509325dccdbc92e38d64d0f7 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound seam that follows the contributes precedent end-to-end; the residual risk is a hand-maintained core-route denylist whose drift guard is a source-text regex.

Watch

  • CORE_APP_ROUTE_SEGMENTS is the only thing stopping a manifest from pointing a clicked row at a core lifecycle handler inside the app's own namespace ("a manifest naming one would pass the prefix test and have the host POST to a core handler with the reader's own session"), and it exists as three hand-synced artifacts — a Python set, a TS set, and a regex scan keyed to the {name|app|app_name} param spellings. A future core route registered through an f-string constant or a new param name escapes the scan silently, and the set — not the scan — is what the allowlist enforces. Reversible (add the segment), but it fails open until noticed.
    Clears when: the drift test enumerates the reserved segments from the actual route table (boot setup_routes and introspect the router) instead of regexing source, or a human accepts the textual guard explicitly.
  • Undocumented hunk: website/src/apps/file-explorer/styles.ts adds min-width:0 to .mc-fe-tab-label — the file-explorer tab is not one of the three contributed-row surfaces and the description never mentions it.
    Clears when: the hunk is named in the description or moved to its own PR.

Suggestions

  • Derive both frontend mirrors (CORE_APP_ROUTE_SEGMENTS, the endpoint character class) from one generated artifact rather than pinning each pair with a bespoke source-reading test; the reserved-segments pin already reads coreAppRoutes.ts textually, and the regex pair has no pin at all.

[DESIGN-REVIEWED] 2ed981b

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 2ed981b

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of 2ed981beb3ecae78509325dccdbc92e38d64d0f7 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. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • 5 — undeclared: framed as pure extraction ("its inline copy is removed, so there is one implementation of the control"), but the helper is strictly tighter than the inline copy it replaces — publish providers now also fail on core segments, reserved app names, and the character allowlist.
  • 7 — undeclared: the core file-editor menu gains a 420px cap and row clipping in the stock build; the description claims "No user-visible change in the stock build."
  • 8 — rides along, undeclared: .mc-fe-tab-label is the file-explorer tab strip, not one of the three surfaces this PR touches (grepped: 3 hits, all in apps/file-explorer/).

What this change ships

Intent: let an installed app add rows to the file, tree, and folder menus from its manifest alone, without editing core — an ADDITION.

  1. Apps can declare contributes.fileMenuItems; validated, capped, signature-covered — justified
  2. File-editor ⋮ menu renders app rows last, attributed "App: " — justified
  3. Tree right-click menu shows app rows; now opens even without the add-to-chat host — justified
  4. Folder-panel rows gain a hover kebab holding app actions — justified
  5. Endpoint allowlist unified into app_endpoint_allowed; publish providers tightened — undeclared
  6. Row activation POSTs path-only to the app's endpoint, redirects refused — justified
  7. Stock file-editor menu capped at 420px, rows clip — undeclared
  8. File-explorer tab-label min-width:0 — rides along, undeclared
  9. "App actions" / "App: {{app}}" strings in every locale — justified
  10. coreAppRoutes.ts mirror module plus its file-scoped i18n lint release — justified
    More than 10 differences exist (dispatch-failure notices on tree and folder panel, extension-seams doc note); these are the 10 most noticeable.

No duplicate mechanism exists: base has zero fileMenuItems hits, no menu seam in extensions.ts, and the three menus are hardcoded as claimed. The inline allowlist in routes.py:416-423 is real and genuinely deleted — one control now, not two.

Watch

  • Publish-provider behavior change: the removed inline check tested only traversal + prefix; app_endpoint_allowed adds CORE_APP_ROUTE_SEGMENTS, RESERVED_APP_PATH_SEGMENTS, and _ENDPOINT_ALLOWED_RE, so an already-published provider endpoint that previously passed can now be dropped with only a log warning. Clears when: the description declares the tightening and confirms no existing provider endpoint is newly refused.
  • The "no-visual-delta" claim vs items 7–8: two stock-build style changes ship under a description asserting none. Clears when: the rider is dropped and the cap is declared as a stock change.

Subtractions

  • Drop the website/src/apps/file-explorer/styles.ts hunk (min-width:0 on .mc-fe-tab-label) — unrelated surface; land it, if wanted, in its own change.

[FIRST-PRINCIPLES-REVIEWED] 2ed981b

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] 2ed981b

@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 Sep 3, 2026

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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[] in app.json with id, label, icon, surfaces: ['file-overflow'|'tree-context'|'folder-row'], endpoint (or command), and when filters; 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-sdk barrel and add unregister (a runtime-loaded contributor must be able to withdraw rows on unmount).

Defects on 9e27b8a

  • website/src/apps/menuContributions.tsx:83 — validation covers label/onSelect but not visible/disabled. A visible: true descriptor passes registration and throws at mi.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 reads mi.disabled, while MarkdownPanel.tsx:513 does. Same descriptor is gated in one menu and live in the other, and onSelect fires in a state the contributor declared invalid.
  • PierreWorkspaceTreeImpl.tsx:107-147TreeContextMenu never returns null; with no onAddToContext and every item's visible() false, right-click opens an empty bordered popup and the querySelector('[role="menuitem"]') fallback has nothing to focus.
  • PierreWorkspaceTreeImpl.tsx:168-170 — the onAddToContext doc ("Absent → still shown but inert") was already false in base and is still wrong here; fix it while touching the file.
  • menuContributions.tsx:41-44group has zero readers; order cannot deliver its documented semantics because built-ins are hardcoded JSX outside the registry at both sites (order < 0 cannot land before them). Drop both or implement them.
  • website/docs/extension-seams.md:14 — "extensionSeams.test.tsx exercises 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.tsx is untouched, so the FolderPanel row buttons still need a core edit. Also "one non-additive line" is +43/-13 across three edit sites in PierreWorkspaceTreeImpl.tsx (:87-88, :112-124, :386) — rendered stock output is unchanged, so fine, but please state it accurately.
  • Tests: menuContributions.test.ts covers registration/sort/collision but has no malformed visible/disabled case (the blocking bug) and no render test that registers an item and asserts the row appears, onSelect fires, and disabled is 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.

@suhasaitham22

Copy link
Copy Markdown
Contributor Author

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

  • Manifest-declared contributes.fileMenuItems[]id, label, icon, surfaces: ['file-overflow' | 'tree-context' | 'folder-row'], endpoint, when. Core reads the declaration and POSTs the file context to the app's endpoint — no live callbacks cross the app boundary — mirroring the publishProvider path end-to-end: apps/manifest.py → aggregator + GET endpoint in apps/routes.py with the same /api/apps/<app>/ endpoint allowlist → client method/type → render + activation.

One grounding note vs. the review wording

  • There is no contributes block in the manifest today; the endpoint-dispatch precedent is the top-level singular publishProvider. So contributes is being introduced fresh here as the grouping for these declarations (rather than extending something that already exists) — flagging since the review cited contributes.commands/publishProviders as precedent.

Defects I'll fix regardless of shape

  • tree menu honoring disabled; TreeContextMenu returning null when it would render zero rows; the onAddToContext doc; dropping/implementing group/order; the missing FolderPanel surface (3rd promised surface); render + negative tests; and the Screenshot Evidence markers.

Will push the reshaped revision shortly.

@suhasaitham22
suhasaitham22 force-pushed the feat/menu-item-registry-seam branch from 9e27b8a to 1d86835 Compare September 3, 2026 22:33
@suhasaitham22 suhasaitham22 changed the title feat: add edition menu-item contribution seams feat: add contributes.fileMenuItems app-manifest contribution Sep 3, 2026
@suhasaitham22

Copy link
Copy Markdown
Contributor Author

Pushed the declarative reshape (1d86835). Summary of how each point is addressed:

  • App-reachable (direction): now a manifest contributes.fileMenuItems[] block — core reads the declaration and POSTs the file context to the app's endpoint (held to the /api/apps/<app>/ allowlist), no app code imported. contributes is introduced fresh, since the only existing endpoint-dispatch precedent is the singular top-level publishProvider (there's no contributes.commands today).
  • disabled ignored / group+order dead contract / empty context menu: the live callbacks are gone entirely — when (extensions + kinds) is core-evaluated — and TreeContextMenu now returns null when it would render zero rows. onAddToContext doc corrected.
  • Third surface: folder-row is implemented in FolderPanel.
  • Tests: backend parse/aggregator/allowlist (incl. traversal + prefix-collision) + a frontend when/render/POST suite.

One thing worth your eyes: row icons currently reuse AppIcon, whose ICON_MAP is a small name allowlist — an icon name outside it falls back to a generic glyph. Happy to widen the map or take a different icon source if you'd prefer. tsc/vitest run in CI (no npm egress on the authoring host).

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.fileMenuItemsAppManifestlist_apps()GET /api/file-menu-itemsuseFileMenuItems → 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.

@github-actions github-actions Bot added readiness: checking Automated validation is still running merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@suhasaitham22
suhasaitham22 force-pushed the feat/menu-item-registry-seam branch from 1d86835 to 137c227 Compare September 4, 2026 00:28
@suhasaitham22

Copy link
Copy Markdown
Contributor Author

Rebased onto main and pushed (137c2276). Point by point:

Blocking

  1. contributes is not net-new — you're right, and thanks for catching it before merge; feat(command-bar): let an installed app contribute command rows #7423 landed ~3h after this commit was authored. Rebased; fileMenuItems is now a second field on main's existing Contributes, and the duplicate class, duplicate _KNOWN_FIELDS entry and duplicate AppManifest field are deleted. commands keeps parsing/serializing/validating — a round-trip test pins that both survive together ({'commands', 'fileMenuItems'}). The "mirrors publishProvider" rationale is gone; Contributes/commands is the precedent now.
  2. mergeable_state / no CI — the rebase clears the dirty state; this push is the first run of any lane. Frontend tsc/vitest still can't run on my host (npm ci --offlineENOTCACHED), so CI is the gate and I'll drive whatever it reports.
  3. Caps_MAX_FILE_MENU_ITEMS_PER_APP = 10 plus a label cap reusing _MAX_TITLE, both mirrored as constants in fileMenuContributions.tsx; the label is measured so the count matches the renderer's UTF-16 view.

Should fix
4. Silent coercionbad_file_menu_items / dropped_file_menu_items (plus bad_surfaces and when.bad_fields) reported from validate(), mirroring commands. No test asserts silent degradation.
5. File content with no declared permission — took the simpler branch: content is out of the contract entirely. Dispatch is {item_id, surface, path, kind?, root?}; an app that needs bytes reads them through its own permitted route. Two tests assert no content in the payload.
6. Bespoke endpointGET /api/file-menu-items, collect_file_menu_items, the handler, the route registration and the second client query are all deleted. Rows read off the shared ['apps'] query as a non-fetching cache subscriber (enabled: false), same posture as the Command Bar. routes.py is net negative.
7. SSRF allowlist — extracted as manifest.app_endpoint_allowed(), one implementation, called by manifest validation and by collect_publish_providers (its inline copy removed). It lives in manifest.py to avoid an import cycle.
8. Tests — file-overflow in MarkdownPanel.test.tsx (renders, path-only POST, rows last, data-option, when, N>1, disabled app, inert-empty) and tree-context in PierreWorkspaceTreeImpl.test.tsx (the renderContextMenu gate, null when no row survives when, the querySelector focus fallback with no built-in row, path+root dispatch, disabled app), plus the surface filter.

Nits — 9: example is now "Package" and the 9-name AppIcon set is documented. 10: stale comment rewritten. 11: app rows moved to a trailing group after the core options, separator only when rows exist.

Also, unprompted: signing_payload now covers fileMenuItems (endpoint decides where a chosen path is sent, so tampering must break the signature; bytes unchanged for commands-only apps, pinned by test), and the endpoint allowlist is enforced at install in AppManifest.validate() — with no server-side aggregator left to filter, that's what stops a row naming /api/shutdown.

Your unconfirmed item resolves to "nothing moves": error-code-baseline.json has no file-menu entry and there's no route-census script — and the endpoint is deleted rather than added.

@dwu96

dwu96 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

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

  • ## Why it matters

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
@iamwhatever

Copy link
Copy Markdown
Collaborator

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

  • ## Why it matters

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.

@suhasaitham22

Copy link
Copy Markdown
Contributor Author

Description updated for the template gate — added a Why it matters section, and renamed Tests / verification to Tests so the required heading matches exactly. All four enforced sections are now present (Problem / Motivation, Why it matters, What changed, Tests), so the workflow runs should auto-approve on the next cycle.

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.

@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 4, 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: checking Automated validation is still running labels Sep 6, 2026
@bolichen97
bolichen97 force-pushed the feat/menu-item-registry-seam branch from fc6b3a3 to b27d340 Compare September 6, 2026 18:35
@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 Sep 6, 2026
@bolichen97
bolichen97 force-pushed the feat/menu-item-registry-seam branch from b27d340 to 1cfe14c Compare September 6, 2026 18:55
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 6, 2026
@bolichen97
bolichen97 enabled auto-merge (squash) September 6, 2026 19:49
@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 6, 2026
bolichen97
bolichen97 previously approved these changes Sep 7, 2026
chenmingwei23
chenmingwei23 previously approved these changes Sep 7, 2026
@bolichen97
bolichen97 dismissed stale reviews from chenmingwei23 and themself via 9eb10b4 September 7, 2026 02:48
@bolichen97
bolichen97 force-pushed the feat/menu-item-registry-seam branch from 1cfe14c to 9eb10b4 Compare September 7, 2026 02:48
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: passed Eligible automated validation passed for the current revision readiness: action required A blocking check or review needs attention labels Sep 7, 2026
bolichen97
bolichen97 previously approved these changes Sep 7, 2026
chenmingwei23
chenmingwei23 previously approved these changes Sep 7, 2026
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.
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)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants