feat: implement authentication response logging#235
Merged
JackZhao10086 merged 16 commits intomainfrom Apr 3, 2026
Merged
Conversation
Add flag to track if cleanup has run and prevent duplicate executions Add test to verify cleanup only runs once
|
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:
📝 WalkthroughWalkthroughAdds centralized auth API path constants and a new auth-response logging subsystem; integrates logging calls into OAuth/device/app-registration/verify flows, updates tests to assert logging behavior and cmdline truncation, and adds doc comments across the auth package. (40 words) Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "lark-cli (User)"
participant Auth as "internal/auth"
participant HTTP as "Auth Provider (HTTP)"
participant FS as "Filesystem (logs dir)"
CLI->>Auth: initiate auth action (device/app/verify)
Auth->>HTTP: send HTTP request (paths from constants)
HTTP-->>Auth: HTTP response (status, headers, body)
Auth->>Auth: handle/parse response
Auth->>Auth: call logHTTPResponse / logSDKResponse
Auth->>FS: open/append `auth-YYYY-MM-DD.log`
FS-->>Auth: write acknowledged
Auth->>Auth: schedule async cleanup (delete logs >7 days)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
the
size/L
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
internal/auth/device_flow_test.go (1)
84-86: Consider clarifying thepath=missingexpectation.The test asserts
path=missingin the log output despite the stub being registered withPathDeviceAuthorization. This suggestslogHTTPResponsecannot extract the request path from*http.Response(possibly becauseresp.Requestis nil or the mock doesn't populate it).Adding a brief comment explaining why
path=missingis expected would improve test maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/device_flow_test.go` around lines 84 - 86, The assertion checks for "path=missing" because the test stub registered with PathDeviceAuthorization does not populate resp.Request, so logHTTPResponse cannot extract the request path and falls back to "missing"; update the test near the strings.Contains(got, "path=missing") assertion to include a short comment explaining that resp.Request is nil in the mock (or that logHTTPResponse reads resp.Request.URL.Path), referencing PathDeviceAuthorization and logHTTPResponse so future readers understand why "path=missing" is the expected log output.internal/auth/auth_response_log.go (3)
51-55: Cleanup runs even when write fails.The cleanup goroutine is spawned unconditionally after the IIFE returns, regardless of whether the write succeeded. Consider moving cleanup inside the success path or checking the error before spawning.
Proposed fix
n, err = func() (int, error) { // ... existing code ... }() - go authResponseLogCleanup(dir, now) + if err == nil { + go authResponseLogCleanup(dir, now) + } return n, err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/auth_response_log.go` around lines 51 - 55, The cleanup goroutine authResponseLogCleanup(dir, now) is started unconditionally even if the write failed; change the logic so the goroutine is spawned only on success by checking the returned err from the IIFE (the value returned to n, err) and only calling go authResponseLogCleanup(dir, now) when err == nil (i.e., move or guard the call so cleanup runs only in the successful write path before returning n, err).
106-120: Potential timestamp inconsistency between log filename and entry.
authResponseLogNow()is called once indefaultLogWriter.Write()(line 30) to determine the log filename, and again here (line 114) for the entry timestamp. If a write spans midnight, the entry timestamp could be for a different day than the log file it's written to.Consider passing the timestamp as a parameter or accepting this minor inconsistency as acceptable for debugging purposes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/auth_response_log.go` around lines 106 - 120, The log entry timestamp can drift from the filename because doLogAuthResponse calls authResponseLogNow() independently; change doLogAuthResponse(path string, status int, logID string) to accept a timestamp parameter (e.g., now time.Time) and use that for the entry timestamp instead of authResponseLogNow(), then have defaultLogWriter.Write compute now once (currently calling authResponseLogNow()) and pass that same now into doLogAuthResponse so filename and entry timestamp are consistent; update all callers of doLogAuthResponse accordingly and remove the extra authResponseLogNow() call in the log path.
28-38: Consider validating that config directory is absolute, or document the relative path fallback behavior.The
GetConfigDir()function (frominternal/core/config.go:63-74) returns a relative path (.lark-cli) when the home directory cannot be determined. This causes logs to be written relative to the current working directory. While the function prints a warning to stderr when this occurs, theWrite()method in this file doesn't explicitly handle or document this edge case. Consider adding validation withfilepath.IsAbs()to ensure consistent behavior, or add a comment explaining why relative paths are acceptable here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/auth_response_log.go` around lines 28 - 38, In defaultLogWriter.Write (the Write method) validate the directory returned by core.GetConfigDir() is absolute using filepath.IsAbs; if it is not absolute, call filepath.Abs to resolve it (or explicitly join it to a known base like os.UserHomeDir when available) before using it to create logs, and update or add a short comment near authResponseLogNow/logMu explaining the fallback behavior so callers know logs may be written relative to CWD only when GetConfigDir() cannot find the home directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/auth/auth_response_log.go`:
- Line 53: The Write() method is starting authResponseLogCleanup on every call
and outside the write-success path; add a package-level sync.Once (e.g.,
cleanupOnce) and use cleanupOnce.Do to launch the goroutine so
authResponseLogCleanup(dir, now) runs only once per process, and move that
guarded call inside the successful-write branch of Write() (after the write
returns nil) so cleanup only starts when the log write succeeded.
In `@internal/auth/verify_test.go`:
- Around line 55-61: Update the failing test case for the "non-JSON response"
scenario so it expects logging to occur: change the test case field wantLog from
false to true for the case where body is "not json". This aligns the test with
verify.go's behavior where logSDKResponse is called unconditionally after sdk.Do
(before json.Unmarshal), so ensure the test case in verify_test.go reflects that
by setting wantLog: true for the "non-JSON response" test entry.
---
Nitpick comments:
In `@internal/auth/auth_response_log.go`:
- Around line 51-55: The cleanup goroutine authResponseLogCleanup(dir, now) is
started unconditionally even if the write failed; change the logic so the
goroutine is spawned only on success by checking the returned err from the IIFE
(the value returned to n, err) and only calling go authResponseLogCleanup(dir,
now) when err == nil (i.e., move or guard the call so cleanup runs only in the
successful write path before returning n, err).
- Around line 106-120: The log entry timestamp can drift from the filename
because doLogAuthResponse calls authResponseLogNow() independently; change
doLogAuthResponse(path string, status int, logID string) to accept a timestamp
parameter (e.g., now time.Time) and use that for the entry timestamp instead of
authResponseLogNow(), then have defaultLogWriter.Write compute now once
(currently calling authResponseLogNow()) and pass that same now into
doLogAuthResponse so filename and entry timestamp are consistent; update all
callers of doLogAuthResponse accordingly and remove the extra
authResponseLogNow() call in the log path.
- Around line 28-38: In defaultLogWriter.Write (the Write method) validate the
directory returned by core.GetConfigDir() is absolute using filepath.IsAbs; if
it is not absolute, call filepath.Abs to resolve it (or explicitly join it to a
known base like os.UserHomeDir when available) before using it to create logs,
and update or add a short comment near authResponseLogNow/logMu explaining the
fallback behavior so callers know logs may be written relative to CWD only when
GetConfigDir() cannot find the home directory.
In `@internal/auth/device_flow_test.go`:
- Around line 84-86: The assertion checks for "path=missing" because the test
stub registered with PathDeviceAuthorization does not populate resp.Request, so
logHTTPResponse cannot extract the request path and falls back to "missing";
update the test near the strings.Contains(got, "path=missing") assertion to
include a short comment explaining that resp.Request is nil in the mock (or that
logHTTPResponse reads resp.Request.URL.Path), referencing
PathDeviceAuthorization and logHTTPResponse so future readers understand why
"path=missing" is the expected log output.
🪄 Autofix (Beta)
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
Run ID: 6424c23e-abef-48c6-9539-cd2a2b6bfc2e
📒 Files selected for processing (9)
cmd/auth/auth.gointernal/auth/app_registration.gointernal/auth/auth_response_log.gointernal/auth/device_flow.gointernal/auth/device_flow_test.gointernal/auth/paths.gointernal/auth/uat_client.gointernal/auth/verify.gointernal/auth/verify_test.go
Greptile SummaryThis PR adds structured authentication response logging to the Lark CLI — writing per-day log files under Key observations:
Confidence Score: 4/5Safe to merge with the ErrNotFound issue resolved or documented; all remaining findings are P2 style/hygiene. One P2 finding (ErrNotFound never produced) borders on a correctness concern: an exported public API surface creates a false expectation for callers but does not break any currently-working code path. Treating it conservatively yields a 4 — the author should clarify intent before it becomes a footgun for the next developer who checks errors.Is(err, keychain.ErrNotFound). internal/keychain/keychain.go (ErrNotFound sentinel) and internal/keychain/auth_log.go (SetAuthLogHooksForTest concurrency) Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as CLI command
participant DF as device_flow / app_registration / uat_client / verify
participant HTTP as HTTP / SDK client
participant Log as keychain.LogAuthResponse
participant File as auth-YYYY-MM-DD.log
CLI->>DF: invoke auth flow
DF->>HTTP: make request
HTTP-->>DF: http.Response / larkcore.ApiResp
DF->>Log: logHTTPResponse(resp) / logSDKResponse(path, apiResp)
Log->>Log: initAuthLogger() [sync.Once]
Log->>File: open auth-DATE.log (O_APPEND|O_CREATE)
Log->>File: cleanupOldLogs(dir, now) [removes files >7 days old]
Log->>File: Printf(auth-response: time=… path=… status=… …)
DF-->>CLI: return result / error
Reviews (11): Last reviewed commit: "refactor(auth): move auth logging logic ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@4be056571a0760fbaf70d8b69f1244c19d503305🧩 Skill updatenpx skills add larksuite/cli#feat/auth_err_log -y -g |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/auth/verify_test.go (1)
57-63:⚠️ Potential issue | 🟠 Major
wantLog: falseis incorrect for the "non-JSON response" test case.Per
verify.go,logSDKResponseis called at line 27 beforejson.Unmarshalat line 33. Sincesdk.Do()succeeds (HTTP 200), logging occurs unconditionally regardless of whether JSON parsing subsequently fails.{ name: "non-JSON response", body: "not json", wantErr: true, errSubstr: "invalid character", - wantLog: false, + wantLog: true, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/verify_test.go` around lines 57 - 63, The test case "non-JSON response" in verify_test.go is wrong: logging happens unconditionally when sdk.Do() succeeds because verify.go calls logSDKResponse before json.Unmarshal, so update the test case (the struct for name "non-JSON response") to set wantLog: true (and keep wantErr true / errSubstr "invalid character") so the expected log assertion matches logSDKResponse being invoked; reference the test case name and the logSDKResponse call in verify.go to locate where the behavior originates.
🧹 Nitpick comments (1)
internal/auth/device_flow_test.go (1)
87-89: Assert the real auth path instead ofpath=missing.At Line 87, this assertion validates a mock artifact (
resp.Requestis absent) rather than expected logging behavior. It weakens coverage for request-path logging regressions.Suggested adjustment
- if !strings.Contains(got, "path=missing") { - t.Fatalf("expected path in log, got %q", got) - } + if !strings.Contains(got, "path="+PathDeviceAuthorization) { + t.Fatalf("expected auth path in log, got %q", got) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/device_flow_test.go` around lines 87 - 89, The test currently asserts that the log contains the placeholder "path=missing" because the mocked HTTP response lacks a populated resp.Request; instead populate resp.Request (with a URL.Path or RequestURI set to the real auth endpoint) or construct the mock response using the same test auth server URL, then update the assertion that inspects got to expect that actual path string (e.g., "/device" or the auth handler's path) rather than "path=missing"; modify the mock resp object (resp.Request) and the test assertion that checks got so it validates the real request path produced by the code under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/auth/device_flow_test.go`:
- Around line 165-189: The test TestDefaultLogWriter_ReleasesLockOnPanic is
invalid because authResponseLogNow() is invoked before the mutex (logMu) is
acquired, so the injected panic never exercises lock-release behavior; fix by
either (A) renaming the test to reflect it only verifies a panic from
authResponseLogNow (e.g., TestDefaultLogWriter_PanicsBeforeLock) and update
assertions/comments accordingly, or (B) add a dedicated test hook that runs
inside the locked section and use that hook to panic: add a new package-level
variable (e.g., authResponseLogInsideLock or authResponseLogPanicHook) and have
defaultLogWriter.Write (or the function in auth_response_log.go that currently
acquires logMu) invoke that hook after logMu.Lock() and before returning; then
update the test to set that hook to panic and assert the mutex is released.
Ensure changes reference defaultLogWriter.Write, authResponseLogNow, logMu, and
the new hook name so the test actually exercises lock-release semantics.
---
Duplicate comments:
In `@internal/auth/verify_test.go`:
- Around line 57-63: The test case "non-JSON response" in verify_test.go is
wrong: logging happens unconditionally when sdk.Do() succeeds because verify.go
calls logSDKResponse before json.Unmarshal, so update the test case (the struct
for name "non-JSON response") to set wantLog: true (and keep wantErr true /
errSubstr "invalid character") so the expected log assertion matches
logSDKResponse being invoked; reference the test case name and the
logSDKResponse call in verify.go to locate where the behavior originates.
---
Nitpick comments:
In `@internal/auth/device_flow_test.go`:
- Around line 87-89: The test currently asserts that the log contains the
placeholder "path=missing" because the mocked HTTP response lacks a populated
resp.Request; instead populate resp.Request (with a URL.Path or RequestURI set
to the real auth endpoint) or construct the mock response using the same test
auth server URL, then update the assertion that inspects got to expect that
actual path string (e.g., "/device" or the auth handler's path) rather than
"path=missing"; modify the mock resp object (resp.Request) and the test
assertion that checks got so it validates the real request path produced by the
code under test.
🪄 Autofix (Beta)
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
Run ID: 30bbf34f-6bc3-4b09-a1d2-d7bf74d215a4
📒 Files selected for processing (5)
internal/auth/app_registration_test.gointernal/auth/device_flow_test.gointernal/auth/scope_test.gointernal/auth/uat_client_options_test.gointernal/auth/verify_test.go
✅ Files skipped from review due to trivial changes (3)
- internal/auth/app_registration_test.go
- internal/auth/scope_test.go
- internal/auth/uat_client_options_test.go
github-actions
bot
added
size/XL
github-actions
bot
added
size/L
liangshuo-1
approved these changes
Apr 3, 2026
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
2 participants
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
This PR introduces response logging for authentication flows to improve debugging and auditability. It also centralizes authentication path constants and implements a background cleanup mechanism to prevent unbounded log growth.
Changes
authResponseLogWriterto record HTTP and SDK authentication responses (including status code, path,x-tt-logid, and command line arguments).internal/auth/paths.go.cleanupOldLogsto automatically remove log files older than 7 days, including safety mechanisms to prevent duplicate background execution per process.device_flow.go,verify.go,uat_client.go, andapp_registration.goto integrate the new logging utility.device_flow_test.goandverify_test.goto cover the new logging logic and the cleanup mechanism.Test Plan
lark authrelated commands worksRelated Issues
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation