feat(lab): CL-10 public evidence trust core - #1705
Conversation
|
✅ Deterministic PR hygiene checks passed. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (168)
📝 WalkthroughWalkthroughThis change adds a large set of features across public evidence export, Codex restart controls, reasoning-effort configuration, adapter and provider handling, ledger durability, and pinned transport timeout classification. ChangesPublic evidence system
Codex restart controls
Reasoning and provider behavior
Ledger and transport hardening
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Dashboard
participant ManagementAPI
participant RestartService
participant ProcessTable
participant ModelsPage
Dashboard->>ManagementAPI: request Codex state or restart
ManagementAPI->>RestartService: read state or perform restart
RestartService->>ProcessTable: enumerate and verify process identity
ProcessTable-->>RestartService: validated process results
RestartService-->>ManagementAPI: restart response
ManagementAPI-->>Dashboard: state or outcome
Dashboard->>ModelsPage: increment restart epoch
ModelsPage->>ManagementAPI: refresh app-server state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
Rebased onto current dev with the reviewed public evidence trust core, consumer durability recovery, sparse-array JCS hardening, and required Windows publisher-key ACL hardening.
Rebase the reviewed CL-10 operator/community layer onto the current public-evidence core. This squashes the child history onto cl10-public-core while preserving the exact conflict-free GitHub merge tree, including the final review fixes.
561475c to
a69b19a
Compare
Follow-up review of bc767a03d found three loose ends. The HTTP failure string still said "Failed to restart Codex" in all eight locales after the action was renamed, so the one message a user sees when it breaks used the vocabulary the rename removed. CodexRestartOutcome and CodexRestartOptions were declared twice in the transport. Declaration merging kept the build green, which is precisely why this is worth removing: the first copy had already drifted, missing formatTimeout. The body-read timeout branch had no test. It is the half of the timeout fix that is easiest to regress — headers arrive, the body stalls, and blaming the payload would send the user looking for a proxy bug that is not there.
The sidebar button is the fix; the models tab is where the belief that something
is wrong actually forms. Someone comparing the picker against this list is
already looking at this page, so the recovery belongs here too.
A banner appears only when the proxy reports a running app-server older than the
catalog. Every other verdict renders nothing: telling a user "we could not tell"
on a page about models is noise, and a locked-down host where enumeration fails
would otherwise show a permanent warning.
The reading is fetched once on mount and on invalidation, never on a timer.
Enumeration shells out to ps, procfs, or PowerShell CIM, and this workspace
already gates its own catalog poll on tab activity for that reason.
Cross-surface invalidation is an epoch rather than a shared controller: App bumps
a counter when a restart settles anywhere, Models takes it as a prop and re-reads.
The backend is already single-flight, so what was missing was invalidation, not
mutual exclusion — without it a restart from the sidebar left the banner on screen.
Two defects the earlier string-matching tests could not see are now covered by
real DOM tests: a page-head restart did not refresh staleness at all, and the
settled callback fired after unmount. The hook also lost its timeout translation
in a rewrite, so a timeout showed the transport's hardcoded English.
Naming note: this file already binds catalogState to the /api/catalog resource,
so the new value is appServerState.
Verification: gui bun test 846 pass / 0 fail; lint, lint:i18n, build green; root
typecheck and privacy:scan green. Live proxy on a fresh build returned
{"state":"fresh","runningCount":4} for the authenticated GET, 401 unauthenticated,
and 403 for an admin token from a foreign origin.
restartCodexAppServers sent process.kill(pid, "SIGTERM") on every platform. On Windows that is not a graceful signal — it is an unconditional terminate of one process, and it leaves the process tree behind. The repository already knew this and already had the right ladder for the proxy in process-control.ts; app-servers never got it. That gap matters most exactly where this feature is most needed. Windows has no Ctrl+Q, so users close the Codex window and the app-server keeps running in the background holding a catalog snapshot — the stale picker this whole unit exists to fix. Windows now uses taskkill /PID <pid> /T /F, resolved from a trusted system directory rather than PATH, with a fallback to the old process.kill so the new path can never be worse than the one it replaces. Unix keeps SIGTERM only: there the signal really is graceful, and following it with SIGKILL would ask a harsher consent than a restart click gives. Survivors are reported instead. The asymmetry is recorded in the function comment and the phase doc so a later reader does not "fix" it into symmetry. Tests drive each branch through injected io: Windows exec receives /PID /T /F and no signal follows, a throwing taskkill falls back to SIGTERM, and Linux and macOS stay SIGTERM-only with no exec and no SIGKILL.
The CLI parity sweep reads GUI sources for /api paths, so naming the catalog route inside a comment made it look like an endpoint the GUI calls with no CLI mirror. The comment only needed to explain why the new value is appServerState rather than catalogState; it did not need to spell the route.
…y fixture (#1714 guard)
…ance fixture (#1714 guard)
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
⏳ DRAFT
What to do
This pull request was already a draft. Its draft status will be preserved after every issue above is resolved. |
Stack
1/2 - trust-core base layer for the CL-10 public evidence stack.
This PR restores the former #1628 branch as the base layer and isolates the public-evidence trust core from the operator/community integration layer.
Scope
Explicit non-scope
This PR does not expose operator CLI/API surfaces and does not implement community import/listing, revocation, provenance, purge integration, or Compatibility Matrix UI. Those remain in stacked PR #1706.
Remote publishing remains out of scope and is not authorized by either PR.
Stack order
cl10-public-core->devfeat/cl-10-public-evidence-contract->cl10-public-coreRestores the branch previously reviewed in #1628 without merging that closed PR.
Summary by CodeRabbit