Skip to content

fix(ci): handle missing server log files in cleanup steps#768

Closed
TimeToBuildBob wants to merge 1 commit intoActivityWatch:masterfrom
TimeToBuildBob:fix/ci-log-cleanup-step
Closed

fix(ci): handle missing server log files in cleanup steps#768
TimeToBuildBob wants to merge 1 commit intoActivityWatch:masterfrom
TimeToBuildBob:fix/ci-log-cleanup-step

Conversation

@TimeToBuildBob
Copy link
Contributor

@TimeToBuildBob TimeToBuildBob commented Feb 22, 2026

Summary

The CI cleanup steps ("Print server logs" and "Move logs to subdir") fail when aw-server-rust master doesn't write log files to ~/.cache/activitywatch/log/*/*.log. The bash glob expands to a literal path that cat/mv can't find, causing the step to exit with code 1.

This has been causing the "Test (node-20, py-3.9, aw-server-rust master)" job to appear as failed even when all actual tests pass.

Fix: Use shopt -s nullglob with a loop so empty globs produce zero iterations instead of a failed expansion.

Test plan

  • Verify the "aw-server-rust master" test job no longer fails on the cleanup step
  • Verify log files are still collected when they do exist (pinned version test jobs)

Important

Fix CI cleanup steps in nodejs.yml to handle missing log files using shopt -s nullglob.

  • CI Fix:
    • Use shopt -s nullglob in Print server logs to console and Move logs to subdir steps in nodejs.yml to handle missing log files gracefully.
    • Prevents CI job failures when aw-server-rust master does not produce log files.
  • Test Plan:
    • Ensure "aw-server-rust master" test job does not fail on cleanup.
    • Verify log files are collected when present.

This description was created by Ellipsis for 1b09b62. You can customize this summary. It will automatically update as commits are pushed.

The "Print server logs" and "Move logs to subdir" steps fail when
aw-server-rust master doesn't write logs to the expected path
(~/.cache/activitywatch/log/*/*.log). The glob expands to a literal
string that cat/mv can't find, causing the step to exit with code 1.

Fix: use `shopt -s nullglob` + loop so empty globs produce no
iterations instead of a failed glob expansion.
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed everything up to 1b09b62 in 5 seconds. Click for details.
  • Reviewed 26 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

Workflow ID: wflow_EJgJpDAmHyE574Hj

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 22, 2026

Greptile Summary

Fixed CI cleanup steps that were failing when aw-server-rust master doesn't write log files. The bash glob ~/.cache/activitywatch/log/*/*.log would expand to a literal string when no files matched, causing cat and mv commands to fail with exit code 1.

Changes:

  • Added shopt -s nullglob to both "Print server logs" and "Move logs to subdir" steps so empty globs produce zero loop iterations instead of failed expansions
  • Converted mv command to a loop for consistency and better handling of multiple files
  • Added proper quoting around $file variables to handle edge cases with special characters in filenames

This ensures the "aw-server-rust master" test job completes successfully even when log files don't exist, while still collecting logs when they are present in other test configurations.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The changes are simple, well-targeted CI configuration improvements that use standard bash practices (nullglob) to handle missing files gracefully. The fix directly addresses a known CI failure without introducing any new complexity or potential side effects.
  • No files require special attention

Important Files Changed

Filename Overview
.github/workflows/nodejs.yml Adds nullglob to prevent CI failures when log files don't exist, improving reliability for cleanup steps

Last reviewed commit: 1b09b62

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@codecov
Copy link

codecov bot commented Feb 22, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 25.60%. Comparing base (86a71c8) to head (1b09b62).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #768   +/-   ##
=======================================
  Coverage   25.60%   25.60%           
=======================================
  Files          30       30           
  Lines        1746     1746           
  Branches      319      319           
=======================================
  Hits          447      447           
- Misses       1233     1277   +44     
+ Partials       66       22   -44     

☔ View full report in Codecov by Sentry.
📢 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.

@ErikBjare
Copy link
Member

But then where are the log files written? Was this a regression from our recent appdirs -> dirs migration? L

Should probably figure out where the log file went instead of ignoring the failed expansion.

@TimeToBuildBob
Copy link
Contributor Author

Good catch — you're right, this is papering over a real regression.

I investigated and the issue is in aw-server-rust PR ActivityWatch/aw-server-rust#562 (appdirs → dirs migration). The old appdirs::user_log_dir appended /log to the cache directory:

Platform Old (appdirs) New (dirs)
Linux ~/.cache/activitywatch/log/aw-server-rust/ ~/.cache/activitywatch/aw-server-rust/
macOS ~/Library/Logs/activitywatch/aw-server-rust/ ~/Library/Caches/activitywatch/aw-server-rust/
Windows {APPDATA}/activitywatch/Logs/aw-server-rust/ {LOCALAPPDATA}/activitywatch/aw-server-rust/

The /log path component was dropped because dirs has no log_dir() equivalent and the PR incorrectly stated the paths were the same. The macOS regression is the most significant since it moves logs from the proper ~/Library/Logs/ to ~/Library/Caches/.

The fix should be in aw-server-rust's get_log_dir() — restore the /log subdirectory (at minimum on Linux), or use proper platform log directories.

This CI cleanup PR should probably wait until that's fixed, since the glob path ~/.cache/activitywatch/log/*/*.log was correct before.

I'll submit a fix PR to aw-server-rust.

@TimeToBuildBob
Copy link
Contributor Author

Fix PR submitted: ActivityWatch/aw-server-rust#565 — restores the /log subdirectory with platform-appropriate paths (Linux: ~/.cache/activitywatch/log/, macOS: ~/Library/Logs/activitywatch/, Windows: {LOCALAPPDATA}/activitywatch/Logs/) and adds a regression test.

@ErikBjare
Copy link
Member

That PR was merged! So this PR shouldn't be needed anymore. Closing.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants