PRO-1042: route Codex plugin login through typed CLI auth - #4
Conversation
|
Dependency update: lua-core-services #2183 must merge and deploy before this draft can release. Direct |
richard-lua
left a comment
There was a problem hiding this comment.
Code review — MEDIUM risk
The security direction here is sound: new logins are routed to lua-cli's private lua auth configure, the conversation no longer collects email/OTP/credentials, the shell hook now denies model-run lua auth configure (with good test coverage), and the CLI-flag lint is extended to docs to prevent secret-bearing commands from reshipping. The docs, SECURITY.md, and permissions template are consistent with the new flow. Two concrete issues below should be addressed before merge; note also that the PR body's own release gate keeps this blocked until upstream CLI/services ship, which is a legitimate constraint rather than a review action.
Major
mcp/lua-platform/src/api-client.mjs:38— The client version is hardcoded (codex-plugin/1.0.0) while the new test assertscodex-plugin/${package.json.version}. The PR's own release gate requires bumping the plugin version before merge, which will immediately break the test — or, if patched by hand, will silently misreport the version to the backend. Derive the header frompackage.jsonso the client and test share one source of truth.
Minor
scripts/lint-cli-flags.mjs:30—SCAN_DIRSwas renamedcommands→skills. Correct for this Codex port, but it silently drops a scan root and the history comment still referencescommands/lua-sync.md. Confirm nocommands/tree ships and update the stale comment.mcp/lua-platform/tests/api-client.test.mjs:74— The "single direct caller" structural test regexes forfetch(/fetchFn(and will misfire on benign refactors (e.g. aprefetch(helper) or missglobalThis.fetch(without a space. Tighten the pattern and document how to extend the allowlist for an intentional new HTTP entry point.
Nothing here is exploitable or data-losing; the findings are correctness/maintainability. Once the version-drift issue is fixed and the release gate conditions are met, this is safe to merge.
PR Risk Reviewer — automated senior review of a7b24a0 · risk: medium · confidence: 0.78
richard-lua
left a comment
There was a problem hiding this comment.
Code review — LOW risk
Previously flagged
- Hardcoded client version will drift from package.json — resolved. The header literal in
api-client.mjs:38is stillcodex-plugin/1.1.0, but the newscripts/lint-release-version.mjs(now in thelintscript) fails CI unless that literal, bothplugin_versionstrings and the server name/version inserver.mjs,.codex-plugin/plugin.json, andmcp/lua-platform/package.jsonall equalpackage.json's version. Drift is now mechanically caught rather than silently shipped, and the api-client test asserts againstpackage.json. - Lint SCAN_DIRS
commands→skills— still open (cosmetic). The rename is correct (nocommands/tree ships), but the history comment inscripts/lint-cli-flags.mjsstill referencescommands/lua-sync.md. - Structural lint test brittleness — still open (minor).
api-client.test.mjs:74keeps thefetch|fetchFnregex with no allowlist-extension guidance.
The core change is sound and well-covered: new logins route to lua-cli's private lua auth configure, the shell hook now denies model-run lua auth configure (with the flagged-variant and flagless cases both tested), the permissions template moves lua auth configure* to deny, and the CLI-flag lint now scans docs/README/SECURITY for secret-bearing auth commands. Docs, SECURITY.md, and the version-consistency lint are internally consistent. Nothing here is exploitable or data-losing.
Minor
scripts/lint-cli-flags.mjs:8— History comment still referencescommands/lua-sync.md, a path that no longer exists after theSCAN_DIRSrename toskills. Update toskills/lua-sync/SKILL.md.mcp/lua-platform/tests/api-client.test.mjs:74— The/\b(?:fetch|fetchFn)\s*\(/guard can misfire on future refactors and documents no way to register an intentional new HTTP entry point. Tighten the pattern and add an allowlist note.mcp/lua-platform/README.md:5— Intro still says "6 read-only tools" and "Claude Code"; contradicts the "5 tools (was 6 before v1.25)" line and the Codex port. Correct the count and host.
Once the release-gate conditions in the PR body are met (upstream CLI 3.28.0 + services deployed, CI rerun against the published dependency), this is safe to merge.
PR Risk Reviewer — automated senior re-review of 2a9f477 · risk: low · confidence: 0.80
PR Risk Reviewer — automated senior review of 2a9f477 · risk: low · confidence: 0.80 · re-review
Summary
LUA_API_KEY,~/.lua-cli/credentials, and.envunchanged. This includes non-dotted legacy keys.lua auth configurein a private terminal. lua-cli 3.28.0 and later issue a typed personal credential after the user selects an organization, one or more agents, and a role. Builder remains the default role.lua auth configure, andlua auth key*remains denied.codex-plugin/1.1.0. Calls that spawn lua-cli use the CLI's own versioned identity.Source-derived caller inventory
The inventory came from a repository-wide
rgbefore editing.skills/lua-auth/SKILL.md,skills/lua-doctor/SKILL.md, and the auth preflight inskills/lua-init/SKILL.mdown setup.lib/credentials.mjsandmcp/lua-platform/src/auth.mjsresolveLUA_API_KEY, the CLI credentials file, and.env. This PR does not change their order or formats.mcp/lua-platform/src/tools/list-agents.mjsandget-agent.mjsspawnlua agents --json. Those requests usecli/<version>.mcp/lua-platform/src/api-client.mjsowns direct Lua API requests for deployment status, primitive versions, and logs. Those requests now usecodex-plugin/1.1.0, which lua-core-services #2183 recognizes as a bounded family.Merge gate
Merge only after
npm view lua-cli versionreports 3.28.0 or later. The current PR head must also have green CI and current-head approval.This PR does not rotate, revoke, rewrite, or invalidate an existing credential. It does not use work from the closed #2008 prototype.
Verification
npm run lintnpm test -- --coverage(249 tests)node scripts/check-coverage.mjsnpm test(83 tests)npm run buildnode scripts/check-bundle-size.mjsLinear: PRO-1042