fix(mecatui): persist trusted server CA certificates - #1233
Conversation
jhrozek
left a comment
There was a problem hiding this comment.
MoE review (security / architecture / test-adequacy / docs-sync, modeled on the pr-review-toolhive skill's category→agent mapping). Overall: the issuer/server CA trust split is genuinely preserved end-to-end, and most of the change is well-tested. One functional regression (finding 1) and a process/doc mismatch (finding 2) should be resolved before merge; the rest are cleanups worth folding into this PR since the diff is still small.
| **Contract:** human-reviewed/v1 | ||
| **Phase:** focused mecatui remote-login capability | ||
| **Status:** in-progress, 2026-09-08 | ||
| **Delivery:** Split |
There was a problem hiding this comment.
[MEDIUM] Delivery classification doesn't match how this PR actually shipped.
This plan is tagged Delivery: Split, but PR #1233 ships the acceptance plan, the ADR, and the full implementation together in one PR — i.e. Combined, not Split (per AGENTS.md, Split means /to-acceptance-plan opens a Plan/Interface PR that stops before implementation).
It also doesn't qualify for the Combined exception as written: that requires every runtime/public/operator/persistence/trust-boundary category to be None — rationale, but this plan's own Interface Contract section lists real content for CLI/config (--server-tls-ca PATH) and Security/authority (override precedence, verified-TLS requirement).
Please either retag this as Combined with a rationale for why splitting added no review value here, or split the PR per the documented workflow.
62397b8 to
9948a24
Compare
jhrozek
left a comment
There was a problem hiding this comment.
MoE panel re-review (spec / standards / test-adequacy / security / architecture / library-reuse / duplication axes) of the 2026-09-09 fix commits. The original HIGH finding (saved CA restored only on the OIDC path) is genuinely fixed, with real regression tests across all three connect modes, and the four cleanup items from the prior round are resolved. Two new items need a look before merge: a cross-confirmed confirmation-bypass bug in the discovered-login flow (finding 1), and two acceptance-plan process items (findings 2–3) that don't match how this PR actually shipped. Findings 4–7 are non-blocking cleanups worth folding in since the diff is still small.
| **Contract:** human-reviewed/v1 | ||
| **Phase:** focused mecatui remote-login capability | ||
| **Status:** in-progress, 2026-09-08 | ||
| **Delivery:** Split |
There was a problem hiding this comment.
[BLOCKER] Delivery: Split doesn't match how this PR shipped. Split requires a separately-merged Plan/Interface PR before implementation lands (docs/development-process.md, "Split path"). This PR ships the acceptance plan, ADR 0319, and the full implementation together in one PR — that's Combined-shaped, but it doesn't meet Combined's eligibility bar either (missing **Expected tasks:** 1 and a non-placeholder **Combined rationale:**). The PR description also has no "Contract linkage" section disclosing a spine waiver (contrast with #1338, which explicitly documented one). Please correct the classification or add the waiver disclosure.
9948a24 to
ed60f07
Compare
Co-Authored-By: mecatl <noreply@stacklok.com>
Co-Authored-By: mecatl <noreply@stacklok.com>
Co-Authored-By: mecatl <noreply@stacklok.com>
Co-Authored-By: mecatl <noreply@stacklok.com>
Co-Authored-By: mecatl <noreply@stacklok.com>
Co-Authored-By: mecatl <noreply@stacklok.com>
Co-Authored-By: mecatl <noreply@stacklok.com>
Co-Authored-By: mecatl <noreply@stacklok.com>
bda7b3e to
141b6f4
Compare
Summary
mecatui connect ADDRESScalls while preserving explicit--tls-caprecedenceCloses #886
Verification
go test ./cmd/mecatui ./internal/adapter/clientauthtask linttask testtask docstask site:buildgo run ./cmd/mecademo