PRO-1042: route Claude plugin login through typed CLI auth - #6
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 right: new-login auth moves out of the model conversation into lua-cli's private lua auth configure, backed by a PreToolUse block hook and permission-template deny rules, with docs/lints/tests updated consistently. Existing credentials are preserved and nothing is rotated or exposed. One concrete correctness bug should be fixed before merge, plus a few advisory edge cases; the PR's own "keep in draft until the release gate clears" note is correct process guidance.
Major
plugins/lua-agent-builder/mcp/lua-platform/src/api-client.mjs:38—X-Lua-Clientis hardcoded to'claude-plugin/1.0.1', but the new testapi-client.test.mjsasserts it equalsclaude-plugin/${package.json version}. This only passes at version 1.0.1; the PR's release gate requires a version bump, which will make the header stale and break the test. Derive the version frompackage.jsonand format asclaude-plugin/${version}.
Minor
plugins/lua-agent-builder/hooks/block-auth-configure.mjs:5— The block hook +Bash(*lua auth configure*)ifgate are substring/pattern-based and evadable (e.g.sh -c, aliases). The authoritative control is the permission-template deny; SECURITY.md's "deny model-run login" wording overstates the hook's guarantee. Treat the hook as best-effort defense-in-depth and lean on the permission deny.plugins/lua-agent-builder/scripts/lint-cli-flags.mjs:26— Auth-doc scanning is inconsistent on missing paths: a missingAUTH_DOC_FILESentry throws a raw ENOENT (uncaught), while a relocatedAUTH_DOC_DIRSentry silently scans nothing. Wrap the file scan in try/catch with a clear message and assert at least one auth doc was scanned so the guard can't silently no-op.plugins/lua-agent-builder/mcp/lua-platform/tests/api-client.test.mjs:72— The "only api-client.mjs calls fetch" static guard is useful but brittle to lexical form (prefetch(, comments,globalThis.fetch). Tighten the regex and add a rationale comment.
PR Risk Reviewer — automated senior review of dfba757 · risk: medium · confidence: 0.78
richard-lua
left a comment
There was a problem hiding this comment.
Code review — LOW risk
Previously flagged
- X-Lua-Client version hardcoded / drifts from package version — resolved. The header is now
claude-plugin/1.1.0(mcp/lua-platform/src/api-client.mjs:38), all metadata surfaces bumped to 1.1.0, and the newscripts/lint-release-version.mjsfails CI if the header,plugin.json,marketplace.json, MCPpackage.json, orserver.mjsversion strings diverge frompackage.json. The api-client test now derives the expected version frompackage.jsoninstead of asserting a fixed value, so it stays green across future bumps. - Block hook is defense-in-depth only — resolved (as documentation). SECURITY.md and README now describe the permission-template deny as the primary gate with the hook as an additional block, matching the actual guarantee. The hook remains substring/regex-based and evadable by shell tricks, which is acceptable for defense-in-depth.
- Auth-flow lint scans a fixed doc file list and can silently no-op — still open.
scripts/lint-cli-flags.mjsstill passesAUTH_DOC_FILESdirectly toreadFile(uncaught ENOENT on a relocated doc) and walksAUTH_DOC_DIRSwith no minimum-scanned assertion. Minor. - Static 'only api-client calls fetch' guard is brittle — mitigated. The regex was tightened to
\b(?:fetch|fetchFn)\s*\(and the test now enumeratessrc/recursively; still lexical, but tighter and with clearer intent. Acceptable.
The version-consistency work closes the one blocking correctness issue from the prior review. The change is well-tested and the PR's own "keep in draft until the release gate (lua-core-services #2183, published lua-cli 3.28.0) clears" note remains the correct process guidance. Remaining findings are advisory.
Minor
scripts/lint-release-version.mjs:22— The lint asserts exact source substrings ('X-Lua-Client': 'claude-plugin/${v}',plugin_version: '${v}') with exact occurrence counts, coupling the guard to source quoting/spacing and blocking any move to deriving the version frompackage.jsonat runtime. Consider having the runtime read the version frompackage.jsonand asserting the resolved value, or tolerate quote/whitespace variants.scripts/lint-cli-flags.mjs:26—AUTH_DOC_FILESare read without try/catch (a relocatedREADME.md/SECURITY.mdthrows an uncaught ENOENT), and no minimum-scanned count is asserted, so a movedAUTH_DOC_DIRSentry silently scans nothing. Wrap the file scan with a clear failure message and assert at least one auth doc was scanned.
PR Risk Reviewer — automated senior review of 28eda77 · risk: low · confidence: 0.82 · 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.claude-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.commands/lua-auth.md,commands/lua-doctor.md, and the auth preflight incommands/lua-init.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 useclaude-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(223 tests)node scripts/check-coverage.mjsnpm test(83 tests)npm run buildnode scripts/check-bundle-size.mjsLinear: PRO-1042