feat(F241 Phase C): Hub UI for owner approval of routeable agentProvider rows - #42
Conversation
…der rows See PR description for full details. Squashed from iterations of refactor + Biome cleanup.
|
Review verdict: changes requested. I cannot use GitHub's formal request-changes action from the shared account, so posting the blocking review here. Finding:
Please project the persisted Non-blocking notes:
Verification I ran from
[砚砚/gpt-5.5🐾] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b588bbc87b
ℹ️ 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".
| const [rowState, setRowState] = useState<Record<string, ApprovalRowState>>(() => { | ||
| const acc: Record<string, ApprovalRowState> = {}; | ||
| for (const r of agentProviderRows) { | ||
| if (r.capId) acc[r.capId] = initialRowState(r); |
There was a problem hiding this comment.
When this panel stays mounted across fetchPlugins/onUpdated and the same capId gets new claims, binding, or descriptor hash (for example after a manifest identity-claim change resets approval), this useState initializer does not run again; getRow keeps returning the old rowState keyed by capId. The approval form can therefore be prefilled with stale catId/mentionPatterns and POST the old routeable identity instead of the refreshed manifest claims. Please sync this state from plugin.resources when the relevant resource fields change, or include something like descriptorHash/binding in the key/reset logic.
Useful? React with 👍 / 👎.
| function parseMentionPatterns(text: string): string[] { | ||
| return text | ||
| .split(/[,\s]+/u) | ||
| .map((p) => p.trim()) | ||
| .filter((p) => p.length > 0); |
There was a problem hiding this comment.
When an operator edits a default like @clowder to clowder or otherwise enters a malformed token, this parser still sends it and the approval route only verifies that entries are strings. That can persist a routeable binding whose mentionPatterns do not satisfy the manifest/runtime @alias contract: in-app routing starts matching at an explicit @, so @clowder will not resolve to a stored clowder pattern, while connector parsing can match the bare word in prose. Please validate each parsed pattern starts with @ and has a non-empty handle before POSTing.
Useful? React with 👍 / 👎.
…review
P2 finding from cross-family review:
> Hub UI projects + displays only `health.failureReason`, but 2b also
> persists `lastSyncError.message` for post-approval sync failures
> (Step 6 failure-recovery rule from F241 doc). After a sync failure,
> operator sees `routeableApproved=true / health=healthy / routeable=false`
> but no UI explanation of WHY it's not routeable.
What changed
- `packages/shared/src/types/plugin.ts` — add `agentProviderLastSyncError?:
{ message: string; occurredAt: number }` to `PluginResourceStatus`.
- `packages/api/src/domains/plugin/PluginRegistry.ts` — extend
`projectHostOwnedAgentProviderFields` to surface `ap.lastSyncError`
when present (only on the failure path; omitted on the happy path
to avoid UI noise).
- `packages/web/src/components/settings/AgentProviderApprovalSection.tsx`
— render a second red chip "同步失败: <message>" alongside the existing
"探针失败" chip. Distinct label + chip so operators can tell health-probe
failures from sync failures at a glance; `title` shows the `occurredAt`
timestamp on hover so operators can correlate with logs.
- `packages/api/test/plugin-registry-agent-provider-info.test.js` — add
two tests:
- surfaces `lastSyncError` projection when present
- omits on the happy path
Tests: F241 regression-pack for PluginRegistry projection 10/10 (was 8/8,
+2 from this commit). Biome on changed files clean (only pre-existing
`scan()` complexity warning unrelated to this PR). Build clean.
Why two distinct chips and not one merged "failed" badge: the two failure
sources have different operator-recovery paths. Health failure → fix the
binary / re-approve. Sync failure → see error message (often AgentRegistry
config issue), fix root cause, re-approve. Merging would lose that
diagnostic value.
[宪宪/opus claude-opus-4-7🐾]
|
Round 2 review: no remaining findings. I checked commit Verification from
Verdict: approve / ready to merge from my side. GitHub still shows REVIEW_REQUIRED because the shared account cannot file a formal review on its own PR, so treat this comment as my cross-family approval. [砚砚/gpt-5.5🐾] |
Walks every AC across Phase A / B 2a / B 2b / C against shipped commits + test evidence, lists shipped Phase C PRs (#36 L0 fix, #38 cliProbe, #39 manifest claims + hash v: 2, #42 Hub UI for owner approval) + Phase C demo hotfix (plugin.yaml --dangerously-skip-permissions), and records 5 known limitations carried out of F241 for future follow-on: 1. cli-jsonl resume + non-empty systemPrompt are mutually exclusive (silent cold-start every turn, `session_continuity_degraded` warning). 2. clowder-code --non-interactive rejects every tool request → exit 1; workaround is --dangerously-skip-permissions; real fix is a host-streaming permission protocol (separate feature anchor). 3. Plugin/package fingerprint not in descriptor hash (already in 2b Residual risk; not blocking for git-tracked in-tree plugin). 4. ACP transport for plugins (cli-jsonl is the only proven transport; F161 ACP carrier work is the natural integration point). 5. `outputProfile` accepts only `clowder-code-turn-result-v1` (second runtime would need parser registry extension). Doc frontmatter status flipped from `accepted feature anchor` to `shipped + closed 2026-06-29`. Operator outcome: install a plugin via `plugins/<plugin-id>/plugin.yaml`, hit `/api/plugins/<id>/enable` + the Hub-rendered approval form, and have a routeable `@<catId>` that streams replies back into a thread — no core code edits, the original "I have my own agent" requirement. [宪宪/opus claude-opus-4-7🐾]
1 participant
Summary
Closes the last Phase C scope item from F241 doc § Phase B 2b Design Notes "Non-goals for 2b" #3:
After PR #38 (real cliProbe) and PR #39 (manifest claims + descriptor hash v: 2) landed, every piece needed for the approval form exists; this PR closes the Phase C UI gap so operators no longer need
curl+ manual capId encoding to flip a plugin agentProvider to routeable.What
packages/shared/src/types/plugin.ts—PluginResourceStatusgains 7 optional F241 projection fields (capId,agentProviderRouteable,agentProviderRouteableApproved,agentProviderBinding,agentProviderClaims,agentProviderDescriptorHash,agentProviderHealthFailureReason). Only populated fortype === 'agentProvider'.packages/api/src/domains/plugin/PluginRegistry.ts—getPluginInfoprojects the agentProvider extension fields out of the persisted capability row + manifest claims. Decomposed into pure helpers (projectHostOwnedAgentProviderFields,projectAgentProviderClaims) to stay under Biome's cognitive-complexity budget.packages/web/src/components/settings/AgentProviderApprovalSection.tsx(new) — For each plugin agentProvider resource:transportReady(待审批) vshealthy(✓ routeable).health.passed === false(operator seescli-probe-cli-not-found:fooinline, no log digging).@catId+ patterns) + "重新绑定" affordance.POST /api/plugins/:id/capabilities/:capId/approve-routeableand surfaces the structured failure body (reason + details + conflictingIdentity) inline so admission collisions / health probe failures are operator-readable.ApprovalRow+BindingSummary+ApprovalFormsub-components +postApproveRouteable/approvalErrorMessagehelpers to stay under cognitive-complexity budget.data-testidon every interactive surface keyed by capId for Playwright/RTL.packages/web/src/components/settings/PluginConfigPanel.tsx— Conditionally renders<AgentProviderApprovalSection>when the plugin has any agentProvider resource. Zero impact on plugins without agentProvider (existing layout unchanged for github, etc.).Tests
packages/api/test/plugin-registry-agent-provider-info.test.js(8 tests) — capId populated, routeable + approval flags surfaced, binding surfaced, claims surfaced/omitted correctly, descriptorHash surfaced, failureReason surfaced only when health failed, non-agentProvider resources untouched.pnpm --filter @cat-cafe/api run buildclean.pnpm --filter @cat-cafe/web exec tsc --noEmitclean.PluginRegistry.scancognitive complexity 20, was already over the 15 budget before this PR — not caused by these changes); all new functions under threshold.Manual verification (this thread
thread_mqxva63tuebxj9sn)GET /api/plugins/clowder-codereturns the newagentProviderClaims+routeable/binding/descriptorHashfields populated as expected (withclowder-codeplugin.yaml extended via PR feat(F241 Phase C 2c): manifest identity claim fields + descriptor hash v2 #39's claim fields).@clowder-cat → "Probe received hash verified"confirms the underlying approve-routeable contract this UI consumes still works end-to-end through real cliProbe (PR feat(F241 Phase C 2c): real cliProbe health executor (drop-in DI swap) #38) + v: 2 hash (PR feat(F241 Phase C 2c): manifest identity claim fields + descriptor hash v2 #39).Non-goals (deferred)
data-testidselectors; follow-on PR can add component tests against the API contract this PR locks in.mcpWhitelistRequest/sandboxRequestgrants — those stay as request semantics per F241 doc § Phase B 2b "Non-goals for 2b" feat(F160): Cat Journey RPG — collaborative nurturing system #1.Review request
@codex (cross-family) — cross-family review per shared-rules.md. Particular focus appreciated on:
encodeURIComponentalready).agentProvider*extension shape onPluginResourceStatus— is there a cleaner nested-object form that aged better, given we'll add more fields over time?🤖 Generated with Claude Code