Skip to content

fix(cli): wmill sync pull updates wmill-lock.yaml for raw apps#9289

Merged
rubenfiszel merged 1 commit into
mainfrom
sync-pull-wmill-locks-raw-apps
May 22, 2026
Merged

fix(cli): wmill sync pull updates wmill-lock.yaml for raw apps#9289
rubenfiszel merged 1 commit into
mainfrom
sync-pull-wmill-locks-raw-apps

Conversation

@hugocasa
Copy link
Copy Markdown
Collaborator

@hugocasa hugocasa commented May 22, 2026

Summary

wmill sync pull was failing to refresh wmill-lock.yaml entries for raw apps. The hashes for raw-app inline scripts/backend runnables stayed stale after pulling remote changes, even though the loop was visibly logging Updating lock metadata for raw app ….

The raw-apps loop in pull() was passing dryRun: true to generateAppLocksInternal, which makes the function return without ever calling updateMetadataGlobalLock. The sibling normal-apps loop directly below already passes dryRun: false — this was a copy/paste leftover from the push side, where dryRun: true is intentional (push only consumes the staleness return value).

History: commit 99b1ae2e0 introduced the buggy dryRun: true; cd4151a84 later fixed the sibling rawApp flag but missed dryRun.

Changes

  • Flip dryRun from true to false in the raw-apps loop of pull() (cli/src/commands/sync/sync.ts:2861) so locks are actually written.

Test plan

  • In a workspace with a raw app, edit a backend runnable on the remote, run wmill sync pull locally, and confirm the raw app's hash in wmill-lock.yaml changes.
  • Immediately run wmill sync push --dry-run and confirm the raw app is reported as up-to-date (no stale-lock warning).

Summary by cubic

Fixes wmill sync pull not updating wmill-lock.yaml for raw apps. Lock metadata now persists, so hashes for inline scripts and backend runnables refresh after pulling and subsequent pushes show up-to-date.

  • Bug Fixes
    • Set dryRun: false when calling generateAppLocksInternal for raw apps during pull, ensuring updateMetadataGlobalLock runs and writes the lock.

Written for commit 42de9b6. Summary will update on new commits. Review in cubic

@hugocasa hugocasa marked this pull request as ready for review May 22, 2026 09:51
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 22, 2026

Claude finished @hugocasa's task in 1m 31s —— View job


Good to merge

Findings

The one-line change is correct and minimal.

Signature tracegenerateAppLocksInternal(appFolder, rawApp, dryRun, workspace, opts, justUpdateMetadataLock?, noStaleMessage?, tree?) (app_metadata.ts:146).

Why the old dryRun: true was wrong on the pull path — the pull loop does not pass a tree, so execution falls into the "Legacy behaviour" branch (app_metadata.ts:246). With dryRun: true and a stale app, the function returns remote_path at line 262 and never reaches the updateMetadataGlobalLock call at line 372 — so wmill-lock.yaml is never refreshed. With dryRun: false, justUpdateMetadataLock: true still short-circuits the inline-script re-upload block at line 278, but execution continues past it to the clearGlobalLock + updateMetadataGlobalLock block at lines 370-373, which is the desired effect.

Sibling parity — the normal-apps loop right below (sync.ts:2868-2879) already passes (rawApp=false, dryRun=false, …, justUpdateMetadataLock=true, noStaleMessage=true). After this fix, the raw-apps loop matches the normal-apps loop except for the rawApp flag — the expected shape.

Push side is unaffected — the push-side pass 1 (sync.ts:3418 / :3432) intentionally passes dryRun: true because it only consumes the return value to collect staleApps, and pass 2 (the autoRegenerate branch at :3501 / :3515) already passes dryRun: false. The PR description's account of the bug origin matches what's in the tree.

AGENTS.md compliance

No violations — single-line behavior fix, no new public surface, no module placement concerns.

Test coverage

  • Backend: not touched.
  • Frontend / CLI: this is a CLI bug fix. There are no surrounding *.test.ts siblings for sync.ts raw-app behavior, so per AGENTS.md guidance ("only flag missing tests for new pure-logic utilities…"), no automated test is required, but the bug class (silent staleness in lock metadata) is exactly the kind that a small fixture-based test of the raw-apps pull loop would catch. Worth considering as a follow-up; not a blocker for this PR.
  • Manual verification: exercise the test plan as written — set up a workspace with a raw app, modify a backend runnable on the remote, run wmill sync pull, and confirm the raw app's hashes in wmill-lock.yaml change. Then run wmill sync push --dry-run and confirm no stale-lock warning. There is no in-app UI to exercise.

  • Read AGENTS.md and the changed file
  • Verify the dryRun: true → false fix is correct
  • Check the sibling normal-apps loop for parity
  • Inspect generateAppLocksInternal behavior on dryRun
  • Post review
    · Branch

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

No issues found across 1 file

Re-trigger cubic

@github-actions
Copy link
Copy Markdown
Contributor

The PR is a single-line, one-character fix. The change is clearly correct based on the function's logic:

  • generateAppLocksInternal's third parameter is dryRun. When true and the app hash is stale, the legacy code path (no tree argument) returns the path string early at line ~257 without reaching updateMetadataGlobalLock (lines ~365-372).
  • The pull() function calls this without a tree argument, so it takes the legacy path.
  • The sibling normal-apps loop at line 2870 already passes dryRun: false.
  • The push-side first pass uses dryRun: true for staleness detection, then a second pass with dryRun: false writes locks — this was the intended pattern that the pull-side raw-apps loop was incorrectly mirroring.

No AGENTS.md violations, no new public surfaces, no security concerns.

Pi Review

Good to merge

Test coverage

CLI (TypeScript) — the codebase has no test infrastructure for the CLI sync module (cli/src/commands/sync/sync.ts), so no automated tests are expected. No new tests are needed for this one-character bug fix.

Manual verification — the PR body already describes the manual test plan: in a workspace with a raw app, edit a backend runnable on the remote, run wmill sync pull, and confirm the raw app's hash in wmill-lock.yaml updates. Then run wmill sync push --dry-run and confirm no staleness.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 42de9b6
Status: ✅  Deploy successful!
Preview URL: https://aae9bd8b.windmill.pages.dev
Branch Preview URL: https://sync-pull-wmill-locks-raw-ap.windmill.pages.dev

View logs

@github-actions
Copy link
Copy Markdown
Contributor

Codex Review

Good to merge

No issues found. Checked for bugs, security, AGENTS.md compliance, public surfaces, and test coverage.

Test coverage

The diff does not add automated coverage for this raw-app pull lockfile path; that is not a merge blocker for the one-line fix, but a regression case in the existing CLI raw-app sync tests would be useful.

Manual verification still needed: run wmill sync pull --yes against a workspace with a changed raw app and confirm the raw-app files are written locally and wmill-lock.yaml updates during the pull, without requiring a later metadata regeneration command.

@rubenfiszel rubenfiszel merged commit 486e5f9 into main May 22, 2026
33 checks passed
@rubenfiszel rubenfiszel deleted the sync-pull-wmill-locks-raw-apps branch May 22, 2026 11:56
@github-actions github-actions Bot locked and limited conversation to collaborators May 22, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants