fix(auth,vc): reject unusable stored tokens, allow bot auth where endpoints support it - #2190
Closed
sang-neo03 wants to merge 12 commits into
Closed
fix(auth,vc): reject unusable stored tokens, allow bot auth where endpoints support it#2190sang-neo03 wants to merge 12 commits into
sang-neo03 wants to merge 12 commits into
Conversation
TokenStatus judged freshness from the expiry timestamps alone, so a stored record whose accessToken was empty came back "valid". encoding/json drops unknown fields silently, which means a writer that misspells the field name (the reported case used "userAccessToken" instead of "accessToken") produces exactly that record: healthy-looking timestamps, complete scope list, no usable token. Every consumer then drew the wrong conclusion from it: - auth status reported ready / available / valid, contradicting the business commands that kept failing - GetValidAccessToken returned the empty string as a bearer token, so calls failed at the server instead of locally with a cause - --as auto resolved to the user identity, because only "expired" fell back to bot - auth check answered "granted" from the scope list of an unusable record TokenStatus now checks the access token before the timestamps and reports the new "corrupted" status, and all four consumers handle it. auth status keeps reporting status "missing" rather than a new enum value: consumers already branch on missing and its recovery action (re-authorize) is the right one here, so the distinguishing detail rides in tokenStatus and the message instead of breaking the output contract. A corrupted record is not deleted. The refresh token next to an empty access token is equally suspect, and keeping the file lets whoever wrote it see what landed on disk — that diagnosis took a long time precisely because nothing named the field.
… not a terminal auth login always polled until the device code expired. The command already detected a non-terminal environment, but only used that to print a longer hint — the hint itself said "this command blocks for up to 10 minutes, set your runner timeout to 600s". In a sandbox nobody opens the verification URL, so those ten minutes always elapse and the caller's whole turn is spent waiting for a result that cannot arrive. The default now follows stdout: unattended runs (piped output, agent sandbox, CI) return the device code and verification URL immediately, and callers decide when to poll with --device-code. An interactive terminal still blocks, since someone is there to scan. Both preferences stay expressible. --wait keeps the poll on a pipe, --no-wait forces the non-blocking path on a terminal, and an explicit --no-wait=false is read as a request to block rather than as an unset flag the terminal check may override. Passing --wait and --no-wait together is a caller bug, so it fails with a typed validation error before any device code is issued rather than resolving to one of them silently. The switch is announced on stderr when neither flag was passed, because a caller that asked for neither mode must be able to tell why it received a device code instead of a completed login, and how to get blocking back.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesTerminal-aware login waiting
Corrupted stored-token handling
Shortcut authentication types
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AuthLogin
participant DeviceAuthorization
participant TokenPolling
User->>AuthLogin: Select --wait, --no-wait, or default
AuthLogin->>AuthLogin: Resolve wait mode from flags and terminal output
AuthLogin->>DeviceAuthorization: Request device authorization
AuthLogin->>TokenPolling: Poll when blocking
AuthLogin-->>User: Return device-code data or authorization result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
github-actions
Bot
added
the
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/auth/check.go`:
- Around line 83-89: Update the corrupted-token branch in the authentication
check command to return the appropriate typed errs authentication error with
token-invalid metadata instead of output.ErrBare(1). Preserve the JSON predicate
result on stdout, and update the command contract and tests to verify the typed
error envelope is emitted on stderr.
In `@cmd/auth/login_test.go`:
- Around line 1252-1261: Update the error assertions in the relevant login test
to obtain the shared problem metadata with errs.ProblemOf(err), asserting its
Category and Subtype values. Retain errors.As only for extracting the concrete
errs.ValidationError and validating its Param value, removing the direct Subtype
assertion on verr.
In `@cmd/auth/login.go`:
- Around line 67-71: Update the option-resolution logic around the no-wait
handling in the login command to detect when the “wait” flag was explicitly
provided, including an explicit false value, and force the resolved path to wait
unless --no-wait is explicitly enabled. Add a command test covering --wait=false
with terminal stdout and verify it resolves to the waiting behavior.
In `@internal/auth/token_corrupted_test.go`:
- Around line 151-163: Extend the assertions in the token-corruption test after
errs.ProblemOf(err) succeeds to verify problem.Category is
errs.CategoryAuthentication, problem.Subtype is errs.SubtypeTokenInvalid, and
IsNeedUserAuthorizationError(err) returns true. Keep the existing
message-fragment and JSON-marshalling checks unchanged, and do not add a Param
assertion because errs.Problem does not expose that field.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 021771ef-c675-417b-b8d8-41fa883e6f6d
📒 Files selected for processing (13)
cmd/auth/auth_test.gocmd/auth/check.gocmd/auth/check_test.gocmd/auth/login.gocmd/auth/login_test.gointernal/auth/errors.gointernal/auth/token_corrupted_test.gointernal/auth/token_store.gointernal/auth/uat_client.gointernal/credential/credential_provider.gointernal/credential/credential_provider_test.gointernal/identitydiag/diagnostics.gointernal/identitydiag/diagnostics_test.go
Comment on lines
+83
to
+89
| // The scope list of a corrupted record still looks complete, so reporting | ||
| // scopes here would answer "granted" for a credential that cannot make a | ||
| // single call. Fail with the cause instead. | ||
| if larkauth.TokenStatus(stored) == larkauth.TokenStatusCorrupted { | ||
| output.PrintJson(f.IOStreams.Out, map[string]interface{}{"ok": false, "error": "corrupted_token", "missing": required}) | ||
| return output.ErrBare(1) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return a typed error for the corrupted-token failure.
Line 88 returns output.ErrBare(1). This bypasses the required typed command-error contract. Return an errs authentication error with token-invalid metadata. Update the command contract and tests so stdout carries the predicate result and stderr carries the typed error envelope.
As per coding guidelines, command-facing failures must use typed errs.* errors and must not use legacy output.Err* helpers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/auth/check.go` around lines 83 - 89, Update the corrupted-token branch in
the authentication check command to return the appropriate typed errs
authentication error with token-invalid metadata instead of output.ErrBare(1).
Preserve the JSON predicate result on stdout, and update the command contract
and tests to verify the typed error envelope is emitted on stderr.
Source: Coding guidelines
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2190 +/- ##
==========================================
+ Coverage 76.00% 76.02% +0.02%
==========================================
Files 966 966
Lines 102541 102577 +36
==========================================
+ Hits 77933 77989 +56
+ Misses 18704 18680 -24
- Partials 5904 5908 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@c3388942053082a422ba959d70d69ba8d7a46150🧩 Skill updatenpx skills add larksuite/cli#feat/auth-unattended-reliability -y -g |
8 tasks
…ed wait flags Review follow-ups on the unattended-auth changes. auth check rejected a corrupted token but still answered "granted" for an expired one. Both statuses leave the scope list intact and both already make `--as auto` fall back to bot, so reporting scopes from either contradicts the rest of the tree. The rejection now covers expired as well, reported as "expired_token" to keep the cause diagnosable. needs_refresh stays accepted: that token serves calls after the automatic refresh on next use, so its scopes are the ones the caller will actually have. auth login only inspected --no-wait's explicit state, so --wait=false was indistinguishable from an absent --wait and the terminal default kept blocking against a stated preference. Both flags now fold into one waitIntent resolved from cobra's Changed() state, where an explicitly negated flag means the opposite of its name. Setting both is a conflict regardless of their values. LoginOptions.effectiveWaitIntent keeps the booleans working for callers that construct the struct directly instead of going through flag parsing, so a set field never silently does nothing. The no-scope hint claimed the command blocks and told callers to pass --no-wait --json. It now reports what will actually happen, branching on stdout because that path is also reached on a terminal with --json. Error-path assertions now check category and reach the preserved cause via errors.As rather than only the subtype and the helper predicate.
config bind --identity user-default hands the Agent a one-call login flow and promises the command "returns automatically once authorization completes". That promise no longer holds by itself: Agent runtimes capture stdout, and auth login now returns the device code and exits 0 there instead of polling. An Agent following the old wording would read that exit as a completed login and continue with the user's request while the user has authorized nothing. The instruction now spells out --wait and why it cannot be dropped, in both languages, and the README agent steps match. The bind message test pins the flag so removing it fails rather than silently restoring the mismatch. Keeping the blocking single call (rather than teaching the two-phase device-code flow here) suits the runtimes this message targets: OpenClaw and Hermes relay stderr to the user in real time, so the URL reaches them while the command waits.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/auth/check_test.go`:
- Around line 249-250: Extend the TokenStatus test table in
cmd/auth/check_test.go with a whitespace-only AccessToken fixture such as "
\t\n", and assert it returns "corrupted_token", alongside the existing
empty-token corrupted case.
- Around line 281-298: Extend the error assertions in the authCheckRun test
using errs.ProblemOf to verify the typed problem category and subtype, while
retaining the existing exit-code and JSON contract checks. Where the status
error wraps a lower-layer cause, assert cause preservation; use errors.As to
inspect Param only when the error is a *errs.ValidationError, since
errs.ProblemOf exposes only problem-level fields.
- Around line 289-301: Update the response parsing in the affected auth check
tests, especially TestAuthCheckRun_NeedsRefreshTokenStillReportsScopes, to
unmarshal into a typed auth-check response projection instead of
map[string]interface{}. Assert that granted contains exactly the im:message
scope, with no missing scope, while retaining the existing ok and error
assertions. Apply the same typed projection to the additional affected assertion
block.
In `@cmd/auth/login.go`:
- Around line 316-323: Update the note-selection branch in the login flow to use
the resolved noWait value instead of stdoutIsTerminal. Revise both messages to
describe the actual blocking or immediate-return behavior determined by noWait,
without assuming anything about the output stream.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: befc402c-0aa6-4c6c-9a69-d8ccacd6eaa2
📒 Files selected for processing (9)
README.mdREADME.zh.mdcmd/auth/check.gocmd/auth/check_test.gocmd/auth/login.gocmd/auth/login_test.gocmd/config/bind_messages.gocmd/config/bind_warning_test.gointernal/auth/token_corrupted_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/auth/check.go
- internal/auth/token_corrupted_test.go
Comment on lines
+249
to
+250
| {name: "corrupted", accessToken: "", expiresIn: time.Hour, refreshIn: 24 * time.Hour, wantError: "corrupted_token"}, | ||
| {name: "expired", accessToken: "user-access-token", expiresIn: -2 * time.Hour, refreshIn: -time.Hour, wantError: "expired_token"}, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover the whitespace-only corrupted-token path.
TokenStatus classifies a whitespace-only AccessToken as corrupted after trimming it. This table tests only "". Add a " \t\n" fixture that expects "corrupted_token" so auth check cannot accept an unusable whitespace token.
As per coding guidelines, “Every behavior change must have an accompanying test.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/auth/check_test.go` around lines 249 - 250, Extend the TokenStatus test
table in cmd/auth/check_test.go with a whitespace-only AccessToken fixture such
as " \t\n", and assert it returns "corrupted_token", alongside the existing
empty-token corrupted case.
Source: Coding guidelines
Comment on lines
+281
to
+298
| err := authCheckRun(&CheckOptions{Factory: f, Scope: "im:message"}) | ||
| if err == nil { | ||
| t.Fatalf("expected a non-zero exit for a %s token", tc.name) | ||
| } | ||
| if got := output.ExitCodeOf(err); got != 1 { | ||
| t.Errorf("exit code = %d, want 1", got) | ||
| } | ||
|
|
||
| var got map[string]interface{} | ||
| if jsonErr := json.Unmarshal(stdout.Bytes(), &got); jsonErr != nil { | ||
| t.Fatalf("json.Unmarshal(stdout) error = %v, stdout:\n%s", jsonErr, stdout.String()) | ||
| } | ||
| if ok, _ := got["ok"].(bool); ok { | ||
| t.Fatalf("ok = true, want false; stdout:\n%s", stdout.String()) | ||
| } | ||
| if got["error"] != tc.wantError { | ||
| t.Fatalf("error = %v, want %s; stdout:\n%s", got["error"], tc.wantError, stdout.String()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert typed error metadata at the status boundary.
This test checks the rendered JSON error code and exit status only. Add assertions for the typed error category and subtype with errs.ProblemOf, and verify cause preservation where the status error wraps a lower-layer error. Keep the JSON assertions because they test the command contract.
Based on learnings, errs.ProblemOf exposes problem-level fields only; use errors.As for Param only when the error is a *errs.ValidationError.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/auth/check_test.go` around lines 281 - 298, Extend the error assertions
in the authCheckRun test using errs.ProblemOf to verify the typed problem
category and subtype, while retaining the existing exit-code and JSON contract
checks. Where the status error wraps a lower-layer cause, assert cause
preservation; use errors.As to inspect Param only when the error is a
*errs.ValidationError, since errs.ProblemOf exposes only problem-level fields.
Sources: Coding guidelines, Learnings
Comment on lines
+289
to
+301
| var got map[string]interface{} | ||
| if jsonErr := json.Unmarshal(stdout.Bytes(), &got); jsonErr != nil { | ||
| t.Fatalf("json.Unmarshal(stdout) error = %v, stdout:\n%s", jsonErr, stdout.String()) | ||
| } | ||
| if ok, _ := got["ok"].(bool); ok { | ||
| t.Fatalf("ok = true, want false; stdout:\n%s", stdout.String()) | ||
| } | ||
| if got["error"] != tc.wantError { | ||
| t.Fatalf("error = %v, want %s; stdout:\n%s", got["error"], tc.wantError, stdout.String()) | ||
| } | ||
| if _, present := got["granted"]; present { | ||
| t.Fatalf("granted must be absent for a %s token, stdout:\n%s", tc.name, stdout.String()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a typed response projection and assert the granted scope.
Replace map[string]interface{} with one typed auth check response projection. In TestAuthCheckRun_NeedsRefreshTokenStillReportsScopes, assert that granted contains exactly "im:message" and that no scope is missing. ok: true alone passes if scope reporting regresses.
As per coding guidelines, “Parse map[string]interface{} into typed structs at the boundary” and “contract tests must assert the changed field or behavior directly.”
Also applies to: 342-347
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/auth/check_test.go` around lines 289 - 301, Update the response parsing
in the affected auth check tests, especially
TestAuthCheckRun_NeedsRefreshTokenStillReportsScopes, to unmarshal into a typed
auth-check response projection instead of map[string]interface{}. Assert that
granted contains exactly the im:message scope, with no missing scope, while
retaining the existing ok and error assertions. Apply the same typed projection
to the additional affected assertion block.
Source: Coding guidelines
docs +search and base +title-resolve both call POST /open-apis/search/v2/doc_wiki/search, which accepts a tenant_access_token. Declaring user-only made the CLI reject --as bot locally, before any request was sent, so callers concluded the platform could not search docs under an app identity. drive +search already declared both identities for the same endpoint. Verified by a real bot-identity call after granting the search:docs:read bot scope, not from metadata alone: registry has no Search v2 record and the pinned SDK v3.7.2 reports user-only for this endpoint while v3.9.4 reports user and tenant.
github-actions
Bot
added
domain/base
sang-neo03
changed the title
vc +detail declared user-only, which broke the bot flow the
lark-vc-agent skill documents: after a bot joins a meeting it is told to
call +detail for note_id / minute_token, but the pre-flight identity
gate rejected the call before it reached the API.
Verified against the live API. GET /open-apis/vc/v1/meetings/{id} with a
tenant token succeeds once the bot is a participant, and keeps working
after the bot leaves. The earlier 121005 permission_denied that had this
endpoint written off came from probing a meeting the bot had never
joined — resource-level permission, not a token-type restriction, as
codemeta_vc.go already documents ("caller is not a participant").
The companion .../recording call returns 122001 (meeting status
unexpected) for an in-progress meeting with no recording, which is a
business state rather than an authorization rejection.
sang-neo03
changed the title
…ined meeting.get answers 121005 with "user lacks permission for the requested resource" whatever identity made the call. Under a bot that wording sends the caller to auth login, but no user authorization can fix it — the endpoint only serves participants and the app never joined the meeting. Rewrite that one combination to name the cause and point at vc +meeting-join, following the existing minutesReadError shape. A user caller and every other error code keep the upstream wording. This surfaced only once 43394b4 let a bot reach the endpoint at all, so it lands with that change rather than separately. Also pin the identity contracts from 06c738c and 43394b4 with AuthTypes assertions for docs +search, base +title-resolve and vc +detail. Their existing execution tests only ever pass --as user, so reverting any of the three declarations to user-only left the suite green. Each new assertion was verified to fail on revert. The lark-vc-agent skill told the agent to keep the bot identity for a meeting_id obtained by joining, then showed `vc +detail` with no --as, which falls back to the user identity and hits the same 121005 from the other side. Both call sites now carry the identity.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/vc/vc_detail_test.go`:
- Around line 45-77: Extend the “bot 121005 is rewritten” test around
meetingDetailPermissionError to assert that ProblemOf preserves the original
error’s Category and Subtype, and verify the returned problem retains the
original typed error as its cause. Keep the existing rewritten bot message and
hint assertions, and apply the same cause-preservation check where appropriate
without changing the user or other-code wording expectations.
In `@skills/lark-vc-agent/SKILL.md`:
- Line 79: Update the meeting artifact lookup command in the listed instruction
to pass the inherited identity via `--as`, preserving the bot identity when the
preceding join used `--as bot`; keep the user identity for user-joined meetings.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d9ee2f72-23a6-4320-914a-3f4e3e14d8c0
📒 Files selected for processing (5)
shortcuts/base/base_resolve_test.goshortcuts/doc/docs_search_test.goshortcuts/vc/vc_detail.goshortcuts/vc/vc_detail_test.goskills/lark-vc-agent/SKILL.md
Comment on lines
+45
to
+77
| t.Run("bot 121005 is rewritten", func(t *testing.T) { | ||
| err := errs.NewPermissionError(errs.SubtypePermissionDenied, upstream). | ||
| WithCode(recordingNoPermissionCode) | ||
| p, ok := errs.ProblemOf(meetingDetailPermissionError(bareMeetingQueryRuntime(core.AsBot), err)) | ||
| if !ok { | ||
| t.Fatalf("ProblemOf failed for %v", err) | ||
| } | ||
| if strings.Contains(p.Message, "user lacks permission") { | ||
| t.Errorf("Message = %q, want the bot-participant wording instead", p.Message) | ||
| } | ||
| if !strings.Contains(p.Hint, "vc +meeting-join") { | ||
| t.Errorf("Hint = %q, want it to point at vc +meeting-join", p.Hint) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("user identity keeps the upstream wording", func(t *testing.T) { | ||
| err := errs.NewPermissionError(errs.SubtypePermissionDenied, upstream). | ||
| WithCode(recordingNoPermissionCode) | ||
| p, _ := errs.ProblemOf(meetingDetailPermissionError(bareMeetingQueryRuntime(core.AsUser), err)) | ||
| if p.Message != upstream { | ||
| t.Errorf("Message = %q, want it left unchanged", p.Message) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("other codes keep the upstream wording", func(t *testing.T) { | ||
| const scopeMsg = "app scope not applied" | ||
| err := errs.NewPermissionError(errs.SubtypeAppScopeNotApplied, scopeMsg). | ||
| WithCode(output.LarkErrAppScopeNotEnabled) | ||
| p, _ := errs.ProblemOf(meetingDetailPermissionError(bareMeetingQueryRuntime(core.AsBot), err)) | ||
| if p.Message != scopeMsg { | ||
| t.Errorf("Message = %q, want it left unchanged", p.Message) | ||
| } | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert typed metadata and error preservation.
Line 45 checks only the rewritten message and hint. Assert that Category and Subtype remain unchanged. Also assert that meetingDetailPermissionError preserves the original typed error. This prevents a message-only rewrite from changing the error contract.
As per coding guidelines, “Error-path tests must assert typed metadata through errs.ProblemOf … and verify cause preservation rather than relying only on message substrings.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shortcuts/vc/vc_detail_test.go` around lines 45 - 77, Extend the “bot 121005
is rewritten” test around meetingDetailPermissionError to assert that ProblemOf
preserves the original error’s Category and Subtype, and verify the returned
problem retains the original typed error as its cause. Keep the existing
rewritten bot message and hint assertions, and apply the same cause-preservation
check where appropriate without changing the user or other-code wording
expectations.
Source: Coding guidelines
meeting.get only returns meetings the calling identity joined, so a meeting_id obtained by a bot join is unreadable as the user. The skill already stated that rule, but four copy-pasteable commands omitted --as and would silently fall back to the default identity: references/lark-vc-agent-meeting-join.md references/lark-vc-agent-meeting-events.md (two sites) references/lark-vc-agent-meeting-leave.md The two join/leave sites are bot-only flows and now pass --as bot. The two meeting-events sites are identity-neutral — that same error is reachable as the user, and the surrounding table already tells the reader to switch back to --as user when the meeting_id came from user discovery — so they say "carry the upstream identity" instead of hardcoding bot. Examples in lark-vc, lark-note, lark-calendar and lark-minutes are left alone: those start from a calendar entry or a minute, where the user identity is correct. Guarded by a test that walks the whole skill directory rather than the entry file, since fixing SKILL.md while leaving the references stale is exactly what happened here.
A completed login reports `"event": "authorization_complete"`, but the --no-wait payload carried no event at all. Both paths exit 0, so a caller had to infer "no token was stored" from the absence of a field that was never documented as a signal. The payload now reports `"event": "authorization_requested"`, using the same vocabulary. Additive only — no existing field changes meaning, so callers that read device_code or verification_url are unaffected. Both names move into constants so the pair cannot drift, while the tests keep asserting the literal strings: following the constant there would let a rename pass CI while breaking every caller.
Reverts the unattended-login change: auth login no longer decides whether to block from whether stdout is a terminal, and the --wait flag it added is gone. The owner asked for this to land as its own change rather than riding along with the token fixes. Reverted to the merge base, not to origin/main. login.go and login_test.go moved on main after this branch forked (#2189), and taking those files from origin/main would have pulled that work in under a revert commit. Merging main will bring it in on its own. This also undoes a mistake of mine: main already emits `"event": "device_authorization"` in the --no-wait payload. The reverted change had dropped that field while rewriting the block, and I "restored" it under a different name (authorization_requested), which would have broken any caller matching on the original value. Reverting the file restores the original name. Still in this PR: the corrupted / expired stored-token fixes, and bot auth for docs +search, base +title-resolve and vc +detail.
sang-neo03
changed the title
SKILL.md's post-meeting step still showed a bare `lark-cli vc +detail --meeting-ids <meeting.id>`; the identity requirement sat in the prose beside it. An agent copies the command, not the sentence, so it still fell back to the default identity. The command now carries `--as <upstream identity>`, matching the two meeting-events sites; the join and leave sites keep `--as bot` because those flows are bot-only. The check that was supposed to catch this had a false negative: it matched whole lines, and that line did contain "--as" — inside the sentence explaining that --as bot is required. It now inspects only the spans a reader would copy (fenced code-block lines, and the contents of inline-code spans), so prose can no longer mask a bare command. Sequenced deliberately: the new predicate was run before touching SKILL.md and failed on exactly that line, which is what shows it catches the case the old one missed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two correctness problems in how the CLI treats stored credentials and identity.
auth statusandauth checkreport a stored token as usable when it carries no access token at all or has fully expired, so an unusable credential looks healthy. Separately, three shortcuts reject--as botin a local pre-flight check even though the endpoints they call accept an app token, which makes callers conclude the platform cannot do something it can.The non-TTY
auth logindefault that this branch originally carried has been reverted and will be proposed separately.Changes
TokenStatusreports a newcorruptedstate when a stored record parses but carries no usable access token.encoding/jsondrops unknown fields silently, so a writer that misspells the field name leaves healthy timestamps, a full scope list, and no token — previously reported as valid.== "expired"/== "valid"string literals were how consumers came to treat an unknown status as usable.auth statusreports it,GetValidAccessTokenfails with a named cause instead of returning an empty bearer token,--as autofalls back to bot, andauth checkno longer answers "granted" from the scope list of an unusable record.auth checkalso rejects a fully expired token, which it previously reported asok: true.auth statuskeepsstatus: "missing"rather than adding a new top-level enum value; the detail rides intokenStatus: "corrupted". Note thatauth listandprofile listexpose the raw status, socorruptedis visible there.docs +searchandbase +title-resolveaccept--as bot. Both callPOST /open-apis/search/v2/doc_wiki/search, anddrive +searchalready declared both identities for that same endpoint — the CLI gave two different answers for one API.vc +detailaccepts--as bot. The rawvc meeting getcommand has always declared both identities forGET /open-apis/vc/v1/meetings/{meeting_id}, and the documented app-identity flow in thelark-vc-agentskill (join a meeting, then readnote_id/minute_token) could not complete because the local gate rejected the call before it was sent.121005message reads "user lacks permission" whatever identity called, which points an app-identity caller atauth logineven though only joining the meeting can help. Rewritten for that one combination, following the existingminutesReadErrorshape in the same package; a user caller and every other error code keep the upstream wording.lark-vc-agentskill carries the calling identity through all its examples.meeting.getonly returns meetings the calling identity joined, so six copy-pasteable commands that omitted--aswould silently fall back to the default identity.--as user, so any declaration could previously be reverted to user-only with the package tests still green.Scope
vc +notes,vc +recording,minutes +detailand threemailshortcuts were evaluated and deliberately left atuser:vc +notesand the--calendar-event-idsbranch ofvc +recordingcall endpoints that reject a tenant token outright (99991663).minutes +detaildepends on an endpoint that returns a business error under a tenant token. Telling "the endpoint does not serve app identities" apart from "this app has no claim to this particular resource" would require a minute owned by the app, which an app identity cannot create. Left unchanged rather than guessed at.mailshortcuts default--mailboxtome, which has no meaning for an app identity.vc +detail's declared scopes remain the user-side ones, so identity-aware scope hints for the bot path are not accurate yet. Splitting them needs the app-side scope requirement confirmed first, so it is left out of this change.Test Plan
go test -raceandgo test -gcflags="all=-N -l"each run clean across all packages.make unit-testcombines both flags, which segfaults insideruntime.mainon go1.26.0 darwin/arm64 before any test executes; the same crash reproduces on a cleanmain, so it is a local toolchain issue (CI runs linux/amd64).go vet ./...,gofmt -l .andgo mod tidyclean.origin/mainmerged in; full suite green afterwards.internal/authandinternal/identitydiag; dropping the expired branch inauth checkreddens its subtest; reverting the threeAuthTypesdeclarations plus the121005rewrite reddens 4 assertions, each printing actual-vs-want; removing--asfrom one skill reference file reddens the skill check with the exact file and line.POST /open-apis/search/v2/doc_wiki/search --as botreturns results with thesearch:docs:readapp scope granted, anddocs +search/base +title-resolvenow reach it. Before the grant the same call returned99991672 app_scope_not_applied— a missing grant, which is not evidence either way about the token type.vc +detailverified end to end with a build of this branch: after joining a meeting viavc +meeting-join --as bot, bothapi GET /open-apis/vc/v1/meetings/{id} --as botandvc +detail --as botreturn the meeting, and access persists after leaving. Against a meeting the app never joined, the rewritten message appears instead of the upstream "user lacks permission" wording.userwere each probed directly rather than inferred from metadata; the observed codes are quoted in the Scope section above.auth status --jsonon a valid credential still reportsready/available: true/valid— no false positives. The corruption case is covered by a test that writes a token file with a misspelled field name and assertsstatus: missingplustokenStatus: corrupted.Related Issues
Summary by CodeRabbit
--waitand--no-wait.