Skip to content

Publish contribution API and pull invalidation - #86

Merged
ashwin-pc merged 2 commits into
mainfrom
feat/public-contributions-invalidation
Aug 8, 2026
Merged

Publish contribution API and pull invalidation#86
ashwin-pc merged 2 commits into
mainfrom
feat/public-contributions-invalidation

Conversation

@ashwin-pc

Copy link
Copy Markdown
Owner

Summary

Completes PR 3 from #82 on top of the unified contribution transport in #84.

Public contribution API

  • adds ctx.ui.web.contribute(key, spec) with explicit slot and kind
  • normalizes public specs immediately into the existing versioned internal union
  • rejects conflicting/missing delivery fields and unsupported slot/kind combinations
  • treats public contribution keys as canonical identities: replacing a key moves it between slots, and contribute(key, undefined) clears it
  • keeps every existing set* API as a typed compatibility wrapper through the same normalization path

Pull invalidation

  • adds ctx.ui.web.update(key)
  • emits session-scoped web_contribution_updated { sessionId, key } only for registered keys
  • active panel and Git-tab hosts pull a fresh render on invalidation
  • hidden/inactive surfaces do no work and render normally when next opened
  • ignores invalidations from background sessions in the active browser view

Executable policies

  • completes slot policy metadata for allowed kinds, accepted view fields/effects, byte budgets, and panel field limits
  • invocation sanitizers now consume policy-owned budgets rather than duplicating constants

Exemplar

Adds the recovered global notepad as a public-API exemplar. It uses:

  • a rendered panel contribution
  • a separate static fab launcher
  • shared Event/form grammar
  • persisted cross-session data
  • update() fan-out so writes refresh every live open notepad panel
  • explicit lifecycle cleanup

The exemplar accounts for most of the line count and is intentionally included now: unlike PR 2, invalidation is present, so it proves the live cross-session contract rather than distracting from transport review.

Assets and webviews remain deferred behind their separate security/design gates.

Testing

  • public registration, replacement, clearing, malformed-spec rejection, invocation, and invalidation unit coverage
  • browser coverage proving an open panel re-pulls after web_contribution_updated
  • all viewport coverage for panel invalidation
  • hardened an existing touch-project copy-timer test whose synthetic hover could clear before its click (visibility remains covered by the adjacent test)

Validation:

  • npm run typecheck
  • npm run test:unit ✅ — 327 tests
  • npm run build
  • focused desktop/tablet/mobile E2E ✅ — 6 tests
  • npm test ✅ — build, auth, unit, desktop, tablet, and mobile suites
    • the existing tablet attachment lifecycle diagnostic encountered its known upload/reload race and passed on automatic retry

Closes the initial public API + invalidation sequence in #82.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b3bfeb34b9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/git/panel.ts
Comment thread examples/pi-web-extensions/notepad.ts
@ashwin-pc

Copy link
Copy Markdown
Owner Author

Reviewed against the PR 3 contract from #82, independently verified on b3bfeb3: typecheck ✅, full unit suite ✅ (327), build ✅, targeted e2e ✅ (web-panel incl. the live-invalidation flow, web-footer, git — 11/11).

Contract conformance — this closes out the #82 PR 1–3 sequence properly:

  • contribute() does exactly what the maintainer-review contract demanded: explicit slot/kind, immediate normalization into the versioned union, conflicting/missing delivery fields rejected (tested), webview entry rejected loudly ("not supported yet") instead of silently ignored — the right way to reserve the future kind.
  • Policies are now genuinely executable: allowedKinds/viewBudget/maxFields consumed by the invoke paths instead of duplicated constants — this closes the "budgets still live in the invoke paths" item I flagged on Unify contribution transport and add panel/FAB slots #84.
  • update() is the RFC's pull-after-push, minimally and correctly: emits only for registered keys (tested), per-tab filtering to the current session, hosts re-pull only when the key is active and open — hidden surfaces do zero work.
  • The exemplar's cross-session fan-out (module-level invalidator set, per-session entries, WeakMap cleanup on shutdown) is the correct composition: the platform primitive stays per-session, and cross-session behavior lives in the extension — no global-broadcast primitive needed. Good validation of the design.
  • Legacy set* wrappers now route through the same normalization path — one code path, wrappers reduced to adapters.

One substantive follow-up (not merge-blocking, but it will bite the first real notepad user):

  1. Invalidation clobbers in-progress form state. webPanels.update() re-invokes with no event → "Loading…" → full re-render. A user mid-typing in the quick-add box (or inside the edit sub-view) when an agent in another session writes loses their input and gets yanked to the default view. Suggest a host-level guard: skip/defer the auto-refresh while body.contains(document.activeElement) on a form control (re-pull on blur), or show a non-destructive "updated — refresh" affordance. Worth an issue now, fix in a small follow-up.

Notes for the record (non-blocking):

  1. Cross-slot key semantics differ between contribute() and the wrappers — public API treats the key as a cross-slot identity (clears all slots), wrappers clear per-slot. Documented and self-consistent, but two sharp edges: mixing setFooter("x") with contribute("x", panelSpec) silently deletes the footer, and the registry is still extension-unnamespaced, so the cross-slot delete widens the blast radius of cross-extension key collisions. One docs line ("prefix keys with your extension name") until extension identity lands with assets.
  2. viewFields/effects policy metadata is declarative — enforcement is structural in each invoke path. Fine, but it can drift; either wire the sanitizer to consume it or comment it as documentation.
  3. /api/mock/event as a mock-gated test seam is a reasonable pattern; worth keeping it type-validated as it is.

Verdict: merge after deciding how to track #1. With this, the RFC's core is delivered end-to-end — kernel (#83), unified transport + panel/fab (#84), public API + invalidation + exemplar (#86) — and the remaining roadmap items are exactly the two gated ones: assets (blocked on the #85 auth decision for the public surface) and webviews (threat-model review).

@ashwin-pc

Copy link
Copy Markdown
Owner Author

Follow-up to my earlier review — four additions requested before merge. Context: a live incident on the dev instance (extension calling a bridge method the running server didn't have) exposed platform gaps that this PR — as the debut of the public API — is the right and cheapest place to close. Full mechanics below each ask.

1. Durable runtime-error surface for extension event handlers

Today an error thrown in an extension's event handler (e.g. session_start registering contributions) takes this path: runner catches → bridge onErrorserver_error transient broadcast. It dies silently three ways: sessions often materialize before any client is attached (zero recipients); when delivered it's a one-shot chat card, not where anyone looks when a UI affordance is missing; and /api/extensions/status only records load-phase errors, so the durable surface has no record. In the incident, setPanel succeeded and the follow-up registration threw — result: a half-registered surface, reachable in state, invisible in UI, with no diagnostic anywhere.

Ask: per-session ring buffer (last ~20) of { path, event, error, timestamp } populated from the existing onError hook, exposed via /api/extensions/status alongside load errors, rendered in the settings Extensions card. Small, and it converts "feature mysteriously absent" into a ten-second diagnosis.

(Full atomic/transactional registration — rolling back a handler's contributions on throw — is deliberately not asked here: it requires extension attribution in the registry, which is the extension-identity work already gating assets. Observability now, atomicity with identity.)

2. ctx.ui.web.capabilities — the public API should be born self-describing

~/.pi/web/extensions/ is copy/curl/symlink-distributed and versioned independently of the pi-web install, so "extension written for newer pi-web running on older pi-web" is a normal deployment state, and its current failure mode is a mid-handler TypeError (which item 1's gap then swallows). If contribute() ships without introspection, every future extension invents typeof ctx.ui.web.X === "function" heuristics — and a capabilities API added later is itself something extensions would have to feature-detect. Shipping it with the API debut avoids that bootstrap paradox.

Ask: ctx.ui.web.capabilities derived from contributionPolicies{ apiVersion: 1, slots: [...], kinds: [...], effects: [...] }. Nearly free now that policies are executable objects; frozen, additive-only thereafter. (engines-style declared compat stays in the distribution phase per #82 — this is only the runtime half.)

3. The exemplar should teach defensive registration

The notepad is where extension authors will learn idioms. It should check capabilities (or degrade gracefully) and report via ctx.ui.notify when the host is too old, rather than teaching "crash into a missing method" as the default pattern. A few lines, high leverage.

4. Still open from the original review: invalidation clobbers in-progress form state

webPanels.update() re-invokes with no event → "Loading…" → full re-render. A user mid-typing in the notepad quick-add (or inside the edit sub-view) when another session writes loses their input. Suggested host-level guard: skip/defer auto-refresh while document.activeElement is a form control inside the panel body (re-pull on blur), or a non-destructive "updated — refresh" affordance. Given the exemplar ships in this PR and its headline feature is live cross-session refresh, the clobbering path is now on by default — this one I'd now consider merge-blocking.

Items 1–3 are new asks from the incident; item 4 is promoted from the earlier review. Explicitly not for this PR: transactional registration (needs extension identity), dev-mode restart-pending detection (supervisor concern, separate small PR), and engines compat (distribution phase).

@ashwin-pc

Copy link
Copy Markdown
Owner Author

Addressed the complete follow-up review in b734603:

  1. Durable runtime diagnostics

    • per-session 20-entry ring buffer from the existing extension onError hook
    • records extension path, event, error, and timestamp
    • /api/extensions/status and reload status now include runtimeErrors
    • runtime failures mark the Extensions card degraded and render durable detail rows
  2. Self-describing public API

    • added deeply frozen ctx.ui.web.capabilities
    • derives API version, slots, kinds, and effects from executable contribution policies
    • unit coverage verifies the contract and frozen arrays
  3. Defensive exemplar

    • notepad checks capabilities and method availability before UI registration
    • warns through ctx.ui.notify on older hosts while leaving the tool available
    • shutdown cleanup is also guarded
  4. Non-destructive panel invalidation

    • invalidation is deferred while a panel form control/contenteditable is focused
    • one coalesced pull occurs after focus leaves the form
    • user actions clear the pending refresh and remain authoritative
    • cross-viewport E2E verifies an unsubmitted draft survives and refresh occurs on blur
  5. Stale Git responses

    • generation checks protect HTML, errors, busy state, and composer-context effects
    • E2E covers an older response resolving after the invalidated refresh
  6. Notepad active-entry cap

    • panel add now enforces the same 200-open-entry cap as the tool path

Also documented extension-prefixed contribution keys and clarified which policy fields are executable versus structural contract metadata.

Validation:

  • typecheck ✅
  • unit suite ✅ — 327 tests
  • build ✅
  • contribution/Git E2E across desktop, tablet, and mobile ✅ — 33 tests
  • full npm test: build/auth/typecheck/unit/desktop/tablet passed; mobile hit an unrelated existing mascot-vs-stop-button pointer race in send-stop.spec.ts twice. The exact failed mobile test passed immediately in isolation. All changed contribution tests passed directly in the full run.

@ashwin-pc

Copy link
Copy Markdown
Owner Author

Re-verified on b734603: typecheck ✅, full unit suite ✅ (327, incl. the new capabilities/ring-buffer contract tests), build ✅, targeted e2e ✅ (12/12, incl. the draft-survival flow). All four asks landed, and three came back better than requested:

  1. Runtime diagnostics — ring buffer wired from onError, capped at 20 (overflow-tested), merged into /api/extensions/status with state promoted to degraded and a count in the message. The defensive event-name extraction chain handles the runner's error shape correctly. Exactly the "ten-second diagnosis" surface the incident demanded.
  2. capabilities — deeply frozen (verified in tests down to the arrays), derived from the executable policies so it can't drift from reality, typed Readonly on PiWebUi.
  3. Defensive exemplar — capability + method checks, graceful notify on old hosts with the tool kept available, guarded shutdown. Bonus catch I'd missed in both prior reviews: the panel quick-add path previously bypassed the 200-entry cap that the tool path enforced — now consistent.
  4. Non-destructive invalidation — the focus-scoped deferral is well-engineered: queueMicrotask on focusout so focus moving between form controls (textarea → Save) keeps the deferral alive; user actions clear pending and stay authoritative; close/session-switch clear it. E2E proves the draft survives the invalidation and the coalesced pull fires on blur.

Also unasked-for but correct: the git-tab host gained the same request-generation guard as the panel host, fixing a latent pre-existing bug where a slow stale tab response could clobber the current view.

Two nano-notes for the record, neither blocking:

  • The exemplar checks capabilities?.apiVersion === 1 — strict equality means a future additive apiVersion: 2 bump would disable every extension that copied this idiom. Since the exemplar already does fine-grained slot/kind checks, consider teaching apiVersion >= 1 (or dropping the version check in favor of the granular ones) so the copied pattern ages well.
  • The focus heuristic protects active typing; a draft abandoned by clicking elsewhere is discarded by the deferred refresh. That's a defensible trade (the alternative — dirty-tracking until submit — risks indefinitely stale panels), just worth one line in the docs so it reads as a decision rather than an accident.

Ready to merge. With this, the #82 PR 1–3 sequence closes with the public API born self-describing and observable — the two properties the live incident proved matter most.

@ashwin-pc
ashwin-pc merged commit 33806f5 into main Aug 8, 2026
1 check passed
@ashwin-pc

Copy link
Copy Markdown
Owner Author

Follow-up (post-merge, separate small PR): make the new API discoverable to agents.

While validating this PR I checked what an agent session actually knows when a user asks it to build a pi-web extension. Findings:

  • contexts/web-ui.md — injected into every pi-web session's system prompt — contains zero mentions of extensions. Nothing pushes the existence of the contribution API to agents.
  • pi itself solves this exact problem in buildSystemPrompt(): the default prompt embeds absolute paths to its own docs/ and examples/ with routing instructions ("when asked about extensions → read docs/extensions.md, examples/extensions/ … follow cross-references before implementing"). Because pi-web appends to that prompt rather than replacing it, those pointers survive — so an agent in pi-web can already author competent pi-core extensions anywhere. The gap is precisely the pi-web layer: docs/pi-web-extensions.md (which this PR updates with the contribute() section) is referenced by nothing in any prompt.
  • Example-idiom census on this branch: four examples teach the legacy wrappers (git-footersetFooter, recapsetHeaderAction, download-artifactsetArtifactAction, github-repo-panelsetGitTab) vs one teaching contribute() (notepad). Since pi's mechanism ends with "copy the examples," an exploring agent will most likely reproduce the majority — legacy — idiom.

Proposed follow-up (deliberately not this PR): extend the proven mechanism rather than invent a parallel one.

  1. Append a pointer block to the generated web context, same shape pi uses (pi-web already resolves its app dir via import.meta.url for contexts/web-ui.md):

    pi-web extension documentation (read when asked to build pi-web extensions or browser UI):
    - API + slots: <appDir>/docs/pi-web-extensions.md
    - Examples: <appDir>/examples/pi-web-extensions/ (notepad.ts shows the current contribute() API)
    

    Push-based, cwd-independent, zero context cost until triggered.

  2. One-line header comments on the four legacy examples ("uses the typed compatibility wrappers; see notepad.ts for the current contribute() API") so exploration doesn't mislead.

  3. Optional, later: an authoring skill only if we want procedural guidance beyond the docs (test-harness recipes, symlink install workflow) — API discovery alone doesn't need it.

Cheap validation once it lands: spawn a fresh session with "build me a pi-web extension that adds a panel" and check it produces a contribute()-based extension unaided.

Filing this as a comment rather than an ask on this PR to keep #86's scope closed — it's ready to merge as-is per the review above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant