Skip to content

fix: write skills state timestamps in local time - #1384

Closed
zhangheng023 wants to merge 1 commit into
mainfrom
fix/skills-state-local-time
Closed

fix: write skills state timestamps in local time#1384
zhangheng023 wants to merge 1 commit into
mainfrom
fix/skills-state-local-time

Conversation

@zhangheng023

@zhangheng023 zhangheng023 commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

lark-cli update wrote skills-state.json.updated_at in UTC even when the
sync ran in a local timezone. This PR preserves the local RFC3339 offset for
both incremental sync and full-install fallback state writes.

Changes

  • Preserve the original time.Time offset in internal/skillscheck/sync.go
    for incremental sync state writes
  • Preserve the original time.Time offset in internal/skillscheck/sync.go
    for full-install fallback state writes
  • Add offset-preservation regression tests in
    internal/skillscheck/sync_test.go for +08:00 and -07:00

Test Plan

  • make unit-test passed
  • validate passed
  • skipped: local-eval passed (lite mode; no shortcut/skill change and no
    applicable sandbox E2E or skillave layer)
  • acceptance-reviewer passed (2/2 cases)
  • manual verification: make build, ./lark-cli update --help, go test ./internal/skillscheck -run '^TestSyncSkills_WritesStateAndDoesNotWriteStamp$' -count=1 -v, go test ./internal/skillscheck -run '^TestSyncSkills_ParseEmptyLocalListsFallBackToFullInstall$' -count=1 -v

Related Issues

N/A

Summary by CodeRabbit

Bug Fixes

  • Fixed timestamp formatting for skill state updates to properly preserve local timezone information instead of always converting to UTC.

@zhangheng023 zhangheng023 added bug Something isn't working domain/core CLI framework and core libraries size/S Low-risk docs, CI, test, or chore only changes labels Jun 10, 2026
@github-actions github-actions Bot added size/M Single-domain feat or fix with limited business impact and removed domain/core CLI framework and core libraries size/S Low-risk docs, CI, test, or chore only changes labels Jun 10, 2026
@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: af32bccc-e9e3-4ad1-9ac7-f2c088f27e8c

📥 Commits

Reviewing files that changed from the base of the PR and between e794fd5 and ee5868b.

📒 Files selected for processing (2)
  • internal/skillscheck/sync.go
  • internal/skillscheck/sync_test.go

📝 Walkthrough

Walkthrough

This PR changes how SkillsState.UpdatedAt timestamps are formatted in two sync paths. The code removes .UTC() conversion before RFC3339 formatting, storing timestamps with local timezone offsets instead of UTC. Tests are updated to inject deterministic time zones and expect corresponding local timezone representations.

Changes

Timestamp formatting

Layer / File(s) Summary
Remove UTC conversion from sync methods
internal/skillscheck/sync.go
SyncSkills and fallbackFullInstall now format SkillsState.UpdatedAt using local timezone instead of converting to UTC first, changing the timezone/offset in the RFC3339 string.
Update test expectations for local timezones
internal/skillscheck/sync_test.go
Both TestSyncSkills_WritesStateAndDoesNotWriteStamp and TestSyncSkills_ParseEmptyLocalListsFallBackToFullInstall inject deterministic Now functions with fixed timezone offsets (UTC+8 and UTC-7) and update expected state.UpdatedAt assertions to match the new local RFC3339 format.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • larksuite/cli#1042: Both PRs touch SyncSkills's persisted SkillsState.UpdatedAt RFC3339 timestamp formatting and expected test values.
  • larksuite/cli#1233: Both PRs modify internal/skillscheck/sync.go around fallbackFullInstall behavior and timestamp handling.

Suggested labels

size/S

Suggested reviewers

  • GeekyMax
  • MaxHuang22
  • liangshuo-1

Poem

🐰 A timestamp hops from UTC's embrace,
Now wearing local offset's tender face,
Tests dance with timezones far and near,
RFC3339 strings hold true and clear!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: preserving local timezone offsets when writing skills state timestamps, moving from UTC to local time format.
Description check ✅ Passed The description includes all required sections: Summary, Changes, Test Plan with checkboxes showing test execution status, and Related Issues. Content is complete and specific.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/skills-state-local-time

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@ee5868be78aeb9a93e5dd8dd716cd49c9ceaf7c1

🧩 Skill update

npx skills add larksuite/cli#fix/skills-state-local-time -y -g

@codecov

codecov Bot commented Jun 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.95%. Comparing base (9d84544) to head (ee5868b).
⚠️ Report is 30 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1384      +/-   ##
==========================================
+ Coverage   71.20%   71.95%   +0.74%     
==========================================
  Files         688      699      +11     
  Lines       65997    66285     +288     
==========================================
+ Hits        46996    47695     +699     
+ Misses      15340    14883     -457     
- Partials     3661     3707      +46     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size/M Single-domain feat or fix with limited business impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant