test(wfengine): full-wiring recalc regression guard + #583 minors (bookshelf-m8mh) #614

zombor · 2026-06-18T16:05:28Z

zombor commented

2026-06-18 16:05:28 +00:00

Summary

PRIMARY: Adds a full-wiring Journey-6 e2e It step that fires POST /settings/match-weights/recalculate through the REAL app.New + StartWorker registration path, polls /admin/wf-status/{id} until completed, then asserts GetWorkflowDetail returns Result == "null" (true success). Catches the original bug: the old engine_integration_test used its own test-local registry so CI was green while the deployed worker was missing the workflow registration. This test FAILS if the workflow or any activity is not registered.
SECONDARY (#583 code-review minors):
1. recalc_scores_workflow.go: replace _ = err with if err != nil { return } for ExecuteActivity.Get() framework error
2. engine_integration_test.go: remove stale comment referencing removed recalcEpochDwell
3. engine_integration_test.go: refactor multi-epoch It blocks to move workflow start+await into JustBeforeEach per project conventions

Why e2e tier

The workflow tester (used by unit tests) registers activities on a test-local registry per Describe block — it does NOT share the production worker's registry. Only tests that boot app.New + StartWorker exercise the actual registration path. The e2e tier (Journey-6) is the right home: justification written in test comments.

Coverage

Added recalcGetWeightsErrActivities (export_test.go) — a test-only struct whose GetWeightsForRecalc method itself returns a permanent error. This covers the new if err != nil branch in the workflow without adding a coverage exclusion. The normal RecalcMatchScoresActivities.GetWeightsForRecalc always returns nil error (settings errors fall back to defaults by design), so a separate test-only struct is required.

Test plan

make test — all unit tests pass (776 specs)
make coverage — 100% on internal/wfengine with new error branch covered
make lint on changed packages — 0 issues
e2e Journey-6 step exercises real app wiring

Closes bead bookshelf-m8mh on merge.

## Summary - **PRIMARY**: Adds a full-wiring Journey-6 e2e `It` step that fires `POST /settings/match-weights/recalculate` through the REAL `app.New` + `StartWorker` registration path, polls `/admin/wf-status/{id}` until `completed`, then asserts `GetWorkflowDetail` returns `Result == "null"` (true success). Catches the original bug: the old `engine_integration_test` used its own test-local registry so CI was green while the deployed worker was missing the workflow registration. This test FAILS if the workflow or any activity is not registered. - **SECONDARY** (#583 code-review minors): 1. `recalc_scores_workflow.go`: replace `_ = err` with `if err != nil { return }` for `ExecuteActivity.Get()` framework error 2. `engine_integration_test.go`: remove stale comment referencing removed `recalcEpochDwell` 3. `engine_integration_test.go`: refactor multi-epoch `It` blocks to move workflow start+await into `JustBeforeEach` per project conventions ## Why e2e tier The workflow tester (used by unit tests) registers activities on a test-local registry per `Describe` block — it does NOT share the production worker's registry. Only tests that boot `app.New` + `StartWorker` exercise the actual registration path. The e2e tier (Journey-6) is the right home: justification written in test comments. ## Coverage Added `recalcGetWeightsErrActivities` (export_test.go) — a test-only struct whose `GetWeightsForRecalc` method itself returns a permanent error. This covers the new `if err != nil` branch in the workflow without adding a coverage exclusion. The normal `RecalcMatchScoresActivities.GetWeightsForRecalc` always returns nil error (settings errors fall back to defaults by design), so a separate test-only struct is required. ## Test plan - [ ] `make test` — all unit tests pass (776 specs) - [ ] `make coverage` — 100% on `internal/wfengine` with new error branch covered - [ ] `make lint` on changed packages — 0 issues - [ ] e2e Journey-6 step exercises real app wiring Closes bead bookshelf-m8mh on merge.

zombor added 1 commit

2026-06-18 16:05:28 +00:00

test(wfengine): full-wiring recalc regression guard + #583 review minor fixes (bookshelf-m8mh)

/ JS Unit Tests (pull_request) Successful in 22s

Details

/ Lint (pull_request) Successful in 1m59s

Details

/ E2E API (pull_request) Successful in 1m59s

Details

/ Integration (pull_request) Successful in 2m46s

Details

/ E2E Browser (pull_request) Successful in 2m53s

Details

/ Test (pull_request) Successful in 2m54s

Details

c4e536614c

PRIMARY — closes CI-green-while-broken gap:
- Adds Journey-6 `It` step: POSTs /settings/match-weights/recalculate, polls
  /admin/wf-status/{id} until "completed", then asserts GetWorkflowDetail returns
  Result == "null" (true success). Fails red if workflow/activity not registered.
- Adds App.GetWorkflowDetail helper (internal/app/app.go) for e2e assertions.

SECONDARY — #583 code-review minors:
1. recalc_scores_workflow.go: _ = err → if err != nil { return } for Get() error
2. engine_integration_test.go: remove stale recalcEpochDwell comment reference
3. engine_integration_test.go: move workflow start+await into JustBeforeEach

For coverage of the new error path: adds recalcGetWeightsErrActivities in
export_test.go — a test-only activity struct whose GetWeightsForRecalc method
itself returns a permanent error (unlike the production struct which always
returns nil, swallowing settings errors in favour of defaults). This allows the
workflow tester to dispatch the activity with an error and hit the new branch.

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

zombor commented

2026-06-18 16:11:29 +00:00

SECURITY REVIEW — PR #614 (bd-bookshelf-m8mh)

Scope: recalc wiring regression test + _ = err → check+return fix + ginkgo/comment cleanup. Primarily test code; low attack surface.

Findings

No security findings.

Auth/admin-gating on POST /settings/match-weights/recalculate: Confirmed unchanged. internal/settings/routes.go:35 wraps RecalcMatchScoresHandler with adminRequired(...) — that wrapper is not touched in this diff. The new e2e test exercises it via authClient, which is the pre-seeded admin session (see e2e/testutil/server.go:163, permission_admin = 1). The endpoint remains fully admin-gated; the test does not bypass or weaken that gating.

_ = err → if err != nil { return ... } in recalc_scores_workflow.go:131: The returned error propagates up through the go-workflows engine, which marks the workflow as completed-with-error. This value is stored in the WorkflowDetail.Result field in the wfengine DB — it is never written to an HTTP response body. The ErrorHandler.writeError (middleware) always writes http.StatusText(status) to the client, never err.Error() (confirmed at internal/middleware/error_handler.go:46,71). No internal error detail is newly exposed to clients.

App.GetWorkflowDetail in internal/app/app.go: New method is not wired to any HTTP route. It is only called from the e2e test via env.App.GetWorkflowDetail(...) (direct struct access, no HTTP path). No new unauthenticated surface introduced.

GET /admin/wf-status/{instanceID} status poll in the test: Uses authClient (admin session) throughout. The /admin/wf-status/ route is registered with adminRequired(...) in internal/wfengine/engine.go:1120 — unchanged by this PR.

Hardcoded credentials / committed DSN: None. The test uses suiteEnv.DSN() and testcontainers / BOOKSHELF_DSN env-var path — the standard pattern.

New injection / unbounded query / resource issue: No production SQL added. The only production change is the if err != nil { return ... } guard in the workflow function, which is pure control-flow with no I/O.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## SECURITY REVIEW — PR #614 (bd-bookshelf-m8mh) **Scope:** recalc wiring regression test + `_ = err` → check+return fix + ginkgo/comment cleanup. Primarily test code; low attack surface. --- ### Findings No security findings. --- **Auth/admin-gating on `POST /settings/match-weights/recalculate`:** Confirmed unchanged. `internal/settings/routes.go:35` wraps `RecalcMatchScoresHandler` with `adminRequired(...)` — that wrapper is not touched in this diff. The new e2e test exercises it via `authClient`, which is the pre-seeded admin session (see `e2e/testutil/server.go:163`, `permission_admin = 1`). The endpoint remains fully admin-gated; the test does not bypass or weaken that gating. **`_ = err` → `if err != nil { return ... }` in `recalc_scores_workflow.go:131`:** The returned error propagates up through the go-workflows engine, which marks the workflow as completed-with-error. This value is stored in the `WorkflowDetail.Result` field in the wfengine DB — it is never written to an HTTP response body. The `ErrorHandler.writeError` (middleware) always writes `http.StatusText(status)` to the client, never `err.Error()` (confirmed at `internal/middleware/error_handler.go:46,71`). No internal error detail is newly exposed to clients. **`App.GetWorkflowDetail` in `internal/app/app.go`:** New method is not wired to any HTTP route. It is only called from the e2e test via `env.App.GetWorkflowDetail(...)` (direct struct access, no HTTP path). No new unauthenticated surface introduced. **`GET /admin/wf-status/{instanceID}` status poll in the test:** Uses `authClient` (admin session) throughout. The `/admin/wf-status/` route is registered with `adminRequired(...)` in `internal/wfengine/engine.go:1120` — unchanged by this PR. **Hardcoded credentials / committed DSN:** None. The test uses `suiteEnv.DSN()` and `testcontainers` / `BOOKSHELF_DSN` env-var path — the standard pattern. **New injection / unbounded query / resource issue:** No production SQL added. The only production change is the `if err != nil { return ... }` guard in the workflow function, which is pure control-flow with no I/O. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented

2026-06-18 16:14:39 +00:00

CODE REVIEW: APPROVED

Phase 0: DEMO Verification

This PR is a test + small production fix. No DEMO block applies — the deliverable is verified by CI (the new tests run under make e2e and make integration). DEMO-N/A per test-only PR convention.

Phase 1: Spec Compliance

All spec requirements addressed:

Full-wiring e2e regression test (e2e/api/journey_6_wfengine_test.go:198): new It fires POST /settings/match-weights/recalculate through the real app.New + StartWorker path, polls /admin/wf-status/{id} until "completed", then asserts GetWorkflowDetail(...).Result == "null". This exercises the production registration path and will fail red if the workflow or any activity is unregistered — exactly what a "workflow not found" regression guard needs.
_ = err fix (internal/wfengine/recalc_scores_workflow.go:132): replaced with if err != nil { return fmt.Errorf(...) }. Framework-level errors (activity not found, context deadline) from ExecuteActivity.Get() are now propagated instead of silently discarded.
Stale comment removed (engine_integration_test.go BeforeEach and It): the two inline recalcEpochDwell references within the modified hunk were updated to reflect the real fix (diag id DESC tiebreaker). The block-level comment at lines 420–435 still references recalcEpochDwell but that text was not touched by this PR (pre-existing, outside the diff hunks) — minor nit only.
Ginkgo restructure (engine_integration_test.go:497–521): workflow start + polling moved from inside It into JustBeforeEach; result state stored in closure-scope vars; two It blocks now each make a single Expect. Correct.
New unit test (recalc_scores_workflow_test.go:342): new Describe with BeforeEach/JustBeforeEach/single-Expect It exercises the if err != nil branch via recalcGetWeightsErrActivities. Structure is clean.

Phase 2: Code Quality

Registration guard correctness (PRIMARY CONCERN): the new e2e It goes through app.New → buildExtendedDeps → buildRecalcMatchScoresDeps → wfengine.New with the real ext.RecalcMatchScores wiring, then app.StartWorker. This is the production registration path. A test-local re-registration of the workflow would bypass this and the guard would be hollow — but the test does NOT re-register anything; it only calls the HTTP endpoint and reads back the result via env.App.GetWorkflowDetail. Confirmed: this test WILL fail if RecalcMatchScoresWorkflow or its activities are removed from buildExtendedDeps.

Multi-epoch ContinueAsNew requirement: the memory rule workflow-integration-test-must-assert-success applies to engine_integration_test.go workflow tests. The existing integration test already drives ≥3 epochs (refactored to use JustBeforeEach in this PR). The new e2e test is a wiring guard, not a ContinueAsNew race test — single-epoch (1 book seeded) is appropriate here. The diag id DESC tiebreaker fix is already in production code and protects both.

app.GetWorkflowDetail (internal/app/app.go:831): clean nil-guard, delegates to wfengine.Engine.GetWorkflowDetail. internal/app is explicitly excluded from the coverage gate by design (pure wiring; verified by e2e). No test needed.

Activity stub correctness (export_test.go:655): recalcGetWeightsErrActivities implements all three methods of the activity interface. go-workflows tester resolves activities by method name (confirmed from registry source: r.activityMap[mt.Name] = mv.Interface()), so "GetWeightsForRecalc" from the stub struct will be dispatched when the workflow calls a.GetWeightsForRecalc on the real *RecalcMatchScoresActivities struct. The stub correctly covers the if err != nil branch.

Minor — stale block comment (internal/wfengine/engine_integration_test.go:423–434): the block comment above the Describe still says "Without the recalcEpochDwell timer … this test fails." The actual fix was the id DESC tiebreaker in diag_accessor.go. This is pre-existing text (not introduced by this PR) but the PR touches the same Describe, making it a good opportunity to update.

[MINOR] internal/wfengine/engine_integration_test.go:423 — stale block comment references recalcEpochDwell as the fix, but the real fix is the id DESC tiebreaker in diag_accessor.go. Pre-existing text, but the PR modifies the same Describe block and could have updated it. No correctness impact.

Minor — comment/reality mismatch (e2e/api/journey_6_wfengine_test.go:216): comment says "Empty library ⇒ single epoch" but BeforeAll seeds one book (bookID). The workflow will run 2 epochs (one with 1 book, one with empty list). Not a correctness issue — the test works either way — but the comment is misleading.

[MINOR] e2e/api/journey_6_wfengine_test.go:216 — comment says "Empty library" but BeforeAll seeds one book. Test still passes, but the documentation is inaccurate.

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

CODE REVIEW: APPROVED ## Phase 0: DEMO Verification This PR is a test + small production fix. No DEMO block applies — the deliverable is verified by CI (the new tests run under `make e2e` and `make integration`). DEMO-N/A per test-only PR convention. --- ## Phase 1: Spec Compliance All spec requirements addressed: - **Full-wiring e2e regression test** (`e2e/api/journey_6_wfengine_test.go:198`): new `It` fires `POST /settings/match-weights/recalculate` through the real `app.New` + `StartWorker` path, polls `/admin/wf-status/{id}` until `"completed"`, then asserts `GetWorkflowDetail(...).Result == "null"`. This exercises the production registration path and will fail red if the workflow or any activity is unregistered — exactly what a "workflow not found" regression guard needs. - **`_ = err` fix** (`internal/wfengine/recalc_scores_workflow.go:132`): replaced with `if err != nil { return fmt.Errorf(...) }`. Framework-level errors (activity not found, context deadline) from `ExecuteActivity.Get()` are now propagated instead of silently discarded. - **Stale comment removed** (`engine_integration_test.go` BeforeEach and It): the two inline `recalcEpochDwell` references within the modified hunk were updated to reflect the real fix (diag `id DESC` tiebreaker). The block-level comment at lines 420–435 still references `recalcEpochDwell` but that text was not touched by this PR (pre-existing, outside the diff hunks) — minor nit only. - **Ginkgo restructure** (`engine_integration_test.go:497–521`): workflow start + polling moved from inside `It` into `JustBeforeEach`; result state stored in closure-scope vars; two `It` blocks now each make a single `Expect`. Correct. - **New unit test** (`recalc_scores_workflow_test.go:342`): new `Describe` with `BeforeEach`/`JustBeforeEach`/single-`Expect` `It` exercises the `if err != nil` branch via `recalcGetWeightsErrActivities`. Structure is clean. --- ## Phase 2: Code Quality **Registration guard correctness (PRIMARY CONCERN):** the new e2e `It` goes through `app.New` → `buildExtendedDeps` → `buildRecalcMatchScoresDeps` → `wfengine.New` with the real `ext.RecalcMatchScores` wiring, then `app.StartWorker`. This is the production registration path. A test-local re-registration of the workflow would bypass this and the guard would be hollow — but the test does NOT re-register anything; it only calls the HTTP endpoint and reads back the result via `env.App.GetWorkflowDetail`. Confirmed: this test WILL fail if `RecalcMatchScoresWorkflow` or its activities are removed from `buildExtendedDeps`. **Multi-epoch ContinueAsNew requirement:** the memory rule [[workflow-integration-test-must-assert-success]] applies to `engine_integration_test.go` workflow tests. The existing integration test already drives ≥3 epochs (refactored to use `JustBeforeEach` in this PR). The new e2e test is a wiring guard, not a ContinueAsNew race test — single-epoch (1 book seeded) is appropriate here. The diag `id DESC` tiebreaker fix is already in production code and protects both. **`app.GetWorkflowDetail`** (`internal/app/app.go:831`): clean nil-guard, delegates to `wfengine.Engine.GetWorkflowDetail`. `internal/app` is explicitly excluded from the coverage gate by design (pure wiring; verified by e2e). No test needed. **Activity stub correctness** (`export_test.go:655`): `recalcGetWeightsErrActivities` implements all three methods of the activity interface. go-workflows tester resolves activities by method name (confirmed from registry source: `r.activityMap[mt.Name] = mv.Interface()`), so `"GetWeightsForRecalc"` from the stub struct will be dispatched when the workflow calls `a.GetWeightsForRecalc` on the real `*RecalcMatchScoresActivities` struct. The stub correctly covers the `if err != nil` branch. **Minor — stale block comment** (`internal/wfengine/engine_integration_test.go:423–434`): the block comment above the Describe still says "Without the recalcEpochDwell timer … this test fails." The actual fix was the `id DESC` tiebreaker in `diag_accessor.go`. This is pre-existing text (not introduced by this PR) but the PR touches the same Describe, making it a good opportunity to update. [MINOR] `internal/wfengine/engine_integration_test.go:423` — stale block comment references `recalcEpochDwell` as the fix, but the real fix is the `id DESC` tiebreaker in `diag_accessor.go`. Pre-existing text, but the PR modifies the same Describe block and could have updated it. No correctness impact. **Minor — comment/reality mismatch** (`e2e/api/journey_6_wfengine_test.go:216`): comment says "Empty library ⇒ single epoch" but `BeforeAll` seeds one book (`bookID`). The workflow will run 2 epochs (one with 1 book, one with empty list). Not a correctness issue — the test works either way — but the comment is misleading. [MINOR] `e2e/api/journey_6_wfengine_test.go:216` — comment says "Empty library" but `BeforeAll` seeds one book. Test still passes, but the documentation is inaccurate. --- REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor commented

2026-06-18 16:14:54 +00:00

CODE REVIEW: APPROVED

Phase 0: DEMO Verification

Test-only PR — no DEMO block required. Correctness verified by CI.

Phase 1: Spec Compliance

All spec requirements addressed:

Full-wiring e2e regression test (e2e/api/journey_6_wfengine_test.go:198): new It fires POST /settings/match-weights/recalculate through the real app.New + StartWorker path, polls /admin/wf-status/{id} until "completed", then asserts GetWorkflowDetail(...).Result == "null". Exercises the production registration path — will fail red if the workflow or activity is unregistered.
_ = err fix (internal/wfengine/recalc_scores_workflow.go:132): replaced with if err != nil { return }. Framework-level errors from ExecuteActivity.Get() are now propagated.
Stale inline recalcEpochDwell comments removed from the modified hunk in engine_integration_test.go. Pre-existing block comment at lines 420-435 not touched (outside diff hunks).
Ginkgo restructure (engine_integration_test.go:497-521): workflow start + polling moved into JustBeforeEach; two It blocks each make one Expect. Correct.
New unit test (recalc_scores_workflow_test.go:342): BeforeEach/JustBeforeEach/single-Expect It exercises the if err != nil branch.

Phase 2: Code Quality

Registration guard correctness: the new e2e It goes through app.New -> buildExtendedDeps -> buildRecalcMatchScoresDeps -> wfengine.New with the real ext.RecalcMatchScores wiring, then app.StartWorker. The test does NOT re-register anything; it only calls the HTTP endpoint and reads back the result via env.App.GetWorkflowDetail. Confirmed: this test WILL fail if RecalcMatchScoresWorkflow or its activities are removed from buildExtendedDeps.

Multi-epoch requirement: workflow-integration-test-must-assert-success applies to engine_integration_test.go. The existing integration test now drives >=3 epochs via JustBeforeEach. The new e2e test is a wiring guard, not a ContinueAsNew race test — single-epoch is appropriate. The diag id DESC tiebreaker fix is in production code and protects both.

app.GetWorkflowDetail (internal/app/app.go:831): clean nil-guard, delegates to wfengine. internal/app is excluded from coverage gate by design.

Activity stub correctness (export_test.go:655): recalcGetWeightsErrActivities implements all three methods. go-workflows resolves activities by method name (registry source confirmed: r.activityMap[mt.Name] = mv.Interface()), so the stub's GetWeightsForRecalc will be dispatched when the workflow calls a.GetWeightsForRecalc on the real struct.

[MINOR] internal/wfengine/engine_integration_test.go:423 — block comment says "Without the recalcEpochDwell timer ... this test fails" but the actual fix is the id DESC tiebreaker in diag_accessor.go. Pre-existing text not introduced by this PR, but the same Describe was modified and could have been updated. No correctness impact.

[MINOR] e2e/api/journey_6_wfengine_test.go:216 — comment says "Empty library => single epoch" but BeforeAll seeds one book (bookID). The workflow runs 2 epochs. Test works correctly either way; comment is inaccurate.

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

CODE REVIEW: APPROVED ## Phase 0: DEMO Verification Test-only PR — no DEMO block required. Correctness verified by CI. --- ## Phase 1: Spec Compliance All spec requirements addressed: - Full-wiring e2e regression test (e2e/api/journey_6_wfengine_test.go:198): new It fires POST /settings/match-weights/recalculate through the real app.New + StartWorker path, polls /admin/wf-status/{id} until "completed", then asserts GetWorkflowDetail(...).Result == "null". Exercises the production registration path — will fail red if the workflow or activity is unregistered. - _ = err fix (internal/wfengine/recalc_scores_workflow.go:132): replaced with if err != nil { return }. Framework-level errors from ExecuteActivity.Get() are now propagated. - Stale inline recalcEpochDwell comments removed from the modified hunk in engine_integration_test.go. Pre-existing block comment at lines 420-435 not touched (outside diff hunks). - Ginkgo restructure (engine_integration_test.go:497-521): workflow start + polling moved into JustBeforeEach; two It blocks each make one Expect. Correct. - New unit test (recalc_scores_workflow_test.go:342): BeforeEach/JustBeforeEach/single-Expect It exercises the if err != nil branch. --- ## Phase 2: Code Quality **Registration guard correctness:** the new e2e It goes through app.New -> buildExtendedDeps -> buildRecalcMatchScoresDeps -> wfengine.New with the real ext.RecalcMatchScores wiring, then app.StartWorker. The test does NOT re-register anything; it only calls the HTTP endpoint and reads back the result via env.App.GetWorkflowDetail. Confirmed: this test WILL fail if RecalcMatchScoresWorkflow or its activities are removed from buildExtendedDeps. **Multi-epoch requirement:** [[workflow-integration-test-must-assert-success]] applies to engine_integration_test.go. The existing integration test now drives >=3 epochs via JustBeforeEach. The new e2e test is a wiring guard, not a ContinueAsNew race test — single-epoch is appropriate. The diag id DESC tiebreaker fix is in production code and protects both. **app.GetWorkflowDetail (internal/app/app.go:831):** clean nil-guard, delegates to wfengine. internal/app is excluded from coverage gate by design. **Activity stub correctness (export_test.go:655):** recalcGetWeightsErrActivities implements all three methods. go-workflows resolves activities by method name (registry source confirmed: r.activityMap[mt.Name] = mv.Interface()), so the stub's GetWeightsForRecalc will be dispatched when the workflow calls a.GetWeightsForRecalc on the real struct. [MINOR] internal/wfengine/engine_integration_test.go:423 — block comment says "Without the recalcEpochDwell timer ... this test fails" but the actual fix is the id DESC tiebreaker in diag_accessor.go. Pre-existing text not introduced by this PR, but the same Describe was modified and could have been updated. No correctness impact. [MINOR] e2e/api/journey_6_wfengine_test.go:216 — comment says "Empty library => single epoch" but BeforeAll seeds one book (bookID). The workflow runs 2 epochs. Test works correctly either way; comment is inaccurate. --- REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor commented

2026-06-18 16:15:11 +00:00

CODE REVIEW: APPROVED

Phase 0: DEMO Verification

Test-only PR - no DEMO block required. Correctness verified by CI.

Phase 1: Spec Compliance

All spec requirements addressed:

Full-wiring e2e regression test (e2e/api/journey_6_wfengine_test.go:198): new It fires POST /settings/match-weights/recalculate through the real app.New + StartWorker path, polls /admin/wf-status/{id} until completed, then asserts GetWorkflowDetail().Result == null. Exercises the production registration path and will fail red if the workflow or any activity is unregistered.
_ = err fix (internal/wfengine/recalc_scores_workflow.go:132): replaced with if err != nil { return }. Framework-level errors from ExecuteActivity.Get() are now propagated instead of silently discarded.
Stale inline recalcEpochDwell comments removed from the modified hunk. Pre-existing block comment at lines 420-435 not touched (outside diff hunks).
Ginkgo restructure (engine_integration_test.go:497-521): workflow start and polling moved into JustBeforeEach; two It blocks each make a single Expect. Correct ginkgo structure.
New unit test (recalc_scores_workflow_test.go:342): BeforeEach/JustBeforeEach/single-Expect It exercises the if err != nil branch via recalcGetWeightsErrActivities.

Phase 2: Code Quality

Registration guard correctness: the new e2e It goes through app.New -> buildExtendedDeps -> buildRecalcMatchScoresDeps -> wfengine.New with the real ext.RecalcMatchScores wiring, then app.StartWorker. The test does NOT re-register anything on its own engine; it only calls the HTTP endpoint and reads back the result via env.App.GetWorkflowDetail. Confirmed: this test WILL fail if RecalcMatchScoresWorkflow or its activities are removed from buildExtendedDeps.

Multi-epoch requirement: workflow-integration-test-must-assert-success applies to engine_integration_test.go. The existing integration test already drives >=3 epochs (now via JustBeforeEach). The new e2e test is a wiring guard, not a ContinueAsNew race test - single-epoch is appropriate. The diag id DESC tiebreaker fix in production code protects both.

app.GetWorkflowDetail (internal/app/app.go:831): clean nil-guard, delegates to wfengine. internal/app is excluded from coverage gate by design (pure wiring).

Activity stub correctness (export_test.go:655): recalcGetWeightsErrActivities implements all three methods. go-workflows resolves activities by method name (registry source confirmed: r.activityMap[mt.Name] = mv.Interface()), so the stub GetWeightsForRecalc will be dispatched when the workflow calls a.GetWeightsForRecalc on the real struct.

[MINOR] internal/wfengine/engine_integration_test.go:423 - block comment says Without the recalcEpochDwell timer this test fails but the actual fix is the id DESC tiebreaker in diag_accessor.go. Pre-existing text not introduced by this PR, but the same Describe was modified. No correctness impact.

[MINOR] e2e/api/journey_6_wfengine_test.go:216 - comment says Empty library => single epoch but BeforeAll seeds one book (bookID). Workflow runs 2 epochs. Test works correctly; comment is inaccurate.

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

CODE REVIEW: APPROVED ## Phase 0: DEMO Verification Test-only PR - no DEMO block required. Correctness verified by CI. --- ## Phase 1: Spec Compliance All spec requirements addressed: - Full-wiring e2e regression test (e2e/api/journey_6_wfengine_test.go:198): new It fires POST /settings/match-weights/recalculate through the real app.New + StartWorker path, polls /admin/wf-status/{id} until completed, then asserts GetWorkflowDetail().Result == null. Exercises the production registration path and will fail red if the workflow or any activity is unregistered. - _ = err fix (internal/wfengine/recalc_scores_workflow.go:132): replaced with if err != nil { return }. Framework-level errors from ExecuteActivity.Get() are now propagated instead of silently discarded. - Stale inline recalcEpochDwell comments removed from the modified hunk. Pre-existing block comment at lines 420-435 not touched (outside diff hunks). - Ginkgo restructure (engine_integration_test.go:497-521): workflow start and polling moved into JustBeforeEach; two It blocks each make a single Expect. Correct ginkgo structure. - New unit test (recalc_scores_workflow_test.go:342): BeforeEach/JustBeforeEach/single-Expect It exercises the if err != nil branch via recalcGetWeightsErrActivities. --- ## Phase 2: Code Quality Registration guard correctness: the new e2e It goes through app.New -> buildExtendedDeps -> buildRecalcMatchScoresDeps -> wfengine.New with the real ext.RecalcMatchScores wiring, then app.StartWorker. The test does NOT re-register anything on its own engine; it only calls the HTTP endpoint and reads back the result via env.App.GetWorkflowDetail. Confirmed: this test WILL fail if RecalcMatchScoresWorkflow or its activities are removed from buildExtendedDeps. Multi-epoch requirement: [[workflow-integration-test-must-assert-success]] applies to engine_integration_test.go. The existing integration test already drives >=3 epochs (now via JustBeforeEach). The new e2e test is a wiring guard, not a ContinueAsNew race test - single-epoch is appropriate. The diag id DESC tiebreaker fix in production code protects both. app.GetWorkflowDetail (internal/app/app.go:831): clean nil-guard, delegates to wfengine. internal/app is excluded from coverage gate by design (pure wiring). Activity stub correctness (export_test.go:655): recalcGetWeightsErrActivities implements all three methods. go-workflows resolves activities by method name (registry source confirmed: r.activityMap[mt.Name] = mv.Interface()), so the stub GetWeightsForRecalc will be dispatched when the workflow calls a.GetWeightsForRecalc on the real struct. [MINOR] internal/wfengine/engine_integration_test.go:423 - block comment says Without the recalcEpochDwell timer this test fails but the actual fix is the id DESC tiebreaker in diag_accessor.go. Pre-existing text not introduced by this PR, but the same Describe was modified. No correctness impact. [MINOR] e2e/api/journey_6_wfengine_test.go:216 - comment says Empty library => single epoch but BeforeAll seeds one book (bookID). Workflow runs 2 epochs. Test works correctly; comment is inaccurate. --- REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor reviewed

2026-06-18 16:19:24 +00:00

internal/app/app.go Outdated

					
				@ -824,0 +827,4 @@

				//

				// This method exposes the wfengine diag layer for tests and admin tooling that

				// need to inspect a workflow's terminal result without going through the HTTP

				// handler (which renders HTML, not JSON).

All http routes should use content negotiation to serve json. if we had that, would this new function not be needed?

zombor referenced this pull request from a commit

2026-06-18 16:26:27 +00:00

feat(wfengine): JSON content negotiation on workflow-detail route; drop test-only accessor (bookshelf-m8mh, #614 review)

zombor added 1 commit

2026-06-18 16:26:27 +00:00

feat(wfengine): JSON content negotiation on workflow-detail route; drop test-only accessor (bookshelf-m8mh, #614 review)

/ JS Unit Tests (pull_request) Successful in 28s

Details

/ Lint (pull_request) Successful in 2m4s

Details

/ E2E API (pull_request) Successful in 2m19s

Details

/ E2E Browser (pull_request) Successful in 2m39s

Details

/ Integration (pull_request) Successful in 3m27s

Details

/ Test (pull_request) Successful in 3m52s

Details

50bd72546d

- Add JSON content negotiation to WorkflowDetailHandler: when Accept: application/json,
  encodes WorkflowDetail as JSON instead of rendering HTML. Mirrors the pattern in
  WorkflowsHandler. Error paths (404/500) also respect the Accept header.
- Add json:"..." tags to WorkflowInstance and WorkflowDetail so the JSON output
  is clean (snake_case field names, omitempty where appropriate).
- Remove App.GetWorkflowDetail accessor from internal/app/app.go — now unneeded
  because the e2e test uses the HTTP endpoint directly.
- Update e2e Journey-6 RecalcMatchScores It: replace env.App.GetWorkflowDetail(...)
  with a real HTTP GET /admin/workflows/{instanceID} with Accept: application/json.
- Add unit tests for the JSON content-negotiation path in workflow_detail_handler_test.go:
  200+Content-Type, JSON body with instance_id, no HTML render, 404 and 500 JSON paths.
- Fix stale comment in engine_integration_test.go (~line 423): remove reference to the
  removed recalcEpochDwell timer; document the real fix (id DESC tiebreaker).
- Fix inaccurate comment in journey_6 (~line 216): "Empty library ⇒ single epoch" was
  wrong; BeforeAll seeds one book → 2 epochs (one page + one terminal ContinueAsNew).

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

zombor commented

2026-06-18 16:29:29 +00:00

SECURITY REVIEW — PR #614 (bd-bookshelf-m8mh)

Scope: JSON content negotiation on GET /admin/workflows/{instanceID}, JSON tags added to WorkflowDetail/WorkflowInstance, removal of App.GetWorkflowDetail, and workflow error-handling fixes.

Admin-gating — confirmed intact.
GET /admin/workflows/{instanceID} is registered at engine.go:1118 as adminRequired(WorkflowDetailHandler(...)). The JSON branch is entirely inside WorkflowDetailHandler — it runs after the adminRequired middleware has already checked auth. There is no bypass: WantsJSON only inspects the Accept request header and returns a boolean; it has no side effect on the auth chain.

New data exposure via JSON — none.
The JSON branch serialises the same WorkflowDetail value that the HTML template receives. No new fields are added; the JSON tags merely name the existing fields for encoding. All workflow Input structs contain only non-secret domain IDs and cursors: book_ids []int64, user_id int64, library_id int64, cursor int64, library_id + scan_done + concurrency, etc. No API keys, tokens, passwords, or PII are present in any Input struct.

Newly serialised fields — no private/secret leak.
WorkflowInstance gains tags for instance_id, execution_id, parent_id, created_at, completed_at, state, queue — all diagnostic metadata already visible on the HTML page. WorkflowDetail gains history, workflow_name, input, result — same data. parent_id, completed_at, queue, history, workflow_name, input are all omitempty; result is always emitted (needed by the test guard). No internal cursor, no secret.

Error paths — no internal detail leaked.
404 emits {"error":"instance not found"} (hardcoded literal). 500 emits {"error":"failed to load workflow detail"} (hardcoded literal). Neither path calls err.Error() into the response body. The actual error is logged to slog with the instance ID, matching the existing HTML path.

App.GetWorkflowDetail removal — no production caller removed.
Searched the full non-test, non-worktree codebase: the only production call site for GetWorkflowDetail is engine.go:1111 inside RegisterRoutes, which calls engine.GetWorkflowDetail directly. No App.GetWorkflowDetail wrapper exists or existed in production code; the removal was test-only and carries no auth implications.

Error handling fix (_ = err → if err != nil { return ... }) — security-neutral, correct.
Propagating the framework-level GetWeightsForRecalc error prevents silent swallowing of an unregistered-activity fault. No security surface.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

This is a clean, low-surface, admin-gated change. The JSON branch is fully behind adminRequired, exposes only the same non-secret diagnostic data already shown in HTML, and error responses are hardcoded generic messages with no internal leakage.

SECURITY REVIEW — PR #614 (bd-bookshelf-m8mh) **Scope:** JSON content negotiation on `GET /admin/workflows/{instanceID}`, JSON tags added to `WorkflowDetail`/`WorkflowInstance`, removal of `App.GetWorkflowDetail`, and workflow error-handling fixes. --- **Admin-gating — confirmed intact.** `GET /admin/workflows/{instanceID}` is registered at engine.go:1118 as `adminRequired(WorkflowDetailHandler(...))`. The JSON branch is entirely inside `WorkflowDetailHandler` — it runs *after* the `adminRequired` middleware has already checked auth. There is no bypass: `WantsJSON` only inspects the `Accept` request header and returns a boolean; it has no side effect on the auth chain. **New data exposure via JSON — none.** The JSON branch serialises the same `WorkflowDetail` value that the HTML template receives. No new fields are added; the JSON tags merely name the existing fields for encoding. All workflow Input structs contain only non-secret domain IDs and cursors: `book_ids []int64`, `user_id int64`, `library_id int64`, `cursor int64`, `library_id + scan_done + concurrency`, etc. No API keys, tokens, passwords, or PII are present in any Input struct. **Newly serialised fields — no private/secret leak.** `WorkflowInstance` gains tags for `instance_id`, `execution_id`, `parent_id`, `created_at`, `completed_at`, `state`, `queue` — all diagnostic metadata already visible on the HTML page. `WorkflowDetail` gains `history`, `workflow_name`, `input`, `result` — same data. `parent_id`, `completed_at`, `queue`, `history`, `workflow_name`, `input` are all `omitempty`; `result` is always emitted (needed by the test guard). No internal cursor, no secret. **Error paths — no internal detail leaked.** 404 emits `{"error":"instance not found"}` (hardcoded literal). 500 emits `{"error":"failed to load workflow detail"}` (hardcoded literal). Neither path calls `err.Error()` into the response body. The actual error is logged to `slog` with the instance ID, matching the existing HTML path. **`App.GetWorkflowDetail` removal — no production caller removed.** Searched the full non-test, non-worktree codebase: the only production call site for `GetWorkflowDetail` is `engine.go:1111` inside `RegisterRoutes`, which calls `engine.GetWorkflowDetail` directly. No `App.GetWorkflowDetail` wrapper exists or existed in production code; the removal was test-only and carries no auth implications. **Error handling fix (`_ = err` → `if err != nil { return ... }`) — security-neutral, correct.** Propagating the framework-level `GetWeightsForRecalc` error prevents silent swallowing of an unregistered-activity fault. No security surface. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor This is a clean, low-surface, admin-gated change. The JSON branch is fully behind `adminRequired`, exposes only the same non-secret diagnostic data already shown in HTML, and error responses are hardcoded generic messages with no internal leakage.

zombor commented

2026-06-18 16:30:56 +00:00

CODE REVIEW: APPROVED

Re-review of SHA 50bd7254 — content-negotiation additions + App.GetWorkflowDetail removal.

Phase 0: DEMO — N/A (no DEMO block; this is a targeted incremental re-review of code-only changes)

Phase 1 + 2: Diff Review

Scope of this re-review: workflow_detail_handler.go, diag_accessor.go, export_test.go, workflow_detail_handler_test.go, engine_integration_test.go, recalc_scores_workflow.go, recalc_scores_workflow_test.go, journey_6_wfengine_test.go.

Content-negotiation (workflow_detail_handler.go)

[MINOR] internal/wfengine/workflow_detail_handler.go:51,59 — JSON error paths use http.Error which sets Content-Type: text/plain; charset=utf-8, not application/json
The 404 and 500 JSON branches write a JSON-shaped body ({"error":"..."}) but http.Error overrides the content-type to text/plain. JSON clients parsing on Content-Type will be surprised. However, this is the same pattern used in workflows_handler.go for its error paths — it is an existing project-wide convention, not a regression introduced here. Consistent with the codebase, not a blocker.

The 200 path is correct: w.Header().Set("Content-Type", "application/json") is set before w.WriteHeader(http.StatusOK), and json.NewEncoder(w).Encode(detail) follows. HTML path is completely unchanged. Pattern mirrors workflows_handler.go:81–85. No issues.

JSON tags (diag_accessor.go)

WorkflowInstance fields: instance_id, execution_id, parent_id (omitempty), created_at, completed_at (omitempty), state, queue (omitempty) — all snake_case, consistent with the JSON API.

WorkflowDetail fields: history (omitempty), workflow_name (omitempty), input (omitempty), result (no omitempty) — result is always present in the JSON output as required by the e2e assertion. The WorkflowInstance is embedded without a top-level key, so instance_id appears at the top level of the JSON object. Consistent and correct.

App.GetWorkflowDetail removal (internal/app/app.go)

git diff origin/main...origin/bd-bookshelf-m8mh -- internal/app/ is empty — the accessor was already absent from app.go (it was wired inline in engine.go via a closure, not exposed as an App method). No dangling callers found. internal/app/export_test.go has no reference. Clean.

e2e regression guard (journey_6_wfengine_test.go)

The new It correctly:

POSTs to /settings/match-weights/recalculate and captures instance_id.
Polls /admin/wf-status/{id} until state == "completed" (30 s deadline).
GETs /admin/workflows/{id} with Accept: application/json and asserts Result == "null".

The "null" assertion is sound: a nil-error workflow return is serialized by go-workflows as JSON null bytes; prettyPayload on []byte("null") unmarshals to nil interface then re-encodes to the string "null". The existing integration tests already assert the same Equal("null") and CI is green.

Regression-guard property is preserved: any registration mismatch produces a non-null error string in Result, making this It fail red.

Multi-Expect in the e2e It is within policy (journey It relaxation).

Unit tests (workflow_detail_handler_test.go)

[MINOR] internal/wfengine/workflow_detail_handler_test.go:130-137 — "returns 200 OK with Content-Type application/json" It contains two Expect calls (status code + header)
Unit tests retain strict one-Expect-per-It per project conventions. These should be two separate It blocks. Does not affect correctness or CI outcome.

[MINOR] internal/wfengine/workflow_detail_handler_test.go:139-151 — "encodes the WorkflowDetail as JSON with instance_id field" It contains two Expect calls (json.Unmarshal + got.InstanceID)
Same rule. The unmarshal success and the field value are two separate assertions. Should be split, or the unmarshal folded into JustBeforeEach.

All JSON branch cases are covered: 200 (Content-Type, body, no HTML), 404, 500. The renderer.lastPage check correctly confirms the HTML path is not taken. Good coverage.

Integration test refactor (engine_integration_test.go)

JustBeforeEach now drives the workflow start + polling + GetWorkflowDetail call, with the two It blocks asserting independently. Correctly follows BeforeEach/JustBeforeEach/It separation. The previous version ran the full workflow twice (once per It), which was wasteful and produced two concurrent workflow runs. Fixed.

recalc_scores_workflow.go — `_ = err` → `if err != nil { return ... }`

Correct. GetWeightsForRecalc defensively swallows settings errors internally, but framework-level errors (activity not registered, context deadline) must propagate. The new error path is covered by the new recalcGetWeightsErrActivities unit test.

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

## CODE REVIEW: APPROVED Re-review of SHA 50bd7254 — content-negotiation additions + `App.GetWorkflowDetail` removal. --- ### Phase 0: DEMO — N/A (no DEMO block; this is a targeted incremental re-review of code-only changes) --- ### Phase 1 + 2: Diff Review **Scope of this re-review:** `workflow_detail_handler.go`, `diag_accessor.go`, `export_test.go`, `workflow_detail_handler_test.go`, `engine_integration_test.go`, `recalc_scores_workflow.go`, `recalc_scores_workflow_test.go`, `journey_6_wfengine_test.go`. --- #### Content-negotiation (workflow_detail_handler.go) [MINOR] internal/wfengine/workflow_detail_handler.go:51,59 — JSON error paths use `http.Error` which sets `Content-Type: text/plain; charset=utf-8`, not `application/json` The 404 and 500 JSON branches write a JSON-shaped body (`{"error":"..."}`) but `http.Error` overrides the content-type to `text/plain`. JSON clients parsing on `Content-Type` will be surprised. However, this is the **same pattern** used in `workflows_handler.go` for its error paths — it is an existing project-wide convention, not a regression introduced here. Consistent with the codebase, not a blocker. The 200 path is correct: `w.Header().Set("Content-Type", "application/json")` is set before `w.WriteHeader(http.StatusOK)`, and `json.NewEncoder(w).Encode(detail)` follows. HTML path is completely unchanged. Pattern mirrors `workflows_handler.go:81–85`. No issues. --- #### JSON tags (diag_accessor.go) `WorkflowInstance` fields: `instance_id`, `execution_id`, `parent_id` (omitempty), `created_at`, `completed_at` (omitempty), `state`, `queue` (omitempty) — all snake_case, consistent with the JSON API. `WorkflowDetail` fields: `history` (omitempty), `workflow_name` (omitempty), `input` (omitempty), `result` (no omitempty) — `result` is always present in the JSON output as required by the e2e assertion. The `WorkflowInstance` is embedded without a top-level key, so `instance_id` appears at the top level of the JSON object. Consistent and correct. --- #### App.GetWorkflowDetail removal (internal/app/app.go) `git diff origin/main...origin/bd-bookshelf-m8mh -- internal/app/` is empty — the accessor was already absent from `app.go` (it was wired inline in `engine.go` via a closure, not exposed as an `App` method). No dangling callers found. `internal/app/export_test.go` has no reference. Clean. --- #### e2e regression guard (journey_6_wfengine_test.go) The new `It` correctly: 1. POSTs to `/settings/match-weights/recalculate` and captures `instance_id`. 2. Polls `/admin/wf-status/{id}` until `state == "completed"` (30 s deadline). 3. GETs `/admin/workflows/{id}` with `Accept: application/json` and asserts `Result == "null"`. The `"null"` assertion is sound: a `nil`-error workflow return is serialized by go-workflows as JSON `null` bytes; `prettyPayload` on `[]byte("null")` unmarshals to `nil` interface then re-encodes to the string `"null"`. The existing integration tests already assert the same `Equal("null")` and CI is green. Regression-guard property is preserved: any registration mismatch produces a non-null error string in `Result`, making this `It` fail red. Multi-Expect in the e2e `It` is within policy (journey `It` relaxation). --- #### Unit tests (workflow_detail_handler_test.go) [MINOR] internal/wfengine/workflow_detail_handler_test.go:130-137 — `"returns 200 OK with Content-Type application/json"` `It` contains two `Expect` calls (status code + header) Unit tests retain strict one-Expect-per-It per project conventions. These should be two separate `It` blocks. Does not affect correctness or CI outcome. [MINOR] internal/wfengine/workflow_detail_handler_test.go:139-151 — `"encodes the WorkflowDetail as JSON with instance_id field"` `It` contains two `Expect` calls (`json.Unmarshal` + `got.InstanceID`) Same rule. The unmarshal success and the field value are two separate assertions. Should be split, or the unmarshal folded into `JustBeforeEach`. All JSON branch cases are covered: 200 (Content-Type, body, no HTML), 404, 500. The `renderer.lastPage` check correctly confirms the HTML path is not taken. Good coverage. --- #### Integration test refactor (engine_integration_test.go) `JustBeforeEach` now drives the workflow start + polling + `GetWorkflowDetail` call, with the two `It` blocks asserting independently. Correctly follows BeforeEach/JustBeforeEach/It separation. The previous version ran the full workflow twice (once per `It`), which was wasteful and produced two concurrent workflow runs. Fixed. --- #### recalc_scores_workflow.go — `_ = err` → `if err != nil { return ... }` Correct. `GetWeightsForRecalc` defensively swallows settings errors internally, but framework-level errors (activity not registered, context deadline) must propagate. The new error path is covered by the new `recalcGetWeightsErrActivities` unit test. --- REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor referenced this pull request from a commit

2026-06-18 16:34:34 +00:00

feat(wfengine): JSON content negotiation on workflow-detail route; drop test-only accessor (bookshelf-m8mh, #614 review)

zombor referenced this pull request from a commit

2026-06-18 16:34:34 +00:00

test(wfengine): split two-Expect Its into one-Expect-per-It (review minor #614)

zombor force-pushed bd-bookshelf-m8mh from 50bd72546d

/ JS Unit Tests (pull_request) Successful in 28s

Details

/ Lint (pull_request) Successful in 2m4s

Details

/ E2E API (pull_request) Successful in 2m19s

Details

/ E2E Browser (pull_request) Successful in 2m39s

Details

/ Integration (pull_request) Successful in 3m27s

Details

/ Test (pull_request) Successful in 3m52s

Details

to e05cf61e18

/ E2E API (pull_request) Successful in 1m54s

Details

/ Lint (pull_request) Successful in 2m4s

Details

/ JS Unit Tests (pull_request) Successful in 44s

Details

/ E2E Browser (pull_request) Successful in 2m49s

Details

/ Integration (pull_request) Successful in 2m51s

Details

/ Test (pull_request) Successful in 3m15s

Details

2026-06-18 16:34:34 +00:00

Compare

zombor merged commit 2d0ac37575 into main

2026-06-18 16:38:03 +00:00

zombor referenced this pull request from a commit

2026-06-18 16:38:09 +00:00

Merge pull request 'test(wfengine): full-wiring recalc regression guard + #583 minors (bookshelf-m8mh)' (#614) from bd-bookshelf-m8mh into main

zombor referenced this pull request

2026-06-19 11:03:11 +00:00

feat(books): JSON content-negotiation for GET /books/bulk/step-edit (bookshelf-7hc1) #636

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project

No assignees

1 participant

Notifications

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!614

Rows
Columns

test(wfengine): full-wiring recalc regression guard + #583 minors (bookshelf-m8mh) #614

Summary

Why e2e tier

Coverage

Test plan

SECURITY REVIEW — PR #614 (bd-bookshelf-m8mh)

Findings

Phase 0: DEMO Verification

Phase 1: Spec Compliance

Phase 2: Code Quality

Phase 0: DEMO Verification

Phase 1: Spec Compliance

Phase 2: Code Quality

Phase 0: DEMO Verification

Phase 1: Spec Compliance

Phase 2: Code Quality

CODE REVIEW: APPROVED

Phase 0: DEMO — N/A (no DEMO block; this is a targeted incremental re-review of code-only changes)

Phase 1 + 2: Diff Review

Content-negotiation (workflow_detail_handler.go)

JSON tags (diag_accessor.go)

App.GetWorkflowDetail removal (internal/app/app.go)

e2e regression guard (journey_6_wfengine_test.go)

Unit tests (workflow_detail_handler_test.go)

Integration test refactor (engine_integration_test.go)

recalc_scores_workflow.go — _ = err → if err != nil { return ... }

recalc_scores_workflow.go — `_ = err` → `if err != nil { return ... }`