fix: accept OAuth DCR metadata in vault set - #288
Conversation
|
Codex review: needs maintainer review before merge. Reviewed August 8, 2026, 4:48 PM ET / 20:48 UTC. ClawSweeper reviewWhat this changesUpdates Regression provenancePossible regression — probable (reproduction; reviewed change). No predecessor PR is attributed. Merge readinessKeep open. Current main and v0.13.0 still reject standard DCR arrays; this PR supplies a focused fix with real CLI persistence proof. A maintainer should select this implementation or the overlapping #287 before either lands. Priority: P1 Review scores
Verification
How this fits togetherThe vault command imports OAuth tokens and dynamic client-registration metadata from stdin or files into MCPorter’s local credential store. OAuth refresh and registration later consume the stored client information to authenticate MCP server access. flowchart LR
A[OAuth registration payload] --> B[Vault set command]
B --> C[Client metadata validation]
C --> D[Local credentials vault]
D --> E[OAuth refresh and registration]
E --> F[Authenticated MCP server access]
Decision needed
Why: Both open PRs repair the same user-visible failure but differ in validation scope and adjacent behavior, so a mechanical merge decision cannot choose the intended long-term credential contract. Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land one narrow, schema-aware DCR import validator with persistence coverage for accepted arrays/timestamps and malformed field values, while preserving null-compatible and provider-specific metadata. Do we have a high-confidence way to reproduce the issue? Yes. Current main’s string-only loop proves the rejection path, and the PR provides an after-fix built-CLI persistence transcript using the reported DCR shape. Is this the best way to solve the issue? Yes for the reported regression: per-field validation accepts standard DCR shapes without discarding provider metadata. The remaining choice is which overlapping PR should own that contract. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against ba80d985c867. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (5 earlier review cycles)
|
|
Addressed the review findings:
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
912fe61 to
b91b185
Compare
|
Exact-head maintainer proof for
Scope is intentionally limited to #286. Two useful findings from concurrent PR #287 remain separate follow-ups: sanitizing malformed-JSON errors and validating token Thanks @feniix for the precise report, compatibility analysis, implementation, and real-CLI proof. |
Summary
Type of change
Real CLI behavior proof
Ran the built CLI with an isolated
MCPORTER_CONFIGandXDG_DATA_HOME, the fake token from #286, DCR arrays and a registration timestamp. No server was contacted.This proves both the reported DCR shape and the previously accepted
nullvalue persist through the real CLI.Test plan
pnpm exec vitest run tests/vault-validation.test.ts tests/vault-command.test.tspnpm checkpnpm testCloses #286