Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Hygiene✅ Deterministic PR hygiene checks passed. |
![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: 9ecff384bb
ℹ️ 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".
| {matrixRows.length === 0 ? ( | ||
| <EmptyState title={t("lab.empty")} /> |
There was a problem hiding this comment.
When an active protocol pair has no match in the first 50 verdicts but does match a later page, the client-side filter makes matrixRows empty and this branch renders only the empty state. The Load more control is inside the opposite branch, so the remaining API pages become unreachable while ProtocolPairStatus labels the pair unverified. Keep pagination available, or fetch until pagination is exhausted, before declaring that no evidence exists.
AGENTS.md reference: gui/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
| </button> | ||
| </div> | ||
| {failed && <p className="awi-delete-error" role="alert">{t("api.plan.failed")}</p>} | ||
| {plan && plan.inbound === inbound && <PlanResult plan={plan} />} |
There was a problem hiding this comment.
After a preview is run, editing a combo's targets or strategy and saving it without renaming the combo preserves the same DetailPanel key and the same ComboProtocolPlan instance. The dirty flag returns to false, but this condition continues rendering the plan captured under the pre-save policy until the operator manually previews again. Reset or remount the preview when the saved baseline changes so the displayed candidates cannot contradict the committed combo configuration.
AGENTS.md reference: gui/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Reviewed draft exact head 9ecff384bb048164e29c8be4da0fe73552cd42e9. Blocking evidence-freshness issue: useSubjectProtocolPairs stores null in the process-global pairCache for every detail-read failure, and missingKey treats that as permanently resolved. A single transient network error or server 5xx therefore makes that subject unresolved for the rest of the dashboard session; the 60-second matrix poll, refreshed projection, filter toggles, and Back/Forward never retry it. This can hide real matching evidence and continue reporting the pair as unverified. Cache successful identities durably, but make failures retryable (for example per load generation or with a bounded backoff/TTL) and add a regression where a failed detail fetch succeeds on a later refresh. Focused provider-summary and five GUI suites passed 53/53 under CPUQuota=200%, MemoryMax=4G, MemorySwapMax=0, TasksMax=128. The PR is still draft, screenshots are pending, exact-head CI is red, and lower PF layers #5814-#5816 remain blocked.
The dashboard's provider panel validates the ?provider block with the shared leaf, so an older or newer server's record is refused instead of half-rendered.
… wire The provider panel needs the adapter, who decided it and per-model overrides; it is read from captureRouteStaticPolicy so no second copy of the adapter rules exists.
A pinned model the operator never listed still receives another wire; leaving it out would make the provider panel claim the provider adapter applies to it.
…ping Covers the provenance collapse, the 64-override cap, a non-echoing 404 and 400s for empty, over-long, control-character and repeated provider parameters.
…ovider An older server answers 404 or ignores the parameter; both read as unavailable so the provider panel hides quietly rather than showing an error.
Protocol deep links keep their target in the hash so Back/Forward restores it; routing reads the path alone and drops a query on any route that does not own one.
One place builds and reads the compatibility pair and provider settings hashes, so the plan panel, the Logs trace and the target pages agree on the spelling.
Shows the wire a provider receives, who decided it and per-model overrides from the ?provider summary; it is read-only so it can never pass for an API exposure switch.
The adapter field stays the only editor and still saves through onUpdateProvider; the panel beside it says which wire an unsaved choice would send.
Lab records openai-chat and friends; protocolFromLabProtocol maps them, and a filtered pair with no recorded row reads unverified rather than failed or unsupported.
Subject details are read only while a pair filter is active, bounded and cached, and the status line keeps Lab verdicts apart from delivery mode.
…otocol The pair lives in the hash so a deep link survives refresh and Back/Forward; an edit replaces the entry so Back leaves the page instead of undoing a filter.
…pair The trace says what a request did; whether that pair is verified is a Lab question, so the row opens the matrix prefiltered to its client API and upstream wire.
A candidate's wire is decided in provider settings and its pair is verified in the matrix; the preview links both instead of restating either.
The plan preview links a candidate to the panel that says which wire it receives; the hash is re-read on Back/Forward and dropped once the user selects another provider.
Asks POST /api/protocols/plan for every feature the client API can express, so the guaranteed/partial split covers the vocabulary; it runs only on an explicit click.
apiBase is threaded from the Combos page so the preview targets the same machine or hub as the editor; a new, unsaved combo has nothing to plan and shows no preview.
Fast refresh needs component-only modules, and keying the fetch on the missing ids removes the exhaustive-deps suppressions that made the React Compiler skip the hook.
…effect Setting focus inside an effect cascaded a second render; adjusting own state during render opens the provider and its Settings tab in one paint and satisfies the compiler.
…d-server hide The panel must name the upstream wire and its decider, expose no switch, and render nothing on a 404 or a server that ignores ?provider.
Lab identities map through protocolFromLabProtocol, an unknown pair is left out, and a pair with no rows reads unverified with no verdict or delivery-mode badge.
Queries survive only on providers and compatibility, a provider link waits for the list and re-applies on hash events, and moving away drops it.
No fetch before the click, the full expressible feature set in the plan body, both candidates rendered, and a quiet hide on an older server.
…ence views protocol-paths owns the ?provider builder and its source mapping; the dashboard doc records the three new panels, the hash-query deep links and their tests.
…erence Operators reading the reference see the provider block, its bounds and that it reports the upstream wire without exposing or toggling any client API.
…igin target The dashboard served by the proxy uses an empty API base, which both views read as no target, so the panel never fetched and the combo candidate paths never rendered. Only an absent base means no target.
a2af3af to
53e4783
Compare
9ecff38 to
272f5a1
Compare
리뷰 · 우선순위 66 / 80이 PR은 대시보드에 있는 화면을 프로토콜 계약에 연결한다. 새 페이지는 없고, 새로 저장하는 주소도 없다. 요청이 어떻게 전달되는지는 Lab이 매긴 판정과 따로 보여 준다. 프로바이더 설정 아래에 읽기 전용 칸이 붙는다. 호환성 표는 들어오는 API와 나가는 형식으로 거를 수 있다. Lab이 적어 둔 콤보 상세의 버튼은 저장된 콤보만 서버에 묻는다. 해시로 바로 연다. 베이스는 라인 - 라인 - 라인 - 메인테이너의 판단이 필요한 지점 베이스가 아직 초안이다. 작성자는 새 테스트를 로컬에서 돌리지 않았고, CI가 첫 실행이라고 적었다. 너의 추천 방향은 유지해라. 닫을 중복 PR은 없다. 베이스를 지금 이 댓글은 grok-bot이 작성했습니다 |
devin-ai-integration
Bot
added
the
priority: P3
|
Maintainer triage: Criteria (P3): Low: new provider/client integration, large or experimental feature (>2000 LOC or >50 files), RFC/roadmap, or long-stale branch. Related / overlapping PRs:
|
2 participants
Summary
PF-11 of the protocol-first-class unit (
devlog/_plan/260924_protocol_first_class/030_gui_and_management_api.md#pf-11-evidence-combo-and-provider-views). Stacked on #5816 (PF-08).Connects the existing Providers, Models → Compatibility, Combos and Logs screens to the protocol contract. No new page and no new write route; delivery mode and Lab verdict stay separate axes.
GET /api/protocols?provider=<name>adds the provider's wire summary (src/protocols/provider-summary.ts, side-effect free): adapter, decision source (hard-pin | operator | registry | provider-default), auth mode, upstream wire, and up to 64 model overrides (modelOverridesTruncatedwhen capped). Bad names → 400invalid_provider, unknown → 404unknown_provider; neither echoes the name. DTO + validator in the leafdto.ts.ProviderProtocolPanelunder the adapter field labels it "upstream wire this provider receives" with its source and overrides. It is read-only —PATCH /api/providersonly accepts per-model adapters for one provider, so the existing adapter field stays the only editor; the panel shows what will change after save.protocolFromLabProtocol. Absent evidence reads "unverified", never failed/unsupported. Subject details are fetched only while a protocol filter is active (≤ 200, 6 at a time, cached per target).POST /api/protocols/plan.#providers?provider=…,#models/compatibility?inbound=…&upstream=…): plan candidate → provider settings / compatibility pair; Logs trace → compatibility pair. Links push history; filter edits replace it, so Back/Forward works.gui/src/styles/protocol-evidence.css(cappedstyles.cssuntouched).Verification
bun x tsc --noEmit: exit 0 on this head.gui:bun x tsc -b,bun run lint,bun run lint:i18n,bun run build: exit 0.bun run structure:check: passed.Tests (
tests/server/protocol-provider-summary.test.ts;gui/tests/{provider-protocol-panel,compatibility-protocol-filter,protocol-deep-links,providers-deep-link,combo-protocol-plan}.test.*) were written and registered but run in the full local suite below.Full local run on the stack head (feat(protocols): protocol paths as a first-class concern — PF-01..PF-12 #5820, which contains this change):
bun run test— the only failures are Lab CL-03/CL-07/CL-08/SEC-02 andrelease helpertimeouts, which fail identically on a checkout without this stack (local environment), plus service/toggle cases that pass when run alone;cd gui && bun test --isolate tests— 2398 pass, 0 fail.CI on this head: all required checks pass.
Screenshots
Captured from the stack head in an isolated home (fake providers, no real credentials).
Provider settings, upstream wire panel
Compatibility, client API and upstream wire filters
Combo detail, candidate paths
Checklist