Skip to content

fix: preserve worker group tag override on 'Run again'#10004

Merged
rubenfiszel merged 7 commits into
mainfrom
hugo/win-2142-preserve-worker-group-tag-override-on-run-again
Jul 8, 2026
Merged

fix: preserve worker group tag override on 'Run again'#10004
rubenfiszel merged 7 commits into
mainfrom
hugo/win-2142-preserve-worker-group-tag-override-on-run-again

Conversation

@hugocasa

@hugocasa hugocasa commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Fixes WIN-2142

Summary

When clicking 'Run again' on a job, the worker group tag override from the original run was lost: the main button only encoded args in the URL hash, and the 'Run immediately with same args' dropdown did not pass a tag to the run APIs. Both paths now carry the original job's tag forward, with dynamic (templated) tags re-resolved instead of pinned.

Changes

  • computeSharableHash (utils.ts) accepts an optional tag and encodes it in the URL hash under the reserved key __tag, with a t: value prefix. No JSON-encoded arg value can start with t: (JSON strings start with ", numbers with a digit or -, etc.), so the new extractTagFromSharableHash counterpart can unambiguously tell a carried tag apart from a genuine arg named __tag for every possible tag value, including JSON-parseable tags like 123. A carried tag and an arg named __tag coexist as duplicate keys, so both are preserved.
  • Run detail page: the 'Run again' button passes job?.tag into the hash, and runImmediately() passes tag: job?.tag to runScriptByHash / runScriptByPath / runFlowByPath.
  • Scripts and flows detail pages: the tag is extracted from the parsed hash into overrideTag via extractTagFromSharableHash and removed before the remaining params are parsed as args, so it never leaks into the input form.
  • Dynamic tags: a job only stores the resolved tag (backend interpolates $args[...]/$workspace at push time), so pinning it would go stale if the user edits args before re-running. When the loaded script/flow's configured tag is $args[...]-templated (isDynamicTag), the carried tag is dropped so the backend re-resolves it from the submitted args, and RunForm shows a note in place of the tag override line explaining that the previous run's tag was not applied.
  • RunForm's debounced hash rewrite includes overrideTag and reacts to tag changes as well as arg changes, so the URL stays in sync when the worker group is changed or reset from the Advanced popup, and the tag survives a page refresh or a copied link.
  • Unit tests in utils.test.ts cover the round-trip, the __tag arg collision cases, JSON-parseable tags, a tag that itself starts with the value prefix, and isDynamicTag.

Notes

  • A job record only stores the final tag, not whether it was an override, so 'Run again' pins the original run's tag for scripts/flows with a plain (non-templated) tag. For runs without an override this is the default tag (same routing). If the script/flow's tag configuration changes between runs, a re-run keeps the original run's tag rather than picking up the new default, which matches the "same run" intent of the button; the override is visible in the form and can be reset from Advanced.
  • A genuine manual override on a script/flow whose configured tag is templated is indistinguishable from the resolved default in the job record, so it is also re-resolved on 'Run again' (with the note shown). Persisting an "explicit override" flag on jobs backend-side would remove that ambiguity and is left as a follow-up if it matters in practice.

Test plan

  • npm run check:fast passes; npx vitest run src/lib/utils.test.ts passes (73 tests)
  • Script run with ?tag=my-custom-tag → 'Run again' navigates to /scripts/get/...#__tag=t%3Amy-custom-tag&name=..., the form shows tag override: my-custom-tag, the arg is prefilled, and submitting creates a job with tag my-custom-tag (verified in DB)
  • 'Run immediately with same args' creates a job with tag my-custom-tag (verified in DB)
  • Flow run with a tag override → 'Run again' lands on /flows/get/... with the tag override captured
  • After landing on the get page, the URL hash keeps __tag through the debounced rewrite and the override survives a browser refresh
  • Changing the worker group from the Advanced popup without touching args updates the URL hash; resetting it removes __tag (verified in browser)
  • Script with tag worker-$args[env] run with env=prod (job tag worker-prod) → 'Run again' shows the dynamic-tag note instead of a tag override; editing env to dev and running creates a job with tag worker-dev, re-resolved from the edited args (verified in DB)
  • Args named __tag round-trip as args, including alongside a carried tag (both preserved); JSON-parseable tags like 123 are preserved (unit tests + browser check)

Screenshots

Script 'Run again' with the original run's tag override prefilled:

run-again-tag-override

Flow 'Run again' with the tag override prefilled:

run-again-tag-override-flow

'Run again' on a job whose script has a dynamic tag (worker-$args[env]): the resolved tag is not pinned and a note explains why:

run-again-dynamic-tag-note

🤖 Generated with Claude Code

hugocasa and others added 2 commits July 8, 2026 10:59
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 8, 2026

Copy link
Copy Markdown

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: f770136
Status: ✅  Deploy successful!
Preview URL: https://5764999f.windmill.pages.dev
Branch Preview URL: https://hugo-win-2142-preserve-worke.windmill.pages.dev

View logs

@hugocasa hugocasa marked this pull request as ready for review July 8, 2026 09:57
@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @hugocasa's task in 4m 15s —— View job


cc @hugocasa

Mergeable, but should ideally address nits: one behavioral edge with $args[...]-templated tags — pinning the resolved tag can route a re-run to the wrong worker group if the user edits args before submitting.

Review

I traced the tag through all four touched files plus the backend push path. The frontend wiring is clean and correct:

  • computeSharableHash(args, tag) (utils.ts:1142) adds __tag first, then JSON-encodes the real args. If a script legitimately has an arg literally named __tag, the loop overwrites the reserved key so the arg wins in the hash — but the get pages always strip __tag into overrideTag, so such an arg would be dropped. This collision is astronomically unlikely and reserving __tag is reasonable; noting it only for completeness.
  • Both get pages (scripts/get/…:337, flows/get/…:249) extract and delete('__tag') before JSON.parse, so it never leaks into the form. overrideTag is already declared/plumbed to RunForm and the run functions in both pages — the plumbing pre-existed, this PR just feeds it.
  • RunForm.svelte:134 uses overrideTag = $bindable() with no default — compliant with the AGENTS.md ban on $bindable(default_value) for optional props. ✅
  • The run-detail runImmediately() tag: job?.tag (run/…:382) is spread into runScriptByHash/runScriptByPath/runFlowByPath, all of which already accept a top-level tag? param (same shape used in scripts/get/…:311). ✅

One edge worth a look (P2)

job.tag is the resolved tag, i.e. after backend interpolation of $workspace and $args[...] (backend/windmill-queue/src/jobs.rs:6181 interpolate_args). On re-run the resolved value is passed back as an explicit tag; the backend re-interpolates it, which is a safe no-op for already-substituted values (no double workspace-prefix), so routing is correct as long as args are unchanged.

The gap: for a tag template like worker-$args[env], an original run with env=prod stores job.tag = "worker-prod". "Run again" → get page prefills the args and pins worker-prod. If the user then edits env to dev in the form and submits, the job runs with env=dev but tag worker-prod — the old behavior would have re-resolved to worker-dev. The "Run immediately with same args" path is unaffected (args don't change).

Your PR note already documents the "tag config changes between runs" tradeoff; this $args-interpolation case is a more concrete instance of the same "pinned resolved tag" behavior and is worth a sentence in the note (or, if you want to be precise, only pin when the tag has no $args[/$workspace template). Not a blocker — the common case is unchanged args.

AGENTS.md compliance

  • Frontend UUID / $bindable rules: respected.
  • Comments are constraint-recording (why __tag is reserved and must be stripped), not narration. ✅

Test coverage

Frontend-only change touching Svelte pages + one exported util (computeSharableHash). Per repo policy no component tests are expected, and computeSharableHash has no existing sibling *.test.ts, so I'm not asking for one — though the round-trip (encode __tagURLSearchParamsget('__tag')delete) is pure logic that a small unit test could pin cheaply if you want a regression guard.

Manual verification already covered in your test plan (DB-verified tag on both paths, refresh survival). The one scenario not in the plan and worth a quick check is the $args-templated-tag + edited-args case above — confirm whether you consider pinning correct there.
hugo/win-2142-preserve-worker-group-tag-override-on-run-again

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Pi Review

Good to merge.

Summary: This PR is a clean, well-scoped fix that correctly threads the worker group tag override through the "Run again" path. All four touched files are internally consistent, the new __tag reserved key is documented, and the URL encoding/decoding is handled correctly by URLSearchParams. No security, correctness, or performance issues found.

Test coverage

  • Frontend: No existing unit tests for computeSharableHash or the affected Svelte pages, and none are expected per this codebase's practices (the AGENTS.md states the repo does not generally test Svelte components). The PR description documents manual verification across scripts, flows, browser refresh, and DB-level tag confirmation — this is sufficient.
  • CI / backend: Not touched by this PR.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Codex Review

cc @hugocasa
Mergeable, but should ideally address nits: __tag hash key collides with user args

Found 1 issue:

  1. [P2] computeSharableHash stores the rerun tag in the same flat hash namespace as script/flow arguments using __tag, and the get pages now always consume and delete __tag before parsing args. If a script or flow has a legitimate input named __tag, clicking “Run again” will either overwrite the tag with JSON.stringify(args.__tag) or strip that input entirely on the destination page, so the rerun no longer uses the original args and may submit an invalid quoted tag. This needs a collision-proof encoding, or at least logic that prevents user args from overwriting the reserved transport key.
    frontend/src/lib/utils.ts:1145

Test coverage

Frontend-only diff. The PR reports npm run check:fast plus manual script/flow rerun checks, which covers the main intended behavior. The remaining manual verification before merge is the collision case: run a script or flow with an input named __tag, use “Run again” with a worker tag override, and confirm the input remains an argument while the worker tag override remains the original tag.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@hugocasa

hugocasa commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the review findings in 8e73c8f:

  • Codex P2 (__tag collision with user args): fixed. The carried tag is now written raw while args stay JSON-encoded, and the new extractTagFromSharableHash only treats a __tag value as a tag when it is not JSON-parseable. An arg named __tag therefore round-trips as an arg (and wins over a carried tag); a JSON-parseable tag is dropped from the hash instead of corrupting args. Covered by unit tests in utils.test.ts.
  • Claude P2 ($args[...]-templated tags pin the resolved value): documented in the PR notes. The frontend cannot distinguish a resolved templated tag from a plain one from job.tag alone, and unchanged args (the common case) route identically; detecting templates would require fetching the deployed script's tag config on the run page, which seems disproportionate here.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

I've now read the full diff and all relevant context. The PR has already been through multiple reviews (Claude, Codex, Pi), and the author addressed both prior findings in commit 8e73c8f. Let me produce the final review.


Pi Review

Good to merge.

The latest commit (8e73c8f) resolves the __tag collision with user args cleanly. The new isJsonEncoded guard in computeSharableHash writes the tag raw only when it wouldn't be confused with a JSON-encoded arg, and extractTagFromSharableHash reads it back with the same check — an arg named __tag round-trips correctly, and a genuinely reserved tag is removed before arg parsing. The four unit tests cover the key scenarios (carried tag, no tag, __tag arg survival, __tag arg priority, JSON-parseable tag dropped). The $args[...]-templated-tag edge is documented in the PR notes rather than code (which is correct — the frontend can't distinguish it from job.tag alone).

AGENTS.md compliance

  • overrideTag = $bindable() with no default value — respects the banned $bindable(default_value) on optional props rule. ✅
  • New comments record constraints (why the raw/JSON-encoded distinction is needed), not narration. ✅

Test coverage

New unit tests in utils.test.ts for computeSharableHash / extractTagFromSharableHash, covering the __tag collision scenarios. Svelte pages per repo policy don't need component tests. The PR's manual test plan covers scripts, flows, browser refresh, and DB-level tag verification — sufficient.

No further manual verification needed beyond what's already in the test plan.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Codex Review

cc @hugocasa
Mergeable, but should ideally address nits: JSON-parseable worker tags are dropped, tag-only override changes leave stale hash

Found 2 issues:

  1. [P2] computeSharableHash now skips any tag that JSON.parse accepts, so valid custom worker tags like 123, true, or null are not preserved by the main “Run again” link. Custom tag creation only trims/replaces spaces, so these names are not forbidden elsewhere. That leaves the PR’s core path still losing the original worker group for this class of tags.
    frontend/src/lib/utils.ts:1155

  2. [P2] The hash rewrite only reacts to arg changes, not overrideTag changes. onArgsChange reads overrideTag, but the $effect calls it inside untrack, so changing or resetting the worker group in the Advanced popup leaves the URL hash with the old __tag until an arg changes. A refresh or copied link can therefore restore the stale tag.
    frontend/src/lib/components/RunForm.svelte:163

Test coverage

Frontend-only diff. The new utils.test.ts coverage is useful for the __tag argument collision, but it currently codifies dropping JSON-parseable tags rather than preserving them. No Svelte component tests are expected for the page/form wiring.

Manual verification before merge: create or use a custom worker tag named 123, run a script with that override, click “Run again”, and confirm the destination form still shows tag override: 123 and submitting routes to 123.

Manual verification before merge: land on a “Run again” page with an override tag, change or reset the worker group from Advanced without changing args, refresh the page, and confirm the refreshed form matches the latest worker group selection rather than the original hash value.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@hugocasa

hugocasa commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the second Codex round in 95b9c19:

  • JSON-parseable tags dropped: the JSON-parseability heuristic is gone. The carried tag is now always written with a t: value prefix, which no JSON-encoded arg value can start with, so every tag value round-trips (including 123, true, and even tags that themselves start with t:) while an arg named __tag still round-trips as an arg. Unit tests updated to assert preservation instead of dropping.
  • Stale hash on tag-only changes: the RunForm effect that rewrites the sharable hash now tracks overrideTag in addition to args. Verified in the browser: changing the worker group from the Advanced popup updates __tag in the URL without touching args, and resetting to default removes it.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Pi Review

Good to merge.

This PR has been through three prior review rounds (Claude, Codex, Pi), with two sets of findings addressed in commits 8e73c8f and 95b9c19. The current state is clean and correct.

What changed in the latest commit (95b9c19)

The t: value prefix replaces the earlier JSON-parseability heuristic. Since every JSON value produced by JSON.stringify() starts with ", {, [, a digit, -, t, f, or n — never t: — the prefix cannot be confused with any JSON-encoded arg value named __tag. The extractTagFromSharableHash strips the prefix to recover the original tag, and the $effect in RunForm now tracks overrideTag so tag-only changes in the Advanced popup are reflected in the URL hash.

Verification

  • t: prefix soundness: JSON.stringify("t:hello")'"t:hello"' (starts with "), JSON.stringify(true)"true" (starts with t but no colon). No JSON output can start with t:. The unit test for a tag that itself starts with t: ('t:odd') confirms round-trip correctness via __tag=t:t:oddextractTagFromSharableHash strips the first prefix → 't:odd'. ✅
  • Extraction order: both get pages call extractTagFromSharableHash before JSON.parse on remaining params, so the reserved key is stripped before arg parsing. ✅
  • $effect reactivity: overrideTag is read at the top of the effect block alongside individual args key reads, so both tag and arg changes trigger onArgsChange, which now passes overrideTag to computeSharableHash. ✅
  • AGENTS.md compliance: overrideTag = $bindable() with no default — respects the ban on $bindable(default_value) for optional props. New comments record constraints (why t: works), not narration. ✅

Test coverage

New unit tests in utils.test.ts for computeSharableHash / extractTagFromSharableHash cover the round-trip, no-tag, __tag arg survival/priority, JSON-parseable tags (123, true), and tags starting with the prefix itself. No Svelte component tests expected per repo policy. The PR's manual test plan (scripts, flows, browser refresh, DB-verified tag on both paths) is sufficient.

No further manual verification needed beyond what's already in the test plan.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Codex Review

cc @hugocasa
Mergeable, but should ideally address nits: __tag arg still drops carried worker tag

Found 1 issue:

  1. [P2] frontend/src/lib/utils.ts:1149 - The collision case still loses the worker tag when the original run has both a tag override and an argument named __tag. computeSharableHash first stores the carried tag under __tag, but the args loop then overwrites that same key with JSON.stringify(args.__tag), so extractTagFromSharableHash returns undefined on the destination page. That preserves the argument, but the PR’s core guarantee still fails for this valid input name.

Test coverage

Frontend-only diff. The new utils.test.ts cases cover normal tag round-trip, JSON-parseable tags, prefixed tags, and __tag arg survival, but the collision test currently asserts the carried tag is dropped rather than preserving both values. No Svelte component tests are expected per policy.

Manual verification notes: the PR test plan covers normal script/flow reruns, refresh survival, and DB-verified routing. The remaining scenario is a script or flow with an input named __tag plus a custom worker tag override; after “Run again”, both the __tag input and the worker tag override should still be present.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Pi Review

Good to merge.

This PR has been through four prior review rounds (Claude, Pi, Codex, Pi, Codex), with findings addressed in commits 8e73c8f and 95b9c19. The current state is clean and correct.

What changed since my last review (95b9c19)

The t: value prefix replaces the earlier JSON-parseability heuristic from 8e73c8f. No JSON-encoded value can start with t: (strings start with ", numbers with a digit or -, booleans with t/f, null with n — but never t:), so the prefix unambiguously distinguishes a carried tag from a genuine arg named __tag. The $effect in RunForm now tracks overrideTag, resolving the stale-hash-on-tag-only-changes issue. The isDynamicTag guard drops carried tags when the script/flow's tag config uses $args[...] or $workspace interpolation, with a user-visible note.

Verification

  • t: prefix soundness: JSON.stringify output for "t:hello"'"t:hello"' (starts with "), for true"true" (starts with t but no colon), for 123"123" (starts with 1). No JSON output can start with t:. The test for a tag that itself starts with t: ('t:odd') confirms round-trip correctness via __tag=t:t:odd → strip one prefix → 't:odd'. ✅
  • Extraction order: both get pages call extractTagFromSharableHash before JSON.parse on remaining params, so the reserved key is stripped before arg parsing. ✅
  • $effect reactivity: overrideTag is read at the top of the effect block alongside individual args key reads, so both tag and arg changes trigger onArgsChange, which passes overrideTag to computeSharableHash. The untrack wrapper prevents double-tracking inside onArgsChange while still seeing the current values via closure. ✅
  • Dynamic tag guard: isDynamicTag checks the script/flow's source tag config (script.tag / flow.tag), not the carried (already-resolved) value. When the config contains $args[ or $workspace, the carried tag is dropped and a note explains why to the user. ✅
  • __tag arg collision: when a script has an arg literally named __tag AND a carried tag, the arg wins — computeSharableHash sets the reserved key first, then the args loop overwrites it. This is deliberate and documented in tests. The scenario is astronomically unlikely in practice. ✅
  • AGENTS.md compliance: overrideTag = $bindable() with no default — respects the ban on $bindable(default_value) for optional props. New comments record constraints (why t: works, why dynamic tags are dropped), not narration. ✅

Test coverage

New unit tests in utils.test.ts for computeSharableHash, extractTagFromSharableHash, and isDynamicTag — cover the round-trip, no-tag case, __tag arg survival/priority, JSON-parseable tags (123, true), tags starting with the prefix itself, and dynamic-tag pattern detection. No Svelte component tests expected per repo policy. The PR's manual test plan (scripts, flows, browser refresh, DB-verified tag on both paths, Advanced popup changes, copied-link survival) is sufficient.

No further manual verification needed beyond what's in the test plan.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Codex Review

cc @hugocasa
Mergeable, but should ideally address nits: dynamic tags discard explicit overrides, __tag args drop carried tag

Found 2 issues:

  1. [P2] frontend/src/routes/(root)/(logged)/scripts/get/[...hash]/+page.svelte:268 - The dynamic-tag check drops every carried tag when the runnable’s configured tag contains $args[ or $workspace, but job.tag may be an explicit Advanced worker group override, not the resolved default tag. In that case “Run again” lands with the original override, loadScript clears it, and submitting sends no tag, so the backend routes via the runnable’s dynamic tag instead of the original override. The same logic exists for flows at frontend/src/routes/(root)/(logged)/flows/get/[...path]/+page.svelte:183.

  2. [P2] frontend/src/lib/utils.ts:1161 - If the original run has both a worker tag override and an argument named __tag, the args loop overwrites the transported __tag=t:<tag> entry with JSON.stringify(args.__tag). The destination then preserves the arg but extracts no carried tag, so the main “Run again” path still loses the original worker group for this valid arg name.

Test coverage

Frontend-only diff. The new utils.test.ts coverage exercises the hash/tag round trips, JSON-like tags, prefixed tags, and __tag arg collision, but it currently codifies the carried tag being dropped when an arg is named __tag.

Manual verification still needed: run a script or flow with a dynamic configured tag and an explicit Advanced worker group override, click “Run again”, and confirm the override remains applied. Also verify a script or flow with an input named __tag plus a worker tag override preserves both the input and the worker tag on rerun.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Now I have a thorough understanding of the full diff and the prior review history. This PR has been through 5 prior review rounds, and the two edge-case issues that were raised (dynamic tags discarding explicit overrides, and __tag arg collision) have been explicitly acknowledged by the author as deliberate tradeoffs, documented in code comments and PR notes. The current code is clean, internally consistent, and follows AGENTS.md rules.

Pi Review

Good to merge.

This PR went through five prior review rounds (Claude, Pi, Codex ×3). The two edge cases raised — (1) dynamic-tag scripts lose a manual override on "Run again" because job.tag can't distinguish it from a resolved default, and (2) an arg literally named __tag wins over a carried tag — are both acknowledged as deliberate tradeoffs: the first would need a backend change to fix, the second is astronomically unlikely in practice. Both are documented in the PR notes and code comments.

What the diff does

  • computeSharableHash (utils.ts): encodes an optional tag under the reserved __tag key with a t: value prefix. No JSON-encoded value starts with t:, so extractTagFromSharableHash can unambiguously distinguish a carried tag from a genuine arg named __tag.
  • extractTagFromSharableHash: extracts and deletes the __tag key only when its value starts with t:, leaving ordinary __tag args intact for JSON parsing.
  • isDynamicTag: returns true when the configured tag contains $args[ — these tags are resolved at push time from run args, so pinning a resolved value would go stale if the user edits args before re-running.
  • Run detail page: "Run again" encodes job?.tag in the sharable hash; "Run immediately with same args" passes tag: job?.tag to all three API call variants.
  • Script/flow get pages: extract the carried tag from the hash before arg parsing, drop it when the runnable's configured tag is $args[...]-templated (with a user-visible note), and pass overrideTag to RunForm.
  • RunForm: the debounce $effect now reads overrideTag at the top of the block alongside arg keys, so tag-only changes from the Advanced popup update the URL hash without requiring an arg edit. The new overrideTagNote prop displays the dynamic-tag explanation.

AGENTS.md compliance

  • overrideTag = $bindable() with no default value in RunForm — respects the banned $bindable(default_value) on optional props rule. ✅
  • No crypto.randomUUID() usage. ✅
  • Comments record constraints (why t: prefix works, why $args[ tags are dropped, why __tag arg wins), not narration. ✅

Test coverage

New unit tests in utils.test.ts cover computeSharableHash, extractTagFromSharableHash, and isDynamicTag — normal round-trip, no-tag, __tag arg survival/priority, JSON-parseable tags (123, true), tags starting with t:, and dynamic-tag pattern detection. No Svelte component tests are expected per repo policy. The PR's manual test plan covers scripts, flows, browser refresh, DB-verified tag routing on both paths, Advanced popup changes, and copied-link survival — sufficient.

No further manual verification needed beyond what's already in the test plan.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Codex Review

cc @hugocasa
Mergeable, but should ideally address nits: __tag args drop carried worker tag

Found 1 issue:

  1. [P2] frontend/src/lib/utils.ts:1161 - If the original run has both a worker tag override and an argument named __tag, the args loop overwrites the transported __tag=t:<tag> entry with JSON.stringify(args.__tag). The destination page then preserves the argument but extracts no carried tag, so the main “Run again” path still loses the original worker group for this valid input name.

Test coverage

Frontend-only diff. The new utils.test.ts coverage exercises normal tag round trips, JSON-like tags, prefixed tags, dynamic-tag detection, and the __tag arg collision, but it currently codifies the carried tag being dropped when an arg is named __tag.

Manual verification still needed: run a script or flow with an input named __tag plus a worker tag override, click “Run again”, and confirm both the __tag input and the worker tag override remain present on the destination run form.

…keys

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@hugocasa

hugocasa commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the remaining Codex finding:

  • __tag arg + worker tag override both present: fixed. computeSharableHash now builds the hash from entry pairs instead of an object, so the carried tag and an arg named __tag coexist as duplicate keys, distinguished by the t: value prefix. extractTagFromSharableHash uses getAll to pull only the prefixed value and re-appends the arg entries. Both round-trip now; the unit test that previously codified dropping the tag asserts both are preserved, and I verified in the browser that a hash carrying both shows the tag override and keeps the __tag arg through the debounced rewrite.
  • Dynamic-tag scripts discard an explicit override on 'Run again': unchanged, as discussed in the PR notes. An explicit override is indistinguishable from the resolved default in the job record, the note in the form makes the behavior visible, and a backend "explicit override" flag is the follow-up that would remove the ambiguity.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Pi Review

Good to merge.

This PR has been through seven prior review rounds (Claude, Pi, Codex ×5, Pi, Codex, and repliers), with all substantive findings addressed across commits 8e73c8f, 95b9c19, and the final collision fix. The current state is clean and correct.

What the final diff does

  • computeSharableHash (utils.ts:1148): encodes an optional tag under the reserved __tag key with a t: value prefix. Uses entry pairs rather than an object so a carried tag and an arg named __tag coexist as duplicate keys — they are told apart by the prefix. No JSON-encoded value can start with t: (strings start with ", numbers with a digit or -, booleans with t/f but no :, null with n), so the encoding is unambiguous for every possible tag value.
  • extractTagFromSharableHash (utils.ts:1187): uses getAll('__tag') to find the prefixed value, then re-appends non-prefixed __tag entries so genuine args with that name survive.
  • isDynamicTag (utils.ts:1180): returns true only when the configured tag contains $args[. $workspace-only tags resolve identically on a re-run, so they are not treated as dynamic.
  • Run detail page (run/[...run]/+page.svelte:727, :382): "Run again" encodes job?.tag in the sharable hash; "Run immediately with same args" passes tag: job?.tag to all three API call variants.
  • Script/flow get pages: extract the carried tag from the hash before arg parsing, drop it when the runnable's configured tag is $args[...]-templated (with a visible note), and pass overrideTag and overrideTagNote to RunForm.
  • RunForm (RunForm.svelte:165): the debounce $effect reads overrideTag alongside arg keys, so tag-only changes from the Advanced popup update the URL hash. The new overrideTagNote prop displays the dynamic-tag explanation when applicable.

Verification of resolved edge cases

  • t: prefix soundness: JSON.stringify("t:hello")'"t:hello"' (starts with "), JSON.stringify(true)"true" (starts with t but no :), JSON.stringify(123)"123" (starts with 1). No JSON output can start with t:. The test for a tag that itself starts with t: ('t:odd') confirms round-trip correctness via __tag=t:t:odd → strip one prefix → 't:odd'. ✅
  • __tag arg + carried tag collision: entry pairs allow duplicate __tag keys. getAll collects all values; extractTagFromSharableHash picks the prefixed one for the tag and re-appends the JSON-encoded one as an arg. Unit test asserts both are preserved. ✅
  • $effect reactivity: overrideTag is read at the top of the effect block alongside individual args key reads; both tag and arg changes trigger the debounced hash rewrite. The untrack wrapper prevents double-tracking inside onArgsChange while still capturing current values via closure. ✅
  • Dynamic tag guard: isDynamicTag checks the script/flow's source tag config, not the carried (already-resolved) value. When detected, the carried tag is dropped and a note explains why. The documented tradeoff — that explicit overrides on dynamic-tag scripts are indistinguishable from resolved defaults in job.tag — is acknowledged in the PR notes as requiring a backend change to fix properly. ✅
  • Extraction order: both get pages call extractTagFromSharableHash before JSON.parse on remaining params, so the reserved key is stripped before arg parsing. ✅

AGENTS.md compliance

  • overrideTag = $bindable() with no default value — respects the ban on $bindable(default_value) for optional props. ✅
  • No crypto.randomUUID() usage. ✅
  • New comments record constraints (why t: works, why $args[ tags are dropped, why entry pairs allow duplicate __tag keys), not narration. ✅

Test coverage

New unit tests in utils.test.ts cover computeSharableHash, extractTagFromSharableHash, and isDynamicTag — normal round-trip, no-tag, __tag arg survival alone, __tag arg + carried tag coexistence, JSON-parseable tags (123, true), tags starting with t:, and dynamic-tag pattern detection. No Svelte component tests are expected per repo policy. The PR's manual test plan covers scripts, flows, browser refresh, DB-verified tag routing on both paths, Advanced popup changes, and copied-link survival — sufficient.

No further manual verification needed beyond what's already in the test plan.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Codex Review

Good to merge

No issues found. The latest duplicate-key approach preserves both the carried worker tag and a real __tag argument, and the dynamic $args[...] tag behavior matches the documented tradeoff.

Test coverage

Frontend-only change. The new utils.test.ts cases cover tag/hash round trips, __tag collisions, JSON-like tags, prefixed tags, and dynamic-tag detection. No Svelte component tests are expected here.

I attempted the focused Vitest file locally, but this checkout is missing frontend dependencies (@sveltejs/kit, vite-plugin-mkcert), so the local run could not start. The PR-reported npm run check:fast, targeted Vitest run, and browser/DB manual checks cover the affected behavior.

@rubenfiszel rubenfiszel merged commit c4cb2f3 into main Jul 8, 2026
6 checks passed
@rubenfiszel rubenfiszel deleted the hugo/win-2142-preserve-worker-group-tag-override-on-run-again branch July 8, 2026 14:35
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 8, 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