chore(wfengine): review-minor sweep — retry comment, doc, test assertions (bookshelf-vguw.6, bookshelf-vguw.18) #611

Merged

zombor merged 2 commits from bd-bookshelf-vguw.6 into main 2026-06-18 16:14:40 +00:00

Conversation 2 Commits 2 Files changed 6 +106 -14

zombor commented 2026-06-18 15:55:20 +00:00

Owner

Copy link

Summary

Non-blocking review minors from two merged PRs (#327 and #339), combined into one tight sweep:

vguw.6 — #327 minors:

• cover_workflow.go: Replace misleading "mirrors old pool policy" comment with the actual go-workflows backoff sequence (30s/60s/120s/240s capped at 5m) and note the minor difference from the old task-pool policy ([0s, 30s, 60s, 120s, 300s]). Chose to fix the comment rather than adjust intervals — the 30s-vs-immediate difference at attempt 1 is not worth the complexity.
• cover_handler.go: Add logger.Warn for the discarded json.Encode error after WriteHeader (previously silently swallowed with _ =). Out-of-scope items (book_id logged before existence, per-bookID idempotency) are tracked in vpu8.
• cover_handler_test.go: Split two-Expect It ("generic message" + "no leak") into two Its; add CoverHandler encode-error Describe with an errorWriter stub to cover the new Warn path.
• engine_integration_test.go: Split four-Expect It (POST cover 202 + instance_id) into a sub-Context with nested JustBeforeEach and three single-Expect Its.

vguw.18 — #339 minors:

• engine.go: Fix package-doc comment Engine.Start/Stop → Engine.StartWorker/Stop to match the actual method name.
• engine_lifecycle_test.go: Add WorkerStarted()==true assertion to the StartWorker success Context; add WorkerStarted()==false assertion to the StartWorker error Context.

Test plan

• make lint (wfengine package): 0 issues
• go test ./internal/wfengine/...: pass
• make coverage: 100% on internal/wfengine, full gate passes

Closes bead bookshelf-vguw.6 and bookshelf-vguw.18 on merge.

## Summary Non-blocking review minors from two merged PRs (#327 and #339), combined into one tight sweep: **vguw.6 — #327 minors:** - `cover_workflow.go`: Replace misleading "mirrors old pool policy" comment with the actual go-workflows backoff sequence (30s/60s/120s/240s capped at 5m) and note the minor difference from the old task-pool policy ([0s, 30s, 60s, 120s, 300s]). Chose to fix the comment rather than adjust intervals — the 30s-vs-immediate difference at attempt 1 is not worth the complexity. - `cover_handler.go`: Add `logger.Warn` for the discarded `json.Encode` error after `WriteHeader` (previously silently swallowed with `_ =`). Out-of-scope items (book_id logged before existence, per-bookID idempotency) are tracked in vpu8. - `cover_handler_test.go`: Split two-Expect `It` ("generic message" + "no leak") into two `It`s; add `CoverHandler encode-error` Describe with an `errorWriter` stub to cover the new Warn path. - `engine_integration_test.go`: Split four-Expect `It` (POST cover 202 + instance_id) into a sub-Context with nested `JustBeforeEach` and three single-Expect `It`s. **vguw.18 — #339 minors:** - `engine.go`: Fix package-doc comment `Engine.Start`/`Stop` → `Engine.StartWorker`/`Stop` to match the actual method name. - `engine_lifecycle_test.go`: Add `WorkerStarted()==true` assertion to the `StartWorker` success `Context`; add `WorkerStarted()==false` assertion to the `StartWorker` error `Context`. ## Test plan - [x] `make lint` (wfengine package): 0 issues - [x] `go test ./internal/wfengine/...`: pass - [x] `make coverage`: 100% on `internal/wfengine`, full gate passes Closes bead bookshelf-vguw.6 and bookshelf-vguw.18 on merge.

zombor added 1 commit 2026-06-18 15:55:20 +00:00

chore(wfengine): review-minor sweep — retry comment, encode warn, test assertions ... 5a3158a7fe

vguw.6 (#327 review minors):
- cover_workflow.go: replace misleading "mirrors old pool" comment with the
  actual go-workflows backoff sequence (30s/60s/120s/240s capped at 5m) and
  note the minor difference from the old task-pool policy
- cover_handler.go: add logger.Warn for discarded json.Encode error after
  WriteHeader (was silently swallowed with _ =)
- cover_handler_test.go: split two-Expect It ("generic message" + "no leak")
  into two Its; add CoverHandler encode-error Describe with errorWriter stub
- engine_integration_test.go: split four-Expect It (POST cover 202 + instance_id)
  into sub-Context with nested JustBeforeEach and three single-Expect Its

vguw.18 (#339 review minors):
- engine.go: fix package-doc comment Engine.Start → Engine.StartWorker (match
  the actual method name)
- engine_lifecycle_test.go: add WorkerStarted()==true assertion to the
  StartWorker-success Context; add WorkerStarted()==false assertion to the
  StartWorker-error Context

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

zombor commented 2026-06-18 16:00:22 +00:00

Author

Owner

Copy link

SECURITY REVIEW — PR #611 (bd-bookshelf-vguw.6)

This is the automated security review. Diff reviewed: origin/main...origin/bd-bookshelf-vguw.6.

Changes in scope: logger.Warn on discarded json.Encode error (cover_handler.go), doc-comment rename (engine.go), retry-schedule comment expansion (cover_workflow.go), and test-assertion additions (cover_handler_test.go, engine_integration_test.go, engine_lifecycle_test.go).

---

New logger.Warn — sensitive-data audit

File: internal/wfengine/cover_handler.go (new lines added in the diff):

logger.Warn("wfengine: cover handler: encode response", "book_id", bookID, "err", err)

Fields logged:

• book_id — an int64 parsed from the URL path value /admin/workflows/_poc/cover/{bookID}. This is a non-sensitive internal integer identifier already visible to the caller in the URL. No PII, no token, no secret.
• err — the error from json.Encoder.Write, which is a network/transport error (e.g. "write failed", "broken pipe"). These are stdlib/OS-level error strings that carry no user data, credentials, or internal path information beyond the transport error itself.
• Message string is a static prefix ("wfengine: cover handler: encode response") — no interpolation.

Conclusion: No sensitive data logged. Logging follows the structured key-value pattern mandated by logging-standard.md (not concatenated into a string). No PII, tokens, or secrets present.

---

Auth / Authorization surface

No changes to auth checks, middleware, or session handling. The endpoint is admin-only (/admin/workflows/_poc/cover/{bookID}); the diff does not alter the route or any access control on it.

---

Input handling

No changes to the strconv.ParseInt validation or the bookID <= 0 guard. The new code runs after the 202 has already been written; there is no new user-controlled value in the log path.

---

Resource bounds / unbounded queries / injection

No new SQL queries, no new fan-out, no new external calls introduced. The diff is purely: error capture → structured log. No injection surface added.

---

Other paths

• engine.go: single comment-word rename (Start → StartWorker) in a package doc. No behavioral change.
• cover_workflow.go: doc-comment expansion explaining the retry schedule arithmetic. No behavioral change; the ActivityOptions values themselves are unchanged.
• Test files: assertion additions only. No production code path changes.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

This is a clean nit sweep. The security surface is minimal: the sole new production line logs two non-sensitive structured fields after a 202 has already been sent. No auth, injection, secret-leakage, or resource-bound concerns.

## SECURITY REVIEW — PR #611 (bd-bookshelf-vguw.6) This is the automated **security** review. Diff reviewed: `origin/main...origin/bd-bookshelf-vguw.6`. Changes in scope: logger.Warn on discarded json.Encode error (`cover_handler.go`), doc-comment rename (`engine.go`), retry-schedule comment expansion (`cover_workflow.go`), and test-assertion additions (`cover_handler_test.go`, `engine_integration_test.go`, `engine_lifecycle_test.go`). --- ### New logger.Warn — sensitive-data audit **File:** `internal/wfengine/cover_handler.go` (new lines added in the diff): ```go logger.Warn("wfengine: cover handler: encode response", "book_id", bookID, "err", err) ``` **Fields logged:** - `book_id` — an `int64` parsed from the URL path value `/admin/workflows/_poc/cover/{bookID}`. This is a non-sensitive internal integer identifier already visible to the caller in the URL. No PII, no token, no secret. - `err` — the error from `json.Encoder.Write`, which is a network/transport error (e.g. "write failed", "broken pipe"). These are stdlib/OS-level error strings that carry no user data, credentials, or internal path information beyond the transport error itself. - Message string is a static prefix (`"wfengine: cover handler: encode response"`) — no interpolation. **Conclusion:** No sensitive data logged. Logging follows the structured key-value pattern mandated by `logging-standard.md` (not concatenated into a string). No PII, tokens, or secrets present. --- ### Auth / Authorization surface No changes to auth checks, middleware, or session handling. The endpoint is admin-only (`/admin/workflows/_poc/cover/{bookID}`); the diff does not alter the route or any access control on it. --- ### Input handling No changes to the `strconv.ParseInt` validation or the `bookID <= 0` guard. The new code runs after the 202 has already been written; there is no new user-controlled value in the log path. --- ### Resource bounds / unbounded queries / injection No new SQL queries, no new fan-out, no new external calls introduced. The diff is purely: error capture → structured log. No injection surface added. --- ### Other paths - `engine.go`: single comment-word rename (`Start` → `StartWorker`) in a package doc. No behavioral change. - `cover_workflow.go`: doc-comment expansion explaining the retry schedule arithmetic. No behavioral change; the `ActivityOptions` values themselves are unchanged. - Test files: assertion additions only. No production code path changes. --- **REVIEW VERDICT: 0 blocker, 0 major, 0 minor** This is a clean nit sweep. The security surface is minimal: the sole new production line logs two non-sensitive structured fields after a 202 has already been sent. No auth, injection, secret-leakage, or resource-bound concerns.

zombor commented 2026-06-18 16:02:08 +00:00

Author

Owner

Copy link

CODE REVIEW: NOT APPROVED

Phase 0: DEMO Verification

No DEMO block is applicable for this review-minor sweep (no new user-facing behaviour). Proceeding directly to code quality.

Phase 1: Spec Compliance

All five stated changes are present in the diff:

• cover_workflow.go: retry comment updated
• cover_handler.go: discarded json.Encode error now logs Warn
• cover_handler_test.go: two-Expect It split + new errorWriter Describe
• engine_integration_test.go: four-Expect It split
• engine.go: package-doc corrected to Engine.StartWorker
• engine_lifecycle_test.go: WorkerStarted() assertions added

Phase 2: Code Quality

---

[BLOCKER] internal/wfengine/cover_workflow.go:15-22 — corrected retry comment shows the wrong delay sequence

The comment was introduced as a "correction" but it is numerically wrong. The go-workflows retry loop in workflow/retries.go:87 computes:

backoffDuration = FirstRetryInterval * BackoffCoefficient^attempt

…where attempt is post-incremented (line 71: attempt++) before the backoff calculation. So on the first retry attempt == 1, not 0. With FirstRetryInterval=30s and BackoffCoefficient=2, the actual sequence is:

Retry #	attempt (post-incr)	Delay	Comment says
1st	1	30s × 2¹ = 60s	30s × 2⁰ = 30s
2nd	2	30s × 2² = 120s	30s × 2¹ = 60s
3rd	3	30s × 2³ = 240s	30s × 2² = 120s
4th	4	30s × 2⁴ = 480s → capped to 300s	30s × 2³ = 240s (comment says capped here)

The comment also says "capped at MaxRetryInterval = 5m on attempt 4+". The cap does first fire on the 4th retry (480s > 300s), so the capping-starts note is coincidentally correct, but the values shown are all wrong by one power of 2.

A wrong "correction" is worse than the original vague comment because it looks authoritative while being incorrect. Either revert to the old non-numeric comment, or fix the sequence to use the 1-indexed attempt values above.

---

[MINOR] internal/wfengine/engine_integration_test.go:263-283 — JustBeforeEach silently discards the JSON decode error

The split It blocks share state set in JustBeforeEach:

_ = json.NewDecoder(resp.Body).Decode(&body)

If encoding fails or the body is empty, body.InstanceID stays zero and the It("returns a non-empty instance_id") test fails with an unhelpful message like Expected '' not to be empty. Prefer Expect(json.NewDecoder(resp.Body).Decode(&body)).To(Succeed()) so a bad body gives a direct failure at the decode step rather than a misleading assertion on the empty field.

---

REVIEW VERDICT: 1 blocker, 0 major, 1 minor

CODE REVIEW: NOT APPROVED ## Phase 0: DEMO Verification No DEMO block is applicable for this review-minor sweep (no new user-facing behaviour). Proceeding directly to code quality. ## Phase 1: Spec Compliance All five stated changes are present in the diff: - `cover_workflow.go`: retry comment updated - `cover_handler.go`: discarded `json.Encode` error now logs `Warn` - `cover_handler_test.go`: two-`Expect` `It` split + new `errorWriter` Describe - `engine_integration_test.go`: four-`Expect` `It` split - `engine.go`: package-doc corrected to `Engine.StartWorker` - `engine_lifecycle_test.go`: `WorkerStarted()` assertions added ## Phase 2: Code Quality --- [BLOCKER] internal/wfengine/cover_workflow.go:15-22 — corrected retry comment shows the wrong delay sequence The comment was introduced as a "correction" but it is numerically wrong. The go-workflows retry loop in `workflow/retries.go:87` computes: ``` backoffDuration = FirstRetryInterval * BackoffCoefficient^attempt ``` …where `attempt` is **post-incremented** (line 71: `attempt++`) before the backoff calculation. So on the first retry `attempt == 1`, not 0. With `FirstRetryInterval=30s` and `BackoffCoefficient=2`, the actual sequence is: | Retry # | attempt (post-incr) | Delay | Comment says | |---------|--------------------|---------|--------------| | 1st | 1 | 30s × 2¹ = **60s** | 30s × 2⁰ = 30s | | 2nd | 2 | 30s × 2² = **120s** | 30s × 2¹ = 60s | | 3rd | 3 | 30s × 2³ = **240s** | 30s × 2² = 120s | | 4th | 4 | 30s × 2⁴ = 480s → capped to **300s** | 30s × 2³ = 240s (comment says capped here) | The comment also says "capped at MaxRetryInterval = 5m on attempt 4+". The cap does first fire on the 4th retry (480s > 300s), so the capping-starts note is coincidentally correct, but the values shown are all wrong by one power of 2. A wrong "correction" is worse than the original vague comment because it looks authoritative while being incorrect. Either revert to the old non-numeric comment, or fix the sequence to use the 1-indexed `attempt` values above. --- [MINOR] internal/wfengine/engine_integration_test.go:263-283 — `JustBeforeEach` silently discards the JSON decode error The split `It` blocks share state set in `JustBeforeEach`: ```go _ = json.NewDecoder(resp.Body).Decode(&body) ``` If encoding fails or the body is empty, `body.InstanceID` stays zero and the `It("returns a non-empty instance_id")` test fails with an unhelpful message like `Expected '' not to be empty`. Prefer `Expect(json.NewDecoder(resp.Body).Decode(&body)).To(Succeed())` so a bad body gives a direct failure at the decode step rather than a misleading assertion on the empty field. --- REVIEW VERDICT: 1 blocker, 0 major, 1 minor

zombor referenced this pull request from a commit 2026-06-18 16:05:36 +00:00

fix(wfengine): correct retry-delay comment + assert decode error (bookshelf-vguw.6, #611 review)

zombor added 1 commit 2026-06-18 16:05:37 +00:00

fix(wfengine): correct retry-delay comment + assert decode error (bookshelf-vguw.6, #611 review) 672fed37e4

zombor referenced this pull request from a commit 2026-06-18 16:10:59 +00:00

fix(wfengine): correct retry-delay comment + assert decode error (bookshelf-vguw.6, #611 review)

zombor force-pushed bd-bookshelf-vguw.6 from 672fed37e4 to 4a5aa8cd79 2026-06-18 16:10:59 +00:00 Compare

zombor merged commit 9bb9b5f9d5 into main 2026-06-18 16:14:40 +00:00

zombor referenced this pull request from a commit 2026-06-18 16:14:41 +00:00

Merge pull request 'chore(wfengine): review-minor sweep — retry comment, doc, test assertions (bookshelf-vguw.6, bookshelf-vguw.18)' (#611) from bd-bookshelf-vguw.6 into main

zombor referenced this pull request 2026-06-30 13:51:27 +00:00

fix(sweep): provider-aware EnrichComicDetail — stop Metron id fetched as ComicVine id (bookshelf-vbtl) #866

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

1 participant

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

No description provided.