feat(settings): workflows admin — dedicated Running section above paginated History [shot:workflow-running-section] (bookshelf-33oo) #1205

Merged

zombor merged 11 commits from bd-bookshelf-33oo into main 2026-07-23 19:47:11 +00:00

Conversation 13 Commits 11 Files changed 16 +1170 -24

zombor commented 2026-07-23 00:28:30 +00:00

Owner

Copy link

Summary

Reworks the existing Workflows admin tab to replace the page-local RUNNING-first sort with a dedicated two-section layout:

• Running section: surfaces ALL active workflow instances (completed_at IS NULL on latest execution per instance_id), up to a cap of 500, using a MySQL window function query. Shows count badge and cap notice.
• History section: existing paginated (newest-first) list of all workflow instances.

The page-local sort only reordered within the current 50-row page, leaving running workflows hidden past page 1.

Changes

• wfengine: ListRunningInstances + makeRunningInstancesQuery using ROW_NUMBER() OVER (PARTITION BY instance_id ORDER BY created_at DESC, id DESC)
• appwire: ListRunningWorkflowInstances in WFTriggerDeps
• app: wiring adapter engine to appwire.WorkflowRow
• settings: split loadWorkflowsTabData into loadRunningWorkflows + loadWorkflowHistory; cap+1 overflow
• templates/pages/settings_shell.html: two-section layout
• Tests: full coverage for new paths in wfengine and settings
• Browser e2e: screenshot step in Journey 13

Test plan

• make test passes
• CI covers unit + integration + e2e
• Journey 13 asserts both sections render
• Screenshot via [shot:workflow-running-section] CI marker

Closes bead bookshelf-33oo on merge.

## Summary Reworks the existing Workflows admin tab to replace the page-local RUNNING-first sort with a dedicated two-section layout: - **Running section**: surfaces ALL active workflow instances (completed_at IS NULL on latest execution per instance_id), up to a cap of 500, using a MySQL window function query. Shows count badge and cap notice. - **History section**: existing paginated (newest-first) list of all workflow instances. The page-local sort only reordered within the current 50-row page, leaving running workflows hidden past page 1. ## Changes - wfengine: ListRunningInstances + makeRunningInstancesQuery using ROW_NUMBER() OVER (PARTITION BY instance_id ORDER BY created_at DESC, id DESC) - appwire: ListRunningWorkflowInstances in WFTriggerDeps - app: wiring adapter engine to appwire.WorkflowRow - settings: split loadWorkflowsTabData into loadRunningWorkflows + loadWorkflowHistory; cap+1 overflow - templates/pages/settings_shell.html: two-section layout - Tests: full coverage for new paths in wfengine and settings - Browser e2e: screenshot step in Journey 13 ## Test plan - [x] make test passes - [x] CI covers unit + integration + e2e - [x] Journey 13 asserts both sections render - [x] Screenshot via [shot:workflow-running-section] CI marker Closes bead bookshelf-33oo on merge.

workflows-running-section.png

63 KiB

workflows-running-section.png

62 KiB

workflows-running-section.png

63 KiB

workflows-after.png

77 KiB

workflows-running-section.png

59 KiB

workflows-running-section.png

58 KiB

workflows-running-section.png

59 KiB

workflows-running-section.png

60 KiB

zombor added 1 commit 2026-07-23 00:28:30 +00:00

feat(settings): sort workflow admin list RUNNING-first via page-local sort (bookshelf-33oo) ... e51aef207b

Apply a stable total-order sort (status rank → CreatedAt DESC → InstanceID ASC)
to the already-fetched page of workflow instances in loadWorkflowsTabData so that
RUNNING instances always appear at the top of the Settings → Workflows tab.

No changes to the workflow DB, its schema, or its queries. The sort is page-local:
go-workflows' own GetWorkflowInstances pagination is preserved; we only reorder the
returned slice before rendering. Running workflows cluster on page 1 in practice
since they are the newest rows.

The tiebreaker (InstanceID ASC) ensures the comparator is a total order so
concurrent instances with identical timestamps sort deterministically — no flaky
ordering risk (per review-standard flake prevention guidance).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor commented 2026-07-23 00:40:34 +00:00

Author

Owner

Copy link

Security Review — PR #1205 (RUNNING-first page-local sort)

Scope: verify no new data exposure, no DB/query change, admin gating intact, no secrets/PII/engine coupling.

Security surface: clean.

• No new data exposure. SortWorkflowInstances reorders the already-fetched, already-truncated page slice in-memory. Page size is unchanged (shellWorkflowsPageSize), no new fields are read (WorkflowInstanceRow = InstanceID/ExecutionID/ParentID/CreatedAt/CompletedAt/State/Queue — all identifiers/timestamps, no secrets/PII), and the workflows view is a global admin-only surface (no per-user scoping to leak).
• Admin gating intact & untouched. adminOnlyTabs["workflows"] = true (shell_handler.go:148) and resolveShellTab returns forbidden for non-admins (shell_handler.go:311). This diff does not touch routing or gating.
• No DB/query change. Purely an in-memory sort.SliceStable; no new query, no injection surface. Comparator is a total order (status rank → CreatedAt desc → InstanceID tiebreaker) — no flaky non-total sort.
• No new logging; no go-workflows engine coupling. The new file imports only sort. internal/settings is a domain-boundary-safe location.

Findings

[MAJOR] internal/settings/shell_handler.go:613 — next-page cursor derived AFTER the page is re-sorted
SortWorkflowInstances(instances) runs before nextID/nextExec are read from instances[len-1] (lines 614-619). The keyset pagination cursor from the go-workflows backend (ListInstances/GetWorkflowInstances) assumes the page is still in the backend's native newest-first (created_at, id) order — the correct "next" cursor is the element that was last in DB order. After the RUNNING-first reorder, instances[len-1] is generally a different row (e.g. the oldest completed instance, which can be NEWER than a long-running old instance pulled to the top). The next page then queries after a cursor that isn't the true page tail, causing instances to be skipped or duplicated across page boundaries whenever a RUNNING instance isn't already at the DB-order tail (only observable with >shellWorkflowsPageSize instances). This is a correctness regression, not a security issue, but it is introduced by this diff. Fix: derive nextID/nextExec from the DB-ordered slice before calling SortWorkflowInstances, or capture the cursor row prior to the sort and sort a copy for display.

REVIEW VERDICT: 0 blocker, 1 major, 0 minor

## Security Review — PR #1205 (RUNNING-first page-local sort) Scope: verify no new data exposure, no DB/query change, admin gating intact, no secrets/PII/engine coupling. **Security surface: clean.** - **No new data exposure.** `SortWorkflowInstances` reorders the already-fetched, already-truncated page slice in-memory. Page size is unchanged (`shellWorkflowsPageSize`), no new fields are read (`WorkflowInstanceRow` = InstanceID/ExecutionID/ParentID/CreatedAt/CompletedAt/State/Queue — all identifiers/timestamps, no secrets/PII), and the workflows view is a global admin-only surface (no per-user scoping to leak). - **Admin gating intact & untouched.** `adminOnlyTabs["workflows"] = true` (shell_handler.go:148) and `resolveShellTab` returns forbidden for non-admins (shell_handler.go:311). This diff does not touch routing or gating. - **No DB/query change.** Purely an in-memory `sort.SliceStable`; no new query, no injection surface. Comparator is a total order (status rank → CreatedAt desc → InstanceID tiebreaker) — no flaky non-total sort. - **No new logging; no go-workflows engine coupling.** The new file imports only `sort`. `internal/settings` is a domain-boundary-safe location. ### Findings [MAJOR] internal/settings/shell_handler.go:613 — next-page cursor derived AFTER the page is re-sorted `SortWorkflowInstances(instances)` runs before `nextID/nextExec` are read from `instances[len-1]` (lines 614-619). The keyset pagination cursor from the go-workflows backend (`ListInstances`/`GetWorkflowInstances`) assumes the page is still in the backend's native newest-first `(created_at, id)` order — the correct "next" cursor is the element that was last in DB order. After the RUNNING-first reorder, `instances[len-1]` is generally a different row (e.g. the oldest completed instance, which can be NEWER than a long-running old instance pulled to the top). The next page then queries `after` a cursor that isn't the true page tail, causing instances to be skipped or duplicated across page boundaries whenever a RUNNING instance isn't already at the DB-order tail (only observable with >`shellWorkflowsPageSize` instances). This is a correctness regression, not a security issue, but it is introduced by this diff. Fix: derive `nextID`/`nextExec` from the DB-ordered slice before calling `SortWorkflowInstances`, or capture the cursor row prior to the sort and sort a copy for display. REVIEW VERDICT: 0 blocker, 1 major, 0 minor

zombor added 1 commit 2026-07-23 00:50:20 +00:00

fix(settings): derive workflows next-page cursor from DB order before display sort (bookshelf-33oo) ... 922b877068

SortWorkflowInstances reorders RUNNING instances to the front of the page.
If the DB-order tail of the page is a RUNNING instance it moves to position 0,
making instances[len-1] post-sort a different completed item and producing a
wrong keyset cursor that skips or duplicates rows across page boundaries.

Fix: capture nextID/nextExec from the DB-ordered tail (after truncation,
before sort) so the cursor always reflects the backend's native order.

Test: adds a regression spec where the DB-order tail is RUNNING; asserts
WorkflowsAfterID/AfterExec equal the running instance's IDs, not the
sorted tail (the test fails against the old code and passes after the fix).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor commented 2026-07-23 01:06:44 +00:00

Author

Owner

Copy link

Code Review — PR #1205 (backfilled by orchestrator; the review agent hit an auth error posting)

[MAJOR] internal/settings/shell_handler.go — next-page cursor derived from the SORTED page, not DB order. The keyset cursor (nextID/nextExec) was read from instances[len-1] after SortWorkflowInstances reordered the page (RUNNING-first), so the cursor no longer pointed at the backend's native (created_at,id) tail → instances could be skipped/duplicated across page boundaries (>1 page). RESOLVED in commit 922b8770: the cursor is now captured from the DB-ordered tail before the display sort, with a RED-before/GREEN-after regression test.

Positives verified: total-order comparator (status rank → CreatedAt DESC → InstanceID tiebreaker) — deterministic, no flaky ties; page-local sort only (no DB/query change); black-box tests; 100% coverage; no new exclusions.

REVIEW VERDICT: 0 blocker, 1 major (fixed in 922b8770), 0 minor

## Code Review — PR #1205 (backfilled by orchestrator; the review agent hit an auth error posting) **[MAJOR] internal/settings/shell_handler.go — next-page cursor derived from the SORTED page, not DB order.** The keyset cursor (`nextID/nextExec`) was read from `instances[len-1]` *after* `SortWorkflowInstances` reordered the page (RUNNING-first), so the cursor no longer pointed at the backend's native `(created_at,id)` tail → instances could be skipped/duplicated across page boundaries (>1 page). **RESOLVED in commit 922b8770**: the cursor is now captured from the DB-ordered tail *before* the display sort, with a RED-before/GREEN-after regression test. Positives verified: total-order comparator (status rank → CreatedAt DESC → InstanceID tiebreaker) — deterministic, no flaky ties; page-local sort only (no DB/query change); black-box tests; 100% coverage; no new exclusions. REVIEW VERDICT: 0 blocker, 1 major (fixed in 922b8770), 0 minor

zombor added 1 commit 2026-07-23 01:31:04 +00:00

feat(settings): dedicated Running section on workflows admin tab (bookshelf-33oo) ... a4d98e8c69

Replace page-local RUNNING-first sort (which only reordered within the current
50-row page) with a dedicated "Running" section that surfaces ALL active
workflows above the paginated History section.

- wfengine: add ListRunningInstances + makeRunningInstancesQuery using
  ROW_NUMBER() OVER (PARTITION BY instance_id ORDER BY created_at DESC, id DESC)
  to identify the latest execution per instance and return only those where
  completed_at IS NULL; LIMIT cap+1 for overflow detection
- appwire: add ListRunningWorkflowInstances func to WFTriggerDeps
- app: wire ListRunningWorkflowInstances adapter (engine → appwire.WorkflowRow)
- settings: split loadWorkflowsTabData into loadRunningWorkflows +
  loadWorkflowHistory; add WorkflowRunning/WorkflowRunningCapped to page data;
  add ListRunningWorkflowInstances to ShellDeps; wire in wire.go
- template: two sections — Running (all active, count badge, cap notice) and
  History (paginated, newest-first)
- tests: full coverage for ListRunningInstances stub + cap+1 overflow path +
  nil-guard + error propagation in both wfengine and settings packages

Closes bead bookshelf-33oo on merge.

zombor changed title from feat(settings): workflows admin list — RUNNING-first page-local sort (bookshelf-33oo) to feat(settings): workflows admin — dedicated Running section above paginated History [shot:workflow-running-section] (bookshelf-33oo) 2026-07-23 01:33:19 +00:00

zombor added 1 commit 2026-07-23 01:34:20 +00:00

test(e2e): add screenshot step for workflow Running section to Journey 13 (bookshelf-33oo) ... 8aa99f2c27

Adds uploadWorkflowRunningScreenshotToPR helper and a final It step in Journey 13
that navigates to the workflows tab, asserts both Running and History sections are
rendered, and posts a screenshot to the PR when SCREENSHOT_JOURNEY=workflow-running-section.
The PR title carries the [shot:workflow-running-section] marker so CI triggers it.

forgejo-actions commented 2026-07-23 01:44:25 +00:00

Copy link

Workflow Running section screenshot (workflows-running-section)

**Workflow Running section screenshot** (workflows-running-section) 

zombor added 1 commit 2026-07-23 01:51:03 +00:00

test(wfengine): integration test for ListRunningInstances scan loop (bookshelf-33oo) ... b75b814e01

Cover the rows.Next()/rows.Scan() path in makeRunningInstancesQuery (lines 172-200
in diag_accessor.go) via a real MySQL integration test. Inserts one running and
one completed instance, asserts ListRunningInstances returns only the running one
and maps the fields correctly.

forgejo-actions commented 2026-07-23 01:54:30 +00:00

Copy link

Workflow Running section screenshot (workflows-running-section)

**Workflow Running section screenshot** (workflows-running-section) 

zombor added 1 commit 2026-07-23 02:16:21 +00:00

test(wfengine): full unit coverage for makeRunningInstancesQuery via fake driver (bookshelf-33oo) ... cae63df9b4

Replace the previous interface-extraction approach (runningRowScanner +
fakeScanRows) with the project-standard fake driver.Connector pattern used by
internal/series. makeRunningInstancesQuery now takes an injected queryContext
func instead of *sql.DB, so unit tests can exercise the full closure body
(defer rows.Close() + scan loop + scan error branch) without a real MySQL
connection.

- diag_accessor.go: change makeRunningInstancesQuery to accept
  func(ctx, query, args...) (*sql.Rows, error); remove runningRowScanner
  interface; scanRunningInstanceRows now takes *sql.Rows directly.
- engine.go: pass db.QueryContext to makeRunningInstancesQuery.
- export_test.go: update MakeRunningInstancesQuery signature; remove
  ScanRunningInstanceRows (no longer needed).
- rows_helper_test.go: new file — fakeRunningConnector + makeRunningQueryFn
  helpers following the internal/series pattern.
- diag_accessor_test.go: replace fakeScanRows tests with fake-driver tests
  covering error path, happy path (two rows + parentID nil-guard), and scan
  error branch (incompatible created_at type → conversion error).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

forgejo-actions commented 2026-07-23 02:19:48 +00:00

Copy link

Workflow Running section screenshot (workflows-running-section)

**Workflow Running section screenshot** (workflows-running-section) 

zombor commented 2026-07-23 02:25:38 +00:00

Author

Owner

Copy link

Security Review — PR #1205 (bd-bookshelf-33oo)

Independent security review of the reworked PR adding ListRunningInstances (window-function query over the go-workflows instances table) surfaced in the admin Settings → Workflows tab.

1. SQL injection — CLEAN. runningInstancesSQL (internal/wfengine/diag_accessor.go:662) is a compile-time const string. The only bound value is the cap limit, passed as a positional ? parameter (queryContext(ctx, runningInstancesSQL, cap), line 688). No identifiers or values are string-concatenated, and cap is the compile-time constant shellWorkflowsRunningCap+1 (=501, shell_handler.go:162), never request-derived. No user input reaches the SQL.

2. Determinism — CLEAN. Outer ORDER BY created_at DESC, instance_id ASC: after the rn = 1 dedup, instance_id is unique per row → total order. Inner ROW_NUMBER() OVER (PARTITION BY instance_id ORDER BY created_at DESC, id DESC) carries the unique id DESC tiebreaker. No observable ties → no flake.

3. Bounded read — CLEAN. Query ends in LIMIT ?; cap fixed at 501. Handler trims to 500 and sets WorkflowRunningCapped (shell_handler.go:166-169). Cannot be coerced into an unbounded scan.

4. AuthZ — CLEAN. "workflows" is in adminOnlyTabs (shell_handler.go:148). The gate at shell_handler.go:311 returns ErrForbidden for non-admins BEFORE loadShellTabData → loadWorkflowsTabData → loadRunningWorkflows is ever reached. The new Running section rides the same gate; no bypass.

5. Data exposure — CLEAN. Selected columns are instance_id, execution_id, parent_instance_id, created_at, completed_at, queue (state hard-set to "running"). No workflow payload/args/metadata, no secrets, no PII. No new logging of sensitive fields. Workflow instances are system-level background jobs, not per-user data, so admin-gating (not per-user scoping) is the correct control.

6. Architecture boundary — CLEAN. The query lives in internal/wfengine, which legitimately owns the instances table. internal/settings consumes results via the cycle-safe appwire.WorkflowRow type and does not import wfengine. No domain package newly couples to go-workflows.

Findings

[MINOR] internal/settings/workflow_instances_sort.go:513 — Exported SortWorkflowInstances (and helper workflowStatusRank) is dead production code.
It is referenced only from *_test.go; no production caller exists (loadWorkflowHistory explicitly "no longer sorts the page"). Not a security issue, but an unused exported surface adds maintenance/confusion. Either wire it into the History render or delete it (and its test) in the same diff.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Security Review — PR #1205 (`bd-bookshelf-33oo`) Independent security review of the reworked PR adding `ListRunningInstances` (window-function query over the go-workflows `instances` table) surfaced in the admin Settings → Workflows tab. **1. SQL injection — CLEAN.** `runningInstancesSQL` (`internal/wfengine/diag_accessor.go:662`) is a compile-time `const` string. The only bound value is the `cap` limit, passed as a positional `?` parameter (`queryContext(ctx, runningInstancesSQL, cap)`, line 688). No identifiers or values are string-concatenated, and `cap` is the compile-time constant `shellWorkflowsRunningCap+1` (=501, `shell_handler.go:162`), never request-derived. No user input reaches the SQL. **2. Determinism — CLEAN.** Outer `ORDER BY created_at DESC, instance_id ASC`: after the `rn = 1` dedup, `instance_id` is unique per row → total order. Inner `ROW_NUMBER() OVER (PARTITION BY instance_id ORDER BY created_at DESC, id DESC)` carries the unique `id DESC` tiebreaker. No observable ties → no flake. **3. Bounded read — CLEAN.** Query ends in `LIMIT ?`; cap fixed at 501. Handler trims to 500 and sets `WorkflowRunningCapped` (`shell_handler.go:166-169`). Cannot be coerced into an unbounded scan. **4. AuthZ — CLEAN.** `"workflows"` is in `adminOnlyTabs` (`shell_handler.go:148`). The gate at `shell_handler.go:311` returns `ErrForbidden` for non-admins BEFORE `loadShellTabData` → `loadWorkflowsTabData` → `loadRunningWorkflows` is ever reached. The new Running section rides the same gate; no bypass. **5. Data exposure — CLEAN.** Selected columns are `instance_id, execution_id, parent_instance_id, created_at, completed_at, queue` (state hard-set to `"running"`). No workflow payload/args/`metadata`, no secrets, no PII. No new logging of sensitive fields. Workflow instances are system-level background jobs, not per-user data, so admin-gating (not per-user scoping) is the correct control. **6. Architecture boundary — CLEAN.** The query lives in `internal/wfengine`, which legitimately owns the `instances` table. `internal/settings` consumes results via the cycle-safe `appwire.WorkflowRow` type and does not import `wfengine`. No domain package newly couples to go-workflows. ### Findings [MINOR] internal/settings/workflow_instances_sort.go:513 — Exported `SortWorkflowInstances` (and helper `workflowStatusRank`) is dead production code. It is referenced only from `*_test.go`; no production caller exists (`loadWorkflowHistory` explicitly "no longer sorts the page"). Not a security issue, but an unused exported surface adds maintenance/confusion. Either wire it into the History render or delete it (and its test) in the same diff. REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor commented 2026-07-23 02:27:00 +00:00

Author

Owner

Copy link

UI Review — PR #1205 (bd-bookshelf-33oo)

Screenshot read: PNG 1280x800 confirmed. Both sections rendered and visible.

---

[MAJOR] internal/settings/shell_handler.go:640 — History section includes RUNNING instances, causing duplicates

Confirmed in screenshot: instance df27bdbd-790a-4675-a671-66bc88e6d441 with state RUNNING appears verbatim in both the Running section (top table) and the History section (bottom table). The comment at line 641-643 explicitly acknowledges "Running instances may appear here too" — but that contradicts the feature spec, where History EXCLUDES running instances. Root cause: loadWorkflowHistory calls ListWorkflowInstances with no filter on completed_at; every active instance lands in both result sets. Fix: add WHERE completed_at IS NOT NULL to the ListWorkflowInstances query (or its underlying SQL), or post-filter in loadWorkflowHistory by excluding any instance ID already present in data.WorkflowRunning. The stale comment at lines 641-643 must also be removed.

[MINOR] templates/pages/settings_shell.html:1753,1799 — heading uses undefined .section-title instead of canonical .tasks-section-title

The h3 "Running" and "History" headings use class="section-title" which has no rule in static/css/main.css. The canonical heading class for .tasks-section containers is .tasks-section-title (main.css:4955). Browser default h3 styling makes this look passable visually, but it diverges from the design system. Fix: replace section-title with tasks-section-title on both headings.

---

Positive observations:

• Two-section layout (Running above History) is clean and properly separated
• Running count badge uses canonical .badge class (main.css:3641)
• Tables use canonical .table class (main.css:1139)
• State pill uses .task-status .task-status--RUNNING matching the rest of the app
• Raw diagnostics UI link placement below History is unobtrusive
• No inline style= attributes found
• No bespoke parallel class system — .tasks-section wrapper is canonical
• Empty Running state renders a sensible "No running workflows." fallback

REVIEW VERDICT: 0 blocker, 1 major, 1 minor

## UI Review — PR #1205 (bd-bookshelf-33oo) **Screenshot read:** PNG 1280x800 confirmed. Both sections rendered and visible. --- [MAJOR] internal/settings/shell_handler.go:640 — History section includes RUNNING instances, causing duplicates Confirmed in screenshot: instance df27bdbd-790a-4675-a671-66bc88e6d441 with state RUNNING appears verbatim in both the Running section (top table) and the History section (bottom table). The comment at line 641-643 explicitly acknowledges "Running instances may appear here too" — but that contradicts the feature spec, where History EXCLUDES running instances. Root cause: loadWorkflowHistory calls ListWorkflowInstances with no filter on completed_at; every active instance lands in both result sets. Fix: add WHERE completed_at IS NOT NULL to the ListWorkflowInstances query (or its underlying SQL), or post-filter in loadWorkflowHistory by excluding any instance ID already present in data.WorkflowRunning. The stale comment at lines 641-643 must also be removed. [MINOR] templates/pages/settings_shell.html:1753,1799 — heading uses undefined .section-title instead of canonical .tasks-section-title The h3 "Running" and "History" headings use class="section-title" which has no rule in static/css/main.css. The canonical heading class for .tasks-section containers is .tasks-section-title (main.css:4955). Browser default h3 styling makes this look passable visually, but it diverges from the design system. Fix: replace section-title with tasks-section-title on both headings. --- **Positive observations:** - Two-section layout (Running above History) is clean and properly separated - Running count badge uses canonical .badge class (main.css:3641) - Tables use canonical .table class (main.css:1139) - State pill uses .task-status .task-status--RUNNING matching the rest of the app - Raw diagnostics UI link placement below History is unobtrusive - No inline style= attributes found - No bespoke parallel class system — .tasks-section wrapper is canonical - Empty Running state renders a sensible "No running workflows." fallback REVIEW VERDICT: 0 blocker, 1 major, 1 minor

zombor commented 2026-07-23 13:52:57 +00:00

Author

Owner

Copy link

UPDATED screenshot — running instance NO LONGER duplicated in History (fix commit)

Running section shows shot-running-001 (state=RUNNING).
History section shows only shot-completed-001 (state=COMPLETED).
The shot-running-001 running instance is correctly excluded from History, both in the initial server-rendered HTML (handler dedup via runningSet) and in the auto-refresh feed (RecentInstancesHandler now filters state=="running" before returning JSON to the workflow-list Stimulus controller).

**UPDATED screenshot — running instance NO LONGER duplicated in History (fix commit)** Running section shows `shot-running-001` (state=RUNNING). History section shows only `shot-completed-001` (state=COMPLETED). The `shot-running-001` running instance is correctly excluded from History, both in the initial server-rendered HTML (handler dedup via `runningSet`) and in the auto-refresh feed (`RecentInstancesHandler` now filters `state=="running"` before returning JSON to the `workflow-list` Stimulus controller). 

zombor added 1 commit 2026-07-23 13:57:52 +00:00

fix(settings): exclude running instances from History section and auto-refresh feed (bookshelf-33oo) ... 4311d13a13

Two-layer dedup fix for the RUNNING/History duplication bug:

1. server-render dedup — loadWorkflowHistory now builds a runningSet from
   data.WorkflowRunning and post-filters it from the History instance list;
   the next-page cursor is computed from the DB-ordered tail before filtering.
   Removes stale "Running instances may appear here too." comment.

2. auto-refresh dedup — RecentInstancesHandler (GET /admin/wf-instances,
   polled by the workflow-list Stimulus controller every 2s) now filters out
   state=="running" entries before returning JSON. Without this fix the JS
   controller would prepend running instances back into the History table on
   every poll, overriding the server-render dedup.

Also fixes all review minors:
- Delete dead SortWorkflowInstances + workflowStatusRank (and their tests)
- Update ListWorkflowInstances field comment (says "post-filters running")
- Fix section heading class section-title → tasks-section-title in template
- Fix browser e2e selectors to be Running-section-specific

Adds two new tests:
- shell_handler_test.go: regression test — running instance present in both
  stubs must not appear in History section output
- recent_instances_handler_test.go: running instances excluded from JSON feed
- diag_accessor_integration_test.go: ContinueAsNew epoch filtering (two
  scenarios: latest terminal excluded; latest non-terminal included)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor added 1 commit 2026-07-23 14:28:28 +00:00

fix(e2e): use execution-level dedup + start worker in Journey 13 ... b1dff73a95

Two browser e2e failures caused by the RecentInstancesHandler running-filter change:

1. shell_handler.go loadWorkflowHistory: switch from instance-level to
   execution-level dedup. The previous code excluded ALL executions for an
   instance_id if its latest epoch was running, hiding older completed CAN
   epochs from History. Now only the specific running ExecutionID is excluded,
   so older completed epochs remain visible in History.

2. Journey 14 (workflow_detail): update >2 assertions to >=1. With
   execution-level dedup only exec-bimx-01 (completed) appears in History;
   exec-bimx-02 (running) is correctly in the Running section only.

3. Journey 13 (workflow_list_autorefresh): start the workflow worker in
   BeforeAll so the triggered ping workflow completes and transitions out of
   "running" state before the auto-refresh poll picks it up. Extend the
   Eventually timeout from 10s to 30s to give the worker time to process the
   workflow under CI load.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor added 1 commit 2026-07-23 14:45:54 +00:00

fix(e2e): normalise heading text to lowercase before asserting "running" ... ecc2a2e647

.tasks-section-title has text-transform:uppercase in CSS, so Chromium's
innerText returns "RUNNING". The previous assertion used ContainSubstring("Running")
which fails a case-sensitive match against "RUNNING". Apply strings.ToLower
before comparing, matching the same pattern used in journey_workflow_detail_test.go.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

forgejo-actions commented 2026-07-23 14:47:51 +00:00

Copy link

Workflow Running section screenshot (workflows-running-section)

**Workflow Running section screenshot** (workflows-running-section) 

zombor added 1 commit 2026-07-23 15:03:41 +00:00

fix(wfengine): one-Expect-per-It in integration test + implement overfetch in RecentInstancesHandler (bookshelf-33oo) ... 23189517d0

- diag_accessor_integration_test: collapse loop-Expect into single
  ContainElement assertion with error folded in (Expect(ids, listErr))
- recent_instances_handler: fetch limit*2 rows so that after filtering
  running instances we still return up to limit completed entries;
  truncate filtered slice to limit before encoding
- recent_instances_handler_test: add overfetch proof — when first 3
  of 6 fetched rows are running, handler still returns 3 non-running

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

forgejo-actions commented 2026-07-23 15:05:16 +00:00

Copy link

Workflow Running section screenshot (workflows-running-section)

**Workflow Running section screenshot** (workflows-running-section) 

zombor added 1 commit 2026-07-23 16:46:42 +00:00

fix(wfengine): remove unreachable overfetch-cap branch to restore 100% coverage ... 782b0f6135

The `if fetch > recentInstancesMax*2` guard was dead code: limit is always
capped at recentInstancesMax (50) by the query-param parse, so
fetch = limit*2 <= recentInstancesMax*2 and the condition can never be true.
Remove the dead branch; add a comment explaining the invariant instead.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

forgejo-actions commented 2026-07-23 16:48:21 +00:00

Copy link

Workflow Running section screenshot (workflows-running-section)

**Workflow Running section screenshot** (workflows-running-section) 

zombor commented 2026-07-23 17:13:17 +00:00

Author

Owner

Copy link

Security re-review — dedup-fix delta (head 782b0f61, since cae63df9)

Scope: the fix delta added to #1205 after the original clean security review — the History-excludes-running filter (internal/settings/shell_handler.go) and the RecentInstancesHandler running-filter + overfetch (internal/wfengine/recent_instances_handler.go).

1. Bounded queries — OK.

• RecentInstancesHandler: limit starts at recentInstancesDefault (20) and is only overwritten when n > 0 && n <= recentInstancesMax (50). Negative, zero, non-numeric, and overflow (strconv.Atoi returns an out-of-range error) all fall through to the default. So limit is in [1,50] and fetch = limit*2 <= 100. No request param can make the fetch unbounded. Post-filter truncation history[:limit] is guarded by len(history) > limit. No mis-behaving path.
• History (loadWorkflowHistory): fetch stays shellWorkflowsPageSize+1 (constant) with keyset cursors (after_id/after_exec). The running-filter is in-memory and does not turn it into a scan. Cursor is derived from the DB-ordered tail BEFORE filtering, so keyset pagination stays correct.

2. Authz — unchanged. GET /admin/wf-instances remains adminRequired(...)-gated (engine.go:1622); the History section is served by the admin settings shell. The delta touches only in-memory row filtering — no new surface, no additional instances exposed to non-admins. The filter strictly reduces what is shown.

3. No data exposure / injection — OK. Filtering is in-memory over already-fetched rows (inst.State != "running" / runningSet membership by ExecutionID). No new SQL, no query built from user input, no new logs, no secrets/PII emitted.

4. Determinism (flake) — OK. Ordering is the pre-existing keyset query (created_at DESC, id DESC total order; id DESC tiebreaker documented as load-bearing, r6lx.2). The delta does not change ordering; the in-memory filter preserves DB order (append in iteration order).

Note (non-blocking, not a security finding): data.WorkflowRunning is capped at shellWorkflowsRunningCap; if running instances exceed that cap, an over-cap running execution would not be in runningSet and could still appear in History — a display-dedup edge, no security impact.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security re-review — dedup-fix delta (head 782b0f61, since cae63df9) Scope: the fix delta added to #1205 after the original clean security review — the History-excludes-running filter (`internal/settings/shell_handler.go`) and the `RecentInstancesHandler` running-filter + overfetch (`internal/wfengine/recent_instances_handler.go`). **1. Bounded queries — OK.** - `RecentInstancesHandler`: `limit` starts at `recentInstancesDefault` (20) and is only overwritten when `n > 0 && n <= recentInstancesMax` (50). Negative, zero, non-numeric, and overflow (`strconv.Atoi` returns an out-of-range error) all fall through to the default. So `limit` is in [1,50] and `fetch = limit*2 <= 100`. No request param can make the fetch unbounded. Post-filter truncation `history[:limit]` is guarded by `len(history) > limit`. No mis-behaving path. - History (`loadWorkflowHistory`): fetch stays `shellWorkflowsPageSize+1` (constant) with keyset cursors (`after_id`/`after_exec`). The running-filter is in-memory and does not turn it into a scan. Cursor is derived from the DB-ordered tail BEFORE filtering, so keyset pagination stays correct. **2. Authz — unchanged.** `GET /admin/wf-instances` remains `adminRequired(...)`-gated (engine.go:1622); the History section is served by the admin settings shell. The delta touches only in-memory row filtering — no new surface, no additional instances exposed to non-admins. The filter strictly reduces what is shown. **3. No data exposure / injection — OK.** Filtering is in-memory over already-fetched rows (`inst.State != "running"` / `runningSet` membership by `ExecutionID`). No new SQL, no query built from user input, no new logs, no secrets/PII emitted. **4. Determinism (flake) — OK.** Ordering is the pre-existing keyset query (`created_at DESC, id DESC` total order; `id DESC` tiebreaker documented as load-bearing, r6lx.2). The delta does not change ordering; the in-memory filter preserves DB order (append in iteration order). Note (non-blocking, not a security finding): `data.WorkflowRunning` is capped at `shellWorkflowsRunningCap`; if running instances exceed that cap, an over-cap running execution would not be in `runningSet` and could still appear in History — a display-dedup edge, no security impact. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor force-pushed bd-bookshelf-33oo from 782b0f6135 to 43efd38feb 2026-07-23 19:42:36 +00:00 Compare

forgejo-actions commented 2026-07-23 19:44:31 +00:00

Copy link

Workflow Running section screenshot (workflows-running-section)

**Workflow Running section screenshot** (workflows-running-section) 

zombor merged commit e1e169f795 into main 2026-07-23 19:47:11 +00:00

zombor referenced this pull request from a commit 2026-07-23 19:47:13 +00:00

Merge pull request 'feat(settings): workflows admin — dedicated Running section above paginated History [shot:workflow-running-section] (bookshelf-33oo)' (#1205) from bd-bookshelf-33oo into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!1205

No description provided.